Fix: recoverable classification for closed mktree transports by flyingrobots · Pull Request #20 · git-stunts/plumbing

flyingrobots · 2026-10-02T10:11:28Z

When external Git GC repacks a repository, an already-running mktree --batch process may exit with a stale object-database view. A raw EPIPE, or typed closed-session input when completion is observed first, escapes the protocol-error recovery used by git-cas. This makes attachment checkpoint/GC checks fail depending on notification ordering.

Normalize only these errors from mktree transport writes to GitProtocolError, preserving the original cause. Producer failures and unrelated transport errors retain their identity. Both single-tree and pipelined batch writes use the same boundary. This change adds no retries or reference-publication behavior; git-cas's existing single fresh-process retry remains authoritative.

Validation:

COPY-based Node 22 Docker: all 244 tests and ESLint pass.
Seven protocol regressions pass on Node, Bun and Deno. Three original regressions fail against unchanged source; a later null-details check also fails before its focused fix. Cover both write paths, already-closed input, unrelated errors, and producer errors.
Full COPY-based multi-runtime gate: all three services pass; Node 244 tests, Bun 244 tests, Deno 31 suites/279 steps with all seven new regressions registered. Counts use each runner’s native units and do not imply identical coverage.
git-warp consumer proof in Docker: six deterministic CAS recovery checks and all 19 attachment integration tests pass; the checkpoint/GC case also passes five consecutive focused runs. Retry exhaustion remains bounded to two process attempts.

Related: git-stunts/git-warp#923. The git-warp local patch is temporary; npm consumers need this repair in a published plumbing release before its dependency adoption can replace the patch.

Release candidate metadata is 3.3.2. Registry publication remains pending merge and tag workflow; downstream adoption is tracked in git-stunts/git-cas#131 and draft PR git-stunts/git-cas#132.

coderabbitai · 2026-10-02T10:11:42Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 68771240-5c51-4db3-ba80-e1fb98bfe25f

📥 Commits

Reviewing files that changed from the base of the PR and between 3ef147d and d728cbf.

⛔ Files ignored due to path filters (1)

package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (5)

CHANGELOG.md
package.json
src/infrastructure/protocols/GitMktreeSession.js
test/GitMktreeTransport.test.js
test/deno_entry.js

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ce252276-bf36-4cb0-b732-3d9ee947f608

📥 Commits

Reviewing files that changed from the base of the PR and between 405e34b and 3ef147d.

📒 Files selected for processing (3)

CHANGELOG.md
src/infrastructure/protocols/GitMktreeSession.js
test/GitMktreeTransport.test.js

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (1)

GitHub Check: test-multi-runtime

🔇 Additional comments (3)

src/infrastructure/protocols/GitMktreeSession.js (1)

1-1: LGTM!

Also applies to: 51-51, 54-54, 80-80, 95-109, 287-292

test/GitMktreeTransport.test.js (1)

1-70: LGTM!

CHANGELOG.md (1)

10-16: LGTM!

📝 Summary

Summary by CodeRabbit

Bug Fixes
- Git tree operations now handle broken or already-closed input as protocol failures, allowing affected sessions to be recognized as stale. The original error is retained for diagnosis, while unrelated transport and producer errors continue to be reported unchanged. This improves recovery from failures that can occur after Git data has been repacked.

Walkthrough

mktree single and batch writes now route through _write. The wrapper converts broken-pipe and already-closed-input errors to GitProtocolError and retains the original cause. Other errors pass through unchanged.

Changes

mktree Transport Error Handling

Layer / File(s)	Summary
Route writes through error conversion `src/infrastructure/protocols/GitMktreeSession.js`, `test/GitMktreeTransport.test.js`, `CHANGELOG.md`	Single and batch writes use `_write`. It converts `EPIPE` and `SESSION_INPUT_CLOSED` errors to `GitProtocolError` with the original cause. Tests cover these conversions, unchanged `EACCES` errors, and producer-thrown `EPIPE` errors. The changelog records the behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 3ef14

The mktree error-handling change appears ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3ef14

The change narrowly classifies closed mktree transport failures without adding write authority or local retries. No introduced security defect was established. Downstream retry and reference-publication behavior was not independently verified, leaving limited integration uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The directly evidenced exposure remains the existing caller-selected repository and mktree process. Base/head inspection shows no new public command, repository-selection authority, or reference-publication path in the changed adapter.

Trust Boundaries and Controls

observed — Classification occurs only around the session transport write. Producer iteration remains outside that boundary, so a producer-thrown EPIPE is not converted into the newly recoverable protocol signal. The command, closeInput, and terminate symbols in the added test file belong to a controlled fixture rather than production entrypoints.

Resilience and Maintainability Implications

observed — After an initiated-write failure, local cleanup terminates the process and closes the response reader rather than returning a partial tree result. This contains local protocol ambiguity; safe replay and reference publication still require downstream consumer behavior that is not independently established here.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main change: classifying closed mktree transports for recovery.
Description check	✅ Passed	The description directly explains the mktree transport error classification, preserved causes, affected write paths, and validation results.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

❤️ Share

_{A rabbit taps the mktree stream,
Then guards the pipe against a break.
Closed input keeps its cause in tow,
While other errors stay the same.
The rabbit hops and checks each row.}

_{Comment @coderabbitai help to get the list of available commands.}

flyingrobots · 2026-10-02T10:19:23Z

Independent agy review at 3ef147d1d755d55672131a54249eb74f4ac21a17. Full feedback follows. I will address the defensive error preservation defect and add the batch producer regression before refreshing review. The user has now authorized the plumbing → git-cas → git-warp publication chain.

