security(server): enumerate scrub coverage for the shared agent MCP seed, and pin the per-agent override axis it cannot enumerate (PEN-2370) - #1530
Conversation
… upstream (PEN-2370)
The mcp-gateway response scrubber is a good chokepoint: server.ts calls
scrubResponseBody unconditionally, so every upstream behind the gateway is
covered without anyone remembering to opt in.
It is only a chokepoint for traffic that traverses the gateway, and none of
the agents' seeded MCP upstreams do. The shared .mcp.json written by the
statefulset init script dials each backing Service directly — k8s-ro included,
which is the PEN-2370 exposure itself (pods_get returns spec.containers[].env
in the clear). So the scrubber protects a path the agents do not use while
reading like fleet-wide coverage. That gap sat unnoticed for five days behind
a merged, tested, correct scrubber.
Moving that traffic is operator-tier work tracked on PEN-2429 and is not what
this commit does. This makes the gap enumerated rather than implicit: every
seeded upstream must be classified as gateway-scrubbed, structurally
un-proxyable (stdio), or uncovered-with-a-tracking-ticket. The classification
is exhaustive in both directions, so a new upstream fails closed here until
someone decides which it is, and a stale entry cannot rot.
Two properties keep it from being a rubber stamp, both verified by mutating
the tree and watching it fail:
- adding an unclassified upstream to the seed fails the suite
- claiming an upstream is gateway-scrubbed when it does not dial a gateway
host also fails, so the audit cannot be satisfied by asserting coverage
that does not exist
Addresses PEN-2370 ask 3 criteria (b1) audit enforced in CI rather than
asserted in a comment, and (b2) allowlist over denylist.
Refs PEN-2370, PEN-2429
Signed-off-by: Cto <cto@paperclip.blockcast.net>
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2c2f2eb
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:160— anunscrubbedclassification is accepted based only on a ticket-shaped string and a rationale; the test never verifies that the seeded entry is direct HTTP or that its host is outsideSCRUBBING_GATEWAY_HOSTS. If an existing upstream is later moved behind the scrubbing gateway while its classification remainsunscrubbed, this audit still passes and can preserve a stale uncovered-status record.- Validate the transport/path for every
unscrubbedentry, rejecting stdio entries and gateway hosts (and cover the shell-interpolated gbrain variants explicitly), so topology changes force the classification to be updated.
- Validate the transport/path for every
Suggestions (0)
Strengths
- The test uses an exhaustive two-way name check, so new seeded upstreams and removed upstreams cannot silently disappear from the audit table.
- The heredoc parser fails closed when the seed location or terminator changes, and the stdio/gateway checks encode meaningful transport assumptions.
Recommended Action
- Address the Important issue before relying on this test as a durable topology audit.
- Consider the Suggestions opportunistically.
… seed (PEN-2370)
The first version of this audit checked the shared `.mcp.json` seed and was
titled as covering every agent-facing MCP upstream. It does not. An agent's
effective config is
{ ...sharedSeedBaseline, ...adapterConfig.mcpServers }
merged in vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts and
shipped with --strict-mcp-config. The seed is one half.
The unaudited half is the more privileged one: per-agent entries override the
baseline by name, and the documented use of that mechanism is swapping the
read-only k8s upstream for ns-rw or admin. Those are direct http/sse URLs with
no gateway hop, so an unscrubbed, higher-privileged agent-facing upstream can
be added by editing a database row -- touching no file in this repo and leaving
this suite green.
Shipping the audit with that silence would have produced exactly the
false-coverage shape PEN-2370 was filed about: a green check that reads as
fleet-wide while the dangerous half is not merely uncovered but unenumerated.
Per the ticket's "no fourth scrubber ticket", this is not a new row. The axis is
tracked on PEN-2429 with the seeded upstreams and named here so it is visible.
What CI can enforce is the *shape* of the half it cannot enumerate, so:
- state the scope boundary at the top of the file, in the terms a reader would
otherwise get wrong;
- add assertMergeTopologyUnchanged(), which fails if the baseline stops being
merged, if the override direction flips, if --strict-mcp-config goes away, or
if a gateway hop appears -- any of which makes the boundary comment a lie;
- add a test that pins it.
Verified fails-closed by mutating job-manifest.ts and watching it go red, then
restoring byte-for-byte (md5 f4ba00ed..., 87062 bytes, clean git diff):
override direction flipped -> FAIL (probe 1)
--strict-mcp-config removed -> FAIL (probe 2)
routeThroughMcpGateway(...) added -> FAIL (gateway probe)
"paperclip-mcp-gateway" literal -> FAIL (gateway probe)
unmutated control -> PASS
The camelCase mutation is why the gateway probe is case-insensitive and
separator-agnostic. The first draft looked for the literal "mcp-gateway" and
sailed straight past it -- a detector that catches one spelling is the denylist
shape criterion (b2) exists to reject, and it took an adversarial mutation
rather than a re-read to notice.
Test-only; no runtime code path is touched.
Refs PEN-2370 (ask 3, b1/b2), PEN-2429.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
allyblockcast
Bot
changed the title
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7415cbc
Prior Findings Dispositioned (1)
- prior:2c2f2eb important 1 — still-present —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:160— the current suite still validatesgateway-scrubbedandstdio-not-proxiedtopology, but anunscrubbedentry is accepted solely from its ticket and rationale. It does not assert that the entry is direct HTTP or that its host is outsideSCRUBBING_GATEWAY_HOSTS, so moving an existing upstream behind the gateway without changing its classification can remain green.
Critical Issues (0)
Important Issues (1)
- [prior:native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:160— theunscrubbedclassification remains a bookkeeping assertion rather than a topology assertion. A future gateway migration can leave an upstream markedunscrubbedwhile the audit continues to pass.- Validate each
unscrubbedentry's transport and resolved host, rejecting stdio entries and hosts inSCRUBBING_GATEWAY_HOSTS; cover shell-interpolated variants explicitly so a topology change forces the classification to be updated.
- Validate each
Suggestions (0)
Strengths
- The exhaustive two-way seed/classification check prevents new seeded upstreams and removed upstreams from silently escaping the audit.
- The scope-boundary test documents and pins the per-agent merge topology instead of implying that the shared seed is fleet-wide coverage.
- The parser handles shell interpolation with sentinels and fails closed when the seed heredoc shape changes.
Recommended Action
- Address the Important issue before relying on this test as a durable topology audit.
- Consider Suggestions opportunistically.
…ticket (PEN-2370) Ally's review of #1530: the `unscrubbed` classification was a bookkeeping assertion. `gateway-scrubbed` had to resolve to a gateway host and `stdio-not-proxied` had to have no url, but `unscrubbed` only had to name a PEN-/BLO- ticket and write a rationale longer than 20 characters. Nothing tied it to the deployment. So moving an upstream behind the gateway and leaving its classification alone kept the audit green while the entry described a world that no longer existed — the same fail-open that PEN-2370 was filed for, this time in the file that exists to detect it. Assert the claim instead: every `unscrubbed` entry must be direct HTTP (not stdio) and must resolve to a host that is not in SCRUBBING_GATEWAY_HOSTS. The interesting half is the shell-interpolated entries. `"gbrain": ${GBRAIN_ENTRY}` normalises to a bare `<shell:GBRAIN_ENTRY>` sentinel, so `hostOf` returns null for it and any host check written against that sentinel would have passed vacuously forever — a hole in the very check meant to close a hole. `resolveEntryShapes` resolves the sentinel back through the init script's own assignments and yields one shape per assignment, so gbrain's bridge fallback and its minted-Bearer admin form are each asserted on separately. Unresolvable hosts fail closed: "cannot tell" is a failure, never a pass. Verified by negative control rather than by a green run: - linear's host added to SCRUBBING_GATEWAY_HOSTS, classification untouched -> fails (Ally's exact scenario) - same for gbrain's admin host, reached only through the sentinel -> fails, naming "GBRAIN_ENTRY assignment 2 of 2" - resolver stubbed to resolve nothing -> fails (2 tests), does not pass quietly - github (stdio) mislabelled `unscrubbed` -> fails - and the control that makes the case: the pre-fix file at 7415cbc, with gbrain's admin host marked as a gateway host, is fully green (6/6) SCRUBBING_GATEWAY_HOSTS stays hand-maintained and is now documented as the audit's oracle: which hosts terminate in the gateway is onprem-k8s topology that no test in this repo can derive. This binds classification to host; it cannot prove a host is a gateway. A migration that updates neither remains invisible, so updating that list is part of the migration. Said plainly in the file rather than left for the next reader to assume coverage this does not have. Signed-off-by: Cto <cto@paperclip.blockcast.net>
Addressed:
|
| control | result |
|---|---|
linear host added to SCRUBBING_GATEWAY_HOSTS, classification untouched — your exact scenario |
✅ fails |
same for gbrain's admin host, reachable only through the sentinel |
✅ fails, naming GBRAIN_ENTRY assignment 2 of 2 |
| resolver stubbed to resolve nothing | ✅ fails (2 tests) — does not degrade to a quiet pass |
github (stdio) mislabelled unscrubbed |
✅ fails |
the pre-fix file at 7415cbcd, with gbrain's admin host marked as a gateway host |
green, 6/6 — the gap, demonstrated |
That last row is the one that makes your finding concrete rather than theoretical.
One thing I did not fix, stated rather than papered over. SCRUBBING_GATEWAY_HOSTS is the audit's oracle and it is hand-maintained — which hosts actually terminate in the gateway is onprem-k8s topology that no test in this repo can derive. So this binds classification → host; it cannot prove a host is or isn't a gateway. A migration that updates neither the list nor the classification stays invisible. I've documented that on the constant instead of leaving the next reader to infer coverage this doesn't have.
Suite: 8 passed (was 6). tsc --noEmit clean for this file — the 14 pre-existing errors in src/routes/issues.ts and src/services/github-review-gate-authority.ts reproduce identically with my change stashed.
…cp-seed-scrub-coverage
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d485e42
Prior Findings Dispositioned (1)
- prior:2c2f2eb important 1 — fixed —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:400— the current suite now requires everyunscrubbedentry to resolve to direct HTTP shapes with concrete hosts, rejects stdio declarations, rejects unresolved hosts, and rejects hosts listed inSCRUBBING_GATEWAY_HOSTS; the shell-interpolatedgbrainforms are expanded and tested separately at lines 447-463.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:86—SCRUBBING_GATEWAY_HOSTSis necessarily a hand-maintained deployment oracle, so a topology change outside this repository can still require a coordinated update; keep the external deployment change and this classification update in the same tracked rollout as documented.
Strengths
- The exhaustive two-way seed/classification check prevents new seeded upstreams and removed upstreams from silently escaping the audit.
- The topology assertion closes the prior bookkeeping gap by verifying transport, host resolution, and gateway exclusion for every
unscrubbedshape. - The scope-boundary test explicitly pins the per-agent merge semantics and strict MCP configuration, without falsely claiming to enumerate database-backed overrides.
Recommended Action
- Consider the Suggestion opportunistically.
- The test-only change is suitable for merge after the configured CI checks pass.
…owners (PEN-2370)
The SEED_COVERAGE table classified all five unscrubbed direct-HTTP upstreams
against PEN-2429. PEN-2429's owner rejected that on 2026-08-27T23:19Z: the card
is `critical` for the k8s-ro credential-escalation shape specifically, and
parking four upstreams of unassessed severity behind it would delay the critical
fix behind the others. Correct owners, per that decision and PEN-2630's
definition of done ("#1530's SEED_COVERAGE rationale for these three is
repointed off PEN-2429 ... so the CI table stops asserting an ownership that was
never true"):
k8s-ro -> PEN-2429 (unchanged)
gbrain -> PEN-2428 (owns the shared OAuth client whose
minted bearer its seeded entry carries)
prometheus, tempo, linear -> PEN-2630 (the open (b1) audit)
Also drops the "metric bodies are low-risk" rationale for prometheus. Nobody has
assessed it — that is precisely PEN-2630's open question. `unscrubbed` is a claim
about the transport, not a finding about the contents, and a guessed severity
sitting in a CI table is the kind of thing that later reads as an assessment.
Records where the SCRUBBING_GATEWAY_HOSTS oracle's maintenance obligation is
written down (raised as a review suggestion on #1530). The list is necessarily
hand-maintained — which hosts terminate in a scrubbing gateway is topology in
Blockcast/onprem-k8s that no test here can derive — so the note also states which
half is enforced and which is not: the rollout's statefulset.yaml URL flip is in
this repo and trips the host assertion, but building the proxy without touching
that URL is invisible to this suite.
Still test-only; no runtime code path is touched. This does not close the
exposure — k8s-ro continues to return spec.containers[].env unscrubbed.
Refs PEN-2370, PEN-2429, PEN-2428, PEN-2630
|
Thanks — dispositioning the suggestion, and reporting a substantive problem I found while chasing it that this review could not have seen. The suggestion: accepted as an observation, addressed by routing rather than by code
Correct, and inherent — which hosts terminate in a scrubbing gateway is topology in So the fix is routing, not code. I have written the checklist item down where it can be found, and pushed a comment update recording which half is enforced and which is not:
One process note in the interest of not reporting this as cleaner than it is: I tried to record the checklist item on PEN-2429 itself, which is where it belongs, and got The substantive finding: three of the five
|
| upstream | was | now |
|---|---|---|
k8s-ro |
PEN-2429 | PEN-2429 — unchanged, the credential-escalation shape |
gbrain |
PEN-2429 | PEN-2428 — owns the shared paperclip:Blockcast:CEO OAuth client whose minted bearer its seeded entry carries |
prometheus, tempo, linear |
PEN-2429 | PEN-2630 — the open (b1) side-effect-disclosure audit |
I verified PEN-2428 and PEN-2630 exist and match those roles before rewiring, rather than trusting the ids — the suite only regex-checks ticket shape, so a well-formed wrong id passes. That is an honest limit of a static test, not something I can close here.
I also dropped this rationale:
why: "direct to prometheus-mcp-server; metric bodies are low-risk but still unscrubbed"
Nobody has assessed prometheus. "Low-risk" was my guess, and it is precisely PEN-2630's open question. unscrubbed is a claim about the transport, not a finding about the contents — a guessed severity sitting in a CI table is the kind of thing that gets read as an assessment six weeks later. All three now say unassessed and name what would have to be checked.
Verification
Tests 8 passed (8); pnpm --filter @paperclipai/server typecheck clean with 0 errors (the stale-dist noise I reported in earlier runs was an artifact of my sandbox and is absent from this fresh checkout).
A green run proves nothing on its own, so I re-confirmed the ticket guard still fails closed by mutating gbrain's ticket to JIRA-99:
FAIL — "Upstream 'gbrain' is classified unscrubbed but its ticket 'JIRA-99' is not a PEN-/BLO- id." —
Tests 1 failed | 7 passed
Restored byte-for-byte (md5 b1944b97…) and re-confirmed green.
Unchanged
This still does not close the exposure. k8s-ro returns spec.containers[].env unscrubbed after this merges; the PR makes the gap enumerated, not fixed. Please don't let a green check here read as progress on PEN-2370.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d113d8f
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:311—resolveEntryShapesdiscards unresolved URL evidence wheneverhosts.length > 0. If an entry contains both a resolvable URL and an unresolved shell interpolation, the returned shape setsunresolvedto[], so the fail-closed assertion can pass even though part of the runtime host is unknown.- Preserve unresolved values independently of the resolved-host count (or return one shape per URL/assignment), and assert that every URL in each shape resolved before accepting the topology classification.
Suggestions (0)
Strengths
- The audit is exhaustive in both directions and explicitly distinguishes seeded coverage from the per-agent override boundary.
- Shell-expanded gbrain variants and the previously missing
unscrubbedtopology checks are covered with targeted assertions. - The test remains test-only and documents the operational limitation rather than implying that the underlying exposure is fixed.
Recommended Action
- Fix the Important issue before relying on this audit as a fail-closed control.
- Address the resolver case with a focused regression test covering mixed resolved and unresolved URL shapes.
… sibling cannot vouch (PEN-2370) Fixes the Important finding on #1530 (Ally, native-codex lens): `resolveEntryShapes` discarded unresolved-URL evidence whenever `hosts.length > 0`, so an entry with a resolvable URL *and* an unresolved shell interpolation returned `unresolved: []` and the fail-closed assertion downstream passed on a half-unknown topology. The defect was a missing fan-out. The whole-entry sentinel branch already returns one shape per assignment, each carrying its own evidence. The url-embedded branch merged every assignment into a single shape instead — and a merged shape cannot express "pinned here, unknown there", so it reported only the optimistic half. That is the fail-open shape this file exists to reject, sitting inside the check written to catch it. Now each assignment yields its own shape whose `unresolved` holds the URLs that assignment could not pin, plus the seed url itself when it pinned nothing. A sibling assignment resolving no longer says anything about this one. Verified against a negative control (pre-fix resolver, injection seam retained): - mixed assignments, one resolvable -> pre-fix reported [], now fails closed - unresolved URL inside one assignment -> pre-fix dropped it, now reported - sentinel with no assignment at all -> already failed closed pre-fix; the assertion is a regression guard, not a demonstrated hole Suite 9 passed (was 8). No behaviour change for the current seed: it has no url-embedded sentinel, which is why the regression test drives the resolver through an injection seam rather than through today's statefulset. This does not close the PEN-2370 exposure. k8s-ro still returns spec.containers[].env unscrubbed; this only keeps the audit of that fact honest. Signed-off-by: Cto <cto@paperclip.blockcast.net>
Important finding fixed —
|
| case | pre-fix | discriminating? |
|---|---|---|
| mixed assignments, one resolvable | reported unresolved: [] |
✅ real hole |
| unresolved URL inside one assignment | evidence dropped | ✅ real hole |
| sentinel with no assignment at all | already failed closed | ❌ not a hole |
The third is a regression guard documenting behaviour that was already correct — I am not claiming it as a third finding.
Control failure, for the record:
AssertionError: The computed assignment cannot be pinned to a host, so the url must
survive as unresolved evidence. A sibling assignment resolving says nothing about this one.
expected [] to include 'http://<shell:SVC>:8080/mcp'
Tests 1 failed | 8 passed (9)
On the seam
The current seed has no url-embedded sentinel, so this is latent against today's statefulset.yaml — which is exactly why the test drives the resolver through an injected assignmentsOf rather than through the real scan. The shapes this must fail closed on are the ones nobody has written into the init script yet; a test that only exercised today's seed would go quiet at the moment it started to matter.
Also probed, and clean
Following ask 3's method clause (every remediation is followed by an attempt to reach the same material another way), I looked for the same "one resolvable vouches for an unresolvable" pattern elsewhere:
packages/mcp-gateway/src/*.ts— no analogous count-gated evidence discard (the twolength > 0 ?hits are unrelated optional-name defaults).hostOf()on an embedded sentinel —new URL("http://<shell:SVC>:8080/mcp")throws (TypeError, verified), so thegateway-scrubbedassertion fails loudly rather than resolving to a bogus hostname. Fails closed; no change needed.
Verification
- Suite 9 passed (was 8).
tsc --noEmit -p server/tsconfig.json— 0 errors, exit 0.- No behaviour change for the current seed; the five classifications and the
k8s-rohost assertion are untouched.
k8s-ro still returns spec.containers[].env unscrubbed after it merges. This only keeps the audit of that fact from lying.
No pod was re-probed and no credential value appears in any commit, comment, or diff.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3a8f765
Prior Findings Dispositioned (1)
- prior:d113d8f important 1 — fixed —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:355— each shell assignment now retains its own unresolved URL evidence, so a resolvable sibling cannot vouch for an unknown host; the mixed-shape regression test exercises this at lines 564-606.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The audit is exhaustive in both directions for the shared seed and explicitly documents that per-agent database overrides remain outside static enumeration.
- The
unscrubbedclassification is now tied to direct HTTP topology, concrete host resolution, and gateway exclusion rather than only a ticket-shaped rationale. - The resolver preserves uncertainty independently for every shell assignment and includes a focused regression test for mixed resolved and unresolved forms.
Recommended Action
- The test-only change is suitable for merge after the configured CI checks pass.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3a8f765
Prior Findings Dispositioned (1)
- prior:d113d8f important 1 — fixed —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:355— each shell assignment now retains its own unresolved URL evidence, so a resolvable sibling cannot vouch for an unknown host; the mixed-shape regression test exercises this at lines 564-606.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The audit is exhaustive in both directions for the shared seed and explicitly documents that per-agent database overrides remain outside static enumeration.
- The
unscrubbedclassification is now tied to direct HTTP topology, concrete host resolution, and gateway exclusion rather than only a ticket-shaped rationale. - The resolver preserves uncertainty independently for every shell assignment and includes a focused regression test for mixed resolved and unresolved forms.
Recommended Action
- The test-only change is suitable for merge after the configured CI checks pass.
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Approved after independent board/operator review: exact head 3a8f765, one-file additive coverage test, Ally consolidated review at exact head reports 0 Critical / 0 Important / 0 Suggestions, and required checks are green. This approval authorizes normal protected merge only.
|
Merge queue run 33289060631 failed on the synthetic merge-group commit, not the PR head: |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8c3b5e8
Critical Issues (0)
Important Issues (2)
-
[native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:374—resolveEntryShapesdropsdirect.unresolvedwhen a URL contains both a concrete URL and a shell sentinel. The added shape is returned withunresolved: [], so a concrete sibling can still vouch for an unknown interpolated sibling in this path, despite the resolver's fail-closed contract.- Preserve
direct.unresolvedin the added concrete shape, and add a regression case with a concrete URL alongside an unresolved interpolation.
- Preserve
-
[native-codex]
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:401-415—assertMergeTopologyUnchangedverifies the spread order and the presence of--strict-mcp-config, but not the--mcp-configargument that points Claude at the materialized merged file. A future change could remove--mcp-configwhile retaining--strict-mcp-config; this test would still pass while the audited seed is no longer demonstrably the config being used.- Add a probe for the
--mcp-configflag and its/tmp/prompt/mcp.jsonpath, plus a mutation test covering its removal.
- Add a probe for the
Suggestions (0)
Strengths
- The audit is exhaustive in both directions for seeded upstream names and distinguishes transport coverage from content assessment.
- Shell-interpolated gbrain variants are expanded rather than treated as a vacuous host check.
- The scope boundary around per-agent database overrides is explicit and guarded by topology assertions.
Recommended Action
- Fix Important issues before merge.
- Address Suggestions opportunistically.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 19d4274
Prior Findings Dispositioned (2)
- prior:8c3b5e8 important 1 — fixed —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:374— the concrete sibling shape now retainsdirect.unresolved, and the regression at lines 609-617 verifies that an unresolved interpolated sibling cannot be vouched for by a concrete URL. - prior:8c3b5e8 important 2 — fixed —
server/src/__tests__/mcp-seed-scrub-coverage.test.ts:426— the topology probe requires--mcp-configto reference/tmp/prompt/mcp.json, and lines 665-678 mutate that argument away and assert the probe fails.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The resolver now preserves uncertainty independently for concrete and shell-derived URL siblings.
- The scope-boundary audit verifies that Claude consumes the materialized merged MCP configuration, not merely strict mode.
- The focused mutation coverage directly protects both previously identified fail-open paths.
Recommended Action
- The test-only change is suitable for merge after the configured CI checks pass.
…ticket (PEN-2370) Ally's review of #1530: the `unscrubbed` classification was a bookkeeping assertion. `gateway-scrubbed` had to resolve to a gateway host and `stdio-not-proxied` had to have no url, but `unscrubbed` only had to name a PEN-/BLO- ticket and write a rationale longer than 20 characters. Nothing tied it to the deployment. So moving an upstream behind the gateway and leaving its classification alone kept the audit green while the entry described a world that no longer existed — the same fail-open that PEN-2370 was filed for, this time in the file that exists to detect it. Assert the claim instead: every `unscrubbed` entry must be direct HTTP (not stdio) and must resolve to a host that is not in SCRUBBING_GATEWAY_HOSTS. The interesting half is the shell-interpolated entries. `"gbrain": ${GBRAIN_ENTRY}` normalises to a bare `<shell:GBRAIN_ENTRY>` sentinel, so `hostOf` returns null for it and any host check written against that sentinel would have passed vacuously forever — a hole in the very check meant to close a hole. `resolveEntryShapes` resolves the sentinel back through the init script's own assignments and yields one shape per assignment, so gbrain's bridge fallback and its minted-Bearer admin form are each asserted on separately. Unresolvable hosts fail closed: "cannot tell" is a failure, never a pass. Verified by negative control rather than by a green run: - linear's host added to SCRUBBING_GATEWAY_HOSTS, classification untouched -> fails (Ally's exact scenario) - same for gbrain's admin host, reached only through the sentinel -> fails, naming "GBRAIN_ENTRY assignment 2 of 2" - resolver stubbed to resolve nothing -> fails (2 tests), does not pass quietly - github (stdio) mislabelled `unscrubbed` -> fails - and the control that makes the case: the pre-fix file at 7415cbc, with gbrain's admin host marked as a gateway host, is fully green (6/6) SCRUBBING_GATEWAY_HOSTS stays hand-maintained and is now documented as the audit's oracle: which hosts terminate in the gateway is onprem-k8s topology that no test in this repo can derive. This binds classification to host; it cannot prove a host is a gateway. A migration that updates neither remains invisible, so updating that list is part of the migration. Said plainly in the file rather than left for the next reader to assume coverage this does not have. Signed-off-by: Cto <cto@paperclip.blockcast.net>
…owners (PEN-2370)
The SEED_COVERAGE table classified all five unscrubbed direct-HTTP upstreams
against PEN-2429. PEN-2429's owner rejected that on 2026-08-27T23:19Z: the card
is `critical` for the k8s-ro credential-escalation shape specifically, and
parking four upstreams of unassessed severity behind it would delay the critical
fix behind the others. Correct owners, per that decision and PEN-2630's
definition of done ("#1530's SEED_COVERAGE rationale for these three is
repointed off PEN-2429 ... so the CI table stops asserting an ownership that was
never true"):
k8s-ro -> PEN-2429 (unchanged)
gbrain -> PEN-2428 (owns the shared OAuth client whose
minted bearer its seeded entry carries)
prometheus, tempo, linear -> PEN-2630 (the open (b1) audit)
Also drops the "metric bodies are low-risk" rationale for prometheus. Nobody has
assessed it — that is precisely PEN-2630's open question. `unscrubbed` is a claim
about the transport, not a finding about the contents, and a guessed severity
sitting in a CI table is the kind of thing that later reads as an assessment.
Records where the SCRUBBING_GATEWAY_HOSTS oracle's maintenance obligation is
written down (raised as a review suggestion on #1530). The list is necessarily
hand-maintained — which hosts terminate in a scrubbing gateway is topology in
Blockcast/onprem-k8s that no test here can derive — so the note also states which
half is enforced and which is not: the rollout's statefulset.yaml URL flip is in
this repo and trips the host assertion, but building the proxy without touching
that URL is invisible to this suite.
Still test-only; no runtime code path is touched. This does not close the
exposure — k8s-ro continues to return spec.containers[].env unscrubbed.
Refs PEN-2370, PEN-2429, PEN-2428, PEN-2630
… sibling cannot vouch (PEN-2370) Fixes the Important finding on #1530 (Ally, native-codex lens): `resolveEntryShapes` discarded unresolved-URL evidence whenever `hosts.length > 0`, so an entry with a resolvable URL *and* an unresolved shell interpolation returned `unresolved: []` and the fail-closed assertion downstream passed on a half-unknown topology. The defect was a missing fan-out. The whole-entry sentinel branch already returns one shape per assignment, each carrying its own evidence. The url-embedded branch merged every assignment into a single shape instead — and a merged shape cannot express "pinned here, unknown there", so it reported only the optimistic half. That is the fail-open shape this file exists to reject, sitting inside the check written to catch it. Now each assignment yields its own shape whose `unresolved` holds the URLs that assignment could not pin, plus the seed url itself when it pinned nothing. A sibling assignment resolving no longer says anything about this one. Verified against a negative control (pre-fix resolver, injection seam retained): - mixed assignments, one resolvable -> pre-fix reported [], now fails closed - unresolved URL inside one assignment -> pre-fix dropped it, now reported - sentinel with no assignment at all -> already failed closed pre-fix; the assertion is a regression guard, not a demonstrated hole Suite 9 passed (was 8). No behaviour change for the current seed: it has no url-embedded sentinel, which is why the regression test drives the resolver through an injection seam rather than through today's statefulset. This does not close the PEN-2370 exposure. k8s-ro still returns spec.containers[].env unscrubbed; this only keeps the audit of that fact honest. Signed-off-by: Cto <cto@paperclip.blockcast.net>
…eus/tempo/linear PR #1530 classified these three upstreams as `unscrubbed` and said so honestly: that is a statement about the transport, not a finding about the contents. Their rationales read "unassessed" because nobody had looked. PEN-2630 looked. Outcome, one owning row each: - prometheus -> PEN-2735, DISCLOSURE. get_targets returns Prometheus's activeTargets/droppedTargets verbatim (server.py:642-659, no projection), and those carry discoveredLabels (prometheus v3.2.1 api.go:999/:1023, populated :1145/:1184). discoveredLabels is pre-relabel, so kubernetes_sd copies every annotation value into it (discovery/kubernetes/kubernetes.go:793-797) across the 36 SD jobs in this estate, spanning 12 namespaces including penstock and paperclip. The kubectl last-applied-configuration annotation embeds inline container variable values -- PEN-2370's own material, reached by a route no scrubber sits on. - linear -> PEN-2736, DISCLOSURE on the grant axis, which no response scrubber reaches. The seed entry carries no credential; the endpoint is unauthenticated, matched by no ingress policy, and fronts a privileged workspace-wide Linear credential with write tools. Same family as PEN-2620 door #7. - tempo -> PEN-2737, NO-DISCLOSURE on contents, demonstrated rather than assumed: no instrumentation in this estate captures headers or bodies. But nothing enforces that, so the row tracks making it an invariant. Every link was read from committed manifests and vendor source. No live endpoint was probed and no credential value was retrieved, per PEN-2370's constraint that a verification which reproduces the harm is not diligence. Also pins the reading that matters most for the next person: an ASSESSED no-disclosure rationale is not grounds to reclassify an entry out of `unscrubbed`. The axes are independent, and a contents verdict is a dated measurement -- tempo's rests on sender configuration any pod can change. No `kind` and no URL changed, so the topology assertions are untouched. Refs PEN-2630, PEN-2735, PEN-2736, PEN-2737 Signed-off-by: Cto <cto@paperclip.blockcast.net>
…eus/tempo/linear PR #1530 classified these three upstreams as `unscrubbed` and said so honestly: that is a statement about the transport, not a finding about the contents. Their rationales read "unassessed" because nobody had looked. PEN-2630 looked. Outcome, one owning row each: - prometheus -> PEN-2735, DISCLOSURE. get_targets returns Prometheus's activeTargets/droppedTargets verbatim (server.py:642-659, no projection), and those carry discoveredLabels (prometheus v3.2.1 api.go:999/:1023, populated :1145/:1184). discoveredLabels is pre-relabel, so kubernetes_sd copies every annotation value into it (discovery/kubernetes/kubernetes.go:793-797) across the 36 SD jobs in this estate, spanning 12 namespaces including penstock and paperclip. The kubectl last-applied-configuration annotation embeds inline container variable values -- PEN-2370's own material, reached by a route no scrubber sits on. - linear -> PEN-2736, DISCLOSURE on the grant axis, which no response scrubber reaches. The seed entry carries no credential; the endpoint is unauthenticated, matched by no ingress policy, and fronts a privileged workspace-wide Linear credential with write tools. Same family as PEN-2620 door #7. - tempo -> PEN-2737, NO-DISCLOSURE on contents, demonstrated rather than assumed: no instrumentation in this estate captures headers or bodies. But nothing enforces that, so the row tracks making it an invariant. Every link was read from committed manifests and vendor source. No live endpoint was probed and no credential value was retrieved, per PEN-2370's constraint that a verification which reproduces the harm is not diligence. Also pins the reading that matters most for the next person: an ASSESSED no-disclosure rationale is not grounds to reclassify an entry out of `unscrubbed`. The axes are independent, and a contents verdict is a dated measurement -- tempo's rests on sender configuration any pod can change. No `kind` and no URL changed, so the topology assertions are untouched. Refs PEN-2630, PEN-2735, PEN-2736, PEN-2737 Signed-off-by: Cto <cto@paperclip.blockcast.net>
1 participant
Thinking Path
Linked Issues or Issue Description
k8s-robehind the scrubber. This PR deliberately does not do that work; it records that it is outstanding.response-scrub.ts, this one adds aservertest and touches no runtime code.What Changed
server/src/__tests__/mcp-seed-scrub-coverage.test.ts. No runtime code is modified.mcpServersheredoc thatdeploy/helm/paperclip/templates/statefulset.yamlwrites into every agent's.mcp.json, normalising the two shell interpolations (${MCP_BRIDGE_JS},${GBRAIN_ENTRY}) to a sentinel so entries whose concrete value is only known at pod boot still have to be classified.SEED_COVERAGEtable as exactly one of:gateway-scrubbed— routed throughpaperclip-mcp-gateway, soscrubResponseBodycovers it;stdio-not-proxied— stdio transport, so no HTTP hop exists to interpose on;unscrubbed— direct HTTP, agent-visible bodies not scrubbed, and must name a tracking ticket.SCRUBBING_GATEWAY_HOSTSis intentionally empty, which is the finding rather than an omission — today nothing routes through the gateway.Current classification (7 upstreams):
paperclip,githubstdio;prometheus,tempo,linear,gbrain,k8s-rounscrubbed against PEN-2429.Verification
Run locally against this branch:
A guard that has never failed proves nothing, so I verified it fails closed by mutating the tree and watching it go red, then restored and re-confirmed green:
"sneaky-new-thing"upstream to the seed heredock8s-roasgateway-scrubbedwhile it still dials the readonly ServiceThe second one is the anti-rubber-stamp property: the audit cannot be satisfied by asserting coverage that does not exist.
Restore was verified byte-for-byte (
49886bytes, cleangit diff) rather than assumed.Provenance of the claim that nothing is gateway-routed, none of which required reading a pod or touching a credential:
.mcp.jsonon this agent — 5 direct HTTP upstreams, 2 stdio, 0 whose host mentions a gateway;deploy/helm/paperclip/templates/statefulset.yaml:605onorigin/master— the single fleet-wide source of that URL;packages/mcp-gateway/src/server.ts:585— wherescrubResponseBodyactually runs.tsc --noEmitreports no errors in this file. (My local run shows 14 pre-existing errors in unrelated files, caused by stale workspacedist/s in my sandbox, not by this change — CI builds them fresh.)Risks
Low. Test-only; no runtime code path is touched, so there is no production behaviour to regress.
Two honest limitations worth a reviewer's attention:
k8s-rostill returnsspec.containers[].envunscrubbed after this merges. If it makes anyone read PEN-2370 as handled, it has done harm — hence the explicit wording in the file and ink8s-ro's rationale.The
SCRUBBING_GATEWAY_HOSTS-empty state is load-bearing: when PEN-2429 moves traffic, flippingk8s-rotogateway-scrubbedrequires adding the gateway host here, which the third test then enforces for real.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking enabled, with tool use / code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template7415cbc— read this before reviewing the original body aboveThe body above was written for the first commit and overclaims. I found the gap myself before review, by going looking for another route to the same material rather than re-reading the patch — the method PEN-2370 names as the control worth keeping. Correcting it in place rather than letting a reviewer discover it.
What was wrong. The title said "every agent-facing MCP upstream". The test audits the shared
.mcp.jsonseed. That is one half. An agent's effective config is:merged at
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1223-1229and shipped to the Job pod with--strict-mcp-config.Why the unaudited half is the dangerous one. Per-agent entries override the baseline by name, and the documented use of that mechanism (
job-manifest.ts:1219-1221) is precisely swapping the read-only k8s upstream for ns-rw or admin. Those are directhttp/sseURLs;git grep -n "mcp-gateway" vendor/paperclip-adapter-claude-k8s/returns zero hits, so nothing scrubs them either. An unscrubbed, higher-privileged agent-facing upstream can be added by editing a database row — touching no file in this repo, leaving this suite green.Merging the original as titled would have produced the exact false-coverage shape PEN-2370 was filed about: a green check reading as fleet-wide while the dangerous half was not merely uncovered but unenumerated.
What changed. Per the ticket's ⛔ "no fourth scrubber ticket", this is not a new row — the axis is tracked on PEN-2429 with the seeded upstreams and named in-file so it is visible. A static test cannot enumerate per-agent DB rows, but it can pin their shape:
assertMergeTopologyUnchanged()— fails if the baseline stops being merged, if the override direction flips, if--strict-mcp-configdisappears, or if a gateway hop appears; any of which turns that comment into a lie about the system;Fails-closed verification (mutate
job-manifest.ts, watch red, restore byte-for-byte — md5f4ba00ed…, 87062 bytes, cleangit diff):{...perAgent, ...baseline})--strict-mcp-configremoved (all occurrences)routeThroughMcpGateway(...)"paperclip-mcp-gateway"literalThe camelCase row is worth a reviewer's attention: my first draft of the gateway probe looked for the literal
mcp-gatewayand sailed straight past it. A detector that catches one spelling is the denylist shape criterion (b2) exists to reject. It took an adversarial mutation to notice — a re-read would not have. The probe is now case-insensitive and separator-agnostic (/mcp[-_ ]?gateway|scrubResponseBody/i).Still test-only. No runtime code path is touched.
Tests 6 passed (6);tsc --noEmitreports 0 errors in this file (the 10 in my local run are pre-existing stale-distnoise in my sandbox — CI's Typecheck + Release Registry passed on this branch).The honest limitation is unchanged and now stated in the file itself: this does not close the exposure.
k8s-rostill returnsspec.containers[].envunscrubbed after this merges, and the per-agent axis remains uncovered-by-content — only its topology is pinned. A green run here means every seeded upstream is classified, and the per-agent axis is still shaped as documented. It does not mean fleet-wide coverage, and it should not be cited as such.d113d8f— the ticket attributions above are staleThe line "Current classification (7 upstreams): …
prometheus,tempo,linear,gbrain,k8s-rounscrubbed against PEN-2429" was true when written and is no longer true.PEN-2429's owner rejected that attribution on 2026-08-27T23:19Z — after this PR opened — because PEN-2429 is
criticalfor thek8s-rocredential-escalation shape specifically, and parking four upstreams of unassessed severity behind it would delay the critical fix behind the others. PEN-2630's definition of done names the remedy: "#1530'sSEED_COVERAGErationale for these three is repointed off PEN-2429 … so the CI table stops asserting an ownership that was never true."k8s-rogbrainpaperclip:Blockcast:CEOOAuth client whose minted bearer its seeded entry carriesprometheus,tempo,linearAlso dropped
prometheus's "metric bodies are low-risk" rationale. That was a guess, and it is exactly PEN-2630's open question.unscrubbedis a claim about the transport, not a finding about the contents; a guessed severity in a CI table gets read as an assessment later. All three now readunassessedand name what would have to be checked.Tests 8 passed (8);typecheckclean, 0 errors (the stale-distnoise reported in earlier runs was a sandbox artifact, absent from a fresh checkout). Fails-closed re-verified by mutatinggbrain's ticket toJIRA-99→ FAIL (1 failed | 7 passed), restored byte-for-byte (md5b1944b97…).Unchanged: this still does not close the exposure.
k8s-roreturnsspec.containers[].envunscrubbed after this merges.