Skip to content

refactor: migrate channel approval system from runs-store to pending-interactions#8428

Merged
awlevin merged 6 commits into
mainfrom
remove-runs/pr3
Feb 25, 2026
Merged

refactor: migrate channel approval system from runs-store to pending-interactions#8428
awlevin merged 6 commits into
mainfrom
remove-runs/pr3

Conversation

@awlevin
Copy link
Copy Markdown
Contributor

@awlevin awlevin commented Feb 24, 2026

Summary

PR 3/6 in the remove-runs-centralize-messages plan.

Migrates the channel approval system (Telegram, SMS) from runId + runs-store + RunOrchestrator to requestId + pending-interactions tracker + session.handleConfirmationResponse().

  • All runId references replaced with requestId across 21 files
  • Telegram callback buttons encode apr::
  • Guardian approval records use requestId (DB migration)
  • Channel inbound routes use session.processMessage() directly
  • Post-decision delivery simplified

Plan: .private/plans/REMOVE_RUNS_CENTRALIZE_MESSAGES.md


Open with Devin

…interactions

Co-Authored-By: Claude <noreply@anthropic.com>
@awlevin awlevin self-assigned this Feb 24, 2026
…elMessageWithApprovals)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

💡 Codex Review

assistantId,
guardianContext: toGuardianRuntimeContext(sourceChannel, guardianCtx),
...(cmdIntent ? { commandIntent: cmdIntent } : {}),

P1 Badge Keep strict side-effect prompting for guardian-gated users

The old approval-aware path forced forceStrictSideEffects=true for non-guardian/unverified_channel actors, but this new call into processMessage no longer carries any equivalent override. In the current runtime, tool prompting enforcement is driven by memoryPolicy.strictSideEffects (default false for standard threads), so these actors can now bypass the previous forced-confirmation behavior and execute side-effect tools without guaranteed guardian approval prompts.

ℹ️ 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 +667 to +670
// The onEvent callback in processMessage registers pending interactions, and
// approval interception (above) handles decisions via the pending-interactions tracker.
processChannelMessageInBackground({
processMessage,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P0 Badge Restore approval-event registration for channel ingress

This refactor routes webhook messages through processChannelMessageInBackground, but that path calls processMessage without any event hook; daemon/server.ts runs session.runAgentLoop(..., () => {}), so confirmation_request events are never registered in pending-interactions (registration currently happens in conversation-routes.ts via makeHubPublisher). When a channel message triggers a tool confirmation, handleApprovalInterception cannot find any pending request, so button/text approvals are treated as stale and the run can remain blocked waiting on a confirmation that users cannot complete.

Useful? React with 👍 / 👎.

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: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 6 additional findings.

Open in Devin Review

awlevin and others added 2 commits February 25, 2026 09:41
… channel files)

Main refactored channel-inbound-routes.ts into inbound-conversation.ts and
inbound-message-handler.ts, and channel-guardian-routes.ts into three
sub-files (guardian-approval-interception.ts, guardian-approval-prompt.ts,
guardian-expiry-sweep.ts). PR3's runId->requestId migration and RunOrchestrator
removal has been applied to all the new split files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	assistant/src/__tests__/channel-approval-routes.test.ts
#	assistant/src/__tests__/channel-approvals.test.ts
#	assistant/src/runtime/channel-approvals.ts
#	assistant/src/runtime/routes/guardian-approval-interception.ts
#	assistant/src/runtime/routes/guardian-expiry-sweep.ts
#	assistant/src/runtime/routes/inbound-message-handler.ts
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 1 new potential issue.

View 11 additional findings in Devin Review.

Open in Devin Review

export const channelGuardianApprovalRequests = sqliteTable('channel_guardian_approval_requests', {
id: text('id').primaryKey(),
runId: text('run_id').notNull(),
requestId: text('request_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.

🔴 Missing DB migration to add request_id column to channel_guardian_approval_requests table

The Drizzle schema definition adds requestId: text('request_id') to channelGuardianApprovalRequests, and the store layer (channel-guardian-store.ts) reads/writes this column, but no migration DDL adds it to the actual SQLite database.

Root Cause and Impact

The table is created by the migration at assistant/src/memory/migrations/110-channel-guardian.ts:49-67 with raw SQL CREATE TABLE IF NOT EXISTS channel_guardian_approval_requests (...) — this SQL does not include a request_id column. Other new columns (like assistant_id at line 74) are added via idempotent ALTER TABLE ... ADD COLUMN statements with try/catch, but there is no corresponding ALTER TABLE channel_guardian_approval_requests ADD COLUMN request_id TEXT anywhere in the migrations.

This means:

  1. Existing databases never get the column added — any query or insert involving request_id will fail with an SQL error.
  2. New databases also don't get the column, since the CREATE TABLE IF NOT EXISTS in the migration defines the actual DDL, not the Drizzle schema.

All new functions that query by this column will fail at runtime:

  • getPendingApprovalForRequest() at channel-guardian-store.ts:706
  • getUnresolvedApprovalForRequest() at channel-guardian-store.ts:748
  • getPendingApprovalByRequestAndGuardianChat() at channel-guardian-store.ts:844
  • createApprovalRequest() when requestId is passed at channel-guardian-store.ts:661

Impact: The guardian approval flow will crash with SQL errors whenever these code paths are exercised, breaking Telegram/SMS guardian approval decisions.

Prompt for agents
Add a migration to the channel guardian migration file at assistant/src/memory/migrations/110-channel-guardian.ts to add the request_id column to the channel_guardian_approval_requests table. Follow the existing pattern used for assistant_id at line 74 of that file. Add this line after the existing ALTER TABLE statements (around line 74):

try { database.run(/*sql*/ `ALTER TABLE channel_guardian_approval_requests ADD COLUMN request_id TEXT`); } catch { /* already exists */ }

This ensures existing databases get the column added, and new databases (where CREATE TABLE IF NOT EXISTS already ran) silently skip it.
Open in Devin Review

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

awlevin and others added 2 commits February 25, 2026 14:24
…MessageWithApprovals

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

- Channel inbound messages now register confirmation_request/secret_request
  events in pending-interactions via makePendingInteractionRegistrar, matching
  the pattern used in conversation-routes.ts via makeHubPublisher
- Add ALTER TABLE migration for request_id column on
  channel_guardian_approval_requests table

Addresses Codex P0 and Devin P0 review feedback on PR #8428.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@awlevin awlevin merged commit 215dcbb into main Feb 25, 2026
4 checks passed
@awlevin awlevin deleted the remove-runs/pr3 branch February 25, 2026 21:31
siddseethepalli added a commit that referenced this pull request Feb 25, 2026
…ration

PR #8428 removed runId from ApprovalPayload and switched callback_data
to use requestId, but the tests were not updated. This removes the
obsolete runId validation test and updates all approval test payloads
to match the new schema.

Co-Authored-By: Claude <noreply@anthropic.com>
siddseethepalli added a commit that referenced this pull request Feb 25, 2026
…ration (#9144)

PR #8428 removed runId from ApprovalPayload and switched callback_data
to use requestId, but the tests were not updated. This removes the
obsolete runId validation test and updates all approval test payloads
to match the new schema.

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