An adversarial independent review has been conducted for git-stunts/plumbing PR #20 on branch fix/mktree-closed-input-recovery at HEAD 3ef147d1d755d55672131a54249eb74f4ac21a17 against base main at 405e34badaa4133ef2a7bca4ef2cbc671624cd8a (v3.3.1).

The comprehensive technical review artifact has been generated at independent_review_pr20.md. Below is the formal review report and verification checklist.

Findings

Finding 1 (P4 — Defensive Property Access)

Location: src/infrastructure/protocols/GitMktreeSession.js:288-290
Concrete Failure Scenario: isClosedMktreeInput directly accesses error.details.code === 'SESSION_INPUT_CLOSED'. If a caller or test mock instantiates a GitPlumbingError with an explicit null details parameter (new GitPlumbingError('msg', 'op', null)), accessing .code raises an unhandled TypeError: Cannot read properties of null (reading 'code').
Evidence: GitPlumbingError.js:9 defaults details = {}, so normal error paths always provide an object.
Suggested Fix: Use defensive optional chaining: return error.details?.code === 'SESSION_INPUT_CLOSED';.

Finding 2 (P4 — Test Coverage Asymmetry)

Location: test/GitMktreeTransport.test.js:62-68
Concrete Failure Scenario: The test suite verifies producer error preservation when streaming entries into write, but lacks a symmetrical test asserting that a failing async generator passed inside a batch to writeMany preserves the raw producer error and aborts before calling _write.
Evidence: Code review confirms prepareTreeBatch evaluates all iterables prior to subprocess write (protocolStarted remains false), so runtime behavior is sound. However, the regression test suite has an asymmetric coverage gap.
Suggested Fix: Expand the producer failure test to cover both write([entries()]) and writeMany([[entries()]]).

Finding 3 (P5 — Downstream Release & Deployment Constraint)

