refactor(checks): migrate repository check suite to .mts - #6940
refactor(checks): migrate repository check suite to .mts#6940laitingsheng wants to merge 12 commits into
Conversation
Checks importing first-party CJS source use dynamic import so the ESM entrypoints resolve named exports at runtime under tsx. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe repository check suite migrates check entrypoints and references to ChangesRepository check migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-6940.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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/checks/dependency-pins.mts`:
- Around line 217-219: Update the version-validation logic near the existing
openshell max-version check to also validate pins.openshell.minVersion with
NUMERIC_VERSION_RE when present. Add a failure for malformed
min_openshell_version using the same X.Y.Z invariant and preserve the existing
maximum-version validation.
In `@scripts/checks/e2e-mock-parity.mts`:
- Line 185: Update the usage error message in the CLI argument validation around
the base option to reference the current e2e-mock-parity.mts entrypoint instead
of the removed .ts filename, while preserving the existing command syntax and
validation behavior.
In `@scripts/checks/layer-import-boundaries.mts`:
- Around line 324-337: Update isCommandBase and the surrounding visit logic to
resolve TypeScript import bindings and identify the registered oclif command
base by symbol, not expression text; accept direct and aliased Command imports
plus namespace-qualified oclif.Command, while rejecting unrelated local classes.
Ensure commandClassCount only counts classes extending exactly that resolved
base, and add focused detection and false-positive tests covering these cases.
- Around line 43-49: Update the walkers in
scripts/checks/layer-import-boundaries.mts lines 43-49 and
scripts/checks/no-test-dist-imports.mts lines 72-77 to inspect directory entries
or use lstatSync(), skip symbolic links before recursion or file processing, and
add a regression test covering a symlink loop in
scripts/checks/no-test-dist-imports.mts. Ensure both checks cannot traverse
PR-controlled symlink cycles.
- Around line 31-32: Update isProductionTsFile to recognize all supported
TypeScript module extensions (.ts, .mts, .cts, and .tsx), while continuing to
exclude test and spec files for each extension. Ensure the layer-boundary scan
uses this predicate so protected-layer audits cannot be bypassed by alternate
TypeScript extensions, preserving the ratchet’s monotonic behavior.
In `@scripts/checks/no-coverage-ignore.mts`:
- Around line 17-19: Update SOURCE_EXTENSIONS to include ".mts" so
no-coverage-ignore scans migrated TypeScript module files. Add focused tests
covering detection of a forbidden directive in an .mts candidate and confirming
non-matching files or content remain false positives.
In `@scripts/checks/no-test-dist-imports.mts`:
- Around line 57-60: Update isFastProjectTestPath to derive fast-project test
entries from the canonical Vitest configuration instead of maintaining a
separate path pattern. Include root test/**/*.test.ts files classified as fast
tests, ensure every candidate belongs to exactly one configured project, and
preserve the existing exclusions such as .claude paths.
In `@scripts/checks/no-unit-blocks-in-live-e2e.mts`:
- Around line 67-82: Update findLiveUnitBlocks to track whether each line is
inside a multi-line /* ... */ block comment, skipping comment contents and
correctly handling comment start and end markers on the same or different lines.
Preserve the existing import, //, and * comment exclusions, and only apply
IT_PRIMITIVE_PATTERN to code outside block comments.
In `@scripts/checks/test-create-require-budget.mts`:
- Around line 19-58: The createRequire allowlists must not be expanded by the
checked PR. Update the validation logic in this test to compare
CLI_CREATE_REQUIRE_FILES and TEST_SUPPORT_CREATE_REQUIRE_FILES against their
merge-base versions, reject any newly added paths, and continue permitting
removals while preserving existing checks.
🪄 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: 1e7aa52c-05ac-40a2-915a-61f1372a2bab
📒 Files selected for processing (33)
.agents/skills/nemoclaw-contributor-update-dependencies/SKILL.md.github/actions/ci-cli-coverage-shard/action.yaml.github/workflows/pr.yamlpackage.jsonscripts/checks/dependency-pins.mtsscripts/checks/direct-credential-env.mtsscripts/checks/e2e-mock-parity.mtsscripts/checks/hermes-light-skin-boundary.mtsscripts/checks/layer-import-boundaries.mtsscripts/checks/local-credential-helper-pin.mtsscripts/checks/no-coverage-ignore.mtsscripts/checks/no-test-dist-imports.mtsscripts/checks/no-unit-blocks-in-live-e2e.mtsscripts/checks/openshell-policy-mutation-read.mtsscripts/checks/run.mtsscripts/checks/test-create-require-budget.mtsscripts/checks/test-title-style.mtsscripts/checks/vitest-project-overlap.mtssrc/lib/messaging/manifest/types.test.tssrc/lib/security/credential-env.tstest/checks-runner.test.tstest/dependency-pins-check.test.tstest/e2e-mock-parity.test.tstest/layer-import-boundaries.test.tstest/local-credential-helper-pin.test.tstest/no-coverage-ignore.test.tstest/no-direct-credential-env.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.tstest/test-boundary-guards.test.tstest/test-create-require-budget.test.tstest/test-title-style.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: 9
🤖 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/checks/dependency-pins.mts`:
- Around line 217-219: Update the version-validation logic near the existing
openshell max-version check to also validate pins.openshell.minVersion with
NUMERIC_VERSION_RE when present. Add a failure for malformed
min_openshell_version using the same X.Y.Z invariant and preserve the existing
maximum-version validation.
In `@scripts/checks/e2e-mock-parity.mts`:
- Line 185: Update the usage error message in the CLI argument validation around
the base option to reference the current e2e-mock-parity.mts entrypoint instead
of the removed .ts filename, while preserving the existing command syntax and
validation behavior.
In `@scripts/checks/layer-import-boundaries.mts`:
- Around line 324-337: Update isCommandBase and the surrounding visit logic to
resolve TypeScript import bindings and identify the registered oclif command
base by symbol, not expression text; accept direct and aliased Command imports
plus namespace-qualified oclif.Command, while rejecting unrelated local classes.
Ensure commandClassCount only counts classes extending exactly that resolved
base, and add focused detection and false-positive tests covering these cases.
- Around line 43-49: Update the walkers in
scripts/checks/layer-import-boundaries.mts lines 43-49 and
scripts/checks/no-test-dist-imports.mts lines 72-77 to inspect directory entries
or use lstatSync(), skip symbolic links before recursion or file processing, and
add a regression test covering a symlink loop in
scripts/checks/no-test-dist-imports.mts. Ensure both checks cannot traverse
PR-controlled symlink cycles.
- Around line 31-32: Update isProductionTsFile to recognize all supported
TypeScript module extensions (.ts, .mts, .cts, and .tsx), while continuing to
exclude test and spec files for each extension. Ensure the layer-boundary scan
uses this predicate so protected-layer audits cannot be bypassed by alternate
TypeScript extensions, preserving the ratchet’s monotonic behavior.
In `@scripts/checks/no-coverage-ignore.mts`:
- Around line 17-19: Update SOURCE_EXTENSIONS to include ".mts" so
no-coverage-ignore scans migrated TypeScript module files. Add focused tests
covering detection of a forbidden directive in an .mts candidate and confirming
non-matching files or content remain false positives.
In `@scripts/checks/no-test-dist-imports.mts`:
- Around line 57-60: Update isFastProjectTestPath to derive fast-project test
entries from the canonical Vitest configuration instead of maintaining a
separate path pattern. Include root test/**/*.test.ts files classified as fast
tests, ensure every candidate belongs to exactly one configured project, and
preserve the existing exclusions such as .claude paths.
In `@scripts/checks/no-unit-blocks-in-live-e2e.mts`:
- Around line 67-82: Update findLiveUnitBlocks to track whether each line is
inside a multi-line /* ... */ block comment, skipping comment contents and
correctly handling comment start and end markers on the same or different lines.
Preserve the existing import, //, and * comment exclusions, and only apply
IT_PRIMITIVE_PATTERN to code outside block comments.
In `@scripts/checks/test-create-require-budget.mts`:
- Around line 19-58: The createRequire allowlists must not be expanded by the
checked PR. Update the validation logic in this test to compare
CLI_CREATE_REQUIRE_FILES and TEST_SUPPORT_CREATE_REQUIRE_FILES against their
merge-base versions, reject any newly added paths, and continue permitting
removals while preserving existing checks.
🪄 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: 1e7aa52c-05ac-40a2-915a-61f1372a2bab
📒 Files selected for processing (33)
.agents/skills/nemoclaw-contributor-update-dependencies/SKILL.md.github/actions/ci-cli-coverage-shard/action.yaml.github/workflows/pr.yamlpackage.jsonscripts/checks/dependency-pins.mtsscripts/checks/direct-credential-env.mtsscripts/checks/e2e-mock-parity.mtsscripts/checks/hermes-light-skin-boundary.mtsscripts/checks/layer-import-boundaries.mtsscripts/checks/local-credential-helper-pin.mtsscripts/checks/no-coverage-ignore.mtsscripts/checks/no-test-dist-imports.mtsscripts/checks/no-unit-blocks-in-live-e2e.mtsscripts/checks/openshell-policy-mutation-read.mtsscripts/checks/run.mtsscripts/checks/test-create-require-budget.mtsscripts/checks/test-title-style.mtsscripts/checks/vitest-project-overlap.mtssrc/lib/messaging/manifest/types.test.tssrc/lib/security/credential-env.tstest/checks-runner.test.tstest/dependency-pins-check.test.tstest/e2e-mock-parity.test.tstest/layer-import-boundaries.test.tstest/local-credential-helper-pin.test.tstest/no-coverage-ignore.test.tstest/no-direct-credential-env.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/policy-mutation-read-discovery.test.tstest/pr-workflow-contract.test.tstest/test-boundary-guards.test.tstest/test-create-require-budget.test.tstest/test-title-style.test.ts
🛑 Comments failed to post (9)
scripts/checks/dependency-pins.mts (1)
217-219: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate
min_openshell_versionas well.Only the maximum is checked against
X.Y.Z. A malformed minimum can pass when its consumers contain the same value, allowing an invalid installer constraint through this guardrail.Proposed fix
+ if (pins.openshell.minVersion && !NUMERIC_VERSION_RE.test(pins.openshell.minVersion)) { + failures.push("nemoclaw-blueprint/blueprint.yaml min_openshell_version must match X.Y.Z"); + } if (pins.openshell.maxVersion && !NUMERIC_VERSION_RE.test(pins.openshell.maxVersion)) {As per path instructions, “Enforce objective invariants with deterministic code.”
📝 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.if (pins.openshell.minVersion && !NUMERIC_VERSION_RE.test(pins.openshell.minVersion)) { failures.push("nemoclaw-blueprint/blueprint.yaml min_openshell_version must match X.Y.Z"); } if (pins.openshell.maxVersion && !NUMERIC_VERSION_RE.test(pins.openshell.maxVersion)) { failures.push("nemoclaw-blueprint/blueprint.yaml max_openshell_version must match X.Y.Z"); }🤖 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/checks/dependency-pins.mts` around lines 217 - 219, Update the version-validation logic near the existing openshell max-version check to also validate pins.openshell.minVersion with NUMERIC_VERSION_RE when present. Add a failure for malformed min_openshell_version using the same X.Y.Z invariant and preserve the existing maximum-version validation.Source: Path instructions
scripts/checks/e2e-mock-parity.mts (1)
185-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the CLI usage to reference the
.mtsentrypoint.The reported command still targets the removed
.tsfile, so copying it will fail.Proposed fix
- if (!base) throw new Error("usage: e2e-mock-parity.ts --base <git-ref> [--head <git-ref>]"); + if (!base) throw new Error("usage: e2e-mock-parity.mts --base <git-ref> [--head <git-ref>]");📝 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.if (!base) throw new Error("usage: e2e-mock-parity.mts --base <git-ref> [--head <git-ref>]");🤖 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/checks/e2e-mock-parity.mts` at line 185, Update the usage error message in the CLI argument validation around the base option to reference the current e2e-mock-parity.mts entrypoint instead of the removed .ts filename, while preserving the existing command syntax and validation behavior.scripts/checks/layer-import-boundaries.mts (3)
31-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scan every supported TypeScript module extension.
Restricting the audit to
.tslets.mts,.cts, or.tsxfiles under protected layers bypass every rule.Proposed fix
function isProductionTsFile(absPath: string): boolean { - return absPath.endsWith(".ts") && !absPath.endsWith(".test.ts") && !absPath.endsWith(".spec.ts"); + return ( + /\.(?:ts|tsx|mts|cts)$/u.test(absPath) && + !/\.(?:test|spec)\.(?:ts|tsx|mts|cts)$/u.test(absPath) + ); } function sourceFileFor(absPath: string): ts.SourceFile { return ts.createSourceFile( absPath, readFileSync(absPath, "utf8"), ts.ScriptTarget.Latest, true, - ts.ScriptKind.TS, + absPath.endsWith(".tsx") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, ); }As per path instructions, “A ratchet must be monotonic and must not be weakenable by the PR it checks.”
Also applies to: 55-62
🤖 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/checks/layer-import-boundaries.mts` around lines 31 - 32, Update isProductionTsFile to recognize all supported TypeScript module extensions (.ts, .mts, .cts, and .tsx), while continuing to exclude test and spec files for each extension. Ensure the layer-boundary scan uses this predicate so protected-layer audits cannot be bypassed by alternate TypeScript extensions, preserving the ratchet’s monotonic behavior.Source: Path instructions
43-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent symlink traversal across repository guardrails.
Both walkers dereference symbolic links, allowing a PR-controlled loop to prevent the checks from completing.
scripts/checks/layer-import-boundaries.mts#L43-L49: use directory entries orlstatSync()and skip symbolic links.scripts/checks/no-test-dist-imports.mts#L72-L77: apply the same non-following traversal and add a symlink-loop regression test.As per path instructions, “A ratchet must be monotonic and must not be weakenable by the PR it checks.”
📍 Affects 2 files
scripts/checks/layer-import-boundaries.mts#L43-L49(this comment)scripts/checks/no-test-dist-imports.mts#L72-L77🤖 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/checks/layer-import-boundaries.mts` around lines 43 - 49, Update the walkers in scripts/checks/layer-import-boundaries.mts lines 43-49 and scripts/checks/no-test-dist-imports.mts lines 72-77 to inspect directory entries or use lstatSync(), skip symbolic links before recursion or file processing, and add a regression test covering a symlink loop in scripts/checks/no-test-dist-imports.mts. Ensure both checks cannot traverse PR-controlled symlink cycles.Source: Path instructions
324-337: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the actual oclif command base instead of matching source text.
A local unrelated
Commandclass is accepted, while aliases such asCommand as OclifCommandoroclif.Commandare rejected. Resolve import bindings so the rule proves that exactly one class extends the registered command base.As per path instructions, guardrails must enforce objective invariants and include focused detection and false-positive tests.
🤖 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/checks/layer-import-boundaries.mts` around lines 324 - 337, Update isCommandBase and the surrounding visit logic to resolve TypeScript import bindings and identify the registered oclif command base by symbol, not expression text; accept direct and aliased Command imports plus namespace-qualified oclif.Command, while rejecting unrelated local classes. Ensure commandClassCount only counts classes extending exactly that resolved base, and add focused detection and false-positive tests covering these cases.Source: Path instructions
scripts/checks/no-coverage-ignore.mts (1)
17-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include
.mtsfiles in the coverage-directive scan.The migration makes
.mtsa live source extension, but this inventory omits it. Consequently, all migrated check entrypoints can contain forbidden coverage directives without detection.Proposed fix
-const SOURCE_EXTENSIONS = new Set([".cjs", ".cts", ".js", ".mjs", ".ts", ".tsx"]); +const SOURCE_EXTENSIONS = new Set([".cjs", ".cts", ".js", ".mjs", ".mts", ".ts", ".tsx"]);Add a focused discovery test proving an
.mtscandidate is scanned. As per path instructions, guardrails require focused detection and false-positive tests.📝 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.const SCAN_ROOTS = ["bin", "src", "scripts", "test", "nemoclaw/src"]; const SOURCE_EXTENSIONS = new Set([".cjs", ".cts", ".js", ".mjs", ".mts", ".ts", ".tsx"]); const SKIP_DIRS = new Set([".git", "coverage", "dist", "node_modules"]);🤖 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/checks/no-coverage-ignore.mts` around lines 17 - 19, Update SOURCE_EXTENSIONS to include ".mts" so no-coverage-ignore scans migrated TypeScript module files. Add focused tests covering detection of a forbidden directive in an .mts candidate and confirming non-matching files or content remain false positives.Source: Path instructions
scripts/checks/no-test-dist-imports.mts (1)
57-60: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Derive fast-project entries from the canonical Vitest configuration.
This inventory omits root
test/**/*.test.tsfiles, althoughscripts/checks/e2e-mock-parity.mtsLines 26-30 classifies them as fast PR tests. Their dependencies are therefore not traversed, allowing a source module that imports compiled internals to evade the transitive audit.As per coding guidelines, every test candidate must belong to exactly one configured project. As per path instructions, derive inventories from a canonical source rather than duplicating lists.
🤖 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/checks/no-test-dist-imports.mts` around lines 57 - 60, Update isFastProjectTestPath to derive fast-project test entries from the canonical Vitest configuration instead of maintaining a separate path pattern. Include root test/**/*.test.ts files classified as fast tests, ensure every candidate belongs to exactly one configured project, and preserve the existing exclusions such as .claude paths.Sources: Coding guidelines, Path instructions
scripts/checks/no-unit-blocks-in-live-e2e.mts (1)
67-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Multi-line block comments aren't skipped, risking false positives.
findLiveUnitBlocksonly skips//line comments and JSDoc-style*-prefixed continuation lines. A plain/* ... */block spanning multiple lines, where the body line doesn't start with*, would still have itsit(...)line flagged — a false positive that fails CI for legitimately commented-out code.🐛 Proposed fix to track block-comment state
export function findLiveUnitBlocks(source: string, file: string): LiveUnitBlockViolation[] { const violations: LiveUnitBlockViolation[] = []; const lines = source.split(/\r\n|\r|\n/); + let inBlockComment = false; for (let i = 0; i < lines.length; i += 1) { const text = lines[i] ?? ""; const trimmed = text.trimStart(); + if (inBlockComment) { + if (trimmed.includes("*/")) inBlockComment = false; + continue; + } + if (trimmed.startsWith("/*") && !trimmed.includes("*/")) { + inBlockComment = true; + continue; + } // Skip import lines (`import { it, test } from "vitest"`) and comments. if (trimmed.startsWith("import ") || trimmed.startsWith("//") || trimmed.startsWith("*")) { continue; } if (IT_PRIMITIVE_PATTERN.test(text)) { violations.push({ file, line: i + 1, text: trimmed }); } } return violations; }📝 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.export function findLiveUnitBlocks(source: string, file: string): LiveUnitBlockViolation[] { const violations: LiveUnitBlockViolation[] = []; const lines = source.split(/\r\n|\r|\n/); let inBlockComment = false; for (let i = 0; i < lines.length; i += 1) { const text = lines[i] ?? ""; const trimmed = text.trimStart(); if (inBlockComment) { if (trimmed.includes("*/")) inBlockComment = false; continue; } if (trimmed.startsWith("/*") && !trimmed.includes("*/")) { inBlockComment = true; continue; } // Skip import lines (`import { it, test } from "vitest"`) and comments. if (trimmed.startsWith("import ") || trimmed.startsWith("//") || trimmed.startsWith("*")) { continue; } if (IT_PRIMITIVE_PATTERN.test(text)) { violations.push({ file, line: i + 1, text: trimmed }); } } return violations; }🤖 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/checks/no-unit-blocks-in-live-e2e.mts` around lines 67 - 82, Update findLiveUnitBlocks to track whether each line is inside a multi-line /* ... */ block comment, skipping comment contents and correctly handling comment start and end markers on the same or different lines. Preserve the existing import, //, and * comment exclusions, and only apply IT_PRIMITIVE_PATTERN to code outside block comments.scripts/checks/test-create-require-budget.mts (1)
19-58: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the
createRequireratchet immutable against the checked PR.A PR can introduce a new seam and add its path to these allowlists, making this check pass. Compare the allowlists with the merge-base and reject additions while permitting removals.
As per path instructions, “A ratchet must be monotonic and must not be weakenable by the PR it checks.”
Also applies to: 159-195
🤖 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/checks/test-create-require-budget.mts` around lines 19 - 58, The createRequire allowlists must not be expanded by the checked PR. Update the validation logic in this test to compare CLI_CREATE_REQUIRE_FILES and TEST_SUPPORT_CREATE_REQUIRE_FILES against their merge-base versions, reject any newly added paths, and continue permitting removals while preserving existing checks.Source: Path instructions
The base-pinned ci-cli-coverage-shard action still invokes the pre-migration `.ts` path until it lands on main. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/checks/e2e-mock-parity.ts (2)
1-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo focused test for the shim's forwarding behavior.
As per path instructions for
scripts/checks/**, checks-related scripts should "require focused tests for both detection and false-positive behavior." This shim has no test asserting it spawnsnode --import tsxagainst the.mtspath with the correct forwarded args/cwd, unlike the other check modules in this cohort. A small unit test mockingspawnSyncwould catch regressions before the shim is removed.🤖 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/checks/e2e-mock-parity.ts` around lines 1 - 27, add a focused unit test for the compatibility shim’s forwarding behavior, mocking spawnSync to verify it invokes the current Node executable with --import tsx, the e2e-mock-parity.mts target, forwarded command-line arguments, and REPO_ROOT as cwd. Also assert the shim propagates the spawned process status, while keeping the test isolated from actually launching the script.Source: Path instructions
9-9: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpawn failure is swallowed silently.
If
spawnSyncfails to launch the child (e.g.,tsxnot resolvable),result.erroris set andresult.statusisnull, soprocess.exit(result.status ?? 1)exits with1but never surfaces the underlying reason. During the migration window this shim is meant to bridge, a silent unexplained failure makes CI triage harder.The
ast-grepcommand-injection hint on thechild_processimport doesn't apply:spawnSyncis called with an argv array (not a shell string) and theshelloption is not enabled, soprocess.argv.slice(2)is passed as discrete arguments rather than being shell-interpolated.🩹 Proposed fix to surface spawn errors
const result = spawnSync( process.execPath, [ "--import", "tsx", path.join(REPO_ROOT, "scripts/checks/e2e-mock-parity.mts"), ...process.argv.slice(2), ], { cwd: REPO_ROOT, stdio: "inherit" }, ); +if (result.error) { + console.error(result.error); +} + process.exit(result.status ?? 1);Also applies to: 15-26
🤖 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/checks/e2e-mock-parity.ts` at line 9, Update the spawnSync execution flow in this script to check result.error before exiting, and report the underlying launch failure through the script’s existing error-output mechanism. Preserve the current status-based exit behavior for successfully spawned commands, including nonzero child exit statuses.
🤖 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 `@scripts/checks/e2e-mock-parity.ts`:
- Around line 1-27: add a focused unit test for the compatibility shim’s
forwarding behavior, mocking spawnSync to verify it invokes the current Node
executable with --import tsx, the e2e-mock-parity.mts target, forwarded
command-line arguments, and REPO_ROOT as cwd. Also assert the shim propagates
the spawned process status, while keeping the test isolated from actually
launching the script.
- Line 9: Update the spawnSync execution flow in this script to check
result.error before exiting, and report the underlying launch failure through
the script’s existing error-output mechanism. Preserve the current status-based
exit behavior for successfully spawned commands, including nonzero child exit
statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62b38d74-0d05-45c4-be30-a09b75717458
📒 Files selected for processing (1)
scripts/checks/e2e-mock-parity.ts
Issue #6921 requires no live scripts/checks/*.ts entrypoint after migration. The trusted, base-pinned ci-cli-coverage-shard action will stay red against the old path until a prerequisite change updates it on main. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
# Conflicts: # package.json # scripts/checks/local-credential-helper-pin.mts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
This migration is not mergeable yet at commit 889dca0:
- The trusted coverage-shard action on current
mainstill invokesscripts/checks/e2e-mock-parity.ts, which this PR deletes. CI fails withERR_MODULE_NOT_FOUND. Land a small compatibility prerequisite that selects the available.mtsor.tsentrypoint, then rebase this PR. scripts/checks/no-coverage-ignore.mtsomits.mtsfromSOURCE_EXTENSIONS, excluding all migrated checks from coverage-directive enforcement. Add.mtsplus positive and false-positive regression tests.scripts/checks/e2e-mock-parity.mtsstill advertises the removed.tscommand; update the diagnostic.- Add the focused negative test required by #6921 for an unreviewed Hermes boundary version.
Local focused tests and the registered checks pass, but these enforcement and trusted-action gaps must be closed before merge.
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/checks/e2e-mock-parity.ts`:
- Around line 4-10: Delete the stale e2e-mock-parity.ts wrapper, including its
forwarding import and main() invocation; retain the implementation in
e2e-mock-parity.mts 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: a2d3029d-fa92-40a0-976d-109f88588280
📒 Files selected for processing (2)
scripts/checks/e2e-mock-parity.mtsscripts/checks/e2e-mock-parity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/checks/e2e-mock-parity.mts
|
Superseded by #7047, a clean current-main replacement. Tinson Lai retains authorship and sign-off on the four original commits, and the follow-up guardrail hardening credits Tinson as co-author. The replacement removes the live .ts shim and addresses the review findings without rewriting the original branch. |
## Summary Migrates the repository check suite from `.ts` entrypoints to `.mts` and removes the final live `.ts` shim. This clean current-`main` replacement for #6940 preserves Tinson Lai as the author of the original four commits and credits him as co-author of the follow-up hardening. ## Related Issue Closes #6921 ## Changes - Rename the coordinated `scripts/checks` suite to `.mts` and update package scripts, workflows, actions, tests, diagnostics, and contributor-skill references. - Preserve the trusted coverage action's current `.mts`-first compatibility selection while leaving no live `scripts/checks/*.ts` entrypoint in the tree. - Close review gaps in version validation, `.mts` coverage scanning, canonical Vitest project discovery, live-E2E AST detection, oclif command binding detection, and merge-base createRequire allowlist enforcement. - Make repository walkers and transitive import resolution symlink-safe and canonical, including loop and protected-layer alias regressions. - Move the compiled credentials shim assertion into the dedicated package-contract test lane. ## 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 repository checks and CI entrypoint extensions change; contributor-facing npm commands and product behavior remain unchanged. Independent docs audit found no stale public references. - [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: independent maintainer security audit completed for the static enforcement and CI changes; no runtime permission or credential handling behavior changes. - [ ] 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 — repository-check focused tests passed; final layer/import-boundary suite: 62/62; package-contract credentials suite: 4/4 - [ ] Applicable broad gate passed — `npm run checks` passed. `npm run check` was attempted, but its CLI coverage lane failed across 265 unrelated tests under the concurrent local environment: spawned Python fixtures reported missing `tomllib`/PyYAML and Node 26 deprecation warnings polluted expected-empty stderr. Focused suites, `npm run check:diff`, typecheck, and repository checks pass; hosted CI remains required. - [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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated validation for Hermes compatibility versions and trusted `createRequire` allowlists. * Expanded static checks to cover modern TypeScript file types, symlink handling, module resolution, Vitest project assignments, and policy-read discovery. * Added stricter validation for OpenShell version pins and credential artifact URLs. * **Bug Fixes** * Improved detection of invalid test structures, import-boundary violations, coverage directives, and live E2E unit blocks. * **Tests** * Expanded coverage for new validation rules, aliases, alternate file extensions, symlinks, and workflow requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
5 participants
Summary
Migrate the repository check suite from
.tsto.mts. The runner and every check entrypoint move together, and all consumers that named the old extension are updated. The check set, ordering, arguments, Windows command construction, and exit behavior are unchanged.Related Issue
Resolves #6921
Changes
scripts/checks/*.tsentrypoints andrun.tsto.mts.checks,test:imports:check,test:projects:check, andtest:titles:checkpackage scripts.pr.yamlandmain.yamlworkflows and theci-cli-coverage-shardcomposite action that invokee2e-mock-parity.direct-credential-envusage string, two source comments, and one skill doc that named the old extension; add the explicit.mtsextension to test imports of the migrated checks.direct-credential-env,hermes-light-skin-boundary,local-credential-helper-pin) to dynamicimport(). Under tsx an ESM.mtsentrypoint cannot statically resolve named exports from a first-party.tsCommonJS module, so the static named imports throw at runtime; dynamicimport()resolves the named exports and keeps full types.npm run checksand the per-check tests protect this.Type of Change
Quality Gates
.ts→.mtsrename plus dynamic-import adaptation of the credential and policy checks; no security behavior change andnpm run checkspasses unchanged — pending maintainer reviewVerification
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 run typecheck:clipass;npm run checksall 12 checks pass on node 22.22.2;npx vitest runon the runner, workflow-contract, and 11 per-check tests — 13 files / 173 tests pass;biome lintandbiome formatcleannpm 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