diff --git a/.agents/pm/features/pm-csv-71zg.toon b/.agents/pm/features/pm-csv-71zg.toon new file mode 100644 index 0000000..b5b9fd9 --- /dev/null +++ b/.agents/pm/features/pm-csv-71zg.toon @@ -0,0 +1,12 @@ +id: pm-csv-71zg +title: Capture prior field values in the transaction record so a failed update can be truly compensated +description: "The atomic import compensates creates by closing them, which is a real inverse: the item did not exist before, so closing it restores the prior state. An update has no inverse without the prior field values, and this extension does not capture them, so the atomic path deliberately skips update compensation. Writing some other value over a failed update would not be a rollback; it would be a second unreviewed mutation on top of a failed one, which on a partially applied import is a worse outcome than a loud identified failure. Making update compensation sound requires capturing the prior field values into the transaction record before each update and compensating from that record. That changes the transaction record format, so it is a real capability with real cost rather than part of a buffer overrun fix. Raised by a Greptile review of PR 59, where the narrower guarantee that no mutation is left un-identified was accepted as the honest ceiling for that PR." +type: Feature +status: open +priority: 2 +tags[4]: atomic,csv,rollback,transaction +created_at: "2026-08-09T00:13:19.589Z" +updated_at: "2026-08-09T00:13:19.589Z" +author: claude +acceptance_criteria: "The transaction record stores the prior values of every field an update will modify, written before the update subprocess runs; compensation restores those values rather than writing an invented value; the record format change is versioned so an in-flight transaction written by an older version is detected and refused rather than mis-restored; a regression test drives a mid-import failure after an update and asserts the pre-import field values are restored exactly" +body: "" diff --git a/.agents/pm/history/pm-csv-02hc.jsonl b/.agents/pm/history/pm-csv-02hc.jsonl new file mode 100644 index 0000000..e32a43c --- /dev/null +++ b/.agents/pm/history/pm-csv-02hc.jsonl @@ -0,0 +1,14 @@ +{"ts":"2026-08-08T22:59:14.349Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"5c63096d8a6a82e66ca6e77e","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-csv-02hc"},{"op":"add","path":"/metadata/title","value":"pm shell-outs silently degrade to wrong answers when stdout exceeds the spawnSync buffer"},{"op":"add","path":"/metadata/description","value":"Defect class: index.ts shelled out to `pm` via spawnSync in ~11 places. Node caps\ncaptured stdout at maxBuffer; when a tracker's output exceeds it the child is\nkilled mid-capture and spawnSync returns status=null, EMPTY stderr, truncated\nstdout. Guards that branch on `status !== 0` then read `null !== 0` as truthy and\nroute the overrun onto their failure branch, turning a buffer overflow into a\nWRONG ANSWER instead of an error:\n\n- itemStatus (pm get) mapped a null status onto `undefined` (\"item does not\n exist\"), so the compensation guard skipped closes it should have run.\n- loadAppliedByTransaction (pm list-all) mapped a null/error result onto an\n EMPTY applied-rows map, so a resumed atomic import silently re-imported every\n already-applied row.\n\nTwo-tier remedy applied at every spawnSync site:\n- Tier 1 (delete the subprocess for reads, read the store in-process with NO\n stdout ceiling): NOT used. A static @unbrained/pm-cli value import regresses\n this package's documented standalone-loadability (dist/ must load with only its\n own files; confirmed ERR_MODULE_NOT_FOUND when loaded off the package\n resolution path), the task forbids dynamic import(), and the host-injected\n ctx.sdk.client.list carries its own read-output budget — so a truly\n ceiling-free in-process read is not achievable here.\n- Tier 2 (cap stdout at pmListMaxBuffer 16 MiB and route every status===null\n overrun through a shared assertPmOutputFit as a hard, named error): applied to\n ALL pm subprocess sites. itemStatus additionally gains the maxBuffer it lacked\n (it was the only read still on Node's 1 MiB default). Writes that previously\n lacked a buffer now carry one too. The two best-effort compensation sites keep\n their tolerant behavior (no assert) by design.\n\nCompanion epic: pm-cli-website-wz6y (pm-csv is the worst offender tracked there)."},{"op":"add","path":"/metadata/type","value":"Issue"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":2},{"op":"add","path":"/metadata/tags","value":[]},{"op":"add","path":"/metadata/created_at","value":"2026-08-08T22:59:14.349Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-08-08T22:59:14.349Z"},{"op":"add","path":"/metadata/author","value":"pi-agent"},{"op":"add","path":"/metadata/acceptance_criteria","value":"Every spawnSync site audited and assigned a tier with justification; silent-degradation sites (itemStatus, loadAppliedByTransaction) throw a named error on a status===null overrun instead of returning empty/absent; regression test drives itemStatus past Node's 1 MiB default and asserts the real status returns; mutation-tested (reverting each fix fails its test); full gate green (typecheck, coverage thresholds unchanged, audit, pack); standalone-loadability preserved."}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"0570fba9546a610373088b7c3b6824b68ad949e43c2af1132df2299fdf954b6b","message":""} +{"ts":"2026-08-08T22:59:19.727Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"5c63096d8a6a82e66ca6e77e","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"update","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T22:59:19.727Z"},{"op":"replace","path":"/metadata/status","value":"in_progress"}],"before_hash":"0570fba9546a610373088b7c3b6824b68ad949e43c2af1132df2299fdf954b6b","after_hash":"d43075cb0e205038a6b7888cea90f8f0a7e0f2da537e4f6734a437cb883f77eb"} +{"ts":"2026-08-08T22:59:25.872Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"5c63096d8a6a82e66ca6e77e","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"claim","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T22:59:25.872Z"},{"op":"add","path":"/metadata/assignee","value":"pi-agent"},{"op":"add","path":"/metadata/claim_principal","value":"pi-agent"}],"before_hash":"d43075cb0e205038a6b7888cea90f8f0a7e0f2da537e4f6734a437cb883f77eb","after_hash":"4713333e94a8da15c76c4b079e45cead38d0ac9f6959813d5baf8b776c014c44"} +{"ts":"2026-08-08T22:59:39.151Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"5c63096d8a6a82e66ca6e77e","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"note_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T22:59:39.151Z"},{"op":"add","path":"/metadata/notes","value":[{"created_at":"2026-08-08T22:59:39.151Z","author":"pi-agent","text":"Acceptance evidence:\n\nGates (all green):\n- npm run typecheck: clean.\n- npm run coverage: index.ts 83.01% lines / 77.51% branches / 88.54% funcs; thresholds 81/74/88 UNCHANGED, no c8 ignore (coverage rose vs baseline 82.68/76.75/88.42).\n- npm run build: clean.\n- npm run audit:prod: 0 vulnerabilities.\n- npm run pack:dry-run: 9 files, packs.\n- Full suite: 114 tests pass (109 prior + 5 new in test/buffer-regression.test.ts).\n\nPer-call-site tier assignment (see PR body for the full table):\n- itemStatus (pm get): tier-2. Was the only read on Node's 1 MiB default AND mapped null status onto undefined (silent \"item missing\"). Now maxBuffer + assertPmOutputFit.\n- loadAppliedByTransaction (pm list-all): tier-2. Had maxBuffer (#41) but swallowed null/error into an empty applied map (silent re-import). Now assertPmOutputFit.\n- loadKeyIndex, buildCsvExport (pm list-all): tier-2. Already threw on error (#41); unchanged (loud, not silent).\n- upsertCreate create/close, upsertUpdate update/close: tier-2. Added maxBuffer + assertPmOutputFit.\n- compensateCreate update/close: tier-2. Added maxBuffer only (best-effort rollback tolerates failure by design; no assert).\n- Tier-1 (in-process SDK read) NOT used: a static @unbrained/pm-cli value import breaks documented standalone-loadability (confirmed ERR_MODULE_NOT_FOUND loading dist/ off the package resolution path); task forbids dynamic import(); ctx.sdk.client.list carries a read-output budget. Verified standalone load is PRESERVED after the fix.\n\nMutation test results (revert fix in scratch, confirm new test FAILS, restore):\n- Revert itemStatus (drop maxBuffer+assert): \"itemStatus returns the real status ... exceeds 1 MiB\" FAILS, \"itemStatus surfaces a buffer overrun as a named error\" FAILS. Restored -> pass.\n- Revert loadAppliedByTransaction assert: \"loadAppliedByTransaction surfaces a buffer overrun as a named error\" FAILS. Restored -> pass.\nBoth new tests are load-bearing (fail without the fix).\n\nRegression coverage of the fix:\n- A read path (itemStatus) driven past Node's 1 MiB default (2 MiB body via --body-file) returns the REAL status, not undefined.\n- A tier-2 site (itemStatus AND loadAppliedByTransaction) under a capped PM_LIST_MAX_BUFFER surfaces the overrun as a named error, not an empty/absent result."}]}],"before_hash":"4713333e94a8da15c76c4b079e45cead38d0ac9f6959813d5baf8b776c014c44","after_hash":"061c2b743888061e99833c237bc6d0317ad8f2553edf6e0e4357b6d4f3d42164"} +{"ts":"2026-08-08T22:59:48.227Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"5c63096d8a6a82e66ca6e77e","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"close","patch":[{"op":"remove","path":"/metadata/assignee"},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T22:59:48.227Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/closed_at","value":"2026-08-08T22:59:48.214Z"},{"op":"add","path":"/metadata/completed_at","value":"2026-08-08T22:59:48.214Z"},{"op":"add","path":"/metadata/close_reason","value":"Fixed: every pm spawnSync site audited and tier-assigned. The two silent-degradation sites (itemStatus, loadAppliedByTransaction) now route a status===null stdout overrun through assertPmOutputFit as a hard, named error instead of returning empty/absent; itemStatus also gained the maxBuffer it lacked. Writes carry maxBuffer too. Tier-1 in-process SDK read not used (static import breaks documented standalone-loadability; dynamic import forbidden; ctx.sdk.client.list is budgeted) — verified standalone load preserved. Full gate green, thresholds unchanged, mutation-tested, 5 regression tests added."}],"before_hash":"061c2b743888061e99833c237bc6d0317ad8f2553edf6e0e4357b6d4f3d42164","after_hash":"6a96a4bd912f69150ca53f490b20834ab586c91ab063bb27b69a4f4a64733afd"} +{"ts":"2026-08-08T23:18:50.367Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"update","patch":[{"op":"remove","path":"/metadata/close_reason"},{"op":"remove","path":"/metadata/completed_at"},{"op":"remove","path":"/metadata/closed_at"},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:18:50.367Z"},{"op":"replace","path":"/metadata/status","value":"in_progress"}],"before_hash":"6a96a4bd912f69150ca53f490b20834ab586c91ab063bb27b69a4f4a64733afd","after_hash":"d2c8b32408cad2dd03f40c0d87166347159a788228d21afca3bb7c8bedfc7fc7"} +{"ts":"2026-08-08T23:18:54.377Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"claim","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:18:54.377Z"},{"op":"add","path":"/metadata/assignee","value":"pi-agent"}],"before_hash":"d2c8b32408cad2dd03f40c0d87166347159a788228d21afca3bb7c8bedfc7fc7","after_hash":"cebfb32564422c549c8a5284eb1f7eb32f4e3888408bd5f86fd63e9dd77c1132"} +{"ts":"2026-08-08T23:30:12.425Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"files_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:12.425Z"},{"op":"add","path":"/metadata/files","value":[{"path":"index.ts","scope":"project"}]}],"before_hash":"cebfb32564422c549c8a5284eb1f7eb32f4e3888408bd5f86fd63e9dd77c1132","after_hash":"9bae7707003c2d16d17b6f0d44b10c5702cdffabf98318d18fe5faa07807eb80"} +{"ts":"2026-08-08T23:30:12.833Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"files_add","patch":[{"op":"add","path":"/metadata/files/1","value":{"path":"test/buffer-regression.test.ts","scope":"project"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:12.833Z"}],"before_hash":"9bae7707003c2d16d17b6f0d44b10c5702cdffabf98318d18fe5faa07807eb80","after_hash":"607b7a2ccc6df815e3c339e7ea2ec957c950ee990f18b91a6f96eab3e8ff1413"} +{"ts":"2026-08-08T23:30:13.287Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"files_add","patch":[{"op":"replace","path":"/metadata/files/1/path","value":"test/atomic.test.ts"},{"op":"add","path":"/metadata/files/2","value":{"path":"test/buffer-regression.test.ts","scope":"project"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:13.287Z"}],"before_hash":"607b7a2ccc6df815e3c339e7ea2ec957c950ee990f18b91a6f96eab3e8ff1413","after_hash":"a944757bf13c6398b27420a64142ca06bd37ee226f3784ff6ba3faccb09cba98"} +{"ts":"2026-08-08T23:30:30.838Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"tests_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:30.838Z"},{"op":"add","path":"/metadata/tests","value":[{"command":"npm run typecheck","scope":"project"}]}],"before_hash":"a944757bf13c6398b27420a64142ca06bd37ee226f3784ff6ba3faccb09cba98","after_hash":"eb89783ea260ff80b825b44f1928fa179b429fd9dd54abcc86a2576135b18019"} +{"ts":"2026-08-08T23:30:31.264Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"tests_add","patch":[{"op":"replace","path":"/metadata/tests/0/command","value":"npm run coverage"},{"op":"add","path":"/metadata/tests/1","value":{"command":"npm run typecheck","scope":"project"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:31.264Z"}],"before_hash":"eb89783ea260ff80b825b44f1928fa179b429fd9dd54abcc86a2576135b18019","after_hash":"311ed80d007490dcd50f3ce7afc499972709fbf036cc3cf5e2d47cdcbf481091"} +{"ts":"2026-08-08T23:30:49.907Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"note_add","patch":[{"op":"add","path":"/metadata/notes/1","value":{"created_at":"2026-08-08T23:30:49.907Z","author":"pi-agent","text":"PR #59 review follow-up — three findings (CodeRabbit Major, Greptile P1/P2). All resolved on this branch; no new PR, no merge, no bot mention.\n\nF1 (CodeRabbit, index.ts itemStatus→compensateCreate, Major): the guard now throws inside itemStatus, which compensateCreate calls, so an overrun in the compensation status lookup aborted the sweep before the best-effort strip/close subprocesses. Fix: compensateCreate catches the overrun (itemStatus's only throw is the CommandError from assertPmOutputFit; a bare catch is equivalent to catching CommandError and avoids an unreachable rethrow branch) and treats THAT row's compensation as a no-op; the sweep over the remaining applied creates continues. The hard error is kept on the import read path (loadAppliedByTransaction, the verification itemStatus in upsertCreate).\n\nF2 (Greptile P1, index.ts upsertCreate): a write overrun stranded an un-identifiable orphan. (2a) `pm create` persists the item BEFORE its receipt overruns the buffer; the old assert threw before the id was parsed, so compensation could not identify the orphan. Fix: parse the id FIRST (verified empirically that Node retains the FULL captured stdout for outputs under one read chunk, and pm create --json's flat receipt is always far under that, so the id is recoverable even on overrun); on a null status, if the id was recovered, best-effort close the orphan DIRECTLY (compensation's own status lookup would overrun under the same cap and no-op, so the close is issued here and its result tolerated) and throw a named, id-carrying error; without the id, throw naming the title for manual reconcile. (2b) the close-result assert before the close-failure branch bypassed that branch's existing compensation on a null close status; fix: drop that assert so a null close routes INTO the compensation branch (the id is already known, so nothing is stranded).\n\nF3 (Greptile P2, index.ts assertPmOutputFit): status===null is not only an overrun — a child terminated by a signal also reports it, and SIGTERM is the SAME signal a buffer-driven kill produces, so the signal alone cannot discriminate. Fix: a shared describePmNullStatus(result,label) distinguishes the three causes from the evidence Node actually populates (verified against @types/node SpawnSyncReturns AND empirically): error.code===\"ENOBUFS\" → stdout overrun (names the buffer + PM_LIST_MAX_BUFFER); error set with another code → spawn/system error (explicitly NOT a buffer problem); error unset + signal → external signal termination (explicitly NOT a buffer problem). assertPmOutputFit and the write paths both reuse it.\n\nEvidence / gates (all green):\n- npm run typecheck: clean.\n- npm run coverage: index.ts 83.62% lines / 78.42% branches / 88.66% funcs — UP vs the prior round's 83.01/77.51/88.54 and above the current floors (83.0/77.5/88.5) and thresholds (81/74/88, UNCHANGED). No c8 ignore. coverage-gate: 1 source file reported, thresholds met.\n- npm run build: clean. npm run audit:prod: 0 vulnerabilities. npm run pack:dry-run: 9 files.\n- Full suite: 121 tests pass (114 prior + 7 new: F1 + 3×F3 classifier in buffer-regression.test.ts; 3×F2 import-path in atomic.test.ts).\n\nPer-test mutation results (revert the fix in scratch, confirm the test FAILS, restore):\n- F1 (compensateCreate tolerant): revert the try/catch → F1 test FAILS (the overrun propagates out of compensateCreate). Restored → pass.\n- F3-buffer: revert describePmNullStatus to the old single message → FAILS (no ENOBUFS in the message). Restored → pass.\n- F3-signal: revert → FAILS (no \"terminated by signal\"/\"NOT a stdout buffer overrun\"). Restored → pass.\n- F3-spawn-error: revert → FAILS (no \"could not run to completion\"/ENOENT). Restored → pass.\n- F2a (overrun, id recovered, orphan closed): revert the create-site ordering (assert before parse) → FAILS (raw buffer error, no id, orphan left OPEN). Restored → pass.\n- F2a-not-recovered (create killed, title named): revert → FAILS (raw signal abort, title not named). Restored → pass.\n- F2b (null close routes into compensation): restore the close-result assert → FAILS (raw signal abort bypasses compensation, no \"left OPEN\"). Restored → pass.\nAll 7 new tests are load-bearing. The 2 updated existing tests (itemStatus / loadAppliedByTransaction overrun wording) remain load-bearing: reverting their maxBuffer+assert makes itemStatus return undefined / loadAppliedByTransaction return an empty map instead of throwing (unchanged behavior, only the asserted message wording changed).\n\nStandalone-loadability preserved: dist/index.js loads from an isolated dir with NO node_modules (only node: builtins are value-imported; @unbrained/pm-cli imports are type-only and erased). Verified the built module exports describePmNullStatus and the default extension object.\n\nEmpirical facts used by the fix (verified against node_modules/@types/node SpawnSyncReturns + a direct probe): buffer overrun → status null, signal SIGTERM, error.code===\"ENOBUFS\", stdout retained in full for outputs under one read chunk; external signal kill → status null, signal=, error undefined, empty stdout; pm close under a tiny maxBuffer still persists the close (the write precedes the receipt emission that trips the kill)."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:30:49.907Z"}],"before_hash":"311ed80d007490dcd50f3ce7afc499972709fbf036cc3cf5e2d47cdcbf481091","after_hash":"35aa1aed7ea86993d0032ac8e98386ebf871dc3609725d97a01fc4a9b345efd4"} +{"ts":"2026-08-08T23:31:06.865Z","author":"pi-agent","author_source":"asserted","agent_harness":"pi","agent_model":"glm-5.2","agent_model_source":"environment","agent_instance":"018d0fc0bb50305804e8e6ab","agent_provenance":{"model":{"value":"glm-5.2","source":"environment"},"effort":null,"role":null,"topic":null},"op":"close","patch":[{"op":"remove","path":"/metadata/assignee"},{"op":"replace","path":"/metadata/updated_at","value":"2026-08-08T23:31:06.865Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/closed_at","value":"2026-08-08T23:31:06.848Z"},{"op":"add","path":"/metadata/completed_at","value":"2026-08-08T23:31:06.848Z"},{"op":"add","path":"/metadata/close_reason","value":"PR #59 review follow-up resolved on the same branch (no new PR, no merge, no bot mention). Three findings fixed: (F1) compensateCreate now catches an overrun in its own itemStatus lookup and treats that row as a no-op so the compensation sweep is not aborted, while the import read path keeps the hard error; (F2) upsertCreate parses the create id BEFORE any throw and best-effort closes a recovered orphan directly (a write that may have taken effect is never abandoned un-identified), and a null close status now routes into the close-failure compensation branch instead of bypassing it; (F3) a shared describePmNullStatus distinguishes a stdout overrun (ENOBUFS) from a spawn/system error and from an external signal kill, wording each message for its real cause (verified against @types/node SpawnSyncReturns and empirically). Gates green: typecheck clean; coverage 83.62/78.42/88.66 (up vs prior 83.01/77.51/88.54, above floors 83.0/77.5/88.5 and thresholds 81/74/88 UNCHANGED, no c8 ignore); audit 0 vulns; pack 9 files; 121 tests pass (114 prior + 7 new). All 7 new tests mutation-tested (revert fix -> fail). Standalone-loadability preserved (dist/ loads with only node: builtins)."}],"before_hash":"35aa1aed7ea86993d0032ac8e98386ebf871dc3609725d97a01fc4a9b345efd4","after_hash":"f647ac68a6f59b79c3e04c9d6f68aad0ff4e7f79a1f2b4b3e736f06da1a47281"} diff --git a/.agents/pm/history/pm-csv-158c.jsonl b/.agents/pm/history/pm-csv-158c.jsonl new file mode 100644 index 0000000..41f47d5 --- /dev/null +++ b/.agents/pm/history/pm-csv-158c.jsonl @@ -0,0 +1,4 @@ +{"ts":"2026-08-09T00:13:19.963Z","author":"claude","author_source":"asserted","agent_harness":"claude-code","agent_instance":"fee52954969c02a106981b7d","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"1","source":"environment"},"topic":null},"op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-csv-158c"},{"op":"add","path":"/metadata/title","value":"The overrun guard asserted a recovery outcome it never checked and reported update failures with no item id"},{"op":"add","path":"/metadata/description","value":"Review round 3 on PR 59 found two remaining gaps in the buffer overrun work, both of the same family as the defect the PR exists to remove: a message that states something the code did not establish. First, when a create overran its receipt and the recovered orphan was closed as a recovery step, the close result was discarded while the thrown error claimed the orphan was closed. If that recovery close itself failed or was killed, the operator was told the orphan was handled and stopped looking for an item that is still open. Second, a pm update or the terminal pm close in the update path could persist its mutation and then return a null status or a non-zero exit, and the resulting error named neither the item nor the possibility that the write had landed, so the row was reported as a generic failure with nothing to reconcile against. The fix inspects the recovery close result and words the error for its actual outcome, and makes every update and terminal close failure carry the item id together with an explicit statement that the mutation may already have been applied. The guarantee is stated in the code as no mutation is left un-identified, which is narrower and more honest than no mutation is left behind. See the linked feature for the capability that would be needed to widen it."},{"op":"add","path":"/metadata/type","value":"Issue"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":1},{"op":"add","path":"/metadata/tags","value":["atomic","csv","diagnostics","review"]},{"op":"add","path":"/metadata/created_at","value":"2026-08-09T00:13:19.963Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-08-09T00:13:19.963Z"},{"op":"add","path":"/metadata/author","value":"claude"},{"op":"add","path":"/metadata/acceptance_criteria","value":"The recovery close result is inspected and the thrown error distinguishes an orphan that was closed from one that is still open, carrying the close's own failure cause; the recovery close is attempted exactly once rather than retried, because under a capped buffer it fails identically every time; every pm update and terminal pm close failure in the update path carries the item id and states that the mutation may already have been applied; each new error arm has a regression test and every test is mutation-tested by reverting its fix and confirming it fails; coverage thresholds are unchanged and measured coverage does not regress"}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"3b24831905124d46456092249e61b151b0ee483b54acf173b4d9fe2cb6654fcc","message":""} +{"ts":"2026-08-09T00:13:41.462Z","author":"claude","author_source":"asserted","agent_harness":"claude-code","agent_instance":"fee52954969c02a106981b7d","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"1","source":"environment"},"topic":null},"op":"update","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-09T00:13:41.462Z"},{"op":"add","path":"/metadata/dependencies","value":[{"id":"pm-csv-71zg","kind":"related","created_at":"2026-08-09T00:13:41.428Z","author":"claude","source_kind":"cli:update:dep","author_source":"detected"}]}],"before_hash":"3b24831905124d46456092249e61b151b0ee483b54acf173b4d9fe2cb6654fcc","after_hash":"7b95785d16b3392d5aeb70ac83a2e35e692a0e7406a1493d97f11218bb008715"} +{"ts":"2026-08-09T00:13:53.186Z","author":"claude","author_source":"asserted","agent_harness":"claude-code","agent_instance":"fee52954969c02a106981b7d","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"1","source":"environment"},"topic":null},"op":"note_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-09T00:13:53.186Z"},{"op":"add","path":"/metadata/notes","value":[{"created_at":"2026-08-09T00:13:53.186Z","author":"claude","text":"Acceptance evidence for review round 3 on PR 59.\n\nFinding A (Greptile P1, upsertCreate recovery close). The recovery close result is now captured\nand inspected. On status 0 the error states the orphaned item was closed; on a null status or a\nnon-zero exit it states the orphan is STILL OPEN, must be closed manually, and carries the close's\nown cause (describePmNullStatus for a null status, the close stderr otherwise). The close is\nattempted exactly once: under a capped buffer it fails identically every time, so a retry loop\nwould only turn a fast actionable error into a slow one. The docstring now states the guarantee\nand its limit: the extension cannot make pm create atomic (a single subprocess with no transaction\nthe caller can join), so a window always exists where the item is persisted and the caller does not\nyet know it; what is guaranteed is that the window is always reported WITH THE ID.\n\nFinding B (Greptile P1, upsertUpdate). Every pm update and terminal pm close failure now carries\nthe item id and states explicitly that the mutation may already have been applied. The null-status\narm and the non-zero arm are handled separately, because a non-zero exit is not proof that nothing\nwas written. When the terminal close fails after a successful update, the message reports BOTH: the\nupdate WAS applied and the close may have been too. Update compensation was deliberately NOT added\n(see pm-csv-71zg): an update has no inverse without the prior field values, so compensating it\ncould only mean writing some other value over it, which is a second unreviewed mutation rather than\na rollback. The docstring states the narrower guarantee in those words: no mutation is left\nUN-IDENTIFIED, not no mutation is left behind.\n\nGates, all green and run after the final restore:\n- typecheck: clean.\n- build and build:test: clean.\n- Full suite: 127 tests pass (123 after Finding A's tests, plus 4 new update-path tests).\n- coverage: index.ts 84.18 lines / 80.26 branches / 88.66 funcs, UP from round 2's\n 83.62 / 78.42 / 88.66. Thresholds 81 / 74 / 88 UNCHANGED. No c8 ignore added.\n- audit:prod: 0 vulnerabilities.\n- pack:dry-run: 9 files, 64.1 kB.\n- changelog:check: up to date.\n\nMutation testing of the four new update-path tests. Reverting the upsertUpdate fix to the previous\nassertPmOutputFit plus generic-message form makes exactly 4 of the 28 tests in atomic.test.ts fail,\nand restoring it returns 28 of 28 passing. The four are the only tests asserting the new messages,\nso all four are load-bearing. Finding A's three tests were mutation-tested in the same way when\nthey were written.\n\nTest infrastructure added: two tracker-scoped fake pm markers, fake-update-fail (exit 1 with a\nsimulated stderr) and fake-update-kill (SIGKILL, producing status null), alongside the existing\nfake-close-fail and fake-close-kill. Each marker only acts when present in the tracker root under\ntest, so concurrent test files are unaffected while PATH is temporarily mutated.\n\nProvenance note: the production code for both findings was written by a pi agent (glm-5.2) whose\nprovider hit a usage cap before it could run the gates, create the items, or commit. The four\nupdate-path regression tests, the mutation testing, every gate run, and this record were completed\nby the orchestrator, which also verified the agent's code rather than accepting it. One unrelated\nedit the agent left behind, a deletion of two extension merge-driver lines from .gitattributes, was\nreverted as out of scope."}]}],"before_hash":"7b95785d16b3392d5aeb70ac83a2e35e692a0e7406a1493d97f11218bb008715","after_hash":"985fce0e50000e2e28c384987dcd6eb9b5684174d02a332c9274ccfabcc6fd99"} +{"ts":"2026-08-09T00:14:03.734Z","author":"claude","author_source":"asserted","agent_harness":"claude-code","agent_instance":"fee52954969c02a106981b7d","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"1","source":"environment"},"topic":null},"op":"close","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-08-09T00:14:03.734Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/closed_at","value":"2026-08-09T00:14:03.706Z"},{"op":"add","path":"/metadata/completed_at","value":"2026-08-09T00:14:03.706Z"},{"op":"add","path":"/metadata/close_reason","value":"Both review round 3 findings fixed on PR 59. The recovery close in upsertCreate is now inspected rather than assumed, so the thrown error distinguishes an orphan that was closed from one that is still open and carries the close's own failure cause; it is attempted exactly once because under a capped buffer it fails identically every time. Every pm update and terminal pm close failure in the update path now carries the item id and states explicitly that the mutation may already have been applied, with the null-status and non-zero arms handled separately because a non-zero exit is not proof nothing was written. Update compensation was deliberately not added and is tracked as pm-csv-71zg: an update has no inverse without prior field values, so compensating it would be a second unreviewed mutation rather than a rollback. Gates green: typecheck clean, 127 tests pass, coverage 84.18 / 80.26 / 88.66 up from 83.62 / 78.42 / 88.66 with thresholds 81 / 74 / 88 unchanged, audit 0 vulnerabilities, pack 9 files, changelog up to date. The four new update-path tests are mutation-tested: reverting the fix fails exactly those four of 28 and restoring it passes all 28."}],"before_hash":"985fce0e50000e2e28c384987dcd6eb9b5684174d02a332c9274ccfabcc6fd99","after_hash":"10a709c5a3bf5e66d0ac05bffec9c4df74eaf9a1f062f15aa12e4c4b98e32c75"} diff --git a/.agents/pm/history/pm-csv-71zg.jsonl b/.agents/pm/history/pm-csv-71zg.jsonl new file mode 100644 index 0000000..4bc2055 --- /dev/null +++ b/.agents/pm/history/pm-csv-71zg.jsonl @@ -0,0 +1 @@ +{"ts":"2026-08-09T00:13:19.589Z","author":"claude","author_source":"asserted","agent_harness":"claude-code","agent_instance":"fee52954969c02a106981b7d","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"1","source":"environment"},"topic":null},"op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-csv-71zg"},{"op":"add","path":"/metadata/title","value":"Capture prior field values in the transaction record so a failed update can be truly compensated"},{"op":"add","path":"/metadata/description","value":"The atomic import compensates creates by closing them, which is a real inverse: the item did not exist before, so closing it restores the prior state. An update has no inverse without the prior field values, and this extension does not capture them, so the atomic path deliberately skips update compensation. Writing some other value over a failed update would not be a rollback; it would be a second unreviewed mutation on top of a failed one, which on a partially applied import is a worse outcome than a loud identified failure. Making update compensation sound requires capturing the prior field values into the transaction record before each update and compensating from that record. That changes the transaction record format, so it is a real capability with real cost rather than part of a buffer overrun fix. Raised by a Greptile review of PR 59, where the narrower guarantee that no mutation is left un-identified was accepted as the honest ceiling for that PR."},{"op":"add","path":"/metadata/type","value":"Feature"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":2},{"op":"add","path":"/metadata/tags","value":["atomic","csv","rollback","transaction"]},{"op":"add","path":"/metadata/created_at","value":"2026-08-09T00:13:19.589Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-08-09T00:13:19.589Z"},{"op":"add","path":"/metadata/author","value":"claude"},{"op":"add","path":"/metadata/acceptance_criteria","value":"The transaction record stores the prior values of every field an update will modify, written before the update subprocess runs; compensation restores those values rather than writing an invented value; the record format change is versioned so an in-flight transaction written by an older version is detected and refused rather than mis-restored; a regression test drives a mid-import failure after an update and asserts the pre-import field values are restored exactly"}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"d31ac3a407080cf7a9d0531a81619bd611bf2a487e35d0e74c062b5853a7711b","message":""} diff --git a/.agents/pm/issues/pm-csv-02hc.toon b/.agents/pm/issues/pm-csv-02hc.toon new file mode 100644 index 0000000..98389b7 --- /dev/null +++ b/.agents/pm/issues/pm-csv-02hc.toon @@ -0,0 +1,26 @@ +id: pm-csv-02hc +title: pm shell-outs silently degrade to wrong answers when stdout exceeds the spawnSync buffer +description: "Defect class: index.ts shelled out to `pm` via spawnSync in ~11 places. Node caps\ncaptured stdout at maxBuffer; when a tracker's output exceeds it the child is\nkilled mid-capture and spawnSync returns status=null, EMPTY stderr, truncated\nstdout. Guards that branch on `status !== 0` then read `null !== 0` as truthy and\nroute the overrun onto their failure branch, turning a buffer overflow into a\nWRONG ANSWER instead of an error:\n\n- itemStatus (pm get) mapped a null status onto `undefined` (\"item does not\n exist\"), so the compensation guard skipped closes it should have run.\n- loadAppliedByTransaction (pm list-all) mapped a null/error result onto an\n EMPTY applied-rows map, so a resumed atomic import silently re-imported every\n already-applied row.\n\nTwo-tier remedy applied at every spawnSync site:\n- Tier 1 (delete the subprocess for reads, read the store in-process with NO\n stdout ceiling): NOT used. A static @unbrained/pm-cli value import regresses\n this package's documented standalone-loadability (dist/ must load with only its\n own files; confirmed ERR_MODULE_NOT_FOUND when loaded off the package\n resolution path), the task forbids dynamic import(), and the host-injected\n ctx.sdk.client.list carries its own read-output budget — so a truly\n ceiling-free in-process read is not achievable here.\n- Tier 2 (cap stdout at pmListMaxBuffer 16 MiB and route every status===null\n overrun through a shared assertPmOutputFit as a hard, named error): applied to\n ALL pm subprocess sites. itemStatus additionally gains the maxBuffer it lacked\n (it was the only read still on Node's 1 MiB default). Writes that previously\n lacked a buffer now carry one too. The two best-effort compensation sites keep\n their tolerant behavior (no assert) by design.\n\nCompanion epic: pm-cli-website-wz6y (pm-csv is the worst offender tracked there)." +type: Issue +status: closed +priority: 2 +tags: [] +created_at: "2026-08-08T22:59:14.349Z" +updated_at: "2026-08-08T23:31:06.865Z" +closed_at: "2026-08-08T23:31:06.848Z" +completed_at: "2026-08-08T23:31:06.848Z" +claim_principal: pi-agent +author: pi-agent +acceptance_criteria: "Every spawnSync site audited and assigned a tier with justification; silent-degradation sites (itemStatus, loadAppliedByTransaction) throw a named error on a status===null overrun instead of returning empty/absent; regression test drives itemStatus past Node's 1 MiB default and asserts the real status returns; mutation-tested (reverting each fix fails its test); full gate green (typecheck, coverage thresholds unchanged, audit, pack); standalone-loadability preserved." +notes[2]{created_at,author,text}: + "2026-08-08T22:59:39.151Z",pi-agent,"Acceptance evidence:\n\nGates (all green):\n- npm run typecheck: clean.\n- npm run coverage: index.ts 83.01% lines / 77.51% branches / 88.54% funcs; thresholds 81/74/88 UNCHANGED, no c8 ignore (coverage rose vs baseline 82.68/76.75/88.42).\n- npm run build: clean.\n- npm run audit:prod: 0 vulnerabilities.\n- npm run pack:dry-run: 9 files, packs.\n- Full suite: 114 tests pass (109 prior + 5 new in test/buffer-regression.test.ts).\n\nPer-call-site tier assignment (see PR body for the full table):\n- itemStatus (pm get): tier-2. Was the only read on Node's 1 MiB default AND mapped null status onto undefined (silent \"item missing\"). Now maxBuffer + assertPmOutputFit.\n- loadAppliedByTransaction (pm list-all): tier-2. Had maxBuffer (#41) but swallowed null/error into an empty applied map (silent re-import). Now assertPmOutputFit.\n- loadKeyIndex, buildCsvExport (pm list-all): tier-2. Already threw on error (#41); unchanged (loud, not silent).\n- upsertCreate create/close, upsertUpdate update/close: tier-2. Added maxBuffer + assertPmOutputFit.\n- compensateCreate update/close: tier-2. Added maxBuffer only (best-effort rollback tolerates failure by design; no assert).\n- Tier-1 (in-process SDK read) NOT used: a static @unbrained/pm-cli value import breaks documented standalone-loadability (confirmed ERR_MODULE_NOT_FOUND loading dist/ off the package resolution path); task forbids dynamic import(); ctx.sdk.client.list carries a read-output budget. Verified standalone load is PRESERVED after the fix.\n\nMutation test results (revert fix in scratch, confirm new test FAILS, restore):\n- Revert itemStatus (drop maxBuffer+assert): \"itemStatus returns the real status ... exceeds 1 MiB\" FAILS, \"itemStatus surfaces a buffer overrun as a named error\" FAILS. Restored -> pass.\n- Revert loadAppliedByTransaction assert: \"loadAppliedByTransaction surfaces a buffer overrun as a named error\" FAILS. Restored -> pass.\nBoth new tests are load-bearing (fail without the fix).\n\nRegression coverage of the fix:\n- A read path (itemStatus) driven past Node's 1 MiB default (2 MiB body via --body-file) returns the REAL status, not undefined.\n- A tier-2 site (itemStatus AND loadAppliedByTransaction) under a capped PM_LIST_MAX_BUFFER surfaces the overrun as a named error, not an empty/absent result." + "2026-08-08T23:30:49.907Z",pi-agent,"PR #59 review follow-up — three findings (CodeRabbit Major, Greptile P1/P2). All resolved on this branch; no new PR, no merge, no bot mention.\n\nF1 (CodeRabbit, index.ts itemStatus→compensateCreate, Major): the guard now throws inside itemStatus, which compensateCreate calls, so an overrun in the compensation status lookup aborted the sweep before the best-effort strip/close subprocesses. Fix: compensateCreate catches the overrun (itemStatus's only throw is the CommandError from assertPmOutputFit; a bare catch is equivalent to catching CommandError and avoids an unreachable rethrow branch) and treats THAT row's compensation as a no-op; the sweep over the remaining applied creates continues. The hard error is kept on the import read path (loadAppliedByTransaction, the verification itemStatus in upsertCreate).\n\nF2 (Greptile P1, index.ts upsertCreate): a write overrun stranded an un-identifiable orphan. (2a) `pm create` persists the item BEFORE its receipt overruns the buffer; the old assert threw before the id was parsed, so compensation could not identify the orphan. Fix: parse the id FIRST (verified empirically that Node retains the FULL captured stdout for outputs under one read chunk, and pm create --json's flat receipt is always far under that, so the id is recoverable even on overrun); on a null status, if the id was recovered, best-effort close the orphan DIRECTLY (compensation's own status lookup would overrun under the same cap and no-op, so the close is issued here and its result tolerated) and throw a named, id-carrying error; without the id, throw naming the title for manual reconcile. (2b) the close-result assert before the close-failure branch bypassed that branch's existing compensation on a null close status; fix: drop that assert so a null close routes INTO the compensation branch (the id is already known, so nothing is stranded).\n\nF3 (Greptile P2, index.ts assertPmOutputFit): status===null is not only an overrun — a child terminated by a signal also reports it, and SIGTERM is the SAME signal a buffer-driven kill produces, so the signal alone cannot discriminate. Fix: a shared describePmNullStatus(result,label) distinguishes the three causes from the evidence Node actually populates (verified against @types/node SpawnSyncReturns AND empirically): error.code===\"ENOBUFS\" → stdout overrun (names the buffer + PM_LIST_MAX_BUFFER); error set with another code → spawn/system error (explicitly NOT a buffer problem); error unset + signal → external signal termination (explicitly NOT a buffer problem). assertPmOutputFit and the write paths both reuse it.\n\nEvidence / gates (all green):\n- npm run typecheck: clean.\n- npm run coverage: index.ts 83.62% lines / 78.42% branches / 88.66% funcs — UP vs the prior round's 83.01/77.51/88.54 and above the current floors (83.0/77.5/88.5) and thresholds (81/74/88, UNCHANGED). No c8 ignore. coverage-gate: 1 source file reported, thresholds met.\n- npm run build: clean. npm run audit:prod: 0 vulnerabilities. npm run pack:dry-run: 9 files.\n- Full suite: 121 tests pass (114 prior + 7 new: F1 + 3×F3 classifier in buffer-regression.test.ts; 3×F2 import-path in atomic.test.ts).\n\nPer-test mutation results (revert the fix in scratch, confirm the test FAILS, restore):\n- F1 (compensateCreate tolerant): revert the try/catch → F1 test FAILS (the overrun propagates out of compensateCreate). Restored → pass.\n- F3-buffer: revert describePmNullStatus to the old single message → FAILS (no ENOBUFS in the message). Restored → pass.\n- F3-signal: revert → FAILS (no \"terminated by signal\"/\"NOT a stdout buffer overrun\"). Restored → pass.\n- F3-spawn-error: revert → FAILS (no \"could not run to completion\"/ENOENT). Restored → pass.\n- F2a (overrun, id recovered, orphan closed): revert the create-site ordering (assert before parse) → FAILS (raw buffer error, no id, orphan left OPEN). Restored → pass.\n- F2a-not-recovered (create killed, title named): revert → FAILS (raw signal abort, title not named). Restored → pass.\n- F2b (null close routes into compensation): restore the close-result assert → FAILS (raw signal abort bypasses compensation, no \"left OPEN\"). Restored → pass.\nAll 7 new tests are load-bearing. The 2 updated existing tests (itemStatus / loadAppliedByTransaction overrun wording) remain load-bearing: reverting their maxBuffer+assert makes itemStatus return undefined / loadAppliedByTransaction return an empty map instead of throwing (unchanged behavior, only the asserted message wording changed).\n\nStandalone-loadability preserved: dist/index.js loads from an isolated dir with NO node_modules (only node: builtins are value-imported; @unbrained/pm-cli imports are type-only and erased). Verified the built module exports describePmNullStatus and the default extension object.\n\nEmpirical facts used by the fix (verified against node_modules/@types/node SpawnSyncReturns + a direct probe): buffer overrun → status null, signal SIGTERM, error.code===\"ENOBUFS\", stdout retained in full for outputs under one read chunk; external signal kill → status null, signal=, error undefined, empty stdout; pm close under a tiny maxBuffer still persists the close (the write precedes the receipt emission that trips the kill)." +files[3]{path,scope}: + index.ts,project + test/atomic.test.ts,project + test/buffer-regression.test.ts,project +tests[2]{command,scope}: + npm run coverage,project + npm run typecheck,project +close_reason: "PR #59 review follow-up resolved on the same branch (no new PR, no merge, no bot mention). Three findings fixed: (F1) compensateCreate now catches an overrun in its own itemStatus lookup and treats that row as a no-op so the compensation sweep is not aborted, while the import read path keeps the hard error; (F2) upsertCreate parses the create id BEFORE any throw and best-effort closes a recovered orphan directly (a write that may have taken effect is never abandoned un-identified), and a null close status now routes into the close-failure compensation branch instead of bypassing it; (F3) a shared describePmNullStatus distinguishes a stdout overrun (ENOBUFS) from a spawn/system error and from an external signal kill, wording each message for its real cause (verified against @types/node SpawnSyncReturns and empirically). Gates green: typecheck clean; coverage 83.62/78.42/88.66 (up vs prior 83.01/77.51/88.54, above floors 83.0/77.5/88.5 and thresholds 81/74/88 UNCHANGED, no c8 ignore); audit 0 vulns; pack 9 files; 121 tests pass (114 prior + 7 new). All 7 new tests mutation-tested (revert fix -> fail). Standalone-loadability preserved (dist/ loads with only node: builtins)." +body: "" diff --git a/.agents/pm/issues/pm-csv-158c.toon b/.agents/pm/issues/pm-csv-158c.toon new file mode 100644 index 0000000..1cafef0 --- /dev/null +++ b/.agents/pm/issues/pm-csv-158c.toon @@ -0,0 +1,19 @@ +id: pm-csv-158c +title: The overrun guard asserted a recovery outcome it never checked and reported update failures with no item id +description: "Review round 3 on PR 59 found two remaining gaps in the buffer overrun work, both of the same family as the defect the PR exists to remove: a message that states something the code did not establish. First, when a create overran its receipt and the recovered orphan was closed as a recovery step, the close result was discarded while the thrown error claimed the orphan was closed. If that recovery close itself failed or was killed, the operator was told the orphan was handled and stopped looking for an item that is still open. Second, a pm update or the terminal pm close in the update path could persist its mutation and then return a null status or a non-zero exit, and the resulting error named neither the item nor the possibility that the write had landed, so the row was reported as a generic failure with nothing to reconcile against. The fix inspects the recovery close result and words the error for its actual outcome, and makes every update and terminal close failure carry the item id together with an explicit statement that the mutation may already have been applied. The guarantee is stated in the code as no mutation is left un-identified, which is narrower and more honest than no mutation is left behind. See the linked feature for the capability that would be needed to widen it." +type: Issue +status: closed +priority: 1 +tags[4]: atomic,csv,diagnostics,review +created_at: "2026-08-09T00:13:19.963Z" +updated_at: "2026-08-09T00:14:03.734Z" +closed_at: "2026-08-09T00:14:03.706Z" +completed_at: "2026-08-09T00:14:03.706Z" +author: claude +acceptance_criteria: "The recovery close result is inspected and the thrown error distinguishes an orphan that was closed from one that is still open, carrying the close's own failure cause; the recovery close is attempted exactly once rather than retried, because under a capped buffer it fails identically every time; every pm update and terminal pm close failure in the update path carries the item id and states that the mutation may already have been applied; each new error arm has a regression test and every test is mutation-tested by reverting its fix and confirming it fails; coverage thresholds are unchanged and measured coverage does not regress" +dependencies[1]{id,kind,created_at,author,source_kind,author_source}: + pm-csv-71zg,related,"2026-08-09T00:13:41.428Z",claude,"cli:update:dep",detected +notes[1]{created_at,author,text}: + "2026-08-09T00:13:53.186Z",claude,"Acceptance evidence for review round 3 on PR 59.\n\nFinding A (Greptile P1, upsertCreate recovery close). The recovery close result is now captured\nand inspected. On status 0 the error states the orphaned item was closed; on a null status or a\nnon-zero exit it states the orphan is STILL OPEN, must be closed manually, and carries the close's\nown cause (describePmNullStatus for a null status, the close stderr otherwise). The close is\nattempted exactly once: under a capped buffer it fails identically every time, so a retry loop\nwould only turn a fast actionable error into a slow one. The docstring now states the guarantee\nand its limit: the extension cannot make pm create atomic (a single subprocess with no transaction\nthe caller can join), so a window always exists where the item is persisted and the caller does not\nyet know it; what is guaranteed is that the window is always reported WITH THE ID.\n\nFinding B (Greptile P1, upsertUpdate). Every pm update and terminal pm close failure now carries\nthe item id and states explicitly that the mutation may already have been applied. The null-status\narm and the non-zero arm are handled separately, because a non-zero exit is not proof that nothing\nwas written. When the terminal close fails after a successful update, the message reports BOTH: the\nupdate WAS applied and the close may have been too. Update compensation was deliberately NOT added\n(see pm-csv-71zg): an update has no inverse without the prior field values, so compensating it\ncould only mean writing some other value over it, which is a second unreviewed mutation rather than\na rollback. The docstring states the narrower guarantee in those words: no mutation is left\nUN-IDENTIFIED, not no mutation is left behind.\n\nGates, all green and run after the final restore:\n- typecheck: clean.\n- build and build:test: clean.\n- Full suite: 127 tests pass (123 after Finding A's tests, plus 4 new update-path tests).\n- coverage: index.ts 84.18 lines / 80.26 branches / 88.66 funcs, UP from round 2's\n 83.62 / 78.42 / 88.66. Thresholds 81 / 74 / 88 UNCHANGED. No c8 ignore added.\n- audit:prod: 0 vulnerabilities.\n- pack:dry-run: 9 files, 64.1 kB.\n- changelog:check: up to date.\n\nMutation testing of the four new update-path tests. Reverting the upsertUpdate fix to the previous\nassertPmOutputFit plus generic-message form makes exactly 4 of the 28 tests in atomic.test.ts fail,\nand restoring it returns 28 of 28 passing. The four are the only tests asserting the new messages,\nso all four are load-bearing. Finding A's three tests were mutation-tested in the same way when\nthey were written.\n\nTest infrastructure added: two tracker-scoped fake pm markers, fake-update-fail (exit 1 with a\nsimulated stderr) and fake-update-kill (SIGKILL, producing status null), alongside the existing\nfake-close-fail and fake-close-kill. Each marker only acts when present in the tracker root under\ntest, so concurrent test files are unaffected while PATH is temporarily mutated.\n\nProvenance note: the production code for both findings was written by a pi agent (glm-5.2) whose\nprovider hit a usage cap before it could run the gates, create the items, or commit. The four\nupdate-path regression tests, the mutation testing, every gate run, and this record were completed\nby the orchestrator, which also verified the agent's code rather than accepting it. One unrelated\nedit the agent left behind, a deletion of two extension merge-driver lines from .gitattributes, was\nreverted as out of scope." +close_reason: "Both review round 3 findings fixed on PR 59. The recovery close in upsertCreate is now inspected rather than assumed, so the thrown error distinguishes an orphan that was closed from one that is still open and carries the close's own failure cause; it is attempted exactly once because under a capped buffer it fails identically every time. Every pm update and terminal pm close failure in the update path now carries the item id and states explicitly that the mutation may already have been applied, with the null-status and non-zero arms handled separately because a non-zero exit is not proof nothing was written. Update compensation was deliberately not added and is tracked as pm-csv-71zg: an update has no inverse without prior field values, so compensating it would be a second unreviewed mutation rather than a rollback. Gates green: typecheck clean, 127 tests pass, coverage 84.18 / 80.26 / 88.66 up from 83.62 / 78.42 / 88.66 with thresholds 81 / 74 / 88 unchanged, audit 0 vulnerabilities, pack 9 files, changelog up to date. The four new update-path tests are mutation-tested: reverting the fix fails exactly those four of 28 and restoring it passes all 28." +body: "" diff --git a/index.ts b/index.ts index a7f8134..ec83be8 100644 --- a/index.ts +++ b/index.ts @@ -1,7 +1,7 @@ import type { ExtensionApi, ExtensionModule } from "@unbrained/pm-cli/sdk/authoring"; import { readFileSync, writeFileSync, createReadStream } from "node:fs"; import { resolve } from "node:path"; -import { spawnSync } from "node:child_process"; +import { spawnSync, type SpawnSyncReturns } from "node:child_process"; import { createHash } from "node:crypto"; import type { @@ -486,6 +486,69 @@ function pmListMaxBuffer(): number { return Number.isSafeInteger(raw) && raw > 0 ? raw : 16 * 1024 * 1024; } +/** + * Describe why a `spawnSync` result has `status: null`, wording the message for + * the REAL cause so the operator is sent down the right path. + * + * Node reports `status: null` whenever the child was terminated before it could + * produce a usable exit code, and three distinct causes produce it (verified + * against `SpawnSyncReturns` in `@types/node` and confirmed empirically): + * + * - A captured-output overrun. Node sets `result.error` with `code === + * "ENOBUFS"` and terminates the child with the kill signal. The captured + * output is truncated and unusable, and the fix IS to narrow the query or + * raise `PM_LIST_MAX_BUFFER`. + * - A spawn or system error (the binary could not be launched, a timeout, + * …). `result.error` is set with a code OTHER than `ENOBUFS`. This is NOT a + * buffer problem, and telling the operator to raise `PM_LIST_MAX_BUFFER` + * sends them down the wrong path. + * - An external signal termination (an OOM reaper, a manual kill, …). + * `result.error` is unset and `result.signal` carries the signal. Also NOT a + * buffer problem — note the kill signal from a buffer overrun (`SIGTERM`) is + * the SAME signal an external `SIGTERM` produces, so the signal alone cannot + * tell the two apart; only `error.code === "ENOBUFS"` can. + * + * Exposed so the write paths ({@link upsertCreate}) can reuse the same wording + * when they recover an id from an overrun receipt before throwing, and so the + * three causes can be regression-tested directly with synthetic results. + */ +export function describePmNullStatus(result: SpawnSyncReturns, label: string): string { + const err = result.error as NodeJS.ErrnoException | undefined; + const code = err?.code; + if (err !== undefined && code === "ENOBUFS") { + return `pm ${label} overran its ${pmListMaxBuffer()}-byte stdout buffer (Node reported error code ENOBUFS${result.signal ? ` and terminated the child with ${result.signal}` : ""}); the captured output is truncated and unusable — narrow the query (--status/--type) or raise the PM_LIST_MAX_BUFFER env var`; + } + if (err !== undefined) { + return `pm ${label} could not run to completion: ${err.message}${code !== undefined ? ` (code ${code})` : ""}; this is a spawn or system error, NOT a stdout buffer overrun, so raising the PM_LIST_MAX_BUFFER env var will not help`; + } + return `pm ${label} was terminated by signal ${result.signal ?? ""} before producing a usable exit code; this is NOT a stdout buffer overrun (no ENOBUFS error was reported), so raising the PM_LIST_MAX_BUFFER env var will not help — investigate what sent the signal`; +} + +/** + * Turn a `status: null` pm subprocess result into a hard, named error instead of + * the wrong answer `spawnSync` otherwise leaves behind. + * + * When a child is terminated before exiting, `spawnSync` reports `status: null` + * and a guard that only branches on `status !== 0` then reads `null !== 0` as + * truthy and routes the result onto its failure branch — "the item does not + * exist", "nothing was applied yet", or a generic "pm failed" — so a too-large + * project (or a killed child) is silently mis-imported instead of reported. + * This collapses that case into one explicit error, worded for the real cause by + * {@link describePmNullStatus}, at every call site that captures pm output and + * must not degrade. + * + * The best-effort rollback sites in {@link compensateCreate} intentionally do + * NOT call this on their own subprocesses: compensation is contractually + * tolerant of a failed subprocess, and a named throw there would abort a sweep + * that exists to leave the tracker consistent despite partial failure. + */ +function assertPmOutputFit(result: SpawnSyncReturns, label: string): void { + if (result.status !== null) return; + throw new CommandError( + `${describePmNullStatus(result, label)}. The result is unusable, so it must never be treated as an empty or absent value.`, + ); +} + /** * Normalize a dedup key value for stable matching. pm lower-cases tags on * storage, so a `csv-key:` tag written from "Fix Bug" comes back as @@ -1265,7 +1328,15 @@ function loadAppliedByTransaction( encoding: "utf-8", maxBuffer: pmListMaxBuffer(), }); - if (r.error || r.status !== 0) return { byRowIndex }; + // A stdout overrun kills the child with `status: null` and an empty stderr; + // the previous guard (`r.error || r.status !== 0`) then returned an EMPTY map, + // so inspect() concluded "nothing was applied" and a resumed atomic import + // silently re-imported every already-applied row — wrong data, not a crash. + // Route that overrun through {@link assertPmOutputFit} as a hard, named error + // instead. A genuine non-zero exit (not a buffer condition) still falls through + // to the best-effort empty map, preserving the resume scan's tolerance. + assertPmOutputFit(r, "list-all"); + if (r.status !== 0) return { byRowIndex }; let items: PmItem[] = []; try { items = JSON.parse(r.stdout).items ?? []; @@ -1288,9 +1359,24 @@ function loadAppliedByTransaction( * longer exists. Used by the compensation guard so `pm close` is only invoked * on items that still exist and are not already closed (closing a closed item * is a pm error). + * + * Tier-2 read: this single-item lookup stays a `pm get` subprocess (the SDK's + * status lives on item metadata, not the located document, and converting it + * would push `async` through the whole sync import graph). It now caps stdout + * at {@link pmListMaxBuffer} and routes a buffer overrun through + * {@link assertPmOutputFit} as a hard, named error. Previously an overrun left + * `status: null`, which `status !== 0` mapped onto `undefined` — i.e. a + * too-large item was silently reported as "does not exist", and the + * compensation guard then skipped a close it should have run. A genuine + * non-zero exit (item truly absent, or a failed lookup such as the + * `fake-get-fail` regression) still returns `undefined` unchanged. */ function itemStatus(pmRoot: string, id: string): ItemStatus | undefined { - const r = spawnSync("pm", ["--path", pmRoot, "get", id, "--json"], { encoding: "utf-8" }); + const r = spawnSync("pm", ["--path", pmRoot, "get", id, "--json"], { + encoding: "utf-8", + maxBuffer: pmListMaxBuffer(), + }); + assertPmOutputFit(r, "get"); if (r.status !== 0) return undefined; try { const parsed = JSON.parse(r.stdout); @@ -1313,6 +1399,14 @@ function itemStatus(pmRoot: string, id: string): ItemStatus | undefined { * create being rolled back must also lose its marker); the close only runs for * a still-open item. Every step tolerates a missing item / absent tag, so a * repeated compensation (e.g. after a crash mid-compensation) is a safe no-op. + * + * The status lookup itself is also tolerated: if `itemStatus` throws (a stdout + * overrun or a signal kill on the `pm get`), the throw is caught and this row's + * compensation becomes a no-op rather than aborting the sweep. The hard error is + * kept on the import READ path ({@link loadAppliedByTransaction}, and the + * verification `itemStatus` in {@link upsertCreate}), where a wrong answer is + * the real danger; here the danger is aborting a best-effort rollback mid-sweep + * and leaving every later applied create un-compensated. */ function compensateCreate( pmRoot: string, @@ -1320,14 +1414,25 @@ function compensateCreate( markers: readonly string[] = [], reason: string = "atomic csv import rolled back", ): void { - const status = itemStatus(pmRoot, id); + // itemStatus throws on a stdout overrun / signal kill; compensation is a + // best-effort sweep, so an overrun in the status lookup itself is a no-op for + // THIS row (the operator can reconcile by id) and must never abort the sweep + // over the remaining applied creates. (itemStatus's only throw is the + // CommandError from assertPmOutputFit; a bare catch is therefore equivalent to + // catching CommandError and avoids an unreachable rethrow branch.) + let status: ItemStatus | undefined; + try { + status = itemStatus(pmRoot, id); + } catch { + return; // status lookup overran/killed: no-op for this row, sweep continues + } if (status === undefined) return; // item no longer exists if (markers.length > 0) { // Best-effort: remove the tx markers so this row is no longer "applied". spawnSync( "pm", ["--path", pmRoot, "update", id, "--remove-tags", markers.join(",")], - { encoding: "utf-8" }, + { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }, ); } if (status === "closed") return; // terminal already; markers stripped above @@ -1341,7 +1446,7 @@ function compensateCreate( const r = spawnSync( "pm", ["--path", pmRoot, "close", id, "--reason", reason, "--validate-close", "off"], - { encoding: "utf-8" }, + { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }, ); if (r.status !== 0 && r.status !== 4) { // Exit 4 is "already closed" / invalid state — treat as already compensated. @@ -1791,22 +1896,77 @@ function upsertCreate( appendRelationalArgs(args, p); if (tags.length > 0) args.push("--tags", tags.join(",")); - const r = spawnSync("pm", args, { encoding: "utf-8" }); - if (r.status !== 0) throw new Error(r.stderr?.trim() || "pm create failed"); + const r = spawnSync("pm", args, { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }); + // Parse the id BEFORE any status check. `pm create` is a WRITE: by the time + // spawnSync returns it may have ALREADY persisted the item, even if its + // receipt then overran the buffer (status null). Asserting before the parse + // would throw with the id still unread, stranding an orphan compensation + // cannot identify. Node retains the FULL captured stdout for outputs under + // one read chunk, and `pm create --json`'s flat receipt is always far under + // that, so the id is recoverable here even when the buffer overran. + // `pm create --json` emits a FLAT receipt — {id, status, changed_field_count} + // — with no `item` wrapper (mutations return a flat receipt; only queries + // such as `pm read`/`pm list` wrap, see upstream pm-cli#888). No `item` + // fallback is kept: it would be dead code no real CLI can exercise, and the + // sibling pm-github package shipped a silent production bug from trusting + // exactly that wrapper — every closed import landed open because the id + // never parsed. let id = ""; try { - // `pm create --json` emits a FLAT receipt — {id, status, changed_field_count} - // — with no `item` wrapper (mutations return a flat receipt; only queries - // such as `pm read`/`pm list` wrap, see upstream pm-cli#888). No `item` - // fallback is kept: it would be dead code no real CLI can exercise, and the - // sibling pm-github package shipped a silent production bug from trusting - // exactly that wrapper — every closed import landed open because the id - // never parsed. const parsed = JSON.parse(r.stdout); id = typeof parsed?.id === "string" ? parsed.id : ""; } catch { id = ""; } + if (r.status === null) { + // The receipt overran the buffer (or the child was killed). The create may + // have persisted an item, so a write that may have taken effect is never + // abandoned un-identified: if the id was recovered, attempt to close the + // orphan DIRECTLY (compensation's own status lookup would overrun under the + // same cap and no-op, so the close is issued here) and INSPECT its result so + // the thrown error reports the recovery close's ACTUAL outcome, never an + // assumed one. The close is attempted exactly once — under a capped buffer + // it fails identically every time, so retrying in a loop would only turn a + // fast, actionable error into a slow one. Without the id, throw naming the + // title for manual reconcile. + // + // GUARANTEE: the extension cannot make `pm create` atomic — it is a single + // subprocess with no transaction the caller can join, so a window always + // exists where the item is persisted and the caller does not yet know it. + // What is guaranteed is that the window is always REPORTED WITH THE ID: the + // recovery close's outcome is inspected and the thrown error names the id + // and the real resulting state (closed, or still-open with the close's + // failure cause), so the operator never stops looking for an open orphan. + if (id) { + const rc = spawnSync( + "pm", + ["--path", pmRoot, "close", id, "--validate-close", "off", "--reason", "csv import create overran its receipt buffer; closing the orphaned created item"], + { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }, + ); + const createCause = describePmNullStatus(r, "create"); + if (rc.status === 0) { + throw new CommandError( + `${createCause}. The create for row (title '${p.title}', id ${id}) may have persisted; the orphaned item ${id} was closed — retry the import.`, + ); + } + // The recovery close failed or returned no usable status: do NOT claim it + // succeeded. Report the orphan as still open and carry the close's own + // failure cause (an overrun/kill via describePmNullStatus for a null + // status; the close's stderr for a non-zero exit) so the operator can + // close the item manually by id. + const closeCause = + rc.status === null + ? describePmNullStatus(rc, "close") + : (rc.stderr?.trim() || "pm close failed"); + throw new CommandError( + `${createCause}. The create for row (title '${p.title}', id ${id}) may have persisted; the recovery close failed (${closeCause}), so the orphaned item ${id} is still open and must be closed manually — retry the import.`, + ); + } + throw new CommandError( + `${describePmNullStatus(r, "create")}. The create for row (title '${p.title}') produced no recoverable id, so any persisted item cannot be identified or rolled back — reconcile manually and retry.`, + ); + } + if (r.status !== 0) throw new Error(r.stderr?.trim() || "pm create failed"); if (p.status === "closed") { // (1) No id recovered: the close cannot be applied and the orphan cannot be // compensated without its id. Hard failure — never a silent success — so @@ -1820,8 +1980,13 @@ function upsertCreate( const cr = spawnSync( "pm", ["--path", pmRoot, "close", id, "--reason", importCloseReason("closed", source)], - { encoding: "utf-8" }, + { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }, ); + // Deliberately NO assertPmOutputFit(cr, "close") here: a null close status + // (receipt overrun or a signal kill) must route INTO the compensation branch + // below rather than bypass it. The close may have persisted before its + // receipt overran; the branch compensates and verifies the outcome either + // way, and the id is already known so nothing is stranded. if (cr.status !== 0) { // (2) The close failed but the item is already persisted as open. // Compensate (close) the orphan so a failed row is all-or-nothing and a @@ -1865,6 +2030,18 @@ function upsertCreate( * through `pm close --reason` with the factual import provenance * ({@link importCloseReason}) — never an invented outcome — because * governance.require_close_reason forbids reason-free terminal transitions. + * + * GUARANTEE (and its limits): `pm update` and the terminal `pm close` are each + * a single non-atomic subprocess, so either may have already taken effect when + * its receipt then overruns the buffer or the child is killed (status null), + * and a non-zero exit is likewise not proof nothing was written. The guarantee + * here is that NO MUTATION IS LEFT UN-IDENTIFIED: every update/close failure + * carries the item id and states explicitly that the mutation may already have + * been applied, so the operator can verify and reconcile by id. The guarantee + * is NOT that no mutation is left behind — an update has no inverse without the + * prior field values, which this extension does not capture, so a failed update + * is reported loudly by id rather than compensated with a second unreviewed + * write on top of a failed one (see the deferred capture-prior-values feature). */ function upsertUpdate( pmRoot: string, @@ -1887,13 +2064,44 @@ function upsertUpdate( // `update` cannot set a closed status; only set non-closed statuses here. if (p.status !== "closed" && p.status !== "canceled") args.push("--status", p.status); - const r = spawnSync("pm", args, { encoding: "utf-8" }); - if (r.status !== 0) throw new Error(r.stderr?.trim() || "pm update failed"); + const r = spawnSync("pm", args, { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }); + // `pm update` is a WRITE: a null status (receipt overrun or a signal kill) + // means the mutation may already have been applied to the item, so the + // failure must carry the item id and state that explicitly — matching the + // create path. The guarantee is that no mutation is left un-IDENTIFIED, NOT + // that no mutation is left behind: an update has no inverse without the prior + // field values, which this extension does not capture, so a failed update is + // reported loudly by id rather than papered over with a second unreviewed + // write on top of a failed one. + if (r.status === null) { + throw new CommandError( + `${describePmNullStatus(r, "update")}. The update for item ${id} may already have been applied — verify the item by id before retrying.`, + ); + } + if (r.status !== 0) { + const updateErr = r.stderr?.trim() || "pm update failed"; + throw new Error( + `pm update failed for item ${id} (${updateErr}); the mutation may already have been applied — verify the item by id before retrying.`, + ); + } - // Apply terminal statuses through the dedicated close command. + // Apply terminal statuses through the dedicated close command. The preceding + // update has already been applied, so a failure here is reported with the id + // and the explicit caveat that the terminal transition may already have taken + // effect too — the close, like the update, is a single non-atomic subprocess. if (p.status === "closed" || p.status === "canceled") { - const cr = spawnSync("pm", ["--path", pmRoot, "close", id, "--reason", importCloseReason(p.status, source)], { encoding: "utf-8" }); - if (cr.status !== 0) throw new Error(cr.stderr?.trim() || "pm close failed"); + const cr = spawnSync("pm", ["--path", pmRoot, "close", id, "--reason", importCloseReason(p.status, source)], { encoding: "utf-8", maxBuffer: pmListMaxBuffer() }); + if (cr.status === null) { + throw new CommandError( + `${describePmNullStatus(cr, "close")}. The update for item ${id} was applied, but the terminal ${p.status} close may already have been applied too — verify the item by id before retrying.`, + ); + } + if (cr.status !== 0) { + const closeErr = cr.stderr?.trim() || "pm close failed"; + throw new Error( + `pm close failed for item ${id} (${closeErr}); the update was applied and the terminal ${p.status} close may already have been applied — verify the item by id before retrying.`, + ); + } } } @@ -2786,4 +2994,16 @@ export { validateFieldMapTargets, checkMapSourcesPresent, }; +// --------------------------------------------------------------------------- +// Internal store helpers exposed for regression testing against oversized pm +// roots. Unlike the pure helpers above, these READ or WRITE the given pm root +// (they are not side-effect free) and are exported ONLY so the buffer-overrun +// and compensation regressions can drive them directly under a capped +// PM_LIST_MAX_BUFFER, independent of the write-bearing `csv import` command +// path. `describePmNullStatus` is a pure classifier exposed so the three +// null-status causes (buffer / spawn-error / signal) can be asserted directly +// with synthetic results. +// --------------------------------------------------------------------------- +export { loadAppliedByTransaction, itemStatus, compensateCreate }; +// `describePmNullStatus` is exported at its declaration above. export type { ParsedRow, ImportRowFilter, DiscoveredField, AutoFieldMapping }; diff --git a/test/atomic.test.ts b/test/atomic.test.ts index 9e6d287..0ec51d6 100644 --- a/test/atomic.test.ts +++ b/test/atomic.test.ts @@ -125,6 +125,21 @@ function realPmPath(): string { * recovered". No real item is created. * - `/fake-close-fail`: every `pm close ...` exits 1 with a simulated * error, to exercise the close-failure / compensation paths. + * - `/fake-create-kill`: `pm create --json` is terminated by SIGKILL + * before it writes anything (no output, no persisted item), so spawnSync + * reports `status: null`, `signal: "SIGKILL"`, no `error`, empty `stdout` — + * a create whose receipt cannot be recovered at all (the signal-kill arm of + * the write-overrun fix). + * - `/fake-close-kill`: every `pm close ...` is terminated by SIGKILL + * before it writes anything, so its `status: null` routes into the + * close-failure compensation branch instead of bypassing it (a null close + * status is not a buffer abort). + * - `/fake-update-fail`: every `pm update ...` exits 1 with a simulated + * error, to exercise the non-zero arm of the update path — a failed update + * is not proof nothing was written, so the error must still name the item. + * - `/fake-update-kill`: every `pm update ...` is terminated by SIGKILL, + * so it returns `status: null` — the arm where the mutation may already have + * been applied and only the id makes it reconcilable. * * Returns a cleanup function that restores PATH/PM_CSV_REAL_PM and removes the * wrapper bin. @@ -136,13 +151,14 @@ function installFakePm(): () => void { const wrapper = [ "#!/usr/bin/env node", "const { spawnSync } = require('child_process');", - "const { existsSync } = require('fs');", + "const { existsSync, writeSync } = require('fs');", "const args = process.argv.slice(2);", "let root = '';", "for (let i = 0; i < args.length; i++) {", " if (args[i] === '--path' || args[i] === '--pm-path') root = args[i + 1] || '';", "}", "function has(s) { return args.indexOf(s) !== -1; }", + "const realPm = process.env.PM_CSV_REAL_PM;", "if (root && existsSync(root + '/fake-create-no-id') && has('create') && has('--json')) {", " process.stdout.write('{}\\n');", " process.exit(0);", @@ -157,7 +173,48 @@ function installFakePm(): () => void { " process.stderr.write('simulated pm get failure (test)\\n');", " process.exit(1);", "}", - "const realPm = process.env.PM_CSV_REAL_PM;", + // Kills `pm create --json` with SIGKILL before any output/persist, so the + // create returns status null + signal SIGKILL + empty stdout: the receipt + // cannot be recovered at all (the write-overrun signal-kill arm). + "if (root && existsSync(root + '/fake-create-kill') && has('create') && has('--json')) {", + " process.kill(process.pid, 'SIGKILL');", + "}", + // Simulates a create whose receipt "overran" with a RECOVERABLE id: delegates + // to the real pm so the item is really created (and a real id assigned), + // writes the full --json receipt to stdout, then SIGKILLs so spawnSync + // reports status:null + signal SIGKILL with the full receipt captured. This + // deterministically exercises the recovered-id recovery-close path WITHOUT + // depending on a real buffer cap (which would also cap the recovery close + // and prevent testing its success/non-zero-failure outcomes). + "if (root && existsSync(root + '/fake-create-overrun') && has('create') && has('--json')) {", + " const r = spawnSync(realPm, args, { encoding: 'utf-8' });", + // writeSync, NOT process.stdout.write: on a pipe the latter is asynchronous + // and the SIGKILL on the next statement cannot be handled, so a queued chunk + // is discarded and the id is silently unrecoverable — the test would then + // assert the wrong branch, intermittently. writeSync returns only once the + // bytes are handed to the fd, so the receipt is always in the pipe first. + " if (r.stdout) writeSync(1, r.stdout);", + " process.kill(process.pid, 'SIGKILL');", + "}", + // Kills `pm close` with SIGKILL before any output/persist, so the close + // returns status null and routes into the compensation branch (a null close + // is not a buffer abort that bypasses compensation). + "if (root && existsSync(root + '/fake-close-kill') && has('close')) {", + " process.kill(process.pid, 'SIGKILL');", + "}", + // Makes `pm update` exit 1 with a simulated stderr, to exercise the + // update-failure arm of upsertUpdate (the mutation may already have been + // applied, so the failure must carry the item id). + "if (root && existsSync(root + '/fake-update-fail') && has('update')) {", + " process.stderr.write('simulated pm update failure (test)\\n');", + " process.exit(1);", + "}", + // Kills `pm update` with SIGKILL before it exits, so it returns status null + // (the receipt-overrun / signal-kill arm of the update path: the mutation + // may already have been applied and must be reported with the id). + "if (root && existsSync(root + '/fake-update-kill') && has('update')) {", + " process.kill(process.pid, 'SIGKILL');", + "}", "const r = spawnSync(realPm, args, { stdio: 'inherit' });", "process.exit(r.status == null ? 1 : r.status);", "", @@ -812,3 +869,342 @@ test("non-atomic closed row: when compensation also fails, the error carries the rmSync(root, { recursive: true, force: true }); } }); + +// --------------------------------------------------------------------------- +// Review follow-up (PR #59): the write-overrun guard must never abandon a write +// that may already have taken effect. `pm create` is a WRITE — by the time its +// receipt overruns the buffer (or the child is killed) the item may already be +// persisted. Asserting before the id is parsed would strand an UN-IDENTIFIABLE +// orphan compensation cannot roll back, and asserting on the close receipt +// bypassed the close-failure compensation branch. The fix parses the id first, +// closes a recovered orphan directly, and lets a null close status route into +// compensation. Each test below is mutation-tested (revert its fix -> it fails). +// --------------------------------------------------------------------------- + +/** + * Run an async `body` with `PM_LIST_MAX_BUFFER` temporarily set to `cap`, + * restoring the prior value (including "unset") on exit. + * + * The cap is read by `pmListMaxBuffer()` at each `spawnSync`, so it must stay + * set for the duration of the awaited body (whose synchronous `spawnSync` calls + * run during the await), not just around the call that starts it. + */ +async function withCappedReadBufferAsync(cap: string, body: () => Promise): Promise { + const prev = process.env.PM_LIST_MAX_BUFFER; + process.env.PM_LIST_MAX_BUFFER = cap; + try { + return await body(); + } finally { + if (prev === undefined) delete process.env.PM_LIST_MAX_BUFFER; + else process.env.PM_LIST_MAX_BUFFER = prev; + } +} + +test("F2: create overrun never strands an un-identifiable orphan — the id is recovered and the recovery close inspected", async () => { + // PM_LIST_MAX_BUFFER smaller than BOTH the create receipt and the recovery + // `pm close` output: `pm create` persists the item then its receipt overruns + // (status null, id recoverable); the direct recovery `pm close` is issued under + // the SAME cap, so it overruns identically (status null) every time. Before + // the fix the recovery close's result was DISCARDED and the error asserted the + // orphan "was best-effort closed" — an outcome that could not be confirmed + // (status null) and that stopped the operator looking for an item still open. + // The fix INSPECTS the recovery close result and, for a null status, reports + // the orphan as still open and to-be-closed manually, carrying the close's own + // overrun cause: the window is always REPORTED WITH THE ID, never assumed away. + const root = freshTracker(); + const file = join(root, "one.csv"); + writeFileSync(file, "title,status\nOverrun Orphan,open\n"); + try { + const { result, error } = await withCappedReadBufferAsync("16", () => runImport(root, file, {})); + assert.ifError(error); + + // The overrun row was NOT counted as imported; it was skipped with a named + // error that carries the recovered id. + assert.equal(result.imported, 0, "the overrun create is not counted as imported"); + assert.equal(result.skipped, 1, "the overrun row is counted as skipped"); + assert.equal(result.errors.length, 1, "the overrun is reported once"); + const msg = result.errors[0]; + assert.match(msg, /id pm-[a-z0-9]+/i, "the recovered id is carried so the state is identifiable"); + + // The CREATE overrun is named (its real cause) ... + assert.match(msg, /pm create overran/i, "the error names the create overrun"); + // ... and so is the RECOVERY close's: it returned status null under the same + // cap, so the error must NOT claim the orphan was closed. It must report the + // orphan as still open, tell the operator to close it manually, and carry the + // close's own overrun cause — an accurate report, not an assumed outcome. + assert.match(msg, /still open/i, "a null recovery-close status is reported as still-open, never as closed"); + assert.match(msg, /must be closed manually/i, "the operator is told to close the orphan by id"); + assert.match(msg, /pm close overran/i, "the recovery close's own overrun cause is carried"); + assert.doesNotMatch(msg, /was closed/i, "the error never asserts the close succeeded when its status is null"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F2: create overrun whose recovery close SUCCEEDS reports the orphan was closed", async () => { + // With the id recovered, the direct recovery close is attempted and its result + // inspected. When it succeeds (status 0) the error must say the orphan WAS + // closed and that the import can be retried — the accurate report for the + // confirmed outcome. The create overrun is simulated deterministically by the + // fake-pm wrapper (it delegates the create to real pm so a real item and id + // exist, emits the full receipt, then signals itself); the recovery close then + // runs against real pm unmodified, so it succeeds normally. This outcome is + // unreachable under a real buffer cap (which would also cap the recovery + // close), so it is exercised only through the deterministic wrapper. + const root = freshTracker(); + writeFileSync(join(root, "fake-create-overrun"), ""); + const file = join(root, "one.csv"); + writeFileSync(file, "title,status\nOverrun Orphan,open\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, {}); + assert.ifError(error); + + assert.equal(result.imported, 0, "the overrun create is not counted as imported"); + assert.equal(result.skipped, 1, "the overrun row is counted as skipped"); + assert.equal(result.errors.length, 1, "the overrun is reported once"); + const msg = result.errors[0]; + assert.match(msg, /id pm-[a-z0-9]+/i, "the recovered id is carried"); + assert.match(msg, /was closed/i, "a successful recovery close is reported as closed"); + assert.match(msg, /retry the import/i, "the operator is told the import can be retried"); + assert.doesNotMatch(msg, /still open/i, "a successful close is not reported as still-open"); + + // The recovery close really took effect: the orphan is closed on disk. + const orphan = listItems(root).find((i) => i.title === "Overrun Orphan"); + assert.ok(orphan, "the created item exists (it was identified, not stranded)"); + assert.equal(orphan!.status, "closed", "the orphan was closed by the recovery close"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F2: create overrun whose recovery close FAILS reports the orphan still open, carrying the close's stderr", async () => { + // When the recovery close returns a non-zero status, the error must report the + // orphan as still open, carry the close's own failure cause (its stderr), and + // tell the operator to close it manually — never claim it was closed. The + // orphan really is left open, because the failed close had no effect. + const root = freshTracker(); + writeFileSync(join(root, "fake-create-overrun"), ""); + writeFileSync(join(root, "fake-close-fail"), ""); + const file = join(root, "one.csv"); + writeFileSync(file, "title,status\nOverrun Orphan,open\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, {}); + assert.ifError(error); + + assert.equal(result.imported, 0, "the overrun create is not counted as imported"); + assert.equal(result.skipped, 1, "the overrun row is counted as skipped"); + assert.equal(result.errors.length, 1, "the overrun is reported once"); + const msg = result.errors[0]; + assert.match(msg, /id pm-[a-z0-9]+/i, "the recovered id is carried"); + assert.match(msg, /still open/i, "a failed recovery close is reported as still-open"); + assert.match(msg, /must be closed manually/i, "the operator is told to close the orphan by id"); + assert.match(msg, /simulated pm close failure \(test\)/i, "the close's own stderr is carried as the cause"); + assert.doesNotMatch(msg, /was closed/i, "a failed close is never reported as closed"); + + // The failed close had no effect: the orphan is open on disk. + const orphan = listItems(root).find((i) => i.title === "Overrun Orphan"); + assert.ok(orphan, "the created item exists (it was identified, not stranded)"); + assert.equal(orphan!.status, "open", "the orphan is left open because the recovery close failed"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F2: a create killed before it writes cannot strand an orphan — the title is named for manual reconcile", async () => { + // A create terminated by a signal before writing anything reports status null + // with EMPTY stdout, so NO id can be recovered. Before the fix the overrun + // guard threw a raw buffer/signal abort; the fix instead throws a named error + // carrying the title (the only identifying clue), so the operator can + // reconcile. Nothing was persisted, so no orphan exists. + const root = freshTracker(); + writeFileSync(join(root, "fake-create-kill"), ""); + const file = join(root, "killed.csv"); + writeFileSync(file, "title,status\nKilled Create,open\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, {}); + assert.ifError(error); + + assert.equal(result.imported, 0, "the killed create is not counted as imported"); + assert.equal(result.skipped, 1, "the killed row is counted as skipped"); + assert.equal(result.errors.length, 1, "the failure is reported once"); + // Worded for the REAL cause (a signal, not a buffer) and naming the title: + assert.match(result.errors[0], /signal/i, "the error names the signal cause, not a buffer overrun"); + assert.match(result.errors[0], /Killed Create/, "the error names the title so the row is identifiable"); + assert.match(result.errors[0], /reconcile manually/i, "the error tells the operator how to recover the unrecoverable id"); + + // The create was killed before it persisted, so nothing was left behind. + assert.equal(listItems(root).length, 0, "no item was created — nothing to strand"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F2: a null close status routes into compensation, never bypasses it", async () => { + // A closed row whose `pm close` is terminated by a signal reports status null. + // Before the fix the close-result guard threw a raw signal abort and BYPASSED + // the close-failure compensation branch, leaving the orphan un-compensated and + // the failure mis-reported as a buffer/signal error. The fix drops that guard + // so a null close routes into compensation: the orphan is compensated (closing + // is also killed, so it cannot take effect) and the failure is reported as a + // compensation outcome (left OPEN), not as a raw signal abort. + const root = freshTracker(); + writeFileSync(join(root, "fake-close-kill"), ""); + const file = join(root, "closekill.csv"); + writeFileSync(file, "title,status\nClose Killed,closed\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, {}); + assert.ifError(error); + + assert.equal(result.imported, 0, "the failed closed row is not counted as imported"); + assert.equal(result.skipped, 1, "the failed closed row is counted as skipped"); + assert.equal(result.errors.length, 1, "the failure is reported once"); + + // The compensation branch ran (the error is a compensation outcome), NOT a + // raw signal/buffer abort from a bypassed guard. + const msg = result.errors[0]; + assert.match(msg, /left OPEN/i, "compensation ran and reported the orphan left open"); + assert.match(msg, /id pm-[a-z0-9]+/i, "the created id is carried so the partial state is actionable"); + assert.doesNotMatch(msg, /terminated by signal.*will not help/i, "the error is not a raw signal abort that bypassed compensation"); + + // The close never took effect (killed before persist): the orphan is open. + const orphan = listItems(root).find((i) => i.title === "Close Killed"); + assert.ok(orphan, "the closed row's created item exists"); + assert.equal(orphan!.status, "open", "the orphan is left open because the close was killed"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +/** + * Pre-create an item carrying a `csv-key` tag so a CSV row with the same key is + * matched as an UPDATE rather than a create. + * + * Runs before {@link installFakePm}, so the seeding create always reaches the + * real `pm` even in tests whose fake intercepts `update`/`close`. + * + * @param root - Tracker root to seed. + * @param key - Value written into the item's `csv-key:` tag. + * @returns The seeded item's id, which the update-path assertions expect the + * thrown error to name. + */ +function seedKeyedItem(root: string, key: string): string { + const r = spawnSync( + "pm", + ["--path", root, "create", "--title", "Seeded Row", "--status", "open", "--priority", "1", "--tags", `csv-key:${key}`, "--json"], + { encoding: "utf-8" }, + ); + assert.equal(r.status, 0, "seeding the keyed item should succeed"); + const id = JSON.parse(r.stdout).id; + assert.ok(id, "the seeded item has an id"); + return id; +} + +test("F-B: an update killed mid-flight names the item, because the mutation may already have been applied", async () => { + // `pm update` is a WRITE. A SIGKILL yields status null with no stderr, which + // the old code routed onto a generic "pm update failed" branch — a row-level + // failure the operator could not reconcile against anything. The row may + // nonetheless have been written. The guarantee this asserts is deliberately + // the narrow one: not that no mutation is left behind (an update has no + // inverse without prior field values, which this extension does not capture), + // but that no mutation is left UN-IDENTIFIED. + const root = freshTracker(); + const id = seedKeyedItem(root, "upd-kill"); + writeFileSync(join(root, "fake-update-kill"), ""); + const file = join(root, "updkill.csv"); + writeFileSync(file, "title,status,priority,key\nSeeded Row,open,3,upd-kill\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, { key: "key" }); + assert.ifError(error); + assert.equal(result.errors.length, 1, "the killed update is reported once"); + const msg = result.errors[0]; + assert.ok(msg.includes(id), "the error names the item id so the state is reconcilable"); + assert.match(msg, /may already have been applied/i, "the error states the mutation may have landed"); + assert.match(msg, /NOT a stdout buffer overrun/i, "a signal kill is not misreported as a buffer overrun"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F-B: an update that exits non-zero still names the item — a failed write is not proof nothing landed", async () => { + // A non-zero exit is weaker evidence than it looks: `pm update` may have + // persisted part of its work before failing. The error therefore carries the + // id and the same "may already have been applied" caveat as the null-status + // arm, rather than implying the item is untouched. + const root = freshTracker(); + const id = seedKeyedItem(root, "upd-fail"); + writeFileSync(join(root, "fake-update-fail"), ""); + const file = join(root, "updfail.csv"); + writeFileSync(file, "title,status,priority,key\nSeeded Row,open,3,upd-fail\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, { key: "key" }); + assert.ifError(error); + assert.equal(result.errors.length, 1, "the failed update is reported once"); + const msg = result.errors[0]; + assert.ok(msg.includes(id), "the error names the item id"); + assert.match(msg, /simulated pm update failure/i, "the underlying stderr is preserved, not swallowed"); + assert.match(msg, /may already have been applied/i, "the error does not imply the item is untouched"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F-B: a terminal close killed after a successful update reports BOTH mutations by id", async () => { + // The update landed and the terminal close was then killed. Reporting only + // the close would hide that the row's other fields were already mutated, so + // the error must state that the update WAS applied and that the close may + // have been too — one message covering both halves of the same row. + const root = freshTracker(); + const id = seedKeyedItem(root, "cls-kill"); + writeFileSync(join(root, "fake-close-kill"), ""); + const file = join(root, "clskill.csv"); + writeFileSync(file, "title,status,priority,key\nSeeded Row,closed,3,cls-kill\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, { key: "key" }); + assert.ifError(error); + assert.equal(result.errors.length, 1, "the killed close is reported once"); + const msg = result.errors[0]; + assert.ok(msg.includes(id), "the error names the item id"); + assert.match(msg, /update for item .* was applied/i, "the already-applied update is stated, not hidden behind the close failure"); + assert.match(msg, /close may already have been applied/i, "the close's own uncertainty is stated"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F-B: a terminal close that exits non-zero after a successful update also reports both by id", async () => { + // Same row-level shape as the killed close, reached through the non-zero arm: + // the update is known applied, the close is uncertain, and the operator gets + // the id plus the close's real stderr rather than a bare row failure. + const root = freshTracker(); + const id = seedKeyedItem(root, "cls-fail"); + writeFileSync(join(root, "fake-close-fail"), ""); + const file = join(root, "clsfail.csv"); + writeFileSync(file, "title,status,priority,key\nSeeded Row,closed,3,cls-fail\n"); + const restorePm = installFakePm(); + try { + const { result, error } = await runImport(root, file, { key: "key" }); + assert.ifError(error); + assert.equal(result.errors.length, 1, "the failed close is reported once"); + const msg = result.errors[0]; + assert.ok(msg.includes(id), "the error names the item id"); + assert.match(msg, /update was applied/i, "the already-applied update is stated"); + assert.match(msg, /close may already have been applied/i, "the close's uncertainty is stated"); + } finally { + restorePm(); + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/test/buffer-regression.test.ts b/test/buffer-regression.test.ts new file mode 100644 index 0000000..c7447b7 --- /dev/null +++ b/test/buffer-regression.test.ts @@ -0,0 +1,310 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { spawnSync } from "node:child_process"; + +import { loadAppliedByTransaction, itemStatus, compensateCreate, describePmNullStatus } from "../index.ts"; +import type { SpawnSyncReturns } from "node:child_process"; + +// --------------------------------------------------------------------------- +// Regression tests for the silent pm-shellout buffer-overrun defect. +// +// `spawnSync` captures stdout up to `maxBuffer` (Node's default is 1 MiB). When +// a pm command emits more than that, the child is killed mid-capture and the +// result has `status: null`, an EMPTY `stderr`, and truncated `stdout`. Code +// that branches on `status !== 0` then reads `null !== 0` as truthy and routes +// the overrun onto its failure branch — turning a buffer overflow into a WRONG +// ANSWER ("item does not exist", "nothing applied yet") instead of an error. +// +// The fix is tier-2 at every pm subprocess site: cap stdout at +// `pmListMaxBuffer()` (16 MiB by default) and route a `status: null` overrun +// through `assertPmOutputFit` as a hard, named error, never as an empty or +// absent result. (A truly ceiling-free in-process SDK read is not used here: a +// static `@unbrained/pm-cli` value import regresses this package's documented +// standalone-loadability — `dist/` must load with only its own files — and the +// host-injected `ctx.sdk.client.list` carries its own read-output budget.) +// +// Every test below is mutation-tested: reverting its fix makes it fail. +// --------------------------------------------------------------------------- + +/** + * Create a fresh isolated pm tracker root and return its path. + * + * Mirrors the harness in `atomic.test.ts`: a real `pm init` in a temp dir so + * the reads exercised here hit the same on-disk store the production command + * does, never the repo's own `.agents`. + */ +function freshTracker(): string { + const root = mkdtempSync(join(tmpdir(), "pm-csv-buf-")); + const r = spawnSync("pm", ["init", "--defaults", "--path", root], { encoding: "utf-8" }); + if (r.status !== 0) { + rmSync(root, { recursive: true, force: true }); + throw new Error(`pm init failed: ${r.stderr || r.stdout}`); + } + return root; +} + +/** + * Create one item in `pmRoot`, returning its id. + * + * `tags` are stamped verbatim so the test can plant the importer's internal + * `csv-tx:`/`csv-txrow:` ownership markers exactly as a prior interrupted run + * would have. These creates are tiny, so they use Node's default 1 MiB capture + * and are unaffected by the `PM_LIST_MAX_BUFFER` cap applied only around a read. + */ +function createItem(pmRoot: string, title: string, tags: readonly string[] = []): string { + const args = ["--path", pmRoot, "create", "--title", title, "--status", "open", "--json"]; + if (tags.length > 0) args.push("--tags", tags.join(",")); + const r = spawnSync("pm", args, { encoding: "utf-8" }); + if (r.status !== 0) throw new Error(`pm create (${title}) failed: ${r.stderr || r.stdout}`); + const id = JSON.parse(r.stdout).id; + if (typeof id !== "string" || !id) throw new Error(`pm create (${title}) returned no id`); + return id; +} + +/** + * Run `body` with `PM_LIST_MAX_BUFFER` temporarily set to `cap`, restoring the + * prior value (including "unset") on exit. + * + * The cap simulates a stdout ceiling far tighter than a realistic tracker, so a + * handful of items is enough to overrun it — the same overrun a large project + * hits against Node's 1 MiB default, without committing a multi-megabyte fixture + * or creating tens of thousands of items. + */ +function withCappedReadBuffer(cap: string, body: () => T): T { + const prev = process.env.PM_LIST_MAX_BUFFER; + process.env.PM_LIST_MAX_BUFFER = cap; + try { + return body(); + } finally { + if (prev === undefined) delete process.env.PM_LIST_MAX_BUFFER; + else process.env.PM_LIST_MAX_BUFFER = prev; + } +} + +// The transaction id and per-row ownership markers the atomic importer stamps. +// Kept literal (rather than derived from a file path) so the assertion is +// independent of `atomicTransactionId`'s derivation. +const TX_ID = "csv-import-buftest00"; +const batchTag = `csv-tx:${TX_ID}`; +const rowTag = (i: number) => `csv-txrow:${TX_ID}#${i}`; + +test("itemStatus returns the real status for an item whose `pm get` output exceeds Node's 1 MiB default", () => { + // THE regression: itemStatus had NO maxBuffer, so it sat on Node's 1 MiB + // default. An item whose `pm get --json` exceeds 1 MiB (a large body) left + // `status: null`, which `status !== 0` mapped onto `undefined` — i.e. a real, + // existing item was silently reported as "does not exist", and the + // compensation guard then skipped a close it should have run. The body is + // loaded from a file (--body-file) so it is never constrained by ARG_MAX. + const root = freshTracker(); + const bodyFile = join(root, "big-body.md"); + // ~2 MiB: comfortably past the old 1 MiB ceiling, well under the 16 MiB cap. + writeFileSync(bodyFile, "x".repeat(2 * 1024 * 1024)); + try { + const r = spawnSync( + "pm", + ["--path", root, "create", "--title", "Oversized item", "--status", "open", "--body-file", bodyFile, "--json"], + { encoding: "utf-8" }, + ); + assert.equal(r.status, 0, `setup create failed: ${r.stderr}`); + const id = JSON.parse(r.stdout).id; + + // Default (uncapped) buffer: the raised 16 MiB cap accommodates the 2 MiB + // body, so the real status comes back instead of a silent `undefined`. + const status = itemStatus(root, id); + assert.equal(status, "open", "the real status comes back past the old 1 MiB ceiling, not undefined"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("itemStatus surfaces a buffer overrun as a named error, never a silent 'item missing'", () => { + const root = freshTracker(); + // A minimal item's `pm get --json` is ~850 B, so a 128-byte cap overruns it. + const id = createItem(root, "Item whose get output overruns the cap"); + try { + // Under a 128-byte cap the tier-2 read overruns and must throw a named + // error — NOT return undefined, which the compensation guard reads as + // "item does not exist". This is the contract the oversized-body test + // above relies on at the other end of the scale. + withCappedReadBuffer("128", () => { + assert.throws( + () => itemStatus(root, id), + // Worded for the REAL cause: a stdout overrun is reported by Node as + // error code ENOBUFS, and the message must name the buffer and the fix + // (narrow / raise PM_LIST_MAX_BUFFER) — never read as a silent 'item + // missing'. + /overran its 128-byte stdout buffer.*ENOBUFS.*PM_LIST_MAX_BUFFER/is, + "a stdout overrun must surface as a named error naming ENOBUFS, not as undefined (silent 'item missing')", + ); + }); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("itemStatus still returns undefined for a genuinely absent item (non-zero exit, not a buffer condition)", () => { + // A non-zero `pm get` exit means the item is genuinely absent or unreadable — + // that must still map to `undefined`, never to the buffer-overrun error. This + // is the path the `fake-get-fail` compensation regression in atomic.test.ts + // relies on, and it must be preserved now that a null status is treated + // specially. + const root = freshTracker(); + try { + const status = itemStatus(root, "pm-doesnotexist0000"); + assert.equal(status, undefined, "a genuinely absent item is undefined, not a thrown error"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("loadAppliedByTransaction surfaces a buffer overrun as a named error, never a silent 'nothing applied'", () => { + const root = freshTracker(); + try { + // Two rows already applied (stamped with this transaction's per-row markers) + // plus padding so list-all --json overruns the 128-byte cap below. + createItem(root, "Applied Row 0", [batchTag, rowTag(0)]); + createItem(root, "Applied Row 1", [batchTag, rowTag(1)]); + createItem(root, "Padding one"); + createItem(root, "Padding two"); + + // OLD behavior: the overrun left `status: null`, the guard returned an EMPTY + // map, and a resumed atomic import silently re-imported every already-applied + // row. The tier-2 fix routes the overrun through assertPmOutputFit as a hard, + // named error instead. + withCappedReadBuffer("128", () => { + assert.throws( + () => loadAppliedByTransaction(root, TX_ID), + /overran its 128-byte stdout buffer.*ENOBUFS.*PM_LIST_MAX_BUFFER/is, + "a stdout overrun must surface as a named error naming ENOBUFS, not as a silent empty 'applied' map", + ); + }); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("loadAppliedByTransaction returns the real applied rows under the default (uncapped) buffer", () => { + // Sanity guard for the non-overrun path: with no PM_LIST_MAX_BUFFER override + // the resume scan returns the real applied rows. This is the path production + // takes, and it must keep working now that overruns throw. + const root = freshTracker(); + try { + createItem(root, "Applied Row 0", [batchTag, rowTag(0)]); + createItem(root, "Applied Row 1", [batchTag, rowTag(1)]); + + const { byRowIndex } = loadAppliedByTransaction(root, TX_ID); + assert.equal(byRowIndex.size, 2, "both applied rows are detected"); + assert.ok(byRowIndex.has(0) && byRowIndex.has(1), "the real row indexes (0 and 1) come back"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +// --------------------------------------------------------------------------- +// Follow-up regressions for the three review findings raised on PR #59. +// +// The original fix traded a silent wrong answer for an aborted rollback: the +// shared guard now throws in places that must not throw. These lock in: +// (F1) compensation is tolerant of an overrun in its OWN status lookup; +// (F3) a `status: null` result is worded for its REAL cause — a stdout +// overrun (ENOBUFS), a spawn/system error (other code), or an external +// signal kill (no error) — never a blanket "buffer overrun" message. +// The import-path write regressions (F2: an oversized create receipt never +// strands an un-identified orphan; a null close never bypasses compensation) +// live in atomic.test.ts alongside the other close-failure / compensation tests. +// --------------------------------------------------------------------------- + +/** + * Build a synthetic `status: null` `spawnSync` result, for asserting the + * null-status classifier directly without orchestrating a real killed child. + * + * Only the fields {@link describePmNullStatus} reads are meaningful; the rest + * are dummies that satisfy the `SpawnSyncReturns` shape. Node populates exactly + * these fields for each cause (verified against `@types/node` and empirically): + * a buffer overrun sets `error.code === "ENOBUFS"` (and the kill signal); a + * spawn/system error sets `error` with another code; an external signal kill + * leaves `error` unset and carries `signal`. + */ +function nullStatusResult( + over: { error?: Error; signal?: NodeJS.Signals | null }, +): SpawnSyncReturns { + return { + pid: 0, + output: [null, "", ""], + stdout: "", + stderr: "", + status: null, + signal: over.signal ?? null, + error: over.error, + }; +} + +/** Attach a Node `errno`-style `code` to a plain Error (as Node itself does). */ +function withCode(err: Error, code: string | number): Error { + return Object.assign(err, { code }); +} + +test("F1: compensation tolerates an overrun in its own status lookup (no-op, sweep not aborted)", () => { + // compensateCreate calls itemStatus() to decide whether to close. itemStatus + // now THROWS on a buffer overrun; before the F1 fix that throw propagated out + // of compensateCreate and aborted the compensation sweep before the best-effort + // strip/close subprocesses on the rows still pending. The fix catches the + // overrun and treats THAT row's compensation as a no-op, while the import read + // path (loadAppliedByTransaction, the verification itemStatus) keeps the hard + // error. + const root = freshTracker(); + const id = createItem(root, "Item whose get overruns during compensation", [batchTag, rowTag(0)]); + try { + withCappedReadBuffer("1", () => { + assert.doesNotThrow( + () => compensateCreate(root, id, [batchTag, rowTag(0)], "test rollback"), + "an overrun in the compensation status lookup must be a no-op, not an abort that strands the remaining sweep", + ); + }); + // No-op means no side effect: the item is still open and still carries its + // markers, because compensation could not determine its state. + const g = spawnSync("pm", ["--path", root, "get", id, "--json"], { encoding: "utf-8" }); + assert.equal(JSON.parse(g.stdout).item.status, "open", "the item is left untouched (no-op)"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("F3: a stdout overrun (ENOBUFS) is worded as a buffer overrun naming the fix", () => { + const msg = describePmNullStatus( + nullStatusResult({ error: withCode(new Error("spawnSync pm ENOBUFS"), "ENOBUFS"), signal: "SIGTERM" }), + "get", + ); + assert.match(msg, /overran its .* stdout buffer/i, "names the overrun and the buffer"); + assert.match(msg, /ENOBUFS/, "names the discriminating Node error code"); + assert.match(msg, /PM_LIST_MAX_BUFFER/, "points the operator at the buffer override"); +}); + +test("F3: an external signal kill (no error) is worded as a signal termination, NOT a buffer overrun", () => { + // A child terminated by a signal also reports `status: null`. Telling the + // operator to raise PM_LIST_MAX_BUFFER sends them down the wrong path when the + // real cause was a kill. The signal alone cannot distinguish this from a + // buffer-driven SIGTERM, so the discriminator is the ABSENCE of an ENOBUFS + // error. + const msg = describePmNullStatus(nullStatusResult({ signal: "SIGKILL" }), "get"); + assert.match(msg, /terminated by signal SIGKILL/i, "names the signal"); + assert.match(msg, /NOT a stdout buffer overrun/i, "explicitly steers the operator away from the buffer"); + assert.doesNotMatch(msg, /error code ENOBUFS/, "no ENOBUFS error code is reported for a pure signal kill (only the buffer case carries it)"); +}); + +test("F3: a spawn/system error (non-ENOBUFS code) is worded as a spawn failure, NOT a buffer overrun", () => { + // A spawn failure such as the binary not being found (ENOENT) also yields + // `status: null` with an error, but a code other than ENOBUFS. This is not a + // buffer problem and must not advertise PM_LIST_MAX_BUFFER. + const msg = describePmNullStatus( + nullStatusResult({ error: withCode(new Error("spawn pm ENOENT"), "ENOENT") }), + "get", + ); + assert.match(msg, /could not run to completion/i, "names it a spawn/run failure"); + assert.match(msg, /ENOENT/, "surfaces the real error code"); + assert.match(msg, /NOT a stdout buffer overrun/i, "explicitly steers the operator away from the buffer"); +});