PR #59 - unbraind/pm-csv

What

pm-csv shells out to pm through spawnSync in ~10 places. Node caps captured stdout at maxBuffer; when a tracker's output exceeds it the child is killed mid-capture and spawnSync returns status: null with an empty stderr and truncated stdout. Guards that branch on status !== 0 then read null !== 0 as truthy and route the overrun onto their failure branch — turning a buffer overflow into a wrong answer instead of an error:

itemStatus (pm get) mapped a null status onto undefined ("item does not exist"), so the compensation guard skipped closes it should have run.
loadAppliedByTransaction (pm list-all) swallowed a null/error result into an empty applied-rows map, so a resumed --atomic import silently re-imported every already-applied row.

This is silent data loss, not a crash — and pm-csv is the worst offender tracked in the pm-cli-website-wz6y epic.

Fix

A shared assertPmOutputFit(result, label) treats status === null as a hard, named error that names the buffer and the PM_LIST_MAX_BUFFER override, applied at every pm subprocess site that must not degrade. itemStatus additionally gains the maxBuffer it lacked (it was the only read still on Node's 1 MiB default); the write sites that previously had no buffer now carry one too.

Per-call-site tier assignment

Line	Site	Tier	Change	Reason
1016	`loadKeyIndex` — `pm list-all` (read)	2	unchanged	Already capped (#41) and already throws on error; loud, not silent.
1290	`loadAppliedByTransaction` — `pm list-all` (read)	2	+ `assertPmOutputFit`	Silent bug: swallowed null/error into an empty "applied" map → resumed atomic import re-imported rows. Now throws.
1338	`itemStatus` — `pm get` (read)	2	+ `maxBuffer` + `assertPmOutputFit`	Silent bug: no buffer (1 MiB default) + `status !== 0` → `undefined` ("item missing"). Now capped 16× and overruns throw.
1376	`compensateCreate` — `pm update` (strip markers)	2	+ `maxBuffer`	Best-effort rollback: tolerates a failed subprocess by design, so no assert.
1390	`compensateCreate` — `pm close`	2	+ `maxBuffer`	Best-effort rollback; existing `status !== 0 && !== 4` log already covers null.
1843	`upsertCreate` — `pm create`	2	+ `maxBuffer` + `assertPmOutputFit`	Write; previously uncapped.
1870	`upsertCreate` — `pm close` (terminal apply)	2	+ `maxBuffer` + `assertPmOutputFit`	Write; previously uncapped.
1941	`upsertUpdate` — `pm update`	2	+ `maxBuffer` + `assertPmOutputFit`	Write; previously uncapped.
1947	`upsertUpdate` — `pm close`	2	+ `maxBuffer` + `assertPmOutputFit`	Write; previously uncapped.
2286	`buildCsvExport` — `pm list-all --include-body` (read)	2	unchanged	Already capped (#41) and already names the ENOBUFS overrun.

Why not tier-1 (delete the subprocess, read the store in-process)?

A truly ceiling-free in-process read is not achievable in this package without regressing a documented property:

A static @unbrained/pm-cli value import breaks standalone-loadability. index.ts:23 documents that "standalone-installed extensions load only their own dist/, so @unbrained/pm-cli is not resolvable at runtime" — confirmed empirically: loading dist/index.js off the package resolution path throws ERR_MODULE_NOT_FOUND. (Every existing @unbrained/pm-cli import here is type-only for exactly this reason.)
Dynamic import() is disallowed by the task.
The host-injected ctx.sdk.client.list (PmClient) carries its own read-output budget, so it is not ceiling-free either.

Tier-2 therefore eliminates the silent wrong-answer (overruns now throw loudly with an actionable message) while preserving standalone-loadability and the sync architecture. Standalone load is verified preserved after the fix.

Tests

test/buffer-regression.test.ts (5 tests), each mutation-tested (revert the fix → the test fails; restore → passes):

itemStatus driven past Node's 1 MiB default (a 2 MiB body via --body-file) returns the real status, not undefined. (reverting the itemStatus fix fails this)
itemStatus and loadAppliedByTransaction under a capped PM_LIST_MAX_BUFFER surface the overrun as a named error, not an empty/absent result. (reverting each fix fails its test)
itemStatus still returns undefined for a genuinely absent item (non-zero exit), preserving the fake-get-fail compensation regression.
loadAppliedByTransaction returns the real applied rows under the default (uncapped) buffer — the production path.

Mutation-test results

Revert itemStatus (drop maxBuffer+assertPmOutputFit): the "exceeds 1 MiB" test and the "surfaces a buffer overrun" test FAIL; restore → pass.
Revert loadAppliedByTransaction assertPmOutputFit: the "surfaces a buffer overrun" test FAILS; restore → pass.

Gates

typecheck, coverage (index.ts 83.0/77.5/88.5; thresholds 81/74/88 unchanged, no c8 ignore), build, audit:prod (0 vulns), pack:dry-run, changelog:check — all green. 114 tests pass (109 prior + 5 new).

Tracking

pm-csv-02hc (companion epic pm-cli-website-wz6y).

Summary by Sourcery

Treat pm subprocess stdout buffer overruns as explicit errors instead of silent fallback results across critical read/write paths.

Bug Fixes:

Prevent loadAppliedByTransaction from interpreting buffer overruns as an empty applied-rows map that causes resumed atomic imports to re-import already-applied rows.
Prevent itemStatus from treating oversized pm get output as a missing item, ensuring compensation logic does not silently skip required closes.

Enhancements:

Introduce a shared assertPmOutputFit helper that detects spawnSync status=null conditions and throws a named error including buffer configuration guidance.
Standardize maxBuffer usage for pm spawnSync calls involved in create, update, close, and itemStatus operations to use pmListMaxBuffer and avoid uncapped stdout captures.
Export loadAppliedByTransaction and itemStatus for targeted regression testing of buffer overrun behavior.

Tests:

Add buffer-regression.test.ts to cover pm stdout overrun scenarios for itemStatus and loadAppliedByTransaction, including oversized bodies, capped buffers, genuine missing items, and normal applied-row detection.

Chores:

Record tracking metadata for issue pm-csv-02hc in the .agents/pm history and issues directories.

Summary by cubic

Surfaces pm shell-out stdout overruns and other spawnSync status: null cases as clear errors, hardens compensation/write paths so rollbacks don’t abort or strand orphans, and makes update failures actionable by naming the item and possible write state.

Bug Fixes
- Added describePmNullStatus to tell ENOBUFS overruns from spawn/system errors and external signal kills; used by assertPmOutputFit and write-path errors.
- Reads: itemStatus and loadAppliedByTransaction now set maxBuffer via PM_LIST_MAX_BUFFER and assert; no more silent “missing item” or empty applied map on overruns.
- Writes: upsertCreate parses the id before any throw; on overrun/kill, best-effort closes the recovered orphan and reports the close’s real outcome (closed vs still open with cause). All create/update/close calls set maxBuffer, with asserts on hard-fail paths.
- Updates: upsertUpdate errors include the item id and state the mutation may already be applied, with distinct messages for status: null vs non-zero exits.
- Compensation: compensateCreate catches an overrun in its own itemStatus check and treats that row as a no-op so the sweep continues; compensation subprocesses add maxBuffer but keep tolerant behavior.
- Tests: flush the fake pm create receipt with writeSync before SIGKILL to make the SIGKILL fixture deterministic and avoid false assertions.

Review round 3 (`52f37e4`)

Two Greptile P1s, both the same family as the defect this PR exists to remove — a message that
states something the code never established.

#	Finding	Disposition
A	Recovery close remains unchecked	Fixed — the close result is inspected, not assumed
B	Persisted updates remain unreconciled	Accepted in part — errors now carry the id; update compensation deliberately not added

A — the recovery close was issued and its result discarded, while the thrown error claimed the
orphan "was best-effort closed". If that close itself failed or was killed, the operator was told
the orphan was handled and stopped looking for an item that is still open. Now: status 0 reports
the orphan closed; a null status or non-zero exit reports it still open, to be closed manually,
carrying the close's own cause. Attempted exactly once — under a capped buffer it fails identically
every time, so a retry loop would only turn a fast, actionable error into a slow one.

B — every pm update and terminal pm close failure now carries the item id and states
explicitly that the mutation may already have been applied. The null-status and non-zero arms are
separate, because a non-zero exit is not proof nothing was written; when the terminal close fails
after a successful update, one message reports both halves.

Deliberately not implemented: update compensation. An update has no inverse without the prior
field values, which this extension does not capture, so "compensating" one could only mean writing
some other value over it — a second unreviewed mutation on top of a failed one, which on a
partially-applied import is worse than a loud, identified failure. The docstrings therefore state
the guarantee in its honest, narrower form: no mutation is left un-identified, not no mutation
is left behind. The capability that would widen it is filed as a real item rather than declined.

Evidence

127 tests pass. Four new update-path regression tests, plus two tracker-scoped fake-pm
markers (fake-update-fail, fake-update-kill) beside the existing close fakes.
Mutation-tested. Reverting the upsertUpdate fix fails exactly those four of 28 in
atomic.test.ts; restoring it passes all 28.
Coverage 84.18 / 80.26 / 88.66, up from 83.62 / 78.42 / 88.66. Thresholds 81 / 74 / 88
unchanged, no c8 ignore added.
typecheck clean · audit:prod 0 vulnerabilities · pack 9 files · changelog up to date.

pm items

pm-csv-158c — this round's fixes (closed, with the full evidence note)
pm-csv-71zg — deferred: capture prior field values so an update can be compensated (open)
pm-csv-02hc — rounds 1–2 (closed)