Merged
Conversation
Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Reviewer's GuideReplaces Cursor-specific configuration with OpenCode-style agent and skill docs, expands the add-endpoint skill with more detailed guidance, introduces a new smoke-test skill and high-level AGENTS documentation, and removes legacy .cursor agents, commands, and rules files. Sequence diagram for the new smoke-tester skill workflowsequenceDiagram
actor Developer
participant Skill_smoke_tester
participant Podman
participant Container_switcher_webapi
participant Health_endpoint
Developer->>Skill_smoke_tester: Invoke smoke-tester instructions
Skill_smoke_tester->>Podman: podman build -t switcher_webapi:local .
Skill_smoke_tester->>Podman: podman run -d --name smoke-test -p 8000:8000 switcher_webapi:local
Podman->>Container_switcher_webapi: Start container switcher_webapi:local
loop Health_poll_attempts_1_to_12
Skill_smoke_tester->>Health_endpoint: HTTP GET /health
Health_endpoint-->>Skill_smoke_tester: HTTP status_code
end
alt Health_status_200
Skill_smoke_tester->>Podman: podman logs smoke-test
Skill_smoke_tester->>Podman: podman stop smoke-test
Skill_smoke_tester->>Podman: podman rm smoke-test
Skill_smoke_tester-->>Developer: Report success
else Health_status_not_200
Skill_smoke_tester-->>Developer: Report failure and logs
Skill_smoke_tester->>Podman: podman stop smoke-test
Skill_smoke_tester->>Podman: podman rm smoke-test
end
Flow diagram for using the updated add-endpoint skillflowchart TD
start[Start: Need_new_API_endpoint]
step1[Define_endpoint_path_constant_in_app_webapp_py]
step2[Create_async_handler_function_using_standard_pattern]
step2a[Extract_query_params_device_type_ip_id_login_key]
step2b[Open_SwitcherApi_async_context_manager]
step2c[Call_appropriate_aioswitcher_method]
step2d[Serialize_result_and_return_JSON]
step3[Add_pytest_asyncio_tests_in_app_tests]
step4[Update_MkDocs_endpoint_documentation_in_docs]
step5[Run_ruff_and_pytest_with_coverage]
end_node[Endpoint_added_and_verified]
start --> step1 --> step2
step2 --> step2a --> step2b --> step2c --> step2d --> step3
step3 --> step4 --> step5 --> end_node
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
Test Results69 tests 69 ✅ 1s ⏱️ Results for commit 41d2caa. ♻️ This comment has been updated with latest results. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The handler pattern description is now duplicated between
AGENTS.mdand.agents/skills/add-endpoint/SKILL.md; consider extracting this into a single canonical source to avoid divergence over time. - The smoke-tester skill assumes
podmanand a fixedlocalhost:8000mapping; if your environments vary, you might want to parameterize the container runtime and port to make this skill reusable across different setups. - AGENTS.md codifies several conventions (Ruff, type hints, coverage, Docker constraints); it may be worth cross-checking these against existing project configuration to ensure they stay aligned and don’t drift from actual tooling or policies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The handler pattern description is now duplicated between `AGENTS.md` and `.agents/skills/add-endpoint/SKILL.md`; consider extracting this into a single canonical source to avoid divergence over time.
- The smoke-tester skill assumes `podman` and a fixed `localhost:8000` mapping; if your environments vary, you might want to parameterize the container runtime and port to make this skill reusable across different setups.
- AGENTS.md codifies several conventions (Ruff, type hints, coverage, Docker constraints); it may be worth cross-checking these against existing project configuration to ensure they stay aligned and don’t drift from actual tooling or policies.
## Individual Comments
### Comment 1
<location path="AGENTS.md" line_range="20" />
<code_context>
-
-- All handlers must be `async def` and return `web.Response`
-- Use `@routes.get` / `@routes.post` / `@routes.patch` / `@routes.delete` decorators
-- Endpoint path constants defined at module level in `app/webapp.py`
-- Error handling must return appropriate HTTP status codes (400, 404, 500)
</code_context>
<issue_to_address>
**suggestion (typo):** Add a verb to this bullet for grammatical completeness.
For example: "Endpoint path constants are defined at module level in `app/webapp.py`."
```suggestion
- Endpoint path constants are defined at module level in `app/webapp.py`
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #988 +/- ##
=======================================
Coverage 96.07% 96.07%
=======================================
Files 1 1
Lines 331 331
=======================================
Hits 318 318
Misses 13 13 🚀 New features to boost your workflow:
|
auto-me-bot
Bot
added
status: review started
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCursor AI tooling configuration and documentation is consolidated from distributed ChangesDocumentation and Tooling Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 59 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/add-endpoint/SKILL.md (1)
60-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Incorrect test patching pattern.
Line 70 instructs patching
app.webapp.SwitcherApi, but the actual test suite patchesaioswitcher.api.SwitcherApimethods directly (e.g.,@patch("aioswitcher.api.SwitcherApi.get_state")). Developers following this guide will write tests that fail to mock correctly.Update the example to use the correct patching pattern:
Corrected test example
-with patch("app.webapp.SwitcherApi") as mock_api: - mock_api.return_value.__aenter__.return_value.your_method = AsyncMock( - return_value=expected_response - ) - resp = await api_client.get(url) - assert resp.status == 200 +@patch("aioswitcher.api.SwitcherApi.your_method") +async def test_successful_your_endpoint_get_request( + api_your_method, + api_client, +): + api_your_method.return_value = expected_response + resp = await api_client.get("/switcher/your_endpoint?type=plug&id=ab1c2d&ip=1.2.3.4") + api_your_method.assert_called_once() + assert resp.status == 200🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/add-endpoint/SKILL.md around lines 60 - 76, The test example patches the wrong symbol; update the patch target from app.webapp.SwitcherApi to the actual library class aioswitcher.api.SwitcherApi and mock the specific async method used by your endpoint (e.g., get_state or the relevant method name) via AsyncMock on that method; in the test function test_successful_your_endpoint_get_request replace patch("app.webapp.SwitcherApi") with patch("aioswitcher.api.SwitcherApi") and set mock_api.return_value.__aenter__.return_value.<your_method> = AsyncMock(return_value=expected_response) so api_client.get(...) sees the mocked async call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/smoke-tester/SKILL.md:
- Around line 37-43: The polling loop currently never fails explicitly; add a
healthy flag (e.g., healthy=0) before the for-loop, set healthy=1 and break when
response equals "200" (replace the short-circuit [ "$response" = "200" ] &&
break with an if that sets healthy), and after the loop check if healthy is not
1: print a failure message, run podman logs smoke-test || true, podman stop
smoke-test || true, podman rm smoke-test || true, and exit 1; otherwise run the
existing podman logs/stop/rm cleanup on success. Ensure the same change is
applied to both occurrences referenced (the blocks around the /health polling at
both locations).
In `@AGENTS.md`:
- Around line 16-21: The AGENTS.md constraint about error handling is out of
sync with the code: either update the documentation to reflect that exceptions
from handlers (ValueError, JSON decode errors, etc.) are currently caught by
error_middleware and translated into 500 responses, or implement consistent
handler-level validation so malformed requests return 400 and missing resources
return 404; to fix, choose one approach and apply it across the codebase—if
updating docs, change the line in AGENTS.md to state that error_middleware
converts uncaught exceptions into 500 and mention delete_schedule is the only
handler returning 404; if implementing consistent handling, add explicit
validation in each request handler that parses request bodies (the functions
that currently raise ValueError) to return web.Response(status=400) for bad
input and ensure resource-not-found cases return web.Response(status=404), and
update or remove error_middleware handling as needed to avoid converting these
to 500.
---
Outside diff comments:
In @.agents/skills/add-endpoint/SKILL.md:
- Around line 60-76: The test example patches the wrong symbol; update the patch
target from app.webapp.SwitcherApi to the actual library class
aioswitcher.api.SwitcherApi and mock the specific async method used by your
endpoint (e.g., get_state or the relevant method name) via AsyncMock on that
method; in the test function test_successful_your_endpoint_get_request replace
patch("app.webapp.SwitcherApi") with patch("aioswitcher.api.SwitcherApi") and
set mock_api.return_value.__aenter__.return_value.<your_method> =
AsyncMock(return_value=expected_response) so api_client.get(...) sees the mocked
async call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09e72f3e-2b13-46b3-98ae-bbbced235a5e
📒 Files selected for processing (20)
.agents/skills/add-endpoint/SKILL.md.agents/skills/smoke-tester/SKILL.md.cursor/agents/code-reviewer.md.cursor/agents/docs-writer.md.cursor/agents/smoke-tester.md.cursor/agents/test-writer.md.cursor/commands/dockerfile-lint.md.cursor/commands/image-build.md.cursor/commands/lint.md.cursor/commands/serve-docs.md.cursor/commands/stop-docs.md.cursor/commands/test.md.cursor/rules/architecture.mdc.cursor/rules/ci-cd.mdc.cursor/rules/cursor-tooling.mdc.cursor/rules/git-workflow.mdc.cursor/rules/project-overview.mdc.cursor/rules/python-conventions.mdc.cursor/rules/testing.mdcAGENTS.md
💤 Files with no reviewable changes (17)
- .cursor/rules/project-overview.mdc
- .cursor/commands/test.md
- .cursor/rules/testing.mdc
- .cursor/commands/serve-docs.md
- .cursor/rules/architecture.mdc
- .cursor/rules/git-workflow.mdc
- .cursor/rules/ci-cd.mdc
- .cursor/commands/stop-docs.md
- .cursor/agents/test-writer.md
- .cursor/rules/python-conventions.mdc
- .cursor/rules/cursor-tooling.mdc
- .cursor/commands/lint.md
- .cursor/agents/smoke-tester.md
- .cursor/commands/dockerfile-lint.md
- .cursor/agents/docs-writer.md
- .cursor/commands/image-build.md
- .cursor/agents/code-reviewer.md
Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Owner
Author
|
@CodeRabbit resoble all - fixed in 41d2caa |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] |
auto-me-bot
Bot
added
status: merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit