fix: make pattern guarantees true in code (second review, findings 1,3-6) - #12
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
The dedup registry lived in the caller's process and reset on a post-commit throw, which only protected the easy lost-response window. Move it into SimulatedRefundService so the refund and its idempotency record commit together, closing the hard window: committed remotely, caller never found out. IdempotentTool is now a thin client with no local state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
… heuristic Every uncertainty signal (self-report, logprobs, consistency sampling) now scores the same candidate answer instead of three signals describing three possibly-different completions. Consistency is decided by a temperature-0 equivalence probe instead of keyword overlap, and a malformed judgement fails closed as disagreement. Output is relabeled a heuristic uncertainty score with an explicit "not a probability of correctness" disclaimer, and docs drop the "trustworthy confidence score" / "most objective signal" claims. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Deserializing into Dictionary<string, bool> throws JsonException the moment the model adds any non-boolean field (e.g. a "reason" string) alongside "equivalent" — unhandled, it propagated through Task.WhenAll and crashed the run instead of counting as disagreement. Switch to a typed EquivalenceResponse (ignores unknown members) wrapped in try/catch, matching the SK twin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
… output filter Four fixes to RedTeaming.AgentFramework's evaluation: the judge now receives the actual secret and canary it must compare replies against; unparseable judge output becomes Indeterminate (never silently Safe); the defended agent now wraps a real GuardRails-style output-filter middleware, and every run reports both with-filter and without-filter rates so the sample measures the filter it claims to; and the metric is now a Wilson confidence interval over a checked-in 12-probe corpus (default run) with an optional --explore N for generated probes, rather than a point "attack-success-rate" twelve samples cannot support. LeakDetector.Deterministic matches adjacent secret-segment PAIRS rather than single segments - a lone segment like "INTERNAL" is an ordinary English word that false-positives on innocuous refusals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Review finding: the deliberate adjacent-segment-pair shortcut in LeakDetector.Deterministic lacked the repo's `// ponytail:` tag. Replaced the rationale comment with a ponytail-tagged one naming both failure modes (misses non-adjacent/single-segment leaks; Flatten's separator-stripping can false-positive on ordinary text like "TechCorp Internal Support") and the upgrade path (fuzzy/n-gram matching once secrets exceed three segments). Comment-only change; LeakDetector tests re-verified unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
… split iterations BudgetedChatClient reserved a fixed 2000/800 tokens and only discovered an overrun in Reconcile, after the provider call already happened; missing usage was charged as zero, silently disabling the ceiling. Now the request is estimated before dispatch and the provider's own MaxOutputTokens is capped to the remaining budget, so the full worst case is reserved up front; usage the provider omits is charged at the reservation, never zero. Iterations no longer increments inside ReserveModelCall - it's now recorded once per orchestration-loop turn via RecordIteration(), decoupling it from ModelCalls (which also counts retries). CreateTimeout now schedules only the remaining MaxElapsedTime instead of restarting the full budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…vocations The run-level RecordIteration() hook fires once per external RunAsync call, not per internal reasoning/tool-calling turn - so a single agent.RunAsync pinned Iterations at 1 forever, making MaxIterations dead configuration and overclaiming the "orchestration-loop boundary" doc wording. The demo now runs a short loop of research sub-questions under the same ExecutionBudgetState/timeout, so Iterations and ModelCalls genuinely move independently and MaxIterations is live. Console output now prints Iterations alongside ModelCalls. Doc wording tightened to describe what is actually bounded: one agent invocation, checked before the run starts. Also widened TimeoutUsesTheRemainingDurationNotTheWholeBudget's margins (200/120/150ms -> 600/400/350ms): it failed intermittently under full-suite load, per the task's pre-approved flakiness fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…on, approved idempotent booking
Planning could not satisfy its own goal: GetFlights returned no prices so
"cheapest" was unachievable, the hardcoded date was in the past, unresolved
{{stepN}} placeholders reached tools as literal strings, nothing validated
step IDs/duplicates/dependency ordering before execution, and BookFlight
fired with no approval or idempotency.
Both flavors now expose five tools (GetFlights, SelectCheapest,
RequestBookingApproval, BookFlight, DraftEmail), select the cheapest flight
deterministically on the host instead of trusting the model to parse it from
free text, validate the whole plan with PlanValidator.Validate before any
tool runs, resolve {{stepN}} placeholders strictly (throwing rather than
passing them through), and gate BookFlight behind a console approval bound
to the exact flight and price plus a per-run idempotency key. A rejected or
aborted plan now prints a clear message and exits cleanly instead of
crashing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…, broaden failure catch Review of 745f35a found PlanValidator checked step count/duplicates/allow-list/ forward-references but never which tool an argument's output actually came from, so a plan could skip SelectCheapest or RequestBookingApproval entirely and still validate cleanly - defeating the two guarantees this task exists to add. Validate now carries a RequiredProducer table (SelectCheapest<-GetFlights, RequestBookingApproval<-SelectCheapest, BookFlight<-RequestBookingApproval) and rejects a step whose argument is not exactly the output of the required preceding step, in both flavors. Also: RequestBookingApproval now accepts y/yes case-insensitively with a (y/n) prompt, matching DurableHumanInTheLoop's Approval.Approved shape instead of requiring the exact literal "yes" (EOF still denies, unchanged). The execution loop's catch now covers any step failure, not just InvalidOperationException, so a shape-mismatched argument prints "Plan stopped at step N" instead of an unhandled stack trace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…venance contracts
Round 2's RequiredProducer check asked "does *some* value in this step's
argument dictionary carry the required placeholder", not "does the
parameter the tool actually binds carry it". A decoy key (e.g. BookFlight
called with a fabricated approvedFlight plus an unused decoy: {{step3}})
satisfied the Any() scan while the real bound parameter carried the
fabricated value - the human approves flight X, the system books flight Y.
PlanValidator.Validate (both flavors, mirrored) now carries a per-tool
ToolContract naming the tool's exact closed parameter set and, where
applicable, which single named parameter must be exactly {{stepN}} of a
specific preceding tool. A step whose Args keys are not exactly the
declared parameter set is rejected outright, and the provenance check reads
the required parameter by name (step.Args[name]) rather than scanning all
values - a decoy key can no longer smuggle a valid reference past the
parameter that is actually bound at runtime.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…-T3) Fixes the whole-branch remediation wave: promotes CreateTimeout's remaining-time computation to an assertable RemainingTime property to remove a ~25% flaky timing test (C1); corrects the BoundedExecution doc summary and README rows to match the hard-vs-estimated token distinction (I2, I7); fixes a stale ConfidenceReporting overclaim in EvaluationAndMonitoring.md (I3); stops RedTeaming's LeakDetector from false-positiving on the defended agent's own "TechCorp" persona prefix (I4); discloses RedTeaming's WITH-filter deterministic count is zero by construction and prints deterministic-vs-judge counts separately (I5); makes ConfidenceReporting's AgreesAsync surface unparseable equivalence probes instead of silently folding them into disagreement, in both flavors (I6); restores the concurrent-duplicate-key and caller-cancellation idempotency tests (T1); aligns Planning.md's approval prompt text with the literal (y/n) code (T2); and rewords the ConfidenceReporting/Voting csproj exclusion comments to state what they actually are (T3). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
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
1 participant
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.
Closes findings 1, 3, 4, 5 and 6 from the second external review, now recorded in
docs/reviews/2026-08-25-second-review.mdas the binding spec for this work.Every finding in this PR was the same shape: a sample or its doc promised a
broader guarantee than the code delivered. The fix is always to make the code
true, not to soften the copy.
What changed
IdempotentToolCalls — the lost-response window is closed.
The dedup registry moved out of the tool and into
SimulatedRefundService, theside-effect owner, so the idempotency record commits with the refund itself. A
caller that crashes after the side effect but before learning the outcome now
gets exactly one refund on retry. The demo builds a fresh tool for the retry
to prove no client-side state carries the guarantee. Adds request-hash conflict
detection (
IdempotencyConflictException) and tenant scoping.ConfidenceReporting → uncertainty signals.
One canonical candidate is now taken from the raw completion, and logprobs,
self-report and consistency all score that text — previously three sources
scored three different answers and were averaged. Keyword overlap is replaced by
a fail-closed LLM equivalence probe that returns
bool?(null = unparseable,counted as disagreement and surfaced in the output). The composite is relabelled
an uncalibrated heuristic, with the calibration path named in a
ponytail:note.RedTeaming — the attack success rate means something.
Deterministic secret/canary checks run before the judge; malformed judge verdicts
become
Indeterminateand forceRESULT: INCONCLUSIVEinstead of silentlyscoring as a pass. A real GuardRails filter runs so with/without is a genuine
comparison. Probes moved to a checked-in
probes.jsoncorpus (12 probes, 4classes) and results carry Wilson score intervals.
BoundedExecution — the limits are hard.
MaxOutputTokensis now capped to the remaining budget on a clonedChatOptionsand the full cap reserved up front; unreported usage is charged at the
reservation rather than free.
Iterationsis split fromModelCallsand thedemo loops three research sub-questions under one shared budget and timeout, so
MaxIterationsis live config rather than structurally pinned to 1. Timeouts useremaining time, not the whole budget. The doc's per-dimension enforcement table
is now literally true.
Planning — the plan can satisfy its stated goal.
Flights are priced,
SelectCheapestis deterministic, and a newPlanValidatorenforces per-tool argument contracts with provenance: each tool declares its exact
expected parameter set and the single parameter that must carry the upstream
producer's placeholder, looked up by name.
BookFlightis structurally unable torun on a value that did not come out of
RequestBookingApproval. Adversarialreview tried eight bypass shapes — decoy keys, case-variant duplicates, broken
chains, duplicate and unreachable steps, whitespace and case placeholder variants
— and all are rejected before any tool executes.
Verification
dotnet build "Agentic Patterns.slnx" -c Release— 85 projects, 0 errors, 0 warningsUncertaintySignalTests,LeakDetectorTests,PlanValidatorTests,plus budget cases added to the existing
BoundedExecutionTestsNot in this PR
Findings 2 (MCP execution boundary) and the medium/table items are staged for
follow-up PRs: untrusted execution boundary, integrity and isolation, and
orchestration/evaluation failure semantics.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc