feat: extend approval, meeting, and budget API responses#834
feat: extend approval, meeting, and budget API responses#834
Conversation
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 enhances the API responses for approvals, meetings, and budget records by introducing computed, client-side-ready analytics and urgency indicators. These additions aim to provide richer data directly from the API, reducing the need for frontend computation and enabling more insightful dashboards and user interfaces. The changes span backend controllers, data models, and corresponding frontend type definitions, ensuring a consistent and robust data flow for these extended features. 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
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request introduces API response enrichment across several domains. The Meeting API now includes token_usage_by_participant, contribution_rank, and meeting_duration_seconds in MeetingRecord responses. The Budget API's /budget/records endpoint is enhanced with daily_summary and period_summary aggregations. Additionally, the Approval API now provides seconds_remaining and urgency_level fields in ApprovalItem responses, along with a new APPROVAL_GATE_REVIEW_TRANSITION_FAILED event. Feedback suggests improving the _to_approval_response function for consistency and testability by passing the current time as a parameter, and optimizing date formatting in the budget controller for better performance.
| def _to_approval_response(item: ApprovalItem) -> ApprovalResponse: | ||
| """Convert an ApprovalItem to an ApprovalResponse with urgency fields. | ||
|
|
||
| Args: | ||
| item: The domain-layer approval item. | ||
|
|
||
| Returns: | ||
| Response DTO with computed ``seconds_remaining`` and ``urgency_level``. | ||
| """ | ||
| now = datetime.now(UTC) | ||
| if item.expires_at is None: | ||
| seconds_remaining = None | ||
| urgency = UrgencyLevel.NO_EXPIRY | ||
| else: | ||
| seconds_remaining = max(0.0, (item.expires_at - now).total_seconds()) | ||
| if seconds_remaining < _URGENCY_CRITICAL_SECONDS: | ||
| urgency = UrgencyLevel.CRITICAL | ||
| elif seconds_remaining < _URGENCY_HIGH_SECONDS: | ||
| urgency = UrgencyLevel.HIGH | ||
| else: | ||
| urgency = UrgencyLevel.NORMAL | ||
| return ApprovalResponse( | ||
| **item.model_dump(), | ||
| seconds_remaining=seconds_remaining, | ||
| urgency_level=urgency, | ||
| ) |
There was a problem hiding this comment.
For consistency and testability, it's better to fetch the current time once and pass it into this function, rather than calling datetime.now(UTC) inside. This is especially important when processing a list of items, as it ensures all items are evaluated against the same timestamp.
I suggest modifying the function signature to accept now as a parameter. You will need to update the call sites. For example:
# In list_approvals
now = datetime.now(UTC)
enriched = tuple(_to_approval_response(i, now=now) for i in page)
# In other methods
return ApiResponse(data=_to_approval_response(item, now=datetime.now(UTC)))def _to_approval_response(item: ApprovalItem, *, now: datetime) -> ApprovalResponse:
"""Convert an ApprovalItem to an ApprovalResponse with urgency fields.
Args:
item: The domain-layer approval item.
now: The current time to compare against.
Returns:
Response DTO with computed ``seconds_remaining`` and ``urgency_level``.
"""
if item.expires_at is None:
seconds_remaining = None
urgency = UrgencyLevel.NO_EXPIRY
else:
seconds_remaining = max(0.0, (item.expires_at - now).total_seconds())
if seconds_remaining < _URGENCY_CRITICAL_SECONDS:
urgency = UrgencyLevel.CRITICAL
elif seconds_remaining < _URGENCY_HIGH_SECONDS:
urgency = UrgencyLevel.HIGH
else:
urgency = UrgencyLevel.NORMAL
return ApprovalResponse(
**item.model_dump(),
seconds_remaining=seconds_remaining,
urgency_level=urgency,
)|
|
||
| by_day: dict[str, list[CostRecord]] = defaultdict(list) | ||
| for r in records: | ||
| by_day[r.timestamp.strftime("%Y-%m-%d")].append(r) |
There was a problem hiding this comment.
For performance, it's generally better to use .date().isoformat() instead of strftime("%Y-%m-%d") for extracting the date part of a datetime object. isoformat() is typically faster.
| by_day[r.timestamp.strftime("%Y-%m-%d")].append(r) | |
| by_day[r.timestamp.date().isoformat()].append(r) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
+ Coverage 92.31% 92.33% +0.02%
==========================================
Files 584 584
Lines 30491 30576 +85
Branches 2934 2941 +7
==========================================
+ Hits 28148 28233 +85
Misses 1854 1854
Partials 489 489 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughAdds computed response enrichments, DTOs, controller helpers, docs, client types, observability events, and tests. Approval endpoints now return 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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 `@docs/design/communication.md`:
- Around line 420-428: The opening sentence incorrectly states that all
enrichment fields on MeetingRecord are derived from
MeetingMinutes.contributions; update the text to separate contribution-derived
analytics from duration by either narrowing the sentence to mention only the
contribution-based fields (`token_usage_by_participant`, `contribution_rank`)
come from `MeetingMinutes.contributions` and then add a second sentence
clarifying that `meeting_duration_seconds` is computed from `ended_at -
started_at` (or `null` when minutes are missing); ensure the identifiers
`MeetingRecord`, `MeetingMinutes.contributions`, `token_usage_by_participant`,
`contribution_rank`, `meeting_duration_seconds`, `ended_at`, and `started_at`
are used exactly as in the spec so readers can locate the data sources.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 116-125: The analytics block in _to_meeting_response emits
participant analytics whenever record.minutes exists and keys them by
MeetingContribution.agent_id, which can leak ID-keyed partial analytics for
non-completed meetings; change the logic to only compute and include
participant-name analytics and meeting_duration_seconds when record.status ==
MeetingStatus.COMPLETED, resolving each MeetingContribution.agent_id to the
participant display name (lookup via the meeting/participant records in the same
response path) before adding to usage, and omit or leave empty these analytics
fields for non-COMPLETED records to match the documented response contract.
In `@tests/unit/api/controllers/test_approvals.py`:
- Around line 571-621: Patch the clock used by the urgency calculation at the
start of the timing-sensitive tests (test_urgency_boundary_at_1hr and
test_urgency_boundary_at_4hrs) so the assertions are deterministic: mock
time.monotonic() to return a fixed value and mock asyncio.sleep() to a no-op
(using monkeypatch or pytest fixtures), then compute the
ApprovalItem.created_at/expires_at relative to that frozen clock before calling
approval_store.add and the GET; this ensures the urgency calculation (which
reads time.monotonic()) yields stable "high" and "normal" results for
ApprovalItem IDs "boundary-1h" and "boundary-4h".
In `@tests/unit/api/controllers/test_budget.py`:
- Around line 174-201: Update the test_summaries_from_all_records_not_page to
include a filtered-summary case: create multiple CostRecord entries including at
least one non-matching record (e.g., agent_id="bob") alongside matching ones
(agent_id="alice"), then call GET "/api/v1/budget/records" with a filter param
(agent_id="alice") and a pagination param (limit=1) and assert that body["data"]
length == 1 (page) but body["period_summary"]["record_count"] and
["total_cost_usd"] reflect only the matching records (sum/count for alice),
ensuring aggregation is applied after filtering by agent_id/task_id.
In `@web/src/api/endpoints/budget.ts`:
- Around line 22-45: The manual unwrap in listCostRecords bypasses response
validation and drops server error_detail; replace the custom body handling with
the shared unwrapping helper (use unwrapPaginated or unwrap that matches
paginated shape) to validate structure and throw errors that include
body.error_detail, or if keeping ApiRequestError, construct it using body.error
and body.error_detail; update listCostRecords to call
apiClient.get<CostRecordListResponseBody> and pass the response into
unwrapPaginated (or call unwrap and map to the paginated result) so
daily_summary and period_summary are validated and ApiRequestError includes
error_detail from the server.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82e53224-01e2-46a1-9ab0-5ae61a6994c4
📒 Files selected for processing (13)
docs/design/communication.mddocs/design/operations.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/observability/events/approval_gate.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.tsweb/src/api/types.ts
📜 Review details
🧰 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
Useexcept A, B:syntax without parentheses for exception handling (PEP 758) -- ruff enforces this on Python 3.14
Add type hints to all public functions and classes; mypy strict mode required
Use Google-style docstrings on all public classes and functions (enforced by ruff D rules)
Immutability enforcement: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries, BaseTool), use copy.deepcopy() at construction + MappingProxyType wrapping. For dict/list fields in frozen Pydantic models, rely on frozen=True and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Use structured concurrency over bare create_task
Keep functions under 50 lines, files under 800 lines
Line length: 88 characters (ruff enforced)
Explicitly handle all errors; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
tests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pytests/unit/api/controllers/test_approvals.pysrc/synthorg/observability/events/approval_gate.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark all tests with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow
Always run pytest with-n autofor parallelism via pytest-xdist; never run tests sequentially. Minimum coverage: 80% (enforced in CI). Timeout: 30 seconds per test globally (do not add per-file markers; non-default overrides like timeout(60) are allowed)
Vendor-agnostic naming 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/small aliases. Vendor names allowed only in: (1) docs/design/operations.md provider list, (2) .claude/ skill/agent files, (3) third-party import paths. Tests must use test-provider, test-small-001, etc.
Use Hypothesis for property-based testing (@given+@settingswith HYPOTHESIS_PROFILE env var: 'ci' for 50 examples (default), 'dev' for 1000 examples). Run dev profile: HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties. .hypothesis/ is gitignored
Never skip, dismiss, or ignore flaky tests -- always fix them fully. For timing-sensitive tests, mock time.monotonic() and asyncio.sleep() for determinism. For tasks blocking indefinitely until cancelled, use asyncio.Event().wait() instead of asyncio.sleep(large_number) -- it is cancellation-safe
Files:
tests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pytests/unit/api/controllers/test_approvals.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use vendor-agnostic naming: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, or tests. Use generic names: example-provider, example-large-001, example-medium-001, example-small-001
React 19 with shadcn/ui + Tailwind CSS 4. Use React hooks (auth, login lockout, WebSocket, polling, optimistic updates), Zustand stores, and lazy-loaded page components. Maintain component organization: ui/ (shadcn primitives), layout/ (app shell, sidebar, status bar), feature dirs for pages
Never skip, dismiss, or ignore flaky tests -- always fix them fully. Mock timing functions for determinism
Files:
web/src/api/endpoints/budget.tsweb/src/api/endpoints/approvals.tsweb/src/api/types.tsweb/src/api/endpoints/meetings.ts
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__)
Use logger.info(EVENT, key=value) with structured kwargs for logging; never use logger.info('msg %s', val) formatting
Never useimport logging/logging.getLogger()/print()in application code. Exception: observability/setup.py and observability/sinks.py may use stdlib logging and print() for bootstrap code only
Use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code. RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig. Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately. RetryExhaustedError signals all retries failed -- engine layer catches this to trigger fallback chains. Rate limiter respects RateLimitError.retry_after
Always read the relevant docs/design/ page before implementing any feature or planning any issue. DESIGN_SPEC.md is a pointer file. When a spec topic is referenced (e.g., 'the Agents page'), read the relevant docs/design/ page before coding
If implementation deviates from the design spec (better approach, scope evolved), alert the user and explain why. User decides whether to proceed or update spec. Do NOT silently diverge. When approved deviations occur, update relevant docs/design/ page to reflect new reality
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation is built with Zensical (config: mkdocs.yml). Docs structure: docs/design/ (10 pages), docs/architecture/, docs/roadmap/, docs/security.md, docs/licensing.md, docs/reference/, docs/rest-api.md + docs/_generated/api-reference.html (generated by scripts/export_openapi.py), docs/api/ (auto-generated via mkdocstrings + Griffe AST-based). Landing page: site/ (Astro with /get/ CLI install page, contact form, SEO)
Files:
docs/design/communication.mddocs/design/operations.md
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Design spec pages are the mandatory starting point for architecture, data models, and behavior. Always read the relevant docs/design/ page before implementing features. If implementation deviates from spec, alert the user with explanation. Update docs/design/ pages when deviations are approved
Files:
docs/design/communication.mddocs/design/operations.md
🧠 Learnings (8)
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
tests/unit/api/controllers/test_budget.pyweb/src/api/endpoints/budget.tsdocs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
web/src/api/endpoints/budget.tsdocs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/observability/events/approval_gate.pydocs/design/operations.md
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
docs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
🪛 markdownlint-cli2 (0.22.0)
docs/design/operations.md
[warning] 1008-1008: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (4)
tests/unit/api/controllers/test_meetings.py (1)
33-89: Analytics coverage is nicely targeted.The new fixtures exercise populated, empty, failed, and trigger paths, and the fixed 120-second delta keeps the duration assertion deterministic.
Also applies to: 123-137, 272-287, 322-439
web/src/api/types.ts (1)
32-32: The shared frontend types match the enriched payloads.This keeps approval urgency, meeting analytics, and budget summaries typed end-to-end instead of forcing downstream casts.
Also applies to: 279-282, 388-402, 765-769
src/synthorg/api/controllers/approvals.py (1)
62-123: Urgency enrichment is applied consistently across all approval endpoints.Using a single converter everywhere keeps the response shape uniform, and clamping
seconds_remainingat zero avoids negative values for expired items.Also applies to: 319-324, 398-424, 431-455, 462-523, 535-686
web/src/api/endpoints/approvals.ts (1)
12-34: The endpoint wrappers now expose the richer approval DTO cleanly.Callers can consume
seconds_remainingandurgency_levelwithout local re-typing.
| if record.minutes is not None: | ||
| for c in record.minutes.contributions: | ||
| usage[c.agent_id] = ( | ||
| usage.get(c.agent_id, 0) + c.input_tokens + c.output_tokens | ||
| ) | ||
| rank = tuple( | ||
| sorted(usage, key=usage.__getitem__, reverse=True), | ||
| ) | ||
| delta = record.minutes.ended_at - record.minutes.started_at | ||
| duration = delta.total_seconds() |
There was a problem hiding this comment.
Return participant names only for completed meetings.
_to_meeting_response() currently emits analytics whenever minutes exists and uses MeetingContribution.agent_id as the outward-facing key. The PR objective says these fields are participant-name analytics and meeting_duration_seconds is completed-only, so a failed/cancelled record with partial minutes would leak ID-keyed analytics that don't match the documented response contract. Gate this block on record.status == MeetingStatus.COMPLETED and resolve IDs to display names here, or update the API/docs/types/tests to explicitly contract on IDs instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/meetings.py` around lines 116 - 125, The
analytics block in _to_meeting_response emits participant analytics whenever
record.minutes exists and keys them by MeetingContribution.agent_id, which can
leak ID-keyed partial analytics for non-completed meetings; change the logic to
only compute and include participant-name analytics and meeting_duration_seconds
when record.status == MeetingStatus.COMPLETED, resolving each
MeetingContribution.agent_id to the participant display name (lookup via the
meeting/participant records in the same response path) before adding to usage,
and omit or leave empty these analytics fields for non-COMPLETED records to
match the documented response contract.
| async def test_urgency_boundary_at_1hr( | ||
| self, | ||
| test_client: TestClient[Any], | ||
| approval_store: ApprovalStore, | ||
| ) -> None: | ||
| now = datetime.now(UTC) | ||
| item = ApprovalItem( | ||
| id="boundary-1h", | ||
| action_type="code_merge", | ||
| title="Boundary test", | ||
| description="desc", | ||
| requested_by="agent-dev", | ||
| risk_level=ApprovalRiskLevel.MEDIUM, | ||
| created_at=now, | ||
| # 3605s in the future -- after processing the remaining | ||
| # time will still be >= 3600s, landing in HIGH (not critical). | ||
| expires_at=now + timedelta(seconds=3605), | ||
| ) | ||
| await approval_store.add(item) | ||
| resp = test_client.get( | ||
| f"{_BASE}/boundary-1h", | ||
| headers=_READ_HEADERS, | ||
| ) | ||
| data = resp.json()["data"] | ||
| assert data["urgency_level"] == "high" | ||
|
|
||
| async def test_urgency_boundary_at_4hrs( | ||
| self, | ||
| test_client: TestClient[Any], | ||
| approval_store: ApprovalStore, | ||
| ) -> None: | ||
| now = datetime.now(UTC) | ||
| item = ApprovalItem( | ||
| id="boundary-4h", | ||
| action_type="code_merge", | ||
| title="Boundary test", | ||
| description="desc", | ||
| requested_by="agent-dev", | ||
| risk_level=ApprovalRiskLevel.MEDIUM, | ||
| created_at=now, | ||
| # 14405s in the future -- after processing the remaining | ||
| # time will still be >= 14400s, landing in NORMAL (not high). | ||
| expires_at=now + timedelta(seconds=14405), | ||
| ) | ||
| await approval_store.add(item) | ||
| resp = test_client.get( | ||
| f"{_BASE}/boundary-4h", | ||
| headers=_READ_HEADERS, | ||
| ) | ||
| data = resp.json()["data"] | ||
| assert data["urgency_level"] == "normal" |
There was a problem hiding this comment.
Freeze time for the threshold boundaries.
The 5-second buffers at Line 587 and Line 613 still make these assertions dependent on runtime speed. Slow CI can flip the 1-hour case into critical or the 4-hour case into high, so please mock the clock the urgency calculation reads instead of relying on wall-clock slack.
As per coding guidelines, "For timing-sensitive tests, mock time.monotonic() and asyncio.sleep() for determinism."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/controllers/test_approvals.py` around lines 571 - 621, Patch
the clock used by the urgency calculation at the start of the timing-sensitive
tests (test_urgency_boundary_at_1hr and test_urgency_boundary_at_4hrs) so the
assertions are deterministic: mock time.monotonic() to return a fixed value and
mock asyncio.sleep() to a no-op (using monkeypatch or pytest fixtures), then
compute the ApprovalItem.created_at/expires_at relative to that frozen clock
before calling approval_store.add and the GET; this ensures the urgency
calculation (which reads time.monotonic()) yields stable "high" and "normal"
results for ApprovalItem IDs "boundary-1h" and "boundary-4h".
There was a problem hiding this comment.
Actionable comments posted: 3
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/controllers/approvals.py (1)
481-487:⚠️ Potential issue | 🟡 MinorRemove duplicate validation or derive the
max_lengthannotation fromQUERY_MAX_LENGTH.
action_typeis declared asAnnotated[str, Parameter(max_length=128)], so Litestar will reject overlong query values at the framework level before this handler executes. The manual validation on lines 503–509 and itsAPI_VALIDATION_FAILEDlog will not fire for HTTP request validation failures, making that instrumentation ineffective for the path you're trying to monitor. Additionally, the hardcoded128can drift fromQUERY_MAX_LENGTH.Either derive the annotation limit from the
QUERY_MAX_LENGTHconstant or remove the duplicate controller-level check if it only exists to catch HTTP path violations.Also applies to: 503-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/approvals.py` around lines 481 - 487, The controller duplicates validation for action_type in list_approvals: the Parameter(max_length=128) annotation already enforces length via Litestar so the manual check and API_VALIDATION_FAILED logging (the block around the manual validation in list_approvals) is redundant and uses a hardcoded 128. Fix by either (A) deriving the annotated max_length from QUERY_MAX_LENGTH (use Parameter(max_length=QUERY_MAX_LENGTH) for action_type) so framework and controller use the same limit, or (B) remove the manual length-check block and its API_VALIDATION_FAILED log inside list_approvals entirely if you only need framework-level validation; ensure any remaining instrumentation references QUERY_MAX_LENGTH rather than the hardcoded 128.
♻️ Duplicate comments (2)
web/src/api/endpoints/budget.ts (1)
36-47:⚠️ Potential issue | 🟠 MajorValidate the success payload before returning it.
This path only checks
success. Ifpagination,daily_summary, orperiod_summaryare missing or malformed, the function returnsundefinedfields and the UI fails later instead of raising here.🛠️ Minimal hardening
const response = await apiClient.get<CostRecordListResponseBody>('/budget/records', { params }) const body = response.data - if (!body?.success) { - throw new ApiRequestError(body?.error ?? 'Unknown API error', body?.error_detail ?? null) + if (!body || typeof body !== 'object') { + throw new ApiRequestError('Unexpected API response format') + } + if (!body.success) { + throw new ApiRequestError(body.error ?? 'Unknown API error', body.error_detail ?? null) + } + if ( + !Array.isArray(body.data) || + !body.pagination || + !Array.isArray(body.daily_summary) || + !body.period_summary + ) { + throw new ApiRequestError('Unexpected API response format') } return { data: body.data,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/endpoints/budget.ts` around lines 36 - 47, The function that calls apiClient.get<CostRecordListResponseBody>('/budget/records', { params }) currently only checks body?.success; extend validation to ensure required payload fields (body.data, body.pagination and its keys total/offset/limit, and body.daily_summary and body.period_summary) are present and of expected types before returning; if any are missing or malformed, throw an ApiRequestError with a clear message (include body?.error_detail if available) so callers don't receive undefined fields—update the handling around the CostRecordListResponseBody response and the return block to perform these checks.src/synthorg/api/controllers/meetings.py (1)
81-105:⚠️ Potential issue | 🟠 MajorAlign meeting analytics with the published contract.
The linked objective says these fields are participant-name analytics for completed meetings. This code still emits analytics for any record that has
minutesand usesMeetingContribution.agent_idas the public key, so failed/cancelled records with partial minutes would leak ID-keyed analytics that do not match the contract. Gate the computation onrecord.status == MeetingStatus.COMPLETED, and resolve IDs to the outward-facing participant names before serializing them. If IDs are intentional, please make that an explicit approved contract change across the spec/types/tests instead of silently diverging.As per coding guidelines, "If implementation deviates from the design spec (better approach, scope evolved), alert the user and explain why. User decides whether to proceed or update spec."
Also applies to: 124-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 81 - 105, The analytics currently emitted from MeetingResponse/meeting serialization are being produced for any record with minutes and keyed by MeetingContribution.agent_id; change the logic that computes token_usage_by_participant, contribution_rank, and meeting_duration_seconds to only run when the MeetingRecord.status equals MeetingStatus.COMPLETED, and map participant identifiers to their outward-facing participant names (resolve agent_id -> participant_name) before building token_usage_by_participant and contribution_rank; if using raw agent IDs is intentional, surface that as an explicit contract change (update spec/types/tests) rather than leaving the current behavior.
🤖 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/controllers/approvals.py`:
- Around line 80-97: Convert the derived fields in ApprovalResponse to computed
properties: remove seconds_remaining and urgency_level as regular Fields and
instead add an internal reference_time Field(exclude=True) to hold the passed
"now" value, then implement two `@computed_field` methods on ApprovalResponse that
compute seconds_remaining from expires_at and reference_time and compute
urgency_level from that seconds_remaining (reuse the existing UrgencyLevel
logic). Update the factory _to_approval_response() to set the internal
reference_time (now) on the ApprovalResponse instance rather than injecting
seconds_remaining/urgency_level so instantiation and serialization follow
pydantic computed_field semantics.
In `@src/synthorg/api/controllers/budget.py`:
- Around line 243-248: Change the domain-event log call from debug to info:
replace the logger.debug(...) invocation that emits API_BUDGET_RECORDS_LISTED
with logger.info(...) and keep the structured keyword args (agent_id, task_id,
record_count=len(records)) rather than printf-style formatting so the event is
recorded at info level consistent with the repo’s event-logging contract; update
the call where API_BUDGET_RECORDS_LISTED, logger, agent_id, task_id, and records
are referenced (in the budget listing code path).
In `@tests/unit/api/controllers/test_budget.py`:
- Around line 140-163: Add assertions to check token totals in the
period_summary for the populated and filtered summary tests: after retrieving
period = body["period_summary"] in test_period_summary_avg_cost (and the other
summary tests around lines 205-233), assert period["total_input_tokens"] ==
<expected_total_input> and period["total_output_tokens"] ==
<expected_total_output> (use the computed expected values from the recorded
CostRecord entries, e.g., input_tokens sum 100*3 and output_tokens sum 50*3 for
the three records in test_period_summary_avg_cost). Ensure you update the two
affected tests (the populated summary and the filtered-summary test) to include
these token-total assertions using the same period variable.
---
Outside diff comments:
In `@src/synthorg/api/controllers/approvals.py`:
- Around line 481-487: The controller duplicates validation for action_type in
list_approvals: the Parameter(max_length=128) annotation already enforces length
via Litestar so the manual check and API_VALIDATION_FAILED logging (the block
around the manual validation in list_approvals) is redundant and uses a
hardcoded 128. Fix by either (A) deriving the annotated max_length from
QUERY_MAX_LENGTH (use Parameter(max_length=QUERY_MAX_LENGTH) for action_type) so
framework and controller use the same limit, or (B) remove the manual
length-check block and its API_VALIDATION_FAILED log inside list_approvals
entirely if you only need framework-level validation; ensure any remaining
instrumentation references QUERY_MAX_LENGTH rather than the hardcoded 128.
---
Duplicate comments:
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 81-105: The analytics currently emitted from
MeetingResponse/meeting serialization are being produced for any record with
minutes and keyed by MeetingContribution.agent_id; change the logic that
computes token_usage_by_participant, contribution_rank, and
meeting_duration_seconds to only run when the MeetingRecord.status equals
MeetingStatus.COMPLETED, and map participant identifiers to their outward-facing
participant names (resolve agent_id -> participant_name) before building
token_usage_by_participant and contribution_rank; if using raw agent IDs is
intentional, surface that as an explicit contract change (update
spec/types/tests) rather than leaving the current behavior.
In `@web/src/api/endpoints/budget.ts`:
- Around line 36-47: The function that calls
apiClient.get<CostRecordListResponseBody>('/budget/records', { params })
currently only checks body?.success; extend validation to ensure required
payload fields (body.data, body.pagination and its keys total/offset/limit, and
body.daily_summary and body.period_summary) are present and of expected types
before returning; if any are missing or malformed, throw an ApiRequestError with
a clear message (include body?.error_detail if available) so callers don't
receive undefined fields—update the handling around the
CostRecordListResponseBody response and the return block to perform these
checks.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83556eaa-c7a4-4ce0-be68-0fd7f7009e41
📒 Files selected for processing (9)
docs/design/communication.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/observability/events/api.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pyweb/src/api/endpoints/budget.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). (4)
- GitHub Check: Build Backend
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation is built with Zensical (config: mkdocs.yml). Docs structure: docs/design/ (10 pages), docs/architecture/, docs/roadmap/, docs/security.md, docs/licensing.md, docs/reference/, docs/rest-api.md + docs/_generated/api-reference.html (generated by scripts/export_openapi.py), docs/api/ (auto-generated via mkdocstrings + Griffe AST-based). Landing page: site/ (Astro with /get/ CLI install page, contact form, SEO)
Files:
docs/design/communication.md
docs/design/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Design spec pages are the mandatory starting point for architecture, data models, and behavior. Always read the relevant docs/design/ page before implementing features. If implementation deviates from spec, alert the user with explanation. Update docs/design/ pages when deviations are approved
Files:
docs/design/communication.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations-- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax without parentheses for exception handling (PEP 758) -- ruff enforces this on Python 3.14
Add type hints to all public functions and classes; mypy strict mode required
Use Google-style docstrings on all public classes and functions (enforced by ruff D rules)
Immutability enforcement: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries, BaseTool), use copy.deepcopy() at construction + MappingProxyType wrapping. For dict/list fields in frozen Pydantic models, rely on frozen=True and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Use structured concurrency over bare create_task
Keep functions under 50 lines, files under 800 lines
Line length: 88 characters (ruff enforced)
Explicitly handle all errors; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pysrc/synthorg/api/controllers/approvals.pytests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/meetings.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__)
Use logger.info(EVENT, key=value) with structured kwargs for logging; never use logger.info('msg %s', val) formatting
Never useimport logging/logging.getLogger()/print()in application code. Exception: observability/setup.py and observability/sinks.py may use stdlib logging and print() for bootstrap code only
Use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code. RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig. Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately. RetryExhaustedError signals all retries failed -- engine layer catches this to trigger fallback chains. Rate limiter respects RateLimitError.retry_after
Always read the relevant docs/design/ page before implementing any feature or planning any issue. DESIGN_SPEC.md is a pointer file. When a spec topic is referenced (e.g., 'the Agents page'), read the relevant docs/design/ page before coding
If implementation deviates from the design spec (better approach, scope evolved), alert the user and explain why. User decides whether to proceed or update spec. Do NOT silently diverge. When approved deviations occur, update relevant docs/design/ page to reflect new reality
Files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/meetings.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Mark all tests with@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, or@pytest.mark.slow
Always run pytest with-n autofor parallelism via pytest-xdist; never run tests sequentially. Minimum coverage: 80% (enforced in CI). Timeout: 30 seconds per test globally (do not add per-file markers; non-default overrides like timeout(60) are allowed)
Vendor-agnostic naming 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/small aliases. Vendor names allowed only in: (1) docs/design/operations.md provider list, (2) .claude/ skill/agent files, (3) third-party import paths. Tests must use test-provider, test-small-001, etc.
Use Hypothesis for property-based testing (@given+@settingswith HYPOTHESIS_PROFILE env var: 'ci' for 50 examples (default), 'dev' for 1000 examples). Run dev profile: HYPOTHESIS_PROFILE=dev uv run python -m pytest tests/ -m unit -n auto -k properties. .hypothesis/ is gitignored
Never skip, dismiss, or ignore flaky tests -- always fix them fully. For timing-sensitive tests, mock time.monotonic() and asyncio.sleep() for determinism. For tasks blocking indefinitely until cancelled, use asyncio.Event().wait() instead of asyncio.sleep(large_number) -- it is cancellation-safe
Files:
tests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_meetings.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use vendor-agnostic naming: NEVER use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, or tests. Use generic names: example-provider, example-large-001, example-medium-001, example-small-001
React 19 with shadcn/ui + Tailwind CSS 4. Use React hooks (auth, login lockout, WebSocket, polling, optimistic updates), Zustand stores, and lazy-loaded page components. Maintain component organization: ui/ (shadcn primitives), layout/ (app shell, sidebar, status bar), feature dirs for pages
Never skip, dismiss, or ignore flaky tests -- always fix them fully. Mock timing functions for determinism
Files:
web/src/api/endpoints/budget.ts
🧠 Learnings (18)
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-24T22:47:39.223Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T22:47:39.223Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under ai_company.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.) — import directly
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
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`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
src/synthorg/api/controllers/budget.pytests/unit/api/controllers/test_budget.pyweb/src/api/endpoints/budget.ts
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
src/synthorg/api/controllers/budget.pytests/unit/api/controllers/test_budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
src/synthorg/api/controllers/budget.pyweb/src/api/endpoints/budget.ts
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/approvals.pytests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
🔇 Additional comments (1)
src/synthorg/api/controllers/approvals.py (1)
390-471: Nice consolidation of the write-side approval flow.
_get_approval_or_404()and_save_decision_and_notify()remove duplicate not-found/conflict handling from both mutation endpoints while keeping thesave_if_pendingguard centralized.
| class ApprovalResponse(ApprovalItem): | ||
| """Approval item enriched with computed urgency fields. | ||
|
|
||
| Attributes: | ||
| seconds_remaining: Seconds until expiry, clamped to 0.0 for | ||
| expired items (``None`` if no TTL). | ||
| urgency_level: Urgency classification based on time remaining. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(frozen=True) | ||
|
|
||
| seconds_remaining: float | None = Field( | ||
| ge=0.0, | ||
| description="Seconds until expiry (null if no TTL set)", | ||
| ) | ||
| urgency_level: UrgencyLevel = Field( | ||
| description="Urgency classification based on remaining time", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Pydantic 2.12.5, what is the recommended pattern for exposing derived response fields via @computed_field while keeping any reference-time input excluded from the serialized schema?
💡 Result:
In Pydantic 2.12.5, the recommended pattern for exposing derived response fields via @computed_field while excluding reference-time input fields (e.g., temporary or sensitive inputs used only for computation) from the serialized schema is to use a dedicated response model. Define input fields normally or with Field(exclude=True) if they should never appear in output. Add @computed_field properties for derived fields, which appear in serialization schema (model_json_schema(mode='serialization')) and model_dump output by default. To exclude specific inputs from serialization at runtime (e.g., for API responses), use model_dump(round_trip=True), which excludes all computed fields' dependencies and computed fields themselves, simulating input-only output. For exposing only derived fields alongside select inputs, use model_dump(exclude={'input_field': True}). In 2.12+, exclude_computed_fields=True in model_dump excludes all computed fields while keeping inputs. Example: from pydantic import BaseModel, computed_field, Field class MyModel(BaseModel): input_a: str # reference-time input input_b: int = Field(exclude=True) # always excluded @computed_field @property def derived(self) -> str: return f"Derived from {self.input_a} and {self.input_b}" # For response: exposes derived, excludes inputs if using round_trip or exclude response = MyModel(input_a='foo', input_b=42) print(response.model_dump(round_trip=True)) # {'input_a': 'foo'} excludes derived print(response.model_dump) # {'input_a': 'foo', 'input_b': 42, 'derived': 'Derived from foo and 42'} print(response.model_dump(exclude_computed_fields=True)) # {'input_a': 'foo', 'input_b': 42} excludes derived (2.12+) For API/FastAPI responses, use the model with mode='serialization' implicitly via response_model, where computed fields appear as readOnly in OpenAPI schema, and inputs can be excluded via Field(exclude=True) or dump options. Use separate input/response models for clearest separation: input model without computed fields, response with computed fields referencing inputs post-validation.
Citations:
- 1: https://docs.pydantic.dev/changelog/
- 2: https://docs.pydantic.dev/latest/changelog/
- 3: Allow computed fields to be excluded from model export pydantic/pydantic#5827
- 4: Exclude a computed field pydantic/pydantic#7114
- 5: https://pypi.org/project/pydantic/
- 6: https://docs.pydantic.dev/2.2/usage/computed_fields/
- 7: https://stackoverflow.com/questions/78023861/how-to-include-a-computed-field-decorated-as-property-in-pydantic-v2-schema-f
- 8: https://docs.pydantic.dev/2.11/concepts/serialization/
🏁 Script executed:
cat -n src/synthorg/api/controllers/approvals.py | head -150Repository: Aureliolo/synthorg
Length of output: 5640
🏁 Script executed:
rg "class ApprovalItem" --type py -A 20Repository: Aureliolo/synthorg
Length of output: 1605
🏁 Script executed:
rg "ApprovalResponse" --type py -B 2 -A 2Repository: Aureliolo/synthorg
Length of output: 2980
🏁 Script executed:
rg "_to_approval_response|ApprovalResponse" --type py -B 3 -A 8 | head -100Repository: Aureliolo/synthorg
Length of output: 6992
Refactor urgency fields to use @computed_field.
seconds_remaining and urgency_level are derived from expires_at and the reference time. Currently stored as regular fields and injected via _to_approval_response(), they should instead be modeled as computed properties. Store the reference time (the now parameter) as an internal excluded field (Field(exclude=True)) and expose the derived values via @computed_field decorators. This prevents inconsistent instantiation and aligns with the guideline: "use @computed_field for derived values instead of storing redundant fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/approvals.py` around lines 80 - 97, Convert the
derived fields in ApprovalResponse to computed properties: remove
seconds_remaining and urgency_level as regular Fields and instead add an
internal reference_time Field(exclude=True) to hold the passed "now" value, then
implement two `@computed_field` methods on ApprovalResponse that compute
seconds_remaining from expires_at and reference_time and compute urgency_level
from that seconds_remaining (reuse the existing UrgencyLevel logic). Update the
factory _to_approval_response() to set the internal reference_time (now) on the
ApprovalResponse instance rather than injecting seconds_remaining/urgency_level
so instantiation and serialization follow pydantic computed_field semantics.
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 `@src/synthorg/api/controllers/approvals.py`:
- Line 457: The call to _publish_approval_event currently passes the input
variable updated; change it to pass the persisted object saved so the published
event reflects the committed state. Locate the call to
_publish_approval_event(request, ws_event, updated) in approvals.py and replace
the third argument with saved (i.e., _publish_approval_event(request, ws_event,
saved)); ensure saved is the variable returned from the persistence step and is
in scope where the call occurs.
- Around line 504-511: Remove the redundant manual length check that inspects
action_type against QUERY_MAX_LENGTH (the if block that logs
API_VALIDATION_FAILED and raises ApiValidationError) because Litestar's
Parameter(max_length=...) rejects oversized values before the handler runs;
eliminate the equivalent duplicated check in the meetings handler as well.
Instead, rely on Litestar validation and, if you need observability, move
logging into a Litestar exception handler for validation errors rather than
keeping these unreachable manual checks.
In `@src/synthorg/api/controllers/budget.py`:
- Around line 145-187: The _build_summaries function currently iterates records
multiple times; change it to a single-pass aggregation by iterating records once
and updating per-day accumulators in by_day (store running totals:
total_cost_usd using math.fsum accumulator or add to a float,
total_input_tokens, total_output_tokens, record_count) keyed by
r.timestamp.date().isoformat(), while also updating period accumulators
(period_total_cost_usd, period_input_tokens, period_output_tokens,
period_record_count); after the loop, construct DailySummary instances from the
sorted by_day accumulators and build PeriodSummary from the period accumulators,
preserving types and using math.fsum for float accuracy where needed to replace
the separate sum() and math.fsum() passes.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 175-182: The manual length check for meeting_type is redundant
because Parameter(max_length=QUERY_MAX_LENGTH) already enforces this; remove the
if block that logs API_VALIDATION_FAILED and raises ApiValidationError (the code
referencing meeting_type, QUERY_MAX_LENGTH, API_VALIDATION_FAILED, and
ApiValidationError) so the handler relies on Litestar's validation. If you want
defense-in-depth instead, remove the in-handler check and add a Litestar
exception handler for request validation errors that logs API_VALIDATION_FAILED
with field="meeting_type" and the max length, rather than keeping the dead-code
check inside the endpoint.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c65633e-cda7-4118-8d32-24e421300b54
📒 Files selected for processing (3)
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.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 Sandbox
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations-- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax without parentheses for exception handling (PEP 758) -- ruff enforces this on Python 3.14
Add type hints to all public functions and classes; mypy strict mode required
Use Google-style docstrings on all public classes and functions (enforced by ruff D rules)
Immutability enforcement: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries, BaseTool), use copy.deepcopy() at construction + MappingProxyType wrapping. For dict/list fields in frozen Pydantic models, rely on frozen=True and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Use structured concurrency over bare create_task
Keep functions under 50 lines, files under 800 lines
Line length: 88 characters (ruff enforced)
Explicitly handle all errors; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.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__)
Use logger.info(EVENT, key=value) with structured kwargs for logging; never use logger.info('msg %s', val) formatting
Never useimport logging/logging.getLogger()/print()in application code. Exception: observability/setup.py and observability/sinks.py may use stdlib logging and print() for bootstrap code only
Use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code. RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig. Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately. RetryExhaustedError signals all retries failed -- engine layer catches this to trigger fallback chains. Rate limiter respects RateLimitError.retry_after
Always read the relevant docs/design/ page before implementing any feature or planning any issue. DESIGN_SPEC.md is a pointer file. When a spec topic is referenced (e.g., 'the Agents page'), read the relevant docs/design/ page before coding
If implementation deviates from the design spec (better approach, scope evolved), alert the user and explain why. User decides whether to proceed or update spec. Do NOT silently diverge. When approved deviations occur, update relevant docs/design/ page to reflect new reality
Files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.py
🧠 Learnings (16)
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always log with structured kwargs: `logger.info(EVENT, key=value)` — never use old-style formatting `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-24T22:47:39.223Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T22:47:39.223Z
Learning: Applies to src/synthorg/**/*.py : Use logger.info(EVENT, key=value) with structured kwargs for logging; never use logger.info('msg %s', val) formatting
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-24T22:47:39.223Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T22:47:39.223Z
Learning: Applies to **/*.py : Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use computed_field for derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
🔇 Additional comments (17)
src/synthorg/api/controllers/budget.py (6)
243-248: LogAPI_BUDGET_RECORDS_LISTEDat info level.Domain events should be logged at
infolevel to appear in the normal observability stream. Usingdebugwill drop this event in production.📋 Minimal fix
- logger.debug( + logger.info( API_BUDGET_RECORDS_LISTED, agent_id=agent_id, task_id=task_id, record_count=len(records), )As per coding guidelines, "Use logger.info(EVENT, key=value) with structured kwargs for logging".
3-25: LGTM — imports are well-organized.The new imports align with the added functionality:
math.fsumfor precise floating-point aggregation,defaultdictfor grouping, and the appropriate Pydantic/DTO components. UsingQUERY_MAX_LENGTHfrom the shared path_params module and the domain event constantAPI_BUDGET_RECORDS_LISTEDfollows project conventions.
44-68: LGTM —DailySummarymodel is well-structured.Frozen Pydantic model with proper field constraints (
ge=0,ge=0.0),NotBlankStrfor the date identifier, and a complete Google-style docstring.
70-101: LGTM —PeriodSummarycorrectly uses@computed_fieldfor derived value.The
avg_cost_usdcomputed field properly handles the division-by-zero case (returning0.0whenrecord_count == 0). This follows the coding guideline to use@computed_fieldfor derived values instead of storing redundant fields.
103-143: LGTM —CostRecordListResponseenforces error/error_detail consistency.The
model_validatorensures both fields are set together or both areNone, andsuccessis correctly derived via@computed_field. The validation logic is bidirectional (lines 130-135), catching both mismatched states.
213-255: LGTM — endpoint correctly computes summaries across all matching records.The implementation properly:
- Fetches all filtered records via
get_records(no pagination at service layer)- Computes summaries from the full result set before pagination
- Returns paginated
dataalongside completedaily_summaryandperiod_summaryThis matches the PR objective that summaries reflect all matching records, not just the current page.
src/synthorg/api/controllers/meetings.py (5)
124-133: Analytics still gated onminutespresence rather thanCOMPLETEDstatus.The previous review flagged that analytics should only be emitted for completed meetings. The current code computes
token_usage_by_participant,contribution_rank, andmeeting_duration_secondswheneverrecord.minutes is not None, which could expose partial analytics for failed or cancelled meetings that have minutes populated.
19-22: LGTM!Event constants correctly imported from the domain-specific module
synthorg.observability.events.api.
81-106: LGTM!The
MeetingResponseDTO is well-structured with proper type annotations, immutability viafrozen=True, and thege=0.0constraint correctly ensures non-negative duration values.
196-226: LGTM!The
get_meetingendpoint correctly returns the enrichedMeetingResponsewith consistent typing and documentation.
252-258: LGTM!The trigger endpoint correctly enriches results and logs with structured kwargs using the appropriate event constant.
src/synthorg/api/controllers/approvals.py (6)
80-97: Acknowledge past review: refactor to@computed_fieldstill applicable.The previous review correctly identified that
seconds_remainingandurgency_levelare derived values that should use@computed_fieldper the coding guidelines. The current implementation stores them as regular fields and computes them externally in_to_approval_response().
1-65: LGTM!Imports and module setup follow the coding guidelines: uses
get_logger(__name__), imports event constants from domain-specific modules, and correctly avoidsfrom __future__ import annotationsper Python 3.14/PEP 649 requirements.
100-129: LGTM!The urgency computation logic correctly implements the spec: clamps
seconds_remainingto>= 0.0, handlesNoneexpiry withNO_EXPIRY, and applies the threshold mapping (critical< 1hr,high< 4hr,normal>= 4hr).
390-416: Good DRY improvement.The
_get_approval_or_404helper consolidates the repeated pattern of fetching an approval and raisingNotFoundErrorwith proper logging. Clean implementation with appropriate type hints and docstring.
524-544: LGTM!Clean refactoring: uses
_get_approval_or_404helper, properly enriches response with urgency fields via_to_approval_response, and has updated docstring reflecting the new return type.
614-724: LGTM!Both
approveandrejectendpoints are cleanly refactored to use the new_get_approval_or_404and_save_decision_and_notifyhelpers, eliminating code duplication while preserving proper error handling semantics (NotFoundError,ConflictError). The consistent use of a singlenowtimestamp for both the decision and the response ensures temporal consistency.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/synthorg/api/controllers/approvals.py (2)
486-487: 🧹 Nitpick | 🔵 TrivialVerify whether the extra
action_typelength check is dead code.The handler already declares
Parameter(max_length=QUERY_MAX_LENGTH). If Litestar still enforces that during request binding, thelen(action_type)branch never runs and this custom logging path cannot fire. In that case, remove the branch and log validation failures from the framework exception handler instead.In Litestar, are query parameters declared with `Parameter(max_length=...)` validated before the route handler executes?Also applies to: 504-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/approvals.py` around lines 486 - 487, The runtime len(action_type) check appears redundant because the parameter is declared as Annotated[str, Parameter(max_length=QUERY_MAX_LENGTH)], so first verify Litestar performs request-time validation for Parameter.max_length (query binding) and if it does, remove the len(action_type) branch and its custom log/validation path from the handler (the parameter named action_type) and rely on the framework's exception handler to log validation failures; if you intentionally need runtime guarding instead, change the signature to allow unvalidated input (e.g., Optional[str] without Parameter(max_length=...)) so the manual len(...) branch is meaningful. Apply the same change to the other similar block referenced (the action_type handling around lines 504-511).
80-129: 🛠️ Refactor suggestion | 🟠 MajorExpose urgency fields via
@computed_field.
seconds_remainingandurgency_levelare derived fromexpires_atplus a reference timestamp, butApprovalResponsemodels them as ordinary inputs and_to_approval_response()injects them manually. That makes inconsistent DTO instances possible and loses the response-only semantics the repo expects; keep the reference time excluded/private and derive both fields with@computed_fieldinstead.In Pydantic v2, what is the recommended pattern for exposing derived response fields via `@computed_field` while keeping the reference-time input excluded from the serialized schema?As per coding guidelines, "Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use
@computed_fieldfor derived values instead of storing redundant fields."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/approvals.py` around lines 80 - 129, Replace the explicit seconds_remaining and urgency_level fields on ApprovalResponse with `@computed_field` properties that derive their values from expires_at and a private timestamp; add a PrivateAttr named _now: datetime | None = PrivateAttr(None) to ApprovalResponse, implement computed_field methods seconds_remaining(self) -> float | None and urgency_level(self) -> UrgencyLevel that use self.expires_at and self._now, keep model_config frozen=True, and update _to_approval_response to build the ApprovalResponse from item.model_dump(), set the private _now on the instance (e.g. object.__setattr__(resp, "_now", now)), then return resp so the derived fields are computed and the reference time is excluded from the schema/serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/synthorg/api/controllers/approvals.py`:
- Around line 486-487: The runtime len(action_type) check appears redundant
because the parameter is declared as Annotated[str,
Parameter(max_length=QUERY_MAX_LENGTH)], so first verify Litestar performs
request-time validation for Parameter.max_length (query binding) and if it does,
remove the len(action_type) branch and its custom log/validation path from the
handler (the parameter named action_type) and rely on the framework's exception
handler to log validation failures; if you intentionally need runtime guarding
instead, change the signature to allow unvalidated input (e.g., Optional[str]
without Parameter(max_length=...)) so the manual len(...) branch is meaningful.
Apply the same change to the other similar block referenced (the action_type
handling around lines 504-511).
- Around line 80-129: Replace the explicit seconds_remaining and urgency_level
fields on ApprovalResponse with `@computed_field` properties that derive their
values from expires_at and a private timestamp; add a PrivateAttr named _now:
datetime | None = PrivateAttr(None) to ApprovalResponse, implement
computed_field methods seconds_remaining(self) -> float | None and
urgency_level(self) -> UrgencyLevel that use self.expires_at and self._now, keep
model_config frozen=True, and update _to_approval_response to build the
ApprovalResponse from item.model_dump(), set the private _now on the instance
(e.g. object.__setattr__(resp, "_now", now)), then return resp so the derived
fields are computed and the reference time is excluded from the
schema/serialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8297a44-e2ec-4e20-a73d-7454f9b3fc74
📒 Files selected for processing (1)
src/synthorg/api/controllers/approvals.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). (4)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations-- Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:syntax without parentheses for exception handling (PEP 758) -- ruff enforces this on Python 3.14
Add type hints to all public functions and classes; mypy strict mode required
Use Google-style docstrings on all public classes and functions (enforced by ruff D rules)
Immutability enforcement: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries, BaseTool), use copy.deepcopy() at construction + MappingProxyType wrapping. For dict/list fields in frozen Pydantic models, rely on frozen=True and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use@computed_fieldfor derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code (multiple tool invocations, parallel agent calls). Use structured concurrency over bare create_task
Keep functions under 50 lines, files under 800 lines
Line length: 88 characters (ruff enforced)
Explicitly handle all errors; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
src/synthorg/api/controllers/approvals.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__)
Use logger.info(EVENT, key=value) with structured kwargs for logging; never use logger.info('msg %s', val) formatting
Never useimport logging/logging.getLogger()/print()in application code. Exception: observability/setup.py and observability/sinks.py may use stdlib logging and print() for bootstrap code only
Use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code. RetryConfig and RateLimiterConfig are set per-provider in ProviderConfig. Retryable errors (is_retryable=True): RateLimitError, ProviderTimeoutError, ProviderConnectionError, ProviderInternalError. Non-retryable errors raise immediately. RetryExhaustedError signals all retries failed -- engine layer catches this to trigger fallback chains. Rate limiter respects RateLimitError.retry_after
Always read the relevant docs/design/ page before implementing any feature or planning any issue. DESIGN_SPEC.md is a pointer file. When a spec topic is referenced (e.g., 'the Agents page'), read the relevant docs/design/ page before coding
If implementation deviates from the design spec (better approach, scope evolved), alert the user and explain why. User decides whether to proceed or update spec. Do NOT silently diverge. When approved deviations occur, update relevant docs/design/ page to reflect new reality
Files:
src/synthorg/api/controllers/approvals.py
🧠 Learnings (10)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-24T22:47:39.223Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T22:47:39.223Z
Learning: Applies to **/*.py : Use Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict); use computed_field for derived values instead of storing redundant fields; use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-24T22:47:39.223Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T22:47:39.223Z
Learning: Applies to **/*.py : Validate at system boundaries: user input, external APIs, config files
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/controllers/approvals.py
🔇 Additional comments (3)
src/synthorg/api/controllers/approvals.py (3)
324-331: Good split between resume and review failure events.Switching this warning to
APPROVAL_GATE_REVIEW_TRANSITION_FAILEDmakes the log taxonomy match the actual code path.
390-471: Nice consolidation of the decision-save path.Pulling lookup/conflict handling, event publication, logging, and resume signaling into shared helpers removes approve/reject drift and keeps the WebSocket event tied to the persisted approval object.
490-490: Good call routing all five endpoints through one mapper.Switching every approval handler to
ApprovalResponseand_to_approval_response()keeps the payload shape and urgency rules consistent across list/detail/create/approve/reject.Also applies to: 529-529, 552-552, 625-625, 681-681, 520-522, 543-545, 612-613, 668-668, 724-724
865502d to
2bb1958
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
web/src/api/endpoints/budget.ts (1)
36-47:⚠️ Potential issue | 🟠 MajorValidate the success payload before building
CostRecordListResult.This branch only checks
success. A malformedsuccess: truebody can still pushundefinedpagination,daily_summary, orperiod_summaryinto callers, which is exactly what the shared unwrap helpers protect against.🛠️ Minimal hardening
const body = response.data - if (!body?.success) { + if (!body || typeof body !== 'object') { + throw new ApiRequestError('Unknown API error') + } + if (!body.success) { throw new ApiRequestError(body.error ?? 'Unknown API error', body.error_detail ?? null) } + if ( + !Array.isArray(body.data) || + !body.pagination || + !Array.isArray(body.daily_summary) || + !body.period_summary + ) { + throw new ApiRequestError('Unexpected API response format') + } return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/endpoints/budget.ts` around lines 36 - 47, The current success-path only checks body.success but can still return a malformed CostRecordListResult with undefined fields; when body?.success is true, validate that body.data, body.pagination (and its total/offset/limit), body.daily_summary, and body.period_summary are present and well-formed before returning, and if any are missing throw an ApiRequestError (reuse the existing ApiRequestError thrown above). Prefer using the shared unwrap helpers used elsewhere (instead of ad-hoc checks) to validate the response shape, referencing the response variable and body object in budget.ts and ensure the returned object builds from the validated fields only.src/synthorg/api/controllers/approvals.py (1)
504-511: 🧹 Nitpick | 🔵 TrivialRedundant validation: Litestar
Parameter(max_length=...)already enforces this.The
Parameter(max_length=QUERY_MAX_LENGTH)at lines 486-487 rejects oversized values during request deserialization, before the handler executes. This manual check is unreachable.If observability for validation failures is needed, use a Litestar exception handler instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/approvals.py` around lines 504 - 511, The manual length check in the handler that inspects action_type against QUERY_MAX_LENGTH (the if-block that logs API_VALIDATION_FAILED and raises ApiValidationError) is redundant because Parameter(max_length=...) already enforces this during deserialization; remove that unreachable if-statement (the logger.warning and raise ApiValidationError lines) referencing action_type/QUERY_MAX_LENGTH/API_VALIDATION_FAILED so the handler no longer performs duplicate validation; if you need observability for such rejections instead, implement a Litestar exception handler for the deserialization/validation exceptions rather than keeping this in the handler.src/synthorg/api/controllers/meetings.py (1)
175-182: 🧹 Nitpick | 🔵 TrivialRedundant validation: Litestar
Parameter(max_length=...)already enforces this.The
Parameter(max_length=QUERY_MAX_LENGTH)at line 160-161 rejects oversized values during request deserialization, before the handler executes. This manual check is unreachable under normal operation.If observability for validation failures is desired, consider a Litestar exception handler instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 175 - 182, The manual length check for meeting_type is redundant because Litestar's Parameter(max_length=QUERY_MAX_LENGTH) already enforces this during deserialization; remove the conditional block that checks len(meeting_type) > QUERY_MAX_LENGTH and the associated logger.warning/ApiValidationError raise (references: meeting_type, QUERY_MAX_LENGTH, API_VALIDATION_FAILED, ApiValidationError, logger.warning) and, if you need observability for such validation failures, add or reuse a Litestar exception handler for request validation errors rather than keeping this unreachable in-handler check.src/synthorg/api/controllers/budget.py (1)
243-248:⚠️ Potential issue | 🟡 MinorLog
API_BUDGET_RECORDS_LISTEDat info level.This is a domain event;
debuglevel will drop it from the standard observability stream. Uselogger.info(...)to match the repo's event-logging contract.📋 Proposed fix
- logger.debug( + logger.info( API_BUDGET_RECORDS_LISTED, agent_id=agent_id, task_id=task_id, record_count=len(records), )As per coding guidelines, "Use structured logging: always
logger.info(EVENT, key=value)— never use logger.info('msg %s', val) formatting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/budget.py` around lines 243 - 248, The domain event API_BUDGET_RECORDS_LISTED is currently logged at debug level; change the call in the budget controller to logger.info(...) so the event is emitted on the standard observability stream, keeping the structured-logging form (pass API_BUDGET_RECORDS_LISTED, agent_id=agent_id, task_id=task_id, record_count=len(records)) rather than message-formatting; update the single call around where API_BUDGET_RECORDS_LISTED is used so the fields remain identical.tests/unit/api/controllers/test_budget.py (2)
205-233:⚠️ Potential issue | 🟡 MinorAdd token total assertions to
test_summaries_respect_agent_filter.For consistency with the
PeriodSummarycontract, include assertions fortotal_input_tokensandtotal_output_tokens. With 2 matching alice records (100 input, 50 output each), expected totals are 200 and 100.🧪 Proposed fix
# Page has 1 record, summaries cover 2 (alice only, not bob) assert len(body["data"]) == 1 assert body["period_summary"]["record_count"] == 2 assert body["period_summary"]["total_cost_usd"] == pytest.approx(0.20) + assert body["period_summary"]["total_input_tokens"] == 200 + assert body["period_summary"]["total_output_tokens"] == 100🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/controllers/test_budget.py` around lines 205 - 233, Update the test_summaries_respect_agent_filter async test to also assert the PeriodSummary token totals: after existing assertions on record_count and total_cost_usd, add checks that body["period_summary"]["total_input_tokens"] equals 200 and body["period_summary"]["total_output_tokens"] equals 100 (two alice records of 100 input and 50 output each) so the PeriodSummary contract is fully validated.
140-163:⚠️ Potential issue | 🟡 MinorAdd token total assertions to
test_period_summary_avg_cost.The test verifies
record_count,total_cost_usd, andavg_cost_usdbut does not assert the token totals. Given that each record hasinput_tokens=100andoutput_tokens=50, the expected totals are 300 and 150 respectively.🧪 Proposed fix to add token assertions
period = body["period_summary"] assert period["record_count"] == 3 assert period["total_cost_usd"] == pytest.approx(0.60) + assert period["total_input_tokens"] == 300 + assert period["total_output_tokens"] == 150 assert period["avg_cost_usd"] == pytest.approx(0.20)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/controllers/test_budget.py` around lines 140 - 163, Add assertions for input and output token totals to the test_period_summary_avg_cost test: after obtaining period = body["period_summary"], assert period["total_input_tokens"] == 300 and assert period["total_output_tokens"] == 150 (these totals come from three records with input_tokens=100 and output_tokens=50). Update the test_period_summary_avg_cost function to include these two assertions so the period summary token aggregates are validated alongside record_count, total_cost_usd, and avg_cost_usd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/communication.md`:
- Around line 429-432: The documentation omits the non-negative constraint on
meeting_duration_seconds and the controller returns delta.total_seconds()
without clamping; instead update the docs to state that meeting_duration_seconds
is non-negative (ge=0.0) and that when ended_at < started_at the API will clamp
the duration to 0.0, and also modify the controller logic that currently returns
delta.total_seconds() (the code that computes the duration) to clamp negative
values to 0.0 before returning so the behavior matches the Pydantic field
meeting_duration_seconds.
In `@tests/unit/api/controllers/test_approvals.py`:
- Around line 451-505: The three near-duplicate tests
(test_urgency_critical_under_1hr, test_urgency_high_under_4hrs,
test_urgency_normal_over_4hrs) should be collapsed into a pytest parametrized
test that drives TTL/frozen expiry and expected urgency/seconds_remaining
ranges; update the test to use `@pytest.mark.parametrize` with tuples like
(approval_id, ttl_seconds, expected_urgency, min_seconds, max_seconds) and call
the existing helper _seed_item and the same request pattern
(f"{_BASE}/{approval_id}" with _READ_HEADERS) inside one test function,
asserting data["urgency_level"] and the seconds_remaining range accordingly;
apply the same refactor to the other duplicated block referenced (lines
~572-630) so thresholds live in one table and future changes are centralized.
---
Duplicate comments:
In `@src/synthorg/api/controllers/approvals.py`:
- Around line 504-511: The manual length check in the handler that inspects
action_type against QUERY_MAX_LENGTH (the if-block that logs
API_VALIDATION_FAILED and raises ApiValidationError) is redundant because
Parameter(max_length=...) already enforces this during deserialization; remove
that unreachable if-statement (the logger.warning and raise ApiValidationError
lines) referencing action_type/QUERY_MAX_LENGTH/API_VALIDATION_FAILED so the
handler no longer performs duplicate validation; if you need observability for
such rejections instead, implement a Litestar exception handler for the
deserialization/validation exceptions rather than keeping this in the handler.
In `@src/synthorg/api/controllers/budget.py`:
- Around line 243-248: The domain event API_BUDGET_RECORDS_LISTED is currently
logged at debug level; change the call in the budget controller to
logger.info(...) so the event is emitted on the standard observability stream,
keeping the structured-logging form (pass API_BUDGET_RECORDS_LISTED,
agent_id=agent_id, task_id=task_id, record_count=len(records)) rather than
message-formatting; update the single call around where
API_BUDGET_RECORDS_LISTED is used so the fields remain identical.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 175-182: The manual length check for meeting_type is redundant
because Litestar's Parameter(max_length=QUERY_MAX_LENGTH) already enforces this
during deserialization; remove the conditional block that checks
len(meeting_type) > QUERY_MAX_LENGTH and the associated
logger.warning/ApiValidationError raise (references: meeting_type,
QUERY_MAX_LENGTH, API_VALIDATION_FAILED, ApiValidationError, logger.warning)
and, if you need observability for such validation failures, add or reuse a
Litestar exception handler for request validation errors rather than keeping
this unreachable in-handler check.
In `@tests/unit/api/controllers/test_budget.py`:
- Around line 205-233: Update the test_summaries_respect_agent_filter async test
to also assert the PeriodSummary token totals: after existing assertions on
record_count and total_cost_usd, add checks that
body["period_summary"]["total_input_tokens"] equals 200 and
body["period_summary"]["total_output_tokens"] equals 100 (two alice records of
100 input and 50 output each) so the PeriodSummary contract is fully validated.
- Around line 140-163: Add assertions for input and output token totals to the
test_period_summary_avg_cost test: after obtaining period =
body["period_summary"], assert period["total_input_tokens"] == 300 and assert
period["total_output_tokens"] == 150 (these totals come from three records with
input_tokens=100 and output_tokens=50). Update the test_period_summary_avg_cost
function to include these two assertions so the period summary token aggregates
are validated alongside record_count, total_cost_usd, and avg_cost_usd.
In `@web/src/api/endpoints/budget.ts`:
- Around line 36-47: The current success-path only checks body.success but can
still return a malformed CostRecordListResult with undefined fields; when
body?.success is true, validate that body.data, body.pagination (and its
total/offset/limit), body.daily_summary, and body.period_summary are present and
well-formed before returning, and if any are missing throw an ApiRequestError
(reuse the existing ApiRequestError thrown above). Prefer using the shared
unwrap helpers used elsewhere (instead of ad-hoc checks) to validate the
response shape, referencing the response variable and body object in budget.ts
and ensure the returned object builds from the validated fields only.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 662bf7fb-3e61-4801-a2fa-f5300a0f37e2
📒 Files selected for processing (14)
docs/design/communication.mddocs/design/operations.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/observability/events/api.pysrc/synthorg/observability/events/approval_gate.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.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). (8)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Detect Changes
- GitHub Check: Dependency Review
- GitHub Check: Build Preview
- GitHub Check: Gitleaks
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations- Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 - ruff enforces this on Python 3.14
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints. Type checking enforced by mypy in strict mode.
Google-style docstrings required on all public classes and functions, enforced by ruff D rules
Use Pydantic v2 conventions: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 (e.g., multiple tool invocations, parallel agent calls) over barecreate_task
Line length must be 88 characters (enforced by ruff)
Functions must be less than 50 lines, files less than 800 lines
Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Never useimport logging/logging.getLogger()/print()in application code (exceptobservability/setup.pyandobservability/sinks.pywhich may use stdlib logging and stderr print for bootstrap)
Always useloggeras the variable name (not_logger, notlog)
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Use structured logging: alwayslogger.info(EVENT, key=value)- never use printf-style formatting likelogger.info("msg %s", val)
All state transitions must log at INFO level
DEBUG logging is appropriate for object creation, internal flow, and entry/exit of key functions
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, etc. Vendor names only in: (1) Operations design page, (2).claude/skill files, (3) third-party import paths.
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every module with business logic MUST import:
from synthorg.observability import get_loggerand instantiatelogger = get_logger(__name__)
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation uses Markdown and is built with Zensical (config:
mkdocs.yml). API reference auto-generated from OpenAPI schema viascripts/export_openapi.py. Library reference auto-generated via mkdocstrings + Griffe.
Files:
docs/design/communication.mddocs/design/operations.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Use@pytest.mark.parametrizefor testing similar cases
Mark all tests with appropriate pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators. Run dev profile with 1000 examples viaHYPOTHESIS_PROFILE=devenvironment variable.
Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic. For tasks blocking indefinitely, useasyncio.Event().wait()instead ofasyncio.sleep(large_number).
Files:
tests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: React 19 + shadcn/ui + Tailwind CSS for web dashboard. TypeScript required inweb/src/.
Run ESLint withnpm --prefix web run lintfor React code style and quality checks
Run TypeScript type checking withnpm --prefix web run type-check
Use Axios for HTTP requests in the web dashboard
Use@tanstack/react-queryfor server state management and caching in the web dashboard
Files:
web/src/api/endpoints/approvals.tsweb/src/api/endpoints/meetings.tsweb/src/api/endpoints/budget.tsweb/src/api/types.ts
web/src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Web dashboard API client uses Axios with endpoint modules (18 domains) in
web/src/api/
Files:
web/src/api/endpoints/approvals.tsweb/src/api/endpoints/meetings.tsweb/src/api/endpoints/budget.tsweb/src/api/types.ts
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/api/**/*.py: REST API defined with Litestar. Errors must use RFC 9457 format insrc/synthorg/api/
Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
🧠 Learnings (31)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/observability/events/approval_gate.pydocs/design/operations.mdsrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under ai_company.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.) — import directly
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
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`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
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 instead of widening timing margins
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic. For tasks blocking indefinitely, use `asyncio.Event().wait()` instead of `asyncio.sleep(large_number)`.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
tests/unit/api/controllers/test_budget.pyweb/src/api/endpoints/budget.tsdocs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
tests/unit/api/controllers/test_budget.pydocs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
web/src/api/endpoints/budget.tsdocs/design/operations.mdsrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/synthorg/api/**/*.py : Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always log with structured kwargs: `logger.info(EVENT, key=value)` — never use old-style formatting `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` for all identifier/name fields.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
🪛 markdownlint-cli2 (0.22.0)
docs/design/operations.md
[warning] 1008-1008: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (17)
tests/unit/api/controllers/test_budget.py (1)
235-340: LGTM!The
TestCostRecordListResponseValidatorclass thoroughly validates theCostRecordListResponsemodel constraints: error/error_detail consistency and the computedsuccessfield derivation.web/src/api/types.ts (4)
32-32: LGTM!The
UrgencyLeveltype correctly mirrors the backendUrgencyLevelenum values.
279-282: LGTM!
ApprovalResponsecorrectly extendsApprovalItemwith the computed urgency fields, matching the backendApprovalResponsemodel.
388-402: LGTM!
DailySummaryandPeriodSummaryinterfaces correctly mirror the backend Pydantic models with appropriate field types.
765-769: LGTM!
MeetingResponsecorrectly extendsMeetingRecordwith analytics fields. Thereadonlymodifier oncontribution_rankappropriately matches the backend's immutable tuple type.tests/unit/api/controllers/test_meetings.py (3)
33-66: LGTM!The
_create_meeting_test_apphelper centralizes test app creation with proper dependency injection, improving maintainability across meeting controller tests.
69-125: LGTM!The
_make_minuteshelper withwith_contributionsparameter enables deterministic analytics testing. The fixed 120-second duration (started = now - timedelta(seconds=120)) ensures stable duration assertions.
348-423: LGTM!
TestMeetingAnalyticsprovides comprehensive coverage for the new analytics fields:
- Token aggregation by participant (agent-alpha: 450, agent-beta: 400)
- Contribution ranking sorted descending
- Duration computation from timestamps
- Empty analytics for non-completed meetings
- Duration presence even with empty contributions
src/synthorg/api/controllers/meetings.py (2)
81-106: LGTM!The
MeetingResponsemodel correctly extendsMeetingRecordwith analytics fields. Thege=0.0constraint onmeeting_duration_secondsprevents negative durations.
108-140: LGTM!The
_to_meeting_responsefunction correctly:
- Aggregates tokens by
agent_id(input + output)- Sorts participants by total tokens descending for
contribution_rank- Computes duration from
ended_at - started_at- Returns empty analytics when
minutesisNonesrc/synthorg/api/controllers/budget.py (3)
44-100: LGTM!
DailySummaryandPeriodSummarymodels are well-designed:
- Appropriate
ge=0constraints on all numeric fieldsavg_cost_usdas@computed_fieldfollows the guideline for derived values- Division-by-zero handled in
avg_cost_usdproperty
103-142: LGTM!
CostRecordListResponsecorrectly enforces the error/error_detail consistency invariant via@model_validatorand derivessuccessfromerrorvia@computed_field.
145-186: LGTM!The
_build_summariesfunction correctly:
- Handles empty records with zero-valued summary
- Groups by ISO date string for daily aggregation
- Uses
math.fsumfor floating-point accuracy- Returns chronologically sorted daily summaries
src/synthorg/api/controllers/approvals.py (4)
63-77: LGTM!The
UrgencyLevelenum and threshold constants are well-defined. UsingStrEnumensures JSON serialization produces lowercase string values matching the documented contract.
80-129: LGTM!The
ApprovalResponsemodel and_to_approval_responsefunction correctly implement the urgency computation:
seconds_remainingclamped to0.0for expired items- Urgency levels correctly bucketed by thresholds
Nonehandling for items without expiry
186-194: LGTM!The
except MemoryError, RecursionError:syntax follows PEP 758 for Python 3.14, as required by coding guidelines.
390-471: LGTM!The
_get_approval_or_404and_save_decision_and_notifyhelpers consolidate common logic:
- Consistent 404 handling with warning logging
- Conflict detection via
save_if_pendingreturn value- Proper sequencing: persist → publish → log → resume
There was a problem hiding this comment.
Actionable comments posted: 3
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/controllers/approvals.py (1)
503-505: 🧹 Nitpick | 🔵 TrivialManual validation is redundant but intentionally retained.
The
Parameter(max_length=QUERY_MAX_LENGTH)constraint on line 485-486 validates before the handler executes. However, per the commit message, this manual check was intentionally restored "to avoid framework error behavior" — likely to provide consistent RFC 9457 error responses or observability logging.If the intent is observability, consider moving this logic to a Litestar exception handler for cleaner separation. Otherwise, a brief comment explaining why the check is retained would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/approvals.py` around lines 503 - 505, The manual max-length check for action_type (using QUERY_MAX_LENGTH) duplicates the Parameter(max_length=...) validation but was intentionally retained; either add a concise comment above the if-block explaining this (mentioning the framework error behavior / RFC 9457 consistent response intent) or move the check into a Litestar exception handler so observability/consistent RFC 9457 responses are handled centrally; update the code around the action_type length check and/or implement a Litestar exception handler that raises ApiValidationError with the same message so the behavior remains identical while removing inline duplication.
♻️ Duplicate comments (2)
src/synthorg/api/controllers/meetings.py (2)
90-97:⚠️ Potential issue | 🟠 MajorResolve analytics keys to participant names before serializing the response.
Issue
#774/ the PR objective definestoken_usage_by_participantasagent_name -> token_count, but the model contract and aggregation logic both exposec.agent_id. That silently changes the public response shape and exposes internal identifiers unless the issue/spec was intentionally re-baselined.Also applies to: 121-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 90 - 97, The public response currently exposes agent IDs instead of agent names for token_usage_by_participant and contribution_rank; update the serialization step that builds the meeting response (the code that constructs token_usage_by_participant and contribution_rank) to map c.agent_id -> agent_name using the meeting/agent lookup (e.g., resolve each agent_id to its human-readable name from the meeting's agent registry) before assigning to the model fields token_usage_by_participant and contribution_rank so the returned keys/values match the spec (agent_name -> token_count and agent_name-sorted tuple). Ensure this mapping is applied in both places where these fields are populated (the aggregation logic that produces token_usage_by_participant and the logic that constructs contribution_rank).
157-158:⚠️ Potential issue | 🟠 Major
Parameter(max_length=...)can short-circuit the custom validation path.If Litestar rejects oversized
meeting_typevalues before the handler runs, Line 172 never executes and clients get framework validation instead of yourApiValidationErrorenvelope. Pick one path: keep the manual RFC 9457 branch and drop theParameterconstraint, or normalize framework validation centrally and delete the branch.#!/bin/bash # Verify whether request-validation failures are globally normalized to RFC 9457 # and whether oversized `meeting_type` is already covered by tests. rg -n -C2 'ApiValidationError|ValidationException|RequestValidation|RFC 9457|problem\+json|exception_handler' src/synthorg/api tests rg -n -C2 'meeting_type exceeds maximum length|max_length=QUERY_MAX_LENGTH|QUERY_MAX_LENGTH' src/synthorg/api testsAs per coding guidelines, "
src/synthorg/api/**/*.py: Errors must use RFC 9457 format insrc/synthorg/api/."Also applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 157 - 158, The Parameter(max_length=QUERY_MAX_LENGTH) on the meeting_type query param prevents your custom RFC 9457-style validation from running; remove the Parameter constraint from the meeting_type annotation (keep the Annotated[str] | None = None signature) and instead perform the length check inside the handler where you raise ApiValidationError (the existing manual branch referencing QUERY_MAX_LENGTH and ApiValidationError), or alternatively remove that manual branch and rely on centralized framework-to-RFC9457 normalization—pick one approach and make the code consistent (i.e., either drop Parameter and keep the manual check+ApiValidationError in the handler, or keep Parameter and delete the handler branch).
🤖 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/controllers/approvals.py`:
- Around line 79-97: ApprovalResponse currently exposes seconds_remaining and
urgency_level as regular fields filled by a factory; change to compute them at
access time using Pydantic's `@computed_field` to avoid mismatched instantiation.
Add an internal reference_time: datetime =
Field(default_factory=datetime.utcnow, exclude=True) on ApprovalResponse (or
accept it through the constructor), remove seconds_remaining and urgency_level
as stored fields, and implement two `@computed_field` methods named
seconds_remaining and urgency_level that compute values from reference_time and
the existing TTL/expiry data on the base ApprovalItem (use the existing
UrgencyLevel mapping logic). Ensure model_config = ConfigDict(frozen=True)
remains and the computed fields are read-only.
In `@src/synthorg/api/controllers/budget.py`:
- Around line 127-135: The model validator _validate_error_detail_consistency
currently raises ValueError when error and error_detail are inconsistent but
does not log; before each raise add a structured logger.warning call (including
the same message and contextual info such as self.error, self.error_detail and
any identifying request/id fields available on the model) so failures are
recorded, then raise the ValueError as before; reference the
_validate_error_detail_consistency method and the error/error_detail fields when
making the log entries.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 90-102: The three analytics fields on MeetingResponse
(token_usage_by_participant, contribution_rank, meeting_duration_seconds) are
derived from MeetingRecord.minutes and should be converted from stored Fields
into `@computed_field` properties on the MeetingResponse model; remove their Field
definitions and implement corresponding `@computed_field` methods that compute
values from self.minutes (reusing the same logic currently used in
_to_meeting_response()), update any serialization/consumer code to read the
computed properties, and delete the duplicate computation in
_to_meeting_response() so the schema owns the derived logic.
---
Outside diff comments:
In `@src/synthorg/api/controllers/approvals.py`:
- Around line 503-505: The manual max-length check for action_type (using
QUERY_MAX_LENGTH) duplicates the Parameter(max_length=...) validation but was
intentionally retained; either add a concise comment above the if-block
explaining this (mentioning the framework error behavior / RFC 9457 consistent
response intent) or move the check into a Litestar exception handler so
observability/consistent RFC 9457 responses are handled centrally; update the
code around the action_type length check and/or implement a Litestar exception
handler that raises ApiValidationError with the same message so the behavior
remains identical while removing inline duplication.
---
Duplicate comments:
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 90-97: The public response currently exposes agent IDs instead of
agent names for token_usage_by_participant and contribution_rank; update the
serialization step that builds the meeting response (the code that constructs
token_usage_by_participant and contribution_rank) to map c.agent_id ->
agent_name using the meeting/agent lookup (e.g., resolve each agent_id to its
human-readable name from the meeting's agent registry) before assigning to the
model fields token_usage_by_participant and contribution_rank so the returned
keys/values match the spec (agent_name -> token_count and agent_name-sorted
tuple). Ensure this mapping is applied in both places where these fields are
populated (the aggregation logic that produces token_usage_by_participant and
the logic that constructs contribution_rank).
- Around line 157-158: The Parameter(max_length=QUERY_MAX_LENGTH) on the
meeting_type query param prevents your custom RFC 9457-style validation from
running; remove the Parameter constraint from the meeting_type annotation (keep
the Annotated[str] | None = None signature) and instead perform the length check
inside the handler where you raise ApiValidationError (the existing manual
branch referencing QUERY_MAX_LENGTH and ApiValidationError), or alternatively
remove that manual branch and rely on centralized framework-to-RFC9457
normalization—pick one approach and make the code consistent (i.e., either drop
Parameter and keep the manual check+ApiValidationError in the handler, or keep
Parameter and delete the handler branch).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41881d5c-b8f9-4f9b-81e3-b5391fa615e9
📒 Files selected for processing (6)
docs/design/communication.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.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). (1)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation uses Markdown and is built with Zensical (config:
mkdocs.yml). API reference auto-generated from OpenAPI schema viascripts/export_openapi.py. Library reference auto-generated via mkdocstrings + Griffe.
Files:
docs/design/communication.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations- Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 - ruff enforces this on Python 3.14
Files:
tests/unit/api/controllers/test_approvals.pysrc/synthorg/api/controllers/meetings.pytests/unit/api/controllers/test_budget.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Use@pytest.mark.parametrizefor testing similar cases
Mark all tests with appropriate pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators. Run dev profile with 1000 examples viaHYPOTHESIS_PROFILE=devenvironment variable.
Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic. For tasks blocking indefinitely, useasyncio.Event().wait()instead ofasyncio.sleep(large_number).
Files:
tests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints. Type checking enforced by mypy in strict mode.
Google-style docstrings required on all public classes and functions, enforced by ruff D rules
Use Pydantic v2 conventions: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 (e.g., multiple tool invocations, parallel agent calls) over barecreate_task
Line length must be 88 characters (enforced by ruff)
Functions must be less than 50 lines, files less than 800 lines
Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Never useimport logging/logging.getLogger()/print()in application code (exceptobservability/setup.pyandobservability/sinks.pywhich may use stdlib logging and stderr print for bootstrap)
Always useloggeras the variable name (not_logger, notlog)
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Use structured logging: alwayslogger.info(EVENT, key=value)- never use printf-style formatting likelogger.info("msg %s", val)
All state transitions must log at INFO level
DEBUG logging is appropriate for object creation, internal flow, and entry/exit of key functions
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, etc. Vendor names only in: (1) Operations design page, (2).claude/skill files, (3) third-party import paths.
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every module with business logic MUST import:
from synthorg.observability import get_loggerand instantiatelogger = get_logger(__name__)
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/api/**/*.py: REST API defined with Litestar. Errors must use RFC 9457 format insrc/synthorg/api/
Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
🧠 Learnings (22)
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
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 instead of widening timing margins
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic. For tasks blocking indefinitely, use `asyncio.Event().wait()` instead of `asyncio.sleep(large_number)`.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to tests/**/*.py : Use `pytest.mark.parametrize` for testing similar cases
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Test markers: `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, `pytest.mark.slow`. Coverage: 80% minimum. Async: `asyncio_mode = 'auto'` — no manual `pytest.mark.asyncio` needed. Timeout: 30 seconds per test. Parallelism: `pytest-xdist` via `-n auto` — ALWAYS include `-n auto` when running pytest, never run tests sequentially.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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/controllers/test_approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
tests/unit/api/controllers/test_budget.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
tests/unit/api/controllers/test_budget.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` for all identifier/name fields.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always log with structured kwargs: `logger.info(EVENT, key=value)` — never use old-style formatting `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
🔇 Additional comments (17)
tests/unit/api/controllers/test_budget.py (2)
91-237: Strong coverage for summary semantics and pagination/filter interactions.The new summary tests validate grouping, ordering, empty-state behavior, and “all matched records vs. current page” behavior well.
239-344: Validator tests are comprehensive for the new response contract.The constructor validation and computed
successbehavior are exercised cleanly across valid and invalid combinations.tests/unit/api/controllers/test_approvals.py (6)
424-438: LGTM!The class is properly marked with
@pytest.mark.unitand the initial test correctly verifies the presence of urgency fields in list responses.
440-449: LGTM!Properly tests the no-expiry case where
seconds_remainingisNoneandurgency_levelis"no_expiry".
451-478: LGTM!Good use of
@pytest.mark.parametrizeto consolidate similar urgency classification tests. The TTL values are safely inside their respective buckets, avoiding timing sensitivity.
480-544: LGTM!These tests appropriately verify that all approval endpoints (create, get, approve, reject) include the new urgency fields in their responses.
546-583: LGTM!The boundary tests now correctly freeze time using
patchand are parametrized as suggested in previous reviews. The test verifies that exactly 1 hour remaining yields"high"(not"critical") and exactly 4 hours yields"normal"(not"high"), matching the<threshold semantics in the controller.
585-609: LGTM!Correctly tests that expired approvals clamp
seconds_remainingto0.0and show"critical"urgency. The 1-hour expiration buffer provides sufficient margin for CI timing variations.src/synthorg/api/controllers/approvals.py (9)
62-76: LGTM!The urgency thresholds are well-documented with clear enum values. The
StrEnumchoice ensures JSON serialization produces lowercase strings matching the API contract.
99-128: LGTM!The urgency computation logic is correct: seconds are clamped to
≥ 0.0, and thresholds use strict<comparisons matching the documented boundaries (critical < 1hr, high < 4hrs, normal ≥ 4hrs).
389-414: LGTM!Good refactoring to centralize the approval lookup and 404 handling. Properly logs at WARNING level before raising, as required by coding guidelines.
417-470: LGTM!Good consolidation of the decision persistence logic. The function correctly uses
saved(the persisted item) for both the WebSocket event and the resume intent, addressing the previous review feedback.
518-538: LGTM!Clean refactoring using the new
_get_approval_or_404helper and response enrichment.
540-606: LGTM!The handler correctly reuses the same
nowtimestamp for both theexpires_atcalculation and response enrichment, ensuring consistency.
608-662: LGTM!Good refactoring with consistent use of helpers. The
nowtimestamp is captured once and used for both the decision timestamp and response enrichment.
664-718: LGTM!The reject handler mirrors the approve handler's structure, ensuring consistent behavior and maintainability.
323-330: LGTM!Using
APPROVAL_GATE_REVIEW_TRANSITION_FAILEDis more semantically accurate for this context than the genericAPPROVAL_GATE_RESUME_FAILED. This improves observability by distinguishing review-gate failures from mid-execution resume failures.
| class ApprovalResponse(ApprovalItem): | ||
| """Approval item enriched with computed urgency fields. | ||
|
|
||
| Attributes: | ||
| seconds_remaining: Seconds until expiry, clamped to 0.0 for | ||
| expired items (``None`` if no TTL). | ||
| urgency_level: Urgency classification based on time remaining. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(frozen=True) | ||
|
|
||
| seconds_remaining: float | None = Field( | ||
| ge=0.0, | ||
| description="Seconds until expiry (null if no TTL set)", | ||
| ) | ||
| urgency_level: UrgencyLevel = Field( | ||
| description="Urgency classification based on remaining time", | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using @computed_field for derived urgency values.
The current implementation stores seconds_remaining and urgency_level as regular fields populated by a factory function. Per coding guidelines, derived values should use @computed_field. The recommended pattern is to add an internal reference_time: datetime = Field(exclude=True) and compute the derived fields as properties. This prevents the possibility of inconsistent instantiation where someone creates an ApprovalResponse with mismatched values.
That said, the factory approach is simpler and the frozen=True config mitigates mutation risks. If the team prefers this pattern for time-dependent derived fields, consider documenting the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/approvals.py` around lines 79 - 97,
ApprovalResponse currently exposes seconds_remaining and urgency_level as
regular fields filled by a factory; change to compute them at access time using
Pydantic's `@computed_field` to avoid mismatched instantiation. Add an internal
reference_time: datetime = Field(default_factory=datetime.utcnow, exclude=True)
on ApprovalResponse (or accept it through the constructor), remove
seconds_remaining and urgency_level as stored fields, and implement two
`@computed_field` methods named seconds_remaining and urgency_level that compute
values from reference_time and the existing TTL/expiry data on the base
ApprovalItem (use the existing UrgencyLevel mapping logic). Ensure model_config
= ConfigDict(frozen=True) remains and the computed fields are read-only.
| @model_validator(mode="after") | ||
| def _validate_error_detail_consistency(self) -> Self: | ||
| """Ensure ``error`` and ``error_detail`` are set together.""" | ||
| if self.error_detail is not None and self.error is None: | ||
| msg = "error_detail requires error to be set" | ||
| raise ValueError(msg) | ||
| if self.error is not None and self.error_detail is None: | ||
| msg = "error must be accompanied by error_detail" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Log validator failures before raising exceptions.
Line 130 and Line 133 raise ValueError without a preceding warning/error log. Please add structured logger.warning(...) context before each raise so response-construction failures are observable.
As per coding guidelines, "Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/budget.py` around lines 127 - 135, The model
validator _validate_error_detail_consistency currently raises ValueError when
error and error_detail are inconsistent but does not log; before each raise add
a structured logger.warning call (including the same message and contextual info
such as self.error, self.error_detail and any identifying request/id fields
available on the model) so failures are recorded, then raise the ValueError as
before; reference the _validate_error_detail_consistency method and the
error/error_detail fields when making the log entries.
| token_usage_by_participant: dict[str, int] = Field( | ||
| default_factory=dict, | ||
| description="Total tokens consumed per participant", | ||
| ) | ||
| contribution_rank: tuple[NotBlankStr, ...] = Field( | ||
| default=(), | ||
| description="Agent IDs sorted by contribution (descending)", | ||
| ) | ||
| meeting_duration_seconds: float | None = Field( | ||
| default=None, | ||
| ge=0.0, | ||
| description="Meeting duration in seconds (null if no minutes)", | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect existing computed-field patterns in the codebase before refactoring
# `MeetingResponse`.
rg -n -C2 '@computed_field|computed_field\(' src/synthorgRepository: Aureliolo/synthorg
Length of output: 24502
🏁 Script executed:
# First, locate the meetings.py file and check its size
fd -n "meetings.py" src/synthorg/api/controllers/
# Then read the relevant lines to see the class structure
cat -n src/synthorg/api/controllers/meetings.py | head -200Repository: Aureliolo/synthorg
Length of output: 8840
🏁 Script executed:
# Read the file without fd -n, use cat directly
cat src/synthorg/api/controllers/meetings.py | sed -n '78,137p'Repository: Aureliolo/synthorg
Length of output: 2102
Refactor to use @computed_field for analytics instead of materializing via helper.
token_usage_by_participant, contribution_rank, and meeting_duration_seconds are pure derivations of MeetingRecord.minutes, so storing them as ordinary fields and computing in _to_meeting_response() duplicates state. Define these three fields as @computed_field properties on MeetingResponse instead—the logic stays with the schema, serialization remains consistent, and the pattern follows Pydantic v2 conventions used throughout the codebase.
Also applies to: 105-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/synthorg/api/controllers/meetings.py` around lines 90 - 102, The three
analytics fields on MeetingResponse (token_usage_by_participant,
contribution_rank, meeting_duration_seconds) are derived from
MeetingRecord.minutes and should be converted from stored Fields into
`@computed_field` properties on the MeetingResponse model; remove their Field
definitions and implement corresponding `@computed_field` methods that compute
values from self.minutes (reusing the same logic currently used in
_to_meeting_response()), update any serialization/consumer code to read the
computed properties, and delete the duplicate computation in
_to_meeting_response() so the schema owns the derived logic.
730b7bc to
adf7f94
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/controllers/budget.py (1)
217-255:⚠️ Potential issue | 🟠 MajorAdd manual query-parameter length checks to match the fallback pattern in
approvals.pyandmeetings.py.Both
agent_idandtask_iduseParameter(max_length=QUERY_MAX_LENGTH), but Litestar's validation crashes the worker instead of returning a proper RFC 9457 error response. Add handler-level guards before callingget_records(...):Example pattern
if agent_id is not None and len(agent_id) > QUERY_MAX_LENGTH: msg = f"agent_id exceeds maximum length of {QUERY_MAX_LENGTH}" raise ApiValidationError(msg) if task_id is not None and len(task_id) > QUERY_MAX_LENGTH: msg = f"task_id exceeds maximum length of {QUERY_MAX_LENGTH}" raise ApiValidationError(msg)(See
src/synthorg/api/controllers/approvals.pyline 506 andsrc/synthorg/api/controllers/meetings.pyline 175 for the same pattern.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/budget.py` around lines 217 - 255, Add explicit length guards for the query params agent_id and task_id before calling AppState.cost_tracker.get_records: check if each is not None and len(value) > QUERY_MAX_LENGTH and raise ApiValidationError with a clear message like "agent_id exceeds maximum length of {QUERY_MAX_LENGTH}" (and similarly for task_id). Place these checks in the same handler function that calls get_records so Litestar doesn't crash and the API returns the proper RFC 9457 validation error.
♻️ Duplicate comments (4)
src/synthorg/api/controllers/meetings.py (2)
78-102: 🛠️ Refactor suggestion | 🟠 MajorMake
token_usage_by_participant,contribution_rank, andmeeting_duration_secondscomputed fields.These values are derived entirely from
MeetingRecord.minutes, but the DTO stores them as normal fields and depends on_to_meeting_response()to keep them synchronized. That leaves room for inconsistentMeetingResponseinstances and makes every endpoint remember a bespoke enrichment step. Move the derivation ontoMeetingResponseitself and let the mapper just wrap the base record.Based on learnings, "use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields."
Also applies to: 105-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 78 - 102, MeetingResponse currently stores token_usage_by_participant, contribution_rank, and meeting_duration_seconds as regular fields and relies on the mapper _to_meeting_response() to populate them; change these to Pydantic computed fields on MeetingResponse so they are derived from MeetingRecord.minutes at access time (use pydantic.computed_field or the v2 equivalent) and remove the redundant Field declarations/defaults; keep the same return types (dict[str,int], tuple[NotBlankStr,...], float|None) and descriptions in docstring/comments, update model_config if needed for computed fields, and simplify the mapper to only wrap the base MeetingRecord into MeetingResponse without performing enrichment.
19-20:⚠️ Potential issue | 🟡 MinorLog the manual
meeting_typerejection before raising.This custom branch exists specifically because you're bypassing framework validation here, but it currently raises
ApiValidationErrorwithout the WARNING/ERROR event required for explicit error paths. Add a structuredAPI_VALIDATION_FAILEDlog with the field name and actual length before raising.🛠️ Minimal fix
-from synthorg.observability.events.api import API_MEETING_TRIGGERED +from synthorg.observability.events.api import ( + API_MEETING_TRIGGERED, + API_VALIDATION_FAILED, +)if meeting_type is not None and len(meeting_type) > QUERY_MAX_LENGTH: + logger.warning( + API_VALIDATION_FAILED, + field="meeting_type", + max_length=QUERY_MAX_LENGTH, + actual_length=len(meeting_type), + ) msg = f"meeting_type exceeds maximum length of {QUERY_MAX_LENGTH}" raise ApiValidationError(msg)As per coding guidelines, "Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising."
Also applies to: 172-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 19 - 20, In the branch in meetings.py that manually rejects the meeting_type (the code that currently raises ApiValidationError), add a structured log call using the API_VALIDATION_FAILED event before raising: include the field name ("meeting_type") and the actual length/value (e.g., len(meeting_type) and meeting_type) in the log payload at WARNING/ERROR level; do the same change for the similar branch around lines 172-177. Locate the branch by searching for the ApiValidationError raises related to meeting_type and insert a processLogger (or existing logger) API_VALIDATION_FAILED event with context (field, actual_length, max_length) immediately prior to raising the exception.src/synthorg/api/controllers/budget.py (1)
127-135:⚠️ Potential issue | 🟠 MajorLog invalid
CostRecordListResponsestates before raising.Both validator branches raise
ValueErrorsilently, so malformed response construction loses the context that would explain why serialization failed.As per coding guidelines, "Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/budget.py` around lines 127 - 135, The validator method _validate_error_detail_consistency currently raises ValueError without logging; update this method to log the invalid CostRecordListResponse state (include which branch triggered, the error message, and relevant context such as self.error and self.error_detail or other identifying fields) at WARNING or ERROR level immediately before raising to preserve diagnostic context; use the module/class logger (e.g., logger or getLogger(__name__)) and ensure the log message is descriptive and includes the same msg string used for the ValueError so callers can correlate logs with the raised exception.src/synthorg/api/controllers/approvals.py (1)
79-97: 🛠️ Refactor suggestion | 🟠 MajorExpose urgency via
@computed_field, not stored fields.
seconds_remainingandurgency_levelare derived fromexpires_atplus the reference time, so keeping them as regular fields still allows inconsistentApprovalResponseinstances outside_to_approval_response(). An excluded backingreference_timewith@computed_fieldkeeps the DTO internally consistent and matches the rest of the Pydantic guidance in this repo.For Pydantic v2.12.5 response models, what is the recommended pattern for exposing derived read-only fields: regular fields populated by a factory, or `@computed_field` backed by an excluded input field?As per coding guidelines, "Use Pydantic v2 conventions:
BaseModel,model_validator,computed_field,ConfigDict. Use@computed_fieldfor derived values instead of storing redundant fields."Also applies to: 99-128
🤖 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/controllers/approvals.py`:
- Around line 503-508: Add a warning log immediately before raising
ApiValidationError in the action_type length check: log the rejected value
(action_type), its length (len(action_type)), and the limit (QUERY_MAX_LENGTH)
at WARNING level so the event is observable, e.g., use the module/controller
logger (logger.warning(...)) in the guard that checks "if action_type is not
None and len(action_type) > QUERY_MAX_LENGTH" before raising
ApiValidationError(msg).
In `@tests/unit/api/controllers/test_meetings.py`:
- Around line 283-299: The test test_trigger_response_includes_analytics
currently only checks for presence of keys, which allows regressions; update it
to assert the concrete analytics values returned when
mock_scheduler.trigger_event returns (_make_record("mtg-triggered"),). After
calling meeting_client.post(...), extract item = resp.json()["data"][0] and
assert specific fields equal the deterministic values produced by
_make_record("mtg-triggered") (e.g., compare token_usage_by_participant,
contribution_rank, and meeting_duration_seconds to their expected numeric/dict
values), ensuring the test fails if defaults or wrong durations are returned;
keep references to mock_scheduler.trigger_event and _make_record to locate where
expected values come from.
---
Outside diff comments:
In `@src/synthorg/api/controllers/budget.py`:
- Around line 217-255: Add explicit length guards for the query params agent_id
and task_id before calling AppState.cost_tracker.get_records: check if each is
not None and len(value) > QUERY_MAX_LENGTH and raise ApiValidationError with a
clear message like "agent_id exceeds maximum length of {QUERY_MAX_LENGTH}" (and
similarly for task_id). Place these checks in the same handler function that
calls get_records so Litestar doesn't crash and the API returns the proper RFC
9457 validation error.
---
Duplicate comments:
In `@src/synthorg/api/controllers/budget.py`:
- Around line 127-135: The validator method _validate_error_detail_consistency
currently raises ValueError without logging; update this method to log the
invalid CostRecordListResponse state (include which branch triggered, the error
message, and relevant context such as self.error and self.error_detail or other
identifying fields) at WARNING or ERROR level immediately before raising to
preserve diagnostic context; use the module/class logger (e.g., logger or
getLogger(__name__)) and ensure the log message is descriptive and includes the
same msg string used for the ValueError so callers can correlate logs with the
raised exception.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 78-102: MeetingResponse currently stores
token_usage_by_participant, contribution_rank, and meeting_duration_seconds as
regular fields and relies on the mapper _to_meeting_response() to populate them;
change these to Pydantic computed fields on MeetingResponse so they are derived
from MeetingRecord.minutes at access time (use pydantic.computed_field or the v2
equivalent) and remove the redundant Field declarations/defaults; keep the same
return types (dict[str,int], tuple[NotBlankStr,...], float|None) and
descriptions in docstring/comments, update model_config if needed for computed
fields, and simplify the mapper to only wrap the base MeetingRecord into
MeetingResponse without performing enrichment.
- Around line 19-20: In the branch in meetings.py that manually rejects the
meeting_type (the code that currently raises ApiValidationError), add a
structured log call using the API_VALIDATION_FAILED event before raising:
include the field name ("meeting_type") and the actual length/value (e.g.,
len(meeting_type) and meeting_type) in the log payload at WARNING/ERROR level;
do the same change for the similar branch around lines 172-177. Locate the
branch by searching for the ApiValidationError raises related to meeting_type
and insert a processLogger (or existing logger) API_VALIDATION_FAILED event with
context (field, actual_length, max_length) immediately prior to raising the
exception.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4277b283-bdd9-48fd-932f-c5a439f3aec0
📒 Files selected for processing (14)
docs/design/communication.mddocs/design/operations.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/observability/events/api.pysrc/synthorg/observability/events/approval_gate.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.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: Test (Python 3.14)
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Build Sandbox
- GitHub Check: Dependency Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Do NOT usefrom __future__ import annotations- Python 3.14 has PEP 649 with native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 - ruff enforces this on Python 3.14
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints. Type checking enforced by mypy in strict mode.
Google-style docstrings required on all public classes and functions, enforced by ruff D rules
Use Pydantic v2 conventions: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 (e.g., multiple tool invocations, parallel agent calls) over barecreate_task
Line length must be 88 characters (enforced by ruff)
Functions must be less than 50 lines, files less than 800 lines
Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Never useimport logging/logging.getLogger()/print()in application code (exceptobservability/setup.pyandobservability/sinks.pywhich may use stdlib logging and stderr print for bootstrap)
Always useloggeras the variable name (not_logger, notlog)
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Use structured logging: alwayslogger.info(EVENT, key=value)- never use printf-style formatting likelogger.info("msg %s", val)
All state transitions must log at INFO level
DEBUG logging is appropriate for object creation, internal flow, and entry/exit of key functions
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, etc. Vendor names only in: (1) Operations design page, (2).claude/skill files, (3) third-party import paths.
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every module with business logic MUST import:
from synthorg.observability import get_loggerand instantiatelogger = get_logger(__name__)
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation uses Markdown and is built with Zensical (config:
mkdocs.yml). API reference auto-generated from OpenAPI schema viascripts/export_openapi.py. Library reference auto-generated via mkdocstrings + Griffe.
Files:
docs/design/communication.mddocs/design/operations.md
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/api/**/*.py: REST API defined with Litestar. Errors must use RFC 9457 format insrc/synthorg/api/
Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Use@pytest.mark.parametrizefor testing similar cases
Mark all tests with appropriate pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators. Run dev profile with 1000 examples viaHYPOTHESIS_PROFILE=devenvironment variable.
Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic. For tasks blocking indefinitely, useasyncio.Event().wait()instead ofasyncio.sleep(large_number).
Files:
tests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_meetings.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: React 19 + shadcn/ui + Tailwind CSS for web dashboard. TypeScript required inweb/src/.
Run ESLint withnpm --prefix web run lintfor React code style and quality checks
Run TypeScript type checking withnpm --prefix web run type-check
Use Axios for HTTP requests in the web dashboard
Use@tanstack/react-queryfor server state management and caching in the web dashboard
Files:
web/src/api/types.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/meetings.ts
web/src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Web dashboard API client uses Axios with endpoint modules (18 domains) in
web/src/api/
Files:
web/src/api/types.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/meetings.ts
🧠 Learnings (45)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/observability/events/approval_gate.pydocs/design/operations.mdsrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under ai_company.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.) — import directly
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Always use event name constants from domain-specific modules under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
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`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
docs/design/operations.mdweb/src/api/endpoints/budget.tssrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
docs/design/operations.mdtests/unit/api/controllers/test_budget.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
docs/design/operations.mdtests/unit/api/controllers/test_budget.pyweb/src/api/endpoints/budget.tssrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
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 instead of widening timing margins
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to tests/**/*.py : Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mock `time.monotonic()` and `asyncio.sleep()` to make them deterministic. For tasks blocking indefinitely, use `asyncio.Event().wait()` instead of `asyncio.sleep(large_number)`.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to tests/**/*.py : Use `pytest.mark.parametrize` for testing similar cases
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Test markers: `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, `pytest.mark.slow`. Coverage: 80% minimum. Async: `asyncio_mode = 'auto'` — no manual `pytest.mark.asyncio` needed. Timeout: 30 seconds per test. Parallelism: `pytest-xdist` via `-n auto` — ALWAYS include `-n auto` when running pytest, never run tests sequentially.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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/controllers/test_approvals.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
tests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/synthorg/api/**/*.py : Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always log with structured kwargs: `logger.info(EVENT, key=value)` — never use old-style formatting `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T16:55:07.730Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T16:55:07.730Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : 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.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to src/synthorg/**/*.py : 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, and key function entry/exit
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:43:14.114Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:43:14.114Z
Learning: Applies to src/synthorg/**/*.py : 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. Pure data models, enums, and re-exports do NOT need logging.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` for all identifier/name fields.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
src/synthorg/api/controllers/approvals.py
🪛 markdownlint-cli2 (0.22.0)
docs/design/operations.md
[warning] 1008-1008: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
Add computed fields to API responses for richer dashboard displays: - Approval responses: seconds_remaining, urgency_level enum (critical/high/normal/no_expiry) on all endpoints - Meeting responses: token_usage_by_participant, contribution_rank, meeting_duration_seconds on list/get/trigger endpoints - Budget records: daily_summary aggregation and period_summary stats (avg cost, total tokens) computed from all filtered records Closes #774 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- PeriodSummary.avg_cost_usd: convert stored field to @computed_field - CostRecordListResponse: add error_detail consistency validator - DailySummary.date: use NotBlankStr instead of raw str - ApprovalResponse.seconds_remaining: add ge=0.0 constraint - MeetingResponse.meeting_duration_seconds: add ge=0.0 constraint - Fix _try_review_gate_transition using wrong log event constant - Add 10 new tests: boundary, edge cases, missing assertions - Update frontend types: ApprovalResponse, MeetingResponse, summaries - Update design spec: approval urgency, meeting analytics, budget summaries Pre-reviewed by 9 agents, 17 findings addressed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ernal reviewers) - Critical: budget.ts error_detail preserved in ApiRequestError (RFC 9457) - Major: _to_approval_response accepts `now` param for deterministic time - Major: boundary tests freeze time via datetime mock (no timing margins) - Major: approve/reject extracted into _get_approval_or_404 and _save_decision_and_notify helpers (under 50-line limit) - Major: contribution_rank uses tuple[NotBlankStr, ...] per convention - Major: communication.md type corrected and duration separated from contribution-derived fields - Major: CostRecordListResponse validator tested (4 new tests) - Medium: math.fsum for floating-point cost summation precision - Medium: .date().isoformat() replaces strftime for date keys - Medium: logging added to budget controller and meetings trigger/validation - Medium: filtered-summary test verifies aggregation is post-filter - Medium: daily summary sort order explicitly asserted - Medium: __import__ antipattern replaced with normal import in test fixtures - Medium: duplicated test client setup extracted into helper - Minor: docstrings improved (thresholds, clamping, duration, aggregation, error_detail pairing, PeriodSummary field order) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces hardcoded 128 in Parameter(max_length=128) annotations with the QUERY_MAX_LENGTH constant across approvals, budget, and meetings controllers, preventing drift between framework-level and controller- level validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use saved (post-persistence) instead of updated (pre-persistence) when publishing WebSocket events, ensuring the event reflects committed state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Clamp meeting_duration_seconds to 0.0 for negative deltas; update docs - Parametrize urgency level tests and boundary tests with pytest.mark - Restore manual query param validation (Parameter max_length crashes for query params instead of returning proper HTTP errors) - Budget records logging raised from DEBUG to INFO - Add token total assertions to period summary and filtered summary tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Litestar Parameter(max_length=...) on query params crashes the worker instead of returning a proper RFC 9457 error response. Document why the manual checks are retained alongside the Parameter annotation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ssertions - Log API_VALIDATION_FAILED before raising in approvals, meetings, and budget controllers for query param length violations - Add manual length guards for budget agent_id/task_id query params (same Litestar crash issue as approvals/meetings) - Assert concrete analytics values in trigger_meeting test instead of just checking key presence Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/synthorg/api/controllers/budget.py (1)
131-140:⚠️ Potential issue | 🟡 MinorAdd logging before raising in the model validator.
The validator raises
ValueErrorat lines 136 and 139 without a preceding log statement. While this is an internal consistency check, the coding guidelines require logging all error paths before raising.,
🛡️ Proposed fix to add logging
`@model_validator`(mode="after") def _validate_error_detail_consistency(self) -> Self: """Ensure ``error`` and ``error_detail`` are set together.""" if self.error_detail is not None and self.error is None: msg = "error_detail requires error to be set" + logger.warning( + API_VALIDATION_FAILED, + reason=msg, + error=self.error, + error_detail_present=self.error_detail is not None, + ) raise ValueError(msg) if self.error is not None and self.error_detail is None: msg = "error must be accompanied by error_detail" + logger.warning( + API_VALIDATION_FAILED, + reason=msg, + error=self.error, + error_detail_present=self.error_detail is not None, + ) raise ValueError(msg) return selfAs per coding guidelines, "All error paths must log at WARNING or ERROR with context before raising."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/budget.py` around lines 131 - 140, The model validator _validate_error_detail_consistency currently raises ValueError in two branches without logging; before each raise add a logging call (logger.error or logger.warning) that includes the same descriptive message and contextual fields (e.g., current self.error and self.error_detail) so the error path is logged prior to raising; ensure you use the module's standard logger (logger or logging.getLogger(__name__)) and log both branches ("error_detail requires error to be set" and "error must be accompanied by error_detail") with context.src/synthorg/api/controllers/meetings.py (2)
93-105: 🛠️ Refactor suggestion | 🟠 MajorUse
@computed_fieldhere, or make this a dedicated view model instead.
MeetingResponse.token_usage_by_participant,MeetingResponse.contribution_rank, andMeetingResponse.meeting_duration_secondsare stored as ordinary fields even though the implementation derives them in_to_meeting_response(). That duplicates state on top ofMeetingRecord; if the final payload really needs extra external data, this should be a separate response model rather thanMeetingRecordplus helper-populated redundant fields.Based on learnings, "Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 93 - 105, The MeetingResponse model currently defines token_usage_by_participant, contribution_rank, and meeting_duration_seconds as ordinary validated fields while their values are derived in _to_meeting_response(), causing redundant state; update MeetingResponse to declare these as Pydantic v2 computed fields using `@computed_field` on methods that return token_usage_by_participant, contribution_rank, and meeting_duration_seconds (or alternatively create a separate view model for the final payload and move these derived properties there), and remove their default Field(...) declarations so the model no longer duplicates MeetingRecord state.
124-139:⚠️ Potential issue | 🟠 MajorThe analytics still don't match the public response contract.
The PR objective says
token_usage_by_participantis keyed by participant name andmeeting_duration_secondsis only for completed meetings. This block still keys analytics byMeetingContribution.agent_idand emits them wheneverrecord.minutesexists, so a non-completed record with retained minutes would leak ID-based analytics that don't match the documented response. Gate this onrecord.status == MeetingStatus.COMPLETEDand resolve the outward keys to participant names here, or update the API/docs/types/tests to explicitly contract on IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/meetings.py` around lines 124 - 139, The analytics must be emitted only for completed meetings and keyed by participant name, not agent ID: change the block that computes usage/rank/duration to run only when record.status == MeetingStatus.COMPLETED and record.minutes is present; build a lookup from participant IDs to names (e.g. from record.participants) and aggregate token usage into usage_by_name using that map (fallback to ID if name missing), compute contribution_rank from that name-keyed map, and set meeting_duration_seconds only for completed meetings (otherwise return None or 0 per contract) before constructing MeetingResponse with token_usage_by_participant, contribution_rank, and meeting_duration_seconds.
🤖 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/controllers/test_meetings.py`:
- Around line 69-124: The test fixture _make_minutes currently sets
participant_ids=("participant-1",) while with_contributions=True creates
MeetingContribution entries for "agent-alpha" and "agent-beta", producing
inconsistent test data; update _make_minutes so participant_ids includes the
contributors (e.g., ("agent-alpha","agent-beta")) whenever with_contributions is
true (or adjust the contributions to match the participant_ids), and then update
the related assertions expecting analytics for agent-alpha and agent-beta;
ensure changes are applied to the other similar fixture usages mentioned (the
block around lines 365-384) so MeetingMinutes, MeetingContribution,
participant_ids, total_input_tokens and total_output_tokens remain consistent.
---
Duplicate comments:
In `@src/synthorg/api/controllers/budget.py`:
- Around line 131-140: The model validator _validate_error_detail_consistency
currently raises ValueError in two branches without logging; before each raise
add a logging call (logger.error or logger.warning) that includes the same
descriptive message and contextual fields (e.g., current self.error and
self.error_detail) so the error path is logged prior to raising; ensure you use
the module's standard logger (logger or logging.getLogger(__name__)) and log
both branches ("error_detail requires error to be set" and "error must be
accompanied by error_detail") with context.
In `@src/synthorg/api/controllers/meetings.py`:
- Around line 93-105: The MeetingResponse model currently defines
token_usage_by_participant, contribution_rank, and meeting_duration_seconds as
ordinary validated fields while their values are derived in
_to_meeting_response(), causing redundant state; update MeetingResponse to
declare these as Pydantic v2 computed fields using `@computed_field` on methods
that return token_usage_by_participant, contribution_rank, and
meeting_duration_seconds (or alternatively create a separate view model for the
final payload and move these derived properties there), and remove their default
Field(...) declarations so the model no longer duplicates MeetingRecord state.
- Around line 124-139: The analytics must be emitted only for completed meetings
and keyed by participant name, not agent ID: change the block that computes
usage/rank/duration to run only when record.status == MeetingStatus.COMPLETED
and record.minutes is present; build a lookup from participant IDs to names
(e.g. from record.participants) and aggregate token usage into usage_by_name
using that map (fallback to ID if name missing), compute contribution_rank from
that name-keyed map, and set meeting_duration_seconds only for completed
meetings (otherwise return None or 0 per contract) before constructing
MeetingResponse with token_usage_by_participant, contribution_rank, and
meeting_duration_seconds.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9006abc9-9c8b-4f6a-9768-1b4c48009cb0
📒 Files selected for processing (4)
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pytests/unit/api/controllers/test_meetings.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). (1)
- 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 with native lazy annotations
Useexcept A, B:syntax (no parentheses) per PEP 758 - ruff enforces this on Python 3.14
Files:
src/synthorg/api/controllers/meetings.pytests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions must have type hints. Type checking enforced by mypy in strict mode.
Google-style docstrings required on all public classes and functions, enforced by ruff D rules
Use Pydantic v2 conventions: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 (e.g., multiple tool invocations, parallel agent calls) over barecreate_task
Line length must be 88 characters (enforced by ruff)
Functions must be less than 50 lines, files less than 800 lines
Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Never useimport logging/logging.getLogger()/print()in application code (exceptobservability/setup.pyandobservability/sinks.pywhich may use stdlib logging and stderr print for bootstrap)
Always useloggeras the variable name (not_logger, notlog)
Always use event name constants from domain-specific modules undersynthorg.observability.events(e.g.,API_REQUEST_STARTEDfromevents.api). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT
Use structured logging: alwayslogger.info(EVENT, key=value)- never use printf-style formatting likelogger.info("msg %s", val)
All state transitions must log at INFO level
DEBUG logging is appropriate for object creation, internal flow, and entry/exit of key functions
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, etc. Vendor names only in: (1) Operations design page, (2).claude/skill files, (3) third-party import paths.
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every module with business logic MUST import:
from synthorg.observability import get_loggerand instantiatelogger = get_logger(__name__)
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/api/**/*.py: REST API defined with Litestar. Errors must use RFC 9457 format insrc/synthorg/api/
Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests must usetest-provider,test-small-001, etc. instead of real vendor names
Use@pytest.mark.parametrizefor testing similar cases
Mark all tests with appropriate pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Use Hypothesis for property-based testing in Python with@givenand@settingsdecorators. Run dev profile with 1000 examples viaHYPOTHESIS_PROFILE=devenvironment variable.
Never skip, dismiss, or ignore flaky tests. For timing-sensitive tests, mocktime.monotonic()andasyncio.sleep()to make them deterministic. For tasks blocking indefinitely, useasyncio.Event().wait()instead ofasyncio.sleep(large_number).
Files:
tests/unit/api/controllers/test_meetings.py
🧠 Learnings (28)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
tests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/synthorg/api/**/*.py : Litestar API must include setup wizard, auth/, auto-wiring, and lifecycle management
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T16:55:07.730Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T16:55:07.730Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : 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.
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to src/synthorg/**/*.py : 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, and key function entry/exit
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T06:43:14.114Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:43:14.114Z
Learning: Applies to src/synthorg/**/*.py : 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. Pure data models, enums, and re-exports do NOT need logging.
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Handle errors explicitly, never silently swallow them. All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T13:22:36.844Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T13:22:36.844Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. Use `computed_field` for derived values instead of storing redundant fields. Use `NotBlankStr` for all identifier/name fields.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
src/synthorg/api/controllers/approvals.py
🔇 Additional comments (20)
src/synthorg/api/controllers/budget.py (7)
3-31: LGTM!Imports are well-organized: standard library, framework, then internal modules. Observability setup follows project conventions with event constants and structured logger instantiation.
48-72: LGTM!
DailySummarycorrectly usesConfigDict(frozen=True)for immutability,NotBlankStrfor the date identifier, and appropriatege=0constraints for numeric fields.
74-105: LGTM!
PeriodSummaryproperly uses@computed_fieldfor derivedavg_cost_usdrather than storing redundant data. Division-by-zero is correctly guarded.
149-191: LGTM!
_build_summariescorrectly handles the empty-records edge case, usesmath.fsumfor accurate floating-point summation, and returns chronologically sorted daily summaries. Field access (cost_usd,timestamp,input_tokens,output_tokens) aligns withCostRecordmodel definition.
217-240: LGTM!Endpoint signature and docstring are well-defined. The return type correctly reflects the enriched response, and the docstring clarifies that summaries span all matching records, not just the current page.
241-253: LGTM!Validation correctly logs
API_VALIDATION_FAILEDat WARNING level with structured context before raisingApiValidationError. The inline comment documenting the Litestar limitation is helpful for future maintainers.
255-273: LGTM!The implementation correctly computes summaries from all filtered records before pagination, ensuring the response reflects complete aggregations regardless of page size. Logging at INFO level with structured kwargs aligns with the observability contract.
src/synthorg/api/controllers/approvals.py (13)
63-64: LGTM!The urgency threshold constants are clearly named and appropriately scoped as module-private. The values align with the spec (1 hour = 3600s for critical, 4 hours = 14400s for high).
67-78: LGTM!
UrgencyLevelenum correctly implements the specified thresholds with clear documentation. UsingStrEnumensures proper JSON serialization for API responses.
80-98: LGTM!The
ApprovalResponseDTO properly extendsApprovalItemwith urgency enrichment fields. Thefrozen=Trueconfig ensures immutability, and thege=0.0constraint onseconds_remainingenforces the clamping invariant at the type level.
100-130: LGTM!The conversion function correctly:
- Handles
Noneexpiry asNO_EXPIRY- Clamps negative remaining time to
0.0for expired items- Applies urgency thresholds in the correct order (critical < high < normal)
- Uses
model_dump()to spread base fields cleanly
186-188: LGTM!Exception handling uses the
except A, B:syntax per PEP 758 guidelines for Python 3.14, correctly re-raising critical errors while catching others for logging.
390-416: LGTM!The
_get_approval_or_404helper properly centralizes the lookup-and-404 pattern with correct observability (warning log with context before raising). This eliminates duplication acrossget_approval,approve, andrejectendpoints.
418-472: LGTM!The
_save_decision_and_notifyhelper cleanly consolidates the decision persistence workflow:
- Atomic
save_if_pendingwith conflict detection- Uses
saved(the persisted item) for event publishing (addressing prior feedback)- Proper logging before raising
ConflictError- Resume intent signaling with the committed state
504-526: LGTM!The manual validation is retained with clear documentation of the Litestar workaround rationale. Using a single
nowtimestamp for all items in the list ensures consistent urgency calculations within the response, which is the correct approach for list endpoints.
528-548: LGTM!The
get_approvalendpoint correctly uses the centralized_get_approval_or_404helper and enriches the response with urgency fields.
550-616: LGTM!The
create_approvalendpoint correctly uses the samenowtimestamp for both thecreated_atfield and the response enrichment, ensuring temporal consistency in the returned urgency values.
618-672: LGTM!The
approveendpoint correctly leverages the centralized helpers and maintains timestamp consistency between the decision timestamp and response enrichment.
674-728: LGTM!The
rejectendpoint follows the same well-structured pattern asapprove, correctly using the centralized helpers and maintaining timestamp consistency.
324-331: LGTM!The updated event constant
APPROVAL_GATE_REVIEW_TRANSITION_FAILEDaligns with the domain-specific event naming convention and provides better observability categorization.
_make_minutes now sets participant_ids to ("agent-alpha", "agent-beta")
when with_contributions=True, matching the actual contributor agent IDs
instead of using a mismatched ("participant-1",).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
25d5247 to
593d8e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/src/api/endpoints/budget.ts (1)
36-47:⚠️ Potential issue | 🟡 MinorThe success path still trusts the payload shape.
This fixes
error_detail, but Line 42 through Line 47 still assumedata,pagination,daily_summary, andperiod_summaryexist wheneversuccessis truthy. A malformed or version-skewed success body will now throw aTypeErrorinstead of the usualApiRequestError.🛡️ Minimal guard
const response = await apiClient.get<CostRecordListResponseBody>('/budget/records', { params }) const body = response.data - if (!body?.success) { - throw new ApiRequestError(body?.error ?? 'Unknown API error', body?.error_detail ?? null) + if (!body || typeof body !== 'object') { + throw new ApiRequestError('Unknown API error') + } + if (!body.success) { + throw new ApiRequestError(body.error ?? 'Unknown API error', body.error_detail ?? null) + } + if ( + !Array.isArray(body.data) || + !body.pagination || + typeof body.pagination.total !== 'number' || + typeof body.pagination.offset !== 'number' || + typeof body.pagination.limit !== 'number' || + !Array.isArray(body.daily_summary) || + !body.period_summary + ) { + throw new ApiRequestError('Unexpected API response format') } return { data: body.data,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/endpoints/budget.ts` around lines 36 - 47, The success branch trusts the returned body shape and can throw a TypeError if fields are missing; guard the response before returning by validating required properties on body (e.g., data, pagination.total/offset/limit, daily_summary, period_summary) after apiClient.get<CostRecordListResponseBody> and before the return, and if any required field is absent or malformed throw an ApiRequestError with a clear message (use body?.error_detail if present); update the logic around the body variable and the return block to perform these checks so malformed success payloads produce ApiRequestError instead of runtime TypeError.src/synthorg/api/controllers/budget.py (1)
131-139:⚠️ Potential issue | 🟠 MajorValidator failures still raise without a warning/error log.
Line 134 and Line 137 are explicit error paths, but they still raise
ValueErrorsilently. That makes malformedCostRecordListResponseconstruction invisible in the normal observability stream.🛡️ Minimal fix
`@model_validator`(mode="after") def _validate_error_detail_consistency(self) -> Self: """Ensure ``error`` and ``error_detail`` are set together.""" if self.error_detail is not None and self.error is None: msg = "error_detail requires error to be set" + logger.warning( + API_VALIDATION_FAILED, + message=msg, + error_present=self.error is not None, + error_detail_present=self.error_detail is not None, + ) raise ValueError(msg) if self.error is not None and self.error_detail is None: msg = "error must be accompanied by error_detail" + logger.warning( + API_VALIDATION_FAILED, + message=msg, + error_present=self.error is not None, + error_detail_present=self.error_detail is not None, + ) raise ValueError(msg) return selfAs per coding guidelines, "All error paths must log at WARNING or ERROR with context before raising."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/synthorg/api/controllers/budget.py` around lines 131 - 139, The validator _validate_error_detail_consistency on CostRecordListResponse raises ValueError on inconsistent error/error_detail but does not log; before each raise, call the module/class logger (e.g., logger.warning or logger.error) with a clear contextual message and include the offending field values (self.error, self.error_detail) so the condition is observable, then raise the same ValueError as before; update both branches (the "error_detail requires error to be set" and "error must be accompanied by error_detail") to perform the log first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/operations.md`:
- Around line 1006-1017: Remove the empty line immediately after the admonition
header `!!! info "Approval API Response Enrichment"` in
docs/design/operations.md so the paragraph following the header is not treated
as an indented code block; ensure the first line of the description starts
directly after the admonition header with no blank line to satisfy markdownlint
MD046.
---
Duplicate comments:
In `@src/synthorg/api/controllers/budget.py`:
- Around line 131-139: The validator _validate_error_detail_consistency on
CostRecordListResponse raises ValueError on inconsistent error/error_detail but
does not log; before each raise, call the module/class logger (e.g.,
logger.warning or logger.error) with a clear contextual message and include the
offending field values (self.error, self.error_detail) so the condition is
observable, then raise the same ValueError as before; update both branches (the
"error_detail requires error to be set" and "error must be accompanied by
error_detail") to perform the log first.
In `@web/src/api/endpoints/budget.ts`:
- Around line 36-47: The success branch trusts the returned body shape and can
throw a TypeError if fields are missing; guard the response before returning by
validating required properties on body (e.g., data,
pagination.total/offset/limit, daily_summary, period_summary) after
apiClient.get<CostRecordListResponseBody> and before the return, and if any
required field is absent or malformed throw an ApiRequestError with a clear
message (use body?.error_detail if present); update the logic around the body
variable and the return block to perform these checks so malformed success
payloads produce ApiRequestError instead of runtime TypeError.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b8947d9-9150-4d2b-8d96-deb1cc27b393
📒 Files selected for processing (14)
docs/design/communication.mddocs/design/operations.mdsrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/observability/events/api.pysrc/synthorg/observability/events/approval_gate.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_meetings.pyweb/src/api/endpoints/approvals.tsweb/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.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). (2)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Backend
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations-- Python 3.14 has PEP 649.
Use PEP 758 except syntax: useexcept A, B:(no parentheses) -- ruff enforces this on Python 3.14.
Line length must be 88 characters (enforced by ruff).
Python code must pass ruff check and format (line length 88). Enforced by pre-commit and CI.
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pytests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pytests/unit/api/controllers/test_meetings.pysrc/synthorg/api/controllers/budget.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Type hints required on all public functions; mypy strict mode enforced.
Docstrings must use Google style and are required on public classes/functions (enforced by ruff D rules).
Create new objects instead of mutating existing ones. For non-Pydantic internal collections, use copy.deepcopy() at construction + MappingProxyType wrapping. For dict/list fields in frozen Pydantic models, rely on frozen=True and use copy.deepcopy() at system boundaries.
Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.
Use Pydantic v2 conventions: BaseModel, model_validator, computed_field, ConfigDict. Use@computed_fieldfor derived values instead of storing redundant fields. Use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Prefer asyncio.TaskGroup for fan-out/fan-in parallel operations in new code. Prefer structured concurrency over bare create_task. Existing code being migrated incrementally.
Functions must be less than 50 lines; files must be less than 800 lines.
Handle errors explicitly; never silently swallow errors.
Validate at system boundaries (user input, external APIs, config files).
Python type checking with mypy strict mode. Enforced by pre-push and CI.
API reference auto-generated via mkdocstrings + Griffe (AST-based from Python docstrings).
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Every module with business logic MUST import logger viafrom synthorg.observability import get_loggerthen assignlogger = get_logger(__name__).
Never useimport logging,logging.getLogger(), orprint()in application code (exception: observability/setup.py and observability/sinks.py may use stdlib logging and print for bootstrap code).
Logger variable must always be namedlogger(not_logger, notlog).
Always use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly:from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Use structured logging: 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 level.
Use DEBUG level for object creation, internal flow, and entry/exit of key functions.
Pure data models, enums, and re-exports do NOT need logging.
All provider calls go through BaseCompletionProvider which applies retry + rate limiting automatically. Never implement retry logic in driver subclasses or calling code.
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, or large/medium/small. Vendor names allowed only in: Operations design page, .claude/ files, third-party import paths.
Files:
src/synthorg/observability/events/approval_gate.pysrc/synthorg/observability/events/api.pysrc/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Docs built with Zensical (Markdown). Config: mkdocs.yml. Design spec: docs/design/ (10 pages). Architecture: docs/architecture/. Roadmap: docs/roadmap/. Generate OpenAPI reference: scripts/export_openapi.py.
Files:
docs/design/communication.mddocs/design/operations.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowfor test classification.
Prefer@pytest.mark.parametrizefor testing similar cases.
Tests must use test-provider, test-small-001, etc. instead of vendor-specific names.
Use Hypothesis for property-based testing in Python (@given+@settings). Use fast-check in React (fc.assert + fc.property). Use native testing.F fuzz functions in Go (Fuzz*). Hypothesis profiles: ci (50 examples, default) and dev (1000 examples) via HYPOTHESIS_PROFILE env var.
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 deterministic. For tasks that must block indefinitely, use asyncio.Event().wait() instead of asyncio.sleep(large_number).
Files:
tests/unit/api/controllers/test_budget.pytests/unit/api/controllers/test_approvals.pytests/unit/api/controllers/test_meetings.py
web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/**/*.{ts,tsx}: Use ESLint in React dashboard. Run withnpm --prefix web run lint.
React TypeScript type checking vianpm --prefix web run type-check.
Files:
web/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.tsweb/src/api/types.tsweb/src/api/endpoints/approvals.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
React 19 dashboard with react-router config and auth/setup guards. Uses Zustand stores for state (auth, WebSocket, domain shells). Uses
@tanstack/react-queryfor data fetching.
Files:
web/src/api/endpoints/budget.tsweb/src/api/endpoints/meetings.tsweb/src/api/types.tsweb/src/api/endpoints/approvals.ts
🧠 Learnings (45)
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
src/synthorg/observability/events/approval_gate.pydocs/design/operations.mdsrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
docs/design/operations.mdweb/src/api/endpoints/budget.tssrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
docs/design/operations.mdtests/unit/api/controllers/test_budget.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
docs/design/operations.mdtests/unit/api/controllers/test_budget.pyweb/src/api/endpoints/budget.tsweb/src/api/types.tssrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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.py
📚 Learning: 2026-03-18T21:23:23.586Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-18T21:23:23.586Z
Learning: Applies to src/synthorg/**/*.py : Event names: always use constants from the domain-specific module under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly from synthorg.observability.events.<domain>.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/observability/**/*.py : Observability package (observability/): structured logging, correlation tracking, log sinks; event constants organized by domain under observability/events/ (e.g., events.api, events.tool, events.git, events.context_budget, events.backup)
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-19T11:33:01.580Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T11:33:01.580Z
Learning: Applies to src/synthorg/**/*.py : Use event constants from `synthorg.observability.events.<domain>` (e.g., `API_REQUEST_STARTED` from `events.api`); import directly and log with structured kwargs: `logger.info(EVENT, key=value)`, never interpolated strings
Applied to files:
src/synthorg/observability/events/api.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from domain-specific modules under synthorg.observability.events (e.g., API_REQUEST_STARTED from events.api, TOOL_INVOKE_START from events.tool). Import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T15:43:05.601Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T15:43:05.601Z
Learning: Applies to src/**/*.py : Use event name constants from domain-specific modules under ai_company.observability.events (e.g., PROVIDER_CALL_START from events.provider, BUDGET_RECORD_ADDED from events.budget, etc.) — import directly
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use event name constants from synthorg.observability.events domain-specific modules (e.g., PROVIDER_CALL_START from events.provider). Import directly: from synthorg.observability.events.<domain> import EVENT_CONSTANT.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`). Import directly: `from ai_company.observability.events.<domain> import EVENT_CONSTANT`.
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
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`); import directly: `from synthorg.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T06:24:56.341Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T06:24:56.341Z
Learning: Applies to src/synthorg/**/*.py : Always use event name constants from the domain-specific module under `synthorg.observability.events` in logging calls
Applied to files:
src/synthorg/observability/events/api.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
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 instead of widening timing margins
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
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 deterministic. For tasks that must block indefinitely, use asyncio.Event().wait() instead of asyncio.sleep(large_number).
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to tests/**/*.py : Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
Learning: Applies to tests/**/*.py : Parametrize: Prefer pytest.mark.parametrize for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Prefer `pytest.mark.parametrize` for testing similar cases.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to tests/**/*.py : Test markers: `pytest.mark.unit`, `pytest.mark.integration`, `pytest.mark.e2e`, `pytest.mark.slow`. Coverage: 80% minimum. Async: `asyncio_mode = 'auto'` — no manual `pytest.mark.asyncio` needed. Timeout: 30 seconds per test. Parallelism: `pytest-xdist` via `-n auto` — ALWAYS include `-n auto` when running pytest, never run tests sequentially.
Applied to files:
tests/unit/api/controllers/test_approvals.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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/controllers/test_approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/meetings.pysrc/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/api/**/*.py : API package (api/): Litestar REST + WebSocket with controllers, guards, channels, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint, provider management endpoint (CRUD + test + presets), backup endpoint, RFC 9457 structured errors, AppState hot-reload slots, service auto-wiring (Phase 1 at construction, Phase 2 on startup), lifecycle helpers
Applied to files:
src/synthorg/api/controllers/approvals.pytests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/api/**/*.py : REST API: Litestar framework, controllers with guards, channels for WebSocket, JWT + API key + WS ticket auth, approval gate integration, coordination endpoint, collaboration endpoint, settings endpoint. RFC 9457 structured errors (ErrorCategory, ErrorCode, ErrorDetail, ProblemDetail, CATEGORY_TITLES, category_title, category_type_uri, content negotiation).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T11:41:02.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T11:41:02.964Z
Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to src/**/*.py : Use Pydantic v2 conventions: BaseModel, model_validator, computed_field, ConfigDict. Use computed_field for derived values instead of storing redundant fields. Use NotBlankStr for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 conventions: `BaseModel`, `model_validator`, `computed_field`, `ConfigDict`. For derived values use `computed_field` instead of storing + validating redundant fields. Use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-16T20:14:00.937Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T20:14:00.937Z
Learning: Applies to **/*.py : Validate: at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to **/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to src/**/*.py : Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-15T21:49:53.264Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T21:49:53.264Z
Learning: Fix everything valid — never skip when review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes). No deferring, no 'out of scope' skipping.
Applied to files:
src/synthorg/api/controllers/approvals.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : 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.
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Applies to src/synthorg/**/*.py : Handle errors explicitly, never silently swallow. Validate at system boundaries (user input, external APIs, config files).
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-14T16:18:57.267Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:18:57.267Z
Learning: Applies to src/ai_company/!(observability)/**/*.py : All error paths must log at WARNING or ERROR with context before raising.
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-16T07:22:28.134Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T07:22:28.134Z
Learning: Applies to src/synthorg/**/*.py : 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, and key function entry/exit
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-17T06:43:14.114Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:43:14.114Z
Learning: Applies to src/synthorg/**/*.py : 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. Pure data models, enums, and re-exports do NOT need logging.
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
src/synthorg/api/controllers/approvals.pysrc/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/communication/**/*.py : Communication package (communication/): message bus, dispatcher, messenger, channels, delegation, loop prevention, conflict resolution; meeting/ subpackage for meeting protocol (round-robin, position papers, structured phases), scheduler (frequency, participant resolver), orchestrator
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Package structure: src/synthorg/ organized as: api/ (REST+WebSocket, Litestar), auth/ (auth subpackage), backup/ (scheduled/manual backups), budget/ (cost tracking, CFO), cli/ (superseded by Go CLI), communication/ (message bus, meetings), config/ (YAML loading), core/ (domain models, resilience config), engine/ (orchestration, task state, coordination, approval gates, stagnation detection, context budget, compaction), hr/ (hiring, performance, promotion), memory/ (pluggable backend, Mem0, retrieval, consolidation), persistence/ (operational data, SQLite, settings), observability/ (logging, correlation, sinks), providers/ (LLM abstraction, LiteLLM, auth types, presets, runtime CRUD), settings/ (runtime-editable, typed definitions, encryption, config bridge), security/ (SecOps, rule engine, output scanning, progressive trust, autonomy levels), templates/ (company templates, personalities), tools/ (registry, built-in tools, git, sandbox, code_runner, MCP...
Applied to files:
tests/unit/api/controllers/test_meetings.py
📚 Learning: 2026-03-19T07:12:14.508Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:12:14.508Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-26T15:16:19.520Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-26T15:16:19.520Z
Learning: Applies to src/synthorg/**/*.py : Use structured logging: always `logger.info(EVENT, key=value)` -- never `logger.info("msg %s", val)`.
Applied to files:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:28:13.207Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:28:13.207Z
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:
src/synthorg/api/controllers/budget.py
📚 Learning: 2026-03-15T18:38:44.202Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T18:38:44.202Z
Learning: Applies to src/synthorg/**/*.py : Always log with structured kwargs: `logger.info(EVENT, key=value)` — never use old-style formatting `logger.info("msg %s", val)`
Applied to files:
src/synthorg/api/controllers/budget.py
🪛 markdownlint-cli2 (0.22.0)
docs/design/operations.md
[warning] 1008-1008: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (18)
src/synthorg/api/controllers/budget.py (1)
221-253: The Litestar Parameter(max_length=...) annotation does not crash the worker in version 2.21.1.The comment at line 241 claims
Parameter(max_length=...)crashes the worker, but Litestar 2.21.1 handles this gracefully by returning a proper RFC 9457–compliant 400 Bad Request response. The manual validation at lines 244+ serves a different purpose: it provides custom error messaging and structured logging with context (field name, actual length, max length), rather than preventing a crash. The annotation and manual validation can coexist without issue.> Likely an incorrect or invalid review comment.tests/unit/api/controllers/test_approvals.py (2)
424-583: Well-structured urgency field tests with proper time mocking.The new
TestApprovalUrgencyFieldsclass follows best practices:
- Uses
@pytest.mark.parametrizefor TTL/urgency variants (lines 451-458, 546-551)- Freezes time via
patch("synthorg.api.controllers.approvals.datetime")for boundary tests (lines 573-577)- Covers all endpoints (list, get, create, approve, reject) for urgency field presence
The boundary test properly mocks the clock the urgency calculation reads, making assertions deterministic.
585-609: LGTM — expired approval clamping test.The test correctly verifies that expired approvals return
seconds_remaining == 0.0andurgency_level == "critical". Direct_itemsinsertion (line 601) is appropriate here to bypass normal validation that would reject already-expired items.web/src/api/types.ts (3)
32-32: LGTM —UrgencyLeveltype correctly defined.The union type matches the backend
UrgencyLevelenum values:'critical' | 'high' | 'normal' | 'no_expiry'.
279-282: LGTM —ApprovalResponsecorrectly extendsApprovalItem.The extension adds the computed urgency fields while preserving all base fields, matching the backend DTO structure.
388-402: LGTM — Budget and meeting analytics DTOs.
DailySummary,PeriodSummary, andMeetingResponsetypes correctly match the backend response enrichments documented in PR objectives.Also applies to: 765-769
web/src/api/endpoints/approvals.ts (1)
1-35: LGTM — Approval endpoint types updated to use enriched response.All five endpoints (
listApprovals,getApproval,createApproval,approveApproval,rejectApproval) now correctly returnApprovalResponsewhich includes the computedseconds_remainingandurgency_levelfields.tests/unit/api/controllers/test_meetings.py (3)
69-129: Well-structured meeting minutes fixture with consistent participant data.The
_make_minuteshelper now correctly:
- Sets
participant_idsto match contributor agent IDs whenwith_contributions=True(line 113-115)- Computes
total_input_tokens/total_output_tokensfrom contributions (lines 110-111)- Sets deterministic duration (120 seconds via
started_at = now - timedelta(seconds=120))This addresses the previous concern about inconsistent participant fixture data.
287-304: Concrete analytics assertions in trigger response test.The test now asserts specific values (
{},[],120.0) rather than just key presence, ensuring regressions to empty defaults or wrong durations will be caught.
354-429: Comprehensive analytics test coverage.The
TestMeetingAnalyticsclass provides thorough coverage:
- Token usage aggregation with exact expected values (lines 377-379)
- Contribution rank ordering verification (line 388)
- Duration computation for completed meetings (line 396)
- Empty analytics for non-completed meetings (lines 404-406)
- Edge case: completed meeting with no contributions (lines 419-429)
src/synthorg/api/controllers/meetings.py (3)
81-105: LGTM —MeetingResponsewith analytics fields.The response DTO correctly:
- Extends
MeetingRecordwith computed analytics- Uses
frozen=Truefor immutability- Applies
ge=0.0constraint onmeeting_duration_seconds- Documents the behavior clearly in the docstring
108-140: Clean analytics computation in_to_meeting_response.The helper correctly:
- Aggregates tokens per participant (input + output)
- Sorts by total tokens descending for
contribution_rank- Clamps duration to non-negative (line 133)
- Returns empty analytics when
minutesisNone
175-186: Manual validation retained with justification.The comment at lines 175-177 explains why manual validation is kept despite
Parameter(max_length=...): Litestar crashes the worker instead of returning RFC 9457 errors. The validation now logsAPI_VALIDATION_FAILEDwith field context before raising.src/synthorg/api/controllers/approvals.py (5)
63-77: LGTM —UrgencyLevelenum with correct thresholds.The enum and threshold constants correctly implement the PR specification:
critical< 1 hour (3600 seconds)high< 4 hours (14400 seconds)normal>= 4 hoursno_expirywhen no TTL set
100-129: Clean urgency computation in_to_approval_response.The helper correctly:
- Returns
None/NO_EXPIRYwhenexpires_atis unset- Clamps
seconds_remainingto>= 0.0for expired items- Applies threshold logic in the correct order (critical → high → normal)
390-471: Well-factored helper functions reduce duplication.
_get_approval_or_404and_save_decision_and_notifyconsolidate repeated patterns from approve/reject endpoints:
- Centralized not-found logging and error raising
- Single place for decision persistence, event publishing, logging, and resume signaling
- Proper conflict handling with logging before raising
504-515: Manual validation with observability — justified.The comment at lines 504-506 explains the Litestar crash issue. The validation now logs
API_VALIDATION_FAILEDwith field context (field,actual_length,max_length) before raising, satisfying the observability requirement.
80-97: 🧹 Nitpick | 🔵 TrivialFactory approach for urgency fields is acceptable.
While
@computed_fieldwould be more idiomatic for derived values, the current factory approach withfrozen=Trueis pragmatic given:
- Time-dependent computation requires a reference timestamp
- Pydantic
@computed_fielddoesn't support external parameters elegantly- Frozen config prevents mutation after construction
Consider documenting this design decision in the docstring if the team agrees on this pattern for time-dependent derived fields.
⛔ Skipped due to learnings
Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-26T15:16:19.520Z Learning: Applies to src/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-20T08:28:32.845Z Learning: Applies to **/*.py : Config vs runtime state: use frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state. Never mix static config fields with mutable runtime fields in one model.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T19:14:27.144Z Learning: Applies to src/synthorg/**/*.py : Use frozen Pydantic models for config/identity; use separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-16T23:05:29.577Z Learning: Applies to **/*.py : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T18:38:44.202Z Learning: Applies to **/*.py : Config vs runtime state: frozen Pydantic models for config/identity; separate mutable-via-copy models (using model_copy(update=...)) for runtime state that evolves. Never mix static config fields with mutable runtime fields in one model.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T19:14:27.144Z Learning: Applies to src/synthorg/**/*.py : For dict/list fields in frozen Pydantic models, rely on frozen=True for field reassignment prevention and copy.deepcopy() at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence).Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T19:14:27.144Z Learning: Applies to **/*.py : Models: Pydantic v2 (BaseModel, model_validator, computed_field, ConfigDict). Use computed_field for derived values instead of storing + validating redundant fields. Use NotBlankStr (from core.types) for all identifier/name fields — including optional (NotBlankStr | None) and tuple (tuple[NotBlankStr, ...]) variants — instead of manual whitespace validators.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-17T11:41:02.964Z Learning: Applies to src/**/*.py : Models: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Use `computed_field` for derived values instead of storing + validating redundant fields. Use `NotBlankStr` for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators.Learnt from: CR Repo: Aureliolo/synthorg PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-15T19:14:27.144Z Learning: Applies to src/synthorg/**/*.py : Use Pydantic v2 with adopted conventions: use computed_field for derived values instead of storing + validating redundant fields; use NotBlankStr from core.types for all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@docs/design/operations.md`:
- Around line 1010-1013: The admonition block is being parsed as a code block
(MD046) because the list is separated/indented from the paragraph; fix by
tightening spacing so the bullet list begins immediately after the paragraph
line without an intervening blank line or extra indentation. Specifically,
ensure the lines for the list items referencing `seconds_remaining` and
`urgency_level` start at the same indentation as the paragraph/admonition marker
(no leading code-block indentation), so the bullets are recognized as a list
rather than an indented code block.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: debb369f-2469-49df-a48e-2dd2e450566e
📒 Files selected for processing (1)
docs/design/operations.md
📜 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). (1)
- GitHub Check: Test (Python 3.14)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Docs built with Zensical (Markdown). Config: mkdocs.yml. Design spec: docs/design/ (10 pages). Architecture: docs/architecture/. Roadmap: docs/roadmap/. Generate OpenAPI reference: scripts/export_openapi.py.
Files:
docs/design/operations.md
🧠 Learnings (4)
📚 Learning: 2026-03-17T22:08:13.456Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T22:08:13.456Z
Learning: Budget: Cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError).
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-17T06:30:14.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T06:30:14.180Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget tracking includes pre-flight/in-flight checks, auto-downgrade, billing periods, cost tiers, quota/subscription. CFO includes anomaly detection, efficiency analysis, downgrade recommendations.
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-19T07:13:44.964Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-19T07:13:44.964Z
Learning: Applies to src/synthorg/budget/**/*.py : Budget package (budget/): cost tracking, budget enforcement (pre-flight/in-flight checks, auto-downgrade), billing periods, cost tiers, quota/subscription tracking, CFO cost optimization (anomaly detection, efficiency analysis, downgrade recommendations, approval decisions), spending reports, budget errors (BudgetExhaustedError, DailyLimitExceededError, QuotaExhaustedError)
Applied to files:
docs/design/operations.md
📚 Learning: 2026-03-15T19:14:27.144Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T19:14:27.144Z
Learning: Applies to src/synthorg/api/**/*.py : Authentication uses JWT + API key. Approval gate integration for high-risk operations.
Applied to files:
docs/design/operations.md
🪛 markdownlint-cli2 (0.22.0)
docs/design/operations.md
[warning] 1011-1011: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔇 Additional comments (1)
docs/design/operations.md (1)
212-218: Accurate contract update for budget summaries.This section correctly states that
daily_summaryandperiod_summaryare computed from all matching records (not page-limited), matching the controller flow and aggregation behavior.
|
|
||
| - **`seconds_remaining`** (`float | null`): seconds until `expires_at`, clamped | ||
| to 0.0 for expired items; `null` when no TTL is set. | ||
| - **`urgency_level`** (enum): `critical` (< 1 hr), `high` (< 4 hrs), |
There was a problem hiding this comment.
Fix MD046 warning in the admonition list block.
The list under this admonition is still being parsed as an indented code block by markdownlint (MD046). Tighten the spacing so the list starts directly after the paragraph line.
📝 Minimal formatting fix
!!! info "Approval API Response Enrichment"
The approval REST API enriches every `ApprovalItem` response with computed
urgency fields so the dashboard can display time-sensitive indicators without
client-side computation:
-
- **`seconds_remaining`** (`float | null`): seconds until `expires_at`, clamped
to 0.0 for expired items; `null` when no TTL is set.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **`seconds_remaining`** (`float | null`): seconds until `expires_at`, clamped | |
| to 0.0 for expired items; `null` when no TTL is set. | |
| - **`urgency_level`** (enum): `critical` (< 1 hr), `high` (< 4 hrs), | |
| - **`seconds_remaining`** (`float | null`): seconds until `expires_at`, clamped | |
| to 0.0 for expired items; `null` when no TTL is set. | |
| - **`urgency_level`** (enum): `critical` (< 1 hr), `high` (< 4 hrs), |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1011-1011: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/operations.md` around lines 1010 - 1013, The admonition block is
being parsed as a code block (MD046) because the list is separated/indented from
the paragraph; fix by tightening spacing so the bullet list begins immediately
after the paragraph line without an intervening blank line or extra indentation.
Specifically, ensure the lines for the list items referencing
`seconds_remaining` and `urgency_level` start at the same indentation as the
paragraph/admonition marker (no leading code-block indentation), so the bullets
are recognized as a list rather than an indented code block.
🤖 I have created a release *beep* *boop* --- #MAJOR CHANGES; We got a somewhat working webui :) ## [0.5.0](v0.4.9...v0.5.0) (2026-03-30) ### Features * add analytics trends and budget forecast API endpoints ([#798](#798)) ([16b61f5](16b61f5)) * add department policies to default templates ([#852](#852)) ([7a41548](7a41548)) * add remaining activity event types (task_started, tool_used, delegation, cost_incurred) ([#832](#832)) ([4252fac](4252fac)) * agent performance, activity, and history API endpoints ([#811](#811)) ([9b75c1d](9b75c1d)) * Agent Profiles and Detail pages (biography, career, performance) ([#874](#874)) ([62d7880](62d7880)) * app shell, Storybook, and CI/CD pipeline ([#819](#819)) ([d4dde90](d4dde90)) * Approvals page with risk grouping, urgency indicators, batch actions ([#889](#889)) ([4e9673d](4e9673d)) * Budget Panel page (P&L dashboard, breakdown charts, forecast) ([#890](#890)) ([b63b0f1](b63b0f1)) * build infrastructure layer (API client, auth, WebSocket) ([#815](#815)) ([9f01d3e](9f01d3e)) * CLI global options infrastructure, UI modes, exit codes, env vars ([#891](#891)) ([fef4fc5](fef4fc5)) * CodeMirror editor and theme preferences toggle ([#905](#905), [#807](#807)) ([#909](#909)) ([41fbedc](41fbedc)) * Company page (department/agent management) ([#888](#888)) ([cfb88b0](cfb88b0)) * comprehensive hint coverage across all CLI commands ([#900](#900)) ([937974e](937974e)) * config system extensions, per-command flags for init/start/stop/status/logs ([#895](#895)) ([32f83fe](32f83fe)) * configurable currency system replacing hardcoded USD ([#854](#854)) ([b372551](b372551)) * Dashboard page (metric cards, activity feed, budget burn) ([#861](#861)) ([7d519d5](7d519d5)) * department health, provider status, and activity feed endpoints ([#818](#818)) ([6d5f196](6d5f196)) * design tokens and core UI components ([#833](#833)) ([ed887f2](ed887f2)) * extend approval, meeting, and budget API responses ([#834](#834)) ([31472bf](31472bf)) * frontend polish -- real-time UX, accessibility, responsive, performance ([#790](#790), [#792](#792), [#791](#791), [#793](#793)) ([#917](#917)) ([f04a537](f04a537)) * implement human roles and access control levels ([#856](#856)) ([d6d8a06](d6d8a06)) * implement semantic conflict detection in workspace merge ([#860](#860)) ([d97283b](d97283b)) * interaction components and animation patterns ([#853](#853)) ([82d4b01](82d4b01)) * Login page + first-run bootstrap + Company page ([#789](#789), [#888](#888)) ([#896](#896)) ([8758e8d](8758e8d)) * Meetings page with timeline viz, token bars, contribution formatting ([#788](#788)) ([#904](#904)) ([b207f46](b207f46)) * Messages page with threading, channel badges, sender indicators ([#787](#787)) ([#903](#903)) ([28293ad](28293ad)) * Org Chart force-directed view and drag-drop reassignment ([#872](#872), [#873](#873)) ([#912](#912)) ([a68a938](a68a938)) * Org Chart page (living nodes, status, CRUD, department health) ([#870](#870)) ([0acbdae](0acbdae)) * per-command flags for remaining commands, auto-behavior wiring, help/discoverability ([#897](#897)) ([3f7afa2](3f7afa2)) * Providers page with backend rework -- health, CRUD, subscription auth ([#893](#893)) ([9f8dd98](9f8dd98)) * scaffold React + Vite + TypeScript + Tailwind project ([#799](#799)) ([bd151aa](bd151aa)) * Settings page with search, dependency indicators, grouped rendering ([#784](#784)) ([#902](#902)) ([a7b9870](a7b9870)) * Setup Wizard rebuild with template comparison, cost estimator, theme customization ([#879](#879)) ([ae8b50b](ae8b50b)) * setup wizard UX -- template filters, card metadata, provider form reuse ([#910](#910)) ([7f04676](7f04676)) * setup wizard UX overhaul -- mode choice, step reorder, provider fixes ([#907](#907)) ([ee964c4](ee964c4)) * structured ModelRequirement in template agent configs ([#795](#795)) ([7433548](7433548)) * Task Board page (rich Kanban, filtering, dependency viz) ([#871](#871)) ([04a19b0](04a19b0)) ### Bug Fixes * align frontend types with backend and debounce WS refetches ([#916](#916)) ([134c11b](134c11b)) * auto-cleanup targets newly pulled images instead of old ones ([#884](#884)) ([50e6591](50e6591)) * correct wipe backup-skip flow and harden error handling ([#808](#808)) ([c05860f](c05860f)) * improve provider setup in wizard, subscription auth, dashboard bugs ([#914](#914)) ([87bf8e6](87bf8e6)) * improve update channel detection and add config get command ([#814](#814)) ([6b137f0](6b137f0)) * resolve all ESLint warnings, add zero-warnings enforcement ([#899](#899)) ([079b46a](079b46a)) * subscription auth uses api_key, base URL optional for cloud providers ([#915](#915)) ([f0098dd](f0098dd)) ### Refactoring * semantic analyzer cleanup -- shared filtering, concurrency, extraction ([#908](#908)) ([81372bf](81372bf)) ### Documentation * brand identity and UX design system from [#765](#765) exploration ([#804](#804)) ([389a9f4](389a9f4)) * page structure and information architecture for v0.5.0 dashboard ([#809](#809)) ([f8d6d4a](f8d6d4a)) * write UX design guidelines with WCAG-verified color system ([#816](#816)) ([4a4594e](4a4594e)) ### Tests * add unit tests for agent hooks and page components ([#875](#875)) ([#901](#901)) ([1d81546](1d81546)) ### CI/CD * bump actions/deploy-pages from 4.0.5 to 5.0.0 in the major group ([#831](#831)) ([01c19de](01c19de)) * bump astral-sh/setup-uv from 7.6.0 to 8.0.0 in /.github/actions/setup-python-uv in the all group ([#920](#920)) ([5f6ba54](5f6ba54)) * bump codecov/codecov-action from 5.5.3 to 6.0.0 in the major group ([#868](#868)) ([f22a181](f22a181)) * bump github/codeql-action from 4.34.1 to 4.35.0 in the all group ([#883](#883)) ([87a4890](87a4890)) * bump sigstore/cosign-installer from 4.1.0 to 4.1.1 in the minor-and-patch group ([#830](#830)) ([7a69050](7a69050)) * bump the all group with 3 updates ([#923](#923)) ([ff27c8e](ff27c8e)) * bump wrangler from 4.76.0 to 4.77.0 in /.github in the minor-and-patch group ([#822](#822)) ([07d43eb](07d43eb)) * bump wrangler from 4.77.0 to 4.78.0 in /.github in the all group ([#882](#882)) ([f84118d](f84118d)) ### Maintenance * add design system enforcement hook and component inventory ([#846](#846)) ([15abc43](15abc43)) * add dev-only auth bypass for frontend testing ([#885](#885)) ([6cdcd8a](6cdcd8a)) * add pre-push rebase check hook ([#855](#855)) ([b637a04](b637a04)) * backend hardening -- eviction/size-caps and model validation ([#911](#911)) ([81253d9](81253d9)) * bump axios from 1.13.6 to 1.14.0 in /web in the all group across 1 directory ([#922](#922)) ([b1b0232](b1b0232)) * bump brace-expansion from 5.0.4 to 5.0.5 in /web ([#862](#862)) ([ba4a565](ba4a565)) * bump eslint-plugin-react-refresh from 0.4.26 to 0.5.2 in /web ([#801](#801)) ([7574bb5](7574bb5)) * bump faker from 40.11.0 to 40.11.1 in the minor-and-patch group ([#803](#803)) ([14d322e](14d322e)) * bump https://github.com/astral-sh/ruff-pre-commit from v0.15.7 to 0.15.8 ([#864](#864)) ([f52901e](f52901e)) * bump nginxinc/nginx-unprivileged from `6582a34` to `f99cc61` in /docker/web in the all group ([#919](#919)) ([df85e4f](df85e4f)) * bump nginxinc/nginx-unprivileged from `ccbac1a` to `6582a34` in /docker/web ([#800](#800)) ([f4e9450](f4e9450)) * bump node from `44bcbf4` to `71be405` in /docker/sandbox ([#827](#827)) ([91bec67](91bec67)) * bump node from `5209bca` to `cf38e1f` in /docker/web ([#863](#863)) ([66d6043](66d6043)) * bump picomatch in /site ([#842](#842)) ([5f20bcc](5f20bcc)) * bump recharts 2->3 and @types/node 22->25 in /web ([#802](#802)) ([a908800](a908800)) * Bump requests from 2.32.5 to 2.33.0 ([#843](#843)) ([41daf69](41daf69)) * bump smol-toml from 1.6.0 to 1.6.1 in /site ([#826](#826)) ([3e5dbe4](3e5dbe4)) * bump the all group with 3 updates ([#921](#921)) ([7bace0b](7bace0b)) * bump the minor-and-patch group across 1 directory with 2 updates ([#829](#829)) ([93e611f](93e611f)) * bump the minor-and-patch group across 1 directory with 3 updates ([#841](#841)) ([7010c8e](7010c8e)) * bump the minor-and-patch group across 1 directory with 3 updates ([#869](#869)) ([548cee5](548cee5)) * bump the minor-and-patch group in /site with 2 updates ([#865](#865)) ([9558101](9558101)) * bump the minor-and-patch group with 2 updates ([#867](#867)) ([4830706](4830706)) * consolidate Dependabot groups to 1 PR per ecosystem ([06d2556](06d2556)) * consolidate Dependabot groups to 1 PR per ecosystem ([#881](#881)) ([06d2556](06d2556)) * improve worktree skill with full dep sync and status enhancements ([#906](#906)) ([772c625](772c625)) * remove Vue remnants and document framework decision ([#851](#851)) ([bf2adf6](bf2adf6)) * update web dependencies and fix brace-expansion CVE ([#880](#880)) ([a7a0ed6](a7a0ed6)) * upgrade to Storybook 10 and TypeScript 6 ([#845](#845)) ([52d95f2](52d95f2)) --- 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
seconds_remaining(computed fromexpires_at - now) andurgency_levelenum (critical/high/normal/no_expiry) on all 5 endpointstoken_usage_by_participant,contribution_rank(sorted desc), andmeeting_duration_secondson all 3 endpointsGET /recordsreturnsCostRecordListResponsewithdaily_summary(per-day aggregation) andperiod_summary(avg cost, total tokens) computed from all matching recordsApprovalResponse,MeetingResponse,UrgencyLevel,DailySummary,PeriodSummaryTest plan
Review coverage
Pre-reviewed by 9 agents (code-reviewer, type-design-analyzer, test-analyzer, comment-analyzer, conventions-enforcer, logging-audit, api-contract-drift, docs-consistency, issue-resolution-verifier). 17 findings addressed:
Closes #774
🤖 Generated with Claude Code