Skip to content

test(security): fix flake in skill scanner cache invalidation test#32159

Closed
markfietje wants to merge 2 commits intoopenclaw:mainfrom
markfietje:fix/security-scanner-test-flake
Closed

test(security): fix flake in skill scanner cache invalidation test#32159
markfietje wants to merge 2 commits intoopenclaw:mainfrom
markfietje:fix/security-scanner-test-flake

Conversation

@markfietje
Copy link
Contributor

@markfietje markfietje commented Mar 2, 2026

Summary

This PR fixes a CI flake in src/security/skill-scanner.test.ts where the cache invalidation test could fail if the filesystem mtime resolution was too low.

Problem

The test reuses cached findings for unchanged files and invalidates on file updates was updating a file with content of the exact same size:

fsSync.writeFileSync(filePath, `const x = eval("1+1");`); // 22 bytes
// ...
await fs.writeFile(filePath, `const x = eval("2+2");`, "utf-8"); // 22 bytes

Since the skill scanner cache uses both size and mtimeMs for invalidation, if the writeFile occurred within the same millisecond (or second, depending on FS resolution) as the first write, the scanner would incorrectly hit the cache, causing the readSpy to only be called once instead of twice.

Solution

Change the second write to have a different file size. This ensures the cache is invalidated even if mtimeMs remains the same.

Verification

  • Reproduction: Confirmed that when file size and mtime are forced to be identical, the test fails.
  • Verified: Verified that changing the file size fixes the potential flake.
  • CI Checks: pnpm check and pnpm test src/security/skill-scanner.test.ts pass locally.

This was referenced Mar 2, 2026
Merged
Closed
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 2, 2026

Greptile Summary

This PR fixes a CI flake in the skill scanner cache invalidation test by ensuring the file size changes when testing cache invalidation. The original test wrote files with identical byte sizes (eval("1+1") and eval("2+2") are both 22 bytes), which could cause the cache to incorrectly persist if the filesystem's mtime resolution was too coarse. The fix adds a comment to the second write to guarantee a different file size, ensuring the cache is always invalidated as expected.

  • Simple and effective fix that addresses the root cause of the flake
  • Test logic remains unchanged and still validates the cache behavior correctly
  • Aligns with the cache implementation which uses both size and mtimeMs for invalidation

Confidence Score: 5/5

  • This PR is safe to merge with no risk - it's a minimal test-only fix that addresses a real flake
  • Score reflects that this is a test-only change with a minimal, well-understood fix. The change adds a comment to ensure file size differs, which directly addresses the cache invalidation race condition described in the PR. No production code is affected, and the test logic remains correct.
  • No files require special attention

Last reviewed commit: 1c75863

@markfietje
Copy link
Contributor Author

markfietje commented Mar 2, 2026

⚠️ Note on CI Failure
Intermittent CI failures are also being caused by a flake in src/cron/isolated-agent.uses-last-non-empty-agent-text-as.test.ts. A fix is in PR #32165.

@markfietje markfietje force-pushed the fix/security-scanner-test-flake branch from 4d60142 to 7dbb6ad Compare March 2, 2026 21:00
@ademczuk ademczuk mentioned this pull request Mar 2, 2026
Merged
3 tasks
@markfietje
Copy link
Contributor Author

markfietje commented Mar 2, 2026

Closing as this has been superseded by internal stabilization fixes in main (e.g., 2f35230). Glad to see the scanner flake is addressed.

@markfietje markfietje closed this Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@markfietje