fix(e2e): parse multiline Docker copy sources - #7685
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDockerfile COPY parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 6906983 in the TypeScript / code-coverage/cliThe overall coverage in commit 6906983 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/live/rebuild-openclaw-old-base-context.ts (1)
23-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting the flag/operand parsing out of
directDockerfileBaseCopySources.
logicalDockerfileInstructionsanddirectDockerfileBaseCopySourcestogether implement a fairly dense hand-rolled parser (continuation joining, escape-directive validation, heredoc detection, leading-flag scanning,--fromskip, operand/source validation) inside two long functions. This was already flagged as high complexity by the diff analysis. Extracting the leading-flag scan (lines 93-108) into a small named helper (e.g.parseLeadingCopyFlags(tokens)) would make each piece independently testable and easier to reason about.As per coding guidelines, "Keep function complexity low; existing complexity hotspots are tracked separately," and this is newly introduced complexity rather than a pre-tracked hotspot.
Also applies to: 77-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/rebuild-openclaw-old-base-context.ts` around lines 23 - 75, Reduce complexity in directDockerfileBaseCopySources by extracting the leading COPY flag/operand parsing into a small named helper such as parseLeadingCopyFlags. Have the helper handle flag scanning and --from detection, while directDockerfileBaseCopySources retains source validation and copying behavior; keep logicalDockerfileInstructions focused on instruction joining and validation.Source: Coding guidelines
test/e2e/support/rebuild-openclaw-old-base-context.test.ts (1)
84-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMalformed-forms test doesn't cover all newly added rejections and is loosely asserted.
Two related gaps in this table-driven test:
- It doesn't include a case for the new escape-directive rejection (
# escape=other than\) or the new unsafe-character path rejection ($*?[]"'), even though the PR objectives explicitly call these out as forms that must be rejected.- All 8 cases share a single
toThrow(/Unsupported direct Dockerfile\.base COPY form|Dangling Dockerfile\.base continuation|Unsupported Dockerfile\.base heredoc instruction/)assertion, so a case could pass for the wrong reason (e.g., an implementation regression that makes case#3throw the heredoc error instead of the "COPY form" error would go unnoticed).Consider pairing each input with its specific expected message/regex, and adding the two missing malformed-input cases.
As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}, tests should "flag ... conditionals that make a test pass without exercising its claim," and this shared OR-regex risks exactly that failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/rebuild-openclaw-old-base-context.test.ts` around lines 84 - 110, Expand the unsupportedForms table in the malformed COPY test to include a non-backslash # escape directive and a COPY path containing unsafe characters ($*?[]"' ). Associate each input with its specific expected error pattern, then assert each case against that pattern instead of using one shared OR-regex, preserving distinct messages for unsupported COPY forms, dangling continuations, and heredoc instructions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/live/rebuild-openclaw-old-base-context.ts`:
- Around line 23-75: Reduce complexity in directDockerfileBaseCopySources by
extracting the leading COPY flag/operand parsing into a small named helper such
as parseLeadingCopyFlags. Have the helper handle flag scanning and --from
detection, while directDockerfileBaseCopySources retains source validation and
copying behavior; keep logicalDockerfileInstructions focused on instruction
joining and validation.
In `@test/e2e/support/rebuild-openclaw-old-base-context.test.ts`:
- Around line 84-110: Expand the unsupportedForms table in the malformed COPY
test to include a non-backslash # escape directive and a COPY path containing
unsafe characters ($*?[]"' ). Associate each input with its specific expected
error pattern, then assert each case against that pattern instead of using one
shared OR-regex, preserving distinct messages for unsupported COPY forms,
dangling continuations, and heredoc instructions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ac50911-b044-42bf-8e19-81149307bf68
📒 Files selected for processing (2)
test/e2e/live/rebuild-openclaw-old-base-context.tstest/e2e/support/rebuild-openclaw-old-base-context.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/support/rebuild-openclaw-old-base-context.test.ts`:
- Around line 182-186: Update the Dockerfile fixture in the rebuild test to make
the COPY instruction span multiple physical lines while retaining an absolute
source operand such as /etc/passwd. Ensure the test exercises rejection after
continuation parsing, rather than validating only a single-line COPY.
🪄 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: Enterprise
Run ID: 94f3b361-2c69-4647-bed3-cde0bf7671e9
📒 Files selected for processing (1)
test/e2e/support/rebuild-openclaw-old-base-context.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: e2e
4 participants
Summary
Rebuild-OpenClaw old-base context discovery now parses backslash-continued Docker
COPYinstructions as logical instructions and stages every source in multi-source forms. Previously, the physical-line parser omitted four required files from the currentDockerfile.base, causing a deterministic build-context failure rather than runner instability.Related Issue
Relates to #7140.
Changes
COPYforms while continuing to exclude--from=<stage>copies.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededtest/e2e/rebuild fixture helper.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project e2e-support test/e2e/support/rebuild-openclaw-old-base-context.test.ts(1 file, 5 tests passed)npm run typecheck,npm run typecheck:cli, project-membership, title-style, source-shape, Biome, and hook checks passed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Dockerfile.basedirectCOPYinstructions, now handling multiline (\-continued) syntax and ignoring comment/blank-line noise.COPYoperands and allowed flags: rejects unsupportedescape=values, heredoc syntax, JSON-arrayCOPY, invalid/malformed forms, unexpected flags, and multi-stageCOPY --fromentries.COPYsource paths by blocking unsafe relative tokens and relative paths starting with--.Tests
COPYfixtures and more malformed/unsafe cases to ensure failures are explicit.