Location: PR Description / package.json:3
Concrete Failure Scenario: Downstream consumer git-warp (issue #923) currently has green evidence based on an unmerged local patch. Npm consumers and standard CI installations cannot adopt this fix until PR Fix: recoverable classification for closed mktree transports #20 is merged into git-stunts/plumbing, a new release (e.g. v3.3.2) is published to npm, and downstream package manifests are updated.
Evidence: The installed dependency in node_modules/@git-stunts/plumbing remains on v3.3.1.
Suggested Fix: Schedule the v3.3.2 release train immediately upon merge of PR Fix: recoverable classification for closed mktree transports #20 and track the downstream dependency update in git-warp #923.

Mandatory Verification Checklist

Every code path traced (file:line to file:line):
- Single write: GitMktreeSession.js:41-63 -> calls this._write for records and this._write(NUL) for tree delimiter. Catches error, calls this.terminate(), and rethrows.
- Batch write: GitMktreeSession.js:70-93 -> prepares batch via prepareTreeBatch -> calls this._write(payload) -> reads OIDs via _readOid. Catches error, calls this.terminate(), and rethrows.
- Classification helper: GitMktreeSession.js:95-108 -> catches error from CommandSession.write -> checks isClosedMktreeInput -> normalizes EPIPE and SESSION_INPUT_CLOSED to GitProtocolError preserving { cause: error }.
- Parallel protocol comparison: GitCatFileSession.js:238, GitFastImportSession.js:39, GitUpdateRefSession.js:38 audited. GitMktreeSession single/batch write error normalization now mirrors ByteReader._readChunk (ByteReader.js:101-105) which already classified EOF as GitProtocolError.
- Downstream consumer: GitObjectSessionPool.js:84-99, 199-221 -> catches error -> aborts dead session via invalidate -> checks mayRetry && error instanceof GitProtocolError (L216) -> successfully retries once with a fresh process.
Every merge audited (SHA and integration invariants):
- PR Fix: recoverable classification for closed mktree transports #20 branch history (405e34b..3ef147d): Exactly 0 merge commits.
- Base commit 405e34badaa4133ef2a7bca4ef2cbc671624cd8a (PR fix(session): settle stdin closure after child exit #19) audited: PR fix(session): settle stdin closure after child exit #19 shutdown invariants (NodeShellRunner.js:130-136 settling on child close; CommandSession.js:28-29, 53-57 setting _settled = true and throwing SESSION_INPUT_CLOSED) are directly integrated into isClosedMktreeInput.
Every constant and claim checked against evidence:
- Buffer and batch limits (MAX_TREE_NAME_BYTES = 8192, MAX_BATCH_BYTES = 64MB, MAX_BATCH_ENTRIES = 65536, MAX_BATCH_TREES = 256, NUL = 0x00) in GitMktreeSession.js:15-19 verified preserved.
- Downstream retry bound (max 1 retry / 2 process attempts total) verified in GitObjectSessionPool.js:193, 217.
Every document figure checked against raw evidence coordinates:
- Upstream RED log: 3 failed, 2 passed (plumbing-mktree-red.log:66).
- Upstream GREEN log: 32 test files, 242 tests passed, 0 lint errors (plumbing-mktree-validation-final.log:42-50).
- Downstream CAS fault RED log: 4 failed, 2 passed (git-warp-mktree-red-verified.log:44).
- Downstream GREEN log: 25 passed (19 content attachment + 6 recovery) (git-warp-mktree-green-final.log:9).
- Downstream GC focus repetitions: 5 consecutive runs passed (git-warp-mktree-gc-repeat.log:1-51).
- Raw Git exit diagnostic: Exit code 128 fatal: entry 'slot-000000' object fb6522ade99477312f0b778c4885566abf87d214 is unavailable (git-warp-818-gc-diagnostic.log:791).

Status of Checks

Inspected Only: Source code across @git-stunts/plumbing, @git-stunts/git-cas, and @git-stunts/git-warp; git commit SHAs, trees, and diffstats; Docker test logs and raw Git process exit diagnostics in <Docker evidence>/.
Executed: Host-side static repository git inspections (git log, git diff, git rev-list, diff -u).
Skipped / Unavailable: Host-side runtime test execution was not executed (per policy, tests must run inside COPY-based Docker without host mounts; evidence was provided via pre-collected container logs); physical hardware power-loss verification was unavailable; npm publication has not yet been executed.

Final Verdict

APPROVE
The adversarial independent review for git-stunts/plumbing PR #20 (fix/mktree-closed-input-recovery) has been completed.

The detailed review and execution audit are documented in the following artifacts:

Comprehensive Review Artifact: independent_review_pr20.md
Review Summary & Walkthrough: walkthrough.md

Summary of Verdict & Key Points

Behavioral Invariants Preserved:
- Only raw EPIPE and typed SESSION_INPUT_CLOSED (GitPlumbingError) from transport writes are re-wrapped to GitProtocolError.
- Original causes are faithfully preserved on details.cause.
- Producer errors (e.g. failing generator iterables) and unrelated transport errors (e.g. EACCES) retain their identity and bypass protocol conversion.
- Framing, sequential chunking, and process cleanup across single and batch writes (write and writeMany) are preserved.
- Compatibility constraints from PR fix(session): settle stdin closure after child exit #19 shutdown invariants remain completely satisfied.
Evidence Verification:
- Upstream: 3 red regressions confirmed failing on parent v3.3.1; all 5 green at HEAD 3ef147d1d755d55672131a54249eb74f4ac21a17; all 242 tests and ESLint pass cleanly in Node 22 Docker.
- Downstream: 4 CAS recovery tests confirmed failing on unpatched plumbing; 6 passing with the repair; all 19 attachment integration tests and 5 consecutive GC repetitions passing.
Status:
- Zero blocking defects (P0–P2).
- Two minor non-blocking observations (P4) and one release tracking reminder (P5) logged in the review report.

Final Verdict: APPROVE

flyingrobots · 2026-10-02T10:29:52Z

Full refreshed independent review at c214871c3c7a9c127bad7da0eac1bd365848bd09. I verified the missing Deno registration and direct Vitest import; remediation and fresh multi-runtime evidence follow.

Ultra-Strict Adversarial Code Review: git-stunts/plumbing PR #20

PR: git-stunts/plumbing#20 — Fix: recoverable classification for closed mktree transports
Base: 405e34badaa4133ef2a7bca4ef2cbc671624cd8a (main / tag v3.3.1, merge of PR fix(session): settle stdin closure after child exit #19)
HEAD: c214871c3c7a9c127bad7da0eac1bd365848bd09 (fix/mktree-closed-input-recovery)
Reviewer Role: Adversarial Independent Reviewer Gate
Scope & Mode: Read-only static inspection on host; evaluation against verified COPY-based Docker evidence in /tmp/plumbing-mktree-evidence. No host execution was performed.

Executive Summary & Review Verdict

The core algorithmic change in GitMktreeSession.js correctly classifies closed mktree inputs (raw OS EPIPE and typed SESSION_INPUT_CLOSED from CommandSession) into GitProtocolError, retaining the underlying cause in details.cause. This directly enables downstream @git-stunts/git-cas's GitObjectSessionPool to trigger its existing single fresh-process retry after external Git GC/repacking, without introducing retries into plumbing itself, without altering publication or reference semantics, and without corrupting stream framing. Producer errors and unrelated transport errors (such as EACCES) preserve their original error types and error codes.

However, an ultra-strict adversarial audit of the PR at HEAD c214871 reveals an integration defect in the cross-runtime test harness, missing multi-runtime CI evidence, and stale verification metrics in the PR metadata. Specifically, the newly added test suite test/GitMktreeTransport.test.js was not registered in test/deno_entry.js and illegally imports from 'vitest' directly, bypassing the repository's multi-runtime test harness for Deno. Furthermore, no Docker multi-runtime test run (./scripts/run-multi-runtime-tests.sh) was recorded in the evidence directory.

Verdict: REQUEST CHANGES

Verified Findings

Finding 1: `test/GitMktreeTransport.test.js` omitted from Deno test entry and imports `vitest` directly

Severity: P2 (High — Test Coverage & Cross-Runtime Integration Defect)
Coordinates:
- test/GitMktreeTransport.test.js:1
- test/deno_entry.js:1-33
- deno.json:3
Concrete Failure Scenario:
The repository explicitly advertises and supports Deno in package.json ("engines": { "deno": ">=2.0.0" }). In Deno, tests are executed via deno task test, which invokes deno test ... test/deno_entry.js.
1. GitMktreeTransport.test.js is missing from the imports in test/deno_entry.js. Consequently, the multi-runtime Deno container never executes any of the 7 mktree transport regression tests.
2. Unlike all other 31 test files in test/** (which rely on global describe, it, expect configured via test/deno_shim.js and Vitest's --globals), GitMktreeTransport.test.js line 1 contains import { describe, expect, it } from 'vitest';. If GitMktreeTransport.test.js is imported into test/deno_entry.js, Deno execution fails on bare specifier resolution for 'vitest', breaking Deno execution.
Evidence:
- grep -rn "from 'vitest'" test/: Only test/GitMktreeTransport.test.js:1 matched.
- Prior PR fix(session): settle stdin closure after child exit #19 added NodeSessionLifecycle.test.js to test/deno_entry.js in commit 590d4ee; PR Fix: recoverable classification for closed mktree transports #20 neglected this step.
Suggested Fix:
1. Remove import { describe, expect, it } from 'vitest'; from test/GitMktreeTransport.test.js and rely on the global test primitives like all other tests in the repository.
2. Add import './GitMktreeTransport.test.js'; to test/deno_entry.js.

Finding 2: Multi-runtime Docker test evidence absent from evidence artifacts

Severity: P3 (Medium — Incomplete Verification Evidence)
Coordinates: <Docker evidence>/
Concrete Failure Scenario:
The repository's authoritative full test gate is ./scripts/run-multi-runtime-tests.sh (npm test), which builds and executes node-test, bun-test, and deno-test via docker-compose.yml.
All logs in /tmp/plumbing-mktree-evidence (plumbing-release-validation.log, plumbing-review-green.log, plumbing-mktree-validation-final.log) record only npm run test:local (vitest run --globals) and eslint . on Node 22. No log demonstrates execution of ./scripts/run-multi-runtime-tests.sh or the Bun and Deno test services for PR Fix: recoverable classification for closed mktree transports #20. Because Finding 1 silently skipped Deno execution, this omission masked Finding 1.
Evidence:
- <Docker evidence>/plumbing-release-validation.log:2-3: shows only test:local and lint.
- No logs for bun-test or deno-test exist in <Docker evidence>/.
Suggested Fix:
Execute ./scripts/run-multi-runtime-tests.sh in Docker after resolving Finding 1, and save the complete multi-runtime log into the evidence directory.

Finding 3: Stale test counts and regression figures in PR #20 description

Severity: P4 (Low — Documentation & Receipt Discrepancy)
Coordinates:
- PR Fix: recoverable classification for closed mktree transports #20 body, captured in <Docker evidence>/plumbing-lawyer-20-discovery.json:13
Concrete Failure Scenario:
The PR body text states:

"Validation: COPY-based Node 22 Docker: all 242 tests and ESLint pass. Five protocol regressions: three fail against unchanged source; all pass after the fix."
This description was authored at commit 3ef147d. Subsequent commit 638f86b introduced 2 additional tests in test/GitMktreeTransport.test.js (testing null error details preservation and parameterizing single vs batch producer errors), bringing the suite from 5 to 7 tests, and the repository total from 242 to 244 tests.
Evidence:
- <Docker evidence>/plumbing-mktree-validation-final.log:43: 242 tests (at 3ef147d).
- <Docker evidence>/plumbing-release-validation.log:43: 244 tests (at c214871).
- <Docker evidence>/plumbing-review-green.log:23: 7 tests in test/GitMktreeTransport.test.js.
Suggested Fix:
Update the PR Fix: recoverable classification for closed mktree transports #20 description on GitHub to state: "all 244 tests pass; seven protocol regressions in GitMktreeTransport.test.js pass at HEAD".

Finding 4: Missing test size and oracle contract header in `GitMktreeTransport.test.js`

Severity: P4 (Low — Repository Standards Conformity)
Coordinates: test/GitMktreeTransport.test.js:33
Concrete Failure Scenario:
In accordance with testing standards applied in PR fix(session): settle stdin closure after child exit #19 (test/NodeSessionLifecycle.test.js:7-11), new unit test suites establishing contract boundaries should declare their test size and oracle invariants at the top of the suite. test/GitMktreeTransport.test.js lacks these markers.

Evidence:
Compare test/NodeSessionLifecycle.test.js:7-11:

// Medium: real Node streams, controlled process events, no subprocess or clock.
// Oracle: closing session input settles once that input is closed. A process
// may close stdin without Writable's successful-flush `finish` event.

with test/GitMktreeTransport.test.js:33, which begins directly with describe(...).

Suggested Fix:
Add standard header comment:

// Small: mock CommandSession and ByteReader, no subprocess.
// Oracle: mktree transport write failures with EPIPE or SESSION_INPUT_CLOSED
// are normalized to GitProtocolError with cause preserved; unrelated transport
// and producer errors retain their identity.

Finding 5: Release packaging prepared ahead of registry publication and consumer adoption

Severity: P5 (Informational — Release Coordination)
Coordinates:
- package.json:3
- package-lock.json:3,9
- CHANGELOG.md:10-16
Concrete Failure Scenario:
Commit c214871 bumps version to 3.3.2 and dates the changelog 2026-10-02. The npm registry is currently at 3.3.1. Downstream consumers (@git-stunts/git-cas at ^3.3.0 and git-warp at ^3.3.1) have verified the recovery mechanism via a local source checkout / temporary patch-package mitigation, but npm package adoption cannot complete until 3.3.2 is tagged and published.
Evidence:
- <git-warp checkout>/node_modules/@git-stunts/plumbing/package.json: shows version 3.3.1.
Suggested Fix:
Acknowledge that c214871 prepares the release metadata, but tag creation, npm publication, and downstream git-cas/git-warp lockfile updates remain follow-up actions once PR Fix: recoverable classification for closed mktree transports #20 is merged.

Mandatory Verification Checklist

1. Code Path Traceability

Every user- and daemon-visible path delivering the changed behavior was audited line-by-line across both base and HEAD:

Behavior	Base Path	HEAD Path	Status & Rule Parity
Single write framing	`GitMktreeSession.js:50,53` (`this._session.write`)	`GitMktreeSession.js:51,54` (`this._write`)	Verified: Routes through `_write`. If `_write` fails, `protocolStarted` is true, process is terminated via `await this.terminate()`, and `GitProtocolError` is thrown.
Batch write framing	`GitMktreeSession.js:79` (`this._session.write`)	`GitMktreeSession.js:80` (`this._write`)	Verified: Routes through `_write`. Single contiguous `payload` write. If `_write` fails, `protocolStarted` is true, process terminated, and `GitProtocolError` is thrown.
Transport classification	None (threw raw `EPIPE` or `SESSION_INPUT_CLOSED`)	`GitMktreeSession.js:95-108` (`_write`)	Verified: Intercepts `isClosedMktreeInput(error)` and wraps in `GitProtocolError('git mktree input closed before its tree response', 'GitMktreeSession._write', { cause: error })`.
Error code filtering	None	`GitMktreeSession.js:287-292` (`isClosedMktreeInput`)	Verified: Strict predicate. Returns `true` only for `GitPlumbingError` with `details?.code === 'SESSION_INPUT_CLOSED'` or `Error` with `code === 'EPIPE'`. Safe against `null` details.
Single write producer error	`GitMktreeSession.js:46-61`	`GitMktreeSession.js:46-61`	Verified: Producer failure in `for await` is NOT caught by `_write`. If entries were already written, `protocolStarted` terminates process to preserve framing, and re-throws original error unchanged.
Batch write producer error	`GitMktreeSession.js:78,242-260`	`GitMktreeSession.js:78,242-260`	Verified: `prepareTreeBatch` exhausts iterables before `protocolStarted = true` or `_write`. Producer error aborts before write; session is NOT terminated; original error propagates unchanged.
Unrelated transport error	None	`GitMktreeSession.js:106`	Verified: `isClosedMktreeInput` returns `false` for `EACCES` or other errors. Original error is re-thrown as-is.
Session retirement/close	`GitMktreeSession.js:125-147`	`GitMktreeSession.js:125-147`	Verified: `close()` awaits `_tail`, invokes `_session.closeInput()`, awaits `_session.finished`, asserts exit code 0, and closes `ByteReader`. Honors PR #19 shutdown invariants.
Session termination	`GitMktreeSession.js:153-161`	`GitMktreeSession.js:153-161`	Verified: Idempotent. Marks `_closed = true`, signals `_session.terminate()`, awaits `_session.finished`, and closes `ByteReader`.

2. Merge Commit & Integration Audit

Range Audited: 405e34badaa4133ef2a7bca4ef2cbc671624cd8a..c214871c3c7a9c127bad7da0eac1bd365848bd09
Merge Commits in PR: Zero. git log 405e34b..c214871 --merges confirmed empty. PR consists of 3 linear commits (3ef147d, 638f86b, c214871).
Parent Base Merge Integration: Base 405e34b is the merge commit of PR fix(session): settle stdin closure after child exit #19 (fix(session): settle stdin closure after child exit), which integrated commits 21630bb and 590d4ee.
- Invariant 1: closeInput() settles cleanly when child process exits before finish. Verified intact.
- Invariant 2: CommandSession.finished settles cleanly and cleans up stream listeners. Verified intact.
- Invariant 3: GitMktreeSession.close() and terminate() await _session.finished. Verified intact; all 6 test/NodeSessionLifecycle.test.js regressions pass.

3. Claims & Evidence Verification

Claim: 638f86b preserves GitPlumbingError with null details using optional chaining:
- Code check: GitMktreeSession.js:289 uses error.details?.code === 'SESSION_INPUT_CLOSED'.
- RED evidence: <Docker evidence>/plumbing-review-red.log:16-37 shows TypeError: Cannot read properties of null (reading 'code') at line 66 before the fix.
- GREEN evidence: <Docker evidence>/plumbing-review-green.log:23 shows all 7 tests passing. Verified.
Claim: 3 upstream protocol regressions fail against unchanged source; 7 pass at HEAD:
- RED evidence: <Docker evidence>/plumbing-mktree-red.log:4-13 shows 3 failures (single broken pipe, batch broken pipe, already-closed input) and 2 passes (unrelated transport failure, producer failure) on unchanged source.
- GREEN evidence: <Docker evidence>/plumbing-release-validation.log:23 shows all 7 tests passing. Verified.
Claim: Downstream git-warp has 6 deterministic CAS recovery tests and 19 real attachment checks passing:
- RED evidence: <Docker evidence>/git-warp-mktree-red-verified.log:4-10 shows 4 failures in mktree-session-recovery.test.mjs against unpatched plumbing.
- GREEN evidence: <Docker evidence>/git-warp-mktree-green-final.log:4-6 shows all 19 content-attachment.test.ts and all 6 mktree-session-recovery.test.mjs tests passing. Verified.
Claim: Checkpoint/GC case passes 5 consecutive focused runs:
- Evidence: <Docker evidence>/git-warp-mktree-gc-repeat.log:1-51 demonstrates 5 consecutive invocations of checkpoint anchoring: content survives GC after checkpoint, all passing (220ms–396ms). Verified.
Claim: Retry exhaustion remains bounded to two process attempts:
- Code check: <git-cas checkout>/src/infrastructure/adapters/GitObjectSessionPool.js:216 passes mayRetry: false on the retry attempt.
- Test check: <Docker evidence>/git-warp-mktree-red-verified.log:58 asserts expect(openings()).toBe(2). Verified.

4. Constants & Thresholds Audit

MAX_TREE_NAME_BYTES = 8192: GitMktreeSession.js:15. Unchanged.
MAX_BATCH_BYTES = 67,108,864 (64 MiB): GitMktreeSession.js:16. Unchanged.
MAX_BATCH_ENTRIES = 65,536: GitMktreeSession.js:17. Unchanged.
MAX_BATCH_TREES = 256: GitMktreeSession.js:18. Unchanged.
String constants: 'SESSION_INPUT_CLOSED' matches CommandSession.js:55,68; 'EPIPE' matches Node error code; 'GIT_PROTOCOL_ERROR' matches GitProtocolError.js:13.

5. Document Figures & Counts Audit

package.json:3: "version": "3.3.2" — Matches package-lock.json:3 and package-lock.json:9.
CHANGELOG.md:10: ## [3.3.2] - 2026-10-02 — Matches current release date. Exactly one ## [Unreleased] heading exists, satisfying test/Changelog.test.js:8.
plumbing-release-validation.log:
- 32 test files, 244 tests passed in 672ms.
- npm pack --dry-run: 71 total files, tarball size 47.3 kB, unpacked size 186.6 kB.
Discrepancy noted: PR body still cites 242 tests and 5 regressions (addressed in Finding 3).

Execution vs. Inspection Status Declaration

In strict compliance with the review protocol:

Inspected Only (Static Analysis on Host):
- Repository files at base 405e34b and HEAD c214871 in <plumbing checkout>.
- Downstream consumer files in <git-cas checkout> and <git-warp checkout>/node_modules/@git-stunts/git-cas.
- All logs, json discovery descriptors, and diagnostic dumps in <Docker evidence>/.
Executed Directly by this Reviewer: None. (No host runtime tests were requested or run; host source tree was not modified).
Inspected Recorded Docker Executions:
- Upstream RED log: Node 22 Vitest (plumbing-mktree-red.log, plumbing-review-red.log).
- Upstream GREEN log: Node 22 Vitest (plumbing-release-validation.log, plumbing-review-green.log).
- Downstream RED log: Vitest / Node (git-warp-mktree-red-verified.log).
- Downstream GREEN log: Vitest / TypeScript / Mermaid / Markdownlint (git-warp-mktree-green-final.log, git-warp-mktree-gc-repeat.log).
- Raw Git process exit: ODB pack invalidation exit code 128 (git-warp-818-gc-diagnostic.log).
Unavailable / Skipped:
- Deno test execution for GitMktreeTransport.test.js: Skipped by the test runner due to Finding 1.
- Multi-runtime Docker execution (scripts/run-multi-runtime-tests.sh running Node, Bun, and Deno concurrently): Unavailable in /tmp/plumbing-mktree-evidence.

Required Remediation Actions Before Approval

Fix Deno Test Harness:
- In test/GitMktreeTransport.test.js, remove line 1 (import { describe, expect, it } from 'vitest';).
- In test/deno_entry.js, add import './GitMktreeTransport.test.js';.
Execute and Record Full Multi-Runtime Docker Tests:
- Run ./scripts/run-multi-runtime-tests.sh in Docker and provide evidence that node-test, bun-test, and deno-test all exit with status code 0.
Update PR Body Metadata:
- Update PR Fix: recoverable classification for closed mktree transports #20 description to cite 7 protocol regressions and 244 total tests.

REQUEST CHANGES

flyingrobots · 2026-10-02T10:35:54Z

Current release candidate is d728cbf. All seven recovery regressions now execute under Node, Bun and Deno; COPY-based multi-runtime services pass (Node/Bun 244 tests; Deno 31 suites/279 steps). Null-details preservation and batch producer errors are covered. Fresh Node lint and pack checks also pass. Independent current-head agy review is running. Branch rules require approval of the latest push; no tag or npm publication will precede merge. @coderabbitai review

coderabbitai · 2026-10-02T10:36:17Z

@flyingrobots I’ll review the latest push at d728cbf56ac85ccef9b276422e6168a68a9e6ad0, including the cross-runtime test integration and error-preservation regressions.

Thanks for the release guidance. Review does not authorize tagging or npm publication before merge.

✏️ Learnings added

Learnt from: flyingrobots
URL: https://github.com/git-stunts/plumbing/pull/20

Timestamp: 2026-10-02T10:36:15.679Z
Learning: For git-stunts/plumbing, release tags and npm publication must wait until the release PR is merged. Branch rules require approval of the latest push.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

flyingrobots · 2026-10-02T10:38:30Z

Full current-head independent agy review: APPROVE at d728cbf56ac85ccef9b276422e6168a68a9e6ad0. Verified remediation and full mandatory checklist follow. Hosted lint and multi-runtime CI are also green at this same head; npm publication remains pending normal merge.

An ultra-strict read-only adversarial independent review has been conducted for git-stunts/plumbing PR #20 on branch fix/mktree-closed-input-recovery at exact HEAD d728cbf56ac85ccef9b276422e6168a68a9e6ad0 against base main at 405e34badaa4133ef2a7bca4ef2cbc671624cd8a (v3.3.1).

The comprehensive technical review artifact has been generated at independent_review_pr20_head_d728cbf.md. Below is the formal review report and verification checklist.

Findings

Prior Review Findings Resolution at HEAD `d728cbf`

RF-1 (P4 — Defensive Property Access): RESOLVED. isClosedMktreeInput uses optional chaining error.details?.code === 'SESSION_INPUT_CLOSED'. Tested with explicitly null details in test/GitMktreeTransport.test.js:65-71; passes cleanly.
RF-2 (P4 — Batch Producer Coverage Asymmetry): RESOLVED. Single and batch producer failure preservation tests are parameterized across ['single', 'batch'] in test/GitMktreeTransport.test.js:73-83.
RF-3 (P2 — Cross-Runtime Test Harness & Deno Omission): RESOLVED. Direct import { describe, expect, it } from 'vitest' was removed from test/GitMktreeTransport.test.js:1, it.each was replaced with standard for...of loops, and the suite was registered in test/deno_entry.js:9. All 7 regression tests execute and pass under Deno.
RF-4 (P3 — Multi-Runtime Docker Evidence): RESOLVED. Authoritative Docker Compose log plumbing-multi-runtime-final.log proves that Node (244 tests), Bun (244 tests), and Deno (31 suites / 279 steps) all exited with status code 0.
RF-5 (P4 — Stale PR Counts): RESOLVED. PR Fix: recoverable classification for closed mktree transports #20 description updated to cite 244 total tests, 7 protocol regressions, and runner-native units.
RF-6 (P4 — Test Size & Oracle Headers): RESOLVED. Medium test size and oracle contract comments added at test/GitMktreeTransport.test.js:32-33.

Active Finding 1 (P5 — Release Coordination & Deployment Constraint)

Severity: P5 (Informational — Release Coordination)
Location: package.json:3, package-lock.json:3,9, CHANGELOG.md:10-16
Concrete Failure Scenario: Package version is staged as 3.3.2 with dated release notes (2026-10-02), but npm registry remains at 3.3.1. Downstream validation in git-warp issue #923 currently relies on an unmerged local patch. Standard CI and npm consumers cannot adopt this recovery until PR Fix: recoverable classification for closed mktree transports #20 merges into main, tag v3.3.2 is created, and the package is published to npm.
Evidence: PR description notes: "Registry publication remains pending merge and tag workflow; downstream adoption is tracked in Adopt released Plumbing recovery and publish git-cas 6.5.11 git-cas#131 and draft PR Adopt Plumbing mktree recovery for git-cas 6.5.11 git-cas#132."
Suggested Fix: Acknowledge that release metadata is correctly staged for the release train; merge PR Fix: recoverable classification for closed mktree transports #20, complete npm tag publication, and merge downstream dependency adoptions (git-cas #132).

Mandatory Verification Checklist

Every code path traced (file:line to file:line):
- Single write framing: GitMktreeSession.write -> encodes record + NUL delimiter -> sets protocolStarted = true -> calls this._write(framed) -> calls delimiter this._write(NUL) -> awaits _readOid. On error, if protocolStarted, calls this.terminate() and rethrows.
- Batch write framing: GitMktreeSession.writeMany -> prepares batch in memory via prepareTreeBatch -> sets protocolStarted = true -> single contiguous write via this._write(payload) -> reads OIDs via _readOid. On error, calls this.terminate() and rethrows.
- Transport write classification: GitMktreeSession._write -> catches error from CommandSession.write -> evaluates isClosedMktreeInput(error) -> normalizes raw EPIPE and typed SESSION_INPUT_CLOSED to GitProtocolError with { cause: error }.
- Producer failure preservation: In single writes (GitMktreeSession.js:46-61), iterable errors bypass _write, terminate the desynchronized process, and propagate with exact identity. In batch writes (GitMktreeSession.js:78,248), iterable errors fail during prepareTreeBatch before protocolStarted = true, keeping the subprocess alive and uncorrupted while propagating with exact identity.
- Unrelated transport failures: Non-closure transport errors (e.g. EACCES) fail isClosedMktreeInput and propagate unchanged via GitMktreeSession._write:106.
- Parallel protocol parity: ByteReader._readChunk already classifies unexpected output stream EOF (next.done) as GitProtocolError. GitMktreeSession._write establishes symmetrical parity for input stream closure.
- Downstream CAS retry consumer: GitObjectSessionPool.#attempt catches failure -> invalidates session via this.invalidate(protocol, session) -> checks if (mayRetry && error instanceof GitProtocolError) -> initiates a single retry attempt with a fresh process (mayRetry: false), bounding retry attempts to 2 total.
Every merge audited (SHA and integration invariants):
- Branch range 405e34badaa4133ef2a7bca4ef2cbc671624cd8a..d728cbf56ac85ccef9b276422e6168a68a9e6ad0: Zero merge commits (git rev-list --merges confirmed empty).
- Base commit 405e34b (PR fix(session): settle stdin closure after child exit #19 merge) invariants: NodeShellRunner.js:128-135 settling input closure on child exit, and CommandSession.js:28-29, 53-57 raising SESSION_INPUT_CLOSED, are seamlessly integrated into isClosedMktreeInput.
Every constant and claim checked against evidence:
- Buffer limits MAX_TREE_NAME_BYTES = 8192, MAX_BATCH_BYTES = 67,108,864, MAX_BATCH_ENTRIES = 65,536, MAX_BATCH_TREES = 256, NUL = 0x00 verified intact.
- Error constants 'SESSION_INPUT_CLOSED', 'EPIPE', and 'GIT_PROTOCOL_ERROR' verified across implementation and tests.
- Downstream max retry count (1 retry / 2 process attempts total) verified in GitObjectSessionPool.js:193, 216-217 and asserted in git-warp-mktree-red-verified.log:58.
Every document figure checked against raw evidence coordinates:
- Upstream RED on base: 3 failed, 2 passed (plumbing-mktree-red.log:4-13,66).
- Upstream null-details RED: 1 failed, 6 passed (plumbing-review-red.log:9-29,43).
- Upstream Deno registration RED: Vitest internal state error (plumbing-deno-registration-red.log:6-20).
- Upstream Deno registration GREEN: 31 suites / 279 steps passed, including all 7 mktree tests (plumbing-deno-registration-green.log:595-603,627).
- Authoritative Multi-Runtime Gate: Bun 244 tests passed (plumbing-multi-runtime-final.log:1286), Node 244 tests passed (plumbing-multi-runtime-final.log:1308), Deno 31 suites/279 steps passed (plumbing-multi-runtime-final.log:2106); all services exited 0 (plumbing-multi-runtime-final.log:1297,1312,2110).
- Upstream Release Node 22 Validation: 32 test files, 244 tests passed, 0 lint errors, tarball dry-run 71 files, 47.3 kB (plumbing-release-final-validation.log:42-50,133-137).
- Downstream CAS fault RED: 4 failed, 2 passed (git-warp-mktree-red-verified.log:4-10).
- Downstream GREEN: 25 passed (19 content attachment + 6 recovery) (git-warp-mktree-green-final.log:4-6,9).
- Downstream GC repetitions: 5 consecutive passed runs (git-warp-mktree-gc-repeat.log:1-51).
- Raw Git exit diagnostic: Exit code 128 fatal: entry ... is unavailable during repacking (git-warp-818-gc-diagnostic.log:791).

Status of Checks

Inspected Only (Static Analysis on Host):
- Source files at base 405e34b and HEAD d728cbf in <plumbing checkout>.
- Downstream pool adapter in <git-cas checkout>/src/infrastructure/adapters/GitObjectSessionPool.js.
- Downstream recovery suite and patch files in <git-warp checkout>.
- Complete Docker test logs, json discovery descriptors, and diagnostic dumps in <Docker evidence>/.
Executed Directly by Reviewer: Host-side static repository git inspections (git status, git log, git rev-list, git diff).
Skipped / Unavailable:
- Host-side runtime test execution was not executed (per repository policy, tests must run inside COPY-based Docker without host mounts; evidence was evaluated from pre-collected Docker logs).
- Physical power-loss verification was unavailable (software process crash and pipe breakage evidence was audited).
- Npm registry publication is not completed (properly staged as a pre-release candidate).

Final Verdict

APPROVE

Fix: classify closed mktree input as a protocol failure

3ef147d

flyingrobots mentioned this pull request Oct 2, 2026

Fix: adopt published CAS recovery for attachment GC git-stunts/git-warp#925

Merged

coderabbitai Bot previously approved these changes Oct 2, 2026

View reviewed changes

Fix: preserve mktree failures without detail metadata

638f86b

flyingrobots dismissed coderabbitai[bot]’s stale review via 638f86b October 2, 2026 10:20

Release: prepare plumbing 3.3.2 transport recovery

c214871

This was referenced Oct 2, 2026

Adopt released Plumbing recovery and publish git-cas 6.5.11 git-stunts/git-cas#131

Closed

Adopt Plumbing mktree recovery for git-cas 6.5.11 git-stunts/git-cas#132

Merged

Fix: execute mktree recovery regressions across all runtimes

d728cbf

flyingrobots merged commit 8811356 into main Oct 2, 2026
3 checks passed

flyingrobots deleted the fix/mktree-closed-input-recovery branch October 2, 2026 11:06

Navigation Menu

Repository navigation

Fix: recoverable classification for closed mktree transports - #20

Fix: recoverable classification for closed mktree transports#20
flyingrobots merged 4 commits into
mainfrom
fix/mktree-closed-input-recovery

flyingrobots commented Oct 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 2, 2026 •

edited

Loading

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

flyingrobots commented Oct 2, 2026

Uh oh!

flyingrobots commented Oct 2, 2026

Uh oh!

flyingrobots commented Oct 2, 2026

Uh oh!

coderabbitai Bot commented Oct 2, 2026 •

edited

Loading

Uh oh!

flyingrobots commented Oct 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

flyingrobots commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Summary by CodeRabbit

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

flyingrobots commented Oct 2, 2026

Findings

Finding 1 (P4 — Defensive Property Access)

Finding 2 (P4 — Test Coverage Asymmetry)

Finding 3 (P5 — Downstream Release & Deployment Constraint)

Mandatory Verification Checklist

Status of Checks

Final Verdict

Summary of Verdict & Key Points

Uh oh!

flyingrobots commented Oct 2, 2026

Ultra-Strict Adversarial Code Review: git-stunts/plumbing PR #20

Executive Summary & Review Verdict

Verified Findings

Finding 1: test/GitMktreeTransport.test.js omitted from Deno test entry and imports vitest directly

Finding 2: Multi-runtime Docker test evidence absent from evidence artifacts

Finding 3: Stale test counts and regression figures in PR #20 description

Finding 4: Missing test size and oracle contract header in GitMktreeTransport.test.js

Finding 5: Release packaging prepared ahead of registry publication and consumer adoption

Mandatory Verification Checklist

1. Code Path Traceability

2. Merge Commit & Integration Audit

3. Claims & Evidence Verification

4. Constants & Thresholds Audit

5. Document Figures & Counts Audit

Execution vs. Inspection Status Declaration

Required Remediation Actions Before Approval

Uh oh!

flyingrobots commented Oct 2, 2026

Uh oh!

coderabbitai Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyingrobots commented Oct 2, 2026

Findings

Prior Review Findings Resolution at HEAD d728cbf

Active Finding 1 (P5 — Release Coordination & Deployment Constraint)

Mandatory Verification Checklist

Status of Checks

Final Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

flyingrobots commented Oct 2, 2026 •

edited

Loading

coderabbitai Bot commented Oct 2, 2026 •

edited

Loading

Finding 1: `test/GitMktreeTransport.test.js` omitted from Deno test entry and imports `vitest` directly

Finding 4: Missing test size and oracle contract header in `GitMktreeTransport.test.js`

coderabbitai Bot commented Oct 2, 2026 •

edited

Loading

Prior Review Findings Resolution at HEAD `d728cbf`