test(cli): enforce initial layer import boundaries#2986
Conversation
This reverts commit 4ebeae4.
## Summary Move OpenShell-facing adapter modules out of the flat `src/lib/` directory into `src/lib/adapters/openshell/**` so process/runtime boundaries are visible in paths. ## Stack Navigation - Position: 55 of 60 - Previous PR: [#2984 — refactor(cli): group pure domain helpers](#2984) - Next PR: [#2986 — test(cli): enforce initial layer import boundaries](#2986) ## Changes - Moved OpenShell command helpers to `src/lib/adapters/openshell/client.ts`. - Moved OpenShell runtime wrapper helpers to `src/lib/adapters/openshell/runtime.ts`. - Moved OpenShell binary resolution and timeout constants to `resolve.ts` and `timeouts.ts`. - Moved associated tests and updated action, command, and integration-test imports. - Kept behavior unchanged; this PR is a structural move plus import updates. ## 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) ## Verification - [x] `npx prek run --all-files --stage pre-push` passes - [x] `npm run build:cli` - [x] `npm run typecheck:cli` - [x] OpenShell adapter targeted tests pass - [x] `npm run ts-migration:guard -- --base origin/refactor/layer-domain-helpers --head HEAD` - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/check-layer-import-boundaries.ts (1)
306-308: 💤 Low valueConsider cross-platform compatible execution guard.
The current pattern
import.meta.url === \file://${process.argv[1]}`may not match correctly on Windows due to path separator differences (/vs`). A more robust approach usesfileURLToPath:♻️ Suggested cross-platform fix
-if (import.meta.url === `file://${process.argv[1]}`) { +if (fileURLToPath(import.meta.url) === path.resolve(process.argv[1])) { main(); }🤖 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-layer-import-boundaries.ts` around lines 306 - 308, Replace the fragile URL string comparison with a cross-platform file path comparison: convert import.meta.url to a filesystem path using fileURLToPath and normalize/resolve process.argv[1] (using path.resolve or equivalent) and then compare those values before calling main(); update the guard around main() (the import.meta.url === `file://${process.argv[1]}` check) to use fileURLToPath(import.meta.url) and a resolved process argv path so it works on Windows and POSIX.
🤖 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/check-layer-import-boundaries.ts`:
- Around line 306-308: Replace the fragile URL string comparison with a
cross-platform file path comparison: convert import.meta.url to a filesystem
path using fileURLToPath and normalize/resolve process.argv[1] (using
path.resolve or equivalent) and then compare those values before calling main();
update the guard around main() (the import.meta.url ===
`file://${process.argv[1]}` check) to use fileURLToPath(import.meta.url) and a
resolved process argv path so it works on Windows and POSIX.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee39c540-f200-4fbf-8b80-f8a235a7a5d2
📒 Files selected for processing (3)
.pre-commit-config.yamlscripts/check-layer-import-boundaries.tstest/layer-import-boundaries.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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-layer-import-boundaries.ts`:
- Around line 267-275: The current check only errors when commandClassCount > 1,
allowing zero-command files to slip through; update the logic around
commandClassCount (the rule labeled "one-command-per-file") to enforce exactly
one command class by replacing the condition with a strict equality check (e.g.,
commandClassCount !== 1) and update the addViolation message to reflect "must
define exactly one registered oclif command class; found X". If certain helper
files under src/lib/commands are intentionally allowed to have zero commands,
add an explicit exemption (e.g., by file name or a helper directory pattern)
before the commandClassCount check so those files are skipped instead of
weakening the rule.
- Around line 83-89: The call-expression visitor currently only handles
identifier-based calls (ts.isIdentifier(node.expression)) so dynamic import(...)
expressions are skipped; update the condition around ts.isCallExpression to also
accept when node.expression is an Import keyword (check node.expression.kind ===
ts.SyntaxKind.ImportKeyword or use ts.isImportExpression/ts.isImportCall where
available), and then ensure you still verify node.arguments.length > 0 and
ts.isStringLiteralLike(node.arguments[0]) before calling
add(node.arguments[0].text, node.arguments[0]); this change touches the same
call-expression check that uses ts.isCallExpression, ts.isIdentifier,
node.expression, ts.SyntaxKind.ImportKeyword, and the add(...) invocation.
🪄 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: 3faea048-1978-42cf-9dc3-f14f8eb450f5
📒 Files selected for processing (1)
scripts/check-layer-import-boundaries.ts
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-layer-import-boundaries.ts`:
- Around line 73-90: The visitor in collectImportRefs (function visit) misses
TypeScript's ImportEqualsDeclaration form; update visit to detect
ts.isImportEqualsDeclaration(node) and when node.moduleReference is an
ExternalModuleReference with a string literal expression extract that module
string and call add(moduleText, the string literal node) so import x =
require("...") references are collected like other import/require forms.
- Around line 257-259: The predicate that identifies command classes currently
treats any heritage clause (including implements) as matching; update the check
in the class detection logic (the ts.isClassDeclaration branch that iterates
node.heritageClauses and uses isCommandBase) to only consider extends clauses by
adding a filter that clause.token === ts.SyntaxKind.ExtendsKeyword before
calling clause.types.some(isCommandBase), so only classes that actually extend
the Command base are counted as commands.
🪄 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: ca040331-bb60-4f38-aecb-454879690a64
📒 Files selected for processing (1)
scripts/check-layer-import-boundaries.ts
| function visit(node: ts.Node): void { | ||
| if (ts.isImportDeclaration(node) && ts.isStringLiteral(node.moduleSpecifier)) { | ||
| add(node.moduleSpecifier.text, node.moduleSpecifier); | ||
| } else if ( | ||
| ts.isExportDeclaration(node) && | ||
| node.moduleSpecifier && | ||
| ts.isStringLiteral(node.moduleSpecifier) | ||
| ) { | ||
| add(node.moduleSpecifier.text, node.moduleSpecifier); | ||
| } else if ( | ||
| ts.isCallExpression(node) && | ||
| ((ts.isIdentifier(node.expression) && node.expression.text === "require") || | ||
| node.expression.kind === ts.SyntaxKind.ImportKeyword) && | ||
| node.arguments.length > 0 && | ||
| ts.isStringLiteralLike(node.arguments[0]) | ||
| ) { | ||
| add(node.arguments[0].text, node.arguments[0]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and see its overall structure
fd scripts/check-layer-import-boundaries.tsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's also get the line count to understand file size
wc -l scripts/check-layer-import-boundaries.tsRepository: NVIDIA/NemoClaw
Length of output: 104
🏁 Script executed:
# Now let's examine the collectImportRefs function around lines 73-90
sed -n '60,100p' scripts/check-layer-import-boundaries.tsRepository: NVIDIA/NemoClaw
Length of output: 1499
🏁 Script executed:
# Let's also check if ImportEqualsDeclaration is mentioned anywhere in the file
rg "ImportEqualsDeclaration" scripts/check-layer-import-boundaries.tsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Check if there's any configuration (tsconfig.json, biome.json) that might restrict import forms
fd -e json -e js | grep -E "(biome|tsconfig)" | head -20Repository: NVIDIA/NemoClaw
Length of output: 162
🏁 Script executed:
# Check the biome.json configuration
cat biome.jsonRepository: NVIDIA/NemoClaw
Length of output: 3139
🏁 Script executed:
# Check tsconfig files to see if there are any restrictions
cat tsconfig.cli.jsonRepository: NVIDIA/NemoClaw
Length of output: 628
🏁 Script executed:
# Search the codebase for any use of "import ... = require"
rg "import\s+\w+\s*=\s*require" --type ts --type jsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Check if there are any tests for the check-layer-import-boundaries script
fd check-layer-import-boundariesRepository: NVIDIA/NemoClaw
Length of output: 100
🏁 Script executed:
# Let's also check if this import form is used in the src directory at all
rg "import\s+\w+\s*=\s*require" src/ --type ts --type jsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's check the SPDX header compliance for this file
head -5 scripts/check-layer-import-boundaries.tsRepository: NVIDIA/NemoClaw
Length of output: 304
🏁 Script executed:
# Let's verify the suggested diff is syntactically correct by checking TypeScript AST types
# Create a simple test to check if ImportEqualsDeclaration and ExternalModuleReference exist
python3 << 'EOF'
# The suggestion uses:
# ts.isImportEqualsDeclaration(node)
# ts.isExternalModuleReference(node.moduleReference)
# node.moduleReference.expression
# ts.isStringLiteralLike(node.moduleReference.expression)
# These are legitimate TypeScript compiler API methods/properties
# Let's verify by searching for any existing usage patterns in TypeScript
print("Verifying TypeScript AST API for ImportEqualsDeclaration...")
print("The following are valid TypeScript compiler API:")
print("- ts.isImportEqualsDeclaration(node): Checks if node is 'import x = require(...)'")
print("- ts.isExternalModuleReference(): Checks if moduleReference is external")
print("- ts.isStringLiteralLike(): Matches string literals and template literals")
print("\nSuggested fix appears syntactically correct for TypeScript Compiler API.")
EOFRepository: NVIDIA/NemoClaw
Length of output: 470
🏁 Script executed:
# Let's check if there are any tests or comments in the file that mention ImportEqualsDeclaration
cat -n scripts/check-layer-import-boundaries.ts | grep -A 5 -B 5 -i "importequals\|import.*require"Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Check what import forms are actually used in src/lib (the directory being checked)
rg "^import\s" src/lib --type ts | head -20Repository: NVIDIA/NemoClaw
Length of output: 1611
🏁 Script executed:
# Also check for any require patterns in src/lib
rg "require\(" src/lib --type ts | head -20Repository: NVIDIA/NemoClaw
Length of output: 1538
Handle import foo = require("...") in the collector.
collectImportRefs() currently handles ES6 imports, exports, require() calls, and dynamic import(), but misses TypeScript's import x = require("...") form (ImportEqualsDeclaration). While this form is not currently used in the codebase, it remains a valid TypeScript import syntax that would bypass all boundary checks if introduced. The suggested fix is both correct and straightforward.
Suggested change
function visit(node: ts.Node): void {
if (ts.isImportDeclaration(node) && ts.isStringLiteral(node.moduleSpecifier)) {
add(node.moduleSpecifier.text, node.moduleSpecifier);
} else if (
ts.isExportDeclaration(node) &&
node.moduleSpecifier &&
ts.isStringLiteral(node.moduleSpecifier)
) {
add(node.moduleSpecifier.text, node.moduleSpecifier);
+ } else if (
+ ts.isImportEqualsDeclaration(node) &&
+ ts.isExternalModuleReference(node.moduleReference) &&
+ node.moduleReference.expression &&
+ ts.isStringLiteralLike(node.moduleReference.expression)
+ ) {
+ add(node.moduleReference.expression.text, node.moduleReference.expression);
} else if (
ts.isCallExpression(node) &&
((ts.isIdentifier(node.expression) && node.expression.text === "require") ||
node.expression.kind === ts.SyntaxKind.ImportKeyword) &&
node.arguments.length > 0 &&🤖 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-layer-import-boundaries.ts` around lines 73 - 90, The visitor
in collectImportRefs (function visit) misses TypeScript's
ImportEqualsDeclaration form; update visit to detect
ts.isImportEqualsDeclaration(node) and when node.moduleReference is an
ExternalModuleReference with a string literal expression extract that module
string and call add(moduleText, the string literal node) so import x =
require("...") references are collected like other import/require forms.
| if ( | ||
| ts.isClassDeclaration(node) && | ||
| node.heritageClauses?.some((clause) => clause.types.some(isCommandBase)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/check-layer-import-boundaries.ts | sed -n '250,270p'Repository: NVIDIA/NemoClaw
Length of output: 767
🏁 Script executed:
cat -n scripts/check-layer-import-boundaries.ts | sed -n '240,280p'Repository: NVIDIA/NemoClaw
Length of output: 1521
🏁 Script executed:
# Let's verify the TypeScript API for heritage clauses
# Check if there are examples of ts.SyntaxKind.ExtendsKeyword usage in the codebase
rg "SyntaxKind\." scripts/ -A 2 -B 2 | head -50Repository: NVIDIA/NemoClaw
Length of output: 3099
🏁 Script executed:
# Search for heritage clause usage in TypeScript API
rg "heritageClauses" scripts/ -B 3 -A 3Repository: NVIDIA/NemoClaw
Length of output: 555
🌐 Web query:
TypeScript heritage clause token extends keyword kind property
💡 Result:
Answer: In the TypeScript compiler API, the “extends”/“implements” keyword in a heritage clause is represented by the HeritageClause node’s token property, whose type is a SyntaxKind value (specifically SyntaxKind.ExtendsKeyword or SyntaxKind.ImplementsKeyword). token is not the kind property you’re asking about. Grounding from the sources: - A HeritageClause interface in the compiler API exposes a token property whose value is a SyntaxKind.ExtendsKeyword | SyntaxKind.ImplementsKeyword [1]. - The TypeScript compiler’s SyntaxKind includes ExtendsKeyword as the keyword kind for extends [2]. - A separate API surface (tsserver typings / services) for HeritageClauseSyntax similarly exposes a kind (node kind) and an extendsOrImplementsKeyword token, but that’s distinct from the heritage clause “token kind” you’re asking about in the compiler API [3]. Practical example (compiler API): - For class/interface declarations, iterate node.heritageClauses and check hc.token === ts.SyntaxKind.ExtendsKeyword to find extends clauses; then inspect hc.types for the base types [1]. If you tell me which library API you’re using (raw typescript compiler API, ts-morph, or TypeScript’s language-service node types), I can provide the exact property names/types for that surface.
Citations:
- 1: https://january.sh/posts/gentle-introduction-to-typescript-compiler-api
- 2: https://mintlify.com/Microsoft/typescript/api/types/syntax-kind
- 3: http://definitelytyped.org/docs/typescript-services--typescriptServices/classes/typescript.heritageclausesyntax.html
🏁 Script executed:
# Look for tests related to checkCommandFile or one-command-per-file
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "one-command-per-file\|checkCommandFile" 2>/dev/nullRepository: NVIDIA/NemoClaw
Length of output: 43
🏁 Script executed:
# Search for test files related to check-layer-import-boundaries
fd -e test.ts -e spec.ts | xargs grep -l "check-layer-import-boundaries\|layer.*import" 2>/dev/null | head -5Repository: NVIDIA/NemoClaw
Length of output: 96
🏁 Script executed:
cat -n test/layer-import-boundaries.test.ts | head -100Repository: NVIDIA/NemoClaw
Length of output: 1075
🏁 Script executed:
# Search for actual command files that might inherit from Command or NemoClawCommand
find . -path ./node_modules -prune -o -name "*.ts" -type f -print | xargs grep -l "extends Command\|extends NemoClawCommand\|implements Command" 2>/dev/null | head -5Repository: NVIDIA/NemoClaw
Length of output: 247
🏁 Script executed:
# Check one of the command files to see the pattern
cat -n ./src/lib/commands/credentials.ts | head -30Repository: NVIDIA/NemoClaw
Length of output: 1136
🏁 Script executed:
# Search for implements Command (case-insensitive)
rg "implements\s+.*Command" --type ts | head -20Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's look for any helper classes or interfaces that might implement or extend Command
rg "class.*implements.*|interface.*extends.*" --type ts | grep -i command | head -20Repository: NVIDIA/NemoClaw
Length of output: 172
🏁 Script executed:
# Let's verify the TypeScript API once more by checking if there's a ts.HeritageClause type definition being used
rg "HeritageClause" --type ts | head -10Repository: NVIDIA/NemoClaw
Length of output: 41
Only count extends clauses as command classes.
The current predicate treats implements Command the same as extends Command, so a helper class can satisfy the exact-one rule without actually being a registered oclif command. Filter on clause.token === ts.SyntaxKind.ExtendsKeyword before checking the base type.
Suggested change
if (
ts.isClassDeclaration(node) &&
- node.heritageClauses?.some((clause) => clause.types.some(isCommandBase))
+ node.heritageClauses?.some(
+ (clause) =>
+ clause.token === ts.SyntaxKind.ExtendsKeyword && clause.types.some(isCommandBase),
+ )
) {
commandClassCount += 1;
}📝 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 ( | |
| ts.isClassDeclaration(node) && | |
| node.heritageClauses?.some((clause) => clause.types.some(isCommandBase)) | |
| if ( | |
| ts.isClassDeclaration(node) && | |
| node.heritageClauses?.some( | |
| (clause) => | |
| clause.token === ts.SyntaxKind.ExtendsKeyword && clause.types.some(isCommandBase), | |
| ) | |
| ) { | |
| commandClassCount += 1; | |
| } |
🤖 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-layer-import-boundaries.ts` around lines 257 - 259, The
predicate that identifies command classes currently treats any heritage clause
(including implements) as matching; update the check in the class detection
logic (the ts.isClassDeclaration branch that iterates node.heritageClauses and
uses isCommandBase) to only consider extends clauses by adding a filter that
clause.token === ts.SyntaxKind.ExtendsKeyword before calling
clause.types.some(isCommandBase), so only classes that actually extend the
Command base are counted as commands.
## Summary Move sandbox-scoped workflow action modules into `src/lib/actions/sandbox/**` so command adapters and sandbox workflow orchestration have separate, visible layers. ## Stack Navigation - Position: 57 of 60 - Previous PR: [#2986 — test(cli): enforce initial layer import boundaries](#2986) - Next PR: [#2988 — refactor(cli): group global actions](#2988) ## Changes - Moved sandbox connect/destroy/rebuild/status/logs/doctor/gateway-state/process-recovery/skill/snapshot actions under `src/lib/actions/sandbox/`. - Moved sandbox policy/channel action helpers under `src/lib/actions/sandbox/policy-channel.ts`. - Updated command adapters, runtime dispatch facade, tests, and remaining callers to the new paths. - Kept behavior unchanged; this PR is a structural move plus import updates. ## 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) ## Verification - [x] `npx prek run --all-files --stage pre-push` passes - [x] `npm run build:cli` - [x] `npm run typecheck:cli` - [x] `npx tsx scripts/check-layer-import-boundaries.ts` - [x] Sandbox command adapter targeted tests pass - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced sandbox cleanup during destruction to properly unload Ollama models, terminate proxy processes, and stop host services for complete resource cleanup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
5 participants
Summary
Add a narrow layer-boundary guard for the new CLI directory structure before moving action/state/adapter modules further.
Stack Navigation
Changes
scripts/check-layer-import-boundaries.tsto validate initial import-boundary rules.Type of Change
Verification
npx prek run --all-files --stage pre-pushpassesnpx tsx scripts/check-layer-import-boundaries.tsnpx vitest run test/layer-import-boundaries.test.tsnpm run source-shape:checkmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit