feat: Phase 6 — DevEx + observability polish#9
Conversation
…g + CI jobs Adds: - .github/dependabot.yml — weekly grouped pip + github-actions updates - pyproject.toml [tool.ruff] with E/F/W/I/UP/B/C4/SIM rule set + per-file ignores - pyproject.toml [tool.mypy] strict on personalize/, permissive on legacy - CI job 'mypy (personalize/ strict, rest permissive)' — soft gate - CI job 'bandit (security lint)' — HARD gate on HIGH severity (currently 0) Fixes surfaced by new linters: - B324 hashlib.md5 in kratos_clone/capture.py — added usedforsecurity=False - B201 flask debug=True in app.py __main__ — annotated #nosec B201 (dev-only) - SIM105 try-except-pass in app.py — replaced with contextlib.suppress(Exception) - UP045 Optional[Callable] in kratos_clone/capture.py — replaced with X | None - 7 mypy errors in personalize/ — fixed (Mapping for invariance, type-ignores for openai SDK overload mismatches, explicit return type annotations) Result: ruff clean, mypy clean on personalize/ (8 files), bandit HIGH=0, 178 tests + 2 skipped. Auto-formatted 21 legacy files via ruff format.
- before_request: parse X-Request-ID header (allow-list [A-Za-z0-9_-]{1,64})
or generate UUID4. Bind to structlog contextvars so every log line in the
request scope auto-inherits request_id=<id>.
- after_request: emit X-Request-ID in response, clear contextvars.
- _safe_request_id: defense-in-depth regex for header injection (in practice
Werkzeug rejects \r\n at the parser level — caught in test).
- 5 tests; full suite 183 passing.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (30)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes Phase 6 of the project, focusing on developer experience and observability. Key improvements include the addition of a Dependabot configuration for automated updates, a comprehensive environment variable reference in the README, and the introduction of X-Request-ID middleware for request tracing. The project's tooling is also enhanced with new configurations for ruff, mypy, and bandit. Review feedback identifies a potential context leakage in the Flask middleware that should be resolved by using teardown_request, suggests adding error handling for base64 decoding in the image patcher to improve resilience, and notes redundant lines in the updated roadmap that require cleanup.
| @app.after_request | ||
| def _emit_request_id(response: Response) -> Response: | ||
| rid = request.environ.get("request_id") | ||
| if rid: | ||
| response.headers["X-Request-ID"] = rid | ||
| structlog.contextvars.clear_contextvars() | ||
| return response |
There was a problem hiding this comment.
Clearing context variables in after_request is not robust because this hook is skipped if an unhandled exception occurs during request processing. This can lead to context leakage (e.g., an old request_id appearing in logs for a subsequent request on the same worker thread). Using teardown_request ensures the context is cleared regardless of whether the request succeeded or failed.
| @app.after_request | |
| def _emit_request_id(response: Response) -> Response: | |
| rid = request.environ.get("request_id") | |
| if rid: | |
| response.headers["X-Request-ID"] = rid | |
| structlog.contextvars.clear_contextvars() | |
| return response | |
| @app.after_request | |
| def _emit_request_id(response: Response) -> Response: | |
| rid = request.environ.get("request_id") | |
| if rid: | |
| response.headers["X-Request-ID"] = rid | |
| return response | |
| @app.teardown_request | |
| def _clear_request_context(exception: Exception | None = None) -> None: | |
| structlog.contextvars.clear_contextvars() |
| # Accept either raw bytes (from gpt-image-1 result) or a base64 string | ||
| # (from older callers / serialized plans). | ||
| raw: bytes = base64.b64decode(payload) if isinstance(payload, str) else payload |
There was a problem hiding this comment.
The base64 decoding of the image payload should be wrapped in a try-except block. If the input string is malformed or not valid base64, base64.b64decode will raise an exception, which would crash the entire personalization process for the current request. Adding error handling here makes the patcher more resilient to unexpected input.
| # Accept either raw bytes (from gpt-image-1 result) or a base64 string | |
| # (from older callers / serialized plans). | |
| raw: bytes = base64.b64decode(payload) if isinstance(payload, str) else payload | |
| # Accept either raw bytes (from gpt-image-1 result) or a base64 string | |
| # (from older callers / serialized plans). | |
| try: | |
| raw: bytes = base64.b64decode(payload) if isinstance(payload, str) else payload | |
| except Exception: | |
| log.warning("invalid_image_payload", slot_id=slot_id) | |
| continue |
| **Out of scope (deferred to Phase 7 / cleanup sprint):** | ||
| - Type hints on `app.py` and full `kratos_clone/` (gradual typing — flip the soft gate when ready) | ||
| - Bumping bandit gate to MEDIUM | ||
| - Type stubs for the OpenAI SDK overload-mismatch (see `# type: ignore[call-overload]` in `personalize/openai_client.py`) |
There was a problem hiding this comment.
The lines immediately following this section (128-130) appear to be dangling table rows from the previous version of the roadmap. They are redundant with the information now provided in the updated table (lines 117-122) and should be removed to maintain correct Markdown structure.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2747ccfe2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _emit_request_id(response: Response) -> Response: | ||
| rid = request.environ.get("request_id") | ||
| if rid: | ||
| response.headers["X-Request-ID"] = rid | ||
| structlog.contextvars.clear_contextvars() |
There was a problem hiding this comment.
The request ID context is only cleared in after_request, but Flask skips after_request when exceptions are propagated (notably in debug/testing modes). Since this app’s __main__ runs with debug=True, an unhandled error can leave structlog contextvars populated and mis-attribute subsequent logs to the previous request. Add a teardown_request (or equivalent finally-path) clear to guarantee cleanup on all request exit paths.
Useful? React with 👍 / 👎.
1 participant
Closes Phase 6 of ROADMAP.md.
Summary
.github/dependabot.ymlpyproject.toml [tool.ruff].github/workflows/ci.yml mypy:|| true).github/workflows/ci.yml bandit:request_idmiddlewareapp.pyKCD_*env-var referenceREADME.mdCode-quality fixes surfaced by new linters
hashlib.md5(url.encode())inkratos_clone/capture.pywas flagged HIGH. Used purely as a content-addressable filename hash (not security). Annotated withusedforsecurity=Falseper stdlib idiom.app.run(debug=True)inapp.py __main__— only ever runs in dev (production usesgunicorn wsgi:app). Annotated# nosec B201with comment.try/except/passinapp.py:_purge_session+process_download— replaced withcontextlib.suppress(Exception).Optional[Callable[...]]inkratos_clone/capture.py→Callable[...] | None(PEP 604).Test plan
uv run pytest -q→ 183 passed, 2 skipped (live OpenAI gated)uv run ruff check && ruff format --check→ all cleanuv run mypy personalize/→ no issues founduv run bandit -r personalize/ kratos_clone/ scripts/ app.py --severity-level high→ exit 0import appthread-count invariant preserved (P2-7)Out of scope (deferred, see TODO.md)
app.pyandkratos_clone/(gradual; soft mypy gate stays soft until ready)Commits