diff --git a/.agents/NOW.md b/.agents/NOW.md index 4c4b0978..abbcc01a 100644 --- a/.agents/NOW.md +++ b/.agents/NOW.md @@ -65,7 +65,7 @@ throughput ⇒ audit the context; per-shape MEASUREMENT arbitrates). into a lock or worktree+PR; operator merges PRs first and does features only via sub-agents; helpers use worktrees on `row/` and open a DRAFT PR at the START, which IS the claim. **W0-W5 LANDED**; role discipline ENFORCING, -`--require-role` still opt-in. Queue: 10 rows — 6 are audit-vacated, with LANDED gate anchors; READ before picking. Backfill: 79 rows, 30 anchored; blocker is claim FAMILIES. +`--require-role` is the DEFAULT. Queue: 10 rows — 6 are audit-vacated, with LANDED gate anchors; READ before picking. Backfill: 79 rows, 30 anchored; blocker is claim FAMILIES. **Upstream inventory** ([spec](specs/upstream-derived-inventory-2026-08-05.md), drift-gated, arch parity BOTH ways): SM060/061/070 below vLLM's floor = OUT-OF-SCOPE; COMP-*/DISTRIBUTED-* are REAL unported work; **all 362 archs now have rows**; llama.cpp's 11 extra devices are IN SCOPE, spike-gated diff --git a/.agents/specs/operator-helper-protocol.md b/.agents/specs/operator-helper-protocol.md index 39c7bff5..28939527 100644 --- a/.agents/specs/operator-helper-protocol.md +++ b/.agents/specs/operator-helper-protocol.md @@ -57,9 +57,30 @@ Acquisition and persistence are different problems and need different mechanisms session ASKS before doing anything else. This is unavoidable: the information does not exist in the environment yet. It is also cheap, once per session. +The question is "what are you here to do?", never "operator or helper?" — the +answer must not require the developer to learn this spec's vocabulary first. The +three answers, and the exact table an agent reads, live in +[`.agents/workflow.md`](../workflow.md#first-question-of-every-session), which +`check-protocol-consistency.py` asserts still carries them. + +**`read-only` is the third answer**, and it is a declared ABSENCE of claim +rather than a third role: it takes no lock and creates no worktree, so a session +that only reads has an honest thing to say. Without it the only ways to satisfy +the gate were the repo-wide operator lock or a throwaway worktree, and faced +with that people reach for `--no-require-role` until the gate means nothing. +Escalating to a real role is one command. + +**The MODE is declared in the same breath.** `--headless` on the claim records +an unattended run in the marker: decide, record each decision in +`.agents/state.md`, never block, never merge, park what will not go green. +It is DECLARED, never inferred — not from the hour, not from silence, not from +the length of the task — and `mode_from_marker` defaults to interactive. + **2. Materialize — make the answer a FACT.** Immediately on answering: -- *operator*: atomically acquire `.agents/operator.lock` (create-exclusive, so a +- *operator*: atomically acquire the operator lock — `vllm-cpp-operator.lock` in + the git COMMON dir, so it is shared by every worktree and can never be + committed (create-exclusive, so a second self-declared operator FAILS rather than racing), carrying session id, host, PID and a heartbeat timestamp; - *helper*: create its worktree and `row/` branch and open the draft PR @@ -68,10 +89,13 @@ does not exist in the environment yet. It is also cheap, once per session. After this step the session IS distinguishable from the outside, which is what the first draft wrongly assumed at the start. -**3. Persist and re-derive — never ask twice.** The role is recorded in a -session-scoped marker (keyed by session id, outside the repo). Re-derivation, not -memory, is what survives context compaction: a session that has forgotten its -role reads the marker and the lock rather than guessing or asking again. +**3. Persist and re-derive — never ask twice.** The role is recorded in a marker +inside the worktree's own git dir, keyed by the WORKTREE and not by the session +id (corrected 2026-08-06: a session id is not stable across tool calls in every +harness, so keying on it made a declared role invisible one call later — see +`.agents/specs/session-onboarding.md`). Re-derivation, not memory, is what +survives context compaction: a session that has forgotten its role reads the +marker and the lock rather than guessing or asking again. **4. Resolution rides on the mandatory preflight.** `scripts/agent-preflight.sh` resolves and PRINTS the role every run, and refuses to pass if a session has not @@ -197,7 +221,7 @@ checker. | W | Item | Gate | |---|---|---| -| W0 | **LANDED** `scripts/agent-role.py` — role machinery: `.agents/operator.lock` (create-exclusive + TTL + heartbeat), session-scoped role marker, and role resolution printed by `agent-preflight.sh`, which FAILS when a session has not declared one | mutation test | +| W0 | **LANDED** `scripts/agent-role.py` — role machinery: `/vllm-cpp-operator.lock` (create-exclusive + TTL + heartbeat), WORKTREE-scoped role marker (§ Determining the role — a session id is not stable across tool calls), `read-only` and `--headless` as declarations, and role resolution printed by `agent-preflight.sh`, which FAILS by default when a session has not declared one | mutation test | | W1 | **LANDED (report-only)** `check-role-discipline.py`: a commit on `main` touching feature paths must arrive via a merged `row/*` PR, not a direct push | mutation test | | W2 | **LANDED** `scripts/claim-view.py` — generated claim view from PR state (`--apply` online, `--check` offline, 14-day TTL) | mutation test + a run against live PRs | | W3 | **LANDED** `scripts/ready-for-helper.py` — the pickable queue asserting the 5 conditions | mutation test | @@ -213,9 +237,10 @@ PR, whoever produced it. `ROLE_DISCIPLINE_SINCE` names that cutover. Turning it on retroactively would redden history created under the current, explicitly sanctioned direct-push policy. Set that constant to the cutover commit when the protocol is adopted, and every -commit after it is enforced. Likewise `agent-preflight.sh` PRINTS the role every -run but only fails on `--require-role`, so an undeclared session is visible -before it is fatal. +commit after it is enforced. `agent-preflight.sh` PRINTS the role every run, and +that was its only effect until 2026-08-06, when an undeclared role became a +FAILING gate by default; `--no-require-role` is the explicit opt-out, and a +`read-only` declaration passes a plain run while failing `--staged`. ## Risks/decisions @@ -231,7 +256,7 @@ before it is fatal. - **Risk: review becomes the bottleneck.** Mitigated by one-row size-capped PRs, mechanical merge criteria, and merge-first sessions. Watch the open-PR count; if it grows monotonically the cap is too loose. -- **Risk: the lock is stale.** `.agents/operator.lock` needs a TTL and a +- **Risk: the lock is stale.** `/vllm-cpp-operator.lock` needs a TTL and a heartbeat, or a crashed operator blocks everyone. Breaking a stale lock must be logged, never silent. - **Risk: the user declares two operators.** Handled by making lock acquisition diff --git a/.agents/specs/orchestration-harness.md b/.agents/specs/orchestration-harness.md new file mode 100644 index 00000000..89d07641 --- /dev/null +++ b/.agents/specs/orchestration-harness.md @@ -0,0 +1,243 @@ +# The orchestration harness — how an operator runs a row + +User-directed 2026-08-06. Status: **accepted design, not yet implemented.** +This document is the contract; the prose and gates named in § Enforcement are +the work it implies. + +Subsystem **B** of two. Subsystem A — +[session onboarding](session-onboarding.md) — landed the entry point: a session +declares operator, helper or read-only, and declares interactive or headless. +This spec is what an **operator** does next. + +## Scope + +The loop an operator follows to take a row from `READY` to a merged PR: +decompose it, dispatch implementer sub-agents, run the row's gates, have an +**independent reviewer sub-agent** attack the result, and iterate. Plus the two +disciplines that make the loop trustworthy: a gate command that can actually +fail, and a review that mutates rather than reads. + +Out of scope: what work to do (the roadmap), the correctness and performance +directives (`AGENTS.md`, `.agents/directives.md`), and the session interview +(subsystem A). + +## Our baseline — we already have almost all of this, under other names + +`~/_git/skills/spec-driven-development` is a working headless pipeline: spec → +plan → serial sub-agent implementation under a TDD gate → reviewer sub-agent → +pushed branch. Reading it against this repo, the striking thing is how little is +missing. Nearly every concept already exists here with a different name: + +| spec-driven-development | vllm.cpp equivalent | Status | +|---|---|---| +| spec → `docs/spec/…` | **spike spec** `.agents/specs/.md`, 9 required sections | exists, CI-gated by the spike gate | +| plan task carrying `{id, deps}` | **row** with a stable ID and dependencies, in an area matrix | exists | +| **Verify** command, must exit nonzero | the row's **Gates** field — "exact commands" | exists, **unenforced** | +| worktree under `~/.cache/sdd/…` | helper worktree on `row/` | exists; `agent-role.py` materializes it | +| `.sdd-state.json` | `.agents/state.md` plus the claim | exists | +| `pending / done / blocked` | `INVENTORIED / SPIKE / READY / ACTIVE / GATING / PARTIAL / DONE / BLOCKED` | exists | +| PR at the end, never merge | the draft PR **is** the claim; the operator merges | exists | +| implementer sub-agent | — | **missing** | +| **reviewer sub-agent** | — | **missing** | + +So B is not a new system. It is three additions to one that is already here. + +## The evidence that decides the design + +This spec is not written from taste. Two branches were executed through exactly +this loop — the P0 live-state audit and subsystem A — and they produced a +measurement. + +**Ten times, a test passed with the thing it named deleted.** Not ten sloppy +tests; ten tests that looked correct to every reader, including the ones that +wrote them. A sample: + +- `assertIn("REQUIRE_ROLE=1", text)` — satisfied by an unrelated line elsewhere + in the same file, so flipping the default the gate existed to set left the + whole suite green. +- Five tests calling `cmd_env_set()` directly and never `main()`, so deleting + the CLI flag left them green while the only command an agent types did + nothing. +- `assertIn("merged", reason)` — `"unmerged"` contains `"merged"`, so it passed + for both verdicts. +- `assertNotIn("PARTIAL", CHECK_FAILS_ON)` — passes for an empty set, i.e. for a + gate that fails on nothing at all. +- A test asserting a filename appears in a path list, which passed when that + file contributed zero rows. +- A row id asserted with `assertIn`, satisfied by the fixture's own queue. + +**Not one of these was found by reading the diff.** Every one was found by a +reviewer that mutated the code and re-ran the suite. And roughly half originated +in the *plan text* — written by the same agent that reviewed its own work and +called it good. + +That is the whole argument for a separate reviewer, and it also says what kind +of reviewer: **the value is mutation, not diff-reading.** A reviewer that reads +a diff and comments on style would have caught none of the ten. + +A second measurement, same two branches: **every single Important finding was +produced by an independent sub-agent, and none by the implementer's own +self-review.** Self-review reliably caught typos and never caught a defect the +author had reasoned themselves into. + +## Design + +### The operator's loop + +The operator does not write the feature. It decomposes, dispatches, verifies, +and integrates. + +``` +row (READY, spike merged) + └─ decompose into tasks, each with a Gate command that exits nonzero on failure + └─ for each task, serially: + ├─ dispatch implementer sub-agent (fresh, TDD, commits in the worktree) + ├─ RUN THE GATE YOURSELF — never accept the implementer's word + ├─ dispatch reviewer sub-agent (fresh, never the one that wrote it) + │ └─ MUTATE, don't read: for each test, delete what it names and re-run + ├─ findings? → fix round (bounded), then a SCOPED re-review + └─ clean? → next task + └─ draft PR (already open — it IS the claim) → operator merges +``` + +**Serial, one task at a time.** Two implementers in one worktree means +concurrent edits to one checkout. Parallelism comes from multiple helpers in +multiple worktrees, which the role model already provides. + +**The operator runs the gate itself.** This is the one failure mode nothing else +catches: if "done" is the implementer's opinion of its own work, the loop has no +floor. + +### The reviewer sub-agent, and what makes it different + +A reviewer is dispatched **fresh** for every task, is **never** the agent that +wrote the code, and is told to attack rather than assess. + +Its binding instruction is **mutation over reading**: + +> For each test in the change, delete or invert the line it names and re-run the +> suite. A test that stays green is a finding, regardless of how it reads. + +Two supporting rules, both learned the same way: + +- **Do not trust the report.** A stated rationale — "kept it simple + deliberately", "left it per YAGNI" — is the implementer grading its own work + and never downgrades a finding's severity. On these two branches, three + implementer reports contained a claim that was false and disclosed as true; + each was caught by a reviewer reproducing it rather than accepting it. +- **A finding the plan mandated is still a finding.** Roughly half of all + Important findings were defects in the plan text. A reviewer that treats the + plan as authority cannot find them; it must report them, labelled, and the + human decides. + +### Gate-command discipline + +A row's `Gates` field already promises "exact commands". Nothing checks that a +gate command can **fail**. A gate that is `true`, `echo ok`, or a command whose +exit status is masked by a pipe collapses "done" into an opinion. + +Three rules, each of which this project has already been bitten by: + +1. **A gate command must exit nonzero on failure.** A task you cannot write one + for is a task that cannot be run through this loop. Split it, restate it, or + narrow its deliverable until a real command can judge it. +2. **Never pipe a gate.** `cmd | tail` reports the exit status of `tail`. + Redirect to a file and check `$?`. +3. **Verify the committed form, not the staged one.** `check-doc-checkpoint.py` + runs `--staged` in preflight, which passes vacuously once work is committed. + Eleven commits on the P0 branch were red while every preflight was green. + +### Headless mode + +Subsystem A made mode a declaration: interactive by default, headless only when +stated. This loop honours it. + +| | interactive | headless | +|---|---|---| +| ambiguity | ask | decide, record in `.agents/state.md`, continue | +| a task that will not go green | ask | park it, skip its dependents, carry on | +| landing | operator merges | never merge, never delete the worktree; push and report | + +Headless never asks — a question to an absent human is a hang, not a pause — and +therefore every decision it makes must appear in the final report. Interactive +is the default precisely because the judgment calls this loop surfaces +(a state transition on the canonical record, a benchmark that moves a binding +number) are the human's to make. + +### What the loop must never do + +- **Never let the reviewer fix what it found.** Findings go back to a fresh + implementer. A reviewer that edits has reviewed its own work. +- **Never fix findings in the operator session.** It pollutes the context that + exists to coordinate, and controller fixes skip review entirely. +- **Never mark a task done without having seen its gate exit 0** with your own + eyes. +- **Never weaken a gate to make a transition pass.** Repair the record. + +## Enforcement + +**Prose.** The loop lives in `.agents/workflow.md`, next to subsystem A's +interview, because that is what an agent reads. `AGENTS.md`'s operator bullet +points at it. + +**`scripts/check-gate-commands.py`** (new, CI-gated, with a mutation suite): +every row at `READY` or later carries at least one gate command; no gate command +is `true`, `:`, `echo …`, or piped into another command. This is checkable from +the tree and needs no network. + +**The reviewer prompt is a tracked artifact**, not folklore: +`.agents/prompts/reviewer.md`, carrying the mutation instruction verbatim. A +prompt that lives only in an operator's head is not a protocol. + +**`scripts/check-protocol-consistency.py`** extends to assert the loop appears +in `.agents/workflow.md`, exactly as it now asserts the role interview. Prose +and gate move in the same change — that checker exists because an obligation was +once migrated in `AGENTS.md` and the checker but not in the manual. + +## Work breakdown + +| # | Work | +|---|---| +| 1 | `.agents/prompts/reviewer.md` and `.agents/prompts/implementer.md` as tracked artifacts | +| 2 | `scripts/check-gate-commands.py` + mutation suite; wire into preflight and CI | +| 3 | The loop written into `.agents/workflow.md`, with `check-protocol-consistency.py` extended in the same change | +| 4 | `AGENTS.md` operator bullet points at the loop; `operator-helper-protocol.md` records that the operator drives work through sub-agents | +| 5 | Backfill gate commands for the rows that lack one, or record honestly that they cannot be gated yet | + +Item 5 will find rows that cannot state a failing gate command. That is a +finding, not an obstacle: those rows cannot be run through this loop until they +are narrowed, and saying so is more useful than a `true` that lets them pass. + +## Risks and decisions + +**Accepted: this makes every task slower.** Two branches of evidence say the +review loop roughly doubles the cost of a task and catches defects that reading +does not. The alternative is not a faster loop; it is the same loop with the +findings still in the tree. + +**Accepted: the reviewer is another agent, with the same blind spots.** It is +not smarter than the implementer — it is *differently positioned*, and it +mutates. The mutation instruction is what makes independence pay; without it a +reviewer converges on the implementer's reasoning, which is exactly how the ten +tests survived their authors. + +**Rejected: let the implementer self-review instead.** Measured across two +branches: self-review caught typos and never caught a defect the author had +reasoned themselves into. Three implementer reports asserted something false in +good faith. + +**Rejected: parallel implementers in one worktree.** Concurrent edits to one +checkout, with nobody to untangle the result. Parallelism belongs at the helper +level, where the role model already isolates it. + +**Rejected: adopting `spec-driven-development` wholesale.** It is headless by +construction and forbids asking anything. That is right for an unattended +overnight run and wrong as this repo's default, where the loop routinely +surfaces decisions about the canonical record that belong to a human. We take +its structure and keep our interaction model. + +**Open:** the P0 branch's gate now cannot re-detect the rows it vacated, because +its own commit messages name them and the evidence rule is a commit-message +mention with no code-touch filter. The same class of question applies to any +gate this loop introduces: *what does this gate stop being able to see once it +has run once?* Worth asking of `check-gate-commands.py` before it lands. diff --git a/.agents/specs/session-onboarding.md b/.agents/specs/session-onboarding.md new file mode 100644 index 00000000..b8bdaf43 --- /dev/null +++ b/.agents/specs/session-onboarding.md @@ -0,0 +1,267 @@ +# Session onboarding — ask, don't assume + +User-directed 2026-08-06. Status: **IMPLEMENTED 2026-08-06**, except the one +piece § Enforcement marks DEFERRED (refusal on write paths other than +`preflight --staged`). This document is the contract; § Work breakdown records +what landed and what did not. + +Subsystem **A** of two. Subsystem B — the orchestration harness an operator +follows to run a row through subagents with an independent review — is a +separate spec and lands after this one. They meet at exactly one point: this +interview's "long campaign" answer hands off into B. + +## Scope + +What happens in the first minute of an agent session: which role this session +holds, which row it is taking, whether it may write at all, and where the +machine-specific values come from. + +Out of scope: what work to do (the roadmap), how to do it (`AGENTS.md` and +`.agents/directives.md`), and the orchestration loop (subsystem B). +`.agents/developer-preferences.md` generation is deliberately excluded — it is +a per-developer profile, not a per-session decision. + +## Our baseline — the obligation exists, nothing triggers it + +Every piece of this already exists as prose or tooling. None of it fires. + +- `.agents/specs/operator-helper-protocol.md` already says the session "ASKS + before doing anything else", and explains why derivation cannot work: several + sessions launch from the same checkout, so nothing distinguishes them until a + role has already been taken. +- `scripts/agent-role.py` already does the hard part — `claim`, materialize + (operator lock in the git COMMON dir, helper = worktree), `heartbeat`, + `release`. It exits 3 when undeclared. +- `scripts/agent-preflight.sh` already has `--require-role`, and it was + **opt-in** when this was written; `.agents/NOW.md` recorded it as "still + opt-in". Both were changed by the work below: it is now the default. +- `AGENTS.md` already says that when `.env` is missing it "is asked, never + inferred". +- There is **no `.claude/settings.json` and no hook of any kind**. Nothing runs + at session start. + +So the protocol depends entirely on an agent reading a 328-line index and +choosing to comply. The 2026-08-04 incident is what that costs: two sessions +pushed to `main` within minutes, neither claimed anything, and a three-way merge +silently produced a VARIANT of the other session's binding numbers. + +**This spec adds no new concepts. It makes the existing obligation fire, and +makes it pleasant when it does.** + +## Design + +### The trigger is preflight, not a hook + +`--require-role` becomes the **default**. `--no-require-role` is the escape for +scripted and CI use. + +A hook was considered and rejected: it would be Claude-Code-specific, and this +protocol is harness-neutral by design (`AGENTS.md` is read by other harnesses +too). Preflight is the harness-neutral trigger the protocol already mandates, +and the push chain (`gate && git push`) means a session that skips preflight +also never lands anything. The role is therefore demanded at the latest before +the first durable effect. + +**A script cannot ask a question.** No harness-neutral mechanism exists for a +shell script to run an interactive prompt, and a hook cannot ask either — hooks +inject text, they do not converse. So the split is fixed: + +| Job | Owner | +|---|---| +| Detect and report what is unresolved | `scripts/agent-onboard.py --probe` | +| Ask the human | the agent, in its own UI | +| Make the answer a fact | `scripts/agent-role.py claim`, `--env-set` | + +The canonical interview text therefore lives in `.agents/workflow.md`, where an +agent actually reads it, not in a script that cannot perform it. + +### Ask about the work, not the vocabulary + +The interview asks what the session is here to do. The role follows from the +answer, and the answer is something the developer already knows. + +| "What are you here to do?" | Role | What it means | +|---|---|---| +| A long or multi-step campaign — several changes, a benchmark grid, a whole row block | **operator** | Owns `main` and the GPU. Merges PRs first. Drives feature work through sub-agents rather than writing it. One at a time, repo-wide. | +| One scoped change — a fix, a port, a single row | **helper** | Isolated worktree on `row/`, draft PR opened at the START. That PR **is** the claim. Never touches `main`. | +| Just looking — reading code, answering a question | **read-only** | No lock, no worktree, no claim. | + +`read-only` is **not a third role.** It is a declared *absence* of claim, +recorded so the gate can tell "decided not to claim" from "never asked". The +two-role model in +[operator-helper-protocol.md](operator-helper-protocol.md) is unchanged. + +It exists because the alternative punishes the most common session. Forcing a +question-answering session to claim `operator` would take the repo-wide lock and +block a real one; forcing `helper` would create a throwaway worktree. Faced with +either, a developer reaches for `--no-require-role`, and the gate erodes to +nothing. A cheap honest answer keeps the gate credible. + +A `read-only` session **passes a plain preflight and is refused by +`scripts/agent-preflight.sh --staged`** — and by nothing else. That refusal is +what SHIPPED; the wider claim this paragraph carried until 2026-08-06 ("refused +by every write path: commit, push, and any matrix or record edit") was never +true of the delivered code. `git commit`, `git push`, the `gate && git push` +chain AGENTS.md mandates (which runs preflight WITHOUT `--staged`) and every +record or matrix edit proceed unimpeded, so past staging `read-only` is the +honour system. Repo-wide write refusal is DEFERRED, not built — see the work +breakdown, item 3. Escalating is one command — a declaration, not a sentence. + +### Mode: interactive by default, headless when asked + +`~/_git/skills/spec-driven-development` forbids asking anything, because it runs +unattended overnight and "a question is a hang, not a pause". This spec is built +on asking. Both are right in their context, so the mode is declared explicitly, +in the same breath as the role — one question, not two. + +| Mode | When | Behaviour on ambiguity | +|---|---|---| +| **interactive** (default) | a human is present | ask | +| **headless** | the human explicitly says the session is unattended | decide, record the decision in `.agents/state.md`, never block, never merge, park what will not go green | + +Headless is never inferred — not from the hour, not from silence, not from a +long-running task. It is stated, exactly like the role. + +### `.env` is asked just in time, never up front + +A missing `.env` does **not** block the role claim, and the developer is never +walked through a template for values the session may never use. + +The agent asks for a value at the moment a gate actually needs it — the oracle +path before an oracle comparison, the gate host before a device run, +`${GPU_LOCK}` before touching the GPU. Anything unanswered is recorded as +empty, and `AGENTS.md` already defines what empty means: the gates that need it +stay `PENDING`. That is an honest state, not a failure. + +`scripts/agent-onboard.py --env-set KEY=VALUE` performs the write, creating +`.env` from `.env.example` on first use. The agent asks; the script writes. +Never substitute another developer's paths, and never infer a value from a +username, a filesystem path or a machine identity. + +## Correction: a role keys on the WORKTREE, not the session + +User-directed 2026-08-06, after Task 3 of the implementation hit it. + +`scripts/agent-role.py`'s docstring asserts the session id is "the parent +process id, which is the agent CLI process and is **stable across tool calls +within a session (measured)**". **That is false in at least one real harness**, +and it was measured to be false three ways: + +- two consecutive `agent-role.py show` calls report different ids + (`ppid:2530150`, then `ppid:2530375`); +- `claim read-only` in one call, then `show` in the next, reports `UNDECLARED`; +- `agent-preflight.sh` in the next call exits 1. + +Every tool call gets a fresh shell, and the harness does not persist environment +variables, so `VLLM_CPP_AGENT_SESSION` cannot be exported once and reused. +Because `resolve()` requires `marker["session"] == me`, a role claimed in one +call is invisible in the next. + +The consequence is worse than inconvenient: `--require-role` default-on becomes +**unpassable rather than strict**, which points every agent straight at +`--no-require-role` — precisely the erosion the `read-only` answer exists to +prevent. A gate that cannot be satisfied is not a stricter gate; it is a gate +people learn to disable. + +**The fix is to key the role on the worktree.** The marker already lives in the +worktree's own git dir, so one worktree is one role, and the spec's own +reasoning supports it: *"a materialized helper is distinguishable from the +primary checkout without any bookkeeping"*. The circularity the original +protocol worried about — that derivation cannot decide a role before one has +been taken — is resolved by the DECLARE step writing the marker; after that, +deriving from the worktree is sound. + +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. +- **Helpers stay isolated.** A helper already materializes its own worktree. + +The cost is explicit: **two agent sessions sharing one checkout now share a +role.** That is the case the session id was invented for, and it is the right +trade — helpers get their own worktree by construction, so the shared case is +almost always the operator's primary checkout, where one role is the correct +answer anyway. `scripts/agent-role.py`'s docstring must lose the false +"measured" claim in the same change. + +## Enforcement + +**`scripts/agent-onboard.py --probe`** (new, harness-neutral, read-only) reports +machine-readable state and asks nothing: + +- role: `operator` | `helper ` | `read-only` | `undeclared` +- mode: `interactive` | `headless` +- `.env`: `present` | `missing` | `incomplete: KEY,KEY` +- helper queue: the `READY` row IDs, from the existing `ready-for-helper.py` + +**`scripts/agent-preflight.sh`**: `--require-role` on by default; +`--no-require-role` to opt out. On an undeclared role it fails with the +interview to run and the exact claim commands — not a bare error code. A gate +that tells you what to do next is the difference between a protocol people +follow and one they route around. + +**`scripts/agent-role.py`** gains `claim read-only` and a `--headless` flag on +`claim`, so mode and role are one materialized fact. + +**Preflight `--staged` refuses a `read-only` session.** DELIVERED: staging is +writing, so `--staged` fails on a `read-only` marker. DEFERRED and NOT built: +"the role check runs before any record edit". Nothing outside preflight consults +the role, so `git commit`, `git push` and every record/matrix edit proceed for a +`read-only` session. Closing that needs a repo-wide write hook (a `pre-commit` +hook is harness-neutral but opt-in per clone; a role check inside every record +checker is neutral but touches ~15 scripts) and is out of this spec's delivered +scope. It is tracked in the SDD ledger rather than assumed done. + +**Prose and gate move together.** `AGENTS.md` T0's role bullet, the +`.agents/workflow.md` session protocol (which carries the canonical interview), +and `operator-helper-protocol.md` are updated in the SAME change as the checker. +`scripts/check-protocol-consistency.py` exists precisely because an obligation +was once migrated in `AGENTS.md` and the checker but not in the manual, which +went on instructing agents to do the thing the migration had removed. Extend it +to assert the interview table appears in `workflow.md`. + +**Never weaken a checker to make a transition pass.** If preflight is red +because the role is undeclared, the answer is to declare it. + +## Work breakdown + +Each item is independently landable. + +| # | Work | +|---|---| +| 1 | `scripts/agent-onboard.py --probe` + mutation tests; reports state, writes nothing | +| 2 | `agent-role.py claim read-only` and `--headless`; the mode becomes a materialized fact | +| 3 | `--require-role` default-on, `--no-require-role` escape, actionable failure text; preflight `--staged` refuses `read-only` (LANDED). Refusal on every OTHER write path — commit, push, record/matrix edit — is DEFERRED, not built | +| 4 | `--env-set KEY=VALUE`, creating `.env` from `.env.example` on first use | +| 5 | Prose: `AGENTS.md` T0, `workflow.md` interview table, `operator-helper-protocol.md` — in the same change as the `check-protocol-consistency.py` extension | + +## Risks and decisions + +**Accepted: preflight is only as binding as the habit of running it.** The +mitigation is structural rather than aspirational — `AGENTS.md` T0 already +requires it at session start and before committing, and chains the push to it, +so the unclaimed session cannot land anything. A hook would close the remaining +gap for one harness at the cost of neutrality; rejected on that basis. + +**Accepted: `read-only` can be over-used.** Someone can declare `read-only` and +then find real work. That is fine and expected — escalation is one command. The +failure it prevents (silent unclaimed writing) is far worse than the one it +allows (an extra claim command mid-session). + +**Rejected: derive the role from the environment.** Already tried and already +recorded as an error in `operator-helper-protocol.md`. Several sessions launch +from one checkout; a helper only becomes environmentally distinguishable *after* +it has taken a worktree, so derivation is circular. + +**Rejected: full `.env` walkthrough at session start.** It asks for values most +sessions never use, and front-loads friction onto the first minute — the exact +moment a developer is least willing to spend it. + +**Rejected: generating `.agents/developer-preferences.md` in this interview.** +It is a per-developer profile, not a per-session decision, and folding it in +would make the first prompt of every session long enough to be skipped. + +**Open, deferred to subsystem B:** an operator that declares `headless` still +has no written orchestration loop to run. That is B's subject, and B is where +the independent reviewer subagent — the single highest-value missing piece — +gets specified. diff --git a/.agents/workflow.md b/.agents/workflow.md index 69a1dc52..871f2c9a 100644 --- a/.agents/workflow.md +++ b/.agents/workflow.md @@ -5,16 +5,57 @@ and continue. Follow this protocol every session. ## Session protocol + +### First question of every session + +`scripts/agent-preflight.sh` fails until this session has declared a role. Ask +what the work is — not which role the developer wants, which is vocabulary they +should not have to learn first. + +**Run `scripts/agent-role.py show` before asking.** The marker is keyed on the +WORKTREE and has no TTL, so a checkout that ever claimed carries that role into +every later session silently, and preflight will not prompt. "First question of +every session" is therefore first question per WORKTREE: if `show` reports a +role this session did not choose, re-claim rather than inherit it. + +| What are you here to do? | Claim | What it means | +|---|---|---| +| A long or multi-step campaign — several changes, a benchmark grid, a whole row block | `scripts/agent-role.py claim operator` | Owns `main` and the GPU. Merges PRs first. Drives feature work through sub-agents rather than writing it. One at a time, repo-wide. | +| One scoped change — a fix, a port, a single row | `scripts/agent-role.py claim helper --row ` | Isolated worktree on `row/`, draft PR opened at the START. That PR **is** the claim. Never touches `main`. | +| Just looking — reading code, answering a question | `scripts/agent-role.py claim read-only` | No lock, no worktree, no claim. Passes a plain preflight; `scripts/agent-preflight.sh --staged` refuses it. | + +`read-only` is a declared **absence** of claim, not a third role. Escalating is +one command. Its refusal is NARROW, and saying otherwise would be the drift this +manual exists to prevent: `scripts/agent-preflight.sh --staged` is the ONLY +write path that refuses a `read-only` session. `git commit`, `git push`, the +`gate && git push` chain (that preflight runs WITHOUT `--staged`) and every +record or matrix edit all proceed unimpeded. Past staging, `read-only` is the +honour system, not a guard — repo-wide write refusal is not built. + +Add `--headless` when the developer has said the run is unattended: decide, +record each decision in `.agents/state.md`, never block, never merge, park what +will not go green. Headless is **declared, never inferred** — not from the hour, +not from silence, not from a long task. + +`.env` is asked **just in time**: when a gate needs a value, ask for that value +and write it with `scripts/agent-onboard.py --env-set KEY=VALUE`. Never walk the +whole template up front, and never infer a value from a username, a path or a +machine identity. Unanswered means empty, and empty means the gates that need it +stay `PENDING`. + +Run `scripts/agent-onboard.py --probe` to see what is still unresolved. + + 0. **Declare your role** before anything else, if this session has not already: - `scripts/agent-role.py show` (exit 3 = undeclared), then - `claim operator` or `claim helper --row `. Helpers then create their - worktree, `row/` branch and DRAFT PR immediately — the draft PR is - the claim. `scripts/ready-for-helper.py` lists what a helper may pick. Full - protocol: [specs/operator-helper-protocol.md](specs/operator-helper-protocol.md). - In a fresh checkout with no untracked `.env`, first walk the developer - through creating it interactively (the `.env.example` template + - [environment registration](environment.md#registering-your-own-environment) - — see AGENTS.md); like the role, the environment is asked, never inferred. + `scripts/agent-role.py show` (exit 3 = undeclared), then the answer from the + interview above. A role keys on the WORKTREE, so one worktree is one role and + it survives the loss of a session id. Helpers then create their worktree, + `row/` branch and DRAFT PR immediately — the draft PR is the claim. + `scripts/ready-for-helper.py` lists what a helper may pick. Full protocol: + [specs/operator-helper-protocol.md](specs/operator-helper-protocol.md). + The environment is asked the same way — never inferred — but per VALUE and + only when a gate needs it, against the `.env.example` template and + [environment registration](environment.md#registering-your-own-environment). 1. **Orient**: read [NOW.md](NOW.md) FIRST — it is the one-Read resume surface (live claims, current gate, next actions) and is rewritten in place every checkpoint. The state tail is trustworthy only below the diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6da95f4c..ed5b8bb4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -88,6 +88,7 @@ jobs: run: | python3 scripts/check-role-discipline.py python3 tests/scripts/test_agent_role.py + python3 tests/scripts/test_agent_onboard.py - name: Claim view, helper queue and PR reviewability run: | python3 scripts/claim-view.py --check diff --git a/AGENTS.md b/AGENTS.md index e8996213..45ae1df0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,15 +102,30 @@ version, this list is the reminder. - **Run `scripts/agent-preflight.sh`** at session start and before committing, and chain the push to it (`gate && git push`) so a red gate cannot be followed by a green push. -- **Know your ROLE before you work.** Operator or helper +- **Know your ROLE before you work**, and ASK for it as the FIRST question of + the session — the interview is in + [workflow.md](.agents/workflow.md#first-question-of-every-session) ([protocol](.agents/specs/operator-helper-protocol.md)). It cannot be derived at session start — several sessions launch from one checkout — so DECLARE it - (`scripts/agent-role.py claim operator|helper --row `), which - materializes it into an exclusive lock or a worktree+PR, after which it is - re-derived rather than remembered. A helper works in an isolated worktree on - `row/` and opens a DRAFT PR at the START: that PR **is** the claim. - The operator merges PRs first thing, owns `main` and the GPU, and drives - feature work through sub-agents rather than writing it. + (`scripts/agent-role.py claim operator|helper --row |read-only`), + which materializes it into an exclusive lock or a worktree+PR, after which it + is re-derived from the WORKTREE rather than remembered. The marker carries no + TTL, so in practice this is the first question per WORKTREE, not per session: + a later session in a checkout that ever claimed INHERITS that role silently. + Run `scripts/agent-role.py show` first and re-declare if it is not yours. + `scripts/agent-preflight.sh` FAILS an undeclared session BY DEFAULT + (`--no-require-role` opts out), so this is a gate, not a convention. + `read-only` is the third answer and a declared ABSENCE of claim — no lock, no + worktree, passes a plain preflight, and refused by `agent-preflight.sh + --staged`, which is the ONLY write path that refuses it. `git commit`, + `git push`, the `gate && git push` chain above (that preflight runs WITHOUT + `--staged`) and every record or matrix edit all proceed: past staging, + `read-only` is the honour system, not a guard. A + helper works in an isolated worktree on `row/` and opens a DRAFT PR at + the START: that PR **is** the claim. The operator merges PRs first thing, owns + `main` and the GPU, and drives feature work through sub-agents rather than + writing it. Add `--headless` only when the developer has SAID the run is + unattended; it is declared, never inferred. - **Never three-way merge a keyed record.** `docs/STATUS.md`, `docs/BENCHMARKS.md`, `docs/FEATURES.md`, `.agents/NOW.md`, the matrices and `coordination.md` are merged by taking `main`'s version wholesale, re-applying diff --git a/docs/BENCHMARKS.md b/docs/BENCHMARKS.md index 69e5a0c4..47827f58 100644 --- a/docs/BENCHMARKS.md +++ b/docs/BENCHMARKS.md @@ -274,9 +274,10 @@ Correctness re-validated bit-identical across the advance, zero golden drift. discarded. Workload equivalence between arms is audited, not assumed: batch cap, token budget, context, corpus bytes, KV and SSM dtypes, kernel family, and graphed decode all match, and the audit is -[recorded](../.agents/specs/benchmark-equivalence-audit-2026-07-15.md). The 2026-08-04/05 records work (agent-record substrate, triage, +[recorded](../.agents/specs/benchmark-equivalence-audit-2026-07-15.md). The 2026-08-04/06 records work (agent-record substrate, triage, compaction, CI concurrency, anchor backfill, the operator/helper protocol W0-W5 -with role discipline now enforcing, and the upstream/device inventory) touched +with role discipline now enforcing, the upstream/device inventory, and session +onboarding through probe 5/5) touched no engine code and moved no number: NOT APPLICABLE, nothing to reproduce. The PR #28 sanitizer repair is also NOT APPLICABLE to performance: both full diff --git a/docs/STATUS.md b/docs/STATUS.md index 6b222aa5..6d7f714a 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -2221,3 +2221,5 @@ The next consumer run exposed a separate Voxtral portability error: its GCC-only `-Wstringop-overflow` suppression is now guarded out of Clang builds, where the unknown warning name was itself fatal under `-Werror`. The Go `go-m1cpu` folding diagnostics in that run were nonfatal and outside this repo. + +**Session onboarding: design accepted, step 5/5** (`.agents/specs/session-onboarding.md`). diff --git a/docs/superpowers/plans/2026-08-06-session-onboarding.md b/docs/superpowers/plans/2026-08-06-session-onboarding.md new file mode 100644 index 00000000..ea1368b2 --- /dev/null +++ b/docs/superpowers/plans/2026-08-06-session-onboarding.md @@ -0,0 +1,962 @@ +# Session Onboarding Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the existing role obligation actually fire at session start, and make answering it pleasant — the agent asks about the work, the role follows, and a session that only reads never has to fake a claim. + +**Architecture:** A fixed three-way split, because a shell script cannot ask a question and a hook cannot converse. `scripts/agent-onboard.py --probe` **reports** state and writes nothing; the **agent** asks, using the canonical interview in `.agents/workflow.md`; `scripts/agent-role.py` and `--env-set` **make the answer a fact**. `agent-preflight.sh` is the harness-neutral trigger: `--require-role` becomes the default, so the first command of a session demands a decision. + +**Tech Stack:** Python 3 standard library only, `argparse`, `importlib.util` for loading hyphenated modules in tests, `unittest`. Bash for `agent-preflight.sh`. Matches the house style of `scripts/check-agent-record.py` and `scripts/agent-role.py`. + +## Global Constraints + +Copied from `AGENTS.md` and `.agents/specs/session-onboarding.md`. Every task's requirements implicitly include this section. + +- **Every commit carries `FOLLOWING_AGENTS_PROTOCOL`** plus `Assisted-by: Claude Code:claude-opus-5 [ClaudeCode]`. **Never** `Signed-off-by` or `Co-Authored-By` from an AI. +- **Run `bash scripts/agent-preflight.sh` before committing; it must exit 0.** Never pipe it — redirect to a file and check `$?`. +- **Every commit touching `scripts/`, `tests/` or `.agents/specs/` also updates `docs/STATUS.md` and `docs/BENCHMARKS.md` in the SAME commit.** Verify the committed form explicitly with `python3 scripts/check-doc-checkpoint.py --commit ` — preflight only runs that checker `--staged`, which passes vacuously after committing. `docs/STATUS.md` sits under a shrink-only char ratchet in `scripts/check-public-doc-tables.py`; if you add text there, stay under the cap or offset it and **lower** the ratchet. Never raise it. `docs/BENCHMARKS.md` is at its 35-prose-paragraph budget and a 700-char-per-paragraph limit — extend the existing "NOT APPLICABLE" paragraph rather than adding a new one. +- **Use a ROLLING doc surface, do not append per task.** Task 1 already added the entry on both pages and left `docs/STATUS.md` at 284,071 of a 284,081 cap and the BENCHMARKS paragraph at 676 of 700. There is no room for five separate additions. Every later task **edits the line Task 1 wrote** — rolling it forward **digit-only**: "step 1/5" → "2/5" → … → "step 5/5". `docs/STATUS.md` now sits at **exactly** its 284,081 cap with zero headroom and the BENCHMARKS paragraph at 699 of 700, so a digit roll is free and **anything longer is red** — never end the sequence with "all 5", which is two characters more and fails. If your entry needs more room, shorten *your own* sentence; never someone else's. Never compact unrelated evidence to buy room: on the previous branch that produced a cross-arm supersession claim that contradicted a paragraph two lines below it. +- **Python standard library only.** `from __future__ import annotations`, type hints, house style. +- **Never weaken a checker to make something pass. Repair the record.** +- **`read-only` is a declared ABSENCE of claim, not a third role.** The claimable roles stay exactly `("operator", "helper")`. +- **Headless is never inferred** — not from the hour, not from silence, not from a long task. It is declared. +- **Never infer a `.env` value** from a username, filesystem path, machine identity, or another developer's paths. Unanswered means empty, and empty means the gates that need it stay `PENDING`. +- Stage explicit paths. Never `git add -A`. + +**Existing interfaces you build on** (read, do not reimplement): + +- `scripts/agent-role.py`: `ROLES = ("operator", "helper")` (line 41), `UNDECLARED_EXIT = 3` (line 47), `session_id() -> str`, `marker_path() -> Path`, `lock_path() -> Path`, `read_json(path) -> dict | None`, `current_branch() -> str`, `resolve() -> dict`, `cmd_show`, `cmd_claim`, `cmd_heartbeat`, `cmd_release`, `main()`. The marker JSON is `{"role", "row", "session", "at"}`. +- `scripts/agent-preflight.sh`: arg loop at lines 24–32, `REQUIRE_ROLE=0` at line 23, `CHECKERS=(...)` at line 34, the `SUITES=(...)` list, and the role block at lines 84–90 which appends `role-undeclared` to `failed`. +- `scripts/ready-for-helper.py` already computes the `READY` queue. +- `.env.example` keys: `VLLM_SOURCE SGLANG_SOURCE LLAMACPP_SOURCE VLLM_ORACLE DEPENDENCY_SOURCE GATE_HOST CUTLASS_DIR GPU_LOCK DEVICE_ARCH DEVICE_TOOLKIT_ROOT DEVICE_COMPILER`. +- `.agents/workflow.md` sections: `## Session protocol` (line 6), `## Tabular lifecycle` (line 142). + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `scripts/agent-onboard.py` (create) | Report unresolved session state (`--probe`); write `.env` values (`--env-set`). Never asks. | +| `tests/scripts/test_agent_onboard.py` (create) | Unit + mutation suite for the probe and the env writer | +| `scripts/agent-role.py` (modify) | Accept `read-only` and `--headless`; carry mode in the marker | +| `tests/scripts/test_agent_role.py` (modify) | Cover the new declarations | +| `scripts/agent-preflight.sh` (modify) | `--require-role` default-on, `--no-require-role`, actionable failure text, refuse `--staged` for `read-only` | +| `scripts/check-protocol-consistency.py` (modify) | Assert the interview table appears in `.agents/workflow.md` | +| `AGENTS.md`, `.agents/workflow.md`, `.agents/specs/operator-helper-protocol.md` (modify) | The prose, moved in the same change as the gate | + +--- + +### Task 1: The probe + +**Files:** +- Create: `scripts/agent-onboard.py` +- Test: `tests/scripts/test_agent_onboard.py` + +**Interfaces:** +- Consumes: `scripts/agent-role.py` — `resolve() -> dict` (keys `role`, `row`, `session`, `branch`, `reason`), loaded via `importlib.util` because the filename is hyphenated. +- Produces: `ENV_KEYS: tuple[str, ...]`; `env_state() -> tuple[str, list[str]]` returning `(status, missing_keys)` where status is `"present" | "missing" | "incomplete" | "unreadable"`; `ready_rows() -> list[str]`; `probe() -> dict` with keys `role`, `row`, `mode`, `env`, `env_missing`, `queue`; `render_probe(state: dict) -> str`; `main(argv=None) -> int`. + +- [ ] **Step 1: Write the failing test** + +Create `tests/scripts/test_agent_onboard.py`: + +```python +#!/usr/bin/env python3 +"""Unit and mutation checks for scripts/agent-onboard.py. + +The probe exists to report what is unresolved. Its one job is to be honest +about absence: a missing .env and an unreadable .env must not look the same as +a complete one, and an undeclared role must never render as a declared one. +""" + +from __future__ import annotations + +import importlib.util +import sys +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[2] + + +def _load(name: str, relative: str): + path = ROOT / relative + spec = importlib.util.spec_from_file_location(name, path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +onboard = _load("agent_onboard", "scripts/agent-onboard.py") + + +class EnvStateTests(unittest.TestCase): + def test_env_keys_match_the_tracked_example(self): + # The probe must never invent a key. .env.example is the only source. + example = (ROOT / ".env.example").read_text(encoding="utf-8") + declared = { + line.split("=", 1)[0] + for line in example.splitlines() + if line and not line.startswith("#") and "=" in line + } + self.assertEqual(set(onboard.ENV_KEYS), declared) + + def test_missing_file_reports_missing_not_incomplete(self): + status, missing = onboard.env_state(ROOT / "does-not-exist-.env") + self.assertEqual(status, "missing") + self.assertEqual(sorted(missing), sorted(onboard.ENV_KEYS)) + + def test_blank_value_counts_as_missing_that_key(self): + # An empty value is a legitimate "unavailable", but the probe still + # has to report it so the agent knows what it may ask for. + text = "\n".join(f"{k}=" for k in onboard.ENV_KEYS) + status, missing = onboard.env_state_from_text(text) + self.assertEqual(status, "incomplete") + self.assertEqual(sorted(missing), sorted(onboard.ENV_KEYS)) + + def test_all_values_present_reports_present(self): + text = "\n".join(f"{k}=/some/path" for k in onboard.ENV_KEYS) + status, missing = onboard.env_state_from_text(text) + self.assertEqual(status, "present") + self.assertEqual(missing, []) + + +class ProbeRenderTests(unittest.TestCase): + UNDECLARED = { + "role": None, "row": None, "mode": "interactive", + "env": "missing", "env_missing": ["VLLM_ORACLE"], "queue": ["ENG-FOO"], + } + + def test_undeclared_role_renders_as_undeclared(self): + out = onboard.render_probe(self.UNDECLARED) + self.assertIn("UNDECLARED", out) + self.assertNotIn("operator", out.split("queue")[0]) + + def test_declared_role_renders_with_its_row(self): + # The row id must NOT be one the fixture queue already contains, or the + # queue line satisfies the assertion and deleting row rendering stays + # green. Assert the `row=` prefix, not the bare id. + out = onboard.render_probe(dict(self.UNDECLARED, role="helper", row="KERNEL-BAR")) + self.assertIn("helper", out) + self.assertIn("row=KERNEL-BAR", out) + + def test_undeclared_render_carries_the_interview_hint(self): + # The hint is the whole point of the probe: without it an agent sees a + # state line and no instruction. Deleting the block must go red. + out = onboard.render_probe(self.UNDECLARED) + self.assertIn("claim", out) + self.assertIn("read-only", out) + self.assertNotIn("claim", onboard.render_probe( + dict(self.UNDECLARED, role="helper", row="KERNEL-BAR"))) + + def test_probe_never_exits_nonzero(self): + # The probe reports; it does not gate. Preflight gates. + self.assertEqual(onboard.main(["--probe"]), 0) + + +if __name__ == "__main__": + unittest.main() +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: FAIL — `FileNotFoundError`/`AssertionError` from `_load`, because `scripts/agent-onboard.py` does not exist. + +- [ ] **Step 3: Write minimal implementation** + +Create `scripts/agent-onboard.py`: + +```python +#!/usr/bin/env python3 +"""Report what a session has not resolved yet, and write .env values. (A) + +This script REPORTS. It never asks and it never decides, because no +harness-neutral mechanism exists for a shell script to run an interactive +prompt, and a hook injects text rather than conversing. The split is fixed: + + this script -> detect and report + the agent -> ask, using the interview in .agents/workflow.md + agent-role.py -> make the answer a fact + + scripts/agent-onboard.py --probe # human-readable state + scripts/agent-onboard.py --probe --json # machine-readable + +--env-set arrives in step 4. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import json +import subprocess +import sys +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] + + +def _load(name: str, relative: str): + path = ROOT / relative + spec = importlib.util.spec_from_file_location(name, path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +role_mod = _load("agent_role", "scripts/agent-role.py") + +ENV_EXAMPLE = ROOT / ".env.example" +ENV_FILE = ROOT / ".env" + + +def _example_keys() -> tuple[str, ...]: + """Keys the tracked example declares. The ONLY source of legal keys.""" + keys = [] + for line in ENV_EXAMPLE.read_text(encoding="utf-8").splitlines(): + if line and not line.startswith("#") and "=" in line: + keys.append(line.split("=", 1)[0]) + return tuple(keys) + + +ENV_KEYS = _example_keys() + + +def env_state_from_text(text: str) -> tuple[str, list[str]]: + """Classify .env content: present | incomplete, plus the unset keys.""" + values = {} + for line in text.splitlines(): + if line and not line.startswith("#") and "=" in line: + key, value = line.split("=", 1) + values[key.strip()] = value.strip() + missing = [key for key in ENV_KEYS if not values.get(key)] + return ("incomplete" if missing else "present"), missing + + +def env_state(path: Path = ENV_FILE) -> tuple[str, list[str]]: + """Classify the .env file. A missing FILE is distinct from missing VALUES.""" + if not path.exists(): + return "missing", list(ENV_KEYS) + return env_state_from_text(path.read_text(encoding="utf-8")) + + +def ready_rows() -> list[str]: + """The READY queue, from the existing checker rather than a second parser.""" + result = subprocess.run( + [sys.executable, str(ROOT / "scripts/ready-for-helper.py")], + cwd=ROOT, capture_output=True, text=True, check=False, + ) + if result.returncode != 0: + return [] + rows = [] + for line in result.stdout.splitlines(): + token = line.strip().strip("`").split()[0] if line.strip() else "" + if token.isupper() and "-" in token: + rows.append(token) + return rows + + +def probe() -> dict: + state = role_mod.resolve() + status, missing = env_state() + 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")), + "reason": state.get("reason"), + # Absent until step 2 teaches resolve() about mode. Rendered as a + # default rather than a declaration, because headless is never + # inferred and neither is interactive. + "mode": state.get("mode"), + "env": status, + "env_missing": missing, + "queue": ready_rows(), + } + + +def render_probe(state: dict) -> str: + role = state["role"] or "UNDECLARED" + row = f" row={state['row']}" if state.get("row") else "" + mode = state.get("mode") or "interactive (default, not declared)" + lines = [ + f"role: {role}{row} mode: {mode}", + f".env: {state['env']}" + + (f" (unset: {', '.join(state['env_missing'])})" if state["env_missing"] else ""), + f"queue: {len(state['queue'])} READY rows" + + (f" — {', '.join(state['queue'][:5])}" if state["queue"] else ""), + ] + 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 ; " + "just looking -> read-only. See .agents/workflow.md." + ) + return "\n".join(lines) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Report unresolved session state.") + parser.add_argument("--probe", action="store_true", help="report session state") + parser.add_argument("--json", action="store_true", help="machine-readable probe") + args = parser.parse_args(argv) + + state = probe() + print(json.dumps(state, indent=2, sort_keys=True) if args.json else render_probe(state)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) +``` + +Make it executable: `chmod +x scripts/agent-onboard.py` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: PASS, 11 tests. + +- [ ] **Step 5: Smoke-test against the real repo** + +Run: `python3 scripts/agent-onboard.py --probe` +Expected: three lines plus the interview hint, since this session has no role marker and no `.env`. + +Run: `python3 scripts/agent-onboard.py --probe --json | python3 -c "import json,sys; d=json.load(sys.stdin); print(sorted(d))"` +Expected 9 keys: `['blocked_by_other_operator', 'env', 'env_missing', 'mode', 'queue', 'queue_error', 'reason', 'role', 'row']` + +- [ ] **Step 6: Update the owed doc surfaces, run preflight, commit** + +Add one short line to `docs/STATUS.md` and extend the existing "NOT APPLICABLE" paragraph in `docs/BENCHMARKS.md` (do not add a new paragraph — the page is at its 35-paragraph budget). Check `docs/STATUS.md` stays under the ratchet in `scripts/check-public-doc-tables.py`. + +```bash +bash scripts/agent-preflight.sh > /tmp/pf.log 2>&1; echo "EXIT=$?" +git add scripts/agent-onboard.py tests/scripts/test_agent_onboard.py docs/STATUS.md docs/BENCHMARKS.md +git commit -F - <<'EOF' +tools(onboard): probe the unresolved session state, ask nothing (A step 1) + +A script cannot run an interactive prompt in any harness-neutral way, so the +probe reports and the agent asks. .env.example is the only source of legal +keys, and a missing FILE is reported distinctly from missing VALUES. + +FOLLOWING_AGENTS_PROTOCOL +Assisted-by: Claude Code:claude-opus-5 [ClaudeCode] +EOF +python3 scripts/check-doc-checkpoint.py --commit "$(git rev-parse HEAD)"; echo "doc-checkpoint EXIT=$?" +``` + +--- + +### Task 2: `read-only` and `--headless` + +**Files:** +- Modify: `scripts/agent-role.py:41` (`ROLES`), `cmd_claim`, `resolve`, `main` +- Test: `tests/scripts/test_agent_role.py` + +**Interfaces:** +- Produces: `CLAIMABLE_ROLES = ("operator", "helper")`; `DECLARABLE = ("operator", "helper", "read-only")`; `resolve()` gains a `"mode"` key valued `"interactive"` or `"headless"`; `agent-role.py claim read-only` and `claim --headless`. + +**The distinction that matters:** `read-only` is a *declared absence of claim*, not a third role. It takes no lock and creates no worktree. `CLAIMABLE_ROLES` stays exactly two, and any code that asks "may this session write?" tests membership in `CLAIMABLE_ROLES`, never `DECLARABLE`. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/scripts/test_agent_role.py`, above its `if __name__` block: + +```python +class ReadOnlyAndModeTests(unittest.TestCase): + def test_claimable_roles_stay_exactly_two(self): + # read-only must never become a third claimable role: it takes no lock + # and no worktree, and every write path keys on CLAIMABLE_ROLES. + self.assertEqual(role.CLAIMABLE_ROLES, ("operator", "helper")) + self.assertIn("read-only", role.DECLARABLE) + self.assertNotIn("read-only", role.CLAIMABLE_ROLES) + + def test_read_only_is_declarable(self): + self.assertIn("read-only", role.DECLARABLE) + + def test_the_roles_alias_is_not_widened(self): + # ROLES means "may write". Widening it to DECLARABLE would silently let + # read-only through every existing write-gating call site, and today + # that mutation leaves the whole suite green. + self.assertEqual(role.ROLES, role.CLAIMABLE_ROLES) + + def test_mode_defaults_to_interactive(self): + # Headless is DECLARED, never inferred. Absent an explicit flag the + # session is interactive. + self.assertEqual(role.mode_from_marker({}), "interactive") + self.assertEqual(role.mode_from_marker({"mode": "headless"}), "headless") + self.assertEqual(role.mode_from_marker({"mode": "nonsense"}), "interactive") +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python3 tests/scripts/test_agent_role.py -v` +Expected: FAIL with `AttributeError: module 'agent_role' has no attribute 'CLAIMABLE_ROLES'`. + +- [ ] **Step 3: Write minimal implementation** + +In `scripts/agent-role.py`, replace the `ROLES` definition at line 41: + +```python +# read-only is a declared ABSENCE of claim, not a third role: it takes no lock +# and creates no worktree. Every "may this session write?" test keys on +# CLAIMABLE_ROLES. Without it, a session that only reads must either take the +# repo-wide operator lock or create a throwaway worktree, and faced with that +# people reach for --no-require-role until the gate means nothing. +CLAIMABLE_ROLES = ("operator", "helper") +DECLARABLE = (*CLAIMABLE_ROLES, "read-only") +ROLES = CLAIMABLE_ROLES # retained: existing call sites mean "may write" + + +def mode_from_marker(marker: dict) -> str: + """Interactive unless headless was DECLARED. Never inferred.""" + return "headless" if marker.get("mode") == "headless" else "interactive" +``` + +In `resolve()`, change the marker acceptance test from `marker.get("role") in ROLES` to `marker.get("role") in DECLARABLE`, add `"mode": mode_from_marker(marker)` to the returned dict on the declared path, and return `"mode": "interactive"` on the undeclared path. A `read-only` marker needs no lock, so skip the operator lock check for it: + +```python + if marker and marker.get("session") == me and marker.get("role") in DECLARABLE: + declared = marker["role"] + if declared == "operator": + if not lock or lock.get("session") != me: + return { + "role": None, + "session": me, + "mode": "interactive", + "reason": "operator marker without a held lock; re-claim", + "branch": current_branch(), + } + return { + "role": declared, + "row": marker.get("row"), + "session": me, + "branch": current_branch(), + "mode": mode_from_marker(marker), + "reason": "declared", + } +``` + +In `cmd_claim`, write the mode into the marker and skip the lock for `read-only` (the existing `if role == "operator":` block already does this by construction — no change needed there): + +```python + marker_path().write_text( + json.dumps({ + "role": role, + "row": args.row, + "session": me, + "mode": "headless" if args.headless else "interactive", + "at": time.time(), + }), + encoding="utf-8", + ) +``` + +In `main()`, widen the claim parser's choices and add the flag: + +```python + claim = sub.add_parser("claim", help="declare and materialize a role") + claim.add_argument("role", choices=DECLARABLE) + claim.add_argument("--row", help="the row a helper is taking") + claim.add_argument( + "--headless", + action="store_true", + help="unattended run: decide and record rather than ask (never inferred)", + ) +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python3 tests/scripts/test_agent_role.py -v` +Expected: PASS, all existing tests plus 3. + +- [ ] **Step 5: Verify the real CLI end to end** + +```bash +python3 scripts/agent-role.py claim read-only +python3 scripts/agent-role.py show +python3 scripts/agent-role.py release +``` +Expected: claims without taking a lock, `show` prints `role=read-only`, release is clean. Confirm no file appeared in the git common dir: `ls "$(git rev-parse --git-common-dir)"/agent-operator.lock 2>/dev/null` prints nothing. + +- [ ] **Step 6: Doc surfaces, preflight, commit** + +Same doc obligation as Task 1. + +```bash +bash scripts/agent-preflight.sh > /tmp/pf.log 2>&1; echo "EXIT=$?" +git add scripts/agent-role.py tests/scripts/test_agent_role.py docs/STATUS.md docs/BENCHMARKS.md +git commit -F - <<'EOF' +tools(role): declare read-only and headless (A step 2) + +read-only is a declared ABSENCE of claim, not a third role: no lock, no +worktree, and CLAIMABLE_ROLES stays exactly two so every write path keeps +keying on it. Headless is declared with the role and never inferred. + +FOLLOWING_AGENTS_PROTOCOL +Assisted-by: Claude Code:claude-opus-5 [ClaudeCode] +EOF +python3 scripts/check-doc-checkpoint.py --commit "$(git rev-parse HEAD)"; echo "doc-checkpoint EXIT=$?" +``` + +--- + +### Task 3: `--require-role` becomes the default + +**Files:** +- Modify: `scripts/agent-preflight.sh` — line 23 (`REQUIRE_ROLE=0`), the arg loop at 24–32, the role block at 84–90, the `SUITES` list +- Test: `tests/scripts/test_agent_onboard.py` + +**Interfaces:** +- Consumes: `agent-role.py`'s `resolve()` and its `--json` output; `CLAIMABLE_ROLES` from Task 2. +- Produces: preflight fails on an undeclared role by default; `--no-require-role` opts out; `--staged` fails for a `read-only` session. + +**The two behaviours that are easy to get backwards:** a `read-only` session **passes** a plain preflight (that is the whole point of the third answer) but **fails** `--staged`, because staging means writing. And the failure message must carry the interview, not just an error code — a gate that tells you what to do next is the difference between a protocol people follow and one they route around. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/scripts/test_agent_onboard.py`, above its `if __name__` block: + +```python +class PreflightWiringTests(unittest.TestCase): + TEXT = (ROOT / "scripts/agent-preflight.sh").read_text(encoding="utf-8") + + def test_require_role_defaults_on(self): + self.assertIn("REQUIRE_ROLE=1", self.TEXT) + self.assertNotIn("REQUIRE_ROLE=0", self.TEXT) + + def test_opt_out_flag_exists(self): + self.assertIn("--no-require-role", self.TEXT) + + def test_failure_text_carries_the_interview(self): + # An error code alone gets routed around. The gate must say what to ask. + self.assertIn("claim read-only", self.TEXT) + self.assertIn("claim helper --row", self.TEXT) + + def test_staged_refuses_read_only(self): + self.assertIn("read-only", self.TEXT) + self.assertIn("STAGED", self.TEXT) + + def test_onboard_suite_is_registered(self): + self.assertIn("test_agent_onboard", self.TEXT) + + def test_read_only_alone_does_not_satisfy_a_write_gate(self): + # agent-role.py show exits 0 for read-only, so --require-role is + # satisfied by a declared ABSENCE of claim. That is correct for a plain + # run and wrong for --staged; the refusal must be explicit. + self.assertIn("read-only-cannot-stage", self.TEXT) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: FAIL on `test_require_role_defaults_on` (`REQUIRE_ROLE=0` is present) and on the flag/interview assertions. + +- [ ] **Step 3: Write minimal implementation** + +In `scripts/agent-preflight.sh`, change line 23 and the arg loop: + +```sh +REQUIRE_ROLE=1 +``` + +```sh + --require-role) REQUIRE_ROLE=1 ;; + --no-require-role) REQUIRE_ROLE=0 ;; +``` + +Replace the role block (lines 84–90) with: + +```sh +else + printf ' \033[33m--\033[0m %s\n' "$(printf '%s' "$role_line" | head -1)" + printf ' This session has not declared a role. Ask what the work is:\n' + printf ' a long or multi-step campaign -> scripts/agent-role.py claim operator\n' + printf ' one scoped change -> scripts/agent-role.py claim helper --row \n' + printf ' just reading or answering -> scripts/agent-role.py claim read-only\n' + printf ' Add --headless to an unattended run. See .agents/workflow.md.\n' + if [ "$REQUIRE_ROLE" -eq 1 ]; then + failed+=("role-undeclared") + fi +fi + +# read-only PASSES a plain preflight -- that is the point of the third answer. +# It fails --staged, because staging is writing. +if [ "$STAGED" -eq 1 ] && printf '%s' "$role_line" | grep -q 'role=read-only'; then + printf ' \033[31mFAIL\033[0m read-only sessions do not write. Claim operator or helper first.\n' + failed+=("read-only-cannot-stage") +fi +``` + +Add `test_agent_onboard` to the `SUITES` list, and update the usage comment block at the top of the file so `--help` documents `--no-require-role`. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: PASS, 12 tests. + +- [ ] **Step 5: Verify both directions on the real gate** + +```bash +python3 scripts/agent-role.py release 2>/dev/null +bash scripts/agent-preflight.sh > /tmp/pf-undeclared.log 2>&1; echo "undeclared EXIT=$?" # expect 1 +bash scripts/agent-preflight.sh --no-require-role > /tmp/pf-optout.log 2>&1; echo "opt-out EXIT=$?" # expect 0 +python3 scripts/agent-role.py claim read-only +bash scripts/agent-preflight.sh > /tmp/pf-readonly.log 2>&1; echo "read-only EXIT=$?" # expect 0 +bash scripts/agent-preflight.sh --staged > /tmp/pf-staged.log 2>&1; echo "read-only staged EXIT=$?" # expect 1 +``` + +Expected exactly: `1`, `0`, `0`, `1`. If the read-only plain run fails, the third answer is broken and the gate will be routed around — fix it before continuing. + +- [ ] **Step 6: Claim a real role, then doc surfaces, preflight, commit** + +You now need a claimable role to commit. `python3 scripts/agent-role.py claim helper --row ` — or `operator` if this session holds `main`. + +```bash +bash scripts/agent-preflight.sh > /tmp/pf.log 2>&1; echo "EXIT=$?" +git add scripts/agent-preflight.sh tests/scripts/test_agent_onboard.py docs/STATUS.md docs/BENCHMARKS.md +git commit -F - <<'EOF' +gate(preflight): demand a role by default (A step 3) + +The obligation already existed in prose and in an opt-in flag, and neither +fired. --require-role is now the default with --no-require-role to opt out, +and the failure carries the interview rather than an error code, because a +gate that does not say what to do next is a gate people route around. + +read-only passes a plain preflight and fails --staged: reading is free, +writing is a claim. + +FOLLOWING_AGENTS_PROTOCOL +Assisted-by: Claude Code:claude-opus-5 [ClaudeCode] +EOF +python3 scripts/check-doc-checkpoint.py --commit "$(git rev-parse HEAD)"; echo "doc-checkpoint EXIT=$?" +``` + +--- + +### Task 4: Just-in-time `.env` + +**Files:** +- Modify: `scripts/agent-onboard.py` (`cmd_env_set` already exists from Task 1 — this task tests and hardens it) +- Test: `tests/scripts/test_agent_onboard.py` + +**Interfaces:** +- Consumes: `ENV_KEYS`, `ENV_FILE`, `ENV_EXAMPLE` from Task 1. +- Produces: `cmd_env_set(pair: str) -> int`, and `--env-set KEY=VALUE` on `main()`. + +**Why this is its own task:** `--env-set` writes to an untracked file that gates read, so an unrecognised key or a clobbered line fails silently and surfaces later as a mysteriously `PENDING` gate. Task 1 deliberately ships the probe WITHOUT it, so these tests have a real RED to go green from. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/scripts/test_agent_onboard.py`, above its `if __name__` block: + +```python +class EnvSetTests(unittest.TestCase): + def setUp(self): + self.tmp = Path(tempfile.mkdtemp()) + self.env = self.tmp / ".env" + self._real = onboard.ENV_FILE + onboard.ENV_FILE = self.env + + def tearDown(self): + onboard.ENV_FILE = self._real + shutil.rmtree(self.tmp) + + def test_unknown_key_is_refused(self): + # Never invent a key: a typo'd name would sit in .env doing nothing + # while the gate that wanted it stays mysteriously PENDING. + self.assertEqual(onboard.cmd_env_set("NOT_A_REAL_KEY=/x"), 2) + self.assertFalse(self.env.exists()) + + def test_missing_pair_is_refused(self): + self.assertEqual(onboard.cmd_env_set("VLLM_ORACLE"), 2) + + def test_first_write_seeds_from_the_example(self): + self.assertEqual(onboard.cmd_env_set(f"{onboard.ENV_KEYS[0]}=/oracle"), 0) + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{onboard.ENV_KEYS[0]}=/oracle", text) + # every other declared key survives, so nothing is silently dropped + for key in onboard.ENV_KEYS: + self.assertIn(key, text) + + def test_second_write_updates_in_place_without_duplicating(self): + key = onboard.ENV_KEYS[0] + onboard.cmd_env_set(f"{key}=/first") + onboard.cmd_env_set(f"{key}=/second") + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{key}=/second", text) + self.assertNotIn("/first", text) + self.assertEqual(sum(1 for l in text.splitlines() if l.startswith(f"{key}=")), 1) + + def test_other_keys_are_not_disturbed(self): + a, b = onboard.ENV_KEYS[0], onboard.ENV_KEYS[1] + onboard.cmd_env_set(f"{a}=/aaa") + onboard.cmd_env_set(f"{b}=/bbb") + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{a}=/aaa", text) + self.assertIn(f"{b}=/bbb", text) +``` + +Add `import shutil` and `import tempfile` to the test file's import block. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: FAIL, 5 errors — `AttributeError: module 'agent_onboard' has no attribute 'cmd_env_set'`. + +- [ ] **Step 3: Write minimal implementation** + +Append to `scripts/agent-onboard.py`, above `main()`: + +```python +def cmd_env_set(pair: str) -> int: + """Write one .env value. Refuses any key .env.example does not declare.""" + if "=" not in pair: + print("ERROR: expected KEY=VALUE", file=sys.stderr) + return 2 + key, value = pair.split("=", 1) + key = key.strip() + if key not in ENV_KEYS: + # A typo would sit in .env doing nothing while the gate that wanted the + # real key stays mysteriously PENDING. + print( + f"ERROR: {key} is not declared in .env.example. Never invent a key; " + f"legal keys are: {', '.join(ENV_KEYS)}", + file=sys.stderr, + ) + return 2 + if not ENV_FILE.exists(): + ENV_FILE.write_text(ENV_EXAMPLE.read_text(encoding="utf-8"), encoding="utf-8") + lines = ENV_FILE.read_text(encoding="utf-8").splitlines() + for index, line in enumerate(lines): + if not line.startswith("#") and line.split("=", 1)[0].strip() == key: + lines[index] = f"{key}={value}" + break + else: + lines.append(f"{key}={value}") + ENV_FILE.write_text("\n".join(lines) + "\n", encoding="utf-8") + print(f"set {key} in .env") + return 0 +``` + +In `main()`, add the flag and dispatch before the probe: + +```python + parser.add_argument("--env-set", metavar="KEY=VALUE", help="write one .env value") +``` + +```python + if args.env_set: + return cmd_env_set(args.env_set) +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python3 tests/scripts/test_agent_onboard.py -v` +Expected: PASS, 17 tests. + +- [ ] **Step 5: Doc surfaces, preflight, commit** + +```bash +bash scripts/agent-preflight.sh > /tmp/pf.log 2>&1; echo "EXIT=$?" +git add scripts/agent-onboard.py tests/scripts/test_agent_onboard.py docs/STATUS.md docs/BENCHMARKS.md +git commit -F - <<'EOF' +tools(onboard): make --env-set safe to call repeatedly (A step 4) + +It writes an untracked file that gates read, so an unrecognised key or a +clobbered line fails silently and surfaces later as a mysteriously PENDING +gate. Unknown keys are refused, the first write seeds from .env.example, and +repeat writes update in place without duplicating or disturbing other keys. + +FOLLOWING_AGENTS_PROTOCOL +Assisted-by: Claude Code:claude-opus-5 [ClaudeCode] +EOF +python3 scripts/check-doc-checkpoint.py --commit "$(git rev-parse HEAD)"; echo "doc-checkpoint EXIT=$?" +``` + +--- + +### Task 5: The prose, moved with the gate + +**Files:** +- Modify: `.agents/workflow.md` (after `## Session protocol`, line 6) +- Modify: `AGENTS.md` (the T0 role bullet) +- Modify: `.agents/specs/operator-helper-protocol.md` +- Modify: `scripts/check-protocol-consistency.py` +- Test: `tests/scripts/test_check_protocol_consistency.py` + +**Interfaces:** +- Consumes: everything from Tasks 1–4. +- Produces: `INTERVIEW_MARKER = ""` in `check-protocol-consistency.py`, asserting the interview block exists in `.agents/workflow.md`. + +**Why the checker moves in the same commit:** `check-protocol-consistency.py` exists because an obligation was once migrated in `AGENTS.md` and the checker but not in the manual, which went on instructing agents to do the thing the migration had removed. Prose is what agents actually read. A gate whose prose lives nowhere is the same failure with the polarity flipped. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/scripts/test_check_protocol_consistency.py`, above its `if __name__` block: + +```python +class InterviewBlockTests(unittest.TestCase): + def test_workflow_carries_the_role_interview(self): + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + self.assertIn(consistency.INTERVIEW_MARKER, text) + self.assertIn("read-only", text) + self.assertIn("claim helper --row", text) + + def test_checker_rejects_a_workflow_without_the_interview(self): + # The mutation this gate exists to catch: the gate ships, the prose + # does not, and agents never learn the precondition. + errors = consistency.interview_errors("# workflow\n\nno interview here\n") + self.assertTrue(errors) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `python3 tests/scripts/test_check_protocol_consistency.py -v` +Expected: FAIL with `AttributeError: module … has no attribute 'INTERVIEW_MARKER'`. + +- [ ] **Step 3: Write the prose and the checker** + +In `.agents/workflow.md`, immediately after the `## Session protocol` heading, insert: + +```markdown + +### First question of every session + +`scripts/agent-preflight.sh` fails until this session has declared a role. Ask +what the work is — not which role the developer wants, which is vocabulary they +should not have to learn first. + +| What are you here to do? | Claim | What it means | +|---|---|---| +| A long or multi-step campaign — several changes, a benchmark grid, a whole row block | `scripts/agent-role.py claim operator` | Owns `main` and the GPU. Merges PRs first. Drives feature work through sub-agents rather than writing it. One at a time, repo-wide. | +| One scoped change — a fix, a port, a single row | `scripts/agent-role.py claim helper --row ` | Isolated worktree on `row/`, draft PR opened at the START. That PR **is** the claim. Never touches `main`. | +| Just looking — reading code, answering a question | `scripts/agent-role.py claim read-only` | No lock, no worktree, no claim. Passes preflight; every write path refuses until you claim a real role. | + +`read-only` is a declared **absence** of claim, not a third role. Escalating is +one command. + +Add `--headless` when the developer has said the run is unattended: decide, +record each decision in `.agents/state.md`, never block, never merge, park what +will not go green. Headless is **declared, never inferred** — not from the hour, +not from silence, not from a long task. + +`.env` is asked **just in time**: when a gate needs a value, ask for that value +and write it with `scripts/agent-onboard.py --env-set KEY=VALUE`. Never walk the +whole template up front, and never infer a value from a username, a path or a +machine identity. Unanswered means empty, and empty means the gates that need it +stay `PENDING`. + +Run `scripts/agent-onboard.py --probe` to see what is still unresolved. + +``` + +In `scripts/check-protocol-consistency.py`, add: + +```python +INTERVIEW_MARKER = "" +INTERVIEW_REQUIRED = ("claim operator", "claim helper --row", "claim read-only", "--headless") + + +def interview_errors(text: str) -> list[str]: + """The role interview must live where agents read it, not only in a gate.""" + if INTERVIEW_MARKER not in text: + return [".agents/workflow.md is missing the role-interview block"] + return [ + f".agents/workflow.md role interview omits {needle!r}" + for needle in INTERVIEW_REQUIRED + if needle not in text + ] +``` + +Call `interview_errors` from `main()` against `.agents/workflow.md` and add its output to the error list. + +**Two residual false claims must die in this task** (found by Task 3's re-review, both outside the lines Task 3 corrected, both passing every existing gate): + +- `.agents/NOW.md` still says "role discipline ENFORCING, `--require-role` still opt-in" — directly contradicted by Task 3's deliverable. +- `.agents/specs/operator-helper-protocol.md:203`'s W0 table row still says "session-scoped role marker" — the exact claim corrected 130 lines earlier in the same file. + +**And add the behavioural pin the text anchors cannot give.** Every assertion in `PreflightWiringTests` greps text, so an *override* still slips through even though a *rewrite* of the default is caught: keeping `REQUIRE_ROLE=1` and adding `REQUIRE_ROLE=0 ` (one trailing space) on the next line leaves the suite green while the gate stops failing. Add a test that RUNS the script: + +```python + def test_preflight_actually_fails_on_an_undeclared_role(self): + # Every other assertion here greps text, so an override on a later line + # slips through. Only executing the script closes the class. + env = dict(os.environ, VLLM_CPP_AGENT_SESSION="probe-no-such-session") + result = subprocess.run( + ["bash", str(ROOT / "scripts/agent-preflight.sh"), "--quiet"], + cwd=ROOT, capture_output=True, text=True, check=False, env=env, + ) + self.assertNotEqual(result.returncode, 0) + self.assertIn("role-undeclared", result.stdout + result.stderr) +``` + +This test must run in a state with no resolvable role. If the worktree carries a marker, the test is meaningless — assert the precondition or skip loudly, never silently pass. + +Update `AGENTS.md`'s T0 role bullet to say the role is asked as the first question of the session, that `read-only` is available, and that preflight demands it by default. Update `.agents/specs/operator-helper-protocol.md` § "Determining the role" to record `read-only` and the mode declaration. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `python3 tests/scripts/test_check_protocol_consistency.py -v` +Expected: PASS. + +- [ ] **Step 5: Verify the whole gate suite** + +```bash +python3 scripts/check-protocol-consistency.py; echo "consistency EXIT=$?" +bash scripts/agent-preflight.sh > /tmp/pf.log 2>&1; echo "preflight EXIT=$?" +``` +Expected: both `EXIT=0`. + +- [ ] **Step 6: Doc surfaces, preflight, commit** + +```bash +git add AGENTS.md .agents/workflow.md .agents/specs/operator-helper-protocol.md \ + scripts/check-protocol-consistency.py tests/scripts/test_check_protocol_consistency.py \ + docs/STATUS.md docs/BENCHMARKS.md +git commit -F - <<'EOF' +docs(protocol): the role interview ships with the gate that demands it (A step 5) + +check-protocol-consistency.py exists because an obligation was once migrated in +AGENTS.md and the checker but not in the manual, which went on instructing +agents to do the thing the migration had removed. A gate whose prose lives +nowhere is that failure with the polarity flipped, so the interview lands in +workflow.md and the checker asserts it is there. + +FOLLOWING_AGENTS_PROTOCOL +Assisted-by: Claude Code:claude-opus-5 [ClaudeCode] +EOF +python3 scripts/check-doc-checkpoint.py --commit "$(git rev-parse HEAD)"; echo "doc-checkpoint EXIT=$?" +``` + +--- + +## Done when + +- `scripts/agent-onboard.py --probe` reports role, mode, `.env` state and the `READY` queue, and writes nothing. +- `agent-preflight.sh` fails on an undeclared role by default; `--no-require-role` opts out; a `read-only` session passes plainly and fails `--staged`. +- `agent-role.py claim read-only` takes no lock and creates no worktree; `--headless` is recorded in the marker. +- `--env-set` refuses unknown keys, seeds from `.env.example`, and updates in place. +- The interview lives in `.agents/workflow.md` and `check-protocol-consistency.py` fails without it. +- Every commit passes `check-doc-checkpoint.py --commit ` in its committed form. + +## Out of scope + +Subsystem B — the orchestration harness: how an operator runs a row through implementer subagents with an independent reviewer, the gate-command discipline that makes a row's `Gates` field a real Verify, and the headless execution loop. It gets its own spec and plan. Also out of scope: generating `.agents/developer-preferences.md`, and any Claude-Code-specific hook. diff --git a/scripts/agent-onboard.py b/scripts/agent-onboard.py new file mode 100755 index 00000000..db915bcd --- /dev/null +++ b/scripts/agent-onboard.py @@ -0,0 +1,284 @@ +#!/usr/bin/env python3 +"""Report what a session has not resolved yet, and record what it answered. (A) + +This script REPORTS, and writes exactly one thing: a .env value it was handed. +It never asks and it never decides, because no harness-neutral mechanism exists +for a shell script to run an interactive prompt, and a hook injects text rather +than conversing. The split is fixed: + + this script -> detect and report; record an answer it is given + the agent -> ask, using the interview in .agents/workflow.md + agent-role.py -> make the answer a fact + + scripts/agent-onboard.py --probe # human-readable state + scripts/agent-onboard.py --probe --json # machine-readable + scripts/agent-onboard.py --env-set KEY=VALUE # record one answered value + +`--env-set` never invents anything. Only keys .env.example declares may be +written, and an unanswered key stays EMPTY -- empty means the gates that need +it stay PENDING, which is an honest state and not a failure. Never infer a +value from a username, a filesystem path, a machine identity or another +developer's setup. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import json +import shlex +import sys +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] + + +def _load(name: str, relative: str): + path = ROOT / relative + spec = importlib.util.spec_from_file_location(name, path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +role_mod = _load("agent_role", "scripts/agent-role.py") + +ENV_EXAMPLE = ROOT / ".env.example" +ENV_FILE = ROOT / ".env" + + +def _example_keys() -> tuple[str, ...]: + """Keys the tracked example declares. The ONLY source of legal keys.""" + keys = [] + for line in ENV_EXAMPLE.read_text(encoding="utf-8").splitlines(): + if line and not line.startswith("#") and "=" in line: + keys.append(line.split("=", 1)[0]) + return tuple(keys) + + +ENV_KEYS = _example_keys() + + +def env_state_from_text(text: str) -> tuple[str, list[str]]: + """Classify .env content: present | incomplete, plus the unset keys.""" + values = {} + for line in text.splitlines(): + if line and not line.startswith("#") and "=" in line: + key, value = line.split("=", 1) + values[key.strip()] = value.strip() + missing = [key for key in ENV_KEYS if not values.get(key)] + return ("incomplete" if missing else "present"), missing + + +def env_state(path: Path = ENV_FILE) -> tuple[str, list[str]]: + """Classify the .env file. A missing FILE is distinct from missing VALUES. + + A file that exists but cannot be read is a THIRD case: reporting it as + absent would send the agent to create a file that is already there, and + reporting it as complete would hide every unresolved value. It gets its + own status, and like a missing file it resolves nothing. + """ + if not path.exists(): + return "missing", list(ENV_KEYS) + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return "unreadable", list(ENV_KEYS) + return env_state_from_text(text) + + +def queue_state() -> tuple[list[str], str | None]: + """The READY queue, plus why it is unavailable when it is. + + ready-for-helper.py's `queue()` IS the computation, so the probe calls it + instead of re-parsing that script's prose: its listing truncates at 40 rows + and its header line ("READY-FOR-HELPER queue: N row(s)") reads like a row + ID to any token filter, so an empty queue would report a phantom row. + + A queue that could not be computed is NOT an empty queue, exactly as an + unreadable .env is not an absent one, so the failure is carried out rather + than swallowed. The probe still never raises: it reports and does not gate. + """ + try: + helper = sys.modules.get("ready_for_helper") or _load( + "ready_for_helper", "scripts/ready-for-helper.py" + ) + pickable, _ = helper.queue() + except Exception as error: # a broken record must not crash the probe + return [], f"{type(error).__name__}: {error}" + return [row.item_id for row in pickable], None + + +def ready_rows() -> list[str]: + """The READY queue alone. Callers that must tell empty from broken apart + use queue_state().""" + return queue_state()[0] + + +def probe() -> dict: + state = role_mod.resolve() + status, missing = env_state() + rows, queue_error = queue_state() + 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")), + "reason": state.get("reason"), + # resolve() now carries this (step 2). Still read with .get and still + # rendered as a DEFAULT when absent: headless is never inferred, so a + # state that carries no mode must not read as a declaration either. + "mode": state.get("mode"), + "env": status, + "env_missing": missing, + "queue": rows, + "queue_error": queue_error, + } + + +def render_probe(state: dict) -> str: + role = state["role"] or "UNDECLARED" + row = f" row={state['row']}" if state.get("row") else "" + mode = state.get("mode") or "interactive (default, not declared)" + if state.get("queue_error"): + queue_line = f"queue: UNAVAILABLE ({state['queue_error']})" + else: + queue_line = f"queue: {len(state['queue'])} READY rows" + ( + f" — {', '.join(state['queue'][:5])}" if state["queue"] else "" + ) + lines = [ + f"role: {role}{row} mode: {mode}", + f".env: {state['env']}" + + (f" (unset: {', '.join(state['env_missing'])})" if state["env_missing"] else ""), + queue_line, + ] + 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 ; " + "just looking -> read-only. See .agents/workflow.md." + ) + return "\n".join(lines) + + +def _render(value: str) -> str: + """Render a value so the file's TWO readers agree on it. + + .env.example documents the loader as `set -a; . ./.env; set +a`, so the file + is shell as well as data. An unquoted `/pa th` makes that loader run `th` + ("command not found") and leave the variable EMPTY, while the probe's own + parser reads the same line as a set value -- the probe reports PRESENT and + the gate stays PENDING, which is the exact confusion this command exists to + prevent. shlex.quote only adds quotes when they are needed, so ordinary + paths are written unchanged. + + Empty is the one value left bare: shlex.quote("") is `''`, and the probe + counts that two-character string as SET. Unanswered must keep reading as + unanswered. + """ + return shlex.quote(value) if value else "" + + +def cmd_env_set(pair: str) -> int: + """Write one .env value. Refuses any key .env.example does not declare.""" + if "=" not in pair: + print("ERROR: expected KEY=VALUE", file=sys.stderr) + return 2 + key, value = pair.split("=", 1) + key = key.strip() + if key not in ENV_KEYS: + # A typo would sit in .env doing nothing while the gate that wanted the + # real key stays mysteriously PENDING. + print( + f"ERROR: {key} is not declared in .env.example. Never invent a key; " + f"legal keys are: {', '.join(ENV_KEYS)}", + file=sys.stderr, + ) + return 2 + # The value is the one field nothing else validates. A line separator inside + # it forges a whole extra .env line that no key check ever saw -- the same + # silent clobber as an unrecognised key, through the back door. + # + # Ask the QUESTION rather than enumerate characters: "\n" and "\r" are 2 of + # the 10 separators str.splitlines() breaks on, and it is splitlines() that + # both env_state_from_text and the rewrite below use, so a "\v" or a U+2028 + # smuggled a forged pair past the key check and the probe then reported the + # forged key as SET. An empty value splits to [] and is legal, so it is the + # one case this cannot phrase as a round trip. + if value and value.splitlines() != [value]: + print( + f"ERROR: the value for {key} contains a line separator, which would " + "forge a second .env line. Pass a single-line value.", + file=sys.stderr, + ) + return 2 + if not ENV_FILE.exists(): + # Seed from the tracked example so every OTHER declared key survives, + # commented and empty, instead of a one-line .env that hides the rest. + ENV_FILE.write_text(ENV_EXAMPLE.read_text(encoding="utf-8"), encoding="utf-8") + try: + lines = ENV_FILE.read_text(encoding="utf-8").splitlines() + except (OSError, UnicodeDecodeError) as error: + # env_state treats an existing-but-unreadable .env as its own third + # case rather than as absent. Writing must agree: a bare traceback + # tells the caller nothing, and "create one" is the wrong instruction + # for a file that is already there. + print(f"ERROR: .env exists but cannot be read ({error})", file=sys.stderr) + return 2 + # Rewrite EVERY match, not just the first. A hand-maintained .env routinely + # carries an override appended at the bottom, and both readers here and + # `set -a; . ./.env` take the LAST assignment, so breaking on the first left + # the file changed, the exit code 0, the message reassuring -- and the + # effective value exactly what it was. Collapse to one line, keeping the + # first line's position so the example's grouping and comments still read. + matches = [ + index + for index, line in enumerate(lines) + if not line.startswith("#") and line.split("=", 1)[0].strip() == key + ] + if matches: + lines[matches[0]] = f"{key}={_render(value)}" + for index in reversed(matches[1:]): + del lines[index] + else: + lines.append(f"{key}={_render(value)}") + ENV_FILE.write_text("\n".join(lines) + "\n", encoding="utf-8") + # An empty value is a legitimate answer -- it means UNAVAILABLE, and the + # gates that need it stay PENDING. Say so rather than let it read as a win. + print(f"set {key} in .env" + ("" if value else " (empty: gates stay PENDING)")) + return 0 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Report unresolved session state.") + parser.add_argument("--probe", action="store_true", help="report session state") + parser.add_argument("--json", action="store_true", help="machine-readable probe") + parser.add_argument("--env-set", metavar="KEY=VALUE", help="write one .env value") + args = parser.parse_args(argv) + + # `is not None`, not truthiness: `--env-set ''` is a malformed write, and + # falling through to the probe would print a state report and exit 0 having + # recorded nothing -- a silent no-op is the failure mode this command is + # built to avoid. cmd_env_set refuses it out loud instead. + if args.env_set is not None: + return cmd_env_set(args.env_set) + + state = probe() + print(json.dumps(state, indent=2, sort_keys=True) if args.json else render_probe(state)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/agent-preflight.sh b/scripts/agent-preflight.sh index ccd5932a..c96c09f7 100755 --- a/scripts/agent-preflight.sh +++ b/scripts/agent-preflight.sh @@ -9,7 +9,22 @@ # scripts/agent-preflight.sh # gates + role + print .agents/NOW.md # scripts/agent-preflight.sh --staged # also check the staged change # scripts/agent-preflight.sh --quiet # gates only, no digest -# scripts/agent-preflight.sh --require-role # FAIL if the role is undeclared +# scripts/agent-preflight.sh --no-require-role # tolerate an undeclared role +# scripts/agent-preflight.sh --role-only # ONLY the role gate; NOT a preflight +# +# A session declares a role, and an UNDECLARED one is a failing gate by default: +# the obligation used to live in prose and in an opt-in flag, and neither fired. +# read-only passes a plain run and fails --staged, because staging is writing. +# +# --role-only exists so the mutation suite can EXECUTE that gate instead of +# grepping it: text assertions catch a REWRITE of the default and miss an +# OVERRIDE on a later line. A nested FULL run is impossible -- this script runs +# the very suite that would call it, so it would recurse without bound -- hence +# a mode that runs the role block and stops. It is not an opt-out: it checks the +# ROLE strictly, which --no-require-role does not, and it never prints the "All +# gates green." banner, because it skips every record gate and mutation suite +# --no-require-role runs and so has not earned it. Neither mode is a superset of +# the other; --role-only is narrower and stricter, and says so on stdout. # # It never writes anything, so it is always safe to run. @@ -20,13 +35,21 @@ cd "$ROOT" STAGED=0 QUIET=0 -REQUIRE_ROLE=0 +ROLE_ONLY=0 +# ON by default: an undeclared session is a FAILING gate. The mutation suite +# anchors on THIS line (`^REQUIRE_ROLE=1$`) and refuses any line-anchored +# assignment of zero, quoted or not, so a silent revert of the default goes red +# however it is spelled. The opt-out in the arg loop is indented and therefore +# not line-anchored, which is what keeps the two distinguishable. +REQUIRE_ROLE=1 for arg in "$@"; do case "$arg" in --staged) STAGED=1 ;; --quiet) QUIET=1 ;; --require-role) REQUIRE_ROLE=1 ;; - -h|--help) sed -n '2,13p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; + --no-require-role) REQUIRE_ROLE=0 ;; + --role-only) ROLE_ONLY=1 ;; + -h|--help) sed -n '2,29p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; *) echo "unknown argument: $arg" >&2; exit 2 ;; esac done @@ -49,6 +72,7 @@ CHECKERS=( SUITES=( test_agent_record test_agent_role + test_agent_onboard test_claim_view test_upstream_inventory test_doc_checkpoint @@ -83,12 +107,32 @@ if role_line=$(python3 scripts/agent-role.py show 2>&1); then printf ' \033[32mok\033[0m %s\n' "$role_line" else printf ' \033[33m--\033[0m %s\n' "$(printf '%s' "$role_line" | head -1)" - printf ' declare it: scripts/agent-role.py claim operator | claim helper --row \n' + printf ' This session has not declared a role. Ask what the work is:\n' + printf ' a long or multi-step campaign -> scripts/agent-role.py claim operator\n' + printf ' one scoped change -> scripts/agent-role.py claim helper --row \n' + printf ' just reading or answering -> scripts/agent-role.py claim read-only\n' + printf ' Add --headless to an unattended run. See .agents/workflow.md.\n' if [ "$REQUIRE_ROLE" -eq 1 ]; then failed+=("role-undeclared") fi fi +# read-only PASSES a plain preflight -- that is the point of the third answer. +# It fails --staged, because staging is writing. +if [ "$STAGED" -eq 1 ] && printf '%s' "$role_line" | grep -q 'role=read-only'; then + printf ' \033[31mFAIL\033[0m read-only sessions do not write. Claim operator or helper first.\n' + failed+=("read-only-cannot-stage") +fi + +if [ "$ROLE_ONLY" -eq 1 ]; then + echo "ROLE CHECK ONLY -- this is NOT a full preflight; no record gate ran." + if [ "${#failed[@]}" -ne 0 ]; then + echo "${#failed[@]} gate(s) failed: ${failed[*]}" + exit 1 + fi + exit 0 +fi + echo "Record gates:" for checker in "${CHECKERS[@]}"; do case "$checker" in diff --git a/scripts/agent-role.py b/scripts/agent-role.py index 7ec4979a..63e783b2 100755 --- a/scripts/agent-role.py +++ b/scripts/agent-role.py @@ -7,22 +7,40 @@ into a fact, and only then re-derived. See .agents/specs/operator-helper-protocol.md. -Two identities make that work, both verified real rather than assumed: +A role keys on the **worktree**, never on the session (user-directed correction, +2026-08-06): -* **session** - `VLLM_CPP_AGENT_SESSION` when set, else the parent process id, - which is the agent CLI process and is stable across tool calls within a - session (measured) while differing between concurrently running sessions. -* **worktree** - `git rev-parse --git-dir`, which is per-worktree +* **worktree** - `git rev-parse --absolute-git-dir`, which is per-worktree (`.git/worktrees/`), so a materialized helper is distinguishable from - the primary checkout without any bookkeeping. + the primary checkout without any bookkeeping. The marker lives inside it, so + one worktree is one role and that role survives a new shell, a new process + and a lost environment. +* **session** - `VLLM_CPP_AGENT_SESSION` when set, else the parent process id. + Recorded as PROVENANCE only, and nothing gates on it. An earlier version of + this file called it "stable across tool calls within a session (measured)". + That was DISPROVEN: at least one real harness gives every tool call a fresh + shell and does not persist environment variables, so a role claimed in one + call resolved as UNDECLARED in the next and `agent-preflight.sh` exited 1 -- + making a default-on role gate unpassable rather than strict, which is how a + 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. +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. scripts/agent-role.py show # resolve; exit 3 if undeclared scripts/agent-role.py claim operator 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 scripts/agent-role.py heartbeat scripts/agent-role.py release """ @@ -38,7 +56,26 @@ from pathlib import Path -ROLES = ("operator", "helper") +# read-only is a declared ABSENCE of claim, not a third role: it takes no lock +# 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 +# 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 +# on, and it is kept at exactly two so that such a test stays correct when one +# is written. Today it has no consumer outside this file and its suite: the one +# write refusal that exists is `agent-preflight.sh --staged`, which matches on +# the rendered `role=read-only` line. Nothing else refuses a read-only session +# (see AGENTS.md and .agents/specs/session-onboarding.md, which say so). +CLAIMABLE_ROLES = ("operator", "helper") +DECLARABLE = (*CLAIMABLE_ROLES, "read-only") +ROLES = CLAIMABLE_ROLES # alias kept as the "may write" name for future callers + + +def mode_from_marker(marker: dict) -> str: + """Interactive unless headless was DECLARED. Never inferred.""" + 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. @@ -52,14 +89,19 @@ def git(*args: str) -> str: def session_id() -> str: - """Stable within one agent session, distinct between concurrent ones.""" + """Provenance only: who declared this. NOT stable across tool calls.""" explicit = os.environ.get("VLLM_CPP_AGENT_SESSION") return explicit if explicit else f"ppid:{os.getppid()}" +def worktree_id() -> str: + """The identity a role keys on. One worktree is one role.""" + return git("rev-parse", "--absolute-git-dir") + + def marker_path() -> Path: """Per-worktree, so a materialized helper carries its own role.""" - return Path(git("rev-parse", "--absolute-git-dir")) / "vllm-cpp-agent-role" + return Path(worktree_id()) / "vllm-cpp-agent-role" def lock_path() -> Path: @@ -68,6 +110,27 @@ def lock_path() -> Path: return Path(common) / "vllm-cpp-operator.lock" +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 + 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. + """ + if not lock: + return False + if lock.get("worktree"): + return lock["worktree"] == worktree_id() + return lock.get("session") == session_id() + + def read_json(path: Path) -> dict | None: try: return json.loads(path.read_text(encoding="utf-8")) @@ -88,36 +151,53 @@ def current_branch() -> str: def resolve() -> dict: - """Return the resolved role for THIS session, or {'role': None, ...}.""" + """Return the resolved role for THIS WORKTREE, or {'role': None, ...}.""" me = session_id() marker = read_json(marker_path()) lock = read_json(lock_path()) - # A marker written by a DIFFERENT session sharing this checkout is not ours. - if marker and marker.get("session") == me and marker.get("role") in ROLES: - role = marker["role"] - if role == "operator": - if not lock or lock.get("session") != me: - return { - "role": None, - "session": me, - "reason": "operator marker without a held lock; re-claim", - "branch": current_branch(), - } + # 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 + # 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): + 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)), + "branch": current_branch(), + } return { - "role": role, + "role": declared, "row": marker.get("row"), "session": me, + "declared_by": marker.get("session"), "branch": current_branch(), + "mode": mode_from_marker(marker), "reason": "declared", } - # Not declared by us. Report what else is going on so the caller can decide. - held_by_other = bool(lock and lock.get("session") != me and not lock_is_stale(lock)) + # 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, "reason": "undeclared", } @@ -146,7 +226,8 @@ def cmd_claim(args: argparse.Namespace) -> int: if role == "operator": path = lock_path() - record = {"session": me, "claimed_at": time.time(), "heartbeat": time.time(), + 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 @@ -156,8 +237,15 @@ def cmd_claim(args: argparse.Namespace) -> int: json.dump(record, handle) except FileExistsError: existing = read_json(path) or {} - if existing.get("session") == me: - pass # already ours, idempotent + 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( @@ -176,9 +264,28 @@ def cmd_claim(args: argparse.Namespace) -> int: file=sys.stderr, ) return 1 + 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})") marker_path().write_text( - json.dumps({"role": role, "row": args.row, "session": me, "at": time.time()}), + json.dumps({ + "role": role, + "row": args.row, + "session": me, + # Declared with the role, so it is a fact rather than a guess: no + # later code has to infer headless from the hour or from silence. + "mode": "headless" if args.headless else "interactive", + "at": time.time(), + }), encoding="utf-8", ) print(f"claimed role={role}" + (f" row={args.row}" if args.row else "")) @@ -199,9 +306,8 @@ def cmd_heartbeat(_: argparse.Namespace) -> int: def cmd_release(_: argparse.Namespace) -> int: - me = session_id() lock = read_json(lock_path()) - if lock and lock.get("session") == me: + if lock_is_ours(lock): lock_path().unlink(missing_ok=True) print("released the operator lock") marker_path().unlink(missing_ok=True) @@ -218,8 +324,13 @@ def main() -> int: show.set_defaults(func=cmd_show) claim = sub.add_parser("claim", help="declare and materialize a role") - claim.add_argument("role", choices=ROLES) + claim.add_argument("role", choices=DECLARABLE) claim.add_argument("--row", help="the row ID a helper claims") + claim.add_argument( + "--headless", + action="store_true", + help="unattended run: decide and record rather than ask (never inferred)", + ) claim.set_defaults(func=cmd_claim) sub.add_parser("heartbeat", help="keep the operator lock alive").set_defaults( diff --git a/scripts/check-protocol-consistency.py b/scripts/check-protocol-consistency.py index 6d7d8b9b..a36f74a5 100644 --- a/scripts/check-protocol-consistency.py +++ b/scripts/check-protocol-consistency.py @@ -21,6 +21,13 @@ |---|---| | `docs/STATUS.md` | every feature/iteration checkpoint | + +The same gate now also asserts that `.agents/workflow.md` carries the ROLE +INTERVIEW, between `` and its `:end`. That is the +same failure with the polarity flipped: agent-preflight.sh refuses a session +that has not declared a role, so an agent who is never told the question, or +never told that `read-only` is one of the answers, meets a red gate with no +instructions -- and a gate people cannot satisfy is a gate people route around. """ from __future__ import annotations @@ -40,6 +47,15 @@ BEGIN = "" END = "" +# The session manual must carry the role interview, because agent-preflight.sh +# now FAILS a session that has not declared a role. A gate whose precondition is +# written down nowhere is the same drift this file exists to prevent, with the +# polarity flipped: instead of prose demanding what the checker dropped, the +# checker demands what no prose ever taught. +INTERVIEW_DOCUMENT = ".agents/workflow.md" +INTERVIEW_MARKER = "" +INTERVIEW_REQUIRED = ("claim operator", "claim helper --row", "claim read-only", "--headless") + # A path in a table cell, e.g. `docs/STATUS.md`. CELL_PATH = re.compile(r"`([^`]+\.md)`") @@ -114,11 +130,30 @@ def document_errors(name: str, text: str, expected: tuple[str, ...]) -> list[str return errors +def interview_errors(text: str) -> list[str]: + """The role interview must live where agents read it, not only in a gate.""" + if INTERVIEW_MARKER not in text: + return [f"{INTERVIEW_DOCUMENT} is missing the role-interview block"] + return [ + f"{INTERVIEW_DOCUMENT} role interview omits {needle!r}" + for needle in INTERVIEW_REQUIRED + if needle not in text + ] + + def main() -> int: expected = obligated_surfaces() failures: list[str] = [] blocks: dict[str, list[str] | None] = {} + interview = ROOT / INTERVIEW_DOCUMENT + if not interview.exists(): + failures.append(f"{INTERVIEW_DOCUMENT} does not exist") + else: + failures.extend( + interview_errors(interview.read_text(encoding="utf-8")) + ) + for name in CONTRACT_DOCUMENTS: path = ROOT / name if not path.exists(): @@ -145,7 +180,9 @@ def main() -> int: "The obligated public surfaces are defined by PUBLIC_CHECKPOINTS and " "FEATURE_CHECKPOINT in scripts/check-doc-checkpoint.py. Mirror them " "in the contract block of every document listed in " - "CONTRACT_DOCUMENTS.", + "CONTRACT_DOCUMENTS. The role interview is the block between " + f"{INTERVIEW_MARKER} and its :end in {INTERVIEW_DOCUMENT}; it must " + "name every answer agent-role.py accepts.", file=sys.stderr, ) return 1 @@ -153,7 +190,8 @@ def main() -> int: print( "OK: the doc-obligation contract in " f"{' and '.join(CONTRACT_DOCUMENTS)} matches " - "scripts/check-doc-checkpoint.py." + f"scripts/check-doc-checkpoint.py, and {INTERVIEW_DOCUMENT} carries the " + "role interview." ) return 0 diff --git a/tests/scripts/test_agent_onboard.py b/tests/scripts/test_agent_onboard.py new file mode 100644 index 00000000..9ef8a8df --- /dev/null +++ b/tests/scripts/test_agent_onboard.py @@ -0,0 +1,562 @@ +#!/usr/bin/env python3 +"""Unit and mutation checks for scripts/agent-onboard.py. + +The probe exists to report what is unresolved. Its one job is to be honest +about absence: a missing .env and an unreadable .env must not look the same as +a complete one, and an undeclared role must never render as a declared one. +""" + +from __future__ import annotations + +import contextlib +import importlib.util +import io +import os +import re +import shutil +import subprocess +import sys +import tempfile +import types +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[2] + + +def _load(name: str, relative: str): + path = ROOT / relative + spec = importlib.util.spec_from_file_location(name, path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +onboard = _load("agent_onboard", "scripts/agent-onboard.py") + + +class EnvStateTests(unittest.TestCase): + def test_env_keys_match_the_tracked_example(self): + # The probe must never invent a key. .env.example is the only source. + example = (ROOT / ".env.example").read_text(encoding="utf-8") + declared = { + line.split("=", 1)[0] + for line in example.splitlines() + if line and not line.startswith("#") and "=" in line + } + self.assertEqual(set(onboard.ENV_KEYS), declared) + + def test_missing_file_reports_missing_not_incomplete(self): + status, missing = onboard.env_state(ROOT / "does-not-exist-.env") + self.assertEqual(status, "missing") + self.assertEqual(sorted(missing), sorted(onboard.ENV_KEYS)) + + def test_blank_value_counts_as_missing_that_key(self): + # An empty value is a legitimate "unavailable", but the probe still + # has to report it so the agent knows what it may ask for. + text = "\n".join(f"{k}=" for k in onboard.ENV_KEYS) + status, missing = onboard.env_state_from_text(text) + self.assertEqual(status, "incomplete") + self.assertEqual(sorted(missing), sorted(onboard.ENV_KEYS)) + + def test_all_values_present_reports_present(self): + text = "\n".join(f"{k}=/some/path" for k in onboard.ENV_KEYS) + status, missing = onboard.env_state_from_text(text) + self.assertEqual(status, "present") + self.assertEqual(missing, []) + + def test_unreadable_file_is_neither_present_nor_absent(self): + # Beyond the brief. A .env that exists but cannot be read must not + # crash the probe, must not read as complete, and must not read as + # absent either: "create one" is the wrong instruction for a file that + # is already there. + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / ".env" + path.mkdir() # exists(), but read_text() raises IsADirectoryError + status, missing = onboard.env_state(path) + self.assertEqual(status, "unreadable") + self.assertEqual(sorted(missing), sorted(onboard.ENV_KEYS)) + + +class ProbeRenderTests(unittest.TestCase): + UNDECLARED = { + "role": None, "row": None, "mode": "interactive", + "env": "missing", "env_missing": ["VLLM_ORACLE"], "queue": ["ENG-FOO"], + } + + def test_undeclared_role_renders_as_undeclared(self): + out = onboard.render_probe(self.UNDECLARED) + self.assertIn("UNDECLARED", out) + self.assertNotIn("operator", out.split("queue")[0]) + + def test_declared_role_renders_with_its_row(self): + # The row id must NOT be one the fixture queue already contains, or the + # queue line satisfies the assertion and deleting row rendering stays + # green. Assert the `row=` prefix, not the bare id. + out = onboard.render_probe(dict(self.UNDECLARED, role="helper", row="KERNEL-BAR")) + self.assertIn("helper", out) + self.assertIn("row=KERNEL-BAR", out) + + def test_undeclared_render_carries_the_interview_hint(self): + # The hint is the whole point of the probe: without it an agent sees a + # state line and no instruction. Deleting the block must go red. + out = onboard.render_probe(self.UNDECLARED) + self.assertIn("claim", out) + self.assertIn("read-only", out) + self.assertNotIn("claim", onboard.render_probe( + dict(self.UNDECLARED, role="helper", row="KERNEL-BAR"))) + # Added: an absent mode is an absence too. resolve() has no mode until + # step 2, so it must render as a default and never as a declaration. + # Assert the whole mode field: the hint line already contains "not + # declared", so a looser assertion would pass on a fabricated mode. + undeclared_mode = {k: v for k, v in self.UNDECLARED.items() if k != "mode"} + self.assertIn( + "mode: interactive (default, not declared)", + 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_probe_never_exits_nonzero(self): + # The probe reports; it does not gate. Preflight gates. + # The render itself goes to a buffer only so the suite's output stays + # clean; the assertion is unchanged. + with contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(onboard.main(["--probe"]), 0) + + +ROLE_SCRIPT = ROOT / "scripts/agent-role.py" + + +class ProbeFieldsComeFromResolve(unittest.TestCase): + """probe()'s RETURNED DICT, against a role claimed through the real CLI. + + 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"`, + `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. + + So these claim in a THROWAWAY repo and read the values back out of probe(). + """ + + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.repo = Path(self.tmp.name) + subprocess.run(["git", "init", "-q"], cwd=self.repo, check=True) + subprocess.run( + ["git", "commit", "-qm", "root", "--allow-empty"], cwd=self.repo, + check=True, + env=dict(os.environ, GIT_AUTHOR_NAME="t", GIT_AUTHOR_EMAIL="t@t", + GIT_COMMITTER_NAME="t", GIT_COMMITTER_EMAIL="t@t")) + + def claim(self, where: Path, session: str, *args: str): + result = subprocess.run( + [sys.executable, str(ROLE_SCRIPT), "claim", *args], cwd=where, + env=dict(os.environ, VLLM_CPP_AGENT_SESSION=session), + capture_output=True, text=True) + return result + + def worktree(self, name: str) -> Path: + 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 probe_in(self, where: Path) -> dict: + saved = os.getcwd() + os.chdir(where) + try: + return onboard.probe() + finally: + os.chdir(saved) + + def test_probe_reports_the_role_and_row_that_were_claimed(self) -> None: + # Kills `role: "operator"`, `row: None`, `mode: "headless"` and + # `reason: None` in one assertion block: a helper claim with no + # --headless flag disagrees with every one of them. + claimed = self.claim(self.repo, "a", "helper", "--row", "PROBE-WIRING") + self.assertEqual(claimed.returncode, 0, claimed.stderr) + state = self.probe_in(self.repo) + self.assertEqual(state["role"], "helper") + self.assertEqual(state["row"], "PROBE-WIRING") + self.assertEqual(state["mode"], "interactive") + self.assertEqual(state["reason"], "declared") + self.assertIs(state["blocked_by_other_operator"], False) + + def test_probe_reports_a_mode_that_was_declared(self) -> None: + # The other half of the mode pin: a hardcoded "interactive" survives the + # test above and dies here. Headless is DECLARED, never inferred, so + # both directions have to come out of the marker. + self.assertEqual( + self.claim(self.repo, "a", "read-only", "--headless").returncode, 0) + state = self.probe_in(self.repo) + 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. + 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) + + 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) + + 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 + # env_state is substituted with a value no hardcode can be, and probe() + # must carry BOTH of its outputs through. + saved = onboard.env_state + onboard.env_state = lambda *a, **k: ("stubbed-status", ["STUB_KEY"]) + try: + state = self.probe_in(self.repo) + finally: + onboard.env_state = saved + self.assertEqual(state["env"], "stubbed-status") + self.assertEqual(state["env_missing"], ["STUB_KEY"]) + + +class QueueTests(unittest.TestCase): + def test_queue_is_the_checkers_own_computation_and_failure_is_visible(self): + # Beyond the brief, two properties of the same function. + # + # 1. The queue must come from ready-for-helper.py's queue(), not from a + # re-parse of its prose: an uppercase-token filter over that stdout + # reads the "READY-FOR-HELPER queue: N row(s)" header as a row (so an + # EMPTY queue reports one phantom row) and drops every mixed-case row + # id such as MODEL-TEXT-glm4-glm4-for-causal-lm. + # 2. A queue that could not be computed is not an empty queue. Swallowing + # the failure into [] is the queue-side twin of reporting an unreadable + # .env as a complete one. + rows = onboard.ready_rows() + result = subprocess.run( + [sys.executable, str(ROOT / "scripts/ready-for-helper.py")], + cwd=ROOT, capture_output=True, text=True, check=True, + ) + header = result.stdout.splitlines()[0] + self.assertIn("READY-FOR-HELPER queue:", header) + self.assertEqual(len(rows), int(header.split(":")[1].split()[0])) + self.assertNotIn("READY-FOR-HELPER", rows) + + def explode(): + raise RuntimeError("record is broken") + + saved = sys.modules.get("ready_for_helper") + sys.modules["ready_for_helper"] = types.SimpleNamespace(queue=explode) + try: + broken_rows, error = onboard.queue_state() + finally: + if saved is None: + del sys.modules["ready_for_helper"] + else: + sys.modules["ready_for_helper"] = saved + self.assertEqual(broken_rows, []) + self.assertIn("record is broken", error) + self.assertIn("UNAVAILABLE", onboard.render_probe( + dict(ProbeRenderTests.UNDECLARED, queue=[], queue_error=error))) + + +class PreflightWiringTests(unittest.TestCase): + TEXT = (ROOT / "scripts/agent-preflight.sh").read_text(encoding="utf-8") + + def test_require_role_defaults_on(self): + # Anchored to the DEFAULT assignment itself -- the line with no + # indentation and nothing else on it. A bare assertIn("REQUIRE_ROLE=1") + # is satisfied by the --require-role arm of the arg loop all by itself, + # so the default could be flipped back and the whole deliverable of this + # change would go unprotected. Any line-anchored assignment of zero is + # refused, quoted or not, because that is what a silent revert looks + # like however it is spelled. + self.assertRegex(self.TEXT, r"(?m)^REQUIRE_ROLE=1$") + self.assertNotRegex(self.TEXT, r"""(?m)^REQUIRE_ROLE=['"]?0['"]?$""") + + def test_opt_out_flag_exists(self): + self.assertIn("--no-require-role", self.TEXT) + + def test_failure_text_carries_the_interview(self): + # An error code alone gets routed around. The gate must say what to ask. + self.assertIn("claim read-only", self.TEXT) + self.assertIn("claim helper --row", self.TEXT) + + def test_staged_refuses_read_only(self): + self.assertIn("read-only", self.TEXT) + self.assertIn("STAGED", self.TEXT) + + def test_the_gate_records_a_failure_and_not_only_a_print(self): + # Every other assertion in this class inspects the text that EXPLAINS + # the gate, so deleting the one line that enforces it -- the failed+=() + # inside the REQUIRE_ROLE branch -- leaves them all green while + # preflight exits 0 on an undeclared role. Pin the enforcing line, and + # pin that it is inside the branch: outside it, --no-require-role would + # stop working instead. + branch = re.search( + r'if \[ "\$REQUIRE_ROLE" -eq 1 \]; then(.*?)\n fi', self.TEXT, re.S) + self.assertIsNotNone(branch, "the REQUIRE_ROLE branch is gone") + self.assertIn('failed+=("role-undeclared")', branch.group(1)) + + def test_onboard_suite_is_registered(self): + self.assertIn("test_agent_onboard", self.TEXT) + + def test_read_only_alone_does_not_satisfy_a_write_gate(self): + # agent-role.py show exits 0 for read-only, so --require-role is + # satisfied by a declared ABSENCE of claim. That is correct for a plain + # run and wrong for --staged; the refusal must be explicit. + self.assertIn("read-only-cannot-stage", self.TEXT) + + def test_preflight_actually_fails_on_an_undeclared_role(self): + # Every other assertion in this class greps text, so a REWRITE of the + # default is caught while an OVERRIDE is not: keep `REQUIRE_ROLE=1` and + # add `REQUIRE_ROLE=0 ` (one trailing space, so the ^...$ anchor misses) + # on a later line and the whole class stays green while the gate stops + # failing. Only executing the script closes that hole. + # + # Two mechanics this test cannot do the obvious way: + # + # * VLLM_CPP_AGENT_SESSION cannot make a role unresolvable. A role keys + # on the WORKTREE (agent-role.py marker_path/lock_path, corrected + # 2026-08-06), so this tree's own marker answers whatever the session + # id says. GIT_DIR is the one knob that moves the marker and the lock + # together, so the run is pointed at an empty git dir instead. + # * The run is --role-only, not a full preflight: agent-preflight.sh + # runs THIS suite, and a nested full run would recurse without bound. + # --role-only executes the same role block and the same REQUIRE_ROLE + # branch, which is the code under test. + with tempfile.TemporaryDirectory() as tmp: + empty = Path(tmp) / "empty-git-dir" + subprocess.run( + ["git", "init", "--quiet", "--bare", str(empty)], check=True) + env = dict(os.environ, GIT_DIR=str(empty)) + env.pop("GIT_WORK_TREE", None) + + # PRECONDITION, asserted and never skipped: if a role still + # resolves here the run below proves nothing and must go red. + probe = subprocess.run( + [sys.executable, str(ROOT / "scripts/agent-role.py"), "show"], + cwd=ROOT, capture_output=True, text=True, check=False, env=env, + ) + self.assertEqual( + probe.returncode, 3, + "precondition failed: a role still resolves under the empty " + f"GIT_DIR, so this test asserts nothing. show said: " + f"{probe.stdout.strip()!r}") + self.assertIn("UNDECLARED", probe.stdout) + + result = subprocess.run( + ["bash", str(ROOT / "scripts/agent-preflight.sh"), "--role-only"], + cwd=ROOT, capture_output=True, text=True, check=False, env=env, + ) + self.assertNotEqual( + result.returncode, 0, + "preflight exited 0 with no resolvable role; the REQUIRE_ROLE " + "default is off however it is spelled") + self.assertIn("role-undeclared", result.stdout + result.stderr) + + def test_role_only_is_not_mistakable_for_a_full_preflight(self): + # --role-only exists so a test can execute the gate without recursing. + # It must never read as a green preflight, or it becomes the opt-out + # --no-require-role was demoted from. + self.assertIn("--role-only", self.TEXT) + run = subprocess.run( + ["bash", str(ROOT / "scripts/agent-preflight.sh"), "--role-only"], + cwd=ROOT, capture_output=True, text=True, check=False, + ) + output = run.stdout + run.stderr + self.assertIn("NOT a full preflight", output) + self.assertNotIn("All gates green", output) + + +class EnvSetTests(unittest.TestCase): + def setUp(self): + self.tmp = Path(tempfile.mkdtemp()) + self.env = self.tmp / ".env" + self._real = onboard.ENV_FILE + onboard.ENV_FILE = self.env + # cmd_env_set reports on stdout and refuses on stderr. Both go to a + # buffer so the suite's own output stays clean -- but the buffers are + # KEPT and asserted on, because a refusal that returns 2 with an empty + # explanation would otherwise pass every test in this class. + self.out, self.err = io.StringIO(), io.StringIO() + stack = contextlib.ExitStack() + stack.enter_context(contextlib.redirect_stdout(self.out)) + stack.enter_context(contextlib.redirect_stderr(self.err)) + self.addCleanup(stack.close) + + def tearDown(self): + onboard.ENV_FILE = self._real + shutil.rmtree(self.tmp) + + def test_unknown_key_is_refused(self): + # Never invent a key: a typo'd name would sit in .env doing nothing + # while the gate that wanted it stays mysteriously PENDING. + self.assertEqual(onboard.cmd_env_set("NOT_A_REAL_KEY=/x"), 2) + self.assertFalse(self.env.exists()) + # An exit code alone gets routed around, and every refusal here is a + # human's typo. Name the offending key and the legal ones. + self.assertIn("NOT_A_REAL_KEY", self.err.getvalue()) + self.assertIn(onboard.ENV_KEYS[0], self.err.getvalue()) + + def test_missing_pair_is_refused(self): + self.assertEqual(onboard.cmd_env_set("VLLM_ORACLE"), 2) + self.assertFalse(self.env.exists()) + self.assertIn("KEY=VALUE", self.err.getvalue()) + + def test_first_write_seeds_from_the_example(self): + self.assertEqual(onboard.cmd_env_set(f"{onboard.ENV_KEYS[0]}=/oracle"), 0) + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{onboard.ENV_KEYS[0]}=/oracle", text) + # every other declared key survives, so nothing is silently dropped + for key in onboard.ENV_KEYS: + self.assertIn(key, text) + + def test_second_write_updates_in_place_without_duplicating(self): + key = onboard.ENV_KEYS[0] + onboard.cmd_env_set(f"{key}=/first") + onboard.cmd_env_set(f"{key}=/second") + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{key}=/second", text) + self.assertNotIn("/first", text) + self.assertEqual(sum(1 for l in text.splitlines() if l.startswith(f"{key}=")), 1) + + def test_other_keys_are_not_disturbed(self): + a, b = onboard.ENV_KEYS[0], onboard.ENV_KEYS[1] + onboard.cmd_env_set(f"{a}=/aaa") + onboard.cmd_env_set(f"{b}=/bbb") + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"{a}=/aaa", text) + self.assertIn(f"{b}=/bbb", text) + + def test_the_flag_is_wired_into_main(self): + # Beyond the brief. Every test above calls cmd_env_set() directly, so + # deleting --env-set or its dispatch in main() leaves all five green + # while the only entry point an agent actually types either dies in + # argparse or silently prints a probe and records nothing. + key = onboard.ENV_KEYS[0] + self.assertEqual(onboard.main(["--env-set", f"{key}=/wired"]), 0) + self.assertIn(f"{key}=/wired", self.env.read_text(encoding="utf-8")) + self.assertEqual(onboard.main(["--env-set", "NOT_A_REAL_KEY=/x"]), 2) + # An empty argument is a malformed write, not "no flag given": a + # truthiness dispatch falls through to the probe and exits 0 having + # recorded nothing, which is the silent no-op this command must not do. + self.assertEqual(onboard.main(["--env-set", ""]), 2) + + def test_an_empty_value_is_accepted_and_still_reads_as_unset(self): + # Beyond the brief, and the rule the whole command serves: unanswered + # means EMPTY, and empty means the gates that need it stay PENDING. + # A writer that refused an empty value would push its caller toward + # inventing one from a path, a username or another developer's setup, + # which is the failure this spec exists to prevent. Clearing a value + # must also be possible, or a wrong answer is unretractable. + key = onboard.ENV_KEYS[0] + self.assertEqual(onboard.cmd_env_set(f"{key}=/somewhere"), 0) + self.assertEqual(onboard.cmd_env_set(f"{key}="), 0) + text = self.env.read_text(encoding="utf-8") + self.assertIn(f"\n{key}=\n", f"\n{text}") + self.assertIn(key, onboard.env_state_from_text(text)[1]) + + def test_no_line_separator_in_a_value_can_forge_a_second_line(self): + # Beyond the brief. The value is whatever the interview answer was, and + # it is the one field no check looks at. A separator inside it appends a + # whole extra .env line that no key check ever saw -- the silent clobber + # this task exists to rule out, arriving through the unvalidated field. + # + # EVERY separator, not just "\n": str.splitlines() is what the reader + # and the rewrite both use, and it breaks on ten. Guarding two of them + # let a "\v" or a U+2028 smuggle a forged pair past the key check, after + # which the probe reported the forged key as SET. This list IS the + # separator set; a guard that enumerates characters again fails here. + key, victim = onboard.ENV_KEYS[0], onboard.ENV_KEYS[3] + for separator in ("\n", "\r", "\r\n", "\v", "\f", + "\x1c", "\x1d", "\x1e", "\x85", "
", "
"): + with self.subTest(separator=repr(separator)): + pair = f"{key}=/ok{separator}{victim}=/forged" + self.assertEqual(onboard.cmd_env_set(pair), 2) + self.assertFalse(self.env.exists()) + # The property, stated the way the guard states it: what is written + # must survive the round trip through the reader that parses it back. + self.assertEqual(onboard.cmd_env_set(f"{key}=/ok
{victim}=/forged"), 2) + self.assertIn("line separator", self.err.getvalue()) + + def test_a_trailing_duplicate_key_cannot_survive_the_write(self): + # Beyond the brief. A hand-maintained .env routinely carries an override + # appended at the bottom, and both this reader and the documented + # `set -a; . ./.env` loader take the LAST assignment. Updating only the + # first match left the file changed, the exit code 0 and the message + # reassuring while the EFFECTIVE value never moved -- the silent no-op + # this command exists to rule out, arriving from the other side. + key, other = onboard.ENV_KEYS[0], onboard.ENV_KEYS[5] + self.env.write_text( + f"{key}=/one\n{other}=h\n{key}=/override\n", encoding="utf-8") + self.assertEqual(onboard.cmd_env_set(f"{key}=/new"), 0) + lines = self.env.read_text(encoding="utf-8").splitlines() + effective = [l.split("=", 1)[1] for l in lines if l.startswith(f"{key}=")][-1] + self.assertEqual(effective, "/new") + self.assertEqual(sum(1 for l in lines if l.startswith(f"{key}=")), 1) + self.assertIn(f"{other}=h", lines) # the unrelated line keeps its place + + def test_an_unreadable_env_is_refused_and_not_a_traceback(self): + # Beyond the brief. env_state treats an existing-but-unreadable .env as + # its own third case; the WRITER has to agree, or the one command an + # agent is told to run dies in a bare traceback that names no fix. + self.env.mkdir() # exists(), but read_text() raises IsADirectoryError + self.assertEqual(onboard.cmd_env_set(f"{onboard.ENV_KEYS[0]}=/x"), 2) + self.assertIn("cannot be read", self.err.getvalue()) + + def test_a_value_needing_quotes_reads_the_same_to_both_readers(self): + # Beyond the brief, same family as the separator finding: .env.example + # documents the loader as `set -a; . ./.env`, so the file is shell as + # well as data. An unquoted "/pa th" makes that loader run `th` and + # leave the variable EMPTY while this probe's parser reports it PRESENT + # -- a gate that stays PENDING with the surface insisting it is set. + key = onboard.ENV_KEYS[0] + self.assertEqual(onboard.cmd_env_set(f"{key}=/pa th"), 0) + text = self.env.read_text(encoding="utf-8") + self.assertNotIn(key, onboard.env_state_from_text(text)[1]) # probe: set + sourced = subprocess.run( + ["sh", "-c", f'set -a; . "$1"; set +a; printf %s "${key}"', "sh", + str(self.env)], + capture_output=True, text=True, check=True, + ) + self.assertEqual(sourced.stdout, "/pa th") # loader: the SAME value + self.assertEqual(sourced.stderr, "") + # and an empty value must still round-trip as UNSET, not as the literal + # two-character '' that shlex.quote would otherwise produce. + self.assertEqual(onboard.cmd_env_set(f"{key}="), 0) + self.assertIn(key, onboard.env_state_from_text( + self.env.read_text(encoding="utf-8"))[1]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index 77359974..78641eb6 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -35,6 +35,7 @@ def _load(name: str, relative: str): discipline = _load("role_discipline", "scripts/check-role-discipline.py") +role = _load("agent_role", "scripts/agent-role.py") ROLE_SCRIPT = ROOT / "scripts/agent-role.py" @@ -46,8 +47,8 @@ def run_role(repo: Path, session: str, *args: str): ) -class RoleLifecycle(unittest.TestCase): - """Exercised against a throwaway repo, never the real one.""" +class _TempRepo: + """A throwaway git repo per test. The real checkout is never touched.""" def setUp(self) -> None: self.tmp = tempfile.TemporaryDirectory() @@ -61,6 +62,22 @@ def setUp(self) -> None: def tearDown(self) -> None: self.tmp.cleanup() + 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. + """ + 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 + + +class RoleLifecycle(_TempRepo, unittest.TestCase): + """Exercised against a throwaway repo, never the real one.""" + def test_undeclared_session_exits_3(self) -> None: self.assertEqual(run_role(self.repo, "a", "show").returncode, 3) @@ -71,18 +88,33 @@ def test_claim_then_resolve(self) -> None: self.assertIn("role=operator", out.stdout) def test_second_operator_is_refused(self) -> None: - """The core mutual-exclusion guarantee.""" + """The core mutual-exclusion guarantee. + + 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. + """ run_role(self.repo, "a", "claim", "operator") - second = run_role(self.repo, "b", "claim", "operator") + second = run_role(self.worktree("rival"), "b", "claim", "operator") self.assertEqual(second.returncode, 1) self.assertIn("already held", second.stderr) - def test_other_session_does_not_inherit_the_role(self) -> None: - """Sessions sharing one checkout must not read each other's marker.""" + def test_another_session_in_the_same_worktree_shares_the_role(self) -> None: + """The accepted cost of keying on the worktree, made explicit. + + This test asserted the opposite until 2026-08-06. The session id was + measured NOT to be stable across tool calls in a real harness, so + requiring it made a declared role invisible one call later and turned + --require-role default-on into an unpassable gate rather than a strict + one. Isolation is preserved where it is real -- see + test_helper_marker_does_not_leak_into_another_worktree -- and + .agents/specs/session-onboarding.md records the trade. + """ run_role(self.repo, "a", "claim", "operator") other = run_role(self.repo, "b", "show") - self.assertEqual(other.returncode, 3) - self.assertIn("UNDECLARED", other.stdout) + self.assertEqual(other.returncode, 0) + self.assertIn("role=operator", other.stdout) def test_helper_requires_a_row(self) -> None: self.assertEqual(run_role(self.repo, "a", "claim", "helper").returncode, 2) @@ -104,7 +136,10 @@ def test_stale_lock_is_broken_but_reported(self) -> None: record = json.loads(lock.read_text()) record["heartbeat"] = time.time() - (10 * 60 * 60) lock.write_text(json.dumps(record)) - took = run_role(self.repo, "b", "claim", "operator") + # 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 @@ -117,6 +152,218 @@ def test_operator_marker_without_lock_does_not_resolve(self) -> None: self.assertEqual(run_role(self.repo, "a", "show").returncode, 3) +class WorktreeKeyedRole(_TempRepo, unittest.TestCase): + """A role keys on the WORKTREE, not the session (user-directed 2026-08-06). + + `.agents/specs/session-onboarding.md`, "Correction: a role keys on the + WORKTREE, not the session". Every test here dies if `resolve()` goes back to + comparing `marker['session']` to the current process. + """ + + def test_helper_role_survives_a_new_session_id(self) -> None: + # THE regression this correction exists to prevent: claim in one tool + # call, resolve in the next, where the parent pid has already changed. + self.assertEqual( + run_role(self.repo, "call-1", "claim", "helper", "--row", "ENG-FOO").returncode, + 0, + ) + later = run_role(self.repo, "call-2-different-pid", "show") + self.assertEqual(later.returncode, 0) + self.assertIn("role=helper", later.stdout) + self.assertIn("row=ENG-FOO", later.stdout) + + def test_operator_role_survives_a_new_session_id(self) -> None: + # The lock is what makes an operator an operator, so lock OWNERSHIP has + # to key on the worktree too. Key only the marker and the operator alone + # still dies at the call boundary, which is the failure that matters + # most: the operator is the role that lands on main. + run_role(self.repo, "call-1", "claim", "operator") + later = run_role(self.repo, "call-2-different-pid", "show") + self.assertEqual(later.returncode, 0) + self.assertIn("role=operator", later.stdout) + + def test_resolve_ignores_the_marker_session(self) -> None: + # The same pin driven through resolve() itself rather than the CLI: a + # marker whose recorded session is NOT this process's must still + # resolve, and the recorded session must survive as provenance. + run_role(self.repo, "some-other-session", "claim", "helper", "--row", "ENG-BAR") + marker = json.loads( + (self.repo / ".git/vllm-cpp-agent-role").read_text(encoding="utf-8")) + self.assertEqual(marker["session"], "some-other-session") + + saved_env = os.environ.get("VLLM_CPP_AGENT_SESSION") + saved_cwd = os.getcwd() + os.environ["VLLM_CPP_AGENT_SESSION"] = "a-completely-different-session" + os.chdir(self.repo) + try: + state = role.resolve() + finally: + os.chdir(saved_cwd) + if saved_env is None: + os.environ.pop("VLLM_CPP_AGENT_SESSION", None) + else: + os.environ["VLLM_CPP_AGENT_SESSION"] = saved_env + + self.assertEqual(state["role"], "helper") + 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". + 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) + + def test_helper_marker_does_not_leak_into_another_worktree(self) -> None: + # Isolation, asserted where it is now real. The SAME session id in + # another worktree must resolve as undeclared: a helper materializes its + # own worktree, so its marker cannot reach anyone else's. + run_role(self.repo, "a", "claim", "helper", "--row", "ENG-FOO") + out = run_role(self.worktree("elsewhere"), "a", "show") + 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 _resolve_in(self, where: Path) -> dict: + """resolve()'s OWN return value, read in `where`. + + The CLI prints a rendering; several keys of the resolved state never + reach stdout, so a dict-level assertion is the only way to pin them. + """ + saved = os.getcwd() + os.chdir(where) + try: + return role.resolve() + 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. + 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") + + 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.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) + + 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 + 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) + + def test_downgrading_out_of_operator_releases_the_lock(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 + # 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. + self.assertEqual(run_role(self.repo, "a", "claim", "operator").returncode, 0) + self.assertTrue(self._lock(self.repo).exists()) + downgrade = run_role(self.repo, "a", "claim", "read-only") + self.assertEqual(downgrade.returncode, 0, downgrade.stderr) + self.assertFalse(self._lock(self.repo).exists()) + 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: + # 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()) + + 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. + 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.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. + 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) + + 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", @@ -187,7 +434,101 @@ def test_a_direct_feature_push_after_cutover_now_FAILS(self) -> None: self.assertTrue(discipline.enforced("HEAD")) def test_live_repository_is_reportable(self) -> None: - self.assertEqual(discipline.main(), 0) + # main() parses sys.argv, which under `unittest -v` still carries the + # runner's own flags and made argparse SystemExit(2) here. The argv is + # isolated; the assertion is unchanged. + saved = sys.argv + sys.argv = [saved[0]] + try: + self.assertEqual(discipline.main(), 0) + finally: + sys.argv = saved + + +class ReadOnlyAndModeTests(unittest.TestCase): + def test_claimable_roles_stay_exactly_two(self): + # read-only must never become a third claimable role: it takes no lock + # and no worktree. CLAIMABLE_ROLES is the vocabulary a "may this session + # write?" test is meant to key on; it has no consumer outside + # agent-role.py and this suite today, so this pin protects the + # constant's meaning rather than a live refusal. + self.assertEqual(role.CLAIMABLE_ROLES, ("operator", "helper")) + self.assertIn("read-only", role.DECLARABLE) + self.assertNotIn("read-only", role.CLAIMABLE_ROLES) + + def test_read_only_is_declarable(self): + self.assertIn("read-only", role.DECLARABLE) + + def test_the_roles_alias_is_not_widened(self): + # ROLES is the alias a write-gating call site would import; no such + # call site exists yet, so keeping it the CLAIMABLE pair is what stops + # the first one from being born wrong. Mutating it to DECLARABLE leaves every other + # assertion in this suite green, so the constraint that keeps read-only + # out of "may this session write?" would be enforced by comment only. + self.assertEqual(role.ROLES, role.CLAIMABLE_ROLES) + self.assertNotIn("read-only", role.ROLES) + + def test_mode_defaults_to_interactive(self): + # Headless is DECLARED, never inferred. Absent an explicit flag the + # session is interactive. + self.assertEqual(role.mode_from_marker({}), "interactive") + self.assertEqual(role.mode_from_marker({"mode": "headless"}), "headless") + self.assertEqual(role.mode_from_marker({"mode": "nonsense"}), "interactive") + + +class ReadOnlyAndModeResolved(_TempRepo, unittest.TestCase): + """Drives resolve() itself, not only the pure helpers above. + + mode_from_marker() can be perfectly correct while resolve() never calls it: + the key would simply be absent from the resolved state and a test that only + exercised the helper would stay green. So these claim through the real CLI + and read the mode back out of resolve()'s OWN return value. + """ + + def _resolve_as(self, session: str, where: Path | None = None) -> dict: + cwd = os.getcwd() + saved = os.environ.get("VLLM_CPP_AGENT_SESSION") + os.chdir(where or self.repo) + os.environ["VLLM_CPP_AGENT_SESSION"] = session + try: + return role.resolve() + finally: + os.chdir(cwd) + if saved is None: + del os.environ["VLLM_CPP_AGENT_SESSION"] + else: + os.environ["VLLM_CPP_AGENT_SESSION"] = saved + + def test_resolve_carries_a_declared_headless_mode(self) -> None: + claimed = run_role(self.repo, "a", "claim", "read-only", "--headless") + self.assertEqual(claimed.returncode, 0, claimed.stderr) + state = self._resolve_as("a") + self.assertEqual(state["role"], "read-only") + self.assertEqual(state["mode"], "headless") + + def test_resolve_reports_interactive_unless_headless_was_declared(self) -> None: + run_role(self.repo, "a", "claim", "helper", "--row", "ENG-FOO") + self.assertEqual(self._resolve_as("a")["mode"], "interactive") + # An UNDECLARED context is interactive too: silence is never headless. + # Genuinely undeclared means another WORKTREE since the 2026-08-06 + # correction; another session id in THIS one resolves to the role that + # was declared here. + undeclared = self._resolve_as("b", where=self.worktree("undeclared")) + 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. + 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.assertIn("role=read-only", run_role(self.repo, "a", "show").stdout) + # ... and a real operator elsewhere is still free to take the lock. + self.assertEqual( + run_role(self.worktree("real-operator"), "b", "claim", "operator").returncode, 0) if __name__ == "__main__": diff --git a/tests/scripts/test_check_protocol_consistency.py b/tests/scripts/test_check_protocol_consistency.py index 27c09fc0..3ed5f361 100644 --- a/tests/scripts/test_check_protocol_consistency.py +++ b/tests/scripts/test_check_protocol_consistency.py @@ -9,8 +9,13 @@ from __future__ import annotations +import contextlib import importlib.util +import io +import re +import shutil import sys +import tempfile import unittest from pathlib import Path @@ -108,5 +113,95 @@ def test_every_contract_document_exists(self) -> None: self.assertTrue((ROOT / name).exists(), name) +class InterviewBlockTests(unittest.TestCase): + def test_workflow_carries_the_role_interview(self): + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + self.assertIn(consistency.INTERVIEW_MARKER, text) + self.assertIn("read-only", text) + self.assertIn("claim helper --row", text) + + def test_checker_rejects_a_workflow_without_the_interview(self): + # The mutation this gate exists to catch: the gate ships, the prose + # does not, and agents never learn the precondition. + errors = consistency.interview_errors("# workflow\n\nno interview here\n") + self.assertTrue(errors) + + def test_every_declarable_role_is_named_in_the_interview(self): + # INTERVIEW_REQUIRED is a hand-written tuple, so emptying or narrowing + # it would leave every other assertion in this class green while the + # checker quietly stopped looking at the answers. Bind it to the roles + # agent-role.py actually accepts instead: a fourth answer must reach the + # prose, and dropping one from the checker is a red build. + role = _load("agent_role_for_interview", "scripts/agent-role.py") + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + for name in role.DECLARABLE: + with self.subTest(role=name): + self.assertIn(f"claim {name}", text) + self.assertTrue( + any(f"claim {name}" in n for n in consistency.INTERVIEW_REQUIRED), + f"INTERVIEW_REQUIRED does not cover 'claim {name}'", + ) + + def test_each_required_answer_is_pinned_individually(self): + # A block that exists but has lost one of the three answers is the + # likelier drift, and the marker alone would not see it. + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + for needle in consistency.INTERVIEW_REQUIRED: + with self.subTest(needle=needle): + self.assertTrue(consistency.interview_errors(text.replace(needle, ""))) + + +class InterviewWiring(unittest.TestCase): + """The checker must CALL interview_errors, not merely define it. + + Every assertion above exercises the function directly, so a `main()` that + never calls it leaves them all green while the gate enforces nothing -- + which is the exact shape of the drift this file exists to catch. + """ + + STRIP = re.compile( + r".*?\n?", re.S + ) + + @contextlib.contextmanager + def _tree(self, workflow_text: str): + """Run consistency.main() against a copy of the repo's own documents.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "scripts").mkdir() + (root / ".agents").mkdir() + shutil.copy( + ROOT / "scripts/check-doc-checkpoint.py", + root / "scripts/check-doc-checkpoint.py", + ) + shutil.copy(ROOT / "AGENTS.md", root / "AGENTS.md") + (root / ".agents/workflow.md").write_text(workflow_text, encoding="utf-8") + saved, consistency.ROOT = consistency.ROOT, root + out, err = io.StringIO(), io.StringIO() + try: + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + yield lambda: (consistency.main(), out.getvalue(), err.getvalue()) + finally: + consistency.ROOT = saved + + def test_faithful_copy_passes(self): + """Positive control: the temp tree itself is not what fails below.""" + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + self.assertIn(consistency.INTERVIEW_MARKER, text) + with self._tree(text) as run: + code, _, err = run() + self.assertEqual(code, 0, err) + + def test_main_fails_when_the_interview_is_deleted(self): + text = (ROOT / ".agents/workflow.md").read_text(encoding="utf-8") + stripped = self.STRIP.sub("", text) + self.assertNotIn(consistency.INTERVIEW_MARKER, stripped) + self.assertNotEqual(stripped, text, "the strip pattern matched nothing") + with self._tree(stripped) as run: + code, _, err = run() + self.assertEqual(code, 1) + self.assertIn("role-interview", err) + + if __name__ == "__main__": unittest.main()