feat(harness-driver): add lifecycle-aware SpawnedAgentHandle #1050
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| /** | ||
| * Lifecycle-aware handle for a spawned agent. | ||
| * | ||
| * `HarnessDriverClient.spawnPty()` / `spawnCli()` / `spawnHeadless()` return one | ||
| * of these instead of a bare {@link SpawnAgentResult}. It is a structural | ||
| * superset of `SpawnAgentResult` (it still carries `name` / `runtime` / | ||
| * `sessionId` / `pid`), so existing callers are unaffected, and it adds the | ||
| * promise-based lifecycle operations consumers previously had to reconstruct | ||
| * from the raw broker event stream: | ||
| * | ||
| * - `waitForExit()` — resolve when the agent exits, with `code` / `signal`. | ||
| * - `waitForIdle()` — resolve on the next idle signal (or on exit). | ||
| * - `exit` / `exitCode` / `exitSignal` — synchronous view of a prior exit. | ||
| * - `release()` — release the agent via the broker. | ||
| * | ||
| * All operations are backed by the client's broker event stream and its event | ||
| * history, so they are replay-correct: calling `waitForExit()` after the agent | ||
| * has already exited resolves immediately from history rather than hanging. | ||
| */ | ||
| import type { HarnessDriverClient } from './client.js'; | ||
| import type { AgentRuntime, BrokerEvent } from './protocol.js'; | ||
| import type { SpawnAgentResult } from './types.js'; | ||
|
|
||
| export interface AgentExitInfo { | ||
| /** `'exited'` when the agent exited; `'timeout'` when the wait elapsed first. */ | ||
| reason: 'exited' | 'timeout'; | ||
| /** Process exit code, when the broker reported one. */ | ||
| code?: number; | ||
| /** Terminating signal, when the agent was killed by one. */ | ||
| signal?: string; | ||
| } | ||
|
|
||
| export interface AgentIdleInfo { | ||
| /** `'idle'` on an idle signal, `'exited'` if the agent exited first, `'timeout'` otherwise. */ | ||
| reason: 'idle' | 'exited' | 'timeout'; | ||
| /** Seconds the agent has been idle, when `reason === 'idle'`. */ | ||
| idleSecs?: number; | ||
| /** Exit details, when `reason === 'exited'`. */ | ||
| exit?: AgentExitInfo; | ||
| } | ||
|
|
||
| export class SpawnedAgentHandle implements SpawnAgentResult { | ||
| readonly name: string; | ||
| readonly runtime: AgentRuntime; | ||
| readonly sessionId?: string; | ||
| readonly pid?: number; | ||
|
|
||
| constructor( | ||
| result: SpawnAgentResult, | ||
| private readonly client: HarnessDriverClient | ||
| ) { | ||
| this.name = result.name; | ||
| this.runtime = result.runtime; | ||
| this.sessionId = result.sessionId; | ||
| this.pid = result.pid; | ||
| } | ||
|
|
||
| /** Exit info if the agent has already exited (from broker event history), else `undefined`. */ | ||
| get exit(): AgentExitInfo | undefined { | ||
| const exited = this.client.getLastEvent('agent_exited', this.name); | ||
| if (exited && exited.kind === 'agent_exited') { | ||
| return { reason: 'exited', code: exited.code, signal: exited.signal }; | ||
| } | ||
| const exit = this.client.getLastEvent('agent_exit', this.name); | ||
| if (exit && exit.kind === 'agent_exit') { | ||
| return { reason: 'exited' }; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| get exitCode(): number | undefined { | ||
| return this.exit?.code; | ||
| } | ||
|
|
||
| get exitSignal(): string | undefined { | ||
| return this.exit?.signal; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve when the agent exits (with `code` / `signal` when the broker reports | ||
| * them), or with `{ reason: 'timeout' }` if `timeoutMs` elapses first. Replays | ||
| * a prior exit from broker history, so it is safe to call after the fact. | ||
| */ | ||
| waitForExit(timeoutMs?: number): Promise<AgentExitInfo> { | ||
| // Ensure the broker event stream is live — `onEvent` only registers a | ||
| // listener; without an active connection no events ever arrive. Idempotent. | ||
| this.client.connectEvents(); | ||
|
|
||
| const already = this.exit; | ||
| if (already) return Promise.resolve(already); | ||
|
|
||
| return new Promise<AgentExitInfo>((resolve) => { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const settle = (info: AgentExitInfo) => { | ||
| if (timer) clearTimeout(timer); | ||
| unsub(); | ||
| resolve(info); | ||
| }; | ||
| const unsub = this.client.onEvent((event: BrokerEvent) => { | ||
| const exit = matchExit(event, this.name); | ||
| if (exit) settle(exit); | ||
| }); | ||
| if (timeoutMs !== undefined) { | ||
| timer = setTimeout(() => settle({ reason: 'timeout' }), timeoutMs); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve on the next idle signal for this agent (edge-triggered: a fresh | ||
| * signal after the call, matching how runners poll-then-nudge). Also resolves | ||
| * if the agent exits first, or with `{ reason: 'timeout' }` after `timeoutMs`. | ||
| */ | ||
| waitForIdle(timeoutMs?: number): Promise<AgentIdleInfo> { | ||
| // Ensure the broker event stream is live (see waitForExit). Idempotent. | ||
| this.client.connectEvents(); | ||
|
|
||
| const already = this.exit; | ||
| if (already) return Promise.resolve({ reason: 'exited', exit: already }); | ||
|
|
||
| return new Promise<AgentIdleInfo>((resolve) => { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const settle = (info: AgentIdleInfo) => { | ||
| if (timer) clearTimeout(timer); | ||
| unsub(); | ||
| resolve(info); | ||
| }; | ||
| // Idle and exit both arrive on the broker event stream. The named | ||
| // `agentIdle` event bus is only populated by call-site hooks (not broker | ||
| // events) in direct-client usage, so it must not be used here. | ||
| const unsub = this.client.onEvent((event: BrokerEvent) => { | ||
| if (event.kind === 'agent_idle' && event.name === this.name) { | ||
| settle({ reason: 'idle', idleSecs: event.idle_secs }); | ||
| return; | ||
| } | ||
| const exit = matchExit(event, this.name); | ||
| if (exit) settle({ reason: 'exited', exit }); | ||
| }); | ||
| if (timeoutMs !== undefined) { | ||
| timer = setTimeout(() => settle({ reason: 'timeout' }), timeoutMs); | ||
| } | ||
| }); | ||
|
Comment on lines
+84
to
+142
Contributor
There was a problem hiding this comment. Start the broker event stream before waiting.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| /** Release the agent via the broker. */ | ||
| release(reason?: string): Promise<{ name: string }> { | ||
| return this.client.release(this.name, reason); | ||
| } | ||
| } | ||
|
|
||
| /** Match an exit `BrokerEvent` for `name`, normalising the two exit kinds. */ | ||
| function matchExit(event: BrokerEvent, name: string): AgentExitInfo | undefined { | ||
| if (event.kind === 'agent_exited' && event.name === name) { | ||
| return { reason: 'exited', code: event.code, signal: event.signal }; | ||
| } | ||
| if (event.kind === 'agent_exit' && event.name === name) { | ||
| return { reason: 'exited' }; | ||
|
Comment on lines
+156
to
+157
There was a problem hiding this comment.
For PTY/headless agents, the broker emits Useful? React with 👍 / 👎. |
||
| } | ||
| return undefined; | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ import type { | |
| ListAgent, | ||
| } from './types.js'; | ||
| import { EventBus } from './event-bus.js'; | ||
| import { SpawnedAgentHandle } from './agent-handle.js'; | ||
| import type { | ||
| AfterAgentReleaseContext, | ||
| AfterAgentSpawnContext, | ||
|
|
@@ -621,7 +622,7 @@ export class HarnessDriverClient { | |
|
|
||
| // ── Agent lifecycle ──────────────────────────────────────────────── | ||
|
|
||
| async spawnPty(input: SpawnPtyInput): Promise<SpawnedAgentHandle> { | ||
| const beforeCtx: BeforeAgentSpawnContext<SpawnPtyInput> = { | ||
| kind: 'pty', | ||
| input, | ||
|
|
@@ -638,14 +639,14 @@ export class HarnessDriverClient { | |
| }); | ||
| const result = SpawnAgentResultSchema.parse(rawResult); | ||
| await this.emitAfterSpawn(beforeCtx, resolvedInput, t0, result, undefined); | ||
| return new SpawnedAgentHandle(result, this); | ||
|
There was a problem hiding this comment. Suggestion: This now returns a class instance that carries an internal Severity Level: Critical 🚨- ❌ JSON serializing SpawnedAgentHandle leaks BrokerTransport.apiKey credential.
- ⚠️ Logs may contain workspace_key and broker base URL.Steps of Reproduction ✅1. `HarnessDriverClient.spawnPty()` in `packages/harness-driver/src/client.ts:46-63`
parses the broker response into `result` using `SpawnAgentResultSchema` and now returns
`new SpawnedAgentHandle(result, this)` (line 63), instead of returning the plain `result`
object.
2. `SpawnedAgentHandle` in `packages/harness-driver/src/agent-handle.ts:42-56` stores the
passed client reference as a `private readonly client: HarnessDriverClient` constructor
parameter (lines 48-51); in the emitted JavaScript this becomes a normal instance property
(not an ES `#private` field), so it is an own, enumerable property of the handle object at
runtime.
3. The `HarnessDriverClient` instance stored on the handle holds a `BrokerTransport` with
an `apiKey` field (`packages/harness-driver/src/transport.ts:132-151`) and a
`workspaceKey` field on the client itself
(`packages/harness-driver/src/client.ts:139-144`), both of which are normal object
properties; nothing marks them as non-enumerable or excludes them from serialization.
4. When a downstream consumer obtains a handle from `spawnPty()` (for example, by adapting
the existing CLI entry `spawnAgentWithClient` in
`packages/cli/src/cli/lib/client-factory.ts:10-14` to `const handle = await
client.spawnPty(options);`) and then logs or transmits it via `JSON.stringify(handle)` or
`{ ...handle }`, JSON serialization walks enumerable own properties and includes the
nested `client` object. That nested object in turn contains the `BrokerTransport.apiKey`
and any `workspaceKey`, so the serialized spawn result now leaks internal connection
credentials and driver state instead of just the intended metadata (`name`, `runtime`,
`sessionId`, `pid`).Fix in Cursor | Fix in VSCode Claude (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** packages/harness-driver/src/client.ts
**Line:** 642:642
**Comment:**
*Security: This now returns a class instance that carries an internal `client` reference, so callers that spread or serialize the spawn result can unintentionally expose deep internal state (including transport credentials) instead of just agent metadata. Keep internals non-enumerable/opaque (for example with true private fields and controlled serialization) before returning this object shape publicly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } catch (err) { | ||
| await this.emitAfterSpawn(beforeCtx, resolvedInput, t0, undefined, err); | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| async spawnCli(input: SpawnCliInput): Promise<SpawnedAgentHandle> { | ||
| const beforeCtx: BeforeAgentSpawnContext<SpawnCliInput> = { | ||
| kind: 'cli', | ||
| input, | ||
|
|
@@ -659,7 +660,7 @@ export class HarnessDriverClient { | |
| private async spawnCliWithContext( | ||
| beforeCtx: BeforeAgentSpawnContext<SpawnCliInput>, | ||
| input: SpawnCliInput | ||
| ): Promise<SpawnedAgentHandle> { | ||
| const t0 = Date.now(); | ||
| const resolvedInput = await this.runBeforeSpawn(beforeCtx); | ||
| const transport = resolveSpawnTransport(resolvedInput); | ||
|
|
@@ -680,14 +681,14 @@ export class HarnessDriverClient { | |
| }); | ||
| const result = SpawnAgentResultSchema.parse(rawResult); | ||
| await this.emitAfterSpawn(beforeCtx, resolvedInput, t0, result, undefined); | ||
| return new SpawnedAgentHandle(result, this); | ||
| } catch (err) { | ||
| await this.emitAfterSpawn(beforeCtx, resolvedInput, t0, undefined, err); | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| async spawnHeadless(input: SpawnHeadlessInput): Promise<SpawnedAgentHandle> { | ||
| const cliInput: SpawnCliInput = { ...input, transport: 'headless' }; | ||
| const beforeCtx: BeforeAgentSpawnContext<SpawnCliInput> = { | ||
| kind: 'headless', | ||
|
|
@@ -699,11 +700,11 @@ export class HarnessDriverClient { | |
| return this.spawnCliWithContext(beforeCtx, cliInput); | ||
| } | ||
|
|
||
| async spawnClaude(input: Omit<SpawnCliInput, 'cli'>): Promise<SpawnedAgentHandle> { | ||
| return this.spawnCli({ ...input, cli: 'claude' }); | ||
| } | ||
|
|
||
| async spawnOpencode(input: Omit<SpawnCliInput, 'cli'>): Promise<SpawnedAgentHandle> { | ||
| return this.spawnCli({ ...input, cli: 'opencode' }); | ||
| } | ||
|
|
||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
agent_exitis an exit-request signal (not a confirmed process termination), but this code treats it as a completed exit. That can makewaitForExit()andexitresolve too early, before the process has actually terminated and before a real exit code/signal is known. Only resolve exit completion from the confirmedagent_exitedevent. [api mismatch]Severity Level: Critical 🚨
Steps of Reproduction ✅
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