From a9bdf0c5ea9af9de7a5869725801a67d030da956 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 10 Aug 2026 16:21:22 +0000 Subject: [PATCH 1/4] spec(role): the operator lock becomes a record of who coordinates where (#285) Spike/spec before code for issue #285, user-directed: the repo-wide operator lock must stop refusing a second coordinator and become a RECORD of who is working where. The exclusivity's stated premise -- "the shared case is the operator's primary checkout, where one role is the correct answer anyway" -- no longer holds: AGENTS.md now requires every unit of work to take its own worktree and to land from a task branch, so there is no shared checkout to protect. An operator is a coordinator whose maximum powers are merging PRs and dispatching sub-agents into worktrees; it never force-pushes main, so git's non-fast-forward refusal is the real interlock. The spec fixes the representation before any code: one record file per worktree under /vllm-cpp-operators/, published by temp+os.replace, so two concurrent claimants write two different paths and neither can lose the other's record. The 2h TTL and stale pruning stay; a stale record is pruned from the display and can no longer refuse anybody. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [Claude Code] --- .agents/roadmap_v1.md | 1 + .agents/specs/operator-record.md | 158 +++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 .agents/specs/operator-record.md diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index a3c613a75..ce232c5c4 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -37,6 +37,7 @@ issue is not yet placed. Keyed record: update in place, never append. | Issue | Row | Title | Kind | |---:|---|---|---| | [#287](https://github.com/mudler/vllm.cpp/issues/287) | `KV-MOONCAKE-STORE` | `MooncakeStoreConnector`: the KV store half is linkable native C++ and gateable over TCP on one box | feature | +| [#285](https://github.com/mudler/vllm.cpp/issues/285) | — | The operator lock refuses a second coordinator; it should only RECORD who is working where (spec `specs/operator-record.md`) | bug | | [#241](https://github.com/mudler/vllm.cpp/issues/241) | `ROAD-V1-H3` | MiniMax-H3: support the PRUNED (AdaLN timestep-curve) checkpoint variants | feature | | [#250](https://github.com/mudler/vllm.cpp/issues/250) | — | `a5b52047` reached main without a task branch, and `check-role-discipline` cannot be waived | bug | | [#243](https://github.com/mudler/vllm.cpp/issues/243) | — | `vllm-feature-gap-analysis.md` is a stale 2026-07-28 snapshot: 9 of 16 HIGH/MED gaps have since landed | bug | diff --git a/.agents/specs/operator-record.md b/.agents/specs/operator-record.md new file mode 100644 index 000000000..5bbe22905 --- /dev/null +++ b/.agents/specs/operator-record.md @@ -0,0 +1,158 @@ +# The operator lock becomes a RECORD of who is coordinating where + +User-directed 2026-08-10, issue +[#285](https://github.com/mudler/vllm.cpp/issues/285). Row `ENG-OPERATOR-RECORD` +(tooling and policy; it owns no claim-matrix row and no product code). + +The developer's words: *"I dont want push force main, never. the operator have +at max the way to merge directly PRs and heavily dispatch sub-agents with +separate worktrees ( == coordinator )"*, and, on the lock itself: *"let's keep +it as a record for who is working where"*. + +## Scope + +`scripts/agent-role.py` stops refusing a second operator. The file in the git +common dir stays, and stops being a lock: it becomes a set of records, one per +worktree, that says who is coordinating where. `show` reports the other live +coordinators. `release` removes only the caller's record. The 2-hour TTL and +stale pruning stay exactly as they are. + +Out of scope: the helper role, the role marker, the `read-only` answer, +`--headless`, `check-role-discipline.py`'s row/PR rule, and anything about how +work lands. Nothing here weakens a gate: no refusal is added, and the one +refusal removed is the subject of the issue. + +## Our baseline — why the exclusivity no longer holds + +`scripts/agent-role.py:28-37` justifies a repo-wide exclusive lock with *"the +shared case is the operator's primary checkout, where one role is the correct +answer anyway."* That premise is gone. `AGENTS.md` § "Work happens in a +worktree" now requires **every** unit of work to take its own linked worktree, +and § "Landing work" requires everything to reach `main` from a task branch. So +there is no shared checkout to protect and no unsynchronised writer to exclude. + +What an operator actually is, per the developer: a **coordinator**. Its maximum +powers are merging PRs directly and dispatching sub-agents into separate +worktrees. It never rewrites shared history — `main` is never force-pushed, with +no `--force` and no `--force-with-lease` — so a plain `git push` refuses any +non-fast-forward and **git itself is the interlock**. Two coordinators racing to +land serialise on that refusal: the loser fetches, re-merges, re-gates and +pushes again. A JSON file in `.git/` never provided that guarantee and cannot. + +What the exclusivity costs is measured, not hypothetical. +`LOCK_TTL_SECONDS = 2 * 60 * 60`, so a session killed mid-flight — one was on +2026-08-10, by a host disk cleanup, leaving a dead pid and a frozen heartbeat — +blocks **all** coordination for up to two hours, and the only remedy is +hand-deleting a file inside `.git/`. A claim refused at 78 minutes is the TTL +working correctly; that it was refused at all is the defect. + +## Design + +**Representation.** One file per worktree in a directory, +`/vllm-cpp-operators/.json`, instead of +one shared `vllm-cpp-operator.lock`. Still in the git common dir, so it is +shared by every worktree and can never be committed. + +That shape is what makes concurrency safe: **a writer only ever touches its own +record**, because the filename is derived from the identity ownership already +keys on. Two coordinators claiming at the same instant write two different +paths, so neither can lose the other's record — there is no read-modify-write of +a shared file anywhere in the design. Each write is `write temp + os.replace` +inside the same directory, which is atomic on POSIX, so a reader sees the old +record or the new one and never a half-written one. A single JSON array or a +JSONL log would both have required rewriting a shared file to release or prune, +which is exactly where a concurrent writer's record gets dropped. + +`O_CREAT|O_EXCL` goes: it existed to make the second claimant fail, and the +second claimant must now succeed. + +**Ownership still keys on the worktree** (`git rev-parse --absolute-git-dir`), +unchanged from the 2026-08-06 correction, and `record_is_ours` keeps the legacy +session fallback for a record written before that correction. + +**Staleness.** `RECORD_TTL_SECONDS` keeps the 2-hour value and the heartbeat +semantics. A stale record is filtered out of every display and unlinked on the +next `claim`, which is a write path; `show` and `resolve` never unlink, because +`agent-preflight.sh` documents itself as never writing anything. A stale record +can no longer refuse anybody, so breaking one is no longer an event: the NOTE +that announced it goes with the refusal it explained. + +**Migration.** A pre-#285 `vllm-cpp-operator.lock` is read as one more record, +so a session that claimed operator before this change still resolves as operator +instead of silently becoming UNDECLARED mid-flight. Its next `claim` or +`release` removes it, which heals the repo the first time either runs. + +**The front doors stop blocking.** `scripts/agent-onboard.py` and +`scripts/agent-start.py` told a session that `claim operator` would fail and +instructed it not to run "a known-failing claim". That claim no longer fails, so +`blocked_by_other_operator` is replaced by `operator_peers` — the live records +that are not this worktree's — and both surfaces report them as information +beside the ordinary claim command. + +## Port map + +None. This is repository tooling with no upstream vLLM counterpart; the porting +inventory's §9 (written from scratch) is where role machinery has always sat. + +## Upstream chain + +Not applicable — vLLM has no agent-role protocol. The authority for this change +is the developer's direction in issue #285. + +## Tests to port + +None to port. `tests/scripts/test_agent_role.py` grows the new behaviour, and +the tests that pinned the removed refusal are rewritten to pin its replacement +rather than deleted: + +| Was | Becomes | +|---|---| +| `test_second_operator_is_refused` | `test_a_second_coordinator_is_recorded_not_refused` | +| `test_one_operator_per_repo_holds_across_worktrees` | `test_show_lists_the_other_live_coordinators` | +| `test_stale_lock_is_broken_but_reported` | `test_a_stale_record_is_pruned_and_never_blocks` | +| `test_a_legacy_lock_cannot_produce_two_operators` | `test_a_legacy_lock_file_is_adopted_as_this_worktrees_record` | +| `test_an_operator_marker_beaten_to_the_lock_reports_the_lockout` | `test_an_operator_whose_record_vanished_is_told_to_re_claim` | + +New: concurrent claims from many worktrees lose no record; `release` removes +only the caller's; `show` reports worktree, session, host and heartbeat age for +each peer. + +## Gates + +- `python3 tests/scripts/test_agent_role.py` (focused, RED first) +- `python3 tests/scripts/test_agent_onboard.py`, `test_agent_start.py` +- `scripts/agent-preflight.sh --quiet` green before and after the commit +- Mutation: every guard the new tests claim to pin is deleted or inverted, the + focused suite is shown RED, and the guard is restored and shown GREEN. + +## Dependencies + +None. Python, docs and tests only; no GPU, no build, no network. + +## Work breakdown + +Single unit — the tool, its suite, the two front doors that consumed the +refusal, and the documents that asserted it. Splitting it would leave the repo +in a state where the tool permits a second coordinator and the docs still forbid +one. + +## Risks/decisions + +- **Risk: a directory of files is harder to inspect than one file.** Accepted; + `show` renders it, which is the point of keeping records at all. +- **Risk: nothing now prevents two coordinators merging to `main` at once.** + That is the decision, not an oversight: git's non-fast-forward refusal is the + real interlock and the lock never was one. It holds only while `main` is never + force-pushed, which `AGENTS.md` now states as a rule. +- **Decision: keep the TTL.** It is correct and already works. Staleness now + only prunes a display entry instead of gating a claim. +- **Decision: no `--force` variant is ever added to any script here.** + +## Outcome + +Landed 2026-08-10 on `row/ENG-OPERATOR-RECORD`. `claim operator` records and +succeeds alongside live peers; `show` lists every other live coordinator with +worktree, session, host and heartbeat age; `release` removes only the caller's +record; a stale record is pruned from the display and blocks nothing. The +refusal path and its `already held` message are gone from the tool, the two +front doors, and the documents. From 1c8313aafecd21adcba3ddc4dcfa09b4e4a8bbf3 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 10 Aug 2026 16:36:58 +0000 Subject: [PATCH 2/4] feat(role): the operator lock becomes a record of who coordinates where (#285) User-directed, issue #285: "let's keep it as a record for who is working where". `claim operator` no longer refuses a second coordinator -- it records this worktree and succeeds -- and `show` lists the other live coordinators instead of naming one owner. ## Why the exclusivity had to go `scripts/agent-role.py` justified a repo-wide exclusive lock with "the shared case is the operator's primary checkout, where one role is the correct answer anyway". AGENTS.md now requires every unit of work to take its own worktree and to reach main from a task branch, so there is no shared checkout to protect. An operator is a COORDINATOR: it merges reviewed PRs and dispatches sub-agents into worktrees, and it never force-pushes main, so a plain `git push` refuses any non-fast-forward and git itself is the interlock. Concurrent coordinators serialise on that refusal; a JSON file in `.git/` never could. What the lock did provide was two hours of blocked coordination whenever a session died mid-flight (LOCK_TTL_SECONDS = 2h), remediable only by hand-deleting a file. That happened on 2026-08-10. ## The representation, and why it is atomic One record per worktree, `/vllm-cpp-operators/.json`, instead of one shared file. A writer only ever touches the path derived from its OWN worktree -- the identity ownership already keys on -- so two claimants racing address two different paths and neither can lose the other's record. Each publish is `write temp + os.replace` in the same directory, atomic on POSIX, so a reader sees the old record or the new one and never half of one. There is no read-modify-write of a shared file anywhere, which a single JSON array or an append log would have needed to release or prune. `O_CREAT|O_EXCL` is gone: it existed to make the second claimant fail. Kept unchanged: ownership keys on the worktree; the 2h TTL and stale pruning. Staleness now only removes a record from the display -- pruning happens in `claim`, never in `show`, because agent-preflight.sh documents itself as never writing. A pre-#285 single-file lock is still read as one record, so a session that claimed before this change keeps resolving, and its next claim or release heals the file away. ## Front doors `blocked_by_other_operator` becomes `operator_peers`. agent-onboard.py and agent-start.py used to print "BLOCKED: the operator lock is held by another live worktree" and instruct the agent not to run "a known-failing claim". That claim now succeeds, so peers are reported as status beside the ordinary claim command. ## Docs AGENTS.md states plainly that the operator is a coordinator, that several may run concurrently, that its powers are merging PRs and dispatching sub-agents in worktrees, and that main is NEVER force-pushed -- a rejected push means fetch, re-merge, re-gate, push again. .agents/workflow.md, the session-onboarding spec ("One operator per repo"), the issue-native-tracking spec and the agent-start design doc carry superseded notes rather than silent deletions. ## Gates RED first: 17 failures + 1 error on the new suite before the tool changed. GREEN after: test_agent_role 54, test_agent_onboard 38, test_agent_start 20. `scripts/agent-preflight.sh --quiet` green before the change and after it; `--staged --quiet` green on this commit's staged tree. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [Claude Code] --- .agents/specs/issue-native-tracking.md | 7 +- .agents/specs/session-onboarding.md | 5 + .agents/workflow.md | 18 +- AGENTS.md | 27 +- docs/USAGE.md | 7 + ...026-08-08-agent-start-entrypoint-design.md | 10 +- scripts/agent-onboard.py | 23 +- scripts/agent-role.py | 393 +++++++++++----- scripts/agent-start.py | 39 +- tests/scripts/test_agent_onboard.py | 79 ++-- tests/scripts/test_agent_role.py | 441 ++++++++++++------ tests/scripts/test_agent_start.py | 53 ++- 12 files changed, 738 insertions(+), 364 deletions(-) diff --git a/.agents/specs/issue-native-tracking.md b/.agents/specs/issue-native-tracking.md index 890b0d4d1..255e669d6 100644 --- a/.agents/specs/issue-native-tracking.md +++ b/.agents/specs/issue-native-tracking.md @@ -153,9 +153,10 @@ of what the row is and why it matters, above the machine fields. 4. open the draft PR. Assignment is atomic and unmergeable, so the claim race disappears. The -operator/helper roles, the exclusive operator lock, and the "helper works in a -worktree and opens a draft PR at the start" rule are unchanged — only the -*medium* of the claim changes. +operator/helper roles, the coordinator record (an exclusive lock when this was +written; a record of who is coordinating where since issue #285), and the +"helper works in a worktree and opens a draft PR at the start" rule are +unchanged — only the *medium* of the claim changes. ### Read cache for offline work diff --git a/.agents/specs/session-onboarding.md b/.agents/specs/session-onboarding.md index f69148842..92135777b 100644 --- a/.agents/specs/session-onboarding.md +++ b/.agents/specs/session-onboarding.md @@ -183,6 +183,11 @@ Two properties are preserved unchanged: - **One operator per repo.** The operator lock stays in the git COMMON dir, so it is shared by every worktree and a second operator still fails. + **SUPERSEDED 2026-08-10 by issue #285** (`specs/operator-record.md`): the file + stays in the git COMMON dir and stays keyed on the worktree, but it is now a + RECORD of who is coordinating where and never a refusal. Several coordinators + may run at once; `main` is never force-pushed, so git's non-fast-forward + refusal is the interlock this lock was pretending to be. - **Helpers stay isolated.** A helper already materializes its own worktree. The cost is explicit: **two agent sessions sharing one checkout now share a diff --git a/.agents/workflow.md b/.agents/workflow.md index b58bea4ba..58f75da1d 100644 --- a/.agents/workflow.md +++ b/.agents/workflow.md @@ -1,14 +1,22 @@ # Task guide — coordinating parallel work -How the operator runs several rows at once without agents colliding. The rules +How a coordinator runs several rows at once without agents colliding. The rules are in [`AGENTS.md`](../AGENTS.md); this is the method. ## The shape of a campaign -The operator holds the plan, the GPU, and main. Everything else is delegated to -fresh agents with bounded briefs. The operator does not implement work that -should be independently reviewed — writing it and reviewing it in one context -defeats the review. +The operator is a coordinator: it holds the plan and the GPU, merges reviewed +PRs, and delegates everything else to fresh agents with bounded briefs in their +own worktrees. It does not implement work that should be independently reviewed +— writing it and reviewing it in one context defeats the review. + +**Several coordinators may run at once.** `scripts/agent-role.py claim operator` +records this worktree and never refuses; `scripts/agent-role.py show` lists the +other live coordinators — worktree, session, host, and time since their last +heartbeat — and prunes anything past the 2-hour TTL. What keeps concurrent +coordinators from colliding is not that file: `main` is never force-pushed, so a +plain `git push` refuses any non-fast-forward. When yours is rejected, fetch, +re-merge, re-run the gate, and push again. For each row: confirm the issue, commit the spec, dispatch an implementer, dispatch a *different* reviewer, return findings to a new implementer, rerun the diff --git a/AGENTS.md b/AGENTS.md index 5f5332687..ea3ed59df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,8 +15,10 @@ the reference for behavior and the bar for speed. what work is intended. Follow its printed action, then rerun it. 2. Declare a role: `scripts/agent-role.py claim operator` for a multi-step integration campaign, `claim helper --row ` for one scoped task, or - `claim read-only` for inspection. Add `--headless` only when the developer - explicitly says the run is unattended. Never infer it. + `claim read-only` for inspection. The operator claim records this worktree + as a coordinator; it is never refused because someone else is coordinating. + Add `--headless` only when the developer explicitly says the run is + unattended. Never infer it. 3. Read `.agents/NOW.md`. It is the live snapshot and fits on one screen. 4. Read only the claimed row, its spec, its evidence, and the task guide for what you are about to do. @@ -97,8 +99,17 @@ conditions. Missing binding context returns `NEEDS_CONTEXT` rather than a guess; a material disagreement returns `NEEDS_DECISION` rather than silent scope change. Use the versioned contracts in [`.agents/prompts/`](.agents/prompts/). -The operator coordinates, owns main integration and the GPU, and does not write -implementations that should be independently reviewed. +The operator is a **coordinator**. It holds the plan and the GPU, merges +reviewed PRs, dispatches sub-agents into separate worktrees, and does not write +implementations that should be independently reviewed. **Several operators may +run at once** — `scripts/agent-role.py claim operator` records who is +coordinating where and never refuses; `show` lists the others. + +**`main` is never force-pushed.** No `--force`, no `--force-with-lease`, by +anyone, ever. That is what makes concurrent coordinators safe: a plain +`git push` refuses any non-fast-forward, so git itself is the interlock. A +rejected push means fetch, re-merge, re-run the gate, and push again — never +force. ## vLLM is the reference @@ -227,9 +238,11 @@ keeps the repair-without-a-round-trip case one step, while still leaving every change on a branch that git can show, revert, and attribute. Run the applicable gate before every push and chain that success directly to the -exact-SHA push. Hooks are bypassable convenience, never proof. If the remote -cannot be queried, report `REMOTE_UNVERIFIED` — unknown is neither absence nor -success, and it authorizes no cleanup. +exact-SHA push. Never force-push, and never add a force variant to a script; a +rejected push is git protecting someone else's merge, so fetch, re-merge, +re-gate and push again. Hooks are bypassable convenience, never proof. If the +remote cannot be queried, report `REMOTE_UNVERIFIED` — unknown is neither +absence nor success, and it authorizes no cleanup. Verified PRs are merged in-session; obsolete ones are closed with the reason recorded. Never end a session with a verified, unmerged PR. diff --git a/docs/USAGE.md b/docs/USAGE.md index 0d78d159a..42d32ac25 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -86,6 +86,13 @@ welcome that the agent should relay. An explicit request can use claim action, rerun it after declaration, then run `scripts/agent-preflight.sh`. The entrypoint is non-interactive and does not mutate the checkout. +The operator role is a coordinator, and **several may run at once**: +`scripts/agent-role.py claim operator` records this worktree and is never +refused, `scripts/agent-role.py show` lists the other live coordinators, and +`scripts/agent-role.py release` removes only this worktree's record. What keeps +concurrent coordinators safe is that `main` is never force-pushed, so a plain +`git push` refuses any non-fast-forward. + ## Running inference (CLI) `vllm-cli` runs a one-shot completion through the C ABI. Source: diff --git a/docs/superpowers/specs/2026-08-08-agent-start-entrypoint-design.md b/docs/superpowers/specs/2026-08-08-agent-start-entrypoint-design.md index 6653d5661..65731e6df 100644 --- a/docs/superpowers/specs/2026-08-08-agent-start-entrypoint-design.md +++ b/docs/superpowers/specs/2026-08-08-agent-start-entrypoint-design.md @@ -123,6 +123,11 @@ is re-derived from materialized state, followed by preflight. An operator lock held by another worktree is reported as a blocker rather than converted into a different role. The agent reports it and obtains direction. +**SUPERSEDED 2026-08-10 by issue #285** (`.agents/specs/operator-record.md`): +there is no lock and no blocker. A live coordinator in another worktree is +reported as status — `other coordinators: N recorded (claim is allowed)` — and +`claim operator` is offered as normal, because it is never refused. + ### Undeclared worktree without explicit intent The entrypoint emits the selected compact-frame welcome. The source constant, @@ -182,7 +187,8 @@ duplicating the interview and points to the canonical entrypoint. - A missing helper row produces a specific next action rather than a malformed claim command. - A held operator lock reports the holder conflict and does not recommend an - unauthorized fallback. + unauthorized fallback. (SUPERSEDED by #285: recorded coordinators are + reported as status and refuse nothing.) - A declared-role/explicit-intent mismatch is visible and never mutates state. - Headless mode is propagated into the exact claim command only when explicitly supplied. @@ -217,7 +223,7 @@ cover: - the compact banner's ASCII-only and maximum-width guarantees; - stable welcome/action delimiters and exact role-claim commands; - inherited-role/explicit-intent conflicts; -- a live operator lock owned by another worktree; +- a live coordinator recorded by another worktree (a blocking lock until #285); - unavailable helper queue and unreadable/incomplete environment states; - absence of environment values and secrets in rendered output; - nonzero exits only for invalid input or an inability to route truthfully; diff --git a/scripts/agent-onboard.py b/scripts/agent-onboard.py index 87f3bfcaa..1d2749f0d 100755 --- a/scripts/agent-onboard.py +++ b/scripts/agent-onboard.py @@ -125,12 +125,14 @@ def probe() -> dict: return { "role": state.get("role"), "row": state.get("row"), - # resolve() distinguishes "never declared" from "the operator lock is - # held by another live session" and from "operator marker without a - # held lock; re-claim". Dropping those makes this front door LESS - # honest than the tool it wraps, and sends a session toward `claim - # operator` when that will fail. - "blocked_by_other_operator": bool(state.get("operator_held_by_other")), + # Who else is coordinating right now, straight from resolve(). This + # replaced `blocked_by_other_operator` when the operator lock became a + # RECORD (issue #285): a recorded peer never blocks a claim, so + # reporting it as a blocker sent sessions away from a command that + # succeeds. Dropping it instead would make this front door LESS honest + # than the tool it wraps -- "who is working where" is the whole reason + # the file is kept. + "operator_peers": state.get("operator_peers") or [], "reason": state.get("reason"), # The role tool owns both facts. Keep the branch from resolve() and # expose the same per-worktree identity used by role markers/locks so @@ -167,12 +169,11 @@ def render_probe(state: dict) -> str: + (f" (unset: {', '.join(state['env_missing'])})" if state["env_missing"] else ""), queue_line, ] + # Rendered whatever this session's role is: any session may need to know + # who else is coordinating. It is information and never an obstacle -- + # `claim operator` is never refused (issue #285). + lines.extend(role_mod.render_peers(state.get("operator_peers") or [])) if state["role"] is None: - if state.get("blocked_by_other_operator"): - lines.append( - "NOTE: the operator lock is held by another live session, so " - "`claim operator` will fail. Take helper or read-only." - ) lines.append( "This session has not declared a role. Ask what the work is, then claim: " "a long campaign -> operator; one scoped change -> helper --row ; " diff --git a/scripts/agent-role.py b/scripts/agent-role.py index 63e783b2f..a01c78221 100755 --- a/scripts/agent-role.py +++ b/scripts/agent-role.py @@ -25,19 +25,42 @@ gate teaches people to disable it. The accepted cost is explicit: two sessions sharing one checkout share a role. -Helpers take their own worktree by construction, so the shared case is the -operator's primary checkout, where one role is the correct answer anyway. See -.agents/specs/session-onboarding.md, "Correction: a role keys on the WORKTREE, -not the session". - -The operator lock lives in the git COMMON dir, not the working tree: it is -shared by every worktree of the repo (the correct scope for "one operator per -repo") and can never be committed by accident. Its ownership keys on the -worktree too, so the operator survives the same call boundary while a second -worktree is still refused. +Every unit of work takes its own worktree, so that case is rare and it is the +right trade. See .agents/specs/session-onboarding.md, "Correction: a role keys +on the WORKTREE, not the session". + +The coordinator RECORDS live in the git COMMON dir, not the working tree: they +are shared by every worktree of the repo (the right scope for "who is +coordinating where") and can never be committed by accident. + +They are a record and NEVER a refusal (user-directed, issue #285, +.agents/specs/operator-record.md). This file used to argue for one exclusive +operator per repo because "the shared case is the operator's primary checkout, +where one role is the correct answer anyway". That premise is gone: AGENTS.md +now requires EVERY unit of work to take its own worktree and to reach `main` +from a task branch, so there is no shared checkout to protect. + +What an operator is, is a COORDINATOR. Its powers are merging PRs and +dispatching sub-agents into separate worktrees; it never rewrites shared +history, and `main` is never force-pushed. A plain `git push` therefore refuses +any non-fast-forward, so git itself is the interlock and concurrent +coordinators serialise on it -- the loser fetches, re-merges, re-gates and +pushes again. A JSON file in `.git/` never provided that guarantee and could +not. What it did provide was two hours of blocked coordination every time a +session died mid-flight, with no remedy but hand-deleting a file. + +So the representation is one record per worktree, +`/vllm-cpp-operators/.json`, rather than +one shared file. A writer only ever touches the path derived from its OWN +worktree -- the identity ownership already keys on -- so two claimants racing +write two different paths and neither can lose the other's record. Each publish +is `write temp + os.replace` in the same directory, which is atomic on POSIX, so +a reader sees the old record or the new one, never half of one. There is no +read-modify-write of a shared file anywhere here, which is exactly what a single +JSON array or an append log would have needed to release or prune. scripts/agent-role.py show # resolve; exit 3 if undeclared - scripts/agent-role.py claim operator + scripts/agent-role.py claim operator # records; never refused scripts/agent-role.py claim helper --row ENG-FOO scripts/agent-role.py claim read-only # declares no claim at all scripts/agent-role.py claim helper --row ENG-FOO --headless @@ -48,6 +71,7 @@ from __future__ import annotations import argparse +import hashlib import json import os import subprocess @@ -56,9 +80,9 @@ from pathlib import Path -# read-only is a declared ABSENCE of claim, not a third role: it takes no lock +# read-only is a declared ABSENCE of claim, not a third role: it records nothing # and creates no worktree. Without it, a session that only reads must either -# take the repo-wide operator lock or create a throwaway worktree, and faced +# record itself as a coordinator or create a throwaway worktree, and faced # with that people reach for --no-require-role until the gate means nothing. # # CLAIMABLE_ROLES is the vocabulary a "may this session write?" test SHOULD key @@ -77,9 +101,23 @@ def mode_from_marker(marker: dict) -> str: return "headless" if marker.get("mode") == "headless" else "interactive" -# A lock older than this with no heartbeat is stale: a crashed operator must not -# block everyone forever. Breaking one is always logged, never silent. -LOCK_TTL_SECONDS = 2 * 60 * 60 +# A record older than this with no heartbeat is stale: it describes a session +# that stopped coordinating, and showing it would make the record lie. The value +# is unchanged from when this was a lock; only the consequence changed. A stale +# record is filtered out of every display and unlinked by the next `claim`, and +# it can no longer refuse anybody, so breaking one is not an event worth +# announcing any more. +RECORD_TTL_SECONDS = 2 * 60 * 60 + +# One directory of per-worktree records. See the module docstring for why this +# shape and not one shared file. +RECORDS_DIRNAME = "vllm-cpp-operators" + +# The pre-#285 single-file lock. Still READ, so a session that claimed operator +# before this change keeps resolving instead of turning UNDECLARED mid-flight +# and failing its next preflight; its next claim or release removes it. Delete +# this once no pre-#285 file can exist. +LEGACY_RECORD_NAME = "vllm-cpp-operator.lock" UNDECLARED_EXIT = 3 @@ -104,31 +142,45 @@ def marker_path() -> Path: return Path(worktree_id()) / "vllm-cpp-agent-role" -def lock_path() -> Path: - """Shared by every worktree of this repo, and never inside the work tree.""" - common = git("rev-parse", "--path-format=absolute", "--git-common-dir") - return Path(common) / "vllm-cpp-operator.lock" +def common_dir() -> Path: + """Shared by every worktree of this repo, and never inside a work tree.""" + return Path(git("rev-parse", "--path-format=absolute", "--git-common-dir")) + +def records_dir() -> Path: + return common_dir() / RECORDS_DIRNAME -def lock_is_ours(lock: dict | None) -> bool: - """Does the operator lock belong to THIS worktree? - Ownership follows the same identity as the role. A lock written before the +def record_path(worktree: str | None = None) -> Path: + """This worktree's OWN record file, and no one else's. + + The name is derived from the worktree, which is the identity ownership keys + on, so two coordinators claiming at the same instant address two different + paths. That -- not a lock -- is what makes concurrent claims safe. The digest + keeps a path that may contain separators or exotic characters usable as one + filename; the record itself carries the readable worktree path. + """ + key = hashlib.sha256((worktree or worktree_id()).encode("utf-8")).hexdigest()[:16] + return records_dir() / f"{key}.json" + + +def legacy_record_path() -> Path: + return common_dir() / LEGACY_RECORD_NAME + + +def record_is_ours(record: dict | None) -> bool: + """Does this coordinator record belong to THIS worktree? + + Ownership follows the same identity as the role. A record written before the 2026-08-06 correction carries no worktree, so it falls back to its recorded - session: that keeps such a lock releasable and re-claimable by the session - that took it instead of wedging the repo until the TTL expires. - - That fallback is WIDER than the worktree rule -- another worktree running - under the same session id also reads it as ours -- so `claim` rewrites the - record instead of passing, stamping the worktree on and healing the - ambiguity the first time it is used. Delete this branch once no - pre-correction lock can exist. + session: that keeps such a record removable by the session that wrote it + instead of lingering in everyone's display until the TTL expires. """ - if not lock: + if not record: return False - if lock.get("worktree"): - return lock["worktree"] == worktree_id() - return lock.get("session") == session_id() + if record.get("worktree"): + return record["worktree"] == worktree_id() + return record.get("session") == session_id() def read_json(path: Path) -> dict | None: @@ -138,9 +190,130 @@ def read_json(path: Path) -> dict | None: return None -def lock_is_stale(record: dict) -> bool: +def record_is_stale(record: dict) -> bool: beat = record.get("heartbeat", record.get("claimed_at", 0)) - return (time.time() - float(beat)) > LOCK_TTL_SECONDS + try: + return (time.time() - float(beat)) > RECORD_TTL_SECONDS + except (TypeError, ValueError): + # A record whose heartbeat is unreadable describes nothing usable. Treat + # it as stale so it is pruned, never as fresh so it lingers forever. + return True + + +def read_records() -> list[dict]: + """Every readable coordinator record, each carrying the file it came from. + + A record being unlinked by its owner WHILE this reads is ordinary, not an + error: read_json answers None and it is simply not listed. Nothing here + writes, because `show` and `resolve` run inside agent-preflight.sh, which + documents itself as never writing anything. + """ + found: list[dict] = [] + try: + entries = sorted(records_dir().iterdir()) + except OSError: + entries = [] + for entry in entries: + if entry.suffix != ".json": + continue + record = read_json(entry) + if record: + found.append({**record, "path": str(entry)}) + legacy = read_json(legacy_record_path()) + if legacy: + found.append({**legacy, "path": str(legacy_record_path()), "legacy": True}) + return found + + +def our_record() -> dict | None: + return next((record for record in read_records() if record_is_ours(record)), None) + + +def peer_records() -> list[dict]: + """The live coordinators that are NOT this worktree. The display's subject.""" + return [ + record + for record in read_records() + if not record_is_ours(record) and not record_is_stale(record) + ] + + +def write_our_record(claimed_at: float | None = None) -> dict: + """Publish THIS worktree's record atomically, and heal any legacy file. + + temp + os.replace inside the same directory: a concurrent reader sees the + previous record or this one and never a partial write. `claimed_at` is + carried over by `heartbeat` and reset by `claim`. + """ + now = time.time() + record = { + "session": session_id(), + "worktree": worktree_id(), + "claimed_at": now if claimed_at is None else claimed_at, + "heartbeat": now, + "host": os.uname().nodename, + "pid": os.getpid(), + } + target = record_path() + target.parent.mkdir(parents=True, exist_ok=True) + temporary = target.with_name(f".{target.name}.{os.getpid()}.tmp") + temporary.write_text(json.dumps(record), encoding="utf-8") + os.replace(temporary, target) + return record + + +def drop_our_record() -> bool: + """Remove THIS worktree's record -- including a legacy file it owns.""" + removed = False + for record in read_records(): + if record_is_ours(record): + try: + Path(record["path"]).unlink(missing_ok=True) + removed = True + except OSError: + pass + return removed + + +def prune_stale_records() -> list[dict]: + """Drop records past the TTL. Called from `claim`, which already writes.""" + pruned = [] + for record in read_records(): + if record_is_stale(record): + try: + Path(record["path"]).unlink(missing_ok=True) + pruned.append(record) + except OSError: + pass + return pruned + + +def describe_record(record: dict) -> str: + """Who, which worktree, and how long since the heartbeat -- ASCII only.""" + try: + beat = float(record.get("heartbeat", record.get("claimed_at", 0))) + except (TypeError, ValueError): + beat = 0.0 + age = int(max(0.0, time.time() - beat)) + return ( + f"{record.get('worktree') or 'unknown worktree'} - " + f"session {record.get('session') or 'unknown'} " + f"on {record.get('host') or 'unknown host'} " + f"(pid {record.get('pid', '?')}, last heartbeat {age}s ago)" + ) + + +def render_peers(peers: list[dict]) -> list[str]: + """The whole point of keeping the file: who else is coordinating, and where. + + Empty when nobody else is, so a solo session reads no conflict where there + is none. + """ + if not peers: + return [] + return [f"other coordinators recorded: {len(peers)}"] + [ + f" - {describe_record(record)}" for record in peers + ] def current_branch() -> str: @@ -154,31 +327,40 @@ def resolve() -> dict: """Return the resolved role for THIS WORKTREE, or {'role': None, ...}.""" me = session_id() marker = read_json(marker_path()) - lock = read_json(lock_path()) + # One read of the records, split two ways: ours decides the role, the live + # rest are reported as peers. Peers never gate anything -- they are + # information, and every path carries them so any session can see who is + # coordinating where. + records = read_records() + mine = next((record for record in records if record_is_ours(record)), None) + peers = [ + record + for record in records + if not record_is_ours(record) and not record_is_stale(record) + ] # The marker is keyed on the WORKTREE, which is where it lives, and NOT on # the session: a session id is not stable across tool calls, so requiring it # made a declared role invisible one call later. `session` is carried # through as `declared_by` provenance and gates nothing. # - # DECLARABLE, not ROLES: read-only is declarable but holds no lock, so it + # DECLARABLE, not ROLES: read-only is declarable but records nothing, so it # must resolve here while still never counting as "may write". if marker and marker.get("role") in DECLARABLE: declared = marker["role"] - if declared == "operator" and not lock_is_ours(lock): + if declared == "operator" and mine is None: return { "role": None, "session": me, "mode": "interactive", - "reason": "operator marker without a held lock; re-claim", - # Carried on THIS path too, and for the same reason the - # undeclared path below carries it: a worktree keyed lock made - # this branch reachable from a LIVE RIVAL, not only from a - # self-lost lock. Without the key, "locked out by another - # operator" renders identically to "never declared", and every - # reader -- render_probe's NOTE, cmd_show's note, the probe's - # JSON -- tells the session to `claim operator`, which exits 1. - "operator_held_by_other": bool(lock and not lock_is_stale(lock)), + # The one path that still refuses to resolve, and it is + # REACHABLE: a host cleanup deleted exactly this file on + # 2026-08-10. It must stay distinguishable from "never + # declared", because the remedy is named here and now always + # works -- `claim operator` records and succeeds whoever else + # is recorded. + "reason": "operator marker without a coordinator record; re-claim", + "operator_peers": peers, "branch": current_branch(), } return { @@ -188,17 +370,17 @@ def resolve() -> dict: "declared_by": marker.get("session"), "branch": current_branch(), "mode": mode_from_marker(marker), + "operator_peers": peers, "reason": "declared", } # Not declared here. Report what else is going on so the caller can decide. - held_by_other = bool(lock and not lock_is_ours(lock) and not lock_is_stale(lock)) return { "role": None, "session": me, "branch": current_branch(), "mode": "interactive", - "operator_held_by_other": held_by_other, + "operator_peers": peers, "reason": "undeclared", } @@ -207,13 +389,19 @@ def cmd_show(args: argparse.Namespace) -> int: state = resolve() if args.json: print(json.dumps(state)) - elif state["role"]: + return 0 if state["role"] else UNDECLARED_EXIT + if state["role"]: row = f" row={state['row']}" if state.get("row") else "" print(f"role={state['role']}{row} session={state['session']} branch={state['branch']}") else: print(f"role=UNDECLARED session={state['session']} branch={state['branch']}") - if state.get("operator_held_by_other"): - print(" note: the operator lock is held by another live session") + if state.get("reason", "").startswith("operator marker"): + print(" note: this worktree's coordinator record is gone; " + "re-run `claim operator` (it is never refused)") + # Printed for EVERY role, declared or not: any session may need to know who + # else is coordinating, and this is the only place that says so. + for line in render_peers(state.get("operator_peers") or []): + print(line) return 0 if state["role"] else UNDECLARED_EXIT @@ -224,57 +412,35 @@ def cmd_claim(args: argparse.Namespace) -> int: print("ERROR: a helper claims one row: --row ", file=sys.stderr) return 2 + peers: list[dict] = [] if role == "operator": - path = lock_path() - record = {"session": me, "worktree": worktree_id(), - "claimed_at": time.time(), "heartbeat": time.time(), - "host": os.uname().nodename, "pid": os.getpid()} - try: - # Create-exclusive: a second self-declared operator FAILS here rather - # than racing on main, which is the whole point of the lock. - fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644) - with os.fdopen(fd, "w") as handle: - json.dump(record, handle) - except FileExistsError: - existing = read_json(path) or {} - if lock_is_ours(existing): - # Already this worktree's. REWRITE rather than pass: it renews - # the heartbeat (ownership keyed on the worktree means a live - # operator re-claims more often than it beats, and a lock that - # ages out while its owner is alive gets broken by someone - # else), and it stamps `worktree` onto a pre-correction lock, - # which is what stops the legacy session fallback below from - # leaving two worktrees resolving as operator at once. - path.write_text(json.dumps(record), encoding="utf-8") - elif lock_is_stale(existing): - age = int(time.time() - float(existing.get("heartbeat", 0))) - print( - f"NOTE: breaking a STALE operator lock held by " - f"{existing.get('session')} on {existing.get('host')} " - f"({age}s without heartbeat, TTL {LOCK_TTL_SECONDS}s)", - file=sys.stderr, - ) - path.write_text(json.dumps(record), encoding="utf-8") - else: - print( - f"ERROR: the operator role is already held by session " - f"{existing.get('session')} on {existing.get('host')}. " - "This session cannot be the operator; take the helper role " - "instead (scripts/agent-role.py claim helper --row ).", - file=sys.stderr, - ) - return 1 + # No refusal exists here any more (issue #285). A second coordinator is + # RECORDED: it merges PRs and dispatches sub-agents into worktrees, and + # since `main` is never force-pushed, git's non-fast-forward refusal is + # the interlock this file was pretending to be. + # + # Prune first, then read the peers, so a record left by a session that + # died mid-flight neither lingers in the display nor is reported as a + # live coordinator. This is a write path, which is why pruning happens + # here and never in `show`. + prune_stale_records() + peers = peer_records() + # Rewriting our own record is the renewal path too: a live coordinator + # re-claims more often than it beats, and a record that ages out while + # its owner is alive disappears from everyone else's display. It also + # replaces a legacy single-file lock this worktree owned, so that file + # cannot linger and be counted twice. + drop_our_record() + write_our_record() else: - # Downgrading OUT of the operator role must not orphan the lock. - # `claim read-only` ("just looking") is the exact command an operator - # types next, and leaving the lock behind is worse than leaving no role - # at all: heartbeat answers "not the operator; nothing to heartbeat", so - # nothing renews it, while a second worktree's `claim operator` is - # refused for the full TTL by a session that holds no role. Releasing - # here is the same ownership test `release` uses. - if lock_is_ours(read_json(lock_path())): - lock_path().unlink(missing_ok=True) - print(f"released the operator lock (this worktree is now {role})") + # Downgrading OUT of the operator role must not orphan the record. + # `claim read-only` ("just looking") is the exact command a coordinator + # types next, and leaving the record behind makes it lie: heartbeat + # answers "not the operator; nothing to heartbeat", so nothing renews + # it, and everyone else sees a coordinator that stopped coordinating + # until the TTL. Removing here is the same ownership test `release` uses. + if drop_our_record(): + print(f"removed this worktree's coordinator record (now {role})") marker_path().write_text( json.dumps({ @@ -289,6 +455,9 @@ def cmd_claim(args: argparse.Namespace) -> int: encoding="utf-8", ) print(f"claimed role={role}" + (f" row={args.row}" if args.row else "")) + # Information, never an obstacle: several coordinators may run at once. + for line in render_peers(peers): + print(line) return 0 @@ -297,19 +466,17 @@ def cmd_heartbeat(_: argparse.Namespace) -> int: if state["role"] != "operator": print("not the operator; nothing to heartbeat") return 0 - path = lock_path() - record = read_json(path) or {} - record["heartbeat"] = time.time() - path.write_text(json.dumps(record), encoding="utf-8") + existing = our_record() or {} + # Only ever this worktree's own file, and `claimed_at` is carried over so + # the record still says when this coordinator started. + write_our_record(claimed_at=existing.get("claimed_at")) print("heartbeat updated") return 0 def cmd_release(_: argparse.Namespace) -> int: - lock = read_json(lock_path()) - if lock_is_ours(lock): - lock_path().unlink(missing_ok=True) - print("released the operator lock") + if drop_our_record(): + print("removed this worktree's coordinator record") marker_path().unlink(missing_ok=True) print("released the role marker") return 0 @@ -333,10 +500,10 @@ def main() -> int: ) claim.set_defaults(func=cmd_claim) - sub.add_parser("heartbeat", help="keep the operator lock alive").set_defaults( + sub.add_parser("heartbeat", help="keep this coordinator record fresh").set_defaults( func=cmd_heartbeat ) - sub.add_parser("release", help="drop the role and any held lock").set_defaults( + sub.add_parser("release", help="drop the role and this worktree's record").set_defaults( func=cmd_release ) diff --git a/scripts/agent-start.py b/scripts/agent-start.py index 398e1fbaf..71b9c60cd 100755 --- a/scripts/agent-start.py +++ b/scripts/agent-start.py @@ -46,11 +46,20 @@ def _load_onboard(): def _status_lines(state: dict) -> list[str]: """Render status labels only; never echo environment values or keys.""" - return [ + lines = [ f"environment: {state.get('env') or 'unavailable'}", f"branch: {state.get('branch') or 'unavailable'}", f"worktree: {state.get('worktree') or 'unavailable'}", ] + # Who else is coordinating. Until issue #285 this router turned the same + # fact into "BLOCKED: the operator lock is held by another live worktree" + # and told the session not to run "a known-failing claim". That claim no + # longer fails: several coordinators may run at once, so a peer is status, + # never a blocker. Kept SHORT -- the welcome route is width-checked. + peers = state.get("operator_peers") or [] + if peers: + lines.append(f"other coordinators: {len(peers)} recorded (claim is allowed)") + return lines def _claim_command(intent: str, row: str | None, headless: bool) -> str: @@ -131,22 +140,6 @@ def _undeclared_actions( lines = _status_lines(state) if intent is None: - if state.get("blocked_by_other_operator"): - reason = state.get("reason") or "reason unavailable" - lines.extend( - [ - "BLOCKED OPTION: the operator lock is held by another live worktree.", - f"Reason: {reason}", - "1. Relay only the welcome block above verbatim.", - "2. Then ask what the contributor is here to do.", - "3. If the contributor chooses operator, report the conflict;", - " do not run a known-failing claim or select another role.", - "4. For helper or read-only, use the matching claim command.", - "5. After claiming, rerun scripts/agent-start.py.", - "6. Then run scripts/agent-preflight.sh.", - ] - ) - return lines lines.extend( [ "1. Relay only the welcome block above verbatim.", @@ -158,18 +151,6 @@ def _undeclared_actions( ) return lines - if intent == "operator" and state.get("blocked_by_other_operator"): - reason = state.get("reason") or "reason unavailable" - lines.extend( - [ - "BLOCKED: the operator lock is held by another live worktree.", - f"Reason: {reason}", - "Do not run a known-failing claim or select another role.", - "Report the conflict and obtain direction.", - ] - ) - return lines - if intent == "helper" and not row: lines.extend(_helper_without_row(state)) return lines diff --git a/tests/scripts/test_agent_onboard.py b/tests/scripts/test_agent_onboard.py index fe79303fb..3196cf5f6 100644 --- a/tests/scripts/test_agent_onboard.py +++ b/tests/scripts/test_agent_onboard.py @@ -118,16 +118,22 @@ def test_undeclared_render_carries_the_interview_hint(self): onboard.render_probe(undeclared_mode), ) - def test_lockout_by_another_operator_is_not_rendered_as_never_declared(self): - # Beyond the brief. resolve() knows the difference; a session locked - # out by a live operator must not be told to claim operator, because - # that claim will fail. Both are role=None, so only the NOTE separates - # them. - blocked = dict(self.UNDECLARED, blocked_by_other_operator=True) - out = onboard.render_probe(blocked) - self.assertIn("NOTE", out) - self.assertIn("held by another live session", out) - self.assertNotIn("NOTE", onboard.render_probe(self.UNDECLARED)) + def test_live_coordinators_are_rendered_and_never_as_a_blocker(self): + # Beyond the brief. Until issue #285 this asserted a NOTE saying the + # lock was "held by another live session" so the reader would not run a + # claim "that will fail". The claim no longer fails, so the peer is + # rendered as information -- who, where, how long since the heartbeat -- + # and the interview hint still offers every role. + peer = {"worktree": "/repo/.git/worktrees/other", "session": "peer-1", + "host": "box", "pid": 42, "heartbeat": 0} + out = onboard.render_probe(dict(self.UNDECLARED, operator_peers=[peer])) + self.assertIn("other coordinators recorded: 1", out) + self.assertIn("/repo/.git/worktrees/other", out) + self.assertIn("peer-1", out) + self.assertNotIn("will fail", out) + self.assertIn("operator", out) # the role is still on the table + # and with nobody else recorded, no coordinator line is invented + self.assertNotIn("other coordinators", onboard.render_probe(self.UNDECLARED)) def test_probe_never_exits_nonzero(self): # The probe reports; it does not gate. Preflight gates. @@ -146,11 +152,11 @@ class ProbeFieldsComeFromResolve(unittest.TestCase): Every other probe test in this file feeds render_probe an already-populated fixture, so nothing asserted that probe() actually reads its fields out of agent-role.py. Five hardcodes therefore left the whole suite green: - `blocked_by_other_operator: False`, `reason: None`, `mode: "headless"`, + `operator_peers: []`, `reason: None`, `mode: "headless"`, `role: "operator"` and `env: "present"`. `--probe` is the front door .agents/workflow.md sends every session to, and a probe that answers `role: operator` to a session that never declared one is worse than no - probe: it points at a `claim operator` that will exit 1. + probe: it reports a claim that was never made. So these claim in a THROWAWAY repo and read the values back out of probe(). """ @@ -198,7 +204,7 @@ def test_probe_reports_the_role_and_row_that_were_claimed(self) -> None: self.assertEqual(state["row"], "PROBE-WIRING") self.assertEqual(state["mode"], "interactive") self.assertEqual(state["reason"], "declared") - self.assertIs(state["blocked_by_other_operator"], False) + self.assertEqual(state["operator_peers"], []) self.assertEqual(state["branch"], "master") self.assertEqual( state["worktree"], @@ -239,29 +245,46 @@ def test_probe_reports_a_mode_that_was_declared(self) -> None: self.assertEqual(state["role"], "read-only") self.assertEqual(state["mode"], "headless") - def test_probe_reports_a_lockout_by_a_live_rival(self) -> None: - # Kills `blocked_by_other_operator: False` and `reason: None`, and pins - # the resolve() branch that reaches this state: an operator marker whose - # lock is now held by ANOTHER LIVE WORKTREE. Since ownership keys on the - # worktree, that is not a hypothetical -- it is what a rival claim - # leaves behind, and rendering it as "never declared" sends the session - # straight at a `claim operator` that exits 1. + def test_probe_carries_the_live_coordinators_out_of_resolve(self) -> None: + # Kills `operator_peers: []` and `reason: None`, against records written + # by the REAL CLI in two real worktrees. Until issue #285 this asserted + # `blocked_by_other_operator: True` and a "held by another live session" + # NOTE; a peer is now reported and blocks nothing. + self.assertEqual(self.claim(self.repo, "a", "operator").returncode, 0) + rival = self.worktree("rival") + self.assertEqual(self.claim(rival, "rival-session", "operator").returncode, 0) + rival_git_dir = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=rival, text=True).strip() + + state = self.probe_in(self.repo) + self.assertEqual(state["role"], "operator") + self.assertEqual(state["reason"], "declared") + self.assertEqual( + [record.get("worktree") for record in state["operator_peers"]], + [rival_git_dir]) + rendered = onboard.render_probe(state) + self.assertIn("other coordinators recorded: 1", rendered) + self.assertIn(rival_git_dir, rendered) + self.assertIn("rival-session", rendered) + + def test_probe_reports_a_vanished_record_as_re_claimable(self) -> None: + # The one state that still fails to resolve, and it is REACHABLE: a + # host cleanup deleted this worktree's record on 2026-08-10. It must + # stay distinguishable from "never declared", because the remedy is + # `claim operator` and it now always succeeds. self.assertEqual(self.claim(self.repo, "a", "operator").returncode, 0) common = subprocess.check_output( ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], cwd=self.repo, text=True).strip() - (Path(common) / "vllm-cpp-operator.lock").unlink() - rival = self.worktree("rival") - self.assertEqual(self.claim(rival, "b", "operator").returncode, 0) + for record in (Path(common) / "vllm-cpp-operators").glob("*.json"): + record.unlink() state = self.probe_in(self.repo) self.assertIsNone(state["role"]) - self.assertIs(state["blocked_by_other_operator"], True) self.assertEqual( - state["reason"], "operator marker without a held lock; re-claim") - # and the human surface must say so, not print the generic hint alone - rendered = onboard.render_probe(state) - self.assertIn("held by another live session", rendered) + state["reason"], + "operator marker without a coordinator record; re-claim") + self.assertEqual(state["operator_peers"], []) def test_probe_reports_the_env_state_it_was_given(self) -> None: # Kills `env: "present"`. It cannot be pinned against the real tree -- diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index f8537bbef..c81539477 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -2,14 +2,15 @@ """Unit and mutation checks for scripts/agent-role.py (W0) and scripts/check-role-discipline.py (W1). -The behaviours that matter are the ones the protocol rests on: a second -self-declared operator must FAIL rather than race, a session sharing a checkout -must NOT inherit another session's role, a stale lock must be breakable but -never silently, and feature code must not reach main without a row/* PR. +The behaviours that matter are the ones the protocol rests on: a coordinator +must be RECORDED and never refused (issue #285), a session sharing a checkout +must NOT inherit another session's role, a stale record must be pruned without +blocking anyone, and feature code must not reach main without a row/* PR. """ from __future__ import annotations +import concurrent.futures import importlib.util import json import os @@ -66,15 +67,54 @@ def tearDown(self) -> None: def worktree(self, name: str) -> Path: """A real second worktree of the throwaway repo. - A role keys on the worktree, so both surviving invariants -- one - operator per repo, and helper isolation -- can only be proven with a - genuine second worktree rather than a second session id. + A role keys on the worktree, so everything this suite proves -- + coordinator records being per-worktree, and helper isolation -- can only + be proven with a genuine second worktree rather than a second session id. """ path = self.repo / f".{name}" subprocess.run(["git", "worktree", "add", "-q", str(path), "-b", name], cwd=self.repo, check=True, capture_output=True) return path + def common(self) -> Path: + return Path(subprocess.check_output( + ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], + cwd=self.repo, text=True).strip()) + + def records_dir(self) -> Path: + """Where coordinator records live: shared by every worktree, never + inside a work tree, and one file per worktree so no two claimants ever + write the same path.""" + return self.common() / "vllm-cpp-operators" + + def record_files(self) -> list[Path]: + directory = self.records_dir() + return sorted(directory.glob("*.json")) if directory.is_dir() else [] + + def records(self) -> list[dict]: + return [json.loads(path.read_text(encoding="utf-8")) for path in self.record_files()] + + def record_of(self, worktree: Path) -> dict: + """The record whose worktree is `worktree`'s git dir. Fails if absent.""" + wanted = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=worktree, text=True).strip() + for record in self.records(): + if record.get("worktree") == wanted: + return record + raise AssertionError(f"no coordinator record for {wanted}: {self.records()}") + + def backdate(self, worktree: Path, seconds: float) -> Path: + """Age one worktree's record past the TTL, as a crashed session leaves it.""" + wanted = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=worktree, text=True).strip() + for path in self.record_files(): + record = json.loads(path.read_text(encoding="utf-8")) + if record.get("worktree") == wanted: + record["heartbeat"] = time.time() - seconds + path.write_text(json.dumps(record), encoding="utf-8") + return path + raise AssertionError(f"no coordinator record for {wanted}") + class RoleLifecycle(_TempRepo, unittest.TestCase): """Exercised against a throwaway repo, never the real one.""" @@ -88,18 +128,25 @@ def test_claim_then_resolve(self) -> None: self.assertEqual(out.returncode, 0) self.assertIn("role=operator", out.stdout) - def test_second_operator_is_refused(self) -> None: - """The core mutual-exclusion guarantee. + def test_a_second_coordinator_is_recorded_not_refused(self) -> None: + """Issue #285, user-directed: the record must never refuse. - Stated across WORKTREES since the 2026-08-06 correction: a role keys on - the worktree, so a second session in the SAME worktree is the same - operator (idempotent), while the lock in the git common dir still - refuses a genuinely different one. + This test asserted the OPPOSITE until 2026-08-10 -- a second worktree's + `claim operator` exited 1 with "already held". An operator is a + coordinator: it merges PRs and dispatches sub-agents into worktrees, and + it never force-pushes `main`, so git's non-fast-forward refusal is the + interlock and a JSON file in `.git/` never was one. The refusal cost two + hours of blocked coordination every time a session died mid-flight. """ - run_role(self.repo, "a", "claim", "operator") - second = run_role(self.worktree("rival"), "b", "claim", "operator") - self.assertEqual(second.returncode, 1) - self.assertIn("already held", second.stderr) + self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) + rival = self.worktree("rival") + second = run_role(rival, "b", "claim", "operator") + self.assertEqual(second.returncode, 0, second.stderr) + self.assertNotIn("already held", second.stderr) + # Both are operators, and both are recorded -- neither displaced the other. + for where in (self.repo, rival): + self.assertIn("role=operator", run_role(where, "z", "show").stdout) + self.assertEqual(len(self.record_files()), 2, self.records()) def test_another_session_in_the_same_worktree_shares_the_role(self) -> None: """The accepted cost of keying on the worktree, made explicit. @@ -123,33 +170,17 @@ def test_helper_requires_a_row(self) -> None: self.assertEqual(ok.returncode, 0) self.assertIn("row=ENG-FOO", run_role(self.repo, "a", "show").stdout) - def test_release_frees_the_lock_for_another_session(self) -> None: + def test_release_removes_this_worktrees_record(self) -> None: run_role(self.repo, "a", "claim", "operator") + self.assertEqual(len(self.record_files()), 1) run_role(self.repo, "a", "release") + self.assertEqual(self.record_files(), []) self.assertEqual(run_role(self.repo, "b", "claim", "operator").returncode, 0) - def test_stale_lock_is_broken_but_reported(self) -> None: + def test_operator_marker_without_a_record_does_not_resolve(self) -> None: run_role(self.repo, "a", "claim", "operator") - common = subprocess.check_output( - ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], - cwd=self.repo, text=True).strip() - lock = Path(common) / "vllm-cpp-operator.lock" - record = json.loads(lock.read_text()) - record["heartbeat"] = time.time() - (10 * 60 * 60) - lock.write_text(json.dumps(record)) - # From ANOTHER worktree: re-claiming inside the worktree that already - # holds the lock is the same operator and is idempotent, so a crashed - # operator can only be displaced from somewhere else. - took = run_role(self.worktree("successor"), "b", "claim", "operator") - self.assertEqual(took.returncode, 0) - self.assertIn("STALE", took.stderr) # broken, but never silently - - def test_operator_marker_without_lock_does_not_resolve(self) -> None: - run_role(self.repo, "a", "claim", "operator") - common = subprocess.check_output( - ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], - cwd=self.repo, text=True).strip() - (Path(common) / "vllm-cpp-operator.lock").unlink() + for path in self.record_files(): + path.unlink() self.assertEqual(run_role(self.repo, "a", "show").returncode, 3) @@ -209,14 +240,20 @@ def test_resolve_ignores_the_marker_session(self) -> None: self.assertEqual(state["row"], "ENG-BAR") self.assertEqual(state["declared_by"], "some-other-session") - def test_one_operator_per_repo_holds_across_worktrees(self) -> None: - # Keying on the worktree must not WIDEN the lock: it lives in the git - # common dir, shared by every worktree, and that is the scope of "one - # operator per repo". + def test_the_records_are_shared_by_every_worktree(self) -> None: + # Keying on the worktree must not NARROW the records: they live in the + # git common dir, shared by every worktree, which is what makes "who is + # coordinating where" answerable from any of them. self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) - second = run_role(self.worktree("rival"), "b", "claim", "operator") - self.assertEqual(second.returncode, 1) - self.assertIn("already held", second.stderr) + rival = self.worktree("rival") + self.assertEqual(run_role(rival, "b", "claim", "operator").returncode, 0) + # The same two records are visible from either side, and neither lives + # in a work tree where a commit could pick it up. + self.assertEqual(len(self.record_files()), 2) + for where in (self.repo, rival): + self.assertNotIn("vllm-cpp-operators", subprocess.check_output( + ["git", "status", "--porcelain", "--untracked-files=all"], + cwd=where, text=True)) def test_helper_marker_does_not_leak_into_another_worktree(self) -> None: # Isolation, asserted where it is now real. The SAME session id in @@ -227,11 +264,10 @@ def test_helper_marker_does_not_leak_into_another_worktree(self) -> None: self.assertEqual(out.returncode, 3) self.assertIn("UNDECLARED", out.stdout) - def _lock(self, repo: Path) -> Path: - common = subprocess.check_output( - ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], - cwd=repo, text=True).strip() - return Path(common) / "vllm-cpp-operator.lock" + def _legacy(self) -> Path: + """The pre-#285 single-file lock. Still read, so a session that claimed + before the change does not silently become UNDECLARED mid-flight.""" + return self.common() / "vllm-cpp-operator.lock" def _resolve_in(self, where: Path) -> dict: """resolve()'s OWN return value, read in `where`. @@ -246,123 +282,244 @@ def _resolve_in(self, where: Path) -> dict: finally: os.chdir(saved) - def test_reclaiming_your_own_lock_refreshes_the_heartbeat(self) -> None: - # The idempotent branch REWRITES the record instead of passing. With - # ownership keyed on the worktree a live operator is likelier to - # re-claim than to heartbeat, and a lock that ages out while its owner - # is alive gets broken by someone else. + def test_reclaiming_your_own_record_refreshes_the_heartbeat(self) -> None: + # A live coordinator is likelier to re-claim than to heartbeat, and a + # record that ages past the TTL while its owner is alive drops out of + # everyone else's display of who is working where. run_role(self.repo, "a", "claim", "operator") - lock = self._lock(self.repo) - record = json.loads(lock.read_text(encoding="utf-8")) - record["heartbeat"] = time.time() - (10 * 60 * 60) - lock.write_text(json.dumps(record), encoding="utf-8") - + self.backdate(self.repo, 10 * 60 * 60) run_role(self.repo, "b", "claim", "operator") self.assertGreater( - json.loads(lock.read_text(encoding="utf-8"))["heartbeat"], - time.time() - 60, - ) - - def test_a_legacy_lock_cannot_produce_two_operators(self) -> None: - # A lock written BEFORE the 2026-08-06 correction carries no worktree, - # so ownership falls back to its recorded session -- and two worktrees - # under the same session id would both read it as theirs. Rewriting on - # the idempotent branch stamps the worktree on, so the fallback heals - # the first time it is used and only one worktree stays the operator. - run_role(self.repo, "s1", "claim", "operator") - lock = self._lock(self.repo) - legacy = json.loads(lock.read_text(encoding="utf-8")) - del legacy["worktree"] - lock.write_text(json.dumps(legacy), encoding="utf-8") - - twin = self.worktree("twin") - self.assertEqual(run_role(twin, "s1", "claim", "operator").returncode, 0) - resolved = [ - run_role(self.repo, "s1", "show"), - run_role(twin, "s1", "show"), - ] - operators = [r for r in resolved if r.returncode == 0 and "role=operator" in r.stdout] - self.assertEqual( - len(operators), 1, - f"exactly one operator expected, got {[r.stdout for r in resolved]}") - - def test_an_operator_marker_beaten_to_the_lock_reports_the_lockout(self) -> None: - # Keying lock OWNERSHIP on the worktree made this branch reachable from - # a LIVE RIVAL, not only from a self-lost lock: another worktree can now - # hold the lock while this one still carries an operator marker. Without - # operator_held_by_other the state is indistinguishable from "never - # declared" -- same JSON, same cmd_show output, same probe NOTE (none) -- - # and every one of those tells the session to `claim operator`, which - # exits 1. Deleting the key from this branch must go red. + self.record_of(self.repo)["heartbeat"], time.time() - 60) + + def test_a_legacy_lock_file_is_adopted_as_this_worktrees_record(self) -> None: + # A pre-#285 session holds the single-file lock. It must keep resolving + # as operator -- turning a live coordinator UNDECLARED mid-flight fails + # its next preflight -- and the next claim must heal the repo by moving + # it into the records directory, so the legacy file cannot linger and be + # counted twice. + legacy = self._legacy() + worktree = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=self.repo, text=True).strip() + legacy.write_text(json.dumps({ + "session": "pre-285", "worktree": worktree, + "claimed_at": time.time(), "heartbeat": time.time(), + "host": "somewhere", "pid": 1}), encoding="utf-8") + (Path(worktree) / "vllm-cpp-agent-role").write_text( + json.dumps({"role": "operator", "session": "pre-285", "at": time.time()}), + encoding="utf-8") + + self.assertIn("role=operator", run_role(self.repo, "pre-285", "show").stdout) + self.assertEqual(run_role(self.repo, "later", "claim", "operator").returncode, 0) + self.assertFalse(legacy.exists(), "the legacy lock file was not healed away") + self.assertEqual(len(self.record_files()), 1, self.records()) + + def test_an_operator_whose_record_vanished_is_told_to_re_claim(self) -> None: + # The one path that still refuses to resolve: an operator marker with no + # record of its own. It is REACHABLE -- a host cleanup deleted exactly + # this file on 2026-08-10 -- and it must be distinguishable from "never + # declared", because the remedy is `claim operator`, which now always + # succeeds. A live coordinator elsewhere changes none of it. self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) - self._lock(self.repo).unlink() rival = self.worktree("live-rival") self.assertEqual(run_role(rival, "b", "claim", "operator").returncode, 0) + self.record_of(self.repo) # precondition: ours exists before we delete it + for path in self.record_files(): + if json.loads(path.read_text(encoding="utf-8")).get("worktree") == \ + subprocess.check_output(["git", "rev-parse", "--absolute-git-dir"], + cwd=self.repo, text=True).strip(): + path.unlink() state = self._resolve_in(self.repo) self.assertIsNone(state["role"]) self.assertEqual( - state["reason"], "operator marker without a held lock; re-claim") - # .get, so DROPPING the key fails on the value rather than erroring - # on the lookup: a missing signal is the same defect as a false one. - self.assertIs(state.get("operator_held_by_other"), True) - # the CLI surface, which is what an agent actually reads + state["reason"], + "operator marker without a coordinator record; re-claim") shown = run_role(self.repo, "a", "show") self.assertEqual(shown.returncode, 3) - self.assertIn("held by another live session", shown.stdout) - - def test_a_self_lost_lock_is_not_reported_as_a_rival(self) -> None: - # The other side of the same key: with the lock simply GONE there is no - # rival, so a blanket True would be a different lie. Hardcoding the key - # either way now fails one of these two tests. - run_role(self.repo, "a", "claim", "operator") - self._lock(self.repo).unlink() - state = self._resolve_in(self.repo) - self.assertIsNone(state["role"]) - self.assertIs(state.get("operator_held_by_other"), False) - self.assertNotIn("another live session", run_role(self.repo, "a", "show").stdout) + # No refusal language survives anywhere: the rival is reported as a + # peer, never as a blocker, and re-claiming works on the spot. + self.assertNotIn("cannot be the operator", shown.stdout + shown.stderr) + self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) + self.assertIn("role=operator", run_role(self.repo, "a", "show").stdout) - def test_downgrading_out_of_operator_releases_the_lock(self) -> None: + def test_downgrading_out_of_operator_releases_the_record(self) -> None: # `claim read-only` ("just looking") is the exact next command an - # operator types, and leaving the lock behind is worse than holding no + # operator types, and leaving the record behind is worse than holding no # role: heartbeat answers "not the operator; nothing to heartbeat", so - # nothing renews it, and a second worktree is refused for the full 2h - # TTL by a session that holds no role at all. + # nothing renews it, and the display then shows a coordinator that is + # not coordinating for the full 2h TTL. self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) - self.assertTrue(self._lock(self.repo).exists()) + self.assertEqual(len(self.record_files()), 1) downgrade = run_role(self.repo, "a", "claim", "read-only") self.assertEqual(downgrade.returncode, 0, downgrade.stderr) - self.assertFalse(self._lock(self.repo).exists()) + self.assertEqual(self.record_files(), []) self.assertIn("role=read-only", run_role(self.repo, "a", "show").stdout) - # the lock is genuinely free, not merely absent from this worktree - successor = run_role(self.worktree("successor"), "b", "claim", "operator") - self.assertEqual(successor.returncode, 0, successor.stderr) - def test_downgrading_to_helper_releases_the_lock_too(self) -> None: + def test_downgrading_to_helper_releases_the_record_too(self) -> None: # Same rule, the other non-operator answer: the release keys on "the # claimed role is not operator", not on read-only specifically. run_role(self.repo, "a", "claim", "operator") self.assertEqual( run_role(self.repo, "a", "claim", "helper", "--row", "ENG-FOO").returncode, 0) - self.assertFalse(self._lock(self.repo).exists()) + self.assertEqual(self.record_files(), []) - def test_a_downgrade_never_frees_ANOTHER_worktrees_lock(self) -> None: - # The release must use the same ownership test `release` does. A - # read-only claim in one worktree stealing the operator lock from - # another would be the mutual-exclusion guarantee inverted. + def test_a_downgrade_never_removes_ANOTHER_worktrees_record(self) -> None: + # The removal must use the same ownership test `release` does. A + # read-only claim in one worktree erasing another worktree's record + # would make the record lie about who is working where. run_role(self.repo, "a", "claim", "operator") elsewhere = self.worktree("bystander") self.assertEqual(run_role(elsewhere, "b", "claim", "read-only").returncode, 0) - self.assertTrue(self._lock(self.repo).exists()) + self.assertEqual(len(self.record_files()), 1) self.assertIn("role=operator", run_role(self.repo, "a", "show").stdout) - def test_release_from_a_new_session_frees_the_lock(self) -> None: - # Release has to cross the call boundary as well, or a lock taken in one - # call is unreleasable in the next and wedges the repo until the TTL. + def test_release_from_a_new_session_removes_the_record(self) -> None: + # Release has to cross the call boundary as well, or a record written in + # one call is unreleasable in the next and shows a phantom coordinator + # until the TTL. run_role(self.repo, "call-1", "claim", "operator") run_role(self.repo, "call-2-different-pid", "release") - taken = run_role(self.worktree("next"), "c", "claim", "operator") - self.assertEqual(taken.returncode, 0) + self.assertEqual(self.record_files(), []) + + +class CoordinatorRecords(_TempRepo, unittest.TestCase): + """Issue #285: the file records who is coordinating where; it never refuses. + + Everything here is stated across REAL worktrees, because a record keys on + the worktree and one file per worktree is what makes concurrent claimants + safe: two claims write two different paths, so neither can lose the other's + record, and each publish is a rename over its own path. + """ + + def peers_shown(self, where: Path) -> str: + shown = run_role(where, "watcher", "show") + return shown.stdout + + def test_show_lists_the_other_live_coordinators(self) -> None: + # The point of keeping the file at all: who, which worktree, and how + # long since the heartbeat. Diagnosing a dead holder needed exactly this + # on 2026-08-10 and the tool would not say it. + run_role(self.repo, "primary", "claim", "operator") + rival = self.worktree("rival") + run_role(rival, "rival-session", "claim", "operator") + rival_git_dir = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=rival, text=True).strip() + + shown = self.peers_shown(self.repo) + self.assertIn("other coordinators", shown) + self.assertIn(rival_git_dir, shown) # which worktree + self.assertIn("rival-session", shown) # who + self.assertIn("heartbeat", shown) # how long since + self.assertRegex(shown, r"\d+s ago") + + def test_show_never_lists_this_worktree_as_a_peer(self) -> None: + # A record that counted itself would report a coordinator conflict with + # nobody, which is how a record starts reading like a lock again. + run_role(self.repo, "solo", "claim", "operator") + own_git_dir = subprocess.check_output( + ["git", "rev-parse", "--absolute-git-dir"], cwd=self.repo, text=True).strip() + shown = run_role(self.repo, "solo", "show").stdout + self.assertIn("role=operator", shown) + self.assertNotIn("other coordinators", shown) + self.assertNotIn(own_git_dir, shown) + + def test_release_removes_only_the_callers_record(self) -> None: + run_role(self.repo, "a", "claim", "operator") + rival = self.worktree("rival") + run_role(rival, "b", "claim", "operator") + + run_role(self.repo, "a", "release") + self.assertEqual(len(self.record_files()), 1, self.records()) + self.record_of(rival) # the survivor is the OTHER worktree's + self.assertIn("role=operator", run_role(rival, "b", "show").stdout) + self.assertEqual(run_role(self.repo, "a", "show").returncode, 3) + + def test_a_stale_record_is_pruned_and_never_blocks(self) -> None: + # A session killed mid-flight leaves a dead pid and a frozen heartbeat. + # The TTL stays; what changes is the consequence -- the stale record + # drops out of the display instead of refusing everyone for two hours. + run_role(self.repo, "crashed", "claim", "operator") + self.backdate(self.repo, 10 * 60 * 60) + successor = self.worktree("successor") + + took = run_role(successor, "b", "claim", "operator") + self.assertEqual(took.returncode, 0, took.stderr) + self.assertNotIn("crashed", took.stdout + took.stderr) + shown = run_role(successor, "b", "show").stdout + self.assertIn("role=operator", shown) + self.assertNotIn("other coordinators", shown) + self.assertNotIn("crashed", shown) + self.assertEqual(len(self.record_files()), 1, self.records()) + self.record_of(successor) + + def test_a_stale_record_is_hidden_before_anything_prunes_it(self) -> None: + # Pruning is a WRITE, so it happens on claim and never in `show`: + # agent-preflight.sh documents itself as never writing anything. The + # display must therefore filter on the TTL itself, not rely on a + # previous claim having cleaned up. + run_role(self.repo, "crashed", "claim", "operator") + rival = self.worktree("rival") + run_role(rival, "live", "claim", "operator") + self.backdate(self.repo, 10 * 60 * 60) + + shown = run_role(rival, "live", "show").stdout + self.assertNotIn("crashed", shown) + self.assertNotIn("other coordinators", shown) + self.assertEqual(len(self.record_files()), 2, "show must not delete anything") + + def test_concurrent_claims_lose_no_record(self) -> None: + # The representation exists for this case. Eight worktrees claim at + # once; every one of them must end up recorded, and every record must be + # readable JSON -- a shared file rewritten by eight writers loses some. + worktrees = [self.worktree(f"coord{index}") for index in range(8)] + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as pool: + results = list(pool.map( + lambda pair: run_role(pair[1], f"s{pair[0]}", "claim", "operator"), + list(enumerate(worktrees)))) + for index, result in enumerate(results): + self.assertEqual(result.returncode, 0, f"claim {index}: {result.stderr}") + self.assertEqual(len(self.record_files()), 8, self.records()) + for worktree in worktrees: + self.record_of(worktree) # every one survived, none was overwritten + # And every claimant sees the other seven. + shown = run_role(worktrees[0], "s0", "show").stdout + self.assertIn("other coordinators recorded: 7", shown) + + def test_a_claim_never_rewrites_another_worktrees_record(self) -> None: + # The atomicity argument in one assertion: a writer only ever touches + # the path derived from its OWN worktree, so a peer's bytes are + # untouched. If claims ever shared a file this is the first thing to go. + run_role(self.repo, "a", "claim", "operator") + before = self.record_files()[0].read_bytes() + rival = self.worktree("rival") + run_role(rival, "b", "claim", "operator") + run_role(rival, "b", "heartbeat") + run_role(rival, "b", "release") + self.assertEqual(self.record_files()[0].read_bytes(), before) + + def test_heartbeat_renews_only_this_worktrees_record(self) -> None: + run_role(self.repo, "a", "claim", "operator") + rival = self.worktree("rival") + run_role(rival, "b", "claim", "operator") + self.backdate(self.repo, 30 * 60) + self.backdate(rival, 30 * 60) + + beat = run_role(rival, "b", "heartbeat") + self.assertEqual(beat.returncode, 0, beat.stderr) + self.assertGreater(self.record_of(rival)["heartbeat"], time.time() - 60) + self.assertLess(self.record_of(self.repo)["heartbeat"], time.time() - 60) + + def test_a_record_is_never_written_inside_a_work_tree(self) -> None: + # It must stay uncommittable. `git status` in the worktree that claimed + # is the check that matters, because that is where a `git add` runs. + run_role(self.repo, "a", "claim", "operator") + self.assertTrue(self.records_dir().is_dir()) + self.assertEqual( + subprocess.check_output( + ["git", "status", "--porcelain", "--untracked-files=all"], + cwd=self.repo, text=True).strip(), + "") class RoleDiscipline(unittest.TestCase): @@ -698,18 +855,16 @@ def test_resolve_reports_interactive_unless_headless_was_declared(self) -> None: self.assertIsNone(undeclared["role"]) self.assertEqual(undeclared["mode"], "interactive") - def test_read_only_takes_no_operator_lock(self) -> None: - # The whole reason read-only exists: a session that only reads must not - # hold the repo-wide operator lock, or it blocks a real operator. + def test_read_only_writes_no_coordinator_record(self) -> None: + # The whole reason read-only exists: a session that only reads is not + # coordinating, so it must not appear in the record of who is. self.assertEqual(run_role(self.repo, "a", "claim", "read-only").returncode, 0) - common = subprocess.check_output( - ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], - cwd=self.repo, text=True).strip() - self.assertFalse((Path(common) / "vllm-cpp-operator.lock").exists()) + self.assertEqual(self.record_files(), []) self.assertIn("role=read-only", run_role(self.repo, "a", "show").stdout) - # ... and a real operator elsewhere is still free to take the lock. + # ... and a real coordinator elsewhere records itself normally. self.assertEqual( run_role(self.worktree("real-operator"), "b", "claim", "operator").returncode, 0) + self.assertEqual(len(self.record_files()), 1) if __name__ == "__main__": diff --git a/tests/scripts/test_agent_start.py b/tests/scripts/test_agent_start.py index f432665be..0f5d91745 100644 --- a/tests/scripts/test_agent_start.py +++ b/tests/scripts/test_agent_start.py @@ -34,7 +34,7 @@ def state(**changes): "mode": "interactive", "branch": "main", "worktree": "/repo/.git", - "blocked_by_other_operator": False, + "operator_peers": [], "reason": "undeclared", "env": "present", "env_missing": [], @@ -180,35 +180,42 @@ def test_conflicting_declared_intent_reports_mismatch_without_claim(self): self.assertNotIn("scripts/agent-role.py claim operator", out) self.assertIn("No worktree or PR was created by this command", out) - def test_external_operator_lock_blocks_known_failing_claim(self): + def test_a_recorded_peer_never_blocks_the_operator_claim(self): + """Issue #285. This test asserted the OPPOSITE until 2026-08-10. + + It required "BLOCKED: the operator lock is held by another live + worktree" and forbade printing the claim command. `claim operator` is + now never refused, so that route sent a session away from a command + that works. The peer is reported as status and the claim is still + offered. + """ out = self.render( - state( - blocked_by_other_operator=True, - reason="operator lock held elsewhere", - ), + state(operator_peers=[{"worktree": "/repo/.git/worktrees/other"}]), intent="operator", ) - self.assertIn("operator lock is held by another live worktree", out) - self.assertIn("operator lock held elsewhere", out) - self.assertNotIn("scripts/agent-role.py claim operator", out) - self.assertNotIn("claim helper", out) + self.assertIn("other coordinators: 1 recorded", out) + self.assertIn("scripts/agent-role.py claim operator", out) + self.assertNotIn("BLOCKED", out) + self.assertNotIn("known-failing", out) - def test_first_time_route_surfaces_operator_lock_before_role_choice(self): + def test_first_time_route_reports_peers_without_withholding_a_role(self): out = self.render( - state( - blocked_by_other_operator=True, - reason="operator lock held elsewhere", - ) + state(operator_peers=[ + {"worktree": "/repo/.git/worktrees/a"}, + {"worktree": "/repo/.git/worktrees/b"}, + ]) ) self.assertIn("WELCOME: RELAY VERBATIM", out) - self.assertIn("operator lock is held by another live worktree", out) - self.assertIn("operator lock held elsewhere", out) - self.assertNotIn( - "Use the matching scripts/agent-role.py claim command.", out - ) - self.assertIn( - "If the contributor chooses operator, report the conflict", out - ) + self.assertIn("other coordinators: 2 recorded", out) + # every role stays on the table, and nothing tells the agent to refuse + self.assertIn("Use the matching scripts/agent-role.py claim command.", out) + self.assertNotIn("BLOCKED", out) + self.assertNotIn("report the conflict", out) + + def test_no_peers_renders_no_coordinator_line(self): + # The other side of the same key: a hardcoded line would announce + # coordinators that do not exist, which is how a record reads as a lock. + self.assertNotIn("other coordinators", self.render(state())) def test_environment_reports_status_only_and_never_secret_values(self): secret = "TOP-SECRET-TOKEN" From c3382fb2fc200ccc4d55f2ab85d0ec8879bd425c Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 10 Aug 2026 17:13:43 +0000 Subject: [PATCH 3/4] fix(role): re-claim replaces its record, and the publish is pinned (#285) Review FAIL on 5a5c2225 returned four findings. No redesign: the record representation, the removed refusal path and the never-writing `show` all stand. 1. `.agents/NOW.md` still said "roles are a lock or worktree+PR" while the file it links to says `claim operator` never refuses. NOW.md is the live snapshot and boot step 3, so it is EDITED, not annotated as superseded. The file sat at exactly its 6,000-character budget, so two words elsewhere in the same paragraph were shortened to pay for the longer phrase. 2. `cmd_claim` called `drop_our_record()` immediately before `write_our_record()`. On a RE-claim that unlinked our own `record_path()` and only then re-created it -- an unlink-then-create where the design promises replace. A process killed inside the window leaves an operator marker with no record, the one state that still refuses to resolve and the exact failure this change exists to remove. The drop is now scoped with `keep_canonical` to the files that are NOT our canonical path (in practice the pre-#285 single-file lock), and `write_our_record`'s `os.replace` does the replacement. The legacy-heal test still pins the behaviour that motivated the call. 3. The atomic publish was pinned by nothing: replacing temp + `os.replace` with an in-place, byte-at-a-time flushing write left all three suites green. `test_a_claim_never_rewrites_another_worktrees_record` pins DISJOINT PATHS only. Two tests now pin the mechanism -- a hardlink taken before a publish must still read the OLD bytes afterwards (an in-place rewrite reaches through it, which is how a reader sees half a record), and a publish must leave no non-`.json` residue. 4. Both defensive branches added by 5a5c2225 were unreachable by any test. One test now drops a corrupt `*.json`, a record with a non-numeric heartbeat and a stray `.tmp` into the records directory and requires `show` to resolve, exit 0, list neither as a coordinator, and write nothing. Also: `agent-onboard.py --probe --json` grew from one bool to whole peer records, so the emitted peer field set is pinned through the CLI. DECLINED, with the reason recorded: `prune_stale_records()` remains read-then-unlink against a PATH rather than the inode it read, so a peer silent past the TTL that republishes inside the window loses that record. The remedy is `claim operator`, which is never refused and which every session runs at the top of its next call, so the cost is one display cycle; re-reading before the unlink would narrow the window without closing it and add a branch no test can reach. The race is documented in the function instead. ## Gates test_agent_role 54 -> 58, test_agent_onboard 38 -> 39, test_agent_start 20 unchanged. `scripts/agent-preflight.sh --quiet` reports the same single failure before and after -- `role-undeclared`, this session's own state, not the tree's. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [Claude Code] --- .agents/NOW.md | 6 +- scripts/agent-role.py | 49 ++++++++++--- tests/scripts/test_agent_onboard.py | 25 +++++++ tests/scripts/test_agent_role.py | 106 ++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 13 deletions(-) diff --git a/.agents/NOW.md b/.agents/NOW.md index 4c3239905..fcd4600ff 100644 --- a/.agents/NOW.md +++ b/.agents/NOW.md @@ -63,9 +63,9 @@ latency/memory on every axis, both gate models, reproduced 2–3x idle. See DECISION); record-era rollover BLOCKED on `DONE` rows bound to `parity-ledger.md` LINE anchors (re-anchor by ROW ID). -**Operator/helper protocol** ([spec](workflow.md)): roles are a lock or -worktree+PR; helpers claim `row/` with a DRAFT PR. Role/entrypoint gates -ENFORCE `agent-start.py` → claim → preflight. Review FAIL loops through a fresh +**Operator/helper protocol** ([spec](workflow.md)): roles are a coordinator +record or worktree+PR; helpers claim `row/` with a DRAFT PR. Role gates +ENFORCE `agent-start.py` → claim → preflight. Review FAIL loops to a fresh implementer until PASS. Queue: 10 rows; backfill 79, 30 anchored. **Upstream inventory** ([spec](specs/upstream-derived-inventory-2026-08-05.md)): SM060/061/070 below vLLM's floor = OUT-OF-SCOPE; COMP-*/DISTRIBUTED-* are REAL diff --git a/scripts/agent-role.py b/scripts/agent-role.py index a01c78221..7a96b3659 100755 --- a/scripts/agent-role.py +++ b/scripts/agent-role.py @@ -262,21 +262,45 @@ def write_our_record(claimed_at: float | None = None) -> dict: return record -def drop_our_record() -> bool: - """Remove THIS worktree's record -- including a legacy file it owns.""" +def drop_our_record(keep_canonical: bool = False) -> bool: + """Remove THIS worktree's record -- including a legacy file it owns. + + `keep_canonical` leaves our own `record_path()` alone and drops only the + other files this worktree owns (in practice the pre-#285 single-file lock). + The claim path needs it: unlinking our canonical record and re-creating it + is an unlink-then-create where the design promises replace, and a session + killed inside that window leaves an operator marker with NO record -- the + exact state issue #285 exists to remove, and the one state that still + refuses to resolve. `write_our_record`'s `os.replace` publishes over the + path without ever removing it, so nothing has to be unlinked first. + """ + canonical = record_path() if keep_canonical else None removed = False for record in read_records(): - if record_is_ours(record): - try: - Path(record["path"]).unlink(missing_ok=True) - removed = True - except OSError: - pass + if not record_is_ours(record): + continue + path = Path(record["path"]) + if canonical is not None and path == canonical: + continue + try: + path.unlink(missing_ok=True) + removed = True + except OSError: + pass return removed def prune_stale_records() -> list[dict]: - """Drop records past the TTL. Called from `claim`, which already writes.""" + """Drop records past the TTL. Called from `claim`, which already writes. + + This targets a PATH, not the inode it read, so a peer that was silent for + more than the TTL and republishes inside this window loses the record it + just wrote. Left as is deliberately: the loser's remedy is `claim operator`, + which is never refused and which every session runs at the top of its next + tool call, so the cost is one display cycle. Re-reading before the unlink + would narrow the window without closing it and would add a branch no test + can reach. + """ pruned = [] for record in read_records(): if record_is_stale(record): @@ -430,7 +454,12 @@ def cmd_claim(args: argparse.Namespace) -> int: # its owner is alive disappears from everyone else's display. It also # replaces a legacy single-file lock this worktree owned, so that file # cannot linger and be counted twice. - drop_our_record() + # + # keep_canonical: only the legacy file is unlinked. Our own record is + # REPLACED by the publish below and never removed first, so a re-claim + # has no window in which this worktree has an operator marker and no + # record. + drop_our_record(keep_canonical=True) write_our_record() else: # Downgrading OUT of the operator role must not orphan the record. diff --git a/tests/scripts/test_agent_onboard.py b/tests/scripts/test_agent_onboard.py index 3196cf5f6..73a99cd46 100644 --- a/tests/scripts/test_agent_onboard.py +++ b/tests/scripts/test_agent_onboard.py @@ -11,6 +11,7 @@ import contextlib import importlib.util import io +import json import os import re import shutil @@ -144,6 +145,7 @@ def test_probe_never_exits_nonzero(self): ROLE_SCRIPT = ROOT / "scripts/agent-role.py" +ONBOARD_SCRIPT = ROOT / "scripts/agent-onboard.py" class ProbeFieldsComeFromResolve(unittest.TestCase): @@ -286,6 +288,29 @@ def test_probe_reports_a_vanished_record_as_re_claimable(self) -> None: "operator marker without a coordinator record; re-claim") self.assertEqual(state["operator_peers"], []) + def test_probe_json_peer_records_carry_exactly_these_fields(self) -> None: + # `--probe --json` used to emit one bool for this. It now emits whole + # peer records, and nothing pinned what may appear in them. This is the + # machine-readable front door .agents/workflow.md sends sessions to, so + # the surface is fixed here: a new field in a coordinator record has to + # be a deliberate edit of this list, not a silent widening. Run through + # the CLI, because the emitted JSON is the surface, not probe()'s dict. + self.assertEqual(self.claim(self.repo, "a", "operator").returncode, 0) + rival = self.worktree("shape-rival") + self.assertEqual(self.claim(rival, "rival-session", "operator").returncode, 0) + + emitted = subprocess.run( + [sys.executable, str(ONBOARD_SCRIPT), "--probe", "--json"], + cwd=self.repo, env=dict(os.environ, VLLM_CPP_AGENT_SESSION="a"), + capture_output=True, text=True) + self.assertEqual(emitted.returncode, 0, emitted.stderr) + state = json.loads(emitted.stdout) + + self.assertEqual(len(state["operator_peers"]), 1, state["operator_peers"]) + self.assertEqual( + sorted(state["operator_peers"][0]), + ["claimed_at", "heartbeat", "host", "path", "pid", "session", "worktree"]) + def test_probe_reports_the_env_state_it_was_given(self) -> None: # Kills `env: "present"`. It cannot be pinned against the real tree -- # a developer with a complete .env would make the hardcode true -- so diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index c81539477..f7da2cec8 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -10,6 +10,7 @@ from __future__ import annotations +import argparse import concurrent.futures import importlib.util import json @@ -522,6 +523,111 @@ def test_a_record_is_never_written_inside_a_work_tree(self) -> None: "") +class RecordPublishAndBadInput(_TempRepo, unittest.TestCase): + """How a record reaches its own path, and what a bad file in the directory + does to `show`. + + `test_a_claim_never_rewrites_another_worktrees_record` pins the DISJOINT + PATHS half of the concurrency argument: it fails on shared-path symptoms and + says nothing about the publish itself. A fresh review (2026-08-10) replaced + `write temp + os.replace` with an in-place, byte-at-a-time flushing write -- + deliberately torn publishes -- and all three suites stayed green, so the + mechanism the module docstring calls atomic was pinned by nothing. The same + review found the re-claim path unlinking its own record before rewriting it, + and both defensive branches (`record_is_stale`'s unreadable heartbeat, + `read_records`'s suffix filter) reachable by no test at all. + """ + + def test_a_publish_replaces_the_record_and_never_rewrites_it_in_place(self) -> None: + # A hardlink is the inode a concurrent reader is holding. `os.replace` + # publishes a NEW inode over the name, so the reader keeps seeing whole + # old bytes; any in-place rewrite reaches through the link, which is + # exactly how a reader gets half a record. + run_role(self.repo, "a", "claim", "operator") + published = self.record_files()[0] + before = published.read_bytes() + witness = published.with_name("witness-hardlink") + os.link(published, witness) + + beat = run_role(self.repo, "a", "heartbeat") + self.assertEqual(beat.returncode, 0, beat.stderr) + self.assertNotEqual(published.read_bytes(), before, "nothing was published") + self.assertEqual( + witness.read_bytes(), before, + "the record was rewritten IN PLACE: a reader holding the previous " + "inode sees the new bytes, so it can observe a partial record") + + def test_a_publish_leaves_no_temporary_file_behind(self) -> None: + # The other half of rename-publish: the temp file is CONSUMED by the + # rename. A publish that copies instead leaves it, and a leftover temp + # is a record-shaped file nobody owns. + run_role(self.repo, "a", "claim", "operator") + run_role(self.repo, "a", "heartbeat") + run_role(self.repo, "a", "claim", "operator") + residue = sorted(entry.name for entry in self.records_dir().iterdir() + if entry.suffix != ".json") + self.assertEqual(residue, [], f"publish residue left behind: {residue}") + + def test_a_reclaim_never_unlinks_its_own_record(self) -> None: + # Re-claim must REPLACE, never unlink-then-create. With the publish + # killed mid-flight, the record from the previous claim has to survive + # byte-for-byte -- an operator marker with no record is the one state + # that still refuses to resolve, and it is what this change exists to + # remove. + run_role(self.repo, "a", "claim", "operator") + before = self.record_files()[0].read_bytes() + + saved = os.getcwd() + os.chdir(self.repo) + try: + with mock.patch.object(role, "write_our_record", + side_effect=RuntimeError("killed mid-publish")): + with self.assertRaises(RuntimeError): + role.cmd_claim(argparse.Namespace( + role="operator", row=None, headless=False)) + finally: + os.chdir(saved) + + self.assertEqual( + len(self.record_files()), 1, + "the re-claim unlinked this worktree's own record before " + f"republishing it: {self.records()}") + self.assertEqual(self.record_files()[0].read_bytes(), before) + + def test_show_survives_a_corrupt_record_a_bad_heartbeat_and_a_stray_temp(self) -> None: + # Nothing exercised either defensive branch. A record whose heartbeat is + # unreadable must read as STALE, not raise out of `show`; a `.tmp` file + # caught mid-publish must not parse as a coordinator. And `show` must + # still write nothing: agent-preflight.sh documents itself as never + # writing, and it calls exactly this path. + run_role(self.repo, "a", "claim", "operator") + directory = self.records_dir() + (directory / "corrupt.json").write_text("{ not json at all", encoding="utf-8") + (directory / "bad-heartbeat.json").write_text(json.dumps({ + "session": "bad-beat-session", "worktree": "/nowhere/.git", + "claimed_at": "not-a-number", "heartbeat": "not-a-number", + "host": "somewhere", "pid": 4321}), encoding="utf-8") + (directory / ".half-published.999.tmp").write_text(json.dumps({ + "session": "stray-temp-session", "worktree": "/torn/.git", + "claimed_at": time.time(), "heartbeat": time.time(), + "host": "somewhere", "pid": 999}), encoding="utf-8") + before = sorted((entry.name, entry.read_bytes()) for entry in directory.iterdir()) + + shown = run_role(self.repo, "a", "show") + + self.assertEqual(shown.returncode, 0, shown.stdout + shown.stderr) + self.assertIn("role=operator", shown.stdout) + self.assertNotIn("Traceback", shown.stderr) + # Neither bad file may become a coordinator: one is past no TTL it can + # state, the other is a temp file mid-publish. + self.assertNotIn("other coordinators", shown.stdout) + self.assertNotIn("bad-beat-session", shown.stdout) + self.assertNotIn("stray-temp-session", shown.stdout) + self.assertEqual( + sorted((entry.name, entry.read_bytes()) for entry in directory.iterdir()), + before, "`show` wrote to the records directory") + + class RoleDiscipline(unittest.TestCase): def test_feature_path_classification(self) -> None: for path in ("src/vllm/a.cpp", "include/vt/b.h", "tests/vt/c.cpp", From 8d0341822f9f6e4957b26d0290fd854fe52faed5 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 10 Aug 2026 17:51:42 +0000 Subject: [PATCH 4/4] fix(role): the claim-path prune stops eating this worktree's own record (#285) Third review round on the operator record. Two findings, and the first falsifies the exact invariant this change exists to establish. 1. MEDIUM. `c3382fb2` scoped `drop_our_record` so a re-claim replaces its record instead of unlinking it, and its comment stated the invariant that follows: "only the legacy file is unlinked. Our own record is REPLACED by the publish below and never removed first, so a re-claim has no window in which this worktree has an operator marker and no record." All three clauses are false whenever this worktree's own record is stale, because `prune_stale_records()` twelve lines above unlinks ANY stale record -- ours included -- before either `drop_our_record` or `write_our_record` runs. Measured on `c3382fb2` with the publish killed mid-flight: === FRESH own record === before re-claim: exit=0 role=operator records surviving = ['784c212ece810bf8.json'] after killed re-claim: exit=0 role=operator === STALE own record === before re-claim: exit=0 role=operator records surviving = [] after killed re-claim: exit=3 role=UNDECLARED note: this worktree's coordinator record is gone; re-run `claim operator` The re-claim CREATES the refusal out of a state that resolved fine a moment earlier -- `resolve` matches our own record by ownership with no staleness filter -- and the stale leg is the COMMON one: the TTL is two hours and every session re-claims at the top of its next tool call. The kill window is narrow; the path through it is not. The prune now takes the same `keep_canonical` scoping as `drop_our_record`. Skipping our own path is safe because `write_our_record` republishes it immediately, and it leaks nothing, because an aged own record was never displayed as a live coordinator in the first place. Both comments now describe what the function does, and the DECLINED path-vs-inode race is restated as covering PEERS only, which is all it now reaches. `test_a_reclaim_never_unlinks_its_own_record` runs both ages of record. One backdate past the TTL is the whole difference, and it is what the round-2 test missed. It also asserts `show` exits 0 both before and after the killed re-claim, so the test fails on the manufactured refusal and not merely on a missing file. 2. LOW, a coverage residual rather than a defect: the shipped publish is correct. Review mutation MINE-B replaced temp + `os.replace` with `target.unlink(missing_ok=True); target.write_text(...)` and all 117 tests stayed green. It creates a new inode, so the hardlink witness still reads the old bytes, and it leaves no temp, so the residue test passes -- but the NAME is transiently absent, which is neither the old record nor the new one, and it drops a concurrent `show` into the same exit-3 refusal as finding 1. The name is now watched directly: during a publish over an existing record, nothing may unlink the published path, and at the instant any content is written that path must already resolve. Polling for the window would be a race, so the publish is observed from inside instead. Also addressed: `--probe --json` pinned its peer field set against a non-legacy peer only, so a pre-#285 record could widen the emitted shape by one `legacy` key without the test noticing. That leg is now pinned too; it retires with `LEGACY_RECORD_NAME`. ## Gates test_agent_role 58 -> 59, test_agent_onboard 39, test_agent_start 20; 117 -> 118 combined, all green. `scripts/agent-preflight.sh --quiet --no-require-role`: all gates green, before and after. Without the flag the same single failure stands before and after -- `role-undeclared`, this session's own state, not the tree's. Mutation, each applied to a scratch copy and restored green afterwards: `prune_stale_records(keep_canonical=True)` -> `prune_stale_records()` and the in-function skip deleted both take the stale leg RED (0 records survive, 1 expected) while the fresh leg stays green; MINE-B takes the new publish test RED on the unlink assertion; a rename-away-then-create variant that touches no unlink at all takes it RED on the absence assertion; dropping the `legacy` tag from `read_records` and adding a field to every record each take the probe field-set test RED. Rebased onto `f323907e`. The `#285` intake row survived a conflict with `#287` and both are present. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [Claude Code] --- .agents/specs/operator-record.md | 38 +++++++-- scripts/agent-role.py | 48 ++++++++--- tests/scripts/test_agent_onboard.py | 28 +++++++ tests/scripts/test_agent_role.py | 126 +++++++++++++++++++++++++--- 4 files changed, 209 insertions(+), 31 deletions(-) diff --git a/.agents/specs/operator-record.md b/.agents/specs/operator-record.md index 5bbe22905..f532305a8 100644 --- a/.agents/specs/operator-record.md +++ b/.agents/specs/operator-record.md @@ -71,11 +71,22 @@ unchanged from the 2026-08-06 correction, and `record_is_ours` keeps the legacy session fallback for a record written before that correction. **Staleness.** `RECORD_TTL_SECONDS` keeps the 2-hour value and the heartbeat -semantics. A stale record is filtered out of every display and unlinked on the -next `claim`, which is a write path; `show` and `resolve` never unlink, because -`agent-preflight.sh` documents itself as never writing anything. A stale record -can no longer refuse anybody, so breaking one is no longer an event: the NOTE -that announced it goes with the refusal it explained. +semantics. Another worktree's stale record is filtered out of every display and +unlinked on the next `claim`, which is a write path; `show` and `resolve` never +unlink, because `agent-preflight.sh` documents itself as never writing anything. +A stale record can no longer refuse anybody, so breaking one is no longer an +event: the NOTE that announced it goes with the refusal it explained. + +The prune skips THIS worktree's own record (`keep_canonical`, third review +round). Our own record is stale on any ordinary re-claim — the TTL is two hours +and a session re-claims at the top of its next tool call — and pruning it before +republishing it is the same unlink-then-create the second round removed from +`drop_our_record`, only reached by the commoner path. It costs nothing to skip: +`write_our_record` republishes that exact path immediately, and `resolve` matches +our own record by ownership with no staleness filter, so an aged own record was +never displayed as a live coordinator in the first place. The consequence of not +skipping is measured, not theoretical: a session killed inside the window turned +a worktree that resolved `role=operator` into `role=UNDECLARED` exit 3. **Migration.** A pre-#285 `vllm-cpp-operator.lock` is read as one more record, so a session that claimed operator before this change still resolves as operator @@ -156,3 +167,20 @@ worktree, session, host and heartbeat age; `release` removes only the caller's record; a stale record is pruned from the display and blocks nothing. The refusal path and its `already held` message are gone from the tool, the two front doors, and the documents. + +Three review rounds. Round 2 scoped `drop_our_record` so a re-claim replaces its +record instead of unlinking it, and pinned the atomic publish. Round 3 found the +same window still open through `prune_stale_records`, which the round-2 comment +had asserted was closed: the prune runs first and unlinks any stale record, +including ours. It is now scoped the same way, and +`test_a_reclaim_never_unlinks_its_own_record` runs both ages of record because +only the fresh one had been covered. Round 3 also pinned what the publish tests +missed — a publish that unlinks the name and recreates it leaves the hardlink +witness and the residue check both green, so the NAME is now watched directly — +and pinned the `legacy` field a pre-#285 peer adds to `--probe --json`. + +Still declined, unchanged: `prune_stale_records` targets a path rather than the +inode it read, so a PEER that republishes inside the window loses that record. +Its remedy is `claim operator`, which is never refused. With `keep_canonical` +that declination is now confined to peers; this worktree's own record is no +longer exposed to it. diff --git a/scripts/agent-role.py b/scripts/agent-role.py index 7a96b3659..2df4249ab 100755 --- a/scripts/agent-role.py +++ b/scripts/agent-role.py @@ -290,19 +290,33 @@ def drop_our_record(keep_canonical: bool = False) -> bool: return removed -def prune_stale_records() -> list[dict]: +def prune_stale_records(keep_canonical: bool = False) -> list[dict]: """Drop records past the TTL. Called from `claim`, which already writes. - This targets a PATH, not the inode it read, so a peer that was silent for - more than the TTL and republishes inside this window loses the record it - just wrote. Left as is deliberately: the loser's remedy is `claim operator`, - which is never refused and which every session runs at the top of its next - tool call, so the cost is one display cycle. Re-reading before the unlink - would narrow the window without closing it and would add a branch no test - can reach. + `keep_canonical` skips THIS worktree's own `record_path()`. The claim path + needs it, and only the claim path calls this: our own record is stale on + every ordinary re-claim -- the TTL is two hours and a session re-claims at + the top of its next tool call -- and pruning it is an unlink-then-create + that this path has already been fixed once to avoid (see `drop_our_record`). + It is safe because `write_our_record` republishes that exact path + immediately afterwards, and it is not a leak: `resolve` matches our own + record by ownership with no staleness filter, so an aged own record was + never being displayed as a live coordinator anyway. + + For every OTHER record this targets a PATH, not the inode it read, so a peer + that was silent for more than the TTL and republishes inside this window + loses the record it just wrote. Left as is deliberately: the loser's remedy + is `claim operator`, which is never refused and which every session runs at + the top of its next tool call, so the cost is one display cycle. Re-reading + before the unlink would narrow the window without closing it and would add a + branch no test can reach. That declination covers peers ONLY -- our own + record is not exposed to it, because `keep_canonical` never unlinks it. """ + canonical = record_path() if keep_canonical else None pruned = [] for record in read_records(): + if canonical is not None and Path(record["path"]) == canonical: + continue if record_is_stale(record): try: Path(record["path"]).unlink(missing_ok=True) @@ -447,7 +461,13 @@ def cmd_claim(args: argparse.Namespace) -> int: # died mid-flight neither lingers in the display nor is reported as a # live coordinator. This is a write path, which is why pruning happens # here and never in `show`. - prune_stale_records() + # + # keep_canonical: the prune is about OTHER sessions. Our own record is + # stale on any ordinary re-claim, and unlinking it here would put back + # exactly the window `drop_our_record(keep_canonical=True)` closes + # below -- an operator marker with no record, manufactured out of a + # state that resolved fine a moment earlier. + prune_stale_records(keep_canonical=True) peers = peer_records() # Rewriting our own record is the renewal path too: a live coordinator # re-claims more often than it beats, and a record that ages out while @@ -455,10 +475,12 @@ def cmd_claim(args: argparse.Namespace) -> int: # replaces a legacy single-file lock this worktree owned, so that file # cannot linger and be counted twice. # - # keep_canonical: only the legacy file is unlinked. Our own record is - # REPLACED by the publish below and never removed first, so a re-claim - # has no window in which this worktree has an operator marker and no - # record. + # keep_canonical: our own `record_path()` is left alone here, so only + # the other files this worktree owns (in practice the pre-#285 legacy + # lock) are unlinked. Together with the scoped prune above, nothing on + # this path removes our record: it is REPLACED by the publish below, so + # a re-claim has no window in which this worktree has an operator marker + # and no record -- at any age of the record it started from. drop_our_record(keep_canonical=True) write_our_record() else: diff --git a/tests/scripts/test_agent_onboard.py b/tests/scripts/test_agent_onboard.py index 73a99cd46..470ad81f8 100644 --- a/tests/scripts/test_agent_onboard.py +++ b/tests/scripts/test_agent_onboard.py @@ -18,6 +18,7 @@ import subprocess import sys import tempfile +import time import types import unittest from pathlib import Path @@ -311,6 +312,33 @@ def test_probe_json_peer_records_carry_exactly_these_fields(self) -> None: sorted(state["operator_peers"][0]), ["claimed_at", "heartbeat", "host", "path", "pid", "session", "worktree"]) + # A pre-#285 peer widens that set by one: `read_records` tags the legacy + # single-file lock with `legacy` on the way through, and it reaches this + # surface like any other peer. Pinned rather than left implicit, because + # the untagged case above passes either way. This leg goes away with + # LEGACY_RECORD_NAME, which agent-role.py already schedules for deletion. + common = subprocess.check_output( + ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], + cwd=self.repo, text=True).strip() + (Path(common) / onboard.role_mod.LEGACY_RECORD_NAME).write_text(json.dumps({ + "session": "legacy-session", "worktree": "/elsewhere/.git", + "claimed_at": time.time(), "heartbeat": time.time(), + "host": "somewhere", "pid": 77}), encoding="utf-8") + + emitted = subprocess.run( + [sys.executable, str(ONBOARD_SCRIPT), "--probe", "--json"], + cwd=self.repo, env=dict(os.environ, VLLM_CPP_AGENT_SESSION="a"), + capture_output=True, text=True) + self.assertEqual(emitted.returncode, 0, emitted.stderr) + peers = {record["session"]: sorted(record) + for record in json.loads(emitted.stdout)["operator_peers"]} + self.assertEqual( + peers, + {"rival-session": ["claimed_at", "heartbeat", "host", "path", "pid", + "session", "worktree"], + "legacy-session": ["claimed_at", "heartbeat", "host", "legacy", + "path", "pid", "session", "worktree"]}) + def test_probe_reports_the_env_state_it_was_given(self) -> None: # Kills `env: "present"`. It cannot be pinned against the real tree -- # a developer with a complete .env would make the hardcode true -- so diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index f7da2cec8..eeff2defd 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -536,6 +536,14 @@ class RecordPublishAndBadInput(_TempRepo, unittest.TestCase): review found the re-claim path unlinking its own record before rewriting it, and both defensive branches (`record_is_stale`'s unreadable heartbeat, `read_records`'s suffix filter) reachable by no test at all. + + A third round then showed both of those repairs still short. The re-claim + test only ever started from a FRESH record, so it never reached the prune + that runs first and unlinked our own record once it was stale -- the common + case, since the TTL is two hours. And both publish tests survive + `unlink(target); target.write_text(new)`: a new inode leaves the hardlink + witness intact and no temp is left, but the NAME is transiently absent, + which is neither the old record nor the new one. """ def test_a_publish_replaces_the_record_and_never_rewrites_it_in_place(self) -> None: @@ -568,15 +576,10 @@ def test_a_publish_leaves_no_temporary_file_behind(self) -> None: if entry.suffix != ".json") self.assertEqual(residue, [], f"publish residue left behind: {residue}") - def test_a_reclaim_never_unlinks_its_own_record(self) -> None: - # Re-claim must REPLACE, never unlink-then-create. With the publish - # killed mid-flight, the record from the previous claim has to survive - # byte-for-byte -- an operator marker with no record is the one state - # that still refuses to resolve, and it is what this change exists to - # remove. - run_role(self.repo, "a", "claim", "operator") - before = self.record_files()[0].read_bytes() - + def _killed_reclaim(self) -> None: + """`claim operator` whose publish dies mid-flight. What survives is the + point: whatever is on disk at that instant is what a killed session + leaves behind.""" saved = os.getcwd() os.chdir(self.repo) try: @@ -588,11 +591,108 @@ def test_a_reclaim_never_unlinks_its_own_record(self) -> None: finally: os.chdir(saved) + def test_a_publish_never_leaves_the_record_NAME_absent(self) -> None: + # The two tests above both survive `target.unlink(); target.write_text()` + # (review mutation MINE-B, 2026-08-10): a fresh inode leaves the hardlink + # witness reading the old bytes, and no temp file is left behind. What + # that publish does do is make the NAME transiently absent, which is + # neither "the old record" nor "the new one" -- a `show` landing in the + # window reports an operator marker with no record and exits 3. + # + # So the NAME is watched rather than the bytes. Polling for the window + # would be a race; instead the publish is observed from inside: at the + # instant any file content is written, the published path must already + # resolve, and nothing may unlink it. Both hold for temp + os.replace, + # and neither holds for unlink-then-create. The record has to exist + # first -- "old or new, never absent" says nothing about the first + # publish, which has no old. + run_role(self.repo, "a", "claim", "operator") + + absent_when_writing: list[str] = [] + unlinked: list[str] = [] + real_write_text = Path.write_text + real_path_unlink = Path.unlink + real_os_unlink = os.unlink + real_os_remove = os.remove + + saved = os.getcwd() + os.chdir(self.repo) + try: + target = role.record_path() + + def watched_write_text(path, *args, **kwargs): + if not target.exists(): + absent_when_writing.append(str(path)) + return real_write_text(path, *args, **kwargs) + + def watched_path_unlink(path, *args, **kwargs): + if Path(path) == target: + unlinked.append(str(path)) + return real_path_unlink(path, *args, **kwargs) + + def watched_os_unlink(path, *args, **kwargs): + if Path(path) == target: + unlinked.append(str(path)) + return real_os_unlink(path, *args, **kwargs) + + def watched_os_remove(path, *args, **kwargs): + if Path(path) == target: + unlinked.append(str(path)) + return real_os_remove(path, *args, **kwargs) + + with mock.patch.object(Path, "write_text", watched_write_text), \ + mock.patch.object(Path, "unlink", watched_path_unlink), \ + mock.patch.object(os, "unlink", watched_os_unlink), \ + mock.patch.object(os, "remove", watched_os_remove): + role.write_our_record() + finally: + os.chdir(saved) + self.assertEqual( - len(self.record_files()), 1, - "the re-claim unlinked this worktree's own record before " - f"republishing it: {self.records()}") - self.assertEqual(self.record_files()[0].read_bytes(), before) + unlinked, [], + "the publish UNLINKED the record name; a reader in that window sees " + "an operator marker with no record, not the old record") + self.assertEqual( + absent_when_writing, [], + "the new bytes were written while the record name did not exist, so " + f"the name was transiently absent: {absent_when_writing}") + + def test_a_reclaim_never_unlinks_its_own_record(self) -> None: + # Re-claim must REPLACE, never unlink-then-create. With the publish + # killed mid-flight, the record from the previous claim has to survive + # byte-for-byte -- an operator marker with no record is the one state + # that still refuses to resolve, and it is what this change exists to + # remove. + # + # Both ages, because they take DIFFERENT code paths and only the fresh + # one was covered: `cmd_claim` prunes before it publishes, and a prune + # that is not scoped to skip our own path unlinks our record whenever it + # is the stale one. Stale is the COMMON case -- the TTL is two hours and + # a session re-claims at the top of its next tool call -- and `resolve` + # matches our own record with no staleness filter, so the state RESOLVES + # FINE until the re-claim destroys it. One backdate is the whole + # difference between the two legs. + for age, backdate_by in (("fresh", None), + ("stale", role.RECORD_TTL_SECONDS + 60)): + with self.subTest(own_record=age): + run_role(self.repo, "a", "claim", "operator") + if backdate_by is not None: + self.backdate(self.repo, backdate_by) + before = self.record_files()[0].read_bytes() + # Whatever its age, this worktree resolves BEFORE the re-claim. + # So any refusal afterwards was manufactured by the re-claim. + self.assertEqual(run_role(self.repo, "a", "show").returncode, 0) + + self._killed_reclaim() + + self.assertEqual( + len(self.record_files()), 1, + "the re-claim unlinked this worktree's own record before " + f"republishing it: {self.records()}") + self.assertEqual(self.record_files()[0].read_bytes(), before) + # The state the whole change exists to remove: an operator + # marker with no record, created out of one that resolved. + self.assertEqual(run_role(self.repo, "a", "show").returncode, 0) def test_show_survives_a_corrupt_record_a_bad_heartbeat_and_a_stray_temp(self) -> None: # Nothing exercised either defensive branch. A record whose heartbeat is