ETA-556: Remove redundant validation in validate-hooks.js#1146
ETA-556: Remove redundant validation in validate-hooks.js#1146ed-teal wants to merge 1 commit intoaffaan-m:mainfrom
Conversation
Simplified validateHookEntry from 75 lines to 21 by removing manual field checks already covered by Ajv schema. Kept only inline JS syntax validation and async-only-on-command check. Updated tests to match.
📝 WalkthroughWalkthroughThe PR refactors hook validation to delegate schema-based checks to the JSON schema validator rather than duplicating them in the script. The validator now focuses solely on non-schema concerns: inline JavaScript syntax validation for command hooks and flagging improper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR simplifies The refactor is generally sound — the JSON schema (
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Read hooks.json] --> B{File exists?}
B -- No --> C[Skip / exit 0]
B -- Yes --> D[Parse JSON]
D -- Invalid JSON --> E[Exit 1]
D -- OK --> F{hooks.schema.json\nexists?}
F -- Yes --> G[AJV schema validation]
G -- Invalid --> H[Print schema errors\nExit 1]
G -- Valid --> I[Iterate hook entries]
F -- No --> I
I --> J{type === command\nand command is string?}
J -- Yes --> K{Matches node -e?}
K -- Yes --> L[vm.Script syntax check]
L -- SyntaxError --> M[Print error\nhasErrors = true]
L -- OK --> N[Continue]
K -- No --> N
J -- No --> N
N --> O{type !== command\nand async present?}
O -- Yes --> P[Print error\nhasErrors = true]
O -- No --> Q[Next hook]
Q --> I
P --> Q
M --> Q
I -- Done --> R{hasErrors?}
R -- Yes --> S[Exit 1]
R -- No --> T[Print summary\nExit 0]
Reviews (1): Last reviewed commit: "ETA-556: Remove redundant manual validat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -33,94 +33,34 @@ const VALID_EVENTS = [ | |||
| const VALID_HOOK_TYPES = ['command', 'http', 'prompt', 'agent']; | |||
There was a problem hiding this comment.
VALID_HOOK_TYPES is no longer used
After removing the !VALID_HOOK_TYPES.includes(hook.type) check, this constant has no remaining references and should be removed to avoid confusion.
| const VALID_HOOK_TYPES = ['command', 'http', 'prompt', 'agent']; | |
| const EVENTS_WITHOUT_MATCHER = new Set(['UserPromptSubmit', 'Notification', 'Stop', 'SubagentStop']); |
| @@ -929,9 +930,8 @@ function runTests() { | |||
| } | |||
| })); | |||
|
|
|||
| const result = runValidatorWithDir('validate-hooks', 'HOOKS_FILE', hooksFile); | |||
| assert.strictEqual(result.code, 1, 'Should reject whitespace-only command'); | |||
| assert.ok(result.stderr.includes('command'), 'Should report command field error'); | |||
| const result = runValidatorWithDirs('validate-hooks', { HOOKS_FILE: hooksFile, HOOKS_SCHEMA_PATH: hooksSchemaPath }); | |||
| assert.strictEqual(result.code, 0, 'Schema minLength allows whitespace-only strings'); | |||
| cleanupTestDir(testDir); | |||
There was a problem hiding this comment.
command values now accepted
The original code used isNonEmptyString (checking value.trim().length > 0) which correctly rejected commands like " ". The JSON schema uses minLength: 1, which only counts characters — a whitespace-only string satisfies minLength: 1 and passes schema validation.
The updated test now explicitly documents this regression by changing the expectation to code: 0 and renaming the test to say "schema only checks minLength". A hook with "command": " " will now pass validation but fail silently at runtime when the shell tries to execute a blank command.
Since this was not a case that the schema naturally covers, the old isNonEmptyString guard was not truly redundant and should have been preserved in validateHookEntry, or the JSON schema's command.minLength constraint should be supplemented with a pattern constraint such as "pattern": "\\S" to require at least one non-whitespace character.
![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 the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/validate-hooks.js`:
- Around line 47-57: The validator currently accepts whitespace-only command
strings (hook.command) because schema minLength:1 allows " "; update the
inline validation in the block handling hook.type === 'command' to reject
commands that are empty after trimming: before attempting the node -e match
(nodeEMatch) check, verify that hook.command.trim().length > 0 and if not log a
clear error via console.error (including label) and set hasErrors = true; keep
the subsequent nodeEMatch/vm.Script syntax check (nodeEMatch and new
vm.Script(...)) unchanged so we still validate inline JS when present.
🪄 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: 59323f08-8028-4d43-a1bd-c477aac3e7b0
📒 Files selected for processing (2)
scripts/ci/validate-hooks.jstests/ci/validators.test.js
| if (hook.type === 'command' && typeof hook.command === 'string') { | ||
| const nodeEMatch = hook.command.match(/^node -e "(.*)"$/s); | ||
| if (nodeEMatch) { | ||
| try { | ||
| new vm.Script(nodeEMatch[1].replace(/\\\\/g, '\\').replace(/\\"/g, '"').replace(/\\n/g, '\n').replace(/\\t/g, '\t')); | ||
| } catch (syntaxErr) { | ||
| console.error(`ERROR: ${label} has invalid inline JS: ${syntaxErr.message}`); | ||
| hasErrors = true; | ||
| } | ||
| } | ||
|
|
||
| return hasErrors; | ||
| } |
There was a problem hiding this comment.
Whitespace-only command strings are now accepted (validation gap).
At Line 47, schema minLength: 1 permits strings like " ", and this function no longer rejects them. That weakens hook validation and defers failure to runtime. Please add a trimmed non-empty check here (or enforce non-whitespace in schema via pattern), then keep tests aligned.
💡 Minimal fix in validator
if (hook.type === 'command' && typeof hook.command === 'string') {
+ if (!hook.command.trim()) {
+ console.error(`ERROR: ${label} command must not be empty or whitespace-only`);
+ hasErrors = true;
+ return hasErrors;
+ }
const nodeEMatch = hook.command.match(/^node -e "(.*)"$/s);
if (nodeEMatch) {
try {
new vm.Script(nodeEMatch[1].replace(/\\\\/g, '\\').replace(/\\"/g, '"').replace(/\\n/g, '\n').replace(/\\t/g, '\t'));
} catch (syntaxErr) {As per coding guidelines: "Always validate all user input before processing at system boundaries" and "Fail fast with clear error messages when validation fails".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/validate-hooks.js` around lines 47 - 57, The validator currently
accepts whitespace-only command strings (hook.command) because schema
minLength:1 allows " "; update the inline validation in the block handling
hook.type === 'command' to reject commands that are empty after trimming: before
attempting the node -e match (nodeEMatch) check, verify that
hook.command.trim().length > 0 and if not log a clear error via console.error
(including label) and set hasErrors = true; keep the subsequent
nodeEMatch/vm.Script syntax check (nodeEMatch and new vm.Script(...)) unchanged
so we still validate inline JS when present.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ci/validate-hooks.js">
<violation number="1" location="scripts/ci/validate-hooks.js:47">
P1: Validation regression: whitespace-only `command` strings (e.g., `" "`) are no longer rejected. The removed `isNonEmptyString` helper checked `value.trim().length > 0`, but the JSON schema's `minLength: 1` only counts raw characters—whitespace satisfies it. Add a trimmed non-empty check here (or add a `"pattern": "\\S"` constraint to the schema) so blank commands fail fast at validation time instead of silently failing at runtime.</violation>
<violation number="2" location="scripts/ci/validate-hooks.js:47">
P3: `VALID_HOOK_TYPES` is now dead code—the only check that referenced it (`!VALID_HOOK_TYPES.includes(hook.type)`) was removed. Delete the constant to avoid confusion.</violation>
</file>
<file name="tests/ci/validators.test.js">
<violation number="1" location="tests/ci/validators.test.js:934">
P2: The test now explicitly documents the whitespace-only command regression as expected behavior. This assertion should expect rejection (exit code 1), not acceptance (exit code 0). After adding a trim check in `validateHookEntry`, revert this test to expect failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| console.error(`ERROR: ${label} has invalid inline JS: ${syntaxErr.message}`); | ||
| hasErrors = true; | ||
| } | ||
| if (hook.type === 'command' && typeof hook.command === 'string') { |
There was a problem hiding this comment.
P1: Validation regression: whitespace-only command strings (e.g., " ") are no longer rejected. The removed isNonEmptyString helper checked value.trim().length > 0, but the JSON schema's minLength: 1 only counts raw characters—whitespace satisfies it. Add a trimmed non-empty check here (or add a "pattern": "\\S" constraint to the schema) so blank commands fail fast at validation time instead of silently failing at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/ci/validate-hooks.js, line 47:
<comment>Validation regression: whitespace-only `command` strings (e.g., `" "`) are no longer rejected. The removed `isNonEmptyString` helper checked `value.trim().length > 0`, but the JSON schema's `minLength: 1` only counts raw characters—whitespace satisfies it. Add a trimmed non-empty check here (or add a `"pattern": "\\S"` constraint to the schema) so blank commands fail fast at validation time instead of silently failing at runtime.</comment>
<file context>
@@ -33,94 +33,34 @@ const VALID_EVENTS = [
- console.error(`ERROR: ${label} has invalid inline JS: ${syntaxErr.message}`);
- hasErrors = true;
- }
+ if (hook.type === 'command' && typeof hook.command === 'string') {
+ const nodeEMatch = hook.command.match(/^node -e "(.*)"$/s);
+ if (nodeEMatch) {
</file context>
| if (hook.type === 'command' && typeof hook.command === 'string') { | |
| if (hook.type === 'command' && typeof hook.command === 'string') { | |
| if (!hook.command.trim()) { | |
| console.error(`ERROR: ${label} command must not be empty or whitespace-only`); | |
| hasErrors = true; | |
| return hasErrors; | |
| } |
| assert.strictEqual(result.code, 1, 'Should reject whitespace-only command'); | ||
| assert.ok(result.stderr.includes('command'), 'Should report command field error'); | ||
| const result = runValidatorWithDirs('validate-hooks', { HOOKS_FILE: hooksFile, HOOKS_SCHEMA_PATH: hooksSchemaPath }); | ||
| assert.strictEqual(result.code, 0, 'Schema minLength allows whitespace-only strings'); |
There was a problem hiding this comment.
P2: The test now explicitly documents the whitespace-only command regression as expected behavior. This assertion should expect rejection (exit code 1), not acceptance (exit code 0). After adding a trim check in validateHookEntry, revert this test to expect failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/ci/validators.test.js, line 934:
<comment>The test now explicitly documents the whitespace-only command regression as expected behavior. This assertion should expect rejection (exit code 1), not acceptance (exit code 0). After adding a trim check in `validateHookEntry`, revert this test to expect failure.</comment>
<file context>
@@ -929,9 +930,8 @@ function runTests() {
- assert.strictEqual(result.code, 1, 'Should reject whitespace-only command');
- assert.ok(result.stderr.includes('command'), 'Should report command field error');
+ const result = runValidatorWithDirs('validate-hooks', { HOOKS_FILE: hooksFile, HOOKS_SCHEMA_PATH: hooksSchemaPath });
+ assert.strictEqual(result.code, 0, 'Schema minLength allows whitespace-only strings');
cleanupTestDir(testDir);
})) passed++; else failed++;
</file context>
| assert.strictEqual(result.code, 0, 'Schema minLength allows whitespace-only strings'); | |
| assert.strictEqual(result.code, 1, 'Should reject whitespace-only command'); |
| @@ -33,94 +33,34 @@ const VALID_EVENTS = [ | |||
| const VALID_HOOK_TYPES = ['command', 'http', 'prompt', 'agent']; | |||
There was a problem hiding this comment.
P3: VALID_HOOK_TYPES is now dead code—the only check that referenced it (!VALID_HOOK_TYPES.includes(hook.type)) was removed. Delete the constant to avoid confusion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/ci/validate-hooks.js, line 47:
<comment>`VALID_HOOK_TYPES` is now dead code—the only check that referenced it (`!VALID_HOOK_TYPES.includes(hook.type)`) was removed. Delete the constant to avoid confusion.</comment>
<file context>
@@ -33,94 +33,34 @@ const VALID_EVENTS = [
- console.error(`ERROR: ${label} has invalid inline JS: ${syntaxErr.message}`);
- hasErrors = true;
- }
+ if (hook.type === 'command' && typeof hook.command === 'string') {
+ const nodeEMatch = hook.command.match(/^node -e "(.*)"$/s);
+ if (nodeEMatch) {
</file context>
1 participant
Why
validate-hooks.js had 90 lines of manual JSON schema validation that duplicated checks already handled by the inline JS syntax checker and async-only-on-command check. The redundancy added maintenance burden without catching additional errors.
What
How to Test
node tests/run-all.jsnode scripts/ci/validate-hooks.jsruns cleanlyAI Usage
Summary by cubic
Remove redundant manual validation in
scripts/ci/validate-hooks.js, relying on schema (ajv) and keeping only inline JS syntax and async-only-on-command checks. Fulfills ETA-556 by removing duplicate checks and simplifying tests (-62 LOC).validateHookEntry; retainednode -einline JS syntax validation and enforcement thatasyncis only for command hooks.HOOKS_SCHEMA_PATHand assert schema-driven errors instead of field-specific messages.Written for commit 5f0865a. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests