MCP: coding_repos_list returns mismatched instanceId, plus intermittent tool-registration/schema loop #692

Activity

1. proagentstore

   added

   bugSomething isn't workingSomething isn't working

   on Aug 16, 2026
2. proagentstore commented on Aug 16, 2026

   

   proagentstoreBot

   on Aug 16, 2026 – with ProAgentStore

   Author

   More actions

   3. No MCP tool to remove a repo from a coding instance

   A third gap found in the same session:

   The MCP surface exposes coding_repo_add and coding_repos_list but no coding_repo_remove / coding_repo_detach. There is a remove_repo tool, but it applies to repo-chat vector ingestion, not coding-instance bindings — so it doesn't help here.

   Consequence: a repo attached in error can only be cleaned up in the console. The same automation that created a binding cannot undo it. Concrete cases motivating this:

   • Instance 55f5f034-3243-4203-bd06-0e5019922afe ("FAGS Platform coder") carries a stray GitHub binding to proagentstore-online/platform, an org the user does not own. The correct repo FreeAgentStore/platform has since been attached alongside it, but the wrong binding cannot be removed via MCP.

   • Instance 3cbb1d1d-5e6d-418f-8917-19e955707468 ("Parents Clubs coder") holds two local bindings with absolute /Users/serge-ivo/... workdirs that no longer exist on disk (cloneStatus: needs_attention), plus an obsolete apps/school-clubs entry — all three are stuck and unremovable via MCP.

   Requested tool: coding_repo_remove (or coding_repo_detach) accepting instance_id plus repo id or name.

   ---

   Addendum on fault #2 (tool-registration/schema loop): the loop is reliably worked around by retrying the identical call unchanged. This points to a registration/caching race rather than a validation problem — the retry succeeds without any change to parameters or schema.
