c8c2ea7e

Record the loop proven on real VMs, and the bug it found

a73x 2026-09-06 09:20

Commit message

Record the loop proven on real VMs, and the bug it found

RETRO.md

@@ @@ -230,3 +230,83 @@ subagents were unavailable. @@
+  reading real user state. Its two halves are covered
+  (`load_trust_policy_merges_global_and_repo_keys`,
+  `global_trusted_keys_path_returns_config_path`); the composition is not.
+## Proving the agent loop — 2026-09-06
+Ran the claim→clone→work→push→release loop for real: first as two processes on
+one host, then as **two eitri VMs** (created, enrolled, raced, destroyed) against
+a live `git-collab-server`. It worked — and it found a bug in under a minute
+that 57 tests had gone green over.
+Evidence: `loop-w1` claimed with token 1, cloned over SSH with a key it had
+generated inside itself, pushed `refs/heads/loop-w1/version-flag` (`0e7b461`),
+and released; `loop-w2` exited 4 and was told who held the lease and until when.
+The worker contract is in `~/code/rad/workshop/durin/worker.sh`.
+### What worked
+- **Real usage found what tests could not, immediately.** The whole point of
+  the slice. Within one run the demo exposed a bug in the ordinary path.
+- **The worker needs only `ssh` and `git`.** Both VMs ran the loop with no
+  `git-collab` binary, no token and no account — the "contributing requires
+  nothing but plain git" invariant, demonstrated rather than asserted.
+- Each VM generated its own keypair, so no private key ever left the machine
+  that used it, and enrolment was one append to `authorized_keys` — durin's
+  key-pool step, executed by hand and found to be enough.
+- Reading the failure instead of the retro note. The note said this test failed
+  on `matches reject pattern`; it did not.
+### Lessons
+- **Claiming by an abbreviated id worked and releasing by it silently did
+  not.** Lease rows are keyed by the full issue id; `acquire` resolved a prefix
+  against the repository's issue refs, but `renew`/`release` used the argument
+  literally, so `release <prefix>` updated a key no row used — and idempotency
+  reported that as `"status":"released"`, exit 0. The lease stayed held until
+  its TTL, blocking every other worker. This is the *ordinary* path: the CLI
+  abbreviates ids everywhere, so `issue claims` prints exactly the id that
+  `issue unclaim` then failed to release. Fixed in `fef0420` by resolving
+  against the lease table — the only thing that can answer "which lease did you
+  mean" once an issue is closed or its ref is gone.
+- **A test asserted the buggy behaviour.** `release_without_a_lease_succeeds`
+  pinned `"status":"released"` for a release that freed nothing, so the suite
+  did not merely miss the bug, it protected it. Every lease test used a full id
+  on both sides of the claim/release pair, so the abbreviated path had no
+  coverage at all despite `acquire_by_issue_id_prefix_reports_full_id` existing.
+  A status that cannot distinguish "freed a lease" from "there was nothing to
+  free" is a status worth splitting: `release` now answers `not-held`.
+- `pkill -f 'git-collab-server --config /tmp/loop'` matches **the shell's own
+  command line**, so it killed the shell running it — twice, while I blamed the
+  server for dying mysteriously. Use `pkill -f '[g]it-collab-server …'`.
+- `set -e` plus `wait` on a job that is *supposed* to exit non-zero aborts the
+  script before it can report the result. The loser exiting 4 is the point.
+- A background process does not survive the terminal invocation that started
+  it. Run a demonstration in one invocation.
+### What to change
+- **A test that takes an id must exercise the abbreviated form on at least one
+  side of the pair.** That is what the CLI prints, so it is what humans and
+  scripts pass back in. Full-id-only tests are testing a path users do not take.
+### Actions for the next slice
+- [ ] Add the `make base-check REF=<ref>` target (carried forward twice now).
+- [ ] Decide whether to spend the control-socket safety margin (carried
+  forward: `SOCKET_PATH_LIMIT` 100 -> 104).
+- [ ] Audit the remaining lease and issue commands for full-id-only test
+  coverage, now that one such gap has cost a real bug.
+### Retained debt
+- **Whoever builds durin, next worker change:** the worker pushes a *branch*,
+  not a patch, because server-side patch creation from `refs/for/<base>` is
+  forge phase 3. A worker can take work and deliver code; the review round trip
+  is not closed through the forge yet, so the loop is proven only up to "code
+  arrives".
+- **Whoever builds durin, first unattended run:** the loop was driven by hand.
+  Nothing yet watches the queue, hires a VM, or reaps one — and the VMs in this
+  run were created and destroyed by explicit API calls, not by a foreman.
+- **Forge owner, next merge-scan change:** the worker writes a `Fixes: <id>`
+  trailer, which nothing consumes. `merge_scan` reads `Patch:` only, so the
+  issue was not closed by the push that fixed it.