Feat/infrastructure refactor#13
Merged
Merged
Conversation
Extract app/infrastructure/ (lifespan, middleware, routing, MCP setup, request context, errors), app/config/ (settings, constants), and app/modules/ packages. Split monolithic pm_tools.py into per-domain files (owner, tenant, lease, rent, maintenance, dashboard). Add flatmates admin endpoints and schemas. Expand AI provider resilience with retry backoff and SSE helper. Add social enum constraints migration and comprehensive MCP user-tools tests.
…ler shutdown, MCP fixes, tests - Convert SSE bus emit() to async across flatmates, visits, share preview - Release main-pool DB session before streaming in agent_chat; use AsyncSessionLocalBG - Use public shutdown APIs for schedulers instead of private _scheduler attr - Add exc_info=True to scheduler error logs for full tracebacks - Fix MCP lease tools: status filter, rent payment params, backward-compat message key - Add lease termination fields + migration - Escape redirect URL in tour share preview for JS safety - Remove defensive getattr patterns in visits endpoint - Add config_overrides ignored warning in AI provider cache - Add process_result_value tests for EnumStringType - Update repo-contract.json and ADRs
…e .gitignore - Add blog SEO auto-compute fields (meta_title, meta_description, etc.) - Refactor AI agent service with improved tool orchestration - Remove obsolete populate_data scripts and seed_flatmates scripts - Update .gitignore for .playwright-mcp, supabase/.temp, local screenshots - Add supabase migrations for blog SEO and property location auto-populate - Add blog image acquisition script and viral-blog-publisher skill - Add blog model and schema unit tests - Update config, SSE, MCP utils, PM authz across codebase
…isons, agent cleanup, migration fixes - SSE: replace immediate reaping of full queues with a configurable full-cycle threshold before eviction - Rent status: query each unpaid status separately instead of passing a list to list_rent_charges - Conversations: use enum members directly instead of .value for status/source comparisons and assignments - AI agent: remove unused agent cache and partial_text from _AgentRunError, emit conversation_info only on first fallback - MCP servers: type _require_auth as NoReturn - Migrations: validate enum constraints, fix trigger function volatility to VOLATILE - Models: add PaymentMethod enum, fix EnumStringType fallback to return raw string - Tests: update rent status tests for per-status query pattern - Cleanup: remove unused imports and variables
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoInfrastructure refactor with admin moderation, PM tools consolidation, and AI provider fallback chain
WalkthroughsDescription**Major infrastructure refactoring with comprehensive feature additions and improvements:** • **Infrastructure Modularization**: Extracted application factory concerns into dedicated modules (infrastructure.lifespan, infrastructure.middleware, infrastructure.errors, infrastructure.mcp, infrastructure.routing) for better separation of concerns and maintainability • **Admin Moderation System**: Added comprehensive flatmates admin endpoints for listing approval/rejection workflows, user report moderation (suspension, warning, dismissal), and pre-screening with SSE real-time event support • **Property Management Tools Expansion**: Split and refactored PM tools into specialized modules (pm_lease_tools, pm_rent_tools, pm_maintenance_tools, pm_tenant_tools) with centralized tool_ops layer for consistent error handling and response formatting • **MCP User Tools Refactoring**: Migrated owner, booking, and tenant tools to use centralized tool_ops module, improving code reusability and reducing duplication • **AI Provider Fallback Chain**: Implemented multi-provider fallback strategy (GLM → Gemini → Groq) with streaming refactor for robust AI agent operations • **Authorization Centralization**: Extracted inline authorization logic into pm_authz service module for bookings and visits, reducing code duplication across endpoints • **Database & Connection Pooling**: Added serverless mode support with NullPool, background pool sessions for long-running operations, and optimized connection lifecycle management • **Blog System Enhancements**: Added SEO analytics (auto-generated titles, descriptions, reading time, word count), source citations, and blog publishing/acquisition scripts with image recovery • **Image Processing Optimization**: Refactored to single-open pattern for thumbnails, WebP conversion, and metadata extraction; fixed deprecated PIL methods • **Enum Type Safety**: Implemented EnumStringType SQLAlchemy decorator with database-level check constraints for type-safe enum storage • **Conversation & Matching**: Added SSE event emissions for messages and conversation updates, Q&A persistence with user match creation • **Configuration Updates**: Environment-based .env file selection, AI provider fallback configuration, image API keys (Pixabay, Pexels), serverless deployment flags • **Security & Rate Limiting**: Added per-IP rate limiting for tour interactions, JSON escaping for redirect URLs, enhanced authentication logging • **Comprehensive Test Coverage**: New unit tests for MCP user tools, flatmates admin endpoints, blog schemas, and social models with auth/error handling validation • **Type Hint Modernization**: Updated codebase to PEP 585 style (dict/list instead of Dict/List, T | None instead of Optional[T]) Diagramflowchart LR
A["Factory"] -->|"extract concerns"| B["Infrastructure<br/>Modules"]
B --> B1["lifespan"]
B --> B2["middleware"]
B --> B3["errors"]
B --> B4["mcp"]
B --> B5["routing"]
C["PM Tools"] -->|"split & centralize"| D["tool_ops<br/>Layer"]
D --> D1["lease_tools"]
D --> D2["rent_tools"]
D --> D3["maintenance_tools"]
D --> D4["tenant_tools"]
E["AI Agents"] -->|"implement fallback"| F["GLM → Gemini → Groq"]
G["Endpoints"] -->|"centralize authz"| H["pm_authz<br/>Service"]
I["Database"] -->|"optimize pooling"| J["Serverless +<br/>Background Pool"]
K["Blog System"] -->|"enhance"| L["SEO Analytics +<br/>Image Acquisition"]
File Changes1. tests/unit/mcp/test_user_tools.py
|
Code Review by Qodo
1. app/modules placeholder package added
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 183 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".factory/skills/qa-backend/SKILL.md">
<violation number="1" location=".factory/skills/qa-backend/SKILL.md:222">
P2: The new seeding command points to a non-existent script path, so the documented recovery step cannot be executed.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:238">
P2: README setup commands reference missing `seed_data` scripts, so the documented data-loading flow will fail.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
| 5. **AI provider timeouts.** Vastu analysis and AI agent calls may take 10-30 seconds. Use `--max-time 60` with curl for these endpoints. | ||
|
|
||
| 6. **Empty database.** Property search returns empty if no data is loaded. Run `uv run python populate_data/load_comprehensive_data.py --quick` to seed test data. | ||
| 6. **Empty database.** Property search returns empty if no data is loaded. Run `uv run python seed_data/01_load_all.py --only hardcoded,seed` to seed test data. |
There was a problem hiding this comment.
P2: The new seeding command points to a non-existent script path, so the documented recovery step cannot be executed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .factory/skills/qa-backend/SKILL.md, line 222:
<comment>The new seeding command points to a non-existent script path, so the documented recovery step cannot be executed.</comment>
<file context>
@@ -219,7 +219,7 @@ Authorization: Bearer <supabase-jwt-token>
5. **AI provider timeouts.** Vastu analysis and AI agent calls may take 10-30 seconds. Use `--max-time 60` with curl for these endpoints.
-6. **Empty database.** Property search returns empty if no data is loaded. Run `uv run python populate_data/load_comprehensive_data.py --quick` to seed test data.
+6. **Empty database.** Property search returns empty if no data is loaded. Run `uv run python seed_data/01_load_all.py --only hardcoded,seed` to seed test data.
7. **MCP tool auth challenge format.** MCP tools requiring auth return a 401 with `WWW-Authenticate: Bearer resource_metadata="..."` header. This is expected -- not a failure.
</file context>
| # Quick load (~51 properties) | ||
| uv run python populate_data/load_comprehensive_data.py --quick | ||
| # Load all seed data (hardcoded + generated) | ||
| uv run python seed_data/01_load_all.py |
There was a problem hiding this comment.
P2: README setup commands reference missing seed_data scripts, so the documented data-loading flow will fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 238:
<comment>README setup commands reference missing `seed_data` scripts, so the documented data-loading flow will fail.</comment>
<file context>
@@ -229,11 +234,15 @@ All endpoints are prefixed with `/api/v1`.
- # Quick load (~51 properties)
- uv run python populate_data/load_comprehensive_data.py --quick
+ # Load all seed data (hardcoded + generated)
+ uv run python seed_data/01_load_all.py
- # Full load (~300 properties)
</file context>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+139
to
+143
| # Snapshot IDs needed after the session is released | ||
| conversation_id = conversation.id | ||
|
|
||
| # Release the main-pool session — streaming may take minutes | ||
| await db.close() |
There was a problem hiding this comment.
🔴 User message and new conversation rolled back by premature db.close() in authenticated chat endpoint
In the authenticated /chat endpoint, conversation_store.add_message(db, ...) flushes the user's message to the main-pool session at line 127, and get_or_create_conversation(db, ...) may create a new conversation at line 116. Then at line 143, await db.close() is called, which closes the session and causes the connection pool to roll back the uncommitted transaction — discarding both the user message and any newly created conversation.
After streaming completes, the persistence code at app/api/api_v1/endpoints/agent_chat.py:183 opens a fresh_db session but only persists widget events and the assistant response — the user message is never re-persisted.
Impact by scenario
Existing conversation (body.conversation_id provided): The conversation row exists in the DB, so conversation_id is valid. The assistant response is persisted, but the user's question is permanently lost. The conversation history is incomplete.
New conversation (no body.conversation_id): The conversation was created on db and rolled back. conversation_id references a non-existent row. The add_message(fresh_db, conversation_id=conversation_id, ...) call at line 195 fails with a foreign key violation, which is caught and logged at line 200. Neither the user message nor the assistant response is persisted.
The old code (before this PR) kept db open throughout streaming and committed everything (user message + assistant response) together in the finally block. The new code splits sessions but fails to commit or re-persist the user message before closing the first session.
Prompt for agents
The bug is that the user message and (potentially new) conversation are flushed to the main-pool DB session but never committed before db.close() rolls them back at line 143. The post-streaming persistence code on fresh_db only persists widget events and assistant response, not the user message.
To fix this, you need to commit the main-pool session before closing it, so the user message and conversation are persisted. Add await db.commit() before await db.close() at line 143 in app/api/api_v1/endpoints/agent_chat.py. This ensures the user message (added at line 127-131) and the conversation (created or updated at line 116-121) are committed to the database before the session is released.
Alternatively, you could re-persist the user message on fresh_db after streaming, but committing before close is simpler and preserves the original semantics where the user message is guaranteed to be stored even if streaming fails.
Was this helpful? React with 👍 or 👎 to provide feedback.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+11
| """Domain modules placeholder. | ||
|
|
||
| Domain code will be migrated here as part of the long-term architecture | ||
| evolution. Currently, domain code lives in the legacy locations: | ||
| - app/api/ — REST endpoints | ||
| - app/services/ — Business logic | ||
| - app/models/ — ORM models | ||
| - app/schemas/ — Pydantic schemas | ||
| - app/repositories/— Data access | ||
| - app/mcp/ — MCP tools | ||
| """ |
There was a problem hiding this comment.
1. app/modules placeholder package added 📘 Rule violation ⚙ Maintainability
New placeholder packages were introduced under reserved directories (app/modules/ and app/shared/) despite no actual domain migration or real usage occurring. This violates the repository policy against shim/placeholder packages in reserved locations and risks architectural drift and confusing/unused structure.
Agent Prompt
## Issue description
The PR introduces placeholder/shim packages under reserved directories (`app/modules/` and `app/shared/`), which is not allowed unless there is an actual domain migration or concrete shared usage.
## Issue Context
PR Compliance ID 15 forbids using reserved directories like `app/modules/` and `app/shared/` for shim-only re-exports or placeholder entrypoints. The added `__init__.py` files are explicitly placeholders (and in the case of `app/shared/`, a redirect), rather than real migrated/shared code.
## Fix Focus Areas
- app/modules/__init__.py[1-11]
- app/shared/__init__.py[1-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
234
to
238
| model_config = SettingsConfigDict( | ||
| env_file=str(BASE_DIR / ".env"), | ||
| env_file=str(BASE_DIR / _ENV_FILE), | ||
| case_sensitive=True, | ||
| extra="ignore", | ||
| ) |
There was a problem hiding this comment.
2. Env file ignored 🐞 Bug ☼ Reliability
SettingsConfigDict now loads env vars from .env.dev/.env.test/.env.prod based on the OS ENVIRONMENT and no longer loads .env, but README/CLAUDE still instruct creating .env, causing the app to start with missing/default config. Because the env-file choice is made before env-file parsing, setting ENVIRONMENT inside .env cannot work (bootstrapping deadlock).
Agent Prompt
## Issue description
`app/core/config.py` selects an environment-specific env file (`.env.dev`, `.env.test`, `.env.prod`) using `os.getenv("ENVIRONMENT")` and then configures Pydantic settings to load only that file. This breaks the documented setup (`cp .env.example .env`) and prevents `ENVIRONMENT` from being configured inside the env file itself.
## Issue Context
- Docs currently instruct copying `.env.example` to `.env`.
- The code defaults to `.env.dev` unless `ENVIRONMENT` is set in the OS environment, so `.env` is effectively ignored.
## Fix Focus Areas
- app/core/config.py[9-16]
- app/core/config.py[234-238]
- README.md[219-223]
- CLAUDE.md[66-67]
## Proposed fix
- Update settings loading to always include `.env` as a base file (backward compatible), and optionally overlay the env-specific file.
- Example: `env_file=(str(BASE_DIR / ".env"), str(BASE_DIR / _ENV_FILE))` (or a list/tuple depending on pydantic-settings support).
- Alternatively/additionally support an explicit `ENV_FILE` OS variable to override.
- Align docs with the intended behavior (either keep `.env` as the primary path, or update README/CLAUDE to instruct `.env.dev` etc., but avoid the bootstrapping trap where `ENVIRONMENT` must be set before env loading).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
2 issues found across 327 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/api/api_v1/endpoints/public.py">
<violation number="1" location="app/api/api_v1/endpoints/public.py:48">
P2: The periodic cleanup only deletes empty buckets, so inactive IP entries with expired timestamps are retained indefinitely and the rate-limit map can grow without bound.</violation>
<violation number="2" location="app/api/api_v1/endpoints/public.py:380">
P1: Rate limiting uses a spoofable IP source (`x-forwarded-for`), allowing clients to evade the new like/unlike limits by sending forged headers.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
| For production, consider storing likes per user/session to prevent abuse. | ||
| Rate-limited per IP to prevent count manipulation. | ||
| """ | ||
| client_ip = get_client_ip(request) |
There was a problem hiding this comment.
P1: Rate limiting uses a spoofable IP source (x-forwarded-for), allowing clients to evade the new like/unlike limits by sending forged headers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/api_v1/endpoints/public.py, line 380:
<comment>Rate limiting uses a spoofable IP source (`x-forwarded-for`), allowing clients to evade the new like/unlike limits by sending forged headers.</comment>
<file context>
@@ -345,9 +375,14 @@ async def like_tour(
- For production, consider storing likes per user/session to prevent abuse.
+ Rate-limited per IP to prevent count manipulation.
"""
+ client_ip = get_client_ip(request)
+ if _is_like_rate_limited(client_ip):
+ raise HTTPException(
</file context>
| # Periodically reap keys with empty timestamp lists to prevent unbounded dict growth | ||
| if now - _last_like_reap > _LIKE_REAP_INTERVAL_S: | ||
| _last_like_reap = now | ||
| stale = [k for k, v in _ip_like_timestamps.items() if not v] |
There was a problem hiding this comment.
P2: The periodic cleanup only deletes empty buckets, so inactive IP entries with expired timestamps are retained indefinitely and the rate-limit map can grow without bound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/api_v1/endpoints/public.py, line 48:
<comment>The periodic cleanup only deletes empty buckets, so inactive IP entries with expired timestamps are retained indefinitely and the rate-limit map can grow without bound.</comment>
<file context>
@@ -21,6 +22,35 @@
+ # Periodically reap keys with empty timestamp lists to prevent unbounded dict growth
+ if now - _last_like_reap > _LIKE_REAP_INTERVAL_S:
+ _last_like_reap = now
+ stale = [k for k, v in _ip_like_timestamps.items() if not v]
+ for k in stale:
+ del _ip_like_timestamps[k]
</file context>
Suggested change
| stale = [k for k, v in _ip_like_timestamps.items() if not v] | |
| stale = [k for k, v in _ip_like_timestamps.items() if not v or v[-1] <= window_start] |
…ing, specific exception handling, CORS validation - Wrap owner_properties_list success response in MCPResponse.success() for consistent shape - Add structured error codes (NOT_FOUND, FORBIDDEN, OPERATION_FAILED, INVALID_INPUT) to tool_ops returns - Replace fragile string matching in MCP user layer with code-based error dispatch - Catch PropertyNotFoundException/InsufficientPermissionsError specifically in get_property_detail - Validate CORS_ORIGINS_STR entries must start with http:// or https:// - Add *.skill to .gitignore
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Refactored the app factory into
app/infrastructureand standardized config underapp.configto simplify wiring and enable serverless scale‑to‑zero. Adds Design Studio image generation, blog SEO fields with a viral publisher flow, structured MCP responses with stricter CORS, flatmates admin moderation, cleaner SSE streaming, and modernized type hints.New Features
NullPoolfor Postgres and in‑memory cache whenSERVERLESS_ENABLED=true; graceful scheduler shutdown.MCPResponse.success(), add structured error codes, code‑based error dispatch; validate CORS origins.app/api/api_v1/endpoints/flatmates_admin.py) with listing/report actions.viral-blog-publisherskill, andpublish_blog.pyfor direct inserts.pm_dashboard_tools,pm_lease_tools,pm_rent_tools,pm_maintenance_tools,pm_tenant_tools) with widget mapping.Migration
app.config(e.g.,from app.config import settings); legacyapp.core.configimports were updated.ENVIRONMENT→.env.dev|.env.test|.env.prod. Updated AI vars: newAI_AGENT_*andGROQ_*; defaults set toGLM_MODEL=glm-5v-turboandGEMINI_MODEL=gemini-3.1-flash-lite-preview.uv run python seed_data/01_load_all.py(replace old populate_data commands).SERVERLESS_ENABLED=trueto avoid persistent DB/cache connections.Written for commit acef90d. Summary will update on new commits.