Skip to content

feat(cli): orca serve stats [--json] — live headless-serve runtime stats - #10612

Open
LesleyMurfin wants to merge 24 commits into
stablyai:mainfrom
LesleyMurfin:ai/serve-stats-json-10608
Open

LesleyMurfin wants to merge 24 commits into
stablyai:mainfrom
LesleyMurfin:ai/serve-stats-json-10608

Conversation

@LesleyMurfin

@LesleyMurfin LesleyMurfin commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

orca serve stats is a quick headcount for a running Orca server. It reports recognized agents, terminals, worktrees, retained orchestration tasks, uptime, version, and the WebSocket port — plus host load/memory pressure and event-loop health. Scripts can ask for JSON and can select a paired remote server with --environment or --pairing-code.

An agent waiting for your answer still counts. Terminals created without a desktop window count too. The port follows the actual listener, including rebind and shutdown. host.* is host-wide, not Orca-attributed — a runaway terminal child shows up in it too.

Closes #10608. Related #10609 and #9840 remain separate resource-health work.

Behavior and implementation

  • Registers serve stats in the command specs, handler, and lazy dispatch manifest. Bare serve still launches locally; the stats subcommand honors remote selection.
  • Adds the serve.stats RPC with no parameters. JSON retains the normal CLI response envelope (ok, result, _meta). The result contains version, runtimeId, uptimeSeconds, port, counts.{agents,tasks,terminals,terminalsUnverifiable,terminalsExited,worktrees,browserPages,browserPagesRetained,tasksByStatus,agentsByState,workersByTerminalState}, host.{loadAverage1m,cpuCoreCount,memoryTotalBytes,memoryAvailableBytes,memoryAvailableSource,swapUsedBytes}, and health.{eventLoopDelayP99Ms}. The three breakdown objects always report every key (0, never omitted); unmeasurable fields are null, never 0 (e.g. loadAverage1m on Windows, swapUsedBytes off Linux, eventLoopDelayP99Ms before the monitor's first sample — which resets on read, so each call reports the window since the previous call).
  • Terminals count connected PTYs in the runtime registry, including headless terminals, split by exit evidence: terminalsExited is what the owning host positively reported gone, terminalsUnverifiable is everything else with no such evidence (not proof of exit, and it does not authorize cleanup). Agents use the existing published pane identity on those PTYs, plus attached native structured sessions with a live lease. Working, waiting, and idle agents are included; recognized identity follows Orca's existing evidence rules.
  • browserPages counts both client-hosted pages and the WebContents-backed pages a headless serve opens offscreen, de-duplicated by page id. browserPagesRetained is the client-hosted subset whose host is gone — a subset of browserPages, not a partition (an offscreen page has no separate host to lose, so it's never retained).
  • Tasks count retained task rows, including completed and failed history. The query uses SQL COUNT(*), avoiding materializing task specifications and joined dispatch rows merely to count them. tasksByStatus does not sum to counts.tasks: it includes the completed/failed rows that counts.tasks excludes.
  • agentsByState reports unknown for any agent whose turn state isn't currently provable (all structured sessions, plus ptys with no live status) — unknown is never idle. workersByTerminalState covers every retained dispatch, so reclaimable/release-unknown pileups are visible without worker-list.
  • Worktrees use the existing managed-worktree listing and visibility rules, including folder workspaces. This listing can refresh repository scans; the command is not a CPU/RAM measurement or a guaranteed constant-time overload probe.
  • The listener publishes its actual bound port on initial bind and rebind, and clears it after shutdown or failed binding (including when a widen's own recovery path also fails — see "Updates since review"). Runtime proxies can omit the local setter.

Review fixes

Independent review found and fixed four defects before merge:

  1. Renderer leaf counts missed headless PTYs.
  2. Turn-duration statistics dropped live agents when they waited for input.
  3. The port became stale after listener lifecycle changes.
  4. The CLI help and handler existed, but the lazy dispatch manifest omitted the command, causing Unknown command: serve stats.

Fix commits: 4a83692898b and cfb6e73fe90. Tests reproduce the failures and exercise real runtime terminal creation, working→waiting transitions, structured native-session ownership, and a real bound WebSocket listener. The original handler tests alone did not catch missing CLI registration.

Updates since review (cfb6e73f → a53803ba)

Three feature commits landed after the review above, followed by two fix-up commits addressing CI and an open review comment:

  • 50a836f9 — reports host load average, CPU core count, memory total/available (with source: proc-meminfo vs free-memory), swap used, and event-loop delay p99. Adds host.* and health.* to the contract.
  • 82bfc0ff — splits proven pty exits (terminalsExited) from unverifiable ones, and separates headless browser pages from client-hosted ones (browserPages/browserPagesRetained).
  • 2817d68e — keeps a proven pty exit recorded across inventory refresh instead of losing the evidence.
  • a53803ba — two unrelated fix-ups:
    • Addresses an outstanding CodeRabbit finding on this PR (runtime-rpc-network-exposure.ts:53): if current.stop() itself rejects during a WebSocket widen, servePort was never cleared, and if the subsequent loopback recovery also failed, serve stats could keep reporting a port nothing was listening on. Now cleared unconditionally in the failure branch.
    • Resyncs en-runtime-required.json (TerminalPane.minimumContrast.* had drifted from en.json after an unrelated upstream change) — fixes the runtime-required-catalog test and static analysis CI checks. Unrelated to serve-stats.

Verified locally for a53803ba: typecheck clean; runtime-required-catalog.test.ts (22 tests) and the runtime-rpc-pairing-offer/runtime-rpc-pairing-mode-persistence suites covering widenWebSocketBind/recoverWebSocketBindAfterFailedWiden (17 tests) all pass. Full CI matrix is running against this push; see checks below rather than the stale per-check history from cfb6e73f.

Validation

Historical validation below was run against cfb6e73fe901f42b64e11d4d613816d65de77f7b, the SHA reviewed before the four commits in "Updates since review" landed. It documents real end-to-end coverage of the command's original surface; the fields added since then are covered by the new unit/integration tests in those commits (serve-stats-host.test.ts, serve-stats-lifecycle.test.ts, pty-inventory-liveness-verdict.test.ts, serve-stats.test.ts) and by CI, not by a repeat of this manual pass.

  • macOS: 132 tests across nine runtime/CLI/DB suites; dispatch-manifest parity adds 11 passing tests. Node and CLI typechecks pass; touched-file lint/format pass. CLI and Electron builds pass.
  • Real built headless server + paired CLI: seeded a test repo/worktree, read stats, created a terminal, executed a marker command, and read stats again. Port was 7389; terminal count changed 1→2. Test worktree and server were cleaned up. This dev harness reads the existing dev profile, so absolute worktree/task counts are not a hermetic zero-state assertion. Direct unpackaged Electron reports its own version; packaged version follows the existing AppEnvironment contract.
  • Linux via Orca SSH: all ten suites, 143 passed. Windows awin via Orca: all ten suites, 141 passed, 2 skipped. Both checked out cfb6e73f and used ORCA_BACKGROUND_LAUNCH=1.
  • No full-repository test or release build is claimed. No merge performed.

Visual proof

N/A for rendered UI: this is a CLI/RPC feature. Behavioral proof is the real paired CLI/PTY round trip above, plus the regression tests. Example captured result after terminal creation, at cfb6e73f (before host/health and the breakdown fields were added):

{"version":"43.4.1","uptimeSeconds":7,"port":7389,"counts":{"agents":0,"tasks":7,"terminals":2,"worktrees":471}}

The version above is the raw Electron development harness version, not a packaged Orca release version. The current contract additionally returns runtimeId, the expanded counts breakdowns, host, and health — see "Behavior and implementation" for the full shape.

Compatibility and review scope

Additive RPC and result type; existing clients and persisted settings remain unchanged. Old servers return the existing method-not-found error for the new command. No stream opcode, pairing identity, migration, install hook, dependency, or provider-specific source-control behavior changes. Counts describe runtime-known occupancy, not proof of death for unreachable remote work; they must not authorize cleanup. Windows/Linux paths and SSH ownership use existing runtime sources. Native structured leases are counted without probing OS processes or re-reading journals.

Readiness review covered correctness/failure paths, security, resource use, mobile/wire compatibility, platform/remoting behavior, and code organization. All four findings received three independent counsel reports, all P2. Blind peer ratings were completed for both review rounds. The Grok seat was attempted but its weekly usage limit prevented a report; the three remaining seats completed both rounds. P2 here means a defect in a new diagnostic rather than loss of an existing core workflow; all four were fixed anyway.

Contribution and AI disclosure

Original feature contributed by Lesley Murfin, AI-assisted as disclosed in the original PR. Maintainer review added the corrective commits and independent validation above. Original Linux/WSL smoke history remains in the existing PR comment.

Updates since a53803ba (22ee7230f2)

What changed: three new fields — renderer footprint, long-poll budget, cgroup pids — each null (never 0) wherever the platform cannot measure it. Verified live on a built headless serve, and that run caught a cgroup-path defect before it shipped. The counts.* breakdowns and the host.*/health.* fields added since the 2026-07-26 smoke comment are already described under "Behavior and implementation" and "Updates since review" above; this section covers only what landed after a53803ba.

New fields

  • counts.browserPageMemoryTotalBytes / counts.browserPageMemoryMaxBytes — resident set of the renderers behind counts.browserPages, de-duplicated by pid because Electron may back several pages with one renderer. A total alone hides the [Bug]: Agent-driven built-in browser tabs are never reclaimed on headless serve — one renderer per tab, no cap #14552 shape (one 1.3 GB page among six), so the max ships beside it. One /proc/<pid>/status VmRSS read per pid — no subprocess, no ps sweep. Page counting moved into the same collector: the bridge's page-id map is both the page population and the only handle a footprint read can start from.
  • health.longPolls — active vs cap for the total, ask, browser-host and specialized admission pools. longPollCap: continuously-held check polls starve worker-start; cap is 16 and not configurable #19342 hit runtime_busy at loadavg 4.8 with 44 GB free and could only find the cap by unpacking app.asar. The counters and caps live on the RPC server, so it registers a reader with the runtime where it already sets the serve port — a pull, so nothing pushes on the long-poll hot path and the snapshot cannot lag the counters. null when no listener is serving.
  • host.pids — cgroup v2 pids.current / pids.max, resolved through /proc/self/cgroup and walked to the nearest enclosing cgroup with the pid controller enabled. The literal max maps to null (unlimited), never to a number.

Live verification (built headless serve, 22ee7230f2)

  • Three offscreen pages reported total=259588096, max=88240128 — matching an independent /proc read of the three renderer pids exactly.
  • Two held terminal.wait calls moved total.active 0 → 2 → 0 against caps 16/8/8/12.
  • host.pids.max matched the unit cgroup's 16384 exactly; current tracked pids.current to within the measuring process's own tasks.

Defect the live run caught before it shipped

Reading a fixed /sys/fs/cgroup path only works inside a container: the kernel omits controller files from the root cgroup, and #18789's pids.max=4096 was a systemd unit limit, not a container limit. The fixed-path read therefore reported the wrong ceiling — or none — on a systemd host. Resolving through /proc/self/cgroup and walking to the nearest pid-controller-enabled cgroup is what produced the exact 16384 match above.

Earlier live-smoke defects, for the record

Both were found by live smoke rather than unit tests, and shipped in 82bfc0ff / 2817d68e:

  1. Headless browser pages were hard-0 on the offscreen path. The count only saw client-hosted pages, so a headless serve's own WebContents-backed pages were invisible — a headless server always reported 0. browserPages now counts both populations, de-duplicated by page id.
  2. A confirmed pty exit was erased by the very next inventory refresh. Positively-reported exit evidence was dropped on re-inventory, so terminalsExited silently regressed into terminalsUnverifiable. The verdict is now retained across refresh.

Tests

New: serve-stats-browser-pages.test.ts, runtime-rpc-long-poll-transport.test.ts. Extended: serve-stats-host.test.ts, serve-stats.test.ts, rpc/methods/serve-stats.test.ts, cli/handlers/core.test.ts. 17 files, +893/−40.

Updates since 22ee7230f2 (7268d85718)

One fix-up commit addressing the latest CodeRabbit round:

  • 7268d85718 — two review fixes: (1) clear the long-poll stats provider when the Unix socket fails to start, so serve stats never advertises long-poll capacity with no listener bound; (2) build cgroup/procfs paths with POSIX literals (/sys/fs/cgroup, /proc/...) instead of path.sep, keeping resolveCgroupV2Directories platform-correct on Windows test hosts. Adds a regression test for the socket-start failure path.

Linked Issue

Architectural Context (RFC #21556)

  • Roadmap Phase: Step 1: Launch & Bind
  • Severity / Priority: P2 (Medium)
  • Why It Ties into the Lifecycle: Operators running headless orca serve had no native CLI command to inspect live PTY counts, memory consumption, and connected clients.
  • System Impact: Implements orca serve stats [--json] exposing real-time server runtime metrics for observability and monitoring.
  • Master Tracking RFC: Part of stablyai/orca#21556

@LesleyMurfin

LesleyMurfin commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Live smoke evidence (2026-07-26)

Live smoke of this draft on a headless Linux serve :6768 (temporary pin 1.4.156-feat-10608 @ 716a8d115, then restore stock 1.4.156).

Smoke orca serve stats --json (feature pin)

{
  "ok": true,
  "result": {
    "version": "1.4.156",
    "uptimeSeconds": 15,
    "port": 6768,
    "counts": {
      "agents": 0,
      "tasks": 112,
      "terminals": 0,
      "worktrees": 26
    }
  }
}

Human output matched. After restore to stock: method_not_found for serve.stats (expected).

No request to undraft yet from this comment — evidence only.

@LesleyMurfin

Copy link
Copy Markdown
Contributor Author

CI status update (2026-09-08)

Current HEAD: f48cb6c7 (ai/serve-stats-json-10608).

Branch is now current with upstream/main (merged, no rebase) and the fork's main has been fast-forwarded to match upstream, so future gate runs diff against the real ~35-file scope instead of a stale multi-thousand-file base.

Fixed this session (see commits on this branch):

  • EAGAIN crash in config/scripts/pr-code-change-scope.mjs — branch was 74 commits behind upstream's streamed-stdin fix for this exact bug. Resolved by the upstream merge.
  • countTasks() included completed/failed rows despite the documented "live counts" contract on RuntimeServeStatsResult. Fixed + regression-tested.
  • Stale advertised port survived a failed transport stop(). Fixed + regression-tested.
  • Upstream's merge added a mandatory runId to OrchestrationDbCore.createTask; this branch's own serve-stats.test.ts / task-store.test.ts / agent-status-producer-census.test.ts didn't supply one. Fixed, verified green on CI (fork run 34220536153, shards 1/8 and 7/8).

Confirmed CI flake, not a regression:

  • browser-route-webrtc-egress.electron.test.ts (shard 8/8) failed once on a 20s Electron-probe watchdog (runner-side dbus/GPU transient errors), passes locally in ~9s, and passed on CI rerun (job 102061995296). Zero file overlap with this PR's diff.

Confirmed pre-existing, not caused by this PR (verified via git blame / diff-scoping, not assumed):

  • runtime-required-catalog.test.ts — 7 unsynced TerminalPane.minimumContrast i18n keys, introduced by upstream commit 12f53da5 (Remove settled-worker automatic resume and hibernation fences #19544), zero overlap with this PR's files.
  • cross-version wire compatibility + Skill update round trip — fork (LesleyMurfin/orca) is missing upstream release tags (v1.4.178-rc.2, v1.4.184, v1.4.190); these jobs resolve tags from the checkout's own remote, so they're invisible in upstream's own CI context.
  • static analysis / React Doctor finding on browser-page-annotation-tray.tsx — authored entirely by upstream commit 60a7ac38 (feat(browser): edit a page annotation's comment and intent inline in the tray #17511), not touched by this PR. Was only visible because the fork's main was 2 weeks stale before today's fast-forward.

No action needed from a reviewer beyond the existing approval gate (action_required on every pull_request run from this account — confirmed as this repo's standing policy for external contributions, not something fixable from the contributor side).

@LesleyMurfin
LesleyMurfin marked this pull request as ready for review September 8, 2026 17:10

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a new orca serve stats [--json] command that queries a running (local or paired/remote) headless-serve runtime for live occupancy stats, backed by a parameterless serve.stats RPC.

  • CLI surface — serve stats handler (client.call('serve.stats') → printResult), command spec, and 'serve stats' key in the lazy-dispatch core group; shouldIgnoreRemoteSelection now ignores only bare serve, so the subcommand honors --environment/pairing selection.
  • RPC — serve.stats registered in ALL_RPC_METHODS and the dispatch manifest (the missing-manifest bug from the prior review is fixed and parity-tested).
  • Aggregation — getServeStats counts agents (connected ptys via existing pane-identity rules + native structured sessions with a live lease), tasks, terminals/unverifiable ptys, worktrees, and browser pages, plus three fixed-key histograms.
  • Port lifecycle — setServePort wired through initial bind, pairing widen, failed-widen restore, failed transport stop, and shutdown so the advertised port never goes stale.
  • Shared canonicalization — OrchestrationTaskStatus and WorkerTerminalListState moved to src/shared so the published histogram keys cannot drift from the DB derivation.
  • Tests — real bound-listener, headless-terminal, working→waiting transition, structured-native-session, and worker-terminal-state coverage; each regression is pinned by an exact assertion that fails without the fix.

I verified the agent-counting paths (no double-count between the pty loop and the native-only structured loop), the deriveServeStatsAgentState precedence against getAgentPromptActivity, the countTasks vs tasksByStatus asymmetry, and every setServePort call site across the lifecycle seams. The declared JSON contract, the additive-RPC wire compatibility (old servers answer method-not-found), and the SSH/unverifiable-verdict vocabulary are all handled consistently.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 6290c9b5-e47f-492e-a0c5-a6181b34ddaf

📥 Commits

Reviewing files that changed from the base of the PR and between 22ee7230f2a83076e9faedd3d8938daedf18df8c and 7268d85718fe71f0d5e2fec70a94caa0c2368605.

📒 Files selected for processing (3)
  • src/main/runtime/runtime-rpc-long-poll-transport.test.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
  • src/main/runtime/serve-stats-host.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
  • src/main/runtime/serve-stats-host.ts
  • src/main/runtime/runtime-rpc-long-poll-transport.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Adds orca serve stats with human-readable and JSON output. Shared contracts define runtime, host, health, task, and worker-terminal statistics. Runtime aggregation counts live tasks, terminals, agents, worktrees, browser pages, and worker terminals. RPC registration exposes serve.stats. CLI routing supports remote runtimes. Lifecycle code tracks the resolved WebSocket port, long-poll capacity, host metrics, and event-loop delay. Tests cover aggregation, lifecycle transitions, RPC dispatch, host metrics, and CLI output.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 7268d

The reviewed changes do not show a concrete unresolved merge risk.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes changes beyond [#10608], including host CPU/RAM and event-loop metrics that the issue lists as non-goals, plus an unrelated localization resynchronization in `src/renderer/src/i18n/en-… Split the unrelated localization fix into a separate pull request. Either remove the host and health metrics from this pull request or link and document the additional approved scope before merge.
Docstring Coverage ⚠️ Warning Docstring coverage is 52.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 41 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation satisfies the primary requirements in [#10608]: it adds the orca serve stats and --json CLI flow, registers the serve.stats RPC, reports live occupancy, version, uptime, and b…
Title check ✅ Passed The title clearly and concisely identifies the main change: adding live runtime statistics to the orca serve stats CLI command with JSON support.
Description check ✅ Passed The description is detailed and covers the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility, review history, and AI disclosure. It does not reproduce every te…
Full details: Out of Scope Changes check

Explanation

The PR includes changes beyond [#10608], including host CPU/RAM and event-loop metrics that the issue lists as non-goals, plus an unrelated localization resynchronization in src/renderer/src/i18n/en-runtime-required.json.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 72ba129d-edbb-4fd3-8399-bf34decb471c

📥 Commits

Reviewing files that changed from the base of the PR and between bba68b1 and f48cb6c7adeb55b871abb3b7eb1033578623093f.

📒 Files selected for processing (32)
  • src/cli/format.ts
  • src/cli/handler-group-manifest.ts
  • src/cli/handlers/core.test.ts
  • src/cli/handlers/core.ts
  • src/cli/index.ts
  • src/cli/runtime-client-deferral.test.ts
  • src/cli/specs/serve.ts
  • src/main/runtime/orca-runtime-pty-foreground-process-reads.ts
  • src/main/runtime/orca-runtime-terminal-drivers.ts
  • src/main/runtime/orchestration/db/tasks/task-store.test.ts
  • src/main/runtime/orchestration/db/tasks/task-store.ts
  • src/main/runtime/orchestration/types.ts
  • src/main/runtime/orchestration/worker-terminal-ownership.ts
  • src/main/runtime/rpc/methods/index.ts
  • src/main/runtime/rpc/methods/orchestration/worker/agent-status-producer-census.test.ts
  • src/main/runtime/rpc/methods/serve-stats.test.ts
  • src/main/runtime/rpc/methods/serve-stats.ts
  • src/main/runtime/runtime-agent-row-store.ts
  • src/main/runtime/runtime-browser-page-registry.ts
  • src/main/runtime/runtime-rpc-pairing-offer.test.ts
  • src/main/runtime/runtime-rpc-shutdown.test.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-network-exposure.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-shutdown.ts
  • src/main/runtime/runtime-worktree-status-projection.ts
  • src/main/runtime/serve-stats-lifecycle.test.ts
  • src/main/runtime/serve-stats.test.ts
  • src/shared/orchestration-fleet-projection.ts
  • src/shared/orchestration-task-status.ts
  • src/shared/runtime-session-contracts.ts
  • src/shared/runtime-types.ts
  • src/shared/worker-terminal-list-state.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/main/runtime/runtime-rpc/runtime-rpc-network-exposure.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — three commits since the prior review extend serve stats with host-wide pressure metrics, an event-loop-delay reading, a proven-exit terminal split, and offscreen-page counting.

  • Host pressure (host.*) — collectServeStatsHost reports load average, core count, total/available memory, availability source, and swap in use from in-process node:os plus one /proc/meminfo read; unmeasurable fields are null in JSON / n/a in human output, never a fabricated 0.
  • Event-loop delay (health.eventLoopDelayP99Ms) — a 20ms monitorEventLoopDelay histogram enabled at listener start, read reset-on-read so a saturated hour isn't diluted by idle days.
  • Proven-exit split — disconnected ptys are now split into terminalsExited (host-delivered exit frame) vs terminalsUnverifiable (lost contact); the exited liveness verdict became a sticky certificate that only positive live re-observation (markPtyLivenessLive) or pty reuse/disposal retracts.
  • Offscreen pages — browserPages now de-dups WebContents/offscreen-backed pages against the client-hosted registry so headless agent-opened tabs stop reporting zero.

I traced the sticky-exited change through the inventory sweep (refreshPtyWorktreeRecordsWithControllerInventory calls markPtyLivenessLive on hasPty() === true, so a positive re-observation still retracts a stale certificate) and the reuse/disposal clear paths, and confirmed the browserPages de-dup against the shared page-id namespace. The new host/health/terminalsExited fields are additive to the declared JSON contract, and the host reader correctly refuses the darwin memory-pressure subprocess and clamps available memory into [free, total]. Tests pin each regression against real libuv sampling and a stubbed win32/procfs-less host.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — one commit since the prior review closes out the port-lifecycle edge case and resyncs a drifted i18n catalog.

  • Widen-recovery servePort clear — when current.stop() itself rejects inside widenWebSocketBind, the catch block now clears servePort before recoverWebSocketBindAfterFailedWiden runs, so a double failure (stop rejects + loopback restore fails) can no longer leave a stale advertised port. Verified that the restored loopback path re-publishes the real port via startWebSocketTransport, so the pre-recovery clear only matters when recovery tears down.
  • i18n catalog resync — drops the drifted TerminalPane.minimumContrast.* keys from en-runtime-required.json, fixing the runtime-required-catalog check; unrelated to serve-stats itself.

The defensive clear is the conservative direction (never report a port with nothing listening) and is consistent with the sibling fix in runtime-rpc-shutdown.ts and the existing setServePort?.(null) convention.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@LesleyMurfin

Copy link
Copy Markdown
Contributor Author

@coderabbitai Re: the "Linked Issues" / "Out of Scope Changes" pre-merge check warnings on host CPU/RAM/swap metrics, event-loop-delay health, and terminal exit-evidence tracking:

These were added intentionally after the original #10608 scope was written, went through the maintainer review round documented above (independent counsel + peer ratings, all P2 and fixed), and are covered by their own tests (serve-stats-host.test.ts, serve-stats-lifecycle.test.ts, pty-inventory-liveness-verdict.test.ts). #10608's original text predates that decision to fold host/health reporting into this same command rather than opening a separate issue for it.

Not pulling this out of the PR — scoping it into serve stats avoids a second RPC/command surface for what's conceptually the same "server occupancy snapshot" query. Leaving this comment for the record rather than acting on the resolution suggestion.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@LesleyMurfin Thanks for the clarification. The added host metrics, event-loop health metric, and terminal exit-evidence tracking are intentional parts of the serve stats snapshot contract. The stated tests cover these additions. I withdraw the scope-based concern.

🐇

You are interacting with an AI system.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e0ab134a-01b9-4341-bf66-db5f7fed2fac

📥 Commits

Reviewing files that changed from the base of the PR and between a53803ba4a10b1be3a9ab3e74e31363bdbde45e3 and 22ee7230f2a83076e9faedd3d8938daedf18df8c.

📒 Files selected for processing (17)
  • src/cli/format.ts
  • src/cli/handlers/core.test.ts
  • src/cli/specs/serve.ts
  • src/main/runtime/orca-runtime-pty-foreground-process-reads.ts
  • src/main/runtime/orca-runtime-terminal-drivers.ts
  • src/main/runtime/rpc/methods/serve-stats.test.ts
  • src/main/runtime/runtime-rpc-long-poll-transport.test.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-request-admission.ts
  • src/main/runtime/runtime-rpc/runtime-rpc-shutdown.ts
  • src/main/runtime/serve-stats-browser-pages.test.ts
  • src/main/runtime/serve-stats-browser-pages.ts
  • src/main/runtime/serve-stats-host.test.ts
  • src/main/runtime/serve-stats-host.ts
  • src/main/runtime/serve-stats.test.ts
  • src/shared/runtime-session-contracts.ts
  • src/shared/runtime-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli/specs/serve.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/main/runtime/runtime-rpc/runtime-rpc-lifecycle.ts
Comment thread src/main/runtime/serve-stats-host.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — one commit since the prior review (a53803ba) extends serve stats with three more observability fields.

  • Renderer footprint (counts.browserPageMemoryTotalBytes / browserPageMemoryMaxBytes) — a new collectServeStatsBrowserPages walks the bridge's page-id registration map once (client-hosted registry pages plus WebContents-backed pages), then sums each renderer's VmRSS from /proc/<pid>/status, de-duplicated by pid; Linux only, null (never 0) elsewhere or when unreadable.
  • Host pid controller (host.pids) — resolves the 0:: line of /proc/self/cgroup and walks nearest-to-root for the enclosing cgroup exposing pids.current/pids.max; the literal max ceiling maps to null (unlimited), not a number.
  • Long-poll admission (health.longPolls) — total/ask/browserHost/specialized pools with active/cap, served via a setLongPollStatsProvider pull-reader registered on listener start and cleared on shutdown; specialized.active is the ask+browserHost sum, matching the shared ceiling in admitLongPoll.
  • CLI/doc surface — human formatter and command-spec notes updated to render the new fields (current=… max=unlimited, pool=active/cap, bytes).

I verified readLongPollStats's caps and the specialized sum against admitLongPoll, the getRegisteredTabs() pageId→webContentsId population and renderer-pid read (webContents.fromId().getOSProcessId()), and the systemd slice-name \x2d escaping in the cgroup path (which matches the on-disk directory name, so no un-escaping is needed). Tests pin each new field with exact assertions and the null-never-0 measurement semantics.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new regression test fails in CI: it calls getServeStats() without seeding an in-memory OrchestrationDb, so getOrchestrationDb() lazy-initializes a real DB through getAppEnvironment().getPath('userData') and throws AppEnvironment not initialized. Reproduced locally with pnpm vitest run src/main/runtime/runtime-rpc-long-poll-transport.test.ts -t "clears the long-poll provider".

Reviewed changes — one commit since the prior review closes a long-poll budget leak on socket-start failure and simplifies the cgroup path resolution.

  • Socket-start failure clears the long-poll provider — socketTransport.start() is wrapped so a failed bind clears setLongPollStatsProvider(null) before re-throwing, matching the existing shutdown path (no listener bound → no admission budget to advertise).
  • POSIX cgroup paths — serve-stats-host.ts now reads literal /proc/meminfo, /proc/self/cgroup, and /sys/fs/cgroup instead of path.join(path.sep, …), dropping the node:path import. These readers are already gated to platform === 'linux', so the hardcoded paths are equivalent and the change is a pure simplification.
  • Regression test — forces UnixSocketTransport.start to reject and asserts health.longPolls is null afterward (the test itself is broken, see the inline comment).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/runtime/runtime-rpc-long-poll-transport.test.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — one commit since the prior review fixes the flagged regression test.

  • In-memory DB seed — the clears the long-poll provider when the Unix socket fails to start test now seeds runtime.setOrchestrationDb(new OrchestrationDb(':memory:')) before exercising getServeStats() and closes it afterward, mirroring the sibling long-poll occupancy test. The previously-flagged lazy-init path (which resolved userData through the app environment) is no longer reachable from this test.

I ran the test with the repo's vitest config (--config config/vitest.config.ts) and it passes; the earlier AppEnvironment not initialized reproduction came from invoking vitest without that config, which is what installs the fake app environment in beforeEach (config/scripts/vitest-host-ports-setup.ts). The in-memory seed is still the right call regardless — it keeps this test off the filesystem like its sibling.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

LesleyMurfin and others added 5 commits September 11, 2026 00:28
Add a read-only CLI + serve.stats RPC that returns live factory occupancy
from a running runtime (agents, tasks, terminals, worktrees) plus version,
uptime, and the post-bind WebSocket port (or null).

- Register under SERVE_COMMAND_SPECS next to bare `serve`
- Honor --environment / pairing (only bare serve ignores remote selection)
- Wire setServePort from RPC server; optional-chain for runtime proxies
- Tests: aggregator, RPC registry dispatch, port-bind regression

Closes stablyai#10608
Add handler unit tests for serve.stats RPC dispatch, factory count
fields, null port rendered as "none", and JSON mode shape.
… ptys

Adds runtimeId plus counts.browserPages, counts.browserPagesRetained and
counts.terminalsUnverifiable to the (unshipped) serve.stats result.

Retained pages hold one of the runtime's 256 page slots with no TTL and no
reaper (stablyai#14552); the disconnected-pty count makes leaked ptys visible (stablyai#9819,
stablyai#9479) without claiming they exited; runtimeId distinguishes a same-generation
runtime from one that restarted underneath the caller (stablyai#9585).
`serve stats` reported only scalars, so the histograms operators actually
needed were hand-tabulated from worker-list and task-list output (stablyai#13047,
stablyai#19388, stablyai#18737). Adds three fixed-key breakdowns to the same single walk:

- counts.tasksByStatus: one grouped scan (countTasksByStatus), every
  TaskStatus always present. Includes completed/failed, which counts.tasks
  excludes, so it deliberately does not sum to counts.tasks.
- counts.agentsByState: working/permission/idle/unknown. PTY-backed agents
  are bucketed from their own current-incarnation evidence (retained hook
  status, prompt lifecycle, live-observed title); structured sessions and
  ptys with no current evidence report `unknown`, because identity and
  liveness prove presence, not turn state (stablyai#19548).
- counts.workersByTerminalState: countWorkerTerminalInventory keyed by all
  six WorkerTerminalListState values, so reclaimable/release_unknown
  pileups are visible without worker-list.

TaskStatus and WorkerTerminalListState move to shared leaf modules so the
wire contract keys on the same unions the DB derives (shared cannot import
main); FleetTerminalState now aliases the latter instead of repeating it.
…tract change

Upstream createTask now requires a runId, and the serve-stats agent-state read is a new hook-row consumer the census has to pin.
…lay p99

serve stats could show every Orca-side count while the host itself was saturated: stablyai#14552 and stablyai#19312 both read as 'Reconnecting to remote runtime' network faults for days (loadavg 105-112, systemd reporting the unit healthy), and stablyai#12588's 60 GiB terminal child degraded SSH with no visible signal.

Adds host.{loadAverage1m,cpuCoreCount,memoryTotalBytes,memoryAvailableBytes,memoryAvailableSource,swapUsedBytes} under its own key (never inside counts, since these are host-wide and not Orca-attributed) and health.eventLoopDelayP99Ms. Unmeasurable fields are null and render n/a, never 0, so Windows' meaningless os.loadavg() zeros cannot read as an idle host. Linux prefers /proc/meminfo MemAvailable over os.freemem() and labels the source; every procfs read is guarded so a container without it yields null instead of failing the command.

The delay histogram is one long-lived monitorEventLoopDelay enabled with the RPC listener and reset on read, so each read reports the window since the previous one rather than a multi-day percentile that goes stale-flat.
`counts.browserPages` counted only RuntimeBrowserPageRegistry, which
`browserTabCreate` populates on the client-hosted branch alone. A headless
serve has no renderer, so `tab create` takes the offscreen branch and the page
was never published there — the field reported 0 for a live, listable,
agent-opened tab, which is exactly the population stablyai#14552 asked for. Count the
WebContents registration map the bridge reads as well, de-duplicated by page
id, and read it via `getRegisteredTabs` rather than `tabList` so a stats read
deregisters nothing. `browserPagesRetained` stays scoped to client-hosted
pages and now says so: an offscreen page has no separate host to lose.

`counts.terminalsUnverifiable` incremented on `connected === false`, but
`onPtyExit` clears `connected` while recording an `exited` liveness verdict
from a host-delivered exit code. Confirmed-dead ptys therefore landed in the
one field documented as carrying no evidence of exit and as not authorizing
cleanup. Split the bucket on that verdict: `terminalsExited` for a positive
death certificate, `terminalsUnverifiable` for absence of contact only, with
anything ambiguous staying on the conservative side.

`terminalsUnverifiable` narrows in meaning; `serve.stats` has not shipped in a
release, so no consumer contract breaks.
The liveness register documented an invariant it did not enforce: an earned
death certificate is kept, and nothing weaker than a host-delivered exit frame
may reach it. Both doubt writers ignored that. The controller-inventory sweep
deleted the entry for any id an answering provider disowned, and recorded
`unverifiable` for any id no provider could be asked about — so a host-delivered
exit only held between `onPtyExit` and the next `terminal list` / `worktree ps` /
`worktree rm`, after which `serve stats` reported the proven death as
`terminalsUnverifiable`, the field whose whole meaning is that cleanup is NOT
authorized.

Enforce it in the register rather than at the sweep's call sites: three writers
in that one sweep shared the bug, plus the stop-error path, and loss of contact
is never a retraction of positive evidence anywhere
(docs/reference/ssh-execution-boundary.md). Doubt stays exactly as clearable as
before.

Disposal is unaffected: retiring a pty or reusing its id deletes the entry
directly (onPtySpawned, registerPty, dropDisconnectedPtyRecord), `getServeStats`
counts from `ptysById` so a disposed pty contributes to neither counter, and the
register's orphan ratchet still bounds retained verdicts at 256.
If current.stop() rejects in widenWebSocketBind(), the servePort clear
on the next line never runs. When the loopback recovery in
recoverWebSocketBindAfterFailedWiden then also fails, wsBoundHost and
the transport are torn down but servePort stays stale, so serve stats
can report a port with nothing listening on it.

Also resyncs en-runtime-required.json: TerminalPane.minimumContrast.*
text had drifted from en.json after an unrelated upstream change,
failing the runtime-required-catalog test and static-analysis CI
checks. Regenerated via sync:localization-runtime-catalog; unrelated
to serve-stats itself.
…roup pids

Three resource-exhaustion signals `serve stats` could not answer, each null (never
0) wherever the platform cannot measure it:

- counts.browserPageMemoryTotalBytes / browserPageMemoryMaxBytes: resident set of
  the renderers behind counts.browserPages, de-duplicated by pid because Electron
  may back several pages with one renderer. stablyai#14552 was one 1.3 GB page among six,
  which a total alone hides, so the max ships beside it. One /proc/<pid>/status
  VmRSS read per pid — no subprocess, no `ps` sweep. Page counting moves into the
  same collector, since the bridge's page-id map is both the page population and
  the only handle a footprint read can start from.

- health.longPolls: active vs cap for the total, ask, browser-host and specialized
  admission pools. stablyai#19342 hit `runtime_busy` at loadavg 4.8 with 44 GB free and
  could only find the cap by unpacking app.asar. The counters and caps live on the
  RPC server, so it registers a reader with the runtime where it already sets the
  serve port — a pull, so nothing pushes on the long-poll hot path and the snapshot
  cannot lag the counters. Null when no listener is serving.

- host.pids: cgroup v2 pids.current / pids.max, resolved through /proc/self/cgroup
  and walked to the nearest enclosing cgroup with the pid controller enabled. A
  fixed /sys/fs/cgroup read only works inside a container: the kernel omits
  controller files from the root cgroup, and stablyai#18789's pids.max=4096 was a systemd
  unit limit. The literal `max` maps to null (unlimited), never to a number.

Verified live on a built headless serve: three offscreen pages reported
total=259588096 max=88240128, matching an independent /proc read of the three
renderer pids exactly; two held terminal.wait calls moved total.active 0 -> 2 -> 0
against caps 16/8/8/12; host.pids.max matched the unit cgroup's 16384 exactly,
with current tracking pids.current to within the measuring process's own tasks.
@svc-orca
svc-orca Bot force-pushed the ai/serve-stats-json-10608 branch from 52c0038 to 2776ecd Compare September 11, 2026 06:29
@nwparker

Copy link
Copy Markdown
Contributor

Fresh behavioral verification (2026-09-11), head 2776ecd5318 plus pushed verification fixes 0c7078d5e66:

  • Built this checkout's Electron server and CLI; used an isolated background profile and a real paired client.
  • Created a real Git worktree, native terminal, and browser page. Terminal command marker and browser navigation/evaluation/screenshot succeeded.
  • serve stats reflected terminal counts 1→2→1 and browser pages 1→0 as resources were created/closed; proven exit count became 1. Ten separate CLI stats calls took 86–105 ms on the small test profile.
  • Exact host collector ran on Linux and Windows. Linux CPU, memory and swap matched OS/procfs; Windows unavailable metrics were null.
  • Fixed an outdated failed-worker test fixture and RPC method typing; stats tests, RPC/CLI integration, node/CLI typechecks and 65 liveness/cleanup regression tests pass.

Two remaining limitations matter for merge review:

  1. Current main conflicts in the agent-status census test and deletes runtime-agent-row-store.ts, which this PR modifies/uses. Integration must adapt to main's replacement state owner and rerun behavior checks.
  2. Linux PID diagnostics only return the nearest readable cgroup. On the real host an unlimited leaf returned {current:63,max:null} despite a parent with {current:653,max:36917}. This cannot be interpreted as effective process headroom across the hierarchy.

Merge risk: high until main integration is resolved and verified. No existing terminal/process-management regression observed; diagnostic hierarchy coverage is incomplete. No merge performed.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — one commit since the prior pullfrog review aligns the serve stats verification with the current runtime APIs. No production behavior changed.

  • Test seeding uses the current worker-terminal API — seedFailedWorkerTerminal now records the owned resource with createWorkerTerminalResourceStatement(...) before calling failWorkerStart(...), replacing the removed adoptResidualTerminal option. This mirrors the real custody-then-fail sequence (recordCreatedWorkerTerminalCustody/prepareStartingWorkerAuthority write the row; failWorkerStart only copies identity), so the seeded reclaimable/retained/release_pending states still derive from deriveWorkerTerminalListState as before.
  • Type-annotation cleanup — serve-stats.ts drops the RpcMethod[] annotation and its type RpcMethod import, matching the sibling stats.ts convention; defineMethod's inferred return type still spreads cleanly into ALL_RPC_METHODS.

I ran the affected suites (src/main/runtime/serve-stats.test.ts, src/main/runtime/rpc/methods/serve-stats.test.ts) — 12 passed.

Pullfrog  | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The merge-resolution code itself is clean, but the branch now fails three blocking static analysis CI gates. Two are caused by the PR's own serve-stats files and were never surfaced; one was introduced by this merge commit. Details inline and below.

Reviewed changes — re-reviewed the delta since the prior pullfrog review (0c7078d5e6 → 8f0f3e7cb2): the upstream/main merge and its conflict resolutions.

  • Merged origin/main (b3579136) — pulled in da5d555259, which deleted RuntimeAgentRowStore in favor of the standalone selectFreshExplicitAgentStatus.
  • Re-pointed serve stats agent-state resolution — resolveServeStatsPtyAgentState now calls selectFreshExplicitAgentStatus instead of the deleted store method; handle-or-paneKey matching and the prompt-floor logic are unchanged.
  • Widened selectFreshExplicitAgentStatus — handle now accepts string | null, guarded by args.handle === null ||; existing non-null callers are unaffected. This re-applies the same widening the PR had made to the deleted store method.
  • Retargeted the lifecycle test — the turn-state test injects a getAgentStatusSnapshot dependency in place of the removed retainAgentRowSnapshot.
  • Formatting/docs — relay files and one e2e assertion reformatted, the cgroup JSDoc gained a hierarchy note, and the producer-census role strings were updated.

⚠️ Generated RPC params catalog is stale

serve.stats is registered in ALL_RPC_METHODS, but src/shared/rpc-contract/rpc-params-catalog.generated.ts has no entry for it, so pnpm run verify:rpc-params-catalog — a required step of the static analysis job in .github/workflows/pr.yml — fails with rpc-params-catalog.generated.ts is out of date.

Technical details
# Stale generated RPC params catalog

## Affected sites
- `src/shared/rpc-contract/rpc-params-catalog.generated.ts` — missing `'serve.stats': null` in `RPC_PARAMS_BY_METHOD`.

## Required outcome
- Regenerate the catalog so `verify:rpc-params-catalog` passes.

## Suggested approach
- Run `pnpm run generate:rpc-params-catalog` and commit the result; it adds exactly one line, `'serve.stats': null,`. A parameterless method needs no `rpc-send-params.ts` change.

## Open questions for the human
- This file is unchanged vs `main` and already lacked `serve.stats` at the prior reviewed commit (`0c7078d5e6`), so this gate appears to have been red before the merge as well — was CI green at any point on this branch?

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/runtime/orca-runtime-pty-foreground-process-reads.ts Outdated
Comment thread src/main/runtime/serve-stats-browser-pages.ts Outdated
- Route webContents OS process ID lookup through RuntimeDesktopSurface to keep electron out of runtime modules.
- Extract serve stats collection logic to serve-stats-aggregation.ts, removing the max-lines bypass.
- Install fake AppEnvironment in serve-stats-lifecycle.test.ts.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

bf791db8b6 clears two of the three blocking static analysis gates flagged last run (max-lines and runtime-electron), but the third — verify:rpc-params-catalog — is still red on this head. One generated line is all that's missing.

Reviewed changes — re-reviewed the delta since the prior pullfrog review (8f0f3e7cb2 → bf791db8b6): a CI-satisfying refactor.

  • Extracted the serve-stats aggregation — getServeStats and resolveServeStatsPtyAgentState moved into a new serve-stats-aggregation.ts (with a structural ServeStatsRuntime type), so orca-runtime-pty-foreground-process-reads.ts drops the max-lines disable and is now 279 lines. The class method delegates; behavior is unchanged.
  • Moved the electron lookup behind a host port — webContents.fromId().getOSProcessId() now lives on the optional RuntimeDesktopSurface.getWebContentsOSProcessId, implemented in src/main/host/electron-runtime-desktop-surface.ts and wired at main-process-preflight.ts:235. The runtime module no longer names electron, and serve-stats-browser-pages.ts keeps its osProcessId injection seam.
  • Test environment setup — the long-poll transport test installs a fake app environment in beforeEach.

Verified on this head: pnpm tc:node clean; serve-stats{,-lifecycle,-browser-pages,-host}.test.ts + rpc/methods/serve-stats.test.ts + runtime-rpc-long-poll-transport.test.ts → 53 passed; check:code-quality:changed clean; check:max-lines-ratchet and check:runtime-electron-ratchet now pass.

⚠️ verify:rpc-params-catalog is still failing

serve.stats is registered in ALL_RPC_METHODS, but src/shared/rpc-contract/rpc-params-catalog.generated.ts still has no entry for it, so pnpm run verify:rpc-params-catalog — a required step of the static analysis job (.github/workflows/pr.yml:210) — exits non-zero on this head. Both prior ratchet findings are fixed, but the fix-up commit did not touch this file, so the branch still cannot go green.

Technical details
# Stale generated RPC params catalog

## Affected sites
- `src/shared/rpc-contract/rpc-params-catalog.generated.ts` — missing `'serve.stats': null`. The generator emits `null` for every `params: null` method (`config/scripts/generate-rpc-params-catalog.mjs:140-142`), so the catalog must be regenerated whenever a parameterless method is added.

## Required outcome
- `pnpm run verify:rpc-params-catalog` passes on the branch head.

## Suggested approach
- Run `pnpm run generate:rpc-params-catalog` and commit the result; it adds exactly one line, `'serve.stats': null,`. A parameterless method needs no `rpc-send-params.ts` change.

## Evidence
- On `bf791db8b6`: `pnpm run verify:rpc-params-catalog` → `src/shared/rpc-contract/rpc-params-catalog.generated.ts is out of date. Run 'pnpm run generate:rpc-params-catalog'`.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — re-reviewed the delta since the prior pullfrog review (bf791db8b6 → b76c5f51f4); it is a single generated-file regeneration with no behavioral surface.

  • Regenerated RPC params catalog — src/shared/rpc-contract/rpc-params-catalog.generated.ts gains 'serve.stats': null in RPC_PARAMS_BY_METHOD (line 1039), which clears the blocking verify:rpc-params-catalog gate flagged in the prior review. Confirmed pnpm run verify:rpc-params-catalog exits 0 on this head; git diff bf791db8b6..HEAD --stat is 1 file, +1.

All three previously tracked blocking static analysis gates (max-lines ratchet, runtime-electron ratchet, RPC params catalog) are now green, and both open threads from review 5186055561 were already resolved by bf791db8b6.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker

Copy link
Copy Markdown
Contributor

Fresh verification on current head b76c5f51f4840e79eb2f574f69a5611838e63623:

  • 10 requested exact-head suites: 145 passed in 6.32s.
  • 8 targeted suites after merging current origin/main: 70 passed in 8.65s.
  • Merge-tree clean; no conflicts. git diff --check clean.
  • Cgroup diagnostics match the documented nearest enabled cgroup-v2 scope. Dedicated tests cover systemd leaf/ancestor, v1, unmounted, malformed, blank, and unlimited (max) cases.
  • No safe additional code fix identified and no files changed.

Recommendation: low merge risk. This verification pass did not merge the PR.

@nwparker

Copy link
Copy Markdown
Contributor

This makes sense + looks good to merge

BUT we're moving some stuff around on orca serve so give just a bit more time!

Disable the process-wide ELD histogram when RPC start fails or stop runs,
document the CLI --json RPC envelope/.result contract, require workspace
permission on serve.stats, drop the browser-pages unknown cast, and null-guard
structured worker host scope when location is missing.
selectFreshExplicitAgentStatus accepts a nullable handle after the main merge,
but the join API is Iterable<string>. Mirror the paneKey guard so a missing
handle becomes an empty list instead of a type error.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: orca serve stats [--json] — live headless-serve runtime stats

2 participants