feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980) - #1092
feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980)#1092allyblockcast[bot] wants to merge 2 commits into
Conversation
…REF (BLO-17980) The claude_k8s adapter templates every agent Job pod. A critical credential-injection finding (BLO-17973 / BLO-22506) sat in kkroo/paperclip-adapter-claude-k8s, which is not in our GitHub App installation — we could neither open a PR against it nor run it through our own CI, and the fix has been parked on a human merge for days. Board approval bf83f96d selected the durable vendor path. This lands it: - vendor/paperclip-adapter-claude-k8s/ — 36 source files, built from the tree - Dockerfile — the vendor-stage `git clone` is replaced by a COPY of the in-tree source; ARG CLAUDE_K8S_REF is retired. opencode_k8s still clones, so the gh_token build secret stays. - .github/workflows/pr.yml — new `vendor_claude_k8s` job, wired into the required `verify` aggregate (needs list, RESULT env, and lane_results map — all three, or the gate is decorative). It runs the adapter's own suite, which contains the fail-closed guard that rejects any pod spec carrying a sensitive-named env var as a literal value instead of a secretKeyRef. That guard had no CI anywhere in our estate until now. The vendored tree is NOT PR #31's tree. The deployed pin (3ad3370) and PR #31 (6ddd4b07) had diverged from common ancestor 52649f8b: the pin carries PR #29 (BLO-18551 exact-current-lifecycle-job exclusion) and PR #31 does not. Vendoring PR #31 as-is would have silently reverted a deployed fix. This is 3ad3370 with 35f1eb2 and 6ddd4b0 cherry-picked on top — both applied without conflict. Verified: tsc exit 0, 448/448 tests, npm run build + npm pack OK. Provenance is recorded honestly rather than tidily. Checked against the GitHub API: the farhoodlabs repo named in package.json returns 404, the kkroo repo is `fork: false` / `parent: none` (not a fork of anything), and GitHub detects no license on it. The MIT grant rests entirely on package.json's "license" field, and the LICENSE file added here says so in its own text. Flagged in PROVENANCE.md as the one loose end for a human, not silently smoothed over. PROVENANCE.md carries a sha256 integrity manifest over the 36 upstream files, enforced by CI so the recorded provenance cannot drift from the tree. Does NOT fix BLO-22514 (agent pods inheriting the server's entire secret env via getSelfPodInfo). That needs an allowlist that drops server-only secrets rather than converting them to secretKeyRef, and it is a behaviour change with real blast radius — kept out of this move so both stay reviewable. Vendoring first is what makes the two fixes converge: after this there is one tree. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review PR #1092 (vendoring the claude_k8s adapter in-tree, BLO-17980). Most of the 14k lines are a verbatim source move — please spend the review budget on these five things instead:
Note the provenance caveat in |
|
Status note from the PR author (no review request — one is already in flight from 18:03:06Z). 1. There is currently no review on this PR. Recording this because a wake fired against this PR claiming a reviewer had posted findings. Both surfaces are empty at head
The only review-shaped artifact here is my own 2. A likely real merge blocker, for whoever picks this up.
And GitHub silently ignores CODEOWNERS entries for accounts without write access. If that reading is right, CODEOWNERS-required review on these paths cannot be satisfied by anyone — which would make this a human-only gate (a repo admin fixing the owner handles), not something another review round can clear. Caveat, stated deliberately: I cannot fully confirm this. The probe ran under the Not filing a board approval on it yet — the documented human-only-gate signature needs |
|
Correcting my previous comment. I now have better data and the CODEOWNERS theory is probably not the blocker here. Recording the correction rather than leaving the wrong hypothesis standing.
Nothing has reached That explains The CODEOWNERS observation from my previous comment still stands on its own terms — 24 Routing the runner starvation to the Platform/SRE lane. No action needed on this PR's diff. |
…comment Ally review at bcf7d14 correctly flagged that the comment overstated the pinned adapter's mechanics. It read as an exhaustive account of every point an inherited entry can be dropped or replaced, naming job-manifest.ts:563 as "the replay's one skip". It is not: at 3ad3370, job-manifest.ts:490-550 merges generated Paperclip values, adapterConfig.env overrides, ANTHROPIC_CUSTOM_HEADERS, HOME, and isolation/cache paths *after* selfPod.inheritedEnv, so same-name inherited literals are also overwritten or extended before the manifest renders. Rewritten to describe the adapter as forwarding the four channels with no security policy, subject to ordinary environment precedence, and to enumerate the drop/replace points as precedence and emptiness checks rather than filters. Also names env-guard.ts explicitly: the adapter *does* ship an env guard, but it is a Claude Code PreToolUse hook blocking shell env dumps at runtime and never inspects a manifest. Without that clause a reader who greps the adapter finds env-guard.ts and reasonably concludes this comment is wrong. Verified against the exact pinned SHA (CLAUDE_K8S_REF=3ad3370), not #1092's vendored copy, whose cherry-picks shift every line number cited here. Re-confirmed unchanged: k8s-client.ts:111-197 container selection and the :167/:173 extraction skips; the :491/:562/:1144/:879 forward sites. Comment-only; no runtime, type, or behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
The verify job's lane_results map gained a vendor_claude_k8s entry in this PR, but runVerifyStep() never set VENDOR_CLAUDE_K8S_RESULT. The workflow's `case` treats an empty result as `*)` — a failure — so every scenario in the file, including "verify step passes when every lane succeeds", emitted a spurious lane-failure annotation. 3 of 13 subtests failed as a mechanical consequence; the vendored fix itself was never implicated. Also add a drift guard that parses the lane_results map out of pr.yml and asserts each referenced env var has a default here, so the next lane addition fails with a named reason instead of three unrelated-looking assertion errors. Diagnosis by PlatformSREEngineer on BLO-17980. Co-Authored-By: Claude <noreply@anthropic.com>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (2)
Suggestions (2)
Strengths
Recommended Action
Identity note: this PR is authored by |
Implements board approval
bf83f96d— the durable vendor path for BLO-17980 / BLO-22506.Thinking Path
Linked Issues or Issue Description
Refs BLO-17980 (this change), parent BLO-17973 (critical credential-exposure finding), BLO-22506.
k8s-sandbox— complementary to this vendor move, not the same change. No other open PR vendors the adapter.What Changed
vendor/paperclip-adapter-claude-k8s/— 36 source files in-tree.ARG CLAUDE_K8S_REF— the Dockerfilegit cloneis replaced by aCOPY.opencode_k8sstill clones, so thegh_tokenbuild secret stays.vendor_claude_k8sinpr.yml, running the adapter's own suite.PROVENANCE.md+LICENSE, including a sha256 integrity manifest over the 36 upstream files, enforced by CI so recorded provenance can't drift from the tree.The part most worth reviewing: this is not PR #31's tree
The deployed pin and the outstanding security fix had diverged from common ancestor
52649f8b:Neither is an ancestor of the other. Vendoring PR #31 as-is would have silently reverted a deployed fix. This vendors
3ad3370with35f1eb2+6ddd4b0cherry-picked on top; both applied without conflict, and the resulting tree differs from PR #31 by exactly the PR #29 delta.Verification
tsc --noEmitnpm testnpm run build+npm packdocker-opencode-runtime-pin)agent-dockerfile.test.tsincl. ordering assertions19adc325…scripts/__tests__/pr-verify-lane-outcome.test.mjsBoth Dockerfile test suites were verified by evaluating their assertions directly against the worktree — vitest's global setup needs a full plugin build this environment can't do. Flagging that method rather than implying I ran the runner.
The acceptance criterion. The new
vendor_claude_k8sjob runs the adapter's own suite, which contains the fail-closed guard rejecting any pod spec that carries a sensitive-named env var (/TOKEN|SECRET|PASSWORD|KEY|CREDENTIAL|AUTH/i) as a literalvalueinstead of asecretKeyRef. That guard had no CI anywhere in our estate until now — the package is deliberately outside the pnpm workspace and root tsconfig references, so nothing else compiles or exercises it.It is wired into the required
verifyaggregate in all three places it needs to be —needs, the*_RESULTenv, and thelane_resultsmap. Adding it toneedsalone would have made the gate decorative.Risks
package.json:farhoodlabs/paperclip-adapter-claude-k8s(named inpackage.json) 404s;kkroo/paperclip-adapter-claude-k8sisfork: false/parent: none— not a fork of anything; GitHub detects no license on it, and there is noLICENSEfile at any SHA. So the MIT grant rests entirely onpackage.json's"license": "MIT". TheLICENSEfile added here says exactly that in its own text and names no individual copyright holder, because none is identified anywhere in the source.PROVENANCE.mdflags this as the one loose end a human may want to close before external redistribution — it is not a blocker for the security fix.getSelfPodInfo()), and does not fix it incidentally either — the existing guard passes asecretKeyRefhappily, so converting values to secret references satisfies the guard while leaving the agent process able to read them. That needs an allowlist which drops server-only secrets. It is a behaviour change with real blast radius (drop the wrong key and agents break), so it is kept out of a 14k-line move to keep both reviewable. Vendoring first is what makes the two converge: after this there is one tree.COPYpath would surface as a build failure, not a silent wrong-version deploy. The pin tests above cover the ordering and path assertions.Model Used
claude-opus-5[1m] via Claude Code
🤖 Generated with Claude Code