Add experimental schema-aware gh aw edit - #55475
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw edit
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add extensive exhaustive integration workflow tests of the cli command |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new edit command still mutates workflow frontmatter in at least one case where the user asked for a semantic no-op: removing on.schedule from a workflow that originally used the short on: syntax rewrites the trigger block into a different shape instead of preserving the original representation.
Blocking theme
unset on.scheduleleavesonexpanded as a map, so workflows can get unrelated source churn from an edit that should only remove scheduling.- I ignored the
grumpy-coderpass because the sub-agent binary was unavailable in this environment.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 6.2 AIC · ⌖ 8.09 AIC · ⊞ 7K
Comment /review to run again
| switch change.kind { | ||
| case "set": | ||
| parent[key] = change.value | ||
| case "unset": |
There was a problem hiding this comment.
Removing on.schedule only deletes the nested key and never collapses on back to its scalar/list form, so a no-op edit can still rewrite untouched triggers into a different YAML shape and create noisy diffs that are unrelated to the requested change.
💡 Why this is risky and how to fix it
editChangeParent eagerly normalizes on: workflow_dispatch and on: [push, workflow_dispatch] into a map so unset on.schedule can operate on parent["schedule"]. After delete(parent, key), nothing converts that synthetic map back to the original scalar/list representation when no structured keys remain.
That means gh aw edit ... --schedule off mutates:
on: workflow_dispatchinto:
on:
workflow_dispatch:which is semantically different formatting for an unchanged trigger set and will cause unnecessary churn in source control. Add a post-edit cleanup step for on that collapses map entries with only nil-valued trigger keys back into the original short form.
There was a problem hiding this comment.
Two small simplifications look worth making here:
- pkg/cli/edit_command.go:177: shrink: YAML-backed assignment parser for simple path=value input. A tiny string/number/bool parser would cut most of the complexity.
- pkg/cli/edit_command.go:190: yagni: generic frontmatter edit engine with six helper layers for one CLI. Inline the few supported mutations until a second caller needs it.
net: -90 lines possible.
Generated by ✂️ Ponytail Reviewer for #55475 · codex · mai10 · 5.69 AIC · ⌖ 0.979 AIC · ⊞ 18.3K
Comment /ponytail to run again
| return editChange{kind: "set", path: "on.schedule", value: []any{map[string]any{"cron": cron}}}, nil | ||
| } | ||
|
|
||
| func parseEditAssignment(assignment, separator string) (editChange, error) { |
There was a problem hiding this comment.
pkg/cli/edit_command.go:177: shrink: YAML-backed assignment parser for simple path=value input. A tiny string/number/bool parser would cut most of the complexity.
| return editChange{kind: "set", path: path, value: value["value"]}, nil | ||
| } | ||
|
|
||
| func applyEditChange(frontmatter map[string]any, change editChange) error { |
There was a problem hiding this comment.
pkg/cli/edit_command.go:190: yagni: generic frontmatter edit engine with six helper layers for one CLI. Inline the few supported mutations until a second caller needs it.
There was a problem hiding this comment.
Pull request overview
Adds an experimental gh aw edit command for schema-aware workflow frontmatter mutations and recompilation.
Changes:
- Adds typed edit, schedule, import, and skill operations.
- Adds validation, dry-run, recompilation, and rollback handling.
- Registers the command and adds CLI tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/edit_command.go |
Implements workflow editing and recompilation. |
pkg/cli/edit_command_test.go |
Tests core edit behavior. |
cmd/gh-aw/main.go |
Registers the new command. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
| if strings.EqualFold(schedule, "off") { | ||
| return editChange{kind: "unset", path: "on.schedule"}, nil | ||
| } | ||
| cron, _, err := parser.ParseSchedule(schedule) |
| case string: | ||
| child = map[string]any{triggers: nil} |
| func replaceFrontmatter(content string, frontmatter map[string]any) (string, error) { | ||
| encoded, err := yaml.MarshalWithOptions(frontmatter, yaml.Indent(2), yaml.IndentSequence(true)) |
| if lockErr != nil && !os.IsNotExist(lockErr) { | ||
| return fmt.Errorf("read generated lock file: %w", lockErr) | ||
| } | ||
| if err := os.WriteFile(workflowPath, []byte(updated), 0o644); err != nil { |
| return writeAndCompileEditedWorkflow(workflowPath, content, updated) | ||
| } | ||
|
|
||
| func writeAndCompileEditedWorkflow(workflowPath string, content []byte, updated string) error { | ||
| lockPath := stringutil.MarkdownToLockFile(workflowPath) | ||
| previousLock, lockErr := os.ReadFile(lockPath) | ||
| lockExisted := lockErr == nil | ||
| if lockErr != nil && !os.IsNotExist(lockErr) { | ||
| return fmt.Errorf("read generated lock file: %w", lockErr) | ||
| } | ||
| if err := os.WriteFile(workflowPath, []byte(updated), 0o644); err != nil { | ||
| return fmt.Errorf("write workflow: %w", err) | ||
| } | ||
| if err := compileWorkflow(context.Background(), workflowPath, false, true, ""); err != nil { |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on correctness risks and test coverage gaps.
📋 Key Themes & Highlights
Key Themes
--schedulesilently replaces all existing cron entries (line 170) — the PR description implies preservation, butkind: "set"overwrites the entire list.- Colon-in-value parsing bug (line 196) —
strings.Cuton:truncates values likegpt-4o:preview; use direct YAML unmarshal. - Silent restore failures in rollback path (line 129) —
_ = os.WriteFile(...)hides errors that leave the working tree inconsistent. - YAML reformatting is a destructive side effect (line 271) — key ordering and comments are lost on every edit; should be documented in
--help. fmt.Sprintcomparison is lossy for non-scalar list values (line 255) — safe today for strings-only lists, but fragile as the generic function evolves.- Rollback path has no test coverage (test line 114) — the most complex behaviour in the PR is entirely untested.
- Over-bundled test (test line 22) — pack too many orthogonal concerns into one case; split for specificity.
Positive Highlights
- ✅ Source-managed workflow guard is clean and well-tested
- ✅
--dry-runis a good safety valve and is tested - ✅
ontrigger coercion (string → map, list → map) handles the most common real-world shapes - ✅ Delegating schedule parsing to
parser.ParseSchedule(after the review comment) is the right design
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 53.9 AIC · ⌖ 11.8 AIC · ⊞ 7.6K
Comment /matt to run again
| if strings.EqualFold(schedule, "off") { | ||
| return editChange{kind: "unset", path: "on.schedule"}, nil | ||
| } | ||
| cron, _, err := parser.ParseSchedule(schedule) |
There was a problem hiding this comment.
[/codebase-design] --schedule silently replaces all existing cron entries — a workflow with two cron triggers loses one without warning.
💡 Details
The PR description says "Preserves existing triggers when adding or removing on.schedule" — this is true for non-schedule triggers (push, pull_request) but the scheduleChange function always produces a kind: "set" that overwrites the entire on.schedule list. At minimum, emit a warning to stderr when existing cron entries are discarded, or introduce --add-schedule / --remove-schedule for additive edits.
@copilot please address this.
| return err | ||
| } | ||
| parent, key, err := editChangeParent(frontmatter, change.path) | ||
| if err != nil { |
There was a problem hiding this comment.
[/codebase-design] parseEditAssignment uses strings.Cut(assignment, separator) which splits on the first occurrence — fine for =, but a YAML inline-value like "model: gpt-4o:preview" would be split at the first :, leaving rawValue = "gpt-4o" and silently dropping :preview.
💡 Suggested fix
For the positional [path: value] argument, consider parsing the entire string as YAML directly (one-key mapping) rather than splitting on : and re-wrapping:
var kv map[string]any
if err := yaml.Unmarshal([]byte(assignment), &kv); err != nil || len(kv) != 1 {
return editChange{}, fmt.Errorf("invalid assignment %q", assignment)
}This lets YAML handle quoting, colons-in-values, etc.
@copilot please address this.
| return nil, err | ||
| } | ||
| changes = append(changes, change) | ||
| } |
There was a problem hiding this comment.
[/tdd] writeAndCompileEditedWorkflow restores the original .md on compile failure, but the restore itself is fire-and-forget (_ = os.WriteFile(...)). If the restore fails (e.g., disk full), the workflow on disk will be in the edited state while the caller receives a compile error — leaving the working tree inconsistent.
💡 Suggested fix
restoreErr := os.WriteFile(workflowPath, content, 0o644)
if restoreErr != nil {
return fmt.Errorf("compile failed (%w); also failed to restore original: %v", err, restoreErr)
}Same pattern for the lock-file restore.
@copilot please address this.
| child, ok := parent[part].(map[string]any) | ||
| if !ok { | ||
| if part == "on" { | ||
| switch triggers := parent[part].(type) { |
There was a problem hiding this comment.
[/codebase-design] editChangeParent creates intermediate maps on demand when a path segment is missing, but never writes those maps back if they were created inside the on special-case branch — specifically, the new child map for string/list on is stored in parent[part], then parent = child, but the map is only persisted for the on key during the next loop iteration. If the path is only one level deep (e.g., on itself), the converted map would be discarded.
💡 Confirm or add a test
Add a test:
frontmatter := map[string]any{"on": []any{"push", "pull_request"}}
applyEditChange(frontmatter, editChange{kind: "set", path: "on.schedule", value: ...})
// verify on still contains push and pull_request keys@copilot please address this.
| child = map[string]any{} | ||
| parent[part] = child | ||
| } | ||
| parent = child |
There was a problem hiding this comment.
[/tdd] applyEditChangeToParent uses fmt.Sprint(value) == fmt.Sprint(change.value) for deduplication/removal — this will silently mismatch for values where fmt.Sprint is lossy (e.g., map[string]any{"cron": "0 * * * *"} becomes "map[cron:0 * * * *]"). For imports and skills (always strings) this is safe today, but the function is generic and used for all list paths.
💡 Suggested fix
Narrow the dedup/removal to fmt.Sprintf("%v", ...) comparisons or use reflect.DeepEqual for correctness, and add a doc comment explaining the limitation:
// String-coerced comparison; works for scalar list elements.
// Do not use for map-typed list entries.@copilot please address this.
| if !ok && parent[key] != nil { | ||
| return fmt.Errorf("cannot add to %q because it is not a list", change.path) | ||
| } | ||
| if !slices.ContainsFunc(values, func(value any) bool { return fmt.Sprint(value) == fmt.Sprint(change.value) }) { |
There was a problem hiding this comment.
[/codebase-design] replaceFrontmatter re-encodes the entire frontmatter map with go-yaml, which will reorder YAML keys alphabetically and strip all comments. For a tool meant to be "safe" to run on human-authored workflows, this is a significant destructive side effect that authors won't expect.
💡 Options
- Accept the limitation and document it clearly in the command's
Longhelp text (e.g., "Note: frontmatter will be reformatted and comments removed"). - Or use a YAML-node–level round-trip library to preserve key order and comments.
At minimum, the PR description and --help output should call this out.
@copilot please address this.
|
|
||
| changes, err := editChangesFromCommand(cmd, []string{"workflow", "model: small"}) | ||
| require.NoError(t, err) | ||
| frontmatter := map[string]any{"on": "workflow_dispatch"} |
There was a problem hiding this comment.
[/tdd] TestEditChangesSupportAssignmentsSchedulesAndImports asserts the expected cron value as a literal fuzzy string ("FUZZY:HOURLY/6 * * *"). If parser.ParseSchedule changes its output format, this test will catch it — but the test name doesn't communicate what behaviour is being verified. The name also packs too many orthogonal concerns into one test.
💡 Suggested split
Split into focused tests following Arrange/Act/Assert:
TestEditSchedule_ConvertsHourlyIntervalTestEditAssignment_SetsStringPathTestEditAddImport_AppendsToList
Each should verify one behaviour with a descriptive name that reads as a specification.
@copilot please address this.
| cmd := NewEditCommand() | ||
| cmd.SetArgs([]string{workflowPath, "max-turns: 20"}) | ||
| assert.ErrorContains(t, cmd.Execute(), "source-managed") | ||
| } |
There was a problem hiding this comment.
[/tdd] There is no test for writeAndCompileEditedWorkflow — specifically, the rollback path (compile failure → restore original .md and lock file) is untested. This is the most complex and highest-risk code in the PR.
💡 Suggested test cases
- Compile fails → original
.mdcontent is restored on disk - Compile fails when lock file existed → lock file is restored
- Compile fails when lock file did not exist → lock file is removed
These can use a table-driven test with a mock compileWorkflow that returns a deliberate error.
@copilot please address this.
Documents the architectural decision to introduce `gh aw edit` as a dedicated, schema-validated mutation path for workflow frontmatter, including alternatives considered and consequences. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR Required — Draft GeneratedStatus: Draft ADR committed to this branch · merge blocked pending review This PR adds 444 new lines to 📄 Decision captured: Introduce What to do next
Why ADRs matter hereADRs create a durable record of why a decision was made, not just what was built. For a CLI command like
This context is especially valuable for experimental features that may evolve or be superseded. ADR sections reference
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Review: gh aw edit (experimental)
Good overall structure — the command is well-scoped, the schema validation before writing is the right approach, and the rollback on compile failure is a thoughtful safety net. Three non-blocking issues found:
- Silent rollback failure (line 110) — if the rollback
os.WriteFilefails, the error is swallowed. A write-then-rename pattern avoids the need for rollback entirely. - Colon-separator ambiguity (line 157) — positional
path: valueargs split on the first colon, so values containing colons (e.g. multi-key YAML) can be misparsed. Worth documenting or hardening. fmt.Sprintdedup (line 280) — theadddedup check is fragile for non-string values;reflect.DeepEqualis more correct.
No blocking issues. The test coverage is solid for the happy paths and several edge cases.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 30.6 AIC · ⌖ 9.3 AIC · ⊞ 6.2K
| _ = os.WriteFile(lockPath, previousLock, 0o644) | ||
| } else { | ||
| _ = os.Remove(lockPath) | ||
| } |
There was a problem hiding this comment.
Reliability: silent rollback failure can leave the workflow file in a broken state
If compileWorkflow fails and one of the rollback os.WriteFile calls also fails (disk full, permission error), the error is silently swallowed with _ = os.WriteFile(...), leaving the user with no diagnostic and potentially a corrupted/partially-written workflow.
Consider using a write-then-rename pattern: write the updated content to a temp file in the same directory, then os.Rename it over the original only after compilation succeeds. That way the original file is never modified until the new version is proven good and no rollback is needed at all.
@copilot please address this.
| } | ||
| if schedule := editFlagString(cmd, "schedule"); schedule != "" { | ||
| change, err := scheduleChange(schedule) | ||
| if err != nil { |
There was a problem hiding this comment.
Correctness: parseEditAssignment with : separator will misparse values containing colons
Using strings.Cut(assignment, ":") to parse positional args like "on.schedule: daily" will stop at the first colon. This is intentional for simple values, but values like "trigger: push: [main]" or any YAML value containing a colon would be split at the wrong position.
The current approach parses the right-hand side as YAML after the cut, so the value daily: 10 would only hand daily to the YAML parser. Consider documenting this limitation clearly, or using a regex/index-of-first-space approach to split path: rest more robustly.
@copilot please address this.
| return fmt.Errorf("cannot remove from %q because it is not a list", change.path) | ||
| } | ||
| parent[key] = slices.DeleteFunc(values, func(value any) bool { return fmt.Sprint(value) == fmt.Sprint(change.value) }) | ||
| default: |
There was a problem hiding this comment.
Harden: add dedup uses fmt.Sprint equality, which is fragile for non-string values
if !slices.ContainsFunc(values, func(value any) bool { return fmt.Sprint(value) == fmt.Sprint(change.value) }) {fmt.Sprint on maps/slices is order-dependent and not a stable equality check. For the imports/skills cases the values will always be strings, but this function is generic and could be called with on.schedule or nested objects in the future. Using fmt.Sprintf("%v", ...) on a map[string]any produces Go's default map printing which has non-deterministic key ordering.
Consider restricting dedup to string-typed values, or using a proper reflect.DeepEqual comparison.
@copilot please address this.
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32774702076
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Forward-progress pass done in b7f2e0f/latest commits, addressing the blocking review and the open reviewer threads:
The compact schedule forms (
Correction on the commit references in my previous reply: the follow-up pass landed in 502dfcd and 6260e69. |
Adds an experimental control-plane command for safely changing workflow frontmatter without manually editing Markdown. It supports typed mutations, schema validation, recompilation, and source-managed workflow protection.
Command surface
gh aw edit <workflow> [path: value]--set,--unset,--add,--remove,--schedule, and--add-import.mdfilenames, and paths consistently with existing commandsWorkflow-safe edits
.lock.ymlsource:Schedules and imports
1h,3h,1d,1w, andweekdayson.scheduleimports.awobject formsgh aw edit repo-assist "max-turns: 20" gh aw edit repo-assist --schedule weekdays gh aw edit repo-assist --add-import shared/common.md gh aw edit repo-assist --unset max-ai-creditsgh aw editfor schema-aware workflow configuration changes #55462