Resparkable space switcher - #81
Open
JohnD-EE wants to merge 9 commits into
Open
Conversation
…ting scopes
The switcher needs somewhere to put the active workspace, and §24.2 only says
where it must not be ("never in a cookie alone"). It goes in the URL as
?space=, with absence meaning personal: a path segment would restructure 27
pages and turn every existing bookmark into a redirect to buy a prettier URL,
and a cookie would survive a back button, a second tab and a pasted link, which
is exactly how a thought lands in the wrong brain.
Absence has one spelling. Not ?space=personal, and not the owner's user id: the
first is a magic value every reader has to know about, the second puts a user
id in every access log and referrer header. resolveActiveSpaceScope still
accepts the user id, because a personal space's key genuinely is that value
since phase 45, but nothing generates it.
A repeated ?space=a&space=b resolves to nothing rather than to the first value.
Two answers to "which brain" has no sensible default, and choosing one is a
coin toss whose losing side is a cross-space read; resolving to personal makes
it fail as a 404 on the caller's own space.
requestSpaceScope() is the one line a route will write from the next commit on,
and it throws NotFoundError rather than returning null because there is exactly
one thing fifty callers would do with the null, and a helper that makes each of
them write their own 404 eventually meets the one that writes a 403.
The direction of travel is that the greppable trust boundary gets shorter:
`rg 'spaceScope\(|spaceScopeFor\('` returns fifty route files today, each an
unaudited mint, and will return services/membership.ts plus the background
paths that have no session to read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
Mechanical, and one line each: `spaceScope(session.user.id)` becomes
`await requestSpaceScope(request, session.user.id)`. What changes is not the
routes but where the decision lives. Before this commit every route file was a
place a scope could be minted for the wrong person and nobody would notice;
after it, `rg 'spaceScope\(|spaceScopeFor\('` returns seventeen lines across
thirteen files, which fits on a screen and can therefore actually be read.
Nothing changes for a user in no group: a request with no ?space= short-circuits
to the same personal scope the tier has minted since Release 1, and does so
without a membership read, which matters because that is nearly every request
the product serves. The existing route suite passes untouched, which is the
evidence for that claim rather than a comment about it.
The handlers.ts docblock claimed the scope "comes from the session, always".
That was true and is now half true: the actor comes from the session and the
workspace comes from the URL, and the point of the file is that membership
decides whether the two go together. A stale docblock on the file that mints
scopes for the whole CRUD surface is worse than none.
Three routes are deliberately left on the personal space:
- vault/export and vault/import, because §24.5 has not settled what exporting a
group workspace means. An archive of a shared brain is several people's
content in one person's download, and an import into one writes somebody
else's rows under the importer's name. Guessing there is a data-protection
decision, so the question stays visibly open in a comment rather than being
silently answered by a one-line edit.
- capture, which moves in its own commit, to an explicit target rather than to
the ambient one.
chat/stream's spend tap was typed `ReturnType<typeof spaceScope>` and logged
the space id under a `userId` key. Both were accurate when a space was a person.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
Two halves of one problem, answered in opposite directions because a server page cannot read its own URL and a client component can. On the server, readResparkable() takes the space as a REQUIRED third argument. Optional would have been kinder to write and is the wrong choice: a page that forgets renders perfectly, the switcher says "Study Group B", and the content is the reader's own. No error, no empty state, nothing to notice. Required hands that check to the type checker, so a new page does not compile until its author has answered "which workspace is this for", and null is a visible answer rather than an omission. Twenty pages thread it from searchParams; two pass null on purpose. /shared is one of those two. It is keyed on the READER, not on a workspace: §13's grants match a grantee's address, so what is shared with somebody does not change when they switch workspace, and narrowing that list by the active space would hide half of it with no way to tell. Phase 49 makes it per-space, when a group can be a grantee and "shared with Study Group B" becomes a different question. On the client, twenty-one components fetch through a tier-owned wrapper that reads window.location.search at call time. Not a context, a module variable or a cookie: each of those three can outlive the navigation that changed the workspace, and a request built from a stale one reads the wrong brain while looking entirely normal. The URL is the only one that cannot go stale, which is why §24.2 put the workspace there. The wrapper turned get(path) into get(path, undefined), which is the same call to the network and a different call to a test: forty-nine assertions noticed. Fixed in the wrapper rather than in the assertions. A pass-through that rewrites the shape of every call in the tier is not a pass-through, and the tests were right to say so. Quick capture and the Sparkey composer are deliberately NOT on the wrapper. They post to /thoughts today, which now follows the ambient workspace, and that is exactly the stickiness §23.4 forbids. They move to an explicit target in the capture commit; leaving them on apiClient keeps the bug visible rather than half-fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
…rkspace The switcher is four lines of behaviour: a switch is router.push to the same page with a different ?space=. Not a state change, not a fetch, not a context update. That one decision is what buys the three properties §24.2 asked for without building any of them: a workspace is a link, the back button walks between workspaces, and two browser tabs can hold two different ones. Every other search param survives a switch, because switching workspace while looking at Tuesday should still be looking at Tuesday. Switching back to personal REMOVES the param rather than setting it to the user id, so the URL is the one the product had before groups existed and every old bookmark still resolves. And with one workspace it renders nothing at all: a switcher with a single entry costs header space to say what the header already says. The larger half of this commit is the unglamorous one. A switcher that works and navigation that drops the param is the worst outcome available: the user switches, clicks the first link, and is silently back in their own brain looking at a page that loads perfectly. So eight router.push sites and both render-time hrefs now carry it. Those two cases read the workspace from different places, and the split is not a preference. An imperative call (push, fetch) reads window.location, which is the freshest answer at the moment somebody acts. A rendered href reads useSearchParams(), because it is server-rendered first and an href that gained a ?space= only after hydration is a mismatch React logs and a link that means two different things depending on how fast you click it. Tab state is per workspace by adding a segment to its localStorage key, and needs no migration: personal keeps the unsuffixed key so existing layouts survive, and a group's key has never been written, so it reads as no state, which is a fresh tab set, which is the right answer for a workspace you have not opened. Keyed on the space id and not the group name, so a rename does not lose somebody's panes. The layout fetches again, for the first time since the shell cutover deleted the counts read. It is the one Resparkable read that belongs in a layout, for a reason specific to layouts: a layout is handed no searchParams and cannot know the active workspace, and this list is the only read in the tier that does not need to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
…you are in The acceptance criterion for this phase, and the reason it is a set of per-path assertions rather than one: the failure is silent. A thought lands in a shared brain because the last workspace was sticky, the capture succeeds, the box clears, and four other people can read it. Nothing goes wrong on screen, so the only thing standing between the product and that outcome is an assertion per way in. So the two directions get opposite defaults. A surface displaying a workspace reads ?space= from the URL; a path that creates a thought does not read it at all. POST /capture takes the target as a body field, defaults to personal when absent, and ignores ?space= even when the caller really is a member of the space it names. That last case is the one that would have passed by accident if the route merely preferred the body over the URL. The UI had drifted from the plan: quick capture and the Sparkey composer posted to /thoughts, which is the ordinary CRUD create and now follows the ambient workspace. Both move to /capture, which the plan's table said all along. They stay on apiClient rather than the space-carrying wrapper, and that is the point rather than an oversight to tidy up later. The target resets to personal after every save and is React state rather than localStorage, unlike Sparkey's mode beside it. A remembered mode is a preference; a remembered capture target is a loaded gun pointing at somebody else's brain three days later. It does survive a failed save, because the text comes back into the box and a retry that silently re-aimed would be a worse answer than the failure was. Sensitivity warns and never filters. The same pure keyword pass the server runs, reused client-side so the warning appears while the words are still in the box. It does not block: the classifier is broad and wrong often enough that refusing would train people to route around it, and somebody telling their group about their week is not making a mistake. No query reads it to hide a row, because that is §23.4's forbidden per-row ACL arriving through a side door, one well-meaning where clause at a time. Two of the plan's six paths cannot write a thought in this codebase at all: /transcribe and /transcribe/image return text into the capture box rather than creating anything. That is what makes them safe, and the test says so rather than asserting a default they do not have. Document upload is the boundary case and goes the other way: it follows the ambient workspace like every other CRUD create, because a document is added TO a workspace you are looking at while a thought is caught and then aimed. It bypasses the client wrapper for its progress events, so it needed the workspace adding by hand or it would have written to the uploader's own space from inside a group. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
…is told Sparkey is a permanent pane, so the moment a member opens a group workspace the agent layer is live in it. A companion that answered about the actor's own goals inside somebody else's brain would be the most confusing thing in the release, so §23.9 cannot wait for a later phase. The context contributor used to ignore its `id` argument outright and read only `request.userId`. That was the correct rule while a person had one brain: trusting a client-supplied contextId would have rendered one person's goals into another's prompt, and buildContext caches on type:id:userId, so it would have served that repeatedly. A group workspace makes ignoring `id` the wrong answer, because the context a member needs there is the group's and an actor id cannot express it. So `id` is now read and RESOLVED. The old guarantee survives by a different road: a forged contextId yields no context rather than somebody else's, and it no longer depends on the chat route having pinned the field, which is what made the original rule necessary. The cache key needs no change, because it already carries the space in `id`; two members of one group get two entries for identical content, which is a wasted slot rather than a leak, and collapsing it would mean dropping the actor from a key whose job is keeping brains apart. requireResparkableUser becomes requireResparkableSpace and becomes async, because verification needs a read. Its two routes now differ in kind rather than only in source. A person acting through an agent: the actor is the session's and the space is a HINT, which core says plainly by routing ChatRequest.scope through hintScope, so membership decides. A background run with no actor: the schedule's scope column is admin-written and there is nobody to check it against, so the carrier is still the authority, exactly as before. Route 1 reads only the new `resparkableSpaceId` key and not the both-keys reader, and that is load-bearing rather than tidy. The legacy `resparkableUserId` is a pre-migration schedule carrier; read as a space target it would send a live user's turn through group resolution against a stranger's id and fail the turn, where the old code simply ignored a value never meant for it. The test that asserted "prefers the session user over the scope" now says which key it is about. A hint naming a space the actor is not in refuses rather than falling back to their personal brain. Falling back sounds gentler and is worse: a model that read "use workspace spc_x" in a document could redirect a turn, and the person would get an answer about their own notes with nothing to show it had been aimed elsewhere first. The tightening this still wants is a scopeIsAuthoritative check on route 2. It is not added blind: getting it wrong silently stops every 04:30 run, and it needs a test proving the scheduler sets the flag before anything should gate on it. Named in the code rather than left as a gap somebody has to rediscover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
Phase 46 left a comment in routes.ts saying this wiring was phase 47's, because adding one static route obliges the whole checklist in the same change and three coverage tests fail until every part of it is done. So: routes, nav-groups, section-help, payloads, tab-registry, change-scope, two tab adapters, two pages, two loading states. The section sits under Manage rather than Organise, and that placement is the design rather than a filing decision. Working IN a group is a workspace switch in the header, which keeps you on whatever page you were looking at; this section is the administrative half, a place you visit occasionally and on purpose to change who has access. Same distinction that put "Shared by me" under Manage and "Shared with me" under Organise. "Open" is a plain link carrying ?space=, not a tab. Opening a group as a tab would seat a group's Today inside the personal workspace's pane tree, which is precisely the confusion the URL-carries-the-space design exists to prevent. Members and invitations are two lists because they are two objects with two lifetimes, which is phase 46's third table showing through to the UI. The sentence saying an invitation grants nothing until it is accepted is the only thing on screen explaining why, so it is asserted rather than decorative. Member rows name user ids and never addresses. Every member can see who else is in a group, which §23.4 makes unavoidable and correct; handing out everybody's email is a separate decision nobody made, and the member route does not return one. Invitation rows DO show the address, because an outstanding invitation is addressed to a mailbox and nothing else, so an admin who cannot see it cannot tell what they invited or withdraw it. Both views mutate through apiClient rather than the space-carrying wrapper. These routes are keyed on the actor and the group id and read no ?space=, so appending one would imply a relationship that is not there. Asserted, because the next person to touch this file will reasonably assume the tier's own client is the right one. Deleting a group and admin succession are absent on purpose. Both are phase 48's and both need more than a button: a typed confirmation and a notification to every member, because deleting a group destroys a workspace several people were writing into. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
…rors plan.md §15 row 47, the README's status list, CHANGELOG's Unreleased section, ui.md's add-a-surface checklist, and a "what actually shipped" section in the design doc recording where building it disagreed with planning it. The six departures, in short: two of the plan's six capture paths cannot write a thought at all here (transcribe returns text into the box, which is what makes it safe); quick capture and the Sparkey composer had drifted onto /thoughts and moved back; resolveSpaceScope() is three functions, and the third is the short-circuit that keeps a membership read off every request in the product; the outbound-sharing routes follow the ambient workspace, because leaving them personal would 404 "share this project" inside a group; vault export and import stay personal because §24.5 has not settled what exporting a shared brain means and both answers are data-protection decisions; and the capability layer's background route still trusts its carrier, with the tightening named in the code rather than guessed at. The layout is force-dynamic. Adding a fetch to it meant Next's static probe reached that read on every route under /resparkable, `cookies` made the probe bail, and readResparkable caught the bail and logged it as a failed read: twenty-one errors per build, all of them lies. Every route here was already dynamic; the line just says so, and a real error in that log is worth noticing again. Verified by a clean build, with /resparkable/groups and /resparkable/groups/[id] in the route table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
A group viewer could still issue/update/revoke grants, send grant invites, and mint/revoke share links, since those services checked entity ownership but never the caller's role. Check permissionsFor(scope.role).write first so viewers are turned away before any row is touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS
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.
Summary
issueGrant/updateGrant/revokeGrant,sendGrantInvite, andmintShareLink/revokeShareLinkchecked entity ownership but never the caller's role, so a groupviewercould still perform write actions. Each now checkspermissionsFor(scope.role).writefirst.Test plan
npm run validatenpm run test:changedviewermember cannot issue/update/revoke grants, send invites, or mint/revoke share links🤖 Generated with Claude Code
https://claude.ai/code/session_01DnSpQpnUwasiE85eF3FdYS