Repository navigation
test(trace): bind replay results by exact tool call - #6659
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: docs
📝 WalkthroughWalkthroughAdds exact ChangesTrace-result binding and replay
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RecordingLlm
participant TraceLlm
participant MockLLM
participant ToolResults
RecordingLlm->>RecordingLlm: create $trace_result with tool_call_id and pointer
TraceLlm->>ToolResults: collect observed tool results
TraceLlm->>TraceLlm: resolve pointer bindings
MockLLM->>ToolResults: map runtime IDs to recorded IDs
ToolResults-->>MockLLM: return pointer-selected values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | f7ac4586dc73 |
Head: f7ac4586dc731766c5e2734a6a24a67a1b26b999
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The versioned trace contract does not currently build its conformance tests, and exact bindings are not applied by the Reborn replay gateway.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Add the missing shared trace-contract vectors
Location: crates/ironclaw_llm/src/trace.rs:552-554
This include_str! references tests/fixtures/llm_traces/trace_contract_cases.json, but that file is not committed. The Python conformance test opens the same missing path. Consequently cargo test -p ironclaw_llm cannot compile and the pytest contract test fails; add the fixture or update both references to a committed location.
2. ❌ [MEDIUM] Apply result bindings in the Reborn replay path
Location: tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.json:383
The new resolver is invoked only by TraceLlm and the Python mock. RebornTraceReplayModelGateway forwards TraceToolCall.arguments directly into provider replay calls, so this marker reaches routine_fire as an object instead of the fresh string value. Resolve bindings from request tool-result metadata/content in that gateway and add a caller-level Reborn replay test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| } | ||
|
|
||
| fn contract_cases() -> ContractCases { | ||
| serde_json::from_str(include_str!( |
There was a problem hiding this comment.
The referenced shared conformance fixture is absent. include_str! will make the crate test build fail, and the Python test references the same path. Please commit the vector file or point both consumers at a committed file.
| "name": "routine_fire", | ||
| "arguments": { | ||
| "name": "{{call_657a9167a2d74cdd88556b34.name}}" | ||
| "name": {"$trace_result":{"tool_call_id":"call_657a9167a2d74cdd88556b34","pointer":"/name"}} |
There was a problem hiding this comment.
RebornTraceReplayModelGateway does not invoke the new binding resolver, so a Reborn replay forwards this marker object as the name argument. Resolve it against the current request's tool results in that gateway and cover the path with a Reborn replay test.
There was a problem hiding this comment.
Not applicable to the reduced PR: this fixture lives under tests/fixtures/llm_traces/live and is replayed by TraceLlm, which resolves the marker. RebornTraceReplayModelGateway only loads the separate reborn_qa fixture set; no code path loads this live fixture through that gateway. Adding a second resolver there would expand this intentionally simplified PR into cross-harness consolidation work.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 29-34: Remove the public re-export block from recording.rs and
make the relocated trace symbols private imports instead. Update references to
use crate::trace directly, or expose them through the intended named cross-crate
facade for ironclaw_reborn_traces, rather than preserving the recording module
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b581618c-922b-46e2-a148-6d37a53018ef
⛔ Files ignored due to path filters (131)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/bootstrap_onboarding.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/iteration_limit.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/long_tool_chain.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/mcp_extension_lifecycle.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/message_queue_during_tools.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/multi_turn_memory.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/prompt_injection_resilience.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/routine_event_any_channel.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/routine_event_telegram.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/routine_news_digest.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/steering.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/tool_error_recovery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/tool_intent_no_false_positive.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/tool_intent_nudge_cap.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/tool_intent_nudge_recovery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/advanced/workspace_search.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/bug_bash/summarization_uses_tools.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_always.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_always_floor.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_bare_yes_no_pending.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_no.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_slash.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/approval_yes.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/injection_in_echo.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/coverage/status_events_tool_chain.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/execution_obligation_exhaustion.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/execution_obligation_followup.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/execution_obligation_no_nudge.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/execution_obligation_nudge.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/external_tool_round_trip.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/multi_tool_chain.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/multi_turn.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/single_tool_echo.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/smoke_text.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/engine_v2/tool_error_recovery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/error_path.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/file_write_read.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/ceo_full_workflow.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/code_review_pr_2483.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/content_creator_full_workflow.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/developer_full_workflow.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/mission_daily_news_digest.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/sandbox_clones_ironclaw_to_megaclaw.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/trader_full_workflow.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/zizmor_scan.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/zizmor_scan_v2.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/memory/write_then_read_same_scope.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/memory/write_to_protected_path_rejected.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/memory_write_read.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/connect_gmail.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/github_notifications.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/investigate_ci_job.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10a_slack_self_attribution.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10b_slack_ooo_status.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10c_slack_thread_replies.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10d_slack_channel_membership.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10e_slack_error_honesty.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10f_slack_mention_encoding.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10g_slack_last_message_sent.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10g_slack_last_message_sent_global.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10h_slack_email_hallucination_guard.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_10i_slack_raw_entity_hygiene.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_2d_calendar_prep_live_chat.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_2f_calendar_prep_email_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_3b_endpoint_status_live_chat.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_3c_endpoint_status_slack_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_3d_endpoint_status_slack_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_4e_github_release_email_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_5d_slack_strategy_doc_answer.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_6c_gmail_to_sheet_live_chat.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_6e_gmail_to_sheet_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_7c_slack_bug_logger_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_7d_slack_bug_message_trigger.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_7e_slack_bug_sheet_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_8b_hn_keyword_live_chat.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_8c_hn_keyword_slack_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_8d_hn_keyword_slack_delivery.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_9b_routine_dm_delivery_exactly_once.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/qa_9d_routine_per_trigger_delivery_target.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_2a_gmail_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_2b_calendar_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_2c_drive_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_2e_calendar_prep_email_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_4a_gmail_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_4b_github_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_4c_github_release_live_chat.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_4d_github_release_slack_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_5b_drive_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_5c_strategy_doc_knowledge_base.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_6a_gmail_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_6b_sheets_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_6d_gmail_to_sheet_routine.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_7b_sheets_connect.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/live_canary/quarantined_retired_activation/qa_9c_slack_digest_names_not_ids.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/routine_crm_inbox.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/routine_health_ping.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/routine_hn_monitor.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/routine_meeting_prep.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/routine_release_watch.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_channel_membership.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_entity_hygiene.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_mention_encoding.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_ooo_status.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_recent_message.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_self_attribution.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/slack_thread_replies.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/web_hn_search.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/web_release_summary.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/reborn_qa/web_status_check.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/recorded/baseball_stats.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/recorded/telegram_check.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/recorded/weather_sf.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/runtime_policy/hosted_dev_no_shell.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/runtime_policy/local_dev_text_only.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/runtime_policy/no_policy_text_only.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/simple_text.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/attachment_audio_transcript.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/attachment_image.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/chain_write_read.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/memory_save_recall.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/robust_correct_tool.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/robust_no_tool.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/smoke_greeting.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/smoke_math.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/tool_echo.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/spot/tool_json.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/threading/concurrent_dispatch.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/threading/multi_turn_state.jsonis excluded by!tests/fixtures/**tests/fixtures/llm_traces/threading/undo_redo.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (14)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace.rscrates/ironclaw_reborn_traces/src/contribution.rsscripts/ci/check-reborn-qa-fixtures.shscripts/import-reborn-run-artifact.pyscripts/test-import-reborn-run-artifact.pytests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/e2e/trace_contract.pytests/support/trace_llm.rstests/trace_format.rs
|
🚅 Deployed to the ironclaw-pr-6659 environment in ironclaw-ci-preview
|
f7ac458 to
5a45376
Compare
github-actions
Bot
added
size: L
serrrfirat
changed the title
|
@ironloopai review |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py (1)
698-706: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePointer
/file/idis silently coupled to the name rewrite 20 lines above.The create call is rewritten to
google-drive__upload_fileat line 684, so this pointer targets the Drive upload result shape rather thangoogle-docs__create_document's. That coupling isn't obvious at this call site and will break quietly-then-loudly if the rewrite target ever changes. One line of comment earns its keep here.♻️ Suggested note
elif name.startswith("google-docs__") and "document_id" in arguments: call["name"] = "google-drive__download_file" + # `create_document` was rewritten to `google-drive__upload_file` + # above, so bind against the Drive upload result shape. call["arguments"] = { "file_id": ( _result_binding(created_document_call_id, "/file/id")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py` around lines 698 - 706, Add a concise inline comment in the google-docs call rewrite branch explaining that `/file/id` intentionally references the result shape produced by the earlier `google-drive__upload_file` rewrite. Keep the existing `file_id` binding and fallback behavior unchanged.tests/e2e/scenarios/test_reborn_qa_trace_replay.py (1)
370-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercises the alias remap through the real HTTP caller — good.
Gap worth closing while you're here:
_capture_trace_tool_call_id_aliaseshas two branches that abandon aliasing (count mismatch at line 486, name divergence at lines 497-503 — see my comment ontests/e2e/mock_llm.pylines 485-505). Neither is covered, so a regression there would surface only as a confusingno tool call with idconflict in an unrelated suite. A third case here — same fixture, but the runtime returns a differently-named or extra tool call — would pin the intended failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py` around lines 370 - 387, Extend the trace replay coverage around created_message to exercise _capture_trace_tool_call_id_aliases when the runtime returns an extra tool call or a differently named tool call. Assert that aliasing is abandoned and the real HTTP caller produces the intended “no tool call with id” conflict, rather than silently matching the wrong call.crates/ironclaw_llm/src/recording.rs (2)
809-848: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAmbiguous exact match still falls through to legacy substitution.
The comment states ambiguous exact matches "remain literal rather than guessing," but when
exact_matches.len() > 1the code doesn't return — it proceeds into thelookup.legacyloop (840-847), which can still rewrite the literal via an unrelated, HashMap-iteration-order-dependent legacy entry that coincidentally shares the same string value. This contradicts the stated invariant for that overlap case and isn't covered by the ambiguous test (which uses an emptylegacymap).🐛 Proposed fix
if let [(tool_call_id, pointer)] = exact_matches.as_slice() { *value = serde_json::json!({ "$trace_result": { "tool_call_id": tool_call_id, "pointer": pointer, } }); return; } + if !exact_matches.is_empty() { + // Ambiguous exact match: stay literal rather than falling + // back to a possibly-unrelated legacy template. + return; + } for (call_id, fields) in &lookup.legacy {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_llm/src/recording.rs` around lines 809 - 848, Update parameterize_value so an ambiguous exact match (exact_matches containing more than one result) returns immediately without entering the lookup.legacy substitution loop. Preserve literal values for this overlap case while retaining exact replacement for a single match and legacy substitution when no exact match exists.
574-906: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument why this expansion cannot move into a smaller module.
crates/ironclaw_llm/src/recording.rsis 2505 lines and this patch makes it larger (+155/-43). Percrates/**/*.rsfile-size guidance, 1,500–3,000 line files should become shorter when touched unless no suitable alternative exists, and additions over ~200 lines need inline justification. Add a one-line// arch-exempt: <reason>, plan#NNNN`` above this expansion, or split this into a companion module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_llm/src/recording.rs` around lines 574 - 906, Document why the new prior-tool lookup and parameterization helpers around build_prior_tool_lookup and parameterize_value remain in recording.rs despite its size. Add a one-line arch-exempt comment above this expansion with a concise reason and the applicable plan issue number, or move the helpers into a suitable companion module if no exemption is justified.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_llm/src/trace_binding.rs`:
- Around line 24-54: Update TraceBindingError to derive its Display and Error
implementations with thiserror, using appropriate error-message annotations for
InvalidMarker, MissingToolCall, and MissingPointer. Remove the manual
fmt::Display and std::error::Error implementations while preserving the existing
message text and field formatting.
In `@tests/e2e/mock_llm.py`:
- Around line 338-346: The tool-call schema must require id before bindings use
it: update _parse_llm_trace’s tool_calls validation in tests/e2e/mock_llm.py
(lines 338-346) to validate id alongside name and arguments. In
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py, replace direct call["id"]
access at lines 683 and 708-709 with optional lookup via call.get("id"),
preserving the existing seeded-identifier fallback and optional guards.
- Around line 338-346: Update _parse_llm_trace to validate each tool call’s id
alongside its name and arguments, requiring id to be present and a string.
Reject malformed trace steps during parsing with the existing validation error
path before _resolve_trace_result_bindings or pending_tool_calls processing.
- Around line 517-522: Rename the copied result variable inside the loop over
_find_tool_results in the result-building flow so the loop target is not
rebound, while preserving the existing dict conversion, tool_call_id mapping,
and results append behavior.
In `@tests/trace_llm_tests.rs`:
- Around line 3-4: Update the support import in tests/trace_llm_tests.rs to use
the crate-root path by changing the TraceLlm symbols’ import from
support::trace_llm to crate::support::trace_llm.
---
Outside diff comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 809-848: Update parameterize_value so an ambiguous exact match
(exact_matches containing more than one result) returns immediately without
entering the lookup.legacy substitution loop. Preserve literal values for this
overlap case while retaining exact replacement for a single match and legacy
substitution when no exact match exists.
- Around line 574-906: Document why the new prior-tool lookup and
parameterization helpers around build_prior_tool_lookup and parameterize_value
remain in recording.rs despite its size. Add a one-line arch-exempt comment
above this expansion with a concise reason and the applicable plan issue number,
or move the helpers into a suitable companion module if no exemption is
justified.
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 698-706: Add a concise inline comment in the google-docs call
rewrite branch explaining that `/file/id` intentionally references the result
shape produced by the earlier `google-drive__upload_file` rewrite. Keep the
existing `file_id` binding and fallback behavior unchanged.
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 370-387: Extend the trace replay coverage around created_message
to exercise _capture_trace_tool_call_id_aliases when the runtime returns an
extra tool call or a differently named tool call. Assert that aliasing is
abandoned and the real HTTP caller produces the intended “no tool call with id”
conflict, rather than silently matching the wrong call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d512560-74ed-4ba5-a6ca-0210a4a64d60
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
There was a problem hiding this comment.
⚠️ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| 0 | 0 | 0 | 5a45376dd3bd |
Head: 5a45376dd3bd817fa1e144d1d2b4d48f1ed34443
Next: Human review or validation is required before merging.
Run details
Status: Current
Needs human: no
Needs validation: yes
Summary
Static review found no actionable defects. Focused runtime validation remains required because this environment lacks Cargo and Python/UV tooling.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
5a45376 to
7716877
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py (1)
698-722: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the Sheets/drive trace markers with the recorded provider result shapes.
These exact markers (
/file/id,/spreadsheet_id,/sheets/0/sheet_id) will fail when the replay resolves against native/canonical return shapes such as Driveidor SheetsspreadsheetId/nested sheet objects, violatingtests/e2e/CLAUDE.md’s recorded→replay determinism requirement. Include a read-only replay invariant that asserts each binding pointer is present in the corresponding fixture payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py` around lines 698 - 722, Update the Google Drive/Sheets call-rewrite logic using _result_binding so pointers match the recorded provider result shapes: Drive’s canonical id, Sheets’ spreadsheetId, and the nested sheet identifier. Add a read-only replay invariant that verifies each generated binding pointer exists in its corresponding fixture payload, preserving recorded-to-replay determinism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 618-629: Update index_json_into to call parse_tool_result_content
for its content parsing instead of duplicating the serde_json::from_str and
coerce_python_repr_to_json fallback chain. Preserve the existing indexing
behavior while making parse_tool_result_content the single shared parsing path.
- Around line 863-901: The find_string_pointers function currently excludes
matches at the document root. Remove the pointer non-empty condition so a root
scalar string matching target records an empty JSON Pointer, preserving
resolve_trace_result_bindings support and exact replay of root scalar tool
results.
In `@tests/e2e/mock_llm.py`:
- Around line 476-523: Update _capture_trace_tool_call_id_aliases so a pending
batch is always cleared when actual_calls and pending_tool_calls have different
lengths. Before returning from the mismatch guard, reset
state["pending_tool_calls"] (or record the drift while dropping the batch),
preventing stale calls from being matched against a later assistant turn.
- Around line 368-390: Update _canonical_trace_result_payload to enforce the
Rust resolver’s trace-binding contract: validate that detail.preview.preview
contains valid JSON and that byte_len and total_bytes are integers, rejecting
non-integer numeric values. Preserve the existing $trace_result envelope,
tool_call_id, and pointer validation.
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 45-87: Extend the trace-result binding tests around
_resolve_trace_result_bindings to use the host-evidence
schema_version/status/trust envelope with detail.preview payloads instead of
only bare content. Add mirrored coverage for complete previews being unwrapped,
truncated previews remaining intact, and JSON Pointer tokens decoding ~0 and ~1
(including the /files/0/a~1b~0c path), while preserving the existing
requested-call and missing-call assertions.
---
Outside diff comments:
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 698-722: Update the Google Drive/Sheets call-rewrite logic using
_result_binding so pointers match the recorded provider result shapes: Drive’s
canonical id, Sheets’ spreadsheetId, and the nested sheet identifier. Add a
read-only replay invariant that verifies each generated binding pointer exists
in its corresponding fixture payload, preserving recorded-to-replay determinism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efa87d87-db0d-41db-90c6-2624c5069583
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
7716877 to
c3162dc
Compare
|
@ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | c3162dc1ba3c |
Head: c3162dc1ba3ca6c0c907273c8fc5289a0eee8afb
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Two correctness issues block the updated trace-replay coverage: QA 9C’s generated disclosure calls now fail strict trace installation, and truncated host-evidence previews can still be treated as binding sources.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Generated QA 9C disclosure calls lack newly-required IDs
Location: tests/e2e/mock_llm.py:183-184
The new validation rejects every tool call without an id, but _load_trace still invokes _inject_deferred_tool_disclosure for QA 9C, and that helper creates capability_info calls with only name and arguments. Installing that transformed trace now returns 400 before the full-path journey runs. Give injected calls deterministic IDs (or otherwise preserve compatibility).
2. ❌ [MEDIUM] Paged evidence previews can be mistaken for complete results
Location: crates/ironclaw_llm/src/trace_binding.rs:119-120
byte_len == total_bytes is not sufficient to establish that a result-reference preview is complete: paged/truncated host observations can carry equal byte counts while detail.next_offset is present. If such a partial preview is valid JSON, this unwraps and binds from it instead of failing as intended. Require an absent/null next_offset before accepting the preview, mirror that condition in mock_llm.py, and cover a valid-JSON truncated preview.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| for tool_index, tool_call in enumerate(tool_calls): | ||
| if ( | ||
| not isinstance(tool_call, dict) | ||
| or not isinstance(tool_call.get("id"), str) |
There was a problem hiding this comment.
QA 9C still injects capability_info calls without IDs in _inject_deferred_tool_disclosure, which _load_trace posts through this parser. This new requirement therefore makes that full-path trace fail installation with 400. Please assign deterministic IDs to those generated calls.
| .get("total_bytes") | ||
| .and_then(serde_json::Value::as_u64), | ||
| ), | ||
| (Some(byte_len), Some(total_bytes)) if byte_len == total_bytes |
There was a problem hiding this comment.
Equal byte counts do not prove a host-evidence preview is complete: paged result references may still have next_offset. A valid-JSON partial preview would be accepted here and bind instead of failing. Require no next_offset as well, and mirror the rule in the Python resolver.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/mock_llm.py (1)
478-492: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDrift guard still leaks a stale batch into later turns.
pendingis only consumed on the success path (Line 525). When the runtime batch length differs, Line 492 returns withstate["pending_tool_calls"]intact, so those recorded IDs survive and get name-matched against a different assistant batch once the counts happen to coincide._trace_tool_results_with_recorded_idsthen relabels the wrong runtime results and$trace_resultresolves to stale data instead of failing — inverting the exact-binding guarantee this PR is adding. Clear (or record) the batch on drift.🐛 Drop the batch instead of carrying it forward
if len(actual_calls) != len(pending): + state["pending_tool_calls"] = [] return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/mock_llm.py` around lines 478 - 492, Update _capture_trace_tool_call_id_aliases so a pending_tool_calls batch is cleared or otherwise discarded before returning when the actual assistant tool-call count differs from pending. Preserve the existing successful alias-mapping behavior, but ensure drifted IDs cannot be reused by later assistant batches or relabeled by _trace_tool_results_with_recorded_ids.tests/e2e/scenarios/test_reborn_qa_trace_replay.py (1)
45-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPython resolver's envelope and pointer-escape paths are still uncovered.
Both cases feed bare dict content, so
_canonical_trace_result_payloadand_decode_trace_pointer_token— the two functions that must stay byte-compatible withresolve_trace_result_bindingsincrates/ironclaw_llm/src/trace_binding.rs— are never exercised, even though real runtime results arrive as theschema_version/status/trust+detail.previewenvelope. Add the mirrored cases the Rust suite already has: complete preview unwrapped, truncated preview (byte_len != total_bytes) left intact, and a~0/~1escaped token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py` around lines 45 - 85, The resolver tests currently bypass the envelope normalization and JSON Pointer decoding paths. Extend the tests around _resolve_trace_result_bindings with cases using the real schema_version/status/trust and detail.preview envelope: unwrap a complete preview, preserve a truncated preview when byte_len differs from total_bytes, and resolve pointers containing ~0/~1 escapes, matching the Rust trace_binding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 2138-2183: Add an async integration-style test that exercises
RecordingLlm::complete_with_tools rather than calling build_prior_tool_lookup or
parameterize_value directly. Configure a tool result containing the nested file
ID, invoke completion with matching arguments, then inspect the persisted
TraceFile and assert it contains the exact $trace_result marker with the
expected tool_call_id and pointer.
---
Duplicate comments:
In `@tests/e2e/mock_llm.py`:
- Around line 478-492: Update _capture_trace_tool_call_id_aliases so a
pending_tool_calls batch is cleared or otherwise discarded before returning when
the actual assistant tool-call count differs from pending. Preserve the existing
successful alias-mapping behavior, but ensure drifted IDs cannot be reused by
later assistant batches or relabeled by _trace_tool_results_with_recorded_ids.
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 45-85: The resolver tests currently bypass the envelope
normalization and JSON Pointer decoding paths. Extend the tests around
_resolve_trace_result_bindings with cases using the real
schema_version/status/trust and detail.preview envelope: unwrap a complete
preview, preserve a truncated preview when byte_len differs from total_bytes,
and resolve pointers containing ~0/~1 escapes, matching the Rust trace_binding
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5d80c29-4a04-41b0-814a-da765cade621
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | 1699dadab81d |
Head: 1699dadab81dd432a6863adec954e14916c06214
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete correctness, security, or maintainability defects found in the exact trace-result binding migration.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/mock_llm.py (1)
180-190: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
idis now load-bearing but still not required to be unique across the trace.Aliases are keyed by recorded id in a single trace-scoped dict (Line 527) and inverted back in
_trace_tool_results_with_recorded_ids(Lines 577-580). A fixture that reuses oneidin two steps overwrites the alias and resolves$trace_resultagainst whichever turn aliased last — a silent wrong-result bind, which is the exact failure mode this PR removes. Parse time is the cheap place to reject it.🛡️ Reject duplicate ids at install time
responses = [] expected_user_inputs = {0: first_response["content"]} request_hints = [] pending_user_input = True + seen_tool_call_ids: set[str] = set() for index, step in enumerate(steps[1:], start=1): @@ raise ValueError( f"trace.steps[{index}].tool_calls[{tool_index}] is invalid" ) + if tool_call["id"] in seen_tool_call_ids: + raise ValueError( + f"trace.steps[{index}].tool_calls[{tool_index}] " + f"reuses tool call id {tool_call['id']!r}" + ) + seen_tool_call_ids.add(tool_call["id"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/mock_llm.py` around lines 180 - 190, Update the tool-call validation loop in the trace parsing path to track recorded tool-call ids across the entire trace and raise ValueError when an id appears more than once. Keep the existing type and field validation, and perform the uniqueness check before installing aliases so duplicate ids cannot overwrite entries used by _trace_tool_results_with_recorded_ids.crates/ironclaw_llm/src/recording.rs (1)
814-863: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo distinctiveness guard on exact-match binding for trivial string literals.
parameterize_valuebinds any argument string that uniquely matches a value inside a prior tool result, with no minimum-length/triviality check ons. A short or common literal (empty string,"true", a locale code, etc.) that happens to appear exactly once in an unrelated tool-result payload gets recorded as a$trace_resultreference to that call, even though it isn't genuinely derived from it — producing a semantically wrong but silently-passing fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_llm/src/recording.rs` around lines 814 - 863, Add a distinctiveness guard in parameterize_value before exact-match binding so trivial literals such as empty strings, booleans, and similarly short/common values are not converted to $trace_result references. Only run the exact-match lookup when s passes the established non-triviality criteria; otherwise preserve the existing legacy lookup and recursive behavior.
♻️ Duplicate comments (1)
crates/ironclaw_llm/src/recording.rs (1)
594-629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parse_tool_result_contentre-introduces the duplicate parse chain a past review flagged.
index_json_into(unchanged, lines 640-642) still hand-rolls the identicalserde_json::from_str(...).ok().or_else(coerce_python_repr_to_json → from_str)fallback that this newparse_tool_result_contentformalizes. The previous review comment askingindex_json_intoto call the shared helper instead of duplicating it is marked addressed, but the duplication is present in this code.♻️ Proposed fix
fn index_json_into( lookup: &mut HashMap<String, HashMap<String, String>>, key: &str, content: &str, ) { - let parsed: Option<serde_json::Value> = serde_json::from_str(content).ok().or_else(|| { - coerce_python_repr_to_json(content).and_then(|s| serde_json::from_str(&s).ok()) - }); + let parsed = parse_tool_result_content(content); let Some(json) = parsed else { return };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_llm/src/recording.rs` around lines 594 - 629, Remove the duplicated JSON parsing chain by updating index_json_into to call the shared parse_tool_result_content helper for its payload parsing. Preserve the existing indexing behavior and use parse_tool_result_content as the single implementation of direct JSON plus Python-representation fallback parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 814-863: Add a distinctiveness guard in parameterize_value before
exact-match binding so trivial literals such as empty strings, booleans, and
similarly short/common values are not converted to $trace_result references.
Only run the exact-match lookup when s passes the established non-triviality
criteria; otherwise preserve the existing legacy lookup and recursive behavior.
In `@tests/e2e/mock_llm.py`:
- Around line 180-190: Update the tool-call validation loop in the trace parsing
path to track recorded tool-call ids across the entire trace and raise
ValueError when an id appears more than once. Keep the existing type and field
validation, and perform the uniqueness check before installing aliases so
duplicate ids cannot overwrite entries used by
_trace_tool_results_with_recorded_ids.
---
Duplicate comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 594-629: Remove the duplicated JSON parsing chain by updating
index_json_into to call the shared parse_tool_result_content helper for its
payload parsing. Preserve the existing indexing behavior and use
parse_tool_result_content as the single implementation of direct JSON plus
Python-representation fallback parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 945b2a23-9646-4022-8751-7eec46781d5e
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
|
Addressed the current-head outside-diff review by rejecting duplicate tool-call IDs at trace install time (with regression coverage). I did not add an arbitrary short-string cutoff to recorder binding: the repository has no established threshold, and such a heuristic would silently reject valid short provider outputs. Exact unique matching remains deterministic; true provenance annotations can be a separate contract if needed. |
1699dad to
0c39889
Compare
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 0c3988976be7 |
Head: 0c3988976be75d3a7211f0ffbb4fe06dfe2d551d
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Exact trace-result replay is not reliable across user turns, and the Rust path still permits ambiguous duplicate call IDs.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Preserve prior-turn results for exact bindings
Location: tests/e2e/mock_llm.py:590
_find_tool_results only returns tool messages after the most recent user message. Therefore a valid marker in a later user turn cannot reference a resource created in an earlier turn: despite the original result and its persisted alias being present in messages, this loop supplies no candidate and replay returns 409. Collect historical tool results by call ID (or retain that evidence separately) before resolving, and add a multi-turn regression.
2. ❌ [MEDIUM] Reject ambiguous duplicate IDs in Rust replay
Location: crates/ironclaw_llm/src/trace_binding.rs:77
Selecting the newest matching result makes a duplicated tool-call ID resolve silently to the wrong resource. The Python trace loader now rejects duplicate fixture IDs, but Rust TraceLlm accepts such traces and reaches this resolver. Reject duplicate observed IDs here or validate unique non-empty IDs when loading Rust traces so $trace_result remains exact.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| for recorded_id, runtime_id in state["tool_call_id_aliases"].items() | ||
| } | ||
| results = [] | ||
| for observed in _find_tool_results(messages): |
There was a problem hiding this comment.
This scans only tool results after the latest user message. A later user turn cannot bind to a resource created in an earlier turn even though the exact call ID is still in the conversation. Preserve historical results by ID (and add a multi-turn binding regression).
| let result = observed | ||
| .iter() | ||
| .rev() | ||
| .find(|result| result.tool_call_id == binding.tool_call_id) |
There was a problem hiding this comment.
Duplicate IDs resolve to the most recent result instead of failing. Rust TraceLlm does not enforce the Python loader's unique-ID rule, so a malformed fixture can bind a later call to the wrong resource. Reject duplicate observed IDs or validate IDs when loading the trace.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_llm/CLAUDE.md`:
- Around line 289-291: Update the documentation statement around
tool-result-derived arguments to acknowledge the legacy `{{call_id.field}}`
fallback used by `parameterize_value` in `recording.rs` when matching is
ambiguous or no call ID is available. Soften the absolute guarantee about the
exact `$trace_result` marker while preserving the documented preferred behavior
and RFC 6901 pointer details.
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 91-118: Extend the Python replay tests around
_canonical_trace_result_payload and _resolve_trace_result_bindings to cover
complete previews: when byte_len equals total_bytes and next_offset is absent,
unwrap the preview and resolve its JSON Pointer successfully. Include a pointer
containing escaped ~0 and ~1 tokens, asserting they decode to the correct object
keys and preserve byte-compatible behavior with resolve_trace_result_bindings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b635fa9-f4bb-48db-9b1c-4cc724352871
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
|
|
||
| Arguments derived from an earlier tool result use an exact `$trace_result` | ||
| marker with the original `tool_call_id` and an RFC 6901 JSON Pointer. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Soften the absolute claim to mention the legacy fallback.
This states unqualified that tool-result-derived arguments "use an exact $trace_result marker," but parameterize_value in recording.rs still falls back to the legacy {{call_id.field}} template when a match is ambiguous or the source is a sanitized user message without a call ID. As written, this overstates the guarantee.
As per coding guidelines, **/*.{rs,md}: "Comments promising cross-layer guarantees must be enforced by code or tests, or softened to describe intent."
📝 Proposed doc fix
-Arguments derived from an earlier tool result use an exact `$trace_result`
-marker with the original `tool_call_id` and an RFC 6901 JSON Pointer.
+Arguments that uniquely match an earlier native tool result use an exact
+`$trace_result` marker with the original `tool_call_id` and an RFC 6901 JSON
+Pointer. Ambiguous matches and sanitized (call-ID-less) tool results still
+fall back to the legacy `{{call_id.field}}` template.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Arguments derived from an earlier tool result use an exact `$trace_result` | |
| marker with the original `tool_call_id` and an RFC 6901 JSON Pointer. | |
| Arguments that uniquely match an earlier native tool result use an exact | |
| `$trace_result` marker with the original `tool_call_id` and an RFC 6901 JSON | |
| Pointer. Ambiguous matches and sanitized (call-ID-less) tool results still | |
| fall back to the legacy `{{call_id.field}}` template. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_llm/CLAUDE.md` around lines 289 - 291, Update the
documentation statement around tool-result-derived arguments to acknowledge the
legacy `{{call_id.field}}` fallback used by `parameterize_value` in
`recording.rs` when matching is ambiguous or no call ID is available. Soften the
absolute guarantee about the exact `$trace_result` marker while preserving the
documented preferred behavior and RFC 6901 pointer details.
Source: Coding guidelines
0c39889 to
86b9af0
Compare
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
⚠️ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| 0 | 0 | 0 | 86b9af0182be |
Head: 86b9af0182bec7ba647df62e12f55a3b74f04212
Next: Human review or validation is required before merging.
Run details
Status: Current
Needs human: no
Needs validation: yes
Summary
Static review found no concrete actionable defects. The 11-file normal PR consistently implements exact call-ID/JSON-Pointer bindings across recording, Rust replay, Python replay, fixture migration, and focused tests.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/e2e/scenarios/test_reborn_qa_trace_replay.py (1)
152-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill only the rejection half is pinned; the unwrap and escape branches remain untested.
This test covers the paged/truncated envelope. Nothing in this file exercises the complementary paths in
mock_llm.py: a complete preview (byte_len == total_bytes,next_offsetabsent) being unwrapped at Lines 491-492, and_decode_trace_pointer_token(Lines 457-469) — no pointer here contains~0or~1. A change that stops unwrapping complete previews, or that mis-decodes an escaped token, passes this entire suite while the Rust lane (which the PR states covers/files/0/a~1b~0c) keeps passing. That is the exact cross-language drift this binding contract is meant to close.Raised on a prior commit and still open.
🧪 Mirror the two Rust cases
+def test_exact_trace_result_binding_unwraps_complete_evidence_preview(): + marker = { + "$trace_result": { + "tool_call_id": "call_create", + "pointer": "/document/documentId", + } + } + preview = json.dumps({"document": {"documentId": "whole-document"}}) + observed = [ + { + "tool_call_id": "call_create", + "content": { + "schema_version": 1, + "status": "success", + "trust": "provider", + "detail": { + "byte_len": len(preview), + "total_bytes": len(preview), + "preview": preview, + }, + }, + } + ] + + assert _resolve_trace_result_bindings(marker, observed) == "whole-document" + + +def test_exact_trace_result_binding_decodes_escaped_pointer_tokens(): + marker = { + "$trace_result": { + "tool_call_id": "call_list", + "pointer": "/files/0/a~1b~0c", + } + } + observed = [ + { + "tool_call_id": "call_list", + "content": {"files": [{"a/b~c": "escaped-hit"}]}, + } + ] + + assert _resolve_trace_result_bindings(marker, observed) == "escaped-hit"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py` around lines 152 - 179, Add tests in this file covering the complementary branches of `_resolve_trace_result_bindings`: verify a complete preview with `byte_len == total_bytes` and no `next_offset` is unwrapped, and verify a JSON Pointer containing escaped `~1` and `~0` tokens is decoded correctly. Mirror the Rust cases and assert the resolved values, while preserving the existing rejection test for paged evidence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Line 716: Update the renamed Drive upload handling around
created_document_call_id to use the actual google-drive__upload_file response
shape, without rewriting create_document IDs as Drive file IDs; keep Docs
document binders pointed to the Docs output. Add readback assertions for the
Drive upload and Sheets create_spreadsheet response fields at /spreadsheet_id
and /sheets/0/sheet_id so invalid shapes fail before binding defaults.
- Around line 741-755: Update the argument-rewriting logic around
created_spreadsheet_call_id so spreadsheet_id and sheet_id always use a
consistent source: bind both to the created spreadsheet when available,
otherwise replace both with the corresponding seeded spreadsheet and seeded
sheet values. Do not leave the harvested sheet_id when falling back to
seeded_spreadsheet.
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 182-221: Add a positive test alongside
test_trace_aliasing_rejects_indistinguishable_parallel_calls that uses two
pending calls with the same normalized tool name but different arguments and
runtime messages in reversed order. Call _capture_trace_tool_call_id_aliases,
assert it succeeds, and verify each recorded call id maps to the runtime id with
matching arguments rather than positional order.
---
Duplicate comments:
In `@tests/e2e/scenarios/test_reborn_qa_trace_replay.py`:
- Around line 152-179: Add tests in this file covering the complementary
branches of `_resolve_trace_result_bindings`: verify a complete preview with
`byte_len == total_bytes` and no `next_offset` is unwrapped, and verify a JSON
Pointer containing escaped `~1` and `~0` tokens is decoded correctly. Mirror the
Rust cases and assert the resolved values, while preserving the existing
rejection test for paged evidence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bafbd91-df36-4950-b820-caf98643a563
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
86b9af0 to
7527858
Compare
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | 75278582ad07 |
Head: 75278582ad07eee5a53fda0bdc832dbf2fa60952
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Approved. Reviewed the focused 11-file merge-base diff; exact ID/pointer binding, alias handling, recorder behavior, and fixture migration are coherent and covered by targeted tests.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_llm/src/recording.rs (1)
574-908: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove the record-side binding logic into
trace_binding.rs, not deeper intorecording.rs.
PriorToolLookup,build_prior_tool_lookup,parse_tool_result_content,parameterize_value,find_string_pointers, andescape_json_pointer_token(~300 new lines) land inline inrecording.rs, which is already deep into the 1,500-3,000 line band. This PR proves a suitable alternative location exists — it createdtrace_binding.rsfor the mirror-image resolve-side logic in the same commit — so the "no suitable alternative location" exemption for growing an oversized file doesn't apply, and there's no// arch-exempt:justification for the >200-line addition.As per coding guidelines,
crates/**/*.rs: "Existing files between 1,500 and 3,000 lines should become shorter when touched unless feature expansion has no suitable alternative location. Files over 3,000 lines require a decomposition tracking issue, and additions over 200 lines require inline justification."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_llm/src/recording.rs` around lines 574 - 908, Move the record-side binding implementation out of recording.rs into the existing trace_binding.rs module, including PriorToolLookup, build_prior_tool_lookup, parse_tool_result_content, parameterize_value, find_string_pointers, and escape_json_pointer_token plus their required helpers. Update recording.rs imports and call sites to use the relocated symbols, preserving behavior while reducing the oversized file; do not add an architecture exemption.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_llm/src/recording.rs`:
- Around line 574-908: Move the record-side binding implementation out of
recording.rs into the existing trace_binding.rs module, including
PriorToolLookup, build_prior_tool_lookup, parse_tool_result_content,
parameterize_value, find_string_pointers, and escape_json_pointer_token plus
their required helpers. Update recording.rs imports and call sites to use the
relocated symbols, preserving behavior while reducing the oversized file; do not
add an architecture exemption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc6bb69d-84f0-471a-84c4-35d47b0100db
⛔ Files ignored due to path filters (2)
tests/fixtures/llm_traces/README.mdis excluded by!tests/fixtures/**tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (9)
crates/ironclaw_llm/CLAUDE.mdcrates/ironclaw_llm/src/lib.rscrates/ironclaw_llm/src/recording.rscrates/ironclaw_llm/src/trace_binding.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_qa_trace_replay.pytests/support/trace_llm.rstests/trace_llm_tests.rs
|
Follow-up on CodeRabbit's outside-diff module-move suggestion: keeping the record-side helpers in |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.97% — 314053 / 365320 lines Per-crate breakdown (61 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
This branch was successfully deployed
1 participant
Summary
tool_call_idplus an RFC 6901 JSON Pointer.TraceLlm, and existing Python mock LLM to produce/resolve that marker, including provider-normalized call IDs and complete host-evidence previews.This deliberately does not introduce trace format versioning, migrate the fixture catalog, change importer behavior, move trace types, or add a cross-language conformance framework.
Change Type
Linked Issue
Related #6524
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo build(built by the standalone Reborn/Emulate E2E setup)cargo test --features integration— Not applicable: no database-backed behavior changed.review-prorpr-shepherd --fixwas run before requesting review.Test Strategy
User behavior:
Recorded-model tests can replay a later capability call using the fresh resource ID returned by the current hermetic provider run. They no longer guess by tool name or a list of possible field names.
Risk areas:
Tests added or updated:
TraceLlmcaller test proves exact binding through the provider call; the existing trace-format suite remains green.What the tests prove:
A binding consumes only the named prior tool call and exact result path; missing IDs, missing pointers, ambiguous recorder matches, and truncated evidence fail rather than selecting a plausible value. The caller-level Docs run proves a fixture call ID still resolves when the provider stack normalizes the runtime call ID.
Commands run:
scripts/pre-commit-safety.shwas also run. Its composition budget passed, then it stopped on an existingARCH-SPRAWLwarning incrates/ironclaw_auth/src/product_auth/api/auth.rs; this PR has no diff in that file.Security Impact
None. This changes test replay infrastructure and the opt-in trace recorder. Binding failures report IDs and paths, not provider result values.
Reborn Trust-Boundary Checklist
N/A: no Reborn trust, policy, ingress, persistence, authorization, or production runtime boundary changes.
Database Impact
None.
Blast Radius
Eleven files: the trace binding helper, recorder, Rust replay support, existing Python mock LLM, focused tests/docs, and one fixture. Existing trace shape and legacy
{{call.field}}replay remain compatible.Rollback Plan
Revert this PR. No database, fixture-catalog, or product migration is involved.
Review Follow-Through
Reviewer judgment is most useful around exact call-ID aliasing and the rule that host-evidence previews are usable only when complete.
Review track: A (docs/tests/chore)