refactor(mcp): remove the MCP surface and clean up after it on upgrade (#1422) - #1425
Merged
github-actions[bot] merged 17 commits intoAug 9, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR removes the MCP server, dependency extra, CLI command, documentation, and MCP-specific tests. It adds report-only stale-registration detection and explicit configuration removal with backups. CLI behavior remains available, and historical ChangesMCP removal and migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SetupCLI
participant McpCleanup
participant HostConfig
User->>SetupCLI: run aelf setup
SetupCLI->>McpCleanup: detect stale MCP state
McpCleanup->>HostConfig: scan known configuration files
HostConfig-->>McpCleanup: matching registrations and notes
McpCleanup-->>SetupCLI: report cleanup state
User->>SetupCLI: run aelf migrate --remove-mcp-config
SetupCLI->>McpCleanup: remove matching registrations
McpCleanup->>HostConfig: create backup and update JSON
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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
Reviewer's GuideRemoves the broken FastMCP-based MCP server surface (code, CLI verb, tests, docs, optional extra) and replaces it with a conservative, one-shot upgrade-time cleanup plus an opt-in migration command, while porting all meaningful MCP-only behaviour and invariants to CLI/library paths (notably confirm, lock/demote/remember semantics and mcp_remember store compatibility) and wiring a host-scoped sentinel so the cleanup runs once per machine from Sequence diagram for the updated confirm CLI path using apply_feedbacksequenceDiagram
actor User
participant CLI as cli._cmd_confirm
participant Store as MemoryStore
participant Feedback as feedback.apply_feedback
User->>CLI: aelf confirm <belief_id> [--source S] [--note TEXT]
CLI->>Store: _open_store()
activate Store
CLI->>Feedback: apply_feedback(store, belief_id, valence=1.0, source, respect_lock=False)
activate Feedback
Feedback->>Store: assert_local_ownership(belief_id)
Store-->>Feedback: (may raise ForeignBeliefError)
Feedback-->>CLI: FeedbackResult(prior_alpha, new_alpha, new_beta)
deactivate Feedback
alt ValueError or ForeignBeliefError
CLI-->>User: stderr "confirm error: <message>" (exit 1)
else Success
CLI-->>User: stdout "confirmed <id>: alpha a->b, mean m [note]"
CLI->>CLI: _feed_log_event("feedback.applied", ...)
end
CLI->>Store: close()
deactivate Store
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
|
[claim:review:Setr:2026-08-06T22:15:33Z] |
Owner
Author
Review: the removal is sound; the new cleanup path had six machine-reachable defectsThe deletion half holds up — I ran the full suite and traced the deleted surface for surviving references, and the 1. Locally-scoped registrations were invisible — and the sentinel latched anyway (major)
This is not hypothetical. Checked structurally against a real host config: the top-level
2. The backup named as the undo path was clobbered (major)The stamp is second-resolution. Two registrations removed from one file in one Verified on a three-entry config: two backups are written and one holds the untouched original. 3–6 (minor, all fixed)
A note on the shape of theseFive of the six are failures of reporting rather than of deletion — the routine is appropriately conservative about editing, and nothing here destroyed data it was not asked to. But four of them end in the same place: the user is told nothing, or told something false, and the sentinel makes it permanent. For a one-shot migration the latch is the amplifier, which is why I gated it on a complete scan rather than only fixing the individual blind spots. CIActions is still recovering from the outage, so the required checks have not run on this head. The suite figure above is local, run exactly as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/post-release-docs-issue.yml (1)
18-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin
actions/checkoutto a commit SHA.
actions/checkout@v4is a mutable reference. Replace it with a reviewed commit SHA and retain the version in a trailing comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/post-release-docs-issue.yml at line 18, Update the actions/checkout step to reference a reviewed immutable commit SHA instead of the mutable v4 tag, and retain the checkout version in a trailing comment.Source: Path instructions
docs/user/COMMANDS.md (1)
72-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new MCP cleanup option.
The current
migrateentry still lists only--from,--apply, and--all. Add--remove-mcp-configand document its stale-registration report, timestamped backups, and opt-in removal behavior.Proposed documentation update
-| `migrate [--from P] [--apply] [--all]` | Port beliefs from the legacy global DB into the active project's per-project DB. Dry-run by default. Read-only on the source. | +| `migrate [--from P] [--apply] [--all] [--remove-mcp-config]` | Port beliefs from the legacy global DB into the active project's per-project DB. `--remove-mcp-config` reports stale MCP registrations and extras, then removes them only after timestamped backups. No configuration is edited without this flag. |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user/COMMANDS.md` at line 72, Update the migrate command documentation to include the new --remove-mcp-config option alongside --from, --apply, and --all. Describe that it reports stale MCP registrations, creates timestamped backups, and removes them only when explicitly enabled.
🧹 Nitpick comments (5)
tests/test_lock_management.py (2)
424-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the single-element slice with
next(iter(...)).Ruff reports RUF015 on both lines.
list(...)[0]materialises the whole result to read one element.♻️ Proposed refactor
- bid = list(s.list_locked_beliefs())[0].id + bid = next(iter(s.list_locked_beliefs())).idAlso applies to: 440-440
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lock_management.py` at line 424, In the affected lock-management tests, replace each list_locked_beliefs() result materialized with list(...)[0] by retrieving the first element via next(iter(...)). Update both occurrences while preserving the existing .id access and test behavior.Source: Linters/SAST tools
217-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree new CLI helpers hand-roll the same
AELFRICE_DBsave/set/restore block. pytest'smonkeypatch.setenvperforms the same swap and restores it during teardown, including on an unexpected exception path. The shared root cause is one duplicated env-swap idiom across the migrated CLI tests.
tests/test_lock_management.py#L217-L230: accept amonkeypatchargument in_demote_via_cliand callmonkeypatch.setenv("AELFRICE_DB", str(db)), then drop the manual save/restore.tests/test_lock_management.py#L368-L382: apply the same change to_lock_via_cli.tests/test_ingest_log.py#L438-L471: replace the inlineos.environblock intest_relock_appends_a_log_row_and_corroborateswithmonkeypatch.setenv, and extract the shared helper if the same pattern is needed again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lock_management.py` around lines 217 - 230, Replace the duplicated manual AELFRICE_DB environment save/set/restore logic with pytest monkeypatch handling: update _demote_via_cli and _lock_via_cli to accept monkeypatch and call setenv, update tests/test_lock_management.py:217-230 and tests/test_lock_management.py:368-382 accordingly, and replace the inline environment block in tests/test_ingest_log.py:438-471 within test_relock_appends_a_log_row_and_corroborates with monkeypatch.setenv; extract a shared helper there only if the pattern is reused.src/aelfrice/mcp_cleanup.py (2)
411-417: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider an atomic write for the edited config.
path.write_texttruncates before it writes. If the process stops mid-write, the host config is left truncated. The backup makes the data recoverable, but the user must restore it by hand. A write to a temporary file in the same directory followed byos.replaceremoves that window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/mcp_cleanup.py` around lines 411 - 417, Replace the direct path.write_text call in the configuration write flow with an atomic same-directory temporary-file write, then commit it using os.replace only after the complete JSON content is written successfully. Preserve UTF-8 encoding, formatting, the existing backup behavior, and the current OSError return message.
162-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
itertools.pairwisefor the adjacent-pair scan.Ruff reports B905 (missing
strict=) and RUF007 on thiszipcall.itertools.pairwisestates the intent directly and removes both warnings.♻️ Proposed refactor
- for first, second in zip(arg_list, arg_list[1:]): + for first, second in pairwise(arg_list): if first in _AELF_COMMANDS and second in _AELF_SUBCOMMANDS: return TrueAdd the import near the other stdlib imports:
from dataclasses import dataclass, field, replace from datetime import datetime, timezone +from itertools import pairwise from pathlib import Path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/mcp_cleanup.py` around lines 162 - 167, Replace the zip-based adjacent-pair scan in the command-detection function with itertools.pairwise, adding the required stdlib import alongside the existing imports. Preserve the current checks against _AELF_COMMANDS and _AELF_SUBCOMMANDS and the existing boolean return behavior.Source: Linters/SAST tools
src/aelfrice/cli.py (1)
6164-6165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--remove-mcp-configsilently ignores the othermigrateflags.The early return runs before any validation.
aelf migrate --remove-mcp-config --apply --from Xperforms only the MCP removal and never reports that--applyand--fromwere ignored. Consider rejecting the combination, or state the exclusivity in the help text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/cli.py` around lines 6164 - 6165, Update the migrate argument handling around remove_mcp_config so --remove-mcp-config cannot silently ignore other migration flags such as --apply or --from. Validate the combination before the early return in the _remove_mcp_config path and report an invalid combination, or explicitly document and enforce that this option is exclusive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/ISSUE_TEMPLATE/question.yml:
- Line 13: Update the COMMANDS link in the question template to target
docs/user/COMMANDS.md instead of docs/COMMANDS.md, preserving the existing
repository URL and link text.
In `@src/aelfrice/cli.py`:
- Around line 6141-6152: Update _remove_mcp_config around find_registrations to
detect the scan-incomplete marker and return a non-zero status when any
configuration was unreadable or invalid, rather than reporting success for no
registrations. Expose the existing _SCAN_INCOMPLETE marker from mcp_cleanup as
SCAN_INCOMPLETE (or provide a public alias), and use that symbol to distinguish
incomplete scans from a clean empty result.
In `@src/aelfrice/federation.py`:
- Line 55: Update the documentation text near the federation exception handling
to say that CLI error messages surface belief_id and owning_scope, replacing the
inaccurate reference to CLI exit codes; leave the surrounding exception behavior
unchanged.
In `@src/aelfrice/mcp_cleanup.py`:
- Around line 418-421: Update the success message returned by the removal flow
to use Registration.location() instead of hardcoding
mcpServers.<registration.key>, while preserving the existing path, backup, and
formatting details.
- Around line 319-326: Update maybe_clean_up_mcp when extracting requirements
from receipt so a non-table tool value is treated as an invalid receipt and
returns False instead of calling .get on it. Validate the tool value before
accessing requirements, while preserving the existing list-type validation and
valid receipt behavior.
In `@src/aelfrice/wonder/dispatch.py`:
- Line 13: Update the docstrings in src/aelfrice/wonder/dispatch.py at lines
13-13, 112-112, and 432-432: document both supported forms, “aelf wonder QUERY”
and “aelf wonder --axes QUERY”; state at line 112 that both forms return the
payload; and at line 432 correct the invocation order and explicitly state that
the CLI serializes and returns the payload.
In `@tests/test_mcp_remember_reader_contract_1422.py`:
- Around line 78-100: Revise
test_a_store_holding_an_mcp_remember_row_still_opens_and_reads to exercise a
legacy persisted record rather than inserting a current row into
MemoryStore(":memory:"). Seed a file-backed SQLite database or use the existing
legacy fixture with the historical source_kind value, close it, reopen it
through MemoryStore, and assert the record remains readable. Move the current
derive and insertion assertions into a separate test.
---
Outside diff comments:
In @.github/workflows/post-release-docs-issue.yml:
- Line 18: Update the actions/checkout step to reference a reviewed immutable
commit SHA instead of the mutable v4 tag, and retain the checkout version in a
trailing comment.
In `@docs/user/COMMANDS.md`:
- Line 72: Update the migrate command documentation to include the new
--remove-mcp-config option alongside --from, --apply, and --all. Describe that
it reports stale MCP registrations, creates timestamped backups, and removes
them only when explicitly enabled.
---
Nitpick comments:
In `@src/aelfrice/cli.py`:
- Around line 6164-6165: Update the migrate argument handling around
remove_mcp_config so --remove-mcp-config cannot silently ignore other migration
flags such as --apply or --from. Validate the combination before the early
return in the _remove_mcp_config path and report an invalid combination, or
explicitly document and enforce that this option is exclusive.
In `@src/aelfrice/mcp_cleanup.py`:
- Around line 411-417: Replace the direct path.write_text call in the
configuration write flow with an atomic same-directory temporary-file write,
then commit it using os.replace only after the complete JSON content is written
successfully. Preserve UTF-8 encoding, formatting, the existing backup behavior,
and the current OSError return message.
- Around line 162-167: Replace the zip-based adjacent-pair scan in the
command-detection function with itertools.pairwise, adding the required stdlib
import alongside the existing imports. Preserve the current checks against
_AELF_COMMANDS and _AELF_SUBCOMMANDS and the existing boolean return behavior.
In `@tests/test_lock_management.py`:
- Line 424: In the affected lock-management tests, replace each
list_locked_beliefs() result materialized with list(...)[0] by retrieving the
first element via next(iter(...)). Update both occurrences while preserving the
existing .id access and test behavior.
- Around line 217-230: Replace the duplicated manual AELFRICE_DB environment
save/set/restore logic with pytest monkeypatch handling: update _demote_via_cli
and _lock_via_cli to accept monkeypatch and call setenv, update
tests/test_lock_management.py:217-230 and tests/test_lock_management.py:368-382
accordingly, and replace the inline environment block in
tests/test_ingest_log.py:438-471 within
test_relock_appends_a_log_row_and_corroborates with monkeypatch.setenv; extract
a shared helper there only if the pattern is reused.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: af4264cf-fca2-4f22-aee4-dcea8bbfc596
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (64)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/question.yml.github/workflows/post-release-docs-issue.ymlCHANGELOG/v4.mdCITATION.cffCONTRIBUTING.mdREADME.mdSECURITY.mddocs/README.mddocs/concepts/ARCHITECTURE.mddocs/concepts/HARNESS_INTEGRATION.mddocs/concepts/PHILOSOPHY.mddocs/concepts/ROADMAP.mddocs/user/COMMANDS.mddocs/user/CONFIG.mddocs/user/INSTALL.mddocs/user/LIMITATIONS.mddocs/user/MCP.mddocs/user/PRIVACY.mddocs/user/README.mddocs/user/SLASH_COMMANDS.mdpyproject.tomlsrc/aelfrice/bm25.pysrc/aelfrice/classification.pysrc/aelfrice/cli.pysrc/aelfrice/db_paths.pysrc/aelfrice/derivation.pysrc/aelfrice/detector_thresholds.pysrc/aelfrice/federation.pysrc/aelfrice/feedback.pysrc/aelfrice/hook.pysrc/aelfrice/lifecycle.pysrc/aelfrice/lock_expiry.pysrc/aelfrice/mcp_cleanup.pysrc/aelfrice/mcp_server.pysrc/aelfrice/models.pysrc/aelfrice/promotion.pysrc/aelfrice/retrieval.pysrc/aelfrice/scoring.pysrc/aelfrice/session_resolution.pysrc/aelfrice/setup.pysrc/aelfrice/store.pysrc/aelfrice/wonder/__init__.pysrc/aelfrice/wonder/dispatch.pysrc/aelfrice/wonder/result.pytests/conftest.pytests/test_cli_confirm.pytests/test_cli_mcp.pytests/test_confirm_posterior_shift.pytests/test_derivation.pytests/test_ingest_log.pytests/test_llm_classifier.pytests/test_lock_management.pytests/test_mcp_cleanup_1422.pytests/test_mcp_lock_via_worker.pytests/test_mcp_remember_reader_contract_1422.pytests/test_mcp_server.pytests/test_mcp_wrapper_layer.pytests/test_phantom_lifecycle_counts.pytests/test_session_id_population_rate.pytests/test_session_id_propagation.pytests/test_slash_commands.pytests/test_to_scope.pytests/test_uninstall_dotdir.py
💤 Files with no reviewable changes (14)
- docs/user/README.md
- src/aelfrice/session_resolution.py
- tests/test_slash_commands.py
- .github/ISSUE_TEMPLATE/bug_report.yml
- pyproject.toml
- src/aelfrice/mcp_server.py
- tests/test_cli_mcp.py
- docs/user/INSTALL.md
- docs/user/MCP.md
- tests/test_mcp_server.py
- tests/test_phantom_lifecycle_counts.py
- tests/test_to_scope.py
- tests/test_mcp_lock_via_worker.py
- tests/test_mcp_wrapper_layer.py
Owner
Author
|
[claim:review:Garsecg:2026-08-06T22:54:59Z] |
Owner
Author
|
[release:review:Garsecg:2026-08-06T22:55:04Z] |
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
Owner
Author
|
[claim:review:Gylf:2026-08-06T22:59:30Z] |
Owner
Author
|
[release:review:Gylf:2026-08-06T22:59:35Z] |
robotrocketscience
force-pushed
the
refactor/issue-1422-remove-mcp-surface
branch
from
August 6, 2026 23:04
1325adf to
f4befc5
Compare
Owner
Author
All seven review threads actioned — five commits, four mutation-checkedEvery one of these was real and in scope. Taking them in order of what they cost. The one that mattered: the AC5 guard did not guard anything
Correct, and worse than stated. Replaced ( While verifying this I found my own framing was overstated, and corrected it. Correctness
Prose (
|
| mutation | result |
|---|---|
restore receipt.get("tool", {}).get(...) |
red |
restore hardcoded mcpServers.{key} |
red |
drop "mcp_remember": RETENTION_FACT |
red |
restore return 1 if failed else 0 |
red |
The last one survived on the first attempt — my initial test only exercised
the no-registrations branch, leaving or incomplete on the final return
untested. Added the partial-removal case, which kills it.
Full suite 7331 passed, 70 skipped, 71 xfailed. The one unrelated failure is
test_slot_conflict::test_latency_budget (p95 6.1ms vs a 5.0ms budget), which
reproduces on unmodified main at 2d1e4aea (1 of 7 runs there, 2 of 7 here) —
a pre-existing marginal perf test on a loaded machine, not something this branch
introduced. CI is the authority for that one and it passed there.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
robotrocketscience
force-pushed
the
refactor/issue-1422-remove-mcp-surface
branch
from
August 6, 2026 23:37
c068300 to
26cb8e1
Compare
Owner
Author
|
[claim:review:Garsecg:2026-08-06T23:48:25Z] |
Owner
Author
|
[release:review:Garsecg:2026-08-06T23:48:29Z] |
Owner
Author
|
[claim:review:Garsecg:2026-08-09T03:51:57Z] |
Owner
Author
|
[release:review:Garsecg:2026-08-09T03:52:02Z] |
Owner
Author
|
[claim:review:Idnn:2026-08-09T03:52:29Z] |
Owner
Author
|
[release:review:Idnn:2026-08-09T03:52:34Z] |
`aelf confirm` is the one feedback surface exempt from the lock floor — documented in docs/user/COMMANDS.md, implemented as a single `respect_lock=False` keyword at one call site, and until now covered by nothing. Every existing test in the file uses unlocked beliefs, where the exemption makes no difference, so dropping the kwarg would report success while the posterior silently never moved. Two arms so neither can pass vacuously: confirm moves a locked belief's alpha 1.0 -> 2.0, and plain `aelf feedback` on the same locked belief leaves it at 1.0. Flipping the kwarg to True turns the first red. This is a pre-existing gap, not one introduced here; it lands first because the call site is about to be relocated.
…module `aelf confirm` reached its implementation via `from aelfrice.mcp_server import tool_confirm`, which made the CLI depend on the MCP surface for a plain library operation. `tool_confirm` was a thin wrapper: one `apply_feedback(...)` call, a ValueError branch, and a dict assembled entirely from fields `FeedbackResult` already exposes. `note` was never persisted — it was echoed back to the caller and nothing else, so it is now read straight off `args`. `respect_lock=False` is carried across deliberately and commented as load-bearing: it is the #1168 lock-floor exemption, and the guard added in the previous commit fails if it is dropped. `ForeignBeliefError` subclasses `ValueError`, so one clause reproduces both the unknown-belief and foreign-belief rejections that the old `confirm.unknown_belief` payload carried. First step of #1422: it removes the only src/ dependency on `mcp_server` other than `serve()` itself.
…surfaces Nothing here is a mechanical import swap: `mcp_server` held real logic next to the MCP plumbing, so each file was triaged assertion by assertion into "wire format, dies with the surface" and "behaviour that must survive". Behaviour ported, not dropped: - test_ingest_log: the post-#264 re-lock contract (a second lock appends a log row and corroborates rather than duplicating) was asserted ONLY through tool_lock. The neighbouring CLI test covers a single lock and stops there, so the contract now has its own CLI-driven test. - test_lock_management: the #391 demote lock-drop audit row, and the lock -> unlock -> re-lock round trip. Both now drive `aelf lock` / `aelf demote` rather than seeding a locked row, because what they exercise is the ingest path that makes "re-lock" mean corroborate. - test_session_id_population_rate: the four tool_lock calls are replaced by four _cmd_lock calls on distinct statements, not deleted — deleting them drops the measured rate to 75% against its own >= 0.80 gate. Re-measured: 10/12 = 83.33%, composition still 12 calls. Wire-format assertions removed with the surface, each after confirming an equivalent exists: tool_stats phantom payloads (core + CLI tests cover the counts and the rendering), tool_promote/tool_demote to_scope shapes (test_to_scope's CLI block covers every case; not-found is test_cli.py:191), tool_unlock/tool_promote shape parity, and tool_confirm's return-dict tests (the CLI contract is test_cli_confirm). test_confirm_posterior_shift keeps its library-level focus on the posterior mathematics and calls apply_feedback through a helper that carries respect_lock=False, with a docstring pointing at test_cli_confirm for the command contract.
The surface has never started on any version of its declared dependency range. `serve()` fails on fastmcp 1.0 and 2.0.0 with `TypeError: FastMCP.tool() got an unexpected keyword argument 'annotations'`, on 2.10.0 with `cannot specify both default and default_factory`, and on 3.2.4 with `NameError: name 'Field' is not defined` — the last because `from pydantic import Field` is a local of `serve()` while `from __future__ import annotations` makes fastmcp resolve tool annotations against module globals. Broken since d6bafcb (2026-05-08), shipped that way in v2.0.1 and every release since. No CI job installs the extra, so nothing could see it. Removed: src/aelfrice/mcp_server.py, `_cmd_mcp` and its subparser, the "mcp" entry in tests/test_slash_commands.py HIDDEN_SUBCOMMANDS (the subparser set is asserted to equal EXPECTED u HIDDEN, so it goes red otherwise), and the four test modules that only exercised the surface. Deliberately NOT removed: the `mcp_remember` source_kind and its frozenset membership in models.py, and derivation.py's handling of it. Existing stores hold rows carrying it, and a reader that rejects a source_kind it wrote last release makes the store unopenable — the #1161 class, unrecoverable in the field. Production stops *writing* it (tool_lock was its only writer); readers keep accepting it, and tests/corpus/replay_soak/v0.1/mcp_remember_v0_1.jsonl still replays through the soak runner's glob, which is what keeps that contract green now that the writer is gone.
`fastmcp` and `pydantic` were declared only for `mcp_server.py`, which is gone; neither is imported anywhere else in src/, tests/, benchmarks/ or scripts/. uv.lock pinned fastmcp under `extra == 'mcp'` and CI runs `uv sync --frozen`, so the lockfile has to move in the same commit or every job fails on a stale lock. Verified with the exact CI invocation (`uv sync --frozen --group dev --extra archive`). No deptry per_rule_ignores entry referenced either package, so the ignore list needs no edit.
…s gone AC5 of #1422. Nothing produces `source_kind='mcp_remember'` any more, so the constants read as dead code to the next person tidying up after the removal — and deleting them is a one-line change that makes every store holding a historical row unopenable. That is the #1161 class, and it is not recoverable in the field. Four assertions: the constant is still in INGEST_SOURCE_KINDS; still in CORROBORATION_SOURCES_USER_EXPLICIT (dropping it there would silently change whether re-asserting an old statement revives a retired belief, #1215); `derive()` still gives such a row its locked priors; and a store holding one still opens and reads it back. The wire string is pinned separately, because renaming the literal orphans on-disk rows just as effectively as deleting the constant. Mutation-checked, all three killed: dropped from INGEST_SOURCE_KINDS -> red; dropped from CORROBORATION_SOURCES_USER_EXPLICIT -> red; renamed to "mcp_remember_legacy" -> red.
The README promised "any MCP host can use the included stdio server" for a surface that has never started. Struck there and everywhere else the docs assert MCP is a supported interface: docs/user/MCP.md deleted, the `mcp` row removed from COMMANDS.md, the `aelfrice[mcp]` line from INSTALL.md, the extras lists in PRIVACY/ARCHITECTURE/PHILOSOPHY, the mcp_server row from ARCHITECTURE's module table, the doc index entries, the bug_report.yml component options, the question.yml doc links, SECURITY.md's MCP threat-surface bullets, CITATION.cff, CONTRIBUTING.md, and the post-release docs checklist. Also swept the in-code prose that named MCP as a live caller — 27 docstrings and comments across store, retrieval, feedback, promotion, scoring, hook, cli, setup, federation, wonder and lock_expiry. A comment saying "long-running processes (MCP server)" is a false claim about the codebase once the server is gone, and this project treats those as defects rather than cosmetics. Deliberately untouched, and this is the rule rather than an oversight: anything that is a *dated record* instead of a current claim. `docs/audits/*` are audit snapshots of a given day; `docs/design/*` record what was designed and decided at the time, several with explicit Status headers and open TBDs; ROADMAP's shipped-version rows and PHILOSOPHY's "15 MCP tools at v3.3" are release history; store.py's #1161 narrative describes what used to break. Rewriting any of those would falsify the record — the removal changes what is true now, not what was true then.
Deleting the code does not clean up after it. Anyone who installed `aelfrice[mcp]` still carries fastmcp in their uv environment, and anyone who pasted the block from the old docs/user/MCP.md still has a host entry pointing at a command that no longer exists. A one-shot pass behind `~/.aelfrice/mcp-surface-removed`, fired from `_cmd_setup_locked` next to the #733 and #1064 migrations, reports both. Two design constraints, neither optional: **It reports; it does not edit.** aelfrice has never written an `mcpServers` key — `git grep mcpServers` on main returns two hits, both lines inside the doc that told users to paste one — so no path constant for those files exists anywhere in the tree, and the file usually holds the user's other servers. lifecycle.py:791-797 already states the house rule for exactly this: anything not named in the dotdir contract is reported and never deleted. `aelf migrate --remove-mcp-config` is the opt-in verb; it backs up to a timestamped sibling before touching anything and says so in its output. **It advises the reinstall rather than running it.** `maybe_migrate_to_uv` may shell out to `uv tool install` because it is guarded to run only when the install is NOT a uv tool install. This targets the opposite population — the dead extra lives precisely in uv-tool installs — so the extra is detected by reading uv's own receipt with stdlib tomllib (no subprocess, no network) and the command is printed for the user to run. The recognition predicate covers all four shapes this project published, not just the current doc's: `aelf mcp`, `uv run --project <abs> aelf mcp`, `aelf serve` (docs/INSTALL.md @99160871) and `python -m aelfrice.mcp_server`, plus the documented-but-never-shipped `aelf-mcp`. It never matches on the map key (users rename it) and never on `aelf` alone (forty other verbs). Sentinel registered in both conftest lists — `_HOME_PINS` and `_PRECREATED_SENTINELS`. The second is the one that matters: an exists()-guarded sentinel pinned at a fresh tmp path *arms* the guarded side effect instead of disarming it, and CI cannot catch that because a runner's HOME is empty. Also added to lifecycle's `_DOTDIR_INSTALL_STATE` and the uninstall agreement test, so the file is not orphaned on teardown. Mutation-checked, all five killed: widening the predicate to a bare command match, matching on the map key, skipping the backup, making the automatic pass edit, and swallowing parse errors silently.
The MCP mention there is not a claim that aelfrice ships an MCP server — it says the onboard classifier needs no MCP roundtrip, which stays true. Rewriting the line would re-add it as a changed line, and the line also carries a token the discretion gate blocks on additions, so touching it trades a correct doc for a blocked push. Left exactly as it was.
…ing the undo Six defects in the one-shot cleanup path, all reachable on a user's machine. Each is mutated red-then-green with __pycache__ cleared. 1. Locally-scoped registrations were invisible. _scan_file read only the top-level mcpServers map, but a local-scope server is stored under projects.<dir>.mcpServers in the same file. So the pass reported 'nothing to clean up' on the commonest registration shape -- and then latched its sentinel, so it never looked again. Registration now carries the project it lives under, both scopes are scanned in one pass, and remove_registration resolves the right container (finding it without carrying the container would find it and be unable to remove it). 2. The backup was clobbered. The stamp is second-resolution, so two registrations removed from one file in one run resolved to the same backup name and the second wrote already-edited content over it. The pre-edit original was then gone while both messages still named that path as the undo. Takes the first free name instead. 3. candidate_config_paths returned ~/.mcp.json and cwd/.mcp.json with no dedup, so with cwd == HOME the same file was scanned twice -- duplicating every note and making a successful removal report failure on the second pass. 4. The sentinel latched even when a config could not be read. A scan that could not read its input has not established that there is nothing to clean up, so it suppressed the one-shot report on exactly the machines that still needed it. The docstring already promised otherwise. 5. _basename did not strip a Windows .exe/.cmd/.bat suffix, so aelf.exe was unrecognised -- and with the map key named aelfrice the routine printed that aelfrice did not publish a command it did publish. The verb is still required, so the strip does not widen the match. 6. mcp_extra_is_installed read only the extras list, missing 'uv tool install --with fastmcp aelfrice', which uv records as a sibling requirement. The CHANGELOG promises that population is reported. Full suite 7314 passed.
`receipt.get("tool", {}).get(...)` calls `.get` on whatever the receipt
holds, so a hand-edited `tool = "aelfrice"` raises AttributeError instead
of the False the docstring promises. `aelf setup`'s broad handler hides
it; a direct `maybe_clean_up_mcp` call does not.
The success line hardcoded `mcpServers.<key>`, so removing a locally-scoped entry reported `mcpServers.aelfrice` for something that actually lives at `projects.<dir>.mcpServers.aelfrice`. The message is the undo instruction, so it pointed at a key the file does not contain. `location()` already spells this and every other message in the module uses it.
`find_registrations` returns notes without registrations when a config is unreadable, so `--remove-mcp-config` printed the note, then "no aelfrice MCP registration found", and exited 0. The user asked for an edit and one input was never inspected; "nothing to remove" and "could not look" have different fixes. `maybe_clean_up_mcp` already made this distinction — `scan_was_incomplete` shares it so the two cannot drift.
…sh insert The round-trip test asserted nothing it claimed to. `beliefs` has no `source_kind` column — the value lives in `ingest_log` — so inserting a derived belief and reading it back passes for any `source_kind` whatsoever. Replaced with a row seeded by raw SQL (going through `record_ingest` would validate against the frozenset and pass by construction on exactly the change this guards), in a file-backed store that is closed and reopened, because opening runs DDL and the migration sweep — where an unrecognised value would actually be rejected or rewritten. Also corrects the module docstring. Removing the constants is *not* the #1161 unopenable-store class: no source column carries a SQL CHECK and the row decoder does not validate. The two reachable costs are that `record_ingest` would reject the value, breaking replay of historical rows including the soak corpus, and that `retention_class_for_source` would silently fall through its default and reclassify the row `unknown` — now asserted, since nothing raises.
…ouched question.yml pointed at docs/COMMANDS.md, which does not exist — the file is docs/user/COMMANDS.md. federation.py said `belief_id`/`owning_scope` reach callers through "CLI exit codes"; the exit code is 1, the message carries them. wonder/dispatch.py documented only `--axes` and in the wrong order — the flag takes the query as its value, and `aelf wonder QUERY` (#645) is the primary form with `--axes QUERY` (#551) retained as an alias. All three were pre-existing or under-stated text on lines this PR already edited to strike the MCP surface.
robotrocketscience
force-pushed
the
refactor/issue-1422-remove-mcp-surface
branch
from
August 9, 2026 03:55
26cb8e1 to
929c2fa
Compare
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: tests/test_mcp_cleanup_1422.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
CodeQL alert 561: the control block bound `result` a second time and never read it. Asserting `ran` on it makes the binding load-bearing — the control now shows the pass actually executed, not merely that the sentinel exists.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged b962eeb → |
Closed
14 tasks
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.
Closes #1422.
Removes the MCP surface and cleans up after it on upgrade. Operator-ratified
2026-08-06, twice: the removal itself, then the amended shape of the cleanup once
discovery showed the auto-edit half rested on paths this repo has never named.
Why remove rather than repair
serve()failed on every version across its declared rangefastmcp>=0.2.0, each differently:TypeError: FastMCP.tool() got an unexpected keyword argument 'annotations'TypeError: cannot specify both default and default_factoryNameError: name 'Field' is not definedNot an upstream regression — there is no version on which it starts. The 3.x
failure has the clean mechanism:
from pydantic import Fieldis a local ofserve(), whilefrom __future__ import annotationsmakes fastmcp resolve toolannotations against module globals, which never contained it.
Broken at
d6bafcb9(2026-05-08), shipped in v2.0.1 and every release since.Five commits touched the module afterwards, one adding a feature by mirroring a
lock window onto it.
ci.ymlinstalls--extra archiveonly and no workflow everinstalled
mcp, so the two tests reaching the code assert the fastmcp-absentbranch and read the ambient environment — green in CI precisely because the
feature was not installed.
test_mcp_wrapper_layer.pydid exercise registration,but through a hand-written shim that never resolves annotations, which is the step
that fails.
Beyond the breakage: tool invocation is at the model's discretion, so MCP cannot
put the right beliefs in the prompt before the model reads the message. That is
the guarantee aelfrice sells. Same reasoning as #605, and borne out by the Codex
host — built on hooks plus a skills bundle, no MCP reference at all.
What this is not
Not a store migration.
mcp_rememberstays a first-classsource_kind, withits frozenset membership intact. Existing stores hold rows carrying it, and a
reader that rejects a value it wrote last release makes the store unopenable —
#1161's class, unrecoverable in the field. Production stops writing it; readers
keep accepting it.
tests/test_mcp_remember_reader_contract_1422.pypins that(mutation-checked: dropping it from either frozenset, or renaming the wire string,
each turn it red), and the replay-soak corpus covers it end to end.
Not a
aelf confirmchange. It reachedapply_feedbackthrough the MCPmodule; it now calls it directly.
respect_lock=False— the #1168 lock-floorexemption — is carried across and commented as load-bearing. It had no test:
every existing confirm test used unlocked beliefs, where the exemption is
invisible. The first commit adds one, before the call site moves.
The seven ported test files
Not a mechanical import swap —
mcp_serverheld real logic beside the plumbing,so each file was triaged assertion by assertion.
Behaviour ported rather than dropped:
test_ingest_log— the post-[v2.x] Derivation worker — beliefs become materialized state #264 re-lock contract (a second lock appends alog row and corroborates rather than duplicating) was asserted only through
tool_lock; the neighbouring CLI test stops at a single lock. Now CLI-driven.test_lock_management— the [v2.0 / Track E]unlock/promote/demotelock-management surface (CLI + MCP) #391 demote lock-drop audit row, and thelock → unlock → re-lock round trip. Both drive
aelf lock/aelf demoterather than seeding a locked row, because what they exercise is the ingest path
that makes "re-lock" mean corroborate.
test_session_id_population_rate— the fourtool_lockcalls arereplaced, not deleted: deleting them drops the measured rate to 75% against
its own
>= 0.80gate. Re-measured at 10/12 = 83.33%, composition still 12.Wire-format assertions removed only after confirming an equivalent exists
(
tool_statsphantom payloads → core + CLI tests;to_scopeshapes →test_to_scope's CLI block, not-found →test_cli.py:191;tool_confirmreturn dicts →
test_cli_confirm).Part 2 — the upgrade cleans up after itself
A one-shot pass behind
~/.aelfrice/mcp-surface-removed, fired from_cmd_setup_lockedbeside the #733 and #1064 migrations.It reports; it does not edit.
git grep mcpServerson main returns two hits,both lines inside the doc that told users to paste one — no path constant for
those files exists in the tree, and the file usually holds the user's other
servers.
lifecycle.py:791-797already states the rule: anything not named in thedotdir contract is reported and never deleted.
aelf migrate --remove-mcp-configis the opt-in verb, behind a timestamped backup.
It advises the reinstall.
maybe_migrate_to_uvmay shell out preciselybecause it never runs on a uv-tool install; this targets the opposite population,
so the extra is detected by reading uv's own receipt with stdlib
tomlliband thecommand is printed rather than run.
The predicate covers all four shapes ever published —
aelf mcp,uv run --project <abs> aelf mcp,aelf serve(docs/INSTALL.md @99160871) andpython -m aelfrice.mcp_server— plus the documented-but-never-shippedaelf-mcp. Never matches on the map key (users rename it) or onaelfalone.Sentinel registered in both conftest lists.
_PRECREATED_SENTINELSis the onethat matters: an
exists()-guarded sentinel pinned at a fresh tmp path arms theguarded side effect instead of disarming it, and CI cannot catch that because a
runner's HOME is empty. Also added to
_DOTDIR_INSTALL_STATEand the uninstallagreement test.
Known reach limit
Both existing one-shot migrations fire only from
_cmd_setup_locked. A user whoruns
uv tool upgrade aelfriceand never runsaelf setupwill not see this.Firing from
auto_install_at_cli_entryis the #1332 re-arm channel and is out ofbounds, so the CHANGELOG says when the cleanup runs rather than claiming the
upgrade cleans up unconditionally.
Deliberately untouched
docs/audits/*anddocs/design/*are dated records; ROADMAP's shipped-versionrows and PHILOSOPHY's "15 MCP tools at v3.3" are release history;
store.py's#1161 narrative describes what used to break. Rewriting any would falsify the
record.
slash_commands/onboard.mdis left byte-identical — its MCP mention isnot a claim that we ship one, and rewriting the line would re-add a token the
discretion gate blocks on additions.
Verification
github/mainuv run --extra archive pytest -q -p no:randomly: 7316 passed, 70 skipped,71 xfailed, 0 failed
uv sync --frozen --group dev --extra archive(CI's exact invocation) succeedsagainst the regenerated lockfile
mcp_rememberreader contract (3), and the cleanup's safety properties (5 —widened predicate, key-only match, skipped backup, auto-edit, swallowed parse
errors). All killed.
Summary by Sourcery
Remove the broken MCP server surface and associated CLI entrypoint while preserving backwards compatibility of stored data, and add an opt-in, one-shot cleanup path for leftover MCP configuration and extras.
Enhancements:
aelf confirmdirectly throughapply_feedbackwhile keeping the lock-floor exemption and tightening tests around confirm behavior, including for locked beliefs.[mcp]uv extras, reporting them onaelf setupand allowing optional removal viaaelf migrate --remove-mcp-config.Build:
[mcp]optional dependency and its fastmcp/pydantic requirements frompyproject.toml, and update the lockfile accordingly.Documentation:
Tests:
Summary by CodeRabbit
Removed
New Features
aelf confirmnow updates locked beliefs while preserving their locks and supports optional notes.Documentation
mcp_rememberdata.