Skip to content

feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961)#11002

Merged
noanflaherty merged 3 commits into
feature/canonical-id-binding-cutoverfrom
swarm/canonical-id-bind/task-6
Mar 1, 2026
Merged

feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961)#11002
noanflaherty merged 3 commits into
feature/canonical-id-binding-cutoverfrom
swarm/canonical-id-bind/task-6

Conversation

@noanflaherty
Copy link
Copy Markdown
Contributor

@noanflaherty noanflaherty commented Mar 1, 2026

Summary

Final milestone of the canonical-identity-binding cutover. Migrates all test files from the legacy isTrusted boolean pattern to guardianPrincipalId principal-match authorization, adds a guard test to prevent reintroduction, and cleans up stale comments.

Changes

Test updates (6 files)

  • guardian-decision-primitive-canonical.test.ts: Full rewrite — added TEST_PRINCIPAL_ID constant, updated guardianActor()/trustedActor() helpers, added guardianPrincipalId to all createCanonicalGuardianRequest calls for decisionable kinds, updated test descriptions
  • guardian-routing-invariants.test.ts: Full rewrite — same principal-match pattern, updated section titles and test descriptions
  • guardian-actions-endpoint.test.ts: Updated 2 assertions from expect(actorContext.isTrusted).toBe(true) to expect(actorContext.guardianPrincipalId).toBeDefined()
  • trusted-contact-inline-approval-integration.test.ts: Updated guardianActor() helper and all createCanonicalGuardianRequest calls
  • tool-grant-request-escalation.test.ts: Updated guardianActor() helper and all 7 createCanonicalGuardianRequest calls
  • tool-approval-handler.test.ts: Updated stale test description

Guard test (1 new file)

  • no-is-trusted-guard.test.ts: Scans production code for isTrusted references (excluding tests, node_modules, and allowed trust-class variables). Verifies ActorContext interface does not declare isTrusted field.

Comment cleanup (2 production files)

  • guardian-request-resolvers.ts: (undefined for desktop/trusted)(undefined for desktop actors)
  • guardian-decision-primitive.ts: there is no trusted bypassprincipal identity must always match

Verification

  • grep confirms zero isTrusted references in production code (excluding allowed patterns)
  • ActorContext interface confirmed clean of isTrusted field

Closes #10961


Open with Devin

)

- 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>
@noanflaherty noanflaherty self-assigned this Mar 1, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@noanflaherty
Copy link
Copy Markdown
Contributor Author

@codex review this PR again — the previous issues have been fixed in commit 2eaada3

@noanflaherty
Copy link
Copy Markdown
Contributor Author

@devin review this PR again — the previous issues have been fixed in commit 2eaada3

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@noanflaherty noanflaherty merged commit 62ed16f into feature/canonical-id-binding-cutover Mar 1, 2026
1 check passed
@noanflaherty noanflaherty deleted the swarm/canonical-id-bind/task-6 branch March 1, 2026 05:18
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 10 additional findings in Devin Review.

Open in Devin Review

externalUserId: 'guardian-1',
channel: 'telegram',
isTrusted: false,
guardianPrincipalId: 'test-principal-id',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Mock guardian binding missing guardianPrincipalId causes end-to-end approval tests to fail with identity_mismatch

The guardianActor() helper was updated to include guardianPrincipalId: 'test-principal-id', but the mock getGuardianBinding return value (lines 81-88) was NOT updated to include guardianPrincipalId. This creates a fatal mismatch in end-to-end tests that go through the handler path.

Root Cause and Impact

When checkPreExecutionGates creates a tool_grant_request via createOrReuseToolGrantRequest (assistant/src/runtime/tool-grant-request-helper.ts:126), it reads binding.guardianPrincipalId ?? undefined from the mock binding. Since the mock binding has no guardianPrincipalId field, the canonical request is created with guardianPrincipalId: null.

Later, when the test calls applyCanonicalGuardianDecision with actorContext: guardianActor() (which has guardianPrincipalId: 'test-principal-id'), the decision primitive at assistant/src/approvals/guardian-decision-primitive.ts:391-401 checks:

if (!request.guardianPrincipalId) {
    return { applied: false, reason: 'identity_mismatch' };
}

Since the request's guardianPrincipalId is null, the approval is rejected. No grant is minted, the inline wait times out, and end-to-end tests like "inline wait: guardian approves during wait -> tool proceeds inline" (line 456) and "pre-existing grant from prior approval is consumed immediately" (line 499) will fail because result.allowed is false instead of true.

Prompt for agents
In assistant/src/__tests__/tool-grant-request-escalation.test.ts, the mock getGuardianBinding at lines 78-91 needs to include guardianPrincipalId in the returned binding object. Update the mock return value at line 81-88 to add guardianPrincipalId: 'test-principal-id' to the binding object, so that requests created through createOrReuseToolGrantRequest will have a guardianPrincipalId that matches the test actor's principal.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

externalUserId: 'guardian-1',
channel: 'telegram',
isTrusted: false,
guardianPrincipalId: 'test-principal-id',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Mock guardian binding missing guardianPrincipalId causes end-to-end inline approval tests to fail

The guardianActor() helper was updated to include guardianPrincipalId: 'test-principal-id', but the mockGuardianBinding object set in beforeEach blocks (e.g. lines 256-263) was NOT updated to include guardianPrincipalId. This causes the same mismatch as in tool-grant-request-escalation.test.ts.

Root Cause and Impact

The mockGuardianBinding at line 256-263 is:

mockGuardianBinding = {
    id: 'binding-1',
    assistantId: 'self',
    channel: 'telegram',
    guardianExternalUserId: 'guardian-1',
    guardianDeliveryChatId: 'guardian-chat-1',
    status: 'active',
    // guardianPrincipalId is MISSING
};

When checkPreExecutionGates creates a tool_grant_request via createOrReuseToolGrantRequest (assistant/src/runtime/tool-grant-request-helper.ts:126), it reads binding.guardianPrincipalId ?? undefined, which resolves to undefined → stored as null in the DB.

When the test later calls applyCanonicalGuardianDecision with guardianActor() (which now has guardianPrincipalId: 'test-principal-id'), the decision primitive at assistant/src/approvals/guardian-decision-primitive.ts:391-401 rejects because the request has no guardianPrincipalId. This breaks tests like "trusted contact requests tool, guardian approves mid-wait, tool executes inline" (line 266) and "complete flow" (line 314).

Prompt for agents
In assistant/src/__tests__/trusted-contact-inline-approval-integration.test.ts, the mockGuardianBinding objects set in all beforeEach blocks (e.g. lines 256-263, and similar blocks in other describe sections) need to include guardianPrincipalId: 'test-principal-id'. Without this, requests created through the handler's createOrReuseToolGrantRequest path will have guardianPrincipalId: null, and all end-to-end approval flows will fail with identity_mismatch.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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>
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.

1 participant