# Review — DDD and organisation, 2026-09-10

Read-only review of `src/` (74,442 lines, 157 files) against `plans/architecture.md`, the
eight ADRs and `CLAUDE.md`, measured against
[the 2026-09-06 review](review-2026-09-06-organisation.md). No code was changed.
`cargo test` — **1,298 passed, 0 failed**; `cargo clippy --all-targets` — zero warnings.

## Verdict

**Better, and by the measure that matters most.** Seven of the first review's thirteen
steps landed (1, 2, 3, 5, 6, 7, 8) and each did what it promised: the biggest test blocks
moved to `#[path]` siblings, `infrastructure/git.rs` became a directory with both
`GitQuery` implementations side by side, and `application/access.rs` now holds every
authorization predicate the layer has — so `too_many_arguments` fell 58 → **39** and the
compiler, not a convention, keeps the `orgs`/`memberships`/`repos` triple together.
Meanwhile the tree grew 8,900 lines (pull requests, the whole ops step, CSRF) *without*
losing a rule: domain still imports nothing outward, **all 38 POST handlers still go
through a use case taking an `Actor`**, `web/` still touches no repository port, and every
one of the 25 ports — including the six the ops step added — has both implementations.
CSRF is the standout: a hand-written check in thirty-one handlers made safe by a test that
reads the source. What is worse is narrower and mostly *new drift of the shape the refactor
just fixed elsewhere*: `visible_repo` is a third copy of the visibility rule sitting outside
`access.rs`; `commit_url` and `compare_url` each exist twice, in page modules, three days
after `web/urls.rs` was created to end exactly that; `web/repo.rs` is still imported by
thirteen modules; and `web/pulls.rs` grew to 1,975 lines with 7 tests. The one *new* gap
rather than a leftover is that the composition root and three CLI commands live in
`main.rs`, a binary with zero tests — which is also the single thing standing between the
project and the HTTP-level harness every review has now asked for twice.

## 1 · Did the first review's refactors deliver?

| Step | Promised | Delivered | Left over |
|---|---|---|---|
| 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 |
| 2 | one guard, one mapping | **yes** — `context::owned_repo`, `owned_profile`, `write_failure`; 8 guards and 4 mappings gone | see below |
| 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 |
| 5 | `access.rs` + `Stores` | **yes, and better than proposed** — 18 modules import it; 56 signatures lost 3 params | `visible_repo` stayed behind |
| 6 | PR numbering off `IssueRepository` | **yes** — `RepoRepository::next_number`; `merge_pull` keeps `issues` correctly (`Closes #n` is an issue write) | none |
| 7 | split `infrastructure/git.rs` | **yes** — `git/{storage,protocol,archive,import,merge}.rs`, `InMemoryGitQuery` beside `DiskGitQuery` | the compatibility re-export (below) |
| 8 | split the `git_query.rs` parsers | **yes** — `parse_{refs,log,tree,diff}.rs`, each with the `--format` const it reads | none |

Four leftovers the brief named, checked:

- **The `InMemoryGitQuery` re-export is real and load-bearing.** `git/mod.rs:33` re-exports
  it from `git_query` so ~a dozen test modules keep compiling; **9 sites still import it via
  `git::`, 1 via `git_query::`**. Harmless, but it means the move step 7 made is not visible
  at any call site — a two-minute `sed` finishes it.
- **`labels::encode` survived.** `labels.rs:137` is byte-for-byte `urls::encode(v, false)`
  (`urls.rs:22`). They agree, so this is duplication, not a bug — `issues.rs:139` and
  `pulls.rs:202` encode the same `?label=` filter through different functions and produce
  the same string. Delete one.
- **`context.rs` still imports two page modules.** `owned_repo` calls
  `super::repo::repo_for` (`:347`) and `owned_profile` calls `super::profile::profile_for`
  (`:362`). The base module every other page depends on now depends upward on two pages.
  This is step 4's job and is the strongest remaining argument for it.
- **`pulls.rs`'s own `write_failure` (`:344`) is correct, not half-done.** Three genuine
  arms (archived, unavailable, stale merge) then `super::context::write_failure(other)` —
  the documented shape. It duplicates only the `Validation` arm; delegating that too would
  make it three arms and a call.

## 2 · What regressed or grew back

- **A third copy of the visibility rule.** `repo::visible_repo` (`repo.rs:200`) repeats the
  org-lookup / repo-lookup / `is_org_member` body of `access::readable_repo` and
  `access::changeable_repo` verbatim, with the arguments in a different order
  (`handle, name, actor, stores` versus `actor, handle, name, stores`). **Seven application
  modules import it** — archive, blame, browse, commit, feed, search, summary. The
  module whose doc says "a use case asks this module; it does not read a `Role` … itself" has
  a fourth resolver living outside it. This is the single item that most directly costs
  milestone 7.
- **URL builders re-scattered, three days after `urls.rs`.** There are **two `commit_url`s**
  (`commit.rs:934`, `&str`, encoded; `blame.rs:531`, `&ObjectId`, unencoded) and **two
  `compare_url`s** (`commit.rs:942`, `refs.rs:67` — identical output, different parameter
  types), each with its own tests. Seventeen more `*_url` functions live in the page that
  happens to use them first (`issues.rs`, `labels.rs`, `pulls.rs`, `blame.rs`, `archive.rs`,
  `refs.rs`, `search.rs`, `backup.rs`); `commit::commit_url` alone is imported by five other modules. Two of
  their doc comments still say the page they link to does not exist yet.
- **`commit.rs` is becoming the new `browse.rs`.** It exports `diff_view`, `commit_log`'s
  consumer set, and — the tell — `took_too_long` (5 modules) and `unwrap_page`, neither of
  which is about commits. Both belong next to `context::timed_out`.
- **`web/repo.rs` is unchanged at 13 importers**, all for `Tab`/`repo_for`/`repo_header`.
  Step 4 was not attempted and is now the largest single piece of shape debt.
- **`web/pulls.rs` grew**, 1,957 → 1,975, and still has 7 tests. It imports from **ten**
  sibling web modules.
- **Counts, honestly:** `too_many_arguments` 58 → **39** (`merge_pull` 17 → 15 params);
  `pub(crate)` in `infrastructure/` 19, in `application/` 9, in `domain/` **0**;
  `browse.rs`'s `pub(super)` items 25 → 16. Nothing widened that should not have.
- **No write or authorization crept into a page.** The only `.insert(` calls in `web/` are a
  `HashSet` and a header. Two authorization decisions do sit in the web layer, both
  deliberate: `web/backup.rs:72` and `web/settings.rs:62` call `is_instance_owner` directly
  (`back_up_for` re-checks, so it is a guard, not the authorization), and
  `web/operator.rs:19` decides the whole operator surface with no `Actor` involved.
  `architecture.md`'s claim that `access.rs` is "the one place that decides who may do what"
  is now true with two footnotes; either give `context` an `instance_owner(cx)` guard and
  document the operator token as a transport credential, or amend the doc.

## 3 · The new aggregates and ports

**They are shaped like the rest, and the two-implementation rule held for every one.**
25 ports: 13 in `domain/repository/` (password reset is the new one) and 12 in
`application/port.rs`. `DatabaseSnapshot`/`SqliteDatabaseSnapshot`+`InMemoryDatabaseSnapshot`,
`BackupArchive`/`TarGz…`+`InMemory…`, `Mailer`/`Lettre…`+`Recording…`, `Clock`/`System…`+
`Fixed…` — every one has its twin, and `InMemoryPasswordResetRepo` is even documented as
appended past `mod tests` on purpose. Specifics:

- **No use case reads the clock except `import_repo`/`run_import`** (`import.rs:100, 162`),
  which is already recorded as the next `Clock` caller. All five other `SystemTime::now()`
  in `application/` are inside test modules. `back_up` takes no `Actor` and `back_up_for`
  does; `mint_reset_for_handle` follows `claim_instance`. Those distinctions are right.
- **`metrics` is a process-wide `&'static Arc<Metrics>`, the only global in the tree.**
  Confined to `infrastructure/` — `grep` finds no reference from `application/` or `domain/`
  — and put into app context so `/metrics` asks its `Cx` like any route. A deliberate,
  documented exception; the cost is that no counter can be asserted from a use-case test.
