Add get_selection / highlight_selection: the agent-to-viewport selection bridge (#189, #190) - #191
Merged
Merged
Conversation
…ion bridge (#189, #190) Implements the two new MCP tools per both issues' refined-spec comments, against the wire format in SecondMouseAU/OCCTSwiftInteraction#17 (in review in parallel; schema treated as final regardless of later wording changes there). No host implements the writer/watcher side yet, so this only consumes hand-written fixture sidecar files. get_selection reads selection.json + host.lock from the resolved output directory (Paths.swift) and returns a three-state result: noHost (selections: null), hostRunning with selections: [] (host live, nothing selected), or hostRunning with selections: [...]. Each entry is resolved against this server's own scene the same way select_topology resolves a match, reusing SelectionTools.graphIndex(...) and SelectionRegistry.record so the minted selectionId composes with remap_selection/measure_distance/ etc. A per-entry resolution failure (bad bodyId, out-of-range index) is reported inline without failing the whole response; a running host with selection.json missing or malformed is reported as an explicit tool error, never swallowed into an empty result. highlight_selection writes highlight_requests/<id>.json atomically (Data.write(to:options:.atomic)) and polls highlight_requests/handled/<id>.json for a bounded timeout, returning the host's real applied/rejected/superseded outcome, an explicit timeout, or noHost immediately (no request written) when no host is running. kind/scheme are validated against their closed wire-format enums before writing; bodyId/index are written through unvalidated against the live scene, since this tool has no other access to check them. Shared plumbing: HostLock.checkLiveness(outputDir:) probes host.lock with a non-blocking shared flock (success means no host, EWOULDBLOCK means one is running), added in the new Sources/OCCTMCPCore/Tools/SelectionBridgeTools.swift per the code-structure policy (one file per tool family). Registers both tools in Server.swift's catalog + dispatch. Updates the tool count (77 -> 79) across README.md, CLAUDE.md, and the docs site, adds the two tools to docs/reference/selection.md and its Families table, and documents the new file in CLAUDE.md's Tools/ list. 13 new fixture-driven tests in SwiftTests/OCCTMCPCoreTests/SelectionBridgeToolsTests.swift cover all three liveness states, per-entry resolution failures, missing/malformed selection.json, atomic request writes, handled/ polling (applied + rejected), the timeout path, the noHost fast path, unvalidated-scene-reference writes, and client-side kind/scheme enum rejection. Fixes PingTests' hardcoded tool count to match. Full suite: 232/232 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summary (commit 812e64c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 812e64c)Status: 3 Issues Found | Recommendation: Address warnings before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 159.7K · Output: 2.3K · Cached: 280.8K |
…, cancellation Three review findings on SelectionBridgeTools.swift, all applied as real correctness/robustness fixes rather than dismissed: - HostLock.checkLiveness treated ANY flock failure as hostRunning, not just EWOULDBLOCK (the actual "something holds the exclusive lock" signal). A failure for another reason (locking unsupported on the filesystem, an interrupted call) now falls open to noHost, matching the file's own already-stated design philosophy for the open() failure case just above it. - readHandledOutcome silently treated a handled/<id>.json that exists but fails to decode the same as "not there yet", so a malformed host response read as a plain timeout instead of surfacing that the host actually responded. Replaced with pollHandledOutcome returning a three-case HandledPoll (pending/decoded/malformed) so the poll loop reports outcome "error" immediately instead of waiting out the whole deadline. - The poll loop's try? await Task.sleep(...) swallowed CancellationError, so a cancelled call kept polling until the timeout instead of exiting early. Added an explicit Task.isCancelled check after each sleep, returning outcome "cancelled". Two new tests cover the malformed-handled-file and cancellation cases. The flock errno branch has no practical way to force a non-EWOULDBLOCK failure through the public API in a portable test, so it's covered by review/inspection rather than a new test; the existing HostLock coverage (held lock -> hostRunning, no lock -> noHost) is unaffected and still passes. swift build + swift test: 234/234 passing (232 previous + 2 new). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
What & why
Implements
get_selectionandhighlight_selection, the read and write halves of theagent-to-viewport-host selection bridge, against the refined-spec acceptance criteria on both
issues (posted 2026-08-21), not the original pre-refinement issue bodies. One PR for both since
they share all the plumbing (the
host.lockliveness probe, the resolved output directory,the wire format).
Neither tool calls into a live selection directly: OCCTMCP is a stdio MCP process with no
persistent link to any app's live
InteractiveContext/ViewportService. Both instead speak thesidecar-file wire format from
SecondMouseAU/OCCTSwiftInteraction#17(the ADR, in review inparallel with this work) in the resolved output directory, the same convention every other
OCCTMCP sidecar already uses. No host implements the writer/watcher side yet
(
OCCTSwiftInteraction#16/ACADStudio#16are still upstream), so every test here plays the hostitself against hand-written fixture files, exactly as both refined-spec comments describe.
get_selection: readsselection.json+host.lock. Three-state result (noHost/hostRunningwith an empty or populatedselectionsarray, never a bare boolean or empty-arraycollapse). Each selection resolves against this server's own scene the same way
select_topologyresolves a match, reusing the existing
SelectionTools.graphIndex(...)convention andSelectionRegistry.record(...), so the mintedselectionIdcomposes withremap_selection/measure_distance/etc. A per-entry resolution failure (badbodyId,out-of-range
index) is reported inline without failing the rest of the response; a runninghost whose
selection.jsonis missing or malformed is reported as an explicit tool error, sincethe host is expected to maintain that sidecar for its whole lifetime.
highlight_selection: writeshighlight_requests/<id>.jsonatomically(
Data.write(to:options:.atomic), temp name + rename) and pollshighlight_requests/handled/<id>.jsonfor a bounded timeout, returning the host's realapplied/rejected/supersededoutcome, an explicittimeout, ornoHostimmediately (norequest written) when no host is running at all.
kind/schemeare validated against theirclosed wire-format enums before writing;
bodyId/indexare written through unvalidated againstthe live scene, since this tool has no other access to check them (a bad reference still comes
back as the host's own
rejectedoutcome through the same poll).Closes #189, Closes #190
Checklist
tests in
SwiftTests/OCCTMCPCoreTests/SelectionBridgeToolsTests.swift, covering everyacceptance-criteria checkbox on both issues (the three liveness states, per-entry resolution
failures, missing/malformed
selection.json, the atomic request write,handled/pollingfor both
appliedandrejectedoutcomes, the timeout path, thenoHostfast path,unvalidated-scene-reference writes, and client-side
kind/schemeenum rejection). Fullsuite: 232/232 passing (
swift build+swift test).Notes for the reviewer
Judgment calls the refined-spec comments didn't fully pin down:
highlight_selection'sidgeneration. The spec says "generating id itself so retries areidempotent (per the ADR's Section 6.1 mitigation)" without the ADR text available to consult
(it's in review in a separate repo). I generate a fresh
UUID()per call rather than adeterministic hash of the request content: a content-derived id would make a genuinely repeated
call (not a retry) with identical arguments silently short-circuit against a stale
handled/file from an earlier, already-completed call, which is wrong for
scheme: "xor"in particular."Idempotent" here is interpreted as "the tool owns id generation so a caller never needs to
invent or coordinate its own id across concurrent calls," combined with the atomic write making
a single call's own request write safe to retry. Flagging this in case the ADR's actual Section
6.1 intends the deterministic-hash reading instead.
selection.jsonmissing entirely. Treated the same as malformed (anexplicit tool error), not as
hostRunning(selections: []), on the reasoning that the ADR expectsa live host to maintain this sidecar for its whole lifetime, so its total absence while
host.lockis held is anomalous rather than "nothing selected yet." The refined spec's bulletgroups "missing/malformed" together, which reads consistently with this, but it's worth a second
look.
highlight_selection's liveness check happens before writing. Whenhost.lockshows nohost, the tool returns
outcome: "noHost"immediately without writing a request file at all,rather than writing it and then timing out. This matches "rather than hanging... when nothing is
consuming
highlight_requests/" and avoids leaving an orphaned request file nobody will everread.
selection.json/highlight_requestsentries carry a plainindex: Intwith no stated enumeration space. Treated it as an ordinal into the same enumerationselect_topologyalready uses (Shape.faces()/.edges()/subShapes(ofType: .vertex)),converting to a
BRepGraphnode index via the existinggraphIndex(...)helper before mintingthe anchor, matching the "resolved the same way
select_topologyalready resolves them"language and the
SubShapeRef.ordinal/Shape.faces()conventionOCCTSwiftInteractionsettled on fleet-wide (per the Re-key SelectionRegistry on GraphUID #182 write-up in this repo's own
SelectionRegistry.swift).SelectionTools'stest suite"; there's no existing
SelectionToolsTests.swiftto add to, so both tools' tests livein a new
SelectionBridgeToolsTests.swiftalongside the newSelectionBridgeTools.swiftsourcefile, per the code-structure policy's one-file-per-tool-family convention.
README.md,CLAUDE.md,the docs site,
okf/), and added both tools todocs/reference/selection.md+its Families table. Left other, unrelated pre-existing doc drift in those same files untouched
(e.g.
docs/reference/README.mdsaid "75-tool" before this PR touched it for an unrelatedreason; fixed that one inline since I was already editing that exact line, but did not do a
wider unrelated cleanup pass).
Not merging — for review.
🤖 Generated with Claude Code