Week 11: Fix API error responses in mounted sub-apps + challenge contributor guide - #560
Open
ashike24 wants to merge 11 commits into
Open
Week 11: Fix API error responses in mounted sub-apps + challenge contributor guide#560ashike24 wants to merge 11 commits into
ashike24 wants to merge 11 commits into
Conversation
request.url.path retains the mount prefix (e.g. /vendor/api/v1/chat) for requests handled by mounted sub-apps like the vendor, ctf, and admin portals. is_api_request() only checked for a /api/ prefix, so it never matched these paths - API error responses (429, 403, 404, 500, etc.) from those apps were rendering the generic HTML error page instead of a JSON response, with the wrong status text shown (e.g. a 429 rate-limit response displayed as '400 - Bad Request'). Fixed to check for /api/ as a path segment anywhere in the path, so it works both for mounted sub-apps and routes handled directly by the root app. Found and fixed during Week 11 final QA pass. Verified live in browser (429 now returns correct JSON body/status) and via full test suite (398 passed, 26 skipped, 6 pre-existing failures unchanged, 0 regressions).
Walks through adding a new CTF challenge end-to-end: YAML definition, detector class, the required-but-non-obvious detector registration step in detectors/implementations/__init__.py, unit test coverage, and a pre-PR checklist. Uses the asi03-ghost-in-the-machine challenge and AgentImpersonationDetector as a worked example throughout, based on the actual patterns in the codebase. Written for Week 11 per the proposal timeline.
Author
|
Note: this diff includes the changes from PRs #521, #532, and #559 as well, since none of those are merged into upstream main yet and this branch was built on top of them (same situation as #559). The changes specific to this PR are:
Everything else in the diff is prior work already under review in the other open PRs. |
Author
|
Closing this in favor of a single combined final submission: #567, which includes both changes from this PR along with everything else from the program. |
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.
Week 11: Final bug fix + contributor guide
Per the proposal timeline (Week 11: "Final bug fixes, QA pass, write second blog post, prepare contributor guide"), this PR covers the code and docs deliverables from that list. The second blog post is being handled separately through the usual publishing channel.
1. Fix: is_api_request() didn't detect API requests inside mounted sub-apps
Commit: 491c46e
While doing a final QA pass, I noticed the rate limiter's 429 response was rendering as an HTML page titled "400 - Bad Request" instead of a JSON error reflecting the real status.
Root cause: is_api_request() in finbot/core/error_handlers.py only checked whether request.url.path started with /api/. But /vendor, /ctf, and /admin are mounted as separate ASGI sub-apps, and request.url.path inside a mounted app retains the mount prefix (e.g. /vendor/api/v1/chat, not /api/v1/chat). The check never matched, so every API error response from those three portals - not just 429s, but 403s, 404s, 500s, etc. - was silently falling back to a mislabeled HTML error page instead of a proper JSON response.
Fix: Check for /api/ as a path segment anywhere in the path, rather than only as a prefix. Works for both mounted sub-apps and routes handled directly by the root app.
Verification:
2. Docs: Challenge contributor guide
Commit: f0b4677
Added docs/contributing-challenges.md, walking through the full process of adding a new CTF challenge: YAML definition structure, the detector class pattern (including the LLM-judge prompt convention), unit test coverage expectations, and a pre-PR checklist.
Uses asi03-ghost-in-the-machine / AgentImpersonationDetector as a worked example throughout, based on the actual code rather than a generic template.
Specifically calls out one non-obvious gotcha: challenge YAML files are auto-discovered on startup, but detector classes are not - they only register if explicitly imported in finbot/ctf/detectors/implementations/init.py. Skipping that step fails silently (the challenge loads, but the detector never resolves at runtime), which seemed worth documenting clearly for future contributors.
Notes for reviewer