Expose the declared work-breakdown and relation MCP tools [AI-1718] - #441
Merged
Conversation
The server shipped five routes for declared work breakdown and relations, and no
agent could reach any of them: `kcap mcp workitems` exposed only declare_work_item
and get_session_work_items, and the server's design deliberately provides no UI
path. The feature was inert.
Adds five tools over the existing routes:
declare_work_breakdown POST /api/work-items/{parentId}/breakdown
retract_work_breakdown POST /api/work-items/{parentId}/breakdown/retract
declare_work_relation POST /api/work-items/{fromId}/relations
retract_work_relation POST /api/work-items/{fromId}/relations/retract
get_work_item_topology GET /api/work-items/{id}/topology
Design decisions worth naming:
- No tool accepts `source` or `declared_by`. The server resolves both from the
authenticated caller and rejects a source of "user" outright, so exposing either
would be an argument the server ignores at best and a spoofing surface at worst.
Asserted by a test over every tool rather than left to reviewer vigilance.
- Ids are REQUIRED with no fallback. ResolveSessionId can default to the ambient
session because "the session I am running in" is unambiguous; there is no ambient
work item, so a default would silently attach the wrong edge of the graph.
- Ids are URL-escaped, so one containing a slash or percent cannot walk out of its
path segment into a different route.
- Semantic validation is NOT duplicated client-side. The rules the server owns —
cross-repo edges, unknown/deleted ids, a parent among its own parts, self-
relations, an empty parts list, the relation_kind vocabulary — surface as coded
4xx bodies through the existing error mapping. What IS validated locally is
SHAPE: a present-but-wrong-typed part_ids throws rather than being dropped,
because a silently omitted selector turns a malformed request into a
differently-shaped one whose rejection reads as if nothing had been sent. Same
reasoning as the existing TryReadInt guard.
- relation_kind is passed through unvalidated on purpose: the server owns the
vocabulary, so a client-side enum would drift from it as kinds are added, and the
server's coded rejection names the real reason.
McpSchemaProperty gains an optional Items: these are the first `array`-typed
properties in any of these MCP servers, and an array with no `items` is incomplete
JSON Schema — a strict client can reject it and a model has to guess the element
type. Optional and trailing, so every existing call is unchanged, and omitted from
the wire when null.
Registration needed no change: kcap-workitems is already Claude-Code-plugin-only.
Mutation-verified — dropping the URL escape, exposing a `source` argument, and
dropping the items schema each fail exactly one test and nothing else.
McpWorkItemsServerTests 25/25; 254 tests green across 13 MCP-adjacent classes,
confirming the shared-record change disturbs no other server's schema.
Not done: teaching skill text when to declare a breakdown versus a relation. There
is no work-items skill today — declare_work_item has never had one either — so this
would mean introducing a new auto-registered skill that costs context in every
session. That is a product call, not part of wiring the tools up, so the when/why
lives in the tool descriptions for now.
…istent
Codex review round 1 — one Medium, four Low.
Medium: escaping alone did not contain an id to its path segment. `.` is
unreserved in RFC 3986, so EscapeDataString leaves it untouched and URI
normalization then REMOVES the segment: an id of "." reached
/api/work-items/breakdown and ".." reached /api/breakdown — a different route
whose response would be attributed to the id the caller passed. My own comment
claimed no id could do this. All-dot ids are now rejected outright; a dot inside an
otherwise-real id is still fine, which the test pins.
Low 1: the shape/semantic split was applied inconsistently. `{"part_ids": null}`
is a PRESENT wrong shape, but the `is { } node` form read it as absence and
silently omitted the field — the exact failure mode the rule exists to prevent.
Presence is now detected with TryGetPropertyValue and an explicit null throws.
Low 2: to_id/relation_kind dropped an explicitly-supplied empty string, so the
caller got the server's "required" error when they HAD supplied the value. Every
supplied string is now forwarded verbatim, including ""; absence stays absence and
a present non-string still fails locally.
Low 3: the "no tool accepts source/declared_by" test proved non-ADVERTISEMENT, not
non-acceptance — without additionalProperties:false a schema does not forbid a
caller sending them. Renamed to match what it checks, given a non-vacuity guard on
the tool count, and paired with a new test asserting the real guarantee: the body
builders never FORWARD either key even when supplied.
Low 4: nothing exercised the dispatch switch, so a wrong method, wrong suffix, or
mismatched body builder would have gone unnoticed. Six tests now drive
HandleToolCallAsync through a capturing HttpMessageHandler and assert the method,
URL and body per tool, plus that a missing id fails before any request is built.
Mutation-verified, each killing exactly one test: dot guard removed; empty-string
drop restored; null-as-absence restored; and the relations route paired with the
breakdown body — which only the new dispatch test catches.
McpWorkItemsServerTests 36/36; 222 green across 9 MCP-adjacent classes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…st instead of catching Codex review round 2, three Low. 1. My dot guard over-rejected. Only "." and ".." are RFC dot segments; "..." and longer runs are ordinary path segments, so refusing them would reject an id the server might accept. Narrowed to exactly those two — and my own test had pinned the over-broad behaviour, so it is now written to fail in BOTH directions: mutation-verified against a guard that is too broad AND one that is absent. Percent-encoded dots confirmed inert: EscapeDataString escapes the '%' itself, and standard URI processing does not decode twice. 2. A bare catch around GetValue<string> would report an unrelated failure as a type error. Replaced with a JsonValue/TryGetValue shape test — applied at all three extraction sites, not only the one flagged. Renamed CopyRequiredishString to CopySuppliedString, which is what it actually does: forward any supplied string including "", reject a present null or non-string, leave an absent key absent. 3. The non-vacuity guard froze the tool count at 7, so adding an unrelated tool would have failed it. Loosened to greater-than-zero; the exact set is already pinned by Tools_list_exposes_the_declare_and_breakdown_surface, and duplicating that pin here bought nothing. McpWorkItemsServerTests 36/36. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoExpose declared work-breakdown and relation MCP tools
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. README workitems tools outdated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+455
to
+458
| new("declare_work_breakdown", | ||
| "Declare that a work item is broken down into parts (sub-items). Idempotent: re-declaring an " | ||
| + "existing part is accepted and reported as existing rather than created. A part can have at " | ||
| + "most one parent, and all items must live in the same repository.", |
There was a problem hiding this comment.
1. Readme workitems tools outdated 📘 Rule violation ⚙ Maintainability
This PR adds five new kcap mcp workitems tools, but README.md still documents the workitems MCP server as providing only two tools. This violates the requirement to update README.md in the same PR for user-facing CLI surface changes.
Agent Prompt
## Issue description
The PR expands the `kcap mcp workitems` tool surface (breakdowns, relations, topology), but `README.md` still states the server provides only `declare_work_item` and `get_session_work_items`.
## Issue Context
Compliance requires updating `README.md` in the same PR for any user-facing CLI surface change, including relevant per-command documentation (and the quick-start/overview if it describes the affected surface).
## Fix Focus Areas
- README.md[179-196]
- README.md[534-548]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
The kcap-server side shipped five routes for declared work breakdown and relations, and no agent could reach any of them.
kcap mcp workitemsexposed onlydeclare_work_itemandget_session_work_items, and the server's design deliberately provides no UI path — so the feature was inert on arrival. This wires it up.declare_work_breakdownPOST /api/work-items/{parentId}/breakdownretract_work_breakdownPOST /api/work-items/{parentId}/breakdown/retractdeclare_work_relationPOST /api/work-items/{fromId}/relationsretract_work_relationPOST /api/work-items/{fromId}/relations/retractget_work_item_topologyGET /api/work-items/{id}/topologyRegistration needed no change —
kcap-workitemsis already Claude-Code-plugin-only.Design decisions
No tool accepts
sourceordeclared_by. The server resolves both from the authenticated caller and rejects asourceof"user"outright. Exposing either would be an argument the server ignores at best and a spoofing surface at worst. Two tests: one that no tool advertises them, and — the real guarantee — that the body builders never forward them even when a caller ignores the schema and sends them anyway. (WithoutadditionalProperties: false, a schema cannot forbid the keys, only decline to invite them.)Ids are required, with no fallback.
ResolveSessionIdcan default to the ambient session because "the session I am running in" is unambiguous. There is no ambient work item, so a default would silently attach the wrong edge of the graph.Ids are contained in their path segment — and escaping alone is not enough.
Uri.EscapeDataStringhandles/,%,?,#. But.is unreserved in RFC 3986, so it survives escaping and URI normalization then removes the segment: an id of"."would reach/api/work-items/breakdownand".."would reach/api/breakdown— a different route whose response would be attributed to the id passed. Exactly those two are rejected;"..."and longer runs are ordinary path segments and are allowed, since refusing them would reject an id the server might accept. Caller-supplied"%2e"is inert:EscapeDataStringescapes the%itself and standard URI processing does not decode twice.Shape is validated locally; semantics are not. The rules the server owns — cross-repo edges, unknown/deleted ids, a parent among its own parts, self-relations, an empty parts list, the
relation_kindvocabulary — surface as coded 4xx bodies through the existing error mapping. What is checked locally is shape: a present-but-wrong-typedpart_idsthrows rather than being dropped, because a silently omitted selector turns a malformed request into a differently-shaped one whose rejection reads as though nothing had been sent. Same reasoning as the existingTryReadIntguard. An explicitly supplied""is forwarded, so the caller gets the server's invalid-value error rather than a misleading "required".No client-side interpretation of server error bodies. The 404 is deliberately non-enumerating — identical whether the item doesn't exist, isn't visible, or is in another repo — so elaborating it client-side would undermine that design. The 400 detail is already in the body the existing mapping surfaces.
McpSchemaPropertygains an optionalItems. These are the firstarray-typed properties in any of these MCP servers, and an array with noitemsis incomplete JSON Schema — a strict client can reject it and a model has to guess the element type. Optional and trailing, so every existingnew("string", …)call is unchanged, and omitted from the wire when null.Verification
Mutation-verified across all three rounds; each mutant fails exactly one test and nothing else:
Item_url_builds_the_route_and_escapes_the_idsourceargument exposedNo_tool_advertises_a_server_owned_source_or_declared_by_argumentitemsschema droppedArray_properties_declare_their_element_typeItem_url_rejects_dot_segment_ids…Relation_body_forwards_an_explicitly_empty_string…Breakdown_body_rejects_an_explicit_null_part_idsDispatch_declare_work_relation_posts_the_relation_body_not_the_breakdown_bodyThat last one matters: the helper tests verify URL and body construction independently, so none of them would catch a switch entry using the wrong method, suffix, or body builder. Six dispatch tests now drive
HandleToolCallAsyncthrough a capturingHttpMessageHandlerand assert the method, URL and body per tool, plus that a missing id fails before any request is built.McpWorkItemsServerTests36/36; 254 green across 13 MCP-adjacent classes, confirming the shared-record change disturbs no other server's schema.Review rounds
Round 1 — one Medium, four Low. The dot-segment hole (my own comment had claimed no id could escape its segment);
{"part_ids": null}read as absence, the exact failure the shape rule exists to prevent; explicitly-empty relation strings dropped; the source/declared_by test proving non-advertisement while claiming non-acceptance; and no dispatch-level coverage at all.Round 2 — three Low. My dot guard over-rejected, and my own test had pinned that over-broad behaviour; a bare
catchcould report an unrelated failure as a type error (fixed at all three extraction sites, not just the flagged one); and the non-vacuity guard froze the tool count so an unrelated new tool would have failed it.Round 3 — clean, with
JsonValue.TryGetValue<string>semantics verified against the .NET runtime source rather than assumed.Deliberately not done
Skill text teaching when to declare a breakdown versus a relation. There is no work-items skill today —
declare_work_itemhas never had one either — so this would mean introducing a new auto-registered skill that costs context in every session. That is a product call, not part of wiring the tools up, so the when/why lives in the tool descriptions for now.🤖 Generated with Claude Code