refactor: split large command modules (Phase 7-8)#152
Conversation
Phase 7: Split api.py (3471 -> 1230 lines) - Extract HTTP handler infrastructure into api_handler.py (2303 lines) - Remove 15 unused CLI-only imports from handler - Add CorrelationEnricher re-export for backward compat Phase 8: Split commands/ui.py (4995 -> 1032 lines) - Extract _INDEX_HTML template into ui_templates.py (1852 lines) - Extract 43 helper functions into ui_payloads.py (2193 lines) - Fix duplicate _LLM_PROVIDER_DEFAULTS constant - Add 19 missing imports to ui.py, remove 7 unused from ui_payloads - Update test monkeypatches for new module structure All 952 tests pass, ruff clean.
📝 WalkthroughWalkthroughThis PR refactors the Retrace architecture by extracting HTTP API handler logic from ChangesArchitectural Refactoring: API Handler and UI Payload Extraction
Possibly Related PRs
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
txmed82
force-pushed
the
refactor/phase-3-modularize-storage
branch
from
1cfcd76 to
a0b45bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/retrace/commands/api_handler.py`:
- Around line 1876-1896: The code clamps limit/offset only in the response but
still passes raw values to store.list_app_error_alert_rules, causing mismatched
slices; fix by normalizing the inputs first (e.g., compute safe_limit = max(1,
min(limit, 500)) and safe_offset = max(0, offset)) immediately after parsing and
validating the ints, then call store.list_app_error_alert_rules with safe_limit
and safe_offset and use those same safe_* values in the _json_response rules and
pagination fields (references: variables limit/offset, safe_limit/safe_offset
you will add, function store.list_app_error_alert_rules, and _json_response).
- Around line 1185-1189: The handler currently decodes request body with
body.decode("utf-8") and only catches json.JSONDecodeError, so a
UnicodeDecodeError will escape and become a 500; wrap the decode in the same
failure path so malformed UTF-8 is treated as invalid JSON: catch
UnicodeDecodeError around body.decode("utf-8") (or catch both UnicodeDecodeError
and json.JSONDecodeError) and call _json_response(self, 400,
{"error":"invalid_json"}) just like the JSONDecodeError branch; apply the same
change to every other site in this module that does body.decode("utf-8")
followed by json.loads (look for uses of self.rfile.read, payload =
json.loads(...), and _json_response).
- Around line 1724-1729: The handler currently trusts Content-Length and calls
self.rfile.read(length) without bounds; validate and bound the length first:
parse Content-Length into length, reject negative values with a 400 (same
"invalid_content_length" pattern), and cap length to the small-body limit used
by other admin POST endpoints before calling self.rfile.read; update the
handling around the length variable and the self.rfile.read(...) call so
oversized requests are refused or truncated to the configured small-body limit.
In `@src/retrace/commands/ui_payloads.py`:
- Around line 771-772: The code treats blank run statuses as real values because
latest_statuses is built as a list of possibly empty strings; change the logic
to either filter out empty/whitespace statuses or use any() when checking for
presence. For example, keep building latest_statuses from latest_run_status (use
str(...).strip()), then create a filtered list like nonempty_latest_statuses =
[s for s in latest_statuses if s] and use if test_links and not
nonempty_latest_statuses (or replace the existing if test_links and not
latest_statuses checks with if test_links and not any(latest_statuses)) to
ensure the "no-run-yet" case is detected; apply the same fix where similar logic
appears (lines around the api_links block and the repeated 871-878 region).
- Around line 2184-2190: The SDK-key response is hardcoding loopback ingest
coordinates; change the ingest_url and build_sentry_dsn(base_url=...) to use the
configured/public replay API base URL instead of "http://127.0.0.1:8788". Locate
the block that sets "ingest_path", "ingest_url", and calls
build_sentry_dsn(public_key=created.key, base_url=...), and replace the
hardcoded base URL with the application configuration or helper that returns the
public replay API base URL (e.g., use the existing replay API
setting/get_replay_api_base_url() or equivalent) so browsers receive the correct
server-facing ingest endpoint and DSN.
- Around line 140-176: The code uses a private httpcore backend
(SNINetworkBackend/start_tls) and passes network_backend into
PinnedHTTPTransport which httpx 0.27.0 does not accept; remove the
SNINetworkBackend import and stop passing network_backend into
PinnedHTTPTransport and instead implement the supported customization: either
pin httpx to a version that documents network_backend support, or refactor
PinnedHTTPTransport (override its handle_request method) to set the SNI hostname
per-request using the documented mechanism (attach sni_hostname to
request.extensions or configure the SSL wrapping inside handle_request based on
the pinned_ip/original_hostname), ensuring you no longer rely on
httpcore._backends.sync or the network_backend kwarg.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce537cec-d526-484a-a7f3-5625d217a4dc
📒 Files selected for processing (6)
src/retrace/commands/api.pysrc/retrace/commands/api_handler.pysrc/retrace/commands/ui.pysrc/retrace/commands/ui_payloads.pysrc/retrace/commands/ui_templates.pytests/test_ui_replay_specs.py
| body = self.rfile.read(length) | ||
| try: | ||
| payload = json.loads(body.decode("utf-8") or "{}") | ||
| except json.JSONDecodeError: | ||
| _json_response(self, 400, {"error": "invalid_json"}) |
There was a problem hiding this comment.
Treat invalid UTF-8 as invalid_json.
body.decode("utf-8") can raise UnicodeDecodeError, which bypasses this 400 path and turns malformed client input into an unhandled 500. The same decode pattern appears in several JSON handlers in this module, so this fix should be applied at each reused call site.
Suggested fix
try:
payload = json.loads(body.decode("utf-8") or "{}")
- except json.JSONDecodeError:
+ except (json.JSONDecodeError, UnicodeDecodeError):
_json_response(self, 400, {"error": "invalid_json"})
return🤖 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 `@src/retrace/commands/api_handler.py` around lines 1185 - 1189, The handler
currently decodes request body with body.decode("utf-8") and only catches
json.JSONDecodeError, so a UnicodeDecodeError will escape and become a 500; wrap
the decode in the same failure path so malformed UTF-8 is treated as invalid
JSON: catch UnicodeDecodeError around body.decode("utf-8") (or catch both
UnicodeDecodeError and json.JSONDecodeError) and call _json_response(self, 400,
{"error":"invalid_json"}) just like the JSONDecodeError branch; apply the same
change to every other site in this module that does body.decode("utf-8")
followed by json.loads (look for uses of self.rfile.read, payload =
json.loads(...), and _json_response).
| length = int(self.headers.get("Content-Length") or "0") | ||
| except ValueError: | ||
| _json_response(self, 400, {"error": "invalid_content_length"}) | ||
| return | ||
| body = self.rfile.read(max(0, length)) if length else b"{}" | ||
| try: |
There was a problem hiding this comment.
Bound the replay-processing request body before reading it.
This handler trusts Content-Length and reads that many bytes without a cap. A caller with a valid service token can force an arbitrarily large allocation on a request thread here, unlike the other admin POST endpoints. Reject negative lengths and apply the same small-body limit before self.rfile.read(...).
Suggested fix
try:
length = int(self.headers.get("Content-Length") or "0")
except ValueError:
_json_response(self, 400, {"error": "invalid_content_length"})
return
+ if length < 0:
+ _json_response(self, 400, {"error": "invalid_content_length"})
+ return
+ if length > 64 * 1024:
+ _json_response(self, 413, {"error": "payload_too_large"})
+ return
body = self.rfile.read(max(0, length)) if length else b"{}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| length = int(self.headers.get("Content-Length") or "0") | |
| except ValueError: | |
| _json_response(self, 400, {"error": "invalid_content_length"}) | |
| return | |
| body = self.rfile.read(max(0, length)) if length else b"{}" | |
| try: | |
| length = int(self.headers.get("Content-Length") or "0") | |
| except ValueError: | |
| _json_response(self, 400, {"error": "invalid_content_length"}) | |
| return | |
| if length < 0: | |
| _json_response(self, 400, {"error": "invalid_content_length"}) | |
| return | |
| if length > 64 * 1024: | |
| _json_response(self, 413, {"error": "payload_too_large"}) | |
| return | |
| body = self.rfile.read(max(0, length)) if length else b"{}" | |
| try: |
🤖 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 `@src/retrace/commands/api_handler.py` around lines 1724 - 1729, The handler
currently trusts Content-Length and calls self.rfile.read(length) without
bounds; validate and bound the length first: parse Content-Length into length,
reject negative values with a 400 (same "invalid_content_length" pattern), and
cap length to the small-body limit used by other admin POST endpoints before
calling self.rfile.read; update the handling around the length variable and the
self.rfile.read(...) call so oversized requests are refused or truncated to the
configured small-body limit.
| try: | ||
| limit = int(params.get("limit") or "100") | ||
| offset = int(params.get("offset") or "0") | ||
| except ValueError: | ||
| _json_response(self, 400, {"error": "invalid_pagination"}) | ||
| return | ||
| rules = store.list_app_error_alert_rules( | ||
| project_id=token.project_id, | ||
| environment_id=environment_id, | ||
| limit=limit, | ||
| offset=offset, | ||
| ) | ||
| _json_response( | ||
| self, | ||
| 200, | ||
| { | ||
| "project_id": token.project_id, | ||
| "environment_id": environment_id, | ||
| "limit": max(1, min(limit, 500)), | ||
| "offset": max(0, offset), | ||
| "rules": [_alert_rule_api_dict(rule) for rule in rules], |
There was a problem hiding this comment.
Clamp pagination before querying storage.
The response normalizes limit and offset, but store.list_app_error_alert_rules(...) still receives the raw values. Negative or oversized inputs can therefore read a different slice than the API reports, and may effectively remove the intended cap.
Suggested fix
try:
limit = int(params.get("limit") or "100")
offset = int(params.get("offset") or "0")
except ValueError:
_json_response(self, 400, {"error": "invalid_pagination"})
return
+ limit = max(1, min(limit, 500))
+ offset = max(0, offset)
rules = store.list_app_error_alert_rules(
project_id=token.project_id,
environment_id=environment_id,
limit=limit,
offset=offset,
@@
{
"project_id": token.project_id,
"environment_id": environment_id,
- "limit": max(1, min(limit, 500)),
- "offset": max(0, offset),
+ "limit": limit,
+ "offset": offset,
"rules": [_alert_rule_api_dict(rule) for rule in rules],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| limit = int(params.get("limit") or "100") | |
| offset = int(params.get("offset") or "0") | |
| except ValueError: | |
| _json_response(self, 400, {"error": "invalid_pagination"}) | |
| return | |
| rules = store.list_app_error_alert_rules( | |
| project_id=token.project_id, | |
| environment_id=environment_id, | |
| limit=limit, | |
| offset=offset, | |
| ) | |
| _json_response( | |
| self, | |
| 200, | |
| { | |
| "project_id": token.project_id, | |
| "environment_id": environment_id, | |
| "limit": max(1, min(limit, 500)), | |
| "offset": max(0, offset), | |
| "rules": [_alert_rule_api_dict(rule) for rule in rules], | |
| try: | |
| limit = int(params.get("limit") or "100") | |
| offset = int(params.get("offset") or "0") | |
| except ValueError: | |
| _json_response(self, 400, {"error": "invalid_pagination"}) | |
| return | |
| limit = max(1, min(limit, 500)) | |
| offset = max(0, offset) | |
| rules = store.list_app_error_alert_rules( | |
| project_id=token.project_id, | |
| environment_id=environment_id, | |
| limit=limit, | |
| offset=offset, | |
| ) | |
| _json_response( | |
| self, | |
| 200, | |
| { | |
| "project_id": token.project_id, | |
| "environment_id": environment_id, | |
| "limit": limit, | |
| "offset": offset, | |
| "rules": [_alert_rule_api_dict(rule) for rule in rules], |
🤖 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 `@src/retrace/commands/api_handler.py` around lines 1876 - 1896, The code
clamps limit/offset only in the response but still passes raw values to
store.list_app_error_alert_rules, causing mismatched slices; fix by normalizing
the inputs first (e.g., compute safe_limit = max(1, min(limit, 500)) and
safe_offset = max(0, offset)) immediately after parsing and validating the ints,
then call store.list_app_error_alert_rules with safe_limit and safe_offset and
use those same safe_* values in the _json_response rules and pagination fields
(references: variables limit/offset, safe_limit/safe_offset you will add,
function store.list_app_error_alert_rules, and _json_response).
| from httpcore._backends.sync import SyncBackend | ||
|
|
||
| class SNINetworkBackend(SyncBackend): | ||
| """NetworkBackend that overrides SNI hostname for SSL connections.""" | ||
|
|
||
| def __init__(self, pinned_ip: str, sni_hostname: str): | ||
| super().__init__() | ||
| self._pinned_ip = pinned_ip | ||
| self._sni_hostname = sni_hostname | ||
|
|
||
| def start_tls( | ||
| self, | ||
| sock: socket.socket, | ||
| ssl_context: ssl.SSLContext, | ||
| server_hostname: Optional[str] = None, | ||
| timeout: Optional[float] = None, | ||
| ) -> ssl.SSLSocket: | ||
| # Override server_hostname to use the original hostname for SNI | ||
| # when we're connecting to our pinned IP | ||
| try: | ||
| peername = sock.getpeername() | ||
| if peername and peername[0] == self._pinned_ip: | ||
| server_hostname = self._sni_hostname | ||
| except Exception: | ||
| pass | ||
|
|
||
| # Wrap the socket with SSL using the correct server_hostname | ||
| return super().start_tls(sock, ssl_context, server_hostname, timeout) | ||
|
|
||
| network_backend = SNINetworkBackend(pinned_ip=pinned_ip, sni_hostname=hostname) | ||
|
|
||
| transport = PinnedHTTPTransport( | ||
| pinned_ip=pinned_ip, | ||
| original_hostname=hostname, | ||
| verify=ssl_context, | ||
| network_backend=network_backend, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In httpx 0.27.0, what parameters does httpx.HTTPTransport.__init__ accept? Does it support network_backend, and what is the supported way to customize backend/SNI behavior for a transport?
💡 Result:
In httpx 0.27.0, httpx.HTTPTransport.init accepts parameters including verify: VerifyTypes = True, cert: CertTypes | None = None, http1: bool = True, http2: bool = False, limits: Limits = DEFAULT_LIMITS, proxy: ProxyTypes | None = None, local_address: str | None = None, uds: str | None = None, socket_options: list | None = None, retries: int = 0.[1][2][3] It does not support a network_backend parameter directly.[4][5][3] Support for network_backend was added later via PR #3749 in 2026, after 0.27.0 (released Feb 2024).[4] The supported way to customize backend behavior is by subclassing httpx.HTTPTransport or httpx.BaseTransport and overriding handle_request or providing a custom httpcore connection pool with the desired NetworkBackend.[1][5][6][7] For example, custom transports can monkey-patch or replace the _pool._network_backend.[6][7] To customize SNI behavior, use the "sni_hostname" request extension on individual requests: extensions={"sni_hostname": "example.com"} when making requests to IP addresses while verifying against a hostname.[8] For transport-level customization (e.g., connect-to IP), subclass the transport to modify connection handling or use socket_options/local_address.[1][9][8]
Citations:
- 1: https://www.python-httpx.org/advanced/transports/
- 2: https://github.com/encode/httpx/blob/master/httpx/_client.py
- 3: https://github.com/encode/httpx/blob/master/CHANGELOG.md
- 4: Implement support for specifying the httpcore Network Backend when creating default transports encode/httpx#3749
- 5: Request to allow passing CustomBackend to httpx client encode/httpx#3180
- 6: https://gist.github.com/toppk/346e7ec85c956e6608cdadd6e1edae1f
- 7: https://gist.github.com/toppk/a5838c9727d806c7845f7be0e2eacf2f
- 8: https://www.python-httpx.org/advanced/extensions/
- 9: add options to override tcp hostname and tls hostname encode/httpx#2080
🏁 Script executed:
# Find the definition of PinnedHTTPTransport
rg -n "class PinnedHTTPTransport" --type pythonRepository: txmed82/retrace
Length of output: 90
🏁 Script executed:
# Look for import statements related to httpx and version specs
rg -n "httpx" src/retrace/commands/ui_payloads.py -A 2 -B 2Repository: txmed82/retrace
Length of output: 2117
🏁 Script executed:
# Find dependency/requirements files to check httpx version
fd -E "*.pyc" "requirements.*|setup.py|setup.cfg|pyproject.toml" -type fRepository: txmed82/retrace
Length of output: 230
🏁 Script executed:
# Find dependency/requirements files
find . -maxdepth 2 -name "requirements*.txt" -o -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" | head -20Repository: txmed82/retrace
Length of output: 76
🏁 Script executed:
# Read the full PinnedHTTPTransport class and surrounding code
sed -n '62,180p' src/retrace/commands/ui_payloads.pyRepository: txmed82/retrace
Length of output: 5032
🏁 Script executed:
# Read pyproject.toml to find httpx version
cat pyproject.tomlRepository: txmed82/retrace
Length of output: 1311
network_backend parameter is unsupported in httpx 0.27.0; code will crash at runtime.
PinnedHTTPTransport(network_backend=...) passes network_backend as a kwarg to httpx.HTTPTransport.__init__(), but httpx 0.27.0 does not accept this parameter. The dependency httpx>=0.27 is loose enough to resolve to 0.27.0, which will cause a TypeError when the HTTPS transport path is exercised. Additionally, the code imports from httpcore._backends.sync, a private API that is not part of the supported interface. To fix: either pin httpx to a version that supports network_backend (if available), or refactor to use the documented customization approach (subclass the transport and override handle_request to configure SSL/SNI at the request level, or use the sni_hostname request extension).
🤖 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 `@src/retrace/commands/ui_payloads.py` around lines 140 - 176, The code uses a
private httpcore backend (SNINetworkBackend/start_tls) and passes
network_backend into PinnedHTTPTransport which httpx 0.27.0 does not accept;
remove the SNINetworkBackend import and stop passing network_backend into
PinnedHTTPTransport and instead implement the supported customization: either
pin httpx to a version that documents network_backend support, or refactor
PinnedHTTPTransport (override its handle_request method) to set the SNI hostname
per-request using the documented mechanism (attach sni_hostname to
request.extensions or configure the SSL wrapping inside handle_request based on
the pinned_ip/original_hostname), ensuring you no longer rely on
httpcore._backends.sync or the network_backend kwarg.
| latest_statuses = [str(link.get("latest_run_status") or "") for link in test_links] | ||
| api_links = [ |
There was a problem hiding this comment.
Blank run statuses are treated as real results.
When a link exists but latest_run_status is still unset, latest_statuses becomes [""], so the later if test_links and not latest_statuses branch never triggers. That drops the "Run linked tests" recommendation for exactly the no-run-yet case.
Suggested fix
- latest_statuses = [str(link.get("latest_run_status") or "") for link in test_links]
+ latest_statuses = [
+ status
+ for link in test_links
+ if (status := str(link.get("latest_run_status") or "").strip())
+ ]Also applies to: 871-878
🤖 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 `@src/retrace/commands/ui_payloads.py` around lines 771 - 772, The code treats
blank run statuses as real values because latest_statuses is built as a list of
possibly empty strings; change the logic to either filter out empty/whitespace
statuses or use any() when checking for presence. For example, keep building
latest_statuses from latest_run_status (use str(...).strip()), then create a
filtered list like nonempty_latest_statuses = [s for s in latest_statuses if s]
and use if test_links and not nonempty_latest_statuses (or replace the existing
if test_links and not latest_statuses checks with if test_links and not
any(latest_statuses)) to ensure the "no-run-yet" case is detected; apply the
same fix where similar logic appears (lines around the api_links block and the
repeated 871-878 region).
| "ingest_path": "/api/sdk/replay", | ||
| "ingest_url": "http://127.0.0.1:8788/api/sdk/replay", | ||
| "sentry_dsn": build_sentry_dsn( | ||
| public_key=created.key, | ||
| base_url="http://127.0.0.1:8788", | ||
| project_id=workspace.project_id, | ||
| ), |
There was a problem hiding this comment.
Don't return loopback ingest coordinates from the SDK-key API.
These values are consumed by browser-side SDKs. Hardcoding 127.0.0.1:8788 means any remote/self-hosted deployment will hand browsers an ingest URL and DSN that point back to the end user's machine instead of the Retrace server. Please derive this from the configured/public replay API base URL.
🤖 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 `@src/retrace/commands/ui_payloads.py` around lines 2184 - 2190, The SDK-key
response is hardcoding loopback ingest coordinates; change the ingest_url and
build_sentry_dsn(base_url=...) to use the configured/public replay API base URL
instead of "http://127.0.0.1:8788". Locate the block that sets "ingest_path",
"ingest_url", and calls build_sentry_dsn(public_key=created.key, base_url=...),
and replace the hardcoded base URL with the application configuration or helper
that returns the public replay API base URL (e.g., use the existing replay API
setting/get_replay_api_base_url() or equivalent) so browsers receive the correct
server-facing ingest endpoint and DSN.
1 participant
Summary
Split two monolithic command modules to improve maintainability:
Phase 7:
api.py(3471 → 1230 lines)api_handler.py(2303 lines)CorrelationEnricherre-export for backward compatPhase 8:
commands/ui.py(4995 → 1032 lines)_INDEX_HTMLtemplate intoui_templates.py(1852 lines)ui_payloads.py(2193 lines)_LLM_PROVIDER_DEFAULTSconstant (pre-existing bug)ui.py, remove 7 unused fromui_payloadsValidation
Summary by CodeRabbit
Bug Fixes
Refactor
Tests