fix: rename --safe-update to --approve and improve safe update UX#26160
fix: rename --safe-update to --approve and improve safe update UX#26160
Conversation
- Rename --safe-update flag to --approve with inverted semantics: --approve now skips safe update enforcement (approves changes) instead of the confusing --safe-update which force-enabled it - Skip enforcement when no prior manifest exists: first compilation or lock files compiled before the safe updates feature now create the manifest baseline silently instead of blocking all secrets/actions - Update remediation message ordering to lead with actionable --approve flag - Add --approve flag to run and upgrade commands (all compilation paths) - Update all unit and integration tests Fixes #26155
There was a problem hiding this comment.
Pull request overview
This PR addresses confusing safe update UX by renaming the CLI flag and changing how safe update enforcement behaves when no prior manifest baseline exists.
Changes:
- Renames
--safe-updateto--approve(with inverted semantics) and plumbs the new flag throughcompile,run, andupgrade. - Skips safe update enforcement when no prior manifest baseline exists (e.g., first compile / pre-manifest lock files), letting the newly written lock file establish the baseline.
- Updates safe update remediation guidance and adjusts unit/integration tests to reflect the new behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_update_enforcement.go | Skips enforcement when manifest == nil; updates remediation message to lead with --approve. |
| pkg/workflow/safe_update_enforcement_test.go | Updates enforcement expectations and remediation-message assertions; updates effectiveSafeUpdate tests for approve. |
| pkg/workflow/compiler_yaml.go | Implements approve override in effectiveSafeUpdate. |
| pkg/workflow/compiler_types.go | Renames compiler flag plumbing from safeUpdate to approve and adds SetApprove. |
| pkg/cli/compile_config.go | Renames compile config field from SafeUpdate to Approve. |
| pkg/cli/compile_compiler_setup.go | Wires CompileConfig.Approve into compiler setup and logging. |
| cmd/gh-aw/main.go | Replaces --safe-update with --approve on compile, and adds --approve to run. |
| pkg/cli/run_workflow_execution.go | Threads Approve through workflow file collection / recompilation path. |
| pkg/cli/run_push.go | Extends collectWorkflowFiles/recompileWorkflow to accept approve and pass it into compilation config. |
| pkg/cli/run_push_test.go | Updates call sites for new collectWorkflowFiles signature. |
| pkg/cli/run_push_integration_test.go | Updates call sites for new collectWorkflowFiles signature. |
| pkg/cli/upgrade_command.go | Adds --approve to upgrade and threads it into compilation config. |
| pkg/cli/compile_safe_update_integration_test.go | Updates integration tests to reflect “first compile creates baseline silently” and uses --approve where needed. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 5
| // Returns a structured, actionable error when violations are found. | ||
| func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string) error { |
There was a problem hiding this comment.
The doc comment immediately above EnforceSafeUpdate still describes nil manifests being treated as an empty baseline that rejects new secrets/actions. The implementation now returns nil and skips enforcement when manifest is nil, so the comment should be updated to reflect the new behavior (nil manifest => skip enforcement; baseline will be created on write).
| 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") | ||
| 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 |
There was a problem hiding this comment.
Returning nil when manifest == nil will also skip enforcement if the caller failed to parse an existing gh-aw-manifest (e.g., malformed JSON) and left the baseline as nil. That silently disables safe update warnings in cases where we likely want a conservative fallback. Consider reserving nil strictly for "no prior baseline" (no lock file / no manifest header) and having callers pass an explicit empty manifest when a lock file exists but the manifest is unreadable.
| // Set safe update flag: when set via CLI it force-enables safe update enforcement | ||
| // independently of the workflow's strict mode setting. |
There was a problem hiding this comment.
The comment above SetApprove still describes the old behavior (“force-enables safe update enforcement independently of strict mode”). With the rename/inverted semantics, this should describe that --approve disables/skips safe update enforcement regardless of strict mode.
| // Set safe update flag: when set via CLI it force-enables safe update enforcement | |
| // independently of the workflow's strict mode setting. | |
| // Set approve flag: when set via CLI it disables/skips safe update enforcement | |
| // regardless of the workflow's strict mode setting. |
| 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") |
There was a problem hiding this comment.
This change removes the existing --safe-update flag entirely, which is a breaking CLI change for any scripts/users that already pass it (they’ll now get an unknown-flag error). Consider keeping --safe-update as a hidden, deprecated alias (e.g., a no-op that preserves the new default behavior) and optionally printing a deprecation warning directing users to --approve.
| 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") | |
| var approveSafeUpdate bool | |
| compileCmd.Flags().BoolVar(&approveSafeUpdate, "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().BoolVar(&approveSafeUpdate, "safe-update", false, "Deprecated alias for --approve") | |
| 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("safe-update"); err != nil { | |
| // Non-fatal: flag is registered even if MarkHidden fails | |
| _ = err | |
| } | |
| if err := compileCmd.Flags().MarkDeprecated("safe-update", "use --approve instead"); err != nil { | |
| // Non-fatal: flag is registered even if MarkDeprecated fails | |
| _ = err | |
| } |
| upgradeLog.Printf("Running upgrade command: verbose=%v, workflowDir=%s, noFix=%v, noCompile=%v, noActions=%v, skipExtensionUpgrade=%v", | ||
| verbose, workflowDir, noFix, noCompile, noActions, skipExtensionUpgrade) |
There was a problem hiding this comment.
runUpgradeCommand now accepts an approve parameter, but the log line doesn’t include it, so the printed diagnostics are incomplete/misleading. Update the format string and args to include approve=%v (or similar).
| upgradeLog.Printf("Running upgrade command: verbose=%v, workflowDir=%s, noFix=%v, noCompile=%v, noActions=%v, skipExtensionUpgrade=%v", | |
| verbose, workflowDir, noFix, noCompile, noActions, skipExtensionUpgrade) | |
| 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) |
This comment has been minimized.
This comment has been minimized.
|
Hey The PR is well-structured, focused entirely on the flag rename and its follow-on logic, includes both unit and integration tests across all affected paths, and has a thorough description. This looks ready for maintainer review! 🎉
|
Summary
Fixes #26155
Resolves the confusing UX around
--safe-updateand--strictflags by:Changes
Renamed
--safe-updateto--approvewith inverted semantics:--approvenow skips safe update enforcement (approves all changes)--safe-updateconfusingly force-enabled enforcement, which was already on by default via strict modeSkip enforcement when no prior manifest exists: When a lock file was compiled before the safe updates feature (no
gh-aw-manifestcomment), or on first compilation (no lock file at all), enforcement is now skipped silently. The newly generated lock file creates the baseline manifest for future compilations.Updated remediation message to lead with the actionable
--approveflag:Added
--approveflag to all compilation paths:compile,run, andupgradecommandsTesting
EnforceSafeUpdate,effectiveSafeUpdate, andbuildSafeUpdateErrorcollectWorkflowFilesandrecompileWorkflowsignatures and test call sites