Merged
Conversation
955c49a to
4801403
Compare
There was a problem hiding this comment.
Code Review Summary
Clean, well-structured PR. The separation into constants.py and config.py is sound, naming follows project conventions, and the parametrized tests for normalize_api_router_prefix are thorough. A few items worth addressing below.
What works well
normalize_api_router_prefixis well-designed with clean logic and good edge-case coverage in testsconstants.pyeliminates scattered magic strings — genuine maintainability win- Consistent Google-style docstrings and type annotations throughout
- Auth dependencies are unaffected by the routing changes — no bypass risk
- Config is resolved once at startup, not per-request — correct pattern
Suggestions (see inline comments)
- Add input validation to
normalize_api_router_prefix— values like../../adminorapi?x=1pass through unchallenged and could confuse reverse proxies - Add test for
get_configured_mt5_api_keyempty-string coercion —MT5_API_KEY=""silently disables auth via theor Nonepattern, which is security-relevant and should be explicitly tested - Consider adversarial inputs in prefix tests —
api//v1,api?v=1, path traversal patterns importlib.reloadtest fragility — consider an app factory pattern to avoid module-level side effects in tests
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
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
/api/v1default so the API serves root-level routes unless a prefix is configuredAPI_ROUTER_PREFIXand allow deployments to set the prefix via environment variableTesting
.agents/skills/local-qa/scripts/qa.shNotes
"", so existing routes in this branch remain unprefixedAPI_ROUTER_PREFIXnormalizes values likeapi/v1,/api/v1, and/api/v1/to the same mounted prefix