test(e2e): migrate test-hermes-sandbox-secret-boundary.sh to vitest - #5607
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:
📝 WalkthroughWalkthroughAdds a new ChangesHermes Sandbox Secret-Boundary E2E Vitest Job and Free-Standing Hermes Jobs
Sequence DiagramsequenceDiagram
participant Test as liveTest()
participant Docker as Docker daemon
participant HermesImage as Hermes image
participant ManagedImage as Managed-tool image
Test->>Docker: build/reuse Hermes image
Docker->>HermesImage: build with BASE_IMAGE arg
Test->>Docker: inspect Hermes boundary
Docker->>HermesImage: run Python inspection script
HermesImage-->>Docker: exit 0 or 1
Docker-->>Test: captured exit + stdout/stderr
Test->>Docker: build/reuse managed image
Docker->>ManagedImage: build with gateway broker
Test->>Docker: inspect managed boundary
Docker->>ManagedImage: run Python inspection script
ManagedImage-->>Docker: exit 0 or 1
Test->>Docker: run startup rejection tests
Docker->>HermesImage: inject secrets via .env or --env
HermesImage-->>Docker: nemoclaw-start exits non-zero
Test->>Test: assert category mentioned, key named, no raw value printed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 |
|
Dispatched same-runner selective Vitest run for |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27982738529
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![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)
test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.ts (1)
22-141: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting shared Python helpers.
The
unquoteandenv_violationsfunctions along with their regex/constant dependencies are duplicated betweenIMAGE_INSPECTION_SCRIPTandMANAGED_TOOL_INSPECTION_SCRIPT. However, since these are self-contained scripts passed to Docker containers via stdin, consolidation would add complexity (string concatenation or shared module mounting).Keeping them duplicated but identical is acceptable for this migration—just be aware that future changes to the validation logic need to be applied in both places.
Also applies to: 143-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.ts` around lines 22 - 141, The Python validation helpers and their dependencies (the unquote function, env_violations function, and regex patterns like secret_key_re, slack_alias_re, along with constants like allowed_nonsecret_keys and allowed_literals) are duplicated between IMAGE_INSPECTION_SCRIPT and MANAGED_TOOL_INSPECTION_SCRIPT. While keeping them duplicated is acceptable for this migration since these are self-contained Docker scripts, be aware that any future modifications to the validation logic in either script must be consistently applied to both IMAGE_INSPECTION_SCRIPT and MANAGED_TOOL_INSPECTION_SCRIPT to prevent them from diverging and causing validation inconsistencies.
🤖 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 `@test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.ts`:
- Line 20: The test file has triggered a conditional complexity guardrail with 7
if statements and ternaries scoring 43 points total. Extract the
environment-dependent conditional logic into separate fixture helper functions
to keep the test code declarative. Create a new fixture file
(test/e2e-scenario/fixtures/hermes-image.ts or similar) and move the
conditionals from lines 20, 261-269, 277, 285, 314, 326, and 517 into helper
functions that return configured values or behaviors (such as a function that
returns whether to skip the test, a function that checks Docker availability and
handles CI vs. local behavior, and a function that determines image build
strategies). Then replace the inline conditionals in the test file with calls to
these helper functions, keeping test bodies focused on assertions rather than
branching logic.
---
Nitpick comments:
In `@test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.ts`:
- Around line 22-141: The Python validation helpers and their dependencies (the
unquote function, env_violations function, and regex patterns like
secret_key_re, slack_alias_re, along with constants like allowed_nonsecret_keys
and allowed_literals) are duplicated between IMAGE_INSPECTION_SCRIPT and
MANAGED_TOOL_INSPECTION_SCRIPT. While keeping them duplicated is acceptable for
this migration since these are self-contained Docker scripts, be aware that any
future modifications to the validation logic in either script must be
consistently applied to both IMAGE_INSPECTION_SCRIPT and
MANAGED_TOOL_INSPECTION_SCRIPT to prevent them from diverging and causing
validation inconsistencies.
🪄 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: 1999f028-327e-4732-9361-c5eb65bbbb25
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/hermes-sandbox-secret-boundary.test.tstest/e2e-scenario/support-tests/hermes-secret-boundary-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27983244409
|
…es-sandbox-secret-boundary # Conflicts: # tools/e2e-scenarios/workflow-boundary.mts
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27995985272
|
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 27996805089
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
1804-1810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent cleanup variable reference compared to hermes-slack-vitest.
Line 1809 uses
"${DOCKER_CONFIG:-}"while the equivalent cleanup inhermes-slack-vitest(line 1661) uses"${DOCKER_CONFIG}". Both approaches work sinceDOCKER_CONFIGis always set earlier, but standardizing on one pattern improves maintainability.🔧 Suggested standardization
Choose one pattern for both jobs. The
:-variant is slightly more defensive:- rm -rf "${DOCKER_CONFIG}" + rm -rf "${DOCKER_CONFIG:-}"Or remove
:-from discord to match slack's simpler approach (both work in practice).🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml around lines 1804 - 1810, The Docker config variable reference in the "Clean up Docker auth" step uses the parameter expansion default syntax `"${DOCKER_CONFIG:-}"` while the equivalent cleanup in the hermes-slack-vitest job uses the simpler `"${DOCKER_CONFIG}"` pattern. Standardize both cleanup blocks to use the same variable reference pattern by either removing the `:-` from the current cleanup to match the simpler approach, or adding `:-` to both for consistency.
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1804-1810: The Docker config variable reference in the "Clean up
Docker auth" step uses the parameter expansion default syntax
`"${DOCKER_CONFIG:-}"` while the equivalent cleanup in the hermes-slack-vitest
job uses the simpler `"${DOCKER_CONFIG}"` pattern. Standardize both cleanup
blocks to use the same variable reference pattern by either removing the `:-`
from the current cleanup to match the simpler approach, or adding `:-` to both
for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f971c888-2c6b-4600-b832-e913e219c85e
📒 Files selected for processing (2)
.github/workflows/e2e-vitest-scenarios.yamltools/e2e-scenarios/workflow-boundary.mts
💤 Files with no reviewable changes (1)
- tools/e2e-scenarios/workflow-boundary.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28004252023
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28005803794
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28007556800
|
wscurran
added
area: ci
3 participants
Summary
Migrate
test/e2e/test-hermes-sandbox-secret-boundary.shwith the simplest equivalent live Vitest coverage.Related Issues
Refs #5098
Assertion parity
test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.tscallsdocker infoanddocker image inspectthroughDockerProbe.expect(...).ubuntu-latest.coveredNEMOCLAW_HERMES_TEST_IMAGEis supplied.buildHermesImageIfNeeded(...)runsdocker build -f agents/hermes/Dockerfile.baseanddocker build -f agents/hermes/Dockerfile --build-arg BASE_IMAGE=....coveredNEMOCLAW_HERMES_TOOL_GATEWAY_BROKER=1and managed presets unlessNEMOCLAW_HERMES_MANAGED_TEST_IMAGEis supplied.buildManagedImageIfNeeded(...)runs the manageddocker buildwith broker and preset build args.covered/sandbox/.hermes/.envmust be a regular file, not missing or symlinked;/sandbox/.hermes/config.yamlmust exist.IMAGE_INSPECTION_SCRIPTandMANAGED_TOOL_INSPECTION_SCRIPTassert file/symlink state insidedocker run --entrypoint python3.covered.envkeys are forbidden unless allow-listed (API_SERVER_HOST,API_SERVER_PORT), blank, stripped marker, OpenShell resolver, or Slack resolver alias.exitCode === 0; violations fail with key/line output..envparsing.coveredplatform_toolsets.api_servermust exist, include Hermes remote toolsets, and not includeno_mcp.inspectImageBoundary(...)asserts the parsedapi_servertoolset includes the legacy required set and excludesno_mcp.config.yamlparsing.coveredinspectManagedToolBoundary(...)checks all required env lines and config fragments.coveredTOOL_GATEWAY_USER_TOKEN,NEMOCLAW_HERMES_TOOL_GATEWAY_REFRESH_TOKEN=, orraw-refresh-tokenin.env/config.MANAGED_TOOL_INSPECTION_SCRIPTrejects those forbidden strings in.envorconfig.yaml.covered.envraw secret entriesDEVTEST_API_TOKEN,INTERNAL_API, andOPENAI_API_KEY=sk-OPENSHELL-PROXY-REWRITE, names the key, mentions raw secret-shaped values, and does not print the raw value.expectStartupRejectsEnvFileEntry(...)runsdocker run --user sandbox --entrypoint /bin/bash ... /usr/local/bin/nemoclaw-start trueand asserts non-zero exit, message text, key, and value absence.nemoclaw-startprocess inside Hermes image.coveredDEVTEST_API_TOKENandNEMOCLAW_HERMES_TOOL_GATEWAY_REFRESH_TOKEN, names the key, mentions process environment, and does not print the raw value.expectStartupRejectsRuntimeEnvEntry(...)runsdocker run --user sandbox --env ... --entrypoint /usr/local/bin/nemoclaw-start ... trueand asserts non-zero exit, message text, key, and value absence.nemoclaw-start.coveredCleanupRegistryremoves generated images withdocker rmi -f.coveredAll legacy assertions are covered or intentionally stronger in Vitest. No row is
missing,partial, orcandidate only.Contract mapping
test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.tsassertions A1-A11./usr/local/bin/nemoclaw-startexecution.Simplicity check
nightly-e2e.yamljobhermes-secret-boundary-e2e, reusablee2e-script.yaml,runs-on: ubuntu-latest, 60-minute timeout, Docker required, no app secrets required.e2e-vitest-scenarios.yamljobhermes-sandbox-secret-boundary-vitest,runs-on: ubuntu-latest, 60-minute timeout.DockerProbe/fixtures only.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate-test-hermes-sandbox-secret-boundary -f jobs=hermes-sandbox-secret-boundary-vitest -f pr_number=5607.Pre-push parity gate
test/e2e/test-hermes-sandbox-secret-boundary.sh111100nightly-e2e.yamlhermes-secret-boundary-e2eonubuntu-latest+ Docker, replaced bye2e-vitest-scenarios.yamlhermes-sandbox-secret-boundary-vitestonubuntu-latest+ Docker.Verification
npm ci --ignore-scriptsnpm run build:cliNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/hermes-sandbox-secret-boundary.test.ts --silent=false --reporter=default(local Docker unavailable, test skipped by fixture gate)npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/hermes-secret-boundary-workflow.test.ts test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --silent=false --reporter=defaultnpm run typecheck -- --pretty falsenpm run test-size:checkgit diff --checkin_progressat dispatch)Summary by CodeRabbit
Tests
.envor process environment.Chores