Repository navigation
fix(host-runtime): classify HTTP error responses as failures - #7330
serrrfirat wants to merge 4 commits into
Conversation
github-actions
Bot
added
scope: docs
|
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 (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFirst-party HTTP tools classify HTTP 4xx and 5xx responses as ChangesHTTP outcome handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HTTPServer
participant HTTPDispatcher
participant StatusClassifier
participant ModelVisibleOutcome
HTTPServer->>HTTPDispatcher: Return status, headers, and body
HTTPDispatcher->>HTTPDispatcher: Shape response once
HTTPDispatcher->>StatusClassifier: Classify status and shaped output
alt Status is 400-599
StatusClassifier->>ModelVisibleOutcome: Return OperationFailed with bounded JSON diagnostics
else Other status
StatusClassifier->>ModelVisibleOutcome: Return successful inspectable response
end
🚥 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 |
🧭 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 5m 4s 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: 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/kernel/ironclaw_host_runtime/src/first_party_tools/http.rs`:
- Around line 251-255: Update the serde_json serialization error handling in the
diagnostic construction to bind the serialization error and preserve it through
the existing cause-preserving FirstPartyCapabilityError constructor, rather than
discarding it with map_err(|_| ...). Keep the OutputDecode classification and
network egress usage unchanged.
In `@crates/kernel/ironclaw_host_runtime/tests/first_party_builtin_tools.rs`:
- Around line 4436-4470: Extend the caller-level HTTP error coverage around
builtin_http_surfaces_http_error_status_as_failed_outcome to include a 500
response through the builtin.http.save capability. Assert that the result is
FailureKind::OperationFailed and that its diagnostic JSON reports status 500,
while preserving the existing 403 builtin.http 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: 40787561-5bb0-4622-9502-4c305421c448
📒 Files selected for processing (4)
crates/kernel/ironclaw_host_runtime/src/first_party_tools/http.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}
There was a problem hiding this comment.
🔍 IronLoop review
🟢 No actionable findings
Reviewed the complete merge-base-to-head change across HTTP response shaping, failure propagation, diagnostics, tests, documentation, and runtime-gate wiring. No actionable defects found.
Validation
- ✅ Focused host-runtime HTTP tests — `cargo test -p ironclaw_host_runtime --test first_party_builtin_tools builtin_http_` passed all 46 selected tests.
- ✅ Diff whitespace check — `git diff --check refs/ironloop/merge-base refs/ironloop/head` completed successfully.
- ⚪ Full workspace validation — Not run. Not run because the focused owning-crate suite and existing CI evidence provided proportionate coverage for this four-file change.
Review details
- Run:
71a3ce2c-1dbe-4c85-98c4-efe93de2098f - Workflow: Review
- Attempts: 1
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Classify builtin.http and builtin.http.save HTTP 4xx/5xx responses as recoverable OperationFailed capability outcomes, preserving sanitized response as model-visible diagnostic context while keeping 1xx/2xx/3xx as successful inspectable results.
Shape: normal primary mode + mechanical modifier (auto-computed from preflight rename scan; no renames in this diff — modifier verified clean).
Coverage: complete. Diff source: local-git (exact base...head commits). 4 files: 2 production, 1 tests, 1 docs. No packetization needed (9.4KB diff). Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Stats: 12 findings (from 15 raw, 12 after dedup) across 1 file. 6 inline comment groups (same-line merged).
Security
- Low HTTP error diagnostic truncated to 4096 bytes at model boundary; large error bodies lost (crates/kernel/ironclaw_host_runtime/src/first_party_tools/http.rs:250-263, confidence 75) — MODEL_DIAGNOSTIC_MAX_BYTES=4096 truncation can cut the serialized diagnostic mid-JSON; status/truncation/redaction markers not guaranteed to survive. Safe direction (truncation caps exposure) but partially contradicts the PR goal. Fix: cap/trim before serialization, keep markers first, add >4KiB regression test.
- Low 429/503 responses now model-visible OperationFailed; retry without backoff can amplify load on third-party targets (http.rs:250-263, confidence 55) — model may immediately re-invoke on 429/503; no host auto-retry, per-call bounds mitigate. Fix: backoff guidance in diagnostic for 429/503 (e.g. Retry-After).
- Low Failure verdict now contextualizes auth_hint; 401/403 from any host nudges extension install and credentialed retry (http.rs:250-263, confidence 50) — pre-existing nudge strengthened by failure verdict; install is approval-gated, no direct exfiltration. Fix: consider suppressing auth_hint when no manifest-known audience for target host.
- Nit builtin.http.save: 4xx/5xx body already persisted before OperationFailed is reported (http.rs:256-263, confidence 60) — save-mode body on disk before failure verdict; blind retry may duplicate writes. Fix: document save-before-failure ordering; note "body was saved despite error status" in diagnostic.
Bugs
- Low builtin.http.save 4xx/5xx diagnostic carries no response body (http.rs:250-263, confidence 55) — save-mode diagnostic has status/headers/saved_body but no body_text; doc/claim gap vs PR body. Fix: state in docs that save-mode error body is recovered via read_file, or include bounded preview.
Tests
- Medium builtin.http.save 4xx/5xx classification has no test (http.rs:248-264, confidence 85) — every builtin.http.save test uses status 200; save-mode 403 verdict and body-save-on-failure unpinned. Fix: add save-mode 403 test asserting OperationFailed + saved_body metadata.
- Low No test for 5xx responses though contract claims 4xx and 5xx (http.rs:250-263, confidence 75) — 403 only pins the branch; 5xx claim in docs untested. Fix: add 500-status test.
- Low Status range boundaries 400/599/600 and informational 1xx untested (http.rs:250-265, confidence 60) — off-by-one regression at edges would pass CI. Fix: parametrized boundary test (400, 599 fail; 100, 304, 600 succeed).
- Low New serde_json::to_string OutputDecode error path never exercised (http.rs:251-256, confidence 50) — merged with finding 10 (same code path); unreachable branch unpinned.
Conventions
- Low map_err(||) drops serde_json cause on HTTP diagnostic serialization (http.rs:251-255, confidence 65) — violates .claude/rules/error-handling.md (map_err(||) clause); also flagged by bugs (dead-branch), maintainability (dead-code), tests (unexercised path). Fix: log bound error before mapping; bind usage once. Also flags duplicated ResourceUsage construction (lines 252-254 vs 261-263).
Local Patterns
- Low New 4xx/5xx failure path emits no tracing::debug unlike sibling error mappings (http.rs:256-264, confidence 65) — http_error and sibling sites log kind/status; new path silent. Fix: emit tracing::debug! with dispatch_error_kind + http_status.
- Low HTTP error classification block lacks the WHY comment sibling code carries (http.rs:248-265, confidence 55) — 4xx-vs-3xx rationale only in docs; add one-line WHY comment.
Maintainability
- Low HTTP status-to-outcome policy inlined in translator dispatch, split from status shaping in http_output (http.rs:248-265, confidence 60) — dispatch() claims "translator only" but now holds status policy; duplicated usage construction + re-serialization in error path. Fix: extract classify_status into http_output.rs.
- Nit Unreachable OutputDecode branch: to_string of shaped output cannot fail (http.rs:251-256, confidence 55) — merged with finding 10; dead arm mislabels error kind.
Approach
No findings.
Notes
- Deliberate compatibility change (4xx/5xx no longer successful results) is correctly scoped: 1xx/2xx/3xx unchanged, OperationFailed reuse, no new variants, redaction/credential/allowlist untouched.
- docs/reborn/contracts/host-runtime.md updated and e2e gate wired via run_test_exact in scripts/reborn-e2e-rust.sh; gate currently pins only 403.
- Review packet (stack/shape context) not required for this single-layer PR; body already documents scope, blast radius, rollback.
| .await? | ||
| .map_err(|error| http_error(error, save_mode))?; | ||
| Ok(shape_response(response, response_body_limit)) | ||
| let status = response.status; |
There was a problem hiding this comment.
Medium — builtin.http.save 4xx/5xx classification has no test.
The new error classification branch runs for both builtin.http and builtin.http.save (dispatch is shared). PR title/body explicitly claim both capabilities classify 4xx/5xx as OperationFailed, but every builtin.http.save test uses status 200. Save-mode diagnostic shape differs materially from inline mode: shape_response emits compact saved_body metadata with no body_text, so the model-facing diagnostic and failure verdict for a save-mode 403 are unpinned. Also untested: whether egress saves the error body to disk while the outcome is OperationFailed.
Fix: Add builtin_http_save_surfaces_http_error_status_as_failed_outcome covering builtin.http.save with 403 response and save_to: assert FailureKind::OperationFailed, Diagnostic contains status 403 and saved_body metadata, and strict host egress used.
Low — Status range boundaries 400/599/600 and informational 1xx untested.
The classification is a numeric range (400..=599). Tests cover 200, 302, and 403 only. Lower bound 400, upper bound 599, just-outside 600, and informational 1xx (100/101) are untested, so a future off-by-one or inverted-range regression at the edges would pass CI.
Fix: Add builtin_http_classifies_status_range_boundaries covering statuses 400, 599 (OperationFailed) and 100, 600, 304 (inspectable success) in one parametrized test.
Also flagged by: tests/Low
There was a problem hiding this comment.
Addressed in b78ed32: added builtin_http_save_surfaces_http_error_status_as_failed_outcome asserting OperationFailed + saved_body metadata in the diagnostic + strict host egress usage.
| .map_err(|error| http_error(error, save_mode))?; | ||
| Ok(shape_response(response, response_body_limit)) | ||
| let status = response.status; | ||
| let shaped = shape_response(response, response_body_limit); |
There was a problem hiding this comment.
Nit — builtin.http.save: 4xx/5xx body already persisted before OperationFailed is reported; model blind-retry risks duplicate writes.
In save mode the egress applies body disposition (writes the sanitized body to the scoped mount) before this status check runs, so a 4xx/5xx response is already on disk when OperationFailed is emitted. The diagnostic does carry saved_body metadata (path, bytes_written), so a careful model can see the side effect completed, but a model treating the verdict as 'nothing happened' may re-invoke and duplicate the write.
Fix: Document the save-before-failure ordering in the contract and/or include an explicit 'body was saved despite the error status' note in the diagnostic.
| Ok(shape_response(response, response_body_limit)) | ||
| let status = response.status; | ||
| let shaped = shape_response(response, response_body_limit); | ||
| if (400..=599).contains(&status) { |
There was a problem hiding this comment.
Low — 429/503 responses now model-visible OperationFailed; retry without backoff can amplify load on third-party targets.
Before this diff, an HTTP 429/503 response returned as a successful tool result. Now every 4xx/5xx (including rate-limit 429 and overload 503) maps to OperationFailed whose fate is ModelVisible. The model may re-invoke immediately with no backoff guidance, amplifying request pressure against the target. Prompt-injected agent directed at a victim URL receives 4xx/5xx and retries in a tight loop. Mitigations: no host auto-retry (RetrySameCall not used for OperationFailed), per-call bounds, network_egress accounting on the error, model decides. Real but low-impact behavior change versus base.
Fix: Keep the verdict but include explicit backoff guidance in the diagnostic for 429/503 (e.g., surface Retry-After), or document that the model must not blindly retry rate-limit/overload responses.
Low — Failure verdict now contextualizes auth_hint; 401/403 from any host nudges extension install and credentialed retry.
shape_response already embeds auth_hint (extension_search/extension_install + 'extension injects the required credentials') for 401/403/407. Pre-diff that hint rode a successful result; post-diff it rides a failure diagnostic that also tells the model to retry through the extension's tools. A 401/403 from an attacker-controlled endpoint now produces an OperationFailed verdict plus an install-and-retry nudge the model is more likely to act on. Bounded: extension install is approval-gated and credential mediation is audience-scoped, so no direct secret exfiltration. Diff enables a stronger version of a pre-existing nudge; worth an explicit risk note, not a blocker.
Fix: Consider suppressing auth_hint (or downgrading the nudge) when the request carried no manifest-known audience for the target host.
Also flagged by: security/Low
There was a problem hiding this comment.
Addressed in b78ed32: documented in host-runtime.md that the host never auto-retries and the model should apply backoff for 429/503.
| let status = response.status; | ||
| let shaped = shape_response(response, response_body_limit); | ||
| if (400..=599).contains(&status) { | ||
| let diagnostic = serde_json::to_string(&shaped.output).map_err(|_| { |
There was a problem hiding this comment.
Low — map_err(|_|) drops serde_json cause on HTTP diagnostic serialization.
New line 251 serializes shaped.output and maps failure with map_err(|| ...), discarding the serde_json::Error binding. .claude/rules/error-handling.md: 'A map_err(|| ...) (a closure discarding the error binding) is not silent-ok-exemptible — a comment does not make the dropped cause reappear. Fix it by carrying the cause or by logging the bound error before mapping. Reject the line otherwise.' Sibling instances pre-exist in this file (http.rs:353, 357, 417), but this diff adds a new instance. Practical impact low: shaped.output is built by shape_response from bounded strings/numbers/bools, so to_string cannot realistically fail — however the added line violates the explicit rule, drops the only cause that would explain an OutputDecode outcome, and duplicates the ResourceUsage construction (lines 252-254 and 261-263).
Fix: Log the bound error before mapping and bind the usage value once instead of constructing it twice.
Low — builtin.http.save 4xx/5xx diagnostic carries no response body.
For HttpSaveMode::Required, shape_response inserts only saved_body metadata when response.saved_body is present and never emits body_text/body_base64. The new error path serializes shaped.output as the diagnostic, so a save-mode 4xx/5xx failure diagnostic contains status/headers/saved_body path but no error body. PR body and docs claim the sanitized response stays model-visible; that holds for inline builtin.http but not builtin.http.save, where the model must issue an extra read_file. Behavior is consistent with the existing save-mode compact-metadata design, so this is a doc/claim gap more than a code fault.
Fix: In docs/reborn/contracts/host-runtime.md and the PR body, state that save-mode error diagnostics carry only saved_body path metadata and the error body is recovered via read_file, or include a bounded body preview in the save-mode diagnostic.
Also flagged by: bugs/Low
There was a problem hiding this comment.
Addressed in b78ed32: map_err(|_|) removed; cause logged, fixed fallback diagnostic payload.
| let shaped = shape_response(response, response_body_limit); | ||
| if (400..=599).contains(&status) { | ||
| let diagnostic = serde_json::to_string(&shaped.output).map_err(|_| { | ||
| FirstPartyCapabilityError::new(RuntimeDispatchErrorKind::OutputDecode).with_usage( |
There was a problem hiding this comment.
Low — New 4xx/5xx failure path emits no tracing::debug unlike sibling error mappings.
Every sibling first-party error-mapping site logs at debug with the dispatch kind before returning FirstPartyCapabilityError: http_error logs 'first-party HTTP egress failed' with runtime_http_reason + dispatch_error_kind (http.rs:509-512), skill_management_error and invalid_trigger_input log it too. The new HTTP 4xx/5xx classification returns OperationFailed with a status-specific summary but no host-side debug log.
Fix: Emit a tracing::debug! with dispatch_error_kind and http_status before returning the classified error, matching the sibling idiom in http_error.
There was a problem hiding this comment.
Addressed in b78ed32: classify_status now emits tracing::debug! with http_status + dispatch_error_kind, matching sibling error mappings.
| })?; | ||
| return Err(FirstPartyCapabilityError::dispatch_with_diagnostic( | ||
| RuntimeDispatchErrorKind::OperationFailed, | ||
| Some(format!("HTTP request returned status {status}")), |
There was a problem hiding this comment.
Low — HTTP error diagnostic truncated to 4096 bytes at model boundary; large error bodies lost.
The new failure path serializes the full shaped output (status, auth_hint, headers, body_text) into the Diagnostic detail, but the model boundary truncates diagnostics to MODEL_DIAGNOSTIC_MAX_BYTES=4096 (result_meta.rs:924) via ModelDiagnostic::truncating (resolution.rs:387). Default inline response_body_limit is 48KiB, max 256KiB, so for any typical 4xx/5xx error body the model receives a char-boundary-cut JSON fragment: body largely dropped, and because serde_json::Map sorts keys, the tail keys (status, truncation, redaction_applied markers) can be cut off entirely. Partially contradicts the PR goal 'preserve the bounded, sanitized response as diagnostic context'. Security direction is safe (truncation caps exposure), but the model can misread a partial JSON fragment and the truncation/redaction markers are not guaranteed to survive. The new test uses a tiny body and stops at the invoke_capability seam, so it cannot pin this.
Fix: Cap the inline error diagnostic to MODEL_DIAGNOSTIC_MAX_BYTES before serialization (or trim the shaped body to fit while keeping status/auth_hint/truncation markers first) and add a regression test with a >4KiB error body asserting status and markers survive.
Low — No test for 5xx responses though contract claims 4xx and 5xx.
Branch covers (400..=599), and docs/reborn/contracts/host-runtime.md states 'HTTP 4xx and 5xx responses' are classified as OperationFailed, pinned by builtin_http_surfaces_http_error_status_as_failed_outcome. That test and the scripts/reborn-e2e-rust.sh gate run only a 403 response. A 5xx exercises the same branch but nothing proves the claimed 5xx behavior or that the e2e pin covers the documented claim.
Fix: Add builtin_http_surfaces_server_error_status_as_failed_outcome covering status 500 with body and asserting FailureKind::OperationFailed plus diagnostic carrying body_text.
Low — HTTP status-to-outcome policy inlined in translator dispatch, split from status shaping in http_output.
dispatch() self-describes as 'a translator only', but the new 4xx/5xx -> OperationFailed policy is inlined there while status-dependent shaping already lives in http_output.rs::shape_response (unauthorized_extension_hint for 401/403/407 plus status tests). Status policy for one capability now spans two modules. The inlined branch also constructs ResourceUsage::default().set_network_egress_bytes(...) twice and re-serializes the just-shaped output, duplicating usage bookkeeping inside the error path.
Fix: Extract a small outcome classifier into http_output.rs next to shape_response/unauthorized_extension_hint, e.g. classify_status(shaped, status) returning Ok for 1xx-3xx and building the diagnostic error once for 4xx/5xx; dispatch then ends with classify_status(shape_response(response, response_body_limit), response.status), deleting the duplicated usage construction, the dead map_err branch, and keeping status policy in one module.
Low — HTTP error classification block lacks the WHY comment sibling code carries.
The 4xx/5xx-vs-3xx split is the crux of this change (transport completion is not capability success; redirects stay inspectable and never followed), yet the block carries no comment. This file's idiom is dense WHY comments. The rationale lives only in docs/reborn/contracts/host-runtime.md; a future reader of dispatch() gets no in-code signal that drawing the failure line at 400 is deliberate and that 3xx must stay successful.
Fix: Add a one-line WHY comment above the status check, matching the file's comment idiom.
Also flagged by: tests/Low
Also flagged by: maintainability/Low
Also flagged by: local-patterns/Low
There was a problem hiding this comment.
Addressed in b78ed32: bounded_failure_diagnostic trims to MODEL_DIAGNOSTIC_MAX_BYTES before serialization; regression test builtin_http_error_diagnostic_respects_model_diagnostic_budget added.
…add status regression tests (nearai#7330)
Addressed multi-agent review feedbackRound 1 review (6 inline threads + body summary) addressed in commit Code —
Tests (4 new integration + 3 new unit):
Gate wiring: Docs — Validation: Deferred: suppressing |
github-actions
Bot
added
size: L
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 `@docs/reborn/contracts/host-runtime.md`:
- Around line 108-116: The host-runtime documentation incorrectly implies that a
later builtin.http.save call can retrieve a body from a prior failed request.
Update the saved-body sequence to state that callers must invoke
builtin.http.save on the original request; only that call writes saved_body
metadata, which builtin.read_file can subsequently read.
🪄 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: 1ddb9afd-4817-4ca2-b25c-5e103042b550
📒 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
|
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 — BLOCKEDTested head SHA: Given/When/Then matrix
Status derivation
Diagnostics
Remaining risks / blockers
Skipped cases
Cleanup status
|
|
Reopened from the main repository as #7342 (same head commit |
Pull request was closed
* fix(host-runtime): classify HTTP error responses as failures * fix(host-runtime): address review — bound HTTP error diagnostics and add status regression tests (#7330) * docs(host-runtime): correct saved-body retrieval sequence for failed HTTP calls (#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 (#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).
…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
1 participant
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)