[kaoru0822-kitauji]

What broke: verify-go-binary.mjs removed its private root even when the MCP client or transport close failed, or while a child it had just sent SIGKILL was still unreaped, and a constructor failure skipped cleanup and leaked the root.
The fix: A new transport adopts the SDK's child into ProviderProcessSupervisor and proves exit and output closure. On POSIX the root goes only after every owner succeeds and lineage positively shows no live descendant; otherwise it is retained, reported and retryable. Windows has no lineage facility here, so it releases on the child's proof alone and the report says so.
The risk: Limited to this verifier, plus one new helper with no other consumer yet. The helper reads the SDK's private _process field, with an IDA precedent, and fails closed if a later SDK removes it. A lineage failure on POSIX now fails the run and keeps the root, where it used to pass. Real Go did not run locally; the real-go-binary lane passed on this head with lineage verified.

Summary

Fixes #8555.

The verifier treated "the close call returned" as "the owner finished". A failed close, and a successful close with a live child, both led straight to root removal. It now holds the root until completion is proven and says so when it cannot.

Problem and expected behavior

Smallest triggers, all reproduced on latest main (f95bdbce7) with the actual verifier:

• new Client or new StdioClientTransport throws: no cleanup runs and the root stays on disk with no report.
• Both closes reject: the root is removed while the MCP child is still running. The report's lineage listed that child as a live descendant.
• The child ignores stdin EOF and SIGTERM: close() resolves right after its final SIGKILL and the recorded order is root-close, child-exit, child-close.

The violated invariant is that a root must outlive anything that can still use it. As the issue says, waiting for a successful SDK close does not establish that.

Change and scope

• scripts/lib/verifier-stdio-transport.mjs adds VerifierStdioClientTransport, a subclass of the SDK transport. closeAndVerifyChild() runs the SDK close, then stops any survivor through the shared supervisor and requires the child's exit and its close event. A rejection keeps the child owned and a later call retries it.
• SDK close() deliberately keeps its contract and does not reject. After a failed handshake the SDK calls it without awaiting, so a rejecting override would become an unhandled rejection and end the process before the root is reported.
• Exit alone is not accepted. A reparented process holding the child's pipes is invisible to the lineage walk, so open pipes keep the root. A scenario covers exactly that case.
• verify-go-binary.mjs constructs its owners inside withVerifierCleanup and gains one fixture owner with this order: client and transport, lineage check, root removal. Lineage used to be observed after the root was gone.
• On POSIX, lineage must be positive. The existing assertVerifierCleanupComplete replaces the old "assert only if verified" condition, so any unavailable result keeps the root and a later close observes again. This is what makes the root rule hold: unknown no longer counts as absent.
• On Windows, completeVerifierRun always returns unavailable because the verifier has no Job Object lease, so requiring it would retain every root. The root is released on the child's completion alone, and omitted_coverage says other descendants were not observed.
• Any owner failure retains the root, even when the child's exit was proven. The failure report gains retained_root, and the thrown GoVerifierCleanupFailure carries retainedRoot and cleanupOwner for retry. Failures stay one flat list with the primary first.
• real-go-binary.yml now also triggers on the two scripts/lib files the verifier depends on for this.

Not included: #8561, #8562 and #8564. A verified lineage only walks live parent chains, so a reparented descendant holding no pipes still reads as absent rather than unknown. That limit of completeVerifierRun is unchanged, and retaining on unavailable does not close it. The SDK child is not placed in its own process group, so there is no group kill of descendants. Moving to spawnOwnedProviderProcess would add that, at the cost of replacing the SDK's stdio client in a verifier meant to exercise it.

Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.

Contract and boundary impact

• Semantic owner and earliest changed stage: the new transport owns child completion, and the verifier's fixture owns release ordering. The unconditional cleanup loop is removed.
• Contribution scope fit and linked maintainer decision: not applicable.
• CLI and MCP/tool-catalog contract: none. The product is unchanged.
• Provider, bridge, target-format, or platform compatibility: verifier only. POSIX executed here; native Windows not run.
• Evidence, artifact, provenance, or reconstruction contract: none.
• Target selection, authority, upstream behavior, and cleanup details: signals go only to the verifier's own MCP child, through its retained handle. A retained root is left on disk and named in the report.
• Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

• Read existing files or captured data.
• Launch or attach to a process.
• Inject instrumentation or run scripts/code.
• Change process memory, registers, files, or configuration.
• Contact network services.
• Recover protected code or work around protections.
• Documentation or usability only; no target interaction.
• Not sure yet; investigation or maintainer guidance needed.

The launched processes are the verifier's own MCP child and compiler, as before.

Evidence and regression coverage

