feat(api): RFC 9457 Phase 2 — ProblemDetail and content negotiation#496
feat(api): RFC 9457 Phase 2 — ProblemDetail and content negotiation#496
Conversation
Add application/problem+json support with content negotiation via Accept header, completing RFC 9457 compliance. Rename ErrorDetail.message to detail, add title/type fields, and introduce ProblemDetail model for bare RFC 9457 responses. - Add CATEGORY_TITLES map, category_title(), category_type_uri() helpers - Add ProblemDetail model with status range and retry_after validation - Add _wants_problem_json() and _build_response() for content negotiation - Update all 10 exception handlers to support both response formats - Add API_CONTENT_NEGOTIATED event constant - Update TypeScript types (ErrorDetail, ProblemDetail interface) - Add docs/errors.md error reference page - Update operations.md design spec with Phase 2 fields Closes #464
…ss, test split Pre-reviewed by 11 agents, 10 findings addressed: - Wrap _wants_problem_json defensively (exception handlers must never crash) - Add defensive fallback in _build_response for Pydantic ValidationError - Improve _get_instance_id logging with error context - Change error_code field from int to ErrorCode (IntEnum) on both models - Change type field from str to NotBlankStr on both models - Extract shared _check_retry_after validator (DRY) - Forward validation details in handle_validation_error - Fix type URIs to use fragment anchors (#auth not /auth) - Add anchor IDs to docs/errors.md for resolvable type URIs - Split test_exception_handlers.py (1029→748 lines) into test_content_negotiation.py - Add ProblemDetail boundary tests (status 399/600) and immutability test
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements RFC 9457–style error responses and content negotiation: adds a bare ProblemDetail DTO, enriches ErrorDetail with title/type and ErrorCode, adds category title/URI helpers and new observability events, and updates exception handlers to return either ApiResponse envelope or application/problem+json based on Accept headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Exception Handler
participant Neg as Content Negotiation
participant Builder as Response Builder
participant Response as Response
Client->>Handler: Request (with Accept)
activate Handler
Handler->>Neg: _wants_problem_json(request)
activate Neg
Neg->>Neg: parse Accept header
Neg-->>Handler: wants_problem_json (bool)
deactivate Neg
Handler->>Handler: assemble error metadata (detail, error_code, category)
Handler->>Builder: _build_response(wants_problem_json, metadata)
activate Builder
alt wants_problem_json == true
Builder->>Builder: create ProblemDetail (type, title, status, detail, instance, error_code, error_category, retryable, retry_after)
Builder->>Response: application/problem+json payload
else
Builder->>Builder: create ApiResponse envelope with ErrorDetail (detail, title, type, error_code, ...)
Builder->>Response: application/json payload
end
deactivate Builder
Handler-->>Client: Response
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the API's error handling by implementing the second phase of RFC 9457. It introduces content negotiation for error responses, allowing clients to request either a traditional API response envelope or a bare, machine-readable Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 194: The single long event-constant line should be broken into grouped
bullets by domain (e.g., api, meeting, memory, performance, approval_gate,
persistence, etc.) so each domain's constants (like API_REQUEST_STARTED,
API_REQUEST_COMPLETED from events.api; MEETING_STARTED, MEETING_SCHEDULER_ERROR
from events.meeting; MEMORY_BACKEND_CONNECTED, MEMORY_ENTRY_STORED from
events.memory; PERF_METRIC_RECORDED, PERF_LLM_SAMPLE_STARTED from
events.performance; APPROVAL_GATE_* from events.approval_gate; PERSISTENCE_*
from events.persistence) appear on their own bullet lines; keep the final import
guidance intact ("Import directly: from synthorg.observability.events.<domain>
import EVENT_CONSTANT") and ensure each bullet shows domain and examples of
constants to make reviews easier and reduce line-length/merge conflicts.
In `@docs/design/operations.md`:
- Around line 1001-1002: Update the two table entries for `title` and `type` to
use "e.g.," (i.e., add a comma after "e.g.") so the examples read "(e.g.,
\"Authentication Error\")" and "(e.g., `https://synthorg.io/docs/errors#auth`)";
locate the rows containing the `title` and `type` fields and edit the example
parentheticals accordingly for consistency.
In `@docs/errors.md`:
- Around line 29-32: Change the local curl examples that use
"https://localhost:8000" to "http://localhost:8000" so they work in typical
local dev setups without TLS; update both occurrences shown in the diff (the
curl snippet at the example using the Accept and Authorization headers and the
second identical example referenced at lines 56-58) to use http and ensure the
rest of the command (headers and path /api/v1/tasks/nonexistent) remains
unchanged.
In `@src/synthorg/api/exception_handlers.py`:
- Around line 185-195: The fallback Response returned in the exception handler
(the Response call that returns content={"error": "Internal server error"},
status_code=status_code) should include an explicit
media_type="application/json" to avoid relying on Litestar's inference; update
the Response invocation in exception_handlers.py (the block that logs
API_REQUEST_ERROR via logger.error with exc_info=True) to pass
media_type="application/json" so the content type is explicit and consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b971daa4-24ab-4951-994f-5b7ecd52904a
📒 Files selected for processing (15)
CLAUDE.mddocs/design/operations.mddocs/errors.mdmkdocs.ymlsrc/synthorg/api/dto.pysrc/synthorg/api/errors.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/observability/events/api.pytests/unit/api/test_content_negotiation.pytests/unit/api/test_dto.pytests/unit/api/test_dto_properties.pytests/unit/api/test_errors.pytests/unit/api/test_exception_handlers.pyweb/src/__tests__/api/client.property.test.tsweb/src/api/types.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Preview
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do not usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/errors.pytests/unit/api/test_exception_handlers.pytests/unit/api/test_errors.pytests/unit/api/test_dto.pytests/unit/api/test_content_negotiation.pysrc/synthorg/api/dto.pytests/unit/api/test_dto_properties.pysrc/synthorg/api/exception_handlers.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; for non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/errors.pysrc/synthorg/api/dto.pysrc/synthorg/api/exception_handlers.py
web/src/**/*.{vue,ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint frontend code with ESLint; type-check with vue-tsc; test with Vitest; audit with npm audit (critical + high severity)
Files:
web/src/__tests__/api/client.property.test.tsweb/src/api/types.ts
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update the relevant
docs/design/page to reflect approved deviations from the spec
Files:
docs/design/operations.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_errors.pytests/unit/api/test_dto.pytests/unit/api/test_content_negotiation.pytests/unit/api/test_dto_properties.py
🧠 Learnings (14)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately without retry. RetryExhaustedError signals that all retries failed — the engine layer catches this to trigger fallback chains.
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects `RateLimitError.retry_after` from providers — automatically pauses future requests
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects RateLimitError.retry_after from providers — automatically pauses future requests.
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Pure data models, enums, and re-exports do NOT need logging
Applied to files:
CLAUDE.md
🧬 Code graph analysis (8)
src/synthorg/api/errors.py (1)
web/src/api/types.ts (1)
ErrorCategory(93-101)
tests/unit/api/test_exception_handlers.py (2)
src/synthorg/api/errors.py (4)
category_title(105-114)category_type_uri(117-126)ErrorCategory(15-28)NotFoundError(167-175)src/synthorg/api/exception_handlers.py (2)
handle_http_exception(414-447)handle_unexpected(326-338)
tests/unit/api/test_errors.py (2)
src/synthorg/api/errors.py (3)
category_title(105-114)category_type_uri(117-126)ErrorCategory(15-28)web/src/api/types.ts (1)
ErrorCategory(93-101)
tests/unit/api/test_dto.py (2)
src/synthorg/api/errors.py (4)
ErrorCategory(15-28)ErrorCode(31-69)category_title(105-114)category_type_uri(117-126)src/synthorg/api/dto.py (2)
ErrorDetail(50-84)ApiResponse(127-155)
tests/unit/api/test_content_negotiation.py (2)
src/synthorg/api/errors.py (6)
ErrorCategory(15-28)ErrorCode(31-69)ServiceUnavailableError(222-231)UnauthorizedError(211-219)category_title(105-114)category_type_uri(117-126)src/synthorg/api/exception_handlers.py (1)
_wants_problem_json(83-99)
src/synthorg/api/dto.py (2)
src/synthorg/api/errors.py (2)
ErrorCategory(15-28)ErrorCode(31-69)web/src/api/types.ts (4)
ErrorCategory(93-101)ErrorCode(103-111)ErrorDetail(113-122)ProblemDetail(124-134)
tests/unit/api/test_dto_properties.py (2)
src/synthorg/api/dto.py (2)
ProblemDetail(87-121)ErrorDetail(50-84)src/synthorg/api/errors.py (4)
ErrorCategory(15-28)ErrorCode(31-69)category_title(105-114)category_type_uri(117-126)
src/synthorg/api/exception_handlers.py (3)
src/synthorg/api/dto.py (3)
ApiResponse(127-155)ErrorDetail(50-84)ProblemDetail(87-121)web/src/api/types.ts (5)
ApiResponse(139-141)ErrorDetail(113-122)ProblemDetail(124-134)ErrorCategory(93-101)ErrorCode(103-111)src/synthorg/api/errors.py (4)
ErrorCategory(15-28)ErrorCode(31-69)category_title(105-114)category_type_uri(117-126)
🪛 LanguageTool
docs/design/operations.md
[style] ~1001-~1001: A comma is missing here.
Context: ...e|str| Static per-category title (e.g. "Authentication Error") | |type|s...
(EG_NO_COMMA)
[style] ~1002-~1002: A comma is missing here.
Context: ...cumentation URI for the error category (e.g. https://synthorg.io/docs/errors#auth)...
(EG_NO_COMMA)
🔇 Additional comments (24)
src/synthorg/observability/events/api.py (1)
50-50: Good addition of a dedicated content-negotiation event constant.This keeps observability calls on shared constants instead of ad-hoc strings.
Based on learnings: Event names should always use constants from domain-specific modules under
synthorg.observability.events.web/src/api/types.ts (1)
114-134: RFC 9457 type-shape updates look correct.
ErrorDetailnow matches the renamed/expanded backend fields, andProblemDetailcleanly models the negotiatedapplication/problem+jsonpayload.mkdocs.yml (1)
124-124: Navigation update is correct.Adding
Error ReferenceunderReferencemakes the new RFC 9457 docs discoverable in-site.web/src/__tests__/api/client.property.test.ts (1)
8-10: Test updates correctly track the new error contract.Switching fixtures/usages to
detailplustitle/typekeeps property tests aligned with the RFC 9457 payload changes.Also applies to: 42-43, 116-117
src/synthorg/api/errors.py (1)
89-127: Category metadata helpers are well-structured and consistent.The immutable
CATEGORY_TITLESmap and typedcategory_title/category_type_urihelpers are clean and RFC 9457-aligned.tests/unit/api/test_errors.py (1)
7-19: Strong metadata coverage added for error taxonomy helpers.These tests materially improve regression safety for category titles and type URI generation.
Also applies to: 92-139
tests/unit/api/test_dto_properties.py (3)
373-385: Frozen model mutation raisesValidationErrorin Pydantic v2 — verify this is intentional.In Pydantic v2, attempting to mutate a frozen model attribute raises
ValidationError(specifically afrozen_instanceerror). This test correctly expectsValidationError, which aligns with Pydantic v2.12.5 behavior.
229-264: LGTM — strategy correctly enforces retry_after/retryable consistency.The
flatmappattern properly captures the category to derive consistenttitleandtypefields viacategory_title(cat)andcategory_type_uri(cat). Both branches (retryable=True/False) correctly handle theretry_afterconstraint.
311-348: LGTM — ProblemDetail strategy mirrors ErrorDetail pattern.Correctly generates
statusin the 400–599 range and maintainsretry_after/retryableconsistency.tests/unit/api/test_dto.py (2)
30-43: LGTM — clean helper for constructing ErrorDetail with RFC 9457 fields.The helper correctly derives
titleandtypefrom the category using the canonical helpers, reducing boilerplate and ensuring consistency across tests.
91-105: LGTM — comprehensive field validation.Test validates all ErrorDetail fields including the new
detailfield (renamed frommessage).tests/unit/api/test_content_negotiation.py (3)
43-72: LGTM — thorough assertion helper for RFC 9457 structure.The helper validates all required RFC 9457 fields and correctly asserts envelope keys are absent in bare ProblemDetail responses.
188-204: Good coverage — 5xx detail scrubbing prevents information leakage.Test correctly verifies that internal error details (IP addresses, connection strings) are not exposed in problem+json responses.
324-346: LGTM — defensive behavior verified for _wants_problem_json.Tests cover nominal cases and the defensive fallback when Accept parsing raises an exception.
docs/design/operations.md (1)
979-1025: LGTM — documentation accurately reflects RFC 9457 implementation.The updated error response format section clearly explains both response formats (envelope vs. bare ProblemDetail), content negotiation via Accept header, and all field definitions align with the code changes.
tests/unit/api/test_exception_handlers.py (3)
59-82: LGTM — assertion helper updated for RFC 9457 fields.The helper now validates
detail,title, andtypefields, ensuring RFC 9457 compliance across all exception handler tests.
551-564: LGTM — explicit assertion for RFC 9457 title and type.Hardcoding expected values ("Resource Not Found", "https://synthorg.io/docs/errors#not_found") provides clear regression protection for the category metadata.
710-725: LGTM — verifies title/type derivation from category.Test confirms that
_build_error_responsecorrectly populatestitleandtypeusingcategory_titleandcategory_type_uri.src/synthorg/api/dto.py (3)
40-47: LGTM — shared validation helper reduces duplication.Extracting
_check_retry_afteras a module-level helper follows DRY and ensures consistent validation betweenErrorDetailandProblemDetail.
87-121: LGTM — ProblemDetail correctly models RFC 9457 bare response.The model follows RFC 9457 field ordering (
type,title,statusfirst) and includes all extension fields (error_code,error_category,retryable,retry_after). The frozen config and validator ensure immutability and consistency.
71-78: LGTM — ErrorDetail field updates align with RFC 9457.The rename from
messagetodetailand addition oftitle/typefields complete the RFC 9457 alignment. Theerror_codetype change toErrorCodeadds type safety.src/synthorg/api/exception_handlers.py (3)
83-99: LGTM — defensive implementation prevents exception handler crashes.The
try/exceptwrapper withreturn Falsefallback ensures Accept header parsing issues never crash the exception handler. This aligns with the module docstring's "last line of defense" principle.
131-195: LGTM — content negotiation with robust fallback.The implementation correctly:
- Checks client preference via
_wants_problem_json- Builds appropriate response type (ProblemDetail vs ApiResponse)
- Falls back to a minimal dict response if response construction fails
The
# type: ignore[return-value]on line 192 is acceptable since the fallback{"error": ...}dict is a last-resort response when structured response building fails.
258-270: LGTM — handler correctly uses_build_responsewith content negotiation.All handlers now route through
_build_responsefor consistent content negotiation and RFC 9457 compliance.
CLAUDE.md
Outdated
| - **Never** use `import logging` / `logging.getLogger()` / `print()` in application code | ||
| - **Variable name**: always `logger` (not `_logger`, not `log`) | ||
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`, `CFO_ANOMALY_DETECTED` from `events.cfo`, `CONFLICT_DETECTED` from `events.conflict`, `MEETING_STARTED` from `events.meeting`, `MEETING_SCHEDULER_STARTED` from `events.meeting`, `MEETING_SCHEDULER_ERROR` from `events.meeting`, `MEETING_SCHEDULER_STOPPED` from `events.meeting`, `MEETING_PERIODIC_TRIGGERED` from `events.meeting`, `MEETING_EVENT_TRIGGERED` from `events.meeting`, `MEETING_PARTICIPANTS_RESOLVED` from `events.meeting`, `MEETING_NO_PARTICIPANTS` from `events.meeting`, `MEETING_NOT_FOUND` from `events.meeting`, `CLASSIFICATION_START` from `events.classification`, `CONSOLIDATION_START` from `events.consolidation`, `ORG_MEMORY_QUERY_START` from `events.org_memory`, `API_REQUEST_STARTED` from `events.api`, `API_REQUEST_COMPLETED` from `events.api`, `API_REQUEST_ERROR` from `events.api`, `API_ROUTE_NOT_FOUND` from `events.api`, `API_HEALTH_CHECK` from `events.api`, `API_COORDINATION_STARTED` from `events.api`, `API_COORDINATION_COMPLETED` from `events.api`, `API_COORDINATION_FAILED` from `events.api`, `API_COORDINATION_AGENT_RESOLVE_FAILED` from `events.api`, `CODE_RUNNER_EXECUTE_START` from `events.code_runner`, `DOCKER_EXECUTE_START` from `events.docker`, `MCP_INVOKE_START` from `events.mcp`, `SECURITY_EVALUATE_START` from `events.security`, `HR_HIRING_REQUEST_CREATED` from `events.hr`, `PERF_METRIC_RECORDED` from `events.performance`, `PERF_LLM_SAMPLE_STARTED` from `events.performance`, `PERF_LLM_SAMPLE_COMPLETED` from `events.performance`, `PERF_LLM_SAMPLE_FAILED` from `events.performance`, `PERF_OVERRIDE_SET` from `events.performance`, `PERF_OVERRIDE_CLEARED` from `events.performance`, `PERF_OVERRIDE_APPLIED` from `events.performance`, `PERF_OVERRIDE_EXPIRED` from `events.performance`, `TRUST_EVALUATE_START` from `events.trust`, `PROMOTION_EVALUATE_START` from `events.promotion`, `PROMPT_BUILD_START` from `events.prompt`, `MEMORY_RETRIEVAL_START` from `events.memory`, `MEMORY_BACKEND_CONNECTED` from `events.memory`, `MEMORY_ENTRY_STORED` from `events.memory`, `MEMORY_BACKEND_SYSTEM_ERROR` from `events.memory`, `MEMORY_RRF_FUSION_COMPLETE` from `events.memory`, `MEMORY_RRF_VALIDATION_FAILED` from `events.memory`, `AUTONOMY_ACTION_AUTO_APPROVED` from `events.autonomy`, `TIMEOUT_POLICY_EVALUATED` from `events.timeout`, `PERSISTENCE_AUDIT_ENTRY_SAVED` from `events.persistence`, `TASK_ENGINE_STARTED` from `events.task_engine`, `COORDINATION_STARTED` from `events.coordination`, `COORDINATION_FACTORY_BUILT` from `events.coordination`, `COMMUNICATION_DISPATCH_START` from `events.communication`, `COMPANY_STARTED` from `events.company`, `CONFIG_LOADED` from `events.config`, `CORRELATION_ID_CREATED` from `events.correlation`, `DECOMPOSITION_STARTED` from `events.decomposition`, `DELEGATION_STARTED` from `events.delegation`, `EXECUTION_LOOP_START` from `events.execution`, `CHECKPOINT_SAVED` from `events.checkpoint`, `PERSISTENCE_CHECKPOINT_SAVED` from `events.persistence`, `GIT_OPERATION_START` from `events.git`, `PARALLEL_GROUP_START` from `events.parallel`, `PERSONALITY_LOADED` from `events.personality`, `QUOTA_CHECKED` from `events.quota`, `ROLE_ASSIGNED` from `events.role`, `ROUTING_STARTED` from `events.routing`, `SANDBOX_EXECUTE_START` from `events.sandbox`, `TASK_CREATED` from `events.task`, `TASK_ASSIGNMENT_STARTED` from `events.task_assignment`, `TASK_ROUTING_STARTED` from `events.task_routing`, `TEMPLATE_LOADED` from `events.template`, `TOOL_INVOKE_START` from `events.tool`, `TOOL_OUTPUT_WITHHELD` from `events.tool`, `WORKSPACE_CREATED` from `events.workspace`, `APPROVAL_GATE_ESCALATION_DETECTED` from `events.approval_gate`, `APPROVAL_GATE_ESCALATION_FAILED` from `events.approval_gate`, `APPROVAL_GATE_INITIALIZED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFIED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFY_FAILED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARKED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARK_FAILED` from `events.approval_gate`, `APPROVAL_GATE_PARK_TASKLESS` from `events.approval_gate`, `APPROVAL_GATE_RESUME_STARTED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_RESUMED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_DELETE_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_TRIGGERED` from `events.approval_gate`, `APPROVAL_GATE_NO_PARKED_CONTEXT` from `events.approval_gate`, `APPROVAL_GATE_LOOP_WIRING_WARNING` from `events.approval_gate`, `STAGNATION_CHECK_PERFORMED` from `events.stagnation`, `STAGNATION_DETECTED` from `events.stagnation`, `STAGNATION_CORRECTION_INJECTED` from `events.stagnation`, `STAGNATION_TERMINATED` from `events.stagnation`, `PERSISTENCE_AGENT_STATE_SAVED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_FETCHED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_ACTIVE_QUERIED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_DELETED` from `events.persistence`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` | ||
| - **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`, `CFO_ANOMALY_DETECTED` from `events.cfo`, `CONFLICT_DETECTED` from `events.conflict`, `MEETING_STARTED` from `events.meeting`, `MEETING_SCHEDULER_STARTED` from `events.meeting`, `MEETING_SCHEDULER_ERROR` from `events.meeting`, `MEETING_SCHEDULER_STOPPED` from `events.meeting`, `MEETING_PERIODIC_TRIGGERED` from `events.meeting`, `MEETING_EVENT_TRIGGERED` from `events.meeting`, `MEETING_PARTICIPANTS_RESOLVED` from `events.meeting`, `MEETING_NO_PARTICIPANTS` from `events.meeting`, `MEETING_NOT_FOUND` from `events.meeting`, `CLASSIFICATION_START` from `events.classification`, `CONSOLIDATION_START` from `events.consolidation`, `ORG_MEMORY_QUERY_START` from `events.org_memory`, `API_REQUEST_STARTED` from `events.api`, `API_REQUEST_COMPLETED` from `events.api`, `API_REQUEST_ERROR` from `events.api`, `API_ROUTE_NOT_FOUND` from `events.api`, `API_HEALTH_CHECK` from `events.api`, `API_COORDINATION_STARTED` from `events.api`, `API_COORDINATION_COMPLETED` from `events.api`, `API_COORDINATION_FAILED` from `events.api`, `API_COORDINATION_AGENT_RESOLVE_FAILED` from `events.api`, `API_CONTENT_NEGOTIATED` from `events.api`, `CODE_RUNNER_EXECUTE_START` from `events.code_runner`, `DOCKER_EXECUTE_START` from `events.docker`, `MCP_INVOKE_START` from `events.mcp`, `SECURITY_EVALUATE_START` from `events.security`, `HR_HIRING_REQUEST_CREATED` from `events.hr`, `PERF_METRIC_RECORDED` from `events.performance`, `PERF_LLM_SAMPLE_STARTED` from `events.performance`, `PERF_LLM_SAMPLE_COMPLETED` from `events.performance`, `PERF_LLM_SAMPLE_FAILED` from `events.performance`, `PERF_OVERRIDE_SET` from `events.performance`, `PERF_OVERRIDE_CLEARED` from `events.performance`, `PERF_OVERRIDE_APPLIED` from `events.performance`, `PERF_OVERRIDE_EXPIRED` from `events.performance`, `TRUST_EVALUATE_START` from `events.trust`, `PROMOTION_EVALUATE_START` from `events.promotion`, `PROMPT_BUILD_START` from `events.prompt`, `MEMORY_RETRIEVAL_START` from `events.memory`, `MEMORY_BACKEND_CONNECTED` from `events.memory`, `MEMORY_ENTRY_STORED` from `events.memory`, `MEMORY_BACKEND_SYSTEM_ERROR` from `events.memory`, `MEMORY_RRF_FUSION_COMPLETE` from `events.memory`, `MEMORY_RRF_VALIDATION_FAILED` from `events.memory`, `AUTONOMY_ACTION_AUTO_APPROVED` from `events.autonomy`, `TIMEOUT_POLICY_EVALUATED` from `events.timeout`, `PERSISTENCE_AUDIT_ENTRY_SAVED` from `events.persistence`, `TASK_ENGINE_STARTED` from `events.task_engine`, `COORDINATION_STARTED` from `events.coordination`, `COORDINATION_FACTORY_BUILT` from `events.coordination`, `COMMUNICATION_DISPATCH_START` from `events.communication`, `COMPANY_STARTED` from `events.company`, `CONFIG_LOADED` from `events.config`, `CORRELATION_ID_CREATED` from `events.correlation`, `DECOMPOSITION_STARTED` from `events.decomposition`, `DELEGATION_STARTED` from `events.delegation`, `EXECUTION_LOOP_START` from `events.execution`, `CHECKPOINT_SAVED` from `events.checkpoint`, `PERSISTENCE_CHECKPOINT_SAVED` from `events.persistence`, `GIT_OPERATION_START` from `events.git`, `PARALLEL_GROUP_START` from `events.parallel`, `PERSONALITY_LOADED` from `events.personality`, `QUOTA_CHECKED` from `events.quota`, `ROLE_ASSIGNED` from `events.role`, `ROUTING_STARTED` from `events.routing`, `SANDBOX_EXECUTE_START` from `events.sandbox`, `TASK_CREATED` from `events.task`, `TASK_ASSIGNMENT_STARTED` from `events.task_assignment`, `TASK_ROUTING_STARTED` from `events.task_routing`, `TEMPLATE_LOADED` from `events.template`, `TOOL_INVOKE_START` from `events.tool`, `TOOL_OUTPUT_WITHHELD` from `events.tool`, `WORKSPACE_CREATED` from `events.workspace`, `APPROVAL_GATE_ESCALATION_DETECTED` from `events.approval_gate`, `APPROVAL_GATE_ESCALATION_FAILED` from `events.approval_gate`, `APPROVAL_GATE_INITIALIZED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFIED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFY_FAILED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARKED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARK_FAILED` from `events.approval_gate`, `APPROVAL_GATE_PARK_TASKLESS` from `events.approval_gate`, `APPROVAL_GATE_RESUME_STARTED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_RESUMED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_DELETE_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_TRIGGERED` from `events.approval_gate`, `APPROVAL_GATE_NO_PARKED_CONTEXT` from `events.approval_gate`, `APPROVAL_GATE_LOOP_WIRING_WARNING` from `events.approval_gate`, `STAGNATION_CHECK_PERFORMED` from `events.stagnation`, `STAGNATION_DETECTED` from `events.stagnation`, `STAGNATION_CORRECTION_INJECTED` from `events.stagnation`, `STAGNATION_TERMINATED` from `events.stagnation`, `PERSISTENCE_AGENT_STATE_SAVED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_FETCHED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_ACTIVE_QUERIED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_DELETED` from `events.persistence`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider splitting the event-constant mega-line into grouped bullets.
Line 194 is hard to maintain and review as one line; grouping by domain (api, meeting, memory, etc.) will reduce future churn and improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 194, The single long event-constant line should be broken
into grouped bullets by domain (e.g., api, meeting, memory, performance,
approval_gate, persistence, etc.) so each domain's constants (like
API_REQUEST_STARTED, API_REQUEST_COMPLETED from events.api; MEETING_STARTED,
MEETING_SCHEDULER_ERROR from events.meeting; MEMORY_BACKEND_CONNECTED,
MEMORY_ENTRY_STORED from events.memory; PERF_METRIC_RECORDED,
PERF_LLM_SAMPLE_STARTED from events.performance; APPROVAL_GATE_* from
events.approval_gate; PERSISTENCE_* from events.persistence) appear on their own
bullet lines; keep the final import guidance intact ("Import directly: from
synthorg.observability.events.<domain> import EVENT_CONSTANT") and ensure each
bullet shows domain and examples of constants to make reviews easier and reduce
line-length/merge conflicts.
There was a problem hiding this comment.
Code Review
This pull request is an excellent implementation of RFC 9457 content negotiation. The changes are comprehensive, covering API logic, DTOs, documentation, and extensive testing. The introduction of the ProblemDetail model and the refactoring of exception handlers to support content negotiation are well-executed. The defensive programming in the exception handlers, with fallbacks and logging, significantly improves robustness. The addition of property-based tests for the new DTOs is a great way to ensure correctness. I have one suggestion to improve logging in the content negotiation logic.
| except Exception: | ||
| return False |
There was a problem hiding this comment.
The exception in _wants_problem_json is silently swallowed. While this function is inside an exception handler and needs to be defensive, swallowing exceptions without logging can hide bugs in parsing the Accept header. This could lead to clients not receiving the problem+json format they requested, with no server-side indication of why. It would be better to log this at a debug level, similar to how _get_instance_id handles fallbacks.
except Exception as exc:
logger.debug(
"accept_header_parse_failed",
error_type=type(exc).__qualname__,
error=str(exc),
)
return False- Add debug logging to _wants_problem_json (was silently swallowing exceptions) - Use ErrorCode.PERSISTENCE_ERROR in handle_persistence_error (was INTERNAL_ERROR) - Parse Retry-After header into retry_after body field in handle_http_exception - Fix _build_response fallback: status→500, explicit media_type, richer log context - Extract _build_problem_detail_response to keep _build_response under 50 lines - Wrap EXCEPTION_HANDLERS in MappingProxyType (immutability convention) - Replace bare string event with API_CORRELATION_FALLBACK/API_ACCEPT_PARSE_FAILED constants - Add detail length cap (_MAX_DETAIL_LEN=512) in handle_http_exception - Add _build_response fallback tests (envelope + problem+json paths) - Add Retry-After header→body propagation test - Deduplicate _make_app into conftest.make_exception_handler_app - Fix docs/errors.md: 502 overlap, planned annotations, http:// in curl examples - Fix docs/design/operations.md: comma after e.g. - Update CLAUDE.md api/ description with new helpers
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #496 +/- ##
==========================================
+ Coverage 93.36% 93.37% +0.01%
==========================================
Files 480 480
Lines 23202 23250 +48
Branches 2215 2217 +2
==========================================
+ Hits 21662 21710 +48
Misses 1216 1216
Partials 324 324 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/api/test_content_negotiation.py`:
- Around line 92-93: Add Google-style docstrings to each public test function
such as test_problem_json_for_auth_error (and the other public tests around
lines referenced) by inserting a short triple-quoted docstring immediately under
the def that describes the test purpose, inputs/conditions, and expected outcome
in Google style (Args:, Returns: if applicable, and a brief description). Update
functions like test_problem_json_for_auth_error, plus the other public test
methods noted in the comment, to have these docstrings so they comply with the
project's ruff D rules.
- Around line 65-313: Consolidate the repeated problem+json mapping tests
(test_problem_json_for_not_found, test_problem_json_for_auth_error,
test_problem_json_for_conflict, test_problem_json_for_validation_error,
test_problem_json_for_unexpected_error, and similar simple mappings) into a
single pytest.mark.parametrize test that iterates tuples of (exception or
exception factory, expected_status, expected_error_code,
expected_error_category, expected_retryable) and uses the shared pattern of
defining a route raising the exception, calling
TestClient(make_exception_handler_app(handler)) with headers {"Accept":
_PROBLEM_JSON}, asserting resp.status_code, asserting _PROBLEM_JSON in
resp.headers["content-type"], parsing body = resp.json(), and calling
self._assert_problem_detail(body, status=..., error_code=...,
error_category=..., retryable=...); keep specialized tests that assert headers
(test_problem_json_for_http_exception_headers), retry metadata, or 5xx scrubbing
as separate tests. Reference the symbols make_exception_handler_app, TestClient,
_PROBLEM_JSON, and _assert_problem_detail when locating the code to change.
- Around line 26-27: The module-level pytest marker only sets pytest.mark.unit;
update the module marker so the test module also has an explicit timeout by
adding pytest.mark.timeout(30) to the pytestmark tuple (or combine markers) so
pytestmark contains both pytest.mark.unit and pytest.mark.timeout(30); modify
the pytestmark declaration (symbol: pytestmark) accordingly.
In `@tests/unit/api/test_exception_handlers.py`:
- Around line 564-578: Update the test_error_detail_has_title_and_type test to
stop asserting hardcoded strings for the error_detail title and type; instead
call the helper functions category_title(NotFoundError) and
category_type_uri(NotFoundError) (or the equivalents used elsewhere) and compare
ed["title"] and ed["type"] to those returned values. Locate the assertions in
test_error_detail_has_title_and_type (which raises NotFoundError via the handler
and uses make_exception_handler_app) and replace the literal "Resource Not
Found" and "https://synthorg.io/docs/errors#not_found" checks with calls to
category_title and category_type_uri so the test uses the canonical metadata
providers.
- Around line 503-521: The module is missing the required 30-second timeout
marker; add or update the module-level pytestmark to enforce
pytest.mark.timeout(30) (ensure pytest is imported) so the entire test file —
including tests like test_http_429_retry_after_header_propagated_to_body, the
tests around lines ~551 and ~741 — runs with a 30s timeout; if a pytestmark
variable already exists, combine it with pytest.mark.timeout(30) rather than
creating a duplicate.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d84054c-b5e4-4591-a4e7-f2fbb9a7fbdf
📒 Files selected for processing (10)
CLAUDE.mddocs/design/operations.mddocs/errors.mdsrc/synthorg/api/app.pysrc/synthorg/api/exception_handlers.pysrc/synthorg/observability/events/api.pytests/unit/api/conftest.pytests/unit/api/test_content_negotiation.pytests/unit/api/test_exception_handlers.pytests/unit/api/test_middleware.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Sandbox
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update the relevant
docs/design/page to reflect approved deviations from the spec
Files:
docs/design/operations.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do not usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
src/synthorg/observability/events/api.pytests/unit/api/conftest.pytests/unit/api/test_content_negotiation.pytests/unit/api/test_exception_handlers.pysrc/synthorg/api/app.pytests/unit/api/test_middleware.pysrc/synthorg/api/exception_handlers.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Create new objects for immutability instead of mutating existing ones; for non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction andMappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, usecopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (usingmodel_copy(update=...)) for runtime state that evolves; never mix static config fields with mutable runtime fields in one model
Use Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; useNotBlankStrfor all identifier/name fields
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls); prefer structured concurrency over barecreate_task
Keep functions under 50 lines and files under 800 lines
Handle errors explicitly, never silently swallow exceptions
Validate at system boundaries (user input, external APIs, config files)
Every module with business logic must importfrom synthorg.observability import get_loggerand createlogger = get_logger(__name__); never useimport loggingorlogging.getLogger()orprint()in application code
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider,BUDGET_RECORD_ADDEDfromevents.budget); import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Pure data models, enums, and re-e...
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/app.pysrc/synthorg/api/exception_handlers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/api/conftest.pytests/unit/api/test_content_negotiation.pytests/unit/api/test_exception_handlers.pytests/unit/api/test_middleware.py
🧠 Learnings (13)
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from domain-specific modules under synthorg.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.pyCLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects `RateLimitError.retry_after` from providers — automatically pauses future requests
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects RateLimitError.retry_after from providers — automatically pauses future requests.
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)`. Never use `import logging` / `logging.getLogger()` / `print()` in application code. Variable name: always `logger` (not `_logger`, not `log`).
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : Structured kwargs in logging: always `logger.info(EVENT, key=value)` — never `logger.info('msg %s', val)`.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Every module with business logic must import `from synthorg.observability import get_logger` and create `logger = get_logger(__name__)`; never use `import logging` or `logging.getLogger()` or `print()` in application code
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Log all state transitions at INFO level with structured kwargs (never use % formatting); log all error paths at WARNING or ERROR with context before raising; log object creation and key function entry/exit at DEBUG
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : All state transitions must log at INFO.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/**/*.py : DEBUG logging for object creation, internal flow, entry/exit of key functions.
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Pure data models, enums, and re-exports do NOT need logging
Applied to files:
CLAUDE.md
🧬 Code graph analysis (2)
tests/unit/api/conftest.py (2)
src/synthorg/api/app.py (1)
create_app(430-585)tests/unit/api/test_exception_handlers.py (16)
handler(82-84)handler(102-104)handler(120-122)handler(139-141)handler(158-160)handler(176-178)handler(194-196)handler(211-213)handler(230-232)handler(249-251)handler(269-270)handler(289-290)handler(309-310)handler(328-329)handler(348-349)handler(367-368)
src/synthorg/api/exception_handlers.py (2)
src/synthorg/api/dto.py (2)
ErrorDetail(50-84)ProblemDetail(87-121)src/synthorg/api/errors.py (4)
ErrorCategory(15-28)ErrorCode(31-69)category_title(105-114)category_type_uri(117-126)
🔇 Additional comments (15)
src/synthorg/observability/events/api.py (1)
50-52: LGTM!The new event constants follow the established naming convention and are correctly typed as
Final[str]. These constants support the RFC 9457 content negotiation logging requirements.docs/design/operations.md (1)
979-1025: LGTM!The RFC 9457 documentation is comprehensive and accurately describes both response formats (envelope and bare ProblemDetail), content negotiation via Accept headers, and all field definitions. The cross-reference to the Error Reference page provides good navigation.
src/synthorg/api/exception_handlers.py (5)
88-109: LGTM!The
_wants_problem_jsonhelper is well-designed with defensive error handling. The fallback toFalseon parse failure ensures the handler never crashes, and the DEBUG-level logging provides observability without noise.
141-167: LGTM!The
_build_problem_detail_responsebuilder correctly constructs RFC 9457-compliant responses with all required fields, proper media type, and header forwarding.
170-230: LGTM!The
_build_responsefunction provides robust content negotiation with a fail-safe fallback. The defensive wrapper ensures exception handlers never crash, and the fallback correctly returns 500 for response-building failures while logging the original status code for debugging.
469-475: LGTM!The Retry-After header parsing handles the integer seconds format correctly. The case-insensitive header lookup and silent suppression of non-integer values (HTTP-date format) is pragmatic for agent consumers.
498-511: LGTM!The
EXCEPTION_HANDLERSmapping correctly usesMappingProxyTypefor read-only enforcement, following the immutability guidelines for non-Pydantic internal collections.src/synthorg/api/app.py (1)
574-574: LGTM!Converting
EXCEPTION_HANDLERSto a dict for Litestar is the correct approach — it preserves the immutability of the sourceMappingProxyTypewhile satisfying Litestar's expected mapping type.tests/unit/api/conftest.py (1)
37-42: LGTM!The
make_exception_handler_apphelper provides a clean, reusable way to create test apps with project exception handlers. The docstring is appropriately concise.tests/unit/api/test_middleware.py (1)
28-28: LGTM!The
dict(EXCEPTION_HANDLERS)pattern is consistently applied across all test app instantiations.CLAUDE.md (2)
116-116: LGTM!The API package description accurately reflects the new RFC 9457 additions including
ProblemDetail, category helpers, and content negotiation.
194-194: Event constants mega-line is growing.The new API event constants have been added correctly, but the single long line continues to grow. Consider splitting into grouped bullets by domain for maintainability.
docs/errors.md (1)
1-226: LGTM!The Error Reference documentation is comprehensive and well-structured. It accurately documents both response formats, the error taxonomy with correct codes, field references matching the Pydantic models, and practical retry guidance. The curl examples correctly use
http://localhost.tests/unit/api/test_content_negotiation.py (1)
31-337: Strong scenario coverage for negotiation behavior.This suite thoroughly exercises envelope vs
application/problem+jsonpaths and key error categories.tests/unit/api/test_exception_handlers.py (1)
741-780: Good defensive fallback coverage for response-construction failures.These tests materially improve resilience guarantees around
_build_response.
| pytestmark = pytest.mark.unit | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout marker usage and pytest-timeout config presence.
rg -n 'pytestmark\s*=|@pytest\.mark\.timeout\(|pytest\.mark\.timeout\(' tests/unit/api/test_content_negotiation.py tests/unit/api/test_exception_handlers.py
rg -n 'pytest-timeout|--timeout|timeout\s*=' pyproject.toml pytest.ini setup.cfg tox.ini 2>/dev/null || trueRepository: Aureliolo/synthorg
Length of output: 291
🏁 Script executed:
# Check pytest configuration context around the timeout setting
sed -n '220,235p' pyproject.tomlRepository: Aureliolo/synthorg
Length of output: 650
🏁 Script executed:
# Search for how timeouts are configured in other test files
rg -A2 -B2 'pytest\.mark\.timeout|pytestmark.*timeout' tests/ --type py | head -50Repository: Aureliolo/synthorg
Length of output: 2422
🏁 Script executed:
# Check if any test modules use explicit timeout markers
rg 'pytest\.mark\.timeout' tests/ --type pyRepository: Aureliolo/synthorg
Length of output: 22891
Add explicit timeout marker to this test module.
Line 26 has only @pytest.mark.unit but lacks the required explicit pytest.mark.timeout(30) marker. This is inconsistent with the established pattern across all other test modules in the codebase.
Suggested fix
-pytestmark = pytest.mark.unit
+pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_content_negotiation.py` around lines 26 - 27, The
module-level pytest marker only sets pytest.mark.unit; update the module marker
so the test module also has an explicit timeout by adding
pytest.mark.timeout(30) to the pytestmark tuple (or combine markers) so
pytestmark contains both pytest.mark.unit and pytest.mark.timeout(30); modify
the pytestmark declaration (symbol: pytestmark) accordingly.
| def test_problem_json_for_auth_error(self) -> None: | ||
| @get("/test") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add Google-style docstrings to public test methods.
Several public methods in this new file are missing docstrings; this violates the repository Python docstring rule.
As per coding guidelines, "Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules".
Also applies to: 114-115, 136-137, 197-198, 318-319, 323-324, 328-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_content_negotiation.py` around lines 92 - 93, Add
Google-style docstrings to each public test function such as
test_problem_json_for_auth_error (and the other public tests around lines
referenced) by inserting a short triple-quoted docstring immediately under the
def that describes the test purpose, inputs/conditions, and expected outcome in
Google style (Args:, Returns: if applicable, and a brief description). Update
functions like test_problem_json_for_auth_error, plus the other public test
methods noted in the comment, to have these docstrings so they comply with the
project's ruff D rules.
| def test_http_429_retry_after_header_propagated_to_body(self) -> None: | ||
| """Retry-After header value is parsed into the body field.""" | ||
|
|
||
| @get("/test") | ||
| async def handler() -> None: | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail="Slow down", | ||
| headers={"Retry-After": "60"}, | ||
| ) | ||
|
|
||
| with TestClient(make_exception_handler_app(handler)) as client: | ||
| resp = client.get("/test") | ||
| assert resp.status_code == 429 | ||
| body = resp.json() | ||
| assert resp.headers.get("retry-after") == "60" | ||
| assert body["error_detail"]["retry_after"] == 60 | ||
| assert body["error_detail"]["retryable"] is True | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm timeout marks are present for this file.
rg -n 'pytestmark\s*=|@pytest\.mark\.timeout\(|pytest\.mark\.timeout\(' tests/unit/api/test_exception_handlers.pyRepository: Aureliolo/synthorg
Length of output: 94
Add 30-second timeout marker to module-level pytestmark.
The file lacks the required timeout enforcement per coding guidelines. Update line 46 to include pytest.mark.timeout(30):
Suggested patch
-pytestmark = pytest.mark.unit
+pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]Also applies to tests at lines 503-521, 551-578, 741-780.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_exception_handlers.py` around lines 503 - 521, The module
is missing the required 30-second timeout marker; add or update the module-level
pytestmark to enforce pytest.mark.timeout(30) (ensure pytest is imported) so the
entire test file — including tests like
test_http_429_retry_after_header_propagated_to_body, the tests around lines ~551
and ~741 — runs with a 30s timeout; if a pytestmark variable already exists,
combine it with pytest.mark.timeout(30) rather than creating a duplicate.
…al helpers - Consolidate 6 simple problem+json mapping tests into a single parametrized test (not_found, auth, conflict, validation, unexpected, retryable) — per CLAUDE.md: "Prefer @pytest.mark.parametrize" - Replace hardcoded title/type strings with category_title() and category_type_uri() calls in test_error_detail_has_title_and_type
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/api/test_content_negotiation.py`:
- Around line 269-282: Add Google-style docstrings to the three public test
functions so they comply with ruff D rules: insert a triple-quoted docstring
immediately under the def line for test_returns_true_for_problem_json,
test_returns_false_for_json, and test_returns_false_for_none that contains a
one-line summary of what the test asserts (e.g., "Verify _wants_problem_json
returns True for 'application/problem+json'"). Keep the docstrings minimal
(single-line summary, optionally a short explanatory sentence) and ensure they
use standard triple quotes so linters accept them.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a91f3528-7ac3-484d-8058-8fe37da88c3a
📒 Files selected for processing (2)
tests/unit/api/test_content_negotiation.pytests/unit/api/test_exception_handlers.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Build Backend
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do not usefrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 — ruff enforces this on Python 3.14
Add type hints to all public functions and classes; enforce mypy strict mode
Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Keep line length to 88 characters (ruff enforced)
Files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_content_negotiation.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark unit tests with@pytest.mark.unit, integration tests with@pytest.mark.integration, e2e tests with@pytest.mark.e2e, and slow tests with@pytest.mark.slow
Useasyncio_mode = "auto"in pytest configuration — no manual@pytest.mark.asyncioneeded on async tests
Set 30-second timeout per test
Prefer@pytest.mark.parametrizefor testing similar cases
Use Hypothesis for property-based testing in Python (@given+@settings); useciprofile (200 examples, default) anddevprofile (1000 examples) controlled viaHYPOTHESIS_PROFILEenv var
Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic
Files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_content_negotiation.py
🧠 Learnings (11)
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to tests/**/*.py : Set 30-second timeout per test
Applied to files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test timeout: 30 seconds per test.
Applied to files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Test markers: pytest.mark.unit, pytest.mark.integration, pytest.mark.e2e, pytest.mark.slow. Coverage: 80% minimum (enforced in CI).
Applied to files:
tests/unit/api/test_exception_handlers.pytests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests — always fix them fully and fundamentally; for timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow exceptions
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects `RateLimitError.retry_after` from providers — automatically pauses future requests
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to src/synthorg/providers/**/*.py : Rate limiter respects RateLimitError.retry_after from providers — automatically pauses future requests.
Applied to files:
tests/unit/api/test_exception_handlers.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases
Applied to files:
tests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-16T07:36:16.026Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:36:16.026Z
Learning: Applies to **/*.py : Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules
Applied to files:
tests/unit/api/test_content_negotiation.py
📚 Learning: 2026-03-15T11:48:14.867Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T11:48:14.867Z
Learning: Applies to **/*.py : Docstrings: Google style, required on public classes and functions (enforced by ruff D rules).
Applied to files:
tests/unit/api/test_content_negotiation.py
🧬 Code graph analysis (1)
tests/unit/api/test_content_negotiation.py (3)
src/synthorg/api/errors.py (6)
ErrorCategory(15-28)ErrorCode(31-69)ServiceUnavailableError(222-231)UnauthorizedError(211-219)category_title(105-114)category_type_uri(117-126)src/synthorg/api/exception_handlers.py (1)
_wants_problem_json(88-109)tests/unit/api/conftest.py (1)
make_exception_handler_app(37-42)
🔇 Additional comments (13)
tests/unit/api/test_content_negotiation.py (4)
26-26: Add explicit timeout marker to this test module.The module-level
pytestmarkis missing the requiredpytest.mark.timeout(30)marker per coding guidelines.Suggested fix
-pytestmark = pytest.mark.unit +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]Based on learnings: "Set 30-second timeout per test".
34-63: LGTM!The
_assert_problem_detailhelper is well-structured with comprehensive RFC 9457 field validation and clear separation between required fields and envelope exclusion checks.
65-150: LGTM!Excellent use of
@pytest.mark.parametrizeconsolidating six error-mapping test cases with descriptive IDs. The assertions correctly usecategory_title()andcategory_type_uri()helpers instead of hardcoded strings.
152-263: LGTM!Comprehensive coverage of content negotiation edge cases: 5xx detail scrubbing, safe header forwarding, and Accept header preference logic. All methods have appropriate docstrings.
tests/unit/api/test_exception_handlers.py (9)
46-46: Add 30-second timeout marker to module-level pytestmark.The module lacks the required timeout enforcement per coding guidelines.
Suggested fix
-pytestmark = pytest.mark.unit +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]Based on learnings: "Set 30-second timeout per test".
28-29: LGTM!The new imports for
category_titleandcategory_type_urihelpers align with the RFC 9457 changes and are used throughout the test assertions.
68-73: LGTM!The updated assertions properly validate RFC 9457 compliance:
detailmatches the top-levelerrorfield, andtitle/typefields are validated for expected format.
86-86: LGTM!The consistent migration to
make_exception_handler_app(handler)fixture improves test maintainability and ensures all tests use the same exception handler configuration.
503-521: LGTM!Good coverage of
Retry-Afterheader propagation: validates the header value ("60" as string) is correctly parsed into the body field (60 as int) and thatretryableis set toTrue.
551-562: LGTM!Important regression test ensuring the
error_detail.detailfield stays synchronized with the top-levelerrorfield.
564-577: LGTM!The test now correctly uses
category_title()andcategory_type_uri()helpers instead of hardcoded strings, addressing the previous review feedback.
727-738: LGTM!Test correctly updated to use
detailparameter (renamed frommessage) and validates the new RFC 9457title/typefields using the canonical helper functions.
741-780: LGTM!Excellent defensive testing for the fallback behavior: covers both the envelope path (
_build_error_response) and the problem+json path (_build_problem_detail_response), ensuring a graceful 500 response when construction fails.
| def test_returns_true_for_problem_json(self) -> None: | ||
| request = MagicMock() | ||
| request.accept.best_match.return_value = _PROBLEM_JSON | ||
| assert _wants_problem_json(request) is True | ||
|
|
||
| def test_returns_false_for_json(self) -> None: | ||
| request = MagicMock() | ||
| request.accept.best_match.return_value = "application/json" | ||
| assert _wants_problem_json(request) is False | ||
|
|
||
| def test_returns_false_for_none(self) -> None: | ||
| request = MagicMock() | ||
| request.accept.best_match.return_value = None | ||
| assert _wants_problem_json(request) is False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add docstrings to public test methods.
The test methods test_returns_true_for_problem_json, test_returns_false_for_json, and test_returns_false_for_none are missing Google-style docstrings required by coding guidelines.
Suggested fix
def test_returns_true_for_problem_json(self) -> None:
+ """Return True when Accept header prefers problem+json."""
request = MagicMock()
request.accept.best_match.return_value = _PROBLEM_JSON
assert _wants_problem_json(request) is True
def test_returns_false_for_json(self) -> None:
+ """Return False when Accept header prefers application/json."""
request = MagicMock()
request.accept.best_match.return_value = "application/json"
assert _wants_problem_json(request) is False
def test_returns_false_for_none(self) -> None:
+ """Return False when best_match returns None."""
request = MagicMock()
request.accept.best_match.return_value = None
assert _wants_problem_json(request) is FalseAs per coding guidelines: "Use Google-style docstrings on all public classes and functions — required and enforced by ruff D rules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_content_negotiation.py` around lines 269 - 282, Add
Google-style docstrings to the three public test functions so they comply with
ruff D rules: insert a triple-quoted docstring immediately under the def line
for test_returns_true_for_problem_json, test_returns_false_for_json, and
test_returns_false_for_none that contains a one-line summary of what the test
asserts (e.g., "Verify _wants_problem_json returns True for
'application/problem+json'"). Keep the docstrings minimal (single-line summary,
optionally a short explanatory sentence) and ensure they use standard triple
quotes so linters accept them.
🤖 I have created a release *beep* *boop* --- ## [0.3.1](v0.3.0...v0.3.1) (2026-03-17) ### Features * **api:** RFC 9457 Phase 2 — ProblemDetail and content negotiation ([#496](#496)) ([30f7c49](30f7c49)) * **cli:** verify container image signatures and SLSA provenance on pull ([#492](#492)) ([bef272d](bef272d)), closes [#491](#491) * **engine:** implement context budget management in execution loops ([#520](#520)) ([181eb8a](181eb8a)), closes [#416](#416) * implement settings persistence layer (DB-backed config) ([#495](#495)) ([4bd99f7](4bd99f7)), closes [#450](#450) * **memory:** implement dual-mode archival in memory consolidation ([#524](#524)) ([4603c9e](4603c9e)), closes [#418](#418) * migrate config consumers to read through SettingsService ([#510](#510)) ([32f553d](32f553d)) * **settings:** implement settings change subscriptions for service hot-reload ([#526](#526)) ([53f908e](53f908e)), closes [#503](#503) * **settings:** register API config in SettingsService with 2-phase init ([#518](#518)) ([29f7481](29f7481)) * **tools:** add SSRF prevention for git clone URLs ([#505](#505)) ([492dd0d](492dd0d)) * **tools:** wire RootConfig.git_clone to GitCloneTool instantiation ([#519](#519)) ([b7d8172](b7d8172)) ### Bug Fixes * **api:** replace JWT query parameter with one-time ticket for WebSocket auth ([#493](#493)) ([22a25f6](22a25f6)), closes [#343](#343) ### Documentation * add uv cache lock contention handling to worktree skill ([#500](#500)) ([bd85a8d](bd85a8d)) * document RFC 9457 dual response formats in OpenAPI schema ([#506](#506)) ([8dd2524](8dd2524)) ### Maintenance * upgrade jsdom from 28 to 29 ([#499](#499)) ([1ea2249](1ea2249)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
application/problem+jsonsupport with content negotiation viaAcceptheader, completing RFC 9457 complianceErrorDetail.message→detail, addtitleandtypefields per RFC 9457 namingProblemDetailmodel for bare RFC 9457 responses (no envelope)CATEGORY_TITLESmap,category_title(),category_type_uri()helpersdocs/errors.md) with resolvable type URI anchorsErrorDetail, newProblemDetailinterface)Test Plan
Review Coverage
Pre-reviewed by 11 agents, 10 findings addressed:
_wants_problem_jsonwrapped defensively (exception handlers must never crash)_build_responsedefensive fallback for Pydantic ValidationErrorerror_code: int→ErrorCode(IntEnum) on both modelstype: str→NotBlankStron both models_check_retry_aftervalidator extracted (DRY)handle_validation_errorFollow-up
Refs #464
🤖 Generated with Claude Code