Phase 10.2A: restrict worker lifecycle authority on v0.20 - #1
Conversation
yshen92
left a comment
There was a problem hiding this comment.
Multi-pass review — Phase 10.2A restricted worker lifecycle
Reviewed at head ba3c3278 against base 01edcadb (full diff, all 17 files) in three passes: core-runtime review, supporting-files review, and an adversarial verification pass that attempted to refute each candidate finding (the two behavioral findings below were reproduced with executable repros against this tree, not just traced).
What checked out clean:
- The claim-binding CAS is genuinely atomic:
_validate_restricted_bindingis advisory and every predicate is re-checked inside thecomplete_task/block_taskUPDATEWHEREclauses underwrite_txn. No TOCTOU. SCM_CREDENTIALSbinding is sound against sibling/grandchild forgery (kernel stamps sender PID/UID at send time; socketpair is inheritance-only), and multiple/oversized/foreign datagrams all fail closed.- Heartbeat-less liveness works:
release_stale_claimsextends live-PID claims (hbNonenever trips the stale backstop),detect_stale_runningskips live bound workers, andenforce_max_runtime+ mandatory positivemax_runtime_secondsremain the hard bound (dispatcher-restart story covered by tests). - The claim-expiry refusal race is intentional and test-covered (
test_claim_expiry_is_rechecked_inside_finalizer_cas) — considered and discarded as a non-finding. - Default-off wiring is provable: the env var is only ever set under
restricted_worker_config()gating; default config path cannot set it. Supporting files (CIbase_shafix, attribution script/mapping, prompt guidance, config version bump) are all consistent and injection-safe; docs make no claim the code doesn't back.
Findings (4): one medium (dispatcher tick abort on worker-declared artifacts), one medium multi-board scoping issue, and two minor — see inline comments. None are activation blockers given the feature is default-off and externally gated on the Phase 10.1 launcher, but the two medium ones are worth fixing before 10.2B builds on this channel.
CI at this head is green (the earlier Review label gate failure was the pre-label run; the label rerun passed).
| "restricted lifecycle goal judge failed open: %s", exc, | ||
| exc_info=True, | ||
| ) | ||
| ok = complete_task( |
There was a problem hiding this comment.
[medium] Worker-declared artifacts can abort the whole dispatch tick via uncaught ArtifactPreservationError.
complete_task here runs with worker-supplied metadata["artifacts"] (merged in _normalize_restricted_result). For a scratch-workspace task, _persist_scratch_completion_artifacts raises ArtifactPreservationError when a declared path resolves inside the workspace but is missing, a directory, or oversized — and nothing between this call and the top of _dispatch_once_locked catches it. The direct-worker path has a dedicated except kb.ArtifactPreservationError in tools/kanban_tools.py (~L795) that returns a tool error the live worker can correct; this new dispatcher-side route into the same raise has no equivalent guard.
Reproduced end-to-end on this tree: tick 1 — dispatch_once raises (gateway's broad except logs "tick failed" and the board skips reclaim/spawn/crash accounting for the tick; binding not popped, channel not closed, datagram already consumed); tick 2 — refusal lands as the misleading "lifecycle channel unavailable: BlockingIOError", then protocol-violation accounting reruns the task and the worker's handoff is permanently lost. The trigger is routine, not adversarial — an LLM naming a deliverable it never wrote is common (it's exactly what the tool-layer guard exists for), and there is no test covering a missing/directory/oversized artifact in restricted mode.
Suggested fix: catch ArtifactPreservationError (or Exception) around the finalizer call in _apply_restricted_worker_result and convert it into a normal (False, reason) refusal so it flows into the existing lifecycle_result_refused event path.
There was a problem hiding this comment.
Addressed in fe08544. The restricted completion path now catches only ArtifactPreservationError around the canonical complete_task call and returns a normal refusal. The regression drives an authenticated clean-exit result through process_restricted_worker_results and verifies the task remains running, scratch workspace survives, the binding is cleaned up, and lifecycle_result_refused records the missing artifact.
| return (True, None) if ok else (False, "claim-bound lifecycle CAS refused") | ||
|
|
||
|
|
||
| def process_restricted_worker_results(conn: sqlite3.Connection) -> list[str]: |
There was a problem hiding this comment.
[medium] _restricted_worker_bindings is process-global but processed per-board connection — cross-board result destruction in multi-board gateways.
process_restricted_worker_results(conn) iterates every binding in the process, but conn belongs to whichever board is ticking (gateway/kanban_watchers.py runs _tick_once_for_board per board in one process, each with its own connection). When board A ticks first after board B's restricted worker exits cleanly, A's tick consumes B's datagram, validates binding.task_id against A's DB, fails with "task no longer exists", records the refusal event against the wrong board, pops the binding, and closes the channel — B's completed work is permanently lost and later surfaces as a protocol violation on B.
Single-board deployments are unaffected. Suggested fix: record the board/DB identity in RestrictedWorkerBinding at spawn time and have process_restricted_worker_results skip bindings that don't belong to the ticking board's DB path.
There was a problem hiding this comment.
Addressed in fe08544. RestrictedWorkerBinding now captures the canonical board DB path at spawn, and process_restricted_worker_results derives the ticking connection's main DB path and skips nonmatching bindings before reading or closing their channels. The two-board regression proves board A leaves board B's result intact and board B then finalizes it.
| "summary": summary, | ||
| "result": result, | ||
| "metadata": metadata, | ||
| "created_cards": created_cards or [], |
There was a problem hiding this comment.
[minor] created_cards is forwarded here but the dispatcher unconditionally refuses it — pre-reject it like board.
_normalize_restricted_result refuses any non-empty created_cards ("restricted workers cannot claim created cards"), but only after the worker has received ok: true / status: "pending_dispatcher" and exited — at which point the entire completion is discarded and the task ends refused → protocol-violated → rerun (verified by repro). board is already pre-rejected a few lines up in this same branch; created_cards deserves the same synchronous rejection so the model gets an immediate tool_error it can correct in-loop. Likelihood is low (kanban_create is hidden in restricted mode so there are no real ids to pass), but KANBAN_COMPLETE_SCHEMA's description still invites populating the field, and the cost when it fires is a lost run.
There was a problem hiding this comment.
Addressed in fe08544. Restricted kanban_complete now rejects non-empty created_cards synchronously before emit_result, with a regression asserting no lifecycle handoff is emitted.
| ) | ||
| from agent.prompt_builder import KANBAN_GUIDANCE, RESTRICTED_KANBAN_GUIDANCE | ||
| if ( | ||
| os.environ.get("HERMES_KANBAN_RESTRICTED_WORKER") |
There was a problem hiding this comment.
[nit] Raw env truthiness diverges from is_restricted_worker().
This gate (and the agent_init-bypass fallback in agent/system_prompt.py) treats any non-empty HERMES_KANBAN_RESTRICTED_WORKER as restricted — including "0"/"false"/"off" — while kanban_lifecycle.is_restricted_worker() parses only {"1","true","yes","on"}, so those values yield the restricted system prompt while the tool/DB layers run in normal direct mode. Nothing shipped hits this (the dispatcher only sets "1") and the divergence fails safe (guidance stricter than reality), so purely a consistency nit: call is_restricted_worker() here instead. The raw-truthiness ImportError fallbacks elsewhere are correct as written — they intentionally fail closed.
There was a problem hiding this comment.
Addressed in fe08544. Both agent initialization and the system-prompt fallback now use kanban_lifecycle.is_restricted_worker(), while the ImportError fallback remains fail-closed. False-like values (false/0/off/no) are covered by regression tests.
Round 2 — 4 resolved, 0 still present, 0 newRe-reviewed the delta
No new issues found in the delta. The provenance doc correctly advances the runtime candidate to Verification: focused suite run locally at Verdict: LGTM — all blocking findings resolved; nothing outstanding from my review. (Own-PR review, so this comment stands in for approval.) |
Two independent bugs let a deleted profile reappear / leave orphaned resources on next launch: 1. hermes_cli/profiles.py's backend-process scanner required argv[0] to resolve to an executable literally named "hermes". Electron's pool-backend spawn resolves the hermes console-script shim's path and execs it via the interpreter directly (python3 /path/to/hermes ...), so argv[0] reports as "python3" and the scanner never matched the running backend -- delete removed the profile's files but left its live backend process running (still bound to a port via uvicorn), which accumulates across repeated delete/recreate cycles. 2. The desktop sidebar's ProfileRail only refreshed its cached profile list once, on mount, so a delete/create/rename from another surface (another window, or the CLI) left a stale ghost entry until something unrelated triggered a refetch. Note: a delete via this window's own Manage-Profiles view already refreshes the shared $profiles atom ProfileRail subscribes to (confirmed by reading refreshProfiles() and handleConfirmDelete()) -- this fix only covers the cross-window/cross- process staleness gap, not a duplicate of the already-merged NousResearch#57329's Manage-Profiles rail-refresh work. Fix 1: recognize a python-interpreter argv[0] exec'ing a hermes-named console-script shim via argv[1]. Fix 2: refresh the profile list on window focus/visibilitychange, matching the existing pattern used elsewhere in the sidebar (sidebar/index.tsx, use-background-sync.ts, star-map.tsx, use-gateway-boot.ts all use the same focus+visibilitychange pattern). ## Related work already on main PR NousResearch#57329 (merged) fixed the *headline* symptom from issue NousResearch#52279 (deleted profile respawns) via a different, non-overlapping mechanism: routing profile-delete through the primary backend instead of spawning a fresh pool backend, plus a separate recreation guard in ensure_hermes_home() (NousResearch#49435, merged) that makes a backend spawned into a deleted profile's directory raise FileNotFoundError instead of silently recreating it. This PR is NOT a duplicate of that fix. Verified: even with both of those merged, a backend process that survives because of gap #1 above still holds a bound port via uvicorn -- it just can no longer resurrect the profile directory. That's real resource-hygiene, not a symptom already covered. Gap #2 touches a different file/component (ProfileRail / profile-switcher.tsx) than NousResearch#57329's rail-refresh half (which touched the Manage-Profiles view's own $profiles.ts / index.tsx) and covers a distinct staleness path (cross-window/cross-process, not same-window delete-then-refresh). Tests: tests/hermes_cli/test_profiles.py -- 156 passed (existing + regression coverage for the argv[0] python-interpreter detection case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Phase 10.2A-local closes the H3 runtime handoff gap on the exact v0.20 line: a native Kanban worker can report only its own bounded complete/block result to the trusted dispatcher without receiving writable board-DB or graph-mutation authority.
The worker sends one identity-free, size-bounded Unix datagram. Linux
SCM_CREDENTIALSand dispatcher-owned spawn state bind it to the exact PID/UID/task/run/profile/workspace/board/claim. The dispatcher revalidates current DB state atomically, then calls the existing canonicalcomplete_task/block_taskpaths so artifacts, redaction, hooks, cleanup, events/runs, ready recomputation, typed blocking, and failure accounting remain unchanged. Liveness stays dispatcher/PID-owned; restricted workers do not write DB heartbeats.Restricted mode is default-off under
kanban.restricted_workersinconfig.yaml. Tool and DB guards are defense in depth. Activation remains blocked until the separately reviewed Phase 10.1 launcher drops UID before Python, execs with stable PID, kills children on parent death, denies DB/WAL/SHM access, and passes real restricted-UID raw-SQL/CLI/import preflight.Exact provenance
3c27eb6234bf91b8ceee9e9071591b31e9b148cb(v2026.8.3, package0.20.0)01edcadbd194f81bd7eceb9ca267737830ce24c0yshen92/hermes-agent:refs/heads/r0-evidence/v2026.8.3-01edcadfe085446b864cb7d31f57499e407ce0f7275c511e07ef7de52ca31b065b7f1545582ee7a5f66e341The R0 object was fetched and verified directly. It was not reconstructed, amended, squashed, or rebased, and its evidence ref was not modified.
Reuse disposition
See
REUSE_DISPOSITION.md,UPSTREAM_EXTRACTION_MAP.md,ADVERSARIAL_REVIEW.md,MAINTENANCE_DISPOSITION.md, andRUNTIME_PROVENANCE.md.Verification
93 passed, 1 skipped(Linux-only SCM credential integration skipped on macOS)git diff --check: passedThe review follow-up additionally binds each control channel to its board DB, turns artifact-preservation errors into dispatcher refusal events without losing scratch state, rejects restricted
created_cardsbefore handoff, and unifies false-like restricted-mode prompt parsing.Boundaries