review: fix captureDisabled module-global cache poisoning#43
review: fix captureDisabled module-global cache poisoning#43kridaydave wants to merge 8 commits into
Conversation
Part of the review split of PR Shreyan1#39 (kridaydave:fix/good-first-issues). This PR handles concern Shreyan1#3 only. The async refactor introduced a module-global `cachedIgnore` that checkIgnoreAsync() wrote and captureDisabled() read. Reproduced bug: a test that sets CC_HABITS_DISABLE and calls checkIgnoreAsync() caches true; a later test that clears the env and calls captureDisabled() expecting false gets true — the stale cached value. In the shared vitest worker this silently poisons captureDisabled() for any later test depending on order, which is exactly the intermittent flakiness the description already flagged. Remove the module-global entirely. Both captureDisabled() and checkIgnoreAsync() now compute the ignore decision fresh on every call (env var + .cc-habits-ignore check), so nothing leaks across invocations. The env var and cwd are read fresh each time, which is free and correct for the short-lived hook process in production. captureDisabled(cached?) optionally accepts a single-invocation value, and the hook handlers thread the value from checkIgnoreAsync() through so there is still no redundant filesystem hit within one invocation. Split from Shreyan1#39; the other three concerns (drop Antigravity, bounded concurrency, config-parse dedup) land in their own PRs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| export function handleUserPromptSubmit(): void { | ||
| export async function handleUserPromptSubmit(): Promise<void> { | ||
| const ignored = await checkIgnoreAsync(); |
Shreyan1
left a comment
Shreyan1
left a comment
There was a problem hiding this comment.
The fix is correct, Kriday. The module-global is gone, both captureDisabled() and checkIgnoreAsync() compute fresh, and the single-invocation value is threaded through so there is no redundant fs hit in the hot path. That is the right resolution.
The one thing this PR must have and does not: a regression test for the bug it fixes (inline). This is the exact poisoning I reproduced, and without a test locking it, it silently comes back the next time someone reaches for a cache.
Same two structural blockers as #42: it still carries antigravity (rebase on #41 to clear the parity gate) and it needs a version bump. The CodeQL HIGH is from the carried #39 base and clears on rebase.
| } | ||
| } | ||
|
|
||
| export function captureDisabled(cached?: boolean | null): boolean { |
There was a problem hiding this comment.
This is the correct fix, computing fresh each call and letting the caller thread a single-invocation value is exactly right. What is missing is the test. Please add the regression I reproduced: set CC_HABITS_DISABLE, call checkIgnoreAsync() (this is what used to cache true), clear the env, then assert captureDisabled() === false. That one test is the whole point of this PR, it is what stops the poisoning from coming back.
|
|
||
| // Adapter ids that have a dedicated hook adapter (used to assert parity in tests). | ||
| export const HOOK_ADAPTERS = ['claude-code', 'gemini', 'codex', 'cline', 'kimi']; | ||
| export const HOOK_ADAPTERS = ['claude-code', 'gemini', 'codex', 'cline', 'kimi', 'antigravity']; |
3 participants
Split out of PR #39 (kridaydave:fix/good-first-issues) to address review concern #3.
The async refactor introduced a module-global
cachedIgnorethatcheckIgnoreAsync()wrote andcaptureDisabled()read. Reproduced bug: a test that setsCC_HABITS_DISABLEand callscheckIgnoreAsync()cachestrue; a later test that clears the env and callscaptureDisabled()expectingfalsegetstrue— the stale cached value. In the shared vitest worker this silently poisonscaptureDisabled()for any later test depending on order, which is exactly the intermittent flakiness the description already flagged.Remove the module-global entirely. Both
captureDisabled()andcheckIgnoreAsync()now compute the ignore decision fresh on every call (env var +.cc-habits-ignorecheck), so nothing leaks across invocations. The env var and cwd are read fresh each time, which is free and correct for the short-lived hook process in production.captureDisabled(cached?)optionally accepts a single-invocation value, and the hook handlers thread the value fromcheckIgnoreAsync()through so there is still no redundant filesystem hit within one invocation.The other three concerns (drop Antigravity #1, bounded concurrency #2, config-parse dedup #4) land in their own PRs, also split from #39.