feat(analysis): add provider and source filtering to analysis requests - #595
Open
ruromero wants to merge 1 commit into
Open
feat(analysis): add provider and source filtering to analysis requests#595ruromero wants to merge 1 commit into
ruromero wants to merge 1 commit into
Conversation
Add support for passing `providers` and `sources` query parameters to the DA `/api/v5/analysis` and `/api/v5/batch-analysis` endpoints. This allows callers to filter which vulnerability providers and sources are used in analysis reports. - Extract shared `appendAnalysisQueryParams()` helper that consolidates recommend, providers, and sources query parameter logic - Add `--providers` and `--sources` CLI flags to stack, component, and stack-batch commands - Support `TRUSTIFY_DA_PROVIDERS` and `TRUSTIFY_DA_SOURCES` env vars as fallbacks when CLI flags are not provided - Add unit tests for URL construction, env var fallback, and HTTP request forwarding Implements TC-5409 Assisted-by: Claude Code
Reviewer's GuideAdds provider/source filtering support to analysis requests by wiring new CLI flags through to shared URL query construction logic, including env-var fallbacks, and extending tests to cover URL building and HTTP forwarding. Sequence diagram for provider/source filtering in analysis requestssequenceDiagram
actor User
participant CLI_stack as cli.stack
participant client
participant requestStack
participant appendAnalysisQueryParams
participant Backend
User ->> CLI_stack: run stack --providers p1,p2 --sources s1
CLI_stack ->> client: stackAnalysis(manifest, html, opts)
client ->> requestStack: requestStack(provider, manifest, url, html, opts)
requestStack ->> appendAnalysisQueryParams: appendAnalysisQueryParams(finalUrl, opts)
alt [TRUSTIFY_DA_RECOMMEND is false]
appendAnalysisQueryParams ->> appendAnalysisQueryParams: url.searchParams.append(recommend,false)
end
alt [providers set via CLI or TRUSTIFY_DA_PROVIDERS]
appendAnalysisQueryParams ->> appendAnalysisQueryParams: url.searchParams.append(providers,providers)
end
alt [sources set via CLI or TRUSTIFY_DA_SOURCES]
appendAnalysisQueryParams ->> appendAnalysisQueryParams: url.searchParams.append(sources,sources)
end
requestStack ->> Backend: fetch(finalUrl, fetchOptions)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In appendAnalysisQueryParams, consider using url.searchParams.set rather than append so repeat invocations (or future callers) don’t accumulate duplicate recommend/providers/sources query params.
- The providers/sources CLI wiring is duplicated across component, stack, and stackBatch handlers; a small helper to apply these options to an opts object would keep the behavior consistent and reduce copy/paste.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In appendAnalysisQueryParams, consider using url.searchParams.set rather than append so repeat invocations (or future callers) don’t accumulate duplicate recommend/providers/sources query params.
- The providers/sources CLI wiring is duplicated across component, stack, and stackBatch handlers; a small helper to apply these options to an opts object would keep the behavior consistent and reduce copy/paste.
## Individual Comments
### Comment 1
<location path="src/analysis.js" line_range="247-259" />
<code_context>
+ * @param {URL} url - the URL object to append query parameters to
+ * @param {import("index.js").Options} [opts={}] - options containing parameter overrides
+ */
+function appendAnalysisQueryParams(url, opts = {}) {
+ if (getCustom('TRUSTIFY_DA_RECOMMEND', 'true', opts) === 'false') {
+ url.searchParams.append('recommend', 'false');
+ }
+ const providers = getCustom('TRUSTIFY_DA_PROVIDERS', null, opts);
+ if (providers) {
+ url.searchParams.append('providers', providers);
+ }
+ const sources = getCustom('TRUSTIFY_DA_SOURCES', null, opts);
+ if (sources) {
+ url.searchParams.append('sources', sources);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use `set` instead of `append` for query params to avoid accidental duplicates
If this helper is called multiple times with the same URL (or the URL is reused), `searchParams.append` on `recommend`, `providers`, and `sources` will create duplicate query params. Using `searchParams.set` keeps the function idempotent and prevents subtle issues as call sites change.
```suggestion
function appendAnalysisQueryParams(url, opts = {}) {
if (getCustom('TRUSTIFY_DA_RECOMMEND', 'true', opts) === 'false') {
url.searchParams.set('recommend', 'false');
}
const providers = getCustom('TRUSTIFY_DA_PROVIDERS', null, opts);
if (providers) {
url.searchParams.set('providers', providers);
}
const sources = getCustom('TRUSTIFY_DA_SOURCES', null, opts);
if (sources) {
url.searchParams.set('sources', sources);
}
}
```
</issue_to_address>
### Comment 2
<location path="test/analysis.test.js" line_range="236-241" />
<code_context>
+ expect(url.searchParams.get('sources')).to.equal('osv,nvd')
+ })
+
+ /** Verifies that no providers or sources params are added when neither is set. */
+ test('does not append providers or sources when neither is set', () => {
+ const url = new URL('http://example.com/api/v5/analysis')
+ analysis.appendAnalysisQueryParams(url, {})
+ expect(url.searchParams.has('providers')).to.equal(false)
+ expect(url.searchParams.has('sources')).to.equal(false)
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Cover behavior of recommend param when true/unspecified to avoid regressions
`appendAnalysisQueryParams` is only tested for the `recommend=false` case. Since this helper now owns the `recommend` param, please add tests that:
- Call it with no `TRUSTIFY_DA_RECOMMEND` configured and verify the `recommend` query param is not added (or whatever the default behavior should be).
- Optionally, set `TRUSTIFY_DA_RECOMMEND='true'` via opts and/or env and verify `recommend` is still not added.
This will lock in the expectation that only `false` produces a `recommend` query param, preserving previous behavior.
Suggested implementation:
```javascript
/** Verifies that no providers or sources params are added when neither is set. */
test('does not append providers or sources when neither is set', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('providers')).to.equal(false)
expect(url.searchParams.has('sources')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is not configured. */
test('does not append recommend when not configured', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('recommend')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is set to true via opts. */
test('does not append recommend when TRUSTIFY_DA_RECOMMEND is true in opts', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, { TRUSTIFY_DA_RECOMMEND: 'true' })
expect(url.searchParams.has('recommend')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is set to true via env. */
test('does not append recommend when TRUSTIFY_DA_RECOMMEND is true in env', () => {
const url = new URL('http://example.com/api/v5/analysis')
const originalEnv = process.env.TRUSTIFY_DA_RECOMMEND
process.env.TRUSTIFY_DA_RECOMMEND = 'true'
try {
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('recommend')).to.equal(false)
} finally {
process.env.TRUSTIFY_DA_RECOMMEND = originalEnv
}
})
```
These tests assume:
1. `appendAnalysisQueryParams` currently only appends the `recommend` query param when it resolves to `false`, and never when it is unset or `true`.
2. The rest of the file already imports or defines `analysis`, `expect`, and the test runner functions (e.g., `suite`, `test`), and uses Node's global `process` object.
If the helper’s behavior differs (e.g., it appends `recommend=true`), you should adjust the `expect(url.searchParams.has('recommend')).to.equal(false)` assertions to reflect the actual desired behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+247
to
+259
| function appendAnalysisQueryParams(url, opts = {}) { | ||
| if (getCustom('TRUSTIFY_DA_RECOMMEND', 'true', opts) === 'false') { | ||
| url.searchParams.append('recommend', 'false'); | ||
| } | ||
| const providers = getCustom('TRUSTIFY_DA_PROVIDERS', null, opts); | ||
| if (providers) { | ||
| url.searchParams.append('providers', providers); | ||
| } | ||
| const sources = getCustom('TRUSTIFY_DA_SOURCES', null, opts); | ||
| if (sources) { | ||
| url.searchParams.append('sources', sources); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Use set instead of append for query params to avoid accidental duplicates
If this helper is called multiple times with the same URL (or the URL is reused), searchParams.append on recommend, providers, and sources will create duplicate query params. Using searchParams.set keeps the function idempotent and prevents subtle issues as call sites change.
Suggested change
| function appendAnalysisQueryParams(url, opts = {}) { | |
| if (getCustom('TRUSTIFY_DA_RECOMMEND', 'true', opts) === 'false') { | |
| url.searchParams.append('recommend', 'false'); | |
| } | |
| const providers = getCustom('TRUSTIFY_DA_PROVIDERS', null, opts); | |
| if (providers) { | |
| url.searchParams.append('providers', providers); | |
| } | |
| const sources = getCustom('TRUSTIFY_DA_SOURCES', null, opts); | |
| if (sources) { | |
| url.searchParams.append('sources', sources); | |
| } | |
| } | |
| function appendAnalysisQueryParams(url, opts = {}) { | |
| if (getCustom('TRUSTIFY_DA_RECOMMEND', 'true', opts) === 'false') { | |
| url.searchParams.set('recommend', 'false'); | |
| } | |
| const providers = getCustom('TRUSTIFY_DA_PROVIDERS', null, opts); | |
| if (providers) { | |
| url.searchParams.set('providers', providers); | |
| } | |
| const sources = getCustom('TRUSTIFY_DA_SOURCES', null, opts); | |
| if (sources) { | |
| url.searchParams.set('sources', sources); | |
| } | |
| } |
Comment on lines
+236
to
+241
| /** Verifies that no providers or sources params are added when neither is set. */ | ||
| test('does not append providers or sources when neither is set', () => { | ||
| const url = new URL('http://example.com/api/v5/analysis') | ||
| analysis.appendAnalysisQueryParams(url, {}) | ||
| expect(url.searchParams.has('providers')).to.equal(false) | ||
| expect(url.searchParams.has('sources')).to.equal(false) |
There was a problem hiding this comment.
suggestion (testing): Cover behavior of recommend param when true/unspecified to avoid regressions
appendAnalysisQueryParams is only tested for the recommend=false case. Since this helper now owns the recommend param, please add tests that:
- Call it with no
TRUSTIFY_DA_RECOMMENDconfigured and verify therecommendquery param is not added (or whatever the default behavior should be). - Optionally, set
TRUSTIFY_DA_RECOMMEND='true'via opts and/or env and verifyrecommendis still not added.
This will lock in the expectation that onlyfalseproduces arecommendquery param, preserving previous behavior.
Suggested implementation:
/** Verifies that no providers or sources params are added when neither is set. */
test('does not append providers or sources when neither is set', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('providers')).to.equal(false)
expect(url.searchParams.has('sources')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is not configured. */
test('does not append recommend when not configured', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('recommend')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is set to true via opts. */
test('does not append recommend when TRUSTIFY_DA_RECOMMEND is true in opts', () => {
const url = new URL('http://example.com/api/v5/analysis')
analysis.appendAnalysisQueryParams(url, { TRUSTIFY_DA_RECOMMEND: 'true' })
expect(url.searchParams.has('recommend')).to.equal(false)
})
/** Verifies that recommend param is not appended when TRUSTIFY_DA_RECOMMEND is set to true via env. */
test('does not append recommend when TRUSTIFY_DA_RECOMMEND is true in env', () => {
const url = new URL('http://example.com/api/v5/analysis')
const originalEnv = process.env.TRUSTIFY_DA_RECOMMEND
process.env.TRUSTIFY_DA_RECOMMEND = 'true'
try {
analysis.appendAnalysisQueryParams(url, {})
expect(url.searchParams.has('recommend')).to.equal(false)
} finally {
process.env.TRUSTIFY_DA_RECOMMEND = originalEnv
}
})These tests assume:
appendAnalysisQueryParamscurrently only appends therecommendquery param when it resolves tofalse, and never when it is unset ortrue.- The rest of the file already imports or defines
analysis,expect, and the test runner functions (e.g.,suite,test), and uses Node's globalprocessobject.
If the helper’s behavior differs (e.g., it appends recommend=true), you should adjust the expect(url.searchParams.has('recommend')).to.equal(false) assertions to reflect the actual desired behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
=======================================
Coverage 91.01% 91.01%
=======================================
Files 38 38
Lines 8001 8001
Branches 1395 1395
=======================================
Hits 7282 7282
Misses 719 719
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--providersand--sourcesCLI flags tostack,component, andstack-batchcommandsTRUSTIFY_DA_PROVIDERSandTRUSTIFY_DA_SOURCESenv var fallbacksappendAnalysisQueryParams()helper inanalysis.jsImplements TC-5409
Test plan
npm run lintpassesnpm testpassesSummary by Sourcery
Add support for configuring analysis providers and sources across CLI commands and backend requests.
New Features:
Enhancements:
Tests: