Skip to content

fix(spec-graph): hold the write-path invariants - #331

Open
Ivan Logutov (VanishJr) wants to merge 3 commits into
JetBrains:mainfrom
VanishJr:fix/spec-graph-write-path
Open

fix(spec-graph): hold the write-path invariants#331
Ivan Logutov (VanishJr) wants to merge 3 commits into
JetBrains:mainfrom
VanishJr:fix/spec-graph-write-path

Conversation

@VanishJr

@VanishJr Ivan Logutov (VanishJr) commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

fix(spec-graph): hold the write-path invariants

The bug

spec_create passed its path parameter straight to join(root, path), so it wrote outside the project, through symlinks, into ignored directories and under non .md names, reporting Created … 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

01-before-card

After

02-after-card

The fix

core gains resolveSpecPath(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 with lstat because the walk never descends one. spec_create also parses the bytes it is about to write, refuses them unless they read back as a spec (an empty id serialized to a file that was not one), and writes with flag: "wx".

Three more defects in the same module, each with a regression test:

  • spec_update picked 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.
  • 'SpecIndex' consumed 'readdir' order, so which file won a duplicate 'id' depended on the filesystem. The glob now filters each directory down to its traversal candidates, then sorts them on the NFC normalized name with a raw name tie breaker, so the order is total on every filesystem.
  • spec_grep matched lines still carrying a trailing \r or a leading BOM, so no anchored pattern matched a CRLF spec, and a non positive limit reported No matches. while matches existed.

Scope

A realpath containment 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_update can overwrite an external edit landing inside the index's revalidation window, and spec_validate reports none of the schema rules the skill documents.

Verifying

Before: spec_create with path: "../evil.md" answers Created ../evil.md and the file exists outside the project. With path: "notes/spec.txt" it answers Created and spec_get then answers No spec with id.

After: both are refused with the reason, and nothing is written.

Checklist

  • Fast gates pass: bun run lint, bun run typecheck, bun run test
  • E2E suite passes for app-affecting changes (bun run e2e, or bun run e2e:full when touching agent behavior)
  • Relevant SPEC.md / top-level specs updated to reflect any boundary, contract, or behavior change
  • I have read the Contributing guide and agree to the Code of Conduct

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@rsolmano Rinat S (rsolmano) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/spec-graph/core/store.ts Outdated
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));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/spec-graph/core/store.ts Outdated
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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Done as suggested + 2 regression tests

Comment thread packages/spec-graph/core/store.ts Outdated
} catch {
return;
}
const order = new Map(dirents.map((d) => [d.name, d.name.normalize("NFC")]));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done + test added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants