SPDD 2026-07-08: close spec sync gaps and enforce add-flow rollback semantics#44358
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Status: DRAFT — needs undraft + CI before review. Daily SPDD spec sync: closes gaps in safe-outputs, guard-policies, model-alias, and repository-package specs. Also hardens Run §28966928999
|
🤖 PR Triage
Score breakdown: Impact 20 + Urgency 15 + Quality 10 Rationale: DRAFT. SPDD spec-sync work — closes spec gaps and enforces add-flow rollback semantics. Useful maintenance but no user-visible urgency. Defer until undraft + CI. Run §28986426320
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44358 does not have the 'implementation' label and has only 55 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Updates several project specifications to close SPDD sync gaps and hardens gh aw add behavior by rolling back files written in the same add operation when a later workflow write fails.
Changes:
- Synced multiple specs (safe-outputs, guard-policies scratchpad, model-alias, repository-package manifest) with explicit implementation cross-references and refreshed verification dates.
- Added guard-policy validation coverage for
allowed-repos: "public"withoutmin-integrity. - Implemented and tested rollback semantics in
addWorkflowsWithTracking()when a later workflow write fails.
Show a summary per file
| File | Description |
|---|---|
| scratchpad/guard-policies-specification.md | Updates deprecation tracking reference and adds GP-11 implementation/test mapping entry. |
| pkg/workflow/tools_validation_test.go | Adds regression test for allowed-repos: public requiring min-integrity. |
| pkg/cli/add_command.go | Adds rollback-on-failure behavior during multi-workflow add operations. |
| pkg/cli/add_command_test.go | Adds test asserting earlier successful writes are removed when a later write fails. |
| docs/src/content/docs/specs/safe-outputs-specification.md | Adds handler-function cross-reference table and expands §8.3 sync mappings for MCE1–MCE5. |
| docs/src/content/docs/specs/repository-package-manifest-specification.md | Clarifies removal scope for package README.md and maps R-PKG-003 rollback coverage to implementation/tests. |
| docs/src/content/docs/specs/model-alias-specification.md | Adds normative “Operations” lifecycle section and updates TOC / last-verified date. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Low
| | `update_release` | `defaultHandler("update_release")` | | ||
| | `upload_asset` | `uploadAssetHandler` | | ||
| | `dispatch_workflow` | `defaultHandler("dispatch_workflow")` | | ||
| | `create_code_scanning_alert` | `defaultHandler("create_code_scanning_alert")` | | ||
| | `autofix_code_scanning_alert` | `defaultHandler("autofix_code_scanning_alert")` | | ||
| | `create_check_run` | `defaultHandler("create_check_run")` | | ||
| | `create_agent_session` | `defaultHandler("create_agent_session")` | | ||
| | `missing_tool` | `defaultHandler("missing_tool")` | |
| if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { | ||
| if tracker != nil { | ||
| if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { | ||
| return fmt.Errorf("failed to add workflow '%s' (rollback also failed): %w", resolved.Spec.String(), errors.Join(err, rollbackErr)) | ||
| } | ||
| } | ||
| return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
Verdict
Score formula: (2/2 × 40) + (2/2 × 30) + (20 − 0) + 0 [inflation penalty] = 90 References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /grill-with-docs — requesting changes on one correctness issue and three quality gaps.
📋 Key Themes & Highlights
Key Themes
- Double-rollback hazard (correctness): The new inner rollback in
addWorkflowsWithTrackingand the existing outer rollback inadd_workflow_pr.gooperate on the sameFileTrackerinstance.FileTrackerdoes not clear its lists after rollback, so the outer call redundantly re-writes original content. This is the highest-priority finding. - Rollback-failure test gap: The test verifies the happy-path rollback (file is gone) but has no coverage for partial-rollback failure, and the
errors.Joinwrapping in the rollback-failure branch may not behave as expected for callers usingerrors.Is. - Implicit test fixture: The rollback test relies on an undocumented implementation detail (nested dir not existing) to trigger the write failure; a future auto-mkdir change would silently break the test contract.
- Spec: normative prose without IDs: The new README-removal paragraph in the repository-package spec adds normative behaviour without a requirement ID, making it un-referenceable from the norms table.
- Spec: §16 placement and cycle-check wording: The Operations section is appended after the norms/appendices rather than in the main body, and the cycle-detection step has ambiguous wording that could mislead implementors.
Positive Highlights
- ✅ Rollback semantics are now aligned with R-PKG-003 — a meaningful hardening of
gh aw add. - ✅ The §7.0.2 handler cross-reference table is a high-value maintenance aid.
- ✅ GP-11 test coverage closes a real spec-to-code sync gap cleanly.
- ✅ Replacing stale "[Open]" tasks with a concrete maintenance note reduces future drift.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 102.6 AIC · ⌖ 6.46 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { | ||
| if tracker != nil { | ||
| if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] Double-rollback risk: addWorkflowsWithTracking now calls tracker.RollbackAllFiles internally, but add_workflow_pr.go (line 93) also calls tracker.RollbackAllFiles on the same tracker when the error returns. Because FileTracker does not clear its lists after rollback, RollbackModifiedFiles re-writes the original content a second time — harmless today but semantically broken and fragile if tracker state is checked post-rollback.
💡 Suggested fix
Clear the tracker lists after a successful rollback so a second call is a no-op:
// After rollback logic in RollbackAllFiles:
ft.CreatedFiles = nil
ft.ModifiedFiles = nil
ft.OriginalContent = nilAlternatively, guard the outer call in add_workflow_pr.go with a comment noting the inner rollback has already been attempted.
@copilot please address this.
| return fmt.Errorf("failed to add workflow '%s' (rollback also failed): %w", resolved.Spec.String(), errors.Join(err, rollbackErr)) | ||
| } | ||
| } | ||
| return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err) |
There was a problem hiding this comment.
[/tdd] Rollback success is not asserted — the test only checks that ok.md does not exist after the error, but it doesn't cover the case where rollback itself fails (partial rollback). The error message for rollback failure uses errors.Join(err, rollbackErr) with %w, which wraps only the first argument under Go's errors.Join semantics; the caller can only errors.Is the original write error, not the rollback error.
💡 Suggested additions
- Add a test case that makes rollback fail (e.g. by making the already-written file read-only) and asserts the combined error message contains both the write error and rollback error strings.
- Consider returning a dedicated type that carries both errors distinctly, or at minimum confirm the wrapping is intentional so
errors.Is(err, writeErr)works as expected.
@copilot please address this.
| Quiet: true, | ||
| }) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to write destination file") |
There was a problem hiding this comment.
[/tdd] The test relies on WorkflowName: "nested/blocked" to trigger a write failure, but the failure mechanism isn't explicit — it's relying on an implementation detail (presumably the nested sub-directory not existing causes the write to fail). This makes the test fragile and unclear: if the implementation starts auto-creating parent directories, the test silently stops exercising rollback.
💡 Suggested improvement
Make the failure condition explicit in the test name and comment, or introduce a test-friendly injection point (e.g., a mock writer) so the cause of failure is decoupled from directory creation behaviour:
// WorkflowName "nested/blocked" causes a write failure because the parent
// directory "nested/" does not exist under workflowsDir and is not created
// by the implementation. If that behaviour changes, this test must be updated.@copilot please address this.
|
|
||
| **R-PKG-R004**: After removal, if the target workflow directory is empty, the implementation MAY remove the empty directory. The implementation MUST NOT remove non-empty directories. | ||
|
|
||
| For R-PKG-R001 scope, package-installed documentation files are treated the same as workflow files: a `README.md` installed as part of the package MUST be removed during package removal unless it has been modified, in which case R-PKG-R002 applies. |
There was a problem hiding this comment.
[/grill-with-docs] The new prose for R-PKG-R001 scope uses the term README.md inline without a requirement ID, making it normative prose that can't be directly referenced in the norms table or sync mapping. The new §12 sync row already references R-PKG-003 (install rollback) but the README-removal behaviour this paragraph describes maps to the existing R-PKG-R001/R002 requirements — the paragraph should reference those explicitly rather than restating them.
💡 Suggestion
Replace the free-form paragraph with a cross-reference:
> **Note**: Documentation files installed by the package (e.g., `README.md`) are within
> the scope of R-PKG-R001 and follow the same modified-file protection as R-PKG-R002.This keeps the statement tied to existing requirement IDs so tooling and the norms table stay consistent.
@copilot please address this.
| 2. **Initialize Guards**: The implementation MUST initialize a depth counter (starting at 0) and a per-call visited-set before traversing alias references. | ||
| 3. **Resolve Recursively**: For each alias hop, the implementation MUST increment depth, merge parameters per §8.3, and reject cycles when the next alias key already exists in the visited-set. | ||
| 4. **Enforce Limits**: The implementation MUST fail with a resolution error when depth exceeds the safeguard ceiling defined by R-MAF-S001. | ||
| 5. **Return Deterministically**: The implementation MUST return the first conforming concrete model selected by §8 ordering rules, or a descriptive error when no candidate can be resolved. |
There was a problem hiding this comment.
[/grill-with-docs] The new §16 Operations section is positioned after §15 Norms (the normative reference table) but references requirement IDs defined in §13 (e.g., R-MAF-S001). Placing the Operations section after the norms table and appendices is unusual — readers expect lifecycle/operational steps to come before the reference tables, not after. Also, step 3 says "reject cycles when the next alias key already exists in the visited-set" but the condition should be the current key, not the next key — misreading could cause implementors to skip the last-hop cycle check.
💡 Suggestions
- Move §16 Operations to sit between §13 Safeguards and §14 Sync Notes to match the natural reading flow.
- Clarify step 3: "reject cycles when the current alias key already exists in the visited-set (before adding it)".
@copilot please address this.
There was a problem hiding this comment.
Review: SPDD 2026-07-08 spec sync + add-flow rollback
Overall: The spec sync work (safe-outputs handler cross-reference table, MCE1–MCE5 wiring notes, model-alias §16 Operations, repository-package README clarification, guard-policy GP-11 test) is well-structured and high-signal. Two blocking issues in the Go change need addressing before merge.
Blocking issues
-
Double-rollback hazard —
addWorkflowsWithTrackingnow rolls back internally on write failure, but all callers inadd_workflow_pr.go(lines 93, 102, 171) also calltracker.RollbackAllFiles()on the error return. The inner rollback deletes the files first, so the outer one is dead code. This creates ambiguous ownership of rollback semantics and risks confusing future callers. Rollback ownership must be consolidated to one side. See inline comment onpkg/cli/add_command.go:310. -
Fragile test failure condition —
TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailuretriggers failure via an undocumented side-effect:WorkflowName: "nested/blocked"writes to a subdirectory that doesn't exist, soos.WriteFilefails with a path error. If nested workflow auto-mkdir is ever added (a natural improvement), the test silently passes without testing rollback. The failure mechanism should be made explicit. See inline comment onpkg/cli/add_command_test.go:659.
Non-blocking observations
- The safe-outputs §7.0.2 handler cross-reference table is a welcome addition, but
close_check_runandpush_to_pull_request_branchboth use non-trivial handler functions — double-check these are correctly mapped. - Model-alias §16 Operations steps 3–5 accurately describe the existing implementation. No issues.
- Guard policy GP-11 test coverage correctly matches the
allowed-repos non-null without min-integrityvalidation path intools_validation_github.goline 131–134.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 107.4 AIC · ⌖ 6.58 AIC · ⊞ 4.8K
| if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { | ||
| if tracker != nil { | ||
| if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { | ||
| return fmt.Errorf("failed to add workflow '%s' (rollback also failed): %w", resolved.Spec.String(), errors.Join(err, rollbackErr)) |
There was a problem hiding this comment.
Double-rollback hazard introduced by this change.
After this PR, addWorkflowsWithTracking rolls back internally on failure (lines 308-312). But every existing caller that already calls tracker.RollbackAllFiles() on error — e.g., add_workflow_pr.go line 93 — will now trigger a second rollback on the same tracker after addWorkflowsWithTracking returns the error.
The second rollback silently no-ops (files already deleted), but it is misleading. Rollback ownership should be consistent: either the callers own rollback (revert the internal rollback added here), or addWorkflowsWithTracking owns it (remove the external RollbackAllFiles calls in callers).
@copilot please address this.
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to write destination file") | ||
|
|
||
| _, statErr := os.Stat(filepath.Join(workflowsDir, "ok.md")) |
There was a problem hiding this comment.
Test relies on an undocumented failure condition that may be fragile.
The test expects WorkflowName: "nested/blocked" to fail with "failed to write destination file". Looking at the code, this fails because nested/blocked implies a subdirectory (.github/workflows/nested/) that does not exist in the test setup, so os.WriteFile fails.
This is an implicit coupling: the test passes only because os.MkdirAll is not called for the workflow subdirectory before writing. If future refactoring adds auto-mkdir for nested workflow names (which is a natural improvement), this test silently stops covering rollback.
Recommend: make the failure condition explicit — e.g., create a read-only file at the target path or use a stub writer — rather than relying on filesystem path creation failure.
@copilot please address this.
There was a problem hiding this comment.
Two issues need addressing before merge — one is a maintenance contract hazard in the rollback implementation, one is a fragile test.
Review themes
Rollback ownership is split across two callers (high)
The new rollback code inside addWorkflowsWithTracking shares a FileTracker with add_workflow_pr.go, which already calls tracker.RollbackAllFiles on the same error path. This creates a double-rollback that is accidentally safe today but violates clear ownership of the tracker's lifecycle. See inline comment on add_command.go.
Fragile test failure mechanism (medium)
The test for rollback relies on nested/blocked triggering a write failure through a missing directory — this is correct but undocumented, and will silently stop exercising rollback if the implementation ever adds MkdirAll for workflow writes (as it already does for skill/agent files). See inline comment on add_command_test.go.
Rest of the diff is solid
The spec updates (safe-outputs §7.0.2 handler table, §8.3 MCE mappings, model-alias §16 operations, pkg-manifest §5.3 README clarification) are accurate, well-scoped, and improve traceability. Guard policy test coverage for GP-11 correctly targets the allowed-repos non-all without min-integrity code path.
🔎 Code quality review by PR Code Quality Reviewer · 138.3 AIC · ⌖ 5.48 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { | ||
| if tracker != nil { | ||
| if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { |
There was a problem hiding this comment.
Double rollback when caller already runs RollbackAllFiles on the same tracker: add_workflow_pr.go calls tracker.RollbackAllFiles at line 93 after addWorkflowsWithTracking returns an error — which now also calls RollbackAllFiles internally. The second invocation re-runs on an already-rolled-back tracker.
💡 Impact and suggested fix
Today this is safe by coincidence:
RollbackCreatedFilesignoresos.IsNotExist, so re-deleting an already-deleted file is a no-op.RollbackModifiedFilesre-writes original content that was already restored, so the result is the same.
But the invariant is broken: the tracker is never cleared after a rollback, and its state looks identical to a pre-rollback state. Any future change to RollbackAllFiles or RollbackModifiedFiles that adds state-dependent logic (e.g., "only restore files not already restored", or clearing the lists post-rollback) will silently change behaviour for the outer caller in add_workflow_pr.go.
Option A (preferred): Move rollback responsibility entirely into addWorkflowsWithTracking and remove the redundant outer call in add_workflow_pr.go line 93.
Option B: After a successful rollback inside addWorkflowsWithTracking, clear the tracker lists so a second call becomes a true no-op:
The PR as written silently relies on two callers sharing a tracker to both call rollback without coordination — that's a maintenance hazard.
| }, | ||
| { | ||
| Spec: &WorkflowSpec{ | ||
| WorkflowPath: "workflows/blocked.md", |
There was a problem hiding this comment.
Test relies on an implicit, undocumented failure mechanism that will silently stop exercising rollback if the implementation changes: WorkflowName: "nested/blocked" triggers a write failure only because no nested/ subdirectory exists — nothing in the test makes this intention visible.
💡 Why this matters and how to harden it
The test relies on os.WriteFile failing because nested/ does not exist and is not created by addWorkflowWithTracking. This is a valid failure path today, but:
-
add_command.goalready callsos.MkdirAllfor skill files (line 736) and agent files (line 809). If a future maintainer adds the sameos.MkdirAllfor standard workflow writes (a plausible extension for nested workflow organization), the failure condition silently disappears and the rollback path is no longer tested.require.Errorwould catch this — but only because the test as a whole fails, not because the intent is clear. -
The test only asserts
ok.mdwas rolled back. It does not assert thatblocked.md(the failing write) left no partial state. Whileos.WriteFileis atomic enough that partial writes are rare, asserting the negative makes the test complete.
Add a comment and a second assertion:
// WorkflowName "nested/blocked" intentionally causes a write failure because the
// "nested/" subdirectory does not exist and addWorkflowWithTracking does not create
// subdirectories for workflow names. This exercises the rollback path.
_, statErr := os.Stat(filepath.Join(workflowsDir, "ok.md"))
assert.True(t, os.IsNotExist(statErr), "successful writes must be rolled back on later write failure")
// blocked.md should also not exist (parent dir never created)
_, blockedErr := os.Stat(filepath.Join(workflowsDir, "nested", "blocked.md"))
assert.True(t, os.IsNotExist(blockedErr), "failing workflow must not leave partial state")
|
@copilot run pr-finisher skill |
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>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran |
|
🎉 This pull request is included in a new release. Release: |
This PR closes the daily SPDD gaps across safe-outputs, guard-policies, model-alias, and repository-package specs, plus adds missing behavioral coverage where the specs identified implementation uncertainty. It also hardens
gh aw addby ensuring same-operation writes are rolled back when a later write fails.Safe Outputs spec sync (P0)
createHandlers()functions insafe_outputs_handlers.cjs.safe_outputs_mcp_server.cjs,safe_outputs_tools_loader.cjs,safe_outputs_handlers.cjs).Guard policy cross-field consistency (GP-11)
allowed-repos: "public"withoutmin-integrityfails validation.Model Alias spec completeness
Repository package manifest spec clarity
README.mdis removed under R-PKG-R001, with modified-file behavior governed by R-PKG-R002.Add flow rollback behavior (R-PKG-003 alignment)
addWorkflowsWithTracking()now rolls back files written in the current operation when a subsequent workflow write fails, and returns joined error context if rollback also fails.