Repository navigation
Add fast-check property tests across the Workshop backend and gatekeeper-kit - #897
Conversation
Preview:
|
|
LGTM! |
9022ed3 to
20d5ad2
Compare
|
LGTM! |
Eval resultsVerdict: ⚪ Unchanged. No task moved beyond what these runs can tell apart from noise.
Failed checks
|
🔬 Eval runs reviewPerformanceNo comparable pass-rate change exceeded noise: change-calendar 100% → 90%, chess 90% → 90%, worker-logs 80% → 90%, with triage and both compaction cases still passing; incident-desk is non-comparable after three candidate run errors. Prompt-cache hits and breaks did not move significantly, while cost/time moved inconsistently: calendar averaged $0.0236 → $0.0214 and 5.5 → 4.4 minutes, versus worker-logs $0.0168 → $0.0192 and 4.0 → 5.1 minutes. Incident-desk’s aborts lost the most runs, chess consumed the most steps (60.1 → 53.9), and single-run Sonnet compaction was most expensive ($1.7746 → $1.8221). ⚪ VERDICT: NO REGRESSION FROM THIS PRThe diff adds integration property tests and seed/cache configuration without changing agent behavior or eval checks, so it does not explain the isolated failures or the within-noise performance differences. TriageFailure modes
Tool errors
What to do
|
|
LGTM! |
… code changes Three end-to-end properties in packages/integration-tests replay generated sequences of real client operations against the Workshop over RPC, checked against a small model: - Approvals: staged writes decided in any order, with and without an auto-approval rule; the ledger, the fixture gatekeeper's state and the ledger-to-gatekeeper id mapping agree after every decision, nothing auto-applies past a pending manual write, and a second decision applies nothing. - Attachments: chats created, replied to and deleted with shared and same-named files; every attachment id returns its exact bytes in exactly the chat that holds it, including in a fresh chat and after deletion. - Code changes: two editors submit stale and retried changes to one draft; every insertion lands once, a retry is not re-applied, and accepting commits the text the change stream describes. FAST_CHECK_SEED pins the generated cases (src/property-runs.ts). CI sets it, the cached test task declares it in cache.env, and the env-passthrough guard accounts for it.
- Code changes: resend is its own op, so a resend can arrive after the other editor's change and must get its original acknowledgement back; the broadcast stream must number its changes 1..n; editors delete lines as well as insert them, as exact spans the way CodeEditor submits its transactions (a minimal diff can straddle lines); every session opens with an edit, and a draft that nets to no change keeps the head. - Approvals: the generator is a uniqueArray of writes (value, flag, decision, rank), which shrinks to minimal cases, and two pinned examples cover approving or rejecting a manual write ahead of auto-approvable ones, which random cases reached only on some seeds. - The 300ms settle sleep runs only while an eligible write is held behind a gate, and waits inside the properties use a 5s timeout, so failing shrinks finish in about 30s instead of the 90s cap. - Attachments: drop a guard for chat-id reuse, which never happens. - Docs no longer claim an unset seed explores on a cached run.
- PROPERTY_RUN_OPTIONS moves from integration-tests to @gadgets/scripts/property-runs. - workshop-backend gains fast-check; its vitest config forwards FAST_CHECK_SEED into workerd, where nodejs_compat copies it into process.env, and its test task fingerprints it. - The env-passthrough guard accounts for the moved read.
- Approvals: the provider may fail one apply, from staging or from any user decision on. A write the drain failed stays pending and is never skipped past; a failed approval is retried or denied; every apply attempt reaches the provider exactly once. - Observer re-verification: a collaborator's 1-3 opens while connections lapse and are repaired, checked against ensureObserver's prompt, rollback, error and observer-id rules.
…isolate - Sharing: Share-dialog action sequences against a reachability model, with remove/revoke previews checked against what they actually change. - Blueprint releases: generated gadget trees round-trip through pack and install, match GitStore's tree, and get an id that depends on the files alone. - Streamed tool input: arbitrary delta splits of real tool-call JSON keep the parser and the code-preview managers in step with the decodable prefix.
…d counts in-isolate - Turn resume: real agent turns stage awaited writes; approvals, rejections, the "always approve" rule, failed applies and workspace restarts are replayed over the session's RPC interface. A suspended turn makes exactly one more model request, when the last write it waits on is approved, and none after a rejection. - Two chats about one gadget: edit, save, discard, ask, accept, update and revert in any order. Each chat's cache and every fold of its log read its own changes on what it last merged, an accept is stale only when the chat changed something and is behind, and it commits exactly the chat's content. - Scope-widening restarts: what an owner does while collaborators have the workspace open, through the owner's client interface. A restart is scheduled exactly when an open session's scope widens (per docs/observers.md), what it gained is blocked until then, and each gadget lists the model's bindings. - Unread counts: two users and the agent in three chats while handoffs are lost, repeated and delayed, and objects reset. Stored rows, queries, summaries and delta-folding subscribers match docs/user-unread-state.md after every step.
- Action set: submissions, approvals and denials in the SharePoint shape (a creation, items inside it, edits of them) while the provider succeeds, fails retryably, refuses or times out. Journal records, the pending projection, handler dispatches, release hooks and refused submissions match a model after every step, and no pending action waits on a reference nothing can still bind. - Credentials: two chats on one connection with single-use refresh tokens, under fast-check's scheduler, while the token expires, the user reconnects or disconnects and the provider revokes the grant. No operation runs under two connections or replays a write, the stored grant is the last one connected, and every minted token is stored, discarded or refused. - gatekeeper-kit gains fast-check and its test task fingerprints FAST_CHECK_SEED.
1f1ff16 to
ba836e8
Compare
|
Review: 1 findings. Posted 1 actionable inline comment. |
…ntities Since #1001 an admission reserves the collaborator's observer id before registering anywhere, and a failed one keeps every registration. The property still modelled the rules it replaced: a failed first-ever open rolling back under a discarded id, and a later open minting a new one. It now checks that each verified pass registers at its binding, that nothing is ever removed, and that every registration names the one observer id. The prompt and error checks are unchanged.
|
LGTM! |
Both ran under vitest's 5s default. The action-set property takes 2-3s locally and took 4.7s and then 9.7s on two CI runners, so the second run timed out. They now get the 120s the workshop-backend properties have, which also leaves a failing run fast-check's 90s to shrink.
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
c377b934.
This review, of 083586c8
✅ Approved — This pull request only adds tests and test wiring, and no problems were found in code quality, security, performance or documentation.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Test-only changes and test fixture wiring; nothing security-relevant.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Read every file the change touches. The test helpers the new properties call exist, the seed is fingerprinted in cache.env and forwarded into workerd, the lockfile matches the fast-check additions, and the fixture gatekeeper's settled/attempts ordering is correct.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 083586c8
…erty The property flipped the hook record itself, because the test Worker did not export the loopback entrypoint enableHook() hands the gatekeeper. It is now exported beside the other loopbacks, and the step calls enableHook() as the Connections tab does, so the property also fails if that method stops reaching the scope check after its gatekeeper round trip.
There was a problem hiding this comment.
Superseded by the review of
f4d997fd.
This review, of c377b934
✅ Approved — This pull request adds only tests and test wiring. The new changes (owner-interface hook steps and the GatekeeperHookLoopback re-export) are consistent with the code they drive, and no issues were found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
The change is test-only plus test fixture wiring, and nothing in it affects security.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
test-worker.ts now re-exports GatekeeperHookLoopback, which src/server.ts exports. That re-export is needed because OverseerClientInterface.enableHook builds the loopback through ctx.exports, and the test pool takes ctx.exports from the test worker's own exports.
observer-scope-restart.test.ts now drives the owner's real enableHook, disableHook and deleteHook, and its model matches what those methods do:
- Hook ids are read before
bindHookallocates them. - Enabling an already enabled hook does nothing, and neither does disabling a disabled one.
- Deleting a hook removes its record.
Replaying after a restart skips join steps before from, so the replay always finishes.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: c377b934
|
Review: 1 findings. Posted 1 actionable inline comment. |
|
🤖 Comment P3 Nothing checks that gatekeeper-kit and integration-tests declare the seed Location: scripts/env-passthrough.test.ts:101–102 The comment says each of the three packages declares P3 The turn-resume property's logging is nearly half the test log Location: packages/workshop-backend/tests/durable-decision-resume.test.ts:995–998 This property silences no logging, where the chat-reroot and unread-state properties and both kit ones do. In the last CI run it printed 995 log blocks, about 16,800 of the 37,200 lines in the workshop-backend test job's log: the P3 Fresh seeds are only ever drawn on a developer's machine Location: .github/workflows/ci.yml:134–140 CI pins the seed to 1 and nothing else runs these suites, so new cases come only from a direct NitsThe header of
|
…sume property The scope-widening property's hook table handed out the stored objects, so a change to a record persisted without a put(): the property passed with the write removed from enableHookRecord() or disableHook(). It now copies records in and out, as real storage does, and fails either way. The turn-resume property logged every generated turn, about 45% of the lines in workshop-backend's CI test log. It now silences the console for its duration, as the chat and unread-count properties do.
Three packages each added FAST_CHECK_SEED to their own test task's cache.env, and nothing checked that two of them did: without it a cached run strips the seed, so CI would run that suite on a fresh seed with no sign of it. The shared vitest task now declares the seed, vitest-task.test.ts pins that, and the env-passthrough guard counts a generated task's shared list as declared. The three per-package overrides are gone. Also names gatekeeper-kit in property-runs.ts, and moves fast-check to a catalog entry so the three packages stay on one version.
There was a problem hiding this comment.
✅ Approved — This pull request adds only tests and test wiring. The new seed declaration on the shared vitest task, its pinning test and the catalog entry for fast-check match the code they drive, and no issues were found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
The changes are test code and test wiring only. They contain no secrets and open no new trust boundary.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
The seed setup is consistent:
FAST_CHECK_SEEDis declared in the shared vitest task'scache.env(scripts/vitest-task-vite-config.ts), andvitest-task.test.tspins it.- workshop-backend forwards the seed into workerd as a binding, with
nodejs_compatenabled. - CI sets the seed to 1.
The fixture gatekeeper's apply-attempts control reads settled before attempts, so it can never report an attempt as settled while it is still in flight. The models in the approvals, observer, resume, action-set and credentials properties match the behaviour they describe.
⚡ Performance: ✅ LGTM
The property runs are time-bounded by interruptAfterTimeLimit: 90_000 and a 120s test timeout, and the seed is pinned in CI. No production path is affected.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: f4d997fd
|
LGTM! |
Adds fast-check property tests for the Workshop backend and for gatekeeper-kit. The end-to-end properties in
packages/integration-testsreplay generated sequences of real client operations over RPC and check the result against a small model; every run uses a fresh account, so runs and shrink attempts can't see each other's state. The in-isolate properties inpackages/workshop-backenddrive the modules directly inside workerd, and the two inpackages/gatekeeper-kitrun under Node. No kernel, shared-API or kit source changes; this is tests, the fixture gatekeeper and test wiring.End to end:
CodeEditorsubmits them, and resend changes after the other's have landed. The stream numbers changes 1..n, every surviving line appears exactly once, a resend gets its original acknowledgement and isn't applied again, and accepting the draft commits the converged textIn-isolate, workshop-backend:
writeFile,editFileandexecuteCodearguments split at arbitrary points, inside escapes and surrogate pairs included, keep the streaming parser and the code-preview managers in step with what the input so far determinesaddGatekeeperdirectly, since the model gatekeeper's entrypoint is not among the test Worker's exports, and a hook is bound the way a gadget binds one, which is not an owner call. A restart is scheduled exactly when an open session's verification scope widens, asdocs/observers.mddefines it; what the session gained is unusable until then; and each gadget lists the model's bindings (Observer verification: fix coverage bookkeeping, restart sessions when scope widens #380, Keep pre-existing observer registrations when re-verification fails #344, add support for single-gadget sharing #894, Let the agent ask the user for an agent spawner #808)docs/user-unread-state.mdafter every step (Add durable unread summaries and message fanout APIs #837, Add unread inbox, web badges, and native badge consumers #838, Connect unread totals to native badges and durable push replacements #842)gatekeeper-kit:
defineActions; the fixture gatekeeper above stages by hand and does notconnect(). The test fails if its schedules never reach a reconnect during a refresh, a discarded mint or a second attemptFAST_CHECK_SEEDpins the generated cases. The shared options live in@gadgets/scripts/property-runs; CI sets the seed to 1, every generatedtesttask declares it incache.env(the shared vitest task does, pinned byvitest-task.test.ts), workshop-backend's vitest config forwards it into workerd, and the env-passthrough guard accounts for it. fast-check is a catalog entry. When it's unset, running vitest directly explores a fresh seed.Left out on purpose: concurrent decisions on one write (already pinned as an expected failure in
workshop-agent-actions.test.ts); an apply that fails while the step that staged its write still runs, whose turn carries on by design (see#drainAutoApprovalsAndResume); in-file merges between the two chats, which each edit their own file so the property needs no merge oracle; and, in the kit, concurrent submit and resolve, authority fences and each vendor's staged reconnect. A scheduled job that runs the suites on fresh seeds is left for a follow-up: CI pins the seed, so new cases are otherwise drawn only on a developer's machine. Three of the four newer backend properties extend their file's existing helpers (a trailingchatIdparameter, two helper splits, one shared fake-gatekeeper helper).Cost: the approvals property takes 4-5s per seed and the observer file 6-7s. In workshop-backend's unit suite the first three in-isolate properties add about 1s, and the four newer ones take about 5s, 4s, 5s and 8s, in four test files that run in parallel. The kit's two take about 3s together locally (the action set 2-3s of it) and took 6s and 11s on two CI runs. With 3 Vitest workers, the full integration suite took 67.1s against 64.6s before the observer property and apply failures were added (one run each); it has not been timed since.
Tested: each property caught every bug planted in the source to check it, with
FAST_CHECK_SEED=1. For the earlier properties: drain skipping past a failed apply, a provider failure recorded as applied, a failed open rolling back its registrations, an observer id minted again after a failed open, a terminal error naming the first pass's failures, four sharing bugs, three release-tree bugs and three streaming-split bugs. For the newer ones, 24 more: resuming while a write is still pending, resuming despite a rejection, a queued auto-approvable write not suspending its turn; a revert keeping the merged commit, reverted changes still folded, an update dropping the chat's own edits, an accept never stale, a discard keeping undeclared pins; a bind dropping its widening check, a newly reached gadget not counting as a widening, gained connections left usable,enableHook()skipping the scope check, a hook's enable and its disable never written back; a redelivered own message reading itself again, an older handoff lowering the count, a deleted chat coming back, failed handoffs never retried; a denial not retiring dependents, a dispatch against an unbound reference, an applied action dispatched again; a refresh overwriting a reconnect, a retry across connections, and a non-replayable operation replayed.On the final code,
pnpm lintpasses (lint, script types,configs:checkand the workspace build), as do the scripts suite with both env guards (370 tests), gatekeeper-kit's Node and workerd projects (777 and 127 tests) and the two workshop-backend test files changed last (91 tests, at seed 1 and at a fresh seed). Workshop-backend's full unit and integration suites last ran locally before the final two commits (2,553 and 3 tests, at seed 1), and the fullintegration-testssuite was not rerun locally after the rebase onto main; CI runs both here. The observer property's model predated #1001's shared observer identities and failed on the first CI run after the rebase; the commit after the kit's brings it up to date. On the next run the kit's action-set property, which had no timeout of its own, passed vitest's 5s default on a slower runner (9.7s, against 4.7s the run before); both kit properties now have the 120s timeout the backend ones have.