Repository navigation
fix(host-runtime): classify HTTP error responses as failures #7330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6281128
b78ed32
e055cac
78b95ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ use crate::{ | |
|
|
||
| use super::{ | ||
| first_party_capability_manifest, | ||
| http_output::{HttpDispatchOutput, shape_response}, | ||
| http_output::{HttpDispatchOutput, classify_status, shape_response}, | ||
| input_error, | ||
| }; | ||
|
|
||
|
|
@@ -245,7 +245,9 @@ pub(super) async fn dispatch( | |
| ) | ||
| .await? | ||
| .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); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| classify_status(shaped, status) | ||
| } | ||
|
|
||
| fn method(input: &Value) -> Result<NetworkMethod, FirstPartyCapabilityError> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.