Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents harmful generation debug logs from including sensitive request content (e.g., purpose, config prompt, user email) by logging only non-sensitive metadata.
Changes:
- Replaces the harmful-generation debug log body dump with a metadata-only log payload (lengths/flags/keys).
- Adds a focused unit test ensuring secret request content is not present in debug logs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/providers/promptfoo.ts |
Introduces a helper to log only safe metadata and updates the debug statement to use it. |
test/providers/promptfoo.test.ts |
Adds a regression test verifying secrets (purpose/config/email) are not emitted via logger.debug. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
👍 All Clear
I reviewed the changes to the Promptfoo harmful generation provider and its tests, focusing on LLM data flows, logging, and potential execution paths. The PR replaces detailed request logging with redacted metadata, reducing exposure of sensitive inputs like prompts and emails. No new LLM capabilities, inputs, or execution sinks were introduced. Overall, this change improves security posture and I did not find LLM security vulnerabilities introduced by this PR.
Minimum severity threshold: 🟡 Medium | To re-scan after changes, comment @promptfoo-scanner
Learn more
📝 WalkthroughWalkthroughThe changes add structured logging to the PromptfooHarmfulCompletionProvider to reduce exposure of sensitive request data. A new helper function Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/providers/promptfoo.test.ts (1)
101-107: Optional: assert structured debug args directly for stronger test precision.Stringifying all debug calls can become noisy over time. Consider asserting message/context arguments from the targeted call.
💡 Suggested test assertion refinement
- const debugLogs = JSON.stringify(debugSpy.mock.calls); - expect(debugLogs).toContain('[HarmfulCompletionProvider] Calling generate harmful API'); - expect(debugLogs).toContain('purposeLength'); - expect(debugLogs).toContain('configKeys'); - expect(debugLogs).not.toContain('secret-purpose-sentinel'); - expect(debugLogs).not.toContain('secret-config-sentinel'); - expect(debugLogs).not.toContain('test@example.com'); + expect(debugSpy).toHaveBeenCalledWith( + '[HarmfulCompletionProvider] Calling generate harmful API', + expect.objectContaining({ + purposeLength: 'secret-purpose-sentinel'.length, + configKeys: ['prompt'], + }), + ); + const loggedContext = debugSpy.mock.calls[0]?.[1]; + const contextStr = JSON.stringify(loggedContext); + expect(contextStr).not.toContain('secret-purpose-sentinel'); + expect(contextStr).not.toContain('secret-config-sentinel'); + expect(contextStr).not.toContain('test@example.com');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/providers/promptfoo.test.ts` around lines 101 - 107, Replace the stringified-wide assertions on debugSpy in test/providers/promptfoo.test.ts with targeted assertions on the specific debug call: find the call in debugSpy.mock.calls that includes the '[HarmfulCompletionProvider] Calling generate harmful API' message, assert that that call's context/args contain keys like purposeLength and configKeys and do not contain sensitive values 'secret-purpose-sentinel', 'secret-config-sentinel' or 'test@example.com'; use debugSpy, the literal message '[HarmfulCompletionProvider] Calling generate harmful API' and the keys purposeLength/configKeys to locate and assert the structured args rather than checking the full JSON string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/providers/promptfoo.test.ts`:
- Around line 101-107: Replace the stringified-wide assertions on debugSpy in
test/providers/promptfoo.test.ts with targeted assertions on the specific debug
call: find the call in debugSpy.mock.calls that includes the
'[HarmfulCompletionProvider] Calling generate harmful API' message, assert that
that call's context/args contain keys like purposeLength and configKeys and do
not contain sensitive values 'secret-purpose-sentinel', 'secret-config-sentinel'
or 'test@example.com'; use debugSpy, the literal message
'[HarmfulCompletionProvider] Calling generate harmful API' and the keys
purposeLength/configKeys to locate and assert the structured args rather than
checking the full JSON string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1c97205-49e6-4784-9160-88669d6c29b6
📒 Files selected for processing (2)
src/providers/promptfoo.tstest/providers/promptfoo.test.ts
…l-generation-logs
…l-generation-logs
Stops harmful generation debug logs from dumping request content.