From 15766f68ef6e028723b4f2656501ff20a196ec04 Mon Sep 17 00:00:00 2001 From: Ben Vinegar <2153+benvinegar@users.noreply.github.com> Date: Fri, 12 Jun 2026 13:39:50 -0400 Subject: [PATCH] fix(feedback): make comment delivery exactly-once across channels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server's agentSeq cursor was advanced by every delivery channel but read by none: the CLI kept its own cursor in a state file, the stdio MCP in a process-local variable, and the /mcp transport defaulted to 0. A fresh `sideshow wait` process (or restarted stdio MCP server) replayed comments the agent had already received via piggyback or another channel. An author=user session read with no explicit `after` now resumes from the session's agentSeq, and all three clients drop their private cursors — CLI, both MCP transports, and piggyback share one exactly-once stream. Explicit `--after ` / `afterSeq` remains as a deliberate replay. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 6 ++++-- CHANGELOG.md | 7 +++++++ bin/sideshow.js | 19 +++++++++-------- guide/AGENT_SETUP.md | 7 ++++--- guide/DESIGN_GUIDE.md | 2 +- mcp/server.ts | 6 +++--- server/app.ts | 11 ++++++++-- server/mcpHttp.ts | 12 ++++++----- test/api.test.ts | 48 ++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 92 insertions(+), 26 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 03a9f48..7a5f63e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,8 +64,10 @@ consciously, not as a side effect): Don't "simplify" it back; e2e covers it on real WebKit. - Feedback cursor: each session carries `agentSeq`, the highest comment seq already delivered to the agent. Piggyback collection and `author=user` - waits advance it; the viewer's unfiltered reads never touch it. Delivery is - exactly-once by design. + waits advance it, and `author=user` session waits with no explicit `after` + resume from it — clients keep no cursor of their own, so CLI, MCP, and + piggyback share one stream. The viewer's unfiltered reads never touch it. + Delivery is exactly-once by design, across channels. - `SqlStore` schema changes need in-place migration — deployed Durable Objects can't be reset. Follow the `pragma_table_info` probe pattern in its constructor. diff --git a/CHANGELOG.md b/CHANGELOG.md index 32daadb..cb968fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,13 @@ All notable user-visible changes to this project are documented in this file. ### Fixed +- Feedback was re-delivered when channels were mixed: a fresh `sideshow wait` + process (or restarted stdio MCP server) started from seq 0 and replayed + comments the agent had already received via piggyback or another channel. + `author=user` session reads with no explicit `after` now resume from the + server-side agent cursor, and the CLI and stdio MCP keep no cursor of their + own — delivery is exactly-once across CLI, MCP, and piggyback. Pass + `--after ` (CLI) or `afterSeq` (MCP at `/mcp`) to deliberately re-read. - A comment that failed to send was silently lost (input cleared, no error). The viewer now echoes comments immediately (pending until confirmed) and on failure restores the text to the input with an error toast. diff --git a/bin/sideshow.js b/bin/sideshow.js index 319ebc5..8651b70 100644 --- a/bin/sideshow.js +++ b/bin/sideshow.js @@ -27,7 +27,8 @@ usage: sideshow wait [options] block until the user comments (long-poll) --session session to watch (default: auto) --timeout max seconds to wait (default 120) - --after only comments after this cursor (default: where you left off) + --after re-read comments after this cursor (default: where the + agent left off, tracked server-side across CLI/MCP) sideshow comment [options] post a reply comment --snippet | --session attach point (default: auto session) --author defaults to agent name @@ -148,7 +149,7 @@ async function resolveSession(flags, { create = false } = {}) { cwd: process.cwd(), }), }); - writeState({ session: session.id, agent: agentName(flags), lastSeq: 0 }); + writeState({ session: session.id, agent: agentName(flags) }); return session.id; } @@ -260,18 +261,18 @@ const commands = { }); const session = await resolveSession(flags); if (!session) fail("no active session — publish something first, or pass --session"); - const state = readState(); - const after = flags.after ?? state.lastSeq ?? 0; const timeout = Math.max(1, Number(flags.timeout ?? 120)); const deadline = Date.now() + timeout * 1000; - let result = { comments: [], lastSeq: Number(after) }; + // No client-side cursor: without --after, the server resumes from the + // session's agent cursor, shared with piggyback and MCP delivery. + let cursor = flags.after; + let result = { comments: [] }; while (Date.now() < deadline && result.comments.length === 0) { const chunk = Math.min(60, Math.ceil((deadline - Date.now()) / 1000)); - result = await api( - `/api/comments?session=${session}&author=user&after=${result.lastSeq}&wait=${chunk}`, - ); + const afterParam = cursor === undefined ? "" : `&after=${cursor}`; + result = await api(`/api/comments?session=${session}&author=user${afterParam}&wait=${chunk}`); + cursor = result.lastSeq; } - if (!flags.after) writeState({ lastSeq: result.lastSeq }); out( result.comments.length > 0 ? { comments: result.comments } diff --git a/guide/AGENT_SETUP.md b/guide/AGENT_SETUP.md index db0372c..f73fdc9 100644 --- a/guide/AGENT_SETUP.md +++ b/guide/AGENT_SETUP.md @@ -29,10 +29,11 @@ two ways: 1. Publish/update responses may include a `userFeedback` array — comments the user left since your last call. Treat them as messages from the user; they are delivered once. -2. To explicitly wait for a reaction (blocks up to 60s, returns JSON; use - `after` from the previous response's `lastSeq` to avoid re-reading): +2. To explicitly wait for a reaction (blocks up to 60s, returns JSON; resumes + where you left off — comments already delivered, on any channel, are not + re-read): - curl -s 'http://localhost:4242/api/comments?session=&author=user&after=&wait=60' + curl -s 'http://localhost:4242/api/comments?session=&author=user&wait=60' If you can run background processes, run this in the background after your first publish and keep working — it exits the moment the user comments; diff --git a/guide/DESIGN_GUIDE.md b/guide/DESIGN_GUIDE.md index 00dda32..078ea4a 100644 --- a/guide/DESIGN_GUIDE.md +++ b/guide/DESIGN_GUIDE.md @@ -13,7 +13,7 @@ Via MCP tools (preferred): `publish_snippet`, `update_snippet`, ``` POST /api/snippets { "title": "...", "html": "...", "session": "", "agent": "your-name" } PUT /api/snippets/:id { "html": "..." } # revise — same card, new version -GET /api/comments?session=&author=user&after=&wait=60 # user feedback (long-poll) +GET /api/comments?session=&author=user&wait=60 # user feedback (long-poll, resumes where you left off) ``` Omit `session` on your first publish and the response's `sessionId` is yours — diff --git a/mcp/server.ts b/mcp/server.ts index 1b0bba3..392eb85 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -36,7 +36,6 @@ const text = (value: unknown) => ({ // One MCP server process lives as long as one agent conversation, so a // lazily-created session shared across tool calls maps cleanly onto it. let sessionId: string | null = process.env.SIDESHOW_SESSION ?? null; -let lastSeq = 0; // `title` is used only when this call creates the session — once one exists // (here or in the viewer, where the user can rename it) it is never retitled. @@ -140,10 +139,11 @@ server.registerTool( async ({ timeoutSeconds }) => { const session = await ensureSession(); const wait = timeoutSeconds ?? 120; + // No client-side cursor: the server resumes author=user reads from the + // session's agent cursor, shared with piggyback delivery. const result = JSON.parse( - await api(`/api/comments?session=${session}&author=user&after=${lastSeq}&wait=${wait}`), + await api(`/api/comments?session=${session}&author=user&wait=${wait}`), ); - lastSeq = result.lastSeq; if (result.comments.length === 0) { return text({ comments: [], note: "no user feedback yet — continue, or wait again later" }); } diff --git a/server/app.ts b/server/app.ts index 05c3bf4..ded2a5d 100644 --- a/server/app.ts +++ b/server/app.ts @@ -166,7 +166,14 @@ export function createApp({ store, viewerHtml, guideMarkdown, setupText, authTok async function waitForComments( q: CommentWait, ): Promise<{ comments: Comment[]; lastSeq: number }> { - const query = { sessionId: q.sessionId, snippetId: q.snippetId, afterSeq: q.afterSeq }; + // An author=user session wait with no explicit cursor resumes from the + // session's agentSeq — "where the agent left off" lives server-side so the + // CLI, both MCP transports, and piggyback share one exactly-once stream. + let afterSeq = q.afterSeq; + if (afterSeq === undefined && q.author === "user" && q.sessionId) { + afterSeq = (await store.getSession(q.sessionId))?.agentSeq; + } + const query = { sessionId: q.sessionId, snippetId: q.snippetId, afterSeq }; const matches = (list: Comment[]) => q.author ? list.filter((cm) => cm.author === q.author) : list; const wait = Math.min(Math.max(q.waitSeconds, 0), MAX_WAIT_SECONDS); @@ -189,7 +196,7 @@ export function createApp({ store, viewerHtml, guideMarkdown, setupText, authTok }); comments = matches(await store.listComments(query)); } - const lastSeq = comments.length > 0 ? comments[comments.length - 1].seq : (q.afterSeq ?? 0); + const lastSeq = comments.length > 0 ? comments[comments.length - 1].seq : (afterSeq ?? 0); // An author=user query is the agent listening (the viewer never filters by // author) — what it receives here should not be re-delivered as piggyback. if (q.author === "user" && q.sessionId && comments.length > 0) { diff --git a/server/mcpHttp.ts b/server/mcpHttp.ts index 1ff6a94..ee976d4 100644 --- a/server/mcpHttp.ts +++ b/server/mcpHttp.ts @@ -35,8 +35,9 @@ const INSTRUCTIONS = "sessionId: pass it as `session` on every later call so your snippets stay grouped. On that first publish, " + 'also pass sessionTitle to name the session after the task at hand (e.g. "Auth refactor") so the user can ' + "tell sessions apart in the sidebar. The user can comment on " + - "snippets in their browser; call wait_for_feedback (passing the lastSeq cursor from the previous result) " + - "after publishing something you want a reaction to. Any publish/update/reply result may also carry a " + + "snippets in their browser; call wait_for_feedback after publishing something you want a reaction to — it " + + "resumes where you left off, so comments already delivered are not repeated. Any publish/update/reply " + + "result may also carry a " + "userFeedback array — comments the user left since your last call. Treat them as messages from the user; " + "they are delivered once."; @@ -97,7 +98,7 @@ const TOOLS = [ name: "wait_for_feedback", description: "Block until the user comments on this session's snippets in their browser (or the timeout passes). " + - "Returns new comments and a lastSeq cursor — pass it back as afterSeq next time to avoid re-reading. " + + "Returns new comments since the agent last received feedback on any channel (including piggyback). " + "Use timeoutSeconds 0 for a non-blocking check.", inputSchema: { type: "object", @@ -105,7 +106,8 @@ const TOOLS = [ session: { type: "string", description: "Session id to watch" }, afterSeq: { type: "number", - description: "lastSeq cursor from the previous call (default 0)", + description: + "explicit cursor override — re-reads comments after this seq (default: where the agent left off)", }, timeoutSeconds: { type: "number", @@ -197,7 +199,7 @@ export function registerMcp(app: Hono, deps: McpDeps) { const result = await deps.waitForComments({ sessionId: String(args.session ?? ""), author: "user", - afterSeq: typeof args.afterSeq === "number" ? args.afterSeq : 0, + afterSeq: typeof args.afterSeq === "number" ? args.afterSeq : undefined, waitSeconds: typeof args.timeoutSeconds === "number" ? args.timeoutSeconds : 60, }); if (result.comments.length === 0) { diff --git a/test/api.test.ts b/test/api.test.ts index c351c7f..85c29ae 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -144,8 +144,9 @@ test("comments attach to snippets and filter by author/after", async () => { assert.equal(all.comments.length, 2); assert.equal(all.comments[0].snippetTitle, "Sketch"); + // explicit after=0: re-read from the start regardless of the agent cursor const users = (await ( - await app.request(`/api/comments?session=${s.sessionId}&author=user`) + await app.request(`/api/comments?session=${s.sessionId}&author=user&after=0`) ).json()) as any; assert.equal(users.comments.length, 1); assert.equal(users.comments[0].text, "love it"); @@ -156,6 +157,51 @@ test("comments attach to snippets and filter by author/after", async () => { assert.equal(later.comments.length, 0); }); +test("author=user reads resume from the agent's server-side cursor", async () => { + const app = makeApp(); + const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; + await app.request("/api/comments", json({ snippet: s.id, text: "first", author: "user" })); + + // no cursor given: delivered once... + const first = (await ( + await app.request(`/api/comments?session=${s.sessionId}&author=user`) + ).json()) as any; + assert.equal(first.comments.length, 1); + assert.equal(first.comments[0].text, "first"); + + // ...and not again on the next cursor-less read (e.g. a fresh CLI process) + const again = (await ( + await app.request(`/api/comments?session=${s.sessionId}&author=user`) + ).json()) as any; + assert.equal(again.comments.length, 0); + + // unfiltered reads (the viewer) never consume the cursor + const viewer = (await (await app.request(`/api/comments?session=${s.sessionId}`)).json()) as any; + assert.equal(viewer.comments.length, 1); +}); + +test("piggyback delivery advances the cursor seen by author=user waits", async () => { + const app = makeApp(); + const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; + await app.request("/api/comments", json({ snippet: s.id, text: "tweak it", author: "user" })); + + // an agent write piggybacks the pending feedback... + const updated = (await ( + await app.request(`/api/snippets/${s.id}`, { + ...json({ html: "

v2

" }), + method: "PUT", + }) + ).json()) as any; + assert.equal(updated.userFeedback.length, 1); + assert.equal(updated.userFeedback[0].text, "tweak it"); + + // ...so a cursor-less wait on another channel must not re-deliver it + const wait = (await ( + await app.request(`/api/comments?session=${s.sessionId}&author=user`) + ).json()) as any; + assert.equal(wait.comments.length, 0); +}); + test("long-poll resolves when a comment arrives", async () => { const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any;