Extract provider process supervision and stream reading from Codex

[brennanb2025]

	Files	Added	Deleted	Net
Test	12	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​111	$\color{#cf222e}{\Huge{\mathbf{−}}}$​33	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​78
Prod	17	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​90	$\color{#cf222e}{\Huge{\mathbf{−}}}$​76	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​14

ELI5

Process supervision and stream reading for structured chat lived in the Codex folder even though Claude already shared the supervisor. Another provider would have to import Codex modules or copy them. This extraction has no user-visible change: existing Claude and Codex chats start, stream, and stop as before.

What Changed

• Move the supervisor, process-tree teardown, stream reader, and exit-deadline function into src/main/provider-process/. Keep the existing Codex launch type as an alias for compatibility.
• Update importers, the command-line build include list, and continuous-integration path and import checks. Keep line framing in the existing shared parser and remove the unused Codex parser.
• Give the child-process environment one rule in one function, resolveProviderChildEnv: start from the inherited environment, apply the launch's overrides, then remove the keys the launch asks to delete. Both the shared spawn function and Codex's short-lived request session (used for trust grants and session index repair) call it; the session's own copy of the rule is gone. Claude continues to supply its complete environment. Caller-supplied environments are copied, not changed.
• The lower-level supervisor wrapper now only accepts a command whose environment is already resolved. Passing a launch that still carries environment overrides is a type error, so an override can no longer be silently ignored there.
• Require a teardown diagnostic label from the caller. Codex passes its existing label; another provider can identify its own teardown correctly in logs.
• The real-binary Codex contract job in continuous integration now also runs when the shared stream reader or the shared environment rule changes, since Codex's index repair depends on both.

The inline supervisor script, process signals, grace periods, and stream pause/resume behavior are unchanged. Codex protocol handling and its per-request timeout remain in the Codex connection and session modules.

Why

Moving the working functions makes them reusable without a dependency on the Codex protocol. Keeping the environment rule in exactly one function means a new caller cannot lose overrides or keep a key it asked to remove, and a later change to the rule (for example, Windows' case-insensitive variable names) reaches every launch at once instead of missing a copy. A full lifecycle unification is a separate follow-up, #25204, because it must build on the child-close changes already in flight.

Linked Issue

Internal foundation work for structured chat through Agent Client Protocol; no separate issue.

Visual Proof

N/A — no UI or interaction change.

Testing

• I manually tested these changes locally
• Automated tests added/updated, or explained why not below

New tests pin the single environment rule directly (override, then delete, without changing the supplied objects) and through the spawn specification on Windows, macOS, and Linux. A type-level test fails to compile if the supervisor wrapper ever accepts a launch with environment overrides again. Teardown tests verify a non-Codex label reaches the Windows teardown and POSIX kill log, while retaining coverage for Codex's existing label.

What I verified / didn't

• The branch was brought up to date by merging current main (no conflicts). The lockfile is unchanged.
• At 987fb5307f2 on macOS, 165 tests passed across 17 explicit files: provider environment rule, provider supervisor unit and integration, provider teardown, Claude process spawn and exit-proof ladder and supervised-stop integration, Codex connection, session, model-catalog probe, account index repair, and teardown integration, the process-tree kill gate, structured session host teardown and recovery resolution, self-initiated tree-kill log, and continuous-integration path classification.
• In review before this change, a side-by-side test (run by the reviewer, outside the repo) of main's spawn specification (plus Codex's old caller-side environment step) against this branch's produced identical results for Codex and Claude on Windows, macOS, and Linux. The environment change here moves that same rule into one function without changing it.
• Local typechecks of the main-process and command-line projects reported no errors in files this PR touches. Their only errors are missing stream-json/stream-chain modules in unrelated files, because the local dependency install predates main adding those packages. CI typecheck on 987fb5307f2 is the authority and was still running when this was written.
• Changed-file formatting and lint and git diff --check passed.
• No Orca, Electron, runtime server, or real agent was launched. Windows, Linux, and SSH execution were not tested locally; platform-parameter tests do not establish behavior on those hosts.
• The unrelated cross-version qoder-history-search-downgrade failure is excluded from acceptance; this PR does not change that test or its request parser.

Review

Differences from the common pattern

• INTENDED: Orca retains a detached POSIX supervisor per child. It detects loss of its owning runtime and removes the provider's process group before reporting exit. This existing mechanism is preserved by the extraction.
• INTENDED: The environment is composed (inherited, plus launch overrides, minus deletions) by one shared function at spawn time. In the common pattern the runtime layer composes the final environment and the process layer receives it as-is. This is safe because Orca's launches already carry override-and-delete fields, and one shared function means no caller can skip the step.
• TEMPORARY: The shared lifecycle—spawning the child, watching its exit, and closing only after proving exit—still lives in the Codex connection, while Claude keeps its own copy. Follow-up Share the managed process lifecycle for structured providers #25204 unifies the provider process lifecycle (building on the child-close work in fix(native-chat): the agent's exit ends its record, and an unconfirmed stop is joined instead of held #24862) before the Agent Client Protocol adapter adopts it; this PR only moves the existing functions and fixes their neutral interfaces.
• TEMPORARY: Windows retains existing process-tree teardown. A planned follow-up will bind Windows agent processes to Orca's lifetime so they are stopped when their owner exits.

The process ownership mechanisms remain different from the common pattern; the move does not claim to unify them.

Agent skill upstream boundary

• Not applicable; this change copies no upstream skill-installer implementation or supporting files.

Notes

Execution stays on the spawning runtime. No client wire format, persisted data, or workspace assumptions change. Folder workspaces retain the same launch-directory behavior.

Checklist

• This PR is small and focused
• I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
• Before/after screenshots or videos attached for UI changes, or N/A with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
• CI typecheck passes (running on 987fb5307f2 at time of writing); affected local tests pass; packaging remains covered by CI

[brennanb2025]

Review summary (head 987fb5307f2)

The problem. Orca's structured chat runs agent command-line tools as child processes. The code that supervises those processes, cleans up their process trees and reads their line-by-line JSON output lived in the Codex folder, even though Claude already used part of it. The next providers (Agent Client Protocol agents such as Grok) would have had to import Codex modules or copy them.

What changes for users. Nothing visible. Claude and Codex chats start, stream and stop exactly as before. This is groundwork that lets the next provider reuse the same process handling.

What the review rounds fixed

• The rule that applies a launch's environment overrides and removed keys now lives in one function (resolveProviderChildEnv). Before, it existed in two copies, and a lower-level exported function accepted a launch with environment fields it silently ignored. That function's type now rejects such a launch at compile time.
• The teardown diagnostic label is supplied by the caller, so another provider's cleanup is no longer logged as Codex.
• The CI job that runs the real Codex binary contract now also runs when the shared stream reader or launch module changes.
• The description was corrected: it now names Share the managed process lifecycle for structured providers #25204 as the follow-up that unifies the full process lifecycle, and it no longer cites a closed PR.

Deferred (small, no behavior impact): the shared stream reader has no direct test at its new location (it is covered through the Codex connection tests); on Windows, environment-variable names in overrides are case-sensitive (this predates this PR); naming and fixture nits.

Heads-up for landing order: #24988 adds a test that fails once src/main/provider-process/ exists while that PR still lists it as allowed-missing. Whichever of the two lands second drops that allowance.

What was verified

• Side-by-side test comparing main's spawn code with this branch for Codex and Claude on Windows, macOS and Linux: identical, 15/15.
• 16 explicit test files (154 tests) on the final head. Two review rounds plus a readiness checklist found no blocking issues.
• CI on 987fb5307f2: typecheck, static analysis, the real Codex contract job, packaging (Linux and Windows) and cross-version wire checks pass.
• Two unit shards fail only in renderer translation tests (source-control-discard-localization.test.ts, NativeChatSupportedAgents.test.tsx). This PR touches no renderer files, and the same tests also fail locally on the merged tree, so they come from current main.

Not verified: no live app or real agent run; Windows, Linux and SSH were covered only by platform-parameterized tests, not real hosts.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6f185aba-98d9-4b6e-9361-bdc69bd47892

📥 Commits

Reviewing files that changed from the base of the PR and between b354aab and 1144921.

📒 Files selected for processing (29)

• config/scripts/ci-shard-timings.json
• config/scripts/pr-code-change-scope.mjs
• config/scripts/pr-code-change-scope.test.mjs
• config/tsconfig.cli.json
• src/main/claude/claude-agent-sdk-process-spawn.test.ts
• src/main/claude/claude-agent-sdk-process-spawn.ts
• src/main/claude/claude-child-exit-proof-ladder.test.ts
• src/main/claude/claude-child-exit-proof-ladder.ts
• src/main/claude/claude-supervised-stop.integration.test.ts
• src/main/codex/codex-app-server-connection.test.ts
• src/main/codex/codex-app-server-connection.ts
• src/main/codex/codex-app-server-jsonl.ts
• src/main/codex/codex-app-server-record-dispatch.ts
• src/main/codex/codex-app-server-session.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-recovery-resolution.ts
• src/main/provider-process/provider-json-record.ts
• src/main/provider-process/provider-process-exit-deadline.ts
• src/main/provider-process/provider-process-launch.test.ts
• src/main/provider-process/provider-process-launch.ts
• src/main/provider-process/provider-process-supervisor.integration.test.ts
• src/main/provider-process/provider-process-supervisor.test.ts
• src/main/provider-process/provider-process-supervisor.ts
• src/main/provider-process/provider-process-teardown.test.ts
• src/main/provider-process/provider-process-teardown.ts
• src/main/provider-process/provider-record-reader.ts
• src/shared/child-process/__fixtures__/child-process-import-allowlist.txt

💤 Files with no reviewable changes (1)

• src/main/codex/codex-app-server-jsonl.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

---

📝 Walkthrough

Walkthrough

The change generalizes process launch, supervision, teardown, and record-reading utilities under provider-process APIs. Codex code adopts the shared APIs, and Claude and native-chat code update supervisor imports. Tests cover environment resolution, supervision, and teardown. CLI includes, CI path matching, timing entries, and the child-process import allowlist are updated.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 11449

The shared process utilities show no established behavior regression or merge-blocking risk. The change is ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 11449

The inspected changes preserve existing command authority, environment filtering and process-cleanup behavior. No introduced security concern was identified. Risk remains low rather than minimal because broader exposure and runtime behavior were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant exposure is local provider processes and the commands, working directories and credentials supplied by their existing callers. The inspected changes do not add cross-service, tenant or infrastructure authority; non-repository consumers were not assessable.

Security Findings and Attack Paths

• inferred — No new attacker-controlled command source or credential-policy bypass was identified in the inspected consumers. The shared utility accepts caller-provided launch data; it does not introduce an external source of commands or credentials. This is a bounded source assessment, not a claim of complete Security coverage.

Trust Boundaries and Controls

• observed — Environment policy remains caller-owned. Claude constructs its scrubbed environment before invoking the shared spawn path; Codex retains explicit deletion after overlays, including the request-session rule for inherited CODEX_HOME. The shared resolver does not replace these policies with unrestricted process.env inheritance for Claude.

Resilience and Maintainability Implications

• observed — The supervisor retains owner-loss shutdown and serialized stop behavior, and waits for provider-group reaping before relaying exit. Codex retains its bounded stop and forced-teardown path. The extraction therefore preserves the inspected controls intended to contain abandoned provider processes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 25 files. (3 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: extracting provider process supervision and stream reading from Codex.
Description check	✅ Passed	The description covers the required sections with clear scope, rationale, testing details, review notes, and checklist status. It explains that there is no UI change and reports that CI typecheck was …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 25 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}