feat(role): the operator lock becomes a record of who coordinates where (#285) - #295
Merged
Conversation
…re (#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 <git-common-dir>/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]
…re (#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, `<git-common-dir>/vllm-cpp-operators/<sha256(worktree) [:16]>.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]
) 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]
…rd (#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]
localai-bot
pushed a commit
that referenced
this pull request
Aug 10, 2026
Concurrent-landing reconciliation only: main advanced with the operator-record work (#285, #295) while this row's gates were running. No conflicts; all doc ratchets and preflight re-verified green on the combined tree before the push. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
bakon11
pushed a commit
to bakon11/vllm.cpp
that referenced
this pull request
Aug 10, 2026
…scapable publish pin Both were recorded as LOW by mudler#295's final review rather than blocked. Neither changes what `agent-role.py` does; the shipped publish is unchanged. 1. `RECORD_TTL_SECONDS`' comment still said a stale record is "unlinked by the next `claim`". `keep_canonical` made that false for THIS worktree's record -- it is REPLACED, never unlinked, which is the whole point of the fix. The byte-identical sentence in the spec was corrected when that landed and the source copy, one screen above the function it describes, was missed. It now carries the same qualification, and a tree-wide search found no third copy. 2. `test_a_publish_never_leaves_the_record_NAME_absent` watched four I/O primitives, so a publish written `os.rename(target, aside)` then `open(target, "w")` escaped it. MEASURED: the pre-mudler#296 suite is 59/59 GREEN under exactly that mutation, while a concurrent `show` in the window returns rc=3 role=UNDECLARED. That is the third consecutive round in which one publish-coverage residual was fixed and the next one opened, and the reason is structural: ANY monkeypatch-watcher pin is escapable by one more primitive, so widening the list chases a fixed point. The pin is now two tests that fail differently. `_watch_publish` (deterministic, in-process) additionally watches `os.rename`/`os.replace` and `Path.rename`/`Path.replace` as SOURCE -- the shipped `os.replace(temp, target)` has the record as DESTINATION and never removes the name -- plus `builtins.open`/`io.open`/`os.open` in write modes. `test_a_concurrent_observer_never_sees_the_record_name_absent` (probabilistic, out-of-process) republishes the record 200 times in a subprocess while the test process does nothing but poll `exists()`. It enumerates no primitive, so it is the only half that can catch one nobody listed. It is ONE-SIDED -- a hit proves absence, a miss proves nothing -- so it cannot go red on correct code however the two processes are scheduled; `os.replace` never lets the name stop resolving. Contention costs detection probability, never a red run. MEASURED 2026-08-10, 20-core box already at load average 263, ~1.5ms per publish against ~1us per poll, 150k-300k polls per run: publish under test watcher observer os.rename + open (documented escape) RED 10/10 (and 10/10 at 60) unlink + write_text (prior escape) RED 10/10 byte-at-a-time in-place rewrite green* 0/6 shipped write temp + os.replace green 40/40 green, 0 absent shipped, both processes on ONE core green 20/20 green os.rename + open, on ONE core RED 1/15 * caught instead by test_a_publish_replaces_the_record_and_never_rewrites_it_ in_place, whose hardlink witness is that mutation's subject. The two single-core rows are why BOTH halves ship rather than either. The observer needs real parallelism -- pinned to one core the publisher is rarely preempted inside a window lasting microseconds -- and is blind by construction to a publish that never makes the name absent. The watcher is unaffected by scheduling but covers only what it names. Neither subsumes the other. Sizing: 200 publishes is ~0.39s, chosen for margin over the 60 that already detected 10/10. The only non-assertion guard is a floor of 200 polls, ~0.1% of the measured count, whose sole job is to notice a poll loop that never ran. The test's stated claim was weakened to match what it proves: it no longer claims "at the instant ANY file content is written ... NOTHING may unlink it", but bounds itself to the primitives `_watch_publish` names and points at the observer for the rest. Gates: test_agent_role 60 (was 59, +1), test_agent_onboard 39, test_agent_start 20, all green; `scripts/agent-preflight.sh --quiet` green before and after (with --no-require-role: this session declared no role, so the role gate would report its 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]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #285. User-directed.
The repo-wide exclusive operator lock becomes a record of who coordinates
where. It never refuses.
Why the exclusivity was wrong
The lock's own rationale was "the shared case is the operator's primary checkout,
where one role is the correct answer anyway." That premise no longer holds: every
task takes its own worktree and lands by merge, so there is no shared checkout to
protect. An operator is a coordinator — its maximum powers are merging PRs and
dispatching sub-agents into worktrees. It never rewrites shared history, so a plain
git pushrefuses any non-fast-forward and git itself is the interlock.The exclusivity cost real time: with a 2-hour TTL, a session killed mid-flight — as
one was, by a disk cleanup, leaving a dead pid and a frozen heartbeat — blocked all
coordination for up to two hours, with no remedy but hand-deleting a file in
.git/.Design
<git-common-dir>/vllm-cpp-operators/<sha256(worktree)[:16]>.json, one file perworktree. A writer only ever touches the path derived from its own worktree, so
concurrent claimants use disjoint paths and there is no read-modify-write of a
shared file anywhere — which a JSON array or an append log would have needed for
release and prune. Publish is temp +
os.replacein the same directory.O_CREAT|O_EXCLis gone; its only job was to fail the second claimant.claim operatorprunes stale peers, publishes its own record and exits 0 whoeverelse is recorded.
showprintsother coordinators recorded: Nwith worktree,session, host, pid and heartbeat age.
releaseand downgrade remove only thecaller's record. Ownership still keys on the worktree. The 2-hour TTL is kept — it
was never the problem.
A pre-#285
vllm-cpp-operator.lockis still read as one record so a livepre-change operator does not silently go UNDECLARED, and is healed away on its next
claim.
Policy
AGENTS.md:108and:241now state the rule that motivated all of this:mainis never force-pushed. No--force, no--force-with-lease, by anyone,ever. A rejected push means fetch, re-merge, re-gate, push again. The operator is
defined as a coordinator, several may run concurrently, and boot step 2 says the
claim is never refused.
.agents/workflow.md,.agents/NOW.md,docs/USAGE.mdfollow; three dated design specs carry superseded notes rather than edits.
Three review rounds, findings 4 → 2 → 2 LOW + 1 nit
Round 1 found the live snapshot still asserting the lock, and that the atomic
publish was pinned by nothing — a mutation replacing
os.replacewithbyte-at-a-time in-place writes left all suites green.
Round 2 found the repair's own comment falsified by the function twelve lines
above it:
prune_stale_records()unlinks any stale record, ours included, so are-claim on a stale-but-healthy record manufactured the orphaned-marker state
this change exists to remove —
exit=3 role=UNDECLAREDout of a state thatresolved fine.
Round 3 fixed that with the same scoping
drop_our_recordalready had, and pinnedthe publish NAME. The final reviewer designed four of its own mutations; MINE-4
proved the two test legs are complementary rather than redundant (the fresh leg
catches round 2's repair, the stale leg round 3's, neither carries the other), and
MINE-2 proved the new scoping did not silently disable peer pruning.
keep_canonical=Truewas verified safe rather than argued: with two real worktreesand the owner's record backdated past the TTL, the aged record appears in no
peer list (
show,probe --json,agent-start), and garbage collection survivesthrough two independent paths — a peer's claim still prunes it, and a downgrade
still removes it.
Gates, verified by the operator rather than taken on report
test_agent_role59,test_agent_onboard39,test_agent_start20 =118.
scripts/agent-preflight.sh --quiet --no-require-role: all gatesgreen, including both committed-range gates.
--no-require-rolebecause asession that never claims sees
role-undeclaredas its own state, not the tree's.Rebase conflict in the intake table (
#287and#285both first row) resolved byunion and independently checked: a pure one-line insertion,
#287byte-identicalto main, no duplicate or lost row.
Two known limitations, recorded not fixed
Both LOW, both judged by the final reviewer as not worth another round, and a
follow-up issue tracks them.
scripts/agent-role.py:107-108's TTL comment still says a stale record is"unlinked by the next
claim", which is now false for our own record — it isreplaced. The spec's copy of the same sentence was corrected; the source copy
was missed.
Path.write_textand three unlink primitives, soan
os.rename+open()publish escapes it — demonstrated, with a realconcurrent
showreturning rc 3. The shipped publish is correct; this iscoverage. As the reviewer put it, each round has fixed one publish-coverage
residual and created the next escape; the robust pin is an outside observer, not
a wider watcher list.
FOLLOWING_AGENTS_PROTOCOL