refactor(onboard): extract dockerfile patch helpers - #3300
Merged
Conversation
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extracts Dockerfile ARG patching, proxy validators, and JSON-encoding helpers from ChangesDockerfile Patch Module Extraction
Sequence DiagramsequenceDiagram
participant Test
participant patchStagedDockerfile
participant Filesystem
Test->>Filesystem: Create temp Dockerfile with baseline ARGs
Test->>Test: Set NEMOCLAW_PROXY_HOST/PORT env vars
Test->>patchStagedDockerfile: Call with provider/model/chat/inference/messaging/compat args
patchStagedDockerfile->>Filesystem: Read Dockerfile
patchStagedDockerfile->>patchStagedDockerfile: Compute sandbox inference config & validate proxy/ports
patchStagedDockerfile->>patchStagedDockerfile: Rewrite ARGs (model/provider/inference/build id/overrides) and sanitize CR/LF
patchStagedDockerfile->>patchStagedDockerfile: Base64-encode messaging/telegram/discord configs as needed
patchStagedDockerfile->>Filesystem: Write patched Dockerfile
Test->>Filesystem: Read and assert ARG values updated / certain ARGs removed
Test->>Filesystem: Cleanup temp directory and clear env vars
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cv
marked this pull request as draft
May 9, 2026 01:23
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…ture-layout # Conflicts: # src/lib/actions/sandbox/destroy.ts # src/lib/inference/health.ts
…d-selection-drift
…d-selection-drift
…ture-layout # Conflicts: # src/lib/list-command-deps.ts
…d-selection-drift
…d-selection-drift
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryPi Semantic E2E AdvisorFailed: pi exited with status 1; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-pi-raw-output.txt |
cv
marked this pull request as ready for review
May 12, 2026 23:09
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/onboard/dockerfile-patch.ts (2)
79-249: 🏗️ Heavy lift
patchStagedDockerfileis too complex; split patch rules into small helpers.The function has many repeated replace blocks and branching paths, which makes behavior harder to audit and extend safely.
Refactor direction (pattern)
+type ArgPatch = { + key: string; + value: string | null; +}; + +function applyArgPatch(dockerfile: string, { key, value }: ArgPatch): string { + if (value === null) return dockerfile; + return dockerfile.replace(new RegExp(`^ARG ${key}=.*$`, "m"), `ARG ${key}=${value}`); +}- dockerfile = dockerfile.replace(/^ARG NEMOCLAW_MODEL=.*$/m, `ARG NEMOCLAW_MODEL=${model}`); - dockerfile = dockerfile.replace( - /^ARG NEMOCLAW_PROVIDER_KEY=.*$/m, - `ARG NEMOCLAW_PROVIDER_KEY=${providerKey}`, - ); +dockerfile = applyArgPatch(dockerfile, { key: "NEMOCLAW_MODEL", value: model }); +dockerfile = applyArgPatch(dockerfile, { key: "NEMOCLAW_PROVIDER_KEY", value: providerKey });As per coding guidelines, "
**/*.{js,ts,tsx}: Keep function complexity low; prefix unused variables with underscore (_)".🤖 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 `@src/lib/onboard/dockerfile-patch.ts` around lines 79 - 249, patchStagedDockerfile is too large and duplicates many dockerfile.replace patterns; extract small helpers and move branching logic into them to reduce complexity. Create helpers such as applyArgReplacement(dockerfile, argName, value), applyEnvValidatedReplacement(dockerfile, envName, argName, validator), applyJsonArgReplacement(dockerfile, argName, obj, encoder = encodeDockerJsonArg) and pinBaseImageIfSandbox(dockerfile, baseImageRef, SANDBOX_BASE_IMAGE) and use those from patchStagedDockerfile (keeping calls to getDockerfileSandboxInferenceConfig, POSITIVE_INT_RE, isValidProxyHost, isValidProxyPort unchanged); replace each repeated regex/replace block with the appropriate helper call so validation and encoding live in one place and the main function reads as a sequence of small steps.
65-67: ⚡ Quick winUse nullish coalescing operator for JSON arg default.
value || {}applies boolean coercion; usevalue ?? {}to more precisely handle onlynullandundefined. While the test confirms this behavior is intentional (line 40 in dockerfile-patch.test.ts expectsencodeDockerJsonArg(null)to yield"{}"), the semantic intent is clearer with the nullish coalescing operator and guards against accidental falsy-value corruption.Suggested fix
export function encodeDockerJsonArg(value: unknown): string { - return Buffer.from(JSON.stringify(value || {}), "utf8").toString("base64"); + return Buffer.from(JSON.stringify(value ?? {}), "utf8").toString("base64"); }🤖 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 `@src/lib/onboard/dockerfile-patch.ts` around lines 65 - 67, The encodeDockerJsonArg function currently uses boolean coercion (value || {}) which incorrectly treats other falsy values like 0, "" or false as null; update the function to use the nullish coalescing operator (value ?? {}) so only null/undefined fall back to an empty object, i.e., change the defaulting logic inside encodeDockerJsonArg to use ?? and keep the rest of the JSON.stringify/base64 behavior unchanged.
🤖 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 `@src/lib/onboard/dockerfile-patch.ts`:
- Around line 79-249: patchStagedDockerfile is too large and duplicates many
dockerfile.replace patterns; extract small helpers and move branching logic into
them to reduce complexity. Create helpers such as
applyArgReplacement(dockerfile, argName, value),
applyEnvValidatedReplacement(dockerfile, envName, argName, validator),
applyJsonArgReplacement(dockerfile, argName, obj, encoder = encodeDockerJsonArg)
and pinBaseImageIfSandbox(dockerfile, baseImageRef, SANDBOX_BASE_IMAGE) and use
those from patchStagedDockerfile (keeping calls to
getDockerfileSandboxInferenceConfig, POSITIVE_INT_RE, isValidProxyHost,
isValidProxyPort unchanged); replace each repeated regex/replace block with the
appropriate helper call so validation and encoding live in one place and the
main function reads as a sequence of small steps.
- Around line 65-67: The encodeDockerJsonArg function currently uses boolean
coercion (value || {}) which incorrectly treats other falsy values like 0, "" or
false as null; update the function to use the nullish coalescing operator (value
?? {}) so only null/undefined fall back to an empty object, i.e., change the
defaulting logic inside encodeDockerJsonArg to use ?? and keep the rest of the
JSON.stringify/base64 behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8b3af31-db82-4619-ba3a-ea635ef8987b
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard/dockerfile-patch.ts (1)
79-254: 🏗️ Heavy lift
patchStagedDockerfilehas grown beyond the project’s complexity guideline.This function is now heavily branchy and repetitive. Consider extracting table-driven ARG rewrites plus small feature-specific patch helpers (base image, inference, env overrides, messaging) to reduce regression risk and simplify tests.
As per coding guidelines,
**/*.{js,ts,tsx}: Keep function complexity low.🤖 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 `@src/lib/onboard/dockerfile-patch.ts` around lines 79 - 254, patchStagedDockerfile is too large and repetitive; refactor it by extracting smaller, testable helpers and a table-driven ARG-rewrite mechanism. Create helpers like patchBaseImageArg(dockerfile, baseImageRef, SANDBOX_BASE_IMAGE), applyArgRewrite(dockerfile, argName, value), applyEncodedArg(dockerfile, argName, value, encoder=encodeDockerJsonArg), and feature-specific small functions for inference config, env overrides (use POSITIVE_INT_RE, isValidProxyHost, isValidProxyPort), and messaging/telegram patches; then have patchStagedDockerfile call getDockerfileSandboxInferenceConfig once and sequentially apply those helpers to produce the final dockerfile before fs.writeFileSync. Ensure all existing symbols (getDockerfileSandboxInferenceConfig, encodeDockerJsonArg, POSITIVE_INT_RE, isValidProxyHost, isValidProxyPort, SANDBOX_BASE_IMAGE) are used so behavior is unchanged and unit tests can target the small helpers.
🤖 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 `@src/lib/onboard/dockerfile-patch.ts`:
- Around line 79-146: The patchStagedDockerfile function interpolates untrusted
values into Dockerfile ARG lines which can allow line-injection via newlines;
create a small sanitizer (e.g., sanitizeDockerArg) that strips or encodes
newline and carriage-return characters (at minimum remove \n and \r) and use it
for all non-constant interpolations in patchStagedDockerfile — specifically
apply it to model, chatUiUrl, buildId, providerKey, primaryModelRef,
inferenceBaseUrl, inferenceApi, darwinVmCompat (converted to sanitized "1"/"0"),
baseImageRef handling, telegramConfig/messagingAllowedIds/discordGuilds values,
and any other places later in the function (lines ~149-217) where inputs/env
values are injected into ARG or Dockerfile lines so that every replacement call
uses the sanitized value instead of the raw input.
---
Nitpick comments:
In `@src/lib/onboard/dockerfile-patch.ts`:
- Around line 79-254: patchStagedDockerfile is too large and repetitive;
refactor it by extracting smaller, testable helpers and a table-driven
ARG-rewrite mechanism. Create helpers like patchBaseImageArg(dockerfile,
baseImageRef, SANDBOX_BASE_IMAGE), applyArgRewrite(dockerfile, argName, value),
applyEncodedArg(dockerfile, argName, value, encoder=encodeDockerJsonArg), and
feature-specific small functions for inference config, env overrides (use
POSITIVE_INT_RE, isValidProxyHost, isValidProxyPort), and messaging/telegram
patches; then have patchStagedDockerfile call
getDockerfileSandboxInferenceConfig once and sequentially apply those helpers to
produce the final dockerfile before fs.writeFileSync. Ensure all existing
symbols (getDockerfileSandboxInferenceConfig, encodeDockerJsonArg,
POSITIVE_INT_RE, isValidProxyHost, isValidProxyPort, SANDBOX_BASE_IMAGE) are
used so behavior is unchanged and unit tests can target the small helpers.
🪄 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: be517081-ebba-415d-a7dc-340bfc39b418
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/dockerfile-patch.test.ts
- src/lib/onboard.ts
This was referenced May 13, 2026
wscurran
added
area: packaging
21 tasks
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
4 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
Extract Dockerfile patching helpers out of the large onboarding module. This continues the onboarding cleanup stack by moving sandbox image ARG rewriting, proxy validation, and Docker JSON ARG encoding into a focused helper module.
Changes
src/lib/onboard/dockerfile-patch.tsfor Dockerfile ARG rewriting, proxy host/port validation, and base64 JSON ARG encoding.src/lib/onboard.tsto import Dockerfile patch helpers while preserving the existingpatchStagedDockerfileexport and call sites.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Refactor
New Features
Tests