Repository navigation
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.
… follow-ups (Fission-AI#1437) * fix(archive): treat early-synced REMOVED deltas as no-ops, plus audit follow-ups Follow-ups from the post-v1.6.0 full-branch audit: - archive: a REMOVED delta whose requirement is already gone from the main spec (early-sync pattern) now warns and continues instead of aborting, matching the ADDED (Fission-AI#1376) and RENAMED (Fission-AI#1386) escapes; spec-update totals now count applied removals only - archive: the has-delta-specs gate matches section headers case-insensitively like the parser, so lowercase headers get the same delta validation errors validate reports - discovery: a symlinked specs/<cap>/spec.md is resolved instead of being invisible (hasAnyFileUnder and the artifact graph already counted it); dangling links are skipped - show: a plain `openspec show <change>` no longer warns about the never-passed `scenarios` flag (commander defaults --no-scenarios to true) - parsers: buildCodeFenceMask now has a single implementation in code-fence.ts; requirement-text.ts re-exports it - templates: apply/update/onboard no longer dead-end core-profile users on /opsx:continue and /opsx:new - they name the CLI fallback (openspec status/instructions) for profiles that do not install those workflows - qwen/bob: command bodies and skills reference commands by the hyphen names their files actually answer to (/opsx-<id>), matching opencode/pi/oh-my-pi - specs-apply: remove the dead applySpecs export (no callers, bypassed store-aware roots) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(archive): reject RENAMED+REMOVED conflicts, surface JSON warnings, skip no-op writes Adversarial-review round for Fission-AI#1437: - a delta that both RENAMEs and REMOVEs the same requirement is rejected explicitly by both validate and archive - the warn-and-continue REMOVED path would otherwise have masked the contradiction that previously failed incidentally at apply time - buildUpdatedSpec collects its warnings and archive --json carries them in a new optional `warnings` array, so agent flows see the same skipped-REMOVED signal humans get on stdout - archive skips rewriting a spec whose operations were all already synced, instead of churning normalization differences into the file (and no longer materializes an empty skeleton for a REMOVED-only new spec) - init's getting-started hint uses each tool's real invocation form (/opsx-propose for qwen/bob/opencode/pi/oh-my-pi) - onboard's pause guidance names the CLI fallback when /opsx:continue is not installed (CodeRabbit) - openspec-conventions spec updated to state the idempotent archive semantics; changeset added Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(archive): abort on near-miss REMOVED typos, honest specsUpdated for no-op archives Round-2 adversarial review for Fission-AI#1437: - a REMOVED header that differs only in case or interior whitespace from an existing requirement is a typo, not an early sync - it stays a hard abort naming the near-miss, instead of degrading to warn-and-continue - specsUpdated is true only when a spec file was actually written; a fully-already-synced change prints "Specs already in sync; no files changed." and reports specsUpdated: false in JSON (CodeRabbit) - agent-contract documents the archive warnings field and specsUpdated semantics; changeset wording fixed (CodeRabbit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(archive): compare the RENAMED+REMOVED conflict case- and whitespace-insensitively Addresses alfred's review on Fission-AI#1437: `RENAMED FROM: Old Name` plus `REMOVED: old name` slipped past the exact-match cross-section guard, so validate passed, archive renamed the requirement, reported the removal as already synced, and archived the change. Both the validator and the apply-side guard now compare the two spellings with the shared foldRequirementName (lowercase, collapsed whitespace), and the error names the variant spelling when it differs. Focused regressions cover both paths; requirement matching everywhere else stays case-sensitive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Two asymmetries left over from the #1376/#1386/#1437 no-op work: - MODIFIED counted every delta as applied even when the block was byte-equal to the main spec, so a fully early-synced change rewrote the file (normalization churn), printed '~ N modified', and reported specsUpdated: true where its ADDED/REMOVED/RENAMED twins print 'Specs already in sync; no files changed.' Count only real replacements. - RENAMED's already-synced skip (source gone, target present) had no near-miss guard: a case/whitespace variant of the source still in the spec means a typo'd header, and REMOVED already hard-aborts on that signal. Apply the same guard, excluding the target itself so a case-only rename still no-ops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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