Fix tool rationale alignment across tool loops - #1933
Conversation
2a22781 to
3f68dfc
Compare
Rebase fallout: tests added after this branch forked construct shell_execute calls without _rationale, so the new rationale gate rejects them before their intended assertions run.
3f68dfc to
24d143e
Compare
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
This review covers the rationale gate, the history restore, and the circuit breaker. The mechanism is sound. The history restore in ChatMessageConverter fixes the root cause: the model drifts because it never sees its own prior rationales.
The inline notes raise two themes:
- The rationale gate moved into
ExecuteAsync. This inverts the call contract and forces a raw-or-cleaned split plus a double interpretation. A preflight-only gate is simpler. - The circuit breaker keys on the exact error prose. A stable
invalid_rationalecode already exists and travels asFailureCode. Key the detection on that code, not on the message text.
| if (ValidateArguments(toolCall.Arguments, resolveMeta) is { } rejection) | ||
| return rejection; | ||
|
|
||
| if (ToolCallMetaExtractor.ValidateRequiredRationale(toolCall.Arguments, resolveMeta) is { } rationaleError) |
There was a problem hiding this comment.
ValidateCore now requires the rationale. Both preflight callers already run InterpretToolCall first. So this gate runs twice per call. The second run also forces a cost: ExecuteAsync must receive the raw call, not the cleaned call, or it rejects its own input.
The pipeline (line 438) and the sub-agent (line 1317) both reject a call with no rationale before execution. Consider one option: keep the rationale gate at the preflight seam only, and drop it from ValidateCore. Then ExecuteAsync keeps its old contract, and the raw-or-cleaned split goes away.
The trade-off is defense in depth for a direct ExecuteAsync caller that skips preflight. No such caller exists today. Authorization stays inside ExecuteAsync, so this is a format gate, not a security gate.
| return rejection.Message; | ||
| } | ||
|
|
||
| toolCall = interpretation.Cleaned; |
There was a problem hiding this comment.
This line strips the meta inside ExecuteAsync. So ExecuteAsync now needs the raw call with _rationale present. A pre-cleaned call has no _rationale, so the internal InterpretToolCall rejects it.
IToolExecutor.ExecuteAsync does not state this rule. A future caller can pass a cleaned call and get a silent rationale rejection as the tool result. Please document the rule on the interface, or adopt the preflight-only gate (see the note at line 89).
| string? sessionScratchDenialDirectory, | ||
| ModelInputBatchBudget modelInputBudget) | ||
| { | ||
| var originalToolCall = tc; |
There was a problem hiding this comment.
originalToolCall exists only because ExecuteAsync re-strips the meta (DispatchingToolExecutor.cs:158). The pipeline interprets the call in preflight, then ExecuteAsync interprets it again. This is a double interpretation.
If the executor keeps its old contract (see the note at DispatchingToolExecutor.cs:89), the pipeline can pass the cleaned tc to the executor again. Then this capture and the second interpretation go away.
| // call's extraction reapplies the timeout/background/rationale. Outbound | ||
| // provider history leaves them out — the model must not see meta keys. | ||
| if (reinjectMeta && ToolCallMeta.Parse(tc.MetaJson) is { } meta) | ||
| if (ToolCallMeta.Parse(tc.MetaJson) is { } meta) |
There was a problem hiding this comment.
This block now runs for outbound provider history too. So the provider sees _rationale in each prior tool call. This choice reinforces the pattern, which is the intent, and it fixes the drift.
One caution: _rationale is not a declared tool parameter. A strict provider can reject a history tool call that holds an undeclared argument. The Spark eval passes today. Please add a short comment here that states this risk and the reason for the choice.
The front-door reject already blocks a tool call that has no rationale. The general tool-iteration budget already bounds a runaway tool loop. So the three-strike rationale breaker adds no protection. Remove it. Keep the front-door reject, the FailureCode transport, and the history restore. The chat TUI redesign consumes the FailureCode. - Restore TurnStateTracker, ActiveToolBatchTracker, and their tests to the dev baseline. - Remove StopToolsAfterInvalidRationale and its two call sites. - Remove IsRequiredRationaleRejection. - Remove the breaker integration tests and the dead fake support. The Actors suite passes 3,424 tests and skips one platform test.
Aaronontheweb
added
context-pipeline
1 participant
Problem
The provider can omit the required
_rationalefield after the first tool loop.Netclaw also dropped the persisted rationale from outbound assistant history, so
the model no longer saw its own prior rationales.
Changes
rejects the call and asks the model to resend it.
This PR does not change the TUI presentation.
Proof
tool_rationale_contracteval passed five of five runs on spark2.