fix(hy3): repair collapsed tool calls and Guardian routing - #83
Draft
jatmn wants to merge 4 commits into
Draft
Conversation
jatmn
marked this pull request as ready for review
August 23, 2026 14:46
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/response_codec.rs" line_range="2453-2467" />
<code_context>
.and_then(Value::as_str)
.unwrap_or("{}");
let call_id = call.get("id").and_then(Value::as_str).unwrap_or("call");
- output.push(tool_call_item(
- name,
- arguments,
</code_context>
<issue_to_address>
**issue (bug_risk):** When a repaired tool call has no upstream `id`, the first emitted call uses the literal fallback ID `"call"`; any other call without an ID in the same response also uses `"call"`. This produces duplicate Responses call IDs instead of the stable, unique IDs promised by the repair path.
**Triggers:** When Hy3 omits tool-call IDs, or when multiple tool-call entries without IDs are present in one completion.
**Suggested fix:** Generate an ID for the first emitted call whenever `call_id` is missing, rather than preserving the literal `"call"` fallback; use the same generated-ID policy for every emitted call that lacks an upstream ID.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the repair misparses a Hy3 argument stream, it can turn one malformed tool call into multiple executable calls, including extra shell or other agent actions; those effects may already occur before a revert. The Hy3-only scope bounds the exposure, and reverting restores the previous routing and parsing behavior but cannot undo actions or review decisions already made.
Blocking findings: src/response_codec.rs:2467
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jatmn
marked this pull request as draft
August 23, 2026 16:38
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
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
Draft status: known issues / not merge-ready
This PR is intentionally a draft. Final-head review found the following unresolved branch-owned issues:
ChatAccum::finish(), and non-streaming conversion builds the complete expanded output array, so a bounded upstream response can still cause excessive CPU/memory use or terminate the proxy. The repair needs a response-scoped recovery/output budget shared by both paths.finish_reasonvalues includinglengthandcontent_filter. Two complete objects observed before truncation/filtering can be converted from one invalid call into multiple valid executable calls. Expansion must be limited to a successful tool-call completion state.The branch is also behind current
main, with overlapping model/configuration changes. GitHub currently reports the PR as conflicting. These issues must be fixed, currentmainintegrated, and the complete validation/review campaign rerun before the PR is marked ready.Investigation
A recent
concentrate.ai/hy3Codex session stopped after Codex rejected oneexec_commandcall with trailing JSON. Correlated Warp debug frames showed that the upstream stream reused tool-call index 0 while emitting two complete argument objects. Warp accumulated those fragments faithfully, producing one invalid argument string; the model then ended the turn. Replaying that malformed history can also trigger an upstream 400.Historical
codex-auto-reviewrouting failures occurred before the session-aware fallback from #58. Current clients should also receive a concrete Hy3auto_review_model_override; live-catalog aliases now localize to their exact discovered route instead of an unroutable barehy3name. Static-catalog collision handling remains an unresolved issue described above.The installed operator configuration inspected during diagnosis predates the repository's Hy3 family defaults. It will need a separate config refresh after this change merges; this PR does not modify a running deployment.
Implementation
The new model-family transform parses only a complete sequence of two or more top-level JSON objects. Single objects, partial JSON, arrays/scalars, and trailing junk are forwarded unchanged. Original trimmed object spans are retained so large integers, exponent spellings, and duplicate keys are not rewritten. Recovery falls back to the original single call above 64 recovered objects or 1 MiB of arguments per source call.
The same repair and ID rules apply to streaming and non-streaming Chat Completions conversion. Native Responses behavior and unrelated model families are unchanged.
Validation
bash scripts/ci-preflight.sh, repeated by commit and pre-push hooks on head3981e9d9b2c4a3b95f78b4fe8f850eff22054879Passing validation does not clear the known design/lifecycle defects above.
Research
hy_v3reasoning/tool parser and automatic tool choicetool_callsarrays and role-toolcontinuationsSummary by Sourcery
Repair Hy3 tool-call conversion and Guardian routing so repeated invocations remain valid and provider-routable.
Bug Fixes:
Enhancements:
Documentation:
Tests: