feat(harness): Cursor ACP adapter + envelope/timeout fix - #44
Merged
Merged
Conversation
Implement AcpSession GenServer for Cursor's Agent Client Protocol, validated against real wire captures from cursor agent acp (Cursor 2.6.22). Key discovery: Cursor ACP uses standard JSON-RPC 2.0 on the wire, not the ndjson-rpc envelope from Julius's Effect-based SDK. This means the existing JsonRpc module works directly — no custom codec needed. AcpSession implements full ProviderBehaviour: handshake (initialize → authenticate → session/new), session/prompt with prompt array format, agent_thought_chunk reasoning stream, tool_call/tool_call_update decomposition, permission handling, and extension pass-through. Routed behind T3CODE_CURSOR_ACP=1 config flag to preserve CursorSession as fallback. Also includes: HarnessClientManager envelope/timeout fix, per-package AGENTS.md files, and task planning docs including 4-provider council review of the migration architecture. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds governance docs and multiple planning/task documents, implements a new Elixir GenServer Changes
Sequence Diagram(s)sequenceDiagram
participant Web as Web Client
participant Server as T3 Server
participant SessionMgr as Harness.SessionManager
participant Acp as Harness.Providers.AcpSession
participant Agent as Cursor Agent (subprocess Port)
Web->>Server: WS request -> start session / session.start
Server->>SessionMgr: create session (provider: "cursor")
SessionMgr->>Acp: start_link (AcpSession) [if cursor_acp_enabled]
Acp->>Agent: spawn Port & send JSON-RPC "initialize"
Agent-->>Acp: initialize response (capabilities)
Acp->>Agent: authenticate / session/new (handshake)
Agent-->>Acp: session state / session/update notifications
Acp->>SessionMgr: emit harness events (session/started, session/update, turn events)
SessionMgr->>Server: project events -> runtime / UI events
Web-->>Server: user actions (prompt/cancel/approval)
Server->>SessionMgr: dispatch to Acp (send_turn/interrupt/respond_to_approval)
Acp->>Agent: JSON-RPC requests (session/prompt, session/cancel, response)
Agent-->>Acp: responses / notifications -> pending map resolution
Acp->>SessionMgr: turn completion / errors / session/closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
apps/harness/test/fixtures/acp/handshake_real.ndjson (1)
9-10: Consider pretty-printing long JSON lines for readability.Lines 9-10 contain thousands of characters on single lines, making them difficult to review and diff. While this preserves the exact wire format, pretty-printing the JSON would improve maintainability without affecting test validity.
Example: Pretty-printed format
>>> {"jsonrpc":"2.0","id":3,"method":"session/new","params":{ "cwd":"/Users/testuser/coding/t3code-OTP", "mcpServers":[] }} <<< {"jsonrpc":"2.0","id":3,"result":{ "sessionId":"00000000-0000-0000-0000-000000000001", "modes":{"currentModeId":"agent","availableModes":[...]}, "models":{"currentModelId":"default[]","availableModels":[...]}, "configOptions":[...] }}This makes it easier to spot changes in diffs while preserving all data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/test/fixtures/acp/handshake_real.ndjson` around lines 9 - 10, The NDJSON fixture contains very long single-line JSON entries (e.g., the records with "id":3 result and the "method":"session/update" params) that hurt readability; reformat those JSON payloads to be pretty-printed (multiline with indentation) while preserving the exact JSON content and values and keeping the existing NDJSON record boundaries/markers (the <<< / >>> separators and each JSON object as a single logical record). Locate the offending entries by their unique fields ("id":3, "result", and "method":"session/update"/"availableCommands") in handshake_real.ndjson and replace the single-line JSON strings with equivalent indented JSON blocks so diffs are readable but tests still parse the same data.apps/harness/lib/harness/session_manager.ex (1)
376-377: Consider test isolation for the global config dependency.
Application.get_env(:harness, :cursor_acp_enabled, false)reads global application state. If tests modify this config value without proper cleanup, it could cause non-deterministic test behavior — especially for e2e tests likee2e_channel_test.exsthat exercise the "cursor" provider path.Consider using a test helper or
Application.put_envwithon_exitcleanup in tests that need to toggle this flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/session_manager.ex` around lines 376 - 377, The code path uses a global flag via Application.get_env(:harness, :cursor_acp_enabled, false) to decide to return AcpSession, which can make tests brittle; update tests (e.g., e2e_channel_test.exs) to avoid leaking global state by saving the original value, calling Application.put_env(:harness, :cursor_acp_enabled, true|false) for the test, and registering an on_exit callback to restore the original value, or add a small test helper function (e.g., set_cursor_acp_enabled/1) that does the put_env + on_exit restore so tests reliably toggle the cursor provider path without affecting other tests.apps/harness/config/runtime.exs (1)
8-10: Normalize env-var whitespace before boolean parsing.Values like
" true "currently evaluate tofalse. Trimming beforeString.downcase/1makes the feature flag parsing more tolerant.Proposed tweak
cursor_acp_enabled = System.get_env("T3CODE_CURSOR_ACP", "0") + |> String.trim() |> String.downcase() |> then(&(&1 in ["1", "true", "yes", "on"]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/config/runtime.exs` around lines 8 - 10, The feature-flag pipeline currently lowercases the raw env string but doesn't trim surrounding whitespace, so values like " true " fail; modify the pipeline that starts with System.get_env("T3CODE_CURSOR_ACP", "0") to call String.trim/1 before String.downcase/1 (i.e., trim then downcase) so the subsequent then/1 check (&1 in ["1", "true", "yes", "on"]) correctly recognizes whitespace-padded truthy values.ai_docs/tasks/007_devin_provider_integration.md (1)
40-52: Add language tags to these fenced architecture blocks.These fences should include a language identifier (
textis fine) to satisfy MD040 and keep docs lint noise down.Also applies to: 56-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai_docs/tasks/007_devin_provider_integration.md` around lines 40 - 52, The fenced architecture blocks describing DevinAdapter (the block containing "DevinAdapter", "startSession", "sendTurn", "poll loop", "ProviderRuntimeEvent", and "ProviderSessionDirectory") need a language tag to satisfy MD040; update the opening fences from ``` to ```text for that block and the similar block at lines ~56-66 so the linter recognizes them as text fences. Ensure you only add the language identifier (e.g., text) to the existing triple-backtick fences surrounding those architecture snippets.ai_docs/tasks/005_pitch_pr581_otp_architecture.md (1)
133-133: Tighten wording/markdown at Line 133 for lint cleanliness.Use “different from” (instead of “different than”) and fix the malformed emphasis around
*when\*to avoid MD037/style lint noise in this section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai_docs/tasks/005_pitch_pr581_otp_architecture.md` at line 133, Replace the phrase "different than" with "different from" and fix the malformed emphasis token `*when\*` by changing it to either proper emphasis "*when*" or inline code "`when`" so the markdown is valid; search for the literal text "different than" and the malformed "*when*" token in the document and update those occurrences to the corrected forms.ai_docs/tasks/003_opencode_model_picker_lag.md (1)
17-27: Specify a language for the root-cause fenced block.Add a language hint (for example
text) to this code fence to satisfy MD040 and improve rendering consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai_docs/tasks/003_opencode_model_picker_lag.md` around lines 17 - 27, The root-cause fenced code block (the block showing the bullet trace starting with "OpenCode API -> 4,070 models (no filtering)") is missing a language hint which triggers MD040; update that fence from ``` to include a language token (for example change the opening fence to ```text) so the block has an explicit language, and ensure the closing fence remains ```; this applies to the fenced block in ai_docs/tasks/003_opencode_model_picker_lag.md that contains the trace used by the root-cause analysis.ai_docs/tasks/006_cursor_acp_migration.md (1)
33-33: Add language identifiers to fenced code blocks.These fences are missing language tags (MD040). Adding
text,elixir,json, etc. will keep lint clean and improve readability.Also applies to: 48-48, 303-303, 377-377, 508-508
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai_docs/tasks/006_cursor_acp_migration.md` at line 33, Several fenced code blocks in ai_docs/tasks/006_cursor_acp_migration.md are missing language identifiers (MD040); update each triple-backtick block (the ``` fences) by adding an appropriate language tag such as text, elixir, json, etc., to the opening fence so the linter passes and readability improves—ensure you update every occurrence of bare ``` in the file (including the instances mentioned) with the correct language hint.apps/harness/lib/harness/providers/acp_session.ex (2)
1024-1047: Unusedreasonparameter inreject_all_pending/2.The
_reasonparameter is ignored (line 1024), but callers pass specific reasons like"Handshake timeout"or"Process exited". Consider using the reason in the error reply at line 1040 for better debugging context.Optional: Use reason in error reply
-defp reject_all_pending(state, _reason) do +defp reject_all_pending(state, reason) do Enum.each(state.pending, fn ... {_id, %{from: from, timer: timer}} -> if timer, do: Process.cancel_timer(timer) - if from, do: GenServer.reply(from, {:error, "Session terminated"}) + if from, do: GenServer.reply(from, {:error, reason}) ... end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/acp_session.ex` around lines 1024 - 1047, The function reject_all_pending/2 currently ignores the passed-in _reason; change the parameter to reason and propagate it into the replies and notifications so callers get context: in the branch that does GenServer.reply(from, {:error, "Session terminated"}) send GenServer.reply(from, {:error, reason}), and also include the reason in the emit_event payloads for "user-input/resolved" and "request/resolved" (e.g. add a "reason" key using the reason variable); keep existing timer cancellation (Process.cancel_timer) and the clearing of state.pending as-is.
494-522: Consider consolidating runtime mode format.The dual check for
"full-access"and"full_access"(line 498) handles format inconsistency defensively. If both formats are intentionally supported for backwards compatibility, consider normalizing upstream when params are received, or document this in the module.Optional: Normalize early or use helper
defp full_access_mode?(params) do Map.get(params, "runtimeMode", "full-access") in ["full-access", "full_access"] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/providers/acp_session.ex` around lines 494 - 522, The code in handle_rpc_request is defensively checking runtime_mode for both "full-access" and "full_access"; normalize the runtime mode once (when state.params are parsed or at start of handle_rpc_request) and then use a single comparison to "full-access" (or implement a small helper like full_access_mode?/1) so you don't need the dual literal check—locate the Map.get(state.params, "runtimeMode", "full-access") usage in handle_rpc_request and replace it with a normalized value (e.g., downcase and replace "_" with "-" or call the helper) before the if that decides to auto-accept or emit a request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 21-29: Add a new "Capabilities / Interfaces / Protocols"
subsection to AGENTS.md that concisely enumerates the repo-level communication
and integration contracts: list Phoenix Channels and WebSocket APIs
(apps/harness and apps/server communication), provider adapter interfaces and
expected adapter entry points (used by apps/harness provider GenServers and
ports), the provider GenServer/port protocol and expected message shapes,
session orchestration and event formats handled by apps/server and apps/web, and
the contract schema boundaries and versioning rules provided by
packages/contracts and runtime helpers in packages/shared; for each item include
a one-line statement of responsibility, expected message/format surface, and
where the authoritative schema/implementation lives (refer to apps/server,
apps/web, apps/harness, packages/contracts, packages/shared).
In `@ai_docs/tasks/006_cursor_acp_migration.md`:
- Line 784: The doc incorrectly references extracting auth method from
initialize response as agentCapabilities.auth.methods[]; update the text and any
example usage to instead read the top-level authMethods field (e.g.,
response.authMethods) when determining the methodId for authenticate (methodId:
"cursor_login"), and add guidance to check for an empty or missing authMethods
array and emit a clear, graceful error path (including headless/device-flow
notes) when no auth methods are present or authentication fails.
- Around line 747-765: The documented state shape is inconsistent: next_id is an
integer (JSON-RPC 2.0 wire ID) but pending is typed as %{String.t() => map()};
update the pending type to use integer() keys (e.g., %{integer() => map()}) so
it matches next_id and the JSON-RPC comment, and adjust any explanatory comment
mentioning "Keys are integer ids" to reflect the integer() key type for pending
(references: next_id, pending, CodexSession).
In `@apps/harness/test/fixtures/acp/full_turn_real.ndjson`:
- Line 4: The fixture contains a PII-bearing absolute path in the JSON-RPC
params (look for the "session/new" entry and its "params.cwd" field) — replace
the user-specific path "/Users/joaquinvenegasarevalo/coding/t3code-OTP" with a
stable placeholder (e.g. "/workspace/t3code-OTP" or a relative path) in
apps/harness/test/fixtures/acp/full_turn_real.ndjson so the committed fixture
contains no personal identifiers.
In `@apps/harness/test/fixtures/acp/handshake_real.ndjson`:
- Around line 8-10: The fixture contains real PII in the "session/new"
params.cwd string and real UUIDs in the "result.sessionId" and the
"session/update" params.sessionId; replace the real username segment
(joaquinvenegasarevalo) in params.cwd with a generic test user/path (e.g.,
/Users/testuser/project or /home/testuser/project) and replace the real session
UUID values in result.sessionId and session/update.params.sessionId with a
deterministic test UUID (e.g., 00000000-0000-0000-0000-000000000001) to sanitize
the fixture while preserving the same keys and JSON structure.
In `@packages/shared/AGENTS.md`:
- Around line 1-20: Add a new dedicated "Agent Capabilities / Interfaces /
Protocols" section to AGENTS.md for packages/shared that lists the explicit
capabilities exposed by this package (e.g., utility types, messaging helpers),
public interfaces and contract names consumers should rely on (mention exported
modules and types by name), and the communication/usage protocols (sync vs async
APIs, error handling conventions, expected input/output shapes and any
event/message formats). Name the section header clearly (e.g., "Agent
Capabilities and Protocols") and include references to the package identity
"packages/shared" and the explicit export surface so consumers know which
modules/types are part of the contract; keep it brief and aligned with the repo
AGENTS contract.
---
Nitpick comments:
In `@ai_docs/tasks/003_opencode_model_picker_lag.md`:
- Around line 17-27: The root-cause fenced code block (the block showing the
bullet trace starting with "OpenCode API -> 4,070 models (no filtering)") is
missing a language hint which triggers MD040; update that fence from ``` to
include a language token (for example change the opening fence to ```text) so
the block has an explicit language, and ensure the closing fence remains ```;
this applies to the fenced block in
ai_docs/tasks/003_opencode_model_picker_lag.md that contains the trace used by
the root-cause analysis.
In `@ai_docs/tasks/005_pitch_pr581_otp_architecture.md`:
- Line 133: Replace the phrase "different than" with "different from" and fix
the malformed emphasis token `*when\*` by changing it to either proper emphasis
"*when*" or inline code "`when`" so the markdown is valid; search for the
literal text "different than" and the malformed "*when*" token in the document
and update those occurrences to the corrected forms.
In `@ai_docs/tasks/006_cursor_acp_migration.md`:
- Line 33: Several fenced code blocks in
ai_docs/tasks/006_cursor_acp_migration.md are missing language identifiers
(MD040); update each triple-backtick block (the ``` fences) by adding an
appropriate language tag such as text, elixir, json, etc., to the opening fence
so the linter passes and readability improves—ensure you update every occurrence
of bare ``` in the file (including the instances mentioned) with the correct
language hint.
In `@ai_docs/tasks/007_devin_provider_integration.md`:
- Around line 40-52: The fenced architecture blocks describing DevinAdapter (the
block containing "DevinAdapter", "startSession", "sendTurn", "poll loop",
"ProviderRuntimeEvent", and "ProviderSessionDirectory") need a language tag to
satisfy MD040; update the opening fences from ``` to ```text for that block and
the similar block at lines ~56-66 so the linter recognizes them as text fences.
Ensure you only add the language identifier (e.g., text) to the existing
triple-backtick fences surrounding those architecture snippets.
In `@apps/harness/config/runtime.exs`:
- Around line 8-10: The feature-flag pipeline currently lowercases the raw env
string but doesn't trim surrounding whitespace, so values like " true " fail;
modify the pipeline that starts with System.get_env("T3CODE_CURSOR_ACP", "0") to
call String.trim/1 before String.downcase/1 (i.e., trim then downcase) so the
subsequent then/1 check (&1 in ["1", "true", "yes", "on"]) correctly recognizes
whitespace-padded truthy values.
In `@apps/harness/lib/harness/providers/acp_session.ex`:
- Around line 1024-1047: The function reject_all_pending/2 currently ignores the
passed-in _reason; change the parameter to reason and propagate it into the
replies and notifications so callers get context: in the branch that does
GenServer.reply(from, {:error, "Session terminated"}) send GenServer.reply(from,
{:error, reason}), and also include the reason in the emit_event payloads for
"user-input/resolved" and "request/resolved" (e.g. add a "reason" key using the
reason variable); keep existing timer cancellation (Process.cancel_timer) and
the clearing of state.pending as-is.
- Around line 494-522: The code in handle_rpc_request is defensively checking
runtime_mode for both "full-access" and "full_access"; normalize the runtime
mode once (when state.params are parsed or at start of handle_rpc_request) and
then use a single comparison to "full-access" (or implement a small helper like
full_access_mode?/1) so you don't need the dual literal check—locate the
Map.get(state.params, "runtimeMode", "full-access") usage in handle_rpc_request
and replace it with a normalized value (e.g., downcase and replace "_" with "-"
or call the helper) before the if that decides to auto-accept or emit a request.
In `@apps/harness/lib/harness/session_manager.ex`:
- Around line 376-377: The code path uses a global flag via
Application.get_env(:harness, :cursor_acp_enabled, false) to decide to return
AcpSession, which can make tests brittle; update tests (e.g.,
e2e_channel_test.exs) to avoid leaking global state by saving the original
value, calling Application.put_env(:harness, :cursor_acp_enabled, true|false)
for the test, and registering an on_exit callback to restore the original value,
or add a small test helper function (e.g., set_cursor_acp_enabled/1) that does
the put_env + on_exit restore so tests reliably toggle the cursor provider path
without affecting other tests.
In `@apps/harness/test/fixtures/acp/handshake_real.ndjson`:
- Around line 9-10: The NDJSON fixture contains very long single-line JSON
entries (e.g., the records with "id":3 result and the "method":"session/update"
params) that hurt readability; reformat those JSON payloads to be pretty-printed
(multiline with indentation) while preserving the exact JSON content and values
and keeping the existing NDJSON record boundaries/markers (the <<< / >>>
separators and each JSON object as a single logical record). Locate the
offending entries by their unique fields ("id":3, "result", and
"method":"session/update"/"availableCommands") in handshake_real.ndjson and
replace the single-line JSON strings with equivalent indented JSON blocks so
diffs are readable but tests still parse the same data.
🪄 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: d8358795-4693-434c-9618-466c90042893
📒 Files selected for processing (26)
AGENTS.mdai_docs/tasks/001_ws_mcp_api.mdai_docs/tasks/002_claude_mcp_extraction.mdai_docs/tasks/003_opencode_model_picker_lag.mdai_docs/tasks/004_error_handling_audit_and_hardening.mdai_docs/tasks/005_pitch_pr581_otp_architecture.mdai_docs/tasks/006_cursor_acp_migration.mdai_docs/tasks/007_devin_provider_integration.mdapps/harness/AGENTS.mdapps/harness/config/runtime.exsapps/harness/lib/harness/json_rpc.exapps/harness/lib/harness/providers/acp_session.exapps/harness/lib/harness/session_manager.exapps/harness/test/fixtures/acp/cancel_turn.ndjsonapps/harness/test/fixtures/acp/error_turn.ndjsonapps/harness/test/fixtures/acp/full_turn_real.ndjsonapps/harness/test/fixtures/acp/handshake_real.ndjsonapps/harness/test/fixtures/acp/reasoning_turn.ndjsonapps/harness/test/fixtures/acp/tool_turn.ndjsonapps/harness/test/harness/json_rpc_test.exsapps/harness/test/harness/providers/acp_session_test.exsapps/server/AGENTS.mdapps/server/src/provider/Layers/HarnessClientManager.tsapps/web/AGENTS.mdpackages/contracts/AGENTS.mdpackages/shared/AGENTS.md
Comment on lines
+21
to
29
| ## Architecture Map | ||
|
|
||
| If a tradeoff is required, choose correctness and robustness over short-term convenience. | ||
| - `apps/server`: Node/Bun server runtime. Owns provider processes, WebSocket APIs, session orchestration, and desktop-facing backend behavior. | ||
| - `apps/web`: React/Vite frontend. Owns session UX, event rendering, local state, and browser interaction flows. | ||
| - `apps/harness`: Phoenix/OTP harness pilot. Owns provider GenServers, supervision, ports, and Elixir-side protocol adapters. | ||
| - `packages/contracts`: Shared schemas and TypeScript contracts only. No app runtime logic. | ||
| - `packages/shared`: Shared runtime helpers used by server and web through explicit subpath exports. | ||
| - `ai_docs`: planning docs, task writeups, and architecture notes. Keep these aligned with shipped behavior. | ||
|
|
There was a problem hiding this comment.
Add an explicit “capabilities/interfaces/protocols” subsection.
The architecture map is clear, but AGENTS guidance for this repo expects explicit capability/interface/protocol documentation. Please add a concise section enumerating these at repo level (for example: Phoenix Channels, provider adapter interfaces, contract schema boundaries).
Based on learnings: “Applies to AGENTS.md : Document agent capabilities, interfaces, and communication protocols in AGENTS.md”.
🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: Ensure spelling is correct
Context: ...ports. - ai_docs: planning docs, task writeups, and architecture notes. Keep these ali...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 21 - 29, Add a new "Capabilities / Interfaces /
Protocols" subsection to AGENTS.md that concisely enumerates the repo-level
communication and integration contracts: list Phoenix Channels and WebSocket
APIs (apps/harness and apps/server communication), provider adapter interfaces
and expected adapter entry points (used by apps/harness provider GenServers and
ports), the provider GenServer/port protocol and expected message shapes,
session orchestration and event formats handled by apps/server and apps/web, and
the contract schema boundaries and versioning rules provided by
packages/contracts and runtime helpers in packages/shared; for each item include
a one-line statement of responsibility, expected message/format surface, and
where the authoritative schema/implementation lives (refer to apps/server,
apps/web, apps/harness, packages/contracts, packages/shared).
Comment on lines
+1
to
+20
| ## What This Folder Is | ||
|
|
||
| - This folder contains shared runtime utilities consumed by multiple apps. | ||
| - Boundaries: reusable helpers with explicit subpath exports belong here. App-specific orchestration and schemas do not. | ||
|
|
||
| ## Local Invariants | ||
|
|
||
| - Preserve explicit subpath exports; do not turn this package into a barrel-style catch-all. | ||
| - Utilities here should stay broadly reusable across server and web. | ||
| - Keep dependencies light and avoid importing app-local modules. | ||
|
|
||
| ## Safe Changes | ||
|
|
||
| - Prefer adding a new focused module plus explicit export entry rather than growing unrelated helpers inside an existing file. | ||
| - Avoid leaking package-internal paths into consumers. | ||
|
|
||
| ## Validate | ||
|
|
||
| - Fast check: `bun run --filter=@t3tools/shared typecheck` | ||
| - Focused tests: `bun run --filter=@t3tools/shared test` |
There was a problem hiding this comment.
Add explicit capabilities/interfaces/protocols section for this agent scope.
This AGENTS file clearly defines purpose and boundaries, but it does not explicitly list agent capabilities, interfaces, and communication protocols for packages/shared. Please add a short dedicated section so the file matches the repo AGENTS contract.
As per coding guidelines: “Document agent capabilities, interfaces, and communication protocols in AGENTS.md”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/AGENTS.md` around lines 1 - 20, Add a new dedicated "Agent
Capabilities / Interfaces / Protocols" section to AGENTS.md for packages/shared
that lists the explicit capabilities exposed by this package (e.g., utility
types, messaging helpers), public interfaces and contract names consumers should
rely on (mention exported modules and types by name), and the
communication/usage protocols (sync vs async APIs, error handling conventions,
expected input/output shapes and any event/message formats). Name the section
header clearly (e.g., "Agent Capabilities and Protocols") and include references
to the package identity "packages/shared" and the explicit export surface so
consumers know which modules/types are part of the contract; keep it brief and
aligned with the repo AGENTS contract.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+345
to
+359
| def terminate(_reason, state) do | ||
| state = %{state | stopped: true} | ||
| _ = reject_all_pending(state, "Session terminated") | ||
| emit_event(state, :session, "session/closed", %{}) | ||
|
|
||
| if state.port do | ||
| try do | ||
| Port.close(state.port) | ||
| catch | ||
| _, _ -> :ok | ||
| end | ||
| end | ||
|
|
||
| :ok | ||
| end |
There was a problem hiding this comment.
🔴 AcpSession terminate does not reply to ready_waiters, orphaning callers on process death
Unlike CodexSession (codex_session.ex:484), AcpSession's terminate callback only calls reject_all_pending (which handles the pending map) but does not reply to ready_waiters. When the cursor binary crashes during the handshake — causing handle_info({port, {:exit_status, ...}}) to stop the GenServer — any wait_for_ready callers are never explicitly replied to. They eventually receive a bare {:EXIT, pid, :normal} signal, producing an unhelpful error like "Session process died during init: :normal" instead of a meaningful error.
This violates the apps/harness/AGENTS.md rule: "Protocol adapters must fail closed: reject pending work on port exit, avoid orphaned waiters, and preserve crash isolation between sessions."
CodexSession's terminate at codex_session.ex:484 correctly replies: Enum.each(state.ready_waiters, &GenServer.reply(&1, {:error, "Session terminated"})).
Suggested change
| def terminate(_reason, state) do | |
| state = %{state | stopped: true} | |
| _ = reject_all_pending(state, "Session terminated") | |
| emit_event(state, :session, "session/closed", %{}) | |
| if state.port do | |
| try do | |
| Port.close(state.port) | |
| catch | |
| _, _ -> :ok | |
| end | |
| end | |
| :ok | |
| end | |
| @impl true | |
| def terminate(_reason, state) do | |
| state = %{state | stopped: true} | |
| Enum.each(state.ready_waiters, &GenServer.reply(&1, {:error, "Session terminated"})) | |
| _ = reject_all_pending(state, "Session terminated") | |
| emit_event(state, :session, "session/closed", %{}) | |
| if state.port do | |
| try do | |
| Port.close(state.port) | |
| catch | |
| _, _ -> :ok | |
| end | |
| end | |
| :ok | |
| end |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+271
to
+278
| def handle_call({:interrupt_turn, _thread_id, _turn_id}, _from, state) do | ||
| send_to_port( | ||
| state, | ||
| JsonRpc.encode_notification("session/cancel", %{"sessionId" => state.acp_session_id}) | ||
| ) | ||
|
|
||
| state = complete_turn(state, "interrupted") | ||
| {:reply, :ok, state} |
There was a problem hiding this comment.
🔴 interrupt_turn does not clear the pending session/prompt entry, causing stale responses to complete the wrong turn
When interrupt_turn is called, it sends a session/cancel notification and calls complete_turn(state, "interrupted") which resets current_turn_id to nil and status to :ready. However, the pending session/prompt RPC entry (with its from caller reference) is not removed from the pending map.
If a new send_turn starts before the ACP process responds to the old prompt, the stale session/prompt response will: (1) reply to the old send_turn caller with the new turn's turnId (state.current_turn_id now points to the new turn, line 434), and (2) call complete_turn("completed") which prematurely completes the new turn (line 439), emitting a spurious turn/completed event and resetting the session to :ready. All subsequent streaming data for the new turn is then dropped by should_apply_session_update? since current_turn_id becomes nil.
Scenario trace
- Turn A starts → pending[4] with from=callerA, current_turn_id="turn-A"
- interrupt_turn → session/cancel sent, complete_turn sets current_turn_id=nil, but pending[4] remains
- Turn B starts → pending[5] with from=callerB, current_turn_id="turn-B"
- Old prompt response for RPC 4 arrives → callerA gets turnId="turn-B" (wrong), complete_turn completes turn-B prematurely
Prompt for agents
In apps/harness/lib/harness/providers/acp_session.ex, the handle_call for {:interrupt_turn, ...} at lines 271-278 needs to remove the pending session/prompt entry and reply to its from caller before calling complete_turn. The current_prompt_rpc_id field stores the RPC ID of the active prompt request.
Replace the handler body (lines 271-278) with logic that:
1. Sends the session/cancel notification (existing line 272-275)
2. Pops the pending entry for state.current_prompt_rpc_id from state.pending
3. If found, cancels its timer and replies to its from with {:ok, %{threadId: state.thread_id, turnId: state.current_turn_id}}
4. Calls complete_turn on the updated state
5. Returns {:reply, :ok, state}
This ensures the send_turn caller gets a response with the correct turn ID and the stale pending entry is removed before any new turn can start.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Reply to ready_waiters in terminate/2 so callers get a meaningful
error instead of bare {:EXIT, pid, :normal} during handshake crashes
- Clear pending session/prompt entry on interrupt_turn to prevent stale
responses from completing the wrong turn after cancel + new turn
- Sanitize PII from test fixtures (real paths → /workspace/t3code-OTP,
session UUIDs → deterministic test values)
- Fix task doc: pending map key type String.t() → integer(), stale
authMethods path reference in Risk #4
- Add opencode.jsonc to .gitignore (contains API key)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AcpSessionGenServer implementing Cursor's Agent Client Protocol (ACP) via standard JSON-RPC 2.0 over stdio, validated against real wire captures fromcursor agent acp(Cursor 2.6.22)JsonRpcmodule reused directly, no custom codec neededT3CODE_CURSOR_ACP=1config flag, preservingCursorSessionas fallbackAGENTS.mdfiles and task planning docs including 4-provider council reviewKey files
apps/harness/lib/harness/providers/acp_session.ex— 1146-line GenServer: handshake, prompt, reasoning (agent_thought_chunk), tool decomposition, permissions, extension pass-throughapps/harness/lib/harness/json_rpc.ex— addedencode_error_response/4apps/harness/test/fixtures/acp/— real wire capture golden files from Cursor 2.6.22ai_docs/tasks/006_cursor_acp_migration.md— full task doc with council review, wire capture findings, and implementation bug trackerTest plan
mix test --no-start(focused ACP + JsonRpc tests)bun fmt,bun lint,bun typecheckT3CODE_CURSOR_ACP=1against real Cursor binary🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests