Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 7146b06708ada50ed6307348aa2a8695f7b20f96 and ccaaeb11d670f4ee5d85bf555662db4d4ff9c89c. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds shared heartbeat freshness states with a ten-minute stale threshold. It parses host-recorded heartbeat timestamps and preserves unreadable values separately from missing values. Worker-list projections expose heartbeat state and age independently from process liveness. The CLI renders numeric ages beside heartbeat states. Federation handling documents host-clock arrival timestamps. Tests cover missing, fresh, stale, future, unreadable, timezone-less, and omitted heartbeat data. Merge Risk: ⚪ Minimal · up to The change adds heartbeat freshness visibility to worker listings and CLI output without altering heartbeat delivery or storage. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
src/main/runtime/orchestration/db/dispatch-context/dispatch-completion.ts (1)
52-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the new TypeScript comments.
Replace multi-line explanations with one concise, non-obvious comment. Remove comments where the test name or type already states the behavior.
src/main/runtime/orchestration/db/dispatch-context/dispatch-completion.ts#L52-L58: Reduce the arrival-time contract to one concise comment.src/main/runtime/orchestration/db/worker-terminal/worker-terminal-listing.ts#L21-L25: Reduce the timestamp and skew explanation to one concise comment.src/main/runtime/orchestration/db/worker-terminal/worker-terminal-listing.ts#L41-L42: Remove or shorten the explanation of publishing unreadable timestamps.src/main/runtime/rpc/methods/orchestration-manual-dispatch-observation.test.ts#L258-L259: Shorten the writer-format explanation.src/main/runtime/rpc/methods/orchestration-manual-dispatch-observation.test.ts#L269-L270: Shorten the malformed-timestamp explanation.src/main/runtime/rpc/methods/orchestration-manual-dispatch-observation.test.ts#L280-L281: Shorten the future-timestamp explanation.src/main/runtime/orchestration/federation-sync.ts#L255-L257: Reduce the relay timestamp contract to one concise comment.src/main/runtime/orchestration/federation-heartbeat-arrival-clock.test.ts#L6-L8: Shorten the suite-level clock explanation.src/main/runtime/orchestration/federation-heartbeat-arrival-clock.test.ts#L115-L117: Shorten the relay-payload explanation.src/cli/handlers/orchestration.ts#L481-L483: Reduce the compatibility explanation to one concise comment.src/cli/handlers/orchestration-worker-list-host-skew.test.ts#L4-L6: Shorten the formatter-mocking explanation.As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
src/cli/handlers/orchestration-worker-list-host-skew.test.ts (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a platform-independent test working directory.
Replace
'/tmp/repo'withprocess.cwd(). The worker-list tests must run unchanged on Windows.
src/cli/handlers/orchestration-worker-list-host-skew.test.ts#L35-L35: Setcwdtoprocess.cwd().src/cli/handlers/orchestration-worker-cli.test.ts#L306-L306: Setcwdtoprocess.cwd().As per coding guidelines, “Use
path.joinor Electron/Node path utilities for file paths; never assume/or\.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81e71322-cfb1-4fb8-9bbf-0e0dd03d06ea
📥 Commits
Reviewing files that changed from the base of the PR and between 80b2a02 and 07413d39db723a509ffac93e09229c1f0700aead.
📒 Files selected for processing (10)
src/cli/handlers/orchestration-worker-cli.test.tssrc/cli/handlers/orchestration-worker-list-host-skew.test.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration-worker-specs.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-completion.tssrc/main/runtime/orchestration/db/worker-terminal/worker-terminal-listing.tssrc/main/runtime/orchestration/federation-heartbeat-arrival-clock.test.tssrc/main/runtime/orchestration/federation-sync.tssrc/main/runtime/rpc/methods/orchestration-manual-dispatch-observation.test.tssrc/main/runtime/rpc/methods/orchestration-worker-release.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
568e5c9 to
c115f60
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebuilt this branch on top of What changed in the design: freshness is no longer a parallel column of its own. It is an optional The Verified on Windows 11 with Node 24.19.0 and pnpm 12.0.0: |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 042f63a3-5476-4aad-8a65-27a923551113
📥 Commits
Reviewing files that changed from the base of the PR and between 8cd0abf and c115f60df40f74152ee9b91332631baa1fd8c9f2.
📒 Files selected for processing (13)
src/cli/handlers/orchestration-worker-list-heartbeat-cli.test.tssrc/cli/handlers/orchestration/worker-terminal-handlers.tssrc/cli/specs/orchestration-worker-specs.tssrc/main/runtime/orchestration/coordinator-task-dispatch.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-completion.tssrc/main/runtime/orchestration/db/utc-timestamp.tssrc/main/runtime/orchestration/db/worker-terminal/worker-terminal-listing.tssrc/main/runtime/orchestration/federation-sync-message.tssrc/main/runtime/rpc/methods/orchestration/worker/worker-list-heartbeat-freshness.test.tssrc/shared/orchestration-fleet-projection.tssrc/shared/orchestration-fleet-worker-projection.tssrc/shared/orchestration-heartbeat-freshness.test.tssrc/shared/orchestration-heartbeat-freshness.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/runtime/orchestration/db/dispatch-context/dispatch-completion.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: c2d84db5-7704-4067-94fa-8e2ccd1b9e76
📥 Commits
Reviewing files that changed from the base of the PR and between c115f60df40f74152ee9b91332631baa1fd8c9f2 and 7146b06708ada50ed6307348aa2a8695f7b20f96.
📒 Files selected for processing (9)
src/cli/handlers/orchestration-worker-list-heartbeat-cli.test.tssrc/cli/handlers/orchestration/worker-terminal-handlers.tssrc/cli/specs/orchestration-worker-specs.tssrc/main/runtime/orchestration/db/utc-timestamp.tssrc/main/runtime/orchestration/db/worker-terminal/worker-terminal-listing.tssrc/main/runtime/rpc/methods/orchestration/worker/worker-list-heartbeat-freshness.test.tssrc/shared/orchestration-fleet-projection.tssrc/shared/orchestration-heartbeat-freshness.test.tssrc/shared/orchestration-heartbeat-freshness.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli/specs/orchestration-worker-specs.ts
- src/cli/handlers/orchestration/worker-terminal-handlers.ts
- src/cli/handlers/orchestration-worker-list-heartbeat-cli.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
`worker-list` could say whether a lane's process was live, but not whether the agent was still reporting. A live process whose agent went quiet is exactly the lane a coordinator loses, and neither signal can be derived from the other. Carry `dispatch_contexts.last_heartbeat_at` through the fleet projection as an optional `heartbeat` field beside `liveness`, and print `heartbeat=<state>` after `liveness=` on the human row. `stale` reuses the 10-minute threshold `warnStaleDispatches` already applies, moved to a shared module so both surfaces keep one definition of the word. The stamp is arrival time on the Run home — every writer stamps it with that clock — so the age is a single-clock subtraction and a federated worker clock cannot skew it. The field is optional at every seam: a host that does not publish it makes the CLI print no segment.
…al stamp out of "never" Two review findings on the worker-list heartbeat receipt. The projection already measured `heartbeat.ageSeconds`, but the CLI response type dropped it and the human line printed `heartbeat=stale` alone, which cannot tell an 11-minute silence from an overnight one. Carry the field and render a short unit beside the state: `heartbeat=stale (43m)`. The age is floored and keeps its sign, so a stamp ahead of this host still reads as negative rather than as "just reported". `none` has no age to print, and a host that publishes the state without the age still prints the bare state. `readUtcTimestampMs` folded a `Date.parse` failure into the same `null` that means "the column was never written", so a corrupt `last_heartbeat_at` was published as `state: 'none'` — a Dispatch that did report, described as one that never did. It now returns `'unreadable'` for a stored value no parser accepts, distinct from `null`, and `FleetHeartbeat` gains the matching state. The string literal rather than `NaN` keeps caller arithmetic type-checked and the value JSON-safe.
…p sub-second future leads negative
Two follow-up review findings on the same receipt.
`exposeUtcTimestamp('')` returns the empty string, and the falsy guard in
`readUtcTimestampMs` then folded it into the `null` that means "the column was
never written". Only SQL NULL is silence: an empty string is a value that was
stored and lost its contents, so it now falls through to the parse and is
classified `unreadable`.
`Math.round(-0.4)` is `-0`, which JSON publishes as `0`, so a stamp up to 499 ms
ahead of this host's clock read as `0s` — the reassurance the negative age exists
to deny. The future branch now floors at one second, so any lead reports at least
`-1s` and survives serialization.
…eat tests The changed-code casting gate rejects new `as` casts. The RPC test now reads the handler's inferred result and the public `db.db` handle, and the CLI test keeps only the RuntimeClient double, with the SAFETY rationale the sibling CLI tests use.
ccaaeb1 to
b8184bf
Compare
This branch has not been deployed
1 participant
ELI5
orca orchestration worker-listcould already tell you whether a lane's process looked alive. It could not tell you whether the agent was still checking in. This adds that second answer next to the first, so a row readsliveness=live heartbeat=stale (43m)when the process is up but the agent went quiet, and the age says whether that silence is minutes or hours old.What Changed
src/shared/orchestration-heartbeat-freshness.ts:projectDispatchHeartbeat(lastReceivedAt, now)returns{ state: 'none' | 'fresh' | 'stale' | 'unreadable', lastReceivedAt, ageSeconds }.unreadableis a stored arrival stamp this host cannot parse — corruption, kept apart from thenonethat means the Dispatch never reported.listWorkerTerminalResourcesnow selectsdispatch_contexts.last_heartbeat_atand exposes it as epoch ms (lastHeartbeatAt).readUtcTimestampMsreturnsnumber | null | 'unreadable':null= the column holds no value,'unreadable'= it holds one no parser accepts. The string literal rather thanNaNkeeps the value JSON-safe (NaNserializes tonull, which would re-create the bug on a wire hop) and makes TypeScript reject arithmetic on it at every call site.OrchestrationFleetWorkergains an optionalheartbeatfield, projected right besidelivenessinprojectOrchestrationFleetWorker. Theworker-listRPC already publishes the whole projection, so no RPC-shape change was needed.worker-listrow printsheartbeat=<state> (<age>)afterliveness=, with the age floored to one short unit (43s/2m/3h) and signed, so a stamp ahead of this host's clock reads-4sinstead of collapsing into a reassuring "just reported".noneandunreadablemeasure no age and print bare; a host that publishes the state without the age still prints the bare state.coordinator-task-dispatch.tsinto the shared module;warnStaleDispatchesnow imports it, so the CLI wordstaleand the coordinator warning cannot drift apart.recordHeartbeat'satis arrival time on this host, and both writers (the federation relay import and the legacy lifecycle RPC) stamp it locally.liveness.verdict(live/unverifiable/exited) is untouched: it is the execution host's verdict on the process, whileheartbeatis protocol-level proof the agent is still reporting — a live process can stop reporting and a reporting agent can have an unverifiable process, so the two are published side by side rather than merged.Why
A coordinator loses a lane in two different ways, and until now
worker-listcould only see one of them.agent_status/ execution-host evidence answers "is the process there"; the heartbeat answers "is the agent still talking to me". Fixing one on top of the other would let aliveprocess hide a silent agent — the failure mode behind #15951.The threshold is not a new number.
warnStaleDispatcheshas warned at 10 minutes (the documented 5-minute heartbeat cadence x 2) for a long time; this reuses that constant instead of inventing a second definition of "stale", which is why it moved to a shared module rather than being copied.Age is a single-clock subtraction.
last_heartbeat_atis written only byrecordHeartbeat, whose contract is thatatis when this host received the heartbeat — the federated relay path deliberately stamps on import and the relayed payload carries no worker timestamp. So a federated lane's age measures silence, not clock skew.Linked Issue
Fixes #15951
Visual Proof
N/A— CLI-only. The rendered change is one extra segment on the existingworker-listrow:That exact line is asserted in
orchestration-worker-list-heartbeat-cli.test.ts.Testing
Rebuilt on top of
upstream/mainafter #16904 landed; the previous branch predated the fleet projection and no longer applied.Gates run locally on Windows 11 (Node 24.19.0, pnpm 12.0.0):
pnpm tc:node— cleanpnpm tc:cli— cleanpnpm run check:code-quality:changed—0 new finding(s) across 13 changed file(s)for oxlint, type-aware, and React Doctorvitest run src/main/runtime/rpc/methods/orchestration/worker src/main/runtime/rpc/methods/orchestration/federation— 50 files, 380 tests passedvitest run src/main/runtime/orchestration— 109 files, 883 tests passedvitest run src/shared— 7218 passed; 7 failures insecure-path-windows-acl.win32,git-fetch-head-lock-key-derivation,agent-hook-listener-relay-dependency, andexternal-automation-jobs-file, all of which fail identically on a cleanupstream/maincheckout on this machine and are unrelated to this diffvitest run src/cli/handlers/orchestration*— 201 passed; 4 failures in argv-quoting assertions (orchestration-gate-cli,orchestration-timeout-cli,orchestration-worker-cli) that also reproduce on cleanupstream/mainhereNew tests:
src/shared/orchestration-heartbeat-freshness.test.ts—nonevs reported, the exact stale boundary, a future stamp staying visible as a negative age, and the field being omitted when the caller carries no stamp.src/main/runtime/rpc/methods/orchestration/worker/worker-list-heartbeat-freshness.test.ts— DB to RPC: three Dispatches aged in one listing, freshness independent of the liveness verdict, and a SQLite space-format stamp read as UTC.src/cli/handlers/orchestration-worker-list-heartbeat-cli.test.ts— the segment renders afterliveness=, and no segment at all when the host published none.Review round 2 (commit
7146b0670)Addressing the two CodeRabbit findings of 2026-09-07 (render the age; keep an unreadable stamp out of
none). Re-run on the same machine:pnpm tc:node,pnpm tc:cli— cleanORCA_CODE_QUALITY_BASE=upstream/main pnpm run check:code-quality:changed—0 new finding(s) across 13 changed file(s)for oxlint, type-aware, and React Doctorvitest run src/cli/handlers/orchestration src/main/runtime/rpc/methods/orchestration/worker src/main/runtime/orchestration/db— 693 passed; the same 4 argv-quoting failures as before, re-confirmed to reproduce with this commit's changes stashedvitest run src/shared/orchestration-*.test.ts— 12 files, 108 tests passedTests added in this round:
orchestration-heartbeat-freshness.test.ts—'unreadable'projectsstate: 'unreadable'with a null age, distinct fromnone; the age formatter floors to one unit and keeps the sign.worker-list-heartbeat-freshness.test.ts—'not-a-timestamp'stored inlast_heartbeat_atpublishesunreadableover RPC while a sibling Dispatch that never reported still publishesnone.orchestration-worker-list-heartbeat-cli.test.ts— the full rendered row asserted exactly, plus fresh-with-age,none,unreadable, and a host that published the state without the age.Review round 3 (commit
ccaaeb11d)Two follow-up Minor findings, both about a corrupt or skewed value borrowing a reassuring word:
readUtcTimestampMsguarded with a falsy check, so an empty stored string folded into thenullthat means "never written". The guard is nowexposed === null, and''falls through to the parse and classifies asunreadable.Math.round(-0.4)is-0and JSON publishes that as0, so a stamp under 500 ms ahead of this host's clock read as0s. The future branch now floors at one second, so any lead reports at least-1sand survives serialization.Regression tests:
''stored inlast_heartbeat_atpublishingunreadableover RPC; leads of 1 ms / 400 ms / 499 ms / 500 ms all reporting-1after aJSON.parse(JSON.stringify(...))round trip, with 1.6 s still reporting-2.Gates re-run:
pnpm tc:nodeandpnpm tc:cliclean;check:code-quality:changed0 new finding(s) across 13 changed file(s);vitest run src/cli/handlers/orchestration src/main/runtime/rpc/methods/orchestration/worker src/main/runtime/orchestration/db694 passed with the same 4 pre-existing argv-quoting failures;vitest run src/shared/orchestration-*.test.ts109 passed.Not checked: I have not driven a live multi-lane run against a built app for this branch, so the evidence above is the automated suites only.
AI Disclosure
Written with Claude (Opus) in Claude Code. All commands, gate output, and the pre-existing-failure comparison against
upstream/mainwere run and read by me.Review
SSH / federation. The heartbeat is Run-home state: it lives in the home
dispatch_contextstable and is written on arrival, whether it came in over the local lifecycle RPC or through the federation relay import.--include-remotestill asks the execution host only about the process, andapplyFederatedFleetObservationsis untouched — a host verdict overrideslivenessand never rewritesheartbeat, which is correct because the remote host has no view of the home's arrival record. A federated lane whose relay is down therefore reportsliveness=unverifiablewith a truthfulheartbeat=stale, which is exactly the distinction a coordinator needs.Wire compatibility.
heartbeatis a new optional field on an existing object (docs/reference/remote-wire-compatibility.md, Rule 1: additive optional fields are safe). No new stream opcode and no capability negotiation. The follow-up commit widens the publishedheartbeat.stateenum withunreadable; that reaches older clients, but the CLI renders the state as text rather than switching on it, so an older client prints the new word instead of dropping a frame. Mixed versions were handled explicitly in both directions: a new CLI against an older runtime sees noheartbeatand prints no segment (covered by a test), and an older CLI against a new runtime ignores the extra field. The projection deliberately distinguishes absent (lastHeartbeatAt === undefined, host does not carry the column, omit the field) from null (state: 'none', this Dispatch genuinely never reported), so a mixed-version gap can never be misread as a silent worker.Cross-platform. No paths, no shell, no process spawning; SQLite timestamps are normalized through the existing
exposeUtcTimestampregex rather than by string slicing, and a stamp that parses to nothing degrades tounreadable— never to a bogus instant, and never to thenonethat would describe a reporting Dispatch as one that never reported.Performance. One extra column on a
SELECTthat already reads the same row; no new query, no new join, no extra round trip. The projection is arithmetic on a number.Security. No new authority, no new capability, no new surface.
last_heartbeat_atwas already published raw byexposeDispatchContextonworker-show.UI. CLI text only; no renderer or mobile surface reads the new field.
Folder workspaces. The feature is keyed on the Dispatch, not on a worktree, so folder workspaces behave identically.
Git providers. Not applicable; nothing touches source control.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
The 10-minute constant now has one home. If a future change re-tunes the heartbeat cadence,
DISPATCH_HEARTBEAT_STALE_AFTER_MSis the single place both the coordinator warning and the CLI word follow.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Typecheck and the changed-code quality gate pass locally; the full
pnpm testandpnpm buildare left to CI, since this machine has pre-existing unrelated failures insrc/sharedandsrc/cli/handlersthat reproduce on a cleanupstream/maincheckout.Author