From b4084ddb2b8101e02c95ffbbf74d2c4f7f70fa94 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Sun, 20 Sep 2026 19:36:32 +0000 Subject: [PATCH 1/2] fix(vendor): stop every vendored-tree PR conflicting on the provenance log (BLO-34872) `vendor/paperclip-adapter-claude-k8s/PROVENANCE.md` held two things that change on every PR touching the vendored tree: a 64-hex integrity hash over the tree, and an append-only per-PR justification table. Both hunks conflict between any two concurrent vendored-tree PRs. Measured on #1873: 4 rebases, PROVENANCE.md the only conflicting file in every one. `BEHIND` is free here (master's ruleset sets merge_queue with merge_method REBASE, so the queue rebases its own entries). `DIRTY` is what forces an agent round-trip, and a round-trip voids the at-head review attestation. The obvious fix -- `merge=union` on PROVENANCE.md -- would corrupt the guard silently: union keeps both sides' hash lines and CI reads `grep -oE '^[0-9a-f]{64}$' PROVENANCE.md | head -1`, so the provenance verdict would depend on merge ordering rather than on the tree, failing permissively on one of the two orderings. So the table moves to PROVENANCE-CHANGES.md and `merge=union` is scoped to that file alone, leaving the hash where a union cannot reach it. - vendor/.../PROVENANCE-CHANGES.md (new): the table, verbatim, plus the three rules that keep union safe (append-only, no 64-hex line, nothing below it). - .gitattributes (new, repo root): union on that one path. - pr.yml: exclusion regex extended to the new file, so the recorded hash is unchanged (verified: 7a91abbd... on both sides of this commit). - scripts/__tests__/provenance-union-merge.test.mjs: asserts the agreement the fix rests on -- every union-merged vendor file is excluded from the hash, no union-merged file carries a 64-hex line, PROVENANCE.md carries exactly one and is not itself union-merged, the doc's regenerate command matches CI's, and the regex names no file absent from the tree. Each of the six guards was mutation-tested individually: reverting any one alone turns the suite red. BLO-34872: https://paperclip.blockcast.net/BLO/issues/BLO-34872 Co-Authored-By: Paperclip --- .gitattributes | 10 ++ .github/workflows/pr.yml | 7 +- .../__tests__/provenance-union-merge.test.mjs | 130 ++++++++++++++++++ .../PROVENANCE-CHANGES.md | 55 ++++++++ .../PROVENANCE.md | 43 ++---- 5 files changed, 211 insertions(+), 34 deletions(-) create mode 100644 .gitattributes create mode 100644 scripts/__tests__/provenance-union-merge.test.mjs create mode 100644 vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000000..8dd5e8f131e0 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,10 @@ +# BLO-34872: the vendored adapter's per-patch log is append-only, so every PR +# touching that tree used to conflict with every other one on it — 4 rebases on +# PR #1873 alone, PROVENANCE.md the only conflicting file each time. Union merge +# keeps both sides' appended rows instead. +# +# Scoped to this one file deliberately. PROVENANCE.md itself must NOT be +# union-merged: it holds the 64-hex integrity hash that the vendor_claude_k8s CI +# step reads with `grep -oE '^[0-9a-f]{64}$' … | head -1`, and a union would keep +# both sides' hash lines, making the guard's verdict depend on merge ordering. +vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md merge=union diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 317c89fbe001..5403ea8be7d7 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -300,6 +300,11 @@ jobs: run: node --test ./scripts/__tests__/merge-group-concurrency.test.mjs timeout-minutes: 1 + - name: Test vendored-provenance union-merge invariants (BLO-34872) + if: ${{ !cancelled() }} + run: node --test ./scripts/__tests__/provenance-union-merge.test.mjs + timeout-minutes: 1 + - name: Test policy node-test timeouts if: ${{ !cancelled() }} run: node --test ./scripts/__tests__/policy-node-test-timeouts.test.mjs @@ -1214,7 +1219,7 @@ jobs: run: | set -euo pipefail actual=$(git ls-files \ - | grep -vxE 'LICENSE|PROVENANCE\.md' \ + | grep -vxE 'LICENSE|PROVENANCE\.md|PROVENANCE-CHANGES\.md' \ | LC_ALL=C sort | xargs sha256sum | sha256sum | cut -d' ' -f1) recorded=$(grep -oE '^[0-9a-f]{64}$' PROVENANCE.md | head -1) echo "actual: $actual" diff --git a/scripts/__tests__/provenance-union-merge.test.mjs b/scripts/__tests__/provenance-union-merge.test.mjs new file mode 100644 index 000000000000..e97437108d3b --- /dev/null +++ b/scripts/__tests__/provenance-union-merge.test.mjs @@ -0,0 +1,130 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { test } from "node:test"; + +// BLO-34872. The vendored adapter's per-patch log is append-only, so before the +// split every PR touching that tree conflicted with every other one on it -- +// 4 rebases on PR #1873 alone, PROVENANCE.md the only conflicting file each +// time. The fix is `merge=union` on the log file, scoped to that file only. +// +// The whole safety of that fix rests on two agreements that nothing else +// checks, and both fail silently: +// +// 1. a union-merged file must be excluded from the integrity hash, or the +// next append breaks the vendor_claude_k8s job for reasons nobody will +// connect to `.gitattributes`; +// 2. a union-merged file must never contain a 64-hex line, or a union keeps +// BOTH sides' lines and CI's `grep ... | head -1` picks whichever sorts +// first -- a provenance verdict decided by merge ordering rather than by +// the tree, which fails permissively on one of the two orderings. +// +// Rename the log file in one place and not the other and both agreements break +// with a green build. Hence this test rather than a comment. + +const repoRoot = new URL("../../", import.meta.url); +const VENDOR_DIR = "vendor/paperclip-adapter-claude-k8s"; + +const read = (p) => readFileSync(new URL(p, repoRoot), "utf8"); +const HEX64 = /^[0-9a-f]{64}$/m; + +/** Paths marked `merge=union` in the repo-root .gitattributes. */ +function unionMergedPaths() { + return read(".gitattributes") + .split("\n") + .map((l) => l.trim()) + .filter((l) => l && !l.startsWith("#")) + .map((l) => l.split(/\s+/)) + .filter(([, ...attrs]) => attrs.includes("merge=union")) + .map(([path]) => path); +} + +/** + * The alternation the vendor_claude_k8s job excludes from the integrity hash, + * read out of the workflow itself so a drift between the two is a failure here + * rather than a surprise in CI. + */ +function ciExclusionAlternatives(source, label) { + const m = source.match(/grep -vxE '([^']+)'/); + assert.ok(m, `${label}: no \`grep -vxE '...'\` exclusion found`); + return m[1].split("|"); +} + +const workflow = read(".github/workflows/pr.yml"); +const provenance = read(`${VENDOR_DIR}/PROVENANCE.md`); + +test("every union-merged vendor file is excluded from the integrity hash", () => { + const union = unionMergedPaths().filter((p) => p.startsWith(`${VENDOR_DIR}/`)); + assert.ok( + union.length > 0, + "expected at least one merge=union path under the vendored tree; if the log file was renamed, update .gitattributes", + ); + + // The CI step runs with working-directory: vendor/..., so `git ls-files` + // emits vendor-relative paths and the regex is written against those. + const excluded = ciExclusionAlternatives(workflow, ".github/workflows/pr.yml"); + + for (const path of union) { + const relative = path.slice(`${VENDOR_DIR}/`.length); + const escaped = relative.replace(/\./g, "\\."); + assert.ok( + excluded.includes(escaped), + `${path} is merge=union but the vendor_claude_k8s exclusion regex does not name '${escaped}' (it has: ${excluded.join("|")}). A union-merged file inside the hash breaks the provenance job on the next concurrent append.`, + ); + } +}); + +test("PROVENANCE.md's documented regenerate command matches the one CI runs", () => { + // Two copies of the same alternation; a reader who follows the doc and gets a + // different hash than CI has no way to tell which is wrong. + assert.deepEqual( + ciExclusionAlternatives(provenance, `${VENDOR_DIR}/PROVENANCE.md`), + ciExclusionAlternatives(workflow, ".github/workflows/pr.yml"), + ); +}); + +test("no union-merged file carries a 64-hex line, and PROVENANCE.md carries exactly one", () => { + for (const path of unionMergedPaths()) { + const matches = read(path).split("\n").filter((l) => HEX64.test(l)); + assert.deepEqual( + matches, + [], + `${path} is merge=union and contains a 64-hex line. A union keeps both sides' copies, so CI's \`grep -oE '^[0-9a-f]{64}$' | head -1\` would resolve by sort order instead of by the tree.`, + ); + } + + const hashLines = provenance.split("\n").filter((l) => HEX64.test(l)); + assert.equal( + hashLines.length, + 1, + `expected exactly one integrity hash line in ${VENDOR_DIR}/PROVENANCE.md, found ${hashLines.length}`, + ); +}); + +test("PROVENANCE.md itself is not union-merged", () => { + assert.ok( + !unionMergedPaths().includes(`${VENDOR_DIR}/PROVENANCE.md`), + "PROVENANCE.md holds the integrity hash; union-merging it is the exact failure this split exists to avoid", + ); +}); + +test("the exclusion regex names no file that is absent from the vendored tree", () => { + // Catches the other half of a rename: the regex still excluding the old + // filename would leave a stale hole in the hash. + const tracked = new Set( + execFileSync("git", ["ls-files"], { + cwd: new URL(`${VENDOR_DIR}/`, repoRoot), + encoding: "utf8", + }) + .split("\n") + .filter(Boolean), + ); + + for (const alternative of ciExclusionAlternatives(workflow, ".github/workflows/pr.yml")) { + const filename = alternative.replace(/\\/g, ""); + assert.ok( + tracked.has(filename), + `the vendor_claude_k8s exclusion regex names '${filename}', which is not tracked under ${VENDOR_DIR}. A stale exclusion silently drops a real file from the integrity hash.`, + ); + } +}); diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md new file mode 100644 index 000000000000..2f339cc95f43 --- /dev/null +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md @@ -0,0 +1,55 @@ +# Local modifications — `paperclip-adapter-claude-k8s` + +Append-only log of the Blockcast patches applied to the vendored tree. Split out +of [PROVENANCE.md](./PROVENANCE.md) under +[BLO-34872](https://paperclip.blockcast.net/BLO/issues/BLO-34872): every PR that +touches the vendored tree appends a row here, so before the split any two +concurrent PRs conflicted on this table and one of them had to rebase — which +voids its at-head review attestation and costs a full review round. + +The repo-root `.gitattributes` marks **this file only** as `merge=union`, so +concurrent appends merge without a conflict. Three rules keep that safe: + +1. **Append at the end. Never edit or reorder existing rows.** Union merge + resolves by keeping both sides' added lines; it cannot reconcile an edit. +2. **Never put a 64-hex string in this file.** The integrity hash lives in + `PROVENANCE.md`, which is *not* union-merged, precisely so that a union can + never introduce a second candidate line for + `grep -oE '^[0-9a-f]{64}$' … | head -1`. +3. **Nothing goes below the table.** Trailing prose turns every append into an + interior edit. + +This file is excluded from the integrity hash (it is a Blockcast addition, not +upstream source), same as `LICENSE` and `PROVENANCE.md`. That exclusion is +asserted by `scripts/__tests__/provenance-union-merge.test.mjs`. + +| commit | files | what | +|---|---|---| +| `cd1630512` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Anchored the `SAFE_ENV_INSPECTION_RE` safe-helper exception to a whole-command invocation. It was evaluated before the full-dump detector and matched the helper anywhere in the command, so a ` && ` compound returned `allow` and executed the dump. Addresses an Ally review finding on Blockcast/paperclip#1092. | +| `cd1630512` | `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts` (new) | Keyed the `getSelfPodInfo()` cache by (kubeconfig path, namespace, hostname). It memoized into one process-global slot while callers pass a per-request kubeconfig, leaking the first execution's image, scheduling, PVC, env and Secret references into later executions against a different cluster. Same review. | +| `8f4f7262a` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Treated `\r`/`\n` as command separators in both classifier copies. Anchoring the helper exception (above) closed the `&&`/`;`/`\|` compounds but not a literal newline: JS `$` without `m` is end-of-input and the argument tail's `\s` spanned newlines, so `paperclip-safe-env\nenv` was a whole-command match, and the dump detector did not treat `\n` as a boundary either. Follow-up on the same Ally review of Blockcast/paperclip#1092. | +| `551c461ef` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Two further dump forms in both classifier copies. (a) Flag-only dumps: `env`/`printenv` stop dumping only when given an *operand*, so requiring a boundary immediately after the utility name let `-0`, `--null` and `-u NAME` through; an option run is now consumed, with `-u`/`--unset` matched together with their argument. (b) Command substitution was never a boundary, so `echo "$(env)"`, `X=$(printenv)` and backtick forms were allowed with no flags at all. Third Ally review pass on Blockcast/paperclip#1092. | +| `551c461ef` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Made the init container's `data` mount conditional on a claim (an unconditional mount named an undeclared volume, which Kubernetes rejects for the whole Pod), and validated + shell-quoted `providers.anthropic.accounts` before interpolating it into the main container's `sh -c`. Same review pass. Both were revised again in `3e0244a78` below. | +| `435219ccf` | `src/server/env-guard.ts` | Comment-only correction. The header claimed behavioural parity with `server/src/agent-shell-guard.ts` "locked by `env-guard.test.ts`". Both halves were false — the test never imports that file and nothing imports it in production; it is dead code, then four fixed bypasses behind. Tracked for removal-or-resync as BLO-22840. | +| `3e0244a78` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Closed the unquoted-command-wrapper bypass class. `SHELL_WRAPPER_RE` unwraps only a *quoted* `-c` payload and whitespace was not a command boundary, so a dump passed as a bare argument to any wrapper (`sh -c env`, `eval env`, `xargs env`, `nohup env`, `timeout 5 env`, `su -c env`, ...) was allowed — 9 of 9 measured payloads, in the real spawned pod script. Split the boundary class: whitespace joins the *leading* class only, while the trailing terminator stays punctuation-only so operand-bearing forms (`env NAME=value cmd`, `printenv HOME`, `grep env file`) stay allowed. Fourth Ally review pass. | +| `3e0244a78` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Three manifest fixes from the same review. (a) Operator-configured mount paths (`workspaceMountPath`, `homeRoot`) reached the init container's `sh -c` unquoted via `browserHome`; now quoted at every site plus a new `assertSafeAbsolutePath` as an independent second defence. (b) A configured account pool with no valid entry fell back to ccrotate's *global* rotation — fail-open, widening credential scope on a config typo; absent and invalid configuration are now distinguished. (c) The `data` volume is now ALWAYS declared (PVC-backed, else `emptyDir`), because the conditional mount from `551c461ef` merely moved the no-PVC failure from admission to an EACCES `mkdir` as runAsUser:1000. | +| `b80b69218` | `src/server/env-guard.ts` | Converged shell unwrapping with `server/src/agent-shell-guard.ts`, adopting its `SHELL_COMMAND_PREFIX_RE` + `readShellCommandArgument` (a human closed the same unquoted-wrapper bypass there in `993bf304c`). Belt-and-braces with the boundary widening in `3e0244a78`: unwrapping is more precise for `sh -c`, the boundary rule is the only thing that reaches non-shell wrappers. Also corrected this file's header claim that the sibling copy was merely "four bypasses behind" — the divergence runs both ways. | +| `85f99a85b` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/execute.test.ts` | Pinned `ANTHROPIC_CUSTOM_HEADERS` into a new `ALWAYS_SECRET_ENV_NAMES` set. `SENSITIVE_ENV_NAME_RE` matches none of its tokens, so the var shipped as a literal `env[].value` despite carrying arbitrary forwarded header lines — including, in principle, an `Authorization:` line set through `adapterConfig.env` or the Penstock session stamp. Routing and both fail-closed guards key off `isSensitiveEnvName()`, so the pin covers all three. BLO-21858, from the BLO-21593 independent review of upstream PR #31 (probe 6). | +| `e1b28276f` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Replaced the boundary-regex classifier with a shell-aware normalizer, in both copies. Five prior rounds each closed one boundary bypass; the fifth Ally review found three more (`env >&2`, `e''nv`, `env -S '-u PATH'`). Re-measured against the real spawned pod script the class was wider than reported: 10 of 12 probe payloads classified `allow` while `/bin/sh` emitted a marker variable, including `e"n"v`, `\env`, `'env'`, `env>&2`, `env 2>&1` and `env -S '-0'`. The cause is structural, not a missing character class — a regex matches command *text*, but the shell executes the command after quote removal, escape processing, redirection stripping and GNU `env -S` re-splitting, so the matched string is not the token that runs. The command is now lexed as a shell would and the resulting words are classified, so spelling variants collapse to one word. The hand-maintained second case list for the embedded copy — the mechanism by which the two copies drifted — is replaced by a differential that drives the whole corpus through both. Fifth Ally review pass on Blockcast/paperclip#1092. | +| `e1b28276f` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Two fail-closed fixes from the same review. (a) A configured account pool of the wrong *shape* (`accounts: "a@example.test"` rather than a list) was collapsed into the same `null` used for "absent" by `Array.isArray(...) ? ... : null`, so it read as unconfigured and selected unrestricted *global* ccrotate rotation — the same credential-scope widening `3e0244a78` fixed for the all-invalid case, still reachable by the likeliest possible typo. Presence is now tested separately from validity at both `providers.anthropic` and `.accounts` (`parseObject` returns `{}` for any non-object, so both levels shared the defect), an explicitly empty pool counts as configured-but-unusable, and diagnostics report the offending TYPE only — never the value, which sits next to credential material. (b) `workspaceMountPath` could equal a mount this builder already emits (`/tmp/prompt`, `/runtime-cache`, an inherited secret mount); those are shape-valid so `assertSafeAbsolutePath` passed them, and the duplicate mountPath yields a Pod Kubernetes rejects outright. Rejected at construction with a message naming the conflict, plus a per-container invariant assertion that backstops mounts appended later (`/var/run`, `prompt-secret`, `mcp-config-secret`) and the init container's independently-built list. Nested paths stay legal. | +| [#1368](https://github.com/Blockcast/paperclip/pull/1368) | `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Carried the source volume's `items:` key selector through propagation. `getSelfPodInfo()` captured only `secretName`/`mountPath`/`defaultMode`, and the mount site rebuilt the volume without a selector, so a source mount projecting ONE key out of a multi-key Secret was re-expanded into EVERY key of that Secret on the agent Job pod. Measured live: `paperclip-api` projects `gbrain-plugin-service-key` alone out of `authbot-mcp-consumer-service-keys`, while agent pods received all 7 keys — agents held more key material than the container the mount was copied from. `optional: true` stays hardcoded at the mount site by design, so a Secret absent in the agent namespace still cannot hard-fail the Job. Refs [BLO-18927](https://paperclip.blockcast.net/BLO/issues/BLO-18927) AC-3; does **not** close [BLO-22514](https://paperclip.blockcast.net/BLO/issues/BLO-22514), which needs the allowlist. | +| [#1377](https://github.com/Blockcast/paperclip/pull/1377) | `src/server/inherit-allowlist.ts` (new), `src/server/inherit-allowlist.test.ts` (new), `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/env-guard.ts` | Allowlisted what agent Job pods inherit from the paperclip server pod. `getSelfPodInfo()` snapshotted the server's ENTIRE env — every literal, every `valueFrom` including `secretKeyRef`, every `envFrom` and every mounted secret volume — with no filter, and `job-manifest.ts` replayed all of it onto every agent Job, so each agent container held `PAPERCLIP_AGENT_JWT_SECRET` (mint an API key for ANY agent), `DATABASE_URL` (bypass the API and all of `authorization.ts`) and `GITHUB_APP_PRIVATE_KEY`. Filtered at `getSelfPodInfo()` rather than at the four replay sites, so a future replay site cannot reintroduce the leak by forgetting to filter, plus a fail-closed `findServerOnlyEnvVarsInPodSpec` backstop in `buildJobManifest` because `SelfPodInfo` is a plain object callers can construct unfiltered. Keep-set derived from actual by-name reads plus an agent-pod consumer sweep — not pattern-matched — and both directions unit-tested, since a filter that dropped everything would pass a deny-only suite while breaking every run in the fleet. Measured against the live server env: 54 vars in, 24 inherited, 30 dropped, 0 control-plane credentials remaining. `env-guard.ts` is comment-only: records the BLO-22514 decision to keep that hook fail-OPEN. Closes [BLO-22514](https://paperclip.blockcast.net/BLO/issues/BLO-22514). | +| [#1411](https://github.com/Blockcast/paperclip/pull/1411) | `src/server/inherit-allowlist.ts`, `src/server/inherit-allowlist.test.ts`, `src/server/k8s-client.test.ts` | Removed `paperclip-github-merge-token` (the `@allyblockcast` USER seat, id 296676656) from `AGENT_SECRET_VOLUME_ALLOWLIST`, so it no longer propagates from the server pod into agent Job pods. That seat's approvals SATISFY required review on repos whose ruleset names the Ally team (onprem-k8s, penstock-llm-proxy-core), so propagating it made "can clear branch protection" a fleet-wide capability — measured live at **108 agent Job pods** mounting it — rather than one service's. It was also unusable from an agent by construction, i.e. exposure with no function: the `gh` wrapper resolves `PAPERCLIP_GITHUB_TOKEN_FILE` (pinned to the App token at `/paperclip/.secrets/github-token/token`, never the seat path), `GH_TOKEN`/`gh auth`/`--with-token` overrides are no-ops because that wrapper re-reads the file per invocation, and shipped skills are forbidden from naming the seat path by `CREDENTIAL_SELECTOR_PATTERNS` in `packages/skills-catalog/src/shipped-catalog.test.ts`. Measured across 240 PRs in onprem-k8s, penstock-llm-proxy-core, paperclip and multicast: the seat authored 0 and pushed 0 (authorship is 100% the App) and merged 11, a path the App already covers. The CONTROL PLANE keeps the mount via `deploy/helm/paperclip/values.blockcast.yaml`, where the dedicated reviewer service that legitimately uses this identity runs — only the agent-Job propagation is removed. Two `k8s-client` tests used the seat as their example of a KEPT volume and were re-pointed at the App token; the base fixture now mounts both, deliberately keeping the seat so the allowlist is exercised against a realistic server pod rather than one curated to contain only inheritable volumes. Companion to the org-side half of [BLO-24056](https://paperclip.blockcast.net/BLO/issues/BLO-24056) (seat dropped to `read` on all 11 in-scope repos). | +| [BLO-25403](https://paperclip.blockcast.net/BLO/issues/BLO-25403) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/config-schema.ts`, `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/execute-environment.test.ts` | Ported upstream `94c97d01d408155a5c173c43ab42304f688e7ce3` (merged as [kkroo#32](https://github.com/kkroo/paperclip-adapter-claude-k8s/pull/32) `c5d1389f`) — the BLO-21812 fix, which the 2026-08-06 vendoring **stranded**: it was authored against a branch that was not part of the `3ad3370`+`35f1eb2`+`6ddd4b0` composition, so it never entered the build path and `CLAUDE_K8S_REF` was retired out from under it. A new `resolveServiceAccountName()` resolves per-agent config → `PAPERCLIP_DEFAULT_SERVICE_ACCOUNT_NAME` (fleet default) → **throw**, replacing `asString(config.serviceAccountName, "") \|\| undefined`, which omitted the key and let Kubernetes admission silently assign the namespace's bare `default` SA — an identity with no cluster-scoped read, and a full misdiagnosed incident ([BLO-21499](https://paperclip.blockcast.net/BLO/issues/BLO-21499)). The resolved SA is echoed on `JobBuildResult`, into the run log and into invocation metadata so identity is attributable without a cluster read. Ported by hand rather than cherry-picked: 4 of 6 files applied clean, but `execute.ts` and the `buildJobManifest` return had drifted under `551c461ef`/`3e0244a78`/`e1b28276f` (`envSecret`, `mcpConfigSecret`), so those two hunks were reapplied against current code. No RBAC object is created or modified. Two cases beyond upstream's pin the load-bearing `.trim()` on both resolution branches — `serviceAccountName` is a `type: "text"` field, so a whitespace-only value is reachable from the UI form and a bare `\|\|` would emit it as a Job SA name the API server rejects (Ally review suggestion on [#1409](https://github.com/Blockcast/paperclip/pull/1409)). | +| [BLO-29804](https://paperclip.blockcast.net/BLO/issues/BLO-29804) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Made the env classification declarative and gated it in CI. `SENSITIVE_ENV_NAME_RE` is fail-closed against over-matching but fail-**open** against a credential-carrying variable whose name matches none of its six patterns — the `ANTHROPIC_CUSTOM_HEADERS` row above is the proof, and pinning names one at a time only fixes the instances someone notices. A new exported `ENV_NAME_CLASSIFICATION` table declares every env name this file can emit as `SECRET` or `SAFE_LITERAL` with a stated reason, and `ALWAYS_SECRET_ENV_NAMES` is now *derived* from it, so declaring a name `SECRET` there is what pins it and the table is the single source of truth. The forcing function is a test suite over the full 2×2×2×2 permutation of isolation / DinD / `adapterConfig.env` / `mcpServers`: an emitted name absent from the table reddens the lane **naming the variable**, so a new env var fails CI in the pull request that introduces it instead of inheriting a default. Coverage alone is not enough, so two further assertions exist — one requires classification and `isSensitiveEnvName` to agree on every *emitted* name, closing the hole where a prefix family (`PAPERCLIP_WORKSPACE_`) would silently absolve a future `PAPERCLIP_WORKSPACE_AUTH_TOKEN` as `SAFE_LITERAL` while the routing correctly Secret-backs it; the other asserts the Secret-backed name set for a fixed context is identical to the pre-change set, so the classification cannot silently move a var between literal and `secretKeyRef`. **No runtime behaviour change** — `isSensitiveEnvName()` keeps its semantics (regex ∪ pinned), all three `SECRET` entries already matched the regex or were already pinned, and the diff is classification plus tests. The three operator-supplied channels (`adapterConfig.env`, `selfPod.inheritedEnv`, `selfPod.inheritedEnvValueFrom`) are deliberately out of the table because their names are data rather than code; the second is separately governed by `AGENT_ENV_ALLOWLIST` in `inherit-allowlist.ts`. Records the decision on [BLO-21858](https://paperclip.blockcast.net/BLO/issues/BLO-21858) remedy (2): inverting to "Secret-backed unless declared safe-literal" is **declined** on measured cost/benefit — 8 `secretKeyRef` vars against 37–41 literals per live `ac-*` pod, so inversion moves ~40 operationally load-bearing fields (`HOME`, `TMPDIR`, `PAPERCLIP_RUN_ID`, the isolation roots) into an opaque Secret and stops `GET Pod` being a triage tool, on a code path that templates every agent Job with no staging tier. Full reasoning and the counter-argument on [BLO-29804](https://paperclip.blockcast.net/BLO/issues/BLO-29804). | +| [#1525](https://github.com/Blockcast/paperclip/pull/1525) | `src/server/parse.ts`, `src/server/parse.test.ts`, `src/server/execute.ts` | Made `skill_not_found` reachable and stopped model prose reaching it ([BLO-7991](https://paperclip.blockcast.net/BLO/issues/BLO-7991) AC3). `isClaudeSkillNotFoundStartupFailure` scans the RAW transcript for `Skill "" not found`, so any run whose transcript merely QUOTED that phrase — model prose, a tool result, this very issue's own body — was classified as a startup skill death. That code is in `NON_RETRYABLE_CONTINUATION_ERROR_CODES` and excluded from the zero-token reset, so a misclassification is **permanent** retry suppression, not a visible error. Guarded on the same surface the scan reads (a parsed signal cannot bound what a raw regex sees), then widened once more when a pre-assistant `user` event — which carries `tool_result` text and has no branch in `parseClaudeStreamJson` at all — proved to slip past an `assistant`-only check. Recorded here retroactively: #1525 updated the integrity hash but added no row and did not bump `-blockcast.N`, which the two rules at the foot of this section require. | +| [BLO-31794](https://paperclip.blockcast.net/BLO/issues/BLO-31794) | `src/server/parse.ts`, `src/server/parse.test.ts` | Inverted that guard from a **blocklist** of stream-json event types to an **allowlist** of harness-authored ones, so an unrecognised type fails closed. The row above widened the same predicate twice for one reason: `parseClaudeStreamJson` branches on exactly three types (`system`+`init`, `assistant`, `result`) and ignores every other one, so each newly-appearing event shape slipped a role-blocklist by default. The hazard was one config edit from live rather than hypothetical — `job-manifest.ts` appends `config.extraArgs` to the CLI argv verbatim (read as `asStringArray(config.extraArgs)`, appended by `claudeArgs.push(...extraArgs)`, from an agent's `adapterConfig`), so `--include-partial-messages` on any single agent re-opens the guard with no code change, no diff and no review. Measured on the CLI this adapter runs (v2.1.210): that flag emits 9 `stream_event`s for a two-word prompt, each wrapping model prose in `event.delta.text_delta`, and `stream_event` was enumerated by no previous version of the guard. The allowlist is `{system, rate_limit_event}` with a stated membership criterion (payload must be entirely harness-authored scalars); `result` is deliberately excluded because a *truncated* one can reach the scan carrying the model's final message. Scoped per line and reading only the first `"type"` per line, so a nested type cannot veto its own line — measured as defence-in-depth rather than a live fix, since a real 1717-byte `init` line carries exactly ONE `"type"` (its `mcp_servers` entries are `{name, status}`, `output_style` a bare string). Detection is unchanged: the four existing guard cases pass unmodified, plus new cases for a production-shaped `init` line, `system:status` (which v2.1.210 emits pre-turn under `--include-partial-messages`), and `rate_limit_event` (the FAR-32 repro in `execute.test.ts`). Verified as a negative control — the new `stream_event` case FAILS against the previous blocklist while all 12 detection-preserving cases pass, so it discriminates the fix rather than merely passing alongside it. **Ally review follow-up on [#1650](https://github.com/Blockcast/paperclip/pull/1650):** the allowlist reaches the `system` *subtype* rather than admitting the type wholesale, because `system` is a multiplexer and admitting it whole reproduced this same defect one level down. Measured against the v2.1.210 binary, `system` carries at least `init`, `status`, `compact_boundary`, `hook_started`, `hook_response` and `mcp_status`; only the first two are admitted. `hook_response` is why this is a live hole rather than future-proofing — the binary builds it as `{type:"system",subtype:"hook_response",…,output,stdout,stderr}`, embedding a hook process's raw stdout, which is operator-configured. It is not merely reachable via `--settings`/`extraArgs`: **Paperclip provisions hooks itself**, and real pod logs on this instance carry a `SessionStart` `hook_response` whose `output` is an operator status message, and another whose `stdout` is an nginx 503 HTML page. (`hook_error`, listed in the review, appears in no v2.1.210 string table and is not a subtype at this version.) A `system` line with no readable subtype fails closed. **Second review round — the subtype gate as first shipped silently disabled detection in production, and this row previously claimed otherwise.** Because Paperclip provisions a `SessionStart` hook, `hook_started`/`hook_response` open the transcript *before* `init` on the large majority of real runs — measured on this instance's pod logs at **6510 of 8036** `init`-carrying logs (81%), with the hook line preceding `init` in **399/399** of a sample carrying both. A whole-transcript "every line must be harness-authored" veto therefore returned `false` on all of them, and the suite stayed green only because its sole positive fixture was a synthetic two-line shape no production run has — precisely the "fix the false positive by disabling detection entirely" failure mode this issue's own acceptance criteria warn about. The predicate now **attributes the phrase to its line** instead of demanding a globally clean transcript: the trigger phrase counts only when it sits on a line the harness authored (an allowlisted event, or a bare non-event line, which in stream-json mode is the CLI speaking outside the protocol; 0 of 6893 sampled production lines are bare, and the third review round below records why that is structural rather than incidental). Every false positive in this family is the phrase *inside* an event payload, so attribution is the more faithful invariant and unknown types still fail closed. Detection now genuinely survives the production preamble, pinned by four new cases (full preamble; each hook line alone; an untrusted event *after* the death), and the negative-control discipline is unchanged — the production-preamble case FAILS against the whole-transcript veto. One deliberate narrowing is recorded in the source: the phrase regex's `\s+` can span a newline, so a phrase straddling two lines would no longer match; the CLI emits it on one line, and the direction is the safe one. Also note this row's own earlier "measured as defence-in-depth rather than a live fix" framing applied to *nested types on an init line*, which remains accurate; it did not license the detection-loss claim. The phrase test still runs before the per-line walk as a pre-filter — the walk re-tests each line and is what decides — skipping an eager `split` of the pod log's stdout stream on the common phrase-absent failure. **Third review round — [BLO-31955](https://paperclip.blockcast.net/BLO/issues/BLO-31955), comment-only:** the bare-line trust (`if (!match) return true`) is now recorded in the source as a **structural** invariant rather than an empirical one, because the sample was the weaker of the two available justifications. The surface the predicate reads has exactly one writer — the `tee` in the pipeline `job-manifest.ts` builds in `claudeInvocation`, `cat … | claude … | tee | > /dev/null` (stage 2 is written as `claude` here for the round in which it was recorded; the fourth round below corrects it to `launcherCommand`), which carries no `2>&1` on any stage — so it receives stage 2's stdout and nothing else; hook stderr, MCP-server stderr and the fail-fast `[wrapper]` line (`failFastFilter`, written to `/dev/stderr` and downstream of the `tee` regardless) bypass it by construction, as does the prompt. Verified end to end rather than taken from the review: `podLogPath` has that one writer, `stdout` is assigned only from it (in `execute.ts`, the `tailResult.value` tail via `fs.open` and the `stdout = onDisk` re-read, so both parse paths share one single-writer file), and the one reader of the *merged* container-log stream (`readPodContainerLogTail`, in `execute.ts`, via `readNamespacedPodLog`) is confined to diagnostics and never reaches the parse surface. The source now names the three things that void the invariant — adding `2>&1` before the `tee`, routing a merged container-log read into the parse surface, and pointing `adapterConfig.agentCommand` at a launcher that writes any line of its own to stdout — which is the point of the change: every prior iteration in this family (BLO-7991 → #1525 → BLO-31794) was an invisible widening from elsewhere, and naming them converts a fifth one into a reviewable diff. The 6893-line sample is retained as corroboration, not as the basis. One predicate change rode along, from the review of this round: `status` was removed from the `system` subtype allowlist. Its admission had been argued under the round-two whole-transcript veto ("compaction cannot occur before the first turn, so any such transcript also carries an `assistant` line, which the guard rejects"), and per-line attribution made that rejection stop happening — a `status` line whose `compact_result`/`compact_error` (compaction summaries derived from model output) quoted the phrase would have classified `skill_not_found`, a permanent retry suppression. Nothing was lost: in `init -> status -> death` the death is the bare line, trusted on its own, and the existing case for that shape still passes; a new negative case pins the `compact_result` false positive, and it fails against the pre-change allowlist. **Fourth review round, same PR:** the enumeration shipped at two and was incomplete — stage 2 of that pipeline is `launcherCommand`, not `claude`, resolved from `validateAgentCommand(config.agentCommand, "claude")` against an operator-editable `adapterConfig` text field ("Agent Launcher", `config-schema.ts`). It touches the *other* premise rather than the single-writer one: `tee` carries no `2>&1` whichever binary feeds it, but "a bare line is the CLI speaking outside the protocol" is a claim about the behaviour of the binary writing stdout, and that binary is configurable. It needs no code edit, no diff and no review, which is what makes it live regardless of how often it is actually used — so no deployment claim is load-bearing here, and the sixth round below withdraws the one that was. Corroborated, not established, by a dated instance reading: **14 of 15** `claude_k8s` agents carried a non-default `adapterConfig.agentCommand` (all `/opt/penstock/bin/penstock-agent-runtime.mjs`) on 2026-09-16; re-measure by counting agents whose `adapterConfig.agentCommand` differs from `"claude"`, after confirming the field is visible at all. Ally's fifth-round review measured `adapterConfig: {}` for all 15 through `paperclipListAgents` and correctly declined to assert the count false on that basis; the sixth round re-ran the same tool from a caller with config visibility and reproduced 14 of 15 exactly, so the discriminator is the caller rather than the tool — a `{}` read is no config visibility, not a rotted count, and the recipe now says so. No live false positive is claimed; the launcher is expected to be a stream-json passthrough and `job-manifest.ts` says so where it sets `PENSTOCK_AGENT_COMMAND` ("the launcher owns provider credentials and starts the native Claude protocol itself"). Recording it converts that unstated assumption into a reviewable premise, which is this row's whole purpose — a proxy surfacing an upstream error body on stdout is the same shape as the nginx 503 page already documented above for `hook_response`. **Sixth review round, same PR — a citation that read plausibly and did not support its claim, which is the exact failure class this row exists to close.** The fourth round argued vector 3 was "the *normal* configuration ... which this repository shows on its own without reference to any instance", citing `Dockerfile.agent` and `docs/runbooks/penstock-claude-local-rollout.md`. Both files exist; neither shows it, and the runbook states the opposite — it is a staged-rollout procedure whose Scope reads "It does **not** activate either feature fleet-wide ... The first deployment is one non-production Job", whose Hard boundaries read "Configure **one named non-production agent** ... Do not bulk-patch a company, adapter type, or fleet default", and which explicitly forbids the inference that was drawn from the image: "**Do not treat a green image build as activation.**" `Dockerfile.agent:16` copies the launcher binary in, establishing AVAILABILITY; selection is per-agent `adapterConfig`, which is not in this tree at all, and `validateAgentCommand(config.agentCommand, "claude")` defaults to exactly `claude`. The claim is withdrawn from both the source block and this row rather than re-argued, because vector 3 never needed it: an operator-editable field requiring no code edit, no diff and no review is live whatever today's usage is, which the preceding clause already said. Two things fall out. The dated instance count is now the only empirical statement in the block and is labelled as corroboration, which is what it always was — the disputed claim was the *repository* inference, not the number. And with both citations withdrawn, every anchor in the block is once more inside `vendor/paperclip-adapter-claude-k8s/`, i.e. inside the 41-file integrity hash, so the `vendor_claude_k8s` lane mechanically catches drift in all of them; the fourth round had put two anchors one directory outside enforcement range, where a rename at rollout completion would have reddened nothing. | +| [#1669](https://github.com/Blockcast/paperclip/pull/1669) | `src/server/prompt-cache.ts`, `src/server/prompt-cache.test.ts`, `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/execute-environment.test.ts` | Classified the OTHER path into BLO-7991's pathology, which the row above cannot see. [BLO-32055](https://paperclip.blockcast.net/BLO/issues/BLO-32055): a live run died with a bare Node `ENOENT ... open '<...>/__runtime__//SKILL.md'` and was reported as the anonymous `adapter_failed` — an agent-pool/adapter fault for what is a skill configuration fault, invisible to skill-health sweeps. The reader was not a skill loader but `hashPathContents`, which walks each declared skill's tree to derive the prompt-bundle **cache key**; its `readFile` was unguarded. That walk runs inside `prepareClaudePromptBundle`, i.e. BEFORE the CLI is spawned, so `stdoutExcerpt` and `stderrExcerpt` were both null and there was no `parsed`, no result event and no transcript — every classifier in `parse.ts` reads a Claude-CLI-authored surface, so all of them are **structurally** blind to it. Not a too-narrow regex and not a regression of #1525: a second real path into the same user-visible failure, on a layer #1525 never inspects. The file's absence is transient — `company-skills.ts materializeRuntimeSkillFiles` refreshes by `fs.rm(recursive)` -> `mkdir` -> per-file `writeFile`, so the sweep publishes a window where the directory exists and `SKILL.md` does not; measured live, the file appeared **43m36s** after the run died on it. Routing this to `skill_not_found` would therefore have been the WRONG fix — that code is in `NON_RETRYABLE_CONTINUATION_ERROR_CODES`, so it converts a self-healing condition into permanent retry suppression, the same over-suppression hazard as the two rows above. Split instead on the source of truth (which skill owns the path, never message text, so the BLO-31794 false-positive class cannot reach this branch): a catalog-backed key yields a new transient `skill_materialization_pending` and a non-catalog-backed one keeps the permanent `skill_not_found`. The new code joins `TRANSIENT_INFRA_CONTINUATION_ERROR_CODES` — **the set that already contained `adapter_failed`** — so retryability is preserved exactly rather than widened. The discriminator is load-bearing rather than cosmetic because `readPaperclipRuntimeSkillEntries` silently switches source: it returns the server-injected catalog entries OR, when config carries none, the adapter's own bundled on-disk skills, a read-only image path where a missing file is a packaging fault no retry can fix. Hashing is still fatal and the error re-thrown rather than swallowed — a half-written tree hashed into a key would mint a bundle whose skills are silently incomplete, which is BLO-7991's original harm traded for a failure nobody sees. **Ally review follow-up:** `readCatalogBackedSkillKeys` is now a literal transcription of the key-deriving half of `normalizeConfiguredPaperclipRuntimeSkills` (`server-utils.ts:2598`) rather than an approximation, which was wrong in both directions — `asString` falls back on an EMPTY string and not merely on a non-string, so `{key:"", name:"x"}` normalizes upstream to key `x` while a `typeof key === "string"` test resolved it to `""` and dropped the entry, marking a catalog-backed skill un-backed (permanent suppression, the one direction this change exists to avoid); and upstream DISCARDS any entry missing `runtimeName` or `source`, which the hand-rolled version contributed anyway, letting a source-less entry colliding with a bundled key mark an image-path fault retryable. Deriving from the same primitives closes both. Each new test is verified as a negative control — the empty-key case fails against the hand-rolled predicate, and the three `execute.ts` cases fail against an inverted ternary or a dropped `instanceof` guard, pinning the seam this change exists to produce (both halves were covered before; the join was not). Two test files convert `./prompt-cache.js` from a whole-module mock to an `importOriginal` partial: the old form replaced the module with a single export, so ADDING any export here broke 23 unrelated tests — a trap worth knowing before adding the next one. **Does not fix the underlying race**: porting `materializePaperclipSkillCopy`'s tmp-dir + rename + lock pattern into `materializeRuntimeSkillFiles` is the RCA fix and is deliberately out of scope here. | +| [BLO-31665](https://paperclip.blockcast.net/BLO/issues/BLO-31665) | `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/secret-adopt.test.ts` (new) | Made an `AlreadyExists` 409 on a run-scoped Secret non-fatal. `execute.ts` creates three Secrets before the Job (prompt, env, mcp-config) and treated **any** throw from the create as fatal, returning `k8s_{prompt,env,mcp_config}_secret_create_failed` and killing the run — so a benign leftover from an earlier attempt of the *same* run stranded the agent. Adopting is safe because the name encodes the run: each Secret is `${jobName}-{prompt,env,mcp}` with `jobName = ac---` (`:1151`), so a full-name collision is a collision with this same `(agentId, runId)` and the contents are re-derived from the same config. A new `createOrAdoptRunSecret()` reads the colliding object and `replaceNamespacedSecret`s it. Replace-on-collision cannot yank a Secret from a pod still mounting it: the `k8s_concurrent_run_blocked` guard lists this agent's Jobs and returns **before** `buildJobManifest`, so by the time any of these creates runs there is no live Job for this agent and a colliding Secret is a leftover of a *dead* attempt by construction. **This is a second, parallel implementation of the operation [#1562](https://github.com/Blockcast/paperclip/pull/1562) fixed** in `packages/plugins/sandbox-providers/kubernetes/src/secret-manager.ts`; that PR never touched this call site, which is why the 409 kept hard-failing runs after it shipped (measured again 2026-09-05T17:03:47Z, run `591c1384`, with the fix live on both tiers). One deliberate divergence from that sibling, plus one measured non-difference. (a) The identity gate fails closed on *positive contradiction* only — a Secret whose `paperclip.io/run-id` or `app.kubernetes.io/managed-by` label **disagrees** with this run is never overwritten, but one **missing** them is adopted. A verbatim port of the sibling's gate would have been inert here regardless, since it requires `paperclip.io/managed-by: paperclip-k8s-plugin` while this adapter writes `app.kubernetes.io/managed-by: paperclip` — a different key *and* value, so it would reject every Secret this adapter writes. (b) `isK8s409` mirrors `isK8s404`'s shape, including its `HTTP-Code:` message probe — but that probe is **redundant, not load-bearing**, and an earlier version of this row claimed otherwise. Measured against the installed `@kubernetes/client-node` 1.4.0 by constructing a real `ApiException`: it is built as `super("HTTP-Code: " + code + ...)` and then sets **only** `this.code` — `statusCode` and `response` are both `undefined`. So the sibling's `code`/`statusCode` predicate **does** fire correctly on the production error, and the concern that it would silently miss it is **withdrawn**. Two consequences worth keeping: `code` is the reliable structured signal, and the pre-existing `isK8s404` does **not** check it, so that predicate works today purely on its message regex; and the tests here construct the genuine `ApiException` rather than a hand-rolled stand-in, so they are evidence about the real error shape rather than about an assumption. A create-409 followed by a read-404 (the adapter's own cleanup reaper racing a retry) now retakes the newly-free name instead of resurfacing the stale 409, which is what the sibling does. Each new `execute()` test is verified as a negative control: all three FAIL against the pre-change file with `k8s_prompt_secret_create_failed`, so they discriminate the fix rather than merely passing alongside it. **Ally review follow-up:** the re-create in the read-404 branch was itself an unguarded create, so a second racer retaking the name resurfaced the raw 409 and killed the run with the very code this change exists to prevent. The function is now a bounded two-pass loop — the retry adopts if the name is taken again, and gives up with the original 409 if create-409/read-404 repeats, so a create/delete duel cannot spin. Verified as a negative control: the second-racer case FAILS against the single-shot version. Also from that review: the log verb now distinguishes `Replaced` (leftover overwritten) from `Recreated` (leftover vanished, name retaken) rather than collapsing both to "Reclaimed", since which occurred is what you want when triaging the next one; `readNamespacedSecret`/`replaceNamespacedSecret` got `beforeEach` defaults in `execute.test.ts`, the same BLO-21858 unstubbed-mock trap its existing comment warns about; and a fourth end-to-end case drives the **env** Secret, since the other three drive the prompt path while the reported incident was `k8s_env_secret_create_failed`. **Second review round:** introducing the verb table between the JSDoc and the function silently **detached** the doc comment — JSDoc binds to the next declaration, so the whole safety rationale documented the `SecretDisposition` type alias and the function it describes had none. Reordered so it binds to the function again; pure reordering, no behaviour change, but it had quietly undone the two commits that made that comment accurate. The live-pod paragraph is also **corrected rather than merely softened**: the concurrency guard counts a Job as running only when it has no `deletionTimestamp` and no Complete/Failed condition, so a Job mid-deletion with a still-terminating pod passes it, and the claim "there is no live Job" was too strong. What actually closes the residual window is the consumption model — the env Secret is read via `secretKeyRef` at container start so a later replace cannot reach a running container, and the prompt/mcp Secrets are volume-mounted but re-derived byte-identically for the same `(agentId, runId)`. The end-to-end fixtures now also build a genuine `ApiException` rather than a plain Error described with the retracted "status only in the message" characterization. **Does not address** the orphaned-Secret leak also described on that issue — that is [#1459](https://github.com/Blockcast/paperclip/pull/1459) (BLO-21857), which edits this same file and will need a rebase against whichever of the two lands second. The issue's "stale error is never cleared" defect was **falsified** while working this: a company-wide census of all 15 agents found zero holding a 409 `errorReason`, and all three originally-named agents had heartbeated within ~20 minutes — the field is overwritten by the next run's outcome, not sticky. | +| `this PR` | `README.md`, `src/index.ts`, `src/server/config-schema.ts`, `src/server/config-schema.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `package.json`, `package-lock.json` | Adds optional, validated Caveman/Penstock launcher selection for `claude_k8s` and agent-scoped Ponytail plugin loading. External launcher jobs skip `ccrotate`, preserve explicit provider/mode environment overrides, and keep credential-shaped values in the existing per-run Secret path. The image/runtime assets remain separately pinned and are not fleet-activated by this adapter change; rollout still requires the documented non-production smoke test. | +| `b2e994611` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Stopped `buildEnvGuardSetupShell` truncating a fleet-shared file in place. The agent HOME is a ReadWriteMany CephFS volume mounted by every agent pod, and `> "$GUARD_DIR/paperclip-env-guard.mjs"` is O_TRUNC, so the whole fleet issued `setattr(size=0)` against one inode. A single truncate wedging in the MDS queued every later one behind it permanently and hung the Bash `PreToolUse` hook fleet-wide; neither an MDS failover nor a scrub clears that, and the poisoned dentry cannot even be renamed over. Installs are now create-if-absent via a pod-unique temp plus `rename(2)`, and the guard filename is content-addressed so a change lands as a new inode. `settings.json` gets the same atomic write and prunes stale guard hook entries. | +| [BLO-27155](https://paperclip.blockcast.net/BLO/issues/BLO-27155) | `src/server/execute.ts`, `src/server/job-adopt.test.ts` (new) | Made a `createNamespacedJob` `AlreadyExists` 409 adoptable when it is this run reattaching to its **own** still-live Job, and stopped the failure path deleting that Job's mounted Secrets. This is the fix for the RCA on BLO-27155, whose headline finding was that a worker restart does **not** reap in-flight runs as `process_lost` — a live Job structurally cannot mint that code. What actually happens is that `resumeRunningExternalRuntimeRuns()` (`server/src/services/heartbeat.ts`) **re-executes** the run from its persisted reservation; the Job name is deterministic per `(agentId, runId)`, so the re-execution's create always collides with the run's own live Job, and the 409 was fatal (`k8s_job_create_failed`). The adapter's designed reattach (`classifyOrphan` → `"reattach"`, `reattachOrphanedJobs` config) is dead code with zero callers, which is why every prior reader assumed reattach already worked. **The destructive half is the part worth reviewing.** Before [BLO-31665](https://paperclip.blockcast.net/BLO/issues/BLO-31665) a large-prompt run died at the *prompt-Secret* create and never reached the Job create; making the three Secrets adopt opened the path to the Job create's catch block, which deleted all three Secrets — objects mounted into the live pod — so a restart stopped merely failing the run and began dismantling a still-running Job's inputs, leaving it orphaned in-cluster with the Paperclip run marked failed. Both post-create abort paths (`!createdJobUid || !onExternalRuntimeLaunched`, and a throw from `onExternalRuntimeLaunched`) are now gated on a new `adoptedExistingJob` flag, so an object this execution *adopted* rather than *created* is never torn down by them: leaking a Job is recoverable by the existing reapers, deleting a live one is not. The gate is a new exported pure predicate `jobAdoptionVerdict()`, deliberately **stricter** than `createOrAdoptRunSecret()`'s. That one tolerates missing labels because a Secret written by an older build must stay reclaimable and its name already encodes the run identity; here a server-assigned UID persisted at launch is available, so identity is never inferred from a name and every condition is *required* rather than merely not-contradicted — no persisted identity, UID mismatch, name mismatch (cross-checked against the reservation, because an adapter-type change re-prefixes the Job name while the reservation holds the old one, [BLO-28865](https://paperclip.blockcast.net/BLO/issues/BLO-28865)), or run-id label mismatch all refuse. That preserves the [BLO-17291](https://paperclip.blockcast.net/BLO/issues/BLO-17291) AC-3 exact-identity guarantee: a same-name object that is not this exact object is still never touched. A 409 that cannot be corroborated by a subsequent read also refuses — a 409 says the name is taken, only a read says by what. The label comparison is against `sanitizeLabelValue(runId)` rather than the raw id, because that is what `job-manifest.ts` writes; ordinary UUIDs sanitize to themselves, so a naive raw comparison passes every realistic test and silently refuses to adopt any run whose id is not already label-safe. Keeping the predicate pure makes the whole fail-closed matrix testable without a cluster (20 cases), with the caller doing the I/O. No new RBAC: `get` on `batch/jobs` is already granted in `deploy/helm/paperclip/templates/role.yaml` and `readNamespacedJob` is already used on two other production paths in this file. **One correction to the BLO-31665 row above.** Its safety argument for replacing a colliding Secret states that the `k8s_concurrent_run_blocked` guard returns before `buildJobManifest`, so "there is no live Job for this agent and a colliding Secret is a leftover of a *dead* attempt by construction." That row already softens the claim once, for a Job mid-deletion whose pod is still terminating. The restart-reattach path is a **second and more direct falsifier**: the guard explicitly `continue`s past a running Job that matches this run's persisted name+uid ("Ignoring current lifecycle Job … during concurrency admission"), so admission provably proceeds *while that Job is live* — which is precisely the path this change exists for. The Secret replace is still safe, but for the reason that row gives second rather than first: the env Secret is read via `secretKeyRef` at container start, and the prompt/mcp Secrets are copied to an `emptyDir` by the init container at pod start, so a later replace cannot reach the running container. Recording it because "no live Job by construction" is the kind of premise a future change would reasonably lean on. | +| [PEN-2955](https://paperclip.blockcast.net/PEN/issues/PEN-2955) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/config-schema.ts` | Cut the default agent-run memory **request** 2Gi → 1536Mi (= 1.5 GiB; the 8Gi limit is unchanged). Sized from the measured distribution of per-run peak working set over 7d, n=10510 agent runs: mean 439 Mi, p95 1.06 GiB, p99 2.95 GiB, max 7.64 GiB — so 1536Mi sits just above p95 and ~96% of runs stay within their request. Deliberately not the mean: the request is the only admission control here and the long tail is what the limit absorbs. (PEN-2943's originally-cited "1.79 GiB max across 38 pods" is a point-in-time snapshot and does not survive a 7d per-pod max, which is the statistic a request should be read against.) Written as `1536Mi` rather than `1.5Gi` because Kubernetes canonicalizes BinarySI quantities on read-back — byte-identical, but a live pod renders `1536Mi`, so the literal matches what a post-deploy grep finds (Ally review suggestion on [#1644](https://github.com/Blockcast/paperclip/pull/1644)). The change is only worth anything if the *default* is what reaches the pod, and that was verified rather than assumed: `mergeEnvironmentConfig` is a **top-level** merge, so the `resources` block that `onprem-k8s/paperclip/penstock-agent-environment-reconciler.yaml` writes into the `penstock-general` environment row — nested `{requests:{cpu:500m,memory:1Gi},limits:{cpu:4,memory:16Gi}}` — never satisfies the dotted `config["resources.requests.memory"]` lookup and is inert. Confirmed live with a positive control: the Security Engineer pod runs on that row (SA `penstock-general-agent`, so the row IS merging) yet reported `requests 1/2Gi, limits 4/8Gi` — the code defaults, with the 8Gi-vs-16Gi limit as the discriminator. A new test pins that lookup so the reclamation cannot be silently undone. `config-schema.ts` states the default in the operator-facing hint, which previously named none. The row's dead `resources` block is a real latent bug tracked separately as [PEN-2986](https://paperclip.blockcast.net/PEN/issues/PEN-2986), not fixed here. | +| [BLO-33894](https://paperclip.blockcast.net/BLO/issues/BLO-33894) | `src/server/job-manifest.test.ts` | Gave the bare-line trust in `claudeLineIsHarnessAuthored` a mechanical tripwire. The row above closes that guard against *unknown event types*; it stays open against **bare** lines, which it trusts outright (`if (!match) return true`). [BLO-31955](https://paperclip.blockcast.net/BLO/issues/BLO-31955) established that this is safe **structurally** rather than empirically — the pod log the guard parses has exactly one writer, the `tee` in the `claudeInvocation` pipeline, and that pipeline carries no `2>&1`, so only Claude's stdout reaches the parse surface — and recorded it in a source comment. A comment is the same class of protection that failed on each of the four prior iterations of this defect family ([BLO-7991](https://paperclip.blockcast.net/BLO/issues/BLO-7991) -> [#1525](https://github.com/Blockcast/paperclip/pull/1525) -> BLO-31794 x2 review rounds): it depends on a reviewer reading a *different* file from the one being edited. Adding `2>&1` before the `tee` — a reasonable-looking edit, e.g. to capture CLI diagnostics in the pod log — would begin routing operator- and MCP-authored stderr onto the parse surface as bare, trusted lines, **with no diff on the guard itself**. One assertion in the existing suite now pins it. Deliberately scoped to the substring between the launcher command and the `tee` rather than the whole command: `>/dev/null 2>&1` appears legitimately in the ccrotate preflight and the git plumbing that precede it in the same string, so a whole-command assertion would be red today, and one written loosely enough to be green would no longer discriminate the real case. That scoping is itself pinned by a negative control asserting `2>&1` IS present upstream of the launcher, so the test cannot pass vacuously if the pipeline is restructured. Verified as a tripwire rather than assumed: inserting `2>&1` before the `tee` reddens it with `expected 'cat /tmp/prompt/prompt.txt | claude \…' not to contain '2>&1'`. Tests only — no runtime behaviour change. Ally review follow-up on [#1662](https://github.com/Blockcast/paperclip/pull/1662) (the single remaining Suggestion, rated non-blocking); filed rather than folded in because that PR is reviewed clean at head and editing a vendored file forces a hash recompute, a version bump and a full re-review. | +| [#1730](https://github.com/Blockcast/paperclip/pull/1730) | `package.json`, `package-lock.json` | Added an npm `overrides` floor of `js-yaml` `>=4.3.2 <5`, moving this lockfile's resolution from `4.1.1` to `4.3.2`. GHSA-2883-xcg3-v3hh (CVE-2026-84375) covers `>=4.0.0 <4.3.2`: an empty merge source bypasses the `maxTotalMergeKeys` accounting, so a small document with many empty merges still burns unbounded CPU. This directory is excluded from `pnpm-workspace.yaml` and carries its own npm lockfile, so the root `pnpm.overrides` fix in the same PR could not reach it — the Dockerfile `vendor` stage installs exactly these pins with `npm ci` before building and packing the adapter. Bounded to the 4.x line deliberately: a bare `>=4.3.2` resolves to `5.4.1`, which npm `overrides` would force past `@kubernetes/client-node`'s declared `^4.1.0`. `npm ci`, `tsc --noEmit` and 891/891 adapter tests pass on `4.3.2`; the floor is locked by a second case in `scripts/js-yaml-security-override.test.js`. From an Ally review finding on this PR. | +| [BLO-33279](https://paperclip.blockcast.net/BLO/issues/BLO-33279) | `src/server/inherit-allowlist.ts`, `src/server/inherit-allowlist.test.ts` | Allowlisted `PENSTOCK_READY_TIMEOUT_MS` for inheritance into agent Jobs. The Caveman readiness budget is read by the launcher **inside the agent pod**, so the fleet-wide default is set as a literal on `worker.extraEnv` (`values.blockcast.yaml`, PR #1766, deployed 2026-09-12). `isAgentInheritableEnvName` is default-deny and the name was not listed, so `k8s-client.ts` dropped it and every agent Job kept the launcher's 15000 ms default — a rendered-green manifest that changed nothing, confirmed by reading a live `ac-*` pod spec on 2026-09-14 (5 other `PENSTOCK_*` present, this one absent) while the defect was still firing. The value is a non-secret integer, bounded at 300000 ms by the launcher and scrubbed from both child processes, so admitting it does not widen the credential boundary the allowlist exists to hold. The general guard lives outside this package, in `deploy/helm/paperclip/tests/penstock-worker-secret.test.mjs`: any literal in `worker.extraEnv` that this allowlist does not admit now fails the build, naming the variable. The runbook that produced the bug claimed "there is no name allowlist or denylist" — false since BLO-22514 — and is corrected in the same change. | diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md index 2cce96d7e391..1e7fb724b6df 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md @@ -103,12 +103,15 @@ Regenerate with: ```sh cd vendor/paperclip-adapter-claude-k8s -git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md' \ +git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md|PROVENANCE-CHANGES\.md' \ | LC_ALL=C sort | xargs sha256sum | sha256sum ``` -`LICENSE` and `PROVENANCE.md` are excluded because they are Blockcast additions, -not upstream files — the hash covers only what came from upstream. The listing +`LICENSE`, `PROVENANCE.md` and `PROVENANCE-CHANGES.md` are excluded because they +are Blockcast additions, not upstream files — the hash covers only what came from +upstream. The exclusion list here, in the `vendor_claude_k8s` CI step and on disk +is held in agreement by `scripts/__tests__/provenance-union-merge.test.mjs`, +which fails if a non-upstream file exists that the regex does not name. The listing comes from `git ls-files` rather than `find` so that `node_modules/`, `dist/` and packed tarballs cannot perturb it. @@ -146,36 +149,10 @@ patches. They are ordinary in-tree changes, reviewed under our own CI — which the point of vendoring — but they mean the tree is **no longer byte-for-byte upstream**, so they are enumerated here rather than left implicit. -| commit | files | what | -|---|---|---| -| `cd1630512` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Anchored the `SAFE_ENV_INSPECTION_RE` safe-helper exception to a whole-command invocation. It was evaluated before the full-dump detector and matched the helper anywhere in the command, so a ` && ` compound returned `allow` and executed the dump. Addresses an Ally review finding on Blockcast/paperclip#1092. | -| `cd1630512` | `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts` (new) | Keyed the `getSelfPodInfo()` cache by (kubeconfig path, namespace, hostname). It memoized into one process-global slot while callers pass a per-request kubeconfig, leaking the first execution's image, scheduling, PVC, env and Secret references into later executions against a different cluster. Same review. | -| `8f4f7262a` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Treated `\r`/`\n` as command separators in both classifier copies. Anchoring the helper exception (above) closed the `&&`/`;`/`\|` compounds but not a literal newline: JS `$` without `m` is end-of-input and the argument tail's `\s` spanned newlines, so `paperclip-safe-env\nenv` was a whole-command match, and the dump detector did not treat `\n` as a boundary either. Follow-up on the same Ally review of Blockcast/paperclip#1092. | -| `551c461ef` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Two further dump forms in both classifier copies. (a) Flag-only dumps: `env`/`printenv` stop dumping only when given an *operand*, so requiring a boundary immediately after the utility name let `-0`, `--null` and `-u NAME` through; an option run is now consumed, with `-u`/`--unset` matched together with their argument. (b) Command substitution was never a boundary, so `echo "$(env)"`, `X=$(printenv)` and backtick forms were allowed with no flags at all. Third Ally review pass on Blockcast/paperclip#1092. | -| `551c461ef` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Made the init container's `data` mount conditional on a claim (an unconditional mount named an undeclared volume, which Kubernetes rejects for the whole Pod), and validated + shell-quoted `providers.anthropic.accounts` before interpolating it into the main container's `sh -c`. Same review pass. Both were revised again in `3e0244a78` below. | -| `435219ccf` | `src/server/env-guard.ts` | Comment-only correction. The header claimed behavioural parity with `server/src/agent-shell-guard.ts` "locked by `env-guard.test.ts`". Both halves were false — the test never imports that file and nothing imports it in production; it is dead code, then four fixed bypasses behind. Tracked for removal-or-resync as BLO-22840. | -| `3e0244a78` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Closed the unquoted-command-wrapper bypass class. `SHELL_WRAPPER_RE` unwraps only a *quoted* `-c` payload and whitespace was not a command boundary, so a dump passed as a bare argument to any wrapper (`sh -c env`, `eval env`, `xargs env`, `nohup env`, `timeout 5 env`, `su -c env`, ...) was allowed — 9 of 9 measured payloads, in the real spawned pod script. Split the boundary class: whitespace joins the *leading* class only, while the trailing terminator stays punctuation-only so operand-bearing forms (`env NAME=value cmd`, `printenv HOME`, `grep env file`) stay allowed. Fourth Ally review pass. | -| `3e0244a78` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Three manifest fixes from the same review. (a) Operator-configured mount paths (`workspaceMountPath`, `homeRoot`) reached the init container's `sh -c` unquoted via `browserHome`; now quoted at every site plus a new `assertSafeAbsolutePath` as an independent second defence. (b) A configured account pool with no valid entry fell back to ccrotate's *global* rotation — fail-open, widening credential scope on a config typo; absent and invalid configuration are now distinguished. (c) The `data` volume is now ALWAYS declared (PVC-backed, else `emptyDir`), because the conditional mount from `551c461ef` merely moved the no-PVC failure from admission to an EACCES `mkdir` as runAsUser:1000. | -| `b80b69218` | `src/server/env-guard.ts` | Converged shell unwrapping with `server/src/agent-shell-guard.ts`, adopting its `SHELL_COMMAND_PREFIX_RE` + `readShellCommandArgument` (a human closed the same unquoted-wrapper bypass there in `993bf304c`). Belt-and-braces with the boundary widening in `3e0244a78`: unwrapping is more precise for `sh -c`, the boundary rule is the only thing that reaches non-shell wrappers. Also corrected this file's header claim that the sibling copy was merely "four bypasses behind" — the divergence runs both ways. | -| `85f99a85b` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/execute.test.ts` | Pinned `ANTHROPIC_CUSTOM_HEADERS` into a new `ALWAYS_SECRET_ENV_NAMES` set. `SENSITIVE_ENV_NAME_RE` matches none of its tokens, so the var shipped as a literal `env[].value` despite carrying arbitrary forwarded header lines — including, in principle, an `Authorization:` line set through `adapterConfig.env` or the Penstock session stamp. Routing and both fail-closed guards key off `isSensitiveEnvName()`, so the pin covers all three. BLO-21858, from the BLO-21593 independent review of upstream PR #31 (probe 6). | -| `e1b28276f` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Replaced the boundary-regex classifier with a shell-aware normalizer, in both copies. Five prior rounds each closed one boundary bypass; the fifth Ally review found three more (`env >&2`, `e''nv`, `env -S '-u PATH'`). Re-measured against the real spawned pod script the class was wider than reported: 10 of 12 probe payloads classified `allow` while `/bin/sh` emitted a marker variable, including `e"n"v`, `\env`, `'env'`, `env>&2`, `env 2>&1` and `env -S '-0'`. The cause is structural, not a missing character class — a regex matches command *text*, but the shell executes the command after quote removal, escape processing, redirection stripping and GNU `env -S` re-splitting, so the matched string is not the token that runs. The command is now lexed as a shell would and the resulting words are classified, so spelling variants collapse to one word. The hand-maintained second case list for the embedded copy — the mechanism by which the two copies drifted — is replaced by a differential that drives the whole corpus through both. Fifth Ally review pass on Blockcast/paperclip#1092. | -| `e1b28276f` | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Two fail-closed fixes from the same review. (a) A configured account pool of the wrong *shape* (`accounts: "a@example.test"` rather than a list) was collapsed into the same `null` used for "absent" by `Array.isArray(...) ? ... : null`, so it read as unconfigured and selected unrestricted *global* ccrotate rotation — the same credential-scope widening `3e0244a78` fixed for the all-invalid case, still reachable by the likeliest possible typo. Presence is now tested separately from validity at both `providers.anthropic` and `.accounts` (`parseObject` returns `{}` for any non-object, so both levels shared the defect), an explicitly empty pool counts as configured-but-unusable, and diagnostics report the offending TYPE only — never the value, which sits next to credential material. (b) `workspaceMountPath` could equal a mount this builder already emits (`/tmp/prompt`, `/runtime-cache`, an inherited secret mount); those are shape-valid so `assertSafeAbsolutePath` passed them, and the duplicate mountPath yields a Pod Kubernetes rejects outright. Rejected at construction with a message naming the conflict, plus a per-container invariant assertion that backstops mounts appended later (`/var/run`, `prompt-secret`, `mcp-config-secret`) and the init container's independently-built list. Nested paths stay legal. | -| [#1368](https://github.com/Blockcast/paperclip/pull/1368) | `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Carried the source volume's `items:` key selector through propagation. `getSelfPodInfo()` captured only `secretName`/`mountPath`/`defaultMode`, and the mount site rebuilt the volume without a selector, so a source mount projecting ONE key out of a multi-key Secret was re-expanded into EVERY key of that Secret on the agent Job pod. Measured live: `paperclip-api` projects `gbrain-plugin-service-key` alone out of `authbot-mcp-consumer-service-keys`, while agent pods received all 7 keys — agents held more key material than the container the mount was copied from. `optional: true` stays hardcoded at the mount site by design, so a Secret absent in the agent namespace still cannot hard-fail the Job. Refs [BLO-18927](https://paperclip.blockcast.net/BLO/issues/BLO-18927) AC-3; does **not** close [BLO-22514](https://paperclip.blockcast.net/BLO/issues/BLO-22514), which needs the allowlist. | -| [#1377](https://github.com/Blockcast/paperclip/pull/1377) | `src/server/inherit-allowlist.ts` (new), `src/server/inherit-allowlist.test.ts` (new), `src/server/k8s-client.ts`, `src/server/k8s-client.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/env-guard.ts` | Allowlisted what agent Job pods inherit from the paperclip server pod. `getSelfPodInfo()` snapshotted the server's ENTIRE env — every literal, every `valueFrom` including `secretKeyRef`, every `envFrom` and every mounted secret volume — with no filter, and `job-manifest.ts` replayed all of it onto every agent Job, so each agent container held `PAPERCLIP_AGENT_JWT_SECRET` (mint an API key for ANY agent), `DATABASE_URL` (bypass the API and all of `authorization.ts`) and `GITHUB_APP_PRIVATE_KEY`. Filtered at `getSelfPodInfo()` rather than at the four replay sites, so a future replay site cannot reintroduce the leak by forgetting to filter, plus a fail-closed `findServerOnlyEnvVarsInPodSpec` backstop in `buildJobManifest` because `SelfPodInfo` is a plain object callers can construct unfiltered. Keep-set derived from actual by-name reads plus an agent-pod consumer sweep — not pattern-matched — and both directions unit-tested, since a filter that dropped everything would pass a deny-only suite while breaking every run in the fleet. Measured against the live server env: 54 vars in, 24 inherited, 30 dropped, 0 control-plane credentials remaining. `env-guard.ts` is comment-only: records the BLO-22514 decision to keep that hook fail-OPEN. Closes [BLO-22514](https://paperclip.blockcast.net/BLO/issues/BLO-22514). | -| [#1411](https://github.com/Blockcast/paperclip/pull/1411) | `src/server/inherit-allowlist.ts`, `src/server/inherit-allowlist.test.ts`, `src/server/k8s-client.test.ts` | Removed `paperclip-github-merge-token` (the `@allyblockcast` USER seat, id 296676656) from `AGENT_SECRET_VOLUME_ALLOWLIST`, so it no longer propagates from the server pod into agent Job pods. That seat's approvals SATISFY required review on repos whose ruleset names the Ally team (onprem-k8s, penstock-llm-proxy-core), so propagating it made "can clear branch protection" a fleet-wide capability — measured live at **108 agent Job pods** mounting it — rather than one service's. It was also unusable from an agent by construction, i.e. exposure with no function: the `gh` wrapper resolves `PAPERCLIP_GITHUB_TOKEN_FILE` (pinned to the App token at `/paperclip/.secrets/github-token/token`, never the seat path), `GH_TOKEN`/`gh auth`/`--with-token` overrides are no-ops because that wrapper re-reads the file per invocation, and shipped skills are forbidden from naming the seat path by `CREDENTIAL_SELECTOR_PATTERNS` in `packages/skills-catalog/src/shipped-catalog.test.ts`. Measured across 240 PRs in onprem-k8s, penstock-llm-proxy-core, paperclip and multicast: the seat authored 0 and pushed 0 (authorship is 100% the App) and merged 11, a path the App already covers. The CONTROL PLANE keeps the mount via `deploy/helm/paperclip/values.blockcast.yaml`, where the dedicated reviewer service that legitimately uses this identity runs — only the agent-Job propagation is removed. Two `k8s-client` tests used the seat as their example of a KEPT volume and were re-pointed at the App token; the base fixture now mounts both, deliberately keeping the seat so the allowlist is exercised against a realistic server pod rather than one curated to contain only inheritable volumes. Companion to the org-side half of [BLO-24056](https://paperclip.blockcast.net/BLO/issues/BLO-24056) (seat dropped to `read` on all 11 in-scope repos). | -| [BLO-25403](https://paperclip.blockcast.net/BLO/issues/BLO-25403) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/config-schema.ts`, `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/execute-environment.test.ts` | Ported upstream `94c97d01d408155a5c173c43ab42304f688e7ce3` (merged as [kkroo#32](https://github.com/kkroo/paperclip-adapter-claude-k8s/pull/32) `c5d1389f`) — the BLO-21812 fix, which the 2026-08-06 vendoring **stranded**: it was authored against a branch that was not part of the `3ad3370`+`35f1eb2`+`6ddd4b0` composition, so it never entered the build path and `CLAUDE_K8S_REF` was retired out from under it. A new `resolveServiceAccountName()` resolves per-agent config → `PAPERCLIP_DEFAULT_SERVICE_ACCOUNT_NAME` (fleet default) → **throw**, replacing `asString(config.serviceAccountName, "") \|\| undefined`, which omitted the key and let Kubernetes admission silently assign the namespace's bare `default` SA — an identity with no cluster-scoped read, and a full misdiagnosed incident ([BLO-21499](https://paperclip.blockcast.net/BLO/issues/BLO-21499)). The resolved SA is echoed on `JobBuildResult`, into the run log and into invocation metadata so identity is attributable without a cluster read. Ported by hand rather than cherry-picked: 4 of 6 files applied clean, but `execute.ts` and the `buildJobManifest` return had drifted under `551c461ef`/`3e0244a78`/`e1b28276f` (`envSecret`, `mcpConfigSecret`), so those two hunks were reapplied against current code. No RBAC object is created or modified. Two cases beyond upstream's pin the load-bearing `.trim()` on both resolution branches — `serviceAccountName` is a `type: "text"` field, so a whitespace-only value is reachable from the UI form and a bare `\|\|` would emit it as a Job SA name the API server rejects (Ally review suggestion on [#1409](https://github.com/Blockcast/paperclip/pull/1409)). | -| [BLO-29804](https://paperclip.blockcast.net/BLO/issues/BLO-29804) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts` | Made the env classification declarative and gated it in CI. `SENSITIVE_ENV_NAME_RE` is fail-closed against over-matching but fail-**open** against a credential-carrying variable whose name matches none of its six patterns — the `ANTHROPIC_CUSTOM_HEADERS` row above is the proof, and pinning names one at a time only fixes the instances someone notices. A new exported `ENV_NAME_CLASSIFICATION` table declares every env name this file can emit as `SECRET` or `SAFE_LITERAL` with a stated reason, and `ALWAYS_SECRET_ENV_NAMES` is now *derived* from it, so declaring a name `SECRET` there is what pins it and the table is the single source of truth. The forcing function is a test suite over the full 2×2×2×2 permutation of isolation / DinD / `adapterConfig.env` / `mcpServers`: an emitted name absent from the table reddens the lane **naming the variable**, so a new env var fails CI in the pull request that introduces it instead of inheriting a default. Coverage alone is not enough, so two further assertions exist — one requires classification and `isSensitiveEnvName` to agree on every *emitted* name, closing the hole where a prefix family (`PAPERCLIP_WORKSPACE_`) would silently absolve a future `PAPERCLIP_WORKSPACE_AUTH_TOKEN` as `SAFE_LITERAL` while the routing correctly Secret-backs it; the other asserts the Secret-backed name set for a fixed context is identical to the pre-change set, so the classification cannot silently move a var between literal and `secretKeyRef`. **No runtime behaviour change** — `isSensitiveEnvName()` keeps its semantics (regex ∪ pinned), all three `SECRET` entries already matched the regex or were already pinned, and the diff is classification plus tests. The three operator-supplied channels (`adapterConfig.env`, `selfPod.inheritedEnv`, `selfPod.inheritedEnvValueFrom`) are deliberately out of the table because their names are data rather than code; the second is separately governed by `AGENT_ENV_ALLOWLIST` in `inherit-allowlist.ts`. Records the decision on [BLO-21858](https://paperclip.blockcast.net/BLO/issues/BLO-21858) remedy (2): inverting to "Secret-backed unless declared safe-literal" is **declined** on measured cost/benefit — 8 `secretKeyRef` vars against 37–41 literals per live `ac-*` pod, so inversion moves ~40 operationally load-bearing fields (`HOME`, `TMPDIR`, `PAPERCLIP_RUN_ID`, the isolation roots) into an opaque Secret and stops `GET Pod` being a triage tool, on a code path that templates every agent Job with no staging tier. Full reasoning and the counter-argument on [BLO-29804](https://paperclip.blockcast.net/BLO/issues/BLO-29804). | -| [#1525](https://github.com/Blockcast/paperclip/pull/1525) | `src/server/parse.ts`, `src/server/parse.test.ts`, `src/server/execute.ts` | Made `skill_not_found` reachable and stopped model prose reaching it ([BLO-7991](https://paperclip.blockcast.net/BLO/issues/BLO-7991) AC3). `isClaudeSkillNotFoundStartupFailure` scans the RAW transcript for `Skill "" not found`, so any run whose transcript merely QUOTED that phrase — model prose, a tool result, this very issue's own body — was classified as a startup skill death. That code is in `NON_RETRYABLE_CONTINUATION_ERROR_CODES` and excluded from the zero-token reset, so a misclassification is **permanent** retry suppression, not a visible error. Guarded on the same surface the scan reads (a parsed signal cannot bound what a raw regex sees), then widened once more when a pre-assistant `user` event — which carries `tool_result` text and has no branch in `parseClaudeStreamJson` at all — proved to slip past an `assistant`-only check. Recorded here retroactively: #1525 updated the integrity hash but added no row and did not bump `-blockcast.N`, which the two rules at the foot of this section require. | -| [BLO-31794](https://paperclip.blockcast.net/BLO/issues/BLO-31794) | `src/server/parse.ts`, `src/server/parse.test.ts` | Inverted that guard from a **blocklist** of stream-json event types to an **allowlist** of harness-authored ones, so an unrecognised type fails closed. The row above widened the same predicate twice for one reason: `parseClaudeStreamJson` branches on exactly three types (`system`+`init`, `assistant`, `result`) and ignores every other one, so each newly-appearing event shape slipped a role-blocklist by default. The hazard was one config edit from live rather than hypothetical — `job-manifest.ts` appends `config.extraArgs` to the CLI argv verbatim (read as `asStringArray(config.extraArgs)`, appended by `claudeArgs.push(...extraArgs)`, from an agent's `adapterConfig`), so `--include-partial-messages` on any single agent re-opens the guard with no code change, no diff and no review. Measured on the CLI this adapter runs (v2.1.210): that flag emits 9 `stream_event`s for a two-word prompt, each wrapping model prose in `event.delta.text_delta`, and `stream_event` was enumerated by no previous version of the guard. The allowlist is `{system, rate_limit_event}` with a stated membership criterion (payload must be entirely harness-authored scalars); `result` is deliberately excluded because a *truncated* one can reach the scan carrying the model's final message. Scoped per line and reading only the first `"type"` per line, so a nested type cannot veto its own line — measured as defence-in-depth rather than a live fix, since a real 1717-byte `init` line carries exactly ONE `"type"` (its `mcp_servers` entries are `{name, status}`, `output_style` a bare string). Detection is unchanged: the four existing guard cases pass unmodified, plus new cases for a production-shaped `init` line, `system:status` (which v2.1.210 emits pre-turn under `--include-partial-messages`), and `rate_limit_event` (the FAR-32 repro in `execute.test.ts`). Verified as a negative control — the new `stream_event` case FAILS against the previous blocklist while all 12 detection-preserving cases pass, so it discriminates the fix rather than merely passing alongside it. **Ally review follow-up on [#1650](https://github.com/Blockcast/paperclip/pull/1650):** the allowlist reaches the `system` *subtype* rather than admitting the type wholesale, because `system` is a multiplexer and admitting it whole reproduced this same defect one level down. Measured against the v2.1.210 binary, `system` carries at least `init`, `status`, `compact_boundary`, `hook_started`, `hook_response` and `mcp_status`; only the first two are admitted. `hook_response` is why this is a live hole rather than future-proofing — the binary builds it as `{type:"system",subtype:"hook_response",…,output,stdout,stderr}`, embedding a hook process's raw stdout, which is operator-configured. It is not merely reachable via `--settings`/`extraArgs`: **Paperclip provisions hooks itself**, and real pod logs on this instance carry a `SessionStart` `hook_response` whose `output` is an operator status message, and another whose `stdout` is an nginx 503 HTML page. (`hook_error`, listed in the review, appears in no v2.1.210 string table and is not a subtype at this version.) A `system` line with no readable subtype fails closed. **Second review round — the subtype gate as first shipped silently disabled detection in production, and this row previously claimed otherwise.** Because Paperclip provisions a `SessionStart` hook, `hook_started`/`hook_response` open the transcript *before* `init` on the large majority of real runs — measured on this instance's pod logs at **6510 of 8036** `init`-carrying logs (81%), with the hook line preceding `init` in **399/399** of a sample carrying both. A whole-transcript "every line must be harness-authored" veto therefore returned `false` on all of them, and the suite stayed green only because its sole positive fixture was a synthetic two-line shape no production run has — precisely the "fix the false positive by disabling detection entirely" failure mode this issue's own acceptance criteria warn about. The predicate now **attributes the phrase to its line** instead of demanding a globally clean transcript: the trigger phrase counts only when it sits on a line the harness authored (an allowlisted event, or a bare non-event line, which in stream-json mode is the CLI speaking outside the protocol; 0 of 6893 sampled production lines are bare, and the third review round below records why that is structural rather than incidental). Every false positive in this family is the phrase *inside* an event payload, so attribution is the more faithful invariant and unknown types still fail closed. Detection now genuinely survives the production preamble, pinned by four new cases (full preamble; each hook line alone; an untrusted event *after* the death), and the negative-control discipline is unchanged — the production-preamble case FAILS against the whole-transcript veto. One deliberate narrowing is recorded in the source: the phrase regex's `\s+` can span a newline, so a phrase straddling two lines would no longer match; the CLI emits it on one line, and the direction is the safe one. Also note this row's own earlier "measured as defence-in-depth rather than a live fix" framing applied to *nested types on an init line*, which remains accurate; it did not license the detection-loss claim. The phrase test still runs before the per-line walk as a pre-filter — the walk re-tests each line and is what decides — skipping an eager `split` of the pod log's stdout stream on the common phrase-absent failure. **Third review round — [BLO-31955](https://paperclip.blockcast.net/BLO/issues/BLO-31955), comment-only:** the bare-line trust (`if (!match) return true`) is now recorded in the source as a **structural** invariant rather than an empirical one, because the sample was the weaker of the two available justifications. The surface the predicate reads has exactly one writer — the `tee` in the pipeline `job-manifest.ts` builds in `claudeInvocation`, `cat … | claude … | tee | > /dev/null` (stage 2 is written as `claude` here for the round in which it was recorded; the fourth round below corrects it to `launcherCommand`), which carries no `2>&1` on any stage — so it receives stage 2's stdout and nothing else; hook stderr, MCP-server stderr and the fail-fast `[wrapper]` line (`failFastFilter`, written to `/dev/stderr` and downstream of the `tee` regardless) bypass it by construction, as does the prompt. Verified end to end rather than taken from the review: `podLogPath` has that one writer, `stdout` is assigned only from it (in `execute.ts`, the `tailResult.value` tail via `fs.open` and the `stdout = onDisk` re-read, so both parse paths share one single-writer file), and the one reader of the *merged* container-log stream (`readPodContainerLogTail`, in `execute.ts`, via `readNamespacedPodLog`) is confined to diagnostics and never reaches the parse surface. The source now names the three things that void the invariant — adding `2>&1` before the `tee`, routing a merged container-log read into the parse surface, and pointing `adapterConfig.agentCommand` at a launcher that writes any line of its own to stdout — which is the point of the change: every prior iteration in this family (BLO-7991 → #1525 → BLO-31794) was an invisible widening from elsewhere, and naming them converts a fifth one into a reviewable diff. The 6893-line sample is retained as corroboration, not as the basis. One predicate change rode along, from the review of this round: `status` was removed from the `system` subtype allowlist. Its admission had been argued under the round-two whole-transcript veto ("compaction cannot occur before the first turn, so any such transcript also carries an `assistant` line, which the guard rejects"), and per-line attribution made that rejection stop happening — a `status` line whose `compact_result`/`compact_error` (compaction summaries derived from model output) quoted the phrase would have classified `skill_not_found`, a permanent retry suppression. Nothing was lost: in `init -> status -> death` the death is the bare line, trusted on its own, and the existing case for that shape still passes; a new negative case pins the `compact_result` false positive, and it fails against the pre-change allowlist. **Fourth review round, same PR:** the enumeration shipped at two and was incomplete — stage 2 of that pipeline is `launcherCommand`, not `claude`, resolved from `validateAgentCommand(config.agentCommand, "claude")` against an operator-editable `adapterConfig` text field ("Agent Launcher", `config-schema.ts`). It touches the *other* premise rather than the single-writer one: `tee` carries no `2>&1` whichever binary feeds it, but "a bare line is the CLI speaking outside the protocol" is a claim about the behaviour of the binary writing stdout, and that binary is configurable. It needs no code edit, no diff and no review, which is what makes it live regardless of how often it is actually used — so no deployment claim is load-bearing here, and the sixth round below withdraws the one that was. Corroborated, not established, by a dated instance reading: **14 of 15** `claude_k8s` agents carried a non-default `adapterConfig.agentCommand` (all `/opt/penstock/bin/penstock-agent-runtime.mjs`) on 2026-09-16; re-measure by counting agents whose `adapterConfig.agentCommand` differs from `"claude"`, after confirming the field is visible at all. Ally's fifth-round review measured `adapterConfig: {}` for all 15 through `paperclipListAgents` and correctly declined to assert the count false on that basis; the sixth round re-ran the same tool from a caller with config visibility and reproduced 14 of 15 exactly, so the discriminator is the caller rather than the tool — a `{}` read is no config visibility, not a rotted count, and the recipe now says so. No live false positive is claimed; the launcher is expected to be a stream-json passthrough and `job-manifest.ts` says so where it sets `PENSTOCK_AGENT_COMMAND` ("the launcher owns provider credentials and starts the native Claude protocol itself"). Recording it converts that unstated assumption into a reviewable premise, which is this row's whole purpose — a proxy surfacing an upstream error body on stdout is the same shape as the nginx 503 page already documented above for `hook_response`. **Sixth review round, same PR — a citation that read plausibly and did not support its claim, which is the exact failure class this row exists to close.** The fourth round argued vector 3 was "the *normal* configuration ... which this repository shows on its own without reference to any instance", citing `Dockerfile.agent` and `docs/runbooks/penstock-claude-local-rollout.md`. Both files exist; neither shows it, and the runbook states the opposite — it is a staged-rollout procedure whose Scope reads "It does **not** activate either feature fleet-wide ... The first deployment is one non-production Job", whose Hard boundaries read "Configure **one named non-production agent** ... Do not bulk-patch a company, adapter type, or fleet default", and which explicitly forbids the inference that was drawn from the image: "**Do not treat a green image build as activation.**" `Dockerfile.agent:16` copies the launcher binary in, establishing AVAILABILITY; selection is per-agent `adapterConfig`, which is not in this tree at all, and `validateAgentCommand(config.agentCommand, "claude")` defaults to exactly `claude`. The claim is withdrawn from both the source block and this row rather than re-argued, because vector 3 never needed it: an operator-editable field requiring no code edit, no diff and no review is live whatever today's usage is, which the preceding clause already said. Two things fall out. The dated instance count is now the only empirical statement in the block and is labelled as corroboration, which is what it always was — the disputed claim was the *repository* inference, not the number. And with both citations withdrawn, every anchor in the block is once more inside `vendor/paperclip-adapter-claude-k8s/`, i.e. inside the 41-file integrity hash, so the `vendor_claude_k8s` lane mechanically catches drift in all of them; the fourth round had put two anchors one directory outside enforcement range, where a rename at rollout completion would have reddened nothing. | -| [#1669](https://github.com/Blockcast/paperclip/pull/1669) | `src/server/prompt-cache.ts`, `src/server/prompt-cache.test.ts`, `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/execute-environment.test.ts` | Classified the OTHER path into BLO-7991's pathology, which the row above cannot see. [BLO-32055](https://paperclip.blockcast.net/BLO/issues/BLO-32055): a live run died with a bare Node `ENOENT ... open '<...>/__runtime__//SKILL.md'` and was reported as the anonymous `adapter_failed` — an agent-pool/adapter fault for what is a skill configuration fault, invisible to skill-health sweeps. The reader was not a skill loader but `hashPathContents`, which walks each declared skill's tree to derive the prompt-bundle **cache key**; its `readFile` was unguarded. That walk runs inside `prepareClaudePromptBundle`, i.e. BEFORE the CLI is spawned, so `stdoutExcerpt` and `stderrExcerpt` were both null and there was no `parsed`, no result event and no transcript — every classifier in `parse.ts` reads a Claude-CLI-authored surface, so all of them are **structurally** blind to it. Not a too-narrow regex and not a regression of #1525: a second real path into the same user-visible failure, on a layer #1525 never inspects. The file's absence is transient — `company-skills.ts materializeRuntimeSkillFiles` refreshes by `fs.rm(recursive)` -> `mkdir` -> per-file `writeFile`, so the sweep publishes a window where the directory exists and `SKILL.md` does not; measured live, the file appeared **43m36s** after the run died on it. Routing this to `skill_not_found` would therefore have been the WRONG fix — that code is in `NON_RETRYABLE_CONTINUATION_ERROR_CODES`, so it converts a self-healing condition into permanent retry suppression, the same over-suppression hazard as the two rows above. Split instead on the source of truth (which skill owns the path, never message text, so the BLO-31794 false-positive class cannot reach this branch): a catalog-backed key yields a new transient `skill_materialization_pending` and a non-catalog-backed one keeps the permanent `skill_not_found`. The new code joins `TRANSIENT_INFRA_CONTINUATION_ERROR_CODES` — **the set that already contained `adapter_failed`** — so retryability is preserved exactly rather than widened. The discriminator is load-bearing rather than cosmetic because `readPaperclipRuntimeSkillEntries` silently switches source: it returns the server-injected catalog entries OR, when config carries none, the adapter's own bundled on-disk skills, a read-only image path where a missing file is a packaging fault no retry can fix. Hashing is still fatal and the error re-thrown rather than swallowed — a half-written tree hashed into a key would mint a bundle whose skills are silently incomplete, which is BLO-7991's original harm traded for a failure nobody sees. **Ally review follow-up:** `readCatalogBackedSkillKeys` is now a literal transcription of the key-deriving half of `normalizeConfiguredPaperclipRuntimeSkills` (`server-utils.ts:2598`) rather than an approximation, which was wrong in both directions — `asString` falls back on an EMPTY string and not merely on a non-string, so `{key:"", name:"x"}` normalizes upstream to key `x` while a `typeof key === "string"` test resolved it to `""` and dropped the entry, marking a catalog-backed skill un-backed (permanent suppression, the one direction this change exists to avoid); and upstream DISCARDS any entry missing `runtimeName` or `source`, which the hand-rolled version contributed anyway, letting a source-less entry colliding with a bundled key mark an image-path fault retryable. Deriving from the same primitives closes both. Each new test is verified as a negative control — the empty-key case fails against the hand-rolled predicate, and the three `execute.ts` cases fail against an inverted ternary or a dropped `instanceof` guard, pinning the seam this change exists to produce (both halves were covered before; the join was not). Two test files convert `./prompt-cache.js` from a whole-module mock to an `importOriginal` partial: the old form replaced the module with a single export, so ADDING any export here broke 23 unrelated tests — a trap worth knowing before adding the next one. **Does not fix the underlying race**: porting `materializePaperclipSkillCopy`'s tmp-dir + rename + lock pattern into `materializeRuntimeSkillFiles` is the RCA fix and is deliberately out of scope here. | -| [BLO-31665](https://paperclip.blockcast.net/BLO/issues/BLO-31665) | `src/server/execute.ts`, `src/server/execute.test.ts`, `src/server/secret-adopt.test.ts` (new) | Made an `AlreadyExists` 409 on a run-scoped Secret non-fatal. `execute.ts` creates three Secrets before the Job (prompt, env, mcp-config) and treated **any** throw from the create as fatal, returning `k8s_{prompt,env,mcp_config}_secret_create_failed` and killing the run — so a benign leftover from an earlier attempt of the *same* run stranded the agent. Adopting is safe because the name encodes the run: each Secret is `${jobName}-{prompt,env,mcp}` with `jobName = ac---` (`:1151`), so a full-name collision is a collision with this same `(agentId, runId)` and the contents are re-derived from the same config. A new `createOrAdoptRunSecret()` reads the colliding object and `replaceNamespacedSecret`s it. Replace-on-collision cannot yank a Secret from a pod still mounting it: the `k8s_concurrent_run_blocked` guard lists this agent's Jobs and returns **before** `buildJobManifest`, so by the time any of these creates runs there is no live Job for this agent and a colliding Secret is a leftover of a *dead* attempt by construction. **This is a second, parallel implementation of the operation [#1562](https://github.com/Blockcast/paperclip/pull/1562) fixed** in `packages/plugins/sandbox-providers/kubernetes/src/secret-manager.ts`; that PR never touched this call site, which is why the 409 kept hard-failing runs after it shipped (measured again 2026-09-05T17:03:47Z, run `591c1384`, with the fix live on both tiers). One deliberate divergence from that sibling, plus one measured non-difference. (a) The identity gate fails closed on *positive contradiction* only — a Secret whose `paperclip.io/run-id` or `app.kubernetes.io/managed-by` label **disagrees** with this run is never overwritten, but one **missing** them is adopted. A verbatim port of the sibling's gate would have been inert here regardless, since it requires `paperclip.io/managed-by: paperclip-k8s-plugin` while this adapter writes `app.kubernetes.io/managed-by: paperclip` — a different key *and* value, so it would reject every Secret this adapter writes. (b) `isK8s409` mirrors `isK8s404`'s shape, including its `HTTP-Code:` message probe — but that probe is **redundant, not load-bearing**, and an earlier version of this row claimed otherwise. Measured against the installed `@kubernetes/client-node` 1.4.0 by constructing a real `ApiException`: it is built as `super("HTTP-Code: " + code + ...)` and then sets **only** `this.code` — `statusCode` and `response` are both `undefined`. So the sibling's `code`/`statusCode` predicate **does** fire correctly on the production error, and the concern that it would silently miss it is **withdrawn**. Two consequences worth keeping: `code` is the reliable structured signal, and the pre-existing `isK8s404` does **not** check it, so that predicate works today purely on its message regex; and the tests here construct the genuine `ApiException` rather than a hand-rolled stand-in, so they are evidence about the real error shape rather than about an assumption. A create-409 followed by a read-404 (the adapter's own cleanup reaper racing a retry) now retakes the newly-free name instead of resurfacing the stale 409, which is what the sibling does. Each new `execute()` test is verified as a negative control: all three FAIL against the pre-change file with `k8s_prompt_secret_create_failed`, so they discriminate the fix rather than merely passing alongside it. **Ally review follow-up:** the re-create in the read-404 branch was itself an unguarded create, so a second racer retaking the name resurfaced the raw 409 and killed the run with the very code this change exists to prevent. The function is now a bounded two-pass loop — the retry adopts if the name is taken again, and gives up with the original 409 if create-409/read-404 repeats, so a create/delete duel cannot spin. Verified as a negative control: the second-racer case FAILS against the single-shot version. Also from that review: the log verb now distinguishes `Replaced` (leftover overwritten) from `Recreated` (leftover vanished, name retaken) rather than collapsing both to "Reclaimed", since which occurred is what you want when triaging the next one; `readNamespacedSecret`/`replaceNamespacedSecret` got `beforeEach` defaults in `execute.test.ts`, the same BLO-21858 unstubbed-mock trap its existing comment warns about; and a fourth end-to-end case drives the **env** Secret, since the other three drive the prompt path while the reported incident was `k8s_env_secret_create_failed`. **Second review round:** introducing the verb table between the JSDoc and the function silently **detached** the doc comment — JSDoc binds to the next declaration, so the whole safety rationale documented the `SecretDisposition` type alias and the function it describes had none. Reordered so it binds to the function again; pure reordering, no behaviour change, but it had quietly undone the two commits that made that comment accurate. The live-pod paragraph is also **corrected rather than merely softened**: the concurrency guard counts a Job as running only when it has no `deletionTimestamp` and no Complete/Failed condition, so a Job mid-deletion with a still-terminating pod passes it, and the claim "there is no live Job" was too strong. What actually closes the residual window is the consumption model — the env Secret is read via `secretKeyRef` at container start so a later replace cannot reach a running container, and the prompt/mcp Secrets are volume-mounted but re-derived byte-identically for the same `(agentId, runId)`. The end-to-end fixtures now also build a genuine `ApiException` rather than a plain Error described with the retracted "status only in the message" characterization. **Does not address** the orphaned-Secret leak also described on that issue — that is [#1459](https://github.com/Blockcast/paperclip/pull/1459) (BLO-21857), which edits this same file and will need a rebase against whichever of the two lands second. The issue's "stale error is never cleared" defect was **falsified** while working this: a company-wide census of all 15 agents found zero holding a 409 `errorReason`, and all three originally-named agents had heartbeated within ~20 minutes — the field is overwritten by the next run's outcome, not sticky. | -| `this PR` | `README.md`, `src/index.ts`, `src/server/config-schema.ts`, `src/server/config-schema.test.ts`, `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `package.json`, `package-lock.json` | Adds optional, validated Caveman/Penstock launcher selection for `claude_k8s` and agent-scoped Ponytail plugin loading. External launcher jobs skip `ccrotate`, preserve explicit provider/mode environment overrides, and keep credential-shaped values in the existing per-run Secret path. The image/runtime assets remain separately pinned and are not fleet-activated by this adapter change; rollout still requires the documented non-production smoke test. | -| `b2e994611` | `src/server/env-guard.ts`, `src/server/env-guard.test.ts` | Stopped `buildEnvGuardSetupShell` truncating a fleet-shared file in place. The agent HOME is a ReadWriteMany CephFS volume mounted by every agent pod, and `> "$GUARD_DIR/paperclip-env-guard.mjs"` is O_TRUNC, so the whole fleet issued `setattr(size=0)` against one inode. A single truncate wedging in the MDS queued every later one behind it permanently and hung the Bash `PreToolUse` hook fleet-wide; neither an MDS failover nor a scrub clears that, and the poisoned dentry cannot even be renamed over. Installs are now create-if-absent via a pod-unique temp plus `rename(2)`, and the guard filename is content-addressed so a change lands as a new inode. `settings.json` gets the same atomic write and prunes stale guard hook entries. | -| [BLO-27155](https://paperclip.blockcast.net/BLO/issues/BLO-27155) | `src/server/execute.ts`, `src/server/job-adopt.test.ts` (new) | Made a `createNamespacedJob` `AlreadyExists` 409 adoptable when it is this run reattaching to its **own** still-live Job, and stopped the failure path deleting that Job's mounted Secrets. This is the fix for the RCA on BLO-27155, whose headline finding was that a worker restart does **not** reap in-flight runs as `process_lost` — a live Job structurally cannot mint that code. What actually happens is that `resumeRunningExternalRuntimeRuns()` (`server/src/services/heartbeat.ts`) **re-executes** the run from its persisted reservation; the Job name is deterministic per `(agentId, runId)`, so the re-execution's create always collides with the run's own live Job, and the 409 was fatal (`k8s_job_create_failed`). The adapter's designed reattach (`classifyOrphan` → `"reattach"`, `reattachOrphanedJobs` config) is dead code with zero callers, which is why every prior reader assumed reattach already worked. **The destructive half is the part worth reviewing.** Before [BLO-31665](https://paperclip.blockcast.net/BLO/issues/BLO-31665) a large-prompt run died at the *prompt-Secret* create and never reached the Job create; making the three Secrets adopt opened the path to the Job create's catch block, which deleted all three Secrets — objects mounted into the live pod — so a restart stopped merely failing the run and began dismantling a still-running Job's inputs, leaving it orphaned in-cluster with the Paperclip run marked failed. Both post-create abort paths (`!createdJobUid || !onExternalRuntimeLaunched`, and a throw from `onExternalRuntimeLaunched`) are now gated on a new `adoptedExistingJob` flag, so an object this execution *adopted* rather than *created* is never torn down by them: leaking a Job is recoverable by the existing reapers, deleting a live one is not. The gate is a new exported pure predicate `jobAdoptionVerdict()`, deliberately **stricter** than `createOrAdoptRunSecret()`'s. That one tolerates missing labels because a Secret written by an older build must stay reclaimable and its name already encodes the run identity; here a server-assigned UID persisted at launch is available, so identity is never inferred from a name and every condition is *required* rather than merely not-contradicted — no persisted identity, UID mismatch, name mismatch (cross-checked against the reservation, because an adapter-type change re-prefixes the Job name while the reservation holds the old one, [BLO-28865](https://paperclip.blockcast.net/BLO/issues/BLO-28865)), or run-id label mismatch all refuse. That preserves the [BLO-17291](https://paperclip.blockcast.net/BLO/issues/BLO-17291) AC-3 exact-identity guarantee: a same-name object that is not this exact object is still never touched. A 409 that cannot be corroborated by a subsequent read also refuses — a 409 says the name is taken, only a read says by what. The label comparison is against `sanitizeLabelValue(runId)` rather than the raw id, because that is what `job-manifest.ts` writes; ordinary UUIDs sanitize to themselves, so a naive raw comparison passes every realistic test and silently refuses to adopt any run whose id is not already label-safe. Keeping the predicate pure makes the whole fail-closed matrix testable without a cluster (20 cases), with the caller doing the I/O. No new RBAC: `get` on `batch/jobs` is already granted in `deploy/helm/paperclip/templates/role.yaml` and `readNamespacedJob` is already used on two other production paths in this file. **One correction to the BLO-31665 row above.** Its safety argument for replacing a colliding Secret states that the `k8s_concurrent_run_blocked` guard returns before `buildJobManifest`, so "there is no live Job for this agent and a colliding Secret is a leftover of a *dead* attempt by construction." That row already softens the claim once, for a Job mid-deletion whose pod is still terminating. The restart-reattach path is a **second and more direct falsifier**: the guard explicitly `continue`s past a running Job that matches this run's persisted name+uid ("Ignoring current lifecycle Job … during concurrency admission"), so admission provably proceeds *while that Job is live* — which is precisely the path this change exists for. The Secret replace is still safe, but for the reason that row gives second rather than first: the env Secret is read via `secretKeyRef` at container start, and the prompt/mcp Secrets are copied to an `emptyDir` by the init container at pod start, so a later replace cannot reach the running container. Recording it because "no live Job by construction" is the kind of premise a future change would reasonably lean on. | -| [PEN-2955](https://paperclip.blockcast.net/PEN/issues/PEN-2955) | `src/server/job-manifest.ts`, `src/server/job-manifest.test.ts`, `src/server/config-schema.ts` | Cut the default agent-run memory **request** 2Gi → 1536Mi (= 1.5 GiB; the 8Gi limit is unchanged). Sized from the measured distribution of per-run peak working set over 7d, n=10510 agent runs: mean 439 Mi, p95 1.06 GiB, p99 2.95 GiB, max 7.64 GiB — so 1536Mi sits just above p95 and ~96% of runs stay within their request. Deliberately not the mean: the request is the only admission control here and the long tail is what the limit absorbs. (PEN-2943's originally-cited "1.79 GiB max across 38 pods" is a point-in-time snapshot and does not survive a 7d per-pod max, which is the statistic a request should be read against.) Written as `1536Mi` rather than `1.5Gi` because Kubernetes canonicalizes BinarySI quantities on read-back — byte-identical, but a live pod renders `1536Mi`, so the literal matches what a post-deploy grep finds (Ally review suggestion on [#1644](https://github.com/Blockcast/paperclip/pull/1644)). The change is only worth anything if the *default* is what reaches the pod, and that was verified rather than assumed: `mergeEnvironmentConfig` is a **top-level** merge, so the `resources` block that `onprem-k8s/paperclip/penstock-agent-environment-reconciler.yaml` writes into the `penstock-general` environment row — nested `{requests:{cpu:500m,memory:1Gi},limits:{cpu:4,memory:16Gi}}` — never satisfies the dotted `config["resources.requests.memory"]` lookup and is inert. Confirmed live with a positive control: the Security Engineer pod runs on that row (SA `penstock-general-agent`, so the row IS merging) yet reported `requests 1/2Gi, limits 4/8Gi` — the code defaults, with the 8Gi-vs-16Gi limit as the discriminator. A new test pins that lookup so the reclamation cannot be silently undone. `config-schema.ts` states the default in the operator-facing hint, which previously named none. The row's dead `resources` block is a real latent bug tracked separately as [PEN-2986](https://paperclip.blockcast.net/PEN/issues/PEN-2986), not fixed here. | -| [BLO-33894](https://paperclip.blockcast.net/BLO/issues/BLO-33894) | `src/server/job-manifest.test.ts` | Gave the bare-line trust in `claudeLineIsHarnessAuthored` a mechanical tripwire. The row above closes that guard against *unknown event types*; it stays open against **bare** lines, which it trusts outright (`if (!match) return true`). [BLO-31955](https://paperclip.blockcast.net/BLO/issues/BLO-31955) established that this is safe **structurally** rather than empirically — the pod log the guard parses has exactly one writer, the `tee` in the `claudeInvocation` pipeline, and that pipeline carries no `2>&1`, so only Claude's stdout reaches the parse surface — and recorded it in a source comment. A comment is the same class of protection that failed on each of the four prior iterations of this defect family ([BLO-7991](https://paperclip.blockcast.net/BLO/issues/BLO-7991) -> [#1525](https://github.com/Blockcast/paperclip/pull/1525) -> BLO-31794 x2 review rounds): it depends on a reviewer reading a *different* file from the one being edited. Adding `2>&1` before the `tee` — a reasonable-looking edit, e.g. to capture CLI diagnostics in the pod log — would begin routing operator- and MCP-authored stderr onto the parse surface as bare, trusted lines, **with no diff on the guard itself**. One assertion in the existing suite now pins it. Deliberately scoped to the substring between the launcher command and the `tee` rather than the whole command: `>/dev/null 2>&1` appears legitimately in the ccrotate preflight and the git plumbing that precede it in the same string, so a whole-command assertion would be red today, and one written loosely enough to be green would no longer discriminate the real case. That scoping is itself pinned by a negative control asserting `2>&1` IS present upstream of the launcher, so the test cannot pass vacuously if the pipeline is restructured. Verified as a tripwire rather than assumed: inserting `2>&1` before the `tee` reddens it with `expected 'cat /tmp/prompt/prompt.txt | claude \…' not to contain '2>&1'`. Tests only — no runtime behaviour change. Ally review follow-up on [#1662](https://github.com/Blockcast/paperclip/pull/1662) (the single remaining Suggestion, rated non-blocking); filed rather than folded in because that PR is reviewed clean at head and editing a vendored file forces a hash recompute, a version bump and a full re-review. | -| [#1730](https://github.com/Blockcast/paperclip/pull/1730) | `package.json`, `package-lock.json` | Added an npm `overrides` floor of `js-yaml` `>=4.3.2 <5`, moving this lockfile's resolution from `4.1.1` to `4.3.2`. GHSA-2883-xcg3-v3hh (CVE-2026-84375) covers `>=4.0.0 <4.3.2`: an empty merge source bypasses the `maxTotalMergeKeys` accounting, so a small document with many empty merges still burns unbounded CPU. This directory is excluded from `pnpm-workspace.yaml` and carries its own npm lockfile, so the root `pnpm.overrides` fix in the same PR could not reach it — the Dockerfile `vendor` stage installs exactly these pins with `npm ci` before building and packing the adapter. Bounded to the 4.x line deliberately: a bare `>=4.3.2` resolves to `5.4.1`, which npm `overrides` would force past `@kubernetes/client-node`'s declared `^4.1.0`. `npm ci`, `tsc --noEmit` and 891/891 adapter tests pass on `4.3.2`; the floor is locked by a second case in `scripts/js-yaml-security-override.test.js`. From an Ally review finding on this PR. | -| [BLO-33279](https://paperclip.blockcast.net/BLO/issues/BLO-33279) | `src/server/inherit-allowlist.ts`, `src/server/inherit-allowlist.test.ts` | Allowlisted `PENSTOCK_READY_TIMEOUT_MS` for inheritance into agent Jobs. The Caveman readiness budget is read by the launcher **inside the agent pod**, so the fleet-wide default is set as a literal on `worker.extraEnv` (`values.blockcast.yaml`, PR #1766, deployed 2026-09-12). `isAgentInheritableEnvName` is default-deny and the name was not listed, so `k8s-client.ts` dropped it and every agent Job kept the launcher's 15000 ms default — a rendered-green manifest that changed nothing, confirmed by reading a live `ac-*` pod spec on 2026-09-14 (5 other `PENSTOCK_*` present, this one absent) while the defect was still firing. The value is a non-secret integer, bounded at 300000 ms by the launcher and scrubbed from both child processes, so admitting it does not widen the credential boundary the allowlist exists to hold. The general guard lives outside this package, in `deploy/helm/paperclip/tests/penstock-worker-secret.test.mjs`: any literal in `worker.extraEnv` that this allowlist does not admit now fails the build, naming the variable. The runbook that produced the bug claimed "there is no name allowlist or denylist" — false since BLO-22514 — and is corrected in the same change. | +The per-patch log lives in [PROVENANCE-CHANGES.md](./PROVENANCE-CHANGES.md), +a separate file so that concurrent PRs appending to it do not conflict +(BLO-34872). It is excluded from the integrity hash below. + The two cherry-picked commits in the composition above remain upstream commits authored against the fork, not Blockcast-local patches. From 269204d82d60d31cf26170533ab0b6968abb5627 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Mon, 21 Sep 2026 12:19:11 +0000 Subject: [PATCH 2/2] fix(vendor): correct a false safety claim in PROVENANCE.md, pin the CI-regex count Ally's review at b4084ddb raised one Important finding and it is correct. PROVENANCE.md claimed the union-merge test "fails if a non-upstream file exists that the regex does not name". It does not, and nothing in the suite does. `provenance-union-merge.test.mjs:111` asserts the converse -- every name in the exclusion regex must be a tracked file (regex is a subset of tracked). Nothing asserts tracked-non-upstream is a subset of regex, and nothing could without a marker separating upstream files from Blockcast additions. Reproduced the reviewer's control at this head: added an un-excluded vendor/.../NOTES.md, git added it, re-ran the suite -- 5/5 still pass. The hash guard does fire (1d55b35f... != 7a91abbd...), but that is a different guard with a different remedy: "regenerate the hash" silently widens the hash to cover a Blockcast-local file, after which "hash matches" no longer means "upstream is unmodified" -- the exact property this document exists to guarantee. That mattered more than a normal doc nit because it was a false statement about a safety guarantee, in the integrity document. Reworded to what the test really checks, and the unchecked direction is now named explicitly along with what it would take to check it. Also took the review's suggestion: ciExclusionAlternatives() took the *first* `grep -vxE '...'` in pr.yml. That is unambiguous today (exactly one occurrence), but a second vendored tree with its own provenance job would bind every assertion to whichever appeared first and leave the suite green while guarding the wrong job. Now asserts exactly one. Mutation-tested per the standing rule, one mutation at a time: guard present + a second regex injected -> 3 fail guard reverted + same mutation -> 5 pass restored -> 5 pass So the new assertion is what catches it, not something else incidentally. Integrity hash unchanged (7a91abbd...); replayed the vendor_claude_k8s guard verbatim -- matches, and exactly one 64-hex line remains in PROVENANCE.md. No upstream file changed, so no PROVENANCE-CHANGES.md row. Co-Authored-By: Paperclip --- .../__tests__/provenance-union-merge.test.mjs | 14 +++++++++++--- .../paperclip-adapter-claude-k8s/PROVENANCE.md | 17 ++++++++++++++--- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/scripts/__tests__/provenance-union-merge.test.mjs b/scripts/__tests__/provenance-union-merge.test.mjs index e97437108d3b..97183d7356e1 100644 --- a/scripts/__tests__/provenance-union-merge.test.mjs +++ b/scripts/__tests__/provenance-union-merge.test.mjs @@ -45,9 +45,17 @@ function unionMergedPaths() { * rather than a surprise in CI. */ function ciExclusionAlternatives(source, label) { - const m = source.match(/grep -vxE '([^']+)'/); - assert.ok(m, `${label}: no \`grep -vxE '...'\` exclusion found`); - return m[1].split("|"); + const all = source.match(/grep -vxE '([^']+)'/g); + assert.ok(all, `${label}: no \`grep -vxE '...'\` exclusion found`); + // A second vendored tree with its own provenance job would bind every + // assertion below to whichever regex appears first, leaving the suite green + // while guarding the wrong job. + assert.equal( + all.length, + 1, + `${label}: expected exactly one \`grep -vxE '...'\` exclusion regex, found ${all.length}`, + ); + return source.match(/grep -vxE '([^']+)'/)[1].split("|"); } const workflow = read(".github/workflows/pr.yml"); diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md index 1e7fb724b6df..f6946a0f4c33 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md @@ -111,9 +111,20 @@ git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md|PROVENANCE-CHANGES\.md' \ are Blockcast additions, not upstream files — the hash covers only what came from upstream. The exclusion list here, in the `vendor_claude_k8s` CI step and on disk is held in agreement by `scripts/__tests__/provenance-union-merge.test.mjs`, -which fails if a non-upstream file exists that the regex does not name. The listing -comes from `git ls-files` rather than `find` so that `node_modules/`, `dist/` -and packed tarballs cannot perturb it. +which fails if the regex names a file that is **not** present in the tree — a +stale exclusion left behind by a rename would silently drop a real file from the +hash. + +**The reverse direction is not checked.** Adding a Blockcast-local file without +excluding it changes the hash rather than failing that test, and the remedy the +hash failure prescribes — regenerate — then widens the hash to cover a +non-upstream file. After that, "hash matches" no longer means "upstream is +unmodified". Checking that direction needs an explicit manifest of +Blockcast-added paths, which does not exist; until it does, extending this +exclusion list is a manual step to get right when adding a file here. + +The listing comes from `git ls-files` rather than `find` so that `node_modules/`, +`dist/` and packed tarballs cannot perturb it. CI enforces this: the `vendor_claude_k8s` job recomputes the hash and fails if it does not match the value recorded above. Change any vendored file and you