Keep confidential platform OAuth staging mistakes off Sentry - #1350
Merged
kody-bot merged 1 commit intoAug 9, 2026
Merged
Conversation
Throw PlatformOauthAppValidationError for caller-fixable upsert rejects, and re-wrap them as McpCallerError in admin_platform_oauth_app_save so agent enable-without-secret mistakes stay on mcp-event lines instead of opening Sentry triage issues. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR adds a typed platform OAuth validation error. The admin save handler converts these validation failures into ChangesPlatform OAuth validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
actor Admin as Admin MCP caller
participant SaveHandler as admin-platform-oauth-app-save
participant Upsert as upsertPlatformOauthApp
participant McpError as McpCallerError
Admin->>SaveHandler: Save platform OAuth app
SaveHandler->>Upsert: Upsert OAuth app
Upsert-->>SaveHandler: PlatformOauthAppValidationError
SaveHandler->>McpError: Wrap validation error as caller error
SaveHandler-->>Admin: McpCallerError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
kentcdodds
marked this pull request as ready for review
August 9, 2026 22:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-capabilities.node.test.ts`:
- Around line 201-205: Remove the duplicate `.all()` invocation from the
auditSqlite query in the failures initialization, so the prepared statement
calls `.all()` exactly once and its returned array is directly cast to the
expected type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 966d545a-f4c8-4123-806d-c5108e83c3e7
📒 Files selected for processing (4)
packages/worker/src/integrations/platform-apps.node.test.tspackages/worker/src/integrations/platform-apps.tspackages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-save.ts
Comment on lines
+201
to
+205
| const failures = auditSqlite | ||
| .prepare( | ||
| `SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`, | ||
| ) | ||
| .all() as Array<{ action: string; result: string }> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate .all() call.
Line 205 calls .all() on the array returned by the call above. Array has no .all() method. This prevents the test file from type checking.
Proposed fix
const failures = auditSqlite
.prepare(
`SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`,
)
.all() as Array<{ action: string; result: string }>
- .all() as Array<{ action: string; result: string }>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const failures = auditSqlite | |
| .prepare( | |
| `SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`, | |
| ) | |
| .all() as Array<{ action: string; result: string }> | |
| const failures = auditSqlite | |
| .prepare( | |
| `SELECT action, result FROM audit_events WHERE result = 'failure' ORDER BY id ASC`, | |
| ) | |
| .all() as Array<{ action: string; result: string }> |
🤖 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
`@packages/worker/src/mcp/capabilities/admin/admin-platform-oauth-app-capabilities.node.test.ts`
around lines 201 - 205, Remove the duplicate `.all()` invocation from the
auditSqlite query in the failures initialization, so the prepared statement
calls `.all()` exactly once and its returned array is directly cast to the
expected type.
Contributor
|
🔎 Preview deployed: https://kody-pr-1350.kody-a99.workers.dev Worker: Mocks:
|
kody-bot
deleted the
cursor/sentry-triage-kody-cloudflare-7662130627-a9fe
branch
August 9, 2026 22:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #1345 / Sentry KODY-CLOUDFLARE-4G: classify caller-fixable
upsertPlatformOauthApprejects asPlatformOauthAppValidationError, and re-wrap them asMcpCallerErrorinadmin_platform_oauth_app_save.Why
The single production event was an agent enabling a confidential platform app without a client secret (release before #1345). Staging without a secret while
enabled: falsealready works; enabling without a secret is still a valid reject, but it must not open Sentry issues that look like platform bugs and trip triage.Testing
PlatformOauthAppValidationErrorMcpCallerErrorfor enable-without-secret (default and staged→enable)System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@9faafa09· Head:307b0445Classification: composes — wires the existing
McpCallerError/ caller-failure observability path onto platform OAuth app input validation; no new primitives or auth semantics.Primitives touched
integrationsrbacMcpCallerErrorSystem map
Admin MCP save validates platform OAuth apps and keeps staging mistakes off Sentry.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Summary by CodeRabbit