feat: add hierarchical spec structure support#660
feat: add hierarchical spec structure support#660lsmonki wants to merge 54 commits intoFission-AI:mainfrom
Conversation
Add support for organizing specifications in nested directory hierarchies alongside the existing flat structure. This enables better organization for complex projects while maintaining full backward compatibility. Features: - Recursive spec discovery with findAllSpecs() utility - Auto-detection of flat vs hierarchical structures - Structure validation with configurable depth limits and naming conventions - 1:1 delta replication (change deltas mirror main spec structure) - Cross-platform path handling (Windows, macOS, Linux) - Updated all commands: list, view, validate, show, archive - Updated skill templates and workflow schema with hierarchical examples - Comprehensive documentation and migration guide - Example project with hierarchical structure Implementation notes: - Fixed getSpecIds() to use recursive discovery - Updated list display to show full paths with uniform padding - Updated workflow schema instructions to use CLI commands
📝 WalkthroughWalkthroughAdds hierarchical spec support: new spec-discovery utility (findAllSpecs, findSpecUpdates, validateSpecStructure), specStructure config with project-level overrides, capability-path semantics across CLI/core (list/view/validate/archive/apply), extensive docs/examples/migration/troubleshooting, many tests, and a .gitignore entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (list/validate/archive)
participant Disc as SpecDiscovery\n(findAllSpecs / findSpecUpdates)
participant FS as Filesystem
participant Val as Validator\n(validateSpecStructure)
CLI->>Disc: request discovered specs or spec updates (baseDir / changeDir)
Disc->>FS: traverse directories & read `spec.md` files
FS-->>Disc: file paths & contents
Disc-->>CLI: DiscoveredSpec[] / SpecUpdate[]
CLI->>Val: validateSpecStructure(DiscoveredSpec[], config)
Val-->>CLI: ValidationIssue[] (may affect exit code)
CLI->>FS: read/write spec files when applying changes (using capability paths)
FS-->>CLI: success / failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/templates/skill-templates.ts (1)
1224-1256:⚠️ Potential issue | 🟡 MinorKeep the mkdir example aligned with
<capability-path>.The save path now uses
<capability-path>, but the mkdir example still uses<capability-name>, which won’t create nested directories for hierarchical specs.✏️ Doc fix
-# Unix/macOS -mkdir -p openspec/changes/<name>/specs/<capability-name> -# Windows (PowerShell) -# New-Item -ItemType Directory -Force -Path "openspec/changes/<name>/specs/<capability-name>" +# Unix/macOS +mkdir -p openspec/changes/<name>/specs/<capability-path> +# Windows (PowerShell) +# New-Item -ItemType Directory -Force -Path "openspec/changes/<name>/specs/<capability-path>"src/commands/validate.ts (1)
118-129:⚠️ Potential issue | 🟠 MajorNormalize spec IDs for Windows compatibility when comparing user input.
getSpecCapabilities()returns spec IDs with platform-specific path separators (backslashes on Windows, forward slashes on Unix). When hierarchical specs are enabled (default withspecStructure.structure: 'auto'), the direct comparison at line 120 fails on Windows if users input paths with forward slashes—e.g.,openspec validate platform/services/apiwon't match the discovered IDplatform\services\api.The codebase already has a
toPosixPath()utility insrc/utils/file-system.ts. Consider normalizing both itemName and specs to forward slashes before comparison, and apply the same normalization to the candidates passed tonearestMatches()to ensure suggestions work correctly.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 3-28: Consolidate duplicate "Unreleased" sections by merging this
hierarchical-specs entry under the single top-level "Unreleased" header (remove
any other duplicate "Unreleased" headers), and update the performance statement:
either remove the "< 100ms" claim or replace it with a verifiable phrasing and
reference to a benchmark (e.g., "Handles 1000+ specs efficiently — see benchmark
XYZ"); ensure cross-references like the `findAllSpecs()` utility and
`specStructure` config remain intact and docs links are preserved.
In `@docs/migration-flat-to-hierarchical.md`:
- Around line 31-37: Several fenced code blocks (for example the block
containing the directory list starting with "_global/ # Cross-cutting
concerns" and the block starting with "openspec/specs/" as well as the other
unlabeled lists later in the document) are missing language identifiers; update
each unlabeled triple-backtick fence to include a language tag (use "text" for
plain directory/list blocks) so each fence becomes ```text ... ```; apply this
change to all unlabeled fences referenced by the reviewer (the examples around
the directory listings and the other unlabeled listing blocks) to satisfy MD040.
In `@docs/troubleshooting-hierarchical-specs.md`:
- Around line 31-33: Several fenced code blocks lack language tags (markdownlint
MD040); update each unlabeled triple-backtick fence containing error
messages—e.g., the blocks with the lines 'ERROR: Invalid segment "Auth" in
capability "platform/Auth"', 'ERROR: Spec "platform/services/api/rest/v1"
exceeds maximum depth 4', and the other error-message blocks referenced—to
include a language identifier such as text (change ``` to ```text) so
markdownlint no longer flags them; apply the same change to all unlabeled fences
listed in the comment.
In `@src/core/validation/validator.ts`:
- Around line 134-136: The code constructs entryPath by concatenating
spec.capability with "/spec.md" which mixes separators on Windows; update the
logic where entryPath is created (referencing parseDeltaSpec and
spec.capability) to use a platform-safe join (e.g., path.join or
FileSystemUtils.join) so the resulting path is consistent across OSes, and
ensure any callers expecting a POSIX-style path are adjusted or normalized as
needed.
In `@src/utils/spec-discovery.ts`:
- Around line 114-123: The capability calculation incorrectly sets capability to
'spec.md' when spec.md is in the base directory because relativePath is empty;
update the logic in src/utils/spec-discovery.ts where you handle entry.isFile()
and entry.name === 'spec.md' to explicitly skip spec.md at the root (i.e., if
relativePath is falsy or equals '' then continue/return early) instead of
pushing a malformed record into specs; ensure you reference the existing
variables capability, relativePath, entry, fullPath, and depth so the push only
happens for valid capability directories.
- Around line 30-39: Remove the duplicate SpecUpdate interface declaration from
specs-apply.ts and rely on the exported SpecUpdate from spec-discovery.ts (which
already has JSDoc). In specs-apply.ts, keep the existing import "type SpecUpdate
as SpecUpdateUtil" and remove the local interface block; if you need to expose
it from specs-apply.ts, re-export the imported type (e.g., export type
SpecUpdateUtil = SpecUpdate) rather than redefining it so SpecUpdate in
spec-discovery.ts remains the single source of truth.
In `@test/utils/spec-discovery.test.ts`:
- Around line 177-185: Rename the test case to reflect actual behavior: change
the 'it' description from "should not find orphaned specs at intermediate
levels" to something like "should find all specs including intermediate spec;
validation of orphaned specs happens separately", and update the inline comment
above the expect to explain that findAllSpecs(hierarchicalFixtureDir)
intentionally returns the intermediate spec (expect(specs).toHaveLength(6)) and
that orphaned-spec validation is handled in a separate step; locate this test
block in test/utils/spec-discovery.test.ts (the it(...) containing the
fs.writeFileSync(... '_global' 'spec.md') and the call to findAllSpecs) and only
adjust the test name and comment text.
🧹 Nitpick comments (9)
examples/hierarchical-specs/README.md (1)
7-20: Add language specifiers to fenced code blocks.The directory structure code blocks are missing language specifiers. While this doesn't affect rendering, it triggers markdown linting warnings (MD040).
📝 Proposed fix
-``` +```text openspec/specs/ _global/ testing/spec.md - Global testing standardsAnd similarly for the block at line 53:
-``` +```text openspec/changes/add-rate-limiting/ proposal.md specs/Also applies to: 53-60
docs/organizing-specs.md (1)
9-16: Add language specifiers to directory structure code blocks.Multiple fenced code blocks showing directory structures are missing language specifiers (MD040). Use
textorplaintextfor consistency with markdown linting rules.📝 Proposed fix for affected blocks
Change opening fence from triple backticks to:
-``` +```textApply to code blocks at lines 9, 27, 71, 77, and 88.
Also applies to: 27-46, 71-74, 77-80, 88-98
src/core/specs-apply.ts (1)
322-323: Potential cross-platform path display inconsistency.The log message uses a forward slash (
/) to construct the display path, butupdate.capabilitymay contain platform-specific separators (path.sep). On Windows, this could result in mixed separators likeopenspec/specs/_global\security/spec.md.Consider normalizing capability to use forward slashes for display purposes:
🔧 Proposed fix for consistent display paths
// Use full capability path for hierarchical support - console.log(`Applying changes to openspec/specs/${update.capability}/spec.md:`); + const displayPath = update.capability.replace(/\\/g, '/'); + console.log(`Applying changes to openspec/specs/${displayPath}/spec.md:`);src/core/view.ts (1)
248-255: Consider aligning hierarchical indentation todepth - 1.Depth 1 specs get an extra indent, which makes roots appear more indented than flat view. A small tweak keeps root items aligned.
💡 Optional indentation tweak
- const indent = ' '.repeat(node.depth); + const indent = ' '.repeat(Math.max(0, node.depth - 1));src/core/global-config.ts (1)
160-170: Apply deep-merge forspecStructureto ensure defaults are preserved for partial configs.Currently,
getGlobalConfig()uses shallow merge, which means if a user's config file contains only{ "specStructure": { "maxDepth": 8 } }, the returnedspecStructurewill have onlymaxDepthand lose the other defaults. WhilegetSpecStructureConfig()currently mitigates this with defensive fallbacks, direct accesses togetGlobalConfig().specStructure(whether now or in the future) would receive incomplete values.Apply the same deep-merge pattern used for
featureFlags:Deep-merge specStructure in getGlobalConfig()
return { ...DEFAULT_CONFIG, ...parsed, // Deep merge featureFlags featureFlags: { ...DEFAULT_CONFIG.featureFlags, ...(parsed.featureFlags || {}) - } + }, + // Deep merge specStructure to preserve defaults for partial configs + specStructure: { + ...DEFAULT_CONFIG.specStructure, + ...(parsed.specStructure || {}) + } };src/core/config-schema.ts (2)
21-21: Minor: Extra leading space in comment.There's a leading space before the
/**that breaks the alignment with other doc comments in the file.Suggested fix
- /** +/**
262-264: Minor: Redundant type assertion.The
as z.ZodErrorcast on line 263 is unnecessary since theinstanceof z.ZodErrorcheck on line 262 already narrows the type.Suggested fix
if (error instanceof z.ZodError) { - const zodError = error as z.ZodError; - const messages = zodError.issues.map((e) => `${e.path.join('.')}: ${e.message}`); + const messages = error.issues.map((e) => `${e.path.join('.')}: ${e.message}`); return { success: false, error: messages.join('; ') }; }openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md (1)
189-192: Add language specifiers to fenced code blocks.The code blocks at lines 189-192 and 214-224 are missing language specifiers. Even for display format examples, adding a specifier (e.g.,
textorplaintext) improves consistency and helps rendering tools.Suggested fix for line 189
-``` +```text ✗ specs/auth/spec.md ← Has spec at intermediate level specs/auth/oauth/spec.md ← Also has spec in child</details> <details> <summary>Suggested fix for line 214</summary> ```diff -``` +```text Specifications: _global/ architecture 42 requirementsAlso applies to: 214-224
src/utils/spec-discovery.ts (1)
294-294: Minor:.gitignorein RESERVED_NAMES is unnecessary.The
RESERVED_NAMESlist includes.gitignore, which is typically a file, not a directory. Since capability paths represent directory names, this entry is unlikely to match anything. Consider removing it or replacing with more relevant directory-specific reserved names if needed.Suggested fix
- const RESERVED_NAMES = ['..', '.', '.git', '.gitignore', 'node_modules', '.openspec']; + const RESERVED_NAMES = ['..', '.', '.git', 'node_modules', '.openspec'];
|
Can we align on the proposal first before diving into implementation? This is a relatively large change. From the contributing section:
|
Sure, no problem at all 👍 |
…ection in archive findAllSpecs() produced invalid capability "spec.md" for spec.md files at baseDir root. Archive delta detection only checked one directory level deep, missing hierarchical deltas at depth 2+.
Replace string interpolation with path.join() in validator, specs-apply, and spec-discovery to avoid mixed separators on Windows.
Remove duplicated SpecUpdate interface from specs-apply.ts, re-export from spec-discovery.ts. Rename Spec to DiscoveredSpec to avoid collision with the schema Spec type.
…play Replace inline path.sep checks with the shared utility function in list.ts and view.ts. Fix view.ts to sort by path when hierarchical and show full capability paths instead of leaf names only.
… reports specStructure was shallow-merged, losing default values when only some fields were set. Orphan detection could report the same parent multiple times when several children shared the same orphaned ancestor.
Support specStructure in openspec/config.yaml with field-level merge precedence (project > global > defaults). Uses sub-field-by-field resilient parsing consistent with rules parsing pattern.
validateSpecStructure() was ignoring config.structure and config.allowMixed fields. Flat mode now rejects depth > 1, hierarchical mode rejects depth === 1, and auto mode with allowMixed: false detects mixed flat/hierarchical specs.
…rsing SpecStructureConfig already has all fields optional, making Partial<SpecStructureConfig> identical. Remove the unnecessary Partial wrapper and the corresponding `as` cast.
The wrapper delegates to a sync function from spec-discovery. Remove async/await from the wrapper and its two call sites since the operation is entirely synchronous.
isSpecStructureHierarchical now accepts a pre-discovered DiscoveredSpec[] to avoid re-traversing the filesystem. list.ts and view.ts pass already-discovered specs instead of triggering a redundant directory walk.
runBulkValidation called findAllSpecs twice: once via getSpecCapabilities() and again for structure validation. Now discovers specs once and derives both the capability list and structure validation from the same result.
getGlobalConfig() was deep-merging raw JSON specStructure without validation, allowing invalid values like structure:"invalid" to pass through silently. Add sub-field Zod validation consistent with project-config parsing.
The _structure pseudo-item inflated item counts and was inconsistent with real spec items. Structure validation issues are now in a dedicated structureValidation field in JSON output and displayed separately in human output.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/specs-apply.ts (1)
157-162:⚠️ Potential issue | 🟡 MinorUse full capability in the “no operations” error.
The current message only reports the leaf directory, which is ambiguous for hierarchical specs with repeated leaf names.Proposed fix
- `Delta parsing found no operations for ${path.basename(path.dirname(update.source))}. ` + + `Delta parsing found no operations for ${update.capability}. ` +
🧹 Nitpick comments (7)
openspec/changes/archive/2026-02-04-project-level-spec-structure/specs/global-config/spec.md (1)
8-10: Add language specifier to fenced code block.The static analysis tool flagged this code block for missing a language specifier. Since this shows a text-based precedence chain, consider adding a language identifier.
Proposed fix
-``` +```text project override → global config → default</details> </blockquote></details> <details> <summary>openspec/specs/global-config/spec.md (1)</summary><blockquote> `107-109`: **Add language specifier to fenced code block.** Same as the archived change spec, this code block needs a language identifier. <details> <summary>Proposed fix</summary> ```diff -``` +```text project override → global config → default</details> </blockquote></details> <details> <summary>openspec/changes/archive/2026-02-04-project-level-spec-structure/design.md (2)</summary><blockquote> `26-31`: **Add language specifier to YAML example.** The static analysis tool flagged this code block. Since it shows YAML configuration, add the `yaml` language identifier. <details> <summary>Proposed fix</summary> ```diff -``` +```yaml specStructure: structure: flat ← valid, kept maxDepth: "very deep" ← invalid, warned + skipped validatePaths: false ← valid, kept</details> --- `41-44`: **Add language specifier to pseudo-code block.** This code block also needs a language identifier. Consider `text` or `typescript` depending on the intent. <details> <summary>Proposed fix</summary> ```diff -``` +```text getSpecStructureConfig(projectOverrides?) → project value ?? global value ?? default</details> </blockquote></details> <details> <summary>test/core/project-config.test.ts (1)</summary><blockquote> `672-689`: **Consider adding upper boundary test for maxDepth.** The test validates `maxDepth: 0` is rejected. Consider adding a test for `maxDepth: 11` (above max of 10) to ensure both boundaries are enforced. <details> <summary>📝 Suggested additional test case</summary> ```typescript it('should reject maxDepth above valid range', () => { const configDir = path.join(tempDir, 'openspec'); fs.mkdirSync(configDir, { recursive: true }); fs.writeFileSync( path.join(configDir, 'config.yaml'), `schema: spec-driven specStructure: maxDepth: 11 ` ); const config = readProjectConfig(tempDir); expect(config?.specStructure).toBeUndefined(); expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining("Invalid 'specStructure.maxDepth'") ); });src/core/project-config.ts (1)
167-223: Consider extracting shared specStructure validation logic.The sub-field parsing logic here is nearly identical to the validation in
src/core/global-config.ts(lines 112-132). While acceptable for now, consider extracting a shared helper to reduce duplication if this pattern expands.src/commands/validate.ts (1)
13-21: Unnecessary async wrapper around synchronous code.
findAllSpecsis synchronous (usesfs.readdirSync), so wrapping it in an async function adds unnecessary overhead. Consider making this synchronous.♻️ Suggested refactor
-/** - * Get all spec capabilities using recursive spec discovery. - * Supports both flat and hierarchical spec structures. - */ -async function getSpecCapabilities(): Promise<string[]> { - const specsDir = path.join(process.cwd(), 'openspec', 'specs'); - const discovered = findAllSpecs(specsDir); - return discovered.map(spec => spec.capability).sort(); -} +/** + * Get all spec capabilities using recursive spec discovery. + * Supports both flat and hierarchical spec structures. + */ +function getSpecCapabilities(): string[] { + const specsDir = path.join(process.cwd(), 'openspec', 'specs'); + const discovered = findAllSpecs(specsDir); + return discovered.map(spec => spec.capability).sort(); +}Then update call sites at lines 96 and 119:
-const [changes, specs] = await Promise.all([getActiveChangeIds(), getSpecCapabilities()]); +const [changes, specs] = [await getActiveChangeIds(), getSpecCapabilities()];
Hierarchical specs use paths like _global/testing, not flat names.
CLI input may use / or \ regardless of platform. Normalize to native path.sep so item names match discovered spec/change IDs on any OS.
…ssage path.basename loses parent segments for hierarchical specs like _global/testing, showing only "testing" instead of the full path.
Root-level Zod errors have an empty path array, producing ": <msg>" instead of a useful prefix. Use <root> as fallback label.
The Zod schema allows maxDepth up to 10 but the hard limit silently capped it at 6, ignoring user configuration for values 7-10.
readdirSync order is OS-dependent. Sorting entries ensures consistent discovery order regardless of filesystem.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`:
- Around line 137-181: The auto-detection in isSpecStructureHierarchical is too
brittle: update isSpecStructureHierarchical (and its callers like
getEffectiveStructure) to handle empty/nonexistent specsDir by returning flat
when no specs, compute hierarchicalCount via findAllSpecs(specsDir) and require
a threshold (e.g., hierarchicalCount/total > 0.20 and hierarchicalCount > 0) to
declare hierarchical to avoid single-directory false positives, and add a
consistency check that flags mixed-depth projects (use config.allowMixed to let
callers accept mixed structures or else surface a warning/error) so detection
isn't triggered by accidental nesting.
- Around line 96-136: Update the design to explicitly define sync semantics:
extend findSpecUpdates to represent deletions (e.g., tombstone files or explicit
delete entries) and state whether absence means "no change" or "delete"; specify
applySpecs behavior to create parent directories for new specs and optionally
remove empty dirs after deletions; define atomicity/transaction model
(all-or-nothing using a staging directory + atomic swap or documented
partial-sync behavior with rollback/retry rules) and explain error handling when
one delta fails; add conflict-resolution rules for applySpecs/findSpecUpdates
(detect concurrent changes via checksums/mtimes or require version stamps, and
surface warnings or reject conflicting deltas); add these operational rules into
this design doc or a new "Sync Behavior" decision and reference the functions
findSpecUpdates and applySpecs where you describe the required changes.
- Around line 183-208: Update the "Naming Conventions" rule to specify that the
regex `/^[a-z0-9-_]+$/` applies to each individual path segment (directory name)
rather than the full capability path, and explicitly forbid leading/trailing
separators; extend that rule to include Windows reserved names (CON, PRN, AUX,
NUL, COM1–COM9, LPT1–LPT9) as disallowed segment values; add a new
"Cross-platform path safety" validation that (a) detects case-only collisions
(e.g., `Auth` vs `auth`) and treats them as ERROR, (b) warns when the full path
length exceeds 200 characters (configurable threshold) to protect Windows
260-char limits, and (c) enforces per-segment checks for reserved names and the
segment regex; mention these changes alongside the existing "Depth Limits" and
"No Orphaned Specs" decisions so reviewers can update implementation/validators
accordingly.
🧹 Nitpick comments (3)
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md (3)
338-360: Consider adding concurrent modification risk.The risks and trade-offs are well-documented with reasonable mitigations. Consider adding one more:
Risk: Concurrent modifications during multi-spec operations (e.g., user edits spec while
openspec syncis running)
→ Mitigation: Document expected behavior (last-write-wins vs error) or implement file locking for atomic operations.
361-386: Consider providing migration automation.The migration plan is solid with comprehensive testing and gradual rollout. Two suggestions:
Migration tool - Manual reorganization (line 366) is error-prone for large projects. Consider offering a CLI command like
openspec migrate-to-hierarchicalthat validates and refactors the structure automatically.Rollback plan - "No rollback needed" (line 384) is optimistic. If a critical bug affects sync operations, users can't easily revert. Consider documenting a rollback procedure (e.g., git revert, config override, or version downgrade steps).
406-435: Excellent post-implementation documentation.Documenting discovered issues and their fixes is valuable for future reference. These real-world findings validate the importance of end-to-end testing.
Suggestion: Consider adding a "Pre-implementation Checklist" section to help prevent similar integration gaps in future features:
- Audit all spec-reading code paths for shallow vs recursive assumptions
- Verify all display code handles variable-length identifiers
- Check all documentation/schema for hardcoded structure assumptions
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Show resolved
Hide resolved
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Show resolved
Hide resolved
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Show resolved
Hide resolved
Add CON, PRN, AUX, NUL, COM1-9, LPT1-9 as disallowed segment names. These cause silent failures on Windows filesystems.
…portability Dual check: warn when full path exceeds 260 chars (Windows MAX_PATH) or when capability path exceeds 160 chars (portability risk).
lsmonki
force-pushed
the
feature/hierarchical-specs-support
branch
from
ffef439 to
a97d7b5
Compare
The inner catch in delta spec detection was silently swallowing read errors. If a spec.md file exists but cannot be read (permissions, encoding), it was skipped without notice, potentially allowing a broken change to be archived without validation.
The sync and archive skill templates used specs/*/spec.md which only matches one level deep, missing hierarchical deltas like specs/backend/services/api/spec.md.
The test name said "maxDepth of 0" but the config and assertion both use maxDepth 1.
ViewCommand displays hierarchical specs in discovery order grouped by domain, not sorted by requirement count. The test incorrectly asserted descending sort order.
…for cross-platform consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/utils/spec-discovery.test.ts`:
- Around line 709-712: The test's reservedNames array contains entries for
capability '..' and '.' whose path fields use path.join which normalizes those
segments; update the path values for the two entries with capability '..' and
'.' to literal/concatenated strings (e.g. '/specs/../spec.md' and
'/specs/./spec.md') or otherwise build them without normalization so the path
fields remain as-written; modify the reservedNames array in
spec-discovery.test.ts (the entries for capability '..' and capability '.')
accordingly.
🧹 Nitpick comments (3)
test/utils/spec-discovery.test.ts (3)
2-2: Unused import:isSpecStructureHierarchical.This symbol is imported but never referenced in any test case. Either add test coverage for it or remove the import.
♻️ Proposed fix
-import { findAllSpecs, isSpecStructureHierarchical, findSpecUpdates, validateSpecStructure, type DiscoveredSpec } from '../../src/utils/spec-discovery.js'; +import { findAllSpecs, findSpecUpdates, validateSpecStructure, type DiscoveredSpec } from '../../src/utils/spec-discovery.js';Based on learnings,
isSpecStructureHierarchical()is intentionally binary and used only for display formatting — it may still deserve at least a basic test to verify the threshold behavior.
60-69: Inline cleanup risks leaking the fixture on assertion failure.If
expecton line 66 throws,fs.rmSyncon line 68 never executes. Consider using a try/finally or moving this into the sharedbeforeEach/afterEachlifecycle.
853-864: Duplicate tests for defaultmaxDepth."should respect config defaults" (line 853) and "should use default maxDepth of 4 when not specified" (line 874) are nearly identical — both pass
{}and assert the same depth error message. Consider removing one.Also applies to: 874-884
The pseudocode used 'relativePath || "."' which implied root-level spec.md is a valid capability. The actual implementation skips it with an early continue. Updated snippet to match.
The regex /^[a-z0-9-_]+$/ validates each individual directory segment, not the full capability path. Added example to prevent confusion.
Filesystem code uses path.join()/path.sep, while docs, prompts, and display output use forward slashes for consistency.
…ision with implementation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`:
- Line 39: The design doc contains a contradiction: the non-goal statement about
"Mixed flat/hierarchical specs" (the sentence at line 39) says mixed specs are
not allowed unless opted-in, but the option declaration allowMixed?: boolean has
a comment saying "// default: true" (line 161). Fix by making the behavior
consistent—either update the non-goal sentence to say mixed flat/hierarchical
specs are allowed by default, or change the allowMixed?: boolean comment to "//
default: false" to require explicit opt-in; reference the non-goal sentence and
the allowMixed? boolean declaration when applying the change so both statements
match.
- Around line 391-398: Document the implicit decisions: add a section stating
that when allowMixed: true the system silently permits mixed flat and
hierarchical specs (no warnings/errors are emitted), and that when allowMixed:
false a mixed layout errors; also clarify the spec creation workflow by noting
there is no dedicated openspec new <spec> command and that spec creation is
performed via the change/delta mechanism (e.g., openspec new change) or by
manual directory configuration under specs/, and explain how users choose flat
vs hierarchical structure (project config or manual directory layout).
🧹 Nitpick comments (1)
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md (1)
408-437: Consider clarifying the timeline context of "Post-Implementation Fixes".This section documents fixes for issues discovered during testing, which is valuable. However, since the PR objectives note this is a formal proposal requested by the reviewer, the title "Post-Implementation Fixes" may confuse readers about whether this is a proposal or retrospective documentation.
Consider one of these alternatives:
- Retitle to clarify timing: "## Implementation Notes: Issues Discovered During Development"
- Add context: "## Post-Implementation Fixes (Documented Retroactively)\n\nSince implementation preceded this formal proposal, these are real issues discovered during development that inform the design:"
- Move to a separate "Lessons Learned" section if you want to keep the design decisions separate from implementation experience
This is a documentation structure suggestion - the content itself is valuable and well-documented.
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Outdated
Show resolved
Hide resolved
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Outdated
Show resolved
Hide resolved
2 participants
OpenSpec currently assumes a flat spec structure (
specs/{capability}/spec.md) where each capability is a direct subdirectory underspecs/. This works for simple projects but becomes limiting as projects grow. Teams need hierarchical organization for:specs/payments/checkout/spec.md,specs/auth/oauth/spec.md)specs/packages/core/spec.md)specs/_global/testing/spec.mdvsspecs/features/export/spec.md)specs/team-platform/infra/spec.md)Commands like
openspec list --specs, validation, sync, and archive fail to discover or work with hierarchically organized specs, forcing teams to use workarounds or manual processes.What Changes
spec.mdfiles at any depth, replacing the current single-level directory scanspecs/(e.g.,_global/testinginstead of justtesting)specStructuresection inopenspec/config.yamlor globally in~/.config/openspec/config.jsonto control structure mode, depth limits, and validation behaviorstructure:'flat'|'hierarchical'|'auto'(default:auto- auto-detect)maxDepth: Maximum hierarchy depth (default:4, recommended:2-3)allowMixed: Allow mixing flat and hierarchical specs (default:true)validatePaths: Enforce naming conventions (default:true)listandviewcommands to show hierarchical specslist,validate,sync,archivecommands to work with hierarchical pathsBenefits:
_global/,shared/,utils/) from feature-specific specsExamples
Directory structures
Flat (current behavior):
Hierarchical (new):
Mixed (flat + hierarchical coexist):
Change deltas mirror main structure 1:1
CLI works with full paths
Output example (
openspec list --specs)Configuration (optional, works without it)
In
openspec/config.yaml:Or globally in
~/.config/openspec/config.json:{ "specStructure": { "structure": "auto", "maxDepth": 4, "allowMixed": true, "validatePaths": true } }References: #662 #581
Summary by CodeRabbit
New Features
Validation / CLI
Documentation
Examples
Tests
Chores