- **The one port that hands git's wire format across the boundary is `GitQuery::diff`.**
  It returns `RawDiff { numstat: Vec<u8>, patch: Vec<u8>, truncated }` — git's stdout — and
  `application/diff.rs:165` (894 lines) parses it. Every other `GitQuery` method returns a
  parsed type, and `infrastructure/git_query/parse_diff.rs` already holds `split_numstat`,
  so the parsing of one format is split across the layer boundary. ADR 0006 says the binary's
  invocation lives in one place; this is the exception. **One call site**
  (`commit.rs:232`), so it is a contained fix.
- `sniff_licence` (`summary.rs:162`) is a pure classification rule with no ports, sitting in
  a use-case module. Domain-shaped, low stakes.

## 4 · Milestone 7 readiness

**The predicates are ready; the write surface is uniformly owner-only and the instance-level
concepts fail closed.** Concretely, with today's code:

*What a second user can already do.* Nothing — there is no registration route, no invite,
and no `RegistrationPolicy` (`architecture.md` names one; `grep` finds zero). A second row in
`users` can only arrive by hand. So milestone 7 is a feature, not a repair.

*What changes, by name.* Only these decide:

| Site | Rule today | M7 |
|---|---|---|
| `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 |
| `access::changeable_repo` — **12 sites** in `repo.rs`, `webhook.rs`, `import.rs` | owner only | stays owner-only; repository administration is not graded |
| `access::readable_repo` — 7 sites | public, or any member | unchanged |
| `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 |
| `repo::visible_repo` — 7 importers | duplicates `readable_repo` | **must be folded in first**, or the third role lands in three bodies |

*What a `Role::Member` can do under today's code*: read a private repository of an org they
belong to, and nothing else — no comment, no issue, no label, no pull request, no push.
Nothing fails **open**. Three things fail **closed** the moment a second user exists, all
deliberately and all documented:

- `is_instance_owner` (`access.rs:127`) → `false` for everybody, because `sole_user` returns
  `None` for two rows (`sqlite.rs:101`, `limit 2` on purpose). The settings page's backup
  section disappears and `/{handle}/settings/backup.tar.gz` 404s for **everyone**, owner
  included. `steid backup` on the box still works, so it is recoverable — but this is the
  first user-visible thing milestone 7 breaks, and it needs an instance-level role.
- `sole_owner_handle` → `None`, so `/` stops redirecting to the owner's profile and serves
  the two-line generic landing (`pages.rs:38`). Safe, and ugly enough to need a real page.
- Backup content: an archive is every row and every private repository, which is why the
  predicate is what it is. Do not loosen it to "any owner of any org".

*What the schema needs*: `memberships` already carries `(org_id, user_id, role)` with a
unique pair, so a third `Role` variant is a value change, not a migration. There is no
instance-level role table and no per-repository collaborator table; whichever is chosen is
an ADR (it decides whether "collaborator" is membership of a personal org or a new relation).

## 5 · The remaining first-review steps, re-ranked

- **4 (`web/frame.rs`) — still right, and now higher.** It also breaks `context.rs`'s
  upward dependency on two page modules, which nothing else does.
- **9 (split `web/pulls.rs`) — still right, with a trap.** `csrf_scan::web_dir()`
  (`csrf_scan.rs:25`) reads `src/infrastructure/web` **non-recursively**. Moving five POST
  handlers into `web/pulls/` silently removes them from the CSRF scan; the
  `handlers().len() >= 30` floor would not catch it (38 today → 33). **Make the scan walk
  subdirectories before any web module becomes a directory** — which also gates step 13.
- **10 (`application/refs.rs`) — still right, unchanged.** `browse.rs` still holds
  `list_refs`, `list_branches`, `list_tags`, `organise_refs`, `RefList`, `RefPage`.
- **11 (split `GitQuery`) — still right, still 24 methods and 26 `&impl GitQuery`
  parameters, and now cheaper** because `Stores` removed the other three. Do the `RawDiff`
  fix (§3) first; it removes one method's worth of the incoherence for free.
