Workspaces 6/10: Rust Workspace registry and stable dor refs - #619
Workspaces 6/10: Rust Workspace registry and stable dor refs#619nedtwigg wants to merge 12 commits into
Conversation
Each webview's store sees only its own Workspaces, so nothing could route a dor request naming a sibling window's, and workspace:<n> was a strip position that a reorder renumbered. Rust now holds the union: every window reports its list on change and the union is broadcast with a revision. Ids are minted only in Rust, workspace-<n> off one counter seeded above every id on disk and handed to webviews in blocks, so the ref workspace:<n> is the id's number and never changes; a host with no registry (VS Code, an older snapshot) still numbers by position. A dor request naming a Workspace or Window is routed to the window holding it, ahead of the caller's own Surface; one the registry cannot place falls through to the caller's window to be refused by name. The browser harness answers the same three commands for its one window. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PkPyEFCxiPo5UFeju5Ya9u
Deploying mouseterm with
|
| Latest commit: |
400a743
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://819d5901.mouseterm.pages.dev |
| Branch Preview URL: | https://workspaces-registry.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict. Mark the PR ready when you want the full review.
The registry, the Rust/TS split, and the coalesced reporting all read well. One design question and a set of concrete defects below.
A non-minted id now advertises a ref that resolves to a different Workspace. workspaceRefFor returns workspace:<n> for a minted id and a position for everything else, but resolveWorkspaceRef gives a number to the minted id first — so the two disagree whenever both kinds share a store. This is not only the pre-registry-snapshot migration case; it is plain VS Code, where DEFAULT_WORKSPACE_ID is literally workspace-1 (lib/src/lib/session-types.ts) and every other Workspace gets a random id. Two Workspaces, reorder the second to the front, and dor list --workspaces prints workspace:1 for both rows while workspace:1 resolves to the default and workspace:2 resolves to nothing. The new test an id the registry did not mint keeps a positional ref pins exactly that asymmetry as intended, so this is a decision rather than an oversight — but before the PR the positional scheme was at least self-consistent, and dor workspace close workspace:1 now has two plausible targets. The same shape reaches standalone through refillIdPool's .catch(() => {}): a failed reservation silently drops generateWorkspaceId back to a random id, which then takes a positional ref alongside minted ones. Making the fallback all-or-nothing — position only when no id in the store is minted — would restore the invariant that one ref names one Workspace.
The ambiguous-name story is documented as a refusal but isn't one. window_of returns None for a name two windows carry, which falls through to the caller's own window — and if that window holds exactly one Workspace by that name, resolveWorkspaceRef resolves it and the command runs. The doc comment, the a_target_routes_to_the_window_holding_it test comment, and docs/specs/standalone.md → "Workspace registry" all say the caller's window refuses with candidates. Local-wins is a defensible rule; it just isn't the one written down. Worth deciding which you want before the text settles.
The remaining items are inline.
… pool A non-minted id advertised a ref that resolved to a different Workspace: workspaceRefFor gave it a strip position while resolveWorkspaceRef gave a number to a minted id first, and in VS Code the default id is literally workspace-1 beside random ones, so a reorder made workspace:1 name two Workspaces. Refs are now positional only while no id in the Window was minted, and an id reads as minted only once a host has installed its pool; an unminted id beside minted ones has no number and is addressed by name, so one ref names one Workspace under either reading. A failed reservation used to fall back silently to a random id, which then took a positional ref beside minted ones. generateWorkspaceId now throws under an installed pool that ran dry, naming the in-flight or failed reservation, and the refill logs the rejection; the block (32) and its low-water mark (8) keep that unreachable in practice. The dor path answers the throw as an error response through the router's catch. Rust parse_target now mirrors POSITIONAL_WORKSPACE_REF, so a Workspace named 007 or 0 is a name, and workspace_reserve_ids floors the counter at 2 at the one point that mints, independent of whether setup seeded it. The ambiguous-name story is documented as it behaves: a name two windows carry falls through to the caller's window, which refuses a duplicate of its own and otherwise resolves its own. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RChsJ5rMUMyfu22UZDfUus
Carries the review fixes from the earlier stages up the stack. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RChsJ5rMUMyfu22UZDfUus
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict. Mark the PR ready when you want the full review.
All five earlier findings are addressed, and the all-or-nothing rule is a much better shape than the per-id one. Three things the new commit leaves open.
The all-or-nothing rule is per-Window, but routing is per-registry, so a positional ref still loses to a sibling Window's minted number. window_of's Target::Number branch matches ref_number(&entry.id) across every window's entries, and route's dor:controlRequest arm consults it before falling back to the caller's own window. A Window restored from a pre-registry snapshot has no minted id, so refsArePositional() holds there and dor list --workspaces prints workspace:2 for its second Workspace — while a sibling Window that has minted one holds workspace-2. dor workspace switch workspace:2 typed in the first Window routes to the second and acts there, with nothing in the output saying the target moved; dor workspace close workspace:2 --force is the same path with a worse ending. Two all-unminted Windows are the milder version: the number matches nothing in the registry, falls through, and resolves against whichever Window answered. Both are the state right after this build first restores two saved Windows, so it isn't only a hand-built corner. docs/specs/dor-cli.md's new "so one ref never reads two ways" and docs/specs/standalone.md's "routes to the window holding it" both read as if the rule were global. Either scope those claims to a Window, or make the rule global — the registry snapshot already carries every window's ids, so refsArePositional could be asked of it rather than of the local store.
The name an unminted ref falls back to is neither unique nor guaranteed non-numeric. workspaceRefFor now answers workspace:<name>, but renameWorkspace accepts any non-empty string: a Workspace named 3 advertises workspace:3, which parseWorkspaceRef reads as a position and resolveWorkspaceRef hands to the minted workspace-3 — one ref, two Workspaces, which is the case this commit set out to close. Two unminted Workspaces sharing a name advertise the identical ref, and the ambiguity message is built from workspaceRefFor too, so its candidate list prints that one string twice and can't disambiguate. Neither is reachable without a pre-registry snapshot, but "never reads two ways" is stronger than the code. Refusing a rename that reads as a position, or minting ids for a restored snapshot so nothing stays unminted, would close it; weakening the sentence is the cheaper option.
A failed first reservation now costs a fresh Window its session writer, where it used to cost only ref stability. installWorkspaceIdPool resolves even when the reservation rejects (the catch logs and returns), leaving registryInstalled true and the pool empty. installWindowPersistence then throws at its generateWorkspaceId() for a Window with no saved blob — before reaching installWindowSessionWriter, the one place it is installed. restoreWindowOrFresh catches, retries with null, throws in the same spot, and returns {}: the app renders on the default workspace-1 and never persists anything, recoverable only by a relaunch that reserves successfully. docs/specs/standalone.md → "Workspace registry" describes this as failing the create, which is what it does everywhere except the one path where the create is the boot. The precise rule looks like the one the spec already states elsewhere: a random id is only unsafe beside a minted one, and a fresh Window has none — suggestion inline.
# Conflicts: # docs/specs/layout.md
# Conflicts: # scripts/spec-word-budgets.json
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict. Mark the PR ready when you want the full review.
Scoping positional refs to "no registry at all" is the right shape, and resolving exact ids ahead of names closes both readings the last round raised. Two consequences of the new fallback, plus small items inline.
The opaque id is permanent, which makes it every upgrading user's ref, not just an outage's. generateWorkspaceId mints workspace-<uuid> only on a failed reservation, but workspaceRefFor gives workspace:<id> to any unminted id under a registry — which is every Workspace in a standalone snapshot written before this PR. Those ids look like workspace-k3j2h1z9-2, so after upgrading, dor list --workspaces prints workspace:workspace-k3j2h1z9-2 and that is what the user types at dor workspace switch, permanently: nothing renumbers a restored id, and the new test pins the same permanence for the reservation-failure case. The cheaper-looking alternative is to renumber at restore rather than to carry a third ref shape — the counter is already seeded above every id on disk (seed_next over snapshot_ids), so the numbers are free, and the ids are load-bearing in exactly two places that both read the same object: saved.workspaces[].id / saved.activeWorkspaceId in installWindowPersistence, and the records map seedWindowSession fills from that same snapshot. Remapping there, before setWorkspaces, would leave nothing unminted on a registry host and let workspace:<id> go away entirely. Worth deciding before the spec text settles, since it is the difference between two ref forms and three.
ref_for no longer has a None, and the contract downstream still promises one. Suggestion inline on the Rust side; the same edit is needed on RegistryWorkspace in standalone/src/workspace-registry.ts, whose ref field is typed string | null and carries the comment "null for one the registry did not mint" — unreachable after this commit. The two assert_eq!(ref_for(...), Some(...)) lines in refs_come_from_the_id_and_never_from_position need the wrapper dropped too. I can push that as a commit if you'd rather not hand-apply it.
One note on the standalone.md suggestion: that spec sits 3 words under its budget step, so the version inline is trimmed to fit (it drops the docs/specs/dor-cli.md → "Handle Model" pointer, which the Handle Model rule now states itself). Keeping the pointer needs node scripts/spec-lint.mjs --ratchet docs/specs/standalone.md in the same PR.
| - **Must allow boot and creation when reservation fails**, using opaque UUID | ||
| ids until the pool recovers; log the failure. Canonical refs follow | ||
| `docs/specs/dor-cli.md` → "Handle Model" (`workspace-store.test.ts`). |
There was a problem hiding this comment.
"until the pool recovers" reads as if the id recovers; it never does. A refilled pool numbers later creates, while the Workspace minted during the outage keeps its UUID — and so its workspace:<uuid> ref — for life, which is exactly what a failed first reservation still creates stable opaque refs and can recover asserts after the second installWorkspaceIdPool.
The pointer is also a bare file name, which AGENTS.md calls out directly ("a bare file name dodges the path lint and rots"); spec-lint passes on it today, so nothing catches the rot. The neighbouring bullet names its test (a_number_with_a_leading_zero_is_a_name), so the full path is the minimum here.
| - **Must allow boot and creation when reservation fails**, using opaque UUID | |
| ids until the pool recovers; log the failure. Canonical refs follow | |
| `docs/specs/dor-cli.md` → "Handle Model" (`workspace-store.test.ts`). | |
| - **Must allow boot and creation when reservation fails**, minting an opaque | |
| `workspace-<uuid>` id and logging it. **Never renumbers it**: that Workspace | |
| keeps a `workspace:<id>` ref for life | |
| (`lib/src/lib/workspace-store.test.ts`). |
| /// The stable `dor` ref of an id, when it has one. | ||
| pub fn ref_for(id: &str) -> Option<String> { | ||
| Some(ref_number(id).map_or_else(|| format!("workspace:{id}"), |n| format!("workspace:{n}"))) |
There was a problem hiding this comment.
Every id now has a ref, so the Option is dead: map_or_else already produces a String on both arms and the Some only re-wraps it. Leaving it means snapshot keeps emitting a field serde could still render null, which is what RegistryWorkspace.ref on the TypeScript side is typed and documented for.
| /// The stable `dor` ref of an id, when it has one. | |
| pub fn ref_for(id: &str) -> Option<String> { | |
| Some(ref_number(id).map_or_else(|| format!("workspace:{id}"), |n| format!("workspace:{n}"))) | |
| /// The stable `dor` ref of an id: its counter number, else the id itself. | |
| pub fn ref_for(id: &str) -> String { | |
| ref_number(id).map_or_else(|| format!("workspace:{id}"), |n| format!("workspace:{n}")) | |
| } |
refs_come_from_the_id_and_never_from_position needs its three Some(...) wrappers dropped to match.
| Workspace carries that name**, else the error lists the candidates. Both are | ||
| accepted bare (`2`, `build`), and **a ref that reads as a number is a ref**, |
There was a problem hiding this comment.
"Both" counted the two forms that existed before this commit; workspace:<id> is now a third, and it is the one a reader is least likely to guess is accepted bare.
| Workspace carries that name**, else the error lists the candidates. Both are | |
| accepted bare (`2`, `build`), and **a ref that reads as a number is a ref**, | |
| Workspace carries that name**, else the error lists the candidates. All three | |
| are accepted bare (`2`, `ws-a`, `build`), and **a ref that reads as a number is a ref**, |
Rust holds the union of every window's Workspaces; ids are minted only in Rust (
workspace-<n>off one counter seeded above every id on disk, reserved in blocks), soworkspace:<n>is stable across reorders and moves. Adorrequest naming a Workspace or Window routes to the window holding it.Based on
workspaces-harden.🤖 Generated with Claude Code
https://claude.ai/code/session_01PkPyEFCxiPo5UFeju5Ya9u