feat: add hierarchical spec structure support #660
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
📝 WalkthroughWalkthroughCentralized hierarchical spec discovery and validation added: new spec-discovery utilities, specStructure config (global + project overrides), commands/parsers/archive/templates switched to capability-path identifiers, extensive docs/examples/migration guides, and large test suites for hierarchical behavior and performance. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (list / validate / archive)
participant Disc as SpecDiscovery (findAllSpecs / findSpecUpdates)
participant FS as Filesystem
participant Validator as Validator (validateSpecStructure)
Note over CLI,Disc: High-level flow for discovery, validation, and apply using capability-paths
CLI->>Disc: request discovered specs or spec updates (baseDir / changeDir)
Disc->>FS: traverse directories & read `spec.md` files
FS-->>Disc: spec paths & contents
Disc-->>CLI: DiscoveredSpec[] / SpecUpdate[]
CLI->>Validator: validateSpecStructure(DiscoveredSpec[], config)
Validator-->>CLI: ValidationIssue[] (may change exit code)
CLI->>FS: read/write spec files when applying changes (using capability paths)
FS-->>CLI: success / failure
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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'];
CHANGELOG.md
Outdated
| ## Unreleased | ||
|
|
||
| ### Minor Changes | ||
|
|
||
| - **Hierarchical spec structures** — OpenSpec now supports organizing specifications in nested directory hierarchies (e.g., `_global/testing/`, `platform/services/api/`) alongside the traditional flat structure. Auto-detects structure, maintains full backward compatibility, and includes comprehensive migration guide. | ||
|
|
||
| **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) | ||
| - Configuration via `specStructure` in global config | ||
|
|
||
| **Updated commands:** | ||
| - `list`, `validate`, `sync`, `archive` - all support hierarchical paths | ||
| - Change parser and validator use recursive discovery | ||
|
|
||
| **Documentation:** | ||
| - [Organizing Specs Guide](docs/organizing-specs.md) | ||
| - [Migration Guide](docs/migration-flat-to-hierarchical.md) | ||
| - [Troubleshooting](docs/troubleshooting-hierarchical-specs.md) | ||
| - [Example project](examples/hierarchical-specs/) | ||
|
|
||
| **Performance:** Handles 1000+ specs efficiently (< 100ms) | ||
|
|
There was a problem hiding this comment.
Avoid duplicate “Unreleased” sections and verify the perf claim.
There are other “Unreleased” headers later in this file, which can confuse release notes. Also, the “< 100ms” claim should be backed by a measurable benchmark or removed to avoid misleading users.
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 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.
|
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.
.gitignore is a file, not a directory, so it can never match a capability path segment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/config-schema.ts (1)
257-265:⚠️ Potential issue | 🟡 MinorAvoid empty-path prefixes in validation errors.
For root-level Zod issues,e.path.join('.')yields an empty string, so messages become": <msg>". Consider a root label fallback.🔧 Suggested fix
- const messages = error.issues.map((e) => `${e.path.join('.')}: ${e.message}`); + const messages = error.issues.map((e) => { + const path = e.path.length ? e.path.join('.') : '<root>'; + return `${path}: ${e.message}`; + });
🤖 Fix all issues with AI agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`:
- Around line 358-360: The sentence about the performance trade-off is missing a
subject; update the fragment "→ **Accepted:** Detection is ~1ms by checking
first few specs. Negligible compared to I/O. Can be overridden with config." to
include an explicit subject (e.g., "The detection" or "This detection") so it
reads cleanly, for example: "→ **Accepted:** The detection adds ~1ms by checking
the first few specs; this is negligible compared to I/O and can be overridden
with config." Apply this change to the sentence that begins with "Auto-detection
adds small overhead to every command" / the subsequent "→ **Accepted:**
Detection..." text.
- Around line 402-404: Update the heading text "4. **Performance optimization
for very large repos (1000+ specs)?**" to remove the vague word "very" and use a
precise metric-driven phrase (for example: "4. **Performance optimization for
large repositories (≥1000 specs)?**" or "4. **Performance optimization for
repositories with 1000+ specs?**") so the intent is clearer; edit the two bullet
points under that heading accordingly to match the tightened phrasing.
In `@src/utils/spec-discovery.ts`:
- Around line 310-323: The validation hard-cap of 6 (HARD_LIMIT_DEPTH) conflicts
with the config/schema allowing maxDepth up to 10, causing valid configs (7–10)
to be rejected; update the hard cap to match the schema by raising
HARD_LIMIT_DEPTH from 6 to 10 (and adjust or remove RECOMMENDED_MAX_DEPTH if
needed) so that effectiveMax = Math.min(maxDepth, HARD_LIMIT_DEPTH) uses the
same upper bound as the config; ensure the change is applied where
RECOMMENDED_MAX_DEPTH, HARD_LIMIT_DEPTH, maxDepth, effectiveMax, and the
specs/spec.depth check are used.
🧹 Nitpick comments (1)
src/utils/spec-discovery.ts (1)
101-129: Sort directory entries for deterministic discovery order.
This avoids OS/filesystem-dependent ordering in outputs and tests.♻️ Suggested refactor
- entries = fs.readdirSync(dir, { withFileTypes: true }); + entries = fs.readdirSync(dir, { withFileTypes: true }); + entries.sort((a, b) => a.name.localeCompare(b.name));
openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md
Show resolved
Hide resolved
| 4. **Performance optimization for very large repos (1000+ specs)?** | ||
| - Consider adding a cache file (`.openspec-cache`) if benchmarks show issues | ||
| - Not implementing initially - optimize if needed |
There was a problem hiding this comment.
Consider tightening phrasing (“very large”).
LanguageTool suggests avoiding “very”.
📝 Suggested edit
-4. **Performance optimization for very large repos (1000+ specs)?**
+4. **Performance optimization for large repos (1000+ specs)?**🧰 Tools
🪛 LanguageTool
[style] ~402-~402: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ally 4. Performance optimization for very large repos (1000+ specs)? - Consider ad...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`
around lines 402 - 404, Update the heading text "4. **Performance optimization
for very large repos (1000+ specs)?**" to remove the vague word "very" and use a
precise metric-driven phrase (for example: "4. **Performance optimization for
large repositories (≥1000 specs)?**" or "4. **Performance optimization for
repositories with 1000+ specs?**") so the intent is clearer; edit the two bullet
points under that heading accordingly to match the tightened phrasing.
… duplicating validation Export the shared Zod schema from config-schema.ts and use safeParse in getGlobalConfig() instead of manual field-by-field validation.
findAllSpecs is synchronous, so the async wrapper and Promise.all were adding overhead for no reason.
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
| ### Decision 2: Replicated Hierarchy in Change Deltas | ||
|
|
||
| **Choice:** Delta specs replicate the main spec directory structure exactly. | ||
|
|
||
| **Rationale:** | ||
| - Eliminates mapping ambiguity: `changes/X/specs/_global/testing/spec.md` → `specs/_global/testing/spec.md` | ||
| - No metadata or configuration needed for sync/archive operations | ||
| - Easy to understand and predict where deltas map | ||
| - Simplifies implementation of `findSpecUpdates()` and `applySpecs()` | ||
|
|
||
| **Alternatives considered:** | ||
| - Flat deltas with metadata: Requires parsing metadata files, can desync | ||
| - Convention-based naming: Complex rules, error-prone | ||
| - Config-based mapping: Requires configuration, not zero-config | ||
|
|
||
| **Implementation:** | ||
| ```typescript | ||
| function findSpecUpdates(changeDir: string, mainSpecsDir: string): SpecUpdate[] { | ||
| const updates: SpecUpdate[] = []; | ||
| const changeSpecsDir = path.join(changeDir, 'specs'); | ||
|
|
||
| // Find all delta specs recursively | ||
| const deltaSpecs = findAllSpecs(changeSpecsDir); | ||
|
|
||
| for (const delta of deltaSpecs) { | ||
| // Map using relative path | ||
| const targetPath = path.join(mainSpecsDir, delta.capability, 'spec.md'); | ||
| const exists = fs.existsSync(targetPath); | ||
|
|
||
| updates.push({ | ||
| source: delta.path, | ||
| target: targetPath, | ||
| capability: delta.capability, | ||
| exists | ||
| }); | ||
| } | ||
|
|
||
| return updates; | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Clarify sync operation semantics and atomicity.
The delta replication design leaves important questions unanswered:
- Deletion handling - The
findSpecUpdates()example only handles additions/modifications. How are spec deletions represented? Does absence in delta mean "delete from main" or "no change"? - Atomicity - No discussion of transaction semantics. If syncing 10 deltas and
#7fails, what happens to#1-6? Consider requiring all-or-nothing semantics or documenting partial-sync behavior. - Directory creation - Does
applySpecs()auto-create parent directories for new specs? What about cleanup of empty directories after deletions? - Conflict resolution - What if main spec changed since delta was created? Should sync detect and warn about conflicts?
These operational semantics should be documented in this design or a separate "Sync Behavior" decision.
🤖 Prompt for AI Agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`
around lines 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.
| ### Decision 3: Auto-Detection with Optional Configuration | ||
|
|
||
| **Choice:** Auto-detect structure by default, allow optional configuration override. | ||
|
|
||
| **Rationale:** | ||
| - Zero-configuration works for 90% of users | ||
| - Detection cost is minimal (scan a few specs) | ||
| - Configuration useful for strict validation and migrations | ||
| - Explicit declaration documents project intent | ||
|
|
||
| **Detection algorithm:** | ||
| ```typescript | ||
| function isSpecStructureHierarchical(specsDir: string): boolean { | ||
| const specs = findAllSpecs(specsDir); | ||
| // If any spec has '/' in capability name, it's hierarchical | ||
| return specs.some(s => s.capability.includes(path.sep)); | ||
| } | ||
| ``` | ||
|
|
||
| **Configuration schema:** | ||
| ```typescript | ||
| interface SpecStructureConfig { | ||
| structure?: 'flat' | 'hierarchical' | 'auto'; // default: 'auto' | ||
| maxDepth?: number; // default: 4 | ||
| allowMixed?: boolean; // default: true | ||
| validatePaths?: boolean; // default: true | ||
| } | ||
| ``` | ||
|
|
||
| Added to global config under `specStructure` key (camelCase following existing `featureFlags` convention). | ||
|
|
||
| **Config usage:** | ||
| ```typescript | ||
| function getEffectiveStructure( | ||
| specsDir: string, | ||
| config: SpecStructureConfig | ||
| ): 'flat' | 'hierarchical' { | ||
| if (config.structure === 'auto' || !config.structure) { | ||
| return isSpecStructureHierarchical(specsDir) | ||
| ? 'hierarchical' | ||
| : 'flat'; | ||
| } | ||
| return config.structure; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Strengthen auto-detection robustness.
The auto-detection algorithm has reliability concerns:
- False positives (line 152) - A single accidentally nested empty directory triggers hierarchical mode. Consider requiring a threshold (e.g., >20% of specs are hierarchical) or checking for intentional hierarchy patterns.
- Empty directory handling - No mention of what happens when
specs/is empty or doesn't exist during detection. - Mixed structure ambiguity - Detection doesn't validate consistency. A malformed project could have both
specs/auth/spec.mdandspecs/payments/oauth/spec.md(mixed depths). Consider validating that the detected structure is intentional, not accidental.
🔍 Proposed detection improvement
function isSpecStructureHierarchical(specsDir: string): boolean {
const specs = findAllSpecs(specsDir);
if (specs.length === 0) return false; // Empty defaults to flat
const hierarchicalCount = specs.filter(s =>
s.capability.includes(path.sep)
).length;
// Require >20% hierarchical to avoid false positives from accidental nesting
return hierarchicalCount > 0 &&
(hierarchicalCount / specs.length) > 0.2;
}🤖 Prompt for AI Agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`
around lines 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.
| ### Decision 4: Validation Rules for Hierarchical Specs | ||
|
|
||
| **Choice:** Implement three validation rules with different severity levels. | ||
|
|
||
| **1. No Orphaned Specs (ERROR):** | ||
| Prevent `spec.md` at intermediate levels: | ||
| ```text | ||
| ✗ specs/auth/spec.md ← Has spec at intermediate level | ||
| specs/auth/oauth/spec.md ← Also has spec in child | ||
| ``` | ||
|
|
||
| **2. Depth Limits (WARNING/ERROR):** | ||
| - Warn at depth 4 (suggest simplification) | ||
| - Error at depth > 6 (hard limit via config `maxDepth`) | ||
| - Default `maxDepth: 4`, recommended 2-3 | ||
|
|
||
| **3. Naming Conventions (ERROR):** | ||
| - Lowercase alphanumeric with hyphens/underscores: `/^[a-z0-9-_]+$/` | ||
| - No reserved names: `..`, `.`, `.git`, `node_modules` | ||
| - No leading/trailing separators | ||
|
|
||
| **Rationale:** | ||
| - Orphaned specs create ambiguity in capability resolution | ||
| - Depth limits prevent over-organization (code smell) | ||
| - Naming conventions ensure cross-platform compatibility | ||
|
|
There was a problem hiding this comment.
Expand validation rules for cross-platform robustness.
The validation rules have gaps:
- Path segment validation (line 200) - The regex
/^[a-z0-9-_]+$/should be applied to individual path segments, not the full capability path. Clarify this applies per-directory-name. - Missing Windows reserved names - Beyond
.gitandnode_modules, Windows reservesCON,PRN,AUX,NUL,COM1-9,LPT1-9. These cause silent failures or errors on Windows. - Case sensitivity - No mention of handling case-insensitive filesystems (macOS HFS+, Windows). Specs
Auth/spec.mdandauth/spec.mdcould collide. - Path length limits - Windows has a 260-character path limit (unless long paths are enabled). Deep hierarchies like
_global/really/deeply/nested/capabilitycould exceed this in delta paths.
🛡️ Suggested validation additions
Add to validation rules:
**4. Cross-platform path safety (ERROR):**
- Validate each path segment against Windows reserved names
- Warn when full path exceeds 200 characters (Windows compatibility)
- Detect case-only collisions (e.g., `Auth` vs `auth`)🤖 Prompt for AI Agents
In `@openspec/changes/archive/2026-02-04-hierarchical-specs-support/design.md`
around lines 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.
2 participants
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:
Implementation notes:
References: #581
Summary by CodeRabbit
New Features
Validation / CLI
Documentation
Examples
Tests
Chores