From dcb5eae3d28a9a3ea44f8244997dc3c9fbe97745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Mon, 24 Aug 2026 19:59:55 -0300 Subject: [PATCH 1/2] feat(review-tutor): analyze Dart and Rust and read one-hop neighbours Add Dart and Rust language tables with language-aware masking, read bounded one-hop neighbour files through Git objects behind a default-off query flag, and pin diff path prefixes so user Git config cannot change snapshot paths. --- packages/review-tutor/src/inputs.ts | 30 +- packages/review-tutor/src/protocol.ts | 24 +- packages/review-tutor/src/server-session.ts | 27 +- packages/review-tutor/src/server.ts | 2 +- .../review-tutor/src/structure-languages.ts | 719 ++++++++++++++++++ .../review-tutor/src/structure-neighbours.ts | 185 +++++ packages/review-tutor/src/structure.ts | 639 ++++++++-------- packages/review-tutor/test/core.test.ts | 34 +- packages/review-tutor/test/structure.test.ts | 610 ++++++++++++++- 9 files changed, 1949 insertions(+), 321 deletions(-) create mode 100644 packages/review-tutor/src/structure-languages.ts create mode 100644 packages/review-tutor/src/structure-neighbours.ts diff --git a/packages/review-tutor/src/inputs.ts b/packages/review-tutor/src/inputs.ts index 8066714..202dca3 100644 --- a/packages/review-tutor/src/inputs.ts +++ b/packages/review-tutor/src/inputs.ts @@ -15,6 +15,13 @@ export type ExecFile = ( const REVISION = /^[A-Za-z0-9][A-Za-z0-9._/~^{}@+-]{0,255}$/; +/** + * Pins the diff format for every local Git read. Without the explicit prefixes, + * `diff.mnemonicPrefix` emits `i/` and `w/` and `diff.noprefix` emits none, so a + * user's Git config would change the paths in every snapshot. + */ +const DIFF_ARGS = ["--no-ext-diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/"]; + function revision(value: string): string { if (value.startsWith("-") || !REVISION.test(value)) { throw new Error( @@ -133,7 +140,7 @@ export async function loadInput( await run( exec, "git", - ["diff", "--no-ext-diff", "--no-color", "--"], + ["diff", ...DIFF_ARGS, "--"], cwd, signal, ), @@ -146,7 +153,7 @@ export async function loadInput( await run( exec, "git", - ["diff", "--cached", "--no-ext-diff", "--no-color", "--"], + ["diff", "--cached", ...DIFF_ARGS, "--"], cwd, signal, ), @@ -160,10 +167,11 @@ export async function loadInput( await run( exec, "git", - ["show", "--no-ext-diff", "--no-color", "--format=fuller", rev, "--"], + ["show", ...DIFF_ARGS, "--format=fuller", rev, "--"], cwd, signal, ), + { revision: rev }, ); } if (request.kind === "range") { @@ -175,15 +183,25 @@ export async function loadInput( await run( exec, "git", - ["diff", "--no-ext-diff", "--no-color", `${from}...${to}`, "--"], + ["diff", ...DIFF_ARGS, `${from}...${to}`, "--"], cwd, signal, ), + { rangeTo: to }, ); } - const pr = request as Extract; - const url = prUrl(pr.url).toString().replace(/\/$/, ""); + return loadPullRequest(request as Extract, cwd, exec, signal); +} + +/** Reads one exact PR head, re-checking the head SHA so a mid-read force-push cannot be mixed in. */ +async function loadPullRequest( + request: Extract, + cwd: string, + exec: ExecFile, + signal?: AbortSignal, +): Promise { + const url = prUrl(request.url).toString().replace(/\/$/, ""); const viewArgs = ["pr", "view", url, "--json", "number,title,url,headRefOid,author"]; const before = JSON.parse(await run(exec, "gh", viewArgs, cwd, signal)) as { number: number; diff --git a/packages/review-tutor/src/protocol.ts b/packages/review-tutor/src/protocol.ts index ee241ab..f9d3c47 100644 --- a/packages/review-tutor/src/protocol.ts +++ b/packages/review-tutor/src/protocol.ts @@ -24,6 +24,14 @@ export const STRUCTURE_LIMITS = { externalNames: 8, } as const; +export const NEIGHBOUR_LIMITS = { + maxFiles: 50, + maxFileBytes: 256 * 1024, + maxTotalBytes: 2 * 1024 * 1024, + timeoutMs: 5000, + maxProbes: 400, +} as const; + export type QuizOutcome = "got_it" | "almost" | "review_again"; export type SourceRequest = @@ -42,6 +50,10 @@ export interface InputSnapshot { content: string; githubUrl?: string; headSha?: string; + /** The exact Git revision a commit snapshot was read from; never parsed back out of `label`. */ + revision?: string; + /** The `to` endpoint of a range snapshot. */ + rangeTo?: string; } export interface ModelChoice { @@ -116,7 +128,8 @@ export interface StructureComparison { export interface StructureFile { path: string; - status: "added" | "removed" | "modified" | "renamed"; + /** `context` marks an unchanged one-hop neighbour, never a changed file. */ + status: "added" | "removed" | "modified" | "renamed" | "context"; renamedFrom?: string; additions: number; deletions: number; @@ -133,7 +146,7 @@ export interface StructureEvidence { export interface StructureEdge { from: string; to: string; - kind: "import" | "reexport" | "require" | "dynamic-import"; + kind: "import" | "reexport" | "require" | "dynamic-import" | "part" | "part-of" | "mod" | "use" | "include"; typeOnly: boolean; status: "added" | "removed" | "modified" | "unchanged"; specifier: string; @@ -153,6 +166,12 @@ export interface StructureLimits { omitted: StructureOmission[]; } +export interface StructureNeighbours { + state: "off" | "on" | "unavailable"; + count: number; + reason?: string; +} + export interface StructureSnapshot { protocol: "rt/1"; inputId: string; @@ -160,6 +179,7 @@ export interface StructureSnapshot { files: StructureFile[]; edges: StructureEdge[]; limits: StructureLimits; + neighbours: StructureNeighbours; } export type SseEventType = "hello" | "state" | "question" | "answer_delta" | "source" | "log_update" | "error" | "bye"; diff --git a/packages/review-tutor/src/server-session.ts b/packages/review-tutor/src/server-session.ts index d162900..e41934d 100644 --- a/packages/review-tutor/src/server-session.ts +++ b/packages/review-tutor/src/server-session.ts @@ -7,7 +7,7 @@ import { buildTutorPrompt } from "./prompt.ts"; import { LIMITS, validateAskRequest, validateLogPatch, validateSourceRequest, type AskRequest, type InputSnapshot, type LearningEntry, type ModelChoice, type QuestionView, type StructureSnapshot } from "./protocol.ts"; import type { StatePaths } from "./paths.ts"; import type { SseHub } from "./sse.ts"; -import { structureSnapshotFor } from "./structure.ts"; +import { structureSnapshotWithNeighbours } from "./structure.ts"; export interface RunnerLike { run(request: { provider: string; model: string; thinking: string; cwd: string; prompt: string }, delta: (text: string) => void): Promise<{ answer: string; usage?: Record }>; @@ -46,6 +46,13 @@ function validateEmptyObject(value: unknown, name: string): void { } } +/** `neighbours=1` turns the bounded one-hop neighbour read on; anything else is a typed error. */ +function parseNeighboursFlag(value: string | null): boolean { + if (value === null || value === "0") return false; + if (value === "1") return true; + throw new Error("neighbours query failed: expected 1, 0, or absent; correct it and retry"); +} + function parseLogLimit(value: string | null): number { if (value === null) return 100; if (!/^(?:0|[1-9]\d*)$/.test(value)) throw new Error( @@ -65,7 +72,7 @@ export class ReviewTutorSession { private readonly queue: string[] = []; private readonly threadHistory: LearningEntry[] = []; private currentInput?: InputSnapshot; - private structureCache?: { inputId: string; snapshot: StructureSnapshot }; + private readonly structureCache = new Map(); private running?: string; private lastHeartbeat = 0; private closing = false; @@ -282,14 +289,20 @@ export class ReviewTutorSession { } } - structure(): SessionReply { + async structure(url: URL): Promise { const input = this.currentInput; if (!input) return { status: 409, value: { error: "structure failed: expected a loaded input snapshot, received none; load a source and retry" } }; - if (this.structureCache?.inputId !== input.id) { - this.structureCache = { inputId: input.id, snapshot: structureSnapshotFor(input) }; - } - return { status: 200, value: this.structureCache.snapshot }; + const neighbours = parseNeighboursFlag(url.searchParams.get("neighbours")); + const key = `${input.id}${String(neighbours)}`; + const cached = this.structureCache.get(key); + if (cached) return { status: 200, value: cached }; + const snapshot = await structureSnapshotWithNeighbours(input, { + neighbours, cwd: this.options.cwd, execFile: this.options.execFile, + }); + if (this.structureCache.size >= 4) this.structureCache.clear(); + this.structureCache.set(key, snapshot); + return { status: 200, value: snapshot }; } async log(url: URL): Promise { diff --git a/packages/review-tutor/src/server.ts b/packages/review-tutor/src/server.ts index dc1c2f1..e6775b8 100644 --- a/packages/review-tutor/src/server.ts +++ b/packages/review-tutor/src/server.ts @@ -125,7 +125,7 @@ async function primaryRoute(request: IncomingMessage, response: ServerResponse, async function secondaryRoute(request: IncomingMessage, response: ServerResponse, url: URL, session: ReviewTutorSession): Promise { const method = request.method; const path = url.pathname; - if (method === "GET" && path === "/api/structure") return send(response, session.structure()); + if (method === "GET" && path === "/api/structure") return send(response, await session.structure(url)); if (method === "GET" && path === "/api/log") return send(response, await session.log(url)); const log = path.match(/^\/api\/log\/([^/]+)$/); if (method === "PATCH" && log) return send(response, await session.patchLog(request, log[1]!)); diff --git a/packages/review-tutor/src/structure-languages.ts b/packages/review-tutor/src/structure-languages.ts new file mode 100644 index 0000000..132e45e --- /dev/null +++ b/packages/review-tutor/src/structure-languages.ts @@ -0,0 +1,719 @@ +import { posix } from "node:path"; +import type { StructureEdge } from "./protocol.ts"; + +/** + * Private language tables for the Structure analyzer. Every entry is a lexical + * table — masking, statement scanning, and candidate-path resolution — for one + * language. There is no plugin surface: adding a language means adding a table + * here and one line to `languageForExtension`. + */ +export type LanguageId = "ts" | "dart" | "rust"; + +export interface Statement { + kind: StructureEdge["kind"]; + specifier: string; + typeOnly: boolean; + offset: number; + specifierOffset: number; + /** Resolve this specifier as exactly one path, with no per-language fallbacks. */ + exact?: boolean; +} + +export interface ScanResult { + statements: Statement[]; + nonLiteral: number[]; + overlong: number[]; + /** Directives this table understands but cannot resolve, reported verbatim. */ + unsupported: { offset: number; reason: string }[]; +} + +/** + * `external` aggregates without an edge, `candidates` is an ordered repo-relative + * candidate list, and `package` defers a Dart `package:` URI until a pubspec is known. + */ +export type ResolutionTarget = + | { kind: "external"; name: string } + | { kind: "candidates"; paths: string[] } + | { kind: "package"; name: string; path: string }; + +const TS_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]; +const JS_TO_TS: Record = { + ".js": [".ts", ".tsx"], + ".jsx": [".tsx"], + ".mjs": [".mts"], + ".cjs": [".cts"], +}; +const WORD_CHARACTER = /[\w$]/; + +export function extensionOf(path: string): string { + const name = path.slice(path.lastIndexOf("/") + 1); + const match = /\.[A-Za-z0-9]+$/.exec(name); + return match ? match[0].toLowerCase() : ""; +} + +export function languageForExtension(extension: string): LanguageId | undefined { + if (TS_EXTENSIONS.includes(extension)) return "ts"; + if (extension === ".dart") return "dart"; + if (extension === ".rs") return "rust"; + return undefined; +} + +function maskSpan(out: string[], from: number, to: number): void { + for (let index = from; index < to && index < out.length; index += 1) { + if (out[index] !== "\n") out[index] = " "; + } +} + +function maskLineComment(text: string, out: string[], start: number): number { + let index = start; + while (index < text.length && text[index] !== "\n") { out[index] = " "; index += 1; } + return index; +} + +/** Nested `/* … *​/` comments, as Dart and Rust both define them. */ +function maskNestedBlock(text: string, out: string[], start: number): number { + let index = start + 2; + let depth = 1; + maskSpan(out, start, index); + while (index < text.length && depth > 0) { + if (text[index] === "/" && text[index + 1] === "*") { depth += 1; maskSpan(out, index, index + 2); index += 2; continue; } + if (text[index] === "*" && text[index + 1] === "/") { depth -= 1; maskSpan(out, index, index + 2); index += 2; continue; } + maskSpan(out, index, index + 1); + index += 1; + } + return index; +} + +// --------------------------------------------------------------------------- +// TypeScript and JavaScript +// --------------------------------------------------------------------------- + +/** Statement keywords, ignoring member access such as `foo.import`. */ +const STATEMENT_KEYWORD = /(?, from: number): number { + let cursor = from; + while (cursor >= 0 && (text[cursor] === " " || text[cursor] === "\t" || text[cursor] === "\n")) cursor -= 1; + return cursor; +} + +/** The identifier ending at `cursor`, or "" when it is longer than an operator keyword. */ +function wordBefore(text: string[], cursor: number): string { + let start = cursor; + while (start >= 0 && WORD_CHARACTER.test(text[start]!)) { + if (cursor - start > 12) return ""; + start -= 1; + } + return text.slice(start + 1, cursor + 1).join(""); +} + +/** True when a `/` at `start` opens a regex literal rather than a division. */ +function opensRegex(out: string[], start: number): boolean { + const cursor = skipBackWhitespace(out, start - 1); + if (cursor < 0) return true; + const previous = out[cursor]!; + if (REGEX_START.has(previous)) return true; + if (previous === ">" && cursor > 0 && out[cursor - 1] === "=") return true; + if (!WORD_CHARACTER.test(previous)) return false; + const word = wordBefore(out, cursor); + if (!EXPRESSION_KEYWORDS.has(word)) return false; + const beforeWord = skipBackWhitespace(out, cursor - word.length); + return beforeWord < 0 || out[beforeWord] !== "."; +} + +function regexEnd(text: string, start: number): number | undefined { + let index = start + 1; + while (index < text.length && text[index] !== "\n") { + if (text[index] === "\\") { index += 2; continue; } + if (text[index] === "/") return index; + index += 1; + } + return undefined; +} + +function maskRegexLiteral(text: string, out: string[], start: number): number | undefined { + if (!opensRegex(out, start)) return undefined; + const end = regexEnd(text, start); + if (end === undefined) return undefined; + for (let index = start; index <= end; index += 1) out[index] = " "; + return end + 1; +} + +/** + * Blanks comment, string, and regex-literal bodies while preserving every UTF-16 + * offset, so no keyword inside them can be read as an import statement. + */ +export function maskLiterals(text: string): string { + const out = text.split(""); + let index = 0; + while (index < text.length) { + const character = text[index]!; + const next = text[index + 1]; + if (character === "/" && next === "/") index = maskLineComment(text, out, index); + else if (character === "/" && next === "*") index = maskBlockComment(text, out, index); + else if (character === "'" || character === "\"" || character === "`") index = maskString(text, out, index); + else if (character === "/") index = maskRegexLiteral(text, out, index) ?? index + 1; + else index += 1; + } + return out.join(""); +} + +function pushFromStatement(text: string, mask: string, at: number, statements: Statement[]): boolean { + FROM_TAIL.lastIndex = at; + const match = FROM_TAIL.exec(mask); + if (!match || NESTED_KEYWORD.test(match[2]!)) return false; + const whole = match[0]; + const open = whole.lastIndexOf(match[3]!, whole.length - 2); + if (open < 0) return false; + statements.push({ + kind: match[1] === "export" ? "reexport" : "import", + specifier: text.slice(at + open + 1, at + whole.length - 1), + typeOnly: /^\s*type\b/.test(match[2]!), + offset: at, + specifierOffset: at + open, + }); + return true; +} + +function pushBareImport(text: string, mask: string, at: number, statements: Statement[]): void { + BARE_TAIL.lastIndex = at; + const match = BARE_TAIL.exec(mask); + if (!match) return; + const whole = match[0]; + const open = whole.indexOf(match[1]!); + statements.push({ + kind: "import", + specifier: text.slice(at + open + 1, at + whole.length - 1), + typeOnly: false, + offset: at, + specifierOffset: at + open, + }); +} + +/** + * True when an import clause runs past the work bound before its module string, + * which is a statement the scanner must report rather than silently drop. + */ +function overlongClause(mask: string, at: number, keyword: string): boolean { + let index = at + keyword.length; + const stop = Math.min(mask.length, index + CLAUSE_SCAN); + while (index < stop && CLAUSE_CHARACTER.test(mask[index]!)) index += 1; + if (index - (at + keyword.length) <= CLAUSE_LIMIT) return false; + return mask[index] === "'" || mask[index] === "\""; +} + +function scanStatementKeywords(text: string, mask: string, statements: Statement[], overlong: number[]): void { + for (const keyword of mask.matchAll(STATEMENT_KEYWORD)) { + if (pushFromStatement(text, mask, keyword.index, statements)) continue; + if (overlongClause(mask, keyword.index, keyword[1]!)) { + overlong.push(keyword.index); + continue; + } + pushBareImport(text, mask, keyword.index, statements); + } +} + +/** `shim . require("x")` and `shim ?. import("x")` are member calls, not module loads. */ +function memberCall(mask: string, at: number): boolean { + const cursor = skipBackWhitespace(mask, at - 1); + return cursor >= 0 && mask[cursor] === "."; +} + +function scanCallStatements(text: string, mask: string, statements: Statement[], nonLiteral: number[]): void { + for (const match of mask.matchAll(CALL_IMPORT)) { + if (memberCall(mask, match.index)) continue; + const start = match.index + match[0].length; + const quote = mask[start]; + const close = quote === "'" || quote === "\"" ? mask.indexOf(quote, start + 1) : -1; + if (close < 0 || !/^\s*\)/.test(mask.slice(close + 1, close + 8))) { + nonLiteral.push(match.index); + continue; + } + statements.push({ + kind: match[1] === "require" ? "require" : "dynamic-import", + specifier: text.slice(start + 1, close), + typeOnly: false, + offset: match.index, + specifierOffset: start, + }); + } +} + +function scanTypeScript(text: string, mask: string): ScanResult { + const statements: Statement[] = []; + const nonLiteral: number[] = []; + const overlong: number[] = []; + scanStatementKeywords(text, mask, statements, overlong); + scanCallStatements(text, mask, statements, nonLiteral); + return { statements, nonLiteral, overlong, unsupported: [] }; +} + +function typeScriptCandidates(base: string): string[] { + const list = [base]; + const extension = extensionOf(base); + for (const replacement of JS_TO_TS[extension] ?? []) { + list.push(`${base.slice(0, -extension.length)}${replacement}`); + } + if (!extension) { + for (const candidate of TS_EXTENSIONS) list.push(`${base}${candidate}`); + for (const candidate of TS_EXTENSIONS) list.push(`${base}/index${candidate}`); + } + return list; +} + +// --------------------------------------------------------------------------- +// Dart +// --------------------------------------------------------------------------- + +const DART_DIRECTIVE = /(? = { + import: "import", export: "reexport", part: "part", +}; + +function scanDart(text: string, mask: string): ScanResult { + const statements: Statement[] = []; + const unsupported: { offset: number; reason: string }[] = []; + for (const directive of mask.matchAll(DART_DIRECTIVE)) { + const keyword = directive[1]!; + const after = directive.index + directive[0].length; + const string = quotedStringAt(text, mask, after); + if (!string) { + const library = DART_LIBRARY_NAME.exec(mask.slice(after, after + 256))?.[0]?.trim(); + if (keyword !== "part" && library) { + unsupported.push({ + offset: directive.index, + reason: `part-of library name '${library}': no import data without a library file path`, + }); + } + continue; + } + statements.push({ + kind: keyword.startsWith("part") && keyword !== "part" ? "part-of" : DART_KINDS[keyword]!, + specifier: string.value, + typeOnly: false, + offset: directive.index, + specifierOffset: string.start, + }); + } + return { statements, nonLiteral: [], overlong: [], unsupported }; +} + +export function pubspecPackageName(content: string): string | undefined { + return PUBSPEC_NAME.exec(content)?.[1]; +} + +function relativeCandidate(from: string, specifier: string): string { + return posix.normalize(posix.join(posix.dirname(from), specifier.replace(/\\/g, "/"))).replace(/\/+$/, ""); +} + +/** `packageRoots` maps a Dart package name to the directory holding its pubspec.yaml. */ +function dartTarget(from: string, statement: Statement, packageRoots: Map): ResolutionTarget { + const specifier = statement.specifier; + const scoped = DART_PACKAGE.exec(specifier); + if (scoped) { + const root = packageRoots.get(scoped[1]!); + if (root === undefined) return { kind: "package", name: scoped[1]!, path: scoped[2]! }; + return { kind: "candidates", paths: [posix.join(root, "lib", scoped[2]!)] }; + } + if (DART_SCHEME.test(specifier)) return { kind: "external", name: specifier }; + return { kind: "candidates", paths: [relativeCandidate(from, specifier)] }; +} + +export function dartPackageCandidates(root: string, path: string): string[] { + return [posix.join(root, "lib", path)]; +} + +// --------------------------------------------------------------------------- +// Rust +// --------------------------------------------------------------------------- + +const RUST_MOD = /(? 10) return start + 1; + maskSpan(out, start + 1, close); + return close + 1; + } + if (text[start + 2] === "'") { maskSpan(out, start + 1, start + 2); return start + 3; } + return start + 1; +} + +/** `r"…"`, `r#"…"#`, `b"…"`, and `br#"…"#` all open a string; a bare `r` or `b` does not. */ +function opensRustString(text: string, index: number): boolean { + const character = text[index]; + if (character !== "r" && character !== "b") return false; + const next = text[index + 1]; + return wordBoundaryBefore(text, index) && (next === "\"" || next === "#" || next === "r"); +} + +function opensRustByteChar(text: string, index: number): boolean { + return text[index] === "b" && text[index + 1] === "'" && wordBoundaryBefore(text, index); +} + +export function maskRust(text: string): string { + const out = text.split(""); + let index = 0; + while (index < text.length) { + const character = text[index]!; + const next = text[index + 1]; + if (character === "/" && next === "/") index = maskLineComment(text, out, index); + else if (character === "/" && next === "*") index = maskNestedBlock(text, out, index); + else if (opensRustString(text, index)) index = maskRustQuoted(text, out, index); + else if (opensRustByteChar(text, index)) index = rustCharLiteral(text, out, index + 1); + else if (character === "\"") index = maskQuoted(text, out, index, false, false); + else if (character === "'") index = rustCharLiteral(text, out, index); + else index += 1; + } + return out.join(""); +} + +/** + * The last `#[path = "…"]` attribute standing immediately before `offset`. The + * attribute is matched in the masked source, so a commented-out one is invisible; + * only its value is read from the raw text. + */ +function rustPathAttribute(text: string, mask: string, offset: number): string | undefined { + const start = Math.max(0, offset - 300); + const window = mask.slice(start, offset); + let found: string | undefined; + for (const match of window.matchAll(RUST_PATH_ATTRIBUTE)) { + const between = window.slice(match.index + match[0].length); + if (!/^[\s]*(?:pub\s*(?:\([^)\n]{0,64}\)\s*)?)?$/.test(between) || between.split("\n").length > 2) continue; + const value = start + match.index + match[0].indexOf("\"") + 1; + found = text.slice(value, value + match[1]!.length); + } + return found; +} + +function scanRustMods(text: string, mask: string, statements: Statement[]): void { + for (const match of mask.matchAll(RUST_MOD)) { + const attribute = rustPathAttribute(text, mask, match.index); + statements.push({ + kind: "mod", + specifier: attribute ?? match[1]!, + typeOnly: false, + offset: match.index, + specifierOffset: match.index, + ...(attribute === undefined ? {} : { exact: true }), + }); + } +} + +function matchingBrace(body: string, open: number): number { + let depth = 0; + for (let index = open; index < body.length; index += 1) { + if (body[index] === "{") depth += 1; + else if (body[index] === "}" && (depth -= 1) === 0) return index; + } + return -1; +} + +function splitTopLevel(inner: string): string[] { + const parts: string[] = []; + let depth = 0; + let start = 0; + for (let index = 0; index < inner.length; index += 1) { + if (inner[index] === "{") depth += 1; + else if (inner[index] === "}") depth -= 1; + else if (inner[index] === "," && depth === 0) { parts.push(inner.slice(start, index)); start = index + 1; } + } + parts.push(inner.slice(start)); + return parts; +} + +/** `crate::{a, b}` is two use paths; `self` inside a group names the group prefix itself. */ +function expandUsePaths(body: string, depth = 0): string[] { + const open = body.indexOf("{"); + if (open < 0) return [body.trim()]; + const close = matchingBrace(body, open); + const prefix = body.slice(0, open).trim().replace(/::$/, ""); + if (close < 0 || depth >= RUST_GROUP_DEPTH) return [prefix]; + const paths: string[] = []; + for (const member of splitTopLevel(body.slice(open + 1, close))) { + const trimmed = member.trim(); + if (!trimmed || paths.length >= RUST_GROUP_LIMIT) continue; + if (trimmed === "self") { paths.push(prefix); continue; } + for (const expanded of expandUsePaths(trimmed, depth + 1)) paths.push(`${prefix}::${expanded}`); + } + return paths.length ? paths.slice(0, RUST_GROUP_LIMIT) : [prefix]; +} + +function scanRustUses(text: string, mask: string, statements: Statement[]): void { + for (const match of mask.matchAll(RUST_USE)) { + const start = match.index + match[0].length; + const semicolon = mask.indexOf(";", start); + if (semicolon < 0 || semicolon - start > RUST_USE_LIMIT) continue; + const body = text.slice(start, semicolon).trim(); + if (!body) continue; + for (const path of expandUsePaths(body.replace(/\s+/g, " "))) { + if (!path) continue; + statements.push({ + kind: match[1] ? "reexport" : "use", + specifier: path, + typeOnly: false, + offset: match.index, + specifierOffset: match.index, + }); + } + } +} + +function scanRustMacros(text: string, mask: string, statements: Statement[]): void { + for (const match of mask.matchAll(RUST_INCLUDE)) { + const string = quotedStringAt(text, mask, match.index + match[0].length); + if (!string) continue; + statements.push({ + kind: "include", specifier: string.value, typeOnly: false, + offset: match.index, specifierOffset: string.start, + }); + } + for (const match of mask.matchAll(RUST_EXTERN)) { + statements.push({ + kind: "use", specifier: match[1]!, typeOnly: false, + offset: match.index, specifierOffset: match.index, + }); + } +} + +function scanRust(text: string, mask: string): ScanResult { + const statements: Statement[] = []; + scanRustMods(text, mask, statements); + scanRustUses(text, mask, statements); + scanRustMacros(text, mask, statements); + return { statements, nonLiteral: [], overlong: [], unsupported: [] }; +} + +/** The directory that holds a Rust file's child modules. */ +function rustModuleDir(from: string): string { + const directory = posix.dirname(from); + const name = from.slice(from.lastIndexOf("/") + 1); + if (RUST_ROOTS.has(name)) return directory; + return posix.join(directory, name.replace(/\.rs$/, "")); +} + +/** `crate::` names the current crate, so the file's own `/src` wins over a repository `src`. */ +function rustCrateRoots(from: string): string[] { + const own = /^(.*)\/src\//.exec(from); + const roots = own ? [`${own[1]}/src`] : []; + if (!roots.includes("src")) roots.push("src"); + return roots; +} + +/** Longest resolvable prefix first: `root/a/b.rs`, `root/a/b/mod.rs`, then `root/a.rs`, … */ +function rustPrefixCandidates(roots: string[], segments: string[]): string[] { + const paths: string[] = []; + for (const root of roots) { + for (let length = segments.length; length > 0; length -= 1) { + const base = posix.join(root, ...segments.slice(0, length)); + paths.push(`${base}.rs`, posix.join(base, "mod.rs")); + } + } + return paths; +} + +function rustUseSegments(specifier: string): string[] { + const head = specifier.split("{")[0]!.split("*")[0]!; + const segments = head.split("::").map((segment) => segment.trim().split(/\s+/)[0]!).filter(Boolean); + const valid: string[] = []; + for (const segment of segments) { + if (!RUST_IDENTIFIER.test(segment)) break; + valid.push(segment); + } + return valid.slice(0, RUST_SEGMENT_LIMIT + 1); +} + +function rustRelativeRoot(from: string, segments: string[]): { root: string; rest: string[] } { + let root = rustModuleDir(from); + let index = 0; + while (segments[index] === "super") { root = posix.dirname(root); index += 1; } + if (segments[index] === "self") index += 1; + return { root, rest: segments.slice(index) }; +} + +function rustUseTarget(from: string, statement: Statement): ResolutionTarget { + const segments = rustUseSegments(statement.specifier); + const head = segments[0]; + if (!head) return { kind: "candidates", paths: [] }; + if (head === "crate") return { kind: "candidates", paths: rustPrefixCandidates(rustCrateRoots(from), segments.slice(1)) }; + if (head === "super" || head === "self") { + const { root, rest } = rustRelativeRoot(from, segments); + return { kind: "candidates", paths: rustPrefixCandidates([root], rest) }; + } + return { kind: "external", name: head }; +} + +function rustTarget(from: string, statement: Statement): ResolutionTarget { + if (statement.kind === "include") return { kind: "candidates", paths: [relativeCandidate(from, statement.specifier)] }; + if (statement.kind === "mod") { + if (statement.exact) return { kind: "candidates", paths: [relativeCandidate(from, statement.specifier)] }; + const directory = rustModuleDir(from); + return { kind: "candidates", paths: [ + posix.join(directory, `${statement.specifier}.rs`), + posix.join(directory, statement.specifier, "mod.rs"), + ] }; + } + return rustUseTarget(from, statement); +} + +// --------------------------------------------------------------------------- +// Dispatch +// --------------------------------------------------------------------------- + +export function maskFor(language: LanguageId, text: string): string { + if (language === "dart") return maskDart(text); + if (language === "rust") return maskRust(text); + return maskLiterals(text); +} + +export function scanFor(language: LanguageId, text: string): ScanResult { + const mask = maskFor(language, text); + if (language === "dart") return scanDart(text, mask); + if (language === "rust") return scanRust(text, mask); + return scanTypeScript(text, mask); +} + +export function targetFor( + language: LanguageId, from: string, statement: Statement, packageRoots: Map, +): ResolutionTarget { + if (language === "dart") return dartTarget(from, statement, packageRoots); + if (language === "rust") return rustTarget(from, statement); + if (!statement.specifier.startsWith(".")) return { kind: "external", name: statement.specifier }; + return { kind: "candidates", paths: typeScriptCandidates(relativeCandidate(from, statement.specifier)) }; +} diff --git a/packages/review-tutor/src/structure-neighbours.ts b/packages/review-tutor/src/structure-neighbours.ts new file mode 100644 index 0000000..6039962 --- /dev/null +++ b/packages/review-tutor/src/structure-neighbours.ts @@ -0,0 +1,185 @@ +import type { ExecFile } from "./inputs.ts"; +import { NEIGHBOUR_LIMITS, type InputSnapshot, type StructureOmission } from "./protocol.ts"; + +/** + * Bounded, read-only neighbour reader. Every byte comes from the Git object + * store through argv-only `git cat-file`; the working tree is never read, so no + * symlink can be followed and no path outside the repository can be reached. + */ +const REVISION = /^[A-Za-z0-9][A-Za-z0-9._/~^{}@+-]{0,255}$/; +const UNSAFE_PATH = /(?:^|[/])[.][.](?:[/]|$)|^[/]|:|[\u0000-\u001F]/; + +export interface NeighbourSource { prefix: string; reason?: string } +export interface NeighbourUnavailable { unavailable: string } + +function revisionOf(value: string): string | undefined { + return !value.startsWith("-") && REVISION.test(value) ? value : undefined; +} + +/** The Git object prefix whose content matches the `to` side of this comparison. */ +export function neighbourSourceFor(input: InputSnapshot): NeighbourSource | NeighbourUnavailable { + if (input.kind === "paste") { + return { unavailable: "pasted code has no Git provenance, so neighbour files cannot be read" }; + } + if (input.kind === "pr") { + return { unavailable: "pull-request patch: base and head objects are not read locally, so neighbour files cannot be read" }; + } + if (input.kind === "worktree") return { prefix: ":", reason: "neighbour content read from the index" }; + if (input.kind === "staged") return { prefix: ":" }; + const raw = input.kind === "commit" ? input.revision : input.rangeTo; + const revision = raw === undefined ? undefined : revisionOf(raw); + if (!revision) { + return { unavailable: "neighbour revision missing: expected the snapshot to carry its Git revision, received none; reload the source and retry" }; + } + return { prefix: `${revision}:` }; +} + +export interface NeighbourReaderOptions { + execFile: ExecFile; + cwd: string; + prefix: string; + now?: () => number; + signal?: AbortSignal; + /** Injectable wall-clock deadline; defaults to `AbortSignal.timeout(timeoutMs)`. */ + deadline?: AbortSignal; +} + +const CAP_REASONS = { + count: `neighbour cap reached: expected at most ${NEIGHBOUR_LIMITS.maxFiles} neighbour files, received more; later neighbours are not read`, + bytes: `neighbour byte cap reached: expected at most ${NEIGHBOUR_LIMITS.maxTotalBytes} neighbour bytes, received more; later neighbours are not read`, + time: `neighbour time cap reached: expected at most ${NEIGHBOUR_LIMITS.timeoutMs} ms of neighbour reads, received more; later neighbours are not read`, + probes: `neighbour lookup cap reached: expected at most ${NEIGHBOUR_LIMITS.maxProbes} Git lookups, received more; later neighbours are not read`, +} as const; + +export class NeighbourReader { + readonly omissions: StructureOmission[] = []; + truncated = false; + private files = 0; + private bytes = 0; + private probes = 0; + private exhausted = false; + private readonly now: () => number; + private readonly started: number; + private readonly deadline: AbortSignal; + private readonly signal: AbortSignal; + + constructor(private readonly options: NeighbourReaderOptions) { + this.now = options.now ?? Date.now; + this.started = this.now(); + this.deadline = options.deadline ?? AbortSignal.timeout(NEIGHBOUR_LIMITS.timeoutMs); + this.signal = options.signal ? AbortSignal.any([options.signal, this.deadline]) : this.deadline; + } + + get count(): number { return this.files; } + + private note(omission: StructureOmission): void { + if (this.omissions.some((existing) => existing.path === omission.path && existing.reason === omission.reason)) return; + this.omissions.push(omission); + } + + private stop(reason: string): void { + this.exhausted = true; + this.truncated = true; + this.note({ reason }); + } + + /** + * A caller that abandoned the request is not a resource cap: stop every further + * lookup and say so, rather than spending the probe budget on doomed calls and + * blaming the cap for the missing neighbours. + */ + private cancelled(): boolean { + if (this.deadline.aborted || !this.signal.aborted) return false; + this.exhausted = true; + this.note({ reason: "neighbour reads stopped: request aborted" }); + return true; + } + + /** True while another bounded Git lookup is allowed. */ + private ready(): boolean { + if (this.exhausted) return false; + if (this.deadline.aborted) { this.stop(CAP_REASONS.time); return false; } + if (this.cancelled()) return false; + if (this.now() - this.started > NEIGHBOUR_LIMITS.timeoutMs) { this.stop(CAP_REASONS.time); return false; } + if (this.probes >= NEIGHBOUR_LIMITS.maxProbes) { this.stop(CAP_REASONS.probes); return false; } + return true; + } + + private safe(path: string): boolean { + if (path && path.length <= 1024 && !path.startsWith("-") && !UNSAFE_PATH.test(path)) return true; + this.note({ + path, + reason: `neighbour path rejected: expected a repo-relative path without '..' or ':', received '${path}'; no import data for that neighbour`, + }); + return false; + } + + private async git(args: string[], maxBuffer: number): Promise { + this.probes += 1; + try { + const result = await this.options.execFile("git", args, { + cwd: this.options.cwd, + maxBuffer, + encoding: "utf8", + signal: this.signal, + }); + return result.stdout; + } catch { + if (this.deadline.aborted) this.stop(CAP_REASONS.time); + else this.cancelled(); + return undefined; + } + } + + private async sizeOf(path: string): Promise { + const stdout = await this.git(["cat-file", "-s", `${this.options.prefix}${path}`], 64); + if (stdout === undefined) return undefined; + const digits = stdout.trim(); + return /^[0-9]{1,15}$/.test(digits) ? Number(digits) : undefined; + } + + /** Reads one blob, charging it against the file, byte, and time budgets. */ + private async take(path: string, size: number, countFile: boolean, silent = false): Promise { + if (size > NEIGHBOUR_LIMITS.maxFileBytes) { + this.truncated = true; + this.note({ + path, + reason: `neighbour file too large: expected at most ${NEIGHBOUR_LIMITS.maxFileBytes} bytes, received ${size}; no import data for that neighbour`, + }); + return undefined; + } + if (this.bytes + size > NEIGHBOUR_LIMITS.maxTotalBytes) { this.stop(CAP_REASONS.bytes); return undefined; } + if (!this.ready()) return undefined; + const content = await this.git(["cat-file", "blob", `${this.options.prefix}${path}`], NEIGHBOUR_LIMITS.maxFileBytes + 4096); + if (content === undefined) { + if (!silent) this.note({ path, reason: "neighbour read failed: expected a readable Git blob, received none; no import data for that neighbour" }); + return undefined; + } + this.bytes += size; + if (countFile) this.files += 1; + return content; + } + + /** The first candidate that exists in the Git object store, with its content. */ + async find(paths: string[]): Promise<{ path: string; content: string } | undefined> { + if (this.exhausted || this.cancelled()) return undefined; + if (this.files >= NEIGHBOUR_LIMITS.maxFiles) { this.stop(CAP_REASONS.count); return undefined; } + for (const path of paths) { + if (!this.ready()) return undefined; + if (!this.safe(path)) continue; + const size = await this.sizeOf(path); + if (size === undefined) continue; + const content = await this.take(path, size, true); + return content === undefined ? undefined : { path, content }; + } + return undefined; + } + + /** Reads a bounded support file (a pubspec) without spending a neighbour slot. */ + async support(path: string): Promise { + if (!this.ready() || !this.safe(path)) return undefined; + const size = await this.sizeOf(path); + if (size === undefined) return undefined; + return this.take(path, size, false, true); + } +} diff --git a/packages/review-tutor/src/structure.ts b/packages/review-tutor/src/structure.ts index 56c3202..5102de9 100644 --- a/packages/review-tutor/src/structure.ts +++ b/packages/review-tutor/src/structure.ts @@ -1,4 +1,5 @@ import { posix } from "node:path"; +import type { ExecFile } from "./inputs.ts"; import { STRUCTURE_LIMITS, type InputSnapshot, @@ -7,9 +8,25 @@ import { type StructureEvidence, type StructureFile, type StructureLimits, + type StructureNeighbours, type StructureOmission, type StructureSnapshot, } from "./protocol.ts"; +import { + CLAUSE_LIMIT, + extensionOf, + languageForExtension, + maskLiterals, + pubspecPackageName, + scanFor, + targetFor, + type LanguageId, + type ResolutionTarget, + type Statement, +} from "./structure-languages.ts"; +import { NeighbourReader, neighbourSourceFor, type NeighbourSource } from "./structure-neighbours.ts"; + +export { maskLiterals }; type LineOrigin = "add" | "del" | "context"; type Side = "new" | "old"; @@ -34,71 +51,66 @@ interface ParsedDiff { interface DocLine { number: number; origin: LineOrigin; text: string; start: number } interface Doc { text: string; lines: DocLine[] } -interface Statement { - kind: StructureEdge["kind"]; - specifier: string; - typeOnly: boolean; - offset: number; - specifierOffset: number; -} interface ResolutionIndex { current: Map; previous: Map } +/** One specifier that matched no changed file, kept for the optional neighbour pass. */ +interface PendingLink { + from: string; + target: ResolutionTarget; + statement: Statement; + status: StructureEdge["status"]; + evidence: StructureEvidence[]; + line: number; + omission?: StructureOmission; + suppressed: boolean; +} + interface Collector { edges: Map; omitted: StructureOmission[]; external: Map>; + pending: PendingLink[]; + packageRoots: Map; unresolved: boolean; + unresolvedLinks: number; truncated: boolean; suppressed: number; droppedEdges: number; } -const ANALYZED_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]; -const EXTENSIONLESS_CANDIDATES = [".ts", ".tsx", ".mts", ".cts", ".js", ".jsx", ".mjs", ".cjs"]; -const JS_TO_TS: Record = { - ".js": [".ts", ".tsx"], - ".jsx": [".tsx"], - ".mjs": [".mts"], - ".cjs": [".cts"], -}; -/** Statement keywords, ignoring member access such as `foo.import`. */ -const STATEMENT_KEYWORD = /(?; neighbours: boolean } + +interface Analysis { + input: InputSnapshot; + reasons: string[]; + files: StructureFile[]; + parsed: ParsedDiff; + collector: Collector; +} + +export interface NeighbourRequest { + neighbours: boolean; + execFile: ExecFile; + cwd: string; + now?: () => number; + signal?: AbortSignal; + deadline?: AbortSignal; +} + +const PUBSPEC_DEPTH = 6; const CONTROL_CHARACTERS = /[\u0000-\u0008\u000A-\u001F\u007F]/g; -const REGEX_START = new Set(["(", ",", "=", ":", "[", "!", "&", "|", "?", "{", "}", ";"]); -const EXPRESSION_KEYWORDS = new Set([ - "return", "typeof", "case", "in", "of", "delete", "void", "throw", - "do", "else", "yield", "await", "instanceof", "new", -]); -const WORD_CHARACTER = /[\w$]/; function compareText(left: string, right: string): number { return left < right ? -1 : left > right ? 1 : 0; } -function extensionOf(path: string): string { - const name = path.slice(path.lastIndexOf("/") + 1); - const match = /\.[A-Za-z0-9]+$/.exec(name); - return match ? match[0].toLowerCase() : ""; -} - function unquotePath(value: string): string { if (!value.startsWith("\"") || !value.endsWith("\"") || value.length < 2) return value; return value.slice(1, -1).replace(/\\(["\\])/g, "$1"); } +/** `inputs.ts` pins `--src-prefix=a/ --dst-prefix=b/`, so only those two ever arrive. */ function stripSidePrefix(value: string): string { const path = unquotePath(value.trim()); return path.replace(/^[ab]\//, ""); @@ -246,110 +258,6 @@ export function parseUnifiedDiff(content: string): ParsedDiff { return { files, reasons: state.reasons, extraFiles: state.extraFiles, longLines: state.longLines }; } -function maskString(text: string, out: string[], start: number): number { - const quote = text[start]!; - let index = start + 1; - while (index < text.length) { - const character = text[index]!; - if (character === "\\") { - out[index] = " "; - if (text[index + 1] !== undefined && text[index + 1] !== "\n") out[index + 1] = " "; - index += 2; - continue; - } - if (character === quote) return index + 1; - if (character === "\n" && quote !== "`") return index; - if (character !== "\n") out[index] = " "; - index += 1; - } - return index; -} - -function maskLineComment(text: string, out: string[], start: number): number { - let index = start; - while (index < text.length && text[index] !== "\n") { out[index] = " "; index += 1; } - return index; -} - -function maskBlockComment(text: string, out: string[], start: number): number { - out[start] = " "; - out[start + 1] = " "; - let index = start + 2; - while (index < text.length && !(text[index] === "*" && text[index + 1] === "/")) { - if (text[index] !== "\n") out[index] = " "; - index += 1; - } - if (index < text.length) { out[index] = " "; out[index + 1] = " "; } - return index + 2; -} - -function skipBackWhitespace(text: ArrayLike, from: number): number { - let cursor = from; - while (cursor >= 0 && (text[cursor] === " " || text[cursor] === "\t" || text[cursor] === "\n")) cursor -= 1; - return cursor; -} - -/** The identifier ending at `cursor`, or "" when it is longer than an operator keyword. */ -function wordBefore(text: string[], cursor: number): string { - let start = cursor; - while (start >= 0 && WORD_CHARACTER.test(text[start]!)) { - if (cursor - start > 12) return ""; - start -= 1; - } - return text.slice(start + 1, cursor + 1).join(""); -} - -/** True when a `/` at `start` opens a regex literal rather than a division. */ -function opensRegex(out: string[], start: number): boolean { - const cursor = skipBackWhitespace(out, start - 1); - if (cursor < 0) return true; - const previous = out[cursor]!; - if (REGEX_START.has(previous)) return true; - if (previous === ">" && cursor > 0 && out[cursor - 1] === "=") return true; - if (!WORD_CHARACTER.test(previous)) return false; - const word = wordBefore(out, cursor); - if (!EXPRESSION_KEYWORDS.has(word)) return false; - const beforeWord = skipBackWhitespace(out, cursor - word.length); - return beforeWord < 0 || out[beforeWord] !== "."; -} - -function regexEnd(text: string, start: number): number | undefined { - let index = start + 1; - while (index < text.length && text[index] !== "\n") { - if (text[index] === "\\") { index += 2; continue; } - if (text[index] === "/") return index; - index += 1; - } - return undefined; -} - -function maskRegexLiteral(text: string, out: string[], start: number): number | undefined { - if (!opensRegex(out, start)) return undefined; - const end = regexEnd(text, start); - if (end === undefined) return undefined; - for (let index = start; index <= end; index += 1) out[index] = " "; - return end + 1; -} - -/** - * Blanks comment, string, and regex-literal bodies while preserving every UTF-16 - * offset, so no keyword inside them can be read as an import statement. - */ -export function maskLiterals(text: string): string { - const out = text.split(""); - let index = 0; - while (index < text.length) { - const character = text[index]!; - const next = text[index + 1]; - if (character === "/" && next === "/") index = maskLineComment(text, out, index); - else if (character === "/" && next === "*") index = maskBlockComment(text, out, index); - else if (character === "'" || character === "\"" || character === "`") index = maskString(text, out, index); - else if (character === "/") index = maskRegexLiteral(text, out, index) ?? index + 1; - else index += 1; - } - return out.join(""); -} - function buildDoc(file: DiffFile, side: Side): Doc { const lines: DocLine[] = []; const parts: string[] = []; @@ -370,6 +278,17 @@ function buildDoc(file: DiffFile, side: Side): Doc { return { text: parts.join("\n"), lines }; } +/** A whole neighbour file reads as unchanged context, one document line per source line. */ +function contentDoc(content: string): Doc { + const lines: DocLine[] = []; + let offset = 0; + content.split("\n").forEach((text, index) => { + lines.push({ number: index + 1, origin: "context", text, start: offset }); + offset += text.length + 1; + }); + return { text: content, lines }; +} + function lineAt(doc: Doc, offset: number): DocLine | undefined { let low = 0; let high = doc.lines.length - 1; @@ -394,119 +313,17 @@ function specifierLineFor(doc: Doc, offset: number, side: Side): DocLine | undef return line; } -function pushFromStatement(doc: Doc, mask: string, at: number, statements: Statement[]): boolean { - FROM_TAIL.lastIndex = at; - const match = FROM_TAIL.exec(mask); - if (!match || NESTED_KEYWORD.test(match[2]!)) return false; - const whole = match[0]; - const open = whole.lastIndexOf(match[3]!, whole.length - 2); - if (open < 0) return false; - statements.push({ - kind: match[1] === "export" ? "reexport" : "import", - specifier: doc.text.slice(at + open + 1, at + whole.length - 1), - typeOnly: /^\s*type\b/.test(match[2]!), - offset: at, - specifierOffset: at + open, - }); - return true; -} - -function pushBareImport(doc: Doc, mask: string, at: number, statements: Statement[]): void { - BARE_TAIL.lastIndex = at; - const match = BARE_TAIL.exec(mask); - if (!match) return; - const whole = match[0]; - const open = whole.indexOf(match[1]!); - statements.push({ - kind: "import", - specifier: doc.text.slice(at + open + 1, at + whole.length - 1), - typeOnly: false, - offset: at, - specifierOffset: at + open, - }); +function languageOf(path: string): LanguageId | undefined { + return languageForExtension(extensionOf(path)); } /** - * True when an import clause runs past the work bound before its module string, - * which is a statement the scanner must report rather than silently drop. - */ -function overlongClause(mask: string, at: number, keyword: string): boolean { - let index = at + keyword.length; - const stop = Math.min(mask.length, index + CLAUSE_SCAN); - while (index < stop && CLAUSE_CHARACTER.test(mask[index]!)) index += 1; - if (index - (at + keyword.length) <= CLAUSE_LIMIT) return false; - return mask[index] === "'" || mask[index] === "\""; -} - -function scanStatementKeywords(doc: Doc, mask: string, statements: Statement[], overlong: number[]): void { - for (const keyword of mask.matchAll(STATEMENT_KEYWORD)) { - if (pushFromStatement(doc, mask, keyword.index, statements)) continue; - if (overlongClause(mask, keyword.index, keyword[1]!)) { - overlong.push(keyword.index); - continue; - } - pushBareImport(doc, mask, keyword.index, statements); - } -} - -/** `shim . require("x")` and `shim ?. import("x")` are member calls, not module loads. */ -function memberCall(mask: string, at: number): boolean { - const cursor = skipBackWhitespace(mask, at - 1); - return cursor >= 0 && mask[cursor] === "."; -} - -function scanCallStatements(doc: Doc, mask: string, statements: Statement[], nonLiteral: number[]): void { - for (const match of mask.matchAll(CALL_IMPORT)) { - if (memberCall(mask, match.index)) continue; - const start = match.index + match[0].length; - const quote = mask[start]; - const close = quote === "'" || quote === "\"" ? mask.indexOf(quote, start + 1) : -1; - if (close < 0 || !/^\s*\)/.test(mask.slice(close + 1, close + 8))) { - nonLiteral.push(match.index); - continue; - } - statements.push({ - kind: match[1] === "require" ? "require" : "dynamic-import", - specifier: doc.text.slice(start + 1, close), - typeOnly: false, - offset: match.index, - specifierOffset: start, - }); - } -} - -/** Lexical, non-executing scan of one reconstructed document side. */ -function scanDoc(doc: Doc): { statements: Statement[]; nonLiteral: number[]; overlong: number[] } { - const mask = maskLiterals(doc.text); - const statements: Statement[] = []; - const nonLiteral: number[] = []; - const overlong: number[] = []; - scanStatementKeywords(doc, mask, statements, overlong); - scanCallStatements(doc, mask, statements, nonLiteral); - return { statements, nonLiteral, overlong }; -} - -function candidatePaths(base: string): string[] { - const list = [base]; - const extension = extensionOf(base); - for (const replacement of JS_TO_TS[extension] ?? []) { - list.push(`${base.slice(0, -extension.length)}${replacement}`); - } - if (!extension) { - for (const candidate of EXTENSIONLESS_CANDIDATES) list.push(`${base}${candidate}`); - for (const candidate of EXTENSIONLESS_CANDIDATES) list.push(`${base}/index${candidate}`); - } - return list; -} - -/** - * Candidate-path matching against the changed-file set only; no Node resolution, + * Candidate-path matching against the known file set only; no Node resolution, * no filesystem access. Old-side lookups consult pre-rename paths first because the * pre-change tree still held them; new-side lookups never do. */ -function resolveSpecifier(from: string, specifier: string, index: ResolutionIndex, side: Side): string | undefined { - const base = posix.normalize(posix.join(posix.dirname(from), specifier.replace(/\\/g, "/"))).replace(/\/+$/, ""); - for (const candidate of candidatePaths(base)) { +function lookup(paths: string[], index: ResolutionIndex, side: Side): string | undefined { + for (const candidate of paths) { const resolved = side === "old" ? index.previous.get(candidate) ?? index.current.get(candidate) : index.current.get(candidate); if (resolved) return resolved; } @@ -519,13 +336,28 @@ function trimText(text: string): string { return clean.length > STRUCTURE_LIMITS.evidenceText ? clean.slice(0, STRUCTURE_LIMITS.evidenceText) : clean; } -function omit(collector: Collector, omission: StructureOmission): void { - if (collector.omitted.some((existing) => existing.path === omission.path && existing.reason === omission.reason)) return; +function omit(collector: Collector, omission: StructureOmission): StructureOmission | undefined { + if (collector.omitted.some((existing) => existing.path === omission.path && existing.reason === omission.reason)) return undefined; if (collector.omitted.length >= STRUCTURE_LIMITS.omittedRows) { collector.suppressed += 1; - return; + return undefined; } collector.omitted.push(omission); + return omission; +} + +/** Withdraws a pending link's omission, including one the row cap had only counted. */ +function drop(collector: Collector, link: PendingLink): void { + if (link.suppressed) { collector.suppressed -= 1; return; } + if (!link.omission) return; + const index = collector.omitted.indexOf(link.omission); + if (index >= 0) collector.omitted.splice(index, 1); +} + +function omitPending(collector: Collector, omission: StructureOmission): Pick { + const before = collector.suppressed; + const created = omit(collector, omission); + return { ...(created ? { omission: created } : {}), suppressed: collector.suppressed > before }; } function addEvidence(edge: StructureEdge, evidence: StructureEvidence[]): void { @@ -550,46 +382,59 @@ function addEdge(collector: Collector, edge: StructureEdge, evidence: StructureE addEvidence(created, evidence); } -function evidenceFor(file: StructureFile, doc: Doc, statement: Statement, specifier: DocLine): StructureEvidence[] { +function evidenceFor(path: string, doc: Doc, statement: Statement, specifier: DocLine): StructureEvidence[] { const first = lineAt(doc, statement.offset); const lines = first && first.number !== specifier.number ? [first, specifier] : [specifier]; - return lines.map((line) => ({ path: file.path, line: line.number, text: trimText(line.text) })); + return lines.map((line) => ({ path, line: line.number, text: trimText(line.text) })); +} + +function addExternal(collector: Collector, path: string, name: string): void { + const bucket = collector.external.get(path) ?? new Set(); + bucket.add(name); + collector.external.set(path, bucket); } function recordStatement( collector: Collector, file: StructureFile, doc: Doc, side: Side, - statement: Statement, index: ResolutionIndex, + statement: Statement, index: ResolutionIndex, context: AnalysisContext, language: LanguageId, ): void { const specifier = specifierLineFor(doc, statement.specifierOffset, side); if (!specifier) return; - if (!statement.specifier.startsWith(".")) { - const bucket = collector.external.get(file.path) ?? new Set(); - bucket.add(statement.specifier); - collector.external.set(file.path, bucket); - return; - } const base = side === "old" ? file.renamedFrom ?? file.path : file.path; - const target = resolveSpecifier(base, statement.specifier, index, side); - if (!target) { - collector.unresolved = true; - omit(collector, { path: file.path, reason: trimText( - `specifier '${statement.specifier}' at line ${specifier.number} matches no changed file: no import data outside the changed set`) }); - return; + const target = targetFor(language, base, statement, context.packageRoots); + if (target.kind === "external") return addExternal(collector, file.path, target.name); + if (target.kind === "package" && !context.neighbours) { + return addExternal(collector, file.path, statement.specifier); } const status = specifier.origin === "add" ? "added" : specifier.origin === "del" ? "removed" : "unchanged"; - addEdge(collector, { - from: file.path, to: target, kind: statement.kind, typeOnly: statement.typeOnly, - status, specifier: statement.specifier, evidence: [], - }, evidenceFor(file, doc, statement, specifier)); + const evidence = evidenceFor(file.path, doc, statement, specifier); + const resolved = target.kind === "candidates" ? lookup(target.paths, index, side) : undefined; + if (resolved) { + return addEdge(collector, { + from: file.path, to: resolved, kind: statement.kind, typeOnly: statement.typeOnly, + status, specifier: statement.specifier, evidence: [], + }, evidence); + } + /** A `package:` URI is only unresolved once a pubspec has named its package; until then it is external. */ + if (target.kind === "package") { + collector.pending.push({ from: file.path, target, statement, status, evidence, suppressed: false, line: specifier.number }); + return; + } + collector.unresolvedLinks += 1; + const omission = omitPending(collector, { path: file.path, reason: trimText( + `specifier '${statement.specifier}' at line ${specifier.number} matches no changed file: no import data outside the changed set`) }); + if (context.neighbours) { + collector.pending.push({ from: file.path, target, statement, status, evidence, line: specifier.number, ...omission }); + } } function analyzeSide( collector: Collector, file: StructureFile, diff: DiffFile, side: Side, - index: ResolutionIndex, budget: { left: number }, + index: ResolutionIndex, budget: { left: number }, context: AnalysisContext, language: LanguageId, ): void { const doc = buildDoc(diff, side); if (!doc.text) return; - const { statements, nonLiteral, overlong } = scanDoc(doc); + const { statements, nonLiteral, overlong, unsupported } = scanFor(language, doc.text); const allowed = statements.slice(0, Math.max(0, budget.left)); budget.left -= statements.length; if (budget.left < 0) { @@ -597,7 +442,12 @@ function analyzeSide( omit(collector, { path: file.path, reason: `statement cap reached: expected at most ${STRUCTURE_LIMITS.statementsPerFile} import statements in this file, received more; later statements are not analyzed` }); } - for (const statement of allowed) recordStatement(collector, file, doc, side, statement, index); + for (const statement of allowed) recordStatement(collector, file, doc, side, statement, index, context, language); + for (const note of unsupported) { + if (!specifierLineFor(doc, note.offset, side)) continue; + collector.unresolved = true; + omit(collector, { path: file.path, reason: trimText(note.reason) }); + } for (const offset of nonLiteral) { const line = specifierLineFor(doc, offset, side); if (!line) continue; @@ -625,7 +475,7 @@ function describeFile(diff: DiffFile): StructureFile { analyzed: false, }; if (diff.binary) return { ...base, reason: "binary content: no import data" }; - if (!ANALYZED_EXTENSIONS.includes(extension)) { + if (!languageForExtension(extension)) { return { ...base, reason: `unsupported file type '${extension || "none"}': no import data` }; } if (!diff.hunks.length) return { ...base, reason: "no diff content for this file: no import data" }; @@ -704,6 +554,8 @@ function limitsOf(truncated: boolean, omitted: StructureOmission[]): StructureLi }; } +const NEIGHBOURS_OFF: StructureNeighbours = { state: "off", count: 0 }; + function emptySnapshot(input: InputSnapshot, reasons: string[]): StructureSnapshot { return { protocol: "rt/1", @@ -712,6 +564,7 @@ function emptySnapshot(input: InputSnapshot, reasons: string[]): StructureSnapsh files: [], edges: [], limits: limitsOf(false, []), + neighbours: NEIGHBOURS_OFF, }; } @@ -724,25 +577,44 @@ function collectExternal(collector: Collector): void { } } -function analyzeFiles(files: StructureFile[], diffs: Map): Collector { +/** Dart package names declared by a pubspec.yaml inside the changed set. */ +function pubspecRoots(diffs: Map): Map { + const roots = new Map(); + for (const [path, diff] of diffs) { + if (path !== "pubspec.yaml" && !path.endsWith("/pubspec.yaml")) continue; + const name = pubspecPackageName(buildDoc(diff, "new").text); + if (name) roots.set(name, posix.dirname(path)); + } + return roots; +} + +function indexOf(files: StructureFile[]): ResolutionIndex { const index: ResolutionIndex = { current: new Map(), previous: new Map() }; for (const file of files) { index.current.set(file.path, file.path); if (file.renamedFrom) index.previous.set(file.renamedFrom, file.path); } + return index; +} + +function analyzeFiles( + files: StructureFile[], diffs: Map, neighbours: boolean, +): { collector: Collector; index: ResolutionIndex } { + const index = indexOf(files); const collector: Collector = { - edges: new Map(), omitted: [], external: new Map(), - unresolved: false, truncated: false, suppressed: 0, droppedEdges: 0, + edges: new Map(), omitted: [], external: new Map(), pending: [], packageRoots: pubspecRoots(diffs), + unresolved: false, unresolvedLinks: 0, truncated: false, suppressed: 0, droppedEdges: 0, }; + const context: AnalysisContext = { packageRoots: collector.packageRoots, neighbours }; for (const file of files) { if (!file.analyzed) continue; + const language = languageOf(file.path)!; const diff = diffs.get(file.path)!; const budget = { left: STRUCTURE_LIMITS.statementsPerFile }; - analyzeSide(collector, file, diff, "new", index, budget); - analyzeSide(collector, file, diff, "old", index, budget); + analyzeSide(collector, file, diff, "new", index, budget, context, language); + analyzeSide(collector, file, diff, "old", index, budget, context, language); } - collectExternal(collector); - return collector; + return { collector, index }; } function sourceReasons(kind: InputSnapshot["kind"]): string[] { @@ -770,24 +642,26 @@ function capOmissions(collector: Collector, parsed: ParsedDiff, kept: number): v } } -/** - * Builds the deterministic Structure snapshot for one immutable input snapshot. - * Pure and lexical: no file reads, no child processes, no Git access, no execution. - */ -export function buildStructureSnapshot(input: InputSnapshot): StructureSnapshot { +function analyze(input: InputSnapshot, neighbours: boolean): { analysis: Analysis; index: ResolutionIndex } { const reasons = sourceReasons(input.kind); - if (input.kind === "paste") return emptySnapshot(input, reasons); const parsed = parseUnifiedDiff(input.content); reasons.push(...parsed.reasons); const files = parsed.files.map(describeFile).sort((left, right) => compareText(left.path, right.path)); - const collector = analyzeFiles(files, new Map(parsed.files.map((file) => [displayPath(file), file] as const))); + const diffs = new Map(parsed.files.map((file) => [displayPath(file), file] as const)); + const { collector, index } = analyzeFiles(files, diffs, neighbours); + return { analysis: { input, reasons, files, parsed, collector }, index }; +} + +function finish(analysis: Analysis, neighbours: StructureNeighbours): StructureSnapshot { + const { input, reasons, files, parsed, collector } = analysis; for (const edge of collector.edges.values()) { edge.evidence.sort((left, right) => left.line - right.line || compareText(left.path, right.path)); } const edges = sortEdges(collapseModified([...collector.edges.values()])); + collectExternal(collector); capOmissions(collector, parsed, files.length); if (collector.truncated) reasons.push("bounded analysis: some changed files, statements, or connections exceed the resource caps and are omitted"); - if (collector.unresolved) reasons.push("some specifiers match no changed file: no import data for those statements"); + if (collector.unresolved || collector.unresolvedLinks > 0) reasons.push("some specifiers match no changed file: no import data for those statements"); return { protocol: "rt/1", inputId: input.id, @@ -795,17 +669,188 @@ export function buildStructureSnapshot(input: InputSnapshot): StructureSnapshot files, edges, limits: limitsOf(collector.truncated, collector.omitted), + neighbours, }; } +/** + * Builds the deterministic Structure snapshot for one immutable input snapshot. + * Pure and lexical: no file reads, no child processes, no Git access, no execution. + */ +export function buildStructureSnapshot(input: InputSnapshot): StructureSnapshot { + if (input.kind === "paste") return emptySnapshot(input, sourceReasons(input.kind)); + return finish(analyze(input, false).analysis, NEIGHBOURS_OFF); +} + /** Route-safe wrapper: a malformed input degrades to a partial snapshot instead of throwing. */ export function structureSnapshotFor(input: InputSnapshot): StructureSnapshot { try { return buildStructureSnapshot(input); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - return emptySnapshot(input, [ - `structure analysis failed: expected a parsable unified diff, received content this analyzer could not process (${message}); reload the source or switch to the Diff view`, - ]); + return failedSnapshot(input, error); + } +} + +function failedSnapshot(input: InputSnapshot, error: unknown): StructureSnapshot { + const message = error instanceof Error ? error.message : String(error); + return emptySnapshot(input, [ + `structure analysis failed: expected a parsable unified diff, received content this analyzer could not process (${message}); reload the source or switch to the Diff view`, + ]); +} + +interface NeighbourState { + reader: NeighbourReader; + found: Map; + index: ResolutionIndex; + analysis: Analysis; +} + +/** Bounded ancestor directories of each importing file, repository root last. */ +function pubspecDirectories(links: PendingLink[]): string[] { + const seen = new Set(); + const directories: string[] = []; + for (const link of links) { + let directory = posix.dirname(link.from); + for (let depth = 0; depth < PUBSPEC_DEPTH && directory !== "." && directory !== "/"; depth += 1) { + if (!seen.has(directory)) { seen.add(directory); directories.push(directory); } + directory = posix.dirname(directory); + } + } + directories.push(""); + return directories; +} + +/** Reads the pubspec that names each deferred `package:` URI; absence is expected and silent. */ +async function loadPubspecs(state: NeighbourState): Promise { + const collector = state.analysis.collector; + const links = collector.pending.filter((link) => link.target.kind === "package"); + if (!links.length) return; + const wanted = new Set(links.map((link) => (link.target as { name: string }).name)); + for (const directory of pubspecDirectories(links)) { + if ([...wanted].every((name) => collector.packageRoots.has(name))) return; + const content = await state.reader.support(posix.join(directory, "pubspec.yaml")); + const name = content === undefined ? undefined : pubspecPackageName(content); + if (name && !collector.packageRoots.has(name)) collector.packageRoots.set(name, directory); + } +} + +function candidatesOf(link: PendingLink, packageRoots: Map): string[] { + if (link.target.kind === "candidates") return link.target.paths; + if (link.target.kind !== "package") return []; + const root = packageRoots.get(link.target.name); + return root === undefined ? [] : [posix.join(root, "lib", link.target.path)]; +} + +function attachNeighbourFile(state: NeighbourState, path: string, content: string): void { + if (state.found.has(path)) return; + state.found.set(path, content); + state.index.current.set(path, path); + state.analysis.files.push({ path, status: "context", additions: 0, deletions: 0, analyzed: true }); +} + +function resolveLink(state: NeighbourState, link: PendingLink, to: string): void { + const collector = state.analysis.collector; + if (link.target.kind === "package") collector.unresolvedLinks += 1; + drop(collector, link); + collector.unresolvedLinks -= 1; + addEdge(collector, { + from: link.from, to, kind: link.statement.kind, typeOnly: link.statement.typeOnly, + status: link.status, specifier: link.statement.specifier, evidence: [], + }, link.evidence); +} + +/** + * A deferred `package:` URI whose package no pubspec named is an external module, + * not a missing file; one whose package is known but whose file is absent is unresolved. + */ +function settlePackage(state: NeighbourState, link: PendingLink): void { + const collector = state.analysis.collector; + if (link.target.kind !== "package") return; + if (!collector.packageRoots.has(link.target.name)) { + return addExternal(collector, link.from, link.statement.specifier); + } + collector.unresolvedLinks += 1; + omit(collector, { path: link.from, reason: trimText( + `specifier '${link.statement.specifier}' at line ${link.line} matches no changed file: no import data outside the changed set`) }); +} + +async function resolvePending(state: NeighbourState): Promise { + const collector = state.analysis.collector; + for (const link of collector.pending) { + const paths = candidatesOf(link, collector.packageRoots); + const known = paths.length ? lookup(paths, state.index, "new") : undefined; + if (known) { + resolveLink(state, link, known); + continue; + } + const read = paths.length ? await state.reader.find(paths) : undefined; + if (!read) { + settlePackage(state, link); + continue; + } + attachNeighbourFile(state, read.path, read.content); + resolveLink(state, link, read.path); + } +} + +/** A neighbour's own outgoing connections, bounded to the already-known file set. */ +function analyzeNeighbour(state: NeighbourState, path: string, content: string): void { + const language = languageOf(path); + if (!language) return; + const collector = state.analysis.collector; + const doc = contentDoc(content); + const { statements } = scanFor(language, doc.text); + for (const statement of statements.slice(0, STRUCTURE_LIMITS.statementsPerFile)) { + const specifier = specifierLineFor(doc, statement.specifierOffset, "new"); + if (!specifier) continue; + const target = targetFor(language, path, statement, collector.packageRoots); + if (target.kind !== "candidates") continue; + const resolved = lookup(target.paths, state.index, "new"); + if (!resolved || resolved === path) continue; + addEdge(collector, { + from: path, to: resolved, kind: statement.kind, typeOnly: statement.typeOnly, + status: "unchanged", specifier: statement.specifier, evidence: [], + }, evidenceFor(path, doc, statement, specifier)); + } +} + +async function attachNeighbours( + analysis: Analysis, index: ResolutionIndex, source: NeighbourSource, request: NeighbourRequest, +): Promise { + const state: NeighbourState = { + reader: new NeighbourReader({ + execFile: request.execFile, cwd: request.cwd, prefix: source.prefix, + ...(request.now ? { now: request.now } : {}), ...(request.signal ? { signal: request.signal } : {}), + ...(request.deadline ? { deadline: request.deadline } : {}), + }), + found: new Map(), index, analysis, + }; + await loadPubspecs(state); + await resolvePending(state); + for (const path of [...state.found.keys()].sort(compareText)) analyzeNeighbour(state, path, state.found.get(path)!); + for (const omission of state.reader.omissions) omit(analysis.collector, omission); + if (state.reader.truncated) analysis.collector.truncated = true; + analysis.files.sort((left, right) => compareText(left.path, right.path)); + if (state.found.size && source.reason) analysis.reasons.push(source.reason); + return { state: "on", count: state.found.size }; +} + +/** + * Route-safe Structure snapshot with optional one-hop neighbours. Neighbour content + * is read only through Git, only for Git-based sources, and only within the caps. + */ +export async function structureSnapshotWithNeighbours( + input: InputSnapshot, request: NeighbourRequest, +): Promise { + try { + if (!request.neighbours) return buildStructureSnapshot(input); + const source = neighbourSourceFor(input); + if ("unavailable" in source) { + return { ...buildStructureSnapshot(input), neighbours: { state: "unavailable", count: 0, reason: source.unavailable } }; + } + const { analysis, index } = analyze(input, true); + return finish(analysis, await attachNeighbours(analysis, index, source, request)); + } catch (error) { + return failedSnapshot(input, error); } } diff --git a/packages/review-tutor/test/core.test.ts b/packages/review-tutor/test/core.test.ts index 0459883..f1bee0f 100644 --- a/packages/review-tutor/test/core.test.ts +++ b/packages/review-tutor/test/core.test.ts @@ -10,6 +10,7 @@ import { validateLogPatch, validateSourceRequest, type LearningEntry, + type SourceRequest, } from "../src/protocol.ts"; const dirs: string[] = []; @@ -136,7 +137,7 @@ describe("input loading", () => { await loadInput({ protocol: "rt/1", kind: "worktree" }, "/repo", exec); expect(exec).toHaveBeenCalledWith( "git", - ["diff", "--no-ext-diff", "--no-color", "--"], + ["diff", "--no-ext-diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", "--"], expect.anything(), ); await expect(loadInput({ @@ -234,29 +235,48 @@ describe("input loading", () => { content: "hello", label: "L", }, "/repo", exec)).toMatchObject({ kind: "paste", label: "L", byteCount: 5 }); - await loadInput({ + expect(await loadInput({ protocol: "rt/1", kind: "commit", revision: "HEAD~1", - }, "/repo", exec); + }, "/repo", exec)).toMatchObject({ kind: "commit", revision: "HEAD~1" }); expect(exec).toHaveBeenLastCalledWith( "git", - ["show", "--no-ext-diff", "--no-color", "--format=fuller", "HEAD~1", "--"], + ["show", "--no-ext-diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", "--format=fuller", "HEAD~1", "--"], expect.anything(), ); - await loadInput({ + expect(await loadInput({ protocol: "rt/1", kind: "range", from: "main", to: "topic", - }, "/repo", exec); + }, "/repo", exec)).toMatchObject({ kind: "range", rangeTo: "topic" }); expect(exec).toHaveBeenLastCalledWith( "git", - ["diff", "--no-ext-diff", "--no-color", "main...topic", "--"], + ["diff", "--no-ext-diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", "main...topic", "--"], expect.anything(), ); }); + it("pins the diff path prefixes for every Git-based source kind", async () => { + const exec = vi.fn(async (_command: string, _argv: string[]) => ({ stdout: "diff", stderr: "" })); + const requests: SourceRequest[] = [ + { protocol: "rt/1", kind: "worktree" }, + { protocol: "rt/1", kind: "staged" }, + { protocol: "rt/1", kind: "commit", revision: "HEAD" }, + { protocol: "rt/1", kind: "range", from: "main", to: "topic" }, + ]; + for (const request of requests) await loadInput(request, "/repo", exec); + expect(exec).toHaveBeenCalledTimes(4); + for (const call of exec.mock.calls) { + expect(call[1]).toContain("--src-prefix=a/"); + expect(call[1]).toContain("--dst-prefix=b/"); + } + expect(exec.mock.calls[1]![1]).toEqual( + ["diff", "--cached", "--no-ext-diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", "--"], + ); + }); + it("detects a PR head race", async () => { let views = 0; const exec = vi.fn(async (_cmd: string, argv: string[]) => argv[1] === "view" diff --git a/packages/review-tutor/test/structure.test.ts b/packages/review-tutor/test/structure.test.ts index ac449ed..5400095 100644 --- a/packages/review-tutor/test/structure.test.ts +++ b/packages/review-tutor/test/structure.test.ts @@ -3,9 +3,12 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; +import type { ExecFile } from "../src/inputs.ts"; import type { InputSnapshot, StructureEdge, StructureSnapshot } from "../src/protocol.ts"; import { startReviewTutorServer } from "../src/server.ts"; -import { buildStructureSnapshot, maskLiterals, parseUnifiedDiff, structureSnapshotFor } from "../src/structure.ts"; +import { + buildStructureSnapshot, maskLiterals, parseUnifiedDiff, structureSnapshotFor, structureSnapshotWithNeighbours, +} from "../src/structure.ts"; const skillPath = fileURLToPath(new URL("../skills/review-tutor/SKILL.md", import.meta.url)); const temporaryRoots: string[] = []; @@ -666,6 +669,567 @@ describe("bounded work", () => { }); }); +const DART_DIFF = changedFile("pubspec.yaml", ["name: demo", "+version: 1.0.1"]) + + changedFile("lib/utils.dart", ["+const utils = 1;"]) + + changedFile("lib/models.dart", ["+const models = 1;"]) + + changedFile("lib/main.g.dart", ["part of 'main.dart';", "+const generated = 1;"]) + + changedFile("lib/service.dart", ["+const service = 1;"]) + + changedFile("lib/stub.dart", ["+const stub = 1;"]) + + changedFile("lib/io_impl.dart", ["+const io = 1;"]) + + changedFile("lib/main.dart", [ + "import 'utils.dart';", + "export 'models.dart';", + "part 'main.g.dart';", + "import 'package:demo/service.dart';", + "import 'package:http/http.dart' as http;", + "import 'dart:io';", + "import 'stub.dart' if (dart.library.io) 'io_impl.dart';", + "+const version = 2;", + ]); + +describe("Dart language table", () => { + const snapshot = buildStructureSnapshot(input(DART_DIFF)); + + it("reads import, export, part, and part-of directives", () => { + expect(edgesFrom(snapshot, "lib/main.dart").map((found) => `${found.kind}:${found.to}`)).toEqual([ + "part:lib/main.g.dart", + "reexport:lib/models.dart", + "import:lib/service.dart", + "import:lib/stub.dart", + "import:lib/utils.dart", + ]); + expect(edge(snapshot, "lib/main.g.dart", "lib/main.dart")).toMatchObject({ + kind: "part-of", specifier: "main.dart", typeOnly: false, status: "unchanged", + }); + }); + + it("maps a package: URI of the changed package through its pubspec", () => { + expect(edge(snapshot, "lib/main.dart", "lib/service.dart")!.specifier).toBe("package:demo/service.dart"); + }); + + it("aggregates package: URIs of other packages and dart: URIs as external", () => { + expect(snapshot.limits.omitted).toContainEqual({ + path: "lib/main.dart", + reason: "external modules (2): dart:io, package:http/http.dart", + }); + }); + + it("takes only the default specifier of a conditional import", () => { + expect(edge(snapshot, "lib/main.dart", "lib/io_impl.dart")).toBeUndefined(); + expect(edge(snapshot, "lib/main.dart", "lib/stub.dart")!.specifier).toBe("stub.dart"); + }); + + it("never reads a directive out of a comment, a triple-quoted string, or a raw string", () => { + const diff = changedFile("lib/real.dart", ["+const real = 1;"]) + + changedFile("lib/commented.dart", ["+const commented = 1;"]) + + changedFile("lib/blocked.dart", ["+const blocked = 1;"]) + + changedFile("lib/triple.dart", ["+const triple = 1;"]) + + changedFile("lib/raw.dart", ["+const raw = 1;"]) + + changedFile("lib/masked.dart", [ + "// import 'commented.dart';", + "/* import 'blocked.dart'; */", + "const a = '''", + "import 'triple.dart';", + "''';", + "const b = r'import \"raw.dart\"';", + "import 'real.dart';", + ]); + const masked = buildStructureSnapshot(input(diff)); + expect(edgesFrom(masked, "lib/masked.dart").map((found) => found.to)).toEqual(["lib/real.dart"]); + expect(edgesFrom(masked, "lib/masked.dart")[0]!.evidence[0]!.line).toBe(7); + }); + + it("reports a library-name part-of directive as unresolvable", () => { + const snapshot = buildStructureSnapshot(input( + changedFile("lib/named.dart", ["part of my_library;", "+const named = 1;"]))); + expect(snapshot.edges).toEqual([]); + expect(snapshot.limits.omitted).toContainEqual({ + path: "lib/named.dart", + reason: "part-of library name 'my_library': no import data without a library file path", + }); + }); +}); + +const RUST_DIFF = changedFile("src/util/mod.rs", ["+pub struct Helper;"]) + + changedFile("src/parser/ast.rs", [ + "use super::lexer::Token;", + "use self::node::Node;", + "+pub struct Ast;", + ]) + + changedFile("src/parser/lexer.rs", ["+pub struct Token;"]) + + changedFile("src/parser/ast/node.rs", ["+pub struct Node;"]) + + changedFile("src/custom/other.rs", ["+pub struct Other;"]) + + changedFile("src/generated.rs", ["+pub const GENERATED: u8 = 1;"]) + + changedFile("src/parser.rs", [ + "mod ast;", + "include!(\"generated.rs\");", + "+pub struct Parser;", + ]) + + changedFile("src/lib.rs", [ + "mod parser;", + "pub mod util;", + "#[path = \"custom/other.rs\"]", + "mod other;", + "use crate::parser::ast::Node;", + "pub use crate::util::Helper;", + "use std::fs;", + "use serde::Serialize;", + "extern crate serde;", + "+pub const VERSION: u8 = 2;", + ]); + +describe("Rust language table", () => { + const snapshot = buildStructureSnapshot(input(RUST_DIFF)); + + it("resolves mod declarations from a crate root and from a nested file", () => { + expect(edge(snapshot, "src/lib.rs", "src/parser.rs")).toMatchObject({ kind: "mod", specifier: "parser" }); + expect(edge(snapshot, "src/lib.rs", "src/util/mod.rs")).toMatchObject({ kind: "mod", specifier: "util" }); + expect(edge(snapshot, "src/parser.rs", "src/parser/ast.rs")).toMatchObject({ kind: "mod", specifier: "ast" }); + }); + + it("honours a #[path] attribute on the preceding line", () => { + expect(edge(snapshot, "src/lib.rs", "src/custom/other.rs")).toMatchObject({ + kind: "mod", specifier: "custom/other.rs", + }); + }); + + it("resolves a use path by its longest resolvable prefix", () => { + expect(edge(snapshot, "src/lib.rs", "src/parser/ast.rs")).toMatchObject({ + kind: "use", specifier: "crate::parser::ast::Node", typeOnly: false, + }); + }); + + it("reports a pub use as a re-export", () => { + expect(edgesFrom(snapshot, "src/lib.rs").some((found) => + found.kind === "reexport" && found.to === "src/util/mod.rs")).toBe(true); + }); + + it("walks super and self module paths", () => { + expect(edge(snapshot, "src/parser/ast.rs", "src/parser/lexer.rs")).toMatchObject({ + kind: "use", specifier: "super::lexer::Token", + }); + expect(edge(snapshot, "src/parser/ast.rs", "src/parser/ast/node.rs")).toMatchObject({ + kind: "use", specifier: "self::node::Node", + }); + }); + + it("aggregates external crates instead of inventing edges", () => { + expect(snapshot.limits.omitted).toContainEqual({ + path: "src/lib.rs", + reason: "external modules (2): serde, std", + }); + }); + + it("resolves an include! macro relative to the including file", () => { + expect(edge(snapshot, "src/parser.rs", "src/generated.rs")).toMatchObject({ + kind: "include", specifier: "generated.rs", + }); + }); + + it("ignores declarations inside nested block comments, raw strings, and char literals", () => { + const diff = changedFile("src/helper.rs", ["+pub struct Helper;"]) + + changedFile("src/hidden.rs", ["+pub struct Hidden;"]) + + changedFile("src/fake.rs", ["+pub struct Fake;"]) + + changedFile("src/tricky.rs", [ + "/* outer /* inner */ mod hidden; */", + "const RAW: &str = r#\"mod fake; use crate::fake;\"#;", + "const QUOTE: char = '\\'';", + "const PLAIN: char = 'x';", + "fn borrow<'a>(value: &'a str) -> &'a str { value }", + "use crate::helper::Helper;", + ]); + const tricky = buildStructureSnapshot(input(diff)); + expect(edgesFrom(tricky, "src/tricky.rs").map((found) => found.to)).toEqual(["src/helper.rs"]); + expect(edgesFrom(tricky, "src/tricky.rs")[0]!.evidence[0]!.line).toBe(6); + }); + + it("prefers the file's own crate root over the repository src directory", () => { + const diff = changedFile("src/parser.rs", ["+pub struct Outer;"]) + + changedFile("pkg/app/src/parser.rs", ["+pub struct Inner;"]) + + changedFile("pkg/app/src/lib.rs", ["use crate::parser::Inner;", "+pub const VERSION: u8 = 1;"]); + const snapshot = buildStructureSnapshot(input(diff)); + expect(edgesFrom(snapshot, "pkg/app/src/lib.rs").map((found) => found.to)).toEqual(["pkg/app/src/parser.rs"]); + }); + + it("ignores a commented-out #[path] attribute", () => { + const diff = changedFile("src/other.rs", ["+pub struct Other;"]) + + changedFile("src/custom/other.rs", ["+pub struct Custom;"]) + + changedFile("src/lib.rs", ["// #[path = \"custom/other.rs\"]", "mod other;", "+pub const VERSION: u8 = 1;"]); + const snapshot = buildStructureSnapshot(input(diff)); + expect(edgesFrom(snapshot, "src/lib.rs").map((found) => found.to)).toEqual(["src/other.rs"]); + }); + + it("resolves a #[path] module exactly, without a mod.rs fallback", () => { + const diff = changedFile("src/gen/mod.rs", ["+pub struct Gen;"]) + + changedFile("src/lib.rs", ["#[path = \"gen.rs\"]", "mod gen;", "+pub const VERSION: u8 = 1;"]); + const snapshot = buildStructureSnapshot(input(diff)); + expect(edgesFrom(snapshot, "src/lib.rs")).toEqual([]); + expect(snapshot.limits.omitted).toContainEqual({ + path: "src/lib.rs", + reason: "specifier 'gen.rs' at line 2 matches no changed file: no import data outside the changed set", + }); + }); + + it("expands brace groups in a use path", () => { + const diff = changedFile("src/alpha.rs", ["+pub struct Alpha;"]) + + changedFile("src/beta.rs", ["+pub struct Beta;"]) + + changedFile("src/inner.rs", ["+pub struct Inner;"]) + + changedFile("src/inner/gamma.rs", ["+pub struct Gamma;"]) + + changedFile("src/lib.rs", [ + "use crate::{alpha, beta};", + "use crate::inner::{self, gamma};", + "+pub const VERSION: u8 = 1;", + ]); + const snapshot = buildStructureSnapshot(input(diff)); + expect(edgesFrom(snapshot, "src/lib.rs").map((found) => `${found.to}|${found.specifier}`)).toEqual([ + "src/alpha.rs|crate::alpha", + "src/beta.rs|crate::beta", + "src/inner.rs|crate::inner", + "src/inner/gamma.rs|crate::inner::gamma", + ]); + }); +}); + +type GitCall = string[]; + +function fakeGit(blobs: Record, calls: GitCall[]): ExecFile { + return async (file, args) => { + calls.push([file, ...args]); + if (file !== "git") throw new Error(`unexpected command '${file}'`); + const spec = args.at(-1)!; + const content = blobs[spec]; + if (content === undefined) throw new Error(`fatal: path does not exist: ${spec}`); + if (args[1] === "-s") return { stdout: `${Buffer.byteLength(content)}\n`, stderr: "" }; + return { stdout: content, stderr: "" }; + }; +} + +const NEIGHBOUR_DIFF = changedFile("src/entry.ts", [ + "+import { n } from \"./neighbour.ts\";", + "+export const entry = 1;", +]); + +const NEIGHBOUR_BLOB = "import { entry } from \"./entry.ts\";\nimport { far } from \"./far.ts\";\nexport const n = 1;\n"; + +async function withNeighbours( + source: InputSnapshot, blobs: Record, calls: GitCall[], now?: () => number, +): Promise { + return structureSnapshotWithNeighbours(source, { + neighbours: true, cwd: "/repo", execFile: fakeGit(blobs, calls), ...(now ? { now } : {}), + }); +} + +describe("one-hop neighbours", () => { + it("reads a worktree neighbour from the index and says so", async () => { + const calls: GitCall[] = []; + const snapshot = await withNeighbours( + input(NEIGHBOUR_DIFF, "worktree"), { ":src/neighbour.ts": NEIGHBOUR_BLOB }, calls); + expect(calls).toEqual([ + ["git", "cat-file", "-s", ":src/neighbour.ts"], + ["git", "cat-file", "blob", ":src/neighbour.ts"], + ]); + expect(snapshot.neighbours).toEqual({ state: "on", count: 1 }); + expect(snapshot.comparison.reasons).toContain("neighbour content read from the index"); + }); + + it("gives a neighbour the context status and keeps its own edges to the changed set", async () => { + const calls: GitCall[] = []; + const snapshot = await withNeighbours( + input(NEIGHBOUR_DIFF, "worktree"), { ":src/neighbour.ts": NEIGHBOUR_BLOB }, calls); + expect(snapshot.files.find((file) => file.path === "src/neighbour.ts")).toEqual({ + path: "src/neighbour.ts", status: "context", additions: 0, deletions: 0, analyzed: true, + }); + expect(edge(snapshot, "src/entry.ts", "src/neighbour.ts")).toMatchObject({ status: "added" }); + expect(edge(snapshot, "src/neighbour.ts", "src/entry.ts")).toMatchObject({ status: "unchanged" }); + expect(snapshot.limits.omitted.some((omission) => omission.reason.includes("./neighbour.ts"))).toBe(false); + }); + + it("stops at one hop", async () => { + const calls: GitCall[] = []; + const snapshot = await withNeighbours(input(NEIGHBOUR_DIFF, "worktree"), { + ":src/neighbour.ts": NEIGHBOUR_BLOB, ":src/far.ts": "export const far = 1;\n", + }, calls); + expect(snapshot.files.map((file) => file.path)).toEqual(["src/entry.ts", "src/neighbour.ts"]); + expect(edge(snapshot, "src/neighbour.ts", "src/far.ts")).toBeUndefined(); + expect(calls.map((call) => call.at(-1))).not.toContain(":src/far.ts"); + }); + + it("reads staged, commit, and range neighbours from the matching Git object", async () => { + const specs: string[] = []; + for (const [source, spec] of [ + [input(NEIGHBOUR_DIFF, "staged"), ":src/neighbour.ts"], + [{ ...input(NEIGHBOUR_DIFF, "commit"), label: "Commit 9fceb02", revision: "9fceb02" }, "9fceb02:src/neighbour.ts"], + [{ ...input(NEIGHBOUR_DIFF, "range"), label: "main...feature", rangeTo: "feature" }, "feature:src/neighbour.ts"], + ] as const) { + const calls: GitCall[] = []; + const snapshot = await withNeighbours(source, { [spec]: NEIGHBOUR_BLOB }, calls); + expect(snapshot.neighbours.count).toBe(1); + specs.push(calls[0]!.at(-1)!); + } + expect(specs).toEqual([":src/neighbour.ts", "9fceb02:src/neighbour.ts", "feature:src/neighbour.ts"]); + }); + + it("reports neighbours as unavailable for pull-request and pasted sources", async () => { + const calls: GitCall[] = []; + const pr = await withNeighbours({ ...input(NEIGHBOUR_DIFF, "pr"), headSha: "abc123" }, {}, calls); + expect(pr.neighbours.state).toBe("unavailable"); + expect(pr.neighbours.reason).toContain("pull-request"); + const paste = await withNeighbours(input("code", "paste"), {}, calls); + expect(paste.neighbours.state).toBe("unavailable"); + expect(paste.neighbours.reason).toContain("Git provenance"); + expect(calls).toEqual([]); + }); + + it("rejects a path that escapes the repository before running Git", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/escape.ts", ["+import { x } from \"../../outside.ts\";"]); + const snapshot = await withNeighbours(input(diff, "worktree"), {}, calls); + expect(calls).toEqual([]); + expect(snapshot.limits.omitted).toContainEqual({ + path: "../outside.ts", + reason: "neighbour path rejected: expected a repo-relative path without '..' or ':', received '../outside.ts'; no import data for that neighbour", + }); + }); + + it("rejects a neighbour path carrying Git object syntax", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", ["+import { x } from \"./2:secret.ts\";"]); + const snapshot = await withNeighbours(input(diff, "worktree"), {}, calls); + expect(calls).toEqual([]); + expect(snapshot.limits.omitted).toContainEqual({ + path: "src/2:secret.ts", + reason: "neighbour path rejected: expected a repo-relative path without '..' or ':', received 'src/2:secret.ts'; no import data for that neighbour", + }); + }); + + it("keeps probing after a missing candidate and never calls it a read failure", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", [ + "+import { x } from \"./x0\";", + "+import { u } from \"./util.js\";", + ]); + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":src/x0.ts": "export const x0 = 1;\n", + ":src/util.ts": "export const util = 1;\n", + }, calls); + expect(snapshot.neighbours.count).toBe(2); + expect(snapshot.files.map((file) => file.path)).toEqual(["src/entry.ts", "src/util.ts", "src/x0.ts"]); + expect(calls[0]).toEqual(["git", "cat-file", "-s", ":src/x0"]); + expect(calls[1]).toEqual(["git", "cat-file", "-s", ":src/x0.ts"]); + expect(snapshot.limits.omitted.some((omission) => omission.reason.includes("read failed"))).toBe(false); + }); + + it("resolves a Rust module through its second candidate", async () => { + const calls: GitCall[] = []; + const snapshot = await withNeighbours( + input(changedFile("src/lib.rs", ["+mod util;"]), "worktree"), + { ":src/util/mod.rs": "pub struct Helper;\n" }, calls); + expect(snapshot.neighbours.count).toBe(1); + expect(calls.map((call) => call.at(-1))).toEqual([ + ":src/util.rs", ":src/util/mod.rs", ":src/util/mod.rs", + ]); + }); + + it("caps the number of Git lookups", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", Array.from({ length: 24 }, (_unused, index) => + `+import { a } from "./missing${index}";`)); + const snapshot = await withNeighbours(input(diff, "worktree"), {}, calls); + expect(snapshot.neighbours.count).toBe(0); + expect(calls).toHaveLength(400); + expect(snapshot.limits.truncated).toBe(true); + expect(snapshot.limits.omitted).toContainEqual({ + reason: "neighbour lookup cap reached: expected at most 400 Git lookups, received more; later neighbours are not read", + }); + }); + + it("aborts an in-flight neighbour read at the wall-clock deadline", async () => { + const diff = changedFile("src/entry.ts", ["+import { a } from \"./a.ts\";"]); + const snapshot = await structureSnapshotWithNeighbours(input(diff, "worktree"), { + neighbours: true, + cwd: "/repo", + deadline: AbortSignal.timeout(5), + execFile: async (_file, _args, options) => new Promise((_resolve, reject) => { + options.signal?.addEventListener("abort", () => reject(new Error("aborted"))); + }), + }); + expect(snapshot.neighbours.count).toBe(0); + expect(snapshot.limits.truncated).toBe(true); + expect(snapshot.limits.omitted).toContainEqual({ + reason: "neighbour time cap reached: expected at most 5000 ms of neighbour reads, received more; later neighbours are not read", + }); + }); + + it("reads nothing when the request is already aborted", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", Array.from({ length: 24 }, (_unused, index) => + `+import { a } from "./missing${index}";`)); + const snapshot = await structureSnapshotWithNeighbours(input(diff, "worktree"), { + neighbours: true, cwd: "/repo", execFile: fakeGit({}, calls), signal: AbortSignal.abort(), + }); + expect(calls).toEqual([]); + expect(snapshot.neighbours).toEqual({ state: "on", count: 0 }); + expect(snapshot.limits.omitted.some((omission) => omission.reason.includes("lookup cap"))).toBe(false); + expect(snapshot.limits.omitted).toContainEqual({ reason: "neighbour reads stopped: request aborted" }); + }); + + it("stops probing when the request aborts mid-read", async () => { + const calls: GitCall[] = []; + const controller = new AbortController(); + const diff = changedFile("src/entry.ts", [ + "+import { a } from \"./a.ts\";", + "+import { b } from \"./b.ts\";", + ]); + const snapshot = await structureSnapshotWithNeighbours(input(diff, "worktree"), { + neighbours: true, + cwd: "/repo", + signal: controller.signal, + execFile: async (file, args) => { + calls.push([file, ...args]); + controller.abort(); + throw new Error("aborted"); + }, + }); + expect(calls).toHaveLength(1); + expect(snapshot.neighbours.count).toBe(0); + expect(snapshot.limits.omitted.some((omission) => omission.reason.includes("lookup cap"))).toBe(false); + expect(snapshot.limits.omitted).toContainEqual({ reason: "neighbour reads stopped: request aborted" }); + }); + + it("aggregates an unknown Dart package as external instead of calling it unresolved", async () => { + const calls: GitCall[] = []; + const diff = changedFile("lib/main.dart", [ + "+import 'package:http/http.dart';", + "+import 'dart:io';", + ]); + const on = await withNeighbours(input(diff, "worktree"), {}, calls); + const off = buildStructureSnapshot(input(diff)); + expect(on.limits.omitted).toContainEqual({ + path: "lib/main.dart", + reason: "external modules (2): dart:io, package:http/http.dart", + }); + expect(on.limits.omitted).toEqual(off.limits.omitted); + expect(on.comparison.reasons).toEqual(off.comparison.reasons); + }); + + it("finds a Dart pubspec by walking up from the importing file", async () => { + const calls: GitCall[] = []; + const diff = changedFile("packages/foo/lib/main.dart", ["+import 'package:foo/service.dart';"]); + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":packages/foo/pubspec.yaml": "name: foo\nversion: 1.0.0\n", + ":packages/foo/lib/service.dart": "const service = 1;\n", + }, calls); + expect(edge(snapshot, "packages/foo/lib/main.dart", "packages/foo/lib/service.dart")).toMatchObject({ + kind: "import", specifier: "package:foo/service.dart", + }); + expect(calls[0]!.at(-1)).toBe(":packages/foo/lib/pubspec.yaml"); + expect(calls.map((call) => call.at(-1))).toContain(":packages/foo/pubspec.yaml"); + }); + + it("uncounts a suppressed omission when a neighbour resolves it", async () => { + const calls: GitCall[] = []; + const fillers = Array.from({ length: 100 }, (_unused, index) => changedFile(`src/f${index}.ts`, [ + `+import { a } from "./gone-${index}-a.ts";`, + `+import { b } from "./gone-${index}-b.ts";`, + ])).join(""); + const diff = `${fillers}${changedFile("src/zz.ts", ["+import { n } from \"./found.ts\";"])}`; + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":src/found.ts": "export const found = 1;\n", + }, calls); + expect(snapshot.limits.omitted).toHaveLength(200); + expect(snapshot.limits.omitted.some((omission) => + omission.reason.startsWith("further omissions not listed"))).toBe(false); + expect(edge(snapshot, "src/zz.ts", "src/found.ts")).toBeDefined(); + }); + + it("caps the neighbour count", async () => { + const calls: GitCall[] = []; + const blobs: Record = {}; + for (let index = 0; index < 60; index += 1) blobs[`:src/n${index}.ts`] = `export const n${index} = 1;\n`; + const diff = changedFile("src/entry.ts", Array.from({ length: 60 }, (_unused, index) => + `+import { n } from "./n${index}.ts";`)); + const snapshot = await withNeighbours(input(diff, "worktree"), blobs, calls); + expect(snapshot.neighbours.count).toBe(50); + expect(snapshot.limits.truncated).toBe(true); + expect(snapshot.limits.omitted).toContainEqual({ + reason: "neighbour cap reached: expected at most 50 neighbour files, received more; later neighbours are not read", + }); + }); + + it("caps a single neighbour file by size", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", [ + "+import { big } from \"./big.ts\";", + "+import { small } from \"./small.ts\";", + ]); + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":src/big.ts": "x".repeat(300_000), ":src/small.ts": "export const small = 1;\n", + }, calls); + expect(snapshot.neighbours.count).toBe(1); + expect(calls.some((call) => call.at(-1) === ":src/big.ts" && call[1] === "blob")).toBe(false); + expect(snapshot.limits.omitted).toContainEqual({ + path: "src/big.ts", + reason: "neighbour file too large: expected at most 262144 bytes, received 300000; no import data for that neighbour", + }); + }); + + it("caps the total neighbour bytes", async () => { + const calls: GitCall[] = []; + const blobs: Record = {}; + for (let index = 0; index < 10; index += 1) blobs[`:src/n${index}.ts`] = "x".repeat(250_000); + const diff = changedFile("src/entry.ts", Array.from({ length: 10 }, (_unused, index) => + `+import { n } from "./n${index}.ts";`)); + const snapshot = await withNeighbours(input(diff, "worktree"), blobs, calls); + expect(snapshot.neighbours.count).toBe(8); + expect(snapshot.limits.truncated).toBe(true); + expect(snapshot.limits.omitted).toContainEqual({ + reason: "neighbour byte cap reached: expected at most 2097152 neighbour bytes, received more; later neighbours are not read", + }); + }); + + it("caps neighbour reads by wall clock", async () => { + const calls: GitCall[] = []; + const diff = changedFile("src/entry.ts", [ + "+import { a } from \"./a.ts\";", + "+import { b } from \"./b.ts\";", + ]); + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":src/a.ts": "export const a = 1;\n", ":src/b.ts": "export const b = 1;\n", + }, calls, () => calls.length * 3000); + expect(snapshot.neighbours.count).toBe(1); + expect(snapshot.limits.truncated).toBe(true); + expect(snapshot.limits.omitted).toContainEqual({ + reason: "neighbour time cap reached: expected at most 5000 ms of neighbour reads, received more; later neighbours are not read", + }); + }); + + it("produces byte-identical JSON with neighbours on, whatever the discovery order", async () => { + const alpha = changedFile("src/alpha.ts", ["+import { one } from \"./one.ts\";"]); + const beta = changedFile("src/beta.ts", ["+import { two } from \"./two.ts\";"]); + const one = "export const one = 1;\n"; + const two = "export const two = 2;\n"; + const first = await withNeighbours( + input(`${alpha}${beta}`, "worktree"), { ":src/one.ts": one, ":src/two.ts": two }, []); + const second = await withNeighbours( + input(`${beta}${alpha}`, "worktree"), { ":src/two.ts": two, ":src/one.ts": one }, []); + expect(second.neighbours.count).toBe(2); + expect(JSON.stringify(first)).toBe(JSON.stringify(second)); + }); + + it("reads a Dart package: URI through a pubspec that is not in the diff", async () => { + const calls: GitCall[] = []; + const diff = changedFile("lib/main.dart", ["+import 'package:demo/service.dart';"]); + const snapshot = await withNeighbours(input(diff, "worktree"), { + ":pubspec.yaml": "name: demo\nversion: 1.0.0\n", + ":lib/service.dart": "const service = 1;\n", + }, calls); + expect(edge(snapshot, "lib/main.dart", "lib/service.dart")).toMatchObject({ + kind: "import", specifier: "package:demo/service.dart", + }); + expect(snapshot.neighbours.count).toBe(1); + }); +}); + async function startServer(diffs: string[]) { const home = await mkdtemp(join(tmpdir(), "review-tutor-structure-")); temporaryRoots.push(home); @@ -723,4 +1287,48 @@ describe("structure endpoint", () => { await server.close(); } }); + + it("caches per neighbours flag and reads neighbours only when asked", async () => { + const home = await mkdtemp(join(tmpdir(), "review-tutor-neighbours-")); + temporaryRoots.push(home); + const calls: GitCall[] = []; + const blob = fakeGit({ ":src/neighbour.ts": NEIGHBOUR_BLOB }, calls); + const server = await startReviewTutorServer({ + cwd: "/repo", + canonicalRepo: "/repo", + models: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + skillPath, + home, + runner: { run: async () => ({ answer: "" }), cancel: () => {}, shutdown: async () => {} }, + execFile: async (file, args, options) => + (args[0] === "diff" ? { stdout: NEIGHBOUR_DIFF, stderr: "" } : blob(file, args, options)), + }); + const headers = { Authorization: `Bearer ${server.token}`, "Content-Type": "application/json" }; + const structure = async (query: string): Promise => + (await fetch(`http://127.0.0.1:${server.port}/api/structure${query}`, { headers })).text(); + try { + await fetch(`http://127.0.0.1:${server.port}/api/source`, { + method: "POST", headers, body: JSON.stringify({ protocol: "rt/1", kind: "worktree" }), + }).then((response) => response.json()); + + const off = await structure(""); + expect(calls).toEqual([]); + expect((JSON.parse(off) as StructureSnapshot).neighbours).toEqual({ state: "off", count: 0 }); + + const on = await structure("?neighbours=1"); + expect(calls).toHaveLength(2); + expect((JSON.parse(on) as StructureSnapshot).neighbours).toEqual({ state: "on", count: 1 }); + + expect(await structure("?neighbours=1")).toBe(on); + expect(await structure("")).toBe(off); + expect(calls).toHaveLength(2); + + const bad = await fetch(`http://127.0.0.1:${server.port}/api/structure?neighbours=yes`, { headers }); + expect(bad.status).toBe(400); + expect((await bad.json() as { error: string }).error) + .toBe("neighbours query failed: expected 1, 0, or absent; correct it and retry"); + } finally { + await server.close(); + } + }); }); From 3af52d0c3a6be223ca842d8f29db51d6019cdf56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Mon, 24 Aug 2026 20:46:46 -0300 Subject: [PATCH 2/2] feat(review-tutor): ask the tutor from structure evidence and show neighbours Add an Include unchanged neighbours control, render context files and the Dart and Rust connection kinds in the list and graph, and let an evidence row open the tutor composer on its exact diff line. --- packages/review-tutor/src/page-script.ts | 168 +++++++++-- packages/review-tutor/src/page-styles.ts | 28 +- packages/review-tutor/src/page.ts | 2 +- packages/review-tutor/test/page.test.ts | 340 ++++++++++++++++++++++- 4 files changed, 490 insertions(+), 48 deletions(-) diff --git a/packages/review-tutor/src/page-script.ts b/packages/review-tutor/src/page-script.ts index 66111fa..af80771 100644 --- a/packages/review-tutor/src/page-script.ts +++ b/packages/review-tutor/src/page-script.ts @@ -74,9 +74,12 @@ export const pageScript = String.raw` structureStatus = "idle", structureRequestSequence = 0, structureMode = readStructureMode(), + structureNeighbours = readStructureNeighbours(), + structureToggleAnnouncement = null, selectedConnection = null, graphSelection = null; const learningBadgeGroups = new Map(); + const structureCache = new Map(); const foreignActiveIds = new Set(); let railCollapsed = sessionStorage.getItem("reviewTutorRailCollapsed") === "1"; let restoreComposerOnDesktop = false; @@ -388,8 +391,24 @@ export const pageScript = String.raw` return "list"; } } + function readStructureNeighbours() { + try { + return sessionStorage.getItem("reviewTutorStructureNeighbours") === "1"; + } catch { + return false; + } + } function structureKindLabel(kind) { - return kind === "reexport" ? "re-export" : kind === "dynamic-import" ? "dynamic" : kind; + return kind === "reexport" ? "re-export" : kind === "dynamic-import" ? "dynamic" : kind === "part-of" ? "part of" : kind; + } + function structureNeighboursAnnouncement() { + return structureNeighbours ? "Neighbours included." : "Neighbours hidden."; + } + function restoreStructureNeighboursFocus() { + if (document.activeElement === document.body) element("structure-neighbours").focus(); + } + function structureCacheKey(inputId) { + return inputId + "|" + (structureNeighbours ? "neighbours" : "changed"); } function resetStructure() { structureRequestSequence++; @@ -397,9 +416,12 @@ export const pageScript = String.raw` structureError = null; structureInputId = null; structureStatus = "idle"; + structureToggleAnnouncement = null; selectedConnection = null; graphSelection = null; element("structure-mode-switch").hidden = true; + element("structure-neighbours-label").hidden = true; + element("structure-neighbours").disabled = false; element("structure-shared").replaceChildren(); element("structure-content").replaceChildren(); element("structure-graph").replaceChildren(); @@ -411,7 +433,31 @@ export const pageScript = String.raw` function normalizedLine(text) { return String(text || "").trim().replace(/\s+/g, " "); } - function jumpToEvidence(evidence, edgeStatus, evidenceIndex, allEvidence) { + function tutorEvidenceIndex(edge, fallbackIndex) { + if (edge.status !== "modified" || edge.evidence.length < 2) return fallbackIndex; + function findAddedEvidence(exact) { + return edge.evidence.findIndex((evidence) => { + const file = files.find((candidate) => candidate.path === evidence.path); + const evidenceText = normalizedLine(evidence.text); + return file?.lines.some((line) => { + const lineText = normalizedLine(line.text); + return line.kind === "addition" && line.selectLine === evidence.line && evidenceText && + (exact ? lineText === evidenceText : lineText.startsWith(evidenceText)); + }); + }); + } + const exactIndex = findAddedEvidence(true); + if (exactIndex >= 0) return exactIndex; + const prefixIndex = findAddedEvidence(false); + return prefixIndex >= 0 ? prefixIndex : fallbackIndex; + } + function jumpToEvidence(evidence, edgeStatus, evidenceIndex, allEvidence, askTutor = false) { + if (askTutor && ["queued", "running"].includes(currentQuestionState)) { + const message = "cancel or wait for the current answer before asking about this connection"; + showError(new Error(message), "Tutor"); + announce("Cancel or wait for the current answer before asking about this connection."); + return; + } clearStructureLanding(); const fileIndex = files.findIndex((file) => file.path === evidence.path); if (fileIndex < 0) { @@ -447,9 +493,18 @@ export const pageScript = String.raw` } const control = rowControl({ fileIndex, rowIndex: target.rowIndex }); const row = rowNode({ fileIndex, rowIndex: target.rowIndex }); + if (askTutor) { + chooseRow(fileIndex, target.rowIndex, false, false); + element("mode").value = "explain"; + composerSelectionKey = null; + openTutor(control); + element("question").value = ""; + updateActions(); + } row?.classList.add("structure-landing"); row?.scrollIntoView({ block: "center" }); - control?.focus({ preventScroll: true }); + if (askTutor) element("question").focus(); + else control?.focus({ preventScroll: true }); } function createConnectionRow(edge, key, options = {}) { const evidenceId = "connection-evidence-" + key; @@ -459,10 +514,12 @@ export const pageScript = String.raw` row.dataset.status = edge.status; row.setAttribute("aria-expanded", String(!!options.expanded)); row.setAttribute("aria-controls", evidenceId); - row.append( - makeSpan("connection-kind", structureKindLabel(edge.kind)), - makeSpan("connection-target", edge.to), - ); + const target = makeSpan("connection-target", edge.to); + const targetWrap = makeSpan("connection-target-wrap", ""); + targetWrap.append(target); + const targetFile = structureSnapshot?.files.find((file) => file.path === edge.to); + if (targetFile?.status === "context") targetWrap.append(makeSpan("connection-context status-context", "CONTEXT")); + row.append(makeSpan("connection-kind", structureKindLabel(edge.kind)), targetWrap); if (edge.typeOnly) row.append(makeSpan("connection-type", "type")); row.append(makeSpan("connection-status status-chip status-" + edge.status, structureStatusLabel(edge.status))); const evidenceList = document.createElement("ul"); @@ -479,7 +536,15 @@ export const pageScript = String.raw` open.className = "ghost open-in-diff"; open.textContent = "Open in Diff"; open.addEventListener("click", () => jumpToEvidence(evidence, edge.status, evidenceIndex, edge.evidence)); - item.append(code, open); + const askTutor = document.createElement("button"); + askTutor.type = "button"; + askTutor.className = "ghost ask-tutor-evidence"; + askTutor.textContent = "Ask the tutor"; + askTutor.addEventListener("click", () => { + const targetIndex = tutorEvidenceIndex(edge, evidenceIndex); + jumpToEvidence(edge.evidence[targetIndex], edge.status, targetIndex, edge.evidence, true); + }); + item.append(code, askTutor, open); evidenceList.append(item); }); row.addEventListener("click", () => { @@ -809,11 +874,20 @@ export const pageScript = String.raw` selectedConnection = null; graphSelection = null; const shared = element("structure-shared"), container = element("structure-content"), graph = element("structure-graph"), modeSwitch = element("structure-mode-switch"); + const neighboursLabel = element("structure-neighbours-label"), neighbours = element("structure-neighbours"); shared.replaceChildren(); container.replaceChildren(); graph.replaceChildren(); element("structure-graph-evidence")?.remove(); - modeSwitch.hidden = true; + modeSwitch.hidden = !structureSnapshot || structureStatus === "error"; + const neighboursState = structureSnapshot?.neighbours?.state; + neighboursLabel.hidden = !structureSnapshot; + neighbours.checked = neighboursState === "unavailable" + ? false + : structureStatus === "loading" || structureStatus === "error" + ? structureNeighbours + : neighboursState === "on"; + neighbours.disabled = structureStatus === "loading" || neighboursState === "unavailable"; container.hidden = false; graph.hidden = true; if (!currentSource || structureStatus === "loading") { @@ -860,23 +934,27 @@ export const pageScript = String.raw` ...snapshot.comparison.reasons.map((reason) => ({ reason })), ...snapshot.limits.omitted, ]; - if (snapshot.comparison.partial || snapshot.limits.truncated) { + const unavailableNeighbours = snapshot.neighbours?.state === "unavailable" ? snapshot.neighbours.reason : null; + if (snapshot.comparison.partial || snapshot.limits.truncated || unavailableNeighbours) { const notice = document.createElement("aside"); notice.id = "structure-partial"; notice.className = "structure-partial"; - const sentence = document.createElement("p"); - sentence.textContent = "Structure analysis is partial; some connections may be missing."; - const details = document.createElement("details"); - const summary = document.createElement("summary"); - summary.textContent = disclosureItems.length + " " + (disclosureItems.length === 1 ? "reason" : "reasons"); - const reasons = document.createElement("ul"); - for (const item of disclosureItems) { - const reason = document.createElement("li"); - reason.textContent = (item.path ? item.path + ": " : "") + item.reason; - reasons.append(reason); + if (snapshot.comparison.partial || snapshot.limits.truncated) { + const sentence = document.createElement("p"); + sentence.textContent = "Structure analysis is partial; some connections may be missing."; + const details = document.createElement("details"); + const summary = document.createElement("summary"); + summary.textContent = disclosureItems.length + " " + (disclosureItems.length === 1 ? "reason" : "reasons"); + const reasons = document.createElement("ul"); + for (const item of disclosureItems) { + const reason = document.createElement("li"); + reason.textContent = (item.path ? item.path + ": " : "") + item.reason; + reasons.append(reason); + } + details.append(summary, reasons); + notice.append(sentence, details); } - details.append(summary, reasons); - notice.append(sentence, details); + if (unavailableNeighbours) notice.append(makeSpan("structure-neighbours-reason", unavailableNeighbours)); shared.append(notice); } if (!snapshot.edges.length) @@ -922,15 +1000,26 @@ export const pageScript = String.raw` renderStructure(); return; } - if (structureInputId === currentSource.id && structureStatus !== "idle") return; const inputId = currentSource.id; + const cacheKey = structureCacheKey(inputId); + if (structureInputId === cacheKey && structureStatus !== "idle") return; + const cached = structureCache.get(cacheKey); + if (cached) { + structureInputId = cacheKey; + structureSnapshot = cached; + structureError = null; + structureStatus = "loaded"; + renderStructure(); + return; + } const sequence = ++structureRequestSequence; - structureInputId = inputId; + const toggleAnnouncement = structureToggleAnnouncement; + structureInputId = cacheKey; structureStatus = "loading"; renderStructure(); announce("Analyzing structure…"); - api("/api/structure").then((snapshot) => { - if (sequence !== structureRequestSequence || currentSource?.id !== inputId) return; + api(structureNeighbours ? "/api/structure?neighbours=1" : "/api/structure").then((snapshot) => { + if (sequence !== structureRequestSequence || currentSource?.id !== inputId || structureCacheKey(inputId) !== cacheKey) return; if (snapshot?.inputId !== inputId) { structureError = "Structure analysis did not match the current source."; structureSnapshot = null; @@ -939,20 +1028,40 @@ export const pageScript = String.raw` announce(structureError); return; } + structureCache.set(cacheKey, snapshot); structureSnapshot = snapshot; structureError = null; structureStatus = "loaded"; renderStructure(); - announce("Structure analysis complete."); + structureToggleAnnouncement = null; + announce("Structure analysis complete." + (toggleAnnouncement ? " " + toggleAnnouncement : "")); + restoreStructureNeighboursFocus(); }).catch((error) => { - if (sequence !== structureRequestSequence || currentSource?.id !== inputId) return; + if (sequence !== structureRequestSequence || currentSource?.id !== inputId || structureCacheKey(inputId) !== cacheKey) return; structureError = error instanceof Error ? error.message : String(error); - structureSnapshot = null; structureStatus = "error"; renderStructure(); + structureToggleAnnouncement = null; announce("Structure analysis failed: " + structureError); + restoreStructureNeighboursFocus(); }); } + function setStructureNeighbours() { + structureNeighbours = element("structure-neighbours").checked; + structureToggleAnnouncement = structureNeighboursAnnouncement(); + try { sessionStorage.setItem("reviewTutorStructureNeighbours", structureNeighbours ? "1" : "0"); } catch {} + const cached = currentSource && structureCache.get(structureCacheKey(currentSource.id)); + structureRequestSequence++; + structureInputId = currentSource ? structureCacheKey(currentSource.id) : null; + structureSnapshot = cached || structureSnapshot; + structureError = null; + structureStatus = cached ? "loaded" : "idle"; + if (cached) { + renderStructure(); + announce(structureToggleAnnouncement); + structureToggleAnnouncement = null; + } else ensureStructure(); + } function setStructureMode(mode) { structureMode = mode === "graph" ? "graph" : "list"; try { sessionStorage.setItem("reviewTutorStructureMode", structureMode); } catch {} @@ -2145,6 +2254,7 @@ export const pageScript = String.raw` tab.addEventListener("click", () => setStructureMode(mode)); tab.addEventListener("keydown", handleStructureModeKey); } + element("structure-neighbours").addEventListener("change", setStructureNeighbours); element("history-previous").addEventListener("click", () => renderHistoryEntry(Math.max(0, historyIndex - 1))); element("history-next").addEventListener("click", () => renderHistoryEntry(Math.min(historyEntries.length - 1, historyIndex + 1))); element("question").addEventListener("input", updateActions); diff --git a/packages/review-tutor/src/page-styles.ts b/packages/review-tutor/src/page-styles.ts index 72e83d0..6c1f349 100644 --- a/packages/review-tutor/src/page-styles.ts +++ b/packages/review-tutor/src/page-styles.ts @@ -290,8 +290,16 @@ position:fixed;top:calc(var(--topbar-h) + 10px);left:50%;translate:-50% 0;z-inde display:none} .structure-section { contain:layout;min-width:0;padding:14px 14px 100px;overflow-x:hidden} +.structure-head { +display:flex;align-items:center;gap:12px;margin:0;flex-wrap:wrap} +.structure-head:has(> :not([hidden])) { +margin-bottom:10px} .structure-mode-switch { -width:max-content;margin:0 0 10px} +width:max-content;margin:0} +.structure-neighbours { +display:flex;align-items:center;gap:7px;color:var(--subtle);font-size:12px;cursor:pointer} +.structure-neighbours input { +width:auto;height:auto;margin:0;accent-color:var(--ember)} #structure-graph { max-width:100%;overflow-x:auto;overflow-y:hidden} .structure-graph-svg { @@ -308,7 +316,7 @@ color:var(--green)} color:var(--rose)} .structure-graph-node.status-modified,.structure-graph-edge.status-modified,.structure-graph-marker.status-modified { color:var(--amber)} -.structure-graph-node.status-unchanged,.structure-graph-node.status-renamed,.structure-graph-edge.status-unchanged,.structure-graph-marker.status-unchanged { +.structure-graph-node.status-unchanged,.structure-graph-node.status-renamed,.structure-graph-node.status-context,.structure-graph-edge.status-unchanged,.structure-graph-marker.status-unchanged { color:var(--muted)} .structure-graph-marker.status-selected { color:var(--ember)} @@ -350,7 +358,7 @@ margin-top:10px} position:static;min-width:0} .structure-file-path { min-width:0;margin:0;font:12px var(--font-mono);overflow:hidden;text-overflow:ellipsis;white-space:nowrap} -.status-chip,.connection-kind,.connection-type { +.status-chip,.connection-kind,.connection-type,.connection-context { flex:none;color:var(--muted);font:600 10px var(--font-mono);letter-spacing:.1em;text-transform:uppercase} .status-added { color:var(--green)} @@ -358,12 +366,16 @@ color:var(--green)} color:var(--rose)} .status-modified { color:var(--amber)} +.status-context { +color:var(--muted)} .structure-file-note { display:block;padding:6px 14px;border-bottom:1px solid var(--hairline);color:var(--muted);font:11px var(--font-mono);overflow-wrap:anywhere} .connection-list { min-width:0} .connection-row { width:100%;min-width:0;min-height:32px;display:grid;grid-template-columns:auto minmax(0,1fr) auto auto;align-items:center;gap:9px;padding:3px 12px;border:0;border-radius:0;background:transparent;text-align:left;white-space:normal} +.connection-target-wrap { +min-width:0;display:flex;align-items:center;gap:7px;flex-wrap:wrap} .connection-row:hover:not(:disabled) { background:var(--surface-2)} .connection-row.selected { @@ -378,7 +390,7 @@ min-width:0;display:flex;align-items:center;gap:12px;padding:7px 12px 7px 22px;b border-top:1px solid var(--hairline)} .evidence-code { flex:1;min-width:0;color:var(--subtle);font:12px/1.5 var(--font-mono);white-space:pre-wrap;overflow-wrap:anywhere} -.open-in-diff { +.open-in-diff,.ask-tutor-evidence { flex:none} .log-section { contain:layout;padding:20px 14px 100px;border-top:1px solid var(--hairline);scroll-margin-top:8px} @@ -477,7 +489,7 @@ overflow:visible;text-overflow:clip;white-space:normal;overflow-wrap:anywhere} grid-template-columns:auto auto auto minmax(0,1fr);grid-template-areas:"kind type status ." "target target target target";gap:2px 8px;padding-top:5px;padding-bottom:5px} .connection-kind { grid-area:kind} -.connection-target { +.connection-target-wrap { grid-area:target} .connection-type { grid-area:type} @@ -485,13 +497,15 @@ grid-area:type} grid-area:status} .connection-evidence li { align-items:stretch;flex-direction:column;padding-left:12px} -.open-in-diff { +.open-in-diff,.ask-tutor-evidence { width:100%;min-height:var(--control-h-touch)} } .diff-scroll { overflow:visible} .structure-section { padding-left:8px;padding-right:8px} +.structure-neighbours { +min-height:var(--control-h-touch)} .connection-row { min-height:var(--control-h-touch)} .file-head { @@ -526,7 +540,7 @@ top:6px} display:block;position:fixed;z-index:30;left:12px;right:12px;bottom:12px;min-height:48px;box-shadow:0 4px 18px #000;background:var(--ember);color:#160a02;border-color:var(--ember);font-weight:700} .mobile-ask:disabled { opacity:1} -.structure-error-card button,.structure-partial summary,.open-in-diff { +.structure-error-card button,.structure-partial summary,.open-in-diff,.ask-tutor-evidence { min-height:var(--control-h-touch)} .structure-partial summary { padding:8px 4px} diff --git a/packages/review-tutor/src/page.ts b/packages/review-tutor/src/page.ts index 5944b8b..ffd3376 100644 --- a/packages/review-tutor/src/page.ts +++ b/packages/review-tutor/src/page.ts @@ -41,7 +41,7 @@ export const pageHtml = `
No code selected

Load a source and enter a question.

-

Load a source to begin reviewing.

+

Load a source to begin reviewing.

`; diff --git a/packages/review-tutor/test/page.test.ts b/packages/review-tutor/test/page.test.ts index 88bd871..f564200 100644 --- a/packages/review-tutor/test/page.test.ts +++ b/packages/review-tutor/test/page.test.ts @@ -35,6 +35,7 @@ const state = { const structure = { protocol: "rt/1", inputId: source.id, + neighbours: { state: "off", count: 0 }, comparison: { kind: "worktree", label: "Working tree", @@ -110,6 +111,16 @@ function structureResponse(value: Promise | Response | unknown | Error return value instanceof Promise || value instanceof Response ? value : json(value); } +function blockStructureStorageReads(window: Window, options: { throwStructureModeRead?: boolean; throwStructureNeighboursRead?: boolean }) { + if (!options.throwStructureModeRead && !options.throwStructureNeighboursRead) return; + const getItem = window.sessionStorage.getItem.bind(window.sessionStorage); + window.sessionStorage.getItem = vi.fn((key: string) => { + if (options.throwStructureModeRead && key === "reviewTutorStructureMode") throw new Error("storage blocked"); + if (options.throwStructureNeighboursRead && key === "reviewTutorStructureNeighbours") throw new Error("storage blocked"); + return getItem(key); + }); +} + async function boot(options: { width?: number; askResponse?: Promise | Response; @@ -121,6 +132,8 @@ async function boot(options: { storedQuizIds?: string; storedStructureMode?: string; throwStructureModeRead?: boolean; + storedStructureNeighbours?: string; + throwStructureNeighboursRead?: boolean; stateResponses?: unknown[]; structureResponses?: Array | Response | unknown | Error>; failHeartbeat?: boolean; @@ -132,14 +145,9 @@ async function boot(options: { if (options.storedPageId !== undefined) window.sessionStorage.setItem("reviewTutorPageId", options.storedPageId); if (options.storedQuizIds !== undefined) window.sessionStorage.setItem("reviewTutorQuizEntryIds", options.storedQuizIds); if (options.storedStructureMode !== undefined) window.sessionStorage.setItem("reviewTutorStructureMode", options.storedStructureMode); + if (options.storedStructureNeighbours !== undefined) window.sessionStorage.setItem("reviewTutorStructureNeighbours", options.storedStructureNeighbours); if (options.railCollapsed) window.sessionStorage.setItem("reviewTutorRailCollapsed", "1"); - if (options.throwStructureModeRead) { - const getItem = window.sessionStorage.getItem.bind(window.sessionStorage); - window.sessionStorage.getItem = vi.fn((key: string) => { - if (key === "reviewTutorStructureMode") throw new Error("storage blocked"); - return getItem(key); - }); - } + blockStructureStorageReads(window, options); const script = pageHtml.match(/