--- status: open last_updated: 2026-09-29 reviewed_artifacts: - docs/architecture/README.md - docs/architecture/overview.md - docs/architecture/transport.md - docs/architecture/backend.md - docs/architecture/doors.md - docs/architecture/open-questions.md - docs/architecture/decisions/ (ADR-001..014) - docs/research/ (vision, git-protocol, gitoxide, alk-stack, reference-policy, pocs, poc-1/2/3 findings, push-captures, negotiation-captures) - docs/sdd_process.md - tasks/architecture/ (all five tracker/backlog tasks) - Cargo.toml - src/lib.rs tool: manual full-corpus read + alkcall/alktty source cross-checks + gitoxide source verification + feature-matrix compile probes + dyn-compatibility probe (scratch crate, MSRV-pinned) + cargo test/clippy/fmt/doc/publish-dry-run reviewer: architecture pre-decomposition review (contradiction + gap + staleness pass) scope_note: phase-1 gate review — the entire committed architecture corpus against the sibling-crate surfaces it leans on (alkcall 0.8, alktty template) and the pinned gitoxide versions. No implementation exists to review (src/lib.rs is the skeleton); this review asks whether the spec set is internally consistent and implementable as written. --- # Review 001 — Architecture Pre-Decomposition Review ## Purpose The phase-1 OQ cycle is complete (OQ-04/ADR-013 and OQ-02/ADR-014 landed 2026-09-25; every wire-shape question has a capture-grounded decision) and the next step is decomposition into implementation tasks. This review is the gate between those phases: it reads the whole architecture corpus the way a decomposer and then an implementation agent would — looking for 1. **Contradictions between decisions** — places where two Accepted documents (or an ADR and the sibling-crate API it cites) disagree, where an implementer would have to pick a winner or guess. 2. **Spec gaps that strand implementers** — wire shapes, trait signatures, or gate mechanics the docs reference but never pin, where two implementation workstreams would invent incompatible answers. 3. **Stale text** — superseded statements left standing in docs the decomposer reads first. Findings A-1/A-2/A-3 are the reason this review exists: each is a place where an implementation agent hits a hard failure (a compile error, an unservable wire path, an unenforceable gate) that the decomposition agent cannot resolve because the mechanism is not specified anywhere. The rest are smaller, but each would have cost a session to discover mid-implementation. This review is docs-only by design — there is no code to review yet (the crate is the skeleton). The code-level review passes (alkty-001 / alksocks-002 style) come after the first implementation batches land. ## Methodology - Full read of every architecture document, all 14 ADRs, every research document, the SDD process, and all five `tasks/architecture/` files. - Cross-check of every load-bearing external API claim against the real sibling sources: alkcall 0.8 (`AccessControl::check` semantics, `OwnershipStore`, `Identity`, `ChannelOpenSpec`, `OperationSpec`, ADR-025/017/011/047 texts) and alktty (`TtyBackend` async-trait pattern, `register_openable`/establisher shape, `TtySession`). - Cross-check of the pinned gitoxide APIs against the reference clone (`/workspace/gitoxide`, matching published 0.87.1): the ADR-013 ingestion composition (`Bundle::write_to_directory_eagerly`, `thin_pack_base_object_lookup`, `PreviousValue::MustExistAndMatch`), ADR-014's subtraction mechanism (`gix_traverse::commit::Simple` filtered walks), `gix_validate::reference::name`, and the gix-pack feature set claims. - **Feature-matrix probe**: `cargo check` under all four configurations (default, `--no-default-features`, `--features sha256`, `--all-features`) — the ADR-012 §4 feature split and the wire-layer-without-gix claim, tested rather than trusted. - **Dyn-compatibility probe**: a scratch crate (`/tmp/opencode/dynasync`) reproducing ADR-012 §1's literal trait shape (`async fn` + `Arc`) under both the active toolchain (1.94) and the pinned MSRV (1.88) — see A-2. - Verification battery on the current tree (see baseline below). - Cross-reference integrity sweep: every ADR-NNN / OQ-NN reference in `docs/architecture/` resolved against existing files, including the sibling-crate ADR citations (alkcall 003/011/017/025/040/047, alknet 033/035); every deferred OQ checked for the two-half protocol (tracker entry + `tasks/architecture/` task) and for circular deferral. ## Verification Baseline All commands on the reviewed tree (commit `d6d013e`, clean): - `cargo check` (default features): **clean**. - `cargo check --no-default-features`: **clean** — the wire layer compiles without gix (the ADR-010/ADR-012 §4 claim is true today). - `cargo check --features sha256` and `--all-features`: **clean**. - `cargo test`: **passes** (0 tests — the skeleton; the suite is trivially green). - `cargo clippy --all-targets -- -D warnings`: **clean**. - `cargo fmt --check`: **clean**. - `cargo doc --no-deps`: **0 warnings**. - `cargo +1.88 check`: **clean** (the manifest's `rust-version = "1.88"` is real). - `cargo publish --dry-run --allow-dirty`: **completes**. - `git --version`: **2.43.0** — the integration-test surface the ADR-013/014 captures were built against is present in this environment for implementation-phase verification. ## Summary Statistics | Severity | Count | IDs | |----------|------:|-----| | Critical | 3 | A-1, A-2, A-3 | | Major | 3 | A-4, A-5, A-6 | | Minor (stale doc) | 3 | D-1, D-2, D-3 | | Observation | 5 | N-1..N-5 | No finding questions the architecture's direction — the pure-protocol- crate shape (ADR-010), the capture-grounded wire decisions (ADR-013/ 014), and the auth model (ADR-011) all survived the pass intact. Every finding is a *mechanism* gap: the WHAT is decided, a HOW the docs implicitly assumed turns out to be wrong or missing, and the resolution is an amendment or one small ADR each. The three criticals share a property: they sit exactly where a decomposer would cut tasks (the op gate, the backend trait signatures, the native session preamble), so they would each silently fork into incompatible implementations rather than fail loudly. Finding prefixes: `A` (architecture/spec defect — decision or mechanism), `D` (documentation staleness), `N` (notes/observations). --- # Part A — Findings ## A-1 [critical] — ADR-012 §3's two-tier op gate ("scope `git:admin` **or** ownership") is not expressible in alkcall's `AccessControl`, which composes restrictions as AND **Files**: `docs/architecture/decisions/012-registry-backing-and-ops.md` §3 (the gate table and the "uses the checks `AccessControl::check` already composes" claim); alkcall `src/registry/spec.rs:94-171` (the mechanism); `docs/architecture/backend.md` §"The two op kinds". **Problem**: ADR-012 §3 gates the CRUD ops with a two-tier rule — `git/repo/delete|update|get` are allowed to "scope `git:admin` **or** ownership" — and states the rule "uses the checks `AccessControl::check` already composes: `required_scopes` for the global tier, `resource_type` + ownership for the self-owned tier." I verified alkcall's `check()` directly: the restrictions compose as **AND**, in a fixed order — `required_scopes` are checked first and fail hard (`Forbidden`), then `required_scopes_any`, then the ownership/resource block. There is no scope-OR-ownership composition, and `required_scopes_any` is scope-set-OR, not scope-or-ownership. A single `OperationSpec` therefore cannot express the gate: - `required_scopes: ["git:admin"]` + `resource_type` on one spec → requires **both**; a self-owned non-admin user is Forbidden at the registry and the handler never runs. - ownership-only (`resource_type` + `resource_id_path`, no scopes) → a global admin who does not own the repo is Forbidden at the registry. - The "or" cannot be recovered handler-side, because the registry gate fires before dispatch reaches the handler in both shapes. Consequence: any of the three gate rows in the ADR-012 §3 table, built naively, either locks out the self-owned tier (the entire point of the two-tier design — "a user who creates a repo administers *their own* repos") or locks out the global admin. This is not a corner: it is the primary authorization path for every management op in the crate. **Options**: - **(a) Handler-side two-tier check (recommended).** `git/repo/create` keeps its pure-scope registry gate (`required_scopes: ["git:repo:create"]` — no ownership tier, so the static engine serves it fine). `git/repo/delete|update|get` carry an empty `AccessControl::default()` and the handler evaluates the two-tier rule itself: admin scope OR `OwnershipProvider::owns(identity, "git/repo", repo_id, action)`, else a generic `FORBIDDEN`. This matches ADR-011 §4's own pattern — *policy lives in alkgit core, one tested function* — and keeps the two-tier rule unit-testable in-crate instead of hidden in an assembly-supplied provider. The gitea lesson is unaffected: visible-surface = authorized-surface is about enforcement living in the same place as exposure, and a handler-side check is exactly as enforcement-shaped as the registry gate (it denies with the same generic error; no existence oracle — the handler must return the same denial for unknown-repo and not-owner, per ADR-008's rule). - **(b) Ownership-provider composition.** Keep the gate registry-side (`resource_type` + `resource_id_path`, no scopes) and have the assembly's `OwnershipProvider` report admin-scoped identities as owning everything. Mechanically clean (all enforcement in alkcall dispatch), but it hides the two-tier policy in assembly code — untestable in-crate, invisible to the architecture, and it overloads `owns()` semantics. - **(c) Extend alkcall's `AccessControl`** with a scope-or-ownership mode. Cleanest long-term but it is a family-crate change on someone else's release cadence; not alkgit's decision to make unilaterally. **Fix (small)**: a short ADR (or an explicit amendment block on ADR-012 §3, the pattern ADR-004 used for ADR-010's layout change) recording the mechanism — recommended: option (a), with the create-op carve-out, the generic-denial requirement, and the `(store, ownership)` handler wiring spelled out. The ADR-012 §3 sentence "uses the checks `AccessControl::check` already composes" must be corrected either way — as written it certifies a composition that does not exist. --- ## A-2 [critical] — ADR-012 §1's literal trait signatures (`async fn` in traits) do not compile as `Arc`: not dyn-compatible (E0038); `async-trait` is absent from the manifest **Files**: `docs/architecture/decisions/012-registry-backing-and-ops.md` §1 (the signature block), `docs/architecture/backend.md` §"Concurrency model" ("The traits are `Send + Sync` object-safe"), §3 ("thin `OperationSpec`+handler pairs over `Arc`"), `Cargo.toml` (no `async-trait`). **Problem**: ADR-012 §1 writes the registry traits with bare `async fn`: ```rust pub trait GitRegistry: Send + Sync { async fn resolve(&self, repo_id: &str) -> Result; } ``` backend.md simultaneously claims the traits are "`Send + Sync` object-safe," and ADR-012 §3 puts them behind `Arc`. Bare `async fn` in a trait is **not dyn-compatible** — I reproduced this exactly with a scratch crate (`/tmp/opencode/dynasync`, a trait with one `async fn` coerced to `Arc`): rustc 1.94 and the pinned MSRV 1.88 both reject it with E0038 ("the trait `Reg` is not dyn-compatible ... method `resolve` is `async`"). The family precedent resolves it the same way every time: `#[async_trait]` (alktty's `TtyBackend`, alkcall's `ProtocolHandler`, both pinned at `async-trait = "0.1"` in their manifests) — but alkgit's manifest does not carry the dependency, and neither ADR-012 §1 nor backend.md says the word. Consequence: the registry traits are the first thing every backend/ops/registry-file workstream touches; the first task that writes `Arc` hits E0038 against a signature the ADR presents as normative. Because these signatures are the OQ-03 freeze inventory, the correction should be pinned *before* decomposition so no task bakes in an ad-hoc answer (hand-rolled `BoxFuture` signatures, a private `ErasedRegistry` wrapper, per-trait `#[async_trait]` vs manual desugaring — three workstreams, three answers, one freeze surface). **Fix (small)**: amend ADR-012 §1 and backend.md to `#[async_trait]` on all five traits (the signatures otherwise stand — `Send + Sync`, `resolve` async per ADR-011 §6, the write supertrait split per alknet ADR-035's shape), and add `async-trait = "0.1"` to the manifest. One conscious note for the freeze inventory: `#[async_trait]` desugars to `Pin>` — that boxed form *is* the published vtable shape, which is fine, but it should be pinned deliberately (OQ-03) rather than discovered at first publish. The five-trait family (`GitRefs`, `GitPackGen`, `GitPackIngest` too) should get the same attribute in the same amendment so all six signatures in the corpus agree. --- ## A-3 [critical] — The native path has no pinned session preamble: the service dimension is missing from the session tuple, and the channels open-op params are pinned as "repo id" only — as written, push is unservable over the `alk/git` ALPN **Files**: `docs/architecture/overview.md` (crate map: "repo id in open-op params"), `docs/architecture/decisions/010-pure-protocol-crate.md` (same pin), `docs/architecture/decisions/002-front-door-blind-core.md` (the session tuple), `docs/architecture/decisions/005-session-substrate-types.md`, `docs/architecture/transport.md` §substrate, `docs/architecture/decisions/013-receive-pack-state-machine.md` §2 (server-speaks-first on duplex push). **Problem**: every door selects the git *service* (upload-pack vs receive-pack) before the protocol starts — http by route (`/{repo}/git-upload-pack` vs `/{repo}/git-receive-pack`, doors.md), ssh by the parsed exec command (`git-upload-pack ''` vs `git-receive-pack ''`, doors.md's alkssh requirement). On the native path the carrier is underdetermined, and the two native sub-paths the crate map names each have a hole: 1. **Channels path** (`register_openable`): the docs pin the open-op params as "repo id in open-op params — the negotiation + ACL point" (ADR-010, overview crate map). Nothing carries the service, and the consequences start before the state machine even runs: - **The open-time ACL point cannot evaluate the write tier.** The reason the repo id rides the open-op params is that the open op is "the negotiation + ACL enforcement point" — ADR-007's resolve→authorize sequence runs *at open time*, before the channel carries protocol data. But `authorize(record, identity, action)` is per-action (ADR-011): fetch needs the read check, push needs the write check. Without the service in the params, the open gate cannot know which check to run — it either admits a pusher it should have rejected at the gate (deferring the write check into the session, weakening the ADR-007 posture) or over-restricts. - **The session cannot choose its state machine.** Both advertisements are server-emitted firsts on the duplex path (the V2 capability advertisement for fetch, ADR-003; the V0 ref advertisement for push — ADR-013 §2 "the server speaks first" — where "speaks first" means the advertisement precedes the client's protocol request, not the open op itself). With no service anywhere in the open-op handshake, the server cannot select which advertisement to emit. **As specified, push over `alk/git` channels is impossible.** 2. **Direct-ALPN path** (`GitAdapter`): the crate map says "POC-1 verbatim," and POC-1's shape carries the service in-band — the git-daemon request line (`git-upload-pack \0host=…\0\0version=2\0` for fetch; `git-receive-pack \0host=…\0` for push, poc-1-findings §2 / push-captures §request), client-sent before the server speaks. But that framing on the `alk/git` ALPN is *alkgit-specific wire format* (real git clients never touch the native path — they speak to doors), and AGENTS.md convention 9 explicitly says alkgit-specific framing "will get ADRs when it exists." No ADR pins it: whether `GitAdapter` parses a git-daemon-style request line, a JSON preamble, or expects the caller to pre-negotiate is unspecified. (POC-1's bridge emulated a git:// TCP server because *real git* was the client; `GitAdapter`'s native clients are `GitSession` and alkcall-speaking embedders — a different contract that no doc defines.) The session tuple itself lacks the dimension: ADR-002's tuple and transport.md's substrate inputs are "(peer identity, resolved repo id, duplex stream, `Limits`)" and "(peer identity, resolved repo id, request-reader, response-writer, `Limits`)" — no service/variant selector. The stateless substrate gets the service from the door's route, but the substrate input must carry it explicitly (nothing in the POST body reliably distinguishes an upload-pack POST from a receive-pack POST before parsing). `GitSession` (the consumer half) mirrors whatever is pinned here, so the consumer half is blocked on it too. **Fix (small — one ADR)**: pin the native session preamble. The shape the corpus already points at (my recommendation, but the ADR should decide): - The channels open-op params schema is `{repo, service}` — JSON, `service ∈ {"git-upload-pack", "git-receive-pack"}`, additive- extension rule (alksocks pinned `{}` with `additionalProperties: false` for exactly this reason: extensions stay additive; here the first extension already exists at v1). The params are the open-time ACL point (ADR-010 unchanged) *and* the service selector — the service at open time is what lets the gate run the correct `authorize` action (read for fetch, write for push) per ADR-011, instead of deferring the write check into the session. - The duplex session tuple gains the service dimension: `(identity, repo, service, stream, limits)` — amend ADR-002, ADR-005, and transport.md's substrate input lists (the authorized- repo marker from D-3 rides the same amendment). - `GitAdapter` (direct ALPN) parses a pinned in-band preamble carrying the same fields — the git-daemon request-line grammar (POC-1 verbatim, real-git-compatible framing, service-in-first-line) is the natural candidate and needs the ADR convention 9 asks for, since it is alkgit-specific wire format on a published ALPN (freeze inventory, one-way). - `GitSession::open_via_channels` sends the same params schema. Note the version dimension does *not* ride the preamble: fetch is V2-only (ADR-003 — V0/V1 clients get a clear error) and push is V0-framed unconditionally (ADR-013 §1), so `service` fully determines the state machine. That is worth stating in the ADR — it is the reason one field suffices. --- ## A-4 [major] — ADR-014 §2's done-round boundary set is "the request's haves," but haves include unverified client claims; the boundary set should be the *recognized* subset (the ADR's own honesty rule) **Files**: `docs/architecture/decisions/014-v2-negotiation-ack-loop.md` §2, `docs/architecture/transport.md` §fetch, cross-checked against `docs/research/negotiation-captures.md` and upstream behavior. **Problem**: ADR-014 §2 says the done round "generates closure(wants) − closure(haves) … with the request's haves as the boundary set." The request's haves are a mix of *acked commons* (which the server verified via `common_haves` in an earlier round) and *new, never-verified* haves the negotiator is offering. Taking the boundary set literally — subtract on the raw request haves — means the server honors unverified client claims: a client (buggy or malicious) that claims have-X without having it receives a pack missing closure(X) ∩ closure(wants) and fails client-side with "did not send all necessary objects." Self-inflicted, not a server-side hole — but it contradicts the ADR's own honesty principle ("never ack what we cannot subtract," §1/§5) at the one place subtraction actually happens, and it deviates from upstream: `upload-pack` marks a have uninteresting only after confirming the object exists server-side. The mechanism to do it right is already in the corpus: §5's `common_haves` seam. The boundary set should be the *recognized* subset — `common_haves(repo, request_haves)` — applied on the done round exactly as on the ack rounds. Existence is the operative predicate for subtraction (a non-commit have can legitimately bound traversal); the is-commit refinement in §1 exists for ACK-line correctness, not for subtraction. I verified the traversal side supports this cheaply: `gix_traverse::commit::Simple::filtered(tips, find, predicate)` takes exactly a boundary predicate ("whether a commit should be included as well as whether its parents should be traversed" — gix-traverse source), so verify-then-subtract is one predicate, not a custom walk. **Fix (small)**: one clause in ADR-014 §2 ("the boundary set is the recognized subset — request haves filtered through `common_haves` — not the raw request list; the ack rule and the subtraction rule are the same honest-boundary rule at different points") and the same clause in transport.md §fetch. Cost: one existence check per have on the done round — already paid on every ack round; no new budget kind. --- ## A-5 [major] — The consumer half (`GitSession`) is named in five documents and specified in none; its scope is the largest uncut ambiguity for decomposition **Files**: `docs/architecture/overview.md` (crate map: "new, small (TtySession analog)"), `docs/architecture/decisions/010-pure-protocol-crate.md`, `docs/architecture/backend.md` §"Public API surface", `docs/architecture/transport.md` §"Public API surface", `docs/architecture/doors.md`, `docs/architecture/decisions/012-registry-backing-and-ops.md` §4 (the dependency rider), `Cargo.toml` (unconditional `gix-protocol = "0.65"` + `gix-transport = "0.59"`). **Problem**: ADR-010 makes the consumer half a load-bearing half — "`GitSession` typed client with `connect_direct` and `open_via_channels` constructors … the alkcall-native primitive for replication/mirroring in the alknet rewrite" — and every public-API list includes it. But no document says what it *does*. The reading that it is "small" like `TtySession` (a typed wrapper over an established byte stream) is doing unexamined work: a *git* client that can drive fetch/push means client-side protocol state machines (want/have negotiation, V2 request encoding, push command/pack construction, response parsing) — an entire second surface that transport.md does not cover (it is server-side only). ADR-012 §4 explicitly deferred the deciding call — "`gix-protocol`'s response parsing or hand-roll the small client surface; `gix-transport` is almost certainly droppable. Implementation-time call, recorded, not an architecture commitment" — but the manifest carries both crates *unconditionally today* (not feature-gated, not optional), so the "pending decision" is already a compile-graph commitment. Consequence: the decomposer either silently omits the half (leaving ADR-010's producer/consumer claim unimplemented and the manifest overweight) or invents its scope (client protocol machines nobody designed). Both are bad; the decision is cheap now. **Options**: - **(a) Thin wrapper in v1 (recommended)** — `GitSession` is what the crate-map words say: `connect_direct` / `open_via_channels` constructors, the session preamble (A-3's shape), and raw pkt-line-adjacent access sufficient for alkgit↔alkgit replication to ride it — with client protocol state machines explicitly deferred (recorded as out of scope). This matches the "TtySession analog" reading exactly (`TtySession` wraps streams; the tty *protocol* lives in the negotiation layer), keeps `gix-protocol` as the future client parser, and lets the manifest carry the two deps honestly as the consumer half's declared seam. - **(b) Defer the half entirely** — drop `gix-protocol`/`gix-transport` from the manifest (real dependency-graph reduction; both pull nontrivial trees), amend ADR-010's crate map and both public-API sections, and record the alknet-replication primitive as a future half. Cleanest scope, but it walks back a headline ADR-010 claim. - **(c) Full client in v1** — make the consumer half real (V2 request encoding, negotiation loop, push construction). This is a second protocol surface with its own capture-testing burden; nothing in the research corpus validates it and no consumer needs it before the alknet rewrite. **Fix**: a user scope decision, then either an amendment (a/b) or a small ADR; whichever is chosen, the manifest's two client-side deps become honest (carried with purpose, or dropped). --- ## A-6 [major] — The backend traits' execution model is unspecified: async-trait methods on the executor vs sync methods on the blocking pool determines where ADR-009's pipeline budget lives **Files**: `docs/architecture/backend.md` §"Concurrency model" ("impls run under the adapter's tokio context"; GitPackIngest "blocking-thread friendly"), §"The trait family" (GitPackGen/ GitPackIngest signatures), `docs/architecture/transport.md` §fetch ("generation runs on `spawn_blocking` with the owned handle moved in"), `docs/architecture/decisions/009-bounded-resources-budget.md` ("max concurrent blocking pipeline tasks … enforced at assembly/acceptance time"), ADR-013 §6 / ADR-014 §5. **Problem**: the corpus describes the execution model three different ways without committing to one. transport.md §fetch states the POC-2 shape in the *wire layer's* voice ("generation runs on `spawn_blocking` with the owned handle moved in — store shared, handle per session") — but the wire layer is backend-trait-only (ADR-010): it holds `Arc` and there is no "store" or "handle" in its vocabulary; store/handle/spawn_blocking is the *gix impl's* internal shape. backend.md says the traits are async (`resolve` is; A-2's amendment makes all of them `#[async_trait]`) and impls "run under the adapter's tokio context" while also being "blocking-thread friendly." And ADR-009's blocking-pipeline budget is "enforced at assembly/acceptance time (reject/slow-path excess concurrent generations)" — but *who* admits: the wire layer around the trait call, or the impl inside it? Two workstreams implementing the same trait from these texts will diverge: one puts `spawn_blocking` in the wire layer (which then can't, because the trait method is async and the layer is gix-free), one puts it in the impl (and then the wire layer's admission budget has nothing to count). Consequence: ADR-009's only concurrency budget for the heaviest operations (pack gen/ingest) has no specified enforcement point, and the wire-layer spec carries gix-implementation vocabulary it structurally cannot use. **Fix (small)**: one paragraph in backend.md's concurrency model (referenced from transport.md), committing to the model the trait signatures already imply: trait methods are `#[async_trait]`; the wire layer enforces the pipeline-concurrency budget itself (a permit acquired around gen/ingest calls — ADR-009's admission point, now concrete); implementations must not block the async executor and own their internal threading (the gix impls run `spawn_blocking` with store-shared/handle-per-session *inside* the impl — POC-2's shape, restated at its true layer). transport.md §fetch's spawn_blocking sentence gets rephrased to the trait-contract version so the wire spec stops speaking gix. --- ## D-1 [minor] — Stale superseded text in the docs the decomposer reads first: the "Internal ops over an admin listener" framing survives in vision.md and alk-stack.md, and AGENTS.md's lifecycle list names resolved OQs as active **Files**: `docs/research/vision.md` (threat-model notes: "Registry management ops, if the gix feature ships any, are alkcall `Visibility::Internal` ops over an admin-only listener"), `docs/research/alk-stack.md` (gitea-lesson item 2: "Admin API = internal ops … `Visibility::Internal` ops over the admin interface"), `AGENTS.md` (Lifecycle Status: "Open architecture questions live in … (OQ-04 receive-pack, OQ-06 registry backing, OQ-08 identity model are the active ones)"). **Problem**: all three statements are superseded. ADR-012 §3 resolved OQ-07 by *rejecting* the Internal-ops framing in exactly these words ("right mechanism, wrong axis" — the ops are `Visibility::External`, gated by scope+ownership, registered by the assembler), and OQ-04/ OQ-06/OQ-08 are all resolved (ADR-013, ADR-012, ADR-011); the active set is OQ-03 (partially resolved) and OQ-05 (deferred). AGENTS.md further declares `docs/research/` "the current source of truth for design direction," which makes the stale bullets actively misleading rather than merely historical: a decomposer told to treat research as truth reads two contradicting admin-API designs and a stale OQ list. **Fix (trivial)**: one-line amendments — vision.md's threat-model bullet and alk-stack.md item 2 get a supersession note pointing at ADR-012 §3 (the research docs are phase-0 records; the note marks the supersession without rewriting history), and AGENTS.md's lifecycle paragraph updates the active-OQ list to OQ-03/OQ-05. --- ## D-2 [minor] — ADR-007 step 3 still names `AccessControl::check` as the enforcement mechanism — superseded in place by ADR-011's `authorize` policy function, with no note on the older ADR **Files**: `docs/architecture/decisions/007-acl-before-advertisement.md` (step 3: "Run alkcall `AccessControl::check(peer_identity)` against the repo's required access"), `docs/architecture/decisions/011-per-repo-authorization.md` §3 (the policy function that replaced it). **Problem**: ADR-011 resolved the gap in ADR-007's own context (the static ACL engine "fails closed on `identity: None`," so it cannot express anonymous-public fetch — verified against alkcall's `check()`, which returns `Forbidden("authentication required")` for `None` with restrictions). ADR-011 records the resolution, but ADR-007's decision text — the doc an implementer reads for the enforcement *order* — still instructs wiring the wrong mechanism. A literal implementation of ADR-007 breaks anonymous-public fetch, the deployment anchor. **Fix (trivial)**: an amendment note on ADR-007 step 3 ("mechanism amended by ADR-011 §3: the per-repo check is alkgit-core's `authorize` policy function evaluated on the registry record; the step *order* is unchanged") — the same pattern ADR-004 used when ADR-010 amended its layout. --- ## D-3 [minor] — The authorized-repo marker ADR-007 promises is missing from the session tuples in transport.md (and backend.md's API list) **Files**: `docs/architecture/decisions/007-acl-before-advertisement.md` ("the session entry points take an authorized-repo marker — a type the adapter constructs only after step 3 passes — so skipping the check is a type error"), `docs/architecture/transport.md` §substrate (inputs listed without the marker), `docs/architecture/backend.md` §"Public API surface" (the marker type is absent from the re-export list). **Problem**: ADR-007's type-level enforcement is one of the design's better moves (skipping ACL is a compile error, not a runtime log), but the promise is unfindable from the transport spec — the substrate input tuples omit the marker, so the decomposer slicing transport tasks from transport.md will not include it, and the first session- tuple task will "fix" the signature by removing it (or never add it). **Fix (trivial)**: add the marker to both substrate input tuples in transport.md (and note it in backend.md's public-API list — it is crate-root surface: the door must be able to construct it). Rides the A-3 tuple amendment if that lands first. --- # Part B — Non-findings (verified correct, recorded to bound re-review) - **The feature story is real, not aspirational.** All four configurations compile today on the skeleton manifest: default (`gix` + `registry-file`), `--no-default-features` (wire-only), `--features sha256`, `--all-features`. ADR-012 §4's feature split and the `default-features = false` embed claim are verified properties of the current manifest, not promises about future code. The manifest also already encodes the ADR-012 §4 facade-dropping (component crates only: gix-odb/pack/ref/object/fsck — no `gix` facade dep) and the sha1 compile-time pin. - **alkcall's ACL semantics verify ADR-011's premise exactly.** `AccessControl::check` (spec.rs:94) fails closed on anonymous callers whenever restrictions exist — the precise property ADR-011 §3 cites as why the static engine cannot express anonymous-public fetch. The *premise* of the per-repo authorization design is confirmed against the real engine, not assumed. - **The alkcall surface the ADRs cite exists as cited**: `Identity.id` as the stable logical id (auth.rs:15, alkcall ADR-025's decoupling), `OwnershipStore::record` as the async mint (ownership.rs:58, ADR-011), `ChannelOpenSpec` + openable-ALPNs-are-operations (spec.rs:26, ADR-047), `resource_id_path` for registry-side ownership extraction, `AccessControl`'s scope/resource composition (the AND-composition that A-1 turns on). alktty's `register_openable`/establisher/ `TtySession` template matches ADR-010's producer/consumer claims piece for piece. - **The gitoxide API pins are real.** Verified in the reference clone: `PreviousValue::MustExistAndMatch` (gix-ref transaction CAS), `Bundle::write_to_directory_eagerly` with `thin_pack_base_object_lookup: Option` (ADR-013 §5/§6's thin-pack composition, exactly the named parameter), `gix_validate::reference::name` (ADR-013 §9), and — load-bearing for ADR-014 — `gix_traverse::commit::Simple::filtered` with `Predicate: FnMut(&oid) -> bool`, the have-boundary traversal A-4's verify-then-subtract rides. The gix-pack feature-set claims (`generate`, `streaming-input`, `parallel` as separate features) match the crate's Cargo.toml. One item remains honestly deferred *by the docs themselves*: `gix-discover` + `gix_odb::Store::at` composition is marked "Verified at implementation" in backend.md — it was not independently verified here, and the docs do not claim it was. - **ADR-013 and ADR-014 are model capture-grounded decisions.** Both resolve every listed unknown against real-client behavior (raw stdio into real `git receive-pack` for policy ground truth; `fetch-pack.c` source cross-checks), record the failure modes of getting it wrong, bind prior tool decisions (ADR-004) to concrete calls, and file their own backlog riders (`oq-13-cas-failfast`). transport.md and backend.md are consistent with both ADRs in every detail I compared (advertisement sets, report framing, ack grammar, budget placements). - **Deferral hygiene is clean.** Both remaining OQs are properly two-half-tracked: OQ-05 (deferred(scope), ecosystem need) and OQ-03 (partially resolved; the freeze inventory itself) each have the tracker entry in `open-questions.md` with a concrete blocked-on condition *and* the machine-readable `tasks/architecture/` tracker task. Neither is circular — OQ-03's blocker (first-publish timing) is genuinely downstream of implementation, and OQ-05 waits on external ecosystem need. Resolved-early trackers (OQ-04, OQ-06) are correctly closed with their resolution recorded. - **Cross-reference integrity is exceptional.** Every ADR-NNN and OQ-NN reference across the corpus resolves to an existing file, and every sibling-crate citation I checked (alkcall ADR-003/011/017/025/ 040/047, alknet ADR-033/035, alkcall `AccessControl`/`OwnershipStore`) says what the citing document claims it says. The superseded pair (ADR-001, ADR-006) is marked Superseded and consistently referenced as such. - **The crate manifest is coherent with the architecture** beyond the two findings above: `rust-version = "1.88"` verified real, the feature comments state their ADR provenance, the dev-deps match the planned test surface (tokio full + test-util), and `cargo publish --dry-run` completes. --- # Part C — Notes and observations ## N-1 [observation] — The advertisement ref-cap's breach behavior is unstated, and truncation is the tempting implementation ADR-009's table carries "max advertisement refs" but its breach rule ("the session ends with a substrate-appropriate error") was written for the loop/size budgets; a streaming ls-refs implementation hitting the cap will be *tempted* to stop emitting refs and flush (a silent partial ref list — the nastiest failure mode a ref advertisement can have, since clones appear to succeed). One clause in transport.md (§Limits): the ref cap is fail-closed like every other budget — breach is an error, never a truncation. ## N-2 [observation] — The unknown-repo ≡ unauthorized indistinguishability is not stated at the wire-mapping point ADR-007 step 2 and ADR-008 pin the rule, but transport.md's error taxonomy (the place `RegistryError` variants get mapped onto pkt-line bands / http statuses) does not mention it. `RegistryError::NotFound` and an authorization failure must collapse to one wire error at that mapping — one sentence in transport.md §error taxonomy prevents the variant leak. ## N-3 [observation] — The freeze inventory's schemas are unpinned: `RepoRecord`, `RegistryError`, and the `git/repo/*` request/response shapes OQ-03 counts "names + schemas" in the freeze inventory, but nothing specifies the serde shape of `RepoRecord` (field names, the grants map's value shape, visibility representation), `RegistryError`'s variant set, or the four ops' JSON schemas. Two workstreams would invent them independently (the ops handlers and the registry-file store both serialize the record). One backend.md section (types + schemas, even as draft) closes it before the freeze inventory is taken as complete. ## N-4 [observation] — ADR-013 §11's push-options seam has no trait input to surface into "surfaced to the ingest/refs seam as per-push metadata" — but no backend trait signature carries a per-push metadata input, and push-options are default-off in v1. Pin the additive shape now (e.g. an `Option<&PushOptions>`-style parameter on the `GitPackIngest` binding, default `None` until the config gate opens) so opening the gate later is a signature-additive change, not a trait redesign. ## N-5 [observation] — `ls-refs=unborn` is advertised but unborn-HEAD serving was never capture-verified Every POC fixture had refs; the advertisement promises `ls-refs=unborn` (transport.md §advertisement, POC-1-validated *as an accepted capability token*, not as a served behavior — serving an unborn HEAD's symref line was never exercised against real git). This is the only advertised promise in the corpus without capture evidence. One implementation test (ls-refs against an unborn repo, real client, both substrates) closes it; if it cannot be served, the honest move per ADR-003 is dropping the token. --- # What's Good - **The two newest ADRs are the strongest documents in the corpus.** ADR-013 and ADR-014 are exactly what capture-grounded architecture should be: real-client walkthroughs (including raw stdio probes for policy ground truth), source cross-checks recorded with versions, every unknown from the OQ resolved with evidence, consequences stated honestly (including the accepted capability gaps), and backlog riders filed where optimization was declined. - **The structural spine (ADR-010/002/005/007/008/009) has survived four decision cycles without drift** — the pure-protocol-crate correction, the feature split, and the two wire ADRs all amend around it cleanly, and the corpus's cross-reference integrity is the best I have traced in the family. - **The honest-capability discipline is consistent everywhere it appears** — fetch declination by omission (POC-1-validated), the push served set matched to exactly what the state machine serves (`quiet`'s omission reasoned through, `report-status-v2`'s v1-shape argument capture-backed), and `git-upload-archive`'s fixed refusal carried across every door. - **The deferral protocol worked.** Both resolved-early deferrals (OQ-04, OQ-06) record *why* the deferral dissolved, both remaining deferrals have concrete non-circular blockers, and the two-half tracking (OQ entry + tracker task) is in place for every one of them. --- # Remediation Nothing is remediated yet — this review is the gate output. Suggested batches for the fix round (each is one session-sized unit; the criticals are ADR-writing work, not code): | ID | Finding | Recommended fix | Effort | Risk | Status | |----|---------|----------------|--------|------|--------| | A-1 | op-gate OR not expressible in `AccessControl` | new ADR (or ADR-012 §3 amendment): handler-side two-tier check, create keeps static scope gate | small | none | **resolved (ADR-015)** — option (a) shape with the OR-term generalized to the `manage` grant | | A-2 | `async fn` traits not dyn-compatible | ADR-012 §1 + backend.md amendment: `#[async_trait]`; add `async-trait = "0.1"` to manifest | small | none | **resolved** — all five traits `#[async_trait]`, desugared boxed form pinned in the freeze inventory (OQ-03), dep in manifest | | A-3 | native preamble / service dimension unpinned | new ADR: open-op params `{repo, service}`, session tuple + stateless entry gain the service selector, `GitAdapter` preamble pinned | moderate | wire-format (freeze inventory) | **resolved (ADR-016)** — `{repo, service}` open-op params (`channels/git/sub`, `additionalProperties: false`), git-daemon request line on the direct path (POC-1 verbatim, freeze inventory), service in both substrate tuples, `GitSession` mirrors the shapes | | A-4 | done-round boundary set unverified | ADR-014 §2 + transport.md clause: boundary = `common_haves`-filtered haves | trivial | none | **resolved** — boundary set is the recognized subset (`common_haves`-filtered), amendment clause in ADR-014 §2 + transport.md §fetch | | A-5 | consumer half unspecified | user scope decision, then amendment or small ADR (recommended: thin wrapper, deps carried with purpose) | small | scope | **resolved (ADR-017)** — superseding the thin-wrapper recommendation: `GitSession` is a real typed client in v1 (`ls_refs`/`fetch`/`push`), grounded in the two deployment use cases (the p2p replicator is the named downstream and needs the client protocol layer); fetch reuses gix-protocol over a custom alkcall `Transport`, push is hand-rolled to ADR-013's shapes (gitoxide has no send-pack), storage-agnostic | | A-6 | trait execution model unspecified | backend.md paragraph + transport.md rephrase: async traits, wire-layer permit, impl-internal spawn_blocking | small | none | **resolved** — backend.md concurrency model: wire layer enforces the ADR-009 permit around gen/ingest trait calls; impls own internal `spawn_blocking` (ADR-009/ADR-013 aligned) | | D-1 | stale Internal-ops + OQ-list text | supersession notes (vision, alk-stack, AGENTS) | trivial | none | **resolved** — supersession notes in vision.md (with ADR-015) and alk-stack.md item 2; AGENTS.md active-OQ list updated to OQ-03/05/16 | | D-2 | ADR-007 step-3 mechanism superseded | amendment note on ADR-007 | trivial | none | **resolved** — amendment note on ADR-007 step 3 (mechanism → ADR-011 `authorize`; step order unchanged) | | D-3 | authorized-repo marker missing from tuples | add to transport.md tuples + backend.md API list | trivial | none | **resolved** — marker added to both substrate input tuples (transport.md) and backend.md public-API list | | N-1 | ref-cap breach behavior | one fail-closed clause in transport.md | trivial | none | **resolved** — ref cap fail-closed clause in transport.md §Limits (breach is an error, never truncation) | | N-2 | unknown ≡ unauthorized at wire mapping | one sentence in transport.md error taxonomy | trivial | none | **resolved** — collapse rule stated at the variant→wire mapping in transport.md §error taxonomy | | N-3 | schemas unpinned | backend.md types/schemas section | small | freeze inventory | **resolved** — backend.md §"Registry types and schemas": `RepoRecord` serde shape (three-action grants, opaque keys, storage-root omitted from all op responses), the five-variant `RegistryError` set with wire mappings, and the four `git/repo/*` op request/response schemas (`additionalProperties: false`, `git:repo:*` error codes) | | N-4 | push-options seam | pin additive parameter shape | trivial | none | **resolved** — `GitPackIngest`'s prepare binding carries `push_options: Option<&PushOptions>` (parsed `(key, value)` pairs, verbatim and un-interpreted; `None` until the config gate opens) — pinned in ADR-013 §11 + backend.md trait description, so opening the gate is value-additive, not a trait redesign | | N-5 | `ls-refs=unborn` unverified | implementation-phase test rider (record in transport.md or a task) | trivial | none | **resolved (rider)** — unborn-HEAD verification recorded in transport.md §ls-refs and tracker task `tasks/architecture/n5-unborn-head-rider.md` (unborn fixture, real client, both substrates; drop the token if it cannot be served — ADR-003) | Suggested sequencing: (1) **A-2 + A-6** together (one trait-surface amendment set + manifest change — they are the same signature surface); (2) **A-3** (the one new wire-format ADR; A-5's decision unblocks with it); (3) **A-1** (the op-gate ADR); (4) **A-4 + D-2 + D-3 + N-1 + N-2** (one doc batch — all transport/ADR amendments, no new decisions); (5) **A-5** (user decision, then the doc change); (6) **D-1 + N-3 + N-4 + N-5** (hygiene batch). After the batch that resolves each spec's findings, the spec docs are candidates for `reviewed` status per the lifecycle definition, and decomposition can start. --- # References - ADR-010/002/005 (the structural spine A-3 amends), ADR-012 (§1/§3 — A-2/A-1's subject), ADR-013/014 (A-4's subject; verified sound), ADR-007/008/011 (the auth chain — D-2/D-3), ADR-009 (N-1, A-6's budget), ADR-003 (honest advertisement — N-5) - alkcall `src/registry/spec.rs` (`AccessControl::check` — A-1's mechanism evidence), `src/core/auth.rs`, `src/core/ownership.rs`; alkcall ADRs 003/011/017/025/040/047 (all citations verified) - alktty `src/backend.rs`/`src/channels.rs`/`src/session.rs` (the async-trait + register_openable + TtySession template) - gitoxide reference clone `/workspace/gitoxide` (gix-ref `PreviousValue`, gix-pack `Bundle::write_to_directory_eagerly`, gix-traverse `Simple::filtered`, gix-validate `reference::name`, gix-pack feature set) - `docs/research/poc-1-findings.md` (the in-band request-line shape — A-3's direct-ALPN evidence), `push-captures.md` / `negotiation-captures.md` (ADR-013/014's normative basis) - The house format: alksocks `docs/reviews/001-rfc1928-full-review.md` (structure, severity legend, non-findings discipline), alkty `docs/reviews/001-code-review.md` (status conventions)