feat(engine): propose deterministic spec-merge engine (part 1) (#1288)#1279
feat(engine): propose deterministic spec-merge engine (part 1) (#1288)#1279clay-good wants to merge 8 commits into
Conversation
…c unarchive Add an OpenSpec change proposing `openspec unarchive [change-name]` and a thin `/opsx:unarchive` skill: the deterministic inverse of `openspec archive`. Core finding driving the design: the archived delta is not self-inverting. ADDED/RENAMED are reversible from the delta; REMOVED/MODIFIED are not (the forward merge discards the pre-image, confirmed in specs-apply.ts and Fission-AI#1246). So archive gains a forward-only reversal snapshot (pre-image + post-merge digest) that makes reversal byte-exact, drift-guarded, and atomic, with graceful degradation for pre-snapshot archives and a --keep-specs escape hatch. Also folds in the CI-determinism recommendation: expose the deterministic merge engine (today applySpecs() has zero callers, reachable only inside archive) as a model-free `openspec sync --check` drift gate — captured as a separable companion, de-risked by this PR's round-trip determinism. Dogfoods OpenSpec: proposal.md, design.md (7 decisions), tasks.md, and spec deltas for cli-unarchive (new), opsx-unarchive-skill (new), cli-archive (reversal snapshot). Validates with `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new OpenSpec change set that defines deterministic ChangesDeterministic sync, unarchive, and format
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
openspec/changes/add-unarchive-command/design.md (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReword the contradictory "zero callers" claim.
"has zero callers" is literally false if the engine is "reachable only inside
archive" —archiveitself is a caller. Rephrase to "has no standalone CLI exposure" or "is not invoked by any command other thanopenspec archive" to match the intended meaning.See matching suggestion in
proposal.mdline 46.🤖 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-unarchive-command/design.md` at line 73, The wording around applySpecs() is contradictory: it is not true that it has “zero callers” if archive invokes it. Update the design text to say that applySpecs() has no standalone CLI exposure or is only invoked by openspec archive, and keep the rest of the point about exposing a separate openspec sync command and --check drift gate intact.openspec/changes/add-unarchive-command/proposal.md (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the "zero callers" claim.
The phrase "has zero callers" contradicts the following clause "it is reachable only inside
openspec archive" — if archive can reach it, archive is a caller. The intended point is that no standalone CLI command or external caller invokesapplySpecs(); it is only reachable bundled insideopenspec archive.Reword to avoid the contradiction, e.g.: "has no standalone CLI exposure — it is reachable only inside
openspec archive, bundled with the folder move."Same wording appears in
design.mdline 73.🤖 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-unarchive-command/proposal.md` at line 46, Reword the claim about applySpecs() so it does not say “zero callers” while also saying it is reachable inside openspec archive, since that is contradictory. Update the proposal text around applySpecs() to state that there is no standalone CLI exposure or external caller, and that the function is only reachable as part of openspec archive’s bundled folder move; apply the same wording consistently in design.md as well.openspec/changes/add-unarchive-command/tasks.md (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconsider the hidden
--skip-specsalias for--keep-specs.In the archive command,
--skip-specsmeans "skip spec updates" (don't merge specs intospecs/). Using--skip-specsas a hidden alias for--keep-specsin unarchive semantically means "skip spec reversal" — the opposite effect. This naming symmetry is likely to confuse users who muscle-memory archive's flag and expect it to behave the same way. Consider removing the alias or renaming it to something unambiguous like--no-spec-restore.🤖 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-unarchive-command/tasks.md` at line 37, The `unarchive` CLI registration in `src/cli/index.ts` should not reuse `--skip-specs` as a hidden alias for `--keep-specs`, because that flag name implies the opposite behavior from archive and will confuse users. Update the `unarchive [change-name]` command wiring to remove the ambiguous alias or replace it with a clearly named option such as `--no-spec-restore`, while keeping the existing `UnarchiveCommand().execute(...)` action and the other flags intact.openspec/changes/add-unarchive-command/specs/cli-unarchive/spec.md (3)
91-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd explicit missing-file drift scenario.
The drift guard should explicitly handle when an affected spec no longer exists (was deleted after archiving). Treating absence as drift is implied by "current content no longer matches," but an explicit scenario prevents implementers from overlooking it:
#### Scenario: Drift on missing spec - **WHEN** an affected spec was deleted after archiving - **THEN** the command treats it as drifted (the current state is absent, which does not match the recorded post-merge digest) - **AND** it refuses to reverse and reports the spec as drifted🤖 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-unarchive-command/specs/cli-unarchive/spec.md` around lines 91 - 104, The drift guard in the unarchive spec needs an explicit missing-file case so implementers do not miss deleted specs. Update the CLI unarchive spec around the drift scenarios to add a “drift on missing spec” case that states an affected spec deleted after archiving is treated as drift, reported as such, and causes the reversal to refuse; keep the wording aligned with the existing reversal snapshot and post-merge digest checks in the drift section.
22-24: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueClarify non-interactive detection beyond
--json."Or otherwise non-interactively" is ambiguous. Specify the conditions that trigger non-interactive mode (e.g.,
--jsonflag,CI=true, or stdin not being a TTY). Without this, implementers may inconsistently handle piped input or environment-based automation.- **WHEN** the command is run with `--json`, or when `stdin` is not a TTY and no `--interactive` flag is forced🤖 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-unarchive-command/specs/cli-unarchive/spec.md` around lines 22 - 24, Clarify the non-interactive requirement in the unarchive CLI spec by updating the acceptance criteria around the command path that requires a change name. In the CLI-unarchive spec, replace the vague “otherwise non-interactively” wording with explicit triggers such as `--json`, `CI=true`, or `stdin` not being a TTY unless `--interactive` is forced, so implementers can consistently detect when to fail with the machine-readable missing-name diagnostic and non-zero exit.
28-28: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueClarify resolution priority between full id and bare name.
The spec does not state whether resolution tries an exact full-id match first before falling back to bare-name stripping. If a user accidentally passes a string that happens to be both a valid full id and a bare name with multiple matches, the behavior could be surprising. Explicitly require:
The command SHALL first attempt an exact match against the full directory id; only if that fails SHALL it apply prefix-stripping bare-name resolution.🤖 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-unarchive-command/specs/cli-unarchive/spec.md` at line 28, Clarify the unarchive target resolution order in the CLI spec by updating the resolution rule in the cli-unarchive spec to state that the command first tries an exact match against the full archived directory id, and only if that fails does it fall back to bare-name prefix stripping. Keep the existing no-ambiguous-selection requirement, and make sure the wording around the target resolution behavior in the spec is unambiguous about the priority between full id and bare name.
🤖 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-unarchive-command/specs/cli-archive/spec.md`:
- Line 5: Clarify the reversal snapshot contract in the archive spec by defining
the exact storage location and schema used by the archive flow. Update the spec
text around the archive operation and `openspec unarchive` expectations to name
the snapshot path relative to the change folder, specify that it is a versioned
JSON format, and define the per-spec fields and encoding so implementers can
read it consistently.
In `@openspec/changes/add-unarchive-command/specs/cli-unarchive/spec.md`:
- Around line 121-136: The atomicity requirement for the unarchive flow is too
vague in the CLI unarchive spec; tighten the sequence and guarantee in the
atomic operation scenario. Update the requirement to explicitly define the order
in the unarchive command: validate the destination and drift-free specs first,
stage reversed specs in a temporary location, atomically swap them into
openspec/specs via the unarchive command’s main reversal flow, then move the
change folder last. Make clear that the implementation must use an atomic
rename/swap approach for the spec update, and that any failure before the final
folder move must leave openspec/specs unchanged with staged data discarded.
- Around line 137-161: Clarify and lock down mixed-delta behavior in the
unarchive spec so the success/failure model is unambiguous. Update the `openspec
unarchive` backward-compatibility requirement to choose either the conservative
path (fail the whole command if any requirement is MODIFIED/REMOVED) or the
partial path (reverse only ADDED/RENAMED items and leave the rest untouched). If
you choose partial reversal, also update the `--json` contract to report
per-spec outcomes using separate reversed/skipped or reversed/failed collections
instead of a single all-or-nothing error result. Make the `Reverse the
self-invertible operations` and `Refuse to guess irreversible operations`
scenarios consistent with the chosen model.
---
Nitpick comments:
In `@openspec/changes/add-unarchive-command/design.md`:
- Line 73: The wording around applySpecs() is contradictory: it is not true that
it has “zero callers” if archive invokes it. Update the design text to say that
applySpecs() has no standalone CLI exposure or is only invoked by openspec
archive, and keep the rest of the point about exposing a separate openspec sync
command and --check drift gate intact.
In `@openspec/changes/add-unarchive-command/proposal.md`:
- Line 46: Reword the claim about applySpecs() so it does not say “zero callers”
while also saying it is reachable inside openspec archive, since that is
contradictory. Update the proposal text around applySpecs() to state that there
is no standalone CLI exposure or external caller, and that the function is only
reachable as part of openspec archive’s bundled folder move; apply the same
wording consistently in design.md as well.
In `@openspec/changes/add-unarchive-command/specs/cli-unarchive/spec.md`:
- Around line 91-104: The drift guard in the unarchive spec needs an explicit
missing-file case so implementers do not miss deleted specs. Update the CLI
unarchive spec around the drift scenarios to add a “drift on missing spec” case
that states an affected spec deleted after archiving is treated as drift,
reported as such, and causes the reversal to refuse; keep the wording aligned
with the existing reversal snapshot and post-merge digest checks in the drift
section.
- Around line 22-24: Clarify the non-interactive requirement in the unarchive
CLI spec by updating the acceptance criteria around the command path that
requires a change name. In the CLI-unarchive spec, replace the vague “otherwise
non-interactively” wording with explicit triggers such as `--json`, `CI=true`,
or `stdin` not being a TTY unless `--interactive` is forced, so implementers can
consistently detect when to fail with the machine-readable missing-name
diagnostic and non-zero exit.
- Line 28: Clarify the unarchive target resolution order in the CLI spec by
updating the resolution rule in the cli-unarchive spec to state that the command
first tries an exact match against the full archived directory id, and only if
that fails does it fall back to bare-name prefix stripping. Keep the existing
no-ambiguous-selection requirement, and make sure the wording around the target
resolution behavior in the spec is unambiguous about the priority between full
id and bare name.
In `@openspec/changes/add-unarchive-command/tasks.md`:
- Line 37: The `unarchive` CLI registration in `src/cli/index.ts` should not
reuse `--skip-specs` as a hidden alias for `--keep-specs`, because that flag
name implies the opposite behavior from archive and will confuse users. Update
the `unarchive [change-name]` command wiring to remove the ambiguous alias or
replace it with a clearly named option such as `--no-spec-restore`, while
keeping the existing `UnarchiveCommand().execute(...)` action and the other
flags intact.
🪄 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: 012c9beb-9647-40d4-a50c-25483db70f85
📒 Files selected for processing (7)
openspec/changes/add-unarchive-command/.openspec.yamlopenspec/changes/add-unarchive-command/design.mdopenspec/changes/add-unarchive-command/proposal.mdopenspec/changes/add-unarchive-command/specs/cli-archive/spec.mdopenspec/changes/add-unarchive-command/specs/cli-unarchive/spec.mdopenspec/changes/add-unarchive-command/specs/opsx-unarchive-skill/spec.mdopenspec/changes/add-unarchive-command/tasks.md
|
|
||
| ### Requirement: Reversal Snapshot Capture | ||
|
|
||
| When the archive operation rewrites main specs, it SHALL capture a self-contained reversal snapshot inside the change folder so that the operation can later be reversed deterministically by `openspec unarchive`. The snapshot SHALL be forward-only and SHALL NOT alter how archive merges, moves, validates, or what it outputs. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Clarify snapshot storage location and format.
The spec states the snapshot is "forward-only" and "inside the change folder," but does not specify the file name, format, or structure. Without this, implementers may choose incompatible representations (e.g., JSON vs. binary, single file vs. per-spec files), and future tooling cannot reliably read it. Specify at minimum the file path relative to the change folder and whether the format is versioned.
Consider:
- The snapshot SHALL be written as `<change-folder>/.openspec/reversal-snapshot.json` containing a versioned schema with per-spec entries of `{ "path": "...", "preMergeBytes": "base64...", "postMergeDigest": "sha256:..." }`.🤖 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-unarchive-command/specs/cli-archive/spec.md` at line 5,
Clarify the reversal snapshot contract in the archive spec by defining the exact
storage location and schema used by the archive flow. Update the spec text
around the archive operation and `openspec unarchive` expectations to name the
snapshot path relative to the change folder, specify that it is a versioned JSON
format, and define the per-spec fields and encoding so implementers can read it
consistently.
| ### Requirement: Backward Compatibility For Pre-Snapshot Archives | ||
|
|
||
| For changes archived before reversal snapshots existed, the command SHALL reverse the operations it can invert from the archived delta alone and SHALL refuse to guess the operations it cannot, never producing an incorrect spec. | ||
|
|
||
| #### Scenario: Reverse the self-invertible operations | ||
|
|
||
| - **WHEN** an archived change has no reversal snapshot | ||
| - **AND** its delta contains only ADDED and/or RENAMED requirements | ||
| - **THEN** the command reverses them by delta inversion (removing added requirements, renaming renamed requirements back) | ||
| - **AND** restores `openspec/specs/` accordingly | ||
|
|
||
| #### Scenario: Refuse to guess irreversible operations | ||
|
|
||
| - **WHEN** an archived change has no reversal snapshot | ||
| - **AND** its delta contains MODIFIED or REMOVED requirements (whose pre-image is not recoverable from the delta) | ||
| - **THEN** the command does not modify those specs | ||
| - **AND** it reports which requirements cannot be safely reversed | ||
| - **AND** it directs the user to `--keep-specs` (and optionally to git-based recovery) | ||
|
|
||
| #### Scenario: Keep-specs always available | ||
|
|
||
| - **WHEN** an archived change has no reversal snapshot | ||
| - **AND** the user runs `openspec unarchive <name> --keep-specs` | ||
| - **THEN** the command restores the folder without touching specs, regardless of which delta operations the change contains | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarify partial reversal semantics for mixed deltas.
When a pre-snapshot change contains both self-invertible and irreversible operations across different specs, the spec implies partial reversal but does not define the success/failure reporting model. Specify whether the command:
- Option A (conservative): Refuses entirely if any spec contains MODIFIED/REMOVED, requiring
--keep-specs. - Option B (partial): Reverses what it can, leaves irreversible specs untouched, and reports partial success with clear per-spec status.
If Option B, the --json output contract in lines 171-176 must accommodate partial success (e.g., reversed: [...], failed: [...] rather than a single error code).
#### Scenario: Mixed invertibility
- **WHEN** a pre-snapshot change has some specs with only ADDED/RENAMED operations and others with MODIFIED/REMOVED
- **THEN** the command SHALL either:
- (conservative) refuse to modify any specs and require `--keep-specs`; or
- (partial) reverse the self-invertible specs, leave the irreversible specs untouched, and report which specs were reversed and which were skipped🤖 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-unarchive-command/specs/cli-unarchive/spec.md` around
lines 137 - 161, Clarify and lock down mixed-delta behavior in the unarchive
spec so the success/failure model is unambiguous. Update the `openspec
unarchive` backward-compatibility requirement to choose either the conservative
path (fail the whole command if any requirement is MODIFIED/REMOVED) or the
partial path (reverse only ADDED/RENAMED items and leave the rest untouched). If
you choose partial reversal, also update the `--json` contract to report
per-spec outcomes using separate reversed/skipped or reversed/failed collections
instead of a single all-or-nothing error result. Make the `Reverse the
self-invertible operations` and `Refuse to guess irreversible operations`
scenarios consistent with the chosen model.
…chive) Re-scope the unarchive proposal around the long-term direction the full Discord thread converged on: the delta->specs merge should be a deterministic, idempotent, REVERSIBLE pure-code engine, exposed as first-class commands. Unarchive becomes one payoff of that engine, alongside a standalone `openspec sync`, a model-free CI/pre-commit drift gate, and crumb-free re-merge. Key insight: one primitive — a per-change applied-delta baseline (pre-image + applied-result digest) — unifies three asks the chat raised separately: deterministic reverse (unarchive), idempotent "no crumbs" re-sync, and hash-based drift detection. Frozen into the archive, it is the unarchive reversal snapshot. Adopted from the discussion: - deterministic, code-only `openspec sync` + `--check` (no model in CI) - `sync --fix` as eslint-style pre-commit auto-fixer - skills (/opsx:sync, /opsx:unarchive) delegate to the CLI; agent never merges (fixes the Fission-AI#1246 "intelligent merge drops scenarios" failure mode) Rejected (documented): removing archive / folding merge into apply — preserves the specs/ = shipped-reality invariant (design Decision 5). Rename change add-unarchive-command -> add-deterministic-sync-and-unarchive. Spec deltas: cli-sync (new), cli-unarchive (new), opsx-unarchive-skill (new), cli-archive (baseline + shared engine), specs-sync-skill (CLI delegation). Validates with `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
openspec unarchive + /opsx:unarchiveopenspec sync + openspec unarchive (+ /opsx:unarchive)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-unarchive/spec.md (1)
162-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
archive-directory-missingin the--jsonerror code enumeration or explicitly map it tonot-found.Line 168-170 defines a distinct "missing archive directory" error condition, but Line 173's explicit
--jsonblocked-condition list (not found, ambiguous, drift, destination exists, or pre-baseline irreversibility) omits it. Clarify whether archive-directory-missing shares thenot-foundcode or has its own stable code so CLI consumers can distinguish "no archive directory" from "no matching change."🤖 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-deterministic-sync-and-unarchive/specs/cli-unarchive/spec.md` around lines 162 - 176, The error-condition spec for the CLI unarchive command is missing a clear JSON mapping for the “missing archive directory” case. Update the `Error Conditions` and `JSON diagnostics` text in the unarchive spec so `archive-directory-missing` is either added as its own stable code or explicitly mapped to `not-found`, and make the `--json` blocked-condition list in the `Scenario: JSON diagnostics` section match that decision consistently.
🧹 Nitpick comments (1)
openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md (1)
75-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish error codes for the two
--checkfailure modes.The two
--checkscenarios describe different root causes: (1) deltas cannot be cleanly applied to base specs, and (2) committed specs have drifted from previously generated output. For machine-readable CI gating and diagnostics, consider requiring distinct stable error codes in the JSON output so callers can differentiate a broken delta from an accidental manual edit without parsing human-readable messages.🤖 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-deterministic-sync-and-unarchive/specs/cli-sync/spec.md` around lines 75 - 97, Update the `Drift Check Mode` requirement in `cli-sync/spec.md` to distinguish the two `--check` failure paths with stable machine-readable error codes in JSON output: one for unappliable deltas and another for drift between committed specs and regenerated output. Adjust the `openspec sync <name> --check` scenarios so callers can reliably tell which condition occurred without parsing the human message, while still preserving the non-zero exit and no-file-modification behavior.
🤖 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.
Outside diff comments:
In
`@openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-unarchive/spec.md`:
- Around line 162-176: The error-condition spec for the CLI unarchive command is
missing a clear JSON mapping for the “missing archive directory” case. Update
the `Error Conditions` and `JSON diagnostics` text in the unarchive spec so
`archive-directory-missing` is either added as its own stable code or explicitly
mapped to `not-found`, and make the `--json` blocked-condition list in the
`Scenario: JSON diagnostics` section match that decision consistently.
---
Nitpick comments:
In
`@openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md`:
- Around line 75-97: Update the `Drift Check Mode` requirement in
`cli-sync/spec.md` to distinguish the two `--check` failure paths with stable
machine-readable error codes in JSON output: one for unappliable deltas and
another for drift between committed specs and regenerated output. Adjust the
`openspec sync <name> --check` scenarios so callers can reliably tell which
condition occurred without parsing the human message, while still preserving the
non-zero exit and no-file-modification behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fab22a2-751f-463e-9673-a894de3557d8
📒 Files selected for processing (9)
openspec/changes/add-deterministic-sync-and-unarchive/.openspec.yamlopenspec/changes/add-deterministic-sync-and-unarchive/design.mdopenspec/changes/add-deterministic-sync-and-unarchive/proposal.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/cli-archive/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/cli-unarchive/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/opsx-unarchive-skill/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/specs-sync-skill/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/tasks.md
💤 Files with no reviewable changes (2)
- openspec/changes/add-deterministic-sync-and-unarchive/.openspec.yaml
- openspec/changes/add-deterministic-sync-and-unarchive/specs/opsx-unarchive-skill/spec.md
✅ Files skipped from review due to trivial changes (4)
- openspec/changes/add-deterministic-sync-and-unarchive/tasks.md
- openspec/changes/add-deterministic-sync-and-unarchive/design.md
- openspec/changes/add-deterministic-sync-and-unarchive/proposal.md
- openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-archive/spec.md
…; scrub names Continue folding the design discussion into PR Fission-AI#1279, within OpenSpec's scope. Adopt (in scope): - Early conflict resolution — sync --check surfaces delta-vs-base and cross-change conflicts at commit/PR time, not at archive. Gets the "resolve conflicts immediately" benefit without abandoning deltas. - Provenance — the merge records which change/delta produced each spec edit; enables a deterministic delta<->spec correspondence check (orphan edit / unapplied delta -> fail) and a future spec-aware diff driver. - 3 new cli-sync requirements: Change Provenance, Delta-Spec Correspondence, Cross-Change Conflict Detection. New design Decisions 11-12. Engage and reject, with reasoning (documented): - Edit specs/ in place; treat git diff as the delta; semantic deltas in a sidecar log. Rejected: the delta layer is what isolates proposed from shipped and makes parallel changes + single-change reverse possible. Its valid kernel (early conflict resolution, reasoning-with-the-change) is adopted via the items above. (Decision 11) Defer (delineated): the spec-aware git diff driver — provenance data is in scope; the diff-driver presentation is a follow-up (Decision 12). Also: scrub contributor names from all PR content (neutral attributions). Validates with `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uth vision Continue folding the design discussion into PR Fission-AI#1279, within OpenSpec's scope. Adopt (in scope, new): - Incremental checking — `sync --check` uses the baseline per-spec digests to skip unchanged specs and re-check only what moved ("use hashes to optimize when checks are needed"). Correctness invariant: a skip is allowed only on an exact digest match; mismatch/missing/unknown-scheme forces a full check, so the incremental verdict always equals the full verdict. New cli-sync requirement + design Decision 13 + tasks. Acknowledge and defer: - Spec linter/formatter ("how the spec looks/reads/organized"). This PR makes OUTPUT specs canonical and byte-stable; an authoring-side formatter is a separable follow-up adjacent to openspec-conventions/validate. Engage and reject, with reasoning (documented, Decision 13): - Spec-as-source-of-truth: edit in place + commit only the spec, treating proposal/design/tasks as disposable. Rejected — those artifacts are the reviewable, resumable product (agreement before code); the delta layer keeps proposed vs shipped isolated. Valid kernel (lightweight changes needn't carry every artifact) is already served by schema flexibility, outside this PR. No human names anywhere in PR content (verified). Validates with `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md (1)
61-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing scenario for re-sync drift due to another change.
The spec covers idempotent re-run (lines 49–53) and drift between committed specs and generated output (lines 86–91), but omits the critical edge case from the design: when
syncre-applies a revised delta and discovers thatspecs/has drifted from the recorded baseline because a later change touched the same requirement. Per design.md Decision 3, the engine must refuse to silently reverse-then-apply over someone else's edit and instead report drift.Add a scenario under
Applied-Delta BaselineorDrift Check Modecovering this case:#### Scenario: Baseline drift on re-sync - **WHEN** sync re-applies a revised delta and the current `openspec/specs/` content for an affected spec differs from the recorded baseline pre-image - **THEN** the command reports baseline drift for that spec - **AND** it exits with a non-zero status code and modifies no files - **AND** it does not proceed with the merge🤖 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-deterministic-sync-and-unarchive/specs/cli-sync/spec.md` around lines 61 - 107, Add a missing scenario for the re-sync drift edge case in the sync spec: when the sync engine re-applies a revised delta and the current openspec/specs/ content no longer matches the recorded applied-delta baseline pre-image, it must report baseline drift instead of proceeding. Update the relevant requirement block around Applied-Delta Baseline / Drift Check Mode to cover that the sync command exits non-zero, modifies no files, and does not continue the merge when this happens, using the same sync terminology already present in the spec.
🧹 Nitpick comments (4)
openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md (2)
132-166: 📐 Maintainability & Code Quality | 🔵 Trivial"Committed" implies git workflow; use "current" or "synced" for generality.
Lines 149 and 153 refer to "committed
openspec/specs/content" and "committedopenspec/specs/" for the correspondence check. The check should apply to the current state ofspecs/whether or not changes are committed. Consider "currentopenspec/specs/content" or "syncedopenspec/specs/content" to avoid tying the requirement to a specific VCS workflow.🤖 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-deterministic-sync-and-unarchive/specs/cli-sync/spec.md` around lines 132 - 166, The wording in the Delta–Spec Correspondence requirement uses “committed” in a way that unnecessarily ties the behavior to git state. Update the requirement text in the sync spec to refer to the current or synced openspec/specs/ content instead, while keeping the same check semantics in the related scenarios. Preserve the meaning in the Requirement: Delta–Spec Correspondence and its scenarios, but replace any “committed” phrasing with neutral terms so the behavior applies regardless of workflow.
117-131: 📐 Maintainability & Code Quality | 🔵 Trivial"root context" is undefined; consider clarifying or referencing.
Line 124 mentions emitting "the root context as JSON" without defining what that includes. If this is a term from existing CLI conventions, add a cross-reference; otherwise, specify the fields (e.g., change name, spec paths affected, baseline status).
Additionally, the blocked-path scenario (lines 128–131) mentions "a machine-readable diagnostic with a stable code" but does not enumerate the codes. For a spec, consider listing the expected diagnostic codes (e.g.,
NO_DELTA,DELTA_NOT_APPLICABLE,CONFLICT,DRIFT,CORRESPONDENCE_FAILURE) so implementations and consumers can rely on them.🤖 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-deterministic-sync-and-unarchive/specs/cli-sync/spec.md` around lines 117 - 131, Clarify the JSON sync spec by defining what “root context” means in the sync command’s JSON output and reference the existing CLI concept or explicitly list its fields (for example, change name, affected spec paths, and baseline status) in the `JSON success` scenario. Also update the `JSON blocked path` scenario in `spec.md` to enumerate the stable diagnostic codes expected from the sync flow so implementations can emit consistent machine-readable errors; use the `openspec sync` JSON scenarios and the requirement text as the place to make these definitions explicit.openspec/changes/add-deterministic-sync-and-unarchive/design.md (2)
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDisambiguate "commit order" from git commits.
"Commit order" in this context means the sequence of file operations, not git commits. Rephrase to "execution order" or "apply order" to avoid confusion.
🤖 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-deterministic-sync-and-unarchive/design.md` around lines 81 - 85, The phrase “commit order” in the Decision 7 text is ambiguous and can be read as git commits instead of file-operation sequencing. Update the wording in the deterministic sync/unarchive design section to use “execution order” or “apply order” when describing the sequence around moveDirectory and the restore/delete steps, so the intent is clear and consistent with the reversal flow.
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify "opaque up to
<name>" phrasing.The intent is clear (strip date/sequence prefix, match on the name portion), but "opaque up to" is awkward. Consider: "treats the leading prefix as strippable up to
<name>" or "prefix is ignored up to<name>".🤖 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-deterministic-sync-and-unarchive/design.md` around lines 76 - 79, Revise the wording in the `Decision` section to replace the awkward “opaque up to <name>” phrasing with clearer language like “strippable” or “ignored up to <name>”. Keep the meaning the same in `design.md`: `unarchive [change-name]` should match by removing the leading archive prefix and using the remaining name portion, while preserving the rest of the behavior described there.
🤖 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-deterministic-sync-and-unarchive/specs/cli-sync/spec.md`:
- Around line 168-187: The cross-change conflict detection requirement is
missing the `--all` mode needed to inspect more than one active change at once.
Update the `openspec sync` spec to add a scenario for `--check --all` (or
equivalent `--all`) that checks every active change for overlapping
requirements, reports conflicts, and returns non-zero. Make sure the `sync`
command definition and the conflict scenarios clearly distinguish single-change
behavior from the new all-changes behavior.
---
Outside diff comments:
In
`@openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md`:
- Around line 61-107: Add a missing scenario for the re-sync drift edge case in
the sync spec: when the sync engine re-applies a revised delta and the current
openspec/specs/ content no longer matches the recorded applied-delta baseline
pre-image, it must report baseline drift instead of proceeding. Update the
relevant requirement block around Applied-Delta Baseline / Drift Check Mode to
cover that the sync command exits non-zero, modifies no files, and does not
continue the merge when this happens, using the same sync terminology already
present in the spec.
---
Nitpick comments:
In `@openspec/changes/add-deterministic-sync-and-unarchive/design.md`:
- Around line 81-85: The phrase “commit order” in the Decision 7 text is
ambiguous and can be read as git commits instead of file-operation sequencing.
Update the wording in the deterministic sync/unarchive design section to use
“execution order” or “apply order” when describing the sequence around
moveDirectory and the restore/delete steps, so the intent is clear and
consistent with the reversal flow.
- Around line 76-79: Revise the wording in the `Decision` section to replace the
awkward “opaque up to <name>” phrasing with clearer language like “strippable”
or “ignored up to <name>”. Keep the meaning the same in `design.md`: `unarchive
[change-name]` should match by removing the leading archive prefix and using the
remaining name portion, while preserving the rest of the behavior described
there.
In
`@openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.md`:
- Around line 132-166: The wording in the Delta–Spec Correspondence requirement
uses “committed” in a way that unnecessarily ties the behavior to git state.
Update the requirement text in the sync spec to refer to the current or synced
openspec/specs/ content instead, while keeping the same check semantics in the
related scenarios. Preserve the meaning in the Requirement: Delta–Spec
Correspondence and its scenarios, but replace any “committed” phrasing with
neutral terms so the behavior applies regardless of workflow.
- Around line 117-131: Clarify the JSON sync spec by defining what “root
context” means in the sync command’s JSON output and reference the existing CLI
concept or explicitly list its fields (for example, change name, affected spec
paths, and baseline status) in the `JSON success` scenario. Also update the
`JSON blocked path` scenario in `spec.md` to enumerate the stable diagnostic
codes expected from the sync flow so implementations can emit consistent
machine-readable errors; use the `openspec sync` JSON scenarios and the
requirement text as the place to make these definitions explicit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ab512a2-6f9b-4090-9454-e632d8e87fc2
📒 Files selected for processing (4)
openspec/changes/add-deterministic-sync-and-unarchive/design.mdopenspec/changes/add-deterministic-sync-and-unarchive/proposal.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/cli-sync/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/tasks.md
✅ Files skipped from review due to trivial changes (1)
- openspec/changes/add-deterministic-sync-and-unarchive/tasks.md
…nspec format) Per request, promote the spec linter/formatter from a deferred follow-up to a first-class capability in this PR. New capability cli-format: `openspec format [target]` — a deterministic, behavior-preserving spec/delta formatter with a `--check` gate, sharing ONE canonicalizer with the merge engine (sync/archive) so their output cannot diverge: synced/archived specs pass `format --check` by construction. - Behavior-preserving: normalizes only presentation (whitespace, blank lines, list markers, heading spacing, canonical delta-section headers); never reorders requirements/scenarios or rewrites prose (parse-before==parse-after). - `--check` is a second model-free gate (CI + pre-commit) alongside sync --check; `--fix`/default writes; incremental via digests; --json. - Pure text → no agent, no /opsx:format skill. Not validate (semantic) and not code-vs-spec verify (inference) — those compose around it. Extract a shared src/core/spec-canonical.ts so merge engine + formatter use one canonicalizer. New design Decision 14; Decision 13's deferral removed. New tasks phase 2A + canonicalizer extraction (1.1a). Updated proposal, goals/non-goals. Validates with `openspec validate --strict`. No human names in PR content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
openspec sync + openspec unarchive (+ /opsx:unarchive)openspec sync + unarchive + format (+ /opsx:unarchive)
There was a problem hiding this comment.
🧹 Nitpick comments (6)
openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-format/spec.md (3)
38-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSpecify the canonical line ending format.
The scenario states that "the canonical output uses normalized line endings regardless of the platform" but does not specify whether the canonical form is LF, CRLF, or otherwise. Without this, implementers may choose differently on different platforms, violating the byte-identical requirement on line 26.
Add an explicit assertion that the canonical line ending is LF (or your chosen convention).
🤖 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-deterministic-sync-and-unarchive/specs/cli-format/spec.md` around lines 38 - 42, The “Line endings normalized” scenario in cli-format/spec.md is underspecified because it says normalized line endings without naming the canonical form. Update the scenario to explicitly state the canonical output uses LF line endings, and make sure the wording aligns with the existing byte-identical requirement so implementers can follow the same convention consistently.
116-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClarify digest storage and invalidation for incremental checking.
The incremental checking requirement mentions "recorded content digests" but does not specify:
- Where digests are recorded (a cache file?
.git?.openspec/?)- How the digest is keyed (by path? by content hash?)
- Invalidation semantics (e.g., when the canonicalizer version changes)
Without this, implementers may build incompatible caching strategies that silently skip re-checking after formatter logic updates.
Add a scenario or note defining the digest storage location and invalidation conditions.
🤖 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-deterministic-sync-and-unarchive/specs/cli-format/spec.md` around lines 116 - 118, Clarify the Incremental Checking requirement by defining where recorded content digests live, how they are keyed, and when they must be invalidated. Update the spec around the format check behavior to reference the incremental cache explicitly (for example, the cache owned by the CLI format implementation) and state that skips are only valid when both the file content and the canonicalizer/formatter version are unchanged. Add a note or scenario near the Incremental Checking requirement that names the digest store, the path/content-hash keying strategy, and the version-based invalidation rule so implementations stay compatible.
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference where "conventions" are defined.
The scenario refers to "the canonical spacing defined by the conventions" without linking to where those conventions are specified. This creates an undefined reference that could lead to inconsistent implementation.
Add a cross-reference to the conventions document or inline the canonical spacing rules.
🤖 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-deterministic-sync-and-unarchive/specs/cli-format/spec.md` around lines 66 - 69, The scenario text in the CLI format spec uses an undefined reference to “the conventions,” so update the canonical heading/spacing wording in the affected scenario to point to the actual conventions source. Use the spec section’s existing scenario title (“Canonical headings and spacing”) and either add a cross-reference to the conventions document or inline the exact spacing rules so the formatter behavior is unambiguous.openspec/changes/add-deterministic-sync-and-unarchive/proposal.md (3)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFlags
--storeand--jsonappear without prior introduction.
--store(sync, unarchive) and--json(format, sync, unarchive) are mentioned here but not introduced in the "What Changes" overview (lines 31-41). Consider adding them to the initial command descriptions for completeness.🤖 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-deterministic-sync-and-unarchive/proposal.md` around lines 53 - 56, The command summaries in proposal.md mention `--store` and `--json` before those flags are introduced in the “What Changes” overview. Update the earlier descriptions for the affected entries (`cli-sync`, `cli-unarchive`, and `cli-format`) to explicitly include these flags so the option set is complete and consistent throughout the document.
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree successive sentences begin with "It." Consider varying the openings for readability.
Current: "It joins... It is not... It is not..."
Suggested: "It joins... This is not... Nor is 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-deterministic-sync-and-unarchive/proposal.md` at line 41, The paragraph in the `openspec format` section has repetitive sentence openings starting with “It,” which hurts readability. Revise the wording in that description so the three consecutive sentences use varied openings, keeping the same meaning while changing the later “It is not” sentences to alternatives such as “This is not” and “Nor is it.”
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value"Three modes" is imprecise — only two are described.
The text lists: (1) default /
--fix(treated as one), and (2)--check. That's two modes, not three. Either split default and--fixas separate (if they differ), or correct the count to "Two modes."🤖 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-deterministic-sync-and-unarchive/proposal.md` around lines 31 - 33, The “Three modes” description in the `openspec sync [change]` section is inaccurate because only two behaviors are defined: default/`--fix` and `--check`. Update the wording in the proposal text to either say “Two modes” or clearly separate default and `--fix` only if they are intended to be distinct, keeping the `cli-sync` description consistent.
🤖 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.
Nitpick comments:
In `@openspec/changes/add-deterministic-sync-and-unarchive/proposal.md`:
- Around line 53-56: The command summaries in proposal.md mention `--store` and
`--json` before those flags are introduced in the “What Changes” overview.
Update the earlier descriptions for the affected entries (`cli-sync`,
`cli-unarchive`, and `cli-format`) to explicitly include these flags so the
option set is complete and consistent throughout the document.
- Line 41: The paragraph in the `openspec format` section has repetitive
sentence openings starting with “It,” which hurts readability. Revise the
wording in that description so the three consecutive sentences use varied
openings, keeping the same meaning while changing the later “It is not”
sentences to alternatives such as “This is not” and “Nor is it.”
- Around line 31-33: The “Three modes” description in the `openspec sync
[change]` section is inaccurate because only two behaviors are defined:
default/`--fix` and `--check`. Update the wording in the proposal text to either
say “Two modes” or clearly separate default and `--fix` only if they are
intended to be distinct, keeping the `cli-sync` description consistent.
In
`@openspec/changes/add-deterministic-sync-and-unarchive/specs/cli-format/spec.md`:
- Around line 38-42: The “Line endings normalized” scenario in
cli-format/spec.md is underspecified because it says normalized line endings
without naming the canonical form. Update the scenario to explicitly state the
canonical output uses LF line endings, and make sure the wording aligns with the
existing byte-identical requirement so implementers can follow the same
convention consistently.
- Around line 116-118: Clarify the Incremental Checking requirement by defining
where recorded content digests live, how they are keyed, and when they must be
invalidated. Update the spec around the format check behavior to reference the
incremental cache explicitly (for example, the cache owned by the CLI format
implementation) and state that skips are only valid when both the file content
and the canonicalizer/formatter version are unchanged. Add a note or scenario
near the Incremental Checking requirement that names the digest store, the
path/content-hash keying strategy, and the version-based invalidation rule so
implementations stay compatible.
- Around line 66-69: The scenario text in the CLI format spec uses an undefined
reference to “the conventions,” so update the canonical heading/spacing wording
in the affected scenario to point to the actual conventions source. Use the spec
section’s existing scenario title (“Canonical headings and spacing”) and either
add a cross-reference to the conventions document or inline the exact spacing
rules so the formatter behavior is unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5fec863-4430-4161-8379-b68e3136e141
📒 Files selected for processing (4)
openspec/changes/add-deterministic-sync-and-unarchive/design.mdopenspec/changes/add-deterministic-sync-and-unarchive/proposal.mdopenspec/changes/add-deterministic-sync-and-unarchive/specs/cli-format/spec.mdopenspec/changes/add-deterministic-sync-and-unarchive/tasks.md
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/add-deterministic-sync-and-unarchive/tasks.md
- openspec/changes/add-deterministic-sync-and-unarchive/design.md
…cope-complete Promote the spec-aware diff driver from a deferred follow-up to a first-class capability, completing the discussion's "review = read spec deltas + reasoning inline" vision deterministically. New capability cli-diff: `openspec diff [target] [--base <ref>]` — a deterministic, pure-code renderer that splices each changed requirement's provenance and rationale inline (what changed and why, together). Usable standalone or, opt-in, as a git diff driver via .gitattributes (never alters git config without consent). Key insight: OpenSpec ALREADY has the "log" — the why lives in proposal.md, the what/where is the applied-delta provenance this PR records — so diff joins existing artifacts rather than building the sidecar reasoning database the discussion reached for. No inference; honest about unattributable changes; not semantic review (that's the verify direction). New design Decision 15 (+ Decision 12 deferral removed) and a "Scope completeness" note: the proposal now covers every deterministic, in-scope idea from the discussion, so later sessions should shift from expansion to sequencing/refinement for owner review. New tasks phase 2B. PR now 7 capabilities. Validates with `openspec validate --strict`. No human names in PR content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
openspec sync + unarchive + format (+ /opsx:unarchive)
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The baseline primitive is the right spine, but this proposal has grown too broad for one implementation pass. I’d narrow the first slice to deterministic merge/baseline, sync check/fix, archive using that path, and CLI-delegated /opsx:sync, then split unarchive, format, and diff into follow-ups once the primitive is proven.
… gate (pre-commit + CI) Address all four CodeRabbit review findings on PR Fission-AI#1279: - cli-archive: pin the applied-delta baseline storage location + versioned schema (.openspec/merge-baseline.json + .openspec/pre-merge/<cap>/spec.md; unknown schema version => treated as no baseline). - cli-unarchive: specify the atomic validate->stage->commit sequence and the per-step failure/rollback semantics. - cli-unarchive: make pre-baseline reversal all-or-nothing (refuse entirely on any MODIFIED/REMOVED rather than partially reverse) for atomicity. - cli-sync: add the --all flag for cross-change conflict detection. Build the deferred CI/hook items (they serve the deterministic linter): new capability cli-check — `openspec check [--fix] [--all]`, the unified deterministic linter that runs format --check + sync --check + validate in one gate. Answers "pre-commit OR CI?": BOTH — same binary, identical verdict; a drift gate is a pure function of the committed files. Ships a runner-agnostic opt-in hook installer and a copy-paste CI step (no model, no API keys). --fix only does mechanical remediation; conflicts still fail. design Decision 16 (+ Decision 4 updated from "staged" to "built"). Now 8 capabilities. Validates with `openspec validate --strict`. No human names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
|
Thanks for the review — all four findings are addressed in the latest push (the change folder was renamed to
Separately, this push also builds the previously-deferred CI/pre-commit items as a new |
|
Thanks Clay. I verified the latest head validates and the four concrete review findings are addressed: baseline storage/schema, unarchive atomicity, pre-baseline all-or-nothing behavior, and cross-change |
clay-good
changed the title
…slice) Per @alfred-openspec's review on Fission-AI#1279 ("too broad for one implementation pass"), narrow this proposal to the load-bearing first slice and split the rest into a follow-up: Keep (this PR): the applied-delta baseline + deterministic merge engine, `openspec sync` (+ `--check`/`--fix`), `archive` routed through the engine, and `/opsx:sync` delegating to the CLI. Capabilities: cli-sync (new), cli-archive (modified), specs-sync-skill (modified). Design Decisions 1-5, 11-13. Move to follow-up (add-spec-tooling-suite): unarchive, format, diff, and the unified `check` gate. Capabilities cli-unarchive, opsx-unarchive-skill, cli-format, cli-diff, cli-check; design Decisions 6-10, 14-16. Rename the change add-deterministic-sync-and-unarchive -> add-deterministic- spec-sync to match the narrowed scope. Validates clean under `openspec validate add-deterministic-spec-sync --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved. This now matches the first-slice scope I was pushing for: deterministic merge/baseline, sync check/fix, archive through the same path, and CLI-delegated /opsx:sync, with unarchive/format/diff/check split into follow-up #1288.\n\nI re-validated the renamed change () and checked the current diff/CI. The scope is now tight enough to merge as a proposal.
|
Tiny correction to my approval note: the validated renamed change is |
TabishB
left a comment
TabishB
left a comment
There was a problem hiding this comment.
As mentioned previously, deterministic merging is something we already had and would go wrong for a variety of reasons. It requires a level of determinism when produce the delta itself that is hard to control. Especially when further edits are made to the delta.
|
|
||
| ## Context | ||
|
|
||
| OpenSpec's spec merge (delta → `specs/`) is reached two ways today: deterministically but only *inside* `openspec archive` (fused to the folder move), or by an AI agent in `/opsx:sync` that "directly edits main specs." The standalone deterministic apply (`applySpecs()`, [src/core/specs-apply.ts:391](../../../src/core/specs-apply.ts)) has zero callers. The design discussion (2026-06) worked from "add unarchive" to a single root cause: **the merge must be deterministic, idempotent, and reversible pure code, exposed as commands** — then a code-only CI drift gate, a `sync --fix` pre-commit hook, crumb-free re-merge, and (later) reverse all fall out of one primitive. |
There was a problem hiding this comment.
I'm not sure whether the merge must be deterministic and idempotent, or if it should be a pure code operation. I think we should just like leave that to Git where possible. Like I don't know how I feel about us trying to make this like a deterministic thing on our end.
Like I'm just not sure of like the complexity involved here, but yeah, I think like the sync command or like the sync skill in general is like ver relatively confusing to users, so I'm not sure, but I'll read through more of this.
| ## Goals / Non-Goals | ||
|
|
||
| **Goals** | ||
| - A deterministic merge: same (delta + base) → byte-identical `specs/`, in pure code, on every platform. |
There was a problem hiding this comment.
Again, similar to the archive command itself, I think the tricky part here is the merge step.
The archive command basically does two things: it merges the spec changes, then moves the folder. As discussed in the other PRs, the reason we stopped using it directly is that the deterministic merge was too brittle. It would fail in weird edge cases, and sometimes a purely mechanical merge does not really make sense anyway.
For example, during the merge you might want to rewrite things, simplify them, or make the final spec read more coherently. So I’m not sure this should be treated as a purely deterministic operation.
clay-good
changed the title
|
@clay-good Let me know if we can close this or there's some other action item you want to take from here. |
3 participants
Status
Open, proposal-only. Validates clean under
openspec validate add-deterministic-spec-sync --strict.Narrowed per @alfred-openspec's review. The earlier revision spanned 8 capabilities; alfred agreed the applied-delta baseline is the right spine but asked to "narrow the first slice to deterministic merge/baseline, sync check/fix, archive using that path, and CLI-delegated /opsx:sync, then split unarchive, format, and diff into follow-ups once the primitive is proven." Done:
sync+ archive integration +/opsx:syncdelegation).add-spec-tooling-suite) carriesunarchive,format,diff, and the unifiedcheckgate — everything that builds on the baseline.add-deterministic-sync-and-unarchive→add-deterministic-spec-syncto match.What's missing (the motivation)
The spec merge (delta →
specs/) is the load-bearing op in OpenSpec, reachable today only insidearchiveor via the agent-driven/opsx:sync— which is non-deterministic, and whose scenario-merge is the mechanism behind #1246's silent scenario drops. The deterministic engineapplySpecs()has zero callers. So today you cannot gate spec drift in CI without a model, cleanly re-merge a revised delta, or catch merge conflicts before archive.What it does (first slice)
One primitive — a per-change applied-delta baseline (pre-image + applied-result digest + per-edit provenance, recorded whenever deltas merge into
specs/) — plus one engine, exposed as:cli-sync(new) — deterministic, idempotentopenspec sync [change];--fix/default regeneratesspecs/(byte-identical for the same delta+base; a revised delta regenerates with no crumbs);--checkis a model-free drift gate for CI + pre-commit that also surfaces conflicts early (delta-vs-base and cross-change), verifies delta↔spec correspondence, and runs incrementally (incremental verdict == full verdict).cli-archive(modified) — routes its merge through the shared engine and records the baseline before moving, so archiving becomes deterministically reversible (unlocking the follow-upunarchive). Forward-only, no output change.specs-sync-skill(modified) —/opsx:syncdelegates toopenspec syncinstead of editing specs in the agent → fixes the Archiving two changes that MODIFY the same requirement silently drops scenarios (distinct from #1112) #1246 scenario-drop class by construction.Rejected, and it shapes the design (design.md): removing
archive/ folding the merge intoapply(breaks thespecs/= shipped-reality invariant, Decision 5); editingspecs/in place with a sidecar reasoning log (breaks proposed-vs-shipped isolation, Decision 11). Their valid kernels are adopted within the delta model — early conflict detection and provenance/correspondence viasync --check(Decisions 11–12).Proof (for a proposal)
openspec validate add-deterministic-spec-sync --strict.applySpecs()has zero callers today (the deterministic engine exists but is unreachable), and the non-deterministic agent merge in/opsx:syncis the documented mechanism behind Archiving two changes that MODIFY the same requirement silently drops scenarios (distinct from #1112) #1246.Notes / scope
unarchive,format,diff, andcheckon this baseline; it's reviewable now and buildable once this merges.specs/); feat(skills): propose /opsx:update planning-artifact update skill #1278 is the artifact-graph drift layer (proposal→design→tasks). Same digest philosophy, different layer.verifydirection, [Proposal] /opsx:validate command to check code against the living specs #880).🤖 Generated with Claude Code