GKaszewski/movies-diary
1
0
Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity

structural refactor and codebase improvements

Browse Source
This commit is contained in:
Gabriel Kaszewski
2026-08-09 14:58:14 +02:00
parent 22b1dd3f56
commit c9715baab8
247 changed files with 11515 additions and 3063 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
docs/adr/0001-general-review-editing.md Normal file
View File

@@ -0,0 +1,8 @@
# General review editing with partial updates
Reviews were insert-only by design — one watch, one immutable record. Adding the WatchMedium field (optional metadata for how a movie was watched) required an update path for backfilling existing entries. Rather than adding a narrow "set medium" operation, we chose general partial editing of all mutable fields (rating, comment, watched_at, watch_medium). This mirrors the update capability that already exists for inbound federated reviews, and avoids accumulating single-field setters as new optional fields are added over time. Local edits broadcast an AP `Update` activity to stay consistent with federation.
## Considered Options
- **Narrow setter per field** — rejected because it would need to be repeated for every future optional field, and the infrastructure cost (use case, repo method, API endpoint, UI) is identical to general edit.
- **Delete and re-create** — rejected because it changes the review ID, breaks AP references, and is a worse UX for correcting a typo.

117
docs/adr/0004-instance-identity.md Normal file
View File

@@ -0,0 +1,117 @@
# InstanceIdentity owns URL derivation; social queries decompose
ADR-0002 and ADR-0003 unified local and federated social operations, but left `base_url`
travelling as a bare `&str`: a parameter on `FollowQuery` methods, a field on
`CompositeSocialAdapter`, and an argument to `SocialIdentity::from_actor_url`,
`format_local_handle` and `host_from_base_url`, scattered across many call sites. With no
owner, each layer re-derived URLs its own way — and `handlers/social.rs` derived
`local:{uuid}`, which `from_actor_url` could not parse, so every POST echoing a local actor's
url back resolved to `Remote` and was routed to ActivityPub. Accepting a local follow request
through the SPA was broken.
## Decision
- `InstanceIdentity` (`crates/domain/src/value_objects/instance.rs`) is a domain value object
owning `actor_url_for`, `handle_for`, `image_url_for`, `actor_url_of`, `identify` and `host`.
`SocialIdentity`'s static URL helpers (`from_actor_url`, `format_local_handle`,
`host_from_base_url`) are deleted so the duplication cannot return.
- AP object URLs — review, goal and watchlist-entry URLs — deliberately stay out of
`InstanceIdentity` and live in the ActivityPub adapter's `urls.rs`
(`crates/adapters/activitypub/src/urls.rs`), which takes `&InstanceIdentity` and builds on
`instance.base_url()`/`instance.actor_url_for()` rather than re-deriving. A review, goal or
watchlist entry isn't an identity of this instance the way an actor URL or handle is — it's a
content object's location — so folding them into the value object would turn it into a general
URL grab-bag instead of an identity abstraction. `actor_url` is the one exception that
delegates straight to `instance.actor_url_for(...)`, so there is exactly one construction of
an actor URL in the codebase. The five ActivityPub adapter structs (`ActivityPubEventHandler`,
`GoalObjectHandler`, `ReviewObjectHandler`, `DomainUserRepoAdapter`, `WatchlistObjectHandler`)
and `handlers/users.rs`'s image-serving endpoints now hold/use `InstanceIdentity` instead of a
bare `base_url: String`, closing the gap the whole-branch review found: this ADR's scope claim
now holds for the entire codebase, not just the follow/social/profile-identity paths that
landed first.
- `handle_for` takes `&str`, not `&Username`. Database rows carry unvalidated
`Option<String>` usernames; requiring a `Username` would force revalidation of data that is
already stored and already trusted.
- Local actors are identified on the wire by their canonical AP actor URL,
`{base_url}/users/{uuid}` — what the database already stores. `identify(actor_url_of(id)) ==
id` is a tested domain property (`instance_actor_url_round_trips_both_variants` in
`crates/domain/src/tests/value_objects.rs`).
- Adapters hold an `InstanceIdentity` rather than receiving `base_url` per call; it is the same
value on every call. `CompositeSocialAdapter` and `GetProfileDeps` both carry one.
- `SocialQuery` splits into `FollowGraphQuery` and `BlockQuery`, along the seam
`CompositeSocialAdapter` already showed: `get_blocked` is the only method that reaches k_ap
(`ap_service.get_blocked_actors`); every `FollowGraphQuery` method — including the new
`get_relation` — goes through the local `follow_query` SQL port instead. `is_following ->
bool` becomes `get_relation -> FollowRelation`, carrying both directions
(`following`/`followed_by`) and distinguishing pending from accepted. `get_relation`
propagates sqlx decode errors as `DomainError::InfrastructureError` rather than folding them
into the same `None` that means "no follow edge" — the original snippet used
`.ok().flatten()`, which would have reported a broken query as a stranger.
- Social **queries** become one file per use case (`get_followers.rs`, `get_following.rs`,
`get_blocked.rs`, `get_pending_followers.rs` under `crates/application/src/social/`),
matching `diary/` and `users/` and making ADR-0002's one-file-per-use-case claim actually
true — it described a pattern the query side never had until now. `SocialQry` and
`execute_query` are deleted outright, along with the `queries.rs` file that held nothing
else.
- The `SocialCmd` **command** dispatcher (`crates/application/src/social/execute.rs`) stays as
a single `execute_command` matching on an enum. Every branch is genuinely "call port, return
event" and `execute_command` publishes the event — a real, uniform abstraction, not an
accident of history the way the query enum was.
- `get_relation`'s application-layer wrapper is deferred, not built. The port method exists and
is tested (against `InMemorySocialRepository`; neither SQL adapter's query is exercised yet),
but nothing calls it until a follow-up plan adds a `/social/relationship` endpoint — building
an unused public function now would be overbuilding.
- The dead `SocialCommandDeps.social_query` field was removed. `execute_command` never read it;
every construction site was cloning an `Arc` nothing consumed.
- `get_profile` (`crates/application/src/users/get_profile.rs`) now assembles the whole
`ProfileIdentity` — username, display name, bio, handle, actor URL, avatar/banner URLs — from
the `User` entity and `InstanceIdentity`, rather than leaving the handler to fill in `None`
for fields it didn't have. `display_name` and `bio` are populated for the first time; the API
had always returned `None` for both despite the entity exposing them.
- `get_profile`'s missing-local-row case was deliberately tolerant, not `NotFound`.
`build_federated_profile_response` built its own `GetProfileDeps` and called `execute()` a
second time with a federated `user_id` that has no row in the local `users` table — there are
three `execute` call sites in `handlers/users.rs`, not one, and the file aliases the import as
`get_user_profile_uc::execute`, which hides two of them from a naive grep. Returning
`NotFound` here would have broken federated profile viewing outright. The tolerant branch was
covered by a permanent regression test,
`tolerates_a_user_id_with_no_local_row_for_federated_style_calls`.
- **Known wart, closed:** the tolerant branch filled `username` and `handle` with empty-string
sentinels, but `actor_url` was still a well-formed-looking `{base_url}/users/{uuid}` pointing
at a user that does not exist locally — one field obviously wrong, one plausibly wrong. It
was inert only because the federated handler never read `profile.identity` (it built its own
handle/actor_url from the resolved `fed` actor). A follow-up refactor (see the C1
bypasses-and-server-split plan, task 1) made that "moment any caller starts consuming
`profile.identity` on the federated path" arrive: `get_profile` split into
`users::get_local_profile` — `Err(DomainError::NotFound(_))` for a missing local row,
`identity` always fully populated — and `users::get_federated_profile_stats`, whose return
type (`FederatedProfileStats`) carries no `identity` field at all, because the federated
handler never read one. The old regression test now lives as
`get_federated_profile_stats::tests::succeeds_for_a_user_id_with_no_local_row`, and a new
one, `get_local_profile::tests::get_local_profile_is_not_found_for_unknown_user`, pins the
behavior change directly. No type in the codebase can represent the old sentinel anymore.
ADR-0006, covering the rest of the bypasses-and-server-split plan, cross-references this
closure.
## Considered Options
- **Teach `from_actor_url` to parse `local:{uuid}`** — rejected because it keeps two encodings
for one concept and lets the API's on-the-wire form diverge from the stored and federated
form.
- **Pass `base_url: String` in deps structs and keep the static helpers** — rejected because it
fixes the symptom while leaving the primitive threaded through every layer and the duplicated
`format!` calls in place.
- **A port for instance identity** — rejected as needless indirection over a static config
value; a value object is testable without mocking.
- **Decompose commands as well as queries** — rejected because the command enum's uniform
"call port, return event" shape is a real abstraction, and splitting it would produce seven
near-identical files plus a dissolved event-publishing seam. The query enum was the one worth
breaking up because it forced every query into a single `Vec<SocialActor>` return type, which
a count or a relationship could not fit.
Footnote, unrelated to the design decisions above: writing this ADR discovered that `.gitignore`'s
`docs/` + `!docs/adr/` pattern never actually re-included anything under `docs/adr/` for new
files — excluding a directory outright stops git from descending into it, so the negation was
dead on arrival. It silently swallowed `docs/adr/0001-general-review-editing.md`, which existed
on disk with zero git history until this ADR's landing fixed the pattern to `docs/*` +
`!docs/adr/` and recovered it in the same commit.

77
docs/adr/0005-single-composition-root.md Normal file
View File

@@ -0,0 +1,77 @@
# crates/composition is the sole composition root; worker stops hand-wiring adapters
The workspace had two composition roots. `crates/presentation` built its adapters and deps
through `crates/composition`; `crates/worker` built its own, separately, in
`crates/worker/src/db.rs` (125 lines) — a 25-field `WorkerDbOutput` of which 22 duplicated fields
`composition`'s then-28-field `DatabaseOutput` already had, two of those under different names
(`_goal_command` / `_watch_event_query`, underscore-prefixed because the worker's copy never read
them). Every adapter change had to be
made twice, in two files that had already drifted (three fields — `deduplicator`,
`image_ref_command`, `image_ref_query` — existed only in the worker's copy, unreachable from the
server). Nothing enforced that the two stayed in sync; only a diff would catch it.
## Decision
**`crates/composition` is now the only composition root. `crates/worker/src/db.rs` is deleted.**
Both binaries call `composition::factory::build_database_adapters` to get one `DatabaseOutput`,
and each then builds only the deps it can honestly use from it.
- `DatabaseOutput` (`crates/composition/src/factory.rs`) is a deliberate 31-field superset of
every adapter either binary needs. The server constructs three adapters it never reads —
`deduplicator`, `image_ref_command`, `image_ref_query` (worker-only) — in exchange for one
construction path that cannot drift. That trade is the point of this ADR: a handful of
wasted `Arc` constructions on the server side is cheaper than a second hand-maintained factory.
- The deps container is split, not shared: `application::Deps` (25 fields across 10 groups) is
built by `composition::build_deps` and consumed only by `crates/presentation`; the five groups
with no server-reachable consumer — `enrich_movie`, `reindex_search`, `merge_duplicates`,
`enrich_person`, `handle_requested` — moved into a new `application::WorkerDeps`, built by
`composition::build_worker_deps` and consumed only by `crates/worker`. Every field of `Deps` is
now reachable from `presentation` except `users.delete_account` — see the dead-code exception
recorded below — and every field of `WorkerDeps` is reachable from `worker`. No field needs an
`Option` or a Noop port to express "not for this binary" — which container it lives in already
says that.
- `application::WorkerServices` is the analogous split on the services side: the strict subset of
`application::Services` the worker can actually construct (`object_storage`, `event_publisher`,
`person_enrichment`). The worker has no `auth`, `password_hasher`, `diary_exporter`,
`document_parser`, or `review_logger` — those use cases don't run in the worker process, so
`WorkerServices` simply doesn't carry them.
- `build_worker_deps` takes `&DatabaseOutput`, not `&Repositories`, even though `Repositories` is
what `build_deps` takes and superficially looks like the more natural shared type. The worker
cannot honestly construct a `Repositories`: five of its six federation-sourced fields
(`remote_watchlist`, `social_command`, `follow_graph`, `block_query`, `federation_admin` — the
sixth, `federated_profile`, is already `Option` in `Repositories` for unrelated reasons) have no
worker-side adapter. `DatabaseOutput` only carries fields both binaries can genuinely fill, so
passing it instead of `Repositories` needed no new `Option` and no fake adapter.
- Exactly one field is unreachable from its own binary, and it is called out by name rather than
folded into a group comment: `Deps.users.delete_account` (`DeleteAccountDeps`) has zero callers
anywhere in the workspace — a pre-existing dead use case, not a consequence of this split. Every
other field that used to be described as "worker-only" or "not reachable" is now reachable from
the binary that reaches it, because it lives in that binary's own container.
- Both binaries were booted under the default feature set (`sqlite`, `sqlite-federation`) to prove
this: the worker reached its steady polling state without panicking, and the server served both
a Bearer-authenticated API call and a cookie-authenticated HTML page. A container wired to the
wrong adapter is exactly the kind of defect unit tests can't see — only a running process can.
## Considered Options
- **A separate `WorkerExtras` struct, kept alongside `Deps`** — rejected because it recreates the
exact problem this ADR fixes: two structs that have to be kept in sync by hand, just smaller
ones than `WorkerDbOutput` was.
- **Noop ports for the five `Services` the worker lacks** (`auth`, `password_hasher`,
`diary_exporter`, `document_parser`, `review_logger`) — rejected because it reintroduces the
sentinel-port pattern this codebase has been actively removing elsewhere; a `Services` field the
worker can't fill should not exist in a struct the worker holds, dressed up as a port that
panics or no-ops if called.
- **`Option` fields on a single shared `Deps`/`Services`, meaning "not for this binary"** —
rejected for the same reason as ADR-0002/0003 avoid `Option` for "not applicable here": absence
is a fact about which binary you're in, and that fact is better expressed by which container
(`Deps` vs `WorkerDeps`, `Services` vs `WorkerServices`) a field lives in than by an `Option`
every reader then has to unwrap or justify.
## Known follow-up, out of scope here
`crates/worker/src/follow_backfill_handler.rs` references `activitypub::ActivityPubPort`
unconditionally, with no `#[cfg(feature = "federation")]` gate. `cargo build -p worker
--no-default-features --features sqlite` (federation off) fails to compile because of it. This
predates worker unification — confirmed byte-identical at the commit before this plan started —
and is unrelated to the composition-root split; it needs its own fix.

152
docs/adr/0006-handlers-call-use-cases-only.md Normal file
View File

@@ -0,0 +1,152 @@
# Handlers call use cases only; check-handler-repos guard, and presentation goes lib-only
CONTRIBUTING.md had said "presentation handlers never touch repositories directly" for a while,
but nothing enforced it. 16 call sites across `crates/presentation/src/handlers/` called a
repository method directly — `repos.user.find_by_id(...)`, `repos.wrapup_repo.find_existing(...)`,
`repos.import_session.get(...)`, and thirteen more — instead of going through an
`application`-layer use case. Each bypass carried real logic that had nowhere principled to live:
`get_user_profile`'s local-vs-federated dispatch, `get_watchlist_page`'s decision to read a local
or a remote repository, `get_wrapup_html`'s "is this report actually ready" gate, `get_mapping_page`'s
"has this session even been parsed" gate, `build_page_context`'s viewer-chrome assembly (email,
admin flag, pending-follower badge count) — all sitting in a handler function, untested except by
whatever HTTP-level test happened to exercise that route, and invisible to `check-appcontext`
because bypassing `application` entirely means there is no `application` code to scan.
A companion problem, orthogonal but resolved in the same plan: `crates/presentation` carried
`main.rs` and every backend-selection feature (`sqlite`, `postgres`, `sqlite-federation`,
`postgres-federation`, `nats`) even though `crates/composition` (ADR-0005) was already the actual
composition root. The binary and the library were the same crate for no reason connected to
either one's job.
## Decision
**Handlers call use cases only, never repository methods, enforced by `make check-handler-repos`.**
The Makefile target greps `crates/presentation/src/handlers/` for `repos.<field>.<method>(` and
fails, naming file and line, on any match whose method name isn't `clone`. `.clone()` stays
legal: ~40 sites in handlers still read `state.app_ctx.repos.<field>.clone()` to pass an `Arc`
positionally into a use case that takes individual params rather than a deps struct — Plan C2's
job, not this plan's. The guard is two grep passes rather than one, specifically so a single
line carrying both a legal clone and an illegal call doesn't hide the illegal one: `grep -o`
extracts each `repos.<field>.<method>(` occurrence onto its own output line (file:line still
attached, since `-n`/`-r` key off the match, not the containing line), then a second `grep -v`
drops the occurrences whose method name is exactly `clone`. A single `grep -v` filtering whole
lines would have wrongly cleared a line pairing `repos.a.clone()` with an illegal
`repos.b.find_by_id(...)`. Verified against the current tree (zero offenders) and against a
deliberately reintroduced `repos.user.find_by_id(` call in `handlers/users.rs::get_user_profile`,
which the guard reported by exact file and line before the call was reverted.
**Sixteen bypass call sites became eleven use cases** (twelve modules, because one use case
split in two — see below), each one absorbing the logic that used to live in the handler:
- `users::get_local_profile` / `users::get_federated_profile_stats` — the local and federated
halves of what was one `get_profile` use case (ADR-0004). Absorbs the local-vs-federated
identity decision described below.
- `users::get_page_viewer` — **page-chrome assembly**: viewer email, admin flag, and pending-
follower badge count, previously three separate lookups scattered through
`handlers/helpers.rs::build_page_context` with independent, inconsistent error tolerance.
`get_page_viewer::execute` now propagates every error with `?`; `build_page_context` is the
one place that decides to degrade chrome on failure (logs, falls back to `(None, false, 0)`).
- `users::resolve_username_to_id` — username → id lookup, previously a bare
`repos.user.find_by_username(...)` call.
- `users::get_profile_settings` — settings-page field assembly (username, display name, bio,
avatar/banner, `also_known_as`, custom profile fields), previously assembled inline from two
separate repository calls in the handler.
- `import::get_mapping_stage` — **import stage gating**: `NotFound` for both "session missing"
and "session has no parsed file yet", collapsing two conditions the old handler redirected to
the same place for. Owns the `SAMPLE_ROW_LIMIT = 5` constant that used to be a bare `.take(5)`.
- `import::get_preview_stage` — the other **import stage gate**: `Ok(NotYetMapped)` when row
results are absent (a business state, not an error) vs `Ok(Ready(PreviewRows))`. Called from
two handler sites (`get_preview_page` HTML and `api_get_preview` JSON) — the one deliberate
exception to "one use case, one call site" in this plan.
- `import::get_session_state` — session summary (columns, mapping status, row count) for the
JSON session-state endpoint.
- `import::apply_profile_and_map` — orchestrates `apply_profile::execute` then
`apply_mapping::execute` inside the use case layer instead of the handler, matching the
existing `MovieEnrichmentHandler`-calls-`enrich_movie` precedent rather than inventing a new
shape for cross-use-case calls.
- `diary::get_user_feed` — **feed author lookup**: resolves the RSS feed's author display name
(from the user's email local-part) alongside the diary entries, previously two separate reads
(`repos.user.find_by_id` then `get_diary::execute`) glued together in `handlers/rss.rs`.
- `watchlist::get_watchlist_for_owner` — **the local-vs-federated watchlist decision**: probes
`repos.user.find_by_id` to decide whether the requested owner is a local user (paginated,
validated `PageParams`) or a federated one (unpaginated, `limit`/`offset` ignored — a
pre-existing asymmetry, preserved deliberately rather than "fixed" into new behavior). Returns
`WatchlistView::Local(Paginated<..>) | Remote(Vec<..>)`; the handler still chooses which
template to render.
- `wrapup::get_ready_report` — **wrapup report gating**: collapses "no record", "wrong status",
"repository error", and "`Ready` but `report: None`" into one `NotFound`/`ValidationError`
surface, reproducing a pre-existing quirk (repo errors becoming a 404) rather than introducing
new-looking behavior for it. Takes `domain::models::wrapup::WrapUpScope` (`User(Uuid) |
Global`) directly — an application-layer duplicate of that type was drafted and then dropped in
favor of the existing domain enum once it was clear the duplicate added no impedance, just a
second name for the same idea.
**The pending-follower count is one use case behind two transports.** Both the SPA's
`GET /api/v1/social/followers/pending/count` endpoint and the classic UI's page chrome
(`get_page_viewer`) read the same `FollowGraphQuery::count_pending_followers` call, which is also
the same method `application::social::count_pending_followers` wraps. No second counting path
exists; the two frontends cannot show different numbers because there is exactly one place the
number comes from.
**The ADR-0004 sentinel wart is closed.** ADR-0004 recorded a "known wart": `get_profile`'s
tolerant, non-`NotFound` handling of a missing local row filled `username`/`handle` with
empty-string sentinels while `actor_url` stayed a well-formed-looking (but meaningless) URL — one
field obviously wrong, one plausibly wrong — and noted it was inert only because no caller read
`profile.identity` on the federated path. This plan is the "moment any caller starts consuming
`profile.identity` on the federated path" ADR-0004 said to watch for, and it resolved the wart by
removing the shared type rather than patching it: `get_local_profile` returns
`Err(DomainError::NotFound(_))` for a missing local row with `identity` always fully populated on
success, and `get_federated_profile_stats`'s return type (`FederatedProfileStats`) carries no
`identity` field at all, because the federated handler never read one. No type in the codebase
can represent the old sentinel state anymore — there is no `ProfileIdentity` for the federated
path to half-fill.
`get_federated_profile_stats::tests::succeeds_for_a_user_id_with_no_local_row` carries forward
the old regression test's intent;
`get_local_profile::tests::get_local_profile_is_not_found_for_unknown_user` pins the new
`NotFound` behavior directly.
**`crates/presentation` is lib-only; `crates/server` owns the binary.** `main.rs` and the
integration test that exercised it moved to a new `crates/server` crate, which now holds every
backend-selection feature (`sqlite`, `postgres`, `sqlite-federation`, `postgres-federation`,
`nats`) that used to live on `presentation`. `presentation`'s `Cargo.toml` now has exactly one
feature axis — `federation` (on by default) — and depends on no adapter crate except
`activitypub`, gated behind that same feature. `cargo tree -p presentation` shows no
`sqlite`/`postgres`/`sqlx`/`infra-wiring` anywhere in the tree. The one remaining adapter
dependency, `activitypub`, is out of scope here — inverting `ActivityPubPort` so `presentation`
depends on nothing but `domain`/`application`/`composition` ports is Plan D's job, not this
plan's.
**`app_ctx.repos` deliberately remains, and stays `pub`.** This plan removed sixteen repository
*method calls* from handlers; it did not touch the ~40 `state.app_ctx.repos.<field>.clone()`
sites that pass an `Arc` positionally into use cases taking individual params, and it did not
delete the `repos` field itself. `check-handler-repos` is written narrowly on purpose — it
forbids the bypass this plan closes, not the clone-and-pass pattern Plan C2 is scoped to migrate.
Removing `repos` now would break every one of those 40 sites for no gain this plan can claim.
## Considered Options
- **Thin entity-fetch use cases** — e.g. a bare `get_user(id) -> User` wrapping
`repos.user.find_by_id` — were rejected as a repository with extra indirection. They would
satisfy `check-handler-repos` literally while leaving every real decision (local-vs-federated,
stage gating, chrome assembly, report gating) sitting in the handler exactly where it was,
which is the actual problem this plan exists to fix, not a naming exercise.
- **Deleting `app_ctx.repos` in this plan** — rejected because ~40 clone-passed sites across the
converted handler files still construct use case calls positionally from it; deleting the field
would require migrating all 40 to deps-struct calls in the same change, which is Plan C2's
scope, not this one's. Landing that migration and the use-case extraction together would make
either piece harder to review and revert independently.
## Known follow-ups, out of scope here
- Plan C2 migrates the ~40 `repos.<field>.clone()` sites in handlers to deps-struct calls and
then removes `app_ctx.repos`. `check-handler-repos` does not (and should not) block on these —
it only forbids the method-call bypass this plan closed.
- Plan D inverts `ActivityPubPort` so `presentation` drops its one remaining adapter dependency,
`activitypub`.
- The pre-existing `follow_backfill_handler.rs` federation-gate bug (recorded in ADR-0005's
follow-ups) is untouched by this plan.
- `cargo test -p composition` in isolation fails one test, `worker_deps_wiring`, with
`unreachable code: worker_deps_wiring needs the sqlite feature; cargo test --workspace enables
it transitively via presentation/worker's default features` — pre-existing, not introduced or
fixed by this plan, and invisible to `make check`'s workspace-wide `cargo test`, which enables
the feature transitively.

148
docs/adr/0007-no-repositories-in-presentation.md Normal file
View File

@@ -0,0 +1,148 @@
# `AppContext` carries no `Repositories`; `composition` is a dev-only dependency of `presentation`
ADR-0006 closed sixteen call sites where a presentation handler called a repository method
directly, but explicitly left `AppContext.repos` in place: ~40 sites still read
`state.app_ctx.repos.<field>.clone()` to pass an `Arc` positionally into a use case that took
individual params rather than a deps struct, and migrating all of them at once would have made
ADR-0006's handler-extraction change harder to review and revert independently. `check-handler-repos`
was written narrowly on purpose, forbidding only the direct-call bypass ADR-0006 closed, not the
clone-and-pass pattern this plan (Plan C2) exists to remove.
Plan C2 did that migration in five tasks. Tasks 1-3 converted the 23 use cases the plan's initial
audit found reading an `Arc` cloned from `Repositories` — across `integrations`/`webhook`, `wrapup`,
`watchlist`, `import`, `users`, `movies`, `goals`, `auth`, and `search` — from individual `Arc`
parameters to a single `&FooDeps` struct each, registered on `application::Deps` and constructed by
`composition::build_deps`, following the convention ~30 other use cases already used. Task 4 replaced
the last two direct repository calls in the codebase — `extractors.rs`'s `AdminApiUser` and
`AdminUser` both called `repos.user.find_by_id(...)` inline to check admin status — with a new
`users::authorize_admin` use case, closing the gap ADR-0006 honestly scoped as handlers-only (these
were two axum extractors, not handler functions).
Task 5 widened `check-handler-repos` from `handlers/` to all of `crates/presentation/src`, and reported
zero offenders. That measurement was wrong. Its pattern was `repos\.[a-z_]+\.clone\(\)` — built to
catch the clone-and-pass shape the plan had inventoried — and it could not see two other shapes present
in the same tree: a bare reference passed by argument (`&state.app_ctx.repos.diary`, no `.clone()`
anywhere) and a `let`-bound access (`if let Some(ref fed_query) = ...repos.federated_profile`,
followed by a method call on the *binding* rather than on `repos` itself). `diary::get_diary`,
`diary::get_review_history`, and `diary::export_diary` all took their repository argument **by
reference** rather than owned, so their three call sites (`handlers/diary.rs`, `handlers/rss.rs`,
`handlers/movies.rs`, `handlers/helpers.rs`) were structurally invisible to a regex anchored on
`.clone()`. `handlers/users.rs`'s federated-profile lookup bound `repos.federated_profile` with `if
let` and called `.get_federated_profile(...)` on the bound variable, which no `repos.<field>.<method>(`
pattern can match across a `let`. The first attempt at this task found the five live sites by
inspection when it went to delete the field the guard was meant to make safe to delete, and correctly
returned blocked rather than deleting `AppContext.repos` out from under them; a new Task 7 then
converted all five (plus a sixth, non-`repos` internal caller broken purely by `get_diary`'s signature
change) before this attempt at Task 6 ran.
## Decision
**`AppContext` no longer has a `repos` field, and `presentation` no longer depends on `composition`
in production.** `pub repos: Repositories` and the `pub use composition::Repositories;` re-export are
both deleted from `crates/presentation/src/context.rs`. There is no route by which any code in
`crates/presentation/src` (outside `src/tests/`, which builds its own `Repositories` purely to drive
`composition::build_deps` for test fixtures) can reach a repository directly — not a stored field, not
an import, not a re-export. Every dependency a handler or extractor needs now comes from
`state.app_ctx.deps`, a prebuilt `Arc<application::Deps>` assembled once at the composition root.
`crates/presentation/Cargo.toml` moves `composition` from `[dependencies]` to `[dev-dependencies]`.
After the field deletion, `composition` is referenced only from `src/tests/extractors.rs`, which calls
`composition::build_deps` to produce a real `Deps` for test fixtures — a legitimate use of the
composition root's public API, just not one production code needs. Confirmed both ways:
`cargo tree -p presentation -e normal --depth 1` shows no `composition` anywhere in the production
dependency tree; `cargo tree -p presentation -e dev --depth 1` shows it as a direct dev-dependency.
`crates/server` (ADR-0006's home for the binary and every backend-selection feature) already depended
on `composition` directly in `[dependencies]` — that entry is untouched by this task — because it is
the actual composition root, not `presentation`. It still builds a `Repositories` and still needs
`AppContext { deps, .. }` without a `repos` field, so this task's field deletion required two small
production-adjacent fixes outside `crates/presentation` that no earlier task in this plan touched:
`crates/server/src/main.rs::wire_dependencies` (production) and `crates/server/tests/api_test.rs`
(its integration-test harness) both used to write `presentation::context::{AppContext, Repositories}`
and construct `AppContext { repos, deps, .. }`; both now import `Repositories` from `composition`
directly and drop `repos` from the struct literal, keeping the local `repos` binding alive only long
enough to pass `&repos` into `composition::build_deps`, the same pattern `presentation`'s own test
helper uses.
**All use cases that used to read an `Arc` out of `AppContext.repos` — 23 converted in Tasks 1-3, three
more (`get_diary`, `get_review_history`, `export_diary`) converted in Task 7 once the guard's blind
spot surfaced them, plus two newly-created use cases (`users::authorize_admin` in Task 4,
`users::get_federated_profile` in Task 7) — now take a single deps struct.** The two `extractors.rs`
admin checks (`AdminApiUser`, `AdminUser`) both now call `users::authorize_admin::execute`, returning
`Result<Option<bool>, DomainError>` so "user not found" and "repository call failed" stay
distinguishable by construction rather than by pattern-matching a `DomainError` variant an adapter
happens not to raise today. `handlers/users.rs`'s federated-profile branch now calls
`users::get_federated_profile::execute`, which mirrors `Repositories::federated_profile`'s own
`Option`-typed optionality (federation can be configured off) rather than inventing a new way to say
"this binary doesn't need it." Both migrations preserve their call sites' exact rejection behavior on
every path (missing port, repository error, empty result, found result) — verified path-by-path
against the pre-migration code in Task 4's and Task 7's own reports, not asserted here.
**Two use cases remain on individual params, correctly, because they were never reachable through
`AppContext.repos` in the first place:** `movies::request_enrichment::fetch_if_stale` and
`movies::enrich_movie::execute`, both called only from `MovieEnrichmentHandler`
(`crates/application/src/movies/event_handler.rs`), a `domain::ports::EventHandler` implementor that
the composition root constructs directly with individual `Arc` fields, the same way any adapter is
wired — never through `state.app_ctx.deps`. `fetch_if_stale` takes fully individual parameters
(`&dyn MovieEnrichmentClient`, `&Arc<dyn MovieProfileRepository>`, plus two value params).
`enrich_movie::execute` actually does take a struct, `EnrichMovieDeps` — but one built locally inside
`MovieEnrichmentHandler::handle` from its own already-held `Arc` fields, not a struct registered on
`application::Deps` or constructed by `composition::build_deps`. Neither shape is what this ADR means
by "use cases now take deps structs"; both are correctly out of scope for the same reason `Services`-
sourced use cases are (see below), and the plan's original note that they're "adapter-called, don't
migrate them" holds up.
**This ADR's "use cases take deps structs" claim is scoped to what Plan C2 touched: use cases that
used to read `AppContext.repos`.** It is not a claim that every `execute` function in the codebase
takes an `application::Deps`-registered struct. Two examples outside that scope, found while
fact-checking this ADR, are worth naming so nobody mistakes their shape for a regression:
`diary::log_review::execute` is called directly by two presentation handlers
(`handlers/diary.rs::post_review` and `::create_review`) with an individual
`&state.app_ctx.services.review_logger` argument — legitimate, because `review_logger` comes from
`AppContext.services`, a field this plan never touched, not from `repos`. `wrapup::compute::execute`,
`import::cleanup::execute`, and `integrations::cleanup::execute` also take individual params, but none
of the three is ever called from a presentation handler — `compute` is called internally by
`wrapup::handle_requested`, and the two `cleanup` use cases are called only from
`crates/application/src/jobs/`, the background-job runner. All four were outside Plan C2's stated
scope before this task started and remain untouched by it.
**`check-handler-repos` is now a single whole-crate rule: any mention of `app_ctx.repos` outside
`src/tests/` is an offender.** The two-pass shape-matching regex (`repos.<field>.<method>(`,
`repos.<field>.clone().<method>(`) is gone. It is gone because it is no longer needed, not because it
was simplified for its own sake: with `AppContext.repos` deleted, there is no shape of legitimate
production access left to distinguish from an illegal one — no clone-as-argument to permit, no direct
call to forbid separately. A single substring match is sufficient and, unlike the old guard,
unevadeable: it cannot be blind to a reference pass or a `let`-binding the way the shape-matching guard
demonstrably was (see above), because it does not attempt to parse the shape of the access at all — it
only asks whether the now-nonexistent field is mentioned. Verified by injecting `app_ctx.repos.user`
into `extractors.rs` (a non-test production file) and confirming `make check-handler-repos` fails,
naming the exact file and line, before reverting.
## Considered Options
- **Leaving the shape-matching two-pass guard in place, just widened to the whole crate** — this is
what Task 5 actually did, and it is what let three reference-pass sites and one `let`-binding site
through with a clean ✅. Once `repos` no longer exists, keeping the old guard's shape-matching would
mean maintaining two regex passes to catch a category of mistake (any mention at all) that a single
`grep` already catches completely. Rejected as strictly worse than the simple rule, not merely
equivalent to it.
- **Deleting `check-handler-repos` entirely, since the compiler now enforces the invariant** — a
production file referencing `app_ctx.repos` simply fails to build once the field is gone, so the
guard is redundant with `cargo build` in the narrow sense of "will this compile." Rejected because
the guard is cheaper to run (`grep` vs. a full workspace build) for `make check`'s fast-path checks,
and because it fails with a direct "here's the offending line" message rather than a compiler error
several frames removed from the actual mistake (e.g. a struct-literal error at the `AppContext`
construction site rather than at the stray reference itself).
## Known follow-ups, out of scope here
- Plan D inverts `ActivityPubPort` so `presentation` drops its one remaining adapter dependency,
`activitypub` — untouched by this plan, exactly as ADR-0006 recorded.
- The pre-existing `follow_backfill_handler.rs` federation-gate bug and the isolated
`cargo test -p composition` `worker_deps_wiring` failure (both recorded in ADR-0006) are unaffected.
- `diary::log_review`, `wrapup::compute`, `import::cleanup`, and `integrations::cleanup` remain on
individual params, correctly — none of them ever read from `AppContext.repos`, so none was ever in
this plan's scope. Whether they *should* eventually move to deps structs for consistency with the
rest of the codebase is a separate, unstarted question this ADR takes no position on.
- `CONTRIBUTING.md`'s architecture rules were updated in this same commit to stop describing use cases
as taking `&AppContext` (a claim already false before this plan) and instead describe the
`&FooDeps`-struct convention this ADR documents.

261
docs/adr/0008-presentation-names-renderers-not-reachers.md Normal file
View File

@@ -0,0 +1,261 @@
# `presentation` may name renderers, not reachers; `ActivityPubPort` inverts into three `domain` ports
ADR-0006 and ADR-0007 both closed with the same known follow-up, in the same words: "Plan D inverts
`ActivityPubPort` so `presentation` drops its one remaining adapter dependency, `activitypub`." That
framing understated the problem in one direction and overstated it in another. Understated, because
`activitypub` was not the only adapter crate `presentation` depended on — `crates/presentation/Cargo.toml`
carried **eleven** adapter-crate entries at `051d9ce`: ten unconditional (`auth`, `metadata`,
`poster-fetcher`, `object-storage`, `template-askama`, `rss`, `export`, `importer`, `jellyfin`, `plex`)
plus `activitypub` as an optional dependency behind the `federation` feature. Overstated, because the
reason the inversion kept getting deferred — that moving the trait into `domain` would drag
`k_ap::RemoteActor` and `k_ap::BlockedDomain` in with it, giving the domain layer a dependency on a
third-party federation library — turned out to be an artifact of treating a 17-method trait as one
indivisible unit.
Seven of the eleven were simply dead. They were found by commenting each dependency line out and
running `cargo check -p presentation --all-features`, then repeating with `--all-targets`: `auth`,
`metadata`, `poster-fetcher`, `object-storage`, `rss`, `export`, `importer`. **Both grep-based methods
that would have been the obvious way to audit this gave wrong answers**, and the reason is worth
recording because it also determines the shape of this ADR's guard: `use <crate>::` misses the
fully-qualified call sites, and plain `<crate>::` collides with same-named *modules* inside
`application` and `presentation`. `auth::` matches `application::auth::logout`. `rss::` matches
`handlers::rss::get_user_feed`. Both crates therefore looked live to a source grep while being
entirely unreferenced as crates. The one function in `presentation` that genuinely named an adapter
type — `mappers/social.rs::remote_actor_to_dto`, taking `activitypub::RemoteActor` — had zero callers
anywhere in the workspace and was deleted with the file.
`ActivityPubPort`'s 17 methods split across three **disjoint** consumer sets, which is what dissolves
the `k_ap`-in-`domain` blocker:
| Methods | Only consumer |
| --- | --- |
| `follow`, `unfollow`, `accept_follower`, `reject_follower`, `remove_follower`, `block_actor`, `unblock_actor`, `get_blocked_actors` | `CompositeSocialAdapter`, **inside the `activitypub` crate itself** |
| `actor_json`, `followers_collection_json`, `following_collection_json`, `get_blocked_domains`, `add_blocked_domain`, `remove_blocked_domain` | `presentation` |
| `get_following`, `import_remote_outbox`, `run_backfill_for_follower` | `worker::FollowBackfillHandler` |
Eight of the seventeen have exactly one call site today, and it lives in the same crate as the trait,
so they did not need a trait. That is a claim about call sites, not reachability: `ActivityPubWire`
also hands `crates/server` the same `Arc<ActivityPubService>` that `CompositeSocialAdapter` wraps (the
`service` field, below), so all eight are `pub` and callable from outside the crate today — nothing
does, but nothing stops it either. See the deferred item below on closing that gap.
Exactly two of the seventeen return `k_ap::RemoteActor` — `get_blocked_actors`, which is one of those
eight and therefore lost its trait, and `get_following`, which is worker-only. Presentation reached
neither. The only `k_ap` type in
presentation's six signatures is `BlockedDomain` in `get_blocked_domains`'s return, and both call
sites consumed it structurally — reading `.domain`, `.reason`, `.blocked_at` into an
`api_types::BlockedDomainResponse` or a `template_askama::BlockedDomainEntry` — without ever naming
the type. `BlockedDomain` is `{ domain: String, reason: Option<String>, blocked_at: String }`;
`blocked_at` is `String` at every layer including both destinations, so a three-field domain-owned
record reproduces it exactly. The worker's `get_following` does return `Vec<RemoteActor>`, but the
worker reads exactly `.url` and `.outbox_url`. Two minimal domain models therefore replace both `k_ap`
types at the boundary, and `crates/domain/Cargo.toml` gained no dependency other than a new
`[dev-dependencies]` entry for `tokio`, to run the noop tests.
## Decision
**`presentation` may name things that render output; it may not name things that reach external
systems or storage.** `template-askama` is the sole permitted adapter crate, because an HTML template
engine is a renderer: it turns data the handler already holds into bytes. Everything else in
`crates/adapters/` reaches — over HTTP, to a database, to object storage, to a message queue — and a
handler that names one of those has, by construction, a path around the application layer that
ADR-0006 and ADR-0007 spent two plans closing. `crates/presentation/Cargo.toml`'s `[dependencies]`
now lists exactly one entry from `crates/adapters/`: `template-askama`. Verified by iterating all 26
directory names under `crates/adapters/` against the section and finding exactly one hit.
**`ActivityPubPort` is deleted and replaced by three consumer-shaped traits in
`domain::ports::federation`.** `ApDocumentPort` (3 methods) serves ActivityPub documents for content
negotiation and is called only from presentation. `InstanceBlocklistPort` (3 methods) is instance-wide
domain blocklist administration, also presentation-only. `ApBackfillPort` (3 methods) pulls remote
content in and pushes local content out after a follow is established, and is worker-side only — no
HTTP handler calls it. The eight adapter-internal methods lost their trait entirely;
`CompositeSocialAdapter` now takes a concrete `Arc<k_ap::ActivityPubService>` and calls them as
inherent methods. All nine surviving methods return `Result<_, DomainError>` rather than
`anyhow::Result<_>`, so the error type crosses the boundary as a domain type. `grep -rn
"ActivityPubPort\|NoopActivityPubService" crates` returns nothing.
**The adapter carries the three impls on a local wrapper type, `ApServiceAdapter`, not on
`k_ap::ActivityPubService` directly — this is forced, not stylistic.** The plan originally specified
`impl domain::ports::ApDocumentPort for k_ap::ActivityPubService`. That does not compile: E0117, the
orphan rule. From inside `crates/adapters/activitypub`, both the trait (owned by `domain`) and the
type (owned by the external `k-ap` crate) are foreign. The deleted `port.rs` compiled only because
`ActivityPubPort` was *local* to that crate — the very property being given up by moving the trait to
`domain`. `ApServiceAdapter { service: Arc<ActivityPubService> }` in
`crates/adapters/activitypub/src/federation_ports.rs` holds all three impls, matching two existing
precedents in the same crate: `DomainUserRepoAdapter` (a local wrapper across a foreign boundary) and
`CompositeSocialAdapter` (one type serving three `domain` traits, handed out as three `Arc`s). A
side effect worth naming, since the plan feared the opposite: because `ApServiceAdapter` has no
inherent `actor_json`, `self.service.actor_json(..)` is unambiguous and no fully-qualified call or
recursion guard is needed. `ActivityPubWire` gained `document`, `blocklist`, and `backfill` fields,
three `Arc` casts of one `ApServiceAdapter`.
**`actor_json` keeps its `Err(_) => 404`; the two collection handlers now return a logged 500.** This
is the plan's one intentional behavior change and lands in its own commit (`8bfeae5`). The asymmetry
is a measured property of `k_ap`, not a preference: `followers_collection_json` and
`following_collection_json` never look the user up. They derive a URL from the UUID and count rows, so
a nonexistent user yields `total = 0` and a valid empty `OrderedCollection` — `Ok`, not `Err`. Every
`Err` those two can produce is therefore a genuine infrastructure or serialization failure, and the
404 they used to emit misreported it as "no such user", hiding real faults from operators.
`actor_json` calls `get_local_actor`, which genuinely fails for a missing user, and a federation peer
probing an actor URL should get 404 — so it is untouched, deliberately. Two tests in
`crates/presentation/src/tests/api_handlers.rs` pin the new behavior; both were observed failing with
`left: 404, right: 500` before the change, and both send `Accept: application/activity+json`, without
which the handlers redirect and the tests would have passed against the old code too.
**Jellyfin and Plex parser construction moved to the composition root.** `presentation`'s two webhook
handlers used to name `jellyfin::JellyfinParser` / `plex::PlexParser` inline, passing each unit struct
by reference straight into `run_ingest`. The parsers are now built once in `composition::build_deps`
and held on `application::Deps`'s `IntegrationsGroup`; the
handlers read `state.app_ctx.deps.integrations.{jellyfin,plex}_parser`. `jellyfin` and `plex` moved
from `presentation`'s `[dependencies]` to `composition`'s, unconditionally. `ingest::execute`'s
signature and its test-suite `FakeParser` injection point are untouched.
**A fourth Makefile guard, `check-presentation-adapters`, encodes the rule.** `make check` now runs
`fmt-check clippy test check-appcontext check-handler-deps check-handler-repos
check-presentation-adapters`, and both CI workflows (`.github/workflows/ci.yml:47` and
`.gitea/workflows/ci.yml:47`, which must stay in sync) invoke it. **It reads `Cargo.toml`, not `src/`,
deliberately** — for exactly the reason the `auth`/`rss` audit above failed: a source-level
`<crate>::` grep cannot distinguish an adapter crate from a same-named module inside `application` or
`presentation`, so it is structurally incapable of deciding this question. The guard `awk`s the
`[dependencies]` section, `sed`s out the key of each `key = ...` line, and rejects any that matches a
directory name under `crates/adapters/` other than `template-askama`. All 26 adapter directory names
equal their package `name` today (verified by reading each `Cargo.toml`), which is what makes
directory names a safe source of truth. Both behaviors were observed: `jellyfin` added under
`[dependencies]` produced `❌ presentation depends on adapter crate(s): jellyfin` and exit 1; the same
line under `[dev-dependencies]` passed, which is what proves the `awk` range actually stops at the
next section header rather than scanning the whole file.
**`cargo build -p worker --no-default-features --features sqlite` compiles again, closing a follow-up
ADR-0005 opened and ADR-0006/0007 carried forward.** `crates/worker/src/follow_backfill_handler.rs`
named `activitypub::ActivityPubPort` unconditionally, with no `#[cfg(feature = "federation")]` gate,
so the federation-off worker build failed with E0433. `FollowBackfillHandler` now holds
`Arc<dyn domain::ports::ApBackfillPort>`, a trait from a crate the worker depends on unconditionally,
so the gate is unnecessary rather than merely added. Three `map_err` closures that converted `anyhow`
to `DomainError` at the call site collapsed to bare `?`, because the port already returns
`DomainError` — verified to produce the same `DomainError` and the same log output as the closures did.
### Four things this ADR does not claim
These were measured or observed during the work and are recorded rather than fixed. This project
documents its guards as useful-not-airtight (see ADR-0007 on `check-handler-repos`); the same honesty
applies here.
**`check-presentation-adapters` has four known blind spots.** Each needs an atypical or deliberately
obfuscated manifest form, and none is present today:
1. `[dependencies.jellyfin]` dotted-table form — `awk`'s `/^\[/` closes the range, and the crate name
lives in the section header rather than on a `name = ` line. Invisible to the guard.
2. `[target.'cfg(...)'.dependencies]` — never scanned at all; the range opens only on an exact
`^\[dependencies\]`. This is the most plausible *accidental* blind spot, though unlikely in a web
backend crate.
3. `alias = { package = "jellyfin", workspace = true }` — the `sed` extracts `alias`, which matches no
adapter directory. Requires deliberate evasion.
4. Directory name equal to package name is *assumed*, not enforced. True for all 26 adapters today; a
future adapter whose directory and package names diverge would be unguarded.
**Two HTML handlers now log one extra line on the error path.** The plan's contract for Tasks 1-7 was
"identical status codes, identical response bodies, identical log lines", and the third of those is
not literally true. `ap_err` in the adapter logs `ActivityPub error: {:?}` before returning — the same
message, at the same level, that presentation's deleted `ap_to_domain` used to emit — so the two
blocklist HTML handlers (`handlers/social.rs` `post_blocked_domain`, `post_remove_blocked_domain`),
which log their own line on error, now emit two lines where they emitted one. The two collection
handlers (`get_followers_collection`, `get_following_collection` in `handlers/social.rs:707` and
`:742`) previously logged nothing on the error path and now log two lines, not one: the adapter's
`ap_err` line and their own (`followers_collection_json error: {:?}` / `following_collection_json
error: {:?}`), so their error path went from 0 lines to 2. The two lines are complementary, not
duplicative — the adapter's carries the cause, the full `anyhow` chain via `{:?}`; the handler's
identifies which operation failed. Strictly more detail, same level, no client-visible change;
unavoidable once the conversion point moved into the adapter, short of reintroducing a shim in
presentation purely to suppress it.
**"The worker sqlite-only build compiles" is not "it is clippy-clean."**
`cargo clippy -p worker --no-default-features --features sqlite -- -D warnings` reports three errors:
unused `app_config`, unused `remote_goal`, and `FollowBackfillHandler is never constructed`. The first
two predate this work and are unrelated to it. No gate runs this combination — `make clippy` is
`cargo clippy -- -D warnings` and CI adds `--all-targets`, both at default features, where the
worker's defaults are `["sqlite", "sqlite-federation"]` and the struct *is* constructed. Gating
`mod follow_backfill_handler;` behind a `federation` cfg would silence one of the three and leave the
combination dirty regardless, so it was measured and deliberately not done.
**`cargo check -p presentation --no-default-features --all-targets` fails, and failed identically
before this work.** Two errors, both in the test target: E0433 `cannot find social in handlers`
(`handlers::social` is federation-gated; something in the test tree names it unconditionally) and
E0277 `Panic: RemoteWatchlistRepository` not satisfied. Measured on the worktree and on master at
`051d9ce` with the same command and the same two errors, so it is pre-existing, not a regression from
the port swap. The library itself compiles federation-off both before and after — `cargo check -p
presentation --no-default-features` finishes clean on both — so the `#[cfg(feature = "federation")]`
gates around the new `ap_document` / `ap_blocklist` fields do hold. It remains a known-broken feature
combination in the repo — not the only one; see the `server`/`worker` `federation` meta-feature gap
recorded under Known follow-ups below, which is also pre-existing.
## Considered Options
- **Moving `ActivityPubPort` into `domain` whole, accepting a `k_ap` dependency in the domain layer** —
the option the previous two ADRs implicitly assumed and rejected, which is why the work kept being
deferred. Rejected here too, but the point is that it was never the only option: splitting by
consumer means the two `k_ap` types that forced the dependency (`RemoteActor`, `BlockedDomain`) land
respectively in the eight methods that need no trait at all and in one method whose three scalar
fields a domain-owned record reproduces exactly.
- **One `domain` trait with all nine cross-boundary methods, instead of three** — simpler to wire: one
`Arc` on `AppContext` instead of two, and one trait to name instead of three. Rejected because the
three consumer sets are disjoint: presentation would hold a handle exposing `run_backfill_for_follower`,
the worker one exposing `actor_json`, and the ability to call a method is what the previous two ADRs
were about removing. Three traits make "no HTTP handler runs a backfill" a compile-time fact rather
than a convention.
- **Grepping `src/` for adapter crate names instead of reading `Cargo.toml`** — the intuitive form of
this guard. Rejected because it was tried as the audit method and demonstrably produced wrong
answers in both directions: `auth` and `rss` looked live because `application::auth` and
`handlers::rss` exist, while fully-qualified call sites are invisible to a `use <crate>::` pattern. A
guard cannot be more reliable than the measurement technique it encodes.
- **Deleting `port.rs` in the same task that added the new ports** — the plan's original shape, which
would have left `presentation`, `server`, and `worker` uncompilable for three tasks and required
skipping `make check` in between, contradicting the plan's own green-at-every-commit constraint. Split
into additive-first (new ports alongside the old trait) and delete-last (after every consumer moved),
which is why the branch has an extra commit and no known-red window.
## Known follow-ups, out of scope here
- **Collapsing the worker's three backfill calls into one adapter-side operation.** `FollowBackfillHandler`
calls `get_following`, filters for the actor, then `import_remote_outbox`. That filtering could live
behind one port method, but moving it would move logic across the boundary during a refactor whose
contract was behavior preservation. Not started; a real question, deliberately left open. The
duplication is also a naming collision: `ApBackfillPort::get_following` is now the third
`get_following` in the `domain::ports` glob namespace, alongside `FollowGraphQuery::get_following`
(`crates/domain/src/ports/social.rs:47`) and `FollowQuery::get_following`
(`crates/domain/src/ports/follow.rs:53`). All three compile and answer different questions, but a
reader seeing `deps.something.get_following()` now has three candidates.
- **Splitting `actor_json`'s error so a missing actor and an infrastructure failure become
distinguishable.** Today both produce `DomainError::InfrastructureError` and the handler returns 404
for either. Doing it properly needs `k_ap` to expose a typed error; doing it by inspecting the
`anyhow` message for a substring would be fragile against a third-party crate's wording. Rejected as
currently formulated, not deferred for lack of time.
- **`service`'s reachability gap, noted above.** `CompositeSocialAdapter::new` is only called from
`crates/server/src/main.rs:127`, outside this crate, using `ActivityPubWire::service`. Moving that
construction into `wire()` itself and exposing `Arc<dyn SocialCommand>` / `FollowGraphQuery` /
`BlockQuery` on `ActivityPubWire` instead would let `service` become crate-private, and would make
"the eight adapter-internal methods are unreachable from outside the crate" true of reachability, not
just of call sites, closing the gap recorded above. Not started here — `wire()` doesn't have the
`UserRepository`, `FollowCommand`, and `FollowQuery` handles `CompositeSocialAdapter::new` takes,
and threading them through is real wiring work, not a rename.
- **The `PRESENTATION_ALLOWED_ADAPTERS` guard trusts `template-askama` by name, not by property.**
`PRESENTATION_ALLOWED_ADAPTERS := template-askama` permits that crate because it is a renderer today,
but nothing stops it from later acquiring a reaching dependency and smuggling it through the guard
transitively — the Makefile checks the manifest's direct `[dependencies]` entries, not what
`template-askama` itself depends on. It is clean today: its own `[dependencies]` are `askama`,
`chrono`, `uuid`, `domain`, `api-types`, none of which reach. The "renderer, not reacher" premise
genuinely holds, but it is trusted rather than checked.
- **A fourth known-broken feature combination, pre-existing and untouched by this branch:**
`cargo check -p server --no-default-features --features sqlite,federation` fails with E0433 on
`activitypub`. `server`'s `federation` feature (`crates/server/Cargo.toml:14`) is a bare meta-feature
that enables `application/federation` and `presentation/federation` but does not imply
`dep:activitypub` — only `sqlite-federation` and `postgres-federation` do that. The same gap exists in
`crates/worker/Cargo.toml`. Neither manifest was modified by this branch (both last touched at
`4d8f836`, before this branch's merge-base `051d9ce`), so this is recorded here rather than
attributed to this plan.
- The other deferred items recorded in ADR-0005/0006/0007 are unaffected by this plan: the isolated
`cargo test -p composition` `worker_deps_wiring` failure, the admin-extractor body assertions, the
SPA zod schemas, and `check-handler-repos`'s substring fragility. The one item this plan does close
is the worker sqlite-only build, as recorded above.
- `crates/presentation/Cargo.toml`'s `[dev-dependencies]` still lists `composition`, which ADR-0007
established as legitimate — test helpers build a real `Repositories` and call `composition::build_deps`.
`check-presentation-adapters` intentionally does not scan that section, and it does not need to:
`composition` is not an adapter crate.

367
docs/adr/0009-federation-is-optional-at-the-dependency-level.md Normal file
View File

@@ -0,0 +1,367 @@
# Federation is optional at the dependency level, not just in code paths
The `federation` feature has existed since ADR-0002 and has been treated by every ADR since as the
switch that decides whether this instance speaks ActivityPub. It did not do that. It gated code
paths, HTTP routes and struct fields; it removed nothing from the build. Measured at this branch's
merge-base `f6ff7dd` with `cargo tree -p server -e normal --prefix none`, deduplicated by
`sed 's/ (\*)$//' | awk '{print $1" "$2}' | sort -u | wc -l`:
| Feature set at `f6ff7dd` | unique crates | `activitypub_federation` |
| --- | --- | --- |
| `sqlite` | 357 | present |
| `sqlite,sqlite-federation` | 357 | present |
| `postgres` | 368 | present |
| `postgres,postgres-federation` | 368 | present |
Identical, to the crate. The cause was one unconditional manifest edge per backend:
`crates/adapters/sqlite/Cargo.toml:16` read `sqlite-federation = { workspace = true }`, and
`crates/adapters/postgres/Cargo.toml:16` the same for `postgres-federation`. Those crates depend on
`k-ap`, which depends on `activitypub_federation`. Anything that wanted a database therefore got the
whole ActivityPub stack, and `--no-default-features --features sqlite` compiled, linked and shipped
it. The feature was a runtime posture, not a build-time decision.
Two consequences followed from that, and only the first was known. The first is size: an operator who
does not want federation still pays for it in compile time, binary size and dependency surface. The
second needs more care than an earlier draft of this document gave it: it is a read-path bug, not the
write-path bug it first looked like.
Because the whole social stack lived on one struct inside the federation crate, a federation-off
`crates/server` had nothing to wire `SocialCommand`/`FollowGraphQuery`/`BlockQuery` to, so it wired
`NoopSocialCommand`/`NoopSocialQuery` — `follow` returned `Ok(())` and wrote nothing, every query
returned `0` or `vec![]`. It is tempting to read that as "following a user on your own instance
silently did nothing," directly contradicting ADR-0003. **That reading is wrong: nothing federation-off
could ever reach `NoopSocialCommand::follow` in the first place.** Every social write route —
`/social/follow`, `/social/unfollow`, and everything else `SocialCommand` exposes — lives in
`federation_api_routes()` (`crates/presentation/src/routes.rs:485`) and `federation_html_routes()`
(`routes.rs:194`), both `#[cfg(feature = "federation")]`, merged into the router only under that gate
(`routes.rs:459`, `:188`). `crates/server/Cargo.toml` takes `presentation` with `default-features =
false`, so on a federation-off build `POST /api/v1/social/follow` is a 404 — the handler was never
compiled in. Nobody could follow anybody, successfully or silently, because there was no route to call.
`NoopSocialCommand::follow`'s silent `Ok(())` was a latent trap, genuinely worth removing, but it was
never a shipped bug: correct-looking code sitting behind a door that no federation-off request could
open.
`NoopSocialQuery` sat behind no such door. Three consumers call `FollowGraphQuery` through routes that
carry **no federation gate at all**: `get_local_profile` (follower/following counts on `/users/{id}`,
which is an ungated route at `routes.rs:76`), `get_page_viewer` (the pending-follower badge), and
`get_activity_feed` (the followed-user set behind the activity feed's `filter_following`). All three
read `deps.social_query`, which `crates/composition` wires from `repos.follow_graph` — before this
branch, `NoopSocialQuery`, federation-off. **A federation-off instance did not fail to follow; it lied
about its own follow graph on every profile view and every activity-feed request** — reporting zero
followers, zero following, no pending requests, no followed users, regardless of what
`ap_followers`/`ap_following` actually held. That is the live bug this branch fixes, and it is the read
half, not the write half: the write half was unreachable over HTTP, so its noop cost nothing; the read
half was reachable everywhere, so its noop cost the truth.
## Decision
**A `<backend>-social` crate speaks only `domain::ports`; a `<backend>-federation` crate speaks
ActivityPub; the former must never depend on the latter.** `crates/adapters/sqlite-social` and
`crates/adapters/postgres-social` are new and hold the implementations of the **seven** *domain* ports
that the federation crates used to carry, across three types. `SqliteSocialRepository` /
`PostgresSocialRepository` carry five — `FollowCommand`, `FollowQuery`, `FederatedProfileQuery`,
`RemoteWatchlistRepository`, `FederationAdminQuery` — and two more sit on their own single-purpose
types: `SqliteApContentQuery` implements `LocalApContentQuery` (`sqlite-social/src/ap_content.rs:9`),
and `SqliteRemoteGoalRepository` implements `RemoteGoalRepository`
(`sqlite-social/src/remote_goals.rs:18`). This is not cosmetic: those last two are exactly what the base
`sqlite` crate re-exported from the federation crate before this split
(`crates/adapters/sqlite/src/lib.rs:35`, `:121`) — they are the answer to "why did `sqlite` depend on
`sqlite-federation` at all." `sqlite-federation` and `postgres-federation` keep the twelve `k_ap` trait
impls — `ActivityRepository`, `KeypairRepository`, `RemoteActorCache`,
`AnnounceRepository`, `RemoteReviewRepository`, `DomainBlocklist`, `ActorBlocklist`, and the five
`follow/` traits. The base backend crates now depend on `-social` unconditionally and on `-federation`
not at all; the federation crates enter the graph only through the `sqlite-federation` /
`postgres-federation` features. That inversion is the whole mechanism: the feature is real because the
edge it gates is the only edge.
Measured on this branch with the same command and the same dedup pipeline:
| Feature set | unique crates | `activitypub_federation` |
| --- | --- | --- |
| `sqlite` | **282** | **absent** |
| `sqlite,sqlite-federation` | 358 | present |
| `postgres` | **293** | **absent** |
| `postgres,postgres-federation` | 369 | present |
Both backends drop exactly **76** crates when federation is off, and `grep -c activitypub_federation`
over the federation-off tree returns `0` for each. The on-counts are 358 and 369 rather than the old
357 and 368 because `sqlite-social` and `postgres-social` are themselves new crates in the graph;
against the old build, a federation-off deployment resolves 75 fewer crates.
**The 76 crates are visible in the shipped image, not only in `cargo tree`.** Both container images
were built and compared:
```
docker build --build-arg FEATURES=sqlite . -> 171MB
docker build --build-arg FEATURES=sqlite,sqlite-federation . -> 190MB
```
**19MB smaller.** This is the measurement that matters to an operator: a crate count is an argument
about the build graph, whereas the image size is the consequence someone actually deploys. It is also
the evidence that the decoupling reaches all the way through — a change that only moved `impl` blocks
around would leave both images the same size.
Note the second invocation is what plain `docker build .` does: **the Dockerfile's default is
`ARG FEATURES=sqlite,sqlite-federation`, i.e. federation-on.** The federation-off image — the entire
point of this ADR — is only built when asked for explicitly, so `docker build .` on its own does not
exercise it. Both commands above are the reproduction, and both should be run before a release that
claims the federation-off deployment works.
**The orphan rule forced the struct split, and this is the third consecutive plan in which it has
dictated a design.** `SqliteFederationRepository` was a two-field struct — `pool` and `instance` —
carrying **seventeen** trait impls: five owned by `domain`, twelve owned by `k_ap`. Moving five impls
to a new crate is not a matter of moving five `impl` blocks, because from inside `sqlite-social` both
`domain`'s traits and a type defined in `sqlite-federation` are foreign, which is E0117. The five
impls needed a type defined where they live, so `SqliteSocialRepository { pool, instance }` exists,
and `PostgresSocialRepository` likewise. ADR-0008 records the first two occasions
(`ApServiceAdapter`, `DomainUserRepoAdapter`); the pattern is now settled enough to state as a rule
of this codebase: **"move these impls to another crate" always implies "and define a type there to
carry them."** Plan for the type, not just the impls.
The two structs are deliberately *not* symmetric. `SqliteFederationRepository` lost its `instance`
field entirely — after the five domain impls left, nothing read it, and the alternative on the table
was an `#[allow(dead_code)]`, which was reversed during review in favour of deleting the field and
updating its seven crate-internal call sites. `SqliteSocialRepository` keeps `instance`, because
`follow_repository.rs` reads it in eight places to build the actor URLs ADR-0003 specifies. The
asymmetry was measured before the postgres half was written, so `postgres-social` was built to the
corrected shape rather than copied from its sibling and then fixed.
**A non-recursive glob misclassified an entire subdirectory, and the correction is worth recording
because it nearly moved the wrong code.** The plan's first pass classified which files were
ActivityPub-free using a `src/*.rs` glob. That glob never descends into `src/follow/`, so `follow/`
was scored as clean and slated to move into the social crate. It is the opposite of clean: all five
of its traits — `FollowerWriter`, `FollowerReader`, `FollowingWriter`, `FollowingReader`,
`FollowMigration` — are `k_ap` traits, and its files import `k_ap::{Follower, FollowerStatus,
RemoteActor, ...}` directly. A recursive rescan before any code moved corrected the classification and
moved the split from a claimed 1333/359 line division to an actual 923/769. `follow/` stayed put.
The near-miss is easy to make because the *local* follow path really is ActivityPub-free — it is just
a different file. `crates/adapters/sqlite-social/src/follow_repository.rs` imports only `domain`,
`sqlx`, `chrono`, `async_trait` and `adapter_common`, names no `k_ap` type, and carries the
`FollowCommand`/`FollowQuery` impls. Two similarly-named things sit side by side: the AP follower
protocol in `follow/`, and ADR-0003's direct-SQL local follow in `follow_repository.rs`. That the file
layout already separated them cleanly is ADR-0003's design showing through — the split was legible
because that decision had been made years earlier. The lesson stands regardless: **scan recursively**.
This was the second wrong classification produced by a single grep pattern in the same session.
**The wiring is correct now; the read path is what a federation-off deployment actually gets today.**
`application::social::LocalSocialService` implements `SocialCommand`, `FollowGraphQuery` and
`BlockQuery` against the domain ports only, and `crates/server/src/main.rs` wires one instance of it
across all three when `federation` is off, replacing three separate noops. The bodies were ported from
`CompositeSocialAdapter`'s local branches rather than rewritten, and were compared token-by-token
against the originals during review — the two copies coexisted and agreed for three commits, which is
what made that comparison possible. `CompositeSocialAdapter` then delegates its local branches to
`Arc<dyn LocalSocial>` and shrank from 299 to 207 lines; every surviving `SocialCommand` line in it is
either a delegation or genuinely ActivityPub-specific.
What this buys a federation-off build, concretely, is the read half described above:
`get_local_profile`, `get_page_viewer` and `get_activity_feed` now read real
`ap_followers`/`ap_following` rows through `LocalSocialService` instead of `NoopSocialQuery`'s
always-empty answer. **An instance rebuilt federation-off stops lying about its own follow graph** —
that is the user-visible improvement this branch ships, and the ADR previously did not name it.
The write half — `SocialCommand::follow`/`unfollow`/`accept_follow`/`reject_follow` — is now wired
correctly and errors honestly for a remote target (below), but it has **no HTTP surface** federation-off:
the social routes that would call it are federation-gated in `presentation`, as established above, and
this branch does not touch that gating. The wiring exists so it is correct on the day those routes are
ungated, not because anything reaches it today. Ungating the local-only social routes — splitting
`federation_api_routes`/`federation_html_routes` into a local-only half (follow, unfollow, accept/reject,
the counts and lists) and an AP-only half (blocks, remote admin queries, the AP follower/following
collections) — was raised and considered during review. **Decision: do not ungate them here.** It is a
deliberate non-goal of this branch and a candidate follow-up, not a gap in it: the split is a
presentation design task nobody has done or reviewed, since the two halves are currently interleaved
one function each, and getting it right means deciding case-by-case which of
`get_followers`/`get_following`/`get_relationship`/etc. are actually AP-agnostic versus AP-flavored in
their response shape. Recorded in the follow-ups below.
**A remote target on a federation-off build now errors, and the error names the actor:** `cannot
reach remote actor {actor}: this instance was built without the federation feature`
(`crates/application/src/social/local_service.rs:74`). This is the branch's one intentional behaviour
change and lands in its own commit (`b8f74eb`). The verb is "reach", not "follow", because the same
constructor serves five call sites including unfollow and unblock, and "cannot follow" would
misdescribe four of them. At the four non-follow sites `{actor}` is whatever `actor_url_of` returns,
which for a persisted row is an AP URL rather than an `@user@host` handle; the message says "remote
actor", not "handle", so both read correctly.
**The wiring fix is guarded by a Makefile grep rather than a test, and that is a limitation, not a
preference.** The decision lives in `wire_dependencies()` — a private `async fn` in
`crates/server/src/main.rs`, a *binary* crate. Binary crates expose no library API, so nothing under
`crates/server/tests/` can call it; `server/tests/api_test.rs` builds `AppState` and `AppContext` by
hand with its own fakes and never exercises the real wiring either. Making it testable means splitting
`server` into lib+bin, a structural change well outside this work. `LocalSocialService`'s *behaviour*
has five unit tests from earlier in the branch; what has no test is the *wiring*, and this repo
already expresses that class of rule as a Makefile grep guard — four existed, and ADR-0006/0007
record the convention. `check-federation-off-social` is therefore the idiomatic instrument here rather
than a consolation prize. It was observed red (three hits at `main.rs:161,164,167`) before the fix and
green after, and is wired into `make check` (`Makefile:4`), `.PHONY`, and line 47 of both
`.github/workflows/ci.yml` and `.gitea/workflows/ci.yml`. `make check` now runs **six** guards: the
five above, plus `check-social-crates-are-ap-free`, added during this review to enforce the invariant
the whole federation-optional property rests on — a `<backend>-social` crate must never depend on
`k-ap`, `activitypub`, or anything matching `*-federation`. A single manifest line
(`k-ap = { version = "0.5.0", registry = "gitea" }` in `sqlite-social/Cargo.toml`) would silently
restore all 76 crates and 19MB while `make check`, `cargo test` and CI stayed green without it.
`check-federation-off-social` was also strengthened in the same review: it previously only asserted the
noops were *absent* from `main.rs`, which passes even if `LocalSocialService` were deleted from the same
file and nothing were wired at all. It now also asserts `LocalSocialService` is *present*.
Its scope is a single file, which is narrower than its siblings and is now documented in the recipe:
`crates/server/src/main.rs` is the sole wiring site, and `worker` and `tui` do not name
`SocialCommand`, `FollowGraphQuery` or `BlockQuery` at all — verified by recursive grep over both
crates' `src/`, zero hits. Relocating the wiring out of `main.rs` would move it out of the guard's
scope; the comment says to widen the guard to `crates/server/src` if that ever happens.
**Both halves share the `ap_followers` and `ap_following` tables, and neither owns the migrations.**
Those tables are created by `crates/adapters/sqlite/migrations/0003_activitypub.sql` and
`crates/adapters/postgres/migrations/0001_initial.sql` — the *base* backend crates, which both halves
already depend on. This is ADR-0003's arrangement unchanged: a local follow writes the same rows in
the same format as a remote one, which is what makes local relationships visible to the fediverse
through the AP collection endpoints without any duplication. The consequence for maintenance is that
**a schema change to either table touches both crates**, and neither crate's directory is where you
would look for the migration. This is a real coupling, accepted deliberately, because the alternative
— a separate `local_follows` table — was rejected in ADR-0003 as two sources of truth for one concept.
**A latent bug in `worker` was exposed, not caused, by cutting the dependency edge.**
`crates/worker/src/main.rs` calls `tokio::signal::ctrl_c()` with no feature gate, but the workspace
`tokio` (`Cargo.toml:44`) declares only `macros, net, rt, rt-multi-thread, sync, time` — no `signal`.
It had been arriving transitively, through `sqlite → sqlite-federation → activitypub_federation`.
Removing that edge broke the worker build, which looked like collateral damage and was not: the worker
had a hard requirement satisfied by a dependency four hops away that it did not declare and had no
reason to expect. It would have broken the moment anything else in the graph shifted. Diagnosed by
stashing back to the pristine tree to confirm the transitive source, and fixed at the place the
requirement actually lives — `crates/worker/Cargo.toml:31` now reads
`tokio = { workspace = true, features = ["signal"] }`. This is the kind of thing a decoupling finds
that no amount of reading finds.
### What this ADR does not claim
**Blocks remain federation-only, and `LocalSocialService::block`/`unblock` always error.** There is no
domain port for block storage — blocking is `k_ap::ActorBlocklist` and `k_ap::DomainBlocklist`, both
`k_ap` traits on the federation struct — so making blocks work federation-off is a new port and new
tables, not a wiring change. It was not attempted.
**`LocalSocialService::get_blocked` returns `Ok(vec![])` federation-off — the same stale-row shape this
section already discusses at length for `FederationAdminQuery`, and worth naming rather than leaving
implicit.** `block`/`unblock` always error, so no block can be *recorded* through this path either way,
but an instance that accumulated blocks while federation-ON and was then rebuilt federation-OFF would
report none. Operationally harmless: block enforcement itself lives in `k_ap`, absent federation-off,
and the route that exposes this list (`/social/blocked`) is federation-gated, so nothing acts on the
stale answer. But it is the same class of claim as the paragraph below makes for
`FederationAdminQuery`, and this section should say so plainly rather than naming `block`/`unblock` and
skipping the query.
**`FederationAdminQuery` is still noop'd federation-off, returning an empty list, and the empty list
is an invariant rather than an assumption.** The weaker argument — "you cannot follow a remote actor
federation-off, so empty is honest" — is only plausible. The stronger one is provable from the wiring
this change installs: `LocalSocialService::follow_resolved` hard-errors on any non-`Local` target
(`local_service.rs:253-256`) and `block`/`unblock` always error, so a federation-off build **cannot
persist a remote follow through this path at all**. The empty list is enforced, not estimated. A
comment at the wiring site now records this so a future reader does not re-derive it.
There is one exception, and it is a display artifact rather than a functional regression: **an
instance that ran federation-ON, accumulated remote follow rows, and was then rebuilt federation-OFF
would report zero remote follows while the rows still exist.** Those follows cannot be acted on
without federation anyway, so nothing is lost operationally, but the admin view understates reality.
Wiring `FederationAdminQuery` for real is viable whenever that is judged worth surfacing — both
`-social` crates implement it, and both are present federation-off. It was left noop'd because doing
otherwise would surface rows the build cannot act on.
**"The federation-off builds compile" is not "they are clippy-clean."**
`cargo clippy -p worker --no-default-features --features sqlite -- -D warnings` still fails, with two
errors: unused `remote_goal` and `FollowBackfillHandler is never constructed`. ADR-0008 recorded
*three* here; the third, unused `app_config`, is incidentally gone because this branch hoists
`let instance` above `build_database_adapters` in both binaries and derives it from `app_config`
unconditionally. The remaining two predate this work and no gate runs this combination. Likewise
`cargo build -p server --no-default-features --features sqlite` emits one pre-existing warning,
unused `ap_content_repo` at `main.rs:65` (introduced at `ac7edd6`, long before this branch), and the
single-backend postgres builds emit unreachable-pattern warnings on `DbPool` matches that are
inherent to compiling a two-variant enum with one variant enabled. All warnings, none errors, none
new.
**The known-broken feature combinations ADR-0008 recorded are unchanged.**
`--features sqlite,federation` on `server` still fails — `federation` is a bare meta-feature that does
not imply `dep:activitypub` — and `cargo check -p presentation --no-default-features --all-targets`
still fails with the same two pre-existing errors. This branch neither fixed nor worsened either.
## Considered Options
- **Gate the existing `sqlite-federation` dependency behind the feature instead of splitting the
crate** — the obvious one-line fix, and the reason it does not work is the whole point. The five
domain-port impls the rest of the system needs unconditionally (`FollowCommand`, `FollowQuery`,
`FederatedProfileQuery`, `RemoteWatchlistRepository`, `FederationAdminQuery`) lived on the same
struct in the same crate as the twelve `k_ap` impls. Gating the edge would have removed those five
too, and the `federation`-off build would have had no follow storage at all — which is precisely the
state that produced the silent-noop bug. The dependency could not be made conditional until the code
behind it was separated by what it speaks.
- **`#[cfg(feature = "federation")]` inside the existing crates rather than two new crates** —
cheaper in file moves, and rejected because a cfg inside a crate does not remove the crate's
manifest dependency on `k-ap`. `cargo tree` would have been unchanged, `activitypub_federation`
would still resolve, and the headline measurement would still read 357/357. Conditional compilation
cannot subtract a dependency edge; only the manifest can.
- **Keep the noops and document that federation-off means no social features** — internally
consistent, and rejected because it contradicts ADR-0003 on the merits, not merely on the docs. That
ADR's entire argument is that a local follow is two SQL writes and does not need ActivityPub. A build
that omitted ActivityPub and therefore omitted local-follow *storage and querying* would prove
ADR-0003 wrong; the split proves it right at the storage and service level — the HTTP surface for the
write path remains gated regardless of which option was chosen here, see the follow-up below.
- **Put `follow_resolved` on `SocialCommand` instead of on a new sibling trait** — the smaller-looking
option, rejected by enumerating implementors rather than by taste. Only `LocalSocialService` and one
domain test stub satisfy all four original `LocalSocial` bounds; `CompositeSocialAdapter` and
`InMemorySocialRepository` are real `SocialCommand` implementors that are *not* `LocalSocial` and
could not meaningfully implement `follow_resolved`. Extending `SocialCommand` would have stranded
two types. `LocalSocial` is now a marker supertrait over five traits with an unconstrained blanket
impl, matching `k_ap::FollowRepository`'s precedent over its own five.
- **Resolve the follow target twice on the local path** — what the plan originally prescribed, and
what the first implementation did: the composite called `resolve_target` to choose a branch, then
handed the raw `FollowTarget` to `local.follow`, which resolved it again. Cost was a redundant
`find_by_username` per handle-based local follow plus a race in which a user deleted between the two
resolutions makes a *federation-ON* deployment emit "this instance was built without the federation
feature", which is nonsense in that configuration. Replaced by `ResolvedFollow::follow_resolved`, so
the composite passes the identity it already computed. The five `LocalSocialService` tests passed
byte-unmodified across that change, which is the evidence that plumbing moved and behaviour did not.
## Known follow-ups, out of scope here
- **Ungating the local-only social routes** (`/social/follow`, `/social/unfollow`, accept/reject, the
counts and lists in `federation_api_routes`/`federation_html_routes`) so a federation-off build gets an
HTTP surface for the write path described above. Considered and deliberately deferred during review,
not because it is undesirable but because it is unreviewed design work: it needs splitting those two
functions into a local-only half and an AP-only half, deciding case-by-case which handlers are
AP-agnostic versus AP-flavored, and nobody has done or reviewed that split. Until it happens,
`LocalSocialService`'s write path is correctly wired and unreachable over HTTP federation-off.
- **The Dockerfile hand-enumerates every workspace member's `Cargo.toml`, and this branch broke it.**
`docker build .` failed at `RUN cargo fetch` with `failed to read
/build/crates/adapters/sqlite-social/Cargo.toml`, because the dependency-cache layer copies manifests
one `COPY` line at a time and the two new crates had no lines. Fixed here by adding them and by
verifying the list now matches `find crates -name Cargo.toml` exactly (37 against 37), with a
comment saying that adding a crate means adding a line.
**This is a standing trap, not a one-off, and it is still unguarded.** Adding any workspace crate
silently breaks the container image, and the breakage is invisible to `make check`, to `cargo build`
in every feature combination, and to CI as currently configured — only a full `docker build` finds
it. This plan tripped it, and the project's history already records a Docker build broken across
three merges for the same reason: nobody ran it. **Recommendation, not implemented here** (it is a
separate decision): a `make` target that diffs the Dockerfile's `COPY` list against
`find crates -name Cargo.toml` and fails on any difference would close it in a few lines, and is
exactly the check that was run by hand to confirm this fix. A seventh guard is cheap; a fourth broken
release is not.
- **The error message names the actor and the build-time cause, but not which mutation was attempted.**
A user who unfollows and sees "cannot reach remote actor X: built without the federation feature"
learns why but not what. Closing this needs either per-site verbs — which reintroduces five message
strings to keep in sync — or route-level logging. Left open deliberately.
- **`FederationAdminQuery`'s stale-row case**, described above. Both `-social` crates already implement
the port federation-off, so this is wiring, not new code, if the staleness is judged worth surfacing.
- **`crates/infra-wiring` (87 lines across two files) still exists to break a real cycle** — `DbPool`,
`EventBusBackend` and `AppConfig` need to be visible to both `composition` and the binaries without
`composition` depending on `application::config`. It looks like a candidate for dissolution and is
not one; the cycle is genuine. Recorded so the next reader does not re-investigate.
- **`build_database_adapters`' two backend arms are near-duplicates** (`crates/composition/src/factory.rs:49`).
Each now constructs a `*SocialRepository` alongside everything else, so the duplication grew slightly
in this branch. Collapsing them needs a trait over the two backends' `wire()` outputs — real design
work, not a tidy-up, and not attempted during a refactor whose contract was behaviour preservation.
- **`tests/local_service.rs`'s `service()` helper takes `user_repo` by value and `follow_store` by
reference.** Pure polish, explicitly left alone.
- **`crates/adapters/` now holds 28 directories, up from the 26 ADR-0008 counted.**
`check-presentation-adapters` iterates those directory names and still passes, so neither new crate
leaked into `presentation`. ADR-0008's caveat carries over unchanged: the guard assumes directory
name equals package name, true for all 28 today but not enforced.
- The deferred items in ADR-0005/0006/0007/0008 are otherwise unaffected by this plan.