refactor(openclaw): migrate chat-send and device self-approval patch tools to .mts - #6967
Conversation
Convert scripts/patch-openclaw-chat-send.js to a typed ESM .mts entrypoint run under Node native type stripping, adding explicit types across the patch helpers and the file-spec model. Update the shebang, usage string, Docker copy/run paths, staged build context, and focused patch and build-context tests; the RUN step gains --experimental-strip-types since the tool is no longer plain JavaScript. Patch selectors, markers, idempotency, exit codes, and audit output are unchanged. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Convert scripts/patch-openclaw-device-self-approval.ts to an explicit ESM .mts entrypoint: the node:fs/node:path requires become imports and the redundant "use strict" directive is dropped. Update the usage string, Docker copy/run paths, staged build context, dependency-review note, and focused patch, contract, and integrity-pin tests. Every CLI, handler, state, transaction, and rollback selector and marker is byte-for-byte unchanged. Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # docs/security/openclaw-2026.6.10-dependency-review.md
…enclaw-patch-tools-mts
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe OpenClaw chat-send and device self-approval patch tools were migrated to ChangesOpenClaw patch module migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-6967.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/patch-openclaw-chat-send.mts (1)
402-443: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail closed instead of silently skipping an unresolved file.
resolveFile(..., { dryRun: false })already marks optional files asskippedand callsfail()when a required file cannot be resolved. Therefore,if (!file) continueis unreachable today and could hide a future contract regression by reporting success without applying a required patch. Replace it with a fail-closed assertion or remove the redundant fallback.Based on learnings, avoid adding defensive error handling around internal helper logic when no realistic failure mode exists. As per coding guidelines, do not add fallback layers without a current requirement.
🤖 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 `@scripts/patch-openclaw-chat-send.mts` around lines 402 - 443, Remove the redundant `if (!file) continue` fallback in `runApplyMode` after `resolveFile`, or replace it with a fail-closed assertion that calls `fail()` when no file is resolved. Preserve the existing `skipped` handling and ensure unresolved required files cannot be reported as successfully processed.Sources: Coding guidelines, Learnings
🤖 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/sandbox-build-context.test.ts`:
- Around line 111-114: Extend the fixture assertions around writeFixture and the
optimized-context checks to verify superseded staged paths are absent, not only
that the new .mts files exist. Seed the legacy filenames where needed or
explicitly assert both old paths—patch-openclaw-chat-send.js and
patch-openclaw-device-self-approval.ts—do not exist, including the corresponding
cases noted in the comment.
---
Nitpick comments:
In `@scripts/patch-openclaw-chat-send.mts`:
- Around line 402-443: Remove the redundant `if (!file) continue` fallback in
`runApplyMode` after `resolveFile`, or replace it with a fail-closed assertion
that calls `fail()` when no file is resolved. Preserve the existing `skipped`
handling and ensure unresolved required files cannot be reported as successfully
processed.
🪄 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: 508243bc-8014-4fa4-aeda-e3042cff5983
📒 Files selected for processing (11)
Dockerfiledocs/security/openclaw-2026.6.10-dependency-review.mdscripts/patch-openclaw-chat-send.mtsscripts/patch-openclaw-device-self-approval.mtssrc/lib/sandbox/build-context.tstest/helpers/openclaw-device-self-approval-patch-harness.tstest/openclaw-chat-send-patch.test.tstest/openclaw-dependency-review.test.tstest/openclaw-integrity-pin-suite.tstest/openclaw-real-patched-dist-harness.test.tstest/sandbox-build-context.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the OpenClaw patch-tool migration and merge resolution against current main. The build context preserves all current patch tools, explicitly excludes the legacy staged files, and the focused patch/dependency/build-context tests pass.
3 participants
Summary
Migrate the two OpenClaw compatibility patch tools from CommonJS to typed ESM
.mtsentrypoints that Node executes under native type stripping withouttsx, following thepatch-openclaw-mcp-npx.mtsprecedent. Module-boundary migration only — no patch shape, selector, marker, idempotency, exit-code, or audit-output change. The two tools share the Dockerfile patch block, the staged build context, the dependency-review note, and several test harnesses, so they move together in one change.Related Issue
Resolves #6928
Resolves #6930
Part of #6918
Changes
Two commits, one per tool.
patch-openclaw-chat-send(#6928) — was plain CommonJS JavaScript:scripts/patch-openclaw-chat-send.js→.mts:require→node:imports, shebang →#!/usr/bin/env -S node --experimental-strip-types, usage string, and explicit types across the patch helpers and the file-spec model (the file was untyped, so a typed.mtsneeds annotations to passtsconfig.cli.json).Dockerfile: COPY/chmod paths, and the RUN step gains--experimental-strip-typessince the tool is no longer plain JavaScript.patch-openclaw-device-self-approval(#6930) — was CommonJS-style TypeScript:scripts/patch-openclaw-device-self-approval.ts→.mts:require→imports and the redundant"use strict"directive dropped (ESM is strict). The Dockerfile RUN already passed--experimental-strip-types, so only the path extension changes.Shared surface (both):
src/lib/sandbox/build-context.ts: staged build-context copy paths.docs/security/openclaw-2026.6.10-dependency-review.md: script path references in the reviewed source boundary and the Patch 8 row.openclaw-chat-send-patch,openclaw-device-self-approval-patch,openclaw-dependency-review(exact Dockerfile COPY/RUN contract),openclaw-integrity-pin-suite,openclaw-real-patched-dist-harness,sandbox-build-context, and the device patch harness.Type of Change
Quality Gates
--auditbehaviour, selectors, markers, exit codes, upgrade contract, and the exact Dockerfile COPY/RUN lines are already pinned byopenclaw-chat-send-patch,openclaw-device-self-approval-patch,openclaw-dependency-review, the integrity-pin suites, andsandbox-build-context; the migration repoints paths and module format without changing the contract, and those suites stay green.docs/change is the internal security dependency-review note's script-path references; no user-facing page or behaviour changes.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 runon the chat-send, device, dependency-review, sandbox-build-context, and integrity-pin suites → 101 passed;npm run typecheck:cli→ pass (merged base, Node 22.22).npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
.mtsTypeScript ESM modules for chat-send and device self-approval, improving compatibility across Docker and sandbox runtime patching.Documentation
.mtspatch script filenames.Tests
.mtsmodule format.