feat(mcp): raise the scan result cap and bound open sessions - #96
Merged
Conversation
github-actions
Bot
added
docs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
=======================================
Coverage 89.89% 89.89%
=======================================
Files 41 41
Lines 3632 3682 +50
=======================================
+ Hits 3265 3310 +45
- Misses 367 372 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are well-scoped with targeted tests, and the remaining feedback is limited to minor naming/documentation maintainability nits.
Pull request overview
This PR adjusts MCP server resource limits to improve scan correctness and prevent unbounded OS-handle growth by capping concurrently open sessions, while also advertising the new limits via server_info.
Changes:
- Increased the default per-scan result cap to
100_000(DEFAULT_MAX_SCAN_RESULTS) to reduce falsepartialresults for mid-sized hit counts. - Introduced
MAX_OPEN_SESSIONS(8) enforced bySessionStore.open, plus safe handle cleanup on refused attaches. - Added/updated tests and docs to cover the new session cap behavior and updated defaults.
File summaries
| File | Description |
|---|---|
| tests/mcp_server/test_toolset.py | Adds a regression test ensuring refused attaches don’t leak a process handle. |
| tests/mcp_server/test_session.py | Adds coverage for enforcing an open-session cap and improves batching test commentary. |
| PyMemoryEditor/mcp/toolset.py | Exposes new limits via server_info and ensures attach closes handles on capped sessions. |
| PyMemoryEditor/mcp/session.py | Introduces MAX_OPEN_SESSIONS and enforces it in SessionStore.open with a recovery-oriented error message. |
| PyMemoryEditor/mcp/config.py | Raises the default maximum scan results to 100,000 with rationale in comments. |
| docs/mcp.md | Updates documented defaults and describes the new open-session limit behavior. |
Review details
Suppressed comments (2)
tests/mcp_server/test_toolset.py:1224
- The variable name
abertosis non-English and makes the intent of the list less clear in an otherwise English test suite; using an English, descriptive name improves readability.
abertos = []
def fake_open(**_kwargs):
process = FakeProcess()
abertos.append(process)
tests/mcp_server/test_session.py:250
- The local variable names
primeiro/segundoare non-English; using English names keeps naming consistent across the test suite.
primeiro = store.open(FakeProcess(), 1, "a").session_id
store.close(primeiro)
segundo = store.open(FakeProcess(), 2, "b").session_id
assert primeiro != segundo
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
JeanExtreme002
added a commit
that referenced
this pull request
Sep 8, 2026
Three findings, all correct. **Non-English identifiers.** `reaberto`, `primeiro`, `segundo`, `abertos` — Portuguese names in an all-English codebase, from writing these tests while conversing in Portuguese. Renamed. **A fixture requested and never used.** `test_a_refused_attach_does_not_leak_ the_handle` took `fake_process` and built its own `FakeProcess` instances inside the patched opener, so the parameter did nothing — and `make_toolset` already depends on that fixture, making it redundant twice over. Dropped. (The report also pointed at `test_a_chain_resolving_past_64_bits_is_refused_ not_returned` for the same thing, but that one does use it — `fake_process.resolve_pointer_chain = ...`. Only the one is unused.) **A hardcoded limit in the docs.** `docs/mcp.md` spells out "Eight targets can be open at once", which a constant change would silently invalidate. Keeping the number rather than pointing the reader at `server_info` — a doc that tells you to call an API to learn a limit is worse for a human — and pinning it instead, the same way `test_advertised_widths_are_exactly_the_accepted_widths` pins the widths. `DEFAULT_MAX_SCAN_RESULTS` gets the same guard, since this PR just changed it. Verified by mutating both constants: each fails its own assertion.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral changes are small, well-contained, and covered by targeted tests; only minor message/test robustness nits were identified.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
PyMemoryEditor/mcp/session.py:197
- The refusal message says "attach again", but the public MCP entrypoint/tool is
open_process; using the tool name makes recovery steps unambiguous for clients/models.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Measured rather than guessed. Per result set the cap costs 4.2 MB against 2.1 MB, and the sort at the end of `_run_batched_scan` goes from 10 ms to 21 ms. Wall clock does not move: reaching either ceiling on a real process took 0.02s against 0.03s, because a value dense enough to hit it yields hits far faster than the 30-second budget can spend — the clock binds, not this. Worst case per session is 20 result sets, so 43 MB against 84 MB. The gain is narrow but it is correctness, not convenience: a scan whose true hit count falls between the two ceilings used to come back flagged `partial`, and refining a truncated set can converge on an address that was never in it. Above the new ceiling nothing changes — a scan for `int 0` matches millions and is unreachable at any sane cap.
`SessionStore` accepted any number of sessions — six opens of the same pid gave six sessions, each holding its own handle. Nothing obliges a model to call `close_process`, and a session that is merely forgotten keeps its handle until the server exits, so the store leaked the one resource garbage collection cannot reclaim: a debug handle on Windows, a task port on macOS. Capped at 8, which is far past real use. It refuses rather than evicting, unlike the per-session scan cap: dropping an old result set is safe because a refine loop never looks back, but closing a handle out from under the model is not, and it has no way to detect that. The message names a session to close and lists what is open, since the model cannot see the store. The naive version of this would have caused the problem it prevents. `attach` opens the handle and *then* registers the session, so a store that refuses leaves the handle with nobody holding it — every rejected attach would cost one more handle than having no cap at all. `attach` now closes it before re-raising, and there is a test that fails without it. `server_info` advertises `max_open_sessions` and `max_scans_per_session`, by the principle already stated next to `max_text_bytes`: a limit this project invented should not have to be discovered by tripping the error. Verified by reverting each half: the cap (3 failures), the close (1).
Three findings, all correct. **Non-English identifiers.** `reaberto`, `primeiro`, `segundo`, `abertos` — Portuguese names in an all-English codebase, from writing these tests while conversing in Portuguese. Renamed. **A fixture requested and never used.** `test_a_refused_attach_does_not_leak_ the_handle` took `fake_process` and built its own `FakeProcess` instances inside the patched opener, so the parameter did nothing — and `make_toolset` already depends on that fixture, making it redundant twice over. Dropped. (The report also pointed at `test_a_chain_resolving_past_64_bits_is_refused_ not_returned` for the same thing, but that one does use it — `fake_process.resolve_pointer_chain = ...`. Only the one is unused.) **A hardcoded limit in the docs.** `docs/mcp.md` spells out "Eight targets can be open at once", which a constant change would silently invalidate. Keeping the number rather than pointing the reader at `server_info` — a doc that tells you to call an API to learn a limit is worse for a human — and pinning it instead, the same way `test_advertised_widths_are_exactly_the_accepted_widths` pins the widths. `DEFAULT_MAX_SCAN_RESULTS` gets the same guard, since this PR just changed it. Verified by mutating both constants: each fails its own assertion.
Two review findings, both correct. **The cap was enforced after the prompt.** `SessionStore.open` is the guard, and it runs once `attach` has already opened the handle — which is after `_register_open_process` has elicited the user's approval. So the flow was: ask "Allow PyMemoryEditor to attach to game.exe?", user approves, then the call fails because the server is full. An approval is the most expensive step in this server and the one thing it must not spend for nothing. `SessionStore.at_capacity()` is now consulted before the prompt. `open` stays the authority — the two can disagree under a concurrent open, which is why the deep guard remains — but the common case fails fast, and the message names the target so a model does not conclude the pid was wrong. **`WORDS[MAX_OPEN_SESSIONS]` raised KeyError above 10.** Reported by Copilot. The test exists to turn a silent doc/code divergence into an actionable message, and bumping the constant to 12 replaced that with `KeyError: 12` — loud, but saying nothing about what to do. Looked up with `.get` now, with the missing spelling as its own assertion. All three cases verified: 8 passes, 6 says the doc disagrees, 12 says to add the word. Verified by reverting the early check: the prompt happens again and `test_a_full_server_refuses_before_spending_an_approval` fails.
JeanExtreme002
force-pushed
the
feat/mcp-scan-caps
branch
from
September 8, 2026 22:10
79b4dae to
d8ae874
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new pre-approval “server full” refusal message should avoid empty target names and provide actionable recovery details (which session_id to close) for the model.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
The cap I added in this PR created a way to disable the feature it protects,
and a review caught it.
Nothing here notices a target exiting. `_rows_for` swallows the read error,
`process_info` keeps answering from cached state, and the session stays in the
store — a session only leaves via `close_process` or shutdown. Before the cap
that was clutter. With a cap that refuses rather than evicts, eight exited
processes hold every slot for the life of the server, every `open_process` is
refused, and the model cannot even find out which sessions are the dead ones.
Reproduced end to end: eight dead targets, ninth attach refused,
`process_info("proc-1")` answering as though alive.
`SessionStore._reap_dead()` now drops sessions whose pid is gone, called from
`open` before it refuses and from `at_capacity` before it reports full.
Evicting a *dead* session is the exception that proves the rule about not
evicting: its handle is already useless, so nothing is taken away. A full
server of live targets still refuses, asserted so the reaper cannot become a
back door around the cap.
`server_info().open_sessions` gains `alive`, so the model can see them before
the store needs the room.
**The reaper's first version had a bug of its own,** found by the argument
fuzzer going intermittently red: `pid_exists(2**31)` raises `OverflowError` —
the value does not fit the platform's pid type — and the reaper runs inside
`open`, so it escaped as something other than a `SessionError` and the model
would have seen only "Error executing tool". `_looks_alive` treats a pid the
OS refuses as dead, which is correct as well as safe: a number the OS will not
accept cannot name a running process. Five clean runs of the suite after,
against one failure in the run before.
**The refusal message no longer says "close the oldest".** The oldest is
usually the target the whole session has been refining, and `close_process`
discards its scan results too, so following the message literally destroyed
the most valuable session. It now lists every session with its scan count, and
names the session already holding the requested pid when there is one — a
model that lost count was being told the server was full rather than that the
target was already open.
**The cap's justification was also wrong**, and that is fixed in the comment
rather than papered over. It priced one result set and the per-session total
(84 MB) while this same PR added the eight-session cap, so the real ceiling on
retained addresses goes from ~336 MB to ~672 MB. And "wall clock is unchanged"
was true of the one value I measured (`int 0`, dense enough that hits arrive
faster than the clock can spend) and not of the change: for a sparser value,
collecting twice as many hits means walking further.
Also drops the stale `50 000` from `_read_addresses`'s docstring, which is
model-facing prompt text this PR had just doubled.
Tests pinned liveness with a fixture, because a fake's pid is an arbitrary
number: several of the cap tests had been passing only because `pid_exists(1)`
is true on a Unix host.
A fifth Copilot finding, on the early refusal I added two commits ago. Both halves correct. **It could name an empty target.** `_name_for_pid` returns `""` for a pid missing from the process listing — verified — and the elicitation prompt directly below already guards with `or "unknown"` for exactly that. Mine did not, so it rendered `attach to "" (pid 4242)`. **And it was the less useful of two messages.** `SessionStore.open` lists every open session with its scan count; my early version said only "close one with close_process first". The early path is the one a model actually reaches, so the duplication had made the common case worse. Fixed by removing the duplication rather than copying the text: `SessionStore.capacity_refusal(pid)` renders the message once, returns `None` when there is room, and both callers use it — `open` raises it as the authoritative guard, the protocol layer raises it before prompting. `at_capacity()` is now a thin wrapper over it. Verified by reverting the `or "unknown"`: the new test fails.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new capacity refusal/error-reporting paths include at least one robustness/usability issue that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
PyMemoryEditor/mcp/server.py:251
- The early refusal path (
at_capacity()) tells the caller to useclose_process, but doesn’t tell them how to find asession_idto close; without that, the instruction is not actionable. Include a pointer toserver_info(or include the open-session list) so the model can recover in one step.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
…again Two defects, one reported and one found reviewing. **`server_info` could crash on an impossible pid.** Reported by Copilot. It rendered `alive` with a bare `pid_exists`, which raises `OverflowError` for a pid outside the platform's range — the same call I had just guarded in the store, written unguarded one file over. Worse there than in the reaper: `server_info` is the tool the instructions tell the model to call first, so a single odd session hid the limits and the policy too, not just the session list. `looks_alive` is now shared rather than duplicated, and the only remaining raw `pid_exists` is inside it. This also explains an instrumentation result that had looked wrong: patching `session.pid_exists` counted zero calls from `server_info`, because `toolset.py` imported the function separately. Two modules, two imports, one guarded. **And the cap stopped being atomic.** Found reviewing my own last commit: the `capacity_refusal` refactor split the check and the insert across two acquisitions of the store's lock, so every racing thread saw room and then every thread inserted. Demonstrated at 12 sessions against a cap of 8 by widening the window; it does not reproduce on its own, which is why the test that guards it injects the same pause — the plain concurrency test passes on the broken implementation, verified by mutation. `_refusal_locked` renders the message with the lock already held, so `open` checks and inserts under one acquisition and reaps outside it. Also drops `SessionStore.is_alive`, which I added in the previous commit and never called.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is a concurrency edge case in SessionStore.open() where dead sessions may not be reaped before refusing, and one concurrency test currently patches a method open() no longer calls, making the intended race-window amplification ineffective.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
CI caught this on Windows alone: `pid_exists(1)` is False there — Windows has no pid 1 (System Idle is 0, System is 4) — so `test_server_info_survives_a_session_with_an_impossible_pid` failed on `assert by_pid[1] is True`. `os.getpid()` is alive by definition on every platform. The commit two before this one said, in its own message, that several cap tests "had been passing only because `pid_exists(1)` is true on a Unix host". I then used pid 1 as the live case in a new test in the same PR. Fourth Windows-only assumption here after the 32 767-character environment variable, `monotonic()`'s ~15.6 ms tick, and the path separator. Verified on Linux in a container as well as locally: 724 passed on both.
Two more Copilot findings on the previous commit, both correct. **`open` could refuse without reaping.** It decided whether to reap from a check taken under an earlier acquisition of the lock, so if the store filled between the two it refused with a slot held by an exited process that nobody had looked at. It now loops: judge under the lock, insert if there is room, otherwise reap once and judge again. At most one reap — a second pass finds nothing new, and refusing has to terminate. **And the test guarding the atomicity fix was guarding nothing.** It patched `capacity_refusal` and slept in it to widen the race, but `open` calls `_refusal_locked` — verified by making `capacity_refusal` raise and watching `open` succeed. The sleep never fired. It passed its mutation check only because that mutant happened to reintroduce the `capacity_refusal` call, so the test was validating one historical edit rather than the property. Replaced with two that hold: * `_refusal_locked` asserts the lock is held, so a naked call fails deterministically instead of when the scheduler cooperates. * Acquisitions are counted across one `open`: one for the correct implementation, two when the check releases the lock before inserting. The assertion alone was not enough, and I checked rather than assumed — splitting check from insert still passed it, because each half held the lock on its own. The counting test is what fails on that. 726 tests, green on Linux in a container as well.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
SessionStore.open() can raise a refusal based on a stale capacity check under concurrency (check under lock, raise after unlock), which can incorrectly reject an open even after a concurrent close() frees a slot.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot again, on the loop from the previous commit, and right. `open` computed the refusal under the lock and raised it after the block, so another thread could `close()` a session in that gap and this caller would be turned away with a message that was already false. The cap was never violated — this is a spurious refusal, not a breach — but the fix is moving two lines up into the `with`, so there is no reason to keep the window. Tested by recording whether the lock is held when the error is constructed, which is the property rather than a timing guess. Reverting the move fails it.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The limit changes are implemented with clear failure semantics, concurrency-safe enforcement, and strong targeted test coverage for the key regressions (handle leaks, refusal messaging, and race conditions).
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to the MCP server's limits, one asked for and one the measurement turned up.
max_scan_results: 50 000 → 100 000Measured rather than estimated:
sort()at the end of_run_batched_scanTime is a non-issue: a value dense enough to hit the ceiling yields hits far
faster than the 30-second budget can spend, so the clock binds, not the cap.
The gain is narrow but it is correctness rather than convenience. A scan whose
true hit count falls between the two ceilings used to come back flagged
partial, and refining a truncated set can converge on an address that wasnever in it. Above the new ceiling nothing changes —
int 0matches millionsand is out of reach at any sane cap.
MAX_OPEN_SESSIONS: new, 8The measurement above is per session, which raised the question of how many
sessions there can be. The answer was: any number. Six opens of the same pid
gave six sessions, each with its own handle.
Nothing obliges a model to call
close_process, and a forgotten session keepsits handle until the server exits — a debug handle on Windows, a task port on
macOS. That is the one resource garbage collection cannot reclaim, so the
store is now bounded.
It refuses rather than evicting, unlike the per-session scan cap. Dropping
an old result set is safe because a refine loop never looks back; closing a
handle out from under the model is not, and it has no way to detect that. The
refusal names a session to close and lists what is open, since the model
cannot see the store.
The naive version would have caused the problem it prevents
attachopens the handle and then registers the session. A store thatrefuses at that point leaves the handle with nobody holding it, so every
rejected attach would have cost one more handle than having no cap at all.
attachnow closes it before re-raising, with a test that fails without it.Also
server_infoadvertisesmax_open_sessionsandmax_scans_per_session, bythe principle already stated next to
max_text_bytes: a limit this projectinvented should not have to be discovered by tripping the error.
Verification
Each guard mutation-tested by reverting it: the session cap (3 failures), the
handle close (1). 707 MCP tests, 91.4% coverage; 617 library tests, 87.2%;
mypyandflake8clean.