feat(harness): Sprint 2 — shared OpenCode runtime architecture - #41
Merged
ranvier2d2 merged 4 commits intoMar 30, 2026
Merged
Conversation
Original prompt from Bastian
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared OpenCode runtime infrastructure: a deterministic RuntimeKey, an ETS-backed RuntimeRegistry with ref-counting and monitoring, a RuntimeSupervisor and OpenCodeRuntime GenServer that owns/opencodes processes and fans SSE/HTTP to subscribers, and refactors OpenCodeSession to lease/subscribe to shared runtimes. Changes
Sequence DiagramsequenceDiagram
participant S as OpenCodeSession
participant Reg as RuntimeRegistry
participant RT as OpenCodeRuntime
participant OP as opencode Process
S->>Reg: lookup(RuntimeKey)
alt runtime found & alive
Reg-->>S: {:ok, runtime_pid}
else not found
S->>RT: request start/lease(RuntimeKey)
RT->>OP: spawn "opencode serve" (port)
OP-->>RT: port established
RT->>OP: GET /global/health
OP-->>RT: healthy
RT->>Reg: register(RuntimeKey, runtime_pid)
Reg-->>S: {:ok, runtime_pid}
end
S->>RT: lease_and_subscribe(session_meta)
RT->>Reg: increment_ref(RuntimeKey)
Reg-->>RT: {:ok, count}
RT-->>S: {:ok, runtime_pid}
OP->>RT: SSE events
RT->>S: {:runtime_sse_event, event} (fan-out)
S->>RT: create_session / send_prompt / fetch_messages / mcp_*
RT->>OP: proxied HTTP requests
OP-->>RT: responses
RT-->>S: proxied responses
S->>RT: release(session_id)
RT->>Reg: decrement_ref(RuntimeKey)
Reg-->>RT: {:ok, new_count}
alt refs == 0 && no subscribers
RT->>RT: schedule idle_shutdown -> stop OP, unregister
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
added
size:XXL
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/harness/lib/harness/providers/opencode_session.ex (1)
1046-1052: Minor: Potential timing gap calculation issue.At line 1051,
stateis reassigned withfirst_permission_asked_at: now. Then line 1052 calculatesgapusingstate.sse_connected_atfrom this same updated state. This works correctly sincesse_connected_atwas set duringsetup_with_runtimeand isn't modified here, but the flow could be clearer by computinggapbefore updating state.💡 Clearer variable scoping
else now = System.monotonic_time(:millisecond) + gap = if state.sse_connected_at, do: now - state.sse_connected_at, else: nil state = %{state | first_permission_asked_at: now, permission_timing_logged: true} - gap = if state.sse_connected_at, do: now - state.sse_connected_at, else: nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 1046 - 1052, The timing gap calculation currently reads sse_connected_at from the updated state after setting first_permission_asked_at, which is confusing; instead compute gap using the original state's sse_connected_at before reassigning state (check the variables permission_timing_logged, first_permission_asked_at, sse_connected_at, and gap), then update state with first_permission_asked_at and permission_timing_logged and use the previously computed gap value.apps/harness/test/harness/opencode/runtime_key_test.exs (1)
42-51: Consider testing hash determinism with different key orderings.This test verifies that the same map literal hashes consistently, but doesn't verify that semantically equivalent maps constructed differently produce the same hash. Adding such a test would help validate the fix for the determinism concern in
hash_mcp_config/1.💡 Additional test case
test "equivalent mcp_config with different key order produces same hash" do # Build maps in different ways to potentially get different internal ordering mcp1 = %{"z" => 1, "a" => 2, "m" => 3} mcp2 = Map.new([{"a", 2}, {"m", 3}, {"z", 1}]) key1 = RuntimeKey.from_params(%{"cwd" => "/tmp/project", "mcp_config" => mcp1}) key2 = RuntimeKey.from_params(%{"cwd" => "/tmp/project", "mcp_config" => mcp2}) assert key1.mcp_config_hash == key2.mcp_config_hash end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/test/harness/opencode/runtime_key_test.exs` around lines 42 - 51, Add a test to verify hash determinism across different map key orderings by constructing semantically identical mcp_config maps with different insertion orders and asserting RuntimeKey.from_params returns RuntimeKey structs whose mcp_config_hash values are equal; specifically, call RuntimeKey.from_params with one map literal and another built via Map.new/1 (or Enum.into) with shuffled key tuples and assert RuntimeKey.hash_mcp_config/1 (indirectly via RuntimeKey.from_params) yields identical hashes to ensure hash_mcp_config/1 is order-independent.apps/harness/test/harness/opencode/runtime_registry_test.exs (2)
70-86: AvoidProcess.sleep/1for synchronization.Per coding guidelines, use
Process.monitor/1withassert_receive {:DOWN, ...}or_ = :sys.get_state(RuntimeRegistry)to ensure the registry has processed the DOWN message before re-registering.♻️ Suggested fix using monitor pattern
test "allows re-registration after process dies" do key = make_key() pid1 = start_dummy_process() + ref = Process.monitor(pid1) assert :ok = RuntimeRegistry.register(key, pid1) # Kill the process send(pid1, :stop) - Process.sleep(50) + assert_receive {:DOWN, ^ref, :process, ^pid1, _}, 500 + # Ensure registry processed its monitor's DOWN message + _ = :sys.get_state(RuntimeRegistry) # Now register a new process — should succeed because the old one is dead pid2 = start_dummy_process()As per coding guidelines: "Avoid
Process.sleep/1andProcess.alive?/1in tests; useProcess.monitor/1and assert on the DOWN message to wait for process completion" and "use_ = :sys.get_state/1to ensure the process has handled prior messages instead of sleeping."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/test/harness/opencode/runtime_registry_test.exs` around lines 70 - 86, The test uses Process.sleep/1 to wait for the dummy process to die before re-registering; replace that with a deterministic wait using Process.monitor/1 and assert_receive on the {:DOWN, _ref, :process, ^pid1, _reason} tuple (or alternatively call _ = :sys.get_state(RuntimeRegistry) to ensure the registry processed the DOWN) so that RuntimeRegistry.register(key, pid2) is attempted only after the registry has handled the previous process termination; update the test around start_dummy_process, send(pid1, :stop), and the subsequent registration/lookup to use the monitor/assert_receive (or sys.get_state) approach instead of Process.sleep.
199-214: AvoidProcess.sleep/1for synchronization.Same issue here—use a monitor and
:sys.get_state/1to synchronize deterministically.♻️ Suggested fix
test "cleans up registry when monitored process dies" do key = make_key() pid = start_dummy_process() + ref = Process.monitor(pid) RuntimeRegistry.register(key, pid) assert {:ok, ^pid} = RuntimeRegistry.lookup(key) # Kill the process Process.exit(pid, :kill) - # Give the registry time to process the DOWN message - Process.sleep(100) + # Wait for process death + assert_receive {:DOWN, ^ref, :process, ^pid, :killed}, 500 + # Ensure registry processed its DOWN message + _ = :sys.get_state(RuntimeRegistry) assert :error = RuntimeRegistry.lookup(key) endAs per coding guidelines: "In tests, use
_ = :sys.get_state/1to ensure the process has handled prior messages instead of sleeping to synchronize before the next call."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/test/harness/opencode/runtime_registry_test.exs` around lines 199 - 214, The test uses Process.sleep/1 to wait for the registry to process the DOWN message; replace that nondeterministic sleep with a deterministic sync: after Process.exit(pid, :kill) call Process.monitor(pid) to ensure a DOWN is delivered and then call _ = :sys.get_state(RuntimeRegistry) (or _ = :sys.get_state(RuntimeRegistryProcess) if the registry is a named process) to force the registry to process its message queue before asserting RuntimeRegistry.lookup(key) returns :error; update the test around RuntimeRegistry.register/2, Process.exit/2 and RuntimeRegistry.lookup/1 to remove Process.sleep/1 and use the monitor + :sys.get_state/1 synchronization instead.apps/harness/lib/harness/providers/opencode_runtime.ex (1)
616-621: TOCTOU race in port allocation.There's a small window between closing the socket and
opencode servebinding to the port where another process could claim it. This is a known limitation of this pattern and typically acceptable, but given the PR objective to "reduce port conflicts," consider documenting this limitation or adding retry logic inspawn_opencodeif port binding fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 616 - 621, find_available_port currently picks a free port then closes the socket, creating a TOCTOU race where another process may bind the port before opencode does; update spawn_opencode to handle bind failures by adding retry logic: on opencode serve failure due to EADDRINUSE (or generic bind error) call find_available_port again and retry spawn up to a small max attempts with short backoff and clear logging, and also add a brief note in the function docstring or comments for find_available_port describing this race condition and why retries are necessary.
🤖 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/lib/harness/opencode/runtime_key.ex`:
- Around line 85-91: The hash_mcp_config/1 function uses Jason.encode!/1 on a
map which can produce non-deterministic JSON for maps with varying key orders;
update hash_mcp_config to canonicalize the config before encoding by recursively
sorting map keys (including nested maps) and preserving deterministic ordering
for lists, then call Jason.encode! on that normalized structure so the resulting
SHA256 (in hash_mcp_config) is stable across semantically identical configs.
Ensure you reference and modify the hash_mcp_config function to perform this
canonicalization step prior to hashing.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 930-937: The recovery path after an :already_registered race calls
RuntimeRegistry.increment_ref(key) but ignores its result, which can be
:not_found if the runtime died between lookup and increment; update the code in
the lease/2 handling to check the return value of
RuntimeRegistry.increment_ref(key) and on :not_found (or other transient
failure) return or loop into a retry (e.g., return :runtime_race_retry or re-run
the lookup/increment sequence) instead of unconditionally returning {:ok,
existing_pid}; ensure the retry is performed inside lease/2 so callers never
receive a false {:ok, pid} for a dead process and reference the
RuntimeRegistry.lookup/1 and RuntimeRegistry.increment_ref/1 calls when making
the change.
- Around line 683-698: The start_sse_listener/1 function is scheduling a
:health_check timer each time it runs (via Process.send_after(...,
`@health_check_interval_ms`)), which causes duplicate timers on SSE reconnects;
modify the code so that start_sse_listener/1 no longer schedules the health
check and instead schedule the periodic health check only once in the initial
:setup handler (or alternatively keep a reference to the existing timer and
cancel it before scheduling a new one), referencing start_sse_listener/1,
sse_loop, the :health_check message, and `@health_check_interval_ms` when making
the change.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 683-700: The event_relevant?/2 filter currently prefers child
session IDs (info.id) over parent IDs, causing parent sessions to miss
session.created events; update event_relevant?/2 to either (A) check for a
parent session first by looking up get_in(data, ["info", "parentID"]) before
get_in(data, ["info", "id"]) or (B) special-case session.created events by
inspecting Map.get(data, "type") (or equivalent) and using parentID when
present; ensure you still fall back to sessionId, session_id, id, and the
nil-pass-through behavior for message-level events, and keep the guard signature
and return semantics of event_relevant?/2 unchanged.
---
Nitpick comments:
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 616-621: find_available_port currently picks a free port then
closes the socket, creating a TOCTOU race where another process may bind the
port before opencode does; update spawn_opencode to handle bind failures by
adding retry logic: on opencode serve failure due to EADDRINUSE (or generic bind
error) call find_available_port again and retry spawn up to a small max attempts
with short backoff and clear logging, and also add a brief note in the function
docstring or comments for find_available_port describing this race condition and
why retries are necessary.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 1046-1052: The timing gap calculation currently reads
sse_connected_at from the updated state after setting first_permission_asked_at,
which is confusing; instead compute gap using the original state's
sse_connected_at before reassigning state (check the variables
permission_timing_logged, first_permission_asked_at, sse_connected_at, and gap),
then update state with first_permission_asked_at and permission_timing_logged
and use the previously computed gap value.
In `@apps/harness/test/harness/opencode/runtime_key_test.exs`:
- Around line 42-51: Add a test to verify hash determinism across different map
key orderings by constructing semantically identical mcp_config maps with
different insertion orders and asserting RuntimeKey.from_params returns
RuntimeKey structs whose mcp_config_hash values are equal; specifically, call
RuntimeKey.from_params with one map literal and another built via Map.new/1 (or
Enum.into) with shuffled key tuples and assert RuntimeKey.hash_mcp_config/1
(indirectly via RuntimeKey.from_params) yields identical hashes to ensure
hash_mcp_config/1 is order-independent.
In `@apps/harness/test/harness/opencode/runtime_registry_test.exs`:
- Around line 70-86: The test uses Process.sleep/1 to wait for the dummy process
to die before re-registering; replace that with a deterministic wait using
Process.monitor/1 and assert_receive on the {:DOWN, _ref, :process, ^pid1,
_reason} tuple (or alternatively call _ = :sys.get_state(RuntimeRegistry) to
ensure the registry processed the DOWN) so that RuntimeRegistry.register(key,
pid2) is attempted only after the registry has handled the previous process
termination; update the test around start_dummy_process, send(pid1, :stop), and
the subsequent registration/lookup to use the monitor/assert_receive (or
sys.get_state) approach instead of Process.sleep.
- Around line 199-214: The test uses Process.sleep/1 to wait for the registry to
process the DOWN message; replace that nondeterministic sleep with a
deterministic sync: after Process.exit(pid, :kill) call Process.monitor(pid) to
ensure a DOWN is delivered and then call _ = :sys.get_state(RuntimeRegistry) (or
_ = :sys.get_state(RuntimeRegistryProcess) if the registry is a named process)
to force the registry to process its message queue before asserting
RuntimeRegistry.lookup(key) returns :error; update the test around
RuntimeRegistry.register/2, Process.exit/2 and RuntimeRegistry.lookup/1 to
remove Process.sleep/1 and use the monitor + :sys.get_state/1 synchronization
instead.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60f20e07-015b-45a6-818a-491cc018b478
📒 Files selected for processing (9)
apps/harness/lib/harness/application.exapps/harness/lib/harness/opencode/runtime_key.exapps/harness/lib/harness/opencode/runtime_registry.exapps/harness/lib/harness/providers/opencode_runtime.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/test/harness/opencode/runtime_key_test.exsapps/harness/test/harness/opencode/runtime_registry_test.exsapps/harness/test/harness/providers/opencode_runtime_test.exsapps/harness/test/harness/providers/opencode_session_test.exs
Comment on lines
+683
to
+698
| defp start_sse_listener(state) do | ||
| parent = self() | ||
| url = "#{state.base_url}/event" | ||
|
|
||
| pid = | ||
| spawn(fn -> | ||
| sse_loop(parent, url) | ||
| end) | ||
|
|
||
| Process.monitor(pid) | ||
|
|
||
| # Schedule periodic health checks | ||
| Process.send_after(self(), :health_check, @health_check_interval_ms) | ||
|
|
||
| pid | ||
| end |
There was a problem hiding this comment.
Health check timer duplication on SSE reconnect.
start_sse_listener/1 schedules a new :health_check timer (line 695) each time it's called. Since this function is invoked both during initial setup and on every SSE reconnect (line 344), multiple independent health check timer chains will accumulate over time, causing redundant health checks and a slow memory leak.
Move health check scheduling to the initial :setup handler only, or track and cancel the existing timer before scheduling a new one.
🛠️ Proposed fix
defp start_sse_listener(state) do
parent = self()
url = "#{state.base_url}/event"
pid =
spawn(fn ->
sse_loop(parent, url)
end)
Process.monitor(pid)
- # Schedule periodic health checks
- Process.send_after(self(), :health_check, `@health_check_interval_ms`)
-
pid
endThen schedule the health check once in the :setup handler:
def handle_info(:setup, state) do
case wait_for_server(state.base_url, 45) do
:ok ->
sse_pid = start_sse_listener(state)
state = %{state | sse_pid: sse_pid, ready: true, health: :ready}
+ # Schedule periodic health checks (once, on initial setup)
+ Process.send_after(self(), :health_check, `@health_check_interval_ms`)
+
Logger.info(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 683 -
698, The start_sse_listener/1 function is scheduling a :health_check timer each
time it runs (via Process.send_after(..., `@health_check_interval_ms`)), which
causes duplicate timers on SSE reconnects; modify the code so that
start_sse_listener/1 no longer schedules the health check and instead schedule
the periodic health check only once in the initial :setup handler (or
alternatively keep a reference to the existing timer and cancel it before
scheduling a new one), referencing start_sse_listener/1, sse_loop, the
:health_check message, and `@health_check_interval_ms` when making the change.
|
Devin is currently unreachable - the session may have died. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/harness/lib/harness/providers/opencode_session.ex (1)
1076-1087:⚠️ Potential issue | 🟡 MinorAuto-approval permission reply result is not checked.
When in
full-accessruntime mode,reply_to_permissionis called but its result is ignored. If the permission reply fails (e.g., network issue, runtime died), the permission request remains pending on the OpenCode server while the session assumes it was approved.Consider logging or handling the error case to aid debugging, or at minimum ensuring the state remains consistent if the reply fails.
🛡️ Proposed fix to log failures
if runtime_mode == "full-access" do # Auto-approve in full-access mode Logger.info("Auto-approving permission #{permission} (full-access mode)") - OpenCodeRuntime.reply_to_permission( - state.runtime_pid, - state.opencode_session_id, - permission_id, - "always" - ) + case OpenCodeRuntime.reply_to_permission( + state.runtime_pid, + state.opencode_session_id, + permission_id, + "always" + ) do + :ok -> :ok + {:error, reason} -> + Logger.warning("Failed to auto-approve permission #{permission}: #{inspect(reason)}") + end state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 1076 - 1087, The auto-approval block ignores the return value of OpenCodeRuntime.reply_to_permission, so failures go unnoticed; update the runtime_mode "full-access" branch to pattern-match the call result (e.g., case OpenCodeRuntime.reply_to_permission(state.runtime_pid, state.opencode_session_id, permission_id, "always") do {:ok, _} -> state; {:error, reason} -> Logger.error("Failed auto-approving permission #{permission}: #{inspect(reason)}"); maybe take corrective action (keep state unchanged or trigger retry/cleanup) end) so errors are logged and state remains consistent, referencing runtime_mode, OpenCodeRuntime.reply_to_permission, state.runtime_pid, state.opencode_session_id, permission_id and permission.
🧹 Nitpick comments (2)
apps/harness/lib/harness/providers/opencode_runtime.ex (2)
648-653: Minor TOCTOU race in port allocation.The pattern of opening a socket on port 0, reading the assigned port, then closing the socket before
opencode servebinds to it leaves a window where another process could claim the port. This is a common technique, but under high concurrency it can cause intermittentopencode servestartup failures.The lease retry logic in
do_lease_and_subscribeshould handle this gracefully, but if you see sporadic runtime spawn failures in production, this would be worth revisiting withSO_REUSEPORTor passing the listening socket file descriptor to the child process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 648 - 653, The current find_available_port function closes the socket after reading the ephemeral port, creating a TOCTOU race before opencode serve binds; modify the approach to avoid reopening the port: either (A) enable true port reuse by using SO_REUSEPORT when creating the listener so another process can bind the same port, or (B) change find_available_port to return the open listening socket (not just the port) and adjust the spawn path (do_lease_and_subscribe / opencode serve invocation) to pass the socket file descriptor to the child process so the child reuses the same listener; update do_lease_and_subscribe to accept and propagate the socket/FD to the child or to handle retry when binding fails as a fallback.
719-724: SSE listener process can become orphaned on runtime crash.The SSE listener is spawned with
spawn/1rather thanspawn_link/1. While the runtime monitors the SSE process (line 724), the reverse is not true. If the runtime GenServer crashes abnormally (bypassingterminate/2), the SSE process continues running as an orphan, sending messages to a dead PID until the TCP read times out at 120 seconds (line 788).Consider using
spawn_link/1so the SSE listener terminates with its parent, or have the SSE process monitor the parent and exit when it detects the parent is down.♻️ Proposed fix using spawn_link
defp start_sse_listener(state) do parent = self() url = "#{state.base_url}/event" pid = - spawn(fn -> + spawn_link(fn -> sse_loop(parent, url) end) - Process.monitor(pid) + # With spawn_link, mutual termination is automatic. + # Keep monitor to detect SSE-side failures for reconnect logic. + Process.monitor(pid) # Schedule periodic health checks Process.send_after(self(), :health_check, `@health_check_interval_ms`) pid end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 719 - 724, The SSE listener is started with spawn/1 and can become orphaned if the parent GenServer crashes; change the spawn call to spawn_link(fn -> sse_loop(parent, url) end) so the SSE process is linked and will terminate with the parent, or alternatively modify sse_loop/2 to monitor the parent (Process.monitor(parent)) on start and handle the :DOWN message by exiting; update the code around the pid assignment and Process.monitor(pid) usage accordingly (use spawn_link and keep Process.monitor(pid) for the parent watching the child, or implement parent-monitoring inside sse_loop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 1076-1087: The auto-approval block ignores the return value of
OpenCodeRuntime.reply_to_permission, so failures go unnoticed; update the
runtime_mode "full-access" branch to pattern-match the call result (e.g., case
OpenCodeRuntime.reply_to_permission(state.runtime_pid,
state.opencode_session_id, permission_id, "always") do {:ok, _} -> state;
{:error, reason} -> Logger.error("Failed auto-approving permission
#{permission}: #{inspect(reason)}"); maybe take corrective action (keep state
unchanged or trigger retry/cleanup) end) so errors are logged and state remains
consistent, referencing runtime_mode, OpenCodeRuntime.reply_to_permission,
state.runtime_pid, state.opencode_session_id, permission_id and permission.
---
Nitpick comments:
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 648-653: The current find_available_port function closes the
socket after reading the ephemeral port, creating a TOCTOU race before opencode
serve binds; modify the approach to avoid reopening the port: either (A) enable
true port reuse by using SO_REUSEPORT when creating the listener so another
process can bind the same port, or (B) change find_available_port to return the
open listening socket (not just the port) and adjust the spawn path
(do_lease_and_subscribe / opencode serve invocation) to pass the socket file
descriptor to the child process so the child reuses the same listener; update
do_lease_and_subscribe to accept and propagate the socket/FD to the child or to
handle retry when binding fails as a fallback.
- Around line 719-724: The SSE listener is started with spawn/1 and can become
orphaned if the parent GenServer crashes; change the spawn call to spawn_link(fn
-> sse_loop(parent, url) end) so the SSE process is linked and will terminate
with the parent, or alternatively modify sse_loop/2 to monitor the parent
(Process.monitor(parent)) on start and handle the :DOWN message by exiting;
update the code around the pid assignment and Process.monitor(pid) usage
accordingly (use spawn_link and keep Process.monitor(pid) for the parent
watching the child, or implement parent-monitoring inside sse_loop).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f25e077f-fed0-4a48-9def-9c90a3446971
📒 Files selected for processing (6)
apps/harness/lib/harness/opencode/runtime_key.exapps/harness/lib/harness/providers/opencode_runtime.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/test/harness/opencode/runtime_key_test.exsapps/harness/test/harness/providers/opencode_runtime_test.exsapps/harness/test/harness/providers/opencode_session_test.exs
✅ Files skipped from review due to trivial changes (1)
- apps/harness/test/harness/providers/opencode_runtime_test.exs
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/harness/test/harness/providers/opencode_session_test.exs
- apps/harness/lib/harness/opencode/runtime_key.ex
- apps/harness/test/harness/opencode/runtime_key_test.exs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/harness/lib/harness/providers/opencode_session.ex (2)
392-429:⚠️ Potential issue | 🟠 MajorDon't mark questions/auto-approvals as resolved before the runtime accepts the reply.
respond_to_user_input/3removes the pending entry before callingreply_to_permission/4, and the full-access auto-approve path ignores that call's result entirely. If the runtime RPC fails, Harness treats the request as resolved locally while OpenCode is still blocked on it, so the turn hangs with no retry path.Also applies to: 1091-1100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 392 - 429, In respond_to_user_input/3, don't remove the pending entry from state before calling OpenCodeRuntime.reply_to_permission; instead call reply_to_permission first (using state.runtime_pid, state.opencode_session_id and the computed permission_id/answer_text), check its result and only update state.pending_permissions to remaining and emit_event("user-input/resolved", ...) after a successful RPC; also propagate/handle errors from OpenCodeRuntime.reply_to_permission (and the full-access auto-approve path) so failures do not mark the request resolved locally and allow retry/cleanup.
530-537:⚠️ Potential issue | 🟠 MajorOnly prune local history after
revert_session/2succeeds.This truncates
state.messagesbefore attempting the server-side revert and ignores the result. On a failed revert,read_thread/2keeps serving the truncated in-memory state, so the UI diverges from the actual OpenCode session until restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 530 - 537, handle_call/3 currently truncates state.messages before calling OpenCodeRuntime.revert_session/2 which can leave in-memory history inconsistent if the remote revert fails; change the order to call OpenCodeRuntime.revert_session(state.runtime_pid, state.opencode_session_id) first (only when state.opencode_session_id, num_turns > 0 and state.runtime_pid are present), pattern-match its result (e.g. {:ok, _} vs {:error, reason}), and only on success update state by dropping the last num_turns messages (state = %{state | messages: Enum.drop(state.messages, -num_turns)}); on failure keep the original state, log the error and return an appropriate error reply without mutating state.messages.
♻️ Duplicate comments (1)
apps/harness/lib/harness/providers/opencode_session.ex (1)
699-713:⚠️ Potential issue | 🟠 Major
session.createdstill routes by child id beforeparentID.For child-session creation events that carry both
info.idandinfo.parentID,event_relevant?/2picks the child id first and the parent wrapper drops the event. SinceOpenCodeRuntimebroadcasts every SSE event to every subscriber, this prevents the parent session from ever emittingcollab_agent_spawn_begin.Also applies to: 896-910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 699 - 713, The code in event_relevant?/2 builds event_session by checking info.id before info.parentID, causing child-session creation events to be associated with the child and dropped for parent subscribers; update the construction of event_session in the event_relevant?/2 clause (the variable event_session) to prefer get_in(data, ["info", "parentID"]) before get_in(data, ["info", "id"]) so parentID is used when both exist, and make the identical change in the other occurrence of the same logic elsewhere in this module so both places use parentID first while preserving the existing nil-or-equals comparison to state.opencode_session_id.
🤖 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/lib/harness/providers/opencode_runtime.ex`:
- Around line 322-329: The {:sse_down, reason} handler can schedule a reconnect
twice because it isn't tied to the monitored pid; change the clause to only act
when the state still contains an active sse_pid. Concretely, replace the
catch-all def handle_info({:sse_down, reason}, state) with a clause that matches
on %{sse_pid: pid} and guards is_pid(pid) (or otherwise verifies pid is
present/alive) before calling schedule_sse_reconnect(state, reason), and add a
no-op fallback that ignores {:sse_down, _} when sse_pid is nil; apply the same
pattern to the other similar handlers (the ones around the other referenced
ranges) so schedule_sse_reconnect/2 is only invoked when the current sse_pid
matches an active listener.
- Around line 259-263: The code sets the node state ready immediately after
spawning the SSE process (in handle_info(:setup)) but start_sse_listener/1 only
spawns the process and does not guarantee the SSE TCP connection is established;
change the flow so the state.ready and health=:ready are set only after the SSE
listener reports a successful connection (e.g., have start_sse_listener/1 return
a reference or change it to start_sse_listener_and_wait/1 that blocks until a
:sse_connected message or reply is received, or implement a short handshake
where the new SSE process sends the parent a {:sse_connected, pid} message), and
update the same pattern in the other occurrence (around the code referenced by
the reviewer at lines 724-756) to ensure wait_for_ready/2 is released only after
confirmed SSE connection.
- Around line 84-87: The synchronous startup blocks in handle_info(:setup)
(which polls wait_for_server for ~45s) can cause GenServer.call timeouts in
do_lease_and_subscribe when invoking GenServer.call(pid, {:lease_and_subscribe,
...}) and similarly for the {:subscribe_initial, ...} call; update these calls
to include an explicit timeout that matches or exceeds the startup budget (e.g.,
pass a timeout ms argument like 45_000) or alternatively move the
wait_for_server polling out of the GenServer callback into an async/own process
so the GenServer.calls in do_lease_and_subscribe and the subscribe_initial path
no longer race with startup; locate the calls in do_lease_and_subscribe and the
code handling {:subscribe_initial, ...} and add the timeout parameter (or
refactor handle_info(:setup)/wait_for_server to not block the GenServer).
- Around line 123-128: The GenServer currently lets queued
GenServer.call(wait_for_ready) callers exit when the server stops instead of
returning {:error, reason}; update the server shutdown logic (the code paths
that stop the GenServer on startup timeout and on port exit) to explicitly reply
to all pending waiters with {:error, reason} before calling
GenServer.stop/terminate (i.e., locate the wait queue in the server state used
by wait_for_ready/2 and send GenServer.reply/2 for each PID with {:error,
shutdown_reason}); ensure wait_for_ready/2' s spec remains {:ok | {:error,
term()}} and remove the workaround in callers only if you instead choose to
change the spec (but prefer replying to queued callers so OpenCodeSession can
receive {:error, reason}).
---
Outside diff comments:
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 392-429: In respond_to_user_input/3, don't remove the pending
entry from state before calling OpenCodeRuntime.reply_to_permission; instead
call reply_to_permission first (using state.runtime_pid,
state.opencode_session_id and the computed permission_id/answer_text), check its
result and only update state.pending_permissions to remaining and
emit_event("user-input/resolved", ...) after a successful RPC; also
propagate/handle errors from OpenCodeRuntime.reply_to_permission (and the
full-access auto-approve path) so failures do not mark the request resolved
locally and allow retry/cleanup.
- Around line 530-537: handle_call/3 currently truncates state.messages before
calling OpenCodeRuntime.revert_session/2 which can leave in-memory history
inconsistent if the remote revert fails; change the order to call
OpenCodeRuntime.revert_session(state.runtime_pid, state.opencode_session_id)
first (only when state.opencode_session_id, num_turns > 0 and state.runtime_pid
are present), pattern-match its result (e.g. {:ok, _} vs {:error, reason}), and
only on success update state by dropping the last num_turns messages (state =
%{state | messages: Enum.drop(state.messages, -num_turns)}); on failure keep the
original state, log the error and return an appropriate error reply without
mutating state.messages.
---
Duplicate comments:
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 699-713: The code in event_relevant?/2 builds event_session by
checking info.id before info.parentID, causing child-session creation events to
be associated with the child and dropped for parent subscribers; update the
construction of event_session in the event_relevant?/2 clause (the variable
event_session) to prefer get_in(data, ["info", "parentID"]) before get_in(data,
["info", "id"]) so parentID is used when both exist, and make the identical
change in the other occurrence of the same logic elsewhere in this module so
both places use parentID first while preserving the existing nil-or-equals
comparison to state.opencode_session_id.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d28733c5-1c6f-4704-ba31-16e2b97cd284
📒 Files selected for processing (2)
apps/harness/lib/harness/providers/opencode_runtime.exapps/harness/lib/harness/providers/opencode_session.ex
Comment on lines
+259
to
+263
| def handle_info(:setup, state) do | ||
| case wait_for_server(state.base_url, 45) do | ||
| :ok -> | ||
| sse_pid = start_sse_listener(state) | ||
| state = %{state | sse_pid: sse_pid, ready: true, health: :ready} |
There was a problem hiding this comment.
ready is set before the SSE stream is actually live.
start_sse_listener/1 only spawns the listener process; it does not confirm TCP connect or that the event stream is established. Releasing wait_for_ready/2 here lets sessions create/send work before SSE is available, which can drop the initial session.* / permission.* events.
Also applies to: 724-756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 259 -
263, The code sets the node state ready immediately after spawning the SSE
process (in handle_info(:setup)) but start_sse_listener/1 only spawns the
process and does not guarantee the SSE TCP connection is established; change the
flow so the state.ready and health=:ready are set only after the SSE listener
reports a successful connection (e.g., have start_sse_listener/1 return a
reference or change it to start_sse_listener_and_wait/1 that blocks until a
:sse_connected message or reply is received, or implement a short handshake
where the new SSE process sends the parent a {:sse_connected, pid} message), and
update the same pattern in the other occurrence (around the code referenced by
the reviewer at lines 724-756) to ensure wait_for_ready/2 is released only after
confirmed SSE connection.
Comment on lines
+322
to
+329
| def handle_info({:sse_down, reason}, state) do | ||
| schedule_sse_reconnect(state, reason) | ||
| end | ||
|
|
||
| # SSE process monitor DOWN | ||
| @impl true | ||
| def handle_info({:DOWN, _ref, :process, pid, reason}, %{sse_pid: pid} = state) do | ||
| schedule_sse_reconnect(state, reason) |
There was a problem hiding this comment.
One listener failure can schedule two reconnects.
The listener sends {:sse_down, reason} and is also monitored. If the :DOWN arrives first, schedule_sse_reconnect/2 clears sse_pid; the later {:sse_down, reason} still matches and schedules a second reconnect because it is not tied to a listener pid. That can start duplicate listeners and inflate the retry counter.
Also applies to: 352-355, 758-760, 791-799
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 322 -
329, The {:sse_down, reason} handler can schedule a reconnect twice because it
isn't tied to the monitored pid; change the clause to only act when the state
still contains an active sse_pid. Concretely, replace the catch-all def
handle_info({:sse_down, reason}, state) with a clause that matches on %{sse_pid:
pid} and guards is_pid(pid) (or otherwise verifies pid is present/alive) before
calling schedule_sse_reconnect(state, reason), and add a no-op fallback that
ignores {:sse_down, _} when sse_pid is nil; apply the same pattern to the other
similar handlers (the ones around the other referenced ranges) so
schedule_sse_reconnect/2 is only invoked when the current sse_pid matches an
active listener.
ranvier2d2
force-pushed
the
devin/1774836104-sprint2-shared-opencode-runtime
branch
from
March 30, 2026 16:13
6ea3bb3 to
b6b2908
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/harness/lib/harness/providers/opencode_session.ex (1)
392-429:⚠️ Potential issue | 🟠 MajorDon't clear local state until the runtime confirms the mutation.
Both permission-reply paths discard their only retry state before checking
OpenCodeRuntime.reply_to_permission/4, androllback_thread/2truncates in-memory history beforeOpenCodeRuntime.revert_session/2succeeds. A transient runtime error now leaves the wrapper and shared runtime out of sync; rollback is especially sticky becauseread_thread/2keeps serving the truncatedstate.messages.Also applies to: 530-539, 1095-1104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 392 - 429, The code is clearing local in-memory state (e.g., updating state.pending_permissions and truncating state.messages in rollback_thread) before confirming the external runtime mutation; change the flow so updates to state are applied only after OpenCodeRuntime calls succeed (OpenCodeRuntime.reply_to_permission/4 and OpenCodeRuntime.revert_session/2) or, if you must optimistically update, retain sufficient retry/undo info and revert the local state on failure. Specifically: in the permission-reply paths (where permission_id is computed and OpenCodeRuntime.reply_to_permission is called) move the state assignment that removes pending_permissions to after the reply succeeds (or wrap the reply in a try/rescue and restore pending_permissions on error); likewise, in rollback_thread adjust the order so state.messages truncation happens only after OpenCodeRuntime.revert_session/2 returns ok (or restore state.messages if revert fails). Ensure read_thread continues to serve consistent state until the runtime confirms mutations.
♻️ Duplicate comments (6)
apps/harness/lib/harness/providers/opencode_session.ex (1)
703-717:⚠️ Potential issue | 🔴 Critical
event_relevant?/2is treating genericdata.idvalues as session ids.That breaks several event shapes handled below:
permission.askedandquestion.askedusedata["id"]for their own object ids, not the session id, so this filter can drop valid per-session events wheneversessionIdis absent. It also still prefers childinfo.idoverinfo.parentIDforsession.created. Restrict the generic probe to actual session fields and special-case the parent/child session shape instead of treating everyidas a session identifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 703 - 717, The event_relevant?/2 function is incorrectly treating a generic data["id"] as the session identifier; remove the Map.get(data, "id") probe and only check explicit session fields ("sessionId" and "session_id"), and change the info-field handling to special-case the parent/child session shape by preferring get_in(data, ["info", "parentID"]) over get_in(data, ["info", "id"]) when deciding session association; update the logic in event_relevant?/2 so that generic object ids (data["id"]) are not used as session ids and only the explicit session fields or the info.parentID child-session shape are considered.apps/harness/lib/harness/providers/opencode_runtime.ex (5)
84-93:⚠️ Potential issue | 🔴 CriticalLease paths can still time out or leak during cold start.
Both
GenServer.call/3sites still use the 5s default whilehandle_info(:setup)can block for ~45s. If the new-runtime path times out afterRuntimeRegistry.register/2,ref_countis already1but no subscriber was recorded, so that runtime never reaches idle shutdown. Give these calls a startup-sized timeout and turn:noproc/timeout exits into a retry instead of crashing the caller.Also applies to: 1000-1006
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 84 - 93, The GenServer.call in do_lease_and_subscribe (the call to GenServer.call(pid, {:lease_and_subscribe, key, thread_id, wrapper_pid})) uses the default 5s which can time out while handle_info(:setup) blocks ~45s; change this call (and the duplicate sites around the 1000–1006 region) to use a startup-sized timeout (e.g., ~60_000 ms) and wrap the call so that :noproc and :timeout exits/errors are handled by retrying (decrementing attempts_left and recursing) instead of letting the caller crash; ensure behavior is consistent with RuntimeRegistry.register/2 and ref_count semantics so a failed call won’t leave ref_count=1 with no subscriber.
430-461:⚠️ Potential issue | 🟠 MajorReplace stale subscriber pids instead of keying duplicates only by
thread_id.
thread_idalone is not a safe duplicate key. If a wrapper restarts under the same thread before the old:DOWNis processed, this returns:okwithout replacing the stale pid or refreshing the monitor. The replacement wrapper then receives no SSE fanout, and the late:DOWNdecrements the live lease.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 430 - 461, The current handle_call clauses for {:lease_and_subscribe, key, thread_id, wrapper_pid} and {:subscribe_initial, thread_id, wrapper_pid} only check Map.has_key?(state.subscribers, thread_id) so a restarted wrapper with the same thread_id will be ignored and its pid/monitor not refreshed; change both handlers to detect an existing pid via Map.get(state.subscribers, thread_id) and if it differs from wrapper_pid, demonitor the old pid, monitor the new wrapper_pid, and replace the entry in state.subscribers (preserving the ref-count behavior for lease_and_subscribe where you call RuntimeRegistry.increment_ref/1 and for subscribe_initial where you must not increment), then call cancel_idle_timer(state) and reply :ok; also ensure the late :DOWN from the old pid will not incorrectly decrement the live lease by only replacing the pid and monitor rather than skipping when a key exists.
322-329:⚠️ Potential issue | 🟠 MajorOne SSE failure can multiply listeners and health-check timers.
sse_loop/2reports failure via{:sse_down, reason}and then exits, so the monitor also emits:DOWN. Because both paths schedule reconnects and everystart_sse_listener/1call queues a fresh:health_checkchain, one disconnect can fan out into parallel listeners plus redundant health polling.Also applies to: 724-736, 758-799
259-263:⚠️ Potential issue | 🟠 Major
readyflips true before the SSE stream is actually established.
wait_for_server/2only proves the HTTP health endpoint is up.start_sse_listener/1returns beforesse_loop/2finishes the TCP connect, but the runtime is marked ready and waiters are released immediately. A newly created session can start work before event delivery exists and miss its firstsession.*/permission.*events.Also applies to: 724-756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 259 - 263, The code marks the runtime ready immediately after start_sse_listener/1 returns even though sse_loop/2 may not have established the TCP/SSE connection; change the flow so readiness and releasing waiters happen only after the SSE stream is actually connected: modify start_sse_listener/1 (or add a new sync helper like establish_sse_connection/1) to perform or wait for the TCP connect and then send a confirmation message (e.g. :sse_connected) back to the caller or return only after connection is established, and update handle_info(:setup, state) to wait for that confirmation (or pattern-match the returned connected result) before setting state.ready = true and state.health = :ready; reference start_sse_listener/1, sse_loop/2 and handle_info(:setup, state) when implementing the synchronous connection/confirmation handshake.
259-276:⚠️ Potential issue | 🟠 MajorReply to queued
wait_for_ready/2callers before stopping.On startup timeout or port exit, the GenServer stops without draining
ready_waiters, soOpenCodeRuntime.wait_for_ready/2exits and bypasses its advertised{:error, reason}contract. Flush that queue with an error reply on every stop path.Also applies to: 282-296, 407-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 259 - 276, The GenServer stop paths (e.g., handle_info(:setup) timeout branch and the other stop/port-exit branches referenced around lines 282-296 and 407-409) never reply to queued callers in state.ready_waiters, causing OpenCodeRuntime.wait_for_ready/2 callers to crash instead of receiving {:error, reason}; fix by iterating state.ready_waiters and replying each waiter with GenServer.reply(waiter, {:error, reason}) before returning {:stop, reason, state} (and then clear ready_waiters from state or set it to []); ensure this pattern is applied in handle_info(:setup) timeout, the port-exit/stop handlers, and any terminate/stop paths so every waiter gets a consistent {:error, reason} response.
🤖 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/lib/harness/opencode/runtime_registry.ex`:
- Around line 103-105: The ETS table is created in init/1 with :ets.new(`@table`,
[:named_table, :set, :public, read_concurrency: true]); change the options list
to use :protected instead of :public so the GenServer remains the sole writer.
Update the call in init/1 (referencing `@table` and the local variable table) to
:ets.new(`@table`, [:named_table, :set, :protected, read_concurrency: true]) and
keep the rest of the return value {:ok, %{table: table, monitors: %{}}}
unchanged.
- Around line 29-39: lookup/1, ref_count/1, and list_all/0 in
Harness.OpenCode.RuntimeRegistry currently call :ets.lookup/2 and
:ets.tab2list/1 directly and crash with :badarg if the ETS table (`@table`) is
missing; guard these calls by calling :ets.whereis(`@table`) first and if it
returns :undefined return safe defaults (:error for lookup/1, 0 for ref_count/1,
and [] for list_all/0). Update the functions (referencing
RuntimeKey.to_string/1, lookup/1, ref_count/1, and list_all/0) to check
:ets.whereis(`@table`) and only call :ets.lookup or :ets.tab2list when the table
exists, otherwise return the appropriate fallback values.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 217-224: The handler for {:runtime_sse_degraded, _pid} currently
only emits "session/degraded" and leaves the active turn in state.turn_state,
which can leave an in-flight turn stuck; update handle_info/2 so that after
emitting session/degraded it also fails or closes any active turn (e.g., if
state.turn_state indicates a running turn) by invoking the existing turn-fail or
transition helper (or implement a small helper like fail_active_turn/1) to set
the turn state to failed/complete and emit the appropriate turn event, or
alternatively kick off the polling fallback before returning {:noreply,
new_state} so the turn cannot remain stuck. Ensure you modify handle_info/2 to
return the updated state reflecting the failed/closed turn.
---
Outside diff comments:
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 392-429: The code is clearing local in-memory state (e.g.,
updating state.pending_permissions and truncating state.messages in
rollback_thread) before confirming the external runtime mutation; change the
flow so updates to state are applied only after OpenCodeRuntime calls succeed
(OpenCodeRuntime.reply_to_permission/4 and OpenCodeRuntime.revert_session/2) or,
if you must optimistically update, retain sufficient retry/undo info and revert
the local state on failure. Specifically: in the permission-reply paths (where
permission_id is computed and OpenCodeRuntime.reply_to_permission is called)
move the state assignment that removes pending_permissions to after the reply
succeeds (or wrap the reply in a try/rescue and restore pending_permissions on
error); likewise, in rollback_thread adjust the order so state.messages
truncation happens only after OpenCodeRuntime.revert_session/2 returns ok (or
restore state.messages if revert fails). Ensure read_thread continues to serve
consistent state until the runtime confirms mutations.
---
Duplicate comments:
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 84-93: The GenServer.call in do_lease_and_subscribe (the call to
GenServer.call(pid, {:lease_and_subscribe, key, thread_id, wrapper_pid})) uses
the default 5s which can time out while handle_info(:setup) blocks ~45s; change
this call (and the duplicate sites around the 1000–1006 region) to use a
startup-sized timeout (e.g., ~60_000 ms) and wrap the call so that :noproc and
:timeout exits/errors are handled by retrying (decrementing attempts_left and
recursing) instead of letting the caller crash; ensure behavior is consistent
with RuntimeRegistry.register/2 and ref_count semantics so a failed call won’t
leave ref_count=1 with no subscriber.
- Around line 430-461: The current handle_call clauses for
{:lease_and_subscribe, key, thread_id, wrapper_pid} and {:subscribe_initial,
thread_id, wrapper_pid} only check Map.has_key?(state.subscribers, thread_id) so
a restarted wrapper with the same thread_id will be ignored and its pid/monitor
not refreshed; change both handlers to detect an existing pid via
Map.get(state.subscribers, thread_id) and if it differs from wrapper_pid,
demonitor the old pid, monitor the new wrapper_pid, and replace the entry in
state.subscribers (preserving the ref-count behavior for lease_and_subscribe
where you call RuntimeRegistry.increment_ref/1 and for subscribe_initial where
you must not increment), then call cancel_idle_timer(state) and reply :ok; also
ensure the late :DOWN from the old pid will not incorrectly decrement the live
lease by only replacing the pid and monitor rather than skipping when a key
exists.
- Around line 259-263: The code marks the runtime ready immediately after
start_sse_listener/1 returns even though sse_loop/2 may not have established the
TCP/SSE connection; change the flow so readiness and releasing waiters happen
only after the SSE stream is actually connected: modify start_sse_listener/1 (or
add a new sync helper like establish_sse_connection/1) to perform or wait for
the TCP connect and then send a confirmation message (e.g. :sse_connected) back
to the caller or return only after connection is established, and update
handle_info(:setup, state) to wait for that confirmation (or pattern-match the
returned connected result) before setting state.ready = true and state.health =
:ready; reference start_sse_listener/1, sse_loop/2 and handle_info(:setup,
state) when implementing the synchronous connection/confirmation handshake.
- Around line 259-276: The GenServer stop paths (e.g., handle_info(:setup)
timeout branch and the other stop/port-exit branches referenced around lines
282-296 and 407-409) never reply to queued callers in state.ready_waiters,
causing OpenCodeRuntime.wait_for_ready/2 callers to crash instead of receiving
{:error, reason}; fix by iterating state.ready_waiters and replying each waiter
with GenServer.reply(waiter, {:error, reason}) before returning {:stop, reason,
state} (and then clear ready_waiters from state or set it to []); ensure this
pattern is applied in handle_info(:setup) timeout, the port-exit/stop handlers,
and any terminate/stop paths so every waiter gets a consistent {:error, reason}
response.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 703-717: The event_relevant?/2 function is incorrectly treating a
generic data["id"] as the session identifier; remove the Map.get(data, "id")
probe and only check explicit session fields ("sessionId" and "session_id"), and
change the info-field handling to special-case the parent/child session shape by
preferring get_in(data, ["info", "parentID"]) over get_in(data, ["info", "id"])
when deciding session association; update the logic in event_relevant?/2 so that
generic object ids (data["id"]) are not used as session ids and only the
explicit session fields or the info.parentID child-session shape are considered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1381a40-0251-44c9-8bfd-b5ba6f396996
📒 Files selected for processing (9)
apps/harness/lib/harness/application.exapps/harness/lib/harness/opencode/runtime_key.exapps/harness/lib/harness/opencode/runtime_registry.exapps/harness/lib/harness/providers/opencode_runtime.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/test/harness/opencode/runtime_key_test.exsapps/harness/test/harness/opencode/runtime_registry_test.exsapps/harness/test/harness/providers/opencode_runtime_test.exsapps/harness/test/harness/providers/opencode_session_test.exs
✅ Files skipped from review due to trivial changes (4)
- apps/harness/test/harness/providers/opencode_runtime_test.exs
- apps/harness/test/harness/opencode/runtime_registry_test.exs
- apps/harness/test/harness/opencode/runtime_key_test.exs
- apps/harness/lib/harness/opencode/runtime_key.ex
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/harness/lib/harness/application.ex
- apps/harness/test/harness/providers/opencode_session_test.exs
Comment on lines
+29
to
+39
| def lookup(%RuntimeKey{} = key) do | ||
| str_key = RuntimeKey.to_string(key) | ||
|
|
||
| case :ets.lookup(@table, str_key) do | ||
| [{^str_key, pid, _ref_count}] when is_pid(pid) -> | ||
| if Process.alive?(pid), do: {:ok, pid}, else: :error | ||
|
|
||
| _ -> | ||
| :error | ||
| end | ||
| end |
There was a problem hiding this comment.
Guard direct ETS reads when the registry table is unavailable.
lookup/1, ref_count/1, and list_all/0 call :ets.lookup/2 and :ets.tab2list/1 directly without checking table existence. If Harness.OpenCode.RuntimeRegistry restarts while these functions are called, they crash with :badarg instead of returning safe defaults. Because Harness.SessionSupervisor starts before the registry in the :rest_for_one supervision tree, session startups during a restart window will hit this failure. Use :ets.whereis(@table) to check table existence first and fall back to :error, 0, and [] respectively.
Also applies to: 83-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/opencode/runtime_registry.ex` around lines 29 - 39,
lookup/1, ref_count/1, and list_all/0 in Harness.OpenCode.RuntimeRegistry
currently call :ets.lookup/2 and :ets.tab2list/1 directly and crash with :badarg
if the ETS table (`@table`) is missing; guard these calls by calling
:ets.whereis(`@table`) first and if it returns :undefined return safe defaults
(:error for lookup/1, 0 for ref_count/1, and [] for list_all/0). Update the
functions (referencing RuntimeKey.to_string/1, lookup/1, ref_count/1, and
list_all/0) to check :ets.whereis(`@table`) and only call :ets.lookup or
:ets.tab2list when the table exists, otherwise return the appropriate fallback
values.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/harness/lib/harness/providers/opencode_session.ex (2)
533-540:⚠️ Potential issue | 🟠 MajorKeep local rollback state in sync with the remote revert.
The local message list is truncated before
OpenCodeRuntime.revert_session/2is checked. If the remote revert fails, the harness and the OpenCode session immediately diverge, and a later hydrate/read will resurrect turns the user already “rolled back”.
391-425:⚠️ Potential issue | 🟠 MajorDon't resolve prompts locally when the reply never reached OpenCode.
respond_to_user_input/3drops the pending request before it knows the answer was accepted, and the full-access path ignores reply failures entirely. A transient runtime error here loses the prompt locally while the remote turn keeps waiting.Also applies to: 1098-1107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 391 - 425, The code currently pops the pending request from state.pending_permissions before calling OpenCodeRuntime.reply_to_permission (in the Map.pop block handling pending permissions), which loses the pending entry if the remote reply fails; instead, call OpenCodeRuntime.reply_to_permission first and only remove the pending entry on a successful reply, or if you must pop first, reinsert the pending entry on any error by Map.put(state, :pending_permissions, Map.put(state.pending_permissions, request_id, pending)); also update the full-access path (the similar logic around respond_to_user_input/3) to check the reply result (match {:ok, _} vs {:error, _}) and handle failures by restoring the pending entry and returning an error/appropriate reply rather than ignoring failures. Ensure you reference the pending variable, permission_id, OpenCodeRuntime.reply_to_permission, and the state.pending_permissions map when making the change.
♻️ Duplicate comments (3)
apps/harness/lib/harness/providers/opencode_runtime.ex (2)
269-273:⚠️ Potential issue | 🟠 MajorDon't release
wait_for_ready/2before the SSE stream is established.The ready flag is set immediately after spawning the listener, but
start_sse_listener/1only returns a pid; it does not confirm the TCP connect or the event-stream handshake. That breaks the documented “server up + SSE connected” contract and lets the first session events race past the wrapper.Also applies to: 756-788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 269 - 273, The handler is marking the runtime ready before the SSE connection is actually established; change the logic so that start_sse_listener/1 returns confirmation of a successful SSE handshake (e.g., {:ok, pid} or {:connected, pid}) or expose a wait_for_ready/2 that blocks until the SSE listener reports connected, and only then update state with sse_pid, ready: true, and health: :ready; update handle_info(:setup) to wait on that confirmation instead of trusting a raw pid, and adjust start_sse_listener/1 (or add a helper) to perform/connect+handshake signalling so the ready flag truly means “server up + SSE connected.”
336-343:⚠️ Potential issue | 🟠 MajorOne SSE failure can still create duplicate reconnect and health-check loops.
The listener sends
{:sse_down, reason}and is also monitored, so a single exit can hitschedule_sse_reconnect/2twice. Each reconnect then callsstart_sse_listener/1, which schedules another:health_checkchain. Tie the down message to the current listener pid and schedule health checks once.Also applies to: 366-369, 756-769, 790-793, 823-827
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 336 - 343, The SSE down message can double-trigger reconnection because the listener sends {:sse_down, reason} and the monitor also sends {:DOWN,...}; change the protocol so the listener sends its pid (e.g. {:sse_down, listener_pid, reason}) and update the handle_info clause to match {:sse_down, pid, reason} and only call schedule_sse_reconnect(state, reason) when pid == state.sse_pid; this ties the down message to the current listener, prevents duplicate calls that each invoke start_sse_listener/1 (and re-schedule :health_check chains), and apply the same pid-guarded pattern to the other similar handle_info blocks referenced (around the other ranges).apps/harness/lib/harness/opencode/runtime_registry.ex (1)
29-39:⚠️ Potential issue | 🟠 MajorGuard the ETS read helpers during registry restarts.
lookup/1,ref_count/1, andlist_all/0still hit the named table directly. IfHarness.OpenCode.RuntimeRegistryis restarting,:ets.lookup/2/:ets.tab2list/1raise:badarginstead of returning the documented fallbacks.Also applies to: 83-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/opencode/runtime_registry.ex` around lines 29 - 39, Guard calls to the ETS table by checking :ets.info(`@table`) (or using a try/rescue) before invoking :ets.lookup or :ets.tab2list so you don't crash during registry restarts; specifically, update lookup/1, ref_count/1 and list_all/0 to return the safe fallbacks (:error for lookup/ref_count and [] for list_all) when :ets.info(`@table`) == :undefined (or when a :badarg would otherwise occur), and only call :ets.lookup(`@table`, str_key) or :ets.tab2list(`@table`) when the table exists; reference the functions lookup/1, ref_count/1, list_all/0 and the module attribute `@table` to find where to add the guard.
🤖 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/lib/harness/opencode/runtime_registry.ex`:
- Around line 29-32: The current lookup (and the insert/update/delete code paths
around the same file where RuntimeKey.to_string is used) uses a pipe-joined
string which can collide; change the ETS key to be the RuntimeKey term (or a
lossless binary like :erlang.term_to_binary(RuntimeKey)) instead of the
flattened string. Update the functions that reference
RuntimeKey.to_string—specifically lookup/1 and the registry insert/update/remove
paths (the code around lines using `@table` with RuntimeKey.to_string) to use the
RuntimeKey struct (or term_to_binary) as the ETS key consistently so lookups,
inserts, and ref-count updates use a non-lossy key.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 566-570: The HTTP helper functions http_get/1 and http_post/2 are
crashing when Req auto-decodes JSON arrays because they assume the response body
is a map and call Jason.decode/1; update their guards/conditions to also accept
lists (add "or is_list(body)" wherever the helpers currently check for a map
body) so pre-decoded JSON arrays are returned as-is to callers like
fetch_messages/2 and fetch_providers/1 which already expect lists; ensure the
helpers return {:ok, body} for list bodies without attempting to re-decode them.
- Around line 269-288: The GenServer.call in subscribe_initial can exit the
caller if the server isn't ready; wrap the call in a try/catch that catches
:exit and returns an error tuple so lease_and_subscribe can retry instead of
crashing: in subscribe_initial replace the direct GenServer.call with a guarded
call (try GenServer.call(...) catch :exit, reason -> {:error, {:genserver_exit,
reason}} end) and branch the logic to return {:error, :server_unavailable} (or
mapped reason) so existing retry paths are used; apply the same protective
wrapper to the analogous call at the other location (lines noted 1032-1038) so
both code paths convert exits into error tuples rather than letting the caller
exit.
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 701-726: event_relevant?/2 currently drops events that only have a
"type" (like %{"type" => "session.idle"}) because only server.heartbeat and
server.connected are accepted; update the pattern matching so "session.idle" is
treated as relevant (either add "session.idle" to the when type in [...] list or
add a new defp event_relevant?(%{"type" => "session.idle"}, _state) do true end
clause), and mirror the same change for the similar handler around the other
occurrence (the block noted at lines ~950-953) so session.idle can complete the
turn.
---
Outside diff comments:
In `@apps/harness/lib/harness/providers/opencode_session.ex`:
- Around line 391-425: The code currently pops the pending request from
state.pending_permissions before calling OpenCodeRuntime.reply_to_permission (in
the Map.pop block handling pending permissions), which loses the pending entry
if the remote reply fails; instead, call OpenCodeRuntime.reply_to_permission
first and only remove the pending entry on a successful reply, or if you must
pop first, reinsert the pending entry on any error by Map.put(state,
:pending_permissions, Map.put(state.pending_permissions, request_id, pending));
also update the full-access path (the similar logic around
respond_to_user_input/3) to check the reply result (match {:ok, _} vs {:error,
_}) and handle failures by restoring the pending entry and returning an
error/appropriate reply rather than ignoring failures. Ensure you reference the
pending variable, permission_id, OpenCodeRuntime.reply_to_permission, and the
state.pending_permissions map when making the change.
---
Duplicate comments:
In `@apps/harness/lib/harness/opencode/runtime_registry.ex`:
- Around line 29-39: Guard calls to the ETS table by checking :ets.info(`@table`)
(or using a try/rescue) before invoking :ets.lookup or :ets.tab2list so you
don't crash during registry restarts; specifically, update lookup/1, ref_count/1
and list_all/0 to return the safe fallbacks (:error for lookup/ref_count and []
for list_all) when :ets.info(`@table`) == :undefined (or when a :badarg would
otherwise occur), and only call :ets.lookup(`@table`, str_key) or
:ets.tab2list(`@table`) when the table exists; reference the functions lookup/1,
ref_count/1, list_all/0 and the module attribute `@table` to find where to add the
guard.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex`:
- Around line 269-273: The handler is marking the runtime ready before the SSE
connection is actually established; change the logic so that
start_sse_listener/1 returns confirmation of a successful SSE handshake (e.g.,
{:ok, pid} or {:connected, pid}) or expose a wait_for_ready/2 that blocks until
the SSE listener reports connected, and only then update state with sse_pid,
ready: true, and health: :ready; update handle_info(:setup) to wait on that
confirmation instead of trusting a raw pid, and adjust start_sse_listener/1 (or
add a helper) to perform/connect+handshake signalling so the ready flag truly
means “server up + SSE connected.”
- Around line 336-343: The SSE down message can double-trigger reconnection
because the listener sends {:sse_down, reason} and the monitor also sends
{:DOWN,...}; change the protocol so the listener sends its pid (e.g. {:sse_down,
listener_pid, reason}) and update the handle_info clause to match {:sse_down,
pid, reason} and only call schedule_sse_reconnect(state, reason) when pid ==
state.sse_pid; this ties the down message to the current listener, prevents
duplicate calls that each invoke start_sse_listener/1 (and re-schedule
:health_check chains), and apply the same pid-guarded pattern to the other
similar handle_info blocks referenced (around the other ranges).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: edf6040b-e154-46bd-98bc-88c4a28f6526
📒 Files selected for processing (3)
apps/harness/lib/harness/opencode/runtime_registry.exapps/harness/lib/harness/providers/opencode_runtime.exapps/harness/lib/harness/providers/opencode_session.ex
Comment on lines
+29
to
+32
| def lookup(%RuntimeKey{} = key) do | ||
| str_key = RuntimeKey.to_string(key) | ||
|
|
||
| case :ets.lookup(@table, str_key) do |
There was a problem hiding this comment.
The registry key serialization is still collision-prone.
apps/harness/lib/harness/opencode/runtime_key.ex:47-61 currently flattens the four key fields with a plain pipe join. Because cwd, binary_path, and config_path can legally contain |, two different RuntimeKey structs can land in the same ETS row here and share the wrong runtime/ref-count entry. Use the struct itself, or another non-lossy binary form, as the ETS key.
Also applies to: 109-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/opencode/runtime_registry.ex` around lines 29 - 32,
The current lookup (and the insert/update/delete code paths around the same file
where RuntimeKey.to_string is used) uses a pipe-joined string which can collide;
change the ETS key to be the RuntimeKey term (or a lossless binary like
:erlang.term_to_binary(RuntimeKey)) instead of the flattened string. Update the
functions that reference RuntimeKey.to_string—specifically lookup/1 and the
registry insert/update/remove paths (the code around lines using `@table` with
RuntimeKey.to_string) to use the RuntimeKey struct (or term_to_binary) as the
ETS key consistently so lookups, inserts, and ref-count updates use a non-lossy
key.
Comment on lines
+269
to
+288
| def handle_info(:setup, state) do | ||
| case wait_for_server(state.base_url, 45) do | ||
| :ok -> | ||
| sse_pid = start_sse_listener(state) | ||
| state = %{state | sse_pid: sse_pid, ready: true, health: :ready} | ||
|
|
||
| Logger.info( | ||
| "OpenCode runtime ready on port #{state.opencode_port} " <> | ||
| "(key: #{RuntimeKey.to_string(state.runtime_key)})" | ||
| ) | ||
|
|
||
| Enum.each(state.ready_waiters, &GenServer.reply(&1, :ok)) | ||
| state = %{state | ready_waiters: []} | ||
| {:noreply, state} | ||
|
|
||
| {:error, :timeout} -> | ||
| Logger.error("OpenCode runtime failed to start on port #{state.opencode_port}") | ||
| Enum.each(state.ready_waiters, &GenServer.reply(&1, {:error, :server_timeout})) | ||
| {:stop, :server_timeout, %{state | ready_waiters: []}} | ||
| end |
There was a problem hiding this comment.
subscribe_initial can exit the caller instead of returning an error tuple.
This GenServer.call/3 is made before the runtime has finished its blocking :setup work. If startup times out or the port dies first, the call exits, so lease_and_subscribe/4 bypasses its retry/error path and can crash the wrapper during setup.
Also applies to: 1032-1038
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 269 -
288, The GenServer.call in subscribe_initial can exit the caller if the server
isn't ready; wrap the call in a try/catch that catches :exit and returns an
error tuple so lease_and_subscribe can retry instead of crashing: in
subscribe_initial replace the direct GenServer.call with a guarded call (try
GenServer.call(...) catch :exit, reason -> {:error, {:genserver_exit, reason}}
end) and branch the logic to return {:error, :server_unavailable} (or mapped
reason) so existing retry paths are used; apply the same protective wrapper to
the analogous call at the other location (lines noted 1032-1038) so both code
paths convert exits into error tuples rather than letting the caller exit.
Comment on lines
+566
to
+570
| def handle_call({:fetch_messages, session_id}, _from, state) do | ||
| case http_get("#{state.base_url}/session/#{session_id}/message") do | ||
| {:ok, %{"messages" => messages}} when is_list(messages) -> {:reply, {:ok, messages}, state} | ||
| {:ok, messages} when is_list(messages) -> {:reply, {:ok, messages}, state} | ||
| {:ok, _} -> {:reply, {:ok, []}, state} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file size and structure
wc -l apps/harness/lib/harness/providers/opencode_runtime.exRepository: Ranvier-Technologies/t3code-OTP
Length of output: 134
🏁 Script executed:
# Let's find the http_get and http_post implementations
rg -n "defp http_get|defp http_post" apps/harness/lib/harness/providers/opencode_runtime.ex -A 10Repository: Ranvier-Technologies/t3code-OTP
Length of output: 993
🏁 Script executed:
# Let's also check the fetch_messages and fetch_providers functions to understand their expectations
rg -n "def fetch_messages|def fetch_providers" apps/harness/lib/harness/providers/opencode_runtime.ex -A 5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 492
🏁 Script executed:
# Let's check the handle_call implementations for fetch_messages and fetch_providers
rg -n "handle_call.*fetch_messages|handle_call.*fetch_providers" apps/harness/lib/harness/providers/opencode_runtime.ex -A 10Repository: Ranvier-Technologies/t3code-OTP
Length of output: 1114
🏁 Script executed:
# Let's also check what the actual API responses look like by looking at all handle_call implementations
sed -n '566,570p' apps/harness/lib/harness/providers/opencode_runtime.ex
sed -n '621,625p' apps/harness/lib/harness/providers/opencode_runtime.exRepository: Ranvier-Technologies/t3code-OTP
Length of output: 765
🏁 Script executed:
# Let's check the other locations mentioned: 979-982, 1007-1013
sed -n '979,989p' apps/harness/lib/harness/providers/opencode_runtime.ex
sed -n '1005,1020p' apps/harness/lib/harness/providers/opencode_runtime.exRepository: Ranvier-Technologies/t3code-OTP
Length of output: 1037
Accept decoded JSON arrays in the HTTP helpers.
http_get/1 and http_post/2 currently only handle map bodies and will crash when Req auto-decodes JSON arrays to Elixir lists. The callers (fetch_messages/2 and fetch_providers/1) already expect to receive lists directly (see pattern matches on lines 569, 624), but the helpers attempt to call Jason.decode(to_string(body)) on lists, producing invalid JSON and crashing the runtime.
Add or is_list(body) to both helpers to accept pre-decoded lists:
Suggested patch
defp http_get(url) do
case Req.get(url, receive_timeout: 10_000) do
{:ok, %{status: status, body: body}} when status in 200..204 ->
- if is_map(body), do: {:ok, body}, else: Jason.decode(to_string(body))
+ cond do
+ is_map(body) or is_list(body) -> {:ok, body}
+ true -> Jason.decode(to_string(body))
+ end
@@
defp http_post(url, body) do
case Req.post(url, json: body, receive_timeout: 30_000) do
{:ok, %{status: status, body: body}} when status in 200..204 ->
- if is_map(body) do
+ if is_map(body) or is_list(body) do
{:ok, body}
else
body_str = to_string(body || "")
if body_str == "", do: {:ok, %{}}, else: Jason.decode(body_str)
endAlso applies to: 621-625, 979-982, 1007-1013
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_runtime.ex` around lines 566 -
570, The HTTP helper functions http_get/1 and http_post/2 are crashing when Req
auto-decodes JSON arrays because they assume the response body is a map and call
Jason.decode/1; update their guards/conditions to also accept lists (add "or
is_list(body)" wherever the helpers currently check for a map body) so
pre-decoded JSON arrays are returned as-is to callers like fetch_messages/2 and
fetch_providers/1 which already expect lists; ensure the helpers return {:ok,
body} for list bodies without attempting to re-decode them.
Comment on lines
+701
to
726
| defp event_relevant?(%{"type" => type}, _state) | ||
| when type in ["server.heartbeat", "server.connected"] do | ||
| true | ||
| end | ||
|
|
||
| event_type = | ||
| Enum.find_value(lines, fn | ||
| "event: " <> type -> String.trim(type) | ||
| _ -> nil | ||
| end) | ||
| defp event_relevant?(%{"data" => data}, state) when is_map(data) do | ||
| session_id = state.opencode_session_id | ||
|
|
||
| data = | ||
| Enum.find_value(lines, fn | ||
| "data: " <> json -> String.trim(json) | ||
| _ -> nil | ||
| end) | ||
| # Check explicit session association fields only — generic data["id"] | ||
| # is an object identifier (message, item), not a session identifier. | ||
| event_session = | ||
| Map.get(data, "sessionId") || | ||
| Map.get(data, "session_id") || | ||
| get_in(data, ["info", "parentID"]) || | ||
| get_in(data, ["info", "id"]) | ||
|
|
||
| if event_type && data do | ||
| case Jason.decode(data) do | ||
| {:ok, parsed} -> [%{"type" => event_type, "data" => parsed}] | ||
| _ -> [] | ||
| end | ||
| else | ||
| [] | ||
| end | ||
| end) | ||
| # If the event has a session identifier, check it matches ours. | ||
| # If no session identifier found, pass through (could be a message-level event | ||
| # that's contextually bound to the active session). | ||
| is_nil(event_session) or event_session == session_id | ||
| end | ||
|
|
||
| {events, remaining} | ||
| end | ||
| defp event_relevant?(event, _state) do | ||
| Logger.debug("Dropping unrecognized SSE event shape: #{inspect(Map.get(event, "type", "unknown"), limit: 100)}") | ||
| false | ||
| end |
There was a problem hiding this comment.
session.idle is filtered out before it can complete the turn.
event_relevant?/2 only allows data-less server.* events. %{"type" => "session.idle"} falls through to the catch-all false clause, so the handler below is dead code. If OpenCode emits session.idle as the completion signal, the turn never closes.
Also applies to: 950-953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/harness/lib/harness/providers/opencode_session.ex` around lines 701 -
726, event_relevant?/2 currently drops events that only have a "type" (like
%{"type" => "session.idle"}) because only server.heartbeat and server.connected
are accepted; update the pattern matching so "session.idle" is treated as
relevant (either add "session.idle" to the when type in [...] list or add a new
defp event_relevant?(%{"type" => "session.idle"}, _state) do true end clause),
and mirror the same change for the similar handler around the other occurrence
(the block noted at lines ~950-953) so session.idle can complete the turn.
- Add RuntimeKey module for deterministic runtime identification - Add RuntimeRegistry (ETS-backed GenServer) for runtime tracking with ref counting - Add OpenCodeRuntime GenServer owning opencode serve process, SSE, HTTP proxy - Refactor OpenCodeSession into thin thread wrapper that leases shared runtime - Update application.ex supervision tree with RuntimeRegistry + RuntimeSupervisor - Route all MCP/session/prompt operations through shared runtime - Add idle TTL shutdown (5 min) when zero threads are leased - Add SSE event fanout from runtime to thread wrappers with session filtering - Add comprehensive tests for RuntimeKey, RuntimeRegistry, OpenCodeRuntime, OpenCodeSession Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
B1+B3: Atomic lease_and_subscribe/4 eliminates the gap between lease
(ref count increment) and subscribe (monitor + subscriber map). No more
leaked ref counts if a session dies before subscribing.
B2: Event filtering catch-all changed from true to false — unknown events
no longer leak across all thread wrappers on a shared runtime.
Double-decrement: release/2 no longer decrements ref count directly.
The unsubscribe handler and DOWN handler are now the only paths that
decrement, and both use Map.pop to ensure idempotent single-decrement.
R1: MCP config hash uses canonical_term + term_to_binary instead of
Jason.encode! for deterministic hashing regardless of map key order.
R2: SSE reconnect uses exponential backoff (1s → 60s cap, 1.5x growth,
10% jitter) with 30 max retries. After exhaustion, runtime enters
degraded state and notifies subscribers.
R3: Registration race retries up to 3 times with structured error
tuples {:error, {:registration_race, key}} and {:error, {:lease_failed,
:retries_exhausted}}.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…handler - Guard against double subscription: lease_and_subscribe and subscribe_initial check if thread_id is already in subscribers before creating a new monitor, preventing ref count leak from duplicate monitors (#5). - Handle :runtime_sse_degraded in session: emit session/degraded event when runtime SSE reconnect is exhausted, instead of silently dropping the message (#8). - Add Logger.debug to event_relevant? catch-all so unrecognized SSE event shapes are logged instead of silently dropped (#10). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. ETS :public → :protected (registry.ex): GenServer is sole writer,
:protected suffices for concurrent reads from other processes.
2. Fail active turn on SSE degraded (session.ex): call
maybe_complete_turn(state, "failed") so in-flight turns don't hang
when SSE reconnect is exhausted.
3. 60s timeout for lease GenServer.call (runtime.ex): the runtime's
handle_info(:setup) blocks up to 45s in wait_for_server, so the
default 5s timeout caused spurious crashes. Also catch :noproc and
:timeout exits to retry instead of crashing.
4. Handle wrapper PID change on resubscribe (runtime.ex): if a wrapper
restarts with the same thread_id but different PID, demonitor old
and monitor new instead of silently ignoring. Prevents stale
monitors and late DOWN decrements.
5. Reply to ready_waiters on stop paths (runtime.ex): timeout and
port-exit handlers now reply {:error, reason} to queued
wait_for_ready callers before stopping.
6. Remove data["id"] from session ID probe (session.ex): generic object
IDs shouldn't be treated as session identifiers — only check
explicit sessionId/session_id fields and info.parentID/info.id.
Declined (YAGNI):
- ETS whereis guard: table is created by supervised GenServer before
consumers; crash on missing table is correct.
- Optimistic state in permission reply: fire-and-forget through runtime;
session dies on runtime DOWN anyway.
- SSE readiness gating: SSE is supplementary; blocking ready on TCP
connect delays session setup for a non-critical feature.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ranvier2d2
force-pushed
the
devin/1774836104-sprint2-shared-opencode-runtime
branch
from
March 30, 2026 17:48
6a50823 to
459c399
Compare
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.
What Changed
Moves OpenCode from a thread-owned process model (each thread spawns its own
opencode serve) to a shared runtime + thread-owned session wrapper model. Threads whose configuration matches (same cwd, binary, config path, MCP config) now share a singleopencode serveOS process.New modules:
Harness.OpenCode.RuntimeKey— Builds a normalized, comparable key from session params (cwd, binary path, config path, MCP config hash)Harness.OpenCode.RuntimeRegistry— ETS-backed GenServer tracking runtime PIDs by key with reference countingHarness.Providers.OpenCodeRuntime(~945 lines) — GenServer owning theopencode serveOS process, SSE listener, HTTP proxy, subscriber management, and idle TTL shutdownRefactored:
OpenCodeSession— Stripped of process ownership (port,opencode_port,sse_pid,base_url,binary_path). Now leases a shared runtime viaRuntimeKey→RuntimeRegistry→OpenCodeRuntime, and delegates all HTTP/MCP operations through the runtime. SSE events are received via fanout from the runtime and filtered byevent_relevant?/2.Supervision tree (
application.ex):Harness.OpenCode.RuntimeRegistry(singleton GenServer)Harness.RuntimeSupervisor(DynamicSupervisor for runtime processes)Why
Multiple OpenCode threads pointing at the same project were each spawning a separate
opencode serveprocess — wasteful in resources and causing port conflicts. A shared runtime allows N threads to multiplex over one OS process, with proper ref-counted lifecycle management and idle cleanup.Checklist
I included before/after screenshots for any UI changes(no UI changes)Reviewer Checklist — things worth extra scrutiny
hash_mcp_configdeterminism (runtime_key.ex:85-91): UsesJason.encode!on a map before hashing. Elixir small maps preserve insertion order but large maps use hash ordering — two logically identical MCP configs could produce different hashes if key order diverges. Consider sorting keys first.lease/2→start_new_runtime/2(opencode_runtime.ex:912-941): If two threads race to create a runtime for the same key, the loser terminates its child and falls back to lookup. The fallback is single-attempt — if the winner also dies in that window, it returns an error string instead of retrying.event_relevant?/2filtering (opencode_session.ex): Events without a recognized session ID field pass through to ALL thread wrappers. Verify this doesn't cause duplicate event emission for session-scoped events that use an unexpected field name.opencodebinary isn't available in CI. Tests cover function exports, struct shape, RuntimeKey construction, and RuntimeRegistry ref counting, but the actual runtime leasing → SSE fanout → session lifecycle path is untested. Needs live validation.Link to Devin session: https://app.devin.ai/sessions/0e882ab9b4224a5fa02ba3e1242fe30b
Requested by: @ranvier2d2
Summary by CodeRabbit
New Features
Reliability
Tests