test(e2e): migrate Telegram injection to Vitest#5576
Merged
Conversation
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a live Vitest e2e scenario ( ChangesTelegram Injection Live E2E Scenario
Sequence Diagram(s)sequenceDiagram
participant Test as telegram-injection.test
participant Host as HostCliClient
participant Sandbox as OpenShell Sandbox
Test->>Host: precleanSandbox
Test->>Host: installSandbox (or skip on rate-limit)
Host->>Host: expectSandboxReady (poll)
Test->>Host: dockerInfo (assert available)
loop metacharacter payloads (${}, ``, quote)
Test->>Host: sendPayloadViaSandboxStdin(payload)
Host->>Host: openshellStdinCommand(base64(payload))
Host->>Sandbox: execShell with stdin pipeline
Sandbox->>Sandbox: base64 decode + execute
Sandbox-->>Test: assert marker file absent (SAFE)
end
loop SSH variant for same payloads
Test->>Host: sendPayloadViaOpenShellSshStdin(payload)
Host->>Host: openshellSshStdinCommand(base64(payload))
Host->>Host: ssh via sandbox ssh-config
Sandbox->>Sandbox: base64 decode + execute
Sandbox-->>Test: assert SSH marker file absent (SAFE)
end
Test->>Host: assertParameterPayloadStaysLiteral (exec path)
Sandbox-->>Test: assert ${NVIDIA_INFERENCE_API_KEY} not expanded
Test->>Host: assertSshParameterPayloadStaysLiteral (SSH path)
Sandbox-->>Test: assert ${NVIDIA_INFERENCE_API_KEY} not expanded
Test->>Host: assertHostProcessTableDoesNotExposeSecret
Host-->>Test: assert key prefix not in ps aux
Test->>Sandbox: assertSandboxProcessTableDoesNotExposeSecret
Sandbox-->>Test: assert key prefix not in ps aux
Test->>Sandbox: validateName (invalid names loop)
Sandbox-->>Test: assert all rejected
Test->>Host: sendPayloadViaSandboxStdin (benign + special chars)
Sandbox-->>Test: assert passthrough success
Test->>Host: CLI status (best-effort)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 46%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
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
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 1 in-scope improvement
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
4080-4093: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInconsistent Docker login retry logic; align with channels-add-remove-vitest pattern.
The
telegram-injection-vitestjob uses a simplified Docker login (line 4093:|| echo "::warning::..."), while thechannels-add-remove-vitestjob (lines 3972-3993) implements a robust 3-attempt retry loop with backoff. For consistency and reliability, adopt the retry pattern here as well.♻️ Adopt the retry-loop pattern from channels-add-remove-vitest
- name: Authenticate to Docker Hub env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} shell: bash run: | set -euo pipefail if [[ -z "${DOCKERHUB_USERNAME}" || -z "${DOCKERHUB_TOKEN}" ]]; then echo "::notice::Docker Hub credentials not configured; continuing with anonymous pulls." exit 0 fi mkdir -p "${DOCKER_CONFIG}" chmod 700 "${DOCKER_CONFIG}" - echo "${DOCKERHUB_TOKEN}" | timeout 30s docker login docker.io --username "${DOCKERHUB_USERNAME}" --password-stdin || echo "::warning::Docker Hub login failed; continuing with anonymous pulls." + login_succeeded=0 + for attempt in 1 2 3; do + if echo "${DOCKERHUB_TOKEN}" | timeout 30s docker login docker.io --username "${DOCKERHUB_USERNAME}" --password-stdin; then + login_succeeded=1 + break + fi + if [[ "$attempt" -lt 3 ]]; then + echo "::warning::Docker Hub login attempt ${attempt} failed; retrying." + sleep 5 + fi + done + if [[ "$login_succeeded" -ne 1 ]]; then + echo "::warning::Docker Hub login failed after 3 attempts; continuing with anonymous pulls." + fi🤖 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 4080 - 4093, The Docker login command in the "Authenticate to Docker Hub" step uses a simple error handling approach with a single warning message, but the channels-add-remove-vitest job implements a more robust retry pattern with exponential backoff. Replace the current one-line Docker login with the docker login --retry logic and 3-attempt retry loop pattern from the channels-add-remove-vitest job (lines 3972-3993) to ensure consistent reliability across both jobs. Apply the same retry loop structure around the docker login docker.io command to handle transient failures gracefully.
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 4107-4117: The "Run Telegram injection live test" step is missing
required OpenShell installation and PATH configuration. Add a new "Install
OpenShell" step before the test step that runs scripts/install-openshell.sh with
NEMOCLAW_NON_INTERACTIVE set to "1", then update the "Run Telegram injection
live test" step to export PATH with the necessary bin directories, verify the
openshell binary location by checking if command -v finds it or if it exists at
$HOME/.local/bin/openshell, export OPENSHELL_BIN with the resolved binary path,
and run openshell --version to confirm installation before executing the vitest
command for telegram-injection.test.ts.
In `@test/e2e-scenario/live/telegram-injection.test.ts`:
- Line 177: The test includes "UPPERCASE" in the invalidNames array, but the
validateName regex pattern at test/e2e-scenario/fixtures/clients/sandbox.ts
explicitly allows uppercase letters in the A-Z character class. Remove
"UPPERCASE" from the invalidNames array in the telegram-injection.test.ts file
since it will actually pass validation, not fail it.
- Around line 88-99: The test body contains a conditional if statement checking
for NVIDIA_ENDPOINT_RATE_LIMIT which violates the linear test structure
guardrail. Extract the rate-limit error handling logic from the test into a new
helper function (e.g., installSandboxOrSkipOnRateLimit in the test helpers file)
that wraps the installSandbox call with the try-catch and rate-limit check,
accepts the skip function as a parameter, and returns the result or triggers
skip internally. Then replace the entire try-catch block in the test with a
single call to this new helper function, keeping the test body free of
conditional logic.
- Around line 30-32: Remove the duplicate base64 function definition from the
test file (the function that converts a string value to base64 encoding using
Buffer.from and toString). Instead, import the base64 function from the
phase6-messaging-helpers module at the top of the file alongside other imports,
then delete the local function definition. Any existing calls to base64 within
this test file will automatically use the shared implementation from
phase6-messaging-helpers.
---
Nitpick comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 4080-4093: The Docker login command in the "Authenticate to Docker
Hub" step uses a simple error handling approach with a single warning message,
but the channels-add-remove-vitest job implements a more robust retry pattern
with exponential backoff. Replace the current one-line Docker login with the
docker login --retry logic and 3-attempt retry loop pattern from the
channels-add-remove-vitest job (lines 3972-3993) to ensure consistent
reliability across both jobs. Apply the same retry loop structure around the
docker login docker.io command to handle transient failures gracefully.
🪄 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: 1884fea5-c762-46a1-8cf7-ad6b16b00679
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/phase6-messaging-helpers.tstest/e2e-scenario/live/telegram-injection.test.ts
Collaborator
Author
|
Addressed review feedback in d45654f/d5d1d2f64:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27928266806
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27962694052
|
jyaunches
approved these changes
Jun 22, 2026
jyaunches
left a comment
jyaunches
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the follow-up at c4931918.
This addresses the earlier coverage concerns:
- added Vitest coverage for the prior bash-test OpenShell SSH route via
openshell sandbox ssh-config+ssh -F, alongside the existingopenshell sandbox execchecks; - restored uppercase sandbox-name rejection coverage and fixed the
--helpargument handling so invalid-name cases reachvalidateName; - added a workflow-boundary validator for
telegram-injection-vitestto keep Docker auth in ``, scopeNVIDIA_INFERENCE_API_KEYto the live test step, and keep artifact upload narrowed to `e2e-artifacts/vitest/telegram-injection/`.
Validation I checked:
- PR Review Advisor reran on
c4931918and reportsmerge_as_is/ no blocking findings. - Required
telegram-injection-vitestpassed in run 27962694052. - Local checks before push: Biome lint, typecheck, build:cli, workflow-boundary validator, and diff whitespace check.
Approving.
This was referenced Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the Telegram injection E2E contract from the legacy bash entry point into the live Vitest E2E system. The replacement keeps the real install/OpenShell sandbox boundary for shell metacharacter payloads, process-table leak checks, and sandbox name validation.
Related Issue
Refs #5098
Changes
test/e2e-scenario/live/telegram-injection.test.tsas Vitest coverage fortest/e2e/test-telegram-injection.sh.test/e2e-scenario/live/phase6-messaging-helpers.tsfor shared Phase 6 install, cleanup, env, registry, and sandbox helpers.telegram-injection-vitestjob into.github/workflows/e2e-vitest-scenarios.yaml.Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Tests
CI/CD