Allow gh aw edit on source-managed workflows - #55875
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
gh aw edit on source-managed workflows
|
Category: bug | Risk: medium | Score: 47/100 (impact 22/50, urgency 12/30, quality 13/20) | Action: batch_review | Batch: workflow-runtime
|
|
Triage update: Category: bug | Risk: medium | Score: 42/100 (impact 20 + urgency 12 + quality 10) | Action: batch_review | Batch: workflow-runtime
|
|
Triage update: Category: bug | Risk: medium | Score: 42/100 (impact 20 + urgency 12 + quality 10) | Action: batch_review | Batch: workflow-runtime
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #55875 does not have the implementation label and has only 21 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
REQUEST_CHANGES
The behavioral change here is bigger than the diff suggests: once gh aw edit starts mutating source: workflows, the update/merge path becomes part of the contract, and this PR does not test that those local frontmatter edits survive a later gh aw update.
Blocking theme
The new tests only prove that edit can write a source: workflow and preserve the source field immediately afterward. They do not cover the follow-on update behavior that the CLI help now explicitly promises (“future updates will merge in those local changes”). Without an integration test through the merge/update path, this is one refactor away from silently dropping user overrides.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 19.8 AIC · ⌖ 6.95 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Allows local edits to source-managed workflows while retaining source metadata and recompiling outputs.
Changes:
- Removes the source-managed edit restriction.
- Updates CLI help and test coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/edit_command.go |
Enables and documents managed workflow edits. |
pkg/cli/edit_command_test.go |
Tests dry-run source retention. |
pkg/cli/edit_command_integration_test.go |
Tests persisted edits and compilation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| The workflow-id may be a workflow name, a Markdown filename, or a path. Changes are | ||
| validated before writing. Workflows managed by a source: declaration cannot be edited. | ||
| validated before writing. Workflows managed by a source: declaration can be edited | ||
| locally; future updates will merge in those local changes. |
There was a problem hiding this comment.
Updated docs/adr/55475-schema-validated-workflow-frontmatter-edit-command.md so the ADR matches the new source-managed edit behavior and documents the managed source: limitation. Commit: ae3aa46.
| validated before writing. Workflows managed by a source: declaration can be edited | ||
| locally; future updates will merge in those local changes. |
There was a problem hiding this comment.
Added a guard that rejects top-level source edits on already source-managed workflows, plus unit coverage for --set source, nested source.*, and --unset source. Commit: ae3aa46.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /grill-with-docs — one minor comment, otherwise clean.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: the overly-restrictive guard is removed at the right layer; the fix is minimal and surgical.
- Help text accuracy: the updated Long description is mostly correct but omits the
--no-mergecaveat (see inline comment).
Positive Highlights
- ✅ Both unit and integration tests updated to cover the new behaviour —
source:preservation verified explicitly in the integration test. - ✅ The unit test correctly uses
--dry-runto verify the file is not mutated, and separately checks the output contains the expected frontmatter keys — clean Arrange/Act/Assert structure. - ✅ The integration test asserts
frontmatter["source"]is preserved after the edit, which is the critical invariant for this change. - ✅ PR description clearly explains the motivation and reconciliation model (
gh aw updatemerges local changes).
The single inline comment asks for a small docs clarification around --no-merge. No blocking issues.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 27.4 AIC · ⌖ 9.91 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Review: Allow gh aw edit on source-managed workflows
Impeccable modes applied: harden, audit
The change is small, well-scoped, and correctly tested. Removing the hard block in favour of the update command's existing 3-way merge is a reasonable UX improvement.
Non-blocking suggestion
There is no runtime hint to the user when they edit a source-managed workflow. Someone who runs gh aw edit on a managed workflow and later runs gh aw update --no-merge will silently lose their edits. A one-line warning when parsed.Frontmatter["source"] is set (e.g. cmd.PrintErrln(...)) would surface the risk at the right moment without blocking the operation.
Otherwise the implementation looks correct. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 19.1 AIC · ⌖ 9.06 AIC · ⊞ 6.2K
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32979958387
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the follow-up pass in ae3aa46: added source-managed edit/update-merge regression coverage, protected managed |
gh aw editrejected workflows withsource:frontmatter, even though local edits should be allowed and later reconciled bygh aw update.Edit behavior
gh aw edit.source:metadata while applying local frontmatter edits.CLI help
Coverage
source:”.Example: