refactor(scripts): migrate OpenClaw config generator to TypeScript#4571
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Addressed the PR Review Advisor items in
Focused verification now passes at 151 tests: Also passed the standalone strict TypeScript check, Biome lint for the touched TS/test files, and |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Added a small direct-import unit-test path in Focused generator coverage command: Coverage for
Validation after the split:
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Expanded the direct unit coverage in Updated focused coverage command: Coverage for
Validation after the expansion:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/security-c2-dockerfile-injection.test.ts (2)
115-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a JavaScript-shaped payload in this Node regression.
After the migration to
node -e, this case still uses a Python payload/comment, so it can stay green without exercising the JavaScript injection surface the file is meant to cover.♻️ Suggested update
- it("semicolons and import statements in URL are literal data", () => { - const dangerous = "http://x; import subprocess; subprocess.run(['id'])"; + it("semicolons and require calls in URL are literal data", () => { + const dangerous = + "http://x; require('node:child_process').execSync('id')"; const result = runNode(fixedSource(), { CHAT_UI_URL: dangerous }); - // The URL is treated as data — urlparse may or may not raise, but - // the key property is that no code injection occurs. Check stdout or stderr - // does NOT contain evidence of os.system/subprocess execution. + // The URL is treated as data. + // The key property is that no injected JavaScript executes. const combined = result.stdout + result.stderr; expect(!combined.includes("uid=")).toBeTruthy(); });🤖 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/security-c2-dockerfile-injection.test.ts` around lines 115 - 123, The test "semicolons and import statements in URL are literal data" currently uses a Python payload string in dangerous and thus doesn't exercise the Node/JavaScript injection surface; update the dangerous value passed into runNode(fixedSource(), { CHAT_UI_URL: dangerous }) to a JavaScript-shaped payload (e.g., something that would execute when interpreted by node -e such as using ; require('child_process').execSync('id') or similar) so the test asserts no JS code execution; keep the test flow (result, combined = result.stdout + result.stderr, expect(!combined.includes("uid=")).toBeTruthy()) and only change the CHAT_UI_URL payload.
130-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail when the generator
RUNlayer is not found.If the regex stops matching because the Dockerfile command is reformatted, the fallback
expect(promoted).toBeTruthy()still passes as long as theENVappears somewhere. That weakens this from an ordering assertion to an existence check.♻️ Suggested update
it("NEMOCLAW_DISABLE_DEVICE_AUTH is promoted to ENV before the config generator RUN layer", () => { const src = fs.readFileSync(DOCKERFILE, "utf-8"); const lines = src.split("\n"); let promoted = false; let inEnvBlock = false; + let sawGeneratorRun = false; for (let i = 0; i < lines.length; i++) { const line = lines[i]; if (/^\s*FROM\b/.test(line)) { promoted = false; inEnvBlock = false; @@ if ( /^\s*RUN\b.*node\s+--experimental-strip-types\s+\/usr\/local\/lib\/nemoclaw\/generate-openclaw-config\.ts\b/.test( line, ) ) { + sawGeneratorRun = true; expect(promoted).toBeTruthy(); return; } } - expect(promoted).toBeTruthy(); + expect(sawGeneratorRun).toBeTruthy(); });🤖 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/security-c2-dockerfile-injection.test.ts` around lines 130 - 159, The test currently falls back to expect(promoted).toBeTruthy() even if the generator RUN line (the regex /^\s*RUN\b.*node\s+--experimental-strip-types\s+\/usr\/local\/lib\/nemoclaw\/generate-openclaw-config\.ts\b/) was never matched, turning an ordering assertion into a mere existence check; add a boolean flag (e.g., foundGeneratorRun) that is set true when that RUN regex matches, assert foundGeneratorRun is true (fail if the RUN line isn’t found), and only then assert promoted is true (or assert promoted && foundGeneratorRun) so the test fails if the generator RUN layer cannot be located while still checking ENV promotion using the existing promoted/inEnvBlock logic.
🤖 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 `@Dockerfile`:
- Around line 397-403: The Docker build breaks because
/usr/local/lib/nemoclaw/generate-openclaw-config.ts is ESM-only but is executed
with node --experimental-strip-types without a nearby package.json declaring
"type":"module"; fix by either: (A) add/copy a minimal package.json containing
{"type":"module"} into /usr/local/lib/nemoclaw/ during the Dockerfile steps so
Node treats generate-openclaw-config.ts as ESM at build time (ensure the Docker
COPY places this package.json next to generate-openclaw-config.ts before the RUN
invocation), or (B) rename scripts/generate-openclaw-config.ts to
generate-openclaw-config.mts and update the Dockerfile COPY, RUN (node
--experimental-strip-types /usr/local/lib/nemoclaw/generate-openclaw-config.mts)
and chmod entries to reference the .mts filename so Node treats it as ESM
automatically.
---
Outside diff comments:
In `@test/security-c2-dockerfile-injection.test.ts`:
- Around line 115-123: The test "semicolons and import statements in URL are
literal data" currently uses a Python payload string in dangerous and thus
doesn't exercise the Node/JavaScript injection surface; update the dangerous
value passed into runNode(fixedSource(), { CHAT_UI_URL: dangerous }) to a
JavaScript-shaped payload (e.g., something that would execute when interpreted
by node -e such as using ; require('child_process').execSync('id') or similar)
so the test asserts no JS code execution; keep the test flow (result, combined =
result.stdout + result.stderr, expect(!combined.includes("uid=")).toBeTruthy())
and only change the CHAT_UI_URL payload.
- Around line 130-159: The test currently falls back to
expect(promoted).toBeTruthy() even if the generator RUN line (the regex
/^\s*RUN\b.*node\s+--experimental-strip-types\s+\/usr\/local\/lib\/nemoclaw\/generate-openclaw-config\.ts\b/)
was never matched, turning an ordering assertion into a mere existence check;
add a boolean flag (e.g., foundGeneratorRun) that is set true when that RUN
regex matches, assert foundGeneratorRun is true (fail if the RUN line isn’t
found), and only then assert promoted is true (or assert promoted &&
foundGeneratorRun) so the test fails if the generator RUN layer cannot be
located while still checking ENV promotion using the existing
promoted/inEnvBlock logic.
🪄 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: 86b08610-8d9a-47b1-b5a6-a6ca6db152e8
📒 Files selected for processing (10)
Dockerfilescripts/generate-openclaw-config.pyscripts/generate-openclaw-config.tsscripts/seed-wechat-accounts.pysrc/lib/sandbox/build-context.tstest/generate-openclaw-config.test.tstest/sandbox-build-context.test.tstest/sandbox-provisioning.test.tstest/security-c2-dockerfile-injection.test.tstest/seed-wechat-accounts.test.ts
💤 Files with no reviewable changes (1)
- scripts/generate-openclaw-config.py
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/generate-openclaw-config.test.ts (1)
55-81: 💤 Low valueOptional: deduplicate env construction between
withConfigEnvandrunConfigScriptRaw.Both build the same
{ PATH, ...BASE_ENV, ...envOverrides, HOME }shape (Lines 40-45 and Lines 57-62). Extracting a smallbuildTestEnv(envOverrides)helper keeps the subprocess and in-process paths from drifting.🤖 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/generate-openclaw-config.test.ts` around lines 55 - 81, Extract the duplicated environment construction into a shared helper (e.g., buildTestEnv) that accepts envOverrides and returns the merged env object ({ PATH, ...BASE_ENV, ...envOverrides, HOME: tmpDir }); update withConfigEnv to call buildTestEnv(envOverrides) instead of reconstructing the object inline, and update runConfigScript (and any runConfigScriptRaw variant) to use buildTestEnv when preparing the env for subprocess/in-process runs so both paths use the same source of truth.
🤖 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/generate-openclaw-config.test.ts`:
- Around line 55-81: Extract the duplicated environment construction into a
shared helper (e.g., buildTestEnv) that accepts envOverrides and returns the
merged env object ({ PATH, ...BASE_ENV, ...envOverrides, HOME: tmpDir }); update
withConfigEnv to call buildTestEnv(envOverrides) instead of reconstructing the
object inline, and update runConfigScript (and any runConfigScriptRaw variant)
to use buildTestEnv when preparing the env for subprocess/in-process runs so
both paths use the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 024dc4f0-19db-4dae-a53c-d6fb3e194271
📒 Files selected for processing (9)
Dockerfilescripts/generate-openclaw-config.mtsscripts/seed-wechat-accounts.pysrc/lib/sandbox/build-context.tstest/generate-openclaw-config.test.tstest/sandbox-build-context.test.tstest/sandbox-provisioning.test.tstest/security-c2-dockerfile-injection.test.tstest/seed-wechat-accounts.test.ts
💤 Files with no reviewable changes (1)
- scripts/generate-openclaw-config.mts
✅ Files skipped from review due to trivial changes (2)
- scripts/seed-wechat-accounts.py
- test/seed-wechat-accounts.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/sandbox-build-context.test.ts
- Dockerfile
- src/lib/sandbox/build-context.ts
- test/security-c2-dockerfile-injection.test.ts
…o local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Addressed the CodeRabbit review from pullrequestreview-4396054818 in 3acb992 by extracting shared buildTestEnv() setup for both the subprocess and in-process generator test helpers. Validation:
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26696782468
|
2 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
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
Migrates
scripts/generate-openclaw-config.pyto a TypeScript entrypoint that runs with Node's experimental type stripping, keeping the generator in line with the rest of the project. Updates Docker/build-context wiring, preserves migration parity called out by review feedback, and adds focused regression tests for the new entrypoint.Related Issue
Stacked on #4277.
Changes
scripts/generate-openclaw-config.ts.node --experimental-strip-typesin the sandbox Docker build and staged build context.URLparser forCHAT_UI_URLsecurity decisions, including userinfo/ambiguous-host cases.enabledout of token and WhatsApp channel blocks.openclaw.plugin.jsonWeChat metadata probe and add coverage for that layout.Type of Change
Verification
Targeted checks run:
npx vitest run --project cli test/generate-openclaw-config.test.ts test/seed-wechat-accounts.test.ts test/security-c2-dockerfile-injection.test.ts test/sandbox-provisioning.test.ts test/sandbox-build-context.test.ts- passed, 151 tests.npx tsc --noEmit --target ES2022 --module preserve --moduleResolution bundler --lib ES2022 --types node --strict --allowImportingTsExtensions scripts/generate-openclaw-config.ts- passed.npx @biomejs/biome lint scripts/generate-openclaw-config.ts test/generate-openclaw-config.test.ts- passed.git diff --check- passed.Known broader-check status:
npx prek run --all-filesdid not start because the hook runner hitself-signed certificate in certificate chainwhile fetching a release.npm testcurrently fails on existing/base issues unrelated to this migration, including missing plugin dist for SSRF parity and status/rebuild/deploy failures.npm run typecheck:clicurrently fails in existingsrc/lib/deploy/index.test.tstype errors unrelated to this generator.npx prek run --all-filespassesnpm testpassesTests added or updated for new or changed behavior
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docsbuilds without warnings (doc changes only)Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Refactor
Tests