Review summary: good MCP foundation overall, but the tool contract for list_tickets_by_status should reject requests that omit all status selectors. Inline comments contain the actionable details. Nit-only findings would be approvable, but this one should request changes.
Review summary: good MCP foundation overall, but the tool contract for list_tickets_by_status should reject requests that omit all status selectors. Inline comments contain the actionable details. Nit-only findings would be approvable, but this one should request changes.
Should fix: this tool schema accepts a call with none of ticket_status_id, status_slug, or status_name, and only fails later inside resolveTicketStatuses(). That makes the MCP contract looser than the real behavior. Please enforce an at least one of rule here (or equivalent explicit validation) so clients get a schema-level/tool-level error instead of discovering it only at runtime.
Should fix: this tool schema accepts a call with none of `ticket_status_id`, `status_slug`, or `status_name`, and only fails later inside `resolveTicketStatuses()`. That makes the MCP contract looser than the real behavior. Please enforce an `at least one of` rule here (or equivalent explicit validation) so clients get a schema-level/tool-level error instead of discovering it only at runtime.
Nit/Optional: since the missing-selector error is now part of the effective tool contract, I'd add a focused test for calling ListTicketsByStatus without ticket_status_id, status_slug, or status_name and asserting the expected MCP error. That keeps the validation behavior from drifting later.
Nit/Optional: since the missing-selector error is now part of the effective tool contract, I'd add a focused test for calling `ListTicketsByStatus` without `ticket_status_id`, `status_slug`, or `status_name` and asserting the expected MCP error. That keeps the validation behavior from drifting later.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Koda
Review summary: good MCP foundation overall, but the tool contract for list_tickets_by_status should reject requests that omit all status selectors. Inline comments contain the actionable details. Nit-only findings would be approvable, but this one should request changes.
@@ -0,0 +31,4 @@'project_id' => ['nullable', 'integer'],'status_name' => ['nullable', 'string'],'status_slug' => ['nullable', 'string'],'ticket_status_id' => ['nullable', 'integer'],Should fix: this tool schema accepts a call with none of
ticket_status_id,status_slug, orstatus_name, and only fails later insideresolveTicketStatuses(). That makes the MCP contract looser than the real behavior. Please enforce anat least one ofrule here (or equivalent explicit validation) so clients get a schema-level/tool-level error instead of discovering it only at runtime.@@ -0,0 +141,4 @@->etc());}public function test_status_ticket_lists_exclude_invisible_projects(): voidNit/Optional: since the missing-selector error is now part of the effective tool contract, I'd add a focused test for calling
ListTicketsByStatuswithoutticket_status_id,status_slug, orstatus_nameand asserting the expected MCP error. That keeps the validation behavior from drifting later.OpenSource/KanbanGit#50