fix(archive): stop failing on RENAMED deltas that were already synced#1386
Conversation
📝 WalkthroughWalkthrough
ChangesArchive rename idempotency
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The RENAMED no-op is safe because the target header is positive evidence, but the REMOVED branch now converts every missing requirement into a warning and no-op. Validation does not compare REMOVED names against the baseline, so a typo or stale name can pass validation and archive while leaving the intended requirement untouched; please keep REMOVED strict for now, or require explicit sync provenance, baseline evidence, or a deliberate override.
The early-sync idempotency fix (Fission-AI#1376) covered only ADDED requirements. A change whose RENAMED deltas were already applied to the baseline by the sync workflow still aborted archive with 'RENAMED failed - source not found'. RENAMED now skips when the source header is gone but the target header exists in the spec — the target's presence is positive evidence the rename was already applied. A rename whose source and target are both missing still aborts, as does every other genuine conflict. Reported counts now reflect only renames actually applied. REMOVED is intentionally left strict: validation does not compare REMOVED names against the baseline, so treating a missing requirement as a no-op would let a typo'd or stale name archive silently.
dae4522 to
c4373bb
Compare
mc856
changed the title
|
Fair point — took your first option. You're right that the warning doesn't close the gap: validation never compares REMOVED names against the baseline, so a typo'd or stale name would pass validate and archive with only a console line to show for it. Narrowed the PR to RENAMED-only in c4373bb: the REMOVED branch is back to the strict error, byte-for-byte what's on If you'd want the provenance/baseline-evidence route for REMOVED (e.g. sync recording what it applied, or an explicit override flag), happy to take that as a follow-up — it felt too big to bolt onto this fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/specs-apply.ts`:
- Line 369: Update the removal-count handling in the spec application flow to
track only successfully applied removals, using a counter such as
removedApplied. Increment it when each REMOVED requirement is actually
processed, leave it unchanged when missing requirements are skipped for new
specs, and assign the resulting counter to removed instead of
plan.removed.length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce40f451-d287-4849-8b90-7d42ca23b7f9
📒 Files selected for processing (3)
.changeset/idempotent-renamed-archive.mdsrc/core/specs-apply.tstest/core/archive.test.ts
| modified: plan.modified.length, | ||
| removed: plan.removed.length, | ||
| renamed: plan.renamed.length, | ||
| renamed: renamedApplied, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Track actual removals for accurate operation counts.
Since the PR aims to ensure operation counts reflect only changes that were actually applied, removed should also track applied changes using a counter (e.g., removedApplied), rather than defaulting to plan.removed.length. When isNewSpec is true, missing REMOVED requirements are skipped (warned and ignored), which means plan.removed.length overcounts the operations actually performed.
💡 Proposed fix for accurate removal counts
- removed: plan.removed.length,
+ removed: removedApplied, // Assuming a `removedApplied` counter is introduced in the REMOVED loop above🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/specs-apply.ts` at line 369, Update the removal-count handling in
the spec application flow to track only successfully applied removals, using a
counter such as removedApplied. Increment it when each REMOVED requirement is
actually processed, leave it unchanged when missing requirements are skipped for
new specs, and assign the resulting counter to removed instead of
plan.removed.length.
There was a problem hiding this comment.
True in the new-spec corner — the #403 path skips a missing REMOVED but still counts it. That's pre-existing behavior on main, though, and per the review above this PR deliberately restores the REMOVED branch byte-for-byte to what's on main (see the scope note). Re-touching that path, even just its count, would undo the narrowing. The counts fix here covers the rename case this PR actually changes; a removedApplied counter can ride along with any future REMOVED follow-up.
There was a problem hiding this comment.
@mc856, agreed. The RENAMED counter change accurately covers the behavior introduced in this PR, while the new-spec REMOVED overcount is pre-existing and intentionally deferred to preserve the narrowed scope.
Would you like me to open a follow-up GitHub issue for the removedApplied accounting fix?
There was a problem hiding this comment.
No need for a separate issue, I think — it's captured in this thread, and it's small enough to ride along with whatever REMOVED provenance follow-up the maintainers decide on. Their call if they'd rather track it on its own.
There was a problem hiding this comment.
@mc856, understood—no follow-up issue. The existing thread provides sufficient context for any future REMOVED provenance work.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at c4373bb: the unsafe REMOVED behavior is gone, REMOVED remains strict, and RENAMED only skips when the target header provides positive evidence that the rename already landed. The focused archive suite passes locally and the full cross-platform CI suite is green.
3 participants
Refs #1376 — this is the follow-up its Notes section anticipated ("REMOVED/RENAMED deltas referencing already-synced state would still error … Can follow up if that pattern shows up in practice").
What was wrong
#1376 made
openspec archiveidempotent for ADDED requirements after an early sync, but scoped the fix to ADDED. Thesyncworkflow template instructs the agent to fully apply REMOVED and RENAMED deltas to the main spec too — so a change synced through the official workflow still could not archive:How it was fixed
In
buildUpdatedSpec(src/core/specs-apply.ts):⚠️ Warning: <spec> - REMOVED requirement "<name>" not found (already removed?); skipping.). This is the answer to the safety concern in fix(archive): stop failing on specs that were already synced before archiving #1376's notes: unlike ADDED (content identity) and RENAMED (target existence), a REMOVED skip has no positive evidence to distinguish "already done" from a typo'd name — so it stays loud rather than silent, and the desired end state (requirement absent) is true either way.- 0/→ 0.Verification
Repro: apply a change's REMOVED/RENAMED deltas to the main spec via the sync workflow, then
openspec archive <change> --yes. Before: abort with exit code 1, change stays active. After (built CLI):Tests in
test/core/archive.test.ts:main, pass here): early-synced REMOVED and RENAMED changes archive cleanly, with no duplicate or resurrected headers; the REMOVED test also asserts the skip warning is printedOne intentional test change: the existing multi-spec atomicity test used "REMOVED of a missing requirement" as its failure trigger, which this fix turns into a no-op. The trigger is now a MODIFIED that cannot resolve; the test's intent — a failure in the second spec aborts the whole archive and leaves both specs untouched — is unchanged.
Archive suite: 38/38. Full suite: 1,993 passed / 1 failed — the failure is
test/commands/workset.test.ts(launch-environment dependent) and fails identically onmain.Notes
Scoped to the two delta branches #1376 left out; the ADDED and MODIFIED paths are untouched.
applySpecs()still doesn't threadsilentintobuildUpdatedSpec— pre-existing, no internal callers, left alone.Summary by CodeRabbit