test(mcp): pin the line that applies the sandbox label, which nothing held #456

Merged

coilyco-ops merged 1 commit from qa/pin-the-sandbox-label-wiring into main

2026-08-13 13:53:20 +00:00

coilyco-ops commented

2026-08-13 13:52:36 +00:00

Member

abdd15c labels every issue the harness files, so attacker-influenceable content is distinguishable in a tracker four agents read and act on. The policy has five tests. The line that invokes it had none.

mutation: delete the two-line block in mcpToolSession.Call
result:   go test ./... — entire suite green

A refactor of the dispatch path drops a security control without a single failure. That is the #291 and #328 shape — a check that exists, reports success, and has stopped covering — arriving on a control rather than on a check.

It asserts ordering, not presence

Presence is the weaker property. The commit chose the harness layer specifically so there is no window:

the label goes into the create-issue call rather than a second call afterwards, and there is no window where the issue exists unlabelled

So the test checks that the label is applied before tool.session.CallTool, which is the invariant that reasoning bought.

Mutation	Result
delete the call	fails, naming the consequence — every filed issue lands unlabelled
move it after the dispatch	fails on ordering
a second `CallTool` appears	fails, because this guards the first

The third matters because a new dispatch path added beside this one would otherwise pass while filing unlabelled issues.

Why a source check, having criticised one earlier today

tool.session is a concrete *mcp.ClientSession, so reaching Call needs a live MCP server. The repository already uses this shape for the same reason — TestTheEvaluationRunnerKeepsLogsOffStdout reads main.go because "the alternative is running the binary".

I flagged the weakness of source checks on #407, where a format-string count missed a duplicate spelled differently. That weakness is real here too, and it is why this one asserts a relationship between two anchors rather than the presence of one string: a rename of either side fails loudly rather than passing vacuously, and the CallTool count catches a second path the first anchor would miss.

The durable version needs session behind an interface so Call can be driven with a fake. That is production code and not mine; if it lands I will replace this with the behavioural test and delete the source check.

What I verified about the control itself

Sound on the adversarial questions I asked of it. withSandboxLabel sets rather than appends, so a model-supplied labels value is overwritten rather than merged — the comment says why, and it is the bypass I went looking for first. toolName is the bare name and the map key is qualified, so the create_issue comparison matches rather than silently never firing. A zero label ID disables the control rather than mislabelling.

Test-only. No production change.

Refs #208

`abdd15c` labels every issue the harness files, so attacker-influenceable content is distinguishable in a tracker four agents read and act on. **The policy has five tests. The line that invokes it had none.** ``` mutation: delete the two-line block in mcpToolSession.Call result: go test ./... — entire suite green ``` A refactor of the dispatch path drops a security control without a single failure. That is the #291 and #328 shape — a check that exists, reports success, and has stopped covering — arriving on a control rather than on a check. ## It asserts ordering, not presence Presence is the weaker property. The commit chose the harness layer specifically so there is no window: > the label goes into the create-issue call rather than a second call afterwards, and there is no window where the issue exists unlabelled So the test checks that the label is applied **before** `tool.session.CallTool`, which is the invariant that reasoning bought. | Mutation | Result | | --- | --- | | delete the call | fails, naming the consequence — every filed issue lands unlabelled | | move it after the dispatch | fails on ordering | | a second `CallTool` appears | fails, because this guards the first | The third matters because a new dispatch path added beside this one would otherwise pass while filing unlabelled issues. ## Why a source check, having criticised one earlier today `tool.session` is a concrete `*mcp.ClientSession`, so reaching `Call` needs a live MCP server. The repository already uses this shape for the same reason — `TestTheEvaluationRunnerKeepsLogsOffStdout` reads `main.go` because *"the alternative is running the binary"*. I flagged the weakness of source checks on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/407, where a format-string count missed a duplicate spelled differently. **That weakness is real here too**, and it is why this one asserts a relationship between two anchors rather than the presence of one string: a rename of either side fails loudly rather than passing vacuously, and the `CallTool` count catches a second path the first anchor would miss. The durable version needs `session` behind an interface so `Call` can be driven with a fake. That is production code and not mine; if it lands I will replace this with the behavioural test and delete the source check. ## What I verified about the control itself Sound on the adversarial questions I asked of it. `withSandboxLabel` **sets** rather than appends, so a model-supplied `labels` value is overwritten rather than merged — the comment says why, and it is the bypass I went looking for first. `toolName` is the bare name and the map key is qualified, so the `create_issue` comparison matches rather than silently never firing. A zero label ID disables the control rather than mislabelling. Test-only. No production change. Refs https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/208

coilyco-ops added 1 commit

2026-08-13 13:52:36 +00:00

test(mcp): pin the line that applies the sandbox label, which nothing held

ci / image-build (pull_request) Successful in 19s

Details

ci / test (pull_request) Successful in 32s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

f2648fb73b

abdd15c labels every issue the harness files, so attacker-influenceable content
is distinguishable in a tracker four agents act on. The policy is covered by
five tests. The line that invokes it was covered by none.

Deleting the two-line block in mcpToolSession.Call leaves the entire suite
green, so a refactor of the dispatch path drops a security control without a
single failure. That is the shape issues 291 and 328 are about, arriving on a
control rather than on a check.

A source check, because tool.session is a concrete *mcp.ClientSession and
reaching Call needs a live server. The repository already uses this shape for
the same reason in TestTheEvaluationRunnerKeepsLogsOffStdout.

It asserts ordering rather than presence, which is the property that matters:
applying after dispatch would leave the issue unlabelled for the length of the
request, and the commit chose the harness layer precisely to avoid that window.

Three ways it bites, all checked. Deleting the call names the consequence.
Moving it after the dispatch fails on ordering. A second CallTool appearing
fails, because this guards the first and would otherwise pass while an
unlabelled path existed beside it.

Refs sirens-echo#208

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 13:52:36 +00:00

coilyco-ops merged commit 00c76ca561 into main

2026-08-13 13:53:20 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-13 13:53:22 +00:00

Merge pull request 'test(mcp): pin the line that applies the sandbox label, which nothing held' (#456) from qa/pin-the-sandbox-label-wiring into main