feat(gatekeeper-kit): replayable runs with account-adjudicated expiry - #441
feat(gatekeeper-kit): replayable runs with account-adjudicated expiry#441ndisidore wants to merge 6 commits into
Conversation
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Gatekeeper-kit tests pass: 343 tests. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 2 actionable inline findings. |
|
No new findings. Previously published inline findings remain applicable to the current head. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
No new findings. Previously published findings remain applicable to |
Not worth actioning. The window is the notify RPC only; fresh()'s #overtaken already covers the wide mint-await window. The delta is error flavor for one caller — it sees expired instead of the reconnect that landed mid-notify — and its re-entry refetches the live credentials. No false notify is created (the grant was confirmed dead when notify fired; the latch handles dedup).
The scenario needs a triple coincidence: 401 + a concurrent fetch adopting a successor inside the ask window + the report reply lost (fail-closed "expired"). Net damage: the one reporting caller gets CredentialsExpiredError instead of CredentialsChangedError. Consider this accepted |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Tests were not run because |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. Current GitHub build, test, and lint checks pass; local tests were unavailable because |
This comment was marked as outdated.
This comment was marked as outdated.
|
LGTM! |
c288f8b to
6754a17
Compare
|
LGTM! |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Empty vendor IDs lose log attribution
An empty vendor ID makes defineActions discard the configured attribution. Both credential helpers and observer tracking repeat this check, leaving their logs unattributed.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
LGTM! |
| } | ||
| this.#logger.error("malformed credential rejection verdict", { | ||
| event: "credentials.rejection.verdict.malformed", | ||
| error: new Error(`unexpected verdict: ${String(verdict)}`), |
There was a problem hiding this comment.
P2 Do not log the malformed verdict value
This fail-closed branch copies an arbitrary out-of-contract RPC result into the logged error message. If a hand-written account implementation accidentally returns a provider response or bearer string instead of a verdict, that credential is persisted in server logs, contrary to the logging rule forbidding tokens and response bodies. Log only static/type metadata, and add a test that a malformed secret-like value is absent from the logged event.
|
Posted 1 actionable inline finding. Tests were not run because |
| // Dual guard, neither subsumes the other: the fence blocks fetches started before an expiry | ||
| // "" is reserved for a never-connected read: adopting live credentials under it would wedge | ||
| // every rejection as retryable, since "" always adjudicates superseded. | ||
| if (current.identity === "") { |
There was a problem hiding this comment.
P1 Clear the previous cache authority before rejecting this read
After a successful id-a/gen-a fetch, this source may hold cache entries partitioned by gen-a. If the account reconnects as B and a hand-written stub accidentally serves B under the reserved empty identity, this branch throws but leaves #generation === "gen-a"; a later cache-first request can therefore return A’s data to B without another credential fetch. Since this response cannot establish which principal is current, clear/fence the authority before throwing, and extend this test to start from an established partition.
|
Posted 1 actionable inline finding. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
d521cdd to
afcf9ed
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
CredentialSource.run gains a replayable option: a credential rejection is retried once with credentials minted through a refreshCredentials channel, and only a rejection of those reports expiry — so derived-bearer ports stop reporting routine stale bearers as grant death. The flag without a channel throws at the call. The refresh is observed, never adopted: plain reads stay the snapshot's only writer, which removes the second concurrent writer earlier revisions fenced around. noteCredentialsExpired now returns the account's verdict on the reported identity — an explicit false resolves as the fixed retry message with the cache authority dropped; anything else (lost answers included) fails closed as accepted. The verdict is asked first, then clear and fence land as one synchronous transition, and a reconnect crossing the refresh fences any authority not adopted past it. Replays coalesce per rejected read via SingleFlight's new object keys. Plan §4.6/§4.13/§5.6/§5.8/§6 reconciled with the reduced model.
afcf9ed to
3feafe3
Compare
| [Symbol.dispose](): void { | ||
| if (this.#disposed) return; | ||
| this.#disposed = true; | ||
| this.#dispose?.(); |
There was a problem hiding this comment.
P2 Defer owned-resource disposal until submitted cursor calls settle
A client can issue its final next() and immediately dispose the cursor stub while retaining the returned RPC promise. Cap’n Web releases the target when the stub is dropped, so this hook can dispose the callback-owned RPC stub while that fetchPage() is still using it, making the already-submitted page fail. Queue the cleanup behind #queue (which synchronously claims each submitted call), and add a gated test covering disposal with running and queued next() calls.
|
Posted 1 actionable inline finding. |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // An unadjudicated report never dead-marks: a transient account outage must not retire a | ||
| // possibly-live identity for the rest of the activation, nor invent an expiry the account | ||
| // never confirmed. | ||
| this.#supersede(answer === "expired" ? identity : undefined); |
There was a problem hiding this comment.
🟡 Concurrent credential retries invalidate each other
Concurrent rejections sharing one account verdict each call #supersede, invalidating sibling refetches after the shared heal. Some replayable calls then fail instead of retrying.
Prompt for agents
CredentialSource.#verdict coalesces the account report with #asks, but every caller awaiting that shared report independently mutates #clearFence and forgets the credential flight after the answer. With simultaneous rejections for one identity, one caller can open its post-heal #current() fetch before another waiter calls #supersede(), so the later waiter fences out and forgets the first fetch. Preserve one shared local verdict transition per identity, or otherwise ensure all waiters apply the transition before any can start the retry refetch. Add a deterministic test that parks the post-verdict getCredentials call while a sibling verdict waiter resumes, and require both replayable operations to retry from one shared successor fetch.
Was this helpful? React with 👍 or 👎 to provide feedback.
| */ | ||
|
|
||
| import { env } from "cloudflare:test"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; |
There was a problem hiding this comment.
P1 Restore a clean lint run
This import leaves vi unused, and line 13 also leaves submissions unused; src/observers.ts:7 similarly imports unused GitCache. The current CI Lint job fails on exactly these three errors, so this head cannot merge until the unused imports are removed.
|
|
||
| // Completing it, after the exchange: | ||
| const claim = claimOAuth<{ startedUnder: string }>(kv, oauthNonce, Date.now()); | ||
| const grant = await exchangeCode(code); |
There was a problem hiding this comment.
P2 Reject a stale callback before exchanging its code
claimOAuth() returns null for an invalid, expired, or replayed nonce, but this canonical example calls exchangeCode() first and dereferences claim only afterwards. A consumer that follows it with a non-null assertion can mint an orphaned provider grant for a callback the account has already rejected. Check claim === null before the token exchange, and cover that a stale nonce never invokes the exchange.
|
Posted 2 actionable inline findings. |
A second audit read the kit as a new gatekeeper author would. Five places advertised a safety property the callbacks could not deliver, or narrowed a canonical contract until the high-level path had to be abandoned. - Actions distinguish a terminal failure known to have left no provider effect from one whose outcome is unknown, and the dependency cascade consults the resolution oracle apply already uses instead of retiring dependents whose reference the provider had bound. - `describe` can declare `pushedCommits` again, with the compiler forcing every future `ActionDescription` field into a kit-owned or provider-owned list. - Provider-backed cursors authorize the exact page they return, including one served from the buffer and the terminal answer of a walk that disclosed nothing -- a zero-result search is an existence oracle. - Observer strategies declare whether they can enforce collection ACLs, and the gate refuses a scope a strategy would silently ignore. - `ObservationGate` reaches the git cache, so a gatekeeper returning commit ids no longer needs a raw queue stub. - Journals and caches require a named keyspace, with an explicit legacy opt-in. Carries two changes made alongside it: `KvTtlCache.partitionedBy` now reads a live `cacheAuthority()` that only vouches for credentials the source still stands behind, and `KvScannable.list` gained storage-level page bounds.
Every existing suite tests one leaf. This one builds a gatekeeper from all of them at once against a fake provider -- account Durable Object on CredentialCoordinator and the connect handshake, resource facet on CredentialSource, ObservationGate, defineActions, KvTtlCache and TokenCursor -- because the contracts a new consumer trips over only appear in assembly. It runs in workerd with real Durable Objects, so persisted stubs, RPC boundaries and storage are genuine. Sixteen tests cover OAuth completion and stale-attempt refusal, repeated credential rotation, tracked child ACLs, a zero-result cursor, a non-idempotent action whose provider outcome is unknown, provisional dependencies, git-cache access, cache repartition on reconnect, action fencing and journal namespace isolation. It found a defect in the API it exercises: `ObservationGate.getGitCache()` was annotated `Promise<GitCache>`, so `using` on the returned caller-owned stub did not compile although the doc told consumers to use it. The return type is now the queue stub own, which keeps it `Disposable`. Also records two things a consumer cannot do, both learned the hard way here: a plain object passed as the approval queue crosses RPC as call-scoped stubs that are disposed when the call returns, and the kit stateful objects are Durable-Object-local by construction -- `perStorage` keys coordination on storage-object identity, so a journal or gate that crossed a boundary would have lost that even if it serialized.
…n, rename sets to collections
Three API reshapes that are free now and impossible after the first adopter.
An action fence was an optional argument at every call site, so omitting one was
invisible: the gatekeeper works, its tests pass, and an action approved under one
provider account later applies under the next. `defineActions` now requires a
set-level `fence` policy with per-kind `fenceOverrides`, and `submit` refuses a
fenced kind staged without one. The policy is `"authority" | "none"` rather than
naming a connection: the journal never interprets the value, so a provider whose
actions should survive re-authorization of the same account fences on a stable
account id instead. The kit still cannot capture the fence -- it must ride the
staging operation own `CredentialRead`, since a second read taken inside `submit`
could land after a reconnect.
`claimOAuth` consumes its nonce before the provider token exchange, so a revoke
or newer reconnect can land while that exchange is in flight and be overwritten by
the older completion. `connect(credentials, { ifGeneration })` compares the
connection the attempt started under -- captured through `advanceToOAuth` metadata,
which already carries it -- and throws `ConnectionSupersededError` rather than
storing a mint the account has moved past. Opt-in, because a flow with no round
trip has no window to fence.
A "set" never said set of what. The concept is a provider-side access-controlled
grouping -- a Confluence space, a Jira project, a repo -- whose ACL governs the
items a read returned. `trackedCollectionObservers`, `hasCollectionAccess` and the
rest follow. Storage is untouched: `collectionPrefix` still defaults to
`"observed:"`, and the kit observers module has no importer outside the kit.
…findings A cursor is returned to the gadget and walked later, so it outlives the call that made it. Built on the session own gate, the first `next()` after the session released its stub failed -- and what failed was the authorization, not the data. `ObservationGate.lease()` opens a second gate over its own duplicate of the queue; both share the binding strategy, so exclusions stay one decision. Making the conformance consumer use it surfaced a constraint worth recording: `dup` is reserved over RPC, so only a real `RpcStub` can lease, not a service binding. Review follow-ups: - An unknown observation outcome latches `observer-withheld` and deletes its marker at once. Compaction only ever ran from `addObserver`, so a binding that admits nobody accumulated one durable key per ambiguous failure -- and since the overseer marks no refusal, that is the default path. - A cache entry is dated from when `load()` resolved, not from when the post-load fence read returned; that read is a live account call that can refresh credentials, and dating from its completion extended the caller TTL. - `ActionOutcomeUnknownError` non-replay rests on `claimBeforeApply`, and the doc promised it unconditionally. It now states the precondition, and the kit logs when the guarantee was unavailable. - Conformance handlers compare the action fence against the read their provider call runs under, closing the window apply entry check cannot; the connect race reproduces a revoke landing inside the exchange rather than before it. `plans/gatekeeper-kit.md` is reconciled: its Status claimed §4 matched the shipped signatures while still naming `trackedSetObservers`, `authority()` and an optional cache `options`, and arguing for the no-extra-round-trip cache partitioning the live fence replaced.
1d3798e to
a321626
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (journal.wasApplied(id)) { | ||
| if (record !== undefined) { | ||
| try { | ||
| journal.retire(id); | ||
| } catch (error) { | ||
| attributed.warn("failed to clear an applied action's leftover record", { | ||
| event: "actions.retire.heal.failed", | ||
| action: id, | ||
| error, | ||
| }); | ||
| } | ||
| } | ||
| return; |
There was a problem hiding this comment.
🟡 Recovered actions skip cache invalidation
When an applied action survives failed deletion, applyRecord heals it and returns without calling resolved. Cache invalidation never runs after the retry succeeds.
Prompt for agents
The applyRecord branch for journal.wasApplied(id) heals a leftover record and returns without invoking the action set's afterResolve hook through resolved("applied"). The original apply never reached that hook because retire threw. Ensure the eventual successful or best-effort healing path delivers the applied resolution needed for cache invalidation, while avoiding an unbounded series of duplicate callbacks when cleanup repeatedly fails.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
1 participant
CredentialSource.rungains areplayableoption: when the provider rejects credentials, the account decides whether they areexpired,superseded, orunavailable, and a replayable operation may retry once with a same-connection successor without crossing a reconnect.This is an improved version of
fetchWithAuthRetrythat many gatekeepers hand rollDerived-bearer ports can heal a rejected current credential inside
reportCredentialsRejected, with identity-keyed single-flight coalescing concurrent mints; grant-death ports instead notify the Workshop and return expired.Each attempt receives its own
{ identity, generation }fence, while namedCredentialsChangedErrorandCredentialsExpiredErrorresults survive RPC boundaries and stale reads, reports, and refetches cannot re-establish cache authority.