branch-followup: scarnecchia.net/ziran#5 — deferred non-blocking review feedback #1

Round-2 review amendments (same PR, fixes now in commit c5ee7cc + the follow-up hardening commit):

F1 amended — src/sync/spaces/response-verification.ts @ c5ee7cc (adversarial-review round 2, Blocker escalation): op payloads are not bound to the signed commit on any page — computeCommitMac HMACs only space‖repo‖rev‖commit.data, so (a) a commit-less page with any nonempty cursor passes as "intermediate" and (b) head-page ops can be edited while commit+MAC stay valid. Forged revs can poison processedRevs/repoCursors so future real ops are silently dropped (durable damage via src/sync/spaces/provider.ts #processRepoOps). Partial mitigation shipped: commit-less pages fail closed on missing/empty cursor (response-verification.ts), allRepoOps pagination is capped at 1000 pages (client.ts MAX_REPO_OPS_PAGES), and the repo cursor now advances on the MAC-verified head rev instead of raw op revs (provider.ts #pollSpace). Remaining core (needs a wire-shape/SDK decision, possibly upstream): fold verified op CIDs into a running RepoCommit and compare matches(commit.data) (pattern per @atproto/space repo-commit.ts) on full replays; the in-repo fake's RepoOp omits cid/prev so the real wire shape is unverified. (2026-10-24)

F6 — src/sync/spaces/client.ts:206-208,377-385 @ c5ee7cc (adversarial-review round 2, Low, non-blocking): a content-type downgrade on listRecords/listRepos yields silent empty results instead of an error. Why it matters: a broken or hostile server looks like "no records" rather than failing loudly. Suggested: throw on an unexpected content type for JSON endpoints. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 2. (2026-10-24)

F7 — src/sync/spaces/client.test.ts:187-203 @ c5ee7cc (adversarial-review round 2, Nit, non-blocking): getRecord tamper tests rebuild responses without the original MAC header, so they reject via missing-header rather than a wrong-MAC mismatch; only badRecordMacFetch and the getRepo tests pin body-vs-MAC binding. Why it matters: the tamper path those tests claim to pin is not the one exercised. Suggested: preserve the original header and corrupt the MAC value instead. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 2. (2026-10-24)

F8 — src/sync/spaces/response-verification.ts:86 @ c5ee7cc (adversarial-review round 2, Nit, non-blocking): on commit-less pages parseMacHeader runs as a shape check whose result is discarded. Why it matters: reader may mistake it for verification; it only rejects malformed headers. Suggested: rename intent via comment or void-binding, or drop it. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 2. (2026-10-24)

Round-3 review amendments (branch now at a40ce02 + the empty-rev fix commit; issues below are NEW or AMENDED — earlier items unchanged):

F2 amended — src/sync/spaces/provider.ts:398-399 @ a40ce02 (adversarial-review round 3, Medium, rebutted for this PR / tracked here): the poll loop's discovery rides the unverified listRepos answer — cursor === head.rev skips allRepoOps, and #writers/roster come from the same response. A forged or wrong authority answer can (a) suppress polling of a writer whose repo has new ops, (b) omit a writer from the roster, or (c) inject a bogus writer that reclassifies a healthy space as a persistent error via classifySpaceError. Liveness/censorship, not integrity: ops still bind at the MAC-verified head and the cursor advances only on verified revs. Rebuttal for this PR: the SDK defines MAC kinds only for record and repo reads (RECORD_MAC_TYPE/COMMIT_MAC_TYPE; prepareMacRequest supports exactly those two kinds), so verifying listRepos needs an upstream wire addition, not a client-side change. This issue is the tracking home; the PR documents the boundary. Adjacent scope note: adopt's entry.roles come from the unverified listRecords/op payloads (provider.ts:745-764) while the checkpoint itself is MAC-verified — same boundary. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 3. (2026-10-24)

F9 — src/sync/spaces/client.ts:358-372 @ a40ce02 (adversarial-review round 3, Low, non-blocking): the 1000-page cap bounds pages, not bytes — ops grows unbounded within the cap and op pages inline full record values, so a hostile or oversized repo PDS can force very large client allocations before the cap trips. Why it matters: memory-amplification DoS on the polling client. Suggested: byte budget per walk or streaming consumption. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 3. (2026-10-24)

F10 — src/sync/spaces/client.ts:364 + src/sync/spaces/provider.ts:306-315 @ a40ce02 (adversarial-review round 3, Low, non-blocking): a repo that always exceeds the page cap is re-walked in full every poll tick — #pollSpace throws, the loop applies normal hot/warm cadence (2s/30s) with no error backoff, so a hot doc against a hostile writer costs ~1000 credentialed requests every 2s (multiplied by retry loops). Why it matters: self-DoS amplification against a hostile or oversized repo. No state corruption (cursors untouched on throw). Suggested: back off #nextPoll on cap/verification failures. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 3. (2026-10-24)

FIXED in round 3 (for the closure ledger): getBlob JSON-content-type regression (binary gating + byte-exact JSON-MIME regression test, a40ce02); vacuous getRecord tamper tests now preserve the MAC header and pin per-case contracts (URI → identity error; CID/value → SpaceMacMismatch, a40ce02); isCommit rejects an empty commit rev, which previously persisted as a falsy cursor and forced full-history re-fetches every poll (follow-up hardening commit).

Round-4 review amendments (branch now at the KEM-cache commit; issues below are NEW or AMENDED — earlier items unchanged):

F11 — src/sync/spaces/provider.ts:397-416 @ HEAD (adversarial-review round 4, Low, non-blocking; pre-existing loop structure): one bad head aborts #pollSpace for the whole space — any per-head throw (hard verification failures, the 1000-page cap, or a head whose server cannot sign a commit) skips the rosterChanged persist and all remaining heads, so a single hostile or broken writer repo freezes every other writer's op ingestion for that space and pins a permanent error. Fails closed (no corruption). Why it matters: availability lever over whole-space sync. Suggested: per-head try/catch; classify, continue, persist what advanced. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 4. (2026-10-24)

F12 — src/sync/spaces/response-verification.ts:92,149 @ HEAD (adversarial-review round 4, Nit, non-blocking): liveness checks accept whitespace cursors and whitespace/"0" revs (only empty strings rejected); no rev-format validation exists client- or SDK-side. Why it matters: not a bypass (the MAC covers rev), but signed garbage degrades the cursor skip gate into permanent re-polling. Suggested: require a TID-parseable rev / non-whitespace cursor. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 4. (2026-10-24)

F13 — src/sync/spaces/provider.ts:794-813 @ HEAD (adversarial-review round 4, Medium, pre-existing code — out of this PR's diff): adopt() seeds processedRevs/deliveredRevs/ackedRevs wholesale from the winning checkpoint's frontier and picks the winner by raw String(createdAt) comparison. MAC authenticity proves the checkpoint came from member X, but X's claims about OTHER writers' watermarks are never validated: any member can publish a checkpoint setting a victim author's rev to a far-future TID, and every future adopter then permanently skips that author's ops (silent, persistent censorship); the attacker can keep winning the createdAt race. Why it matters: authentic-but-unauthorized data flows straight into sync watermarks. Suggested: trust only frontier[checkpointAuthor] verbatim; clamp foreign entries to each repo's MAC-verified head rev, or ignore foreign entries and let the poller catch up (needs care around re-delivery semantics). Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 4. (2026-10-24)

FIXED in round 4 (closure ledger): per-request DID resolution amplified by page count — SpaceClient now caches the resolved KEM key share per repo for 60s, MAC-mismatch retries drop the entry so rotation re-resolves on the one retry (rotation and repeated-mismatch tests still green; new test pins one KEM resolution across paginated requests).

Round-5 review amendments (final cycle; branch at 0c3efff):

FIXED in round 5 (closure ledger): stale prepareMacRequest type reference left client.ts unclean under deno check after the KEM-cache split — #credCall now uses the exported PreparedMacRequest type (commit 0c3efff). Caught by the round-5 standard reviewer; suites and deno check green after.

F14 — src/sync/spaces/provider.ts:299-316 @ 0c3efff (adversarial-review round 5, Low, non-blocking): removing auto-forget of deleted spaces leaves a permanent per-cycle SpaceDeleted error behind the single global #error slot whose end-of-cycle value depends on space iteration order. Why it matters: the surfaced error may name an arbitrary space rather than the first/important failure. Suggested: per-space error state or bounded auto-forget. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 5. (2026-10-24)

F15 — src/sync/spaces/client.ts:476-479 @ 0c3efff (adversarial-review round 5, Low, non-blocking): isMacRefreshError depends on the server error name BadSpaceMac for undecapsulable key shares; the fake uses that name, real upstream PDS shape unverified. Why it matters: name drift costs ≤60s of failed reads per TTL window (self-healing via the KEM cache TTL). Suggested: broaden the match or pin the upstream name with a test. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 5. (2026-10-24)

F16 — src/sync/spaces/client.ts:152-163 @ 0c3efff (adversarial-review round 5, Nit, non-blocking): #kemShares never evicts and has no in-flight dedup — same profile as the existing #creds/#pdsCache caches. Suggested: shared eviction policy across the three caches. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 5. (2026-10-24)

F17 — src/sync/spaces/session-fetch.ts:13 @ 0c3efff (adversarial-review round 5, Nit, non-blocking): startsWith(base) is a prefix match, not an origin match; latent today since requests go to the honest PDS. Suggested: compare origins. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 5. (2026-10-24)

F18 — rawCid duplicated in client.ts / fake.ts / spaces-spike.ts @ 0c3efff (adversarial-review round 5, Nit, non-blocking): triplicated helper risks drift between test and production CID extraction. Suggested: export one copy and import it everywhere. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 5. (2026-10-24)

Reviewer nuance worth recording under F13's scope: adopt's checkpoint pick races on claimed createdAt across members — arguably broader than the frontier-watermark wording of F13; any F13 fix should cover winner selection too.

src/sync/spaces/httpsig.vectors.json:6,25 @ 456fb27 — F6 (adversarial-review, non-blocking): the fixture still carries pre-alpha wire strings ("Atproto-Space credential", untagged atproto-space signature-input label, a Bearer delegation entry) and no test consumes it (repo-wide grep for vectors finds no loader; the live wire vectors are inline in httpsig.test.ts). Why it matters: after the scheme rename (901630a), a stale reference fixture invites copying the rejected scheme back into the client. Suggested: delete the file, or regenerate its vectors from the current SDK and add a consumer test. Source: https://tangled.org/scarnecchia.net/ziran/pulls/5 — review round 4. (2026-10-25)