3. serge-ivo

   added

   P2: correctnessReal defect, no live harm today — inert fields, miscounts, missing guardsReal defect, no live harm today — inert fields, miscounts, missing guards

   backendBackend / Worker / API workBackend / Worker / API work

   mcpMCP server surface — the operator's instrument panel, not the productMCP server surface — the operator's instrument panel, not the product

   coderThe Coder wedge agent (#68) — Engine, Pilot, Co-pilot, Loop, OverseerThe Coder wedge agent (#68) — Engine, Pilot, Co-pilot, Loop, Overseer

   on Aug 18, 2026
4. serge-ivo

   added a commit that references this issue on Aug 29, 2026

   fix(mcp): guard coding_repos_list against cross-instance contamination (

   ...

   759978f
5. serge-ivo commented on Aug 29, 2026

   

   serge-ivo

   on Aug 29, 2026

   Contributor

   More actions

   Resolution

   Part 1 fixed — commit 759978f.

   What was wrong

   coding_repos_list made a correctly-scoped API call but had no defence against the MCP transport layer mis-routing a concurrent response. When five concurrent calls each asked for a different instance, the transport could deliver response B to the caller who asked for instance A — so the returned repos would carry instanceId: B against a request for instance_id: A. The tool had no way to detect or reject that.

   Additionally, API errors (404 for stale/wrong instance ids) were silently collapsed to {repos:[]} rather than surfaced to the caller.

   What the fix does

   1. Extracted a pure function filterReposByInstance(repos, instanceId) that drops any repo whose instanceId field does not match the requested instance. In the normal single-request path every repo already carries the correct instanceId, so the filter is a no-op. Under concurrent mis-routing it returns empty rather than leaking the wrong instance's repos.

   2. Added an explicit r.error check before consuming the repos, so API errors surface as the actual message rather than as empty repos.

   3. Added instanceId?: string to the CodingRepoRow interface (the field was returned by the API but not typed).

   4. Five unit tests pin the filter contract: matching pass-through, mismatching filtered out, mixed-response partial filter, no-instanceId defensive pass-through, and empty input.

   Part 2 — tool-registration / schema loop

   This is the same root cause as #759 ("has not been loaded yet, retry with exact parameter names"). That ticket is the canonical one and carries more context. Leaving this issue open until #759 is resolved (or until it's confirmed #759 covers the full symptom set). The retry-succeeds characterisation, the self-contradicting error message, and the occurrence on first touch of any tool in a session are all documented there.
6. serge-ivo

   added a commit that references this issue on Sep 10, 2026

   feat(mcp): coding_repo_remove — the counterpart coding_repo_add never…

   ...

   4fa0914
7. serge-ivo commented on Sep 10, 2026

   

   serge-ivo

   on Sep 10, 2026

   Contributor

   More actions

   Status: fault 1 verified fixed, the missing removal tool shipped, fault 2 still external

   Two of the three items on this ticket are now closed out. The third is unchanged and is the reason this cannot close yet.

   1. Mismatched instanceId — fixed in 759978fe, re-verified at HEAD

   Worth stating the mechanism precisely, because it is not what it looks like: the API was never wrong. Its query is scoped by instanceId AND userId, and sequential calls were always self-consistent — as this issue's own repro notes. The corruption was in the MCP transport layer delivering response B to the caller who asked for instance A. The tool had no way to detect that, so it reported the wrong instance's repos as the answer to a correctly-scoped question.

   filterReposByInstance did not exist before that commit — it is the fix, not a pre-existing function that was misbehaving. It drops any repo whose instanceId does not match the one requested:

   export function filterReposByInstance(repos: CodingRepoRow[], instanceId: string): CodingRepoRow[] {
   	return repos.filter((r) => !r.instanceId || r.instanceId === instanceId);
   }

   On the normal single-request path every repo already carries the right instanceId, so it is a no-op; under mis-routing it returns empty rather than leaking another instance's repos. The same commit also stopped API errors being collapsed to {repos:[]} (a 404 from a stale instance id read as "no repos") and added instanceId?: string to CodingRepoRow, which the API returned but nothing typed.

   Re-verified at 4fa09145: 20 tests green in coding-tools.test.ts, 5 of them pinning the filter contract — matching pass-through, mismatching filtered out, mixed-response partial filter, no-instanceId defensive pass-through, empty input.

   2. coding_repo_remove — shipped in 4fa09145

   The gap raised in the comment above, and the one thing here that needed new code.

   The endpoint already existed. DELETE /v1/instances/:id/coding/repos/:repoId has been in routes/coding-repos.ts all along; its only caller was agents/coder/web. That is also why no guard ever caught the gap — check-mcp-parity.mjs inventories store/console/src, so a capability reachable only from the Coder UI is invisible to it.

   It is not a bare row delete: it asks the runner to end any ACTIVE session on the repo first, because removing the binding is the last handle on that engine process. If a runner does not confirm, the removal still completes (a flaky runner must not trap an owner with a repo they cannot remove) and the reply carries a warning naming how many engines may still be running. That warning is passed through verbatim — swallowing it would recreate the orphaned-process problem one layer up.

   • destructive, not write. The binding is cheap to recreate with coding_repo_add, which alone would argue for write — but the call reaches out and stops engines on the owner's machine, and a scope is about what a call can disturb. Confirm string "coding_repo_remove", plus dry_run.
   • repo_id or repo_name, as the comment asked. The name resolves through filterReposByInstance — this issue's other half, load-bearing here: under concurrent mis-routing, resolving a name against a foreign listing would land a destructive call on an instance the caller never named.
   • An ambiguous name is refused, naming the matching ids, rather than resolved to the first match. The "Parents Clubs coder" case in the comment holds three stale local bindings; deleting the wrong one is not recoverable.
   • It does not delete the repository, any code, or anything on GitHub — and it is not remove_repo, which detaches a repo from repo-chat vector ingestion. Both facts are in the tool's own description, since that is what a model actually reads.

   16 tests, all green. Surface 0.1.24 → 0.1.25; surface-gated beside coding_repo_add, so 153 registrations become 154, MCP_TOOL_GATED 23 → 24, MCP_TOOL_ALWAYS_ON unchanged at 130. Fourteenth confirm-gated tool.

   The three bindings named in the comment (55f5f034…'s wrong-org GitHub binding, 3cbb1d1d…'s two dead local workdirs plus the obsolete apps/school-clubs entry) are now removable over MCP.

   3. The registration / "has not been loaded yet" loop — still external

   Unchanged, and one correction to how it is often summarised: this is not a server-side registration race, and the server does not emit those strings at all. #759 traced it:

   • grep -r "has not been loaded yet" over workers/ packages/ store/ agents/ returns nothing. The strings are also absent from @modelcontextprotocol/sdk@1.29.0, agents/mcp, mcp-remote, and the Claude Desktop app bundle. They are synthesized by the client. ProAgentStore:<name> is Claude Code's own client-side registry format.
   • There is no window in which a call can arrive before registration completes: partyserver's #initialize() wraps onStart() (which awaits init(), including userGroups()) in ctx.blockConcurrencyWhile(), a Durable Object primitive that holds every incoming request until initialization finishes.
   • The actual mechanism is client-side schema caching: on a new session the client may optimistically dispatch a call using a prior session's cached schema before its own tools/list response has been processed. The identical retry succeeds because by then it has been — which is exactly the "retry-succeeds-unchanged" characterisation in the comment above.

   #759 is labelled invalid / P3: later, and its BA triage recommends closing as external with a docs note. So this issue cannot close on its stated terms — it is waiting on "#759 resolved", and #759 is waiting on someone to accept its own diagnosis. Either closing #759 as external or recording that acceptance here would unblock it.

   4. A security invariant was tightened on the way through

   coding_repo_remove collided with repo-policies.test.ts, which enforces #322: promotion of a repo policy to act must stay a human action, because a policy is the one thing on this platform that acts with nobody present.

   It enforced that by forbidding any MCP source from constructing a per-repo path at all — a path-shape proxy for a method-specific rule. The promotion route is PUT …/coding/repos/:repoId; DELETE on the same path cannot promote anything. I checked the real invariant first: no MCP source PUTs to a repo path, so it held.

   Left as a path guard it would refuse every legitimate per-repo tool forever, and the likely response is to widen the allowed list — which is what would eventually admit a real PUT. So it now states the rule directly: no MCP authedCall may pair a repo path with method: "PUT". Comments are stripped first, because two of the three "offenders" it reported were this commit's own docblock naming the endpoint, and a guard that cannot tell an explanation from an instruction trains people to delete the explanation.

   Reported honestly: my first version of that guard was broken. It passed against a deliberately planted PUT — the path is held in a const and the call site is 400 characters away, so the window measured nothing. It now resolves path-holding identifiers and judges each call by its own first argument, and carries a G1 assertion that the scan saw at least three repo-path calls, so it cannot pass vacuously. Verified red three ways: planted PUT fails, blinded predicate fails, restored passes. 38 tests green.

   Suite at 4fa09145

   pnpm test 11,123 / 11,124 (619/621 files); pnpm typecheck 10/10 projects plus all three tsconfig.test.json programs; biome clean over 1,412 files; 13/13 check-*.mjs guards; docs:drift clean. The single failure is environmental and pre-existing — migration-hygiene.test.ts runs its guard with --require-history against a shallow checkout; CI uses fetch-depth: 0.
8. serge-ivo

   added a commit that references this issue on Sep 10, 2026

   fix(mcp): userGroups gets one bounded retry, so a blip cannot erase t…

   ...

   4983422
9. serge-ivo

   closed this as completedin f8e402aon Sep 12, 2026