[abhisheksharma001]

Summary

Fixes #8561. scripts/verify-signal-recording.mjs acquired its private fixture root, then awaited /proc/self/limits outside any cleanup ownership, and its cleanup closed the root even when the MCP client or transport refused to close. A failed limits read leaked the root; a refused owner close removed the directory the surviving child could still be using.

Problem and expected behavior

Reported: the verifier allocates the root before reading /proc/self/limits outside the cleanup scope, and its cleanup attempts client, transport and root closes independently, so root close runs even when both MCP closes fail. Expected: enter cleanup ownership immediately after root acquisition, retain the root while acquired ownership is unresolved, preserve every failure, and establish owned process completion before release.

Change and scope

• scripts/verify-signal-recording.mjs: the inherited address-space limit is read before the root exists, so nothing acquired can leak if that read fails. The MCP client and transport are constructed inside the withVerifierCleanup operation, so no work is awaited between root acquisition and its cleanup ownership. Cleanup is one owned task: close the client and transport through closeVerifierResources (both are attempted, every refusal retained), observe the verifier's process lineage with completeVerifierRun and require it verified with assertVerifierCleanupComplete, then close the root. A refused owner close or unverified lineage leaves the root in place and the thrown error names the retained root path with the original failures as its cause. The final report reuses the lineage observed before release.
• Not included: proving the SDK stdio child's completion through the transport itself. fix(verifiers): retain the Go fixture root until MCP ownership completes #8650 introduces a transport that does this for the Go verifier; this verifier can adopt it once that lands. Lineage verification before release already retains the root when any owned descendant survives.

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; shared helpers (verifier-cleanup.mjs, verifier-run.mjs) 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-only verifier as before; the lineage check uses the existing /proc observer.

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

• Target selection, authority, upstream behavior, and cleanup details: the root is now retained on any unresolved ownership; its path is reported in the failure.

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

• Read existing files or captured data.

Evidence and regression coverage

• Tests added or updated: tests/boundary/process/signalVerifierCleanup.test.ts runs the real verifier as a subprocess under tests/fixtures/signalVerifierPreload.mjs, which authors the Linux guard, the limits read, the fixture root, the MCP owners and the lineage observation through module.registerHooks (the pattern of tests/fixtures/flutterVerifierPreload.mjs). Five cases: an unreadable limits file acquires no root; a connection failure releases the root only after client-close, transport-close and lineage-observe; a refused client close and a refused transport close each still attempt the other owner, keep the root and report the retained path; unverified lineage keeps the root.
• Base reproduction: with the base script, the refused-close and unverified-lineage scenarios record root-close after the owner closes (root released). With this change they stop at the owners or the lineage observation and the root directory remains.
• User-visible CLI/MCP output: none. On a retained root the verifier now exits with Signal recording verification failed; retained fixture root <path> because its cleanup is incomplete. whose cause carries the primary and cleanup failures.
• Remaining proof gaps: no real sigrok or Linux run; the successful-path report change (...verifier) is exercised only by the real lane.

Validation performed

• npm run test:focused -- tests/boundary/process/signalVerifierCleanup.test.ts — 5 passed
• npx oxfmt --check on the three files — clean; npx oxlint — only the pre-existing max-lines-per-function warning for the verifier's operation function (331 lines before, 338 now)

Compatibility, safety, and release

• Breaking changes or migration steps: none.
• Real Hopper/Ghidra, browser, or OS coverage: not run; the real signal lane (real-signal-recording.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 reviewed changes appear safe to merge; no actionable defect was established.

Summary

The verifier now reads inherited limits before creating its fixture directory. Cleanup attempts both MCP owner closes and checks process lineage before removing that directory.

• The signal verifier keeps its fixture root until cleanup is verified.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Read inherited limits] --> B[Create fixture root]
  B --> C[Run verifier operation]
  C --> D[Attempt both owner closes]
  D --> E{Both closes succeeded?}
  E -- No --> R[Retain root and report failures]
  E -- Yes --> F[Observe process lineage]
  F --> G{Verified with no descendants?}
  G -- No --> R
  G -- Yes --> H[Remove fixture root]

Loading

_{Reviews (1) · Last reviewed commit: "fix(verifiers): keep the signal fixture ..." · 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-03T14:30:27.293768Z	b40a3ba	Draft marked ready

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

[CassieFox23]

TL;DR: On Node 22.23.4, 24.22.1 and 26.12.1, all 5 tests pass at the PR head. With the old scripts/verify-signal-recording.mjs put back, all 5 fail on each version. The CI run on this head is completed/action_required, with no test jobs.

Check	Node 22.23.4	Node 24.22.1	Node 26.12.1
Head b40a3ba, tests/boundary/process/signalVerifierCleanup.test.ts	pass: 1/1 file, 5/5 tests	pass: 1/1 file, 5/5 tests	pass: 1/1 file, 5/5 tests
Red run: same head and test, old script from 9bea563	fail: 1/1 file, 5/5 tests	fail: 1/1 file, 5/5 tests	fail: 1/1 file, 5/5 tests

• The red run fails the same five tests on all three Node versions.
• The names, from the Node 24.22.1 log, are:
  • acquires no fixture root before the inherited limits are readable
  • releases the root only after its owners closed and lineage was verified
  • retains the root and the other owner's close when the client close is refused
  • retains the root and the other owner's close when the transport close is refused
  • retains the root when owned process lineage cannot be verified
• The PR changes 3 files against the merge base: scripts/verify-signal-recording.mjs, tests/boundary/process/signalVerifierCleanup.test.ts and tests/fixtures/signalVerifierPreload.mjs.
• The head is 373 commits behind main 9d0e87f.
• GitHub shows mergeable true and state blocked.
• Workflow runs on the head:
  • CI: completed/action_required
  • CodeQL production: completed/action_required
  • PR title (2 runs): completed/action_required
  • PR readiness (3 runs): completed/success
• Check runs: Greptile Review and three Validate author declaration runs are success (4 in all).
• The commit status PR readiness is success.
• No CI test jobs (Source tests, Portable tests, Test shard) appear in the check runs of the CI run.

Not run

• A real root class target that exercises the success path, since the mocked Client.connect always throws
• 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: b40a3ba / 9bea563 (merge base with main)
• Red run: b40a3ba with the changed source file 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/signalVerifierCleanup.test.ts

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