feat(engine): propose deterministic spec-merge engine (part 2) (#1279) #1288
Closed
clay-good wants to merge 1 commit into
Closed
feat(engine): propose deterministic spec-merge engine (part 2) (#1279) #1288clay-good wants to merge 1 commit into
clay-good wants to merge 1 commit into
Conversation
…AI#1279) The follow-up @alfred-openspec asked for: split unarchive, format, diff, and the unified check gate out of the deterministic-sync spine (Fission-AI#1279) into their own change, to land once the merge engine + applied-delta baseline is proven. Adds five capabilities, all built on Fission-AI#1279's baseline (pre-image + digest + provenance) and shared canonicalizer — no new merge semantics: - cli-unarchive + opsx-unarchive-skill: deterministic inverse of archive (byte-exact reverse from the baseline, drift-guarded, atomic). - cli-format: behavior-preserving formatter sharing Fission-AI#1279's canonicalizer. - cli-diff: spec-aware diff splicing provenance + rationale inline. - cli-check: unified format/sync/validate gate, same binary for pre-commit and CI, with an opt-in hook installer + CI template. Design Decisions 6-10 (unarchive) and 14-16 (format/diff/check); the model- shaping rejections (keep archive; keep deltas) stay in Fission-AI#1279 Decisions 5/11/13. Prerequisite: Fission-AI#1279. Validates clean under `openspec validate add-spec-tooling-suite --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a new OpenSpec change proposal ("add-spec-tooling-suite") consisting entirely of documentation: metadata, a design document, a proposal document, four capability spec files (unarchive, format, diff, check), an opsx-unarchive-skill spec, and a tasks plan, all describing tooling built on the existing merge baseline. ChangesSpec tooling suite proposal
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@openspec/changes/add-spec-tooling-suite/specs/cli-check/spec.md`:
- Around line 61-75: The incremental checking requirement is too narrow because
it only keys off per-file digests, which can miss verdict changes when `--all`
enables cross-change checks. Update the check command’s skip/cache logic to
include the active gate set plus the current change-set or revision, so
unchanged file content is only skipped when the full active context matches;
otherwise force a full re-check. Use the existing `Incremental Checking`
requirement and the `openspec check` behavior to locate and adjust the cache
validity rules.
In `@openspec/changes/add-spec-tooling-suite/specs/cli-diff/spec.md`:
- Around line 19-23: Update the “No inference” scenario wording in the CLI diff
spec so it says the renderer computes the change set from repository contents in
code, not from git diff output. In the spec entry for the diff renderer, keep
the deterministic contract explicit and note that git is only used for the
opt-in driver integration, while the renderer itself should rely on recorded
provenance/rationale and repository state.
In `@openspec/changes/add-spec-tooling-suite/specs/cli-format/spec.md`:
- Around line 116-129: The incremental check in the CLI format spec currently
keys skips only on content digests, which can miss formatter/canonicalizer
changes. Update the Requirement: Incremental Checking and its scenarios so the
cache key in `format --check` also includes a formatter/canonicalizer
fingerprint, or explicitly invalidates recorded digests when the logic changes.
Make sure the guidance in `spec.md` ties the skip decision to both the file
digest and the formatter version/fingerprint so unchanged content is only
skipped when the full check result would remain identical.
In `@openspec/changes/add-spec-tooling-suite/specs/cli-unarchive/spec.md`:
- Around line 123-145: The reversal flow in the CLI unarchive spec is not atomic
across interruption because `openspec/specs/` is swapped before the archive
folder move completes. Update the `validate -> stage -> commit` sequence so
`spec swap` and `folder move` are protected by a durable recovery marker or
journal in `.openspec/`, allowing `unarchive` to resume or roll back after a
crash. If you cannot add that persistence, tighten the wording around the
`Abort. No files were changed.` guarantee to only cover in-process failures.
🪄 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: e2b51ac0-98a1-4331-afb2-59c8f289d6f4
📒 Files selected for processing (9)
openspec/changes/add-spec-tooling-suite/.openspec.yamlopenspec/changes/add-spec-tooling-suite/design.mdopenspec/changes/add-spec-tooling-suite/proposal.mdopenspec/changes/add-spec-tooling-suite/specs/cli-check/spec.mdopenspec/changes/add-spec-tooling-suite/specs/cli-diff/spec.mdopenspec/changes/add-spec-tooling-suite/specs/cli-format/spec.mdopenspec/changes/add-spec-tooling-suite/specs/cli-unarchive/spec.mdopenspec/changes/add-spec-tooling-suite/specs/opsx-unarchive-skill/spec.mdopenspec/changes/add-spec-tooling-suite/tasks.md
Comment on lines
+61
to
+75
| ### Requirement: Incremental Checking | ||
|
|
||
| The check command MAY use recorded content digests to skip specs and files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the verdict versus a full check. | ||
|
|
||
| #### Scenario: Unchanged inputs skipped | ||
|
|
||
| - **WHEN** `openspec check` runs and an input's digest matches its recorded digest | ||
| - **THEN** the command may skip re-checking it | ||
| - **AND** the overall verdict is identical to a full check | ||
|
|
||
| #### Scenario: Changed or unknown inputs fully checked | ||
|
|
||
| - **WHEN** an input's digest does not match, is missing, or uses an unrecognized scheme | ||
| - **THEN** the command performs the full check for it | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make incremental checks aware of the active change set.
A per-file digest is not enough once --all can add cross-change checks: the file contents may stay unchanged while the active change graph changes, so the verdict changes too. Key the cache by the gate set plus the change-set/revision, or invalidate it when cross-change inputs change.
🧩 Suggested wording
- A skip SHALL be permitted only when it cannot change the verdict versus a full check.
+ A skip SHALL be permitted only when it cannot change the verdict versus a full check, and the recorded digest MUST be tied to the exact gate set and active change-set/revision that produced it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Requirement: Incremental Checking | |
| The check command MAY use recorded content digests to skip specs and files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the verdict versus a full check. | |
| #### Scenario: Unchanged inputs skipped | |
| - **WHEN** `openspec check` runs and an input's digest matches its recorded digest | |
| - **THEN** the command may skip re-checking it | |
| - **AND** the overall verdict is identical to a full check | |
| #### Scenario: Changed or unknown inputs fully checked | |
| - **WHEN** an input's digest does not match, is missing, or uses an unrecognized scheme | |
| - **THEN** the command performs the full check for it | |
| ### Requirement: Incremental Checking | |
| The check command MAY use recorded content digests to skip specs and files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the verdict versus a full check, and the recorded digest MUST be tied to the exact gate set and active change-set/revision that produced it. | |
| #### Scenario: Unchanged inputs skipped | |
| - **WHEN** `openspec check` runs and an input's digest matches its recorded digest | |
| - **THEN** the command may skip re-checking it | |
| - **AND** the overall verdict is identical to a full check | |
| #### Scenario: Changed or unknown inputs fully checked | |
| - **WHEN** an input's digest does not match, is missing, or uses an unrecognized scheme | |
| - **THEN** the command performs the full check for it |
🤖 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 `@openspec/changes/add-spec-tooling-suite/specs/cli-check/spec.md` around lines
61 - 75, The incremental checking requirement is too narrow because it only keys
off per-file digests, which can miss verdict changes when `--all` enables
cross-change checks. Update the check command’s skip/cache logic to include the
active gate set plus the current change-set or revision, so unchanged file
content is only skipped when the full active context matches; otherwise force a
full re-check. Use the existing `Incremental Checking` requirement and the
`openspec check` behavior to locate and adjust the cache validity rules.
Comment on lines
+19
to
+23
| #### Scenario: No inference | ||
|
|
||
| - **WHEN** the command renders a diff | ||
| - **THEN** it composes the result from the git diff and the recorded provenance/rationale in code | ||
| - **AND** it does not call a language model |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't derive the renderer from git diff output.
The deterministic contract is stronger than this wording: git diff can vary with user config and external drivers, so the renderer should compute the change set from repository contents in code and reserve git only for the opt-in driver integration.
🛠️ Suggested wording
- THEN it composes the result from the git diff and the recorded provenance/rationale in code
+ THEN it computes the result from repository contents and the recorded provenance/rationale in code📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #### Scenario: No inference | |
| - **WHEN** the command renders a diff | |
| - **THEN** it composes the result from the git diff and the recorded provenance/rationale in code | |
| - **AND** it does not call a language model | |
| #### Scenario: No inference | |
| - **WHEN** the command renders a diff | |
| - **THEN** it computes the result from repository contents and the recorded provenance/rationale in code | |
| - **AND** it does not call a language model |
🤖 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 `@openspec/changes/add-spec-tooling-suite/specs/cli-diff/spec.md` around lines
19 - 23, Update the “No inference” scenario wording in the CLI diff spec so it
says the renderer computes the change set from repository contents in code, not
from git diff output. In the spec entry for the diff renderer, keep the
deterministic contract explicit and note that git is only used for the opt-in
driver integration, while the renderer itself should rely on recorded
provenance/rationale and repository state.
Comment on lines
+116
to
+129
| ### Requirement: Incremental Checking | ||
|
|
||
| The format check MAY use recorded content digests to skip files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the result versus a full check. | ||
|
|
||
| #### Scenario: Unchanged file skipped | ||
|
|
||
| - **WHEN** `--check` runs and a file's current content digest matches the recorded digest | ||
| - **THEN** the command may skip re-checking that file | ||
| - **AND** the overall result is identical to checking it fully | ||
|
|
||
| #### Scenario: Changed or unknown file fully checked | ||
|
|
||
| - **WHEN** a file's digest does not match, no digest is recorded, or the recorded digest uses an unrecognized scheme | ||
| - **THEN** the command performs the full check for that file |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Tie the digest skip to the formatter version.
A content-only digest is not enough here: if the canonicalizer changes, an unchanged file can still need a full pass, and format --check can return a stale pass. Please include a formatter/canonicalizer fingerprint in the cache key or invalidate the cache when that logic changes.
🔧 Suggested wording
- A skip SHALL be permitted only when it cannot change the result versus a full check.
+ A skip SHALL be permitted only when it cannot change the result versus a full check, and the recorded digest MUST be tied to the formatter/canonicalizer version (or invalidated when that version changes).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Requirement: Incremental Checking | |
| The format check MAY use recorded content digests to skip files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the result versus a full check. | |
| #### Scenario: Unchanged file skipped | |
| - **WHEN** `--check` runs and a file's current content digest matches the recorded digest | |
| - **THEN** the command may skip re-checking that file | |
| - **AND** the overall result is identical to checking it fully | |
| #### Scenario: Changed or unknown file fully checked | |
| - **WHEN** a file's digest does not match, no digest is recorded, or the recorded digest uses an unrecognized scheme | |
| - **THEN** the command performs the full check for that file | |
| ### Requirement: Incremental Checking | |
| The format check MAY use recorded content digests to skip files whose content is unchanged since they were last checked, re-checking only what changed. A skip SHALL be permitted only when it cannot change the result versus a full check, and the recorded digest MUST be tied to the formatter/canonicalizer version (or invalidated when that version changes). | |
| #### Scenario: Unchanged file skipped | |
| - **WHEN** `--check` runs and a file's current content digest matches the recorded digest | |
| - **THEN** the command may skip re-checking that file | |
| - **AND** the overall result is identical to checking it fully | |
| #### Scenario: Changed or unknown file fully checked | |
| - **WHEN** a file's digest does not match, no digest is recorded, or the recorded digest uses an unrecognized scheme | |
| - **THEN** the command performs the full check for that file |
🤖 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 `@openspec/changes/add-spec-tooling-suite/specs/cli-format/spec.md` around
lines 116 - 129, The incremental check in the CLI format spec currently keys
skips only on content digests, which can miss formatter/canonicalizer changes.
Update the Requirement: Incremental Checking and its scenarios so the cache key
in `format --check` also includes a formatter/canonicalizer fingerprint, or
explicitly invalidates recorded digests when the logic changes. Make sure the
guidance in `spec.md` ties the skip decision to both the file digest and the
formatter version/fingerprint so unchanged content is only skipped when the full
check result would remain identical.
Comment on lines
+123
to
+145
| The command SHALL apply the reversal atomically using a defined sequence: validate, then stage, then commit, so that any failure before the final commit step leaves the filesystem unchanged. | ||
|
|
||
| #### Scenario: Defined sequence | ||
|
|
||
| - **WHEN** the command performs a reversal that touches specs | ||
| - **THEN** it executes in this order: | ||
| 1. validate that the destination `openspec/changes/<name>/` does not exist and that every affected spec is present and drift-free against the baseline; | ||
| 2. compute the reversed spec content and write it to a temporary staging area inside `.openspec/` (no change yet to `openspec/specs/`); | ||
| 3. swap the staged specs into `openspec/specs/`; | ||
| 4. move the change folder from archive back to active. | ||
|
|
||
| #### Scenario: Failure before the folder move leaves specs untouched | ||
|
|
||
| - **WHEN** validation or staging (steps 1–2) fails | ||
| - **THEN** the command reports `Abort. No files were changed.` | ||
| - **AND** `openspec/specs/` and the archive folder are exactly as they were | ||
|
|
||
| #### Scenario: Failure of the final move rolls specs back | ||
|
|
||
| - **WHEN** the spec swap (step 3) has occurred but the folder move (step 4) fails | ||
| - **THEN** the command restores `openspec/specs/` from the still-present baseline | ||
| - **AND** reports `Abort. No files were changed.` | ||
| - **AND** if rollback itself cannot complete, it reports the partial state and the exact recovery steps rather than leaving a silent inconsistency |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This isn’t atomic across interruption.
The sequence updates openspec/specs/ before the folder move completes, so a crash or kill between steps 3 and 4 can still leave the repo split even though the text promises “Abort. No files were changed.” Either narrow the guarantee to in-process failures or add a durable recovery marker/journal so the operation can be resumed or rolled back after interruption.
Suggested wording
- The command SHALL apply the reversal atomically using a defined sequence: validate, then stage, then commit, so that any failure before the final commit step leaves the filesystem unchanged.
+ The command SHALL use a crash-recoverable sequence: validate, then stage, then swap specs, then move the folder. Any failure before the swap leaves the filesystem unchanged; a failure after the swap must be recoverable via a durable journal or recovery marker.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The command SHALL apply the reversal atomically using a defined sequence: validate, then stage, then commit, so that any failure before the final commit step leaves the filesystem unchanged. | |
| #### Scenario: Defined sequence | |
| - **WHEN** the command performs a reversal that touches specs | |
| - **THEN** it executes in this order: | |
| 1. validate that the destination `openspec/changes/<name>/` does not exist and that every affected spec is present and drift-free against the baseline; | |
| 2. compute the reversed spec content and write it to a temporary staging area inside `.openspec/` (no change yet to `openspec/specs/`); | |
| 3. swap the staged specs into `openspec/specs/`; | |
| 4. move the change folder from archive back to active. | |
| #### Scenario: Failure before the folder move leaves specs untouched | |
| - **WHEN** validation or staging (steps 1–2) fails | |
| - **THEN** the command reports `Abort. No files were changed.` | |
| - **AND** `openspec/specs/` and the archive folder are exactly as they were | |
| #### Scenario: Failure of the final move rolls specs back | |
| - **WHEN** the spec swap (step 3) has occurred but the folder move (step 4) fails | |
| - **THEN** the command restores `openspec/specs/` from the still-present baseline | |
| - **AND** reports `Abort. No files were changed.` | |
| - **AND** if rollback itself cannot complete, it reports the partial state and the exact recovery steps rather than leaving a silent inconsistency | |
| The command SHALL use a crash-recoverable sequence: validate, then stage, then swap specs, then move the folder. Any failure before the swap leaves the filesystem unchanged; a failure after the swap must be recoverable via a durable journal or recovery marker. | |
| #### Scenario: Defined sequence | |
| - **WHEN** the command performs a reversal that touches specs | |
| - **THEN** it executes in this order: | |
| 1. validate that the destination `openspec/changes/<name>/` does not exist and that every affected spec is present and drift-free against the baseline; | |
| 2. compute the reversed spec content and write it to a temporary staging area inside `.openspec/` (no change yet to `openspec/specs/`); | |
| 3. swap the staged specs into `openspec/specs/`; | |
| 4. move the change folder from archive back to active. | |
| #### Scenario: Failure before the folder move leaves specs untouched | |
| - **WHEN** validation or staging (steps 1–2) fails | |
| - **THEN** the command reports `Abort. No files were changed.` | |
| - **AND** `openspec/specs/` and the archive folder are exactly as they were | |
| #### Scenario: Failure of the final move rolls specs back | |
| - **WHEN** the spec swap (step 3) has occurred but the folder move (step 4) fails | |
| - **THEN** the command restores `openspec/specs/` from the still-present baseline | |
| - **AND** reports `Abort. No files were changed.` | |
| - **AND** if rollback itself cannot complete, it reports the partial state and the exact recovery steps rather than leaving a silent inconsistency |
🤖 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 `@openspec/changes/add-spec-tooling-suite/specs/cli-unarchive/spec.md` around
lines 123 - 145, The reversal flow in the CLI unarchive spec is not atomic
across interruption because `openspec/specs/` is swapped before the archive
folder move completes. Update the `validate -> stage -> commit` sequence so
`spec swap` and `folder move` are protected by a durable recovery marker or
journal in `.openspec/`, allowing `unarchive` to resume or roll back after a
crash. If you cannot add that persistence, tighten the wording around the
`Abort. No files were changed.` guarantee to only cover in-process failures.
clay-good
changed the title
Contributor
|
@clay-good Are we still going ahead with this? I'm a little confused by the status of this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
Open, proposal-only (proposal + design + tasks + 5 spec deltas). Validates clean under
openspec validate add-spec-tooling-suite --strict.This is the follow-up @alfred-openspec asked for on #1279 ("narrow the first slice… then split unarchive, format, and diff into follow-ups once the primitive is proven"). #1279 was narrowed to the deterministic merge engine + applied-delta baseline +
sync; this PR carries the commands that build on that primitive.Prerequisite
Depends on #1279 (
add-deterministic-spec-sync): the merge engine, the applied-delta baseline (pre-image + digest + provenance), the sharedsrc/core/spec-canonical.tscanonicalizer, andopenspec sync --check. This change adds no new merge semantics — it builds commands on top. Merge #1279 first.What it does
Four commands, all reusing #1279's baseline + canonicalizer:
openspec unarchive(+/opsx:unarchive) — the deterministic inverse of archive: resolve the archived folder (prefix-tolerant, never auto-picking), reverse the spec merge from the baseline pre-image under a drift guard, move the folder back, atomically (--keep-specsescape hatch; pre-baseline archives degrade all-or-nothing). Gives archive the rollback Feat : Add Extensible Hook Capability to OpenSpec Archive Operation #682 noted it lacks.openspec format— a deterministic, behavior-preserving spec formatter (--checkgate) sharing one canonicalizer with the merge engine, so synced/archived specs passformat --checkby construction.parse-before == parse-after; pure text, no skill.openspec diff— a spec-aware diff that splices the provenance feat(engine): propose deterministic spec-merge engine (part 1) (#1288) #1279 records + the change'sproposal.mdrationale inline; opt-in git diff driver. No sidecar reasoning store, no inference.openspec check— the unified deterministic linter (format --check+sync --check+validate) behind one verdict, plus an opt-in hook installer and CI step — the same binary for pre-commit and CI, no model, no API keys.Capabilities (5 spec deltas)
cli-unarchive(new),opsx-unarchive-skill(new),cli-format(new),cli-diff(new),cli-check(new). Design Decisions 6–10 (unarchive) and 14–16 (format/diff/check) live in this change'sdesign.md; the model-shaping rejections (keeparchive; keep deltas) stay in #1279's Decisions 5/11/13.Proof (for a proposal)
openspec validate add-spec-tooling-suite --strict.Notes / scope
verifydirection, [Proposal] /opsx:validate command to check code against the living specs #880); and owner policy — which hook runner to standardize, whether thecheckgate is on by default (the capability itself is built).🤖 Generated with Claude Code
Summary by CodeRabbit