Skip to content

fix: harden session ID sanitization and improve test coverage#1202

Closed
jyaunches wants to merge 14 commits intoNVIDIA:mainfrom
jyaunches:fix/telegram-bridge-injection-v2
Closed

fix: harden session ID sanitization and improve test coverage#1202
jyaunches wants to merge 14 commits intoNVIDIA:mainfrom
jyaunches:fix/telegram-bridge-injection-v2

Conversation

@jyaunches
Copy link
Copy Markdown
Contributor

@jyaunches jyaunches commented Mar 31, 2026

Summary

This PR addresses review feedback from the telegram bridge command injection fix, adding additional security hardening and comprehensive test coverage.

Security Fixes

Leading Hyphen Stripping in sanitizeSessionId()

Session IDs like --help or -v could be interpreted as command-line flags by nemoclaw-start. This fix strips leading hyphens to prevent option injection attacks.

// Before: -123456789 (negative Telegram chat ID) → -123456789 (option injection risk)
// After:  -123456789 → 123456789 (safe)

Documentation

Added security rationale explaining why session ID validation differs from SANDBOX_NAME validation (session IDs come from Telegram chat IDs which can be negative numbers).

Testability Improvements

  • Added exitOnError option to initConfig() for testing without process.exit()
  • initConfig() now returns the configuration object for verification
  • Added JSDoc documentation for all initConfig options

New Tests

Unit Tests (test/telegram-bridge.test.js)

Category Tests Added
Option injection prevention 5 tests (--help, -v, leading hyphens, negative chat IDs)
initConfig validation 4 tests (returns config, missing token/apiKey, invalid sandbox)
Edge cases 5 tests (empty, special chars, unicode, very long IDs, only hyphens)
Multi-line messages 1 test (source code pattern verification)
Mocked spawn verification 3 tests (stdin/stdout patterns, cleanup handlers)

E2E Tests (test/e2e/test-telegram-injection.sh)

Phase Tests
Phase 8: Multi-line Messages T12: injection on line 2, T13: newlines + backticks
Phase 9: Session ID Option Injection T14: leading hyphen stripping, T15/T15b: real code path
Phase 10: Empty/Edge Cases T16: empty session ID, T17: special chars only
Phase 11: Cleanup Removes all /tmp/injection-proof-* marker files

Test Results

Test Files:  40 passed | 1 skipped (41)
Tests:       712 passed | 3 skipped (715)

Files Changed

File Changes
scripts/telegram-bridge.js +53 / -13
test/telegram-bridge.test.js +284 / -1
test/e2e/test-telegram-injection.sh +237 / -0

Related

Summary by CodeRabbit

  • Bug Fixes

    • Closed Telegram bridge command- and shell-injection vectors with stricter input validation, session‑ID sanitization, secure secret handling, safer remote invocation, temp-file hardening, and a 4096-character message limit.
  • Tests

    • Added broad unit, integration, and E2E coverage, including real-path invocation checks, injection probes, temp-file cleanup, and edge-case validation tests.
  • Chores

    • Updated CI workflow inputs/token handling, relaxed fork-run restrictions, and bumped the CLI installer version.

Re-specification for PR NVIDIA#119 which has gone stale.
Fixes NVIDIA#118
…sing

- Pass user messages and API key via stdin instead of shell interpolation
- Add message length validation (4096 char limit)
- Use execFileSync for ssh-config call (no shell interpretation)
- Use cryptographically random temp file paths (prevent symlink race)
- Export functions for testing (runAgentInSandbox, sanitizeSessionId)
- Refactor config initialization for testability

Fixes NVIDIA#118
See: PR NVIDIA#119, PR NVIDIA#320 (upstream)
- Add test/telegram-bridge.test.js with 18 unit tests for:
  - sanitizeSessionId validation
  - MAX_MESSAGE_LENGTH constant
  - Source code security patterns (execFileSync, mkdtempSync, stdin, etc.)

- Update test/e2e/test-telegram-injection.sh Phase 7 to:
  - Actually invoke runAgentInSandbox() via Node.js test harness
  - Test injection payloads through production code path
  - Verify API key not in process arguments

Addresses PR NVIDIA#1092 feedback from @cv about tests bypassing real code paths.
- Update Brev CLI from v0.6.310 to v0.6.322
- Replace broken --cpu flag with 'brev search cpu | brev create' pattern
- Add brev_token input to workflow for manual token override
- Replace BREV_CPU env var with BREV_MIN_VCPU and BREV_MIN_RAM
- Increase SSH wait timeout from 5 min to 7.5 min for slower providers

The --cpu flag in Brev CLI v0.6.310 was silently ignored, always creating
GPU instances. The new pattern uses 'brev search cpu' to find CPU-only
instance types and pipes them to 'brev create' which tries them in order.
… + brev-setup.sh

The startup script from OpenShell-Community was not running reliably on
Brev instances, causing 40-minute timeouts waiting for a log file that
was never created.

Simplify to:
- brev search cpu | brev create (bare instance, no startup script)
- rsync code to remote
- run brev-setup.sh directly for bootstrap

This is more reliable and easier to debug since all setup happens
via SSH commands we control.
Security fixes:
- Strip leading hyphens in sanitizeSessionId() to prevent option injection
  (e.g., '--help', '-v' could be interpreted as flags by nemoclaw-start)
- Document security rationale for session ID vs SANDBOX_NAME validation differences

Testability improvements:
- Add exitOnError option to initConfig() for testing without process.exit()
- Return configuration object from initConfig() for verification
- Add JSDoc documentation for all initConfig options

New unit tests (test/telegram-bridge.test.js):
- Option injection prevention (leading hyphens, --help, -v, negative chat IDs)
- initConfig validation (returns config, missing token/apiKey, invalid sandbox)
- Edge cases (empty, special chars, unicode, very long IDs, only hyphens)
- Multi-line message handling verification
- Mocked spawn stdin/stdout pattern verification

New E2E tests (test/e2e/test-telegram-injection.sh):
- Phase 8: Multi-line message injection (T12, T13)
- Phase 9: Session ID option injection prevention (T14, T15, T15b)
- Phase 10: Empty and edge case handling (T16, T17)
- Phase 11: Cleanup of injection marker files

All 712 tests pass.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 31, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Deferred Telegram bridge init into testable exports; added sessionId sanitization and 4096-char message cap; switched remote invocation to stdin-based SSH to eliminate shell interpolation; hardened temp-file and exec usage; added extensive specs, unit and e2e tests; adjusted CI workflow inputs and Brev CLI pinning to allow forked e2e runs.

Changes

