test(e2e): comprehensive REST API test coverage

[G4614]

Expands the REST-path e2e suite with exec, file-I/O, lifecycle, real-world, and Node napi-rs cases (Node comprehensive: 7 → 31), plus the API→Runner chain-proof test.

Test plan:

• E2E dev run (green): 149 passed, 3 skipped, 4 xfailed, 0 failed — run 28659513452
• Node driver output visible per case in the log (BOX_ID / marker / OK)
• test_exec_timeout.py (2 cases): parked in the workflow --ignore list — depends on the guest fix in fix(guest): kill exec process group on timeout so orphans don't hang the exec #910; un-ignore once that lands

Notes:

• Stacked on ci(deploy): isolate non-release runner runtime caches #724 (chore/e2e-required-merge-gate), which owns e2e-dev.yml; this PR is test cases only. Node per-case output needs both ci(deploy): isolate non-release runner runtime caches #724's -s and this PR's driver echo.
• Green is with the 2 fix(guest): kill exec process group on timeout so orphans don't hang the exec #910-dependent exec-timeout cases and 4 strict-xfail known bugs excluded — it does not claim exec timeout is covered.
• Intentionally excludes snapshot / cloneBox / export / importBox / metrics / getOrCreate from Node — the Python e2e suite doesn't exercise them either (no evidence they work over REST).

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds new end-to-end pytest modules covering CLI subprocess behavior, REST exec edge cases, file I/O, lifecycle/box-management, real-world scenarios, and a Node SDK comprehensive test driver plus wrapper. Also adds a new GitHub Actions E2E workflow and a small helper swap in an existing path-verification test.

Changes

E2E comprehensive test coverage

Layer / File(s)	Summary
E2E workflow setup .github/workflows/e2e-dev.yml	New CI workflow builds CLI/Node/Python SDKs, prepares credentials, runs the e2e pytest suite, and uploads test results.
CLI comprehensive test suite scripts/test/e2e/cases/test_cli_comprehensive.py	New subprocess-driven tests validate CLI exec stream separation, large output, env/workdir flags, sequential execs, exit codes, error handling, listing, and named box creation.
REST exec comprehensive tests scripts/test/e2e/cases/test_exec_comprehensive.py	New async tests validate stdout/stderr isolation, exit/signal code propagation, large output integrity, unicode handling, empty output, user overrides, concurrency/env isolation, workdir edge cases, and streaming.
File I/O comprehensive tests scripts/test/e2e/cases/test_files_comprehensive.py	New async tests validate copy_in/copy_out round-trips for large files, empty files, nested directories, overwrite, missing paths, binary integrity, executable bits, and multi-file copies.
Lifecycle and box-management tests scripts/test/e2e/cases/test_lifecycle_comprehensive.py	New async tests validate rootfs persistence, exec-on-stopped-box failure, state transitions, leak checks, custom CPU/memory, box isolation, and force-remove semantics.
Node SDK pytest wrapper and driver scripts/test/e2e/cases/test_node_comprehensive.py, scripts/test/e2e/sdks/node/e2e_comprehensive.ts	New Python wrapper invokes a new TypeScript driver via npx tsx, covering exec, file copy, lifecycle, and API contract scenarios for the Node SDK.
Real-world scenario tests scripts/test/e2e/cases/test_real_world.py	New async tests cover scripting, pipelines, git, file transfer, compilation, networking, process management, permissions, and diagnostics workflows.
Path verification helper swap scripts/test/e2e/cases/test_path_verification.py	Replaces ctx_request_json with request_json in the fallback box-lookup path.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• boxlite-ai/boxlite#678: Modifies the same test_path_verification.py fallback helper swap from ctx_request_json to request_json.

Suggested reviewers: DorianZheng

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: comprehensive REST API e2e coverage.
Description check	✅ Passed	The description covers the summary and verification plan, and it includes notes; only the exact template headings are missing.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/e2e-comprehensive-tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (7)

scripts/test/e2e/cases/test_cli_comprehensive.py (2)

31-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Regex for box id is unanchored.

BOX_ID_RE = re.compile(r"[A-Za-z0-9]{12}") matches any 12 consecutive alphanumeric characters anywhere in the output, including as a substring of a longer token (e.g. an embedded hash/digest). This can silently extract the wrong id. Consider anchoring with word boundaries (\b[A-Za-z0-9]{12}\b) or a more specific delimiter-aware pattern.

