docs(server): design gateway_credentials at-rest encryption + server_secrets store (RIG-2863) - #859
Open
rigel-mintaka wants to merge 16 commits into
Open
docs(server): design gateway_credentials at-rest encryption + server_secrets store (RIG-2863)#859rigel-mintaka wants to merge 16 commits into
rigel-mintaka wants to merge 16 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-server-rig-2863-cred.compass-eng-docs.pages.dev Deployed from Changed pages: |
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…-2863) Round-1 review of #859 found 2 high + 4 medium, all folded (no Matt ruling relitigated): - **F1 (high)** — corrected the false "user SetSecret path physically CANNOT reach the master key" claim: C1 splits the declaration registries, NOT the shared SecretSpec provider keyspace, so the reserved-name guard is now mandatory on BOTH the admin RPC and the authenticatedOpen user SetSecret/DeleteSecret path. Added the user-path red-green test and a boot key-fingerprint tripwire (T5) so a swapped provider value is detected, not silently adopted. - **F2 (high)** — replaced T0's non-executable in-migration DML row-move with a boot-time config-driven reconcile (server-secret names are per-deployment flag/env config the embedded migration cannot know), and enumerated all six names (primary + reviewer App PEM, webhook, three Linear secrets). - **F3 (medium)** — added the mandatory GRANT to compass_app/compass_system (0001's ON ALL TABLES snapshot does not cover later migrations) and a stated bucket-A RLS decision (Matt-ruled: server_secrets is deployment-global infra, no tenant_id, RLS off, added to the rls_pgtest bucketA allow-list). - **F4 (medium)** — OQ-3 resolved to D7 (Matt: api_key rows encrypted identically to OAuth rows, one seal/open path). - **F5 (medium)** — re-anchored four drifted 0001_init.sql citations (post-RIG-3106 line shift). - **F6 (medium)** — D1 column names aligned to T3/ledger (value_ciphertext/value_nonce). - **F7/F8 (low)** — added the RIG-2863 tracker ref; named the C2 residual risk concretely. Follow-up RIG-3237 filed for per-tenant at-rest isolation + gateway-topology exposure. Spec-impact: none (design record). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…-2863) Round-1 review of #859 found 2 high + 4 medium, all folded (no Matt ruling relitigated): - **F1 (high)** — corrected the false "user SetSecret path physically CANNOT reach the master key" claim: C1 splits the declaration registries, NOT the shared SecretSpec provider keyspace, so the reserved-name guard is now mandatory on BOTH the admin RPC and the authenticatedOpen user SetSecret/DeleteSecret path. Added the user-path red-green test and a boot key-fingerprint tripwire (T5) so a swapped provider value is detected, not silently adopted. - **F2 (high)** — replaced T0's non-executable in-migration DML row-move with a boot-time config-driven reconcile (server-secret names are per-deployment flag/env config the embedded migration cannot know), and enumerated all six names (primary + reviewer App PEM, webhook, three Linear secrets). - **F3 (medium)** — added the mandatory GRANT to compass_app/compass_system (0001's ON ALL TABLES snapshot does not cover later migrations) and a stated bucket-A RLS decision (Matt-ruled: server_secrets is deployment-global infra, no tenant_id, RLS off, added to the rls_pgtest bucketA allow-list). - **F4 (medium)** — OQ-3 resolved to D7 (Matt: api_key rows encrypted identically to OAuth rows, one seal/open path). - **F5 (medium)** — re-anchored four drifted 0001_init.sql citations (post-RIG-3106 line shift). - **F6 (medium)** — D1 column names aligned to T3/ledger (value_ciphertext/value_nonce). - **F7/F8 (low)** — added the RIG-2863 tracker ref; named the C2 residual risk concretely. Follow-up RIG-3237 filed for per-tenant at-rest isolation + gateway-topology exposure. Spec-impact: none (design record). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…(RIG-2863) Round-2 review of #859 returned 0 high / 3 medium / 3 low. The round-1 body fixes all held; the blockers were that the fix commit refreshed the record body but not the terse `## Tasks` checklist, plus one new defect the F2 fix introduced. - R2-1/R2-2 (medium): rewrite the T0 + T2 `## Tasks` checklist entries to match the fixed body — T2 no longer says the reserved-name guard is admin-RPC-only, and T0 now names the boot-time config-driven reconcile (not in-migration DML), all six server-secret names, the GRANT, and the bucketA allow-list edit. - R2-3 (medium, new defect): the F2 fix relocated the row move from a pre-policy migration to a post-policy boot reconcile, where the source `secrets` table is FORCE RLS and the ordinary tenant-scoped store path sees only the bootstrap tenant. Specify the reconcile runs under `store.WithSystemRole` (BYPASSRLS) — a fifth WithSystemRole entrypoint that extends DL-315's "only at the four named entrypoints" clause — and add the two-tenant reconcile red-green test. - R2-4 (low): add the key-swap fingerprint tripwire to the T5 checklist entry. - R2-5/R2-6 (low): correct two citation anchors added by the round-1 fixes (`rls_pgtest_test.go:544`->`:543`; `secrets_service.go:214`->`:208`). Additive review-fix commit atop the round-1 fix; no ledger delta (DL-324 is unchanged — the reconcile execution context is a T0 mechanism detail, ratified at the PR gate). Ledger-impact: none Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-2863-creds-encryption-design
branch
from
September 4, 2026 18:04
58b972e to
d62e9ad
Compare
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…-2863) Round-1 review of #859 found 2 high + 4 medium, all folded (no Matt ruling relitigated): - **F1 (high)** — corrected the false "user SetSecret path physically CANNOT reach the master key" claim: C1 splits the declaration registries, NOT the shared SecretSpec provider keyspace, so the reserved-name guard is now mandatory on BOTH the admin RPC and the authenticatedOpen user SetSecret/DeleteSecret path. Added the user-path red-green test and a boot key-fingerprint tripwire (T5) so a swapped provider value is detected, not silently adopted. - **F2 (high)** — replaced T0's non-executable in-migration DML row-move with a boot-time config-driven reconcile (server-secret names are per-deployment flag/env config the embedded migration cannot know), and enumerated all six names (primary + reviewer App PEM, webhook, three Linear secrets). - **F3 (medium)** — added the mandatory GRANT to compass_app/compass_system (0001's ON ALL TABLES snapshot does not cover later migrations) and a stated bucket-A RLS decision (Matt-ruled: server_secrets is deployment-global infra, no tenant_id, RLS off, added to the rls_pgtest bucketA allow-list). - **F4 (medium)** — OQ-3 resolved to D7 (Matt: api_key rows encrypted identically to OAuth rows, one seal/open path). - **F5 (medium)** — re-anchored four drifted 0001_init.sql citations (post-RIG-3106 line shift). - **F6 (medium)** — D1 column names aligned to T3/ledger (value_ciphertext/value_nonce). - **F7/F8 (low)** — added the RIG-2863 tracker ref; named the C2 residual risk concretely. Follow-up RIG-3237 filed for per-tenant at-rest isolation + gateway-topology exposure. Spec-impact: none (design record). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…(RIG-2863) Round-2 review of #859 returned 0 high / 3 medium / 3 low. The round-1 body fixes all held; the blockers were that the fix commit refreshed the record body but not the terse `## Tasks` checklist, plus one new defect the F2 fix introduced. - R2-1/R2-2 (medium): rewrite the T0 + T2 `## Tasks` checklist entries to match the fixed body — T2 no longer says the reserved-name guard is admin-RPC-only, and T0 now names the boot-time config-driven reconcile (not in-migration DML), all six server-secret names, the GRANT, and the bucketA allow-list edit. - R2-3 (medium, new defect): the F2 fix relocated the row move from a pre-policy migration to a post-policy boot reconcile, where the source `secrets` table is FORCE RLS and the ordinary tenant-scoped store path sees only the bootstrap tenant. Specify the reconcile runs under `store.WithSystemRole` (BYPASSRLS) — a fifth WithSystemRole entrypoint that extends DL-315's "only at the four named entrypoints" clause — and add the two-tenant reconcile red-green test. - R2-4 (low): add the key-swap fingerprint tripwire to the T5 checklist entry. - R2-5/R2-6 (low): correct two citation anchors added by the round-1 fixes (`rls_pgtest_test.go:544`->`:543`; `secrets_service.go:214`->`:208`). Additive review-fix commit atop the round-1 fix; no ledger delta (DL-324 is unchanged — the reconcile execution context is a T0 mechanism detail, ratified at the PR gate). Ledger-impact: none Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-2863-creds-encryption-design
branch
from
September 4, 2026 19:27
d62e9ad to
d6f2eee
Compare
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…863) Review of #868 returned 0 high / 1 medium / 2 low. Citations (16/16) and the SubjectService ledger row were a clean pass; the blocker was executor-contract coherence. - F1 (medium): the Plan framed all five tasks as one "PR2 executor contract", but T4 (the service-door mount) targets a Server surface that does not exist in the tree yet, and no corpus task owns minting a SubjectService token — so the class as written ships unreachable. Reframed the Plan into two scopes: the enum half (T1/T2/T3/T5) lands now as PR2 (the urgent half — enum number + CHECK are painful to change once token rows exist); the door half (T4) lands WITH the RIG-2863 (RIG-1715 T2) service surface it mounts on, ordered after it. Added OQ-3 naming where token issuance lives (the RIG-2863 T4 slice, an IssueServiceToken mirroring MintRunnerToken) so an executor never improvises a mint path on a security-critical door. - F2 (low): citation span `lines 348-350` -> `348-351` (the quoted "disable)" falls on 351). - F3 (low, pre-existing): noted that `runnerhub.RunnerTokenRegistered` is the one token-store consumer outside the kind-gated door contract — flagged for the T4 executor to kind-check or document, out of scope for this record. Ledger-collision renumber: main's #870 (RIG-3096) merged its own DL-324 (forge live-test app-actor) after this stack forked, and the sibling encryption PR (#859) took DL-325/DL-326. Renumbered this record's SubjectService row to the next free id, DL-327 (was DL-325), leaving main's DL-324 untouched. Additive review-fix commit. Ledger-impact: DL-327 (SubjectService, renumbered from DL-325) Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…sign (RIG-3238) ReviewDesign869r2 verified all 7 round-1 findings CLOSED but caught two mediums the round-1 fold's own new prose introduced. Both are mechanical text corrections with no design consequence. - MEDIUM (guard-bullet facts): the ID-allocation/freeze-order Global Constraint bullet had wrong PR->DL attributions on every clause. Ground truth (verified via gh pr diff + main's ledger): main's tail ends at DL-324 (landed by #870, merged); DL-325 is claimed by #804 (RIG-3070, draft) AND #859 (RIG-2863); DL-326 -- this record's own number -- is claimed by #859, which is review-ready (NOT draft). The bullet missed the real live collision (#859 on DL-326) and named #868 (which actually claims DL-327). Rewrote the factual clauses to match; kept the two correct load-bearing clauses (the MUST-re-grep-before-landing instruction and does-NOT-depend-on-#804). The immutable DL-326 cell was already merge-order-safe (name+issue cite), so this only corrected mutable prose. - MEDIUM (unknown-count consistency): round-1 widened the ledger cell from five to six unknowns (adding runner-on-darwin / OQ-12) but left three "five" sites in the record body: the Why-STAGED enumeration, the reject-default alternative, and the T-1 task heading. Aligned all three to six + OQ-12, and aligned the threat classification (OQ-1, OQ-2, and OQ-12 threaten adoption) with the cell's widened re-open trigger. - LOW: re-wrapped two range-overwrite orphan lines to the file's ~72-col width. Checks: markdownlint 0 errors; design-ledger-gate OK (291 rows, 122 headers). Status stays Draft -- freezes Active on merge. Refs RIG-3238 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…secrets store (RIG-2863) Addendum to the frozen `compass-server-llm-gateway` record: encryption-at-rest for the `gateway_credentials` value store, master-key custody, and a physically separate `server_secrets` store for server-only secrets. Decisions (all Matt-ruled): - **D1** application-layer AES-256-GCM envelope encryption; ciphertext+nonce+ key_version columns, no plaintext value column ever; fresh 96-bit nonce per write (2^32-per-key bound quantified against the OAuth-refresh write rate). - **D2** 256-bit master key auto-provisioned zero-human-step into the ONE configured SecretSpec provider under a Postgres advisory lock with an every-boot read-back verify; key never touches the DB. - **D3** managed-plane KMS is a provider-URI deployment config, not a code fork. - **D4** `key_version` column from day one (cheap now, expensive retrofit). - **D5** redaction beyond fmt verbs (unexported key bytes / slog.LogValuer + MarshalJSON), never the RPC serialization type. - **D6 (mechanism C1)** server-only secrets get a NEW physically separate `server_secrets` store — a names-only table + a SECOND SpecResolver instance over it (shared profile) + an admin-gated SetServerSecret/DeleteServerSecret RPC. The delivery boundary is the resolver's manifest, so a server secret is STRUCTURALLY undeliverable to agent containers (default-closed by construction). Removes the public proto change the earlier SERVER_ONLY-delivery-kind fold required; the container FetchSecrets path is byte-for-byte unchanged. The PEM/webhook/Linear server secrets — server_only by convention only today, riding the inject-all path into every agent container — move into `server_secrets` in this PR chain (a DB-row move, provider values untouched), closing that pre-existing exposure (OQ-4, Matt-ruled: same PR chain). Tasks: T0 server_secrets store (prerequisite) → T1 envelope pkg → T2 boot provision seam → T3 ciphertext columns → T4 seal/open wiring → T5 tripwire + redaction assertions. Open sub-fork flagged for review: the server-provisioned master key's `declared_by` FK is specced NULLABLE (NULL = server-provisioned; honest provenance, no boot-ordering coupling) rather than attributed to the bootstrap-admin account or an @Compass sentinel — a one-line T0 change either way with no downstream effect. Ledger: DL-324 (Storage). Spec-impact: adds `server_secrets` table + SetServerSecret/DeleteServerSecret admin RPC + gateway_credentials ciphertext columns to the RIG-2863 build. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…-2863) Round-1 review of #859 found 2 high + 4 medium, all folded (no Matt ruling relitigated): - **F1 (high)** — corrected the false "user SetSecret path physically CANNOT reach the master key" claim: C1 splits the declaration registries, NOT the shared SecretSpec provider keyspace, so the reserved-name guard is now mandatory on BOTH the admin RPC and the authenticatedOpen user SetSecret/DeleteSecret path. Added the user-path red-green test and a boot key-fingerprint tripwire (T5) so a swapped provider value is detected, not silently adopted. - **F2 (high)** — replaced T0's non-executable in-migration DML row-move with a boot-time config-driven reconcile (server-secret names are per-deployment flag/env config the embedded migration cannot know), and enumerated all six names (primary + reviewer App PEM, webhook, three Linear secrets). - **F3 (medium)** — added the mandatory GRANT to compass_app/compass_system (0001's ON ALL TABLES snapshot does not cover later migrations) and a stated bucket-A RLS decision (Matt-ruled: server_secrets is deployment-global infra, no tenant_id, RLS off, added to the rls_pgtest bucketA allow-list). - **F4 (medium)** — OQ-3 resolved to D7 (Matt: api_key rows encrypted identically to OAuth rows, one seal/open path). - **F5 (medium)** — re-anchored four drifted 0001_init.sql citations (post-RIG-3106 line shift). - **F6 (medium)** — D1 column names aligned to T3/ledger (value_ciphertext/value_nonce). - **F7/F8 (low)** — added the RIG-2863 tracker ref; named the C2 residual risk concretely. Follow-up RIG-3237 filed for per-tenant at-rest isolation + gateway-topology exposure. Spec-impact: none (design record). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-2 review of #859 returned 0 high / 3 medium / 3 low. The round-1 body fixes all held; the blockers were that the fix commit refreshed the record body but not the terse `## Tasks` checklist, plus one new defect the F2 fix introduced. - R2-1/R2-2 (medium): rewrite the T0 + T2 `## Tasks` checklist entries to match the fixed body — T2 no longer says the reserved-name guard is admin-RPC-only, and T0 now names the boot-time config-driven reconcile (not in-migration DML), all six server-secret names, the GRANT, and the bucketA allow-list edit. - R2-3 (medium, new defect): the F2 fix relocated the row move from a pre-policy migration to a post-policy boot reconcile, where the source `secrets` table is FORCE RLS and the ordinary tenant-scoped store path sees only the bootstrap tenant. Specify the reconcile runs under `store.WithSystemRole` (BYPASSRLS) — a fifth WithSystemRole entrypoint that extends DL-315's "only at the four named entrypoints" clause — and add the two-tenant reconcile red-green test. - R2-4 (low): add the key-swap fingerprint tripwire to the T5 checklist entry. - R2-5/R2-6 (low): correct two citation anchors added by the round-1 fixes (`rls_pgtest_test.go:544`->`:543`; `secrets_service.go:214`->`:208`). Additive review-fix commit atop the round-1 fix; no ledger delta (DL-324 is unchanged — the reconcile execution context is a T0 mechanism detail, ratified at the PR gate). Ledger-impact: none Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-3 review returned 0 high / 4 medium / 3 low. All six round-2 fixes held; the blockers were three new grounded defects in round-2's reconcile-execution- context paragraph (the WithSystemRole ruling itself is correct and stands) plus one ledger-recording judgment call Matt ruled on. - M1 (medium): the paragraph justified WithSystemRole by claiming FORCE RLS binds even the owner at boot. False — the owner connection is a superuser and a superuser bypasses even FORCE (0001_init.sql:848-851; rls_pgtest_test.go: 282-284). Restated on the true, superuser-independent reason: the reconcile takes the compass_app path, a non-owner non-BYPASSRLS role confined to the bootstrap tenant, so a row under another tenant is silently skipped without WithSystemRole. - M2 (medium): the prescribed two-tenant same-name test is unconstructible — `secrets.name` is a global PRIMARY KEY (0001_init.sql:396), so at most one row per configured name exists deployment-wide. Reframed the motivation and the test to the real case: a row declared under a NON-bootstrap tenant. - M3 (medium): corrected the "fifth entrypoint" miscount — the shipped tree arms WithSystemRole at THREE call sites (consumer.go:316, hub.go:753/814), and hub.go:814 (forge-notification-ack) is already a BYPASSRLS site DL-315's four names omit. Enumeration restated against the shipped set. - L1/L2/L3 (low): cite the arming code (armQueue/beginTenantTx) not just the WithSystemRole entrypoint; quote DL-315 verbatim; move the reconcile test into T0's Tests enumeration. - M4 -> OQ-5 (Matt-ruled: a new ledger row): the DL-315 BYPASSRLS allow-list widening lands as DL-326 (Refines DL-315, which stays Active), restating the true shipped entrypoint set incl. the previously-unledgered forge-notification- ack arm. Discoverability at the surface where the BYPASSRLS-audit question gets asked, over prose buried in an at-rest-encryption record. Ledger-collision renumber: main's #870 (RIG-3096) merged its own DL-324 (forge live-test app-actor) after this stack forked. Renumbered this addendum's ledger rows to the next free sequential ids — the `gateway_credentials` encryption decision is DL-325 (was DL-324), the BYPASSRLS allow-list widening stays DL-326, and the sibling SubjectService PR (#868) takes DL-327 — leaving main's DL-324 untouched. Additive review-fix commit atop the round-2 fix. Ledger-impact: DL-325 (encryption, renumbered from DL-324), DL-326 (new, Refines DL-315) Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-4 review (0 focus-area regressions; DL-326 entrypoints, renumber integrity, and R3 M1/M2/M3 all verified holding) surfaced one high + two mediums + two lows, all against the T0 prerequisite that OQ-5's ledgering made load-bearing this round. All findings verified against the tree at main `8fef2846` before folding; none is a design fork. - **H1 (T0 consumer-repoint):** the reconcile moves the six server-secret NAMES out of `secrets`, but the record pinned the container resolver (serve.go:528) as UNCHANGED and never repointed the boot consumers that resolve those names through it. As written that hard-fails boot for every GitHub-App deployment (`validateForgeSecret` → `forge secret %q not declared`) and silently unmounts the Linear webhook + write/notify lanes (`forgeSecretDeclared`/`buildLinearTokenSource` treat an absent name as a legitimate off-state). Added an explicit consumer-repoint deliverable (thread the SERVER resolver into buildForgeReadWiring/buildLinearWebhookWiring/ buildLinearTokenSource/wireForgeWriteCaller/buildForgeWriteService + the newDeclaredSecretResolver/newCachedWebhookSecret closures), pinned the reconcile-before-forge-wiring ordering, corrected the resolver-UNCHANGED sentence, and added positive forge-lane-wiring assertions to T0's test list (the silent modes need a positive assert, since absent is indistinguishable from a legitimate off-state). - **M1 (F1 guard scope):** widened the user-path guard from a reserved-name/ prefix match to a `server_secrets` MEMBERSHIP test on SetSecret/DeleteSecret (and DeclareSecret at the store door) — the five non-master names are arbitrary per-deployment config with no reserved prefix, so a reserved-name- only guard leaves the `authenticatedOpen` SetSecret path able to mint a shadow `secrets` row under a server-secret name and get it container- delivered. Corrected the C1 "structurally undeliverable" claim to state the guard, not the table split, is what makes undeliverability total; updated the C2 named residual to cover all six names. - **M2 (T2 bounded critical section):** the advisory-lock critical section had no time bound on the provider round-trips, so a hung provider holds a shared-constant lock indefinitely and wedges every booting instance. Added a bounded-ctx + try-lock/lock_timeout requirement mirroring the 30s serve.go:1731 precedent, plus the corresponding fleet-wedge test. - **L1:** package-qualified + symbol-anchored the BYPASSRLS entrypoint citations in the record body and the DL-326 ledger row (`go/internal/ runnerhub/hub.go` `forgeNotificationAck` etc.), since a bare `hub.go:814` drifts and defeats the discoverability the OQ-5 row exists for. - **L2:** fixed two citation slips in the reconcile-execution-context paragraph (`compass_system` CREATE ROLE is 0001_init.sql:879 not :882-887; the arming code is in `armTx` :199-215, shared by `beginTenantTx` :185-195, not "beginTenantTx :202-204"). Ledger-impact: DL-325, DL-326 (DL-326 citation symbol-anchored; both rows' record links now carry a resolving #anchor, required since the fold grew the record past the 50 KB design-ledger-gate threshold) Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-5 adversarial review of PR #859 returned 2H/3M/2L; all seven verified against the tree at main `8fef2846` before folding. None is a design fork — each corrects a plan-executability or internal-consistency defect that honors an invariant the record already asserts. Additive commit atop the bookmark. - **H1 (silent forge-write capability loss):** the T0 failure-mode taxonomy was two modes ((a) hard boot fail, (b) silent Linear degradation) but a THIRD exists that the reviewer-PEM `validateForgeSecret` (serve.go:1638) hides behind. `wireForgeWriteCaller` (:587) gates on `forgeWritesEnabled` (:1583); with both App PEM names moved out of the container resolver the predicate reads both-absent, `warnPartialForgeWriteSecrets` returns silently (:1826-1827), the write caller is never mounted, and :1638 is unreachable — agent forge WRITES fail-closed fleet-wide with no boot error. Split the taxonomy into three modes, moved :1638 into mode (c), and added the missing T0 positive assertion (forge WRITE caller MOUNTED after one reconcile pass). - **H2 (uncancellable Resolve wedge):** the T2 bounded-critical-section fix prescribed a ctx timeout, but `SpecResolver.Resolve` threads ctx only into `DeclaredSecrets` (resolver.go:136); the provider round-trip is `b.Load()` (resolver.go:165), whose SDK signature carries no ctx (secretspec-go v0.15.0 secretspec.go:245) and blocks in an uncancellable FFI call. State the Set(bounded)/Resolve(unbounded) asymmetry and prescribe the mechanism that works: run the boot resolve on its own goroutine + select on the derived ctx, so the provisioner returns a bounded error and releases the xact lock while the orphaned FFI goroutine is knowingly leaked. Restated the T2 test. - **M1 (reconcile skips default-named Linear pair):** pinned the reconcile name source to `cfg.Forge.resolved()` (serve.go:232-246), not the raw flag/env layer — two Linear names carry code defaults (serve.go:202-203) applied after that layer, so a reconcile over `main.go` flags sees `""` and silently skips them, leaving them on the inject-all path. Added a T0 test. - **M2 (buildDoors resolver-fork hazard):** named the `buildDoors` fork explicitly — its single `resolver` param (serve.go:677) feeds both `buildLinearWebhookWiring` (:788, MUST repoint) and `buildNetworkServer` (:803) -> `runnerhub.NewMountedHandler` (network_door.go:313, the container FetchSecrets path that MUST NOT). So `buildDoors` takes BOTH resolvers; swapping its one argument would invert D6. - **M3 (dangling D7 reference):** DL-325 and OQ-3 cited a decision "D7" the record never defined. Promoted the api_key/OAuth uniform-treatment ruling into a real `### D7` heading in the Approach section; OQ-3 and DL-325 now resolve. - **L1:** defined the reserved prefix `GATEWAY_CREDENTIALS_` inline at its three uses (was undefined). - **L2:** caveated the header "structurally undeliverable" claim with the F1 guard dependency the D6 body already states (the split alone is necessary but not sufficient). markdownlint clean; design-ledger-gate OK (293 rows, 123 headers); no dup DL ids. Record grew to 59547 B; DL-325/DL-326 anchors already present. Ledger-impact: DL-325, DL-326 (text unchanged; D7 referent now exists) Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-2863-creds-encryption-design
branch
from
September 4, 2026 21:08
9ee2a9e to
899aabc
Compare
rigel-mintaka
added a commit
that referenced
this pull request
Sep 4, 2026
…863) Review of #868 returned 0 high / 1 medium / 2 low. Citations (16/16) and the SubjectService ledger row were a clean pass; the blocker was executor-contract coherence. - F1 (medium): the Plan framed all five tasks as one "PR2 executor contract", but T4 (the service-door mount) targets a Server surface that does not exist in the tree yet, and no corpus task owns minting a SubjectService token — so the class as written ships unreachable. Reframed the Plan into two scopes: the enum half (T1/T2/T3/T5) lands now as PR2 (the urgent half — enum number + CHECK are painful to change once token rows exist); the door half (T4) lands WITH the RIG-2863 (RIG-1715 T2) service surface it mounts on, ordered after it. Added OQ-3 naming where token issuance lives (the RIG-2863 T4 slice, an IssueServiceToken mirroring MintRunnerToken) so an executor never improvises a mint path on a security-critical door. - F2 (low): citation span `lines 348-350` -> `348-351` (the quoted "disable)" falls on 351). - F3 (low, pre-existing): noted that `runnerhub.RunnerTokenRegistered` is the one token-store consumer outside the kind-gated door contract — flagged for the T4 executor to kind-check or document, out of scope for this record. Ledger-collision renumber: main's #870 (RIG-3096) merged its own DL-324 (forge live-test app-actor) after this stack forked, and the sibling encryption PR (#859) took DL-325/DL-326. Renumbered this record's SubjectService row to the next free id, DL-327 (was DL-325), leaving main's DL-324 untouched. Additive review-fix commit. Ledger-impact: DL-327 (SubjectService, renumbered from DL-325) Spec-impact: none Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-6 adversarial review returned 0H/2M/3L on the gateway-credentials at-rest encryption addendum. Both round-5 HIGH fixes verified correct and tree-accurate; the two mediums are defects the round-5 fold itself introduced or left, plus three low citation/mechanic nits. All folded as a new commit atop the bookmark (additive review-fix convention). - M1 (text-corruption regression): the round-5 edit deleted the opening half of the T0 non-bootstrap-tenant test's red-condition parenthetical, leaving a dangling `rows and skips it silently).` fragment. Restored the full clause (`red without WithSystemRole: the compass_app-scoped reconcile sees zero rows...`) — the single sentence stating why the test must fail, and the entire justification for DL-326's BYPASSRLS widening. T0 Tests-block paren balance re-verified 33/33. - M2 (self-contradictory enumeration): the C2 named-residual bullet counted "all SIX ... master key + ... two Linear client secrets", contradicting the record's canonical set (six MIGRATED names = primary+reviewer App PEM + webhook + three Linear secrets, master key a seventh provisioned directly). Restated as all SEVEN server-secret names with the master-key-vs-migrated split explicit, so the F1 membership guard's coverage set names the Linear webhook secret. - L1: qualified the T2 fingerprint placement to mirror T5 (persisted server-side alongside the master-key declaration; the server_secrets DDL has no column for it yet). - L2: named the H2 goroutine mechanic — the parent goroutine owns the pgx.Tx (not concurrency-safe) and does the rollback; only the ctx-less provider call is offloaded, reporting through a buffered (cap-1) channel so the leaked FFI goroutine can send and exit rather than block forever. - L3: softened the superuser-owner citation to what the cited lines establish (0001_init.sql:848-851 states the property; rls_pgtest_test.go asserts it of the harness, not production). markdownlint clean; design-ledger-gate OK (293 rows, 123 headers valid; DL-325/DL-326 anchors still resolve, record 60403 B). Ledger-impact: none (DL-325/DL-326 unchanged). Spec-impact: none (design record only). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-7 review returned 0 high / 4 medium / 3 low on PR #859; all seven grounded against the tree at `eb5ef7a1` and folded additively. ### Mediums - **M1 — key-swap tripwire had no schema home.** T5 placed the non-secret key fingerprint "beside `key_version` in `server_secrets`", but `key_version` is a T3 column on `gateway_credentials` and `server_secrets` is names-only — a circular, unimplementable reference for the T0-then-T2 executor. Gave the tripwire a real home: a single-row `server_key_state` table (`key_version`/`key_fingerprint`/`fingerprint_salt`) in T0's migration, bucket-A infrastructure joining the RLS allow-list; a salted digest is not a VALUE, so the names-only invariant is untouched. Rewrote the T2 pointer and T5 placement to name it; added it to the T0 Tasks line. - **M2 — superuser citation inverted.** The record claimed `rls_pgtest_test.go:282-284` asserts the superuser-owner property "not of production", but the cited comment reads "the pgtest harness (like production) connects as a SUPERUSER owner" — the opposite — and it is a prose comment, not an assertion. Rewrote the parenthetical to match the cited text (assumption stated in two comments, not an enforced invariant). - **M3 — bounded-critical-section self-contradiction.** One sentence said "runs the boot RESOLVE/SET on its own goroutine" while the same paragraph establishes `Set` is ctx-bounded (`exec.CommandContext`, resolver.go:233) and "offloads ONLY the ctx-less provider call". Under the RESOLVE/SET reading a leaked goroutine could `Set` after the parent's Rollback, outside the released advisory lock, overwriting a key another booter already sealed rows under — the exact race the lock exists to close. Made it unambiguous: only the ctx-less `Resolve` is offloaded; the parent runs `Set` in-lock on the success branch; the offloaded goroutine performs no provider write. - **M4 — F1 membership guard one-directional.** The user path rejected a `server_secrets` name, but `SetServerSecret` carried no mirror guard against a name already live in `secrets`, and the reconcile skipped a both-tables name — the same D6-inverting end state, reachable in the opposite order and unhealed. Restated the invariant order-free ("no name ever in both tables"), made F1 symmetric (mirror guard on the admin RPC), made the reconcile self-healing (deletes the `secrets` shadow row on collision), and added the two red-green tests. ### Lows - **L1 (DECISIONS.md DL-325)** — named the symmetric F1 membership guard alongside the reserved-name guard (the reserved-name check covers only 1 of 7 names). - **L2** — added an ordering note that failure mode (c) is masked by (a) on a full no-repoint and is the partial-repoint mode. - **L3** — split the mode-(b) citation to its two real sites (`forgeSecretDeclared` at serve.go:1746-1757; its caller's off-state at :1082-1083). Design record only; no code or proto change. Both prior-round HIGHs (forge taxonomy, Resolve cancellation asymmetry) re-verified intact. markdownlint 0, design-ledger-gate OK (293 rows, anchors resolve at 64364 B). Spec-impact: none. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…(RIG-2863) Round-8 review returned 0H/3M/6L; all seven round-7 folds HELD and both prior HIGHs (3-mode forge taxonomy, Resolve cancellation asymmetry) remain intact. The three mediums are consistency gaps the large round-7 M1/M4 folds left behind; all nine findings folded, each grounded against the tree at `eb5ef7a1`. - M (finding 1): the two M4 red-green tests landed only in the Tasks checklist, never in T0's own `- Tests:` contract bullet (the per-task test contract an executor implements against). Mirrored the admin-path MIRROR-GUARD and the RECONCILE-HEAL assertions into the `- Tests:` bullet. - M (finding 2): a D6 sentence claimed the user `SetSecret` path "could not carry a server secret anyway", contradicting the F1 user-path guard rationale 250 lines later. The cite (secrets_service.go:275-284) is byte-exact but the inference was false — rejecting an unspecified delivery does not stop a caller minting a shadow `secrets` row under a server-secret NAME. Narrowed the sentence to the true claim and made it point AT the guard as mandatory, not away from it. - M (finding 3): "together they make the undeliverability total" overstated against the record's own cited non-atomicity disclaimer (secrets_service.go:88-91: "not atomic and assumes no concurrent same-name writer"). Both guards are non-atomic check-then-act; a concurrent same-name writer is a residual both-tables window. Replaced "total" with the honest bound (the guards close every SEQUENTIAL order; the concurrent residual is the same single-writer MVP assumption the existing path already carries, atomic closure deferred to the multi-writer work the comment already flags). Also widened the boot reconcile from the six configured names to a full `server_secrets` scan so an operator-declared server secret raced into both tables self-heals too, regardless of provenance. - L (findings 4-9): pointer "above" -> "below" for the server_key_state grant rationale; named `server_key_state` in the names-only Global Constraint; added the bounded-offload clause to the every-boot read-back verify (same uncancellable Load on a steady-state boot); `admin_gate.go:119` -> `:122-125` (the wrapped-comment line -> the actual authenticatedOpen case block, matching the correct cite already at :504); re-wrapped the M2-fold's 101-col line to ~78; disambiguated the Tasks GRANT summary so server_key_state reads SELECT/INSERT/UPDATE (no DELETE) vs server_secrets's full grant. Record 64364 -> 67086 B (>50KB; both DECISIONS.md links still resolve — D1/T0 headings untouched). markdownlint 0, design-ledger-gate OK (293 rows, 123 headers). Interdiff +51/-15, record only. Refs RIG-2863 Spec-impact: design record only; no code or ledger-row change. The concurrent-writer atomic closure is deferred, not added — this record does not widen the single-writer contract. Co-authored-by: Matt Wilkinson <matt@rigel.build>
…mbership guard (RIG-2863) Round-9 review of the gateway_credentials at-rest encryption addendum returned 0H/2M/4L — the first fold in nine rounds with no new clipped-clause defect. All seven R7 folds and both R8 mediums held; both prior HIGHs (3-mode forge taxonomy, Resolve cancellation asymmetry) intact. The two mediums are genuine frozen-contract defects the R8 admin-RPC text surfaced, both driver-foldable off the frozen C1 ruling (false-premise correction / body-vs-Tasks consistency), no Matt fork. - **M1 — "NO public proto change" is false as stated.** The record asserts it four times as a load-bearing C1 property, but the `SetServerSecret`/`DeleteServerSecret` admin RPC C1 mandates (T0) IS an additive public-proto change: `SecretsService` lives in `proto/compass/v1/compass.proto` (the public `buf.gen.yaml` codegen lane, generating the checked-in `packages/compass-client/src/gen` + `go/gen` trees), and adding the RPCs is CI-enforced — `proto/moon.yml`'s `drift` re-gens and diffs the public trees, and `go/internal/auth/classify_exhaustive_test.go` reds CI on any generated procedure not classified. Narrowed the claim to its honest bound at all five sites (:21, :249/:254, :311/:316, :947→Tasks-T0, :995): C1 needs no public proto ENUM change and leaves FetchSecrets byte-for-byte unchanged, but adds two additive `SecretsService` methods. Added the CI consequence to Tasks-T0 (`moon run compass-proto:gen` + classify both procedures or `classify_exhaustive_test` reds). Mirrored into the DL-325 ledger row (edit-before-freeze). The C1 RULING is unchanged — it genuinely removes the SERVER_ONLY delivery-kind ENUM addition, the real and correct claim. - **M2 — Tasks under-specified the user-path guard as reserved-name-only.** Tasks T0 and T2 named only the reserved-name half, contradicting the body's central F1 finding (:526-531) that a reserved-name check is INSUFFICIENT because the six configured server-secret names are arbitrary operator config (`go/cmd/compass-server/main.go:414-450`; the Linear defaults `LINEAR_FORGE_CLIENT_ID`/ `LINEAR_FORGE_CLIENT_SECRET`, serve.go:202-203) carrying no `GATEWAY_CREDENTIALS_` prefix — so the guard must be a MEMBERSHIP test. A reserved-name-only executor would ship the exact `authenticatedOpen` shadow-row hole D6 exists to close. Made both Tasks lines name both halves (membership + reserved), matching the body and the Tests bullet (already correct). - **L1** — named the dangling "the trio" antecedent inline at :232 (the declare/set/rollback trio, introduced 475 lines later). - **L2** — stated the reconcile's lock discipline explicitly (:552): same `pg_advisory_xact_lock` constant key as T2, its OWN transaction taken/released before the T2 provisioning transaction, ordering constraint only that it precede `buildForgeReadWiring`. - **L3** — re-flowed the :545-556 reconcile paragraph to the surrounding width (removed the ragged mid-clause line the R8 full-scan fold left). - **L4** — noted that `WithSystemRole`'s own doc comment (tenant_tx.go:41-47) already names the forge-notification-ack arm, so the DL-315 staleness is in the ledger prose not the code's documentation; mirrored into the DL-326 row. Both mediums grounded against tree eb5ef7a (proto public-lane + classify_exhaustive_test + six-name provenance verified). markdownlint 0, design-ledger-gate OK (293 rows, 123 headers); both DL-325/DL-326 record anchors resolve (D1/T0 headings untouched). Ledger-impact: DL-325 proto clause narrowed to ENUM-only + admin-RPC method delta; DL-326 forge-arm clause notes the doc-comment already names the arm (both edit-before-freeze). Spec-impact: none (design record only). Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…pairs + F1 symmetry (RIG-2863) Round-10 review of the `gateway_credentials` at-rest encryption addendum returned 0 high / 1 medium / 4 low; all five are driver-foldable off the frozen C1 mechanism (no design fork). The single medium is a scheduling gap this record's own R9 fold created but did not schedule. ### M1 — T0 must schedule the doc-comment repairs its own changes force (medium) T0 adds a fifth `store.WithSystemRole` site (the boot reconcile) and two `adminOnly` `SecretsService` methods. Both falsify shipped code comments the moment T0 lands: `tenant_tx.go`'s "applied ONLY at the four named background-loop entrypoints" ONLY-claim (:36-38, :43-45), and five "three/3 procedures authenticatedOpen" SecretsService comments (`admin_gate.go:116-121`, `network_door.go:288-290`, `secrets_service.go:6`, `serve.go:555`, `serve.go:749-750` — all verified byte-exact at `eb5ef7a1`). This is exactly the failure DL-326 was minted to prevent, relocated from the ledger into the code. Added an explicit doc-repair deliverable to Tasks T0 and softened the body L4 clause (:620-625) to note T0 itself widens the code doc's ONLY-claim, so both surfaces are corrected in this PR chain. ### F1 symmetry + store-door parity (lows) - **Tasks T0** re-attaches SYMMETRIC to the guard *pair* (not the user-path half alone, which could license skipping the admin half) and names the admin MIRROR membership guard as an interface deliverable, at both the RPC and the `store.DeclareServerSecret` store door. - **Store bullet** (:443-452): `DeclareServerSecret` now carries the mirror of the `store.DeclareSecret` membership guard, so a `server_secrets` shadow row can never be created regardless of which writer reaches it — F1 held at BOTH doors, not only at the RPC. - **Reconcile lock discipline** (:564-569): the boot reconcile now inherits T2's bounded acquisition (`pg_try_advisory_xact_lock` in a bounded retry loop, or a session `lock_timeout`) so a contended boot fails closed with a diagnosable error rather than parking — it is the first toucher of that shared key at boot. - **Delivery enum** (:259-261): narrowed the literally-false "two-valued in all four representations" (the proto rep quotes three enum values) to "gains no new value in any of the four representations — two live delivery kinds plus the proto3 `UNSPECIFIED` zero sentinel". Both prior HIGHs (3-mode forge taxonomy silent-degradation; Resolve cancellation asymmetry) and all R7/R8/R9 folds re-confirmed intact; exhaustive citation audit found zero drifted cites. Record 68683 → 70607 B (>50KB; both DL-325/DL-326 anchors resolve). markdownlint 0, design-ledger-gate OK (293 rows, 123 headers). Ledger-impact: none (DL-325/DL-326 rows unchanged this round). Spec-impact: none. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…k + complete T0 doc-repairs (RIG-2863)
R11 review returned 0H/1M/4L. The high is a self-inflicted regression the R10 F4 fold introduced; all five findings are driver-foldable off the frozen C1 mechanism (no Matt fork — pure consistency repair of an already-frozen mechanism, completing a deliverable, and reflow).
### H1 — reconcile/store-door deadlock (R10 F4 regression)
The R10 fold made the `store.DeclareServerSecret` mirror guard **unconditional** ("shadow row can never be created regardless of which writer reaches it"), which collided head-on with T0's own boot reconcile — a writer whose defining precondition (a name still live in `secrets`) is exactly what the guard rejects. As frozen, T0 could not execute its own OQ-4 deliverable: the reconcile would fail closed on all six configured names on every existing deployment, leaving the inject-all exposure D6 exists to close open.
Fix (driver-foldable ordering repair, not the guard-free-reconcile fork): pin the reconcile's intra-transaction order to **DELETE the `secrets` row FIRST, then INSERT into `server_secrets`** within its single `WithSystemRole` transaction, so the store-door membership SELECT sees the delete already applied. Narrowed the absolute "regardless of which writer" claim at both the body store-door bullet and the Tasks T0 restatement to "no writer can create a shadow row while the name is live in `secrets`", explicitly noting the reconcile satisfies the guard by deleting first. Added a `reconcile-orders-delete-before-insert` red-green test to the T0 list (an insert-before-delete reconcile reds against the guard).
### M1 — T0 doc-repair deliverable was under-enumerated
The R10 fold scheduled five Go comment repairs but omitted two more doc comments T0's own two `adminOnly` `SecretsService` methods falsify:
- `proto/compass/v1/compass.proto:175-179` (the `SecretsService` service comment) — the worst of the set: it is the public wire contract and propagates verbatim into the drift-gated `packages/compass-client/src/gen` + `packages/compass-agent/src/gen` trees, and `moon run compass-proto:gen` regenerates the same stale text, so the drift gate does NOT catch it.
- `go/internal/auth/admin_gate.go:22-26` (the `adminOnly` type doc) — T0 adds the first non-CompassService members of this class.
Both added to the "doc-comment repairs T0's own changes require" clause, plus an opportunistic note on the pre-existing (not T0-caused) staleness at `admin_gate.go:40-41` / `classify_exhaustive_test.go:45-46`.
### Lows
- L1/L2 — reflowed two unwrapped-splice artifacts (the Tasks guard splice, the F2 lock-discipline rejoin) to the record's ~80-col convention.
- L3 — folded into M1's opportunistic note.
Record-only interdiff (+42/-15). No DECISIONS.md change (DL-325/DL-326 rows unchanged from R9). markdownlint 0 errors; design-ledger-gate OK (293 rows, 123 headers); record 72488 B (>50KB — both DL-325/DL-326 anchors resolve, D1/T0 headings untouched).
Ledger-impact: none (no DECISIONS.md edit this round).
Spec-impact: none.
Refs RIG-2863
Co-authored-by: Matt Wilkinson <matt@rigel.build>
…reconcile requires + correct gen-lane label (RIG-2863)
Round 12 returned 0H/2M/3L. First regression-free round in twelve: the reviewer confirmed the R11 deadlock fix HOLDS at all three sites and found no new defect in the added text. Both mediums are pre-existing gaps R11's own text made newly load-bearing; folded both plus one low. L2/L3 (cosmetic reflow, no width gate) left as-is.
### M1 — the atomic single-transaction claim was not implementable through the store API the record specifies (driver-foldable false-premise correction)
R11's reconcile fix claimed "within the reconcile's single `WithSystemRole` transaction, DELETE the `secrets` row FIRST, then INSERT ... (atomic: no crash window)". But the record named the two operations as plain-signature store methods, and every such method executes through `scopedDBTX` as its OWN implicit transaction — one `pool.SendBatch` per statement (`go/internal/store/tenant_tx.go:80-92`; :64-70 states multi-statement-tx methods use `beginTenantTx` instead). So two plain calls are two transactions, the "atomic" claim is false as written, and a crash between them strands the name in NEITHER table — strictly worse than pre-fold (the heal scans only names live in `secrets`).
The store carries exactly one multi-statement-tx seam: `(*Store).WithTx(ctx, fn)` (`coordination.go:368`), which runs `fn` in one `beginTenantTx` and honors the system role via `armTx` (`tenant_tx.go:199-215`), plus the shipped `…Tx(ctx, tx, …)` method convention (`coordination.go:86/168`, `dm.go:49/97`). Naming the seam completes the already-frozen atomicity requirement — not a fork. Reconcile bullet now specifies `store.WithTx(store.WithSystemRole(ctx), …)` with tx-bound `DeleteSecretDeclarationTx`/`DeclareServerSecretTx` variants and states why the plain methods would break atomicity.
### M2 — gen-lane label corrected (the reviewer's go/gen claim did not hold against the tree)
The reviewer flagged the `SecretsService` service-comment repair as also propagating into `go/gen` (the connect-go tree). Verified against tree `eb5ef7a1`: the SERVICE comment ("User-facing secret registry writes…") has ZERO matches in either `go/gen` file — `compass.connect.go:803` is generator boilerplate and :805-812 are per-method comments T0 does not touch. So no go/gen target was missed. The real defect the finding also raised IS correct: the record mislabeled `packages/compass-agent/src/gen` as a "public" tree. Per `proto/moon.yml:35-40` it is an internal-only lane (`buf.gen.agent-ts.yaml`); only `packages/compass-client/src/gen` is public (`buf.gen.yaml`). Corrected the label; did not add the unsupported go/gen target.
### L1 — ORDERING assertion added to the T0 Plan Tests bullet
The `reconcile-orders-delete-before-insert` assertion lived only in the Tasks test list; added a matching ORDERING assertion beside RECONCILE-HEAL in the Plan Tests bullet (the prescriptive list), including the both-commit-or-both-roll-back property M1 now pins.
Record-only this round (DECISIONS.md DL-325/DL-326 unchanged). Record 72488 → 73667 B (>50KB; both anchors resolve). markdownlint 0 errors; design-ledger-gate OK (293 rows, 123 headers). Interdiff +24/-10, record only.
Ledger-impact: none. Spec-impact: none. Refs RIG-2863
Co-authored-by: Matt Wilkinson <matt@rigel.build>
…mirror sites + fix ORDERING-assertion polarity (RIG-2863) Round 13 adversarial review of the credentials-at-rest encryption record returned 0 High / 2 Medium / 2 Low — the first regression-free round in which every finding is a driver-foldable consistency repair off the frozen mechanism, no design fork. Both mediums grounded byte-exact against the tree at `eb5ef7a1`; the two lows are one cheap citation-precision fix and one confirmed-cosmetic reflow left as-is. Record-only (DECISIONS.md DL-325/DL-326 unchanged from R9). ### M1 — the two reconcile mirror sites named a construct the R12 fold removed R12 rewrote the row-migration bullet to run the reconcile's DELETE+INSERT inside one `store.WithTx(store.WithSystemRole(ctx), …)` transaction, but left the two mirror sites (the `DeclareServerSecret` store-door bullet and the T0 Tasks restatement) saying the delete happens "inside the `WithSystemRole` transaction". After the fold there is no such construct: `store.WithSystemRole(ctx)` is a context marker (`context.WithValue`, tenant_tx.go:47-49), not a transaction, and passing it to a plain store method is exactly the two-implicit-transactions path the fold forbids. An executor implementing the store door or working the Tasks checklist from either mirror alone would write the two-transaction crash window the record now also requires a test to red on. Both mirror phrases now name the actual single-transaction construct `store.WithTx(store.WithSystemRole(ctx), …)` and the tx-bound `DeclareServerSecretTx` insert. ### M2 — the R13-added ORDERING assertion inverted the atomicity polarity The ORDERING test assertion I added in R12 stated the failure mode as the guarantee: "a crash between them strands the name in neither table". Under one transaction a crash rolls both statements back, so the name stays in `secrets` — it is stranded in neither table only under the two-transaction implementation the record forbids. The record's own corrected reconcile bullet states the correct polarity at the D6 mechanism. Fixed the assertion to "leaves the name in `secrets`, never stranded in neither table", matching the reference; the trailing "red if the DELETE and INSERT run as two transactions rather than one `store.WithTx`" parenthetical now agrees with the clause it qualifies. ### L — off-by-two citation The gen-lane classification cite `proto/moon.yml:35-40` clipped the closing of the agent-ts internal-lane clause; corrected to `:35-41`. The substance (compass-agent/src/gen internal-only via buf.gen.agent-ts.yaml; compass-client/src/gen + go/gen public via buf.gen.yaml) was already accurate. The two reflow orphans (L2/L3) are confirmed grammatically whole with no width gate; left as-is. Spec-impact: none. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…o the DeclareServerSecretTx variant the reconcile uses (RIG-2863) R14 review returned 0H/1M/2L, all driver-foldable off the frozen record. The medium is a pre-existing gap the R13 M1 fold newly exposed (not a regression): R13 correctly routed the boot reconcile through the tx-bound `DeclareServerSecretTx` variant, but the store-door membership guard was still specified only on the plain `DeclareServerSecret`. Read literally, an executor could build an unguarded `DeclareServerSecretTx` twin — bypassing F1's store door entirely and silently deleting the `reconcile-orders-delete-before-insert` tripwire (that test asserts the guard reds on the wrong-order reconcile; with no guard on the Tx path there is nothing to red). Folds (record-only; completes the already-frozen "F1 holds at BOTH doors" mechanism under the name the record already mandates — no fork, decides nothing): - **M (guard on the Tx variant)** — store-door bullet now states `DeclareServerSecret` AND its tx-bound `DeclareServerSecretTx` variant BOTH carry the mirror guard via one shared unexported check (so they can never drift); the Row-migration bullet adds that each `…Tx` variant carries the same store-door guard as its plain counterpart, so the DELETE-FIRST ordering is what admits the insert; the Tasks T0 deliverable names both variants at the store door. - **L (citation)** — `binding_cgo.go:26` → `:30` (the `C.secretspec_resolve(req)` call site; :26 is the doc-comment start). - **L (reflow orphans)** — confirmed cosmetic (no width gate, MD013 off), left as-is. Mechanical pass clean: guarded single-occurrence replaces; in-memory diff record-only +11/-6; every edited region re-read whole; backticks even, paren net 0, D1/T0 anchor headings byte-identical. markdownlint 0 errors; design-ledger-gate OK (293 rows, 123 headers). Record 73310 → 73666 B (>50KB; both DL-325/DL-326 anchors resolve). No DECISIONS.md change (DL-325/326 unchanged from R9). Ledger-impact: none (record-only; DL-325/DL-326 unchanged). Spec-impact: none. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…e to the insert side + name DeleteSecretDeclarationTx in Tasks T0 (RIG-2863) #859 R15 adversarial review returned `0H/2M/2L`. This commit folds the driver-foldable medium (a self-inflicted regression from the R14 fold) and one low; the second medium is a cross-tenant security fork routed to Matt at the merge gate (see below), not folded. ### M1 (medium, FOLDED) — R14 fold widened the guard onto the DELETE side, breaking the reconcile heal arm The R14 fold wrote the Row-migration guard clause distributively over a two-item list (`DeleteSecretDeclarationTx`/`DeclareServerSecretTx`), so "each carrying the SAME store-door membership guard" extended the guard to the delete variant too. That is wrong two ways: (1) `DeleteSecretDeclaration` carries no store-door membership guard anywhere in the record, so the delete-side claim references a guard that does not exist; (2) if an executor builds one, it rejects exactly the deletes the reconcile heal arm requires (delete a `secrets` row for a name simultaneously live in `server_secrets`, :573-583), making the `reconcile-heals-both-tables-collision` acceptance criterion unsatisfiable against the same paragraph's mandated implementation. Narrowed the clause to the insert side only — `DeclareServerSecretTx` carries the guard mirroring its plain `DeclareServerSecret` counterpart — with an explicit parenthetical that the delete side carries no membership guard (the heal arm depends on being able to delete a shadow row whose name is live in `server_secrets`), so a later fold cannot re-widen it. Driver-foldable: narrows an over-broad clause back to the R14 contract and decides nothing. ### L (low, FOLDED) — DeleteSecretDeclarationTx mandatory in Plan, absent from Tasks checklist The tx-bound `DeleteSecretDeclarationTx` is load-bearing in the Row-migration bullet but never named in the Tasks T0 deliverable list, so an executor working the checklist would not build it (no `…Tx` sibling of `DeleteSecretDeclaration` exists today, store/secrets.go:160). Added it to the T0 reconcile deliverable as a new (both-NEW) tx-bound pair. ### M2 (medium, ROUTED TO MATT — not folded) The shared store-door membership check runs a SELECT against `secrets`, which is FORCE ROW LEVEL SECURITY. The reconcile caller arms BYPASSRLS (`store.WithSystemRole`); the admin-RPC caller (`SetServerSecret`) is tenant-scoped (`compass_app`, no BYPASSRLS). An admin `SetServerSecret` for a name whose `secrets` row was declared under a different tenant runs a membership SELECT that returns zero rows, admits the insert, and creates the both-tables state F1 forbids. The fix is to arm the guard's `secrets` membership SELECT under BYPASSRLS on the admin path too — a cross-tenant BYPASSRLS widening, which this record already declares a Matt-ruled security event (:620-626 documents the identical blind spot for the reconcile). Surfaced to Matt at the merge gate with the reviewer's recommended fix; not auto-folded. ### L (low, left as-is) — reflow orphans Cosmetic, no width gate (`.markdownlint.json` is `MD013: false`). Re-confirmed grammatically whole for a fourth round. Record-only (no DECISIONS.md change; DL-325/326 unchanged). markdownlint 0 issues; design-ledger-gate OK (293 rows, 123 headers). Record 73666 → 74082 B. Ledger-impact: none. Spec-impact: none. Refs RIG-2863 Co-authored-by: Matt Wilkinson <matt@rigel.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addendum to the frozen
compass-server-llm-gatewayrecord: encryption-at-restfor the
gateway_credentialsvalue store, master-key custody, and a physicallyseparate
server_secretsstore for server-only secrets.Decisions (all Matt-ruled):
key_version columns, no plaintext value column ever; fresh 96-bit nonce per
write (2^32-per-key bound quantified against the OAuth-refresh write rate).
configured SecretSpec provider under a Postgres advisory lock with an
every-boot read-back verify; key never touches the DB.
key_versioncolumn from day one (cheap now, expensive retrofit).MarshalJSON), never the RPC serialization type.
server_secretsstore — a names-only table + a SECOND SpecResolver instanceover it (shared profile) + an admin-gated SetServerSecret/DeleteServerSecret
RPC. The delivery boundary is the resolver's manifest, so a server secret is
STRUCTURALLY undeliverable to agent containers (default-closed by
construction). Removes the public proto change the earlier
SERVER_ONLY-delivery-kind fold required; the container FetchSecrets path is
byte-for-byte unchanged.
The PEM/webhook/Linear server secrets — server_only by convention only today,
riding the inject-all path into every agent container — move into
server_secretsin this PR chain (a DB-row move, provider values untouched),closing that pre-existing exposure (OQ-4, Matt-ruled: same PR chain).
Tasks: T0 server_secrets store (prerequisite) → T1 envelope pkg → T2 boot
provision seam → T3 ciphertext columns → T4 seal/open wiring → T5 tripwire +
redaction assertions.
Open sub-fork flagged for review: the server-provisioned master key's
declared_byFK is specced NULLABLE (NULL = server-provisioned; honestprovenance, no boot-ordering coupling) rather than attributed to the
bootstrap-admin account or an @Compass sentinel — a one-line T0 change either
way with no downstream effect.
Ledger: DL-324 (Storage).
Spec-impact: adds
server_secretstable + SetServerSecret/DeleteServerSecretadmin RPC + gateway_credentials ciphertext columns to the RIG-2863 build.
Refs RIG-2863
Co-authored-by: Matt Wilkinson matt@rigel.build