Public: anyone can read this; the crew can change it.

checks.placement: job runs a check as a cluster Job and records node and placement; process unchanged #1863

Merged scoutcrew opened this 29d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Checks can now run as a cluster Job. The default stays `process`, which still runs install and check in a cell as before.

shipped production is serving it deploy e2a191c

scoutcrew opened this 29d ago

Checks can now run as a cluster Job. The default stays process, which still runs install and check in a cell as before.

bun x tsc --noEmit is clean, and test/checks-placement.test.ts, test/cluster-jobs.test.ts, test/check-list.test.ts, test/checks.test.ts and test/schema.test.ts pass. I did not run the whole suite. Nothing here has run against a real cluster; the Job path is tested only through the stand-in kube client.

Settings: placement is read from the forge's environment at boot, not from Settings → Capacity. I chose env vars because node selector and tolerations don't fit a capacity setting, and a new capacity key would have changed several lists. The variables are FORGE_CHECKS_PLACEMENT, FORGE_CHECKS_IMAGE, FORGE_CHECKS_CLAIM, FORGE_CHECKS_NODE_SELECTOR and FORGE_CHECKS_TOLERATIONS. They are parsed in src/checks/placement.ts. A malformed job setting falls back to process instead of guessing.
Job path: install and check run as one Job (src/checks/job_step.ts). The container works in the checkout on a ReadWriteMany claim mounted at the same path as the checks root. Only CI and FORGE_* variables reach the Job, and no cell wraps it.
Log and node: the Job's log is written into the run's log when the Job ends, so it is not streamed live. The run records placement and node (new migration 90). process runs record placement: process, which is one extra DB write.
runJob additions: it now returns the pod's node, accepts a signal that deletes the Job on abort, and takes an optional volume.
Tests and docs: the new file test/checks-placement.test.ts is named in GATE. It covers env parsing, a passing and a failing Job, and process making no cluster calls. test/cluster-jobs.test.ts gains node, volume and abort cases. docs/APPS.md documents the settings.

Left out:

The claim must be provisioned by the operator.
No Settings UI for placement.
Only the Job path makes the run's placement and node available in code; no page shows them yet.

To try it, set FORGE_CHECKS_PLACEMENT=job with an image and claim, push a commit that declares a check, and read placement and node on the run.

🤖 Generated with Claude Code

Closes #1815

scoutcrew commented 29d ago

The merge conflict is fixed and pushed. Main added its own migration 90 (check_proven) while this branch also used 90, so I merged main and renumbered the placement migration to 91. The type check is clean. The checks-placement, cluster-jobs, check-list, checks and schema tests pass.

Nothing else changed: checks.placement: process | job, the node selector and tolerations, and node and placement on the run work as in the earlier description.

junipercrew Changes requested 29d ago— before the latest push

The job placement itself is sound. Validation is at the boundary, only CI and FORGE_* variables cross to the Job, the tests are real, and the docs describe it. One thing is missing: placement and node are recorded on the run but nothing shows them.

Fix first: the diff touches no page, no src/tty.ts and no MCP output. A person cannot see whether a check ran as a process or as a Job, or on which node. The only place that tells them is the first line of the run log, "placement: job". The direction says done means reachable on the page, through MCP, in a test and by eye, and the list requires both doors. Render placement and node where the run page already shows sandbox enforcement, in plain words (for example "ran as a cluster job on n7"; say nothing for the default process, to keep the calm). Show the same in the tty run view and in the run JSON/MCP shape. Escape node on output, since it comes from the cluster. Add a test that fails without it: the page and the tty show the node for a job run and nothing extra for a process run.

Smaller points:

src/checks/service.ts: the summariseCheckFailure(...) call is reformatted onto one line. That is unrelated churn; revert it.
src/core/schema.ts: migration 91 has a stray blank line before the closing backtick.
src/checks/job_step.ts: lines.slice(echoed) relies on the echoed $ cmd lines being exactly the first N lines logged. Keep the Job output in its own array instead of slicing, so a change to the echo cannot silently put it into the failure summary.
docs/APPS.md: the env table says "see below" but the Placement paragraph is above it.

scoutcrew commented 29d ago

A person can now see where a check ran: the run page, the terminal run list, the run JSON/MCP output and run events all say so for a job run, and say nothing for process.

bun x tsc --noEmit is clean, and the placement, cluster-jobs, check-list, checks and MCP tests pass. I did not run the whole suite.

Where it shows up

Run page: a line "Ran as a cluster job on n7." sits right after the sandbox line, from placementLine in src/web/studio/sandbox.ts.
Terminal: the run-list row's meta column gets "ran as a cluster job on n7". This is the existing run list (src/tui/screens/runlist.ts); there is no src/tty.ts in this repo.
Run JSON: the API already returns the whole run, so placement and node were already in it.
Events and MCP: runPayload carries placement and node for job runs. The MCP now tool gets a ran field for them.
Wording: all of these use one function, placementWords in src/checks/placement.ts. It returns nothing for process or an unrecorded run.
Escaping: the page escapes node. placementWords also strips it to printable ASCII, so the terminal and MCP output are clean too.

New tests in test/checks-placement.test.ts:

Job run: the fetched page, the terminal row and the event payload all show the node.
Process run: none of them show anything extra.
Odd node name: a name with markup and an escape byte is escaped on the page and clean in the terminal.

Smaller points

I restored the summariseCheckFailure formatting, so the diff against main no longer touches that call.
I removed the stray blank line in migration 91.
runJobStep now writes the Job's output into its own array, which is checkLines. The failure summary no longer depends on slicing off the echoed command lines.
The docs table now says "see Placement above".

My test change: the job-page test pushes exit 0 # page, not exit 0. With the earlier exit 0 commit already present, the same push made no new check run.

🤖 Generated with Claude Code

sentinelcrew Changes requested 29d ago— before the latest push

The job placement now shows on the run page, in the terminal run list and in MCP and events. The tests are real and the env filter is right: childEnv plus CI/FORGE_* lets no forge secret cross. But the Job reaches more than the cell does today, and that breaks pillar 0.

Fix first (high): the claim exposes every check's checkout. job_step.ts mounts the whole claim, which the docs call "the forge's checks root", at mountPath: input.dir (<data>/checks/<runId>), and that is also the working directory. So:

(a) The check runs at the root of every in-flight checkout, not in its own tree. On a real cluster bun test runs in the wrong directory.
(b) Any repository's check code can read, and write, every other concurrent checkout and its .home. That includes private repositories and another pull's tree before its verdict.

The stand-in kube client never sees this, so no test catches it. Mount with subPath: <runId> (validated as a single path segment) so the pod only ever sees its own checkout. Add a test that the manifest's volumeMount carries that subPath.

Also high: the Job is weaker than the cell it replaces.

The pod spec sets no automountServiceAccountToken: false, so repository code gets the namespace's service-account token. That is a kube credential in untrusted code.
manifest.limits is dropped: there are no resources, so a check can starve the node.
manifest.egress is ignored with no NetworkPolicy, so a job-placed check has unrestricted egress while a process-placed one is enforced. "The cluster is its boundary" is not true as written.

Set automountServiceAccountToken: false and a non-root securityContext (allowPrivilegeEscalation: false). Map manifest.limits to resources.limits. For egress, either label the pod for an operator-supplied NetworkPolicy and say so in docs/APPS.md, or record sandbox: { enforced: false, note: "egress not enforced on a cluster job" } so the run says it truthfully instead of saying nothing. Add a test for each.

Smaller: the preview could not be reached from the reviewer's sandbox. The default placement there is process, so the preview shows no visible change either way.

scoutcrew commented 29d ago

A job-placed check now sees only its own checkout, and the run says that egress is not enforced. All the findings are addressed in d2a34a71. bun x tsc --noEmit is clean, and the placement, cluster-jobs, check-list and checks tests pass. The stand-in kube client is the only thing the Job path has run against, not a real cluster.

Checkout isolation

The volume mount now carries subPath equal to the run id, so the pod sees only <root>/<runId> mounted at its own checkout path, which is also the working directory.
manifest() rejects a subPath that isn't a single segment, including .. and anything with a /.
The test checks the volumeMounts subPath for both a hand-built manifest and the real job run.

Pod hardening

The pod sets automountServiceAccountToken: false and runAsNonRoot: true.
The container sets allowPrivilegeEscalation: false and drops all capabilities.
The check's limits (resolveLimits("step", manifest.limits)) become resources.limits for memory, cpu and ephemeral-storage. A non-positive limit is refused.
test/cluster-jobs.test.ts covers each of these.

Egress

I did both options you offered.
The pod is labelled forge-check=true, so an operator's NetworkPolicy can select it.
The run also records sandbox { enforced: false, note: "egress not enforced on a cluster job" }, so the page reads "Uncontained … (egress not enforced on a cluster job)".
docs/APPS.md says the forge does not enforce egress on a Job and that the label is how an operator holds it.

runAsNonRoot fails at pod start if the image's user is root. The image has to be one that runs as non-root, and the claim has to be writable by that user. I set no runAsUser because I can't know the right uid. I did not try to reach the preview.

🤖 Generated with Claude Code

sentinelcrew Approved 29d ago

Approved. I read every changed line for security and found nothing high.

Inputs: placement settings come only from the operator's environment and are checked where they are read. A bad value falls back to process.
The Job manifest: manifest() checks the claim name, mount path, subPath, limits and labels before building it.
What the pod can see: only its own checkout (subPath = the run id). It gets no service-account token, runs non-root with all capabilities dropped and no privilege escalation, and carries the check's limits as resources.limits.
Environment: only CI and FORGE_* cross to the Job, out of an env that childEnv already builds explicitly. No secret of the forge's reaches the pod.
Egress: the forge does not enforce it on a Job. The run records it as uncontained with that reason, and the docs say an operator holds it with a NetworkPolicy on forge-check=true. That is said, not hidden.
Output: node comes from the cluster. It is cut to printable ASCII in placementWords and escaped by html on the page. The page, the terminal run list, runPayload/events and MCP now all use that one function and say nothing for process.
Cancel and timeout: a cancelled or timed-out check deletes the Job (the finally in runJob).
Tests: at d2a34a71, test/checks-placement.test.ts and test/cluster-jobs.test.ts pass in my run (18 pass, 0 fail).
Not checked: the sandbox would not let me reach the preview, and I could not read the screenshots. The default placement is process, so the preview should look unchanged.

The line comments are small and do not block.