🤖 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/test/e2e/cases/test_cli_comprehensive.py` at line 31, The BOX_ID_RE
pattern in test_cli_comprehensive.py is too loose and can match 12-character
substrings inside longer tokens. Update the regex used for box-id extraction so
it only matches a standalone box id, such as by adding word boundaries or
another delimiter-aware constraint, and keep the change scoped to the BOX_ID_RE
constant so the parsing logic uses the stricter match everywhere.

---

171-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hardcoded box name risks collisions under parallel test runs.

name = "cli-e2e-named-box" is a fixed literal; concurrent CI runs or repeated local invocations could collide on box naming.

🤖 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/test/e2e/cases/test_cli_comprehensive.py` around lines 171 - 177, The
fixed box name in test_cli_run_with_name can collide across parallel or repeated
runs. Replace the hardcoded name assignment with a per-test unique value
generated at runtime in this test, and keep using that variable in the run()
call and any later assertions so each invocation gets an isolated box name.

scripts/test/e2e/cases/test_lifecycle_comprehensive.py (1)

1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docstring references an untested case.

The module docstring lists "double-stop is idempotent or returns typed error" (line 6) as covered, but no test implements double-stop. Either add the test or trim the docstring to match actual coverage.

🤖 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/test/e2e/cases/test_lifecycle_comprehensive.py` around lines 1 - 12,
The module docstring in test_lifecycle_comprehensive.py claims double-stop
behavior is covered, but no corresponding test exists. Either add a test in the
lifecycle suite (for example alongside the existing lifecycle/stop-start
coverage in the test_lifecycle_comprehensive module) that exercises stopping an
already-stopped box and asserts the expected idempotent or typed-error behavior,
or remove that bullet from the docstring so it matches the implemented coverage.

scripts/test/e2e/cases/test_files_comprehensive.py (1)

1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docstring overclaims test coverage.

The module docstring lists "symlink handling (follow vs preserve)" and "file with special characters in name" as covered edge cases, but no test in this file exercises either scenario. This can mislead future contributors into assuming that coverage exists.

♻️ Suggested fix

 """Comprehensive file I/O coverage over REST.

 Extends test_files_io.py with edge cases for the copy_in / copy_out chain:

   - large file transfer (4 MB) integrity via sha256
-  - symlink handling (follow vs preserve)
   - empty file round-trip
-  - file with special characters in name
   - deeply nested directory structure
   - copy_out of a non-existent path
   - permission preservation (executable bit)
   - overwrite=True replaces content
 """

Or add the corresponding test_symlink_* and test_special_char_filename_* tests to match the stated coverage.

🤖 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/test/e2e/cases/test_files_comprehensive.py` around lines 1 - 13, The
module docstring in test_files_comprehensive.py overstates the actual coverage
by mentioning symlink handling and special-character filenames that are not
exercised in this file. Update the docstring to match the real tests present, or
add the missing test cases (for example, symlink and special-character filename
scenarios) so the documented edge cases align with the coverage in the test
module.

scripts/test/e2e/cases/test_real_world.py (1)

204-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Multi-layer backslash escaping for C source is fragile/clever code.

Producing the C file via printf with quadruple-escaped \\\\n sequences (so they survive both Python and shell printf interpretation) is hard to reason about and easy to break on any small edit. Other tests in this same file (test_write_and_run_python_script, test_python_multifile_project) already use the simpler, safer heredoc (cat > file << 'EOF' ... EOF) pattern that avoids escaping entirely — reuse that here for consistency and clarity.

♻️ Proposed refactor using heredoc

-    c_code = (
-        '`#include` <stdio.h>\\n'
-        'int main() {\\n'
-        '    for (int i = 0; i < 5; i++) printf("i=%d\\\\n", i);\\n'
-        '    return 0;\\n'
-        '}\\n'
-    )
+    c_code = (
+        "`#include` <stdio.h>\n"
+        "int main() {\n"
+        "    for (int i = 0; i < 5; i++) printf(\"i=%d\\n\", i);\n"
+        "    return 0;\n"
+        "}\n"
+    )
     ex = await box.exec(
         "sh", ["-c",
-               f'printf "{c_code}" > /root/test.c && '
+               f"cat > /root/test.c << 'CEOF'\n{c_code}CEOF\n"
                "gcc -o /root/test /root/test.c && "
                "/root/test"],
     )

As per coding guidelines, "Use boring code: prefer obvious code over clever code because code is read more than written."

