Skip to content

fix: restore forceStrictSideEffects for non-guardian/unverified channel actors#9174

Closed
awlevin wants to merge 1 commit into
mainfrom
fix/restore-strict-side-effects-for-non-guardian-actors
Closed

fix: restore forceStrictSideEffects for non-guardian/unverified channel actors#9174
awlevin wants to merge 1 commit into
mainfrom
fix/restore-strict-side-effects-for-non-guardian-actors

Conversation

@awlevin

@awlevin awlevin commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Restores the forceStrictSideEffects=true override for non-guardian and unverified_channel actors in prepareSessionForMessage (daemon server)
  • The migration from runs-store to pending-interactions (PR refactor: migrate channel approval system from runs-store to pending-interactions #8428) dropped this invariant, allowing these actors to potentially bypass forced-confirmation behavior and execute side-effect tools without guardian approval prompts
  • Mirrors the existing pattern in voice-session-bridge.ts where strict side-effects are forced for non-guardian actors

Test plan

  • Existing daemon-server-session-init tests pass
  • Existing session-tool-setup-side-effect-flag tests pass
  • Existing voice-session-bridge tests pass (16/16)
  • Type-check passes (no new errors introduced)
  • Manual verification: non-guardian actor on a channel triggers approval prompts for side-effect tools

🤖 Generated with Claude Code


Open with Devin

…el actors

The migration from runs-store to pending-interactions dropped the
forceStrictSideEffects=true override for non-guardian and
unverified_channel actors. Without this, these actors could execute
side-effect tools without guaranteed guardian approval prompts.

This fix adds the override in prepareSessionForMessage, mirroring the
pattern already used in voice-session-bridge.ts — when the guardian
context indicates a non-guardian or unverified_channel actor, the
session's memoryPolicy.strictSideEffects is set to true so all
side-effect tools require approval.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

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.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +774 to +779
if (actorRole === 'non-guardian' || actorRole === 'unverified_channel') {
session.memoryPolicy = {
...session.memoryPolicy,
strictSideEffects: true,
};
}

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.

🔴 strictSideEffects is never reset for guardian actors on a reused session

When a non-guardian or unverified_channel actor sends a message, strictSideEffects is set to true on the session's memoryPolicy. However, when a subsequent message arrives from a guardian actor (or with no actorRole), the code does not reset strictSideEffects back to its derived default. Since sessions are reused across messages for the same conversationId, the true value sticks.

Root cause and comparison with voice-session-bridge

The voice-session-bridge at assistant/src/calls/voice-session-bridge.ts:293-298 always computes and sets strictSideEffects for every turn, regardless of actor role:

const strictSideEffects = forceStrictSideEffects
    ?? deps.deriveDefaultStrictSideEffects(opts.conversationId);
session.memoryPolicy = {
    ...session.memoryPolicy,
    strictSideEffects,
};

When isGuardian is true, forceStrictSideEffects is undefined, so it falls back to the derived default (typically false for standard threads). This ensures strictSideEffects is properly reset for guardian actors.

In contrast, the new code in server.ts:774-779 only sets strictSideEffects conditionally:

if (actorRole === 'non-guardian' || actorRole === 'unverified_channel') {
    session.memoryPolicy = {
        ...session.memoryPolicy,
        strictSideEffects: true,
    };
}

There is no else branch to reset strictSideEffects to false (or its derived default). So if a non-guardian message is followed by a guardian message on the same session, the guardian will still have strictSideEffects: true, causing unnecessary approval prompts for side-effect tools.

Impact: Guardian actors on a session that previously received a non-guardian message will be incorrectly forced through approval prompts for every side-effect tool invocation.

Suggested change
if (actorRole === 'non-guardian' || actorRole === 'unverified_channel') {
session.memoryPolicy = {
...session.memoryPolicy,
strictSideEffects: true,
};
}
if (actorRole === 'non-guardian' || actorRole === 'unverified_channel') {
session.memoryPolicy = {
...session.memoryPolicy,
strictSideEffects: true,
};
} else {
session.memoryPolicy = {
...session.memoryPolicy,
strictSideEffects: this.deriveMemoryPolicy(conversationId).strictSideEffects,
};
}
Open in Devin Review

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ae126a52f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +774 to +778
if (actorRole === 'non-guardian' || actorRole === 'unverified_channel') {
session.memoryPolicy = {
...session.memoryPolicy,
strictSideEffects: true,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recompute strict side-effects for every actor role change

This branch only ever flips strictSideEffects to true, but never restores it when later turns come from a trusted actor. Because getOrCreateSession reuses the same Session object per conversation, once a non-guardian/unverified turn hits this path, the strict flag remains sticky for subsequent guardian turns in that conversation, forcing side-effect approvals unexpectedly and potentially blocking normal guardian actions.

Useful? React with 👍 / 👎.

@noanflaherty

Copy link
Copy Markdown
Contributor

Shelving project — closing all open PRs

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