Skip to content

fix(agent): isolate response language from connector context (toby) - #1990

Draft
OliverBryant wants to merge 10 commits into
xorbitsai:mainfrom
OliverBryant:codex/fix-request-language-consistency
Draft

fix(agent): isolate response language from connector context (toby)#1990
OliverBryant wants to merge 10 commits into
xorbitsai:mainfrom
OliverBryant:codex/fix-request-language-consistency

Conversation

@OliverBryant

@OliverBryant OliverBryant commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Status

This pull request is now a draft umbrella and implementation reference. The aggregate branch will not be merged and will receive no further aggregate fixes.

Original context

Connector- or file-enriched execution text can include names, email addresses, quoted source content, and localized metadata in a language different from the user-authored request. That auxiliary context must not silently determine the language of user-facing output.

request_context.output_language remains the sole caller-controlled hard language authority. Without that pin, the answering model must derive language from canonical user-authored request provenance while preserving explicit and implicit target-language intent, context-dependent requests, and Simplified versus Traditional Chinese.

The aggregate implementation explored this contract across root context, Auto, ReAct, DAG planning, DAG steps, completion, final-answer schemas, compaction, and checkpoint restoration. Repeated review showed that provenance, policy representation, and activation need independently reviewable acceptance boundaries.

Replacement rollout

Parent tracking issue: #2062

Replacement pull requests will be created serially from the latest upstream main after the preceding layer merges. Activation remains the final layer; no replacement branch will be based on another feature branch.

Reference policy

  • This aggregate branch is reference material only and will not be merged.
  • Existing aggregate CI results are historical evidence; each replacement pull request owns its independent validation and required CI.
  • This umbrella will close without merge after all three replacement layers merge and the end-to-end acceptance criteria pass.
  • General provider input-plus-output admission remains tracked separately in fix: enforce provider input and output context budgets #2041 and is out of scope for this rollout.

@XprobeBot XprobeBot added the bug Something isn't working label Sep 1, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@rogercloud

Copy link
Copy Markdown
Collaborator

ci is failing

@OliverBryant
OliverBryant force-pushed the codex/fix-request-language-consistency branch from e4d2e19 to 476d0fa Compare September 1, 2026 06:45

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR separates user-authored display text from connector-enriched execution context when deriving response language. It introduces a request-only language harness and threads it through root system context, Auto/ReAct guidance, DAG planning, and DAG completion while preserving caller-pinned request_context.output_language as the hard authority. It also adds and updates prompt-level regression assertions for the new seam.

Blocking: yes — recommended event: REQUEST_CHANGES

Approach

Verdict: acceptable-with-reservations. The hard-versus-soft authority split and the shared output_language_directives seam fit the stated problem and cover the relevant response paths. However, the request-only helper currently couples language-rule text to a complete request value; structured plan and completion payloads already carry that value, so this design introduces avoidable unbounded prompt duplication and context-capacity risk. That reservation is a blocking major finding below.

Update summary

The current PR head contains three commits. 01b4efce (fix(agent): isolate response language from connector context) changed production code to add the request-only harness and pass user-authored request text through the root, planner, and completion language paths, with initial coverage. 7af3fad5 (chore(tests): apply isort formatting) only formatted the new harness test, while 476d0fad (test(agent): update language prompt assertions) updated assertions across the Auto, context, DAG, and ReAct tests. The complete history contains no prior technical findings; the only conversation roots are the Gemini quota warning (5488913970) and rogercloud's ci is failing note (5489203088), both informational.

Findings (major)

1. src/xagent/core/agent/context/execution.py:527 — blank display text falls back to connector-enriched execution text

Severity: major
Blocking: yes

  • Reachable trigger: A supported AgentRunner/AgentExecutionAdapter/ExecutionRegistry continuation supplies a non-empty execution_message containing connector or file enrichment and explicitly supplies display_message as "" or whitespace, with no caller-pinned request_context.output_language. The core injection contract rejects a missing value (None) in the relevant split case, but accepts an explicit blank value and persists it in Message.metadata; file-only or attachment-backed turns are supported by this contract.
  • Observable impact: _current_user_request_text(prefer_display=True) only returns display_message when display.strip() is non-empty, then falls back to the full Message.content. This new root call therefore gives request_only_language_harness connector/file text while labeling it as the user-authored request. Foreign-language connector metadata, addresses, quoted source text, or file instructions can steer root/Auto/ReAct prose, DAG plan prose, and DAG completion/final-answer prose into the connector language instead of the user's conversational language, violating the user-visible response-language contract. The same fallback is reached by the new planner calls at src/xagent/core/agent/pattern/dag/plan_generator.py:564,577-583 and completion calls at src/xagent/core/agent/pattern/dag/dag.py:1506,1518-1527.
  • Why the caller/type/API contract permits it: The core APIs expose separate execution_message and display_message values and do not require a non-empty display string; endpoint-specific non-empty text validators do not constrain these core live-control callers. Persistence and checkpoint restoration retain the display metadata, so a blank display value remains a real supported turn rather than being normalized away. A caller-provided output-language pin is a valid mitigation, but the unpinned mode is explicitly supported and is supposed to use user-authored display text as its soft language signal.
  • PR causation: Before this change, the root system-context, planner, and completion language boundaries did not pass these request values into the new request-only harness. The PR's new request= arguments at the root and structured DAG consumers directly expose the existing non-empty-display fallback as the sole quoted language source.
  • Required fix: Make the language-specific extractors distinguish a missing display_message key from a present blank/whitespace value. Preserve any compatibility fallback only for a missing key; for a present blank value, return an empty language request and do not substitute Message.content. Apply that strict semantics to both _current_user_request_text and latest_user_text, and add root, plan, and completion regressions using blank and whitespace display text with foreign connector-enriched execution text. Do not reject blank display text outright, because that would break the supported file-only/attachment turn contract.

2. src/xagent/core/agent/language.py:498 — structured prompts duplicate an unbounded complete request

Severity: major
Blocking: yes

  • Reachable trigger: An unpinned DAG request is large enough to be near the selected provider's finite context window. The supported request contracts permit arbitrary non-empty content (the public schema has no maximum, legacy message inputs are unconstrained strings, and persisted content is Text), while the bespoke planner and completion payloads have no token budgeting or compaction for these added fields.
  • Observable impact: request_only_language_harness now serializes the complete request into output_language_policy. The plan payload already carries latest_user_request and the execution transcript in messages, so this adds approximately one request-sized copy; the completion payload adds the request to output_language_policy and again to user_authored_language_request, on top of its existing transcript fields, adding approximately two copies. The verifier measured a 3,000-character request at roughly 3,422 estimated tokens for the baseline plan payload versus 4,450 now, and roughly 3,098 versus 4,886 for completion using the repository's four-characters-per-token estimator. Thus a provider call that fit a 4,096-token configuration before this PR can be rejected for context length, or lose budget needed for step_results and candidate_output; even below the limit, every request incurs proportional extra input-token and cost work.
  • Why the caller/type/API contract permits it: Complete request text is reachable and cannot simply be truncated: explicit target-language instructions may occur anywhere in the request, and the language contract requires preserving them. No provider-side deduplication contract removes the copies, and context compaction only handles stored ExecutionContext messages, not these structured plan/completion JSON payloads. The structured consumers already have a clean field to reference (latest_user_request in plan_generator.py:576 and user_authored_language_request in dag.py:1524-1526), so carrying the value once is compatible with the existing payload contract.
  • PR causation: In the base code, the plan and completion consumers passed no request to output_language_directives, so their policy text was fixed-size. This PR changed language.py:583-601 to render the full request-only harness for unpinned sections and added the structured request fields/callsites, creating the new request-sized copies.
  • Required fix: Separate rule text from request-value carriage for structured consumers. Keep the complete JSON quote only at the root system-context boundary (or another boundary that genuinely lacks a clean request field); make the plan policy a fixed-size instruction that references latest_user_request, and make the completion policy reference user_authored_language_request. Preserve the clean request once per structured payload and update payload tests to assert field references and the absence of a second full quote. Do not solve this by truncating the request.

