Repository navigation
feat(opencode-agents): open one OpenCode session per call - #179
Conversation
ba09746 to
89320c9
Compare
|
Since #177 and this PR both handle the judge/session problem: is the plan to land #177 first (for the For context, a trial merge of main + #177 + this PR conflicts in |
|
@jeffdupont Keep only #177's This PR already opens a session per call, so a judge does not enter the agent's session. Leaving the raise in place would block that. a29afc3 records the block-array history on the span and does not bring the raise over. #177 and #180 stay open. |
jeffdupont
left a comment
There was a problem hiding this comment.
Thanks for this — the per-call session model is a nice simplification. A few things need addressing before merge:
1. Run can hang indefinitely when the message POST fails (server.py:116-126, _read_until_done; client created with timeout=None at server.py:66)
While the post task is pending, the loop awaits lines.__anext__() with no timeout and only checks post.done() after another SSE line arrives. A real /event stream never closes, so if POST /session/{id}/message returns e.g. 400 (bad provider/model id) without a session.idle, the handler hangs until some unrelated bus frame shows up — or forever. Reproduced with a MockTransport whose /event stays open after server.connected and the POST returns 400. Suggest racing the next line against the post task (asyncio.wait(..., return_when=FIRST_COMPLETED)), raising immediately on a ≥400 status, and a finite default timeout for the non-SSE calls. Tests miss this because every mocked /event body is finite.
2. Judges are re-enabled but templates aren't rendered (handler.py:690-731)
This PR drops the skip_judges=True guidance, but nothing in the package calls parse_template. run_judges passes variables={"message_history", "response_to_evaluate"} and expects the handler to substitute them, so the judge sees literal {{message_history}} and produces a degraded score that's still tracked. It also regresses agent instructions using {{ldContext.*}} (on main, the events callable could render them). Either land rendering here / merge #182 first, or keep the skip_judges advice until #182 lands. test_a_judge_opens_its_own_session only asserts two sessions opened — it should assert the rendered judge prompt and a parsed score.
3. Abort on early exit / session.error depends on GC (handler.py:579-590, 664-683)
The async generators are iterated with bare async for and never aclose()d, so the finally (cancel post, POST /abort, close stream/owned client) only runs whenever asyncio's asyncgen finalizer gets to it. Reproduced: breaking after the first TEXT_MESSAGE_CONTENT sent zero aborts until a sleep + gc.collect(). With an injected client this can race the caller closing it. Wrap with contextlib.aclosing(...) and add tests asserting the abort happens immediately.
4. 50 ms drain can truncate output/usage (server.py:17)
Once the POST returns, any 50 ms gap ends the turn even without this session's session.idle. On a loaded or remote server the final message.updated (with usage) can arrive later and get silently dropped. Prefer session.idle as the authoritative end with a much longer fallback, or reconcile from the POST response body.
5. Judges run as a full OpenCode agent with tools (handler.py:486-510)
A judge prompt containing the evaluated (possibly user-influenced) response runs with bash/edit tools in the server's cwd. At minimum document it; ideally allow a tool-less agent for judges (e.g. judge_agent=).
Nits
server.py:133-135/handler.py:113: asession.idlewith nosessionIDis treated as this session's end; require an exact match.handler.py:524:kwargs.pop("base_url")raises a bareKeyError;create_opencode_handlerraises a clearTypeErrorfor the same case.
Also flagging merge order with #182 and #177 — #182 should land before (or with) this, and this makes most of #177's judge guard redundant.
|
Addressed in d29fe06.
Nits: an idle event with no session id does not end the turn. OpenCode tests: 42 passed. Not merged. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d29fe06. Configure here.
Co-authored-by: alexis <alexis@launchdarkly.com>
Import AsyncIterable for the existing reducer and drop the unused test import. Co-authored-by: alexis <alexis@launchdarkly.com>
A failed message POST raises without waiting on an open event bus. Early exit and session.error abort through aclosing. This session's idle ends the turn, with a longer fallback so the final usage is kept. Agent instructions, including a judge prompt, are rendered. Co-authored-by: alexis <alexis@launchdarkly.com>
… stream A quiet bus and a failed message POST both leave the pending /event line task running. Cancel it and await it, swallowing CancelledError, before GET /event closes. Co-authored-by: alexis <alexis@launchdarkly.com>
The Bugbot fix push did not trigger the pull_request workflow.
c8d2911 to
491939d
Compare
jeffdupont
left a comment
There was a problem hiding this comment.
Re-reviewed at 491939d. Every point from my 20 Oct review is fixed. For each one, I removed the fix and checked that a test fails:
- Failed message POST no longer hangs: the next SSE line is raced against the POST (
server.py:155-166), and calls use finite timeouts. See the one ask below. - Judge prompts are rendered:
_system_textrunsparse_templateoninstructions. A messages-mode judge reaches the handler with its messages already collapsed intoinstructionsby core (judges.py:141-153), so it is covered too. Removing the render failstest_a_judge_opens_its_own_session. - Abort no longer waits for garbage collection: every generator level is wrapped in
aclosing. Turningaclosinginto a no-op failstest_breaking_aborts_without_waiting_for_gc. - No 50 ms drain: this session's
session.idleends the turn, with a 5 s quiet fallback. Setting the fallback back to 50 ms failstest_a_late_completion_survives_the_post_returning. - Judge tools: documented in the README and the docstring. That is the minimum I asked for; a tool-less judge agent can come later.
- Nits:
session.idleneeds an exactsessionIDmatch (a test fails without it), and a missingbase_urlraisesTypeError. Bugbot's cancelled-read finding is fixed and tested.
43 passed locally. CI is green, and the branch merges cleanly with main now that #177 has landed.
One ask (should fix, not blocking): test_a_failed_message_post_does_not_wait_for_the_bus passes even with the race removed. _OpenBus sends assistant_open and text_delta right away, and when the second frame arrives the loop sees the finished POST and raises. A real server sends nothing after rejecting the message, and that silence is the hang this fix prevents. With a silent bus for the 400 case (head = [] if script.message_status >= 400 else [...]), the test passes with the fix and times out at 2 s without it. Making that change means the test actually guards the fix.
Stack: #182 sits on this PR, and #180 is still open even though #182 says that route was rejected. Can #180 be closed?
A 400 used to be followed by assistant events, so the test passed even without racing the POST. A rejected message sends nothing. Co-authored-by: alexis <alexis@launchdarkly.com>
<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
## Decision
Render `{{variables}}` in non-system config messages with
`parse_template`. History and the user turn stay raw. Instruction
rendering, including the judge prompt, is already on #179, which is
merged to main.
## Why
Configured messages were posted with placeholders still in them. The
other handlers render those messages. History and the latest user turn
are already concrete text.
## What this PR does not do
- It does not treat the model resolver as a writer of
`config["instructions"]`. `ModelResolver` only returns a provider id and
a model id.
- It does not add a messages-mode route.
- It does not change the session lifecycle from #179.
- It does not choose the AI Config key.
## Next
Observability #2547 passes `base_url` to `create_opencode_handler`. This
PR's base is main.
<!-- CURSOR_AGENT_PR_BODY_END -->
<div><a
href="https://cursor.com/agents/bc-1c257247-7e7f-505a-a3a8-201076b7d425?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a
href="https://cursor.com/background-agent?bcId=bc-1c257247-7e7f-505a-a3a8-201076b7d425&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>

Decision
One OpenCode session per call. The handler takes
base_urlfor a runningopencode serve, opensGET /event, thenPOST /sessionandPOST /session/{id}/message, and reduces only that session with the existing_Reducer. A judge gets its own session. Agent instructions, including the judge prompt, are rendered withparse_template.Why
The reducer drops every event whose session id is not the one it was bound to.
create_opencode_handlerused to take onesession_idfor the whole process, so a judge prompt entered the agent's session, or the agent's answer was scored as the verdict. A Vega-local handler was rejected: the session boundary belongs in this package.A failed message POST used to wait forever on an open
/eventstream. Early exit waited on garbage collection before aborting. A 50ms drain could drop the finalmessage.updated.events=andsession_id=are removed. The package is not on PyPI and is still pre-v1.What this PR deliberately does not do
create_opencode_handlerstays("*", "agent").Next
#182 renders
{{variables}}in non-system config messages. History and the user turn stay raw. #180 stays open and is not part of this stack.