feat(api): adopt RFC 9457 problem+json as the canonical HTTP error format#1442
Open
JesseTheRobot wants to merge 3 commits into
Open
feat(api): adopt RFC 9457 problem+json as the canonical HTTP error format#1442JesseTheRobot wants to merge 3 commits into
JesseTheRobot wants to merge 3 commits into
Conversation
…rmat Foundation for the HTTP interface conventions work (step 1 of 4): - Add shared Problem/ProblemItem types to irys-types so the server and every client import one error shape - Map every ApiError variant to (status, type, title, code) and render application/problem+json with a traceId (W3C trace id from the active OTEL span) plus a deprecated `error` mirror of `detail` for legacy parsers (removed in a future release) - Genericize 5xx detail so internal error messages never leak; the real error is logged server-side keyed by traceId - Route extractor failures (JSON body, path, query) and unmatched routes through ApiError. Note: malformed path/query params now return 400 (INVALID_PATH_PARAMETER / INVALID_QUERY_PARAMETER) instead of the actix defaults 404/422 - Add normalize_problem middleware backstop: synthesizes problem+json for empty/text framework error bodies (405/413/panics) and stamps the traceparent header on all responses. Structured application/* bodies are never clobbered, preserving the /execution-rpc JSON-RPC passthrough - Design doc with the steps 2-4 roadmap (utoipa OpenAPI generation + /v1/openapi.json + docs UI, remaining route annotation, TUI client collapse) in docs/superpowers/specs/
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements RFC 9457 (Problem Details) error handling for the Irys API server. It adds shared ChangesRFC 9457 Problem Details Error Handling
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
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/api-server/src/conventions.rs`:
- Around line 95-103: The synthesized RFC 9457 response built in the
should_synthesize_problem path loses protocol headers because res.into_parts()
discards the original response; update the code after calling res.into_parts()
(where you create problem via problem_from_status and build the new HttpResponse
with PROBLEM_JSON) to copy headers from _discarded.headers() into the new
response.headers_mut() before stamp_traceparent, skipping or overriding
Content-Type and Content-Length (and any other intentionally replaced headers)
so protocol-significant headers from the original response are preserved when
returning ServiceResponse::new(req, response).
- Around line 158-159: Replace the non-standard #[actix_web::test] attributes on
async tests (e.g., the malformed_json_body_renders_problem test and the other
async test functions in this file currently annotated with #[actix_web::test])
with the repository-standard async test macro: use #[tokio::test] for plain
async tests, or #[test_log::test(tokio::test)] if you want test-logging/tracing
output; update the test attribute lines accordingly so the tests run under the
crate's preferred test harness.
In `@crates/api-server/src/lib.rs`:
- Line 5: The crate currently exposes `trace` publicly via `pub mod trace;`
which expands the public API unnecessarily; change the declaration to a private
module (`mod trace;`) so internal references like `crate::trace` in `error.rs`
continue to work while preventing external consumers from depending on
`irys_api_server::trace`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfe464b2-576b-4714-913d-f12d7366ea37
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/api-server/Cargo.tomlcrates/api-server/src/conventions.rscrates/api-server/src/error.rscrates/api-server/src/lib.rscrates/api-server/src/trace.rscrates/types/src/lib.rscrates/types/src/problem.rsdocs/superpowers/specs/2026-06-03-http-interface-conventions-design.md
Comment on lines
+158
to
+159
| #[actix_web::test] | ||
| async fn malformed_json_body_renders_problem() { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file and read the relevant lines
head -250 crates/api-server/src/conventions.rs | tail -100Repository: Irys-xyz/irys
Length of output: 3980
🏁 Script executed:
# Get more context around the lines mentioned
sed -n '150,250p' crates/api-server/src/conventions.rsRepository: Irys-xyz/irys
Length of output: 4042
🏁 Script executed:
# Check if there are any other test macros used in the same file
rg -n '#\[.*test.*\]' crates/api-server/src/conventions.rsRepository: Irys-xyz/irys
Length of output: 320
🏁 Script executed:
# Check CLAUDE.md or similar documentation
find . -name "CLAUDE.md" -o -name "README*.md" | head -5Repository: Irys-xyz/irys
Length of output: 199
🏁 Script executed:
# Search for test macro usage patterns across the api-server crate
rg '#\[(actix_web::test|tokio::test|test_log::test)' crates/api-server/Repository: Irys-xyz/irys
Length of output: 578
🏁 Script executed:
# Check if there are other actix-web integration tests in the codebase
rg 'test::init_service|test::TestRequest' --type rust | head -20Repository: Irys-xyz/irys
Length of output: 1976
🏁 Script executed:
# Look at what the actix-web::test macro actually does by checking Cargo.toml
grep -A 5 -B 5 'actix-web' crates/api-server/Cargo.tomlRepository: Irys-xyz/irys
Length of output: 694
🏁 Script executed:
# Check CLAUDE.md for specific guidance on test macros
cat CLAUDE.md | grep -A 20 -B 5 "test"Repository: Irys-xyz/irys
Length of output: 4007
🏁 Script executed:
# Check if the project has any documentation about actix-web testing conventions
rg -i "actix.*test|integration.*test" README.md CLAUDE.mdRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check the test in error.rs that uses #[actix_web::test]
sed -n '1,50p' crates/api-server/src/error.rs | grep -A 30 "test"Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check the commented-out tests in block.rs to see why they're commented out
grep -B 5 -A 15 '#\[tokio::test\]' crates/api-server/src/routes/block.rsRepository: Irys-xyz/irys
Length of output: 1706
🏁 Script executed:
# Check if there's any guidance about actix-web tests specifically
rg -i "actix" CLAUDE.mdRepository: Irys-xyz/irys
Length of output: 218
🏁 Script executed:
# Let's check what #[actix_web::test] actually is and whether it's compatible with standard tokio::test
rg -B 5 -A 10 'actix_web::test' crates/api-server/src/error.rsRepository: Irys-xyz/irys
Length of output: 639
🏁 Script executed:
# Check if there are any other examples in the codebase using #[tokio::test] with actix test utilities
rg -B 2 '#\[tokio::test\]' crates/api-server/ -A 5Repository: Irys-xyz/irys
Length of output: 1794
🏁 Script executed:
# Check Cargo.toml to understand actix-web version and features
cat crates/api-server/Cargo.tomlRepository: Irys-xyz/irys
Length of output: 1111
Use the repo-standard async test macro in these tests.
These are async tests but they use #[actix_web::test] instead of the crate convention. Prefer #[tokio::test], or #[test_log::test(tokio::test)] where you want tracing/warning-path logs surfaced during test failures.
Also applies to: 183-184, 195-196, 217-218, 237-238
🤖 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/api-server/src/conventions.rs` around lines 158 - 159, Replace the
non-standard #[actix_web::test] attributes on async tests (e.g., the
malformed_json_body_renders_problem test and the other async test functions in
this file currently annotated with #[actix_web::test]) with the
repository-standard async test macro: use #[tokio::test] for plain async tests,
or #[test_log::test(tokio::test)] if you want test-logging/tracing output;
update the test attribute lines accordingly so the tests run under the crate's
preferred test harness.
Contributor
- Preserve protocol-significant headers (e.g. Allow on 405) when the normalize_problem backstop replaces an error body; only the body-descriptive headers (Content-Type/Length, Transfer-Encoding) are superseded by the synthesized Problem - Make the trace module crate-private; it is an internal helper and was needlessly part of the public API
Contributor
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/api-server/src/conventions.rs`:
- Line 19: The header-preservation loop currently keeps body-encoding metadata
which causes clients to mis-decode the replacement problem+json body; update the
header-copying logic to also exclude CONTENT_ENCODING (alongside CONTENT_LENGTH,
CONTENT_TYPE and TRANSFER_ENCODING) so the new body isn't labeled as
gzip/deflate; locate the header-copying/remove section that references
CONTENT_LENGTH, CONTENT_TYPE and TRANSFER_ENCODING and add CONTENT_ENCODING to
the excluded set or call headers.remove(CONTENT_ENCODING) when replacing the
response body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89309d0b-af14-42b6-ab00-f08e1d8f21ff
📒 Files selected for processing (3)
crates/api-server/src/conventions.rscrates/api-server/src/lib.rscrates/api-server/src/trace.rs
| error::Error, | ||
| http::{ | ||
| StatusCode, | ||
| header::{CONTENT_LENGTH, CONTENT_TYPE, HeaderName, HeaderValue, TRANSFER_ENCODING}, |
Contributor
There was a problem hiding this comment.
Drop Content-Encoding when replacing the body.
This loop still preserves body-encoding metadata from the discarded response. If the original 4xx/5xx carried Content-Encoding: gzip, clients will try to decode the new plain problem+json body as gzip and fail. Exclude CONTENT_ENCODING alongside the other body-descriptive headers.
Suggested fix
- header::{CONTENT_LENGTH, CONTENT_TYPE, HeaderName, HeaderValue, TRANSFER_ENCODING},
+ header::{
+ CONTENT_ENCODING, CONTENT_LENGTH, CONTENT_TYPE, HeaderName, HeaderValue,
+ TRANSFER_ENCODING,
+ },
@@
- if name != CONTENT_TYPE && name != CONTENT_LENGTH && name != TRANSFER_ENCODING {
+ if name != CONTENT_TYPE
+ && name != CONTENT_LENGTH
+ && name != TRANSFER_ENCODING
+ && name != CONTENT_ENCODING
+ {
response.headers_mut().append(name.clone(), value.clone());
}Also applies to: 102-107
🤖 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/api-server/src/conventions.rs` at line 19, The header-preservation
loop currently keeps body-encoding metadata which causes clients to mis-decode
the replacement problem+json body; update the header-copying logic to also
exclude CONTENT_ENCODING (alongside CONTENT_LENGTH, CONTENT_TYPE and
TRANSFER_ENCODING) so the new body isn't labeled as gzip/deflate; locate the
header-copying/remove section that references CONTENT_LENGTH, CONTENT_TYPE and
TRANSFER_ENCODING and add CONTENT_ENCODING to the excluded set or call
headers.remove(CONTENT_ENCODING) when replacing the response body.
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
1 participant
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
Step 1 of the HTTP interface conventions work (design doc): every error response from the API server now has exactly one shape — RFC 9457 Problem Details, served as
application/problem+json— regardless of where in the stack the error originated.{ "type": "https://docs.irys.xyz/api/errors/block-not-found", "title": "Block not found", "status": 404, "detail": "Block 0xabc123 was not found in the block tree or database", "code": "BLOCK_NOT_FOUND", "traceId": "0af7651916cd43dd8448eb211c80319c", "error": "Block 0xabc123 was not found in the block tree or database" }Changes
irys-types: new sharedProblem/ProblemItemtypes (serde-only) — the server and every client import one shape.errors[]+ RFC 6901pointercarry multi-field validation failures.ApiError: every variant maps to(status, type, title, code);error_response()renders problem+json. The ad-hoc(String, StatusCode)tuple errors inherit a code derived from their status. NewValidation { errors }variant for field-level failures.detailis genericized on server errors so internal messages never leak; the real error is logged server-side keyed bytraceId(W3C trace id from the active OTEL span, also surfaced as atraceparentresponse header on all responses).conventions.rs): JSON-body/path/query extractor failures and unmatched routes route throughApiError(fixes the JSON-decode handler that returned plain text); anormalize_problemmiddleware backstops everything else (framework 405/413, panics). The backstop only synthesizes for empty/text/*bodies — structuredapplication/*bodies are never clobbered, preserving the/execution-rpcJSON-RPC passthrough verbatim.errorstring mirrorsdetailfor one release so legacy parsers of{"error": "..."}keep working.Behavior changes (intentional)
errorfield retained for the deprecation window).INVALID_PATH_PARAMETER/INVALID_QUERY_PARAMETER) instead of actix's defaults (404/422). No in-repo consumer asserted the old codes.Test plan
ApiError→Problemmapping, 5xx non-leak, extractor/default-service/middleware behavior, proxy-passthrough preservationcargo test -p irys-api-server --lib→ 95 passed;cargo test -p irys-types --lib problem→ 3 passedcargo fmt+cargo clippy --testsclean on touched cratesRoadmap (documented, not in this PR)
utoipacode-first OpenAPI:/v1/openapi.json+ Swagger UI at/v1/docs, committed spec + CI drift checkirys-api-client🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation