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: