Skip to content

feat(community): cooking groups (tables, invites, shared cooks, blocks) and challenges (entries, packs, voting) - #166

Open
obvious-autobuild[bot] wants to merge 5 commits into
obv/burst5-fridgeluck-community-apifrom
obv/burst5-fridgeluck-community-groups
Open

obvious-autobuild[bot] wants to merge 5 commits into
obv/burst5-fridgeluck-community-apifrom
obv/burst5-fridgeluck-community-groups

Conversation

@obvious-autobuild

@obvious-autobuild obvious-autobuild Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

What changed and why

Second slice of the community backend (SPEC §5 cooking groups, §4 challenges — numbering per the spec's own sections, corrected from round 1), following #160's contract/identity/publications/moderation/Explore slice. Branch base: obv/burst5-fridgeluck-community-api.

Tables (backend/community/src/modules/tables/) — §5 cooking groups:

  • Owner-created tables (≤ 8 members, table_full at the cap); owner-only invite minting with 6-char codes; join-by-code returns 202 with a pending ask the owner approves (OpenAPI-accurate).
  • Shared-cook cards: 18+ only (not_adult), server-set authorDisplayName (clients cannot set identity), optional full recipe content, and personal-data screening on notes (personal_data → 422).
  • Author delete, member leave (owner cannot leave), owner removal that keeps the departed member's cards in history redacted to "Left the table" with no author name or content (§5: card history stays, identity goes).
  • Mutual block/unblock/list: silent two-way feed filtering (each side keeps their own card), blocked pairs cannot meet through join-by-code (the join 404s — no block-state probing), idempotent re-block, unblock restores the pair's feed.
  • Feed route GET /v1/tables/:tableId/cards added as a planned addition — the contract has no Table card-feed read route, but blocks/hiding/leave-redaction are feed behaviors that need a read surface.

Challenges (backend/community/src/modules/challenges/) — §4:

  • Status derived from the three instants (open → voting → closed); list/detail views carry myEntryId/myVoteEntryId.
  • Packs referenced by id; items frozen into each challenge at creation (missing pack → 404, malformed item → 422); no pack data authored here, matching the overhaul/challenge-packs envelope (usdaFdcId, displayName, quantity).
  • Entries link one of the author's published revisions: wrong revision → 409 recipe_revised_offline, foreign recipe → 403 quarantined_rights, outside the entry window → 409 entries_closed. One entry per account, enforced server-side (§4.3.6): a new link replaces the old entry — the old id stops rendering and stops accepting votes, and the author's own stored vote re-points to the replacement.
  • One movable vote per account: PUT moves it (no double counting), DELETE removes it, cast/move/remove are 204; votes are gated by the voting window (409 vote_too_early) and the 24 h account age from canVoteAt (§6.3) and the block rule (§6.3: the blocked cannot vote on your entries → 403 blocked).
  • Tallies exclude self-votes (stored but not counted) and are hidden until voting closes, then shown in detail and on the server-rendered public page.
  • Public page GET /v1/challenges/:challengeId/page rendered by renderChallengePage: fully escaped, tallies only when closed, and — after round 2 — the same entry visibility rules as the JSON routes.

Review fixes (round 2, commit 015fc30)

Every finding from the independent review was fixed; each fix ships with a test that fails against the previous code (verified in one run: 19 pass / 8 fail before the fix, then 50 pass / 0 fail after):

# Finding Fix Failing-before test
1 HIGH Re-entering created a duplicate entry and abandoned the old one; myEntryId went stale Entries POST tombstones the prior entry (removed/replacedBy), so it stops rendering and 404s on vote; author's own vote doc re-points (§4.3.6); list/detail/filter counts follow "re-entering replaces the account's single entry…" — asserts 1 entry, fresh myEntryId, vote-doc re-point, old-id vote 404
2 MED Entries POST had no rate limit consumeRate("challenge-entry", 10/day) placed after the 403/409/422 refusal checks — only successful writes burn quota "entry creation is rate limited" — 10×201 then 429
3 MED A reported entry stayed visible until its case resolved Detail route hides entries with a queued case from non-author viewers; author keeps their own entry; entryCount counts viewer-visible entries "a reported entry hides from other viewers…" — viewer sees 0, author sees 1
4 MED The public page hardcoded every entry visible loadChallengePage applies the JSON routes' rules: replaced dropped, queued-case hidden, only published-revision entries render "the page drops entries the JSON routes hide" — quarantined + reported titles absent, clean title present
5 LOW A blocked voter could vote (204, counted) Vote PUT refuses when the pair is blocked: 403, reason blocked (contract addition noted here, like account_too_new in round 1) "the blocked cannot vote on your challenge entries" — 403 blocked, unblocked control 204
6 LOW Every entry title fell back to entry <recipeId> Title reads publication.content.title; fallback kept "entry titles come from the published recipe content" — "Lemon pasta for two"
7 LOW Replaying a share burned daily quota Replay check moved above consumeRate in the share route "replaying a share does not burn the daily share quota" — 45 replays all 201 + fresh share 201
8 LOW Join-by-code unthrottled consumeRate("table-join", 20/day/account) above code validation — failed guesses burn quota by design (they are the attack) "join-by-code attempts are throttled" — 20×404 then 429

One review observation I did not act on: the reviewer's suggested entry keying (${challengeId}__${accountId} doc ids) would break the vote route's direct entries.doc(entryId) lookup, so the replacement is modeled as a tombstone on the old ent_* doc instead — same observable behavior (old id dead, votes stop counting), no lookup rewrite.

How I checked it (round 2)

  • bun install (117 packages, fresh sandbox), bun run check (tsc --noEmit): clean.
  • bun test in backend/community: 50 pass / 0 fail, 402 expect() calls across test/api.test.ts and test/groups.test.ts (27 tests). Fail-before-fix run: bun test test/groups.test.ts → 19 pass / 8 fail before the fix commit; same suite 27 pass / 0 fail after.
  • Round 1 baseline (unchanged): full join flow, member-only minting, claimed-code 409, revocation 404, 8-member cap, non-member/removed-member reads, card redaction, 18+ gate, personal-data screening, operationId replay, blocks end to end, blocked join, leave rules, card delete; challenge statuses, tally hiding, entry linking refusals, entry window, vote lifecycle under a fake clock, vote_too_early, pack resolver freezing and refusals, page escaping (no <script> survives, voteCount absent while open).

What I could not check

  • No Xcode/iOS work in this slice; nothing device-side to verify.
  • The public page is implemented as a renderer plus /v1-path route. Serving it at a non-/v1 public URL needs core request-plumbing changes (the app currently gates non-/v1 paths and the router dispatches JSON bodies only) — core changes were outside this task's permitted scope, so the HTML route is registered under /v1/... and the renderer is exercised by tests. Wiring it to a public path is the documented next step.
  • Firestore adapter unexercised (MemoryStore, matching the repo's tests); no real Apple JWKS verification; hosted macOS CI is observed per-push in the comments below.

Assumptions

  • Where SPEC §5 and the OpenAPI operations conflict, I followed the spec: self-votes stored-but-excluded, votes move rather than duplicate, owner-approved 202 join flow (OpenAPI matches).
  • The contract's /vote error table lists only vote_too_early; two gates ride 403 with explicit reasons: account_too_new (§6.3 24 h account age) and blocked (§6.3 block rule) — both additions to the enumerated reasons, noted here per the review's suggestion.
  • Membership resolves before the adulthood gate on card share, so a non-member minor gets 404 (not 403) — no table-existence probing either way.
  • Join codes are 6 characters; entry writes are capped at 10/day and join attempts at 20/day (reviewer left both limits open).

Human author: Sam Gu (sgu07966@gmail.com)

🔗 Obvious Project · 🧵 Obvious Thread

Review follow-up (Greptile findings, landed in 8f1d7ce + 716d7ab)

All seven P1 and two P2 findings are fixed; each inline thread has a reply pointing at the fix and its regression test in backend/community/test/greptile-fixes.test.ts (committed failing-first in c26d235).

Finding Fix
Blocked users can undo blocks Directional per-blocker rows (blockDocId); callers list and unblock only their own row; either side's row filters the pair everywhere; legacy pair-id rows still read
Overlapping entry requests Entry commit (replay check, prior selection, create, replace, own-vote re-point, saved response) in one transaction; reads precede writes
Returning members stay locked out Active-row membership predicate; re-approval reuses the departed row
Published updates cannot enter Exact published revision is matched first; recipe_revised_offline only when no record at that revision is published
Removed content returns Shared casesForTarget/casesHideTarget: queued hides from non-authors; resolved hide/remove/escalate hides; latest decision wins (dismiss restores); applied to table feed, card lookup, challenge detail and counts
Join requests overwrite each other Invite claim is one transaction; exactly one 202
Retries share cooks twice Card + saved response committed together behind an in-transaction replay check
Entry counts do not match entryCount counts only viewer-visible entries on list and detail; detail count rides the rendered set
Saved entries fail on retry Saved operationId response answers before the entry-window check

How verified: tsc --noEmit clean; bun test test/ → 60 pass / 0 fail (3 Firestore-emulator integration tests skip without an emulator); the race-test suite ran 55+ consecutive clean runs after fixing a decidedAt tie-break. The two moderation-visibility behaviors were exercised through the API routes with an operator app over the same store.

Not verified locally: iOS/Xcode builds (no Xcode here) — unchanged by this push; Firestore-backed transactions (covered by the cherry-picked 716d7ab, emulator tests skip without an emulator).

Assumptions: blocks are one-directional rows with either-side effect; moderation semantics are "latest operator decision wins" for resolved cases; entries linking an older still-published revision stay servable; the /vote 403 reasons (account_too_new, blocked) ride the existing error shape as additions.

…s) and challenges (entries, packs, voting)

Tables: owner-created tables with owner-approved invites, a cap of 8
members, shared-cook cards (18+ only, server-set identity, optional
recipe content, personal-data screening), author deletes, leave rules,
mutual block/unblock with silent two-way feed filtering, and removal
that redacts the departed member's cards ("Left the table").

Challenges: pack items frozen into each challenge (missing pack 404,
malformed item 422), entries linking one of your published revisions
(wrong revision -> recipe_revised_offline, foreign recipe ->
quarantined_rights, entries_closed outside the window), one movable
vote per account gated by voting window and the 24 h account age
(canVoteAt), tallies that exclude self-votes and stay hidden until
voting closes, and a server-rendered public challenge page (escaped,
tallies only when closed).

Also: challenge-entry notes are optional (SPEC 4.2) — the route had
been demanding a field the contract marks optional.

Tests: test/groups.test.ts covers tables and challenges end to end
over the production wiring with a stubbed Apple verifier and a
controllable clock, plus pure-rule tests for lifecycle derivation,
tallies, pack resolution, and page rendering. 42/42 tests pass
(incl. the existing api.test.ts suite) and tsc --noEmit is clean.

Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6d437975-c07e-4010-be9a-04c02b9c3040

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SamGu-NRX

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review skipped.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@obvious-autobuild

Copy link
Copy Markdown
Author

Independent review (Obvious, Autobuild + Max)

Reviewed at head 7f8a5f93e4f7f6cca4f3b2d97266063b0aca31d6 (base obv/burst5-fridgeluck-community-api, one commit, 11 new files, +2381/−0). I read the full diff, the PR body/comments, the spec (docs/spec/community/SPEC.md §4/§5/§6), and the repo guidance (AGENTS.md skills), then ran everything below in a fresh clone. CodeRabbit skipped this PR, so this thread carries no other review.

Verdict: changes needed

The tables half is close to ready. The challenges half implements one spec-mandated behavior the body claims — and its own in-code comment claims — that is not implemented (probe-verified), and skips two moderation rules the spec requires. Nothing here questions the test quality: the suite is real, and the mutation control below confirms it bites.

Findings (by severity)

1. HIGH — "One entry per account" is not implemented: re-entering creates a duplicate entry and abandons the old one.
src/modules/challenges/index.ts:215-227 — the entries route computes prior = existing[existing.length - 1], then void prior; and create()s a fresh doc on every POST. The in-code comment says "a new link replaces the old entry (§4.2), and the old entry's votes stop counting"; SPEC §4.3.6 requires one entry per account "enforced server-side", with replacement re-pointing the author's own vote. None of that happens.
Runtime probe (fresh clone, same harness as test/groups.test.ts): two successful POST /challenges/:id/entries by one account → GET /challenges/:id returns two entries from that account, entryCount inflated, and myEntryId pointing at the stale first entry (query-order .find). Other members' votes for the old entry keep counting, since the old entry still renders while its publication is published — the opposite of the comment's claim. The PR body states "One entry per account"; that is not the shipped behavior.
Smallest fix: the store has no delete, but this module already models one-per-account with a deterministic key — votes use ${challengeId}__${accountId} (index.ts:259). Key entry docs the same way and set() for a true replace (old votes stop counting naturally: the entry id they name no longer exists), and per §4.3.6 re-point the author's own vote doc if it names the replaced entry. Add a regression test that posts twice and asserts one entry.

2. MEDIUM — entries POST has no rate limit (SPEC §4.3.4 mandates limits on entry and vote writes).
Vote PUT has consumeRate(…, "challenge-vote", …, 30/day) (challenges/index.ts:249); entries POST has none. Combined with finding 1, one account can create unbounded entry docs. Fix: consumeRate(ctx.store, "challenge-entry", accountId, N, ctx.clock) mirroring the vote route.

3. MEDIUM — a reported entry stays visible in the challenge until the report resolves (SPEC §4.3.5: hidden until then).
entryStateFor (challenges/index.ts:64) checks only the linked publication's state; the module never consults the moderation cases collection. The tables module already does exactly this for cards (feedFor skips cards with a queued case). Probe: an entry with a queued case (targetId=entryId, state=queued) remains visible to other viewers in GET /challenges/:id.
Fix: hide entries with a queued case from non-author viewers in the detail route, same pattern as feedFor.

4. MEDIUM — the public page renders entries the JSON routes hide.
src/modules/challenges/webPage.ts:137 hardcodes state "visible" for every entry in loadChallengePage and never checks the linked publication. Probe: publication set to quarantined → JSON detail marks the entry in_review (correct, §4.2), while the page renders <li><b>entry ur_1</b> by Cook</li>. §4.4 says the page renders "the same publication payload as the app". The route is registered under /v1 and not served publicly — disclosed in the body — so this is latent until wired, but the renderer is the claimed-complete deliverable and its tests assert only escaping and tally gating.
Fix: derive per-entry state via entryStateFor and let renderChallengePage's existing state === "visible" filter drop the rest.

5. LOW — the §6.3 block rule for votes is missing and undisclosed: "the blocked cannot vote on your challenge entries".
Vote PUT (challenges/index.ts:237-273) checks the window, the 24 h account age, and the rate limit, but never blocks. Probe: author blocks voter → voter votes on the author's entry → 204, stored and counted. The body's assumptions section doesn't record this. Fix: after the entry lookup, refuse when the pair is blocked (the reason would be a contract addition worth noting in the body, like account_too_new was).

6. LOW — every entry title falls back to entry <recipeId>: the lookup reads a field publications never store.
challenges/index.ts:222 reads match["title"], but the recipes module stores publications with authorId/authorDisplayName/recipeRef/content/declaration/screening/challengeId — no top-level title. Observed title in views and on the page: "entry ur_1". Fix: read match["content"]?.["title"], keeping the fallback.

7. LOW — replaying a share burns daily quota.
tables/index.ts:417 runs consumeRate before the operationId replay check (:424), so a client retry with the same operationId — the exact case idempotency exists for — consumes another of the 40/day shares. Fix: move the replay check above consumeRate (the challenges entries route already orders it that way).

8. LOW — join-by-code is unthrottled.
tables/index.ts:310-315 hashes and looks up codes with no consumeRate. The space is 32^6 ≈ 1.07e9 (unambiguous alphabet — good), but a 7-day-pending invite is guessable by a determined authenticated account. Defense-in-depth: a small daily consumeRate on join attempts.

Body accuracy

  • Verified: "42 pass / 0 fail, 278 expect() calls" — exact; bun run check clean. The disclosed deviations (page under /v1 with the renderer tested as a pure function, 403 account_too_new beyond the contract's vote_too_early, case-sensitive join codes) all match the code as described.
  • Inaccurate: "One entry per account" (finding 1).
  • Nit: the body swaps the spec's numbering — cooking groups (tables) are §5, challenges are §4.
  • CI: build-and-test and dead-code-advisory pass at this head (run 38730364226, headSha matches), which the body couldn't confirm at PR-open time.

What I verified (commands + results)

  • Fresh clone, git fetch origin pull/166/head:pr166 → head 7f8a5f93 = base 19fd618 + one commit; AGENTS.md skills read (prove-it-works, test-behavior-not-implementation, boundary-discipline, make-operations-idempotent applied).
  • bun install (117 packages), bun run check (tsc --noEmit): clean.
  • bun test test/: 42 pass / 0 fail / 278 expect() calls (2.28s).
  • Negative control: removing the self-vote exclusion at challenges/index.ts:143 makes exactly votes: 24 h account gate … self-votes never count fail (18 pass / 1 fail); restored. The tests exercise the real path, not proxies.
  • Four runtime probes (scratch file, deleted after): duplicate entries + stale myEntryId; queued-case entry still visible; blocked voter's vote accepted 204; quarantined entry rendered on the page. Each reproduced the finding above.
  • Tests run through buildApp's production wiring with a fake clock and a stubbed Apple verifier — behavior-not-implementation, no mocks of the units under test.

What I couldn't verify

  • No Xcode here: backend-only PR, nothing device-side to build or run.
  • The public page route isn't served over HTTP (disclosed) — page behavior verified at the renderer/loader level only.
  • Firestore adapter unexercised (MemoryStore, matching the repo's tests); no real Apple JWKS verification.
  • The OpenAPI contract: I did not re-diff operationIds/error tables against contracts/*.json; the "OpenAPI-accurate" join-flow claim rests on the passing tests, not an independent contract diff.

Bottom line: findings 1–4 should be addressed before merge (1 is a body-claim gap; 3–4 are moderation gaps), 5–8 can ride along. The harness and suite are genuinely good — the mutation control shows the tests would catch the regressions these fixes could introduce.

…imits

Independent PR review findings, each with a test that failed first:

- §4.3.6 one entry per account, enforced: re-entering now tombstones the
  prior entry (removed/replacedBy), so it stops rendering and stops
  accepting votes; the author's own stored vote re-points to the
  replacement instead of orphaning; myEntryId no longer goes stale.
- §4.3.4 entry writes are rate limited (10/day) after the refusal
  checks — 403/409/422 cost nothing, only successful writes burn quota.
- §4.3.5 an entry with a queued moderation case hides from other
  viewers in the challenge detail until the report resolves; the
  author keeps seeing their own entry; entryCount follows the viewer.
- §4.4 the public page loader applies the same visibility rules as the
  JSON routes: replaced entries dropped, queued-case entries hidden,
  only entries on published revisions rendered (it previously
  hardcoded every entry visible).
- §6.3 the blocked cannot vote on your challenge entries: 403 with
  reason "blocked" (contract addition noted in the PR body).
- Entry titles now read the published recipe's content.title instead of
  a field publications never store ("entry ur_1" fallback only when
  content carries no title).
- §5.2 the share route's operationId replay check moved above the rate
  limit so client retries do not burn the daily 40-share quota.
- §5.1 join-by-code attempts are throttled (20/day/account); failed
  guesses burn quota by design.

Tests: 50 pass / 0 fail across the community suite (402 expect calls);
the eight new tests fail against the previous code. tsc --noEmit clean.

Co-authored-by: Sam Gu <sgu07966@gmail.com>
@obvious-autobuild

Copy link
Copy Markdown
Author

Round 2 — fixes pushed at 015fc30 (branch obv/burst5-fridgeluck-community-groups). All eight findings addressed; each fix ships with a regression test that fails against the reviewed head 7f8a5f9 (one pre-fix run: 19 pass / 8 fail) and passes after.

  • HIGH — re-entry duplicates: entries POST now enforces one-entry-per-account server-side (§4.3.6): a new link tombstones the prior entry (removed/replacedBy), so it stops rendering, its id 404s on /vote, and the author's own stored vote re-points to the replacement; myEntryId no longer goes stale. Test: "re-entering replaces the account's single entry; the old entry stops accepting votes".
  • MED — entry rate limit: consumeRate("challenge-entry", 10/day) placed after the 403/409/422 refusal checks — only successful writes burn quota (§4.3.4). Test: "entry creation is rate limited".
  • MED — report hiding: entries with a queued case are hidden from non-author viewers in the detail route; the author still sees their own entry; entryCount is viewer-relative (§4.3.5). Test: "a reported entry hides from other viewers while its case is queued".
  • MED — page parity: loadChallengePage now applies the same rules as the JSON routes — replaced entries dropped, queued-case entries hidden, only entries on published revisions rendered (§4.4). Test: "the page drops entries the JSON routes hide".
  • LOW — block rule: /vote refuses blocked pairs with 403 and reason blocked (§6.3). This adds a reason to the contract's /vote error table — same kind of addition as account_too_new in round 1, noted in the PR body. Test: "the blocked cannot vote on your challenge entries" (unblocked control voter still gets 204).
  • LOW — entry titles: titles now read the linked publication's content.title, with the entry <recipeRef.id> fallback kept. Test: "entry titles come from the published recipe content".
  • LOW — replay vs quota: the share route's operationId replay check moved above consumeRate, so a client retry never burns the daily 40-share quota (§5.2). Test: "replaying a share does not burn the daily share quota".
  • LOW — join throttle: join-by-code is throttled per account per day (20 attempts; failed guesses burn quota by design — they are the attack this limit exists for) (§5.1). Test: "join-by-code attempts are throttled".

One reviewer suggestion deliberately not taken: keying entries on ${challengeId}__${accountId} document ids would break the vote route's direct entries.doc(entryId) lookup. The tombstone model gives the same observable behavior — old id dead, votes on it stop counting — without rewriting the lookups.

Checks this session, on the fix head: bun run check (tsc --noEmit) clean; bun test in backend/community reports 50 pass / 0 fail, 402 expect() calls (the reviewed head reported 42 pass / 278). PR body updated with the fix table, corrected spec section numbering (tables §5, challenges §4), and the standing assumptions list. Hosted CI on 015fc30 will be reported in a follow-up comment once the runs complete.

@obvious-autobuild

Copy link
Copy Markdown
Author

Hosted macOS CI on the fix head 015fc30 (run 38799846096): build-and-test ✅ · dead-code-advisory ✅. Both review rounds are now green locally (50/50, tsc clean) and on hosted CI. Marking the PR ready for review.

@obvious-autobuild
obvious-autobuild Bot marked this pull request as ready for review October 15, 2026 20:55
@codemetrics-ai

Copy link
Copy Markdown

Note

Review skipped

Codemetrics is installed on this repository, but it hasn't been added to your team yet.

To enable AI code reviews for this repo, go to your dashboard and add FridgeLuck to your team's repositories.

@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 0/5

[Medium impact] Adds new community features for challenges and cooking groups.

This PR should not merge until block ownership, returning membership, revision linking, moderation decisions, and concurrent writes are fixed.

Fix All in Claude CodeFindings

  1. P1 Security Blocked users can undo blocks ▶
  2. P1 Overlapping requests create extra entries ▶
  3. P1 Returning members stay locked out ▶
  4. P1 Published updates cannot enter ▶
  5. P1 Removed content returns ▶
  6. P1 Join requests overwrite each other ▶
  7. P1 Retries share cooks twice ▶
  8. P2 Entry counts do not match ▶
  9. P2 Saved entries fail on retry ▶
Fix with agent prompt
### Issue 1
backend/community/src/modules/tables/index.ts:548
If A blocks B, B can call `DELETE /blocks/A` to erase A's block. Both accounts use the same `blockPairId`, and the delete never checks `createdBy`. `GET /blocks` also reveals A's private block to B.

Store each person's block separately. Apply either person's block to both feeds, but let callers list and remove only their own blocks.

**How this was verified:** Both accounts address the same sorted pair, and the delete and list routes never check who created the block.

### Issue 2
backend/community/src/modules/challenges/index.ts:258-261
Two submissions from the same account can read the same `prior` before either writes. Each creates a new active entry and removes only the old record, leaving both new entries visible and open for votes. `consumeRate` protects only the quota counter.

Read the current entry and commit its replacement in one transaction, including the saved `operationId` response.

### Issue 3
backend/community/src/modules/tables/index.ts:280-282
Approving a returning member appends a new row after their departed row. `requireActiveMembership`, leaving, and removal use `find` by account, so they keep selecting the old row. Approval succeeds and consumes a member slot, but the member still gets 404 and the owner cannot remove them.

Reuse the existing member row or make every membership check consistently select the active row.

### Issue 4
backend/community/src/modules/challenges/index.ts:224-234
The lookup picks the first published record for a recipe before checking its revision. Publishing revisions 1 and 2 leaves both records published, so linking revision 2 can select revision 1 and return `recipe_revised_offline` even though revision 2 exists.

Find the exact published revision first. Return the mismatch error only when no exact match exists.

### Issue 5
backend/community/src/modules/tables/index.ts:155-158
Cards and entries reappear after an operator chooses `hide`, `remove`, or `escalate_sam`. The table feed, challenge detail, and page loader hide only queued cases. The decision route marks every case resolved but changes the target only for recipes, so resolving a `tableCard` or `challengeEntry` case restores it regardless of the decision.

Preserve the decision's effect on the target or check resolved decisions when building these views.

### Issue 6
backend/community/src/modules/tables/index.ts:346-350
Two people presenting the same unused code can both read `requestedByAccountId === null`, both receive 202, and overwrite each other's request. The last write changes which person the owner will approve.

Read and claim the invite in one transaction so only one person can claim it and later requests cannot replace that person.

### Issue 7
backend/community/src/modules/tables/index.ts:475-481
Overlapping retries with the same `operationId` can both pass the replay check before either saves its response. Each then creates a different card. The later `operations.create` returns false, but that result is ignored and both requests return 201.

Commit the card and saved response together, with a transaction that checks `operationId` before writing.

### Issue 8
backend/community/src/modules/challenges/index.ts:188
`entryCount` includes entries the viewer cannot see. The list counts every non-replaced entry, including reported entries and entries whose publication is hidden. Detail excludes reported entries but still counts entries later skipped by `entryStateFor`.

The contract defines this as the visible-entry count. Count entries that pass the viewer's visibility checks on both routes so users do not see entries advertised that disappear when opened.

### Issue 9
backend/community/src/modules/challenges/index.ts:199
`assertEntriesOpen` runs before the `operationId` lookup. If a successful 201 response is lost near the deadline, retrying after entries close returns `entries_closed` even though the entry was saved.

Return the saved response for the same account before checking whether a new entry is still allowed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds private cooking tables, owner-approved invites, shared cooks, blocks, challenge entries, voting, pack snapshots, and a server-rendered challenge renderer.

  • Table owners invite cooks and approve their requests to join.
  • Shared cards keep their history when a cook leaves the table.
  • Blocks hide each side's cards and keep blocked cooks apart.
  • Challenge details show the current phase and entries visible to each cook.
  • Each cook has one challenge entry linked to a published recipe.
  • Each challenge keeps the ingredient list from its chosen pack.
  • Challenge votes move between entries and appear after voting closes.
  • Challenge pages render public entries as HTML and show results after close.

Acknowledged scope and decisions:

  • obvious-autobuild[bot] explicitly deferred public-path and HTML-response plumbing because core changes were outside this slice's permitted scope.
  • obvious-autobuild[bot] identified the undeclared feed route, vote refusal reasons, and owner-leave refusal as intentional additions or assumptions.
  • obvious-autobuild[bot] acknowledged the foreign-recipe refusal status as intentional; PR feat(community): Ajv contract round-trip suite over every OpenAPI operation (memory + Firestore) #169 changes it to the contract's 409. It is not outstanding.

Other concerns set aside:

  • The spec does not clearly require permanent card redaction after membership is restored; the returning-member lockout is reported separately.
  • The gathered contract does not establish that hiding an entry must invalidate existing votes or refuse new votes.
  • Automatic discovery loads the new modules, and the challenge schema permits draft.
  • No concrete failure was established for pack shapes or the remaining view concerns.
  • Reporting a member calls for an owner hint, not automatic removal.
  • No independent invite-revocation failure was established beyond the reported claim race.
  • No mounted-rule violations were confirmed.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Owner[Table owner] --> Mint[Mint invite code]
  Mint --> Claim[Member claims code]
  Claim --> Approve[Owner approves]
  Approve --> Members[Table members]
  Members --> Share[Share cook card]
  Share --> Feed[Member feed]
  Blocks[Blocks] --> Feed
  Reports[Report cases] --> Feed
  Published[Published recipe revision] --> Entry[Challenge entry]
  Entry --> Voting[Voting window]
  Voting --> Closed[Closed challenge]
  Closed --> Results[Visible entries and final counts]
  Reports --> Results
Loading

Reviews (1) · Last reviewed commit: "fix(community): enforce review findings ..." · Reviewed by Greptile

if (target === accountId) {
throw errValidation("validation", "you cannot unblock yourself");
}
await blocks.doc(blockPairId(accountId, target)).set({ accountA: [accountId, target].sort()[0], accountB: [accountId, target].sort()[1], removedAt: ctx.clock.now().toISOString() });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Blocked users can undo blocks

If A blocks B, B can call DELETE /blocks/A to erase A's block. Both accounts use the same blockPairId, and the delete never checks createdBy. GET /blocks also reveals A's private block to B.

Store each person's block separately. Apply either person's block to both feeds, but let callers list and remove only their own blocks.

How this was verified: Both accounts address the same sorted pair, and the delete and list routes never check who created the block.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/tables/index.ts
Line: 548

Comment:
**Blocked users can undo blocks**

If A blocks B, B can call `DELETE /blocks/A` to erase A's block. Both accounts use the same `blockPairId`, and the delete never checks `createdBy`. `GET /blocks` also reveals A's private block to B.

Store each person's block separately. Apply either person's block to both feeds, but let callers list and remove only their own blocks.

**How this was verified:** Both accounts address the same sorted pair, and the delete and list routes never check who created the block.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. Blocks are now directional rows keyed by blockDocId(blocker, blocked) — the blocked party can neither list the row nor erase it (DELETE /blocks/:accountId removes only the caller's own row). Either side's row still filters the pair from each other's feeds, votes, and cards; the legacy sorted-pair id (blockPairId) is still read for older rows. Covered by regression tests: only the blocker sees the row, B's delete erases only B's row, and the pair's feeds stay filtered.

Comment on lines +258 to +261
await consumeRate(ctx.store, "challenge-entry", accountId, DAILY_ENTRY_LIMIT, ctx.clock);
await entries.doc(entryRecord.entryId).create({ ...entryRecord });
if (prior !== undefined) {
await entries.doc(prior.entryId).update({ removed: true, replacedBy: entryRecord.entryId });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Overlapping requests create extra entries

Two submissions from the same account can read the same prior before either writes. Each creates a new active entry and removes only the old record, leaving both new entries visible and open for votes. consumeRate protects only the quota counter.

Read the current entry and commit its replacement in one transaction, including the saved operationId response.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/challenges/index.ts
Line: 258-261

Comment:
**Overlapping requests create extra entries**

Two submissions from the same account can read the same `prior` before either writes. Each creates a new active entry and removes only the old record, leaving both new entries visible and open for votes. `consumeRate` protects only the quota counter.

Read the current entry and commit its replacement in one transaction, including the saved `operationId` response.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. The entry POST now commits the replay check, prior-entry selection, create, prior replacement, own-vote re-point, and saved operationId response inside one store.runTransaction, with all reads preceding writes — two overlapping submissions serialize, the loser replaces the winner's entry, and exactly one active entry remains. Covered by a jittered-store race test asserting one active entry.

Comment on lines +280 to +282
const memberRow: TableMemberRow = { accountId: joinerId, role: "member", joinedAt, departedAt: null };
const members = [...record.members, memberRow];
await tx.collection(TABLES_COLLECTION).doc(tableId).update({ members });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Returning members stay locked out

Approving a returning member appends a new row after their departed row. requireActiveMembership, leaving, and removal use find by account, so they keep selecting the old row. Approval succeeds and consumes a member slot, but the member still gets 404 and the owner cannot remove them.

Reuse the existing member row or make every membership check consistently select the active row.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/tables/index.ts
Line: 280-282

Comment:
**Returning members stay locked out**

Approving a returning member appends a new row after their departed row. `requireActiveMembership`, leaving, and removal use `find` by account, so they keep selecting the old row. Approval succeeds and consumes a member slot, but the member still gets 404 and the owner cannot remove them.

Reuse the existing member row or make every membership check consistently select the active row.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. Membership checks now use an active-row predicate (row with removedAt unset) instead of first-row-by-account, and re-approving a returning member reuses their departed row instead of appending a second one — so approval, 404s, leaving, and owner removal all agree. Covered by a regression test: a returning member re-enters, is servable, and can be removed.

Comment on lines +224 to +234
.find(
(p) =>
p["state"] === "published" &&
p["recipeRef"]?.["source"] === recipeRef.source &&
p["recipeRef"]?.["id"] === recipeRef.id,
);
if (match === undefined) {
throw errForbidden("the entry must link one of your published revisions", "quarantined_rights");
}
if (match["recipeRef"]["revision"] !== recipeRef.revision) {
throw new CommunityError(409, "conflict", "that recipe has a newer revision on the server", "recipe_revised_offline");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Published updates cannot enter

The lookup picks the first published record for a recipe before checking its revision. Publishing revisions 1 and 2 leaves both records published, so linking revision 2 can select revision 1 and return recipe_revised_offline even though revision 2 exists.

Find the exact published revision first. Return the mismatch error only when no exact match exists.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/challenges/index.ts
Line: 224-234

Comment:
**Published updates cannot enter**

The lookup picks the first published record for a recipe before checking its revision. Publishing revisions 1 and 2 leaves both records published, so linking revision 2 can select revision 1 and return `recipe_revised_offline` even though revision 2 exists.

Find the exact published revision first. Return the mismatch error only when no exact match exists.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. The route now collects every published record for the recipe and picks the EXACT revision when it is published; recipe_revised_offline is returned only when no record at that revision is published. Re-publishing revision 2 (or linking revision 1 while it is still live) no longer refuses. Covered by a regression test: linking the newest published revision enters instead of refusing.

Comment on lines +155 to +158
const openCases = await ctx.store.collection(CASES_COLLECTION).where("targetId", card.cardId).where("state", "queued").get();
if (openCases.length > 0) {
continue;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Removed content returns

Cards and entries reappear after an operator chooses hide, remove, or escalate_sam. The table feed, challenge detail, and page loader hide only queued cases. The decision route marks every case resolved but changes the target only for recipes, so resolving a tableCard or challengeEntry case restores it regardless of the decision.

Preserve the decision's effect on the target or check resolved decisions when building these views.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/tables/index.ts
Line: 155-158

Comment:
**Removed content returns**

Cards and entries reappear after an operator chooses `hide`, `remove`, or `escalate_sam`. The table feed, challenge detail, and page loader hide only queued cases. The decision route marks every case resolved but changes the target only for recipes, so resolving a `tableCard` or `challengeEntry` case restores it regardless of the decision.

Preserve the decision's effect on the target or check resolved decisions when building these views.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. Added shared casesForTarget/casesHideTarget helpers in core/views.ts: a queued case hides content from everyone but its author; a resolved hide/remove/escalate keeps it hidden from everyone; the latest decidedAt wins, so dismissing a fresh report over previously hidden content restores it. The table feed, single-card lookup, challenge detail, and list entry counts all ride the helpers. Covered by regression tests: hide keeps a card hidden, a later dismiss restores it, and remove keeps a challenge entry hidden.

Comment on lines +346 to +350
if (invite.requestedByAccountId !== null && invite.requestedByAccountId !== accountId) {
throw errConflict("conflict", "someone has already asked to join with this code");
}
if (invite.requestedByAccountId === null) {
await invites.doc(invite.inviteId).update({ requestedByAccountId: accountId, requestedByDisplayName: requesterName });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Join requests overwrite each other

Two people presenting the same unused code can both read requestedByAccountId === null, both receive 202, and overwrite each other's request. The last write changes which person the owner will approve.

Read and claim the invite in one transaction so only one person can claim it and later requests cannot replace that person.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/tables/index.ts
Line: 346-350

Comment:
**Join requests overwrite each other**

Two people presenting the same unused code can both read `requestedByAccountId === null`, both receive 202, and overwrite each other's request. The last write changes which person the owner will approve.

Read and claim the invite in one transaction so only one person can claim it and later requests cannot replace that person.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. The join route reads and claims the invite row inside one transaction, so of two people presenting the same unused code exactly one claims it and later requests cannot overwrite the claim. Covered by a jittered-store race test asserting exactly one 202 claim.

Comment on lines +475 to +481
await cards.doc(cardRecord.cardId).create({ ...cardRecord });
const response = cardView(cardRecord, accountId, false);
await ctx.store.collection(OPERATIONS_COLLECTION).doc(operationId).create({
accountId,
createdAt: cardRecord.createdAt,
response,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Retries share cooks twice

Overlapping retries with the same operationId can both pass the replay check before either saves its response. Each then creates a different card. The later operations.create returns false, but that result is ignored and both requests return 201.

Commit the card and saved response together, with a transaction that checks operationId before writing.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/tables/index.ts
Line: 475-481

Comment:
**Retries share cooks twice**

Overlapping retries with the same `operationId` can both pass the replay check before either saves its response. Each then creates a different card. The later `operations.create` returns false, but that result is ignored and both requests return 201.

Commit the card and saved response together, with a transaction that checks `operationId` before writing.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. Card create now commits the operationId replay check, card write, and saved response in one transaction; overlapping retries with the same operationId converge on one card and both receive the same saved response. Covered by a jittered-store race test asserting one card and identical 201 responses.

return String(a["createdAt"]).localeCompare(String(b["createdAt"]));
});
return json(200, {
challenge: challengeView(record, status, viewerEntries.length, { myEntryId: myEntry?.entryId ?? null, myVoteEntryId: myVote }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Entry counts do not match

entryCount includes entries the viewer cannot see. The list counts every non-replaced entry, including reported entries and entries whose publication is hidden. Detail excludes reported entries but still counts entries later skipped by entryStateFor.

The contract defines this as the visible-entry count. Count entries that pass the viewer's visibility checks on both routes so users do not see entries advertised that disappear when opened.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/challenges/index.ts
Line: 188

Comment:
**Entry counts do not match**

`entryCount` includes entries the viewer cannot see. The list counts every non-replaced entry, including reported entries and entries whose publication is hidden. Detail excludes reported entries but still counts entries later skipped by `entryStateFor`.

The contract defines this as the visible-entry count. Count entries that pass the viewer's visibility checks on both routes so users do not see entries advertised that disappear when opened.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. Both list and detail now count only entries the viewer can open (queued-case hiding from non-authors, resolved hide/remove/escalate, in-review entries of others all excluded); the detail count rides the rendered entry set so tile and list can never disagree. Covered by a regression test asserting the same viewer-visible count on list and detail.

handler: async (req) => {
const accountId = requireAccount(req);
const record = await requireChallenge(challengeIdShape.validate(req.params["challengeId"]));
assertEntriesOpen(record);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Saved entries fail on retry

assertEntriesOpen runs before the operationId lookup. If a successful 201 response is lost near the deadline, retrying after entries close returns entries_closed even though the entry was saved.

Return the saved response for the same account before checking whether a new entry is still allowed.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/community/src/modules/challenges/index.ts
Line: 199

Comment:
**Saved entries fail on retry**

`assertEntriesOpen` runs before the `operationId` lookup. If a successful 201 response is lost near the deadline, retrying after entries close returns `entries_closed` even though the entry was saved.

Return the saved response for the same account before checking whether a new entry is still allowed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f1d7ce. The POST route now validates the operationId, answers a saved response for the same account first, and only then checks the entry window — a retry that lost its 201 near the deadline replays the stored response instead of failing with entries_closed. Covered by a regression test replaying a saved entry after the window closes.

ObviousApp and others added 3 commits October 15, 2026 21:57
One test per unresolved Greptile thread: per-person block rows, resolved
moderation decisions keeping their effect, returning members, invite-claim
and operationId races (driven with a macrotask-jittered store wrapper),
published-revision selection, viewer-visible entry counts, and retries of
saved entries after the window closes. All ten fail on 015fc30; the
pre-existing suite is green there (50 pass).
Route every transactional read and write through the SDK Transaction
(t.get/t.create/t.set/t.update, t.get(query)), so a thrown fn commits nothing
and concurrent fns retry on contention instead of losing writes. The SDK
forbids reads after writes, so the transactional view keeps a per-call write
journal (as the in-memory store does): doc reads answer from the journal once
a doc is written in the tx, and update() merges onto journal state, priming it
via t.get only while reads are still allowed.

Emulator probes pin the seam properties: rollback (a fn that writes then
throws commits nothing), read-your-writes, and two parallel counter
increments without a lost update. On the reviewed head the rollback probe
fails (the write survives) and the parallel probe loses an update.

Co-authored-by: Sam Gu <sgu07966@gmail.com>
- blocks: directional per-blocker rows (blockDocId); either side's row hides
  the pair, legacy sorted-pair rows still read, and only the blocker can list
  or unblock (§6.3)
- moderation: shared casesForTarget/casesHideTarget helpers — queued cases
  hide content from non-authors, resolved hide/remove/escalate keep it hidden
  from everyone, and the latest operator decision wins (a dismiss of a fresh
  report restores the content); tables feeds, card lookup, and challenge
  entry views all ride the helpers
- tables: invite claims and card creates are one-transaction
  read-check-write-write with saved-response replay; joining an open seat
  reuses the removed member's row; feed entry counts match the rendered set
- challenges: entryCount counts only what the viewer can open on list and
  detail; detail tallies ride the rendered set; the saved entry response
  answers before the window check so a late retry replays instead of 409;
  entry commits (create + prior replacement + own-vote re-point + saved
  response) run in one transaction; published-revision selection picks the
  exact revision when published so older live revisions stay enterable
- firestore: finish the f1e2e31 cherry-pick resolution (no conflict markers)

Verified: tsc clean; bun test 60 pass / 0 fail (3 Firestore-emulator tests
skip without an emulator); greptile-fixes suite 55+ consecutive clean runs.

Co-authored-by: obvious-autobuild[bot] <app/obvious-autobuild[bot]@users.noreply.github.com>
obvious-autobuild Bot pushed a commit that referenced this pull request Oct 16, 2026
…l HTTP

Boot the production app behind Bun.serve on the Firestore emulator and
issue genuinely concurrent requests for the four overlapping-request
paths behind PR #166: two joiners presenting one code (one 202, one
409), two entries from one account (one active entry), two retries with
one operationId (one card, same saved response), and two approvals at
the member cap (one claims the last slot, one table_full). An in-flight
probe asserts the requests really overlapped in the server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants