fix(types): validate unsafe websocket casts#823
Conversation
open-inspect
Bot
added
the
automation:unsafe-cast
📝 WalkthroughWalkthroughThis PR replaces compile-time TypeScript types with Zod runtime schemas for ChangesClientMessage/Attachment Zod Schemas and Durable Object Validation
Slack Notify Zod Schemas and Web Component Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: one or more packages not found in the registry. 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #823, fix(types): validate unsafe websocket casts, by @ColeMurray replaces unsafe boundary casts with shared Zod schemas for websocket messages and Slack notify envelopes. Reviewed 7 changed files (+169/-58); the implementation looks consistent with the existing producers/consumers and improves trust-boundary validation without introducing blocking issues.
Critical Issues
None found.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The new
clientMessageSchemaand Slack notify envelope schemas make the runtime contract the source of truth for the corresponding TypeScript types. - The Durable Object websocket handlers now fail closed on malformed client and sandbox messages instead of dispatching unchecked data.
- Focused schema tests cover valid and malformed client/Slack envelope cases.
Questions
None.
Verification
npm test -w @open-inspect/shared: 226 tests passed.npm run build -w @open-inspect/shared: passed.
Verdict
Approve.
| private async handleSandboxMessage(ws: WebSocket, message: string): Promise<void> { | ||
| try { | ||
| const event = JSON.parse(message) as SandboxEvent; | ||
| const result = sandboxEventSchema.safeParse(JSON.parse(message)); |
There was a problem hiding this comment.
[deep review] This turns the Zod schema into a yes/no gate and then throws away the useful boundary failure details. Since this PR is establishing the parse-dont-assert pattern for websocket trust boundaries, can we avoid copying this inline pattern into each handler and instead use a small shared parse helper that returns/logs the schema issues with the boundary name? Otherwise future schema drift between the sandbox/client and control plane becomes a generic Invalid sandbox event / Invalid client message with no actionable shape information, and every new websocket boundary is likely to grow the same ad-hoc parse/throw block.
| | { type: "typing" } | ||
| | { type: "presence"; status: "active" | "idle"; cursor?: { line: number; file: string } } | ||
| | { type: "fetch_history"; cursor: { timestamp: number; id: string }; limit?: number }; | ||
| export const clientMessageSchema = z.discriminatedUnion("type", [ |
There was a problem hiding this comment.
[deep review] This shared index is already 865 lines and is now taking on more runtime boundary-validation logic, not just type exports. Can we decompose the websocket/boundary schemas into a focused module and re-export them from here instead? This keeps the contract reusable without turning the catch-all shared types file into the canonical home for every future Zod schema, which is exactly how this file crosses the 1k-line threshold and becomes harder to scan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/shared/src/slack/types.ts`:
- Around line 68-76: The Slack notification schema currently exposes a duration
field named retryAfter without its unit, so rename the field in
slackNotifyToolEnvelopeSchema to encode that it is in seconds and update all
consumers to match. Keep the discriminated union shape in types.ts consistent,
then adjust downstream mappings such as slack-notify-event.tsx so the new schema
field feeds the existing retryAfterSeconds prop instead of propagating the
ambiguous name.
In `@packages/shared/src/types/index.ts`:
- Around line 338-341: The fetch_history schema in the shared types is stricter
than the handler contract: the current z.object for the fetch_history payload
requires cursor, which prevents handleFetchHistory from reaching its existing
data.cursor? INVALID_CURSOR path. Update the fetch_history type definition to
match the handler’s optional cursor behavior, or make the handler
signature/error handling consistent with the required cursor contract in the
same change, using the fetch_history and handleFetchHistory symbols to keep both
aligned.
🪄 Autofix (Beta)
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
Run ID: 0a6d0169-adb2-41c5-a912-2fcbbdd78134
📒 Files selected for processing (7)
packages/control-plane/src/session/durable-object.tspackages/shared/src/slack/index.tspackages/shared/src/slack/types.test.tspackages/shared/src/slack/types.tspackages/shared/src/types/boundary-schemas.test.tspackages/shared/src/types/index.tspackages/web/src/components/slack-notify-event.tsx
| export const slackNotifyToolEnvelopeSchema = z.discriminatedUnion("ok", [ | ||
| slackNotifySuccessOutputSchema, | ||
| z.object({ | ||
| ok: z.literal(false), | ||
| reason: slackDenialReasonSchema, | ||
| agentMessage: z.string(), | ||
| retryAfter: z.number().optional(), | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Encode the duration unit in the retryAfter field name.
retryAfter is a duration but its name omits the unit. The web component already maps it to a retryAfterSeconds prop (slack-notify-event.tsx Line 131), confirming the value is in seconds. Since this field is being threaded through a new schema path, rename it here (the root) and update consumers.
♻️ Proposed rename
z.object({
ok: z.literal(false),
reason: slackDenialReasonSchema,
agentMessage: z.string(),
- retryAfter: z.number().optional(),
+ retryAfterSeconds: z.number().optional(),
}),Downstream in slack-notify-event.tsx:
- retryAfterSeconds={envelopeDenial?.retryAfter}
+ retryAfterSeconds={envelopeDenial?.retryAfterSeconds}As per coding guidelines: "encode the unit in variable names" and "When threading existing fields through new code paths in TypeScript, evaluate whether the existing design (naming, types, units) is correct and fix bad names or units in the same change rather than propagating problems".
📝 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.
| export const slackNotifyToolEnvelopeSchema = z.discriminatedUnion("ok", [ | |
| slackNotifySuccessOutputSchema, | |
| z.object({ | |
| ok: z.literal(false), | |
| reason: slackDenialReasonSchema, | |
| agentMessage: z.string(), | |
| retryAfter: z.number().optional(), | |
| }), | |
| ]); | |
| export const slackNotifyToolEnvelopeSchema = z.discriminatedUnion("ok", [ | |
| slackNotifySuccessOutputSchema, | |
| z.object({ | |
| ok: z.literal(false), | |
| reason: slackDenialReasonSchema, | |
| agentMessage: z.string(), | |
| retryAfterSeconds: z.number().optional(), | |
| }), | |
| ]); |
🤖 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/shared/src/slack/types.ts` around lines 68 - 76, The Slack
notification schema currently exposes a duration field named retryAfter without
its unit, so rename the field in slackNotifyToolEnvelopeSchema to encode that it
is in seconds and update all consumers to match. Keep the discriminated union
shape in types.ts consistent, then adjust downstream mappings such as
slack-notify-event.tsx so the new schema field feeds the existing
retryAfterSeconds prop instead of propagating the ambiguous name.
Source: Coding guidelines
| z.object({ | ||
| type: z.literal("fetch_history"), | ||
| cursor: z.object({ timestamp: z.number(), id: z.string() }), | ||
| limit: z.number().optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep fetch_history aligned with the handler contract.
Line 340 rejects omitted cursors before handleFetchHistory can return its existing INVALID_CURSOR response for data.cursor?. Either make cursor optional here to preserve that path, or tighten the handler signature/error path in the same change.
Proposed contract-preserving fix
z.object({
type: z.literal("fetch_history"),
- cursor: z.object({ timestamp: z.number(), id: z.string() }),
+ cursor: z.object({ timestamp: z.number(), id: z.string() }).optional(),
limit: z.number().optional(),
}),📝 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.
| z.object({ | |
| type: z.literal("fetch_history"), | |
| cursor: z.object({ timestamp: z.number(), id: z.string() }), | |
| limit: z.number().optional(), | |
| z.object({ | |
| type: z.literal("fetch_history"), | |
| cursor: z.object({ timestamp: z.number(), id: z.string() }).optional(), | |
| limit: z.number().optional(), |
🤖 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/shared/src/types/index.ts` around lines 338 - 341, The fetch_history
schema in the shared types is stricter than the handler contract: the current
z.object for the fetch_history payload requires cursor, which prevents
handleFetchHistory from reaching its existing data.cursor? INVALID_CURSOR path.
Update the fetch_history type definition to match the handler’s optional cursor
behavior, or make the handler signature/error handling consistent with the
required cursor contract in the same change, using the fetch_history and
handleFetchHistory symbols to keep both aligned.
1 participant
This is an automated nightly unsafe-cast remediation. It replaces selected high-risk TypeScript assertions at trust boundaries with Zod
safeParsevalidation, following the TypeScript Coding Standards unsafe-cast / parse-don't-assert guidance and the Zod boundary-validation pattern established in PR #807.packages/control-plane/src/session/durable-object.ts:1126JSON.parse(message) as SandboxEventfor sandbox WebSocket events, bypassing the existingsandboxEventSchemasandboxEventSchema.safeParsebeforeprocessSandboxEventpackages/control-plane/src/session/durable-object.ts:1140JSON.parse(message) as ClientMessagefor client WebSocket messagesclientMessageSchema/z.infertype source and parse withsafeParsebefore dispatchpackages/web/src/components/slack-notify-event.tsx:58obj as unknown as SlackNotifySuccessOutput/obj as unknown as ParsedDenialdouble-casts for parsed tool outputslackNotifyToolEnvelopeSchema.safeParseVerification:
npm run build -w @open-inspect/sharednpm run build -w @open-inspect/control-planeNODE_ENV=production npm run build -w @open-inspect/webnpm run typechecknpm run lint.opencode/tooling directory out of the workspace so the clean-repo lint target was evaluatednpm run formatnpm test -w @open-inspect/sharednpm test -w @open-inspect/control-planenpm test -w @open-inspect/webCreated with Open-Inspect
Summary by CodeRabbit
New Features
Bug Fixes
Tests