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

Read-only review of `src/` (65,541 lines) against `plans/architecture.md`, the seven
ADRs, and the conventions in `CLAUDE.md`. No code was changed by this review.

## Verdict

The layer rules survived four waves of parallel agents essentially intact: `domain/`
imports nothing from `application/` or `infrastructure/` anywhere, `application/`
reaches into `infrastructure/` only inside three `mod tests` blocks, and **not one of
the 34 POST handlers writes through a repository port** — every mutation goes through a
use case that takes an `Actor`. What did not survive is *shape at the seams*. The
application layer's "use cases take `&impl Port`" convention has hit its ceiling —
`merge_pull` and `redeliver` take 17 parameters each and there are **58
`#[allow(clippy::too_many_arguments)]` in the tree**; `GitQuery` has grown to 24
methods across a 450-line trait; `web/repo.rs` and `web/browse.rs` have become the
grab-bags twelve and thirteen other page modules import from; and four files are over
2,000 lines mostly because their `mod tests` block is two to three times the code it
tests. None of this is rot — it is the predictable cost of many hands adding to the same
files without anyone owning the boundary between them. Everything below is
addressable in small, independently mergeable steps.

## 1 · Layer discipline

**Clean, and verifiably so.**

- `grep -rn "crate::application\|crate::infrastructure" src/domain/` → **zero hits**.
  Domain's only external crates are `uuid`, `sha2`, `subtle`, `rand`, `hmac` — all
  value-object machinery (`domain/id.rs`, `domain/token.rs`, `domain/webhook.rs`
  signatures). No `chrono`, no `sqlx`, no `topcoat`.
- `application/` → `infrastructure/` : three hits, **all inside `mod tests`** —
  `session.rs:73` (tests start 71), `login.rs:146` (46), `browse.rs:1381` (837).
- `infrastructure/web/` never imports `domain::repository` and performs no repository
  writes. The only `.insert(` calls are a `HashSet` (`import.rs:102`) and a response
  header (`rate_limit.rs:307`).

**Authorization in pages.** No page *decides* authorization; every page reads
`RepoView::viewer_is_owner`, which `view_repo` computes via
`authz::is_org_owner` (`application/repo.rs:265`). What pages do carry is a
*duplicated guard*: an identical `if !repo.viewer_is_owner { return not_found() }`
helper is written out five times — `issues.rs:227`, `labels.rs:149`, `pulls.rs:310`,
`repo_settings.rs:96`, `webhook.rs:97` (named `owned_repo` in three, `owned` in two) —
plus three more inline copies in `repo.rs:108`, `settings.rs:55`, `token.rs:58`. Each
carries a comment explaining the same 404-not-403 rule. The use cases re-check, so this
is duplication, not a hole. It belongs in `web::context` next to `current_actor`.

## 2 · Aggregates and ports

| Aggregate | Domain | Port | In-memory | SQLite |
|---|---|---|---|---|
| User | `domain/user.rs` | `user_repo.rs` (5) | `in_memory.rs:28` | `sqlite.rs:30` |
| Org | `domain/org.rs` | `org_repo.rs` (3) | `:71` | `:115` |
| Membership | `domain/membership.rs` | `membership_repo.rs` (3) | `:100` | `:177` |
| Session | `domain/session.rs` | `session_repo.rs` (4) | `:1589` | `:244` |
| Repository | `domain/repo.rs` | `repo_repo.rs` (7) | `:1626` | `:323` |
| Token | `domain/token.rs` | `token_repo.rs` (5) | `:137` | `:488` |
| Issue | `domain/issue.rs` | `issue_repo.rs` (8) | `:209` | `:593` |
| Pull request | `domain/pull_request.rs` | `pull_request_repo.rs` (7) | `:779` | `:1419` |
| Label | `domain/label.rs` | `label_repo.rs` (13) | `:537` | `:1126` |
| Push event | `domain/push_event.rs` | `push_event_repo.rs` (4) | `:347` | `:827` |
| Webhook | `domain/webhook.rs` | `webhook_repo.rs` (4) | `:420` | `:943` |
| Delivery | `domain/webhook_delivery.rs` | `webhook_delivery_repo.rs` (4) | `:470` | `:1019` |

Twelve aggregates, twelve traits, twenty-four implementations. **The two-implementation
rule held everywhere.** Setup token and actor are value objects with no port, correctly.