Cohort / File(s) Summary
CI/CD Workflow
.github/workflows/e2e-brev.yaml
Removed use_launchable inputs, added optional brev_token workflow_dispatch input, stopped exporting USE_LAUNCHABLE, commented-out repo restriction to allow forks, and bumped Brev CLI pin from v0.6.310 → v0.6.322+.
Security Specifications & Tests Plan
.specs/telegram-bridge-command-injection-fix/spec.md, .specs/.../tests.md, .specs/.../validation.md
Added multi‑phase remediation, validation and test plans covering strict SANDBOX_NAME regex, sanitizeSessionId(), 4096-char limit, stdin-based remote protocol, temp-file hardening, execFileSync usage, acceptance criteria and rollout/rollback guidance.
Telegram Bridge Core Script
scripts/telegram-bridge.js
Deferred env init into initConfig(), exported initConfig, sanitizeSessionId, runAgentInSandbox, and MAX_MESSAGE_LENGTH; gated main() behind require.main === module; runAgentInSandbox() sanitizes sessionId, rejects empty sanitized IDs, uses execFileSync for SSH config, sends API key+message via SSH stdin, and improves temp-file cleanup and error handlers.
E2E Provisioning / Runner
test/e2e/brev-e2e.test.js
Removed launchable/legacy branching and USE_LAUNCHABLE; introduced BREV_MIN_VCPU/BREV_MIN_RAM with unified `brev search cpu
E2E Injection Scenarios
test/e2e/test-telegram-injection.sh
Added Phases 7–11 running the real runAgentInSandbox() via a temporary Node harness; added T9–T17 checks for command substitution/backticks, multi-line payloads, process-arg leak checks for API key, sanitizeSessionId() edge cases, and expanded cleanup.
Unit Tests
test/telegram-bridge.test.js
New Vitest suite validating sanitizeSessionId() behavior, MAX_MESSAGE_LENGTH, initConfig() mapping and error paths, static/source-pattern checks (no unsafe interpolated exec, use of execFileSync, mkdtempSync, restrictive temp modes, stdin-based protocol), and multiple edge-case assertions.

Sequence Diagram

sequenceDiagram
    participant TG as Telegram API
    participant Bridge as Telegram Bridge
    participant SSH as SSH/RemoteHost
    participant Sandbox as Brev Sandbox
    participant Agent as NemoClaw Agent

    TG->>Bridge: Message + sessionId
    activate Bridge
    Bridge->>Bridge: Validate & sanitize inputs
    Note over Bridge: Check message length (≤4096)\nSanitize sessionId (no leading -)
    Bridge->>SSH: Open SSH connection, send stdin payload
    Note over Bridge,SSH: stdin:\nLine1: API_KEY\nLines2+: Message
    activate SSH
    SSH->>SSH: Read stdin lines
    SSH->>SSH: Export line1 → NVIDIA_API_KEY
    SSH->>Sandbox: Execute nemoclaw-start using exported env and message
    activate Sandbox
    Sandbox->>Agent: Invoke agent with message
    activate Agent
    Agent->>Agent: Process request
    deactivate Agent
    Sandbox-->>SSH: Return result
    deactivate Sandbox
    SSH-->>Bridge: SSH stdout result
    deactivate SSH
    Bridge->>TG: Send reply message
    deactivate Bridge
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through scripts to chase each bug,
Hid keys in stdin, tightened each snug rug,
Stripped naughty names and capped messages long,
Temp holes sealed tight, no secrets belong—
A little rabbit patch, secure and smug.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: hardening session ID sanitization and improving test coverage for the Telegram bridge security fix.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/telegram-bridge-injection-v2

Comment @coderabbitai help to get the list of available commands and usage tips.

The stdin-based secret passing via 'eval "$(cat)"' was interfering
with SSH sessions that need interactive stdin (like NodeSource setup
scripts that use sudo). Write secrets to a temp file on the remote
machine instead, then source it before running commands.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
scripts/telegram-bridge.js (1)

198-204: Consider logging when session ID is rejected.

When sanitizeSessionId returns null and the function resolves with "Error: Invalid session ID", there's no logging to help diagnose issues. Consider adding a debug log for operational visibility.

Add debug logging for rejected session IDs
     // Sanitize session ID - reject if empty after sanitization
     const safeSessionId = sanitizeSessionId(sessionId);
     if (!safeSessionId) {
+      console.log(`[debug] Rejected session ID: ${String(sessionId).slice(0, 50)}`);
       resolve("Error: Invalid session ID");
       return;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/telegram-bridge.js` around lines 198 - 204, The code resolves with
"Error: Invalid session ID" when sanitizeSessionId(sessionId) returns null but
doesn't log anything; update the Promise block around
sanitizeSessionId/safeSessionId to log a debug/warn message (e.g., using the
module's logger or console.debug) that includes the original sessionId and the
fact it was rejected before calling resolve in the error branch; reference the
sanitizeSessionId call and the safeSessionId variable inside the function that
returns the Promise so you can add the logging immediately before
resolve("Error: Invalid session ID").
.github/workflows/e2e-brev.yaml (1)

54-57: Sensitive token input — ensure it's not logged.

The new brev_token input allows passing a Brev refresh token via workflow dispatch. Since this is a sensitive credential, verify that GitHub Actions masks it appropriately (string inputs are not automatically masked like secrets).

Consider documenting that users should use the secret instead of this input when possible, or mark this input as sensitive if GitHub Actions supports that in the future.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/e2e-brev.yaml around lines 54 - 57, The workflow exposes a
sensitive credential via the brev_token input and may inadvertently log it;
remove the default value and update the brev_token input description to
explicitly state "sensitive — prefer using the BREV_API_TOKEN secret", then
ensure any step that consumes github.event.inputs.brev_token assigns it to an
environment variable (e.g., BREV_TOKEN) without printing or echoing it and
prefers secrets fallback (e.g., BREV_TOKEN: ${{ github.event.inputs.brev_token
|| secrets.BREV_API_TOKEN }}); also audit any uses of brev_token in steps
(checks for occurrences of github.event.inputs.brev_token, brev_token) and
remove any debug/echo/::debug calls that could expose it.
.specs/telegram-bridge-command-injection-fix/validation.md (1)

1-4: Missing SPDX license header.

Per coding guidelines, Markdown files must include the SPDX license header using HTML comments at the top of the file.

Add SPDX header
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
 # Validation Plan: Telegram Bridge Command Injection Fix

 Generated from: `.specs/telegram-bridge-command-injection-fix/spec.md`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.specs/telegram-bridge-command-injection-fix/validation.md around lines 1 -
4, Add the missing SPDX license header to the top of
.specs/telegram-bridge-command-injection-fix/validation.md by inserting the
required HTML comment SPDX line (e.g., <!-- SPDX-License-Identifier: MIT --> or
the project’s chosen identifier) as the first non-whitespace content in the file
so the header appears before the "# Validation Plan" title; ensure the exact
SPDX identifier used across the repo is applied and there are no extra
characters or blank lines before the comment.
.specs/telegram-bridge-command-injection-fix/spec.md (1)

1-4: Missing SPDX license header.

Per coding guidelines, Markdown files must include the SPDX license header using HTML comments.

Add SPDX header
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
 # Spec: Fix Command Injection in Telegram Bridge

 ## Problem Statement
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.specs/telegram-bridge-command-injection-fix/spec.md around lines 1 - 4, Add
the missing SPDX license header as an HTML comment at the very top of the
Markdown spec (before the heading "# Spec: Fix Command Injection in Telegram
Bridge"); insert the appropriate SPDX identifier per project guidelines (e.g.,
the project's license ID) so the file contains a valid SPDX license comment
header.
.specs/telegram-bridge-command-injection-fix/tests.md (1)

1-4: Missing SPDX license header.

Per coding guidelines, Markdown files must include the SPDX license header using HTML comments.

Add SPDX header
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
 # Test Specification: Telegram Bridge Command Injection Fix

 This test guide supports TDD implementation of the command injection fix for `scripts/telegram-bridge.js`.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.specs/telegram-bridge-command-injection-fix/tests.md around lines 1 - 4,
Add the SPDX license header as an HTML comment at the very top of the "Test
Specification: Telegram Bridge Command Injection Fix" Markdown file (tests.md);
insert a single-line SPDX identifier comment (e.g. <!-- SPDX-License-Identifier:
MIT --> or the project's agreed identifier) before any other content so the file
complies with the coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/e2e/test-telegram-injection.sh`:
- Around line 597-617: The test references a deleted test runner file named
test-real-bridge.js, causing T12 to fail; either restore/recreate
test-real-bridge.js to accept the same CLI args (payload and tag
'e2e-multiline-t12') and produce the same behavior used by this test, or update
the test invocation to call the new/renamed test runner currently in the repo;
ensure the recreated/used script honors the MULTILINE_PAYLOAD input and exits in
the same way so the sandbox_exec file-check logic for /tmp/injection-proof-t12
continues to work.
- Around line 586-588: The test harness file is being deleted prematurely by the
cleanup command "rm -f /tmp/test-real-bridge.js" before Phase 8 (T12) that
depends on it; move that rm command out of the current location and add it to
the existing cleanup block in Phase 11 so the harness is only removed after all
tests (including T12) have run, ensuring the Phase 11 cleanup block (where
partial cleanup already exists) performs the final removal.

---

Nitpick comments:
In @.github/workflows/e2e-brev.yaml:
- Around line 54-57: The workflow exposes a sensitive credential via the
brev_token input and may inadvertently log it; remove the default value and
update the brev_token input description to explicitly state "sensitive — prefer
using the BREV_API_TOKEN secret", then ensure any step that consumes
github.event.inputs.brev_token assigns it to an environment variable (e.g.,
BREV_TOKEN) without printing or echoing it and prefers secrets fallback (e.g.,
BREV_TOKEN: ${{ github.event.inputs.brev_token || secrets.BREV_API_TOKEN }});
also audit any uses of brev_token in steps (checks for occurrences of
github.event.inputs.brev_token, brev_token) and remove any debug/echo/::debug
calls that could expose it.

In @.specs/telegram-bridge-command-injection-fix/spec.md:
- Around line 1-4: Add the missing SPDX license header as an HTML comment at the
very top of the Markdown spec (before the heading "# Spec: Fix Command Injection
in Telegram Bridge"); insert the appropriate SPDX identifier per project
guidelines (e.g., the project's license ID) so the file contains a valid SPDX
license comment header.

In @.specs/telegram-bridge-command-injection-fix/tests.md:
- Around line 1-4: Add the SPDX license header as an HTML comment at the very
top of the "Test Specification: Telegram Bridge Command Injection Fix" Markdown
file (tests.md); insert a single-line SPDX identifier comment (e.g. <!--
SPDX-License-Identifier: MIT --> or the project's agreed identifier) before any
other content so the file complies with the coding guidelines.

In @.specs/telegram-bridge-command-injection-fix/validation.md:
- Around line 1-4: Add the missing SPDX license header to the top of
.specs/telegram-bridge-command-injection-fix/validation.md by inserting the
required HTML comment SPDX line (e.g., <!-- SPDX-License-Identifier: MIT --> or
the project’s chosen identifier) as the first non-whitespace content in the file
so the header appears before the "# Validation Plan" title; ensure the exact
SPDX identifier used across the repo is applied and there are no extra
characters or blank lines before the comment.

In `@scripts/telegram-bridge.js`:
- Around line 198-204: The code resolves with "Error: Invalid session ID" when
sanitizeSessionId(sessionId) returns null but doesn't log anything; update the
Promise block around sanitizeSessionId/safeSessionId to log a debug/warn message
(e.g., using the module's logger or console.debug) that includes the original
sessionId and the fact it was rejected before calling resolve in the error
branch; reference the sanitizeSessionId call and the safeSessionId variable
inside the function that returns the Promise so you can add the logging
immediately before resolve("Error: Invalid session ID").
🪄 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: Pro

Run ID: 0b06a18f-84be-4155-8ba1-57d3dd61b5fe

📥 Commits

Reviewing files that changed from the base of the PR and between 82789a6 and 25a846e.

📒 Files selected for processing (8)
  • .github/workflows/e2e-brev.yaml
  • .specs/telegram-bridge-command-injection-fix/spec.md
  • .specs/telegram-bridge-command-injection-fix/tests.md
  • .specs/telegram-bridge-command-injection-fix/validation.md
  • scripts/telegram-bridge.js
  • test/e2e/brev-e2e.test.js
  • test/e2e/test-telegram-injection.sh
  • test/telegram-bridge.test.js

Comment on lines +586 to +588
# Clean up test script
rm -f /tmp/test-real-bridge.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Test harness deleted before it's used in Phase 8.

The test script /tmp/test-real-bridge.js is deleted at line 587, but Phase 8 (T12) at lines 605-609 attempts to use it. This will cause T12 to fail with "file not found".

Move cleanup to after all tests that use the script
-# Clean up test script
-rm -f /tmp/test-real-bridge.js

 # ══════════════════════════════════════════════════════════════════
 # Phase 8: Multi-line Message Handling
 # ══════════════════════════════════════════════════════════════════

Then move the cleanup to Phase 11 (which already has partial cleanup at lines 821-822).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/test-telegram-injection.sh` around lines 586 - 588, The test harness
file is being deleted prematurely by the cleanup command "rm -f
/tmp/test-real-bridge.js" before Phase 8 (T12) that depends on it; move that rm
command out of the current location and add it to the existing cleanup block in
Phase 11 so the harness is only removed after all tests (including T12) have
run, ensuring the Phase 11 cleanup block (where partial cleanup already exists)
performs the final removal.

Comment on lines +597 to +617
if [ -z "$OPENSHELL_PATH" ]; then
skip "T12: openshell not found, cannot test multi-line messages"
else
sandbox_exec "rm -f /tmp/injection-proof-t12" >/dev/null 2>&1

# Multi-line message with injection attempt on line 2
MULTILINE_PAYLOAD=$'Hello\n$(touch /tmp/injection-proof-t12)\nWorld'

t12_result=$(cd "$REPO" && OPENSHELL_PATH="$OPENSHELL_PATH" \
NEMOCLAW_SANDBOX_NAME="$SANDBOX_NAME" \
timeout 120 node /tmp/test-real-bridge.js \
"$MULTILINE_PAYLOAD" \
'e2e-multiline-t12' 2>&1) || true

injection_check_t12=$(sandbox_exec "test -f /tmp/injection-proof-t12 && echo EXPLOITED || echo SAFE")
if echo "$injection_check_t12" | grep -q "SAFE"; then
pass "T12: Multi-line message injection was NOT executed"
else
fail "T12: Multi-line message injection was EXECUTED — vulnerability!"
fi
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

T12 will fail due to missing test script (see earlier comment).

This test references /tmp/test-real-bridge.js which was deleted at line 587. Additionally, ensure the test script is recreated or preserved for this phase.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/test-telegram-injection.sh` around lines 597 - 617, The test
references a deleted test runner file named test-real-bridge.js, causing T12 to
fail; either restore/recreate test-real-bridge.js to accept the same CLI args
(payload and tag 'e2e-multiline-t12') and produce the same behavior used by this
test, or update the test invocation to call the new/renamed test runner
currently in the repo; ensure the recreated/used script honors the
MULTILINE_PAYLOAD input and exits in the same way so the sandbox_exec file-check
logic for /tmp/injection-proof-t12 continues to work.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
test/e2e/brev-e2e.test.js (2)

30-32: Consider validating parsed integer values.

If BREV_MIN_VCPU or BREV_MIN_RAM contain non-numeric values (e.g., "abc"), parseInt returns NaN, which would produce a malformed command at line 161. While CI typically controls these values, adding a guard would prevent confusing errors.

Suggested guard
 // CPU instance specs: min vCPUs and RAM for the instance search
-const BREV_MIN_VCPU = parseInt(process.env.BREV_MIN_VCPU || "4", 10);
-const BREV_MIN_RAM = parseInt(process.env.BREV_MIN_RAM || "16", 10);
+const BREV_MIN_VCPU = parseInt(process.env.BREV_MIN_VCPU || "4", 10) || 4;
+const BREV_MIN_RAM = parseInt(process.env.BREV_MIN_RAM || "16", 10) || 16;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/brev-e2e.test.js` around lines 30 - 32, The parsed constants
BREV_MIN_VCPU and BREV_MIN_RAM can become NaN if env vars are non-numeric; after
the parseInt calls check Number.isFinite or isNaN and either fallback to safe
defaults (e.g., 4 and 16) or throw a clear error; update the initialization that
uses parseInt to validate and coerce to an integer (e.g., use
Math.floor/Number.isFinite check) and ensure any log or thrown message
references the offending env var names so the failing command that uses
BREV_MIN_VCPU/BREV_MIN_RAM will not be built with NaN.

157-164: Consider exposing BREV_MIN_VCPU and BREV_MIN_RAM as workflow inputs if CI runs should support variable instance sizing.

The test code reads these from environment variables with defaults (4 vCPUs / 16 GB RAM), but the workflow at .github/workflows/e2e-brev.yaml does not expose them as inputs or in the env: block. This means CI runs will always use the hardcoded defaults. If this is intentional, no change needed; otherwise, consider adding them as workflow dispatch inputs to allow operators to override instance sizing for specific runs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/brev-e2e.test.js` around lines 157 - 164, The test uses environment
vars BREV_MIN_VCPU and BREV_MIN_RAM with defaults, but the CI workflow doesn't
expose them; update the workflow to add workflow inputs (e.g., min_vcpu,
min_ram) and wire those inputs into the job env so BREV_MIN_VCPU and
BREV_MIN_RAM are set for the test run (preserve current defaults when inputs are
absent); ensure the e2e job's environment variables map the inputs to
BREV_MIN_VCPU and BREV_MIN_RAM so the test's execSync call picks up overridden
sizes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 30-32: The parsed constants BREV_MIN_VCPU and BREV_MIN_RAM can
become NaN if env vars are non-numeric; after the parseInt calls check
Number.isFinite or isNaN and either fallback to safe defaults (e.g., 4 and 16)
or throw a clear error; update the initialization that uses parseInt to validate
and coerce to an integer (e.g., use Math.floor/Number.isFinite check) and ensure
any log or thrown message references the offending env var names so the failing
command that uses BREV_MIN_VCPU/BREV_MIN_RAM will not be built with NaN.
- Around line 157-164: The test uses environment vars BREV_MIN_VCPU and
BREV_MIN_RAM with defaults, but the CI workflow doesn't expose them; update the
workflow to add workflow inputs (e.g., min_vcpu, min_ram) and wire those inputs
into the job env so BREV_MIN_VCPU and BREV_MIN_RAM are set for the test run
(preserve current defaults when inputs are absent); ensure the e2e job's
environment variables map the inputs to BREV_MIN_VCPU and BREV_MIN_RAM so the
test's execSync call picks up overridden sizes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b61a5bb1-ccfe-4491-9122-7e820cd95de5

📥 Commits

Reviewing files that changed from the base of the PR and between 25a846e and 03a7f6e.

📒 Files selected for processing (1)
  • test/e2e/brev-e2e.test.js

Replace overengineered sshWithSecrets() stdin piping and temp file
approaches with simple env var exports in the SSH command. This is
an ephemeral CI VM that gets destroyed after tests.
@jyaunches
Copy link
Copy Markdown
Contributor Author

Closing in favor of cleaner PRs: #1205 (Brev E2E) and a new PR for just the telegram bridge fix.

@jyaunches jyaunches closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant