fix(session-start): move config require inside try-catch to honor exit-0 contract#72
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.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughSplit configuration logic into focused modules: defaults, TOML parsing, deep-merge/freeze, and validation; added 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 (framing/annotate/audit)
participant Safe as safeLoadConfig / safeLoadWeights
participant TOML as parseToml
participant Validate as validateConfig
participant Merge as mergeConfig
participant Defaults as DEFAULT_* constants
CLI->>Safe: request config (opts)
Safe->>TOML: parse project TOML (if present)
TOML-->>Safe: parsed config
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_* when loader/parse fails
Safe-->>CLI: return frozen config (merged or fallback)
CLI->>CLI: proceed with main logic using returned config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 19 seconds.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: 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
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| }); |
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
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
|
Verify each finding against the current code and only fix it if needed. Inline comments:
In
In
In
In
In
|
|
@copilot Verify each finding against the current code and only fix it if needed. Duplicate comments:
Nitpick comments:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const validateClaimStructure = _claim_structure?.validateClaimStructure || null; | ||
| const CLAIM_LABEL_ALTERNATION = _claim_structure?.CLAIM_LABEL_ALTERNATION || null; |
There was a problem hiding this comment.
validateClaimStructure is allowed to become null when require('./hallucination-claim-structure.cjs') fails, but the rest of the file calls it unconditionally later (e.g. validateClaimStructure(lastAssistantText)). This will throw a TypeError and break the hook’s “exit 0 always” contract. Make validateClaimStructure always a function (e.g. a stub that returns { structured: false, valid: true, errors: [] }) and give CLAIM_LABEL_ALTERNATION a safe fallback so downstream regex construction remains coherent when the module can’t be loaded.
| const validateClaimStructure = _claim_structure?.validateClaimStructure || null; | |
| const CLAIM_LABEL_ALTERNATION = _claim_structure?.CLAIM_LABEL_ALTERNATION || null; | |
| const validateClaimStructure = | |
| _claim_structure?.validateClaimStructure || | |
| (() => ({ structured: false, valid: true, errors: [] })); | |
| const CLAIM_LABEL_ALTERNATION = | |
| _claim_structure?.CLAIM_LABEL_ALTERNATION || 'claim'; |
| return deepFreeze({ | ||
| ...DEFAULT_CONFIG, | ||
| weights: { ...DEFAULT_WEIGHTS }, | ||
| thresholds: { ...DEFAULT_THRESHOLDS }, | ||
| confidenceWeights: { ...DEFAULT_CONFIDENCE_WEIGHTS }, | ||
| categories: {}, | ||
| ignorePatterns: [], | ||
| ignoreBlocks: [], | ||
| evidenceMarkers: [], | ||
| allowlist: [], | ||
| responseTemplates: {}, | ||
| }); |
There was a problem hiding this comment.
In the fallback path, deepFreeze({ ...DEFAULT_CONFIG, ... }) will freeze any nested objects/arrays that are still referenced from DEFAULT_CONFIG (because the spread is shallow). At minimum ignoreCategories is currently not overridden here, so calling safeLoadConfig() can end up freezing DEFAULT_CONFIG.ignoreCategories as a side effect. Build the fallback config without reusing nested references from DEFAULT_CONFIG (explicitly clone all array/object fields, including ignoreCategories), then deep-freeze the clone.
|
@copilot update the min version of node to strictly be higher than when node:sqlite became available. |
All findings verified and fixed in commit
|
|
@copilot please rebase |
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/hallucination-config-merge.cjs`:
- Around line 59-67: The current logic destructures and spreads overCat assuming
it's a plain object, but arrays passed as overCat produce malformed merged
category objects; update the early non-mergeable check in the merge loop to
treat arrays the same as non-objects by changing the condition to also test
Array.isArray(overCat) (i.e., if typeof overCat !== 'object' || overCat === null
|| Array.isArray(overCat)) then set merged[catName] = overCat and continue;
ensure this check occurs before any destructuring of baseCat/overCat and before
constructing mergedCat (references: overCat, baseCat, merged[catName],
customPatterns, replacePatterns, mergedCat).
🪄 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: 560c2b7c-2145-4438-95e6-854d89277cb6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
scripts/hallucination-audit-stop.cjsscripts/hallucination-config-merge.cjsscripts/hallucination-config-safe.cjsscripts/hallucination-config-toml.cjsscripts/hallucination-config-validate.cjstests/hallucination-config-safe.test.cjs
✅ Files skipped from review due to trivial changes (1)
- scripts/hallucination-config-safe.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/hallucination-config-toml.cjs
| if (typeof overCat !== 'object' || overCat === null) { | ||
| merged[catName] = overCat; | ||
| continue; | ||
| } | ||
| // Extract customPatterns and replacePatterns before spreading to handle | ||
| // replacePatterns:true correctly even when overCat.customPatterns is absent. | ||
| const { customPatterns: basePatterns, ...baseCatRest } = baseCat; | ||
| const { customPatterns: overPatterns, replacePatterns, ...overCatRest } = overCat; | ||
| const mergedCat = { ...baseCatRest, ...overCatRest }; |
There was a problem hiding this comment.
Handle array category overrides as non-mergeable values.
At Line 59, overCat arrays currently flow into object-destructuring/spread logic, which can yield malformed merged category objects (numeric keys, unexpected carry-over fields). Arrays should be treated like other non-plain override values.
Suggested patch
- if (typeof overCat !== 'object' || overCat === null) {
+ if (typeof overCat !== 'object' || overCat === null || Array.isArray(overCat)) {
merged[catName] = overCat;
continue;
}
+ const baseCatObj =
+ typeof baseCat === 'object' && baseCat !== null && !Array.isArray(baseCat)
+ ? baseCat
+ : {};
// Extract customPatterns and replacePatterns before spreading to handle
// replacePatterns:true correctly even when overCat.customPatterns is absent.
- const { customPatterns: basePatterns, ...baseCatRest } = baseCat;
+ const { customPatterns: basePatterns, ...baseCatRest } = baseCatObj;🤖 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 59 - 67, The current
logic destructures and spreads overCat assuming it's a plain object, but arrays
passed as overCat produce malformed merged category objects; update the early
non-mergeable check in the merge loop to treat arrays the same as non-objects by
changing the condition to also test Array.isArray(overCat) (i.e., if typeof
overCat !== 'object' || overCat === null || Array.isArray(overCat)) then set
merged[catName] = overCat and continue; ensure this check occurs before any
destructuring of baseCat/overCat and before constructing mergedCat (references:
overCat, baseCat, merged[catName], customPatterns, replacePatterns, mergedCat).
Verified in commit
Done in commit |
Done in commit |
|
@copilot it's failing biome |
Fixed in commit
Biome now exits 0, all 589 tests still pass. |
|
🎉 This PR is included in version 1.17.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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