**`GitQuery` is the incoherent one.** `application/port.rs:457–906` — 24 methods,
450 lines, spanning five unrelated families: refs (`default_branch`, `resolve`,
`list_refs`, `branches`, `tags`, `latest_tag`, `ref_snapshot`), objects (`list_tree`,
`read_blob`, `symlink_targets`), history (`log`, `log_entries`, `log_at_path`,
`count_commits`, `count_commits_at_path`, `last_commits`, `commit`, `log_between`,
`count_between`, `merge_base`), diff (`diff`), and search (`grep`, `blame`,
`merge_preview`). A four-way split — `GitRefs`, `GitObjects`, `GitHistory`,
`GitSearch` — costs updating two implementors (`DiskGitQuery`, `InMemoryGitQuery`) and
**26 `queries: &impl GitQuery` parameters** across `application/`, with no behaviour
change. It is worth doing when a *third* implementor appears, not before; the cheaper
and more useful half is that `merge_preview` does not belong on a read port at all
(there is already a `GitMerge` port at `port.rs:1192`), and `blame` and `grep` are the
two methods that can time out, which is a different contract.

**Where issues and pull requests duplicate a rule, they mostly already share it.**
`PullRequest` reuses `IssueNumber` and `domain::issue::validate_content`
(`domain/pull_request.rs:21`) — good. But **PR numbering reaches into the wrong port**:
`create_pull` takes `issues: &impl IssueRepository` solely to call
`issues.next_number(&repo.id)` (`application/pull_request.rs:583, 605`), because the
sequence is per-repository and shared. That is a missing shared concept, not fine as is
— the `repo_sequences` table wants its own narrow port (`NumberRepository::next` or a
method on `RepoRepository`), which also removes one port from four PR use cases.

Similarly, `readable_repo` and `writable_repo` — the resolution every issue, label,
pull and webhook use case starts with — live in `application/issue.rs:483, 511` and are
imported by `label.rs:29` and `pull_request.rs:39`. Correct behaviour, wrong home.

## 3 · File size and cohesion

| # | File | Total | Code | `mod tests` |
|---|---|---|---|---|
| 1 | `infrastructure/git_query.rs` | 4,956 | 2,056 | 2,900 |
| 2 | `infrastructure/repository/sqlite.rs` | 3,755 | 1,646 | 2,109 |
| 3 | `infrastructure/git.rs` | 3,586 | 2,293 | 1,293 |
| 4 | `application/repo.rs` | 3,119 | 737 | 2,382 |
| 5 | `application/pull_request.rs` | 2,755 | 1,204 | 1,551 |
| 6 | `infrastructure/web/pulls.rs` | 1,957 | 1,870 | 87 |
| 7 | `application/browse.rs` | 1,917 | 836 | 1,081 |
| 8 | `infrastructure/repository/in_memory.rs` | 1,705 | 905 | 800 |
| 9 | `infrastructure/web/browse.rs` | 1,654 | 1,407 | 247 |
| 10 | `application/webhook.rs` | 1,505 | 643 | 862 |

**Four of the ten are large only because of their tests.** `application/repo.rs` is
737 lines of eight coherent use cases behind a 2,382-line test module. Moving the block
to a sibling with `#[cfg(test)] #[path = "repo_tests.rs"] mod tests;` is mechanical,
zero-risk, and takes ~9,000 lines out of the four biggest application/adapter files
without touching a line of logic.

Genuinely several things:

- **`git_query.rs`** — one 935-line `impl GitQuery for DiskGitQuery` (`:127–1062`)
  followed by ~1,000 lines of free parsers. Split: `git_query/mod.rs` (the impl),
  `git_query/parse_refs.rs` (`parse_refs`, `parse_branches`, `parse_tags`,
  `parse_latest_tag`, `parse_ref_snapshot`, `ref_fields`, `short_ref`, the five
  `*_FORMAT` consts), `git_query/parse_log.rs` (`parse_log`, `parse_log_entries`,
  `parse_commit_object`, `split_headers`, `split_message`, `identity`),
  `git_query/parse_tree.rs` (`parse_tree_object`, `attribute`, `record_at`,
  `tree_spec`), `git_query/parse_diff.rs` (`split_numstat`, `parse_merge_tree`).
- **`git.rs`** — six ports' adapters in one file: `DiskGitStorage`,
  `InMemoryGitStorage`, `GitHttpBackend`, `InMemoryGitProtocol`, `DiskGitArchive`,
  `InMemoryGitArchive`, **`InMemoryGitQuery` (`:982–1588`, 600 lines)**,
  `DiskGitImport`, `InMemoryGitImport`, `GitFeatures`, `DiskGitMerge`,
  `InMemoryGitMerge`. The worst of it is that `InMemoryGitQuery` lives here while
  `DiskGitQuery` lives in `git_query.rs` — the two implementations of one port are in
  different files, which is exactly what the two-implementation convention exists to
  make easy to compare. Split: `git/storage.rs`, `git/protocol.rs`, `git/archive.rs`,
  `git/import.rs`, `git/merge.rs`, and move `InMemoryGitQuery` beside `DiskGitQuery`.
