a73x

16d61346

Promote retro lessons into CLAUDE.md and restructure RETRO.md around actions

a73x   2026-09-06 08:27

Commit message
Promote retro lessons into CLAUDE.md and restructure RETRO.md around actions
CLAUDE.md
Old New
@@ -34,4 +34,122 @@ Rust 2021 edition: Follow standard conventions
34 34
35 35
36 <!-- MANUAL ADDITIONS START --> 36 <!-- MANUAL ADDITIONS START -->
37
38 ## Invariants — do not break, they are load-bearing
39
40 Stated here because a *plan* once instructed the opposite and was implemented
41 faithfully (2026-09-06, lease tokens). A step that contradicts one of these is
42 a bug in the plan, to be fixed in the plan document.
43
44 - **Contributing code requires nothing but plain git.** Everything else may
45 require the server. This replaced the older "if it only works with
46 `git-collab-server`, it is not a git-collab feature" test on 2026-09-05; see
47 `docs/superpowers/specs/2026-09-05-server-authoritative-collab-design.md`.
48 - **The event DAG is append-only.** A correction is a new event, never an edit
49 of an old one, so the record stays an audit trail. `patch log --timeline` is
50 where that is visible.
51 - **Every historical `event.json` shape stays readable.** Events are signed, so
52 nothing can rewrite one; making a field required broke 39 patches in the
53 user's repositories once (see the `PatchCreate.commit` comment in
54 `src/event.rs`, which is load-bearing prose). "Absent" is a state the types
55 must be able to express.
56 - **Unknown repo and unauthorized repo answer identically.** Same bytes, same
57 exit code, over SSH and HTTP alike, so an error cannot be used to probe which
58 private repositories exist. Enforced in `handle_release_command`,
59 `handle_lease_command` and the HTTP 404-collapsing in `git_http.rs`.
60 - **Lease rows are a per-issue tenure ledger and are never deleted.** Freeing
61 one (release *or* expiry) clears `holder`; `token` only ever increments.
62 Deleting a row, or sweeping an expired one, resets the counter and destroys
63 the only property a fencing token has. Reads filter, they do not reap.
64 - **Revision refs are named by commit OID, not revision number.** `r/<n>` does
65 not survive two clones revising offline.
66 - **One rule per question.** Liveness of a lease is `Row::held` and nothing
67 else; two predicates for one question is how they drift.
68 - **The `commit-msg` hook never fails a commit.** It runs inside `git commit`;
69 the one thing it must not do is give git a reason to abort. No matching
70 patch, several, a detached HEAD, or no binary on `PATH` all leave the message
71 exactly as written.
72 - **Prose abbreviates ids; `--json` carries all forty characters.** The JSON id
73 is what a script holds on to. `abbrev::for_issues`/`for_patches` compute the
74 display width from the whole id set, never from the filtered rows on screen.
75
76 ## Working rules
77
78 Each of these exists because of a specific failure; the date is when it bit.
79
80 - **Never touch the index or HEAD to answer a question.** No
81 `stash`/`switch`/`reset` to check whether a failure pre-dates your work — use
82 `git worktree add --detach /tmp/base-check <ref>`. Under Delta's replicating
83 VFS a broken `&&` chain left the tree clean and the work in `stash@{0}`,
84 twice in one session, the second time two hours after writing the lesson down
85 (2026-09-06).
86 - **Branch before the first commit of a slice.** Delta's Changes view diffs the
87 thread's branch against its base, so committing onto `main` renders an empty
88 diff while the work piles up invisibly (2026-09-06, five commits).
89 - **Stage by path, never `git add -A`.** It swept `.codex`, `.superpowers/`
90 scratch and two unrelated untracked docs into a one-file commit (2026-09-06).
91 - **Wrap remote probes in `timeout`, and look for a local clone before cloning
92 fresh.** A bare `ssh` probe of git.a73x.sh hung two minutes (2026-08-25).
93 - **Read the existing resolution before proposing a design.** A display-time
94 merge-base recomputation was proposed before reading `patch.rs`, which
95 already prefers the *recorded* base — a recomputed merge-base equals head
96 once a patch lands, rendering an empty diff.
97 - **Do not write a claim in a comment you have not tested.** A comment asserted
98 `SORT_NONE` was unstable for same-timestamp commits; only `TIME` is.
99 - **Enumerate the boundary members before the happy path.** Empty, single,
100 first/root, duplicate or already-seen, freed-then-reused. Five separate
101 file-history bugs and one lease bug were all one shape: the rule was right
102 for the common case and wrong at a boundary.
103 - **Ask the outside about the inside.** A component reporting on itself cannot
104 catch itself being wrong: keys come from `ssh-keygen`, not our own encoder;
105 interop is asserted through the real `ssh` client and real `git`.
106 - **Capture `$?` before piping.** `cargo test | grep -c ok` reports grep's exit
107 status, not the build's.
108 - **A green suite is not user acceptance.** Say what was verified and what was
109 only asserted.
110
111 ## Gates
112
113 `make lint` (fmt + check + clippy) before a commit; `make ci` (lint + test +
114 build) before delivery. `cargo clippy --all-targets` is expected to be silent.
115
116 **The baseline is not green, and was not before you arrived** (measured at
117 `702a1e6`, 2026-09-06): nine `trust::tests` fail because they call
118 `load_trust_policy`, which reads the developer's real
119 `~/.config/git-collab/trusted-keys` rather than using the
120 `load_trust_policy_with_global` seam that exists for injecting one; and
121 `a_sync_authenticates_to_the_server_once` fails with `matches reject pattern`.
122 Both are non-hermetic tests reading real user state. Until they are fixed,
123 `cargo test --no-fail-fast` is the only way to see the other targets, and a new
124 failure has to be told apart from these ten by name. Fixing them is an open
125 action in `RETRO.md`.
126
127 ## Reading this repo without burning context
128
129 `wc -l` on 2026-09-06; re-measure before trusting a figure. Prefer `grep -n`
130 for the symbol, then `sed -n 'A,Bp'` for a window.
131
132 - **Do not `cat` these:** `src/tui/mod.rs` (2.7k), `src/state.rs` (2.4k,
133 the DAG-replay materialisers), `src/patch.rs` (2.1k),
134 `src/server/ssh/session.rs` (1.9k), `src/lib.rs` (1.8k, the command
135 dispatch), `src/tui/widgets.rs` (1.5k), `src/server/governance/conf.rs`
136 (1.3k), `src/cli.rs` (1.2k), `src/sync.rs` (1.1k),
137 `src/server/repos.rs` (1.1k), `src/server/setup.rs` (1.1k). Tests too:
138 `tests/sync_test.rs` (2.6k), `tests/common/mod.rs` (2.0k, the harness),
139 `tests/collab_test.rs` (1.8k).
140 - Plans under `docs/superpowers/plans/` are implementation scripts and run to
141 thousands of lines with code inline; read the task you are on.
142 - Pipe command output (`| tail -30`, `grep -c`, `2>/dev/null`). A full
143 `cargo test` run is thousands of tokens of re-billed noise.
144
145 ## Where the answers live
146
147 `docs/superpowers/specs/` designs, `docs/superpowers/plans/` implementation
148 plans (the current direction is the 2026-09-05 server-authoritative design and
149 its five phases) · `docs/design-debt.md` accepted debt and when to revisit ·
150 `RETRO.md` open actions and retained debt, **read before scoping a slice** ·
151 `README.md` user-facing behaviour, and the place a claim about the CLI's output
152 gets checked · `~/code/rad/workshop` the agent workshop this server is one
153 piece of (`heimdall`, `durin`).
154
37 <!-- MANUAL ADDITIONS END --> 155 <!-- MANUAL ADDITIONS END -->
RETRO.md
Old New
@@ -0,0 +1,135 @@
1 # Retrospectives
2
3 One section per delivered slice, newest last. Each carries what was delivered,
4 what worked, what to change, and **actions the next slice must close** — marked
5 `[x]` with evidence, or carried forward with a reason. Retained debt gets an
6 owner and a *trigger* rather than a date, so it fires when someone next touches
7 that area instead of rotting.
8
9 This file is an input, not a diary: read the open actions before scoping the
10 next slice. Lessons that generalise past their slice get promoted into
11 `CLAUDE.md`, which is loaded every session — this file is not. The format is
12 borrowed from `~/code/rad/mux`, whose `RETRO.md` has closed 49 actions this
13 way.
14
15 ## file-history view (date not recorded)
16
17 Retrofitted into this structure from flat notes; the lessons are verbatim, and
18 no narrative has been reconstructed around them.
19
20 ### Lessons
21
22 - 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.
23 - 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.
24 - `.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.
25 - 404'd on deleted files because the handler validated the path exists at HEAD. A deletion test caught it — the existence check was wrong.
26 - (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.
27 - (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.
28 - 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.
29
30 ### What to change
31
32 Five of these six are one shape: **the rule was right for the common case and
33 wrong at a boundary** — first parent vs all parents, blob oid vs oid+mode, the
34 vacuous root commit, `take` before `filter`. Enumerate the boundary members of
35 a set before writing the happy path.
36
37 ## push-to-create, rejected — 2026-08-25
38
39 ### Lessons
40
41 - 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.
42
43 ## Patch web diff (date not recorded)
44
45 ### Lessons
46
47 - 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.
48
49 ## Issue leases — phase 1 of server-authoritative collab — 2026-09-06
50
51 Delivered atomic TTL work claims on issues: a SQLite lease store, the
52 `collab-lease` SSH exec verb (acquire/renew/release/list), `issue
53 claim|unclaim|renew|claims` on the CLI, and claims shown in the web UI. The
54 SSH key is the credential — no tokens were added. 8 planned tasks, 6 commits,
55 57 new tests; `cargo clippy --all-targets` clean. Plan:
56 `docs/superpowers/plans/2026-09-05-issue-leases.md`.
57
58 ### What worked
59
60 - Writing the plan before implementing gave every task a failing-test-first
61 shape, and the e2e tests were what caught the one real design bug.
62 - Extracting `remote_ssh` before the CLI needed it kept `release` and `lease`
63 on one SSH client path rather than two.
64 - Deriving the lease `repo` key in one function (`leases::repo_key`) once two
65 callers appeared. The symptom of skipping that would have been a claim that
66 exists over SSH and is invisible on the page.
67 - Writing the README section forced a check of what the commands actually
68 print, which is how the un-abbreviated ids were caught.
69
70 ### Lessons
71
72 - 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.
73 - two liveness predicates (`Lease::live` and `Row::held`) existed briefly for the same question. Deleted the unused one rather than leaving both to drift.
74 - 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.
75 - 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.
76 - 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.
77 - `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.
78
79 ### What to change
80
81 - **A plan is a hypothesis, not an authority.** This is the first bug here whose
82 source was the plan document itself, faithfully implemented. Plans are very
83 detailed in this repo (the release plan is ~2,400 lines with code), which
84 mostly helps but means a wrong instruction gets built exactly as written. So
85 plans state **invariants** beside steps, and a step that contradicts one is a
86 plan bug to fix in the document (done: `571c422`).
87 - Lessons were being filed in this file, which nothing reads at decision time.
88 Anything that generalises now goes to `CLAUDE.md`.
89
90 ### Actions for the next slice
91
92 Owner: whoever scopes the next slice. Mark `[x]` with evidence, or carry
93 forward with a reason. A passing test does not by itself close an action about
94 judgement or process.
95
96 - [x] Promote the generalisable rules from this file into `CLAUDE.md`'s manual
97 section, each naming the failure it prevents. Evidence: the Invariants and
98 Working rules sections added 2026-09-06.
99 - [x] State the invariants that existed only in my head or in scattered code
100 comments (lease token monotonicity, the identical-reply rule, append-only
101 events, plain-git contributing). Evidence: `CLAUDE.md` Invariants section.
102 - [ ] **Make `cargo test` green so the gate stops being decorative.** Nine
103 `trust::tests` read the developer's real `~/.config/git-collab/trusted-keys`
104 instead of using the existing `load_trust_policy_with_global` seam;
105 `a_sync_authenticates_to_the_server_once` fails on refs/collab pushes with
106 `matches reject pattern`. Both confirmed failing at base `702a1e6`, so
107 neither is new. Six of this file's entries end with "a test caught it" —
108 tests are the primary defence here, and a permanently-red baseline means new
109 red cannot be told from old red. That is what cost two near-losses of working
110 state above.
111 - [ ] Add a `make base-check REF=<ref>` target wrapping
112 `git worktree add --detach` + test + `worktree remove`, so the safe way to
113 inspect a baseline is one command. A note alone demonstrably did not hold.
114 - [ ] Add the boundary-member checklist (empty, single, first/root,
115 duplicate/already-seen, freed-then-reused) to the plan-writing routine, and
116 require plans to state invariants beside steps.
117
118 ### Retained debt
119
120 - **Lease owner, next lease change:** fencing tokens are stored and reported
121 but nothing verifies them — there is no server-mediated write to fence until
122 phase 3. When patch revisions start flowing through the server, every
123 holder-authored write must carry its token and stale ones must be refused.
124 - **Web UI owner, next issue-page change:** an ungoverned server shows the raw
125 key fingerprint (`key:SHA256:…`) as the claim holder. Correct, unreadable.
126 Governance already supplies a person's name; ungoverned has nothing better
127 without a local alias map.
128 - **Lease owner, when a repo is deleted:** lease rows are keyed by repo path
129 and never reclaimed, so deleting a repository leaves its rows behind. Harmless
130 at any plausible size (one row per ever-claimed issue) and deliberate — the
131 rows carry the fencing high-water mark — but it is unbounded growth with no
132 sweeper.
133 - **Whoever tracks this file:** `RETRO.md` is still untracked in git. An
134 untracked retro cannot be reviewed, referenced from a commit, or read by
135 anyone else, which is part of why its lessons did not propagate.