Prior-findings checklist

  • No prior technical findings or technical threads require resolution. Review bodies and inline review comments were empty, with no technical replies, duplicate occurrences, or tracking references.
  • [i] Informational history only: comment 5488913970 was a Gemini quota warning, and comment 5489203088 was rogercloud's ci is failing statement. Neither contains a technical finding or reply chain.

Review limitations

The Simplification Lens was unavailable due to the usage limit. No simplification finding is generated, and no empty Simplification opportunities section is included.

Blocking status & recommended decision

Blocking: yes.

Blocking issues:

  • src/xagent/core/agent/context/execution.py:527major — blank display text can make connector/file-enriched execution content control the user-visible response language. [new]
  • src/xagent/core/agent/language.py:498major — unbounded request copies can turn a baseline-fitting DAG call into a provider context-length failure and consume completion payload budget. [new]

Recommended decision: REQUEST_CHANGES

Comment thread src/xagent/core/agent/context/execution.py Outdated
Comment thread src/xagent/core/agent/language.py

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The runtime keeps two forms of each user turn: the display_message the human actually typed, and an execution message enriched with connector context (sender names, email addresses, localized metadata). The previous soft language policy pointed vaguely at "the current user request," so a question asked in English over, say, a Spanish-language email connector could come back in Spanish. This PR makes request_context.output_language the only hard authority and, when it is absent, feeds the model only the JSON-quoted display_message as language evidence — applied consistently across the root system context, Auto, ReAct, and DAG planning/steps/completion, plus the final-answer schemas.

Round 0 design verdict: acceptable-with-reservations. The fix is the right shape given the constraint: separating display text from connector-enriched execution text is the minimum viable seam, and the deeper root cause — connector context being concatenated into user content at all instead of carried as separate metadata — is a much larger refactor that is reasonably out of scope here. The reservations are about accumulation: the codebase now carries roughly seven overlapping language-prose snippets (output_language_policy, response_language_rules, final_answer_language_rule, dag_step_language_rules, plan_language_rules, request_only_language_harness, _structured_request_language_policy), and that duplication has already produced a real defect (F4 below). The empty-request case also appears never to have been designed for explicitly; a single if not request: return response_language_rules() guard in the two harness builders would have removed several of the findings below at the source.

Update since last review

Since the last round the branch picked up 8c3efb2b ("harden request language boundaries"), 476d0fad ("update language prompt assertions"), 7af3fad5 (isort), and 01b4efce. The substantive change is 8c3efb2b: it introduces the tri-state display_message_override() extractor (distinguishing key-absent from present-but-blank from non-string) and switches the DAG plan and completion payloads to reference-only language policies that name a field instead of re-embedding its value. That addresses both prior findings. The same mechanism, however, introduced several new issues listed under Findings.

Prior findings

Prior Finding 1 — blank display_message fell back to connector-enriched execution content: FIXED. src/xagent/core/agent/context/enrichment.py:113-126 now implements a tri-state contract — missing key / non-dict metadata / non-string value yield None (execution-content fallback preserved only there), while a present string, blank included, is .strip()'d and treated as authoritative-empty with no fallback. Both ExecutionContext._current_user_request_text(prefer_display=True) and latest_user_text(prefer_display=True) route through it. tests/core/agent/test_request_language_harness.py:72-141 (15 tests, run and passing) parametrizes blank and whitespace display_message across the root, DAG-plan, and DAG-completion paths and asserts polluted connector text never reaches the language-governing text. Thread already resolved.

Prior Finding 2 — DAG plan/completion payloads embedded the whole request a second time inside output_language_policy: FIXED. _structured_request_language_policy() at src/xagent/core/agent/language.py:511-526 now references the field name only (e.g. "the latest_user_request field") instead of re-quoting the value. tests/core/agent/test_request_language_harness.py::test_structured_language_payloads_include_a_large_request_exactly_once (run and passing) confirms each structured payload contains the request exactly once. Thread already resolved.

Findings

F1 — MAJOR · Blocking: yes · src/xagent/core/agent/pattern/dag/dag.py:1710-1714

_step_instruction was not updated by this PR: it calls output_language_directives(effective_output_language(root_context), section="dag_step_instruction") with no request=. The dispatcher in language.py has no explicit branch for that section, so it falls through to request_only_language_harness(request) with the default request="". When no output language is pinned — the default case — this renders a harness that JSON-quotes an empty string as the "user-authored request" and tells the model to pick a language from that empty quote. The same DAG step's system prompt already carries a correct harness quoting the real request (dag_step_request_anchor, src/xagent/core/agent/context/execution.py:598-611), and both land in the same LLM call, so the model gets two contradictory language harnesses at once. This is a regression against base, where the same fallthrough emitted output_language_policy(language) — self-contained and non-contradictory even when empty.

Fix: thread request=latest_user_text(root_context, prefer_display=True) through this call site (root_context is already in scope, and the same helper is used at src/xagent/core/agent/pattern/dag/dag.py:1506), or restore output_language_policy(language) as the dag_step_instruction fallback in language.py.

Blocking rationale: every DAG step without a pinned output language emits a self-contradictory language instruction inside a single model call, directly undermining the reliability this PR exists to deliver.

T4 (test, MINOR, tied to F1): tests/core/agent/test_output_language_seam.py:161-163 and tests/core/agent/test_dag.py:5298 currently assert the empty-request harness as the correct DAG-step-instruction output, in setups that explicitly construct a real non-empty user request first. Both codify the defect and must be updated to assert the real request is preserved when F1 is fixed.

F2 — MAJOR · Blocking: yes · src/xagent/core/agent/context/execution.py:521-541

On the root (non-DAG) path with no display_message metadata — the common non-connector case, where display falls back to content — current_task (line 524) and request (line 530, via prefer_display=True) resolve to the same string, and the root system context now appends it twice: once raw under "Current user request:", then again JSON-quoted inside request_only_language_harness(request). Verified empirically: context._system_context().count(request) == 2 for this case. Base used response_language_rules() for the no-pinned-language branch, which embeds no request text at all, so this is a new per-turn token cost proportional to request length on every root LLM call without a pinned output language. The new regression test only covers the deliberately-constructed case where display_message differs from content, so it never exercises this fallback.

Fix: when the harness is appended immediately after the same text was printed verbatim as "Current user request," either drop the quoted copy inside the harness or drop the raw line and rely on the harness alone.

Blocking rationale: an unbounded per-call token regression on the default chat path, untested — for large supported requests it recreates exactly the context-budget risk Prior Finding 2 was fixed for, just in a different code path.

F3 — MAJOR · Blocking: no · src/xagent/core/agent/language.py:529-541 (+ call sites pattern/auto/auto.py:1370,1391, pattern/react/react.py:1086,2075,2091)

The default subject of final_answer_language_rule changed from the self-contained "current user request" to a pure pointer, "authoritative output language guidance in the system context". But the root language block at src/xagent/core/agent/context/execution.py:526 is emitted only when current_task is truthy (if current_task and not dag_step_id:), and current_task can be empty — src/xagent/core/agent/runner.py:227 only calls add_user_message(task, ...) if task:, so an attachment- or context-ref-only turn (and some resumed/scheduled invocations) produces no language guidance block at all, while all five final-answer/decision schemas still tell the model to follow "the authoritative guidance in the system context" — a dangling reference. The base default degraded gracefully in the same scenario. No test exercises the empty-current_task path; test_output_language_seam.py's helpers always add a non-empty user message.

Fix: keep the old self-contained default subject as a base, or fall back to it when the directive block will not render.

Blocking rationale: a real regression with zero coverage, but it requires an edge-case invocation shape (no free-text task) off the mainstream chat path — worth fixing, not worth gating merge.

F4 — MINOR · src/xagent/core/agent/language.py:511-526

subject = f"the \{request_field}` field"is passed intoresponse_language_rules(subject=...), whose template already prepends "the " before {subject}in three places. Rendering the actual output confirms it reads "Use the same natural language as **the the**latest_user_requestfield…", three times, in every DAG plan-generation and completion-assessment prompt._structured_request_language_policy is new in this PR; the helper it misuses is unchanged. Existing tests assert only that the substring "latest_user_request` field" appears, so nothing catches it.