🤖 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/test/e2e/cases/test_real_world.py` around lines 204 - 222, The C
source creation in test_real_world.py is using fragile nested escaping through
c_code and printf inside box.exec, which is hard to maintain. Refactor this
setup to use the same heredoc approach already used in
test_write_and_run_python_script and test_python_multifile_project, so
/root/test.c is written with clear literal content instead of quadruple-escaped
newline sequences. Keep the rest of the compile-and-run flow unchanged,
including the gcc execution and the existing skip handling in the
test_real_world path.

Source: Coding guidelines

scripts/test/e2e/sdks/node/e2e_comprehensive.ts (2)

28-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

drainStream has no timeout guard.

If stream.next() never resolves to null (e.g. a hung stream), this loop blocks forever with no diagnostic. The only backstop is the 180s outer subprocess timeout in the Python wrapper, which won't indicate where the hang occurred.

As per coding guidelines, "Make concurrency behavior explicit: timeouts, retries, and cancellation must be explicit for external work."

♻️ Proposed fix: add an explicit drain timeout

-async function drainStream(stream: any): Promise<string> {
+async function drainStream(stream: any, timeoutMs = 30000): Promise<string> {
   let result = '';
   while (true) {
-    const chunk = await stream.next();
+    const chunk = await Promise.race([
+      stream.next(),
+      new Promise<never>((_, reject) =>
+        setTimeout(() => reject(new Error('drainStream: timed out waiting for chunk')), timeoutMs)),
+    ]);
     if (chunk === null) break;
     result += chunk;
   }
   return result;
 }

🤖 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/test/e2e/sdks/node/e2e_comprehensive.ts` around lines 28 - 36, The
drainStream helper currently waits on stream.next() indefinitely, so add an
explicit timeout/cancellation guard inside drainStream to fail fast with a clear
diagnostic if the stream hangs. Update the loop in drainStream in
e2e_comprehensive.ts to track elapsed time or use a timed wrapper around
stream.next(), and throw an error that identifies the stream-drain operation
when the limit is exceeded.

Source: Coding guidelines

---

60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Long, fragile positional exec() calls with null/false/undefined placeholders.

Calls like box.exec('pwd', [], null, false, undefined, undefined, '/tmp') (Line 105) rely on 7 positional args with two unnamed undefined placeholders — any reordering of the underlying napi-rs binding's parameters will silently break these calls. This pattern repeats across every exec() call in the file.

Consider a small local wrapper that accepts a typed options object (env, mergeStderr, cwd, etc.) and maps it to the underlying positional signature, so call sites stay self-documenting.

As per coding guidelines, "Keep argument lists short; group related values into typed options; avoid boolean flags that make one function do two workflows."

♻️ Proposed fix: wrap exec with a typed options helper

+interface ExecOpts {
+  env?: [string, string][] | null;
+  mergeStderr?: boolean;
+  cwd?: string;
+}
+
+async function run(box: any, cmd: string, args: string[], opts: ExecOpts = {}) {
+  return box.exec(cmd, args, opts.env ?? null, opts.mergeStderr ?? false,
+    undefined, undefined, opts.cwd);
+}

-      const ex = await box.exec('pwd', [], null, false, undefined, undefined, '/tmp');
+      const ex = await run(box, 'pwd', [], { cwd: '/tmp' });

Also applies to: 94-95, 105-105, 115-115, 125-126

🤖 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/test/e2e/sdks/node/e2e_comprehensive.ts` at line 60, The exec() calls
in the Node E2E test file use long positional argument lists with
null/false/undefined placeholders, which is brittle and hard to read. Add a
small local helper around box.exec that takes a typed options object (for env,
mergeStderr, cwd, and similar settings) and maps it to the underlying signature,
then update the repeated exec() call sites to use that helper so the intent is
self-documenting and the parameters are less fragile. Use the existing box.exec
call sites and the shared helper name you introduce to keep the changes easy to
locate.

Source: Coding guidelines

🤖 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/test/e2e/cases/test_cli_comprehensive.py`:
- Line 31: The box-id extraction in detached_box and test_cli_run_with_name can
fail before cleanup is registered, leaving a running box behind. Move the regex
assertion and box-id parsing inside the cleanup-protected flow so the
finally/yield path is always established immediately after box creation, and use
the detached_box and test_cli_run_with_name helpers to ensure any created box is
always stopped even if BOX_ID_RE does not match.

In `@scripts/test/e2e/cases/test_exec_comprehensive.py`:
- Around line 253-264: The test `test_exec_cwd_nonexistent_returns_error` is
swallowing every exception, which can hide unrelated failures. Tighten the
`try/except` around `box.exec` and `ex.wait` so only the expected
spawn-failed/nonexistent-cwd case is accepted, similar to
`test_nonexistent_command_returns_error`. In the `test_exec_comprehensive.py`
suite, inspect the raised exception message or type from
`box.exec`/`drain`/`wait` before treating it as success, and re-raise anything
unexpected.

In `@scripts/test/e2e/cases/test_lifecycle_comprehensive.py`:
- Around line 38-45: The lifecycle test is using fixed asyncio.sleep calls after
b.stop() and b.start(), which makes the stop/start checks flaky. Replace those
blind waits in test_lifecycle_comprehensive with a bounded polling wait that
repeatedly checks rt.get(b.id) (or the guest status field used elsewhere in the
test suite) until the instance is fully stopped or started. Keep the existing
stop/start flow in place, but gate the next assertion/action on the observed
state transition rather than elapsed time, and apply the same change to the
other sleep-based waits referenced in this test.
- Around line 71-81: The negative lifecycle test currently swallows every
exception in the stopped-box exec path, which makes the assertion too
permissive. In test_lifecycle_comprehensive.py, tighten the try/except around
b.exec/drain/ex.wait so only the expected stopped-box failure is accepted; use
the specific exception type or verify the exception message/condition instead of
a blanket except Exception. Keep the existing exec/drain/wait flow and the exec
on stopped box scenario, but make the failure check explicit so unrelated SDK or
transport errors do not count as success.

In `@scripts/test/e2e/cases/test_real_world.py`:
- Around line 153-158: The stderr value from drain(ex) is being stored in "_"
and then reused in the git-not-found detection, which is misleading and fragile.
Update the test_real_world flow to give stderr an intent-revealing name like
err, and use that named variable in the combined output check and the final
assert context so the skip logic stays clear and consistent with
test_compile_and_run_c.
- Around line 264-270: The shell command in the test is masking failure because
`echo DONE` runs unconditionally after `kill` and `wait`, so the assertion never
proves the process was terminated. Update the command built in
`test_real_world.py` inside the `box.exec` call to make `echo DONE` contingent
on successful termination (for example by chaining it with the preceding command
instead of a semicolon), and keep the test logic obvious by using the existing
`box.exec`/`sleep 999`/`kill $SPID` flow without adding clever control flow.

In `@scripts/test/e2e/sdks/node/e2e_comprehensive.ts`:
- Line 12: The file-level coverage comment is stale because it mentions “copy_in
+ verify” even though there is no corresponding test in this suite. Update the
header comment near the top of e2e_comprehensive.ts to reflect the actual
covered cases, or remove the “copy_in + verify” entry entirely if it is no
longer implemented; use the nearby comment block and the test suite naming in
e2e_comprehensive.ts as the place to align the documentation with current
behavior.
- Around line 23-26: The die() helper currently calls process.exit(2), which
bypasses the existing try/catch/finally cleanup path in e2e_comprehensive.ts.
Update die() to throw an error instead of exiting so the surrounding catch block
can handle termination and the finally block can still run cleanup for the
created box.

---

Nitpick comments:
In `@scripts/test/e2e/cases/test_cli_comprehensive.py`:
- Line 31: The BOX_ID_RE pattern in test_cli_comprehensive.py is too loose and
can match 12-character substrings inside longer tokens. Update the regex used
for box-id extraction so it only matches a standalone box id, such as by adding
word boundaries or another delimiter-aware constraint, and keep the change
scoped to the BOX_ID_RE constant so the parsing logic uses the stricter match
everywhere.
- Around line 171-177: The fixed box name in test_cli_run_with_name can collide
across parallel or repeated runs. Replace the hardcoded name assignment with a
per-test unique value generated at runtime in this test, and keep using that
variable in the run() call and any later assertions so each invocation gets an
isolated box name.

In `@scripts/test/e2e/cases/test_files_comprehensive.py`:
- Around line 1-13: The module docstring in test_files_comprehensive.py
overstates the actual coverage by mentioning symlink handling and
special-character filenames that are not exercised in this file. Update the
docstring to match the real tests present, or add the missing test cases (for
example, symlink and special-character filename scenarios) so the documented
edge cases align with the coverage in the test module.

