Keep the original input under FreshContextPerIteration with ContinueWithMessages - #1063
PratikDhanave (PratikDhanave) wants to merge 1 commit into
Conversation
…ithMessages nextMessages returned the evaluator's explicit ContinueWithMessages messages before the FreshContextPerIteration branch, so in fresh mode the reinvocation received only the explicit messages. FreshContextPerIteration also resets the session to a pristine snapshot taken before iteration 1 (which does not contain the original request), so the original user input was lost from both the message list and the session - contradicting the documented contract that fresh mode 'restarts each reinvocation from the original input messages'. In fresh mode, always re-seed InitialMessages and compose the explicit messages on top; non-fresh behavior (explicit messages verbatim) is unchanged.
There was a problem hiding this comment.
🟢 Approval recommended
Final comments identify only minor nits, with no approval-blocking issues.
Pull request overview
This pull request preserves the original input when fresh-context iterations also include explicit continuation messages.
Changes:
- Re-seeds fresh iterations from initial messages.
- Appends explicit continuation messages.
- Adds regression coverage.
File summaries
| File | Summary |
|---|---|
agent/harness/loop/loop.go |
Updates fresh-context message composition. |
agent/harness/loop/loop_test.go |
Adds continuation-message regression coverage. |
Review details
Suppressed comments (3)
agent/harness/loop/loop.go:293
- This fresh-mode path introduces an undocumented exception to the public contracts:
Evaluation.Messagesis documented as being sent verbatim, whileFreshContextPerIterationis documented as rebuilding the original input plus aggregated feedback. Here the request isInitialMessagesplus explicit messages and the aggregated feedback message is skipped. Please update the API comments to document this precedence so callers can predict mixed evaluations.
if len(evaluation.Messages) > 0 {
explicit := cloneMessages(evaluation.Messages)
nextMessages = append(nextMessages, explicit...)
return nextMessages, explicit
agent/harness/loop/loop_test.go:581
- This test only inspects the provider input; it would still pass if the new fresh-mode path returned no surfaced explicit message. Assert the collected response contains
explicit, as the existing non-freshContinueWithMessagestest does, so the second return value is covered too.
if _, err := a.RunText(context.Background(), "original").Collect(); err != nil {
t.Fatal(err)
}
agent/harness/loop/loop_test.go:581
- The reported regression also includes the reset session, but this test uses an implicit per-run session, for which the default history provider is disabled. A regression that still omits
originalfrom the fresh session's stored history would therefore pass; add an explicit session/history provider and inspect the second call's session state as well.
if _, err := a.RunText(context.Background(), "original").Collect(); err != nil {
t.Fatal(err)
}
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Scope: public API, user-visible behavior Changed Go contract: Upstream evidence reviewed:
Result: findings reported — a semantic divergence from the upstream (.NET/Python) No exported Go API surface (types/signatures) changed —
|
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · copilot · auto · 102.1 AIC · ⌖ 5.39 AIC · ⊞ 9.6K
| nextMessages := cloneMessages(loopCtx.InitialMessages) | ||
| if len(evaluation.Messages) > 0 { | ||
| explicit := cloneMessages(evaluation.Messages) | ||
| nextMessages = append(nextMessages, explicit...) | ||
| return nextMessages, explicit |
There was a problem hiding this comment.
Parity issue with upstream FreshContextPerIteration + explicit "continue with messages" behavior
When FreshContextPerIteration is true and the evaluator supplies explicit messages, this composes them on top of loopCtx.InitialMessages (nextMessages = append(cloneMessages(loopCtx.InitialMessages), explicit...)). Both upstream implementations treat an evaluator-supplied explicit next-input as a full, verbatim override of the message list — not something that gets composed with the initial/original input — while FreshContextPerIteration/fresh_context only governs session and default-feedback-input state, not this override.
.NET:dotnet/src/Microsoft.Agents.AI/Harness/Loop/LoopAgent.cs,EvaluateAndBuildNextAsyncreturnsLoopNextStep.Continue(winner.Messages, ...)verbatim whenwinner.Messages is not null, beforeBuildNextMessages(the method that re-seedsInitialMessages) is ever invoked. The doc comment statesContinueWithMessages"bypass[es] this construction" (the fresh-context re-seed)..NETtestRunAsync_Fresh_WithContinueWithMessages_RecreatesSessionAsyncassertscapture.MessagesPerCall[1] == ["explicit"]exactly (no trace of the original "go" input), while confirming the session is still reset each iteration — i.e. fresh-context only resets session state, not the message override.- Python:
python/packages/core/agent_framework/_harness/_loop.py,_resolve_next_messagesendsnext_msgs(the caller's explicit override) as the entire next input;fresh_contextthere only changes the default-nudge fallback and session/progress handling, not an explicit override.
Suggested resolution: when evaluation.Messages is non-empty under FreshContextPerIteration, send those messages verbatim (as the pre-PR non-fresh branch already does), and let fresh-context mode continue to reset only the session state — matching ContinueWithMessages's documented "bypass" semantics in both upstream SDKs. If the original bug report's concern (losing the original input entirely) still needs addressing, that should be solved via a separate opt-in mechanism analogous to upstream rather than by unconditionally prepending InitialMessages to every explicit-messages continuation.
Problem
nextMessages(agent/harness/loop/loop.go) returned the evaluator's explicitContinueWithMessagesmessages before theFreshContextPerIterationbranch:So when an evaluator returns
ContinueWithMessages(...)whileFreshContextPerIterationis enabled, the reinvocation gets only the explicit messages. Fresh mode also resets the session to a pristine snapshot taken before iteration 1 (which does not yet contain the original request), so the original user input is lost from both the message list and the session.This contradicts the
FreshContextPerIterationdoc: "restarts each reinvocation from the original input messages plus an aggregated feedback log …".Fix
Check
FreshContextPerIterationfirst: always re-seedInitialMessages, and when the evaluator also supplied explicit messages, compose them on top of the fresh initial context (surfacing the explicit messages). Non-fresh mode is unchanged — explicitContinueWithMessagesis still sent verbatim (locked byTestLoop_ContinueWithMessagesSendsMessagesVerbatim).Test
TestLoop_FreshContextPerIteration_ContinueWithMessagesKeepsInitialruns a fresh-context loop whose evaluator returnsContinueWithMessages(["explicit"])and asserts the reinvocation still contains the original"original"input plus the explicit message. Fails before the fix ([explicit]only), passes after; the existing fresh-context + feedback test remains green.