From 366ab3ed795bb702b422bc95dffe33dc54f3eb61 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Wed, 15 Apr 2026 12:34:25 +1000 Subject: [PATCH 1/2] fix: rename --approve-updates to --approve and skip safe update enforcement on first compile - Rename --approve-updates flag to --approve in compile, run, and upgrade commands - Change first-compile behavior: skip enforcement when no prior manifest exists instead of flagging all new secrets/actions (baseline is created silently) - Update remediation message to reference --approve instead of --approve-updates - Update all tests to reflect new first-compile behavior --- cmd/gh-aw/main.go | 12 ++-- pkg/cli/compile_compiler_setup.go | 2 +- .../compile_safe_update_integration_test.go | 70 ++++++++----------- pkg/cli/upgrade_command.go | 8 +-- pkg/workflow/compiler_types.go | 2 +- pkg/workflow/compiler_yaml.go | 2 +- pkg/workflow/safe_update_enforcement.go | 21 +++--- pkg/workflow/safe_update_enforcement_test.go | 12 ++-- 8 files changed, 58 insertions(+), 71 deletions(-) diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index 51c420bd5aa..0e3c4aa6e32 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -233,7 +233,7 @@ Examples: var compileCmd = &cobra.Command{ Use: "compile [workflow]...", Short: "Compile agentic workflow Markdown files into GitHub Actions YAML", - Long: `Compile one or more agentic workflow Markdown files into GitHub Actions YAML. + Long: `Compile one or more agentic workflows to YAML workflows. If no workflows are specified, all Markdown files in .github/workflows will be compiled. @@ -286,7 +286,7 @@ Examples: failFast, _ := cmd.Flags().GetBool("fail-fast") noCheckUpdate, _ := cmd.Flags().GetBool("no-check-update") scheduleSeed, _ := cmd.Flags().GetString("schedule-seed") - approve, _ := cmd.Flags().GetBool("approve-updates") + approve, _ := cmd.Flags().GetBool("approve") validateImages, _ := cmd.Flags().GetBool("validate-images") priorManifestFile, _ := cmd.Flags().GetString("prior-manifest-file") verbose, _ := cmd.Flags().GetBool("verbose") @@ -399,7 +399,7 @@ Examples: push, _ := cmd.Flags().GetBool("push") dryRun, _ := cmd.Flags().GetBool("dry-run") jsonOutput, _ := cmd.Flags().GetBool("json") - approveRun, _ := cmd.Flags().GetBool("approve-updates") + approveRun, _ := cmd.Flags().GetBool("approve") if err := validateEngine(engineOverride); err != nil { return err @@ -694,7 +694,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all compileCmd.Flags().Bool("fail-fast", false, "Stop at the first validation error instead of collecting all errors") compileCmd.Flags().Bool("no-check-update", false, "Skip checking for gh-aw updates") compileCmd.Flags().String("schedule-seed", "", "Override the repository slug (owner/repo) used as seed for fuzzy schedule scattering (e.g. 'github/gh-aw'). Bypasses git remote detection entirely. Use this when your git remote is not named 'origin' and you have multiple remotes configured") - compileCmd.Flags().Bool("approve-updates", false, "Approve all safe update changes. When strict mode is active (the default), the compiler emits warnings for new restricted secrets or unapproved action additions/removals not present in the existing gh-aw-manifest. Use this flag to approve and skip safe update enforcement") + compileCmd.Flags().Bool("approve", false, "Approve all safe update changes. When strict mode is active (the default), the compiler emits warnings for new restricted secrets or unapproved action additions/removals not present in the existing gh-aw-manifest. Use this flag to approve and skip safe update enforcement") compileCmd.Flags().Bool("validate-images", false, "Require Docker to be available for container image validation. Without this flag, container image validation is silently skipped when Docker is not installed or the daemon is not running") compileCmd.Flags().String("prior-manifest-file", "", "Path to a JSON file containing pre-cached gh-aw-manifests (map[lockFile]*GHAWManifest); used by the MCP server to supply a tamper-proof manifest baseline captured at startup") if err := compileCmd.Flags().MarkHidden("prior-manifest-file"); err != nil { @@ -729,13 +729,13 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all runCmd.Flags().Bool("enable-if-needed", false, "Enable the workflow before running if needed, and restore state afterward") runCmd.Flags().StringP("engine", "e", "", "Override AI engine (claude, codex, copilot, custom)") runCmd.Flags().StringP("repo", "r", "", "Target repository ([HOST/]owner/repo format). Defaults to current repository") - runCmd.Flags().String("ref", "", "Branch or tag name to run the workflow on (e.g., main, v1.0.0)") + runCmd.Flags().String("ref", "", "Branch or tag name to run the workflow on (default: current branch)") runCmd.Flags().Bool("auto-merge-prs", false, "Auto-merge any pull requests created during the workflow execution") runCmd.Flags().StringArrayP("raw-field", "F", []string{}, "Add a string parameter in key=value format (can be used multiple times)") runCmd.Flags().Bool("push", false, "Commit and push workflow files (including transitive imports) before running") runCmd.Flags().Bool("dry-run", false, "Validate workflow without actually triggering execution on GitHub Actions") runCmd.Flags().BoolP("json", "j", false, "Output results in JSON format") - runCmd.Flags().Bool("approve-updates", false, "Approve all safe update changes during compilation (skip safe update enforcement)") + runCmd.Flags().Bool("approve", false, "Approve all safe update changes during compilation (skip safe update enforcement)") // Register completions for run command runCmd.ValidArgsFunction = cli.CompleteWorkflowNames cli.RegisterEngineFlagCompletion(runCmd) diff --git a/pkg/cli/compile_compiler_setup.go b/pkg/cli/compile_compiler_setup.go index 308a2aac77a..a8c35be81d3 100644 --- a/pkg/cli/compile_compiler_setup.go +++ b/pkg/cli/compile_compiler_setup.go @@ -160,7 +160,7 @@ func configureCompilerFlags(compiler *workflow.Compiler, config CompileConfig) { // regardless of the workflow's strict mode setting. compiler.SetApprove(config.Approve) if config.Approve { - compileCompilerSetupLog.Print("Safe update changes approved via --approve-updates flag: skipping safe update enforcement for new restricted secrets or unapproved action additions/removals") + compileCompilerSetupLog.Print("Safe update changes approved via --approve flag: skipping safe update enforcement for new restricted secrets or unapproved action additions/removals") } // Set require docker flag: when set, container image validation fails instead of diff --git a/pkg/cli/compile_safe_update_integration_test.go b/pkg/cli/compile_safe_update_integration_test.go index 6139754d27f..b178f1171c4 100644 --- a/pkg/cli/compile_safe_update_integration_test.go +++ b/pkg/cli/compile_safe_update_integration_test.go @@ -91,10 +91,8 @@ func manifestLockFileWithSecret(secretName string) string { } // TestSafeUpdateFirstCompileCreatesBaseline verifies that the first compilation -// (with no prior manifest) still enforces safe update mode and emits a -// SECURITY REVIEW REQUIRED warning so agents review newly introduced secrets. -// The compile itself succeeds (warnings do not fail the build) and the lock file -// written with the manifest serves as the baseline for future compilations. +// (with no prior manifest) creates the manifest baseline silently without any +// safe update warnings. Enforcement only kicks in on subsequent compilations. func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -103,17 +101,17 @@ func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithSecret), 0o644), "should write workflow file") - // First compile with no prior lock file: should succeed but emit safe update - // warnings because the agent must review newly introduced secrets. + // First compile with no prior lock file: should succeed without safe update warnings + // because there is no prior manifest to compare against. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) - assert.NoError(t, err, "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) - // Safe update warning must be emitted even on first compile so the agent reviews secrets. - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should emit safe update warnings so the agent reviews newly introduced secrets") + assert.NoError(t, err, "first compile should succeed\nOutput:\n%s", outputStr) + // No safe update warnings on first compile (no prior manifest to compare against) + assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should not emit safe update warnings when no prior manifest exists") // Lock file must be written with the manifest baseline lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-secret.lock.yml") lockContent, readErr := os.ReadFile(lockFilePath) @@ -126,9 +124,8 @@ func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { } // TestSafeUpdateFirstCompileCreatesBaselineForActions verifies that the first -// compilation with a custom action and no prior manifest still enforces safe -// update mode, emitting a SECURITY REVIEW REQUIRED warning. The compile succeeds -// (warnings do not fail the build) and the new lock file serves as the baseline. +// compilation with a custom action and no prior manifest creates the baseline +// silently without safe update warnings. func TestSafeUpdateFirstCompileCreatesBaselineForActions(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -137,21 +134,20 @@ func TestSafeUpdateFirstCompileCreatesBaselineForActions(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithCustomAction), 0o644), "should write workflow file") - // First compile with no prior lock file: should succeed but emit safe update - // warning so the agent reviews the newly introduced custom action. + // First compile with no prior lock file: should succeed without safe update warnings. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) - assert.NoError(t, err, "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should emit safe update warnings so the agent reviews newly introduced actions") + assert.NoError(t, err, "first compile should succeed\nOutput:\n%s", outputStr) + assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should not emit safe update warnings when no prior manifest exists") // Lock file must be written lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-action.lock.yml") _, statErr := os.Stat(lockFilePath) assert.NoError(t, statErr, "lock file should be written after first compile") - t.Logf("First compile correctly emitted warnings for new action.\nOutput:\n%s", outputStr) + t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) } // TestSafeUpdateAllowsKnownSecretWithPriorManifest verifies that safe update @@ -400,7 +396,7 @@ func TestSafeUpdateManifestIncludesImportedSecret(t *testing.T) { "should write workflow file") // Compile with --approve so we can inspect the manifest freely without safe update warnings. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve-updates") + cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve") cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) @@ -420,10 +416,8 @@ func TestSafeUpdateManifestIncludesImportedSecret(t *testing.T) { } // TestSafeUpdateFirstCompileCreatesBaselineForImport verifies that the first compilation -// of a workflow that imports a shared config containing a secret emits a -// SECURITY REVIEW REQUIRED warning so the agent reviews newly introduced secrets. -// The compile succeeds (warnings don't fail the build) and the lock file written -// serves as the baseline for future compilations. +// of a workflow that imports a shared config containing a secret creates the baseline +// manifest silently without safe update warnings. func TestSafeUpdateFirstCompileCreatesBaselineForImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -434,17 +428,17 @@ func TestSafeUpdateFirstCompileCreatesBaselineForImport(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithImport), 0o644), "should write workflow file") - // No prior lock file — first compile enforces safe update and emits a warning. + // No prior lock file — first compile creates baseline silently. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) assert.NoError(t, err, - "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should emit safe update warnings so the agent reviews newly introduced secrets") - t.Logf("First compile correctly emitted warnings for imported secret.\nOutput:\n%s", outputStr) + "first compile should succeed\nOutput:\n%s", outputStr) + assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should not emit safe update warnings when no prior manifest exists") + t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) } // TestSafeUpdateAllowsImportedSecretWithPriorManifest verifies that safe update @@ -509,7 +503,7 @@ func TestSafeUpdateManifestIncludesTransitivelyImportedSecret(t *testing.T) { "should write workflow file") // Compile with --approve so we can freely inspect the manifest without safe update warnings. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve-updates") + cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve") cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) @@ -528,11 +522,7 @@ func TestSafeUpdateManifestIncludesTransitivelyImportedSecret(t *testing.T) { } } -// TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport verifies that -// the first compilation of a workflow with a transitive import chain enforces -// safe update mode and emits a SECURITY REVIEW REQUIRED warning. The compile -// succeeds (warnings don't fail the build) and the new lock file serves as -// the baseline. +// TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport verifies that\n// the first compilation of a workflow with a transitive import chain creates the\n// baseline manifest silently without safe update warnings. func TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -544,15 +534,15 @@ func TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport(t *testing.T) os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithTransitiveImport), 0o644), "should write workflow file") - // No prior lock file — first compile enforces safe update and emits a warning. + // No prior lock file — first compile creates baseline silently. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) assert.NoError(t, err, - "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should emit safe update warnings so the agent reviews newly introduced secrets") - t.Logf("First compile correctly emitted warnings for transitively imported secret.\nOutput:\n%s", outputStr) + "first compile should succeed\nOutput:\n%s", outputStr) + assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should not emit safe update warnings when no prior manifest exists") + t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) } diff --git a/pkg/cli/upgrade_command.go b/pkg/cli/upgrade_command.go index 373479d849a..19ca74221c4 100644 --- a/pkg/cli/upgrade_command.go +++ b/pkg/cli/upgrade_command.go @@ -75,7 +75,7 @@ Examples: auditFlag, _ := cmd.Flags().GetBool("audit") jsonOutput, _ := cmd.Flags().GetBool("json") skipExtensionUpgrade, _ := cmd.Flags().GetBool("skip-extension-upgrade") - approveUpgrade, _ := cmd.Flags().GetBool("approve-updates") + approveUpgrade, _ := cmd.Flags().GetBool("approve") // Handle audit mode if auditFlag { @@ -111,7 +111,7 @@ Examples: cmd.Flags().Bool("pr", false, "Alias for --create-pull-request") _ = cmd.Flags().MarkHidden("pr") // Hide the short alias from help output cmd.Flags().Bool("audit", false, "Check dependency health without performing upgrades") - cmd.Flags().Bool("approve-updates", false, "Approve all safe update changes during compilation (skip safe update enforcement)") + cmd.Flags().Bool("approve", false, "Approve all safe update changes during compilation (skip safe update enforcement)") cmd.Flags().Bool("skip-extension-upgrade", false, "Skip automatic extension upgrade (used internally to prevent recursion after upgrade)") _ = cmd.Flags().MarkHidden("skip-extension-upgrade") addJSONFlag(cmd) @@ -143,8 +143,8 @@ func runDependencyAudit(verbose bool, jsonOutput bool) error { // runUpgradeCommand executes the upgrade process func runUpgradeCommand(ctx context.Context, verbose bool, workflowDir string, noFix bool, noCompile bool, noActions bool, skipExtensionUpgrade bool, approve bool) error { - upgradeLog.Printf("Running upgrade command: verbose=%v, workflowDir=%s, noFix=%v, noCompile=%v, noActions=%v, skipExtensionUpgrade=%v, approve=%v", - verbose, workflowDir, noFix, noCompile, noActions, skipExtensionUpgrade, approve) + upgradeLog.Printf("Running upgrade command: verbose=%v, workflowDir=%s, noFix=%v, noCompile=%v, noActions=%v, skipExtensionUpgrade=%v", + verbose, workflowDir, noFix, noCompile, noActions, skipExtensionUpgrade) // Step 0b: Ensure gh-aw extension is on the latest version. // If the extension was just upgraded, re-launch the freshly-installed binary diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index 57bcc5d697b..c453082432d 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -163,7 +163,7 @@ func (c *Compiler) SetNoEmit(noEmit bool) { c.noEmit = noEmit } -// SetApprove configures whether to skip safe update enforcement via the CLI --approve-updates flag. +// SetApprove configures whether to skip safe update enforcement via the CLI --approve flag. // When true, safe update enforcement is disabled regardless of strict mode setting, // approving all changes. func (c *Compiler) SetApprove(approve bool) { diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 050236e4e22..ead53a1735f 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -37,7 +37,7 @@ func (c *Compiler) effectiveStrictMode(frontmatter map[string]any) bool { // effectiveSafeUpdate returns true when safe update mode should be enforced for // the given workflow. Safe update mode is equivalent to strict mode: it is // enabled whenever strict mode is active (CLI --strict flag, frontmatter -// strict: true, or the default). It can be disabled via the CLI --approve-updates flag +// strict: true, or the default). It can be disabled via the CLI --approve flag // to approve all changes. func (c *Compiler) effectiveSafeUpdate(data *WorkflowData) bool { if c.approve { diff --git a/pkg/workflow/safe_update_enforcement.go b/pkg/workflow/safe_update_enforcement.go index d3f0a8d6422..5c0110e395a 100644 --- a/pkg/workflow/safe_update_enforcement.go +++ b/pkg/workflow/safe_update_enforcement.go @@ -31,12 +31,9 @@ var ghAwInternalSecrets = map[string]bool{ // changes have been introduced compared to those recorded in the existing manifest. // // manifest is the gh-aw-manifest extracted from the current lock file before -// recompilation. When nil (no lock file exists yet, or the lock file predates -// the safe-updates feature), it is treated as an empty baseline so that all -// non-GITHUB_TOKEN secrets and all custom actions are flagged on the very first -// compilation. This ensures agents receive a SECURITY REVIEW REQUIRED prompt even -// on the initial code-generation run. The newly generated lock file then embeds -// the manifest as the baseline for future compilations. +// recompilation. When nil (no lock file exists yet), it is treated as an empty +// manifest so that all non-GITHUB_TOKEN secrets and all custom actions are rejected +// on a first-time safe-update compilation. // // secretNames contains the raw names produced by CollectSecretReferences (i.e. // they may or may not carry the "secrets." prefix; both forms are normalized @@ -48,10 +45,12 @@ var ghAwInternalSecrets = map[string]bool{ // Returns a structured, actionable error when violations are found. func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string) error { if manifest == nil { - // Treat no prior manifest as an empty baseline so that newly introduced - // secrets and actions are flagged on first compilation as well. - safeUpdateLog.Print("No existing manifest found; enforcing safe update with empty baseline (new secrets/actions will be flagged)") - manifest = &GHAWManifest{Version: currentGHAWManifestVersion} + // No prior manifest found — either the lock file was compiled before the safe + // updates feature existed, or this is the very first compilation. In both cases + // skip enforcement: the newly generated lock file will embed a manifest that + // serves as the baseline for future compilations. + safeUpdateLog.Print("No existing manifest found; skipping safe update enforcement (baseline will be created)") + return nil } secretViolations := collectSecretViolations(manifest, secretNames) @@ -213,7 +212,7 @@ func buildSafeUpdateError(secretViolations, addedActions, removedActions []strin sb.WriteString(strings.Join(removedActions, "\n - ")) } - sb.WriteString("\n\nRemediation options:\n 1. Use the --approve-updates flag to allow the changes.\n 2. Revert the unapproved changes.\n 3. Use an interactive coding agent to review and approve the changes.") + sb.WriteString("\n\nRemediation options:\n 1. Use the --approve flag to allow the changes.\n 2. Revert the unapproved changes.\n 3. Use an interactive coding agent to review and approve the changes.") return fmt.Errorf("%s", sb.String()) } diff --git a/pkg/workflow/safe_update_enforcement_test.go b/pkg/workflow/safe_update_enforcement_test.go index e4357e75fa9..053db5dd03f 100644 --- a/pkg/workflow/safe_update_enforcement_test.go +++ b/pkg/workflow/safe_update_enforcement_test.go @@ -19,20 +19,18 @@ func TestEnforceSafeUpdate(t *testing.T) { wantErrMsgs []string }{ { - name: "nil manifest (no lock file) enforces on first compile — new secret flagged", + name: "nil manifest (no lock file) skips enforcement on first compile", manifest: nil, secretNames: []string{"MY_SECRET"}, actionRefs: []string{}, - wantErr: true, - wantErrMsgs: []string{"MY_SECRET", "safe update mode"}, + wantErr: false, }, { - name: "nil manifest (no lock file) enforces on first compile — custom action flagged", + name: "nil manifest (no lock file) skips enforcement for custom actions", manifest: nil, secretNames: []string{}, actionRefs: []string{"my-org/my-action@abc1234 # v1"}, - wantErr: true, - wantErrMsgs: []string{"my-org/my-action", "safe update mode"}, + wantErr: false, }, { name: "nil manifest (no lock file) allows GITHUB_TOKEN on first compile", @@ -291,7 +289,7 @@ func TestBuildSafeUpdateError(t *testing.T) { assert.Contains(t, msg, "safe update mode", "error message") assert.Contains(t, msg, "NEW_SECRET", "violation in message") assert.Contains(t, msg, "ANOTHER_SECRET", "violation in message") - assert.Contains(t, msg, "--approve-updates", "remediation guidance") + assert.Contains(t, msg, "--approve", "remediation guidance") }) t.Run("added actions only", func(t *testing.T) { From 17c747a5bba5e6dfbc30eb9f117c75b0c601fb04 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Wed, 15 Apr 2026 12:57:16 +1000 Subject: [PATCH 2/2] fix: enforce safe update on first compile (no lock file), skip only for legacy lock files without manifest Distinguish between two nil-manifest cases: - No lock file exists (new workflow): pass &GHAWManifest{} (empty non-nil) to EnforceSafeUpdate so all new secrets/actions are flagged for review - Lock file exists without a gh-aw-manifest section (legacy): pass nil to EnforceSafeUpdate which skips enforcement This restores SECURITY REVIEW REQUIRED warnings on first compilation while still allowing legacy lock files to upgrade without false positives. --- .../compile_safe_update_integration_test.go | 68 +++++++++++-------- pkg/workflow/compiler.go | 6 +- pkg/workflow/safe_update_enforcement.go | 19 +++--- pkg/workflow/safe_update_enforcement_test.go | 31 +++++++-- 4 files changed, 82 insertions(+), 42 deletions(-) diff --git a/pkg/cli/compile_safe_update_integration_test.go b/pkg/cli/compile_safe_update_integration_test.go index b178f1171c4..6c253e9a85f 100644 --- a/pkg/cli/compile_safe_update_integration_test.go +++ b/pkg/cli/compile_safe_update_integration_test.go @@ -91,8 +91,10 @@ func manifestLockFileWithSecret(secretName string) string { } // TestSafeUpdateFirstCompileCreatesBaseline verifies that the first compilation -// (with no prior manifest) creates the manifest baseline silently without any -// safe update warnings. Enforcement only kicks in on subsequent compilations. +// (with no prior lock file) enforces safe update mode and emits a +// SECURITY REVIEW REQUIRED warning so agents review newly introduced secrets. +// The compile itself succeeds (warnings do not fail the build) and the lock file +// written with the manifest serves as the baseline for future compilations. func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -101,17 +103,17 @@ func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithSecret), 0o644), "should write workflow file") - // First compile with no prior lock file: should succeed without safe update warnings - // because there is no prior manifest to compare against. + // First compile with no prior lock file: should succeed but emit safe update + // warnings because the agent must review newly introduced secrets. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) - assert.NoError(t, err, "first compile should succeed\nOutput:\n%s", outputStr) - // No safe update warnings on first compile (no prior manifest to compare against) - assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should not emit safe update warnings when no prior manifest exists") + assert.NoError(t, err, "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) + // Safe update warning must be emitted even on first compile so the agent reviews secrets. + assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should emit safe update warnings so the agent reviews newly introduced secrets") // Lock file must be written with the manifest baseline lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-secret.lock.yml") lockContent, readErr := os.ReadFile(lockFilePath) @@ -120,12 +122,13 @@ func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { "lock file should contain a gh-aw-manifest header after first compile") assert.Contains(t, string(lockContent), "MY_API_SECRET", "manifest should include the secret from the workflow") - t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) + t.Logf("First compile correctly emitted warnings.\nOutput:\n%s", outputStr) } // TestSafeUpdateFirstCompileCreatesBaselineForActions verifies that the first -// compilation with a custom action and no prior manifest creates the baseline -// silently without safe update warnings. +// compilation with a custom action and no prior manifest still enforces safe +// update mode, emitting a SECURITY REVIEW REQUIRED warning. The compile succeeds +// (warnings do not fail the build) and the new lock file serves as the baseline. func TestSafeUpdateFirstCompileCreatesBaselineForActions(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -134,20 +137,21 @@ func TestSafeUpdateFirstCompileCreatesBaselineForActions(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithCustomAction), 0o644), "should write workflow file") - // First compile with no prior lock file: should succeed without safe update warnings. + // First compile with no prior lock file: should succeed but emit safe update + // warning so the agent reviews the newly introduced custom action. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) - assert.NoError(t, err, "first compile should succeed\nOutput:\n%s", outputStr) - assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should not emit safe update warnings when no prior manifest exists") + assert.NoError(t, err, "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) + assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should emit safe update warnings so the agent reviews newly introduced actions") // Lock file must be written lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-action.lock.yml") _, statErr := os.Stat(lockFilePath) assert.NoError(t, statErr, "lock file should be written after first compile") - t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) + t.Logf("First compile correctly emitted warnings for new action.\nOutput:\n%s", outputStr) } // TestSafeUpdateAllowsKnownSecretWithPriorManifest verifies that safe update @@ -416,8 +420,10 @@ func TestSafeUpdateManifestIncludesImportedSecret(t *testing.T) { } // TestSafeUpdateFirstCompileCreatesBaselineForImport verifies that the first compilation -// of a workflow that imports a shared config containing a secret creates the baseline -// manifest silently without safe update warnings. +// of a workflow that imports a shared config containing a secret emits a +// SECURITY REVIEW REQUIRED warning so the agent reviews newly introduced secrets. +// The compile succeeds (warnings don't fail the build) and the lock file written +// serves as the baseline for future compilations. func TestSafeUpdateFirstCompileCreatesBaselineForImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -428,17 +434,17 @@ func TestSafeUpdateFirstCompileCreatesBaselineForImport(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithImport), 0o644), "should write workflow file") - // No prior lock file — first compile creates baseline silently. + // No prior lock file — first compile enforces safe update and emits a warning. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) assert.NoError(t, err, - "first compile should succeed\nOutput:\n%s", outputStr) - assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should not emit safe update warnings when no prior manifest exists") - t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) + "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) + assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should emit safe update warnings so the agent reviews newly introduced secrets") + t.Logf("First compile correctly emitted warnings for imported secret.\nOutput:\n%s", outputStr) } // TestSafeUpdateAllowsImportedSecretWithPriorManifest verifies that safe update @@ -522,7 +528,11 @@ func TestSafeUpdateManifestIncludesTransitivelyImportedSecret(t *testing.T) { } } -// TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport verifies that\n// the first compilation of a workflow with a transitive import chain creates the\n// baseline manifest silently without safe update warnings. +// TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport verifies that +// the first compilation of a workflow with a transitive import chain enforces +// safe update mode and emits a SECURITY REVIEW REQUIRED warning. The compile +// succeeds (warnings don't fail the build) and the new lock file serves as +// the baseline. func TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -534,15 +544,15 @@ func TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport(t *testing.T) os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithTransitiveImport), 0o644), "should write workflow file") - // No prior lock file — first compile creates baseline silently. + // No prior lock file — first compile enforces safe update and emits a warning. cmd := exec.Command(setup.binaryPath, "compile", workflowPath) cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) assert.NoError(t, err, - "first compile should succeed\nOutput:\n%s", outputStr) - assert.NotContains(t, outputStr, "SECURITY REVIEW REQUIRED", - "first compile should not emit safe update warnings when no prior manifest exists") - t.Logf("First compile correctly created baseline without warnings.\nOutput:\n%s", outputStr) + "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) + assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", + "first compile should emit safe update warnings so the agent reviews newly introduced secrets") + t.Logf("First compile correctly emitted warnings for transitively imported secret.\nOutput:\n%s", outputStr) } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index a90d1773638..bc54674732d 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -718,7 +718,11 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath log.Printf("Failed to parse filesystem gh-aw-manifest: %v. Safe update enforcement will treat as empty manifest.", parseErr) } } else { - log.Printf("Lock file %s not found on filesystem either (new workflow or not yet written). Safe update enforcement will treat as empty manifest.", lockFile) + // No lock file anywhere — this is a brand-new workflow. Use an empty + // (non-nil) manifest so EnforceSafeUpdate applies enforcement and flags + // any newly introduced secrets or actions for review. + log.Printf("Lock file %s not found (new workflow). Safe update enforcement will use an empty baseline.", lockFile) + oldManifest = &GHAWManifest{Version: currentGHAWManifestVersion} } } diff --git a/pkg/workflow/safe_update_enforcement.go b/pkg/workflow/safe_update_enforcement.go index 5c0110e395a..9fdcbc09566 100644 --- a/pkg/workflow/safe_update_enforcement.go +++ b/pkg/workflow/safe_update_enforcement.go @@ -31,9 +31,14 @@ var ghAwInternalSecrets = map[string]bool{ // changes have been introduced compared to those recorded in the existing manifest. // // manifest is the gh-aw-manifest extracted from the current lock file before -// recompilation. When nil (no lock file exists yet), it is treated as an empty -// manifest so that all non-GITHUB_TOKEN secrets and all custom actions are rejected -// on a first-time safe-update compilation. +// recompilation. +// +// - nil means a lock file was found but it predates the safe-updates feature +// (no gh-aw-manifest section). Enforcement is skipped so legacy lock files +// are not flagged on upgrade. +// - non-nil (including an empty &GHAWManifest{}) means the caller has a +// baseline to compare against. Pass &GHAWManifest{} when no lock file +// exists yet (first compilation); all new secrets/actions will be flagged. // // secretNames contains the raw names produced by CollectSecretReferences (i.e. // they may or may not carry the "secrets." prefix; both forms are normalized @@ -45,11 +50,9 @@ var ghAwInternalSecrets = map[string]bool{ // Returns a structured, actionable error when violations are found. func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string) error { if manifest == nil { - // No prior manifest found — either the lock file was compiled before the safe - // updates feature existed, or this is the very first compilation. In both cases - // skip enforcement: the newly generated lock file will embed a manifest that - // serves as the baseline for future compilations. - safeUpdateLog.Print("No existing manifest found; skipping safe update enforcement (baseline will be created)") + // Lock file exists but predates the safe-updates feature (no gh-aw-manifest + // section). Skip enforcement so legacy lock files are not flagged on upgrade. + safeUpdateLog.Print("Lock file has no gh-aw-manifest; skipping safe update enforcement (legacy lock file)") return nil } diff --git a/pkg/workflow/safe_update_enforcement_test.go b/pkg/workflow/safe_update_enforcement_test.go index 053db5dd03f..3d141a9fd82 100644 --- a/pkg/workflow/safe_update_enforcement_test.go +++ b/pkg/workflow/safe_update_enforcement_test.go @@ -19,33 +19,56 @@ func TestEnforceSafeUpdate(t *testing.T) { wantErrMsgs []string }{ { - name: "nil manifest (no lock file) skips enforcement on first compile", + name: "nil manifest (lock file without manifest section) skips enforcement", manifest: nil, secretNames: []string{"MY_SECRET"}, actionRefs: []string{}, wantErr: false, }, { - name: "nil manifest (no lock file) skips enforcement for custom actions", + name: "nil manifest (lock file without manifest section) skips enforcement for actions", manifest: nil, secretNames: []string{}, actionRefs: []string{"my-org/my-action@abc1234 # v1"}, wantErr: false, }, { - name: "nil manifest (no lock file) allows GITHUB_TOKEN on first compile", + name: "nil manifest (lock file without manifest section) skips with GITHUB_TOKEN", manifest: nil, secretNames: []string{"GITHUB_TOKEN"}, actionRefs: []string{}, wantErr: false, }, { - name: "nil manifest (no lock file) with no secrets or actions passes", + name: "nil manifest (lock file without manifest section) skips with no secrets", manifest: nil, secretNames: []string{}, actionRefs: []string{}, wantErr: false, }, + { + name: "empty non-nil manifest (no lock file) enforces — new secret flagged", + manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + secretNames: []string{"MY_SECRET"}, + actionRefs: []string{}, + wantErr: true, + wantErrMsgs: []string{"MY_SECRET", "safe update mode"}, + }, + { + name: "empty non-nil manifest (no lock file) enforces — custom action flagged", + manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + secretNames: []string{}, + actionRefs: []string{"my-org/my-action@abc1234 # v1"}, + wantErr: true, + wantErrMsgs: []string{"my-org/my-action", "safe update mode"}, + }, + { + name: "empty non-nil manifest (no lock file) allows GITHUB_TOKEN", + manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + secretNames: []string{"GITHUB_TOKEN"}, + actionRefs: []string{}, + wantErr: false, + }, { name: "empty secrets and actions with existing manifest passes", manifest: &GHAWManifest{Version: 1, Secrets: []string{}, Actions: []GHAWManifestAction{}},