fix(runtime): harden the ACP/Hermes runtime path — /v1 switch, instance pinning, serialization, context usage#1891
Conversation
…ce pinning, serialization, context usage (ADR 0033)
Four fixes surfaced by making Hermes a first-class runtime, all validated
against a live acp:hermes server:
- Non-streaming turns (OpenAI-compat /v1, desktop /api/chat fallback,
internal self-prompts) now honor agent_runtime — they silently ran the
native loop, which a gateway-less ACP-only setup (the Hermes preset
persona) cannot serve. New _acp_turn_collected folds the frame stream
into the single-message shape those callers expect.
- Operator MCP child env now pins the RESOLVED instance root
(PROTOAGENT_HOME) + forwards PROTOAGENT_BOX_ROOT. The ACP agent spawns
the MCP server with a replaced env, so a PROTOAGENT_HOME-scoped
instance's child booted the DEFAULT stores — found live when a smoke
agent's task_create landed on the operator's prod board.
- ACP turns are serialized per thread (_thread_lock) in both the streaming
and non-streaming switches — AcpClient forbids concurrent prompts on one
session; the acquire/release refcount only guarded eviction.
- ACP-native usage_update ({used, size} context pressure — hermes-acp
sends it; live: 24891/256000) is recorded by AcpClient and surfaced on
the usage frame as context_used_tokens/context_window_tokens. Tokens and
cost stay 0 — estimates must not pollute cost telemetry.
- Frozen desktop sidecar: operator MCP now launches via a new
`operator-mcp` dispatch verb (sys.executable rejects `-m
server.operator_mcp` argv there — #1603's class).
- `protoagent runtime use` hints `down && up` when a live server still
runs the previous runtime.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
WalkthroughThis PR adds context-usage tracking through AcpClient and AcpRuntime, surfaces it as usage frames in server/chat.py's streaming and new non-streaming ACP paths, wraps ACP streaming in per-thread locking, propagates additional environment variables and frozen-dispatch behavior in operator_mcp_server_spec, adds a running-server check with restart hints to runtime/cli.py, and forwards a new operator-mcp verb in server/cli.py. ChangesACP Usage Tracking and Chat Integration
Operator MCP Environment Propagation and CLI Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client as Chat Client
participant Chat as server/chat.py
participant Runtime as AcpRuntime
participant AcpClient as AcpClient
Client->>Chat: chat request (acp runtime)
Chat->>Runtime: acquire runtime + _acp_drive_turn
Runtime->>AcpClient: run_turn / stream updates
AcpClient-->>Runtime: usage_update (used, size)
Runtime-->>Chat: last_usage()
Chat->>Chat: build usage frame (context_used_tokens, context_window_tokens)
Chat-->>Client: usage frame + done frame (or collected assistant message)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #1891 | fix(runtime): harden the ACP/Hermes runtime path — /v1 switch, instance pinning, serialization, context usage
VERDICT: WARN (pending CI — all 8 checks still settling; auto-promote to APPROVE on terminal-green)
CI Status
- All 8 checks: queued / in_progress
- Author reports local pass: ruff ✓, import contracts 3/3 ✓, 3415 tests passed
- Live smoke (isolated
acp:hermeson :7897): all 7 checks including/v1/chat/completions,task_createstore isolation, A2A, multi-turn — PASSED
Diff Review (8 files, +299/-15)
server/chat.py: New_acp_turn_collectedmirrors the streaming ACP switch for non-streaming/v1and/api/chatcallers — drives the turn under_thread_lock, collects frames, returns OpenAI-shapeusage. The streaming path's usage frame now surfacescontext_used_tokens/context_window_tokenswhen the agent reports ACP-nativeusage_update(hermes-acp does). Tokens/cost stay 0 — no pollution of cost telemetry.runtime/acp_runtime.py:operator_mcp_server_specnow forwardsPROTOAGENT_HOME+PROTOAGENT_BOX_ROOTto the child MCP, fixing a live-found instance-leak where a smoke agent'stask_createlanded on the operator's prod board. Frozen sidecar dispatch usesoperator-mcpverb instead of-m server.operator_mcpargv.last_usage()accessor added.plugins/coding_agent/acp_client.py:_handle_updatenow capturesusage_updateintoself.last_usagedict with try/except guard.runtime/cli.py:_cmd_usedetects a live server on the old runtime and tells the user toprotoagent down && protoagent up— good UX.
Observations
- CLAWPATCH/MEDIUM:
runtime/acp_runtime.py:44-45— bareacp:config (empty agent after split) falls through as a valid ACP runtime with an empty agent string. Pre-existing, not introduced here; worth a follow-up validation guard. - CLAWPATCH/LOW:
server/cli.py:107-112— pidfile reader catchesOSError/ValueErrorbut notUnicodeDecodeErroron corrupt pidfiles. Pre-existing. - CLAWPATCH/LOW:
server/cli.py:115-120—_pid_alivemisreports running processes as dead onPermissionError. Pre-existing. - CLAWPATCH/LOW:
runtime/acp_runtime.py:243,252-260— aux ACP clients in_AUX_CLIENTSare never explicitly closed at shutdown. Pre-existing leak. - No blocking defects identified in the diff itself. All four core fixes are well-scoped, defensively coded, and validated against a live instance.
The approve-on-green policy will promote this to APPROVED automatically once every check reaches terminal-green — no further re-review needed on a clean CI pass.
— Quinn, QA Engineer
|
Submitted COMMENT review on #1891. |
|
✅ CI went terminal-green with no blockers on the prior review — promoting it to APPROVED per the approve-on-green policy (#748). |
There was a problem hiding this comment.
CI terminal-green, no blockers on prior review — auto-approving on green (#748).
|
🔀 Approved and terminal-green, but GitHub's auto-merge never completed (phantom BLOCKED — ws-5sc); the reconciliation sweep completed the squash merge. |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head a890a3079285 · shadow — comment-only
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
This PR carries low overall risk. The single confirmed finding — an unhandled AttributeError in _running_server_port when a pidfile contains valid JSON that isn't a dict — is a minor edge-case correctness gap. It's the one to fix first (and only). The panel had no disagreements; the two findings the structural pass (protopatch) flagged about workspace test coverage and the acp: edge case were both verified as genuine defects but dropped here because neither file is touched by PR #1891 — they're pre-existing issues, not regressions introduced by this change. Nothing in the diff raises blocking concerns.
Gap: The structural pass surfaced two real pre-existing defects outside the diff (untested workspace lifecycle, opaque acp: error message). They merit their own tracking tickets but are out of scope for this PR review.
[
{
"file": "runtime/cli.py",
"line": 275,
"severity": "minor",
"category": "correctness",
"claim": "`_running_server_port` catches (OSError, ValueError, TypeError) but not AttributeError — if the pid file contains valid JSON that is not a dict (e.g. a bare array `[]`), `rec.get(\"port\")` raises unhandled AttributeError instead of cleanly returning None, crashing `_cmd_use`.",
"evidence": "The except clause on line 275: `except (OSError, ValueError, TypeError):` — but `json.loads` of `[]` returns a Python list, and calling `.get(\"port\")` on a list raises `AttributeError`, which is not a subclass of any of the three caught types.",
"verdict": "confirmed",
"note": "Read runtime/cli.py `_running_server_port()` from the PR head: try-block calls `rec.get(\"port\")` after `json.loads(...)`. If pidfile contains `[]`, `json.loads` returns a list; `[].get(\"port\")` → AttributeError, not caught by `except (OSError, ValueError, TypeError)`. Genuine gap, though low-probability (pidfile is server-authored as a dict)."
}
]![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@runtime/cli.py`:
- Around line 274-278: The `server.pid` parsing in the `runtime use` path
assumes `json.loads` returns a dict, so a non-dict JSON value can make
`rec.get("port")` raise an uncaught `AttributeError`. Update the logic in the
`runtime/cli.py` code path that reads `instance_paths().instance_root /
"server.pid"` to validate that `rec` is a dict before accessing `"port"`, and
treat any non-dict or malformed content the same as the existing failure cases
by returning `None`. Keep the fix localized near the `json.loads` / `port`
extraction block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7876d4e2-a8fa-478b-b81a-bbc31e9609ea
📒 Files selected for processing (8)
plugins/coding_agent/acp_client.pyruntime/acp_runtime.pyruntime/cli.pyserver/chat.pyserver/cli.pytests/test_acp_runtime.pytests/test_cli.pytests/test_runtime_cli.py
| try: | ||
| rec = json.loads((instance_paths().instance_root / "server.pid").read_text(encoding="utf-8")) | ||
| port = int(rec.get("port") or 0) | ||
| except (OSError, ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against non-dict server.pid content.
rec.get("port") assumes rec is a dict. If the pidfile ever contains a non-dict JSON value (e.g. truncated/corrupted during a concurrent write by protoagent up), .get raises AttributeError, which isn't in the caught exception tuple, crashing runtime use instead of gracefully returning None.
🛡️ Proposed fix
- try:
- rec = json.loads((instance_paths().instance_root / "server.pid").read_text(encoding="utf-8"))
- port = int(rec.get("port") or 0)
- except (OSError, ValueError, TypeError):
+ try:
+ rec = json.loads((instance_paths().instance_root / "server.pid").read_text(encoding="utf-8"))
+ port = int(rec.get("port") or 0) if isinstance(rec, dict) else 0
+ except (OSError, ValueError, TypeError, AttributeError):
return None📝 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.
| try: | |
| rec = json.loads((instance_paths().instance_root / "server.pid").read_text(encoding="utf-8")) | |
| port = int(rec.get("port") or 0) | |
| except (OSError, ValueError, TypeError): | |
| return None | |
| try: | |
| rec = json.loads((instance_paths().instance_root / "server.pid").read_text(encoding="utf-8")) | |
| port = int(rec.get("port") or 0) if isinstance(rec, dict) else 0 | |
| except (OSError, ValueError, TypeError, AttributeError): | |
| return None |
🤖 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 `@runtime/cli.py` around lines 274 - 278, The `server.pid` parsing in the
`runtime use` path assumes `json.loads` returns a dict, so a non-dict JSON value
can make `rec.get("port")` raise an uncaught `AttributeError`. Update the logic
in the `runtime/cli.py` code path that reads `instance_paths().instance_root /
"server.pid"` to validate that `rec` is a dict before accessing `"port"`, and
treat any non-dict or malformed content the same as the existing failure cases
by returning `None`. Keep the fix localized near the `json.loads` / `port`
extraction block.
….96.0 cut (#1895) Nine PRs merged since v0.95.1 shipped without a changelog entry, so the generated release notes (updater + Discord, single-source from CHANGELOG) would have omitted a headline feature and a security-relevant default change. Backfill before rolling the release: - Added: Hermes first-class runtime (#1889 + hardening #1891) - Changed: filesystem.allow_run defaults OFF on the headless tier (#1888) - Fixed: fleet members no longer inherit hub identity (#1886/#1882/#1881), whole trace on a dedicated Langfuse project (#1893), background knowledge ingest wakes the agent (#1887), quieter plugin hot-reload logs (#1885) The already-present entries (#1880 tracing, #1890 plugin-view 401, #1879 subagent max_turns) are unchanged. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Hardening batch on top of #1889 (
protoagent hermespreset), all four fixes validated against a liveacp:hermesserver (isolated instance, gateway-less — Hermes as the only brain).Fixes
agent_runtime— the OpenAI-compat/v1endpoint, desktop/api/chatfallback, and internal self-prompts silently ran the native loop under anacp:*config. A gateway-less ACP-only setup (exactly the Hermes-preset persona) can't serve that path at all. New_acp_turn_collectedmirrors the streaming switch and folds frames into the single-message shape (+ OpenAI-shapeusage). Live:/v1/chat/completionsanswered on an instance withapi_key_configured: false.PROTOAGENT_INSTANCEwas forwarded, so aPROTOAGENT_HOME-scoped instance's child booted the default stores. Found live: the smoke agent'stask_createlanded on the operator's prod board. Now carries resolvedPROTOAGENT_HOME+PROTOAGENT_BOX_ROOT. (Leaked test row removed; store integrity verified.)AcpClientforbids concurrent prompts on one session;_acp_acquirerefcount only guards eviction. Both switches now hold the same_thread_lockthe native turns use (also excludes compact/rewind races).usage_update({used, size}; hermes-acp sends one per response — live:24891/256000) is recorded and surfaced on the usage frame ascontext_used_tokens/context_window_tokens. Tokens/cost stay 0: estimates must not pollute cost telemetry. Console can render a context ring off this (follow-up).operator-mcpdispatch verb; the frozen entrypoint rejects-m server.operator_mcpargv (fix(fleet): frozen-sidecar member spawn + surface boot failures #1603's class). Source checkouts unchanged.runtime useprintsprotoagent down && protoagent upwhen a live server still runs the old runtime.Deep smoke (live, isolated instance on :7897)
scripts/live_smoke.pyacp:hermes, headless setup (ACP-only, no gateway key)/api/chatnon-streaming turntask_create→ instance store round-tripSendStreamingMessageTASK_STATE_COMPLETEDmodel=acp:hermes, 0 cost,tool_calls=1/v1/chat/completionsusage_updatecapture{used: 24891, size: 256000}Gates: ruff ✓, import contracts 3/3 ✓, full suite 3415 passed.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
operator-mcpcommand in the CLI.Bug Fixes
Refactor