oficina / docs Public

Explicit claude-fable-5 design APPROVE, no blockers, frozen e065b888bc9fef6cdcaaf2a0ca457d4923dd9d16. Both shared TryLock and TeamMode marker/release-floor amendments approved. A final clarification revision will pin response abort/client retry and empty local delivery-list behavior plus the nonblocking advisories before implementation. Full review:

APPROVE

I reviewed e065b88 against the inline Docs main sources, the approved principal design/implementation, the approved roster contract, and the frozen roster module at 667dc82. No blockers. Both explicit deltas are correct in direction and mechanically feasible. Findings by lens, then advisories.

The TryLock amendment (delta 1) — approved. The inversion is real and source-backed: publish, UpdateRecord and the generation-fence path in fetch all hold Cache.mu across Durable.Save, and Lease.Valid/Remaining take that same mutex. With Durable over Docs' single-connection write pool, an in-transaction lease check and a publishing refresh wait on each other. The fix is minimal and safety-preserving in the only direction that matters: a true from Valid still requires acquiring the lock and running the unchanged version/usable/deadline checks — and, importantly, observe() still runs on every successful acquisition, so the wall-clock fence is never bypassed on any path that answers true. Contention can only produce false/0, which routes to the existing retryable 503 with the save queue intact (the exact failure shape HasGrant's doc comment exists to protect). Rejecting released-lock persistence is right: publish currently relies on version/digest/deadline re-checks under the same lock that spans save; releasing it means a serial-persistence protocol plus compare/fence/republish — a change to the reviewed atomic-cache contract, not a Docs integration detail. The deterministic regression test (real writer held, refresh's Save genuinely waiting, lease calls in-transaction, mutation restoring blocking Lock must fail) is the right shape and will fail for the right reason. Grant-backed writes correctly bypass the lease entirely, so contention never blocks guests.

Mode marker + release floor (delta 2) — approved. The narrowing of the principal design's rollback paragraph is a genuine correction, not a contradiction: the old/new/old/new drill proved data compatibility, and the 3ed7665-lineage rollback argument addressed Home-only impersonation — but inline app.go/grants.go show that lineage grants any allowlisted local person full OwnerID scope with no roster, so a removed local member regains everything under rollback. The marker is honestly scoped (refuses new binaries booting modeless on an activated DB; explicitly cannot constrain old binaries; hence the mandatory binary floor), fails closed on malformed/unreadable, and pins bindings rather than generation, so legitimate generation changes don't trip it. The issuer census claim checks out against the R0.23 Session struct (no issuer field) — census + immutable marker + exact-issuer lookups is the correct closure.

Current-integration claims are source-accurate. Every "current main" assertion I could check against inline evidence holds exactly: ApplyUpdate's pre-replay check, out-of-band snapshot transaction, post-commit updated_at bump under owned(doc.OwnerID); readable/writable OwnerID inclusion; connect-time-only SSE authorization; the Home resolver's empty-address refusal; updatesPost's 404-on-ErrNotWritable. The remedies target each precisely, and moving the updated_at bump inside the transaction under the actor scope strictly tightens D4 rather than weakening it.

Nil-address, grant-only scope, standalone separation, streams — consistent with both binding contracts. Nil emitting no grant clause (rather than comparing empty) is the right defense against malformed historical grants and matches the principal contract verbatim. The single scope constructor with OwnerID disabled under NotMember/Unknown implements the roster contract's "no global has-any-grant gate" ruling and my carried-forward advisory. The Unknown-vs-NotMember response split (503 vs 404, no fabricated empty authoritative list) preserves both non-disclosure and the save queue. SSE per-event rechecks, deadline timer, re-arm-from-new-decision, and no manufactured grant timer match the module's own Lease comment; Remaining()==0 on contention degrades to the expiry path, which fails closed.

Advisories (non-blocking):

1. Pin updatesPost's status for definite NotMember. "Return 503 before updateIsClientFault classification" and "404 for known nonmembership" can both be read to apply to a fresh-snapshot-removed writer; the difference decides whether their queued batch is retained (503, retried forever by a permanently revoked client) or dropped (404). Both readings are commit-safe; decide one and test it before branch 3.
2. Add the existence-oracle test pair. The invariant is stated; the test isn't: in each standing state, a nonexistent sqid and an existing-inaccessible sqid must answer identically — especially both-503 under Unknown, where a 404/503 split would be an oracle.
3. Pin marker ordering against first-activation crash. Commit the marker before the listener serves any request, making "no Home-only principal ever exists on a marker-less DB" a theorem rather than a walkthrough (today the crash window leaves only inert schema, but nothing pins that).
4. Give the marker row the principal ledger's immutability triggers (refuse UPDATE/DELETE). Old binaries ignore the table and raw SQL is excluded by fiat, but future tooling isn't.
5. State the deadlock mechanism precisely in the amendment. With a context-honoring adapter the blocking-Lock cycle resolves at FetchTimeout as a repeated whole-writer stall; with any non-context-honoring wait — or UpdateRecord called with an unbounded ctx once runtime reload lands — it is permanent. TryLock is correct either way; the amendment's test ("return immediately") already asserts the right thing.
6. Land the unknown-kid nil-key guard (my prior module advisory 1) in the same amendment revision, since the module is being revised anyway.
7. Canary the contention rate deliberately. Publish's Save still waits ≤5s on the writer under edit load; sustained writer saturation can expire the cache and 503-storm members. The design names this; make the canary threshold explicit before branch 4.

The two absolutes hold end to end: a removed member's standing dies within the measured 60-second ceiling on every path including silent streams and in-flight transactions, and no contention, refusal, or uncertainty ever widens access — every ambiguous state resolves to refuse-and-retry with the editor's bytes intact. Implementation remains gated on the amended module's own CI and Fable code review before Docs pins it, per the design's own sequencing.

Final explicit claude-fable-5 design APPROVE, no blockers, frozen 75818ad99288a7411de0257c196828db2da322b4. Initial whole-design e065b88 approval also recorded above. No runtime implementation or deployment. Exact boundaries are in implementation/team-mode-integration.md. Carry three nonblocking implementation checks: count contention-caused mid-body aborts as well as 503s in canary; use exact Content-Length on all already-buffered guarded output and name any unbuffered exception; assert actual failed save-status indicator for definite 404 revocation. Artifacts /tmp/oficina-team-mode-design-review-final/final.txt and ci.json. Full final review:

APPROVE

I reviewed 75818ad as a delta against my approved e065b88, checking each changed requirement against the inline sources. No blockers. Every one of my seven prior advisories is landed, and each landing is correct against the evidence, not merely responsive. Findings per changed area, then advisories.

Response abort and cursor retention — correct and implementable. The client-side claims check out exactly: fetchLog/fetchSince gate on res.ok then await arrayBuffer() before any seq is consumed, and drain() advances cursor only after fetch and onUpdates succeed, with the outer catch leaving it untouched — so a rejected arrayBuffer yields retry-from-same-place, which is what the design asserts. The dangerous residual case is real and the design names it: a clean-EOF truncation at an update boundary would decode cleanly and let the header's Docs-Seq (computed from the full buffer before any write) drag the cursor past bytes never applied. The two defenses are the right ones — exact Content-Length whenever the full buffered length is known (updatesGet already computes total for the maxLogBytes cap, so it always is there), and mandatory real-Chromium/WebKit proof with activation blocked if abort degrades to clean EOF. http.ErrAbortHandler is the correct Go mechanism; "preserving its semantics through middleware" is the right implementation check to name. "Server must stop even when the client cannot retry" puts the security property above client convenience, correctly.

Empty TeamMode allowlist — correct, and it closes a hole. An empty nonnil set makes configured() refuse everyone while allows() still falls through to granted(), which is exactly "nobody implicit, grants still admit guests" against the inline auth.go. Applying it in development too removes the nil-addrs admit-everyone latitude from TeamMode entirely — good, since that latitude is precisely what the roster replaces. The exception to the startup-refusal rule is justified on its own terms: the refusal exists because in standalone, admit-nobody is a silent total lockout; in TeamMode the primary door is the Home handoff, so an empty local list is an honest state for a fresh Home-only team, and a fabricated seed email would be worse. Keeping mailer/instance-key production requirements preserves guest delivery without a logged-mail fallback.

404/503 split for updatesPost — pinned in the right direction. Against save-queue.mjs inline: 4xx drops and reports, 5xx requeues — so 404 for definite NotMember terminates the retry loop that a permanently revoked client would otherwise run forever, and 503 for Unknown/contended/storage retains the batch. The absolutes survive: only ambiguous states must retain bytes; definite refusal reporting 'failed' is not a widening and not a loss of the local CRDT state. Non-disclosure holds because 404 is already the existing not-writable shape, and the new oracle-pair test requirement makes the nonexistent/inaccessible identity checkable rather than asserted.

Marker ordering and immutability triggers — lands advisories 3 and 4 as stated. Commit-before-listener makes "no Home-only principal on a marker-less DB" a theorem. UPDATE/DELETE refusal triggers are the principal ledger's proven pattern; routing binding changes through a reviewed migration (which can drop/recreate triggers) rather than runtime rewrite is the correct escape valve.

Stall description, unknown-kid guard, canary thresholds — all correct. The FetchTimeout-repeated-stall vs unbounded-permanent distinction is precisely advisory 5. The unknown-kid map lookup before the verifier is wire-neutral and correctly scoped into the same amendment revision, which already carries its own CI and Fable code-review gate. The canary numbers are sane and falsifiable: 480 posts against ~2 lease refreshes in the window makes <1% contention plausible under TryLock, zero deadlocks is guaranteed by construction and the canary verifies it, and "initial engineering thresholds, revisable only by recorded review" is the right epistemic status for first numbers.

Advisories (non-blocking):

1. Count mid-body aborts in the canary's contention metric. Per-chunk nonblocking lease checks can abort a long response for a still-valid member purely on mutex contention with a refresh's Save. That fails closed and retries from an unchanged cursor — correct direction — but it is not a 503, so the "<1% retryable contention responses" threshold won't see it unless aborts are counted explicitly. Make the browser drive and canary count them.

1. State whether every guarded path is fully buffered. "Set exact Content-Length when the complete buffered length is known" implies some path where it isn't. If renderer output, JSON search, exports and updatesGet all buffer (the inline sources suggest they do), say so — then Content-Length is universal and the clean-EOF browser proof covers only genuinely exotic transports. If any guarded path streams unbuffered, the browser proof must include it by name.

1. Assert the user-visible 'failed' status in the 404-drop test. The design says the queue "drops/reports"; the test for definite-NotMember revocation should assert the report reaches the status indicator, since a silent drop would look identical to a green run in every server-side check.

The two absolutes hold through this revision: revocation still dies within the 60-second ceiling on every path — the per-chunk checks strengthen the late-emission edge — and no ambiguity widens access, with definite refusal now correctly terminal instead of masquerading as a retryable outage. Implementation remains gated exactly as before: the amended module's own CI and Fable code review precede the Docs pin, and activation stays blocked on the real-browser abort proof.