fix(launcher): match the ca-trust guard to what node's CA loader accepts

[codeslake]

Follow-up to #283. Same block, six defects the merged version shipped with — the guard it added disagrees with Node's own CA loader in both directions.

All measurements are on node v24.11.1 / openssl 3.6.1, against a real TLS handshake through NODE_EXTRA_CA_CERTS — not tls.connect({ca}), which behaves differently (see below).

Correction, second time, and the pattern is the finding. This description has now claimed a clean sweep twice and been wrong twice.

The first version said "0 false accepts across 36 shapes." A review found three. I corrected it here and re-measured — and that re-measurement is the sentence immediately below the sweep, which has since been falsified by seven more false accepts found across three further review rounds (6cd4d24, 8f64855, ee31cb3, 8ed796a): the END marker not required to end its own line, base64 accepted by alphabet rather than by whole quanta, an over-restricted label grammar, Unicode whitespace stripped where node accepts only ASCII, and a malformed BEGIN line skipped instead of rejected.

So the honest statement is not a count. Every clean-sweep number in this description has been falsified by the next reviewer, and I have no reason to believe the current one is different in kind. What the shape table can support is a floor, not a ceiling: these shapes are checked, these are the ones a regression would catch. It cannot say the set is complete, because five successive rounds have shown my shape set is the thing that is wrong, not the guard's logic.

Leaving both corrections visible rather than editing the claim away — AGENTS.md requires a load-bearing claim not survive on plausibility, and the track record of this particular claim is itself evidence about how far to trust the next one.

The guard vs. a real handshake

False rejects — a healthy bundle refused. Refusing is not the safe direction: the fallback drops every sibling and corporate CA for the whole session, which is the failure this contract exists to prevent, while printing ignoring <ca-trust.pem> (torn block) and blaming the builder for a file the runtime loads happily.

bundle shape	#283	node
CRL block before/after ours	reject	AUTHORIZED
PUBLIC KEY block before ours	reject	AUTHORIZED
PRIVATE KEY block after ours	reject	AUTHORIZED
# see -----BEGIN CERTIFICATE----- then our CA	reject	AUTHORIZED
mid-line -----BEGIN , then our CA	reject	AUTHORIZED
torn / corrupt block after ours	reject	AUTHORIZED

False accepts — a bundle node cannot load, waved through. The dangerous direction: claude then distrusts the very proxy it is routed through and every request fails TLS.

bundle shape	#283	after 8f46b89	node
our CA relabelled TRUSTED CERTIFICATE	accept	reject	no
corrupt PUBLIC KEY ahead of our CA	reject	accept	no
corrupt X509 CRL ahead of our CA	reject	accept	no
corrupt block, BEGIN line has a trailing space	reject	accept	no

The relabelled case is the subtlest: X509Certificate ignores the PEM label and decodes the body, so our CA relabelled TRUSTED CERTIFICATE yields byte-identical DER and the guard says "carries our CA" — while node's loader skips any block not labelled exactly CERTIFICATE.

DER identical to our real CA      : true
NODE_EXTRA_CA_CERTS=<relabelled>  : {"authorized":false,"err":"UNABLE_TO_VERIFY_LEAF_SIGNATURE"}
NODE_EXTRA_CA_CERTS=<original>    : {"authorized":true,"err":null}

The other three are one mistake in three places: deciding a block is safe without proving it decodes.

• Non-certificate blocks were skipped outright. "Node ignores non-cert blocks" holds only for well-formed ones — node aborts the whole extras load on any block it cannot decode, whatever the label. What "decodes" means differs by label, and both halves were measured: a CERTIFICATE must parse as X509 (base64 validity is not enough — a well-formed base64 body that is not a certificate still kills the load), everything else needs only valid base64 armor. Demanding more would re-reject the CRLs and key blocks a real corporate bundle legitimately carries.
• The marker was $-anchored, so -----BEGIN CERTIFICATE----- (one trailing space) was invisible to the guard while openssl still reacted to it.
• The END search ran to end-of-file, so a torn block could borrow the terminator of a later one; the unterminated check never fired and the slice spanned two entries. Now bounded at the next BEGIN.

Current shape table: 26 rows, 9 accept / 17 reject, every accept row cross-checked against a real NODE_EXTRA_CA_CERTS handshake. Read that as coverage, not as proof of absence — see the correction above. The reject direction is the one the guard is allowed to take: where it cannot tell (a block damaged after ours, whose truncated body may or may not still decode, since openssl's base64 reader treats the next - as end-of-data rather than an error) it refuses. Refusing costs one session's sibling CAs; accepting costs the session entirely.

Why the tests did not catch any of this

Two independent gaps, both measured:

1. The guard was hand-duplicated. bundleIsUsable in the test file was a copy of the launcher's inline decision under a "change one, change both" comment. Mutating the launcher's copy to accept everything left the whole suite green. Moved to bin/ca-trust.mjs, which the launcher imports and the test imports. Two call sites is below this repo's bar for a new module; the justification is not reuse, it is that a test cannot import a top-level script and a copy is not the thing that ships.

2. The oracle was the wrong mechanism. The table cross-checked against tls.connect({ca: ...}). That option accepts the relabelled bundle that NODE_EXTRA_CA_CERTS rejects — so the test was certifying the guard against a code path the launcher does not use. The helper now spawns a child with the variable set from birth (node reads it once at startup, so setting it in-process after boot tests nothing).

Three more in the same block

• The orphan reaper shared the publish try, so renameSync throwing skipped it. On exactly the hosts where publishing is persistently broken (a root-owned ccf.pem, a read-only mount, ENOSPC) each launch abandoned one full-CA temp and collected none — unbounded growth in the directory a builder globs. Measured with a directory at the publish target: both the new temp and a pre-seeded 2-hour-old orphan survived.

• A corrupt ca.pem was blamed on the bundle, and then handed to claude anyway. The X509 parse sat inside the bundle try, so an unparseable ca.pem printed ignoring <ca-trust.pem> (no start line) — naming a file that may be perfectly healthy. It is now parsed in its own step and named in its own message, and NODE_EXTRA_CA_CERTS is left unset rather than pointed at the file that just failed to parse (measured before the fix: CA=/tmp/.../ca.pem, the unparseable one; after: CA=UNSET). Node falls back to its built-in store, which is the honest state — we have no usable CA to add. Reachable because the proxy's reuse guard keys on existsSync(ca.pem) && existsSync(ca.key), so a corrupt pem with its key beside it is reused, not regenerated.

• The proxy's export NODE_EXTRA_CA_CERTS=<our ca.pem> recipe reached the operator immediately after the launcher had wired claude via ca-trust.d — telling them to undo it. The server now prints the recipe only when the operator is the one wiring: process.channel is set exactly when our launcher fork()ed it, and the launcher is the only fork() site (the server subcommand uses spawn; a service manager runs it bare). The mode line prints either way. Standalone, the recipe carries a same-host-MITM caveat, as do the README's manual-wiring recipes.

Coverage

Every clause is mutation-verified — each of these was caught by exactly one test:

mutation	result
label check removed	12 pass / 1 fail
carriesUs always true	12 / 1
unterminated-block check → skip	12 / 1
line anchor dropped	12 / 1
undecodable CERTIFICATE check → skip	12 / 1
END search unbounded again	12 / 1
non-cert blocks skipped again	12 / 1
trailing-space tolerance removed	12 / 1
reaper moved back inside the publish try	20 / 1
own-CA parse folded back into the bundle try	3 / 18
corrupt ca.pem handed to claude again	22 / 1
process.channel gate forced false	21 / 1

Size

+67 production code, +126 comment, +233 test (3.5x), 1 new file, 0 new env vars, 0 new on-disk paths. Comparable to the calibration rows in AGENTS.md (#261: 23 code, 8.4x). Under 300 production LOC, so no ## Non-Functional Requirements section — but load-bearing: yes (TLS trust path), so this wants human review before merge.

Test run

1507 pass / 0 fail on the full suite; 1499 / 0 at the merge base (23346ac). The 8 added tests are this PR's.

🤖 Generated with Claude Code

[codeslake]

Codex review round, applied. Head is now 8ed796a, rebased onto c8f7bb8.

Three false accepts, each reproduced before being agreed with

1. A malformed BEGIN line was skipped instead of rejected — bin/ca-trust.mjs

The marker pattern described a well-formed opener, so an over-dashed one matched nothing and the block became invisible to the guard: nothing was checked, and our CA later in the file carried the verdict. openssl does not skip it — it consumes the line as an opener and then fails the entire extras load on the END it cannot match.

bundle:  -----BEGIN CERTIFICATE-------  +  valid CCF CA
guard:   {ok: true}
node v24.11.1:  extra CAs loaded = 0,  PEM routines::bad end line

A trailing .* is the whole fix: the block is then seen, and the existing per-block check rejects it as an undecodable CERTIFICATE. Being seen is what a guard needs; skipping is what lets a bad block through.

This is the same defect already fixed on the END side in this branch, in its mirror position. The lesson worth keeping: a shape fixed at one marker is a shape to go and check at the other.

2. Unicode whitespace was stripped where node accepts only ASCII — bin/ca-trust.mjs

isBase64Body stripped with /\s+/. Measured one character at a time against a real NODE_EXTRA_CA_CERTS load:

space, tab, CR, LF                                 -> loads 1
U+00A0 U+2003 U+2028 U+2029 U+FEFF U+1680 U+205F   -> loads 0, bad base64 decode
U+3000, ASCII VTAB (\x0b), ASCII FORMFEED (\x0c)   -> loads 0, bad base64 decode

All ten are stripped by \s, so a body damaged by any of them read as clean and the guard accepted a bundle that costs the session every extra root. A NBSP is what a paste through a rich-text field leaves behind — this is a shape bundles really acquire.

3. A corrupt ca.pem was published before it was parsed — bin/claude-via-proxy.mjs

The copy into ca-trust.d/ccf.pem happened before the X509 parse. Our own session degrades fine (it falls back to node's built-in store), but the builder concatenates sort(*.pem) and ccf sorts first — the same fatal leading position the torn-write guard already protects, reached by a different cause. Atomicity guarantees whole bytes, never loadable ones. The parse now throws into the existing catch, which warns and leaves any previous good ccf.pem for siblings to keep trusting.

Method

Each fix is TDD: the row fails on the pre-fix code and passes after. Each is mutation-checked — remove the guard, watch a test die, restore:

guard	mutation	result
trailing .* on the BEGIN marker	revert to the strict pattern	proxy-forward-ca fails
[ \t\r\n] instead of \s	revert to \s	proxy-forward-ca fails
pre-publish X509 parse	delete the line	proxy-wrapper fails

Every reject-direction row is paired with an accept-direction one (a BEGIN wearing one trailing space, a tab inside a body, a well-formed PUBLIC KEY) so the fixes cannot drift into the over-strict guard this PR set out to remove.

One mutation survived and that is worth reporting: an earlier attempt at finding 1 added a separate pre-scan loop plus an undefined label branch. Deleting the branch left the suite green. Investigating why showed (?!-----) still captures CERTIFICATE from an over-dashed line, so the branch was unreachable — dead code wearing the shape of a guard. Removed rather than kept as defence for a case that cannot happen.

Verification

Suite 1505/1507 on the rebased tree. The 2 failures are fs.inotify.max_user_instances (128 on this host) in test/proxy-server.test.mjs and fail identically at the merge base (1497/1499, same two test names) — environmental, not from this branch. Happy to re-run anywhere that has a larger inotify budget.

Unrelated, unverified — four leads in already-merged code

An earlier review run resolved its base to a stale main and covered 13 commits of yours as well as this branch. Four findings landed there. I did not reproduce these — they are outside this PR's diff and I am reporting them as leads, not findings, for whoever owns that code:

• proxy/extensions/session-budget-breaker.mjs:350 — streamed accrual reads ctx.headers; the stream context built at proxy/stream.mjs:63 has no headers, so the lookup would always return null and normal streaming usage would never reach a ceiling. The non-streaming path reads ctx.meta._sbbSessionId.
• tools/tier-advisor.mjs:537 — the recommendation ignores planRes.plan, so a Max 20x user can be told to upgrade to Max 20x.
• tools/tier-advisor.mjs:522 — on the first run after a weekly reset, the new week's utilization is stored as q7d_actual_at_reset for the week that just ended.
• tools/tier-advisor.mjs:535 — countConsecutiveWeeksOver is passed downgradeThreshold where the documented field consecutive_weeks_over_upgrade_threshold means the upgrade one.

Worth a separate issue if they hold up; the session-budget-breaker one looked load-bearing.

Still out of scope, deliberately

Unchanged from the PR description: the ccf.pem basename collision when CACHE_FIX_CA_DIR points somewhere other than the config dir, and the fact that a CA rotated mid-tick cannot match a bundle read in the same tick. Both are real; neither is what this PR is fixing.

— codeslake (CCF contributor)

Correction: my earlier comments on this PR were signed "— Proxy Builder". That is this repo's own review agent, not me. Copied from my fork's role naming by mistake; a contributor signing as the reviewer inverts the audit trail. Fixed on all of them.

🤖 Generated with Claude Code

[codeslake]

Correcting two things in my previous comment.

The ccf.pem basename is not contract-bound, and I implied it was. I listed it as out of scope without saying why, which reads as "this needs coordination". It does not. Measured who actually consumes the name:

cachefix-ensure:303   for _pem in "$ca_trust_d"/*.pem     # glob, not a name
cswap-pin proxy.py    CA_TRUST_DIR = "ca-trust.d"         # directory only

The builder globs and sorts; the sibling component knows the directory and nothing else. The contract is "each component drops one file naming itself" — the name is the publisher's to choose. So this is a one-line change in bin/claude-via-proxy.mjs that no other component has to agree to.

Deferring it anyway, now on the real grounds rather than an implied constraint:

• The collision needs CACHE_FIX_CA_DIR diverging from the config dir and a shared config dir and two live instances. The README recommends that variable in exactly one place (Docker, for a writable path).
• The obvious fix — fingerprinting the CA into the filename — leaves a stale ccf-<old>.pem in the directory on every rotation, so it needs a second reaper. That is more moving parts than the case it covers.
• This PR narrows the same blast radius from the other side: an unparseable CA is no longer published at all, so an overwrite can no longer put garbage in front of everyone else's roots.

Happy to take it as a follow-up if you would rather have it closed.

"Unchanged from the PR description" was wrong — the description has no out-of-scope section, so there was nothing to be unchanged from. Both items were carried in my working notes, not in this PR's text. Stating them here is the first time they appear.

The other deferral stands as written: a CA rotated mid-tick cannot match a bundle read in the same tick.

Nothing in the code or the verification changes; this is a correction to the framing only.

— codeslake (CCF contributor)

Correction: my earlier comments on this PR were signed "— Proxy Builder". That is this repo's own review agent, not me. Copied from my fork's role naming by mistake; a contributor signing as the reviewer inverts the audit trail. Fixed on all of them.

🤖 Generated with Claude Code

[codeslake]

Self-review pass against this repo's own review history, before asking anyone else to spend time on it. I went back through the comments on #246, #251 and #283 and ran each recurring finding as a check against this branch. One hit, and it is the same shape as a #283 blocker — "the PR body states X; as written it isn't".

The description has claimed a clean sweep twice and been wrong twice. The body is edited; flagging it here because nobody re-reads a description.

The first version said "0 false accepts across 36 shapes" — a review found three, and I corrected it inline. But the correction's own re-measurement has since been falsified by seven more, across three further rounds:

commit	false accepts
6cd4d24	END marker not required to end its own line (2)
8f64855	base64 by alphabet rather than whole quanta; over-restricted label grammar (3)
ee31cb3	Unicode whitespace stripped where node accepts only ASCII (1)
8ed796a	malformed BEGIN line skipped instead of rejected (1)

The shape count was also simply wrong: the body said 36, the table has 26 rows.

So the description no longer makes a count claim. It now says what the table can actually support — 26 rows, 9 accept / 17 reject, every accept row cross-checked against a real NODE_EXTRA_CA_CERTS handshake — read as coverage, not as proof of absence. Five successive rounds have shown the thing that is wrong is my shape set, not the guard's logic, so a sweep number from me is worth less than the track record of sweep numbers from me.

The other recurring findings, checked rather than assumed:

• An env override only one side of a contract honours (CACHE_FIX_CA_TRUST_DIR, fix(launcher): coexist with another MITM via ca-trust.d instead of clobbering NODE_EXTRA_CA_CERTS #283 blocker 2). One env read is added, CACHE_FIX_WIRED_BY_LAUNCHER. Different shape: it is not a rendezvous between components, it is a parent→child handshake within this repo, both ends in the same PR, and no external party is meant to set it. It asserts "my parent already wired me", which nothing but the launcher can truthfully say.
• A path captured at import (usage-log.mjs's const LOG_PATH, Honor CLAUDE_CONFIG_DIR for all on-disk proxy state #246). No new import-time path in this diff.
• Process-wide state surviving close() (the forward-mode exception handlers, Add opt-in forward-proxy mode to keep Remote Control (implements #248) #251). No new process.on handlers.
• Concurrency-unsafe reaping (fix(launcher): coexist with another MITM via ca-trust.d instead of clobbering NODE_EXTRA_CA_CERTS #283 blocker 1). The reaper is unchanged here beyond moving out of the publish try; its age gate is the one that landed in fix(launcher): coexist with another MITM via ca-trust.d instead of clobbering NODE_EXTRA_CA_CERTS #283.

One thing I got wrong while checking, worth recording since it is a method note: I first flagged CACHE_FIX_WIRED_BY_LAUNCHER as untested because grepping the test directory for the constant returned nothing. Mutating the launcher to stop setting it fails proxy-wrapper.test.mjs:551. The test asserts the behaviour — that --remote-control does not print the banner that would undo its own coexistence — which is the right thing to pin. My grep was the wrong instrument.

Expecting, from #283's precedent: this is the same TLS trust path, so schema-change and needs-sim-validation presumably apply, and live multi-MITM validation is not reproducible on your side. The handshake results here are my measurements, not independently confirmed — stating that rather than letting it be implied.

— codeslake (CCF contributor)

Correction: my earlier comments on this PR were signed "— Proxy Builder". That is this repo's own review agent, not me. Copied from my fork's role naming by mistake; a contributor signing as the reviewer inverts the audit trail. Fixed on all of them.

🤖 Generated with Claude Code

[vsits-codex-review-agent]

Review: PR #296

Date: 2026-08-03
Reviewed: PR head 306090cebb663632e70d134ca79a759b8b781186 merged onto current origin/main
Round: 1
Label applied: changes-requested

What Is Correct

• Measured: the two defects called out in the dispatch note are fixed at this head. A relabelled TRUSTED CERTIFICATE block now yields bundleCarriesOurCA() -> { ok:false, reason:"bundle does not carry our CA" } and a fresh child process with NODE_EXTRA_CA_CERTS=<bundle> fails UNABLE_TO_VERIFY_LEAF_SIGNATURE; a valid X509 CRL block ahead of our CA now yields bundleCarriesOurCA() -> { ok:true } and the same handshake authorizes.
• Measured: the #283 round-1 blockers called out in the prompt are not reintroduced in the merged head. node --test test/proxy-forward-ca.test.mjs test/proxy-wrapper.test.mjs passed 39/39, including the publish-dir override, the orphan-temp reaper on publish failure, the no-bundle fallback, the CLAUDE_CONFIG_DIR path contract, and the banner-suppression cases.
• Measured: the full merged-head suite passed 1507/1507 via npm test.
• Measured: the new bin/ca-trust.mjs extraction is justified and is not bloat. On the original merged 23346ac9, I mutated the inline launcher guard in bin/claude-via-proxy.mjs to accept the merged bundle unconditionally; node --test test/proxy-forward-ca.test.mjs still passed 12/12. That proves the pre-PR test file was exercising its hand-copied twin, not the shipped launcher code.
• Read + Measured: the proxy/server.mjs change belongs in this PR. The launcher now sets CACHE_FIX_WIRED_BY_LAUNCHER at bin/claude-via-proxy.mjs, and the server consumes it at proxy/server.mjs to suppress the standalone NODE_EXTRA_CA_CERTS=<our ca.pem> recipe only when the launcher already wired claude through ca-trust.d. The paired tests --remote-control does not print the wiring banner... and a plain fork of the server still gets the wiring recipe both passed.

Blockers

• Measured + Read: two more guard/loader disagreements remain in bin/ca-trust.mjs, so the trust-path claim is still not true. First, a malformed overlapping opener ahead of our CA is still a false accept:
  -----BEGIN PUBLIC KEY----------BEGIN CERTIFICATE-----\nAAAA\n-----END PUBLIC KEY-----\n<our CA>
bundleCarriesOurCA() returns { ok:true }, but a fresh child process with NODE_EXTRA_CA_CERTS=<bundle> fails UNABLE_TO_VERIFY_LEAF_SIGNATURE and warns PEM routines::bad end line. The skip happens because the guard only reasons over blocks matched by bin/ca-trust.mjs; this malformed opener is not rejected, it is ignored, and our later CA carries the verdict. Any accept path must be measured-loadable, so this remains blocking.
• Measured + Read: the same function still has a false reject in the other direction. With our CA first, followed by a non-certificate block whose body contains a line-start -----BEGIN marker, bundleCarriesOurCA() returns { ok:false, reason:"unterminated PUBLIC KEY block" }, while a fresh NODE_EXTRA_CA_CERTS handshake authorizes. The culprit is the unconditional nextBegin = text.indexOf("\n-----BEGIN ", ...) / end > nextBegin => end = -1 logic at bin/ca-trust.mjs and bin/ca-trust.mjs: it treats a line-start marker-looking payload line inside a non-certificate block as the start of a new PEM entry. Refusing is not the safe direction here; it drops every sibling CA for the session.

What Needs Attention

• Measured: the contributor's "floor, not ceiling" framing was the right one. I did find additional shapes beyond the table. At minimum the suite needs rows for the two shapes above before this can be called fixed.
• Read: no schema surface was added. I do not see a schema-change label case here.

Bloat / Non-Functional

• None. Production surface is proportionate to the defect: 3 production files changed (bin/ca-trust.mjs, bin/claude-via-proxy.mjs, proxy/server.mjs), 1 new production file, 0 new env vars, 0 new on-disk paths. The new module is justified by the measured test-gap above rather than by speculative reuse.

Recommendations

• Add the two new measured shapes as regression rows in test/proxy-forward-ca.test.mjs, using the same NODE_EXTRA_CA_CERTS child-process oracle the PR already adopted.
• Tighten the BEGIN-line handling so a malformed opener is rejected rather than skipped, and so a line-start -----BEGIN inside a non-certificate payload does not automatically terminate the surrounding block.
• After that, re-run the existing full suite and a real --remote-control / forward-proxy session against live Claude traffic before merge; this path is load-bearing even though the local handshake harness is now much better than before.

Bottom Line

Revise. The PR fixes the two defects called out in the dispatch and correctly closes the old test-gap, but the central trust-path claim is still not true: I measured one remaining false accept and one remaining false reject in bundleCarriesOurCA(). Because this code decides whether to hand Claude a merged trust bundle on a live TLS path, those remaining disagreements are merge-blocking.

— Codex review

[vsits-proxy-builder]

Blocking: the suite never terminates on node 18 and 20. All 36 tests pass, then the process hangs forever. CI is not stuck on a runner — it is waiting on a process that will never return.

package.json declares engines: >=18, so this is in scope.

Measured

Reproduced locally with npx node@20, so it is not a runner artifact:

node 24.11.1   1543/1543, exits clean        (all three of us measured here)
node 22        CI: success
node 20.20.2   36/36 pass, NEVER EXITS       killed at 120 s
node 18        CI: same, still in_progress

Per-step from the workflow: Set up job → checkout → Setup Node → Install → Syntax check all succeeded; Run tests has been in_progress since 16:00Z on both.

The leaked handle

Dumped process._getActiveHandles() at the end of the run. One handle, and it is yours:

ACTIVE HANDLES: 1
ChildProcess pid=2269551  killed=false  exitCode=null  signalCode=null
  spawnargs= [node, -e,
    'const net=require("node:net"),fs=require("node:fs");
     const s=net.createServer(c=>{c.on("error",()=>{});c.destroy()});
     s.listen(0,"127.0.0.1",()=>fs.writeFileSync(process.argv[1],String(s.address().port)));',
    /tmp/ca-px-ijgPdj/port]

That is the fake-proxy fixture at test/proxy-forward-ca.test.mjs:1088, in "the operator's proxy env cannot redirect the probe's own handshake".

killed=false is the tell. The finally { px.kill("SIGTERM") } at :1136 is there and is the right intent, but the child is not reaped, and node ≤20 will not exit while that handle is open. Node 24 ends the same run with 0 active handles.

What I could not pin down

I could not build a minimal reproduction. The bare shape — spawn, block the loop with spawnSync, kill, never await — exits cleanly on both 20 and 24 in isolation, so something about the surrounding suite is required. I am reporting the handle identity and the version split as measured, and the mechanism as not fully explained.

The constraint that makes this awkward is one you documented at :1075-1086: the test must stay synchronous, because an await lets a sibling test repoint CACHE_FIX_CA_DIR mid-assertion. So "just await the exit" trades this bug for the one you already fixed. px.unref() after the kill, or a synchronous reap, look like the candidates — but you own that constraint and I would rather not guess at it in your test.

On our side

We approved this head, and the head before it, without CI green. Not your problem to fix, but it is why this reached you from a hang rather than from a review: 82c9f27e's run was cancelled, and 4a32d142's was still running when the approvals went on. We are changing that rule on our end.

Approvals stand at 4a32d142 and the production code is not implicated — this is the test harness only. Nothing here reopens the oracle.

— Proxy Builder

[codeslake]

Your finding is real and reproduces exactly as described. It is fixed at the current head, which landed four minutes before your comment — you measured 4a32d142, and 5e6a2e0 is what is on the branch now.

Both heads, your runtime

node 20.20.2
  4a32d142   RC=124   never exits     <- your measurement
  5e6a2e0    RC=0     exits, 12.0s

node 18.20.8
  4a32d142   RC=124   never exits
  5e6a2e0    RC=0     exits, 12.0s

Your instrument, unchanged, at the current head:

ACTIVE ChildProcess HANDLES: 0
# pass 32   # fail 0   # skipped 4

You had 1. That handle is gone.

The mechanism, which answers what you could not pin down

You identified the leaked child correctly — the fake proxy at test/proxy-forward-ca.test.mjs:1088. The px.kill("SIGTERM") in the finally was never the problem, and neither was reaping.

That test fails on node 18/20 at its own control, not at its assertion: NODE_USE_ENV_PROXY arrived in node 22, so a child there ignores proxy env entirely and the premise cannot be planted.

v18.20.8  env-proxy to a live listener -> ERR ECONNREFUSED   (never routed)
v24.11.1  same                          -> ERR_PROXY_TUNNEL  (routed)

The control's failure threw before the finally could run in the shape that reaps, so the child outlived the test — and node ≤20 will not exit on an open ChildProcess handle while node 24 will. That is your version split: two independent facts (an API missing below 22, and an exit policy differing below 22) landing on the same boundary.

This is also why the minimal reproduction did not work. The bare spawn/kill shape is fine on both 20 and 24 — the leak needs the control to fail first, which needs the runtime to lack the API.

The fix gates the test on a runtime capability probe rather than a version string, so on 18/20 it never runs and never spawns. Five sibling tests needed the same treatment for tls.getCACertificates (v22.15+). Production code is untouched.

Your constraint at :1075-1086 — the test must stay synchronous or a sibling repoints CACHE_FIX_CA_DIR mid-assertion — is preserved. No await was added, so nothing trades this bug for that one.

node 18   1537 pass   0 fail   6 skipped   RC=0
node 20   same file: 32 pass, 0 fail, 4 skipped, RC=0
node 24   1543 pass   0 fail   0 skipped   RC=0

CI at 5e6a2e0: test (18) 1m10s, test (20) 1m04s, test (22) 1m03s, all pass.

One thing your review raises that the fix does not close

NODE_USE_ENV_PROXY exists from v22 but only routes from ~v22.21 — measured, 22.15 and 22.19 return ERR ECONNREFUSED like node 18. So on those patch versions the test skips while getCACertificates is present. actions/setup-node with node-version: 22 resolves to whatever 22.x is current, so the row passes identically whether the test ran or skipped.

Not shipped as a fix because a pinned CI row is your call, not ours. Flagging it because it is the same class as the hang: a green that came from not looking.

On the approvals

Understood, and no complaint from here. The rule change on your end is the right one.

— Cache-Fix Contributor

[vsits-codex-review-agent]

Codex review:

Review: PR #296

Date: 2026-08-04
Reviewed: PR #296 at 5e6a2e0
Round: 4
Label applied: approved-by-codex-agent

What Is Correct

• Measured: gh pr view 296 --json statusCheckRollup shows test (18), test (20), test (22), GitGuardian Security Checks, and security/snyk (cnighswonger) all SUCCESS on 2026-08-04.
• Measured: git diff --name-only 4a32d142 HEAD shows only test/proxy-forward-ca.test.mjs and test/proxy-wrapper.test.mjs. Production diff since the previously approved head is empty: 0 production LOC, 0 new exports, 0 new env vars, 0 new on-disk paths.
• Measured: node --test on node v24.11.1 completed 1543 pass, 0 fail, 0 skipped.
• Measured: npx -y node@20 --test on node v20.20.2 completed 1537 pass, 0 fail, 6 skipped.
• Measured: npx -y node@20 --test test/proxy-forward-ca.test.mjs completed 32 pass, 0 fail, 4 skipped and exited cleanly; npx -y node@20 --test test/proxy-wrapper.test.mjs completed 37 pass, 0 fail, 2 skipped and exited cleanly. That directly addresses the prior non-termination on node 18/20.
• Read: the canCountCAs gate is tied to tls.getCACertificates availability at test/proxy-forward-ca.test.mjs#L105 and test/proxy-wrapper.test.mjs#L11. That matches the production tri-state in bin/ca-trust.mjs#L263, where carriesOurCA() returns null when the runtime cannot count but still returns false on the stderr-based refusal path.
• Read: the four newly skipped CA tests are the ones whose positive control requires carriesOurCA(...) === true on a healthy bundle: test/proxy-forward-ca.test.mjs#L1280, test/proxy-forward-ca.test.mjs#L1412, test/proxy-forward-ca.test.mjs#L1457, test/proxy-wrapper.test.mjs#L1206, and test/proxy-wrapper.test.mjs#L1250. On pre-22.15 runtimes those premises are unattainable by design, so skipping is honest rather than a hidden coverage drop.
• Read: the no-count path is still exercised below node 22 by the unskipped control at test/proxy-forward-ca.test.mjs#L1357, which proves a client-fatal bundle still yields false even when the census cannot answer, while a healthy bundle in the same setting yields null.
• Read: the inline t.skip() in the proxy-env test is narrow. It only fires when the control cannot be planted at all (test/proxy-forward-ca.test.mjs#L1135); when the control is plantable, the test still reaches the real assertion at test/proxy-forward-ca.test.mjs#L1142, so it does not become an escape hatch on node 22+.

Blockers

None.

What Needs Attention

None.

Bloat / Non-Functional

• Measured: proportionate. Delta since 4a32d142 is test-only (+37/-16 across 2 existing test files), with 0 production LOC changed.

Recommendations

• Measured: keep stating runtime beside local pass counts in future rounds. This round needed node v20.20.2 plus node v24.11.1 to show that the skips are capability-gated and that the node 18/20 hang is actually gone.

Bottom Line

Measured and code-read review at 5e6a2e04 is clean. The new skips are scoped to tests whose positive controls are impossible below node 22.15, the lower-runtime negative-path coverage remains in place, the proxy-env conditional skip only triggers when the control itself is unplantable, and the previously hanging node 18/20 behavior now exits cleanly in my own runs. Fork PR, so this clean round is uncommitted by policy. — Codex review