refactor(inference): migrate Bedrock runtime adapter launcher to .mts - #6938
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Bedrock runtime adapter launcher now uses a typed ChangesBedrock launcher migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![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 (2)
src/lib/inference/bedrock-runtime-adapter.test.ts (2)
390-401: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a near-match rejection case.
The test does not reject
bedrock-runtime-adapter-backup.jsor a command argument containing the needle, so it would pass while false-positive process detection remains possible. Add a negative case for a similarly named launcher.As per path instructions, behavioral tests should protect the process-recognition contract, including rejection of near matches.
🤖 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/inference/bedrock-runtime-adapter.test.ts` around lines 390 - 401, Extend the test case around isLocalAdapterProcess to add negative assertions for the similarly named bedrock-runtime-adapter-backup.js launcher and for a command argument that merely contains adapterProcessNeedle. Preserve the existing positive checks for the old and new launcher filenames and the unrelated OpenRouter rejection.Source: Path instructions
386-388: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the launch boundary, not only the private path helper.
This test says “spawns” but only checks
__test.getAdapterScriptPath(). A regression inensureBedrockRuntimeAdapterorspawnDetachedNodeAdaptercould still pass. Assert the actual detached-spawn argument, or rename the test if helper-only coverage is intentional.As per path instructions, tests should verify observable behavior through the public boundary rather than private-shape assertions.
🤖 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/inference/bedrock-runtime-adapter.test.ts` around lines 386 - 388, Update the test around ensureBedrockRuntimeAdapter and spawnDetachedNodeAdapter to exercise the public launch flow and assert the actual detached-spawn argument ends with bedrock-runtime-adapter.mts. If this test remains limited to __test.getAdapterScriptPath(), rename it to describe helper-path coverage rather than claiming that it spawns the launcher.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.
Inline comments:
In `@src/lib/inference/bedrock-runtime-adapter.ts`:
- Around line 319-336: Replace the bare ADAPTER_PROCESS_NEEDLE matching used by
isAdapterProcess and killStaleAdapter with one shared exact launcher-filename
matcher that accepts only bedrock-runtime-adapter.mts or legacy .js basenames
with proper boundaries. Update
src/lib/inference/bedrock-runtime-adapter.test.ts:390-401 to include a similarly
named launcher or argument and assert it is rejected; update
test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts:585-585 to apply the
same matcher to both procfs and ps command paths.
---
Nitpick comments:
In `@src/lib/inference/bedrock-runtime-adapter.test.ts`:
- Around line 390-401: Extend the test case around isLocalAdapterProcess to add
negative assertions for the similarly named bedrock-runtime-adapter-backup.js
launcher and for a command argument that merely contains adapterProcessNeedle.
Preserve the existing positive checks for the old and new launcher filenames and
the unrelated OpenRouter rejection.
- Around line 386-388: Update the test around ensureBedrockRuntimeAdapter and
spawnDetachedNodeAdapter to exercise the public launch flow and assert the
actual detached-spawn argument ends with bedrock-runtime-adapter.mts. If this
test remains limited to __test.getAdapterScriptPath(), rename it to describe
helper-path coverage rather than claiming that it spawns the launcher.
🪄 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: b2043080-d4c5-4f8e-8ced-af0facdd8cde
📒 Files selected for processing (4)
scripts/bedrock-runtime-adapter.mtssrc/lib/inference/bedrock-runtime-adapter.test.tssrc/lib/inference/bedrock-runtime-adapter.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…ocess detection Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts (1)
584-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid copying the production launcher matcher into the E2E helper.
This regex duplicates
src/lib/inference/bedrock-runtime-adapter.ts’s production matcher, so a defect in the matching rule can be reproduced by both code paths and still pass E2E validation. Assert the observed command line against the expected current and legacy launcher basenames, leaving matcher edge cases to focused unit tests.As per path instructions, E2E tests should prioritize behavioral confidence and flag copied production algorithms.
Also applies to: 595-599
🤖 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/bedrock-runtime-compatible-anthropic.test.ts` around lines 584 - 590, Remove the duplicated BEDROCK_ADAPTER_LAUNCHER_PATTERN from isBedrockAdapterProcess and stop copying the production matcher logic. Instead, assert the observed command line against the expected current and legacy launcher basenames directly, preserving detection for both launchers while leaving matcher edge cases to focused unit tests.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/bedrock-runtime-compatible-anthropic.test.ts`:
- Around line 584-590: Remove the duplicated BEDROCK_ADAPTER_LAUNCHER_PATTERN
from isBedrockAdapterProcess and stop copying the production matcher logic.
Instead, assert the observed command line against the expected current and
legacy launcher basenames directly, preserving detection for both launchers
while leaving matcher edge cases to focused unit tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ff2ece8-1b23-47f7-a78d-1de2dc267d88
📒 Files selected for processing (4)
src/lib/inference/bedrock-runtime-adapter.test.tssrc/lib/inference/bedrock-runtime-adapter.tssrc/lib/inference/local-adapter-lifecycle.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/inference/bedrock-runtime-adapter.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
This PR is blocked by the current Bedrock E2E workflow contract. The job does not set to the matrix agent, so the controller authorizes / shards while the child reports ; the evidence run fails before it can validate the migration. Land a small mainline workflow prerequisite that sets the shard from and adds workflow-boundary/controller regression coverage, then rebase and rerun this PR. The existing runtime jobs passed, but the required controller/evidence contract is not satisfied.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Correction with the affected contract: the Bedrock job does not set NEMOCLAW_E2E_SHARD to the matrix agent, so the controller authorizes openclaw and hermes shards while the child reports default. Land a small mainline workflow prerequisite that sets the shard from matrix.agent and adds workflow-boundary and controller regression coverage, then rebase and rerun this PR. The existing runtime jobs passed, but the required controller and evidence contract is not satisfied.
Superseded by the immediately following corrected review with complete path and contract names.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Post-prerequisite reconciliation plan: after #6969 merges, merge current main manually. A blind merge is text-clean but would leave two NEMOCLAW_E2E_SHARD keys in the Bedrock job and duplicate validator blocks. Keep one mapping after NEMOCLAW_AGENT, keep the main validator with the must-pass-matrix.agent diagnostic, and preserve both the launcher process matcher and the live shard assertion. Then run the focused adapter/lifecycle, E2E-workflow, risk-plan, typecheck, and package-contract checks before fresh protected E2E. |
<!-- markdownlint-disable MD041 --> ## Summary Unblocks the v0.0.84 TypeScript migration PRs whose required evidence depends on trusted workflow code from `main`. Coverage actions now select the available TypeScript entrypoint, and the Bedrock matrix reports the agent-specific shard expected by the E2E controller. ## Changes - Make the base-trusted CLI and plugin coverage actions prefer the migrated `.mts` entrypoints and fall back to `.ts` for older branches. This compatibility window is required because #6935 deletes the old entrypoints in the PR checkout while the action implementation still comes from `main`; the action-contract test executes `.mts`-only, `.ts`-only, and missing-entrypoint fixtures across all four consumers. Remove the `.ts` fallback after #6935 lands and no open migration branch depends on the old entrypoints. - Set `NEMOCLAW_E2E_SHARD` from the Bedrock job's `matrix.agent` so #6938 produces distinct OpenClaw and Hermes evidence. Workflow-boundary and controller tests protect the mapping and reject drift. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal CI compatibility and evidence routing only; the documentation-writer review found no user-facing command, default, configuration, API, policy, or supported workflow change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: maintainer and independent review found no secret, permission, dependency, or untrusted-input expansion; fixed paths are quoted and missing entrypoints fail closed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/pr-workflow-contract.test.ts test/pr-e2e-gate-shards.test.ts --project integration` (22 passed); `npx vitest run test/e2e/support/e2e-workflow.test.ts --project e2e-support` (36 passed); workflow validator, `npm run source-shape:check`, and `npm run checks` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **CI & Build Improvements** - Updated CLI coverage and sourcemap verification to automatically run the correct `.mts` vs `.ts` script variant when present. - **Bug Fixes** - Fixed Bedrock-compatible Anthropic E2E shard propagation by requiring the runtime shard to match the matrix agent and failing fast on drift/mismatch. - **Tests** - Added/expanded E2E and PR workflow contract tests for shard expectations and boundary validation. - Enhanced workflow step execution coverage with optional working-directory support and `.ts`/`.mts` entrypoint compatibility checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Prerequisite #6969 is merged, this branch is reconciled, and the final diff preserves one agent-shard mapping plus a live assertion that the CI shard matches the matrix agent. Focused workflow, gate, adapter, package, type, and full coverage checks pass; re-review is clean.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-reviewed after #6969 and reconciliation. The launcher migration, bounded legacy/new process recognition, single shard mapping, CI shard assertion, and regression coverage are correct. Focused suites, package contracts, type-check, full coverage, commit hooks, and pre-push all pass.
|
Advisor follow-up: I reproduced the launcher directly on the repository minimum runtime with |
## Summary Migrate the host-side Ollama authentication proxy from CommonJS `scripts/ollama-auth-proxy.js` to the typed ESM `scripts/ollama-auth-proxy.mts` entrypoint while preserving the existing request-handling and lifecycle contract. This maintainer salvage preserves the verified commits from #6949 and reconciles its process matcher with the Bedrock adapter migration merged in #6938. ## Related Issue Resolves #6926 Part of #6918 Supersedes #6949 ## Changes - Rename the Ollama authentication proxy entrypoint to `.mts` and retain its fail-closed Bearer-token check, byte-length gate before `timingSafeEqual`, sensitive-header stripping, loopback backend, public listener, and nonzero `EADDRINUSE` behavior. - Match only filename-bounded legacy `.js` and current `.mts` proxy processes during lifecycle cleanup and uninstall, with positive coverage for both names and negative coverage for helper and suffix near matches. - Reconcile the shared local-adapter matcher after #6938: strings use substring matching, regular expressions retain Bedrock's bounded launcher matching, and callbacks support Ollama's ownership predicate. All call sites use the existing `processMatcher` name. - Repoint unit, recovery, uninstall, and live E2E fixtures to the `.mts` entrypoint. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: the filenames and shared matcher are internal; existing setup, lifecycle, uninstall, and port-conflict documentation remains accurate. A documentation-writer review found no page or standalone changelog change necessary. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: the focused security re-review on #6949 passed after the bounded matcher and negative tests were added; the conflict resolution preserves #6938's bounded Bedrock matcher and was independently revalidated. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 85 focused lifecycle, Bedrock, uninstall, handler, and recovery tests passed after generating ignored build artifacts. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional validation: `npm run build:cli` and `npm run typecheck:cli` passed. All original #6949 commits remain in the branch unchanged, and the signed merge commit records the mechanical two-file resolution against current `main`. --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Updated the authenticated Ollama proxy launcher to the TypeScript module entrypoint with configurable ports. * **Bug Fixes** * Improved proxy and local-adapter process detection so cleanup targets only the intended auth-proxy variants. * Enhanced handling of proxy startup when the configured port is already in use, and better behavior during backend disconnects. * **Tests** * Expanded coverage for proxy ownership, restart/recovery flows, near-name process matching, and error cases (including port conflicts and backend disconnect scenarios). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
2 participants
Summary
Migrates the Bedrock runtime adapter launcher
scripts/bedrock-runtime-adapter.jsfrom a thin CommonJS shim to a typed explicit-ESM.mtsentrypoint, without adding a root"type": "module"declaration. The adapter implementation stays insrc/lib/inference/bedrock-runtime-adapter.ts; the launcher keeps loading the compiled module and starting the adapter from the environment. Start, ownership detection, recovery, stop, state, PID, token, and log behaviour are unchanged.Related Issue
Resolves #6925
Changes
scripts/bedrock-runtime-adapter.jsis renamed toscripts/bedrock-runtime-adapter.mtsand itsrequireis replaced with a static ESMimportof the compiled adapter, named explicitly as../dist/lib/inference/bedrock-runtime-adapter.js. This mirrors the existingopenrouter-runtime-adapter-entry.tslauncher shape.>=22.19.0) withouttsxor installed development dependencies: the adapter is spawned asnode <launcher>and Node strips the.mtstypes natively.bedrock-runtime-adapter), matching theopenrouter-runtime-adapterconvention. A single needle now recognises both the old.jsand the new.mtslauncher, so a pre-upgrade adapter process is detected and stopped during cleanup rather than silently orphaned. The packaged launcher path resolved by the lifecycle now names the.mtsfile.package.jsonneeds no change:filesalready ships the wholescripts/directory, andtsconfig.cli.jsonalready includesscripts/**/*.mts.Type of Change
Quality Gates
scripts/launcher file-extension migration with unchanged runtime behaviour; no contributor-facing command example or doc page references this launcher path.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 unavailablenpm 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
.jsand.mtsadapter launchers.