fix(session-start): move config require inside try-catch to honor exit-0 contract#72
Open
Jamie-BitFlight wants to merge 1 commit intomainfrom
Open
fix(session-start): move config require inside try-catch to honor exit-0 contract#72Jamie-BitFlight wants to merge 1 commit intomainfrom
Jamie-BitFlight wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the SessionStart hook script so it consistently honors its “exit 0 always” contract by preventing a top-level require() failure from terminating the process before main() runs.
Changes:
- Removed the top-level
require('./hallucination-config.cjs'). - Re-added the
require()insidemain()’s existingtry/catchso config load/require failures fall back to default framing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
📝 WalkthroughWalkthroughSplit and harden configuration handling: added defaults, TOML parser, merge/deep-freeze, and validation modules; introduced non-throwing safe loaders; updated scripts to consume safe loaders/defaults; and added unit tests and coverage entries for the new modules. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Script (audit/annotate/framing)
participant Safe as safeLoadConfig / safeLoadWeights
participant TOML as parseToml
participant Validate as validateConfig
participant Merge as mergeConfig
participant Defaults as DEFAULT_* constants
CLI->>Safe: safeLoadConfig(opts)
Safe->>TOML: parse project TOML (if present)
TOML-->>Safe: parsed config object
Safe->>Validate: validateConfig(parsed, source)
Validate-->>Safe: validated config
Safe->>Merge: mergeConfig(Defaults.DEFAULT_CONFIG, validated)
Merge-->>Safe: merged config
Safe->>Defaults: fallback to DEFAULT_WEIGHTS/THRESHOLDS on loader failure
Safe-->>CLI: return frozen config (merged or fallback)
CLI->>CLI: execute main logic using returned config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/hallucination-audit-stop.cjs`:
- Around line 61-68: The current fallback sets validateClaimStructure and
CLAIM_LABEL_ALTERNATION to null which later causes unconditional calls and a
broken LABELED_CLAIM_LINE_RE; instead provide safe defaults: if _claim_structure
is missing set validateClaimStructure to a noop validator (e.g., a function that
returns false) and set CLAIM_LABEL_ALTERNATION to a safe empty alternation
string (so regex assembly remains valid), and/or add guarded checks before any
use of validateClaimStructure and when constructing LABELED_CLAIM_LINE_RE;
locate the symbols _claim_structure, validateClaimStructure,
CLAIM_LABEL_ALTERNATION and LABELED_CLAIM_LINE_RE to apply these fallbacks or
guards.
In `@scripts/hallucination-config-merge.cjs`:
- Around line 42-57: The merge writes potentially dangerous keys like
"__proto__" into plain objects (see the loop over Object.keys(override), and the
variables baseCats, overCats, merged, baseCat, overCat and the final assignment
to result[key] / merged[catName]); to fix, ensure merged/result objects are
created with no prototype (use Object.create(null)) and/or explicitly skip
prototype-polluting keys (e.g. if (catName === '__proto__' || catName ===
'constructor') continue) before assigning to merged[catName] and likewise skip
override keys before setting result[key]; apply the same guard to the similar
merge block at the other location (lines ~69-84).
- Around line 60-68: The current merge creates mergedCat via `{ ...baseCat,
...overCat }` which preserves baseCat.customPatterns even when
overCat.replacePatterns is true; instead, exclude customPatterns from the
initial spread and explicitly set mergedCat.customPatterns afterwards: extract
customPatterns (and replacePatterns) from baseCat/overCat, build mergedCat from
the remaining properties (`baseCat`/`overCat` spreads without customPatterns),
then if overCat.replacePatterns is true set mergedCat.customPatterns to
overCat.customPatterns (or [] if undefined), otherwise set
mergedCat.customPatterns to the concatenation of baseCat.customPatterns and
overCat.customPatterns when they are arrays (or the existing array if only one
side has it).
In `@scripts/hallucination-config-safe.cjs`:
- Around line 14-20: The top-level require of deepFreeze from
hallucination-config-merge.cjs can crash the module before safeLoadConfig()
runs; move the require into the safe loader and guard it: remove the top-level
import of deepFreeze and instead require('./hallucination-config-merge.cjs')
inside safeLoadConfig() within a try/catch, use the exported deepFreeze if
available, and fall back to a no-op identity function when the require fails so
safeLoadConfig() can continue and return the default fallback config; reference
deepFreeze and safeLoadConfig to locate where to change the import and add the
try/catch/fallback.
In `@scripts/hallucination-config-toml.cjs`:
- Around line 115-126: The parsed TOML maps are created as plain {} which allows
prototype pollution (e.g., headers like [tool.hallucination-detector.__proto__])
— change all map/table allocations to null-prototype objects (use
Object.create(null)) so keys like "__proto__" don't walk/write to
Object.prototype; specifically replace the table = {} in parseTomlInlineTable
and any other places that build maps/tables (the code that assigns/creates
'current' and the section/table-creation logic referenced around lines 136-167)
to use Object.create(null) and ensure nested/inline table creation follows the
same pattern.
- Around line 83-90: The current shared replace chain corrupts TOML literal
strings and mis-decodes escapes; change the branch that handles quoted strings
so single-quoted (literal) strings are returned as s.slice(1, -1) with no escape
processing, while double-quoted (basic) strings apply escape decoding in the
correct order (first collapse escaped backslashes, e.g., replace(/\\\\/g,'\\'),
then decode sequences like \\n, \\t and escaped quotes like \\" ) — update the
conditional around s.startsWith('"') / s.startsWith("'") to separate the
single-quote and double-quote cases and perform the replacements only for the
double-quoted path.
In `@scripts/hallucination-config-validate.cjs`:
- Around line 141-146: The weights container validation currently only checks
that obj.weights is an object but not that each entry is a numeric value; update
the block that handles 'weights' to iterate over Object.keys(obj.weights) and
for each key validate that typeof value === 'number' and Number.isFinite(value);
for any invalid entry call warn('weights.' + key, obj.weights[key],
DEFAULT_WEIGHTS[key]) (or warn('weights', obj.weights, DEFAULT_WEIGHTS) if
key-specific default not available) and delete obj.weights[key]; keep the
existing container-level checks (typeof obj.weights !== 'object' || obj.weights
=== null || Array.isArray(obj.weights)) that call warn('weights', obj.weights,
DEFAULT_WEIGHTS) and delete obj.weights so only numeric per-key values survive
into the merge/scoring path.
In `@tests/hallucination-config-safe.test.cjs`:
- Around line 4-6: The beforeEach currently calls vi.resetModules() but leaves
mocks registered with vi.doMock() in place; update the test setup by calling
vi.doUnmock('../scripts/hallucination-config.cjs') (and any other modules you
previously vi.doMock()ed) in the beforeEach that contains vi.resetModules(), so
mocks are removed between runs and the "loader works" / DEFAULT_WEIGHTS
assertions run with clean mocks; ensure you add the same vi.doUnmock call(s) in
the other beforeEach block covering lines 177-193 to fully isolate tests.
🪄 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: 8bb41a8b-2015-4101-a2c7-8efc45f34815
📒 Files selected for processing (15)
scripts/hallucination-annotate.cjsscripts/hallucination-audit-stop.cjsscripts/hallucination-config-defaults.cjsscripts/hallucination-config-merge.cjsscripts/hallucination-config-safe.cjsscripts/hallucination-config-toml.cjsscripts/hallucination-config-validate.cjsscripts/hallucination-config.cjsscripts/hallucination-framing-session-start.cjstests/hallucination-config-defaults.test.cjstests/hallucination-config-merge.test.cjstests/hallucination-config-safe.test.cjstests/hallucination-config-toml.test.cjstests/hallucination-config-validate.test.cjsvitest.config.cjs
✅ Files skipped from review due to trivial changes (3)
- vitest.config.cjs
- tests/hallucination-config-toml.test.cjs
- scripts/hallucination-config-defaults.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/hallucination-framing-session-start.cjs
Comment on lines
+61
to
+68
| let _claim_structure = null; | ||
| try { | ||
| _claim_structure = require('./hallucination-claim-structure.cjs'); | ||
| } catch { | ||
| /* claim structure analysis unavailable */ | ||
| } | ||
| const validateClaimStructure = _claim_structure?.validateClaimStructure || null; | ||
| const CLAIM_LABEL_ALTERNATION = _claim_structure?.CLAIM_LABEL_ALTERNATION || null; |
Contributor
There was a problem hiding this comment.
Don't fall back to null for claim-structure helpers.
validateClaimStructure becomes null here, but Line 2782 still calls it unconditionally. That turns the intended graceful-degradation path into an exit-1 crash, and Line 2432 also compiles a bogus LABELED_CLAIM_LINE_RE from null.
🛠️ Suggested fix
-const validateClaimStructure = _claim_structure?.validateClaimStructure || null;
-const CLAIM_LABEL_ALTERNATION = _claim_structure?.CLAIM_LABEL_ALTERNATION || null;
+const validateClaimStructure =
+ _claim_structure?.validateClaimStructure ||
+ (() => ({ valid: true, structured: false, claims: [], errors: [] }));
+const CLAIM_LABEL_ALTERNATION =
+ _claim_structure?.CLAIM_LABEL_ALTERNATION ||
+ 'VERIFIED|INFERRED|UNKNOWN|SPECULATION|CORRELATED|CAUSAL|REJECTED';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-audit-stop.cjs` around lines 61 - 68, The current
fallback sets validateClaimStructure and CLAIM_LABEL_ALTERNATION to null which
later causes unconditional calls and a broken LABELED_CLAIM_LINE_RE; instead
provide safe defaults: if _claim_structure is missing set validateClaimStructure
to a noop validator (e.g., a function that returns false) and set
CLAIM_LABEL_ALTERNATION to a safe empty alternation string (so regex assembly
remains valid), and/or add guarded checks before any use of
validateClaimStructure and when constructing LABELED_CLAIM_LINE_RE; locate the
symbols _claim_structure, validateClaimStructure, CLAIM_LABEL_ALTERNATION and
LABELED_CLAIM_LINE_RE to apply these fallbacks or guards.
Comment on lines
+42
to
+57
| for (const key of Object.keys(override)) { | ||
| const overVal = override[key]; | ||
| const baseVal = base[key]; | ||
|
|
||
| if (key === 'categories') { | ||
| // Merge categories map, with special customPatterns concatenation logic. | ||
| const baseCats = | ||
| typeof baseVal === 'object' && baseVal !== null && !Array.isArray(baseVal) ? baseVal : {}; | ||
| const overCats = | ||
| typeof overVal === 'object' && overVal !== null && !Array.isArray(overVal) ? overVal : {}; | ||
| const merged = { ...baseCats }; | ||
| for (const catName of Object.keys(overCats)) { | ||
| const baseCat = baseCats[catName] || {}; | ||
| const overCat = overCats[catName]; | ||
| if (typeof overCat !== 'object' || overCat === null) { | ||
| merged[catName] = overCat; |
Contributor
There was a problem hiding this comment.
Skip prototype-polluting keys while merging config.
result[key] = ... and merged[catName] = ... write repo-controlled keys into plain objects. A JSON/CJS/TOML config with an own __proto__ can smuggle inherited values into the merged config instead of staying as data.
🔒 Suggested hardening
for (const key of Object.keys(override)) {
+ if (key === '__proto__') continue;
const overVal = override[key];
const baseVal = base[key];
@@
const merged = { ...baseCats };
for (const catName of Object.keys(overCats)) {
+ if (catName === '__proto__') continue;
const baseCat = baseCats[catName] || {};
const overCat = overCats[catName];
if (typeof overCat !== 'object' || overCat === null) {
merged[catName] = overCat;
continue;Also applies to: 69-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-merge.cjs` around lines 42 - 57, The merge
writes potentially dangerous keys like "__proto__" into plain objects (see the
loop over Object.keys(override), and the variables baseCats, overCats, merged,
baseCat, overCat and the final assignment to result[key] / merged[catName]); to
fix, ensure merged/result objects are created with no prototype (use
Object.create(null)) and/or explicitly skip prototype-polluting keys (e.g. if
(catName === '__proto__' || catName === 'constructor') continue) before
assigning to merged[catName] and likewise skip override keys before setting
result[key]; apply the same guard to the similar merge block at the other
location (lines ~69-84).
Comment on lines
+60
to
+68
| const mergedCat = { ...baseCat, ...overCat }; | ||
| // Concatenate customPatterns unless replacePatterns is true in the override. | ||
| if ( | ||
| !overCat.replacePatterns && | ||
| Array.isArray(baseCat.customPatterns) && | ||
| Array.isArray(overCat.customPatterns) | ||
| ) { | ||
| mergedCat.customPatterns = [...baseCat.customPatterns, ...overCat.customPatterns]; | ||
| } |
Contributor
There was a problem hiding this comment.
replacePatterns: true still leaks lower-priority customPatterns.
mergedCat starts from { ...baseCat, ...overCat }, so an override like { replacePatterns: true } still keeps the inherited customPatterns. That makes it impossible to clear lower-priority custom patterns.
🛠️ Suggested fix
const mergedCat = { ...baseCat, ...overCat };
// Concatenate customPatterns unless replacePatterns is true in the override.
- if (
- !overCat.replacePatterns &&
- Array.isArray(baseCat.customPatterns) &&
- Array.isArray(overCat.customPatterns)
- ) {
+ if (overCat.replacePatterns) {
+ mergedCat.customPatterns = Array.isArray(overCat.customPatterns)
+ ? [...overCat.customPatterns]
+ : [];
+ } else if (
+ Array.isArray(baseCat.customPatterns) &&
+ Array.isArray(overCat.customPatterns)
+ ) {
mergedCat.customPatterns = [...baseCat.customPatterns, ...overCat.customPatterns];
}📝 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 mergedCat = { ...baseCat, ...overCat }; | |
| // Concatenate customPatterns unless replacePatterns is true in the override. | |
| if ( | |
| !overCat.replacePatterns && | |
| Array.isArray(baseCat.customPatterns) && | |
| Array.isArray(overCat.customPatterns) | |
| ) { | |
| mergedCat.customPatterns = [...baseCat.customPatterns, ...overCat.customPatterns]; | |
| } | |
| const mergedCat = { ...baseCat, ...overCat }; | |
| // Concatenate customPatterns unless replacePatterns is true in the override. | |
| if (overCat.replacePatterns) { | |
| mergedCat.customPatterns = Array.isArray(overCat.customPatterns) | |
| ? [...overCat.customPatterns] | |
| : []; | |
| } else if ( | |
| Array.isArray(baseCat.customPatterns) && | |
| Array.isArray(overCat.customPatterns) | |
| ) { | |
| mergedCat.customPatterns = [...baseCat.customPatterns, ...overCat.customPatterns]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-merge.cjs` around lines 60 - 68, The current
merge creates mergedCat via `{ ...baseCat, ...overCat }` which preserves
baseCat.customPatterns even when overCat.replacePatterns is true; instead,
exclude customPatterns from the initial spread and explicitly set
mergedCat.customPatterns afterwards: extract customPatterns (and
replacePatterns) from baseCat/overCat, build mergedCat from the remaining
properties (`baseCat`/`overCat` spreads without customPatterns), then if
overCat.replacePatterns is true set mergedCat.customPatterns to
overCat.customPatterns (or [] if undefined), otherwise set
mergedCat.customPatterns to the concatenation of baseCat.customPatterns and
overCat.customPatterns when they are arrays (or the existing array if only one
side has it).
Comment on lines
+14
to
+20
| const { | ||
| DEFAULT_WEIGHTS, | ||
| DEFAULT_THRESHOLDS, | ||
| DEFAULT_CONFIDENCE_WEIGHTS, | ||
| DEFAULT_CONFIG, | ||
| } = require('./hallucination-config-defaults.cjs'); | ||
| const { deepFreeze } = require('./hallucination-config-merge.cjs'); |
Contributor
There was a problem hiding this comment.
Avoid a nontrivial top-level dependency in the safe loader.
If hallucination-config-merge.cjs has a syntax/runtime failure, requiring this "safe" wrapper throws before safeLoadConfig() runs, so the fallback path never gets a chance.
🛠️ Suggested fix
const {
DEFAULT_WEIGHTS,
DEFAULT_THRESHOLDS,
DEFAULT_CONFIDENCE_WEIGHTS,
DEFAULT_CONFIG,
} = require('./hallucination-config-defaults.cjs');
-const { deepFreeze } = require('./hallucination-config-merge.cjs');
+
+function deepFreeze(obj) {
+ if (obj === null || typeof obj !== 'object') return obj;
+ for (const val of Object.values(obj)) {
+ deepFreeze(val);
+ }
+ return Object.freeze(obj);
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-safe.cjs` around lines 14 - 20, The top-level
require of deepFreeze from hallucination-config-merge.cjs can crash the module
before safeLoadConfig() runs; move the require into the safe loader and guard
it: remove the top-level import of deepFreeze and instead
require('./hallucination-config-merge.cjs') inside safeLoadConfig() within a
try/catch, use the exported deepFreeze if available, and fall back to a no-op
identity function when the require fails so safeLoadConfig() can continue and
return the default fallback config; reference deepFreeze and safeLoadConfig to
locate where to change the import and add the try/catch/fallback.
Comment on lines
+83
to
+90
| if ((s.startsWith('"') && s.endsWith('"')) || (s.startsWith("'") && s.endsWith("'"))) { | ||
| return s | ||
| .slice(1, -1) | ||
| .replace(/\\n/g, '\n') | ||
| .replace(/\\t/g, '\t') | ||
| .replace(/\\\\/g, '\\') | ||
| .replace(/\\"/g, '"'); | ||
| } |
Contributor
There was a problem hiding this comment.
Parse TOML basic and literal strings separately.
This shared replace chain corrupts literal strings ('C:\temp\foo' turns \t into a tab) and even mis-decodes basic strings like "\\n" because \n is rewritten before escaped backslashes are collapsed.
🛠️ Suggested fix
- if ((s.startsWith('"') && s.endsWith('"')) || (s.startsWith("'") && s.endsWith("'"))) {
- return s
- .slice(1, -1)
- .replace(/\\n/g, '\n')
- .replace(/\\t/g, '\t')
- .replace(/\\\\/g, '\\')
- .replace(/\\"/g, '"');
+ if (s.startsWith("'") && s.endsWith("'")) {
+ return s.slice(1, -1);
+ }
+ if (s.startsWith('"') && s.endsWith('"')) {
+ return JSON.parse(s);
}📝 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
| if ((s.startsWith('"') && s.endsWith('"')) || (s.startsWith("'") && s.endsWith("'"))) { | |
| return s | |
| .slice(1, -1) | |
| .replace(/\\n/g, '\n') | |
| .replace(/\\t/g, '\t') | |
| .replace(/\\\\/g, '\\') | |
| .replace(/\\"/g, '"'); | |
| } | |
| if (s.startsWith("'") && s.endsWith("'")) { | |
| return s.slice(1, -1); | |
| } | |
| if (s.startsWith('"') && s.endsWith('"')) { | |
| return JSON.parse(s); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-toml.cjs` around lines 83 - 90, The current
shared replace chain corrupts TOML literal strings and mis-decodes escapes;
change the branch that handles quoted strings so single-quoted (literal) strings
are returned as s.slice(1, -1) with no escape processing, while double-quoted
(basic) strings apply escape decoding in the correct order (first collapse
escaped backslashes, e.g., replace(/\\\\/g,'\\'), then decode sequences like
\\n, \\t and escaped quotes like \\" ) — update the conditional around
s.startsWith('"') / s.startsWith("'") to separate the single-quote and
double-quote cases and perform the replacements only for the double-quoted path.
Comment on lines
+115
to
+126
| function parseTomlInlineTable(content) { | ||
| const table = {}; | ||
| if (!content.trim()) return table; | ||
| for (const pair of splitTopLevel(content.trim(), ',')) { | ||
| const p = pair.trim(); | ||
| const eqIdx = p.indexOf('='); | ||
| if (eqIdx === -1) continue; | ||
| const k = p.slice(0, eqIdx).trim(); | ||
| const v = p.slice(eqIdx + 1).trim(); | ||
| if (k) table[k] = parseTomlValue(v); | ||
| } | ||
| return table; |
Contributor
There was a problem hiding this comment.
Use null-prototype objects for parsed TOML maps.
These containers are ordinary {} objects. A header like [tool.hallucination-detector.__proto__] makes current walk onto Object.prototype, and Line 167 then writes through it. Inline tables have the same __proto__ problem, so a repo-controlled pyproject.toml can prototype-pollute the parser output during hook startup.
🔒 Suggested hardening
function parseTomlInlineTable(content) {
- const table = {};
+ const table = Object.create(null);
if (!content.trim()) return table;
for (const pair of splitTopLevel(content.trim(), ',')) {
const p = pair.trim();
const eqIdx = p.indexOf('=');
if (eqIdx === -1) continue;
@@
function parseToml(source) {
- const result = {};
+ const result = Object.create(null);
let current = result;
@@
current = result;
for (const part of parts) {
if (typeof current[part] !== 'object' || current[part] === null) {
- current[part] = {};
+ current[part] = Object.create(null);
}
current = current[part];
}Also applies to: 136-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-toml.cjs` around lines 115 - 126, The parsed
TOML maps are created as plain {} which allows prototype pollution (e.g.,
headers like [tool.hallucination-detector.__proto__]) — change all map/table
allocations to null-prototype objects (use Object.create(null)) so keys like
"__proto__" don't walk/write to Object.prototype; specifically replace the table
= {} in parseTomlInlineTable and any other places that build maps/tables (the
code that assigns/creates 'current' and the section/table-creation logic
referenced around lines 136-167) to use Object.create(null) and ensure
nested/inline table creation follows the same pattern.
Comment on lines
+141
to
+146
| // weights: object with numeric values | ||
| if ('weights' in obj) { | ||
| if (typeof obj.weights !== 'object' || obj.weights === null || Array.isArray(obj.weights)) { | ||
| warn('weights', obj.weights, DEFAULT_WEIGHTS); | ||
| delete obj.weights; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate individual weights entries, not just the container.
This branch currently accepts values like { weights: { speculation_language: 'high' } } as valid. That lets non-numeric overrides survive into the merge/scoring path and can turn later calculations into NaN or other coercion bugs. Invalid keys should be dropped here so defaults win.
Suggested fix
if ('weights' in obj) {
if (typeof obj.weights !== 'object' || obj.weights === null || Array.isArray(obj.weights)) {
warn('weights', obj.weights, DEFAULT_WEIGHTS);
delete obj.weights;
+ } else {
+ for (const key of Object.keys(obj.weights)) {
+ if (!Number.isFinite(obj.weights[key])) {
+ warn(`weights.${key}`, obj.weights[key], DEFAULT_WEIGHTS[key]);
+ delete obj.weights[key];
+ }
+ }
}
}📝 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
| // weights: object with numeric values | |
| if ('weights' in obj) { | |
| if (typeof obj.weights !== 'object' || obj.weights === null || Array.isArray(obj.weights)) { | |
| warn('weights', obj.weights, DEFAULT_WEIGHTS); | |
| delete obj.weights; | |
| } | |
| // weights: object with numeric values | |
| if ('weights' in obj) { | |
| if (typeof obj.weights !== 'object' || obj.weights === null || Array.isArray(obj.weights)) { | |
| warn('weights', obj.weights, DEFAULT_WEIGHTS); | |
| delete obj.weights; | |
| } else { | |
| for (const key of Object.keys(obj.weights)) { | |
| if (!Number.isFinite(obj.weights[key])) { | |
| warn(`weights.${key}`, obj.weights[key], DEFAULT_WEIGHTS[key]); | |
| delete obj.weights[key]; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hallucination-config-validate.cjs` around lines 141 - 146, The
weights container validation currently only checks that obj.weights is an object
but not that each entry is a numeric value; update the block that handles
'weights' to iterate over Object.keys(obj.weights) and for each key validate
that typeof value === 'number' and Number.isFinite(value); for any invalid entry
call warn('weights.' + key, obj.weights[key], DEFAULT_WEIGHTS[key]) (or
warn('weights', obj.weights, DEFAULT_WEIGHTS) if key-specific default not
available) and delete obj.weights[key]; keep the existing container-level checks
(typeof obj.weights !== 'object' || obj.weights === null ||
Array.isArray(obj.weights)) that call warn('weights', obj.weights,
DEFAULT_WEIGHTS) and delete obj.weights so only numeric per-key values survive
into the merge/scoring path.
Comment on lines
+4
to
+6
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest 4, does vi.resetModules() clear module mocks registered with vi.doMock(), or do tests need vi.unmock/vi.doUnmock to remove the mock between test cases?
💡 Result:
In Vitest 4, vi.resetModules() only clears the module cache (so modules can be re-evaluated on the next dynamic import). It does not clear the mocks registry, so it will not remove module mocks you registered with vi.doMock()/vi.mock(). The docs explicitly warn: “Does not reset mocks registry. To clear mocks registry, use vi.unmock or vi.doUnmock.” [1]
So if you need the module mock to not apply in a later test case, you must call vi.doUnmock() (or vi.unmock() when appropriate) between tests (and then re-import, since unmocking doesn’t change already-imported bindings) [1]. Also, Vitest caches a mock factory’s result until vi.unmock/vi.doUnmock is called [2].
Tip: vi.doMock() returns a disposable that can auto-call vi.doUnmock() via using, if your environment supports it [3].
Sources: [1] [2] [3]
Use vi.doUnmock() to clear module mocks between tests.
vi.resetModules() only clears the module cache and does not remove mocks registered with vi.doMock(). The vi.doMock('../scripts/hallucination-config.cjs', ...) calls from throwing-loader cases persist across test runs, so the later "loader works" assertions may pass only because DEFAULT_WEIGHTS exposes the same keys as the fallback, masking test isolation issues.
Suggested fix
beforeEach(() => {
vi.resetModules();
+ vi.doUnmock('../scripts/hallucination-config.cjs');
});Also applies to: 177-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hallucination-config-safe.test.cjs` around lines 4 - 6, The beforeEach
currently calls vi.resetModules() but leaves mocks registered with vi.doMock()
in place; update the test setup by calling
vi.doUnmock('../scripts/hallucination-config.cjs') (and any other modules you
previously vi.doMock()ed) in the beforeEach that contains vi.resetModules(), so
mocks are removed between runs and the "loader works" / DEFAULT_WEIGHTS
assertions run with clean mocks; ensure you add the same vi.doUnmock call(s) in
the other beforeEach block covering lines 177-193 to fully isolate tests.
…-0 contract Problem: hallucination-config.cjs (878 lines) bundled constants, TOML parser, validation, merge logic, and cascading config resolution. Hook scripts required it at module top level without try-catch — a syntax error in any of the 878 lines would crash hooks and disable hallucination detection. Solution: Separated Interface with Safe Default Wrapper. Extract 4 focused modules: - hallucination-config-defaults.cjs: pure data constants (cannot throw) - hallucination-config-toml.cjs: TOML parser - hallucination-config-merge.cjs: deep merge + freeze - hallucination-config-validate.cjs: schema validation Add hallucination-config-safe.cjs: lazy-requires the full loader inside function bodies, returns frozen defaults on any failure. Migrate consumers: - audit-stop: uses safeLoadConfig/safeLoadWeights, wraps all requires - session-start: uses safeLoadConfig, removes manual try-catch - annotate: imports DEFAULT_WEIGHTS directly from defaults leaf The orchestrator (hallucination-config.cjs) re-exports all 10 original symbols unchanged for backward compatibility. 169 new tests. Coverage: Lines 82%, Branches 70%, Functions 84%. https://claude.ai/code/session_01LQK9deMj1VncLCzexrmKdh
Jamie-BitFlight
force-pushed
the
claude/fix-sessionstart-require-34mwZ
branch
from
0578ab4 to
d7d31e6
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/hallucination-config-toml.cjs (2)
115-126:⚠️ Potential issue | 🔴 CriticalHarden parsed TOML maps against prototype pollution.
Lines 116, 137, and 154 build plain objects. Keys like
__proto__can mutate prototype behavior during section traversal/assignment.Suggested fix
function parseTomlInlineTable(content) { - const table = {}; + const table = Object.create(null); if (!content.trim()) return table; for (const pair of splitTopLevel(content.trim(), ',')) { @@ function parseToml(source) { - const result = {}; + const result = Object.create(null); let current = result; @@ for (const part of parts) { if (typeof current[part] !== 'object' || current[part] === null) { - current[part] = {}; + current[part] = Object.create(null); } current = current[part]; }Also applies to: 136-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hallucination-config-toml.cjs` around lines 115 - 126, The code constructs plain objects (e.g., in parseTomlInlineTable) which is vulnerable to prototype pollution; change object initializations from {} to Object.create(null) and, when assigning parsed keys (e.g., in parseTomlInlineTable and the other table/map builders around the same region), explicitly skip or reject dangerous keys such as "__proto__", "prototype", and "constructor" before calling parseTomlValue/assigning, so sections never mutate Object.prototype or add constructor properties.
83-90:⚠️ Potential issue | 🟠 MajorSeparate TOML literal vs basic string parsing.
Line 83 currently applies escape decoding to both quote styles. That mutates literal strings and can decode basic strings incorrectly when backslashes are involved.
Suggested fix
- if ((s.startsWith('"') && s.endsWith('"')) || (s.startsWith("'") && s.endsWith("'"))) { - return s - .slice(1, -1) - .replace(/\\n/g, '\n') - .replace(/\\t/g, '\t') - .replace(/\\\\/g, '\\') - .replace(/\\"/g, '"'); - } + if (s.startsWith("'") && s.endsWith("'")) { + return s.slice(1, -1); + } + if (s.startsWith('"') && s.endsWith('"')) { + return JSON.parse(s); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hallucination-config-toml.cjs` around lines 83 - 90, The current branch treats both double-quoted and single-quoted TOML strings the same and runs escape decoding for both; change it so double-quoted (basic) strings keep the escape replacements (the .replace(/\\n/g,...), .replace(/\\t/g,...), .replace(/\\\\/g,...), .replace(/\\"/g, '"') sequence applied when s.startsWith('"') is true) while single-quoted (literal) strings only return s.slice(1, -1) with no escape processing when s.startsWith("'") is true; update the if/else so the double-quote path applies decoding and the single-quote path returns the raw inner content.
🧹 Nitpick comments (1)
scripts/hallucination-audit-stop.cjs (1)
3179-3181: Aliased exports silently swallow config errors.Re-exporting
loadConfigandloadWeightsas aliases of the safe variants means any consumer (including tests) now gets silent fallback to defaults on any config error. This is intentional for the "Exit 0 always" contract, but be aware that:
- Tests expecting specific config behaviors may pass for unintended reasons (config error → silent fallback → defaults used).
- Configuration typos or invalid files are now silently ignored rather than surfaced.
Consider logging a warning to stderr (which won't affect hook behavior) when the safe wrapper catches an error, to aid debugging:
// In hallucination-config-safe.cjs } catch (e) { process.stderr.write(`[hallucination-detector] Config load error: ${e.message}\n`); return deepFreeze({ ...DEFAULT_CONFIG, ... }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hallucination-audit-stop.cjs` around lines 3179 - 3181, The aliased exports loadConfig/loadWeights (safeLoadConfig/safeLoadWeights) silently swallow errors; update the catch blocks inside hallucination-config-safe.cjs in the safeLoadConfig and safeLoadWeights implementations to write a concise warning to stderr (e.g., include `[hallucination-detector] Config load error:` plus the error message) before returning the default/deepFreeze fallback so failures are still surfaced to logs while keeping the "Exit 0 always" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/hallucination-config-toml.cjs`:
- Around line 115-126: The code constructs plain objects (e.g., in
parseTomlInlineTable) which is vulnerable to prototype pollution; change object
initializations from {} to Object.create(null) and, when assigning parsed keys
(e.g., in parseTomlInlineTable and the other table/map builders around the same
region), explicitly skip or reject dangerous keys such as "__proto__",
"prototype", and "constructor" before calling parseTomlValue/assigning, so
sections never mutate Object.prototype or add constructor properties.
- Around line 83-90: The current branch treats both double-quoted and
single-quoted TOML strings the same and runs escape decoding for both; change it
so double-quoted (basic) strings keep the escape replacements (the
.replace(/\\n/g,...), .replace(/\\t/g,...), .replace(/\\\\/g,...),
.replace(/\\"/g, '"') sequence applied when s.startsWith('"') is true) while
single-quoted (literal) strings only return s.slice(1, -1) with no escape
processing when s.startsWith("'") is true; update the if/else so the
double-quote path applies decoding and the single-quote path returns the raw
inner content.
---
Nitpick comments:
In `@scripts/hallucination-audit-stop.cjs`:
- Around line 3179-3181: The aliased exports loadConfig/loadWeights
(safeLoadConfig/safeLoadWeights) silently swallow errors; update the catch
blocks inside hallucination-config-safe.cjs in the safeLoadConfig and
safeLoadWeights implementations to write a concise warning to stderr (e.g.,
include `[hallucination-detector] Config load error:` plus the error message)
before returning the default/deepFreeze fallback so failures are still surfaced
to logs while keeping the "Exit 0 always" behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8825407-171c-494e-a0c9-69e92dc9ce3a
📒 Files selected for processing (15)
scripts/hallucination-annotate.cjsscripts/hallucination-audit-stop.cjsscripts/hallucination-config-defaults.cjsscripts/hallucination-config-merge.cjsscripts/hallucination-config-safe.cjsscripts/hallucination-config-toml.cjsscripts/hallucination-config-validate.cjsscripts/hallucination-config.cjsscripts/hallucination-framing-session-start.cjstests/hallucination-config-defaults.test.cjstests/hallucination-config-merge.test.cjstests/hallucination-config-safe.test.cjstests/hallucination-config-toml.test.cjstests/hallucination-config-validate.test.cjsvitest.config.cjs
✅ Files skipped from review due to trivial changes (3)
- vitest.config.cjs
- scripts/hallucination-framing-session-start.cjs
- scripts/hallucination-config-validate.cjs
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/hallucination-annotate.cjs
- tests/hallucination-config-toml.test.cjs
- tests/hallucination-config-safe.test.cjs
- tests/hallucination-config-defaults.test.cjs
- scripts/hallucination-config-safe.cjs
- scripts/hallucination-config.cjs
- tests/hallucination-config-merge.test.cjs
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
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.
Top-level
require('./hallucination-config.cjs')could crash the scriptwith exit code 1 before main() runs. Moving it inside main()'s existing
try-catch ensures any require failure falls through to default framing,
honoring the "Exit 0 always" contract.
https://claude.ai/code/session_01LQK9deMj1VncLCzexrmKdh
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests