diff --git a/decisions/2026-10-05-pm-agent-issue-to-pr.md b/decisions/2026-10-05-pm-agent-issue-to-pr.md index f07d9e0..28dcec5 100644 --- a/decisions/2026-10-05-pm-agent-issue-to-pr.md +++ b/decisions/2026-10-05-pm-agent-issue-to-pr.md @@ -90,6 +90,26 @@ All offline, in the pinned toolbox (`./infra/devcontainer/dev.sh run`), never on kustomization, because the kustomization's comment named it by filename. Correctly refused: the distance between a filename in a comment and a line under `resources:` is one careless edit. The comment now points at it in prose. +- **`infra/agent/probe-validation.sh` → 17 refusals, 0 failures.** A new, committed + negative-path probe for `pm-worker`'s input guards — the things that stand between an + injected environment variable and a clone from the wrong host or a push to the wrong ref. + Each case asserts a non-zero exit AND the specific message, so a guard that stopped firing + cannot pass by failing somewhere else. It refuses: an argument on the pinned command; + `main`, a wrong prefix, `feat/x;id`, `../../etc/passwd`, too-short slugs, and a branch equal + to the base; a redirected git host, a real-and-reachable wrong one (`github.com` — the + egress policy permits tcp/443, so this one is genuinely reachable), and the right host with + a smuggled path; a malformed owner or repository; an unknown mode; a non-numeric and a zero + issue number; and each missing credential. `build-image.sh` also runs it inside the built + image, where the interpreter is the one that will run in the cluster. +- **Writing that probe corrected an overclaim in the code.** The `PM_BRANCH != "main"` guard + is UNREACHABLE while the pattern check above it holds (`main` has no `feat|fix|docs|chore/` + prefix), so the comment claiming it as the defence against pushing to main was wrong about + which line does the work. It is kept — as an assertion that does not depend on the regex + being right — and the comment now says so. +- **Two more defects found by self-review of the unattended paths, both fixed:** the worker's + `EXIT` trap ended in a failing test, which under `set -e` can replace the script's real exit + code with that failure; and a provenance *log line* read a file through a redirect that + `set -o pipefail` would have turned into a dead worker if the file were absent. - **A self-review of the trigger found and fixed one real defect** before any of this ran: `last_activity()` compared RFC 3339 timestamp STRINGS to find the newest comment, which is only correct while every offset Gitea returns is identical — and the failure mode is the diff --git a/docs/plans/pm-agent-issue-to-pr.md b/docs/plans/pm-agent-issue-to-pr.md index 4196677..f6f42e1 100644 --- a/docs/plans/pm-agent-issue-to-pr.md +++ b/docs/plans/pm-agent-issue-to-pr.md @@ -215,7 +215,10 @@ holds a git write token and can author code. So: under the pinned CLI (the first run was 27/28 and caught a real defect: the volume rule denied a valid `emptyDir: {}`); `check-jsonschema` on the changed ServiceDefinition; the canonical render reproduces `apps/wave-0/gitea.yaml` with the new policy in it; `bash -n` -and `ast.parse` on the two entrypoints. **`./ci/gate.sh` → exit 3 (UNDETERMINED), with +and `ast.parse` on the two entrypoints; `PM_SELFTEST=1 pm-dispatch` → 26 assertions, 0 +failures; `infra/agent/probe-validation.sh` → **17 refusals, 0 failures** against +`pm-worker`'s input guards, including the three that matter most (a redirected git host, a +reachable-but-wrong one, and `main` as the branch). **`./ci/gate.sh` → exit 3 (UNDETERMINED), with `gitea-settings` the one stage that did not run** — no `SETTINGS_API_TOKEN` on this workstation — and every other stage green, `spec-immutability` included. rc 3 is not a pass. The gate also refused an earlier revision of this change twice, both times correctly; see diff --git a/infra/agent/build-image.sh b/infra/agent/build-image.sh index c2663d6..eb65585 100755 --- a/infra/agent/build-image.sh +++ b/infra/agent/build-image.sh @@ -134,6 +134,21 @@ if [ "${VERIFY}" -eq 1 ]; then echo "--- no Anthropic API key is baked into the image (it would outrank the subscription token) ---" [ -z "${ANTHROPIC_API_KEY:-}${ANTHROPIC_AUTH_TOKEN:-}" ] && echo " confirmed: neither is set" ' + # The worker's input guards, exercised in the image, with the interpreter and the script + # that will actually run in the cluster. The probe is mounted read-only rather than baked + # in: it is a build-time check, not something a worker Job has any business carrying. + log "Probing pm-worker's input validation inside the image" + "${ENGINE}" run --rm \ + --user 65532:65532 \ + --read-only \ + --tmpfs /workspace:rw,mode=1777 \ + --tmpfs /tmp:rw,mode=1777 \ + --network none \ + --volume "${HERE}/probe-validation.sh:/probe-validation.sh:ro,Z" \ + --entrypoint "" \ + "${IMAGE_REF}" \ + /bin/bash /probe-validation.sh + log "Image verified" fi diff --git a/infra/agent/pm-worker b/infra/agent/pm-worker index e92bb46..5f7be31 100755 --- a/infra/agent/pm-worker +++ b/infra/agent/pm-worker @@ -73,8 +73,15 @@ esac [[ "${PM_BRANCH}" =~ ^(feat|fix|docs|chore)/[a-z0-9][a-z0-9-]{2,48}$ ]] \ || die "PM_BRANCH '${PM_BRANCH}' does not match ^(feat|fix|docs|chore)/[a-z0-9][a-z0-9-]{2,48}$" -# NEVER the default branch. A worker that could be handed PM_BRANCH=main would push to main -# on its first commit, which CLAUDE.md §1 forbids in the strongest terms it has. +# NEVER the default branch. A worker handed PM_BRANCH=main would push to main on its first +# commit, which CLAUDE.md §1 forbids in the strongest terms it has. +# +# The base-branch check is the one that can fire on its own: a slug that satisfies the +# pattern above can still equal PM_BASE_BRANCH if someone sets that to a feature branch. +# The literal `main` check below is UNREACHABLE while the pattern holds — `main` has no +# `feat|fix|docs|chore/` prefix — and is kept deliberately anyway, as an assertion that does +# not depend on that regex being right. If the pattern is ever loosened, this still holds. +# infra/agent/probe-validation.sh exercises both, and records which message each produces. [ "${PM_BRANCH}" != "${PM_BASE_BRANCH}" ] || die "PM_BRANCH must not be the base branch" [ "${PM_BRANCH}" != "main" ] || die "refusing to work on 'main' (CLAUDE.md §1)" @@ -115,7 +122,13 @@ OUT=/workspace/out mkdir -p "$SRC" "$OUT" "$TMPDIR" "${CLAUDE_CONFIG_DIR:-/workspace/.claude}" log "assignment: issue #${ISSUE} · mode ${PM_TASK_MODE} · branch ${PM_BRANCH} · model ${MODEL}" -log "toolchain: $(cat /etc/agent-runner-versions 2>/dev/null | tr '\n' ' ')" +# Guarded with a readability test rather than a redirect plus `2>/dev/null`: redirections are +# applied left to right, so a failing `< file` is reported by the shell BEFORE stderr is +# silenced. And guarded at all because `set -o pipefail` + `set -e` would otherwise let a +# missing provenance file (this script run outside the image) kill the worker on a LOG LINE. +VERSIONS="" +[ -r /etc/agent-runner-versions ] && VERSIONS="$(tr '\n' ' ' < /etc/agent-runner-versions)" +log "toolchain: ${VERSIONS:-(no provenance file; not the agent-runner image?)}" # --- 2. the Gitea API, and git credentials that never touch disk in a URL ---- # WHY GIT_ASKPASS AND NOT A CREDENTIAL IN THE REMOTE URL (or ~/.git-credentials): a token @@ -214,7 +227,11 @@ report_failure() { } > "${OUT}/failure.md" post_issue_comment "${OUT}/failure.md" } -trap 'rc=$?; [ "$rc" -ne 0 ] && report_failure "an unhandled error (exit ${rc}); see the worker log"' EXIT +# An `if`, not `test && report`: the EXIT trap's own exit status matters under `set -e`, and +# a trailing failed test in it can replace the script's real exit code with that failure. The +# `if` form always leaves the trap returning 0, so the status the script exits with is the one +# it meant. +trap 'rc=$?; if [ "$rc" -ne 0 ]; then report_failure "an unhandled error (exit ${rc}); see the worker log"; fi' EXIT # --- 3. clone and put the branch under our feet ------------------------------ log "cloning ${PM_GITEA_OWNER}/${PM_GITEA_REPO} from ${PM_GITEA_URL}" diff --git a/infra/agent/probe-validation.sh b/infra/agent/probe-validation.sh new file mode 100755 index 0000000..2690e8c --- /dev/null +++ b/infra/agent/probe-validation.sh @@ -0,0 +1,120 @@ +#!/usr/bin/env bash +############################################################################### +# infra/agent/probe-validation.sh — prove pm-worker REFUSES hostile input. +# +# ./probe-validation.sh # from anywhere; needs only bash +# +# WHY THIS EXISTS. pm-worker's `command` is pinned by admission with no args, so everything a +# caller could influence arrives in the ENVIRONMENT — and the guards at the top of that +# script are what stand between an injected variable and a clone from the wrong host, a push +# to the wrong ref, or a session with the wrong credential. Guards that nothing exercises are +# a claim, not a control. Each case below asserts BOTH a non-zero exit AND the specific +# message, because a check that only grepped stderr would pass a build where the guard had +# stopped rejecting and something else had started failing instead. +# +# Every case must die BEFORE the script reaches any network call: the probe runs with a +# scrubbed environment and fake credentials, and if a guard ever stopped firing, the case +# would fail here rather than by really cloning something. +# +# Run offline, no cluster, no Gitea, no model. infra/agent/build-image.sh runs it inside the +# built image as well, where the interpreter and the script are the ones that will actually +# run in the cluster. +############################################################################### +set -uo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +WORKER="${PM_WORKER_PATH:-${HERE}/pm-worker}" +[ -r "$WORKER" ] || { echo "cannot read ${WORKER}" >&2; exit 2; } + +# A complete, VALID assignment. Each case below overrides exactly one thing, so a failure +# names the guard rather than a pile of missing variables. +BASE=( + PM_TASK_MODE=implement + PM_ISSUE_NUMBER=42 + PM_BRANCH=feat/some-real-slug + PM_BASE_BRANCH=main + PM_GITEA_URL=http://gitea.gitea.svc.cluster.local:3000 + PM_GITEA_OWNER=Home-Network + PM_GITEA_REPO=home-network-k3s-impl + PM_GITEA_USER=pm-agent + GITEA_TOKEN=not-a-real-token + CLAUDE_CODE_OAUTH_TOKEN=not-a-real-token +) + +PASS=0 +FAIL=0 + +# `env -i` so nothing from the invoking shell leaks in and accidentally satisfies a guard. +refuses() { + local label="$1" want="$2"; shift 2 + local out rc=0 + out="$(env -i PATH="$PATH" HOME=/tmp "${BASE[@]}" "$@" bash "$WORKER" 2>&1)" || rc=$? + if [ "$rc" -eq 0 ]; then + printf ' FAIL %s — exited 0; it should have been refused\n' "$label" + FAIL=$((FAIL + 1)) + return + fi + if printf '%s' "$out" | grep -qF "$want"; then + printf ' ok %s (exit %s)\n' "$label" "$rc" + PASS=$((PASS + 1)) + else + printf ' FAIL %s — refused (exit %s) but not for the expected reason %q:\n' "$label" "$rc" "$want" + printf '%s\n' "$out" | tail -3 | sed 's/^/ /' + FAIL=$((FAIL + 1)) + fi +} + +echo "pm-worker input-validation probe ($WORKER)" + +# An argument on a pinned command is caller-supplied code. There is no argument surface. +out="$(env -i PATH="$PATH" HOME=/tmp "${BASE[@]}" bash "$WORKER" --layer l2 2>&1)" || rc=$? +if printf '%s' "$out" | grep -qF "takes no arguments"; then + echo " ok arguments are refused (exit ${rc:-?})"; PASS=$((PASS + 1)) +else + echo " FAIL arguments are refused — got: $(printf '%s' "$out" | tail -1)"; FAIL=$((FAIL + 1)) +fi + +# --- the ref it may create and push --------------------------------------- +refuses "branch 'main' (CLAUDE.md §1)" "does not match" PM_BRANCH=main +refuses "branch with a wrong prefix" "does not match" PM_BRANCH=feature/x +refuses "branch with a shell metacharacter" "does not match" 'PM_BRANCH=feat/x;id' +refuses "branch with path traversal" "does not match" 'PM_BRANCH=../../etc/passwd' +refuses "branch too short for the slug rule" "does not match" PM_BRANCH=fix/ab +refuses "branch equal to the base branch" "must not be the base branch" \ + PM_BRANCH=feat/same-slug PM_BASE_BRANCH=feat/same-slug + +# --- where code comes FROM and goes TO ------------------------------------- +# The namespace's egress policy permits tcp/443 to the world (the model needs it), so an +# injected host would be REACHABLE. These are the cases that matter most. +refuses "a redirected git host" "is not an allowed git host" \ + PM_GITEA_URL=https://evil.invalid +refuses "a real, reachable, WRONG git host" "is not an allowed git host" \ + PM_GITEA_URL=https://github.com +refuses "the right host on a smuggled path" "is not an allowed git host" \ + PM_GITEA_URL=http://gitea.gitea.svc.cluster.local:3000/../.. +refuses "a malformed owner" "PM_GITEA_OWNER is malformed" \ + 'PM_GITEA_OWNER=../other-org' +refuses "a malformed repository" "PM_GITEA_REPO is malformed" \ + 'PM_GITEA_REPO=repo;id' + +# --- the assignment itself ------------------------------------------------- +refuses "an unknown task mode" "must be 'implement' or 'revise'" \ + PM_TASK_MODE=merge +refuses "a non-numeric issue number" "must be a positive integer" \ + PM_ISSUE_NUMBER=42x +refuses "issue number zero" "must be a positive integer" \ + PM_ISSUE_NUMBER=0 + +# --- the credentials ------------------------------------------------------ +refuses "a missing model credential" "CLAUDE_CODE_OAUTH_TOKEN is not set" \ + CLAUDE_CODE_OAUTH_TOKEN= +refuses "a missing git credential" "GITEA_TOKEN is not set" \ + GITEA_TOKEN= + +echo +if [ "$FAIL" -eq 0 ]; then + echo "probe OK — ${PASS} refusal(s), 0 failure(s)" + exit 0 +fi +echo "probe FAILED — ${FAIL} of $((PASS + FAIL)) case(s) did not refuse as expected" +exit 1 diff --git a/platform/pm-agent/README.md b/platform/pm-agent/README.md index dcfa95d..ddbbbbd 100644 --- a/platform/pm-agent/README.md +++ b/platform/pm-agent/README.md @@ -151,7 +151,9 @@ nobody enumerated — on the sibling policy that was `initContainers` (measured: compliant `containers[0]` and a privileged init container was **admitted**, pass 3 fail 0), then `ephemeralContainers`, then `env`, then `envFrom`. All of them are closed here from the start, and `platform/kyverno/tests/pm-agent/` proves each refusal offline: **28 assertions, -28 passing** under the pinned `kyverno` CLI. +28 passing** under the pinned `kyverno` CLI. The worker's own input guards have their own +offline probe — `infra/agent/probe-validation.sh`, **17 refusals** — because a guard nothing +exercises is a claim rather than a control. > The policy is applied by `platform-config/kustomization.yaml`, and it has to be: adding a > ClusterPolicy only to `platform/kyverno/admission/kustomization.yaml` is a **silent