fix(agent): recover delegation judgments and preserve execution ownership - #943
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of d5cf673523cff0b0fcf871d4a9303c39e56d0c92..ddaef6055ce07096421a7e1c3e321038aa806003, covering all four commits and the combined diff (22 files, +606/-96).
Recommendation: fix the P2 below before merging.
One actionable finding: the new Introspect error projection can completely hide recent execution failures after ten admission rejections have accumulated. The inline comment identifies the truncation point, trigger and regression coverage needed. This is a regression in the diagnostic view, not a claim that execution failures disappear from the underlying health tracker.
Other review conclusions:
- Invalid-response correction, configured fallback and primary deadline retry share the existing second-call allowance. The added branches do not introduce a third call within one judgment invocation, and cancellation, lease-loss and remaining-budget checks remain in place.
- Exhausted invalid initial candidate assessments become internal unavailability rather than user ambiguity. Repeated spawn attempts under the same authenticated intent reuse that failed assessment.
- Requiring explicit slot arrays aligns the prompt examples with the parser and removes implicit universal-scope expansion.
- The CLI foreground filter preserves child callback settlement. The updated serial/parallel tests check both foreground ownership and the run identities of the actual result callbacks.
- Output-format guidance remains a prompt contract, without a task-specific answer rewriter. The new live cases require actual child model inference and adoption of the child result.
- The implementation extends existing owners without a new execution path, registry, database query or transaction. The net +510 lines have concrete correctness and verification purposes. A malformed judgment can now incur one additional auxiliary inference; that cost is bounded.
Verification: git diff --check passed. At review time, CI lint/check and turn-core/services/plan had passed; runtime, CLI and other lanes were still running. No local Rust tests were executed because this environment has no Rust toolchain. Author-reported local and live-harness results were not independently rerun. The P2 follows directly from the iterator ordering and truncation in the production snapshot builder; the existing new test covers only one rejection with an empty execution-error history.
| .chain(state.turn_guard.health.recent_errors(10)) | ||
| .take(10) |
There was a problem hiding this comment.
[P2] Preserve execution errors when the rejection list reaches its cap
This appends execution failures after up to ten admission rejections and then truncates the combined list to ten. Once the current turn has accumulated ten Rejected records, even a later Bash timeout or file-operation failure is omitted from introspect(facet="errors"): health.recent_errors(10) contains the fresh failure, but none of its entries can survive this take(10). Subsequent successful calls do not remove the earlier rejection records, so the diagnostic view can keep showing stale admission problems while hiding the failure the agent needs to recover from.
Use a bounded merge that cannot let one category completely starve the other, and add a snapshot-level regression with ten earlier rejections followed by a new execution failure. Assert that the new failure remains visible while the projection stays bounded.
There was a problem hiding this comment.
Adopted in 4af170d. The shared Introspect snapshot alternates existing execution and admission error sources within the unchanged ten-entry cap, preserving source-local recency, redaction, and unknown admission timestamps. It reuses the health errors collected for alerts rather than scanning twice; no storage or database I/O added. Snapshot regression covers ten old rejections plus a fresh timeout, both categories saturated, and each category alone. Introspect 19/19 and runtime lib/tests Clippy pass; independent gpt-6-astra / xhigh review approved. Rebased onto fd40b74 and updated this PR.
ddaef60 to
4af170d
Compare
Summary
Fix named-model delegation recovery and observation ownership without adding a second execution path.
Related issue
No linked issue. Follow-up to observed named-model delegation failures.
Change type
User and compatibility impact
Natural named-model delegation continues through the existing authenticated candidate selector. Invalid selector responses no longer masquerade as user ambiguity. Child events no longer contaminate the foreground callback snapshot. No compatibility shim, migration, configuration change, model-name alias, task-specific matcher or output-rewriting filter is added.
Architecture and complexity delta
main(fd40b7402): 23 files, 705 insertions / 103 deletions. This is a focused correctness repair, not a claimed large-scale cleanup milestone.Verification
CI follow-up
Current head:
4af170d1e. Fresh remote CI for this head must still finish; local gates below do not claim all remote checks are green.Rebased onto
maincommitfd40b7402(#942), preserving its whole-request SSE API and command budget; no retired recovery/judgment path is restored. Updated the CLI budget fixture to admit the actual typed request. Post-rebase CLI callback, ownership and budget gates: 6/6 PASS.The subsequent remote run on
ddaef6055passed all jobs except one bridge E2E fixture. That fixture now supplies both invalid assessment responses permitted by bounded repair and checks the actualdelegation_model_assessment_unavailableerror. It retains zero children,executed: false, exact total request count, and exactly two shared assessment calls for the batch. The discovery-only fixture now explicitly denies skills, independently of the machine's local skill catalog. Post-rebase full Web agent E2E suite: 54/54 PASS (two existing ignored tests); these are deterministic provider-boundary tests, not a fresh paid-provider cohort.Adopted review comment
4178480735: a full admission-rejection list previously hid execution failures in Introspect. The shared snapshot now fairly merges both existing error sources within the same 10-entry cap, preserves each source's recency and redaction, and avoids a duplicate health scan. Regression coverage exercises a fresh timeout after ten rejections, both categories saturated, and each category alone. Runtime Introspect gate: 19/19 PASS. Runtime lib/tests Clippy (-D warnings): PASS, 3m56s; format/diff checks pass. No additional storage or database I/O. Independentgpt-6-astra/xhighreview approved this delta.The initial CI found three deterministic gaps across two jobs:
Follow-up commit:
ddaef6055. Independentgpt-6-astra/xhighreview approved the repair. Formatting and diff checks pass.cargo clippy -p astra-tools -p astra-test-harness --lib --tests -- -D warnings: PASS (1m29s).agent(wait)and runtime completion waiting while retaining queued-not-applied and exact reply-correlation assertions.cargo nextest run -p astra-tools -p astra-test-harness -p astra-runtime --lib -E 'test(schemas::tests::) | test(criteria::tests::) | test(messaging::e2e_loop_tests::) | test(tool_registry::surface)' --test-threads 2 --status-level fail --final-status-level fail: 277/277 PASS. Fresh CI after the follow-up push remains pending; this is not a claim that all remote checks are green.Prior live cohort and owning-boundary gates
Live-cohort candidate:
8d472a035df38ed93416149d7d1b5220bc80998b, based onmain(d5cf673523cff0b0fcf871d4a9303c39e56d0c92). Follow-up CI fixes shorten discovery summaries and update deterministic fixtures/assertions, without changing execution, selectors or output-format policy.cargo fmt --all -- --checkandgit diff --check: PASS.cargo nextest run -p astra-runtime --lib -E 'test(prompts::) | test(agent_fanout) | test(fanout_) | test(wait_) | test(parent_coordination) | test(tool_registry::surface)' --test-threads 1 --status-level fail --final-status-level fail: 343/343 PASS.cargo clippy -p astra-runtime -p astra-tools -p astra-turn-core --lib --tests -- -D warnings: PASS.gpt-6-astra/xhigh; no remaining blockers. Post-cohort evidence independently checked with the same configuration.First fixed-cohort outcomes, all criteria passed with no warnings:
flash_delegate_simple_glmflash_scoped_child_and_parentflash_child_compact_jsonflash_semantic_model_reference_glmflash_fanout_model_defaultget_resultsflash_spawn_prohibited_model_fail_closedEvidence limits
The earlier strict-integer failure is retained; a later pass does not prove guaranteed LLM compliance. Two cohort cases still performed avoidable discovery despite resident
agentavailability. Fanout improved from a prior single sample of 5 rounds / 12.928s to 4 rounds / 9.503s, but this is not a controlled latency distribution or general cost-saving claim. Logical usage coverage is complete in these reports; complete child cache coverage and historical billing reconciliation are not established. No full-workspace CI pass or configured-Jev live cohort is claimed. Reproduction instructions and case definitions are checked in; private traces, credentials and local artifacts are not.Final checklist