Skip to content

Retype grant identity fields to PrincipalId (#200) - #245

Merged
George-RD merged 1 commit into
mainfrom
George-RD/dev-200
Aug 21, 2026
Merged

George-RD merged 1 commit into
mainfrom
George-RD/dev-200

Conversation

@George-RD

@George-RD George-RD commented Aug 21, 2026

Copy link
Copy Markdown
Owner

What & why

Phase 2 of typed owner identity (spec #197). Retypes the grant identity fields
from String to the serde-transparent PrincipalId(Ulid) newtype landed by
#199, so owner identity is typed end-to-end and the pre-#200 non-Ulid user
shapes ("owner", "kernel", a stringified i64) become unrepresentable by
construction.

  • Schemas: TaskGrant.user, RootAuthority.user, and
    SelectionToken.user/selected_by are now PrincipalId. RootAuthority::from_grant
    drops the String clone (PrincipalId is Copy); canonical_bytes is
    untouched — PrincipalId serializes transparently as the bare Ulid string.
  • Clean cutover, no shims: every composition/read/SQL site across
    openspine-authority, openspine-kernel, and openspine-gate migrated.
    Production grants now carry the real owner PrincipalId
    (state.owner_principal_id), replacing "owner"/"kernel"/owner_user_id
    string shapes. WorkerIdentity.owner and OperationAuthorization.owner_principal_id
    remain String (documented downstream cutover) and convert via .to_string().
  • SQL: store::load_owner_miner_audit_slice takes a PrincipalId; the
    json_extract('$.user') predicate still binds text, the app-level recheck is
    now a typed equality.

Wire format

Unchanged. PrincipalId is #[serde(transparent)] over Ulid, so a grant that
sealed a stringified Ulid in user verifies identically after the retype
(D-005). Proven by a new MAC-preimage + serialized-user byte test in
grant_chain/tests.rs.

Type safety

Three compile_fail doctests on PrincipalId assert a String literal can no
longer be assigned to TaskGrant.user, RootAuthority.user, or
SelectionToken.user. (Built-in doctests instead of trybuild: no new
dependency / network fetch and no compiler-version-fragile .stderr goldens.)

Verification

./scripts/check.sh green (fmt, clippy -D warnings, full workspace tests
incl. kernel E2E against the real shell binary, file-size/claims/omp-ceremony
checks, OpenSpec --all --strict). Kernel suite: 1128 passing.

Notes (implementation-notes 5-line summary)

  1. Retyped TaskGrant.user, RootAuthority.user, SelectionToken.user/selected_by
    from String to the serde-transparent PrincipalId(Ulid) newtype from Introduce PrincipalId newtype and OwnerPrincipal aggregate #199.
  2. Clean cutover of every composition/read/SQL site across schemas, authority,
    kernel, and gate — production owner/kernel/owner_user_id shapes now resolve to
    the real owner PrincipalId; no compatibility shims.
  3. Wire format unchanged: PrincipalId serializes as the bare Ulid string, so
    existing sealed grant MACs verify unmodified (D-005).
  4. Added a MAC-preimage + wire byte-preservation test and three compile_fail
    doctests proving String literals no longer type-check for the identity fields.
  5. ./scripts/check.sh green; full workspace suite passes.

Deviations worth a reviewer's eye

  • No PrincipalId::OWNER/KERNEL constants exist in Introduce PrincipalId newtype and OwnerPrincipal aggregate #199, so kernel-authored
    synthetic grants (kernel_notify_grant, mint_reconfirm_grant) — which held
    user: "kernel" — were threaded the owner PrincipalId (AD-146 single owner;
    no code reads user == "kernel" as a discriminator).
  • issued_by intentionally stays String (out of ticket scope).
  • Overlay-control test helpers keep a user: &str label mapped through a new
    deterministic test_support::principal(label) (SHA-256 → Ulid) so
    owner-distinctness security regressions (regressions.rs, portability.rs)
    stay meaningful.

Closes #200


Summary by cubic

Retypes grant identity fields from String to a typed PrincipalId(Ulid) to make owner identity unambiguous and compile‑time safe. Previously TaskGrant.user, RootAuthority.user, and SelectionToken.user/selected_by accepted free‑form strings (e.g., "owner", "kernel", stringified i64); now they require a PrincipalId. Wire format is unchanged because PrincipalId is #[serde(transparent)] over Ulid.

  • Schema changes span openspine-schemas, with clean cutover across openspine-authority, openspine-kernel, and openspine-gate. Kernel‑authored synthetic grants that used "kernel" now carry the actual owner PrincipalId.
  • SQL: Store::load_owner_miner_audit_slice takes a PrincipalId; we still bind the JSON extract as text and compare via PrincipalId::to_string().
  • Behavior stays the same at the edges: sealed grants verify identically; canonical MAC preimage and serialized JSON bytes are preserved (new tests assert this). Three compile_fail doctests prove raw String assignments no longer type‑check.

Bolded section titles are not used because all content above is essential and minimal for review and rollout.

Written for commit 3fda22b. Summary will update on new commits.

Review in cubic

Retype TaskGrant.user, RootAuthority.user, and SelectionToken.user/selected_by
from String to the serde-transparent PrincipalId(Ulid) newtype (#199). Clean
cutover of every composition, read, and SQL site across schemas, authority,
kernel, and gate; production owner/kernel string shapes now resolve to the real
owner PrincipalId. Wire format unchanged, so existing sealed grant MACs verify
unmodified (D-005). Adds a MAC-preimage + wire byte-preservation test and three
compile_fail doctests proving String literals no longer type-check.

Closes #200
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@George-RD, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 66d26c02-f228-430a-9d27-22f4825d6dc1

📥 Commits

Reviewing files that changed from the base of the PR and between 323cf76 and 3fda22b.

📒 Files selected for processing (47)
  • crates/openspine-authority/src/compose.rs
  • crates/openspine-authority/src/worker_grant.rs
  • crates/openspine-gate/src/gate/tests.rs
  • crates/openspine-gate/src/gate/token_tests.rs
  • crates/openspine-kernel/src/api/artifact_propose_miner_grouping_tests.rs
  • crates/openspine-kernel/src/api/artifact_propose_miner_tests.rs
  • crates/openspine-kernel/src/api/briefcase_tests.rs
  • crates/openspine-kernel/src/api/dispatch_tests.rs
  • crates/openspine-kernel/src/api/generate_tests.rs
  • crates/openspine-kernel/src/api/overlay_export_restore.rs
  • crates/openspine-kernel/src/api/scoped_admission_support.rs
  • crates/openspine-kernel/src/api/task.rs
  • crates/openspine-kernel/src/api/worker.rs
  • crates/openspine-kernel/src/briefcase/tests.rs
  • crates/openspine-kernel/src/connectors_tests.rs
  • crates/openspine-kernel/src/escalation.rs
  • crates/openspine-kernel/src/overlay_export_restore/control.rs
  • crates/openspine-kernel/src/overlay_export_restore/control_tests.rs
  • crates/openspine-kernel/src/overlay_export_restore/control_tests/portability.rs
  • crates/openspine-kernel/src/overlay_export_restore/operation_tests/mod.rs
  • crates/openspine-kernel/src/overlay_export_restore/operation_tests/regressions.rs
  • crates/openspine-kernel/src/overlay_startup_tests.rs
  • crates/openspine-kernel/src/pipeline/approval.rs
  • crates/openspine-kernel/src/pipeline/message_notify.rs
  • crates/openspine-kernel/src/pipeline/mod.rs
  • crates/openspine-kernel/src/pipeline/owner_review_decision.rs
  • crates/openspine-kernel/src/pipeline/selection.rs
  • crates/openspine-kernel/src/pipeline/tests/approval.rs
  • crates/openspine-kernel/src/pipeline/tests/driver.rs
  • crates/openspine-kernel/src/pipeline/tests/task_board.rs
  • crates/openspine-kernel/src/pipeline/worker_e2e_tests.rs
  • crates/openspine-kernel/src/pipeline/worker_result_consumer_tests.rs
  • crates/openspine-kernel/src/reflection_miner_runtime.rs
  • crates/openspine-kernel/src/reflection_miner_runtime/scheduled.rs
  • crates/openspine-kernel/src/secret_intake.rs
  • crates/openspine-kernel/src/store/reflection_miner_support.rs
  • crates/openspine-kernel/src/store/tests.rs
  • crates/openspine-kernel/src/store/worker_dispatch_tests.rs
  • crates/openspine-kernel/src/store/worker_supervision_tests.rs
  • crates/openspine-kernel/src/test_support.rs
  • crates/openspine-schemas/src/escalation.rs
  • crates/openspine-schemas/src/grant.rs
  • crates/openspine-schemas/src/grant_chain.rs
  • crates/openspine-schemas/src/grant_chain/tests.rs
  • crates/openspine-schemas/src/ids.rs
  • crates/openspine-schemas/src/reflection_miner_tests.rs
  • crates/openspine-schemas/src/selection.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@George-RD
George-RD merged commit 6d68b77 into main Aug 21, 2026
2 of 3 checks passed
@George-RD
George-RD deleted the George-RD/dev-200 branch August 21, 2026 03:30
George-RD added a commit that referenced this pull request Aug 21, 2026
#245 retyped TaskGrant.user to PrincipalId but missed the
project_catalog integration-test fixture builder, which broke
`cargo clippy --all-targets`. Apply the same Ulid::new().into()
pattern used across #245's test updates.
George-RD added a commit that referenced this pull request Aug 21, 2026
…) (#246)

ProvenanceLabelAllowlist caveat (AD-060 discipline, dormant on owner grant) + fixes the #245/#242 cross-merge build breaker in authority test fixture. Closes #226.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retype TaskGrant.user and RootAuthority.user to PrincipalId

1 participant