Discovery foundations review — 2026-09-19

Verdict: changes requested. Five actionable findings: two P1, three P2. The cursor core and published-content embedding work substantially implement the first milestone, but mobile virtualization, the open-source export, and three integration behaviors need correction. The transport flag being off does not disable the frontend or profile-cache changes.

Reviewed the complete tracked diff and untracked discovery/embedding implementations and tests against docs/superpowers/plans/2026-09-19-discovery-foundations.md, at HEAD 04bc2500797ac6603ca12335b3a253497764be2e. Read worktree AGENTS.md, the embedding implementation report, and the final discovery release report. Also inspected the newly added HTTP integration test, its isolated-test bootstrap registration, and the synthetic benchmark. This review changes only this requested report: no implementation edits, commits, pushes, browsers, subagents, external requests, or production access.

Actionable findings

1. P1 — Use the document scroll owner for mobile virtualization

Evidence: discovery-grid.tsx:16, especially lines 16–19 and 31–38, always chooses the hub-main element and passes it to useVirtualizer. However, mobile-viewport.ts:127 enables document scrolling on phones, and globals.css:4733 makes .hub-layout-scroll content-height with overflow: visible. page-scroll.ts:2 already exposes this distinction.

Exact scenario: On a phone, load more than 96 cards. The virtualizer observes the full-height content panel as its viewport and listens to that panel rather than window scroll. It can mount the entire accumulated catalog instead of a viewport-sized subset. This loses the memory/DOM bound precisely on long mobile visits, even with the cursor flag off.

Local reproduction: Ran the actual DiscoveryGrid in jsdom, with the existing mobile document-scroll marker, a 390px window, and a content-height hub panel. For 1,725 items, 1,725 cards mounted. Dispatching window scroll at 15,000px did not change the rendered range. This is a DOM-only fixture, not a browser measurement. The submitted test covers only a fixed-height element scroller.

Action: Select window/document virtualization when isDocumentPageScroller is true, with the correct document scroll margin; retain element virtualization on desktop. Handle viewport-mode transitions. Add a document-scroll regression asserting a bounded mounted count and a changed range after window scrolling.

2. P1 — Update the open-source export boundary for the new modules/tests

Evidence: discovery-feed.ts:9 imports fetchDiscoveryCandidatesByIds and fetchDiscoveryCatalogBatch. manifest.json:126 replaces recommendations with the local stub, which exports neither function. The new discovery files are not excluded, and routes/worlds.ts imports the feed unconditionally. Independently, embedding-backfill.test.ts:5 imports ../../scripts/embed-worlds.js, while manifest.json:45 excludes that script but retains this test.

Exact scenario: Commit these new files and run the required OSS export verification. The exported server contains imports of missing recommendation exports and a retained test referencing a missing script. Hosted typecheck/build passing cannot catch these export substitutions. These are build-time failures regardless of DISCOVERY_CURSOR_ENABLED.

Verification: A local TypeScript AST comparison confirmed both missing stub exports. The script/test exclusion mismatch was checked against the manifest and server tsconfig.json (include: ["src"]). I did not generate/export a second tree during this read-only review. Exporting the current untracked tree would also be insufficient evidence: the exporter copies only tracked files.

Action: Route/stub the discovery implementation appropriately for the local edition, and exclude the hosted backfill test or provide its supported dependency. Verify the complete prospective committed export, including new HTTP tests, rather than adding only enough exports to silence one diagnostic.

3. P2 — Give discovery its own IP rate-limit scope

Evidence: worlds.ts:654 creates ipRateLimitMiddleware(180, 60) without a scope. rate-limit.ts:240 therefore uses rl:ip:<ip>, the same key used by auth.ts:94 with a 40/minute threshold and the existing feed-event endpoint with a different threshold.

Exact scenario: With cursors enabled, one reader or several readers sharing a NAT make 40 successful discovery requests within a minute. A subsequent login/signup/reset request from that IP is rejected even if there were no preceding auth requests. Conversely, event traffic consumes discovery's allowance.

Local reproduction: Exercised the real Hono middleware in an isolated child with credentials stripped, dotenv disabled, and external fetch blocked: 40 discovery responses were 200; the next auth request was 429 with a 60-second retry. No real auth endpoint or external service was called.

Action: Supply a dedicated scope such as "discovery". Add a cross-endpoint regression proving discovery consumption does not reduce auth capacity. This requires no new rate-limiter architecture.

4. P2 — Invalidate the newly dismissal-bearing profile cache on dismiss and undo

Evidence: discovery-eligibility.ts:10 now includes dismissals in global exclusions. recommendations.ts:2178 copies those IDs/groups into the cached profile, which is returned unchanged on a cache hit at lines 2068–2089 and has a 300-second TTL. The dismiss/undo handlers at feed-events.ts:152 and feed-events.ts:177 invalidate engagement and feed caches, not the recommendation profile.

Exact scenarios:

• Dismiss a story, build a profile while it is dismissed, then undo and start a fresh Discover session before the profile expires. The authoritative SQL exclusion now permits it, but the stale profile still removes it and its group during ranking. Undo remains ineffective for up to five minutes, including on the cursor path.
• On the legacy path, cache EN and ZH profiles, dismiss the EN variant, then browse the cached ZH scope. The refreshed engagement dismissal set removes the EN ID only; the ZH profile has not acquired the newly excluded group, so its sibling can reappear. This path remains active with the flag off.

Action: Invalidate all language-scoped recommendation profiles on both dismissal mutations, or keep these mutable exclusions out of cached rank eligibility. Add the plan's missing cache-hit loader/route regression: the current SQL helper test does not exercise Redis profile reuse or undo.

5. P2 — Bound pages that add no visible cards, not just empty response arrays

