feat(macos): native Connect Client form — preview-first, token-guarded (spec 091) - #941
Open
Dumbris wants to merge 40 commits into
Open
feat(macos): native Connect Client form — preview-first, token-guarded (spec 091)#941Dumbris wants to merge 40 commits into
Dumbris wants to merge 40 commits into
Conversation
…ion undo, preview contract, legacy sheet routing)
…penCode absent case, sanitized existing entry, protocol coverage
…oken for all change kinds, protocol fixture split
…ary-by-construction, HMAC token incl. pending entry + adoption, refusal in preview, 409 discriminator, strict-socket, SC-006 home)
…te-capable scoping, absent-control convention
…rinfo-stripping restatements
… SC-003 trace, FR-007 field in T019)
go test ./internal/connect/... ./internal/httpapi/... is green with no pre-existing failures; the SC-003 undo round-trip is already covered by the unchanged undo_test.go suite (byte-exact restore + created-file removal), confirmed green here.
Related #91 502 XCTest cases, 0 failures on branch 091-connect-client-form before any spec-091 Swift change.
The preview now reports WHAT a connect would overwrite without echoing any client-config content: a new EntrySummary built by construction from a fixed whitelist of parsed projections (entry name, transport type, endpoint with query/userinfo/fragment stripped, command, header NAMES, env NAMES). The existing-entry resolution mirrors the write, including OpenCode's equivalent-entry adoption, so the summary names the key that actually disappears. Tests feed rotated header values, bearer/env secrets, ?apikey= and user:pass@ URLs, mcp-remote --header args and user-authored fields, and assert none of those values appear anywhere in the serialized preview.
Related #91 ConnectPreviewModel decodes the spec-091 preview additions (sanitized existing-entry summary, opaque precondition token, verbatim connect refusal) and derives the change kind, safety-net statement and credential notice the form renders before any Connect control exists. ClientStatus gains icon/server_name/access_state/remediation, all optional so an older core still decodes, with a generic symbol for an unknown client id.
DerivePreconditionToken is an HMAC-SHA256 over a canonical, length-prefixed encoding of the raw pre-write state a preview described — config path, file existence, the resolved (adoption-aware) target entry name and value — plus the exact pending entry that would be written, so external drift AND proxy-side drift (key rotation, require_mcp_auth toggle, listen-address change) both invalidate a preview. The key is per-core-instance, random and in-memory: the token is neither forgeable nor usable as an offline confirmation oracle for a masked value, and never outlives the process. Preview and the write share one pre-write-state resolver so both hash the same lookup.
The absent-config guard that makes OpenCode non-connectable moves out of Connect into a shared connectRefusal(), and the preview runs it, reporting the reason verbatim in connect_refusal. A form can now hide its Connect control and show the reason up front instead of letting the user discover it by clicking — and can no longer promise "a new file will be created; Undo removes it" for a client where that is false. The test compares the preview's refusal to the real write's error text, so the two guards cannot drift apart silently.
…ocket writes Related #91 Adds clientDetail, connectPreview, connect (force + precondition token), undoConnect and disconnect to APIClient, with a 409 surfaced as a typed connectConflict carrying the core's action discriminator. transportKind exposes whether the client talks over the private socket; the three mutating calls refuse any other transport and are sent in strict-socket mode, so a socket that disappears mid-session fails the write instead of letting it ride TCP.
ConnectWithPrecondition re-resolves the pre-write state and the entry it would write, recomputes the preview's token and refuses before any backup when anything drifted — the file appearing or vanishing, the resolved (possibly adopted) entry changing in a way the sanitized summary hides, or the proxy's own config changing what would be written. force=true rides with the token for a replace and never rescues a stale one; an absent token keeps today's behavior for the Web UI and CLI. The 409 now discriminates precondition_failed (re-preview) from already_exists (pass force), at the top level and inside data, so a replace flow can branch instead of looping. Swagger regenerated.
…form Related #91 @mainactor reducer over a narrow data-source seam: stat-only list with a 2 s reachability poll, detail plus no-write preview on explicit selection, and a Connect control that exists only while a preview is resolved for the current client and entry name, was not refused by the core, and has a readable config. Writes echo the preview's precondition token, an overwrite sends force only together with that token, and a precondition_failed conflict re-previews exactly once instead of retrying. Mutating actions are disabled off-socket and while in flight.
Serves the real REST handler over a real Unix socket through the production
ListenerManager, multiplexListener and ConnContext, then POSTs
/api/v1/connect/{client} with no credential at all and requires the client
config to actually be written — joining listener, source tagging, admin auth
conversion and the write gate, which httptest can never do (it creates no
connection, so ConnContext never runs). A restricted agent token over TCP on
the same running server is the negative control.
The ConnContext hook moves out of the http.Server literal into a named
taggedConnContext so the test drives the production tagging verbatim rather
than a copy that could drift.
Related #91 Adds the "Connect Client…" tray item beside "Add Server…", routed through one shared presentation route that the app delegate presents directly — sheet on the main window, standalone window otherwise — with no delayed notification chains. The view is thin over ConnectClientModel: stat-only client list, detail plus preview pane (pending entry, config path, sanitized existing-entry summary, safety-net statement, credential notice, verbatim refusal), an Advanced entry-name override collapsed by default, and an action bar whose Connect control only exists when the model says a matching preview is rendered. Stable accessibility identifiers back the UI tests.
Related #91 The list renders the cheap existence-only truth ("config present" / "no config found" / unsupported + reason) and is overlaid with the authoritative state only for clients the user explicitly selected, so opening the form still reads no client config contents. A completed connect or disconnect re-reads just the affected client instead of the whole list.
Related #91 Undo appears only for a connect performed in this open form, carries that connect's backup identity reduced to the bare filename the core requires (a path is a 400), and disappears once used or once the form closes — the core keeps no cross-session undo state. Disconnect now raises a confirmation naming the config file and the entry; the request is sent only by confirming it, so no control reaches the write without the disclosure.
Related #91 The dashboard's Connect Clients button opened a sheet that wrote a client's config directly — no preview, no backup disclosure. It now posts the shared Connect Client presentation route, and the sheet is deleted along with the preview-less connectToClient/disconnectFromClient API methods it used, so no native path can write a client config without the preview step (FR-012).
Adds docs/features/connect-clients.md covering the connect surface and the two Spec 091 contract deltas: - preview: existing_entry_summary (whitelist projection of the entry a connect would replace — names of headers/env, never values; endpoint with query, userinfo and fragment stripped) and connect_refusal (the write's own verbatim refusal, today OpenCode with no config); - write: precondition_token (keyed HMAC over config path, file existence, the resolved entry's raw value and the pending entry) plus the discriminated 409 (precondition_failed vs already_exists). Mirrors both onto the REST reference. Also fixes quickstart.md — the manual run needs the scratch --config file seeded first, since the core exits rather than creating it — and records the manual protocol as NOT RUN. Related #
findEquivalentJSONServerName returned the first match out of a randomly-ordered Go map, and the preview, the precondition check and the write each called it independently. Two entries pointing at the MCP endpoint could therefore have the token hash one entry while the write replaced or deleted another - a spurious precondition_failed, or a silent force-delete of an entry the summary never named. The matcher now prefers the requested name, then the lowest URL-equivalent name in sorted order, and ConnectWithPrecondition resolves the pre-write state once and threads that single resolution through both the token check and the write.
The write refuses two things regardless of user intent: an absent OpenCode config, and a .jsonc that actually uses comments (rewriting it would strip them). Preview only ran the first, and a commented file parses leniently, so the form rendered a resolved preview with an enabled Connect control for a write that always returns 400 - exactly the click-to-discover contradiction FR-003 exists to close. Preview now runs both guards and reports the first refusal verbatim.
…oken newExistingEntry projected the resolved config value to a map, so any non-object value (a hand-edited string, number or array under the target key) became a nil map and canonicalized to the constant "null". Two completely different values therefore produced byte-identical tokens: the drift was invisible, the precondition passed, and a force write replaced state the user never saw - the exact class FR-005 promises to catch. The resolved entry now carries its raw value alongside the object projection; the summary keeps using the projection, the token hashes the raw value.
The connect, disconnect and undo handlers decoded the request body only when r.ContentLength > 0. A chunked or otherwise streamed request has ContentLength -1, so the whole body was dropped: force and precondition_token vanished and the write ran in unguarded legacy mode, which is the opposite of what a caller sending a token asked for. Bodies are now decoded whenever one is present, with an empty body treated as an absent one.
connectRefusal ran before the precondition check, so an OpenCode config deleted between the preview and the confirm produced the flat "no OpenCode config found" error (HTTP 400) instead of the discriminated 409 precondition_failed FR-005 mandates for a file disappearing after an add/replace preview. The form read that as a hard failure rather than re-previewing. The refusal guard now runs after the drift check, so it still answers the tokenless legacy path unchanged.
The 409 was documented as contracts.ErrorResponse {success, error,
request_id}, while the handler writes {success, data, error, action}.
Spec 091 makes the top-level action THE discriminator between
already_exists and precondition_failed, so a client generated from the
OpenAPI document could not see the one field it must branch on.
Adds ConnectConflictResponse, writes it from the handler instead of an
ad-hoc map, and regenerates the spec.
…rite The only busy signal was action == .inFlight, and invalidatePreview() unconditionally reset action to .idle. Since neither the client list nor the entry-name field was disabled, clicking another row or editing the name while a POST was suspended cleared the busy state, re-enabled Connect and allowed a second concurrent read-modify-write of the same config file. On completion the outcome was published into whatever pane was showing, so one client's success banner could render under another client's name while its own row kept a stale label. Busy is now its own flag, an outcome is only published for the client it was performed on, and the affected client's row refreshes even when the selection moved on. The post-action preview refresh no longer restores its captured outcome over a newer action - an Undo started during that refresh keeps its own state and cannot be sent twice.
The FR-009 guard bare-returned without reverting the List's own selection, so clicking an unsupported client moved the highlight while the detail pane, the rendered preview and an enabled Connect button all still belonged to the previously selected client - pressing Connect then wrote a config the user was not looking at. The model now states which selection the list may hold and the view snaps the highlight back to it. Re-selecting the current client is also no longer a reselect, so the revert cannot trigger a redundant refetch.
connectControlExists hid the Connect control for any replace whose token was empty, including against a core that does not send precondition_token at all. A tray newer than its core therefore showed a full preview, a backup promise and no button, with nothing said about why - permanently. The preview now records whether the core sent the field (a 091 core always does, even empty), which separates "this core has no tokens" - the documented legacy fallback, control available, no token sent - from "this core owed me a token and gave none", which stays blocked but now states its reason and stops promising a backup.
The standalone form window is .closable with isReleasedWhenClosed = false, and only the form's own Close button ran the dismissal path. A red-button close therefore just ordered the window out: the hosting view, the model and the SwiftUI .task behind them stayed alive, so the 2 s core-reachability poll kept hitting the core from a window nobody could see until the next presentation replaced it. Window and model are now owned together by ConnectFormWindowLifecycle, which both exits run through: the session-scoped undo ends and the view hierarchy is released, which is what actually cancels the task.
…tion The snap-back guard reverts the highlight after the fact, which is right for a click but turns an arrow-key walk into a trap: the caret lands on an unsupported row, is bounced back, and every supported row BELOW it becomes unreachable by keyboard (a mouse can jump past it). Rows now declare their selectability to the List via selectionDisabled, so it skips them natively. That API is macOS 14+, and the package deploys to macOS 13, so it is applied through an availability-gated modifier; below 14 the model-side guard remains the backstop.
Presented as a sheet, the form's window never gets a close notification of its own. Probed on macOS: AppKit lets a sheet-bearing parent close, orders the sheet out with it, posts willClose for the HOST only, and never runs beginSheet's completion handler - which was the only teardown hook the sheet path had. Closing the main window with the Connect sheet attached therefore leaked the model and its 2 s reachability poll. The lifecycle now records the host at adoption and tears down on its close too, with window.sheetParent as a second route for a form adopted without one.
The lifecycle tests proved the hosting view and model are released, but nothing ever started the poll, so its cancellation was only inferred. This runs the poll for real against an unreachable core with a counting sleeper: it iterates, and the counter freezes once the task carrying it is cancelled. Verified by mutation - dropping loadList's Task.isCancelled check turns it red with 6835 iterations after cancellation. The SwiftUI half of the chain stays untested by necessity: probed here, a .task never starts under swift test even with a key, activated window and a hosting controller. The test comment records that.
Deploying mcpproxy-docs with
|
| Latest commit: |
0b7c70a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ecc183a.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://091-connect-client-form.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The preimage hashed the RESOLVED entry name, which is the empty string for every absent target, and never the requested one. Two previews for different server_name values over the same config therefore produced an identical token, so a token minted for "alpha" authorized a write that created "beta" - a key the user never previewed. The same substitution worked when two requested names adopted one equivalent entry. The preimage is now a named PreconditionState carrying the client id and the requested name alongside the observed state, both length-prefixed like every other field. Verified by mutation: dropping the requested name again makes the write create the substituted key. research.md D1 described the old preimage; corrected.
Every os.Stat error collapsed to accessAbsent, so a config blocked by permissions (its own, or its parent directory's) previewed as a create: "this file does not exist; it will be created, and Undo removes it", Connect available, token minted - and the denial was discovered only by clicking, the very thing running the write's guards in the preview is for. Only IsNotExist is absence now. A permission error becomes the typed AccessError (403 + remediation) and anything else is classified conservatively as malformed, which offers no Connect control either. The same misclassification is fixed in GetStatus and the stat-only aggregate, and the OpenCode resolver now treats an unstattable candidate as present rather than silently targeting the .json its .jsonc shadows. Existence checks route through a stat seam alongside the existing read seam, so this is testable without depending on permission bits; a real chmod-000 test covers the OS path too.
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
2 participants
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
A native tray path for connecting AI clients (Claude Code, Cursor, Windsurf, VS Code, Codex, Gemini, OpenCode, Claude Desktop) — "Connect Client…" menu item + SwiftUI form replacing the Web-UI-only flow and the legacy preview-less dashboard sheet:
forcenever rescues a stale token.Backend deltas are additive and back-compatible (absent token = legacy behavior; Web UI unchanged): 3 new preview fields, optional
precondition_tokenon the write, typed 409 discriminator, plus a SC-006 socket-to-gated-route e2e test.Spec & reviews
Full speckit artifacts in
specs/091-connect-client-form/— spec approved after 4 Codex rounds; plan/tasks/implementation reviewed by an internal adversarial multi-agent panel with refute-verification (Codex quota-locked and Gemini CLI sunset mid-pipeline; 12 confirmed code findings all fixed + closure-verified, incl. a nondeterministic adoption-lookup class proven empirically).Testing
internal/connect+internal/httpapigreen incl. no-raw-secrets suites (rotated keys, bearer/env secrets,?apikey=,user:pass@); full./internal/...green; linter 0 issues;make swagger-verifygreen.docs/features/connect-clients.md+ refreshed REST reference.specs/091-connect-client-form/verification/manual-protocol.md) — NOT RUN, needs a live app run.Deliberate follow-ups
selectionDisabledunavailable — keyboard skip of unsupported rows is 14+; model-side guard remains the backstop.