Fix 868 #869
Fix 868 #869
Conversation
Signed-off-by: JonahSussman <[email protected]>
Signed-off-by: JonahSussman <[email protected]>
Signed-off-by: JonahSussman <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
kai_mcp_solution_server/tests/mcp_loader_script.py (1)
180-180: Harden JSON shape before iterating.Guard against malformed entries to avoid KeyError on item["name"]/["args"] and provide clearer errors.
Apply this diff:
- sequence: list[dict[str, Any]] = json.loads(content) + sequence: list[dict[str, Any]] = json.loads(content) + if not isinstance(sequence, list): + raise ValueError("Sequence JSON must be a list") + for i, item in enumerate(sequence): + if not isinstance(item, dict) or "name" not in item or "args" not in item: + raise ValueError(f"Invalid sequence item at index {i}: expected dict with 'name' and 'args'")kai_mcp_solution_server/tests/mcp_client.py (1)
66-67: Good: parse ID via TextContent; consider a small helper to DRY.Both blocks do identical parsing; a tiny util reduces duplication and error handling.
Example:
def _first_text(result) -> str: return cast(TextContent, result[0]).text if result else "" incident_id = int(_first_text(result))Also applies to: 153-154
kai_mcp_solution_server/tests/ssl_utils.py (1)
97-122: Optional: preserve metadata on patched inits and narrow ignores.Use functools.wraps to keep name/doc, and prefer targeted ignore codes. Current approach works.
Example:
from functools import wraps @wraps(self.original_httpx_client_init) # type: ignore[arg-type] def patched_client_init(client_self: httpx.Client, *args: Any, **kwargs: Any) -> None: kwargs["verify"] = False cast(Callable[..., None], self.original_httpx_client_init)(client_self, *args, **kwargs) # type: ignore[arg-type]Also applies to: 160-164
kai_mcp_solution_server/tests/test_multiple_integration.py (3)
29-43: Don't close an event loop you didn't create.In rare cases get_event_loop() could return a pre-existing loop; only close loops created here.
Apply this diff:
def run_async_in_thread( fn: Callable[..., Awaitable[T]], *args: Any, **kwargs: Any ) -> T: - try: - loop = asyncio.get_event_loop() - except RuntimeError: - loop = asyncio.new_event_loop() - asyncio.set_event_loop(loop) + created_loop = False + try: + loop = asyncio.get_event_loop() + except RuntimeError: + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + created_loop = True try: result = loop.run_until_complete(fn(*args, **kwargs)) return result finally: - loop.close() + if created_loop: + loop.close()
206-209: Factor out JSON-from-TextContent parsing to reduce repetition.Multiple blocks decode cast(TextContent, resp.content[0]).text then json.loads(...).
Add once:
def parse_json_content(resp) -> Any: return json.loads(cast(TextContent, resp.content[0]).text)Then:
metric = SuccessRateMetric(**parse_json_content(get_success_rate)) best_hint = GetBestHintResult(**parse_json_content(get_best_hint))Also applies to: 242-244, 330-332, 366-368, 389-391
433-439: Scope SSL bypass to the client task.Use the context manager so patches are restored even on error.
Apply this diff:
- print(f"[Client {client_id}] starting") - apply_ssl_bypass() - - client = Client( - transport=f"http://{multiple_user_mcp_args.host}:{multiple_user_mcp_args.port}", - ) + print(f"[Client {client_id}] starting") + with apply_ssl_bypass(): + client = Client( + transport=f"http://{multiple_user_mcp_args.host}:{multiple_user_mcp_args.port}", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py(2 hunks)kai_mcp_solution_server/tests/mcp_client.py(3 hunks)kai_mcp_solution_server/tests/mcp_loader_script.py(2 hunks)kai_mcp_solution_server/tests/ssl_utils.py(4 hunks)kai_mcp_solution_server/tests/test_multiple_integration.py(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (1)
kai_mcp_solution_server/src/kai_mcp_solution_server/constants.py (1)
log(9-10)
kai_mcp_solution_server/tests/test_multiple_integration.py (2)
kai_mcp_solution_server/ts-mcp-client/src/types/sdk.d.ts (1)
CallToolResult(22-24)kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (4)
SuccessRateMetric(722-728)get_success_rate(731-787)GetBestHintResult(664-666)get_best_hint(669-697)
🪛 Ruff (0.12.2)
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py
162-162: Use raise without specifying exception name
Remove exception name
(TRY201)
kai_mcp_solution_server/tests/test_multiple_integration.py
473-475: Abstract raise to an inner function
(TRY301)
473-475: Create your own exception
(TRY002)
473-475: Avoid specifying long messages outside the exception class
(TRY003)
482-484: Create your own exception
(TRY002)
482-484: Avoid specifying long messages outside the exception class
(TRY003)
517-519: Abstract raise to an inner function
(TRY301)
517-519: Create your own exception
(TRY002)
517-519: Avoid specifying long messages outside the exception class
(TRY003)
526-528: Create your own exception
(TRY002)
526-528: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Check Types on PR
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py
[error] 150-150: Mypy failed during './run_mypy.sh' step: Unused 'type: ignore' comment (unused-ignore).
[error] 166-166: Mypy failed during './run_mypy.sh' step: Unused 'type: ignore' comment (unused-ignore).
[error] 168-168: Mypy failed during './run_mypy.sh' step: Unused 'type: ignore' comment (unused-ignore).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
🔇 Additional comments (5)
kai_mcp_solution_server/tests/mcp_loader_script.py (1)
9-9: Import change is fine.Adding Any is consistent with the JSON payload handling below.
kai_mcp_solution_server/tests/mcp_client.py (1)
16-16: Type imports LGTM.cast/TextContent imports align with fastmcp return types.
Also applies to: 20-21
kai_mcp_solution_server/tests/test_multiple_integration.py (2)
66-68: Path handling LGTM.Using pathlib.Path for server_path improves clarity and typing.
668-689: Concurrency harness looks good.Fail-fast behavior with as_completed and self.fail is appropriate for stress runs.
kai_mcp_solution_server/src/kai_mcp_solution_server/server.py (1)
150-150: Fix CI: remove unused type: ignore, use bare raise, and make ctx cleanup typed and safe.Found
# type: ignorein kai_mcp_solution_server/src/kai_mcp_solution_server/server.py at lines 150, 166, 168 (also occurrences at 741 and 884)../run_mypy.shwas not found so mypy couldn't be executed — re-run CI/mypy after applying the diff below.async def kai_solution_server_lifespan( server: FastMCP[KaiSolutionServerContext], ) -> AsyncIterator[KaiSolutionServerContext]: log("kai_solution_server_lifespan") - try: - settings = SolutionServerSettings() # type: ignore + ctx: KaiSolutionServerContext | None = None + try: + settings = SolutionServerSettings() log(f"Settings: {settings.model_dump_json(indent=2)}") log("creating context") - ctx = KaiSolutionServerContext(settings) + ctx = KaiSolutionServerContext(settings) await ctx.create() log("yielding context") yield ctx - except Exception as e: + except Exception: log(f"Error in lifespan: {traceback.format_exc()}") - raise e + raise finally: # Clean up database connections when client disconnects - if "ctx" in locals() and hasattr(ctx, "engine"): # type: ignore - log("Disposing database engine...") - await ctx.engine.dispose() # type: ignore - log("Database engine disposed") + if ctx is not None: + engine = getattr(ctx, "engine", None) + if engine is not None: + log("Disposing database engine...") + await engine.dispose() + log("Database engine disposed")Optional follow-up (add precise engine/session typing in KaiSolutionServerContext.init):
# in __init__ from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession, async_sessionmaker self.engine: AsyncEngine | None = None self.session_maker: async_sessionmaker[AsyncSession] | None = None
Signed-off-by: JonahSussman <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: JonahSussman <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: JonahSussman <[email protected]>
Signed-off-by: JonahSussman <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: JonahSussman <[email protected]>
|
@coderabbitai help |
This comment was marked as outdated.
This comment was marked as outdated.
|
@coderabbitai pause |
Signed-off-by: JonahSussman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/stress-test-mcp-server.yml (1)
96-101: psql may not exist on ubuntu-latest; this ‘always()’ step can fail the job post‑success.Install the client or gate the step to only run on failures.
Option A — install psql client:
- name: Check PostgreSQL connection count if: always() + env: + DEBIAN_FRONTEND: noninteractive run: | + sudo apt-get update -y + sudo apt-get install -y postgresql-client PGPASSWORD=kai_password psql -h localhost -U kai_user -d kai_test_db -c \ "SELECT count(*), state FROM pg_stat_activity GROUP BY state;"Option B — only run on failure (no install):
- - name: Check PostgreSQL connection count - if: always() + - name: Check PostgreSQL connection count (on failure) + if: failure() run: | PGPASSWORD=kai_password psql -h localhost -U kai_user -d kai_test_db -c \ "SELECT count(*), state FROM pg_stat_activity GROUP BY state;"
♻️ Duplicate comments (1)
.github/workflows/stress-test-mcp-server.yml (1)
3-5: Don’t run this heavy stress workflow on every push/PR; narrow triggers + add safeguards.Scope by branch/paths, keep manual runs, and add permissions/concurrency to cut CI cost/flakiness. This mirrors an earlier comment; reiterating with a concrete patch.
Apply this minimal trigger refactor:
-on: - pull_request: {} - push: {} +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + branches: [main] + paths: + - 'kai_mcp_solution_server/**' + - '.github/workflows/stress-test-mcp-server.yml' + push: + branches: [main] + paths: + - 'kai_mcp_solution_server/**' + - '.github/workflows/stress-test-mcp-server.yml'Optional hardening (recommended):
name: MCP Server Stress Test -on: +on: # ... (as above) +permissions: + contents: read + jobs: stress-test-postgres: + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
🧹 Nitpick comments (1)
.github/workflows/stress-test-mcp-server.yml (1)
64-64: Make --root-path conditional; passing an empty value can break uvicorn/ASGI routing.Only set --root-path when MOUNT_PATH is non-empty.
- uv run python -m uvicorn kai_mcp_solution_server.server:app --host 0.0.0.0 --port 8000 --root-path=${MOUNT_PATH:-} & + ROOT_PATH_ARG="" + if [[ -n "${MOUNT_PATH:-}" ]]; then ROOT_PATH_ARG="--root-path=${MOUNT_PATH}"; fi + uv run python -m uvicorn kai_mcp_solution_server.server:app --host 0.0.0.0 --port 8000 ${ROOT_PATH_ARG} &
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/stress-test-mcp-server.yml(2 hunks)kai_mcp_solution_server/tools/deploy/Containerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kai_mcp_solution_server/tools/deploy/Containerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
✅ Actions performedReviews paused. |
|
Closed in favor of #878 |
|
Actually we may still want this, never mind |
2 participants
Summary by CodeRabbit
New Features
Tests
Refactor
Chores