fix(e2e): skip lockfiles in snapshot leak scan#3617
Conversation
This test demonstrates the nightly snapshot failure where dependency lockfiles can match broad credential patterns. Expected: runtime config/env files are scanned, dependency lockfiles are skipped. Actual: no shared helper exists, so lockfiles are scanned by the E2E script.
Dependency lockfiles can contain non-secret package metadata that matches broad credential patterns such as sk-. Keep scanning runtime JSON and env files, but exclude lockfiles from the coarse snapshot E2E leak check.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExclude common package-manager lockfiles from snapshot credential scans, add shouldScanSnapshotFileForCredentials with basename-based exclusions and tests, update the Phase 8 E2E scan script accordingly, and add an npm install --before timestamp to the E2E Advisor workflow. ChangesE2E Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/credential-filter.test.ts (1)
157-162: ⚡ Quick winAdd lockfile-variant and path-form regression assertions.
Please also cover
pnpm-lock.ymland a path input case so basename-based exclusion stays guarded.Suggested test additions
it("skips dependency lockfiles that can contain non-secret package metadata matches", () => { expect(shouldScanSnapshotFileForCredentials("package-lock.json")).toBe(false); expect(shouldScanSnapshotFileForCredentials("npm-shrinkwrap.json")).toBe(false); expect(shouldScanSnapshotFileForCredentials("yarn.lock")).toBe(false); expect(shouldScanSnapshotFileForCredentials("pnpm-lock.yaml")).toBe(false); + expect(shouldScanSnapshotFileForCredentials("pnpm-lock.yml")).toBe(false); + expect(shouldScanSnapshotFileForCredentials("/tmp/package-lock.json")).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/credential-filter.test.ts` around lines 157 - 162, Add assertions to the existing test that verify shouldScanSnapshotFileForCredentials also excludes the alternative pnpm lockfile name "pnpm-lock.yml" and that basename-based exclusion still works when a full/relative path is supplied (e.g., "path/to/package-lock.json" should return false). Update the test block in credential-filter.test.ts where shouldScanSnapshotFileForCredentials is exercised to include these two additional expect checks so both filename variants and path inputs are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/credential-filter.ts`:
- Around line 173-177: The function shouldScanSnapshotFileForCredentials
lowercases the full path instead of the file's basename so
SNAPSHOT_CREDENTIAL_SCAN_EXCLUDED_BASENAMES won't match paths like
/tmp/package-lock.json; change the implementation to first obtain the basename
(use Node's path.basename on filename), then lowercase that basename and use it
for the exclusion check
(SNAPSHOT_CREDENTIAL_SCAN_EXCLUDED_BASENAMES.has(basename)) and for the
subsequent checks (basename === ".env" || basename.endsWith(".env") ||
basename.endsWith(".json")). Ensure you import/require path if not already
present.
---
Nitpick comments:
In `@src/lib/credential-filter.test.ts`:
- Around line 157-162: Add assertions to the existing test that verify
shouldScanSnapshotFileForCredentials also excludes the alternative pnpm lockfile
name "pnpm-lock.yml" and that basename-based exclusion still works when a
full/relative path is supplied (e.g., "path/to/package-lock.json" should return
false). Update the test block in credential-filter.test.ts where
shouldScanSnapshotFileForCredentials is exercised to include these two
additional expect checks so both filename variants and path inputs are covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2ba309cc-8e0d-44a7-964f-43a5a5847c32
📒 Files selected for processing (3)
src/lib/credential-filter.test.tssrc/lib/credential-filter.tstest/e2e/test-snapshot-commands.sh
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Selective E2E Results — ✅ All requested jobs passedRun: 25935470561
|
Selective E2E Results — ✅ All requested jobs passedRun: 25935944925
|
Selective E2E Results — ✅ All requested jobs passedRun: 25936516884
|
Summary
Root cause
Nightly
snapshot-commands-e2efailed in runs25931606052and25933526098because Phase 8 scans all*.jsonfiles under snapshot backups for broad patterns likesk-. Dependency lockfiles such asextensions/openclaw-weixin/package-lock.jsoncan contain non-secret package metadata matching those patterns, causing a false positive.Validation
npm test -- --run src/lib/credential-filter.test.tsbash -n test/e2e/test-snapshot-commands.shnpm test -- --run test/snapshot.test.ts src/lib/credential-filter.test.tsnpm run build:cliFixes nightly E2E snapshot false positive observed in runs 25931606052 and 25933526098.
Summary by CodeRabbit
Tests
Chores