fix(mcp,contract): scope lease_status to this session; restore the §1 boundary test - #103
Open
V3RON wants to merge 5 commits into
Open
fix(mcp,contract): scope lease_status to this session; restore the §1 boundary test#103V3RON wants to merge 5 commits into
V3RON wants to merge 5 commits into
Conversation
lease.list filters by owner principal only, with no mode or connection filter, so it could return a held/detached lease this MCP connection never requested (e.g. the CLI's --detach lease under the same SIMLOCK_AGENT_ID principal). lease_status took leases[0] blindly, reporting a foreign lease as held and never releasing it on close. Track the id of the lease this session's own lease() call obtained (not a cache of its state -- always re-read via lease.list) and filter status() to mode === "held" && id === that tracked id. Correct the false doc-comment claiming leases holds at most one entry.
The stack collapsed three JSON code examples onto single lines, which
oxfmt --check rejects. Re-run oxfmt to restore the multi-line
formatting CI enforces, and correct the "progress" push example: the
CLI emits {push:"progress", ...progress} flattened (no requestId, no
nested "progress" key), not the wrapped shape the example showed.
parseRawLeaseLost and parseRawLeaseProgress (and their RawLeaseLost/ RawLeaseProgress types) are leftovers of the hand-written push parsers ADR 0003 §11 replaced with the typed contract client -- fallow flagged both as unused exports, and neither is referenced anywhere in the repo, including this file's own test.
The test kept only lines matching ^\s*import\s and extracted "from"
from that same line. The project's formatter breaks any import with
more than one named binding across multiple lines, so for such an
import the "import {" line has no "from" and the "} from '...'" line
is never in scope at all -- the one mechanism enforcing ADR 0003 §1's
"the contract imports nothing from core/daemon/drivers" was blind to
the import style this repo actually writes.
Strip comments and scan the whole file text for from "..." specifiers
instead, the way src/simlock-client/no-core-leak.test.ts already does.
Verified by temporarily adding a multi-line `import { ... } from
"../core/index.js"` to schemas.ts: the old logic missed it, the new
logic fails on it.
The typed client in src/simlock-client supersedes it entirely; nothing outside the package imported it any more, so ADR 0003 §11's "raw parsers and hand-built payloads deleted" is only true once these six modules go with them. Also drops a fallow suppression that went stale when queueDepth moved behind the dispatcher, and repoints the comments that named the deleted module.
V3RON
force-pushed
the
fix/0003-10-mcp-hygiene
branch
from
September 3, 2026 17:23
04d588f to
d6f0815
Compare
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
1 participant
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.
Stacked on #102. Three blocking defects from the adversarial review, plus the dead-code cleanup ADR §11 implies.
lease_statusreported a lease the session does not holdThe handler's doc-comment claimed
lease.listreturns at most one entry because "this session only ever requestsmode: "held"leases under one requester id". That is not whatlease.listdoes — the dispatcher filters by owner principal only, with no mode or connection filter — and the handler tookleases[0]blindly.With
SIMLOCK_AGENT_ID=agent-7, the CLI and MCP share a principal. An agent runningsimlock lease --detachin a shell and then calling MCPlease_statusgot{held: true, mode: "detached"}for a lease its connection never requested and will not release on close. The cache-based implementation this stack deleted got this right.lease_statusstill callslease.listunconditionally — no cache of lease state returns, which is what §11 removed — but the session now tracks only the id of the lease its ownlease()call obtained, cleared on release and on thelease-lostpush, and filters tomode === "held"and that id. The false doc-comments are corrected.The boundary test enforcing §1 was blind to multi-line imports
boundary.test.tskept only lines matching^\s*import\sand looked forfrom "…"on that same line. The repo's own formatter breaks any import with more than one named binding across lines — so for exactly the style this project writes, theimport {line has nofromand the} from "../core/index.js";line was never examined at all.The single mechanism enforcing ADR §1's central constraint — "the contract imports nothing from
core,daemon, ordrivers… this is what keeps private types out of the public package surface" — would have passed a multi-lineimport { DeviceRecord } from "../core/index.js";inschemas.ts.Now it strips comments and scans the whole file, matching the approach
no-core-leak.test.tsalready used correctly. The fix was demonstrated: a multi-line core import was temporarily added toschemas.ts, the suite failed as it should, and the import was reverted.Gates that were failing, both clean on
mainREADME.mdhad three JSON examples collapsed onto single lines, failingformat:check. Reformatted — and while verifying them against the current schemas, theprogresspush example turned out to be wrong: the CLI emits it flattened, with no nestedprogresskey. Fixed.fallowreported dead exports left over from the hand-written parsers §11 says are deleted.The
daemon-clientpackage is deletedChasing those dead exports showed the whole of
src/daemon-client/was referenced only from comments —src/simlock-client/supersedes it entirely. Six modules and their tests are removed, so §11's "raw parsers and hand-built payloads deleted" is finally true rather than approximately true. A fallow suppression that went stale whenqueueDepthmoved behind the dispatcher goes with them, and the comments naming the deleted module are repointed.After this the
fallowgate reports zero dead code and one fewer duplication group.