In `@scripts/test/e2e/cases/test_lifecycle_comprehensive.py`:
- Around line 1-12: The module docstring in test_lifecycle_comprehensive.py
claims double-stop behavior is covered, but no corresponding test exists. Either
add a test in the lifecycle suite (for example alongside the existing
lifecycle/stop-start coverage in the test_lifecycle_comprehensive module) that
exercises stopping an already-stopped box and asserts the expected idempotent or
typed-error behavior, or remove that bullet from the docstring so it matches the
implemented coverage.

In `@scripts/test/e2e/cases/test_real_world.py`:
- Around line 204-222: The C source creation in test_real_world.py is using
fragile nested escaping through c_code and printf inside box.exec, which is hard
to maintain. Refactor this setup to use the same heredoc approach already used
in test_write_and_run_python_script and test_python_multifile_project, so
/root/test.c is written with clear literal content instead of quadruple-escaped
newline sequences. Keep the rest of the compile-and-run flow unchanged,
including the gcc execution and the existing skip handling in the
test_real_world path.

In `@scripts/test/e2e/sdks/node/e2e_comprehensive.ts`:
- Around line 28-36: The drainStream helper currently waits on stream.next()
indefinitely, so add an explicit timeout/cancellation guard inside drainStream
to fail fast with a clear diagnostic if the stream hangs. Update the loop in
drainStream in e2e_comprehensive.ts to track elapsed time or use a timed wrapper
around stream.next(), and throw an error that identifies the stream-drain
operation when the limit is exceeded.
- Line 60: The exec() calls in the Node E2E test file use long positional
argument lists with null/false/undefined placeholders, which is brittle and hard
to read. Add a small local helper around box.exec that takes a typed options
object (for env, mergeStderr, cwd, and similar settings) and maps it to the
underlying signature, then update the repeated exec() call sites to use that
helper so the intent is self-documenting and the parameters are less fragile.
Use the existing box.exec call sites and the shared helper name you introduce to
keep the changes easy to locate.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 07d57df7-0987-4182-9003-3a0d5300ae5e

📥 Commits

Reviewing files that changed from the base of the PR and between 5c51bd7 and df45666.

📒 Files selected for processing (8)

• scripts/test/e2e/cases/test_cli_comprehensive.py
• scripts/test/e2e/cases/test_exec_comprehensive.py
• scripts/test/e2e/cases/test_files_comprehensive.py
• scripts/test/e2e/cases/test_lifecycle_comprehensive.py
• scripts/test/e2e/cases/test_node_comprehensive.py
• scripts/test/e2e/cases/test_path_verification.py
• scripts/test/e2e/cases/test_real_world.py
• scripts/test/e2e/sdks/node/e2e_comprehensive.ts

[coderabbitai]

🧹 Nitpick comments (3)

scripts/test/e2e/cases/test_node_comprehensive.py (1)

107-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Repetitive assertion boilerplate across ~15 tests.

Each test follows the identical _run → returncode == 0 → marker-substring pattern. Parametrizing with pytest.mark.parametrize (test name, marker string) would cut this to a fraction of the lines while still producing distinct JUnit test IDs for the uploaded pytest-junit.xml.

♻️ Example refactor

`@pytest.mark.parametrize`("case,marker", [
    ("signal_exit", "SIGNAL_EXIT=ok"),
    ("large_stderr", "LARGE_STDERR=ok"),
    ("many_env", "MANY_ENV=ok"),
    ("unicode", "UNICODE=ok"),
    ("copy_roundtrip", "COPY_ROUNDTRIP=ok"),
    ("copy_binary", "COPY_BINARY=ok"),
    ("copy_large", "COPY_LARGE=ok"),
    ("copy_nested", "COPY_NESTED=ok"),
    ("lifecycle_stop_start", "LIFECYCLE_STOP_START=ok"),
    ("box_info", "BOX_INFO=ok"),
    ("two_boxes_isolated", "TWO_BOXES_ISOLATED=ok"),
    ("list_info", "LIST_INFO=ok"),
])
def test_node_case(node_env, case, marker):
    r = _run(node_env, case)
    assert r.returncode == 0, f"exit={r.returncode}\nstderr:\n{r.stderr}"
    assert marker in r.stdout

🤖 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/test/e2e/cases/test_node_comprehensive.py` around lines 107 - 188,
The tests in test_node_comprehensive.py repeat the same _run/assert pattern
across many nearly identical cases, so refactor them into a single parametrized
test using pytest.mark.parametrize. Keep the case names and marker strings as
parameters in a new test function (for example, a unified test_node_case that
calls _run), and preserve separate JUnit-visible IDs by relying on pytest’s
parametrized test naming rather than individual duplicate test bodies.

scripts/test/e2e/sdks/node/e2e_comprehensive.ts (1)

264-282: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff

Fixed sleeps instead of polling actual box state.

setTimeout(r, 1000) after stop() and setTimeout(r, 2000) after start() are used to wait for state transitions rather than polling getInfo/status or awaiting a completion signal. This is flaky under load (too short) and wastes time otherwise (too long), and conflicts with the guideline to avoid sleep-based waiting for events in favor of explicit completion signals.

As per coding guidelines, "Do not use sleep for events; use channels, waitpid, or futures instead."

🤖 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/test/e2e/sdks/node/e2e_comprehensive.ts` around lines 264 - 282, The
lifecycle_stop_start test in e2e_comprehensive.ts is relying on fixed delays
after b.stop() and b.start() instead of waiting for the box to actually reach
the expected state. Replace those sleeps with explicit synchronization by
polling the box state via the existing runtime/box status API or awaiting a
completion signal/future tied to stop/start in the newBox/rt flow, so the test
only proceeds once the box is fully stopped and restarted.

Source: Coding guidelines

.github/workflows/e2e-dev.yml (1)

49-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider persist-credentials: false on checkout.

Static analysis flags credential persistence via actions/checkout@v5 default persist-credentials: true. Since subsequent steps run cargo/npm/pip installs (third-party code) and curl calls to an external dev API, disabling credential persistence reduces the blast radius if any of that tooling is compromised.

🔒 Suggested fix

       - uses: actions/checkout@v5
+        with:
+          persist-credentials: false

🤖 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-dev.yml at line 49, The checkout step is persisting
Git credentials by default, which is unnecessary for this workflow and increases
risk. Update the `actions/checkout@v5` step in the e2e dev workflow to disable
credential persistence by setting `persist-credentials: false`, so later
`cargo`, `npm`, `pip`, and `curl` steps cannot reuse repo credentials if
compromised.

Source: Linters/SAST tools

🤖 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-dev.yml:
- Line 49: The checkout step is persisting Git credentials by default, which is
unnecessary for this workflow and increases risk. Update the
`actions/checkout@v5` step in the e2e dev workflow to disable credential
persistence by setting `persist-credentials: false`, so later `cargo`, `npm`,
`pip`, and `curl` steps cannot reuse repo credentials if compromised.

In `@scripts/test/e2e/cases/test_node_comprehensive.py`:
- Around line 107-188: The tests in test_node_comprehensive.py repeat the same
_run/assert pattern across many nearly identical cases, so refactor them into a
single parametrized test using pytest.mark.parametrize. Keep the case names and
marker strings as parameters in a new test function (for example, a unified
test_node_case that calls _run), and preserve separate JUnit-visible IDs by
relying on pytest’s parametrized test naming rather than individual duplicate
test bodies.

In `@scripts/test/e2e/sdks/node/e2e_comprehensive.ts`:
- Around line 264-282: The lifecycle_stop_start test in e2e_comprehensive.ts is
relying on fixed delays after b.stop() and b.start() instead of waiting for the
box to actually reach the expected state. Replace those sleeps with explicit
synchronization by polling the box state via the existing runtime/box status API
or awaiting a completion signal/future tied to stop/start in the newBox/rt flow,
so the test only proceeds once the box is fully stopped and restarted.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 30ca3e49-d807-4a0d-ae8e-5412afa3d1ef

📥 Commits

Reviewing files that changed from the base of the PR and between df45666 and 61cfceb.

📒 Files selected for processing (3)

• .github/workflows/e2e-dev.yml
• scripts/test/e2e/cases/test_node_comprehensive.py
• scripts/test/e2e/sdks/node/e2e_comprehensive.ts

[coderabbitai]

Actionable comments posted: 1

🤖 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/test/e2e/sdks/node/e2e_comprehensive.ts`:
- Around line 279-285: The box.name() coverage block in e2e_comprehensive.ts is
too narrowly gated and is skipped during full-suite runs. Update the `box_name`
branch to match the other cases by allowing execution when `TEST === 'all'` as
well as when `TEST === 'box_name'`, so the `newBox`/`b.name()` assertion runs in
comprehensive mode.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ef661a9d-d474-4078-9820-1608a333d2e3

📥 Commits

Reviewing files that changed from the base of the PR and between 45c5f9c and b0236c3.

📒 Files selected for processing (2)

• scripts/test/e2e/cases/test_node_comprehensive.py
• scripts/test/e2e/sdks/node/e2e_comprehensive.ts