test(cli): replace source-shape checks with behavior tests - #3653
Conversation
|
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:
📝 WalkthroughWalkthroughRefactors onboard to use extracted gateway liveness and destroy helpers, centralizes sandbox image-tag parsing, tightens workflow/test assertions by parsing YAML, and strengthens test-discovery heuristics to attribute production reads via function returns plus a fallback scan. ChangesGateway and sandbox refactoring
Test improvements and tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
2745-2755: Run the onboarding E2Es for this refactor.These paths now sit directly on gateway reuse/destroy and sandbox-create registration, so I'd smoke at least
sandbox-operations-e2e,openshell-gateway-upgrade-e2e, and one full onboard flow before merge.As per coding guidelines: "
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow. E2E test recommendation: ...sandbox-operations-e2e...openshell-gateway-upgrade-e2e..."Also applies to: 3641-3641, 5750-5750, 9621-9621
🤖 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/onboard.ts` around lines 2745 - 2755, The change to how gateway reuse/destroy and sandbox-create registration are wired (the call from destroyGateway to destroyGatewayWithVolumeCleanup and related symbols like destroyGateway, destroyGatewayWithVolumeCleanup, GATEWAY_NAME, hasLifecycleCommands/runCaptureOpenshell, isLinuxDockerDriverGatewayEnabled, removeDockerDriverGatewayRegistration, runOpenshell, stopDockerDriverGatewayProcess) requires running full E2E smoke tests to validate behavior; run sandbox-operations-e2e, openshell-gateway-upgrade-e2e, and a full onboard flow (including any flows touching the referenced lines at 3641, 5750, 9621) and fix any failures caused by lifecycle, registration, volume cleanup, or gateway removal edge cases uncovered by those tests.
🤖 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/find-source-shape-tests.ts`:
- Around line 130-132: The current regex that checks for
["'`]\.\.\/["'`]\s*,\s*["'`](?:\.github|agents|bin|dist|nemoclaw|nemoclaw-blueprint|scripts|src|Dockerfile(?:\.base)?|install\.sh|package\.json)["'`]
only matches literal "../" segments and misses function calls like join("..",
"src"); update the pattern in scripts/find-source-shape-tests.ts to also accept
the join-style shape by adding an alternative that matches
join\(\s*["'`]\.\.["'`]\s*,\s*["'`](?:\.github|agents|bin|dist|nemoclaw|nemoclaw-blueprint|scripts|src|Dockerfile(?:\.base)?|install\.sh|package\.json)["'`]\s*\)
(or a more general variant capturing ["'`]\.\.["'`]\s*,\s*["'`]...["'`] inside
parentheses) so functions like join("..","src") are detected by the same check.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2745-2755: The change to how gateway reuse/destroy and
sandbox-create registration are wired (the call from destroyGateway to
destroyGatewayWithVolumeCleanup and related symbols like destroyGateway,
destroyGatewayWithVolumeCleanup, GATEWAY_NAME,
hasLifecycleCommands/runCaptureOpenshell, isLinuxDockerDriverGatewayEnabled,
removeDockerDriverGatewayRegistration, runOpenshell,
stopDockerDriverGatewayProcess) requires running full E2E smoke tests to
validate behavior; run sandbox-operations-e2e, openshell-gateway-upgrade-e2e,
and a full onboard flow (including any flows touching the referenced lines at
3641, 5750, 9621) and fix any failures caused by lifecycle, registration, volume
cleanup, or gateway removal edge cases uncovered by those tests.
🪄 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: b4b689be-20bd-4dd6-a4c3-f5200f6d752d
📒 Files selected for processing (12)
ci/source-shape-test-budget.jsonscripts/find-source-shape-tests.tssrc/lib/domain/sandbox/image-tag.test.tssrc/lib/domain/sandbox/image-tag.tssrc/lib/onboard.tssrc/lib/onboard/gateway-container-running.test.tssrc/lib/onboard/gateway-container-running.tssrc/lib/onboard/gateway-destroy.test.tssrc/lib/onboard/gateway-destroy.tstest/gateway-cleanup.test.tstest/gateway-liveness-probe.test.tstest/image-cleanup.test.ts
💤 Files with no reviewable changes (2)
- test/gateway-cleanup.test.ts
- test/image-cleanup.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25977273152
|
Selective E2E Results — ✅ All requested jobs passedRun: 25977824050
|
Selective E2E Results — ✅ All requested jobs passedRun: 25978198926
|
Selective E2E Results — ✅ All requested jobs passedRun: 25978233732
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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/find-source-shape-tests.ts`:
- Around line 345-351: collectSourceFunctions currently descends into nested
functions so a return inside an inner function can mark the outer function as a
source; in visitFunctionBody add a guard that skips traversing into
function-like nodes (e.g., detect ts.isFunctionDeclaration /
ts.isFunctionExpression / ts.isArrowFunction / ts.isMethodDeclaration /
ts.isConstructorDeclaration or use ts.isFunctionLike if available) so that when
you encounter such a node you do not call ts.forEachChild on it (but still allow
checking return statements at the current level via the existing
ts.isReturnStatement logic); update visitFunctionBody to return early for nested
function-like nodes to ensure sourceReadFromExpression is only applied to
returns belonging to the outer function.
- Around line 100-102: The regex in stripStringLiterals incorrectly escapes the
character class as [\\s\\S], which matches literal backslashes and letters
instead of any character; update the pattern to use [\s\S] in the regex literal
so it matches all characters inside strings. Replace the current regex in
stripStringLiterals with one like /(['"`])(?:\\.|(?!\1)[\s\S])*\1/g (i.e.,
change [\\s\\S] → [\s\S]) so quoted string contents are properly stripped.
🪄 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: 6b781244-19b8-43fc-ac13-b871870a4350
📒 Files selected for processing (12)
scripts/find-source-shape-tests.tssrc/lib/agent/onboard.test.tssrc/lib/agent/onboard.tssrc/lib/shields/index.test.tstest/credential-exposure.test.tstest/http-proxy-fix-sync.test.tstest/onboard.test.tstest/runner.test.tstest/sandbox-init.test.tstest/sandbox-provisioning.test.tstest/security-c2-dockerfile-injection.test.tstest/security-sandbox-tar-traversal.test.ts
💤 Files with no reviewable changes (8)
- test/security-sandbox-tar-traversal.test.ts
- test/sandbox-init.test.ts
- src/lib/agent/onboard.test.ts
- test/security-c2-dockerfile-injection.test.ts
- test/sandbox-provisioning.test.ts
- test/runner.test.ts
- test/credential-exposure.test.ts
- test/onboard.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25979126286
|
|
Addressed the latest CodeRabbit scanner feedback in commit 4a699dc:
Validation after the fixes:
All passed. I did not run the recommended remote E2E smoke jobs locally; CI's E2E recommendation/check workflow is in progress on the PR. |
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/find-source-shape-tests.ts`:
- Around line 267-303: callTargetName currently returns the method name for
PropertyAccessExpression (e.g. obj.method -> "method"), which lets a call on an
object be misattributed to an unrelated top-level function; change
callTargetName to only return a name for plain Identifier expressions and return
null for PropertyAccessExpression so sourceReadFromInitializer only matches
direct function identifiers (use callTargetName to detect calls) — update
callTargetName implementation and any call-sites relying on its previous
behavior (e.g., the logic in sourceReadFromInitializer that looks up
sourceFunctions by targetName).
- Around line 706-709: The merged assertions array (variable assertions
populated from collectAssertionsInNode and fallbackLineScan) can contain
duplicates; before reporting/source_shape_assertions are generated, deduplicate
that array by a stable unique key (for example combine the assertion's
file/path, start position/line, and assertion text or an existing id) and
replace the concatenated array with the filtered unique list; update the code
around the assertions declaration (where collectAssertionsInNode(...) and
fallbackLineScan(...) are combined) to perform this dedupe so downstream
reporting uses only unique assertions.
🪄 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: 9a6cba8a-a01d-4b79-9440-2ec467ff77e2
📒 Files selected for processing (1)
scripts/find-source-shape-tests.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25979438358
|
Selective E2E Results — ✅ All requested jobs passedRun: 25979558030
|
Selective E2E Results — ✅ All requested jobs passedRun: 25981424979
|
Selective E2E Results — ✅ All requested jobs passedRun: 25999588978
|
3 participants
Summary
Replaces brittle source-text regression guards with behavior-focused tests and small extracted helpers while keeping the source-shape budget at zero. The scanner now detects repo-root source reads, helper-returned source reads,
indexOf/searchderivations,expect.unreachable, matcher-argument comparisons, and direct shell/Dockerfile source assertions that were previously missed.Changes
scripts/find-source-shape-tests.tsdetection while keepingci/source-shape-test-budget.jsonatmaxSourceShapeCases: 0.src/lib/domain/sandbox/image-tag.tswith direct behavior tests.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Tests
Refactor