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

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_urls (commit.rs:934, &str, encoded; blame.rs:531, &ObjectId, unencoded) and two compare_urls (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:

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 Ps. M7 = a milestone 7 prerequisite.

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), 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.