chore(scripts): migrate source and test analysis tools to .mts - #6935
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-6935.docs.buildwithfern.com/nemoclaw |
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 |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review 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: Blockers
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…e-shape checks 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 pull request migrates analysis and coverage tooling to explicit ChangesESM tooling migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant PackageScripts
participant AnalysisCLI
participant Filesystem
participant TestSuite
Developer->>PackageScripts: invoke .mts analysis command
PackageScripts->>AnalysisCLI: execute selected tool
AnalysisCLI->>Filesystem: scan source, test, coverage, or dist files
Filesystem-->>AnalysisCLI: return inputs and findings
AnalysisCLI-->>TestSuite: produce output and exit status
TestSuite-->>Developer: validate CLI and import behavior
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/check-coverage-ratchet.mts`:
- Around line 71-73: Update the usage error message in the coverage-ratchet CLI
to advertise the current `.mts` entrypoint instead of the obsolete `.ts` path,
leaving the argument documentation unchanged.
In `@test/dist-sourcemaps.test.ts`:
- Line 36: Update the parent dist sourcemap test suite title in describe to
append the migration issue reference as a final (`#1234`)-style suffix, leaving
the nested it titles unchanged.
🪄 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: 42857500-9ae5-4b64-95bf-dfea712b2819
📒 Files selected for processing (22)
.github/actions/ci-cli-coverage-merge/action.yaml.github/actions/ci-cli-coverage-shard/action.yaml.github/actions/ci-plugin-coverage/action.yaml.pre-commit-config.yamlpackage.jsonscripts/check-coverage-ratchet.mtsscripts/check-dist-sourcemaps.mtsscripts/check-test-file-size-budget.mtsscripts/find-source-shape-tests.mtsscripts/find-test-conditionals.mtsscripts/list-command-helper-uses.mtsscripts/type-safety-hotspots.mtstest/codebase-growth-guardrails-conditionals.test.tstest/coverage-ratchet.test.tstest/dist-sourcemaps.test.tstest/docker-abstraction-guard.test.tstest/list-command-helper-uses.test.tstest/pr-workflow-contract.test.tstest/source-shape-scanner.test.tstest/test-conditionals-scanner.test.tstest/test-file-size-budget.test.tstest/type-safety-hotspots.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 `@scripts/check-coverage-ratchet.mts`:
- Around line 71-73: Update the usage error message in the coverage-ratchet CLI
to advertise the current `.mts` entrypoint instead of the obsolete `.ts` path,
leaving the argument documentation unchanged.
In `@test/dist-sourcemaps.test.ts`:
- Line 36: Update the parent dist sourcemap test suite title in describe to
append the migration issue reference as a final (`#1234`)-style suffix, leaving
the nested it titles unchanged.
🪄 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: 42857500-9ae5-4b64-95bf-dfea712b2819
📒 Files selected for processing (22)
.github/actions/ci-cli-coverage-merge/action.yaml.github/actions/ci-cli-coverage-shard/action.yaml.github/actions/ci-plugin-coverage/action.yaml.pre-commit-config.yamlpackage.jsonscripts/check-coverage-ratchet.mtsscripts/check-dist-sourcemaps.mtsscripts/check-test-file-size-budget.mtsscripts/find-source-shape-tests.mtsscripts/find-test-conditionals.mtsscripts/list-command-helper-uses.mtsscripts/type-safety-hotspots.mtstest/codebase-growth-guardrails-conditionals.test.tstest/coverage-ratchet.test.tstest/dist-sourcemaps.test.tstest/docker-abstraction-guard.test.tstest/list-command-helper-uses.test.tstest/pr-workflow-contract.test.tstest/source-shape-scanner.test.tstest/test-conditionals-scanner.test.tstest/test-file-size-budget.test.tstest/type-safety-hotspots.test.ts
🛑 Comments failed to post (2)
scripts/check-coverage-ratchet.mts (1)
71-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the usage path to
.mts.The CLI advertises
coverage-ratchet.ts, leaving a live old-path reference and directing users to a removed entrypoint.Proposed fix
- "Usage: coverage-ratchet.ts <coverage-summary.json> <coverage-threshold.json> [label]", + "Usage: check-coverage-ratchet.mts <coverage-summary.json> <coverage-threshold.json> [label]",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.throw new Error( "Usage: check-coverage-ratchet.mts <coverage-summary.json> <coverage-threshold.json> [label]", );🤖 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/check-coverage-ratchet.mts` around lines 71 - 73, Update the usage error message in the coverage-ratchet CLI to advertise the current `.mts` entrypoint instead of the obsolete `.ts` path, leaving the argument documentation unchanged.test/dist-sourcemaps.test.ts (1)
36-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the issue suffix to the suite title.
Append the migration issue reference to the parent
describe; its suffix covers the nestedittitles.Proposed fix
-describe("dist sourcemap checks", () => { +describe("dist sourcemap checks (`#6922`)", () => {As per coding guidelines, “Use behavior-oriented test titles and place local issue references in a final
(#1234)suffix.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe("dist sourcemap checks (`#6922`)", () => {🤖 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/dist-sourcemaps.test.ts` at line 36, Update the parent dist sourcemap test suite title in describe to append the migration issue reference as a final (`#1234`)-style suffix, leaving the nested it titles unchanged.Source: Coding guidelines
…ools-mts Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # package.json
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
This PR is blocked by the trusted coverage actions on current : they still invoke and , which this PR deletes. Land a small compatibility prerequisite that selects the available or entrypoint, then rebase this PR, remove the transitional files, resolve the overlap with #6940, and rerun the full gate. Until that base compatibility exists, the PR cannot produce valid required-check evidence.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Correction with the affected paths: trusted coverage actions on current main still invoke scripts/check-dist-sourcemaps.ts and scripts/check-coverage-ratchet.ts, which this PR deletes. Land a small compatibility prerequisite that selects the available .mts or .ts entrypoint, then rebase this PR, remove the transitional .ts files, resolve the overlap with #6940, and rerun the full gate. Until that base compatibility exists, the PR cannot produce valid required-check evidence.
Superseded by the immediately following corrected review with complete path and contract names.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/check-coverage-ratchet.ts`:
- Around line 4-21: Remove the legacy wrapper files
scripts/check-coverage-ratchet.ts and scripts/check-dist-sourcemaps.ts, since
package scripts and CI use their .mts entrypoints. Only retain either shim if a
documented external consumer requires it; in that case, add a regression test
covering the compatibility path.
In `@scripts/check-dist-sourcemaps.ts`:
- Line 18: Update the command options in the check-dist-sourcemaps flow to stop
forcing cwd to REPO_ROOT, so relative distDir arguments continue resolving from
the caller’s working directory. Preserve REPO_ROOT usage for repository-specific
paths or setup that requires it, but leave subprocess path resolution relative
to the invocation directory.
🪄 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: e54a10f3-acd2-458e-a055-c83343226bb3
📒 Files selected for processing (2)
scripts/check-coverage-ratchet.tsscripts/check-dist-sourcemaps.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Post-prerequisite reconciliation plan: after #6969 merges, merge current main and resolve the four coverage-action hunks by keeping the .mts-first/.ts-fallback blocks from main. Delete the two transitional wrapper files in this PR, but do not replace those action blocks with direct .mts-only commands yet; #6969 intentionally keeps the fallback for trusted-CI compatibility during the migration window. Retain the compatibility regression from main, then run the focused coverage/sourcemap/scanner/action-contract tests and the normal repo checks. |
<!-- 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 and this branch is reconciled. The final diff removes the obsolete wrappers, retains current main coverage-action compatibility, preserves the intended .mts migrations, and passes focused plus full coverage validation. 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 final branch cleanly migrates the internal analysis tools to .mts, retains trusted coverage-action compatibility, removes obsolete wrappers, and adds entrypoint regressions. Focused tests, full CLI/plugin coverage, commit hooks, and pre-push all pass.
2 participants
Summary
Migrates the seven source- and test-analysis tools under
scripts/—check-coverage-ratchet,check-dist-sourcemaps,check-test-file-size-budget,find-source-shape-tests,find-test-conditionals,list-command-helper-uses, andtype-safety-hotspots— from.tsto explicit ESM.mts, making each tool's module contract explicit without adding a root"type": "module"declaration. This is the low–medium-effort analysis-tools workstream of the tool migration epic; scanner findings, coverage-ratchet output, metrics, JSON output, and exit codes are unchanged.Related Issue
Resolves #6922
Changes
.mts. Six were already ESM (import/exportwith animport.meta.urlmain guard); onlycheck-dist-sourcemapscarried a CommonJSrequire.main === moduleguard, which would throw under.mts. It now uses the same ESM main check as its sibling tools (fileURLToPath(import.meta.url) === path.resolve(process.argv[1] ?? "")) via anode:urlimport, so direct execution and helper import stay separate.find-source-shape-tests,find-test-conditionals, andlist-command-helper-usesnow name their.mtsentrypoints in the direct-invocation check and usage strings.package.json: thetype-safety:hotspots,source-shape:scan,source-shape:check,test-size:check,test-conditionals:scan, and coverage-ratchet/dist-sourcemap steps intest:coverage:cliandtest:coverage:pluginnow invoke the.mtspaths.ci-cli-coverage-merge,ci-cli-coverage-shard,ci-plugin-coverage) and the two.pre-commit-config.yamlhookfilespatterns now target the.mtspaths.coverage-ratchet,list-command-helper-uses,docker-abstraction-guard,pr-workflow-contract) now reference the.mtsfiles, so they keep exercising the same entrypoints throughtsx..tspaths remains. The extensionless helper imports in the focused scanner tests resolve undertsconfig.cli.json'sbundlerresolution; that config already globsscripts/**/*.mts.Type of Change
Quality Gates
scripts/tool file-extension migration; no contributor-facing command example or doc page references these entrypoint paths.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
New Features
Bug Fixes
Tests