feat: add on-demand screen collections - #3217
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code-analysis diffPainscore total: 7997.19 → 7999.07 (+1.88) 🆕 New findings (30)
…and 10 more. ✅ Resolved (30)
…and 10 more. 📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
…y-collections # Conflicts: # public/screen-library/viewer.js
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — changes requested
Three prior defects remain: missing artifacts can strand collections, dispatch failures are not retriable, and source selection can choose main instead of dev. The native-export issue is fixed.
Findings
-
BLOCKING · scripts/screens/complete-collection.mjs:33 · Finalize collections when a capture artifact is missing
If a locale capture job fails before uploading capture.json, the publish job still runs, but this throw exits before the manifest is updated. The collection remains running and POST /capture refuses to queue a running collection, so it cannot be retried. Treat absent or wrong-commit artifacts as failed locales, persist the partial status and finishedAt, and only then report the incomplete run. -
MAJOR · scripts/screens/collection-worker/index.mjs:105 · Make dispatch failures retriable
queueCapture stores the request and marks the manifest queued before calling GitHub. If that dispatch returns an error or the request fails, the API returns 503 but leaves the collection queued; every later capture request then returns queued:false without dispatching anything. Dispatch before committing queued state, or catch failures and restore a retryable status. -
MAJOR · scripts/screens/collection-worker/index.mjs:49 · Select the latest dev capture, not historical main
bestEntry admits both dev and main and then sorts only by global sequence. When a complete main capture has a higher sequence than the latest dev capture, new collections reuse main assets even though the workflow targets dev and the documented contract says latest complete dev. Filter this source selection to branch === 'dev' (or otherwise make dev mandatory). -
MINOR · scripts/screens/collection-worker/index.mjs:90 · [claude-opus] The captureMissing dispatch path mutates shared R2 state with no test
queueCapture(scripts/screens/collection-worker/index.mjs:90) writescollection-requests/<id>.json, overwritescollections/<id>/manifest.jsonwithcapture.status: 'queued', and dispatches thescreen-library-collection.ymlworkflow.prepareCollectionCapture(scripts/screens/collection-request.mjs:32) then overwrites the same manifest tostatus: 'running'and enforces theCapture request does not match collection gapsguard. Neither function, nor thePOST /v1/collections/<id>/captureroute (collection-worker/index.mjs:176), is reached by any test in this PR.
Evidence: grep -rn "prepareCollectionCapture\|queueCapture" scripts/screens/*.test.mjs returns nothing. collection-worker.test.mjs exercises only the fully-complete case — its fixture has profile already captured, so collection.missing is empty and queueCapture is never entered. collection-request.test.mjs covers validateCollectionRequest only, not the surrounding state mutation. collection-worker-prepare.test.mjs tests collectionWorkerConfiguration (wrangler config), which is unrelated despite the similar name. The sibling R2-mutating entry point in this same PR, completeCollection, did get a full memory-storage test — so the omission is inconsistent with the PR's own standard.
This is the shipped default: docs/screen-collections/choice-overload.json sets "captureMissing": true, and 10 of its 16 items are brand-new screen IDs that cannot exist in any published capture, so the documented first run goes straight down the untested branch. It is also exactly where prior findings P2, P3 and P4 sit.
Exact untested case: create a collection whose spec has at least one screen/locale pair absent from the latest complete capture, with captureMissing: true — assert that collection-requests/<id>.json is written with the resolved 40-hex targetCommit and a screens map equal to missingByLocale, that the stored manifest reads capture.status === 'queued', and that a workflow dispatch was issued with {ref: 'dev', inputs: {collection_id}}. Add the prepareCollectionCapture counterpart: one case flipping the manifest to running, and one asserting a request naming a screen that is not a real gap is rejected.
Checked clean
- Confirmed the detached worktree HEAD and supplied base SHA, then reviewed the exact diff.
- Rechecked all four prior findings against current code and commit history.
- Verified the 404 fixture now imports a shared component that remains available when native-build disables src/app/not-found.tsx.
- Reviewed collection validation, R2 object allowlists, Access/service-token boundaries, GitHub dispatch flow, MCP service binding, viewer rendering, and focused-capture workflow behavior.
- Ran pnpm screens:test: 88 tests passed; 8 could not start because this detached worktree has no node_modules (missing tsx/pngjs), so no test failure was attributed to the PR.
- Inspected exact-head CI: refs, analyze, bot-approval, dependency-age, and automated review checks passed; capture and preview checks were still running during review.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: f410c94bc273 · Context: repo · Took 8m
There was a problem hiding this comment.
Chip review — changes requested
Request changes: the Access identity is not verified, a failed create-and-dispatch call does not expose a usable retry handle, the moved 404 bypasses its localization lint boundary, and collection coverage goes stale after a locale switch.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:25 · Authorize writes from verified Access claims — also flagged by moonshotai/kimi-k3
Any request that reaches this Worker can satisfy actor() with an arbitrary non-empty assertion header and a forged @peanut.me email; neither header is tied to a verified signature, issuer, or audience. During an absent or mis-scoped Access configuration, that lets the caller write collection state and dispatch the secret-bearing GitHub workflow. Use the Worker's verified ctx.access identity (and expected audience) or cryptographically verify the Access JWT and take the email from those claims; keep the private service-token path separate. -
MAJOR · scripts/screens/collection-worker/index.mjs:131 · Return the failed collection's retry handle
When POST /v1/collections creates a random-ID collection and the workflow dispatch fails, this catch persists status=failed and rethrows; the outer handler returns only the GitHub error. The real caller therefore never receives the random collection ID or URL and there is no list endpoint from which to discover it, so it cannot call /capture. The new test succeeds only by inspecting the fake R2 bucket directly. Return the failed collection and ID in a usable response, or roll back the created objects so the caller can retry creation without orphaning state. -
MAJOR · src/components/Global/NotFoundScreen.tsx:42 · Restore the moved 404 component's lint boundary
Moving this page under src/components/Global makes it subject to the repository's localization guard, but this revision wraps every English string in a JSX expression solely to evade react/jsx-no-literals. The file now passes lint while violating the contract the rule is meant to enforce. If the provider-free 404 is intentionally English-only, add a narrow documented ESLint exclusion; otherwise supply translated copy through a provider-aware boundary. Do not hide the literals in braces. -
MINOR · public/screen-library/viewer.js:724 · Refresh coverage after switching collection locale
This handler replaces rows and rerenders the tiles but never recomputes #coverage, which was derived once from the initial locale. In a partial collection where English is captured and pt-BR has gaps, switching to pt-BR shows unavailable tiles while the header still reports the English captured count. Factor coverage rendering into a helper invoked whenever collection rows change, and cover mixed locale statuses in the viewer test.
Checked clean
- Exact head, supplied base, and merge base match; trusted PR title and description were read without fetching issue or review comments.
- 36 focused collection, request, worker, MCP, viewer, and storage-boundary tests pass at this head; git diff --check is clean.
- Missing capture artifacts now persist a partial manifest and failedLocales before failing the workflow.
- Source selection now requires a complete synthetic dev capture and ignores a newer historical main entry.
- The captureMissing happy path now exercises the shared R2 request and manifest mutations.
- Current Cloudflare Workers Access guidance was checked for verified ctx.access identity and JWT validation requirements.
- All completed GitHub checks at this head are green or skipped; eight visual capture jobs were still running at review time.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: b0ec51a40947 · Context: repo, web · Took 12m
🖼 Visual diff — 1 screen moved2 of 96 shots changed · 94 identical · baseline
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
There was a problem hiding this comment.
Chip review — changes requested
Found an unauthenticated MCP write path and one exact-head formatting failure; the nine supplied prior findings are corrected at this head.
Findings
-
BLOCKING · scripts/screens/mcp/index.mjs:82 · Require verified Access on the MCP ingress
This Worker invokes the collection service with the shared internal credential but never checkscontext.access. If the custom-domain Access application is absent or removed, an anonymous/mcpcaller can usecreate_collectionorcapture_missing_statesto write manifests and dispatch the privileged capture workflow. Fail closed before constructing the handler: require the expected Access audience and a verified@peanut.meidentity, and add that audience to the generated MCP configuration. -
MINOR · scripts/screens/collection-worker.test.mjs:108 · Format the new collection-worker test
The exact-headformatcheck fails while the exact base's format check succeeds. With the locked Prettier 3.8.1, this addedworker.fetchexpression formats onto one line. Commit the repository formatter's output so the format gate can pass. -
MINOR · scripts/screens/collection-worker/index.mjs:25 · [moonshotai/kimi-k3] Service token compared with non-constant-time equality
In actor(), the shared COLLECTION_SERVICE_TOKEN is checked withinternal ===Bearer ${env.COLLECTION_SERVICE_TOKEN}``. String equality short-circuits on the first differing byte, enabling a timing side channel against the service-binding credential (which also authorizes direct CLI access to the collection API over HTTPS). Use a constant-time comparison (e.g., crypto.subtle.timingSafeEqual over encoded bytes, or hash both sides with SHA-256 and compare digests) before accepting the token.
Checked clean
- The detached worktree HEAD and merge base matched the supplied head and base SHAs; trusted PR metadata matched the supplied author and dev base.
- Re-evaluated P1-P9: partial finalization, retriable dispatch, latest-dev source selection, failed-create retry response, the 404 lint scope, locale coverage, and collection API Access identity checks are corrected at this head.
- Exact-head CI passed typecheck, ESLint, unit, screen-tests, native export, and analyze; format and ci-success failed, while visual capture jobs were still running when reviewed.
- The detached checkout lacks the full node_modules tree, so a local full screens:test run was dependency-blocked; dependency-free collection auth, retry, MCP client, viewer, and workflow-policy tests passed, and exact-head CI screen-tests passed.
- Cloudflare's current Workers Access contract confirms direct authenticated invocations expose ctx.access, unauthenticated invocations do not, and service bindings do not propagate the caller's Access context.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 18e0fc17204e · Context: repo, web · Took 19m
There was a problem hiding this comment.
Chip review — changes requested
Three findings remain: the generated Worker bundles omit their shared authentication module, the documented local capture-directory CLI path fails, and the exact-head format gate is red.
Findings
-
BLOCKING · scripts/screens/collection-worker/prepare.mjs:45 · Include the Access helper in both deployed Worker bundles
Both generated Worker entrypoints import../access.mjs, but the collection preparation copies onlycollection-core.mjsand the MCP preparation copies only its index/client files. A normal configured deployment therefore produces bundles with noaccess.mjs, so Wrangler cannot resolve the module and neither new Worker can deploy. Copy the helper into both generated roots and cover the generated bundles with a dry-run or import-resolution test. -
MAJOR · scripts/screens/collection-cli.mjs:29 · Accept capture.json when a report argument is a directory
Following the documented--report=en:/tmp/screens-enflow fails:run-capture.mjscreatescapture.jsonin that directory, whilereportInputunconditionally appendsmanifest.jsonfor every directory argument. The CLI throws ENOENT before producing the collection. Resolvecapture.jsonas the directory fallback (while retainingmanifest.jsonsupport) and add the raw-capture-directory case to the CLI test. -
MINOR · scripts/screens/mcp/index.mjs:84 · Restore the exact-head format gate
Theformatcheck fails on this SHA. Reproducing Prettier identifies this condition plus Tailwind class-order drift insrc/components/Global/NotFoundScreen.tsxandsrc/components/Setup/Views/Residence.tsx. Format those three files and rerun the gate. -
BLOCKING · scripts/screens/collection-worker/prepare.mjs:45 · [claude-opus] Worker deploy trees omit access.mjs and no test covers the emitted module graph
scripts/screens/collection-worker/index.mjs:9andscripts/screens/mcp/index.mjs:4both import../access.mjs, which is new in this revision. Neither prepare script copies it: -
collection-worker/prepare.mjs:45copies only['collection-core.mjs']into the target root, thencollection-worker/index.mjsintotarget/collection-worker/.../access.mjsresolves totarget/access.mjs, never written. -
mcp/prepare.mjs:35-36copies only['index.mjs','client.mjs']intotarget/mcp/.../access.mjsresolves totarget/access.mjs, never written; the workflow only symlinksnode_modulesalongside it.
grep -rn "access.mjs" scripts/ .github/ returns exactly three hits — the two imports and the unit test — so nothing else stages the file. In .github/workflows/screen-library-publish.yml, the Deploy collection API Worker and Deploy MCP Worker steps run wrangler deploy directly against these prepared directories, so esbuild fails to resolve the import and both control-plane Workers fail to build once SCREEN_LIBRARY_COLLECTION_API_URL / SCREEN_LIBRARY_MCP_URL are set (the docs in this PR list both as required).
The untested case, precisely: prepareCollectionWorker() and prepareMcpWorker() — the functions that emit the deployable tree — have no test at all. collection-worker-prepare.test.mjs and mcp-prepare.test.mjs only exercise collectionWorkerConfiguration() / mcpWorkerConfiguration(), the pure config builders. A test that runs the prepare function into a temp dir and dynamically imports the emitted index.mjs (or asserts the copied file set) would have caught this.
Fix: add 'access.mjs' to the copy list in collection-worker/prepare.mjs:45, copy scripts/screens/access.mjs to join(target, 'access.mjs') in prepareMcpWorker, and cover both with a prepare-emit test.
Checked clean
- Confirmed the detached worktree head, supplied base SHA, and merge base exactly match the request.
- Rechecked P1-P10: P1-P8 and P10 are fixed at this head; P9's cited collection-worker test is now formatted.
- Verified Cloudflare Access audience/identity semantics and the service-binding non-propagation boundary against the official Cloudflare Workers documentation; the internal bearer hop matches that boundary.
- Reviewed workflow target-SHA ancestry, missing-artifact finalization, retry state, latest-dev source selection, collection validation, viewer locale switching, and 404 lint coverage.
- Exact-head CI has green unit, screen-tests, typecheck, ESLint, native-export, and preview checks; format and aggregate ci-success are failing, while screenshot capture checks were still running.
- Scoped Node screen tests produced 35 passes; complete-collection could not run locally because the detached worktree has no pngjs installation, while the exact-head screen-tests CI check is green.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: d869c1f132aa · Context: repo, Cloudflare documentation · Took 16m
There was a problem hiding this comment.
Chip review — changes requested
Four findings remain: collection dispatch cannot start from dev, abandoned captures cannot retry, the hosted CLI cannot cross Access, and the format gate is red.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:116 · Register the capture workflow on the default branch
This dispatch names a workflow added only by this dev-targeted PR, while the repository default branch is main. GitHub accepts workflow_dispatch only for workflows registered on the default branch, so after this merges to dev every collection with missing states still takes the dispatch-failure path and no capture starts. Bootstrap this workflow on main before relying on it, or route dispatch through an already registered default-branch workflow while keeping ref: dev. -
MAJOR · scripts/screens/collection-worker/index.mjs:212 · Let abandoned running captures retry
The request job marks the collection running before capture and publication. If that run is cancelled or the publish job fails before completeCollection updates the manifest, every later POST /capture returns queued:false forever. Add an attempt/generation marker and a bounded stale-running recovery path so an abandoned run can be requeued without racing a live attempt. -
MAJOR · scripts/screens/collection-cli.mjs:87 · Authenticate the hosted CLI through Access
The public API origin is configured behind Cloudflare Access, but this request sends only the Worker's internal bearer token. Access authenticates service clients with its client-id/client-secret headers or managed OAuth before a request reaches the Worker, so the CLI receives the Access response and never creates the collection. Use an Access credential flow for the hosted CLI and keep COLLECTION_SERVICE_TOKEN private to the Worker service binding. -
MINOR · src/components/Global/NotFoundScreen.tsx:26 · Make the changed UI files pass Prettier
The required format check fails on this exact SHA: pnpm prettier --check . flags this file and src/components/Setup/Views/Residence.tsx, leaving ci-success red. Run the repository formatter on both changed files and commit its output. -
MAJOR · scripts/screens/collection-cli.mjs:79 · [moonshotai/kimi-k3] Hosted CLI still uses the shared service token instead of Access identity
createHostedCollection in scripts/screens/collection-cli.mjs sends COLLECTION_SERVICE_TOKEN as a bearer token directly to the collection API, and actor() in scripts/screens/collection-worker/index.mjs accepts that token as full authorization returning the fixed identity 'mcp@internal'. The docs describe this token as authenticating only the service-binding hop, but the CLI path lets any holder of the token create collections and dispatch GitHub Actions workflows (which consume the GITHUB_ACTIONS_TOKEN and CI minutes) with no per-user attribution and no Access policy enforcement. If the token leaks from a developer shell or CI log, there is no way to revoke one user's access without rotating it on both Workers. Fix: have the CLI authenticate through Cloudflare Access (cf-access-token or an Access service token scoped per user) and only accept COLLECTION_SERVICE_TOKEN on requests arriving via the MCP service binding.
Checked clean
- Pinned head, trusted author, dev base, and merge base matched the supplied review scope.
- P1-P12 were reassessed against this head; their missing-artifact, retry-handle, dev-source, Access, bundle, CLI-input, lint-boundary, coverage, and exact-file formatting defects are fixed.
- CI at this SHA passes screen-tests, unit, typecheck, eslint, native-export, ds-lint, preview, and provenance gates; format fails on two changed UI files and capture jobs remain in progress.
- Official GitHub workflow_dispatch and Cloudflare Access service-authentication contracts were checked against the implementation.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 23ee3f225540 · Context: repo, ci, official_docs · Took 13m
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The collection retry and Access changes close most prior findings, but hosted CLI authentication still fails, two retry paths can leave or corrupt capture state, and the format gate remains red.
Findings
-
MAJOR · scripts/screens/access.mjs:6 · Accept Access service-token identities for the hosted CLI
The hosted CLI now correctly sends Cloudflare Access service-token headers, but Access service-token identities containcommon_name/service_token_status, not an employeeemail. ThereforeverifiedAccessIdentity()returns null at line 6 and the collection Worker answers 401 for every documented CLI request. Preserve the email-domain check for human sessions, while accepting a verified service-token identity that the collection Access policy explicitly authorizes. -
MAJOR · scripts/screens/complete-collection.mjs:28 · Bind completion to the invoking capture attempt
A stale workflow does not carry its own attempt into completion. After attempt A is declared stale and attempt B overwrites both R2 objects, A's publish job reads B's request and B's manifest here, so this equality passes and A can mark B partial/complete or publish artifacts against B's target. Emit the attempt from the request job, pass it through the workflow tocompleteCollection, and reject unless that caller-supplied attempt matches both current objects. -
MINOR · src/components/Global/NotFoundScreen.tsx:26 · Make the changed UI files pass Prettier
The exact-headformatcheck still fails onsrc/components/Global/NotFoundScreen.tsxandsrc/components/Setup/Views/Residence.tsx. Run the repository's Prettier formatter on both changed files and commit the resulting class/order changes.
Checked and not raised again
- MAJOR · scripts/screens/collection-worker/index.mjs:212 · Let abandoned queued captures retry — this review checked it and does not believe it. No task filed.
Checked clean
- Confirmed the detached worktree is exactly 26dd1db and its merge base is ceeeb22.
- Read the trusted PR metadata and exact-head CI; screen tests, typecheck, and ESLint pass, while the format job fails on the two changed UI files.
- Traced collection creation, dispatch, request preparation, partial completion, stale-running recovery, and workflow concurrency through the Worker and GitHub Actions code.
- Checked Cloudflare's current Access Worker and service-token identity contracts against both public ingress paths and the service-binding hop.
- Reviewed collection viewer locale switching, generated Worker bundles, local report discovery, and the moved 404 lint boundary.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 26dd1db5f9f8 · Context: repo, ci, web · Took 17m
There was a problem hiding this comment.
Chip review — changes requested
Request changes: the focused capture workflow cannot be dispatched after a dev-only merge, and four additional auth, concurrency, retry, and formatting defects remain.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:130 · Register the capture workflow on the default branch
The repository default branch is main, but this PR targets dev and the workflow is still absent from main. GitHub only delivers workflow_dispatch when the workflow file exists on the default branch, so this dispatch returns 404 and every collection with missing states fails to start. Register a safe version of this workflow on main before enabling the Worker, or trigger an already-registered default-branch workflow. -
MAJOR · scripts/screens/access.mjs:9 · Accept Cloudflare's boolean service-token identity flag
Cloudflare's Access identity contract exposes service_token_status as boolean true, not the string "active" mocked by the new test. A valid hosted CLI request therefore passes Access but verifiedAccessIdentity returns null and the collection Worker answers 401. Check for true and make the test use the documented identity shape. -
MAJOR · scripts/screens/complete-collection.mjs:28 · Make the attempt check atomic with collection completion
The attempt is compared only before WebP conversion and uploads. If the Worker expires that attempt and queues a replacement after this check but before the final allowOverwrite put, the old run can still overwrite the newer manifest and erase its attempt ID. Re-read and compare at commit time and use an ETag/conditional write (or another compare-and-swap) for the manifest update. -
MAJOR · scripts/screens/collection-worker/index.mjs:275 · Let abandoned queued captures retry
Queued captures still have no expiry path. If GitHub accepts the dispatch but the run is cancelled or dropped before prepareCollectionCapture marks it running, every later POST /capture returns queued:false forever. Apply a timeout to requestedAt and issue a fresh attempt just as the running-state recovery does. -
MINOR · src/components/Global/NotFoundScreen.tsx:26 · Make the changed UI files pass Prettier
The exact-head format check still fails on src/components/Global/NotFoundScreen.tsx and src/components/Setup/Views/Residence.tsx. Run the repository's locked Prettier setup and commit both outputs; this is the same surviving defect previously recorded as P16 and P19.
Checked clean
- Confirmed the detached worktree HEAD and merge base exactly match the supplied head and base SHAs; PR author and base ref also match.
- Rechecked all prior findings: P1-P12 except the duplicate formatting item, P14-P15, and P21 are fixed by the current code; P13, P17-P20 remain as reported above.
- P16 and P19 describe the same surviving two-file Prettier failure; P19 is used as the prior-finding anchor.
- CI at this head passed screen tests, ESLint, typecheck, native export, unit tests, captures, and ds-shots; format failed on the two cited UI files.
- The screen-library publisher failed while verifying an unchanged external Cloudflare credential, so it was not attributed to this diff.
- Verified the GitHub workflow_dispatch default-branch rule and Cloudflare Access identity field types against their current official documentation.
- Traced missing-artifact finalization, failed dispatch retry, source selection, Access ingress, bundle preparation, locale switching, CLI report directories, and stale-running recovery through code and tests.
Security review: did not run — the model reasoned past its deadline without writing an answer. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 9f9d80399e17 · Context: repo, ci, provider_docs · Took 25m
There was a problem hiding this comment.
Chip review — changes requested
Found one blocking dispatch failure, one major completion/retry race, and one minor formatting gate failure.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:130 · Register the capture workflow on the default branch
The Worker dispatches screen-library-collection.yml, but this repository's default branch is main and that path is absent there. GitHub only delivers workflow_dispatch when the workflow file exists on the default branch, so after a dev merge every captureMissing request reaches a failed dispatch and no focused capture can start. Bootstrap this workflow on main, or dispatch through an already registered default-branch workflow, before enabling the collection service. -
MAJOR · scripts/screens/collection-worker/index.mjs:262 · Make stale-retry transitions conditional
A run can cross the 90-minute stale threshold while its publish job is finishing because the 60-minute limit covers only the capture job. If /capture reads stale running attempt A, then A commits complete before this unconditional put, this write restores A's old missing snapshot and queues attempt B; if B's dispatch then fails, a successfully completed collection is left failed and missing. The completion-side If-Match does not prevent this reverse interleaving. Carry the observed R2 etag through the failed-to-queued transition and use conditional puts with reload/re-evaluation on conflicts, or serialize mutations per collection. -
MINOR · src/components/Global/NotFoundScreen.tsx:34 · Format the two changed UI files
The exact-head format check fails for src/components/Global/NotFoundScreen.tsx and src/components/Setup/Views/Residence.tsx. Run the repository Prettier configuration over both files and commit the output; the required format gate remains red at this head. -
MINOR · scripts/screens/collection-worker.test.mjs:285 · [claude-opus] Stale-queued capture recovery is the one mutating branch with no test
collectionRoutehas two expiry-recovery branches that both write afailedmanifest to R2 and then re-dispatch the GitHub Actions workflow with a fresh attempt ID.collection-worker.test.mjscovers therunningbranch ('an abandoned running capture gets a new retry attempt', which asserts the new attempt lands incollection-requests/<id>.json) but never exercises thequeuedbranch at scripts/screens/collection-worker/index.mjs:275. Untested case: a collection whosecapture.statusisqueuedwith arequestedAtolder thanCAPTURE_STALE_AFTER_MS, POSTed to/v1/collections/<id>/capture— it should overwrite the manifest tofailed, re-read, and queue a new attempt.
This is not a cosmetic gap: the two branches guard the timestamp differently. The running branch bails out on an unparseable timestamp (!Number.isFinite(startedAt) || ... at line 250), so a manifest with a bad startedAt is stuck in running forever, while the queued branch (Number.isFinite(requestedAt) && ...) falls through to recovery on the same input. A test over the queued path would have surfaced that asymmetry; as it stands each branch's write-then-re-dispatch is only verified for one of the two statuses.
Fix: add a sibling of the existing abandoned-running test that seeds capture = { status: 'queued', requestedAt: <2h ago>, attempt: <uuid> }, asserts 202 with a new attempt, and asserts the persisted manifest and collection-requests/<id>.json agree on that attempt.
Checked clean
- Verified the detached HEAD, supplied base and merge base, trusted author, PR metadata, and clean worktree.
- Reviewed Access identity enforcement, service-token claims, Worker bundle preparation, MCP service binding, hosted CLI authentication, source selection, retry state, completion CAS, collection locale rendering, and workflow secret boundaries.
- Reviewed exact-head CI: unit, screen-tests, typecheck, eslint, native-export, and preview passed; format failed on the two reported UI files.
- Checked official Cloudflare Access and R2 conditional-write behavior and GitHub workflow_dispatch default-branch requirements.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 8a93af5de78f · Context: repo, ci, provider-docs · Took 16m
There was a problem hiding this comment.
Chip review — changes requested
The capture workflow remains unregistered on the default branch, and exact-head formatting still fails in two changed UI files.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:153 · Register the capture workflow on the default branch
This still dispatches the new workflow by filename, but peanut-ui's default branch ismainand that file is absent there. GitHub only registersworkflow_dispatchworkflows from the default branch, so after this PR merges todev, every missing-state request reaches this call and gets a 404; the collection is marked failed instead of starting capture. Land/register the workflow onmainfirst, or dispatch an already registered default-branch workflow. -
MINOR · src/components/Global/NotFoundScreen.tsx:34 · Format the two changed UI files
Theformatcheck at this exact SHA fails onsrc/components/Global/NotFoundScreen.tsxandsrc/components/Setup/Views/Residence.tsx, which also leaves the requiredci-successgate red. Run the repository Prettier command on both files and commit the result. -
MAJOR · scripts/screens/collection-worker.test.mjs:80 · [claude-opus] Conditional-write guard on capture retry has no test that can fail
The last two commits made the stale-capture retry path conditional:collectionRoutepassesstored.etagintoputJson, which turns it into an R2onlyIf: {etagMatches}write, and the loser of a race is meant to fall intoif (!recovery.ok)/ theCollection changed while queuing a capture.catch and returnqueued: falseinstead of dispatching a second GitHub Actions run. None of that is covered. The fake bucket in collection-worker.test.mjs returns{ json }with nohttpEtag/etag(line 73), soreadJsonWithMetadatayieldsetag: nullandputJsonnever attachesonlyIfin any test; itsput(line 80) ignores the options argument entirely and returnsundefined, whichputJsonmaps took: true, so the failure branch is unreachable by construction. The same holds in complete-collection.test.mjs:memoryStorageimplements noreadWithMetadata, solatestManifest.etagisundefinedand theifMatchcommit guard is never sent.
Exact untested case: two concurrent POST /v1/collections/<id>/capture against a collection whose capture is stale-queued or stale-running — one request must win the etag-conditional manifest write and return 202, the other must observe the precondition failure and return the re-read collection with queued: false, dispatching no second workflow. Today deleting etag: stored.etag from both recovery writes and from the queueCapture(candidate, env, stored.etag) call leaves the whole suite green, i.e. the guard could be removed silently while this shared-state mutation double-dispatches CI and overwrites a replacement attempt's manifest.
Fix: give the worker's fake bucket real R2 semantics — return httpEtag from get, store a per-key etag that changes on write, and have put return null when onlyIf.etagMatches does not match the current etag — then add a test that mutates the manifest between the read and the conditional write and asserts the second caller gets queued: false with exactly one dispatch call recorded. Mirror it for complete-collection by adding readWithMetadata to memoryStorage and rejecting a stale ifMatch.
Checked clean
- Confirmed the detached worktree head and merge base match the supplied SHAs.
- Revalidated every prior finding against the current auth, retry, completion, workflow, CLI, viewer, bundle-preparation, lint-boundary, and test code.
- Verified the repository default branch is main and the new capture workflow is absent there.
- Checked exact-head CI: screen tests, unit tests, typecheck, ESLint, native export, and preview deployment passed; format failed on the two named UI files while visual capture jobs were still running.
- Compared the Access identity and service-token fields and workflow-dispatch behavior with current Cloudflare and GitHub documentation.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: cb294c5a46b5 · Context: repo, web · Took 13m
There was a problem hiding this comment.
Chip review — changes requested
The capture control plane still cannot dispatch its workflow after a dev-only merge; the formatting regression is fixed.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:153 · Register the capture workflow on the default branch
The deployed control-plane Worker dispatches this workflow by filename, but peanut-ui's default branch is main and main still has no .github/workflows/screen-library-collection.yml. GitHub requires a workflow_dispatch workflow to exist on the default branch, so merging this PR only to dev makes every missing-state request receive a 404 from GitHub and leaves the capture failed. Register this workflow on main before deploying the Worker, or dispatch through a workflow that is already registered there. -
MAJOR · scripts/screens/complete-collection.test.mjs:44 · [claude-opus] Mid-run attempt-expiry guard in completeCollection has no test
completeCollectionmutates shared R2 state (collections/<id>/manifest.json,collection-entries/<id>.json, publishedassets/*). Its commit path deliberately re-reads the manifest and request immediately before writing and uses a conditional put (scripts/screens/complete-collection.mjs:82-101), with a comment stating the reason: the worker can expire this attempt while conversion/uploads are in progress, and an old worker must not overwrite a replacement attempt. That race is real — the worker declares a running capture abandoned after CAPTURE_STALE_AFTER_MS (90 min) and mints a new attempt ID (scripts/screens/collection-worker/index.mjs:263-283), while the capture job has a 60-minute timeout and the publish job runs withif: always().
No test covers the guard firing. complete-collection.test.mjs:197 ('a late completion cannot overwrite a newer capture attempt') mismatches the attempt up front, so it throws at the entry check on line 33 and never reaches the re-read or the conditional write. complete-collection.test.mjs:44 ('memory storage rejects a stale completion ETag') calls storage.put directly and asserts only that the test double honours ifMatch — it never invokes completeCollection. Correspondingly the beforeConditionalPut hook added to the memory storage double (line 21, fired at line 33) is never assigned in this file: it is scaffolding for a race test that was not written. The exact untested case is: attempt is current when completeCollection starts, the worker replaces it (new attempt ID + new manifest ETag) during WebP conversion/upload, and the completion must abort with 'Capture attempt is no longer current' rather than write status: complete over the newer attempt. Drop either the re-read recheck or the ifMatch and every existing test still passes.
Fix: add two cases using the existing hook — set storage.beforeConditionalPut to rewrite the manifest and collection-requests/<id>.json with a fresh attempt, and assert (a) completeCollection rejects, and (b) the stored manifest still holds the newer attempt and was not flipped to complete. The sibling worker test at collection-worker.test.mjs:346 does exactly this for the queue side and is the pattern to copy.
Checked clean
- Verified the supplied detached worktree is exactly da5d0b4 and the merge base is the supplied dev SHA.
- Verified exact-head screen-tests, format, ESLint, typecheck, CodeQL, supply-chain, and preview checks pass; unit, native-export, design-system screenshots, and screen captures were still running at review time.
- Verified the current formatting commit only reorders utility classes in the two previously failing UI files, and the exact-head format gate now passes.
- Verified Cloudflare Access identity and audience checks guard both Worker ingresses, including boolean service-token identities, and the shared helper is bundled into both deployments.
- Verified failed, stale-running, and stale-queued capture retries use attempt IDs and conditional R2 writes, with race-path tests at this head.
- Verified completion rechecks the invoking attempt and manifest ETag before committing, including missing-artifact and late-completion coverage.
- Verified the hosted CLI uses Access service-token headers, local report directories accept capture.json, and collection locale switches refresh coverage.
- Verified main is the repository default branch, the new workflow file is absent there, and GitHub's workflow_dispatch documentation requires it on the default branch.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: da5d0b4449f2 · Context: repo, ci, GitHub Actions docs, Cloudflare Access docs · Took 12m
There was a problem hiding this comment.
Chip review — changes requested
One blocking deployment issue and two major defects remain: the capture workflow is not registered on the default branch, concurrent retries can clobber queued state, and incomplete collection pages hide missing entries.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:153 · Register the capture workflow on the default branch
Every missing-state request dispatches this workflow by filename, but the repository default branch ismainand the Actions API currently returns 404 forscreen-library-collection.ymlbecause the file exists only in thisdev-targeted pull request. After the Worker is deployed fromdev, a collection with missing assets is persisted, dispatch fails, and the API returns 503 without ever starting capture. Register the workflow onmainbefore deploying this path, or dispatch through an entrypoint already registered on the default branch. -
MAJOR · scripts/screens/collection-worker/index.mjs:170 · Do not roll back a capture attempt that lost the queue race
Two simultaneousPOST /capturerequests can read the same failed or partial manifest ETag. The winner conditionally persists and dispatches its attempt; the loser getsmanifest.ok === falsebeforemanifestEtagis assigned, then this catch callsputJsonwith a null ETag and unconditionally overwrites the winner asfailed. Reproduced at this head: the calls returned 202 and 200 with exactly one workflow dispatch, but the stored capture ended failed with no attempt. Skip the failure rollback when the queue-state write did not succeed, and keep rollback conditional on the ETag produced by a successful queue write. -
MAJOR · public/screen-library/viewer.js:110 · Keep missing collection entries visible
For an uncaptured locale variant this creates anafterobject with no image. The sharedfilteredScreenRowsavailability gate then drops the row entirely, so an incomplete shareable collection hides each requested missing item; when all items are missing the page saysNo screens matcheven while coverage says capture is in progress. Allow collection rows through the availability gate sorenderTilecan show their missing reason, and cover an incomplete collection in the viewer test.
Checked clean
- Confirmed the detached worktree head and merge base exactly match the supplied SHAs.
- Read trusted pull request metadata and exact-head checks; the aggregate CI gate, format, eslint, typecheck, screen-tests, unit, and native-export passed. The screen publisher's Cloudflare-token failure also occurs on the base SHA and preceding dev runs, so it is not introduced by this PR.
- Verified the default branch is main, the workflow registry returns 404 for screen-library-collection.yml, and GitHub's workflow_dispatch contract requires the workflow on the default branch.
- Verified Access identity and service-token authentication against the current Cloudflare Workers identity contract, including boolean service_token_status and common_name.
- Checked dispatch failure recovery, stale queued/running retries, R2 conditional transitions, attempt-bound completion, and the new mid-run completion-race test; separately reproduced the queue-state rollback race reported above.
- Checked CLI report-directory and Access authentication paths, Worker bundle preparation, MCP ingress, collection locale coverage, and the moved 404 lint boundary.
- Checked collection viewer ordering, notes, locale switching, and incomplete variants; missing variants are dropped by the availability filter as reported above.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 04ce5c541ecb · Context: repo, github, cloudflare-docs · Took 32m
There was a problem hiding this comment.
Chip review — changes requested
One blocking dispatch defect remains, and the documented hosted CLI setup omits its required Cloudflare Access Service Auth policy.
Findings
-
BLOCKING · scripts/screens/collection-worker/index.mjs:153 · Register the capture workflow on the default branch
Every missing-state request dispatches this workflow by filename, but the repository default branch ismainand.github/workflows/screen-library-collection.ymlis still absent there. After this PR merges only todev, GitHub rejects the dispatch, so every collection that needs a new capture is left failed/retriable and never starts its workflow. Register the workflow onmainbefore enabling the Worker, or dispatch through an entrypoint already registered on the default branch. -
MAJOR · docs/SCREEN-LIBRARY.md:86 · Provision the CLI's Access Service Auth policy
Following this setup creates only the interactive Google/@peanut.me Allow policy. Cloudflare admits the service-token headers used bycreateHostedCollectiononly when the collection Access application also has aService Authpolicy for that token; without it, Access returns a login or denial response and the documented hosted CLI cannot create a collection. Add creation of the CLI Access service token and its Service Auth policy to the setup instructions. -
MINOR · scripts/screens/collection-core.mjs:98 · [moonshotai/kimi-k3] Creator's internal email published in publicly readable collection manifests
composeCollection stores createdBy (the verified @peanut.me email or service-token name from the Access identity) in the collection manifest, and the collection worker writes that manifest to R2 at collections//manifest.json. The public viewer fetches these manifests unauthenticated from /screen-data/collections//manifest.json (viewer.js), so any internet visitor can enumerate employee email addresses (e.g. reviewer@peanut.me) of everyone who creates a collection. Fix: strip or hash createdBy before persisting the manifest that is served publicly (keep it only in a private audit object such as collection-requests/), or omit it from the public viewer payload.
Checked clean
- Confirmed the detached worktree head and merge base exactly match the supplied SHAs.
- Read the trusted pull request metadata and exact-head checks; ci-success, unit, format, eslint, typecheck, screen-tests, native-export, and deployment preview passed, while screenshot captures were still in progress.
- Verified through the repository API that the default branch is main and screen-library-collection.yml is absent there.
- Verified Cloudflare's current Access contract for Worker identity, service-token identity fields, service bindings, and the requirement that service tokens match a Service Auth policy.
- Checked Access authorization, generated Worker bundles, hosted CLI headers, GitHub dispatch failure recovery, stale queued/running retries, conditional R2 transitions, and attempt-bound completion.
- Checked collection ordering, locale coverage refresh, incomplete-entry rendering, the moved 404 lint boundary, and exact-head formatting.
- Focused dependency-free tests passed. Two local suites could not start because the read-only worktree has no installed pngjs dependency; the exact-head screen-tests check passed those repository tests.
Security review by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 80008ff8691a · Context: repo, github, cloudflare-docs · Took 12m
There was a problem hiding this comment.
Chip review — changes requested
One blocking defect remains: missing-state captures cannot dispatch because the new workflow is absent from the repository's default branch.
Findings
- BLOCKING · scripts/screens/collection-worker/index.mjs:153 · Register the capture workflow on the default branch
The repository default branch is main, but this new workflow exists only in a PR targeting dev. GitHub only registers workflow_dispatch workflows that exist on the default branch, so this filename-based dispatch returns an error after the PR merges to dev and every collection with missing states is left failed instead of being captured. Put a dispatchable workflow on main (or use an already registered default-branch workflow) before enabling this call.
Checked clean
- Verified the exact head and merge base; the worktree is clean.
- Checked Access identity validation, service-token handling, internal service-binding authentication, and both deployed Worker bundles.
- Checked capture dispatch failure recovery, stale queued/running retries, conditional transitions, attempt-bound completion, and partial completion.
- Checked hosted CLI Access headers, local capture.json directory input, collection viewer locale coverage, and missing-entry rendering.
- Checked the exact-head CI results: all completed checks pass; capture matrix and ds-shots jobs remain in progress.
- Confirmed from official Cloudflare documentation that ctx.access supplies verified identity and service-token fields, and from official GitHub documentation that workflow_dispatch requires the workflow on the default branch.
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 8c43b9bd287e · Context: repo, web · Took 15m
1 participant
Summary
/collections/<id>/pagesubuntu-24.04, keep PNG inside CI for exact validation, and publish only 393 x 852 WebP assetsdevcapturesArchitecture
search_screens,create_collection,get_collection_status, andcapture_missing_states; it reaches the collection service only through a service bindingValidation
pnpm screens:test(122 passing after merging currentdev)pnpm typecheckactionlint .github/workflows/screen-library-collection.ymlnode scripts/native-build.js)Setup after merge
SCREEN_LIBRARY_COLLECTION_API_URLandSCREEN_LIBRARY_MCP_URLrepository variables using custom HTTPS originsGITHUB_ACTIONS_TOKENon the collection Worker and the same randomCOLLECTION_SERVICE_TOKENon both new WorkersCLOUDFLARE_API_TOKENpermissions to deploy both Workers and custom domains@peanut.meallow rule; configure the MCP Access application for remote MCP authenticationNo Worker has been deployed by this PR.