[abhisheksharma001]

Summary

Fixes #8562. scripts/verify-recorded-crash.mjs closed its private fixture root after session and sentinel closes even when those closes failed, and observed its process lineage only after the root was gone. Separately, connectRecordedCrash acquired the SDK transport before the verifier owned the session, so a failed connection whose transport close was also refused lost the only handle to a possibly live transport, and the verifier could still release the root.

Problem and expected behavior

Reported: cleanup attempts root close regardless of session and sentinel close outcomes; lineage is observed after root close; a failed connection whose transport close also rejects reports only the close error with an empty cause. Expected: retain the root while session or sentinel ownership is unresolved, establish owned process completion before release, and preserve both the connection and the cleanup failure.

Change and scope

• scripts/verify-recorded-crash.mjs: cleanup is one owned sequence. Session and sentinel closes run through closeVerifierResources, so every owner is attempted and every refusal is retained. Process lineage is then observed and required verified with assertVerifierCleanupComplete; only after that is the root closed. Any failure in that sequence keeps the root, adds the individual failures to the report, and the thrown AggregateError names the retained root path. The report gains retained_root (null once released). If the sequence stopped before observing lineage, the report still observes it afterwards, without asserting on it.
• scripts/verify/recorded-crash/public.mjs: connectRecordedCrash becomes openRecordedCrashSession, a factory whose connect() is a separate step. The transport is acquired by the factory and released only by close(), so verify-recorded-crash.mjs and trace-client.mjs register each session as an owner before connecting it: a failed connection, or a refused close after one, is closed by the owned cleanup like any other session, and a refused close retains the root instead of being reported as diagnostics only. The trace client runs its connect, inspection and close through withVerifierCleanup, so a refused close is retained beside the primary failure rather than replacing it.
• Not included: proving the SDK stdio child's completion through the transport (what fix(verifiers): retain the Go fixture root until MCP ownership completes #8650 adds for the Go verifier) and a cleanup retry entry point; the retained root and its named path are what a retry would start from.

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

Contract and boundary impact

• Semantic owner and earliest changed stage: the verifier's own ownership order and its session helper; shared helpers are unchanged.

• Contribution scope fit and linked maintainer decision: not applicable.

• CLI and MCP/tool-catalog contract: none.

• Provider, bridge, target-format, or platform compatibility: Linux x64 verifier as before.

• Evidence, artifact, provenance, or reconstruction contract: none.

• Target selection, authority, upstream behavior, and cleanup details: the root is retained on any unresolved ownership or unverified lineage; the report and the failure name its path.

• Generated metadata, package, or installation impact: none.

• Read existing files or captured data.

Evidence and regression coverage

• Tests added or updated: tests/boundary/process/recordedCrashVerifierCleanup.test.ts runs the real verifier as a subprocess under tests/fixtures/recordedCrashVerifierPreload.mjs, which authors the Linux x64 guard, the pid_max read, the fixture root, the fixture tooling, the sentinel, the lineage observation and, for the real public.mjs, the SDK owners and the CLI (the pattern of tests/fixtures/flutterVerifierPreload.mjs). Eight cases: a prerequisite failure that acquired no owner releases the root after lineage; a CLI failure releases the root only after client-close, transport-close, sentinel-close and lineage-observe; a refused client, transport or sentinel close still attempts the other owners, keeps the root, reports retained_root and names the path; unverified lineage keeps the root; a failed connection still closes its owned session and releases the root after lineage; a failed connection whose transport close is refused keeps the root and reports both failures.
• Base reproduction: with the base scripts, the refused-close and unverified-lineage scenarios record root-close before lineage-observe (root released), and the connection scenarios close the transport outside any owner and release the root. With this change the refused scenarios, including the refused close after a failed connection, stop before root-close with the directory present; with the session registered only after connect(), exactly the two connection cases fail.
• User-visible CLI/MCP output: none. The verifier report gains "retained_root": null | "<path>"; on a retained root the final error reads Recorded crash verification failed; retained fixture root <path> because its cleanup is incomplete.
• Remaining proof gaps: no real GDB, pwndbg, strace or Linux run; the success path is exercised only by the real lane.

Validation performed

• npm run test:focused -- tests/boundary/process/recordedCrashVerifierCleanup.test.ts — 8 passed
• npx oxfmt --check on the five files — clean; npx oxlint — only the pre-existing max-lines-per-function warning on the session factory

Compatibility, safety, and release

