M2: Backfill + startup invariants for guardian principal#10969
Merged
noanflaherty merged 2 commits intoMar 1, 2026
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
👀 Where to focus your review
Risk level: Medium — data migration that expires pending requests that can't be deterministically bound to a principal. This is the intended behavior per the task spec, but worth confirming no edge case leaves a legitimately actionable request expired. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…ktop rebinding predicate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Contributor
Author
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
noanflaherty
merged commit 78e7990
into
feature/canonical-id-binding-cutover
1 check passed
5 tasks
noanflaherty
added a commit
that referenced
this pull request
Mar 1, 2026
* feat: add guardianPrincipalId to schema and storage plumbing (#10965) Co-authored-by: Claude <noreply@anthropic.com> * M2: Backfill + startup invariants for guardian principal (#10969) * feat: backfill guardianPrincipalId and enforce startup invariants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: restrict backfill expiration to expired requests and tighten desktop rebinding predicate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * feat: propagate guardianPrincipalId through runtime contexts (#10978) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * M4: Bind principal at all canonical request creation sites (#10980) * feat: bind guardianPrincipalId at all canonical request creation sites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add try/catch guards around createCanonicalGuardianRequest in all caller paths Wrap createCanonicalGuardianRequest calls in try/catch in three locations to handle IntegrityError when guardianPrincipalId is missing: 1. daemon/server.ts (makePendingInteractionRegistrar) - prevents crash when session.guardianContext is undefined during tool approval events 2. runtime/routes/conversation-routes.ts (makeHubPublisher) - same pattern for the HTTP hub publisher path 3. runtime/access-request-helper.ts - preserves the intentional no-binding fallback path (documented at file header) where access requests proceed without guardian identity All catch blocks log at debug level matching the existing pattern in daemon/handlers/sessions.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add try/catch in inbound-message-handler and fix misleading notified:true in access-request-helper - Wrap createCanonicalGuardianRequest call in inbound-message-handler.ts ingress escalation path with try/catch to prevent unhandled IntegrityError from crashing the HTTP handler with a 500. On failure, logs a warning and continues to the notification pipeline. - Fix catch block in access-request-helper.ts to return notified: false instead of notified: true, since the emitNotificationSignal call is never reached when the error is caught. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: pass guardianPrincipalId to all createBinding callsites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * M5: Cutover decision authorization + remove isTrusted (#10990) * feat: cutover decision authorization to principal-based and remove isTrusted Replace the isTrusted compatibility bypass in all runtime types and callsites with principal-based authorization. Decision authorization is now purely principal-based: actor.guardianPrincipalId must match request.guardianPrincipalId for any applied decision. There is no longer a trusted bypass path. Key changes: - Remove isTrusted from ActorContext type, replace with guardianPrincipalId - Add three-step principal validation in applyCanonicalGuardianDecision() - Add guardianPrincipalId filter to canonical guardian store queries - Update guardian-reply-router to use principal-based request discovery - Update all callsites (HTTP routes, daemon handlers, session process, inbound message handler) to resolve and pass guardianPrincipalId - Replace isTrusted-based guardianReplyText conditionals with channel checks Closes #10960 * fix: resolve cross-channel principal mismatch and non-vellum channel fallback - conversation-routes.ts: Fall back to session.guardianContext for verifiedActorExternalUserId and verifiedActorPrincipalId when actorVerification is null (non-vellum channels). - guardian-decision-primitive.ts: Allow cross-channel guardian decisions by checking actor principal against the assistant's canonical vellum binding principal when direct principal match fails. - guardian-reply-router.ts: Add guardianPrincipalId filter to the conversation fallback query in findPendingCanonicalRequests so guardians only see requests they are authorized to act on. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add cross-channel principal fallback to code-only clarification check Extract isAuthorizedGuardianPrincipal() shared helper from the inline cross-channel fallback logic in applyCanonicalGuardianDecision, and use it in the router's code-only clarification identity check. This ensures a desktop guardian entering a request code for a cross-channel request (e.g. Telegram-originated) sees the clarification context instead of getting "Request not found". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) (#11002) * feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) - Update 6 test files to use guardianPrincipalId principal-match pattern instead of legacy isTrusted boolean - Add guard test (no-is-trusted-guard.test.ts) to prevent isTrusted reintroduction in production code - Clean up 3 stale comments referencing trusted bypass and desktop/trusted with principal-based terminology - Verify no production isTrusted references remain (only allowed trust-class variable names: isTrustedActor, isTrustedContact, isTrustedTrustClass) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: correct guard test pipe and identity mismatch test field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use JS-level allowlist filtering in guard test to prevent masking --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address holistic review — decidedByPrincipalId, access_request exempt, backfill expiry, code lookup guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: make isAuthorizedGuardianPrincipal symmetric for cross-channel approval Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address reviewer feedback — binding check, pre-bootstrap IPC, migration access_request exclusion - isAuthorizedGuardianPrincipal: verify actor's principal belongs to an active guardian binding before allowing cross-channel approval, closing the asymmetric authorization gap (Devin #3/#4) - local-actor-identity: eagerly create vellum binding via ensureVellumGuardianBinding in pre-bootstrap IPC path so downstream decisionable requests always have a guardianPrincipalId (Devin #5) - Migration 126: exclude access_request from expiry sweep in steps 3a and 3c since access_request is non-decisionable and proceeds via the invite flow (Codex P2 #2) - Update roundtrip tests to supply guardianPrincipalId for decisionable tool_approval requests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: unify channel binding principals + fix test fixtures for IntegrityError guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: align migration registry checkpoint key with migration function (_v1 -> _v2) * fix: narrow CanonicalDecisionResult union before accessing resolverReplyText --------- Co-authored-by: Claude <noreply@anthropic.com>
tkheyfets
pushed a commit
that referenced
this pull request
Mar 2, 2026
* feat: add guardianPrincipalId to schema and storage plumbing (#10965) Co-authored-by: Claude <noreply@anthropic.com> * M2: Backfill + startup invariants for guardian principal (#10969) * feat: backfill guardianPrincipalId and enforce startup invariants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: restrict backfill expiration to expired requests and tighten desktop rebinding predicate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * feat: propagate guardianPrincipalId through runtime contexts (#10978) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * M4: Bind principal at all canonical request creation sites (#10980) * feat: bind guardianPrincipalId at all canonical request creation sites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add try/catch guards around createCanonicalGuardianRequest in all caller paths Wrap createCanonicalGuardianRequest calls in try/catch in three locations to handle IntegrityError when guardianPrincipalId is missing: 1. daemon/server.ts (makePendingInteractionRegistrar) - prevents crash when session.guardianContext is undefined during tool approval events 2. runtime/routes/conversation-routes.ts (makeHubPublisher) - same pattern for the HTTP hub publisher path 3. runtime/access-request-helper.ts - preserves the intentional no-binding fallback path (documented at file header) where access requests proceed without guardian identity All catch blocks log at debug level matching the existing pattern in daemon/handlers/sessions.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add try/catch in inbound-message-handler and fix misleading notified:true in access-request-helper - Wrap createCanonicalGuardianRequest call in inbound-message-handler.ts ingress escalation path with try/catch to prevent unhandled IntegrityError from crashing the HTTP handler with a 500. On failure, logs a warning and continues to the notification pipeline. - Fix catch block in access-request-helper.ts to return notified: false instead of notified: true, since the emitNotificationSignal call is never reached when the error is caught. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: pass guardianPrincipalId to all createBinding callsites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * M5: Cutover decision authorization + remove isTrusted (#10990) * feat: cutover decision authorization to principal-based and remove isTrusted Replace the isTrusted compatibility bypass in all runtime types and callsites with principal-based authorization. Decision authorization is now purely principal-based: actor.guardianPrincipalId must match request.guardianPrincipalId for any applied decision. There is no longer a trusted bypass path. Key changes: - Remove isTrusted from ActorContext type, replace with guardianPrincipalId - Add three-step principal validation in applyCanonicalGuardianDecision() - Add guardianPrincipalId filter to canonical guardian store queries - Update guardian-reply-router to use principal-based request discovery - Update all callsites (HTTP routes, daemon handlers, session process, inbound message handler) to resolve and pass guardianPrincipalId - Replace isTrusted-based guardianReplyText conditionals with channel checks Closes #10960 * fix: resolve cross-channel principal mismatch and non-vellum channel fallback - conversation-routes.ts: Fall back to session.guardianContext for verifiedActorExternalUserId and verifiedActorPrincipalId when actorVerification is null (non-vellum channels). - guardian-decision-primitive.ts: Allow cross-channel guardian decisions by checking actor principal against the assistant's canonical vellum binding principal when direct principal match fails. - guardian-reply-router.ts: Add guardianPrincipalId filter to the conversation fallback query in findPendingCanonicalRequests so guardians only see requests they are authorized to act on. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add cross-channel principal fallback to code-only clarification check Extract isAuthorizedGuardianPrincipal() shared helper from the inline cross-channel fallback logic in applyCanonicalGuardianDecision, and use it in the router's code-only clarification identity check. This ensures a desktop guardian entering a request code for a cross-channel request (e.g. Telegram-originated) sees the clarification context instead of getting "Request not found". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) (#11002) * feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) - Update 6 test files to use guardianPrincipalId principal-match pattern instead of legacy isTrusted boolean - Add guard test (no-is-trusted-guard.test.ts) to prevent isTrusted reintroduction in production code - Clean up 3 stale comments referencing trusted bypass and desktop/trusted with principal-based terminology - Verify no production isTrusted references remain (only allowed trust-class variable names: isTrustedActor, isTrustedContact, isTrustedTrustClass) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: correct guard test pipe and identity mismatch test field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use JS-level allowlist filtering in guard test to prevent masking --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix: address holistic review — decidedByPrincipalId, access_request exempt, backfill expiry, code lookup guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: make isAuthorizedGuardianPrincipal symmetric for cross-channel approval Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address reviewer feedback — binding check, pre-bootstrap IPC, migration access_request exclusion - isAuthorizedGuardianPrincipal: verify actor's principal belongs to an active guardian binding before allowing cross-channel approval, closing the asymmetric authorization gap (Devin #3/#4) - local-actor-identity: eagerly create vellum binding via ensureVellumGuardianBinding in pre-bootstrap IPC path so downstream decisionable requests always have a guardianPrincipalId (Devin #5) - Migration 126: exclude access_request from expiry sweep in steps 3a and 3c since access_request is non-decisionable and proceeds via the invite flow (Codex P2 #2) - Update roundtrip tests to supply guardianPrincipalId for decisionable tool_approval requests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: unify channel binding principals + fix test fixtures for IntegrityError guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: align migration registry checkpoint key with migration function (_v1 -> _v2) * fix: narrow CanonicalDecisionResult union before accessing resolverReplyText --------- Co-authored-by: Claude <noreply@anthropic.com>
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
1 participant
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.
Backfill guardianPrincipalId for existing channel_guardian_bindings and canonical_guardian_requests rows. Update guardian-vellum-migration to set principal on binding creation. Expire unresolvable pending requests. Part of #10955.