fix(spec-graph): hold the write-path invariants - #331
fix(spec-graph): hold the write-path invariants#331Ivan Logutov (VanishJr) wants to merge 3 commits into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rinat S (rsolmano)
left a comment
There was a problem hiding this comment.
Review summary
The overall direction is strong and the new tests cover the primary regressions well. However, the canonical-path and deterministic-order invariants still have two filesystem-dependent gaps, including one reproduced on a default case-insensitive macOS filesystem. Please address the findings below before merge. I also included a low-severity performance improvement for the new synchronous sorting path.
| const lexical = normalize(path); | ||
| const segments = lexical.split(sep); | ||
| if (segments[0] === "..") return { error: `Path must stay inside the project root: ${path}` }; | ||
| const ignored = segments.find((segment) => IGNORED_DIRS.has(segment)); |
There was a problem hiding this comment.
[P1] Resolve existing components using their on-disk spelling
This ignored-directory check uses the caller's lexical spelling. On a case-insensitive filesystem, NODE_MODULES/SPEC.md resolves into an existing node_modules directory, passes validation, and can be written successfully, but SpecIndex.walk() sees node_modules and skips the created file. I reproduced this on macOS: resolution succeeded, the file appeared under the lowercase directory, and the resulting index contained no spec. The same issue lets the returned rel disagree with the path later reported by the index when existing directory casing or Unicode normalization differs.
Please canonicalize each existing path component against its parent directory entry before checking IGNORED_DIRS and constructing rel, and add case-alias coverage for ignored and normal directories.
There was a problem hiding this comment.
Done as you proposed. Two calls it left open: it fails closed on zero or two folded matches and it stops canonicalizing at the first component that doesn't exist. That stopping rule left one more gap, closed by a case-folded ignored check on the write path; the glob stays byte-exact.
| dirents.sort((a, b) => { | ||
| const left = order.get(a.name) ?? a.name; | ||
| const right = order.get(b.name) ?? b.name; | ||
| return left < right ? -1 : left > right ? 1 : 0; |
There was a problem hiding this comment.
[P2] Make the NFC ordering a total order
Distinct names such as precomposed é and decomposed e\u0301 normalize to the same key, so this comparator returns 0. Stable sort then preserves the unspecified readdirSync() order, leaving duplicate-ID ownership dependent on filesystem/insertion order.
Please add a deterministic raw-name/code-unit tie-breaker after the normalized comparison, with a regression test covering two canonically equivalent names in opposite input orders.
There was a problem hiding this comment.
I agree. Done as suggested + 2 regression tests
| } catch { | ||
| return; | ||
| } | ||
| const order = new Map(dirents.map((d) => [d.name, d.name.normalize("NFC")])); |
There was a problem hiding this comment.
[P3] Filter traversal candidates before sorting
Every index refresh now NFC-normalizes and sorts all entries in each directory before ignored directories and non-Markdown files are discarded. Because scans are synchronous and occur on every spec read, a directory with many irrelevant files adds avoidable O(n log n) event-loop work.
Please filter to traversable directories and .md files first, then normalize and sort only those candidates. This is low severity, but it is straightforward to address while correcting the comparator.
There was a problem hiding this comment.
Done + test added
fix(spec-graph): hold the write-path invariants
The bug
spec_createpassed itspathparameter straight tojoin(root, path), so it wrote outside the project, through symlinks, into ignored directories and under non.mdnames, reportingCreated …for files that no other spec tool can read back.It also reported the caller's spelling rather than the path the index produces.
Before
After
The fix
coregainsresolveSpecPath(root, path), the single answer to whether the index could ever see a path, returning the canonical relative path the tool then reports. It requires root relative, inside the root,.md, outside the ignored directories, an existing root, and no symlink at any component beneath the root, checked per component withlstatbecause the walk never descends one.spec_createalso parses the bytes it is about to write, refuses them unless they read back as a spec (an emptyidserialized to a file that was not one), and writes withflag: "wx".Three more defects in the same module, each with a regression test:
spec_updatepicked the file's line ending from the whole text, so one CRLF anywhere in the body rewrote every line ending in the file. It now rewrites the frontmatter block alone and splices the body back byte for byte, BOM included.spec_grepmatched lines still carrying a trailing\ror a leading BOM, so no anchored pattern matched a CRLF spec, and a non positivelimitreportedNo matches.while matches existed.Scope
A
realpathcontainment check was tried and withdrawn: it accepts a symlink that points back inside the root, and the walk skips that file anyway. Rejecting every symlinked component is both stricter and simpler.Further defects in this module are left out and want their own issues: an indented
---inside a multi line YAML scalar relocates the closing fence, list edits drop comments on the field they edit, a model supplied regex can stall the in process host,spec_updatecan overwrite an external edit landing inside the index's revalidation window, andspec_validatereports none of the schema rules the skill documents.Verifying
Before:
spec_createwithpath: "../evil.md"answersCreated ../evil.mdand the file exists outside the project. Withpath: "notes/spec.txt"it answersCreatedandspec_getthen answersNo spec with id.After: both are refused with the reason, and nothing is written.
Checklist
bun run lint,bun run typecheck,bun run testbun run e2e, orbun run e2e:fullwhen touching agent behavior)SPEC.md/ top-level specs updated to reflect any boundary, contract, or behavior change🤖 Generated with Claude Code