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/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/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/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(/