Skip to content

Commit cd95b7e

Browse files
committed
Fix the Chromium assertion: playwright-cli is stateful and I invented its API
CI's last failure. The assertion ran: playwright-cli screenshot --browser-arg=--no-sandbox file:///page.html /tmp/x.png Every part of that is wrong, and none of it was read from the tool. Read from the published @playwright/cli@0.1.17 tarball instead: - It is STATEFUL. `open <url>` starts a session; `screenshot --filename <path>` acts on the current page. There is no one-shot screenshot-a-URL form -- `screenshot [target]` takes an element reference from a page snapshot. - There is no --browser-arg. The sandbox is controlled by PLAYWRIGHT_MCP_SANDBOX, an env var. So INT-CONTAINER-RUNTIME-CONTRACT saying "Chromium must launch with --no-sandbox" named a mechanism that does not exist; the decision was right, the lever was imaginary. - file:// navigation is BLOCKED by default outside cwd unless PLAYWRIGHT_MCP_ALLOW_UNRESTRICTED_FILE_ACCESS is set. A good default, and the assertion does not weaken it: the page is served from /workspace, which is cwd. Same root cause as the ModelRuntime bug one commit ago -- describing an upstream API from inference rather than from the artifact. The difference is only that this one failed loudly. Also closed a real gap: INT-CONTAINER-RUNTIME-CONTRACT still hardcoded ANTHROPIC_API_KEY in the env allowlist, while the decision to support any pi provider was only ever written in the plan. The key variable is now specified as derived from pi's own table via findEnvKeys(provider) -- ~30 providers, no hardcoding, no pass-through, and it cannot drift when pi adds one. PLAYWRIGHT_MCP_SANDBOX added to the allowlist, or the job works in CI and fails in production.
1 parent 68d7abb commit cd95b7e

2 files changed

Lines changed: 36 additions & 15 deletions

File tree

.github/workflows/pi-upgrade-check.yml

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,18 @@ jobs:
147147
# CONST-ISOLATION-CONTAINER-PER-JOB requires. --shm-size, NOT --ipc=host: Playwright recommends
148148
# the latter, but it shares the HOST IPC namespace with a container running adversarial-input
149149
# agent code. The crash it prevents is caused by a 64MB /dev/shm, so fix that instead.
150-
- name: Chromium screenshots a local page as non-root
150+
# playwright-cli is STATEFUL: `open` starts a session, `screenshot` acts on the current page.
151+
# It is not a one-shot that takes a URL. The sandbox is controlled by PLAYWRIGHT_MCP_SANDBOX,
152+
# not by a --no-sandbox argument. And file:// navigation is blocked by default outside cwd --
153+
# a good default we do NOT weaken here: the page is served from /workspace, which is cwd.
154+
- name: Chromium screenshots a page as non-root
151155
run: |
152-
echo '<html><body><h1>pi-dispatch</h1></body></html>' > fixture/page.html
156+
mkdir -p ws && echo '<html><body><h1>pi-dispatch</h1></body></html>' > ws/page.html
153157
docker run --rm --init --cap-drop=ALL --security-opt no-new-privileges --shm-size=1g \
154-
-v "$PWD/fixture:/fixture:ro" --entrypoint sh pi-job:ci \
155-
-c 'playwright-cli screenshot --browser-arg=--no-sandbox file:///fixture/page.html /tmp/x.png && test -s /tmp/x.png' \
156-
|| { echo "::error::Chromium unusable as non-root — check PLAYWRIGHT_BROWSERS_PATH at build AND run"; exit 1; }
158+
-e PLAYWRIGHT_MCP_SANDBOX=false \
159+
-v "$PWD/ws:/workspace" --entrypoint sh pi-job:ci \
160+
-c 'playwright-cli open file:///workspace/page.html && playwright-cli screenshot --filename /workspace/shot.png && test -s /workspace/shot.png' \
161+
|| { echo "::error::Chromium unusable as non-root — check PLAYWRIGHT_BROWSERS_PATH at build AND run, and PLAYWRIGHT_MCP_SANDBOX at run"; exit 1; }
157162
158163
# Fonts absent => tofu boxes => screenshots that look fine and contain no legible text. That
159164
# silently guts the requirement full Chromium is in this image for.

