Skip to content

Commit a86178a

Browse files
committed
docs(planning): record PR #8 architecture review and stale-doc pointers
Documents the client_id vs machine_id dual-axis binding scope finding from the PR #8 review, corrects two "out of scope" decisions that shipped anyway (007de1f), and adds a Future TODOs list (identity model, oversized files, doc hygiene). Flags stale Status/Branch fields in deny-by-default-bindable-callers.md and projects-grouped-machine-cards.md with a pointer back to the review doc. Signed-off-by: crimsonsunset <jsangio1@gmail.com>
1 parent f40fdbb commit a86178a

3 files changed

Lines changed: 55 additions & 4 deletions

File tree

docs/planning/deny-by-default-bindable-callers.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
**Depends on:** `projects-grouped-machine-cards.md` + `workspace-binding-project-adopt.md` merged (one-card-per-path `Entry.bindings[]`, `EntryKind`, `machine_id` on bindings, 3-tier resolver)
77
**Unblocks:** A folder/client gets **zero** backend tools until it has an explicit representation on the Projects/Clients page — and the whole feature stops depending on the now-deprecated MCP `roots` primitive
88

9+
> ⚠️ **Status/Branch stale as of Jul 1, 2026** — this shipped on `feat/workspace-machine-binding` (Jun 29 commits), not on a separate `feat/deny-by-default-routing` branch. See [`workspace-machine-binding.md`](./workspace-machine-binding.md#future-todos-from-pr-8-review-jul-1-2026) Future TODOs.
10+
911
---
1012

1113
## Problem

docs/planning/projects-grouped-machine-cards.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
**Depends on:** `feat/workspace-machine-binding` fully merged (migrations 033/034, machine CRUD, `machine_id` on bindings)
77
**Unblocks:** Full homelab multi-box workflow — same project path on Gondor and Rohan each routing to a different FeatureSet, both visible at a glance
88

9+
> ⚠️ **Status/Branch stale as of Jul 1, 2026** — this shipped in-branch on `feat/workspace-machine-binding` (Jun 25 commits), not on a separate `feat/projects-grouped-machine-cards` branch. See [`workspace-machine-binding.md`](./workspace-machine-binding.md#future-todos-from-pr-8-review-jul-1-2026) Future TODOs.
10+
911
---
1012

1113
## Problem

docs/planning/workspace-machine-binding.md

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,38 @@
11
# Workspace Machine Binding
22

3-
**Last Updated:** Jun 30, 2026
4-
**Status:** In progress — Phases 1–4 largely landed; per-device header for tunneled multi-device routing landed Jun 30, 2026
3+
**Last Updated:** Jul 1, 2026
4+
**Status:** In progress — Phases 1–4 largely landed; per-device header for tunneled multi-device routing landed Jun 30, 2026; PR #8 (this branch) reviewed Jul 1, 2026 — see architecture review below
55
**Branch:** `feat/workspace-machine-binding` (off `dev-rebased`)
66
**Depends on:** `dev-rebased` label/icon port complete (migration 032 landed)
77
**Unblocks:** Per-machine project organization and override routing; homelab multi-box workflow
88

99
---
1010

11+
## Update — Jul 1, 2026 (PR #8 architecture review — client_id vs machine_id)
12+
13+
PR #8 review surfaced that `machine_id` didn't replace the pre-existing `client_id` scoping (migration 027) — it stacked on top of it. `WorkspaceBinding` now carries **two independent optional scope fields**, and the resolver reconciles both instead of one:
14+
15+
- `client_id` — OAuth app identity (Cursor, Claude Desktop, …), pre-existing.
16+
- `machine_id` — physical host identity, this feature.
17+
18+
`find_exact_for_machine` handles all four combinations (client+machine, machine-only canonical, client-only legacy, global), with client+machine taking priority over machine-only:
19+
20+
```322:346:crates/mcpmux-storage/src/repositories/workspace_binding_repository.rs
21+
async fn find_exact_for_machine(...) -> Result<Option<WorkspaceBinding>> {
22+
let bindings = self.list().await?;
23+
// Client+machine scoped binding takes priority over machine-only canonical.
24+
...
25+
}
26+
```
27+
28+
This directly contradicts two "Out of scope" decisions below (marked ⚠️ **STALE** — see Scope section) that assumed client+machine combined scoping would never be needed. It shipped anyway (`007de1f`, Jun 27) to fix bindings created via the workspace-needs-binding popup that had both `machine_id` and `client_id` set and were invisible to the old machine-only lookup.
29+
30+
**Not a bug** — traced the priority order end-to-end, it's internally consistent and tested (`test_machine_id_round_trip` etc). But it means the OAuth client-identity system and the machine-identity system are two separate trust/scope axes stacked together with a compatibility fallback (`if local_machine_id.is_none() { find_exact_for_roots... }` for installs that haven't adopted machines yet), not one unified caller-identity concept. See **Future TODOs** below.
31+
32+
Full PR review + architecture discussion: [PR #8](https://github.com/crimsonsunset/mcp-mux/pull/8).
33+
34+
---
35+
1136
## Update — Jun 26, 2026 (Settings Machine Identity, remote web admin)
1237

1338
Remote web admin (`mux.joe-hassio.com` → static SPA on `:45819`) now uses a **viewer-only main card** in Settings → Machine Identity:
@@ -93,8 +118,8 @@ Two things break down:
93118

94119
| Item | Reason |
95120
| ---- | ------ |
96-
| Per-client + per-machine combined scoping `(client_id, machine_id, workspace_root)` | Three-dimensional uniqueness adds complexity with no current use case. Revisit if per-client routing is ever fully wired. |
97-
| Machine-aware `client_id` scoped bindings | Client-scoped bindings are not currently used in production routing. Keep machine scope orthogonal for now. |
121+
| ~~Per-client + per-machine combined scoping `(client_id, machine_id, workspace_root)`~~ | ⚠️ **STALE — shipped anyway.** `find_exact_for_machine` matches client+machine scoped bindings before falling back to machine-only canonical (`007de1f`, Jun 27) — the workspace-needs-binding popup writes both fields and needed a lookup path that could see them. See Jul 1 review update above. |
122+
| ~~Machine-aware `client_id` scoped bindings~~ | ⚠️ **STALE — shipped anyway.** Client-scoped bindings are the fallback path (`WorkspaceBinding::new_scoped_multi`) when no machine identity is available at bind time (`bind_workspace.rs`). Machine and client scope are not orthogonal in the current resolver. |
98123
| Cross-machine DB sync / export | Each McpMux install has its own SQLite. Syncing machines across installs is a future cloud/sync feature. |
99124
| Machine icon upload from web admin | Icon upload requires a file picker; follow the same deferral as base-dirs on web (text/emoji only for now). |
100125
| Automatic hostname detection | Querying `hostname` at runtime is easy but creates a false sense of automation — the user should consciously name their machines. Pre-fill the hostname field from `gethostname()` as a hint only. |
@@ -293,3 +318,25 @@ Clicking "Set up" opens a small modal: name field (required), icon (optional), h
293318

294319
- [`dev-rebased-post-port-completion.md`](./dev-rebased-post-port-completion.md) — label/icon port this feature builds on (migration 032)
295320
- [`dev-to-main-port.md`](./dev-to-main-port.md) — broader port history; Phase 7 workspace binding metadata work
321+
322+
---
323+
324+
## Future TODOs (from PR #8 review, Jul 1 2026)
325+
326+
**Identity model:**
327+
- [ ] Decide whether `client_id` scoping and `machine_id` scoping should be unified into one caller-identity concept, or formally documented as two intentionally-separate axes (client = app, machine = host) with the current four-combination priority as the permanent design. Right now it's implicit — nobody decided it, it accreted across migrations 027 → 033–035 → `007de1f`.
328+
- [ ] If unified: evaluate whether `WorkspaceBinding.client_id` can be deprecated in favor of always deriving scope through `inbound_clients.machine_id`, now that every OAuth client can be assigned a machine. Blocked on: pre-existing client-scoped bindings in the wild would need a migration.
329+
330+
**File size (deferred twice already — `workspace-binding-project-adopt.md`, `sidesheet-panel-identity-header.md`):**
331+
- [ ] `WorkspacesPage.tsx` (2014 lines, 9 components in one file) — extract `EntryCard`, `EntryCardRoutingTable`, `EffectiveFeaturesContent`, `MachineRegistrationModal` into their own files.
332+
- [ ] `SettingsPage.tsx` (2052 lines, one 1436-line `SettingsPage()` function) — extract `MachineIdentitySection` and other settings sections out of the main component.
333+
- [ ] `workspace-binding-panel.component.tsx` (1296 lines) and `workspace-binding-form.component.tsx` (996 lines) — both born oversized; split by section (identity header / routing fields / scope fields) now while the code is fresh, before the next feature adds to them.
334+
335+
**Doc hygiene:**
336+
- [ ] `deny-by-default-bindable-callers.md` still says `Status: Planning — ready to implement` and `Branch: feat/deny-by-default-routing` despite shipping on this branch Jun 29 — update status/branch fields.
337+
- [ ] `projects-grouped-machine-cards.md` planned a follow-on branch that never happened (landed in-branch Jun 25) — same cleanup.
338+
339+
**Lower priority / tracked, not urgent:**
340+
- [ ] `find_exact_for_machine` / `find_exact_global` do an in-memory `list()` scan per lookup per root — fine at homelab scale, revisit if bindings scale past "tens."
341+
- [ ] `feature_set_repo` field on `FeatureSetResolverService` is `#[allow(dead_code)]`, kept for constructor API stability — either finish removing it or drop the "temporary" framing in the comment.
342+
- [ ] No CI ran against this branch (fork, Actions not enabled) — fine for now per solo-project workflow, but the gap grows with PR size like this one.

0 commit comments

Comments
 (0)