Repository navigation
notes(N2a): notes backend: private and shared notes, policies, definers, purge, export (#1671) - #1721
Conversation
…1671) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new notes module (@crove/mod-notes) that enables members to create private or shared notes and link them to records like parties or files. The implementation spans database migrations, RLS policies, API endpoints, background worker jobs for trash cleanup and member purging, and comprehensive tests. The review feedback suggests a performance optimization in the noteUpdate command to combine two separate database queries targeting the same row into a single query, eliminating a redundant roundtrip.
…rower grants (#1671) From the fresh-context review and Codex: - trash_due refuses any context (an agent context on the worker login listed every org) and calls LEAST and GREATEST unqualified (they are SQL syntax, not pg_catalog functions). - Commands check a note against what the caller reads, then lock it: FOR UPDATE also applies the update policy, which hid a shared note from a plain member and named the wrong refusal. - A purge job carries its removal event; a later removal of the same user leaves the notes to its own job. - Owners and admins change only the shared notes their whole-org scope lets them read; search and tainted are not granted for an update. - The reads run for viewers too, so a member made a viewer still reads their own notes (ADR-0037 §2). - Tests: a manager cannot make a shared note private, nobody but the author links it, set_visibility and link.remove refuse an owner, and the removed-added-removed case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
There was a problem hiding this comment.
⏱️ Code Review completed (37 eligible files · 148,929 chars · 1 model call(s) · 5 PR unit(s))
Whole diff with bounded context at base 7b47400 / head ca90d9e.
Excluded by noise policy: pnpm-lock.yaml.
✅ CLEAN_PASS: Complete diff reviewed
🔒 Dependency check
✅ Passed. Every new package version comes from the public registry or a source this lockfile already used for it, its checksum is pinned where the format records one, and OSV lists no new malicious package or high or critical vulnerability for the versions it was asked about.
…or an edit (#1671) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
⏱️ Code Review completed (37 eligible files · 148,823 chars · 2 model call(s) · 5 PR unit(s))
Whole diff with bounded context at base b747def / head 54fa20e.
Excluded by noise policy: pnpm-lock.yaml.
✅ CLEAN_PASS: Complete diff reviewed
Context investigation was bounded to the explicitly requested snapshot paths.
🔒 Dependency check
✅ Passed. Every new package version comes from the public registry or a source this lockfile already used for it, its checksum is pinned where the format records one, and OSV lists no new malicious package or high or critical vulnerability for the versions it was asked about.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
⏱️ Code Review completed (37 eligible files · 148,823 chars · 1 model call(s) · 5 PR unit(s))
Whole diff with bounded context at base bce55f9 / head c6fba33.
Excluded by noise policy: pnpm-lock.yaml.
✅ CLEAN_PASS: Complete diff reviewed
🔒 Dependency check
✅ Passed. Every new package version comes from the public registry or a source this lockfile already used for it, its checksum is pinned where the format records one, and OSV lists no new malicious package or high or critical vulnerability for the versions it was asked about.
From the coordinator's security review: notes.note.create, notes.link.add and notes.link.remove wrote the linked record's type and id into the audit detail whatever the note's visibility. Audit rows are append-only, exported to the owner and outlive the member purge, so for a private note they now say only that the target is private. Tests: a private note's audit rows carry no target; a private note's link read by id answers nothing to any other actor; a second purge run removes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Coordinator security review on c6fba33 (CHANGES REQUESTED): taken in 876fd1a.
|
There was a problem hiding this comment.
⏱️ Code Review completed (37 eligible files · 150,994 chars · 2 model call(s) · 6 PR unit(s))
Whole diff with bounded context at base bce55f9 / head 876fd1a.
Excluded by noise policy: pnpm-lock.yaml.
✅ CLEAN_PASS: Complete diff reviewed
🔒 Dependency check
✅ Passed. Every new package version comes from the public registry or a source this lockfile already used for it, its checksum is pinned where the format records one, and OSV lists no new malicious package or high or critical vulnerability for the versions it was asked about.
Part of #1671 (notes step N2, notes.md §4, ADR-0037). This is the backend half. The UI half (the notes screen, the record panels, the removal warning on the members screen, the e2e test) follows in a second PR, which closes the issue. The coordinator agreed to this split.
What changes
Database (
20261103130000_crove_notes_core.sql):crove_noteswithnoteandlink, columns as the N1 matrix factories expect.apply_tenant_rls, which installs the tenant policy, the grants and the agent write guard. Neither table is on the agents' exempt list.le_scope*, and to the system context. Viewers, entity-limited members and agents see none.author_user_id = ctx_user_id().org_id,id,author_user_id,created_at,searchandtainted.searchtsvector throughcrove_kernel.search_vector, with a GIN index.note(ids only).trash_due: worker login, outside every context.trash_emptyandmember_items_purge: worker login, in the org's system context.private_counts: an owner or admin in a user context, or the worker in the system context.Org purge (
20261103140000_crove_kernel_org_purge_notes.sql): main's newestorg_purge, copied verbatim, withcrove_notes.linkandcrove_notes.noteadded at the head ofv_kernel_first. They are deleted before parties and files (links reference them) and before tasks.Module
modules/notes(@crove/mod-notes):title,body,tagsandsearchare classedpii;permissions: [];notes.note.notes.note.create,update(with an expected revision),set_visibility;pinandunpin,archiveandrestore,delete(to the trash);notes.link.addandremove;notes.trash.emptyandnotes.member_items.purge, both R3 irreversible.notes.note.list(views: mine, shared, pinned, archived, trash; filter by tag or by linked record),get,search;notes.record.panel;notes.private_counts(owners and admins).auditValuemarkers for the text. Command results carry ids and the revision only.notesjoinsAVAILABLE_MODULES. RPC routesnotes/*answer 404 when the module is off.Worker (
apps/kernel/src/notes-worker.ts, plus a one-line hook in the outbox relay and the job registration inworker.ts):kernel.member.removedqueuesnotes.member_items.purge30 days later. The payload carries ids only, and the key is built withjobKey().trash_due, thennotes.trash.emptyper org.Export:
EXPORT_TABLESgains three entries:notes.note, withoutsearch; the system context's policies leave private notes out;notes.link;notes.private_count, read from thecrove_notes.private_counts()definer: rows of org, author and count.Other: the offboarding runbook's deletion map, the messages for 7 error codes and 17 command labels (vi and en),
STREAM_HINT_TYPES, the check-unions anddb-typesschema lists, and the hand-written generated types. CI's db-types check confirms them.Decisions taken here, and why
pg_trgm). Finance's search already usescrove_kernel.search_vectorandsearch_query: words typed without marks match each word as a prefix, so "hoa don" finds "hóa đơn".pg_trgmis not installed anywhere. The column issearch tsvector(classed pii, left out of the export), notsearch_text.NOTE_REVISION_CONFLICT), notNOTES.REVISION_CONFLICT.notes.note.restorebrings a note back from the archive or from the trash.kernel.member.removedevent id. If the same user was removed again later (added back in between), the earlier job leaves the notes to the later one's job. Outbox ids are UUIDv7, soid >means "later".Acceptance
What a user will see change: nothing on screen yet. The API (
notes/*RPC), the worker jobs, the export and the purge are live. The screens come in the UI PR.notes.db.test.ts, "lets a member write a private note on a client and a shared one…"invariant/notes-private.db.test.ts(raw SQL per context: reads, searches, updates, sweeps, deletes, forged inserts, links), plus the N1 RLS matrix from mainnotes.db.test.ts, "keeps markers, not text, in the audit rows"notes.db.test.ts(removal, then purge, re-add and re-removal);notes-private.db.test.ts(the definer);notes-worker.test.ts(queued 30 days later, ids only)notes.db.test.ts, "exports the shared note and a count of the private one"org-purge.db.test.tsseeds a private note, a shared note and a link; all are deleted, and no note text appears in the audit rownotesoff nothing loadsmodule-enablement.db.test.ts: commands absent,notes/*RPC answers 404Review results:
c6fba33b: CHANGES REQUESTED, one blocking Medium finding: audit rows named the record a private note is about. Fixed in876fd1aa, with tests; the two invariant-test gaps it named are taken too.72597f64:trash_dueworked in an agent context on the worker login and listed every org with trashed notes. It now refuses any context.searchandtaintedwere updatable. Fixed: they are no longer granted for an update.set_visibilityandlink.removerun by an owner, and the re-removal case.codex review --base origin/main, effort high) on78744b78raised three findings, all fixed in72597f64:pg_catalog.greatest()andpg_catalog.least()don't exist (LEASTandGREATESTare SQL syntax), which broketrash_due. The CI run of78744b78caught it too.ca90d9e2: both commands now touch the note.zExportRecordrequires every current table. Not new: every earlier addition toEXPORT_TABLEShad the same effect, the CRM tables in feat(crm): the public booking page (#1318) #1352 included. Filed as offboarding: rebinding an export made before new export tables were added #1726 (P3, Icebox).notes.note.updateread the note twice). Taken in54fa20ef; the thread is answered and resolved.Risk left:
Security review (session; protected paths: migrations, invariant suite, package.json, lockfile)
Verdict: no blocking finding left. The review covered tenancy, row policies, definers, audit, export and purge. No money, auth or approval code is touched.
Who reads a private note. Only a context whose
ctx_user_id()is its author.ctx_user_id()comes from the sealed context (T11), so an app cannot claim another user; a forged context reaches nothing (invariant test). There are three actor types:The invariant test proves this per actor (another member, an owner, an admin, a viewer, an entity-limited member, an agent, the system) for reads, reads by id, search, links, updates, no-WHERE sweeps, deletes, inserts written as the author, and link inserts and deletes. Main's N1 RLS matrix ran from base on this PR and passed: cross-org isolation for every actor, and the system context reading exactly the shared rows by id and writing none.
Who writes.
author_user_id,org_id,searchortainted; they are not granted for an update.apply_tenant_rls, and the tables are not on the exempt list (Agents: allowlist agent-context writes and smaller hardening (review of #244) #285 unchanged).Definers. All four use
SECURITY DEFINERwithsearch_path = pg_catalog, pg_temp,REVOKE ALL ... FROM PUBLICand explicit grants. None returns text.trash_due: worker login, no context.trash_emptyandmember_items_purge: worker login, the org's system context.private_counts: an owner or admin in a user context, or the worker's system context.Side channels.
auditValue(the test asserts the markers and searches the rows for every title, body and tag).target: 'private'(coordinator review,876fd1aa). A shared note's facts name the record.NOTE_LINK_EXISTSis reachable by the author only.searchvector is not exported.Purge and export.
org_purgeis main's newest definition verbatim plus two names. The fresh reviewer diffed it independently.Protected paths touched: two migrations, a new invariant test file (
notes-private.db.test.ts, self-contained: it imports only package code),apps/kernel/package.json,modules/notes/package.jsonandpnpm-lock.yaml(the new workspace package only; no external dependency added).🤖 Generated with Claude Code