fix(mcp): give Review write tools ToolAnnotations so hosts can prompt - #1392
Merged
Teingi merged 2 commits intoAug 30, 2026
Merged
Conversation
`_annotate_mcp_component` promises to "describe the side effects that an MCP host should use for approval decisions", but its branches only cover the read-only set, `handoff_current_work`, and `commit_handoff`. `approve_artifact_candidate`, `reject_artifact_candidate`, and `revise_artifact_candidate` fall through and are projected with `annotations: None`, so a host has nothing to key a confirmation prompt off for the three Review operations that change Candidate state. Project them with `destructiveHint=True` and add a regression test mirroring the existing handoff annotation test. The projected tool set is unchanged; annotations are advisory hints, not an authorization boundary (RFC 0050). Relates to oceanbase#1391.
|
|
Teingi
reviewed
Aug 30, 2026
MCP defines idempotentHint by additional environment effect, not by the retry returning the same successful response. An exact replay of approve/reject/revise is rejected by the pending-head CAS (ArtifactCandidateRepository.lock_pending) before any Candidate version, Artifact revision, index update, or status transition is written, so repeated identical calls have no additional effect. Requested in review.
Contributor
Author
|
Note on the force-push: it only corrects the commit author identity (a stray local git config pointed at an email tied to a different GitHub account, which kept the CLA check pending). No content changes — |
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
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.
Which issue or RFC does this PR close?
Relates to #1391. It deliberately does not close it.
#1391 raises a question only you can answer: whether the Review write operations should stay on the
agent-facing MCP surface, or whether the documentation should be corrected instead. This PR takes no
position on that. It lands only the piece that is correct under either answer, so the larger decision
can take as long as it needs.
Consistent with
docs/en/rfcs/0050_artifact_candidate_review_inbox.md:235-238— "PowerContext doesnot treat MCP visibility as an authorization boundary" — this PR does not turn annotations into one.
They are advisory hints a host may act on.
Rationale for this change
_annotate_mcp_componentinsrc/powercontext/server/mcp.pycarries this docstring:Its branches cover the read-only set,
handoff_current_work, andcommit_handoff.approve_artifact_candidate,reject_artifact_candidate, andrevise_artifact_candidatefallthrough all of them, so they are projected with no annotations at all. Against a stock
powercontext server runon this branch's parent commit:So the three tools that change Candidate state are the three that give a host nothing to key a
confirmation prompt off. A host that gates on
destructiveHinttreats an unannotated tool asordinary. Since the projection already annotates the Handoff writes, this reads as an omission
rather than a decision, and this PR just finishes what the function sets out to do.
What changes are included in this PR?
src/powercontext/server/mcp.py— add_MCP_REVIEW_WRITE_OPERATION_IDSand one branch in_annotate_mcp_component, projecting the three Review write operations withreadOnlyHint=False,destructiveHint=True,idempotentHint=True,openWorldHint=False(
idempotentHintwasFalseas first submitted; changed indd6f059per review).tests/test_mcp.py— addtest_mcp_describes_review_write_side_effects_for_host_approval,mirroring the existing
test_mcp_describes_handoff_tool_side_effects_for_host_approval, so afuture change that drops the annotations fails here rather than silently in a host's
confirmation logic.
+39 lines, 2 files. Deliberate non-goals, so this stays reviewable in isolation: no operation added
to or removed from
_MCP_OPERATION_IDS, no schema change, no new configuration, no docs or RFCedits, no HTTP/Client/CLI/
/reviewschange, and no change toopenapi/powercontext.yaml.Two judgment calls worth your attention, since I would rather you disagree with them now than
find them yourself:
idempotentHint— resolved in review, nowTrue(dd6f059). As first submitted this wasFalse, on the reading that a repeated identical call errors rather than no-opping. The reviewcorrectly pointed out that MCP defines the hint by additional environment effect, not by the retry
returning the same successful response:
expected_versionis required (ge=1, no default) on eachrequest model, and an exact replay is rejected by the pending-head CAS
(
ArtifactCandidateRepository.lock_pending) before any Candidate version, Artifact revision, indexupdate, or status transition is written — so the tools are idempotent in the MCP sense.
destructiveHint=Trueonrevise_artifact_candidateis arguable and I want to flag it ratherthan bury it. Approve and reject are clearly destructive: both are terminal and irreversible, and
approve additionally makes content reachable through retrieval. Revise is not terminal — per
docs/en/docs/how-to/review-candidates.md:101the candidate "remainspendingand has a higherversion" — and at the storage layer it is additive, which is the same shape ascommit_handoff,annotated
destructiveHint=Falsethirty lines above. I grouped it with the terminal pair becausethe request body is a complete replacement proposal, so it swaps out the proposal a reviewer was
about to act on and invalidates any
expected_versionthey were holding; and becausedestructiveHintdefaults totruefor a non-read-only tool, so the choice is between an explicittrueand an explicit claim of "additive only". If you would rather it bedestructiveHint=Falsefor storage-level consistency with
commit_handoff, say the word and I will split the branch.Also for completeness: nine projected write tools still carry no annotations
(
capture_content_source,create_work_contract,acknowledge_handoff,record_task_outcome,activate_handoff,finalize_handoff,remember_memory,revise_memory_entry,retire_memory_entry). I left them alone to keep this diff small and single-purpose. If you wantthem covered, the durable fix is an invariant test asserting that every non-read-only projected tool
carries annotations — I did not add it here because it would fail today on those nine and force
exactly the scope expansion this PR is trying to avoid. Happy to do it as a follow-up if you want it.
Are there any user-facing changes?
Yes, additive only.
tools/listnow reports annotations on the three Review write tools instead ofnull, so MCP hosts that confirm before destructive tool calls will prompt on them. Hosts thatignore annotations are unaffected.
Not a breaking change: the same tools are still projected, with the same input schemas and the same
server-side behavior. No change to the HTTP API, the Python Client, the CLI, the
/reviewspage,openapi/powercontext.yaml, or any persisted format. No migration.How was this change tested?
Ubuntu 24.04.4 LTS (WSL2, kernel 6.6.87.2), Python 3.12.3, uv 0.12.7,
uv syncagainst thecommitted
uv.lock. All output below is verbatim: captured at the first submission, andre-verified at the review follow-up where marked. (The branch was force-pushed once solely to
correct the commit author identity for the CLA check; the trees are unchanged —
git diffbetweenthe originally recorded commits
5b26f98/6266c16and the current80a75ff/dd6f059is empty.)The new test fails without the fix. Reverting only
src/powercontext/server/mcp.pyto theparent commit and keeping the test:
make check— exit 0:make unit-test— exit 0:make test(full suite, includingtests/e2e) — exit 0:tests/test_mcp.py— 12 passed, including the new case:Manual validation against a running Server, not just the in-memory client. Started a stock
powercontext server runfrom this branch and connected afastmcp.Clienttohttp://127.0.0.1:8000/mcp/over Streamable HTTP — the same channel an agent host uses(re-captured at the review follow-up, now
dd6f059):Tool count is unchanged at 24, confirming the projected surface is untouched.
Follow-up commit
dd6f059(idempotentHint=True, requested in review) was verified the sameway, red first: with only the test updated, the assertion fails against the prior
mcp.py(
assert False is Trueattests/test_mcp.py:366); after the one-flag change,tests/test_mcp.pypasses 12/12,make checkexits 0, andmake testreports950 passed, 10 skipped— identical totals to the first submission.make contract-testwas not run: the MCP surface is projected at runtime fromapp.openapi()viaOpenAPIProvider, so no generated artifact changes.make docs-testwas not run: no docs changed.AI usage statement
This change was prepared with assistance from an AI coding assistant (Claude, via Claude Code),
working under my direction. The assistant wrote the patch and the test and ran the commands. I set
the scope — specifically the constraint that this PR stay non-breaking and take no position on the
open question in #1391.
Every command and output quoted above was executed on the environment described and pasted verbatim;
none of it is a model-generated summary or reconstruction. The same applies to the file and line
references. Please review it as AI-authored code — it is 37 lines and the whole diff fits on a
screen.