Orchestration and evaluation failure semantics (finding 4) - #15
Merged
Conversation
… all fail Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…g the gap Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…tent turns Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…ion, real approval Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…the money-tool ceiling The concurrency test raced a single nonce once, which a read-then-write reserve survives 97-100% of the time; it now repeats 5000 trials on a fresh nonce and catches the reverted implementation 25/25. Commit-after-invoke and reserve-after-checks were asserted only in comments: Reserved and Consumed deny identically, so no test could tell them apart. Both now have an assertion that goes red when the ordering moves. MoneyMovingTools gets the ponytail comment naming its silent ceiling, the approver's grant is sized from the snapshot instead of a hard-coded figure and guarded so an unattended run cannot die on it, and both the sample and the doc now say that throwing keeps a refusal off the tool channel only because this host invokes the function directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Add ToolCallBudgetFilter, a Semantic Kernel IFunctionInvocationFilter that throws once the 10-call budget is exhausted, so the call that would exceed it never runs. Registered on ReasoningAndActing's local kernel; the top-level catch prints a PARTIAL result in the same shape BoundedExecution uses. The "at most 10 tool calls" prompt sentence is now documented as a hint, not the control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Review round 1: replace the InvalidOperationException message-substring match in Program.cs's catch with a dedicated ToolCallBudgetExceededException, matching BoundedExecution.AgentFramework's BudgetExceededException shape - a reworded message could no longer silently kill the catch. Tests now pin the exception type itself. Also fixes csproj ProjectReference ordering (the exclusion comment must sit with the project it follows) and tags the GuardAsync test seam with a ponytail comment naming its ceiling and upgrade path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…ple orderings Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
… to contains Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…unts separately F1 from review round 1: folding unreviewed golden cases (already have an expected answer + tier, just need sign-off) and the trace-derived candidate (no expected answer yet, needs one written from scratch) into one "candidate case(s) awaiting review" count contradicted the very distinction this task introduced. Print and document them as two separate counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…ing it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…t soft Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Every action moves to its current major and is pinned by the commit SHA that tag points at, with the version in a trailing comment. A mutable tag is a supply-chain hole: whoever can move the tag can run code in a workflow that holds packages: write on this repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…tion
ToolCallBudgetFilter was an IFunctionInvocationFilter that threw. SK 1.79's
FunctionCallsProcessor wraps every auto-invoked call in a catch-all that turns
any exception into a tool-result error message and keeps looping, so the throw
blocked the tool body but not the loop, and handed the model the budget refusal
as tool output to paraphrase — the exact failure mode task 4.3 removed from
ToolAuthorization.
Measured against real SK 1.79 with a stub endpoint that always requests a tool:
before (IFunctionInvocationFilter + throw): inner 10, model calls 129,
118 tool messages reading "Error: ... Tool-call budget of 10 exhausted."
after (IAutoFunctionInvocationFilter + context.Terminate = true):
inner 10, model calls 11, no refusal text reaches the model
Same mechanism GoalMonitoringFilter already used two projects away. SK returns
normally after Terminate, so the stop leaves via a BudgetExhausted flag that
Program.cs reads for its PARTIAL block; ToolCallBudgetExceededException and the
now-unreachable catch are deleted. The filter instance is registered per run, so
the counter's lifetime matches the per-run budget the doc claims.
AutoFunctionInvocationContext has a public 5-arg constructor, so the tests now
drive OnAutoFunctionInvocationAsync directly against the type the runtime uses;
the GuardAsync seam and its ponytail comment are gone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
RubricJudgeEvaluator deserialized the judge's reply with no try/catch: "" and
"not json" threw JsonException and crashed the whole LLMAsJudge run before the
pairwise probe ran, and "{}" produced a NumericMetric of 0 — below the rubric's
own floor of 1, so an unparseable verdict was recorded as worse than the worst
possible answer. Only the literal "null" ever reached the ?? fallback.
ParseVerdict now catches JsonException and rejects any score outside 1-5,
returning null; the evaluator turns that into a value-less NumericMetric with an
Indeterminate reason, and Program.cs prints INDETERMINATE rather than a blank
column that reads like a zero. Same ruling as JudgeParsing.Parse in task 4.5.
Tests drive the real evaluator through EvaluateAsync with a scripted judge
client, covering all four measured inputs plus whitespace, wrong-shaped JSON,
and out-of-range scores.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…t away Resolve() collapsed "which slot the reference candidate sat in" into "did it win" before the statistic was formed, so five trials drawn in one slot produced the same number as five alternating ones and the randomisation did no work. A judge that is simply wrong — it prefers the vague answer in both slots — reported "Position bias DETECTED: 100%". Summarize now takes Trial(slot, verdict), computes the reference candidate's win rate within each slot, and reports the absolute difference as PositionSwing. Always-picks-slot-A swings 1; always-wrong swings 0; a slot with no determinate verdict makes the swing null (not measurable) rather than 0. The five orderings are a balanced 3/2 shuffle instead of five coin flips, which also removes the 6.25% all-one-slot case. Doc frontmatter summary, body, README row and console labels all say swing now. --selfcheck still runs the real Summarize offline against fixed inputs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…al branches ResourceAwareOptimization.SemanticKernel caught bare Exception and fell through to the next tier on anything at all, while its AgentFramework twin was narrowed in an earlier PR. Narrowed to HttpOperationException / ClientResultException / HttpRequestException / TaskCanceledException. The twin's extra `!cancellationToken.IsCancellationRequested` guard has no counterpart here — the SK sample threads no token, so a TaskCanceledException can only be a timeout; the comment names the guard to add if a token is ever introduced. OrchestratorWorkers.md's mermaid ended at W1/W2/W3 -> S while the prose beside it described WorkerRegistry.Assess labelling runs Complete / Partial / Abstained. The diagram now shows the assess step, the all-failed abstain that skips synthesis, and the partial branch that flags the answer incomplete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
`S -->|complete| R[Answer]` reused the node id already bound to the registry, so the rendered graph lost the registry and gave "Answer" its inbound and outbound edges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
…filter ToolCallBudgetFilterTests already covers OnAutoFunctionInvocationAsync directly, but nothing in the suite drove real Semantic Kernel's auto-invocation loop — the seam where the original throwing-filter defect actually lived (129 model calls instead of 11, budget refusal handed to the model as tool output). Add ToolCallBudgetFilterRealLoopTests: a real Kernel wired the way Program.cs wires it, pointed at a stub HttpMessageHandler (ScriptedToolCallHttpHandler, added to Fakes.cs) that always answers with another tool call. Verified red under the original throwing shape (129 model calls, matching the measured figure) and under a >= boundary mutation (9 tool calls instead of 10); green restored on both. Also softens ReasoningAndActing.md's bare "128-round"/"129" SK-internals numbers to point at this test instead. 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
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.
Fourth and final PR remediating the second review (
docs/reviews/2026-08-25-second-review.md). It covers orchestration failure semantics and evaluation fail-closed behaviour: what each of these samples does when work fails, when a judge cannot be parsed, and when a bound is reached.What changed, per sample
Orchestrator-Workers — a failed worker no longer disappears.
BuildSynthesisInputfiltered failures out, so the synthesiser saw a gap it could not know about and confidently wrote over it. Failures now arrive withSTATUS: FAILED, the error, and an explicit instruction not to invent an output. A newWorkerRegistry.Assessclassifies the runComplete/Partial/Abstained; the sample skips the synthesiser entirely when every worker failed, and labels a partial run.Exception handling — cancellation is not a transient fault.
Retry.RunAsync's genericcatchconverted a cancelled turn into(null, lastError), so a caller who asked to stop got a fallback instead. It rethrows now. The Semantic Kernel twin had the identical defect — a cancelled call was swallowed, retried, and finally answered from a different kernel function — and is fixed too. The whole-turn-retry idempotency caveat now reaches the pattern doc and cross-linksIdempotentToolCalls.Tool authorization — a one-time capability now has a commit point. The nonce was burned during
Authorize, before the tool ran, so any pre-effect failure permanently destroyed a valid capability.Authorizenow reserves;Commitconsumes after the effect is durable;Releasereturns a reservation after a verified pre-effect failure. Money amounts are validated whether or not a maximum is configured — previously a refund with no ceiling accepted a negative amount. And an approval request is a typedPendingApprovalthe host handles, not a string the model receives as tool output and paraphrases.ReAct — the tool-call bound is enforced by the host. "Use at most 10 tool calls" was a sentence in a prompt. It is now an
IAutoFunctionInvocationFilterthat stops the loop at the boundary. See the note below: the first implementation of this did not work, and the reason is worth reading.LLM-as-Judge — an unparseable verdict is
Indeterminate, never a win.winner == "B" ? "B" : "A"meant every malformed reply counted as a victory for candidate A.JudgeParsing.Parsenow returnsIndeterminatefor anything that is not exactly"A"or"B", indeterminates are excluded from the rate and counted, and a single position swap became five balanced randomized orderings.Regression evals — a trace is a candidate, not a golden case. Extraction produced a
GoldenCasedirectly from a recorded run, freezing the model's own historical answer into the suite as the thing to match. It now produces aCandidateCaseawaiting review; only cases a human signed off withReviewedByare evaluated. The"exact"tier is renamed"contains", which is what it always did. A run that evaluated nothing now fails the gate instead of passing green.Resource-aware optimization — the budget is soft, and the docs say so. The last-resort call after every tier failed was never recorded, so the printed spend under-reported the real spend. It is accounted for now.
budgetbecamesoftBudgetin both flavors, and the copy stopped promising to "stop spending when the budget runs out" — routing decisions are made from spend observed after each call, which is what makes it soft. The docs point atBoundedExecutionfor a pre-call ceiling.CI — actions pinned by commit SHA. Every action moved to its current major and is pinned to the commit its tag points at, with the version in a trailing comment. A mutable tag is a supply-chain hole in a workflow holding
packages: write.The finding worth reading
Task 4.4 was first implemented as a Semantic Kernel
IFunctionInvocationFilterthat threw when the budget was exceeded. It passed its unit tests. It did not work.SK 1.79 wraps every auto-invoked function call in a catch-all that converts any exception into a tool-result error and continues the loop. Measured against real SK with a stubbed endpoint: the tool body was correctly stopped after 10 calls, but the loop made 129 model calls instead of 11, and the budget refusal was handed back to the model as tool output to paraphrase — the same failure mode this PR removes from
ToolAuthorizationtwo commits earlier. Meanwhile the pattern doc had been rewritten to claim the loop stopped.The unit tests could not see any of it, because they drove a seam the runtime never calls.
The fix is
IAutoFunctionInvocationFilter+context.Terminate = true, the shapeGoalSettingsAndMonitoring.SemanticKernelalready used. It is now pinned byToolCallBudgetFilterRealLoopTests, which drives the real auto-invocation loop through a stubHttpMessageHandler— no network, no port, sub-second — and asserts 10 tool bodies, 11 model calls, and that the refusal text appears in no request sent to the model. That test goes red at 129 model calls under the original throwing shape.Verification
dotnet build "Agentic Patterns.slnx" -c Release— 85 projects, 0 errors, 0 warningsdotnet test "Agentic Patterns.slnx" -c Release --no-build— 300/300 (238 before this PR), 16.5sToolAuthorizationPolicywas measured rather than read: its first test caught its own reverted implementation 3 times in 500 runs, and 0 in 300 under load. The committed version catches it 25/25.RubricJudgeEvaluator, below); the rest reported none, with the sites checked named.[Fact]/[Theory]204 → 239. Exactly one assertion changed across the branch —Assert.DoesNotContain("simulated failure", …)inverted toAssert.Contains, because the behaviour it pinned is the behaviour this PR deliberately reverses.Known gaps
RubricJudgeEvaluatorwas a fifth instance of the coerced-verdict defect, in the same project Task 4.5 hardened — it threw on malformed JSON and scored{}as 0, below the rubric's own floor of 1. Fixed in this PR, but it was found by the whole-branch review, not by the task that touched the file next door.ToolAuthorization'sMoneyMovingToolsis a hand-maintained set inside the policy. Register a new money-moving tool and forget the line, and it gets no amount floor. Marked with its ceiling and upgrade path; moving the flag onto tool registration touches every sample that registers a tool and deserves its own change.HumanInTheLoop.SemanticKernelstill returnsAPPROVAL_DENIED: …as function-result text — the shapeToolAuthorization.mdnow argues against. Real, out of this PR's scope, deliberately left for its own task.Commit/Releasereturnvoidand no-op on an invalid transition, as specified. Every invalid transition was verified to fail safe (no path creates authority or resurrects a consumed capability), but a host that commits the wrong nonce gets no signal..SemanticKerneltwins sit outside the test assembly by design, so twin-only fixes — the retry cancellation rethrow, the narrowed fallback catch — rest on review rather than on a test.RegressionEvalswrites candidates underbin/, so a candidate does not survive a clean to actually be reviewed. Deliberate for a sample; a real repo commits them beside the corpus.🤖 Generated with Claude Code
https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc