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

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

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.

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