feat: webhooks#168
Conversation
WalkthroughThis change introduces a comprehensive webhook subsystem. It adds database models, migrations, and API endpoints for managing webhook endpoints per workspace, including creation, listing, and deletion. Webhook events can now be published and delivered to registered endpoints, with event types and payloads defined. The queue and deriver subsystems are refactored to support work unit keys and webhook event processing. Extensive tests and documentation are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
participant QueueManager
participant Deriver
participant WebhookEndpoint
User->>API: POST /workspaces/{workspace}/webhooks (register webhook)
API->>DB: Validate workspace, create/list webhook endpoint
DB-->>API: Webhook endpoint created/listed
API-->>User: Endpoint info
Note over QueueManager,Deriver: (Later, when an event triggers)
Deriver->>WebhookEndpoint: publish_webhook_event(event)
WebhookEndpoint->>DB: Add webhook event to queue
QueueManager->>DB: Poll queue, claim work unit
QueueManager->>Deriver: process_webhook(payload)
Deriver->>WebhookEndpoint: deliver_webhook(db, payload)
WebhookEndpoint->>User: HTTP POST to registered webhook URLs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.py📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
src/main.py📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: the honcho project uses python 3.11 in dockerfile and requires python >=3.10 in pyproject.toml. the ...Applied to files:
📚 Learning: applies to src/models.py : feature flags on workspace, peer, and session levels...Applied to files:
📚 Learning: applies to src/main.py : global exception handlers defined in main.py...Applied to files:
📚 Learning: in the webhook settings implementation for plastic-labs/honcho, the field name max_workspace_limit i...Applied to files:
📚 Learning: the webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 we...Applied to files:
📚 Learning: rajat-ahuja1997 is comfortable exposing token counts in messagecreate schema as a public property wh...Applied to files:
📚 Learning: the `get_or_create_session` function in this codebase is designed to handle both session creation an...Applied to files:
📚 Learning: the codebase uses a layered error handling approach where the crud layer raises valueerror for busin...Applied to files:
🧬 Code Graph Analysis (1)src/schemas.py (1)
🪛 Ruff (0.12.2)src/schemas.py373-373: Avoid specifying long messages outside the exception class (TRY003) 377-377: Avoid specifying long messages outside the exception class (TRY003) 384-384: Abstract (TRY301) 384-384: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (2)
src/webhooks/events.py (1)
22-22: Remove trailing blank line.class WebhookEvent(BaseModel): type: WebhookEventType data: WebhookPayload workspace_name: str -src/routers/webhooks.py (1)
54-68: Add return type annotation.-async def delete_webhook_endpoint( +async def delete_webhook_endpoint( workspace_id: Annotated[str, Path(description="Workspace ID")], endpoint_id: Annotated[str, Path(description="Webhook endpoint ID")], jwt_params: JWTParams = Depends(require_auth()), db: AsyncSession = db, -): +) -> None:
♻️ Duplicate comments (2)
src/utils/shared_models.py (1)
11-11: Same dependency point as earlierSame
typing_extensionsimport assummarizer.py; the comment above applies.tests/bench/run_tests.py (1)
21-29: Import switch consistent – see earlier noteImporting
TypedDictfromtyping_extensionskeeps the bench runner aligned. Dependency check comment already supplied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.pre-commit-config.yaml(0 hunks)docs/v2/guides/mcp.mdx(1 hunks)migrations/versions/88b0fb10906f_add_webhooks_table.py(1 hunks)scripts/update_version.py(1 hunks)src/config.py(2 hunks)src/crud/__init__.py(2 hunks)src/crud/webhook.py(1 hunks)src/crud/workspace.py(2 hunks)src/main.py(3 hunks)src/models.py(4 hunks)src/routers/webhooks.py(1 hunks)src/schemas.py(2 hunks)src/utils/shared_models.py(1 hunks)src/utils/summarizer.py(1 hunks)src/webhooks/event_emitter.py(1 hunks)src/webhooks/events.py(1 hunks)src/webhooks/webhook_delivery.py(1 hunks)tests/bench/run_tests.py(1 hunks)tests/routes/test_webhooks.py(1 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/utils/shared_models.pysrc/utils/summarizer.pysrc/main.pysrc/crud/__init__.pysrc/crud/workspace.pysrc/webhooks/events.pysrc/webhooks/event_emitter.pysrc/crud/webhook.pysrc/schemas.pysrc/config.pysrc/models.pysrc/routers/webhooks.pysrc/webhooks/webhook_delivery.py
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/bench/run_tests.pytests/routes/test_webhooks.py
src/main.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Global exception handlers defined in main.py
Files:
src/main.py
src/models.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/models.py: Use explicit type hints with SQLAlchemy mapped_column annotations
All tables use text IDs (nanoid format) as primary keys
Composite foreign keys for multi-tenant relationships
Feature flags on workspace, peer, and session levels
JSONB metadata fields for extensibility
HNSW indexes for vector similarity search
Files:
src/models.py
src/{models,crud}.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Token counting on messages for usage tracking
Files:
src/models.py
src/routers/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
API routes must follow the pattern: /v1/{resource}/{id}/{action}
Files:
src/routers/webhooks.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
src/main.py (2)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/main.py : Global exception handlers defined in main.py
src/crud/__init__.py (2)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Learnt from: dr-frmr
PR: #131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The get_or_create_session function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.
src/crud/workspace.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/**/*.py : Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
migrations/versions/88b0fb10906f_add_webhooks_table.py (3)
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:36-77
Timestamp: 2025-06-19T14:07:32.309Z
Learning: The migration script migrations/versions/d429de0e5338_adopt_peer_paradigm.py properly handles foreign key constraints during table updates by explicitly dropping foreign key constraints before making schema changes, preventing constraint violations during the migration process.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:438-441
Timestamp: 2025-06-18T16:44:57.733Z
Learning: In the Honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not workspaces.id (integer primary key). This means that during migrations when workspaces.public_id is renamed to workspaces.id, the foreign key relationship with messages.app_id remains valid.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:188-194
Timestamp: 2025-06-18T15:58:51.202Z
Learning: In the migration file migrations/versions/d429de0e5338_adopt_peer_paradigm.py, the team has explicitly decided to accept SQL injection risks from f-string interpolation of schema names, despite the security concerns around DDL injection via the DATABASE_SCHEMA environment variable.
docs/v2/guides/mcp.mdx (1)
Learnt from: VVoruganti
PR: #115
File: README.md:393-396
Timestamp: 2025-05-29T16:27:13.808Z
Learning: CONTRIBUTING.md exists at the repository root in the Honcho project and contains a comprehensive contributing guide. Automated verification scripts can sometimes fail to detect existing files, so manual verification may be needed when users dispute automated findings.
src/schemas.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
src/models.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
tests/routes/test_webhooks.py (1)
Learnt from: Rajat-Ahuja1997
PR: #131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use @pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
🧬 Code Graph Analysis (5)
src/main.py (1)
src/webhooks/webhook_delivery.py (2)
start(30-42)stop(44-51)
src/crud/__init__.py (2)
src/crud/webhook.py (3)
create_webhook_endpoint(13-47)delete_webhook_endpoint(75-111)list_webhook_endpoints(50-72)src/routers/webhooks.py (3)
create_webhook_endpoint(22-36)delete_webhook_endpoint(55-67)list_webhook_endpoints(40-51)
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
migrations/utils.py (1)
index_exists(43-51)
src/schemas.py (1)
src/models.py (1)
WebhookEndpoint(409-428)
src/models.py (1)
src/schemas.py (1)
WebhookEndpoint(386-392)
🪛 Ruff (0.12.2)
src/crud/workspace.py
93-93: Avoid specifying long messages outside the exception class
(TRY003)
migrations/versions/88b0fb10906f_add_webhooks_table.py
65-65: Trailing comma missing
Add trailing comma
(COM812)
src/webhooks/event_emitter.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
20-20: Trailing comma missing
Add trailing comma
(COM812)
38-38: Do not catch blind exception: Exception
(BLE001)
39-39: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
39-39: Logging statement uses f-string
(G004)
42-42: Trailing comma missing
Add trailing comma
(COM812)
src/crud/webhook.py
45-45: Logging statement uses f-string
(G004)
45-45: Trailing comma missing
Add trailing comma
(COM812)
51-51: Trailing comma missing
Add trailing comma
(COM812)
67-67: Trailing comma missing
Add trailing comma
(COM812)
76-76: Trailing comma missing
Add trailing comma
(COM812)
102-104: Avoid specifying long messages outside the exception class
(TRY003)
103-103: Trailing comma missing
Add trailing comma
(COM812)
110-110: Logging statement uses f-string
(G004)
110-110: Trailing comma missing
Add trailing comma
(COM812)
src/schemas.py
368-368: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Avoid specifying long messages outside the exception class
(TRY003)
379-379: Abstract raise to an inner function
(TRY301)
379-379: Avoid specifying long messages outside the exception class
(TRY003)
src/models.py
413-413: Trailing comma missing
Add trailing comma
(COM812)
417-417: Trailing comma missing
Add trailing comma
(COM812)
tests/routes/test_webhooks.py
10-10: Missing return type annotation for public function test_create_webhook_endpoint
Add return type annotation: None
(ANN201)
11-11: Trailing comma missing
Add trailing comma
(COM812)
27-27: Missing return type annotation for public function test_create_webhook_endpoint_invalid_url
Add return type annotation: None
(ANN201)
28-28: Trailing comma missing
Add trailing comma
(COM812)
42-42: Missing return type annotation for public function test_create_webhook_endpoint_missing_workspace
Add return type annotation: None
(ANN201)
52-52: Missing return type annotation for public function test_list_webhook_endpoints_empty
Add return type annotation: None
(ANN201)
53-53: Trailing comma missing
Add trailing comma
(COM812)
62-62: Missing return type annotation for public function test_list_webhook_endpoints_with_data
Add return type annotation: None
(ANN201)
63-63: Trailing comma missing
Add trailing comma
(COM812)
94-94: Missing return type annotation for public function test_list_webhook_endpoints_missing_workspace
Add return type annotation: None
(ANN201)
101-101: Missing return type annotation for public function test_delete_webhook_endpoint
Add return type annotation: None
(ANN201)
102-102: Trailing comma missing
Add trailing comma
(COM812)
117-117: Trailing comma missing
Add trailing comma
(COM812)
128-128: Missing return type annotation for public function test_delete_webhook_endpoint_not_found
Add return type annotation: None
(ANN201)
129-129: Trailing comma missing
Add trailing comma
(COM812)
134-134: Trailing comma missing
Add trailing comma
(COM812)
141-141: Missing return type annotation for public function test_delete_webhook_endpoint_missing_workspace
Add return type annotation: None
(ANN201)
148-148: Missing return type annotation for public function test_multiple_endpoints_per_workspace
Add return type annotation: None
(ANN201)
149-149: Trailing comma missing
Add trailing comma
(COM812)
183-183: Trailing comma missing
Add trailing comma
(COM812)
src/routers/webhooks.py
22-22: Missing return type annotation for public function create_webhook_endpoint
(ANN201)
24-26: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
25-25: Trailing comma missing
Add trailing comma
(COM812)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
35-35: Avoid specifying long messages outside the exception class
(TRY003)
40-40: Missing return type annotation for public function list_webhook_endpoints
(ANN201)
42-42: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
42-42: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Missing return type annotation for public function delete_webhook_endpoint
Add return type annotation: None
(ANN201)
58-58: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
58-58: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
src/webhooks/webhook_delivery.py
34-34: Trailing comma missing
Add trailing comma
(COM812)
38-38: Logging statement uses f-string
(G004)
38-38: Trailing comma missing
Add trailing comma
(COM812)
42-42: Store a reference to the return value of asyncio.create_task
(RUF006)
64-64: Do not catch blind exception: Exception
(BLE001)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
65-65: Logging statement uses f-string
(G004)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Trailing comma missing
Add trailing comma
(COM812)
86-86: Unnecessary assignment to signature before return statement
Remove unnecessary assignment
(RET504)
103-103: Logging statement uses f-string
(G004)
103-103: Trailing comma missing
Add trailing comma
(COM812)
110-110: datetime.datetime.now() called without a tz argument
(DTZ005)
114-114: Trailing comma missing
Add trailing comma
(COM812)
121-121: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
121-121: Logging statement uses f-string
(G004)
136-136: Logging statement uses f-string
(G004)
136-136: Trailing comma missing
Add trailing comma
(COM812)
140-140: Logging statement uses f-string
(G004)
140-140: Trailing comma missing
Add trailing comma
(COM812)
144-146: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
145-145: Logging statement uses f-string
(G004)
145-145: Trailing comma missing
Add trailing comma
(COM812)
147-147: Do not catch blind exception: Exception
(BLE001)
148-150: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
149-149: Logging statement uses f-string
(G004)
149-149: Trailing comma missing
Add trailing comma
(COM812)
166-166: Consider moving this statement to an else block
(TRY300)
166-166: Unnecessary assignment to urls before return statement
Remove unnecessary assignment
(RET504)
167-167: Do not catch blind exception: Exception
(BLE001)
168-170: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
169-169: Logging statement uses f-string
(G004)
169-169: Trailing comma missing
Add trailing comma
(COM812)
🪛 LanguageTool
docs/v2/guides/mcp.mdx
[grammar] ~10-~10: Use correct spacing
Context: ...aude Desktop and navigate to custom MCP servers. If you don't have node installed you will need to do that. Claude Desktop or Claude Code can help! 2. Add Honcho to your Claude desktop config...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[typographical] ~16-~16: To join two clauses or set off examples, consider using an em dash.
Context: ...e a username for Honcho to refer to you as -- preferably what you want Claude to actua...
(QB_NEW_EN_DASH_RULE_EM)
[grammar] ~16-~16: Use correct spacing
Context: ...y what you want Claude to actually call you. json { "mcpServers": { "honcho": { "command": "npx", "args": [ "mcp-remote", "https://mcp.honcho.dev", "--header", "Authorization:${AUTH_HEADER}", "--header", "X-Honcho-User-Name:${USER_NAME}" ], "env": { "AUTH_HEADER": "Bearer <your-honcho-key>", "USER_NAME": "<your-name>" } } } } You may customize your assistant name an...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~39-~39: Use correct spacing
Context: ...tant name and/or workspace ID. Both are optional. json { "mcpServers": { "honcho": { "command": "npx", "args": [ "mcp-remote", "https://mcp.honcho.dev", "--header", "Authorization:${AUTH_HEADER}", "--header", "X-Honcho-User-Name:${USER_NAME}", "--header", "X-Honcho-Assistant-Name:${ASSISTANT_NAME}", "--header", "X-Honcho-Workspace-ID:${WORKSPACE_ID}" ], "env": { "AUTH_HEADER": "Bearer <your-honcho-key>", "USER_NAME": "<your-name>", "ASSISTANT_NAME": "<your-assistant-name>", "WORKSPACE_ID": "<your-custom-workspace-id>" } } } } 3. Restart the Claude Desktop app. Upon rel...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~69-~69: Correctly pair commas and coordinating conjunctions
Context: ...top app. Upon relaunch, it should start Honcho and the tools should be available! 4. ...
(QB_NEW_EN_OTHER_ERROR_IDS_14)
[grammar] ~69-~69: Use correct spacing
Context: ...ld start Honcho and the tools should be available! 4. Finally, Claude needs instructions on ho...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~71-~71: Use correct spacing
Context: ...ons.md) into the "Project Instructions" field. Claude should then query for insights be...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[style] ~73-~73: Consider using polite language here.
Context: ...s own memory with Honcho, feel free to let us know or mak...
(INSERT_PLEASE)
[style] ~73-~73: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1293 characters long)
Context: ...b.com/plastic-labs/honcho/tree/main/mcp)!
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (26)
scripts/update_version.py (1)
171-200:_clean_changelog_sections()refactor looks correct and clearerThe new merged-condition blocks keep the previous behaviour while reducing nesting, and the end-of-file section handling is still covered. No functional or performance regressions spotted.
src/crud/__init__.py (1)
40-44: LGTM! Webhook CRUD imports properly integrated.The webhook CRUD functions are correctly imported and exported following the established patterns in this module. The imports align with the functions available in
src/crud/webhook.pyand maintain proper alphabetical organization.Also applies to: 90-94
src/main.py (3)
27-27: LGTM! Webhook components properly imported.The webhook router and delivery service imports follow the established patterns and are correctly integrated into the application.
Also applies to: 31-31
106-113: LGTM! Proper async lifecycle management for webhook delivery service.The webhook delivery service is correctly integrated into the FastAPI lifespan context manager:
- Service starts before the application handles requests
- Service stops in the finally block ensuring cleanup even on exceptions
- Database engine disposal is properly deferred until after the webhook service stops
This follows best practices for async resource management.
161-161: LGTM! Webhook router properly included.The webhook router is correctly included with the
/v2prefix, maintaining consistency with other API routers.src/crud/workspace.py (1)
68-94: LGTM! Well-implemented workspace validation function.The
get_workspacefunction is properly implemented with:
- Clear function signature and Google-style docstring
- Correct database query pattern
- Appropriate exception handling with
ResourceNotFoundException- Proper return type annotation
This function provides essential workspace validation for webhook CRUD operations.
src/config.py (1)
239-244: LGTM! Webhook configuration properly implemented.The
WebhookSettingsclass follows established patterns:
- Inherits from
HonchoSettingswith appropriate configuration- Uses consistent env_prefix pattern (
WEBHOOKS_)- Optional fields with sensible defaults for webhook functionality
- Properly integrated into
AppSettingsusing default_factory patternThe configuration fields (
PROXY_URLandWEBHOOK_ENCRYPTION_KEY) are well-suited for webhook functionality.Also applies to: 276-276
migrations/versions/88b0fb10906f_add_webhooks_table.py (4)
27-50: LGTM! Well-designed webhook_endpoints table.The table schema is properly designed with:
- Appropriate column types and constraints
- Foreign key relationship to
workspaces.name- URL length constraint (2048 chars) that's reasonable for webhook URLs
- Proper timestamp column with timezone and server default
- Named check constraint following conventions
52-58: LGTM! Proper index for query performance.The index on
workspace_nameis well-placed to optimize workspace-scoped webhook endpoint queries, following the established naming convention.
61-74: LGTM! Safe downgrade implementation.The downgrade function properly uses the
index_existsutility to safely drop the index before removing the table, preventing errors if the migration is rolled back multiple times.
65-65: Minor style: Add trailing comma.Static analysis suggests adding a trailing comma for consistency with Python style guidelines.
- "webhook_endpoints", "idx_webhook_endpoints_workspace_lookup", inspector + "webhook_endpoints", "idx_webhook_endpoints_workspace_lookup", inspector,⛔ Skipped due to learnings
Learnt from: VVoruganti PR: plastic-labs/honcho#164 File: sdks/python/src/honcho/async_client/peer.py:136-136 Timestamp: 2025-07-21T15:53:20.965Z Learning: The honcho repository should have COM812 (trailing comma missing) rule disabled in the root pyproject.toml Ruff configuration by adding it to the existing ignore list: ignore = ["E501", "B008", "COM812"]. The maintainers do not want trailing comma suggestions in code reviews.Learnt from: Rajat-Ahuja1997 PR: plastic-labs/honcho#144 File: tests/integration/test_message_embeddings.py:128-187 Timestamp: 2025-06-26T18:35:46.478Z Learning: The honcho repository does not use trailing commas in function signatures, function calls, or object instantiation. This is consistently applied across the entire codebase including test files, and reviews should not suggest adding trailing commas.Learnt from: VVoruganti PR: plastic-labs/honcho#164 File: sdks/python/src/honcho/async_client/peer.py:136-136 Timestamp: 2025-07-21T15:53:20.965Z Learning: The honcho repository maintainers do not want trailing comma suggestions or complaints from static analysis tools like Ruff. The COM812 rule should be disabled in the Ruff configuration to avoid these suggestions in future reviews.src/webhooks/events.py (1)
6-21: LGTM! Well-structured webhook event models.The event type enum and Pydantic models provide a clean, type-safe foundation for the webhook event system. The use of a type alias for
WebhookPayloadis a good extensibility pattern.src/webhooks/event_emitter.py (1)
13-52: LGTM! Clean event emission design.The WebhookEventEmitter provides a clean async queue-based design for decoupling event generation from delivery. The singleton pattern is appropriate here.
src/routers/webhooks.py (1)
15-68: LGTM! Well-structured webhook API endpoints.The router follows FastAPI best practices with proper authentication, dependency injection, and clear endpoint structure. The consistent authentication pattern across all endpoints ensures proper access control.
src/crud/webhook.py (3)
13-47: LGTM! Well-structured CRUD function with proper error handling.The function correctly validates workspace existence, creates the webhook endpoint, and includes appropriate logging. The implementation follows the coding guidelines with proper async patterns and SQLAlchemy usage.
50-72: LGTM! Clean list function implementation.The function properly validates workspace existence and efficiently queries webhook endpoints using SQLAlchemy select statements. The conversion to schema objects is handled correctly.
75-111: LGTM! Proper deletion logic with comprehensive validation.The function correctly validates both workspace and endpoint existence before deletion, with appropriate error handling when the endpoint is not found. The implementation follows best practices for async database operations.
src/models.py (2)
408-429: LGTM! Well-designed WebhookEndpoint model.The WebhookEndpoint model follows all the established patterns in the codebase:
- Uses text ID with nanoid (consistent with other models)
- Proper foreign key constraint to workspace
- Appropriate URL length constraint (2048 chars)
- Index on workspace_name for efficient lookups
- Correct relationship definition
- Proper repr method for debugging
The implementation aligns perfectly with the coding guidelines for models.py.
93-93: LGTM! Proper bidirectional relationship setup.The webhook_endpoints relationship in the Workspace model correctly establishes the bidirectional relationship with the WebhookEndpoint model, following the established patterns in the codebase.
tests/routes/test_webhooks.py (7)
9-24: LGTM! Comprehensive test for webhook endpoint creation.This test properly validates all aspects of webhook endpoint creation including the response structure, field values, and status code. The test follows the established patterns in the codebase.
26-38: LGTM! Important validation test for invalid URLs.This test ensures that the URL validation is working correctly, preventing invalid URLs from being stored. The error message and type validation are properly tested.
41-48: LGTM! Proper error handling test for missing workspace.This test validates that the system correctly handles requests for non-existent workspaces with appropriate 404 responses and error messages.
51-91: LGTM! Thorough testing of listing functionality.These tests comprehensively cover the listing endpoint behavior:
- Empty state handling
- Multiple endpoints per workspace
- Proper response structure validation
The tests ensure the API works correctly in various scenarios.
93-97: LGTM! Consistent error handling test.This test ensures consistent error handling for listing operations when workspaces don't exist.
100-144: LGTM! Comprehensive deletion testing.These tests thoroughly cover the deletion functionality:
- Successful deletion with verification
- Error handling for non-existent endpoints
- Error handling for non-existent workspaces
The tests ensure proper cleanup and error responses.
147-191: LGTM! Excellent edge case testing.This test validates a crucial edge case - ensuring that workspaces can have multiple webhook endpoints and that deletion of one doesn't affect others. This is important for the webhook functionality and demonstrates thorough test planning.
| except Exception as e: | ||
| logger.error(f"Failed to emit webhook event {event_type}: {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling specificity.
Catching generic Exception can mask programming errors. Consider catching specific exceptions or at least logging the full traceback.
- except Exception as e:
- logger.error(f"Failed to emit webhook event {event_type}: {e}")
+ except Exception:
+ logger.exception(f"Failed to emit webhook event {event_type}")📝 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.
| except Exception as e: | |
| logger.error(f"Failed to emit webhook event {event_type}: {e}") | |
| except Exception: | |
| logger.exception(f"Failed to emit webhook event {event_type}") |
🧰 Tools
🪛 Ruff (0.12.2)
38-38: Do not catch blind exception: Exception
(BLE001)
39-39: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
39-39: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In src/webhooks/event_emitter.py around lines 38 to 40, the code catches a
generic Exception which can hide programming errors. Modify the exception
handling to catch more specific exceptions related to the webhook emission
process if possible. If specific exceptions are not known, at least log the full
traceback using the logging module's exception method to provide detailed error
context for debugging.
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
tests/routes/test_webhooks.py (1)
1-188: Consider adding authorization test cases.The test suite would benefit from tests that verify authorization behavior:
- Non-admin user trying to create webhook for different workspace
- Non-admin user trying to delete any webhook (should fail)
- Non-admin user listing webhooks with different workspace filter
Would you like me to help generate these authorization test cases?
♻️ Duplicate comments (8)
src/schemas.py (1)
359-384: Enhance URL validation and improve exception handling.The URL validation should block localhost and use custom exception types as previously suggested.
src/webhooks/webhook_delivery.py (4)
42-42: Store task reference to prevent premature garbage collection.The created task needs to be stored to prevent potential issues.
64-66: Use logging.exception for better debugging.Replace logger.error with logger.exception to automatically include stack traces.
110-110: Use timezone-aware datetime.Using datetime without timezone can cause issues in distributed systems.
124-142: Improve per-endpoint delivery logging.The current implementation logs aggregate results which makes debugging difficult when some endpoints succeed and others fail.
src/routers/webhooks.py (2)
22-28: Add return type annotation.-async def get_or_create_webhook_endpoint( +async def get_or_create_webhook_endpoint( webhook: schemas.WebhookEndpointCreate = Body( ..., description="Webhook endpoint parameters" ), jwt_params: JWTParams = Depends(require_auth()), db: AsyncSession = db, -): +) -> schemas.WebhookEndpoint:
43-47: Add return type annotation.-async def list_webhook_endpoints( +async def list_webhook_endpoints( workspace_name: str | None = None, jwt_params: JWTParams = Depends(require_auth()), db: AsyncSession = db, -): +) -> list[schemas.WebhookEndpoint]:src/models.py (1)
33-36: WebhookStatus enum is unused.The WebhookStatus enum is defined but not referenced in the WebhookEndpoint model or elsewhere. Consider either:
- Removing it until needed
- Adding a status field to WebhookEndpoint model
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
migrations/versions/88b0fb10906f_add_webhooks_table.py(1 hunks)src/config.py(2 hunks)src/crud/__init__.py(2 hunks)src/crud/webhook.py(1 hunks)src/models.py(4 hunks)src/routers/webhooks.py(1 hunks)src/schemas.py(2 hunks)src/webhooks/webhook_delivery.py(1 hunks)tests/routes/test_webhooks.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/crud/__init__.pysrc/config.pysrc/webhooks/webhook_delivery.pysrc/models.pysrc/schemas.pysrc/routers/webhooks.pysrc/crud/webhook.py
src/models.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/models.py: Use explicit type hints with SQLAlchemy mapped_column annotations
All tables use text IDs (nanoid format) as primary keys
Composite foreign keys for multi-tenant relationships
Feature flags on workspace, peer, and session levels
JSONB metadata fields for extensibility
HNSW indexes for vector similarity search
Files:
src/models.py
src/{models,crud}.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Token counting on messages for usage tracking
Files:
src/models.py
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/routes/test_webhooks.py
src/routers/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
API routes must follow the pattern: /v1/{resource}/{id}/{action}
Files:
src/routers/webhooks.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
src/crud/__init__.py (2)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Learnt from: dr-frmr
PR: #131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The get_or_create_session function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.
src/config.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
migrations/versions/88b0fb10906f_add_webhooks_table.py (4)
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:36-77
Timestamp: 2025-06-19T14:07:32.309Z
Learning: The migration script migrations/versions/d429de0e5338_adopt_peer_paradigm.py properly handles foreign key constraints during table updates by explicitly dropping foreign key constraints before making schema changes, preventing constraint violations during the migration process.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:438-441
Timestamp: 2025-06-18T16:44:57.733Z
Learning: In the Honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not workspaces.id (integer primary key). This means that during migrations when workspaces.public_id is renamed to workspaces.id, the foreign key relationship with messages.app_id remains valid.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:188-194
Timestamp: 2025-06-18T15:58:51.202Z
Learning: In the migration file migrations/versions/d429de0e5338_adopt_peer_paradigm.py, the team has explicitly decided to accept SQL injection risks from f-string interpolation of schema names, despite the security concerns around DDL injection via the DATABASE_SCHEMA environment variable.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:918-942
Timestamp: 2025-06-19T14:32:02.934Z
Learning: The queue and active_queue_sessions tables in the migration script migrations/versions/d429de0e5338_adopt_peer_paradigm.py are expected to remain small, so per-row updates are acceptable and performance optimizations like set-based updates are not necessary.
src/webhooks/webhook_delivery.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/**/*.py : Explicit error handling with appropriate exception types
src/models.py (1)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
tests/routes/test_webhooks.py (1)
Learnt from: Rajat-Ahuja1997
PR: #131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use @pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
src/schemas.py (2)
Learnt from: Rajat-Ahuja1997
PR: #131
File: src/crud.py:374-377
Timestamp: 2025-06-18T21:24:55.907Z
Learning: The codebase uses a layered error handling approach where the CRUD layer raises ValueError for business rule violations (like peer limits), and the API layer catches these ValueError exceptions and translates them to appropriate domain-specific exceptions (ValidationException for validation errors, ResourceNotFoundException for missing resources). This provides clean separation of concerns between business logic and HTTP semantics.
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
🧬 Code Graph Analysis (7)
src/crud/__init__.py (2)
src/crud/webhook.py (3)
delete_webhook_endpoint(89-113)get_or_create_webhook_endpoint(13-58)list_webhook_endpoints(61-86)src/routers/webhooks.py (3)
delete_webhook_endpoint(59-71)get_or_create_webhook_endpoint(22-39)list_webhook_endpoints(43-55)
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
migrations/utils.py (1)
index_exists(43-51)
src/webhooks/webhook_delivery.py (4)
src/dependencies.py (1)
tracked_db(32-61)src/webhooks/events.py (1)
WebhookEvent(18-21)src/webhooks/event_emitter.py (1)
get_event(46-48)src/crud/webhook.py (1)
list_webhook_endpoints(61-86)
src/models.py (1)
src/schemas.py (1)
WebhookEndpoint(387-393)
tests/routes/test_webhooks.py (3)
src/models.py (2)
Peer(111-142)Workspace(88-107)src/schemas.py (2)
Peer(81-92)Workspace(44-54)tests/conftest.py (2)
client(168-189)sample_data(235-253)
src/schemas.py (1)
src/models.py (1)
WebhookEndpoint(409-428)
src/routers/webhooks.py (5)
src/exceptions.py (1)
AuthenticationException(45-49)src/security.py (2)
JWTParams(30-62)require_auth(115-155)src/models.py (1)
WebhookEndpoint(409-428)src/schemas.py (2)
WebhookEndpoint(387-393)WebhookEndpointCreate(359-384)src/crud/webhook.py (3)
get_or_create_webhook_endpoint(13-58)list_webhook_endpoints(61-86)delete_webhook_endpoint(89-113)
🪛 Ruff (0.12.2)
migrations/versions/88b0fb10906f_add_webhooks_table.py
65-65: Trailing comma missing
Add trailing comma
(COM812)
src/webhooks/webhook_delivery.py
34-34: Trailing comma missing
Add trailing comma
(COM812)
38-38: Logging statement uses f-string
(G004)
38-38: Trailing comma missing
Add trailing comma
(COM812)
42-42: Store a reference to the return value of asyncio.create_task
(RUF006)
64-64: Do not catch blind exception: Exception
(BLE001)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
65-65: Logging statement uses f-string
(G004)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Trailing comma missing
Add trailing comma
(COM812)
86-86: Unnecessary assignment to signature before return statement
Remove unnecessary assignment
(RET504)
103-103: Logging statement uses f-string
(G004)
103-103: Trailing comma missing
Add trailing comma
(COM812)
110-110: datetime.datetime.now() called without a tz argument
(DTZ005)
114-114: Trailing comma missing
Add trailing comma
(COM812)
121-121: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
121-121: Logging statement uses f-string
(G004)
136-136: Logging statement uses f-string
(G004)
136-136: Trailing comma missing
Add trailing comma
(COM812)
140-140: Logging statement uses f-string
(G004)
140-140: Trailing comma missing
Add trailing comma
(COM812)
144-146: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
145-145: Logging statement uses f-string
(G004)
145-145: Trailing comma missing
Add trailing comma
(COM812)
147-147: Do not catch blind exception: Exception
(BLE001)
148-150: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
149-149: Logging statement uses f-string
(G004)
149-149: Trailing comma missing
Add trailing comma
(COM812)
171-171: Consider moving this statement to an else block
(TRY300)
171-171: Unnecessary assignment to urls before return statement
Remove unnecessary assignment
(RET504)
172-172: Do not catch blind exception: Exception
(BLE001)
173-175: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
174-174: Logging statement uses f-string
(G004)
174-174: Trailing comma missing
Add trailing comma
(COM812)
src/models.py
413-413: Trailing comma missing
Add trailing comma
(COM812)
417-417: Trailing comma missing
Add trailing comma
(COM812)
tests/routes/test_webhooks.py
10-10: Missing return type annotation for public function test_create_webhook_endpoint
Add return type annotation: None
(ANN201)
11-11: Trailing comma missing
Add trailing comma
(COM812)
30-30: Missing return type annotation for public function test_create_webhook_endpoint_invalid_url
Add return type annotation: None
(ANN201)
31-31: Trailing comma missing
Add trailing comma
(COM812)
48-48: Missing return type annotation for public function test_create_webhook_endpoint_missing_workspace
Add return type annotation: None
(ANN201)
61-61: Missing return type annotation for public function test_list_webhook_endpoints_with_data
Add return type annotation: None
(ANN201)
62-62: Trailing comma missing
Add trailing comma
(COM812)
102-102: Missing return type annotation for public function test_list_webhook_endpoints_missing_workspace
Add return type annotation: None
(ANN201)
109-109: Missing return type annotation for public function test_delete_webhook_endpoint
Add return type annotation: None
(ANN201)
110-110: Trailing comma missing
Add trailing comma
(COM812)
137-137: Missing return type annotation for public function test_delete_webhook_endpoint_not_found
Add return type annotation: None
(ANN201)
144-144: Missing return type annotation for public function test_multiple_endpoints_per_workspace
Add return type annotation: None
(ANN201)
145-145: Trailing comma missing
Add trailing comma
(COM812)
src/schemas.py
369-369: Avoid specifying long messages outside the exception class
(TRY003)
373-373: Avoid specifying long messages outside the exception class
(TRY003)
380-380: Abstract raise to an inner function
(TRY301)
380-380: Avoid specifying long messages outside the exception class
(TRY003)
src/routers/webhooks.py
22-22: Missing return type annotation for public function get_or_create_webhook_endpoint
(ANN201)
23-25: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
24-24: Trailing comma missing
Add trailing comma
(COM812)
26-26: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Missing return type annotation for public function list_webhook_endpoints
(ANN201)
45-45: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
45-45: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Missing return type annotation for public function delete_webhook_endpoint
Add return type annotation: None
(ANN201)
61-61: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
61-61: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
src/crud/webhook.py
40-40: Trailing comma missing
Add trailing comma
(COM812)
57-57: Logging statement uses f-string
(G004)
62-62: Trailing comma missing
Add trailing comma
(COM812)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Logging statement uses f-string
(G004)
🔇 Additional comments (4)
src/crud/__init__.py (1)
40-44: LGTM!The webhook CRUD functions are properly imported and exported following the established patterns in this module.
Also applies to: 90-93
src/config.py (1)
239-245: LGTM!The webhook settings follow the established configuration patterns with appropriate types, defaults, and environment variable prefix.
Also applies to: 277-277
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
26-75: LGTM with a minor formatting fix.The migration properly creates the webhook_endpoints table with appropriate constraints and indexes. The downgrade function correctly checks for index existence before dropping.
Apply this diff to fix the formatting:
if index_exists( - "webhook_endpoints", "idx_webhook_endpoints_workspace_lookup", inspector + "webhook_endpoints", "idx_webhook_endpoints_workspace_lookup", inspector, ):src/crud/webhook.py (1)
48-58: Enforce webhook limit in CRUD layer.The get_or_create function should check if the workspace has reached its webhook limit before creating a new endpoint. This aligns with the PR objective to limit the maximum number of webhooks a user can create.
Consider adding a check like:
# Check webhook limit existing_count = await db.execute( select(func.count()).select_from(models.WebhookEndpoint).where( models.WebhookEndpoint.workspace_name == webhook.workspace_name ) ) if existing_count.scalar() >= MAX_WEBHOOKS_PER_WORKSPACE: raise ValidationException(f"Webhook limit of {MAX_WEBHOOKS_PER_WORKSPACE} reached")⛔ Skipped due to learnings
Learnt from: Rajat-Ahuja1997 PR: plastic-labs/honcho#131 File: src/crud.py:374-377 Timestamp: 2025-06-18T21:24:55.907Z Learning: The codebase uses a layered error handling approach where the CRUD layer raises ValueError for business rule violations (like peer limits), and the API layer catches these ValueError exceptions and translates them to appropriate domain-specific exceptions (ValidationException for validation errors, ResourceNotFoundException for missing resources). This provides clean separation of concerns between business logic and HTTP semantics.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
src/utils/summarizer.py (3)
241-247: Off-by-one: re-querying the last summarised message leads to duplicate content
start_idis set to the message id already covered by the previous summary, whilecrud.get_messages_id_rangeis inclusive of both ends. The first message inmessageswill therefore repeat in every subsequent summary, steadily bloating summaries and skewing token counts.- start_id=latest_summary["message_id"] if latest_summary else 0, + start_id=(latest_summary["message_id"] + 1) if latest_summary else 0,Alternatively make
get_messages_id_rangestart-exclusive, but adjust one side only.
404-410: Same duplication bug when assembling history for context
get_summarized_historypasses the previous summary’smessage_idas the inclusivestart_id. Users will see the last message twice (once inside the summary, once in<recent_messages>). Apply the same+ 1fix here.- start_id=summary["message_id"], + start_id=summary["message_id"] + 1,
166-176: Shadowing top-level functions – rename inner helpers for clarityDefining inner coroutines named
create_long_summary/create_short_summarymasks the module-level functions of the same names, making stack-traces and IDE navigation harder.- async def create_long_summary(): + async def _persist_long_summary(): ... - async def create_short_summary(): + async def _persist_short_summary(): ... - create_long_summary(), - create_short_summary(), + _persist_long_summary(), + _persist_short_summary(),Also applies to: 178-188
♻️ Duplicate comments (15)
src/utils/summarizer.py (1)
10-10:typing_extensionsimport requires manifest entry – already raised earlierImporting
TypedDictfromtyping_extensionsis the right choice for Py < 3.12 compatibility, but remember the package must be declared inpyproject.toml/requirements.txt. This exact point was flagged in a prior review; please ensure it has been resolved.src/schemas.py (1)
359-384: Enhance URL validation security and exception handling.The URL validation logic is good but needs improvements:
- Security: Add explicit localhost blocking to prevent SSRF attacks
- Exception handling: Use domain-specific exceptions per static analysis recommendations
+from src.exceptions import ValidationException + @field_validator("url") @classmethod def validate_webhook_url(cls, v: str) -> str: parsed = urlparse(v) if not all([parsed.scheme, parsed.netloc]): - raise ValueError("Invalid URL format") + raise ValidationException("Invalid URL format") # Only allow HTTP/HTTPS if parsed.scheme not in ["http", "https"]: - raise ValueError("Only HTTP and HTTPS URLs are allowed") + raise ValidationException("Only HTTP and HTTPS URLs are allowed") # Block private/internal addresses if parsed.hostname: + # Block localhost variations + if parsed.hostname.lower() in ["localhost", "127.0.0.1", "::1"]: + raise ValidationException("Localhost URLs are not allowed") + try: ip_address = ipaddress.ip_address(parsed.hostname) if ip_address.is_private: - raise ValueError("Private IP addresses are not allowed") + raise ValidationException("Private IP addresses are not allowed") except ValueError: # Not an IP address, it's a hostname pass return vsrc/webhooks/webhook_delivery.py (9)
37-37: Store task reference to prevent potential issues.The created task should be stored to prevent it from being garbage collected prematurely.
59-61: Use logging.exception for better debugging.When catching exceptions, use logger.exception to include stack traces.
77-81: Simplify signature generation return.The signature variable assignment is unnecessary before the return statement.
105-105: Use timezone-aware datetime.Using
datetime.now()without timezone can cause issues in distributed systems.
113-117: Use logger.exception for better error tracking.When catching exceptions, use logger.exception to include stack traces.
119-136: Improve per-endpoint delivery logging and error handling.Currently, the code iterates through all endpoints but only logs aggregate success/failure. This makes debugging difficult when some endpoints succeed and others fail.
138-145: Use logger.exception in exception handlers.Both exception handlers should use logger.exception for better debugging.
163-167: Use logger.exception in error handler.Use logger.exception to include stack trace information.
73-75: Consider using a dedicated webhook secret.Using
AUTH_JWT_SECRETfor webhook signatures couples webhook security to JWT authentication. Consider introducing a separateWEBHOOK_SECRETenvironment variable for webhook signing to allow independent configuration.- auth_secret = os.getenv("AUTH_JWT_SECRET") - if not auth_secret: - raise ValueError("AUTH_JWT_SECRET not found - cannot sign webhook") + webhook_secret = os.getenv("WEBHOOK_SECRET") or os.getenv("AUTH_JWT_SECRET") + if not webhook_secret: + raise ValueError("WEBHOOK_SECRET or AUTH_JWT_SECRET not found - cannot sign webhook") - auth_secret.encode("utf-8"), payload.encode("utf-8"), hashlib.sha256 + webhook_secret.encode("utf-8"), payload.encode("utf-8"), hashlib.sha256src/crud/webhook.py (2)
60-60: Use structured logging instead of f-string.For better performance and consistency with logging best practices, use structured logging.
111-111: Use structured logging instead of f-string.For consistency with logging best practices, use structured logging.
tests/routes/test_webhooks.py (1)
131-135: Test may be flaky due to assumption about empty webhook list.The test assumes the webhook list will be empty after deletion, but this could fail if other tests create webhooks that persist.
src/routers/webhooks.py (1)
63-67: Add return type annotation.Missing return type annotation for the delete endpoint.
-async def delete_webhook_endpoint( +async def delete_webhook_endpoint( endpoint_id: Annotated[str, Path(description="Webhook endpoint ID")], jwt_params: JWTParams = Depends(require_auth()), db: AsyncSession = db, -): +) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
migrations/versions/88b0fb10906f_add_webhooks_table.py(1 hunks)src/config.py(3 hunks)src/crud/webhook.py(1 hunks)src/main.py(3 hunks)src/models.py(2 hunks)src/routers/webhooks.py(1 hunks)src/schemas.py(2 hunks)src/utils/summarizer.py(1 hunks)src/webhooks/webhook_delivery.py(1 hunks)tests/routes/test_webhooks.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/utils/summarizer.pysrc/main.pysrc/webhooks/webhook_delivery.pysrc/config.pysrc/models.pysrc/schemas.pysrc/routers/webhooks.pysrc/crud/webhook.py
src/main.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Global exception handlers defined in main.py
Files:
src/main.py
src/models.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/models.py: Use explicit type hints with SQLAlchemy mapped_column annotations
All tables use text IDs (nanoid format) as primary keys
Composite foreign keys for multi-tenant relationships
Feature flags on workspace, peer, and session levels
JSONB metadata fields for extensibility
HNSW indexes for vector similarity search
Files:
src/models.py
src/{models,crud}.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Token counting on messages for usage tracking
Files:
src/models.py
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/routes/test_webhooks.py
src/routers/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
API routes must follow the pattern: /v1/{resource}/{id}/{action}
Files:
src/routers/webhooks.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: VVoruganti
PR: plastic-labs/honcho#168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
src/utils/summarizer.py (3)
Learnt from: VVoruganti
PR: #168
File: src/utils/summarizer.py:10-10
Timestamp: 2025-07-30T19:44:05.940Z
Learning: The Honcho project uses Python 3.11 in Dockerfile and requires Python >=3.10 in pyproject.toml. The project uses typing_extensions for the override decorator which is required for Python < 3.12 compatibility.
Learnt from: Rajat-Ahuja1997
PR: #144
File: src/embeddings.py:217-224
Timestamp: 2025-06-26T18:33:10.167Z
Learning: In the plastic-labs/honcho project, the user's linter enforces spaces around colons in slice notation (e.g., encoded_tokens[i : i + max_tokens]), which conflicts with Flake8's E203 rule but is valid Python syntax and preferred by Black formatter.
Learnt from: VVoruganti
PR: #164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository should have COM812 (trailing comma missing) rule disabled in the root pyproject.toml Ruff configuration by adding it to the existing ignore list: ignore = ["E501", "B008", "COM812"]. The maintainers do not want trailing comma suggestions in code reviews.
src/main.py (2)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/main.py : Global exception handlers defined in main.py
src/webhooks/webhook_delivery.py (2)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/**/*.py : Explicit error handling with appropriate exception types
Learnt from: VVoruganti
PR: #168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
migrations/versions/88b0fb10906f_add_webhooks_table.py (3)
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:36-77
Timestamp: 2025-06-19T14:07:32.309Z
Learning: The migration script migrations/versions/d429de0e5338_adopt_peer_paradigm.py properly handles foreign key constraints during table updates by explicitly dropping foreign key constraints before making schema changes, preventing constraint violations during the migration process.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:438-441
Timestamp: 2025-06-18T16:44:57.733Z
Learning: In the Honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not workspaces.id (integer primary key). This means that during migrations when workspaces.public_id is renamed to workspaces.id, the foreign key relationship with messages.app_id remains valid.
Learnt from: Rajat-Ahuja1997
PR: #131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:188-194
Timestamp: 2025-06-18T15:58:51.202Z
Learning: In the migration file migrations/versions/d429de0e5338_adopt_peer_paradigm.py, the team has explicitly decided to accept SQL injection risks from f-string interpolation of schema names, despite the security concerns around DDL injection via the DATABASE_SCHEMA environment variable.
src/config.py (1)
Learnt from: VVoruganti
PR: #168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
src/models.py (5)
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Use explicit type hints with SQLAlchemy mapped_column annotations
Learnt from: VVoruganti
PR: #164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository should have COM812 (trailing comma missing) rule disabled in the root pyproject.toml Ruff configuration by adding it to the existing ignore list: ignore = ["E501", "B008", "COM812"]. The maintainers do not want trailing comma suggestions in code reviews.
Learnt from: Rajat-Ahuja1997
PR: #144
File: tests/integration/test_message_embeddings.py:128-187
Timestamp: 2025-06-26T18:35:46.478Z
Learning: The honcho repository does not use trailing commas in function signatures, function calls, or object instantiation. This is consistently applied across the entire codebase including test files, and reviews should not suggest adding trailing commas.
Learnt from: VVoruganti
PR: #164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository maintainers do not want trailing comma suggestions or complaints from static analysis tools like Ruff. The COM812 rule should be disabled in the Ruff configuration to avoid these suggestions in future reviews.
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
src/schemas.py (2)
Learnt from: Rajat-Ahuja1997
PR: #131
File: src/crud.py:374-377
Timestamp: 2025-06-18T21:24:55.907Z
Learning: The codebase uses a layered error handling approach where the CRUD layer raises ValueError for business rule violations (like peer limits), and the API layer catches these ValueError exceptions and translates them to appropriate domain-specific exceptions (ValidationException for validation errors, ResourceNotFoundException for missing resources). This provides clean separation of concerns between business logic and HTTP semantics.
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
tests/routes/test_webhooks.py (2)
Learnt from: Rajat-Ahuja1997
PR: #131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use @pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Learnt from: VVoruganti
PR: #168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
src/routers/webhooks.py (1)
Learnt from: VVoruganti
PR: #168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
src/crud/webhook.py (1)
Learnt from: VVoruganti
PR: #168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.500Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
🧬 Code Graph Analysis (5)
src/main.py (1)
src/webhooks/webhook_delivery.py (2)
start(29-37)stop(39-46)
src/webhooks/webhook_delivery.py (4)
src/dependencies.py (1)
tracked_db(32-61)src/webhooks/events.py (1)
WebhookEvent(18-21)src/webhooks/event_emitter.py (1)
get_event(46-48)src/crud/webhook.py (1)
list_webhook_endpoints(64-84)
src/models.py (1)
src/schemas.py (1)
WebhookEndpoint(387-393)
src/schemas.py (1)
src/models.py (1)
WebhookEndpoint(402-421)
src/crud/webhook.py (3)
src/routers/webhooks.py (3)
get_or_create_webhook_endpoint(26-43)list_webhook_endpoints(47-59)delete_webhook_endpoint(63-75)src/schemas.py (2)
WebhookEndpointCreate(359-384)WebhookEndpoint(387-393)src/models.py (1)
WebhookEndpoint(402-421)
🪛 Ruff (0.12.2)
src/webhooks/webhook_delivery.py
33-33: Trailing comma missing
Add trailing comma
(COM812)
37-37: Store a reference to the return value of asyncio.create_task
(RUF006)
59-59: Do not catch blind exception: Exception
(BLE001)
60-60: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
60-60: Logging statement uses f-string
(G004)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
78-78: Trailing comma missing
Add trailing comma
(COM812)
81-81: Unnecessary assignment to signature before return statement
Remove unnecessary assignment
(RET504)
98-98: Logging statement uses f-string
(G004)
98-98: Trailing comma missing
Add trailing comma
(COM812)
105-105: datetime.datetime.now() called without a tz argument
(DTZ005)
109-109: Trailing comma missing
Add trailing comma
(COM812)
116-116: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
116-116: Logging statement uses f-string
(G004)
131-131: Logging statement uses f-string
(G004)
131-131: Trailing comma missing
Add trailing comma
(COM812)
135-135: Logging statement uses f-string
(G004)
135-135: Trailing comma missing
Add trailing comma
(COM812)
139-141: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
140-140: Logging statement uses f-string
(G004)
140-140: Trailing comma missing
Add trailing comma
(COM812)
142-142: Do not catch blind exception: Exception
(BLE001)
143-145: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
144-144: Logging statement uses f-string
(G004)
144-144: Trailing comma missing
Add trailing comma
(COM812)
163-163: Do not catch blind exception: Exception
(BLE001)
164-166: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
165-165: Logging statement uses f-string
(G004)
165-165: Trailing comma missing
Add trailing comma
(COM812)
migrations/versions/88b0fb10906f_add_webhooks_table.py
65-65: Trailing comma missing
Add trailing comma
(COM812)
src/models.py
406-406: Trailing comma missing
Add trailing comma
(COM812)
410-410: Trailing comma missing
Add trailing comma
(COM812)
src/schemas.py
369-369: Avoid specifying long messages outside the exception class
(TRY003)
373-373: Avoid specifying long messages outside the exception class
(TRY003)
380-380: Abstract raise to an inner function
(TRY301)
380-380: Avoid specifying long messages outside the exception class
(TRY003)
tests/routes/test_webhooks.py
10-10: Missing return type annotation for public function test_create_webhook_endpoint
Add return type annotation: None
(ANN201)
11-11: Trailing comma missing
Add trailing comma
(COM812)
30-30: Missing return type annotation for public function test_create_webhook_endpoint_invalid_url
Add return type annotation: None
(ANN201)
31-31: Trailing comma missing
Add trailing comma
(COM812)
48-48: Missing return type annotation for public function test_create_webhook_endpoint_missing_workspace
Add return type annotation: None
(ANN201)
61-61: Missing return type annotation for public function test_list_webhook_endpoints_with_data
Add return type annotation: None
(ANN201)
62-62: Trailing comma missing
Add trailing comma
(COM812)
103-103: Missing return type annotation for public function test_list_webhook_endpoints_missing_workspace
Add return type annotation: None
(ANN201)
110-110: Missing return type annotation for public function test_delete_webhook_endpoint
Add return type annotation: None
(ANN201)
111-111: Trailing comma missing
Add trailing comma
(COM812)
139-139: Missing return type annotation for public function test_delete_webhook_endpoint_not_found
Add return type annotation: None
(ANN201)
146-146: Missing return type annotation for public function test_multiple_endpoints_per_workspace
Add return type annotation: None
(ANN201)
147-147: Trailing comma missing
Add trailing comma
(COM812)
src/routers/webhooks.py
26-26: Missing return type annotation for public function get_or_create_webhook_endpoint
(ANN201)
27-29: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
28-28: Trailing comma missing
Add trailing comma
(COM812)
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-30: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
41-41: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Missing return type annotation for public function list_webhook_endpoints
(ANN201)
49-49: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
49-49: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
56-56: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Missing return type annotation for public function delete_webhook_endpoint
Add return type annotation: None
(ANN201)
65-65: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
65-65: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Missing return type annotation for public function test_emit
Add return type annotation: None
(ANN201)
80-80: Test function parameter workspace_name has default argument
Remove default argument
(PT028)
src/crud/webhook.py
42-44: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Trailing comma missing
Add trailing comma
(COM812)
60-60: Logging statement uses f-string
(G004)
65-65: Trailing comma missing
Add trailing comma
(COM812)
81-81: Trailing comma missing
Add trailing comma
(COM812)
84-84: Unnecessary assignment to stmt before return statement
Remove unnecessary assignment
(RET504)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Logging statement uses f-string
(G004)
🔇 Additional comments (16)
src/main.py (2)
106-113: LGTM! Proper async lifecycle management.The webhook delivery service integration follows the correct async lifecycle pattern with startup in the context manager and cleanup in the finally block. The ordering ensures the service starts before the application serves requests and stops cleanly during shutdown.
161-161: LGTM! Webhook router properly integrated.The webhook router is correctly included under the "/v2" API prefix, maintaining consistency with other API routes.
src/config.py (2)
57-57: LGTM! TOML configuration mapping added.The SECTION_MAP entry correctly enables webhook settings to be loaded from TOML configuration files under the "webhooks" section.
240-243: LGTM! Webhook settings properly configured.The WebhookSettings class follows the established pattern with proper inheritance, environment prefix, and the workspace limit default of 10 aligns with the retrieved learning requirements.
migrations/versions/88b0fb10906f_add_webhooks_table.py (2)
27-58: LGTM! Well-structured table creation.The webhook_endpoints table is properly defined with:
- Appropriate column types and constraints
- Foreign key relationship to workspaces.name
- URL length constraint (2048 chars) for security
- Optimized index on workspace_name for lookups
61-74: LGTM! Safe downgrade implementation.The downgrade function properly checks for index existence before attempting to drop it, preventing errors during rollback operations.
src/models.py (2)
86-86: LGTM! Proper relationship configuration.The webhook_endpoints relationship is correctly defined with proper back_populates reference to maintain bidirectional association.
401-422: LGTM! Well-designed ORM model.The WebhookEndpoint model follows established patterns with:
- Proper nanoid primary key generation
- Correct foreign key relationship to workspaces
- Appropriate constraints matching the database schema
- Index for query optimization
- Helpful repr method for debugging
src/schemas.py (1)
387-393: LGTM! Well-structured response schema.The WebhookEndpoint schema properly defines all fields with correct serialization aliases and Pydantic configuration for ORM integration.
src/crud/webhook.py (2)
40-44: LGTM! Webhook limit enforcement implemented correctly.The implementation correctly enforces the workspace webhook limit using
settings.WEBHOOKS.WORKSPACE_LIMITas specified in the requirements. The error message is clear and informative.
87-111: LGTM! Delete logic implemented correctly.The function properly validates endpoint existence and raises appropriate exceptions. The database operations are handled correctly with proper commit.
tests/routes/test_webhooks.py (5)
9-26: LGTM! Well-structured creation test.The test properly validates webhook creation with correct status codes and response field verification. The async pattern usage is appropriate for FastAPI testing.
29-44: LGTM! Proper validation testing.The test correctly validates the URL format validation logic, expecting appropriate 422 status codes for invalid URLs.
47-57: LGTM! Workspace validation test is correct.The test properly validates that non-existent workspaces return 404 errors with appropriate error messages.
60-99: LGTM! Comprehensive listing test.The test properly handles existing data by using initial_count and correctly validates that multiple endpoints can be created and listed for a workspace.
145-192: LGTM! Excellent comprehensive test.This test thoroughly validates multiple endpoint scenarios, properly handles existing data with initial_count, and correctly verifies that deleting one endpoint doesn't affect others. Well-designed test coverage.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (9)
src/webhooks/README.md (1)
21-21: Fix typo in documentationThere's a typo "facilitate" is misspelled.
-Note that the webhooks require the *deriver* process to be running to facilitate the delivery of the webhook. +Note that the webhooks require the *deriver* process to be running to facilitate the delivery of the webhook.src/webhooks/webhook_delivery.py (1)
18-78: Replace f-string logging with proper formattingMultiple logging statements use f-strings which is against logging best practices. Use lazy formatting with % or pass arguments separately.
if not webhook_urls: logger.info( - f"No webhook endpoints for workspace {payload.workspace_name}, skipping." + "No webhook endpoints for workspace %s, skipping.", + payload.workspace_name, ) return # ... other code ... for url, result in zip(webhook_urls, results, strict=False): if isinstance(result, httpx.Response): if 200 <= result.status_code < 300: logger.info( - f"Successfully delivered webhook {payload.event_type} to {url}" + "Successfully delivered webhook %s to %s", + payload.event_type, + url, ) else: logger.error( - f"Failed delivery for {payload.event_type} to {url}. Status: {result.status_code}" + "Failed delivery for %s to %s. Status: %s", + payload.event_type, + url, + result.status_code, ) else: logger.error( - f"Failed delivery for {payload.event_type} to {url}. Exception: {result}" + "Failed delivery for %s to %s. Exception: %s", + payload.event_type, + url, + result, ) except httpx.RequestError: - logger.exception(f"Error sending webhook for {payload.workspace_name}.") + logger.exception("Error sending webhook for %s.", payload.workspace_name)migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
66-78: Inconsistency with model definition for task_type columnThe migration adds
task_typeas nullable, but the model likely defines it as non-nullable. This could cause ORM issues.Consider updating to match the model:
op.add_column( "queue", - sa.Column("task_type", sa.TEXT(), nullable=True), + sa.Column("task_type", sa.TEXT(), nullable=False, server_default="representation"), schema=schema, )src/deriver/utils.py (1)
12-15: Fix incorrect docstringThe docstring mentions parameters that don't match the function signature.
def get_work_unit_key(task_type: str, payload: dict[str, Any]) -> str: """ - Generate a work unit key for a given task type, workspace name, and event type. + Generate a work unit key for a given task type and payload. """src/crud/webhook.py (2)
61-61: Use structured logging instead of f-string.
115-115: Use structured logging instead of f-string.src/webhooks/events.py (1)
30-30: Consider using Literal type for queue_type field.For better type safety and to restrict valid values, consider using a Literal type for the
queue_typefield.- queue_type: str + queue_type: Literal["representation", "summary", "webhook"]This ensures only valid queue types can be assigned and provides better IDE support and type checking.
tests/routes/test_webhooks.py (1)
128-133: Test may be flaky due to assumption about empty webhook list.The test assumes the webhook list will be empty after deletion (line 132), but this could fail if other tests create webhooks that persist. Consider checking that the specific webhook was deleted instead:
- assert response_data["items"] == [] + endpoints = response_data["items"] + endpoint_ids = [ep["id"] for ep in endpoints] + assert endpoint_id not in endpoint_idssrc/deriver/queue_manager.py (1)
225-225: Add return type annotation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
migrations/versions/88b0fb10906f_add_webhooks_table.py(1 hunks)src/config.py(4 hunks)src/crud/webhook.py(1 hunks)src/deriver/queue_manager.py(12 hunks)src/deriver/utils.py(1 hunks)src/routers/webhooks.py(1 hunks)src/webhooks/README.md(1 hunks)src/webhooks/events.py(1 hunks)src/webhooks/webhook_delivery.py(1 hunks)tests/routes/test_webhooks.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/config.pysrc/webhooks/webhook_delivery.pysrc/crud/webhook.pysrc/routers/webhooks.pysrc/deriver/utils.pysrc/webhooks/events.pysrc/deriver/queue_manager.py
src/routers/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
API routes must follow the pattern: /v1/{resource}/{id}/{action}
Files:
src/routers/webhooks.py
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/routes/test_webhooks.py
🧠 Learnings (15)
📓 Common learnings
Learnt from: VVoruganti
PR: plastic-labs/honcho#168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.513Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#168
File: src/config.py:240-245
Timestamp: 2025-08-04T20:21:58.211Z
Learning: In the webhook settings implementation for plastic-labs/honcho, the field name MAX_WORKSPACE_LIMIT is used instead of MAX_WEBHOOKS_PER_WORKSPACE, which maps to the environment variable WEBHOOKS_MAX_WORKSPACE_LIMIT for configuring the maximum number of webhooks per workspace.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
📚 Learning: the webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 we...
Learnt from: VVoruganti
PR: plastic-labs/honcho#168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.513Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
Applied to files:
src/webhooks/README.mdsrc/config.pysrc/webhooks/webhook_delivery.pysrc/crud/webhook.pysrc/routers/webhooks.pysrc/deriver/utils.pytests/routes/test_webhooks.py
📚 Learning: in the webhook settings implementation for plastic-labs/honcho, the field name max_workspace_limit i...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#168
File: src/config.py:240-245
Timestamp: 2025-08-04T20:21:58.211Z
Learning: In the webhook settings implementation for plastic-labs/honcho, the field name MAX_WORKSPACE_LIMIT is used instead of MAX_WEBHOOKS_PER_WORKSPACE, which maps to the environment variable WEBHOOKS_MAX_WORKSPACE_LIMIT for configuring the maximum number of webhooks per workspace.
Applied to files:
src/config.pysrc/webhooks/webhook_delivery.pysrc/crud/webhook.pysrc/routers/webhooks.pysrc/deriver/utils.pytests/routes/test_webhooks.py
📚 Learning: rajat-ahuja1997 is comfortable exposing token counts in messagecreate schema as a public property wh...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
Applied to files:
src/config.py
📚 Learning: applies to src/**/*.py : explicit error handling with appropriate exception types...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/**/*.py : Explicit error handling with appropriate exception types
Applied to files:
src/webhooks/webhook_delivery.py
📚 Learning: the honcho repository should have com812 (trailing comma missing) rule disabled in the root pyprojec...
Learnt from: VVoruganti
PR: plastic-labs/honcho#164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository should have COM812 (trailing comma missing) rule disabled in the root pyproject.toml Ruff configuration by adding it to the existing ignore list: ignore = ["E501", "B008", "COM812"]. The maintainers do not want trailing comma suggestions in code reviews.
Applied to files:
src/webhooks/webhook_delivery.pysrc/deriver/queue_manager.py
📚 Learning: the honcho repository maintainers do not want trailing comma suggestions or complaints from static a...
Learnt from: VVoruganti
PR: plastic-labs/honcho#164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository maintainers do not want trailing comma suggestions or complaints from static analysis tools like Ruff. The COM812 rule should be disabled in the Ruff configuration to avoid these suggestions in future reviews.
Applied to files:
src/webhooks/webhook_delivery.pysrc/deriver/queue_manager.py
📚 Learning: the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` properly handles fore...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:36-77
Timestamp: 2025-06-19T14:07:32.309Z
Learning: The migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` properly handles foreign key constraints during table updates by explicitly dropping foreign key constraints before making schema changes, preventing constraint violations during the migration process.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: the queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:918-942
Timestamp: 2025-06-19T14:32:02.934Z
Learning: The queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` are expected to remain small, so per-row updates are acceptable and performance optimizations like set-based updates are not necessary.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.pysrc/deriver/queue_manager.py
📚 Learning: in the honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not ...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:438-441
Timestamp: 2025-06-18T16:44:57.733Z
Learning: In the Honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not workspaces.id (integer primary key). This means that during migrations when workspaces.public_id is renamed to workspaces.id, the foreign key relationship with messages.app_id remains valid.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: in the migration file `migrations/versions/d429de0e5338_adopt_peer_paradigm.py`, the team has explic...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:188-194
Timestamp: 2025-06-18T15:58:51.202Z
Learning: In the migration file `migrations/versions/d429de0e5338_adopt_peer_paradigm.py`, the team has explicitly decided to accept SQL injection risks from f-string interpolation of schema names, despite the security concerns around DDL injection via the DATABASE_SCHEMA environment variable.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: applies to src/models.py : use explicit type hints with sqlalchemy mapped_column annotations...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Use explicit type hints with SQLAlchemy mapped_column annotations
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: applies to src/models.py : feature flags on workspace, peer, and session levels...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Applied to files:
src/routers/webhooks.py
📚 Learning: in fastapi tests, it's common to use @pytest.mark.asyncio with a synchronous testclient. the async m...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Applied to files:
tests/routes/test_webhooks.py
📚 Learning: the honcho repository does not use trailing commas in function signatures, function calls, or object...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: tests/integration/test_message_embeddings.py:128-187
Timestamp: 2025-06-26T18:35:46.478Z
Learning: The honcho repository does not use trailing commas in function signatures, function calls, or object instantiation. This is consistently applied across the entire codebase including test files, and reviews should not suggest adding trailing commas.
Applied to files:
src/deriver/queue_manager.py
🪛 LanguageTool
src/webhooks/README.md
[grammar] ~1-~1: Use correct spacing
Context: # Webhooks Webhooks are used to deliver event notif...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: Use correct spacing
Context: ... event notifications to user-configured URLs. ## System Architecture The webhooks system...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~5-~5: Use correct spacing
Context: ...ons to user-configured URLs. ## System Architecture The webhooks system consists of several ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use correct spacing
Context: ...webhooks system consists of several key components: * API Endpoints (routers/webhooks.py):...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~11-~11: Use correct spacing
Context: ...sending the webhook to the subscriber's URL. ## Event Flow 1. An event is triggered wit...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~13-~13: Use correct spacing
Context: ...hook to the subscriber's URL. ## Event Flow 1. An event is triggered within the applica...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~19-~19: Use correct spacing
Context: ..._webhookfunction. 5.deliver_webhook` fetches all subscriber URLs for the event's wor...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~19-~19: Use correct spacing
Context: ... and sends an HTTP POST request to each URL. Note that the webhooks require the *deri...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~21-~21: Ensure spelling is correct
Context: ...L. Note that the webhooks require the deriver process to be running to facilitate th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.12.2)
src/config.py
255-255: Possible binding to all interfaces
(S104)
src/webhooks/webhook_delivery.py
27-27: Logging statement uses f-string
(G004)
27-27: Trailing comma missing
Add trailing comma
(COM812)
37-37: Trailing comma missing
Add trailing comma
(COM812)
63-63: Logging statement uses f-string
(G004)
63-63: Trailing comma missing
Add trailing comma
(COM812)
67-67: Logging statement uses f-string
(G004)
67-67: Trailing comma missing
Add trailing comma
(COM812)
71-71: Logging statement uses f-string
(G004)
71-71: Trailing comma missing
Add trailing comma
(COM812)
75-75: Logging statement uses f-string
(G004)
89-89: Logging statement uses f-string
(G004)
99-99: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Trailing comma missing
Add trailing comma
(COM812)
migrations/versions/88b0fb10906f_add_webhooks_table.py
82-101: Possible SQL injection vector through string-based query construction
(S608)
101-101: Trailing comma missing
Add trailing comma
(COM812)
102-102: Trailing comma missing
Add trailing comma
(COM812)
123-123: Trailing comma missing
Add trailing comma
(COM812)
126-126: Trailing comma missing
Add trailing comma
(COM812)
147-147: Trailing comma missing
Add trailing comma
(COM812)
165-165: Trailing comma missing
Add trailing comma
(COM812)
168-168: Trailing comma missing
Add trailing comma
(COM812)
src/crud/webhook.py
43-45: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Trailing comma missing
Add trailing comma
(COM812)
61-61: Logging statement uses f-string
(G004)
66-66: Trailing comma missing
Add trailing comma
(COM812)
82-82: Trailing comma missing
Add trailing comma
(COM812)
87-87: Trailing comma missing
Add trailing comma
(COM812)
108-110: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Trailing comma missing
Add trailing comma
(COM812)
115-115: Logging statement uses f-string
(G004)
src/routers/webhooks.py
30-32: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-31: Trailing comma missing
Add trailing comma
(COM812)
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-33: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
40-40: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Trailing comma missing
Add trailing comma
(COM812)
47-49: Avoid specifying long messages outside the exception class
(TRY003)
48-48: Trailing comma missing
Add trailing comma
(COM812)
55-55: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
55-55: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
72-72: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
72-72: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Test function parameter workspace_id has default argument
Remove default argument
(PT028)
88-88: Test function parameter jwt_params has default argument
Remove default argument
(PT028)
88-88: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
88-88: Do not perform function call require_auth in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
src/deriver/utils.py
18-18: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
43-45: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Trailing comma missing
Add trailing comma
(COM812)
56-58: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Trailing comma missing
Add trailing comma
(COM812)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
src/webhooks/events.py
63-63: Trailing comma missing
Add trailing comma
(COM812)
tests/routes/test_webhooks.py
11-11: Missing return type annotation for public function test_create_webhook_endpoint
Add return type annotation: None
(ANN201)
12-12: Trailing comma missing
Add trailing comma
(COM812)
30-30: Missing return type annotation for public function test_create_webhook_endpoint_invalid_url
Add return type annotation: None
(ANN201)
31-31: Trailing comma missing
Add trailing comma
(COM812)
47-47: Missing return type annotation for public function test_create_webhook_endpoint_missing_workspace
Add return type annotation: None
(ANN201)
59-59: Missing return type annotation for public function test_list_webhook_endpoints_with_data
Add return type annotation: None
(ANN201)
60-60: Trailing comma missing
Add trailing comma
(COM812)
99-99: Missing return type annotation for public function test_list_webhook_endpoints_missing_workspace
Add return type annotation: None
(ANN201)
106-106: Missing return type annotation for public function test_delete_webhook_endpoint
Add return type annotation: None
(ANN201)
107-107: Trailing comma missing
Add trailing comma
(COM812)
124-124: Trailing comma missing
Add trailing comma
(COM812)
136-136: Missing return type annotation for public function test_delete_webhook_endpoint_not_found
Add return type annotation: None
(ANN201)
137-137: Trailing comma missing
Add trailing comma
(COM812)
141-141: Trailing comma missing
Add trailing comma
(COM812)
148-148: Missing return type annotation for public function test_multiple_endpoints_per_workspace
Add return type annotation: None
(ANN201)
149-149: Trailing comma missing
Add trailing comma
(COM812)
187-187: Trailing comma missing
Add trailing comma
(COM812)
200-200: Missing return type annotation for public function test_create_duplicate_webhook_endpoint
Add return type annotation: None
(ANN201)
201-201: Trailing comma missing
Add trailing comma
(COM812)
231-231: Missing return type annotation for public function test_max_webhook_endpoints_per_workspace
Add return type annotation: None
(ANN201)
232-232: Trailing comma missing
Add trailing comma
(COM812)
258-258: Missing return type annotation for public function test_same_endpoint_in_different_workspaces
Add return type annotation: None
(ANN201)
259-259: Trailing comma missing
Add trailing comma
(COM812)
src/deriver/queue_manager.py
98-98: Logging statement uses f-string
(G004)
105-105: Trailing comma missing
Add trailing comma
(COM812)
179-179: Trailing comma missing
Add trailing comma
(COM812)
201-201: Logging statement uses f-string
(G004)
201-201: Trailing comma missing
Add trailing comma
(COM812)
225-225: Missing return type annotation for public function process_work_unit
Add return type annotation: None
(ANN201)
227-227: Logging statement uses f-string
(G004)
237-237: Logging statement uses f-string
(G004)
244-244: Logging statement uses f-string
(G004)
244-244: Trailing comma missing
Add trailing comma
(COM812)
248-248: Logging statement uses f-string
(G004)
248-248: Trailing comma missing
Add trailing comma
(COM812)
251-251: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
252-252: Logging statement uses f-string
(G004)
252-252: Use explicit conversion flag
Replace with conversion flag
(RUF010)
264-264: Logging statement uses f-string
(G004)
264-264: Trailing comma missing
Add trailing comma
(COM812)
272-272: Trailing comma missing
Add trailing comma
(COM812)
277-277: Logging statement uses f-string
(G004)
277-277: Trailing comma missing
Add trailing comma
(COM812)
281-281: Logging statement uses f-string
(G004)
284-284: Trailing comma missing
Add trailing comma
(COM812)
285-285: Trailing comma missing
Add trailing comma
(COM812)
301-301: Logging statement uses f-string
(G004)
301-301: Trailing comma missing
Add trailing comma
(COM812)
310-310: Trailing comma missing
Add trailing comma
(COM812)
314-314: Logging statement uses f-string
(G004)
314-314: Trailing comma missing
Add trailing comma
(COM812)
320-320: Logging statement uses f-string
(G004)
320-320: Trailing comma missing
Add trailing comma
(COM812)
327-327: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (4)
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
79-104: LGTM! Backfill logic properly handles existing queue itemsThe backfill logic correctly populates
work_unit_keyfor unprocessed queue items, addressing the concern about breaking existing queue items during migration.src/crud/webhook.py (1)
108-110: Move exception message to ResourceNotFoundException initialization.The long exception message could be better handled within the exception class itself.
- raise ResourceNotFoundException( - f"Webhook endpoint {endpoint_id} not found for workspace {workspace_name}" - ) + raise ResourceNotFoundException( + f"Webhook endpoint {endpoint_id} not found for workspace {workspace_name}", + )Note: Add trailing comma for consistency with the codebase style.
Likely an incorrect or invalid review comment.
src/deriver/queue_manager.py (1)
299-311: Good implementation of queue.empty webhook events.The webhook event publishing logic correctly:
- Parses the work unit key to extract metadata
- Only publishes for relevant task types (representation, summary)
- Includes all necessary context in the event payload
- Handles errors gracefully without disrupting queue processing
src/routers/webhooks.py (1)
52-65: LGTM! Pagination properly implemented.The endpoint correctly implements pagination using
fastapi_paginationand has proper authorization checks.
| # Only publish webhook if we actually removed an active session | ||
| if delete_result.rowcount > 0 and message_count > 0: | ||
| try: | ||
| from src.deriver.utils import parse_work_unit_key | ||
| from src.webhooks.events import ( | ||
| QueueEmptyEvent, | ||
| publish_webhook_event, | ||
| ) | ||
|
|
||
| parsed_key = parse_work_unit_key(work_unit_key) | ||
| if parsed_key["task_type"] in ["representation", "summary"]: | ||
| logger.info( | ||
| f"Publishing queue.empty event for {work_unit_key}" | ||
| ) | ||
| await publish_webhook_event( | ||
| QueueEmptyEvent( | ||
| workspace_id=parsed_key["workspace_name"], | ||
| queue_type=parsed_key["task_type"], | ||
| session_id=parsed_key["session_name"], | ||
| sender_name=parsed_key["sender_name"], | ||
| observer_name=parsed_key["target_name"], | ||
| ) | ||
| ) | ||
| else: | ||
| logger.debug( | ||
| f"Skipping queue.empty event for webhook work unit {work_unit_key}" | ||
| ) | ||
| except Exception: | ||
| logger.exception("Error triggering queue_empty webhook") | ||
| else: | ||
| logger.debug( | ||
| f"Work unit {work_unit_key} already cleaned up by another worker, skipping webhook" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extracting webhook publishing to a separate method.
The webhook publishing logic is quite substantial and could benefit from being extracted to improve readability and testability.
async def _publish_queue_empty_webhook(self, work_unit_key: str) -> None:
"""Publish a queue.empty webhook event if applicable."""
try:
from src.deriver.utils import parse_work_unit_key
from src.webhooks.events import (
QueueEmptyEvent,
publish_webhook_event,
)
parsed_key = parse_work_unit_key(work_unit_key)
if parsed_key["task_type"] in ["representation", "summary"]:
logger.info(
f"Publishing queue.empty event for {work_unit_key}"
)
await publish_webhook_event(
QueueEmptyEvent(
workspace_id=parsed_key["workspace_name"],
queue_type=parsed_key["task_type"],
session_id=parsed_key["session_name"],
sender_name=parsed_key["sender_name"],
observer_name=parsed_key["target_name"],
)
)
else:
logger.debug(
f"Skipping queue.empty event for webhook work unit {work_unit_key}"
)
except Exception:
logger.exception("Error triggering queue_empty webhook")Then call it in the main method:
- try:
- from src.deriver.utils import parse_work_unit_key
- # ... rest of the code
- except Exception:
- logger.exception("Error triggering queue_empty webhook")
+ await self._publish_queue_empty_webhook(work_unit_key)🧰 Tools
🪛 Ruff (0.12.2)
301-301: Logging statement uses f-string
(G004)
301-301: Trailing comma missing
Add trailing comma
(COM812)
310-310: Trailing comma missing
Add trailing comma
(COM812)
314-314: Logging statement uses f-string
(G004)
314-314: Trailing comma missing
Add trailing comma
(COM812)
320-320: Logging statement uses f-string
(G004)
320-320: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In src/deriver/queue_manager.py around lines 289 to 322, the webhook publishing
logic is embedded directly in the main method, making it bulky and harder to
read or test. Extract this logic into a separate async method named
_publish_queue_empty_webhook that takes work_unit_key as a parameter. Move all
the try-except and import statements inside this new method, preserving the
existing functionality and logging. Then replace the original webhook publishing
block with a call to this new method to improve code clarity and
maintainability.
| # Clean up stale work units | ||
| five_minutes_ago = datetime.now(UTC) - timedelta( | ||
| five_minutes_ago = datetime.now(timezone.utc) - timedelta( | ||
| minutes=settings.DERIVER.STALE_SESSION_TIMEOUT_MINUTES |
There was a problem hiding this comment.
this timeout is currently 5min in testing/prod. we should conside adding an additional check to the stale cleanup query e.g.
.where(
~exists(
select(1)
.select_from(models.QueueItem)
.where(
models.QueueItem.work_unit_key
== models.ActiveQueueSession.work_unit_key,
~models.QueueItem.processed,
)
)
)
and then we can have an additional check where we delete any active queue sessions where last_update >20min (likely a crashed worker)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
tests/crud/test_session.py(1 hunks)tests/deriver/conftest.py(3 hunks)tests/deriver/test_deriver_processing.py(1 hunks)tests/deriver/test_queue_operations.py(1 hunks)tests/deriver/test_queue_processing.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/deriver/test_queue_operations.pytests/crud/test_session.pytests/deriver/conftest.pytests/deriver/test_queue_processing.pytests/deriver/test_deriver_processing.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: VVoruganti
PR: plastic-labs/honcho#168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.513Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#168
File: src/config.py:240-245
Timestamp: 2025-08-04T20:21:58.211Z
Learning: In the webhook settings implementation for plastic-labs/honcho, the field name MAX_WORKSPACE_LIMIT is used instead of MAX_WEBHOOKS_PER_WORKSPACE, which maps to the environment variable WEBHOOKS_MAX_WORKSPACE_LIMIT for configuring the maximum number of webhooks per workspace.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
📚 Learning: the queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:918-942
Timestamp: 2025-06-19T14:32:02.934Z
Learning: The queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` are expected to remain small, so per-row updates are acceptable and performance optimizations like set-based updates are not necessary.
Applied to files:
tests/deriver/test_queue_operations.pytests/crud/test_session.pytests/deriver/conftest.py
📚 Learning: the `queue` and `active_queue_sessions` tables in the database schema reference `sessions.id` (numer...
Learnt from: dr-frmr
PR: plastic-labs/honcho#106
File: migrations/versions/a1b2c3d4e5f6_initial_schema.py:172-179
Timestamp: 2025-05-13T15:48:42.783Z
Learning: The `queue` and `active_queue_sessions` tables in the database schema reference `sessions.id` (numeric) rather than `sessions.public_id` (text) because they are internal-only tables not exposed through the API, where performance considerations outweigh consistent identifier usage.
Applied to files:
tests/deriver/test_queue_operations.py
📚 Learning: the `get_or_create_session` function in this codebase is designed to handle both session creation an...
Learnt from: dr-frmr
PR: plastic-labs/honcho#131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The `get_or_create_session` function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.
Applied to files:
tests/deriver/test_queue_operations.pytests/crud/test_session.py
📚 Learning: in fastapi tests, it's common to use @pytest.mark.asyncio with a synchronous testclient. the async m...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Applied to files:
tests/deriver/test_deriver_processing.py
📚 Learning: applies to tests/**/*.py : tests in pytest with fixtures in tests/conftest.py...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to tests/**/*.py : Tests in pytest with fixtures in tests/conftest.py
Applied to files:
tests/deriver/test_deriver_processing.py
🧬 Code Graph Analysis (4)
tests/deriver/test_queue_operations.py (1)
tests/deriver/conftest.py (1)
create_active_queue_session(257-272)
tests/deriver/conftest.py (3)
src/deriver/utils.py (1)
get_work_unit_key(12-31)src/deriver/deriver.py (1)
Deriver(83-326)src/models.py (2)
QueueItem(366-378)ActiveQueueSession(382-391)
tests/deriver/test_queue_processing.py (3)
src/deriver/queue_manager.py (1)
QueueManager(28-339)src/deriver/consumer.py (1)
process_item(18-50)tests/deriver/conftest.py (1)
sample_session_with_peers(56-84)
tests/deriver/test_deriver_processing.py (2)
tests/deriver/conftest.py (1)
sample_session_with_peers(56-84)src/deriver/utils.py (1)
get_work_unit_key(12-31)
🪛 Ruff (0.12.2)
tests/deriver/conftest.py
23-23: Dynamically typed expressions (typing.Any) are disallowed in _self
(ANN401)
23-23: Trailing comma missing
Add trailing comma
(COM812)
tests/deriver/test_queue_processing.py
114-114: Missing return type annotation for public function test_work_unit_key_format
Add return type annotation: None
(ANN201)
115-115: Trailing comma missing
Add trailing comma
(COM812)
tests/deriver/test_deriver_processing.py
42-42: Missing return type annotation for public function test_work_unit_key_generation
Add return type annotation: None
(ANN201)
🔇 Additional comments (12)
tests/crud/test_session.py (2)
65-65: LGTM: Correct unpacking for new data structure.The unpacking correctly handles the addition of the
is_activeboolean field to the session peer configuration tuples, aligning with the broader work unit refactoring.
69-69: LGTM: Proper type validation for new field.The type assertion correctly validates that
is_activeis a boolean, maintaining test robustness for the updated data structure.tests/deriver/test_queue_operations.py (1)
73-90: LGTM! Test correctly validates the new work unit key format.The test has been properly updated to reflect the refactoring from WorkUnit objects to work_unit_key strings. The validation logic correctly verifies the colon-separated format and ensures all expected components are present.
tests/deriver/test_deriver_processing.py (1)
47-78: LGTM! Comprehensive testing of work unit key generation.The test correctly validates both representation and summary task types, including the proper handling of None values for sender/target names in summary tasks. The test logic aligns well with the
get_work_unit_keyutility function implementation.tests/deriver/test_queue_processing.py (4)
38-42: LGTM! Assertions correctly updated for string-based work unit keys.The test logic properly validates that work units are now strings with the expected task type prefixes, aligning with the refactoring changes.
62-63: LGTM! ActiveQueueSession creation updated correctly.The model instantiation now uses the simplified work_unit_key parameter, consistent with the database schema changes.
108-108: LGTM! process_item call updated with explicit task_type.The function call now includes the task_type parameter, matching the updated signature in
src/deriver/consumer.py.
117-141: LGTM! Comprehensive validation of work unit key formats.The test properly validates both representation and summary work unit key formats, including the correct handling of None values for summary tasks.
tests/deriver/conftest.py (4)
22-24: Mock function signature correctly updated.The mock function now includes the
_task_typeparameter, aligning with the updatedprocess_messagemethod signature in the actual implementation.Note: The static analysis hints about
Anytype annotation and trailing comma are minor style issues that don't affect functionality.
14-14: LGTM! Appropriate import for work unit key generation.The import of
get_work_unit_keyutility function is correctly added to support the new work unit key generation logic.
180-187: LGTM! Queue item creation updated for new schema.The fixture correctly generates both
task_typeandwork_unit_keyfields for each queue item, properly integrating with the new database schema and work unit key approach.
261-265: LGTM! Simplified ActiveQueueSession creation.The fixture has been correctly simplified to use only the
work_unit_keyparameter, reflecting the database schema changes that consolidated multiple fields into a single work unit key.
| WHERE s.id = q.session_id AND q.processed = false AND q.work_unit_key IS NULL | ||
| """ | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Make the queue work_unit column nullable after the transformation
There was a problem hiding this comment.
same thing with the task_type column
| class ParsedWorkUnit(TypedDict): | ||
| task_type: str | ||
| workspace_name: str | ||
| session_name: str | None | ||
| sender_name: str | None | ||
| target_name: str | None | ||
|
|
||
|
|
There was a problem hiding this comment.
Potential optimization for the future is to put this all into a pydantic object that can take in the work_unit_key and parse it automatically as well as handle the transform back and forth
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
67-77: Column addition strategy is correct.Adding columns as nullable initially before backfilling is the right approach for handling existing data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
migrations/versions/88b0fb10906f_add_webhooks_table.py(1 hunks)src/models.py(5 hunks)tests/routes/test_queue_status.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/routes/test_queue_status.py
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/models.py
src/models.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/models.py: Use explicit type hints with SQLAlchemy mapped_column annotations
All tables use text IDs (nanoid format) as primary keys
Composite foreign keys for multi-tenant relationships
Feature flags on workspace, peer, and session levels
JSONB metadata fields for extensibility
HNSW indexes for vector similarity search
Files:
src/models.py
src/{models,crud}.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Token counting on messages for usage tracking
Files:
src/models.py
🧠 Learnings (13)
📓 Common learnings
Learnt from: VVoruganti
PR: plastic-labs/honcho#168
File: src/routers/webhooks.py:22-39
Timestamp: 2025-07-30T19:44:32.513Z
Learning: The webhook system should enforce a maximum limit of webhooks per workspace, with a default of 10 webhooks per workspace. This limit should be configurable via the WEBHOOKS_MAX_WEBHOOKS_PER_WORKSPACE environment variable and implemented by checking the current webhook count before creating new ones.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#168
File: src/config.py:240-245
Timestamp: 2025-08-04T20:21:58.211Z
Learning: In the webhook settings implementation for plastic-labs/honcho, the field name MAX_WORKSPACE_LIMIT is used instead of MAX_WEBHOOKS_PER_WORKSPACE, which maps to the environment variable WEBHOOKS_MAX_WORKSPACE_LIMIT for configuring the maximum number of webhooks per workspace.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: src/crud.py:1250-1250
Timestamp: 2025-06-26T18:39:54.942Z
Learning: Rajat-Ahuja1997 is comfortable exposing token counts in MessageCreate schema as a public property when it improves code maintainability and avoids type checking errors.
📚 Learning: the queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:918-942
Timestamp: 2025-06-19T14:32:02.934Z
Learning: The queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` are expected to remain small, so per-row updates are acceptable and performance optimizations like set-based updates are not necessary.
Applied to files:
tests/routes/test_queue_status.pymigrations/versions/88b0fb10906f_add_webhooks_table.pysrc/models.py
📚 Learning: in fastapi tests, it's common to use @pytest.mark.asyncio with a synchronous testclient. the async m...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Applied to files:
tests/routes/test_queue_status.py
📚 Learning: the `get_or_create_session` function in this codebase is designed to handle both session creation an...
Learnt from: dr-frmr
PR: plastic-labs/honcho#131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The `get_or_create_session` function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.
Applied to files:
tests/routes/test_queue_status.py
📚 Learning: applies to src/models.py : feature flags on workspace, peer, and session levels...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Applied to files:
tests/routes/test_queue_status.pysrc/models.py
📚 Learning: the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` properly handles fore...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:36-77
Timestamp: 2025-06-19T14:07:32.309Z
Learning: The migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` properly handles foreign key constraints during table updates by explicitly dropping foreign key constraints before making schema changes, preventing constraint violations during the migration process.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: in the migration file `migrations/versions/d429de0e5338_adopt_peer_paradigm.py`, the team has explic...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:188-194
Timestamp: 2025-06-18T15:58:51.202Z
Learning: In the migration file `migrations/versions/d429de0e5338_adopt_peer_paradigm.py`, the team has explicitly decided to accept SQL injection risks from f-string interpolation of schema names, despite the security concerns around DDL injection via the DATABASE_SCHEMA environment variable.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.py
📚 Learning: in the honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not ...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:438-441
Timestamp: 2025-06-18T16:44:57.733Z
Learning: In the Honcho database schema, messages.app_id references workspaces.public_id (string nanoid), not workspaces.id (integer primary key). This means that during migrations when workspaces.public_id is renamed to workspaces.id, the foreign key relationship with messages.app_id remains valid.
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.pysrc/models.py
📚 Learning: applies to src/models.py : use explicit type hints with sqlalchemy mapped_column annotations...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Use explicit type hints with SQLAlchemy mapped_column annotations
Applied to files:
migrations/versions/88b0fb10906f_add_webhooks_table.pysrc/models.py
📚 Learning: the honcho repository should have com812 (trailing comma missing) rule disabled in the root pyprojec...
Learnt from: VVoruganti
PR: plastic-labs/honcho#164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository should have COM812 (trailing comma missing) rule disabled in the root pyproject.toml Ruff configuration by adding it to the existing ignore list: ignore = ["E501", "B008", "COM812"]. The maintainers do not want trailing comma suggestions in code reviews.
Applied to files:
src/models.py
📚 Learning: the honcho repository does not use trailing commas in function signatures, function calls, or object...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#144
File: tests/integration/test_message_embeddings.py:128-187
Timestamp: 2025-06-26T18:35:46.478Z
Learning: The honcho repository does not use trailing commas in function signatures, function calls, or object instantiation. This is consistently applied across the entire codebase including test files, and reviews should not suggest adding trailing commas.
Applied to files:
src/models.py
📚 Learning: the honcho repository maintainers do not want trailing comma suggestions or complaints from static a...
Learnt from: VVoruganti
PR: plastic-labs/honcho#164
File: sdks/python/src/honcho/async_client/peer.py:136-136
Timestamp: 2025-07-21T15:53:20.965Z
Learning: The honcho repository maintainers do not want trailing comma suggestions or complaints from static analysis tools like Ruff. The COM812 rule should be disabled in the Ruff configuration to avoid these suggestions in future reviews.
Applied to files:
src/models.py
📚 Learning: the `queue` and `active_queue_sessions` tables in the database schema reference `sessions.id` (numer...
Learnt from: dr-frmr
PR: plastic-labs/honcho#106
File: migrations/versions/a1b2c3d4e5f6_initial_schema.py:172-179
Timestamp: 2025-05-13T15:48:42.783Z
Learning: The `queue` and `active_queue_sessions` tables in the database schema reference `sessions.id` (numeric) rather than `sessions.public_id` (text) because they are internal-only tables not exposed through the API, where performance considerations outweigh consistent identifier usage.
Applied to files:
src/models.py
🧬 Code Graph Analysis (3)
tests/routes/test_queue_status.py (2)
src/deriver/utils.py (1)
get_work_unit_key(12-31)src/models.py (1)
QueueItem(366-378)
migrations/versions/88b0fb10906f_add_webhooks_table.py (1)
migrations/utils.py (4)
column_exists(19-29)constraint_exists(54-73)index_exists(43-51)table_exists(11-16)
src/models.py (1)
src/schemas.py (1)
WebhookEndpoint(386-392)
🪛 Ruff (0.12.2)
migrations/versions/88b0fb10906f_add_webhooks_table.py
82-107: Possible SQL injection vector through string-based query construction
(S608)
107-107: Trailing comma missing
Add trailing comma
(COM812)
108-108: Trailing comma missing
Add trailing comma
(COM812)
133-133: Trailing comma missing
Add trailing comma
(COM812)
136-136: Trailing comma missing
Add trailing comma
(COM812)
157-157: Trailing comma missing
Add trailing comma
(COM812)
175-175: Trailing comma missing
Add trailing comma
(COM812)
178-178: Trailing comma missing
Add trailing comma
(COM812)
src/models.py
390-390: Trailing comma missing
Add trailing comma
(COM812)
399-399: Trailing comma missing
Add trailing comma
(COM812)
403-403: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (13)
tests/routes/test_queue_status.py (4)
6-6: LGTM!The import of
get_work_unit_keyis correctly added to support the new work unit key generation in tests.
148-164: Correctly updated to use the new queue schema.The queue item creation properly sets
task_type, generateswork_unit_keyusing the utility function, and includes all required fields in the payload.
222-238: Consistent implementation of the new queue schema.The test correctly creates queue items with the required
task_typeandwork_unit_keyfields for multiple sessions.
285-296: LGTM!Queue item creation follows the established pattern with proper
task_typeandwork_unit_keyfields.migrations/versions/88b0fb10906f_add_webhooks_table.py (5)
33-64: Webhook endpoints table structure looks good.The table creation with constraints and indexing is well-designed for efficient webhook management.
79-109: Excellent data migration strategy.The batched backfill properly handles existing queue items by:
- Using COALESCE to handle missing fields gracefully
- Processing in 1000-row batches to avoid locking issues
- Generating work_unit_key in the expected format
This addresses the data transformation requirement mentioned in previous reviews.
111-113: Proper constraint enforcement after data migration.Making columns non-nullable after backfilling ensures data integrity without breaking existing records.
116-149: Safe and defensive migration of active_queue_sessions table.The use of inspector utilities to check existence before dropping prevents migration failures. The unique constraint on
work_unit_keyproperly enforces single active session per work unit.
152-186: Complete and safe downgrade implementation.The downgrade function properly reverses all schema changes with existence checks to ensure idempotent execution.
src/models.py (4)
3-3: Proper setup for webhook support.The Literal import and webhook_endpoints relationship are correctly added to support the new webhook functionality.
Also applies to: 86-86
362-362: Well-defined task type constraints.The TaskType literal ensures only valid task types can be used in the queue system.
374-376: Queue model correctly updated for new work unit tracking.The
work_unit_keyandtask_typefields properly replace the previous multi-field identification system.
387-387: Simplified work unit tracking in active sessions.The single
work_unit_keywith unique constraint properly replaces multiple identifying fields.
| @final | ||
| class WebhookEndpoint(Base): | ||
| __tablename__: str = "webhook_endpoints" | ||
| id: Mapped[str] = mapped_column(TEXT, default=generate_nanoid, primary_key=True) | ||
| workspace_name: Mapped[str] = mapped_column( | ||
| ForeignKey("workspaces.name"), index=True, nullable=False | ||
| ) | ||
| url: Mapped[str] = mapped_column(TEXT, nullable=False) | ||
| created_at: Mapped[datetime.datetime] = mapped_column( | ||
| DateTime(timezone=True), default=func.now() | ||
| ) | ||
|
|
||
| workspace = relationship("Workspace", back_populates="webhook_endpoints") | ||
|
|
||
| __table_args__ = ( | ||
| UniqueConstraint( | ||
| "session_id", | ||
| "sender_name", | ||
| "target_name", | ||
| "task_type", | ||
| name="unique_active_queue_session", | ||
| ), | ||
| CheckConstraint("length(url) <= 2048", name="webhook_endpoint_url_length"), | ||
| Index("idx_webhook_endpoints_workspace_lookup", "workspace_name"), | ||
| ) | ||
|
|
||
| def __repr__(self) -> str: | ||
| return f"WebhookEndpoint(id={self.id}, workspace_name={self.workspace_name}, url={self.url})" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
WebhookEndpoint model is well-structured.
The model properly defines webhook endpoints with appropriate constraints and indexing. Consider adding a status field in a future iteration to enable/disable webhooks without deletion, as suggested in previous reviews.
🧰 Tools
🪛 Ruff (0.12.2)
399-399: Trailing comma missing
Add trailing comma
(COM812)
403-403: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In src/models.py around lines 394 to 414, the WebhookEndpoint model is correctly
defined with constraints and indexing. No immediate changes are needed now, but
for future improvements, consider adding a status field to the model to allow
enabling or disabling webhook endpoints without deleting them.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores