Skip to content

Support templatable safe-outputs.staged values and GitHub expressions#41296

Merged
pelikhan merged 10 commits into
mainfrom
copilot/ensure-staged-field-boolean
Jun 24, 2026
Merged

Support templatable safe-outputs.staged values and GitHub expressions#41296
pelikhan merged 10 commits into
mainfrom
copilot/ensure-staged-field-boolean

Conversation

Copilot AI commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

safe-outputs.staged was treated as a plain boolean, which prevented expression-based configuration from flowing cleanly through schema validation, compilation, and the SafeOutput runtime. This change makes staged a templatable boolean end to end so literal booleans and GitHub expressions are both supported consistently.

  • Schema and frontmatter parsing

    • Update SafeOutput schema entries so staged accepts templatable_boolean instead of only boolean.
    • Extend config parsing to accept either literal booleans or GitHub expressions for top-level and per-handler staged.
  • Compiler and runtime config

    • Convert staged-related workflow types from bool to *TemplatableBool.
    • Preserve expression-backed staged values when generating handler config JSON and safe-output env vars.
    • Keep staged resolution logic centralized so literal true, literal false, and expressions are handled consistently.
  • JavaScript SafeOutput handling

    • Update staged-mode detection to treat resolved expression values the same as literal true.
    • Reuse shared truthiness handling across handlers instead of ad hoc checks.
  • Coverage

    • Add schema/compiler tests covering expression-backed staged values.
    • Add JS coverage for staged handling when config is provided as a resolved string value and when an expression is still unresolved.
safe-outputs:
  staged: ${{ inputs.staged }}
  create-issue:
    title: "Example"
    body: "Previewable issue body"
    staged: ${{ github.event_name != 'push' }}

Copilot AI and others added 5 commits June 24, 2026 15:30
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review June 24, 2026 20:19
Copilot AI review requested due to automatic review settings June 24, 2026 20:19
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (485 new lines across schema, compiler, runtime types, and JS handlers) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/41296-templatable-safe-outputs-staged.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next
  1. Review the draft ADR committed to your branch — it was generated from the PR diff and description.
  2. Complete the missing sections — confirm the human deciders, refine the decision rationale, and validate that the listed alternatives match what you actually considered.
  3. Commit the finalized ADR to docs/adr/ on your branch.
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-41296: Make safe-outputs.staged a Templatable Boolean

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.

📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🔒 Blocking: link the completed ADR in the PR body to clear this gate.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · 93.8 AIC · ⌖ 10.4 AIC · ⊞ 11.6K ·

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates safe-outputs.staged to be a templatable boolean end-to-end, so users can configure staged mode using either literal booleans (true/false) or GitHub Actions expressions (${{ ... }}), and have that flow consistently through schema validation, compilation, and the SafeOutput runtime.

Changes:

  • Expand the workflow schema to accept templatable_boolean for top-level and per-handler safe-outputs.*.staged, plus add schema validation coverage.
  • Convert staged configuration plumbing from bool to *TemplatableBool across compiler/workflow types and env/config generation, preserving expression-backed values.
  • Update JavaScript staged detection to treat resolved string values ("true") equivalently to boolean true, and add JS coverage.
Show a summary per file
File Description
pkg/workflow/test_helpers_shared_test.go Adds a shared test helper for *TemplatableBool values.
pkg/workflow/templatables.go Implements YAML/JSON unmarshaling for TemplatableBool and adds staged-resolution helpers.
pkg/workflow/staged_test.go Updates staged-mode unit tests for pointer-backed staged config.
pkg/workflow/staged_awinfo_test.go Updates aw-info generation tests for pointer-backed staged config.
pkg/workflow/safe_outputs_test.go Updates env var tests to use *TemplatableBool for staged.
pkg/workflow/safe_outputs_runtime.go Updates runtime staged checks to use TemplatableBool helpers.
pkg/workflow/safe_outputs_permissions.go Updates staged permission gating for pointer-backed staged values.
pkg/workflow/safe_outputs_permissions_test.go Updates permission tests to the new staged representation.
pkg/workflow/safe_outputs_import_test.go Updates import tests to treat staged as templatable/pointer-backed.
pkg/workflow/safe_outputs_handler_registry.go Switches handler config emission to AddTemplatableBool for staged.
pkg/workflow/safe_outputs_env.go Preserves staged expressions when emitting job env vars and map-based env.
pkg/workflow/safe_outputs_config.go Extends config parsing to accept bools/expressions for top-level and per-handler staged.
pkg/workflow/safe_outputs_config_helpers_test.go Updates helper tests for staged pointer types.
pkg/workflow/safe_outputs_config_generation_test.go Updates config-generation tests for staged pointer types.
pkg/workflow/safe_outputs_allow_workflows_test.go Updates allow-workflows permission test for staged pointer type.
pkg/workflow/safe_output_helpers_test.go Updates env helper tests for staged pointer type and staged propagation.
pkg/workflow/safe_output_handlers.go Updates staged reflection helpers to return *TemplatableBool for handlers.
pkg/workflow/imports.go Updates safe-outputs merge logic for pointer-backed staged config.
pkg/workflow/frontmatter_types_test.go Updates type-safety test for staged pointer type.
pkg/workflow/dispatch_repository.go Updates dispatch-repository tool config to use *TemplatableBool staged.
pkg/workflow/compiler_yaml.go Propagates staged value into aw-info as a string (including expressions).
pkg/workflow/compiler_types.go Changes SafeOutputsConfig.Staged and BaseSafeOutputConfig.Staged to *TemplatableBool.
pkg/workflow/compiler_safe_outputs_job.go Updates staged gating for SARIF outputs and job staged behavior.
pkg/workflow/compiler_safe_outputs_job_test.go Updates job-level env var tests to staged pointer type.
pkg/workflow/compiler_safe_outputs_env_test.go Adds coverage for staged expressions and updates trial-mode/staged behavior expectations.
pkg/workflow/compiler_safe_outputs_config_test.go Adds handler-config JSON coverage for staged expressions and updates existing staged tests.
pkg/workflow/compiler_safe_output_jobs.go Updates staged gating when deciding whether to add the SARIF upload job.
pkg/parser/schemas/main_workflow_schema.json Changes staged fields from boolean to templatable_boolean across safe-outputs.
pkg/parser/schema_test.go Adds schema validation coverage for expression-backed safe-outputs staged.
actions/setup/js/upload_artifact.cjs Uses shared templatable truthiness for staged detection.
actions/setup/js/submit_pr_review.cjs Treats "true" as staged when propagating staged state to the review buffer.
actions/setup/js/safe_output_helpers.cjs Adds isTemplatableTrue and updates isStagedMode to accept "true".
actions/setup/js/handler_scaffold.test.cjs Adds coverage for staged truthiness with "true" and unresolved expressions.
actions/setup/js/create_pr_review_comment.cjs Treats "true" as staged when propagating staged state to the review buffer.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 35/35 changed files
  • Comments generated: 4

Comment on lines 420 to 428
// Handle staged flag
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
safeOutputsConfigLog.Printf("staged: %v", err)
}
if staged, exists := outputMap["staged"]; exists {
if stagedBool, ok := staged.(bool); ok {
config.Staged = stagedBool
if stagedStr, ok := staged.(string); ok && stagedStr != "" {
value := TemplatableBool(stagedStr)
config.Staged = &value
}
Comment thread pkg/workflow/safe_outputs_env.go Outdated
Comment on lines +279 to +283
// Add the global staged env var once if staged mode is enabled, not in trial mode,
// and at least one handler is configured. Staged mode is independent of target-repo.
if !c.trialMode && data.SafeOutputs.Staged && hasAnySafeOutputEnabled(data.SafeOutputs) {
*steps = append(*steps, " GH_AW_SAFE_OUTPUTS_STAGED: \"true\"\n")
if hasAnySafeOutputEnabled(data.SafeOutputs) {
if value := resolveSafeOutputsStagedValue(c.trialMode, data.SafeOutputs.Staged); value != nil {
*steps = append(*steps, buildTemplatableBoolEnvVar("GH_AW_SAFE_OUTPUTS_STAGED", value)...)
Comment on lines 10 to 21
// DispatchRepositoryToolConfig defines a single repository dispatch tool within dispatch_repository
type DispatchRepositoryToolConfig struct {
Description string `yaml:"description,omitempty"` // Human-readable description
Workflow string `yaml:"workflow"` // Target workflow name (for traceability and payload)
EventType string `yaml:"event_type"` // repository_dispatch event_type
Repository string `yaml:"repository,omitempty"` // Single target repository (owner/repo)
AllowedRepositories []string `yaml:"allowed_repositories,omitempty"` // Multiple allowed target repositories
Inputs map[string]any `yaml:"inputs,omitempty"` // Input schema (similar to workflow_dispatch inputs)
Max *string `yaml:"max,omitempty"` // Max dispatch executions (templatable int)
GitHubToken string `yaml:"github-token,omitempty"` // Optional override token
Staged bool `yaml:"staged,omitempty"` // If true, preview-only mode
Description string `yaml:"description,omitempty"` // Human-readable description
Workflow string `yaml:"workflow"` // Target workflow name (for traceability and payload)
EventType string `yaml:"event_type"` // repository_dispatch event_type
Repository string `yaml:"repository,omitempty"` // Single target repository (owner/repo)
AllowedRepositories []string `yaml:"allowed_repositories,omitempty"` // Multiple allowed target repositories
Inputs map[string]any `yaml:"inputs,omitempty"` // Input schema (similar to workflow_dispatch inputs)
Max *string `yaml:"max,omitempty"` // Max dispatch executions (templatable int)
GitHubToken string `yaml:"github-token,omitempty"` // Optional override token
Staged *TemplatableBool `yaml:"staged,omitempty"` // Templatable preview-only mode
}
Comment thread pkg/parser/schema_test.go
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 94/100 — Excellent

Analyzed 5 test(s): 5 design, 0 implementation, 0 guideline violations (1 minor non-blocking flag).

📊 Metrics & Test Classification (5 tests analyzed)
Metric Value
New/modified tests analyzed 5
✅ Design tests (behavioral contracts) 5 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 4 (80%)
Duplicate test clusters 0
Test inflation detected No
🚨 Coding-guideline violations 0 (Go mock libraries / missing build tags)
Test File Classification Issues Detected
TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_SafeOutputsStagedExpressionAccepted pkg/parser/schema_test.go:590 ✅ Design
TestHandlerConfigStagedExpression pkg/workflow/compiler_safe_outputs_config_test.go:2560 ✅ Design ⚠️ One bare assert.Equal missing message arg (line 2595)
"create issues with staged expression" (table row in TestAddAllSafeOutputConfigEnvVars) pkg/workflow/compiler_safe_outputs_env_test.go:48 ✅ Design
should pass isStaged as true when config.staged is the string "true" actions/setup/js/handler_scaffold.test.cjs:78 ✅ Design
should treat unresolved staged expressions as false until runtime resolves them actions/setup/js/handler_scaffold.test.cjs:92 ✅ Design

Go: 3 (*_test.go); JavaScript: 2 (*.test.cjs). Other languages detected but not scored.

Note on refactoring changes: The majority of diff lines across the 15 other test files are mechanical type-refactor updates (Staged: trueStaged: templatableBoolPtr("true")) adapting to the new TemplatableBool type. These update existing test scaffolding, not new test logic, and are not counted as new tests.

⚠️ Flagged Tests — Non-blocking (1 issue)

TestHandlerConfigStagedExpression (pkg/workflow/compiler_safe_outputs_config_test.go:2595) — ⚠️ Minor: assert.Equal(t, "${{ inputs.staged }}", handlerConfig["staged"]) is missing a descriptive message argument. Guidelines require a context string so failures are self-explanatory. Suggested fix: add "staged expression should pass through to handler config JSON unchanged" as a third argument.

This is a non-blocking advisory and does not trigger a review rejection.

Verdict

Check passed. 0% implementation tests (threshold: 30%). All 5 new tests verify observable behavioral contracts: schema acceptance of template expressions, correct env-var serialization, and isStaged coercion logic at both the Go compiler and JS runtime layers.

🧪 Test quality analysis by Test Quality Sentinel · 104.6 AIC · ⌖ 21.1 AIC · ⊞ 8.4K ·

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 94/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd, /zoom-out, and /grill-with-docs — commenting with minor improvement suggestions, no blocking issues.

📋 Key Themes & Highlights

Key Themes

  • Consistency gap in upload_artifact.cjs: the only handler that assembles staged mode detection manually instead of delegating to isStagedMode(config) — future maintenance risk.
  • Expression-backed per-handler staged is silently false at compile time: isHandlerStaged treats ${{ inputs.staged }} as not-staged, so write permissions and checkout steps are always compiled in for expression-backed per-handler staged fields. This is likely intentional but isn't documented.
  • Schema examples not updated: all 22 staged entries still show [true, false] — the headline new capability (expressions) is absent from the IDE autocomplete hints.
  • Minor test gaps: TestHandlerConfigStagedExpression is missing t.Parallel(); no test for staged: "false" (string) in the JS suite; global staged parsing has an if/if where the per-handler code uses if/else if.

Positive Highlights

  • ✅ End-to-end coverage: schema → parser → Go compiler types → env var generation → JS runtime, all with tests at each layer.
  • TemplatableBool.UnmarshalJSON/UnmarshalYAML both reject arbitrary strings (only expressions allowed) with clear error messages.
  • isTemplatableTrue nicely centralises the JS truthiness check; the JS test for the unresolved-expression-is-false contract is a great explicit specification.
  • resolveSafeOutputsStagedValue / buildTemplatableBoolEnvVar cleanly separate the compile-time env-var generation concern from the type definition.
  • templatableBoolPtrToStringPtr used consistently across all 48 handler-registry entries — no ad-hoc conversions.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 127.2 AIC · ⌖ 9.88 AIC · ⊞ 6.5K

Comment thread actions/setup/js/upload_artifact.cjs Outdated
const filtersInclude = Array.isArray(config["filters-include"]) ? config["filters-include"] : [];
const filtersExclude = Array.isArray(config["filters-exclude"]) ? config["filters-exclude"] : [];
const isStaged = config["staged"] === true || process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";
const isStaged = isTemplatableTrue(config["staged"]) || process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] isStaged is computed inline here rather than delegating to isStagedMode(config), making upload_artifact the only handler in the codebase that assembles the staged check manually.

If isStagedMode is ever extended (e.g. to add a new resolution path), upload_artifact will silently diverge.

💡 Suggested fix
const { isTemplatableTrue, isStagedMode } = require('./safe_output_helpers.cjs');
// ...
const isStaged = isStagedMode(config);

isStagedMode already encapsulates both the env-var check and isTemplatableTrue(config.staged) — a one-line replacement with no behaviour change.

func isHandlerStaged(globalStaged, handlerStaged bool) bool {
return globalStaged || handlerStaged
func isHandlerStaged(globalStaged bool, handlerStaged *TemplatableBool) bool {
return globalStaged || templatableBoolIsTrue(handlerStaged)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/grill-with-docs] isHandlerStaged returns false when handlerStaged is an expression (e.g. ${{ inputs.staged }}) because templatableBoolIsTrue only matches the literal string "true".

This is a compile-time/runtime split: the compiler can't evaluate expressions, so it treats expression-backed staged as not

"description": "When true, emit step summary messages instead of making GitHub API calls for this specific output type (preview mode)",
"examples": [true, false]
},
"samples": {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/grill-with-docs] The examples field still only shows [true, false] across all 22 staged entries. Now that GitHub Actions expressions are the headline new feature, the examples should demonstrate the primary new use-case.

💡 Suggested change (apply to all staged entries)
"examples": [true, false, "${{ inputs.staged }}"]

This is purely documentation/schema tooling — it doesn't affect validation — but it's the first thing users see when their IDE autocompletes the field and will prevent confusion about whether expressions are actually accepted.

}
}

func TestHandlerConfigStagedExpression(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] TestHandlerConfigStagedExpression is missing t.Parallel(). Every other top-level test function in this file calls t.Parallel() as its first line — this one doesn't, which will cause it to run serially and slow down the test suite over time.

💡 Fix
func TestHandlerConfigStagedExpression(t *testing.T) {
	t.Parallel()
	// ...
}

await factory(config);

expect(setupSpy).toHaveBeenCalledWith(config, 5, false);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new tests cover staged: "true" (resolves to true) and staged: "${{ inputs.staged }}" (unresolved, stays false) — but not staged: "false" (explicitly disabled as a resolved string). Given that isTemplatableTrue is the central safety gate for staged mode, a test asserting isTemplatableTrue("false") === false would make the contract complete and guard against future string-coercion mistakes.

💡 Suggested test case
it('should pass isStaged as false when config.staged is the string "false"', async () => {
  const setupSpy = vi.fn().mockResolvedValue(async () => ({ success: true }));
  const factory = createCountGatedHandler({
    handlerType: 'test_handler',
    setup: setupSpy,
  });
  const config = { max: 5, staged: 'false' };
  await factory(config);
  expect(setupSpy).toHaveBeenCalledWith(config, 5, false);
});

Comment thread pkg/workflow/safe_outputs_config.go Outdated
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
safeOutputsConfigLog.Printf("staged: %v", err)
}
if staged, exists := outputMap["staged"]; exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Global staged parsing uses a sequential if here (not else if), so if preprocessBoolFieldAsString returns an error the code still enters the if staged, exists := outputMap["staged"] block and silently fails the type assertion. The per-handler equivalent at line 844 correctly uses else if, making it impossible to reach the parse step on failure.

The current code has no bug (the type-assert fails gracefully), but the inconsistency makes the intent hard to read and would mask a future scenario where preprocess partially modifies the map.

💡 Suggested fix
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
    safeOutputsConfigLog.Printf("staged: %v", err)
} else if staged, exists := outputMap["staged"]; exists {
    if stagedStr, ok := staged.(string); ok && stagedStr != "" {
        value := TemplatableBool(stagedStr)
        config.Staged = &value
    }
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES — Three bugs must be fixed before merge; two others are medium-priority design issues.

### Blocking issues (3)
  1. extractSafeOutputsConfig missing else if guard (safe_outputs_config.go:424) — when preprocessBoolFieldAsString rejects an invalid staged string it logs the error, but the immediately-following if block still reads the unchanged (invalid) map entry and creates a TemplatableBool from it. Per-handler parsing already does this correctly with else if on line 846; the top-level path must match.

  2. link_sub_issue.allowed_repos lost expression support (safe_outputs_handler_registry.go:372) — AddTemplatableStringSlice was changed to AddStringSlice with no justification. This reverts pre-existing expression support for that field and is unrelated to the staged feature.

  3. "Added staged flag" log fires for every safe-output workflow (safe_outputs_env.go:284) — the log was moved outside the value != nil guard; it now fires even when no staged env var was emitted. The comment above the block still says "not in trial mode" which is no longer true.

### Medium-priority issues (2)
  1. GH_AW_INFO_STAGED carries unresolved expression strings (compiler_yaml.go:844) — gh aw audit and any awinfo consumer that treats this field as a boolean will misparse when the value is ${{ inputs.staged }}.

  2. Manifest upload always included for expression-backed staged (compiler_safe_outputs_job.go:514) — compile-time always takes the not-staged path for expressions; at runtime if the expression resolves to true, the manifest upload step runs against empty output. This needs either runtime-conditional steps or an explicit documented trade-off comment.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🔎 Code quality review by PR Code Quality Reviewer · 198.1 AIC · ⌖ 9.8 AIC · ⊞ 5.2K

Comment thread pkg/workflow/safe_outputs_config.go Outdated
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
safeOutputsConfigLog.Printf("staged: %v", err)
}
if staged, exists := outputMap["staged"]; exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid staged values are silently accepted when top-level validation fails: extractSafeOutputsConfig uses two independent if blocks, so a rejected staged string still creates a TemplatableBool — unlike parseBaseSafeOutputConfig which correctly uses else if.

