feat(gatekeeper-kit): replayable runs with account-adjudicated expiry - #441
feat(gatekeeper-kit): replayable runs with account-adjudicated expiry#441ndisidore wants to merge 33 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.
…allowing its error
…ordinator logs by vendor Also: the cache example marks its read replayable, run()'s throws doc names the fetch-path expiry source, CredentialsWithIdentity is declared as the read plus credentials, and an integration test pins the two-attempt cap when every mint succeeds but the provider keeps rejecting.
…unt re-serves the rejected identity The superseded retry's refetch adopts before the same-identity check throws, restoring the vouch for a credential the provider just rejected — so a cache-first re-entry serves the partition instead of re-entering, masking the lazy account for a TTL while the non-replayable path honestly bypasses. Supersede before the throw, as the ask does.
8d3db43 to
2c1c7a4
Compare
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.
…ecided expiry Three paths decided expiry before awaiting the Workshop notification and never looked again — snapshot's rethrow and both of adjudicateRejection's expired verdicts — so a reconnect landing during the notify RPC still surfaced grant death despite the live replacement. Re-check the fence after the await, as every other await in the coordinator already does: snapshot serves the replacement triple, the verdicts answer superseded. Also refuse a retry whose refetch the source did not itself adopt: a fenced-out response is stale evidence that can postdate an adopted reconnect, and be0768a's collapse deleted the crossed-generation bookkeeping that used to refuse it — the account only adjudicates failures that reach it, and this execution never asks. The live-state comparison replaces the deleted set: generations never revert, so a stale refetch can never re-match adopted state.
…into one method A #notified(identity, notify) coordinator method now owns "await the notification, then report whether the identity fence survived" — snapshot's catch and #expired both consume it, snapshot no longer pays the fence check on non-notify failures, and the module-level notified() helper is absorbed. snapshot also reuses #connected() instead of restating its body. Tests share a stallingNotify() scaffold and the two mid-notify verdict cases fold into one it.each; the fenced-out-refetch test uses the source() helper.
…adoption mid-verdict
|
No new findings. Previously published inline findings remain applicable to the current head. |
|
No new findings. Previously published findings remain applicable. |
capnweb rebuilds errors on decode, keeping enumerable own props but mapping unknown names to plain Error — so the name-only matchers went blind across the Gadget-facing hop. Both errors now carry a code the matchers accept alongside the name, following workshop-shared's coded-error precedent.
…dicts The adopted-successor shortcut was only exercised replayable, so an unconditional replay would double-execute non-replayable operations unnoticed; and repeat reports all re-answered expired, indistinguishable from a cached verdict wrongly expiring a reconnect.
|
LGTM! |
…xtures A MarkedError base owns the name+code transport-stable convention for both credential errors, and the CredentialsChangedError throw sites name the error in place. The credentials suite folds its hand-rolled accounts into source(), queuedSource(), and mutableSource() helpers and hoists stalledRun for the composition describe.
c288f8b to
6754a17
Compare
|
LGTM! |
There was a problem hiding this comment.
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! |
…isconnect "Superseded" promises a live successor, but a clear() landing during a rejection adjudication — mid-notify, mid-mint, or before the report — rotated the fence with nothing stored, so the verdict sent callers re-entering into a disconnected account. A moved fence now answers by successor: superseded when one is stored, expired when a disconnect left none, matching snapshot()'s own handling of the same race. The disconnect itself still never notifies.
| } | ||
| 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 |
…"" reads A thrown or malformed rejection report still fails closed as expiry, but is answered internally as "unadjudicated" and never dead-marks: a transient account outage must not retire a possibly-live identity — for a non-refreshing grant that disabled cache adoption for the rest of the activation. The source now refuses a read served under the reserved "" identity instead of letting a hand-written stub wedge every rejection as retryable, and the verdict union derives from one runtime tuple. Docs and the composition suite stop implying snapshot's stored grant crosses RPC unprojected: a new test wires the coordinator's Grant against the source's public projection.
| // 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. |
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.