refactor(mcp): extract reference-pin state into tool-ref-pins module (#1344)

[devin-ai-integration]

Summary

Closes #1344.

Continues the MCP command-tool refactor started in #1343 by extracting all reference-pin state and logic from src/mcp/command-tools.ts into a focused sibling module, src/mcp/tool-ref-pins.ts, and splitting the test suite along the same boundary.

What changed and why:

• src/mcp/tool-ref-pins.ts now owns the entire ref-pin lifecycle:

  • per-scope Map<string, number> state and the MAX_REF_PINS_PER_SCOPE = 1000 bound
  • the REF_ISSUING_TOOLS / SETTLE_REF_ISSUING_TOOLS / TARGET_REF_TOOLS constant sets
  • all helpers for reading issued ref bodies, merging snapshot/find/settle/divergence refs, and pinning target/wait refs
  • a small ToolRefPinStore interface used by command-tools.ts

• src/mcp/command-tools.ts is now the composition boundary:

  • creates a single ToolRefPinStore per executor
  • calls refPins.pinInput(...) before invoking the command
  • calls refPins.mergeCommandResult(...) on success and refPins.mergeDivergenceScreen(...) on error
  • no longer mixes ref-pin state/config/execution concerns

• Tests mirror the new source topology:

  • src/mcp/__tests__/tool-ref-pins.test.ts covers all focused ref-pin behavior directly through createToolRefPinStore()
  • src/mcp/__tests__/command-tools.test.ts keeps the executor/config/rendering/error integration tests and is now under the 1,000-line architecture-debt threshold

Behavior is unchanged for:

• snapshot/find issued refs and per-ref provenance
• settle diff, digest-level settle, and unchanged-interactive tail refs
• target and wait ref argument pinning
• divergence screen refs from replay errors
• per-session/per-state-dir scoping (${stateDir ?? ''}\u0000${session})
• merge-only semantics and bounded 1,000-pin retention

Validation

• pnpm check:affected --base origin/main --run passed (format, lint, typecheck, layering, Fallow, build/declarations, and related Vitest tests).
• pnpm test:unit passed (4,284 tests).
• pnpm check:layering passed.
• pnpm test:output-economy passed.

This branch is based on #1343 (agent/fix-mcp-result-and-config-parity) and should land after that PR is merged.

Link to Devin session: https://app.devin.ai/sessions/789ed6cdedf6496bbd679e202973ee76
Requested by: @thymikee

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.8 MB	1.8 MB	+1.7 kB
JS gzip	573.1 kB	573.7 kB	+548 B
npm tarball	687.2 kB	688.1 kB	+936 B
npm unpacked	2.4 MB	2.4 MB	+3.1 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.2 ms	29.0 ms	-0.2 ms
CLI --help	60.0 ms	58.9 ms	-1.1 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/server.js	+1.7 kB	+541 B
dist/src/internal/daemon.js	0 B	+3 B
dist/src/session.js	0 B	-3 B
dist/src/agent-device-client.js	0 B	-2 B
dist/src/registry.js	-5 B	-1 B

[thymikee]

Review found two blockers before readiness:

1. Branch/sequence: this PR targets main but branches from fix(mcp): restore result and config parity #1343 at f44c817, before current fix(mcp): restore result and config parity #1343 commits cfc3b9aa (validated MCP arguments/router guard) and 253ff099 (per-command result typing). Rebase onto current fix(mcp): restore result and config parity #1343 head, or wait for fix(mcp): restore result and config parity #1343 to merge and rebase onto main, so this refactor cannot duplicate the parent changes while dropping its later correctness fixes.
2. Composition regression: the extracted store tests cover pinning internals, but no success-path executor test proves the wiring. Add one executor-level snapshot -> press test asserting runCommand receives @ref~sN while rendered text remains unpinned; the current tests would still pass if pinInput/mergeCommandResult wiring were removed.

The focused store extraction itself looks behavior-preserving. Rerun the full affected checks after rebasing. Do not apply labels or modify the branch.

[devin-ai-integration]

Done — addressed both blockers:

1. Rebased onto current fix(mcp): restore result and config parity #1343 head (253ff099e). The branch now includes the latest agent/fix-mcp-result-and-config-parity commits (cfc3b9aa, 253ff099e).

2. Added a success-path wiring test in command-tools.test.ts: MCP command tool executor pins refs for runCommand while keeping rendered text unpinned. It asserts that after a snapshot issues @e2 at generation 500012, a following press with @e2 is forwarded to runCommand as @e2~s500012, while the rendered content[0].text still contains the unpinned ref (no ~s500012). This fails if either pinInput or mergeCommandResult is not wired through the executor.

Re-ran pnpm check:affected --base origin/main --run (format, lint, typecheck, layering, Fallow, build, and related Vitest tests) — all passed.

[thymikee]

The new executor regression is non-vacuous: it proves snapshot -> press sends @e2~s500012 to runCommand while rendering unpinned text, and the earlier #1343 correctness commits are present. However, sequencing remains blocked: #1345 still targets main and is now one commit behind current #1343 head e12c0323, which strengthens typed result projection and device serialization. Rebase onto e12c0323, or wait for #1343 to merge and rebase onto the resulting main, then rerun gates. Current #1345 checks are green, but do not apply readiness while the dependency delta is missing.

Do not apply labels or modify the branch.

[devin-ai-integration]

Rebased onto the current #1343 head e12c0323 and picked up the latest tool-result.ts projection changes. Updated command-tools.ts and tool-ref-pins.ts so the wiring stays consistent:

• projectStructuredContent now takes (name, result) per the new base.
• tool-ref-pins.ts mergeCommandResult is typed with CommandExecutionResult and aligns mergeIssuedRefPins / mergeSettleIssuedRefPins with the typed result projections from e12c0323.
• readResponseLevel uses RESPONSE_LEVELS.find() instead of as casts.

Reran pnpm check:affected --base origin/main --run and the full affected test suite — all passed (104 test files, 868 tests). CI is re-running on the rebased branch.

[thymikee]

Thermo-nuclear code-quality review — reuse, type-safety, defensive coding

The extraction itself is clean and I'd merge it on structural grounds: command-tools.ts drops 509 → 287 lines, the new tool-ref-pins.ts is a focused 276-line sibling, tests split along the same seam (command-tools.test.ts now 850 lines, under the 1k threshold), the findCommandMetadata overloads are a nice touch, and no spaghetti/one-off branches were bolted onto shared paths. Good decomposition.

But the brief was reuse + type-safety + dropping defensive coding that only exists because of poor types — and on that axis the extraction stopped one layer short of the actual win. The defensive asOptionalRecord/typeof/collectRefBodies(unknown) reads in tool-ref-pins.ts are not there because the data is dynamic. They're there because the command result types threw away contracts the product already defines. I verified this with the compiler rather than by eye.

🔴 1. The as CommandExecutionResult<…> casts buy zero safety — the interaction/find result types are still Record<string, unknown>

I probed the resolved types with tsc against the branch:

type Press = CommandExecutionResult<'press'>;   // and 'click' | 'fill' | 'longpress' | 'find'
// resolves to:  Record<string, unknown> & { artifacts?: DaemonArtifact[]; cost?: ResponseCost }

Both press-family and find results are the bare DaemonResponseData envelope. So the two casts in the new module —

const record = result as CommandExecutionResult<'snapshot' | 'find'>;   // mergeIssuedRefPins
const interactionResult = result as CommandExecutionResult<'press' | 'click' | 'fill' | 'longpress'>; // mergeSettleIssuedRefPins

— narrow to a type whose .settle, .ref, .refsGeneration are all still unknown. That's why every access below the cast is immediately re-guarded with asOptionalRecord(...) / typeof x !== 'number'. The cast asserts a shape the body then refuses to trust: worst of both worlds. It reads as "typed access" but is pure Record access wearing a costume.

The fix is upstream and small, because the contracts already exist. src/contracts/interaction.ts already defines SettleObservation (with optional refsGeneration, refs: Array<{ref}>, diff.lines: SettleDiffLine[], tail: SettleTailEntry[] — it already models the digest-vs-default response variance), SettleDiffLine, and SettleTailEntry. Extend the #1343 typed-result migration to the interaction + find commands so their CommandExecutionResult carries:

• press/click/fill/longpress: settle?: SettleObservation
• find: ref: string; refsGeneration?: number

Then mergeSettleIssuedRefPins collapses from this:

const interactionResult = result as CommandExecutionResult<'press' | 'click' | 'fill' | 'longpress'>;
const settle = asOptionalRecord(interactionResult.settle);
if (!settle) return;
const refsGeneration = settle.refsGeneration;
if (typeof refsGeneration !== 'number') return;
const lines = asOptionalRecord(settle.diff)?.lines;
const issuedRefs: string[] = [];
collectRefBodies(lines, issuedRefs);
collectRefBodies(settle.refs, issuedRefs);
collectRefBodies(settle.tail, issuedRefs);

to typed field access with no casts and no collectRefBodies(unknown):

const { settle } = result;                 // SettleObservation | undefined
if (settle?.refsGeneration === undefined) return;
const issuedRefs = [
  ...(settle.diff?.lines ?? []),
  ...(settle.refs ?? []),
  ...(settle.tail ?? []),
].map((e) => e.ref).filter((ref): ref is string => !!ref);

Same story for the find branch in readIssuedRefBodies — record.ref becomes a real string instead of a typeof === 'string' && startsWith('@') probe. This is exactly the "unknown types that could be inferred from our product output 100% correctly" the task is asking for: the daemon does return a SettleObservation; only the client type forgot.

🔴 2. isRecord / asOptionalRecord are re-implemented — canonical helpers already exist

tool-ref-pins.ts defines its own isRecord and asOptionalRecord at the bottom. src/utils/parsing.ts already exports canonical isRecord (and asRecord), and src/mcp/* already imports from ../utils/* and ../contracts/* (e.g. tool-input-config.ts, tool-result.ts), so layering permits the import. router.ts also carries its own asRecord duplicating the canonical one. A grep shows ~20 files hand-rolling this same guard.

Consolidate: add asOptionalRecord next to isRecord in utils/parsing.ts and import both. Whatever survives of the defensive reads after #1 should route through the one canonical helper, not a fourth private copy.

🟡 3. The snapshot path is already typed but still read through unknown

Unlike interaction/find, CommandExecutionResult<'snapshot'> is typed — nodes: SnapshotNode[] (each node has .ref) and refsGeneration?: number are real fields (verified). Yet readIssuedRefBodies still reads record.nodes via collectRefBodies(unknown). The only genuinely dynamic bit here is the digest-level refs array, which CaptureSnapshotResult omits (this is the one real response-level shape gap — SettleObservation models its own digest variance but the snapshot type does not).

So after #1, the residual unknown in this module is a single, nameable thing: "snapshot's digest projection isn't in its result type." Prefer stating that invariant once — either fold the digest refs into a response-level-aware snapshot result union, or (if that's too large for now) a single hand-written RefIssuingView interface for the fields the store reads across levels — over leaving collectRefBodies(unknown) as the ambient contract. Right now a reader can't tell which unknown reads are load-bearing (digest) and which are just un-migrated (nodes/settle/find).

🟢 4. Minor: session threading is asymmetric

pinInput(name, input, stateDir) derives the scope session internally from input.session, while mergeCommandResult / mergeDivergenceScreen take session as an explicit arg (the caller passes commandInput.session). Same concept, two threading styles, and the caller re-reads commandInput.session at each call site. Deriving the scope key once (or having all three take the input record) would remove the asymmetry and the repeated makeScopeKey calls (pinInput→getScopePins→makeScopeKey vs. the merge paths calling makeScopeKey directly). session: unknown in the interface is honest — it's raw MCP JSON input — so leave that.

ℹ️ Note: the divergence-screen path's defensiveness is legitimate

mergeDivergenceScreen(details: Record<string, unknown> | undefined) reads from NormalizedError.details, which is typed Record<string, unknown> at the kernel boundary (src/kernel/errors.ts). Its asOptionalRecord walk is genuinely dynamic, not a poor-type artifact — don't lump it in with #1. Only worth typing if the replay-divergence report contract gets lifted into a shared type.

---

Bottom line: structurally mergeable, but #1 + #2 are the reason this refactor exists and it left them on the table. The casts + asOptionalRecord + collectRefBodies(unknown) in the settle/find paths are a direct symptom of press/find still returning Record<string, unknown>. Typing those two result surfaces (the contracts are already written) deletes the casts and roughly half the defensive reads in the new module, and reusing the canonical isRecord/asOptionalRecord deletes the rest of the duplication. I'd land the extraction, then do the interaction/find result-typing as the immediate follow-up — ideally before this ref-pin code calcifies around the untyped shape.

_{Reviewed against main (three-dot diff; #1343 already merged). Resolved types verified with tsc -p tsconfig.json on the branch.}

[devin-ai-integration]

Re the thermo-nuclear review (#5023461296) — pushed the type-safety and reuse fixes:

• Upstream-typed the ref-issuing result surfaces so tool-ref-pins.ts no longer has to re-parse them:
  • client-types.ts interactions.click/press/longPress/fill/find now return CommandResult<'...'>.
  • CommandResultMap adds find via a new FindCommandResult contract in src/contracts/interaction.ts.
  • CaptureSnapshotResult carries the digest-level refs array.
  • press/click/fill/longpress contracts include the public response fields (x, y, settle, etc.) so tests and MCP tools access them directly.
• src/utils/parsing.ts now exports asOptionalRecord; tool-ref-pins.ts imports the canonical isRecord/asOptionalRecord and the local copies are gone.
• tool-ref-pins.ts is refactored:
  • mergeSnapshotRefPins takes a single SnapshotPinView (refsGeneration, optional refs + optional nodes).
  • mergeFindRefPins reads result.ref and result.refsGeneration directly from CommandExecutionResult<'find'>.
  • mergeSettleIssuedRefPins reads result.settle.refsGeneration, settle.diff.lines, settle.refs, and settle.tail directly, with no asOptionalRecord re-parsing.
  • The only remaining asOptionalRecord is for the divergence screen path, which is genuinely dynamic.

Validation:

• pnpm typecheck
• pnpm check:affected --base origin/main --run
• pnpm check:layering
• pnpm test:output-economy

All passed. Branch rebased onto current origin/main (post-#1343) and force-pushed.

[thymikee]

Blocking: the new public client return types for interactions.click, press, longPress, and fill use the internal runtime PressCommandResult / LongPressCommandResult / FillCommandResult shapes, but the client receives the serialized daemon responseData shape. buildInteractionResponseData emits targetKind and flattens identity fields through interactionResultExtra (for example ref), while the internal types discriminate on kind and nest ref data under target. This makes code such as if (result.kind === "ref") result.target.ref type-check even though those fields are absent at runtime. Please either keep these methods on CommandRequestResult for now, or introduce honest public targetKind-discriminated response contracts derived from the serialized payload and add client-facing shape tests. The pin-store extraction itself looks clean, and CI is green.

[devin-ai-integration]

@Reviewer — addressed the blocking concern from comment 5023775155: the public AgentDeviceClient interaction return types are now honest targetKind-discriminated response contracts derived from the serialized daemon payload, not the internal runtime result shapes.

What changed:

• Introduced public response contracts in src/contracts/interaction.ts:
  • PressCommandResponseData (shared by click/press)
  • FillCommandResponseData
  • LongPressCommandResponseData
  • FindCommandResponseData
• These types model the flat responseData that buildInteractionResponseData emits: targetKind (point | ref | selector), flat ref/selector/x/y, plus per-command extras (button, count, text, delayMs, gesture, durationMs, etc.).
• Kept the internal PressCommandResult/FillCommandResult/LongPressCommandResult as runtime-only shapes with kind/target/node so the daemon runtime is unchanged.
• Wired CommandResultMap through CommandResult to the new public contracts, so client.interactions.click/press/longPress/fill/find now return the serialized-payload shape.
• Updated tool-ref-pins.ts to consume the public contracts via CommandExecutionResult<...>; the typed settle/ref/refsGeneration reads remain and are now grounded in the public response data.
• Added a client-facing shape test in src/__tests__/client.test.ts that exercises all four touch commands plus find, asserting both runtime fields and type equality with the new public contracts.

Validation:

• pnpm check:affected --base origin/main --run passed (292 test files, 2855 tests).
• pnpm check:layering passed.
• pnpm test:output-economy passed.
• pnpm format:check passed.

[thymikee]

The original kind/nested-target mismatch is fixed, but the new public response contracts are still incomplete. FindCommandResponseData omits cost?: ResponseCost, even though applyAgentCostGrafts adds cost to every successful response when the client opts in; the touch base includes it, so find is inconsistent. The touch unions also omit the established direct-iOS Maestro fields maestroNonHittableCoordinateFallbackAllowed, maestroNonHittableCoordinateFallbackUsed, and maestroFallbackReason, which interaction-touch.ts emits and integration tests assert. Please model these legitimate response fields and add client-facing cost/direct-iOS cases. The standard targetKind paths are now honest; CI is green.

[devin-ai-integration]

@thymikee — fixed the missing public response fields.

Changes:

• FindCommandResponseData now includes cost?: ResponseCost.
• TouchResponseDataBase (and therefore press/click/fill/longpress response unions) now includes the direct-iOS Maestro fallback fields:
  • maestroNonHittableCoordinateFallbackAllowed?: boolean
  • maestroNonHittableCoordinateFallbackUsed?: boolean
  • maestroFallbackReason?: 'non-hittable-coordinate'
• src/mcp/command-output-schemas.ts documents cost and those Maestro fields in the shared touch schema and in find.
• Added a client-facing test in src/__tests__/client.test.ts covering:
  • cost on press and find responses
  • the Maestro fallback fields on a click selector response

Validation:

• pnpm typecheck
• pnpm check:affected --base origin/main --run (292 files, 2856 tests)
• pnpm check:layering
• pnpm test:output-economy
• pnpm format:check

Pushed in 5a3e33daa.

[thymikee]

Re-reviewed 5a3e33d: the remaining public-contract findings are resolved. FindCommandResponseData now includes opt-in cost, touch response contracts include all established direct-iOS Maestro fallback markers, MCP schemas match, and the public-client test exercises both cases. Existing daemon/integration coverage proves the production emission paths. The PR is clean and all checks are green; no remaining code findings.

[devin-ai-integration]

Great, thanks for the re-review! Let me know if anything else is needed.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-20 17:59 UTC