[Experiment] orchestrator-v2 hardening: telemetry, resilience, capability routing#43
[Experiment] orchestrator-v2 hardening: telemetry, resilience, capability routing#43matt-wright86 wants to merge 4 commits into
Conversation
- Route A: propagate execution telemetry from the engine through the runner RPC to the TaskSource as a terminal outcome; unify ExecutionStats in interfaces-task-source. - G1: persist sessionId (and telemetry) on the failure path so a failed run stays resumable. - I3: guard task-source callbacks so a throw can't leak a peer's in-flight slot, hang the runner's terminal RPC, or escape as an unhandled rejection (settle/recordBestEffort helpers + a route().catch net). - I1: capability-aware routing -- runners advertise their registered agents in the heartbeat; the orchestrator only dispatches a task to a capable peer (backward compatible: a runner that advertises nothing = capable of anything). - Adds regression tests (orchestrator.spec I3, peer-registry.spec I1). 43/43 tests pass; build + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- examples/claude-engine: a runnable real-engine deployment (runner.yaml onboarding + a claude-CLI Engine that maps CLI JSON to EngineResult, so real token/cost telemetry flows). Registers examples/* as a workspace member. - docs/experiment: the session's gap report, retrospective (lessons), and dogfood-run notes with telemetry receipts (three escalating real-engine runs, incl. the G1 resume payoff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds telemetry (ExecutionStats) propagation from task engines through TaskSource/RPC to the orchestrator, introduces capability-aware peer routing via heartbeat capability advertisement and PeerRegistry matching, adds error-isolation helpers (recordBestEffort, RPC try/catch), and adds a Claude CLI example plus experiment documentation. ChangesTelemetry propagation through TaskSource and RPC
Capability-aware peer routing
RPC error isolation and dispatch/disconnect resilience
Claude-engine example, workspace config, and experiment documentation
Sequence Diagram(s)sequenceDiagram
participant RunTaskAgent
participant Engine
participant DispatchHandler
participant ResultHandler
participant TaskSource
RunTaskAgent->>Engine: execute(context, sessionId)
Engine-->>RunTaskAgent: engineResult (sessionId, stats)
RunTaskAgent->>RunTaskAgent: ctx.setState(sessionId)
RunTaskAgent-->>DispatchHandler: ScriptResult with telemetry
DispatchHandler->>ResultHandler: task.complete/task.fail RPC {taskId, telemetry}
ResultHandler->>TaskSource: completeTask/failTask(taskId, telemetry)
sequenceDiagram
participant Runner
participant PeerRegistry
participant Orchestrator
Runner->>PeerRegistry: heartbeat with capabilities
PeerRegistry->>PeerRegistry: store capabilities set per peer
Orchestrator->>PeerRegistry: waitForAvailablePeer(capabilityKey(agentType, agentName))
PeerRegistry-->>Orchestrator: matching peer via canHandle()
Related Issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: orchestrator-v2, telemetry, capability-routing, documentation Suggested reviewers: devzeebo 🐰 A carrot-fueled hop through code so deep, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
orchestrator-v2/packages/orchestrator/src/peer-registry.spec.ts (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for explicit empty-capabilities advertisement.
Current tests cover omitted
capabilities(treated as "capable of anything") and non-empty capability sets, but not a runner advertisingcapabilities: []explicitly — whichcanHandletreats as "capable of nothing," the opposite of the omitted-field case. A regression test would help lock in this distinction (see related comment onrunner.ts).🤖 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 `@orchestrator-v2/packages/orchestrator/src/peer-registry.spec.ts` around lines 29 - 41, Add a regression test in PeerRegistry capability routing to cover the explicit empty-capabilities case, since the current suite only checks omitted capabilities and non-empty sets. Update peer-registry.spec.ts with a scenario using a runner that advertises capabilities: [] and assert that selecting_a_peer_for_the_special_agent does not choose that runner, reflecting the canHandle behavior in runner.ts where an explicit empty list means capable of nothing. Keep the existing capability-routing tests intact and name the new case clearly so it documents the distinction from the omitted-field behavior.
🤖 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 `@orchestrator-v2/docs/experiment/dogfood-runs.md`:
- Around line 78-89: The telemetry receipt block in the dogfood runs doc is
using a bare fenced code block, which triggers markdownlint. Update the fenced
block to include an explicit language tag, using the existing telemetry table
snippet near the summary section so the document remains lint-clean. Keep the
content unchanged and only adjust the opening/closing fence around the table
output.
In `@orchestrator-v2/docs/experiment/lessons.md`:
- Around line 81-88: Update the prose in the “Hardest — a 3-stage pipeline with
retry + session-resume” lesson so the phrase “analyze task” is replaced with
“analysis task.” Keep the rest of the paragraph unchanged and ensure the wording
reads as a noun phrase for readability/lint compliance.
In `@orchestrator-v2/examples/claude-engine/README.md`:
- Around line 31-36: The expected-output fenced block in the README example is
missing a language tag, which causes the docs lint failure. Update the fenced
block around the sample output in the README to use an explicit tag such as text
or bash so the example stays lint-clean.
In `@orchestrator-v2/examples/claude-engine/run.mjs`:
- Around line 66-77: The demo TaskSource in run.mjs is stateless, so
checkpoint/session updates and failure telemetry are dropped. Update the source
object’s setState, completeTask, and failTask methods to keep a small in-memory
state store and persist incoming state changes, and make failTask forward its
telemetry argument in the log path just like completeTask does. Use the existing
source object and its methods as the place to wire this in so the demo exercises
the telemetry plumbing end to end.
- Around line 31-55: The catch block in the claude engine run path is dropping
useful JSON from failed CLI executions, so partial session data is lost. Update
the error handling around execFileAsync in run.mjs to inspect and parse
error.stdout before falling back to error.message, and use that parsed payload
to populate sessionId, lastMessage, and stats when available. Keep the existing
success path in the same function, but make the failure path preserve any valid
JSON emitted by claude -p --output-format json.
In `@orchestrator-v2/packages/interfaces-task-source/src/types.ts`:
- Around line 92-109: The isExecutionStats type guard currently treats any
number as valid, so NaN and Infinity can slip through as telemetry. Update the
validation in isExecutionStats to require each EXECUTION_STATS_FIELDS entry to
be a finite numeric value, using the existing record check in types.ts so
corrupt stats are rejected before reaching completeTask or failTask.
In `@orchestrator-v2/packages/orchestrator/src/rpc-router.ts`:
- Around line 65-70: The success reply is inside the same try/catch as the state
update and scheduler call, so a throw from sendRpcResponse can be misreported as
TASK_SOURCE_ERROR or SCHEDULER_ERROR and may send a duplicate rpc.response.
Refactor rpc-router.ts in handleTaskSourceUpdate and handleSchedulerCall so only
the risky operation (taskSource.setState or scheduler.call) is inside the try,
then sendRpcResponse after the try succeeds, mirroring ResultHandler.settle to
keep success replies outside the error-handling scope.
- Around line 20-24: The top-level catch in rpc-router’s route() fire-and-forget
path only logs and can leave a request without any response frame, causing the
caller’s RPC to hang. Update the error handling around this.router/route(peer,
payload.id, payload.method, payload.params) so that when an unexpected exception
escapes route(), you also send an error reply for that requestId via
sendRpcError (or equivalent), while keeping the existing log; if sendRpcError
can fail, guard that call too so the peer still won’t be left waiting
indefinitely.
---
Nitpick comments:
In `@orchestrator-v2/packages/orchestrator/src/peer-registry.spec.ts`:
- Around line 29-41: Add a regression test in PeerRegistry capability routing to
cover the explicit empty-capabilities case, since the current suite only checks
omitted capabilities and non-empty sets. Update peer-registry.spec.ts with a
scenario using a runner that advertises capabilities: [] and assert that
selecting_a_peer_for_the_special_agent does not choose that runner, reflecting
the canHandle behavior in runner.ts where an explicit empty list means capable
of nothing. Keep the existing capability-routing tests intact and name the new
case clearly so it documents the distinction from the omitted-field behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32972857-2f51-44a7-99b9-a8712cac6889
⛔ Files ignored due to path filters (1)
orchestrator-v2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
orchestrator-v2/docs/experiment/README.mdorchestrator-v2/docs/experiment/dogfood-runs.mdorchestrator-v2/docs/experiment/gap-report.mdorchestrator-v2/docs/experiment/lessons.mdorchestrator-v2/examples/claude-engine/README.mdorchestrator-v2/examples/claude-engine/package.jsonorchestrator-v2/examples/claude-engine/run.mjsorchestrator-v2/packages/agent-3-task/src/run-task-agent.tsorchestrator-v2/packages/interfaces-task-source/src/index.tsorchestrator-v2/packages/interfaces-task-source/src/types.tsorchestrator-v2/packages/interfaces-task/src/index.tsorchestrator-v2/packages/interfaces-task/src/types.tsorchestrator-v2/packages/orchestrator/src/best-effort.tsorchestrator-v2/packages/orchestrator/src/dispatch-ack-handler.tsorchestrator-v2/packages/orchestrator/src/orchestrator.spec.tsorchestrator-v2/packages/orchestrator/src/orchestrator.tsorchestrator-v2/packages/orchestrator/src/peer-registry.spec.tsorchestrator-v2/packages/orchestrator/src/peer-registry.tsorchestrator-v2/packages/orchestrator/src/result-handler.tsorchestrator-v2/packages/orchestrator/src/rpc-router.tsorchestrator-v2/packages/protocol/src/frames.tsorchestrator-v2/packages/protocol/src/index.tsorchestrator-v2/packages/protocol/src/types.tsorchestrator-v2/packages/runner/src/dispatch-handler.tsorchestrator-v2/packages/runner/src/heartbeat.tsorchestrator-v2/packages/runner/src/runner.tsorchestrator-v2/pnpm-workspace.yamlorchestrator-v2/publish.js
| ``` | ||
| task att model outcome in out cacheR cost$ ms | ||
| ------------------------------------------------------------------------ | ||
| an-telemetry 1 haiku completed 10 473 21220 0.0171 6931 | ||
| an-routing 1 haiku failed 10 531 17157 0.0253 6980 | ||
| an-resilience 1 haiku completed 10 609 17157 0.0255 7265 | ||
| an-routing 2 haiku completed 10 402 27639 0.0059 5037 | ||
| synthesize 1 sonnet completed 11202 169 23131 0.0780 3334 | ||
| critique 1 haiku completed 10 538 21220 0.0175 7613 | ||
| ------------------------------------------------------------------------ | ||
| TOTAL 6 calls 11252 2722 127524 0.1693 37160 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the telemetry receipt fence.
Markdownlint flags this bare fence. Add a language tag such as text so the doc stays lint-clean.
Fix
-```
+```text
task att model outcome in out cacheR cost$ ms
------------------------------------------------------------------------
an-telemetry 1 haiku completed 10 473 21220 0.0171 6931
@@
TOTAL 6 calls 11252 2722 127524 0.1693 37160
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| task att model outcome in out cacheR cost$ ms | |
| ------------------------------------------------------------------------ | |
| an-telemetry 1 haiku completed 10 473 21220 0.0171 6931 | |
| an-routing 1 haiku failed 10 531 17157 0.0253 6980 | |
| an-resilience 1 haiku completed 10 609 17157 0.0255 7265 | |
| an-routing 2 haiku completed 10 402 27639 0.0059 5037 | |
| synthesize 1 sonnet completed 11202 169 23131 0.0780 3334 | |
| critique 1 haiku completed 10 538 21220 0.0175 7613 | |
| ------------------------------------------------------------------------ | |
| TOTAL 6 calls 11252 2722 127524 0.1693 37160 | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@orchestrator-v2/docs/experiment/dogfood-runs.md` around lines 78 - 89, The
telemetry receipt block in the dogfood runs doc is using a bare fenced code
block, which triggers markdownlint. Update the fenced block to include an
explicit language tag, using the existing telemetry table snippet near the
summary section so the document remains lint-clean. Keep the content unchanged
and only adjust the opening/closing fence around the table output.
Source: Linters/SAST tools
| **Hardest — a 3-stage pipeline with retry + session-resume.** Three analyses fanned out to an | ||
| _analyzer_ runner (concurrent), a _synthesizer_ runner (a **sonnet** engine) combined them, then a | ||
| _critic_ runner (haiku) reviewed the result — capability-routed across three specialized runners with | ||
| **heterogeneous engines**. One analyze task was flaky: it failed after doing its work, and the task | ||
| source **retried it with `--resume`** — attempt 2 cost **$0.0059 vs. $0.0253** (4.3× cheaper) by | ||
| reusing the session instead of rebuilding context. That is the concrete payoff of the G1 fix (failed | ||
| runs stay resumable). Per-attempt telemetry receipts and the pipeline's (self-critiquing) output are | ||
| in **[dogfood-runs.md](./dogfood-runs.md)**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the noun phrase.
“analyze task” should be “analysis task” here; the current wording reads like a verb phrase and trips readability/lint checks.
🧰 Tools
🪛 LanguageTool
[grammar] ~84-~84: Ensure spelling is correct
Context: ...ers with heterogeneous engines. One analyze task was flaky: it failed after doing i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@orchestrator-v2/docs/experiment/lessons.md` around lines 81 - 88, Update the
prose in the “Hardest — a 3-stage pipeline with retry + session-resume” lesson
so the phrase “analyze task” is replaced with “analysis task.” Keep the rest of
the paragraph unchanged and ensure the wording reads as a noun phrase for
readability/lint compliance.
Source: Linters/SAST tools
| ``` | ||
| orchestrator listening on ws://127.0.0.1:xxxxx | ||
| ✓ t1 completed telemetry={in:10, out:61, turns:1, $0.02..} | ||
| ✓ t2 completed telemetry={in:10, out:128, turns:1, $0.01..} | ||
| done. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tag the output fence.
The expected-output block is missing a language tag, which triggers MD040 in the docs lint. Mark it as text or bash so the README stays lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@orchestrator-v2/examples/claude-engine/README.md` around lines 31 - 36, The
expected-output fenced block in the README example is missing a language tag,
which causes the docs lint failure. Update the fenced block around the sample
output in the README to use an explicit tag such as text or bash so the example
stays lint-clean.
Source: Linters/SAST tools
| try { | ||
| const { stdout } = await execFileAsync("claude", args, { | ||
| cwd: ctx.workingDir, | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| timeout: 90_000, | ||
| }); | ||
| const r = JSON.parse(stdout); | ||
| return { | ||
| success: !r.is_error, | ||
| skipFulfill: false, | ||
| lastMessage: r.result ?? null, | ||
| sessionId: r.session_id, | ||
| stats: { | ||
| durationMs: r.duration_ms ?? 0, | ||
| inputTokens: r.usage?.input_tokens ?? 0, | ||
| outputTokens: r.usage?.output_tokens ?? 0, | ||
| cacheReadTokens: r.usage?.cache_read_input_tokens ?? 0, | ||
| cacheCreationTokens: r.usage?.cache_creation_input_tokens ?? 0, | ||
| totalCostUsd: r.total_cost_usd ?? 0, | ||
| numTurns: r.num_turns ?? 0, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| return { success: false, skipFulfill: false, lastMessage: String(error?.message ?? error), stats: null }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the example file first
ast-grep outline orchestrator-v2/examples/claude-engine/run.mjs --view expanded
# Show the relevant section with line numbers
sed -n '1,220p' orchestrator-v2/examples/claude-engine/run.mjs | cat -n
# Search for related result/failure handling and any CLI contract docs
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'is_error|session_id|usage|total_cost_usd|num_turns|stats: null|skipFulfill|failTask|setState|claude' \
orchestrator-v2Repository: devzeebo/bifrost
Length of output: 33106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline orchestrator-v2/examples/claude-engine/run.mjs --view expanded
printf '\n--- run.mjs ---\n'
sed -n '1,220p' orchestrator-v2/examples/claude-engine/run.mjs | cat -n
printf '\n--- related symbols ---\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'is_error|session_id|usage|total_cost_usd|num_turns|stats: null|skipFulfill|failTask|setState|claude' \
orchestrator-v2Repository: devzeebo/bifrost
Length of output: 33148
🌐 Web query:
Claude Code CLI JSON output non-zero exit stdout stderr is_error session_id usage total_cost_usd
💡 Result:
When using Claude Code in headless mode via claude -p with the --output-format json flag, the CLI outputs a single JSON object to stdout containing comprehensive session metadata [1][2]. Key JSON Fields: - is_error: A boolean indicating whether the session encountered a fatal error [2]. - session_id: A unique string identifier for the session, used for resuming conversations via --resume [1][2][3]. - total_cost_usd: A numeric value representing the cost in USD [2][4]. In a fresh session, this is the cost of the specific call; in resumed sessions, it is cumulative across all turns in that session [3]. - usage: An object providing a flat token breakdown, including input_tokens, output_tokens, and cache-related fields [2][3]. - result: The model's primary text response [2]. - structured_output: Populated only when --json-schema is utilized [1][2]. Exit Codes and Error Handling: Currently, the CLI's exit code behavior is limited for programmatic automation [5][6]. The command typically returns an exit code of 1 for various failure types, including API errors (e.g., 500, 429, 401) and local execution errors [5]. Stderr is often empty in these scenarios, making it difficult for calling programs to distinguish between transient (retryable) and permanent failures based solely on standard streams or exit codes [5][6]. Developers often must parse the is_error field within the JSON object to programmatically confirm if a session failed [2]. It is important to note that the JSON output is a single object, not an array of messages [1][7]. While official documentation and examples recommend using jq to parse these fields (e.g., jq -r '.session_id'), inconsistencies in some documentation sections have occasionally mislabeled the output format as an array [1][7].
Citations:
- 1: https://code.claude.com/docs/en/headless
- 2: https://github.com/vladolaru/claude-code-plugins/blob/main/docs/claude-code-cli-reference.md
- 3: https://agentmastered.com/getting-started/json-envelope/
- 4: [DOCS] Headless mode docs omit total_cost_usd field from JSON output description anthropics/claude-code#20893
- 5: Feature request: structured error output and exit code differentiation for claude -p pipe mode anthropics/claude-code#35540
- 6: [FEATURE] Failure classification taxonomy for headless/autonomous sessions anthropics/claude-code#33558
- 7: [DOCS] JSON output format description varies between Headless and Common Workflows guides anthropics/claude-code#19942
Preserve JSON from failed CLI runs. claude -p --output-format json can still emit session metadata on a non-zero exit, but this catch block drops error.stdout and loses session_id, usage, and any partial result. Parse the rejected stdout before falling back to the message.
🤖 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 `@orchestrator-v2/examples/claude-engine/run.mjs` around lines 31 - 55, The
catch block in the claude engine run path is dropping useful JSON from failed
CLI executions, so partial session data is lost. Update the error handling
around execFileAsync in run.mjs to inspect and parse error.stdout before falling
back to error.message, and use that parsed payload to populate sessionId,
lastMessage, and stats when available. Keep the existing success path in the
same function, but make the failure path preserve any valid JSON emitted by
claude -p --output-format json.
| const source = { | ||
| async *watchTasks() { | ||
| for (const t of tasks) yield t; | ||
| }, | ||
| async completeTask(taskId, telemetry) { | ||
| log(` ✓ ${taskId} completed telemetry=${fmt(telemetry)}`); | ||
| }, | ||
| async failTask(taskId, error) { | ||
| log(` ✗ ${taskId} failed: ${error}`); | ||
| }, | ||
| async pauseTask() {}, | ||
| async setState() {}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the demo TaskSource stateful.
setState() is a no-op and failTask() ignores its telemetry argument, so checkpoint/session updates disappear and the failure path never exercises the new telemetry plumbing. If this example is meant to validate the hardening work, keep a tiny in-memory store here and forward telemetry on failures too.
🤖 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 `@orchestrator-v2/examples/claude-engine/run.mjs` around lines 66 - 77, The
demo TaskSource in run.mjs is stateless, so checkpoint/session updates and
failure telemetry are dropped. Update the source object’s setState,
completeTask, and failTask methods to keep a small in-memory state store and
persist incoming state changes, and make failTask forward its telemetry argument
in the log path just like completeTask does. Use the existing source object and
its methods as the place to wire this in so the demo exercises the telemetry
plumbing end to end.
|
|
||
| const EXECUTION_STATS_FIELDS = [ | ||
| "durationMs", | ||
| "inputTokens", | ||
| "outputTokens", | ||
| "cacheReadTokens", | ||
| "cacheCreationTokens", | ||
| "totalCostUsd", | ||
| "numTurns", | ||
| ] as const; | ||
|
|
||
| export function isExecutionStats(value: unknown): value is ExecutionStats { | ||
| if (value === null || typeof value !== "object") { | ||
| return false; | ||
| } | ||
| const record = value as Record<string, unknown>; | ||
| return EXECUTION_STATS_FIELDS.every((field) => typeof record[field] === "number"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Type guard accepts NaN/Infinity as valid telemetry.
typeof value === "number" is true for NaN and Infinity. If an engine miscomputes a stat (e.g., division by zero), this guard will still validate it and let corrupt telemetry flow through to completeTask/failTask.
🛠️ Proposed fix
export function isExecutionStats(value: unknown): value is ExecutionStats {
if (value === null || typeof value !== "object") {
return false;
}
const record = value as Record<string, unknown>;
- return EXECUTION_STATS_FIELDS.every((field) => typeof record[field] === "number");
+ return EXECUTION_STATS_FIELDS.every((field) => Number.isFinite(record[field]));
}🤖 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 `@orchestrator-v2/packages/interfaces-task-source/src/types.ts` around lines 92
- 109, The isExecutionStats type guard currently treats any number as valid, so
NaN and Infinity can slip through as telemetry. Update the validation in
isExecutionStats to require each EXECUTION_STATS_FIELDS entry to be a finite
numeric value, using the existing record check in types.ts so corrupt stats are
rejected before reaching completeTask or failTask.
| // route() is fire-and-forget; ensure a rejecting handler can never escape as an | ||
| // unhandled promise rejection (which is process-fatal on default Node settings). | ||
| void this.route(peer, payload.id, payload.method, payload.params).catch((error: unknown) => { | ||
| console.error(`Failed to handle RPC "${payload.method}":`, error); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Top-level .catch only logs — never replies, so an unexpected error can still hang the caller's RPC.
The fix eliminates the unhandled-rejection risk, but if an exception escapes route() before any handler calls sendRpcResponse/sendRpcError for that requestId (e.g. an unforeseen error path), the peer never gets a response frame and its RPC call hangs indefinitely — exactly the "hang terminal RPCs" failure mode this cohort is meant to close.
🔧 Suggested fix: also reply with an error response
void this.route(peer, payload.id, payload.method, payload.params).catch((error: unknown) => {
console.error(`Failed to handle RPC "${payload.method}":`, error);
+ sendRpcError(peer, payload.id, "INTERNAL_ERROR", errorMessage(error));
});Note sendRpcError itself could throw (e.g. disconnected peer) so ideally wrap it too, but this closes the common gap.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // route() is fire-and-forget; ensure a rejecting handler can never escape as an | |
| // unhandled promise rejection (which is process-fatal on default Node settings). | |
| void this.route(peer, payload.id, payload.method, payload.params).catch((error: unknown) => { | |
| console.error(`Failed to handle RPC "${payload.method}":`, error); | |
| }); | |
| // route() is fire-and-forget; ensure a rejecting handler can never escape as an | |
| // unhandled promise rejection (which is process-fatal on default Node settings). | |
| void this.route(peer, payload.id, payload.method, payload.params).catch((error: unknown) => { | |
| console.error(`Failed to handle RPC "${payload.method}":`, error); | |
| sendRpcError(peer, payload.id, "INTERNAL_ERROR", errorMessage(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 `@orchestrator-v2/packages/orchestrator/src/rpc-router.ts` around lines 20 -
24, The top-level catch in rpc-router’s route() fire-and-forget path only logs
and can leave a request without any response frame, causing the caller’s RPC to
hang. Update the error handling around this.router/route(peer, payload.id,
payload.method, payload.params) so that when an unexpected exception escapes
route(), you also send an error reply for that requestId via sendRpcError (or
equivalent), while keeping the existing log; if sendRpcError can fail, guard
that call too so the peer still won’t be left waiting indefinitely.
| try { | ||
| await this.taskSource.setState(parsed.taskId, parsed.taskState); | ||
| sendRpcResponse(peer, requestId, { ok: true }); | ||
| } catch (error) { | ||
| sendRpcError(peer, requestId, "TASK_SOURCE_ERROR", errorMessage(error)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
try scope also covers the success-path sendRpcResponse, risking a duplicate/misleading error reply.
If sendRpcResponse throws after setState/scheduler.call already succeeded, the catch block reports it as TASK_SOURCE_ERROR/SCHEDULER_ERROR and sends a second rpc.response for the same requestId — misleading and potentially double-answering the caller. ResultHandler.settle (result-handler.ts) keeps the success reply outside the error-handling scope for this reason.
🔧 Suggested fix: only guard the risky call, reply outside the try
try {
await this.taskSource.setState(parsed.taskId, parsed.taskState);
- sendRpcResponse(peer, requestId, { ok: true });
} catch (error) {
sendRpcError(peer, requestId, "TASK_SOURCE_ERROR", errorMessage(error));
+ return;
}
+ sendRpcResponse(peer, requestId, { ok: true });Apply the analogous change to handleSchedulerCall.
Also applies to: 84-89
🤖 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 `@orchestrator-v2/packages/orchestrator/src/rpc-router.ts` around lines 65 -
70, The success reply is inside the same try/catch as the state update and
scheduler call, so a throw from sendRpcResponse can be misreported as
TASK_SOURCE_ERROR or SCHEDULER_ERROR and may send a duplicate rpc.response.
Refactor rpc-router.ts in handleTaskSourceUpdate and handleSchedulerCall so only
the risky operation (taskSource.setState or scheduler.call) is inside the try,
then sendRpcResponse after the try succeeds, mirroring ResultHandler.settle to
keep success replies outside the error-handling scope.
…t layout - I1: treat null (unadvertised) capabilities as "no capabilities" rather than "handles anything" (fail-closed) per review; stub runners now advertise, and peer-registry.spec asserts an unadvertised runner is not selected. - rpc-router: fold the error-message unwrap into sendRpcError and drop the standalone errorMessage helper. - heartbeat: pass the capabilities array directly instead of cloning it. - peer-registry.spec: move the Context type and helpers below the describe block. - lessons: note the fail-closed capability behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
oxfmt pass over pre-existing files flagged by vp check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Exploratory hardening of the
orchestrator-v2rebuild — telemetry propagation, failureresilience, and capability-aware routing — plus a runnable real-engine example and a full
write-up. Bundles several distinct fixes for shared review (happy to split into focused PRs);
see
orchestrator-v2/docs/experiment/.Motivation
A session that audited and dogfooded orchestrator-v2 found that its "thin" terminal boundary
dropped everything but
taskId— execution telemetry, task-source callback errors, and runnercapability all had to be threaded deliberately. This branch does that threading, with regression
tests, and captures the findings.
Type of change
/simplifypassesorchestrator.spec(I3),peer-registry.spec(I1)Checklist
go/npxwere not usedmake lintpasses — N/A:orchestrator-v2uses its ownvp/oxlint toolchain (not wired intomake);vp checkis cleanmake testpasses — N/A:vp run -r testis green (43/43)make buildpasses — N/A:vp run -r buildis cleanmain; branch is current withmain/simplifypasses)Notes for reviewers
pursued, it should split into focused PRs (Route A / I1 / I3).
shape as the telemetry gap, for the result), reconcile 3 correctness-level doc gaps, build
agent-4-workflow+ aTaskSourceread-side.canHandle's fail-open default (nullcapabilities → handles anything) as surprising. It's deliberate (backward-compat for
non-advertising runners, load-bearing for the stub-runner tests) but deserves an explicit
rationale comment or an opt-in.
orchestrator/package.json(thetest-helpersexport) was deliberately leftout of this branch.
CONTRIBUTING.mdpredatesorchestrator-v2/and doesn't document itsvptoolchain — worth updating.🤖 Generated with Claude Code