feat(redteam): Cloud-based plugin severity overrides #4348
Conversation
TestGru AssignmentSummary
Files
Tip You can |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| return provider.label || `custom-function-${idx}`; | ||
| } | ||
| if ((provider as ProviderOptions).id) { | ||
| return (provider as ProviderOptions).id!; |
There was a problem hiding this comment.
Avoid using the non-null assertion operator ! when returning provider.id. Since you already check if provider.id exists, replace (provider as ProviderOptions).id! with a safe access or add an invariant to guarantee its presence.
| return (provider as ProviderOptions).id!; | |
| return (provider as ProviderOptions).id; |
This comment was generated because it violated a code review rule: irule_xyCqADUsMuORnlRY.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @will-holley - I've reviewed your changes - here's some feedback:
- In getPluginSeverityOverridesFromCloud you’re swallowing the original error context—consider wrapping and re-throwing the HTTP status and response body to preserve diagnostic info.
- The getProviderIds helper has become quite complex with multiple input types—consider simplifying its branches or adding in-code comments to clarify expected behavior for each case.
- When applying plugin severity overrides in doGenerateRedteam, overrides that don’t match any loaded plugin are dropped silently—consider logging a warning if an override ID has no corresponding plugin to catch misconfigurations.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
TestGru AssignmentSummary
Files
Tip You can |
sklein12
left a comment
sklein12
left a comment
There was a problem hiding this comment.
Sorry to be the bearer 🐻 of bad news. We should do this in the cloud in our shared utilities for creating unified configs.
|
Tip For best results, initiate chat on the files or code changes. @mldangelo, hello! I see you've mentioned me. How can I help you today? If you have any questions about the code, need analysis of specific files, or would like me to review something in particular, just let me know! |
WalkthroughThe changes introduce new utility functions for cloud provider detection and ID extraction, refactor provider validation logic to use these utilities, and add support for fetching and applying plugin severity overrides from cloud providers during redteam generation. Associated tests are updated to mock the new utilities and provider ID extraction logic. Changes
Poem
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
npm error Exit handler never called! ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/providers.test.ts (1)
35-35: Use the imported constant instead of hardcoding the cloud provider prefix.As mentioned in a previous review, consider using the
CLOUD_PROVIDER_PREFIXconstant (already imported on line 5) instead of hardcoding'promptfoo://provider/'in the cloud utility mocks. This improves consistency and maintainability.- .mockImplementation((path: string) => path.startsWith('promptfoo://provider/')); + .mockImplementation((path: string) => path.startsWith(CLOUD_PROVIDER_PREFIX));- .mockImplementation((path: string) => path.slice('promptfoo://provider/'.length)); + .mockImplementation((path: string) => path.slice(CLOUD_PROVIDER_PREFIX.length));Also applies to: 38-38
src/providers/index.ts (1)
264-264: Remove the non-null assertion operator for safer code.As mentioned in a previous review, avoid using the non-null assertion operator
!when returningprovider.id. Since you already check ifprovider.idexists, this should be safe to remove.- return (provider as ProviderOptions).id!; + return (provider as ProviderOptions).id;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/providers/index.ts(3 hunks)src/redteam/commands/generate.ts(7 hunks)src/util/cloud.ts(3 hunks)test/providers.test.ts(2 hunks)test/redteam/commands/generate.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/redteam/commands/generate.ts (1)
src/util/cloud.ts (2)
isCloudProvider(96-98)getCloudDatabaseId(100-102)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tusk Tester
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
test/redteam/commands/generate.test.ts (1)
68-68: LGTM! Mock is appropriate for testing redteam generation logic.The mock for
getProviderIdsis well-placed and returns test data consistent with other provider mocks in the file. This allows the test to focus on redteam generation behavior rather than provider ID extraction logic.src/providers/index.ts (2)
11-11: Excellent refactoring to use cloud utility functions.The replacement of direct prefix checks with
isCloudProviderandgetCloudDatabaseIdutilities improves code organization and maintainability. This encapsulates cloud provider logic in a centralized location.Also applies to: 33-34, 37-37
250-272: Well-implemented utility function with comprehensive input handling.The
getProviderIdsfunction effectively handles various provider input types and provides a clean interface for extracting provider IDs. The error handling for invalid inputs is appropriate.src/util/cloud.ts (3)
96-102: Clean and well-encapsulated cloud provider utility functions.The
isCloudProviderandgetCloudDatabaseIdfunctions provide a clear interface for cloud provider detection and ID extraction, improving code organization and reusability.
50-53: Good enhancement to provider configuration handling.The fallback from
nametolabelprovides a sensible default when no explicit label is configured, improving user experience.
109-157: Comprehensive implementation of plugin severity overrides feature.The function includes proper error handling, detailed logging, and robust response parsing. The null return for missing associations provides clear semantics for downstream consumers.
src/redteam/commands/generate.ts (1)
136-138: Good catch switching fromconsttolet.Re-declaring
pluginSeverityOverridesasletunblocks later reassignment and resolves the issue raised in the previous review cycle.
| ...(pluginSeverityOverridesId ? { pluginSeverityOverridesId } : {}), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
pluginSeverityOverridesId is only persisted in one code path – leads to inconsistent metadata
pluginSeverityOverridesId is written to the YAML when options.output is provided (lines 404-406) but not when:
- The user appends (
--write, lines 462-468) - Generation defaults to
redteam.yaml(lines 506-507)
As a result, downstream tooling cannot reliably rely on this field.
Refactor the metadata construction in both branches to include the override ID, e.g.:
- metadata: {
+ metadata: {
...(existingConfig.metadata || {}),
+ ...(pluginSeverityOverridesId ? { pluginSeverityOverridesId } : {}),
},Apply the same patch to the “default redteam.yaml” branch.
Also applies to: 462-468, 506-507
🤖 Prompt for AI Agents
In src/redteam/commands/generate.ts around lines 404-406, 462-468, and 506-507,
the pluginSeverityOverridesId field is only included in the metadata when
options.output is provided, causing inconsistent metadata. Refactor the metadata
construction in all these code paths to consistently include
pluginSeverityOverridesId by merging it into the metadata object wherever it is
created or written, ensuring downstream tools can reliably access this field.
sklein12
left a comment
sklein12
left a comment
There was a problem hiding this comment.
two comments about error messages then it's good to go. Please address those before merging
|
⏩ No test scenarios generated (5fe5357) View output ↗ Tip New to Tusk? Learn more here. View check history
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/util/cloud.ts (1)
120-124: Typo in error message string
formattedErrorMessagereads “Failed to provider from cloud”. Looks like “fetch provider” was intended.- const formattedErrorMessage = `Failed to provider from cloud: ${errorMessage}. HTTP Status: ${response.status} -- ${response.statusText}.`; + const formattedErrorMessage = `Failed to fetch provider from cloud: ${errorMessage}. HTTP Status: ${response.status} -- ${response.statusText}.`;src/redteam/index.ts (2)
723-725: Duplicate severity-fallback logic – centralise to avoid driftSeverity resolution here duplicates the logic embedded a few lines below for custom plugins and elsewhere. Consider wrapping this in a helper (e.g.
resolveSeverity(plugin)) to keep behaviour consistent and remove repetition.
770-771: Inconsistent nullish/boolean coalescingHere you use the
||operator, whereas the non-custom branch uses??.
||will ignore valid falsy severities (should you ever extend the enum) while??only checks fornull/undefined. Recommend aligning with??for consistency.- severity: - plugin.severity || getPluginSeverity(plugin.id, resolvePluginConfig(plugin.config)), + severity: + plugin.severity ?? getPluginSeverity(plugin.id, resolvePluginConfig(plugin.config)),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/providers/index.ts(3 hunks)src/redteam/commands/generate.ts(7 hunks)src/redteam/index.ts(2 hunks)src/util/cloud.ts(2 hunks)test/redteam/commands/generate.test.ts(1 hunks)test/redteam/validators.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/providers/index.ts
- src/redteam/commands/generate.ts
- test/redteam/commands/generate.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/**`: - This is a CLI tool so errors need to be handled gracefully and logged with lots of information so the user can give us enough data to fix the issue or pass it to the de...
src/**: - This is a CLI tool so errors need to be handled gracefully and logged with lots of information so the user can give us enough data to fix the issue or pass it to the developers.
src/redteam/index.tssrc/util/cloud.ts
🧬 Code Graph Analysis (2)
test/redteam/validators.test.ts (1)
src/validators/redteam.ts (1)
RedteamConfigSchema(165-379)
src/util/cloud.ts (3)
src/constants.ts (1)
CLOUD_PROVIDER_PREFIX(45-45)src/globalConfig/cloud.ts (1)
cloudConfig(140-140)src/logger.ts (1)
logger(191-203)
🪛 Biome (1.9.4)
src/util/cloud.ts
[error] 154-154: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (2)
src/util/cloud.ts (1)
152-158: Quadratic spread on accumulator – switch to direct assignmentUsing the spread operator inside
reducerecreates the accumulator on every iteration (O(n²)). Construct the object in-place instead.- severities: pluginSeveritySet.members.reduce( - (acc: Record<Plugin, Severity>, member: { pluginId: Plugin; severity: Severity }) => ({ - ...acc, - [member.pluginId]: member.severity, - }), - {}, - ), + severities: pluginSeveritySet.members.reduce( + (acc: Record<Plugin, Severity>, { pluginId, severity }) => { + acc[pluginId] = severity; + return acc; + }, + {} as Record<Plugin, Severity>, + ),[ suggest_essential_refactor ]
🧰 Tools
🪛 Biome (1.9.4)
[error] 154-154: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
test/redteam/validators.test.ts (1)
891-950: Tests accurately cover newseveritybehaviourThe added cases thoroughly verify preservation of
severityfor direct plugins, mixed presence/absence, and collection expansion – good coverage.
Summary by CodeRabbit
New Features
Bug Fixes
Tests