fix(crawl): make TestConfigureLauncher/sandbox pass in containers (LAB-4994)#184
Conversation
…ine (LAB-4994)
TestConfigureLauncher/sandbox asserted the absolute presence of the
`no-sandbox` launcher flag. go-rod's launcher.New() adds --no-sandbox by
default when it detects a container (defaultFlags[NoSandbox] set when
inContainer), so l.Has("no-sandbox") reported true in Docker/dev-containers
regardless of Vespasian's own logic. The three "flag should be absent"
sub-cases therefore failed inside containers, breaking `make check`
locally even on a clean checkout.
Assert on Vespasian's contribution relative to the launcher baseline
instead of the absolute flag: the flag must be present whenever
configureLauncher opts in, and otherwise must match go-rod's default.
On CI's VM runner (no container -> baseline absent) this reduces to the
original assertion, preserving full regression detection of Vespasian's
sandbox logic; in dev-containers (baseline present) it passes for both
uid 0 and uid != 0. Test-only change; no runtime behavior is altered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughBrowser launcher configuration now centralizes the no-sandbox decision in ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Review
Critical issues
None.
Security
No security concerns flagged.
Test coverage
No critical issues — LGTM pending human review. Test-only change: the new baseline-delta assertion (want := tt.vespasianOn || baselineNoSandbox) is sound — it collapses to the original assertion on CI (container baseline absent → full regression detection of the opt/env→flag logic preserved) and tolerates go-rod's container default without letting Vespasian silently strip it. browser.go runtime behavior is unchanged.
There was a problem hiding this comment.
Codex Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
pkg/crawl/browser_test.go:69: In environments wherebaselineNoSandboxis already true, the assertions atpkg/crawl/browser_test.go:93can no longer catch a regression whereconfigureLauncheralways enablesNoSandbox. Consider factoring the Vespasian sandbox decision into a small helper and testing that directly, while keeping this launcher-baseline test for rod compatibility.
Could not run go test ./pkg/crawl -run TestConfigureLauncher -count=1 here because the sandbox is read-only and Go tried to create /home/runner/go.
Reviewed by Codex (gpt-5.5)
Extract the no-sandbox opt-in decision (opts.NoSandbox ||
VESPASIAN_NO_SANDBOX=="true") from configureLauncher into a pure
vespasianEnablesNoSandbox helper and assert on it directly in the sandbox
subtest, alongside the existing launcher-baseline Has("no-sandbox") check.
The prior baseline-relative assertion (want := tt.vespasianOn ||
baselineNoSandbox) becomes a tautology inside dev-containers, where
go-rod's launcher.New() auto-adds --no-sandbox so baselineNoSandbox==true:
the three negative cases would then pass even if configureLauncher were
regressed to unconditionally enable the flag. Asserting the pure decision
directly restores full regression detection in every environment (verified
in-container: injecting an always-on regression now fails the negative
cases), while the launcher-baseline check is kept for go-rod compatibility.
Addresses capability-pr-review findings SEC-BE-001 / TEST-001 and the Codex
review suggestion on PR #184. browser.go runtime behavior is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(LAB-4994) Address non-blocking review nits from capability-pr-review on PR #184: - QUAL-001: configureLauncher's doc comment restated the no-sandbox opt-in condition that now lives in vespasianEnablesNoSandbox; trim it to reference the helper so the condition has a single documented source of truth. - TEST-001: hoist the BrowserOptions{NoSandbox: tt.noSandbox} literal into one local reused by both the helper assertion and the configureLauncher call. No behavior change; make check (fmt, vet, lint, go test -race ./...) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Praetorian Capability PR ReviewPR: #184 Recommendation: COMMENT
Non-blockers (1)Quality (1)
|
Address capability-pr-review QUAL-001 (nit): the helper reads
os.Getenv("VESPASIAN_NO_SANDBOX"), so 'pure helper' was inaccurate.
Reword to 'self-contained helper'. Comment-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…994) - QUAL-001: reword test comment 'pure helper' -> 'self-contained helper' to match browser.go (the helper reads an env var, so not pure). - QUAL-002: rewrap browser.go helper doc comment to <=79 cols. - TEST-001: document the load-bearing 'CI runs on a non-container VM' assumption behind the launcher-baseline assertion (vacuous in a container; the vespasianEnablesNoSandbox assertion is the environment-independent guard). Comment/test-only; no production behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review + test results (head
|
| Lane | Verdict |
|---|---|
| QUAL (code quality) | ✅ no findings |
| TEST | ✅ no findings |
| SEC-BE (backend security) | ✅ no findings — refactor is token-for-token equivalent to the original inline condition |
| REQ (ticket alignment) | ✅ both LAB-4994 acceptance criteria met |
| DOC | n/a — comment/test-only change |
The intermediate run (before this commit) surfaced 3 non-blocking items, all now fixed in ea0531c:
- QUAL — the test comment still said "pure helper" (same wording corrected in
browser.go) → reworded to "self-contained helper". - QUAL — the reworded doc line exceeded the comment block's wrap width → rewrapped to ≤79 cols.
- TEST (low) — the launcher-baseline assertion
want := tt.vespasianOn || baselineNoSandboxis vacuous in a container (it only exercises theconfigureLauncherwiring on CI's non-container VM) → documented that load-bearing assumption in-line; thevespasianEnablesNoSandboxassertion remains the environment-independent regression guard.
Static / dynamic / live tests — all green
| Category | Command | Result |
|---|---|---|
| Static | gofmt -l · go vet ./... · golangci-lint run |
clean · rc=0 · 0 issues |
| Dynamic | go test -race ./... |
18 packages ok, 0 failures / 0 races |
| Dynamic (PR target) | go test -run TestConfigureLauncher/sandbox ./pkg/crawl |
✅ passes fresh as uid 1000 and uid 0 in-container |
| Live — offline | run-live-tests.sh --group offline |
19 / 19 passed |
| Live — browser | run-live-tests.sh --group live |
7 / 7 passed (rest-api, soap-service, graphql-server, concat-spa, concat-spa-two-stage, edge-cases, crawl-depth) |
Live browser tests ran with VESPASIAN_NO_SANDBOX=true (matching CI) against a real Chromium; the dev-container's /usr/bin/chromium-browser is a snap stub, so a runnable Chromium was put on PATH for the run.
🤖 Generated with Claude Code
eli-wald
left a comment
eli-wald
left a comment
There was a problem hiding this comment.
Praetorian Capability PR Review
PR: #184
Linear: https://linear.app/praetorianlabs/issue/LAB-4994
Head SHA: ea0531c
Reviewed at: 2026-07-21T07:09:12-07:00
Recommendation: APPROVE
| Totals | critical | high | medium | low | nit |
|---|---|---|---|---|---|
| Count | 0 | 0 | 0 | 0 | 0 |
| Blockers | 0 | 0 | 0 | 0 | 0 |
No findings. The prior review's QUAL-001 ("pure helper" doc comment) was addressed in commit 7685aede (reworded to "self-contained helper"), and the sandbox-test comment nits were addressed in ea0531cd. Both LAB-4994 acceptance criteria remain met; browser.go runtime behavior is unchanged (the vespasianEnablesNoSandbox extraction is byte-for-byte equivalent to the prior inline condition).
2 participants
Summary
pkg/crawl.TestConfigureLauncher/sandboxfailed inside Docker / dev-containers, somake check/make testcould not pass locally there — even on a clean checkout with no source changes. This fixes the test so it is deterministic across CI and containerized dev environments.Fixes LAB-4994.
Root cause
The test asserted the absolute presence of the
no-sandboxlauncher flag. go-rod'slauncher.New()adds--no-sandboxby default when it detects a container (defaultFlags[flags.NoSandbox]is set wheninContainer, which is true when/.dockerenvexists). Sol.Has("no-sandbox")reportedtruein dev-containers regardless of Vespasian's own logic, and the three "flag should be absent" sub-cases failed:Vespasian's own logic in
configureLauncheris correct — the assertion just conflated it with the launcher default.Fix
Adopted option 1 from the ticket (test-only, baseline delta). The test now computes the go-rod baseline (
launcher.New().Has("no-sandbox")) and asserts on Vespasian's contribution: the flag must be present wheneverconfigureLauncheropts in, and otherwise must match the launcher default (Vespasian must not add it on its own).want := tt.vespasianOn || baselineNoSandboxwant := tt.vespasianOn, byte-identical to the original assertion → full regression detection of Vespasian's opt/env → flag logic is preserved.Rejected option 2 (
NoSandbox(false)): that callsl.Delete(flags.NoSandbox), which would strip the flag go-rod deliberately adds for root-in-container and could break Chrome launch. This change is test-only;browser.goruntime behavior is unchanged.Verification
go test ./pkg/crawl/...passes in the dev-container as uid 1000 and as uid 0.make check(fmt, vet, lint = 0 issues,go test -race ./...) passes fully in the container.Acceptance criteria
go test ./pkg/crawl/...passes in a container for both uid 0 and uid != 0, and continues to pass on CI.🤖 Generated with Claude Code