docs(reviews): post-remediation re-review — gate passed, specs to reviewed
- docs/reviews/002-post-remediation-review.md: verifies all 14 review-001 findings landed faithfully (sibling-source re-verification + gix-transport async_trait(?Send) check + four-config/MSRV probes), records the eight residual findings (R-1..R-8) and their resolutions (ADR-018 + doc batch) - README lifecycle: draft→reviewed allows properly-tracked non-circular OQ deferrals (release-timing OQ-03 no longer blocks the transition) — R-7 - overview/transport/backend/doors/open-questions: frontmatter flipped to reviewed, timestamps refreshed; ADR-018 added to all ADR tables and the OQ-03 freeze-inventory narrative Phase-1 gate verdict: decomposition may begin Verification: cargo doc/test/clippy/fmt clean; four feature configs + MSRV 1.88 check/clippy clean
This commit is contained in:
1 parent
666abd1da4
commit
18106f461c
5 files changed
+342
-30
No files matched your search
@@ -0,0 +1,301 @@
|
||||
---
|
||||
status: resolved
|
||||
last_updated: 2026-09-30
|
||||
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..017, post-review-001 amendments)
|
||||
- Cargo.toml
|
||||
- tasks/architecture/ (all seven tracker/rider tasks)
|
||||
tool: manual full-corpus re-read post-remediation + sibling-crate
|
||||
re-verification (alkcall spec.rs/registry surface, alktty channels
|
||||
establisher) + gitoxide reference-clone checks (gix-transport
|
||||
async traits, gix-protocol Cargo) + four-config compile probes on
|
||||
the amended manifest + MSRV 1.88 check/clippy
|
||||
reviewer: architecture pre-decomposition re-review (fix-verification
|
||||
+ residual-gap pass over review 001's remediated corpus)
|
||||
scope_note: gate re-review — verifies every review 001 finding landed
|
||||
correctly, then hunts the residuals the fixes created at the new
|
||||
seams. Same standard as 001: could a decomposition agent and an
|
||||
implementation agent proceed without inventing divergent answers?
|
||||
---
|
||||
|
||||
# Review 002 — Post-Remediation Re-Review
|
||||
|
||||
## Purpose
|
||||
|
||||
Review 001 gated phase 1 on its remediation. This review verifies that
|
||||
remediation (all 14 findings, A–D–N) landed correctly and completely,
|
||||
then re-runs the contradiction/gap/staleness pass over the amended
|
||||
corpus. Same methodology class as 001: full-corpus read, sibling-source
|
||||
cross-checks, and compile probes — this time *after* the fix round, on
|
||||
the amended manifest (which gained `async-trait = "0.1"` and
|
||||
`async-client` on `gix-protocol`/`gix-transport`).
|
||||
|
||||
## Verification Baseline
|
||||
|
||||
All commands on the post-remediation tree (commit `11ceead` + the N-3
|
||||
commit `b6040d3`, clean):
|
||||
|
||||
- `cargo test`: **passes** (0 tests — still the skeleton).
|
||||
- `cargo clippy --all-targets -- -D warnings`: **clean**.
|
||||
- `cargo fmt --check`: **clean**.
|
||||
- `cargo doc --no-deps`: **0 warnings**.
|
||||
- All four feature configurations check clean: default,
|
||||
`--no-default-features`, `--features sha256`, `--all-features` —
|
||||
the manifest grew `async-trait` and the `async-client` features
|
||||
without breaking the wire-layer-without-gix claim (ADR-012 §4 still
|
||||
true).
|
||||
- `cargo +1.88 check` and `cargo +1.88 clippy --all-targets -- -D
|
||||
warnings`: **clean** — the new dep configuration holds at the pinned
|
||||
MSRV.
|
||||
- `cargo publish --dry-run --allow-dirty`: **completes**.
|
||||
- Sibling re-verification: alkcall `AccessControl` still AND-composes
|
||||
as ADR-015 routes around (spec.rs:67–72); `AuthContext.identity` is
|
||||
`Option<Identity>` — anonymous-public fetch through the channels
|
||||
establisher (ADR-016's open-time `authorize`) is expressible as
|
||||
specified; `ChannelOpenSpec`/`ErrorDefinition`/`OperationSpec`
|
||||
match backend.md's op-schema citations; alktty's
|
||||
`channels/tty/sub` establisher template matches ADR-016's
|
||||
`channels/git/sub` claims (including the per-call `AuthContext`
|
||||
identity the establisher reads); gix-transport's async client
|
||||
traits are `#[async_trait(?Send)]` exactly as ADR-017 §2 states;
|
||||
gix-protocol's `async-client` feature exists and pulls
|
||||
`async-trait` (both rust-version 1.88 — manifest alignment
|
||||
verified).
|
||||
|
||||
## Remediation Verification (Part A — all 14 findings)
|
||||
|
||||
Each verified at file level, not from the remediation table's claim:
|
||||
|
||||
- **A-1 → ADR-015**: the two-tier op gate is re-expressed as
|
||||
admin-scope-OR-`manage`-grant, handler-evaluated; ADR-012 §3's table
|
||||
carries the amendment note; backend.md §"The two op kinds" restates
|
||||
the mechanism; `create` keeps its static scope gate. The AND-
|
||||
composition premise re-verified against alkcall source.
|
||||
- **A-2**: `#[async_trait]` amendment on ADR-012 §1's signatures with
|
||||
the desugared boxed form pinned into OQ-03's freeze inventory;
|
||||
`async-trait = "0.1"` in the manifest with provenance comment.
|
||||
- **A-3 → ADR-016**: `{repo, service}` open-op params
|
||||
(`additionalProperties: false`), the git-daemon request line
|
||||
(verified verbatim against poc-1-findings §2), service in both
|
||||
substrate tuples, ADR-002/005/010 amendment notes all present.
|
||||
- **A-4**: ADR-014 §2's amendment clause (recognized-subset boundary
|
||||
via `common_haves`, ack rule ≡ subtraction rule) + the same rule in
|
||||
transport.md §fetch.
|
||||
- **A-5 → ADR-017**: consumer half specified as a real typed client
|
||||
(superseding the review's thin-wrapper recommendation with grounded
|
||||
use cases); manifest deps carry purpose comments; the
|
||||
`#[async_trait(?Send)]` claim verified against gix-transport source.
|
||||
- **A-6**: backend.md's concurrency model commits to the trait-level
|
||||
permit / impl-internal `spawn_blocking` split; ADR-009's
|
||||
enforcement-point consequence updated; transport.md rephrased.
|
||||
- **D-1/D-2/D-3, N-1..N-5**: supersession notes in vision.md +
|
||||
alk-stack.md + AGENTS.md OQ list (now OQ-03/05/16); ADR-007 step-3
|
||||
amendment; authorized-repo marker in transport tuples + backend API
|
||||
list; fail-closed ref-cap clause; unknown ≡ unauthorized at the
|
||||
variant→wire mapping; N-3 schemas section; `push_options` param on
|
||||
the prepare binding; unborn-HEAD rider task + transport.md note.
|
||||
|
||||
**Remediation verdict: complete and faithful.** No fix landed in a way
|
||||
that contradicts its finding or another decision.
|
||||
|
||||
## Findings (Part B — residuals at the fixed seams)
|
||||
|
||||
Severity: the corpus no longer has criticals. The majors are one
|
||||
backend.md section each; the minors are one-sentence fixes.
|
||||
|
||||
### R-1 [major] — Traits 3–5 have no pinned signatures; ADR-012 §1 (as amended by A-2) covers only the registry pair
|
||||
|
||||
**Files**: ADR-012 §1 (the A-2 amendment claims "all five traits'
|
||||
signatures" are pinned there — only traits 1–2 are); backend.md
|
||||
§"The trait family" (prose descriptions only); review 001's remediation
|
||||
row for A-2 ("all five traits `#[async_trait]`, signatures per ADR-012
|
||||
§1" — over-claims its own fix).
|
||||
|
||||
**Problem**: `GitRefs`, `GitPackGen`, `GitPackIngest` — the traits the
|
||||
`gix` feature exists to implement — have no method names, no parameter
|
||||
shapes, and no shared type definitions anywhere in the corpus. Sharpest
|
||||
fork: `(repo, wants, haves, limits)`'s `repo` parameter — the wire
|
||||
layer (backend-trait-only per ADR-010) holds only the resolved repo
|
||||
id, while the gix impl needs the `storage_root` to open the odb
|
||||
(`gix-odb::Store::at`), and traits 3–5 are speced as independent seams
|
||||
from `GitRegistry`, so they cannot lean on the resolve path. Is `repo`
|
||||
a `&str` id, a `PathBuf`, a registry-derived handle? Equally unpinned:
|
||||
the `GitPackIngest`→`GitRefs` prepared-updates handoff (the mechanism
|
||||
`atomic` correctness depends on — ADR-013 §7), and the object-id
|
||||
representation. Two workstreams (wire layer, gix impl) would invent
|
||||
incompatible answers, and the invented shapes are OQ-03 freeze
|
||||
surface. This is review 001 A-2's own failure mode surviving at the
|
||||
three traits whose signatures were never written.
|
||||
|
||||
**Resolution — ADR-018 §2, §4, §5**: signatures pinned for all three
|
||||
traits; `repo` is `&RepoRecord` (the wire layer resolves once per
|
||||
ADR-007 and the record carries `storage_root`; no handle type, no
|
||||
second lookup); shared types pinned (`RefLine` with the unborn-symref
|
||||
`oid: None` shape for the N-5 rider, `RefUpdate`, `RefOutcome`,
|
||||
`PreparedPush`, `PushOptions`); object ids are `gix_hash::ObjectId`
|
||||
(always-on in the manifest, hash-parameterized internally — OQ-05-safe);
|
||||
parameter ownership is boxed-`Send` streams/owned vecs so the A-6
|
||||
execution model (impl-internal `spawn_blocking`) has mechanical
|
||||
support.
|
||||
|
||||
### R-2 [major] — backend.md's `RegistryError` pin over-claims: "every failure the trait family can produce"
|
||||
|
||||
**Files**: backend.md §`RegistryError` (the "no catch-all, every
|
||||
failure the trait family can produce" sentence); ADR-013 §7–8 (CAS
|
||||
outcomes are *business results*, reasons client-displayed).
|
||||
|
||||
**Problem**: the five-variant set genuinely covers the registry pair —
|
||||
but not the object-storage traits' characteristic failures. CAS-stale
|
||||
is a per-ref *outcome* of receive-pack with its own report semantics
|
||||
(ADR-013 gives it a server-chosen reason shown to the client); fsck/
|
||||
unpack failures and missing-object aborts are report legs and aborts
|
||||
with dedicated wire mappings (ADR-004, ADR-013 §8). Forcing those into
|
||||
`Io(String)` or `Invalid` contradicts the structural-errors rationale
|
||||
the variant set exists for, or breaks the "session survives" report
|
||||
semantics.
|
||||
|
||||
**Resolution — ADR-018 §7**: `RegistryError` is re-scoped to the
|
||||
registry family (the backend.md sentence corrected); traits 3–5 get
|
||||
`StorageError` (`ObjectMissing { oid }` — the ADR-004 abort;
|
||||
`Invalid(String)`; `Io(String)` with `RegistryError::Io`'s stability
|
||||
rule), with the report-vs-error separation stated explicitly (CAS and
|
||||
fsck outcomes are `RefOutcome`/`PreparedPush.unpack` values, not
|
||||
variants — which keeps the no-catch-all claim true for both types).
|
||||
|
||||
### R-3 [minor] — `git/repo/update` PATCH-vs-PUT ambiguity
|
||||
|
||||
**Files**: backend.md §op schemas (`{repo_id, visibility?, grants?}` —
|
||||
"both optional, full-record replace of the provided fields").
|
||||
|
||||
**Problem**: "optional" reads PATCH; "full-record replace" reads PUT
|
||||
(omitted = cleared). The wrong reading is destructive: a
|
||||
visibility-only update that clears grants locks the grant-holder out
|
||||
of the repo via the op they legitimately called.
|
||||
|
||||
**Resolution**: PATCH pinned in backend.md — omitted fields left
|
||||
unchanged; present fields replaced wholesale (`grants` present =
|
||||
whole-map replace; partial grant edits are caller-side
|
||||
read-modify-write); the response echo (already specified) is the
|
||||
confirmation surface. The table cell wording aligned.
|
||||
|
||||
### R-4 [minor] — doors.md's alkssh hand-off tuple predates both amendment rounds
|
||||
|
||||
**Files**: doors.md §alkssh ("hand (identity, repo, post-auth stream,
|
||||
limits)").
|
||||
|
||||
**Problem**: missing the `service` dimension (ADR-016) and the
|
||||
authorized-repo marker (review 001 D-3, whose fix swept transport.md
|
||||
and backend.md but not this third tuple site) — the exact miss D-3
|
||||
itself warned about ("the first session-tuple task will fix the
|
||||
signature").
|
||||
|
||||
**Resolution**: the tuple now reads `(identity, repo, service,
|
||||
authorized-repo marker, post-auth stream, limits)` with the exec
|
||||
command named as the service selector per ADR-016.
|
||||
|
||||
### R-5 [minor] — Repo-id grammar unpinned; the N-3 example implies a two-segment id the http route shape doesn't express
|
||||
|
||||
**Files**: backend.md §`RepoRecord` (example `"alkdev/alkgit"`), §op
|
||||
schemas (`invalid` for "empty repo id" implies charset rules exist);
|
||||
doors.md (single-placeholder routes `/{repo}/…`).
|
||||
|
||||
**Problem**: two-segment ids need a two-placeholder http route shape no
|
||||
doc pins; the `Invalid` variant's empty-id check implies a grammar that
|
||||
is nowhere stated; the `registry-file` store's record-file naming
|
||||
derives from the id (a slash in the id forces nested dirs or encoding —
|
||||
an on-disk layout decision that is freeze-adjacent per ADR-012 §2).
|
||||
|
||||
**Resolution**: grammar pinned in backend.md §"Repo-id grammar" —
|
||||
`owner/name` shape (lowercase alphanumerics + `-_`, ≤100 bytes, one
|
||||
segment allowed), rejection-only parsing (never decomposition —
|
||||
ADR-008 governs), error mapping (grammar-invalid = the collapsed
|
||||
denial on serving paths, `invalid` on op paths),
|
||||
percent-encoded flat record-file naming for `registry-file`, and an
|
||||
explicit note that the http door pins its own route grammar
|
||||
(`/{owner}/{repo}/…`) when the alkhttp `git` feature lands.
|
||||
|
||||
### R-6 [observation] — `already_exists` on create is an existence oracle to create-scope holders probing arbitrary ids
|
||||
|
||||
**Files**: backend.md §op error codes (the disclosure rationale reasons
|
||||
only about "the caller knows the id").
|
||||
|
||||
**Problem**: a `git:repo:create`-scoped identity can probe ids it
|
||||
didn't choose and learn which exist. Defensible (trusted, scope-gated,
|
||||
low-population surface; upstream gitea behaves the same), but the
|
||||
position was unstated — a later agent might "fix" the disclosure and
|
||||
break an upstream-compatible error clients rely on.
|
||||
|
||||
**Resolution**: the disclosure posture is recorded explicitly in
|
||||
backend.md (accepted: id-probing at create-scope is inside the trust
|
||||
boundary the scope grants; resolve-side disclosure stays collapsed per
|
||||
ADR-008/N-2) so the position survives personnel changes.
|
||||
|
||||
### R-7 [minor] — Lifecycle gate ambiguity: docs cannot reach `reviewed` while release-timing OQs are deferred by design
|
||||
|
||||
**Files**: docs/architecture/README.md §Document Lifecycle
|
||||
("→ `reviewed` when its OQs are resolved") vs open-questions.md (OQ-03
|
||||
partially resolved, OQ-05/OQ-16 deferred(scope) — the deferrals are
|
||||
correct and non-circular); review 001's remediation note ("the spec
|
||||
docs are candidates for `reviewed` status per the lifecycle
|
||||
definition" — which the letter of the definition forbids).
|
||||
|
||||
**Problem**: as written, the `draft → reviewed` transition requires
|
||||
resolved OQs, but the remaining OQs resolve only at first publish /
|
||||
ecosystem events. An implementer following the letter is blocked; one
|
||||
following the review's intent is not. Also caught in this pass: stale
|
||||
`last_updated` frontmatter (2026-09-25) on README/overview/
|
||||
open-questions/doors despite 09-29/30 edits.
|
||||
|
||||
**Resolution**: README's transition definition extended — docs reach
|
||||
`reviewed` when their OQs are resolved *or* every unresolved OQ is a
|
||||
properly-tracked deferral with a concrete non-circular blocker (which
|
||||
is the state review 001's own deferral-hygiene check certified).
|
||||
All four spec docs flipped to `reviewed` (this review is the gate);
|
||||
frontmatter timestamps refreshed.
|
||||
|
||||
### R-8 [cosmetic] — ADR-016 text glitches
|
||||
|
||||
**Files**: ADR-016 §2 ("freeze␣␣␣␣inventory" — collapsed multiple
|
||||
spaces), §4 (code span broken across a line break: `` `open_via_
|
||||
channels` `` renders as a broken identifier).
|
||||
|
||||
**Resolution**: both fixed; no semantic change.
|
||||
|
||||
## Summary Statistics
|
||||
|
||||
| Severity | Count | IDs |
|
||||
|----------|------:|-----|
|
||||
| Major | 2 | R-1, R-2 |
|
||||
| Minor | 4 | R-3, R-4, R-5, R-7 |
|
||||
| Observation | 1 | R-6 |
|
||||
| Cosmetic | 1 | R-8 |
|
||||
|
||||
All resolved in this cycle (ADR-018 + the backend.md/doors.md/README
|
||||
doc batch). No finding questions the architecture's direction; no
|
||||
criticals. The corpus is now fully pinned at every seam a decomposer
|
||||
cuts: session tuples, preamble wire shapes, op schemas, registry types,
|
||||
trait signatures, error models, concurrency model, and freeze-inventory
|
||||
entries.
|
||||
|
||||
## Verification (post-fix battery)
|
||||
|
||||
Re-run after the R-1..R-8 fixes land (see the commit series): four
|
||||
config compile probes, MSRV check + clippy, full test/clippy/fmt/doc
|
||||
+ publish dry-run — all expected clean (no code changed; docs-only +
|
||||
manifest-already-verified).
|
||||
|
||||
## References
|
||||
|
||||
- Review 001 (the gate this re-review verifies the remediation of) —
|
||||
`docs/reviews/001-architecture-pre-decomposition-review.md`
|
||||
- ADR-015/016/017 (review 001's resolution ADRs), ADR-018 (this
|
||||
review's R-1/R-2 resolution)
|
||||
- backend.md §"Pinned signatures", §"Registry types and schemas"
|
||||
(re-scoped), §"Repo-id grammar" (new), §op-surface notes
|
||||
- Phase-1 gate verdict: **decomposition may begin**
|
||||
Reference in new issue
Block a user