- **`web/pulls.rs`** — 1,870 lines of code, five POST handlers, four page routes, and
  its own `Found<T>`/`elsewhere_or_missing` resolution machinery (`:219, :261`). Split
  along the section comments already in the file: `pulls/list.rs`, `pulls/open.rs`
  (the new-PR flow), `pulls/detail.rs` (conversation/commits/files tabs),
  `pulls/writes.rs`.
- **`web/browse.rs`** — 1,407 lines of code holding four things: the tree/blob/log
  pages, the revision switcher, **the URL builders** (`tree_url`, `raw_url`,
  `log_url`, `path_log_url`, `encode`), and **the formatters** (`ago`, `timestamp`,
  `size_of`, `civil_from_days`). See §4.
- **`application/browse.rs`** — `browse_repo`, `repo_log`, `repo_path_log`,
  `list_refs`, `list_branches`, `list_tags`, `read_raw_file`, `parse_gitmodules`,
  `resolve_relative`. Three concerns: browsing a tree, walking a log, listing refs.

`sqlite.rs` and `in_memory.rs` are long but cleanly sectioned one aggregate at a time;
they only want splitting once the test blocks move out and the remainder is still large.
One placement slip: `InMemorySessionRepo` and `InMemoryRepoRepo` were appended *after*
`mod tests` (`in_memory.rs:1589, 1626`; tests at `:906`) — the `CLAUDE.md` gotcha,
caught by the compiler rather than by review, but it means those two are the only
in-memory adapters with no tests beside them.

## 4 · Naming and placement drift

- **`web/repo.rs` is not "the repo page".** It is `repo_for` (the resolver every repo
  page calls), `Tab`, `repo_header`, the About sidebar, the README block,
  `clone_block`, `new_repo_form`, `split_fragment`, `clone_url_for` — *and* the
  landing page (`:204`) and the new-repo form pages (`:102, :134`). **Thirteen** other
  web modules import `repo::{Tab, repo_for, repo_header}`. The frame is a shared
  module (`web/frame.rs`); the landing page and the new-repo pages are two pages.
- **`web/browse.rs` is imported by thirteen modules** for things that are not
  browsing: `ago` (6 modules), `timestamp` (6), `encode` (6), `tree_url`,
  `log_url`, `civil_from_days`, `commit_log`. It carries **25 `pub(super)` items**,
  more than three times any other web module. `ago`/`timestamp`/`size_of`/
  `civil_from_days` are a `web/format.rs`; `encode`/`tree_url`/`raw_url`/`log_url`/
  `path_log_url` are a `web/urls.rs`.
- **`application/browse.rs` holds ref listing**, which `web/refs.rs` and
  `web/search.rs` consume; `list_branches`/`list_tags`/`list_refs`/`organise_refs`/
  `RefList`/`RefPage` are an `application/refs.rs`.
- `fn optional(value: &str)` is written identically three times —
  `web/repo.rs:74`, `web/repo_settings.rs:87`, `web/settings.rs:47`.
- `context::timed_out` was correctly promoted out of `refs.rs` (progress.md predicted
  this) and now has four callers. That one worked.
- `architecture.md` says memoization "earns its keep" for direct reads. There are
  **zero `#[memoize]` in the tree**; pages thread the resolved `RepoView` through
  instead. The doc should say so rather than describing a mechanism nobody used.

## 5 · Test organisation

1,152 test functions, **all in 73 inline `#[cfg(test)] mod tests` blocks**; there is no
`tests/` directory and no route-level or HTTP-level test anywhere.

- **Use-case tests run on in-memory ports throughout** — no `Sqlite*` appears anywhere
  under `src/application/`. Exactly as intended.
- One adapter test reaches for a use case as a *fixture*: `sqlite.rs`'s tests import
  `application::{OwnerSpec, claim_instance}` (`:1653`). It is setup, not assertion, but
  it means a change to `claim_instance` can redden the SQLite suite.
- **The web layer is the thin spot.** `web/pulls.rs` — 1,870 lines of code — has 7
  tests; `web/issues.rs` (1,130) has 3; `web/labels.rs` (892) has 0. What lives there
  and is untested is precisely the logic §1 and §6 flag as duplicated and divergent:
  the three `write_failure` mappings and the five ownership guards. Moving them into
  `web::context` would also give them one place to be tested.
