Skip to content

fix(mcp): give Review write tools ToolAnnotations so hosts can prompt - #1392

Merged
Teingi merged 2 commits into
oceanbase:masterfrom
russell001209-ai:fix/review-tool-annotations
Aug 30, 2026
Merged

fix(mcp): give Review write tools ToolAnnotations so hosts can prompt#1392
Teingi merged 2 commits into
oceanbase:masterfrom
russell001209-ai:fix/review-tool-annotations

Conversation

@russell001209-ai

@russell001209-ai russell001209-ai commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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 does
not 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_component in src/powercontext/server/mcp.py carries this docstring:

Describe the side effects that an MCP host should use for approval decisions.

Its branches cover the read-only set, handoff_current_work, and commit_handoff.
approve_artifact_candidate, reject_artifact_candidate, and revise_artifact_candidate fall
through all of them, so they are projected with no annotations at all. Against a stock
powercontext server run on this branch's parent commit:

$ uv run python show_annotations.py     # fastmcp.Client -> http://127.0.0.1:8000/mcp/
tools/list returned 24 tools
  approve_artifact_candidate
    annotations: None
  reject_artifact_candidate
    annotations: None
  revise_artifact_candidate
    annotations: None

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 destructiveHint treats an unannotated tool as
ordinary. 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?

  1. src/powercontext/server/mcp.py — add _MCP_REVIEW_WRITE_OPERATION_IDS and one branch in
    _annotate_mcp_component, projecting the three Review write operations with
    readOnlyHint=False, destructiveHint=True, idempotentHint=True, openWorldHint=False
    (idempotentHint was False as first submitted; changed in dd6f059 per review).
  2. tests/test_mcp.py — add test_mcp_describes_review_write_side_effects_for_host_approval,
    mirroring the existing test_mcp_describes_handoff_tool_side_effects_for_host_approval, so a
    future 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 RFC
edits, no HTTP/Client/CLI//reviews change, and no change to openapi/powercontext.yaml.

Two judgment calls worth your attention, since I would rather you disagree with them now than
find them yourself:

idempotentHintresolved in review, now True (dd6f059). As first submitted this was
False, on the reading that a repeated identical call errors rather than no-opping. The review
correctly pointed out that MCP defines the hint by additional environment effect, not by the retry
returning the same successful response: expected_version is required (ge=1, no default) on each
request model, and an exact replay is rejected by the pending-head CAS
(ArtifactCandidateRepository.lock_pending) before any Candidate version, Artifact revision, index
update, or status transition is written — so the tools are idempotent in the MCP sense.

destructiveHint=True on revise_artifact_candidate is arguable and I want to flag it rather
than 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:101 the candidate "remains pending and has a higher
version" — and at the storage layer it is additive, which is the same shape as commit_handoff,
annotated destructiveHint=False thirty lines above. I grouped it with the terminal pair because
the request body is a complete replacement proposal, so it swaps out the proposal a reviewer was
about to act on and invalidates any expected_version they were holding; and because
destructiveHint defaults to true for a non-read-only tool, so the choice is between an explicit
true and an explicit claim of "additive only". If you would rather it be destructiveHint=False
for 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 want
them 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/list now reports annotations on the three Review write tools instead of
null, so MCP hosts that confirm before destructive tool calls will prompt on them. Hosts that
ignore 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 /reviews page,
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 sync against the
committed uv.lock. All output below is verbatim: captured at the first submission, and
re-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 diff between
the originally recorded commits 5b26f98/6266c16 and the current 80a75ff/dd6f059 is empty.)

The new test fails without the fix. Reverting only src/powercontext/server/mcp.py to the
parent commit and keeping the test:

$ git checkout HEAD^ -- src/powercontext/server/mcp.py
$ uv run python -m pytest tests/test_mcp.py::test_mcp_describes_review_write_side_effects_for_host_approval -q
>           assert decision is not None, f"{name} carries no annotations for an MCP host to prompt on"
E           AssertionError: approve_artifact_candidate carries no annotations for an MCP host to prompt on
E           assert None is not None

tests/test_mcp.py:363: AssertionError
=========================== short test summary info ============================
FAILED tests/test_mcp.py::test_mcp_describes_review_write_side_effects_for_host_approval
1 failed in 2.20s

make check — exit 0:

$ make check
🚀 Checking lock file consistency with 'pyproject.toml'
Resolved 190 packages in 1ms
🚀 Linting code: Running prek
check for case conflicts.................................................Passed
check for merge conflicts................................................Passed
check toml...............................................................Passed
check yaml...............................................................Passed
check json...............................................................Passed
pretty format json.......................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff check...............................................................Passed
ruff format..............................................................Passed
ty check.................................................................Passed
🚀 Static type checking: Running ty
All checks passed!
🚀 Static type checking: Running ty for the Pydantic AI integration
All checks passed!

make unit-test — exit 0:

$ make unit-test
================= 893 passed, 1 skipped, 6 warnings in 42.30s ==================

make test (full suite, including tests/e2e) — exit 0:

$ make test
============ 950 passed, 10 skipped, 6 warnings in 60.05s (0:01:00) ============

tests/test_mcp.py — 12 passed, including the new case:

$ uv run python -m pytest tests/test_mcp.py -v
collecting ... collected 12 items
...
tests/test_mcp.py::test_mcp_describes_handoff_tool_side_effects_for_host_approval PASSED [ 66%]
tests/test_mcp.py::test_mcp_describes_review_write_side_effects_for_host_approval PASSED [ 75%]
tests/test_mcp.py::test_mcp_exact_entry_tools_use_nested_citations PASSED [ 83%]
tests/test_mcp.py::test_mcp_bridge_reuses_logical_request_id_and_is_marked_internal PASSED [ 91%]
tests/test_mcp.py::test_mcp_access_log_counts_the_logical_tool_call_without_the_bridge PASSED [100%]

============================== 12 passed in 2.70s ==============================

Manual validation against a running Server, not just the in-memory client. Started a stock
powercontext server run from this branch and connected a fastmcp.Client to
http://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):

tools/list returned 24 tools
  approve_artifact_candidate
    annotations: {"readOnlyHint": false, "destructiveHint": true, "idempotentHint": true, "openWorldHint": false}
  reject_artifact_candidate
    annotations: {"readOnlyHint": false, "destructiveHint": true, "idempotentHint": true, "openWorldHint": false}
  revise_artifact_candidate
    annotations: {"readOnlyHint": false, "destructiveHint": true, "idempotentHint": true, "openWorldHint": false}

Tool count is unchanged at 24, confirming the projected surface is untouched.

Follow-up commit dd6f059 (idempotentHint=True, requested in review) was verified the same
way, red first: with only the test updated, the assertion fails against the prior mcp.py
(assert False is True at tests/test_mcp.py:366); after the one-flag change,
tests/test_mcp.py passes 12/12, make check exits 0, and make test reports
950 passed, 10 skipped — identical totals to the first submission.

make contract-test was not run: the MCP surface is projected at runtime from app.openapi() via
OpenAPIProvider, so no generated artifact changes. make docs-test was 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.

`_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.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment thread src/powercontext/server/mcp.py Outdated
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.

@Teingi Teingi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Teingi
Teingi merged commit 6347e7e into oceanbase:master Aug 30, 2026
12 of 13 checks passed
@russell001209-ai

Copy link
Copy Markdown
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 — git diff between the previous tips (5b26f98, 6266c16) and the current ones (80a75ff, dd6f059) is empty. Sorry for the noise, and thanks for approving the CI run — the green results from 6266c16 are for the identical tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants