Ensure command services require async handlers

[matdev83]

Summary

• add an async-callable guard for command service handlers and wrap adaptors accordingly
• extend command service module tests to cover synchronous handler rejection

Testing

• pytest --no-testmon tests/unit/core/test_command_service_module.py

---

https://chatgpt.com/codex/tasks/task_e_68ec2699ab208333aa457b9299c22d89

Summary by CodeRabbit

• Bug Fixes

  • Improved validation for command service handlers to require async callables, preventing misconfiguration and related runtime issues.
  • Clearer error messages when a handler is missing or not async, aiding quicker troubleshooting.

• Tests

  • Added unit tests ensuring non-async callables are rejected and that async callables (including partials/wrapped async functions) are accepted.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds detection and enforcement that command service handlers are async callables. FunctionCommandService.init and ensure_command_service now validate async-ness (including partials and bound callables) and raise TypeError for sync or None handlers. Tests cover acceptance of async partials and rejection of sync callables.

Changes

Cohort / File(s)	Summary
Command service async validation src/core/interfaces/command_service.py	Add internal helper _is_async_callable using asyncio.iscoroutinefunction; import renamed helper alias; validate handler is callable and async in FunctionCommandService.__init__; ensure_command_service checks async-ness before wrapping; raise TypeError or ValueError for invalid/None handlers.
Unit tests for validation tests/unit/core/test_command_service_module.py	Add functools.partial import and tests: test_ensure_command_service_accepts_partial_async_callable (partial of async callable is accepted) and test_ensure_command_service_rejects_sync_callable (sync callable causes TypeError mentioning "async").

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Ensure as ensure_command_service
  participant FCS as FunctionCommandService
  participant Handler as Handler (callable)

  Caller->>Ensure: provide handler
  Note right of Ensure: _is_async_callable checks callable form\n(direct, bound, partial, __call__/__func__)
  alt handler is async
    Ensure->>FCS: instantiate with handler
    FCS-->>Ensure: instance
    Ensure-->>Caller: CommandService instance
    Caller->>FCS: execute(message)
    FCS->>Handler: await handler(message)
    Handler-->>FCS: result
    FCS-->>Caller: result
  else handler is sync or None
    Ensure-->>Caller: raise TypeError / ValueError
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I twitch my whiskers, sniff the change,
Async I seek across each range.
Partials, bounds — I check them all,
No blocking thumps, no sleepy fall.
Tests hop by and cheer: "All clear!" 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely describes the primary change of enforcing async handlers for command services, matching the PR’s focus on adding an async-callable guard and related tests. It is specific enough for a reviewer or teammate to understand the main intent without extraneous detail or generic wording. The phrasing is clear and avoids unnecessary noise.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/fix-identified-frontend-bug

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

• @codex fix this CI failure
• @codex address that feedback

[chatgpt-codex-connector]

Guard misclassifies async partials as synchronous

The new _is_async_callable check only calls inspect.iscoroutinefunction on the candidate or its __call__ method. Objects such as functools.partial(async_fn, ...) are still fully awaitable (the wrapped function is an async def), but isocoroutinefunction returns False for partials because their __call__ is implemented in C. As a result ensure_command_service now raises TypeError for valid async handlers that were previously accepted and work with the await in FunctionCommandService.process_commands. Any existing callers that bind parameters via functools.partial or similar callables will regress. Consider checking iscoroutinefunction(getattr(candidate, "func", candidate)) or using asyncio.iscoroutinefunction which handles partials.

Useful? React with 👍 / 👎.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/core/interfaces/command_service.py (1)

33-36: Remove redundant async validation in FunctionCommandService.init

The ensure_command_service public API already enforces this check and there are no other direct instantiations of FunctionCommandService.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd216cf and 95feb52.

📒 Files selected for processing (2)

• src/core/interfaces/command_service.py (4 hunks)
• tests/unit/core/test_command_service_module.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting

Files:

• tests/unit/core/test_command_service_module.py
• src/core/interfaces/command_service.py

src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions

Files:

• src/core/interfaces/command_service.py

🧬 Code graph analysis (2)

tests/unit/core/test_command_service_module.py (2)

src/core/domain/processed_result.py (1)

• ProcessedResult (8-19)

src/core/interfaces/command_service.py (1)

• ensure_command_service (45-75)

src/core/interfaces/command_service.py (1)

tests/unit/core/test_command_service_module.py (2)

• handler (34-39)
• handler (52-57)

🪛 Ruff (0.13.3)

src/core/interfaces/command_service.py

19-19: Using hasattr(x, "__call__") to test if x is callable is unreliable. Use callable(x) for consistent results.

(B004)

---

34-36: Avoid specifying long messages outside the exception class

(TRY003)

---

70-72: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: test (3.10)
• GitHub Check: Analyze (python)

🔇 Additional comments (4)

tests/unit/core/test_command_service_module.py (1)

51-63: LGTM! Test correctly verifies synchronous callable rejection.

The test follows the existing pattern, properly validates that synchronous handlers are rejected with a TypeError containing "async", and doesn't require the @pytest.mark.asyncio decorator since it performs no async operations.

src/core/interfaces/command_service.py (3)

4-4: LGTM! Correct import for async detection.

The iscoroutinefunction import from Python's inspect module is properly placed and necessary for the async validation logic.

---

13-21: LGTM! Async detection logic is correct.

The helper properly detects both async functions (fast path) and callable objects with async __call__ methods. The use of getattr with a None default is appropriate, and the static analysis hint about hasattr on line 19 is a false positive.

---

69-72: LGTM! Validation logic is correct.

The async validation correctly rejects synchronous callables before wrapping them in FunctionCommandService. The error message clearly indicates the requirement for async callables and matches the test expectations.

Note: This validation duplicates the check in FunctionCommandService.__init__ (lines 33-36). See the previous comment for details on potential refactoring.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/core/interfaces/command_service.py (1)

13-31: Comprehensive async-callable detection.

The implementation thoroughly covers multiple callable patterns (direct functions, partials, callable objects, bound methods). The layered checks ensure robust detection across edge cases.

Optional: Refine docstring terminology.

Consider replacing "awaitable callable" with "async callable" or "coroutine function" for precision. While "awaitable callable" is understandable in context, it technically conflates callable type (async function) with return type (awaitable/coroutine).

-    """Return ``True`` when *candidate* is an awaitable callable."""
+    """Return ``True`` when *candidate* is an async callable (coroutine function)."""

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95feb52 and 10a1d15.

📒 Files selected for processing (2)

• src/core/interfaces/command_service.py (4 hunks)
• tests/unit/core/test_command_service_module.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit/core/test_command_service_module.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting

Files:

• src/core/interfaces/command_service.py

src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions

Files:

• src/core/interfaces/command_service.py

🔇 Additional comments (2)

src/core/interfaces/command_service.py (2)

3-3: Import addresses previous partial-handling concern.

Using asyncio.iscoroutinefunction instead of inspect.iscoroutinefunction correctly handles functools.partial objects wrapping async functions. This resolves the critical issue flagged in the previous review where async partials were misclassified as synchronous.

---

44-47: Validation correctly enforces async handler requirement.

Both validation points properly check async-ness after confirming the handler is callable. The error messages are clear and specific, adhering to the project's coding guidelines.

Also applies to: 80-83