| 1 | # Review — DDD and organisation, 2026-09-10 |
| 2 | |
| 3 | Read-only review of `src/` (74,442 lines, 157 files) against `plans/architecture.md`, the |
| 4 | eight ADRs and `CLAUDE.md`, measured against |
| 5 | [the 2026-09-06 review](review-2026-09-06-organisation.md). No code was changed. |
| 6 | `cargo test` — **1,298 passed, 0 failed**; `cargo clippy --all-targets` — zero warnings. |
| 7 | |
| 8 | ## Verdict |
| 9 | |
| 10 | **Better, and by the measure that matters most.** Seven of the first review's thirteen |
| 11 | steps landed (1, 2, 3, 5, 6, 7, 8) and each did what it promised: the biggest test blocks |
| 12 | moved to `#[path]` siblings, `infrastructure/git.rs` became a directory with both |
| 13 | `GitQuery` implementations side by side, and `application/access.rs` now holds every |
| 14 | authorization predicate the layer has — so `too_many_arguments` fell 58 → **39** and the |
| 15 | compiler, not a convention, keeps the `orgs`/`memberships`/`repos` triple together. |
| 16 | Meanwhile the tree grew 8,900 lines (pull requests, the whole ops step, CSRF) *without* |
| 17 | losing a rule: domain still imports nothing outward, **all 38 POST handlers still go |
| 18 | through a use case taking an `Actor`**, `web/` still touches no repository port, and every |
| 19 | one of the 25 ports — including the six the ops step added — has both implementations. |
| 20 | CSRF is the standout: a hand-written check in thirty-one handlers made safe by a test that |
| 21 | reads the source. What is worse is narrower and mostly *new drift of the shape the refactor |
| 22 | just fixed elsewhere*: `visible_repo` is a third copy of the visibility rule sitting outside |
| 23 | `access.rs`; `commit_url` and `compare_url` each exist twice, in page modules, three days |
| 24 | after `web/urls.rs` was created to end exactly that; `web/repo.rs` is still imported by |
| 25 | thirteen modules; and `web/pulls.rs` grew to 1,975 lines with 7 tests. The one *new* gap |
| 26 | rather than a leftover is that the composition root and three CLI commands live in |
| 27 | `main.rs`, a binary with zero tests — which is also the single thing standing between the |
| 28 | project and the HTTP-level harness every review has now asked for twice. |
| 29 | |
| 30 | ## 1 · Did the first review's refactors deliver? |
| 31 | |
| 32 | |
| 33 | |
| 34 | | 1 | test blocks to siblings | **yes** — `repo.rs` 3,127→674, `git_query.rs` 4,958→1,297, `sqlite.rs` 4,158→1,791 | `web/pulls.rs` (1,975) and `repository/in_memory.rs` (1,797) are past the 1,500 threshold with tests inline | |
| 35 | | 2 | one guard, one mapping | **yes** — `context::owned_repo`, `owned_profile`, `write_failure`; 8 guards and 4 mappings gone | see below | |
| 36 | | 3 | `web/format.rs`, `web/urls.rs` | **partly** — `browse.rs` 1,654→1,343, importers 16→7, three `fn optional` folded to one | see below | |
| 37 | | 5 | `access.rs` + `Stores` | **yes, and better than proposed** — 18 modules import it; 56 signatures lost 3 params | `visible_repo` stayed behind | |
| 38 | | 6 | PR numbering off `IssueRepository` | **yes** — `RepoRepository::next_number`; `merge_pull` keeps `issues` correctly (`Closes #n` is an issue write) | none | |
| 39 | | 7 | split `infrastructure/git.rs` | **yes** — `git/{storage,protocol,archive,import,merge}.rs`, `InMemoryGitQuery` beside `DiskGitQuery` | the compatibility re-export (below) | |
| 40 | | 8 | split the `git_query.rs` parsers | **yes** — `parse_{refs,log,tree,diff}.rs`, each with the `--format` const it reads | none | |
| 41 | |
| 42 | Four leftovers the brief named, checked: |
| 43 | |
| 44 | - **The `InMemoryGitQuery` re-export is real and load-bearing.** `git/mod.rs:33` re-exports |
| 45 | it from `git_query` so ~a dozen test modules keep compiling; **9 sites still import it via |
| 46 | `git::`, 1 via `git_query::`**. Harmless, but it means the move step 7 made is not visible |
| 47 | at any call site — a two-minute `sed` finishes it. |
| 48 | - **`labels::encode` survived.** `labels.rs:137` is byte-for-byte `urls::encode(v, false)` |
| 49 | (`urls.rs:22`). They agree, so this is duplication, not a bug — `issues.rs:139` and |
| 50 | `pulls.rs:202` encode the same `?label=` filter through different functions and produce |
| 51 | the same string. Delete one. |
| 52 | - **`context.rs` still imports two page modules.** `owned_repo` calls |
| 53 | `super::repo::repo_for` (`:347`) and `owned_profile` calls `super::profile::profile_for` |
| 54 | (`:362`). The base module every other page depends on now depends upward on two pages. |
| 55 | This is step 4's job and is the strongest remaining argument for it. |
| 56 | - **`pulls.rs`'s own `write_failure` (`:344`) is correct, not half-done.** Three genuine |
| 57 | arms (archived, unavailable, stale merge) then `super::context::write_failure(other)` — |
| 58 | the documented shape. It duplicates only the `Validation` arm; delegating that too would |
| 59 | make it three arms and a call. |
| 60 | |
| 61 | ## 2 · What regressed or grew back |
| 62 | |
| 63 | - **A third copy of the visibility rule.** `repo::visible_repo` (`repo.rs:200`) repeats the |
| 64 | org-lookup / repo-lookup / `is_org_member` body of `access::readable_repo` and |
| 65 | `access::changeable_repo` verbatim, with the arguments in a different order |
| 66 | (`handle, name, actor, stores` versus `actor, handle, name, stores`). **Seven application |
| 67 | modules import it** — archive, blame, browse, commit, feed, search, summary. The |
| 68 | module whose doc says "a use case asks this module; it does not read a `Role` … itself" has |
| 69 | a fourth resolver living outside it. This is the single item that most directly costs |
| 70 | milestone 7. |
| 71 | - **URL builders re-scattered, three days after `urls.rs`.** There are **two `commit_url`s** |
| 72 | (`commit.rs:934`, `&str`, encoded; `blame.rs:531`, `&ObjectId`, unencoded) and **two |
| 73 | `compare_url`s** (`commit.rs:942`, `refs.rs:67` — identical output, different parameter |
| 74 | types), each with its own tests. Seventeen more `*_url` functions live in the page that |
| 75 | happens to use them first (`issues.rs`, `labels.rs`, `pulls.rs`, `blame.rs`, `archive.rs`, |
| 76 | `refs.rs`, `search.rs`, `backup.rs`); `commit::commit_url` alone is imported by five other modules. Two of |
| 77 | their doc comments still say the page they link to does not exist yet. |
| 78 | - **`commit.rs` is becoming the new `browse.rs`.** It exports `diff_view`, `commit_log`'s |
| 79 | consumer set, and — the tell — `took_too_long` (5 modules) and `unwrap_page`, neither of |
| 80 | which is about commits. Both belong next to `context::timed_out`. |
| 81 | - **`web/repo.rs` is unchanged at 13 importers**, all for `Tab`/`repo_for`/`repo_header`. |
| 82 | Step 4 was not attempted and is now the largest single piece of shape debt. |
| 83 | - **`web/pulls.rs` grew**, 1,957 → 1,975, and still has 7 tests. It imports from **ten** |
| 84 | sibling web modules. |
| 85 | - **Counts, honestly:** `too_many_arguments` 58 → **39** (`merge_pull` 17 → 15 params); |
| 86 | `pub(crate)` in `infrastructure/` 19, in `application/` 9, in `domain/` **0**; |
| 87 | `browse.rs`'s `pub(super)` items 25 → 16. Nothing widened that should not have. |
| 88 | - **No write or authorization crept into a page.** The only `.insert(` calls in `web/` are a |
| 89 | `HashSet` and a header. Two authorization decisions do sit in the web layer, both |
| 90 | deliberate: `web/backup.rs:72` and `web/settings.rs:62` call `is_instance_owner` directly |
| 91 | (`back_up_for` re-checks, so it is a guard, not the authorization), and |
| 92 | `web/operator.rs:19` decides the whole operator surface with no `Actor` involved. |
| 93 | `architecture.md`'s claim that `access.rs` is "the one place that decides who may do what" |
| 94 | is now true with two footnotes; either give `context` an `instance_owner(cx)` guard and |
| 95 | document the operator token as a transport credential, or amend the doc. |
| 96 | |
| 97 | ## 3 · The new aggregates and ports |
| 98 | |
| 99 | **They are shaped like the rest, and the two-implementation rule held for every one.** |
| 100 | 25 ports: 13 in `domain/repository/` (password reset is the new one) and 12 in |
| 101 | `application/port.rs`. `DatabaseSnapshot`/`SqliteDatabaseSnapshot`+`InMemoryDatabaseSnapshot`, |
| 102 | `BackupArchive`/`TarGz…`+`InMemory…`, `Mailer`/`Lettre…`+`Recording…`, `Clock`/`System…`+ |
| 103 | `Fixed…` — every one has its twin, and `InMemoryPasswordResetRepo` is even documented as |
| 104 | appended past `mod tests` on purpose. Specifics: |
| 105 | |
| 106 | - **No use case reads the clock except `import_repo`/`run_import`** (`import.rs:100, 162`), |
| 107 | which is already recorded as the next `Clock` caller. All five other `SystemTime::now()` |
| 108 | in `application/` are inside test modules. `back_up` takes no `Actor` and `back_up_for` |
| 109 | does; `mint_reset_for_handle` follows `claim_instance`. Those distinctions are right. |
| 110 | - **`metrics` is a process-wide `&'static Arc<Metrics>`, the only global in the tree.** |
| 111 | Confined to `infrastructure/` — `grep` finds no reference from `application/` or `domain/` |
| 112 | — and put into app context so `/metrics` asks its `Cx` like any route. A deliberate, |
| 113 | documented exception; the cost is that no counter can be asserted from a use-case test. |
| 114 | - **The one port that hands git's wire format across the boundary is `GitQuery::diff`.** |
| 115 | It returns `RawDiff { numstat: Vec<u8>, patch: Vec<u8>, truncated }` — git's stdout — and |
| 116 | `application/diff.rs:165` (894 lines) parses it. Every other `GitQuery` method returns a |
| 117 | parsed type, and `infrastructure/git_query/parse_diff.rs` already holds `split_numstat`, |
| 118 | so the parsing of one format is split across the layer boundary. ADR 0006 says the binary's |
| 119 | invocation lives in one place; this is the exception. **One call site** |
| 120 | (`commit.rs:232`), so it is a contained fix. |
| 121 | - `sniff_licence` (`summary.rs:162`) is a pure classification rule with no ports, sitting in |
| 122 | a use-case module. Domain-shaped, low stakes. |
| 123 | |
| 124 | ## 4 · Milestone 7 readiness |
| 125 | |
| 126 | **The predicates are ready; the write surface is uniformly owner-only and the instance-level |
| 127 | concepts fail closed.** Concretely, with today's code: |
| 128 | |
| 129 | *What a second user can already do.* Nothing — there is no registration route, no invite, |
| 130 | and no `RegistrationPolicy` (`architecture.md` names one; `grep` finds zero). A second row in |
| 131 | `users` can only arrive by hand. So milestone 7 is a feature, not a repair. |
| 132 | |
| 133 | *What changes, by name.* Only these decide: |
| 134 | |
| 135 | |
| 136 | |
| 137 | | `access::writable_repo` — **14 call sites** in `issue.rs`, `label.rs`, `pull_request.rs` | owner only | the one that gains a permission argument: comment / open / label yes, merge no | |
| 138 | | `access::changeable_repo` — **12 sites** in `repo.rs`, `webhook.rs`, `import.rs` | owner only | stays owner-only; repository administration is not graded | |
| 139 | | `access::readable_repo` — 7 sites | public, or any member | unchanged | |
| 140 | | `is_org_owner` called directly — `repo.rs:63` (create), `push.rs:139`, `git.rs:179` (push), `import.rs:73`, `profile.rs:82` | owner only | `git.rs:179` is the one a collaborator needs; the rest stay | |
| 141 | | `repo::visible_repo` — 7 importers | duplicates `readable_repo` | **must be folded in first**, or the third role lands in three bodies | |
| 142 | |
| 143 | *What a `Role::Member` can do under today's code*: read a private repository of an org they |
| 144 | belong to, and nothing else — no comment, no issue, no label, no pull request, no push. |
| 145 | Nothing fails **open**. Three things fail **closed** the moment a second user exists, all |
| 146 | deliberately and all documented: |
| 147 | |
| 148 | - `is_instance_owner` (`access.rs:127`) → `false` for everybody, because `sole_user` returns |
| 149 | `None` for two rows (`sqlite.rs:101`, `limit 2` on purpose). The settings page's backup |
| 150 | section disappears and `/{handle}/settings/backup.tar.gz` 404s for **everyone**, owner |
| 151 | included. `steid backup` on the box still works, so it is recoverable — but this is the |
| 152 | first user-visible thing milestone 7 breaks, and it needs an instance-level role. |
| 153 | - `sole_owner_handle` → `None`, so `/` stops redirecting to the owner's profile and serves |
| 154 | the two-line generic landing (`pages.rs:38`). Safe, and ugly enough to need a real page. |
| 155 | - Backup content: an archive is every row and every private repository, which is why the |
| 156 | predicate is what it is. Do not loosen it to "any owner of any org". |
| 157 | |
| 158 | *What the schema needs*: `memberships` already carries `(org_id, user_id, role)` with a |
| 159 | unique pair, so a third `Role` variant is a value change, not a migration. There is no |
| 160 | instance-level role table and no per-repository collaborator table; whichever is chosen is |
| 161 | an ADR (it decides whether "collaborator" is membership of a personal org or a new relation). |
| 162 | |
| 163 | ## 5 · The remaining first-review steps, re-ranked |
| 164 | |
| 165 | - **4 (`web/frame.rs`) — still right, and now higher.** It also breaks `context.rs`'s |
| 166 | upward dependency on two page modules, which nothing else does. |
| 167 | - **9 (split `web/pulls.rs`) — still right, with a trap.** `csrf_scan::web_dir()` |
| 168 | (`csrf_scan.rs:25`) reads `src/infrastructure/web` **non-recursively**. Moving five POST |
| 169 | handlers into `web/pulls/` silently removes them from the CSRF scan; the |
| 170 | `handlers().len() >= 30` floor would not catch it (38 today → 33). **Make the scan walk |
| 171 | subdirectories before any web module becomes a directory** — which also gates step 13. |
| 172 | - **10 (`application/refs.rs`) — still right, unchanged.** `browse.rs` still holds |
| 173 | `list_refs`, `list_branches`, `list_tags`, `organise_refs`, `RefList`, `RefPage`. |
| 174 | - **11 (split `GitQuery`) — still right, still 24 methods and 26 `&impl GitQuery` |
| 175 | parameters, and now cheaper** because `Stores` removed the other three. Do the `RawDiff` |
| 176 | fix (§3) first; it removes one method's worth of the incoherence for free. |
| 177 | - **12 (`RepoEvent`) — right, and the pressure grew.** A merge is deliberately recorded as |
| 178 | a `PushEvent` (`pull_request.rs:863`), which is honest — the ref really moved — but |
| 179 | `Merged.closed` (the issues a merge closed) is returned to the caller and dropped, and |
| 180 | `feed::issue_feed` had to query issues directly rather than read events. Still needs an ADR. |
| 181 | - **13 (`/api`) — the gap widened.** 87 routes, **5 under `/api`**, two of which |
| 182 | (`/api/instance`, `/api/setup`) are operator plumbing rather than the read/write-line |
| 183 | check ROADMAP.md wants. Four waves became five. |
| 184 | |
| 185 | **Which precede milestone 7:** the `visible_repo` fold (below, step 1) and an |
| 186 | instance-owner decision (step 3). Nothing else does; steps 4, 9, 11, 12, 13 are independent |
| 187 | of it. |
| 188 | |
| 189 | ## 6 · Layer discipline and tests |
| 190 | |
| 191 | - **Domain imports nothing outward** — the one hit is a doc link (`domain/password.rs:79`). |
| 192 | - **`application/` → `infrastructure/` is three hits, all in tests** (`session.rs:73`, |
| 193 | `login.rs:150`, `browse_tests.rs:525`) plus one doc link. The third is new and is the only |
| 194 | real one: a use-case test imports the adapter constant `FOLLOWED_LOG_SCAN`, so an adapter |
| 195 | tuning change reddens an application test. Move the cap onto the port or into the fixture. |
| 196 | - **1,298 tests, all inline or in `#[path]` siblings; still no `tests/` directory and still |
| 197 | no test that builds a `Cx`.** `web/pulls.rs`: 1,887 lines of code, 7 tests. |
| 198 | `web/labels.rs`: 827 / 2. `web/issues.rs`: 1,093 / 3. |
| 199 | - **What the HTTP gap now costs.** It is no longer only about pages. CSRF is enforced by a |
| 200 | test that *reads source text* — it cannot see a handler that spells its route across two |
| 201 | lines, cannot see one in a subdirectory, and has never executed the 403 page it protects. |
| 202 | The security-headers layer, the 303-after-POST rule, the rate limiters' ordering |
| 203 | (`/metrics` gates after auth, `/auth/login` before), `__Host-` cookie attributes and the |
| 204 | operator surface's 404-not-401 are all HTTP-level facts asserted by hand in a browser. |
| 205 | - **What would close it, checked against the vendored crate rather than assumed:** |
| 206 | `topcoat::serve_until(listener, service, signal)` takes any `Listener`, and |
| 207 | `impl Listener for tokio::net::TcpListener` exists |
| 208 | (`topcoat-router-0.5.0/src/listener.rs:40`). So a `tests/` harness binding `127.0.0.1:0` |
| 209 | against a temp SQLite and temp data dir is buildable **today**. The only blocker is that |
| 210 | the app builder — fifteen `app_context` calls — is inline in `main.rs`, which is outside |
| 211 | the library. Extracting it is the whole prerequisite. |
| 212 | - **`main.rs` is 523 lines with 0 tests**, and holds `run_backup`, `run_restore` and |
| 213 | `print_reset_link` — the operator's recovery path, the least testable code in the tree. |
| 214 | `src/cli.rs` (in the library, 3 tests) parses the arguments; the commands themselves do not |
| 215 | live with it. |
| 216 | |
| 217 | ## Proposed reshaping |
| 218 | |
| 219 | Ordered. Every step compiles alone; none changes behaviour unless noted. **P** = safe in |
| 220 | parallel with other **P**s. **M7** = a milestone 7 prerequisite. |
| 221 | |
| 222 | |
| 223 | |
| 224 | | 1 **M7** | Fold `repo::visible_repo` into `access.rs` as the shared visibility resolver `readable_repo`/`changeable_repo` already need; unify argument order | `application/{repo,access}.rs` + 8 importers | small | low — mechanical, `access_tests.rs` already pins agreement | one body for the third role instead of three | |
| 225 | | 2 **P** | Extract the composition root: `infrastructure::web::builder(config, pool)`, `main` calls it; move `run_backup`/`run_restore`/`print_reset_link` into the library | `main.rs`, new `infrastructure/cli.rs` | small | low | **the HTTP harness and the first CLI test** | |
| 226 | | 3 **M7** | Decide the instance-owner question: `context::instance_owner(cx)` beside `owned_repo`, and either move `operator_authorized` behind an `Actor::Operator` or amend `architecture.md`'s "one place" claim | `web/{context,backup,settings,operator}.rs`, `architecture.md` | small | low | `access.rs`'s claim becomes true again; names the thing M7 has to replace | |
| 227 | | 4 **P** | Finish step 3: every `*_url` into `web/urls.rs`, both `commit_url`s and both `compare_url`s collapsed, `labels::encode` deleted, `took_too_long`/`unwrap_page` into `context`; refresh the two stale "this page does not exist" docs | 14 `web/*.rs` | small | low — one behaviour question: `blame`'s unencoded sha | page modules stop being each other's utility library | |
| 228 | | 5 **P** | Make `csrf_scan` walk `web/` recursively and raise the floor to the real count | `web/csrf_scan.rs` | tiny | none | **gates steps 7 and 11** — without it, either silently drops handlers from the scan | |
| 229 | | 6 | `web/frame.rs`: `repo_for`, `Tab`, `repo_header`, sidebar, README, `clone_block`, `clone_url_for`, `split_fragment`; `web/new_repo.rs` takes the two forms; `web/repo.rs` keeps its pages | 14 `web/*.rs` | half a day | low | the 13-importer hotspot stops being a page, and `context.rs` stops importing upward | |
| 230 | | 7 | Split `web/pulls.rs` into `pulls/{list,open,detail,writes}.rs` — **after 5** | `web/pulls.rs` → 4 | half a day | low | the largest untested web module gets seams; obeys the 1,500-line rule | |
| 231 | | 8 **P** | `GitQuery::diff` returns `Diff`; `application/diff.rs`'s parser moves beside `split_numstat` in `git_query/parse_diff.rs` | `port.rs`, 2 impls, `commit.rs:232` | small | low | git's wire format stops crossing the boundary; makes 11 cleaner | |
| 232 | | 9 **P** | `application/refs.rs` out of `browse.rs` (was step 10); finish the `InMemoryGitQuery` import sweep and drop the compatibility re-export | `application/browse.rs`, `web/{refs,search}.rs`, ~10 test modules | small | none | module names say what they hold | |
| 233 | | 10 | The HTTP harness: `tests/` booting `serve_until` on port 0 against a temp DB — first assertions on the CSRF 403 page, the 303-after-POST, `__Host-` cookie attributes, the security headers on a 404, and `/metrics` 404-not-401 — **after 2** | new `tests/` | a day | low | the gap both reviews have now named; makes 7's seams worth having | |
| 234 | | 11 | Split `GitQuery` into `GitRefs`/`GitObjects`/`GitHistory`/`GitSearch`; `merge_preview` onto `GitMerge` — **after 8**, **ADR amendment to 0006** | `port.rs`, 2 impls, 26 sites | a day | medium | narrow ports as 0006 intended | |
| 235 | | 12 | `RepoEvent` append-only log; `PushEvent` one variant; `Merged.closed` and issue/comment events become rows — **needs an ADR** | `domain/`, both adapters, `application/{webhook,feed,push,issue,pull_request}.rs` | a day | high — schema and payload | issue/PR webhooks, a real activity feed, notifications | |
| 236 | | 13 | `/api` for issues, pulls, labels, browse — **after 5** | `web/api.rs` → `web/api/` | a day+ | low | the standing practice restarts and the read/write line gets its third consumer | |
| 237 | |
| 238 | **Hotspots.** 6, 7, 11 and 12 each touch files many others import; run them one at a time. |
| 239 | 1, 2, 4, 5, 8, 9 are disjoint. **Do 5 before 7 or 13** — it is ten minutes and it is the |
| 240 | difference between a refactor and a silent hole. |
| 241 | |
| 242 | **ADRs owed:** 11 (amendment to |
| 243 | [0006](decisions/0006-git-binary-behind-narrow-ports.md)), 12 (new — an event payload is |
| 244 | expensive to reverse), and milestone 7's collaborator model (is a collaborator a membership |
| 245 | of a personal org, or a new per-repository relation?). |
| 246 | |
| 247 | **Docs owed regardless:** `architecture.md`'s "from_str returns Result" section names |
| 248 | `RegistrationPolicy`, which does not exist; its Access section claims one place decides who |
| 249 | may do what, which `visible_repo`, `is_instance_owner` in two pages and `operator_authorized` |
| 250 | each qualify; and its test-file section says "six files follow it as of 2026-09-08" while |
| 251 | `web/pulls.rs` (1,975) and `repository/in_memory.rs` (1,797) are past the threshold and do |
| 252 | not. |