feat: add kanban protocols and hook events (praisonai-agents core SDK)#1721
Conversation
- Add KanbanStoreProtocol in praisonaiagents/kanban/protocols.py - Add VALID_KANBAN_STATUSES constant matching PraisonAIUI columns - Add 6 kanban hook events to HookEvent enum (KANBAN_TASK_*) - Add KanbanHookInput dataclass for typed payloads - Add comprehensive unit tests covering all functionality - Follow AGENTS.md protocol-driven design (protocols in core, implementations in wrapper) - No new dependencies or heavy implementations Fixes #1719 Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR establishes protocol-first kanban contracts in the praisonaiagents core SDK by defining ChangesKanban Protocol Contracts & Hook Events
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/5480309b-3d48-4bd9-b55f-89dc11e39468 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/5480309b-3d48-4bd9-b55f-89dc11e39468 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/praisonai-agents/praisonaiagents/kanban/protocols.py`:
- Around line 58-164: The protocol currently exposes only sync store methods
(get_board, get_task, create_task, update_task, move_task, bulk_update,
delete_task, list_events, health); add async counterparts for each (e.g.,
aget_board, aget_task, acreate_task, aupdate_task, amove_task, abulk_update,
adelete_task, alist_events, ahealth) with identical signatures but declared
async and returning the same types, or alternatively create a separate
AsyncKanbanStore protocol/interface exposing those async methods; ensure
implementations are expected to perform non-blocking I/O (use asyncio
primitives) and do not block the event loop when implementing these methods.
- Around line 166-204: The KanbanStoreProtocol currently marked
`@runtime_checkable` exposes add_comment, link_tasks, and unlink_tasks as required
at runtime even though they are optional; split these into one or two extension
protocols (e.g., KanbanCommentingProtocol for add_comment and
KanbanLinkingProtocol for link_tasks/unlink_tasks), remove those methods from
KanbanStoreProtocol, and keep KanbanStoreProtocol as the runtime-checkable base;
create the new protocols (optionally decorated with `@runtime_checkable` if you
need isinstance checks on them) that declare add_comment, link_tasks, and
unlink_tasks and update any adapters/wrappers to implement the appropriate
extension protocol instead of adding optional methods to the base.
In `@src/praisonai-agents/tests/unit/test_kanban_protocol.py`:
- Around line 1-234: Add a new integration test in this file that actually runs
the agent through the kanban feature: create a real Agent instance, call
agent.start(...) with a prompt that triggers the kanban flow (e.g., "create a
kanban task titled X in board Y"), await the returned text output from
agent.start(), and assert the output contains expected model-generated text
(e.g., the task title/confirmation and kanban-related keywords). Also verify any
emitted hook payloads by checking KanbanHookInput fields or HookEvent values if
the agent exposes hooks (use KanbanHookInput and HookEvent to validate payload
shape). Name the test clearly (e.g., test_kanban_agent_real_run) and keep it in
this test module so it runs with the rest of the kanban tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f5a84ba-4121-462c-b0bf-83069f03a2d5
📒 Files selected for processing (4)
src/praisonai-agents/praisonaiagents/hooks/types.pysrc/praisonai-agents/praisonaiagents/kanban/__init__.pysrc/praisonai-agents/praisonaiagents/kanban/protocols.pysrc/praisonai-agents/tests/unit/test_kanban_protocol.py
| def get_board( | ||
| self, | ||
| *, | ||
| board: str = "default", | ||
| tenant: str | None = None, | ||
| include_archived: bool = False, | ||
| ) -> dict: | ||
| """Get board data with tasks grouped by status. | ||
|
|
||
| Args: | ||
| board: Board name (default: "default") | ||
| tenant: Optional tenant filter | ||
| include_archived: Whether to include archived tasks | ||
|
|
||
| Returns: | ||
| Board data with tasks organized by status columns | ||
| """ | ||
| ... | ||
|
|
||
| def get_task(self, task_id: str) -> dict | None: | ||
| """Get a single task by ID. | ||
|
|
||
| Args: | ||
| task_id: Task identifier | ||
|
|
||
| Returns: | ||
| Task data or None if not found | ||
| """ | ||
| ... | ||
|
|
||
| def create_task(self, data: dict) -> dict: | ||
| """Create a new task. | ||
|
|
||
| Args: | ||
| data: Task creation data (title, body, status, etc.) | ||
|
|
||
| Returns: | ||
| Created task data with assigned ID | ||
| """ | ||
| ... | ||
|
|
||
| def update_task(self, task_id: str, data: dict) -> dict | None: | ||
| """Update an existing task. | ||
|
|
||
| Args: | ||
| task_id: Task identifier | ||
| data: Fields to update | ||
|
|
||
| Returns: | ||
| Updated task data or None if not found | ||
| """ | ||
| ... | ||
|
|
||
| def move_task(self, task_id: str, status: str) -> dict | None: | ||
| """Move a task to a different status. | ||
|
|
||
| Args: | ||
| task_id: Task identifier | ||
| status: New status (must be in VALID_KANBAN_STATUSES) | ||
|
|
||
| Returns: | ||
| Updated task data or None if not found | ||
| """ | ||
| ... | ||
|
|
||
| def bulk_update(self, task_ids: list[str], status: str) -> dict: | ||
| """Update multiple tasks to the same status. | ||
|
|
||
| Args: | ||
| task_ids: List of task identifiers | ||
| status: New status for all tasks | ||
|
|
||
| Returns: | ||
| Results summary with success/failure counts | ||
| """ | ||
| ... | ||
|
|
||
| def delete_task(self, task_id: str) -> bool: | ||
| """Delete a task. | ||
|
|
||
| Args: | ||
| task_id: Task identifier | ||
|
|
||
| Returns: | ||
| True if deleted, False if not found | ||
| """ | ||
| ... | ||
|
|
||
| def list_events(self, since: float = 0.0, board: str = "default") -> list[dict]: | ||
| """List kanban events since a timestamp. | ||
|
|
||
| Args: | ||
| since: Unix timestamp to filter events from | ||
| board: Board name to filter events | ||
|
|
||
| Returns: | ||
| List of event data | ||
| """ | ||
| ... | ||
|
|
||
| def health(self) -> dict: | ||
| """Check store health status. | ||
|
|
||
| Returns: | ||
| Health status information | ||
| """ | ||
| ... |
There was a problem hiding this comment.
Add async variants for store I/O contract methods.
Line 58-Line 164 define only synchronous store operations (get_board, get_task, create_task, etc.). For core SDK contracts, this forces async callers toward blocking adapters and violates the SDK async I/O requirement. Add async counterparts (e.g., aget_task, acreate_task, etc.) or a dedicated async protocol.
As per coding guidelines, All I/O operations must have both sync and async variants; never block the event loop with sync I/O in async context; use asyncio primitives for coordination, not threading.
🤖 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 `@src/praisonai-agents/praisonaiagents/kanban/protocols.py` around lines 58 -
164, The protocol currently exposes only sync store methods (get_board,
get_task, create_task, update_task, move_task, bulk_update, delete_task,
list_events, health); add async counterparts for each (e.g., aget_board,
aget_task, acreate_task, aupdate_task, amove_task, abulk_update, adelete_task,
alist_events, ahealth) with identical signatures but declared async and
returning the same types, or alternatively create a separate AsyncKanbanStore
protocol/interface exposing those async methods; ensure implementations are
expected to perform non-blocking I/O (use asyncio primitives) and do not block
the event loop when implementing these methods.
| # Optional P4 extensions - define in protocol now, implement in wrapper later | ||
| # Use default implementations or NotImplementedError in adapters | ||
|
|
||
| def add_comment(self, task_id: str, text: str, author: str | None = None) -> dict | None: | ||
| """Add a comment to a task (optional P4 extension). | ||
|
|
||
| Args: | ||
| task_id: Task identifier | ||
| text: Comment text | ||
| author: Optional comment author | ||
|
|
||
| Returns: | ||
| Comment data or None if task not found | ||
| """ | ||
| raise NotImplementedError("Comment functionality not implemented") | ||
|
|
||
| def link_tasks(self, parent_id: str, child_id: str) -> bool: | ||
| """Link two tasks in parent-child relationship (optional P4 extension). | ||
|
|
||
| Args: | ||
| parent_id: Parent task identifier | ||
| child_id: Child task identifier | ||
|
|
||
| Returns: | ||
| True if linked successfully, False otherwise | ||
| """ | ||
| raise NotImplementedError("Task linking not implemented") | ||
|
|
||
| def unlink_tasks(self, parent_id: str, child_id: str) -> bool: | ||
| """Unlink parent-child task relationship (optional P4 extension). | ||
|
|
||
| Args: | ||
| parent_id: Parent task identifier | ||
| child_id: Child task identifier | ||
|
|
||
| Returns: | ||
| True if unlinked successfully, False otherwise | ||
| """ | ||
| raise NotImplementedError("Task unlinking not implemented") No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python typing.Protocol with @runtime_checkable, do methods declared on the protocol (including ones with default implementations) count as required attributes for isinstance structural checks?
💡 Result:
Yes, when using a @runtime_checkable Protocol, methods and attributes defined in the Protocol are considered required members for the purposes of an isinstance structural check [1][2]. At runtime, the isinstance check essentially verifies that the object possesses the specified attributes and methods by name [1][3]. Specifically: 1. Existence vs. Signatures: The runtime check only verifies the presence of the required members (e.g., using hasattr internally) [1][2]. It does not validate the type signatures, argument counts, or default implementations [1][3]. 2. Default Implementations: If a method in a Protocol provides a default implementation (by having a function body rather than just...), that method is still considered a required member for the isinstance check [1]. The runtime check simply verifies that the object has a member with that name; it does not care whether that member was inherited from the Protocol or implemented by the object itself [1][4]. 3. Limitations: Because these checks are "rudimentary," they are not fully type-safe [1][5]. The runtime check will return True if the object has an attribute of the correct name, even if that attribute is not a callable method or has a completely different signature than the one defined in the Protocol [1][2]. For these reasons, official documentation often notes that isinstance with Protocols is not completely safe at runtime and advises using it with caution [1][5]. For stricter validation, you may need to use tools like the inspect module to manually verify signatures if you require runtime assurance of functionality [3].
Citations:
- 1: https://typing.python.org/en/latest/reference/protocols.html
- 2: https://mypy.readthedocs.io/en/stable/protocols.html
- 3: https://stackoverflow.com/questions/68280688/how-can-i-make-isinstance-on-protocols-also-include-function-signatures-and-da
- 4: https://www.turingtaco.com/protocols-default-methods-inheritance-and-more/
- 5: https://typing.python.org/en/latest/spec/protocol.html
Split “optional” KanbanStoreProtocol methods into extension protocols.
Because KanbanStoreProtocol is @runtime_checkable, isinstance(..., KanbanStoreProtocol) treats members declared on the protocol as required by name; add_comment, link_tasks, and unlink_tasks therefore aren’t optional at runtime even though they raise NotImplementedError. Split into a base required protocol plus dedicated extension protocol(s), or stop making the base protocol @runtime_checkable.
File: src/praisonai-agents/praisonaiagents/kanban/protocols.py (lines 166-204)
🤖 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 `@src/praisonai-agents/praisonaiagents/kanban/protocols.py` around lines 166 -
204, The KanbanStoreProtocol currently marked `@runtime_checkable` exposes
add_comment, link_tasks, and unlink_tasks as required at runtime even though
they are optional; split these into one or two extension protocols (e.g.,
KanbanCommentingProtocol for add_comment and KanbanLinkingProtocol for
link_tasks/unlink_tasks), remove those methods from KanbanStoreProtocol, and
keep KanbanStoreProtocol as the runtime-checkable base; create the new protocols
(optionally decorated with `@runtime_checkable` if you need isinstance checks on
them) that declare add_comment, link_tasks, and unlink_tasks and update any
adapters/wrappers to implement the appropriate extension protocol instead of
adding optional methods to the base.
| """ | ||
| Unit tests for kanban protocols and hook events. | ||
|
|
||
| Tests the protocol contracts and hook events without any heavy | ||
| implementations like SQLite or LLM calls. | ||
| """ | ||
|
|
||
| import pytest | ||
| from typing import runtime_checkable | ||
|
|
||
| from praisonaiagents.kanban.protocols import ( | ||
| KanbanStoreProtocol, | ||
| KanbanTaskProtocol, | ||
| VALID_KANBAN_STATUSES, | ||
| ) | ||
| from praisonaiagents.hooks.types import ( | ||
| HookEvent, | ||
| KanbanHookInput, | ||
| ) | ||
|
|
||
|
|
||
| class TestKanbanProtocol: | ||
| """Test KanbanStoreProtocol runtime checking and constants.""" | ||
|
|
||
| def test_kanban_store_protocol_is_runtime_checkable(self): | ||
| """Test that KanbanStoreProtocol is @runtime_checkable.""" | ||
| assert runtime_checkable(KanbanStoreProtocol) | ||
|
|
||
| def test_valid_kanban_statuses_includes_ui_columns(self): | ||
| """Test that VALID_KANBAN_STATUSES includes all PraisonAIUI columns.""" | ||
| expected_statuses = { | ||
| "triage", | ||
| "todo", | ||
| "ready", | ||
| "running", | ||
| "blocked", | ||
| "review", | ||
| "done", | ||
| "archived" | ||
| } | ||
| assert VALID_KANBAN_STATUSES == expected_statuses | ||
|
|
||
| def test_valid_kanban_statuses_is_frozen(self): | ||
| """Test that VALID_KANBAN_STATUSES is immutable.""" | ||
| assert isinstance(VALID_KANBAN_STATUSES, frozenset) | ||
|
|
||
| def test_mock_store_passes_isinstance_check(self): | ||
| """Test that a mock store implementing the protocol passes isinstance check.""" | ||
|
|
||
| class MockKanbanStore: | ||
| """Mock implementation for testing.""" | ||
|
|
||
| def get_board(self, *, board="default", tenant=None, include_archived=False): | ||
| return {"board": board, "tasks": []} | ||
|
|
||
| def get_task(self, task_id: str): | ||
| return {"id": task_id, "title": "Test Task"} | ||
|
|
||
| def create_task(self, data: dict): | ||
| return {"id": "task_123", **data} | ||
|
|
||
| def update_task(self, task_id: str, data: dict): | ||
| return {"id": task_id, **data} | ||
|
|
||
| def move_task(self, task_id: str, status: str): | ||
| return {"id": task_id, "status": status} | ||
|
|
||
| def bulk_update(self, task_ids: list[str], status: str): | ||
| return {"updated": len(task_ids), "status": status} | ||
|
|
||
| def delete_task(self, task_id: str): | ||
| return True | ||
|
|
||
| def list_events(self, since: float = 0.0, board: str = "default"): | ||
| return [] | ||
|
|
||
| def health(self): | ||
| return {"status": "healthy"} | ||
|
|
||
| # Optional P4 methods can raise NotImplementedError | ||
| def add_comment(self, task_id: str, text: str, author: str = None): | ||
| raise NotImplementedError("Comment functionality not implemented") | ||
|
|
||
| def link_tasks(self, parent_id: str, child_id: str): | ||
| raise NotImplementedError("Task linking not implemented") | ||
|
|
||
| def unlink_tasks(self, parent_id: str, child_id: str): | ||
| raise NotImplementedError("Task unlinking not implemented") | ||
|
|
||
| mock_store = MockKanbanStore() | ||
| assert isinstance(mock_store, KanbanStoreProtocol) | ||
|
|
||
| def test_kanban_task_protocol_shape(self): | ||
| """Test KanbanTaskProtocol typed dict structure.""" | ||
| # This tests that the TypedDict can be used for type hints | ||
| # The shape is validated at runtime by mypy/type checkers | ||
| task: KanbanTaskProtocol = { | ||
| "id": "task_123", | ||
| "title": "Test Task", | ||
| "body": "Task description", | ||
| "status": "todo", | ||
| "assignee": "user@example.com", | ||
| "priority": "high", | ||
| "tenant": "test_tenant", | ||
| "board": "default", | ||
| "created_at": 1640995200.0, | ||
| "updated_at": 1640995200.0, | ||
| } | ||
|
|
||
| # Basic validation that we can access the fields | ||
| assert task["id"] == "task_123" | ||
| assert task["title"] == "Test Task" | ||
| assert task["status"] == "todo" | ||
|
|
||
|
|
||
| class TestKanbanHookEvents: | ||
| """Test kanban hook events and payload types.""" | ||
|
|
||
| def test_kanban_hook_events_in_enum(self): | ||
| """Test that all kanban hook events are registered in HookEvent.""" | ||
| expected_events = [ | ||
| HookEvent.KANBAN_TASK_CREATED, | ||
| HookEvent.KANBAN_TASK_CLAIMED, | ||
| HookEvent.KANBAN_TASK_MOVED, | ||
| HookEvent.KANBAN_TASK_DONE, | ||
| HookEvent.KANBAN_TASK_BLOCKED, | ||
| HookEvent.KANBAN_TASK_FAILED, | ||
| ] | ||
|
|
||
| for event in expected_events: | ||
| assert event in HookEvent | ||
|
|
||
| def test_kanban_hook_events_serializable(self): | ||
| """Test that kanban hook events can be serialized.""" | ||
| events = [ | ||
| HookEvent.KANBAN_TASK_CREATED, | ||
| HookEvent.KANBAN_TASK_CLAIMED, | ||
| HookEvent.KANBAN_TASK_MOVED, | ||
| HookEvent.KANBAN_TASK_DONE, | ||
| HookEvent.KANBAN_TASK_BLOCKED, | ||
| HookEvent.KANBAN_TASK_FAILED, | ||
| ] | ||
|
|
||
| for event in events: | ||
| # Should be able to convert to string and back | ||
| assert str(event) == event.value | ||
| assert HookEvent(event.value) == event | ||
|
|
||
| def test_kanban_hook_input_creation(self): | ||
| """Test KanbanHookInput dataclass creation and fields.""" | ||
| hook_input = KanbanHookInput( | ||
| session_id="session_123", | ||
| cwd="/home/user", | ||
| event_name="kanban_task_created", | ||
| timestamp="2023-01-01T00:00:00Z", | ||
| task_id="task_456", | ||
| board="project_board", | ||
| status="todo", | ||
| assignee="user@example.com", | ||
| from_status=None, | ||
| to_status="running" | ||
| ) | ||
|
|
||
| assert hook_input.session_id == "session_123" | ||
| assert hook_input.task_id == "task_456" | ||
| assert hook_input.board == "project_board" | ||
| assert hook_input.status == "todo" | ||
| assert hook_input.assignee == "user@example.com" | ||
| assert hook_input.from_status is None | ||
| assert hook_input.to_status == "running" | ||
|
|
||
| def test_kanban_hook_input_defaults(self): | ||
| """Test KanbanHookInput with default values.""" | ||
| hook_input = KanbanHookInput( | ||
| session_id="session_123", | ||
| cwd="/home/user", | ||
| event_name="kanban_task_created", | ||
| timestamp="2023-01-01T00:00:00Z" | ||
| ) | ||
|
|
||
| # Check defaults | ||
| assert hook_input.task_id == "" | ||
| assert hook_input.board == "default" | ||
| assert hook_input.status == "" | ||
| assert hook_input.assignee is None | ||
| assert hook_input.from_status is None | ||
| assert hook_input.to_status is None | ||
|
|
||
| def test_kanban_hook_input_to_dict(self): | ||
| """Test KanbanHookInput serialization to dict.""" | ||
| hook_input = KanbanHookInput( | ||
| session_id="session_123", | ||
| cwd="/home/user", | ||
| event_name="kanban_task_moved", | ||
| timestamp="2023-01-01T00:00:00Z", | ||
| task_id="task_456", | ||
| board="project_board", | ||
| from_status="todo", | ||
| to_status="running" | ||
| ) | ||
|
|
||
| result = hook_input.to_dict() | ||
|
|
||
| assert result["session_id"] == "session_123" | ||
| assert result["event_name"] == "kanban_task_moved" | ||
| assert result["task_id"] == "task_456" | ||
| assert result["board"] == "project_board" | ||
| assert result["from_status"] == "todo" | ||
| assert result["to_status"] == "running" | ||
|
|
||
|
|
||
| class TestProtocolIntegration: | ||
| """Integration tests for protocol usage patterns.""" | ||
|
|
||
| def test_protocol_import_structure(self): | ||
| """Test that protocols can be imported from expected paths.""" | ||
| # Test direct imports | ||
| from praisonaiagents.kanban.protocols import KanbanStoreProtocol | ||
| from praisonaiagents.hooks.types import KanbanHookInput | ||
|
|
||
| assert KanbanStoreProtocol is not None | ||
| assert KanbanHookInput is not None | ||
|
|
||
| def test_lazy_import_from_package_root(self): | ||
| """Test that kanban protocols can be imported from package root.""" | ||
| from praisonaiagents.kanban import ( | ||
| KanbanStoreProtocol, | ||
| KanbanTaskProtocol, | ||
| VALID_KANBAN_STATUSES, | ||
| ) | ||
|
|
||
| assert KanbanStoreProtocol is not None | ||
| assert KanbanTaskProtocol is not None | ||
| assert VALID_KANBAN_STATUSES is not None No newline at end of file |
There was a problem hiding this comment.
Add required real agentic coverage for this feature.
This file only validates protocol/types behavior. It does not include a test that runs agent.start() with a real prompt and verifies model-generated text output for the kanban feature path.
As per coding guidelines, Real agentic tests are MANDATORY for every feature: Agent must call agent.start() with a real prompt, call the LLM, and produce actual text response—not just smoke tests of object construction.
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 81-81: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 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 `@src/praisonai-agents/tests/unit/test_kanban_protocol.py` around lines 1 -
234, Add a new integration test in this file that actually runs the agent
through the kanban feature: create a real Agent instance, call agent.start(...)
with a prompt that triggers the kanban flow (e.g., "create a kanban task titled
X in board Y"), await the returned text output from agent.start(), and assert
the output contains expected model-generated text (e.g., the task
title/confirmation and kanban-related keywords). Also verify any emitted hook
payloads by checking KanbanHookInput fields or HookEvent values if the agent
exposes hooks (use KanbanHookInput and HookEvent to validate payload shape).
Name the test clearly (e.g., test_kanban_agent_real_run) and keep it in this
test module so it runs with the rest of the kanban tests.
Completed a full PR review and applied one fix in Findings:
Review summary:
No UI changes in this PR. |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Architecture Review Checklist
Phase 2: Fix Valid Issues Analysis & Implementation ✅ Finding 2 (@runtime_checkable + optional methods): VALID - Fixed by splitting into extension protocols:
❌ Finding 3 (Integration test): INVALID FOR CORE SDK - Real agentic tests belong in wrapper layer where agents exist, not protocol-only core SDK Phase 2: Implement Valid Fixes
Phase 3: Final Verdict
✅ ARCHITECTURE REVIEW PASSED - APPROVING PRSummary of Changes Made:
Why Other Findings Were Rejected:
Architecture Compliance: This PR perfectly implements kanban protocol contracts for the core SDK while maintaining architectural integrity. |
Greptile SummaryThis PR adds kanban protocol contracts (
Confidence Score: 4/5Safe to merge after fixing the missing KanbanHookInput export in hooks/init.py. The new kanban protocols and HookEvent entries are additive and backward-compatible. The one concrete defect is that KanbanHookInput is added to hooks/types.py but never registered in hooks/init.py's lazy-loader or all, so any consumer following the established import pattern (from praisonaiagents.hooks import KanbanHookInput) will get an AttributeError. The test suite avoids this gap by importing directly from the submodule. The protocol files themselves are clean. hooks/init.py needs to be updated to expose KanbanHookInput via the lazy-loader and all; the test file would benefit from an import-from-package test to catch this gap in the future. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class HookInput {
+session_id: str
+cwd: str
+event_name: str
+timestamp: str
+agent_name: Optional[str]
+extra: Dict
+to_dict() Dict
}
class KanbanHookInput {
+task_id: str
+board: str
+status: str
+assignee: Optional[str]
+from_status: Optional[str]
+to_status: Optional[str]
+to_dict() Dict
}
class HookEvent {
<<enumeration>>
KANBAN_TASK_CREATED
KANBAN_TASK_CLAIMED
KANBAN_TASK_MOVED
KANBAN_TASK_DONE
KANBAN_TASK_BLOCKED
KANBAN_TASK_FAILED
}
class KanbanStoreProtocol {
<<Protocol>>
+get_board() dict
+get_task(task_id) dict|None
+create_task(data) dict
+update_task(task_id, data) dict|None
+move_task(task_id, status) dict|None
+bulk_update(task_ids, status) dict
+delete_task(task_id) bool
+list_events(since, board) list
+health() dict
}
class KanbanCommentingProtocol {
<<Protocol>>
+add_comment(task_id, text, author) dict|None
}
class KanbanLinkingProtocol {
<<Protocol>>
+link_tasks(parent_id, child_id) bool
+unlink_tasks(parent_id, child_id) bool
}
KanbanHookInput --|> HookInput
KanbanHookInput ..> HookEvent : emits
Reviews (2): Last reviewed commit: "fix: split kanban protocol optional meth..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_kanban_store_protocol_is_runtime_checkable(self): | ||
| """Test that KanbanStoreProtocol is @runtime_checkable.""" | ||
| assert runtime_checkable(KanbanStoreProtocol) |
There was a problem hiding this comment.
assert runtime_checkable(KanbanStoreProtocol) re-applies the runtime_checkable decorator to the already-decorated class and asserts the class is truthy — a condition that is always true for any class. It does not verify that KanbanStoreProtocol is actually runtime-checkable. The correct check is against the internal _is_runtime_protocol flag set by the decorator.
| def test_kanban_store_protocol_is_runtime_checkable(self): | |
| """Test that KanbanStoreProtocol is @runtime_checkable.""" | |
| assert runtime_checkable(KanbanStoreProtocol) | |
| def test_kanban_store_protocol_is_runtime_checkable(self): | |
| """Test that KanbanStoreProtocol is @runtime_checkable.""" | |
| assert getattr(KanbanStoreProtocol, "_is_runtime_protocol", False) |
Fixes CodeRabbit finding about @runtime_checkable protocols requiring all declared methods at runtime. Splits optional methods (add_comment, link_tasks, unlink_tasks) into separate extension protocols: - KanbanCommentingProtocol for commenting functionality - KanbanLinkingProtocol for task relationship functionality This allows stores to implement core KanbanStoreProtocol without breaking isinstance checks, while optionally implementing extension protocols for additional features. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Adds protocol-first kanban contracts to the praisonaiagents core SDK so the wrapper (praisonai) and PraisonAIUI can share a stable interface.
What's Implemented
Architecture Compliance
✅ Protocol-driven core: Protocols only, no heavy implementations
✅ No new dependencies: Uses stdlib and existing imports
✅ Lazy loading: Optional imports via package structure
✅ Naming conventions: Protocol suffix per AGENTS.md
✅ Backward compatible: No breaking changes
Fixes #1719
Generated with Claude Code
Summary by CodeRabbit
New Features