Repository navigation
fix: surface pm spawnSync stdout overruns as errors, not silent wrong answers #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
02ee15c
fix: surface pm spawnSync stdout overruns as errors, not silent wrong…
unbraind cdb1bf3
fix: stop the overrun guard from aborting rollbacks or stranding writ…
unbraind 52f37e4
fix: report the recovery close's real outcome and name the item on ev…
unbraind 7846ed4
test: flush the fake create receipt with writeSync before SIGKILL
unbraind File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: "" |
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"} | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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":""} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.