From 92415992f0364cec6e417941fd9d62aab43651a9 Mon Sep 17 00:00:00 2001 From: VanishJr <91227368+VanishJr@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:22:01 +0200 Subject: [PATCH 1/3] fix(spec-graph): hold the write-path invariants Co-Authored-By: Claude Opus 5 (1M context) --- packages/spec-graph/SPEC.md | 27 +++-- packages/spec-graph/core/SPEC.md | 31 ++++- packages/spec-graph/core/core.test.ts | 146 +++++++++++++++++++++++- packages/spec-graph/core/index.ts | 9 +- packages/spec-graph/core/parse.ts | 14 ++- packages/spec-graph/core/query.ts | 8 +- packages/spec-graph/core/store.ts | 53 ++++++++- packages/spec-graph/tools/SPEC.md | 5 + packages/spec-graph/tools/create.ts | 29 +++-- packages/spec-graph/tools/grep.ts | 6 +- packages/spec-graph/tools/tools.test.ts | 95 ++++++++++++++- 11 files changed, 382 insertions(+), 41 deletions(-) diff --git a/packages/spec-graph/SPEC.md b/packages/spec-graph/SPEC.md index dabf05e8c..6a35a6a7c 100644 --- a/packages/spec-graph/SPEC.md +++ b/packages/spec-graph/SPEC.md @@ -19,7 +19,8 @@ It manages the frontmatter schema the repo's specs use — a file is a spec once `id` and `type`; the schema itself is documented in the skill. Frontmatter is handled with the `yaml` library, link/metadata lists inline. `spec_create` writes new frontmatter in a canonical field order; `spec_update` edits an existing file **in place** — it preserves the file's own field order and any -comments / nested fields, and writes back the original line ending (LF or CRLF). +comments / nested fields, rewrites the frontmatter block alone (in the line ending that block already +used), and leaves every byte of the body untouched. ## Boundary @@ -45,22 +46,24 @@ own boundary and leaves. ## Derived read index The filesystem is the source of truth; the model the tools read is **derived, in-memory, read-only, and -revalidated on demand**. Each read re-globs the spec set (ignoring `node_modules`/`.git`/`dist`/`build`) -and revalidates every file by `(mtimeMs, size)`: unchanged files reuse their cached parse, changed/new -files are re-parsed, vanished files are evicted, and the derived graph is rebuilt only when the spec set -actually changed. So specs added, deleted, or edited from any source — including pi's normal `write`/`edit` -— are always current, while redundant re-parse/rebuild is skipped when nothing moved. One `SpecIndex` is -reused per root (keyed by cwd) so the cache pays off across an agent's calls. The one theoretical miss is -an edit landing within the same mtime tick *and* keeping byte length identical (negligible; a content hash -is the sanctioned escalation). +revalidated on demand**. Each read re-globs the spec set in a name-sorted walk (ignoring +`node_modules`/`.git`/`dist`/`build`) and revalidates every file by `(mtimeMs, size)`: unchanged files +reuse their cached parse, changed/new files are re-parsed, vanished files are evicted, and the derived +graph is rebuilt only when the spec set actually changed. So specs added, deleted, or edited from any +source — including pi's normal `write`/`edit` — are always current, while redundant re-parse/rebuild is +skipped when nothing moved. One `SpecIndex` is reused per root (keyed by cwd) so the cache pays off +across an agent's calls. The one theoretical miss is an edit landing within the same mtime tick *and* +keeping byte length identical (negligible; a content hash is the sanctioned escalation). ## Tools Read — `spec_grep` (content search, narrowable by metadata), `spec_get` (a node's frontmatter, resolved links, and path — no body), `spec_graph` (a bounded subtree/ancestors/neighbors slice). Manage — -`spec_create`, `spec_update` (frontmatter only), `spec_delete`, `spec_validate`. Per-tool usage lives in -the skill and in each tool's `description`; **none edit prose** — prose is written/edited with pi's normal -`read`/`write`/`edit`. +`spec_create` (which refuses any path the index could never see — see `resolveSpecPath` in +`submodule-spec-graph-core` — and any parameters that would write a file born a non-spec), +`spec_update` (frontmatter only), `spec_delete`, `spec_validate`. Per-tool +usage lives in the skill and in each tool's `description`; **none edit prose** — prose is written/edited +with pi's normal `read`/`write`/`edit`. ## Knowledge delivery diff --git a/packages/spec-graph/core/SPEC.md b/packages/spec-graph/core/SPEC.md index 61f3fcedf..93790a696 100644 --- a/packages/spec-graph/core/SPEC.md +++ b/packages/spec-graph/core/SPEC.md @@ -36,14 +36,27 @@ Acyclic and one-way: `parse` is the root, `graph` builds on it, and `query`/`val | `graph.ts` | files → nodes + edges (parent tree, DAG + reverse); duplicate-id tracking | `parse` | | `query.ts` | content grep with metadata filters; bounded graph slices | `parse`, `graph` | | `validate.ts` | dangling links, duplicate ids, parent cycles | `parse`, `graph` | -| `store.ts` | `SpecIndex`: the on-demand fs glob + per-file parse cache + memoized graph (the `core/index` module) | `parse`, `graph`, `query` | +| `store.ts` | `SpecIndex`: the on-demand fs glob + per-file parse cache + memoized graph (the `core/index` module); the indexable-path rule (`resolveSpecPath`) | `parse`, `graph`, `query` | ## Invariants - No `@earendil-works/*` import anywhere under `core/`. - `buildGraph` is pure (same input → same output); the index revalidates each file by `(mtimeMs, size)`, memoizes the graph, and never serves a stale one. -- On a duplicate `id`, the first file seen wins the node slot; the duplicate set is recorded for `validate`. +- The glob sorts each directory's entries by their **NFC-normalized** name, so the spec order — and + therefore which file wins a duplicate `id` — is the same on every filesystem, not whatever order + `readdir` happened to return, and not a different answer on a filesystem that hands back decomposed + (NFD) names. +- On a duplicate `id`, the first file in that order wins the node slot; the duplicate set is recorded for + `validate`. +- `resolveSpecPath` is the single answer to "could the index ever see this path?" and it answers with the + **canonical relative path**, so a caller can never report an identity the index will not produce. + It requires: root-relative, inside the root, `.md`, outside the ignored dirs, an existing root, and no + symlink at any component beneath the root — checked with `lstat` per component, dangling links included, + because the walk never descends a symlink. That last rule is what a string check and a `realpath` + comparison both miss: a link is rejected whether it leaves the project, lands in an ignored directory, + or points back at an indexed one, since in every case the file it creates is invisible to every other + spec tool. - `SpecNode.type` stays `string`: the read model indexes whatever is on disk, so it tolerates any `type`; the `SPEC_TYPES` vocabulary constrains only the `spec_create` authoring surface, never the graph. - Finite vocabularies (`SPEC_TYPES`, `SPEC_STATUSES`, `SLICE_DIRECTIONS`, `LINK_KINDS`, `IDENTITY_FIELDS`) @@ -52,6 +65,14 @@ Acyclic and one-way: `parse` is the root, `graph` builds on it, and `query`/`val - Reads coerce frontmatter to a scalar/string-array dialect (lossy — nested maps and comments are dropped), which is fine for the derived model. The write path (`updateFrontmatterText`) is **lossless**: it mutates a live `yaml` Document in place, so untouched fields keep their order and any comments / - nested values survive, and it writes the file back in its original line ending (LF or CRLF). Field - order is **preserved, never re-sorted** — `FIELD_ORDER` is only the order `spec_create` builds *new* - frontmatter in. The `\r`-strip on the fence-interior lines is what makes CRLF-authored files parse. + nested values survive. Field order is **preserved, never re-sorted** — `FIELD_ORDER` is only the order + `spec_create` builds *new* frontmatter in. The `\r`-strip on the fence-interior lines is what makes + CRLF-authored files parse. +- The write path rewrites **only the frontmatter block**: the body is spliced back byte-for-byte, a leading + BOM is put back, and the line ending applied to the rewritten block is read from the frontmatter's own + first break (LF or CRLF). Nothing is inferred from the body — a file whose prose happens to mix endings + keeps every prose byte it had, which is what makes "`spec_update` never edits prose" true of the bytes + and not just of the fields. +- The read paths carry the same obligation the other way: `grepSpecs` splits on `\n`, drops a trailing + `\r` and a leading BOM before matching, so an anchored pattern behaves identically on an LF spec, a + CRLF spec, and a BOM-prefixed one — the BOM would otherwise hide line 1 from every `^` pattern. diff --git a/packages/spec-graph/core/core.test.ts b/packages/spec-graph/core/core.test.ts index 110cb3db3..2977b7679 100644 --- a/packages/spec-graph/core/core.test.ts +++ b/packages/spec-graph/core/core.test.ts @@ -1,9 +1,10 @@ import { expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { buildGraph, + DEFAULT_GREP_LIMIT, FIELD_ORDER, FIELDS, graphSlice, @@ -15,8 +16,10 @@ import { LIST_LINK_FIELDS, parseFile, REQUIRED_FIELDS, + resolveSpecPath, SINGLE_LINK_FIELDS, SLICE_DIRECTIONS, + SPEC_FILE_EXTENSION, SPEC_STATUSES, SPEC_TYPES, SpecIndex, @@ -203,6 +206,30 @@ test("updateFrontmatterText writes the file back in its original CRLF line endin expect(/(? { + const body = "# Body\nplain line\r\nanother\n"; + const res = updateFrontmatterText(`---\nid: a\ntype: module-design\ntitle: T\n---\n${body}`, { + set: { title: "T2" }, + }) as { content: string }; + expect(res.content).toBe(`---\nid: a\ntype: module-design\ntitle: T2\n---\n${body}`); +}); + +test("updateFrontmatterText reads the line ending from the frontmatter, not from any body line", () => { + const lfWithOneCrlfInProse = "---\nid: a\ntype: module-design\ntitle: T\n---\nprose\r\n"; + const res = updateFrontmatterText(lfWithOneCrlfInProse, { set: { title: "T2" } }) as { + content: string; + }; + expect(res.content.startsWith("---\nid: a\n")).toBe(true); + expect(res.content.endsWith("prose\r\n")).toBe(true); +}); + +test("updateFrontmatterText keeps a leading BOM the split step strips off", () => { + const file = "\ufeff---\nid: a\ntype: module-design\ntitle: T\n---\nbody\n"; + const res = updateFrontmatterText(file, { set: { title: "T2" } }) as { content: string }; + expect(res.content.startsWith("\ufeff")).toBe(true); + expect(parseFile(res.content).frontmatter?.title).toBe("T2"); +}); + test("updateFrontmatterText rejects set on a list field (use addList/removeList instead)", () => { const res = updateFrontmatterText("---\nid: a\ntype: t\n---\nbody\n", { set: { tags: "a, b" } }); expect("error" in res).toBe(true); @@ -317,6 +344,30 @@ test("grepSpecs marks truncated only when a match exists beyond the limit", () = expect(cut.truncated).toBe(true); }); +test("grepSpecs strips the CR of a CRLF spec so anchored patterns still match", () => { + const entries = [ + { + path: "a.md", + content: "---\r\nid: a\r\ntype: t\r\n---\r\nhello world\r\n", + frontmatter: { id: "a", type: "t" }, + }, + ]; + expect(grepSpecs(entries, { pattern: "world$", regex: true }).matches).toHaveLength(1); + expect(grepSpecs(entries, { pattern: "^hello", regex: true }).matches[0]?.snippet).toBe( + "hello world", + ); +}); + +test("grepSpecs falls back to the default limit rather than reporting a silent truncation", () => { + const content = [{ path: "a.md", content: "x\nx", frontmatter: { id: "a", type: "t" } }]; + for (const limit of [0, -5, 0.5]) { + const res = grepSpecs(content, { pattern: "x", limit }); + expect(res.matches).toHaveLength(2); + expect(res.truncated).toBe(false); + } + expect(DEFAULT_GREP_LIMIT).toBe(200); +}); + test("validateGraph flags dangling links, duplicate ids, and parent cycles", () => { const g = buildGraph([ { path: "a.md", frontmatter: { id: "a", type: "t", parent: "b", "depends-on": ["ghost"] } }, @@ -439,3 +490,96 @@ test("SpecIndex tracks a file entering and leaving spec-hood via its frontmatter expect([...index.graph().nodes.keys()]).toEqual([]); }); }); + +function withProject(fn: (root: string, outer: string) => void): void { + withIndexRoot((outer) => { + const root = join(outer, "project"); + mkdirSync(root, { recursive: true }); + fn(root, outer); + }); +} + +const ok = (r: ReturnType): string => { + if ("error" in r) throw new Error(`expected a resolved path, got: ${r.error}`); + return r.rel; +}; + +test("resolveSpecPath returns the canonical relative path the index would report", () => { + withProject((root) => { + expect(ok(resolveSpecPath(root, "SPEC.md"))).toBe("SPEC.md"); + expect(ok(resolveSpecPath(root, "packages/core/SPEC.md"))).toBe("packages/core/SPEC.md"); + expect(ok(resolveSpecPath(root, "./packages/core/SPEC.md"))).toBe("packages/core/SPEC.md"); + expect(ok(resolveSpecPath(root, "pkg/./sub/../SPEC.md"))).toBe("pkg/SPEC.md"); + expect(SPEC_FILE_EXTENSION).toBe(".md"); + }); +}); + +test("resolveSpecPath rejects every path the index could never see", () => { + withProject((root) => { + for (const path of [ + "", + "../outside.md", + "pkg/../../outside.md", + "/etc/outside.md", + "notes/spec.txt", + "node_modules/dep/SPEC.md", + "pkg/dist/SPEC.md", + ]) { + expect(resolveSpecPath(root, path)).toHaveProperty("error"); + } + }); +}); + +test("resolveSpecPath rejects a symlinked directory even when it points back inside the root", () => { + withProject((root, outer) => { + mkdirSync(join(root, "real"), { recursive: true }); + mkdirSync(join(root, "node_modules", "hidden"), { recursive: true }); + mkdirSync(join(outer, "elsewhere"), { recursive: true }); + symlinkSync(join(outer, "elsewhere"), join(root, "away"), "dir"); + symlinkSync(join(outer, "never-created"), join(root, "gone"), "dir"); + symlinkSync(join(root, "node_modules", "hidden"), join(root, "docs"), "dir"); + symlinkSync(join(root, "real"), join(root, "alias"), "dir"); + + expect(resolveSpecPath(root, "away/evil.md")).toHaveProperty("error"); + expect(resolveSpecPath(root, "gone/evil.md")).toHaveProperty("error"); + expect(resolveSpecPath(root, "docs/ghost.md")).toHaveProperty("error"); + expect(resolveSpecPath(root, "alias/SPEC.md")).toHaveProperty("error"); + expect(ok(resolveSpecPath(root, "real/SPEC.md"))).toBe("real/SPEC.md"); + }); +}); + +test("resolveSpecPath rejects a symlink at the leaf, dangling or not", () => { + withProject((root, outer) => { + symlinkSync(join(outer, "never-created.md"), join(root, "dangling.md")); + writeFileSync(join(outer, "real-outside.md"), "outside\n"); + symlinkSync(join(outer, "real-outside.md"), join(root, "live.md")); + + expect(resolveSpecPath(root, "dangling.md")).toHaveProperty("error"); + expect(resolveSpecPath(root, "live.md")).toHaveProperty("error"); + }); +}); + +test("resolveSpecPath fails closed when the root does not exist", () => { + expect(resolveSpecPath(join(tmpdir(), "spec-index-definitely-absent"), "SPEC.md")).toHaveProperty( + "error", + ); +}); + +test("SpecIndex walks in a stable order, so a duplicate id resolves the same everywhere", () => { + withIndexRoot((root) => { + for (const name of ["z-later", "a-earlier", "m-middle"]) { + mkdirSync(join(root, name), { recursive: true }); + writeFileSync( + join(root, name, "SPEC.md"), + "---\nid: dup\ntype: module-design\ntitle: Dup\n---\n", + ); + } + const graph = new SpecIndex(root).graph(); + expect(graph.nodes.get("dup")?.path).toBe("a-earlier/SPEC.md"); + expect(graph.duplicateIds.get("dup")).toEqual([ + "a-earlier/SPEC.md", + "m-middle/SPEC.md", + "z-later/SPEC.md", + ]); + }); +}); diff --git a/packages/spec-graph/core/index.ts b/packages/spec-graph/core/index.ts index 33769524a..53cc4ffba 100644 --- a/packages/spec-graph/core/index.ts +++ b/packages/spec-graph/core/index.ts @@ -34,6 +34,7 @@ export { } from "./parse.ts"; export { + DEFAULT_GREP_LIMIT, type GraphSlice, type GrepMatch, type GrepOptions, @@ -46,7 +47,13 @@ export { type SpecContentEntry, type SpecFilters, } from "./query.ts"; -export { type SpecFileRecord, SpecIndex } from "./store.ts"; +export { + resolveSpecPath, + SPEC_FILE_EXTENSION, + type SpecFileRecord, + SpecIndex, + type SpecPathResolution, +} from "./store.ts"; export { type DanglingLink, type DuplicateId, diff --git a/packages/spec-graph/core/parse.ts b/packages/spec-graph/core/parse.ts index 9c49e049f..a38b19682 100644 --- a/packages/spec-graph/core/parse.ts +++ b/packages/spec-graph/core/parse.ts @@ -53,8 +53,15 @@ export const FIELD_ORDER = [ export type LinkKind = (typeof SINGLE_LINK_FIELDS)[number] | (typeof LIST_LINK_FIELDS)[number]; const FENCE = "---"; +const BOM = "\ufeff"; +const CRLF = "\r\n"; const TO_STRING = { lineWidth: 0, flowCollectionPadding: false } as const; +function lineEnding(text: string): string { + const first = text.indexOf("\n"); + return first > 0 && text[first - 1] === "\r" ? CRLF : "\n"; +} + export interface ParsedFile { frontmatter: Frontmatter | null; body: string; @@ -76,7 +83,7 @@ function toFrontmatter(loaded: unknown): Frontmatter | null { } function splitFrontmatter(content: string): { fmText: string | null; body: string } { - const normalized = content.startsWith("\ufeff") ? content.slice(1) : content; + const normalized = content.startsWith(BOM) ? content.slice(1) : content; const lines = normalized.split("\n"); if (lines[0]?.trim() !== FENCE) return { fmText: null, body: content }; let end = -1; @@ -215,6 +222,7 @@ export function updateFrontmatterText( } inlineLists(doc); - const out = `${FENCE}\n${doc.toString(TO_STRING)}${FENCE}\n${body}`; - return { content: fileText.includes("\r\n") ? out.replace(/\r?\n/g, "\r\n") : out }; + const block = `${FENCE}\n${doc.toString(TO_STRING)}${FENCE}\n`; + const rewritten = lineEnding(fileText) === CRLF ? block.replace(/\n/g, CRLF) : block; + return { content: `${fileText.startsWith(BOM) ? BOM : ""}${rewritten}${body}` }; } diff --git a/packages/spec-graph/core/query.ts b/packages/spec-graph/core/query.ts index f58c17605..b78da28e1 100644 --- a/packages/spec-graph/core/query.ts +++ b/packages/spec-graph/core/query.ts @@ -32,6 +32,8 @@ export interface GrepResult { truncated: boolean; } +export const DEFAULT_GREP_LIMIT = 200; + function matchesFilters(fm: Frontmatter, filters: SpecFilters): boolean { if (filters.type !== undefined && scalar(fm, FIELDS.type) !== filters.type) return false; if (filters.parent !== undefined && scalar(fm, FIELDS.parent) !== filters.parent) return false; @@ -56,14 +58,16 @@ function buildMatcher(opts: GrepOptions): (line: string) => boolean { } export function grepSpecs(entries: SpecContentEntry[], opts: GrepOptions): GrepResult { - const limit = opts.limit ?? 200; + const requested = Math.trunc(opts.limit ?? DEFAULT_GREP_LIMIT); + const limit = requested > 0 ? requested : DEFAULT_GREP_LIMIT; const matcher = buildMatcher(opts); const matches: GrepMatch[] = []; for (const entry of entries) { if (!matchesFilters(entry.frontmatter, opts)) continue; const lines = entry.content.split("\n"); for (let i = 0; i < lines.length; i++) { - const line = lines[i] ?? ""; + const raw = (i === 0 ? (lines[0]?.replace(/^\ufeff/, "") ?? "") : (lines[i] ?? "")) as string; + const line = raw.endsWith("\r") ? raw.slice(0, -1) : raw; if (matcher(line)) { if (matches.length >= limit) return { matches, truncated: true }; matches.push({ path: entry.path, line: i + 1, snippet: line.trim() }); diff --git a/packages/spec-graph/core/store.ts b/packages/spec-graph/core/store.ts index dee4bf4f8..a65e41f76 100644 --- a/packages/spec-graph/core/store.ts +++ b/packages/spec-graph/core/store.ts @@ -1,11 +1,52 @@ -import { readdirSync, readFileSync, statSync } from "node:fs"; -import { join, relative, sep } from "node:path"; +import { existsSync, lstatSync, readdirSync, readFileSync, statSync } from "node:fs"; +import { isAbsolute, join, normalize, relative, sep } from "node:path"; import { buildGraph, type SpecGraph } from "./graph.ts"; import { FIELDS, type Frontmatter, isSpec, parseFile, scalar } from "./parse.ts"; import type { SpecContentEntry } from "./query.ts"; const IGNORED_DIRS = new Set(["node_modules", ".git", "dist", "build"]); +export const SPEC_FILE_EXTENSION = ".md"; + +function isSymlink(target: string): boolean { + try { + return lstatSync(target).isSymbolicLink(); + } catch { + return false; + } +} + +export type SpecPathResolution = { rel: string; abs: string } | { error: string }; + +export function resolveSpecPath(root: string, path: string): SpecPathResolution { + if (path.trim() === "") return { error: "Path must not be empty." }; + if (isAbsolute(path)) return { error: `Path must be root-relative, not absolute: ${path}` }; + if (!path.endsWith(SPEC_FILE_EXTENSION)) { + return { error: `Spec files must end in ${SPEC_FILE_EXTENSION}: ${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)); + if (ignored !== undefined) { + return { + error: `Path is inside an ignored directory ("${ignored}") and would not be indexed: ${path}`, + }; + } + if (!existsSync(root)) return { error: `Project root does not exist: ${root}` }; + + let walked = root; + for (const segment of segments) { + walked = join(walked, segment); + if (isSymlink(walked)) { + return { error: `Path passes through a symlink, which the index never follows: ${path}` }; + } + } + + return { rel: lexical.split(sep).join("/"), abs: join(root, lexical) }; +} + export interface SpecFileRecord { abs: string; rel: string; @@ -45,12 +86,18 @@ export class SpecIndex { } catch { return; } + const order = new Map(dirents.map((d) => [d.name, d.name.normalize("NFC")])); + 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; + }); for (const dirent of dirents) { const abs = join(dir, dirent.name); if (dirent.isDirectory()) { if (IGNORED_DIRS.has(dirent.name)) continue; yield* this.walk(abs); - } else if (dirent.isFile() && dirent.name.endsWith(".md")) { + } else if (dirent.isFile() && dirent.name.endsWith(SPEC_FILE_EXTENSION)) { yield abs; } } diff --git a/packages/spec-graph/tools/SPEC.md b/packages/spec-graph/tools/SPEC.md index c9d495f17..8549eedbe 100644 --- a/packages/spec-graph/tools/SPEC.md +++ b/packages/spec-graph/tools/SPEC.md @@ -46,6 +46,11 @@ file; the other four are read-only. `spec_create.status` ← `SPEC_STATUSES`, `spec_graph.direction` ← `SLICE_DIRECTIONS`, `spec_graph.edge` ← `LINK_KINDS`) — never re-typed literals, so a `core` rename flows here with no edit (pinned by `tools/tools.test.ts`). +- `spec_create` writes only what the index can later read back, and it checks that in three places: the + target path goes through `core`'s `resolveSpecPath` (which also yields the canonical relative path the + tool reports, so its `Created ` never disagrees with what `spec_get` will say), the assembled + bytes must parse back as a spec before anything is written, and the write itself is exclusive + (`flag: "wx"`) so the existence check cannot be raced or satisfied by a link. - The spec root is `ctx.cwd`; one `SpecIndex` is reused per root (freshness handled in `core/` — see `module-spec-graph`). `spec_update` reads via `recordForId` to reuse the scan's cached read; write tools just write, and the next read picks the change up. diff --git a/packages/spec-graph/tools/create.ts b/packages/spec-graph/tools/create.ts index 2d5e1d53f..bc8532424 100644 --- a/packages/spec-graph/tools/create.ts +++ b/packages/spec-graph/tools/create.ts @@ -6,6 +6,9 @@ import { Type } from "typebox"; import { FIELDS, type Frontmatter, + isSpec, + parseFile, + resolveSpecPath, SPEC_STATUSES, SPEC_TYPES, serializeFrontmatter, @@ -14,7 +17,8 @@ import { errorResult, getIndex, scaffoldBody, textResult } from "./shared.ts"; const parameters = Type.Object({ path: Type.String({ - description: "Root-relative path for the new spec file (e.g. src/foo/SPEC.md).", + description: + "Root-relative path for the new spec file, ending in .md (e.g. src/foo/SPEC.md). Must stay inside the project root.", }), id: Type.String({ description: "Unique spec id." }), type: StringEnum(SPEC_TYPES, { @@ -40,14 +44,17 @@ export function registerSpecCreate(pi: ExtensionAPI): void { name: "spec_create", label: "Spec Create", description: - "Create a new spec file with scaffolded frontmatter (id, type, title, an optional status, and any links) and a heading-only body stub chosen by type. Fails if the file already exists or the id is already in use. Edit prose afterward with the write/edit tools.", + "Create a new spec file with scaffolded frontmatter (id, type, title, an optional status, and any links) and a heading-only body stub chosen by type. Fails if the file already exists, the id is already in use, or the path is not an indexable root-relative .md path. Edit prose afterward with the write/edit tools.", promptSnippet: "spec_create — create a new spec file with scaffolded frontmatter (id/type/title/links) and heading stubs.", parameters, async execute(_callId, params, _signal, _onUpdate, ctx) { + const resolved = resolveSpecPath(ctx.cwd, params.path); + if ("error" in resolved) return errorResult(resolved.error); + const { rel, abs } = resolved; + const index = getIndex(ctx.cwd); - const abs = index.absPath(params.path); - if (existsSync(abs)) return errorResult(`File already exists: ${params.path}`); + if (existsSync(abs)) return errorResult(`File already exists: ${rel}`); if (index.graph().nodes.has(params.id)) { return errorResult(`Spec id "${params.id}" is already in use.`); } @@ -63,16 +70,18 @@ export function registerSpecCreate(pi: ExtensionAPI): void { if (params.tags?.length) fm[FIELDS.tags] = params.tags; const content = `${serializeFrontmatter(fm)}\n${scaffoldBody(params.type)}`; + if (!isSpec(parseFile(content).frontmatter)) { + return errorResult( + `Refusing to write ${rel}: the frontmatter would not be a spec (id and type must be non-empty).`, + ); + } try { mkdirSync(dirname(abs), { recursive: true }); - writeFileSync(abs, content, "utf8"); + writeFileSync(abs, content, { encoding: "utf8", flag: "wx" }); } catch (err) { - return errorResult(`Failed to write ${params.path}: ${(err as Error).message}`); + return errorResult(`Failed to write ${rel}: ${(err as Error).message}`); } - return textResult(`Created ${params.path} (id: ${params.id}).`, { - path: params.path, - id: params.id, - }); + return textResult(`Created ${rel} (id: ${params.id}).`, { path: rel, id: params.id }); }, }); } diff --git a/packages/spec-graph/tools/grep.ts b/packages/spec-graph/tools/grep.ts index 50ddaf34b..dbf44990a 100644 --- a/packages/spec-graph/tools/grep.ts +++ b/packages/spec-graph/tools/grep.ts @@ -1,6 +1,6 @@ import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; import { Type } from "typebox"; -import { type GrepResult, grepSpecs } from "../core/index.ts"; +import { DEFAULT_GREP_LIMIT, type GrepResult, grepSpecs } from "../core/index.ts"; import { errorResult, getIndex, textResult } from "./shared.ts"; const parameters = Type.Object({ @@ -19,7 +19,9 @@ const parameters = Type.Object({ dependsOn: Type.Optional( Type.String({ description: "Only search specs that depend-on this id." }), ), - limit: Type.Optional(Type.Number({ description: "Max matches to return (default: 200)." })), + limit: Type.Optional( + Type.Number({ description: `Max matches to return (default: ${DEFAULT_GREP_LIMIT}).` }), + ), }); export function registerSpecGrep(pi: ExtensionAPI): void { diff --git a/packages/spec-graph/tools/tools.test.ts b/packages/spec-graph/tools/tools.test.ts index e148c63cd..cd2664fed 100644 --- a/packages/spec-graph/tools/tools.test.ts +++ b/packages/spec-graph/tools/tools.test.ts @@ -1,5 +1,13 @@ import { expect, test } from "bun:test"; -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { @@ -8,7 +16,13 @@ import type { ExtensionContext, ToolDefinition, } from "@earendil-works/pi-coding-agent"; -import { LINK_KINDS, SLICE_DIRECTIONS, SPEC_STATUSES, SPEC_TYPES } from "../core/index.ts"; +import { + LINK_KINDS, + resolveSpecPath, + SLICE_DIRECTIONS, + SPEC_STATUSES, + SPEC_TYPES, +} from "../core/index.ts"; import { registerSpecTools } from "./index.ts"; const tools = new Map(); @@ -176,6 +190,83 @@ test("spec_create rejects a duplicate id and an existing path", async () => { }); }); +test("spec_create refuses a path the index could never see, and writes nothing", async () => { + await withRoot(async (outer) => { + const root = join(outer, "project"); + mkdirSync(root, { recursive: true }); + const rejected = [ + "../escape.md", + "pkg/../../escape.md", + "/tmp/escape.md", + "notes/spec.txt", + "node_modules/dep/SPEC.md", + "", + ]; + let n = 0; + for (const path of rejected) { + const res = await run( + "spec_create", + { path, id: `id-${n++}`, type: "module-design", title: "T" }, + root, + ); + expect(isError(res)).toBe(true); + const expected = resolveSpecPath(root, path); + expect(text(res)).toContain((expected as { error: string }).error); + } + expect(existsSync(join(outer, "escape.md"))).toBe(false); + expect(existsSync(join(root, "notes/spec.txt"))).toBe(false); + }); +}); + +test("spec_create writes nothing through a symlink, dangling leaf included", async () => { + await withRoot(async (outer) => { + const root = join(outer, "project"); + mkdirSync(join(outer, "elsewhere"), { recursive: true }); + mkdirSync(root, { recursive: true }); + symlinkSync(join(outer, "elsewhere"), join(root, "link"), "dir"); + symlinkSync(join(outer, "leaf.md"), join(root, "leaf.md")); + + for (const path of ["link/evil.md", "leaf.md"]) { + const res = await run( + "spec_create", + { path, id: `evil-${path}`, type: "module-design", title: "E" }, + root, + ); + expect(isError(res)).toBe(true); + } + expect(existsSync(join(outer, "elsewhere", "evil.md"))).toBe(false); + expect(existsSync(join(outer, "leaf.md"))).toBe(false); + }); +}); + +test("spec_create reports the canonical path the index will produce, not the caller's spelling", async () => { + await withRoot(async (root) => { + const res = await run( + "spec_create", + { path: "./pkg/sub/../SPEC.md", id: "pkg", type: "module-design", title: "P" }, + root, + ); + expect(isError(res)).toBe(false); + expect(res.details).toMatchObject({ path: "pkg/SPEC.md" }); + expect(text(res)).toContain("pkg/SPEC.md"); + + const got = await run("spec_get", { id: "pkg" }, root); + expect(text(got)).toContain("path: pkg/SPEC.md"); + }); +}); + +test("spec_create refuses params that would write a file born a non-spec", async () => { + await withRoot(async (root) => { + const res = await run( + "spec_create", + { path: "blank.md", id: "", type: "module-design", title: "T" }, + root, + ); + expect(isError(res)).toBe(true); + expect(existsSync(join(root, "blank.md"))).toBe(false); + }); +}); + test("spec_graph returns a bounded subtree; spec_grep searches within specs + metadata filter", async () => { await withRoot(async (root) => { await run( From b708d14d826dce80b8d69d48584343c8e87c7190 Mon Sep 17 00:00:00 2001 From: VanishJr <91227368+VanishJr@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:09:48 +0200 Subject: [PATCH 2/3] fix(spec-graph): refuse an ignored directory in every spelling --- packages/spec-graph/SPEC.md | 4 +- packages/spec-graph/core/SPEC.md | 56 ++++++- packages/spec-graph/core/core.test.ts | 210 +++++++++++++++++++++++- packages/spec-graph/core/store.ts | 123 +++++++++++--- packages/spec-graph/tools/tools.test.ts | 29 ++++ 5 files changed, 387 insertions(+), 35 deletions(-) diff --git a/packages/spec-graph/SPEC.md b/packages/spec-graph/SPEC.md index 6a35a6a7c..33bf5e330 100644 --- a/packages/spec-graph/SPEC.md +++ b/packages/spec-graph/SPEC.md @@ -46,8 +46,8 @@ own boundary and leaves. ## Derived read index The filesystem is the source of truth; the model the tools read is **derived, in-memory, read-only, and -revalidated on demand**. Each read re-globs the spec set in a name-sorted walk (ignoring -`node_modules`/`.git`/`dist`/`build`) and revalidates every file by `(mtimeMs, size)`: unchanged files +revalidated on demand**. Each read re-runs the **glob** — a name-sorted traversal that ignores +`node_modules`/`.git`/`dist`/`build` — and revalidates every file by `(mtimeMs, size)`: unchanged files reuse their cached parse, changed/new files are re-parsed, vanished files are evicted, and the derived graph is rebuilt only when the spec set actually changed. So specs added, deleted, or edited from any source — including pi's normal `write`/`edit` — are always current, while redundant re-parse/rebuild is diff --git a/packages/spec-graph/core/SPEC.md b/packages/spec-graph/core/SPEC.md index 93790a696..69eb77952 100644 --- a/packages/spec-graph/core/SPEC.md +++ b/packages/spec-graph/core/SPEC.md @@ -20,7 +20,10 @@ grep with metadata filters, bounded graph slices, and structural validation. Imp - **Owns:** everything above. The filesystem is the source of truth; the model is derived, in-memory, and read-only — the revalidation mechanism lives in `module-spec-graph` (*Derived read index*). - **Public surface:** the `index.ts` **barrel**. `tools/` imports the model only through it, never a leaf - file directly. + file directly. The barrel binds *consumers*; the module's own test (`core/core.test.ts`) may import a + leaf directly to reach helpers that are deliberately not public — the pure segment resolver and the + glob's order comparator in `store.ts`, which pin `resolveSpecPath` and the glob order without touching + a filesystem. - **Allowed deps:** `yaml`; Node built-ins. - **Forbidden:** any `@earendil-works/*` (this is what keeps `core/` isolated and unit-testable) and any `@thinkrail/*` package. @@ -43,20 +46,61 @@ Acyclic and one-way: `parse` is the root, `graph` builds on it, and `query`/`val - No `@earendil-works/*` import anywhere under `core/`. - `buildGraph` is pure (same input → same output); the index revalidates each file by `(mtimeMs, size)`, memoizes the graph, and never serves a stale one. -- The glob sorts each directory's entries by their **NFC-normalized** name, so the spec order — and - therefore which file wins a duplicate `id` — is the same on every filesystem, not whatever order - `readdir` happened to return, and not a different answer on a filesystem that hands back decomposed - (NFD) names. +- The glob filters each directory's entries down to the traversal candidates **first** — directories that + are not ignored, plus `.md` files — and only then normalizes and sorts them, so a directory full of + irrelevant entries costs the per-read scan nothing beyond the pass it discards them in. +- That order is **total**: candidates compare by their **NFC-normalized** name and tie-break on the raw + name by code unit. So the spec order — and therefore which file wins a duplicate `id` — is the same on + every filesystem: not whatever order `readdir` happened to return, not a different answer on a + filesystem that hands back decomposed (NFD) names, and not `readdir`'s order again for two canonically + equivalent spellings (precomposed `é` vs `e\u0301`), which share one NFC key and would otherwise be + left wherever a stable sort found them. + Directories and `.md` files stay in **one** candidate list, so a subdirectory is descended in its place + among its sibling files rather than before or after all of them, and the spec sequence is name-ordered + end to end. The filter-then-sort rewrite **preserves** that, it did not introduce it; the test exists + because two separate lists are the obvious shape to reach for and would silently move the + duplicate-`id` winner. - On a duplicate `id`, the first file in that order wins the node slot; the duplicate set is recorded for `validate`. - `resolveSpecPath` is the single answer to "could the index ever see this path?" and it answers with the **canonical relative path**, so a caller can never report an identity the index will not produce. It requires: root-relative, inside the root, `.md`, outside the ignored dirs, an existing root, and no symlink at any component beneath the root — checked with `lstat` per component, dangling links included, - because the walk never descends a symlink. That last rule is what a string check and a `realpath` + because the glob never descends a symlink. That last rule is what a string check and a `realpath` comparison both miss: a link is rejected whether it leaves the project, lands in an ignored directory, or points back at an indexed one, since in every case the file it creates is invisible to every other spec tool. +- `resolveSpecPath` canonicalizes each component to its **on-disk spelling** before it judges or reports + it. A component whose bytes already match an entry of its parent directory is taken as written; a + component that resolves on this filesystem *without* matching any entry byte-for-byte — a + case-insensitive or Unicode-folding filesystem — becomes the one parent entry equal to it under + `normalize("NFC").toLowerCase()`. Zero or two such entries is an **error**: it fails closed rather than + guess an identity. Canonicalization stops at the first component that does not resolve, and the rest + keeps the caller's spelling, because on a case-sensitive filesystem a new `Docs/` beside an existing + `docs/` really is a new directory the glob will see. `rel` and `abs` are assembled from those canonical + segments, never from the lexical spelling. A lexical `rel` is what let `NODE_MODULES/SPEC.md` report an + identity the glob would never produce on a case-insensitive filesystem, where the write landed in + `node_modules/`. The `.md` rule is judged twice, on the caller's string and again on the canonical + `rel`, because a leaf that canonicalizes to `SPEC.MD` is a file the byte-exact glob never indexes. + Existence is probed with `lstat`, so a dangling symlink counts as resolving and still reaches the + symlink rejection above. + Canonicalization reads the **parent's** listing, so every parent that exists is listed — not only the + ones whose component exists — and a parent that exists but cannot be listed is an **error**, never an + empty listing. That `readdir` is exactly the call the glob makes there: a directory the resolver cannot + list is one the glob abandons, so a spec written under it would be invisible for the same reason a + `node_modules` one is. +- **The write path over-refuses; the read path stays exact.** `resolveSpecPath` matches `IGNORED_DIRS` + on the `normalize("NFC").toLowerCase()` fold of each component, so it refuses `NODE_MODULES/SPEC.md` + in every spelling; the glob keeps matching the byte-exact `readdir` name. The two mistakes are not the + same size. A resolver that refuses too much answers the caller with a reason it can act on; a glob that + skips too much drops a directory a person really named `Build/` out of the index with no sign at all. + Canonicalization alone did not reach the write path, because it only speaks for a component that + already exists: on a project whose `node_modules` was not installed yet, `NODE_MODULES/SPEC.md` + resolved, created the real `node_modules` on a case-insensitive filesystem, and handed the glob every + dependency under it to index. Folding the glob as well bought nothing against that — an installed + `node_modules` is lowercase on disk and the exact check already skips it — and it cost the `Build/` + case, so the glob stays exact. What the fold does not reach at all: a spec written into `NODE_MODULES/` + by pi's own `write` never passes through `resolveSpecPath`. - `SpecNode.type` stays `string`: the read model indexes whatever is on disk, so it tolerates any `type`; the `SPEC_TYPES` vocabulary constrains only the `spec_create` authoring surface, never the graph. - Finite vocabularies (`SPEC_TYPES`, `SPEC_STATUSES`, `SLICE_DIRECTIONS`, `LINK_KINDS`, `IDENTITY_FIELDS`) diff --git a/packages/spec-graph/core/core.test.ts b/packages/spec-graph/core/core.test.ts index 2977b7679..8d94988f5 100644 --- a/packages/spec-graph/core/core.test.ts +++ b/packages/spec-graph/core/core.test.ts @@ -1,5 +1,14 @@ import { expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -27,6 +36,59 @@ import { updateFrontmatterText, validateGraph, } from "./index.ts"; +import { + compareWalkEntries, + resolvePathSegment, + type SegmentResolution, + toWalkEntry, +} from "./store.ts"; + +function probeFilesystem(): { + foldsCase: boolean; + foldsUnicode: boolean; + deniesListing: boolean; +} { + const probe = mkdtempSync(join(tmpdir(), "spec-probe-")); + try { + for (const name of ["CaseProbe", "caf\u00e9", "cafe\u0301"]) { + mkdirSync(join(probe, name), { recursive: true }); + } + const foldsCase = existsSync(join(probe, "caseprobe")); + const foldsUnicode = readdirSync(probe).length < 3; + return { + foldsCase, + foldsUnicode, + deniesListing: withUnlistableDirectory(probe, refusesListing), + }; + } finally { + rmSync(probe, { recursive: true, force: true }); + } +} + +function withUnlistableDirectory(parent: string, fn: (dir: string) => T): T { + const dir = join(parent, "unlistable"); + mkdirSync(dir, { recursive: true }); + chmodSync(dir, 0o311); + try { + return fn(dir); + } finally { + chmodSync(dir, 0o755); + } +} + +function refusesListing(dir: string): boolean { + try { + readdirSync(dir); + return false; + } catch { + return true; + } +} + +const { foldsCase, foldsUnicode, deniesListing } = probeFilesystem(); +const caseFolding = test.skipIf(!foldsCase); +const spellingPreserving = test.skipIf(foldsUnicode); +const listingDenied = test.skipIf(!deniesListing); test("the finite-vocabulary tuples carry exactly their members", () => { expect([...IDENTITY_FIELDS]).toEqual(["id", "type"]); @@ -499,17 +561,20 @@ function withProject(fn: (root: string, outer: string) => void): void { }); } -const ok = (r: ReturnType): string => { +const ok = (r: ReturnType, root: string): string => { if ("error" in r) throw new Error(`expected a resolved path, got: ${r.error}`); + expect(r.abs).toBe(join(root, ...r.rel.split("/"))); return r.rel; }; test("resolveSpecPath returns the canonical relative path the index would report", () => { withProject((root) => { - expect(ok(resolveSpecPath(root, "SPEC.md"))).toBe("SPEC.md"); - expect(ok(resolveSpecPath(root, "packages/core/SPEC.md"))).toBe("packages/core/SPEC.md"); - expect(ok(resolveSpecPath(root, "./packages/core/SPEC.md"))).toBe("packages/core/SPEC.md"); - expect(ok(resolveSpecPath(root, "pkg/./sub/../SPEC.md"))).toBe("pkg/SPEC.md"); + expect(ok(resolveSpecPath(root, "SPEC.md"), root)).toBe("SPEC.md"); + expect(ok(resolveSpecPath(root, "packages/core/SPEC.md"), root)).toBe("packages/core/SPEC.md"); + expect(ok(resolveSpecPath(root, "./packages/core/SPEC.md"), root)).toBe( + "packages/core/SPEC.md", + ); + expect(ok(resolveSpecPath(root, "pkg/./sub/../SPEC.md"), root)).toBe("pkg/SPEC.md"); expect(SPEC_FILE_EXTENSION).toBe(".md"); }); }); @@ -544,7 +609,7 @@ test("resolveSpecPath rejects a symlinked directory even when it points back ins expect(resolveSpecPath(root, "gone/evil.md")).toHaveProperty("error"); expect(resolveSpecPath(root, "docs/ghost.md")).toHaveProperty("error"); expect(resolveSpecPath(root, "alias/SPEC.md")).toHaveProperty("error"); - expect(ok(resolveSpecPath(root, "real/SPEC.md"))).toBe("real/SPEC.md"); + expect(ok(resolveSpecPath(root, "real/SPEC.md"), root)).toBe("real/SPEC.md"); }); }); @@ -559,12 +624,73 @@ test("resolveSpecPath rejects a symlink at the leaf, dangling or not", () => { }); }); +const resolvedName = (r: SegmentResolution): string => { + if ("error" in r) throw new Error(`expected a resolved segment, got: ${r.error}`); + return r.name; +}; + +test("resolvePathSegment canonicalizes to the on-disk spelling and never guesses", () => { + expect(resolvedName(resolvePathSegment(["docs", "pkg"], "docs", true))).toBe("docs"); + expect(resolvedName(resolvePathSegment(["Docs", "pkg"], "docs", true))).toBe("Docs"); + expect(resolvedName(resolvePathSegment(["caf\u00e9"], "cafe\u0301", true))).toBe("caf\u00e9"); + expect(resolvedName(resolvePathSegment(["docs"], "Docs", false))).toBe("Docs"); + expect(resolvedName(resolvePathSegment(["docs"], "SPEC.md", false))).toBe("SPEC.md"); + + expect(resolvePathSegment(["Docs", "docs"], "DOCS", true)).toHaveProperty("error"); + expect(resolvePathSegment(["pkg"], "docs", true)).toHaveProperty("error"); +}); + +test("resolvePathSegment refuses an ignored directory in any spelling, existing or not", () => { + expect(resolvePathSegment(["node_modules"], "node_modules", true)).toHaveProperty("error"); + expect(resolvePathSegment(["node_modules"], "NODE_MODULES", true)).toHaveProperty("error"); + for (const spelling of [ + "node_modules", + "NODE_MODULES", + "Node_Modules", + "DIST", + "BUILD", + ".GIT", + ]) { + expect(resolvePathSegment([], spelling, false)).toHaveProperty("error"); + } +}); + +caseFolding("resolveSpecPath resolves a case alias to the spelling the index will walk", () => { + withProject((root) => { + mkdirSync(join(root, "node_modules"), { recursive: true }); + mkdirSync(join(root, "docs"), { recursive: true }); + + expect(resolveSpecPath(root, "NODE_MODULES/SPEC.md")).toHaveProperty("error"); + expect(ok(resolveSpecPath(root, "Docs/SPEC.md"), root)).toBe("docs/SPEC.md"); + expect(ok(resolveSpecPath(root, "Docs/Nested/SPEC.md"), root)).toBe("docs/Nested/SPEC.md"); + + writeFileSync( + join(root, "docs", "SPEC.MD"), + "---\nid: shouty\ntype: module-design\ntitle: S\n---\n", + ); + expect(resolveSpecPath(root, "docs/spec.md")).toHaveProperty("error"); + }); +}); + test("resolveSpecPath fails closed when the root does not exist", () => { expect(resolveSpecPath(join(tmpdir(), "spec-index-definitely-absent"), "SPEC.md")).toHaveProperty( "error", ); }); +listingDenied( + "resolveSpecPath fails closed when a parent directory exists but cannot be listed", + () => { + withProject((root) => { + withUnlistableDirectory(root, () => { + expect(resolveSpecPath(root, "unlistable/SPEC.md")).toHaveProperty("error"); + expect(resolveSpecPath(root, "unlistable/nested/SPEC.md")).toHaveProperty("error"); + }); + expect(ok(resolveSpecPath(root, "unlistable/SPEC.md"), root)).toBe("unlistable/SPEC.md"); + }); + }, +); + test("SpecIndex walks in a stable order, so a duplicate id resolves the same everywhere", () => { withIndexRoot((root) => { for (const name of ["z-later", "a-earlier", "m-middle"]) { @@ -583,3 +709,73 @@ test("SpecIndex walks in a stable order, so a duplicate id resolves the same eve ]); }); }); + +test("SpecIndex walks a directory in its place among its sibling files, not before or after them", () => { + withIndexRoot((root) => { + const spec = "---\nid: dup\ntype: module-design\ntitle: Dup\n---\n"; + mkdirSync(join(root, "b-dir"), { recursive: true }); + writeFileSync(join(root, "a.md"), spec); + writeFileSync(join(root, "b-dir", "SPEC.md"), spec); + writeFileSync(join(root, "c.md"), spec); + + const graph = new SpecIndex(root).graph(); + expect(graph.duplicateIds.get("dup")).toEqual(["a.md", "b-dir/SPEC.md", "c.md"]); + expect(graph.nodes.get("dup")?.path).toBe("a.md"); + }); +}); + +test("the glob keeps the byte-exact ignored rule the resolver deliberately over-refuses", () => { + withIndexRoot((root) => { + mkdirSync(join(root, "NODE_MODULES"), { recursive: true }); + writeFileSync( + join(root, "NODE_MODULES", "SPEC.md"), + "---\nid: shouted\ntype: module-design\ntitle: S\n---\n", + ); + + expect(resolveSpecPath(root, "NODE_MODULES/SPEC.md")).toHaveProperty("error"); + expect(new SpecIndex(root).graph().nodes.has("shouted")).toBe(true); + }); +}); + +const walkOrder = (names: readonly string[]): string[] => + names + .map((name) => toWalkEntry(name, false)) + .sort(compareWalkEntries) + .map((entry) => entry.name); + +test("the walk order compares NFC-normalized names, not raw code units", () => { + expect(walkOrder(["A\u0308pfel", "Banana"])).toEqual(["Banana", "A\u0308pfel"]); + expect(walkOrder(["Banana", "A\u0308pfel"])).toEqual(["Banana", "A\u0308pfel"]); + expect(walkOrder(["\u00c4pfel", "Banana"])).toEqual(["Banana", "\u00c4pfel"]); +}); + +test("the walk order is total, so canonically equivalent names sort the same either way round", () => { + const names = ["caf\u00e9", "cafe\u0301", "zebra", "\u00c4pfel", "A\u0308pfel", "apple"]; + expect(walkOrder(names)).toEqual(walkOrder([...names].reverse())); + expect(walkOrder(["zebra", "caf\u00e9", "apple", "cafe\u0301"])).toEqual([ + "apple", + "cafe\u0301", + "caf\u00e9", + "zebra", + ]); +}); + +spellingPreserving( + "SpecIndex resolves a duplicate id the same way for canonically equivalent directory names", + () => { + withIndexRoot((root) => { + const spellings = ["caf\u00e9", "cafe\u0301"]; + for (const name of spellings) { + mkdirSync(join(root, name), { recursive: true }); + writeFileSync( + join(root, name, "SPEC.md"), + "---\nid: dup\ntype: module-design\ntitle: Dup\n---\n", + ); + } + const expected = ["cafe\u0301/SPEC.md", "caf\u00e9/SPEC.md"]; + const graph = new SpecIndex(root).graph(); + expect(graph.duplicateIds.get("dup")).toEqual(expected); + expect(graph.nodes.get("dup")?.path).toBe(expected[0]); + }); + }, +); diff --git a/packages/spec-graph/core/store.ts b/packages/spec-graph/core/store.ts index a65e41f76..9bb662fc8 100644 --- a/packages/spec-graph/core/store.ts +++ b/packages/spec-graph/core/store.ts @@ -16,6 +16,60 @@ function isSymlink(target: string): boolean { } } +function resolves(target: string): boolean { + try { + lstatSync(target); + return true; + } catch { + return false; + } +} + +function directoryEntries(dir: string): string[] | null { + try { + return readdirSync(dir); + } catch { + return null; + } +} + +function fold(name: string): string { + return name.normalize("NFC").toLowerCase(); +} + +function isIgnoredName(name: string): boolean { + return IGNORED_DIRS.has(fold(name)); +} + +export type SegmentResolution = { name: string } | { error: string }; + +export function resolvePathSegment( + entries: readonly string[], + segment: string, + exists: boolean, +): SegmentResolution { + let name = segment; + if (exists && !entries.includes(segment)) { + const folded = fold(segment); + const [only, ...rest] = entries.filter((entry) => fold(entry) === folded); + if (only === undefined) { + return { + error: `Path component "${segment}" resolves to no entry its parent directory lists`, + }; + } + if (rest.length > 0) { + return { + error: `Path component "${segment}" matches more than one entry on this filesystem ("${only}", "${rest.join('", "')}")`, + }; + } + name = only; + } + if (isIgnoredName(name)) { + return { error: `Path is inside an ignored directory ("${name}") and would not be indexed` }; + } + return { name }; +} + export type SpecPathResolution = { rel: string; abs: string } | { error: string }; export function resolveSpecPath(root: string, path: string): SpecPathResolution { @@ -25,26 +79,56 @@ export function resolveSpecPath(root: string, path: string): SpecPathResolution return { error: `Spec files must end in ${SPEC_FILE_EXTENSION}: ${path}` }; } - const lexical = normalize(path); - const segments = lexical.split(sep); + const segments = normalize(path).split(sep); if (segments[0] === "..") return { error: `Path must stay inside the project root: ${path}` }; - const ignored = segments.find((segment) => IGNORED_DIRS.has(segment)); - if (ignored !== undefined) { - return { - error: `Path is inside an ignored directory ("${ignored}") and would not be indexed: ${path}`, - }; - } if (!existsSync(root)) return { error: `Project root does not exist: ${root}` }; let walked = root; + let walkedExists = true; + const canonical: string[] = []; for (const segment of segments) { - walked = join(walked, segment); + let entries: readonly string[] = []; + if (walkedExists) { + const listed = directoryEntries(walked); + if (listed === null) { + return { + error: `Path passes through a directory the index cannot list: ${path}`, + }; + } + entries = listed; + } + const exists: boolean = walkedExists && resolves(join(walked, segment)); + const resolution = resolvePathSegment(entries, segment, exists); + if ("error" in resolution) return { error: `${resolution.error}: ${path}` }; + walked = join(walked, resolution.name); if (isSymlink(walked)) { return { error: `Path passes through a symlink, which the index never follows: ${path}` }; } + canonical.push(resolution.name); + walkedExists = exists; } - return { rel: lexical.split(sep).join("/"), abs: join(root, lexical) }; + const rel = canonical.join("/"); + if (!rel.endsWith(SPEC_FILE_EXTENSION)) { + return { error: `Spec files must end in ${SPEC_FILE_EXTENSION}: ${path}` }; + } + return { rel, abs: walked }; +} + +export interface WalkEntry { + readonly name: string; + readonly key: string; + readonly directory: boolean; +} + +export function toWalkEntry(name: string, directory: boolean): WalkEntry { + return { name, key: name.normalize("NFC"), directory }; +} + +export function compareWalkEntries(a: WalkEntry, b: WalkEntry): number { + if (a.key !== b.key) return a.key < b.key ? -1 : 1; + if (a.name !== b.name) return a.name < b.name ? -1 : 1; + return 0; } export interface SpecFileRecord { @@ -86,21 +170,20 @@ export class SpecIndex { } catch { return; } - const order = new Map(dirents.map((d) => [d.name, d.name.normalize("NFC")])); - 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; - }); + const candidates: WalkEntry[] = []; for (const dirent of dirents) { - const abs = join(dir, dirent.name); if (dirent.isDirectory()) { - if (IGNORED_DIRS.has(dirent.name)) continue; - yield* this.walk(abs); + if (!IGNORED_DIRS.has(dirent.name)) candidates.push(toWalkEntry(dirent.name, true)); } else if (dirent.isFile() && dirent.name.endsWith(SPEC_FILE_EXTENSION)) { - yield abs; + candidates.push(toWalkEntry(dirent.name, false)); } } + candidates.sort(compareWalkEntries); + for (const candidate of candidates) { + const abs = join(dir, candidate.name); + if (candidate.directory) yield* this.walk(abs); + else yield abs; + } } private scan(): SpecFileRecord[] { diff --git a/packages/spec-graph/tools/tools.test.ts b/packages/spec-graph/tools/tools.test.ts index cd2664fed..32c0e7c94 100644 --- a/packages/spec-graph/tools/tools.test.ts +++ b/packages/spec-graph/tools/tools.test.ts @@ -25,6 +25,18 @@ import { } from "../core/index.ts"; import { registerSpecTools } from "./index.ts"; +function foldsCase(): boolean { + const probe = mkdtempSync(join(tmpdir(), "spec-probe-")); + try { + mkdirSync(join(probe, "CaseProbe"), { recursive: true }); + return existsSync(join(probe, "caseprobe")); + } finally { + rmSync(probe, { recursive: true, force: true }); + } +} + +const caseFolding = test.skipIf(!foldsCase()); + const tools = new Map(); registerSpecTools({ registerTool(tool: ToolDefinition) { @@ -218,6 +230,23 @@ test("spec_create refuses a path the index could never see, and writes nothing", }); }); +caseFolding( + "spec_create refuses a case alias of an ignored directory, and writes nothing", + async () => { + await withRoot(async (root) => { + mkdirSync(join(root, "node_modules"), { recursive: true }); + + const res = await run( + "spec_create", + { path: "NODE_MODULES/SPEC.md", id: "aliased", type: "module-design", title: "A" }, + root, + ); + expect(isError(res)).toBe(true); + expect(existsSync(join(root, "node_modules", "SPEC.md"))).toBe(false); + }); + }, +); + test("spec_create writes nothing through a symlink, dangling leaf included", async () => { await withRoot(async (outer) => { const root = join(outer, "project"); From 03047117aa959825e512b986d4e30bb04ffbd21c Mon Sep 17 00:00:00 2001 From: rsolmano Date: Wed, 2 Sep 2026 14:16:28 +0200 Subject: [PATCH 3/3] fix(spec-graph): close Windows path escapes --- .github/workflows/ci.yml | 1 + packages/spec-graph/core/SPEC.md | 21 ++++++--- packages/spec-graph/core/core.test.ts | 63 ++++++++++++++++++++++++- packages/spec-graph/core/store.ts | 33 ++++++++++++- packages/spec-graph/tools/tools.test.ts | 16 +++++++ 5 files changed, 124 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7cc37cc72..74ef9078b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,6 +131,7 @@ jobs: key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }} restore-keys: ${{ runner.os }}-bun- - run: bun install --frozen-lockfile + - run: bun test packages/spec-graph - run: bun run build:binary - run: bun run smoke:binary diff --git a/packages/spec-graph/core/SPEC.md b/packages/spec-graph/core/SPEC.md index 69eb77952..b7ba6ffb7 100644 --- a/packages/spec-graph/core/SPEC.md +++ b/packages/spec-graph/core/SPEC.md @@ -21,9 +21,9 @@ grep with metadata filters, bounded graph slices, and structural validation. Imp read-only — the revalidation mechanism lives in `module-spec-graph` (*Derived read index*). - **Public surface:** the `index.ts` **barrel**. `tools/` imports the model only through it, never a leaf file directly. The barrel binds *consumers*; the module's own test (`core/core.test.ts`) may import a - leaf directly to reach helpers that are deliberately not public — the pure segment resolver and the - glob's order comparator in `store.ts`, which pin `resolveSpecPath` and the glob order without touching - a filesystem. + leaf directly to reach helpers that are deliberately not public — the pure path guards, segment + resolver, and glob order comparator in `store.ts`, which pin `resolveSpecPath` and the glob order + without touching a filesystem. - **Allowed deps:** `yaml`; Node built-ins. - **Forbidden:** any `@earendil-works/*` (this is what keeps `core/` isolated and unit-testable) and any `@thinkrail/*` package. @@ -66,10 +66,17 @@ Acyclic and one-way: `parse` is the root, `graph` builds on it, and `query`/`val **canonical relative path**, so a caller can never report an identity the index will not produce. It requires: root-relative, inside the root, `.md`, outside the ignored dirs, an existing root, and no symlink at any component beneath the root — checked with `lstat` per component, dangling links included, - because the glob never descends a symlink. That last rule is what a string check and a `realpath` - comparison both miss: a link is rejected whether it leaves the project, lands in an ignored directory, - or points back at an indexed one, since in every case the file it creates is invisible to every other - spec tool. + because the glob never descends a symlink. On Windows, colon-bearing paths are refused before + normalization: this closes both drive-relative paths (`C:..\\outside.md`, which `isAbsolute` does not + recognize) and NTFS alternate data streams (`file:SPEC.md`, which `readdir` cannot see). Every `..` + segment that survives normalization is also refused, and the canonical absolute result must pass a + final `relative(root, target)` containment check. These overlapping gates are deliberate: path syntax, + normalization, and component-wise canonicalization must not be able to undermine each other. Portable + `win32` arithmetic tests pin the escape on every host; the Windows CI lane also runs the package tests + natively, including the `resolveSpecPath` and `spec_create` integrations. The symlink rule is what a + string check and a `realpath` comparison both miss: a link is rejected whether it + leaves the project, lands in an ignored directory, or points back at an indexed one, since in every case + the file it creates is invisible to every other spec tool. - `resolveSpecPath` canonicalizes each component to its **on-disk spelling** before it judges or reports it. A component whose bytes already match an entry of its parent directory is taken as written; a component that resolves on this filesystem *without* matching any entry byte-for-byte — a diff --git a/packages/spec-graph/core/core.test.ts b/packages/spec-graph/core/core.test.ts index 8d94988f5..dad3ba716 100644 --- a/packages/spec-graph/core/core.test.ts +++ b/packages/spec-graph/core/core.test.ts @@ -10,7 +10,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, win32 } from "node:path"; import { buildGraph, DEFAULT_GREP_LIMIT, @@ -38,6 +38,8 @@ import { } from "./index.ts"; import { compareWalkEntries, + hasWindowsNamespaceSyntax, + isPathInsideRoot, resolvePathSegment, type SegmentResolution, toWalkEntry, @@ -89,6 +91,8 @@ const { foldsCase, foldsUnicode, deniesListing } = probeFilesystem(); const caseFolding = test.skipIf(!foldsCase); const spellingPreserving = test.skipIf(foldsUnicode); const listingDenied = test.skipIf(!deniesListing); +const windowsOnly = test.skipIf(process.platform !== "win32"); +const nonWindows = test.skipIf(process.platform === "win32"); test("the finite-vocabulary tuples carry exactly their members", () => { expect([...IDENTITY_FIELDS]).toEqual(["id", "type"]); @@ -595,6 +599,63 @@ test("resolveSpecPath rejects every path the index could never see", () => { }); }); +test("Windows drive and stream syntax is never a root-relative spec path", () => { + for (const path of [ + "C:SPEC.md", + "C:..\\..\\outside.md", + "notes.txt:SPEC.md", + "dir\\notes.txt:SPEC.md", + "\\rooted\\SPEC.md", + "\\\\server\\share\\SPEC.md", + ]) { + expect(hasWindowsNamespaceSyntax(path)).toBe(true); + } + for (const path of ["SPEC.md", "dir\\SPEC.md"]) { + expect(hasWindowsNamespaceSyntax(path)).toBe(false); + } +}); + +test("the final containment gate catches Windows drive-relative traversal after joining", () => { + const root = "C:\\repo"; + const attack = "C:..\\..\\..\\outside.md"; + let target = root; + for (const segment of win32.normalize(attack).split(win32.sep)) { + target = win32.join(target, segment); + } + + expect(win32.isAbsolute(attack)).toBe(false); + expect(target).toBe("C:\\outside.md"); + expect(isPathInsideRoot(root, "C:\\repo\\docs\\SPEC.md", win32)).toBe(true); + expect(isPathInsideRoot(root, target, win32)).toBe(false); + expect(isPathInsideRoot(root, "D:\\outside.md", win32)).toBe(false); +}); + +windowsOnly("resolveSpecPath rejects Windows drive-relative and stream paths", () => { + withProject((root) => { + for (const path of ["C:..\\..\\outside.md", "notes.txt:SPEC.md"]) { + expect(resolveSpecPath(root, path)).toHaveProperty("error"); + } + }); +}); + +nonWindows("Windows-only syntax keeps its literal index identity on POSIX", () => { + withProject((root) => { + const names = ["notes.txt:SPEC.md", "dir\\SPEC.md"]; + for (const [index, path] of names.entries()) { + const resolved = resolveSpecPath(root, path); + if ("error" in resolved) throw new Error(`expected a resolved path, got: ${resolved.error}`); + writeFileSync( + resolved.abs, + `---\nid: literal-${index}\ntype: module-design\ntitle: Literal\n---\n`, + ); + } + + const graph = new SpecIndex(root).graph(); + expect(graph.nodes.get("literal-0")?.path).toBe(names[0]); + expect(graph.nodes.get("literal-1")?.path).toBe(names[1]); + }); +}); + test("resolveSpecPath rejects a symlinked directory even when it points back inside the root", () => { withProject((root, outer) => { mkdirSync(join(root, "real"), { recursive: true }); diff --git a/packages/spec-graph/core/store.ts b/packages/spec-graph/core/store.ts index 9bb662fc8..9224dd78a 100644 --- a/packages/spec-graph/core/store.ts +++ b/packages/spec-graph/core/store.ts @@ -1,5 +1,5 @@ import { existsSync, lstatSync, readdirSync, readFileSync, statSync } from "node:fs"; -import { isAbsolute, join, normalize, relative, sep } from "node:path"; +import { isAbsolute, join, normalize, relative, sep, win32 } from "node:path"; import { buildGraph, type SpecGraph } from "./graph.ts"; import { FIELDS, type Frontmatter, isSpec, parseFile, scalar } from "./parse.ts"; import type { SpecContentEntry } from "./query.ts"; @@ -8,6 +8,27 @@ const IGNORED_DIRS = new Set(["node_modules", ".git", "dist", "build"]); export const SPEC_FILE_EXTENSION = ".md"; +interface PathSemantics { + isAbsolute(path: string): boolean; + relative(from: string, to: string): string; + sep: string; +} + +const NATIVE_PATH_SEMANTICS: PathSemantics = { isAbsolute, relative, sep }; + +export function hasWindowsNamespaceSyntax(path: string): boolean { + return win32.parse(path).root !== "" || path.includes(":"); +} + +export function isPathInsideRoot( + root: string, + target: string, + paths: PathSemantics = NATIVE_PATH_SEMANTICS, +): boolean { + const rel = paths.relative(root, target); + return rel !== ".." && !rel.startsWith(`..${paths.sep}`) && !paths.isAbsolute(rel); +} + function isSymlink(target: string): boolean { try { return lstatSync(target).isSymbolicLink(); @@ -75,12 +96,17 @@ export type SpecPathResolution = { rel: string; abs: string } | { error: string export function resolveSpecPath(root: string, path: string): SpecPathResolution { if (path.trim() === "") return { error: "Path must not be empty." }; if (isAbsolute(path)) return { error: `Path must be root-relative, not absolute: ${path}` }; + if (process.platform === "win32" && hasWindowsNamespaceSyntax(path)) { + return { error: `Path must not use Windows drive or stream syntax: ${path}` }; + } if (!path.endsWith(SPEC_FILE_EXTENSION)) { return { error: `Spec files must end in ${SPEC_FILE_EXTENSION}: ${path}` }; } const segments = normalize(path).split(sep); - if (segments[0] === "..") return { error: `Path must stay inside the project root: ${path}` }; + if (segments.includes("..")) { + return { error: `Path must stay inside the project root: ${path}` }; + } if (!existsSync(root)) return { error: `Project root does not exist: ${root}` }; let walked = root; @@ -112,6 +138,9 @@ export function resolveSpecPath(root: string, path: string): SpecPathResolution if (!rel.endsWith(SPEC_FILE_EXTENSION)) { return { error: `Spec files must end in ${SPEC_FILE_EXTENSION}: ${path}` }; } + if (!isPathInsideRoot(root, walked)) { + return { error: `Path must stay inside the project root: ${path}` }; + } return { rel, abs: walked }; } diff --git a/packages/spec-graph/tools/tools.test.ts b/packages/spec-graph/tools/tools.test.ts index 32c0e7c94..0244bfa05 100644 --- a/packages/spec-graph/tools/tools.test.ts +++ b/packages/spec-graph/tools/tools.test.ts @@ -36,6 +36,7 @@ function foldsCase(): boolean { } const caseFolding = test.skipIf(!foldsCase()); +const windowsOnly = test.skipIf(process.platform !== "win32"); const tools = new Map(); registerSpecTools({ @@ -230,6 +231,21 @@ test("spec_create refuses a path the index could never see, and writes nothing", }); }); +windowsOnly("spec_create refuses Windows drive-relative and stream paths", async () => { + await withRoot(async (root) => { + let n = 0; + for (const path of ["C:..\\..\\outside.md", "notes.txt:SPEC.md"]) { + const result = await run( + "spec_create", + { path, id: `windows-${n++}`, type: "task-spec", title: "Windows" }, + root, + ); + expect(isError(result)).toBe(true); + expect(text(result)).toContain("Windows drive or stream syntax"); + } + }); +}); + caseFolding( "spec_create refuses a case alias of an ignored directory, and writes nothing", async () => {