Add harness runtime plans#987
Conversation
|
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 harness config/schema support and SDK resolver wiring so the SDK can resolve named harnesses (static or attached) into broker-executable PTY or headless app-server configs; broker and worker code accept and enforce harnessConfig, track sessionId/pid, and add a headless app-server worker driver, tests, docs, and trajectories. ChangesHarness Runtime Config Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 28 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Preview deployed!
This preview will be cleaned up when the PR is merged or closed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84d87355ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let session_id = cmd.session_id.clone(); | ||
| let release = cmd.release.trim().to_ascii_lowercase(); | ||
| let auth = app_server_auth_from_env(); | ||
| let http = reqwest::Client::new(); |
There was a problem hiding this comment.
Create the app-server reqwest::Client with an explicit timeout; Client::new() uses no request timeout, so a slow or partitioned endpoint can block send_opencode_prompt/release calls indefinitely. In that state the worker stops progressing deliveries (and may never process shutdown cleanly), which can stall message flow until an external kill happens.
Useful? React with 👍 / 👎.
| for (const searchPath of searchPaths) { | ||
| const candidate = path.join(expandHome(searchPath), command); | ||
| if (existsSync(candidate)) { | ||
| return candidate; |
There was a problem hiding this comment.
Do not rewrite command to a local absolute path during SDK-side static harness resolution. existsSync against searchPaths runs on the caller machine, so in detached/remote broker setups this can serialize a host-specific path that does not exist where the broker executes, causing spawn failures even when the plain command name would have worked in the broker environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 17 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
packages/sdk/src/client.ts (1)
615-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecompute transport and headless validation after spawn patches.
Line 616/Line 617 resolve and validate using the original input, but
runBeforeSpawncan patchharnessPlan. That can produce staletransportin the POST body and reject valid patched requests.Suggested fix
async spawnProvider(input: SpawnProviderInput): Promise<SpawnAgentResult> { - const transport = resolveSpawnTransport(input); - if (transport === 'headless' && !isHeadlessProvider(input.provider) && !input.harnessPlan) { - throw new Error( - `provider '${input.provider}' does not support headless transport (supported: claude, opencode)` - ); - } - const beforeCtx: BeforeAgentSpawnContext = { kind: 'provider', input, @@ }; const t0 = Date.now(); const resolvedInput = (await this.runBeforeSpawn(beforeCtx)) as SpawnProviderInput; + const transport = resolveSpawnTransport(resolvedInput); + if (transport === 'headless' && !isHeadlessProvider(resolvedInput.provider) && !resolvedInput.harnessPlan) { + throw new Error( + `provider '${resolvedInput.provider}' does not support headless transport (supported: claude, opencode)` + ); + } try { const result = await this.transport.request<SpawnAgentResult>('/api/spawn', { method: 'POST', body: JSON.stringify(buildSpawnProviderBody(resolvedInput, transport)), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/client.ts` around lines 615 - 635, The current spawnProvider computes transport and validates headless support from the original input before running runBeforeSpawn, so patches (e.g., to harnessPlan) can make the transport or validation stale; update spawnProvider to call resolveSpawnTransport and perform the headless isHeadlessProvider check again after obtaining resolvedInput (the result of runBeforeSpawn) and use the recomputed transport when calling buildSpawnProviderBody and when performing the headless-rejection throw; specifically, reassign or create a new transport variable from resolveSpawnTransport(resolvedInput) and re-run the headless conditional before the POST to /api/spawn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trajectories/index.json:
- Line 10: Update the "path" entries in .trajectories/index.json to use
canonical repo-relative paths instead of local absolute paths: replace values
like
"/private/tmp/relay-harness-runtime-plans/.trajectories/completed/2026-04/traj_05xg7j388bc4.json"
with ".trajectories/completed/2026-04/traj_05xg7j388bc4.json" (apply the same
normalization for all occurrences referenced, e.g., entries at lines 17, 24,
906, 913, 1123), ensure the "path" property uses a consistent schema across all
records, and run a quick schema/lookup check to confirm consumers resolve the
repo-relative paths correctly.
In `@crates/broker/src/runtime/app_server.rs`:
- Around line 16-17: The HTTP client calls in run_app_server_worker currently
use reqwest::Client::new() and call request.send().await in
send_app_server_request without an explicit per-call deadline; update the code
to enforce a strict timeout so broker-critical paths (deliver_relay,
shutdown_worker) do not block for the default client timeout. Either construct
the client with a bounded timeout via
reqwest::Client::builder().timeout(Duration::from_secs(...)) or wrap the
per-request future in tokio::time::timeout(...) around the request.send().await
inside send_app_server_request (and propagate/handle the timeout error up to
deliver_relay/shutdown_worker), referencing run_app_server_worker,
send_app_server_request, deliver_relay, shutdown_worker and the
reqwest::Client::new()/request.send().await sites to locate where to apply the
change.
In `@crates/broker/src/worker.rs`:
- Around line 325-341: Before applying plan.auth into harness_env, explicitly
remove any existing AGENT_RELAY_APP_SERVER_AUTH_* entries so stale credentials
from self.worker_env are not inherited; modify the code around the harness_env
population (the block handling plan.auth in the function where harness_env is
built) to first remove keys "AGENT_RELAY_APP_SERVER_AUTH_TYPE",
"AGENT_RELAY_APP_SERVER_AUTH_TOKEN", "AGENT_RELAY_APP_SERVER_AUTH_USERNAME", and
"AGENT_RELAY_APP_SERVER_AUTH_PASSWORD" from harness_env (or clear them from the
base env copy) and then apply the plan.auth pushes; apply the same change in the
analogous spot handling the other plan auth block (the one corresponding to
lines 514-519).
In `@packages/sdk/src/relay.ts`:
- Around line 1479-1491: ensureAgentHandle currently returns an existing Agent
from knownAgents without merging new metadata, so later-arriving sessionId/pid
are ignored; update ensureAgentHandle to, when an existing agent is found, merge
incoming metadata into that Agent (e.g., set or update sessionId and pid on the
Agent instance or call an Agent method like setMetadata/updateSession) before
returning it, ensuring makeAgent remains the creation path and knownAgents still
caches the instance; apply the same merge/update logic to the other place that
returns existing agent handles without absorbing metadata (the other
ensure/lookup path that uses makeAgent/knownAgents).
In `@web/content/docs/harnesses.mdx`:
- Around line 132-133: Replace the incorrect SDK method reference snapshotPty
with the actual client method snapshot(...) in the docs line that lists PTY-only
routes (alongside sendInput and resizePty) so readers can copy/paste the correct
API call; update the text to mention snapshot(...) as the PTY snapshot method to
match the client API.
---
Outside diff comments:
In `@packages/sdk/src/client.ts`:
- Around line 615-635: The current spawnProvider computes transport and
validates headless support from the original input before running
runBeforeSpawn, so patches (e.g., to harnessPlan) can make the transport or
validation stale; update spawnProvider to call resolveSpawnTransport and perform
the headless isHeadlessProvider check again after obtaining resolvedInput (the
result of runBeforeSpawn) and use the recomputed transport when calling
buildSpawnProviderBody and when performing the headless-rejection throw;
specifically, reassign or create a new transport variable from
resolveSpawnTransport(resolvedInput) and re-run the headless conditional before
the POST to /api/spawn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f6556c6-037c-4f59-962c-1ebc59e3cc9e
📒 Files selected for processing (30)
.trajectories/completed/2026-05/traj_7i9tigaejfje.json.trajectories/completed/2026-05/traj_7i9tigaejfje.md.trajectories/completed/2026-05/traj_i2pjnx3dll5b.json.trajectories/completed/2026-05/traj_i2pjnx3dll5b.md.trajectories/completed/2026-05/traj_i2pjnx3dll5b.trace.json.trajectories/index.jsonCHANGELOG.mdcrates/broker/src/cli/mod.rscrates/broker/src/listen_api.rscrates/broker/src/protocol.rscrates/broker/src/runtime/api.rscrates/broker/src/runtime/app_server.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/spawn_spec.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/supervisor.rscrates/broker/src/worker.rsdocs/harness-runtime-plan.mdpackages/sdk/src/__tests__/harness.test.tspackages/sdk/src/client.tspackages/sdk/src/harness.tspackages/sdk/src/index.tspackages/sdk/src/lifecycle-hooks.tspackages/sdk/src/protocol.tspackages/sdk/src/relay.tspackages/sdk/src/types.tsweb/content/docs/harnesses.mdxweb/lib/docs-nav.ts
|
Addressed the valid review findings in commit 29c9e3a. Fixed:
Checked and did not change:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/relay.ts (1)
1488-1497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't overwrite an existing handle's runtime with the default
'pty'.
ensureAgentHandle()now always updates existing handles, but itsruntimeparameter still defaults to'pty'. Call sites that omit it currently rewrite a headless handle to PTY on therelay_inboundpath. Preserve the current runtime when none was supplied.Possible fix
private ensureAgentHandle( name: string, - runtime: AgentRuntime = 'pty', + runtime: AgentRuntime | undefined = undefined, channels: string[] = [], metadata?: { sessionId?: string; pid?: number } ): Agent { const existing = this.knownAgents.get(name); if (existing) { this.updateAgentHandle(existing, runtime, channels, metadata); return existing; } - const agent = this.makeAgent(name, runtime, channels, metadata); + const agent = this.makeAgent(name, runtime ?? 'pty', channels, metadata); this.knownAgents.set(name, agent); return agent; } private updateAgentHandle( agent: Agent, - runtime: AgentRuntime, + runtime: AgentRuntime | undefined, channels: string[], metadata?: { sessionId?: string; pid?: number } ): void { const internal = agent as InternalAgent; - internal._setRuntime(runtime); + if (runtime !== undefined) { + internal._setRuntime(runtime); + } if (channels.length > 0) { internal._setChannels([...new Set([...agent.channels, ...channels])]); } internal._setMetadata(metadata); }Also applies to: 1504-1515
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/relay.ts` around lines 1488 - 1497, The ensureAgentHandle function currently defaults runtime to 'pty' which causes existing agent handles to be overwritten when callers omit runtime; change the signature so runtime is optional (no default) and modify the logic so updateAgentHandle(existing, ...) only applies a new runtime when the caller passed one (i.e. check for runtime !== undefined), while when creating a new Agent use runtime ?? 'pty' to fall back to PTY; update both ensureAgentHandle and the corresponding update path (and the other similar block around lines 1504-1515) to preserve existing runtime unless an explicit runtime was provided.
🧹 Nitpick comments (1)
packages/sdk/src/__tests__/lifecycle-hooks.test.ts (1)
218-233: ⚡ Quick winDrop the explicit transport from this regression test.
Because the input already sets
transport: 'headless', the request body stays headless even ifspawnProvider()stops recomputing transport afterbeforeAgentSpawn. Omittingtransporthere would actually lock in the hook-addedharnessPlanas the thing that flips the request to headless.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/lifecycle-hooks.test.ts` around lines 218 - 233, The test is forcing transport to 'headless' which masks the behavior the hook should set; remove the explicit transport property from the spawnProvider call (the call to spawnProvider with name: 'patched-headless' / provider: 'custom-provider') and also remove the transport assertion from the expect(captures[0].body) object so the test verifies the hook-added harnessPlan (sessionId: 'ses_hook', runtime: 'headless', driver: 'app_server') and that spawnProvider/beforeAgentSpawn logic flips transport indirectly via harnessPlan rather than being pre-set by the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/src/harness.ts`:
- Around line 210-223: resolveCommand is returning early for Windows-style paths
like "~\bin\claude" because expandHome currently only expands "~/" and leaves
"~\" intact, so expandedCommand.includes('\\') triggers the direct-path return
before home expansion; fix by updating expandHome to recognize and replace a
leading "~\" as well as "~/" (handling both path separators) so that
resolveCommand's use of expandHome yields a fully expanded path and the
includes('/')/includes('\\') checks operate on the expanded value.
In `@packages/sdk/src/relay.ts`:
- Around line 1195-1198: listAgents() is treating the broker snapshot as a merge
target instead of authoritative state, so updateAgentHandle(existing,
entry.runtime, entry.channels, entry) must stop unioning channels and instead
sync them to the snapshot; change updateAgentHandle (or add a new
syncAgentHandle) to accept a "replaceChannels" behavior and assign
existing.channels = entry.channels (including empty arrays to clear
subscriptions) rather than unions, and ensure other fields keep the existing
update logic (runtime, other metadata) while using the entry.channels from the
snapshot to fully replace agent.channels.
---
Outside diff comments:
In `@packages/sdk/src/relay.ts`:
- Around line 1488-1497: The ensureAgentHandle function currently defaults
runtime to 'pty' which causes existing agent handles to be overwritten when
callers omit runtime; change the signature so runtime is optional (no default)
and modify the logic so updateAgentHandle(existing, ...) only applies a new
runtime when the caller passed one (i.e. check for runtime !== undefined), while
when creating a new Agent use runtime ?? 'pty' to fall back to PTY; update both
ensureAgentHandle and the corresponding update path (and the other similar block
around lines 1504-1515) to preserve existing runtime unless an explicit runtime
was provided.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/lifecycle-hooks.test.ts`:
- Around line 218-233: The test is forcing transport to 'headless' which masks
the behavior the hook should set; remove the explicit transport property from
the spawnProvider call (the call to spawnProvider with name: 'patched-headless'
/ provider: 'custom-provider') and also remove the transport assertion from the
expect(captures[0].body) object so the test verifies the hook-added harnessPlan
(sessionId: 'ses_hook', runtime: 'headless', driver: 'app_server') and that
spawnProvider/beforeAgentSpawn logic flips transport indirectly via harnessPlan
rather than being pre-set by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e823841-65e4-4c7c-9c89-2fe42ece56d1
📒 Files selected for processing (12)
.trajectories/completed/2026-05/traj_0d1efjk6aeo2.json.trajectories/completed/2026-05/traj_0d1efjk6aeo2.md.trajectories/index.jsoncrates/broker/src/protocol.rscrates/broker/src/runtime/app_server.rscrates/broker/src/worker.rsdocs/harness-runtime-plan.mdpackages/sdk/src/__tests__/harness.test.tspackages/sdk/src/__tests__/lifecycle-hooks.test.tspackages/sdk/src/client.tspackages/sdk/src/harness.tspackages/sdk/src/relay.ts
✅ Files skipped from review due to trivial changes (4)
- .trajectories/completed/2026-05/traj_0d1efjk6aeo2.json
- .trajectories/completed/2026-05/traj_0d1efjk6aeo2.md
- docs/harness-runtime-plan.md
- .trajectories/index.json
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/src/relay.ts">
<violation number="1" location="packages/sdk/src/relay.ts:1512">
P2: `updateAgentHandle` cannot clear channels, so agent handles can retain stale channel subscriptions when the broker reports `[]`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Addressed the harness-runtime review follow-ups in 8c429f2: broker now tracks/report harness PID separately from wrapper worker PID, app-server plans validate protocol/auth/attached host settings at spawn, app-server release has enough grace for the 30s HTTP timeout, app-server delivery no longer emits delivery_verified, and harness docs/examples now call out permission-flag ownership and env allowlisting. Validation: cargo check -p agent-relay-broker; cargo test -p agent-relay-broker app_server; cargo test -p agent-relay-broker harness; npm --prefix packages/sdk run check; npm --prefix web run build. |
There was a problem hiding this comment.
1 issue found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/runtime/app_server.rs">
<violation number="1" location="crates/broker/src/runtime/app_server.rs:153">
P2: `delivery_injected` now fires after the HTTP call but still reports the pre-request timestamp, which makes injected timing inaccurate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Renamed the harness output terminology in 21a480e: SDK types/fields now use harnessConfig / ResolvedHarnessConfig / PtyHarnessConfig / HeadlessAppServerHarnessConfig, docs refer to broker-executable harness configs, and the design file is now docs/harness-runtime-config.md. The broker still accepts legacy harnessPlan/harness_plan JSON aliases so older payloads do not fail while the public SDK/docs use the new naming. Validation: cargo check -p agent-relay-broker; cargo test -p agent-relay-broker harness; cargo test -p agent-relay-broker app_server; npm --prefix packages/sdk run check; npx vitest run --config vitest.config.ts src/tests/harness.test.ts src/tests/lifecycle-hooks.test.ts from packages/sdk; npm --prefix web run build. |
|
Added a docs-specific terminology pass in c2629f8: the Harnesses page now defines Harness definition, Harness resolver, Harness config, and the one-off harnessConfig field before examples; the runtime design note has the same naming model and uses resolveHarnessConfig. I also searched docs/content for old harnessPlan / harness plan wording; remaining harnessPlan references are only broker compatibility aliases/tests. Validation: npm --prefix web run build. |
|
Fixed the clippy failures in e02a295. Changes: boxed AgentSpec in the large protocol enum variants without changing serialized JSON shape, replaced the manual env suppression contains check, and moved runtime util tests after non-test items. Validation: cargo clippy -p agent-relay-broker --all-targets -- -D warnings; cargo test -p agent-relay-broker harness; cargo test -p agent-relay-broker app_server. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/sdk/src/__tests__/lifecycle-hooks.test.ts (1)
218-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not actually prove transport recomputation from patched
harnessConfigLine 221 sets
transport: 'headless'up front, so this test still passes without recomputing transport after hook patches. Remove explicit transport from input so the assertion at Line 227 validates recomputation behavior.Suggested test adjustment
await client.spawnProvider({ name: 'patched-headless', provider: 'custom-provider', - transport: 'headless', }); expect(captures[0].body).toMatchObject({ name: 'patched-headless', cli: 'custom-provider', transport: 'headless', harnessConfig: { runtime: 'headless', driver: 'app_server', sessionId: 'ses_hook', }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/lifecycle-hooks.test.ts` around lines 218 - 233, The test currently passes transport explicitly so it doesn't verify recomputation; update the test that calls client.spawnProvider (in packages/sdk/src/__tests__/lifecycle-hooks.test.ts) to remove the explicit transport field from the input object (leave name:'patched-headless' and provider:'custom-provider' but omit transport), so that transport must be derived from the patched harnessConfig applied by the hook; keep the assertion on captures[0].body.transport === 'headless' and ensure the harnessConfig patch (sessionId:'ses_hook', runtime:'headless', driver:'app_server') is still applied by the hook so the test verifies recomputation.crates/broker/src/listen_api.rs (1)
652-671:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat explicit
nullharness configs as absent.An explicit
"harnessConfig": nullcurrently returns 400 because key presence takes the parse branch, even though this field is optional. That's a common payload shape from JS clients and should behave the same as omission.Suggested fix
let harness_config = match body .get("harness_config") .or_else(|| body.get("harnessConfig")) .or_else(|| body.get("harness_plan")) .or_else(|| body.get("harnessPlan")) .cloned() { + Some(Value::Null) => None, Some(value) => match serde_json::from_value::<ResolvedHarnessConfig>(value) { Ok(config) => Some(config), Err(error) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/listen_api.rs` around lines 652 - 671, The current harness_config branch treats an explicit JSON null as a present value and attempts to parse it, producing a 400; update the match handling for the cloned value from body.get(...).cloned() so that if value.is_null() it is treated as None (absent) instead of attempting serde_json::from_value. In other words, in the Some(value) arm of the harness_config handling (the block that calls serde_json::from_value::<ResolvedHarnessConfig>(value)), first check value.is_null() and return None when true; only call from_value when value is non-null and retain the existing error response on parse failure.crates/broker/src/protocol.rs (1)
343-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
BrokerEvent::AgentSpawned.session_idwire name + omit-None behavior
- With
#[serde(tag = "kind", rename_all = "snake_case")],session_idwill serialize assession_id(notsessionId), so SDK consumers expectingsessionIdwon’t match.#[serde(default)] session_id: Option<String>will serializeNoneasnull; if the contract expects the field to be omitted when absent, addskip_serializing_if = "Option::is_none".Suggested fix
- #[serde(default)] + #[serde( + default, + rename = "sessionId", + alias = "session_id", + skip_serializing_if = "Option::is_none" + )] session_id: Option<String>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/protocol.rs` around lines 343 - 344, The BrokerEvent::AgentSpawned.session_id field currently serializes as snake_case and emits nulls; update its serde attributes to force the wire name to "sessionId" and omit None values by adding serde(rename = "sessionId", skip_serializing_if = "Option::is_none") (you can keep serde(default) if needed for deserialization), i.e., adjust the attributes on the session_id field so consumers receive "sessionId" and absent values are omitted.crates/broker/src/runtime/app_server.rs (1)
168-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t mark ambiguous
prompt_asyncPOST failures as retryable.
send_opencode_prompt()is a non-idempotentPOST, but this error branch always emitsretryable: true. If the request times out or the connection drops after the app-server has already accepted the prompt, the broker can replay the same delivery and inject it twice into the session. Please either add an idempotency key derived fromdelivery_id/event_id, or make these transport failures non-retryable until the downstream API can dedupe them.Also applies to: 285-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/runtime/app_server.rs` around lines 168 - 189, The error branch that reports delivery failures currently marks them as retryable even though send_opencode_prompt (a non-idempotent POST) may have already been accepted; update the worker_error payload emitted by the delivery failure path (the send_frame call that sends "worker_error") to set "retryable": false (or alternatively add an idempotency key derived from delivery_id/event_id and include it in the payload) so transports do not blindly retry ambiguous POST failures; apply the same change to the other identical branch around the 285-302 area; look for the send_frame calls that emit "delivery_failed" and the subsequent send_frame that emits "worker_error" to locate where to change the payload.
♻️ Duplicate comments (2)
packages/sdk/src/harness.ts (1)
215-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpand
~\...before the direct-path fast path.
resolveCommand()still returns early for Windows-style home-relative paths becauseexpandHome()only rewrites~/.... A command like~\bin\claudeis therefore passed through unexpanded.Possible fix
function expandHome(value: string): string { if (value === '~') return homedir(); - if (value.startsWith('~/')) return path.join(homedir(), value.slice(2)); + if (value.startsWith('~/') || value.startsWith('~\\')) { + return path.join(homedir(), value.slice(2)); + } return value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/harness.ts` around lines 215 - 233, resolveCommand currently expands home only for POSIX style '~/...', so Windows-style home-relative paths like '~\bin\claude' slip through unexpanded; update expandHome to also handle backslash prefixes (e.g., check startsWith('~\\') and return path.join(homedir(), value.slice(2))) while keeping the existing '~' and '~/’ behavior so resolveCommand's direct-path fast path sees an expanded path; modify the expandHome function to cover both '~/...' and '~\...' cases.packages/sdk/src/relay.ts (1)
1193-1198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync
listAgents()channels to the broker snapshot.
updateAgentHandle()still unions channels and ignores empty snapshots. WhenlistAgents()refreshes an existing handle after an unsubscribe, stale channels remain onagent.channels, which can also sendresolveChannel()to the wrong target.Possible fix
- this.updateAgentHandle(existing, entry.runtime, entry.channels, entry); + this.updateAgentHandle(existing, entry.runtime, entry.channels, entry, true); return existing; } @@ private updateAgentHandle( agent: Agent, runtime: AgentRuntime, channels: string[], - metadata?: { sessionId?: string; pid?: number } + metadata?: { sessionId?: string; pid?: number }, + replaceChannels = false ): void { const internal = agent as InternalAgent; internal._setRuntime(runtime); - if (channels.length > 0) { + if (replaceChannels) { + internal._setChannels([...channels]); + } else if (channels.length > 0) { internal._setChannels([...new Set([...agent.channels, ...channels])]); } internal._setMetadata(metadata); }Also applies to: 1504-1516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/relay.ts` around lines 1193 - 1198, listAgents() currently refreshes existing handles by calling updateAgentHandle() which unions channels and ignores empty snapshots, leaving stale channels after an unsubscribe; change the update logic so that updateAgentHandle(existing, entry.runtime, entry.channels, entry) replaces existing.channels with the snapshot (even if empty) rather than unioning, and ensure updateAgentHandle and any code that uses agent.channels (e.g., resolveChannel()) treat the provided channels array as authoritative so stale channels are removed when entry.channels is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/broker/src/pty.rs`:
- Around line 464-466: The child_pid() accessor currently returns only the
spawn-time cached self.child_pid which can be stale; change child_pid() to, when
self.child_pid is None, call the existing process_id() (the same mechanism used
by has_exited()) to attempt to discover a PID discovered later, update the
cached self.child_pid with the discovered value, and then return the Option<u32>
so downstream fields like worker_ready.pid get populated when possible; use the
same helper logic/pattern as has_exited() to avoid duplication.
In `@crates/broker/src/worker.rs`:
- Around line 898-906: validate_app_server_config() currently allows plain
"http" endpoints and will attach credentials; change the validation around
parsed_endpoint/scheme so that if the URL scheme is "http" you reject it unless
the host is loopback (e.g., host is "localhost", "127.0.0.1" or "::1");
additionally, if any authentication is configured on the app server (e.g.,
config.auth, config.basic_auth, or whatever field holds bearer/basic creds),
always require "https" and bail with an error mentioning config.endpoint when
auth is present and scheme != "https"; apply the same guard to the other
validation branch that inspects the endpoint (the spawn-related code referenced
nearby) so credentials are only sent over HTTPS or to loopback HTTP.
---
Outside diff comments:
In `@crates/broker/src/listen_api.rs`:
- Around line 652-671: The current harness_config branch treats an explicit JSON
null as a present value and attempts to parse it, producing a 400; update the
match handling for the cloned value from body.get(...).cloned() so that if
value.is_null() it is treated as None (absent) instead of attempting
serde_json::from_value. In other words, in the Some(value) arm of the
harness_config handling (the block that calls
serde_json::from_value::<ResolvedHarnessConfig>(value)), first check
value.is_null() and return None when true; only call from_value when value is
non-null and retain the existing error response on parse failure.
In `@crates/broker/src/protocol.rs`:
- Around line 343-344: The BrokerEvent::AgentSpawned.session_id field currently
serializes as snake_case and emits nulls; update its serde attributes to force
the wire name to "sessionId" and omit None values by adding serde(rename =
"sessionId", skip_serializing_if = "Option::is_none") (you can keep
serde(default) if needed for deserialization), i.e., adjust the attributes on
the session_id field so consumers receive "sessionId" and absent values are
omitted.
In `@crates/broker/src/runtime/app_server.rs`:
- Around line 168-189: The error branch that reports delivery failures currently
marks them as retryable even though send_opencode_prompt (a non-idempotent POST)
may have already been accepted; update the worker_error payload emitted by the
delivery failure path (the send_frame call that sends "worker_error") to set
"retryable": false (or alternatively add an idempotency key derived from
delivery_id/event_id and include it in the payload) so transports do not blindly
retry ambiguous POST failures; apply the same change to the other identical
branch around the 285-302 area; look for the send_frame calls that emit
"delivery_failed" and the subsequent send_frame that emits "worker_error" to
locate where to change the payload.
In `@packages/sdk/src/__tests__/lifecycle-hooks.test.ts`:
- Around line 218-233: The test currently passes transport explicitly so it
doesn't verify recomputation; update the test that calls client.spawnProvider
(in packages/sdk/src/__tests__/lifecycle-hooks.test.ts) to remove the explicit
transport field from the input object (leave name:'patched-headless' and
provider:'custom-provider' but omit transport), so that transport must be
derived from the patched harnessConfig applied by the hook; keep the assertion
on captures[0].body.transport === 'headless' and ensure the harnessConfig patch
(sessionId:'ses_hook', runtime:'headless', driver:'app_server') is still applied
by the hook so the test verifies recomputation.
---
Duplicate comments:
In `@packages/sdk/src/harness.ts`:
- Around line 215-233: resolveCommand currently expands home only for POSIX
style '~/...', so Windows-style home-relative paths like '~\bin\claude' slip
through unexpanded; update expandHome to also handle backslash prefixes (e.g.,
check startsWith('~\\') and return path.join(homedir(), value.slice(2))) while
keeping the existing '~' and '~/’ behavior so resolveCommand's direct-path fast
path sees an expanded path; modify the expandHome function to cover both '~/...'
and '~\...' cases.
In `@packages/sdk/src/relay.ts`:
- Around line 1193-1198: listAgents() currently refreshes existing handles by
calling updateAgentHandle() which unions channels and ignores empty snapshots,
leaving stale channels after an unsubscribe; change the update logic so that
updateAgentHandle(existing, entry.runtime, entry.channels, entry) replaces
existing.channels with the snapshot (even if empty) rather than unioning, and
ensure updateAgentHandle and any code that uses agent.channels (e.g.,
resolveChannel()) treat the provided channels array as authoritative so stale
channels are removed when entry.channels is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fce1a56-8956-4b1d-baba-b4fe3b3542db
📒 Files selected for processing (30)
.trajectories/completed/2026-05/traj_9dj3qiugt26j.json.trajectories/completed/2026-05/traj_9dj3qiugt26j.md.trajectories/completed/2026-05/traj_q2r3c9dmdep7.json.trajectories/completed/2026-05/traj_q2r3c9dmdep7.md.trajectories/index.jsonCHANGELOG.mdcrates/broker/src/cli/mod.rscrates/broker/src/listen_api.rscrates/broker/src/protocol.rscrates/broker/src/pty.rscrates/broker/src/pty_worker.rscrates/broker/src/runtime/api.rscrates/broker/src/runtime/app_server.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/spawn_spec.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/supervisor.rscrates/broker/src/worker.rsdocs/harness-runtime-config.mdpackages/sdk/src/__tests__/harness.test.tspackages/sdk/src/__tests__/lifecycle-hooks.test.tspackages/sdk/src/client.tspackages/sdk/src/harness.tspackages/sdk/src/lifecycle-hooks.tspackages/sdk/src/protocol.tspackages/sdk/src/relay.tspackages/sdk/src/types.tsweb/content/docs/harnesses.mdx
✅ Files skipped from review due to trivial changes (5)
- .trajectories/completed/2026-05/traj_9dj3qiugt26j.json
- .trajectories/index.json
- .trajectories/completed/2026-05/traj_q2r3c9dmdep7.json
- CHANGELOG.md
- web/content/docs/harnesses.mdx
|
Made headless harness driver optional in 75f019c. When runtime is headless, the broker defaults missing driver to app_server during deserialization, and the SDK static resolver defaults missing driver to app_server as well. Docs/examples now omit driver for OpenCode headless configs. Validation: cargo check -p agent-relay-broker; npm --prefix packages/sdk run check; npm --prefix web run build; cargo test -p agent-relay-broker harness; cargo test -p agent-relay-broker app_server; cargo clippy -p agent-relay-broker --all-targets -- -D warnings; targeted SDK vitest for harness/lifecycle hooks. |
|
Updated the PR to remove the durable SDK resolver shape and make harnesses broker-executable data instead. Changes pushed in 4fdda79:
Validation: cargo check/clippy/tests for broker harness/listen_api/app_server/relaycast selection, SDK typecheck and harness/lifecycle tests, and web build. The web build succeeded but logged sandbox DNS warnings for api.github.com fetches during static generation. |
There was a problem hiding this comment.
1 issue found across 20 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Agreed on the footgun. I removed the broker-local harness registry and pushed e86f7d0. Current shape:
Validation rerun: SDK typecheck + harness/lifecycle vitest, cargo check, clippy, broker harness/listen_api/app_server/relaycast tests, and web build. |
|
Resolved the merge conflicts by merging Review follow-ups addressed in that push:
Validation run:
GitHub now reports the branch is no longer dirty; PR is blocked only by review/check requirements. |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Rename/move trajectory files from the top-level .trajectories tree into the .agentworkforce/trajectories directory, preserving compacted and completed subdirectories and filenames. This reorganizes storage of JSON/MD/trace files and includes the addition of traj_9d5mdl0oj0ao.{json,md} in completed/2026-05.
This reverts commit 7dd7af6.
1 participant
Summary
Test Plan
Validation