fix(runner): restore streamed stderr callbacks

[frankekn]

Restore the optional onStderr callback removed in #115. Consumers importing the packaged managed runner can otherwise lose progress notifications; a child waiting for an acknowledgment triggered by stderr will time out because the callback never runs.

Reuse the original guarded dispatch and handler-error path. A real subprocess regression requires streamed stderr to trigger a stdin acknowledgment before the child can exit, and verifies that aggregate stderr remains available. Existing CLI/ACP callers do not supply this hook; no affected production consumer has been identified.

Validation: the regression fails with dispatch absent and passes after restoration; focused runner/ACP tests, pnpm check, pnpm lint, full pnpm test, pnpm build, git diff --check, and structured autoreview passed.

gateClass: D: compatibility restoration with no changes to production model inputs, candidate handling or review verdicts. Live model eval is skipped under the owner's prior instruction for this cleanup work, not recorded as passing. No merge or deployment performed.

[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-09-08T07:07:51.724187Z	b7a713f	PR opened

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

[Copilot]

🟢 Approval recommended

The change is a compatibility restoration with consistent error-handling semantics and a targeted regression test that would fail without the restored callback.

Pull request overview

This PR restores the optional onStderr streamed callback in runManagedRunnerProcess (removed in #115) to preserve compatibility for consumers that rely on stderr-driven progress/handshake signaling, and adds a regression test to ensure stderr can be observed and acted on before the child exits.

Changes:

• Reintroduces onStderr?: RunnerChunkHandler on ManagedRunnerProcessInvocation.
• Invokes invocation.onStderr(text, controller) for streamed stderr chunks with the same guarded dispatch + handler-failure path used for stdout.
• Adds a focused regression test that requires stderr delivery to trigger a stdin acknowledgment before the subprocess can finish.

File summaries

File	Description
src/shared/runner-process.ts	Restores the onStderr hook and dispatches it safely during stderr streaming.
src/shared/runner-process.test.ts	Adds a regression test proving stderr can be consumed in time to drive a stdin handshake and still preserves aggregated stderr.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7a713fcc1

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

Run the mandatory Class D gate

This restores observable managed-runner behavior, so it is a delivery/reliability pipeline change, but the commit contains only the implementation and a unit test; a repo-wide search finds no Class D report or RESULTS entry for this candidate, and the commit message explicitly says the live eval was skipped. Run and record the required property suite, x3 drift/honeypot gate, and canary before shipping.

AGENTS.md reference: AGENTS.md:L81-L83

Useful? React with 👍 / 👎.

[github-actions]

LGTM ✅ — PASS: onStderr restore matches onStdout dispatch; production callers omit the hook so behavior is unchanged; handshake regression is covered.

Coverage: full diff reviewed in one pass (2 files)

Review target: PR #122 25c3eee..b7a713f

Findings

No actionable findings. Prefer this over padding weak ones.

Checked (8)

• Surface: src/shared/runner-process.ts is the runner-process seam; this diff only restores optional public onStderr on ManagedRunnerProcessInvocation and the stderr data dispatch. No sandbox, env allowlist, clone, or token-stripping change.
• Precondition substitution: onStderr default undefined → invocation.onStderr === undefined @src/shared/runner-process.ts:286 → skip callback, still collect stderr and refresh idle → passes (optional-absent path). Live in-repo callers omit the field: spawnRunnerProcess @src/shared/runner-process.ts:77 (used from src/shared/codex.ts:1020,1065,1100,1134,1189) and runAcp @src/shared/acp.ts:54-63. No other 1-layer reader (rg onStderr / runManagedRunnerProcess).
• TRIGGER A not fired: the new text === null || invocation.onStderr === undefined check is optional-hook dispatch, not an approval/submit/route/proceed gate. Supplying onStderr enables the callback; omitting it does not reject a previously allowed user action.
• TRIGGER B not fired: no new loop, recursion, or repeated await. Callback is synchronous inside the existing stderr 'data' listener.
• TRIGGER_C cleared carrier=ManagedRunnerProcessInvocation.onStderr @src/shared/runner-process.ts:46 promises=optional per-chunk stderr string + controller body=after successful collect, onStderr(text, controller) @src/shared/runner-process.ts:286-288 callers=[src/shared/acp.ts:54 relies=no; src/shared/runner-process.ts:77 relies=no; src/shared/runner-process.test.ts:243 relies=yes streaming-before-exit]
• TRIGGER_D cleared swallow=@src/shared/runner-process.ts:289-291 signal=onStderr throw previously_reached=failFromHandler @src/shared/runner-process.ts:193-198 (same as onStdout; hook absent on base so no prior consumer path) now_receives=spawnError + SIGKILL then result.error @186-189 callers=[exported runManagedRunnerProcess distinguishable=yes via result.error; acp.ts:54/spawnRunnerProcess@77 do not pass onStderr]
• Restore is byte-identical to onStdout @273-281 and to the hook removed in 9c63d08 (original 89b96a5). Not a second in-repo stderr API.
• Test runManagedRunnerProcess delivers stderr before exit so a consumer can acknowledge readiness @src/shared/runner-process.test.ts:225-258 requires streamed stderr to unlock stdin before exit; a close-time-only callback would ETIMEDOUT/exit 2.

Residual risk (1)

• PR declares gateClass D and skips live eval; no eval/RESULTS entry for b7a713f. In-repo production callers do not pass onStderr, so review outputs are unchanged. Process gap, not an unverified live-path sentinel; does not block the code verdict. Validation: pnpm exec node --test --test-concurrency=1 --import tsx src/shared/runner-process.test.ts

_{2 calls · review 4m 38s → critic 1m 8s · total 5m 46s}

[github-actions]

Needlefish re-review @ b7a713f — ✅ 0 resolved · ❌ 0 still open · 🆕 0 new → LGTM

[frankekn]

Verified the premise against the published artifact rather than in-repo callers, because "no in-repo consumer" is the wrong test for a package that ships dist without an exports gate.

onStderr was a published API. npm pack needlefish@0.4.1 (the version currently on npm, 2026-08-01) contains it in dist/shared/runner-process.js:

172:  if (text === null || invocation.onStderr === undefined)
175:      invocation.onStderr(text, controller);

Timeline: introduced 89b96a5 (2026-07-05, ACP adapter), shipped in v0.3.1 → v0.4.1, removed by 9c63d08 (2026-09-06, "remove unused runner state"). That removal is contained in v0.4.3, v0.4.4, v0.4.5. npm still serves 0.4.1, so the break has not reached consumers yet — merging this before the next publish is what keeps it from ever shipping. package.json declares no exports, so deep imports of needlefish/dist/... are resolvable by design-default.

Re: the mandatory Class D gate (P1 above). The gate's own criterion is provenance containment, and it holds by inspection here: no caller in src/ or eval/ passes onStderr (only acp.ts:61 passes onStdout), so on every in-repo path invocation.onStderr === undefined and the handler returns before doing anything — successful-path output is byte-identical. The added dispatch is a byte-exact mirror of the existing onStdout handler, including failFromHandler (sets spawnError, then SIGKILL), so a throwing handler still fails closed. collect() returns null only when a buffer error already exists or when this chunk exceeds RUNNER_MAX_BUFFER_BYTES — and that path has already begun a SIGKILL, so skipping the callback there cannot strand a consumer waiting on a readiness marker.

Re-verified on today's main (6efd0c9), not the 2026-09-08 base this PR was opened against: merges clean, pnpm check and pnpm lint clean, full suite 962/962 (961 baseline + this PR's regression test). main has not touched runner-process.ts since this PR was opened, so it is not stale.

Merging on that basis. The live model eval remains skipped per the owner's standing instruction for this cleanup work, as the PR body already records.