Skip to content

Apply access policy when reusing channel agents - #6838

Open
jmecom wants to merge 3 commits into
mainfrom
jm/fix-owner-only-agent-reuse
Open

Apply access policy when reusing channel agents#6838
jmecom wants to merge 3 commits into
mainfrom
jm/fix-owner-only-agent-reuse

Conversation

@jmecom

@jmecom jmecom commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Reused managed identities now resolve their inbound-author policy with the same precedence as a newly created agent: an explicit instance policy wins, then the linked persona's policy, then owner-only.

The policy is applied in both reuse paths. Channel-agent provisioning updates the identity before attachment. The message composer now does the same when autocomplete selects an existing identity outside the channel, and it updates a newly added conversation participant before starting it or sending the message. A failed policy update stops the remaining operation.

The production Playwright regression starts with a persona-linked agent set to anyone with a stale allowlist. It drives autocomplete and message send, verifies update_managed_agent happens before membership and start, and confirms the stored policy is owner-only after the message is sent. Unit tests also cover generic reuse, persona defaults for owner-only, anyone, and allowlist, explicit overrides, matching-policy no-ops, and update failure.

This closes the stale-policy entry described by Project Loupe issue #497. The separate Welcome Team reuse path is outside this PR.

Checked with the 5,506-test desktop suite, pnpm check, pnpm typecheck, pnpm check:file-sizes, and the focused Playwright composer regression.

Signed-off-by: Jordan Mecom <jm@squareup.com>
@jmecom
jmecom marked this pull request as ready for review August 25, 2026 22:58
@jmecom
jmecom requested a review from a team as a code owner August 25, 2026 22:58

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I traced the reuse path at the exact PR head and combined that source review with the headless composer run. There is one IMPORTANT correctness/security blocker: production callers that omit respondTo still preserve a reused agent’s broad policy, so the stated fix does not hold in the real workflow. The new seam test is mutation-sensitive, and completed CI lanes are green, but its explicit owner-only input bypasses the production failure.

// choice in the dialog is always honored, even when reusing.
const needsRespondToUpdate =
input.respondTo && input.respondTo !== "owner-only";
const needsRespondToUpdate = input.respondTo !== undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMPORTANT — Correctness / security: this condition still skips the update for the real production callers. useMentionSendFlow.ts, useQuickBotDrop.ts, and template application omit respondTo; omission means the safe owner-only default when creating, but this branch treats it as “preserve the reused agent’s policy.” In the headless composer run at fa8c0c550, an out-of-channel agent seeded with respond_to: "anyone" produced no update_managed_agent call and remained open through attach/start/send. I independently traced the same omission across those callsites.

Please derive the effective mode here as input.respondTo ?? "owner-only", compare the mode and allowlist against the reusable agent, and issue the update before reuse when either differs. For non-allowlist modes, send respondToAllowlist: [] so dormant authorization does not survive. Cover omitted respondTo for both persona and generic reuse; the current test passes owner-only explicitly and therefore bypasses this defect.

Signed-off-by: Jordan Mecom <jm@squareup.com>
@jmecom
jmecom requested a review from wpfleger96 August 25, 2026 23:53

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I re-reviewed the author’s fix at exact head 1afb0accbb90239c0cbed09ab4acc54393495243. The helper test is mutation-sensitive and CI is green, but the real composer identity-reuse path still bypasses this helper. Independent headless verification reproduced the stale broad policy after attach/start/send, so the original IMPORTANT correctness/security defect remains.

) {
// Creation defaults an omitted mode to owner-only. Reuse must apply the
// same default instead of inheriting the agent's previous access policy.
const respondTo = input.respondTo ?? "owner-only";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMPORTANT — Correctness/Security: This fallback is not reached by the production path that prompted the fix. An existing managed-agent autocomplete result is handled by ensureManagedAgentMentionsReady() in desktop/src/features/messages/ui/useMentionSendFlow.ts:128-171, which directly attaches/starts the identity without calling provisionChannelManagedAgent(). In an exact-head composer run, an out-of-channel persona-linked agent seeded as anyone plus a stale allowlist emitted no update_managed_agent and remained broad after attach/start/send. The new tests call this helper seam, so they do not catch that bypass. Please enforce the intended gate in the already-managed identity attach path and add a production-caller regression. Also resolve omitted policy using the same definition-aware semantics as fresh minting: this unconditional owner-only fallback diverges for linked personas whose stored default is anyone or allowlist.

Signed-off-by: Jordan Mecom <jm@squareup.com>
@jmecom jmecom changed the title Honor owner-only policy when reusing channel agents Apply access policy when reusing channel agents Aug 26, 2026
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.

2 participants