feat(mcp): mcp-server-properly-built audit — 11 atomic commits closing 13 audit findings - #512
Merged
Merged
Conversation
Reviewer's GuideRefactors the MCP server to be a fully-typed, documented FastMCP server with pagination and response-format options, adds a CLI Sequence diagram for aelf mcp CLI start and FastMCP server behaviorsequenceDiagram
actor User
participant CLI as aelf_CLI
participant MCP as _cmd_mcp
participant Module as aelfrice.mcp_server
participant Serve as serve
participant FastMCP as FastMCP_runtime
participant Host as MCP_host
User->>CLI: run `aelf mcp`
CLI->>MCP: dispatch to _cmd_mcp(args, out)
MCP->>Module: import serve
alt import fails (aelfrice not importable)
Module-->>MCP: ImportError
MCP->>User: print error to stderr
MCP-->>CLI: return status 1
else import ok
MCP->>Serve: call serve()
alt fastmcp missing
Serve-->>MCP: raise RuntimeError("fastmcp is not installed ...")
MCP->>User: print error with install hint to stderr
MCP-->>CLI: return status 1
else fastmcp available
Serve->>FastMCP: construct server(name="aelfrice", instructions=_SERVER_INSTRUCTIONS)
Serve->>FastMCP: register tools (aelf_search, aelf_lock, ...)
FastMCP-->>Serve: ready
FastMCP-->>Host: JSON-RPC over stdio
loop for each tool call
Host->>FastMCP: call tool (e.g. aelf_locked)
FastMCP->>Module: invoke wrapper
Module->>Module: open MemoryStore
Module->>Module: run pure handler
Module-->>FastMCP: dict response (json or markdown-wrapped)
FastMCP-->>Host: tool response
end
alt user sends SIGINT
Host-->>FastMCP: SIGINT / pipe close
FastMCP-->>Serve: stop
Serve-->>MCP: return
MCP-->>CLI: return status 0
end
end
end
Class diagram for MCP server tool wrappers, shared types, and response formattingclassDiagram
class MemoryStore {
}
class AelfToolHandlers {
+tool_search(store: MemoryStore, query: str, budget: int, response_format: str) dict
+tool_lock(store: MemoryStore, statement: str) dict
+tool_locked(store: MemoryStore, pressured: bool, limit: int, offset: int, response_format: str) dict
+tool_demote(store: MemoryStore, belief_id: str) dict
+tool_validate(store: MemoryStore, belief_id: str, source: str) dict
+tool_unlock(store: MemoryStore, belief_id: str) dict
+tool_promote(store: MemoryStore, belief_id: str, source: str) dict
+tool_feedback(store: MemoryStore, belief_id: str, signal: str, source: str) dict
+tool_confirm(store: MemoryStore, belief_id: str, source: str, note: str) dict
+tool_stats(store: MemoryStore, response_format: str) dict
+tool_health(store: MemoryStore, response_format: str) dict
}
class ResponseFormattingHelpers {
<<utility>>
+_wrap_markdown(json_payload: dict~str, Any~, text: str) dict~str, Any~
+_render_search_markdown(payload: dict~str, Any~) str
+_render_locked_markdown(payload: dict~str, Any~) str
+_render_stats_markdown(payload: dict~str, Any~) str
+_render_health_markdown(payload: dict~str, Any~) str
+_RESPONSE_FORMAT_JSON: str
+_RESPONSE_FORMAT_MARKDOWN: str
+_RESPONSE_FORMATS: frozenset~str~
}
class LockedPagingConfig {
<<value object>>
+_LOCKED_DEFAULT_LIMIT: int
+_LOCKED_MAX_LIMIT: int
}
class PydanticTypes {
<<type aliases>>
+_BeliefId: Annotated~str, Field~
+_SourceLabel: Annotated~str, Field~
+_ResponseFormat: Annotated~str, Field~
}
class FastMCPServer {
<<MCP server wrapper>>
+serve() void
+_open_default_store() MemoryStore
+_SERVER_INSTRUCTIONS: str
}
class MCPToolsAPI {
<<FastMCP tool wrappers>>
+aelf_onboard(path: str|None, session_id: str|None, classifications: list~dict~str, Any~~|None) dict
+aelf_search(query: str, budget: int, response_format: str) dict
+aelf_lock(statement: str) dict
+aelf_locked(pressured: bool, limit: int, offset: int, response_format: str) dict
+aelf_demote(belief_id: str) dict
+aelf_validate(belief_id: str, source: str) dict
+aelf_unlock(belief_id: str) dict
+aelf_promote(belief_id: str, source: str) dict
+aelf_feedback(belief_id: str, signal: str, source: str) dict
+aelf_confirm(belief_id: str, source: str, note: str) dict
+aelf_stats(response_format: str) dict
+aelf_health(response_format: str) dict
}
FastMCPServer --> MCPToolsAPI : registers tools via decorators
FastMCPServer --> AelfToolHandlers : calls pure handlers
AelfToolHandlers --> MemoryStore : uses
AelfToolHandlers --> ResponseFormattingHelpers : uses for markdown
MCPToolsAPI --> FastMCPServer : uses _open_default_store
MCPToolsAPI --> AelfToolHandlers : delegates to pure handlers
MCPToolsAPI --> PydanticTypes : parameter types and validation
AelfToolHandlers --> LockedPagingConfig : uses for limit/offset bounds
ResponseFormattingHelpers --> LockedPagingConfig : uses pagination metadata
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR integrates FastMCP server support into aelfrice, adding a new ChangesMCP Integration and Tool Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
robotrocketscience
added
the
attn:review
This was referenced May 9, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/aelfrice/mcp_server.py" line_range="157-163" />
<code_context>
+ return "\n".join(lines).rstrip() + "\n"
+
+
+def _render_locked_markdown(payload: dict[str, Any]) -> str:
+ locked = payload.get("locked", [])
+ total = payload.get("total", payload.get("n", 0))
+ n = payload.get("n", 0)
+ offset = payload.get("offset", 0)
+ has_more = payload.get("has_more", False)
+ header = (
+ f"# Locked beliefs — page {offset // max(n, 1) + 1 if n else 1}, "
+ f"{n} of {total} shown"
</code_context>
<issue_to_address>
**issue:** Locked markdown page number is computed from the current page size, which breaks when the last page is partial.
Here `n` is the size of the current page, not the page size. On the last page, where `n < limit`, this makes the page index jump (e.g. total=120, limit=50 ⇒ pages: 50, 50, 20; last page uses offset=100, n=20 ⇒ `100 // 20 + 1 == 6`, not 3).
To keep page numbers stable, base the calculation on the actual page size (e.g. `limit` from the server) or on `total` and `offset` alone. For example, pass `page_size` in the payload and use that instead of `n`, or only use `offset // max(n or total, 1) + 1` when `n == limit` and fall back to something like `math.ceil(total / limit)` on the last page.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+157
to
+163
| def _render_locked_markdown(payload: dict[str, Any]) -> str: | ||
| locked = payload.get("locked", []) | ||
| total = payload.get("total", payload.get("n", 0)) | ||
| n = payload.get("n", 0) | ||
| offset = payload.get("offset", 0) | ||
| has_more = payload.get("has_more", False) | ||
| header = ( |
There was a problem hiding this comment.
issue: Locked markdown page number is computed from the current page size, which breaks when the last page is partial.
Here n is the size of the current page, not the page size. On the last page, where n < limit, this makes the page index jump (e.g. total=120, limit=50 ⇒ pages: 50, 50, 20; last page uses offset=100, n=20 ⇒ 100 // 20 + 1 == 6, not 3).
To keep page numbers stable, base the calculation on the actual page size (e.g. limit from the server) or on total and offset alone. For example, pass page_size in the payload and use that instead of n, or only use offset // max(n or total, 1) + 1 when n == limit and fall back to something like math.ceil(total / limit) on the last page.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| """ | ||
| import aelfrice.mcp_server as mod | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
| import ast | ||
| import aelfrice.mcp_server as mod | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
| "openWorldHint", | ||
| } | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
| import ast | ||
| import aelfrice.mcp_server as mod | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
| """ | ||
| import aelfrice.mcp_server as mod | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
| def _serve_function() -> ast.FunctionDef: | ||
| import aelfrice.mcp_server as mod | ||
|
|
||
| src = open(mod.__file__, "r", encoding="utf-8").read() |
|
|
||
| _RESPONSE_FORMAT_JSON: Final[str] = "json" | ||
| _RESPONSE_FORMAT_MARKDOWN: Final[str] = "markdown" | ||
| _RESPONSE_FORMATS: Final[frozenset[str]] = frozenset( |
|
|
||
|
|
||
| def _serve_function() -> ast.FunctionDef: | ||
| import aelfrice.mcp_server as mod |
Owner
Author
|
[claim:review:Toug:2026-05-09T06:04:35Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Owner
Author
|
Status: Changes requested — pytest (3.12) + deptry both failing Two real CI failures need fixes before merge. 1.
|
robotrocketscience
added
attn:unblock
Owner
Author
|
[release:review:Toug:2026-05-09T06:05:51Z] |
robotrocketscience
force-pushed
the
feat/mcp-server-properly-built
branch
from
May 9, 2026 06:10
e32279d to
e08b2b2
Compare
robotrocketscience
added
attn:review
Owner
Author
|
Force-pushed e08b2b2 with both fixes from the review:
Also rebased onto current Re-flagged |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Owner
Author
|
[claim:review:Gylf:2026-05-09T06:19:14Z] |
… server The MCP server module shipped without an entry point. pyproject.toml [project.scripts] exposed no aelf-mcp; cli.py had no `aelf mcp` subcommand; mcp_server.py had no __main__ block. Hosts configuring an MCP server entry had nothing to point at. Adds: - `aelf mcp` subcommand wired to a new _cmd_mcp handler that imports serve(), translates the [mcp]-extra-missing RuntimeError into an actionable stderr message + exit 1, and treats SIGINT as clean exit. - `if __name__ == "__main__": serve()` guard in mcp_server.py so `python -m aelfrice.mcp_server` is a usable fallback. - tests/test_cli_mcp.py covering: subcommand registration, --help visibility, missing-fastmcp error path, __main__ guard, module resolution, and a static guard against print()-to-stdout regressions (stdio MCP servers must keep stdout clean for JSON-RPC). Phase 1 C1 of the mcp-server-properly-built audit (closes critical gap: server is unstartable as shipped).
…uard FastMCP reads the @mcp.tool-decorated function's docstring as the tool's `description` field exposed to the host LLM. The wrappers in serve() had none, so hosts got an empty description — tools were essentially undiscoverable, even though the underlying tool_* pure handlers had their own docstrings (which FastMCP never sees). Each wrapper now has: - one-line summary of purpose - read-only / mutating posture stated in prose - Args with examples + constraint hints - Returns with the discriminating `kind` enum and full payload schema Adds tests/test_cli_mcp.py::test_every_decorated_aelf_tool_has_a_docstring as a static regression guard: parses mcp_server.py with `ast`, finds all @mcp.tool decorators inside serve(), asserts each decorated fn has a non-empty docstring. Catches future tool additions that forget the description. Phase 1 C2 of the mcp-server-properly-built audit.
…e refs
docs/MCP.md previously documented two non-existent invocation paths:
- `aelf-mcp` console script (never wired in pyproject [project.scripts])
- `python -m aelfrice.mcp_server.serve` (invalid — `python -m` runs a
module's __main__, not a function)
Replaces with the two paths actually shipped in feat/mcp-server-properly-built:
- `aelf mcp` (CLI subcommand)
- `python -m aelfrice.mcp_server` (now resolvable via the new __main__ guard)
Updated host-config example to use `command: "aelf", args: ["mcp"]` for
end-user installs; kept the `uv run --project` form as a source-checkout
variant.
docs/COMMANDS.md: adds a `mcp` row to the lifecycle table and renames the
`upgrade` row to `upgrade-cmd` to match the post-#427 canonical name (the
deprecated alias note is preserved).
Phase 1 C3 of the mcp-server-properly-built audit. Server is now both
startable AND documented.
This gate closes phase 1 of the mcp-server-properly-built audit. Three atomic commits on this branch resolve the two CRITICAL findings (server unstartable as shipped; tools have empty descriptions) and add docs for the new entrypoint. Commits in scope: 9adca85 feat(mcp): add `aelf mcp` subcommand + python -m fallback 904bfb0 feat(mcp): docstrings on all 12 @mcp.tool wrappers + AST guard 455eaac docs(mcp): document `aelf mcp` entrypoint + fix stale refs Verification: - pytest tests/test_cli_mcp.py tests/test_mcp_server.py: 48 passed (40 prior + 8 new) - aelf --help: 'mcp' subcommand appears in subcommand list with help string - aelf mcp (no [mcp] extra): exits 1, stderr "error: fastmcp is not installed" - AST static guard test_every_decorated_aelf_tool_has_a_docstring: passes - AST static guard test_mcp_handlers_never_print_to_stdout: passes - python -m aelfrice.mcp_server: importlib resolves the module - docs/MCP.md: install/run section now matches shipped behavior - docs/COMMANDS.md: lifecycle table includes mcp row, upgrade-cmd canonical Blockers (require user decision before next phase): - [user] Greenlight phase 2 (annotations + instructions= + tool_lock assert fix + stale-docstring fix) on this same branch? - [user] Phase 1 alone is mergeable as-is — server works for the first time. Open PR now or wait for phases 2-4 to land first? - [user] Re-run `uv tool upgrade aelfrice` on your local install? Phase 1 has no impact until installed; you're still on v1.6.0. Open questions: (none) Rollback: - git revert 455eaac 904bfb0 9adca85 # in this order; rolls phase 1 back to a40d546
…potent/openWorld hints
MCP hosts use tool annotations to gate dangerous operations (e.g. require
human approval for destructiveHint=True tools, or auto-allow read-only
ones). Without annotations, fastmcp's defaults are destructiveHint=True
+ openWorldHint=True per spec — the worst-of-both-worlds default that
forces approval prompts on read-only tools and discloses no constraint
on actually-destructive ones.
Annotation matrix (all openWorldHint=False — local SQLite, no network):
Tool read destr idem
aelf_search Y N Y (FTS5 lookup)
aelf_locked Y N Y (list)
aelf_stats Y N Y (counts)
aelf_health Y N Y (regime classifier)
aelf_lock N N Y (re-lock = upgrade, content-addressed)
aelf_validate N N Y (origin promotion; already_validated no-op)
aelf_unlock N N Y (lock clear; already_unlocked no-op)
aelf_promote N N Y (alias of validate)
aelf_demote N Y N (drops a tier; only mutating tool tagged destructive)
aelf_feedback N N N (Beta posterior shifts each call)
aelf_confirm N N N (Beta posterior shifts each call)
aelf_onboard N N N (start/accept/status — worst-case posture)
Adds tests/test_cli_mcp.py::test_every_decorated_aelf_tool_has_annotations
as a static AST guard: every @mcp.tool() must pass an `annotations={...}`
dict containing all four required hint keys. Catches future tool
additions that forget to annotate.
Phase 2 M1 of the mcp-server-properly-built audit.
…mment Adds a module-level _SERVER_INSTRUCTIONS constant and passes it to the FastMCP(...) constructor. Hosts that surface the instructions field (Claude Desktop, recent fastmcp clients) now receive a server-level overview at registration time grouping tools by READ / WRITE / TIER posture and naming the local-only network constraint. The overview is concise on purpose: hosts treat instructions as a hint, not a manual; per-tool docstrings (added in 904bfb0) carry detail. Also fixes the stale module docstring claim "exposing the 9 user-visible tools" — actual count is 12 (onboard, search, lock, locked, demote, validate, unlock, promote, feedback, confirm, stats, health). Adds unlock + promote rows to the surface table (they were missing entirely). Adds tests/test_cli_mcp.py::test_server_passes_instructions_to_fastmcp as a static AST guard on the constructor call + a sanity check that _SERVER_INSTRUCTIONS is non-trivial (>100 chars stripped). Phase 2 M2 + M3 of the mcp-server-properly-built audit.
…r on empty derivation
`tool_lock` had `assert derived.belief is not None` directly on the hot
path. When the classifier sets persist=False (empty input post-strip,
question-shaped statement, anything else the derivation worker rejects),
the assert fires and the MCP tool surface crashes with an unhandled
exception. A crashing tool is far worse host-UX than a kind-tagged error
the agent can read and act on.
Replaces with a structured return:
{"kind": "lock.error", "id": "", "action": "error",
"error": "derivation produced no belief from the supplied statement
(likely empty after normalization)"}
Matches the existing lock.error shape used downstream (run_worker
reported empty derived_belief_ids list) and adds the populated `error`
field for actionability.
Adds tests/test_mcp_lock_via_worker.py::
test_lock_returns_structured_error_when_derivation_yields_no_belief —
monkeypatches `derive` to return DerivationOutput(belief=None,
skip_reason="empty") and asserts the new shape.
Phase 2 M4 of the mcp-server-properly-built audit. Closes the last
phase-2 finding (#9 in the audit list).
Tools previously took raw primitives (str, int, bool, dict). FastMCP's
auto-schema-from-typehints exposed those without descriptions, length
limits, or value patterns. The host LLM saw `aelf_lock(statement: str)`
with no hint about what 'statement' meant or how long it could be.
Adds `Annotated[type, Field(...)]` constraints to every wrapper param:
- statement (lock): min_length=1, max_length=2000
- query (search): min_length=1, max_length=500
- budget (search): ge=1, le=100_000
- belief_id (six): min_length=1, max_length=64 — shared _BeliefId alias
- source (four): max_length=128 — shared _SourceLabel alias
- signal (feedback): pattern=r"^(used|harmful)$" (still validated at
runtime; pattern is a hint to the host LLM)
- note (confirm): max_length=2000
- path (onboard): max_length=4096
- session_id: max_length=128
- pressured (locked): bool with description
- classifications: list[dict] with description
`pydantic.Field` is imported lazily inside serve() (after fastmcp is
confirmed present) so `aelfrice.mcp_server` stays importable without
the [mcp] extra — preserving the existing test_module_imports_without_
fastmcp invariant.
Field aliases (_BeliefId, _SourceLabel) are inline inside serve() for
the same reason: they reference the lazily-imported Field symbol.
Phase 3 I1 of the mcp-server-properly-built audit. Closes major audit
gap #4 (no Pydantic input validation).
Per MCP best-practices guidance: tools that list resources must respect a limit param, return pagination metadata (has_more, next_offset, total), and never load unbounded result sets. tool_locked previously returned the entire locked-belief list with no bound — fine at today's typical corpus size (<100 locks) but a latent footgun for power users or future expansion. Pure handler tool_locked now takes: - limit (default 50, clamped to [1, 500]) - offset (default 0, clamped at zero on the low end) Defensive clamping happens in the pure handler, not just the wrapper — keeps tool_locked safe when called directly outside the FastMCP layer (no Pydantic constraint enforcement there). Return shape adds: total, offset (echoed), has_more, next_offset (None when has_more is False). Existing keys (kind, n, locked) preserved. Wrapper aelf_locked exposes the new params via Annotated[..., Field] with descriptions matching the audit guidance. 4 new tests cover: default-page first/second-page round-trip, oversize limit clamping to _LOCKED_MAX_LIMIT, negative offset clamping to zero. Phase 3 I3 of the mcp-server-properly-built audit. Closes audit gap #12 (tool_locked returns ALL locked beliefs with no pagination).
…ats/health)
Per MCP best-practices guidance: tools that return structured data
should support both JSON (machine-readable, default) and Markdown
(human-readable). For aelfrice's four read-only tools, the LLM in the
host loop natively reads JSON dicts; markdown is only useful when raw
tool output flows through to a human display surface.
Implements the markdown path with an always-dict return wrapper to
preserve the dict-only return type:
{"kind": "<original.kind>.markdown",
"format": "markdown",
"text": "rendered string"}
Per-tool renderers (`_render_search_markdown`, `_render_locked_markdown`,
`_render_stats_markdown`, `_render_health_markdown`) live as
module-level helpers so they're testable without fastmcp installed.
The pure handlers tool_search / tool_locked / tool_stats / tool_health
gain a `response_format: str = "json"` kwarg; default behavior is
unchanged. Wrappers expose the param via a shared _ResponseFormat
Annotated alias with `pattern=r"^(json|markdown)$"` so hosts get a
clean validation error on unknown formats.
Defensive: pure handlers fall through to JSON for unrecognized format
strings rather than raising — covered by a regression test.
6 new tests cover: markdown wrapping for each of the 4 read tools,
JSON default unchanged, unknown format fall-through.
Phase 3 I2 of the mcp-server-properly-built audit. Closes audit minor
gap #11 (no JSON/Markdown response_format).
Closes the last audit major-gap: registration layer was untested. All
40 prior MCP tests targeted pure tool_* handlers; nothing exercised
the @mcp.tool-decorated aelf_* wrappers, so any regression in the
decorator-call shape, store lifetime, or annotation propagation would
have shipped silently.
Two test strategies in tests/test_mcp_wrapper_layer.py:
1. Static AST guards (work without fastmcp installed):
- test_all_expected_wrappers_present: 12 aelf_* wrappers exist
- test_each_wrapper_calls_its_matching_pure_handler: aelf_X delegates
to tool_X (catches typos in delegation)
- test_each_wrapper_opens_and_closes_store: try/finally store
lifetime hygiene (catches leak refactors)
2. fastmcp shim (works against any stub interpreter):
- Installs a minimal _FakeFastMCP into sys.modules['fastmcp']
- Reloads aelfrice.mcp_server, calls serve()
- Captures every @mcp.tool registration + decorator kwargs
- Asserts: 12 tools registered, all have annotations, all four
hint keys present, readOnlyHint set is exactly the 4 read tools,
destructiveHint set is exactly {aelf_demote}, instructions= passed,
name="aelfrice" passed.
The shim also stubs `pydantic` if absent in the dev env, since serve()
imports `pydantic.Field` lazily after the fastmcp import.
Phase 4 of the mcp-server-properly-built audit. 76 MCP tests now
cover the full surface (40 pure handlers + 8 cli/main + 28 wrapper +
markdown + pagination).
…dy for PR
Closes the mcp-builder audit of src/aelfrice/mcp_server.py started this
session. Resolves both CRITICAL findings, all six MAJOR findings, and
five of the eight MINOR findings. The deferred minors are documented
in this gate body for triage in follow-up issues.
## Commits in scope (10 atomic + 1 phase-1 gate before this)
Phase 1 — server is startable and discoverable
9adca85 feat(mcp): aelf mcp subcommand + python -m fallback
904bfb0 feat(mcp): docstrings on all 12 wrappers + AST regression guard
455eaac docs(mcp): document `aelf mcp` entrypoint + fix stale refs
46df710 gate: phase 1
Phase 2 — well-formed
a7576ad feat(mcp): annotations on every @mcp.tool
232d8ab feat(mcp): instructions= overview + 9-vs-12 stale comment fix
5f21c29 fix(mcp): tool_lock structured error vs AssertionError
Phase 3 — input/output discipline
6c6bf6c feat(mcp): Pydantic Field constraints via Annotated
174d245 feat(mcp): cursor pagination on tool_locked
7f04675 feat(mcp): response_format=markdown on read tools
Phase 4 — wrapper-layer testing
0214c17 test(mcp): wrapper-layer tests via static AST + fastmcp shim
## Verification
- pytest 76/76 passing across MCP test files (40 prior + 36 new):
test_mcp_server.py, test_mcp_lock_via_worker.py,
test_mcp_wrapper_layer.py, test_cli_mcp.py
- aelf --help: 'mcp' subcommand visible with help string
- aelf mcp (no [mcp] extra): exits 1 with actionable stderr
- Static AST guards: docstrings, annotations, instructions=, store
lifetime, no print()-to-stdout, all green
- fastmcp shim test: 12 tools register, all with full annotations,
read-only / destructive sets match expected
- Discretion grep: clean (only mentions Claude Code/Desktop, both
pre-existing on main as the intended MCP host targets)
## Audit findings closure
CRITICAL #1 Server unstartable → CLOSED (Phase 1 C1)
CRITICAL #2 Empty tool descriptions → CLOSED (Phase 1 C2)
MAJOR #3 No tool annotations → CLOSED (Phase 2 M1)
MAJOR #4 No Pydantic input validation → CLOSED (Phase 3 I1)
MAJOR #5 No server instructions= → CLOSED (Phase 2 M2)
MAJOR #6 No README/docs MCP setup section → CLOSED (Phase 1 C3)
MAJOR #7 Registration layer untested → CLOSED (Phase 4)
MAJOR #8 tool_lock hard assert → CLOSED (Phase 2 M4)
MINOR #11 No response_format enum → CLOSED (Phase 3 I2)
MINOR #12 No pagination on aelf_locked → CLOSED (Phase 3 I3)
MINOR #13 Stale 9-vs-12 tools comment → CLOSED (Phase 2 M3)
## Deferred (file follow-up issues)
MINOR #1 Server name "aelfrice" vs convention "aelfrice_mcp"
— backwards-compat break, not load-bearing. Defer.
MINOR #6 Sync handlers (no async def) — defensible for SQLite,
revisit if/when an async I/O dep lands.
MINOR #14 Polymorphic tool_onboard (3 input shapes in one tool)
— design call. Re-evaluate after host telemetry.
MINOR #15 Untyped fastmcp cast `_FastMCP: Any` — fastmcp lacks
stubs upstream. Pragmatic; revisit when stubs ship.
## Behavior changes worth flagging in PR description
- `tool_locked` return shape: ADDED keys total/has_more/next_offset.
Existing keys (kind/n/locked) preserved. Callers that consumed
`n` as "total locks across all locks" should switch to `total`.
- `tool_locked` returns first 50 by default instead of all locks. Tests
on stores with <50 locks behave identically.
- `tool_lock` returns lock.error dict (with `error` field) instead of
raising AssertionError on empty derivation. Callers that grep for
AssertionError need to update; callers that check `out["kind"]` are
fine.
## Blockers (require user decision before next phase)
- [user] Open PR? Branch is local-only on `feat/mcp-server-properly-built`.
Need a `git push -u github feat/mcp-server-properly-built` to land it,
then `gh pr create`. Not done by this gate.
- [user] File the four deferred-minor follow-up issues now or after
PR merge?
- [user] Resume parked feat/aelf-upgrade-imperative work next session?
That branch has no commits but the worktree state is captured in the
end-of-session handoff.
Open questions: (none)
Rollback:
- Whole branch: never merged → just delete `feat/mcp-server-properly-built`.
- Per-phase: `git revert <phase-commit-range>` (see commit list above).
The pre-push hook's banned-vocabulary list (~/projects/aelfrice/.git/
hooks/pre-push) flags any added line containing 'Claude Code' or other
host-product names — the policy treats even legitimate product
references in committed content as session-context leakage.
Two added lines on this branch tripped the check:
- docs/MCP.md:28 host-config preamble that listed host products
by name (the equivalent line on main also names 'Claude Code'
but the hook only checks `+` lines, so unmodified ones pass)
- src/aelfrice/cli.py:4388 docstring/comment naming host products
as the audience
Scrubbed both:
- docs/MCP.md: 'Host config — any MCP-capable host:' (was
'Host config — Claude Code, Codex, Claude Desktop, any MCP-capable
host:')
- cli.py: 'MCP-capable hosts configuring a server entry can discover
it' (was 'hosts (Claude Desktop / Claude Code) configuring an MCP
entry can discover it')
Net cumulative diff (main..HEAD) no longer has any `+` line
containing banned vocab. Pre-push hook now passes.
Functionality and link semantics are unchanged — the host-product
names were prose context, not normative content.
mcp_server.py imports `from pydantic import Field` lazily inside serve() (gated to fastmcp availability) for tool-parameter Annotated constraints. Previously satisfied transitively via fastmcp; deptry's static analysis flags the direct import as undeclared. Declared in the existing [mcp] optional group so the default install stays lean (pydantic only resolves when [mcp] is requested, alongside fastmcp which already pulls it transitively).
robotrocketscience
force-pushed
the
feat/mcp-server-properly-built
branch
from
May 9, 2026 06:20
e08b2b2 to
75f66af
Compare
3 tasks
Owner
Author
|
[release:review:Gylf:2026-05-09T06:23:11Z] |
robotrocketscience
added a commit
that referenced
this pull request
May 9, 2026
…itecture Three diagrams Sourcery generated against PRs #512 and #513 that map the moving parts visually: 1. /aelf:upgrade orchestrator (sequence) → docs/SLASH_COMMANDS.md The four-step Bash orchestration: detect → execute → refresh slash bundle → clear banner cache. 2. detect_reachable_installs() running-venv suppression (flowchart) → docs/SLASH_COMMANDS.md Why uv-run no longer triggers a spurious "multiple aelfrice installs detected" warning. 3. aelf-mcp startup + tool dispatch (sequence) + wrapper/handler layering (class) → docs/MCP.md The CLI → serve() → FastMCP → wrapper → pure-handler call graph plus the response-format / pagination / pydantic-typed surface. The MCP architecture diagram references symbols (`aelf mcp`, `_SERVER_INSTRUCTIONS`, `_wrap_markdown`, `_LOCKED_*_LIMIT`, `_BeliefId`) introduced by PR #512, which is in `attn:unblock` at time of writing. The pre-existing handler/wrapper structure is on main today; the rest becomes accurate when #512 lands. Source attribution noted at the bottom of each diagram block.
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
2 participants
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.
Summary
Audit of
src/aelfrice/mcp_server.pyagainst themcp-builderskill. Closes both critical findings, all six major findings, and five of eight minor findings; four minors deferred (issues to follow).11 atomic commits + 2 gate commits. 76 MCP tests (40 prior + 36 new).
Audit findings closed
instructions=overviewtool_lockhardassertresponse_formatenumaelf_lockedBehavior changes worth flagging
tool_lockedreturn shape: ADDED keystotal/has_more/next_offset. Existing keys (kind/n/locked) preserved. Callers that consumednas "total locks across all locks" should switch tototal.tool_lockedpaginates by default: returns first 50, not all. Tests on stores with <50 locks behave identically.tool_lockreturns structuredlock.errordict (witherrorfield) instead of raisingAssertionErroron empty derivation.aelf mcpsubcommand +python -m aelfrice.mcp_serverentry point (server was previously unstartable).response_format="markdown"for human-display flow. JSON is the default; existing callers unaffected.Deferred (follow-up issues to file)
aelfricevs conventionaelfrice_mcptool_onboard(3 input shapes in one tool) — design call, re-evaluate after host telemetry_FastMCP: Anycast — pragmatic until fastmcp ships type stubsTest plan
tests/test_mcp_server.py, 50+ tests)tests/test_cli_mcp.py, 8 tests)tests/test_mcp_lock_via_worker.py, 6 tests including new error-path coverage)tests/test_mcp_wrapper_layer.py, 10 tests — static AST guards + fastmcp shim)aelf --helpshowsmcpsubcommandaelf mcp(no[mcp]extra) exits 1 with actionable stderrinstructions=, store lifetime, noprint()to stdoutmainSummary by Sourcery
Add a fully startable MCP server with documented tool surface, richer responses, and CLI entrypoint, while tightening validation, annotations, and tests around MCP integration.
New Features:
aelf mcpCLI subcommand andpython -m aelfrice.mcp_servermodule entry point.Bug Fixes:
lock.errorresponse from the lock tool when belief derivation fails instead of crashing with an assertion error.Enhancements:
Documentation:
mcpsubcommand.Tests:
mcpsubcommand and the MCP wrapper/registration layer.Summary by CodeRabbit
Release Notes
New Features
aelf mcpCLI subcommand to run the FastMCP stdio serverDocumentation
upgrade-cmdis now the primary upgrade command;upgrademarked as deprecated