a73x

e0e6ae94

Record the green baseline and what it cost to get there

a73x   2026-09-06 08:53

Commit message
Record the green baseline and what it cost to get there
CLAUDE.md
Old New
@@ -113,16 +113,26 @@ Each of these exists because of a specific failure; the date is when it bit.
113 `make lint` (fmt + check + clippy) before a commit; `make ci` (lint + test + 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. 114 build) before delivery. `cargo clippy --all-targets` is expected to be silent.
115 115
116 **The baseline is not green, and was not before you arrived** (measured at 116 **The baseline is green as of 2026-09-06** — `cargo test` exits 0, 61 targets.
117 `702a1e6`, 2026-09-06): nine `trust::tests` fail because they call 117 It was red for the ten tests before that, so a red run is now news: it is
118 `load_trust_policy`, which reads the developer's real 118 yours. Do not go looking for a known-bad list.
119 `~/.config/git-collab/trusted-keys` rather than using the 119
120 `load_trust_policy_with_global` seam that exists for injecting one; and 120 A passing run still prints `FAILED refs/collab/… matches reject pattern` —
121 `a_sync_authenticates_to_the_server_once` fails with `matches reject pattern`. 121 that is a governance test asserting a push *is* refused, and greping the
122 Both are non-hermetic tests reading real user state. Until they are fixed, 122 output for `FAILED` will mislead you. Read `test result:` lines, and capture
123 `cargo test --no-fail-fast` is the only way to see the other targets, and a new 123 `$?` rather than trusting either.
124 failure has to be told apart from these ten by name. Fixing them is an open 124
125 action in `RETRO.md`. 125 The two things that had made it red are both **tests reading state they did
126 not own**, so:
127
128 - **A test never reads real user state.** `load_trust_policy` merges
129 `~/.config/git-collab/trusted-keys`, so nine tests asserted against whatever
130 keys the machine happened to trust. Use `load_trust_policy_with_global` with
131 an explicit path or `None`.
132 - **A test never depends on the ambient environment's shape.** Connection
133 sharing needs a control socket inside `sun_path`, so
134 `sync_ssh_connection_test` failed wherever `TMPDIR` was long (CI containers,
135 Delta worktrees). It pins a short `TMPDIR` now.
126 136
127 ## Reading this repo without burning context 137 ## Reading this repo without burning context
128 138
RETRO.md
Old New
@@ -99,18 +99,18 @@ judgement or process.
99 - [x] State the invariants that existed only in my head or in scattered code 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 100 comments (lease token monotonicity, the identical-reply rule, append-only
101 events, plain-git contributing). Evidence: `CLAUDE.md` Invariants section. 101 events, plain-git contributing). Evidence: `CLAUDE.md` Invariants section.
102 - [ ] **Make `cargo test` green so the gate stops being decorative.** Nine 102 - [x] **Make `cargo test` green so the gate stops being decorative.** Done
103 `trust::tests` read the developer's real `~/.config/git-collab/trusted-keys` 103 2026-09-06 (`7a72f5a`, `c461e69`): `cargo test` exits 0 across 61 targets.
104 instead of using the existing `load_trust_policy_with_global` seam; 104 Nine `trust::tests` now load through the `load_trust_policy_with_global`
105 `a_sync_authenticates_to_the_server_once` fails on refs/collab pushes with 105 seam. The sync failure's cause was **not** what this action claimed — see
106 `matches reject pattern`. Both confirmed failing at base `702a1e6`, so 106 the next section; I had attributed it to a `matches reject pattern` line
107 neither is new. Six of this file's entries end with "a test caught it" — 107 that a *passing* governance test prints, without verifying it. It was a
108 tests are the primary defence here, and a permanently-red baseline means new 108 control-socket path overflow, and a real product bug.
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 109 - [ ] Add a `make base-check REF=<ref>` target wrapping
112 `git worktree add --detach` + test + `worktree remove`, so the safe way to 110 `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. 111 inspect a baseline is one command. A note alone demonstrably did not hold.
112 (Carried forward: still open, and cheaper now that a green baseline makes
113 "is this mine?" answerable without one.)
114 - [x] Add the boundary-member checklist (empty, single, first/root, 114 - [x] Add the boundary-member checklist (empty, single, first/root,
115 duplicate/already-seen, freed-then-reused) to the plan-writing routine, and 115 duplicate/already-seen, freed-then-reused) to the plan-writing routine, and
116 require plans to state invariants beside steps. Evidence: the "Plans state 116 require plans to state invariants beside steps. Evidence: the "Plans state
@@ -136,3 +136,87 @@ judgement or process.
136 unexercised — no slice has been delivered under it, and its independent-review 136 unexercised — no slice has been delivered under it, and its independent-review
137 step degrades to a disclosed self-review while subagents are unavailable here. 137 step degrades to a disclosed self-review while subagents are unavailable here.
138 The first real use is also its test. 138 The first real use is also its test.
139
140 ## A green baseline — 2026-09-06
141
142 Delivered `cargo test` exiting 0 across 61 targets, from ten failures that
143 predated this work. Both causes were tests reading state they did not own, but
144 one of them was hiding a real product bug. Run under the newly adopted
145 sprint-delivery workflow; its review step was a disclosed self-review, because
146 subagents were unavailable.
147
148 ### What worked
149
150 - **Diagnosing before believing my own notes.** The open action asserted the
151 sync test failed with `matches reject pattern`. It does not. That string is
152 printed by a *passing* governance test that asserts a push is refused, and I
153 had grepped a `--no-fail-fast` run for `FAILED` and attributed another test's
154 expected output to this one. Reading the actual failure took one command and
155 found something entirely different.
156 - Proving the arithmetic in the shell before editing the guard: the directory
157 is 59 bytes, the old budget computed exactly 100 against a limit of 100, and
158 the path ssh actually opened was 117.
159 - Fixing nine call sites through one test helper rather than nine edits, so the
160 hermetic path is the easy one, plus a warning at the definition site where
161 someone reaching for the wrong function will read it.
162 - The self-review pass earned its place: it caught that a stricter budget
163 silently *removes* working sharing in a narrow band, which I would not have
164 thought to measure otherwise.
165
166 ### Lessons
167
168 - **`git-collab sync` was broken outright under a long `TMPDIR`, on any
169 platform.** `ssh_share::socket_dir` budgeted for the 40-character `%C` hash
170 but not for the `.XXXXXXXXXXXXXXXX` that ssh appends while bringing the
171 master up and renames away afterwards — 17 bytes it never accounted for. A
172 directory of 51–59 bytes therefore passed the guard and handed ssh a path it
173 refused, so the *fetch* died with `unix_listener: path "…" too long` and
174 exit 128. Not a degraded optimisation: no sync at all. The guard's author had
175 anticipated exactly this failure and written the check for it; the check was
176 just short by the one component nobody sees, because it exists only between
177 `bind` and `rename`.
178 - **A test that reads what it does not own passes or fails by accident.** Nine
179 trust tests read the developer's real `~/.config/git-collab/trusted-keys`
180 through a convenience wrapper, and the sync test inherited whatever `TMPDIR`
181 the environment handed it. Both had injection seams available already
182 (`load_trust_policy_with_global`, and `TMPDIR` on the child process); neither
183 used them.
184 - A red baseline cost more than the ten tests. It is what made "is this mine?"
185 expensive enough that I twice risked the working tree to answer it, and it is
186 why a genuine product bug sat behind a line item that said "non-hermetic
187 test".
188
189 ### What to change
190
191 - When a retrospective action states a *cause*, it states evidence for it or
192 says the cause is unconfirmed. This one asserted a cause from a misread grep,
193 and the assertion survived into `CLAUDE.md` before being checked.
194 - Grep `test result:` and capture `$?`; never grep a test run for `FAILED`.
195 Expected-failure output exists and it says `FAILED` too. Now in `CLAUDE.md`.
196
197 ### Actions for the next slice
198
199 - [ ] Add the `make base-check REF=<ref>` target (carried forward from the
200 lease slice, still open, and now cheap).
201 - [ ] Decide whether to spend the control-socket safety margin. See the debt
202 below: the fix is correct but costs sharing in a narrow band, and recovering
203 it means raising `SOCKET_PATH_LIMIT` from 100 toward the real 104/108. That
204 is someone else's deliberate margin, so it wants a decision, not a patch.
205
206 ### Retained debt
207
208 - **Sync owner, next `ssh_share` change:** the corrected budget declines to
209 share for temp directories of 43–50 bytes (Linux) or 43–46 (macOS) that
210 previously shared successfully — the band between the conservative
211 `SOCKET_PATH_LIMIT` of 100 and the real `sun_path` limits of 108 and 104.
212 Correctness over an optimisation, and consistent with the constant's existing
213 4-byte slack, but it is coverage spent. Raising the limit to the portable
214 minimum of 104 would recover most of it.
215 - **Sync owner, when a sync is slow for no visible reason:** declining to share
216 is silent. There is no way to tell a one-login sync from a two-login one
217 without reading the server log, so the band above is invisible in use.
218 - **Trust owner, next `trust.rs` change:** `load_trust_policy` — the wrapper
219 that merges the global file — now has no test, because testing it requires
220 reading real user state. Its two halves are covered
221 (`load_trust_policy_merges_global_and_repo_keys`,
222 `global_trusted_keys_path_returns_config_path`); the composition is not.