• Tests added or updated: tests/boundary/process/goVerifierCleanup.test.ts, 20 scenarios that import the actual verifier in a subprocess with a fake compiler. Faults are authored; the SDK client and transport are real except in the three scenarios that replace an SDK module. Scenarios that reach a child use real rea mcp once, as a control, and a disposable child otherwise.
• Coverage against the issue's list: late exit after the final kill (SIGKILL held 400 ms past SDK close()), a kill that never lands, constructor failures, failed spawn, refused handshake, each mix of client, transport and root close failures, retained-root reporting, and retry after each retained case.
• Also covered: open pipes held by a reparented process, another live descendant, unread stderr, and an SDK that exposes no child handle.
• Lineage unknown on POSIX, each retained then retried: a failed snapshot and an unreadable descendant environment, injected at the real observer's reads for both its Linux and ps branches, and a descendant with a foreign run token, which needs no injection.
• Windows decision: the real lineage branch and the verifier's decision run under a process.platform flip around that one window. Everything else stays POSIX.
• Base reproduction (executed, macOS arm64, Node 24.22.1): against the base verifier, 17 of the 20 scenarios fail. The three controls pass, as expected: real-server, missing-compiler and spawn-failure.
• Review finding reproduced (executed): with the verifier at cdb875257, a failed snapshot, an unreadable environment and a foreign token each reported unavailable, removed the root and raised no cleanup failure. Those three and the Windows scenario fail there; live-descendant also fails there, only because its expected message was tightened.
• Harness cleanup (executed): with an assertion forced to fail in pipe-holder and undelivered-kill, the earlier harness left one process running in each and this one leaves none. The base and cdb875257 test runs above also left none.
• Mutation checks (executed): removing the stderr drain, the pipe check or the completion proof each failed the matching scenario. Removing the completion proof first passed late-exit, because the lineage ps happened to give the child time to die, so I held the kill back to make that scenario strict. Requiring lineage on Windows too fails the Windows scenario.
• User-visible output: the failure report gains one field, and the message names the root:

"retained_root": "/…/rea-go-verifier-8x5P0a",
GoVerifierCleanupFailure: Real Go metadata verification failed; retained fixture root /…/rea-go-verifier-8x5P0a because its cleanup is incomplete.

• Remaining proof gaps: no real Go compiler ran locally; the success path with real Go is proven only by the real-go-binary lane on Linux. Native Windows was not run and the test is skipped there, so the Windows release path has only the simulated run above. The other two unavailable reasons, a changed parent and an absent launcher, were not run separately; they take the same check. Frequency of the late-exit window in real runs stays NOT_ENOUGH_INFO.

Lifecycle regressions from docs/testing.md: mixed child outcomes and cleanup retry are covered. A refused handshake is covered; a child that never answers the handshake is not, because it would wait out the SDK's 60-second default. Cancellation during final cleanup does not apply, as this verifier has no cancellation path.

For evidence-bearing changes: not applicable, no Evidence contract changed.

• Observed, derived, and inferred claims remain distinguishable.
• Artifact identity, source provenance, and failed attempts remain preserved.
• Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

• After merging main: npx vitest run on goVerifierCleanup, verifierCleanup, verifierRun, goBinaryMcp and ciChangeScope — 397 passed in 5 files.
• After merging main: goVerifierCleanup alone, 3 consecutive runs — 20 of 20 each, about 27 seconds each.
• real-go-binary on this head (CI, Linux, Go 1.26.1) — passed, status: passed, 15 binaries, 30 public cases, lineage verified with no descendants.
• npm run check:fast — passed.
• npm run format:check — passed.
• Before the lineage change: node scripts/verify-go-binary.mjs with no Go installed — exit 1, primary "requires Go" error kept, retained_root: null, no root left on disk.
• Before the lineage change: the same entrypoint with a fake compiler, real rea mcp and a refused client close — exit 1, primary and cleanup failures both printed, root retained and named.
• node scripts/ci/plan.mjs for this diff — selects code, runtime and workflows, so the new test runs in the built process-boundary project.

Docs and catalog are unchanged, so docs:check and metadata:check were not run. Analysis cost is unaffected. The new test file adds about 27 seconds to process-boundary, mostly the SDK's two fixed 2-second close grace periods.

Compatibility, safety, and release

• Breaking changes or migration steps: none.
• Real Hopper/Ghidra, browser, or OS coverage: none run. Real Go ran only in the real-go-binary CI lane, on Linux. This change could have broken its success path, since unavailable now fails the run; that lane reported lineage verified, so it did not.
• Package or release metadata impact: none.
• Security, privacy, process, or containment review: a retained root stays private and on disk until someone removes or retries it. A verifier run that ends with a retained child leaves that child running, because the proof failed and nothing further can be done from an exiting process.

Author readiness

Human owner: @kaoru0822-kitauji

• I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
• I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
• I declare this PR ready for initial maintainer review.

Declarations entered by Claude Code on the owner's standing written instruction.

Before merge

• This PR addresses a concrete problem or an agreed enhancement.
• Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
• The PR has one focused outcome and the title follows type(scope): outcome.
• Related issue is linked, or the reason for not linking one is stated above.
• Tests cover changed observable behavior and meaningful failure paths.
• Owning docs, contracts, and generated metadata are updated where needed.
• User-visible CLI/MCP changes include representative output.
• I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T10:04:19.550183Z	61aea43	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[kaoru0822-kitauji]

CodeQL #684 (js/unvalidated-dynamic-method-call, tests/fixtures/goVerifierCleanupHarness.mjs:233) is a false positive; I am leaving it open, not dismissed.

Taint path: process.argv[2] → Map.get → setUpScenario(). That argument is a scenario name written as a literal in goVerifierCleanup.test.ts. The Map holds only the harness's declared handlers, and any other name fails the assert.ok before the call. Executed: nope, __proto__ and constructor each exit 1 with Unknown scenario. The rule's concern is an unhandled TypeError from an unexpected target, and this test-only process has no such path.

It appears because cdb875257 replaced #677 with this lookup. cdb875257 also cleared the path alerts, and #677–#683 now report fixed. I did not push again to satisfy this rule, because it would restart CI with no behavior change.

[greptile-apps]

Confidence Score: 5/5

[Low impact] The PR appears safe to merge; no new actionable defect was established.

Summary

The Go verifier now keeps its private fixture root until its owners finish cleanup.

• The Go verifier keeps its fixture root until cleanup is proven.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Verification finishes or fails] --> B[Close client and transport]
  B --> C{Child exit and pipes closed?}
  C -->|No| R[Retain root and report retry owner]
  C -->|Yes| D{Platform}
  D -->|POSIX| E{Lineage verified and empty?}
  E -->|No| R
  E -->|Yes| F[Remove fixture root]
  D -->|Windows| G[Report missing descendant coverage]
  G --> F

Loading

_{Reviews (4) · Last reviewed commit: "Merge origin/main into fix/8555-go-verif..." · Reviewed by Greptile}

[yash27git]

This is a false positive. The dynamic invocation is restricted to a statically declared scenario allowlist.

scenarios contains only locally defined handlers. The harness creates a Map from Object.entries(scenarios), looks up the requested scenario, and validates the result with assert.ok(setUpScenario) before invoking it.

Because Map does not expose inherited object properties, inputs such as __proto__ and constructor cannot resolve to prototype methods. Unknown scenario names fail before invocation.

This behavior was also tested explicitly: nope, __proto__, and constructor each exit with Unknown scenario.

There is no arbitrary method dispatch or attacker-controlled function execution here. I recommend keeping this finding classified as a false positive rather than changing correct test-harness logic solely to satisfy the static-analysis rule.

[CassieFox23]

TL;DR: I ran the one changed test file on Node 22.23.4, 24.22.1 and 26.12.1. All 20 tests passed on each version. On head 61aea43, the CodeQL check run is failure and the mergeable state is blocked.

Check	Node 22.23.4	Node 24.22.1	Node 26.12.1
Test Files	pass, 1 of 1	pass, 1 of 1	pass, 1 of 1
Tests	pass, 20 of 20	pass, 20 of 20	pass, 20 of 20

The file was tests/boundary/process/goVerifierCleanup.test.ts. The PR changes 6 files against the merge base 93b8b82: 1 test file and 5 others. The head is 385 commits behind main 9d0e87f.

CI on the head, read with gh: mergeable is true and state is blocked. The PR is not a draft.

• Workflow runs CI, PR #8650, PR readiness, PR title and Real Go binary metadata are completed/success.
• Workflow run CodeQL production is completed/skipped.
• Check runs: 23 success, 25 skipped, 1 failure.
• The one failed check run is CodeQL.
• Among the successes are Source tests, real-go-binary and CI required.
• The commit status PR readiness is success.

Not run

• A run of npx vitest run tests/boundary/process/goVerifierCleanup.test.ts about 10 times on Linux, once under CPU load
• A merge with current main, 9d0e87f

Test setup

• Box: Fedora 44 VM (4 vCPUs, 8 GB) on my Nobara 44 PC, Linux 7.2.11
• Node: 22.23.4, 24.22.1, 26.12.1
• Head / base: 61aea43 / 93b8b82 (merge base with main)
• Install: npm ci --offline and npm run test:prepare on Node 24.22.1

Commands

npm ci --offline
npm run test:prepare
xvfb-run -a npm run test:focused -- tests/boundary/process/goVerifierCleanup.test.ts

🤖 Written and tested with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