Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens runtime configuration and improves structured logging consistency across services by validating key Settings fields, normalizing log_level, enriching Loguru records with stable context fields, and clearing per-request ContextVar state in the gateway middleware; it also adds unit/integration coverage for these behaviors.
Changes:
- Added Pydantic
Fieldconstraints for numeric settings and alog_levelnormalizer/validator inSettings. - Added a Loguru record patcher to inject
service,request_id, andjob_idinto structured logs. - Introduced
clear_request_context()and invoked it from the FastAPI middleware; added tests for config/logging and a gateway request-context test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/minutes_core/config.py |
Adds numeric constraints and normalizes/validates log_level early. |
src/minutes_core/logging.py |
Introduces _record_patch and clear_request_context; wires patcher into Loguru configuration. |
src/minutes_gateway/app.py |
Ensures request ContextVars are cleared via middleware finally. |
tests/core/test_config.py |
New unit tests for settings validation and log level normalization. |
tests/core/test_logging.py |
Adds tests for record patching and context clearing. |
tests/gateway/test_app.py |
Adds an integration-style test intended to assert request context is cleared after /health. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| bind_request_context(request_id=request.state.request_id) | ||
| response = await call_next(request) | ||
| response.headers["x-request-id"] = request.state.request_id | ||
| return response | ||
| try: | ||
| response = await call_next(request) | ||
| response.headers["x-request-id"] = request.state.request_id | ||
| return response | ||
| finally: | ||
| clear_request_context() |
| from minutes_core.logging import ( | ||
| InterceptHandler, | ||
| _record_patch, | ||
| bind_request_context, | ||
| clear_request_context, | ||
| job_id_var, | ||
| request_id_var, | ||
| ) |
| """请求结束后应清理 ContextVar，避免跨请求上下文残留。""" | ||
| from minutes_core.logging import job_id_var, request_id_var | ||
|
|
||
| with gateway_harness_factory() as harness: | ||
| response = harness.client.get("/health", headers={"x-request-id": "req-cleanup"}) | ||
| assert response.status_code == 200 | ||
| assert response.headers["x-request-id"] == "req-cleanup" | ||
|
|
||
| assert request_id_var.get() is None | ||
| assert job_id_var.get() is None |
| valid_levels = {"TRACE", "DEBUG", "INFO", "SUCCESS", "WARNING", "ERROR", "CRITICAL"} | ||
| if normalized not in valid_levels: | ||
| valid = ", ".join(sorted(valid_levels)) | ||
| raise ValueError(f"Invalid MINUTES_LOG_LEVEL={value!r}. Expected one of: {valid}") |
![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: b70aa91dea
ℹ️ 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".
| finally: | ||
| clear_request_context() |
There was a problem hiding this comment.
Clearing request_id_var/job_id_var in this finally runs immediately after call_next() returns a Response, not after the response body is fully sent, so logs emitted during response streaming (for example the SSE path returned by stream_job_events) or later middleware/server logging for that request lose correlation IDs and get null context fields. Move cleanup to a point that executes after response completion (e.g., ASGI send completion or a response-finalization hook) so per-request logs stay traceable through the full lifecycle.
Useful? React with 👍 / 👎.
…d actor job_id binding Cherry-pick improvements from codex/refine-settings-and-logging-for-stability (PR #2), rejecting all reverts. Adds _record_patch for consistent structured log fields, clear_request_context to prevent cross-request leaks, Field constraints on config, log_level normalization (WARN→WARNING, FATAL→CRITICAL), and bind_request_context in all Dramatiq actors for job_id traceability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Motivation
log_levelearly.Description
Fieldconstraints toport,sync_wait_timeout_s,sqlite_busy_timeout_ms, andstt_timeout_secondsinSettingsto enforce valid ranges.@field_validatornormalize_log_levelto canonicalize and validatelog_level(including alias mapping forWARN/FATAL)._record_patchto injectservice,request_id, andjob_idinto each structured log record and wired it intologuruvialogger.configure(patcher=...).clear_request_context()and ensured the FastAPI request middleware calls it in afinallyblock to clear context after each request./health.Testing
tests/core/test_config.pywhich validateSettingsfield constraints andlog_levelnormalization, and they passed.tests/core/test_logging.pywhich exerciseInterceptHandler,_record_patch,bind_request_context, andclear_request_context, and they passed.tests/gateway/test_app.pywhich verifies the request middleware clears context after requests, and they passed.Codex Task