RETRO.md

Ref: main   Size: 22.2 KiB   History

# Retrospectives

One section per delivered slice, newest last. Each carries what was delivered,
what worked, what to change, and **actions the next slice must close** — marked
`[x]` with evidence, or carried forward with a reason. Retained debt gets an
owner and a *trigger* rather than a date, so it fires when someone next touches
that area instead of rotting.

This file is an input, not a diary: read the open actions before scoping the
next slice. Lessons that generalise past their slice get promoted into
`CLAUDE.md`, which is loaded every session — this file is not. The format is
borrowed from `~/code/rad/mux`, whose `RETRO.md` has closed 49 actions this
way.

## file-history view (date not recorded)

Retrofitted into this structure from flat notes; the lessons are verbatim, and
no narrative has been reconstructed around them.

### Lessons

- the `git2` revwalk API is `set_sorting`, not `set_sort_mode` (API drift from docs I'd seen); wasted a compile cycle on the rename.
- wrote a comment claiming `SORT_NONE` was unstable for same-timestamp commits — never tested it. The demo proved both `NONE` and `TOPOLOGICAL` are stable; only `TIME` is broken. A review caught the unverified claim.
- `.take(limit)` was before `.filter_map()`, so a repo with 200+ commits could show an empty history if early commits didn't touch the file. Caught in self-review, not by a test.
- 404'd on deleted files because the handler validated the path exists at HEAD. A deletion test caught it — the existence check was wrong.
- (P1) compared only parent 0, so a clean non-fast-forward merge whose entry matched a non-first parent was listed alongside the source-branch commit. Fixed by comparing all parents (git's TREESAME rule: suppress if matching any). A merge test caught it.
- (P2) compared only blob OIDs, so a mode-only change (chmod +x, same oid) was omitted. Fixed by including the tree-entry mode in the comparison. A chmod test caught it.
- the all-parents comparison made a root commit with no parents and no file vacuously "differs from all parents" — a never-tracked path showed the initial commit. Fixed by requiring root commits to actually introduce the path.

### What to change

Five of these six are one shape: **the rule was right for the common case and
wrong at a boundary** — first parent vs all parents, blob oid vs oid+mode, the
vacuous root commit, `take` before `filter`. Enumerate the boundary members of
a set before writing the happy path.

## push-to-create, rejected — 2026-08-25

### Lessons

- probing git.a73x.sh with bare ssh hung 2 min (no timeout) and I cloned settings fresh before checking ~/code/settings existed; wrap remote probes in `timeout` and look for a local clone first.

## Patch web diff (date not recorded)

### Lessons

- proposed recomputing merge-base at display time before reading `patch.rs`, which already prefers the *recorded* base (a recomputed merge-base equals head once a patch lands, rendering empty) — reading the CLI's resolution first would have skipped a wrong design.

## Issue leases — phase 1 of server-authoritative collab — 2026-09-06

Delivered atomic TTL work claims on issues: a SQLite lease store, the
`collab-lease` SSH exec verb (acquire/renew/release/list), `issue
claim|unclaim|renew|claims` on the CLI, and claims shown in the web UI. The
SSH key is the credential — no tokens were added. 8 planned tasks, 6 commits,
57 new tests; `cargo clippy --all-targets` clean. Plan:
`docs/superpowers/plans/2026-09-05-issue-leases.md`.

### What worked

- Writing the plan before implementing gave every task a failing-test-first
  shape, and the e2e tests were what caught the one real design bug.
- Extracting `remote_ssh` before the CLI needed it kept `release` and `lease`
  on one SSH client path rather than two.
- Deriving the lease `repo` key in one function (`leases::repo_key`) once two
  callers appeared. The symptom of skipping that would have been a claim that
  exists over SSH and is invisible on the page.
- Writing the README section forced a check of what the commands actually
  print, which is how the un-abbreviated ids were caught.

### Lessons

- the plan told me to `DELETE` a lease row on release and to reap expired rows on every read. Both reset the per-issue `token` to 1, destroying the one property a fencing token has (tenure 1's zombie passing a fresh tenure 1's check). The reaping path was worse: *reading* the lease list would have silently reset fencing. An e2e test asserting the second tenure gets token 2 caught it; the fix is that rows are a tenure ledger, never deleted — `holder IS NULL` means free.
- two liveness predicates (`Lease::live` and `Row::held`) existed briefly for the same question. Deleted the unused one rather than leaving both to drift.
- to check whether some unrelated `trust::tests` failures pre-dated my work I did `git stash -u` + `switch main` + `switch back` + `stash pop` inside a Delta worktree. The `&&` chain didn't complete, and I landed on `main` with an apparently-empty stash list — the WIP looked lost (it wasn't; `git reflog stash` still had it). Under a replicating VFS, don't stash-and-switch to inspect a baseline: read the file, or use a throwaway `git worktree`. The failures were in `trust.rs`, which I had never touched — the check wasn't worth the risk in the first place.
- repeated the stash-and-switch mistake I had *just* written up, two hours later, to check the same kind of pre-existing failure. The `&&` chain broke at the same place and the tree came back clean with the work in `stash@{0}` again. The right tool was there all along: `git worktree add --detach /tmp/base-check <base>` gives a pristine checkout with no effect on the working tree, and that is what finally confirmed both failures were pre-existing. Writing the lesson down is not the same as applying it — when a command would touch the index or HEAD to answer a question, answer the question a different way.
- committed the first five commits straight onto `main`, which is the branch Delta's Changes view diffs *against*: the UI showed nothing while five commits sat there, and the work had to be moved onto `leases` after the fact. Branch before the first commit of a slice, never onto the base.
- `git add -A` swept `.codex`, `.superpowers/` scratch and two unrelated untracked docs into a commit that was supposed to be one plan file. Stage by path.

### What to change

- **A plan is a hypothesis, not an authority.** This is the first bug here whose
  source was the plan document itself, faithfully implemented. Plans are very
  detailed in this repo (the release plan is ~2,400 lines with code), which
  mostly helps but means a wrong instruction gets built exactly as written. So
  plans state **invariants** beside steps, and a step that contradicts one is a
  plan bug to fix in the document (done: `571c422`).
- Lessons were being filed in this file, which nothing reads at decision time.
  Anything that generalises now goes to `CLAUDE.md`.

### Actions for the next slice

Owner: whoever scopes the next slice. Mark `[x]` with evidence, or carry
forward with a reason. A passing test does not by itself close an action about
judgement or process.

- [x] Promote the generalisable rules from this file into `CLAUDE.md`'s manual
  section, each naming the failure it prevents. Evidence: the Invariants and
  Working rules sections added 2026-09-06.
- [x] State the invariants that existed only in my head or in scattered code
  comments (lease token monotonicity, the identical-reply rule, append-only
  events, plain-git contributing). Evidence: `CLAUDE.md` Invariants section.
- [x] **Make `cargo test` green so the gate stops being decorative.** Done
  2026-09-06 (`7a72f5a`, `c461e69`): `cargo test` exits 0 across 61 targets.
  Nine `trust::tests` now load through the `load_trust_policy_with_global`
  seam. The sync failure's cause was **not** what this action claimed — see
  the next section; I had attributed it to a `matches reject pattern` line
  that a *passing* governance test prints, without verifying it. It was a
  control-socket path overflow, and a real product bug.
- [x] Add a `make base-check REF=<ref>` target wrapping
  `git worktree add --detach` + test + `worktree remove`, so the safe way to
  inspect a baseline is one command. A note alone demonstrably did not hold.
  Done in `ea15d81`; missing and invalid refs fail without moving `HEAD`, test
  failures fail the target, and successful checks clean their worktree.
- [x] Add the boundary-member checklist (empty, single, first/root,
  duplicate/already-seen, freed-then-reused) to the plan-writing routine, and
  require plans to state invariants beside steps. Evidence: the "Plans state
  invariants, not just steps" section of
  `.agents/skills/sprint-delivery/SKILL.md`, adopted 2026-09-06.

### Retained debt

- **Lease owner, next lease change:** fencing tokens are stored and reported
  but nothing verifies them — there is no server-mediated write to fence until
  phase 3. When patch revisions start flowing through the server, every
  holder-authored write must carry its token and stale ones must be refused.
- **Web UI owner, next issue-page change:** an ungoverned server shows the raw
  key fingerprint (`key:SHA256:…`) as the claim holder. Correct, unreadable.
  Governance already supplies a person's name; ungoverned has nothing better
  without a local alias map.
- **Lease owner, when a repo is deleted:** lease rows are keyed by repo path
  and never reclaimed, so deleting a repository leaves its rows behind. Harmless
  at any plausible size (one row per ever-claimed issue) and deliberate — the
  rows carry the fencing high-water mark — but it is unbounded growth with no
  sweeper.
- **Whoever runs the next slice:** the sprint-delivery workflow is adopted but
  unexercised — no slice has been delivered under it, and its independent-review
  step degrades to a disclosed self-review while subagents are unavailable here.
  The first real use is also its test.

## A green baseline — 2026-09-06

Delivered `cargo test` exiting 0 across 61 targets, from ten failures that
predated this work. Both causes were tests reading state they did not own, but
one of them was hiding a real product bug. Run under the newly adopted
sprint-delivery workflow; its review step was a disclosed self-review, because
subagents were unavailable.

### What worked

- **Diagnosing before believing my own notes.** The open action asserted the
  sync test failed with `matches reject pattern`. It does not. That string is
  printed by a *passing* governance test that asserts a push is refused, and I
  had grepped a `--no-fail-fast` run for `FAILED` and attributed another test's
  expected output to this one. Reading the actual failure took one command and
  found something entirely different.
- Proving the arithmetic in the shell before editing the guard: the directory
  is 59 bytes, the old budget computed exactly 100 against a limit of 100, and
  the path ssh actually opened was 117.
- Fixing nine call sites through one test helper rather than nine edits, so the
  hermetic path is the easy one, plus a warning at the definition site where
  someone reaching for the wrong function will read it.
- The self-review pass earned its place: it caught that a stricter budget
  silently *removes* working sharing in a narrow band, which I would not have
  thought to measure otherwise.

### Lessons

- **`git-collab sync` was broken outright under a long `TMPDIR`, on any
  platform.** `ssh_share::socket_dir` budgeted for the 40-character `%C` hash
  but not for the `.XXXXXXXXXXXXXXXX` that ssh appends while bringing the
  master up and renames away afterwards — 17 bytes it never accounted for. A
  directory of 51–59 bytes therefore passed the guard and handed ssh a path it
  refused, so the *fetch* died with `unix_listener: path "…" too long` and
  exit 128. Not a degraded optimisation: no sync at all. The guard's author had
  anticipated exactly this failure and written the check for it; the check was
  just short by the one component nobody sees, because it exists only between
  `bind` and `rename`.
- **A test that reads what it does not own passes or fails by accident.** Nine
  trust tests read the developer's real `~/.config/git-collab/trusted-keys`
  through a convenience wrapper, and the sync test inherited whatever `TMPDIR`
  the environment handed it. Both had injection seams available already
  (`load_trust_policy_with_global`, and `TMPDIR` on the child process); neither
  used them.
- A red baseline cost more than the ten tests. It is what made "is this mine?"
  expensive enough that I twice risked the working tree to answer it, and it is
  why a genuine product bug sat behind a line item that said "non-hermetic
  test".
- **My first regression test did not catch the bug, and I only found that out
  by checking.** It used a *deliberately long* `TMPDIR`, which the old budget
  refused as well — so it passed with the fix neutralised and proved nothing.
  The bug lived in a band (a 28-38 byte `TMPDIR`) that the old budget accepted
  and ssh then rejected; only a fixture sitting inside that band reproduces it.
  "Long enough to fail" was the intuition and it was wrong by construction: a
  guard that is too *strict* fails safe, so testing past the boundary tests the
  wrong side of it. **Run a regression test against the unfixed code and watch
  it fail** — with the fix reverted this one reproduces the original error
  verbatim (`unix_listener: path "…" too long`, exit 128).

### What to change

- When a retrospective action states a *cause*, it states evidence for it or
  says the cause is unconfirmed. This one asserted a cause from a misread grep,
  and the assertion survived into `CLAUDE.md` before being checked.
- Grep `test result:` and capture `$?`; never grep a test run for `FAILED`.
  Expected-failure output exists and it says `FAILED` too. Now in `CLAUDE.md`.

### Actions for the next slice

- [x] Add the `make base-check REF=<ref>` target. Done in `ea15d81`; both its
  passing and failing cleanup paths were exercised without moving `HEAD`.
- [x] Decide whether to spend the control-socket safety margin. Keep
  `SOCKET_PATH_LIMIT` at 100: sharing is an optimisation, the fallback is
  correct, and no observed sync problem justifies spending the portable
  margin. Revisit only under the retained debt's slow-sync trigger.

### Retained debt

- **Sync owner, next `ssh_share` change:** the corrected budget declines to
  share for temp directories of 43–50 bytes (Linux) or 43–46 (macOS) that
  previously shared successfully — the band between the conservative
  `SOCKET_PATH_LIMIT` of 100 and the real `sun_path` limits of 108 and 104.
  Correctness over an optimisation, and consistent with the constant's existing
  4-byte slack, but it is coverage spent. Raising the limit to the portable
  minimum of 104 would recover most of it.
- **Sync owner, when a sync is slow for no visible reason:** declining to share
  is silent. There is no way to tell a one-login sync from a two-login one
  without reading the server log, so the band above is invisible in use.
- **Trust owner, next `trust.rs` change:** `load_trust_policy` — the wrapper
  that merges the global file — now has no test, because testing it requires
  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

- [x] Add the `make base-check REF=<ref>` target. Done in `ea15d81`.
- [x] Decide whether to spend the control-socket safety margin. Keep 100 until
  the retained slow-sync trigger fires; correctness beats recovering a silent
  optimisation without evidence.
- [x] Audit the remaining lease and issue commands for full-id-only test
  coverage. `claim`, `renew`, and `unclaim` now exercise the abbreviated form
  in `lease_cli_test` (`4d802a0`), and the release case fails against the
  unfixed `4a60f08` as expected. Every local issue id-taking operation resolves
  through `state::resolve_issue_ref`; its unique and ambiguous prefix rules are
  covered in `abbrev_test`.

### 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.

## Closing the lease branch — 2026-09-06

Closed the process actions that had accumulated behind the lease slice before
making the forge a dependency of an unattended foreman.

### What was delivered

- `make base-check REF=<ref>` tests another revision in a detached temporary
  worktree and removes it on success or failure without moving the caller's
  `HEAD` or index.
- The CLI lease suite now sends abbreviated issue ids through `claim`, `renew`,
  and `unclaim`, matching the ids humans and scripts get back from list output.
- Ambiguous lease-prefix coverage creates 17 issues, so the pigeonhole
  guarantee replaces a random early return that could skip the assertion.
- The SSH socket limit stays at 100 bytes. The 100-to-104 band is retained debt,
  not an unmeasured reason to spend a portability margin.

### What worked

- Running the unclaim test against pre-fix `4a60f08` reproduced the real bug:
  the command reported success but a second principal was still refused.
- The new baseline target's failure path found the nine trust failures on the
  old `main`, then removed its linked worktree and left the lease branch
  untouched. Its success path ran all tests at `HEAD` and did the same cleanup.
- Auditing the issue mutation functions found one resolver
  (`state::resolve_issue_ref`) rather than a collection of command-specific
  prefix rules, so representative boundary tests cover the shared rule.

### Lessons

- A probabilistic collision test that returns early is not coverage. When the
  namespace has 16 members, creating 17 inputs makes the boundary deterministic.
- A green baseline on an unlanded branch does not make the base branch green.
  Landing the fixes is part of closing the slice, not release administration to
  defer indefinitely.

### Review and retained debt

Review was a deliberately adversarial self-review, not independent: subagents
were unavailable. It found and removed the probabilistic test escape. No new
duplicate predicate or temporary state remains. Existing trigger-bound debt
above is unchanged: fencing waits for server-mediated revisions, silent SSH
sharing waits for a measured slow sync, and raw ungoverned holder ids wait for
the next issue-page change.