Repository navigation
Feature/m2 relationship lore graph kareem - #21
Conversation
|
Vercel preview is ready: https://hivelore-ngzuhzszv-kareemn1936s-projects.vercel.app Commit: 7b3359b |
|
@KareemN1936 reviewed. The engineering here is strong — 7 tests with real assertions (not smoke tests), clean typecheck/lint on both apps, both migrations apply cleanly on Postgres 16 with no Prisma drift. But there are two blockers, and a structural problem with the stack that needs a decision before any of this merges. First, the structural issueThis PR contains the entire lore CRUD API, which is also PR #19. Both add The genuinely new work here is the two relationship endpoints, status-aware relation projection, Also, a naming note: there is no graph visualization in this PR. The relationship UI is a BlockersB1 — Canon can be mutated with no governance.
|
|
Two corrections to my review above, in fairness to you — I'd read the PR and the code but not your Trello update from 20:36, and it answers both points. 1. The sanitization change was not "silent" — you declared it. I wrote that #21 "silently reverts #19's XSS hardening". Your card update explicitly lists "removed unsafe lore body HTML rendering" among the changes. You announced it; I just didn't see it because it was on Trello rather than in the PR description. Apologies for the characterisation. Reading it as a deliberate strategy rather than an accident, it's also more defensible than I gave it credit for: don't escape on write, and never render as HTML. That's coherent, and it matches what's actually in the code — So I'm withdrawing this as a should-fix. What remains is narrower and worth one line in the PR description rather than a code change: 2. You explained the base choice, and it's reasonable. Your note says you based on The reconciliation question still stands though, because the mechanical problem is real: #19 and #21 both add The two blockers from the review are unaffected: the missing status check on the relationship routes ( For future PRs, worth putting this kind of context in the PR description as well as the card — I review from the PR first, and you clearly did the thinking; it just wasn't where I was looking. |
|
@mcfarhat Thanks for the careful review. I pushed fixes in Decision on #19/#21: I prefer closing #19 and letting #21 carry the lore API, since #21 is now the newer integrated branch and includes the relationship work plus the review fixes. That avoids merging two divergent Addressed:
Sanitization invariant: Naming note: this PR implements relationship linking and navigable graph edges, not a node-link graph visualization. I’m treating the visual graph as follow-up work unless you want it folded into this card. |
|
@KareemN1936 re-reviewed at Thank you for the reconciliation call too. Closing #19 and keeping #18 as a web-only base is exactly right, and I verified it: #18 is now 8 files, all under Verified fixed
New — introduced by the fixesD1 — the B1 fix is stricter than the hole required, and it makes the feature unreachable. Source-DRAFT alone would have closed the governance hole: a draft's outgoing link to canon is already hidden from the public by the projection at D2 — regression from the B2 fix: the relationship list is stale after Link/Remove. D3 — the lore detail page lost SSR and now soft-404s. It's an 11-line passthrough, so public canon lore renders an empty shell plus "Loading lore entry…", and D4 — no refresh-on-401. Still open from last round
NitsThe duplicate-helper problem got worse rather than better: CoverageB1 and S1 both have tests that mutation-testing proves bite. Still uncovered: S3 (the fake DB doesn't enforce the unique constraint), the metadata and per-entry caps, the rate limits, and everything web-side — there are no test files under And your new B1 regression test — the one guarding the blocker — never runs in CI, because there's no test step. Carded: https://trello.com/c/TM6P5w2s Fix D1 (or explain it), D2 and D3, and this is close. |
af851a4 to
ce8c1b9
Compare
|
@mcfarhat Pushed Addressed the latest relationship/lore review follow-ups:
Verification:
|
|
@KareemN1936 verified at D1 — relaxation done correctly, and I checked the dangerous partGuards are right: POST I asked you to relax this, so I owed it a careful check of the consequence — if a draft can link to canon, does that edge show up on the canon entry's public page? It does not. I ran it rather than tracing it, against a canon entry with a draft-sourced incoming relation:
But the bound you added to compensate has a problem. Concretely: 100 drafts × 100 outgoing links all pointing at one canon entry — 10,000 inbound rows, each individually legal. Postgres returns an arbitrary 100-row window, the JS filter discards all of them as drafts, and the canon entry's real relations vanish from its public page. Not a leak — a suppression vector, and still an ungoverned draft affecting canon's public rendering. Even benignly, no D2 — fixed
Minor: D3 — regression. This one has to go backThe SSR half is right: But End result: create lore → It's half-regressed rather than fully: S6 backfill — correct
The test fixture is a time bomb — fix this first
expiresAt: new Date('2026-08-10T12:15:00.000Z'),It is now 12:54 UTC on 2026-08-10. Every It's pre-existing rather than from this commit, but it's exactly the class of thing that makes "tests green" meaningless, so please fix it here. Use a relative expiry ( Still openS2 is now mostly done — rate limits are on every lore route, which is more than I asked for. Still open exactly as flagged: the per-entry cap counts only CoverageD1's relaxed rule is tested ( Fix D3 and the fixture, add the |
|
Addressed the remaining review blockers in 6d4281b:
Quality and Vercel checks are green. |
|
@KareemN1936 verified at 2. Test fixture — fixed and verified by running it. 3. I specifically checked the thing I was worried about — whether the status filter would hide an author's own draft relations from themselves. It doesn't. One caveat worth knowing: this fix has zero test coverage. The fake DB's Item 1 — this is structural, and I've been sending you round in circlesRound one: SSR worked, drafts 404'd. Round two: drafts worked, no real 404. Round three: drafts work again, and a genuinely missing entry now returns HTTP 200 with a warning alert instead of the not-found page. Here's why it keeps swapping, and it isn't a coding slip. The API returns the same The actual fix: get the token to the server. Read the httpOnly refresh cookie in the server component via If that's too big for this PR — which is a fair call — then the minimum improvement is calling Why it matters at all: crawlers will index every mistyped or deleted lore URL as a live page ( Regressions in
|
|
Pushed Changes:
Validation:
|
|
@KareemN1936 reviewed First — you were right, and I was wrongYour objection to the server-side refresh exchange holds, and it's stronger than you put it. I checked: I'm dropping that ask entirely. For the record there is a path — BlockersB1 — A contributor can erase canon relationships from every curator's view. No proposal, no vote.This is the one that matters. Three things you added combine:
Measured against real Postgres — a canon entry with one genuine canon→canon edge plus 100 newer draft-sourced edges from a single contributor: Curators are precisely the role that moderates canon, and they're the ones who lose sight of it. Their view also renders all those attacker-authored draft edges as "Connected lore" with attacker-controlled Fix needs all three: cap inbound edges too, keep an ordering that can't be dominated by recency, and mark non-canon relations in the curator view. B2 — The relationship-create response leaks other users' private drafts
Reproduced end-to-end: B3 —
|
| removed | result |
|---|---|
visibleRelatedEntryWhere → {} |
8 pass |
| relationship cap check | 8 pass |
Serializable isolation |
8 pass |
take limit |
8 pass |
| metadata 16 KB cap | 8 pass |
relation orderBy |
8 pass |
The fake DB's includeLore ignores nested where/take/orderBy, and its $transaction takes one parameter so the options object is discarded. The suite can't observe anything you added. That isn't your fault — it's the test-double design — but it does mean "8/8 green" carries no information about this commit, and it's why B1 and B2 sat in a PR that passed everything.
B1, B2 and B3 are the merge gates. B4 is a two-line move.
|
Pushed Addressed the merge gates:
Also fixed related should-fixes:
Added focused regressions for unreadable draft relationship targets and inbound relationship caps. Validation:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
@KareemN1936 reviewed 446de62 with two independent passes again. Real Postgres, real Prisma with the pg adapter, real Fastify inject, and the web side driven in headless Chrome against a production build. The two passes disagreed on one point and I executed the tiebreak myself — details below.
Genuinely fixed, and verified
B2 — the draft-title leak is properly closed. canReadTarget runs before the create (:903-910), and the probe matrix is now uniform:
GET /lore/vic-secret 404
POST /relationships {vic-secret} 404 (no title)
POST /relationships {vic-submitted} 404
POST /relationships {does-not-exist} 404
POST /relationships {own draft} / {canon} 201 (legit path intact)
curator POST {vic-secret} 201 (correct — may read it)
And there's a test that bites: deleting the guard turns the suite red.
B4 — notFound() now fires. Flag in the catch, notFound() during render. In a real browser: nonexistent entry renders the actual "Page not found" page for both anonymous and authenticated users, the blank-description bug is gone, and an author's own draft still resolves through the client retry.
The inbound cap works, and the race holds. :939-957 counts OR: [{sourceId},{targetId}] inside the serializable transaction. Live test: 99 pre-existing edges plus 10 concurrent creates → {201:1, 409:8, 500:1} and the table settled at exactly 100. The eviction I measured last round is gone — curator sees n=100 with the real canon edge present.
Also fixed: status in the PATCH schema now 400s via .strict() (guarded by a test), relationType is z.enum(RELATIONSHIP_TYPES) so the <img onerror> string 400s, and the existence oracle is closed on PATCH and DELETE — permission check now precedes the status check on both.
lore.test.ts is 10/10 with two new tests.
Blockers
B1 — the P2034 handling is dead code. Concurrent creates still 500.
isPrismaTransactionConflict (:466-468) tests error.code === 'P2034'. But this stack is Prisma 7.8 with @prisma/adapter-pg (lib/prisma.ts:16), and under a driver adapter a commit-time conflict arrives as:
ctor=DriverAdapterError name=DriverAdapterError code=undefined msg=TransactionWriteConflict
code is undefined, so it escapes both the retry loop (:1013) and the 409 mapping (:1016-1020) and reaches Fastify's default handler. Both passes reproduced it independently — 5 trials × 10 concurrent creates on distinct sources gave {201:16, 409:20, 500:14}, i.e. 28% 500s and only 32% of legitimate creates succeeding.
The source-path leak is gone (0/50 responses contained a filesystem path), so that part landed. Fix the predicate to also match error.cause?.kind === 'TransactionWriteConflict' (or PG 40001/40P01), and give the retry jittered backoff — three immediate retries against a relation-level predicate lock won't clear.
B2 — the curator badge you added can never render, and curators still can't see non-canon edges on canon
RelationshipStatusBadge (lore-entry-detail-client.tsx:44-50) is correct and wired at :134/:151. But page.tsx:12-16 always does an anonymous SSR fetch, which succeeds for published canon — and the client effect early-returns when initialEntry is present (:245-251). So on a canon page an authenticated curator never issues the token-bearing fetch.
Verified in-browser with a real curator token:
- API, authenticated:
incomingRelations: [{ source: { status: "DRAFT", title: "Private Draft Entry" }}] - Browser, same token, same URL: "Connected lore / No connections yet."
So B1(c) from the last round is unmet in practice, and the badge is unreachable. The contribute page does this correctly — worth copying that shape: SSR for the anonymous view, then an authenticated refetch when a token exists.
B3 — the cap fix traded eviction for denial-of-service on other people's canon
Because the cap now counts inbound edges and applies to the target — which the caller doesn't own — a plain CONTRIBUTOR can fill a victim's quota: 13 throwaway drafts × 8 relation types = 100 legal edges, roughly 113 requests, about four minutes at the 30/min limit.
After that, a legitimate contributor gets 409 — and so does a CURATOR. Verified. Recovery is 100 manual deletes, and the attacker refills. An ungoverned contributor can permanently freeze a canon entry's relationship budget. A per-author sub-quota (say 10 inbound per author per entry) fixes it without reopening the eviction.
B4 — the existence oracle is still live on POST-relationships
This is where my two passes disagreed, so I checked it myself. One pass reported the oracle fixed everywhere; the other reported it still open. The truth is a split:
PATCH(:1308) — permission (403) then status (409). Fixed.DELETEentry (:1452) — permission then status. Fixed.POST /relationships(:914vs:922) — status (409) still comes before permission (403). Not fixed.
So probing another author's entry as sourceId still yields a three-way: 404 nonexistent, 409 exists-but-wrong-status, 403 exists-and-right-status-but-not-yours. GET returns a uniform 404 for all three. Move the canMutateDraft call above the status check, as you did on the other two.
Should-fix
The ordering depends on enum declaration order, and the order is wrong. status: 'desc' with LoreStatus declared DRAFT, SUBMITTED, APPROVED_FOR_PUBLICATION, PUBLISHED_CANON, ARCHIVED ranks ARCHIVED above PUBLISHED_CANON. Measured against 301 inbound rows: with archived rows present the curator's window becomes {ARCHIVED: 100} and the real canon edge disappears. Not reachable today — no route sets ARCHIVED, and the cap holds totals at 100 — so this guard currently rests entirely on two other guards. Say "canon first" explicitly rather than relying on the enum ordinal, because the first moderation/archive route reopens it.
Relationship edges become permanently immutable once their source publishes. :1118 requires source.status === DRAFT to delete. Verified: create an edge on a draft, publish the draft, and DELETE returns 409 — for the author and the curator. There's no proposal path for edges and no route honours EDIT_CANON_MODERATION_FIX or MARK_SPAM_ABUSE for relationships. So edges enter canon unilaterally and can never leave, which is the inverse of the governance model.
Entry deletion destroys other actors' edges with no audit trail. :1471-1477 deleteMany on both directions, then a single LORE_ENTRY_DELETED row. Verified: a contributor deleting their own draft destroyed 2 edges including a curator's, with 0 LORE_RELATIONSHIP_DELETED rows — and it routes around the draft-only-source rule above.
The backfill fix cannot reach any database that already ran it. You edited 20260810134000 in place to add AND "status" <> 'PUBLISHED_CANON'. I tested that: mark the migration applied with the old SQL, swap in the edited file, run migrate deploy → "No pending migrations to apply." Stored checksum 51e57924… vs file e155ebd4…, and Prisma 7.8 doesn't flag the drift, it silently skips. So any environment that ran the earlier commits keeps its canon rows rewritten permanently. This needs a new corrective migration, plus an audit row for the rows it touched.
Smaller ones: toLocaleDateString() at lore-entry-detail-client.tsx:216 now lives in a client component (it was server-side before), so SSR and client disagree across timezones and locales — a hydration mismatch. Promise.all inside $transaction (:939-952) pipelines on one pg client, which is deprecated in pg 8 and removed in pg 9 (you're on ^8.22.0); sequential awaits cost nothing here. SSR errors other than 404 rethrow into the root error.tsx, and since all SSR renders share one rate-limit bucket, 125 requests turned a page render into a full-page failure. Curator projections differ between list and detail for the same entry. "Suggest edit" still offers Delete entry on canon (lore-entry-form.tsx:561), a dead-end button. pageSize still hardcoded to 100 with no typeahead, and q still doesn't escape %/_.
Mutation testing — real progress, one gap that matters
Three of your new guards now genuinely bite (deleting each turns the suite red): the inbound cap, canReadTarget, and PATCH .strict(). That's a real improvement — last round nothing did.
Still unguarded: the source-only cap variant, the canon-first ordering, the P2034 handling, relationType's enum, the PATCH permission/status order, visibleRelatedEntryWhere, and take. And the one I'd fix first:
Deleting
canMutateDraftfrom POST-relationships entirely leaves the suite 10/10 green.
The only thing stopping a contributor creating relationships off another user's draft can be removed without a single test noticing. Worth one test even if nothing runs it in CI yet.
B1 through B4 are the merge gates. B1 and B4 are small; B2 and B3 need a design decision each.
|
@mcfarhat Pushed Addressed the latest PR #21 merge gates:
Also added a forward-only audit migration for published canon rows that may have been touched by the earlier QUEST/STORY backfill before it was scoped to non-canon rows. Added focused regressions for POST permission ordering, per-author inbound quota, target DoS prevention, driver-adapter conflict retry, and relationship type validation. Validation:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
@KareemN1936 reviewed 78db984 with two passes again. All four blockers are genuinely fixed, and this is the strongest round of the six — verified by execution, not by reading.
- B1 — transaction conflicts. Your predicate covers both shapes, and instrumenting the transaction proved both actually occur: 8 ×
DriverAdapterError/cause.kindand 46 ×code: 'P2034'in one run. So the widened matcher was necessary, not belt-and-braces. 90 concurrent creates →{201:60, 409:30}, zero 500s, and 0/90 responses leaked a path or raw error. The backoff is real, jittered and bounded rather than an immediatecontinue. - B2 — the curator badge renders. Production build, headless Chrome, real curator token:
Private Draft Entry / DRAFT / Allied With this entryon screen, and the anonymous SSR HTML does not contain that title. Both directions correct. - B3 — the DoS is closed. Attacker blocked at 409; a different contributor and a curator both still get 201.
- B4 — oracle closed. No-membership probes return a uniform 403 across nonexistent / DRAFT / SUBMITTED / canon.
And mutation testing went from 3 guards biting to 6 of 8, including canMutateDraft on POST-relationships — the one I singled out last round. Tests 10 → 14. That's real.
Now the part that matters: two of these fixes broke things that matter more than what they fixed.
Blockers
1. The per-author cap breaks the feature it protects
MAX_INBOUND_RELATIONSHIPS_PER_AUTHOR_PER_ENTRY = 10 (:22, enforced :977-992) is per author, per entry. Reproduced: one contributor linking their own character drafts into their own faction hub with member_of gets 201 for #1–#10 and 409 for #11. Identical against a canon hub.
member_of on a faction and located_in on a city are the canonical relationship-graph use cases for M2, and a single-author world — which is every new world — is now hard-capped at ten links into any hub. The limits are also inverted: 100 outbound from one entry, 10 inbound from one author. And the message reads "This author has reached the inbound relationship limit", which lands as a moderation notice on someone linking their own drafts to their own faction.
Your new test (relationship target total does not let one author freeze another author out) passes the stated repro — a fresh source into a saturated target — and never exercises the general case, which is why this got through.
Suggestion: keep a per-author quota but set it well above realistic authoring (50+), or scope it to cross-author inbound only — the attack was one contributor saturating someone else's entry, so a quota that only counts edges where source.authorId !== target.authorId closes the DoS without touching the normal path.
2. Removing the target-total cap reopened the ordering bug
The total-inbound check is gone (:966-971 now counts only the source's own degree). That was the thing making the ARCHIVED-first ordering unreachable. With 121 inbound rows the curator's window is 100 archived edges and the real canon edge is evicted.
To be fair: this is no longer a one-account attack — the per-author quota means it takes ~13 accounts. But the organic path gets there without malice, since a popular canon hub in an active world reaches 121 inbound edges naturally. Fix the ordering explicitly (canon first, rather than relying on enum declaration order) and it stops mattering either way.
3. The retry budget is about 3× too small, and it fails writes that conflict with nothing
RELATIONSHIP_TRANSACTION_RETRIES = 3 gives two retries at 25–75 ms. Measured:
- 8 concurrent valid distinct edges into a target with room to spare → 7 × 201, 1 × 409 "Relationship write conflicted". A legitimate write lost.
- 12 fully disjoint source/target pairs, no shared entity at all → 2 of 12 fail.
- 13–40% hard-failure rate, worst on an empty table, where SSI degenerates to relation-level predicate locks — i.e. worst in a brand-new world.
Decisive: raising only RELATIONSHIP_TRANSACTION_RETRIES from 3 → 10 drops the failure rate to 0.0% at every table size (0/200/1000/5000 rows). The shape is right, the magnitude is wrong. Total added latency at 3 attempts is ~125 ms, so there's plenty of headroom. (Dropping to ReadCommitted also gives 0% but makes the cap racy — don't.)
4. The audit migration writes false canon-mutation records
20260810190000_audit_prior_canon_lore_type_backfill was seeded onto a fresh database the bad backfill never touched:
| row | state | audit row written? |
|---|---|---|
loreType=QUEST, entityType=QUEST — created cleanly |
fine | YES — false positive |
loreType=STORY, entityType=STORY_CONTRIBUTION — created cleanly |
fine | YES — false positive |
loreType=OTHER, entityType=QUEST — the row the scoped backfill deliberately leaves inconsistent |
actually wrong | NO |
So on every fresh deploy and preview environment it writes LORE_ENTRY_UPDATED governance records, actorId NULL, claiming canon "may have been touched" — for entries nothing ever touched, in an app whose stated invariant is that canon changes only via proposals and votes. It audits the wrong set, and it still doesn't repair the rows the original unscoped backfill damaged.
Either drop it, or narrow it to rows that are actually inconsistent and pair it with a repair.
Should-fix
The curator projection fix landed on the detail route only. :707 initialises includeAllNonPublicRelations = false and only assigns it inside the non-canon branch (:726-740), while the detail route computes it unconditionally (:807). Same canon entry, same curator: detail shows 4 inbound, list shows 1 — and a plain contributor sees more in the list view than the curator does, which is the inverse of what round 5 fixed. The hub says "1 connection", clicking through says 4.
The world hub never sends a token. lore-entity-tabs.tsx:70-73 calls listLoreEntries with no accessToken — getStoredAccessToken isn't even imported — so a contributor's own drafts never appear on the hub.
Private drafts and missing entries still return HTTP 200. Verified against a real server: canon 200, someone else's draft 200, nonexistent 200, all serving a "Loading lore entry…" shell with the 404 only firing client-side. Round 3 had this server-side; it moved client-side in round 4 and the status code never came back. Crawlers index loading shells, and with JS disabled the page never resolves.
Source-side oracle is the mirror of the one you fixed. On POST-relationships as a contributor: nonexistent → 404, cross-world → 404, another user's DRAFT/SUBMITTED/ARCHIVED → 403 uniformly. The status masking works, but 404-vs-403 still discloses "this id exists in this world and isn't yours". You hardened the target side to 404-both-ways; the source side needs the same.
A target's owner can't prune inbound edges. DELETE requires sourceId === params.entryId (:1126-1130), so the canon entry's own author gets 404 — only the source's author or a curator can delete. Combined with the cap above, an author who hits the limit has no remediation from their own entry, and lore-entry-form.tsx:510 renders only outgoingRelations, so there's no inbound UI at all.
The sub-quota is keyed on the wrong identity. :977-984 counts by source.authorId, not the acting user. Verified: a curator creating an edge from a victim's draft succeeds, the audit records actorId = curator, but the quota consumed is the victim's. So one curator can exhaust every contributor's quota toward a hub, and the quota and the audit trail disagree about who acted.
Unmapped Prisma errors still leak paths. sendLoreError (:453-461) rethrows anything that isn't a LoreRouteError, and there's no setErrorHandler. P2034 and P2002 are mapped — which is why the leak I measured last round is gone — but e.g. a P2003 still returns the absolute path and source lines. Worth a generic 500 mapper rather than per-code whack-a-mole.
Not yours, but you should know
The API rate limiter is a single global bucket per IP. auth-rate-limit-store.ts:47 builds its key from routeInfo.method/.path, which @fastify/rate-limit v11 doesn't populate in that shape, so every route collapses to ANY:unknown:<ip>. Reproduced: 25 anonymous lore GETs then one POST /auth/challenge → 429. That means every config.rateLimit you added to lore.ts is inert, and because SSR fetches server-side with TRUST_PROXY=false, all SSR traffic shares one bucket.
This is on develop already — it came in with Perla's merged #15, so it isn't yours to fix and I'm raising it with her separately. Flagging it because your SSR + client-refetch pattern is the heaviest consumer, and because page.tsx:19-23 only catches 404 — a 429 rethrows and hard-errors the public lore page. Worth catching non-404s there regardless of who fixes the store.
The 13 red tests in worlds.test.ts and contributions.test.ts are the refreshSession stub gap, unchanged and not from this commit.
Blockers 1 and 3 are the ones I'd do first — 1 because it breaks normal authoring, 3 because it's a one-constant change with a measured result.
|
@mcfarhat Pushed Addressed the latest PR #21 blockers:
Also fixed related follow-ups:
Validation:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Reviewed at 88ecd259, two independent passes, both executing against real Postgres 16 + real Prisma 7 + real Fastify routes rather than reading the diff.
All three of your claims hold, and the three earlier findings are genuinely fixed. Taking them in turn:
The Prisma 7 conflict matcher — proven, not read. I caused real Postgres SSI serialization failures against the installed @prisma/adapter-pg and drove the actual route at forced-overlap concurrency:
HTTP status tally : {"201":8}
$transaction attempts : 19 (8 requests + 11 retries)
real conflicts absorbed: 11 (both error shapes observed)
relationships persisted: 8 audit rows persisted: 8
11 real conflicts classified and retried, zero 500s, no lost or duplicated writes. Both live shapes exist and both are matched — PrismaClientKnownRequestError/P2034, and the bare DriverAdapterError with code === undefined and cause.kind === 'TransactionWriteConflict' that I flagged last time.
The repair migration is precisely scoped. Seeded 15 rows — 2 genuine repair cases, 5 look-alike decoys, 8 edge/NULL cases. It changed exactly the 2, wrote exactly 2 WorldAuditLog rows, and left every decoy alone including lowercase "quest", " QUEST", jsonb null, jsonb scalar and array forms. Idempotent on re-run. The NOT EXISTS audit guard is real: I reverted a row and re-ran, and it re-repaired without writing a second audit row. On a 200k-row table it plans as a bitmap index scan, takes only RowExclusiveLock, and finishes in under 35 ms.
Authorization and canon visibility are clean. Full role matrix — anonymous, non-member, reader, contributor, other-author, founder, curator — against entries in all five statuses, including the case I most expected to leak: a PUBLISHED_CANON hub related in both directions to private entries from two different authors. No title, id, type, status or count reached an unauthorized viewer. The 404 body for a nonexistent id and an existing-but-invisible id are byte-identical. Cross-world edges refused, self-edges 400, canon-as-source 409. I also mutation-tested every guard: each one is load-bearing.
That's a genuinely good result, and better than either of your previous rounds. Four things before it merges.
1. BLOCKER — this PR currently has nowhere to land
Its base is feature/m2-lore-entities-crud-frontend-kareem, which is the head branch of PR #18 — closed on 08-10. So #21 targets a branch with no open PR and no path to develop. Your plan on #20 ("rebase after #21 lands") can't happen while that's true.
Please re-point this PR's base to develop. Related: I've closed #16 as superseded by #20 and retargeted #20 to develop too, so that whole chain is now flat.
2. BLOCKER — merging this alongside #20 silently deletes a file you depend on, with no conflict
This is the one I'd most want you to see, because git reports success.
apps/web/src/lib/api/session.ts exists in the branch point you share with #20. #21 never touches it; #16/#20's commit fe6e038 deletes it. Git sees delete-vs-unmodified and applies the delete cleanly — no conflict, nothing in the merge output. Reproduced in both directions and in all 8 merge orders tried:
develop + pr21 → merge pr16 : conflicted files = [worldId]/page.tsx only
session.ts listed as conflicted? 0
session.ts after merge: DELETED (silent)
files still importing @/lib/api/session: 4
I then assembled the intended end state and compiled it, and it fails exactly there:
src/components/lore/lore-contribute-client.tsx(9,38): error TS2307: Cannot find module '@/lib/api/session'
src/components/lore/lore-entity-tabs.tsx(13,38): error TS2307: ...
src/components/lore/lore-entry-detail-client.tsx(12,38): error TS2307: ...
src/components/lore/lore-entry-form.tsx(23,38): error TS2307: ...
Please migrate those four components off getStoredAccessToken to the auth-session-provider that #20's lineage introduced. Doing it now, in this PR, is much safer than discovering it at merge time — a guaranteed build break that produces no conflict marker is exactly the kind of thing that reaches develop unnoticed.
3. BLOCKER — the lore list endpoint returns megabytes to anonymous callers
With 100 canon entries and dense canon-to-canon edges — reachable through the normal flow, since edges are created while the source is a draft and persist when it's published — GET /worlds/w1/lore?pageSize=100 returned:
200 OK, 180 ms, 3,326 KB, 20,000 relationship objects
100 entries × 2 directions × take: 100. The route's rate limit is 120/min, so that's roughly 400 MB/min per IP, unauthenticated. And incomingRelations/outgoingRelations are read only by the detail page and the entry form — the list view never uses them. Dropping relations from the list projection fixes this outright and costs you nothing.
4. Please validate content.entityType
validateLoreContent checks summary and body and nothing else, and entityType appears nowhere else in the API — it's written straight from the browser form. Your repair migration keys its canon loreType rewrites on content->>'entityType'. So a contributor can set entityType: "QUEST" on an entry that is deliberately typed OTHER, and the migration will retype their published canon row — bypassing the API's own rule that canon is immutable (PATCH rejects non-DRAFT) and desyncing the row from its HiveReference, which schema.prisma:441 explicitly warns against.
Severity is limited by the migration being one-shot — the value has to be planted before it runs — so I'm not calling it critical. But entityType is load-bearing for canonical type and should be validated against the known vocabulary on write.
Not blocking — worth a follow-up card
PATCH/DELETEleak existence whereGETcorrectly doesn't. Probing another author's hidden draft as a contributor:GET→ 404, butPATCH→ 403 andDELETE→ 403; a nonexistent id gives 404 on all three. You hardenedGETfor exactly this reason. Ids are cuids so it isn't brute-forceable — it's an inconsistency, not a break.- The
P2034branch of the conflict matcher is untested. Deleting it leaves the suite fully green while producing real 500s (5 of 8 requests in my run). The retry test fabricates only theDriverAdapterErrorshape. lore.test.tsruns against a hand-rolled in-memory Prisma fake whoseincludeLore()ignoresinclude.where,takeandorderByentirely. That's whyvisibleRelatedEntryWherecan be deleted with no test failing — the DB-level guard can never be exercised. I verified it's correct against real Prisma myself, but the coverage gap is worth knowing about; it's the reason both of us keep needing real databases to settle questions about this file.- The earlier backfill migration retypes ARCHIVED rows and writes no audit row at all, while the repair migration audits — asymmetric for two migrations doing the same class of rewrite.
- No relationship-type validation against endpoint entity types:
CHARACTER located_in CHARACTERandCHARACTER rules CHARACTERboth return 201. For a feature called "typed relationships" that's worth a constraint. A allied_with BandA enemy_of Bcan coexist, sincerelationTypeis in the unique key. No reciprocal-consistency check.
On your validation claims
Your API suite is 16/16 on the new lore tests and typecheck/lint/prettier/build are all clean. But the suite as a whole is 109 pass / 13 fail on this head. I checked before attributing them: the failing subtest name sets are identical on your base branch, and the cause is base commit 0a9f340 adding isRefreshSessionActive without updating the test fakes. Not yours — but the PR body lists validation as passing without mentioning them, and they'll follow this branch wherever it goes. Worth a line in the description so the next reader doesn't have to re-derive it.
Once the base is re-pointed and the four session.ts importers are migrated, this is in the best shape of the three open HiveLore PRs. Measured merge order across every combination is #22 → #21 → #20, and #22 + #21 together comes out fully green — 172/172 tests, clean build, clean migrate deploy, zero drift — so this PR is likely to be the second thing to land.
|
Pushed Covered:
Verified locally:
|
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Re-reviewed at 6262751d. All four blockers are closed, verified by execution — and a couple of them are closed better than I asked for.
1. Base re-pointed. merge-base(pr21, develop) is develop's head exactly, so this is now a fast-forward. Closing #18 didn't orphan anything — its head is an ancestor of this branch.
2. The silent build break is gone, and it's now a visible conflict. This is the outcome I wanted. Control run on the old head merged with #20: no conflict on the deleted file, then four TS2307 errors at compile time. On the new head: the merge surfaces 3 real conflicts instead, and once resolved, typecheck and build both exit 0 with zero TS2307. A conflict you can see beats a break you can't.
3. The list payload — 30× smaller. Same seed both sides (100 canon entries, 9,900 relationships), anonymous ?pageSize=100:
old new
list bytes 4,798 KB -> 158.7 KB
relationship objs 19,800 -> 0
detail bytes 48.0 KB -> 48.0 KB (byte-identical)
detail relations 99/99 -> 99/99
Splitting loreEntryListInclude() from loreEntryInclude() is exactly right, and the detail page kept everything.
4. content.entityType validated — stricter than I asked. I asked for validation against the known vocabulary; you validate that it matches the row's loreType. On both create and update:
400 "DRAGON" / "Character" / "character" must match the lore type
400 ["CHARACTER"] / null / 42 / {} must be a string
400 "QUEST" on a CHARACTER row must match the lore type
201 "CHARACTER" (correct) · 201 absent (optional)
And it holds when loreType itself changes in the same PATCH. That closes the repair-migration hazard properly rather than papering over it.
Everything I'd previously verified still holds after the fixes — I re-checked rather than assuming:
- Concurrency: 30 simultaneous relationship creates → 30× 201, zero 5xx, all 30 persisted, while Postgres logged 85 genuine serialization failures underneath. The matcher and retry loop are doing real work.
- Canon visibility: full matrix across all five statuses. The hard case — a
PUBLISHED_CANONhub related in both directions to private entries from two different authors — still leaks nothing to anonymous, non-member or reader:out=[] in=[], no title, no id, no existence. Author A sees only A's neighbour, B only B's, curator both. - Repair migration: 11 seeded rows spanning the space, exactly the 2 genuinely-inconsistent ones repaired, 2 audit rows, idempotent on re-run.
One correction I owe you
I told you the 13 failing tests weren't yours. That was wrong, and I'm sorry — you may have skipped them on my say-so.
I said they were inherited from your old base branch, caused by isRefreshSessionActive being added without updating the test fakes. I checked properly this time:
isRefreshSessionActive added by: db50deb "Fix/auth secret hardening (#15)"
^ that IS develop's head commit
worlds.test.ts exists on develop? NO in this PR? YES
contributions.test.ts exists on develop? NO in this PR? YES
lore.test.ts exists on develop? NO in this PR? YES
So the middleware was never exclusive to the old base — it's on develop — and all three test files are added by this PR. They're your tests failing against develop's auth middleware, which makes them yours to fix now that you're targeting develop.
The good news is you've already solved it once: lore.test.ts:355-364 fakes refreshSession correctly and passes 16/16 standalone. worlds.test.ts and contributions.test.ts have no such fake, so auth returns null and assertions get 401 where they expect 403/200 (e.g. worlds.test.ts:592). Copying that block across is the whole fix.
Please do that before this merges. It matters more than it looks: CI doesn't currently run these tests — the test step only arrives with #22 — so this is green today and goes red the moment #22 lands. Same trap as the other two we found today, where a check that can't fail reads as a check that passed.
One thing that needs a decision, not a fix
#21 and #20 no longer merge cleanly — three conflicts, and two are genuine:
providers/auth-session-provider.tsx— add/add, both PRs create it. Both export an identicalAuthSessionContextValue, and #20's is a strict superset (it adds the lock and request timeouts from its own review), so take #20's.worlds/[worldId]/contribute/page.tsx— this one is a product question, not a merge:LoreContributeClientandContributionEditorFormare two different contribute experiences. Someone has to decide which one users get.
I'd rather that were decided deliberately than settled by whoever merges second. Worth a word with Mohammad.
Scope note
Re-targeting flattened the whole unmerged stack into this PR — 22 commits, 41 files, +10,630/−300, now carrying world-seed backend, hub hardening, contribution drafts, worlds frontend and #18's lore CRUD, all under a "relationship lore graph" title. The tree is coherent (build and tests confirm) and I'm not asking you to unpick it this late. Just worth a line in the description so the next reader isn't surprised.
Genuinely strong round — the relationship graph work is the most solid of the three HiveLore PRs, and the visibility model held up against everything I threw at it. Fix the two test files and I think this is ready.
|
Pushed \�8b2383\ to fix the route test fakes now that #21 targets \develop\ and the auth middleware verifies active refresh sessions.\n\nChanges:\n- Added active |
|
Remote checks are green on \�8b2383\ too: Quality passed and Vercel Preview passed. |
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Verified at a8b23832. 122 tests, 122 pass, 0 fail. The 13 failures are gone, fixed with exactly the refreshSession fake from your own lore.test.ts copied into both files — clean, minimal, no test weakened to get there.
(One note for whoever runs these next: on this branch the route suites need AUTH_JWT_SECRET / AUTH_REFRESH_SECRET in the environment or three suites crash on load and you silently get 92 tests instead of 122. I hit that myself mid-verify. #22's run-tests.mjs injects them, so it stops being a footgun once that lands.)
Approving. All four blockers from the original review are closed, and I re-verified after the fixes that nothing regressed: the transaction-conflict matcher still absorbs real serialization failures with zero 5xx, the canon visibility matrix still leaks nothing on the hard case, and the repair migration still touches exactly the rows it should.
Two things that are not blockers on this PR, for the record:
-
The merge with #20 needs a real integration — three conflicts, of which
providers/auth-session-provider.tsxshould take #20's version (strict superset, adds the lock and request timeouts), andcontribute/page.tsxis a product decision rather than a merge (see below). -
The contribute page — decision made, and it's the spec's. Mohammad's call is to follow
docs/planning/product-world-ux.md. §8.4 describes one editor screen with an entry type selector, structured fields per type, a relationship picker, and a Submit Proposal button; Journey B has a writer creating a character and ending with "Submits proposal for vote". So a lore entity goes through the proposal and voting flow — it isn't created directly.Concretely that means: keep your per-type structured fields and the relationship picker from this PR, and have them submit into #20's contribution/proposal flow rather than
POST /worlds/{id}/loredirectly. YourLoreContributeClientis the half that matches §8.4's structure; #20's is the half that matches its governance. #20's dead "unsupported category" branch is the seam where the two were meant to meet.That's follow-up work, not a change to this PR — I'd rather land this and do the integration deliberately than bolt it on now.
Nice turnaround on the tests, and sorry again for sending you the wrong way on them the first time.
Summary
Implements M2 relationship linking for lore entries as the newer integrated lore stack.
This PR intentionally carries the lore CRUD API/UI plus the relationship linking work. PR #19 has been closed in favor of this PR to avoid merging two divergent
routes/lore.ts,lore.test.ts, Prisma schema, and migration implementations.Included
Security / Rendering Invariant
content.bodymay contain raw user/editor HTML.The lore UI must not render lore body content as markup. Current rendering uses React text output only. If rich rendered lore bodies are added later, sanitize on render with DOMPurify or equivalent before any HTML injection.
Graph Note
This PR implements relationship linking and navigable graph edges. It does not include a node-link graph visualization; treating that visual graph as follow-up work unless the reviewer wants it folded into this card.
Validation