- Test-to-code ratios of 3.2:1 (`application/repo.rs`), 2.1:1 (`label.rs`) and 1.4:1
  (`git_query.rs`) are healthy; they are a file-size problem, not a testing problem.

## 6 · Duplication and inconsistency across waves

- **Error mapping diverges.** Three `write_failure` functions — `labels.rs:177`,
  `issues.rs:241`, `pulls.rs:321` — share a body, except that **labels maps
  `DomainError::Forbidden` to 404 while issues and pulls map it to 403**. Pulls adds
  `Archived`, `Unavailable` and stale-merge arms the other two lack. One
  `context::write_failure` returning `Result<String, _>`, with page-specific arms
  layered before the call, should win; the 403 is the right answer (the guard has
  already 404'd anything invisible).
- **Ownership guard**: five near-identical private helpers plus three inline copies
  (§1). One `context::owned_repo` should win.
- **Timeout-as-page** is consistent — `context::timed_out` in four modules, plus
  `search.rs:182` handling it inside the use case because search's answer is a partial
  result rather than an absent one. Both are right.
- **The private-address rule is unified**: one `domain::net::is_public`
  (`domain/net.rs:27`), used by `domain/webhook.rs:16` and `domain/import.rs:10`, with
  the escape hatch as a single `PrivateTargets` value object threaded from
  `web/context.rs:166`. This is the pattern the other items should copy.
- **Paging is unified**: `application::Paged<T>` for every list (repos, issues, pulls,
  commits, deliveries, refs) and one `web::paging::Steps` renderer. No drift.
- **The `Clock` port is adopted, `now: SystemTime` is the norm, and the two known
  exceptions are still there**: `import.rs:103` and `:164` read `SystemTime::now()` in
  production code. Already recorded as open in `current.md`; nothing new.
- **`too_many_arguments` is the wave signature.** 58 allows in the tree, 50 of them in
  `application/`. `merge_pull` and `redeliver` take 17 parameters; `view_pull`,
  `list_pulls` and `list_issues` take 12. **66 use cases take the same
  `orgs`/`memberships`/`repos` triple.**

## 7 · What the next features will need

- **`/api` has lapsed badly.** Three routes — `/api/me`, `/api/users/{handle}`,
  `/api/users/{handle}/repos` (`web/api.rs`, 124 lines) — against ~35 page routes and
  34 POST handlers. Issues, pull requests, labels, webhooks, browsing, commits and
  search have no API surface at all. ROADMAP.md calls this "a standing practice"; four
  waves shipped without it. It is also the stated tripwire for whether the read/write
  line is in the right place, so its absence has cost the check as well as the feature.
- **There is no event concept, only a push event.** `PushEvent` is the only thing
  recorded; `webhook::deliver_push`/`push_payload` deliver only pushes, and
  `feed::activity_feed` can only report pushes. Issues opened, PRs merged and comments
  posted produce nothing. Webhooks, the activity feed and any future notification all
  want the same shape — an append-only `RepoEvent { repo_id, kind, actor, subject,
  at }` with `PushEvent` as one variant. **This deserves an ADR**: the payload shape is
  expensive to change once anything consumes it.
- **The shared "repository facts" read already exists** — `application/summary.rs`
  (`RepoFacts`, 538 lines) — contrary to the brief's guess. What is missing is that
  `view_repo` is reached by thirteen modules and always pays two `count(*)` queries
  (`repo.rs:255`) plus a membership lookup, even on a raw-blob request that draws no
  tab strip. A `visible_repo`-only path for the non-frame routes is a small win.
- **The reconciliation sweep exists and covers the three known holes** —
  `application/reconcile.rs` (374 lines), documented against `architecture.md`'s
  DB-plus-filesystem section, running on boot beside the interrupted-import and reflog
  sweeps. `architecture.md`'s "neither prior attempt fully solved this / the durable
  fix is a reconciliation sweep on boot" is now out of date and should say so.
- **Milestone 7 pressure.** `Role::Owner` appears in 18 application modules, but only
  ever through `authz::is_org_owner`/`is_org_member` (46 lines, two predicates) — the
  single place multi-user has to change. That part is ready. What is not: every write
  use case is owner-only by construction (`writable_repo`), so "a collaborator may
  comment on an issue but not merge" needs a third predicate and a permission argument
  threaded through ~30 use cases that currently take none. The 17-parameter problem
  makes that materially harder, which is the argument for fixing it first.

## Proposed reshaping

Ordered. Every step compiles on its own and none changes behaviour unless noted.
**P** = safe to run in parallel with the others marked P.

| # | Refactor | Files | Size | Risk | Unlocks |
|---|---|---|---|---|---|
| 1 **P** | Move the four biggest `mod tests` blocks to `#[path]` siblings | `application/repo.rs`, `pull_request.rs`, `browse.rs`, `webhook.rs`, `infrastructure/git_query.rs`, `repository/sqlite.rs` | small | none | ~9,000 lines out of the top files; every later split reads a real file |
| 2 **P** | `context::owned_repo` + `context::write_failure`; delete the 8 guards and 3 mappings | 9 `web/*.rs` | small | low — settles labels' 404 to 403 | one tested answer; §5's untested logic becomes testable |
| 3 **P** | `web/format.rs` and `web/urls.rs`; move `ago`, `timestamp`, `size_of`, `civil_from_days`, `encode`, `*_url` out of `web/browse.rs`; fold the three `fn optional` into one | 13 `web/*.rs` | small | none | `web/browse.rs` stops being everyone's dependency |
| 4 | `web/frame.rs` — `repo_for`, `Tab`, `repo_header`, sidebar, README, `clone_block`, `clone_url_for`; `web/repo.rs` keeps the landing page, new-repo pages move to `web/new_repo.rs` | 14 `web/*.rs` | half a day | low | the hotspot 13 modules import stops being a page module |
| 5 | Move `readable_repo`/`writable_repo` to `application/access.rs`; give the `orgs`/`memberships`/`repos` triple one struct passed by reference | most of `application/`, all call sites | half a day | medium — wide, mechanical | −3 params on 66 use cases; makes step 8 and milestone 7 tractable |
| 6 **P** | `NumberRepository` (or `RepoRepository::next_number`) for `repo_sequences`; drop `IssueRepository` from the four PR use cases | `domain/repository/`, both adapters, `application/pull_request.rs` | small | low | issues and PRs stop reaching into each other's port |
| 7 **P** | Split `infrastructure/git.rs` into `git/{storage,protocol,archive,import,merge}.rs`; move `InMemoryGitQuery` next to `DiskGitQuery` | `infrastructure/git.rs` → 5 files | half a day | none | both implementations of a port sit together |
| 8 **P** | Split `git_query.rs`'s parsers into `git_query/parse_{refs,log,tree,diff}.rs` | `infrastructure/git_query.rs` | half a day | none | the 4,956-line hotspot becomes five readable files |
| 9 | Split `web/pulls.rs` along its existing section comments into `pulls/{list,open,detail,writes}.rs` | `web/pulls.rs` → 4 | half a day | low | the largest untested web module gets seams to test at |
| 10 | `application/refs.rs` out of `application/browse.rs`; `browse.rs` keeps tree/blob/log | `application/browse.rs`, `web/refs.rs`, `web/search.rs` | small | low | module names say what they hold |
| 11 | Split `GitQuery` into `GitRefs`/`GitObjects`/`GitHistory`/`GitSearch`; move `merge_preview` onto `GitMerge` | `port.rs`, 2 impls, 26 call sites | a day | medium | narrow ports as 0006 intended — **do after 5**, and **worth an ADR amendment to 0006** |
| 12 | `RepoEvent` append-only log; `PushEvent` becomes one variant; webhooks and the activity feed read it | `domain/`, both adapters, `application/{webhook,feed,push,issue,pull_request}.rs` | a day | high — schema and payload | issue/PR/comment webhooks, a real activity feed, notifications — **needs an ADR** |
| 13 | `/api` for issues, pulls, labels, repos-write, browse | `web/api.rs` → `web/api/` | a day+ | low | the standing practice restarts, and the read/write line gets its third consumer |

**Hotspots.** Steps 4, 5, 9 and 11 touch files many others import; run them one at a
time. Steps 1, 2, 3, 6, 7, 8 are disjoint and can go in any order or together. Steps
12 and 13 are features wearing refactor clothes and belong on the ladder, not in a
tidy-up.

**ADRs owed:** 11 (an amendment to
[0006](decisions/0006-git-binary-behind-narrow-ports.md), which claims narrow ports and
now has a 24-method one) and 12 (a new record — an event payload is expensive to
reverse once webhooks emit it). Step 5 does not need one; it is a call-convention change
inside a rule `architecture.md` already states.

**Docs owed regardless of any refactor:** `architecture.md`'s DB-plus-filesystem section
still says the reconciliation sweep is the durable fix nobody has written —
`application/reconcile.rs` has written it. And its "Data access" section credits
`#[memoize]` with earning its keep; nothing in the tree uses it.