Add --dry-run mode to update and upgrade commands and dashboard#42361
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a --dry-run preview mode to gh aw update and gh aw upgrade, wires it into MCP tools (with dry_run defaulting to true for safe elicitation), and exposes a “Dry run” toggle in the Agentic Workflows Dashboard Maintenance tab so users can preview maintenance actions before mutating files.
Changes:
- Add
--dry-runflag toupdateandupgradeCLI commands and short-circuit file-mutating steps while emitting “(dry run) Would …” output. - Extend MCP management tooling with
dry_runinput (defaulttrue) and introduce a newupgradeMCP tool. - Update dashboard UI/state to default to dry-run and append
--dry-runto maintenance run commands when enabled.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/upgrade_command.go | Adds --dry-run flag and a dry-run early-exit path with “Would …” messaging. |
| pkg/cli/update_workflows.go | Adds DryRun option and skips workflow file writes/compilation when enabled. |
| pkg/cli/update_command.go | Adds --dry-run flag plumbed into update options and prints preview header. |
| pkg/cli/mcp_tools_management.go | Adds dry_run to MCP update tool schema (default true) and introduces MCP upgrade tool. |
| pkg/cli/mcp_server.go | Registers update/upgrade MCP tools with error handling. |
| pkg/cli/mcp_server_unit_test.go | Updates expected tool list to include upgrade. |
| pkg/cli/mcp_server_tools_test.go | Updates expected tool list to include upgrade. |
| .github/extensions/agentic-workflows-dashboard/web/index.html | Adds a Dry run checkbox and a “Dry run” badge in Maintenance UI. |
| .github/extensions/agentic-workflows-dashboard/web/app.js | Builds in dry-run default state and appends --dry-run to run commands. |
| .github/extensions/agentic-workflows-dashboard/src/app.ts | Source changes for dashboard dry-run state and command construction. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Low
| targetRepo, _ := cmd.Flags().GetString("repo") | ||
| targetOrg, _ := cmd.Flags().GetString("org") | ||
| repoGlobs, _ := cmd.Flags().GetStringSlice("repos") | ||
| dryRun, _ := cmd.Flags().GetBool("dry-run") | ||
|
|
| approveUpgrade, _ := cmd.Flags().GetBool("approve") | ||
| preReleases, _ := cmd.Flags().GetBool("pre-releases") | ||
| targetOrg, _ := cmd.Flags().GetString("org") | ||
| repoGlobs, _ := cmd.Flags().GetStringSlice("repos") | ||
| dryRun, _ := cmd.Flags().GetBool("dry-run") |
| cmd.Flags().Bool("create-issue", false, "Open a GitHub issue in each org repository that has pending workflow updates (requires --org)") | ||
| cmd.Flags().BoolP("yes", "y", false, "Auto-accept org-mode create confirmations (required in CI)") | ||
| cmd.Flags().String("cool-down", "7d", coolDownFlagUsage) | ||
| cmd.Flags().Bool("dry-run", false, "Preview updates without applying any changes") |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 2 ( Notes: Both changes are single-line updates adding Verdict
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (166 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add --dry-run mode to update and upgrade commands and dashboard
The feature design is solid — --dry-run defaults to true in MCP elicitation and the dashboard, all file-writing steps are correctly gated, and the output messages accurately mirror what each skipped step would have done.
One correctness bug needs to be fixed before merge; two smaller issues are also flagged.
Issues found (3)
| Severity | File | Issue |
|---|---|---|
| 🔴 Bug | mcp_server.go:93 |
Early return on registerUpdateTool failure skips registerFixTool entirely |
| 🟡 Minor | mcp_tools_management.go:114 |
AddSchemaDefault error is swallowed — silent failure removes the dry-run-on-by-default guarantee |
| 🟡 Minor | web/app.js:3312 |
days field additions to reportWindows are not present in the app.ts source |
What's good:
- Dry-run messages in both
updateandupgradeaccurately track the conditions (noFix,noActions,noCompile) of the real steps they shadow. - Security scanning still runs in dry-run mode (correct — it's a read-only check).
- MCP
upgradetool is properly registered and tested in both test files. - Dashboard defaults (
dryRun: true) and theLabel--attentionbadge are well-wired.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 60.3 AIC · ⌖ 6.92 AIC · ⊞ 4.9K
| registerUpdateTool(server, execCmd) | ||
| if err := registerUpdateTool(server, execCmd); err != nil { | ||
| mcpLog.Printf("Failed to register update tool: %v", err) | ||
| return server |
There was a problem hiding this comment.
Bug: early return skips registerFixTool (and any tools registered after it).
When registerUpdateTool fails — e.g. if GenerateSchema returns an error — the function returns the partially-configured server at this line, so registerUpgradeTool and registerFixTool are never called. The MCP server would be silently missing the fix tool (and the upgrade tool).
Consider logging the error and continuing rather than returning early:
if err := registerUpdateTool(server, execCmd); err != nil {
mcpLog.Printf("Failed to register update tool: %v", err)
// fall through — other tools must still be registered
}
if err := registerUpgradeTool(server, execCmd); err != nil {
mcpLog.Printf("Failed to register upgrade tool: %v", err)
}
registerFixTool(server, execCmd)If a hard-stop is truly required, at minimum move registerFixTool before the early returns so it is always registered.
@copilot please address this.
| } | ||
| // Add elicitation default: dry_run defaults to true (safe preview mode) | ||
| if err := AddSchemaDefault(updateSchema, "dry_run", true); err != nil { | ||
| mcpToolsManagementLog.Printf("Failed to add default for dry_run: %v", err) |
There was a problem hiding this comment.
Silent failure undermines the dry-run-default UX.
AddSchemaDefault errors are only logged and then swallowed. If this call fails, dry_run will have no default value in the JSON schema, so the elicitation UI will not show the checkbox pre-checked — exactly the safety property this PR is trying to guarantee.
Consider returning the error so the caller (registerUpdateTool) can propagate it:
if err := AddSchemaDefault(updateSchema, "dry_run", true); err != nil {
return fmt.Errorf("failed to add dry_run default: %w", err)
}Same pattern applies to the equivalent block in registerUpgradeTool (~line 283).
@copilot please address this.
| { id: "3d", label: "3 days", startDate: "-3d" }, | ||
| { id: "7d", label: "7 days", startDate: "-1w" }, | ||
| { id: "1mo", label: "1 month", startDate: "-1mo" } | ||
| { id: "3d", label: "3 days", startDate: "-3d", days: 3 }, |
There was a problem hiding this comment.
Unrelated change in compiled output not present in the source (app.ts).
The days field addition to reportWindows entries appears in app.js but has no corresponding change in app.ts. If app.js is a compiled artefact, it should be regenerated from source and never manually edited — otherwise the build is no longer reproducible.
Please verify:
- Does
app.tsactually contain thesedaysfields (perhaps pre-existing, not shown in the diff)? - If not, either add the matching source change to
app.tsand recompile, or remove the manual edit fromapp.js.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 issues found in dry-run wiring
The dry-run feature is well-structured overall, but there are two correctness issues in the MCP registration path and one logic bug in the upgrade command.
🔍 Blocking issues
1. — silently dropped on error path
When or fails (schema-generation error), is never called. The server is returned without the tool — a regression from the previous code where was always registered. See inline comment.
2. — error swallowed in both registrations
If fails for , execution continues and the tool is registered without the default. The stated invariant ("dry_run defaults to true for safe preview mode") is silently violated in both and . See inline comment.
**3. — fires incorrectly when **
Prints "(dry run) Would check gh-aw extension version" even when the extension step was explicitly bypassed by the internal re-launch flag. See inline comment.
i️ Pre-existing issues noted by earlier reviewers (not counted above)
-
- / silently creates empty PRs/issues in org mode (already flagged on update and upgrade commands).
- Integration tests still assert is not a valid flag for (already flagged).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 96.3 AIC · ⌖ 7.63 AIC · ⊞ 5.2K
Comment /review to run again
| registerUpdateTool(server, execCmd) | ||
| if err := registerUpdateTool(server, execCmd); err != nil { | ||
| mcpLog.Printf("Failed to register update tool: %v", err) | ||
| return server |
There was a problem hiding this comment.
registerFixTool is silently skipped on schema-generation failure — partially-configured server returned. If registerUpdateTool (or registerUpgradeTool) fails, the function returns before registerFixTool is called on line 99, so the server is missing the fix tool. This is a regression: previously fix was unconditionally registered.
💡 Suggested fix
Move registerFixTool before the guarded registrations, or register remaining tools even on error:
registerAddTool(server, execCmd)
registerFixTool(server, execCmd) // move up — always safe
if err := registerUpdateTool(server, execCmd); err != nil {
mcpLog.Printf("Failed to register update tool: %v", err)
return server
}
if err := registerUpgradeTool(server, execCmd); err != nil {
mcpLog.Printf("Failed to register upgrade tool: %v", err)
return server
}GenerateSchema fails only on programmer error (malformed struct tags), so this path will not be triggered in production — but the current code silently drops fix if it ever does.
| } | ||
| // Add elicitation default: dry_run defaults to true (safe preview mode) | ||
| if err := AddSchemaDefault(updateSchema, "dry_run", true); err != nil { | ||
| mcpToolsManagementLog.Printf("Failed to add default for dry_run: %v", err) |
There was a problem hiding this comment.
AddSchemaDefault failure is silently swallowed — dry_run may not default to true without any diagnostic. The error is logged and then discarded; registerUpdateTool continues and registers the tool with an incomplete schema. The "safe preview mode" invariant (dry_run=true by default) is silently violated.
💡 Suggested fix
Return the error so the caller can handle it, consistent with the pattern used for GenerateSchema just above:
if err := AddSchemaDefault(updateSchema, "dry_run", true); err != nil {
mcpToolsManagementLog.Printf("Failed to add default for dry_run: %v", err)
return err // propagate instead of silently continuing
}The same issue exists in registerUpgradeTool at the analogous AddSchemaDefault call (line ~283).
| @@ -237,6 +246,24 @@ func runUpgradeCommand(opts upgradeOptions) error { | |||
| // Signal the entry-point to exit cleanly without repeating those steps. | |||
| return &ExitCodeError{Code: 0} | |||
| } | |||
| } else if opts.dryRun { | |||
There was a problem hiding this comment.
Misleading dry-run message when --skip-extension-upgrade is set. The else if opts.dryRun branch fires for both skipExtensionUpgrade=false (normal dry-run) and skipExtensionUpgrade=true (internal re-launch after a real upgrade). In the latter case, printing "(dry run) Would check gh-aw extension version" is wrong — the extension check was already intentionally bypassed.
💡 Suggested fix
Guard the dry-run message with the same skip flag used in the non-dry-run path:
} else if opts.dryRun && !opts.skipExtensionUpgrade {
upgradeLog.Print("Dry-run mode: skipping extension version check")
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("(dry run) Would check gh-aw extension version"))
}--skip-extension-upgrade is a hidden internal flag only used when the binary re-launches itself, so this edge case will not be user-visible in practice — but the logic is semantically wrong and could confuse future readers or automated tests.
|
@copilot run pr-finisher skill |
gh aw updateandgh aw upgradehad no way to preview what they would do before modifying files. Adds--dry-runto both commands and wires it through the MCP tools (defaulting to checked) and the dashboard Maintenance tab.CLI
update --dry-run: skips file writes inupdateWorkflow; skipsUpdateActions,UpdateActionsInWorkflowFiles,UpdateContainerPins, and recompile inRunUpdateWorkflowsupgrade --dry-run: skips extension self-upgrade, Copilot artifact update, codemods, action version updates, and recompile; prints a(dry run) Would …line for each skipped stepMCP tools
updatetool: addeddry_runfield; schema now usesAddSchemaDefault(…, "dry_run", true)so the checkbox is checked by defaultupgradeMCP tool with the same dry-run-default schema pattern; registered inmcp_server.goDashboard
buildMaintenanceCommandappends--dry-runtorun-update/run-upgradewhen the checkbox is onLabel--attentionbadge shown when dry run is active