fix: rename --safe-update to --approve and improve safe update UX#26160
Open
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
Contributor
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
Comment on lines
45
to
46
| // 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).
Comment on lines
+46
to
+50
| 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.
pkg/cli/compile_compiler_setup.go
Outdated
Comment on lines
159
to
160
| // 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.
Suggested change
| // 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. |
cmd/gh-aw/main.go
Outdated
Comment on lines
697
to
699
| 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.
Suggested change
| 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 | |
| } |
pkg/cli/upgrade_command.go
Outdated
Comment on lines
146
to
147
| 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).
Suggested change
| 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.
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! The --approve flag rename improves UX clarity. Both renamed fields are consistent throughout the codebase. Run 24408518985.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #26231
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #26229
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none💥 [THE END] — Illustrated by Smoke Claude · ● 362.4K
cmd/gh-aw/main.go
Outdated
| noCheckUpdate, _ := cmd.Flags().GetBool("no-check-update") | ||
| scheduleSeed, _ := cmd.Flags().GetString("schedule-seed") | ||
| safeUpdate, _ := cmd.Flags().GetBool("safe-update") | ||
| approve, _ := cmd.Flags().GetBool("approve") |
Contributor
There was a problem hiding this comment.
🤖 Smoke test review comment — Run 24408518985. The rename from --safe-update to --approve is a semantically clearer name: it tells the user they're approving changes, rather than describing an internal mechanism. Good ergonomics improvement!
| 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. |
Contributor
There was a problem hiding this comment.
🤖 Smoke test review comment — Run 24408518985. The updated field comment is more precise — "skipping safe update enforcement" is actionable guidance for the caller compared to the previous "force-enable safe update mode" which described the old inverted semantics.
Collaborator
|
@copilot safe-update enforcement needs to be done on first compilation (or with legacy scripts) as well to handle the first code generation run as well. |
…empty baseline) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/dfe33092-16cf-424a-b8ef-5efacd5611ef Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Done in d0bd501. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Collaborator
|
@copilot rename --approve to --approve-updates |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/eac7b5e7-8676-4993-a097-9931b95c3ec1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Done in b2562f0. Renamed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Collaborator
|
@copilot review all changes for consistency, simplicity, security. Update PR description with better summary |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8dc906ee-3964-4338-916e-aeff2587eae9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Reviewed all changes (0603b45). Two consistency issues fixed:
Also updated the PR description with a clearer summary of all changes. |
Contributor
|
Hey The PR is well-structured, covers all compilation paths, includes comprehensive unit and integration tests, and has a clear description linking back to the root issue. Everything looks ready for maintainer review. 🎉
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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✨ PR Review Safe Output Test - Run 24408518985
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".To allow these resources, lower
min-integrityin your GitHub frontmatter: