feat(validate): check MODIFIED headers against canonical base; --accept-cross-change-base opt-in (#1112)#1113
Conversation
…anonical base; add --accept-cross-change-base opt-in Fixes Fission-AI#1112. `Validator.validateChangeDeltaSpecs` now mirrors `openspec archive`'s existing strict check: for every MODIFIED / REMOVED / RENAMED-from entry in a change delta, look up `openspec/specs/<cap>/spec.md` and verify the `### Requirement: <name>` header exists there (whitespace- insensitive). Without this, authoring bugs only surfaced at archive time — typically days or weeks after the implementing PR shipped. Opt-in `--accept-cross-change-base` (also exposed via the `Validator({ acceptCrossChangeBase: true })` constructor option) also checks sister-pending changes at `openspec/changes/<other>/specs/ <cap>/spec.md` and passes if the header is found there. Covers the legitimate cross-change pattern (sister change in flight; you're extending its requirements before either change archives). `openspec archive` remains strict regardless of the new flag — the flag affects ONLY write-time validate semantics. Users opting in at write time commit to either archiving the sister change first OR folding this change into it before this change's own archive. Implementation notes: - New helper `Validator.checkDeltaAgainstCanonicalBase` invoked from inside the existing per-spec loop in `validateChangeDeltaSpecs`. Adds per-target ERROR-level issues with actionable guidance (which flag to use, what archive will refuse, how to switch to ADDED). - New `Validator(options: ValidatorOptions)` constructor signature. Legacy `new Validator(true)` boolean constructor preserved via type overload — backward compatible. - CLI flag `--accept-cross-change-base` added to `openspec validate`. Plumbed through `ExecuteOptions` → `validateByType` / `runBulkValidation`. Tests: - 9 new tests covering: MODIFIED-vs-missing-canonical default error, CREATE-shape (no canonical exists), MODIFIED matches canonical pass, cross-change MODIFIED default error vs flag-pass, sister-pending with non-matching name still errors (regression guard), REMOVED-not- found, RENAMED-from-not-found, legacy boolean-constructor backward compat. - Full existing suite (1511 tests) still green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds early validation of delta spec references at write time, catching MODIFIED/REMOVED/RENAMED-from headers that don't exist in the canonical base spec. A new ChangesDelta spec canonical-base cross-reference validation
Sequence DiagramsequenceDiagram
participant CLI as CLI validate
participant Execute as ValidateCommand.execute
participant ValidateByType as validateByType
participant Validator as Validator
participant Helper as checkDeltaAgainstCanonicalBase
CLI->>Execute: {acceptCrossChangeBase: true}
Execute->>ValidateByType: pass acceptCrossChangeBase in opts
ValidateByType->>Validator: new Validator({strictMode, acceptCrossChangeBase})
Validator->>Validator: validateChangeDeltaSpecs()
loop for each delta spec
Validator->>Helper: check MODIFIED/REMOVED/RENAMED targets
Helper->>Helper: extractRequirementHeaderNames from canonical
alt acceptCrossChangeBase true
Helper->>Helper: also scan sister-pending changes
end
Helper->>Validator: push ERROR if target missing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
There was a problem hiding this comment.
Thanks, this is the right fix shape and it closes a real validate/archive trust gap.
One scope concern before approval: --accept-cross-change-base currently accepts REMOVED and RENAMED-from targets from sister pending changes too, but the issue rationale and tests only justify the cross-change case for MODIFIED. Please either narrow the opt-in to MODIFIED only, or add explicit rationale and tests for why cross-change REMOVED and RENAMED-from should be supported.
I checked the diff against the validation path and ran pnpm exec vitest run test/core/validation.test.ts locally, which passed.
Closes #1112.
Summary
openspec validate <change>now runs the same MODIFIED/REMOVED/RENAMED-from base-header check thatopenspec archivealready enforces — so authoring bugs surface at write time instead of weeks later at archive time.A new opt-in flag
--accept-cross-change-basewidens the lookup to include sister-pending changes (the one legitimate cross-change pattern). Archive remains strict regardless.Behavior
openspec validate <change>(default)openspec validate <change> --accept-cross-change-baseopenspec/changes/<other>/specs/<cap>/spec.md(sister-pending)openspec archive <change>new Validator(true)(legacy boolean constructor)acceptCrossChangeBase: false)Why opt-in instead of strict-everywhere
There's a legitimate cross-change pattern (sister change in flight; you're extending its requirements before either change archives). Forcing every MODIFIED to require a fully-archived canonical base would make that pattern impossible. The opt-in encodes "I know I'm writing against a sister-pending base; I'll archive the sister or fold this in before archive."
Archive stays strict because the canonical record matters once shipped. The two checks have different jobs:
Implementation
src/core/validation/validator.ts— newValidatorOptionsshape (strictMode,acceptCrossChangeBase); new helpercheckDeltaAgainstCanonicalBaseinvoked from inside the existing per-spec loop invalidateChangeDeltaSpecs. ~140 lines added.src/commands/validate.ts— plumbacceptCrossChangeBasethroughExecuteOptions→validateByType/runBulkValidation. ~15 lines.src/cli/index.ts— register--accept-cross-change-baseon the top-levelvalidatecommand. 2 lines.new Validator(true)boolean constructor preserved via type overload — no breaking change.Tests
9 new tests in
test/core/validation.test.ts:acceptCrossChangeBase=true→ pass.acceptCrossChangeBase=true+ non-matching name in both canonical AND sister → still errors (regression guard).new Validator(true)boolean constructor backward compat.Existing test suite: 1520/1520 pass (was 1511; +9 from new tests, no regressions).
Naming
--accept-cross-change-baseis descriptive but long. Happy to rename to whatever the project prefers —--allow-pending-base,--allow-cross-change,--accept-sister-deltas, etc. I went with the most literal name because the help text fits the design ("accept a base that lives in another in-flight change").Summary by CodeRabbit
Release Notes
--accept-cross-change-baseflag to theopenspec validatecommand to optionally include sister-pending changes during validation.