Fix: subject = f"\{request_field}` field"`.

F5 — MINOR · src/xagent/core/agent/pattern/dag/dag.py:1524-1526

When a language is pinned, the payload still emits "user_authored_language_request": "" — a dead key the accompanying policy text never references. Cosmetic clutter, no functional or budget impact; suggest omitting the key entirely when output_language is set.

F6 — LOW, informational · src/xagent/core/agent/runner.py:902-908 (pre-existing) × src/xagent/core/agent/context/enrichment.py:113-126

runner.py unconditionally coerces any present-but-non-string display_message (JSON null included) to "" before display_message_override() ever sees it, so the function's documented "legacy non-string values retain the execution-content fallback" branch is unreachable through the production runner path — it is reachable only from hand-constructed test contexts, which is exactly how test_unsupported_display_metadata_preserves_execution_content_fallback is written. This is practically safe and not a re-opening of Prior Finding 1: the resulting authoritative-empty harness still tells the model to derive language from conversation context, not from polluted execution content. It is a docstring-accuracy and coverage gap. Suggested follow-up: either correct the docstring to match real reachability, or have runner.py preserve None so the documented branch becomes live, plus an end-to-end runner test with display_message: null.

Test quality notes

  • T1tests/core/agent/test_request_language_harness.py:40-59: test_request_language_harness_preserves_the_whole_request_without_detection is parametrized over five inputs of differing language and length, but four of its five assertions check invariant boilerplate constants of the function under test (request_only_language_harness does not branch on input language or length). Only the json.dumps(...) in harness assertion depends on the parametrized value, so all five cases hit identical code with identical branch coverage while implying cross-language discrimination testing that isn't happening.
  • T2tests/core/agent/test_request_language_harness.py:243-251: test_final_answer_schemas_follow_the_shared_language_guidance asserts on literal substrings copied from final_answer_language_rule(). It does verify the guidance is wired into both the ReAct and Auto schemas (it would catch an entirely missing call), but as a string-echo check it would miss the right-looking text landing on the wrong field, or a swapped subject= that still happens to contain those substrings.
  • T3tests/core/agent/test_context.py:743-747: test_dag_step_language_quote_uses_the_typed_message was loosened from quote.startswith(typed) to typed in quote. The loosening is legitimately forced by the new JSON-string wrapping, but a tighter JSON-aware form — quote.startswith(json.dumps(typed, ensure_ascii=False)) — was available and would keep a positional guarantee; today the only remaining safety net is the separate "Attached file(s)" not in quote check.
  • T4 — see under F1.

Simplification opportunities

  • src/xagent/core/agent/language.py L484: shrink — request_only_language_harness (L484) and _structured_request_language_policy (L511) are two ~20-line near-duplicate prose blocks (same "not language evidence" list, same Chinese Simplified/Traditional clause, same "controls language only" clause), differing only in quote-the-value versus name-the-field framing; a single parameterized helper (subject, optional quoted_value, an empty-request wording toggle) covers both call sites — and would have prevented F4, which arose precisely because these two blocks are maintained by hand.

net: -8 to -12 lines possible

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

  • [new] F1 — src/xagent/core/agent/pattern/dag/dag.py:1710-1714: missed call-site update emits an empty-request language harness that contradicts the correct harness in the same DAG-step model call, on the default no-pinned-language path.
  • [new] F2 — src/xagent/core/agent/context/execution.py:521-541: the root system context now duplicates the full user request text on every unpinned root call, an untested token-cost regression proportional to request length.

F3-F6 and the test notes are not merge blockers, but F3 and F4 are cheap and worth folding into the same round.

Comment thread src/xagent/core/agent/context/execution.py Outdated
Comment thread src/xagent/core/agent/language.py Outdated
Comment thread src/xagent/core/agent/language.py Outdated
Comment thread src/xagent/core/agent/pattern/dag/dag.py
@OliverBryant

Copy link
Copy Markdown
Contributor Author

Addressed the overall-review items in c785684. DAG step instructions now use fixed-size guidance that points to the authoritative real-request anchor already present in child system context, with no empty JSON quote or duplicated request. Runner behavior remains intentionally unchanged: present null/non-string display values are normalized to authoritative empty text before extraction, and the extractor documentation plus runner-level regressions now make the direct/restored-context fallback boundary explicit.\n\nThe test refactor preserves the explicit Spanish target, Simplified Chinese, Traditional Chinese, short request, mixed-language, Unicode, and special-character cases with diagnostic IDs. Serialization assertions now anchor at the exact JSON field boundary, schema assertions verify exact field placement and complete guidance, and both defective DAG instruction assertions now require the real request in system context but not in step text. Shared setup and soft-policy prose were consolidated without merging the distinct root, structured, empty, step, or pinned behaviors. The 32-test targeted suite, 29 language tests, 13 context-language tests, 8 relevant Auto/ReAct/DAG/runner siblings, isort, Ruff check/format, py_compile, and diff checks pass. Separate F1, F2, and F3 mutations each made its regression fail before restoration. Local pytest-cov support is unavailable in the validated environment, so no local coverage percentage is claimed.

@OliverBryant
OliverBryant force-pushed the codex/fix-request-language-consistency branch from c785684 to 92f9611 Compare September 1, 2026 09:56

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR separates response-language guidance from connector/file-enriched execution text across root, Auto, ReAct, DAG planning/steps/completion, and final-answer schemas, while retaining request_context.output_language as the only caller-controlled hard authority. It introduces shared request-only guidance and carries the display/request boundary through fresh and resumed contexts. The approach is directionally sound, but this re-review confirms three major PR-caused blockers: a blank display loses a DAG child language anchor, a resumed clarification answer becomes completion-language authority, and the root harness adds unbounded request-sized duplication.
Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

Since the previous review, the rebased five-commit sequence is b6402e07 (isolate response language from connector context), c67b55e9 (apply test isort formatting), 37cde4fa (update language-prompt assertions), 47e1257d (harden request-language boundaries), and 92f9611d (close remaining language-prompt review gaps). This review is against the exact range BASE=31a2bb27b1d5174d142b5d8089ef9286eee6e81e through HEAD=92f9611db01499b7327dc9415ae003581233d96f (13 files, +590/-80); the inline anchors below were recomputed for this range.

Round 0 design verdict

Acceptable with reservations. The normalized hard authority, shared renderer, structured-field references, and absence of a persisted heuristic language label fit the stated problem. The design still needed an explicit presentation-versus-authorship boundary, correct classification of resumed DAG clarification answers, and finite-budget ownership for full request text. The synthetic file-only presentation-placeholder concern was independently dropped: those placeholders predate this PR and no deterministic wrong-language regression was established. The resumed-DAG selector and unbudgeted duplication remain confirmed, and Round 1 additionally found the concrete blank-display DAG-child anchor regression below.

Prior-history checklist

All duplicate occurrences and replies are consolidated under one canonical root; no historical root is reopened.

  • FIXED — Blank/whitespace display fallback.
  • FIXED — Structured plan/completion policy re-embedded request.
  • FIXED — DAG-step empty harness / missing step anchor.
  • FIXED — Missing-display root request duplication.
  • FIXED — Dangling default final-answer guidance.
  • FIXED — Doubled the the structured policy.
  • FIXED — Pinned completion dead field.
  • DROPPED — Runner normalization/documentation reachability concern; the relevant code is pre-existing and unchanged in this range.
  • FIXED — Parameterized harness test-invariant redundancy.
  • FIXED — Weak schema test assertions.
  • FIXED — Weakened typed-quote assertion.

Current blocking findings

1. Present blank display suppresses the DAG child language anchor — src/xagent/core/agent/context/execution.py:539 — [major][blocking]

Trigger: An unpinned, supported context has a nonempty execution request containing connector/file enrichment and a present display_message equal to "" or whitespace; a DAG child is then created or restored. Evidence: The tri-state display helper returns a present blank string, and the new branch at execution.py:539 accepts it as authoritative. The child system-context builder passes that value to the DAG request-anchor dispatcher, which still omits the request-only harness when the request is empty. Provider-facing message serialization excludes message metadata, so the child is left with generic current-request guidance while the enriched connector/file text remains visible. Impact: A supported DAG step can use auxiliary connector/attachment text as language evidence and emit user-facing prose or persisted tool arguments in the wrong language, diverging from the root/planner/completion contract. Fix: Preserve the present-empty state with a presence-aware section or equivalent and emit explicit empty-request language guidance for DAG children, without falling back to enriched content; add fresh and restored blank/whitespace child regressions.

2. DAG completion treats a waiting clarification answer as language authority — src/xagent/core/agent/pattern/dag/dag.py:1506 — [major][blocking]

Trigger: An unpinned DAG starts with a substantive request in one language, a child pauses through ask_user/send_message(expect_response=True), and the user supplies substantive clarification in another language without requesting a language change; the run resumes and completes. Evidence: The added selector uses unfiltered latest_user_text(context, prefer_display=True), which reverse-scans user messages without excluding response_to_waiting_for_user or DAG scaffolding. The actual forwarding path marks only the child copy; the root answer seen by completion remains unmarked. When no hard language is pinned, the new user_authored_language_request field therefore contains the clarification answer, and its field-only policy makes that value the sole soft language authority; completion strips metadata and has no downstream language validator to mitigate it. Impact: Final DAG synthesis can switch to the clarification answer's language even though the original request established the language and the answer did not request translation. Fix: Preserve pending-question classification on forwarded root answers (including checkpoint-restored resumes) and use the marker-aware top-level request selector for completion and replanning; add live and restored cross-language wait/resume regressions.

3. Root request-only harness duplicates the full request without a finite budget — src/xagent/core/agent/language.py:496 — [major][blocking]

Trigger: Send a nonempty request through a supported WebSocket or Slack file/connector turn with no pinned output language, so the display request is R while the execution request is R+F. Evidence: The added json.dumps(request, ensure_ascii=False) always serializes the complete request with no bound or deduplication. The root system context already renders the enriched execution request, and the user message carries that same enriched request; the new harness adds another full R. Normal compaction truncates visible messages only, does not budget generated system context, and ordinary Auto/ReAct calls do not supply a max-token limit. Impact: A finite request can fit the BASE prompt but exceed the provider context in HEAD, causing a context-length failure before the answer or forcing loss of required context. Fix: Keep language isolation while carrying one canonical full request/reference and account for it in the actual prompt budget, separating enrichment where necessary; do not silently truncate the request because language-bearing instructions may occur mid-request.

Limitations

No local tests, linters, formatters, dependency installs, or runtime validation were run. All 15 reported CI checks completed successfully at the final preflight. The history extractor and simplification scans were unavailable (usage_limit_reached); prior history was manually reconstructed from the complete review, conversation, and inline-comment exports. No linked issues were supplied.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

  • src/xagent/core/agent/context/execution.py:539, major — present blank display suppresses the DAG child request-only language anchor and permits enriched auxiliary text to bias user-visible language. [new]
  • src/xagent/core/agent/pattern/dag/dag.py:1506, major — an unfiltered resumed clarification answer can become the sole completion-language request and switch final synthesis language. [new]
  • src/xagent/core/agent/language.py:496, major — an unbounded duplicate request can push a previously fitting enriched turn over the finite provider context limit. [new]

Each listed root has a supported trigger, concrete impact, and current-code evidence of PR causation against the exact BASE/HEAD range. The fixed and dropped historical roots do not contribute to the blocking decision.

Comment thread src/xagent/core/agent/context/execution.py Outdated
Comment thread src/xagent/core/agent/pattern/dag/dag.py Outdated
Comment thread src/xagent/core/agent/language.py
@OliverBryant

Copy link
Copy Markdown
Contributor Author

Implemented the round-three root-cause refactor in 0097114.

  • Added a durable tri-state request provenance model and one marker-aware top-level request selector shared by root, DAG planning, and completion assessment.
  • Preserved authoritative empty display text in fresh and restored DAG children.
  • Marked live and restored wait/resume answers without allowing an incidental cross-language clarification to replace the original request; explicit language-switch instructions remain visible and honored by policy.
  • Canonicalized root provider rendering so enriched execution content and clean display guidance each have one intended representation, and included dynamic system content in token accounting without truncating requests.
  • Audited every output_language_directives call site and the Auto/ReAct final-answer schema paths; no additional blocker was found.

Validation:

  • 189 DAG, Auto, request-language, and output-language tests passed.
  • 102 execution-context tests passed.
  • Runner null/non-string display normalization passed for both cases.
  • Ruff check/format, py_compile, and git diff --check passed.
  • Independent mutations for empty DAG anchors, wait-response selection, root copy cardinality, and dynamic-system budgeting all failed their corresponding regressions before restoration.

The branch remains mergeable with current upstream main, and all three new review threads have been replied to and resolved.

@rogercloud

Copy link
Copy Markdown
Collaborator

ci is failing

@OliverBryant
OliverBryant requested review from rogercloud and removed request for rogercloud September 2, 2026 06:24

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR separates the user-authored display_message from connector/file-enriched execution content when deriving response language, so auxiliary metadata cannot silently dictate user-facing prose. It introduces request-only and structured language guidance and propagates that boundary through root context, Auto/ReAct schemas, DAG planning/steps/completion, checkpoint restoration, and pending-question handling, while retaining request_context.output_language as the hard authority. The direction fits the problem, but the unpinned policy composition and request-anchor lifecycle still leave correctness and finite-context failures.
Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

Since the previous review, 0097114f fix(agent): preserve request language provenance adds a durable tri-state TopLevelUserRequest, shares marker-aware independent-request selection across root/planning/completion, marks waiting answers in root and child contexts, preserves blank-display provenance through fresh/restored DAG children, and updates rendered-system token accounting plus related regressions. f17439c6 test(agent): align restored prompt assertions only updates tests/core/agent/test_runner.py assertions for restored/non-string display metadata to match that provenance model; it does not change production behavior.

Approach verdict

The execution/display split, tri-state provenance, marker-aware top-level selection, and pinned-language short-circuit are the right abstractions for the stated problem. The implementation remains problematic because the new soft guidance is composed with an explicit-only legacy rule, compaction can delete the message that a child anchor references, and root rendering still carries an unbounded request-sized copy. The independently confirmed blockers below are separate roots, with the two policy-conflict roots sharing one changed helper line but having different triggers and impacts.

Prior-findings checklist

The Spark history extractor failed with usage_limit_reached, so occurrences and replies were manually reconstructed and grouped from the complete review-body, conversation, and inline-comment exports. Each row below is one canonical root; duplicate body entries, inline roots, replies, and later checklist references are attached to that row rather than reported again.

  • FIXED — H1: blank/present-whitespace display fell back to connector-enriched execution content (5075344906; inline 3901920474 → reply 3902014024; checklist 5076949073).
  • FIXED — H2: structured plan/completion policy re-embedded the request (5075344906; inline 3901928681 → reply 3902015106; checklist 5076949073).
  • FIXED — H3: DAG-step instruction emitted an empty request-only harness and lost its concrete anchor (body-only 5075717034 F1/T4; author context 5491608668; checklist 5076949073).
  • FIXED — H4: the missing-display root path duplicated the request (5075717034; inline 3902206322 → reply 3902534492; checklist 5076949073).
  • FIXED — H5: default final-answer guidance dangled when the root language block was absent (5075717034; inline 3902206340 → reply 3902535097; checklist 5076949073).
  • FIXED — H6: structured policy rendered the doubled article the the (5075717034; inline 3902206347 → reply 3902535573; checklist 5076949073).
  • FIXED — H7: pinned completion payload retained the dead user_authored_language_request field (5075717034; inline 3902206364 → reply 3902536135; checklist 5076949073).
  • DROPPED — H8: runner normalization/documentation reachability concern; the cited runner behavior is pre-existing and unchanged, and no concrete product impact was established (5075717034 F6; checklist 5076949073).
  • FIXED — H9: parameterized harness cases repeated invariant-only assertions and implied false breadth (body-only 5075717034 T1; author context 5491608668; checklist 5076949073).
  • FIXED — H10: final-answer schema assertions were too weakly tied to field placement (body-only 5075717034 T2; author context 5491608668; checklist 5076949073).
  • FIXED — H11: typed DAG-step quote assertion was weakened to a substring check (body-only 5075717034 T3; author context 5491608668; checklist 5076949073).
  • FIXED — N1: a present blank display suppressed the DAG-child request-only anchor; the current re-report is dropped because the tri-state anchor fix covers fresh and restored children (3903243821 → reply 3910364957; blocker body 5076949073; author context 5503750375).
  • FIXED — N2: a waiting clarification answer became the completion-language authority; marker propagation and selection now fix the historical root (3903244488 → reply 3910365051; blocker body 5076949073; author context 5503750375).
  • NOT FIXED — N3 [prior]: the root request-only harness still carries an unbounded request-sized duplication/context risk (3903245440 → reply 3910365107; blocker body 5076949073; Round 1 R1-4 is the same root). It remains a current finding below.

Informational conversation history, not findings: Gemini quota warning 5488913970; ci is failing notes 5489203088 and 5503828366; author implementation reports 5491608668 and 5503750375. Any validation statements in prior reviews or these comments are historical claims only.

Current findings

1. [prior] Root request-only harness retains unbounded request-sized duplication/context risk

Location: src/xagent/core/agent/language.py:496
Severity: major
Blocking: yes

Reachable trigger: An ordinary supported WebSocket turn is unpinned, has a non-empty display request R, and sends execution content E that is either equal to R or contains R plus connector/file enrichment. Choose a finite configured provider context window and a request near its boundary; the receive path has no application message-size admission bound.

Current behavior and impact: request_only_language_harness() unconditionally inserts json.dumps(request, ensure_ascii=False) into generated system content at this line, while get_messages_for_llm() still sends the execution message containing E. The equal-display path therefore carries R in both the harness and the user message, and an enriched path still carries R in the harness plus in E; the harness also adds fixed prose overhead. Ordinary Auto/ReAct calls do not provide an input budget, and compaction truncates stored messages rather than this generated system block or the current user message. A request that fit the base prompt can consequently cross the provider's finite input window in HEAD and fail before answering, reducing service availability and room for tools/output.

PR causation/base comparison: The base equal-display path already had two copies of E (raw root guidance plus the user message), so the old claim of a newly added third copy is not uniform after the refactor. The PR nevertheless materially worsens that supported boundary by replacing the shorter raw root block with this full request quote and fixed harness text (about 1008 additional characters in the verified equal-display shape), while adding no provider-admission or current-request truncation mechanism. The new rendered-system estimate is used for compaction/estimation, not as a provider-call fit check.

I saw the author's replies 3910365107 and 5503750375 claiming that canonical copy counts and dynamic accounting fixed this root. Current code confirms removal of the old raw line, but it still serializes the complete display request here alongside the execution message and does not enforce the selected provider's input budget; this is the same underlying root, not a new issue.

Fix: Keep language isolation while carrying one canonical request/reference at the provider boundary, and enforce the selected model's total input/output budget before ordinary Auto/ReAct calls. Preserve complete language-bearing input rather than silently truncating it; add a boundary case that fits BASE but is rejected by the current duplicated representation.

2. [new] Unpinned soft policy contradicts implicit target-language intent

Location: src/xagent/core/agent/language.py:518
Severity: major
Blocking: yes

Reachable trigger: An unpinned supported request can imply a target language without naming it, for example Rewrite this announcement so our Shanghai colleagues can read it easily. The planner contract treats this as an implicit cross-language request; no type or caller contract requires request_context.output_language for it.

Current behavior and impact: _soft_request_language_guidance() first says to honor explicit and implicit translation/rewrite/answer requests (language.py:506-512), but this new append invokes response_language_rules(subject=subject). That rule says to use the subject's natural language and to change language only when the subject explicitly asks (language.py:470-480). The same explicit-only wording also remains in the unpinned DAG scope policy (language.py:414-420) and planner response-language description (src/xagent/core/agent/pattern/dag/plan_generator.py:482-491). A model following the latter authority can produce an English plan/final answer for the Shanghai-audience request; the planner's checks validate labels/script mismatch, not the semantic audience intent, so the wrong-language plan can be accepted and propagate through steps and completion.

PR causation/base comparison: The explicit-only rule existed in BASE, but BASE did not compose it with the new implicit-support guidance or claim this combined unpinned contract. The PR adds _soft_request_language_guidance, routes root/structured/anchor surfaces through it, and adds explicit-and-implicit wording to final-answer guidance while leaving the incompatible rule in place. This contradictory composite is therefore introduced/materially worsened by the PR.

Fix: Define one canonical unpinned policy that covers both explicit and implicit target-language intent, then remove or rewrite the explicit-only denial from every unpinned root, DAG-scope, planner, structured, Auto, ReAct, and completion surface. Preserve hard request_context.output_language precedence and assert the complete rendered policies cannot contain both authorities.

3. [new] Unpinned soft policy contradicts the explicit language-switch exception for marked pending answers

Location: src/xagent/core/agent/language.py:518
Severity: major
Blocking: yes

Reachable trigger: An unpinned request starts in one language, a supported ReAct or DAG interaction pauses for a question, and the user answers with an explicit switch such as Continúa la respuesta en español.. The resumed answer is marked response_to_waiting_for_user; this is distinct from an ordinary new request and is intentionally retained as conversational context.

Current behavior and impact: The new guidance says that a pending-question answer does not replace the independent request unless that answer explicitly asks to translate, rewrite, or continue in another language (language.py:510-512). The appended response_language_rules at this line instead says earlier turns cannot change the response language unless the independent subject itself explicitly asks (language.py:470-480). Because top_level_user_request() deliberately keeps the original request as the policy subject while the marked answer remains visible, one rendered prompt authorizes both the explicit Spanish switch and the original language. Auto/ReAct, DAG steps, planning, or completion can follow the denial and produce final or step prose in the wrong language, violating the newly stated pending-answer exception.

PR causation/base comparison: BASE had the generic explicit-only rule but no pending-answer exception. The PR adds that exception and marker-aware wait/resume propagation, then appends the unchanged generic denial at this line, leaving the supported explicit-switch path contradictory. The marker and resumed interaction make the trigger reachable; no downstream semantic language validator resolves the conflict.

Fix: Render one marker-aware unpinned policy: retain the independent request as the baseline, allow only a marked pending answer that explicitly requests a translation/rewrite/continuation to override it, and remove the incompatible generic denial. Assert the complete root/plan/completion/DAG policies contain the exception without a later clause that negates it.

4. [new] Compaction can remove the independent request referenced by the DAG language policy

Location: src/xagent/core/agent/context/execution.py:623
Severity: major
Blocking: yes

Reachable trigger: A supported unpinned DAG child accumulates a long/tool-heavy step and crosses either summary or truncate compaction. The original independent request is older than the DAG dependency/step messages; compaction may retain a summary and latest visible user message or only a tail, while top_level_user_request() skips messages marked with dag_step_id or response_to_waiting_for_user.

Current behavior and impact: After those messages are removed, the selector falls back to task metadata with display_state="missing". This branch passes None to dag_step_request_anchor, which emits only a reference to the latest independent user message even though that message no longer exists. The child consequently has no preserved clean display-language source; summary text, step/scaffolding text, dependency results, or connector-enriched content can steer child response language and user-facing tool arguments, artifacts, and step results. A later root completion assessment may recover the request boundary, but it cannot repair those already-created child outputs.

PR causation/base comparison: Compaction's ability to drop old messages predates the PR, but BASE's DAG anchor passed _current_user_request_text(prefer_display=True), so the task fallback still materialized the original request after the message was dropped. The PR introduces the display-state distinction and this request=None branch, intentionally suppressing that fallback without persisting a canonical TopLevelUserRequest through compaction or checkpoint restoration; the new rendered-system accounting also makes the threshold easier to reach. This is a PR-caused regression in the new language-isolation contract, not merely pre-existing compaction behavior.

Fix: Persist the canonical independent language_text and display state through both compaction strategies, DAG child serialization/restoration, and waiting continuations, or retain the independent request message whenever the anchor references it. Do not rely on a lossy summary or a pointer to a message that may have been removed; cover fresh, restored, truncate, and summary paths.

Review limitations

No tests, builds, lint, formatters, dependency installs, or runtime checks were run locally for this follow-up. Any test, CI, or validation result in prior reviews or author comments is a historical claim only and is not evidence from this review. Spark history extraction failed with usage_limit_reached; the canonical history above was manually reconstructed from the complete raw exports. The Simplification Lens was unavailable due usage_limit_reached, so no Simplification opportunities section is included. No linked issues were supplied.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

  • src/xagent/core/agent/language.py:496, major — the full display request is carried again in generated system text and can push a previously fitting supported turn over the finite provider context limit. [prior]
  • src/xagent/core/agent/language.py:518, major — contradictory unpinned rules can reject an implicit target-language intent and produce the wrong plan/final language. [new]
  • src/xagent/core/agent/language.py:518, major — contradictory unpinned rules can ignore an explicit language switch in a marked pending answer and produce wrong-language output. [new]
  • src/xagent/core/agent/context/execution.py:623, major — compaction can remove the request referenced by the child anchor, allowing scaffold/summary/connector language to steer child outputs and artifacts. [new]

Comment thread src/xagent/core/agent/language.py
Comment thread src/xagent/core/agent/language.py Outdated
Comment thread src/xagent/core/agent/context/execution.py
@OliverBryant

Copy link
Copy Markdown
Contributor Author

Round 4 is complete in 0032c18.

Fixed:

  • Unified every unpinned response-language surface on one marker-aware policy that honors explicit and implicit target intent while preserving request_context.output_language as the sole hard authority.
  • Allowed an explicitly marked pending-question answer to change language only when it explicitly requests translation, rewriting, or continuation in another language.
  • Persisted execution text, clean language text, and display tri-state before child creation and both compaction paths, with live-request refresh and checkpoint restoration.
  • Audited and aligned root, structured payload, DAG scope/planner/retry, Auto, ReAct, completion, final-answer, and create-agent persisted-prose guidance.

Declined as a PR blocker:

  • The root prompt does not add an unbounded request duplicate relative to the merge base. Both versions carry two copies; the measured HEAD delta is a fixed 1,008 characters for a 10,000-character sentinel. The broader provider input-plus-output admission concern is tracked in fix: enforce provider input and output context budgets #2041.

Validation:

  • 50 focused request-policy/provenance tests passed, including the 18-case fresh/summary/truncate and live/restored display-tri-state matrix.
  • The broader language seam and directly related Auto, DAG, ReAct, context, runner, and create-agent selections passed during this round.
  • Ruff check, Ruff format check, py_compile, and git diff --check passed.
  • Four independent mutations proved failure detection for the explicit-only contradiction, marked-answer override, summary provenance, and truncate/restore provenance.
  • A three-way merge-tree check against current origin/main completed without conflicts.

All three current review threads have been replied to and resolved.

@rogercloud

Copy link
Copy Markdown
Collaborator

ci is failing

@OliverBryant
OliverBryant requested review from rogercloud and removed request for rogercloud September 2, 2026 09:31

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR summary

This PR isolates the user-visible request from connector- and file-enriched execution content when deriving response language, while keeping request_context.output_language as the sole caller-controlled hard authority. It adds tri-state TopLevelUserRequest provenance and marker-aware guidance across root prompts, Auto/ReAct schemas, DAG planning, steps and completion, compaction/checkpoint restoration, and pending-question handling. It also updates the prompt-contract tests and related language assertions for those surfaces.

Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

Since the last review commit f17439c6, exactly three commits were added. 0032c18e persists execution text, clean language text, and display state before child creation and both compaction paths, and consolidates unpinned surfaces on one marker-aware policy; 9134be2e changes codespell test text; c1a68f6 updates web instruction-language assertions. These updates preserve the earlier fixes and remove the policy contradictions, but they do not migrate pre-PR compacted DAG children or retain the pending question in custom planner/completion payloads.

Round 0 approach verdict

Sound; no macro design finding admitted. The provenance model, hard-versus-soft authority split, and centralized policy fit the stated language-isolation problem and cover the distinct model-generation boundaries. The remaining issues are local compatibility and structured-payload contract failures, not objections to the overall approach.

Prior-findings checklist

The history extractor and both Simplification Lens attempts were unavailable with usage_limit_reached; the complete review-body, conversation, and inline exports were manually reconstructed and inventory-checked. Each row below is one canonical root and retains its source, reply/context, and duplicate checklist occurrences.

Root Status Source, reply/context, duplicate occurrences, and verified disposition
H1 — blank/present-whitespace display fallback FIXED Review 5075344906; inline 3901920474 → reply 3902014024; checklist duplicates 5076949073 and 5087160527. Present blank/whitespace display now remains an authoritative empty value instead of falling back to enriched content.
H2 — structured plan/completion request re-embedding FIXED Review 5075344906; inline 3901928681 → reply 3902015106; checklist duplicates 5076949073 and 5087160527; later status/context 5075717034 and 5491608668. Structured policies now reference payload fields without re-embedding their values.
H3 — DAG-step empty harness / missing concrete anchor FIXED Body-only review 5075717034 F1/T4; author context 5491608668; checklist duplicates 5076949073 and 5087160527. DAG-step guidance now points to the child’s concrete request anchor instead of emitting an empty request harness.
H4 — missing-display root request duplication FIXED Review 5075717034 F2; inline 3902206322 → reply 3902534492; checklist duplicates 5076949073 and 5087160527. The missing-display root uses fixed, non-quoting existing-request guidance.
H5 — dangling default final-answer guidance FIXED Review 5075717034 F3; inline 3902206340 → reply 3902535097; checklist duplicates 5076949073 and 5087160527. Default final-answer callers now carry a self-contained fallback when root guidance is absent.
H6 — doubled the the structured policy FIXED Review 5075717034 F4; inline 3902206347 → reply 3902535573; checklist duplicates 5076949073 and 5087160527. Structured policy rendering no longer doubles the article.
H7 — pinned completion dead field FIXED Review 5075717034 F5; inline 3902206364 → reply 3902536135; checklist duplicates 5076949073 and 5087160527. Pinned completion payloads now omit the unreferenced user_authored_language_request field.
H8 — runner normalization/documentation reachability DROPPED Review 5075717034 F6; author context 5491608668; checklist duplicates 5076949073 and 5087160527. Runner normalization is pre-existing and unchanged, and no concrete product impact was established.
H9 — invariant-only parameterized harness tests FIXED Body-only review 5075717034 T1; author context 5491608668; checklist duplicates 5076949073 and 5087160527. Invariant assertions are separated from the value-dependent parameterized assertion.
H10 — weak final-answer schema assertions FIXED Body-only review 5075717034 T2; author context 5491608668; checklist duplicates 5076949073 and 5087160527. Tests now assert the complete rule in the exact nested answer fields.
H11 — weakened typed-quote assertion FIXED Body-only review 5075717034 T3; author context 5491608668; checklist duplicates 5076949073 and 5087160527. The JSON-aware positional assertion has been restored.
N1 — blank display suppresses the DAG-child anchor FIXED Review 5076949073 blocker 1; inline 3903243821 → reply 3910364957; author context 5503750375; checklist duplicate 5087160527. The present-empty anchor is now emitted for fresh and restored children.
N2 — waiting clarification answer becomes completion authority FIXED Review 5076949073 blocker 2; inline 3903244488 → reply 3910365051; author context 5503750375; checklist duplicate 5087160527. Marker-aware selection now keeps the independent request as the baseline.
N3 — unbounded root request duplication/provider admission DROPPED — tracked, not reported Review 5076949073 blocker 3; inline 3903245440 → reply 3910365107; prior/current duplicate review 5087160527; later inline 3912040087 → reply 3912540015; author tracking conversation 5507258387. The author explicitly declined this as a PR blocker and opened #2041; its body defines general provider input/output admission as separate from PR #1990 and has zero comments. N3 is not counted as fixed or as a current blocker.

No prior root survives into the current findings. N3 remains dropped under the explicit tracking disposition above and is not re-reported.

Current findings

R1-2 — Legacy compacted DAG-child checkpoint loses provenance after resume

Location: src/xagent/core/agent/context/enrichment.py:232
Severity: major
Blocking: yes

Reachable trigger and contract: A supported, unpinned connector/file turn can start before this PR with a clean display_message and enriched execution text, create a DAG child, and compact that child until the copied root user message is removed. The active child can then be checkpointed and resumed after deployment through ExecutionContext.from_dict and DAG restoration. Such a legacy payload has no _xagent_top_level_user_request; the checkpoint format remains compatible with missing optional metadata, and the child’s remaining DAG-scoped/scaffolding messages are intentionally excluded by the selector. No caller-provided output-language pin is required for this supported path.

Current behavior and concrete impact: With no independent message and no stored provenance snapshot, the fallback at line 232 reads context.metadata["task"]. For this trigger that value is the enriched execution prompt, so the selector returns it as language_text with display_state="missing" and persists the polluted value as the new snapshot. The child system-context path at src/xagent/core/agent/context/execution.py:619-629 consequently passes no request text to the DAG anchor, which only points to the latest independent user message even though compaction already removed it. The resumed child therefore has no clean display-language source; summary/scaffold/step/dependency or connector text can steer user-facing step prose, persisted tool arguments, and artifacts. The verifying worker’s narrow restore reproduction observed both the polluted snapshot and the pointer-only child system context.

Contract/invariant analysis: Compaction is deliberately lossy, and restored root and child metadata are separate dictionaries; a clean request still available in the root context is not implicitly visible in a legacy child. Snapshots taken by current code before current child creation or current compaction cannot repair a child checkpoint that was already compacted by the pre-PR code. The supported legacy restore path therefore requires an explicit migration rather than assuming the new key exists.

PR causation: The base payloads had no provenance key, and the base child path used the existing task/request fallback when rendering its DAG anchor. This PR introduced the new provenance key and the display_state="missing"/request=None behavior but added no migration for old active children. It thereby leaves a supported pre-PR checkpoint without the clean request boundary that the new implementation promises; this is a PR-caused compatibility regression, not merely pre-existing compaction loss.

Specific fix: At active-child restoration, including the waiting-step restore path, derive the canonical clean TopLevelUserRequest from root_context and hydrate/persist it into a legacy child before any selector or system-context rendering. Preserve a valid child snapshot and never replace it with the enriched task fallback. Add normal and cold-restore coverage using a pre-PR serialized child with no provenance key, its copied root message removed by summary/truncate compaction, only DAG-scoped messages remaining, an enriched task, and a clean root display request.

R1-3 — Marked pending-answer serializer drops the question context

Location: src/xagent/core/agent/context/enrichment.py:260
Severity: major
Blocking: yes

Reachable trigger and contract: During a supported DAG interaction, ask_user_question or send_message(expect_response=True) can ask an arbitrary question such as “Which language should the email use?”, and the user can answer tersely with an unambiguous label such as Spanish or 繁體中文. Forwarding stores response_to_waiting_for_user.question and message_type on both the root answer and the forwarded child copy (src/xagent/core/agent/pattern/dag/dag.py:1904-1926); no contract requires the question to be English or excludes a language-selection question. The answer can reach planner re-generation or completion without a caller-pinned output language.

Current behavior and concrete impact: language_prompt_message() adds only role, content, and the generic user_message_context="pending_agent_question_response" at line 260; it drops the pending question and its message type. Both the planner payload (plan_generator.py:571-589) and completion payload (dag.py:1510-1533) use this custom serializer, while the normal provider renderer preserves the semantic context as Pending question: ... / User answer: ... (execution.py:432-443). The structured model therefore sees {content: "Spanish", user_message_context: "pending_agent_question_response"} without knowing what was asked. The shared policy permits a marked answer to override the independent-request baseline only when the answer itself explicitly asks to translate, rewrite, or continue; a one-word answer cannot satisfy that wording even though the pending question makes it an explicit language selection. Planner/final-answer prose can consequently remain in the original language or otherwise be generated without the user’s requested target language, producing a wrong-language user-visible result.

Contract/invariant analysis: The existing normal renderer demonstrates that response_to_waiting_for_user.question is required semantic context, not marker-only bookkeeping. The question schema accepts arbitrary text, and no downstream semantic validator reconstructs the dropped question: planner validation checks labels/script mismatch, while completion consumes the custom payload. The top-level selector correctly excludes the marked answer, so this is distinct from fixed root N2; the very exclusion that protects the baseline makes preserving the question necessary for a terse answer to remain interpretable.

PR causation: The base planner/completion serializers emitted raw role/content and did not introduce this marker-aware language exception. This PR added the root/child marker propagation and language_prompt_message() but retained only a generic marker in the structured payloads, dropping the context needed by the new policy to interpret question-dependent answers. The missing-question serializer path and resulting wrong-language behavior are therefore PR-caused.

Specific fix: Carry bounded structured pending-question context (at least the question and message_type, and any needed options) alongside the generic marker in both planner and completion payloads. Extend the shared policy so a marked answer may override the baseline when its pending question explicitly asks for output language and the answer is an unambiguous supported language label, while retaining the N2 protection for non-language questions. Add planner and completion cases for a terse language-selection answer plus a control such as Spanish answering Which city?.

Review limitations

  • History extraction and both Simplification Lens attempts failed with usage_limit_reached; manual extraction used the complete review-body, conversation, inline-comment, and linked-issue exports.
  • No issue is linked from the PR. The only tracking reference is author-opened #2041, whose body explicitly scopes general provider admission separately from PR #1990 and whose comment export is empty.
  • No local tests, builds, or lint ran in the main review. Narrow runtime checks run by workers were specific evidence for the named findings only, not project-wide validation.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

  • src/xagent/core/agent/context/enrichment.py:232major — a legacy compacted DAG child can lose clean request provenance on resume, leaving connector/scaffold text able to steer user-facing step output and artifacts. [new]
  • src/xagent/core/agent/context/enrichment.py:260major — marked terse language answers lose their pending question in planner/completion payloads and can produce wrong-language output. [new]

has_pending_response=has_pending_response,
)

task = (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major / blocking: A supported unpinned DAG can resume a pre-PR checkpoint after compaction removed the copied root message, leaving no _xagent_top_level_user_request; this fallback then promotes enriched metadata["task"] as language text while the child anchor points to a deleted independent message, so connector/scaffold text can steer user-facing step prose and artifacts. Please hydrate a valid snapshot from the restored root context before rendering any legacy child (preserving valid child snapshots), and add live/cold restore coverage.

and isinstance(metadata, dict)
and metadata.get("response_to_waiting_for_user")
):
payload["user_message_context"] = "pending_agent_question_response"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major / blocking: A supported DAG language-selection question can receive a terse answer such as Spanish; this serializer keeps only the generic marker and drops response_to_waiting_for_user.question, so planner/completion cannot interpret the answer and may emit final prose in the baseline language. Please carry bounded pending-question/message-type context in both structured payloads and let an unambiguous answer override only for a language-selection question, with a non-language control test.

has_pending_response=has_pending_response,
)

task = (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major / blocking: A supported unpinned DAG can resume a pre-PR checkpoint after compaction removed the copied root message, leaving no _xagent_top_level_user_request; this fallback then promotes enriched metadata["task"] as language text while the child anchor points to a deleted independent message, so connector/scaffold text can steer user-facing step prose and artifacts. Please hydrate a valid snapshot from the restored root context before rendering any legacy child (preserving valid child snapshots), and add live/cold restore coverage.

and isinstance(metadata, dict)
and metadata.get("response_to_waiting_for_user")
):
payload["user_message_context"] = "pending_agent_question_response"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major / blocking: A supported DAG language-selection question can receive a terse answer such as Spanish; this serializer keeps only the generic marker and drops response_to_waiting_for_user.question, so planner/completion cannot interpret the answer and may emit final prose in the baseline language. Please carry bounded pending-question/message-type context in both structured payloads and let an unambiguous answer override only for a language-selection question, with a non-language control test.

@OliverBryant

Copy link
Copy Markdown
Contributor Author

PR #1990 is now a draft umbrella and implementation reference for the staged rollout tracked in #2062. The aggregate branch will not be merged, and no further aggregate fixes will be pushed. Replacement pull requests will be created in order for #2065, #2063, and #2064, each from the latest upstream main after the preceding layer merges. Provider admission work remains separate in #2041.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants