Merge pull request #75 from fiddlerwoaroof/docs/orchestrator-batch

docs: the wave skill, the Copilot gate, and six doctrines a hard reset discarded

.claude/skills/code-review/SKILL.md

@@ -95,9 +95,13 @@ From `design-principles.md` §7 and `doctrine/2`:
 
 ## Verifying
 
+Run the gate **CI runs**, flags and all. Clippy exits 0 on warnings, so without
+`-D warnings` this file was green on exactly the input CI is red on — a check
+that cannot fail in the direction the defect lies.
+
 ```bash
-cargo test --workspace          # must stay under ~2s
-cargo clippy --workspace --all-targets
+cargo test --workspace --no-fail-fast    # fail-fast hides the blast radius
+cargo clippy --workspace --all-targets -- -D warnings
 cargo fmt --all --check
 ```
 

.claude/skills/wave/SKILL.md

@@ -0,0 +1,83 @@
+---
+name: wave
+description: Dispatch parallel agents to work through open beads and shepherd each to a merged PR. Use when an orchestrator is running a wave, partitioning the backlog, or writing a brief for a work-unit agent.
+---
+
+# Running a wave
+
+An orchestrator chunks the open backlog, fans agents out, reviews what comes
+back, and merges. `AGENTS.md` has the history; this is the operating procedure.
+
+## Every agent gets its own worktree. No exceptions.
+
+Dispatch with `isolation: "worktree"`. Without it every agent edits the **same**
+working tree: one `git status` shows four agents' half-finished files, any commit
+sweeps up the others' work, and any `git checkout` destroys it. This has happened
+here. If you discover it mid-wave, stop every agent, have each create a worktree
+and copy across **only files it can prove it touched**, then `git checkout --`
+those files in the primary tree.
+
+In the worktree: `direnv allow`, or `bd` reports a version *mismatch* rather than
+failing cleanly. `direnv status` prints `Found RC allowed 0`, and `0` means
+allowed — it reads like the opposite.
+
+## Partitioning
+
+By **crate or directory**, never by ticket count. **Tickets that share a file go
+to one agent**, to be done sequentially — four widget migrations all touching
+`src/draw.rs` are a guaranteed conflict split four ways. Group first, then count.
+
+Tell each agent what ground is *not* theirs, and to report rather than guess when
+it must cross. Two agents adding a workspace member will still collide in
+`Cargo.toml`/`Cargo.lock`; keep both sides.
+
+## What every brief must carry
+
+1. **The expected base commit, by name.** Worktrees do not reliably branch from
+   HEAD — one was created three merges behind and only noticed because it was
+   told to check.
+2. **The baseline**, so "N passing" is checkable rather than asserted: Rust test
+   count, harness counts, node counts.
+3. **Its ground, and the ground that is not.**
+4. **Never write** `.beads/**`, `prolog-sessions/kb.pl`, `context/repo-map.md`.
+   Agents read them freely and report proposed text; the orchestrator applies it.
+5. **The commit trailers and PR footer**, spelled out.
+6. **Do not push to `main`, do not merge.** The orchestrator merges.
+7. **What to report**: branch and head SHA, PR number, what changed per ticket,
+   *what could not be verified and why*, proposed repo-map/kb text, and **what it
+   learned that the ticket did not predict** — consistently the most valuable
+   part, and omitted unless asked for explicitly.
+
+## Method to require, because this repo's defect is a test that passes
+
+- Failing test first, then the fix.
+- Every guard proved by **injecting the fault it names** and watching it fail
+  **by name**. An injection that does not compile reads as a clean run, so
+  require a positive signal the build ran.
+- `cargo test --workspace` is **fail-fast**; `--no-fail-fast` for any claim about
+  blast radius.
+- A textually clean merge can **delete tests** with no conflict marker, and
+  `cargo check` will not notice. After any merge that moves a test module, diff
+  test **names** — a count is masked by the other side's additions.
+
+## Shepherding
+
+- Wait on checks with `scripts/pr-watch <pr> --expect <sha>`. Never hand-roll a
+  poll; see the `pr-watch` skill for the three ways they report green while wrong.
+- Review before merging. Copilot's quota may be exhausted and its request is a
+  **silent no-op**; an absent review and an unrequested one both read as
+  "pending". Dispatch an adversarial reviewer and give it the head SHA, the base,
+  and a test-merge, so it reviews the tree that will exist.
+- A re-review is of the **delta**, at a named SHA. A review of a superseded
+  commit reads as a pass.
+- Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`.
+- Anything posted to GitHub goes out under the owner's account: open every
+  comment with a line saying a Claude agent wrote it.
+
+## Between waves
+
+Run the code-review skill on the merged range. The characteristic find here is
+not a wrong line, it is **a test that passes while the thing it names is
+broken** — a vacuous fold test, a one-category fixture, a helper that fabricates
+what the code under test needs. Ask the reviewer to inject the fault and watch
+the check fail.

AGENTS.md

@@ -153,6 +153,16 @@ worth it on a repo with other collaborators.
 One PR per wave, built by a **dedicated PR agent in its own worktree**, so waves
 pipeline against it: the next wave runs while the PR absorbs review fixes.
 
+**Copilot is not reviewing this repository, and the request is a silent no-op.**
+Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61,
+#66, #68 and #70 carry none. Both request paths report success and produce zero
+`review_requested` events — the GraphQL mutation returns `clientMutationId: null`
+and the REST call returns 200 with `requested_reviewers: []`. So **do not wait on
+it**: an absent review and an unrequested one look identical from outside, which
+is `doctrine(review_is_requested_not_inherited)` seen from the requester's side.
+Review is an adversarial agent the orchestrator dispatches, given the head SHA.
+The mechanics below are kept for the day it comes back.
+
 Copilot review must be **re-requested after every fix push**; it does not
 re-review on its own. The mutation takes a **`botIds`** field — `userIds` fails
 with *"Could not resolve to User node"*:
@@ -207,8 +217,10 @@ is to branch from `gh/main`, cherry-pick, open a follow-up PR, and delete the
 stray re-creation. Check `gh pr view <n> --json state` *before* pushing a fix to
 a PR you have not looked at in a few minutes, rather than after.
 
-Merge only once Copilot passes. Prefer a **merge commit** — `AGENTS.md` says
-merges over rebases, and a wave's commits are individually meaningful.
+Merge only once a review has passed — an adversarial agent's while Copilot is
+absent, and it must name the head SHA it reviewed. Prefer a **merge commit** —
+`AGENTS.md` says merges over rebases, and a wave's commits are individually
+meaningful.
 
 ### Never hand-edit `.beads/issues.jsonl`
 

README.md

@@ -433,9 +433,10 @@ Caveat, narrowed twice: `harness/` runs `web/app.js` under Chromium — the
 standalone deployment under Firefox too — and `OpfsFs` under Chromium and
 Firefox, in CI. The grouped and multi-level DOM is executed since
 improvise-plcx, against a fixture document the harness serves beside the demo.
-What is still never executed is `bindings.rs`, the CSS and the
-`ResizeObserver` arithmetic — and Safari, whose Playwright build provides no
-OPFS at all.
+What is still never executed is the CSS and the `ResizeObserver` arithmetic —
+and Safari, whose Playwright build provides no OPFS at all. `bindings.rs` **is**
+run: the no-`?ws` test loads the app, which spawns the worker, which calls
+`start()` and `OpfsFs::open()`.
 
 ## What's interesting about the architecture
 

context/test-evidence/clients.md

@@ -857,7 +857,12 @@ command. It failed on the first press past the end — client 0, host 4 — and
 wire was changed to match the effect. Whether both should wrap instead is
 improvise-trd3.
 
-#### Retired — the help spec, `? opens help, and its rows are the shared ones`
+#### "? opens help, and its rows carry the keys and the words, not empty frames"
+
+This entry was demoted to prose as Retired when PR #61 deleted the spec. PR #72
+restored the coverage under a new name, so the claim is live again and the
+heading now names the test that exists. What follows is the original evidence;
+improvise-igxp's stronger assertions are recorded separately below.
 
 improvise-ktv1's acceptance in one place: **a browser shows help.** `?` had
 entered `AppMode::Help` since the keymap was shared, and nothing appeared —
@@ -876,11 +881,18 @@ always open passes any check that only ever opens it.
 It runs against the **assembled** site like every other test here, so it
 covers `render.rs`, the serialized model, `web/app.js` and the CSS together.
 
-**The test no longer exists.** `36ade35` moved the shell suite to node and
-deleted it with the rest of `harness/tests/widgets.spec.js`; nothing replaced
-the assertion, and the heading went on promising it for a whole release --
-which is the cost improvise-5ray names. The heading is demoted rather than
-deleted so the injections above survive for whoever restores the coverage.
+**It was gone for a release, and that is why the entry reads oddly.** `36ade35`
+deleted it with the rest of `harness/tests/widgets.spec.js` and nothing replaced
+it, so the heading promised coverage that did not exist -- exactly the cost
+improvise-5ray names, and invisible because the evidence checker then read only
+`###` headings and only Rust names. PR #69 taught it to see both; PR #72
+restored the coverage under the current name, with the stronger content
+assertions improvise-igxp asked for.
+
+The injections above are the original two and still describe the property. The
+present test is `harness/tests/help.spec.js`, and its own entry below records
+what igxp added: the rows are read as `dt`/`dd` pairs and asserted as text, so
+blanking `keys.textContent` fails it where `toBeVisible` passed.
 
 ### the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable
 

prolog-sessions/kb.pl

@@ -222,6 +222,15 @@ doctrine(a_key_rule_must_ask_whether_the_relation_is_multi_valued,'scripts/kb ke
 doctrine(a_local_shell_is_not_the_ci_shell,'zsh ABORTS on an unmatched glob and bash leaves it as a literal, so a CI step written and probed in zsh can fail in a way no developer shell can reproduce. node --test web/tests/*.test.js under bash therefore globbed the literal itself, matched nothing, reported 0 tests and exited 0 -- both node steps gated nothing the moment their directory moved (improvise-vz54). Local shell vs CI shell is a fault-injection axis in its own right: probe a CI line under the shell the workflow names, not the one you are typing in. The fix moves the glob into scripts/node-test.sh so the expansion is the script\'s and can be refused.').
 doctrine(a_convention_that_keeps_a_checker_quiet_is_the_one_that_blinds_it,'context/test-evidence used #### for browser tests precisely BECAUSE --check-evidence resolved names against Rust fn names only, so a ### heading naming a Playwright test would be reported as an orphan. The workaround made every browser entry invisible, and a deleted test left its heading promising coverage for a whole release (improvise-5ray). Cure: collect ### and #### alike and decide by SHAPE -- a bare identifier is a Rust test, a quoted string is a JS test, anything else is prose naming nothing. When a checker forces a convention on the thing it checks, ask what that convention hides.').
 doctrine(a_refusal_is_evidence_only_when_it_is_the_one_you_named,'Two kb-test cases passed against a tree with the fix they guarded reverted, because a DIFFERENT guard refused the same delta and the assertion was only \"fails, and the kb is unchanged\" (review of PR #69). A tool with several guards will usually refuse a broken delta somehow, so \"it was refused\" is nearly free and says nothing about the fix under test. Assert the REFUSAL MESSAGE, or construct a case only the fix can catch -- and prove it by reverting that one fix and watching the case fail. The same trap in reverse: a case asserting an OUTCOME (the add lands and every sibling survives) can discriminate where a message cannot, because what a wrong key destroys is the outcome.').
+doctrine(a_weak_check_and_a_strong_one_agree_until_the_defect,'Five instances in one wave, and they are one mistake rather than five: a guard watching KeyOutcome::Upstream but not Ignored, clippy with versus without -D warnings, a grep for \"warning[\" versus a bare \"warning:\", toBeVisible versus toHaveText, and a grep for test( versus node own pass count. In every case the weak check and the strong one AGREE ON EVERY INPUT ANYONE TRIED and diverge only on the input that is also the defect -- which is why the weak one survives review and why running it proves nothing. The common cause is a check written against the failure you IMAGINED rather than the outcome the USER SEES. The repair is the same each time: assert over the observable -- is anything on the screen, did the process exit non-zero, did the document move -- rather than over the mechanism, which variant, which log prefix, which substring. Two things in this wave got it right and are the pattern to copy: furnished() seats the panel cursors off the edge so move-panel-cursor can answer the question rather than the test agreeing not to ask it, and scripts/node-test.sh reads node own report instead of grepping for test(.').
+doctrine(every_wave_agent_gets_its_own_worktree,'Dispatching parallel agents without worktree isolation puts all of them in the SAME working tree. Observed 2026-09-20 with four agents: one git status showed one agent effect.rs beside another harness/build.sh and a third scripts/node-test.sh, so any commit would have swept up half-finished work from three units and any checkout would have destroyed it. The tell is cheap -- run git status in the primary tree after dispatch and see whether it holds files from more than one unit. Recovery is per-agent: create a worktree, copy across only the files that agent can PROVE it touched, then git checkout -- those files in the primary tree; an agent that guesses which files are its own makes it worse. Ambiguity is real -- two units can legitimately both be in crates/improvise-protocol -- so the instruction is to check an edit history, not to infer from the ticket.').
+doctrine(the_orchestrator_delegates_substantial_work,'User instruction 2026-09-20: substantial work goes to an agent in a worktree rather than being done in the orchestrator, and repeated tasks become a script or a skill rather than being retyped. The obvious reason is that heavy orchestrator work blocks every sub-agent waiting on it. The non-obvious one is that orchestrator work lands in the SHARED primary tree, so it collides with whatever the agents are doing there -- an orchestrator fixing a bug in scripts/ while an agent owns scripts/ is a conflict the agent did not agree to. Corollary: a brief retyped by hand drifts between dispatches, which is what the wave skill exists to stop.').
+doctrine(a_message_assertion_is_a_uniqueness_claim_about_the_whole_vocabulary,'A test asserting WHICH refusal fired -- rather than that something refused -- is only as strong as that message being unique across every refusal the code can emit. The moment two share a phrase it degrades back to \"something refused\", the exact defect it was written to repair, wearing a more convincing sentence. So when an assertion moves onto a message, enumerate the whole error vocabulary: on PR #69 the reviewer listed all seven REFUSED strings in kb-merge.pl and confirmed each asserted phrase appears once, in the refusal its case names. Where no message can discriminate -- a wrong has_key is always caught by check_total, the backstop -- assert the OUTCOME instead, and guard the backstop separately or a case can lean on it while claiming to test something else.').
+doctrine(run_the_gate_ci_runs,'A local gate differing from CI in its flags or its output filter can only fail in directions the defect is not. On PR #71 the author ran clippy WITHOUT -D warnings and grepped for a lint name in brackets, which cannot match the bare \"warning:\" unused_imports emits, so the check printed zero and read clean while CI went red. Worse, the repo own code-review skill specified the same flagless invocation, so it was the INSTRUCTION rather than one person shell mistake -- a reviewer following it would report clean and be wrong. Invoke the gate exactly as the workflow does, flags and all.').
+doctrine(an_observation_correct_about_what_changed_can_be_wrong_about_when_it_is_read,'improvise-pdvm/75ld replaced a DECLARED classification with an OBSERVED one, which is right. The cost landed on WHEN the observation is read: comparing document_config once AROUND the batch is correct about what changed and wrong about ordering, because Save clears dirty INSIDE the loop, so [SetNumberFormat, Save] saved and then re-raised the flag. The fix is position, not suppression -- the comparison moved into the loop, using |= so it only ever sets. The asymmetry that decides it: nothing ever CLEARS layout_version, so an end-of-batch comparison is right there, while dirty has an effect that unsets it mid-batch. The discriminating probes rule out BOTH wrong fixes: [SetNumberFormat] alone must still be dirty, or LayoutGeometry has been demoted and the document never saves; and [Save, SetNumberFormat] must be dirty, or the fix is a blanket never-dirty-after-save.').
+doctrine(a_siblings_unmerged_pr_is_invisible_to_bd_search,'AGENTS.md requires a bd search before filing. In a parallel wave that is not sufficient: a fix living in a sibling agent UNMERGED PR is in no tracker state at all, so the search correctly returns nothing and the duplicate is filed anyway. Observed 2026-09-20 -- the tooling agent filed improvise-v5pc to restore a deleted help assertion while the harness agent was landing exactly that in PR #72. Neither agent erred. Check OPEN PRS as well as the tracker, and have the orchestrator reconcile filings against merged work at wave end rather than trusting each agent to have seen the others.').
+doctrine(a_hard_reset_discards_knowledge_written_to_the_working_branch,'The orchestrator wrote doctrines to kb.pl on local main, pushed each as a branch, then ran git reset --hard gh/main to resync -- twice -- and silently discarded six clauses that existed only in the unmerged local commits. Nothing failed: kb_consistent passed on the reduced file, the counts simply read lower, and the loss was found only by grepping for clause names remembered from the session. Two rules follow. Write a fact ONCE, on the branch that will carry it to main, not on a local main that is going to be resynced. And after any reset or force-move of a branch holding knowledge, re-count the predicates and diff clause NAMES against what you believe you wrote -- a total that merely looks plausible is the same non-evidence a passing test count is.').
+doctrine(a_checker_that_reads_headings_cannot_see_a_contradicting_body,'check-evidence validates that a heading names a test that exists. It says nothing about the PROSE under the heading, which is what a human actually reads. Demonstrated on this repo 2026-09-20: renaming a retired entry to the test that had been restored made the checker green while the body three paragraphs down still said \"The test no longer exists\" and \"nothing replaced the assertion\" -- one entry making mutually exclusive claims, and the gate could not see it because the gate was never about that. Found by review, not by the checker. The general form is that a mechanical check licenses exactly the claim it makes and no neighbouring one, so a rename is a read of the whole entry rather than an edit of its first line -- a_fact_stated_twice_is_changed_by_grep applied WITHIN a document rather than across them.').
 work_done('improvise-8tk',bug_fix,'ZZ/:wq quit even when the save failed, silently discarding the model. Save now aborts the effect batch on failure; save_to_current_path/1 returns Result<String,String>; three tests in ui/effect.rs.').
 work_done('improvise-907',bug_fix,'Import wizard step machine redesign: validation_error() gate at ReviewProposals (no-category and over-limit), history-stack previous() bound to Left on every step, and Done made a real confirmation step holding the built workbook. Superseded improvise-gfq, improvise-o1c, improvise-c6j.').
 work_done('improvise-6sq',wont_do,'Merging EditOrDrill and EnterEditAtCursorCmd into one EnterEdit{mode} is not safe: they differ in WHEN the cell value is read, not in a mode parameter. Added ui/app.rs::tests::i_and_o_diverge_on_an_empty_records_grid as executable evidence.').
@@ -460,6 +469,7 @@ kb_issue_found(static_str_blocks_deserialize,'a borrowed &\'static str field mak
 kb_issue_found(ticket_premise_false_8uz,'improvise-8uz asserted selected/row_offset/col_offset were \"PERSISTED in .improv\". They were not: improv.pest has no cursor production and format_md emitted none. The claim shaped the ticket\'s whole option set, so it was checked with a test before any of the options was weighed.').
 kb_issue_found(ticket_premise_false_rml,'improvise-rml called category_panel.rs an \"exact copy\" of tile_bar.rs. It was not: the colors were identical but the labels differed (\"Row ↕\" vs \"Row\", \"None ∅\" vs \"·\"). Only the color table was true duplication; the four label sets are four deliberate vocabularies and were preserved as four methods.').
 kb_issue_found(duplicated_active_view,'adding ViewState::active_view created a second copy of \"which view is active\". The improvise-8uz characterization test caught the desync immediately because it called workbook.switch_view directly. Held in lockstep by two writers until improvise-jytm, which made the divergence deliberate: the guard test the_two_active_view_names_never_diverge was REPLACED, not deleted, by the_documents_default_view_outlives_this_clients_switches and a_fresh_session_opens_the_documents_default_view -- a deliberate split with no guard is how one silently regresses into the other.').
+kb_issue_found(copilot_stopped_reviewing_this_repo,'Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 copilot-pull-request-reviewer[bot] reviews; #61, #66, #68 and #70 carry none. Both request paths report success and register nothing -- the GraphQL requestReviews mutation with botIds BOT_kgDOCnlnWA returns clientMutationId null, and POST /pulls/<n>/requested_reviewers with copilot-pull-request-reviewer[bot] returns 200 with requested_reviewers empty -- and no review_requested timeline event appears. Cause unknown; something changed between #27 and #61. Consequence for the process: AGENTS.md said \"merge only once Copilot passes\", which is an instruction to wait forever on a review that was never requested, since an ABSENT review and an UNREQUESTED one look identical from outside. That is doctrine(review_is_requested_not_inherited) seen from the requesters side, and doctrine(an_advisory_gate_is_not_a_gate) with the gate removed rather than never added. AGENTS.md now says review is an adversarial agent the orchestrator dispatches, naming the head SHA, and keeps the Copilot mechanics for the day it returns.').
 functional(invariant/2).
 functional(location/3).
 functional(doctrine/2).