SPDD 2026-07-08: close spec sync gaps and enforce add-flow rollback semantics #44358
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -277,20 +277,48 @@ func addWorkflows(ctx context.Context, workflows []*ResolvedWorkflow, opts AddOp | |
| return addWorkflowsWithTracking(ctx, workflows, tracker, opts) | ||
| } | ||
|
|
||
| func prepareGitAttributesTracking(tracker *FileTracker) (path string, existed bool) { | ||
| gitRoot, err := gitutil.FindGitRoot() | ||
| if err != nil { | ||
| addLog.Printf("Skipping .gitattributes tracking setup: failed to find git root: %v", err) | ||
| return "", false | ||
| } | ||
|
|
||
| path = filepath.Join(gitRoot, ".gitattributes") | ||
| existed = fileutil.FileExists(path) | ||
| if tracker != nil && existed { | ||
| tracker.TrackModified(path) | ||
| } | ||
|
|
||
| return path, existed | ||
| } | ||
|
|
||
| func trackGitAttributesIfCreated(tracker *FileTracker, path string, existed bool, updated bool) { | ||
| if tracker == nil || path == "" || existed || !updated { | ||
| return | ||
| } | ||
| tracker.TrackCreated(path) | ||
| } | ||
|
|
||
| // addWorkflows handles workflow addition using pre-fetched content | ||
| func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow, tracker *FileTracker, opts AddOptions) error { | ||
| addLog.Printf("Adding %d workflow(s) with tracking: force=%v, disableSecurityScanner=%v", len(workflows), opts.Force, opts.DisableSecurityScanner) | ||
| // Ensure .gitattributes is configured unless flag is set | ||
| if !opts.NoGitattributes { | ||
| gitAttributesPath, gitAttributesExisted := prepareGitAttributesTracking(tracker) | ||
|
|
||
| addLog.Print("Configuring .gitattributes") | ||
| if updated, err := ensureGitAttributes(); err != nil { | ||
| addLog.Printf("Failed to configure .gitattributes: %v", err) | ||
| if opts.Verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to update .gitattributes: %v", err))) | ||
| } | ||
| // Don't fail the entire operation if gitattributes update fails | ||
| } else if updated { | ||
| trackGitAttributesIfCreated(tracker, gitAttributesPath, gitAttributesExisted, updated) | ||
| if opts.Verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Configured .gitattributes")) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -305,6 +333,11 @@ func addWorkflowsWithTracking(ctx context.Context, workflows []*ResolvedWorkflow | |
| } | ||
|
|
||
| if err := addWorkflowWithTracking(ctx, resolved, tracker, opts); err != nil { | ||
| if tracker != nil { | ||
| if rollbackErr := tracker.RollbackAllFiles(opts.Verbose); rollbackErr != nil { | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] Double-rollback risk: 💡 Suggested fixClear 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 @copilot please address this.
Contributor
There was a problem hiding this comment. Double rollback when caller already runs 💡 Impact and suggested fixToday this is safe by coincidence:
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 Option A (preferred): Move rollback responsibility entirely into Option B: After a successful rollback inside The PR as written silently relies on two callers sharing a tracker to both call rollback without coordination — that's a maintenance hazard. |
||
| return fmt.Errorf("failed to add workflow '%s' (rollback also failed): %w", resolved.Spec.String(), errors.Join(err, rollbackErr)) | ||
|
Contributor
There was a problem hiding this comment. Double-rollback hazard introduced by this change. After this PR, 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 @copilot please address this. |
||
| } | ||
| } | ||
| return fmt.Errorf("failed to add workflow '%s': %w", resolved.Spec.String(), err) | ||
|
Comment on lines
335
to
341
Contributor
There was a problem hiding this comment. [/tdd] Rollback success is not asserted — the test only checks that 💡 Suggested additions
@copilot please address this. |
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -620,6 +620,51 @@ func TestAddWorkflowWithTracking_ActionWorkflow_Force(t *testing.T) { | |
| assert.Equal(t, newContent, written) | ||
| } | ||
|
|
||
| func TestAddWorkflowsWithTracking_RollsBackWrittenFilesOnWriteFailure(t *testing.T) { | ||
| tempDir := testutil.TempDir(t, "test-add-workflows-rollback-*") | ||
| workflowsDir := setupMinimalGitRepo(t, tempDir) | ||
|
|
||
| validContent := []byte("---\nengine: claude\n---\n\n# workflow\n") | ||
| // workflow name "nested/blocked" intentionally causes write failure because | ||
| // addWorkflowWithTracking does not create nested workflow directories. | ||
| workflows := []*ResolvedWorkflow{ | ||
| { | ||
| Spec: &WorkflowSpec{ | ||
| WorkflowPath: "workflows/ok.md", | ||
| WorkflowName: "ok", | ||
| }, | ||
| Content: validContent, | ||
| SourceInfo: &FetchedWorkflow{ | ||
| IsLocal: true, | ||
| }, | ||
| }, | ||
| { | ||
| Spec: &WorkflowSpec{ | ||
| WorkflowPath: "workflows/blocked.md", | ||
|
Contributor
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: 💡 Why this matters and how to harden itThe test relies on
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") |
||
| WorkflowName: "nested/blocked", | ||
| }, | ||
| Content: validContent, | ||
| SourceInfo: &FetchedWorkflow{ | ||
| IsLocal: true, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| err := addWorkflowsWithTracking(context.Background(), workflows, NewFileTracker(), AddOptions{ | ||
| NoGitattributes: true, | ||
| DisableSecurityScanner: true, | ||
| Quiet: true, | ||
| }) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to write destination file") | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The test relies on 💡 Suggested improvementMake 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. |
||
|
|
||
| _, statErr := os.Stat(filepath.Join(workflowsDir, "ok.md")) | ||
|
Contributor
There was a problem hiding this comment. Test relies on an undocumented failure condition that may be fragile. The test expects This is an implicit coupling: the test passes only because 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. |
||
| assert.True(t, os.IsNotExist(statErr), "successful writes from this operation should be rolled back on later write failure") | ||
|
|
||
| _, blockedStatErr := os.Stat(filepath.Join(workflowsDir, "nested", "blocked.md")) | ||
| assert.True(t, os.IsNotExist(blockedStatErr), "failing workflow should not leave partial output files") | ||
| } | ||
|
|
||
| func TestAddSkillFileWithTracking_PreservesPathFromSkillsRoot(t *testing.T) { | ||
| gitRoot := testutil.TempDir(t, "test-add-skill-path-*") | ||
| resolved := &ResolvedWorkflow{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/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
@copilot please address this.