fix(host-runtime): classify HTTP error responses as failures - #7342
Merged
Merged
Conversation
…add status regression tests (#7330)
|
🚅 Deployed to the ironclaw-pr-7342 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 13:11
Destroyed
serrrfirat
added
size: L
16 of 29 tasks
📝 WalkthroughSummary by CodeRabbit
WalkthroughFirst-party HTTP tools classify 4xx and 5xx responses as ChangesHTTP status classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPDispatch
participant classify_status
participant FirstPartyCapabilityError
HTTPDispatch->>classify_status: shaped response, status, and wall-clock time
classify_status->>FirstPartyCapabilityError: 4xx/5xx bounded diagnostic
classify_status-->>HTTPDispatch: success or OperationFailed result
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 |
Contributor
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 7m 36s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs`:
- Around line 139-170: Update the trimming loop around the inline body branches
so body_text or body_base64 is selected only when its current content can still
shrink; once it reaches zero length, continue to the headers branch instead of
repeatedly selecting the empty body. Preserve the existing truncation
bookkeeping, then add a caller-level regression test covering an oversized
response with a body that trims to empty and headers exceeding
MODEL_DIAGNOSTIC_MAX_BYTES, verifying headers and other diagnostic fields are
retained rather than returning fallback_diagnostic.
In `@docs/reborn/contracts/host-runtime.md`:
- Around line 110-118: Update the save-mode body contract in the surrounding
documentation to state that the original sanitized body is retained only when
the initial call uses builtin.http.save, and only up to the existing save-mode
limit. Clarify that the failure diagnostic replaces only body content with
saved_body metadata; preserve status, auth_hint, and truncation metadata in the
diagnostic, and remove the implication that re-issuing the request is the normal
inspection mechanism.
- Around line 119-121: Update the retry guidance in the ToolRecoveryObservation
contract paragraph to document that 429/503 failures may populate retry_after_ms
with provider-delay metadata. Preserve the existing backoff requirement and
explicitly retain that retry_after_ms being None does not permit an immediate
retry.
🪄 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: c4f9d1fe-450a-4bd2-84ee-3a4381565230
📒 Files selected for processing (5)
crates/kernel/ironclaw_host_runtime/src/first_party_tools/http.rscrates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rscrates/kernel/ironclaw_host_runtime/tests/first_party_builtin_tools.rsdocs/reborn/contracts/host-runtime.mdscripts/reborn-e2e-rust.sh
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🔍 IronLoop review
Found 1 medium-severity model-visible diagnostic truncation issue.
Findings: 🟠 Medium 1
🟠 Medium · Reserve space for the downstream safety envelope
Inline on crates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs:120. See the inline comment for details.
Validation
- ✅ Focused HTTP integration tests — `cargo test -p ironclaw_host_runtime --test first_party_builtin_tools builtin_http_` passed: 50 tests.
- ✅ HTTP output unit tests — `cargo test -p ironclaw_host_runtime --lib first_party_tools::http_output::tests` passed: 5 tests.
- ✅ Static checks — `cargo fmt --all -- --check`, `git diff --check`, and `bash -n scripts/reborn-e2e-rust.sh` passed.
Review details
- Run:
bf109816-7a5f-498d-b8b6-220736c01264 - Workflow: Review
- Attempts: 1
| return fallback_diagnostic(status); | ||
| }; | ||
| let mut output = output.clone(); | ||
| if serialized_output_len(&output) <= MODEL_DIAGNOSTIC_MAX_BYTES { |
Contributor
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Reserve space for the downstream safety envelope
The 4 KiB cap is applied before the diagnostic enters the loop’s model-visible scrubber. If an error body contains an injection pattern, that scrubber wraps the entire JSON diagnostic in a several-hundred-byte external-content notice, and the resolution boundary then truncates it back to 4 KiB. For a large 4xx/5xx body this cuts the wrapped JSON before its tail fields (`status` and `truncation` are serialized near the end), so the model does not receive the complete valid JSON/context this change promises. Cover an injection-bearing, near-limit HTTP error through the loop resolution boundary and trim with enough headroom (or preserve the structured fields after fencing).
Collaborator
Author
Railway preview QA — PASSTested head SHA: Given/When/Then matrix
Status derivation
The deployed build demonstrably contains the PR change: old code reported every received HTTP response as a successful result; the preview reports received 403 responses as Environment notes (not PR behavior)
Cleanup status
Skipped cases
|
…cover header/base64 branches - share one budget-trim engine (fit_output_to_budget) between the success path and the failure diagnostic; the diagnostic trim now converges in a strict-progress loop instead of an 8-iteration cap, and the headers branch stays reachable when the inline body is empty or absent, so header-heavy 4xx/5xx diagnostics keep status/auth_hint/truncation instead of collapsing to the fallback verdict - scrub DEL/C1 control bytes (U+007F..U+009F) from the serialized diagnostic: serde_json does not escape them, and ModelDiagnostic validation would otherwise replace the whole diagnostic with the fixed fallback sentence at the resolution boundary - move the shaped output by value instead of cloning it per error response - add unit coverage for the empty-body/header trim, base64 alignment on the failure path, and control-char scrubbing; wire the redirect regression test into the architecture-runtime gate
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 17:09
Destroyed
…ck, pin envelope size - shape 4xx/5xx responses at the 4 KiB diagnostic budget instead of the success inline limit so the discarded success-budget trim pass (and its serializations) no longer runs on the failure path - attach dispatch wall_clock_ms to the OperationFailed usage, matching the sibling first-party dispatches' failure-path accounting - pin the truncation envelope below its reserved budget with a unit test - correct the fit_output_to_budget doc to describe the incremental re-measure loop rather than a fixed three-serialization bound
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 17:39
Destroyed
…lback shape - single is_error_status predicate shared by the dispatch shape-limit selection and classify_status, so the 400..=599 boundary cannot drift - pin failure usage accounting: classify_status unit test asserts egress bytes + wall_clock_ms on the OperationFailed outcome; the 403 integration test asserts egress bytes reach the governor for failed calls - pin the fallback diagnostic payload shape for non-object output and note why the serde-failure branch is unreachable by construction
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 18:13
Destroyed
…ed-outcome contract - reborn_integration_http_matcher asserted the pre-change contract that a scripted HTTP 500 surfaces as a successful tool result; the new classification makes it a recoverable OperationFailed outcome, so the test now asserts ToolErrorClass::Failed with the operation_failed kind (run still completes; docstring updated to the contract doc) - pin the post-fit fallback safety valve with an oversized untrimmable key test; correct the governor-accounting comment; dedupe the 400 boundary rationale onto is_error_status
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 18:40
Destroyed
…cument fence interaction - tests/integration/CLAUDE.md .with_status entry now states 4xx/5xx classify as a Failed tool outcome (operation_failed) with sanitized diagnostic context; other statuses remain Completed results - host-runtime contract doc records the loop-host injection-fence interaction: verdict semantics never depend on the fenced diagnostic surviving the observation bound (OperationFailed + safe summary always reach the model) - document the fit_output_to_budget convergence bound (<= 3 passes)
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 18:59
Destroyed
serde_json serializes every Value string without revalidating UTF-8 (probed: even an unsafe lone-surrogate string serializes Ok), so the serde-failure arm is a pure defensive guard for future Value shapes, not a reachable lone-surrogate path. Correct the doc comment and the fallback test note to state the empirical fact.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 19:24
Destroyed
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 (2)
crates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs (2)
617-662: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert decoded byte metadata for base64 trimming.
This test checks four-character alignment but not
truncation.bytes_returned. That field is byte-based. Assert that it equals the decoded length oftrimmed_base64; otherwise an encoded-character count can pass this test.Proposed assertion
assert_eq!( trimmed_base64.len() % 4, 0, "trimmed base64 must stay multiple-of-4 aligned" ); + let decoded_bytes = BASE64_STANDARD + .decode(trimmed_base64) + .expect("trimmed base64 must decode") + .len(); + assert_eq!( + parsed["truncation"]["bytes_returned"], + json!(decoded_bytes), + "truncation bytes must count decoded body bytes" + );🤖 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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs` around lines 617 - 662, Update failure_diagnostic_trims_base64_body_with_large_headers to assert that truncation.bytes_returned equals the decoded byte length of trimmed_base64, using the existing base64 decoding support. Keep the current four-character alignment and JSON validity assertions unchanged.
699-719: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a non-zero request size in the usage regression.
response_with_status(403)setsrequest_bytesto0. The assertion can pass if failure handling always reports zero egress. Set a non-zero request size and assert thatusage.network_egress_bytespreserves it.Proposed test adjustment
- let shaped = shape_response(response_with_status(403), 48 * 1024); + let mut response = response_with_status(403); + response.request_bytes = 17; + let shaped = shape_response(response, 48 * 1024); ... - usage.network_egress_bytes, 0, - "empty GET request carries no egress bytes" + usage.network_egress_bytes, 17, + "failure must preserve request egress bytes"🤖 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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs` around lines 699 - 719, Update classify_status_failure_carries_egress_and_wall_clock_usage to use a non-zero request size when creating the 403 response, then assert usage.network_egress_bytes equals that request size while retaining the wall-clock assertion and successful 2xx check.
🤖 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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs`:
- Around line 685-690: Rename
failure_diagnostic_falls_back_on_unserializable_output to reflect that it tests
bounded_failure_diagnostic’s non-object fallback for serde_json::Value::Null,
not an unserializable output or serializer-error path.
---
Outside diff comments:
In `@crates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs`:
- Around line 617-662: Update
failure_diagnostic_trims_base64_body_with_large_headers to assert that
truncation.bytes_returned equals the decoded byte length of trimmed_base64,
using the existing base64 decoding support. Keep the current four-character
alignment and JSON validity assertions unchanged.
- Around line 699-719: Update
classify_status_failure_carries_egress_and_wall_clock_usage to use a non-zero
request size when creating the 403 response, then assert
usage.network_egress_bytes equals that request size while retaining the
wall-clock assertion and successful 2xx check.
🪄 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: 1d95ac69-89fe-4b9b-90fa-23f6c4b53b02
📒 Files selected for processing (1)
crates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs
…ostic envelope The re-inserted truncation envelope carried only the diagnostic-budget trim state, so a 4xx/5xx response whose shape stage had already marked headers or body as truncated (e.g. more than 32 headers) could end up with headers_truncated:true beside an envelope claiming headers:false. OR the surviving keys into the envelope and pin with a regression test; boundary doc now states the full complement (outside 400..=599 stays inspectable, including out-of-spec 600+).
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 19:46
Destroyed
github-actions
Bot
added
size: XL
…t, fix trim rationale - tests/integration/support/http_matcher.rs module doc still claimed .with_status non-2xx stays Completed; now documents 4xx/5xx as a model-visible Failed operation_failed outcome - bounded_failure_diagnostic doc: head-keeping truncation cuts only the last-sorted keys (status, truncation envelope); auth_hint sorts first and survives
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 7, 2026 19:57
Destroyed
serrrfirat
commented
Aug 7, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Reclassify builtin.http and builtin.http.save HTTP 4xx/5xx responses from successful results into recoverable OperationFailed outcomes carrying sanitized diagnostic context.
Shape: normal primary mode, no modifiers (5-file PR reviewed against exact local diff; 8-file surface by final round after doc/test sync).
Coverage: complete — local-git diff source, 8 files (2 production, 3 tests, 2 docs, 1 CI), no packetization, 0 failed reviewers, 0 limitations.
Stats: 0 findings at head efa76f7ef0 (18 findings across 9 review-fix rounds, all fixed and re-verified). Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Fix-loop history (all verified fixed at head)
- Round 1 (8 findings): headers-trim branch unreachable with empty
body_text→ sharedfit_output_to_budgetwith independent branches; DEL/C1 control chars surviving serde escaping →scrub_model_diagnostic_controlson both diagnostic paths; up-to-8x re-serialization → convergent strict-progress loop; per-error Map clone → move by value; untested base64/headers trim branches → 2 new unit tests; duplicated safety comment → removed; duplicate trim engine → single shared pass; missing gate wiring → redirect test added to architecture-runtime gate. - Round 2 (4): envelope budget unpinned →
truncation_envelope_fits_its_reserved_budget; discarded success-budget trim on error path → shape 4xx/5xx atMODEL_DIAGNOSTIC_MAX_BYTES; missing wall_clock on failure usage →set_wall_clock_msinclassify_status; stale doc bound → rewritten. - Round 3 (3): failure usage never asserted → unit pin + governor egress assertion; fallback payload unpinned → test; duplicated 400..=599 predicate → single
is_error_status. - Round 4 (1 HIGH + 3): stale
reborn_integration_http_matcher5xx-as-success contract test (broke on this head) → migrated toFailed/operation_failedcontract; misleading governor comment → corrected; post-fit fallback branch → test; duplicated boundary doc → deduped. - Round 5 (3): stale
tests/integration/CLAUDE.md.with_statusdoc → synced; injection-fence overflow interaction → documented in contract (verdict independent of fenced diagnostic); loop pass-count concern → convergence bound documented (≤3 passes). - Round 6 (1): serde-failure arm "untested" → probed empirically (serde_json serializes every
Valuestring without revalidating UTF-8; arm is a pure defensive guard, no in-process test can exercise it without UB) → doc corrected. - Round 7 (2): envelope dropped shape-stage truncation flags → OR surviving keys, pinned by 33-header regression test; boundary doc omitted 600+ tail → complement stated.
- Round 8 (2): stale support-module doc → synced; trim-rationale doc overstated cut list → corrected (auth_hint sorts first and survives).
- Round 9: zero findings from all 8 reviewers.
Validation
cargo test -p ironclaw_host_runtime --lib first_party_tools::http_output— 13 passedcargo test -p ironclaw_host_runtime --test first_party_builtin_tools builtin_http_— 50 passedcargo test -p ironclaw_integration_tests --test reborn_integration_http_matcher— 22 passed (incl. migrated 5xx test)cargo clippy -p ironclaw_host_runtime --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— clean
Remaining risks (documented, accepted)
- Injection-fenced error diagnostics may be tail-truncated at the loop-host observation bound; verdict + safe summary are independent (documented in
docs/reborn/contracts/host-runtime.md). serialize_diagnosticserde-failure arm is an untestable defensive guard (serde_json cannot fail onValueserialization; probed).- Save-mode failure writes the sanitized body before the verdict; retry may duplicate the write (documented).
Collaborator
|
@ironloopai review |
Contributor
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Manual command by think-in-universe · attempt 1 of 3 · completed in 20m 1s IronLoop completed the review and posted it to GitHub. 🔗 Result |
think-in-universe
previously approved these changes
Aug 10, 2026
Contributor
There was a problem hiding this comment.
🔍 IronLoop review
Found one medium and one low correctness issue in failed-response diagnostic handling.
Findings: 🟠 Medium 1 · 🟡 Low 1
🟠 Medium · Preserve the original egress cap when shaping failed responses
Inline on crates/kernel/ironclaw_host_runtime/src/first_party_tools/http.rs:262. See the inline comment for details.
🟡 Low · Retain saved-body evidence when the diagnostic falls back
Inline on crates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs:142. See the inline comment for details.
Validation
- ✅ Diff hygiene — No whitespace errors were reported for the pull request diff.
- ⚪ Focused test execution — Not run. Not run; static tracing of the changed classifier through the partial-response egress and body-store paths was sufficient to establish the findings.
Review details
- Run:
ce15715d-1554-431d-9649-ec5ca84b389f - Workflow: Review
- Attempts: 1
Comment on lines
+257
to
+262
| let shape_limit = if is_error_status(status) { | ||
| u64::try_from(MODEL_DIAGNOSTIC_MAX_BYTES).unwrap_or(response_body_limit) | ||
| } else { | ||
| response_body_limit | ||
| }; | ||
| let shaped = shape_response(response, shape_limit); |
Contributor
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Preserve the original egress cap when shaping failed responses
This replaces the caller’s cap with 4 KiB before `shape_response`, which uses that argument to determine whether the egress already truncated the body. The model-visible egress returns a partial response when a body crosses the configured cap: a 403 with `response_body_limit: 1` and a longer body has one returned byte and `response_bytes = 2`, but is shaped against 4096 and loses `body_truncated` and the truncation envelope. The model consequently sees a partial error body as complete. Keep the original egress cap for truncation accounting and apply the diagnostic display budget separately.
Comment on lines
+140
to
+142
| let trim = fit_output_to_budget(&mut output, final_budget); | ||
| if serialized_output_len(&output) > final_budget { | ||
| return fallback_diagnostic(status); |
Contributor
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟡 Low · Retain saved-body evidence when the diagnostic falls back
`save_to` accepts inputs up to the general 1 MiB tool-input limit and `ScopedPath` has no path-length bound. A long accepted save target can make `saved_body.path` push a 4xx/5xx diagnostic over 4 KiB; the trim routine only reduces bodies and headers, so this branch replaces the entire detail with the fallback even though `builtin.http.save` already wrote the response. The failed outcome then omits both the destination and `bytes_written`, undermining the new guidance to inspect the saved body before retrying. Bound the save path or retain compact saved-body metadata in the fallback.
…p, fence headroom, saved-body fallback (#7342) - shape failed responses at the caller's response_body_limit again so egress-truncation accounting (body_was_truncated_by_egress) stays correct; the diagnostic display budget is applied separately. Pinned by builtin_http_error_diagnostic_preserves_egress_truncation_flag. - reserve MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES so a diagnostic wrapped in the loop-host external-content fence still fits the observation budget; pinned by failure_diagnostic_stays_within_budget_when_fenced. - retain compact saved_body evidence (bounded path prefix + bytes_written) in the fallback verdict instead of dropping the save destination; pinned by failure_diagnostic_fallback_retains_saved_body_evidence. - rename failure_diagnostic_falls_back_on_unserializable_output to the non-object contract it actually exercises (Value::Null serializes). - host-runtime contract: correct the save-mode body-retention wording and document retry_after_ms semantics (None does not permit immediate retry).
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7342
August 10, 2026 10:37
Destroyed
Collaborator
Author
|
Addressed review feedback at head
Validation at head: 15 Note: the head moved after the prior approval, so re-review is welcome (bots re-run automatically on push). |
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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs`:
- Around line 140-145: Update bounded_failure_diagnostic so its early return
only applies when serialized_output_len(&output) is at most
MODEL_DIAGNOSTIC_MAX_BYTES minus MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES; larger
diagnostics must continue through fit_output_to_budget. Add a caller-level
regression at the nearest observation seam that triggers fencing for a
diagnostic in this range and verifies the final observation is bounded and
complete.
🪄 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: d544b7c1-b2f3-4f77-9383-d9072bc86069
📒 Files selected for processing (4)
crates/kernel/ironclaw_host_runtime/src/first_party_tools/http.rscrates/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rscrates/kernel/ironclaw_host_runtime/tests/first_party_builtin_tools.rsdocs/reborn/contracts/host-runtime.md
Comment on lines
+140
to
+145
| fn bounded_failure_diagnostic(output: Value, status: u16) -> String { | ||
| let Value::Object(mut output) = output else { | ||
| return fallback_diagnostic(status, None); | ||
| }; | ||
| if serialized_output_len(&output) <= MODEL_DIAGNOSTIC_MAX_BYTES { | ||
| return scrub_model_diagnostic_controls(serialize_diagnostic(&output, status)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reserve fence headroom before the early return.
Line 144 returns a 3–4 KiB diagnostic without fitting it to the fenced budget. If the diagnostic triggers the external-content fence, the fence pushes it past MODEL_DIAGNOSTIC_MAX_BYTES. The observation boundary can then truncate the diagnostic.
Only return early when the serialized output fits below MODEL_DIAGNOSTIC_MAX_BYTES - MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES. Otherwise, use fit_output_to_budget. Add a caller-level regression that fences a diagnostic in this range and verifies the final observation remains bounded and complete.
Proposed fix
fn bounded_failure_diagnostic(output: Value, status: u16) -> String {
let Value::Object(mut output) = output else {
return fallback_diagnostic(status, None);
};
- if serialized_output_len(&output) <= MODEL_DIAGNOSTIC_MAX_BYTES {
+ let unfenced_budget =
+ MODEL_DIAGNOSTIC_MAX_BYTES.saturating_sub(MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES);
+ if serialized_output_len(&output) <= unfenced_budget {
return scrub_model_diagnostic_controls(serialize_diagnostic(&output, status));
}As per coding guidelines, “For new or changed production-wired behavior, add a caller-level test at the nearest meaningful seam.” As per path instructions, “Test through the caller” applies when a helper 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/kernel/ironclaw_host_runtime/src/first_party_tools/http_output.rs`
around lines 140 - 145, Update bounded_failure_diagnostic so its early return
only applies when serialized_output_len(&output) is at most
MODEL_DIAGNOSTIC_MAX_BYTES minus MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES; larger
diagnostics must continue through fit_output_to_budget. Add a caller-level
regression at the nearest observation seam that triggers fencing for a
diagnostic in this range and verifies the final observation is bounded and
complete.
Sources: Coding guidelines, Path instructions
BenKurrek
approved these changes
Aug 10, 2026
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…7342) * fix(host-runtime): classify HTTP error responses as failures * fix(host-runtime): address review — bound HTTP error diagnostics and add status regression tests (nearai#7330) * docs(host-runtime): correct saved-body retrieval sequence for failed HTTP calls (nearai#7330) * fix(host-runtime): converge failure-diagnostic trim, scrub controls, cover header/base64 branches - share one budget-trim engine (fit_output_to_budget) between the success path and the failure diagnostic; the diagnostic trim now converges in a strict-progress loop instead of an 8-iteration cap, and the headers branch stays reachable when the inline body is empty or absent, so header-heavy 4xx/5xx diagnostics keep status/auth_hint/truncation instead of collapsing to the fallback verdict - scrub DEL/C1 control bytes (U+007F..U+009F) from the serialized diagnostic: serde_json does not escape them, and ModelDiagnostic validation would otherwise replace the whole diagnostic with the fixed fallback sentence at the resolution boundary - move the shaped output by value instead of cloning it per error response - add unit coverage for the empty-body/header trim, base64 alignment on the failure path, and control-char scrubbing; wire the redirect regression test into the architecture-runtime gate * fix(host-runtime): shape errors at diagnostic budget, attach wall clock, pin envelope size - shape 4xx/5xx responses at the 4 KiB diagnostic budget instead of the success inline limit so the discarded success-budget trim pass (and its serializations) no longer runs on the failure path - attach dispatch wall_clock_ms to the OperationFailed usage, matching the sibling first-party dispatches' failure-path accounting - pin the truncation envelope below its reserved budget with a unit test - correct the fit_output_to_budget doc to describe the incremental re-measure loop rather than a fixed three-serialization bound * fix(host-runtime): pin error-status predicate, failure usage, and fallback shape - single is_error_status predicate shared by the dispatch shape-limit selection and classify_status, so the 400..=599 boundary cannot drift - pin failure usage accounting: classify_status unit test asserts egress bytes + wall_clock_ms on the OperationFailed outcome; the 403 integration test asserts egress bytes reach the governor for failed calls - pin the fallback diagnostic payload shape for non-object output and note why the serde-failure branch is unreachable by construction * fix(host-runtime): migrate stale 5xx-success integration test to failed-outcome contract - reborn_integration_http_matcher asserted the pre-change contract that a scripted HTTP 500 surfaces as a successful tool result; the new classification makes it a recoverable OperationFailed outcome, so the test now asserts ToolErrorClass::Failed with the operation_failed kind (run still completes; docstring updated to the contract doc) - pin the post-fit fallback safety valve with an oversized untrimmable key test; correct the governor-accounting comment; dedupe the 400 boundary rationale onto is_error_status * docs(host-runtime): sync matcher guide to failed-outcome contract, document fence interaction - tests/integration/CLAUDE.md .with_status entry now states 4xx/5xx classify as a Failed tool outcome (operation_failed) with sanitized diagnostic context; other statuses remain Completed results - host-runtime contract doc records the loop-host injection-fence interaction: verdict semantics never depend on the fenced diagnostic surviving the observation bound (OperationFailed + safe summary always reach the model) - document the fit_output_to_budget convergence bound (<= 3 passes) * docs(host-runtime): correct serialize_diagnostic guard rationale serde_json serializes every Value string without revalidating UTF-8 (probed: even an unsafe lone-surrogate string serializes Ok), so the serde-failure arm is a pure defensive guard for future Value shapes, not a reachable lone-surrogate path. Correct the doc comment and the fallback test note to state the empirical fact. * fix(host-runtime): keep shape-stage truncation flags in failure-diagnostic envelope The re-inserted truncation envelope carried only the diagnostic-budget trim state, so a 4xx/5xx response whose shape stage had already marked headers or body as truncated (e.g. more than 32 headers) could end up with headers_truncated:true beside an envelope claiming headers:false. OR the surviving keys into the envelope and pin with a regression test; boundary doc now states the full complement (outside 400..=599 stays inspectable, including out-of-spec 600+). * docs(host-runtime): sync support-module doc to failed-outcome contract, fix trim rationale - tests/integration/support/http_matcher.rs module doc still claimed .with_status non-2xx stays Completed; now documents 4xx/5xx as a model-visible Failed operation_failed outcome - bounded_failure_diagnostic doc: head-keeping truncation cuts only the last-sorted keys (status, truncation envelope); auth_hint sorts first and survives * fix(host-runtime): address coderabbitai/ironloopai review — egress cap, fence headroom, saved-body fallback (nearai#7342) - shape failed responses at the caller's response_body_limit again so egress-truncation accounting (body_was_truncated_by_egress) stays correct; the diagnostic display budget is applied separately. Pinned by builtin_http_error_diagnostic_preserves_egress_truncation_flag. - reserve MODEL_DIAGNOSTIC_FENCE_HEADROOM_BYTES so a diagnostic wrapped in the loop-host external-content fence still fits the observation budget; pinned by failure_diagnostic_stays_within_budget_when_fenced. - retain compact saved_body evidence (bounded path prefix + bytes_written) in the fallback verdict instead of dropping the save destination; pinned by failure_diagnostic_fallback_retains_saved_body_evidence. - rename failure_diagnostic_falls_back_on_unserializable_output to the non-object contract it actually exercises (Value::Null serializes). - host-runtime contract: correct the save-mode body-retention wording and document retry_after_ms semantics (None does not permit immediate retry).
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
builtin.httpandbuiltin.http.saveHTTP 4xx/5xx responses as recoverableOperationFailedcapability outcomesChange Type
Linked Issue
None. Reproduced from an exported production thread where an HTTP 403 was reported to the model as a successful tool result.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildbuiltin_http_*tests and the completearchitecture-runtimegatecargo test -p <owning-crate> --features integrationif database-backed or runtime-integration behavior changed (the rootintegrationfeature is empty — the flag is per-crate)review-prorpr-shepherd --fixwas run before requesting reviewAdditional scoped validation:
cargo clippy -p ironclaw_host_runtime --all-targets --all-features -- -D warningsgit diff --checkbash -n scripts/reborn-e2e-rust.shTest Strategy
User behavior:
Given an authorized
builtin.httpcall whose server returns HTTP 403, when the response reaches the capability boundary, then the model receives a recoverable failed tool outcome with the sanitized status, body, and authentication hint instead of a successful tool result. HTTP redirects remain inspectable results.Risk areas:
Tests added or updated:
HostRuntime::invoke_capabilityseam and does not require model orchestrationscripts/reborn-e2e-rust.sh architecture-runtimeWhat the tests prove:
FailureKind::OperationFailedCommands run:
cargo fmt --all -- --checkcargo test -p ironclaw_host_runtime --test first_party_builtin_tools builtin_http_cargo clippy -p ironclaw_host_runtime --all-targets --all-features -- -D warningsscripts/reborn-e2e-rust.sh architecture-runtimegit diff --checkbash -n scripts/reborn-e2e-rust.shSecurity Impact
Changes model-visible network tool execution semantics but does not change permissions, allowlists, credential injection, redaction, or transport policy. HTTP error bodies continue through the existing bounded, sanitized diagnostic channel and are re-scrubbed and injection-fenced before model observation.
Reborn Trust-Boundary Checklist
OperationFailedand diagnostic contracts are reusedscripts/reborn-e2e-rust.sh architecture-runtimepassed using the existingOperationFailedmappingserde(default)fields fail closed or have migration tests: Not applicable; no serialized fields changedTransient,Permanent,Misconfigured,PolicyDeniedor equivalent): HTTP 4xx/5xx consistently use existingOperationFailedDatabase Impact
None.
Blast Radius
builtin.httpandbuiltin.http.saveresponse classification only. Clients that intentionally inspect 4xx/5xx responses now receive a recoverable failed capability outcome rather than success, but retain the sanitized response as diagnostic context. 1xx, 2xx, and 3xx behavior is unchanged.Rollback Plan
Revert this commit to restore the previous behavior where every received HTTP response was returned as a successful capability result. No data migration or persisted schema rollback is required.
Review Follow-Through
Reviewer judgment requested on the deliberate compatibility change for expected 4xx statuses such as 404 or 409. The model retains the full bounded diagnostic and can continue, but the verdict is now correctly non-successful.
Review track: C (runtime error semantics)