Add config-backed harness adapters#978
Closed
willwashburn wants to merge 16 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds harness adapters to broker and SDKs with sessionId propagation, implements a built-in MCP stdio server and CLI command, updates MCP invocation to “npx -y agent-relay mcp,” revises docs/tests, and tweaks release changelog filtering. ChangesHarness adapters, sessionId propagation, and MCP stdio server
Sequence Diagram(s)(none) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 84 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ec8236a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…omain # Conflicts: # .github/workflows/publish.yml # .trajectories/index.json # AGENTS.md # CHANGELOG.md # crates/broker/src/relaycast/mod.rs # crates/broker/src/snippets.rs # crates/broker/src/worker.rs # package-lock.json # package.json # packages/sdk/src/relay.ts # src/cli/relaycast-mcp.ts # web/sst.config.ts
Contributor
|
Preview deployed!
This preview will be cleaned up when the PR is merged or closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sdk-py/src/agent_relay/relay.py (1)
646-657:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh
session_idfor already-known agents inlist_agents().When an agent already exists in
_known_agents, the freshsessionIdfrom broker output is currently ignored, soAgent.session_idcan remain stale.Suggested fix
for entry in raw_list: name = entry.get("name", "") existing = self._known_agents.get(name) if existing: + session_id = entry.get("sessionId") + if session_id: + existing._session_id = session_id agents.append(existing) else: agent = Agent( name=name, runtime=entry.get("runtime", "pty"),🤖 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-py/src/agent_relay/relay.py` around lines 646 - 657, In list_agents(), when an agent name matches an entry in self._known_agents, refresh the Agent.session_id from the incoming broker entry so it doesn't stay stale; specifically, inside the branch where existing = self._known_agents.get(name) is truthy, set existing.session_id = entry.get("sessionId") (and then append existing to agents) so the cached Agent object is updated with the new session id.packages/sdk/src/workflows/runner.ts (1)
1747-1761:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the harness
proxyProviderbefore the single-provider fallback.Right now a harness-specific provider hint is ignored whenever exactly one proxy provider is configured. That can silently mint a token for the wrong upstream instead of failing on the missing provider config.
Suggested change
- if (configuredProviders.length === 1) { - const [onlyProvider] = configuredProviders; - if (onlyProvider === 'openai' || onlyProvider === 'anthropic' || onlyProvider === 'openrouter') { - return onlyProvider; - } - } - const harnessProxyProvider = getCliDefinition(agentDef.cli)?.proxyProvider; if ( harnessProxyProvider === 'openai' || harnessProxyProvider === 'anthropic' || harnessProxyProvider === 'openrouter' ) { return harnessProxyProvider; } + + if (configuredProviders.length === 1) { + const [onlyProvider] = configuredProviders; + if (onlyProvider === 'openai' || onlyProvider === 'anthropic' || onlyProvider === 'openrouter') { + return onlyProvider; + } + }🤖 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/workflows/runner.ts` around lines 1747 - 1761, The harness proxyProvider check is currently evaluated after the single-provider fallback and gets ignored when configuredProviders.length === 1; change the logic to evaluate harnessProxyProvider (from getCliDefinition(agentDef.cli)?.proxyProvider) before returning a single configured provider so that if harnessProxyProvider is one of 'openai'|'anthropic'|'openrouter' it is returned first; update the code around configuredProviders and harnessProxyProvider in runner.ts so harnessProxyProvider takes precedence over the single-provider fallback (or alternatively gate the single-provider return to only run when harnessProxyProvider is undefined).
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/spawn-harness.test.ts (1)
79-105: 💤 Low valueConsider documenting why
nonInteractiveArgsis excluded from serialization.The test registers an adapter at line 82 with
nonInteractiveArgs, but the assertion at lines 98-102 only checks forbinaries,bypassFlag, andsearchPaths. IfnonInteractiveArgsis intentionally excluded because it's a non-serializable function, a brief comment would clarify the test's intent.📝 Suggested comment
await relay.spawn('worker', 'registered-spawn-harness', 'ship it', { channels: ['general'] }); + // nonInteractiveArgs is a function and not serialized to the broker expect(spawnPty).toHaveBeenCalledWith(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/__tests__/spawn-harness.test.ts` around lines 79 - 105, The test registers an adapter with registerHarnessAdapter including a nonInteractiveArgs function but asserts only serializable fields; add a brief inline comment near the registration (or above the expect) clarifying that nonInteractiveArgs is intentionally excluded from the serialized harness payload because functions are non-serializable and only plain data (binaries, bypassFlag, searchPaths) should be sent to spawnPty via AgentRelay.spawn; mention the symbol names registerHarnessAdapter, nonInteractiveArgs, spawnPty and AgentRelay.spawn so future readers understand why the assertion omits nonInteractiveArgs.packages/sdk/src/client.ts (1)
119-123: ⚡ Quick winAdd documentation for the new interface.
The new
SpawnAgentResultinterface introduces an optionalsessionIdfield but lacks JSDoc comments explaining:
- When is
sessionIdpopulated vs. undefined?- What is the
sessionIdused for?- How should consumers handle the optional field?
Adding documentation would improve the developer experience for this public API.
📝 Suggested documentation
+/** + * Result of spawning an agent via the broker. + */ export interface SpawnAgentResult { + /** Name of the spawned agent. */ name: string; + /** Runtime information (pty/headless) for the spawned agent. */ runtime: AgentRuntime; + /** + * Optional session identifier for the agent. Present when the broker + * establishes a session-aware connection (e.g., for harness-backed agents). + */ sessionId?: string; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/client.ts` around lines 119 - 123, Add JSDoc to the exported SpawnAgentResult interface (the declaration with name, runtime, and optional sessionId) that explains when sessionId is populated versus undefined, what the sessionId represents/used for (e.g., correlating agent runtime sessions, resuming or tracking interactions), and how consumers should handle the optional field (check for undefined, treat as transient/read-only identifier). Keep the comments short and actionable so callers of SpawnAgentResult understand lifecycle and nullability of sessionId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trajectories/completed/2026-05/traj_dqgg2q4scsvt.json:
- Line 47: The committed trajectory JSON contains a machine-specific absolute
path in the "projectId" field; update the record that sets projectId to use a
repo-relative identifier (e.g., repository root name or a relative path) or
remove the "projectId" key entirely for committed artifacts so no local user
path (/Users/...) is leaked; locate the JSON object with the "projectId" key in
traj_dqgg2q4scsvt.json and replace the absolute path value with a portable
identifier or delete that key before committing.
In @.trajectories/completed/2026-05/traj_r3eic6rt84pq.json:
- Line 19: The committed JSON contains an absolute local path in the "projectId"
field which leaks workstation/user info; update the code that writes or
generates this field (the logic that sets "projectId") to emit a sanitized,
portable identifier instead — e.g. use a repository name, git remote URL, repo
root relative path, or an environment-provided ID rather than an absolute path
like "/Users/will/..."; locate the place where "projectId" is assigned in the
serializer/generator and replace the absolute-path value with a normalized value
(strip home dir, convert to relative path, or substitute a stable repo
identifier) and add a unit/test asserting no leading "/" or user home components
are present.
In @.trajectories/completed/2026-05/traj_r3eic6rt84pq.md:
- Line 12: Change the phrase "SDK focused tests" to "SDK-focused tests" in the
updated PR message; locate the exact text "SDK focused tests" in the line that
currently reads "validating SDK focused tests plus repo typecheck before pushing
codex/structured-agent-results" and insert a hyphen between "SDK" and "focused"
so it reads "validating SDK-focused tests plus repo typecheck before pushing
codex/structured-agent-results".
In @.trajectories/index.json:
- Around line 1207-1234: Entries like "traj_pjadgfw0mtw4", "traj_dqgg2q4scsvt",
"traj_r3eic6rt84pq", and "traj_4b2d63f6ljvh" currently store user-specific
absolute paths (e.g. /Users/will/...), so update the code that writes/serializes
.trajectories index entries to record repository-relative or
workspace-normalized paths instead of absolute home paths; specifically, replace
the writer that sets the "path" value to use a relative path computed from the
repo root (e.g., path.relative(repoRoot, absolutePath) or equivalent) and ensure
normalization to POSIX style, then update any existing committed entries to
their repo-relative equivalents so no local identity leaks remain.
In `@crates/broker/src/codex_session.rs`:
- Around line 18-26: The timeout wrapper can cancel the future running
create_resumable_codex_thread_inner and drop the spawned tokio::process::Child
without calling child.kill()/child.wait(), potentially orphaning the Codex
subprocess; to fix, configure the Command used inside
create_resumable_codex_thread_inner with .kill_on_drop(true) before spawn so
dropping the Child always terminates the process, and/or restructure the timeout
usage to drive explicit cleanup (e.g., use tokio::select! to detect the timeout
and call child.kill().await and child.wait().await) so the existing
child.kill()/child.wait() paths are always executed; update the code in
create_resumable_codex_thread_inner (where Command is built and the child is
spawned) and ensure the child.kill()/child.wait() cleanup remains present.
In `@crates/broker/src/listen_api.rs`:
- Around line 652-668: The code currently attempts to deserialize whatever value
is present for "harness", rejecting requests where the client sends JSON null;
change the extraction for harness so that if body.get("harness") yields
Some(Value::Null) it is treated as None before calling serde_json::from_value.
Locate the existing chain that uses
body.get("harness").cloned().map(serde_json::from_value::<HarnessDefinition>).transpose()
and replace it with logic that filters out Value::Null (e.g., map only when the
cloned value is not null or explicitly match and return None for Value::Null) so
that explicit nulls are treated as an omitted optional field.
In `@crates/broker/src/worker.rs`:
- Around line 957-1001: resolve_command_with_paths and resolve_harness_command
currently treat any regular file as runnable; change the file existence checks
to require executability. Replace candidate.is_file() (and
Path::new(&resolved).is_file()) with a platform-aware "is executable" check
(e.g. on Unix check metadata().permissions().mode() & 0o111 != 0, on Windows
ensure is_file() and that the extension matches PATHEXT) when testing candidates
built via expand_home_path, PATH iteration, and the final resolved path; keep
using canonicalize_display for returned paths and otherwise fall back to the
original command string.
In `@packages/openclaw/src/setup.ts`:
- Around line 400-401: In the relaycast registration calls that build the npx
argument array (the sequences containing '--arg', 'agent-relay', '--arg', 'mcp'
used in the relaycast config-add invocations), add the '-y' flag for npx
invocation so the arguments become ['-y', 'agent-relay', 'mcp'] (i.e., insert
'-y' before 'agent-relay') for both occurrences; update the argument arrays used
in the relaycast config-add calls so npx runs non-interactively.
In `@packages/sdk-py/src/agent_relay/client.py`:
- Line 352: Replace single-line if statements that combine the condition and
assignment on one line (which trigger Ruff E701) with standard multi-line if
blocks: find the occurrences where you have "if harness is not None:
payload['harness'] = harness" and the similar one around line 398, and change
them to use a multi-line structure (if ... is not None: newline indented
assignment) so each statement is on its own line; keep the same variable names
(payload, harness) and surrounding logic intact.
In `@packages/sdk/src/cli-registry.ts`:
- Around line 124-136: normalizeCliKey and lookupCliKey currently accept inputs
like ":foo" and produce an empty base key; update both to validate the
post-split base key (not just the trimmed input). In normalizeCliKey (function
normalizeCliKey) after splitting on ':' ensure the base part is non-empty and
throw a descriptive Error if it's empty; in lookupCliKey (function lookupCliKey)
perform the same split and return undefined when the base part is empty so
ambiguous keys like ":foo" are rejected consistently.
In `@packages/sdk/src/relay.ts`:
- Around line 646-647: The code is registering instance-specific harnesses into
a shared CLI registry via registerHarnessAdapters(this.harnesses), causing
cross-instance leaks; instead avoid writing to the global registry—keep
harnesses instance-scoped by removing calls to registerHarnessAdapters in the
constructor (and similar calls at 672-679), ensure resolveHarnessForSpawn
consults this.harnesses first (already done) and pass this.harnesses into any
local resolver functions or store an instance-scoped registry on the AgentRelay
object, and if you must register globally, register on construction and
unregister the same entries in shutdown (e.g., via an unregisterHarnessAdapters
counterpart) to prevent leaks.
- Around line 1155-1159: The cached agent returned from the list.map callback
may be missing a sessionId; before returning the existing handle from
this.knownAgents.get(entry.name) update its sessionId with entry.sessionId
(e.g., if (entry.sessionId) existing.sessionId = entry.sessionId) and persist if
needed (this.knownAgents.set(existing.name, existing) or rely on object
reference) so cached agents created earlier get the broker-provided sessionId;
locate the callback around makeAgent/knownAgents.get in relay.ts to apply this
change.
In `@packages/sdk/src/workflows/runner.ts`:
- Around line 2909-2915: The code currently calls registerConfigHarnesses before
validateConfig which can leave global harness state when validation fails;
change execute() and resume() to first resolve variables
(this.resolveVariables), then call this.validateConfig(resolved) and only after
successful validation call this.registerConfigHarnesses(resolved); likewise,
after computing runtimeConfig = this.applyReliabilityDefaults(resolved) run
this.validateConfig(runtimeConfig) (or reuse validation if redundant) and then
call this.registerConfigHarnesses(runtimeConfig); update both occurrences that
currently call registerConfigHarnesses before validateConfig to the new order to
avoid registering harness adapters for invalid configs.
- Around line 571-572: The code calls registerHarnessAdapters(this.harnesses)
which mutates a shared CLI registry read by getCliDefinition() and
buildModelArgs(), causing Runner-scoped harnesses to leak globally; fix by
preventing global mutation—either 1) snapshot the current registry around the
call (save the existing registry, call registerHarnessAdapters(this.harnesses)
to register runner-specific adapters, then restore the saved registry after CLI
construction/parsing), or 2) refactor registerHarnessAdapters to register into a
provided local registry instance (or return an unregister/cleanup function) and
update WorkflowRunner to use that local registry when calling
getCliDefinition()/buildModelArgs(); refer to this.harnesses,
registerHarnessAdapters, getCliDefinition, buildModelArgs, and the
WorkflowRunner constructor to locate the change.
In `@plugins/codex-relay-skill/scripts/setup.sh`:
- Line 157: The args generation currently assumes an unscoped subcommand ("args
= [\"-y\", \"agent-relay\", \"mcp\"]"); update the args_line to invoke the
scoped MCP CLI package instead by using the scoped package name (e.g. set
args_line to "args = [\"-y\", \"`@agent-relay/mcp`\"]") so the script
installs/runs the MCP CLI provided by the `@agent-relay` scope; modify the
args_line symbol in setup.sh accordingly.
In `@scripts/watch-cli-tools.sh`:
- Line 76: Replace the relative path assignment for RELAYCAST_MCP_COMMAND with
an absolute path computed from the script/repo location: determine the
repository root or the script directory at runtime and set/export
RELAYCAST_MCP_COMMAND to the absolute path to dist/src/cli/relaycast-mcp.js
instead of "dist/src/cli/relaycast-mcp.js" so the command no longer depends on
the current working directory; update the export of RELAYCAST_MCP_COMMAND
accordingly.
In `@src/cli/relaycast-mcp.ts`:
- Around line 155-163: The code currently creates and clears
session.subscriptions (SubscriptionManager) lazily and resets it on
workspace/token changes so resources/subscribe loses pre-registration
subscriptions and existing subscriptions stop after workspace.set_key or
agent.register; fix by making a single long-lived SubscriptionManager stored on
the MCP session object (e.g., session.subscriptions) that is initialized once
(if null) and never cleared on rebuilds, update resources/subscribe to always
record URIs via that persistent SubscriptionManager even before registration,
and when rebuilding the websocket bridge (wsBridge) reuse the same
SubscriptionManager rather than creating or nulling it so subscriptions survive
bootstrap/rebind cycles.
---
Outside diff comments:
In `@packages/sdk-py/src/agent_relay/relay.py`:
- Around line 646-657: In list_agents(), when an agent name matches an entry in
self._known_agents, refresh the Agent.session_id from the incoming broker entry
so it doesn't stay stale; specifically, inside the branch where existing =
self._known_agents.get(name) is truthy, set existing.session_id =
entry.get("sessionId") (and then append existing to agents) so the cached Agent
object is updated with the new session id.
In `@packages/sdk/src/workflows/runner.ts`:
- Around line 1747-1761: The harness proxyProvider check is currently evaluated
after the single-provider fallback and gets ignored when
configuredProviders.length === 1; change the logic to evaluate
harnessProxyProvider (from getCliDefinition(agentDef.cli)?.proxyProvider) before
returning a single configured provider so that if harnessProxyProvider is one of
'openai'|'anthropic'|'openrouter' it is returned first; update the code around
configuredProviders and harnessProxyProvider in runner.ts so
harnessProxyProvider takes precedence over the single-provider fallback (or
alternatively gate the single-provider return to only run when
harnessProxyProvider is undefined).
---
Nitpick comments:
In `@packages/sdk/src/__tests__/spawn-harness.test.ts`:
- Around line 79-105: The test registers an adapter with registerHarnessAdapter
including a nonInteractiveArgs function but asserts only serializable fields;
add a brief inline comment near the registration (or above the expect)
clarifying that nonInteractiveArgs is intentionally excluded from the serialized
harness payload because functions are non-serializable and only plain data
(binaries, bypassFlag, searchPaths) should be sent to spawnPty via
AgentRelay.spawn; mention the symbol names registerHarnessAdapter,
nonInteractiveArgs, spawnPty and AgentRelay.spawn so future readers understand
why the assertion omits nonInteractiveArgs.
In `@packages/sdk/src/client.ts`:
- Around line 119-123: Add JSDoc to the exported SpawnAgentResult interface (the
declaration with name, runtime, and optional sessionId) that explains when
sessionId is populated versus undefined, what the sessionId represents/used for
(e.g., correlating agent runtime sessions, resuming or tracking interactions),
and how consumers should handle the optional field (check for undefined, treat
as transient/read-only identifier). Keep the comments short and actionable so
callers of SpawnAgentResult understand lifecycle and nullability of sessionId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9206d8b4-7437-4933-86ad-1c8d2a1f0dd4
⛔ Files ignored due to path filters (3)
.claude/scheduled_tasks.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonplugins/gemini-relay-extension/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
.github/workflows/publish.yml.trajectories/active/traj_4b2d63f6ljvh.json.trajectories/completed/2026-05/traj_dqgg2q4scsvt.json.trajectories/completed/2026-05/traj_dqgg2q4scsvt.md.trajectories/completed/2026-05/traj_ft1pwdlcrmcn.json.trajectories/completed/2026-05/traj_ft1pwdlcrmcn.md.trajectories/completed/2026-05/traj_ft1pwdlcrmcn.trace.json.trajectories/completed/2026-05/traj_pjadgfw0mtw4.json.trajectories/completed/2026-05/traj_pjadgfw0mtw4.md.trajectories/completed/2026-05/traj_pjadgfw0mtw4.trace.json.trajectories/completed/2026-05/traj_r3eic6rt84pq.json.trajectories/completed/2026-05/traj_r3eic6rt84pq.md.trajectories/index.jsonCHANGELOG.mdcrates/broker/src/cli/mod.rscrates/broker/src/cli_mcp_args.rscrates/broker/src/codex_session.rscrates/broker/src/lib.rscrates/broker/src/listen_api.rscrates/broker/src/protocol.rscrates/broker/src/relaycast/mod.rscrates/broker/src/runtime/api.rscrates/broker/src/runtime/init.rscrates/broker/src/runtime/mod.rscrates/broker/src/runtime/relaycast_events.rscrates/broker/src/runtime/session.rscrates/broker/src/runtime/spawn_spec.rscrates/broker/src/runtime/tests.rscrates/broker/src/runtime/worker_events.rscrates/broker/src/snippets.rscrates/broker/src/supervisor.rscrates/broker/src/worker.rscrates/broker/src/wrap.rsdocs/cli-command-tree.mdpackage.jsonpackages/openclaw/skill/SKILL.mdpackages/openclaw/src/setup.tspackages/sdk-py/src/agent_relay/__init__.pypackages/sdk-py/src/agent_relay/builder.pypackages/sdk-py/src/agent_relay/client.pypackages/sdk-py/src/agent_relay/relay.pypackages/sdk-py/src/agent_relay/types.pypackages/sdk-py/tests/test_builder.pypackages/sdk-py/tests/test_relay_harness.pypackages/sdk-swift/Sources/AgentRelaySDK/RelayTypes.swiftpackages/sdk/README.mdpackages/sdk/src/__tests__/spawn-harness.test.tspackages/sdk/src/cli-registry.tspackages/sdk/src/cli-resolver.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/lifecycle-hooks.tspackages/sdk/src/protocol.tspackages/sdk/src/relay-adapter.tspackages/sdk/src/relay.tspackages/sdk/src/types.tspackages/sdk/src/workflows/README.mdpackages/sdk/src/workflows/__tests__/harness-adapters.test.tspackages/sdk/src/workflows/builder.tspackages/sdk/src/workflows/index.tspackages/sdk/src/workflows/process-backend-executor.tspackages/sdk/src/workflows/process-spawner.tspackages/sdk/src/workflows/run.tspackages/sdk/src/workflows/runner.tspackages/sdk/src/workflows/schema.jsonpackages/sdk/src/workflows/types.tspackages/workflow-types/src/index.tsplugins/codex-relay-skill/README.mdplugins/codex-relay-skill/codex-config/config.tomlplugins/codex-relay-skill/scripts/setup.shplugins/gemini-relay-extension/package.jsonplugins/gemini-relay-extension/relay-server.jsscripts/watch-cli-tools.shsrc/cli/bootstrap.tssrc/cli/commands/agent-management.tssrc/cli/relaycast-mcp.startup.test.tssrc/cli/relaycast-mcp.tstests/mcp_merge_e2e.rsweb/content/docs/harnesses.mdxweb/content/docs/reference-cli.mdxweb/content/docs/reference-workflows.mdxweb/content/docs/spawning-an-agent.mdxweb/lib/docs-nav.ts
💤 Files with no reviewable changes (6)
- crates/broker/src/relaycast/mod.rs
- crates/broker/src/wrap.rs
- plugins/gemini-relay-extension/package.json
- crates/broker/src/runtime/init.rs
- .github/workflows/publish.yml
- crates/broker/src/runtime/session.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.trajectories/index.json (1)
1202-1235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUser-specific absolute paths are still being committed.
The five newly added trajectory entries (
traj_pjadgfw0mtw4,traj_dqgg2q4scsvt,traj_r3eic6rt84pq,traj_ft1pwdlcrmcn,traj_4b2d63f6ljvh) all contain absolute paths starting with/Users/will/Projects/AgentWorkforce/relay/, which exposes local username and machine-specific directory structure. These should be repository-relative paths (e.g.,.trajectories/completed/2026-05/traj_*.json).🔧 Example of repository-relative path format
"traj_4b2d63f6ljvh": { "title": "Fix CI run 26263517444", "status": "completed", "startedAt": "2026-05-22T01:47:08.312Z", "completedAt": "2026-05-25T01:45:41.710Z", - "path": "/Users/will/Projects/AgentWorkforce/relay/.trajectories/completed/2026-05/traj_4b2d63f6ljvh.json" + "path": ".trajectories/completed/2026-05/traj_4b2d63f6ljvh.json" }🤖 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 @.trajectories/index.json around lines 1202 - 1235, The path fields for the trajectory entries traj_pjadgfw0mtw4, traj_dqgg2q4scsvt, traj_r3eic6rt84pq, traj_ft1pwdlcrmcn, and traj_4b2d63f6ljvh in .trajectories/index.json contain user-specific absolute paths; change each "path" value to a repository-relative path like ".trajectories/completed/2026-05/<traj_id>.json" (e.g. ".trajectories/completed/2026-05/traj_pjadgfw0mtw4.json") and scan the file for any other absolute paths to replace; if these files are generated, update the generator to use path.relative or a repo-root join instead of hardcoding the user's home/project absolute path.
🤖 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.
Duplicate comments:
In @.trajectories/index.json:
- Around line 1202-1235: The path fields for the trajectory entries
traj_pjadgfw0mtw4, traj_dqgg2q4scsvt, traj_r3eic6rt84pq, traj_ft1pwdlcrmcn, and
traj_4b2d63f6ljvh in .trajectories/index.json contain user-specific absolute
paths; change each "path" value to a repository-relative path like
".trajectories/completed/2026-05/<traj_id>.json" (e.g.
".trajectories/completed/2026-05/traj_pjadgfw0mtw4.json") and scan the file for
any other absolute paths to replace; if these files are generated, update the
generator to use path.relative or a repo-root join instead of hardcoding the
user's home/project absolute path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 74665153-1c19-4305-90a5-fbfe3ed0d4f2
📒 Files selected for processing (15)
.trajectories/completed/2026-05/traj_4b2d63f6ljvh.json.trajectories/completed/2026-05/traj_4b2d63f6ljvh.md.trajectories/completed/2026-05/traj_4b2d63f6ljvh.trace.json.trajectories/index.jsonCHANGELOG.mdcrates/broker/src/protocol.rscrates/broker/src/worker.rspackages/sdk-py/src/agent_relay/types.pypackages/sdk/src/__tests__/spawn-harness.test.tspackages/sdk/src/cli-registry.tspackages/sdk/src/relay.tspackages/sdk/src/workflows/__tests__/harness-adapters.test.tspackages/sdk/src/workflows/schema.jsonpackages/workflow-types/src/index.tsweb/content/docs/harnesses.mdx
✅ Files skipped from review due to trivial changes (3)
- .trajectories/completed/2026-05/traj_4b2d63f6ljvh.json
- CHANGELOG.md
- web/content/docs/harnesses.mdx
There was a problem hiding this comment.
1 issue found across 16 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
willwashburn
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/relay.ts (1)
646-646:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize local harness keys before storing them.
resolveHarnessForSpawn()looks up harnesses by the normalized base CLI key, but these paths store the raw name. A registration like"codex:o3"or" custom "succeeds here and is never found during spawn, so the relay silently falls back to the built-in/default harness instead of the caller’s override.Suggested fix
function cloneHarnessMap( harnesses: Record<string, HarnessDefinition> | undefined ): Record<string, HarnessDefinition> { if (!harnesses) return {}; return Object.fromEntries( - Object.entries(harnesses).map(([name, definition]) => [name, defineHarnessDefinition(name, definition)]) + Object.entries(harnesses).map(([name, definition]) => { + const key = name.trim().split(':')[0]?.trim(); + if (!key) { + throw new Error('Harness name must be a non-empty string'); + } + return [key, defineHarnessDefinition(key, definition)]; + }) ); } registerHarness(name: string, definition: HarnessDefinition): this { - const key = name.trim(); + const key = name.trim().split(':')[0]?.trim(); if (!key) { throw new Error('registerHarness() expects a non-empty harness name'); } const resolved = defineHarnessDefinition(key, definition); this.harnesses[key] = cloneHarnessDefinition(resolved);Also applies to: 671-678
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/relay.ts` at line 646, The harness map is stored with raw keys so resolveHarnessForSpawn can't find entries; when assigning this.harnesses (and similarly in the block around the other assignment at 671-678), normalize each key from options.harnesses to the same base-CLI form resolveHarnessForSpawn expects (trim whitespace, normalize case and any prefix/suffix formatting) before calling cloneHarnessMap/storeing into this.harnesses so lookups by normalized CLI key succeed for registered overrides.
♻️ Duplicate comments (1)
packages/sdk/src/workflows/runner.ts (1)
2098-2112:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSnapshot/restore still races across concurrent runners.
This still mutates a process-global harness registry for the full run. If two
WorkflowRunners overlap, whichever one restores first can wipe out the other runner's adapters and make CLI resolution nondeterministic.🤖 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/workflows/runner.ts` around lines 2098 - 2112, The current installConfigHarnesses/restoreConfigHarnesses flow mutates a process-global registry (via snapshotHarnessAdapters, registerHarnessAdapters, restoreHarnessAdapters) and races across concurrent WorkflowRunner instances; instead create a per-run HarnessRegistry instance (or context) and register this.harnesses and config?.harnesses into that local registry rather than the global one, then update any resolver/consumer functions to accept and use the per-run registry (rather than relying on global registerHarnessAdapters/restoreHarnessAdapters); if immediate global compatibility is required, protect the snapshot/restore with a process-wide lock (mutex) to make the swap atomic, but preferred fix is to avoid global mutation by introducing and passing a per-run registry (replace calls to snapshotHarnessAdapters/registerHarnessAdapters/restoreHarnessAdapters with building and using the new HarnessRegistrySnapshot-like object locally and wire consumers to accept it).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/src/workflows/runner.ts`:
- Line 3168: The call to installConfigHarnesses(config) happens before entering
runWorkflowCore()'s guarded try, so if installConfigHarnesses throws the
catch/finally never run and runner state can be left in pending; move the call
into the try block inside runWorkflowCore(), assign its result to the existing
harnessSnapshot variable there, and adjust the finally block to only call
restore (or the equivalent cleanup) when harnessSnapshot is truthy/defined;
apply the same change to the other location around line 3550 where
installConfigHarnesses is invoked.
---
Outside diff comments:
In `@packages/sdk/src/relay.ts`:
- Line 646: The harness map is stored with raw keys so resolveHarnessForSpawn
can't find entries; when assigning this.harnesses (and similarly in the block
around the other assignment at 671-678), normalize each key from
options.harnesses to the same base-CLI form resolveHarnessForSpawn expects (trim
whitespace, normalize case and any prefix/suffix formatting) before calling
cloneHarnessMap/storeing into this.harnesses so lookups by normalized CLI key
succeed for registered overrides.
---
Duplicate comments:
In `@packages/sdk/src/workflows/runner.ts`:
- Around line 2098-2112: The current
installConfigHarnesses/restoreConfigHarnesses flow mutates a process-global
registry (via snapshotHarnessAdapters, registerHarnessAdapters,
restoreHarnessAdapters) and races across concurrent WorkflowRunner instances;
instead create a per-run HarnessRegistry instance (or context) and register
this.harnesses and config?.harnesses into that local registry rather than the
global one, then update any resolver/consumer functions to accept and use the
per-run registry (rather than relying on global
registerHarnessAdapters/restoreHarnessAdapters); if immediate global
compatibility is required, protect the snapshot/restore with a process-wide lock
(mutex) to make the swap atomic, but preferred fix is to avoid global mutation
by introducing and passing a per-run registry (replace calls to
snapshotHarnessAdapters/registerHarnessAdapters/restoreHarnessAdapters with
building and using the new HarnessRegistrySnapshot-like object locally and wire
consumers to accept it).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e046e77-2213-49b4-944f-9d1d431e8edd
📒 Files selected for processing (25)
.trajectories/completed/2026-05/traj_4b2d63f6ljvh.json.trajectories/completed/2026-05/traj_dqgg2q4scsvt.json.trajectories/completed/2026-05/traj_ft1pwdlcrmcn.json.trajectories/completed/2026-05/traj_pjadgfw0mtw4.json.trajectories/completed/2026-05/traj_q97ei72svf2f.json.trajectories/completed/2026-05/traj_q97ei72svf2f.md.trajectories/completed/2026-05/traj_r3eic6rt84pq.json.trajectories/completed/2026-05/traj_r3eic6rt84pq.md.trajectories/index.jsoncrates/broker/src/codex_session.rscrates/broker/src/listen_api.rscrates/broker/src/protocol.rscrates/broker/src/worker.rspackages/openclaw/src/setup.tspackages/sdk-py/src/agent_relay/client.pypackages/sdk-py/src/agent_relay/relay.pypackages/sdk-swift/Sources/AgentRelaySDK/RelayTypes.swiftpackages/sdk/src/__tests__/spawn-harness.test.tspackages/sdk/src/cli-registry.tspackages/sdk/src/relay.tspackages/sdk/src/workflows/__tests__/harness-adapters.test.tspackages/sdk/src/workflows/runner.tsscripts/watch-cli-tools.shsrc/cli/bootstrap.test.tssrc/cli/relaycast-mcp.ts
✅ Files skipped from review due to trivial changes (8)
- .trajectories/completed/2026-05/traj_r3eic6rt84pq.md
- .trajectories/completed/2026-05/traj_q97ei72svf2f.md
- .trajectories/completed/2026-05/traj_q97ei72svf2f.json
- .trajectories/completed/2026-05/traj_pjadgfw0mtw4.json
- .trajectories/index.json
- src/cli/bootstrap.test.ts
- .trajectories/completed/2026-05/traj_ft1pwdlcrmcn.json
- .trajectories/completed/2026-05/traj_4b2d63f6ljvh.json
…omain # Conflicts: # .trajectories/index.json
There was a problem hiding this comment.
4 issues found across 25 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".trajectories/index.json">
<violation number="1" location=".trajectories/index.json:1193">
P2: Use `<repo-root>` placeholder instead of absolute path. The `path` field uses `/Users/will/Projects/AgentWorkforce/relay/...` while every other entry in this file uses the `<repo-root>` convention. This path will be unresolvable on any other machine or CI environment.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| "status": "completed", | ||
| "startedAt": "2026-05-25T14:04:38.337Z", | ||
| "completedAt": "2026-05-25T14:19:16.978Z", | ||
| "path": "/Users/will/Projects/AgentWorkforce/relay/.trajectories/completed/2026-05/traj_ps68dydvgfuz.json" |
There was a problem hiding this comment.
P2: Use <repo-root> placeholder instead of absolute path. The path field uses /Users/will/Projects/AgentWorkforce/relay/... while every other entry in this file uses the <repo-root> convention. This path will be unresolvable on any other machine or CI environment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .trajectories/index.json, line 1193:
<comment>Use `<repo-root>` placeholder instead of absolute path. The `path` field uses `/Users/will/Projects/AgentWorkforce/relay/...` while every other entry in this file uses the `<repo-root>` convention. This path will be unresolvable on any other machine or CI environment.</comment>
<file context>
@@ -1184,6 +1184,13 @@
+ "status": "completed",
+ "startedAt": "2026-05-25T14:04:38.337Z",
+ "completedAt": "2026-05-25T14:19:16.978Z",
+ "path": "/Users/will/Projects/AgentWorkforce/relay/.trajectories/completed/2026-05/traj_ps68dydvgfuz.json"
}
}
</file context>
Suggested change
| "path": "/Users/will/Projects/AgentWorkforce/relay/.trajectories/completed/2026-05/traj_ps68dydvgfuz.json" | |
| "path": "<repo-root>/.trajectories/completed/2026-05/traj_ps68dydvgfuz.json" |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
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
This PR changes harness support from a hard-coded CLI registry plus broker special cases into a config-backed adapter model used by both SDKs and real
agent-relayspawning.adaptertoHarnessDefinitionso a harness can select broker-owned lifecycle behavior separately from its executable name.codex,claude,opencode,gemini,goose,aider,droid, Cursor Agent) as serializable harness configs exposed from the TypeScript SDK.adapter: "codex"withbinary: "company-codex"keeps Codex session/MCP/model behavior while changing the binary/search paths./docs/harnesseswith built-in usage, custom harness definitions, and wrapper examples.Not in this PR
Those are follow-on pieces. This PR establishes the shared harness definition and adapter identity needed for that direction.
Testing
npm --prefix packages/sdk run buildnpx vitest run src/__tests__/spawn-harness.test.ts src/workflows/__tests__/harness-adapters.test.tsnpx tsc --noEmitcargo test -p agent-relay-broker harnesscargo check -p agent-relay-brokernpm --prefix web run buildwebbuild still logs existingapi.github.comDNS failures during static generation in the sandbox, but exits successfully.