• Breaking changes or migration steps: none; the report gains one field.
• Real Hopper/Ghidra, browser, or OS coverage: not run; the real recorded-crash lane (real-recorded-crash.yml) exercises the success path.
• Package or release metadata impact: none.
• Security, privacy, process, or containment review: cleanup now fails closed; no directory is removed while an owner's close or lineage is unresolved.

Author readiness

Human owner: @abhisheksharma001

• 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.

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

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the previous error-loss finding is fixed and no new blocking issue was found.

Summary

The PR keeps the recorded-crash fixture root until its owners close and process lineage is verified.

• The verifier keeps its fixture directory until owned cleanup is complete.
• Recorded-crash sessions stay owned even when connecting fails.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Create fixture root] --> B[Register session before connecting]
  B --> C[Run verification]
  C --> D[Attempt every session and sentinel close]
  D --> E{All closes succeed?}
  E -->|No| R[Retain root and report its path]
  E -->|Yes| F[Observe process lineage]
  F --> G{Cleanup verified?}
  G -->|No| R
  G -->|Yes| H[Remove root]

Loading

_{Reviews (3) · Last reviewed commit: "fix(verifiers): keep the trace client's ..." · Reviewed by Greptile}

[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-03T16:38:59.728538Z	5819e45	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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e66a36624

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5819e45f35

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[morluto]

Hey @abhisheksharma001, thanks for the PR! This fix looks good to me. What's left: there are 2 unresolved review threads from Codex (1 still on current code), e.g. "Preserve the trace client's connection failure"; the one on an outdated line may just need resolving if it's already handled. Happy to merge once that's sorted.

[abhisheksharma001]

The merge conflicts with main are resolved and every review thread has been addressed. The CI, CodeQL production and PR title workflows are waiting for approval to run on this fork PR (action_required); could you approve them when you have a moment?

[CassieFox23]

TL;DR: I ran the PR's changed test file on Node 22.23.4, 24.22.1 and 26.12.1. All 8 tests pass on the head and fail on the old source files. The CI run on this head is action_required, with no CI test jobs in it.

Check	Node 22.23.4	Node 24.22.1	Node 26.12.1
Head 13bc110	pass: 1 of 1 files, 8 of 8 tests	pass: 1 of 1 files, 8 of 8 tests	pass: 1 of 1 files, 8 of 8 tests
Red run (3 source files from 9bea563)	fail: 1 of 1 files, 8 of 8 tests	fail: 1 of 1 files, 8 of 8 tests	fail: 1 of 1 files, 8 of 8 tests

The test file is tests/boundary/process/recordedCrashVerifierCleanup.test.ts. In the red run I swapped back scripts/verify-recorded-crash.mjs, scripts/verify/recorded-crash/public.mjs and scripts/verify/recorded-crash/trace-client.mjs. The same 8 tests fail on all three Node versions.

On Node 24.22.1, all 8 failures are one ZodError at tests/boundary/process/recordedCrashVerifierCleanup.test.ts:66. The error says retained_root is undefined where a string is expected.

The PR changes 5 files against the merge base 9bea563. The head is 373 commits behind main 9d0e87f.

CI on the head, read live with gh:

• Workflow runs: CI is completed/action_required. CodeQL production is completed/action_required. PR title (2 runs) is completed/action_required. PR readiness (2 runs) is completed/success.
• Check runs: Greptile Review and Validate author declaration (2 runs) are success.
• Commit status: PR readiness is success.
• No Source tests, Portable tests or Test shard jobs appear in the check runs of the CI run.
• GitHub shows mergeable true and state blocked.

Not run

• A probe uses the real @modelcontextprotocol/client stdio transport. It points openRecordedCrashSession at a nonexistent entrypoint. Then it calls connect() and close(). It checks whether close() throws.
• A run of the success path, where the root is closed and retained_root is null.
• 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: 13bc110 / 9bea563 (merge base with main)
• Red run: 13bc110 with the changed source files from 9bea563
• 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/recordedCrashVerifierCleanup.test.ts

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

[abhisheksharma001]

@morluto following up on my 11-06 comment. The review threads from your 11-05 note are resolved and the PR shows as mergeable.

CI, CodeQL production and PR title on head 13bc110 are still action_required, so no test jobs have run. Could you approve them when you have a moment?

CassieFox23 also ran the changed test file on Node 22, 24 and 26: all 8 tests pass on this head and fail on the old source.

[abhisheksharma001]

@morluto a gentle second ping. Same blocker as above: CI, CodeQL production and PR title on 13bc110a2 are action_required, and nothing else is open on my side. If a different approach or a split would suit you better, tell me and I will do that.