fix(providers): analyze all FROM lines in multi-stage Dockerfiles via batch analysis#577
Open
a-oren wants to merge 1 commit into
Open
fix(providers): analyze all FROM lines in multi-stage Dockerfiles via batch analysis#577a-oren wants to merge 1 commit into
a-oren wants to merge 1 commit into
Conversation
… batch analysis Change the Dockerfile/Containerfile provider to parse every FROM instruction instead of only the last one. Each image gets its own SBOM in a purl-keyed map that is routed to the /api/v5/batch-analysis endpoint via requestStackBatch(). - Rename parseFromImage() to parseAllFromImages() returning an array - Build sbomByPurl map in getImageSBOM() following requestImages() pattern - Add batch flag to provider result for routing in analysis.js - Resolve ARG substitutions using declared defaults (matching VS Code extension) - FROM lines with unresolvable ARGs (no default) are skipped without failing Implements TC-5070 Assisted-by: Claude Code
Reviewer's GuideUpdates the Dockerfile/Containerfile OCI provider to parse and analyze all FROM stages in multi-stage Dockerfiles, resolve ARG-based image references using defaults, and emit batch SBOM results that are routed through new batch-handling paths in the analysis pipeline. Sequence diagram for batch Dockerfile stack analysissequenceDiagram
actor User
participant CLI as requestStack
participant Provider as oci_dockerfile_provider
participant Batch as requestStackBatch
User->>CLI: invoke requestStack(manifest, url, opts)
CLI->>Provider: provideStack(manifest, opts)
Provider-->>CLI: { content, contentType, ecosystem, batch:true }
CLI->>Batch: requestStackBatch(JSON.parse(content), url, html, opts)
Batch-->>User: batch analysis result
Flow diagram for parsing all FROM images with ARG resolutionflowchart TD
A[parseAllFromImages]
A-->B[getParser]
A-->C[getFromQuery]
A-->D[getArgQuery]
B & C & D -->E[parser.parse]
E-->F[collectArgs]
F-->G[iterate FROM matches]
G-->H{containsExpansion}
H-->|no|I[add imageSpec.text to images]
H-->|yes|J[resolveArgs]
J-->K{resolved != null}
K-->|yes|L[add resolved to images]
K-->|no|G
I-->M{images.length > 0}
L-->M
M-->|yes|N[return images]
M-->|no|O[throw error]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
a-oren
requested review from
Strum355 and
ruromero
and removed request for
ruromero
![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
getImageSBOM, the same image could appear in multiple FROM stages and will trigger multiplegenerateImageSBOMcalls even though thesbomByPurlentry is overwritten; consider de‑duplicating by purl or image ref to avoid redundant SBOM generation work. - When
parseAllFromImagesends up skipping all FROM lines due to unresolved ARGs, the thrown error is quite generic; including the list of unresolved variables or at least the problematic FROM lines would make it easier to diagnose why the Dockerfile could not be resolved.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getImageSBOM`, the same image could appear in multiple FROM stages and will trigger multiple `generateImageSBOM` calls even though the `sbomByPurl` entry is overwritten; consider de‑duplicating by purl or image ref to avoid redundant SBOM generation work.
- When `parseAllFromImages` ends up skipping all FROM lines due to unresolved ARGs, the thrown error is quite generic; including the list of unresolved variables or at least the problematic FROM lines would make it easier to diagnose why the Dockerfile could not be resolved.
## Individual Comments
### Comment 1
<location path="src/providers/containerfile_parser.js" line_range="25" />
<code_context>
+
+export async function getArgQuery() {
+ const language = await init();
+ return new Query(language, '(arg_instruction (arg_pair name: (unquoted_string) @name default: (unquoted_string) @default))');
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ARG query only captures unquoted defaults, missing valid quoted ARG defaults.
By restricting both `name` and `default` to `unquoted_string`, this misses valid cases like `ARG FOO="bar"` (or other representations tree-sitter may use). Those ARGs will be invisible to `collectArgs`, causing FROM lines that depend on them to be treated as unresolvable and skipped. Please broaden the matched node types for `default` (and possibly `name`) so quoted and alternative forms are included.
Suggested implementation:
```javascript
export async function getArgQuery() {
const language = await init();
return new Query(
language,
'(arg_instruction (arg_pair name: (_) @name default: (_) @default))',
);
}
```
If downstream code assumes `@name` and `@default` are always `unquoted_string` nodes, you may need to adjust it to handle other node types (e.g., string literals) by inspecting the captured nodes and normalizing their text (stripping quotes where needed).
</issue_to_address>
### Comment 2
<location path="test/providers/oci_dockerfile.test.js" line_range="65-66" />
<code_context>
+ expect(result).to.deep.equal(['node:18', 'nginx:alpine'])
+ })
+
+ /** Verifies that FROM lines with ARG substitution are resolved using declared defaults. */
+ test('resolves ARG substitution in FROM lines using declared defaults', async () => {
+ const content = [
+ 'ARG BASE_IMAGE=ubuntu:22.04',
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case covering `$VAR` (non-braced) substitutions in FROM to exercise both resolveArgs patterns
Currently only the `${VAR}` path in `resolveArgs` is covered by tests. To also exercise the `$VAR` branch of the regex, please add a similar test using `$BASE_IMAGE` in a FROM line, e.g.:
```js
test('resolves $VAR substitution in FROM lines using declared defaults', async () => {
const content = [
'ARG BASE_IMAGE=ubuntu:22.04',
'FROM $BASE_IMAGE AS base',
'FROM alpine:3.18',
].join('\n')
const result = await parseAllFromImages(content)
expect(result).to.deep.equal(['ubuntu:22.04', 'alpine:3.18'])
})
```
This ensures the `$VAR` pattern remains covered and prevents regressions on that path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| export async function getArgQuery() { | ||
| const language = await init(); | ||
| return new Query(language, '(arg_instruction (arg_pair name: (unquoted_string) @name default: (unquoted_string) @default))'); |
There was a problem hiding this comment.
suggestion (bug_risk): ARG query only captures unquoted defaults, missing valid quoted ARG defaults.
By restricting both name and default to unquoted_string, this misses valid cases like ARG FOO="bar" (or other representations tree-sitter may use). Those ARGs will be invisible to collectArgs, causing FROM lines that depend on them to be treated as unresolvable and skipped. Please broaden the matched node types for default (and possibly name) so quoted and alternative forms are included.
Suggested implementation:
export async function getArgQuery() {
const language = await init();
return new Query(
language,
'(arg_instruction (arg_pair name: (_) @name default: (_) @default))',
);
}If downstream code assumes @name and @default are always unquoted_string nodes, you may need to adjust it to handle other node types (e.g., string literals) by inspecting the captured nodes and normalizing their text (stripping quotes where needed).
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — the current query correctly captures only ARGs with unquoted defaults, which is sufficient for static resolution. Broadening to wildcard node types would also require downstream normalization (quote stripping). No sub-task created.
Member
There was a problem hiding this comment.
Although Ive never seen quoted ones in the wild (I was thinking of something else), we should ideally handle them. Removing the quotes will be necessary
There was a problem hiding this comment.
Agreed, handling them would make this more robust, and the extra normalization is pretty contained.
Concretely, I’d suggest two small tweaks:
- Broaden the query to accept any node type for name/default:
export async function getArgQuery() {
const language = await init();
return new Query(
language,
'(arg_instruction (arg_pair name: (_) @name default: (_) @default))',
);
}- Normalize in
collectArgsso we strip surrounding quotes from defaults (and optionally names, though they’re usually bare identifiers):
function stripQuotes(text) {
if (!text) return text;
// Handle both single and double quoted strings
if ((text.startsWith('"') && text.endsWith('"')) || (text.startsWith('\'') && text.endsWith('\''))) {
return text.slice(1, -1);
}
return text;
}
function collectArgs(tree, argQuery) {
const args = new Map();
for (const match of argQuery.matches(tree.rootNode)) {
const nameNode = match.captures.find(c => c.name === 'name')?.node;
const defaultNode = match.captures.find(c => c.name === 'default')?.node;
if (!nameNode || !defaultNode) continue;
const name = nameNode.text; // typically unquoted identifier
const defaultValue = stripQuotes(defaultNode.text);
if (name && defaultValue) {
args.set(name, defaultValue);
}
}
return args;
}That keeps the rest of the pipeline unchanged while covering ARG FOO="bar", ARG FOO='bar', and the existing unquoted cases.
If you’d like, I can also suggest a follow-up test case for a Dockerfile with quoted ARG defaults to ensure this path is exercised.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
+ Coverage 90.75% 90.99% +0.24%
==========================================
Files 36 38 +2
Lines 7766 7986 +220
Branches 1353 1391 +38
==========================================
+ Hits 7048 7267 +219
- Misses 718 719 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — Classified as suggestion (2 items):
|
Contributor
Author
Verification Report for TC-5070 (commit df1dfe6)
Overall: WARNScope Containment details:
Repetitive Test Detection details: Several This comment was AI-generated by sdlc-workflow/verify-pr v0.12.2. |
Strum355
reviewed
Jul 10, 2026
| const sbomByPurl = {} | ||
| for (const image of images) { | ||
| const imageRef = parseImageRef(image, opts) | ||
| sbomByPurl[imageRef.getPackageURL().toString()] = generateImageSBOM(imageRef, opts) |
Member
There was a problem hiding this comment.
If a containerfile lists the same image multiple times, this is rerun unnecessarily
Strum355
approved these changes
Jul 10, 2026
Strum355
left a comment
Strum355
left a comment
Member
There was a problem hiding this comment.
Some small changes but LGTM otherwise
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
3 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
/api/v5/batch-analysisChanges
src/providers/oci_dockerfile.js— renameparseFromImage()→parseAllFromImages(), buildsbomByPurlmap, addbatch: trueflagsrc/providers/containerfile_parser.js— addgetArgQuery()for ARG instruction parsingsrc/analysis.js— detect batch provider results and route torequestStackBatch()test/providers/oci_dockerfile.test.js— update tests for array return, add ARG resolution and batch output testsImplements TC-5070
Test plan
parseAllFromImagesreturns array of all image refsnpm test— 496 passing, 0 new failuresnpm run lint— 0 errors🤖 Generated with Claude Code
Summary by Sourcery
Analyze all FROM instructions in Dockerfiles for OCI image SBOM generation and route batch SBOM results through stack/component analysis.
New Features:
Enhancements:
Tests: