Repository navigation
Pre-approve kcap ledger servers and annotate every MCP tool - #1112
Conversation
Codex reads a missing MCP annotation as destructive and open-world, so an unannotated tool is prompted for on every call, and its automatic reviewer ends the turn on a refusal rather than asking. Pre-approval covers servers whose writes land only in the user's own Capacitor workspace; flows and artefacts keep prompting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoPre-approve safe ledger MCP servers and annotate all tools
AI Description
Diagram
High-Level Assessment
Files changed (26)
|
Code Review by Qodo
1. Destructive updates appear additive
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ["audience_project"] = new("string", "Target project slug when audience is 'project' — the PEOPLE axis (its members become editors; you must be a member). Distinct from 'project' below"), | ||
| ["project"] = new("string", "Target project slug — the PLACE axis: moves the memory's home context to that project (takes precedence over audience)") | ||
| }, ["id"])), | ||
| }, ["id"]), McpToolAnnotations.Upsert), |
There was a problem hiding this comment.
1. Destructive updates appear additive 🐞 Bug ⛨ Security
rescope_memory, close_artefact_responses, and update_plan_task use the Upsert preset, which advertises DestructiveHint: false, even though they replace existing audience, closure, or task state and rescope_memory can widen access to team, organization, or project members. When a harness or approval client relies on MCP annotations rather than a server-trust setting, it can classify audience changes, response closure, and status or notes overwrites as additive writes and omit the intended sensitive-state review.
Agent Prompt
## Issue description
Three tools that overwrite existing state use the non-destructive `Upsert` annotation. `rescope_memory` can also widen who can see and edit an existing memory by promoting it to team, organization, or project audiences, so clients deriving approval decisions from MCP annotations cannot distinguish these operations from harmless additive updates.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[405-413]
- src/Capacitor.Cli/Commands/McpArtefactsServer.cs[553-561]
- src/Capacitor.Cli/Commands/McpPlansServer.cs[577-588]
- src/Capacitor.Cli/Commands/McpToolAnnotations.cs[11-18]
## Recommended Fix
Assign `McpToolAnnotations.Destructive` to `rescope_memory`, `close_artefact_responses`, and `update_plan_task`. Retain their existing idempotent and closed-world behavior while advertising that they replace sensitive existing state, then extend annotation tests to assert that `destructiveHint` is `true` for each operation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
rescope_memory now advertises destructiveHint: true. close_artefact_responses and update_plan_task stay non-destructive: a reversible flag and a status transition are the operation itself, not an overwrite of data, and marking every state change destructive would leave the hint saying nothing.
| new("kcap-memory", ["mcp", "memory"], NeedsProjectCwd: true, | ||
| "Team memory — search, read, and save durable learnings."), | ||
| "Team memory — search, read, and save durable learnings.", AutoApprove: true), |
There was a problem hiding this comment.
2. Shared memories bypass approval 🔗 Cross-repo conflict ⛨ Security
KcapMcpServers marks the mixed-capability kcap-memory, kcap-workitems, and plans servers as AutoApprove even though they expose shared-scope and destructive writes rather than only private or read-only operations. When Codex or Gemini consumes the generated server-level trust settings, calls can create, rescope, archive, or expose memories; merge work items, detach sessions, and alter cross-repository breakdowns or relations; replace task lists; and upload project-file contents without applying the individual tools’ approval annotations.
Agent Prompt
## Issue description
The generated Codex and Gemini configurations grant server-wide approval to the mixed-capability memory, work-items, and plans servers. Those servers expose shared-scope and destructive operations—including visibility changes, archival, cross-repository work-item mutations, task replacement, and project-file uploads—so server-level trust bypasses the per-tool safety annotations intended to distinguish reads from sensitive writes.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Mcp/KcapMcpServers.cs[29-34]
- src/Capacitor.Cli.Core/Harness/Codex/CodexConfigToml.cs[224-230]
- src/Capacitor.Cli.Core/Mcp/JsonMcpConfigWriter.cs[117-123]
- src/Capacitor.Cli/Commands/McpMemoryServer.cs[383-416]
- src/Capacitor.Cli/Commands/McpWorkItemsServer.cs[172-207]
- src/Capacitor.Cli/Commands/McpWorkItemsServer.cs[410-477]
## Recommended Fix
Remove server-level `AutoApprove: true` from `kcap-memory`, `kcap-workitems`, and other servers containing writable tools, including plans. Preserve the per-tool MCP annotations so annotation-aware clients can approve read-only tools individually and distinguish write types, while clients that support only server-wide trust continue prompting before shared-scope or destructive mutations.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
kcap-memory no longer carries server-level auto-approve and relies on its annotations. kcap-workitems and kcap-plans keep it: their destructive tools touch only the record of the session that calls them, the destination the hooks already post to without a prompt. Codex runs a non-destructive, closed-world tool without approval and sends a destructive one to approval, so on Codex the split follows the annotations either way.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264a1bba47
ℹ️ 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 address that feedback".
| new("kcap-memory", ["mcp", "memory"], NeedsProjectCwd: true, | ||
| "Team memory — search, read, and save durable learnings."), | ||
| "Team memory — search, read, and save durable learnings.", AutoApprove: true), |
There was a problem hiding this comment.
When save_memory is called with audience: "org" or rescope_memory promotes an existing private memory, this flag renders trust: true for Gemini and default_tools_approval_mode = "approve" for Codex, so the disclosure runs without user confirmation. McpMemoryServer.BuildToolsList explicitly permits team, project, and organization-wide audiences, making this unlike a purely private ledger write and creating the same audience-widening risk for which this change leaves kcap-artefacts unapproved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
kcap-memory is no longer pre-approved at registration and relies on its annotations, which mark rescope_memory destructive; save_memory stays additive.
| ["flow_run_id"] | ||
| ) | ||
| ), | ||
| McpToolAnnotations.Read |
There was a problem hiding this comment.
On a successful get_review_flow_status call, HandleToolCallAsync formats pending messages and then calls AckRenderedMessagesAsync, so the call modifies server state and those messages normally cannot be retrieved again; the same applies to get_flow_status. Advertising readOnlyHint: true lets clients treat this as a harmless read, which is especially problematic if the MCP response is lost after the acknowledgement and the client retries expecting the messages to remain available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both status tools now advertise readOnlyHint: false, destructiveHint: false, idempotentHint: false.
| ["plan_id"] = new("string", "Plan to write to. Defaults to the session's current plan."), | ||
| ["session_id"] = new("string", "Session making the declaration. Defaults to the session this server runs in when omitted.") | ||
| }, ["tasks"])), | ||
| }, ["tasks"]), McpToolAnnotations.Destructive), |
There was a problem hiding this comment.
When task objects omit task_id, as the advertised schema permits, the description says IDs are minted for them; repeating the same full-snapshot call therefore replaces the list with newly identified tasks rather than having no additional effect. McpToolAnnotations.Destructive sets idempotentHint: true, so clients may incorrectly consider this call safe to retry and silently churn task identities; this tool needs a destructive, non-idempotent annotation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
set_plan_tasks now advertises destructiveHint: true and idempotentHint: false.
| ["audience_project"] = new("string", "Target project slug when audience is 'project' — the PEOPLE axis (its members become editors; you must be a member). Distinct from 'project' below"), | ||
| ["project"] = new("string", "Target project slug — the PLACE axis: moves the memory's home context to that project (takes precedence over audience)") | ||
| }, ["id"])), | ||
| }, ["id"]), McpToolAnnotations.Upsert), |
There was a problem hiding this comment.
When an existing memory is moved from an organization or team audience to a private or different audience, this operation revokes existing viewers and overwrites the access scope. The Upsert preset advertises destructiveHint: false, which describes the operation as additive and can cause annotation-driven approval clients to under-classify a consequential access-control change; use a destructive annotation for rescope_memory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
rescope_memory now advertises destructiveHint: true.
A flow status call acknowledges the messages it renders, so it is not a read; replacing the task list mints ids for entries without one, so a repeat is not a no-op; rescoping a memory overwrites its access scope; a loose end re-declared lands on the same server stream. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A save or rescope can widen who sees a memory, the same reason artefacts are not pre-approved. Codex's default mode already runs the non-destructive memory tools without a prompt from the annotations, so only archive, update and rescope reach its reviewer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Closes #1111 — AI-3132
What & why
Codex decides whether an MCP call needs approval from the tool's annotations: a tool marked non-destructive and closed-world runs without one, a destructive tool goes to approval, and with
approvals_reviewer = "auto_review"that approval is a model that can end the turn. No kcap tool advertised annotations, so every call went to approval anddeclare_plan_documentwas refused as an upload with no named destination. Every tool now carries annotations for what its handler does, the plans description names where the content goes, and registration pre-approveskcap-workitemsandkcap-plansbeside the read-only servers, since even their destructive tools touch only the session's own record.kcap-memorystays on annotations: a save or rescope can widen who sees a memory.Where to look
McpToolAnnotationsfixes six presets; each tool's pick is the claim to check, chosen by what the handler does rather than by the name: the two flow status calls acknowledge the messages they render, so they are not reads, andset_plan_tasksmints ids for entries without one, so it is destructive and not idempotent. A Codex entry kcap wrote is healed to the new shape on the nextkcap setup; one Codex has since added per-tool approvals under is left alone.Verification
Codex 0.155.1 with the auto-reviewer on refused
declare_plan_document: "would upload local document contents without explicit destination approval". After the change,--treenode-filter "/*/*/Mcp*/*"on the CLI unit suite: 445 passed, 0 failed; the integration MCP classes: 59 passed; the Core registration and descriptor classes: 132 passed.dotnet publish -c Release: no IL2026/IL3050 warnings. A live Codex run against the healed entry is still owed.🤖 Generated with Claude Code