refactor: replace _ErrorResponseSpec NamedTuple with TypedDict#554
refactor: replace _ErrorResponseSpec NamedTuple with TypedDict#554
Conversation
Converts _ErrorResponseSpec from NamedTuple to TypedDict so entries are plain dicts with key-only access, eliminating positional indexing. Closes #509 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-reviewed by 9 agents, 2 findings addressed (em-dashes, ReadOnly annotations evaluated and dropped as unnecessary for private constant). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaced Unicode em dashes with ASCII double dashes and made small punctuation/text adjustments in error description strings and comments in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 refactors the internal representation of error response specifications by migrating from Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors _ErrorResponseSpec from a NamedTuple to a TypedDict to prevent positional indexing, and updates all access to use dictionary-style bracket notation. The changes also replace unicode em-dashes with ASCII dashes for consistency. The refactoring is correct and achieves its goal. However, I've noted a concern about the loss of runtime immutability for the _ERROR_RESPONSES constant, which now contains mutable dictionaries.
src/synthorg/api/openapi.py
Outdated
|
|
||
|
|
||
| class _ErrorResponseSpec(NamedTuple): | ||
| class _ErrorResponseSpec(TypedDict): |
There was a problem hiding this comment.
Changing from NamedTuple to TypedDict makes the elements of _ERROR_RESPONSES mutable dicts. This removes the runtime immutability guarantee that NamedTuple provided, creating a risk of accidental modification of this constant data. Since this data structure is intended to be a constant, it should ideally be immutable to prevent future bugs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
=======================================
Coverage 92.49% 92.49%
=======================================
Files 542 542
Lines 26655 26655
Branches 2544 2544
=======================================
Hits 24655 24655
Misses 1598 1598
Partials 402 402 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…shes Revert _ErrorResponseSpec from TypedDict to NamedTuple to preserve runtime immutability guarantees (TypedDict instances are mutable dicts). Restore attribute-style access (spec.status vs spec["status"]). Replace 3 remaining em-dash characters in comments/docstrings with ASCII dashes. Addresses review findings from 5 independent sources (conventions-enforcer, python-reviewer, type-design-analyzer, gemini-code-assist, code-reviewer). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/api/openapi.py (1)
373-401: 🛠️ Refactor suggestion | 🟠 MajorReturn updated paths instead of mutating the argument.
This helper mutates the passed
pathsmapping in place and the new docstring now makes that part of its contract. Prefer returning an updated copy and assigning it at the call site so the helper stays reusable and aligned with the repo’s immutability rule.As per coding guidelines: "Immutability: create new objects, never mutate existing ones."♻️ Refactor sketch
def _inject_operation_responses( paths: dict[str, Any], response_keys: list[str], status_for_key: dict[str, str], -) -> None: +) -> dict[str, Any]: """Inject error response refs into each operation in *paths*. - Mutates *paths* in place -- the caller is responsible for - passing a deep-copied schema. + Return a copy of *paths* with reusable error response refs injected. """ - for path, path_item in paths.items(): + updated_paths = copy.deepcopy(paths) + for path, path_item in updated_paths.items(): for method, operation in path_item.items(): if not isinstance(operation, dict) or "responses" not in operation: continue op_responses = operation["responses"] for key in response_keys: status_code = status_for_key[key] if not _should_inject(key, path, method, operation): continue if status_code == "400": # Only replace Litestar's auto-generated # ValidationException 400; preserve custom 400s. existing = op_responses.get("400") if existing is None or _is_litestar_validation_400( existing, ): op_responses["400"] = _response_ref(key) elif status_code not in op_responses: op_responses[status_code] = _response_ref(key) + return updated_paths- _inject_operation_responses( - result.get("paths", {}), - response_keys, - status_for_key, - ) + if "paths" in result: + result["paths"] = _inject_operation_responses( + result["paths"], + response_keys, + status_for_key, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/openapi.py` around lines 373 - 401, The helper _inject_operation_responses currently mutates the passed-in paths dict; change it to return a new updated mapping instead: create a shallow/deep copy of paths at the start (e.g., new_paths = copy.deepcopy(paths)), operate on new_paths (using the same logic that references _should_inject, _is_litestar_validation_400, and _response_ref) and return new_paths at the end, and update call sites to assign the result back instead of relying on in-place mutation so the function is immutable and side-effect free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/synthorg/api/openapi.py`:
- Around line 349-353: The comment and code that exclude "Conflict" for DELETE
(in the dict construction with keys "BadRequest", "NotFound", "Conflict") makes
a protocol-level claim ("DELETE excluded -- idempotent; conflicts are a
create/update concern.") which is incorrect; update the inline comment next to
the "Conflict" entry (and/or surrounding docstring) to state the
project-specific invariant that justifies excluding 409 for method == "delete"
(or remove the exclusion if no such invariant exists). Specifically, locate the
dict where "Conflict": is_write and method != "delete" is set and either (a)
replace the current justification with a clear project-level explanation naming
the invariant/assumption that prevents DELETE from returning 409, or (b) if no
invariant applies, remove the method != "delete" clause so DELETE can map to
"Conflict" like other write methods.
---
Outside diff comments:
In `@src/synthorg/api/openapi.py`:
- Around line 373-401: The helper _inject_operation_responses currently mutates
the passed-in paths dict; change it to return a new updated mapping instead:
create a shallow/deep copy of paths at the start (e.g., new_paths =
copy.deepcopy(paths)), operate on new_paths (using the same logic that
references _should_inject, _is_litestar_validation_400, and _response_ref) and
return new_paths at the end, and update call sites to assign the result back
instead of relying on in-place mutation so the function is immutable and
side-effect free.
🪄 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: 3b5cb8f8-4afe-428f-b359-16674e10ffbb
📒 Files selected for processing (1)
src/synthorg/api/openapi.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 Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
PEP 758 except syntax: useexcept A, B:(no parentheses) — ruff enforces this on Python 3.14
Line length: 88 characters (ruff)
Files:
src/synthorg/api/openapi.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Type hints: all public functions, mypy strict mode
Docstrings: Google style, required on public classes/functions (enforced by ruff D rules)
Immutability: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence)
Config vs runtime state: frozen Pydantic models for config/identity; 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
Models: Pydantic v2 (BaseModel,model_validator,computed_field,ConfigDict). Use@computed_fieldfor derived values instead of storing + validating redundant fields
UseNotBlankStr(fromcore.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators
Async concurrency: preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over barecreate_task
Functions: < 50 lines, files < 800 lines
Errors: handle explicitly, never silently swallow
Validate: at system boundaries (user input, external APIs, config files)
Files:
src/synthorg/api/openapi.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST have:from synthorg.observability import get_loggerthenlogger = get_logger(__name__)
Never useimport logging/logging.getLogger()/print()in application code
Variable name: alwayslogger(not_logger, notlog)
Event names: always use constants from the domain-specific module undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api,TOOL_INVOKE_STARTfromevents.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Structured kwargs: alwayslogger.info(EVENT, key=value)— neverlogger.info("msg %s", val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO
DEBUG for object creation, internal flow, entry/exit of key functions
API reference: auto-generated from docstrings via mkdocstrings + Griffe (AST-based, no imports); document all public APIs with Google-style docstrings
Observability: use structured logging via get_logger() and observability.events constants; correlation tracking for multi-agent workflows
Files:
src/synthorg/api/openapi.py
{src,tests}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Vendor-agnostic everywhere: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:
example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases
Files:
src/synthorg/api/openapi.py
src/synthorg/{api,auth,backup,budget,cli,communication,config,core,engine,hr,memory,persistence,observability,providers,security,settings,templates,tools}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Google style docstrings for all public classes/functions in the API subpackage and other modules; includes parameter, return, raises, and example sections
Files:
src/synthorg/api/openapi.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
REST API: RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail); use problem detail responses for all API errors
Files:
src/synthorg/api/openapi.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T11:08:24.271Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail); use problem detail responses for all API errors
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T11:08:24.271Z
Learning: Applies to src/synthorg/{api,auth,backup,budget,cli,communication,config,core,engine,hr,memory,persistence,observability,providers,security,settings,templates,tools}/**/*.py : Google style docstrings for all public classes/functions in the API subpackage and other modules; includes parameter, return, raises, and example sections
📚 Learning: 2026-03-18T11:08:24.271Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T11:08:24.271Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail); use problem detail responses for all API errors
Applied to files:
src/synthorg/api/openapi.py
📚 Learning: 2026-03-18T11:08:24.271Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T11:08:24.271Z
Learning: Applies to src/synthorg/{api,auth,backup,budget,cli,communication,config,core,engine,hr,memory,persistence,observability,providers,security,settings,templates,tools}/**/*.py : Google style docstrings for all public classes/functions in the API subpackage and other modules; includes parameter, return, raises, and example sections
Applied to files:
src/synthorg/api/openapi.py
🔇 Additional comments (1)
src/synthorg/api/openapi.py (1)
83-165: ASCII-only prose cleanup looks safe.These edits are documentation-only: they change emitted descriptions/comments without changing the response shape, content negotiation, or example payloads.
Also applies to: 202-202
Replace misleading "idempotent" justification with the actual project-specific invariant: DELETE endpoints are unconditional removals with no state preconditions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.3.5](v0.3.4...v0.3.5) (2026-03-18) ### Features * **api:** auto-wire backend services at startup ([#555](#555)) ([0e52c47](0e52c47)) ### Bug Fixes * **api:** resolve WebSocket 403 rejection ([#549](#549)) ([#556](#556)) ([60453d2](60453d2)) * **cli:** verify SLSA provenance via GitHub attestation API ([#548](#548)) ([91d4f79](91d4f79)), closes [#532](#532) ### Performance * **test:** speed up test suite -- reduce Hypothesis examples and eliminate real sleeps ([#557](#557)) ([d5f3a41](d5f3a41)) ### Refactoring * replace _ErrorResponseSpec NamedTuple with TypedDict ([#554](#554)) ([71cc6e1](71cc6e1)) ### Maintenance * **docker:** suppress pydantic v1 warning on Python 3.14 ([#552](#552)) ([cbe1f05](cbe1f05)), closes [#551](#551) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
_ErrorResponseSpecfromNamedTupletoTypedDictso entries are plain dicts with key-only access, eliminating positional indexingspec.field) to bracket style (spec["field"])\u2014) with ASCII dashes (--) in error description stringsCloses #509
Review coverage
Pre-reviewed by 9 agents: docs-consistency, code-reviewer, python-reviewer, type-design-analyzer, pr-test-analyzer, conventions-enforcer, logging-audit, resilience-audit, issue-resolution-verifier. 2 findings addressed (em-dashes fixed, ReadOnly annotations evaluated and dropped as unnecessary for private module-level constant).
Test plan
🤖 Generated with Claude Code