feat(types): add hook contracts and policies - #1153
Conversation
📝 WalkthroughWalkthroughAdded hook schemas and public types, phase and tool matching, exit classification, UTF-8-safe output handling, sanitization, top-level exports, hook message support, and comprehensive contract tests. ChangesHook support
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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/types/src/hooks.ts`:
- Around line 261-264: Update sanitizeHookOutput so its non-printable-character
regex excludes the C1 control range U+0080–U+009F while continuing to preserve
valid Unicode characters above it. Add regression coverage verifying that an
8-bit CSI sequence such as \u009b31m is removed from sanitized output.
🪄 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: ca8ee0f6-9e93-412a-8d18-43342c6d2f93
📒 Files selected for processing (4)
packages/types/src/__tests__/hooks.test.tspackages/types/src/hooks.tspackages/types/src/index.tspackages/types/src/message.ts
| export function sanitizeHookOutput(output: string): string { | ||
| // ANSI control sequences are removed before other non-printable characters. | ||
| // eslint-disable-next-line no-control-regex | ||
| return output.replace(/\u001B\[[0-?]*[ -/]*[@-~]/g, "").replace(/[^\t\n\r\x20-\x7E\u0080-\uFFFF]/g, "") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove C1 control characters from sanitized output.
The second regex preserves U+0080 through U+009F. These are C1 control characters. For example, sanitizeHookOutput("\u009b31mred") retains an 8-bit CSI sequence. Exclude this range and add regression coverage.
Proposed fix
- return output.replace(/\u001B\[[0-?]*[ -/]*[`@-`~]/g, "").replace(/[^\t\n\r\x20-\x7E\u0080-\uFFFF]/g, "")
+ return output.replace(/\u001B\[[0-?]*[ -/]*[`@-`~]/g, "").replace(/[^\t\n\r\x20-\x7E\u00A0-\uFFFF]/g, "")📝 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 function sanitizeHookOutput(output: string): string { | |
| // ANSI control sequences are removed before other non-printable characters. | |
| // eslint-disable-next-line no-control-regex | |
| return output.replace(/\u001B\[[0-?]*[ -/]*[@-~]/g, "").replace(/[^\t\n\r\x20-\x7E\u0080-\uFFFF]/g, "") | |
| export function sanitizeHookOutput(output: string): string { | |
| // ANSI control sequences are removed before other non-printable characters. | |
| // eslint-disable-next-line no-control-regex | |
| return output.replace(/\u001B\[[0-?]*[ -/]*[`@-`~]/g, "").replace(/[^\t\n\r\x20-\x7E\u00A0-\uFFFF]/g, "") | |
| } |
🤖 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/types/src/hooks.ts` around lines 261 - 264, Update
sanitizeHookOutput so its non-printable-character regex excludes the C1 control
range U+0080–U+009F while continuing to preserve valid Unicode characters above
it. Add regression coverage verifying that an 8-bit CSI sequence such as
\u009b31m is removed from sanitized output.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Had some implementation comments
| phase: hookPhaseSchema, | ||
| status: hookRunStatusSchema, | ||
| exitCode: z.number().int().optional(), | ||
| stdoutSummary: z.string().optional(), |
There was a problem hiding this comment.
stdoutSummary/stderrSummary here (and outputSummary/errorSummary on hookMessageSchema) are unbounded, unlike every other string in this file. The 64K/16K caps live only in the runtime truncators, so a summary that skips them (tampered history file, IPC, or a future caller) passes validation at multi-MB. Bounding these at the schema (.max(...)) keeps the size invariant on the trust boundary itself. Note .max counts characters, not bytes.
|
|
||
| export const DEFAULT_HOOK_DEFINITIONS: readonly HookDefinition[] = Object.freeze([]) | ||
|
|
||
| export const hookInvocationSchema = z.discriminatedUnion("phase", [ |
There was a problem hiding this comment.
Both branches repeat version/hookRunId/taskId/instanceId/workspacePath verbatim; only preToolUse adds tool. hookDefinitionSchema above already uses a base + .extend per phase. Extending a shared base here removes the duplication and stops the two branches drifting when a shared field changes.
| return definitions.filter((definition) => definition.enabled && hookMatchesDefinition(definition, phase, toolName)) | ||
| } | ||
|
|
||
| function hookMatchesDefinition(definition: HookDefinition, phase: HookPhase, toolName?: ToolName): boolean { |
There was a problem hiding this comment.
hookMatchesDefinition just re-expresses the phase branching the hookMatches implementation already does, and re-checks toolName a second time. getMatchingHooks can dispatch inline (phase === "sessionStart" ? hookMatches(d, "sessionStart") : hookMatches(d, "preToolUse", toolName!)), dropping this helper and one guard. Keeps one source of matching logic.
github-actions
Bot
added
awaiting-author
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Address maintainer or CODEOWNER feedback, then push an update. Review-state labels are managed by this workflow; do not edit them manually. |
github-actions
Bot
added
coderabbit-review-active
2 participants
Stack
This is 1 of 4 in the Zoo Code hooks MVP stack. Review and merge from the bottom upward. Do not merge a later PR before its predecessor.
mainfm/zoo-hooks-contractsfm/zoo-hooks-settingssessionStartintegrationfm/zoo-hooks-session-startpreToolUse, E2E, and documentationSummary
Define the shared, runtime-validated contract for the hooks MVP without executing hooks or exposing settings yet.
Scope
sessionStartandpreToolUsephases with exact tool-name matching.say: "hook"chat-message data to the shared message contract.@roo-code/types.Data Flow
flowchart LR A[Hook definition] --> B[Schema validation] B --> C{Phase} C -->|sessionStart| D[Lifecycle match] C -->|preToolUse| E[Exact tool-name match] D --> F[Invocation contract] E --> F F --> G[Exit and output policy] G --> H[Structured hook result]Tests
packages/types/src/__tests__/hooks.test.tspnpm --dir src exec eslint --prune-suppressions --max-warnings=0on all subsequently touched core test files, with no suppression-count increase.pnpm test(437 files passed, 7,356 tests passed, 39 skipped).pnpm check-types,pnpm lint, andpnpm buildpassed.USE_MOCK=true TEST_FILE=hooks.test pnpm --filter @roo-code/vscode-e2e test:runpassed.Risks
Review Notes
main.#1153->#1154->#1156->#1155.