fix(specs): discover nested spec paths recursively across parse, apply, and archive#1355
fix(specs): discover nested spec paths recursively across parse, apply, and archive#1355clay-good wants to merge 3 commits into
Conversation
…y, and archive Delta discovery previously read only specs/<name>/spec.md one directory level below a change's specs root, so nested layouts like specs/<area>/<capability>/spec.md were silently skipped: show reported deltaCount 0, and archive/apply completed without merging the delta into the main specs directory. Main-spec discovery had the same one-level assumption, so nested capabilities were also invisible to list, show, view, and validate. Introduce a shared recursive discoverSpecFiles() helper and use it in the change parser, findSpecUpdates (apply/sync/archive), archive's delta detection, and main-spec discovery (item-discovery, list, spec list, view). Capability ids are the directory path relative to the specs root, forward-slash separated on every platform, and apply/archive preserve the relative path when writing the target spec. Symlinks are not followed and dot-directories are skipped. Fixes #1353 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds recursive ChangesNested spec discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Command
participant discoverSpecFiles
participant Parser
participant SpecStorage
Command->>discoverSpecFiles: discover nested spec.md files
discoverSpecFiles-->>Command: return capability IDs and paths
Command->>Parser: read and parse discovered specs
Parser-->>Command: return spec metadata or delta data
Command->>SpecStorage: apply or archive using capability-relative IDs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/utils/spec-discovery.ts (2)
38-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
localeComparesort is not guaranteed deterministic across environments.The docstring promises results are "sorted by id for deterministic output," but
Array.prototype.sortwith no-localelocaleComparefollows the process's ICU default locale (which can differ by OS/CILANG/LC_ALL), so ordering can vary between environments for edge cases (punctuation/case collation). A plain code-point comparison guarantees the deterministic contract that's documented.♻️ Proposed fix for deterministic ordering
- return results.sort((a, b) => a.id.localeCompare(b.id)); + return results.sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0));Please confirm whether Node's default
Intl/ICU locale resolution in this project's CI can vary (e.g., viaLANG), which would make the current sort non-deterministic across environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/spec-discovery.ts` at line 38, Replace the locale-dependent comparator in the results sort within the spec-discovery routine with a plain deterministic code-point comparison of a.id and b.id, preserving ascending ID order and the documented cross-environment determinism.
22-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallows all
readdirerrors, not just a missing root.Any failure (EACCES, EIO, etc.) on a subdirectory is treated identically to "doesn't exist" — the subtree is silently dropped with no diagnostic. A permission problem on one capability folder would make its
spec.mdsilently vanish from discovery.🛡️ Proposed fix to surface non-ENOENT errors
try { entries = await fs.readdir(dir, { withFileTypes: true }); - } catch { + } catch (err: any) { + if (err?.code !== 'ENOENT') { + console.warn(`Warning: could not read directory ${dir}: ${err?.message ?? err}`); + } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/spec-discovery.ts` around lines 22 - 27, Update the readdir error handling in the spec-discovery traversal to suppress only ENOENT for a missing directory; propagate or surface all other errors, including permission and I/O failures, instead of silently returning. Preserve the existing missing-directory behavior and anchor the change to the fs.readdir call and its surrounding catch block.test/utils/spec-discovery.test.ts (1)
22-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the documented "symlinks are not followed" behavior.
discoverSpecFiles's docstring explicitly calls out symlink handling, but none of these tests create a symlinked directory orspec.mdto confirm it's actually skipped rather than followed. Worth a regression test (guarded/skipped on platforms without symlink privileges, e.g. Windows CI without dev mode).As per path instructions, "Add an alias-path regression test when touching path identity logic," which applies here since symlinks are a path-aliasing mechanism this module explicitly documents handling for.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils/spec-discovery.test.ts` around lines 22 - 70, Add a regression test alongside the discoverSpecFiles tests that creates symlinked directories and/or spec.md entries, then verifies discoverSpecFiles skips them and does not return aliased paths. Guard or skip the test when symlink creation is unavailable, such as restricted Windows environments, while preserving existing discovery assertions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/spec-discovery.ts`:
- Line 38: Replace the locale-dependent comparator in the results sort within
the spec-discovery routine with a plain deterministic code-point comparison of
a.id and b.id, preserving ascending ID order and the documented
cross-environment determinism.
- Around line 22-27: Update the readdir error handling in the spec-discovery
traversal to suppress only ENOENT for a missing directory; propagate or surface
all other errors, including permission and I/O failures, instead of silently
returning. Preserve the existing missing-directory behavior and anchor the
change to the fs.readdir call and its surrounding catch block.
In `@test/utils/spec-discovery.test.ts`:
- Around line 22-70: Add a regression test alongside the discoverSpecFiles tests
that creates symlinked directories and/or spec.md entries, then verifies
discoverSpecFiles skips them and does not return aliased paths. Guard or skip
the test when symlink creation is unavailable, such as restricted Windows
environments, while preserving existing discovery assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13f62c4c-b1df-404d-b432-7e5f18f7cec6
📒 Files selected for processing (11)
src/commands/spec.tssrc/core/archive.tssrc/core/list.tssrc/core/parsers/change-parser.tssrc/core/specs-apply.tssrc/core/view.tssrc/utils/item-discovery.tssrc/utils/spec-discovery.tstest/core/archive.test.tstest/core/parsers/change-parser.test.tstest/utils/spec-discovery.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for jumping on this. The shared recursive helper is the right shape and the main archive/apply path looks much better now.
One change I think we should make before merging: \ currently catches every \ error and returns. That preserves missing-root behavior, but it also means EACCES/EIO on a nested specs directory silently removes that subtree from validate/show/archive/apply. Since this helper now feeds the merge path, that can recreate the same class of silent omission that #1353 is trying to close.
Please suppress only the expected missing-root case, and surface or throw non-ENOENT errors so archive/apply cannot complete while skipping an unreadable capability. A small regression test for non-ENOENT handling would make this solid. Symlink skipping is also worth a test since the helper documents it, but the unreadable-directory behavior is the merge blocker for me.
discoverSpecFiles swallowed every readdir error, so an unreadable (EACCES/EIO) capability directory silently vanished from validate/show/ archive/apply — recreating the data-loss class #1353 is closing, now on the merge path. Suppress only the expected missing-root ENOENT and rethrow everything else. Adds regression tests for non-ENOENT (ENOTDIR + guarded EACCES) and the documented symlink-not-followed behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good catch, agreed this was the merge blocker. Fixed in the latest commit:
} catch (err: any) {
if (err?.code === 'ENOENT') return;
throw err;
}Regression tests added:
Full suite green locally. |
localeCompare follows the process's ICU locale, so ordering could vary by OS/CI. Code-point comparison guarantees the deterministic output the docstring promises. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/spec-discovery.ts`:
- Around line 26-32: Update the walk function in spec discovery to distinguish
the initial specs root from descendant directories, suppressing ENOENT only for
the root and rethrowing it for nested readdir failures. Pass an isRoot indicator
or separate the root read while preserving normal traversal behavior. Add a
regression test that causes descendant readdir to fail with ENOENT and verifies
the error is propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1647d7e-7cfc-4923-97fa-7fa59da31446
📒 Files selected for processing (2)
src/utils/spec-discovery.tstest/utils/spec-discovery.test.ts
| const walk = async (dir: string, segments: string[]): Promise<void> => { | ||
| let entries; | ||
| try { | ||
| entries = await fs.readdir(dir, { withFileTypes: true }); | ||
| } catch (err: any) { | ||
| if (err?.code === 'ENOENT') return; | ||
| throw err; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only suppress ENOENT for the initial specs root.
Line 31 also swallows ENOENT from nested directories, so a capability deleted or made unavailable during traversal is silently omitted from apply/archive output. Pass an isRoot flag (or handle the initial readdir separately) and rethrow descendant failures.
Proposed fix
- const walk = async (dir: string, segments: string[]): Promise<void> => {
+ const walk = async (dir: string, segments: string[], isRoot = false): Promise<void> => {
let entries;
try {
entries = await fs.readdir(dir, { withFileTypes: true });
} catch (err: any) {
- if (err?.code === 'ENOENT') return;
+ if (isRoot && err?.code === 'ENOENT') return;
throw err;
}
for (const entry of entries) {
if (entry.name.startsWith('.')) continue;
if (entry.isDirectory()) {
- await walk(path.join(dir, entry.name), [...segments, entry.name]);
+ await walk(path.join(dir, entry.name), [...segments, entry.name]);
}
}
};
- await walk(specsRoot, []);
+ await walk(specsRoot, [], true);Add a regression test that makes a descendant readdir fail with ENOENT.
📝 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.
| const walk = async (dir: string, segments: string[]): Promise<void> => { | |
| let entries; | |
| try { | |
| entries = await fs.readdir(dir, { withFileTypes: true }); | |
| } catch (err: any) { | |
| if (err?.code === 'ENOENT') return; | |
| throw err; | |
| const walk = async (dir: string, segments: string[], isRoot = false): Promise<void> => { | |
| let entries; | |
| try { | |
| entries = await fs.readdir(dir, { withFileTypes: true }); | |
| } catch (err: any) { | |
| if (isRoot && err?.code === 'ENOENT') return; | |
| throw err; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/spec-discovery.ts` around lines 26 - 32, Update the walk function
in spec discovery to distinguish the initial specs root from descendant
directories, suppressing ENOENT only for the root and rethrowing it for nested
readdir failures. Pass an isRoot indicator or separate the root read while
preserving normal traversal behavior. Add a regression test that causes
descendant readdir to fail with ENOENT and verifies the error is propagated.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the nested discovery path end to end and this looks solid to me. The shared helper is scoped well, preserves the relative capability id through apply/archive, avoids symlink traversal, and the focused plus full test suites are green.
2 participants
Fixes #1353.
Status: Ready for review. Full suite green locally (the 17 zsh-installer failures are a known local oh-my-zsh artifact).
What was wrong: Delta discovery only read
specs/<name>/spec.mdone directory level below a change's specs root. A nested delta likechanges/my-change/specs/platform/example-capability/spec.mdwas silently skipped:show --json --deltas-onlyreporteddeltaCount: 0, andarchive/applycompleted "successfully" without merging the delta into the main specs — the data-loss case in #1353. Main-spec discovery had the same one-level assumption, so nested capabilities were also invisible tolist,show,view, andvalidate. (#1280 had already made one validator path recursive; the parser and apply paths still weren't.)How it was fixed: A shared recursive
discoverSpecFiles()helper (src/utils/spec-discovery.ts) now backs every discovery site: the change parser,findSpecUpdates(apply/sync/archive), archive's delta detection, and main-spec discovery (getSpecIds,list,spec list,view). Capability ids are the directory path relative to the specs root, forward-slash separated on every platform (platform/example-capability), and apply/archive preserve that relative path when writing the target spec. Symlinks are not followed; dot-directories are skipped; flat one-level layouts behave exactly as before.Replication / proof: Using the issue's minimal repro (
changes/nested-test/specs/platform/example-capability/spec.md):Before (v1.6.0 build from main):
showreturneddeltaCount: 0, andarchive --yescompleted while leavingopenspec/specs/empty.After:
Regression tests:
discoverSpecFilesunit tests (flat, nested, dot-dir/root-file exclusion, missing root), a nested-id change-parser test, and a nested archive round-trip test.Notes: Duplicate-id detection and Windows-specific CI tests from the issue's wish list are not included to keep this surgical — ids are derived from unique directory paths, and
path.joinhandles separators.spec show platform/example-capabilityandvalidatealready resolve slash ids since every consumer builds paths withpath.join(specsDir, id, 'spec.md').🤖 Generated with Claude Code
Summary by CodeRabbit