fix(loop): preserve pageable result_read continuation references - #7135
Conversation
Return the original pageable result reference from result_read while retaining inline-only chunk persistence and evidence. Key transcript dedup by provider call so multiple pages can safely share one durable source reference, with regression coverage for replay and a two-page continuation.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-7135 environment in ironclaw-ci-preview
|
github-actions
Bot
added
size: L
🔎 Review · PR #7135
Submitted review →Reviewed the complete trusted base-to-head comparison. The change consistently preserves the original pageable result reference, distinguishes transcript entries by provider call identity, retains legacy filesystem lookup compatibility, and adds appropriate backend and caller-path regressions. No actionable findings identified. Automatic · PR opened · attempt 1 of 3 · completed in 1m 46s Run details
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesDurable result flow
Subagent provider-call identity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant result_read
participant ThreadHistory
participant AwaitEdge
Client->>result_read: request result page with durable reference and offset
result_read->>ThreadHistory: resolve and persist result-read metadata
ThreadHistory-->>result_read: return durable reference and next offset
result_read-->>Client: return page and continuation metadata
AwaitEdge->>ThreadHistory: update parent tool-result reference with provider call ID
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.
🔍 Review complete · PR #7135
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. The change consistently preserves the original pageable result reference, distinguishes transcript entries by provider call identity, retains legacy filesystem lookup compatibility, and adds appropriate backend and caller-path regressions. No actionable findings identified.
Validation and technical details
- Confirmed comparison refs: base 1e2a294 to head 11eda63.
- Inspected all seven changed files and surrounding result-writing, transcript lookup, update, replay, indexing, and model-observation code.
- git diff --check completed successfully.
- Checked changed production additions for prohibited unwrap/expect calls, unsafe code, and hardcoded temporary paths; none found.
- Focused Cargo tests could not be executed because cargo is unavailable in the review environment (/bin/bash: cargo: command not found).
- Base:
main - Head:
codex/fix-result-read-continuation-refat11eda63 - Run:
12b00d73-29af-4fb0-b1e9-54fc158a48bd
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_loop_host/src/result_read.rs`:
- Around line 242-248: Update the LoopResultRef::new call in result_read to
preserve and propagate the original validation error while adding contextual
information; replace the map_err closure’s discarded error binding with an
error-aware conversion. Keep the existing AgentLoopHostError classification and
message context, and do not introduce a separate validation path.
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 3294-3307: The provider-keyed lookup and legacy fallback currently
share matches_tool_result_reference_invocation, allowing fallback matches on
rows with provider metadata. Split the predicates in filesystem_service.rs at
matches_tool_result_reference_invocation and the corresponding in_memory.rs
site: keep provider_call_id matching for the keyed lookup, but make the fallback
match only records whose tool_result_provider_call is absent.
- Line 2202: Update update_tool_result_reference to preserve and use the
specific message_id returned by append_tool_result_reference for each tool
result/reference row, rather than performing a generic lookup with no
provider-call ID. Ensure the read-modify-write follows the shared bounded CAS
update path and cannot target another row when multiple writers share the same
result reference. Add a regression test covering multiple writers and verifying
each summary update applies to its original entry.
In `@crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs`:
- Around line 122-138: Extend the filesystem session thread contract test to
assert each retained ToolResultReference row’s provider_call_id matches its
originating provider call, not just message identity and count. Add a mixed case
appending page one with no provider call and page two with Some(call_2), then
verify both rows remain distinct and retain their correct provider metadata
through list_thread_history. Use the existing legacy-fallback scenario and
history assertions as the test location.
In `@tests/integration/tool_call.rs`:
- Around line 736-746: Update the envelope selection in the integration test
around persisted_tool_result_envelopes() to filter entries by
envelope.result_ref == result_ref, rather than taking the last two rows. Assert
that exactly two matching result_read envelopes exist, then select page one and
page two using their stable order while preserving the existing assertions.
🪄 Autofix
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: e977d814-daa1-44b2-a745-2175475f957a
📒 Files selected for processing (7)
crates/ironclaw_loop_host/src/result_read.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/filesystem_service/message_lookup_index.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_threads/tests/session_thread_contract.rstests/integration/tool_call.rs
🔎 Review · PR #7135
The target changed before this Run could finish. Automatic · PR opened · attempt 0 of 3 · cancelled after 1s Run details
|
Railway preview QA — PASSTested the exact deployed PR head:
Given / When / Then evidence
Exact regression resultThe production WebUI caller successfully reused one original pageable identity across two distinct Exploratory attempts not counted as evidence
Skipped as unrelatedPermissions/role denial, responsive layout, uploads, auth lifecycle beyond preview login, and streaming cadence were not tested because this PR changes backend result-reference continuation and durable replay behavior, not those surfaces. |
|
CI follow-up for commit
Local evidence:
|
|
CI is green on |
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 (3)
crates/ironclaw_runner/src/subagent/await_edge/resolver.rs (1)
1572-1640: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a collision regression for
spawn_provider_call_id.The drain test gives each child a unique
result_ref. It passes if provider-call propagation is removed because legacy generic lookup can still find each row. Use one sharedresult_refwith distinct provider call IDs. Assert that each placeholder receives its own terminal summary.
crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1572-L1640: use a shared result reference and assert provider-call-specific transcript updates.crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1062-L1100: assert that reconstruction retainsspawn_provider_call_id.crates/ironclaw_runner/src/subagent/await_edge/store.rs#L489-L503: assert that legacy conversion retainsspawn_provider_call_id.As per coding guidelines, “New or changed production-wired behavior must have a caller-level test” and “Every bug fix must include a regression test that fails before the fix.”
🤖 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_runner/src/subagent/await_edge/resolver.rs` around lines 1572 - 1640, Strengthen the collision regression: in crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1572-L1640, give all drained children one shared result_ref while keeping distinct spawn_provider_call_id values, then assert each placeholder receives its own terminal summary. In crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1062-L1100, assert reconstruction preserves AwaitedChildSetRecord.spawn_provider_call_id. In crates/ironclaw_runner/src/subagent/await_edge/store.rs#L489-L503, assert legacy conversion also preserves spawn_provider_call_id.Source: Coding guidelines
tests/integration/tool_call.rs (1)
799-806: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the second continuation is terminal.
page_two_detail["next_offset"]is only compared withpage_two["next_offset"]. A regression that emits a third-page offset preserves that equality and passes this test. Assert thatpage_two["next_offset"]isnullto verify the terminal marker required by this continuation scenario.Proposed regression assertion
assert_eq!( page_two_detail["next_offset"].as_u64(), page_two["next_offset"].as_u64(), "second-page replay metadata must match the second page output" ); + assert!( + page_two["next_offset"].is_null(), + "the second continuation must be terminal" + );🤖 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/integration/tool_call.rs` around lines 799 - 806, Update the second-page assertions in the continuation scenario to verify that page_two["next_offset"] is null, confirming the second continuation is terminal. Retain the existing equality check with page_two_detail["next_offset"] so replay metadata remains validated.crates/ironclaw_threads/src/filesystem_service.rs (1)
1989-2054: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake provider-call replay lookups the write authority.
append_tool_result_referenceusesfind_tool_result_reference_messageand then persists separately:write_new_messageskips a populated lookup key instead of returning the winner, so concurrent identical replays can create duplicate records.apply_message_updatealso writes provider indexes best-effort after the message CAS, so a failedtool_result_provider_callindex write can cause a later generic-row replay to add another row. Store the provider-call key in the same transaction/CAS as the message or lookup failure, and update the generic row as a conflict or validation failure rather than a replay duplicate, with concurrent-replay and index-write-failure regression tests.🤖 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_threads/src/filesystem_service.rs` around lines 1989 - 2054, Make provider-call replay lookup authoritative in append_tool_result_reference and the related write/update flow: atomically claim or persist the provider-call key with the message CAS, and have write_new_message return the existing winner when the key is already populated instead of creating another row. Treat provider-index write failures as lookup/validation failures and update the existing generic row as a conflict rather than appending a duplicate. Add regression coverage for concurrent identical replays and provider-index write failures.Sources: Coding guidelines, Path instructions
🤖 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_runner/src/subagent/await_edge/resolver.rs`:
- Around line 1572-1640: Strengthen the collision regression: in
crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1572-L1640, give all
drained children one shared result_ref while keeping distinct
spawn_provider_call_id values, then assert each placeholder receives its own
terminal summary. In
crates/ironclaw_runner/src/subagent/await_edge/resolver.rs#L1062-L1100, assert
reconstruction preserves AwaitedChildSetRecord.spawn_provider_call_id. In
crates/ironclaw_runner/src/subagent/await_edge/store.rs#L489-L503, assert legacy
conversion also preserves spawn_provider_call_id.
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1989-2054: Make provider-call replay lookup authoritative in
append_tool_result_reference and the related write/update flow: atomically claim
or persist the provider-call key with the message CAS, and have
write_new_message return the existing winner when the key is already populated
instead of creating another row. Treat provider-index write failures as
lookup/validation failures and update the existing generic row as a conflict
rather than appending a duplicate. Add regression coverage for concurrent
identical replays and provider-index write failures.
In `@tests/integration/tool_call.rs`:
- Around line 799-806: Update the second-page assertions in the continuation
scenario to verify that page_two["next_offset"] is null, confirming the second
continuation is terminal. Retain the existing equality check with
page_two_detail["next_offset"] so replay metadata remains validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b92839d-6411-4e30-8e93-1072cb9887bf
📒 Files selected for processing (13)
crates/ironclaw_loop_host/src/result_read.rscrates/ironclaw_loop_host/src/subagent_spawn_port.rscrates/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/ironclaw_runner/src/subagent/await_edge/mod.rscrates/ironclaw_runner/src/subagent/await_edge/resolver.rscrates/ironclaw_runner/src/subagent/await_edge/store.rscrates/ironclaw_runner/src/subagent/prompt_material.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_threads/tests/session_thread_contract.rstests/integration/tool_call.rs
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_reborn_composition/src/runtime/tests/core.rs (1)
1038-1040: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert against the original
result_ref.
result_refis captured from the original observation at Lines 974-977 and passed toresult_readat Lines 996-997. This assertion compares two fields from the returned payload. If both fields contain the staging-write reference, the test still passes. Compare both returned fields with the capturedresult_refso the caller-path regression test enforces the corrected continuation contract.Suggested assertion
+ let expected_result_ref = result_ref.clone(); assert_eq!( - detail["result_ref"], observation["result_ref"], - "result_read replay must expose only the original pageable result reference" + detail["result_ref"].as_str(), + Some(expected_result_ref.as_str()), + "result_read detail must preserve the original pageable result reference" ); + assert_eq!( + observation["result_ref"].as_str(), + Some(expected_result_ref.as_str()), + "result_read observation must preserve the original pageable result reference" + );🤖 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_reborn_composition/src/runtime/tests/core.rs` around lines 1038 - 1040, Update the assertion in the result_read replay test to compare both returned payload fields against the captured original result_ref from the initial observation, rather than comparing the returned fields to each other. Preserve the existing regression coverage for the corrected continuation contract.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_reborn_composition/src/runtime/tests/core.rs`:
- Around line 1038-1040: Update the assertion in the result_read replay test to
compare both returned payload fields against the captured original result_ref
from the initial observation, rather than comparing the returned fields to each
other. Preserve the existing regression coverage for the corrected continuation
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9bf8aa4-c7c8-4aeb-899f-5788144062fa
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/runtime/tests/core.rs
Railway preview QA — PASS (review-fix head)Tested the exact deployed PR head:
Given / When / Then evidence
Exact regression assertionThe production WebUI caller fed the first surfaced Exploratory call excluded from the assertionAfter the two target byte-page calls succeeded, the preview assistant made one extra exploratory Skipped as unrelatedPermissions/role denial, responsive layout, uploads, and auth lifecycle beyond preview login were not retested because this PR changes backend result-reference continuation and durable replay behavior. |
Final verification — green
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Railway preview QA — PASS
Given / When / Then evidence
Exact regression result: the model-visible continuation identity remained usable through all pages and terminated correctly; no fresh inline-only reference displaced continuation authority. Cleanup: browser tabs were finalized. No files or external systems were mutated. One clearly identifiable acceptance-test conversation remains only in the ephemeral PR preview; it was retained to preserve reviewable read-back evidence. Skipped: permissions/denied-role and unrelated UI routes, because this PR changes backend result-reference continuation rather than authorization or frontend behavior. External model selection was used only to drive the real caller; the deterministic pagination assertions above are based on rendered tool outcomes and byte counts. |
Merge-queue remediation — PASSExact head:
Railway browser evidence: #7135 (comment) The PR is CI-green and conflict-free. GitHub still shows |
…-continuation-ref
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs (1)
1572-1641: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake provider-call identity observable in these regression tests.
The mixed-status test uses a distinct
result_reffor each child.update_tool_result_referencecan therefore select the correct placeholder withoutprovider_call_id. Use one sharedresult_refand assert that each provider-call envelope receives its own terminal summary.
crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs#L1572-L1641: use the same result reference for both parent placeholders, then assert updates select the matching provider call ID.crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs#L1062-L1101: assertreconstruct_edgepreservesspawn_provider_call_id.crates/loop/ironclaw_turn_runner/src/subagent/await_edge/store.rs#L478-L504: assert legacy metadata projection preservesspawn_provider_call_id.This violates the
Test through the callerinvariant for a helper that gates transcript updates. As per coding guidelines, “New or changed production-wired behavior must have a caller-level test at the nearest meaningful seam.” As per path instructions, “Test through the caller” requires a test that drives the real call site for a helper that gates a side effect.🤖 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/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs` around lines 1572 - 1641, Update the mixed-status regression test around the await-edge setup in crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs:1572-1641 to reuse one result_ref for both parent placeholders and assert each terminal update selects the matching provider_call_id and summary. In crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs:1062-1101, extend the reconstruct_edge assertions to preserve spawn_provider_call_id. In crates/loop/ironclaw_turn_runner/src/subagent/await_edge/store.rs:478-504, assert the legacy metadata projection also preserves spawn_provider_call_id.Sources: Coding guidelines, Path instructions
🤖 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/integration/tool_call.rs`:
- Around line 598-607: Update the result_read-related documentation entry in
tests/CLAUDE.md to describe the re-scoped tool_call.rs integration scenario: two
continuation turns on the same thread, both persisted result_read envelopes, and
continuation via result_ref and next_offset while preserving the durable
read_file serialization. Keep the existing Tools reference and make the
documentation reflect the current test behavior.
---
Outside diff comments:
In `@crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs`:
- Around line 1572-1641: Update the mixed-status regression test around the
await-edge setup in
crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs:1572-1641
to reuse one result_ref for both parent placeholders and assert each terminal
update selects the matching provider_call_id and summary. In
crates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rs:1062-1101,
extend the reconstruct_edge assertions to preserve spawn_provider_call_id. In
crates/loop/ironclaw_turn_runner/src/subagent/await_edge/store.rs:478-504,
assert the legacy metadata projection also preserves spawn_provider_call_id.
🪄 Autofix
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: 2853faf2-8418-43ab-9d50-39b1fde774e2
📒 Files selected for processing (16)
crates/app/ironclaw_composition/src/runtime/capability_host/tests.rscrates/app/ironclaw_composition/src/runtime/tests/core.rscrates/domains/ironclaw_threads/src/contract.rscrates/domains/ironclaw_threads/src/filesystem_service.rscrates/domains/ironclaw_threads/src/filesystem_service/message_lookup_index.rscrates/domains/ironclaw_threads/src/in_memory.rscrates/domains/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/domains/ironclaw_threads/tests/session_thread_contract.rscrates/loop/ironclaw_loop_host/src/result_read.rscrates/loop/ironclaw_loop_host/src/subagent_spawn_port.rscrates/loop/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/loop/ironclaw_turn_runner/src/subagent/await_edge/mod.rscrates/loop/ironclaw_turn_runner/src/subagent/await_edge/resolver.rscrates/loop/ironclaw_turn_runner/src/subagent/await_edge/store.rscrates/loop/ironclaw_turn_runner/src/subagent/prompt_material.rstests/integration/tool_call.rs
| /// `result_read` continuation (issue #5838): two subsequent scripted turns on | ||
| /// the SAME thread page the durable `read_file` result. Page two is invoked | ||
| /// exclusively with the `result_ref` and `next_offset` surfaced by page one, | ||
| /// proving that model-visible continuation metadata retains the original | ||
| /// pageable identity instead of exposing the fresh `InlineOnly` write ref. | ||
| /// Both chunks continue byte-exactly through the SAME canonical serialization | ||
| /// `tool_result_output` returns for `read_file` — no gap, no overlap — and | ||
| /// report the durable record's true `total_bytes`. Page one's chunk contains a | ||
| /// credential marker, so its inline preview is suppressed; the continuation | ||
| /// identity and offset must survive independently of preview content. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check that tests/CLAUDE.md describes the re-scoped result_read continuation scenario.
set -euo pipefail
fd -t f 'CLAUDE.md' tests
# Test: the scenario name and its documented shape. Expect a description matching two continuation pages.
rg -n -C 6 'result_read_continues_a_durable_result_byte_exactly|tool_call' tests/CLAUDE.md || true
# Confirm the change touched the doc in this branch.
git log --name-only -1 --pretty=format: | rg -n 'tests/CLAUDE\.md' || echo "tests/CLAUDE.md not modified in HEAD commit"Repository: nearai/ironclaw
Length of output: 2811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
md_path = Path("tests/CL AUDE.md")
if md_path.exists():
text = md_path.read_text()
lines = []
for i, line in enumerate(text.splitlines(), 1):
if "tool_call.rs" in line or "result_read_continues_a_durable_result_byte_exactly" in line:
start=max(1,i-8); end=min(len(text.splitlines()),i+9)
rows=text.splitlines()
print(f"\n--- context around line {i} ---")
for j in range(start,end+1):
print(f"{j}:{rows[j-1]}")
else:
print("tests/CL AUDE.md missing")
test_path = Path("tests/integration/tool_call.rs")
if test_path.exists():
txt=test_path.read_text().splitlines()
for idx,line in enumerate(txt,1):
if "result_read_continues_a_durable_result_byte_exactly" in line:
print(f"\n--- test symbol at line {idx} ---")
for j in range(max(1,idx-5), min(len(txt), idx+14)+1):
print(f"{j}:{txt[j-1]}")
PY
# List changed tracked files in the PR diff, if available.
git diff --name-only -- tests/CLAUDE.md tests/integration/tool_call.rs || trueRepository: nearai/ironclaw
Length of output: 1236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- tests CLAUDE.md tool sections ---"
rg -n -C 3 'tool_call\.rs|result_read_continues_a_durable_result_byte_exactly|result_read' tests/CLAUDE.md || echo "no matches"
echo "--- files tracked matching CLAUDE.md ---"
git ls-files | rg '(^|/)CLAUDE\.md$|CLAUDE|claude|AGENTS' || true
echo "--- test function surrounding lines ---"
sed -n '588,650p' tests/integration/tool_call.rsRepository: nearai/ironclaw
Length of output: 9657
Add the re-scoped result_read scenario to tests/CLAUDE.md.
This integration test now expects two continuation turns and checks both persisted result_read envelopes. Path invariants require the same-commit tests/CLAUDE.md update for materially re-scoped Reborn integration tests. The current entry only references tool_call.rs under Tools.
🤖 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/integration/tool_call.rs` around lines 598 - 607, Update the
result_read-related documentation entry in tests/CLAUDE.md to describe the
re-scoped tool_call.rs integration scenario: two continuation turns on the same
thread, both persisted result_read envelopes, and continuation via result_ref
and next_offset while preserving the durable read_file serialization. Keep the
existing Tools reference and make the documentation reflect the current test
behavior.
Source: Path instructions
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 (3)
tests/integration/tool_call.rs (3)
406-408: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKey the HTTP response by URL.
with_real_egress_response_bodies([source_bytes])binds the body by response order. Registersource_byteswithScriptedHttpResponseforHTTP_TOOL_URL. This prevents a later request-order change from serving the fixture to the wrong request.As per path instructions, “Use
ScriptedHttpResponseas the canonical keyed HTTP scripting API; add new per-URL responses there rather than creating a parallel matcher.”🤖 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/integration/tool_call.rs` around lines 406 - 408, Update the RebornIntegrationHarness setup to register source_bytes through ScriptedHttpResponse keyed by HTTP_TOOL_URL instead of using with_real_egress_response_bodies. Preserve the existing real egress pipeline while ensuring the fixture is selected by URL rather than response order.Source: Path instructions
357-400: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove the large fixture and assertions out of the test body.
json_queries_scoped_file_and_adjacent_array_indicesspans about 180 lines and embeds large nestedserde_json::json!fixtures. Extract the fixture, scripted replies, and repeated assertions into focused helpers. Keep the visible test body asbuild → submit_turn → assert.As per path instructions, “Keep individual tests approximately 3–12 lines with no nested structs in the body.”
🤖 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/integration/tool_call.rs` around lines 357 - 400, Refactor json_queries_scoped_file_and_adjacent_array_indices so its body only builds the scenario, calls submit_turn, and performs the high-level assertion. Move the large serde_json fixture, scripted replies, repeated assertions, and related setup into focused helper functions, ensuring the test body contains no nested structs and remains approximately 3–12 lines.Source: Path instructions
486-503: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert these query results through the scoped result helper.
assert_tool_result_containsscans every recorded capability result withany(), so"$","value-15"or"1.740..."can satisfy the assertion from another tool result in the same turn. Usetool_result_output("builtin.json")plus a scoped assertion that checks the intended query/path/output, or add a scoped*_sincevariant for the JSON query output.🤖 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/integration/tool_call.rs` around lines 486 - 503, Replace the broad assert_tool_result_contains calls for the JSON query expectations with scoped assertions against tool_result_output("builtin.json"), ensuring each expected path/query value is validated only within the builtin.json result. If the existing helper cannot express this, add and use a JSON-specific *_since scoped variant while preserving the invalid-input error assertion’s intended 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.
Outside diff comments:
In `@tests/integration/tool_call.rs`:
- Around line 406-408: Update the RebornIntegrationHarness setup to register
source_bytes through ScriptedHttpResponse keyed by HTTP_TOOL_URL instead of
using with_real_egress_response_bodies. Preserve the existing real egress
pipeline while ensuring the fixture is selected by URL rather than response
order.
- Around line 357-400: Refactor
json_queries_scoped_file_and_adjacent_array_indices so its body only builds the
scenario, calls submit_turn, and performs the high-level assertion. Move the
large serde_json fixture, scripted replies, repeated assertions, and related
setup into focused helper functions, ensuring the test body contains no nested
structs and remains approximately 3–12 lines.
- Around line 486-503: Replace the broad assert_tool_result_contains calls for
the JSON query expectations with scoped assertions against
tool_result_output("builtin.json"), ensuring each expected path/query value is
validated only within the builtin.json result. If the existing helper cannot
express this, add and use a JSON-specific *_since scoped variant while
preserving the invalid-input error assertion’s intended behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea8bc08f-fb88-467b-a124-de8bd5a1e87b
📒 Files selected for processing (1)
tests/integration/tool_call.rs
|
@ironloopai review |
🔎 Review · PR #7135
Submitted review →Reviewed the complete trusted base-to-head comparison, including the large mainline merge and the feature-specific result paging, provider-call indexing, replay, and subagent-settlement paths. No concrete, actionable defect was identified. The implementation preserves the original pageable reference while retaining inline evidence writes, separates exact provider-call replay from distinct page calls sharing a reference, and carries spawn identity through durable await-edge recovery. Manual command by @think-in-universe · attempt 1 of 3 · completed in 2m 19s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #7135
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison, including the large mainline merge and the feature-specific result paging, provider-call indexing, replay, and subagent-settlement paths. No concrete, actionable defect was identified. The implementation preserves the original pageable reference while retaining inline evidence writes, separates exact provider-call replay from distinct page calls sharing a reference, and carries spawn identity through durable await-edge recovery.
Validation and technical details
- Verified refs/ironloop/base (1f6b56d) against refs/ironloop/head (362b462): 390 files changed, +5,591/-17,693, including merged mainline changes.
- Inspected the complete changed-area inventory and repository/crate guidance; the codebase graph was unavailable, so live-code tracing and targeted rg searches were used as the prescribed fallback.
- Traced result_read completion through inline result staging, sanitized model observation, output evidence, transcript persistence, provider-call-specific lookup indexes, filesystem CAS updates, and in-memory parity.
- Traced spawn provider-call identity from registration and authorization through SubagentThreadMetadata, AwaitedChildSetRecord, AwaitEdge persistence/reconstruction, and exact parent transcript settlement.
- Reviewed the added in-memory, filesystem, composition, runner, loop-host, and two-page integration assertions covering exact replay, shared-reference pagination, legacy fallback, restart behavior, and settlement targeting.
- git diff --check refs/ironloop/base refs/ironloop/head completed successfully, and the worktree remained unchanged.
- Focused Rust tests could not be executed in this review environment because cargo is not installed (/bin/bash: cargo: command not found); test source and supplied CI context were inspected instead.
- Base:
main - Head:
codex/fix-result-read-continuation-refat362b462 - Run:
e9c0fe98-4c05-49df-a3c4-5ab6fd74f654
…rai#7135) * fix(loop): preserve result_read continuation reference Return the original pageable result reference from result_read while retaining inline-only chunk persistence and evidence. Key transcript dedup by provider call so multiple pages can safely share one durable source reference, with regression coverage for replay and a two-page continuation. * test(composition): align result read continuation assertions * fix(loop): pin result updates to provider calls * fix(ci): update subagent metadata fixture * test(composition): keep result lookup helper test-only * test(reborn): restore await-edge fixture compatibility
This branch was successfully deployed
3 participants
Summary
builtin.result_readinstead of exposing the fresh inline-only staging write reference.Change Type
Linked Issue
Related #5838
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— passed in CI viaClippy (all-features); focused all-target/all-feature clippy also passed for both changed crates.cargo build— covered by the focused test and clippy builds; no separate workspace build.result_readintegration (1), in-memory result-reference contracts (9), filesystem result-reference contracts (3), spawn identity (1), await-edge suite (10), focused composition cancellation test (1), and all affected Reborn buckets/integration suites on final head048c4c348.cargo test --features integrationif database-backed or integration behavior changed — not applicable: no database schema/backend integration changed; filesystem persistence is covered directly.048c4c3482968ca1c215748e55853cbb7f473cd1; two expanded continuation cards reused the original reference at offsets 24576 and 49152, replay survived refresh, and cleanup was verified.review-prorpr-shepherd --fixwas run before requesting review — not run; normal review remains pending.Test Strategy
User behavior:
When a large durable tool result requires multiple
result_readcalls, the model receives one usable continuation identity: the original pageable result reference. Feeding that reference andnext_offsetinto the next call returns the next byte-exact page without replay or dedup conflicts.Risk areas:
Tests added or updated:
result_readscenario now performs two pages and feeds page one's surfaced reference and offset verbatim into page two.What the tests prove:
The completed tool outcome and persisted replay envelope retain the original durable reference; each page has the correct offsets, total length, digest/evidence path, and sanitized observation; exact call replay remains idempotent while two provider calls may share one continuation reference. Railway additionally proves the production WebUI caller can reuse the same reference across offsets 24576 and 49152, render the terminal marker, and replay the completed history after refresh.
Commands run:
Railway browser evidence for exact final head: #7135 (comment)
Security Impact
None. Permission, authorization, redaction, secret mediation, sandboxing, and external network behavior are unchanged. Credential-like inline previews remain suppressed.
Reborn Trust-Boundary Checklist
LoopResultRefnow carries the original durable continuation authority through completion.serde(default)fields fail closed or have migration tests: the additive optional spawn provider-call identity defaults to legacy lookup only for pre-existing durable edges; fresh edges persist exact identity, and reconstruction tests cover both shapes.Internal.Database Impact
None. No database migration or schema change. Filesystem transcript lookup adds provider-call-specific index keys alongside the existing generic index and retains compatibility fallback for pre-existing rows.
Blast Radius
builtin.result_readcompletion identity and thread transcript result-reference indexing. A regression could affect multi-page model continuations or replay lookup; inline result persistence, bytes, offsets, and output evidence stay on the existing writer path.Rollback Plan
Revert the review-fix commit, then the original continuation-reference commit if a full rollback is required. Existing data requires no migration rollback: generic result-reference indexes remain written and readable, and provider-call-specific indexes are additive derived lookup entries.
Review Follow-Through
Please focus on the provider-call-specific dedup key and legacy generic-index fallback, especially the distinction between exact replay and a distinct page invocation sharing the same durable source reference. Railway exact-head caller-path and refresh-replay evidence is recorded in the PR conversation.
Review track: C (runtime/persistence behavior)