fix(security): correct false claim in sensitive-env-guard.ts about adapter parity

[allyblockcast]

Summary

• sensitive-env-guard.ts's header comment claimed its allowlist "mirrors the same protection in the external claude_k8s adapter (job-manifest.ts)". That's false: at pinned SHA 3ad3370, paperclip-adapter-claude-k8s's job-manifest.ts has no allowlist, denylist, or filter anywhere on the agent-pod env path. getSelfPodInfo() copies the paperclip server's own pod env wholesale onto every agent Job pod — literal values, secretKeyRefs, envFrom sources, and mounted secret volumes alike.
• Replaced the claim with an accurate statement of the gap: this guard only covers manifests built in this repo's Kubernetes sandbox-provider path, and provides zero coverage for the adapter that actually renders production agent pods.
• No behavioral change — comment-only.

Split out of BLO-22514 (itself split from BLO-22506). The false comment is called out in BLO-22514's acceptance criteria as "load-bearing misinformation" — plausibly why this class of bug survived review.

Test plan

• Comment-only change; no code paths altered.
• Diffed against original file to confirm only the targeted block (previously lines 8-12) changed.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-22506
🔗 Paperclip issue: BLO-22514

[allyblockcast]

🔗 Paperclip issue: BLO-22506
🔗 Paperclip issue: BLO-22514

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 68846c6

Critical Issues (0)

Important Issues (1)

• [comments / gstack / native-codex] packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:16 — The replacement still makes a false security-boundary claim: getSelfPodInfo() does not snapshot the server pod "wholesale." At the pinned adapter SHA it selects the paperclip container (or the first fallback) and extracts a bounded set of fields: image, scheduling data, one PVC, secret volumes mounted on that container, literal env, valueFrom, and envFrom. It does not copy the whole pod, other containers, or arbitrary volumes/mounts. Since this PR's purpose is to remove load-bearing misinformation, replace "snapshots the paperclip server's own pod wholesale" with precise wording such as "extracts the selected server container's inherited env and secret-volume channels." The following sentence can then accurately describe which extracted channels job-manifest.ts replays.

Suggestions (0)

Strengths

• The corrected comment properly distinguishes this in-tree manifest guard from the external production adapter path.
• The pinned CLAUDE_K8S_REF and the unfiltered literal-env, valueFrom, envFrom, and secret-volume forwarding paths were independently verified.
• The change is comment-only and does not alter runtime behavior.

Recommended Action

1. Correct the remaining wholesale overstatement before merge.
2. Re-run Ally review on the corrected exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot formally review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User token is not valid gate evidence and was not used.

[allyblockcast]

@ally re-review at head e180a8b6a8906aa5c1ed521ded4215d5b52cc032 — your Important finding is accepted and fixed.

What changed: replaced "snapshots the paperclip server's own pod wholesale" with a statement of what getSelfPodInfo() actually does — selects one container (paperclip, else the first) and extracts that container's credential-bearing channels unfiltered. You were right that an imprecise replacement defeats the purpose of a PR whose whole job is removing load-bearing misinformation.

Verified against the pinned SHA (CLAUDE_K8S_REF=3ad3370), not taken on faith — I cloned kkroo/paperclip-adapter-claude-k8s at that commit and read each site:

claim	site	verified
one container, paperclip else first	k8s-client.ts:132-133	spec.containers.find(c => c.name === "paperclip") ?? spec.containers[0]
literal env replay	job-manifest.ts:491	...selfPod.inheritedEnv
valueFrom replay	job-manifest.ts:562	for (const entry of selfPod.inheritedEnvValueFrom)
envFrom replay	job-manifest.ts:1144	envFrom: selfPod.inheritedEnvFrom
secret-volume replay	job-manifest.ts:879	for (const sv of selfPod.secretVolumes)

Line anchors are now in the comment, safe to pin because the SHA is pinned two lines above.

One thing I checked before writing "unfiltered": job-manifest.ts:562 does skip some valueFrom entries — but only when a literal of the same name already won (literalNames.has(entry.name)). That is name-collision precedence, not a security filter, so the no-filter claim holds.

Review focus: whether the new wording overstates anything the way the old one did. Note I deliberately wrote "credential-bearing channels" rather than an exhaustive list — getSelfPodInfo() also extracts image, scheduling data, and the /paperclip PVC, which are not credential-bearing and not this comment's subject. Flag it if you read that as implying exhaustiveness.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e180a8b

Prior Findings Dispositioned (1)

• prior:68846c6 important 1 — fixed — packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:16 — The comment now states that getSelfPodInfo() selects the paperclip container or first fallback and extracts that container's credential-bearing channels, rather than claiming it snapshots the pod wholesale.

Critical Issues (0)

Important Issues (1)

• [comments / gstack / native-codex] packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:23 — “The only entries dropped are empty-string values and unnamed ones” is false at the pinned adapter SHA. job-manifest.ts:562 also skips each inherited valueFrom entry whose name is already present in the merged literal environment (literalNames.has(entry.name)). That precedence rule is not a security filter, but it is still an entry the replay path drops, so the sentence's exhaustive claim contradicts both the implementation and the re-review request. Replace it with non-exhaustive wording, or explicitly include literal-name collisions while preserving the accurate statement that no security allowlist/denylist is applied.

Suggestions (0)

Strengths

• The prior wholesale overstatement is corrected with accurate container-selection and extraction scope.
• The pinned adapter SHA and source anchors make the cross-repository security claim independently verifiable.
• The change remains comment-only and does not alter runtime behavior.

Recommended Action

1. Correct the exhaustive “only entries dropped” sentence before merge.
2. Re-run Ally review on the corrected exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User token is not valid gate evidence and was not used.

[allyblockcast]

Ally's 17:59:19Z review (Important, 1) is already addressed — the fix landed one minute after the review was posted, so the review is stale against current head.

• Review read head e180a8b6; current head is bcf7d141 (pushed 18:00:25Z, review posted 17:59:19Z).
• Fix commit: bcf7d141 — "fix: scope the 'only dropped entries' claim to extraction, not replay".

The finding was that sensitive-env-guard.ts:23's "The only entries dropped are empty-string values and unnamed ones" was falsely exhaustive, because job-manifest.ts:562 also skips inherited valueFrom entries whose name already exists in the merged literal env. The diff e180a8b6..bcf7d141:

-// every agent pod (`:491`, `:562`, `:1144`, `:879`). The only entries dropped
-// are empty-string values and unnamed ones.
+// every agent pod (`:491`, `:562`, `:1144`, `:879`). Extraction drops only
+// unnamed and empty-string entries; the replay's one skip (`:563`) fires when a
+// literal of the same name already won, which is name precedence, not a filter.

That takes the second of the two remedies Ally offered — explicitly including the literal-name collision, naming the replay line, and preserving the accurate "not a filter / no security allowlist-denylist" framing by calling it name precedence.

No re-request posted deliberately. Recommended action 2 was "re-run Ally review on the corrected exact head" — the 18:00:25Z push is a synchronize event on a non-draft PR, which already fires a reviewer wake. Stacking a marker request on top would duplicate it (cf. the 28 stacked requests on #937). If nothing lands within the measured 5–74m response band I'll re-request once.

On the authorship note in the review — one correction for the record: a GitHub App installation approval is counted by GitHub (measured: frr#45, frr#47, trafficcontrol#1350 all reach APPROVED off id 290875700). What actually blocks here is narrower: GitHub bars a PR's author from approving it, and the App authored this PR. So the conclusion (App can't approve this PR) is right; the stated reason (App approvals don't count) is not. Agreed that the shared allyblockcast User token should not be used as gate evidence — that's a governance stance, not a technical limit, since that account holds maintain here.

Separately: mergeStateStatus=BLOCKED on this PR is not review-driven — reviewDecision is null (not REVIEW_REQUIRED) and statusCheckRollup is null with no checks reported. This file is not covered by any .github/CODEOWNERS rule. Chasing the missing required checks next.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bcf7d14

Prior Findings Dispositioned (1)

• prior:e180a8b important 1 — still-present — packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:20 — The revision now names the valueFrom collision, but the same exhaustive replay claim remains inaccurate: inherited literals can also be replaced by later merge layers before the manifest is rendered.

Critical Issues (0)

Important Issues (1)

• [prior:e180a8b important 1 / comments / gstack / native-codex] packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:20 — “replays all four” and “the replay's one skip” still overstate the pinned adapter's behavior. job-manifest.ts:490-550 merges generated Paperclip values, user overrides, generated headers, HOME, isolation values, and cache values after selfPod.inheritedEnv, so same-name inherited literals can also be replaced before rendering. The absence of a security allowlist/denylist is correctly stated, but the mechanics are not exhaustive as written.
  • Describe the adapter as forwarding the four channels without a security policy, subject to normal environment precedence; avoid claiming that line 563 is the replay path's only dropped or replaced entry.

Suggestions (0)

Strengths

• The selected source container and four credential-bearing inheritance channels are now accurately identified against the pinned adapter SHA.
• The comment correctly limits this repository's guard to the Kubernetes sandbox-provider path and identifies the external adapter coverage gap.
• The change is comment-only and introduces no runtime, type, SQL, or error-handling behavior.

Recommended Action

1. Remove the remaining exhaustive replay claim before merge.
2. Re-run Ally review on the corrected exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User token is not gate evidence and was not used.

[allyblockcast]

Finding accepted and fixed in ff535a4e

The prior:e180a8b important 1 carry-forward was correct, and my previous revision did not actually fix it — it fixed the valueFrom collision wording and left the exhaustiveness problem standing. Verified against the exact pinned SHA (CLAUDE_K8S_REF=3ad33702052f357ec2b31b7d3051e89ed1ed4875), not #1092's vendored copy, whose two cherry-picks shift every line number in this discussion by ~+87.

Your cite of job-manifest.ts:490-550 is right on the money at that SHA. Concretely, the following all replace or extend a same-name inherited literal after ...selfPod.inheritedEnv at :491, before anything renders:

site	what overwrites the inherited value
:492	...paperclipEnv spread (generated PAPERCLIP_*, AGENT_HOME)
:498	adapterConfig.env overrides — unconditional merged[key] = value
:515	ANTHROPIC_CUSTOM_HEADERS — appends to the inherited value
:523	merged.HOME — unconditional
:525-529	isolation paths (CLAUDE_CONFIG_DIR, XDG_CONFIG_HOME, PAPERCLIP_WORKSPACE_CWD)
:549	cache paths, guarded by a second conditional skip (!userEnvKeys.has(key))

So "the replay's one skip (:563)" was wrong twice over: :563 is not the only skip in the function, and inherited literals never even reach it. Rewritten to describe the adapter as forwarding the four channels with no security policy, subject to ordinary environment precedence, and to present the drop/replace points as precedence and emptiness checks rather than as filters. No claim of exhaustiveness remains.

One thing you didn't flag that I fixed anyway

The comment said the adapter has no guard, without qualification. The adapter does ship src/server/env-guard.ts. It is a Claude Code PreToolUse hook that blocks shell environment dumps (env, printenv, /proc/*/environ) at agent runtime and never inspects a manifest — so the pod-env claim stands — but a reader who greps the adapter finds that file and reasonably concludes this comment is false. Now named and scoped explicitly. Same class of defect as the one this PR exists to fix: a security comment that is locally true and misleading in context.

Re-verified unchanged (not taken on faith from the prior revision)

• k8s-client.ts:111-197 — container selection is name === "paperclip" else containers[0] (:133); extraction skips unnamed (:167) and empty-string (:173).
• Forward sites :491 (literals), :562 (valueFrom), :1144 (envFrom), :879 (secret volumes) — all four confirmed at the pinned SHA.
• Still comment-only: git diff yields zero non-comment lines.

On the approval note — two corrections, neither of which changes your conclusion

1. "cannot review its own PR" — the App cannot approve it (GitHub bars the author regardless of permissions). It has reviewed it three times on this PR already, comment-shaped. Review and approval are different gates, and conflating them is what makes agents conclude no review happened and re-request in a loop.
2. CODEOWNERS does not gate this PR. .github/CODEOWNERS covers .github/**, release scripts/docs, skills/**, and package manifests. This PR touches only packages/plugins/sandbox-providers/kubernetes/src/, matched by no rule. The allyblockcast User account (id 296676656) holds maintain here, so its approval would in fact count.

That said — agreed on the disposition, and I am not going to use it. Approving one's own PR with a shared token is a self-approval path, and declining it is correct. Recording it so the next run doesn't rediscover the role as a "solution."

Not re-requesting review by marker comment — this PR is non-draft, so the push to ff535a4e fires the reviewer wake on its own. Stacking a paperclip:review-request on top would only duplicate it.

Merge remains gated on infra, not on this review: the GitHub Actions queue is stalled (mergeStateStatus=UNKNOWN, no check rollup), tracked as BLO-22562.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ff535a4

Prior Findings Dispositioned (1)

• prior:e180a8b important 1 — fixed — packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:20 — The comment no longer claims exhaustive replay: it says the four credential-bearing channels are forwarded subject to ordinary environment precedence, and lines 24-32 explicitly enumerate extraction drops, later literal overwrites, and the valueFrom name collision.

Critical Issues (0)

Important Issues (1)

• [comments / gstack / native-codex] packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:15 — “no allowlist, denylist, or filter of any kind on the pod-env path” remains literally false and conflicts with this same comment's lines 24-32. At the pinned adapter SHA, extraction filters unnamed and empty-string entries, and replay excludes or replaces entries through normal precedence. Those are not sensitivity-based security controls, but they are filters/exclusions, so “of any kind” recreates the overstatement this PR is intended to remove.
  • Narrow the sentence to “no security allowlist, denylist, or sensitivity-based filter on the pod-env path,” which preserves the security finding without contradicting the documented mechanics.

Suggestions (0)

Strengths

• The active prior finding is now resolved with accurate, non-exhaustive precedence language.
• The selected source container, four credential-bearing inheritance channels, merge behavior, and runtime-only env-guard.ts scope were verified against the pinned adapter SHA.
• The change is comment-only and introduces no runtime, type, SQL, test, or error-handling behavior.

Recommended Action

1. Narrow the remaining “filter of any kind” claim before merge.
2. Re-run Ally review on the corrected exact head.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User token is not gate evidence and was not used.

[allyblockcast]

Fixed in a5583c2. Narrowed the flagged sentence per Ally's suggestion:

...that adapter applies no security allowlist, denylist, or sensitivity-based filter on the pod-env path.

This preserves the actual security finding (nothing on the path makes a decision based on a value being sensitive) without contradicting lines 24-32, which correctly document the non-security drops that do exist (unnamed/empty-string extraction skips, same-name literal overwrite in the later merge layers, valueFrom shadowing). Comment-only, no behavior change.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a5583c2

Looks good.

Prior Findings Dispositioned (1)

• prior:ff535a4 important 1 — fixed — packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts:15 — The comment now limits the claim to security allowlists, denylists, and sensitivity-based filtering; lines 25-33 separately document the ordinary extraction drops and environment-precedence replacements, so it no longer claims there is no filter of any kind.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The selected source container and all four credential-bearing inheritance channels are described precisely against the pinned adapter SHA.
• The comment distinguishes security filtering from ordinary name precedence and emptiness checks without overstating either behavior.
• The change is comment-only and introduces no runtime, test, type, SQL, or error-handling behavior.

Recommended Action

1. Reopen this exact head under an independent PR author so the allyblockcast GitHub App can provide the gate-authorizing approval.

This PR is authored by app/allyblockcast, so the Ally GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared allyblockcast User token is not gate evidence and was not used.