diff --git a/cmd/gh-aw/main.go b/cmd/gh-aw/main.go index 500007af20..343e33a458 100644 --- a/cmd/gh-aw/main.go +++ b/cmd/gh-aw/main.go @@ -286,7 +286,7 @@ Examples: failFast, _ := cmd.Flags().GetBool("fail-fast") noCheckUpdate, _ := cmd.Flags().GetBool("no-check-update") scheduleSeed, _ := cmd.Flags().GetString("schedule-seed") - safeUpdate, _ := cmd.Flags().GetBool("safe-update") + approve, _ := cmd.Flags().GetBool("approve-updates") validateImages, _ := cmd.Flags().GetBool("validate-images") priorManifestFile, _ := cmd.Flags().GetString("prior-manifest-file") verbose, _ := cmd.Flags().GetBool("verbose") @@ -343,7 +343,7 @@ Examples: Stats: stats, FailFast: failFast, ScheduleSeed: scheduleSeed, - SafeUpdate: safeUpdate, + Approve: approve, ValidateImages: validateImages, PriorManifestFile: priorManifestFile, } @@ -399,6 +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") if err := validateEngine(engineOverride); err != nil { return err @@ -437,6 +438,7 @@ Examples: Verbose: verboseFlag, DryRun: dryRun, JSON: jsonOutput, + Approve: approveRun, }) }, } @@ -692,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("safe-update", false, "Force-enable safe update mode independently of strict mode. Safe update mode is normally equivalent to strict mode: it emits a warning prompt when compilations introduce new restricted secrets or unapproved action additions/removals not present in the existing gh-aw-manifest. Use this flag to enable safe update enforcement on a workflow that has strict: false in its frontmatter") + 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("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 { @@ -733,6 +735,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all 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)") // 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 706425a3f0..308a2aac77 100644 --- a/pkg/cli/compile_compiler_setup.go +++ b/pkg/cli/compile_compiler_setup.go @@ -156,11 +156,11 @@ func configureCompilerFlags(compiler *workflow.Compiler, config CompileConfig) { compileCompilerSetupLog.Print("Force refresh action pins enabled: will clear cache and resolve all actions from GitHub API") } - // Set safe update flag: when set via CLI it force-enables safe update enforcement - // independently of the workflow's strict mode setting. - compiler.SetSafeUpdate(config.SafeUpdate) - if config.SafeUpdate { - compileCompilerSetupLog.Print("Safe update mode force-enabled via --safe-update flag: compilations introducing new restricted secrets or unapproved action additions/removals will emit a warning prompt requesting agent review and a PR security note") + // Set safe update flag: when set via CLI it disables/skips safe update enforcement + // 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") } // Set require docker flag: when set, container image validation fails instead of diff --git a/pkg/cli/compile_config.go b/pkg/cli/compile_config.go index 39ad3962f9..055b7d3759 100644 --- a/pkg/cli/compile_config.go +++ b/pkg/cli/compile_config.go @@ -29,7 +29,7 @@ type CompileConfig struct { Stats bool // Display statistics table sorted by file size FailFast bool // Stop at first error instead of collecting all errors ScheduleSeed string // Override repository slug used for fuzzy schedule scattering (e.g. owner/repo) - SafeUpdate bool // Force-enable safe update mode regardless of strict mode setting. Safe update mode is normally equivalent to strict mode (active whenever strict mode is active). + Approve bool // Approve all safe update changes, skipping safe update enforcement regardless of strict mode setting. ValidateImages bool // Require Docker to be available for container image validation (fail instead of skipping when Docker is unavailable) PriorManifestFile string // Path to a JSON file containing pre-cached manifests (map[lockFile]*GHAWManifest) collected at MCP server startup; takes precedence over git HEAD / filesystem reads for safe update enforcement } diff --git a/pkg/cli/compile_safe_update_integration_test.go b/pkg/cli/compile_safe_update_integration_test.go index 00f4b980b6..6139754d27 100644 --- a/pkg/cli/compile_safe_update_integration_test.go +++ b/pkg/cli/compile_safe_update_integration_test.go @@ -90,11 +90,12 @@ func manifestLockFileWithSecret(secretName string) string { ) } -// TestSafeUpdateWarnOnNewSecretOnFirstCompile verifies that --safe-update emits a -// warning (not an error) when a first compilation introduces a non-GITHUB_TOKEN -// secret and no prior manifest exists. The compilation must succeed and the lock -// file must be written so that the agent receives the actionable warning. -func TestSafeUpdateWarnsOnNewSecretOnFirstCompile(t *testing.T) { +// 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. +func TestSafeUpdateFirstCompileCreatesBaseline(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -102,28 +103,33 @@ func TestSafeUpdateWarnsOnNewSecretOnFirstCompile(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithSecret), 0o644), "should write workflow file") - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // 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) - // Compilation must succeed (warning, not error) - assert.NoError(t, err, "compile should succeed in safe update mode (violation emits warning, not error)\nOutput:\n%s", outputStr) - // Warning must reference the violation and request a security review - assert.Contains(t, outputStr, "safe update mode", "output should mention safe update mode") - assert.Contains(t, outputStr, "MY_API_SECRET", "warning should name the offending secret") - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", "warning should request a security review") - // Lock file must still be written + 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") - _, statErr := os.Stat(lockFilePath) - assert.NoError(t, statErr, "lock file should be written even when a safe-update warning is emitted") - t.Logf("Safe update correctly emitted warning for new secret.\nOutput:\n%s", outputStr) + lockContent, readErr := os.ReadFile(lockFilePath) + require.NoError(t, readErr, "should read lock file after first compile") + assert.Contains(t, string(lockContent), "gh-aw-manifest:", + "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) } -// TestSafeUpdateWarnOnNewCustomActionOnFirstCompile verifies that --safe-update emits -// a warning (not an error) when a first compilation introduces a non-actions/* action -// reference and no prior manifest exists. Compilation must succeed. -func TestSafeUpdateWarnsOnNewCustomActionOnFirstCompile(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. +func TestSafeUpdateFirstCompileCreatesBaselineForActions(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -131,33 +137,31 @@ func TestSafeUpdateWarnsOnNewCustomActionOnFirstCompile(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithCustomAction), 0o644), "should write workflow file") - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // 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) - // Compilation must succeed (warning, not error) - assert.NoError(t, err, "compile should succeed in safe update mode (violation emits warning, not error)\nOutput:\n%s", outputStr) - // Warning must reference the violation and request a security review - assert.Contains(t, outputStr, "safe update mode", "output should mention safe update mode") - assert.Contains(t, outputStr, "my-org/custom-action", "warning should name the offending action") - assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", "warning should request a security review") - // Lock file must still be written + 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 even when a safe-update warning is emitted") - t.Logf("Safe update correctly emitted warning for new custom action.\nOutput:\n%s", outputStr) + 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) } -// TestSafeUpdateAllowsKnownSecretWithPriorManifest verifies that --safe-update -// allows a compilation when the secret is already recorded in the prior manifest -// embedded in the existing lock file. +// TestSafeUpdateAllowsKnownSecretWithPriorManifest verifies that safe update +// enforcement allows a compilation when the secret is already recorded in the +// prior manifest embedded in the existing lock file. // -// The test uses a two-step approach: first compile without --safe-update to produce -// a complete lock file with the full manifest (including engine-internal secrets and -// actions), then compile again with --safe-update. Since nothing changed between the -// two compilations, no new secrets or actions are introduced and the second compile -// must succeed. +// The test uses a two-step approach: first compile to produce a complete lock +// file with the full manifest (including engine-internal secrets and actions), +// then compile again. Since nothing changed between the two compilations, no +// new secrets or actions are introduced and the second compile must succeed. func TestSafeUpdateAllowsKnownSecretWithPriorManifest(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -166,13 +170,13 @@ func TestSafeUpdateAllowsKnownSecretWithPriorManifest(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithSecret), 0o644), "should write workflow file") - // Step 1: Compile without --safe-update to generate the full lock file + manifest. + // Step 1: Compile to generate the full lock file + manifest. // (Engine-internal secrets such as COPILOT_GITHUB_TOKEN are also captured here.) step1Cmd := exec.Command(setup.binaryPath, "compile", workflowPath) step1Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") step1Out, step1Err := step1Cmd.CombinedOutput() require.NoError(t, step1Err, - "first compilation (no --safe-update) should succeed\nOutput:\n%s", string(step1Out)) + "first compilation should succeed\nOutput:\n%s", string(step1Out)) lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-known-secret.lock.yml") lockContent, readErr := os.ReadFile(lockFilePath) @@ -180,9 +184,9 @@ func TestSafeUpdateAllowsKnownSecretWithPriorManifest(t *testing.T) { require.Contains(t, string(lockContent), "MY_API_SECRET", "lock file manifest should include MY_API_SECRET after first compile") - // Step 2: Compile the identical workflow with --safe-update. The lock file from - // step 1 acts as the prior manifest. Nothing changed, so this must succeed. - step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // Step 2: Compile the identical workflow again. The lock file from step 1 acts + // as the prior manifest. Nothing changed, so this must succeed without warnings. + step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath) step2Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := step2Cmd.CombinedOutput() outputStr := string(output) @@ -191,14 +195,13 @@ func TestSafeUpdateAllowsKnownSecretWithPriorManifest(t *testing.T) { t.Logf("Safe update correctly allowed known secret.\nOutput:\n%s", outputStr) } -// TestSafeUpdateAllowsGitHubTokenOnFirstCompile verifies that --safe-update allows -// a compilation that introduces no new non-GITHUB_TOKEN secrets compared to a +// TestSafeUpdateAllowsGitHubTokenOnFirstCompile verifies that safe update enforcement +// allows a compilation that introduces no new non-GITHUB_TOKEN secrets compared to a // previously recorded manifest. // -// Uses a two-step approach: step 1 compiles without --safe-update to record the -// baseline manifest (which includes engine-internal secrets in release mode); step 2 -// recompiles the same workflow with --safe-update and expects success because the -// manifest is unchanged. +// Uses a two-step approach: step 1 compiles to record the baseline manifest (which +// includes engine-internal secrets in release mode); step 2 recompiles the same +// workflow and expects success because the manifest is unchanged. func TestSafeUpdateAllowsGitHubTokenOnFirstCompile(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -212,7 +215,7 @@ func TestSafeUpdateAllowsGitHubTokenOnFirstCompile(t *testing.T) { step1Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") step1Out, step1Err := step1Cmd.CombinedOutput() require.NoError(t, step1Err, - "first compilation (no --safe-update) should succeed\nOutput:\n%s", string(step1Out)) + "first compilation should succeed\nOutput:\n%s", string(step1Out)) lockFilePath := filepath.Join(setup.workflowsDir, "safe-update-basic.lock.yml") lockContent, readErr := os.ReadFile(lockFilePath) @@ -220,8 +223,8 @@ func TestSafeUpdateAllowsGitHubTokenOnFirstCompile(t *testing.T) { require.Contains(t, string(lockContent), "gh-aw-manifest:", "lock file should contain a gh-aw-manifest header after first compile") - // Step 2: Re-compile with --safe-update. No secrets were added so this must succeed. - step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // Step 2: Re-compile. No secrets were added so this must succeed. + step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath) step2Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := step2Cmd.CombinedOutput() outputStr := string(output) @@ -237,8 +240,9 @@ func TestSafeUpdateAllowsGitHubTokenOnFirstCompile(t *testing.T) { } // safeUpdateWorkflowNonStrict is a minimal workflow that explicitly opts out of -// strict mode. Because safe update mode == strict mode, setting strict: false -// also disables safe update enforcement, letting new secrets compile freely. +// strict mode. Because safe update enforcement follows strict mode, setting +// strict: false also disables safe update enforcement, letting new secrets +// compile freely. const safeUpdateWorkflowNonStrict = `--- name: Non Strict Workflow on: @@ -264,7 +268,7 @@ Workflow with strict: false, which also disables safe update enforcement. // TestSafeUpdateNoFlagAllowsNewSecret verifies that when strict mode is disabled // (strict: false in frontmatter) safe update enforcement is also disabled — new -// secrets compile without any safe-update warning. +// secrets compile without any safe update warning. func TestSafeUpdateNoFlagAllowsNewSecret(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -395,8 +399,8 @@ func TestSafeUpdateManifestIncludesImportedSecret(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithImport), 0o644), "should write workflow file") - // Compile without --safe-update so we can inspect the manifest freely. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath) + // Compile with --approve so we can inspect the manifest freely without safe update warnings. + cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve-updates") cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) @@ -415,10 +419,12 @@ func TestSafeUpdateManifestIncludesImportedSecret(t *testing.T) { } } -// TestSafeUpdateRejectsNewSecretFromImport verifies that --safe-update emits a warning -// when the new secret is introduced via an imported workflow rather than directly in -// the top-level workflow frontmatter. Compilation must still succeed. -func TestSafeUpdateWarnsOnNewSecretFromImport(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. +func TestSafeUpdateFirstCompileCreatesBaselineForImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -428,32 +434,27 @@ func TestSafeUpdateWarnsOnNewSecretFromImport(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithImport), 0o644), "should write workflow file") - // No prior lock file — safe update treats this as an empty manifest. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // 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) - // Compilation must succeed (warning, not error) assert.NoError(t, err, - "compile should succeed (violation emits warning, not error) when import introduces a new secret under --safe-update\nOutput:\n%s", outputStr) - assert.Contains(t, outputStr, "safe update mode", - "warning should mention safe update mode") - assert.Contains(t, outputStr, "SHARED_API_KEY", - "warning should name the secret that came from the import") + "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "warning should request a security review") - t.Logf("Safe update correctly warned about secret introduced via import.\nOutput:\n%s", outputStr) + "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 -// allows compilation when the secret introduced by an import is already recorded -// in the prior lock file's gh-aw-manifest. +// TestSafeUpdateAllowsImportedSecretWithPriorManifest verifies that safe update +// enforcement allows compilation when the secret introduced by an import is already +// recorded in the prior lock file's gh-aw-manifest. // // The test uses a two-step approach to avoid hard-coding the full set of // engine-required secrets in the prior manifest: -// 1. Compile without --safe-update to produce a lock file with the full manifest. -// 2. Compile again with --safe-update; the existing lock file (from step 1) acts as +// 1. Compile to produce a lock file with the full manifest. +// 2. Compile again; the existing lock file (from step 1) acts as // the prior manifest and the compilation should succeed since no new // secrets or actions are being introduced. func TestSafeUpdateAllowsImportedSecretWithPriorManifest(t *testing.T) { @@ -466,12 +467,12 @@ func TestSafeUpdateAllowsImportedSecretWithPriorManifest(t *testing.T) { require.NoError(t, os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithImport), 0o644), "should write workflow file") - // Step 1: Compile without --safe-update to generate the lock file + manifest. + // Step 1: Compile to generate the lock file + manifest. step1Cmd := exec.Command(setup.binaryPath, "compile", workflowPath) step1Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") step1Out, step1Err := step1Cmd.CombinedOutput() require.NoError(t, step1Err, - "first compilation (no --safe-update) should succeed\nOutput:\n%s", string(step1Out)) + "first compilation should succeed\nOutput:\n%s", string(step1Out)) // Verify the lock file was created and contains the manifest. lockPath := filepath.Join(setup.workflowsDir, "import-approved.lock.yml") @@ -480,15 +481,15 @@ func TestSafeUpdateAllowsImportedSecretWithPriorManifest(t *testing.T) { require.Contains(t, string(lockContent), "SHARED_API_KEY", "lock file manifest should include the imported secret after first compile") - // Step 2: Compile again with --safe-update. The lock file from step 1 serves - // as the prior manifest. No new secrets or actions are introduced so this must succeed. - step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // Step 2: Compile again. The lock file from step 1 serves as the prior manifest. + // No new secrets or actions are introduced so this must succeed. + step2Cmd := exec.Command(setup.binaryPath, "compile", workflowPath) step2Cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") step2Out, step2Err := step2Cmd.CombinedOutput() outputStr := string(step2Out) assert.NoError(t, step2Err, - "second compilation (with --safe-update) should succeed when imported secret is already in the manifest\nOutput:\n%s", outputStr) + "second compilation should succeed when imported secret is already in the manifest\nOutput:\n%s", outputStr) t.Logf("Safe update correctly allowed pre-approved imported secret.\nOutput:\n%s", outputStr) } @@ -507,8 +508,8 @@ func TestSafeUpdateManifestIncludesTransitivelyImportedSecret(t *testing.T) { os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithTransitiveImport), 0o644), "should write workflow file") - // Compile without --safe-update so we can freely inspect the manifest. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath) + // Compile with --approve so we can freely inspect the manifest without safe update warnings. + cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--approve-updates") cmd.Env = append(os.Environ(), "GH_AW_ACTION_MODE=release") output, err := cmd.CombinedOutput() outputStr := string(output) @@ -527,10 +528,12 @@ func TestSafeUpdateManifestIncludesTransitivelyImportedSecret(t *testing.T) { } } -// TestSafeUpdateRejectsTransitivelyImportedSecretOnFirstCompile verifies that -// --safe-update emits a warning when the new secret is introduced via a transitive -// import (A imports B, B imports C, C declares the secret). Compilation must succeed. -func TestSafeUpdateWarnsOnTransitivelyImportedSecretOnFirstCompile(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. +func TestSafeUpdateFirstCompileCreatesBaselineForTransitiveImport(t *testing.T) { setup := setupIntegrationTest(t) defer setup.cleanup() @@ -541,20 +544,15 @@ func TestSafeUpdateWarnsOnTransitivelyImportedSecretOnFirstCompile(t *testing.T) os.WriteFile(workflowPath, []byte(safeUpdateWorkflowWithTransitiveImport), 0o644), "should write workflow file") - // No prior lock file — safe update treats this as an empty manifest. - cmd := exec.Command(setup.binaryPath, "compile", workflowPath, "--safe-update") + // 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) - // Compilation must succeed (warning, not error) assert.NoError(t, err, - "compile should succeed (violation emits warning, not error) when a transitive import introduces a new secret under --safe-update\nOutput:\n%s", outputStr) - assert.Contains(t, outputStr, "safe update mode", - "warning should mention safe update mode") - assert.Contains(t, outputStr, "DEEP_NESTED_SECRET", - "warning should name the secret that came from the transitive import") + "first compile should succeed (warnings don't fail the build)\nOutput:\n%s", outputStr) assert.Contains(t, outputStr, "SECURITY REVIEW REQUIRED", - "warning should request a security review") - t.Logf("Safe update correctly warned about secret from transitive import.\nOutput:\n%s", outputStr) + "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/cli/run_push.go b/pkg/cli/run_push.go index b29920f6d7..2ffd446d02 100644 --- a/pkg/cli/run_push.go +++ b/pkg/cli/run_push.go @@ -27,7 +27,7 @@ var runPushLog = logger.New("cli:run_push") // and the transitive closure of all imported files. // Note: This function always recompiles the workflow to ensure the lock file is up-to-date, // regardless of the frontmatter hash status. -func collectWorkflowFiles(ctx context.Context, workflowPath string, verbose bool) ([]string, error) { +func collectWorkflowFiles(ctx context.Context, workflowPath string, verbose bool, approve bool) ([]string, error) { runPushLog.Printf("Collecting files for workflow: %s", workflowPath) files := make(map[string]bool) // Use map to avoid duplicates @@ -90,7 +90,7 @@ func collectWorkflowFiles(ctx context.Context, workflowPath string, verbose bool // Always recompile (hash check is for observability only) runPushLog.Printf("Always recompiling workflow: %s", absWorkflowPath) - if err := recompileWorkflow(ctx, absWorkflowPath, verbose); err != nil { + if err := recompileWorkflow(ctx, absWorkflowPath, verbose, approve); err != nil { runPushLog.Printf("Failed to recompile workflow: %v", err) return nil, fmt.Errorf("failed to recompile workflow: %w", err) } @@ -128,7 +128,7 @@ func collectWorkflowFiles(ctx context.Context, workflowPath string, verbose bool } // recompileWorkflow compiles a workflow using CompileWorkflows -func recompileWorkflow(ctx context.Context, workflowPath string, verbose bool) error { +func recompileWorkflow(ctx context.Context, workflowPath string, verbose bool, approve bool) error { runPushLog.Printf("Recompiling workflow: %s", workflowPath) config := CompileConfig{ @@ -144,6 +144,7 @@ func recompileWorkflow(ctx context.Context, workflowPath string, verbose bool) e TrialMode: false, TrialLogicalRepoSlug: "", Strict: false, + Approve: approve, } runPushLog.Printf("Compilation config: Validate=%v, NoEmit=%v", config.Validate, config.NoEmit) diff --git a/pkg/cli/run_push_integration_test.go b/pkg/cli/run_push_integration_test.go index 7e148f3ec9..ec0f6b855a 100644 --- a/pkg/cli/run_push_integration_test.go +++ b/pkg/cli/run_push_integration_test.go @@ -34,7 +34,7 @@ This is a test workflow without a lock file. require.NoError(t, err) // Test collecting files - should now compile the workflow and create lock file - files, err := collectWorkflowFiles(context.Background(), workflowPath, false) + files, err := collectWorkflowFiles(context.Background(), workflowPath, false, false) require.NoError(t, err) assert.Len(t, files, 2, "Should collect workflow .md file and auto-generate lock file") diff --git a/pkg/cli/run_push_test.go b/pkg/cli/run_push_test.go index b3c86cd9fc..fbb9294b50 100644 --- a/pkg/cli/run_push_test.go +++ b/pkg/cli/run_push_test.go @@ -41,7 +41,7 @@ on: workflow_dispatch require.NoError(t, err) // Test collecting files - files, err := collectWorkflowFiles(context.Background(), workflowPath, false) + files, err := collectWorkflowFiles(context.Background(), workflowPath, false, false) require.NoError(t, err) assert.Len(t, files, 2, "Should collect workflow .md and .lock.yml files") @@ -89,7 +89,7 @@ on: workflow_dispatch require.NoError(t, err) // Test collecting files - files, err := collectWorkflowFiles(context.Background(), workflowPath, false) + files, err := collectWorkflowFiles(context.Background(), workflowPath, false, false) require.NoError(t, err) assert.Len(t, files, 3, "Should collect workflow, lock, and imported files") @@ -150,7 +150,7 @@ on: workflow_dispatch require.NoError(t, err) // Test collecting files - files, err := collectWorkflowFiles(context.Background(), workflowPath, false) + files, err := collectWorkflowFiles(context.Background(), workflowPath, false, false) require.NoError(t, err) assert.Len(t, files, 4, "Should collect workflow, lock, and all transitive imports") @@ -504,7 +504,7 @@ jobs: time.Sleep(100 * time.Millisecond) // Collect workflow files (which should always trigger recompilation) - files, err := collectWorkflowFiles(context.Background(), workflowPath, false) + files, err := collectWorkflowFiles(context.Background(), workflowPath, false, false) require.NoError(t, err) assert.Len(t, files, 2, "Should collect workflow .md and .lock.yml files") diff --git a/pkg/cli/run_workflow_execution.go b/pkg/cli/run_workflow_execution.go index 0c2fbc7575..c02d6745c7 100644 --- a/pkg/cli/run_workflow_execution.go +++ b/pkg/cli/run_workflow_execution.go @@ -36,6 +36,7 @@ type RunOptions struct { Verbose bool // Enable verbose output DryRun bool // Validate without actually triggering JSON bool // Output results in JSON format + Approve bool // Approve safe update changes during compilation } // WorkflowRunResult contains the result of a single workflow run trigger for JSON output @@ -277,7 +278,7 @@ func RunWorkflowOnGitHub(ctx context.Context, workflowIdOrName string, opts RunO // Collect the workflow .md file, .lock.yml file, and transitive imports workflowMarkdownPath := stringutil.LockFileToMarkdown(lockFilePath) - files, err := collectWorkflowFiles(ctx, workflowMarkdownPath, opts.Verbose) + files, err := collectWorkflowFiles(ctx, workflowMarkdownPath, opts.Verbose, opts.Approve) if err != nil { return fmt.Errorf("failed to collect workflow files: %w", err) } diff --git a/pkg/cli/upgrade_command.go b/pkg/cli/upgrade_command.go index 97d6c39c59..373479d849 100644 --- a/pkg/cli/upgrade_command.go +++ b/pkg/cli/upgrade_command.go @@ -75,6 +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") // Handle audit mode if auditFlag { @@ -87,7 +88,7 @@ Examples: } } - if err := runUpgradeCommand(cmd.Context(), verbose, dir, noFix, noCompile, noActions, skipExtensionUpgrade); err != nil { + if err := runUpgradeCommand(cmd.Context(), verbose, dir, noFix, noCompile, noActions, skipExtensionUpgrade, approveUpgrade); err != nil { return err } @@ -110,6 +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("skip-extension-upgrade", false, "Skip automatic extension upgrade (used internally to prevent recursion after upgrade)") _ = cmd.Flags().MarkHidden("skip-extension-upgrade") addJSONFlag(cmd) @@ -140,9 +142,9 @@ 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) error { - upgradeLog.Printf("Running upgrade command: verbose=%v, workflowDir=%s, noFix=%v, noCompile=%v, noActions=%v, skipExtensionUpgrade=%v", - verbose, workflowDir, noFix, noCompile, noActions, skipExtensionUpgrade) +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) // Step 0b: Ensure gh-aw extension is on the latest version. // If the extension was just upgraded, re-launch the freshly-installed binary @@ -257,6 +259,7 @@ func runUpgradeCommand(ctx context.Context, verbose bool, workflowDir string, no compiler := createAndConfigureCompiler(CompileConfig{ Verbose: verbose, WorkflowDir: workflowDir, + Approve: approve, }) // Determine workflow directory diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index d34c895058..57bcc5d697 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -57,7 +57,7 @@ type Compiler struct { skipValidation bool // If true, skip schema validation noEmit bool // If true, validate without generating lock files strictMode bool // If true, enforce strict validation requirements - safeUpdate bool // If true, enforce safe update mode (reject newly introduced secrets) + approve bool // If true, approve safe update changes (skip safe update enforcement) trialMode bool // If true, suppress safe outputs for trial mode execution trialLogicalRepoSlug string // If set in trial mode, the logical repository to checkout refreshStopTime bool // If true, regenerate stop-after times instead of preserving existing ones @@ -163,12 +163,11 @@ func (c *Compiler) SetNoEmit(noEmit bool) { c.noEmit = noEmit } -// SetSafeUpdate configures whether to force-enable safe update mode via the CLI flag. -// Safe update mode is normally equivalent to strict mode (active when strict mode is active). -// This flag provides an additional override to enable safe update mode independently of -// the strict mode setting. -func (c *Compiler) SetSafeUpdate(safeUpdate bool) { - c.safeUpdate = safeUpdate +// SetApprove configures whether to skip safe update enforcement via the CLI --approve-updates flag. +// When true, safe update enforcement is disabled regardless of strict mode setting, +// approving all changes. +func (c *Compiler) SetApprove(approve bool) { + c.approve = approve } // SetFileTracker sets the file tracker for tracking created files diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 7eda7b6656..050236e4e2 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -37,11 +37,11 @@ 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 also be force-enabled via the CLI -// --safe-update flag independently of strict mode. +// strict: true, or the default). It can be disabled via the CLI --approve-updates flag +// to approve all changes. func (c *Compiler) effectiveSafeUpdate(data *WorkflowData) bool { - if c.safeUpdate { - return true + if c.approve { + return false } return c.effectiveStrictMode(data.RawFrontmatter) } diff --git a/pkg/workflow/safe_update_enforcement.go b/pkg/workflow/safe_update_enforcement.go index 37c684893b..d3f0a8d642 100644 --- a/pkg/workflow/safe_update_enforcement.go +++ b/pkg/workflow/safe_update_enforcement.go @@ -31,9 +31,12 @@ 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. 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. // // secretNames contains the raw names produced by CollectSecretReferences (i.e. // they may or may not carry the "secrets." prefix; both forms are normalized @@ -45,10 +48,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 lock file – treat as an empty manifest so safe-update enforcement - // blocks any secrets (other than GITHUB_TOKEN) and any custom actions on the - // first compilation, matching the principle of least privilege. - safeUpdateLog.Print("No existing manifest found; treating as empty manifest for safe update enforcement") + // 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} } @@ -211,7 +213,7 @@ func buildSafeUpdateError(secretViolations, addedActions, removedActions []strin sb.WriteString(strings.Join(removedActions, "\n - ")) } - sb.WriteString("\n\nRemediation options:\n 1. Use an interactive agentic flow (e.g. Copilot CLI) to review and approve the changes.\n 2. Remove the --safe-update flag to allow the change.\n 3. Revert the unapproved changes from your workflow if they were added unintentionally.") + 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.") 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 8a068870ec..e4357e75fa 100644 --- a/pkg/workflow/safe_update_enforcement_test.go +++ b/pkg/workflow/safe_update_enforcement_test.go @@ -19,7 +19,7 @@ func TestEnforceSafeUpdate(t *testing.T) { wantErrMsgs []string }{ { - name: "nil manifest (no lock file) blocks secrets on first compile", + name: "nil manifest (no lock file) enforces on first compile — new secret flagged", manifest: nil, secretNames: []string{"MY_SECRET"}, actionRefs: []string{}, @@ -27,7 +27,7 @@ func TestEnforceSafeUpdate(t *testing.T) { wantErrMsgs: []string{"MY_SECRET", "safe update mode"}, }, { - name: "nil manifest (no lock file) blocks custom actions on first compile", + name: "nil manifest (no lock file) enforces on first compile — custom action flagged", manifest: nil, secretNames: []string{}, actionRefs: []string{"my-org/my-action@abc1234 # v1"}, @@ -291,7 +291,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, "interactive agentic flow", "remediation guidance") + assert.Contains(t, msg, "--approve-updates", "remediation guidance") }) t.Run("added actions only", func(t *testing.T) { @@ -433,48 +433,53 @@ func TestCollectActionViolations(t *testing.T) { } func TestEffectiveSafeUpdate(t *testing.T) { - // effectiveSafeUpdate is equivalent to effectiveStrictMode: - // it returns true when the compiler safeUpdate flag is set, OR when strict - // mode is active (which defaults to true unless frontmatter sets strict: false). + // effectiveSafeUpdate returns true when strict mode is active (default true), + // UNLESS the compiler approve flag is set, which skips enforcement entirely. tests := []struct { name string - compilerFlag bool + approveFlag bool rawFrontmatter map[string]any want bool }{ { - name: "compiler flag off, no frontmatter => true (strict default)", - compilerFlag: false, - want: true, // strict mode defaults to true, so safe update is enabled + name: "approve off, no frontmatter => true (strict default)", + approveFlag: false, + want: true, // strict mode defaults to true, so safe update is enabled }, { - name: "compiler flag on => true", - compilerFlag: true, - want: true, + name: "approve on => false (enforcement skipped)", + approveFlag: true, + want: false, }, { - name: "frontmatter strict: false, compiler flag off => false", - compilerFlag: false, + name: "frontmatter strict: false, approve off => false", + approveFlag: false, rawFrontmatter: map[string]any{"strict": false}, want: false, }, { - name: "frontmatter strict: false, compiler flag on => true", - compilerFlag: true, + name: "frontmatter strict: false, approve on => false", + approveFlag: true, rawFrontmatter: map[string]any{"strict": false}, - want: true, // CLI flag overrides frontmatter + want: false, }, { - name: "frontmatter strict: true, compiler flag off => true", - compilerFlag: false, + name: "frontmatter strict: true, approve off => true", + approveFlag: false, rawFrontmatter: map[string]any{"strict": true}, want: true, }, + { + name: "frontmatter strict: true, approve on => false (flag overrides)", + approveFlag: true, + rawFrontmatter: map[string]any{"strict": true}, + want: false, // --approve overrides strict mode + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c := &Compiler{safeUpdate: tt.compilerFlag} + c := &Compiler{approve: tt.approveFlag} data := &WorkflowData{RawFrontmatter: tt.rawFrontmatter} got := c.effectiveSafeUpdate(data) assert.Equal(t, tt.want, got, "effectiveSafeUpdate result")