💡 Suggested fix

Change the two separate if blocks to else if, matching the correct pattern on line 846:

// Current (broken):
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
    safeOutputsConfigLog.Printf("staged: %v", err)
}
if staged, exists := outputMap["staged"]; exists { // runs even after error
    if stagedStr, ok := staged.(string); ok && stagedStr != "" {
        value := TemplatableBool(stagedStr)  // TemplatableBool("notanexpr") stored silently
        config.Staged = &value
    }
}

// Fixed:
if err := preprocessBoolFieldAsString(outputMap, "staged", safeOutputsConfigLog); err != nil {
    safeOutputsConfigLog.Printf("staged: %v", err)
} else if staged, exists := outputMap["staged"]; exists {
    if stagedStr, ok := staged.(string); ok && stagedStr != "" {
        value := TemplatableBool(stagedStr)
        config.Staged = &value
    }
}

preprocessBoolFieldAsString does not delete the key on failure, so the invalid string remains in outputMap. The second if then picks it up and creates an invalid TemplatableBool despite the error being logged. Per-handler parsing already does this correctly — this top-level path needs the same guard.

@@ -372,7 +372,7 @@ var handlerRegistry = map[string]handlerBuilder{
AddIfNotEmpty("target-repo", c.TargetRepoSlug).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintentional regression: link_sub_issue.allowed_repos loses expression support: AddTemplatableStringSlice was replaced with AddStringSlice, removing the ability to use a GitHub Actions expression for this field. This is unrelated to the staged feature and was present in main before this PR.

💡 Suggested fix

Restore the original call:

// Restore the previous behaviour:
AddTemplatableStringSlice("allowed_repos", c.AllowedRepos).

Every other handler in this file uses AddStringSlice for allowed_repos. link_sub_issue was the only handler that also accepted a GitHub expression for this field (via AddTemplatableStringSlice). That capability is now gone without any explanation in the commit. Any workflow relying on allowed_repos: ${{ inputs.repos }} in a link_sub_issue handler will silently get a serialised []string with the expression literal instead of the string the runtime needs to evaluate.

if hasAnySafeOutputEnabled(data.SafeOutputs) {
if value := resolveSafeOutputsStagedValue(c.trialMode, data.SafeOutputs.Staged); value != nil {
*steps = append(*steps, buildTemplatableBoolEnvVar("GH_AW_SAFE_OUTPUTS_STAGED", value)...)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spurious log and stale comment: safeOutputsEnvLog.Print("Added staged flag") now fires for every workflow that has any safe output enabled, regardless of whether staged mode was actually set or the env var was emitted.

💡 Suggested fix

Move the log inside the value != nil guard, and update the stale comment above:

// Add the global staged env var once if staged mode is enabled
// (trial mode included) and at least one handler is configured.
if hasAnySafeOutputEnabled(data.SafeOutputs) {
    if value := resolveSafeOutputsStagedValue(c.trialMode, data.SafeOutputs.Staged); value != nil {
        *steps = append(*steps, buildTemplatableBoolEnvVar("GH_AW_SAFE_OUTPUTS_STAGED", value)...)
        safeOutputsEnvLog.Print("Added staged flag")  // moved inside
    }
}

The comment still says "not in trial mode" (from the old guard !c.trialMode && data.SafeOutputs.Staged) but that condition was removed. As written, the log misleads anyone tracing why GH_AW_SAFE_OUTPUTS_STAGED is missing from a compiled workflow — it will appear in logs even when nothing was added.

if data.SafeOutputs != nil && data.SafeOutputs.Staged {
stagedValue = "true"
if data.SafeOutputs != nil && data.SafeOutputs.Staged != nil {
stagedValue = data.SafeOutputs.Staged.String()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GH_AW_INFO_STAGED now carries raw unresolved expressions: stagedValue is set to data.SafeOutputs.Staged.String() unconditionally, so the awinfo YAML can now contain ${{ inputs.staged }} as a literal string instead of a resolved boolean.

💡 Why this matters

The GH_AW_INFO_STAGED env var in awinfo is used by gh aw audit and any tool that reads workflow metadata to understand whether the workflow is staged. Those consumers expect "true" or "false" — not a raw GitHub Actions expression string. An unresolved expression there breaks machine-readable audit output and any conditional logic that parses the field.

If the intent is for awinfo to reflect the source-level config (expression preserved), the field semantics must be documented and any consumers that interpret it as a boolean must be updated to handle the expression case. If the intent is to retain resolved-boolean semantics, only emit the string when it is a literal value:

stagedValue := "false"
if data.SafeOutputs != nil && templatableBoolIsTrue(data.SafeOutputs.Staged) {
    stagedValue = "true"
}
// For expression-backed staged, the runtime value is only known at execution time;
// leave stagedValue as "false" (or emit the expression and update consumers).

// is always available for the audit command to display.
// In staged mode, no items are actually created in GitHub so there is nothing to record.
isStaged := c.trialMode || data.SafeOutputs.Staged
isStaged := c.trialMode || templatableBoolIsTrue(data.SafeOutputs.Staged)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manifest upload step is unconditionally included when staged is an expression: templatableBoolIsTrue returns false for any expression, so isStaged is always false at compile time and the manifest upload step is always emitted — even when staged: ${{ inputs.staged }} evaluates to true at runtime.

💡 Impact and options

When staged: ${{ inputs.staged }} resolves to true at runtime, safe-output handlers produce no real GitHub API calls, but the manifest upload step still runs and may upload an empty or inconsistent manifest. The same logic applies to the SARIF step on line 325.

Option A — Accept as a known limitation and document it: expression-backed staged is conservative at compile time (always emits all steps); runtime behavior is correct but may upload an empty manifest when staged.

Option B — Wrap the manifest upload in a runtime conditional:

- name: Upload safe output items manifest
  if: ${{ inputs.staged != 'true' }}

Option B requires buildSafeOutputItemsManifestUploadStep to accept a conditional expression, which is a larger change. Option A is acceptable if documented, but currently there is no comment or test acknowledging this trade-off, leaving it as a silent footgun for users who discover the empty manifest upload.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI and others added 3 commits June 24, 2026 21:11
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Done. I ran the PR finisher pass, addressed the staged follow-up issues, and pushed the fixes in commit 83bb423.

@pelikhan

Copy link
Copy Markdown
Collaborator

Copilot AI commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Fix https://github.com/github/gh-aw/actions/runs/28130268158/job/83305343436

I investigated that run/job.

The failure in https://github.com/github/gh-aw/actions/runs/28130268158/job/83305343436 is from the PR merge ref at that time (base commit 2550a11...), and it fails in actions/setup/js/parse_codex_log.cjs with TS nullability errors unrelated to this PR’s staged changes.

Current PR head 83bb423 already has the fixed parse_codex_log.cjs content (same blob as current main), and local required checks pass (make fmt, make lint, make test-unit, make test, plus actions/setup/js npm run typecheck).

So there was no additional patch to apply in this branch; CI needs a fresh maintainer-triggered run on the current merge ref.

@github-actions

Copy link
Copy Markdown
Contributor

``
Please fix the failing action job linked above (https://github.com/github/gh-aw/actions/runs/28130268158/job/83305343436) and re-run the checks.

Generated by 👨‍🍳 PR Sous Chef · 44.9 AIC · ⌖ 1 AIC · ⊞ 17.6K ·

@pelikhan pelikhan merged commit 88dcb3b into main Jun 24, 2026
37 of 39 checks passed
@pelikhan pelikhan deleted the copilot/ensure-staged-field-boolean branch June 24, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants