From 633e102971d7fc59d681fe844ad4eb6ddebb8842 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 23 Aug 2026 16:56:51 -0700 Subject: [PATCH] fix: verify issue-update writes instead of inferring success (#12051) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents report task state to the control plane with `PATCH /api/issues/{id}` at the end of each heartbeat > - On remote sandbox targets those writes cross a relay that can fail at the connection level > - An agent that pipes its status curl through `head` cannot see that failure; the write is lost but the run reports success > - The issue then stays `in_progress` with no disposition, and the missing-disposition recovery must repair it > - This pull request makes the issue-update helper verify every write, and it teaches the shared skill to require verified writes > - The benefit is that a lost status write becomes a visible, retried failure instead of a silent success ## Linked Issues or Issue Description No public issue exists for this defect. The description below follows the bug report template. **What happened?** A sandboxed heartbeat run answered its issue in a comment. It then sent `PATCH /api/issues/{id}` with `status: done` through `curl -sf ... | head -c 400`. The relay dropped the connection. The `-f` flag suppressed the error output, and the pipe replaced curl's exit code with the exit code of `head`. The agent saw empty output and exit 0. It reported the write as an "empty 2xx" success and exited. The issue stayed `in_progress`, and the successful-run recovery had to close it in a corrective run. **Expected behavior** A status write that does not reach the server must surface as a failure. The helper script must retry transient failures. It must exit non-zero when the write is unconfirmed. Skill guidance must forbid write patterns that hide failures. **Steps to reproduce** 1. Point `PAPERCLIP_API_URL` at an endpoint that drops connections intermittently. 2. Finalize an issue with `curl -sf -X PATCH "$PAPERCLIP_API_URL/api/issues/$ID" -d '{"status":"done"}' | head -c 400`. 3. Observe exit code 0 with empty output while the server never received the PATCH. ## What Changed - `scripts/paperclip-issue-update.sh` now captures `%{http_code}`, retries a retryable failure (connection-level, 429, 5xx) once — two attempts total, which matches the shared bounded-write-retry rule — rejects an empty 2xx body, and confirms the response echoes the requested status before it exits 0. - Failure output states plainly that the write was NOT saved, so the calling agent reports it accurately. - `skills/paperclip/SKILL.md` Step 8 adds a required "Verify writes — never infer them" rule: a successful PATCH always returns the updated issue JSON, disposition writes must never run through `head`/`tail` pipelines, and an unconfirmed write must be reported as FAILED. - `server/src/__tests__/paperclip-skill-utils.test.ts` pins the new skill rule; a new `paperclip-issue-update-helper.test.ts` exercises the helper's behavior end-to-end. ## Verification - `bash -n scripts/paperclip-issue-update.sh` - `server/src/__tests__/paperclip-issue-update-helper.test.ts` runs the helper end-to-end against a local HTTP server: confirmed-echo success (exit 0), empty 2xx (exit 1), wrong echoed status (exit 1), 422 reject (exit 1, exactly one request), 503 then success (two requests), connection refused (two attempts, then exit 1 with a "NOT saved" report). - `npx vitest run server/src/__tests__/paperclip-issue-update-helper.test.ts server/src/__tests__/paperclip-skill-utils.test.ts server/src/__tests__/cli-invocation-safety.test.ts` — 50 passed. ## Risks - Low risk. The success-path output is unchanged (the updated issue JSON). - The helper now exits non-zero on unconfirmed writes. Callers that previously missed silent failures now see explicit errors. That is the intended behavior change. - The single retry re-sends the PATCH after a retryable failure. If the first request committed and only its response was lost, an attached comment can post twice. The duplicate is visible and benign; the prior behavior lost the write silently. ## Model Used - Claude Fable 5 (Anthropic), model id `claude-fable-5`, extended thinking enabled, agentic tool use via Claude Code (CLI harness), 200k context window. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- scripts/paperclip-issue-update.sh | 68 ++++++- .../paperclip-issue-update-helper.test.ts | 186 ++++++++++++++++++ .../__tests__/paperclip-skill-utils.test.ts | 11 ++ skills/paperclip/SKILL.md | 2 + 4 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 server/src/__tests__/paperclip-issue-update-helper.test.ts diff --git a/scripts/paperclip-issue-update.sh b/scripts/paperclip-issue-update.sh index 2645d77ff8..f8717e31c0 100755 --- a/scripts/paperclip-issue-update.sh +++ b/scripts/paperclip-issue-update.sh @@ -102,9 +102,65 @@ if [[ -z "${PAPERCLIP_API_URL:-}" || -z "${PAPERCLIP_API_KEY:-}" || -z "${PAPERC exit 1 fi -curl -sS -X PATCH \ - "$PAPERCLIP_API_URL/api/issues/$issue_id" \ - -H "Authorization: Bearer $PAPERCLIP_API_KEY" \ - -H "X-Paperclip-Run-Id: $PAPERCLIP_RUN_ID" \ - -H 'Content-Type: application/json' \ - --data-binary "$payload" +# A successful PATCH always returns the updated issue JSON. An empty body or a +# connection-level failure means the write did NOT land, even when a pipeline +# exit code says otherwise, so verify the response instead of inferring success. +# Two attempts total: the shared heartbeat policy stops a control-plane write +# after two consecutive failures, so the helper must not send a third. +max_attempts=2 +attempt=1 +while :; do + http_code="" + body="" + set +e + response="$( + curl -sS -m 30 -X PATCH \ + "$PAPERCLIP_API_URL/api/issues/$issue_id" \ + -H "Authorization: Bearer $PAPERCLIP_API_KEY" \ + -H "X-Paperclip-Run-Id: $PAPERCLIP_RUN_ID" \ + -H 'Content-Type: application/json' \ + --data-binary "$payload" \ + -w '\n%{http_code}' + )" + curl_exit=$? + set -e + + if [[ "$curl_exit" -eq 0 ]]; then + http_code="${response##*$'\n'}" + body="${response%$'\n'*}" + fi + + if [[ "$curl_exit" -eq 0 && "$http_code" == 2* ]]; then + if [[ -z "$body" ]]; then + printf 'Issue update FAILED: HTTP %s with an empty response body. A real update echoes the issue JSON; treat this write as not saved.\n' "$http_code" >&2 + exit 1 + fi + if [[ -n "$status" ]]; then + returned_status="$(jq -r '.status // empty' <<<"$body" 2>/dev/null || true)" + if [[ "$returned_status" != "$status" ]]; then + printf 'Issue update FAILED: server echoed status %s instead of requested %s.\n' "${returned_status:-}" "$status" >&2 + printf '%s\n' "$body" >&2 + exit 1 + fi + fi + printf '%s\n' "$body" + exit 0 + fi + + # 4xx (other than 429) is a definitive rejection; retrying cannot change it. + if [[ "$curl_exit" -eq 0 && "$http_code" == 4* && "$http_code" != "429" ]]; then + printf 'Issue update rejected (HTTP %s).\n' "$http_code" >&2 + [[ -n "$body" ]] && printf '%s\n' "$body" >&2 + exit 1 + fi + + if (( attempt >= max_attempts )); then + printf 'Issue update FAILED after %d attempts (curl exit %s, HTTP %s). The status/comment was NOT saved — report this write as failed, do not assume it landed.\n' "$max_attempts" "$curl_exit" "${http_code:-000}" >&2 + [[ -n "$body" ]] && printf '%s\n' "$body" >&2 + exit 1 + fi + + printf 'Issue update attempt %d/%d failed (curl exit %s, HTTP %s); retrying...\n' "$attempt" "$max_attempts" "$curl_exit" "${http_code:-000}" >&2 + sleep $((attempt * 2)) + attempt=$((attempt + 1)) +done diff --git a/server/src/__tests__/paperclip-issue-update-helper.test.ts b/server/src/__tests__/paperclip-issue-update-helper.test.ts new file mode 100644 index 0000000000..59a81de3f6 --- /dev/null +++ b/server/src/__tests__/paperclip-issue-update-helper.test.ts @@ -0,0 +1,186 @@ +import { spawn } from "node:child_process"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +// End-to-end coverage for scripts/paperclip-issue-update.sh: the helper must +// only exit 0 when the server confirms the write by echoing the update, must +// classify failures (retry connection-level faults and 5xx, never retry a +// definitive 4xx), and must stop at two attempts total to honor the shared +// bounded-write-retry rule. +const HELPER_PATH = path.resolve("scripts/paperclip-issue-update.sh"); + +interface HelperResult { + code: number | null; + stdout: string; + stderr: string; +} + +interface RecordedRequest { + method: string; + url: string; + body: string; +} + +describe("paperclip issue update helper", () => { + const cleanupFns: Array<() => Promise> = []; + + afterEach(async () => { + while (cleanupFns.length > 0) { + const cleanup = cleanupFns.pop(); + if (!cleanup) continue; + await cleanup().catch(() => undefined); + } + }); + + async function startServer( + respond: (request: RecordedRequest, attempt: number, res: http.ServerResponse) => void, + ): Promise<{ baseUrl: string; requests: RecordedRequest[] }> { + const requests: RecordedRequest[] = []; + const server = http.createServer((req, res) => { + let body = ""; + req.on("data", (chunk) => { + body += chunk; + }); + req.on("end", () => { + const recorded: RecordedRequest = { + method: req.method ?? "", + url: req.url ?? "", + body, + }; + requests.push(recorded); + respond(recorded, requests.length, res); + }); + }); + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + cleanupFns.push( + () => + new Promise((resolve) => { + server.close(() => resolve()); + }), + ); + const { port } = server.address() as AddressInfo; + return { baseUrl: `http://127.0.0.1:${port}`, requests }; + } + + function runHelper(apiUrl: string, args: string[]): Promise { + return new Promise((resolve, reject) => { + const child = spawn("bash", [HELPER_PATH, ...args], { + env: { + ...process.env, + PAPERCLIP_API_URL: apiUrl, + PAPERCLIP_API_KEY: "test-key", + PAPERCLIP_RUN_ID: "test-run", + }, + stdio: ["ignore", "pipe", "pipe"], + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { + stdout += chunk; + }); + child.stderr.on("data", (chunk) => { + stderr += chunk; + }); + child.on("error", reject); + child.on("close", (code) => resolve({ code, stdout, stderr })); + }); + } + + const doneArgs = ["--issue-id", "issue-1", "--status", "done", "--comment", "closing note"]; + + it("exits 0 and prints the issue JSON when the server echoes the requested status", async () => { + const { baseUrl, requests } = await startServer((request, _attempt, res) => { + const payload = JSON.parse(request.body) as { status?: string; comment?: string }; + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ id: "issue-1", status: payload.status })); + }); + + const result = await runHelper(baseUrl, doneArgs); + + expect(result.code).toBe(0); + expect(JSON.parse(result.stdout)).toMatchObject({ id: "issue-1", status: "done" }); + expect(requests).toHaveLength(1); + expect(requests[0]?.method).toBe("PATCH"); + expect(requests[0]?.url).toBe("/api/issues/issue-1"); + expect(JSON.parse(requests[0]?.body ?? "{}")).toEqual({ status: "done", comment: "closing note" }); + }); + + it("fails an empty 2xx body instead of treating it as success", async () => { + const { baseUrl } = await startServer((_request, _attempt, res) => { + res.writeHead(200, { "content-length": "0" }); + res.end(); + }); + + const result = await runHelper(baseUrl, doneArgs); + + expect(result.code).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("empty response body"); + }); + + it("fails when the server echoes a different status than requested", async () => { + const { baseUrl } = await startServer((_request, _attempt, res) => { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ id: "issue-1", status: "in_progress" })); + }); + + const result = await runHelper(baseUrl, doneArgs); + + expect(result.code).toBe(1); + expect(result.stderr).toContain("echoed status in_progress"); + }); + + it("does not retry a definitive 4xx rejection", async () => { + const { baseUrl, requests } = await startServer((_request, _attempt, res) => { + res.writeHead(422, { "content-type": "application/json" }); + res.end(JSON.stringify({ error: "validation" })); + }); + + const result = await runHelper(baseUrl, doneArgs); + + expect(result.code).toBe(1); + expect(result.stderr).toContain("rejected (HTTP 422)"); + expect(requests).toHaveLength(1); + }); + + it("retries a 5xx once and succeeds when the retry lands", async () => { + const { baseUrl, requests } = await startServer((request, attempt, res) => { + if (attempt === 1) { + res.writeHead(503, { "content-type": "application/json" }); + res.end(JSON.stringify({ error: "unavailable" })); + return; + } + const payload = JSON.parse(request.body) as { status?: string }; + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ id: "issue-1", status: payload.status })); + }); + + const result = await runHelper(baseUrl, doneArgs); + + expect(result.code).toBe(0); + expect(requests).toHaveLength(2); + expect(result.stderr).toContain("retrying"); + }); + + it("stops after two attempts on connection-level failure and reports the write as not saved", async () => { + // Bind and close a listener so the port is real but refuses connections. + const probe = http.createServer(); + await new Promise((resolve) => { + probe.listen(0, "127.0.0.1", resolve); + }); + const { port } = probe.address() as AddressInfo; + await new Promise((resolve) => { + probe.close(() => resolve()); + }); + + const result = await runHelper(`http://127.0.0.1:${port}`, doneArgs); + + expect(result.code).toBe(1); + expect(result.stderr).toContain("FAILED after 2 attempts"); + expect(result.stderr).toContain("NOT saved"); + }, 15_000); +}); diff --git a/server/src/__tests__/paperclip-skill-utils.test.ts b/server/src/__tests__/paperclip-skill-utils.test.ts index 69534941b6..fff38667d8 100644 --- a/server/src/__tests__/paperclip-skill-utils.test.ts +++ b/server/src/__tests__/paperclip-skill-utils.test.ts @@ -90,6 +90,17 @@ describe("paperclip skill utils", () => { expect(skillBody).toContain("`assigneeUserId` is null"); }); + it("requires issue-update writes to be verified, not inferred", async () => { + const skillBody = await fs.readFile(path.resolve("skills/paperclip/SKILL.md"), "utf8"); + + expect(skillBody).toContain("Verify writes — never infer them"); + expect(skillBody).toContain("An empty response body means the write FAILED"); + expect(skillBody).toContain("Never pipe a disposition write through `head`/`tail`"); + // The helper's verification behavior (HTTP status parsing, retry + // classification, attempt bound, exit codes) is exercised end-to-end in + // paperclip-issue-update-helper.test.ts against a live local server. + }); + it("keeps the create-issue-interaction-ui guide as a maintainer-only skill", async () => { const skillPath = path.resolve(".agents/skills/create-issue-interaction-ui/SKILL.md"); const skillBody = await fs.readFile(skillPath, "utf8"); diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 33aeceda40..18f3457f54 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -112,6 +112,8 @@ For technical upload instructions, read `references/artifacts.md`. **Bounded write retry.** If the same control-plane write fails twice consecutively, stop retrying that write for the rest of the heartbeat. Continue any useful work that does not depend on it, report the failed write in your final response, and rely on the adapter/runtime status channel as the sanctioned fallback. Do not burn additional tool calls repeatedly attempting the same comment or status mutation in a degraded environment. +**Verify writes — never infer them.** A successful `PATCH /api/issues/{id}` always returns the updated issue JSON. An empty response body means the write FAILED, even if the command exited 0. Never pipe a disposition write through `head`/`tail` and never rely on `curl -f` inside a pipeline — the pipe swallows curl's exit status, and a lost connection then looks identical to success. Use `scripts/paperclip-issue-update.sh` (it checks the HTTP status, retries connection-level failures, and confirms the echoed `status`); if you must hand-roll curl, capture `-w '%{http_code}'` and check the response echoes your update. When a status write cannot be confirmed, your final report must say the write FAILED — not that it "was sent" — so the recovery path gets accurate context. + If you are blocked at any point, you MUST update the issue to `blocked` before exiting the heartbeat, with a comment that explains the blocker and who needs to act. Before ending any heartbeat, apply this final-disposition checklist: