From 1e37d4a05e7c4de5567541847b40ba41ef604e17 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 18:07:55 -0700 Subject: [PATCH 1/5] fix(api): validate PATCH bodies on /api/external-annotations, and read diagramAnchor defensively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH merged its body into the stored annotation verbatim in both runtimes, so any local process could store a value POST refuses. `{"diagramAnchor": null}` was answered 200 and the SSE broadcast then blanked the open page: DiagramBlock read `.family` off it during render. Two layers: - `validateAnnotationPatch` in @plannotator/core/external-annotation (the module both handlers already import) allowlists and field-validates the patch with the same validators POST applies — diagramAnchor through parseDiagramAnchor, htmlAnchor / elementContext / the target arrays through their own parsers, the scalars by type and cap. Unknown keys are dropped, `id` and `source` stay immutable, `null` clears an optional field and is refused on an anchor or a structural one. - The renderer no longer trusts the field: DiagramBlock and Viewer read it with `!= null` / `?.`, so no ingest — API, draft or share link — can make a render throw. --- .github/workflows/test.yml | 1 + .../server/external-annotations.test.ts | 117 ++++++++ .../server/external-annotations.ts | 18 +- apps/pi-extension/vendor.sh | 2 +- packages/core/external-annotation.ts | 256 +++++++++++++++++- packages/server/external-annotations.test.ts | 150 ++++++++++ packages/server/external-annotations.ts | 15 +- packages/ui/components/DiagramBlock.tsx | 10 +- .../Viewer.diagramAnchorHostile.test.tsx | 184 +++++++++++++ packages/ui/components/Viewer.tsx | 5 +- 10 files changed, 745 insertions(+), 13 deletions(-) create mode 100644 packages/ui/components/Viewer.diagramAnchorHostile.test.tsx diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a726b2af4..afe5a8e1e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -115,6 +115,7 @@ jobs: packages/ui/utils/diagramAnchorGraphviz.test.ts packages/ui/hooks/useAnnotationHighlighter.diagramSkip.test.tsx packages/ui/components/Viewer.diagramLazyRestore.test.tsx + packages/ui/components/Viewer.diagramAnchorHostile.test.tsx packages/ui/components/CommentPopover.skillReferences.test.tsx packages/ui/components/SkillReferenceMenu.placement.test.tsx packages/ui/components/sidebar/FileBrowser.test.ts diff --git a/apps/pi-extension/server/external-annotations.test.ts b/apps/pi-extension/server/external-annotations.test.ts index e76a83fdd..98a342426 100644 --- a/apps/pi-extension/server/external-annotations.test.ts +++ b/apps/pi-extension/server/external-annotations.test.ts @@ -91,3 +91,120 @@ describe("pi external annotations: PATCH inReplyTo", () => { expect((await patch(second, { text: "still fine" })).status).toBe(200); }); }); + +/** + * Node mirror of the PATCH body-validation describe in + * packages/server/external-annotations.test.ts (#1560 follow-up): PATCH used + * to merge its body verbatim, so `{"diagramAnchor": null}` was answered 200 + * and then blanked the page when the renderer read `.family` off it. + */ +describe("pi external annotations: PATCH body validation", () => { + const handler = createExternalAnnotationHandler("plan"); + const reviewHandler = createExternalAnnotationHandler("review"); + let server: Server; + let base = ""; + + beforeAll(async () => { + server = createServer(async (req, res) => { + const url = requestUrl(req); + const forReview = url.searchParams.get("mode") === "review"; + const target = forReview ? reviewHandler : handler; + const handled = await target.handle(req, res, url); + if (!handled) { + res.writeHead(404); + res.end(); + } + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const address = server.address(); + if (!address || typeof address === "string") throw new Error("no port"); + base = `http://127.0.0.1:${address.port}`; + }); + + afterAll(() => { + server.close(); + }); + + const seed = (target: typeof handler, body: unknown) => { + const added = target.addAnnotations(body); + if ("error" in added) throw new Error(added.error); + return added.ids[0]!; + }; + + const patch = async (id: string, body: unknown, mode?: "review") => { + const qs = mode ? `&mode=${mode}` : ""; + const res = await fetch(`${base}/api/external-annotations?id=${encodeURIComponent(id)}${qs}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + return { + status: res.status, + body: (await res.json()) as { error?: string; annotation?: Record }, + }; + }; + + const VALID_ANCHOR = { v: 1, family: "flowchart", kind: "node", id: "D", label: "Approve?", sourceLine: [7, 7] }; + + test("refuses every malformed diagramAnchor and leaves the stored one intact", async () => { + const id = seed(handler, { + source: "linter", + type: "COMMENT", + text: "external finding", + originalText: "Approve?", + diagramAnchor: VALID_ANCHOR, + }); + + const nulled = await patch(id, { diagramAnchor: null }); + expect(nulled.status).toBe(400); + expect(nulled.body.error).toContain("diagramAnchor"); + + for (const bad of [ + "nope", + 7, + {}, + [], + { ...VALID_ANCHOR, v: 2 }, + { ...VALID_ANCHOR, family: "not-a-family" }, + ]) { + expect((await patch(id, { diagramAnchor: bad })).status).toBe(400); + } + + const snapshot = (await (await fetch(`${base}/api/external-annotations`)).json()) as { + annotations: Array<{ id: string; diagramAnchor?: unknown }>; + }; + expect(snapshot.annotations.find((a) => a.id === id)?.diagramAnchor).toEqual(VALID_ANCHOR); + }); + + test("normalizes an accepted anchor and validates the other fields", async () => { + const id = seed(handler, { source: "linter", text: "note" }); + + const ok = await patch(id, { diagramAnchor: { ...VALID_ANCHOR, label: "x".repeat(600), stowaway: "dropped" } }); + expect(ok.status).toBe(200); + const stored = ok.body.annotation?.diagramAnchor as Record; + expect((stored.label as string).length).toBe(400); + expect(stored).not.toHaveProperty("stowaway"); + + expect((await patch(id, { diagramAnchor: { ...VALID_ANCHOR, sourceLine: [-3, -3] } })).status).toBe(200); + expect((await patch(id, { htmlAnchor: { tagName: "div" } })).status).toBe(400); + expect((await patch(id, { htmlAnchor: { selector: "#a", tagName: "div" } })).status).toBe(200); + expect((await patch(id, { elementContext: { id: "no-tag" } })).status).toBe(400); + expect((await patch(id, { images: [{ name: "a" }] })).status).toBe(400); + expect((await patch(id, { type: "NOT_A_TYPE" })).status).toBe(400); + expect((await patch(id, { text: 7 })).status).toBe(400); + expect((await patch(id, [{ text: "x" }])).status).toBe(400); + + const dropped = await patch(id, { text: "edited", notAField: { deep: true } }); + expect(dropped.status).toBe(200); + expect(dropped.body.annotation?.text).toBe("edited"); + expect(dropped.body.annotation).not.toHaveProperty("notAField"); + }); + + test("review mode keeps its own field set", async () => { + const id = seed(reviewHandler, { source: "linter", filePath: "a.ts", lineStart: 1, lineEnd: 1, text: "note" }); + expect((await patch(id, { severity: "catastrophic" }, "review")).status).toBe(400); + expect((await patch(id, { severity: "nit" }, "review")).status).toBe(200); + expect((await patch(id, { decorations: ["explode"] }, "review")).status).toBe(400); + expect((await patch(id, { lineStart: "3" }, "review")).status).toBe(400); + }); +}); diff --git a/apps/pi-extension/server/external-annotations.ts b/apps/pi-extension/server/external-annotations.ts index ef0eccdb8..bf4a49a31 100644 --- a/apps/pi-extension/server/external-annotations.ts +++ b/apps/pi-extension/server/external-annotations.ts @@ -15,6 +15,7 @@ import { HEARTBEAT_COMMENT, HEARTBEAT_INTERVAL_MS, validateReplyTarget, + validateAnnotationPatch, type StorableAnnotation, type ExternalAnnotationEvent, } from "../generated/external-annotation.ts"; @@ -154,19 +155,28 @@ export function createExternalAnnotationHandler(mode: "plan" | "review") { json(res, { error: "Invalid JSON" }, 400); return true; } + // Field-level validation with the same validators POST applies: + // unknown keys are dropped and a malformed structured value is a + // 400, never a stored one the renderer then reads a property off + // (`{"diagramAnchor": null}` used to blank the page). + // Mirrors packages/server/external-annotations.ts. + const patch = validateAnnotationPatch(mode, body); + if ("error" in patch) { + json(res, { error: patch.error }, 400); + return true; + } // A reply must point at an existing, different annotation and must // not close a cycle: the export and the panel treat cycle members as // roots, but the invalid state should not be creatable in the first // place. (POST never carries inReplyTo, so PATCH is the only ingest.) - // Mirrors packages/server/external-annotations.ts. - if (body && typeof body === "object" && "inReplyTo" in body) { - const problem = validateReplyTarget(store.getAll(), id, (body as { inReplyTo?: unknown }).inReplyTo); + if ("inReplyTo" in patch.fields) { + const problem = validateReplyTarget(store.getAll(), id, patch.fields.inReplyTo); if (problem) { json(res, { error: problem }, 400); return true; } } - const updated = store.update(id, body as Partial); + const updated = store.update(id, patch.fields as Partial); if (!updated) { json(res, { error: "Not found" }, 404); return true; diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index d1f31a780..33c5c6c9e 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -8,7 +8,7 @@ rm -rf generated mkdir -p generated generated/ai/providers # Modules that MOVED to @plannotator/core — vendor the real impl from core. -for f in feedback-templates project favicon code-file annotatable annotation-threads diagram-anchor external-annotation agent-jobs agent-terminal source-save open-in-apps diff-paths diff-files guide guide-format guide-viewer-manifest compress crypto; do +for f in feedback-templates project favicon code-file annotatable annotation-threads diagram-anchor html-anchor external-annotation agent-jobs agent-terminal source-save open-in-apps diff-paths diff-files guide guide-format guide-viewer-manifest compress crypto; do src="../../packages/core/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/core/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/packages/core/external-annotation.ts b/packages/core/external-annotation.ts index 5804bfe08..8344ca908 100644 --- a/packages/core/external-annotation.ts +++ b/packages/core/external-annotation.ts @@ -14,7 +14,17 @@ // adapters import it from the module they already use. export { validateReplyTarget } from "./annotation-threads"; -import { parseDiagramAnchor, type DiagramAnchor } from "./diagram-anchor"; +import { + parseDiagramAdditionalTargets, + parseDiagramAnchor, + type DiagramAnchor, +} from "./diagram-anchor"; +import { + MAX_PAGE_URL_LENGTH, + parseHtmlAdditionalTargets, + parseHtmlElementAnchor, + parseHtmlElementContext, +} from "./html-anchor"; // --------------------------------------------------------------------------- // Types @@ -527,3 +537,247 @@ export function createAnnotationStore(): Annotatio }, }; } + +// --------------------------------------------------------------------------- +// PATCH validation (shared by both runtimes) +// --------------------------------------------------------------------------- + +/** + * `PATCH /api/external-annotations?id=…` used to merge its body into the + * stored annotation verbatim: an unauthenticated localhost surface could + * write `{"diagramAnchor": null}` — a value POST refuses — and the renderer + * then read `.family` off it and took the page down (#1560 follow-up). + * + * The patch is now allowlisted and field-validated with the SAME validators + * POST applies: a structured field goes through its own fail-closed parser + * and a bad value is a 400, never a stored one. Unknown keys are dropped + * (the wire shape is additive, so an unknown key is a newer or foreign + * writer, not a reason to refuse the whole patch); `id` and `source` stay + * immutable — the store pins them too, this is the outer layer. + * + * Empty after filtering is fine: the PATCH is then a no-op that still + * answers 200 with the annotation, exactly as a patch of unknown keys did. + * `null` on an optional field keeps its established meaning — clear it — and + * is normalized to `undefined` so the stored row never holds a nullish value + * a consumer could read a property off; a required field refuses it. + */ +export type AnnotationPatchMode = "plan" | "review"; + +type FieldValidator = (value: unknown) => { value: unknown } | ParseError; + +/** Cap mirrors the viewer's own diagram multi-select ceiling. */ +const MAX_DIAGRAM_ADDITIONAL_TARGETS = 16; +const MAX_PATCH_IMAGES = 50; +const MAX_PATCH_IMAGE_STRING = 4096; + +const ok = (value: unknown): { value: unknown } => ({ value }); + +const str: FieldValidator = (value) => + typeof value === "string" ? ok(value) : { error: "must be a string" }; + +const bool: FieldValidator = (value) => + typeof value === "boolean" ? ok(value) : { error: "must be a boolean" }; + +const finiteNumber: FieldValidator = (value) => + typeof value === "number" && Number.isFinite(value) + ? ok(value) + : { error: "must be a finite number" }; + +const positiveInt: FieldValidator = (value) => + typeof value === "number" && Number.isSafeInteger(value) && value > 0 + ? ok(value) + : { error: "must be a positive integer" }; + +const oneOf = + (values: readonly string[]): FieldValidator => + (value) => + typeof value === "string" && values.includes(value) + ? ok(value) + : { error: `must be one of: ${values.join(", ")}` }; + +const cappedStr = + (max: number): FieldValidator => + (value) => + typeof value === "string" && value.length <= max + ? ok(value) + : { error: `must be a string of at most ${max} characters` }; + +/** Attached images: `{ path, name }` pairs. */ +const imageList: FieldValidator = (value) => { + if (!Array.isArray(value)) return { error: "must be an array" }; + if (value.length > MAX_PATCH_IMAGES) { + return { error: `must have at most ${MAX_PATCH_IMAGES} entries` }; + } + const out: Array<{ path: string; name: string }> = []; + for (const entry of value) { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) { + return { error: "entries must be { path, name } objects" }; + } + const { path, name } = entry as Record; + if (typeof path !== "string" || path.length === 0 || path.length > MAX_PATCH_IMAGE_STRING) { + return { error: 'entries need a non-empty string "path"' }; + } + if (typeof name !== "string" || name.length > MAX_PATCH_IMAGE_STRING) { + return { error: 'entries need a string "name"' }; + } + out.push({ path, name }); + } + return ok(out); +}; + +/** Parser-backed validators: the stored value is REPLACED by what the parser + * returns, so a merged field is always one the renderer can read. */ +const viaParser = + (parse: (value: unknown) => T | null | undefined, hint: string): FieldValidator => + (value) => { + const parsed = parse(value); + if (parsed === null || parsed === undefined) return { error: hint }; + return ok(parsed); + }; + +/** Lenient array parsers (junk entries are dropped, never fatal) still + * require an array: a scalar is a caller error worth reporting. */ +const viaArrayParser = + (parse: (value: unknown) => T[]): FieldValidator => + (value) => + Array.isArray(value) ? ok(parse(value)) : { error: "must be an array" }; + +const PLAN_PATCH_FIELDS: Record = { + type: oneOf(VALID_PLAN_TYPES), + text: str, + originalText: str, + author: str, + images: imageList, + isQuickLabel: bool, + quickLabelTip: str, + diffContext: oneOf(["added", "removed", "modified"]), + pageUrl: cappedStr(MAX_PAGE_URL_LENGTH), + prUrl: str, + inReplyTo: str, + blockId: str, + startOffset: finiteNumber, + endOffset: finiteNumber, + diagramAnchor: viaParser( + parseDiagramAnchor, + 'expected { v: 1, family, kind, id | from + to, label, sourceLine }', + ), + diagramAdditionalTargets: viaArrayParser((value) => + parseDiagramAdditionalTargets(value, MAX_DIAGRAM_ADDITIONAL_TARGETS), + ), + htmlAnchor: viaParser( + parseHtmlElementAnchor, + 'expected { selector, tagName, text?, point? }', + ), + htmlAdditionalTargets: viaArrayParser((value) => parseHtmlAdditionalTargets(value)), + elementContext: viaParser( + parseHtmlElementContext, + 'expected a bounded element description carrying a "tag"', + ), +}; + +const REVIEW_PATCH_FIELDS: Record = { + type: oneOf(VALID_REVIEW_TYPES), + scope: oneOf(VALID_SCOPES), + side: oneOf(VALID_SIDES), + filePath: str, + lineStart: finiteNumber, + lineEnd: finiteNumber, + charStart: finiteNumber, + charEnd: finiteNumber, + tokenText: str, + selectedText: str, + selectedTextFromEdits: bool, + text: str, + suggestedCode: str, + originalCode: str, + images: imageList, + author: str, + severity: oneOf(["important", "nit", "pre_existing"]), + reasoning: str, + reviewProfileLabel: str, + conventionalLabel: str, + decorations: (value) => { + if (!Array.isArray(value)) return { error: "must be an array" }; + const allowed = ["blocking", "non-blocking", "if-minor"]; + for (const entry of value) { + if (typeof entry !== "string" || !allowed.includes(entry)) { + return { error: `entries must be one of: ${allowed.join(", ")}` }; + } + } + return ok([...value]); + }, + inReplyTo: str, + prUrl: str, + prNumber: positiveInt, + prTitle: str, + prRepo: str, + diffScope: oneOf(["layer", "full-stack"]), + commitSha: str, + commitSubject: str, + gitButlerDiffType: str, + gitButlerDiffLabel: str, + gitButlerBase: str, + gitButlerSnapshotId: str, +}; + +/** + * Fields that carry the annotation's structure rather than its content: a + * PATCH may change them, never clear them. An annotation with no `type`, or a + * line comment with no `filePath`, is not a thing the UI can render — and the + * anchors are the crash vector this validator exists for, so `null` on one is + * the caller error it looks like (400), not a silent un-anchoring. + */ +const NON_CLEARABLE_FIELDS = new Set([ + "type", + "originalText", + "blockId", + "startOffset", + "endOffset", + "scope", + "side", + "filePath", + "lineStart", + "lineEnd", + "diagramAnchor", + "diagramAdditionalTargets", + "htmlAnchor", + "htmlAdditionalTargets", + "elementContext", +]); + +/** + * Validate and narrow a PATCH body for `mode`. Returns the fields that may be + * merged into the stored annotation, or a `ParseError` naming the field whose + * value failed its validator. + */ +export function validateAnnotationPatch( + mode: AnnotationPatchMode, + body: unknown, +): { fields: Record } | ParseError { + if (!body || typeof body !== "object" || Array.isArray(body)) { + return { error: "Request body must be a JSON object" }; + } + const fieldsByName = mode === "plan" ? PLAN_PATCH_FIELDS : REVIEW_PATCH_FIELDS; + const fields: Record = {}; + for (const [key, value] of Object.entries(body as Record)) { + // Identity fields are pinned (the store deletes them too). + if (key === "id" || key === "source" || key === "__proto__") continue; + const validator = fieldsByName[key]; + // Unknown key: dropped, not fatal — the wire shape is additive. + if (!validator) continue; + // `undefined` never survives JSON, but a host calling the validator + // directly may pass it: treat it as "not provided". + if (value === undefined) continue; + if (value === null) { + if (NON_CLEARABLE_FIELDS.has(key)) { + return { error: `invalid "${key}": must not be null` }; + } + fields[key] = undefined; + continue; + } + const result = validator(value); + if ("error" in result) return { error: `invalid "${key}": ${result.error}` }; + fields[key] = result.value; + } + return { fields }; +} diff --git a/packages/server/external-annotations.test.ts b/packages/server/external-annotations.test.ts index c26fcd86f..84d816d55 100644 --- a/packages/server/external-annotations.test.ts +++ b/packages/server/external-annotations.test.ts @@ -126,3 +126,153 @@ describe("PATCH /api/external-annotations", () => { expect((await patch(second, { text: "still fine" })).status).toBe(200); }); }); + +// --------------------------------------------------------------------------- +// PATCH body validation (#1560 follow-up) +// --------------------------------------------------------------------------- + +/** + * PATCH used to merge its body verbatim, so it could store values POST + * refuses. `{"diagramAnchor": null}` was answered 200 and then took the whole + * page down when the renderer read `.family` off it. Every field a PATCH can + * set now runs the validator POST runs. + */ +describe("PATCH /api/external-annotations: body validation", () => { + const seed = (handler: ReturnType, body: unknown) => { + const added = handler.addAnnotations(body); + if ("error" in added) throw new Error(added.error); + return added.ids[0]!; + }; + + const patchWith = (handler: ReturnType, id: string) => + async (body: unknown) => { + const url = `http://localhost/api/external-annotations?id=${encodeURIComponent(id)}`; + const res = await handler.handle( + new Request(url, { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body) }), + new URL(url), + ); + return { + status: res!.status, + body: (await res!.json()) as { error?: string; annotation?: Record }, + }; + }; + + const VALID_ANCHOR = { v: 1, family: "flowchart", kind: "node", id: "D", label: "Approve?", sourceLine: [7, 7] }; + + test("refuses every malformed diagramAnchor and never stores one", async () => { + const handler = createExternalAnnotationHandler("plan"); + const id = seed(handler, { + source: "linter", + type: "COMMENT", + text: "external finding", + originalText: "Approve?", + diagramAnchor: VALID_ANCHOR, + }); + const patch = patchWith(handler, id); + + // The reported crash vector first: `null` answered 200 and blanked the page. + const nulled = await patch({ diagramAnchor: null }); + expect(nulled.status).toBe(400); + expect(nulled.body.error).toContain("diagramAnchor"); + + for (const bad of [ + "nope", + 7, + {}, + [], + { ...VALID_ANCHOR, v: 2 }, + { ...VALID_ANCHOR, family: "not-a-family" }, + { ...VALID_ANCHOR, kind: "node", id: undefined, from: undefined, to: undefined }, + ]) { + const res = await patch({ diagramAnchor: bad }); + expect(res.status).toBe(400); + } + + // The stored anchor is untouched by every refused patch. + const url = "http://localhost/api/external-annotations"; + const snapshot = (await (await handler.handle(new Request(url), new URL(url)))!.json()) as { + annotations: Array<{ id: string; diagramAnchor?: unknown }>; + }; + expect(snapshot.annotations.find((a) => a.id === id)?.diagramAnchor).toEqual(VALID_ANCHOR); + }); + + test("accepts a valid diagramAnchor and normalizes it through the parser", async () => { + const handler = createExternalAnnotationHandler("plan"); + const id = seed(handler, { source: "linter", text: "note" }); + const patch = patchWith(handler, id); + + const ok = await patch({ diagramAnchor: { ...VALID_ANCHOR, label: "x".repeat(600), stowaway: "dropped" } }); + expect(ok.status).toBe(200); + const stored = ok.body.annotation?.diagramAnchor as Record; + // Parser caps (400) and drops unknown keys — the same normalization POST applies. + expect((stored.label as string).length).toBe(400); + expect(stored).not.toHaveProperty("stowaway"); + + // An out-of-range source line drops to null while the target survives. + const negative = await patch({ diagramAnchor: { ...VALID_ANCHOR, sourceLine: [-3, -3] } }); + expect(negative.status).toBe(200); + expect((negative.body.annotation?.diagramAnchor as { sourceLine: unknown }).sourceLine).toBeNull(); + }); + + test("validates the other structured fields and the scalars", async () => { + const handler = createExternalAnnotationHandler("plan"); + const id = seed(handler, { source: "linter", text: "note" }); + const patch = patchWith(handler, id); + + expect((await patch({ htmlAnchor: null })).status).toBe(400); + expect((await patch({ htmlAnchor: { tagName: "div" } })).status).toBe(400); + expect((await patch({ htmlAnchor: { selector: "#a", tagName: "div" } })).status).toBe(200); + + expect((await patch({ elementContext: { id: "no-tag" } })).status).toBe(400); + expect((await patch({ elementContext: { tag: "BUTTON" } })).status).toBe(200); + + expect((await patch({ images: "nope" })).status).toBe(400); + expect((await patch({ images: [{ name: "a" }] })).status).toBe(400); + expect((await patch({ images: [{ path: "/tmp/a.png", name: "a" }] })).status).toBe(200); + + expect((await patch({ type: "NOT_A_TYPE" })).status).toBe(400); + expect((await patch({ type: "DELETION" })).status).toBe(200); + expect((await patch({ text: 7 })).status).toBe(400); + expect((await patch({ originalText: {} })).status).toBe(400); + expect((await patch({ pageUrl: "/a".padEnd(4000, "b") })).status).toBe(400); + expect((await patch({ pageUrl: "/settings?tab=2" })).status).toBe(200); + }); + + test("drops unknown keys instead of storing them, and keeps id immutable", async () => { + const handler = createExternalAnnotationHandler("plan"); + const id = seed(handler, { source: "linter", text: "note" }); + const patch = patchWith(handler, id); + + const res = await patch({ text: "edited", notAField: { deep: true }, id: "hijacked" }); + expect(res.status).toBe(200); + expect(res.body.annotation?.text).toBe("edited"); + expect(res.body.annotation?.id).toBe(id); + expect(res.body.annotation).not.toHaveProperty("notAField"); + }); + + test("refuses a non-object body", async () => { + const handler = createExternalAnnotationHandler("plan"); + const id = seed(handler, { source: "linter", text: "note" }); + const patch = patchWith(handler, id); + expect((await patch([{ text: "x" }])).status).toBe(400); + expect((await patch("text")).status).toBe(400); + expect((await patch(null)).status).toBe(400); + }); + + test("review mode keeps its own field set", async () => { + const handler = createExternalAnnotationHandler("review"); + const id = seed(handler, { source: "linter", filePath: "a.ts", lineStart: 1, lineEnd: 1, text: "note" }); + const patch = patchWith(handler, id); + + expect((await patch({ severity: "catastrophic" })).status).toBe(400); + expect((await patch({ severity: "nit" })).status).toBe(200); + expect((await patch({ decorations: ["blocking"] })).status).toBe(200); + expect((await patch({ decorations: ["explode"] })).status).toBe(400); + expect((await patch({ suggestedCode: "const a = 1;" })).status).toBe(200); + expect((await patch({ lineStart: "3" })).status).toBe(400); + // A plan-only field is not a review field: dropped, not stored. + const dropped = await patch({ diagramAnchor: VALID_ANCHOR }); + expect(dropped.status).toBe(200); + expect(dropped.body.annotation).not.toHaveProperty("diagramAnchor"); + }); +}); diff --git a/packages/server/external-annotations.ts b/packages/server/external-annotations.ts index 3cf2eb964..8093c1b0c 100644 --- a/packages/server/external-annotations.ts +++ b/packages/server/external-annotations.ts @@ -17,6 +17,7 @@ import { HEARTBEAT_COMMENT, HEARTBEAT_INTERVAL_MS, validateReplyTarget, + validateAnnotationPatch, type AnnotationStore, type StorableAnnotation, type ExternalAnnotationEvent, @@ -176,15 +177,23 @@ export function createExternalAnnotationHandler( } catch { return Response.json({ error: "Invalid JSON" }, { status: 400 }); } + // Field-level validation with the same validators POST applies: + // unknown keys are dropped and a malformed structured value is a 400, + // never a stored one that the renderer then reads a property off + // (`{"diagramAnchor": null}` used to blank the page). + const patch = validateAnnotationPatch(mode, body); + if ("error" in patch) { + return Response.json({ error: patch.error }, { status: 400 }); + } // A reply must point at an existing, different annotation and must // not close a cycle: the export and the panel treat cycle members as // roots, but the invalid state should not be creatable in the first // place. (POST never carries inReplyTo, so PATCH is the only ingest.) - if (body && typeof body === "object" && "inReplyTo" in body) { - const problem = validateReplyTarget(store.getAll(), id, (body as { inReplyTo?: unknown }).inReplyTo); + if ("inReplyTo" in patch.fields) { + const problem = validateReplyTarget(store.getAll(), id, patch.fields.inReplyTo); if (problem) return Response.json({ error: problem }, { status: 400 }); } - const updated = store.update(id, body as Partial); + const updated = store.update(id, patch.fields as Partial); if (!updated) { return Response.json({ error: "Not found" }, { status: 404 }); } diff --git a/packages/ui/components/DiagramBlock.tsx b/packages/ui/components/DiagramBlock.tsx index 828d2eacb..6bba8c642 100644 --- a/packages/ui/components/DiagramBlock.tsx +++ b/packages/ui/components/DiagramBlock.tsx @@ -134,19 +134,23 @@ export const DiagramBlock: React.FC = const claims = sharedClaims ?? ownClaims; const claimsVersion = useSyncExternalStore(claims.subscribe, claims.getVersion, claims.getVersion); + // `diagramAnchor` is read defensively everywhere: a row can reach the + // renderer from any local ingest (the external-annotations API, a draft, a + // share link), so a nullish or malformed anchor must list as unanchored, + // never take the page down with a property read (`null.family`). const ownAnnotations = useMemo( - () => annotations.filter((ann) => ann.diagramAnchor !== undefined && ann.blockId === block.id), + () => annotations.filter((ann) => ann.diagramAnchor != null && ann.blockId === block.id), [annotations, block.id], ); const unownedAnnotations = useMemo( () => annotations.filter( (ann) => - ann.diagramAnchor !== undefined && + ann.diagramAnchor != null && ann.blockId !== block.id && !claims.blockIds.includes(ann.blockId) && // A Graphviz anchor names a DOT part; it is never a Mermaid one. - (ann.diagramAnchor.family === 'graphviz') === (kind === 'graphviz'), + (ann.diagramAnchor?.family === 'graphviz') === (kind === 'graphviz'), ), [annotations, block.id, claims, kind], ); diff --git a/packages/ui/components/Viewer.diagramAnchorHostile.test.tsx b/packages/ui/components/Viewer.diagramAnchorHostile.test.tsx new file mode 100644 index 000000000..0baec87bb --- /dev/null +++ b/packages/ui/components/Viewer.diagramAnchorHostile.test.tsx @@ -0,0 +1,184 @@ +/** + * A stored annotation whose `diagramAnchor` is not an anchor must never take + * the document down. + * + * `PATCH /api/external-annotations` merged its body verbatim, so any local + * process could store `{"diagramAnchor": null}` — a value POST refuses — and + * the diagram block then read `.family` off it: `TypeError: Cannot read + * properties of null`, thrown during render, the whole page blank. The server + * validator closes the hole; this is the second layer, because a row can also + * reach the renderer from a draft, a share link, or a future writer. + * + * What regresses if this fails: one malformed row blanks the page instead of + * listing as an ordinary comment. + * + * DOM-gated (DOM_TESTS=1). + */ +import { afterAll, afterEach, beforeAll, describe, expect, test } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import React, { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { installInertDiagramSvgParser } from '../test-setup/diagramSvg'; +import { AnnotationType, type Annotation } from '../types'; +import { parseMarkdownToBlocks } from '../utils/parser'; + +const hasDom = typeof document !== 'undefined'; + +// Viewer pulls in web-highlighter, whose UMD bundle reads `window` at +// module-eval time; import lazily (same pattern as Viewer.diagramLazyRestore). +const viewerMod = hasDom ? await import('./Viewer') : null; +const Viewer = viewerMod?.Viewer as typeof import('./Viewer')['Viewer']; +const panelMod = hasDom ? await import('./AnnotationPanel') : null; +const AnnotationPanel = panelMod?.AnnotationPanel as typeof import('./AnnotationPanel')['AnnotationPanel']; +const mermaidMod = hasDom ? await import('./MermaidBlock') : null; +const setLoader = mermaidMod?.__setMermaidRuntimeLoaderForTests as + typeof import('./MermaidBlock')['__setMermaidRuntimeLoaderForTests']; + +const FIXTURES = join(import.meta.dir, '..', 'test-setup', 'fixtures', 'diagrams'); +const CAPTURE_ID = 'diagram-fixture'; +const SVG = readFileSync(join(FIXTURES, '06-flowchart-review-decision.svg'), 'utf8'); +const GEOMETRY = JSON.parse(readFileSync(join(FIXTURES, '06-flowchart-review-decision.geometry.json'), 'utf8')) as { + elements: Record; +}; +const CAPTURED = Object.entries(GEOMETRY.elements).map(([id, entry]) => [id.slice(CAPTURE_ID.length), entry] as const); + +const PROSE_AFTER = 'Some prose after the diagram.'; +const MARKDOWN = [ + '# Plan', + '', + 'Some prose before the diagram.', + '', + '```mermaid', + 'flowchart LR', + ' U([Reviewer]) --> D{Approve?}', + ' D -->|Yes| M[(Merge)]', + ' D -->|No| R[Revise]', + '```', + '', + PROSE_AFTER, +].join('\n'); + +const BLOCKS = hasDom ? parseMarkdownToBlocks(MARKDOWN) : []; +const FENCE_ID = hasDom ? (BLOCKS.find((b) => b.type === 'code')?.id ?? '') : ''; + +const row = (id: string, diagramAnchor: unknown): Annotation => + ({ + id, + blockId: FENCE_ID, + startOffset: 0, + endOffset: 0, + type: AnnotationType.COMMENT, + text: 'external finding', + originalText: 'Approve?', + createdA: 1, + source: 'linter', + diagramAnchor, + }) as unknown as Annotation; + +/** Exactly the shapes an unvalidated PATCH could store. `null` is the one + * that threw; the rest are inert today and must stay that way. */ +const HOSTILE: Annotation[] = [ + row('h-null', null), + row('h-string', 'nope'), + row('h-number', 7), + row('h-empty-object', {}), + row('h-wrong-v', { v: 2, family: 'flowchart', kind: 'node', id: 'D', label: 'Approve?', sourceLine: [7, 7] }), + row('h-unknown-family', { v: 1, family: 'not-a-family', kind: 'node', id: 'D', label: 'Approve?', sourceLine: [7, 7] }), +]; + +let root: Root | null = null; +let host: HTMLElement | null = null; +let restoreParser: (() => void) | null = null; +const svgProto = (hasDom ? ((globalThis as { SVGGraphicsElement?: typeof SVGElement }).SVGGraphicsElement ?? SVGElement).prototype : {}) as unknown as Record; +const elementProto = (hasDom ? Element.prototype : {}) as unknown as Record; +const saved = { getBBox: svgProto['getBBox'], getScreenCTM: svgProto['getScreenCTM'] }; +const noop = (): void => {}; + +beforeAll(() => { + if (!hasDom) return; + restoreParser = installInertDiagramSvgParser(); + elementProto['setPointerCapture'] ??= noop; + elementProto['releasePointerCapture'] ??= noop; + elementProto['hasPointerCapture'] ??= () => false; + svgProto['getBBox'] = function (this: Element) { + if (this.tagName.toLowerCase() === 'svg') return { x: 0, y: 0, width: 452, height: 182 }; + const found = CAPTURED.find(([suffix]) => this.id.endsWith(suffix)); + return found === undefined ? { x: 0, y: 0, width: 0, height: 0 } : { ...found[1].bbox }; + }; + svgProto['getScreenCTM'] = function (this: Element) { + if (this.tagName.toLowerCase() === 'svg') return { a: 1, b: 0, c: 0, d: 1, e: 0, f: 0 }; + return CAPTURED.find(([suffix]) => this.id.endsWith(suffix))?.[1].ctm ?? null; + }; + setLoader((async () => ({ + initialize: noop, + render: (id: string) => Promise.resolve({ svg: SVG.replaceAll(CAPTURE_ID, id) }), + })) as never); +}); + +afterAll(() => { + if (!hasDom) return; + svgProto['getBBox'] = saved.getBBox; + svgProto['getScreenCTM'] = saved.getScreenCTM; + setLoader(undefined); + restoreParser?.(); +}); + +afterEach(async () => { + if (root !== null) { + const finished = root; + await act(async () => { finished.unmount(); }); + root = null; + } + host?.remove(); + host = null; + if (hasDom) document.body.innerHTML = ''; +}); + +async function settle(ms = 25): Promise { + await act(async () => { await new Promise((resolve) => setTimeout(resolve, ms)); }); +} + +describe.if(hasDom)('Viewer: an annotation carrying a malformed diagramAnchor', () => { + test('renders the document and lists the row instead of throwing', async () => { + host = document.createElement('div'); + document.body.appendChild(host); + + await act(async () => { + root = createRoot(host!); + root.render( + <> + {}} + onSelectAnnotation={() => {}} + selectedAnnotationId={null} + mode="comment" + taterMode={false} + disableCodePathValidation + /> + {}} + onDelete={() => {}} + /> + , + ); + }); + await settle(80); + + // The document is on screen — the crash blanked it entirely + // (`document.body.innerText.length === 0`, root with no children). + expect(host!.textContent).toContain(PROSE_AFTER); + expect(host!.querySelector('[data-diagram-block]')).not.toBeNull(); + // Every hostile row is still readable in the panel. + for (const ann of HOSTILE) { + expect(host!.querySelector(`[data-annotation-panel] [data-annotation-id="${ann.id}"]`)).not.toBeNull(); + } + }); +}); diff --git a/packages/ui/components/Viewer.tsx b/packages/ui/components/Viewer.tsx index 211f98979..45c47c400 100644 --- a/packages/ui/components/Viewer.tsx +++ b/packages/ui/components/Viewer.tsx @@ -1071,7 +1071,10 @@ export const Viewer = forwardRef(({ // it is unanchored, and the highlighter (which skips it) will not say so. useEffect(() => { if (diagramBlockKey !== '' || onRestoreReport === undefined) return; - const ids = annotations.filter((ann) => ann.diagramAnchor !== undefined).map((ann) => ann.id); + // `!= null`, not `!== undefined`: a nullish anchor is no anchor at all — + // the highlighter restores such a row by text, so it must not be counted + // here as a diagram comment nothing could resolve. + const ids = annotations.filter((ann) => ann.diagramAnchor != null).map((ann) => ann.id); if (ids.length > 0) onRestoreReport({ attempted: ids, unanchored: ids }); }, [annotations, diagramBlockKey, onRestoreReport]); From d13b50daf873c01f4c5e591659a95d79c2667a1b Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 18:09:56 -0700 Subject: [PATCH 2/5] fix(code-nav): keep the root vendor/ tree excluded for a first-party package of the same name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The origin-file exemption (#1558) lifted a directory exclusion whenever the ORIGIN path contained that segment anywhere, which lifted it tree-wide: a request from `src/main/java/com/example/vendor/app/Widget.java` also returned matches from the repo-root `vendor/` third-party tree it exists to exclude. Narrowed to the directory INSTANCE the origin lives in: - a root-only glob (`!/vendor`) is only lifted when the origin's FIRST segment is that directory — a deep same-named package was never pruned by it anyway; - `isCodeNavPathAllowed` post-filters every result, so an always-ignored name lifted for an origin under one `node_modules` no longer returns matches from a different one. A file that really lives under an excluded root still finds its own siblings. --- packages/shared/code-nav.test.ts | 86 ++++++++++++++++++++++++++++++-- packages/shared/code-nav.ts | 63 +++++++++++++++++++++-- 2 files changed, 143 insertions(+), 6 deletions(-) diff --git a/packages/shared/code-nav.test.ts b/packages/shared/code-nav.test.ts index 1c5a8b2d1..3b039b899 100644 --- a/packages/shared/code-nav.test.ts +++ b/packages/shared/code-nav.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { buildRgArgs, + isCodeNavPathAllowed, resolveCodeNav, buildSignature, classifyMatch, @@ -988,18 +989,28 @@ describe("buildRgArgs directory exclusions (#1558)", () => { } }); - test("a segment the origin file lives under is not excluded for that request", () => { + // The anchored glob only ever prunes the SEARCH ROOT, so an origin in a + // first-party `…/vendor/app/` package needs nothing lifted — and lifting it + // handed that request the real root `vendor/` tree back (#1559 follow-up). + test("a deep same-named package does not lift the root-only exclusion", () => { const args = buildRgArgs( "fetchContent", "java", "src/main/java/com/example/vendor/app/ExampleService.java", ); - expect(args).not.toContain("!/vendor"); - // Unrelated exclusions are untouched. + expect(args).toContain("!/vendor"); expect(args).toContain("!node_modules"); expect(args).toContain("!/target"); }); + test("an origin that really lives under the root directory lifts it", () => { + const args = buildRgArgs("fetchContent", "java", "vendor/third_party/Vendored.java"); + expect(args).not.toContain("!/vendor"); + // Only that one name: the other root-only exclusions stand. + expect(args).toContain("!/target"); + expect(args).toContain("!node_modules"); + }); + test("the origin-file rule also lifts an always-ignored segment", () => { const args = buildRgArgs("x", undefined, "packages/node_modules/dep/index.js"); expect(args).not.toContain("!node_modules"); @@ -1056,6 +1067,8 @@ describeRg("resolveCodeNav against real ripgrep (#1558)", () => { `${root}/src/app/node_modules/dep/Dep.java`, javaSource("Dep"), ); + // A neutral first-party file, the third of the QA repro's three. + await Bun.write(`${root}/src/other/Other.java`, javaSource("Other")); }); afterAll(async () => { @@ -1103,4 +1116,71 @@ describeRg("resolveCodeNav against real ripgrep (#1558)", () => { const found = paths(await resolve("src/app/node_modules/dep/Caller.java")); expect(found.some((p) => p.includes("node_modules/dep/Dep.java"))).toBe(true); }); + + // The QA repro (#1559 follow-up): three files define the same symbol — a + // first-party package whose path contains `vendor`, the repo-root `vendor/` + // tree, and a neutral file. From the package origin the ROOT match was + // returned, because the origin-file rule lifted the anchored glob. + test("a first-party `vendor` package does not un-exclude the root vendor tree", async () => { + const found = paths(await resolve("src/main/java/com/example/vendor/app/ExampleService.java")); + expect(found.some((p) => p.includes("com/example/vendor/app/ExampleService.java"))).toBe(true); + expect(found.some((p) => p.includes("src/other/"))).toBe(true); + expect(found.some((p) => p.includes("vendor/third_party/"))).toBe(false); + }); + + test("a file that really lives under root vendor/ still finds its own siblings", async () => { + const found = paths(await resolve("vendor/third_party/Caller.java")); + expect(found.some((p) => p.includes("vendor/third_party/Vendored.java"))).toBe(true); + // The exemption is that directory only: node_modules stays excluded. + expect(found.some((p) => p.includes("node_modules"))).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// Origin-file exemption is per directory INSTANCE (#1559 follow-up) +// --------------------------------------------------------------------------- + +/** + * The exemption exists so a symbol in a changed file finds its own siblings + * even under a directory whose NAME looks like tool output. Lifting the glob + * lifted it tree-wide, so a request from a first-party `…/vendor/app/` package + * also returned the repo-root `vendor/` third-party tree. The rule is now: a + * match under an excluded directory is kept only when that same directory is + * an ancestor of the origin file. + */ +describe("isCodeNavPathAllowed", () => { + const DEEP_ORIGIN = "src/main/java/com/example/vendor/app/Widget.java"; + + test("the root-level excluded tree stays excluded for a deep same-named package", () => { + expect(isCodeNavPathAllowed("vendor/RootVendorFile.java", DEEP_ORIGIN)).toBe(false); + // …while the origin's own package is untouched (it was never pruned). + expect(isCodeNavPathAllowed("src/main/java/com/example/vendor/app/Other.java", DEEP_ORIGIN)).toBe(true); + expect(isCodeNavPathAllowed("src/other/Other.java", DEEP_ORIGIN)).toBe(true); + }); + + test("a file that really lives under the root directory keeps its siblings", () => { + const origin = "vendor/RootVendorFile.java"; + expect(isCodeNavPathAllowed("vendor/Sibling.java", origin)).toBe(true); + expect(isCodeNavPathAllowed("vendor/nested/Deep.java", origin)).toBe(true); + // A DIFFERENT excluded root is still excluded for it. + expect(isCodeNavPathAllowed("dist/Built.java", origin)).toBe(false); + }); + + test("always-ignored names are per instance too", () => { + const origin = "packages/app/node_modules/dep/index.js"; + expect(isCodeNavPathAllowed("packages/app/node_modules/dep/other.js", origin)).toBe(true); + expect(isCodeNavPathAllowed("packages/lib/node_modules/dep/index.js", origin)).toBe(false); + expect(isCodeNavPathAllowed("node_modules/dep/index.js", origin)).toBe(false); + }); + + test("with no origin, and for ordinary paths, the plain rules apply", () => { + expect(isCodeNavPathAllowed("vendor/x.java")).toBe(false); + expect(isCodeNavPathAllowed("src/x.java")).toBe(true); + // A nested `vendor/` is not root-level, so the anchored rule leaves it. + expect(isCodeNavPathAllowed("src/vendor/x.java")).toBe(true); + // A FILE named like an excluded directory is a file, not a directory. + expect(isCodeNavPathAllowed("src/vendor")).toBe(true); + expect(isCodeNavPathAllowed("./vendor/x.java")).toBe(false); + expect(isCodeNavPathAllowed("vendor\\x.java")).toBe(false); + }); }); diff --git a/packages/shared/code-nav.ts b/packages/shared/code-nav.ts index 53ce5c180..75c387169 100644 --- a/packages/shared/code-nav.ts +++ b/packages/shared/code-nav.ts @@ -283,7 +283,58 @@ function isTestFile(filePath: string): boolean { /** Path segments of a repo-relative file path, `/` and `\` alike. */ function pathSegments(filePath: string): Set { - return new Set(filePath.split(/[/\\]/).filter(Boolean)); + return new Set(splitPath(filePath)); +} + +/** Ordered path segments, `/` and `\` alike, with `.` and empties dropped. */ +function splitPath(filePath: string): string[] { + return filePath.split(/[/\\]/).filter((segment) => segment && segment !== "."); +} + +/** + * Is `matchPath` allowed for a request that came from `originFilePath`? + * + * The origin-file exemption (#1558) lifts a directory exclusion so a symbol in + * a changed file can always find its own siblings. Lifting the GLOB, though, + * lifts it for the whole tree: a request from + * `src/main/java/com/example/vendor/app/Widget.java` also started returning + * matches from the repo-ROOT `vendor/`, which is exactly the third-party tree + * the exclusion exists for (#1559 follow-up). + * + * So the exemption is narrowed to the directory INSTANCE the origin lives in: + * a match under an excluded directory is kept only when that same directory is + * an ancestor of the origin file. Root-only names are additionally only + * considered at the search root, which is where `--glob !/vendor` prunes. + */ +export function isCodeNavPathAllowed( + matchPath: string, + originFilePath?: string, +): boolean { + const segments = splitPath(matchPath); + const origin = originFilePath ? splitPath(originFilePath) : []; + // Only directory segments can be excluded; the last segment is the file. + for (let i = 0; i < segments.length - 1; i++) { + const name = segments[i]!; + const excluded = + CODE_NAV_ALWAYS_IGNORED_DIRS.includes(name) || + (i === 0 && CODE_NAV_ROOT_ONLY_IGNORED_DIRS.includes(name)); + if (!excluded) continue; + if (!isAncestorOfOrigin(segments, origin, i)) return false; + } + return true; +} + +/** Do `segments[0..i]` name the same directory the origin sits under? */ +function isAncestorOfOrigin( + segments: string[], + origin: string[], + i: number, +): boolean { + if (origin.length <= i + 1) return false; + for (let j = 0; j <= i; j++) { + if (origin[j] !== segments[j]) return false; + } + return true; } export function buildRgArgs( @@ -311,6 +362,12 @@ export function buildRgArgs( const originSegments = originFilePath ? pathSegments(originFilePath) : new Set(); + // Which directory the origin file sits at the TOP of, if any. A root-only + // glob prunes the search root alone, so only an origin that actually lives + // under that root directory needs the exclusion lifted — a first-party + // package deeper in the tree (`src/…/vendor/app/`) was never pruned by it, + // and lifting it for that request un-excluded the real `vendor/` (#1559). + const originRoot = originFilePath ? (splitPath(originFilePath)[0] ?? "") : ""; for (const dir of CODE_NAV_ALWAYS_IGNORED_DIRS) { if (originSegments.has(dir)) continue; @@ -318,7 +375,7 @@ export function buildRgArgs( } for (const dir of CODE_NAV_ROOT_ONLY_IGNORED_DIRS) { - if (originSegments.has(dir)) continue; + if (originRoot === dir) continue; // Leading slash anchors the glob to the search root, so only a top-level // `vendor/` (etc.) is pruned — not a same-named package deeper in the tree. args.push("--glob", `!/${dir}`); @@ -579,7 +636,7 @@ export async function resolveCodeNav( result.stdout, request.symbol, request.language, - ); + ).filter((loc) => isCodeNavPathAllowed(loc.filePath, request.filePath)); const ranked = rankLocations(locations, { sourceFilePath: request.filePath, From fc61ee846a9d5c10ea5799eb700070028ea98295 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 17 Sep 2026 18:25:37 -0700 Subject: [PATCH 3/5] fix(print): print the light half of the palette, so a dark-theme page prints on white paper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The print stylesheet has always assumed white paper: it paints the ground white and the text near-black. A dark-palette page kept its own tokens under it, and #1560's diagram engine made that visible — Mermaid 12 draws node and edge labels as real HTML inside ``, so the blanket `div, span, p { color: #1a1a1a !important }` repainted them near-black on a near-black node fill and the flowchart printed as empty boxes. - ThemeProvider renders the LIGHT half of the user's pair while printing. The class write happens inside the `beforeprint` handler, because a real print snapshot is taken before React would flush; the `print` media query drives the same switch for headless emulation and preview, where the async Mermaid re-render also lands. The stored preference is never touched, and a light-mode user sees no change. - print.css exempts diagram content from the typography rules (`:not([data-diagram-block] *)`): a diagram colours itself, and its own `