Repository navigation
fix: surface pm spawnSync stdout overruns as errors, not silent wrong answers - #59
Conversation
… answers
Root cause: pm-csv shells out to `pm` through spawnSync in ~11 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) had NO maxBuffer (Node's 1 MiB default) and 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.
Fix: a shared assertPmOutputFit 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; the write sites (create/update/close) that previously had
no buffer now carry one too. The two best-effort compensation sites keep their
tolerant behavior (no assert) by design.
Tier-1 (delete the subprocess and read the store in-process, no ceiling) is not
used: a static `@unbrained/pm-cli` value import regresses this package's
documented standalone-loadability (dist/ must load with only its own files;
confirmed ERR_MODULE_NOT_FOUND when loaded off the package resolution path),
dynamic import() is disallowed, and the host-injected ctx.sdk.client.list
carries its own read-output budget. Standalone load is verified preserved.
Regression tests (test/buffer-regression.test.ts), each mutation-tested:
- itemStatus driven past Node's 1 MiB default (2 MiB body via --body-file)
returns the real status, not undefined.
- itemStatus and loadAppliedByTransaction under a capped PM_LIST_MAX_BUFFER
surface the overrun as a named error, not an empty/absent result.
- itemStatus still returns undefined for a genuinely absent item (preserves the
fake-get-fail compensation regression).
Refs pm-csv-02hc (companion epic pm-cli-website-wz6y).
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Summary by CodeRabbit
WalkthroughPM subprocess handling now bounds stdout, classifies null statuses, recovers created orphans, and reports uncertain mutations. Compensation remains tolerant. Regression tests cover reads, writes, closes, signals, overruns, and spawn errors. ChangesPM subprocess hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideIntroduces a shared assertPmOutputFit helper to treat pm spawnSync stdout buffer overruns as explicit errors instead of silent wrong answers, wires it into critical pm subprocess call sites, adds maxBuffer caps consistently, exports internal read helpers for regression tests, and adds targeted tests to lock in the new behavior. Sequence diagram for pm stdout overrun handling via assertPmOutputFitsequenceDiagram
participant pmCsv as pmCsvFunction
participant spawnSync as spawnSync
participant pmProcess as pmSubprocess
participant assertPmOutputFit as assertPmOutputFit
participant CommandError as CommandError
pmCsv->>spawnSync: spawnSync("pm", args, { encoding, maxBuffer: pmListMaxBuffer })
spawnSync->>pmProcess: run pm command
pmProcess-->>spawnSync: stdout exceeds maxBuffer
spawnSync-->>pmCsv: result { status: null, stdout: truncated, stderr: "" }
pmCsv->>assertPmOutputFit: assertPmOutputFit(result, label)
assertPmOutputFit->>pmListMaxBuffer: pmListMaxBuffer()
assertPmOutputFit-->>CommandError: new CommandError(message)
CommandError-->>pmCsv: throw
pmCsv-->>pmCsv: abort instead of treating result as empty/absent
alt status not null
pmCsv->>assertPmOutputFit: assertPmOutputFit(result, label)
assertPmOutputFit-->>pmCsv: return
pmCsv-->>pmCsv: branch on result.status (0 vs non-zero)
end
Flow diagram for spawnSync result handling with assertPmOutputFitflowchart TD
A["Call spawnSync with maxBuffer from pmListMaxBuffer"] --> B["Receive result from spawnSync"]
B --> C{result.status === null?}
C -->|Yes| D["assertPmOutputFit throws CommandError naming buffer overrun"]
C -->|No| E{result.status !== 0?}
E -->|Yes| F["Handle non-zero exit (e.g. empty map or undefined)"]
E -->|No| G["Parse result.stdout and proceed with normal data path"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.ts`:
- Around line 1325-1342: Update compensateCreate to catch CommandError thrown by
itemStatus during its compensation status lookups and return without aborting
the rollback sweep, while preserving the hard error for non-compensation
callers. Cover both compensation lookup sites and add a regression test for a
capped-buffer pm get result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a782e85d-0724-4d61-8e63-c83fd355a39a
📒 Files selected for processing (4)
.agents/pm/history/pm-csv-02hc.jsonl.agents/pm/issues/pm-csv-02hc.toonindex.tstest/buffer-regression.test.ts
Greptile SummaryThe PR prevents
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| index.ts | Adds cause-specific null-status diagnostics, caps subprocess output, preserves create identity before error handling, verifies recovery-close outcomes, and identifies potentially persisted update mutations. |
| test/atomic.test.ts | Adds deterministic create, recovery-close, update, and terminal-close failure coverage, including synchronous receipt emission before forced termination. |
| test/buffer-regression.test.ts | Exercises oversized read output and each null-status diagnostic category while preserving genuine missing-item behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Run pm subprocess] --> B{status}
B -->|0| C[Continue with successful result]
B -->|non-zero| D[Report operation and item id]
B -->|null| E{error.code}
E -->|ENOBUFS| F[Report stdout overrun and buffer override]
E -->|other error| G[Report spawn or system failure]
E -->|no error, signal set| H[Report external signal termination]
F --> I{Create id recovered?}
G --> I
H --> I
I -->|Yes| J[Attempt and inspect recovery close]
I -->|No| K[Report title for manual reconciliation]
J --> L{Close succeeded?}
L -->|Yes| M[Report orphan closed]
L -->|No| N[Report item still open with id and cause]
Reviews (4): Last reviewed commit: "test: flush the fake create receipt with..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e orphans Root cause: the shared assertPmOutputFit added to surface stdout overruns as errors trades a silent wrong answer for an aborted rollback in three places where a throw must not happen, because the same status===null signal marks both a buffer overrun AND a child that was killed, and a write may already have persisted by the time its receipt overruns. 1. compensateCreate calls itemStatus to decide whether to close, and itemStatus now throws on an overrun — so the compensation sweep aborted before reaching its own best-effort strip/close subprocesses, leaving every later applied create un-compensated. The docstring promised compensation was a no-op on partial failure; the code disagreed. compensateCreate now catches an overrun in its OWN status lookup and treats that row as a no-op, while the import read path (loadAppliedByTransaction, the verification itemStatus) keeps the hard error where a wrong answer is the real danger. 2. A write overrun stranded an un-identifiable orphan. `pm create` persists the item BEFORE its receipt overruns the buffer, and the guard threw before the id was parsed, so compensation could not identify what to roll back; the same guard on the close result bypassed the close-failure compensation branch. upsertCreate now parses the id FIRST (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), best-effort closes a recovered orphan directly, and throws a named, id-carrying error — never abandoning a write that may have taken effect without identifying it. The close-result guard is dropped so a null close status routes INTO the compensation branch (the id is already known, so nothing is stranded). 3. status===null is not only an overrun: a child terminated by a signal reports it too, and SIGTERM is the SAME signal a buffer-driven kill produces, so the signal alone cannot tell them apart. The old message asserted a buffer overrun and pointed the operator at PM_LIST_MAX_BUFFER even when the real cause was a kill. A shared describePmNullStatus now distinguishes the three causes from the evidence Node actually populates (verified against @types/node SpawnSyncReturns and empirically): error.code==="ENOBUFS" → stdout overrun; error set with another code → spawn/system error; error unset + signal → external signal kill. Each message is worded for its real cause, and is reused by both the read-path assert and the write-path errors. Verified against node_modules/@types/node and confirmed empirically: a buffer overrun sets error.code ENOBUFS and retains the full stdout for outputs under one read chunk; an external signal kill leaves error unset; pm close under a tiny maxBuffer still persists the close (the write precedes the receipt). Gates: typecheck clean; coverage 83.62/78.42/88.66 (up vs 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 + 7 new). Each new test is mutation-tested (revert its fix -> it fails). Standalone-loadability preserved (dist/ loads with only node: builtins; @unbrained/pm-cli imports are type-only). Refs pm-csv-02hc.
…ery update failure Review round 3 on #59. Both findings are the same family as the defect this PR exists to remove: a message that states something the code never established. Finding A (Greptile P1, upsertCreate) - 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 still open. The result is now inspected: 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 (describePmNullStatus for a null status, the close stderr otherwise). The close is 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. Finding B (Greptile P1, upsertUpdate) - a pm update or terminal pm close could persist its mutation and then return a null status or non-zero exit, and the error named neither the item nor the possibility that the write had landed. Every such 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 that nothing was written; when the terminal close fails after a successful update, the message reports both halves. Deliberately NOT implemented: update compensation in the atomic path. An update has no inverse without the prior field values, which this extension does not capture, so "compensating" it 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 guarantee is therefore stated in the docstrings in its honest, narrower form: no mutation is left UN-IDENTIFIED, not no mutation is left behind. The capability that would widen it is tracked as pm-csv-71zg. Tests: four new update-path regression tests, plus two tracker-scoped fake pm markers (fake-update-fail, fake-update-kill) alongside the existing close fakes. Mutation-tested - reverting the upsertUpdate fix fails exactly those four of 28 in atomic.test.ts, and restoring it passes all 28. Gates: typecheck clean; 127 tests pass; coverage 84.18 / 80.26 / 88.66, up from 83.62 / 78.42 / 88.66, thresholds 81 / 74 / 88 unchanged and no c8 ignore added; audit:prod 0 vulnerabilities; pack 9 files; changelog up to date.
|
@coderabbitai full review Round 3 is pushed as Context worth carrying into this review, because it defines what a correct finding looks like here: The invertibility test decides whether compensation is sound. A create has a true inverse The defect class this PR is about is a message that asserts an unverified outcome. Round 1 fixed One implementation note that will look like a gap and is not: Everything green: 127 tests, coverage 84.18 / 80.26 / 88.66 (thresholds 81 / 74 / 88 unchanged), |
|
I will focus on messages that state subprocess or compensation outcomes without observing those outcomes. I will also verify that create compensation remains limited to invertible create operations. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
2009-2018: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWrap the verification
itemStatusso a null-status lookup still reports the item id.Line 2009 calls
itemStatusdirectly. Since this PR,itemStatusthrows aCommandErrorwhen itspm getreturnsstatus: null(overrun or signal kill). That throw replaces the message built on Lines 2015-2017, so the failure loses the item id and the title. The comment on Lines 2003-2008 still assumes the lookup only returnsundefinedwhen it fails.This is reachable in the same conditions this PR targets: a small
PM_LIST_MAX_BUFFERmakes thepm closereceipt overrun,compensateCreateno-ops on its own overrun, and the verification lookup then throws a raw buffer message with no id. The created item is left open and un-identified, which contradicts the stated guarantee that every possibly-applied mutation is reported with its id.Catch the throw and treat it as an unverified compensation, so the existing message is still produced.
🛠️ Proposed fix
- const compensatedStatus = itemStatus(pmRoot, id); + // The verification lookup can itself return `status: null` (overrun or + // signal kill) and throw. Treat that as "unverified" so the failure still + // names the id and title, instead of surfacing a raw buffer error that + // identifies nothing. + let compensatedStatus: ItemStatus | undefined; + try { + compensatedStatus = itemStatus(pmRoot, id); + } catch { + compensatedStatus = undefined; + } if (compensatedStatus !== "closed") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.ts` around lines 2009 - 2018, Wrap the verification call to itemStatus in the compensation failure path so CommandError or other lookup failures are converted to an unverified status instead of escaping. Preserve the existing compensatedStatus message branch, ensuring failures still report the item title and id while retaining the existing handling for returned statuses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/pm/history/pm-csv-158c.jsonl:
- Around line 1-4: Regenerate the PM audit records using actual UTC event times
no later than August 8, 2026, preserving append-only history by adding
corrective entries rather than rewriting persisted events. Update
.agents/pm/history/pm-csv-158c.jsonl lines 1-4 with corrected timestamps and
matching hashes; regenerate .agents/pm/history/pm-csv-71zg.jsonl lines 1-1 with
its actual creation time; then regenerate the materialized metadata in
.agents/pm/features/pm-csv-71zg.toon lines 8-9 and
.agents/pm/issues/pm-csv-158c.toon lines 8-17 from the corrected histories,
including dependency and note timestamps.
In `@test/atomic.test.ts`:
- Around line 189-193: Replace the asynchronous process.stdout.write call in the
fake-create-overrun handling block with a synchronous fs.writeSync(1, ...)
write, preserving the existing stdout content and ensuring it completes before
process.kill sends SIGKILL.
---
Outside diff comments:
In `@index.ts`:
- Around line 2009-2018: Wrap the verification call to itemStatus in the
compensation failure path so CommandError or other lookup failures are converted
to an unverified status instead of escaping. Preserve the existing
compensatedStatus message branch, ensuring failures still report the item title
and id while retaining the existing handling for returned statuses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0fd994ab-9481-4874-ac93-82d720b3353e
📒 Files selected for processing (9)
.agents/pm/features/pm-csv-71zg.toon.agents/pm/history/pm-csv-02hc.jsonl.agents/pm/history/pm-csv-158c.jsonl.agents/pm/history/pm-csv-71zg.jsonl.agents/pm/issues/pm-csv-02hc.toon.agents/pm/issues/pm-csv-158c.toonindex.tstest/atomic.test.tstest/buffer-regression.test.ts
process.stdout.write on a pipe is asynchronous, and the SIGKILL on the next statement cannot be handled, so a queued chunk is discarded. When that happened the receipt never reached the parent, the id was not recovered, and the test silently exercised the no-id branch while asserting the recovered-id one - a non-deterministic test that fails INTO THE WRONG ASSERTION rather than into an error, which would have read as flake and been 'fixed' by a retry. writeSync(1, ...) returns only once the bytes are handed to the fd. The hazard is named at the call site so the more idiomatic form is not restored. Found by CodeRabbit. Notable that the harness had the same failure mode as the code under test: this PR is about a subprocess killed after its side effect but before its output is readable, and the fixture reproduces that by killing a subprocess.
|
@coderabbitai full review
The other finding in that batch (pm ledger timestamps read as future-dated) was declined with Nothing else changed since the round-3 review. |
|
The ledger timestamp finding does not apply. The event timestamps precede the review timestamp on 2026-08-09. I withdraw that finding.
|
|
Merging. All checks green ( @coderabbitai — thank you for withdrawing the timestamp finding explicitly rather than letting Three rounds, one defect class. Worth stating plainly for both of you, because it is the most
Every round found the same shape one level deeper: code asserting a result it did not observe. The guarantee this PR ships is deliberately narrower than it could have been made to sound: no |
|
Tip For best results, initiate chat on the files or code changes.
The stated guarantee is correct: the code identifies uncertain mutations. It does not claim that every mutation was reversed. I will apply this review rule in future subprocess and recovery paths: code and test fixtures must not assert an outcome unless they observed that outcome. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
|
Confirmed — the rule is saved and will flag any future PR that adds update compensation without prior-value capture in the transaction record. The three-round progression you described is a clean example of the pattern worth naming: each round the defect moved one step closer to the claim site rather than to the actual observed state. Round 3 being sharpest because the fixture had the same failure mode as the code under test is the kind of thing that usually only gets noticed in retrospect. Worth the write-up. Good luck with the merge. |
1 participant
What
pm-csvshells out topmthroughspawnSyncin ~10 places. Node caps captured stdout atmaxBuffer; when a tracker's output exceeds it the child is killed mid-capture andspawnSyncreturnsstatus: nullwith an emptystderrand truncatedstdout. Guards that branch onstatus !== 0then readnull !== 0as 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 ontoundefined("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--atomicimport 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-wz6yepic.Fix
A shared
assertPmOutputFit(result, label)treatsstatus === nullas a hard, named error that names the buffer and thePM_LIST_MAX_BUFFERoverride, applied at every pm subprocess site that must not degrade.itemStatusadditionally gains themaxBufferit 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
loadKeyIndex—pm list-all(read)loadAppliedByTransaction—pm list-all(read)assertPmOutputFititemStatus—pm get(read)maxBuffer+assertPmOutputFitstatus !== 0→undefined("item missing"). Now capped 16× and overruns throw.compensateCreate—pm update(strip markers)maxBuffercompensateCreate—pm closemaxBufferstatus !== 0 && !== 4log already covers null.upsertCreate—pm createmaxBuffer+assertPmOutputFitupsertCreate—pm close(terminal apply)maxBuffer+assertPmOutputFitupsertUpdate—pm updatemaxBuffer+assertPmOutputFitupsertUpdate—pm closemaxBuffer+assertPmOutputFitbuildCsvExport—pm list-all --include-body(read)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:
@unbrained/pm-clivalue import breaks standalone-loadability.index.ts:23documents that "standalone-installed extensions load only their owndist/, so@unbrained/pm-cliis not resolvable at runtime" — confirmed empirically: loadingdist/index.jsoff the package resolution path throwsERR_MODULE_NOT_FOUND. (Every existing@unbrained/pm-cliimport here is type-only for exactly this reason.)import()is disallowed by the task.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):itemStatusdriven past Node's 1 MiB default (a 2 MiB body via--body-file) returns the real status, notundefined. (reverting the itemStatus fix fails this)itemStatusandloadAppliedByTransactionunder a cappedPM_LIST_MAX_BUFFERsurface the overrun as a named error, not an empty/absent result. (reverting each fix fails its test)itemStatusstill returnsundefinedfor a genuinely absent item (non-zero exit), preserving thefake-get-failcompensation regression.loadAppliedByTransactionreturns the real applied rows under the default (uncapped) buffer — the production path.Mutation-test results
itemStatus(dropmaxBuffer+assertPmOutputFit): the "exceeds 1 MiB" test and the "surfaces a buffer overrun" test FAIL; restore → pass.loadAppliedByTransactionassertPmOutputFit: 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, noc8 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:
Enhancements:
Tests:
Chores:
Summary by cubic
Surfaces
pmshell-outstdoutoverruns and otherspawnSyncstatus: nullcases 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.describePmNullStatusto tell ENOBUFS overruns from spawn/system errors and external signal kills; used byassertPmOutputFitand write-path errors.itemStatusandloadAppliedByTransactionnow setmaxBufferviaPM_LIST_MAX_BUFFERand assert; no more silent “missing item” or empty applied map on overruns.upsertCreateparses 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 setmaxBuffer, with asserts on hard-fail paths.upsertUpdateerrors include the item id and state the mutation may already be applied, with distinct messages forstatus: nullvs non-zero exits.compensateCreatecatches an overrun in its ownitemStatuscheck and treats that row as a no-op so the sweep continues; compensation subprocesses addmaxBufferbut keep tolerant behavior.pm createreceipt withwriteSyncbeforeSIGKILLto make the SIGKILL fixture deterministic and avoid false assertions.Written for commit 7846ed4. Summary will update on new commits.
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.
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 updateand terminalpm closefailure now carries the item id and statesexplicitly 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
pmmarkers (
fake-update-fail,fake-update-kill) beside the existing close fakes.upsertUpdatefix fails exactly those four of 28 inatomic.test.ts; restoring it passes all 28.unchanged, no
c8 ignoreadded.audit:prod0 vulnerabilities · pack 9 files · changelog up to date.pm items