docs(platform): durable-delivery ACL provisioning design - #3
docs(platform): durable-delivery ACL provisioning design#3mattwilkinsonn wants to merge 7 commits into
Conversation
Design record for closing the cotal_acl_<space> provisioning gap: recommends bring-up-owned ACL provisioning (cotal up / provision-acl) plus spawn's provision-when-daemon-live completeness fix. Co-Authored-By: seal <noreply@sealedsecurity.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a design document describing durable-delivery ACL provisioning, recommending provisioning during ChangesDesign Document
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a design record for fixing the durable-delivery ACL provisioning gap — the missing
Confidence Score: 4/5The PR adds a design-only document with no code changes, so there is no runtime risk; however the document explicitly states it is not ready to be treated as a binding contract. The design record is thorough and the motivating analysis is well-grounded, but the preamble calls this a frozen contract while the body explicitly marks it unfrozen and declares a merge freeze-block until four open questions are resolved. docs/designs/platform/durable-delivery-acl-provisioning.md — the preamble's frozen contract claim needs to be reconciled with the explicit NOT frozen status and freeze-block in the Open Questions section. Important Files Changed
Reviews (7): Last reviewed commit: "docs(platform): fold design-critic pass ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/platform/durable-delivery-acl-provisioning.md`:
- Around line 94-99: Treat ACL provisioning as a required startup step, not a
log-and-continue task: in the `cotal up` flow after `postStart`, make the
idempotent ACL routine fail the bring-up when provisioning does not succeed, or
explicitly mark the process/status unhealthy instead of returning success. Keep
`cotal provision-acl` as the re-run entrypoint, but ensure the same underlying
routine used by `up` and the standalone command propagates failure so the broker
cannot appear healthy while ACLs are missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c530814d-f815-4f11-8678-f82ab5591be5
📒 Files selected for processing (1)
docs/designs/platform/durable-delivery-acl-provisioning.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
Drop the ephemeral "this session" anchoring (flagged by greptile P2 + cubic P3) so the durable design record is self-contained: the mint.ts zero-match search and the 0->8 self-heal evidence read as standing facts, not session-scoped observations. Co-Authored-By: seal <noreply@sealedsecurity.com>
Two freeze-prep clarifications from PR #3 review (mercator's supervisor rulings; substantive design forks stay open for Matt): - Soft-fail posture: mark the cotal-up auto-provision hook as the one deliberate, scoped exception to the 'No fallbacks' constraint (up orchestrates many agents; re-runnable; mirrors up.ts:284-285), with the standalone command staying hard-fail. Cross-referenced both sections so the spec isn't ambiguous (cubic P2, greptile P2). - 'Row is the whole gate' caveat: true only for the spawn/manager-provisioned population (the 7-agent evidence set already held dm/dlv durables); a mint+exec-omp agent has neither, so the provisioning step writes the full footprint (dm+dlv+acl). Corroborated by Codex on PR #4 and cubic's creds-only finding here; the DLV fix landed on PR #4 (f40ab37). Co-Authored-By: seal <noreply@sealedsecurity.com>
…aveat PR #3 re-review internal-consistency fixes (greptile 4/5 + cubic P2, both bot-only): - Task 2 step 3 said commitAcl-only, contradicting the scope caveat (a mint+exec-omp agent needs dm_<id> + dlv_<id> + the ACL row). Expand it to the full durable footprint (provisionDmInbox + provisionDlvInbox + commitAcl), all idempotent — matching what provisionAgent writes and what the DLV fix landed on PR #4 (f40ab37). Interfaces 'produces' line updated to match. - The fork's option (ii) claimed the standalone command covers a persona-file-less out-of-band agent 'via its creds file', but both paths enumerate listPersonas (agent-files only, no creds/ scan). Corrected to state it honestly as a known, deferred gap rather than false coverage. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… assertions Two review-accuracy fixes on the frozen contract (cubic on #3): - Step 3 said 'exactly what provisionAgent writes for a spawned agent', but a bare cotal spawn passes durableMembership:false and skips the ACL row (provision.ts:272). Clarify it's the durableMembership:true footprint, not the live-only path — so an implementer doesn't assume the row is already written. - Task 2 test cycle asserted only the ACL row (readAcl); a row-only impl would pass while still leaving dm_<id>/dlv_<id> absent → pumpDlv no-ops → still wake-blind. Add explicit dm/dlv durable-existence assertions (the checks that actually guard the incomplete-footprint bug the design identifies). Refs #3. Co-Authored-By: seal <noreply@sealedsecurity.com>
…rseable creds Two review findings on the reuse logic (cubic P1 + greptile P2 on #7): - Reuse now requires profile === "agent". The durable-ACL-orphan rationale is agent-specific (persona-refresh workflow + dm/dlv durables keyed to the id); observer/admin creds have neither, and silently preserving a privileged admin key across re-mints would extend its lifetime unexpectedly. Observer/admin now always rotate, as before. - A present-but-unparseable creds file (empty, truncated, not a user creds file) now fails loud with an actionable error naming --force, instead of letting identityFromCreds throw a raw parse exception. Silently rotating there would orphan the durable row the existing id may still own, so fresh-mint is not a safe fallback — the operator must opt in via --force or remove the stale file. Verified: agent re-mint keeps id; admin/observer re-mint rotate; corrupt creds at the out path errors naming --force (no raw crash, no silent rotate). Refs #3. Co-Authored-By: seal <noreply@sealedsecurity.com>
Layer 1 — packages/core/smoke/identity.smoke.ts (offline, 4 asserts): identityFromCreds
round-trips {id, seed} unchanged; agrees with idFromCreds (one-id-everywhere); rejects
spliced creds (seed vs foreign JWT subject) and a missing seed block.
Layer 2 — implementations/cli/smoke/mint-reuse.smoke.ts (hermetic, exercises the real
mint() against a tmp .cotal root, 8 checks): re-minting an agent reuses the SAME id;
--force rotates; a persona ACL change refreshes the baked sub.allow WITHOUT rotating the
id; first mint of a new name mints fresh (absent-creds path, no crash); observer + admin
re-mint rotate (reuse is agent-only); an unparseable existing creds file fails loud naming
--force (no silent rotate, no raw crash).
Red-green verified: reverting the reuse block to unconditional newIdentity() fails checks
1, 3, and 6 (two mints → two ids; the exact #3 cubic-P1 durable-orphan bug); the fix turns
them green. Registered both as smoke:identity + smoke:mint-reuse and wired into smoke:ci.
Refs #3.
Co-Authored-By: seal <noreply@sealedsecurity.com>
cubic P2 on #7: the corrupt-creds check only asserted the error message, so a future mint() refactor that wrote fresh creds *before* surfacing the parse error would still pass while silently rotating the id — the exact regression the test guards. Add a filesystem-state assertion: after the failed mint, the corrupt file must be byte-unchanged (still empty), proving no silent fresh-mint. Refs #3. Co-Authored-By: seal <noreply@sealedsecurity.com>
cubic P3 on #8: the ESC regression asserted the inbox drains on the next clean turn-end, but FakeHost never modeled the held nextTurn content actually being delivered into a turn — so it proved held-AND-acked, not held-AND-delivered (the drain came purely from the connector's surfaced bookkeeping). A future regression where the hidden queue never redelivers would still pass. Model OMP's deferred continuation (#promptQueuedHiddenNextTurnMessages, agent-session.ts:7323-7346) with FakeHost.flushHeld(): it drains the hidden next-turn queue into consumed turn content, exactly as a clean redelivery does. Test 9 now flushes the held message into a real turn and asserts it was consumed BEFORE the clean turn-end acks it — so the ack is proven to follow an actual delivery, closing cubic's gap and mirroring the real deliver-then-ack ordering. Refs #3. Co-Authored-By: seal <noreply@sealedsecurity.com>
…sage Capture the coalescing invariant at the ack site (greptile P1 on #8): a future reader touching ackSurfaced must see WHY a nextTurn message parked during an ESC unwind can't be acked before it's consumed. OMP holds the wire-level agent_end while #promptInFlightCount > 0 and lets a later one supersede it (agent-session.ts:2787-2799), so a wire-level subscriber sees one agent_end at the true settle — the interrupted turn and the deferred continuation collapse into a single post-consume event. Backstop: ackSurfaced drains only front-matching ids, so an unconsumed survivor redelivers (fails safe). Comment only, no behavior change. Refs #3. Co-Authored-By: seal <noreply@sealedsecurity.com>
Matt's calls (asked directly): spawn auto-detect via delivery lease (D1), cotal up mint-if-absent (D2) — folded from OQ3/OQ4 to Decisions. OQ2/OQ5 fold as explicit non-load-bearing deferrals (D3/D4). The (ii)-vs-core fork is NOT picked: Matt ruled it a core Cotal issue to deliberate before a CLI-layer patch. Filed SEA-1168. Open Questions now carries one explicit FREEZE BLOCKER pointing at it — record must not merge-freeze until SEA-1168 decides CLI-layer vs core fix. Refs SEA-1168 Co-Authored-By: seal <noreply@sealedsecurity.com>
Adversarial read-only critic pass (SEA-1188). Two clear improvements folded, three code-grounded forks surfaced as load-bearing Open Questions that compound the existing SEA-1168 freeze block. D1/D2 ratification text left untouched; their challenges surfaced as OQs for Matt's ruling. Forks (now OQ#2/#3/#4, blocking freeze): - OQ#2: enumerate creds/*.creds, not persona files. listPersonas scans .md only (personas.ts:33), but the target recipe `cotal mint --profile agent` writes only creds (mint.ts:88-92), so every plain-minted agent is invisible to both provisioning paths and stays wake-blind. The earlier "edge case" deferral is false against the code; it is the primary path. - OQ#3 (reopens D1): lease-conditional row-write strands spawn agents wake-blind on a daemon-restart race. The short-lived provisioner (prov.stop right after, spawn.ts:257) never backfills; no self-heal path exists. Alternative: always write (DEFER-inert, accretion already accepted by D4) or explicit --live-only. - OQ#4 (reopens D2): up-minting orphans the id-keyed footprint on every normal bring-up (fresh id per mint; operator's later `cotal mint` mints a different id, orphaning up's). Alternative: decouple identity-minting from bring-up. Folded improvements: - F4: the up soft-fail is not analogous to the daemon soft-fail (daemon self- heals via reconcile, provisioning does not) -> make residual blindness observable/actionable, not just logged. - F5: derive the committed ACL from the decoded JWT sub.allow (single source) and add a wildcard round-trip test; the shared helper alone doesn't close the creds-exists cross-matcher seam. - Task 3 grant corrected: STREAM.INFO on the delivery bucket is already granted via streamSetup; only MSG.GET was missing (record over-specified). Co-Authored-By: seal <noreply@sealedsecurity.com>
Design-critic pass (SEA-1188) — folded, 3 freeze-blocking forks for your callRan the mandated adversarial critic pass before freeze (read-only, grounded against the core + CLI source on this branch). Pushed the fold in
Folded without needing you:
Confirmed TRUE against source: Net: nothing here changes that SEA-1168 gates the freeze — but when it rules "CLI layer," OQ#2–#4 need answering too. All three are on the same single freeze gate. |
2 participants
Design record (design only, no implementation) for the durable-delivery ACL provisioning gap: the
cotal_acl_<space>read-ACL row is the keystone the delivery daemon re-authorizes durable @mention-wake deliveries against (absent ⇒ DEFER, never deliver), but neither of the launch paths agents actually use writes it —cotal mintis offline creds-only, andcotal spawnhard-codesdurableMembership: false. Live evidence: 0 rows across 7 connected agents; an out-of-bandcommitAclbackfill self-healed 0→8 durable memberships with no reconnect, proving the row is the whole gate.The record captures the 3-way fork (mint-online / bring-up-owned provisioning / mint
--commit-aclflag) and recommends (ii): one idempotent TS routine provisioning ACL rows for every persona-dir agent, auto-run at the tail ofcotal upand exposed as a re-runnablecotal provision-acl, keeping mint offline-pure — plus the completeness half: spawn switches to provision-when-daemon-live (delivery-lease probe) and its stale live-only rationale is rewritten. Plan carries global constraints (privileged writes only, row == mintedallowSubscribe, atomiccommitAclCAS, TS-only, red→green against a livenats-server) and per-task interfaces; five open questions are batched for the freeze gate.Co-Authored-By: seal noreply@sealedsecurity.com