lifecycle: status — date sharding + openspec migrate - #2
Conversation
ixxie
changed the title
ixxie
left a comment
ixxie
left a comment
There was a problem hiding this comment.
Self-review of layer 2 (typecheck clean, 225 targeted tests green; all findings below reproduced empirically in a scratch repo).
🔴 1. Sharded layout breaks every change-enumerating surface outside list/sync/ship
The shared discovery covers list/sync/ship/creation only. Still enumerating flat:
src/utils/item-discovery.ts—getActiveChangeIdsfeedsvalidate,show,status,instructions, and shell completionssrc/core/view.ts(readdirSyncofchanges/)src/commands/workflow/shared.ts
On a migrated tree this is actively wrong, not just blind: getActiveChangeIds returns ['2026'] — the year shard reported as a change. First thing an upstream maintainer will hit after running the demo migration. Fix is the same retarget through discoverChanges already done for list.
🔴 2. Legacy name reuse migrates into permanent id ambiguity, silently
archive/2026-05-12-add-auth/ + active add-auth/ is idiomatic under archive mode — the date prefix exists precisely to allow reuse. migrate shards both to id add-auth with no warning (even in --dry-run); resolveChangeDir then throws ambiguous forever, so sync add-auth / ship add-auth are permanently broken and the name is unusable for new changes. Needs pre-flight duplicate-id detection in the plan — refuse (or require rename) naming the colliding paths, in dry-run too. Underlying design question for the proposal text: a change's identity in sharded mode is really (date, name) but every command takes a bare name — either forbid reuse or specify disambiguation.
🟠 3. Interrupted migration wedges the tree permanently
The config flip is last, so a mid-run crash (e.g. finding 2's sibling: two proposed changes colliding on a flat target) leaves archive-mode config + partial shards. Re-run: toStatus's flat scan treats 2026/ as a change named 2026 and dies on EINVAL: rename changes/2026 → changes/2026/08/13-2026 — every time, unrecoverable without git restore. Fix: skip YEAR_DIR-matching entries in the flat scan (reuse the shard rule); with finding 2's pre-flight the whole plan validates before the first rename.
Recommendation
Hold this layer until 1–3 are fixed (1 and 3 are small; 2 needs the refuse-vs-rename design call). Layer 1 peels upstream independently as-is.
| }); | ||
| } | ||
|
|
||
| for (const entry of await this.dirs(changesDir)) { |
There was a problem hiding this comment.
Finding 3 anchor: this flat scan sees a partially-migrated tree's 2026/ year shard as a change named 2026 and later attempts rename changes/2026 → changes/2026/08/13-2026 → EINVAL, wedging every re-run. Skip entries matching YEAR_DIR here, mirroring discoverChanges' shard rule.
| // shipped-but-unfolded change must be folded (or unshipped) first. Reuse | ||
| // the gate itself rather than a parallel reimplementation of its verdict. | ||
| const exitBefore = process.exitCode; | ||
| const silencedLog = console.log; |
There was a problem hiding this comment.
Monkey-patching console.log and sniffing process.exitCode to read the gate's verdict will draw upstream review fire, and it's blind if exitCode was already 1. SyncCommand.execute already builds a SyncReport — have it return the report and keep the exit-code side effect at the CLI edge; this call site then just reads report.clean.
| // would be a second, contradicting record. | ||
| delete stamped.status; | ||
| } | ||
| await fs.writeFile(path.join(move.to, '.openspec.yaml'), stringifyYaml(stamped), 'utf-8'); |
There was a problem hiding this comment.
"Tolerant raw-YAML" is half-true: unknown keys survive, but stringifyYaml destroys comments and key order in .openspec.yaml. Probably fine for a machine-written file — but either say so in the PR body or round-trip via yaml's Document API.
| // fall through to sharded lookup | ||
| } | ||
|
|
||
| const matches = (await discoverChanges(changesDir)).filter((c) => c.id === id); |
There was a problem hiding this comment.
Finding 2 anchor: this ambiguity throw is correct, but migrate can create the ambiguous state from an idiomatic legacy repo (archived 2026-05-12-add-auth + active add-auth) with no warning — after which this throws forever for that id. The migration plan needs a duplicate-id pre-flight.
034f545 to
de2efb2
Compare
|
Review findings addressed in de2efb2 (branch rebased on the updated lifecycle-status):
New regression tests for all five; full suite green minus the 4 known environmental failures. |
ixxie
left a comment
ixxie
left a comment
There was a problem hiding this comment.
Second pass over de2efb2/c4f0d7e/fd9e069: all first-pass findings verified fixed — re-ran the repros empirically (ambiguity refused with collisions named incl. --dry-run; half-migrated tree resumes to completion with the config flipped; getActiveChangeIds returns real ids on a sharded tree; a hand-written YAML comment survives stamping). tsc clean, 244 tests green. The traversal guard on ids is a nice unprompted addition.
Three new findings, all minor:
🟡 1. resolveChangeDir hands out shard dirs as changes
The flat-first stat bypasses the shard rule: resolveChangeDir(cd, '2026') returns changes/2026 (verified). On a sharded tree, show 2026 / validate 2026 then operate on the year directory. Discovery already makes such names invisible — the resolver should refuse YEAR_DIR/archive names before the flat stat so the two agree on what is addressable.
🟡 2. openspec new change reports the wrong path under status mode
src/commands/workflow/new-change.ts:81 prints formatChangeLocation — a flat join — while createChange actually sharded the dir. The happy-path success message points at a directory that doesn't exist. formatChangeLocation needs a shard-aware variant (or take the created dir from createChange's return).
⚪ 3. extractNameFromPath shard logic duplicated
json-converter.ts and validator.ts now carry identical shard-recognition blocks (the commit message itself says "both copies"). Extract a shared helper — upstream review will ask for it otherwise.
Also still open from pass 1: the proposal pitches sync --check --changed as the scoped pre-commit tier, but only the change-name positional exists.
| * a hostile id cannot address anything outside changes/. | ||
| */ | ||
| export async function resolveChangeDir(changesDir: string, id: string): Promise<string | null> { | ||
| if (!id || id.startsWith('.') || id.includes('/') || id.includes('\\') || id.includes('\0')) { |
There was a problem hiding this comment.
New finding 1 anchor: this guard rejects hostile ids, but the flat stat below still resolves '2026' to the year shard dir on a sharded tree (verified). Add YEAR_DIR.test(id) || id === 'archive' to the refusal so the resolver and discovery agree on the addressable namespace.
| for (let i = parts.length - 1; i >= 0; i--) { | ||
| if (parts[i] === 'specs' || parts[i] === 'changes') { | ||
| if (i < parts.length - 1) { | ||
| // creation-date sharded layout: changes/YYYY/MM/DD-<name>/... |
There was a problem hiding this comment.
New finding 3 anchor: this block and its twin in validator.ts are byte-identical — extract a shared changeNameFromPath helper (change-discovery.ts is the natural home) before upstream asks.
| * (flat or creation-date sharded), falling back to the flat join so callers | ||
| * on a not-yet-created change still get a path to report. | ||
| */ | ||
| export async function resolvePlanningChangeDir( |
There was a problem hiding this comment.
New finding 2 anchor: formatChangeLocation below is still a flat join, and its one caller (new-change.ts:81) prints it as the location of a change createChange just sharded — the success message points at a nonexistent path under status mode. Needs the same shard-aware treatment as this resolver.
|
Second-pass findings addressed in bf2debd + e50da8a:
On the pass-1 leftover: the proposal doc was already softened in the first round — Note for posterity: |
|
Verified |
e50da8a to
60075e4
Compare
Changes under `lifecycle: status` shard as changes/YYYY/MM/DD-<name>/ — assigned at birth, immutable, so location encodes only the creation date and nothing ever moves. A shared discovery module enumerates both layouts (YYYY/MM dirs are shards to walk into; anything else is a change; the DD- prefix strips from the id), list/sync/ship resolve ids through it, and createChange shards new changes. Duplicate ids across shard dates are rejected at creation and on lookup. `openspec migrate` converts a legacy project one way: archived changes become changes/YYYY/MM/DD-<name>/ with status: shipped (the folder date's meaning shifts from archival to creation — the closest surviving record), active changes shard by their created date as proposed, config gains lifecycle: status. Metadata edits are tolerant raw-YAML key writes, never strict-schema round-trips — a migration that drops fields it does not understand destroys history. --dry-run prints the plan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reversal moves only bookkeeping, like the forward direction: shipped changes return to changes/archive/YYYY-MM-DD-<name>/ (dates from the shard path), proposed changes return flat, the status key is stripped (under archive mode, location is the state), empty shard dirs prune, and the config line disappears. No spec text changes in either direction — archive-mode specs/ is folded shipped reality, which is exactly what status-mode maintains, so the round-trip is a pure relayout (covered by a round-trip test). Refuses while any shipped change has unfolded deltas: the archive layout asserts folds that must actually exist. One honest asymmetry, printed on completion: changes shipped under status mode carry their creation date into the archive folder name, where convention reads an archival date. This is the exit ramp the experimental flag's exit criteria require — if the mode is ever removed rather than graduated, --to archive is how projects return to supported ground. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mbiguity and survives interruption Review fixes on the sharded layout: - getActiveChangeIds, getAvailableChanges and the view dashboard enumerate through discoverChanges, so a migrated tree no longer reports the year shard as a change named '2026'; show, validate, status and instructions resolve sharded dirs via resolveChangeDir (with the flat join kept as fallback behind the traversal guard). - resolveChangeDir returns null for ids discovery could never produce (separators, dot segments), so hostile ids cannot address anything outside changes/; discovery skips hidden dirs like the flat scan did. - migrate pre-flights id ambiguity: a legacy name reused across archive eras — idiomatic under archive mode — would shard into two dirs no bare id can address, so the plan is refused with the collisions named before the first rename (dry-run included). Duplicate targets refuse likewise instead of clobbering. - an interrupted migration now resumes: the flat scan skips year shards left by a partial run instead of renaming changes/YYYY into itself. - the reverse-migration gate reads SyncCommand's returned report via the new silent option instead of monkey-patching console.log and sniffing process.exitCode. - metadata stamping edits the YAML document in place, preserving comments and key order legacy files may carry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live QA on the demo caught show --json reporting a sharded change as '2026': both extractNameFromPath copies took the segment after 'changes/', which in the sharded layout is the year. They now recognize changes/YYYY/MM/DD-<name>/ and return the de-prefixed name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… name derivation share one source of truth Second-pass review fixes: - resolveChangeDir refuses 'archive' and year-shaped ids, so the resolver and discovery agree on the addressable namespace — the flat-first stat no longer hands out changes/2026 as a change. - openspec new change derives its success message from the dir createChange actually made instead of a flat join, so under lifecycle: status it prints the sharded path that exists. - the two byte-identical extractNameFromPath copies delegate to a shared itemNameFromPath in change-discovery, where the shard rule lives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/ under status mode Live QA caught openspec new change scaffolding changes/archive/ into a status-mode tree — the one directory the mode abolishes. The root-completion scaffold is now mode-aware; root health never required the dir (a missing archive/ raises no diagnostic). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
60075e4 to
ae3107a
Compare
|
Superseded: layer 2 is now part of the single upstream PR (Fission-AI#1684), which carries the whole design per the vision issue Fission-AI#1683. The staging stack is no longer split. |
1 participant
Layer 2 of 2, stacked on #1 — review only this layer's diff.
Creation-date sharding (
changes/YYYY/MM/DD-<name>/, assigned at birth, immutable — location never encodes lifecycle state) via a shared discovery module poweringlist/sync/ship/creation; andopenspec migrate, the one-way legacy conversion: archived changes become shardedshipped(the folder date's meaning shifts from archival to creation), active changes shard asproposed, config flips, nothing is deleted.--dry-runprints the plan; metadata stamping is tolerant raw-YAML so unknown legacy fields survive.Demonstrated end-to-end in the demo repo's migration commit: five months of real archive-mode history converted by one command, gate green immediately — https://github.com/ixxie/openspec-status-demo
🤖 Generated with Claude Code