fix(test): replace realistic API key values with obviously-fake patterns#1482
fix(test): replace realistic API key values with obviously-fake patterns#1482dknos wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughTest credential literals in many test files were replaced with clearly-marked placeholder tokens (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
Comment |
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 `@test/onboard-selection.test.js`:
- Around line 1813-1816: The curl stub currently only treats
'nvapi-TEST-GOOD-NOT-REAL-000000' as a valid token while the test retries using
'nvapi-good' and asserts payload.key contains that value; update the stub so the
auth check matches the retry token. Concretely, modify the conditional that
tests the auth variable (the grep against 'nvapi-TEST-GOOD-NOT-REAL-000000') to
also accept 'nvapi-good' (or match a common prefix like 'nvapi-good') so the
branch that returns a 200 and body='{"id":"resp_123"}' will run for the retry
token; ensure consistency with the test assertion that inspects payload.key and
keep the url checks for '/responses$' and '/chat/completions$' unchanged.
🪄 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: Pro
Run ID: 01487594-6dc5-437c-acb9-4aef42c3bf85
📒 Files selected for processing (10)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/commands/migration-state.test.tssrc/lib/debug.test.tssrc/lib/onboard-session.test.tssrc/lib/validation.test.tstest/credentials.test.jstest/onboard-selection.test.jstest/onboard.test.jstest/runner.test.jstest/smoke-macos-install.test.js
|
✨ Thanks for submitting this fix, which proposes a way to replace realistic-looking API key values in test files with obviously fake ones to avoid credential leakage alerts. Possibly related open issues: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Ready to merge. All checks pass, CodeRabbit found no issues. This secures test fixtures against credential scanner false positives — fixes #1440. |
70706a7 to
f9ce195
Compare
Test fixtures used values like nvapi-secret-value and sk-bad that trigger secret scanners. Replaced with clearly-marked test values. Fixes NVIDIA#1440 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dknos <rneebo@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: dknos <rneebo@gmail.com>
f9ce195 to
3d3437c
Compare
|
Thanks for this fix — replacing realistic-looking API key patterns in tests with obviously-fake values is good hygiene and worth having. The codebase has changed significantly since this was opened, including a TypeScript migration (#1673) that moved most logic from |
|
Thanks for the thoughtful review @wscurran. Going to step back on this one — the TypeScript migration has moved the test layout enough that a clean rebase would effectively be rewriting the patch, and the underlying hygiene point is minor. Please feel free to pull any of the fake-key patterns into future tests if useful. Closing to reduce queue noise. |
Summary
nvapi-secret-value→nvapi-TEST-NOT-A-REAL-KEY-000000)Test plan
npm testto verify all tests still pass with new valuesFixes #1440
🤖 Generated with Claude Code
Summary by CodeRabbit
Signed-off-by: dknos rneebo@gmail.com