feat: Add hierarchical specs change proposal#839
Open
victorhsb wants to merge 2 commits intoFission-AI:mainfrom
Open
feat: Add hierarchical specs change proposal#839victorhsb wants to merge 2 commits intoFission-AI:mainfrom
victorhsb wants to merge 2 commits intoFission-AI:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughThis PR implements hierarchical spec support for OpenSpec, enabling nested spec organization through forward-slash-based IDs (e.g., Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/CLI
participant CLI as CLI Handler
participant Discovery as getSpecIds()
participant Resolver as specIdToPath()
participant Parser as MarkdownParser
participant UI as Console Output
User->>CLI: openspec spec list [prefix]
CLI->>Discovery: getSpecIds(specsDir)
Discovery-->>CLI: [cli/show, cli/validate, domain/feature, ...]
CLI->>CLI: Filter by prefix (if provided)
CLI->>Resolver: specIdToPath(filtered IDs)
Resolver-->>CLI: File paths
CLI->>Parser: Parse spec.md for each ID
Parser-->>CLI: Requirement counts
CLI->>UI: Display table with hierarchical IDs
UI-->>User: Spec id | Requirement count
sequenceDiagram
actor User as User
participant CLI as validate CLI
participant TypeDetect as Type Detection
participant Resolver as specIdToPath()
participant FileSystem as File System
participant Validator as Validator
participant Output as Output
User->>CLI: openspec validate cli/show
CLI->>TypeDetect: Detect hierarchical spec ID
TypeDetect->>Resolver: Resolve cli/show to path
Resolver-->>TypeDetect: openspec/specs/cli/show/spec.md
TypeDetect->>FileSystem: Check file exists
FileSystem-->>TypeDetect: File found
TypeDetect->>Validator: Validate spec
Validator-->>Output: Validation result with hierarchical ID
Output-->>User: Spec cli/show valid/invalid
sequenceDiagram
actor User as User
participant Archive as Archive Command
participant Discovery as Glob Pattern
participant IdExtract as specId Extraction
participant Resolver as specIdToPath()
participant Source as Source File
participant Target as Target Directory
participant Confirm as Confirmation Prompt
User->>Archive: Run archive
Archive->>Discovery: Find spec.md under changes/*/specs/
Discovery-->>Archive: [cli/show/spec.md, domain/feature/spec.md, ...]
loop For each delta spec
Archive->>IdExtract: Extract specId from path
IdExtract-->>Archive: cli/show, domain/feature
Archive->>Resolver: Resolve target path
Resolver-->>Archive: openspec/specs/cli/show/spec.md
Archive->>Target: Create intermediate directories
Target-->>Archive: Dirs created
Archive->>Source: Read delta spec content
Source-->>Archive: Content
end
Archive->>Confirm: Display confirmation with hierarchical paths
Confirm-->>User: NEW/EXISTING specs with full hierarchical IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (1)
openspec/changes/hierarchical-specs/specs/cli-list/spec.md (1)
19-21: Clarify subtree prefix matching semantics to avoid false positives.Please explicitly define segment-boundary matching for subtree filters (e.g.,
cli/should not matchclient/...). Without this, implementations may incorrectly use rawstartsWith("cli").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/hierarchical-specs/specs/cli-list/spec.md` around lines 19 - 21, The subtree filter semantics are ambiguous—implementations must perform segment-boundary matching rather than raw prefix matching for filters like the `--specs` argument; update the text around the examples for `openspec list --specs cli/` to state that a trailing slash denotes a segment boundary and that the filter should match only IDs whose next character after the prefix is a path separator (e.g., `cli/` matches `cli/foo` and `cli/bar/baz` but not `client/foo`), and add a short clarifying example showing a non-match (`client/...`) to prevent using startsWith("cli").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/hierarchical-specs/proposal.md`:
- Line 13: The "**BREAKING**" label on the sentence about flat spec IDs
conflicts with the compatibility guarantee; change the label to a
compatibility/compat note or explicitly enumerate the exact breaking surface if
one exists (e.g., rename "**BREAKING**" to "**COMPATIBILITY**" or replace the
sentence with a note like "Compatibility: Existing flat spec IDs remain valid;
tools that treat spec IDs as opaque strings need no changes, but tools that
parse spec ID structure must be updated" and if there truly is a breaking
change, state the precise affected behavior and the conditions under which
migration is required, referencing the "**BREAKING**" token and the sentence
about flat spec IDs for context).
---
Nitpick comments:
In `@openspec/changes/hierarchical-specs/specs/cli-list/spec.md`:
- Around line 19-21: The subtree filter semantics are ambiguous—implementations
must perform segment-boundary matching rather than raw prefix matching for
filters like the `--specs` argument; update the text around the examples for
`openspec list --specs cli/` to state that a trailing slash denotes a segment
boundary and that the filter should match only IDs whose next character after
the prefix is a path separator (e.g., `cli/` matches `cli/foo` and `cli/bar/baz`
but not `client/foo`), and add a short clarifying example showing a non-match
(`client/...`) to prevent using startsWith("cli").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f57b95c7-dffd-4ca2-a69b-486b0a736f0b
📒 Files selected for processing (12)
openspec/changes/hierarchical-specs/.openspec.yamlopenspec/changes/hierarchical-specs/design.mdopenspec/changes/hierarchical-specs/proposal.mdopenspec/changes/hierarchical-specs/specs/cli-archive/spec.mdopenspec/changes/hierarchical-specs/specs/cli-list/spec.mdopenspec/changes/hierarchical-specs/specs/cli-show/spec.mdopenspec/changes/hierarchical-specs/specs/cli-spec/spec.mdopenspec/changes/hierarchical-specs/specs/cli-validate/spec.mdopenspec/changes/hierarchical-specs/specs/cli-view/spec.mdopenspec/changes/hierarchical-specs/specs/hierarchical-spec-discovery/spec.mdopenspec/changes/hierarchical-specs/specs/hierarchical-spec-resolution/spec.mdopenspec/changes/hierarchical-specs/tasks.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/hierarchical-specs/specs/cli-list/spec.md`:
- Around line 25-38: The spec currently mandates table output but the CLI
supports a --json mode, so add an explicit exception: update the "Displaying
change list (default)" and "Displaying spec list" scenarios in
specs/cli-list/spec.md to state that when the --json flag is present the command
SHALL emit machine-readable JSON (structured arrays/objects with fields for
change name, task progress, spec id, requirement count) instead of a
human-readable table; reference the CLI option --json (from the CLI entry point)
as the trigger and ensure the new scenario clarifies the JSON schema or refers
to a separate JSON output schema section.
- Around line 19-23: The CLI contract is ambiguous: choose a single canonical
form for the subtree filter (either make it a valued option like "--specs
<prefix>" or a positional argument "<prefix>") and update the CLI parser and
help text accordingly so "openspec list" unambiguously accepts a prefix value;
concretely, modify the CLI definition where the "--specs" flag is declared to
accept a string value (or add a positional parameter for the prefix) and update
the command handler for "openspec list" to read that value, enforce the existing
segment-boundary matching logic (prefix must end with '/' or be followed by a
path separator), and update any tests/docs that reference the old flag-only
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e218854e-ef51-46c4-9475-81c0f9cee2b5
📒 Files selected for processing (2)
openspec/changes/hierarchical-specs/proposal.mdopenspec/changes/hierarchical-specs/specs/cli-list/spec.md
Comment on lines
+25
to
+38
| The command SHALL display items in a clear, readable table format with mode-appropriate progress or counts. | ||
|
|
||
| #### Scenario: Displaying change list (default) | ||
| - **WHEN** displaying the list of changes | ||
| - **THEN** show a table with columns: | ||
| - Change name (directory name) | ||
| - Task progress (e.g., "3/5 tasks" or "✓ Complete") | ||
|
|
||
| #### Scenario: Displaying spec list | ||
| - **WHEN** displaying the list of specs | ||
| - **THEN** show a table with columns: | ||
| - Spec id (full hierarchical path, e.g., `cli/show` or `cli-show`) | ||
| - Requirement count (e.g., "requirements 12") | ||
|
|
Contributor
There was a problem hiding this comment.
Table-only output requirement conflicts with machine-readable JSON mode.
This section says output “SHALL” be table format, but the command surface includes --json (see src/cli/index.ts Lines 173-190). Add an explicit exception/scenario for JSON output to avoid contradictory requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/hierarchical-specs/specs/cli-list/spec.md` around lines 25 -
38, The spec currently mandates table output but the CLI supports a --json mode,
so add an explicit exception: update the "Displaying change list (default)" and
"Displaying spec list" scenarios in specs/cli-list/spec.md to state that when
the --json flag is present the command SHALL emit machine-readable JSON
(structured arrays/objects with fields for change name, task progress, spec id,
requirement count) instead of a human-readable table; reference the CLI option
--json (from the CLI entry point) as the trigger and ensure the new scenario
clarifies the JSON schema or refers to a separate JSON output schema section.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/list.ts (1)
156-193:⚠️ Potential issue | 🟡 MinorMissing JSON output support for specs mode.
The
executemethod handlesjsonoutput for changes mode (lines 100-101 and 131-141) but the specs mode (lines 167-169 and 186-192) only outputs to console, ignoringoptions.json. This inconsistency breaks programmatic use cases where callers expect JSON output for both modes.Additionally, the empty-result case (line 168) should also respect the
jsonflag, matching the pattern used in changes mode (line 101).🐛 Proposed fix to add JSON output for specs mode
+ if (allIds.length === 0) { + if (json) { + console.log(JSON.stringify({ specs: [] })); + } else { + console.log('No specs found.'); + } + return; + }And after collecting specs:
console.log('Specs:'); const padding = ' '; const nameWidth = Math.max(...specs.map(s => s.id.length)); + + if (json) { + console.log(JSON.stringify({ specs }, null, 2)); + return; + } + for (const spec of specs) { const padded = spec.id.padEnd(nameWidth); console.log(`${padding}${padded} requirements ${spec.requirementCount}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/list.ts` around lines 156 - 193, The specs mode in execute currently always prints to console and ignores options.json; update the branch that handles specs (the block that builds SpecInfo[] `specs` from `allIds`, uses `specIdToPath`, `MarkdownParser.parseSpec`, and prints "No specs found.") to mirror changes mode JSON behavior: when `options.json` is true emit a JSON object (e.g., { specs: [{ id, requirementCount }, ...] } or an empty { specs: [] } for the no-specs case) to stdout and return, otherwise keep the existing human-readable console output; ensure you use the existing `specs` array and `SpecInfo` shape and handle the empty-result path by returning the JSON empty list when `options.json` is set.
🧹 Nitpick comments (5)
src/core/specs-apply.ts (1)
441-445: Consider reusingwriteUpdatedSpecto avoid write/log divergence.
applySpecsre-implements directory creation and file writes already encapsulated bywriteUpdatedSpec, which increases maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/specs-apply.ts` around lines 441 - 445, In applySpecs replace the manual directory creation and file write (the block using path.dirname(p.update.target), fs.mkdir(...), and fs.writeFile(...)) with a call to the existing writeUpdatedSpec function so all writes and logging stay centralized; locate the code in applySpecs that writes p.rebuilt to p.update.target and invoke writeUpdatedSpec(p.update.target, p.rebuilt) (or the matching signature) instead, removing the duplicated fs.mkdir/fs.writeFile logic and relying on writeUpdatedSpec’s behavior and logging.openspec/changes/archive/2026-03-13-hierarchical-specs/specs/hierarchical-spec-resolution/spec.md (1)
44-45: Optional: Consider hyphenating "full-path matching" for consistency.Static analysis suggests using a hyphen when "full path" modifies "matching" (i.e., "full-path matching"). This is a minor style nit and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/archive/2026-03-13-hierarchical-specs/specs/hierarchical-spec-resolution/spec.md` around lines 44 - 45, Change the phrase "full path matching" to hyphenated "full-path matching" in the requirement text under the heading "### Requirement: Fuzzy matching for hierarchical IDs" so the compound modifier reads "full-path matching" (keep "leaf-segment matching" as-is) to maintain consistent compound-adjective style.src/commands/spec.ts (1)
153-175: Defensive existence check may indicate stale discovery.The
existsSync(specPath)check at line 166 is defensive, but ifgetSpecIds()returns an ID, the correspondingspec.mdshould exist. Returningnulland filtering suggests a potential race condition or stale cache scenario.Consider logging a warning when a discovered spec ID doesn't resolve to an existing file, as this could indicate a bug in discovery or a concurrent file deletion.
💡 Optional: Add diagnostic logging for missing specs
const specs = allIds.map(id => { const specPath = specIdToPath(id, SPECS_DIR); if (existsSync(specPath)) { try { const spec = parseSpecFromFile(specPath, id); return { id, title: spec.name, requirementCount: spec.requirements.length }; } catch { return { id, title: id, requirementCount: 0 }; } } + // Spec ID was discovered but file doesn't exist - possible race or stale discovery + console.warn(`Warning: Discovered spec '${id}' but file not found at ${specPath}`); return null; }).filter((spec): spec is { id: string; title: string; requirementCount: number } => spec !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spec.ts` around lines 153 - 175, The discovery code in the .action handler uses existsSync(specPath) defensively and silently drops missing specs, which can hide stale/corrupt discovery or concurrent-deletion issues; update the block that iterates allIds (references: getSpecIds, specIdToPath, SPECS_DIR, parseSpecFromFile) to emit a warning via the existing logger (or processLogger) when a spec ID resolves to a non-existent file (include the spec id and resolved specPath in the message), so missing files are visible for diagnostics while keeping the current behavior of skipping unparsable files (still catch parseSpecFromFile errors and log them as parse failures).src/cli/index.ts (1)
179-184: Consider warning when prefix is used with --changes mode.The
prefixargument is passed toListCommand.executeregardless of mode, but only used whenmode === 'specs'. If a user runsopenspec list cli/ --changes, the prefix is silently ignored, which could be confusing.💡 Optional: Warn about ignored prefix in changes mode
.action(async (prefix?: string, options?: { specs?: boolean; changes?: boolean; sort?: string; json?: boolean }) => { try { const listCommand = new ListCommand(); const mode: 'changes' | 'specs' = options?.specs ? 'specs' : 'changes'; + if (prefix && mode === 'changes') { + console.warn('Warning: Prefix filtering is only supported with --specs. Prefix ignored.'); + } const sort = options?.sort === 'name' ? 'name' : 'recent'; await listCommand.execute('.', mode, { sort, json: options?.json, specsPrefix: prefix });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 179 - 184, The code passes prefix into ListCommand.execute unconditionally but execute only uses specsPrefix when mode === 'specs', so if a user supplies a prefix with --changes it is silently ignored; add a user-facing warning before calling new ListCommand().execute: detect if prefix is truthy and mode === 'changes' and emit a concise warning (e.g., console.warn or the CLI logger used in this file) stating the prefix will be ignored in changes mode; keep the rest of the call intact and reference ListCommand, execute, mode, and prefix when making the change.test/specs/hierarchical-specs.integration.test.ts (1)
101-115: Console mock should use try/finally to ensure restoration.If the test throws before line 109,
console.logremains overridden, potentially affecting subsequent tests.🛡️ Wrap in try/finally to ensure cleanup
it('ListCommand with specsPrefix filters correctly', async () => { const output: string[] = []; const origLog = console.log; console.log = (...args: any[]) => output.push(args.join(' ')); + try { const cmd = new ListCommand(); await cmd.execute(testDir, 'specs', { specsPrefix: 'cli/' }); - - console.log = origLog; + } finally { + console.log = origLog; + } const joined = output.join('\n'); expect(joined).toContain('cli/show'); expect(joined).toContain('cli/validate'); expect(joined).not.toContain('client/config'); expect(joined).not.toContain('auth'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/specs/hierarchical-specs.integration.test.ts` around lines 101 - 115, The test overrides console.log (origLog and output) before calling new ListCommand().execute — wrap the call and assertions in a try/finally so console.log is always restored: move the await cmd.execute(...) and subsequent assertions into a try block and restore console.log in a finally block (keeping origLog, output, and joined logic), referencing the ListCommand instantiation and execute(testDir, 'specs', { specsPrefix: 'cli/' }) call and the origLog/console.log override to ensure cleanup even if the test throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/specs/cli/archive/spec.md`:
- Around line 77-82: Update the spec wording that currently implies global
pre-validation ("validate all operations before applying") to state per-delta,
non-transactional semantics: change phrasing around "recursively discover delta
specs" / "parse and apply delta changes" to specify that each delta is validated
and applied independently (validate each delta before applying it, apply deltas
atomically per-delta), and explicitly note that failures in one delta do not
trigger rollback of previously applied deltas and there is no global transaction
across all discovered deltas.
In `@src/core/archive.ts`:
- Around line 119-131: The empty catches around the probe logic can hide I/O
errors and cause hasDeltaSpecs to remain false, skipping
validateChangeDeltaSpecs(); change the error handling in the outer try (around
fg(...) producing matches) and the inner try (around fs.readFile(candidatePath,
...)) so that on any error you either rethrow or mark hasDeltaSpecs = true and
break/exit to force validation; specifically update the blocks that use matches,
specId (from specIdToPath) and content so errors are not silently swallowed—log
the error (or rethrow) and set hasDeltaSpecs = true before breaking/returning so
validateChangeDeltaSpecs() runs reliably.
In `@src/core/specs-apply.ts`:
- Around line 73-77: The loop deriving specId from each match can produce an
empty string for root spec files (match like 'spec.md'), so add a guard after
computing specId (variable specId in the for...of over matches) that checks if
specId === '' and if so throw or return a clear error describing the invalid
root spec delta (including the original match and directories
changeSpecsDir/mainSpecsDir) before calling specIdToPath; this ensures you fail
fast and avoid passing an empty ID into specIdToPath.
In `@src/core/validation/validator.ts`:
- Around line 364-379: The function extractNameFromPath currently only strips
'spec.md' from the tail, causing paths like
'openspec/changes/my-change/proposal.md' to resolve to 'my-change/proposal.md';
update the logic where you handle the tail (inside extractNameFromPath) to check
the anchor segment (parts[i]) and drop the appropriate trailing filename: if the
anchor is 'specs' remove a trailing 'spec.md', and if the anchor is 'changes'
remove a trailing 'proposal.md' (or generically drop either 'spec.md' or
'proposal.md' if present), then return the remaining joined tail so change IDs
become just 'my-change'.
In `@src/utils/item-discovery.ts`:
- Around line 34-43: getSpecIds currently only guards fs.access but calls
fg('**/spec.md', { cwd: specsPath, ... }) which can throw and cause hard
failures; wrap the fast-glob call (the fg invocation that produces matches from
specsPath) in a try/catch (or wrap the body of getSpecIds) and on any error
return an empty array [] so transient/unreadable-tree I/O errors are handled
consistently with the existing fs.access check; locate the fg call and the
matches variable inside getSpecIds to apply this change.
In `@src/utils/spec-paths.ts`:
- Around line 16-24: The function pathToSpecId currently unconditionally pops
the last path segment which corrupts IDs when filePath doesn't end with
"spec.md"; update pathToSpecId to normalize the relative path (variable
relative), inspect the final segment in parts (or use path.basename) and only
remove it if it equals "spec.md" — otherwise throw a clear Error (or return the
normalized relative path unchanged) so callers know the input shape is invalid;
adjust callers/tests accordingly to handle the explicit failure.
- Around line 7-10: The specIdToPath function currently joins raw specId
segments into a filesystem path, which allows traversal via '.'/''/'..'
segments; sanitize and validate the segments before joining: split specId,
filter out empty and '.' segments, reject or throw on any '..' segment (or
otherwise escape it), then use path.join(baseDir, ...cleanSegments, 'spec.md')
and finally assert the normalized result remains within baseDir (e.g., compare
path.resolve(baseDir) with path.resolve(result).startsWith) to prevent directory
traversal; update specIdToPath accordingly.
In `@test/commands/spec.test.ts`:
- Around line 314-327: The test "should not match on partial segment boundary
(client/ should not match cli/)" has a race where fs.mkdir(...).then(...) and
fs.writeFile are not awaited before running execSync; make the test reliably
synchronous by marking the test callback async and awaiting the directory
creation and file write (await fs.promises.mkdir and await fs.promises.writeFile
or await the promise chain) prior to calling execSync(`node ${openspecBin} spec
list cli/`); ensure you reference the same identifiers used here (the test name
string, specsDir, openspecBin) so the spec file exists before the execSync
assertion runs.
---
Outside diff comments:
In `@src/core/list.ts`:
- Around line 156-193: The specs mode in execute currently always prints to
console and ignores options.json; update the branch that handles specs (the
block that builds SpecInfo[] `specs` from `allIds`, uses `specIdToPath`,
`MarkdownParser.parseSpec`, and prints "No specs found.") to mirror changes mode
JSON behavior: when `options.json` is true emit a JSON object (e.g., { specs: [{
id, requirementCount }, ...] } or an empty { specs: [] } for the no-specs case)
to stdout and return, otherwise keep the existing human-readable console output;
ensure you use the existing `specs` array and `SpecInfo` shape and handle the
empty-result path by returning the JSON empty list when `options.json` is set.
---
Nitpick comments:
In
`@openspec/changes/archive/2026-03-13-hierarchical-specs/specs/hierarchical-spec-resolution/spec.md`:
- Around line 44-45: Change the phrase "full path matching" to hyphenated
"full-path matching" in the requirement text under the heading "### Requirement:
Fuzzy matching for hierarchical IDs" so the compound modifier reads "full-path
matching" (keep "leaf-segment matching" as-is) to maintain consistent
compound-adjective style.
In `@src/cli/index.ts`:
- Around line 179-184: The code passes prefix into ListCommand.execute
unconditionally but execute only uses specsPrefix when mode === 'specs', so if a
user supplies a prefix with --changes it is silently ignored; add a user-facing
warning before calling new ListCommand().execute: detect if prefix is truthy and
mode === 'changes' and emit a concise warning (e.g., console.warn or the CLI
logger used in this file) stating the prefix will be ignored in changes mode;
keep the rest of the call intact and reference ListCommand, execute, mode, and
prefix when making the change.
In `@src/commands/spec.ts`:
- Around line 153-175: The discovery code in the .action handler uses
existsSync(specPath) defensively and silently drops missing specs, which can
hide stale/corrupt discovery or concurrent-deletion issues; update the block
that iterates allIds (references: getSpecIds, specIdToPath, SPECS_DIR,
parseSpecFromFile) to emit a warning via the existing logger (or processLogger)
when a spec ID resolves to a non-existent file (include the spec id and resolved
specPath in the message), so missing files are visible for diagnostics while
keeping the current behavior of skipping unparsable files (still catch
parseSpecFromFile errors and log them as parse failures).
In `@src/core/specs-apply.ts`:
- Around line 441-445: In applySpecs replace the manual directory creation and
file write (the block using path.dirname(p.update.target), fs.mkdir(...), and
fs.writeFile(...)) with a call to the existing writeUpdatedSpec function so all
writes and logging stay centralized; locate the code in applySpecs that writes
p.rebuilt to p.update.target and invoke writeUpdatedSpec(p.update.target,
p.rebuilt) (or the matching signature) instead, removing the duplicated
fs.mkdir/fs.writeFile logic and relying on writeUpdatedSpec’s behavior and
logging.
In `@test/specs/hierarchical-specs.integration.test.ts`:
- Around line 101-115: The test overrides console.log (origLog and output)
before calling new ListCommand().execute — wrap the call and assertions in a
try/finally so console.log is always restored: move the await cmd.execute(...)
and subsequent assertions into a try block and restore console.log in a finally
block (keeping origLog, output, and joined logic), referencing the ListCommand
instantiation and execute(testDir, 'specs', { specsPrefix: 'cli/' }) call and
the origLog/console.log override to ensure cleanup even if the test throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c357154-fd50-4692-951c-e84c3ba3b353
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
openspec/AGENTS.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/.openspec.yamlopenspec/changes/archive/2026-03-13-hierarchical-specs/design.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/proposal.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-archive/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-list/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-show/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-spec/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-validate/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/cli-view/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/hierarchical-spec-discovery/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/specs/hierarchical-spec-resolution/spec.mdopenspec/changes/archive/2026-03-13-hierarchical-specs/tasks.mdopenspec/specs/cli/archive/spec.mdopenspec/specs/cli/artifact-workflow/spec.mdopenspec/specs/cli/change/spec.mdopenspec/specs/cli/completion/spec.mdopenspec/specs/cli/config/spec.mdopenspec/specs/cli/feedback/spec.mdopenspec/specs/cli/init/spec.mdopenspec/specs/cli/list/spec.mdopenspec/specs/cli/show/spec.mdopenspec/specs/cli/spec/spec.mdopenspec/specs/cli/update/spec.mdopenspec/specs/cli/validate/spec.mdopenspec/specs/cli/view/spec.mdopenspec/specs/hierarchical-spec-discovery/spec.mdopenspec/specs/hierarchical-spec-resolution/spec.mdsrc/cli/index.tssrc/commands/spec.tssrc/commands/validate.tssrc/core/archive.tssrc/core/list.tssrc/core/specs-apply.tssrc/core/validation/validator.tssrc/core/view.tssrc/utils/item-discovery.tssrc/utils/match.tssrc/utils/spec-paths.tstest/commands/spec.test.tstest/core/archive.test.tstest/specs/hierarchical-specs.integration.test.tstest/utils/item-discovery.test.tstest/utils/match.test.ts
✅ Files skipped from review due to trivial changes (5)
- openspec/specs/cli/validate/spec.md
- openspec/changes/archive/2026-03-13-hierarchical-specs/.openspec.yaml
- openspec/changes/archive/2026-03-13-hierarchical-specs/tasks.md
- openspec/specs/hierarchical-spec-discovery/spec.md
- openspec/AGENTS.md
openspec/specs/cli/archive/spec.md
Outdated
Comment on lines
+77
to
+82
| - **WHEN** archiving a change with delta-based specs | ||
| - **THEN** parse and apply delta changes as defined in openspec-conventions | ||
| - **THEN** recursively discover delta specs within the change's `specs/` directory | ||
| - **AND** parse and apply delta changes as defined in openspec-conventions | ||
| - **AND** validate all operations before applying | ||
| - **AND** create nested directories under `openspec/specs/` as needed for new hierarchical specs | ||
|
|
Contributor
There was a problem hiding this comment.
Clarify per-delta (non-transactional) semantics in this requirement.
Line 80 currently reads like global pre-validation across all deltas before any apply. Please clarify that validation/application is per delta and does not imply global rollback semantics.
✏️ Suggested wording
-- **AND** validate all operations before applying
+- **AND** validate each delta operation before applying that delta
+- **AND** do not imply global rollback across multiple deltas📝 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
| - **WHEN** archiving a change with delta-based specs | |
| - **THEN** parse and apply delta changes as defined in openspec-conventions | |
| - **THEN** recursively discover delta specs within the change's `specs/` directory | |
| - **AND** parse and apply delta changes as defined in openspec-conventions | |
| - **AND** validate all operations before applying | |
| - **AND** create nested directories under `openspec/specs/` as needed for new hierarchical specs | |
| - **WHEN** archiving a change with delta-based specs | |
| - **THEN** recursively discover delta specs within the change's `specs/` directory | |
| - **AND** parse and apply delta changes as defined in openspec-conventions | |
| - **AND** validate each delta operation before applying that delta | |
| - **AND** do not imply global rollback across multiple deltas | |
| - **AND** create nested directories under `openspec/specs/` as needed for new hierarchical specs | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/cli/archive/spec.md` around lines 77 - 82, Update the spec
wording that currently implies global pre-validation ("validate all operations
before applying") to state per-delta, non-transactional semantics: change
phrasing around "recursively discover delta specs" / "parse and apply delta
changes" to specify that each delta is validated and applied independently
(validate each delta before applying it, apply deltas atomically per-delta), and
explicitly note that failures in one delta do not trigger rollback of previously
applied deltas and there is no global transaction across all discovered deltas.
src/core/archive.ts
Outdated
Comment on lines
119
to
131
| const matches = await fg('**/spec.md', { cwd: changeSpecsDir, dot: false, onlyFiles: true }); | ||
| for (const match of matches) { | ||
| try { | ||
| const specId = match.slice(0, -'/spec.md'.length); | ||
| const candidatePath = specIdToPath(specId, changeSpecsDir); | ||
| const content = await fs.readFile(candidatePath, 'utf-8'); | ||
| if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) { | ||
| hasDeltaSpecs = true; | ||
| break; | ||
| } | ||
| } catch {} | ||
| } | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
Don’t silently bypass delta validation when probe reads fail.
With the current empty catches, Line 121-131 can leave hasDeltaSpecs false on I/O/probe errors, and the archive flow then skips validateChangeDeltaSpecs() entirely.
Suggested fix (fail-safe behavior)
- let hasDeltaSpecs = false;
+ let hasDeltaSpecs = false;
try {
const matches = await fg('**/spec.md', { cwd: changeSpecsDir, dot: false, onlyFiles: true });
+ hasDeltaSpecs = matches.length > 0; // fail-safe: any spec delta file should be validated
for (const match of matches) {
- try {
- const specId = match.slice(0, -'/spec.md'.length);
- const candidatePath = specIdToPath(specId, changeSpecsDir);
- const content = await fs.readFile(candidatePath, 'utf-8');
- if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) {
- hasDeltaSpecs = true;
- break;
- }
- } catch {}
+ const specId = match.slice(0, -'/spec.md'.length);
+ const candidatePath = specIdToPath(specId, changeSpecsDir);
+ const content = await fs.readFile(candidatePath, 'utf-8');
+ if (/^##\s+(ADDED|MODIFIED|REMOVED|RENAMED)\s+Requirements/m.test(content)) {
+ break;
+ }
}
- } catch {}
+ } catch (err) {
+ throw err;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/archive.ts` around lines 119 - 131, The empty catches around the
probe logic can hide I/O errors and cause hasDeltaSpecs to remain false,
skipping validateChangeDeltaSpecs(); change the error handling in the outer try
(around fg(...) producing matches) and the inner try (around
fs.readFile(candidatePath, ...)) so that on any error you either rethrow or mark
hasDeltaSpecs = true and break/exit to force validation; specifically update the
blocks that use matches, specId (from specIdToPath) and content so errors are
not silently swallowed—log the error (or rethrow) and set hasDeltaSpecs = true
before breaking/returning so validateChangeDeltaSpecs() runs reliably.
src/core/specs-apply.ts
Outdated
Comment on lines
+73
to
+77
| for (const match of matches) { | ||
| // match = 'cli/show/spec.md' → specId = 'cli/show' | ||
| const specId = match.slice(0, -'/spec.md'.length); | ||
| const specFile = specIdToPath(specId, changeSpecsDir); | ||
| const targetFile = specIdToPath(specId, mainSpecsDir); |
Contributor
There was a problem hiding this comment.
Reject empty spec IDs derived from root spec.md.
If a delta file exists at changes/<name>/specs/spec.md, specId becomes '' (Line 75), and path resolution proceeds with an invalid ID. Fail fast with a clear error before calling specIdToPath.
💡 Proposed fix
for (const match of matches) {
// match = 'cli/show/spec.md' → specId = 'cli/show'
const specId = match.slice(0, -'/spec.md'.length);
+ if (!specId) {
+ throw new Error(
+ `Invalid delta spec path '${match}'. Expected '<spec-id>/spec.md' under openspec/changes/<change>/specs/.`
+ );
+ }
const specFile = specIdToPath(specId, changeSpecsDir);
const targetFile = specIdToPath(specId, mainSpecsDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/specs-apply.ts` around lines 73 - 77, The loop deriving specId from
each match can produce an empty string for root spec files (match like
'spec.md'), so add a guard after computing specId (variable specId in the
for...of over matches) that checks if specId === '' and if so throw or return a
clear error describing the invalid root spec delta (including the original match
and directories changeSpecsDir/mainSpecsDir) before calling specIdToPath; this
ensures you fail fast and avoid passing an empty ID into specIdToPath.
Comment on lines
364
to
379
| private extractNameFromPath(filePath: string): string { | ||
| const normalizedPath = FileSystemUtils.toPosixPath(filePath); | ||
| const parts = normalizedPath.split('/'); | ||
|
|
||
| // Look for the directory name after 'specs' or 'changes' | ||
|
|
||
| // Find the index of 'specs' or 'changes' and return the full relative path | ||
| // from after that segment up to (but not including) the final 'spec.md' file. | ||
| // e.g. 'openspec/specs/cli/show/spec.md' → 'cli/show' | ||
| for (let i = parts.length - 1; i >= 0; i--) { | ||
| if (parts[i] === 'specs' || parts[i] === 'changes') { | ||
| if (i < parts.length - 1) { | ||
| return parts[i + 1]; | ||
| // Collect all parts between the anchor and the last filename | ||
| const tail = parts.slice(i + 1); | ||
| // Drop trailing 'spec.md' if present | ||
| if (tail[tail.length - 1] === 'spec.md') tail.pop(); | ||
| if (tail.length > 0) return tail.join('/'); | ||
| } |
Contributor
There was a problem hiding this comment.
extractNameFromPath now misidentifies change names for proposal.md paths.
At Line 375-378, a change path like openspec/changes/my-change/proposal.md resolves to my-change/proposal.md, which breaks the expected change ID.
Suggested fix
for (let i = parts.length - 1; i >= 0; i--) {
- if (parts[i] === 'specs' || parts[i] === 'changes') {
+ if (parts[i] === 'specs') {
if (i < parts.length - 1) {
const tail = parts.slice(i + 1);
if (tail[tail.length - 1] === 'spec.md') tail.pop();
if (tail.length > 0) return tail.join('/');
}
+ } else if (parts[i] === 'changes') {
+ const tail = parts.slice(i + 1);
+ if (tail.length > 0) return tail[0]; // change ID
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/validation/validator.ts` around lines 364 - 379, The function
extractNameFromPath currently only strips 'spec.md' from the tail, causing paths
like 'openspec/changes/my-change/proposal.md' to resolve to
'my-change/proposal.md'; update the logic where you handle the tail (inside
extractNameFromPath) to check the anchor segment (parts[i]) and drop the
appropriate trailing filename: if the anchor is 'specs' remove a trailing
'spec.md', and if the anchor is 'changes' remove a trailing 'proposal.md' (or
generically drop either 'spec.md' or 'proposal.md' if present), then return the
remaining joined tail so change IDs become just 'my-change'.
src/utils/item-discovery.ts
Outdated
Comment on lines
34
to
43
| const matches = await fg('**/spec.md', { | ||
| cwd: specsPath, | ||
| dot: false, | ||
| onlyFiles: true, | ||
| }); | ||
| // strip trailing "/spec.md" to derive spec ID (e.g. "cli/show/spec.md" → "cli/show") | ||
| return matches | ||
| .map(match => match.slice(0, -'/spec.md'.length)) | ||
| .sort(); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle discovery I/O failures consistently in getSpecIds.
Line 34 can throw, but this function currently only guards fs.access. That turns transient/unreadable-tree cases into hard failures instead of returning [].
Suggested fix
- const matches = await fg('**/spec.md', {
- cwd: specsPath,
- dot: false,
- onlyFiles: true,
- });
+ let matches: string[] = [];
+ try {
+ matches = await fg('**/spec.md', {
+ cwd: specsPath,
+ dot: false,
+ onlyFiles: true,
+ });
+ } catch {
+ return [];
+ }📝 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
| const matches = await fg('**/spec.md', { | |
| cwd: specsPath, | |
| dot: false, | |
| onlyFiles: true, | |
| }); | |
| // strip trailing "/spec.md" to derive spec ID (e.g. "cli/show/spec.md" → "cli/show") | |
| return matches | |
| .map(match => match.slice(0, -'/spec.md'.length)) | |
| .sort(); | |
| } | |
| let matches: string[] = []; | |
| try { | |
| matches = await fg('**/spec.md', { | |
| cwd: specsPath, | |
| dot: false, | |
| onlyFiles: true, | |
| }); | |
| } catch { | |
| return []; | |
| } | |
| // strip trailing "/spec.md" to derive spec ID (e.g. "cli/show/spec.md" → "cli/show") | |
| return matches | |
| .map(match => match.slice(0, -'/spec.md'.length)) | |
| .sort(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/item-discovery.ts` around lines 34 - 43, getSpecIds currently only
guards fs.access but calls fg('**/spec.md', { cwd: specsPath, ... }) which can
throw and cause hard failures; wrap the fast-glob call (the fg invocation that
produces matches from specsPath) in a try/catch (or wrap the body of getSpecIds)
and on any error return an empty array [] so transient/unreadable-tree I/O
errors are handled consistently with the existing fs.access check; locate the fg
call and the matches variable inside getSpecIds to apply this change.
src/utils/spec-paths.ts
Outdated
Comment on lines
+7
to
+10
| export function specIdToPath(specId: string, baseDir: string): string { | ||
| const parts = specId.split('/'); | ||
| return path.join(baseDir, ...parts, 'spec.md'); | ||
| } |
Contributor
There was a problem hiding this comment.
Harden spec ID to path conversion against traversal/alias segments.
Line 8 currently trusts raw ID segments, so IDs containing .././empty parts can resolve outside baseDir or create ambiguous aliases.
Suggested fix
export function specIdToPath(specId: string, baseDir: string): string {
- const parts = specId.split('/');
- return path.join(baseDir, ...parts, 'spec.md');
+ const normalized = specId.replace(/\\/g, '/');
+ const parts = normalized.split('/').filter(Boolean);
+ if (parts.length === 0 || parts.some(p => p === '.' || p === '..')) {
+ throw new Error(`Invalid spec ID: "${specId}"`);
+ }
+
+ const resolved = path.resolve(baseDir, ...parts, 'spec.md');
+ const rel = path.relative(path.resolve(baseDir), resolved);
+ if (rel.startsWith('..') || path.isAbsolute(rel)) {
+ throw new Error(`Spec ID escapes specs directory: "${specId}"`);
+ }
+ return resolved;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/spec-paths.ts` around lines 7 - 10, The specIdToPath function
currently joins raw specId segments into a filesystem path, which allows
traversal via '.'/''/'..' segments; sanitize and validate the segments before
joining: split specId, filter out empty and '.' segments, reject or throw on any
'..' segment (or otherwise escape it), then use path.join(baseDir,
...cleanSegments, 'spec.md') and finally assert the normalized result remains
within baseDir (e.g., compare path.resolve(baseDir) with
path.resolve(result).startsWith) to prevent directory traversal; update
specIdToPath accordingly.
src/utils/spec-paths.ts
Outdated
Comment on lines
+16
to
+24
| export function pathToSpecId(filePath: string, specsDir: string): string { | ||
| const relative = path.relative(specsDir, filePath); | ||
| // relative is e.g. "cli/show/spec.md" on unix or "cli\show\spec.md" on windows | ||
| // strip the trailing "spec.md" component and normalize separators to "/" | ||
| const parts = relative.split(path.sep); | ||
| // remove the last "spec.md" component | ||
| parts.pop(); | ||
| return parts.join('/'); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate filePath shape before converting to a spec ID.
Line 22 always pops the last segment, even when filePath is not a .../spec.md path, which can silently produce wrong IDs.
Suggested fix
export function pathToSpecId(filePath: string, specsDir: string): string {
const relative = path.relative(specsDir, filePath);
- // relative is e.g. "cli/show/spec.md" on unix or "cli\show\spec.md" on windows
- // strip the trailing "spec.md" component and normalize separators to "/"
- const parts = relative.split(path.sep);
- // remove the last "spec.md" component
- parts.pop();
+ const parts = relative.split(/[\\/]/);
+ if (parts[parts.length - 1] !== 'spec.md') {
+ throw new Error(`Expected a spec.md path, got: "${filePath}"`);
+ }
+ parts.pop();
return parts.join('/');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/spec-paths.ts` around lines 16 - 24, The function pathToSpecId
currently unconditionally pops the last path segment which corrupts IDs when
filePath doesn't end with "spec.md"; update pathToSpecId to normalize the
relative path (variable relative), inspect the final segment in parts (or use
path.basename) and only remove it if it equals "spec.md" — otherwise throw a
clear Error (or return the normalized relative path unchanged) so callers know
the input shape is invalid; adjust callers/tests accordingly to handle the
explicit failure.
test/commands/spec.test.ts
Outdated
Comment on lines
+314
to
+327
| it('should not match on partial segment boundary (client/ should not match cli/)', () => { | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| process.chdir(testDir); | ||
| // Create a "client/foo" spec to verify prefix boundary is respected | ||
| fs.mkdir(path.join(specsDir, 'client', 'foo'), { recursive: true }).then(() => | ||
| fs.writeFile(path.join(specsDir, 'client', 'foo', 'spec.md'), `## Purpose\nClient spec.\n\n## Requirements\n\n### Requirement: Foo\nSHALL foo.\n\n#### Scenario: Basic\n- **GIVEN** it\n- **WHEN** run\n- **THEN** ok`) | ||
| ); | ||
| const output = execSync(`node ${openspecBin} spec list cli/`, { encoding: 'utf-8' }); | ||
| expect(output).not.toContain('client/foo'); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Race condition: async directory creation not awaited before running test command.
Lines 319-321 use fs.mkdir().then() without await, so the client/foo spec may not exist when execSync runs on line 322. This makes the test flaky—it might pass by accident when the assertion checks that client/foo is not present, but the test intent (verifying prefix boundary) isn't reliably exercised.
🐛 Proposed fix
it('should not match on partial segment boundary (client/ should not match cli/)', () => {
const originalCwd = process.cwd();
try {
process.chdir(testDir);
// Create a "client/foo" spec to verify prefix boundary is respected
- fs.mkdir(path.join(specsDir, 'client', 'foo'), { recursive: true }).then(() =>
- fs.writeFile(path.join(specsDir, 'client', 'foo', 'spec.md'), `## Purpose\nClient spec.\n\n## Requirements\n\n### Requirement: Foo\nSHALL foo.\n\n#### Scenario: Basic\n- **GIVEN** it\n- **WHEN** run\n- **THEN** ok`)
- );
+ const clientDir = path.join(specsDir, 'client', 'foo');
+ require('fs').mkdirSync(clientDir, { recursive: true });
+ require('fs').writeFileSync(path.join(clientDir, 'spec.md'), `## Purpose\nClient spec.\n\n## Requirements\n\n### Requirement: Foo\nSHALL foo.\n\n#### Scenario: Basic\n- **GIVEN** it\n- **WHEN** run\n- **THEN** ok`);
const output = execSync(`node ${openspecBin} spec list cli/`, { encoding: 'utf-8' });
expect(output).not.toContain('client/foo');
} finally {Alternatively, mark the test as async and await the promises.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/spec.test.ts` around lines 314 - 327, The test "should not
match on partial segment boundary (client/ should not match cli/)" has a race
where fs.mkdir(...).then(...) and fs.writeFile are not awaited before running
execSync; make the test reliably synchronous by marking the test callback async
and awaiting the directory creation and file write (await fs.promises.mkdir and
await fs.promises.writeFile or await the promise chain) prior to calling
execSync(`node ${openspecBin} spec list cli/`); ensure you reference the same
identifiers used here (the test name string, specsDir, openspecBin) so the spec
file exists before the execSync assertion runs.
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
1 participant
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.
Hierarchical Specs
This change was proposed as an OpenSpec change — see openspec/changes/hierarchical-specs/ for the full proposal, design decisions, and task breakdown.
Problem
OpenSpec stores all specs flat under openspec/specs/. As projects grow, teams encode hierarchy in flat names (cli-show, cli-validate, schema-fork-command). With 38+ specs already, this is noisy, collision-prone, and hard to navigate.
What This Changes
Spec IDs become path-based, supporting arbitrary nesting depth:
Before
openspec/specs/cli-show/spec.md → ID: cli-show
After
openspec/specs/cli/show/spec.md → ID: cli/show
Backward Compatibility
No migration required. Flat and hierarchical specs coexist — cli-show and cli/show are distinct spec IDs. Existing changes and archives continue to work unchanged.
Scope
Generated with Claude Code using claude-opus-4-6
Summary by CodeRabbit
Release Notes
New Features
cli/show).openspec spec list cli/) to view only related specs.Documentation