specs/interfaces.md

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,18 @@ Evidence convention as in `constitution.md`.
337337
a common reason for zombie processes."* Our entrypoint `exec`s the runner, so **node is PID 1** and
338338
reaps nothing. Chromium spawns many processes; zombies accumulate against `--pids-limit` until the
339339
job dies of something unrelated to its actual work.
340-
- Env: `ANTHROPIC_API_KEY`, `GITHUB_TOKEN` (scoped, 1h), `PI_JOB_ID`,
341-
`PLAYWRIGHT_BROWSERS_PATH=/ms-playwright`, `PI_CODING_AGENT_DIR` (if not `$HOME/.pi/agent`)
340+
- Env: **the configured provider's key variable(s), derived — not hardcoded** (see below);
341+
`GITHUB_TOKEN` (scoped, 1h — GitHub-backed jobs only); `PI_JOB_ID`; `PI_PROVIDER`; `PI_MODEL`;
342+
`PI_MAX_TURNS`; `PLAYWRIGHT_BROWSERS_PATH=/ms-playwright`; `PLAYWRIGHT_MCP_SANDBOX=false`;
343+
`PI_CODING_AGENT_DIR` (if not `$HOME/.pi/agent`)
344+
- **The provider key variable is derived from pi's own table via `findEnvKeys(provider)`**
345+
(`import { findEnvKeys } from "@earendil-works/pi-ai/compat"`), never hardcoded and never
346+
pass-through. pi supports ~30 providers, each with its own variable, so "support any model" must not
347+
become "forward everything" — `no-broad-env-into-container` is a BLOCKER. Deriving the allowlist
348+
from pi's table rather than copying it means it **cannot drift** when pi adds a provider, and a
349+
hand-maintained copy is exactly the reinvention `no-reimplementing-pi` forbids. For `anthropic` the
350+
call returns `["ANTHROPIC_OAUTH_TOKEN", "ANTHROPIC_API_KEY"]`**the array order *is* the
351+
precedence**, which is precisely the trap this rule exists for.
342352
- Mounts: `/job:ro`, `/workspace:rw` — delivered by named volume + `volume-subpath`, never a host bind
343353
mount (`DES-JOB-FILES-VIA-VOLUME-SUBPATH`)
344354
- No TTY (`-it` absent)
@@ -355,15 +365,21 @@ Evidence convention as in `constitution.md`.
355365
RUN mkdir -p /home/pi/.pi/agent && chown -R pi:pi /home/pi/.pi
356366
COPY --chown=pi:pi guardrails/HARD_RULES.md /home/pi/.pi/agent/APPEND_SYSTEM.md
357367
```
358-
- **Chromium must launch with `--no-sandbox`** — and this is a **deliberate divergence from
359-
Playwright's docs**, which do not mention the flag at all. Their supported path for non-root Chromium
360-
is a custom seccomp profile granting `clone`/`setns`/`unshare`, or `--cap-add=SYS_ADMIN`. We give it
361-
neither: `--cap-drop=ALL` *is* `CONST-ISOLATION-CONTAINER-PER-JOB`'s enforcement surface.
362-
`--no-sandbox` does not acquire the privilege — it skips the code path needing it, leaving the
363-
container as the only boundary, which is exactly what this project already decided the boundary is.
364-
**Never "fix" a Chromium launch error by adding `SYS_ADMIN` or widening seccomp**: that trades the
365-
outer boundary for an inner one against adversarial input, inverting the security model. Written
368+
- **Chromium's own sandbox is disabled via `PLAYWRIGHT_MCP_SANDBOX=false`** — an env var, **not** a
369+
`--no-sandbox` argument, and not a `playwright-cli` flag. This is a **deliberate divergence from
370+
Playwright's docs**, which never mention disabling it: their supported path for non-root Chromium is
371+
a custom seccomp profile granting `clone`/`setns`/`unshare`, or `--cap-add=SYS_ADMIN`. We give it
372+
neither, because `--cap-drop=ALL` *is* `CONST-ISOLATION-CONTAINER-PER-JOB`'s enforcement surface.
373+
Disabling the inner sandbox does not acquire the privilege — it skips the code path that needs it,
374+
leaving the container as the only boundary, which is what this project already decided the boundary
375+
is. **Never "fix" a Chromium launch error by adding `SYS_ADMIN` or widening seccomp**: that trades
376+
the outer boundary for an inner one against adversarial input, inverting the security model. Written
366377
here because the vendor's own documentation recommends the thing we must not do.
378+
- **`playwright-cli` is stateful and its file access is restricted by default.** `open <url>` starts a
379+
session; `screenshot --filename <path>` acts on the current page. There is no one-shot
380+
`screenshot <url> <path>` form. Navigation to `file://` is **blocked outside cwd** unless
381+
`PLAYWRIGHT_MCP_ALLOW_UNRESTRICTED_FILE_ACCESS` is set — a good default that this project does not
382+
weaken: the agent's pages are served from `/workspace`, which is cwd.
367383
- **Fonts are required, and their absence is silent.** `bookworm-slim` ships none, so Chromium renders
368384
tofu boxes and screenshots look plausible while containing no legible text — which would quietly
369385
gut `REQ-FRONTEND-VISUAL-VERIFY`, the capability the whole image exists for. Install `fontconfig` +

0 commit comments

Comments
 (0)