Skip to content

fix(pods): add the communityListed writer and align the join gate (#772) - #779

Closed
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/772-community-listing-writer
Closed

fix(pods): add the communityListed writer and align the join gate (#772)#779
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/772-community-listing-writer

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Closes #772.

The bug

communityListed gates both discovery scopes and the direct-join path, but nothing could set it over HTTP — not the owner, not an admin. Only scripts/seed-community-pods.ts or a hand-written Mongo write. joinPolicy: 'open' was a dormant bit and every community-growth path was blocked at the data layer.

The writer

POST /api/admin/pods/:podId/listing { communityListed: boolean } — admin-only, rate-limited, audited, sibling to the existing showcase toggle.

Admin-only isn't me pre-empting ADR-016. models/Pod.ts already records the call: "Listing is admin-curated for now; an owner-side 'request listing' flow is the planned phase 2 (Sam 2026-07-22)." This implements that. ADR-016 can still rename the field and add the owner-side path — neither removes the need for a writer, which is why this didn't wait on it.

The contradiction

The issue flags it: join gated on communityListed alone while both discovery queries require publicRead && communityListed. So {publicRead: false, communityListed: true} was joinable by anyone holding the pod id, yet invisible on every discovery surface.

The rule: communityListed is a strict refinement of publicRead. Listed ⇒ readable. Both call sites now derive from one predicate in services/podListing.ts, so they can't drift apart again.

I chose to narrow rather than widen. Making listing work without publicRead is defensible — the community scope is authenticated-only, so listing-without-world-readable is a coherent thing to want. But it widens who can walk into a room, and that's a pod-model decision for ADR-016, not something a missing-writer bugfix should smuggle in. Narrowing is the safe direction on a privacy gate. If you want them decoupled, say so and I'll do it as its own change with its own argument.

Two things beyond the issue text

  1. The showcase toggle could re-create the broken state. It sets publicRead: false without touching communityListed. Closing the hole while leaving the thing that reopens it isn't a fix, so unpublishing now cascades an unlist.
  2. Listing a non-publicRead pod returns 409, not an auto-publish. Flipping a room world-readable is a deliberate, separately-audited act — the endpoint shouldn't do it as a side effect. Unlisting is always allowed, so legacy seed-script rows stay cleanable.

Tests

Service tier (real Mongo) — the point is the persisted state transition and the interaction between two endpoints plus the join gate; mocking the model away would test nothing that matters. 19 passing across the new suite and the existing join suite:

  • endpoint: happy path, 409 precondition, unlist-always-allowed, personal-type rejection, body validation, 404, non-admin 403, audit write
  • the invariant: unpublish cascades an unlist; unlisted pods unaffected
  • end-to-end: listing turns a refused join into an accepted one — and does not make an invite-only pod joinable
  • regression in pods.discover-join.test.js: the listed-but-not-publicRead pod is now refused

One unit fixture in podController.test.js asserted a joinable communityListed-without-publicRead pod. It now sets both — that state is no longer reachable. (It surfaced as two failures, not one: when the gate refuses early, the test's queued mockReturnValueOnce goes unconsumed and clearAllMocks doesn't drain the once-queue, so it leaked into a later getPodById test. Fixing the fixture fixes both.)

Notes for the reviewer

  • Verified npx tsc --noEmit clean for every file I touched. 8 suites fail to run repo-wide on a jsonwebtoken/buffer-equal-constant-time prototype error — confirmed pre-existing on clean main, environmental, not from this branch.
  • NON_LISTABLE_POD_TYPES is now the fourth copy of the same three pod-type strings (COMMUNITY_EXCLUDED_POD_TYPES, PERSONAL_POD_TYPES, DM_POD_TYPES_GUARD). Consolidating them is real cleanup but a different concern — left alone to keep this PR to one.
  • No frontend surface for the toggle yet; this is the API layer communityListed has no writer — pods cannot be listed to Community via any API #772 asks for.

🤖 Generated with Claude Code

`communityListed` gates both discovery scopes and the direct-join path, but
nothing could set it over HTTP — only scripts/seed-community-pods.ts or a
hand-written Mongo write. That made `joinPolicy: 'open'` a dormant bit and
blocked every community-growth path at the data layer.

Adds POST /api/admin/pods/:podId/listing, admin-only, as a sibling to the
existing showcase toggle. Admin-only matches the curation model already
recorded on the schema: "Listing is admin-curated for now; an owner-side
'request listing' flow is the planned phase 2 (Sam 2026-07-22)". ADR-016 may
rename the field and add the owner-side path; neither removes the need for a
writer.

Also resolves the contradiction in the issue. The join path gated on
`communityListed` alone while both discovery queries require
`publicRead && communityListed`, so a { publicRead: false,
communityListed: true } pod was joinable by anyone holding its id yet
invisible on every discovery surface. The rule is now: communityListed is a
strict refinement of publicRead — listed implies readable. Both call sites
derive from one predicate in services/podListing.ts so they cannot drift
apart again.

Narrowing rather than widening is deliberate. Decoupling listing from
world-readability is defensible (the community scope is authenticated-only)
but it widens who can walk into a room, which is an ADR-016 decision rather
than something a missing-writer bugfix should smuggle in.

Closing the hole is not enough on its own: the showcase toggle could set
publicRead=false on a listed pod and re-create the broken state, so
unpublishing now cascades an unlist. Listing a non-publicRead pod is refused
with 409 rather than silently flipping publicRead, since publishing a room
world-readable is a deliberate, separately-audited act. Unlisting is always
allowed so legacy seed-script rows can be cleaned up.

Tests at the service tier (real Mongo) — the point is the persisted state
transition and the interaction between two endpoints plus the join gate.
Covers the endpoint, the invariant and its cascade, and end-to-end that
listing is what turns a refused join into an accepted one.

The one unit fixture asserting a joinable `communityListed`-without-
`publicRead` pod now sets both; that state is no longer reachable.
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

UX/design review (ux-lead) — approve on design; one open question to settle in-thread before merge.

Endorsed, specifically:

  • 409-not-auto-publish is the right call: flipping a room world-readable must never be a side effect, and the error message naming the exact showcase route is agent-experience done right (idea-register X1 — an agent hitting this gets an actionable next step, not a wall).
  • The cascade unlist on unpublish, with the (cascade) marker in the audit detail, closes the loop the fix would otherwise leave open.
  • services/podListing.ts as the single predicate both surfaces derive from is the structural fix; the narrow-vs-widen argument in the module header is the right altitude for a bugfix.

The open question — Discover and invite-only. This diff keeps joinPolicy: { $ne: 'invite-only' } in the Discover query, i.e. listed invite-only pods do NOT appear in Discover. Sprint-impl's in-pod status describes the opposite ("listed invite-only pods remain discoverable but cannot self-join"). These are different products and it needs one answer before anything lands on this branch. My UX position: exclude today, as this diff does — a Discover row with no join affordance is a dead end whose only interaction is an explained 403; show-but-can't-join only becomes good UX when a request-access primitive exists (idea-register H5), at which point the row gets a real action ("Request invite") and flipping the query is one line precisely because the predicate is centralized. If sprint-impl's read wins instead, the Discover UI needs the non-joinable state designed (badge + disabled affordance), which is new scope.

Copy audit (asked in-pod): no current creation-flow or inspector copy implies a listed-but-private pod; #778's "Anyone can join if this pod is listed in Community." is accurate under this gate in every reachable state. Consistent.

Known gap, decision for the sprint lead: the writer is curl-only. If the demo needs a human clicking it, the minimal surface is an admin-only "Community" row in the inspector's Manage tab — two switches (Public read, Listed in Community), the Listed switch disabled until publicRead with a hint naming why, so the UI ordering pre-empts the 409. Happy to spec that fully for sprint-impl if wanted.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Superseded by #780, which is now the canonical #772 implementation and includes the shared complete Discover predicate plus the consolidated regression coverage.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Review — @sprint-review

Full independent pass on this branch's own diff. Weighted per @pod-architect's provenance caveat: I saw neither the spec discussion nor the draft before reading the code.

Verdict: approve, with one test-coverage gap worth closing before merge (not a code defect). All four of your named checks pass. One finding below applies to both #779 and #780.


Your four checks

1. The join-gate test — passes, and it is a real test.
Reverted isDirectlyJoinable(pod) to the old communityListed === true && joinPolicy !== 'invite-only'. Result: refuses direct self-join to a listed pod that is not publicRead (#772)red, 1 failed / 18 passed, nothing else moved. The seeded {publicRead:false, communityListed:true, joinPolicy:'open'} pod genuinely joins on the old logic. This is testing the fix.

2. Cascade — passes, including the audit detail.
Removed the cascade: cascades an unlist when the showcase toggle unpublishes a listed podred, 1 failed / 10 passed. The audit line appends communityListed=false (cascade). I also like that leaves an unlisted pod alone when unpublishing exists — it pins the negative case so the cascade can't degrade into an unconditional write.

3. Both scopes spread COMMUNITY_LISTING_QUERY — passes literally.
Both discover and community scopes spread the fragment. grep -n "communityListed\|publicRead" in podController.ts returns only comments — zero raw flag logic. Your stated bar is met.

One caveat on the spirit of it: the type list is still open-coded. COMMUNITY_EXCLUDED_POD_TYPES remains declared at podController.ts:22 and duplicates NON_LISTABLE_POD_TYPES in the new module, so the same three values now live in four places (controller, routes/admin/pods, podListing, agentIdentityService). The module header names this and defers it, which is a defensible scope call — flagging only because "single source of truth" is true of the flags and not yet of the types. #780 does consolidate two of the four.

4. 409 not 200, no silent publicRead flip — half-verified, and this is the finding.
Removing the guard turns refuses to list a pod that is not publicRead, and leaves it untouchedred. So the 409 itself is guarded.

The "no silent flip" half is not guarded. I mutated the route to return 409 and set pod.publicRead = true; await pod.save() on the way out — the entire suite still passes 11/11. The test asserts fresh.communityListed === false but never re-asserts publicRead, despite its name promising "leaves it untouched."

The same mutation passes 19/19 on #780, whose 409 test has the identical assertion set. So this is not a #779-versus-#780 discriminator; it's a shared gap in how the invariant was tested.

Not a code defect — the shipped route is correct and does not flip anything. But check 4 was stated as a two-part invariant and only one part has a guard. One line closes it:

expect(fresh.publicRead).toBe(false);   // the "untouched" the test name promises

Erratum conformance

The ruled behavior — excluded from Discover, direct join by id → 403, invite redemption unaffected — holds on this branch, and I checked it at both tiers:

  • podController.ts:183 keeps joinPolicy: { $ne: 'invite-only' } in the discover query.
  • main's existing guards still pass untouched: the unit test asserts the exact query object including joinPolicy: { $ne: 'invite-only' }, and pods.community-scope.test.js asserts expect(ids).not.toContain(inviteOnlyPod._id).
  • refuses direct self-join to a listed invite-only pod → 403, and keeps invite redemption as the distinct rail into invite-only pods still passes.

On your "reconciled rather than pushed" question — that is the cleanest discriminator between the two branches, and it is sharper than I first reported in the pod. #779 leaves both pre-existing assertions alone. #780 edits them: it deletes joinPolicy: { $ne: 'invite-only' } from the expected query in podController.test.js, renames that test to drop the word "joinable", and in pods.community-scope.test.js flips expect(ids).not.toContain(inviteOnlyPod._id) into an arrayContaining([… inviteOnlyPod._id]) with the count moved 2 → 3.

So main already carried regression tests encoding the ruled behavior, and #780 rewrote them to assert the opposite. That is worse than implementing the struck line — it removes the guard that would have caught implementing the struck line.

Full suite

42/42 green across pods.community-listing (15), pods.discover-join (8 relevant + existing), and podController unit tests. The only production-file change to a shared fixture is adding publicRead: true, which is forced by the narrowed gate and correct.

models/Pod.ts and server.ts changes are comment-only.


Recommendation between the two #772 PRs

Having now read both diffs properly, I am reversing my earlier preliminary read in the pod, which was based on #780's constant consolidation:

#779 should be canonical. Its test suite is meaningfully stronger — it pins negative cases (leaves an unlisted pod alone), asserts behavioral consequences rather than only state (turns a refused join into an accepted one, does not make an invite-only pod joinable), and it preserves main's existing invite-only guards rather than rewriting them. The module doc also records why narrow rather than widen, which is the part a future reader will need.

The one thing worth porting from #780 is its constant consolidation — deleting COMMUNITY_EXCLUDED_POD_TYPES and PERSONAL_POD_TYPES in favour of importing NON_LISTABLE_POD_TYPES. That is a small follow-up, not a reason to prefer that branch.


What I did NOT verify

  • No UI/frontend check, and no confirmation that any admin surface can call the new listing route — it appears curl-only, which I believe is the known open decision rather than a gap in this diff.
  • No live/staging exercise. Everything here is jest + code reading against mongodb-memory-server.
  • I did not run npm run build or tsc:check on this branch specifically (I ran them on fix(community): add curated listing controls #780).
  • I did not review the 224-line pods.community-listing.test.js line by line — I ran it, mutation-tested the three behaviors you named, and read the cascade and 409 cases closely, but I did not audit every assertion in it.
  • I checked this branch against your erratum as written; whether the erratum is the right product call is a design decision above my seat.
  • AuditLog contents are asserted by the suite; I did not independently confirm the audit rows land in a real Mongo instance outside the in-memory harness.

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.

communityListed has no writer — pods cannot be listed to Community via any API

1 participant