Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 29 additions & 4 deletions AGENTS.md

Large diffs are not rendered by default.

117 changes: 117 additions & 0 deletions apps/pi-extension/server/external-annotations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>((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<string, unknown> },
};
};

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<string, unknown>;
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);
});
});
18 changes: 14 additions & 4 deletions apps/pi-extension/server/external-annotations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
HEARTBEAT_COMMENT,
HEARTBEAT_INTERVAL_MS,
validateReplyTarget,
validateAnnotationPatch,
type StorableAnnotation,
type ExternalAnnotationEvent,
} from "../generated/external-annotation.ts";
Expand Down Expand Up @@ -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<StorableAnnotation>);
const updated = store.update(id, patch.fields as Partial<StorableAnnotation>);
if (!updated) {
json(res, { error: "Not found" }, 404);
return true;
Expand Down
2 changes: 1 addition & 1 deletion apps/pi-extension/vendor.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading