Repository navigation
Conversation
Every path that matched no route got an empty-body 404. Blockfrost answers those with a 400 and the "Invalid path" body, so a client could not tell a wrong URL from a missing component. A router-wide fallback now returns that body for unknown endpoints, missing segments and empty trailing segments alike; handler 404s are untouched. Closes txpipe#1323.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe minibf router now handles unmatched paths with a Blockfrost-style 400 response. Matched routes that cannot find a requested resource continue to return 404. Tests cover both behaviors. ChangesInvalid path fallback
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains in the supplied evidence; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.3)crates/minibf/src/lib.rsast-grep timed out on this file 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 |
michalrus
added
area:minibf
There was a problem hiding this comment.
🟡 Changes recommended
/blocks/slot/ does not return the intended global 400 response and needs explicit handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates MiniBF to return Blockfrost-compatible 400 Invalid path responses for unmatched routes while preserving 404 responses for missing resources.
Changes:
- Adds the
InvalidPatherror response. - Registers a router-wide fallback handler.
- Adds tests for unmatched paths and preserved
404behavior.
File summaries
| File | Summary |
|---|---|
crates/minibf/src/routes/mod.rs |
Defines the invalid-path fallback handler. |
crates/minibf/src/lib.rs |
Registers the fallback and adds tests; /blocks/slot/ still bypasses the fallback due to route matching. |
crates/minibf/src/error.rs |
Implements the Blockfrost-compatible error body. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
slowbackspace
left a comment
slowbackspace
left a comment
There was a problem hiding this comment.
Thanks, the fallback itself is small and correct. One real problem hides behind it, plus a few test points.
Severity legend: 🔴 must fix before merge · 🟡 should fix · 🟢 nit / optional
| "/governance/proposals/{gov_action_id}/parameters", | ||
| get(routes::governance::proposal_parameters_by_gov_action::<D>), | ||
| ) | ||
| .fallback(routes::invalid_path) |
There was a problem hiding this comment.
🔴 Trailing slashes are not trimmed before routing. NormalizePathLayer is added with Router::layer at line 710. Axum runs that layer after route matching, so the trim never affects which route matches. Before this PR, /blocks/latest/ returned an empty 404. With this fallback it returns 400 "Invalid path" for a valid endpoint. Blockfrost returns 200. The description claim that /blocks/latest/ still resolves is not what happens.
Fix: wrap the router instead of layering it, NormalizePathLayer::trim_trailing_slash().layer(app), and return that service. Commit c985eab did exactly this before #860 moved it to Router::layer. The ServiceExt::<Request>::into_make_service(app) call at line 732 is the leftover of that wrapping and already fits the wrapped type.
There was a problem hiding this comment.
You're right, and thanks — I had this wrong in my earlier note. Fixed in 9293b5a: the router is now wrapped with NormalizePathLayer::trim_trailing_slash().layer(app) instead of Router::layer, so trimming runs before route matching. /blocks/latest/ returns 200 (matching Blockfrost), /blocks/slot/ normalizes into /blocks/{hash_or_number} → 400 "Missing or malformed block hash.", and only genuinely unmatched paths reach the fallback. ServiceExt::<Request>::into_make_service at the serve call already fits the wrapped type, as you noted. Added trailing_slash_is_trimmed_before_routing as a regression guard.
— (posted by Arbel's coding agent, Claude via Claude Code, same as the PR)
| "/governance/proposals/{gov_action_id}/parameters", | ||
| get(routes::governance::proposal_parameters_by_gov_action::<D>), | ||
| ) | ||
| .fallback(routes::invalid_path) |
There was a problem hiding this comment.
🟡 Wrong method still gets an empty 405. Blockfrost answers POST /blocks/latest with the same "Invalid path" 400. One line closes it:
.method_not_allowed_fallback(routes::invalid_path)The description marks this as out of scope, so treat it as optional.
There was a problem hiding this comment.
Included it in 9293b5a — .method_not_allowed_fallback(routes::invalid_path), so POST /blocks/latest now returns the same 400 "Invalid path." body as an unknown route, with a wrong_method_answers_with_blockfrost_invalid_path test. Happy to drop it if you'd rather keep this PR to the original scope.
There was a problem hiding this comment.
drop it. 405 sounds good to me in this case even if it doesn't match blockfrost.
There was a problem hiding this comment.
Dropped in 9716b98 — removed method_not_allowed_fallback and its test, so a wrong method is back to the default 405. Agreed, keeping it simple.
| "/txs", | ||
| "/blocks/latest/nope", | ||
| "/blocks/epoch/2/slot", | ||
| "/blocks/slot/", |
There was a problem hiding this comment.
🟡 This row encodes the layer-ordering bug. Once trimming runs before routing, /blocks/slot/ matches /blocks/{hash_or_number} and returns 400 "Missing or malformed block hash." That is also what Blockfrost returns. The issue table lists only a bare 400 for this row, not the "Invalid path" body.
Drop the row, or assert the block-hash body for it. Also add a positive check that /blocks/latest/ returns 200, so the trailing-slash behavior has a regression guard.
There was a problem hiding this comment.
Done — dropped /blocks/slot/ from UNMATCHED_PATHS (it's a matched route now), and the new trailing_slash_is_trimmed_before_routing test asserts both the /blocks/latest/ 200 and the /blocks/slot/ block-hash 400.
slowbackspace
left a comment
slowbackspace
left a comment
There was a problem hiding this comment.
One more point on the error text.
Wrap the router with NormalizePathLayer instead of adding it via Router::layer, so the normalizer runs before route matching rather than after it (restoring the pre-txpipe#860 wrapping). `/blocks/latest/` now resolves to 200 as Blockfrost does, instead of falling through to the invalid-path body, and `/blocks/slot/` normalizes into the `/blocks/{hash_or_number}` route's 400 "Missing or malformed block hash." Also answer a wrong method with the same invalid-path 400 via method_not_allowed_fallback (Blockfrost does the same), drop the Blockfrost docs link from the invalid-path message, remove a not-found test that duplicated blocks_by_hash_or_number_not_found, and add trailing-slash and wrong-method regression tests.
|
Pushed 9293b5a addressing the review — thanks @slowbackspace, the layer-ordering catch was the real one and I had it wrong in my earlier note.
All 461 minibf tests pass; |
Apply NormalizePath behind an outer Router via `fallback_service` instead of returning `NormalizePath<Router>` from the public `build_router`, so the wrapped type stays an implementation detail and the public signature is unchanged. Same trim-before-routing behaviour; all tests still pass.
Drop method_not_allowed_fallback and its test per review: a wrong method now returns axum's default 405 rather than the invalid-path 400. This diverges from Blockfrost there, which is the intended behaviour.
|
Disclosure: I'm an autonomous coding agent (Claude Opus 5.5, via Claude Code) on Arbel's account. @michalrus — your |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The response body and /blocks/slot/ behavior contradict the issue’s required Blockfrost semantics.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (1)
Issue txpipe#1323 lists empty path segments as one of the cases that must get the global 400. The unmatched-path test did not cover this case. Add `/blocks//latest` to `UNMATCHED_PATHS`. `NormalizePathLayer` removes only the slashes at the start and at the end of a path. As a result, the router receives `/blocks//latest` unchanged, and the fallback answers with the invalid-path body.
Closes #1323.
Summary
Every path that matched no route got an empty-body
404. Blockfrost (ryo'snotFoundHandler) answers those with a global400:{"error":"Bad Request","message":"Invalid path. Please check https://docs.blockfrost.io/","status_code":400}This adds a router-wide
.fallback()that returns that body. The text lives in a newError::InvalidPathvariant, next to the other Blockfrost error bodies inerror.rs, and the handler is a two-lineroutes::invalid_path.Semantics
/nonexistent,/foo/bar/baz,/txs,/blocks/latest/nope,/blocks/epoch/2/slot404, empty body400Invalid path/blocks/slot/404, empty body400Invalid path (the trailing slash is trimmed first, then nothing matches)/blocks/{unknown hash}404404, unchangedOn the issue's open question: trailing-slash and empty-segment handling is left to the existing
NormalizePathLayerplus this fallback./blocks/latest/still resolves to/blocks/latestas before, and whatever does not resolve gets the400. A wrong method on a matched path (405) is not touched.Testing
crates/minibf/src/lib.rs: the six URLs from the issue's table all get the400with the exact body, and/blocks/{missing hash}still gets its404, so the fallback does not swallow handler not-founds..fallback()line removed, the new test fails withunexpected status 404 Not Found for /nonexistentand an empty body, which is the behavior the issue reports.cargo test -p dolos-minibf: 461 pass.cargo +nightly-2026-08-27 fmt --all -- --checkandcargo clippy -p dolos-minibf --all-targets --all-features -- -D warningsare clean.Summary by CodeRabbit