- **12 (`RepoEvent`) — right, and the pressure grew.** A merge is deliberately recorded as
  a `PushEvent` (`pull_request.rs:863`), which is honest — the ref really moved — but
  `Merged.closed` (the issues a merge closed) is returned to the caller and dropped, and
  `feed::issue_feed` had to query issues directly rather than read events. Still needs an ADR.
- **13 (`/api`) — the gap widened.** 87 routes, **5 under `/api`**, two of which
  (`/api/instance`, `/api/setup`) are operator plumbing rather than the read/write-line
  check ROADMAP.md wants. Four waves became five.

**Which precede milestone 7:** the `visible_repo` fold (below, step 1) and an
instance-owner decision (step 3). Nothing else does; steps 4, 9, 11, 12, 13 are independent
of it.

## 6 · Layer discipline and tests

- **Domain imports nothing outward** — the one hit is a doc link (`domain/password.rs:79`).
- **`application/` → `infrastructure/` is three hits, all in tests** (`session.rs:73`,
  `login.rs:150`, `browse_tests.rs:525`) plus one doc link. The third is new and is the only
  real one: a use-case test imports the adapter constant `FOLLOWED_LOG_SCAN`, so an adapter
  tuning change reddens an application test. Move the cap onto the port or into the fixture.
- **1,298 tests, all inline or in `#[path]` siblings; still no `tests/` directory and still
  no test that builds a `Cx`.** `web/pulls.rs`: 1,887 lines of code, 7 tests.
  `web/labels.rs`: 827 / 2. `web/issues.rs`: 1,093 / 3.
- **What the HTTP gap now costs.** It is no longer only about pages. CSRF is enforced by a
  test that *reads source text* — it cannot see a handler that spells its route across two
  lines, cannot see one in a subdirectory, and has never executed the 403 page it protects.
  The security-headers layer, the 303-after-POST rule, the rate limiters' ordering
  (`/metrics` gates after auth, `/auth/login` before), `__Host-` cookie attributes and the
  operator surface's 404-not-401 are all HTTP-level facts asserted by hand in a browser.
- **What would close it, checked against the vendored crate rather than assumed:**
  `topcoat::serve_until(listener, service, signal)` takes any `Listener`, and
  `impl Listener for tokio::net::TcpListener` exists
  (`topcoat-router-0.5.0/src/listener.rs:40`). So a `tests/` harness binding `127.0.0.1:0`
  against a temp SQLite and temp data dir is buildable **today**. The only blocker is that
  the app builder — fifteen `app_context` calls — is inline in `main.rs`, which is outside
  the library. Extracting it is the whole prerequisite.
- **`main.rs` is 523 lines with 0 tests**, and holds `run_backup`, `run_restore` and
  `print_reset_link` — the operator's recovery path, the least testable code in the tree.
  `src/cli.rs` (in the library, 3 tests) parses the arguments; the commands themselves do not
  live with it.

## Proposed reshaping

Ordered. Every step compiles alone; none changes behaviour unless noted. **P** = safe in
parallel with other **P**s. **M7** = a milestone 7 prerequisite.

| # | Step | Files | Size | Risk | Unlocks |
|---|---|---|---|---|---|
| 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 |
| 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** |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |

**Hotspots.** 6, 7, 11 and 12 each touch files many others import; run them one at a time.
1, 2, 4, 5, 8, 9 are disjoint. **Do 5 before 7 or 13** — it is ten minutes and it is the
difference between a refactor and a silent hole.

**ADRs owed:** 11 (amendment to
[0006](decisions/0006-git-binary-behind-narrow-ports.md)), 12 (new — an event payload is
expensive to reverse), and milestone 7's collaborator model (is a collaborator a membership
of a personal org, or a new per-repository relation?).

**Docs owed regardless:** `architecture.md`'s "from_str returns Result" section names
`RegistrationPolicy`, which does not exist; its Access section claims one place decides who
may do what, which `visible_repo`, `is_instance_owner` in two pages and `operator_authorized`
each qualify; and its test-file section says "six files follow it as of 2026-09-08" while
`web/pulls.rs` (1,975) and `repository/in_memory.rs` (1,797) are past the threshold and do
not.