From 18bd2a4d7e098ecfe12842db432e5be3a587e204 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Sun, 20 Sep 2026 19:36:32 +0000 Subject: [PATCH 1/5] 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 69e97e0e4626..684e18ce749f 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 @@ -1226,7 +1231,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 68a7fcd15681..584fc8711dab 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 dea886fd6157d3fe57f2535c4d424a6c709e6483 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Mon, 21 Sep 2026 12:19:11 +0000 Subject: [PATCH 2/5] 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 584fc8711dab..f7aa9f6b4f1d 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 From 870c6e01501acf4ebd6c9fa33f322a7fe21d1ab6 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Mon, 21 Sep 2026 16:21:23 +0000 Subject: [PATCH 3/5] fix(vendor): take the version and the integrity hash out of the vendored tree (BLO-35109) BLO-34872 removed one of the five hunks that make any two concurrent vendored-adapter PRs conflict. This removes the other four. The remaining hunks were the 64-hex integrity hash and a version line recorded in three files. Both are single-valued by construction, so `merge=union` cannot reach either: a union keeps both sides' lines, which for the hash makes CI's `grep -oE '^[0-9a-f]{64}$' | head -1` resolve the provenance verdict by sort order rather than by the tree, and for the version produces invalid JSON. Version: stop bumping `-blockcast.N` per-PR. Measured 2026-09-21, the value appears in exactly five places and all five are inside the vendored directory; nothing outside reads it. The image builds the package from source and packs it with a glob (`mv paperclip-adapter-claude-k8s-*.tgz`), and the Dockerfile already says so: "claude_k8s - edit vendor/paperclip-adapter-claude-k8s/ and open a PR. Nothing to pin or bump." Three of the four hunks were ceremony for a number no consumer reads. Documentation-only change; no version is altered. Hash: delete it. It never attested upstream-ness -- it was recomputed from our own diverged tree, as the section it replaces admitted in its final paragraph. And it was a false-positive generator rather than a conflict detector: two PRs editing different lines of one vendored file merge correctly, and the combined tree's hash matched neither recorded value, so it failed on every combination of two changes, correct or not. What it was actually for -- vendored source does not change without being recorded -- is now checked directly by scripts/check-vendored-provenance-log.mjs: if a change touches vendored source, PROVENANCE-CHANGES.md must gain a row. A state invariant stored in the tree becomes a transition invariant read off the diff, so nothing is stored and nothing can conflict. The in-diff review surface is now the log row itself rather than an opaque hash no reviewer could verify. The guard also enforces append-only on the log unconditionally, which is what makes BLO-34872's `merge=union` safe: a union cannot reconcile an edit, so a rewritten row would be silently duplicated on the next concurrent append. Verified: - AC1: two scratch branches each editing a different vendored source file and appending a row. On master as measured: CONFLICT in PROVENANCE.md, package.json and package-lock.json. On this branch: rebased with no manual resolution, both rows preserved. - AC2/AC3: 13 tests in provenance-union-merge.test.mjs, including that no provenance file carries a 64-hex line at all, so no merge ordering can introduce a second candidate. - Mutation-tested per the standing rule: all ten guards reverted one at a time, each turning the suite red. Two guards that survived their first mutation were fixed rather than documented -- one test iterated the list under test, so deleting an entry deleted its own case; one filter was dead code and was removed. - actionlint clean on .github/workflows/pr.yml. Co-Authored-By: Claude --- .github/workflows/pr.yml | 48 +-- .../__tests__/provenance-union-merge.test.mjs | 342 +++++++++++++----- scripts/check-vendored-provenance-log.mjs | 113 ++++++ .../PROVENANCE-CHANGES.md | 19 +- .../PROVENANCE.md | 82 +++-- 5 files changed, 451 insertions(+), 153 deletions(-) create mode 100644 scripts/check-vendored-provenance-log.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 684e18ce749f..f96a4deb3600 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -305,6 +305,24 @@ jobs: run: node --test ./scripts/__tests__/provenance-union-merge.test.mjs timeout-minutes: 1 + - name: Require vendored-adapter source changes to be logged (BLO-35109) + # Replaces the stored 64-hex integrity manifest that used to live in + # vendor/paperclip-adapter-claude-k8s/PROVENANCE.md and was verified by + # the vendor_claude_k8s job. That hash was single-valued by construction, + # so every pair of concurrent vendored PRs produced two different values + # on one line and always conflicted -- and `merge=union` (BLO-34872) + # structurally cannot reach a single-valued field. This asserts the same + # property the hash existed for -- vendored source does not change + # without a provenance row -- off the diff, which stores nothing and so + # cannot conflict. + # + # Runs in this job rather than in vendor_claude_k8s because this one + # already checks out full history (fetch-depth: 0); the vendored job + # checks out at depth 1 and cannot resolve $PR_BASE_SHA. + if: ${{ !cancelled() }} + run: node ./scripts/check-vendored-provenance-log.mjs --base "$PR_BASE_SHA" --head "$PR_HEAD_SHA" + timeout-minutes: 1 + - name: Test policy node-test timeouts if: ${{ !cancelled() }} run: node --test ./scripts/__tests__/policy-node-test-timeouts.test.mjs @@ -1223,28 +1241,14 @@ jobs: - name: Test run: npm test - # Guards the PROVENANCE.md integrity manifest. If someone edits the - # vendored source, this hash changes and PROVENANCE.md must be updated - # in the same PR — otherwise the recorded provenance silently drifts - # from what is actually in the tree. - - name: Verify provenance manifest - run: | - set -euo pipefail - actual=$(git ls-files \ - | 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" - echo "recorded: $recorded" - if [ -z "$recorded" ]; then - echo "::error::No 64-hex integrity hash found in PROVENANCE.md." - exit 1 - fi - if [ "$actual" != "$recorded" ]; then - echo "::error::Vendored source changed but PROVENANCE.md integrity hash was not updated." - echo "Update the hash in vendor/paperclip-adapter-claude-k8s/PROVENANCE.md to: $actual" - exit 1 - fi + # The stored 64-hex integrity manifest that used to be verified here was + # removed under BLO-35109: it was single-valued, so every pair of + # concurrent vendored PRs conflicted on it, and it could not tell a bad + # merge from two good ones (it failed on every combination of two + # correctly-merged changes). The property it existed for -- vendored + # source does not change without a provenance row -- is now asserted off + # the diff by the `Require vendored-adapter source changes to be logged` + # step in the `policy` job, which stores nothing and so cannot conflict. verify: # Preserve the legacy required-check name while the underlying work runs in parallel. diff --git a/scripts/__tests__/provenance-union-merge.test.mjs b/scripts/__tests__/provenance-union-merge.test.mjs index 97183d7356e1..ef6e565ae1d1 100644 --- a/scripts/__tests__/provenance-union-merge.test.mjs +++ b/scripts/__tests__/provenance-union-merge.test.mjs @@ -1,30 +1,30 @@ import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; -import { readFileSync } from "node:fs"; -import { test } from "node:test"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { after, 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. +import { + LOG, + NOT_SOURCE, + VENDOR_DIR, + checkVendoredProvenanceLog, +} from "../check-vendored-provenance-log.mjs"; + +// BLO-34872 + BLO-35109. Two concurrent PRs touching the vendored adapter used to +// conflict on five hunks: an append-only log row, a 64-hex integrity hash, and a +// version line recorded in three files. BLO-34872 moved the log row into its own +// `merge=union` file. BLO-35109 removed the hash (single-valued, so a union would +// have made the provenance verdict depend on sort order) and stopped bumping the +// version per-PR (nothing outside the tree reads it). // -// 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. +// Both halves rest on agreements that nothing else checks and that fail silently +// -- a union-merged file quietly acquiring a single-valued field, a guard whose +// exclusion list rots past a rename, a log file nothing forces anyone to append +// to. Hence tests rather than comments. 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; @@ -39,100 +39,254 @@ function unionMergedPaths() { .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 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"); -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"), - ); -}); +// -------------------------------------------------------------------------- +// Static invariants over the real tree +// -------------------------------------------------------------------------- -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)); +test("no provenance file carries a 64-hex line (BLO-35109 AC3)", () => { + // Zero candidates, so no merge ordering can ever introduce a second. This is + // the assertion that catches someone reviving the stored integrity manifest: + // a single-valued field in a union-merged file is resolved by sort order + // rather than by the tree, and one of the two orderings fails permissively. + for (const path of [`${VENDOR_DIR}/PROVENANCE.md`, LOG]) { + 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.`, + `${path} contains a 64-hex line. Single-valued fields cannot live in this tree: they conflict on every concurrent PR, and moving one into the union-merged log would let a union keep both candidates.`, ); } - - 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", () => { +test("the append-only log is union-merged and PROVENANCE.md is not", () => { + const union = unionMergedPaths(); + assert.ok( + union.includes(LOG), + `${LOG} must be marked merge=union in .gitattributes; without it every concurrent append conflicts again`, + ); 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", + !union.includes(`${VENDOR_DIR}/PROVENANCE.md`), + "PROVENANCE.md is prose and tables; a union cannot reconcile an interior edit", ); }); -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. +test("the guard's non-source list names only files present in the tree", () => { + // The other half of a rename: a stale entry would silently stop requiring a + // log row for a file that still exists under a new name. const tracked = new Set( execFileSync("git", ["ls-files"], { cwd: new URL(`${VENDOR_DIR}/`, repoRoot), encoding: "utf8", }) .split("\n") - .filter(Boolean), + .filter(Boolean) + .map((p) => `${VENDOR_DIR}/${p}`), ); - for (const alternative of ciExclusionAlternatives(workflow, ".github/workflows/pr.yml")) { - const filename = alternative.replace(/\\/g, ""); + for (const path of NOT_SOURCE) { 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.`, + tracked.has(path), + `check-vendored-provenance-log.mjs excludes '${path}', which is not tracked under ${VENDOR_DIR}. A stale exclusion stops requiring a provenance row for a real file.`, ); } }); + +test("CI runs the guard, and no longer verifies a stored manifest", () => { + const workflow = read(".github/workflows/pr.yml"); + assert.match( + workflow, + /node \.\/scripts\/check-vendored-provenance-log\.mjs --base "\$PR_BASE_SHA" --head "\$PR_HEAD_SHA"/, + "the provenance-log guard is not wired into .github/workflows/pr.yml", + ); + assert.doesNotMatch( + workflow, + /grep -oE '\^\[0-9a-f\]\{64\}\$'/, + "pr.yml still reads a stored 64-hex manifest; that is the single-valued field BLO-35109 removed", + ); +}); + +// -------------------------------------------------------------------------- +// Behaviour of the guard, against real git history +// -------------------------------------------------------------------------- + +const SOURCE = `${VENDOR_DIR}/src/server/job-manifest.ts`; +const SECOND_SOURCE = `${VENDOR_DIR}/src/server/execute.ts`; +const LOG_HEADER = "| commit | files | what |\n|---|---|---|\n| `seed` | x | y |\n"; + +const scratchDirs = []; +after(() => { + for (const dir of scratchDirs) rmSync(dir, { recursive: true, force: true }); +}); + +function scratchRepo() { + const dir = mkdtempSync(join(tmpdir(), "provenance-guard-")); + scratchDirs.push(dir); + const git = (...args) => execFileSync("git", args, { cwd: dir, encoding: "utf8" }); + const write = (rel, body) => { + mkdirSync(dirname(join(dir, rel)), { recursive: true }); + writeFileSync(join(dir, rel), body); + }; + + git("init", "--quiet", "--initial-branch=master"); + git("config", "user.email", "test@example.com"); + git("config", "user.name", "test"); + + write(".gitattributes", `${LOG} merge=union\n`); + write(`${VENDOR_DIR}/LICENSE`, "MIT\n"); + write(SOURCE, "export const manifest = 1;\n"); + write(SECOND_SOURCE, "export const execute = 1;\n"); + write(`${VENDOR_DIR}/PROVENANCE.md`, "# Provenance\n"); + write(LOG, LOG_HEADER); + git("add", "-A"); + git("commit", "--quiet", "-m", "seed"); + + const base = git("rev-parse", "HEAD").trim(); + const commit = (message) => { + git("add", "-A"); + git("commit", "--quiet", "-m", message); + }; + const check = () => checkVendoredProvenanceLog({ base, head: "HEAD", cwd: dir }); + return { dir, git, write, commit, check, base }; +} + +test("a vendored source change with no log row is rejected", () => { + const { write, commit, check } = scratchRepo(); + write(SOURCE, "export const manifest = 2;\n"); + commit("touch vendored source"); + + const result = check(); + assert.equal(result.ok, false); + assert.match(result.reason, /gained no row/); + assert.ok( + result.detail.some((line) => line.includes(SOURCE)), + "the failure should name the changed file", + ); +}); + +test("a vendored source change with an appended log row passes", () => { + const { write, commit, check } = scratchRepo(); + write(SOURCE, "export const manifest = 2;\n"); + write(LOG, `${LOG_HEADER}| \`abc\` | job-manifest.ts | bumped the manifest |\n`); + commit("touch vendored source and log it"); + + assert.deepEqual(check(), { ok: true }); +}); + +test("deleting a line from the append-only log is rejected", () => { + // Union merge keeps both sides' added lines and cannot reconcile an edit, so + // an edited or reordered row would be silently duplicated on the next + // concurrent append. In a diff, an edit is a deletion. + const { write, commit, check } = scratchRepo(); + write(SOURCE, "export const manifest = 2;\n"); + write(LOG, "| commit | files | what |\n|---|---|---|\n| `seed` | x | EDITED |\n"); + commit("rewrite an existing log row"); + + const result = check(); + assert.equal(result.ok, false); + assert.match(result.reason, /append-only/); +}); + +test("editing a log row is rejected even with no source change", () => { + // The append-only rule is unconditional: `merge=union` duplicates a rewritten + // row on the next concurrent append whether or not the same PR touched source. + const { write, commit, check } = scratchRepo(); + write(LOG, "| commit | files | what |\n|---|---|---|\n| `seed` | x | EDITED |\n"); + commit("rewrite an existing log row, nothing else"); + + const result = check(); + assert.equal(result.ok, false); + assert.match(result.reason, /append-only/); +}); + +test("appending a log row on its own is not itself a change needing a row", () => { + const { write, commit, check } = scratchRepo(); + write(LOG, `${LOG_HEADER}| \`abc\` | - | a note |\n`); + commit("log append only"); + + assert.deepEqual(check(), { ok: true }); +}); + +test("changing only the Blockcast-added provenance files needs no row", () => { + // Paths are written out rather than read off NOT_SOURCE on purpose. Iterating + // the list under test means deleting an entry deletes its own case, so the + // suite stays green on exactly the change it exists to catch. + for (const path of [`${VENDOR_DIR}/LICENSE`, `${VENDOR_DIR}/PROVENANCE.md`]) { + const { write, commit, check } = scratchRepo(); + write(path, "Blockcast-added, not upstream source.\n"); + commit(`touch ${path}`); + + assert.deepEqual(check(), { ok: true }, `${path} should not require a log row`); + } + + // And the other direction: the list must not quietly grow to cover real + // source, which would stop requiring a row for it. + assert.deepEqual(NOT_SOURCE, [`${VENDOR_DIR}/LICENSE`, `${VENDOR_DIR}/PROVENANCE.md`]); +}); + +test("a change that does not touch the vendored tree at all passes", () => { + const { write, commit, check } = scratchRepo(); + write("README.md", "unrelated\n"); + commit("unrelated"); + + assert.deepEqual(check(), { ok: true }); +}); + +test("commits that landed on the base branch are not attributed to this change", () => { + // $PR_BASE_SHA is the base branch's *tip*, not the merge base. A two-dot diff + // against it reports the base's own log rows as deletions, so an unrelated + // append on master would fail every open vendored PR with a bogus + // "append-only" violation. The three-dot range is what prevents that. + const { dir, git, write, commit, base } = scratchRepo(); + + git("checkout", "--quiet", "-b", "feature"); + write(SOURCE, "export const manifest = 2;\n"); + write(LOG, `${LOG_HEADER}| \`fff\` | job-manifest.ts | the PR's own change |\n`); + commit("feature work, logged"); + + git("checkout", "--quiet", "master"); + write(LOG, `${LOG_HEADER}| \`mmm\` | - | landed on master after the branch point |\n`); + commit("unrelated master append"); + const baseTip = git("rev-parse", "HEAD").trim(); + + assert.notEqual(baseTip, base, "master must have moved for this test to mean anything"); + git("checkout", "--quiet", "feature"); + + assert.deepEqual( + checkVendoredProvenanceLog({ base: baseTip, head: "HEAD", cwd: dir }), + { ok: true }, + ); +}); + +// -------------------------------------------------------------------------- +// The whole point: two concurrent vendored changes rebase without conflict +// -------------------------------------------------------------------------- + +test("two concurrent realistic vendored changes rebase with no conflict (BLO-35109 AC1)", () => { + const { dir, git, write, commit, base } = scratchRepo(); + + // Branch A: edits one vendored source file and appends its row. + git("checkout", "--quiet", "-b", "branch-a"); + write(SOURCE, "export const manifest = 2;\n"); + write(LOG, `${LOG_HEADER}| \`aaa\` | job-manifest.ts | change A |\n`); + commit("change A"); + + // Branch B: edits a different vendored source file and appends its own row. + git("checkout", "--quiet", "-b", "branch-b", base); + write(SECOND_SOURCE, "export const execute = 2;\n"); + write(LOG, `${LOG_HEADER}| \`bbb\` | execute.ts | change B |\n`); + commit("change B"); + + // Before BLO-34872 + BLO-35109 this rebase conflicted every time: on the log + // row, on the 64-hex hash, and on the version line in three files. + git("rebase", "branch-a"); + + const merged = readFileSync(join(dir, LOG), "utf8"); + assert.ok(merged.includes("change A"), "union merge dropped branch A's row"); + assert.ok(merged.includes("change B"), "union merge dropped branch B's row"); + assert.ok(!merged.includes("<<<<<<<"), "the rebase left conflict markers"); + + // And the rebased result still satisfies the guard. + assert.deepEqual(checkVendoredProvenanceLog({ base, head: "HEAD", cwd: dir }), { ok: true }); +}); diff --git a/scripts/check-vendored-provenance-log.mjs b/scripts/check-vendored-provenance-log.mjs new file mode 100644 index 000000000000..d180858d2e17 --- /dev/null +++ b/scripts/check-vendored-provenance-log.mjs @@ -0,0 +1,113 @@ +#!/usr/bin/env node +// BLO-35109. Replaces the stored 64-hex integrity manifest that used to live in +// vendor/paperclip-adapter-claude-k8s/PROVENANCE.md. +// +// That hash was single-valued by construction, so two concurrent PRs that each +// touched the vendored tree always produced two different values on one line and +// always conflicted -- and `merge=union` (BLO-34872) structurally cannot reach a +// single-valued field: a union would keep both hashes and CI's +// `grep -oE '^[0-9a-f]{64}$' | head -1` would then resolve the provenance verdict +// by sort order rather than by the tree. +// +// The hash never attested upstream-ness -- it was recomputed from our own tree, +// which has diverged. Its only real job was to force a PROVENANCE edit whenever +// vendored source changed. This checks that directly instead: a state invariant +// (a stored constant, which every PR must rewrite) becomes a transition +// invariant (a diff, which nothing stores and nothing can conflict on). +// +// It is also strictly less false-positive than the hash was. Two PRs editing +// different lines of the same vendored file merge correctly, and the combined +// tree's hash matched neither recorded value -- so the hash failed every +// combination, correct or not. It could not tell a bad merge from two good ones. +// +// Usage: node scripts/check-vendored-provenance-log.mjs --base [--head ] +import { execFileSync } from "node:child_process"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +export const VENDOR_DIR = "vendor/paperclip-adapter-claude-k8s"; +export const LOG = `${VENDOR_DIR}/PROVENANCE-CHANGES.md`; + +// Blockcast additions, not vendored source. Changing one of these alone is not a +// vendored change and needs no log row. +// +// The log file is deliberately NOT listed, and that is not an oversight: a +// change touching only the log is already satisfied by the row it appends, so +// an entry for it has no failing mutation -- i.e. it would be a comment wearing +// a guard's clothes. Measured: removing it leaves the suite green. +export const NOT_SOURCE = [`${VENDOR_DIR}/LICENSE`, `${VENDOR_DIR}/PROVENANCE.md`]; + +/** + * @returns {{ok: true} | {ok: false, reason: string, detail: string[]}} + */ +export function checkVendoredProvenanceLog({ base, head = "HEAD", cwd }) { + const git = (...args) => execFileSync("git", args, { encoding: "utf8", cwd }); + // Three-dot: diff against the merge base, so base-branch commits landed since + // the branch point do not masquerade as changes made by this PR. + const range = `${base}...${head}`; + + const numstat = git("diff", "--numstat", range, "--", LOG).trim(); + const [added, deleted] = numstat + ? numstat.split("\t").slice(0, 2).map(Number) + : [0, 0]; + + // Unconditional, because append-only is what makes `merge=union` on this file + // safe at all: a union resolves by keeping both sides' added lines and cannot + // reconcile an edit, so a rewritten row would be silently duplicated on the + // next concurrent append. In a diff, an edit or a reorder is a deletion. + if (deleted > 0) { + return { + ok: false, + reason: `${LOG} is append-only, but this change removes ${deleted} line(s) from it.`, + detail: [ + "Append new rows at the end; never edit or reorder existing ones.", + "To correct an earlier row, append a row that supersedes it.", + ], + }; + } + + const changed = git("diff", "--name-only", range, "--", VENDOR_DIR) + .split("\n") + .filter(Boolean) + .filter((p) => !NOT_SOURCE.includes(p)); + + if (changed.length === 0) return { ok: true }; + + if (added < 1) { + return { + ok: false, + reason: `Vendored source changed but ${LOG} gained no row.`, + detail: [ + "Append one row describing the change, so the recorded provenance does", + "not silently drift from what is actually in the tree. Changed files:", + ...changed.map((p) => ` ${p}`), + ], + }; + } + + return { ok: true }; +} + +const arg = (name) => { + const i = process.argv.indexOf(name); + return i < 0 ? undefined : process.argv[i + 1]; +}; + +export function isMainModule(argvPath = process.argv[1], moduleUrl = import.meta.url) { + return Boolean(argvPath) && resolve(argvPath) === fileURLToPath(moduleUrl); +} + +if (isMainModule()) { + const base = arg("--base"); + if (!base) { + console.error("usage: --base [--head ]"); + process.exit(2); + } + const result = checkVendoredProvenanceLog({ base, head: arg("--head") ?? "HEAD" }); + if (!result.ok) { + console.error(`::error::${result.reason}`); + for (const line of result.detail) console.error(line); + process.exit(1); + } + console.log(`${LOG}: ok`); +} diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md index 2f339cc95f43..fbc801180adc 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md @@ -12,15 +12,22 @@ 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`. + `scripts/check-vendored-provenance-log.mjs` fails any change that deletes a + line from this file — which is what an edit or a reorder looks like in a + diff. To correct an earlier row, append a row that supersedes it. +2. **Never put a 64-hex string in this file.** A union keeps both sides' lines, + so any single-valued field placed here acquires a second candidate on the + next concurrent append and is then resolved by sort order rather than by the + tree. This is why the integrity hash was *removed* from `PROVENANCE.md` + under [BLO-35109](https://paperclip.blockcast.net/BLO/issues/BLO-35109) + rather than moved here — see [Integrity](./PROVENANCE.md#integrity). 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 +Appending a row here is **required**, not conventional: CI fails any change that +touches vendored source without one. That guard replaced the stored integrity +hash, which is what used to force a PROVENANCE edit. See +`scripts/check-vendored-provenance-log.mjs`, wired into the `policy` job and asserted by `scripts/__tests__/provenance-union-merge.test.mjs`. | commit | files | what | diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md index f7aa9f6b4f1d..fac68c6b066c 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md @@ -92,36 +92,41 @@ applied since. ### Integrity -A manifest of `sha256(path)` over all 41 in-tree files, sorted by path under -`LC_ALL=C`, itself hashes to: - -``` -8c3a5f3d741ff0567bbe9f4a33ff7e9b520396dbc3cebe635d12d23b5f81fdf4 -``` - -Regenerate with: - -```sh -cd vendor/paperclip-adapter-claude-k8s -git ls-files | grep -vxE 'LICENSE|PROVENANCE\.md|PROVENANCE-CHANGES\.md' \ - | LC_ALL=C sort | xargs sha256sum | sha256sum -``` - -`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 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. +**There is no recorded integrity hash, deliberately — removed under +[BLO-35109](https://paperclip.blockcast.net/BLO/issues/BLO-35109).** What +replaced it is the append-only log in +[PROVENANCE-CHANGES.md](./PROVENANCE-CHANGES.md): CI fails any change that +touches vendored source without appending a row there +(`scripts/check-vendored-provenance-log.mjs`, run from the `policy` job). + +A single 64-hex manifest of the tree used to be recorded here and recomputed by +CI. It was removed for three reasons, in ascending order of importance: + +1. **It did not attest what it appeared to.** The hash was recomputed from *our* + tree, which has diverged from upstream — so "hash matches" never meant + "upstream is unmodified", only "the tree is what the last editor recorded". + The section that stood here said as much in its final paragraph. +2. **It was a false-positive generator, not a conflict detector.** Two PRs + editing different lines of the same vendored file merge correctly, and the + combined tree's hash matched *neither* recorded value. It failed on every + combination of two changes, correct or not, so it could not tell a bad merge + from two good ones. +3. **It was single-valued, so it made concurrent work serial.** Every pair of + PRs touching this tree conflicted on that one line. `merge=union` + (BLO-34872) cannot reach it: a union keeps both sides' lines, and CI's + `grep -oE '^[0-9a-f]{64}$' … | head -1` would then have resolved the + provenance verdict by sort order rather than by the tree — failing + permissively on one of the two orderings. + +The property the hash existed for — vendored source does not change without the +change being recorded — survives, as a *transition* invariant checked against +the merge base instead of a *state* invariant stored in the file. Nothing is +stored, so nothing can conflict, and the in-diff review surface is now the log +row itself rather than an opaque hash nobody could verify by reading. + +`scripts/__tests__/provenance-union-merge.test.mjs` asserts that no 64-hex line +is reintroduced into either provenance file, so a future revival is caught +rather than quietly re-creating the conflict. The listing comes from `git ls-files` rather than `find` so that `node_modules/`, `dist/` and packed tarballs cannot perturb it. @@ -192,7 +197,22 @@ prerelease identifiers would have decided it — and `blockcast` sorts *below* `kkroo` alphabetically, making the release read as a downgrade to anything comparing versions. -Bump `-blockcast.N` for subsequent changes to this directory. +Bump `-blockcast.N` **only when something outside this directory needs to tell +two builds of it apart** — which, as of +[BLO-35109](https://paperclip.blockcast.net/BLO/issues/BLO-35109), nothing does. +Do **not** bump it per-PR. + +Measured 2026-09-21: the `-blockcast.N` version string appears in exactly five +places, all of them inside this directory (`package.json`, `package-lock.json` +×2, and twice in this file). Nothing outside the vendored tree reads it. The +image builds this package from source and packs it with a glob — +`mv paperclip-adapter-claude-k8s-*.tgz` — so the number never reaches the +Dockerfile, which says so itself: *"claude_k8s — edit +vendor/paperclip-adapter-claude-k8s/ and open a PR. Nothing to pin or bump."* + +Bumping it per-PR was not free. It put a version line in five places into every +vendored PR's diff, which is three of the four hunks that used to make any two +concurrent PRs on this tree conflict — for a number no consumer reads. ### The inert upstream workflow From b4a1997c9eb2e060839ca9335f34a5920eed89a9 Mon Sep 17 00:00:00 2001 From: PlatformSREEngineer Date: Tue, 22 Sep 2026 19:08:52 +0000 Subject: [PATCH 4/5] fix(vendor): reject a binary provenance log instead of passing it (BLO-35109) Addresses both Important findings from Ally's review at 9ab2b172b. 1. `check-vendored-provenance-log.mjs` parsed `git diff --numstat` with `map(Number)`. git emits `-\t-` for a blob it treats as binary, so both counts became NaN and every comparison against them was false -- the guard passed on exactly the input it exists to reject. A stray NUL is enough to disable the append-only rule silently, and append-only is what makes `merge=union` safe on that file at all. Now rejected explicitly. 2. `.gitattributes` justified excluding PROVENANCE.md from `merge=union` by pointing at the 64-hex hash -- which this same PR deletes. The exclusion is still right, but its only stated reason was checkably false, so a reader who verified it would find the hash gone and could widen the union to the very file the line exists to protect. Restated against what remains true. Also takes the review's first suggestion: the require-a-row check counted added *lines*, so a blank line satisfied it. It now requires an added line shaped like a table row. Both new guards are mutation-tested individually and each kills its own test: non-finite check removed -> `a log git treats as binary...` fails; row-shape reverted to the bare count -> `a blank added line...` fails. Control 15/15. Co-Authored-By: Paperclip --- .gitattributes | 18 ++++++-- .../__tests__/provenance-union-merge.test.mjs | 42 +++++++++++++++++++ scripts/check-vendored-provenance-log.mjs | 28 ++++++++++++- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/.gitattributes b/.gitattributes index 8dd5e8f131e0..ade065d469e1 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,8 +3,18 @@ # 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. +# Scoped to this one file deliberately, and it must stay scoped. A union merge +# is only safe on an append-only list of independent rows: it resolves by +# keeping both sides' added lines, so it cannot reconcile an interior edit and +# cannot choose between two values of a field that has only one. PROVENANCE.md +# is prose and single-valued tables, so union-merging it would interleave both +# sides' text rather than merge it. +# +# That is also why nothing single-valued may move *into* this file. BLO-35109 +# deleted the 64-hex integrity hash that used to live in PROVENANCE.md for +# exactly this reason: under a union both candidates survive, and CI's +# `grep … | head -1` would then resolve the provenance verdict by sort order +# rather than by the tree. `provenance-union-merge.test.mjs` asserts both +# halves -- that this file is union-merged, that PROVENANCE.md is not, and that +# neither carries a 64-hex line. vendor/paperclip-adapter-claude-k8s/PROVENANCE-CHANGES.md merge=union diff --git a/scripts/__tests__/provenance-union-merge.test.mjs b/scripts/__tests__/provenance-union-merge.test.mjs index ef6e565ae1d1..a2e4d2798fea 100644 --- a/scripts/__tests__/provenance-union-merge.test.mjs +++ b/scripts/__tests__/provenance-union-merge.test.mjs @@ -225,6 +225,48 @@ test("changing only the Blockcast-added provenance files needs no row", () => { assert.deepEqual(NOT_SOURCE, [`${VENDOR_DIR}/LICENSE`, `${VENDOR_DIR}/PROVENANCE.md`]); }); +test("a log git treats as binary is rejected, not silently passed", () => { + // `git diff --numstat` emits `-\t-` for a binary blob, so both counts parse + // to NaN and every comparison against them is false. Without an explicit + // non-finite check the guard passes on exactly the input it exists to + // reject. A stray NUL from a bad editor or a pasted binary snippet is enough. + // + // Asserting the *reason* is what makes this a real mutation test: drop the + // non-finite check and the destructive rewrite below stops being reported as + // an unverifiable file, which is the defect. The no-source-change case is + // the pure fail-open -- with the check gone it returns ok:true outright. + const withSource = scratchRepo(); + withSource.write(SOURCE, "export const manifest = 2;\n"); + withSource.write(LOG, `${LOG_HEADER}| \`abc\` | job\0manifest.ts | binary now |\n`); + withSource.commit("rewrite the log as a binary blob, and touch source"); + + const a = withSource.check(); + assert.equal(a.ok, false); + assert.match(a.reason, /not a text file/); + + const logOnly = scratchRepo(); + logOnly.write(LOG, "| commit | files | what |\n|---|---|---|\n| `seed` | x | \0 |\n"); + logOnly.commit("destructively rewrite the log as a binary blob"); + + const b = logOnly.check(); + assert.equal(b.ok, false, "a binary log must never verify as ok"); + assert.match(b.reason, /not a text file/); +}); + +test("a blank added line does not satisfy the require-a-row guard", () => { + // `added > 0` counts lines, and a blank line is a line. Requiring an added + // line shaped like a table row keeps the cheapest way to silence the guard + // being to actually write the row. + const { write, commit, check } = scratchRepo(); + write(SOURCE, "export const manifest = 2;\n"); + write(LOG, `${LOG_HEADER}\n \n`); + commit("touch vendored source, append only whitespace"); + + const result = check(); + assert.equal(result.ok, false); + assert.match(result.reason, /gained no row/); +}); + test("a change that does not touch the vendored tree at all passes", () => { const { write, commit, check } = scratchRepo(); write("README.md", "unrelated\n"); diff --git a/scripts/check-vendored-provenance-log.mjs b/scripts/check-vendored-provenance-log.mjs index d180858d2e17..8f1f6e32d6f0 100644 --- a/scripts/check-vendored-provenance-log.mjs +++ b/scripts/check-vendored-provenance-log.mjs @@ -51,6 +51,23 @@ export function checkVendoredProvenanceLog({ base, head = "HEAD", cwd }) { ? numstat.split("\t").slice(0, 2).map(Number) : [0, 0]; + // `git diff --numstat` emits `-\t-` for a blob it treats as binary, so both + // counts parse to NaN and every comparison below is false: the guard would + // pass on exactly the input it exists to reject -- including a destructively + // rewritten log, which is the case the append-only rule is here for. Reject + // non-finite rather than comparing against it. + if (!Number.isFinite(added) || !Number.isFinite(deleted)) { + return { + ok: false, + reason: `${LOG} is not a text file; provenance cannot be verified.`, + detail: [ + "git reports it as binary, so added/removed rows cannot be counted and", + "the append-only rule cannot be enforced. Check for a stray NUL byte or", + "a non-UTF-8 encoding, and restore the file as UTF-8 text.", + ], + }; + } + // Unconditional, because append-only is what makes `merge=union` on this file // safe at all: a union resolves by keeping both sides' added lines and cannot // reconcile an edit, so a rewritten row would be silently duplicated on the @@ -73,7 +90,16 @@ export function checkVendoredProvenanceLog({ base, head = "HEAD", cwd }) { if (changed.length === 0) return { ok: true }; - if (added < 1) { + // Count added *rows*, not added lines: a bare `added > 0` is satisfied by a + // blank line, so the cheapest way to silence the guard would be to add + // nothing. Row quality is still left to human review -- this only rules out + // the whitespace-only satisfier. The `+++ b/path` diff header cannot match, + // since the character after its leading `+` is neither space nor `|`. + const addedRows = git("diff", "--unified=0", range, "--", LOG) + .split("\n") + .filter((line) => /^\+\s*\|/.test(line)); + + if (addedRows.length < 1) { return { ok: false, reason: `Vendored source changed but ${LOG} gained no row.`, From 6be3bbf7f4e48d899b9b7b7a458b974cb0138236 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Wed, 23 Sep 2026 00:37:38 +0000 Subject: [PATCH 5/5] docs(vendor): drop the stale integrity-hash mandate in PROVENANCE.md Ally (Important, at b4a1997c) found that PROVENANCE.md:177-179 still said in bold that any vendored change must update the integrity hash and that the vendor_claude_k8s job fails otherwise. Both halves are false at this head: BLO-35109 deleted the hash (PROVENANCE.md:95 says so) and this PR removed the CI step that printed it. A contributor following the mandate would restore a hash, which is exactly what the AC3 test forbids. A milder instance at :170 said the changes log is excluded from the hash below. Replace the mandate with the real gate: a row appended to PROVENANCE-CHANGES.md, enforced by scripts/check-vendored-provenance-log.mjs from the policy job. Drop the "integrity hash below" clause at :170. PROVENANCE.md is in the guard's NOT_SOURCE list, so no log row is needed. Also take Ally's suggestion: one assertion in provenance-union-merge.test.mjs that PROVENANCE.md contains no "must update the integrity hash" prose, so the class is closed rather than the instance. Controls: node --test scripts/__tests__/provenance-union-merge.test.mjs 16/16 pass; node scripts/check-vendored-provenance-log.mjs --base origin/master --head HEAD ok; grep -c '^[0-9a-f]{64}$' over vendor/**/PROVENANCE*.md is 0. Negative: reverting the doc edit fails the new assertion (15/16); appending a 64-hex line fails AC3 (15/16); both restored. Co-Authored-By: Claude Fable 5.1 --- scripts/__tests__/provenance-union-merge.test.mjs | 11 +++++++++++ vendor/paperclip-adapter-claude-k8s/PROVENANCE.md | 10 ++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/scripts/__tests__/provenance-union-merge.test.mjs b/scripts/__tests__/provenance-union-merge.test.mjs index a2e4d2798fea..30bf0a26eacd 100644 --- a/scripts/__tests__/provenance-union-merge.test.mjs +++ b/scripts/__tests__/provenance-union-merge.test.mjs @@ -60,6 +60,17 @@ test("no provenance file carries a 64-hex line (BLO-35109 AC3)", () => { } }); +test("PROVENANCE.md does not instruct contributors to update the deleted hash", () => { + // The content check above cannot see prose. A surviving "update the integrity + // hash" mandate sends a contributor looking for a hash that no longer exists, + // and the good-faith repair is to restore one, tripping the AC3 assertion. + assert.doesNotMatch( + read(`${VENDOR_DIR}/PROVENANCE.md`), + /must\s+update the integrity hash/i, + "PROVENANCE.md still mandates updating a hash BLO-35109 deleted", + ); +}); + test("the append-only log is union-merged and PROVENANCE.md is not", () => { const union = unionMergedPaths(); assert.ok( diff --git a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md index fac68c6b066c..0994504d62d3 100644 --- a/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md +++ b/vendor/paperclip-adapter-claude-k8s/PROVENANCE.md @@ -167,16 +167,18 @@ upstream**, so they are enumerated here rather than left implicit. 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. +(BLO-34872). The two cherry-picked commits in the composition above remain upstream commits authored against the fork, not Blockcast-local patches. Future changes to this directory are ordinary in-tree changes to this repository: edit, open a PR, let CI run. There is no longer an external fork to -push to first, and `CLAUDE_K8S_REF` no longer exists. **Any change here must -update the integrity hash in the same PR** — CI fails the `vendor_claude_k8s` -job otherwise, and prints the expected value. +push to first, and `CLAUDE_K8S_REF` no longer exists. There is no integrity +hash to update (see [Integrity](#integrity)). What gates a vendored change now +is a row appended to [PROVENANCE-CHANGES.md](./PROVENANCE-CHANGES.md): +`scripts/check-vendored-provenance-log.mjs`, run from the `policy` job, fails +any PR that touches vendored source without one. ### Versioning