Adopt AI SDK harness#1282
Open
willwashburn wants to merge 4 commits into
Open
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
📝 WalkthroughWalkthroughThis PR establishes Node.js 22 as the runtime baseline and adds AI SDK native harness support through adapter registries, broker-supervised sidecars, versioned protocols, replayable events, observability reducers, CLI attach modes, Relaycast publication, and integration/soak validation. ChangesAI SDK native harness adoption
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
38 issues found and verified against the latest diff
Not reviewed (too large): .agentworkforce/trajectories/completed/2026-07/traj_ajse65eu6brc.trace.json (~2,206 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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="tests/integration/ai-sdk-harnesses/real-adapters.test.ts">
<violation number="1" location="tests/integration/ai-sdk-harnesses/real-adapters.test.ts:64">
P1: When preflight fails or `createHarness()` throws before `host` is assigned, the `finally` block crashes with `TypeError: Cannot read properties of undefined (reading 'catch')` because `host?.destroy()` short-circuits to `undefined`, and `.catch()` is called on that `undefined` value. This either masks the real error or blows up the skip path.</violation>
</file>
<file name=".workflow-artifacts/plan-001/final-signoff-3.md">
<violation number="1" location=".workflow-artifacts/plan-001/final-signoff-3.md:5">
P2: This signoff can be consumed as an approval even though the required settled-tree full-suite gate has not run after the final repairs. Marking the verdict conditional (or withholding approval until the results are recorded) would keep the artifact consistent with its own verification evidence.</violation>
</file>
<file name="packages/sdk/src/session/types.ts">
<violation number="1" location="packages/sdk/src/session/types.ts:521">
P2: A `file.changed` update can pass through the AI SDK mapper at runtime even though `CanonicalAgentSessionEvent` forbids `operation: 'update'`. Preserving `update` in the canonical type or normalizing it to `modify` before publication would prevent this type/runtime contract mismatch.</violation>
</file>
<file name="packages/harnesses/src/define.ts">
<violation number="1" location="packages/harnesses/src/define.ts:137">
P2: Custom PTY harnesses now fail during descriptor creation because observability lookup only recognizes the fixed built-in command names. Keeping `definePtyHarness` generic requires an unavailable/generic PTY profile for unknown commands, or an optional profile lookup, rather than throwing for valid custom definitions.</violation>
<violation number="2" location="packages/harnesses/src/define.ts:364">
P2: Managed PTY factories drop all optional fields from the supplied `ptyDefinition`, so fleet spawns silently lose configured arguments, environment, working directory, session ID, delivery, and metadata. Preserving the `StaticPtyHarnessDefinition` fields in this returned object keeps the declared `ManagedPtyHarness` contract and the existing spawn configuration behavior.</violation>
</file>
<file name=".workflow-artifacts/plan-001/final-signoff.md">
<violation number="1" location=".workflow-artifacts/plan-001/final-signoff.md:11">
P1: Plan 001 is incorrectly rejected for missing Relaycast semantic publication: the broker already forwards canonical semantic frames to Relaycast through `run_hosted_agent_event_publisher`. Please remove or rewrite this P1 finding instead of requiring a repair that is already implemented.</violation>
<violation number="2" location=".workflow-artifacts/plan-001/final-signoff.md:24">
P1: Plan 001 is incorrectly rejected for a dead PTY translator: live PTY spawns already translate broker boundaries into canonical local events, while the broker publishes the same PTY profile through Relaycast. Please remove or rewrite this P1 finding to reflect the implemented path.</violation>
</file>
<file name="packages/harnesses/src/ai-sdk/local-host-sandbox.ts">
<violation number="1" location="packages/harnesses/src/ai-sdk/local-host-sandbox.ts:349">
P2: Removing a leased port with `setPorts([])` leaks its loopback reservation, so the session cannot release that port on stop and the provider keeps holding it. Releasing removed ports before replacing `#ports` would keep port ownership consistent.</violation>
</file>
<file name="plans/001-adopt-ai-sdk-harnesses.md">
<violation number="1" location="plans/001-adopt-ai-sdk-harnesses.md:452">
P1: A green real-adapter run can contain no authenticated execution because unavailable CLIs and credentials are skipped by adapter name, so it cannot prove the stated promotion gate. Keep skips for ordinary CI, but make promotion mode fail unless every target adapter has a non-skipped authenticated run.</violation>
</file>
<file name=".workflow-artifacts/plan-001/claude-review-1.md">
<violation number="1" location=".workflow-artifacts/plan-001/claude-review-1.md:15">
P2: This finding would incorrectly block the change: duplicate `deliver_relay` frames are acknowledged twice while `RelayHarnessSession` injects once. The implementation and existing sidecar test already cover the claimed redelivery behavior, so this P1 should be removed or revised.</violation>
<violation number="2" location=".workflow-artifacts/plan-001/claude-review-1.md:35">
P2: Semantic startup history is not erased by `agent_spawned`: the replay buffer resets the previous generation before launch and treats the later broadcast as observational. This P1 is contradicted by the implementation and `semantic_startup_events_before_spawn_broadcast_remain_replayable` test, so it should be removed or revised.</violation>
<violation number="3" location=".workflow-artifacts/plan-001/claude-review-1.md:53">
P2: Semantic sidecars already publish capability discovery, mapped events, and reducer transitions through the broker-facing output. The existing sidecar test covers those outputs, so this P1's reproduction is false and should be removed or revised.</violation>
<violation number="4" location=".workflow-artifacts/plan-001/claude-review-1.md:71">
P2: The public runtime metadata does not advertise the full registry lifecycle: inaccessible operations are explicitly reported as `false`. Therefore the claimed `detach: true`/`stop: true` discovery path is not present; this finding should distinguish internal adapter capabilities from public metadata or be removed.</violation>
</file>
<file name=".workflow-artifacts/plan-001/final-signoff-2.md">
<violation number="1" location=".workflow-artifacts/plan-001/final-signoff-2.md:20">
P1: The signoff’s P1 rejects supported CLI/fleet/direct PTY spawns as unobservable, but the broker already emits canonical PTY events for those paths. This section should be removed or narrowed to wrapper-local translation coverage rather than requiring another broker-owned publisher.</violation>
<violation number="2" location=".workflow-artifacts/plan-001/final-signoff-2.md:34">
P2: The signoff treats loss during a Relaycast outage as a blocker even though this PR explicitly defines hosted high-frequency observability as bounded best-effort, not a durable audit ledger. Unless another public surface promises durability, this should be recorded as accepted contract behavior (with optional drop metrics), not a required persisted outbox repair.</violation>
</file>
<file name="crates/broker/src/runtime/worker_events.rs">
<violation number="1" location="crates/broker/src/runtime/worker_events.rs:208">
P2: PTY workers that become idle before producing a stream never publish the canonical `idle` activity, leaving observers stuck at `starting`. The idle transition should be emitted whenever the current activity is not already idle; emit `turn.settled` only when an active turn exists.</violation>
</file>
<file name="packages/cli/src/cli/commands/local-agent.ts">
<violation number="1" location="packages/cli/src/cli/commands/local-agent.ts:60">
P2: A broker failure while probing the agent type now rejects `runAttach` before `attachView`/`attachDrive` can report the error, causing `local agent attach` to fail as an uncaught promise instead of returning its normal nonzero exit path. Treat probe failures as non-semantic (or handle the rejection) so the existing attach error handling remains effective.</violation>
</file>
<file name="packages/sdk/src/session/activity-reducer.ts">
<violation number="1" location="packages/sdk/src/session/activity-reducer.ts:98">
P2: Parallel same-named tool calls without a stable `callId` or `run` are collapsed into one active entry, so completing one call can make activity stop reporting `using_tool` while the other is still running. The reducer should require or create a per-call correlation key and only remove the matching observation on completion.</violation>
<violation number="2" location="packages/sdk/src/session/activity-reducer.ts:211">
P2: Late events from an earlier turn can corrupt the current activity because text, reasoning, and tool cases never validate the event's provenance against the active `turnId`. Filtering turn-scoped work events to the current turn (and ignoring them after the turn/session is no longer active) would prevent stale stream callbacks from producing false busy states.</violation>
</file>
<file name="packages/sdk/src/agent-relay.ts">
<violation number="1" location="packages/sdk/src/agent-relay.ts:408">
P2: Publishing with a Relaycast agent ID does not reliably target the agent's durable session-event stream because `sessionEvents.emit` is name-keyed. Resolve the agent name before calling the Relaycast API, or change this API to accept an agent name explicitly and keep the local ID separate.</violation>
<violation number="2" location="packages/sdk/src/agent-relay.ts:499">
P2: Agent-scoped clients propagate Relaycast session-event failures, unlike `AgentRelay.publishSessionEvent`, which reports and absorbs them. Applying the same best-effort/error-hook handling here would keep both `AgentRelayAgent` implementations consistent during Relaycast outages.</violation>
</file>
<file name="packages/harnesses/src/ai-sdk/relay-session.ts">
<violation number="1" location="packages/harnesses/src/ai-sdk/relay-session.ts:396">
P2: `next-tool-call` deliveries are executed immediately instead of waiting for the next tool call, so the advertised delivery mode is not honored. Deferring this mode until a `tool.called` event (or removing it from the declared modes) would keep the session contract accurate.</violation>
<violation number="2" location="packages/harnesses/src/ai-sdk/relay-session.ts:399">
P2: Failed deliveries from `startTurn`, `submitUserMessage`, or a full queue are invisible to `onEvent` observers even though `delivery.failed` is declared in `events.emits`. Emitting the failure event for every failed receipt would keep listener/replay consumers consistent with the returned receipt.</violation>
</file>
<file name=".workflow-artifacts/plan-001/final-signoff-resolution.md">
<violation number="1" location=".workflow-artifacts/plan-001/final-signoff-resolution.md:5">
P1: Live harness observability is not published to Relaycast despite this signoff: both semantic sidecar and PTY paths call `emitSessionEvent`, which is local-only, so `messaging.sessionEvents.list` cannot expose those events. Route both paths through the hosted publication/queue path while preserving local listener delivery.</violation>
</file>
<file name="packages/cli/src/cli/lib/attach-semantic.ts">
<violation number="1" location="packages/cli/src/cli/lib/attach-semantic.ts:236">
P2: When the semantic worker exits, `attachSemantic` can hang forever because the live iterator is only stopped by SIGINT, `/detach`, or a closed stream; broker lifecycle exit events are ignored. Handling the worker-exit/error events and stopping the iterator would let `view`/`drive` terminate when the harness is gone.</violation>
<violation number="2" location="packages/cli/src/cli/lib/attach-semantic.ts:251">
P2: After `/detach`, Ctrl+C, or normal completion, this command removes the semantic listener but leaves the broker event WebSocket/reconnect loop alive, which can keep the CLI process running and continue network activity. Disconnecting the event transport during teardown would release the client resources.</violation>
</file>
<file name="crates/broker/src/runtime/event_loop.rs">
<violation number="1" location="crates/broker/src/runtime/event_loop.rs:38">
P2: Workspace-specific observability events can be published to the default Relaycast workspace when their `WorkspaceId` is absent from `clients`, because this fallback treats an unresolved `Some(workspace_id)` like an intentionally default-routed event. Keeping the default fallback only for `None` and dropping/logging unresolved workspace IDs avoids cross-workspace event misattribution.</violation>
</file>
<file name="packages/harness-driver/src/protocol.ts">
<violation number="1" location="packages/harness-driver/src/protocol.ts:635">
P2: Semantic commands and their responses can be represented without the top-level request ID required by the sidecar and broker, causing a command to be ignored or an acknowledgement to be dropped. Mark `request_id` required in both semantic command variants so typed producers cannot construct an unusable frame.</violation>
</file>
<file name="packages/sdk/src/__tests__/activity-reducer.test.ts">
<violation number="1" location="packages/sdk/src/__tests__/activity-reducer.test.ts:43">
P1: Duplicate `text.started` event with the same sequence number (3) will be silently dropped by the reducer's deduplication, so the assertion on `result.transition` will fail: after `reduce()` processes `text.started` with `observation(3)`, `lastSequence` becomes 3, and the subsequent `reduceAgentActivity()` call with the same `observation(3)` returns `{ state: previous }` with an undefined transition instead of the expected `activity.changed`. Remove the redundant first `reduce()` call or use a distinct sequence for one of the two events.</violation>
</file>
<file name="packages/harness-driver/src/broker-driver.ts">
<violation number="1" location="packages/harness-driver/src/broker-driver.ts:68">
P2: High-output PTY workers can grow this buffer without bound when no observer is attached yet: every matching `worker_stream` chunk is retained until `observeBrokerEvents` or `release`. A bounded queue/drop policy would preserve the spawn race fix without turning delayed observation into an unbounded memory sink.</violation>
<violation number="2" location="packages/harness-driver/src/broker-driver.ts:192">
P2: One rejected semantic listener silently suppresses all subsequent semantic events until the runtime closes, and `onError` is delayed until shutdown. Handling listener failures per event—like `observeBrokerEvents` does—would notify `onError` and either continue delivery or explicitly close the observer.</violation>
<violation number="3" location="packages/harness-driver/src/broker-driver.ts:197">
P2: A failed semantic-history request leaks both the live semantic subscription and the runtime-exit listener because setup exits before a disposer is returned. Wrapping stream/history setup in cleanup handling would prevent later broker events and reconnects from retaining this observer.</violation>
</file>
<file name=".workflow-artifacts/plan-001/codex-review-1.md">
<violation number="1" location=".workflow-artifacts/plan-001/codex-review-1.md:97">
P2: The executed-evidence conclusion incorrectly says the green suites do not exercise symlink traversal or conflicting idempotency-key reuse, even though both cases have dedicated tests in the repository. Please correct this coverage statement and limit the untested list to the delayed competitor and end-to-end listener scenarios that are actually missing.</violation>
</file>
<file name="packages/harnesses/src/ai-sdk/harness-host.ts">
<violation number="1" location="packages/harnesses/src/ai-sdk/harness-host.ts:369">
P1: An initial `continueFrom` is never actually continued: `start()` passes it to `doStart()` but does not call `doContinueTurn()`, so recovering an in-flight turn through the constructor silently loses the continuation path. The startup flow could invoke the continuation and return its `HarnessTurnHandle`, or reject this option instead of accepting it.</violation>
<violation number="2" location="packages/harnesses/src/ai-sdk/harness-host.ts:535">
P1: A failed `doContinueTurn()` leaves the host stuck in `running` with a stale turn and no control, making the session unusable after a transient adapter error. Handling this call like `doPromptTurn()`—resetting the generation-bound turn state, emitting the error, and restoring an appropriate state—would preserve lifecycle recovery.</violation>
<violation number="3" location="packages/harnesses/src/ai-sdk/harness-host.ts:536">
P1: Continued turns ignore the host-level `abortSignal`, unlike normal prompt turns, so cancellation of the host does not stop an in-flight continuation. Passing the same combined signal used by `startTurn()` would keep both lifecycle paths cancellable.</violation>
</file>
<file name="packages/harness-driver/src/driver-types.ts">
<violation number="1" location="packages/harness-driver/src/driver-types.ts:25">
P2: Action-based `agent.create` calls cannot use the new harness launch contract: `createAgentSchema` rejects `harnessConfig` before the driver receives it. Adding `harnessConfig` to that schema with the runtime-specific shape would allow semantic sidecar spawns through the public action API.</violation>
</file>
<file name="packages/harnesses/src/ai-sdk/sidecar.ts">
<violation number="1" location="packages/harnesses/src/ai-sdk/sidecar.ts:162">
P2: Deferred deliveries that fail after being queued never receive a `delivery_failed` frame, so the broker keeps them pending and may retry or dead-letter them instead of completing the delivery workflow. Handling `delivery.failed` here should clear the pending entry and send the corresponding failure frame.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| throw error; | ||
| } finally { | ||
| await host?.destroy().catch(() => undefined); |
Contributor
There was a problem hiding this comment.
P1: When preflight fails or createHarness() throws before host is assigned, the finally block crashes with TypeError: Cannot read properties of undefined (reading 'catch') because host?.destroy() short-circuits to undefined, and .catch() is called on that undefined value. This either masks the real error or blows up the skip path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration/ai-sdk-harnesses/real-adapters.test.ts, line 64:
<comment>When preflight fails or `createHarness()` throws before `host` is assigned, the `finally` block crashes with `TypeError: Cannot read properties of undefined (reading 'catch')` because `host?.destroy()` short-circuits to `undefined`, and `.catch()` is called on that `undefined` value. This either masks the real error or blows up the skip path.</comment>
<file context>
@@ -0,0 +1,71 @@
+ }
+ throw error;
+ } finally {
+ await host?.destroy().catch(() => undefined);
+ await rm(root, { recursive: true, force: true });
+ }
</file context>
|
|
||
| ## Findings | ||
|
|
||
| ### P1 - Semantic events reach the local `AgentRelay` listener hub, but are still not published by Relaycast |
Contributor
There was a problem hiding this comment.
P1: Plan 001 is incorrectly rejected for missing Relaycast semantic publication: the broker already forwards canonical semantic frames to Relaycast through run_hosted_agent_event_publisher. Please remove or rewrite this P1 finding instead of requiring a repair that is already implemented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .workflow-artifacts/plan-001/final-signoff.md, line 11:
<comment>Plan 001 is incorrectly rejected for missing Relaycast semantic publication: the broker already forwards canonical semantic frames to Relaycast through `run_hosted_agent_event_publisher`. Please remove or rewrite this P1 finding instead of requiring a repair that is already implemented.</comment>
<file context>
@@ -0,0 +1,81 @@
+
+## Findings
+
+### P1 - Semantic events reach the local `AgentRelay` listener hub, but are still not published by Relaycast
+
+The Codex resolution is only partially true.
</file context>
|
|
||
| Required repair: publish the canonical envelopes through the authenticated Relaycast transport with stable public event names and wire round-trip tests, or change the plan/docs/changelog to describe the local broker/SDK observer plane accurately. The Plan 001 done criterion itself currently requires the former. | ||
|
|
||
| ### P1 - PTY observability is a dead translator, so PTY runtimes emit none of the advertised canonical profile |
Contributor
There was a problem hiding this comment.
P1: Plan 001 is incorrectly rejected for a dead PTY translator: live PTY spawns already translate broker boundaries into canonical local events, while the broker publishes the same PTY profile through Relaycast. Please remove or rewrite this P1 finding to reflect the implemented path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .workflow-artifacts/plan-001/final-signoff.md, line 24:
<comment>Plan 001 is incorrectly rejected for a dead PTY translator: live PTY spawns already translate broker boundaries into canonical local events, while the broker publishes the same PTY profile through Relaycast. Please remove or rewrite this P1 finding to reflect the implemented path.</comment>
<file context>
@@ -0,0 +1,81 @@
+
+Required repair: publish the canonical envelopes through the authenticated Relaycast transport with stable public event names and wire round-trip tests, or change the plan/docs/changelog to describe the local broker/SDK observer plane accurately. The Plan 001 done criterion itself currently requires the former.
+
+### P1 - PTY observability is a dead translator, so PTY runtimes emit none of the advertised canonical profile
+
+- `packages/harnesses/src/observability.ts:118-207` defines `createPtyObservabilityTranslator`.
</file context>
|
|
||
| ## Hosted Relaycast transport | ||
|
|
||
| - Canonical semantic sidecar frames are enqueued independently for Relaycast's authenticated `POST /v1/agents/:name/events` SDK surface while remaining available on the local broker stream. |
Contributor
There was a problem hiding this comment.
P1: Live harness observability is not published to Relaycast despite this signoff: both semantic sidecar and PTY paths call emitSessionEvent, which is local-only, so messaging.sessionEvents.list cannot expose those events. Route both paths through the hosted publication/queue path while preserving local listener delivery.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .workflow-artifacts/plan-001/final-signoff-resolution.md, line 5:
<comment>Live harness observability is not published to Relaycast despite this signoff: both semantic sidecar and PTY paths call `emitSessionEvent`, which is local-only, so `messaging.sessionEvents.list` cannot expose those events. Route both paths through the hosted publication/queue path while preserving local listener delivery.</comment>
<file context>
@@ -0,0 +1,37 @@
+
+## Hosted Relaycast transport
+
+- Canonical semantic sidecar frames are enqueued independently for Relaycast's authenticated `POST /v1/agents/:name/events` SDK surface while remaining available on the local broker stream.
+- `AgentRelay.publishSessionEvent` delivers a canonical event to local listeners and Relaycast durable agent-event storage. `messaging.sessionEvents.list` exposes the upstream read surface to other Relay applications.
+- A loopback HTTP round-trip asserts the stable event type and canonical payload sent to Relaycast.
</file context>
| it('follows the reference lifecycle and waits for turn settlement before becoming idle', () => { | ||
| let state = createAgentActivityState(); | ||
|
|
||
| let result = reduceAgentActivity(state, { |
Contributor
There was a problem hiding this comment.
P1: Duplicate text.started event with the same sequence number (3) will be silently dropped by the reducer's deduplication, so the assertion on result.transition will fail: after reduce() processes text.started with observation(3), lastSequence becomes 3, and the subsequent reduceAgentActivity() call with the same observation(3) returns { state: previous } with an undefined transition instead of the expected activity.changed. Remove the redundant first reduce() call or use a distinct sequence for one of the two events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/__tests__/activity-reducer.test.ts, line 43:
<comment>Duplicate `text.started` event with the same sequence number (3) will be silently dropped by the reducer's deduplication, so the assertion on `result.transition` will fail: after `reduce()` processes `text.started` with `observation(3)`, `lastSequence` becomes 3, and the subsequent `reduceAgentActivity()` call with the same `observation(3)` returns `{ state: previous }` with an undefined transition instead of the expected `activity.changed`. Remove the redundant first `reduce()` call or use a distinct sequence for one of the two events.</comment>
<file context>
@@ -0,0 +1,307 @@
+ it('follows the reference lifecycle and waits for turn settlement before becoming idle', () => {
+ let state = createAgentActivityState();
+
+ let result = reduceAgentActivity(state, {
+ type: 'session.starting',
+ observability: observation(1),
</file context>
| if (ports.some((port) => !this.#ports.includes(port))) { | ||
| throw new Error('Local sandbox ports are exclusively allocated by the provider'); | ||
| } | ||
| this.#ports = [...ports]; |
Contributor
There was a problem hiding this comment.
P2: Removing a leased port with setPorts([]) leaks its loopback reservation, so the session cannot release that port on stop and the provider keeps holding it. Releasing removed ports before replacing #ports would keep port ownership consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/harnesses/src/ai-sdk/local-host-sandbox.ts, line 349:
<comment>Removing a leased port with `setPorts([])` leaks its loopback reservation, so the session cannot release that port on stop and the provider keeps holding it. Releasing removed ports before replacing `#ports` would keep port ownership consistent.</comment>
<file context>
@@ -0,0 +1,551 @@
+ if (ports.some((port) => !this.#ports.includes(port))) {
+ throw new Error('Local sandbox ports are exclusively allocated by the provider');
+ }
+ this.#ports = [...ports];
+ }
+
</file context>
| return attachDrive(name, options); | ||
| } | ||
| export function runAttach(name: string, mode: AttachMode, options: SemanticAttachOptions): Promise<number> { | ||
| return isSemanticHarness(name, options).then((semantic) => { |
Contributor
There was a problem hiding this comment.
P2: A broker failure while probing the agent type now rejects runAttach before attachView/attachDrive can report the error, causing local agent attach to fail as an uncaught promise instead of returning its normal nonzero exit path. Treat probe failures as non-semantic (or handle the rejection) so the existing attach error handling remains effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/commands/local-agent.ts, line 60:
<comment>A broker failure while probing the agent type now rejects `runAttach` before `attachView`/`attachDrive` can report the error, causing `local agent attach` to fail as an uncaught promise instead of returning its normal nonzero exit path. Treat probe failures as non-semantic (or handle the rejection) so the existing attach error handling remains effective.</comment>
<file context>
@@ -55,32 +56,27 @@ export type AttachMode = 'drive' | 'view' | 'passthrough';
- return attachDrive(name, options);
- }
+export function runAttach(name: string, mode: AttachMode, options: SemanticAttachOptions): Promise<number> {
+ return isSemanticHarness(name, options).then((semantic) => {
+ if (semantic) return attachSemantic(name, mode, options);
+ switch (mode) {
</file context>
Suggested change
| return isSemanticHarness(name, options).then((semantic) => { | |
| return isSemanticHarness(name, options).catch(() => false).then((semantic) => { |
| emitSessionEvent: (agentId, event) => hub.emitSessionEvent(agentId, event), | ||
| publishSessionEvent: async (agentId, event) => { | ||
| hub.emitSessionEvent(agentId, event); | ||
| await messaging.sessionEvents?.emit(agentId, event); |
Contributor
There was a problem hiding this comment.
P2: Agent-scoped clients propagate Relaycast session-event failures, unlike AgentRelay.publishSessionEvent, which reports and absorbs them. Applying the same best-effort/error-hook handling here would keep both AgentRelayAgent implementations consistent during Relaycast outages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/agent-relay.ts, line 499:
<comment>Agent-scoped clients propagate Relaycast session-event failures, unlike `AgentRelay.publishSessionEvent`, which reports and absorbs them. Applying the same best-effort/error-hook handling here would keep both `AgentRelayAgent` implementations consistent during Relaycast outages.</comment>
<file context>
@@ -480,6 +494,10 @@ export function agentRelayAgent(
emitSessionEvent: (agentId, event) => hub.emitSessionEvent(agentId, event),
+ publishSessionEvent: async (agentId, event) => {
+ hub.emitSessionEvent(agentId, event);
+ await messaging.sessionEvents?.emit(agentId, event);
+ },
};
</file context>
| this.host.hasActiveTurn && (context.mode === 'next-message' || context.mode === 'on-idle'); | ||
| if (shouldQueue) { | ||
| if (this.#queue.length >= this.#maxQueueSize) { | ||
| return this.#remember(key, { |
Contributor
There was a problem hiding this comment.
P2: Failed deliveries from startTurn, submitUserMessage, or a full queue are invisible to onEvent observers even though delivery.failed is declared in events.emits. Emitting the failure event for every failed receipt would keep listener/replay consumers consistent with the returned receipt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/harnesses/src/ai-sdk/relay-session.ts, line 399:
<comment>Failed deliveries from `startTurn`, `submitUserMessage`, or a full queue are invisible to `onEvent` observers even though `delivery.failed` is declared in `events.emits`. Emitting the failure event for every failed receipt would keep listener/replay consumers consistent with the returned receipt.</comment>
<file context>
@@ -0,0 +1,438 @@
+ this.host.hasActiveTurn && (context.mode === 'next-message' || context.mode === 'on-idle');
+ if (shouldQueue) {
+ if (this.#queue.length >= this.#maxQueueSize) {
+ return this.#remember(key, {
+ status: 'failed',
+ deliveryId: context.id,
</file context>
| const unsubscribe = client.onEvent((event) => { | ||
| if (!('name' in event) || event.name !== name) return; | ||
| if (live) live(event); | ||
| else pending.push(event); |
Contributor
There was a problem hiding this comment.
P2: High-output PTY workers can grow this buffer without bound when no observer is attached yet: every matching worker_stream chunk is retained until observeBrokerEvents or release. A bounded queue/drop policy would preserve the spawn race fix without turning delayed observation into an unbounded memory sink.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/harness-driver/src/broker-driver.ts, line 68:
<comment>High-output PTY workers can grow this buffer without bound when no observer is attached yet: every matching `worker_stream` chunk is retained until `observeBrokerEvents` or `release`. A bounded queue/drop policy would preserve the spawn race fix without turning delayed observation into an unbounded memory sink.</comment>
<file context>
@@ -38,12 +44,37 @@ export class BrokerDriver implements AgentDriver {
+ const unsubscribe = client.onEvent((event) => {
+ if (!('name' in event) || event.name !== name) return;
+ if (live) live(event);
+ else pending.push(event);
+ });
+ return {
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/verify-publish.yml (1)
420-423: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove Node 20 from
verify-dockermatrix.The summary output for Node 20 was removed, but the
verify-dockerstrategy matrix at line 365 still includes'20'(node: ['20', '22']). This means the job will still run for Node 20, but the result will be confusingly aggregated under "Node 22" in the markdown summary.Please remove
'20'from theverify-dockermatrix to fully align with the Node 22 migration plan.🤖 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 @.github/workflows/verify-publish.yml around lines 420 - 423, Update the verify-docker strategy matrix to remove Node 20, leaving only Node 22 so the workflow execution matches the Docker test summary and migration plan.
🟠 Major comments (21)
tests/integration/ai-sdk-harnesses/real-adapters.test.ts-10-10 (1)
10-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not convert arbitrary adapter failures into skips.
Matching generic text such as
not found,unavailable, orpnpmcan classify model-resolution, packaging, and adapter regressions as environmental skips. Restrict skipping to typed credential or missing-executable failures and rethrow ambiguous errors.Also applies to: 55-61
🤖 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 `@tests/integration/ai-sdk-harnesses/real-adapters.test.ts` at line 10, Restrict AUTH_OR_CLI_ERROR and the related skip handling in the adapter test to explicit credential/authentication failures or missing-executable errors, removing broad matches such as “not found,” “unavailable,” and “pnpm.” Ensure ambiguous adapter, model-resolution, packaging, and regression errors are rethrown instead of converted into skips.tests/integration/ai-sdk-harnesses/adapter-contract.test.ts-23-155 (1)
23-155: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe deterministic acceptance suites do not exercise registered adapter implementations.
Both suites substitute
FakeHarnessController, so adapter-specific failures cannot affect their results.
tests/integration/ai-sdk-harnesses/adapter-contract.test.ts#L23-L155: instantiate adapter-backed deterministic fixtures rather than mirroring registry declarations in a generic fake.tests/integration/ai-sdk-harnesses/soak-contract.test.ts#L44-L180: collect metrics from those adapter-backed fixtures, or rename the evidence as a shared-host fake soak.🤖 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 `@tests/integration/ai-sdk-harnesses/adapter-contract.test.ts` around lines 23 - 155, Replace the FakeHarnessController-based fixtures with adapter-backed deterministic fixtures so registered implementations are actually exercised. In tests/integration/ai-sdk-harnesses/adapter-contract.test.ts:23-155, update the adapter-contract setup around aiSdkAdapterRegistry and host; in tests/integration/ai-sdk-harnesses/soak-contract.test.ts:44-180, collect soak metrics from those same adapter-backed fixtures, or explicitly rename the suite to describe a shared-host fake soak if it remains fake.packages/sdk/src/agent-relay.ts-121-121 (1)
121-121: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the listener ID and Relaycast agent name distinct.
publishSessionEvent(agentId, ...)uses the same value for local ID-based dispatch andagents.events.emit(name, ...). Since agent IDs and names are distinct, events can be persisted against the wrong identifier or fail entirely. Accept both identity fields or resolve the storage name separately; the test currently masks this by passing"Worker"as the ID.Also applies to: 405-408, 497-499
🤖 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/agent-relay.ts` at line 121, Update publishSessionEvent and its implementations/call sites to keep the local agentId separate from the Relaycast agent name used by agents.events.emit. Accept or resolve both identity values, use agentId for listener dispatch and persistence, and use the Relaycast name for emission; update the affected tests so they pass distinct identifiers.packages/sdk/src/session/activity-reducer.ts-152-157 (1)
152-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdvance the sequence watermark for ignored activity transitions.
Line 153 ignores
activity.changedbefore recording its sequence. If sequence 10 is ignored and sequence 9 arrives afterward, sequence 9 is incorrectly processed as current, violating replay/out-of-order deduplication.Proposed fix
const observation = observed(event); - if (!observation || event.type === 'activity.changed') return { state: previous }; + if (!observation) return { state: previous }; if (observation.sequence <= previous.lastSequence) return { state: previous }; + if (event.type === 'activity.changed') { + return { + state: { ...previous, lastSequence: observation.sequence }, + }; + } const state = cloneState(previous);Add a regression test with an ignored transition followed by a lower-sequence lifecycle event.
🤖 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/session/activity-reducer.ts` around lines 152 - 157, Update the reducer logic around observed and activity.changed events so ignored activity transitions still advance the sequence watermark before returning the unchanged activity state. Preserve deduplication for sequences at or below the updated watermark, and add a regression test covering an ignored higher-sequence transition followed by a lower-sequence lifecycle event.packages/sdk/src/agent-relay.ts-497-500 (1)
497-500: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMatch the class’s best-effort publication behavior.
This facade rejects when Relaycast is unavailable, while
AgentRelay.publishSessionEventreports and swallows the same failure. Wrap this emission and route errors throughoptions?.onErrorso callers do not receive implementation-dependent outage behavior.Proposed parity fix
publishSessionEvent: async (agentId, event) => { hub.emitSessionEvent(agentId, event); - await messaging.sessionEvents?.emit(agentId, event); + try { + await messaging.sessionEvents?.emit(agentId, event); + } catch (error) { + options?.onError?.(error, { + source: 'listener', + selector: event.type, + operation: 'publish_session_event', + }); + } },🤖 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/agent-relay.ts` around lines 497 - 500, Update publishSessionEvent to use best-effort error handling: wrap the hub and optional messaging session-event emissions in a try/catch, and route any failure through options?.onError without rejecting to callers. Preserve the existing emission order and successful behavior.packages/sdk/src/listeners.ts-531-541 (1)
531-541: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftType canonical selectors from
AgentEvent, not the legacy session union.The current map exposes
AgentSessionEventvariants, so handlers such astool.called,file.changed, anderrorstill see optional observability and legacy fields despite using canonical selectors. Build the mapped envelope fromAgentEvent, and normalize or reject legacy events before dispatch.Proposed type correction
import type { AgentActivity, + AgentEvent, AgentEventType, AgentSessionEvent, } from './session/index.js'; export type RelayCanonicalSessionEventMap = { - [TType in CanonicalSessionSelector]: RelaySessionEvent<TType>; + [TType in CanonicalSessionSelector]: { + type: TType; + agentId: string; + event: Extract<AgentEvent, { type: TType }>; + }; };🤖 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/listeners.ts` around lines 531 - 541, Update RelaySessionEvent and RelayCanonicalSessionEventMap to derive event payloads from the canonical AgentEvent union rather than AgentSessionEvent, so canonical selectors such as tool.called, file.changed, and error expose only canonical fields. Normalize or reject legacy session events before dispatch so handlers cannot receive AgentSessionEvent variants through the canonical map.packages/harnesses/src/define.ts-236-236 (1)
236-236: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unsupported AI SDK selection from
new()too.Line 236 silently returns a PTY descriptor for calls such as
gemini.new({ backend: 'ai-sdk' }), whilecreate()correctly rejects the same explicit request. Keep both public entry points consistent.Proposed fix
- new: (input) => build(input), + new: (input = {}) => { + if (input.backend === 'ai-sdk') { + throw new Error(`No AI SDK harness adapter is registered for ${definition.command}`); + } + return build(input); + },🤖 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/harnesses/src/define.ts` at line 236, Update the `new` entry point to reject an explicit unsupported `backend: 'ai-sdk'` request, matching the validation behavior of `create()`, while preserving normal PTY descriptor construction for supported backends.packages/harnesses/src/ai-sdk/relay-session.ts-58-94 (1)
58-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdvertise the emitted
session.releasedevent.
release()emitssession.released, butcapabilities.events.emitsomits it. This makes the published event contract inaccurate.Also applies to: 429-435
🤖 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/harnesses/src/ai-sdk/relay-session.ts` around lines 58 - 94, Add 'session.released' to the AI_SDK_EVENT_TYPES array used by capabilities.events.emits, ensuring the advertised event contract matches the event emitted by release().packages/harnesses/src/ai-sdk/local-host-sandbox.ts-345-350 (1)
345-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease removed ports and make released ports reusable.
setPorts()drops removed leases without releasing them, while#releasePorts()never deletes ports from#usedPorts. Long-running session churn leaks reservations and eventually exhausts eligible ephemeral ports.Proposed fix
async setPorts(ports: readonly number[]) { if (ports.some((port) => !this.#ports.includes(port))) { throw new Error('Local sandbox ports are exclusively allocated by the provider'); } + await this.#releasePorts(this.#ports.filter((port) => !ports.includes(port))); this.#ports = [...ports]; } async `#releasePorts`(ports: readonly number[]): Promise<void> { for (const port of ports) { await this.#handoffPort(port); this.#ports.delete(port); + this.#usedPorts.delete(port); } }Also applies to: 442-447
🤖 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/harnesses/src/ai-sdk/local-host-sandbox.ts` around lines 345 - 350, Update setPorts and `#releasePorts` to release ports removed from the current allocation and delete every released port from `#usedPorts`, while preserving validation that all requested ports are provider-allocated. Ensure released ports become eligible for reuse during later allocations.packages/harnesses/src/ai-sdk/relay-session.ts-289-297 (1)
289-297: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not advertise
next-tool-calluntil its semantics are implemented.During an active turn, this mode falls through to
#accept()and injects immediately. Either defer it until a tool-call boundary or remove it from the capabilities list.Also applies to: 395-397
🤖 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/harnesses/src/ai-sdk/relay-session.ts` around lines 289 - 297, Remove next-tool-call from the advertised delivery modes in the RelaySession capabilities configuration. Until its boundary semantics are implemented in the delivery handling path, keep only the currently supported modes and preserve the existing capability structure.packages/harnesses/src/ai-sdk/harness-host.ts-301-312 (1)
301-312: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize normalized event delivery.
Stream callbacks invoke
#emitconcurrently. Later raw events can therefore be published before activity transitions derived from earlier events, violating observable event order. Queue listener dispatch inside#emit.Also applies to: 417-419, 537-539
🤖 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/harnesses/src/ai-sdk/harness-host.ts` around lines 301 - 312, Update `#emit` to serialize normalized event delivery through a shared promise chain or equivalent dispatch queue, ensuring each event’s listener notifications complete before the next event is published. Preserve the existing listener snapshot, Promise.allSettled behavior, sequencing, and turn metadata, and apply the same queued dispatch behavior to the related `#emit` call sites.packages/harnesses/src/ai-sdk/relay-session.ts-429-435 (1)
429-435: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFinalize deferred deliveries before clearing the queue.
Release silently discards queued messages whose callers received
deferredreceipts. Replace each receipt with a terminal failure and emitdelivery.failedbefore destroying the host.🤖 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/harnesses/src/ai-sdk/relay-session.ts` around lines 429 - 435, Update release in the session class to finalize every queued deferred delivery before clearing `#queue`: replace each receipt with a terminal failure and emit the corresponding delivery.failed event, then clear the queue and continue destroying the host and emitting session.released. Preserve the existing released guard and serialized execution.packages/harnesses/src/ai-sdk/harness-host.ts-530-540 (1)
530-540: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRollback the host state when continuation setup fails.
If
doContinueTurn()rejects, the host remainsrunningwith no control object, blocking subsequent lifecycle operations. Clear the turn fields and restoresuspendedor transition tofailedin a catch block.🤖 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/harnesses/src/ai-sdk/harness-host.ts` around lines 530 - 540, Update the continuation setup around doContinueTurn in the session resume flow to catch rejected promises and roll back the host state instead of leaving it running without a control object. In the catch path, clear the active turn fields and restore suspended state or transition to failed according to the existing lifecycle conventions, while preserving the successful control assignment and event flow.packages/harnesses/src/ai-sdk/sidecar.ts-176-177 (1)
176-177: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuarantee host cleanup with
try/finally.Any readline or output-write failure exits before Lines 371–372. Because sandbox children are detached process groups, this can leave adapter processes and port reservations alive.
Also applies to: 371-372
🤖 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/harnesses/src/ai-sdk/sidecar.ts` around lines 176 - 177, Wrap the line-processing and output-writing flow around createInterface and the `for await (const line of lines)` loop in a try/finally block. Ensure the existing host cleanup at lines 371–372 runs from the finally block even when readline iteration or output writes throw, while preserving the current processing behavior on success.packages/harnesses/src/ai-sdk/harness-host.ts-273-279 (1)
273-279: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSettle every promise-capable callback invocation. Both hooks allow asynchronous callbacks but discard their results, allowing rejected writes or listeners to become unhandled rejections.
packages/harnesses/src/ai-sdk/harness-host.ts#L273-L279: await, enqueue, or explicitly catchoptions.onDiagnostic.packages/harnesses/src/ai-sdk/relay-session.ts#L311-L323: dispatch the initial capabilities event through the settled#emitpath.🤖 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/harnesses/src/ai-sdk/harness-host.ts` around lines 273 - 279, Settle the asynchronous diagnostic callback in the observability report wrapper by awaiting, enqueueing, or explicitly catching options.onDiagnostic rather than discarding its promise; preserve the existing report and `#emit` behavior. In packages/harnesses/src/ai-sdk/relay-session.ts lines 311-323, route the initial capabilities event through the settled `#emit` path so any asynchronous listener or write rejection is handled.packages/harnesses/src/ai-sdk/sidecar.ts-389-394 (1)
389-394: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce the Node 22 baseline inside the shared sidecar entrypoint.
Both the CLI
__ai-sdk-sidecarbranch andsidecar-main.tsinvoke this function without passing throughrunCli()and its version assertion. Add the check here so every sidecar launch fails clearly on unsupported runtimes.🤖 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/harnesses/src/ai-sdk/sidecar.ts` around lines 389 - 394, Update runAiSdkSidecarMain to enforce the Node 22 runtime baseline before loading configuration or starting the sidecar. Reuse the existing version assertion used by runCli(), ensuring unsupported runtimes fail with the same clear error regardless of how the shared entrypoint is invoked.crates/broker/src/protocol.rs-586-598 (1)
586-598: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
instructionsin the typed native-command protocol.The HTTP endpoint accepts and forwards
instructions, but this enum omits it. Any typed round-trip will silently discard that field, breaking compact-command instructions.Proposed fix
NativeHarnessCommand { protocol_version: u32, kind: String, idempotency_key: String, #[serde(default, skip_serializing_if = "Option::is_none")] text: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] mode: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] approval_id: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + instructions: Option<String>, },let command = BrokerToWorker::NativeHarnessCommand { protocol_version: 1, - kind: "submit_user_message".into(), + kind: "compact".into(), idempotency_key: "input-1".into(), - text: Some("continue".into()), - mode: Some("active".into()), + text: None, + mode: None, approval_id: None, + instructions: Some("Preserve open tasks".into()), };Also applies to: 747-761
🤖 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 586 - 598, Add an optional serde-compatible instructions field to the NativeHarnessCommand variant, matching the HTTP payload’s field name and the existing optional text/mode/approval_id fields. Ensure typed serialization and deserialization preserve instructions during round-trips.packages/harness-driver/src/protocol.ts-145-153 (1)
145-153: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign the command frame across TypeScript, Rust, and the sidecar.
The Rust round-trip frame has neither
instructionsnorrequest_id, while this contract advertises compact guidance and the sidecar requiresrequest_id. Guidance may be dropped and uncorrelated commands rejected. Update all protocol definitions and add a cross-language compact-command test.Also applies to: 634-638
🤖 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/harness-driver/src/protocol.ts` around lines 145 - 153, Align NativeHarnessCommand and its Rust and sidecar equivalents by adding the missing request_id and optional compact-only instructions fields with matching serialization names and optionality. Update command handling so both fields survive round trips and request_id is accepted where required, then add a cross-language test covering a compact command with instructions and correlation via request_id.packages/harness-driver/src/broker-driver.ts-190-197 (1)
190-197: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecover the delivery chain after listener failures.
A rejected listener leaves
deliveryrejected, so every later event skips the listener while the stream continues consuming events.Proposed fix
- delivery = delivery.then(() => listener(event)); + delivery = delivery + .then(() => listener(event)) + .catch(async (error) => onError?.(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 `@packages/harness-driver/src/broker-driver.ts` around lines 190 - 197, Update the publish function’s delivery chaining so a listener rejection does not leave delivery permanently rejected. Ensure each subsequent event still invokes listener(event), while preserving event ordering and the existing highWater and terminalSeen behavior.packages/harness-driver/src/broker-driver.ts-59-59 (1)
59-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not classify every headless runtime as native.
Non-native headless workers also use this transport, but their history endpoint returns
unsupported_runtime. Pass the actual harness runtime discriminator instead.Proposed fix
- return this.runtimeHandle(client, result, transport === 'headless', buffered); + return this.runtimeHandle( + client, + result, + input.harnessConfig?.runtime === 'native', + buffered + );🤖 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/harness-driver/src/broker-driver.ts` at line 59, Update the call to runtimeHandle in the broker driver so its native-runtime discriminator reflects the actual harness runtime rather than treating every transport === 'headless' case as native. Preserve the existing client, result, and buffered arguments, and use the available runtime discriminator that distinguishes native from non-native headless workers.packages/harness-driver/src/broker-driver.ts-198-199 (1)
198-199: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail and clean up when history seeding is incomplete.
A rejected history request leaves both subscriptions active, while
history.gap === truesilently produces an incomplete event timeline. Dispose the iterator/runtime listener and surface either condition before replaying events.Proposed fix
- const history = await client.getAgentEventHistory(name, 0); + let history: AgentEventHistoryResponse; + try { + history = await client.getAgentEventHistory(name, 0); + if (history.gap) { + throw new Error(`agent event history for ${JSON.stringify(name)} contains a replay gap`); + } + } catch (error) { + stopped = true; + stopOnRuntimeExit(); + await iterator.return?.(); + throw error; + } for (const event of history.events) publish(event);🤖 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/harness-driver/src/broker-driver.ts` around lines 198 - 199, Update the history seeding flow around client.getAgentEventHistory and the event replay loop to detect a rejected history request or history.gap === true before publishing events. On either condition, dispose both the iterator and runtime listener, then surface the failure instead of replaying an incomplete timeline; preserve normal replay when history is complete.
🟡 Minor comments (2)
tests/integration/ai-sdk-harnesses/pty-reference-profile.test.ts-41-66 (1)
41-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert every reference event family, not a partial subset.
toMatchObjectand the omittedmodel,warnings, anddiagnosticsentries allow this “every reference gap” test to pass when those families are missing from the report.Proposed fix
- expect(comparison.agentEventFamilies).toMatchObject({ + expect(comparison.agentEventFamilies).toEqual({ lifecycle: { reference: ['exact'], candidate: ['exact'] }, turns: { reference: ['exact'], candidate: ['inferred'] }, text: { reference: ['exact'], candidate: ['unavailable'] }, reasoning: { reference: ['exact'], candidate: ['unavailable'] }, tools: { reference: ['exact'], candidate: ['unavailable'] }, tool_approvals: { reference: ['exact'], candidate: ['unavailable'] }, files: { reference: ['exact'], candidate: ['unavailable'] }, compaction: { reference: ['exact'], candidate: ['unavailable'] }, + model: { reference: ['exact'], candidate: ['unavailable'] }, + warnings: { reference: ['exact'], candidate: ['unavailable'] }, usage: { reference: ['exact'], candidate: ['unavailable'] }, + diagnostics: { reference: ['exact'], candidate: ['unavailable'] }, errors: { reference: ['exact'], candidate: ['exact'] }, });🤖 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 `@tests/integration/ai-sdk-harnesses/pty-reference-profile.test.ts` around lines 41 - 66, Update the comparison assertions in the test so they require the complete activities and agentEventFamilies reports rather than permitting partial matches. Include the omitted model, warnings, and diagnostics reference entries, and use strict equality assertions so missing or extra event families cause the test to fail.packages/cli/src/cli/lib/client-factory.test.ts-157-163 (1)
157-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a real Bun executable path here.
process.argv[0]is a path, not the literalbun, so this fixture should mirror the compiled-binary argv shape (/path/to/bun) to exercise the sidecar detection path instead of masking it.🤖 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/cli/src/cli/lib/client-factory.test.ts` around lines 157 - 163, Update the nativeSidecarLaunch test fixture to use a realistic Bun executable path (such as an absolute path) as process.argv[0] instead of the literal "bun", while preserving the compiled Bun argv shape and existing expected launch result.
🤖 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.
Outside diff comments:
In @.github/workflows/verify-publish.yml:
- Around line 420-423: Update the verify-docker strategy matrix to remove Node
20, leaving only Node 22 so the workflow execution matches the Docker test
summary and migration plan.
---
Major comments:
In `@crates/broker/src/protocol.rs`:
- Around line 586-598: Add an optional serde-compatible instructions field to
the NativeHarnessCommand variant, matching the HTTP payload’s field name and the
existing optional text/mode/approval_id fields. Ensure typed serialization and
deserialization preserve instructions during round-trips.
In `@packages/harness-driver/src/broker-driver.ts`:
- Around line 190-197: Update the publish function’s delivery chaining so a
listener rejection does not leave delivery permanently rejected. Ensure each
subsequent event still invokes listener(event), while preserving event ordering
and the existing highWater and terminalSeen behavior.
- Line 59: Update the call to runtimeHandle in the broker driver so its
native-runtime discriminator reflects the actual harness runtime rather than
treating every transport === 'headless' case as native. Preserve the existing
client, result, and buffered arguments, and use the available runtime
discriminator that distinguishes native from non-native headless workers.
- Around line 198-199: Update the history seeding flow around
client.getAgentEventHistory and the event replay loop to detect a rejected
history request or history.gap === true before publishing events. On either
condition, dispose both the iterator and runtime listener, then surface the
failure instead of replaying an incomplete timeline; preserve normal replay when
history is complete.
In `@packages/harness-driver/src/protocol.ts`:
- Around line 145-153: Align NativeHarnessCommand and its Rust and sidecar
equivalents by adding the missing request_id and optional compact-only
instructions fields with matching serialization names and optionality. Update
command handling so both fields survive round trips and request_id is accepted
where required, then add a cross-language test covering a compact command with
instructions and correlation via request_id.
In `@packages/harnesses/src/ai-sdk/harness-host.ts`:
- Around line 301-312: Update `#emit` to serialize normalized event delivery
through a shared promise chain or equivalent dispatch queue, ensuring each
event’s listener notifications complete before the next event is published.
Preserve the existing listener snapshot, Promise.allSettled behavior,
sequencing, and turn metadata, and apply the same queued dispatch behavior to
the related `#emit` call sites.
- Around line 530-540: Update the continuation setup around doContinueTurn in
the session resume flow to catch rejected promises and roll back the host state
instead of leaving it running without a control object. In the catch path, clear
the active turn fields and restore suspended state or transition to failed
according to the existing lifecycle conventions, while preserving the successful
control assignment and event flow.
- Around line 273-279: Settle the asynchronous diagnostic callback in the
observability report wrapper by awaiting, enqueueing, or explicitly catching
options.onDiagnostic rather than discarding its promise; preserve the existing
report and `#emit` behavior. In packages/harnesses/src/ai-sdk/relay-session.ts
lines 311-323, route the initial capabilities event through the settled `#emit`
path so any asynchronous listener or write rejection is handled.
In `@packages/harnesses/src/ai-sdk/local-host-sandbox.ts`:
- Around line 345-350: Update setPorts and `#releasePorts` to release ports
removed from the current allocation and delete every released port from
`#usedPorts`, while preserving validation that all requested ports are
provider-allocated. Ensure released ports become eligible for reuse during later
allocations.
In `@packages/harnesses/src/ai-sdk/relay-session.ts`:
- Around line 58-94: Add 'session.released' to the AI_SDK_EVENT_TYPES array used
by capabilities.events.emits, ensuring the advertised event contract matches the
event emitted by release().
- Around line 289-297: Remove next-tool-call from the advertised delivery modes
in the RelaySession capabilities configuration. Until its boundary semantics are
implemented in the delivery handling path, keep only the currently supported
modes and preserve the existing capability structure.
- Around line 429-435: Update release in the session class to finalize every
queued deferred delivery before clearing `#queue`: replace each receipt with a
terminal failure and emit the corresponding delivery.failed event, then clear
the queue and continue destroying the host and emitting session.released.
Preserve the existing released guard and serialized execution.
In `@packages/harnesses/src/ai-sdk/sidecar.ts`:
- Around line 176-177: Wrap the line-processing and output-writing flow around
createInterface and the `for await (const line of lines)` loop in a try/finally
block. Ensure the existing host cleanup at lines 371–372 runs from the finally
block even when readline iteration or output writes throw, while preserving the
current processing behavior on success.
- Around line 389-394: Update runAiSdkSidecarMain to enforce the Node 22 runtime
baseline before loading configuration or starting the sidecar. Reuse the
existing version assertion used by runCli(), ensuring unsupported runtimes fail
with the same clear error regardless of how the shared entrypoint is invoked.
In `@packages/harnesses/src/define.ts`:
- Line 236: Update the `new` entry point to reject an explicit unsupported
`backend: 'ai-sdk'` request, matching the validation behavior of `create()`,
while preserving normal PTY descriptor construction for supported backends.
In `@packages/sdk/src/agent-relay.ts`:
- Line 121: Update publishSessionEvent and its implementations/call sites to
keep the local agentId separate from the Relaycast agent name used by
agents.events.emit. Accept or resolve both identity values, use agentId for
listener dispatch and persistence, and use the Relaycast name for emission;
update the affected tests so they pass distinct identifiers.
- Around line 497-500: Update publishSessionEvent to use best-effort error
handling: wrap the hub and optional messaging session-event emissions in a
try/catch, and route any failure through options?.onError without rejecting to
callers. Preserve the existing emission order and successful behavior.
In `@packages/sdk/src/listeners.ts`:
- Around line 531-541: Update RelaySessionEvent and
RelayCanonicalSessionEventMap to derive event payloads from the canonical
AgentEvent union rather than AgentSessionEvent, so canonical selectors such as
tool.called, file.changed, and error expose only canonical fields. Normalize or
reject legacy session events before dispatch so handlers cannot receive
AgentSessionEvent variants through the canonical map.
In `@packages/sdk/src/session/activity-reducer.ts`:
- Around line 152-157: Update the reducer logic around observed and
activity.changed events so ignored activity transitions still advance the
sequence watermark before returning the unchanged activity state. Preserve
deduplication for sequences at or below the updated watermark, and add a
regression test covering an ignored higher-sequence transition followed by a
lower-sequence lifecycle event.
In `@tests/integration/ai-sdk-harnesses/adapter-contract.test.ts`:
- Around line 23-155: Replace the FakeHarnessController-based fixtures with
adapter-backed deterministic fixtures so registered implementations are actually
exercised. In
tests/integration/ai-sdk-harnesses/adapter-contract.test.ts:23-155, update the
adapter-contract setup around aiSdkAdapterRegistry and host; in
tests/integration/ai-sdk-harnesses/soak-contract.test.ts:44-180, collect soak
metrics from those same adapter-backed fixtures, or explicitly rename the suite
to describe a shared-host fake soak if it remains fake.
In `@tests/integration/ai-sdk-harnesses/real-adapters.test.ts`:
- Line 10: Restrict AUTH_OR_CLI_ERROR and the related skip handling in the
adapter test to explicit credential/authentication failures or
missing-executable errors, removing broad matches such as “not found,”
“unavailable,” and “pnpm.” Ensure ambiguous adapter, model-resolution,
packaging, and regression errors are rethrown instead of converted into skips.
---
Minor comments:
In `@packages/cli/src/cli/lib/client-factory.test.ts`:
- Around line 157-163: Update the nativeSidecarLaunch test fixture to use a
realistic Bun executable path (such as an absolute path) as process.argv[0]
instead of the literal "bun", while preserving the compiled Bun argv shape and
existing expected launch result.
In `@tests/integration/ai-sdk-harnesses/pty-reference-profile.test.ts`:
- Around line 41-66: Update the comparison assertions in the test so they
require the complete activities and agentEventFamilies reports rather than
permitting partial matches. Include the omitted model, warnings, and diagnostics
reference entries, and use strict equality assertions so missing or extra event
families cause the test to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56c5b3e9-c62c-4ae8-8102-62412e18991c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (139)
.agentworkforce/trajectories/completed/2026-07/traj_20khcr8z0v13/summary.md.agentworkforce/trajectories/completed/2026-07/traj_20khcr8z0v13/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_681sx2jr2m46/summary.md.agentworkforce/trajectories/completed/2026-07/traj_681sx2jr2m46/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_8rr4e8wisgh3/summary.md.agentworkforce/trajectories/completed/2026-07/traj_8rr4e8wisgh3/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_ajse65eu6brc.trace.json.agentworkforce/trajectories/completed/2026-07/traj_ajse65eu6brc/summary.md.agentworkforce/trajectories/completed/2026-07/traj_ajse65eu6brc/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_b8i9trq4ou7i/summary.md.agentworkforce/trajectories/completed/2026-07/traj_b8i9trq4ou7i/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_bvv35fxv38pq/summary.md.agentworkforce/trajectories/completed/2026-07/traj_bvv35fxv38pq/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_cyut1lxn0261/summary.md.agentworkforce/trajectories/completed/2026-07/traj_cyut1lxn0261/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_id1ypo49qd36/summary.md.agentworkforce/trajectories/completed/2026-07/traj_id1ypo49qd36/trajectory.json.agentworkforce/trajectories/completed/2026-07/traj_mpf6nswkfvex/summary.md.agentworkforce/trajectories/completed/2026-07/traj_mpf6nswkfvex/trajectory.json.github/workflows/codegen-models.yml.github/workflows/e2e-tests.yml.github/workflows/fleet-e2e.yml.github/workflows/node-compat.yml.github/workflows/prettier-fmt-fix.yml.github/workflows/prod-smoke.yml.github/workflows/stress-tests.yml.github/workflows/test-install.yml.github/workflows/test.yml.github/workflows/update-cli-versions.yml.github/workflows/verify-publish.yml.workflow-artifacts/plan-001/claude-review-1-resolution.md.workflow-artifacts/plan-001/claude-review-1.md.workflow-artifacts/plan-001/codex-review-1-resolution.md.workflow-artifacts/plan-001/codex-review-1.md.workflow-artifacts/plan-001/final-signoff-2.md.workflow-artifacts/plan-001/final-signoff-3.md.workflow-artifacts/plan-001/final-signoff-resolution.md.workflow-artifacts/plan-001/final-signoff.md.workflow-artifacts/plan-001/self-reflection.mdCHANGELOG.mdREADME.mdcrates/broker/src/listen_api.rscrates/broker/src/protocol.rscrates/broker/src/relaycast/ws.rscrates/broker/src/replay_buffer.rscrates/broker/src/runtime/api.rscrates/broker/src/runtime/event_loop.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/maintenance.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/worker.rspackage.jsonpackages/brand/package.jsonpackages/broker-darwin-arm64/package.jsonpackages/broker-darwin-x64/package.jsonpackages/broker-linux-arm64/package.jsonpackages/broker-linux-x64/package.jsonpackages/broker-win32-x64/package.jsonpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/cli/bootstrap.tspackages/cli/src/cli/commands/local-agent.test.tspackages/cli/src/cli/commands/local-agent.tspackages/cli/src/cli/entrypoint.test.tspackages/cli/src/cli/index.tspackages/cli/src/cli/lib/attach-native.test.tspackages/cli/src/cli/lib/attach-native.tspackages/cli/src/cli/lib/client-factory.test.tspackages/cli/src/cli/lib/client-factory.tspackages/cli/src/cli/lib/fleet-sidecar.test.tspackages/cli/src/cli/lib/node-version.test.tspackages/cli/src/cli/lib/node-version.tspackages/cloud/package.jsonpackages/config/package.jsonpackages/evals/package.jsonpackages/fleet/package.jsonpackages/harness-driver/package.jsonpackages/harness-driver/src/broker-driver.tspackages/harness-driver/src/client.tspackages/harness-driver/src/driver-types.tspackages/harness-driver/src/harness.tspackages/harness-driver/src/native-client.test.tspackages/harness-driver/src/protocol.tspackages/harness-driver/src/spawn-request.tspackages/harness-driver/src/types.tspackages/harnesses/README.mdpackages/harnesses/package.jsonpackages/harnesses/src/ai-sdk/adapter-registry.test.tspackages/harnesses/src/ai-sdk/adapter-registry.tspackages/harnesses/src/ai-sdk/harness-host.test.tspackages/harnesses/src/ai-sdk/harness-host.tspackages/harnesses/src/ai-sdk/index.tspackages/harnesses/src/ai-sdk/local-host-sandbox.test.tspackages/harnesses/src/ai-sdk/local-host-sandbox.tspackages/harnesses/src/ai-sdk/relay-session.test.tspackages/harnesses/src/ai-sdk/relay-session.tspackages/harnesses/src/ai-sdk/sidecar-main.tspackages/harnesses/src/ai-sdk/sidecar.test.tspackages/harnesses/src/ai-sdk/sidecar.tspackages/harnesses/src/define-spawn.test.tspackages/harnesses/src/define.tspackages/harnesses/src/index.tspackages/harnesses/src/observability.test.tspackages/harnesses/src/observability.tspackages/integration-prompts/package.jsonpackages/policy/package.jsonpackages/sdk/package.jsonpackages/sdk/src/__tests__/activity-reducer.test.tspackages/sdk/src/__tests__/listeners.test-d.tspackages/sdk/src/__tests__/listeners.test.tspackages/sdk/src/__tests__/messaging.test.tspackages/sdk/src/agent-relay.tspackages/sdk/src/listeners.tspackages/sdk/src/messaging/relaycast-client.tspackages/sdk/src/messaging/relaycast.tspackages/sdk/src/messaging/types.tspackages/sdk/src/session/activity-reducer.tspackages/sdk/src/session/index.tspackages/sdk/src/session/types.tspackages/utils/package.jsonplans/001-adopt-ai-sdk-harnesses.mdplans/README.mdtests/e2e/fleet/harness.tstests/integration/ai-sdk-harnesses/adapter-contract.test.tstests/integration/ai-sdk-harnesses/fakes.tstests/integration/ai-sdk-harnesses/native-attach-order.test.tstests/integration/ai-sdk-harnesses/observability-contract.test.tstests/integration/ai-sdk-harnesses/profile-report.tstests/integration/ai-sdk-harnesses/pty-reference-profile.test.tstests/integration/ai-sdk-harnesses/real-adapters.test.tstests/integration/ai-sdk-harnesses/soak-contract.test.tstests/integration/broker/fixtures/native-sidecar.tstests/integration/broker/native-harness.test.tsvitest.config.ts
willwashburn
changed the title
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
agent-relay node agent spawn|new --backend ai-sdk, with--backend auto|ptypreserving the existing PTY rollout and unsupported-harness fallbackpty | native) from normalized observability (AgentEvent), withheadlessdocumented as an internal broker process wrapperRollout boundary
All AI SDK adapters remain experimental. Claude Code, Codex, and OpenCode retain PTY as the default shared path. Pi and Deep Agents are explicit AI SDK-only options. Plan 001 remains IN PROGRESS until authenticated real-provider and cross-platform promotion gates pass.
Native attach supports structured
viewand line-orienteddrive; terminalpassthrough, task-exit, and--exit-after-taskremain PTY-only.Hosted high-frequency observability is ordered and bounded but intentionally best-effort during Relaycast outages; it is not a durable audit ledger.
Verification
npm test: 107 files passed, 1,320 tests passed, 14 skippedcargo test: 1,024 tests passed, 5 ignorednode agent spawn pi --backend ai-sdk-> embedded sidecar/adapter; Node CLI launch; native command/events/release protocolnpm run typechecknpm run lint: 0 errors, 39 existing warningscargo fmt --all -- --checkgit diff --checkNo UI surface changed.