Close the third review's follow-ups, and fix the SkillLearning contract gate - #16
Merged
Conversation
Seven findings from the third review, each an edge semantic or a claim stronger than the implementation behind it. 1. MCP sandbox lifecycle could leak a running container. McpClient owns the `docker run` process, and SIGKILLing that CLI does not stop the daemon-side container — so the sandbox was bounded on entry but not on exit. Make SandboxRunner.RemoveContainerAsync public (kill-by-name is part of the guarantee, not an implementation detail of RunAsync) and tear the named container down in a finally block on both MCP flavors, covering a failed handshake and a Ctrl-C as well as the happy path. 2. RedTeaming claimed to measure "the real GuardRails filter". It does not: GuardRails filters PII and length, RedTeaming needs a protected-material check, and nothing is actually shared between them. Rather than grow a feature in GuardRails to make the sentence true, say what the sample does — a GuardRails-STYLE deterministic output filter — in the code, the console banners, README and the pattern doc. No claim of coverage, so no drift. 3. Retry treated every OperationCanceledException as caller cancellation, so a dependency's own blown deadline skipped every remaining attempt. Cancellation expresses caller intent; a timeout expresses dependency failure, and .NET spells both with the same exception family — so ask the token instead of inferring from the type. RunAsync takes the caller's CancellationToken and rethrows only `when (cancellationToken.IsCancellationRequested)`; LocationTools converts its own 200ms deadline into a TimeoutException rather than letting an ambiguous OCE escape, and the circuit breaker counts that as the transient dependency failure it is. 4. IdempotentToolCalls printed "a FRESH caller process" for what is a fresh caller object in the same process. The architecture was already right — the dedup state lives with the side-effect owner — so correct the wording rather than build cross-process infrastructure for one sample. 5. PatternExplorer forwarded every DOTNET_-prefixed variable to child samples. That prefix is a namespace, not a category of harmless configuration: DOTNET_STARTUP_HOOKS loads an arbitrary assembly into the child. Replace the wildcard with a named six-entry set — exactly the ambient authority the surrounding environment.Clear() exists to remove. 6. LLMAsJudge announced "Position bias DETECTED" on any nonzero swing across five trials, where one differing verdict is as easily sampling noise. The probe demonstrates how to MEASURE position dependence; report what was observed and leave the claim to a real trial count. 7. The MCP server package was pinned but its base image was not, so two builds of the same image tag on different days could differ. Pin node:22-alpine by index digest alongside the tag, with the re-resolve command in the file. Tests: a dependency's own cancellation is retried rather than mistaken for caller intent; DOTNET_STARTUP_HOOKS is not forwarded; a TimeoutException trips the circuit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GEiuxZnHjyAfM8r8Loo4dc
…gest proves
The review's three "smaller observations" — none is a defect, each is a name or
a comment claiming more than the code delivers.
BoundedExecution: MaxEstimatedCost was a ceiling in name only. Unlike model
calls, tool calls and elapsed time, cost is derived from an admission estimate
(~4 chars/token) and reconciled after the provider has already read and billed
the request, so a sufficiently unusual input lands slightly over and is caught
one call late. Rename it EstimatedCostBudget — and MaxInputTokens to
InputTokenBudget, which has exactly the same property; renaming only the cost
field would have implied the input ceiling is hard. ExecutionBudget's doc
states the rule: Max* is enforced before dispatch, *Budget is reconciled after,
a detector rather than a guarantee.
ToolAuthorization: `var approverApproved = true;` never exercised denial, and
that line copied into a real host is a silent auto-approver no reviewer
notices. Put it behind IApprover, implemented by a DemoApprover that announces
"[DEMO APPROVER: automatically approving …]" on every call. One interface, one
implementation, on purpose: the only implementation here is a fake, and naming
the seam is what tells a reader which half they still have to build.
SkillLearning: the SHA-256 in manifest.json detects unexpected content
mutation; it does not authenticate content against an attacker. manifest.json
sits beside the file it vouches for, so a checksum with no secret and no
external root of trust cannot survive an adversary who already holds the write
access it is checking. Say so on ReadVerified, and stop the exception message
("was modified after approval") from implying tamper-proofing. No signatures
added — that is a different mechanism, not a stronger hash.
Test: rewriting SKILL.md AND the manifest digest is NOT detected, pinning the
documented limit so the prose cannot drift into a stronger claim than the code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GEiuxZnHjyAfM8r8Loo4dc
…kill
The sample crashed on every run with "Skill contract tests failed; candidate
was not promoted", so episode 2 never ran.
Root cause: ProvisionEmployeeSkillTests.Pass required the literal "first.last".
That string exists only in CreateAccount's ERROR message, and that error never
fires. Asked to provision "Maria Fernandez" the agent guesses maria.fernandez
on its first try, ^[a-z]+\.[a-z]+$ accepts it, and the rule therefore never
enters the trajectory the reflection distils from. Instrumenting a run made it
unambiguous:
CALL CreateAccount(username=maria.fernandez)
-> OK: account maria.fernandez created. <- first try, no error
idx first.last=-1 E5=492 team-=1163 onboarding=152
Every other clause passed. Three captured distillations were all faithful,
recording the E5 tier and the team-<department>-eu id verbatim — the gate was
rejecting correct skills for omitting a fact the episode cannot teach.
Pass now asserts the four tool names in the order the system enforces, plus the
two conventions only episode 1's errors reveal. Tool names appear verbatim in
the trajectory so the reflection echoes them reliably, whereas a prose template
survives only if an error quoted it. The username rule is deliberately not
asserted, and the reason is recorded next to the code: a contract test may only
assert what the run can actually produce.
Why the suite never caught it: SkillLifecycleTests.ValidSkill was a
hand-written ideal containing "first.last" and naming no tool at all — nothing
like real reflection output. It kept 300+ tests green while the sample could
not promote a single candidate. Reshaped to look like real output; its
Assert.Contains("first.last") now checks CreateAccount.
Also: a refused candidate now prints "[gate] Skill contract tests failed…" and
stops instead of throwing. The gate blocking a bad candidate is this pattern
working; an unhandled stack trace taught the opposite.
New SkillContractTests.cs uses real captured model output as its fixture —
RealReflectionOutputPasses fails against the old check.
Verified: 5/5 clean end-to-end runs (episode 1 fumbles, episode 2 zero errors).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GEiuxZnHjyAfM8r8Loo4dc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the third review's recommendations, then fixes a defect found while verifying them.
Each commit is green on its own, so the history bisects: 303 → 304 → 308 tests.
1. The seven follow-ups (
3c8ca7a)SandboxRunner.RemoveContainerAsyncmade public; both MCP flavors tear the named container down in afinallyOperationCanceledExceptionas caller cancellationwhen (cancellationToken.IsCancellationRequested)DOTNET_*wildcard forwarded to child samplesnode:22-alpine@sha256:c610fcdf…, index digest, verified by rebuildOn #2 I chose rewording over extraction: nothing is actually duplicated today — GuardRails filters PII and length, RedTeaming needs a protected-material check — so "sharing" would have meant growing a feature in GuardRails purely to make a sentence true.
On #3, the lesson is made concrete:
LocationToolsconverts its own blown 200 ms deadline into aTimeoutExceptionrather than letting an ambiguous OCE escape, and the circuit breaker counts that as transient. Cancellation expresses caller intent; a timeout expresses dependency failure — .NET spells both with the same exception family, so ask the token rather than infer from the type.2. The smaller observations (
dc7e950)MaxEstimatedCost→EstimatedCostBudget, andMaxInputTokens→InputTokenBudget. Both are admission estimates reconciled after the provider has already billed, so neither is a ceiling the host can promise. Renaming only the cost field would have implied the input limit is hard.Max*= enforced before dispatch;*Budget= reconciled after.IApprover/DemoApproverreplacevar approverApproved = true;, announcing[DEMO APPROVER: automatically approving …]on every call. That bare boolean copied into a real host is a silent auto-approver; aDemoApproveris obvious on sight.ReadVerifiednow spells out that SHA-256 detects unexpected content mutation while a signed manifest authenticates against an attacker —manifest.jsonsits beside the file it vouches for. No signatures added, per the review.3. The SkillLearning contract gate (
dee0816)Found while verifying the above: the sample crashed on every run, so episode 2 never ran.
ProvisionEmployeeSkillTests.Passrequired the literal"first.last". That string exists only inCreateAccount's error message — and the error never fires:The agent guesses
maria.fernandezfirst try,^[a-z]+\.[a-z]+$accepts it, so the rule never enters the trajectory the reflection distils from. Every other clause passed. Three captured distillations were all faithful — the gate was rejecting correct skills for omitting a fact the episode cannot teach.Passnow asserts the four tool names in enforced order plus the two conventions episode 1's errors actually reveal (E5,team-<department>-eu). Tool names appear verbatim in the trajectory, so the reflection echoes them reliably. A contract test may only assert what the run can actually produce.Why 300+ tests stayed green:
SkillLifecycleTests.ValidSkillwas a hand-written ideal containingfirst.lastand naming no tool at all — nothing like real reflection output. Reshaped, andSkillContractTests.csnow uses real captured model output as its fixture.Also: a refused candidate prints
[gate] …and stops instead of throwing. The gate blocking a bad candidate is this pattern working; a stack trace taught the opposite.Verification
Not done
ProvisioningSystemis unchanged. Making the username convention genuinely unguessable would restore a third learnable fact, but it redesigns the demo's content and any such rule is still a coin-flip on whether the model guesses it — the same fragility one layer over.🤖 Generated with Claude Code
https://claude.ai/code/session_01GEiuxZnHjyAfM8r8Loo4dc