From 8c0cc708907d94a0bcf152a7f70695aa83114e26 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Sun, 16 Aug 2026 14:39:45 -0400 Subject: [PATCH] fix(api): reserve user feedback author --- .changeset/tidy-authors-rest.md | 5 ++++ bin/sideshow.js | 12 +++------ extensions/sideshow.js | 7 +----- mcp/server.ts | 2 +- server/app.ts | 25 +++++++++++++++---- server/mcpHttp.ts | 9 ++----- server/mcpSpec.ts | 1 - server/sqlStore.ts | 3 ++- server/storage.ts | 3 ++- server/types.ts | 7 ++++++ test/api.test.ts | 36 ++++++++++++++++++++++++++- test/cli.test.ts | 22 ++++++++-------- test/feedbackSqlStore.test.ts | 2 +- test/mcpStdio.test.ts | 2 +- test/piExtension.test.ts | 6 ++--- test/storeContract.ts | 6 +++++ test/workerIntegration.integration.ts | 2 +- 17 files changed, 101 insertions(+), 49 deletions(-) create mode 100644 .changeset/tidy-authors-rest.md diff --git a/.changeset/tidy-authors-rest.md b/.changeset/tidy-authors-rest.md new file mode 100644 index 00000000..4e68f196 --- /dev/null +++ b/.changeset/tidy-authors-rest.md @@ -0,0 +1,5 @@ +--- +"sideshow": patch +--- + +Comment authors are now derived from the session agent for CLI, MCP, and other programmatic writes. The reserved `user` label is limited to same-origin viewer comments, preventing agent integrations from forging user feedback. diff --git a/bin/sideshow.js b/bin/sideshow.js index 98112269..19b7c26d 100755 --- a/bin/sideshow.js +++ b/bin/sideshow.js @@ -131,7 +131,6 @@ usage: sideshow comment [options] reply to the user on a post --post post to attach the comment to (required; --surface is a deprecated alias) - --author defaults to agent name sideshow list [--session |--all] list posts sideshow show show a single post (surfaces, indexes, ids, version, history) sideshow sessions list sessions @@ -1506,8 +1505,6 @@ const commands = { post: { type: "string" }, surface: { type: "string" }, // deprecated alias snippet: { type: "string" }, // legacy alias - author: { type: "string" }, - agent: { type: "string" }, }, }); const text = positionals.join(" ").trim(); @@ -1519,11 +1516,7 @@ const commands = { out( await api("/api/comments", { method: "POST", - body: JSON.stringify({ - text, - surface: post, - author: flags.author ?? agentName(flags), - }), + body: JSON.stringify({ text, surface: post }), }), ); }, @@ -1589,8 +1582,11 @@ const commands = { }); } if (step.comment) { + // Demo comments model a person using the viewer. Normal CLI writes + // never send an author, so they derive the session agent instead. await api("/api/comments", { method: "POST", + headers: { "sec-fetch-site": "same-origin" }, body: JSON.stringify({ surface: post.id, ...step.comment }), }); } diff --git a/extensions/sideshow.js b/extensions/sideshow.js index 31bfb8d5..3aa061ee 100644 --- a/extensions/sideshow.js +++ b/extensions/sideshow.js @@ -577,16 +577,11 @@ export default function sideshowExtension(pi) { properties: { surfaceId: { type: "string", description: "Surface thread to reply under" }, message: { type: "string", description: "Plain-text reply" }, - author: { type: "string", description: 'Agent name; defaults to SIDESHOW_AGENT or "pi"' }, }, required: ["surfaceId", "message"], }, async execute(_toolCallId, params) { - const body = { - text: params.message, - surface: params.surfaceId, - author: params.author ?? agentName(), - }; + const body = { text: params.message, surface: params.surfaceId }; const comment = await requestJson("/api/comments", { method: "POST", body: JSON.stringify(body), diff --git a/mcp/server.ts b/mcp/server.ts index a3c91cdf..e5776d3a 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -213,7 +213,7 @@ server.registerTool( const created = JSON.parse( await api("/api/comments", { method: "POST", - body: JSON.stringify({ surface: postId ?? surfaceId, text: message, author: AGENT }), + body: JSON.stringify({ surface: postId ?? surfaceId, text: message }), }), ); return text(created); diff --git a/server/app.ts b/server/app.ts index 71e9415f..0067a833 100644 --- a/server/app.ts +++ b/server/app.ts @@ -33,6 +33,7 @@ import { type DiffSurface, htmlSurface, isSandboxedSurfaceKind, + reservedAgent, type MarkdownSurface, MAX_ASSET_BYTES, surfacesByteLength, @@ -709,7 +710,9 @@ export function createApp({ async function createComment(input: { text: string; surface?: string; - author: string; + // Viewer-originated comments may set "user" or "surface". All agent + // channels omit this and derive their author from the owning session. + author?: "user" | "surface"; anchor?: unknown; }): Promise< { comment: Comment; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 } @@ -719,10 +722,13 @@ export function createApp({ if (!input.surface) return { error: 'provide a "surface" id', status: 400 }; const post = await store.getPost(input.surface); if (!post) return { error: "post not found", status: 404 }; + const session = await store.getSession(post.sessionId); + if (!session) return { error: "session not found", status: 404 }; + const author = input.author ?? reservedAgent(session.agent); const comment = await store.createComment({ sessionId: post.sessionId, postId: post.id, - author: input.author, + author, text: input.text.trim().slice(0, MAX_COMMENT_TEXT), anchor: sanitizeCommentAnchor(input.anchor, post), }); @@ -736,8 +742,7 @@ export function createApp({ }); // agent replies are writes too — piggyback pending feedback on them, but // never on the user's own comments - const userFeedback = - input.author === "user" ? undefined : await collectFeedback(comment.sessionId); + const userFeedback = author === "user" ? undefined : await collectFeedback(comment.sessionId); return { comment, userFeedback }; } @@ -1413,10 +1418,20 @@ export function createApp({ return c.json({ error: 'body must include non-empty "text" string' }, 400); } const surface = typeof body.surface === "string" ? body.surface : body.snippet; + // The browser sets Fetch Metadata on same-origin requests. Only the trusted + // viewer may declare the two non-agent labels; CLI, MCP, and raw HTTP calls + // instead derive their author from the session and cannot mint "user". + // Sandboxed surfaces have opaque origins, so their postMessage bridge is + // stamped "surface" by the trusted viewer rather than by contained code. + const isViewerOrigin = c.req.header("sec-fetch-site") === "same-origin"; + const author = + isViewerOrigin && (body.author === "user" || body.author === "surface") + ? body.author + : undefined; const result = await createComment({ text: body.text, surface: typeof surface === "string" ? surface : undefined, - author: typeof body.author === "string" ? body.author : "user", + author, anchor: body.anchor, }); if ("error" in result) return c.json({ error: result.error }, result.status); diff --git a/server/mcpHttp.ts b/server/mcpHttp.ts index 758cb1b3..3a4e9c0e 100644 --- a/server/mcpHttp.ts +++ b/server/mcpHttp.ts @@ -54,7 +54,6 @@ export interface McpDeps { createComment(input: { text: string; surface?: string; - author: string; }): Promise<{ comment: Comment; userFeedback?: Feedback[] } | { error: string; status: number }>; waitForComments(q: CommentWait): Promise<{ comments: Comment[]; lastSeq: number }>; uploadAsset(input: { @@ -152,15 +151,11 @@ export function registerMcp(app: Hono, deps: McpDeps) { ); } case "reply_to_user": { - // "user" is the reserved trust label, minted only by the viewer's - // composer (genuine human keystrokes). The agent may name itself - // anything else, but never the user — that would forge feedback. - const named = typeof args.author === "string" ? args.author.trim() : ""; - const author = named && named !== "user" ? named : "agent"; + // createComment derives the reply author from the session; MCP cannot + // choose a label or mint the reserved human "user" identity. const result = await deps.createComment({ text: String(args.message ?? ""), surface: String(args.postId ?? args.surfaceId ?? ""), - author, }); if ("error" in result) throw new Error(result.error); return JSON.stringify( diff --git a/server/mcpSpec.ts b/server/mcpSpec.ts index 9dead092..d85664b4 100644 --- a/server/mcpSpec.ts +++ b/server/mcpSpec.ts @@ -305,7 +305,6 @@ export const HTTP_MCP_TOOLS = [ postId: { type: "string", description: field.postId }, surfaceId: { type: "string", description: "Deprecated alias of postId" }, message: { type: "string", description: "Plain-text reply" }, - author: { type: "string", description: 'Agent name; "user" is reserved' }, }, required: ["message"], }, diff --git a/server/sqlStore.ts b/server/sqlStore.ts index 2238131f..20caf6b9 100644 --- a/server/sqlStore.ts +++ b/server/sqlStore.ts @@ -14,6 +14,7 @@ import { MAX_WORKSPACE_ASSET_BYTES, newId, normalizeSurfaceIds, + reservedAgent, selectEvictions, type Session, type SqlStorage, @@ -310,7 +311,7 @@ export class SqlStore implements Store { const now = new Date().toISOString(); const session: Session = { id: newId(), - agent: stripNul(input.agent).trim() || "agent", + agent: reservedAgent(stripNul(input.agent).trim() || "agent"), title: stripNul(input.title)?.trim() || null, cwd: stripNul(input.cwd ?? null), createdAt: now, diff --git a/server/storage.ts b/server/storage.ts index 9c00ff28..a1365873 100644 --- a/server/storage.ts +++ b/server/storage.ts @@ -16,6 +16,7 @@ import { MAX_WORKSPACE_ASSET_BYTES, newId, normalizeSurfaceIds, + reservedAgent, selectEvictions, type Session, stripNul, @@ -273,7 +274,7 @@ export class JsonFileStore implements Store { const now = new Date().toISOString(); const session: Session = { id: newId(), - agent: stripNul(input.agent).trim() || "agent", + agent: reservedAgent(stripNul(input.agent).trim() || "agent"), title: stripNul(input.title)?.trim() || null, cwd: stripNul(input.cwd ?? null), createdAt: now, diff --git a/server/types.ts b/server/types.ts index 1049a068..e87ba147 100644 --- a/server/types.ts +++ b/server/types.ts @@ -439,6 +439,13 @@ export interface WorkspaceSnapshot { export const HISTORY_LIMIT = 20; +// "user" is the reserved trust label for genuine human comments. A session +// agent with that name could otherwise have its programmatic comments delivered +// as user feedback, so both stores normalize it when creating sessions. +export function reservedAgent(name: string): string { + return name === "user" ? "agent" : name; +} + // SQLite terminates a TEXT value at the first embedded NUL byte, while the JSON // store preserves it — so the two stores would diverge on a NUL. A NUL has no // place in a title/comment/label anyway, so both stores strip it from stored diff --git a/test/api.test.ts b/test/api.test.ts index db1ac206..7df49d92 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -36,7 +36,15 @@ function makeApp( }); } +// API tests that write user comments model the trusted viewer. Keep a separate +// helper for the regression proving programmatic callers cannot mint that label. const json = (body: unknown) => ({ + method: "POST", + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, + body: JSON.stringify(body), +}); + +const rawJson = (body: unknown) => ({ method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify(body), @@ -817,6 +825,29 @@ test("snippet page is wrapped with CSP, bridge, and kit", async () => { assert.ok(page.includes(' { + const app = makeApp(); + const post = (await ( + await app.request("/api/snippets", rawJson({ html: "

x

", agent: "my-agent" })) + ).json()) as any; + + const forged = (await ( + await app.request( + "/api/comments", + rawJson({ snippet: post.id, text: "fake user", author: "user" }), + ) + ).json()) as any; + assert.equal(forged.author, "my-agent"); + + const viewer = (await ( + await app.request( + "/api/comments", + json({ snippet: post.id, text: "real user", author: "user" }), + ) + ).json()) as any; + assert.equal(viewer.author, "user"); +}); + test("comments attach to snippets and filter by author/after", async () => { const app = makeApp(); const s = (await ( @@ -1656,7 +1687,10 @@ test("agent writes piggyback unseen user comments, delivered once", async () => // the user comments while the agent works on something else await app.request("/api/comments", json({ snippet: s.id, text: "wrong color", author: "user" })); - await app.request("/api/comments", json({ snippet: s.id, text: "also add a key" })); + await app.request( + "/api/comments", + json({ snippet: s.id, text: "also add a key", author: "user" }), + ); // the agent's next write carries the feedback const updated = (await ( diff --git a/test/cli.test.ts b/test/cli.test.ts index e92c862c..0abb688d 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -76,7 +76,7 @@ function serveApp() { const post = (url: string, body: unknown) => fetch(url, { method: "POST", - headers: { "content-type": "application/json" }, + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, body: JSON.stringify(body), }).then((r) => r.json() as Promise); @@ -1110,24 +1110,22 @@ test("wait --after with a non-number fails fast", async () => { // --- comment (agent replies to the user) ---------------------------------- -test("comment replies on a post; --author overrides the default agent name", async () => { +test("comment replies use the session agent; --author is rejected", async () => { const server = await serveSession(); try { const file = tmpFile("c.html", "

x

"); const id = JSON.parse((await cli(server, "publish", file)).stdout).id; - // default author falls back to "agent" when no --author/--agent/env is set - const def = await cli(server, "comment", "on it", "--post", id); - assert.equal(def.code, 0); - assert.equal(JSON.parse(def.stdout).author, "agent"); - - // --author sets the reply's author explicitly - const named = await cli(server, "comment", "on it", "--post", id, "--author", "bot7"); - assert.equal(named.code, 0); - const out = JSON.parse(named.stdout); + const reply = await cli(server, "comment", "on it", "--post", id); + assert.equal(reply.code, 0); + const out = JSON.parse(reply.stdout); assert.equal(out.text, "on it"); assert.equal(out.postId, id); - assert.equal(out.author, "bot7"); + assert.equal(out.author, "cli-test"); + + const forged = await cli(server, "comment", "on it", "--post", id, "--author", "user"); + assert.notEqual(forged.code, 0); + assert.match(forged.stderr, /Unknown option '--author'/); } finally { await server.close(); } diff --git a/test/feedbackSqlStore.test.ts b/test/feedbackSqlStore.test.ts index e08a14f4..71d08cbf 100644 --- a/test/feedbackSqlStore.test.ts +++ b/test/feedbackSqlStore.test.ts @@ -23,7 +23,7 @@ function makeSqlApp() { const json = (body: unknown) => ({ method: "POST", - headers: { "content-type": "application/json" }, + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, body: JSON.stringify(body), }); diff --git a/test/mcpStdio.test.ts b/test/mcpStdio.test.ts index 744fcfc1..3a35f0e1 100644 --- a/test/mcpStdio.test.ts +++ b/test/mcpStdio.test.ts @@ -146,7 +146,7 @@ async function fetchJson(url: string, path: string, init?: RequestInit) { const json = (body: unknown, method = "POST"): RequestInit => ({ method, - headers: { "content-type": "application/json" }, + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, body: JSON.stringify(body), }); diff --git a/test/piExtension.test.ts b/test/piExtension.test.ts index 3365cf7b..1815e252 100644 --- a/test/piExtension.test.ts +++ b/test/piExtension.test.ts @@ -143,7 +143,7 @@ function authInit(init: RequestInit = {}): RequestInit { ...init, headers: { authorization: "Bearer test-token", - ...(init.body ? { "content-type": "application/json" } : {}), + ...(init.body ? { "content-type": "application/json", "sec-fetch-site": "same-origin" } : {}), ...init.headers, }, }; @@ -379,13 +379,13 @@ test( const surfaceReply = await invoke( harness, "sideshow_reply_to_user", - { surfaceId: surface.id, message: "Acknowledged", author: "review-pi" }, + { surfaceId: surface.id, message: "Acknowledged" }, ctx, ); assert.match(text(surfaceReply), new RegExp(`on surface ${surface.id}`)); assert.match(text(surfaceReply), /One more thought/); assert.equal(surfaceReply.details?.postId, surface.id); - assert.equal(surfaceReply.details?.author, "review-pi"); + assert.equal(surfaceReply.details?.author, "contract-pi"); const second = await invoke( harness, diff --git a/test/storeContract.ts b/test/storeContract.ts index 235abaaa..129b00f9 100644 --- a/test/storeContract.ts +++ b/test/storeContract.ts @@ -84,6 +84,12 @@ export function runStoreContract(name: string, makeStore: () => Store | Promise< assert.equal(await store.getSetting("other"), null); }); + contract('reserves "user" as an agent name', async (store) => { + const session = await store.createSession({ agent: "user" }); + assert.equal(session.agent, "agent"); + assert.equal((await store.getSession(session.id))?.agent, "agent"); + }); + contract("renames sessions; blank title clears it; unknown id is null", async (store) => { const session = await store.createSession({ agent: "pi", title: "Old" }); const renamed = await store.renameSession(session.id, " New "); diff --git a/test/workerIntegration.integration.ts b/test/workerIntegration.integration.ts index 43d7c89d..2766f239 100644 --- a/test/workerIntegration.integration.ts +++ b/test/workerIntegration.integration.ts @@ -27,7 +27,7 @@ type AssetResult = { function json(body: unknown, method = "POST") { return { method, - headers: { ...AUTH, "content-type": "application/json" }, + headers: { ...AUTH, "content-type": "application/json", "sec-fetch-site": "same-origin" }, body: JSON.stringify(body), }; }