fix(ai): make GitHub adapter suppression authorization-aware#1922
fix(ai): make GitHub adapter suppression authorization-aware#19222witstudios wants to merge 3 commits into
Conversation
Non-admin users in production had no working GitHub surface at all. buildPageSpaceTools registers the sandbox git/gh CLI toolkit for every user whenever the CODE_EXECUTION_ENABLED kill-switch is on, with no per-user admin check (that check lives entirely in canRunCode). But suppressGithubIntegrationTools suppressed the always-working GitHub OAuth integration tools whenever those sandbox tool *names* were merely present in the resolved tool set, without checking whether the caller could actually invoke them. For a non-admin, the sandbox tools were "present" (triggering suppression) but denied at execute-time by canRunCode's app_admin_required check — leaving no GitHub surface at all. suppressGithubIntegrationTools now calls canRunCode and only suppresses the OAuth tools when the sandbox toolkit is both present and actually authorized for this caller. Threaded requestOrigin/agentPageId through the two resolvers and the one agent-to-agent (ask_agent) call site that needs 'agent' origin; the other four call sites already default correctly to 'user' origin. The production admin-only gate in can-run-code.ts is untouched.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughGitHub integration tool suppression in the AI tool-filtering module is changed from synchronous presence-based logic to an async, authorization-gated check using ChangesAuthorization-gated GitHub tool suppression
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ask_agent
participant resolvePageAgentIntegrationTools
participant suppressGithubIntegrationTools
participant canRunCode
ask_agent->>ask_agent: derive agentPageId (chatSource.agentPageId ?? callingPage.id)
ask_agent->>resolvePageAgentIntegrationTools: currentTools, requestOrigin, agentPageId
resolvePageAgentIntegrationTools->>suppressGithubIntegrationTools: integrationTools, currentTools, { userId, driveId, requestOrigin, agentPageId }
suppressGithubIntegrationTools->>canRunCode: canRunCode(auth)
canRunCode-->>suppressGithubIntegrationTools: { ok, reason? }
alt authorized
suppressGithubIntegrationTools-->>resolvePageAgentIntegrationTools: filtered tools (GitHub suppressed)
else denied
suppressGithubIntegrationTools-->>resolvePageAgentIntegrationTools: unfiltered tools (GitHub kept)
end
resolvePageAgentIntegrationTools-->>ask_agent: final tool set
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Self-review caught a real bug in the original fix: ask_agent's new canRunCode-backed suppression check passed `agentPageId: agentId` (the consulted TARGET agent), but resolveSandboxActorContext — the code path that actually authorizes real sandbox tool execution — always resolves agentPageId from `chatSource.agentPageId` (falling back to `parentAgentId`), both of which are the CALLING agent's identity, never the target's. Nested ask_agent calls never override chatSource, so this is true at any nesting depth. Concretely: if the calling agent lacks drive edit access but the consulted target agent has it, the new suppression check (using the target's identity) would authorize and hide GitHub OAuth tools, while the real invocation-time check (using the caller's identity, per existing codebase convention — see actor-permissions.ts's getAgentPageId/resolveActingAgentId, which read the same chatSource.agentPageId field for the same reason) would deny the sandbox tool. Net effect: the target agent loses both GitHub surfaces — exactly the bug this PR exists to fix, reintroduced via the one path this PR touches. Fix: pass `executionContext?.chatSource?.agentPageId ?? executionContext?.locationContext?.currentPage?.id`, mirroring resolveSandboxActorContext's own formula exactly, so the suppression decision always matches the real authorization outcome. Also: clarify tool-filtering.ts's module header and JSDoc (the file's "simple, no DB" self-description no longer fully holds for this one function — flag it so a future simplification pass doesn't silently revert the fix), and strengthen resolver tests to assert the exact canRunCode call args (userId/driveId/requestOrigin/agentPageId), not just suppress/keep outcomes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Proactive /simplify pass on the previous two commits, informed by a 4-angle parallel review (reuse, simplification, efficiency, altitude): - tool-filtering.ts: suppressGithubIntegrationTools's `auth` param was a hand-copied subset of CanRunCodeInput (minus `deps`), then the body destructured and rebuilt it field-by-field just to call canRunCode. Now typed as `Omit<CanRunCodeInput, 'deps'>` and forwarded directly (`canRunCode(auth)`) — one less place to keep in sync if CanRunCodeInput's shape ever changes. - agent-communication-tools.ts: the previous commit's agentPageId fix independently re-derived `executionContext?.locationContext?.currentPage?.id` inline, duplicating both the pre-existing `callingPage` local (declared earlier in the same function) and nestedContext's own parentAgentId assignment a few lines below. Both call sites now reuse `callingPage?.id`, removing the second within-function copy of the same expression and tightening the invariant the fix's comment describes from "two independently-written expressions that happen to agree" to "one shared value used twice." Considered and skipped (would expand scope beyond a proactive cleanup pass): extracting a new cross-file helper for the agentPageId formula (the altitude review confirmed this would be misleading — it isn't really the same formula as resolveSandboxActorContext's, just a coincidentally-equivalent derivation tied to this file's own nestedContext construction); making the grants fetch and canRunCode check run concurrently in the two resolvers (would fire the DB-backed auth check unconditionally even when grants turn out to be empty, trading a latency win in the sandbox-active case for wasted queries in the common no-integrations case — not a clear improvement); and extracting a shared canRunCode-mock test fixture (two lines duplicated across two test files, not worth a new fixture file). No behavior change — typecheck clean, all 93 tests across the four touched test files still pass, full apps/web suite green (11645/11646, one pre-existing unrelated failure).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/lib/ai/core/integration-tool-resolver.ts (1)
89-126: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid the duplicate
canRunCodelookup inapps/web/src/lib/ai/core/integration-tool-resolver.ts. Both resolver paths hide GitHub tools withsuppressGithubIntegrationTools(...), and the sandbox tool gate runs the same auth again at execution time, so sandbox-tool requests pay an extra DB round-trip. Thread the result through if the later gate uses the same inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/ai/core/integration-tool-resolver.ts` around lines 89 - 126, The resolver in resolvePageAgentIntegrationTools is doing an extra canRunCode-driven authorization check for sandbox GitHub tools even though suppressGithubIntegrationTools already applies the same gate, causing a redundant DB round-trip. Update the integration-tool path so the canRunCode result is computed once and threaded through to the later sandbox/tool-exposure check, reusing the existing inputs (userId, driveId, requestOrigin, agentPageId) instead of re-looking it up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/src/lib/ai/core/integration-tool-resolver.ts`:
- Around line 89-126: The resolver in resolvePageAgentIntegrationTools is doing
an extra canRunCode-driven authorization check for sandbox GitHub tools even
though suppressGithubIntegrationTools already applies the same gate, causing a
redundant DB round-trip. Update the integration-tool path so the canRunCode
result is computed once and threaded through to the later sandbox/tool-exposure
check, reusing the existing inputs (userId, driveId, requestOrigin, agentPageId)
instead of re-looking it up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8de63400-8093-4ceb-a7bf-db00b98af59c
📒 Files selected for processing (6)
apps/web/src/lib/ai/core/__tests__/integration-tool-resolver.test.tsapps/web/src/lib/ai/core/__tests__/tool-filtering.test.tsapps/web/src/lib/ai/core/integration-tool-resolver.tsapps/web/src/lib/ai/core/tool-filtering.tsapps/web/src/lib/ai/tools/__tests__/agent-communication-tools.test.tsapps/web/src/lib/ai/tools/agent-communication-tools.ts
|
Addressing CodeRabbit's finding on Confirmed and correct as an observation — this was flagged and consciously accepted before this review ran (see the "Known trade-offs" section of the PR description, added prior to this comment). Not threading the result through is intentional, for two reasons:
No code change made. Flagging as a legitimate, separately-scoped follow-up if the added DB load ever proves material in practice (it's bounded to requests whose tool set includes the sandbox git/gh toolkit, gated behind |
1 participant
Summary
Non-admin users had no working GitHub surface in production at all — a side effect of two independently-correct pieces of code colliding:
buildPageSpaceTools(apps/web/src/lib/ai/core/ai-tools.ts) registers the sandbox git/gh CLI toolkit for every user whenever the globalCODE_EXECUTION_ENABLEDenv kill-switch is on, gated only by the env flag, with no per-user check.suppressGithubIntegrationTools(apps/web/src/lib/ai/core/tool-filtering.ts, added in Resolve GitHub integration/sandbox tool overlap #1879 to de-dupe the two GitHub surfaces) hid the always-working GitHub OAuth integration tools whenever the sandbox git/gh tool names were merely present in the resolved tool set — it never checked whether those tools would actually be authorized for this caller.The real per-user authorization for sandbox code execution lives entirely in
canRunCode(packages/lib/src/services/sandbox/can-run-code.ts), which in production requires app-admin (app_admin_required, from #1664's deliberate Fly Sprites rollout guard). That gate is untouched by this PR.Net effect for a non-admin in production: sandbox tools are "present" (triggering suppression) but denied at execute-time by
canRunCode— leaving no GitHub surface at all.Fix
suppressGithubIntegrationToolsis nowasyncand callscanRunCode({ userId, driveId, requestOrigin, agentPageId }). It only suppresses the GitHub OAuth tools when the sandbox toolkit is both present and actually authorized for this caller right now. This is a listing/UX decision, not a new security boundary — it never grants sandbox access, only decides which already-permitted-or-not surface to show.resolvePageAgentIntegrationTools/resolveGlobalAssistantIntegrationToolsgained optionalrequestOrigin/agentPageIdparams, threaded into the now-awaited suppression call.ask_agenttool's nested consult call (the one agent-to-agent path) now passesrequestOrigin: 'agent'andagentPageIdderived fromchatSource.agentPageId ?? locationContext.currentPage.id— not the consulted target agent's own ID. This matters:resolveSandboxActorContext(the code path that authorizes real sandbox tool execution) always resolvesagentPageIdfrom the calling agent'schatSource/parentAgentId, never the target's, and nestedask_agentcalls never overridechatSourceat any depth. An earlier version of this fix passed the target agent's ID here, which would have let the suppression decision diverge from the real execute-time authorization outcome — reintroducing this exact bug via the one path this PR touches. Caught and fixed via self-review before merge; see the second commit for the full trace.'user'is already the correct default for them (verifiedpage-agents/consult/route.tsis a direct user/MCP-token entry point, structurally equivalent to the chat route, not an agent-to-agent path — it does not needrequestOrigin: 'agent').Test plan
bun run --filter web typecheckpassesapps/webtest suite passes against a real Postgres test DB (790/791 files, 11641/11642 tests) — the one pre-existing failure (grouping.test.tsmidnight-boundary test) is untouched by this diff and unrelated (message-grouping display logic)tool-filtering.test.tsandintegration-tool-resolver.test.tsboth cover the "sandbox tools present but not authorized → GitHub NOT suppressed" regression case for both resolvers, plus explicit assertions on the exactcanRunCodecall args (userId/driveId/requestOrigin/agentPageId)agent-communication-tools.test.tscovers theagentPageIdidentity fix directly: assertsresolvePageAgentIntegrationToolsis called with the calling agent'schatSource.agentPageId(orlocationContext.currentPage.idfallback), never the consulted target's IDpackages/lib/src/services/sandbox/can-run-code.tsis completely untouched (git diffconfirms zero changes)git diff --statscoped to:tool-filtering.ts,integration-tool-resolver.ts,agent-communication-tools.ts, and their test files — no unrelated refactorsKnown trade-offs (accepted, not blocking)
canRunCode's DB calls error transiently, it returns{ok: false, reason: 'error'}, and suppression treats that the same as "not authorized" — i.e. it fails open by not suppressing GitHub tools for that one turn. This is the safe direction (matches this PR's whole goal: never leave a user with zero GitHub surface) but means tool-set membership can very rarely vary turn-to-turn under DB flakiness, not just on real config changes.canRunCode's DB queries (permissions/role lookups) during tool-list resolution, in addition to the pre-existing execute-time check. This was a deliberate, explicit instruction for this fix (reusecanRunCodedirectly, avoid the heavierresolveSandboxActorContext) — an optimization to avoid double-querying would touchcan-run-code.tsor call-site plumbing beyond this PR's scope.https://claude.ai/code/session_01J3NZDRTyLssfCupNLHRgEr
Summary by CodeRabbit