Observer verification: fix coverage bookkeeping, restart sessions when scope widens - #380
Open
Maximo-Guk wants to merge 7 commits into
Open
Observer verification: fix coverage bookkeeping, restart sessions when scope widens#380Maximo-Guk wants to merge 7 commits into
Maximo-Guk wants to merge 7 commits into
Conversation
The persisted observer record is the standing claim that a collaborator was verified for a producer -- `ensureObserver` reads it back on their next open and re-registers them off the account choice it holds, and `authorizeObservation` reads it from other turns. So a collaborator whose live re-verification just failed must not keep an entry saying they are covered: until now a revoked collaborator stayed "verified" until their next *successful* open. `fail()` now drops the failed gatekeeper from the persisted `accountChoices` synchronously with the failure determination, `getVerifier` moves inside the per-gatekeeper `try` so a verifier-acquisition rejection scrubs like any other refusal (and surfaces the descriptive denial rather than the raw RPC error, with no mid-flight `Promise.all` rejection to stale the rollback snapshot), and the terminal catch de-registers invalidated registrations alongside newly-added ones. That last part is a fail-open regression for a *returning* observer, marked with a TODO here and fixed in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The persisted observer record is meant to state what a collaborator's most recent open verified, and `ensureObserver` re-registers them off the account choices it holds. But a choice for a gatekeeper outside their current verification scope survived every open that could not check it: a "use" collaborator who opens while a connection is unbound from every gadget verifies nothing against it, yet their stale entry stays -- and the moment the connection is rebound (rebinding keeps the same gatekeeper id) the next open silently re-registers them off a choice made for a scope the workspace no longer has, instead of asking them again. Step 2 now drops every account choice for a gatekeeper outside the collaborator's live verification scope, even when the remaining scope is empty (that is exactly the everything-unbound open). The gatekeeper-side registration is deliberately kept: it preserves forward exclusion via `byObserverId`, and the next successful open's `addObserver` overwrites the verifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n fails. `ensureObserver`'s rollback removed the gatekeeper registrations of every binding that failed the call (`invalidated`), not just the ones the call created (`newlyAdded`). For a collaborator who was already an admitted observer, that de-registered them from a gatekeeper they had previously been verified against -- and a de-registered observer is one the gatekeeper stops naming in `ObservationDescription.excludeObservers`, so an observation it would have excluded them from is admitted with nothing left to block it. The coverage scrub this rollback accompanies is not a substitute for the registration. They cover different sets: `gatekeeper-confluence` -- the only in-repo producer of `excludeObservers` -- never marks an observation `prohibitAllSharing`, so for it the scrub covers none of the affected reads. The reachable sequence is a collaborator whose Confluence access is revoked upstream, whose re-open therefore fails, and whose pre-existing live session then watches the owner's agent read a page they cannot access. So roll back `invalidated` only on a first-ever verification, where the minted observerId is discarded along with the unpersisted record and a registration left behind would linger unresolvable. A returning observer's id is already persisted, so keeping their registrations is fail-closed (a registration can only add exclusion names) and the next successful open's `addObserver` overwrites the verifier. This restores the invariant `registeredBeforeCall` was introduced to state: roll back only what this call added. Coverage is still scrubbed either way, so a revoked collaborator's record stops claiming they were verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ens.
Authorization and observer verification run only at open(). Nothing re-ran
them when the set of gatekeepers a collaborator must be verified against
*grew* mid-session -- adding a connection, or binding one into a gadget --
so a collaborator who opened before the growth kept a live session holding
access they were never verified for.
Fix it with the mechanism already used to revoke a collaborator: generalize
scheduleRevocationRestart() to scheduleAccessRestart(reason) and add
#restartIfShared(), which flushes, waits 100ms and aborts the DO so every
client reconnects and re-opens against the new scope. It is a no-op when the
workspace has no collaborators, so a solo workspace is never disturbed.
Four sites widen scope and now restart: addGatekeeper, a permanent
bindWorkpiece, a merge that promotes a binding edge into "use" scope, and a
denied re-verification that scrubbed a persisted account choice. The merge
case compares the effective account-requiring "use" scope before and after
promotion rather than restarting on any promotion, since most merges promote
neither a gadget with bindings nor an edge to a connection anyone is verified
against. It reads the scope through the non-throwing gatekeeperVendorId()
rather than #inScopeGatekeepers("use"), whose observerVendorId() throws on a
legacy record with no creationSpec -- an unrelated legacy connection must not
turn an accepted merge into an error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`addGatekeeper` published the gatekeeper record before awaiting the gatekeeper's `describe()`, because `getGatekeeperFacet(id)` resolved the class from that record. The DO's input gate is open across the await and ids are allocated sequentially, so a live `build` session could guess the id and `getGatekeeperById()`/`openSession()` on the owner's brand-new connection -- which gates on nothing but record existence -- for as long as `describe()` took, all of it before `#restartIfShared` severed it. `getGatekeeperFacet` now optionally takes the class directly, so the record is published exactly once, after `describe()` resolves. Nothing a gatekeeper's `describe()` can reach calls back into the overseer to resolve itself by record, so no caller needs the early put. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
receiveExternalMessage() checked only the caller's role. Observer verification -- which is how a collaborator earns the right to see what the workspace has read -- runs at open(), so a "build" collaborator who never opened the workspace, or whose upstream access was since revoked, could still drive the agent and have it answer out of chat history and gadget storage. Extract the gate open() applies into authorizeCollaborator(): resolve the effective role from the permission graph, then run ensureObserver for that role. Both entry points call it. The external path passes requireRole: "build", so an insufficient role is denied before verification runs -- a "use" collaborator would otherwise be verified, or told to go fix a verification failure, for access this path can never grant them. It also passes no configureCb, since there is no channel to prompt on: an unverified caller is told to open the workspace in a browser, which is where configuration happens. roleRank is exported for the requireRole comparison, so it ranks rather than string-compares. Also has open() await ambient reconciliation before authorizing rather than between the role check and verification, which is where the two halves now join. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/observers.md gains a "Restarting when verification scope widens" subsection: the four triggers, why the merge trigger compares scopes rather than firing on any promotion, why shrinking scope and role rises are deliberately not triggers, why addGatekeeper's publication order is load-bearing under the restart, and where the enforcement moment actually falls for each trigger. Step 3 gains the record prune, the scrub-and-restart failure path and the returning-observer rollback rule; edge cases 3 and 5 are rewritten around them, and Step 6's justification for an orphaned entry is corrected -- a registration is what admits an open, so a stale one grants nothing on its own. docs/sharing.md renames scheduleRevocationRestart and documents the abort's second purpose, whose trigger is a grant rather than a revocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Maximo-Guk
force-pushed
the
observer-session-restart
branch
from
August 28, 2026 21:51
db563f7 to
4aa84ec
Compare
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| gatekeeperRecord.resourceTitle = description.title; | ||
| gatekeeperRecord.resourceUrl = description.url; | ||
| gatekeeperRecord.hasSlashCommands = description.hasSlashCommands; | ||
| this.storage.gatekeepers.put(gatekeeperRecord); |
There was a problem hiding this comment.
[P1] Keep the widened capability hidden until old sessions are invalidated
The record becomes collaborator-visible here, but the restart below still has to resolve getSharingManager(), sync storage, and sleep for 100 ms. During that interval an existing build collaborator can use the sequential ID to pipeline getGatekeeperById(id).openSession() and read from this brand-new resource without ever passing observer verification. Unlike the revocation delay, this is a first-time grant of access, so the grace window violates the observer invariant. Please gate access by the session generation / verified scope until reconnect, or otherwise invalidate collaborator sessions before publishing the record.
| #restartIfShared(reason: string): void { | ||
| this.getSharingManager().then(sharing => { | ||
| if (sharing.listCollaborators().length === 0) return; | ||
| return this.scheduleAccessRestart(reason); |
There was a problem hiding this comment.
[P1] Do not let this delayed abort overtake a later access mutation
A widening call can sync and enter the 100 ms wait, then a concurrent owner request can remove a collaborator: the permission edge is written synchronously, but that request awaits observer teardown/listing refresh before reaching its own scheduleAccessRestart(). The earlier timer can therefore call ctx.abort() first; because abort ignores the output gate, the later revocation write may be lost and the collaborator can become authorized again after restart. Coalesce these restarts and perform a final sync while preventing new access mutations immediately before aborting.
| // trip happened, since nothing persisted in the Workshop contains it. An implementation | ||
| // that merely checked the record would keep accepting him here. | ||
| await setVerifyOutcome(accountLabel(bobAccount), { allow: false, reason: DENIED_REASON }); | ||
| const revoked = await submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }); |
There was a problem hiding this comment.
[P1] Wait for the restart triggered by this denial
This failed re-verification scrubs Bob's persisted choice and schedules ctx.abort() about 100 ms later, but the test returns immediately and disposes its workspace/session. observer-reverification.test.ts documents that an abort landing after the last client leaves crashes local workerd; because these tests are concurrent, this can fail an unrelated sibling. Keep a probe alive and wait until it observes the disconnect (or otherwise settle the restart) before leaving the test.
|
Submitted 3 actionable inline review findings. |
Maximo-Guk
marked this pull request as ready for review
August 28, 2026 22:11
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+2230
to
+2232
| if (chatId === undefined && targetRecord.creationSpec && | ||
| "vendorId" in targetRecord.creationSpec) { | ||
| this.#restartIfShared("Gadget restarted because a connection was bound to a gadget."); |
There was a problem hiding this comment.
🟡 Rebinding an in-scope connection needlessly restarts the workspace
A permanent bind of a vendor-backed connection restarts every session, even when that connection is already in the use scope. Rebinding it into another gadget or under another name widens no one's scope, yet still severs the shared workspace — unlike mergeChatChanges, which compares scope before and after.
Prompt for agents
In bindWorkpiece (packages/workshop-backend/src/overseer.ts), the permanent-bind branch unconditionally calls #restartIfShared when the target gatekeeper has a vendorId. But a gatekeeper can already be bound by another gadget or under another name, in which case its id is already in #gadgetBoundGatekeeperIds()/#accountRequiringUseScope() and binding it again widens no 'use' collaborator's verification scope. The mergeChatChanges path avoids this by snapshotting #accountRequiringUseScope() before promotion and only restarting when the after-set contains a new id. Apply the same guard here: before the bind, check whether the target id was already present in the account-requiring use scope, and only call #restartIfShared if the bind actually adds it. This prevents needlessly severing every session on a shared workspace for a bind that changed nobody's scope.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Why:
Before this PR: We didn't sever existing observers, this was just actually documented in the original plan "existing observers see an incremental modal for just the new binding on their next open..."
The main gap we have right now is adding a binding does not restart live sessions, so an already-open collaborator is only verified against it at their next open.
What:
authorizeCollaborator() is now responsible for being the single gate for checking whether observers have access to underyling data. Everything routes through here, including
receiveExternalMessage()which previously only checked the role and never checked whether the observer had access to the underlying data.We also decided to abort the DO in order to revoke the RPC capabilities, we already use the same mechanism when revoking collaborators and we can just the same mechanism here. We made this a little less disruptive by checking graph first to see if there's any other collaborators before aborting the DO
Testing:
I added extensive integration tests in
observer-role-scope.test.ts,observer-reverification.test.tsandexternal-message-verification( even though it's actually used yet in any gatekeepers ) for testing the observer verification logic