feat: multi-provider orchestration with Elixir OTP harness - #1
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between b8117b3a639207965cede20a15f926d008955c85 and e72e0f2. ⛔ Files ignored due to path filters (21)
📒 Files selected for processing (89)
📝 WalkthroughWalkthroughWide-ranging changes across Harness (Elixir) and Server (TypeScript) touch provider/session lifecycles, PTY runtime selection, Claude SDK options, error handling in stress/benchmark scripts, new e2e Playwright tests, many benchmark scripts, and assorted test/spec additions and formatting refinements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/harness/config/runtime.exs`:
- Around line 15-17: The current check only rejects the literal
"dev-harness-secret" so blank or whitespace values still pass; update the
conditional around harness_secret to reject nil/empty/whitespace and the default
literal by normalizing (e.g., trim and fallback to empty string) and then
raising when the trimmed value is empty or equals "dev-harness-secret" (keep the
same raise message), ensuring this validation runs where the existing if
harness_secret ... end block and the harness_secret variable are defined.
In `@apps/harness/lib/harness/providers/codex_session.ex`:
- Around line 540-542: send_to_port/2 currently returns {:error, :port_closed}
but callers ignore its result, causing false-success replies; change
send_to_port/2 callsites in this module to propagate and handle its {:ok, _} |
{:error, _} return instead of discarding it (e.g., replace blind
send_to_port(...) calls with a case or with {:ok, _} = send_to_port(...) pattern
or by returning the error upstream), and update any higher-level functions that
aggregate those calls so they return an error when transport fails rather than
always returning :ok; keep send_to_port/2 signature but ensure its callers (all
places invoking send_to_port/2 in this file) do not ignore the result and
properly propagate {:error, :port_closed}.
In `@apps/harness/lib/harness/session_manager.ex`:
- Around line 89-95: The lifecycle event emission uses the incoming provider
before reuse resolution, causing mismatched provider state; update the flow so
the code resolves the running provider via
Registry.lookup(Harness.SessionRegistry, thread_id) into actual_provider
(currently computed in the reuse branch) before emitting the
"session/connecting" event, and use actual_provider in the event payload (or
move/hoist the "session/connecting" emission to after the start/reuse decision
is made) so the emitted provider always matches the provider returned for the
session.
In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts`:
- Around line 890-891: Replace the single fixed sleep used after auto-creating a
thread in ProviderRuntimeIngestion (the yield* Effect.sleep("50 millis") after
the thread.create handling) with a bounded readiness retry loop: attempt up to
~8 retries with a small backoff (e.g., 25ms) between attempts and re-check the
thread materialization/lookup each iteration, proceeding once materialized or
falling back only after retries exhausted; apply the same retry pattern to the
later branch where the code currently logs "Dropping event" when the thread is
still missing so that the drop path only occurs after the bounded retry attempts
fail. Ensure you modify the thread-create handling and the subsequent
missing-thread check (look for references to thread.create, the sleep call, and
the "Dropping event" log) to use the retry-with-backoff logic instead of a
single sleep.
In `@apps/server/src/serverLayers.ts`:
- Around line 53-57: The current runtime selection sets runtime = "bun" whenever
Bun is present, which causes Bun on Windows to import BunPTY (whose layer dies);
change the selection logic so that when process.versions.bun is present but
process.platform === "win32" you fall back to the Node adapter. Update the
runtime/loading branch that uses runtime and runtimePtyAdapterLoaders (the
runtime variable and loader lookup used to produce ptyAdapterModule in
makeServerRuntimeServicesLayer) to choose 'node' instead of 'bun' on Windows so
Bun installs use the Node PTY adapter.
In `@scripts/stress-test-node.ts`:
- Around line 496-501: The turnTimings entry for a given tid can remain stale
because only the timeout path deletes it; update measureTurnLatency so every
completion path removes turnTimings.get(tid): modify the resolver stored in
turnTimings (resolver: (lat) => { turnTimings.delete(tid); clearTimeout(timer);
resolve(lat); }) and also delete the entry in the sendTurn rejection handler
(sendTurn(session, prompt).catch(() => { turnTimings.delete(tid);
clearTimeout(timer); resolve(-1); })), keeping the existing timeout path that
already deletes the entry.
In `@scripts/test-harness-connection.ts`:
- Around line 46-55: After JSON.parse, validate that the parsed msg is an array
of expected length and types before destructuring: check Array.isArray(msg) and
msg.length >= 5, then safely extract elements (or index into msg) and normalize
the ref to a string (e.g., const refStr = String(ref)) before using
pending.has(refStr); also ensure event is a string before comparing to
"phx_reply" so destructuring and lookups cannot throw on malformed frames in the
block around msg, msg parsing, and the event/ref handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3af1b4e3-e44d-43cd-8865-0542a75899ef
📥 Commits
Reviewing files that changed from the base of the PR and between 929a3ce and b8117b3a639207965cede20a15f926d008955c85.
📒 Files selected for processing (24)
apps/harness/bin/harnessapps/harness/config/runtime.exsapps/harness/lib/harness/providers/codex_session.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/lib/harness/session_manager.exapps/harness/lib/harness/snapshot.exapps/harness/test/harness/snapshot_server_test.exsapps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/HarnessClientManager.tsapps/server/src/serverLayers.tsapps/server/src/terminal/Layers/BunPTY.tsapps/server/src/terminal/Layers/NodePTY.tsscripts/node-metrics-collector.tsscripts/stress-test-exception.tsscripts/stress-test-node.tsscripts/stress-test-real-claude.tsscripts/stress-test-real-workload.tsscripts/stress-test-scale50.tsscripts/stress-test-subagent.tsscripts/test-harness-connection.ts
Comment on lines
+46
to
55
| let msg: unknown[]; | ||
| try { | ||
| msg = JSON.parse(text); | ||
| } catch (err) { | ||
| console.error(" Failed to parse message:", err); | ||
| return; | ||
| } | ||
| const [, ref, , event, payload] = msg; | ||
|
|
||
| if (event === "phx_reply" && ref && pending.has(ref)) { |
There was a problem hiding this comment.
Validate Phoenix frame shape before destructuring.
At Line 53, parsing success does not guarantee tuple shape. A non-array payload can still throw at destructuring, and ref should be normalized to string for pending lookups.
Suggested fix
- let msg: unknown[];
+ let msg: unknown;
try {
msg = JSON.parse(text);
} catch (err) {
console.error(" Failed to parse message:", err);
return;
}
- const [, ref, , event, payload] = msg;
+ if (!Array.isArray(msg) || msg.length !== 5) {
+ console.warn(" Ignoring non-Phoenix frame");
+ return;
+ }
+ const [, refRaw, , event, payload] = msg;
+ const ref = refRaw == null ? null : String(refRaw);
- if (event === "phx_reply" && ref && pending.has(ref)) {
+ if (event === "phx_reply" && ref !== null && pending.has(ref)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-harness-connection.ts` around lines 46 - 55, After JSON.parse,
validate that the parsed msg is an array of expected length and types before
destructuring: check Array.isArray(msg) and msg.length >= 5, then safely extract
elements (or index into msg) and normalize the ref to a string (e.g., const
refStr = String(ref)) before using pending.has(refStr); also ensure event is a
string before comparing to "phx_reply" so destructuring and lookups cannot throw
on malformed frames in the block around msg, msg parsing, and the event/ref
handling.
Collaborator
Author
There was a problem hiding this comment.
Acceptable for a dev diagnostic script. The Phoenix Channel protocol guarantees well-formed JSON frames over WebSocket, so parse failures here indicate a broken transport (which surfaces as a connection error). Adding defensive validation would obscure the real issue in a tool meant for debugging.
There was a problem hiding this comment.
@ranvier2d2, that's a fair point — for a diagnostic tool, noisy failure is preferable to silent swallowing, and the transport error will surface clearly before any malformed frame could appear. I'll defer to your judgment here.
🐇 🤔
✏️ Learnings added
Learnt from: ranvier2d2
Repo: Ranvier-Technologies/t3code-OTP PR: 1
File: scripts/test-harness-connection.ts:46-55
Timestamp: 2026-03-24T15:20:48.633Z
Learning: In `scripts/test-harness-connection.ts`, the lack of defensive Array shape validation after `JSON.parse` is intentional. This is a developer diagnostic script where Phoenix Channel protocol guarantees well-formed frames; parse failures indicate broken transport and should surface loudly rather than be silently caught. Adding frame-shape guards would obscure the real issue in a debugging tool.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8117b3a63
ℹ️ 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".
Comment on lines
+178
to
+180
| const child = spawn("claude", [ | ||
| "--print", | ||
| "--output-format", "stream-json", |
There was a problem hiding this comment.
Switching from the shell command to spawn("claude", ...) removed the previous < /dev/null redirection, so each child now keeps an open stdin pipe. In environments where claude --print still reads stdin when available, these runs can hang waiting for EOF, which skews stress-test completion and latency metrics. Please explicitly close stdin (for example stdio[0] = "ignore" or child.stdin.end()) to preserve the prior non-interactive behavior.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
✅ Fixed. Changed stdio[0] from "pipe" to "ignore" so stdin is closed immediately, matching the prior < /dev/null behavior.
Comment on lines
+821
to
+823
| step_id = Map.get(part, "id") | ||
|
|
||
| if step_id do |
There was a problem hiding this comment.
step-start emits item/started with a synthetic ID when part.id is missing, but step-finish only emits completion if part.id is present. When OpenCode omits IDs (which the start path already anticipates), the completion event is dropped and the UI can show permanently-running step items. Persisting/deriving the fallback ID for finish events would avoid dangling work-log entries.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
✅ Fixed. step-finish now generates a fallback ID with generate_id() when part.id is missing (same pattern as step-start). The nil guard was removed so completion events are always emitted, preventing dangling work-log entries.
Clear the listSessions cache when startSession or stopSession is called, preventing stale cache from causing "Cannot recover thread" errors when ProviderService.sendTurn races with session creation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked from pingdotgg/t3code@754cded2. Changes PTY adapter selection from static typeof-Bun check to runtime dynamic import. Renames exports to `layer` convention. Original authors: shivam, Julius Marminge
Cherry-picked from pingdotgg/t3code@2b6640af. Adds settingSources: ["user", "project", "local"] to Claude SDK createQuery so it loads CLAUDE.md, project settings, and local config from the filesystem. Original author: Harshit Agarwal
Classify subtask/agent/step-start/step-finish SSE part types in opencode_session.ex, track child sessions via parentID, and map collab_agent_spawn_begin to task.started in HarnessClientAdapter. Verified E2E: OpenCode dispatched 2 parallel subagents (Explore + General), child sessions tracked, steps and subtask tool calls visible in UI work log. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change collab_agent_spawn_begin mapping from task.started (filtered by session-logic.ts) to item.started with collab_agent_tool_call itemType. Child session spawns now appear in the UI work log with HammerIcon alongside step entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HIGH: fix pushChannel→push runtime crash in HarnessClientManager, add warning log + 50ms delay for projection lag in ProviderRuntimeIngestion, fix provider mismatch in SessionManager already_started, guard nil port in CodexSession, fix runtime_mode typespec in Snapshot. MEDIUM: enforce T3CODE_HARNESS_SECRET in prod, replace Process.sleep with :sys.get_state in snapshot tests. LOW: add import error logging to 5 stress-test scripts, validate duration in node-metrics-collector, fix dead code in bin/harness, wrap JSON.parse in test-harness-connection, fix shell injection in stress-test-real-claude spawn, fix async executor anti-pattern and empty catch in stress-test-node, deduplicate test prefix in ProjectionPipeline.test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude --print mode emits task_progress and task_notification as system messages through the Elixir harness. These were falling through to codexEventMapping which doesn't handle unprefixed task events, causing them to be logged as "unmapped" and dropped. Now handled in mapHarnessEventToRuntimeEvents: - task_progress → item.started with tool name and description - task_notification → item.completed with summary - collab_waiting_begin → item.started (parent waits for subagent) - collab_waiting_end → item.completed (subagent finished) Verified via 4-provider E2E test: Claude spawned 2 subagents (task_ids a17d50dd... and a6bbcc70...) that were previously invisible in the UI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Provider routing: - Single makeServerProviderLayer: Claude + Codex via Node SDK, Cursor + OpenCode via Elixir harness (conditional on harnessPort) - claudeBinaryPath app setting + health check fallback paths - Codex delta dedup: suppress duplicate agent_message_content_delta E2E tests: - @playwright/test setup at apps/web/e2e/ with page-object helpers - All 4 providers verified (send+receive, picker) with screenshot evidence CodeRabbit fixes: - cursor_session.ex: :noeol buffer, spawn failure reply, synthetic resume ID, read_thread returns actual turns - claude_session.ex: :noeol buffer prepend - session_manager.ex: validate threadId, connecting event after start_child - projector.ex: clear active_turn on session/error - snapshot_server.ex: replay gap off-by-one - HarnessClientAdapter.ts: session-not-found pattern, replay gap handling - ProviderHealth.ts: cursor-agent binary, Claude fallback paths - codexEventMapping.ts: remove duplicate delta handler - composerDraftStore.ts: inferProviderForModel for stickyModel - appSettings.ts: Partial<Record> for custom model config - opencode_session.ex: step-finish fallback ID - runtime.exs: reject blank/whitespace harness secret - serverLayers.ts: Bun on Windows PTY fallback - CI vouch gate: only run on vouch:trusted PRs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ranvier2d2
force-pushed
the
feat/opencode-subagents
branch
from
March 24, 2026 15:32
b8117b3 to
e72e0f2
Compare
This was referenced Mar 24, 2026
9 tasks
This was referenced Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Multi-provider AI coding orchestration for T3 Code — 4 providers (Claude, Codex, Cursor, OpenCode) running through a hybrid Node SDK + Elixir OTP architecture.
Architecture
Key Changes
Provider Routing (
serverLayers.ts)makeServerProviderLayer()— no conditional modesharnessPortconfigured, otherwiseProviderUnsupportedErrorElixir Harness (
apps/harness/)SessionManagerroutes to provider-specific GenServers (ClaudeSession, CursorSession, OpenCodeSession, CodexSession):noeol/:eolbuffer handlingE2E Tests (
apps/web/e2e/)@playwright/testwith page-object helpersBug Fixes (CodeRabbit)
:eolhandler in cursor_session.ex and claude_session.ex now prepends buffered:noeoldata before JSON decodeagent_message_content_deltaevent--resume,read_threadreturns actual turnssession/connectingonly for new sessionsactive_turnonsession/errorcursor-agentbinary usageTest Plan
vitest run— unit tests pass (appSettings 21/21, codexCliVersion 26/26)playwright test— E2E 8/8 (Codex, Claude, Cursor, OpenCode × send+receive, picker)tsc --noEmit— compiles clean (1 pre-existing error in ProviderRuntimeIngestion.ts)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation