Tegra E2E coverage + cloudflared tunnel + clawkeep fix - #105
Merged
Merged
Conversation
… cache Host .env was leaking CLAWBOX_HOME=/home/nexus0 and FILES_ROOT into the container where systemd loaded them via EnvironmentFile, breaking install and writes inside the container. Also exclude e2e-install/cache/ so the saved 352 MB clawbox-e2e image tarball never lands in git, and whitelist .env.test.example so the docs stay tracked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Cloudflare quick-tunnel support so owners can expose the device's
web UI without a Cloudflare account or domain:
- src/lib/tunnel.ts spawns `cloudflared tunnel --url http://localhost:80`,
extracts the trycloudflare.com URL from stdout, persists pid + url +
state under \$CLAWBOX_DATA_DIR (defaults to \$CLAWBOX_ROOT/data, falling
back to /home/clawbox/clawbox/data — never the hardcoded /data that
silently rejected writes and hung the enable handler for 5 min).
- /setup-api/tunnel/{enable,disable,status} route handlers for the UI.
- Unit tests for the lib and an older systemd-managed cloudflared lib,
plus route tests for the three handlers.
- 85-tunnel.spec.ts e2e: stubs cloudflared with a base64-encoded shell
script (echo + JSON.stringify mangles literal \\n), drives enable →
status → disable through the real container.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
\`git add -A\` (no pathspec) walks the tree and silently honors .gitignore. The previous explicit \`-- . :(exclude).clawkeep/config.json\` form errored on a freshly-init'd repo because syncIgnore writes the .clawkeep internals into .gitignore — git then refuses the explicitly-named-but-ignored path. The \`:(exclude)\` was redundant since those paths are already ignored. Adds 55-clawkeep.spec.ts which exercises init → configure → snap against the live container under an isolated /home/clawbox/clawkeep-e2e tree (must NOT live under data/, which is itself gitignored and trips clawkeep's own \`git add -A\`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e, credentials
Adds Vitest coverage for previously-untested paths:
- clawai-connect: ClawBox AI portal token format + connect flow
- portal + portal-heartbeat: registration token verification + heartbeat
(committed test file uses placeholder token claw_0123…cdef matching the
real format; never a real token)
- sqlite-store: kv-style sqlite persistence
- routes/portal: /setup-api/portal/* request shape
- routes/credentials-verify: chpasswd verification path
- routes/system-hostname: hostname GET/POST validation
- routes/wifi-saved-update: saved-network update happy + edge cases
- routes/network-internet: /setup-api/network/internet liveness check
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n, desktop UI, MCP, VNC, ollama
New specs driving the live container end-to-end:
- 05-captive-portal: Android/Apple/Windows/Firefox detection probes
redirect to 10.42.0.1
- 15-login-relogin: clearing the session cookie forces /login;
re-auth issues a fresh cookie and lands back on the desktop
- 25-desktop-ui: real React-shell smoke (not API) — shelf launcher,
Settings window, system tray. Uses .filter({ visible: true }) on
shelf-launcher-button because ChromeShelf renders mobile + desktop
variants and tailwind hides one
- 35-mcp: clawbox-cli wrapper through docker exec. Uses absolute
/home/clawbox/.bun/bin/bun path (docker exec --user clawbox doesn't
load login PATH)
- 45-vnc: /setup-api/vnc status surface
- 75-ollama: status + soft-skip search on 502 when ollama.com unreachable
- 85-tunnel: already landed earlier in this branch
Existing specs touched:
- 10-setup-wizard: drop the Local AI step assertion (PR #104 removed it
from the wizard and routed it to Settings → Local AI on demand)
- 20-settings: 9 new panel assertions (WiFi saved/scan, AI providers/
status, Telegram, Remote Access, system power, About, installed_apps)
Helpers + scaffolding:
- helpers/setup-api: add TunnelStatus/getTunnelStatus/enable/disable,
VNC status, OllamaStatus + searchOllama (returns { results } not
{ models }), ClawKeepStatus + init/configure/snap helpers
- .env.test.example: documents CLAWBOX_AI_API_KEY +
OPENCLAW_PORTAL_{EMAIL,PASSWORD} (real values stay in .env.test which
is gitignored)
- save-image.sh: persists clawbox-e2e:latest as a gzipped tarball under
e2e-install/cache/ (gitignored) so re-runs skip the 12-min build
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR adds comprehensive end-to-end test suites for core features (login, settings, desktop UI, tunnel, Ollama, VNC, clawkeep) and introduces tunnel management capabilities with start/stop/status API routes and underlying implementation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TunnelRoute as /setup-api/tunnel/enable
participant TunnelLib as Tunnel Library
participant Cloudflared as cloudflared Process
Client->>TunnelRoute: POST /setup-api/tunnel/enable
TunnelRoute->>TunnelLib: isCloudflaredInstalled()
TunnelLib-->>TunnelRoute: installed: true/false
alt cloudflared not installed
TunnelRoute-->>Client: 400 { success: false, error }
else cloudflared installed
TunnelRoute->>TunnelLib: startTunnel()
TunnelLib->>Cloudflared: spawn cloudflared tunnel --url http://localhost:80
Cloudflared-->>TunnelLib: output: https://xxx.trycloudflare.com
TunnelLib->>TunnelLib: parseUrl() & writeTunnelState()
TunnelLib-->>TunnelRoute: { success: true, tunnelUrl }
TunnelRoute-->>Client: 200 { success: true, tunnelUrl }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
CI Summary✅ Tests
⏳ E2E
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 12-15: Add a clear warning about secrets to prevent accidental
commits: update the .gitignore (around the .env* / !.env.test.example entries)
to include a short comment stating that .env.test.example must never contain
real credentials and is for example values only, and also add a matching header
comment to the top of the .env.test.example file itself; additionally, add a
lightweight CI check or lint rule to validate that .env.test.example does not
contain common secret patterns (API keys, private keys, or long base64 strings)
so reviewers are alerted if real secrets are introduced.
In `@e2e-install/.env.test.example`:
- Around line 19-22: Reorder the env variable lines so they follow dotenv-linter
ordering: move GEMINI_API_KEY to appear before OPENAI_API_KEY (keeping
ANTHROPIC_API_KEY and OPENROUTER_API_KEY unchanged) — ensure the sequence is
ANTHROPIC_API_KEY, GEMINI_API_KEY, OPENAI_API_KEY, OPENROUTER_API_KEY to satisfy
the linter.
In `@e2e-install/05-captive-portal.spec.ts`:
- Around line 30-42: The status assertion is too permissive (allows 4xx); update
the check in the test callback for probe.path so it fails on any 4xx/5xx:
replace the expect call on res.status() that currently uses toBeLessThan(500)
with a stricter assertion (e.g., toBeLessThan(400)) so only 1xx–3xx (and
200/204) are accepted; adjust the expect message if needed to reference
probe.path and that 4xx/5xx are unacceptable.
In `@e2e-install/15-login-relogin.spec.ts`:
- Around line 28-57: Wrap the test body that creates a browser context (ctx =
await browser.newContext()) in a try/finally so ctx.close() always runs: move
all actions and assertions that use ctx/page into the try block and call await
ctx.close() in the finally block; do the same fix for the other test around
lines 59-73. Reference the test name "clearing cookies sends / → /login then
back to /", the ctx variable, and the page usage (page.goto, page.fill,
page.click, page.waitForURL) to locate where to wrap with try/finally.
In `@e2e-install/20-settings.spec.ts`:
- Around line 118-128: The test "WiFi panel — re-scan returns network list"
currently asserts res.ok which makes the test flaky in environments without WiFi
support; change the logic so the test only proceeds to parse and assert the
response body when the fetch to `${BASE_URL}/setup-api/wifi/scan?live=1` returns
ok, otherwise gracefully skip/return (or allow known non-ok statuses) instead of
failing the test. Specifically, remove the strict expect(res.ok).toBe(true) and
replace it with a conditional: if (!res.ok) return (or assert allowed status),
else await res.json() into body and assert typeof body.scanning === "boolean"
(using the existing body variable) so the test tolerates CI environments where
nmcli/qemu is unavailable.
In `@e2e-install/25-desktop-ui.spec.ts`:
- Around line 33-101: Create a reusable async helper gotoDesktop(page) that
encapsulates the repeated flow: navigate to BASE_URL with waitUntil
"domcontentloaded", detect if page.url().includes("/login") then fill the
password using SETUP_PASSWORD and submit while waiting for URL to leave /login
(preserve existing timeouts), and finally wait for the desktop root test id
("desktop-root") to be visible. Replace the duplicated blocks at the start of
each test with a single await gotoDesktop(page); keep existing selectors (e.g.,
'input[type="password"]', 'button[type="submit"]') and timeouts when
implementing the helper so behavior is unchanged.
In `@e2e-install/35-mcp.spec.ts`:
- Around line 69-90: The test `test("code init creates a project on disk + tidy
with code delete"...)` creates a project but only deletes it on the success
path; wrap the test body in a try/finally so the cleanup `dockerExec(cli("code",
"delete", projectId), { user: "clawbox", timeoutMs: 30_000 })` always runs
regardless of assertion failures, keeping `projectId` scoped outside the try;
use the existing `dockerExec` and `cli` calls in the finally block and preserve
the timeouts and user option.
In `@e2e-install/55-clawkeep.spec.ts`:
- Around line 94-106: The test currently only asserts typeof ls === "string", so
it always passes; update the assertion to verify the backup target actually
contains artifacts by trimming and parsing ls to an integer (e.g.,
Number(ls.trim()) or parseInt(ls, 10)) and assert that the resulting count is >
0. Locate the test named "backup directory holds at least one tracked artifact"
and modify the assertion that uses the ls result from dockerExec (which queries
TARGET_ABS) to check numeric > 0 rather than just string type.
In `@e2e-install/85-tunnel.spec.ts`:
- Around line 86-93: The fixed 500ms sleep in the "status reflects running
tunnel + URL" test is flaky; replace the one-off setTimeout with a polling loop
that repeatedly calls getTunnelStatus (or reads the status source) at short
intervals (e.g., 100–200ms) until status.running === true (and status.tunnelUrl
=== FAKE_URL) or a reasonable timeout (e.g., 5s) is exceeded, then assert
status.enabled/running/tunnelUrl; remove the setTimeout and ensure the test
fails fast on timeout to avoid hanging CI.
In `@e2e-install/save-image.sh`:
- Around line 37-49: In the heredoc in save-image.sh update the restore
instruction to quote the OUT variable: replace the unquoted redirection usage in
the string "docker load < $OUT" with a quoted version "docker load < \"$OUT\""
so the printed instructions use a safely-quoted $OUT; locate this text in the
heredoc block that prints the "[save-image] done." message.
- Around line 17-34: Replace the fixed "$OUT.tmp" temp file with a secure mktemp
created inside CACHE_DIR and register a trap to remove that temp file on
EXIT/failure; specifically, in save-image.sh create a temp file (e.g.,
TMP="$(mktemp "${CACHE_DIR}/clawbox-e2e.XXXXXX.tar.gz")"), pipe docker save |
gzip into that TMP, on success mv "$TMP" "$OUT", and install a trap 'trap "rm -f
\"$TMP\" || true" EXIT' so partial files are cleaned up if the pipeline fails
(ensure variables CACHE_DIR, OUT, TMP and the trap are used in the docker save
-> gzip -> mv sequence).
In `@src/app/setup-api/tunnel/disable/route.ts`:
- Around line 13-27: The POST route currently calls stopTunnel() without a
try/catch so thrown errors bypass the JSON contract; wrap the await stopTunnel()
call in a try/catch inside the POST function (route.ts) and on catch return
NextResponse.json({ success: false, error: String(err) }, { status: 500 }); also
keep the existing branch for result.success so both thrown errors and returned
failure results produce the same JSON error response; reference stopTunnel and
NextResponse in the POST handler when updating.
In `@src/app/setup-api/tunnel/enable/route.ts`:
- Around line 13-42: Wrap the entire POST handler body in a try/catch and return
a consistent JSON error response if any awaited call throws (e.g.,
isCloudflaredInstalled() or startTunnel()), so the response shape remains {
success: false, error: string } and appropriate status codes are set via
NextResponse.json; update the POST function to catch thrown exceptions, log or
include a concise error message, and return that JSON error (status 500) instead
of letting the exception bubble.
In `@src/app/setup-api/tunnel/status/route.ts`:
- Around line 13-21: The GET handler should run getTunnelStatus() and
isCloudflaredInstalled() concurrently (use Promise.all) and wrap the awaits in
try/catch; on success return NextResponse.json({...status,
cloudflaredInstalled}); on failure return a structured JSON error with a 500
status (e.g., NextResponse.json({ error: 'Failed to get tunnel status', details:
String(err) }, { status: 500 })); update the GET function to reference
getTunnelStatus, isCloudflaredInstalled, and NextResponse.json accordingly.
In `@src/lib/tunnel.ts`:
- Around line 155-166: The code marks resolved = true before the state-write
Promise.all completes in startTunnel, causing the outer promise to hang if any
write (writeFile(TUNNEL_PID_FILE), writeFile(TUNNEL_URL_FILE), writeState)
rejects; modify startTunnel so that resolved is set only after Promise.all
fulfills, and add a .catch handler on the Promise.all to either reject the outer
promise or resolve with a failure result and perform any cleanup (e.g., remove
partial files or reset state) so the outer promise never remains pending; locate
the block that references tunnelUrl, resolved, Promise.all and the symbols
TUNNEL_PID_FILE, TUNNEL_URL_FILE, writeState to implement this change.
- Around line 204-213: The code unconditionally signals the PID read from disk
(the variable pid and the process.kill calls) without verifying ownership;
update the shutdown logic (around the pid variable usage in src/lib/tunnel.ts)
to first validate the target process belongs to cloudflared by reading its
command line/executable (e.g., /proc/<pid>/cmdline or /proc/<pid>/exe on Linux,
or use a platform-appropriate ps/tasklist lookup) and ensure it contains the
expected binary name/args (e.g., "cloudflared") before sending SIGTERM/SIGKILL;
if the check fails, log a warning and do not send signals (and optionally remove
or rotate the stale pid file) so you never kill an unrelated process.
- Around line 8-13: The code currently imports exec and creates execAsync
(promisified exec) and uses it to check for cloudflared installation; replace
this with execFile to avoid shell injection: import execFile from
"child_process" (or import { execFile } from "child_process"), create
execFileAsync = promisify(execFile) instead of execAsync, and update the
cloudflared-check call site (the function that invokes execAsync to run the
cloudflared binary) to use execFileAsync with the binary path and args array
(not a shell string); remove any remaining uses of exec/execAsync in this
module.
In `@src/tests/routes/credentials-verify.test.ts`:
- Around line 28-70: Add call-contract assertions: in each test assert
clientIpMock was called, assert checkRateLimitMock was called (e.g., with the
mocked IP) in tests that proceed past rate-limiting and assert it was called
once in the rate-limited test, and in the password-handling tests assert
verifyPasswordMock is called with the submitted password for the "incorrect" and
"correct" cases and NOT called for the rate-limited, malformed JSON, and
empty-password cases; use the existing mock names (clientIpMock,
checkRateLimitMock, verifyPasswordMock) and the request helper (makeRequest) to
locate where to insert these expect(...) assertions.
In `@src/tests/routes/network-internet.test.ts`:
- Around line 22-27: Remove the redundant afterEach cleanup that calls
execFileMock.mockReset() since execFileMock.mockReset() is already invoked in
beforeEach; delete the afterEach(() => execFileMock.mockReset()) block so only
the beforeEach contains execFileMock.mockReset() and vi.resetModules() to avoid
duplicate resets while keeping test isolation.
In `@src/tests/routes/system-hostname.test.ts`:
- Around line 35-43: The test suite overwrites process.env.CLAWBOX_ROOT in the
beforeAll hook and currently deletes it in afterAll which can break other tests;
change this by capturing the original value at the start (e.g. const
prevClawboxRoot = process.env.CLAWBOX_ROOT in beforeAll or at file scope) and in
afterAll restore it (if prevClawboxRoot !== undefined set
process.env.CLAWBOX_ROOT = prevClawboxRoot else delete
process.env.CLAWBOX_ROOT), while keeping the existing cleanup of TEST_ROOT and
HOSTNAME_ENV_PATH; update the beforeAll/afterAll blocks that reference
CLAWBOX_ROOT, TEST_ROOT, and HOSTNAME_ENV_PATH accordingly.
- Around line 15-24: The execFile mock currently calls the callback with (err,
{stdout, stderr}) which doesn't match Node's (err, stdout, stderr) signature;
update the mock's callback parameter to cb: (err: Error | null, stdout: string,
stderr: string) => void and call cb with cb(result?.error ?? null,
result?.stdout ?? "", result?.stderr ?? ""); locate the mocked execFile function
and the helper execFileMock to ensure the returned result.error, result.stdout
and result.stderr are mapped into the three separate callback args and adjust
typings accordingly so tests exercise the real callback shape.
In `@src/tests/routes/wifi-saved-update.test.ts`:
- Around line 27-28: The test file currently calls execFileMock.mockReset() in
both beforeEach and afterEach, which is redundant; remove one of the hooks
(either delete the afterEach(() => execFileMock.mockReset()) or the beforeEach
version) and keep a single reset hook to ensure a clean mock state (reference
the beforeEach and afterEach that call execFileMock.mockReset() to locate the
lines to remove).
- Around line 107-113: Add a new test in the same suite that imports the route
module (const mod = await import("@/app/setup-api/wifi/update/route")) and calls
mod.POST with makeRequest({ action: "update", ssid: "TestNet-Home", password:
"<64-char-string>" }) where the password is 64 characters long, then assert the
response status is 400 to cover the password.length > 63 validation branch; keep
the test pattern consistent with the existing "rejects passwords shorter than 8
chars" case and name it something like "rejects passwords longer than 63 chars".
- Around line 115-129: The test "updates a network's password and reactivates
it" currently checks only the response; add assertions that execFileMock was
invoked with the expected nmcli commands so command wiring is tested: after
calling the POST handler imported from "@/app/setup-api/wifi/update/route" (via
POST and makeRequest) assert execFileMock was called for a nmcli "connection
modify" with the target SSID and new password arguments and also called for a
nmcli "connection up" (or equivalent reactivation) for that SSID; reference
execFileMock, the POST function from the imported module, and the test's
makeRequest payload to locate where to add the expects.
In `@src/tests/unit/clawai-connect.test.ts`:
- Around line 36-41: The uniqueness test for createClawAiUserCode is brittle
because it asserts exactly 1000 unique values from 1000 random draws; change the
assertion to a high lower bound instead (e.g.
expect(codes.size).toBeGreaterThanOrEqual(995)) so the test tolerates an
extremely rare collision while still ensuring sufficient randomness; update the
assertion in the test that calls connect.createClawAiUserCode() in the
"createClawAiUserCode is sufficiently random across many calls" case
accordingly.
In `@src/tests/unit/cloudflared.test.ts`:
- Around line 32-45: The teardown unconditionally deletes
process.env.CLAWBOX_ROOT and process.env.CLOUDFLARED_BIN which can erase
pre-existing environment values; modify the setup to capture previous values
(e.g., const prevClawboxRoot = process.env.CLAWBOX_ROOT, const
prevCloudflaredBin = process.env.CLOUDFLARED_BIN) before overwriting them in
beforeAll, and in afterAll restore them (process.env.CLAWBOX_ROOT =
prevClawboxRoot ?? undefined; process.env.CLOUDFLARED_BIN = prevCloudflaredBin
?? undefined) instead of using delete, updating the test file's
beforeAll/afterAll blocks and references to CLAWBOX_ROOT and CLOUDFLARED_BIN
accordingly.
In `@src/tests/unit/portal-heartbeat.test.ts`:
- Around line 20-37: The test's beforeAll overrides globalThis.fetch with
fetchMock but afterAll does not restore it, risking cross-suite pollution;
update the test (in src/tests/unit/portal-heartbeat.test.ts) to save the
original fetch (e.g., const originalFetch = globalThis.fetch) before assigning
fetchMock in beforeAll and then restore it in afterAll (globalThis.fetch =
originalFetch), keeping the existing cleanup of env vars and TEST_ROOT;
reference the beforeAll/afterAll blocks and the fetchMock assignment to locate
where to add the save/restore.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 47093f0c-7b47-4985-af10-36873be009e9
📒 Files selected for processing (31)
.dockerignore.gitignoree2e-install/.env.test.examplee2e-install/05-captive-portal.spec.tse2e-install/10-setup-wizard.spec.tse2e-install/15-login-relogin.spec.tse2e-install/20-settings.spec.tse2e-install/25-desktop-ui.spec.tse2e-install/35-mcp.spec.tse2e-install/45-vnc.spec.tse2e-install/55-clawkeep.spec.tse2e-install/75-ollama.spec.tse2e-install/85-tunnel.spec.tse2e-install/helpers/setup-api.tse2e-install/save-image.shsrc/app/setup-api/tunnel/disable/route.tssrc/app/setup-api/tunnel/enable/route.tssrc/app/setup-api/tunnel/status/route.tssrc/lib/clawkeep.tssrc/lib/tunnel.tssrc/tests/routes/credentials-verify.test.tssrc/tests/routes/network-internet.test.tssrc/tests/routes/portal.test.tssrc/tests/routes/system-hostname.test.tssrc/tests/routes/tunnel.test.tssrc/tests/routes/wifi-saved-update.test.tssrc/tests/unit/clawai-connect.test.tssrc/tests/unit/cloudflared.test.tssrc/tests/unit/portal-heartbeat.test.tssrc/tests/unit/sqlite-store.test.tssrc/tests/unit/tunnel.test.ts
Comment on lines
12
to
+15
| .env* | ||
| !.env.example | ||
| !.env.template | ||
| !.env.test.example |
There was a problem hiding this comment.
Risk: negating .env.test.example can lead to accidental secret commits.
You’re ignoring .env*, then explicitly unignoring !.env.test.example (Line 15). That order is correct, but it’s still easy for a “test example” file to grow real credentials over time. Add a short comment in the .gitignore (or in the file header) stating that env.test.example must never contain real secrets, and ideally enforce this in CI (lint/check for common secret patterns).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 12 - 15, Add a clear warning about secrets to
prevent accidental commits: update the .gitignore (around the .env* /
!.env.test.example entries) to include a short comment stating that
.env.test.example must never contain real credentials and is for example values
only, and also add a matching header comment to the top of the .env.test.example
file itself; additionally, add a lightweight CI check or lint rule to validate
that .env.test.example does not contain common secret patterns (API keys,
private keys, or long base64 strings) so reviewers are alerted if real secrets
are introduced.
Comment on lines
+19
to
+22
| ANTHROPIC_API_KEY= | ||
| OPENAI_API_KEY= | ||
| GEMINI_API_KEY= | ||
| OPENROUTER_API_KEY= |
There was a problem hiding this comment.
Reorder provider keys to satisfy dotenv-linter ordering.
GEMINI_API_KEY should appear before OPENAI_API_KEY to match the linter rule.
Proposed patch
ANTHROPIC_API_KEY=
-OPENAI_API_KEY=
GEMINI_API_KEY=
+OPENAI_API_KEY=
OPENROUTER_API_KEY=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ANTHROPIC_API_KEY= | |
| OPENAI_API_KEY= | |
| GEMINI_API_KEY= | |
| OPENROUTER_API_KEY= | |
| ANTHROPIC_API_KEY= | |
| GEMINI_API_KEY= | |
| OPENAI_API_KEY= | |
| OPENROUTER_API_KEY= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 21-21: [UnorderedKey] The GEMINI_API_KEY key should go before the OPENAI_API_KEY key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-install/.env.test.example` around lines 19 - 22, Reorder the env variable
lines so they follow dotenv-linter ordering: move GEMINI_API_KEY to appear
before OPENAI_API_KEY (keeping ANTHROPIC_API_KEY and OPENROUTER_API_KEY
unchanged) — ensure the sequence is ANTHROPIC_API_KEY, GEMINI_API_KEY,
OPENAI_API_KEY, OPENROUTER_API_KEY to satisfy the linter.
Comment on lines
+30
to
+42
| test(`responds to ${probe.path}`, async ({ request }) => { | ||
| // We don't strictly assert the body contents — that varies by probe — | ||
| // but every probe must return ≤ 4xx and either redirect us or hand | ||
| // back content so the captive-portal banner pops. | ||
| const res = await request.get(`${BASE_URL}${probe.path}`, { | ||
| headers: { "user-agent": probe.ua }, | ||
| maxRedirects: 0, | ||
| failOnStatusCode: false, | ||
| }); | ||
| // 200 / 204 / 30x are all acceptable: each tells the OS something | ||
| // sensible. 5xx = our middleware crashed and needs fixing. | ||
| expect(res.status(), `${probe.path} returned 5xx`).toBeLessThan(500); | ||
| }); |
There was a problem hiding this comment.
Probe assertion is too permissive to catch regressions.
Line 41 allows any 4xx, so missing probe interception (e.g., raw 404) still passes.
Proposed tightening
test(`responds to ${probe.path}`, async ({ request }) => {
@@
const res = await request.get(`${BASE_URL}${probe.path}`, {
headers: { "user-agent": probe.ua },
maxRedirects: 0,
failOnStatusCode: false,
});
- // 200 / 204 / 30x are all acceptable: each tells the OS something
- // sensible. 5xx = our middleware crashed and needs fixing.
- expect(res.status(), `${probe.path} returned 5xx`).toBeLessThan(500);
+ const status = res.status();
+ if (status >= 300 && status < 400) {
+ const location = res.headers()["location"] ?? "";
+ expect(location, `${probe.path} redirect target`).toContain("10.42.0.1");
+ } else {
+ expect([200, 204], `${probe.path} should be handled`).toContain(status);
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test(`responds to ${probe.path}`, async ({ request }) => { | |
| // We don't strictly assert the body contents — that varies by probe — | |
| // but every probe must return ≤ 4xx and either redirect us or hand | |
| // back content so the captive-portal banner pops. | |
| const res = await request.get(`${BASE_URL}${probe.path}`, { | |
| headers: { "user-agent": probe.ua }, | |
| maxRedirects: 0, | |
| failOnStatusCode: false, | |
| }); | |
| // 200 / 204 / 30x are all acceptable: each tells the OS something | |
| // sensible. 5xx = our middleware crashed and needs fixing. | |
| expect(res.status(), `${probe.path} returned 5xx`).toBeLessThan(500); | |
| }); | |
| test(`responds to ${probe.path}`, async ({ request }) => { | |
| // We don't strictly assert the body contents — that varies by probe — | |
| // but every probe must return ≤ 4xx and either redirect us or hand | |
| // back content so the captive-portal banner pops. | |
| const res = await request.get(`${BASE_URL}${probe.path}`, { | |
| headers: { "user-agent": probe.ua }, | |
| maxRedirects: 0, | |
| failOnStatusCode: false, | |
| }); | |
| const status = res.status(); | |
| if (status >= 300 && status < 400) { | |
| const location = res.headers()["location"] ?? ""; | |
| expect(location, `${probe.path} redirect target`).toContain("10.42.0.1"); | |
| } else { | |
| expect([200, 204], `${probe.path} should be handled`).toContain(status); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-install/05-captive-portal.spec.ts` around lines 30 - 42, The status
assertion is too permissive (allows 4xx); update the check in the test callback
for probe.path so it fails on any 4xx/5xx: replace the expect call on
res.status() that currently uses toBeLessThan(500) with a stricter assertion
(e.g., toBeLessThan(400)) so only 1xx–3xx (and 200/204) are accepted; adjust the
expect message if needed to reference probe.path and that 4xx/5xx are
unacceptable.
Comment on lines
+28
to
+57
| test("clearing cookies sends / → /login then back to /", async ({ | ||
| browser, | ||
| }) => { | ||
| // Brand-new context = no inherited cookies. | ||
| const ctx = await browser.newContext(); | ||
| const page = await ctx.newPage(); | ||
|
|
||
| // Step 1: anonymous request to the desktop should be redirected. | ||
| const homeResponse = await page.goto(BASE_URL, { waitUntil: "domcontentloaded" }); | ||
| expect(homeResponse, "no response from /").not.toBeNull(); | ||
| expect(page.url()).toMatch(/\/login/); | ||
|
|
||
| // Step 2: submit the password. | ||
| await page.fill('input[type="password"]', SETUP_PASSWORD); | ||
| await Promise.all([ | ||
| page.waitForURL((url) => !url.pathname.startsWith("/login"), { | ||
| timeout: 15_000, | ||
| }), | ||
| page.click('button[type="submit"]'), | ||
| ]); | ||
|
|
||
| // Step 3: /me path should now serve the desktop, not redirect. | ||
| const desktopResponse = await page.goto(BASE_URL, { | ||
| waitUntil: "domcontentloaded", | ||
| }); | ||
| expect(desktopResponse?.status(), "desktop fetch should be 200").toBe(200); | ||
| expect(page.url()).not.toMatch(/\/login/); | ||
|
|
||
| await ctx.close(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Always close browser contexts with try/finally.
If any assertion fails before the last line, the context remains open and can destabilize subsequent specs.
Proposed refactor
test("clearing cookies sends / → /login then back to /", async ({
browser,
}) => {
// Brand-new context = no inherited cookies.
const ctx = await browser.newContext();
- const page = await ctx.newPage();
+ try {
+ const page = await ctx.newPage();
@@
- await ctx.close();
+ } finally {
+ await ctx.close();
+ }
});
@@
test("wrong password is rejected without minting a session", async ({
browser,
}) => {
const ctx = await browser.newContext();
- const page = await ctx.newPage();
- await page.goto(`${BASE_URL}/login`, { waitUntil: "domcontentloaded" });
- await page.fill('input[type="password"]', "definitely-not-the-password");
- await page.click('button[type="submit"]');
+ try {
+ const page = await ctx.newPage();
+ await page.goto(`${BASE_URL}/login`, { waitUntil: "domcontentloaded" });
+ await page.fill('input[type="password"]', "definitely-not-the-password");
+ await page.click('button[type="submit"]');
@@
- await ctx.close();
+ } finally {
+ await ctx.close();
+ }
});Also applies to: 59-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-install/15-login-relogin.spec.ts` around lines 28 - 57, Wrap the test
body that creates a browser context (ctx = await browser.newContext()) in a
try/finally so ctx.close() always runs: move all actions and assertions that use
ctx/page into the try block and call await ctx.close() in the finally block; do
the same fix for the other test around lines 59-73. Reference the test name
"clearing cookies sends / → /login then back to /", the ctx variable, and the
page usage (page.goto, page.fill, page.click, page.waitForURL) to locate where
to wrap with try/finally.
Comment on lines
+118
to
+128
| test("WiFi panel — re-scan returns network list", async () => { | ||
| const res = await fetch(`${BASE_URL}/setup-api/wifi/scan?live=1`, { | ||
| method: "POST", | ||
| }); | ||
| expect(res.ok).toBe(true); | ||
| const body = (await res.json()) as { | ||
| scanning: boolean; | ||
| networks: Array<{ ssid: string }> | null; | ||
| }; | ||
| expect(typeof body.scanning).toBe("boolean"); | ||
| }); |
There was a problem hiding this comment.
Align WiFi scan assertion with constrained test environments.
Line 122 currently requires res.ok, but the file already documents nmcli/qemu instability (Line 108-110). This can make the scan test flaky in CI variants without full WiFi support.
Proposed fix
test("WiFi panel — re-scan returns network list", async () => {
const res = await fetch(`${BASE_URL}/setup-api/wifi/scan?live=1`, {
method: "POST",
});
- expect(res.ok).toBe(true);
- const body = (await res.json()) as {
- scanning: boolean;
- networks: Array<{ ssid: string }> | null;
- };
- expect(typeof body.scanning).toBe("boolean");
+ expect([200, 500]).toContain(res.status);
+ if (res.ok) {
+ const body = (await res.json()) as {
+ scanning: boolean;
+ networks: Array<{ ssid: string }> | null;
+ };
+ expect(typeof body.scanning).toBe("boolean");
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("WiFi panel — re-scan returns network list", async () => { | |
| const res = await fetch(`${BASE_URL}/setup-api/wifi/scan?live=1`, { | |
| method: "POST", | |
| }); | |
| expect(res.ok).toBe(true); | |
| const body = (await res.json()) as { | |
| scanning: boolean; | |
| networks: Array<{ ssid: string }> | null; | |
| }; | |
| expect(typeof body.scanning).toBe("boolean"); | |
| }); | |
| test("WiFi panel — re-scan returns network list", async () => { | |
| const res = await fetch(`${BASE_URL}/setup-api/wifi/scan?live=1`, { | |
| method: "POST", | |
| }); | |
| expect([200, 500]).toContain(res.status); | |
| if (res.ok) { | |
| const body = (await res.json()) as { | |
| scanning: boolean; | |
| networks: Array<{ ssid: string }> | null; | |
| }; | |
| expect(typeof body.scanning).toBe("boolean"); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-install/20-settings.spec.ts` around lines 118 - 128, The test "WiFi panel
— re-scan returns network list" currently asserts res.ok which makes the test
flaky in environments without WiFi support; change the logic so the test only
proceeds to parse and assert the response body when the fetch to
`${BASE_URL}/setup-api/wifi/scan?live=1` returns ok, otherwise gracefully
skip/return (or allow known non-ok statuses) instead of failing the test.
Specifically, remove the strict expect(res.ok).toBe(true) and replace it with a
conditional: if (!res.ok) return (or assert allowed status), else await
res.json() into body and assert typeof body.scanning === "boolean" (using the
existing body variable) so the test tolerates CI environments where nmcli/qemu
is unavailable.
Comment on lines
+107
to
+113
| it("rejects passwords shorter than 8 chars", async () => { | ||
| const mod = await import("@/app/setup-api/wifi/update/route"); | ||
| const res = await mod.POST( | ||
| makeRequest({ action: "update", ssid: "TestNet-Home", password: "short" }), | ||
| ); | ||
| expect(res.status).toBe(400); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add coverage for the password.length > 63 validation branch.
This suite checks short passwords but misses the upper-bound rejection path implemented in the route. Add a 64-char password case to prevent regressions in that branch.
Proposed test addition
it("rejects passwords shorter than 8 chars", async () => {
const mod = await import("@/app/setup-api/wifi/update/route");
const res = await mod.POST(
makeRequest({ action: "update", ssid: "TestNet-Home", password: "short" }),
);
expect(res.status).toBe(400);
});
+
+ it("rejects passwords longer than 63 chars", async () => {
+ const mod = await import("@/app/setup-api/wifi/update/route");
+ const tooLong = "a".repeat(64);
+ const res = await mod.POST(
+ makeRequest({ action: "update", ssid: "TestNet-Home", password: tooLong }),
+ );
+ expect(res.status).toBe(400);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("rejects passwords shorter than 8 chars", async () => { | |
| const mod = await import("@/app/setup-api/wifi/update/route"); | |
| const res = await mod.POST( | |
| makeRequest({ action: "update", ssid: "TestNet-Home", password: "short" }), | |
| ); | |
| expect(res.status).toBe(400); | |
| }); | |
| it("rejects passwords shorter than 8 chars", async () => { | |
| const mod = await import("@/app/setup-api/wifi/update/route"); | |
| const res = await mod.POST( | |
| makeRequest({ action: "update", ssid: "TestNet-Home", password: "short" }), | |
| ); | |
| expect(res.status).toBe(400); | |
| }); | |
| it("rejects passwords longer than 63 chars", async () => { | |
| const mod = await import("@/app/setup-api/wifi/update/route"); | |
| const tooLong = "a".repeat(64); | |
| const res = await mod.POST( | |
| makeRequest({ action: "update", ssid: "TestNet-Home", password: tooLong }), | |
| ); | |
| expect(res.status).toBe(400); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/routes/wifi-saved-update.test.ts` around lines 107 - 113, Add a new
test in the same suite that imports the route module (const mod = await
import("@/app/setup-api/wifi/update/route")) and calls mod.POST with
makeRequest({ action: "update", ssid: "TestNet-Home", password:
"<64-char-string>" }) where the password is 64 characters long, then assert the
response status is 400 to cover the password.length > 63 validation branch; keep
the test pattern consistent with the existing "rejects passwords shorter than 8
chars" case and name it something like "rejects passwords longer than 63 chars".
Comment on lines
+115
to
+129
| it("updates a network's password and reactivates it", async () => { | ||
| execFileMock.mockReturnValue({ stdout: "" }); | ||
| const mod = await import("@/app/setup-api/wifi/update/route"); | ||
| const res = await mod.POST( | ||
| makeRequest({ | ||
| action: "update", | ||
| ssid: "TestNet-Home", | ||
| password: "valid-password-123", | ||
| }), | ||
| ); | ||
| expect(res.status).toBe(200); | ||
| const body = await res.json(); | ||
| expect(body.success).toBe(true); | ||
| expect(body.connected).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert nmcli connection modify/up call arguments in update-path tests.
Current assertions validate response shape, but they don’t verify the side effects that matter most for this route. Add call assertions so these tests fail if command wiring regresses.
Proposed assertion strengthening
it("updates a network's password and reactivates it", async () => {
execFileMock.mockReturnValue({ stdout: "" });
const mod = await import("@/app/setup-api/wifi/update/route");
@@
const body = await res.json();
expect(body.success).toBe(true);
expect(body.connected).toBe(true);
+ expect(execFileMock).toHaveBeenCalledWith("nmcli", [
+ "connection", "modify", "TestNet-Home",
+ "wifi-sec.key-mgmt", "wpa-psk",
+ "wifi-sec.psk", "valid-password-123",
+ ]);
+ expect(execFileMock).toHaveBeenCalledWith("nmcli", [
+ "connection", "up", "TestNet-Home",
+ ]);
});
@@
it("reports reactivateError when nmcli connection up fails", async () => {
@@
const res = await mod.POST(
@@
);
+ expect(res.status).toBe(200);
const body = await res.json();
expect(body.success).toBe(true);
expect(body.connected).toBe(false);
expect(body.reactivateError).toMatch(/AP not found/);
+ expect(execFileMock).toHaveBeenCalledWith("nmcli", [
+ "connection", "up", "TestNet-Home",
+ ]);
});Also applies to: 131-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/routes/wifi-saved-update.test.ts` around lines 115 - 129, The test
"updates a network's password and reactivates it" currently checks only the
response; add assertions that execFileMock was invoked with the expected nmcli
commands so command wiring is tested: after calling the POST handler imported
from "@/app/setup-api/wifi/update/route" (via POST and makeRequest) assert
execFileMock was called for a nmcli "connection modify" with the target SSID and
new password arguments and also called for a nmcli "connection up" (or
equivalent reactivation) for that SSID; reference execFileMock, the POST
function from the imported module, and the test's makeRequest payload to locate
where to add the expects.
Comment on lines
+36
to
+41
| it("createClawAiUserCode is sufficiently random across many calls", () => { | ||
| const codes = new Set<string>(); | ||
| for (let i = 0; i < 1000; i += 1) codes.add(connect.createClawAiUserCode()); | ||
| // 8-char alphabet32 has > 1e12 codes; collisions in 1k draws should be 0. | ||
| expect(codes.size).toBe(1000); | ||
| }); |
There was a problem hiding this comment.
Avoid a probabilistic CI flake in the uniqueness assertion.
At Line 40, requiring exactly 1000 unique random codes can fail rarely by chance. Use a high lower bound instead of absolute equality.
Proposed patch
- expect(codes.size).toBe(1000);
+ expect(codes.size).toBeGreaterThanOrEqual(995);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("createClawAiUserCode is sufficiently random across many calls", () => { | |
| const codes = new Set<string>(); | |
| for (let i = 0; i < 1000; i += 1) codes.add(connect.createClawAiUserCode()); | |
| // 8-char alphabet32 has > 1e12 codes; collisions in 1k draws should be 0. | |
| expect(codes.size).toBe(1000); | |
| }); | |
| it("createClawAiUserCode is sufficiently random across many calls", () => { | |
| const codes = new Set<string>(); | |
| for (let i = 0; i < 1000; i += 1) codes.add(connect.createClawAiUserCode()); | |
| // 8-char alphabet32 has > 1e12 codes; collisions in 1k draws should be 0. | |
| expect(codes.size).toBeGreaterThanOrEqual(995); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/unit/clawai-connect.test.ts` around lines 36 - 41, The uniqueness
test for createClawAiUserCode is brittle because it asserts exactly 1000 unique
values from 1000 random draws; change the assertion to a high lower bound
instead (e.g. expect(codes.size).toBeGreaterThanOrEqual(995)) so the test
tolerates an extremely rare collision while still ensuring sufficient
randomness; update the assertion in the test that calls
connect.createClawAiUserCode() in the "createClawAiUserCode is sufficiently
random across many calls" case accordingly.
Comment on lines
+32
to
+45
| beforeAll(async () => { | ||
| process.env.CLAWBOX_ROOT = TEST_ROOT; | ||
| process.env.CLOUDFLARED_BIN = FAKE_BIN; | ||
| await fs.mkdir(DATA_DIR, { recursive: true }); | ||
| vi.resetModules(); | ||
| cloudflared = await import("@/lib/cloudflared"); | ||
| await fs.mkdir(cloudflared.CLOUDFLARED_DIR, { recursive: true }); | ||
| TUNNEL_URL_FILE = cloudflared.TUNNEL_URL_FILE; | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| delete process.env.CLAWBOX_ROOT; | ||
| delete process.env.CLOUDFLARED_BIN; | ||
| await fs.rm(TEST_ROOT, { recursive: true, force: true }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Restore previous env values instead of deleting them in teardown.
At Line 43 and Line 44, unconditional delete can erase env values that existed before this suite, causing cross-test contamination.
Proposed patch
+const PREV_ENV = {
+ CLAWBOX_ROOT: process.env.CLAWBOX_ROOT,
+ CLOUDFLARED_BIN: process.env.CLOUDFLARED_BIN,
+};
+
beforeAll(async () => {
process.env.CLAWBOX_ROOT = TEST_ROOT;
process.env.CLOUDFLARED_BIN = FAKE_BIN;
@@
afterAll(async () => {
- delete process.env.CLAWBOX_ROOT;
- delete process.env.CLOUDFLARED_BIN;
+ if (PREV_ENV.CLAWBOX_ROOT === undefined) delete process.env.CLAWBOX_ROOT;
+ else process.env.CLAWBOX_ROOT = PREV_ENV.CLAWBOX_ROOT;
+ if (PREV_ENV.CLOUDFLARED_BIN === undefined) delete process.env.CLOUDFLARED_BIN;
+ else process.env.CLOUDFLARED_BIN = PREV_ENV.CLOUDFLARED_BIN;
await fs.rm(TEST_ROOT, { recursive: true, force: true });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeAll(async () => { | |
| process.env.CLAWBOX_ROOT = TEST_ROOT; | |
| process.env.CLOUDFLARED_BIN = FAKE_BIN; | |
| await fs.mkdir(DATA_DIR, { recursive: true }); | |
| vi.resetModules(); | |
| cloudflared = await import("@/lib/cloudflared"); | |
| await fs.mkdir(cloudflared.CLOUDFLARED_DIR, { recursive: true }); | |
| TUNNEL_URL_FILE = cloudflared.TUNNEL_URL_FILE; | |
| }); | |
| afterAll(async () => { | |
| delete process.env.CLAWBOX_ROOT; | |
| delete process.env.CLOUDFLARED_BIN; | |
| await fs.rm(TEST_ROOT, { recursive: true, force: true }); | |
| const PREV_ENV = { | |
| CLAWBOX_ROOT: process.env.CLAWBOX_ROOT, | |
| CLOUDFLARED_BIN: process.env.CLOUDFLARED_BIN, | |
| }; | |
| beforeAll(async () => { | |
| process.env.CLAWBOX_ROOT = TEST_ROOT; | |
| process.env.CLOUDFLARED_BIN = FAKE_BIN; | |
| await fs.mkdir(DATA_DIR, { recursive: true }); | |
| vi.resetModules(); | |
| cloudflared = await import("@/lib/cloudflared"); | |
| await fs.mkdir(cloudflared.CLOUDFLARED_DIR, { recursive: true }); | |
| TUNNEL_URL_FILE = cloudflared.TUNNEL_URL_FILE; | |
| }); | |
| afterAll(async () => { | |
| if (PREV_ENV.CLAWBOX_ROOT === undefined) delete process.env.CLAWBOX_ROOT; | |
| else process.env.CLAWBOX_ROOT = PREV_ENV.CLAWBOX_ROOT; | |
| if (PREV_ENV.CLOUDFLARED_BIN === undefined) delete process.env.CLOUDFLARED_BIN; | |
| else process.env.CLOUDFLARED_BIN = PREV_ENV.CLOUDFLARED_BIN; | |
| await fs.rm(TEST_ROOT, { recursive: true, force: true }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/unit/cloudflared.test.ts` around lines 32 - 45, The teardown
unconditionally deletes process.env.CLAWBOX_ROOT and process.env.CLOUDFLARED_BIN
which can erase pre-existing environment values; modify the setup to capture
previous values (e.g., const prevClawboxRoot = process.env.CLAWBOX_ROOT, const
prevCloudflaredBin = process.env.CLOUDFLARED_BIN) before overwriting them in
beforeAll, and in afterAll restore them (process.env.CLAWBOX_ROOT =
prevClawboxRoot ?? undefined; process.env.CLOUDFLARED_BIN = prevCloudflaredBin
?? undefined) instead of using delete, updating the test file's
beforeAll/afterAll blocks and references to CLAWBOX_ROOT and CLOUDFLARED_BIN
accordingly.
Comment on lines
+20
to
+37
| beforeAll(async () => { | ||
| process.env.CLAWBOX_ROOT = TEST_ROOT; | ||
| process.env.CLOUDFLARED_BIN = path.join(TEST_ROOT, "fake-cf"); | ||
| process.env.PORTAL_HEARTBEAT_URL = "https://test.invalid/api/heartbeat"; | ||
| await fs.mkdir(path.join(TEST_ROOT, "data"), { recursive: true }); | ||
| vi.resetModules(); | ||
| // @ts-expect-error overriding global fetch | ||
| globalThis.fetch = fetchMock; | ||
| configStore = await import("@/lib/config-store"); | ||
| heartbeat = await import("@/lib/portal-heartbeat"); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| delete process.env.CLAWBOX_ROOT; | ||
| delete process.env.CLOUDFLARED_BIN; | ||
| delete process.env.PORTAL_HEARTBEAT_URL; | ||
| await fs.rm(TEST_ROOT, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
Restore globalThis.fetch in teardown to prevent cross-suite pollution.
At Line 27, globalThis.fetch is overridden, but not restored in afterAll. This can make unrelated tests accidentally use this mock.
Proposed patch
const fetchMock = vi.fn();
+const originalFetch = globalThis.fetch;
@@
afterAll(async () => {
+ globalThis.fetch = originalFetch;
delete process.env.CLAWBOX_ROOT;
delete process.env.CLOUDFLARED_BIN;
delete process.env.PORTAL_HEARTBEAT_URL;
await fs.rm(TEST_ROOT, { recursive: true, force: true });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tests/unit/portal-heartbeat.test.ts` around lines 20 - 37, The test's
beforeAll overrides globalThis.fetch with fetchMock but afterAll does not
restore it, risking cross-suite pollution; update the test (in
src/tests/unit/portal-heartbeat.test.ts) to save the original fetch (e.g., const
originalFetch = globalThis.fetch) before assigning fetchMock in beforeAll and
then restore it in afterAll (globalThis.fetch = originalFetch), keeping the
existing cleanup of env vars and TEST_ROOT; reference the beforeAll/afterAll
blocks and the fetchMock assignment to locate where to add the save/restore.
This was referenced Apr 28, 2026
Merged
Merged
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
1 participant
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
Commits
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores