Repository navigation
fix(agent): keep typed hosted failures and honor transcript-autoload suppression; un-ignore harness e2e tests - #6879
Merged
Conversation
Add two end-to-end tests for streaming tool-call argument accumulation. The first test drives the engine and UI delta forwarding through a ScriptedProvider, asserting that the tool receives the full assembled argument set from ModelResponse.message.tool_calls while the progress channel carries the individual ToolCallArgsDelta fragments. The second test exercises the real provider HTTP and SSE-parse path against an in-test upstream, verifying that the provider's accumulation buffer correctly reassembles function.arguments fragments split across SSE chunks at awkward byte offsets. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ion test The `#[ignore]` attribute was removed from the `streaming_tool_call_accumulation` test function, allowing it to run as part of the normal test suite instead of requiring the `--ignored` flag. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Added eprintln! calls in the ScriptedProvider test helper to log the remaining response count and incoming message count during streaming tests, making it easier to diagnose test failures by observing the sequence of calls at runtime. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Remove debug print statements and conditionally replay stream events only when the response contains tool calls. Without this guard, the test harness would emit tool-call fragments ahead of a text-only final answer, which no real provider produces and which causes the agent loop to re-dispatch the call on every iteration until the repeat guard stops the turn. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ol call test The comment describing why the test filters to iteration 1 was misleading, as it suggested iteration 2 also emits the same delta sequence. The ScriptedProvider actually replays stream events only for the response carrying the tool call, so only iteration 1 emits deltas and the filter simply pins that iteration. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The `is_turn_timeout_error` function now also matches the `exceeded its per-model-call ceiling` phrase, so that a wedged model call that exhausts its retries is classified as a turn timeout rather than falling through to the generic inference bucket. A test case and the corresponding end-to-end test are updated to reflect this fix for issue tinyhumansai#6375. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The per-model-call ceiling error is now handled as a retryable `CallTimeout` rather than a turn timeout, so the anchor string and its associated test case have been removed to prevent misclassification. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
… use_skill The end-to-end test for skill install hand-offs was updated to reflect that `setup_skills` is now a member of the `skills` tool pack rather than a directly advertised tool. The helper function was renamed and extended to accept a pack parameter, and the assertions now check that the orchestrator offers the pack through `use_skill` instead of advertising the hand-off tool directly. The test function and its inner async counterpart were also renamed to match the new behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…_skill Update the orchestrator e2e test to reflect that since tinyhumansai#6787, `setup_skills` is a member of the `skills` pack and a packed hand-off no longer closes its pack to the orchestrator. The raw `skill_registry_install` is now reachable through `use_skill`, but guarded by the approval gate, so the test now denies the approval prompt instead of approving it and verifies the install is refused. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Rename the test that verifies the orchestrator defers its MCP registry tools without a hand-off, and update its doc comment to clarify that the tools are not advertised on the orchestrator's wire due to issue tinyhumansai#6787. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Replace the manual stream-draining logic with a call to `invoke_agent_streaming` and unify error handling for both streaming and non-streaming paths. Previously, the streaming branch consumed the agent stream item by item and collapsed all failures into a sanitized string, losing the original error kind. The new code keeps the typed `HostedError` through the turn error, allowing `web_errors` to classify timeouts, limits, and provider failures correctly. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Two end-to-end tests for turn-level overrides were previously ignored because the hosted root session did not carry tool-calling capability, causing the tests to fail on their own control assertions. A new `ScriptedModel::native_tools` constructor now creates a model profile that declares native tool calling, which lets the hosted harness send the toolbelt schema upstream. The `suppress_transcript_autoload` and `turn_overrides_reset` tests are un-ignored and updated to use this constructor, restoring coverage for the scenarios they were designed to validate. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…d-scoped session resume The test for `suppress_transcript_autoload` is updated to reflect that `turn()` now resumes by durable session identity rather than by agent name, making the lookup thread-scoped. The test is restructured into three clear assertions: a different thread never replays another thread's transcript, the same thread resumes its own transcript as a control, and the suppression override makes the same-thread turn start clean. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The baseline count of ignored tests was reduced from 22 to 17, reflecting a decrease in the number of tests that are expected to be skipped during CI runs. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test for per-call ceiling behaviour now accounts for the retry schedule introduced in tinyhumansai#6413, where a wedged call is retried up to five times with exponential backoff rather than ending immediately after the first ceiling cut. The assertion on elapsed time is relaxed from 8 seconds to 90 seconds, and a new assertion verifies that the upstream was called at least twice, confirming the ceiling triggered retries. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When a runtime session attempts to transition to a new state, the code now checks whether a session host is present before proceeding. This prevents a panic that occurred when the host was unexpectedly absent, ensuring graceful handling of edge cases in session lifecycle management. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When a runtime session is not found in the session host, the system now returns an appropriate error instead of panicking or proceeding with an invalid state. This ensures graceful failure and clearer diagnostics for missing session scenarios. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Replace the `transcript_autoload_suppressed` method with `turn_resume_mode` that returns a `ResumeMode` enum, enabling more nuanced control over how session history is resumed. This change addresses issue tinyhumansai#6377 where the previous boolean approach could not distinguish between suppressing autoload and other resume strategies needed for thread-bound sessions. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The line-count exception for the runtime_session.rs file in the OpenHuman Rust layout check was reduced from 1979 to 1966 to reflect the removal of generic session state code that was moved to the tinyagents-runtime crate, keeping the allowance exact for the remaining composition logic. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat several long string literals and function call arguments to stay within the project's line-length limit, and reorder `#[cfg(test)] mod` declarations in a handful of files so that the module attributes appear immediately before their corresponding module name. No behaviour changes are introduced. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted several multi-line string literals and error messages to fit on single lines, and reordered `#[cfg(test)]` module declarations to follow a consistent pattern. These changes are purely cosmetic with no behavioral impact. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper review
|
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughHosted turns now select transcript resume modes through a dedicated method and preserve typed invocation errors. End-to-end tests cover transcript overrides, streamed tool arguments, retry-aware timeouts, packed skill hand-offs, and MCP tool advertisement. ChangesHosted turns and harness behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changes preserve hosted error classifications and honor transcript suppression, with restored regression coverage. No actionable blocker remains; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens conversation-history suppression and preserves failure categories. No new security weakness was established, but interruption handling and shared-session persistence remain only partially verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR also changes behavior and tests that are not required by Resolution Remove the unrelated transcript-autoload and turn-override behavior and tests from this PR, or link them to an active issue with matching coding requirements. Move the skill-pack, registry-install, MCP, and orchestrator tool-pack updates to a separate scoped PR or provide an active linked issue that requires them. Full details: Docstring CoverageExplanation Docstring coverage is 43.40% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 6 files. (2 skipped: 2 unsupported.)
A rabbit watches arguments stream, Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root-causes the ignored and failing tests in
agent_harness_e2eandagent_turn_overrides_e2e. Two are real product bugs, one is a fixture that described a stream no provider produces, and the rest are tests that predate intended product changes.Closes #6375
Closes #6377
Root causes and fixes
streaming_tool_call_accumulation(Hosted TinyAgents stream regressions lose turn continuation and typed failures #6375): the 3x dispatch was the fixture, not the harness.ScriptedProvider.streamreplayed the sameToolCallDeltafragments before every response, including the text-only final answer. The hosted harness deliberately treats streamed tool fragments as authoritative and rebuilds the terminal response's tool calls from them (invoke_model_streaming_once,agent_loop/model_call.rs). So the final "stream final" response also dispatchedecho_tool: three identical successful batches, then the repeat guard stopped the turn. The fixture now streams fragments only for a response that carries tool calls. The test and its provider-level SSE sibling (provider_sse_tool_args_accumulation) had been dropped frommainby a merge resolution, so they are restored here, with no#[ignore].model_call_ceiling_...(Hosted TinyAgents stream regressions lose turn continuation and typed failures #6375): the hosted boundary dropped the failure kind. The host drainedinvoke_agent_stream, whose terminalFaileditem is sanitized to one string (hosted agent invocation failed) for every failure, then wrapped it asTinyAgentsError::Model. A per-call timeout was therefore indistinguishable from a provider error and classified asinference. The intended contract isturn_timeout(is_turn_timeout_error,timeout_bound_tag). The root fix is upstream, tinyagents#261: a publicinvoke_agent_streamingreturning the typedHostedError. The host now uses it for both surfaces and converts with the existingFrom<HostedError> for TinyAgentsError, so the kind survives. The test's< 8sbound is replaced because a per-call ceiling is now a retryableCallTimeoutthat the turn policy retries on its 5-attempt schedule (Upstream 429 on the selected model kills the turn after one retry — no backoff, no fallback, no user-visible retry #6413), so the turn ends after about 50s. It now pinsturn_timeout, at least 2 upstream calls, and< 90s.f6bffc219f, "defer rarely-used tools and pack setup_skills hand-off") deliberately movedsetup_skillsinto theskillspack, deferred the fourmcp_registry_*tools off the orchestrator's wire, and made a packed hand-off stop closing its pack to the orchestrator. Updated to that documented intent:orchestrator_hands_skill_installs_to_skill_setup_directlybecomes..._through_the_skills_pack: the hand-off is called asuse_skill {skills, setup_skills}.orchestrator_cannot_install_a_skill_through_the_raw_registry_toolbecomesorchestrator_raw_skill_install_through_use_skill_needs_approval: the guard is now the approval gate, and a denied install writes nothing.orchestrator_advertises_direct_mcp_toolsbecomesorchestrator_defers_its_mcp_registry_tools_without_a_hand_off.suppress_transcript_autoloadis a real product bug.turn()ran the thread-bound explicit resume (runtime.resume) before the lifecycle hook appliedbegin_turn_resume, so the override suppressed nothing for a thread-bound session. The flows builder and others pass a thread id together with this override. The resume mode is now decided before the explicit resume (OpenHumanSessionHost::turn_resume_mode). The test's old control (a different thread id replays another thread's transcript) described the removed latest-by-agent-name lookup. It now pins that another thread never gets the transcript, that the same thread resumes (the control), and that the override makes the same-thread turn start clean.turn_overrides_apply_to_exactly_one_turn_and_then_resetis a fixture issue.ModelProfile::default()has no native tool calling, so the hosted harness renders the belt into the prompt and sends an emptyrequest.tools. The test now usesScriptedModel::native_tools.Pin
vendor/tinyagents->4583fd645c2426a9484df429493c9d16dd8bc4fa(branchfix-hosted-stream-typed-failure, tinyagents#261, based on the previous pinb88a2728). Merge #261 first; the gitlink then needs a repoint to the merged main SHA.Tests
streaming_tool_call_accumulation(repeat-guard text instead of "stream final"),model_call_ceiling(error_type=inference), the three orchestrator tests,suppress_transcript_autoload...(prior marker present in the suppressed prompt), as run on this base.scripts/test-rust-with-mock.sh:--test agent_harness_e2e: 27 passed, 0 ignored--test agent_turn_overrides_e2e: 4 passed, 0 ignoredcargo test -p openhuman --lib --features <product>: 9033 passed, 0 failed, 39 ignored.cargo check --tests -p openhuman -p openhuman-cli --features <product>clean.cargo test -p tinyagents-harness --lib runtime::(47 passed) and clippy-D warningsclean.check-gated-test-allowlistandcheck-feature-forwarding: OK.check-submodule-monotonic: OK.check-agent-runtime-boundary: only the 2 pre-existing stale openhuman_backend_model baseline entries.rust:layout: only the pre-existing failures inall_tests.rsandopenhuman_backend_model*.rs. Theruntime_session.rspin is lowered to 1966.check-ignored-tests:testsbaseline 22 -> 17.openhuman-coreis 39 against a baseline of 38 on main already, which is not from this PR.Notes
--no-verify.per_model_callvsrun_remainingtag (timeout_bound_tag) can no longer see which bound fired and reportsunclassified_timeout. The user-facing class is correct. Restoring the tag needs the kind or bound carried onHostedError; it is not done here.Summary by CodeRabbit