Evidence: rec-tab.tsx:153 resets the empty-page counter whenever data.length > 0, before accounting for deduplication at line 146 or local Library/hidden filtering at lines 263–265. The observer at rec-tab.tsx:211 continues automatically while that counter stays below three.

Exact scenario: A visible sentinel receives nonempty legacy pages whose IDs are already in the rendered feed after score changes, or pages whose new IDs are all locally excluded. None adds a visible card, but every response resets the retry budget, so automatic requests continue until server exhaustion instead of stopping for the explicit continuation button.

Local reproduction: Rendered the actual RecTab control flow using in-memory import mocks and a visible IntersectionObserver. Ten legacy pages each contained the same 24 IDs with total: 240. The component made 10 HTTP requests, rendered 24 unique cards, and automatically consumed nine consecutive pages adding zero visible cards. No source/test file was modified.

Action: Count newly appended, currently visible unique cards when updating the automatic-scan budget. Stop after three zero-addition pages while retaining the manual button and cursor. Cover both duplicate-only and locally filtered pages at the component level; the seven existing focused tests cover the helpers and desktop grid, not this interaction.

First-milestone compliance and checks

Area	Assessment
Published greeting semantics	Substantially complete. Shared builder reads enabled greeting entries in engine order, honors authoritative entries over legacy fallback, bounds inputs, and hashes the versioned payload.
Publication concurrency/backfill	No concrete defect found in the changed paths. Approval/held-edit callers use committed RETURNING rows; service and backfill condition writes on publication and the input snapshot. Missing-only backfill rechecks embedding IS NULL. Dry-run defaults, explicit apply version, read-only CLI session, finite-vector checks, bounded batches, and omission of story bodies from errors are present.
Cursor CAS/retries	No concrete double-advance defect found. CAS checks the stored revision, the losing request reloads the committed page, and three retained pages preserve IDs/order/continuation. Replay rehydrates current eligibility. Actual shared-Redis timeout/ambiguous-commit behavior was inspected, not exercised against Redis.
Eligibility/group exclusions	Primary-DB publication/content/visibility/block predicates and global Library/favorite/review/dismissal group exclusion are reused for hydration, including replay. Search/tag bypass remains intact. Cached dismissal lifecycle has finding 4.
Finite reachability/diversity	Core tests traverse 1,725 items from a 247-item pool and consume excluded batches without false exhaustion. The added real Hono/PGlite test traverses 713 stories and covers private responses, replay, and guest ownership. Keyset batches are 200, at most three new scans/request; recent creator/topic history crosses pages.
Client async/error/restart	Generation checks, synchronous append exclusion, aborts, server continuations, explicit 409 restart and 503 retry paths are present. Error UI removes the automatic sentinel. Findings 1 and 5 prevent considering frontend continuity/resource handling complete.
Duplicate impressions	Parent opportunity ledger survives virtual row remounts; feed/world/position identifies a claim. Durable serve insert tolerates replay conflicts. I found no concrete duplicate client-impression defect in that path. Full future analytics identity/event semantics are outside this review.
Integration/release	Runbook and dry-run commands are present. Export integration needs finding 2. Representative concurrent HTTP/DB/Redis load remains an explicit pre-enable gate; pure CPU timing is not a capacity result.

Reported verification accepted as controller/worker evidence, not rerun wholesale here: 134 recommendation tests; seven focused app tests; 16 focused embedding tests / 41 with related publishing tests; the additional 713-story HTTP integration test; full typecheck/build passes. The final report also records a production read-only dry-run of 1,725 inputs with zero provider calls/writes. I read that evidence and made no production connection myself. Reviewer checks were the local reproductions and static export analysis described above.

Redis/resource limits relevant to rollout

The release report correctly acknowledges linear session history and defers expansion pending measurement. This review does not demand a new datastore or treat future ML/measurement/cooldown work as missing first-milestone code. Keep the documented load gate: the bound of three catalog scans is not a bound on total session serialization or total live session count.

• discovery-store.ts:10 decodes the entire previous JSON inside Redis Lua to inspect its revision; line 26 sends the entire new session. Exact/group histories are cloned, serialized, transferred, and decoded as they grow. Queues/replay pages are bounded; those histories are not fixed-size.
• Reviewer serialization measurements with UUID world IDs and one UUID group per world: 134,643 bytes at 1,725 delivered; 780,093 at 10,000; 7,800,093 at 100,000, excluding the rest of the session. These are generated data sizes, not Redis latency claims. The submitted 110,660-byte benchmark uses eight-character IDs and no groups, so it is a different fixture.
• discovery-session.ts:66 creates another one-hour key on every no-cursor request, including the same actor/scope. A reviewer 247-UUID initial-pool fixture stored 11,693 bytes after its first page. The configured rate alone permits 10,800 requests/hour/IP, or approximately 126 MB of session JSON for repeated first-page requests at that fixture size, before Redis overhead; this is a permitted-volume calculation, not observed production load. The production adapter has no equivalent of the development store's 100-session cap.

Before enabling beyond the limited rollout, include realistic IDs/groups, repeated session starts, concurrent users, Redis serialization/eviction, and origin retrieval cost in the stated load checks. Use the measured result to choose admission/session limits or a smaller atomic revision representation if needed. The current fixture does not establish those capacities. Initial candidate retrieval is also recomputed per new session; graph/stat/model caches do not constitute shared candidate-batch caching, so that planned optimization remains partial and should be tracked as such.

No future experiment assignment, label maturation, durable exposure cooldown, model-quality uplift, or new infrastructure is required by these findings. Keep the transport off while addressing the concrete regressions; the mobile and OSS issues still need correction before landing a deployable release with that flag off.