Skip to content

notes(N2a): notes backend: private and shared notes, policies, definers, purge, export (#1671) - #1721

Merged
JOY (JOY) merged 13 commits into
mainfrom
claude/notes-n2-1671
Oct 28, 2026
Merged

JOY (JOY) merged 13 commits into
mainfrom
claude/notes-n2-1671

Conversation

@JOY

@JOY JOY (JOY) commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

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):

  • Schema crove_notes with note and link, columns as the N1 matrix factories expect.
  • Each table goes through apply_tenant_rls, which installs the tenant policy, the grants and the agent write guard. Neither table is on the agents' exempt list.
  • Restrictive policies per ADR-0037 §2:
    • A private note is visible to its author only.
    • A shared note is also visible to owners, admins and members with le_scope *, and to the system context. Viewers, entity-limited members and agents see none.
    • Insert requires author_user_id = ctx_user_id().
    • Update is allowed for the author, and for owners and admins with whole-org scope on a shared note. The check keeps a shared note shared.
    • A link reads through its note. Only the note's author inserts or deletes one.
  • Grants:
    • DELETE on notes is revoked; only the definers and the purge delete.
    • The UPDATE grant leaves out org_id, id, author_user_id, created_at, search and tainted.
    • Links can't be updated.
  • Search: a BEFORE trigger folds the title, tags and body into a search tsvector through crove_kernel.search_vector, with a GIN index.
  • Stream hint note (ids only).
  • Definers that never return text:
    • trash_due: worker login, outside every context.
    • trash_empty and member_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.
  • Two pg-boss queues.

Org purge (20261103140000_crove_kernel_org_purge_notes.sql): main's newest org_purge, copied verbatim, with crove_notes.link and crove_notes.note added at the head of v_kernel_first. They are deleted before parties and files (links reference them) and before tasks.

Module modules/notes (@crove/mod-notes):

  • The manifest:
    • title, body, tags and search are classed pii;
    • permissions: [];
    • entity notes.note.
  • Commands:
    • notes.note.create, update (with an expected revision), set_visibility;
    • pin and unpin, archive and restore, delete (to the trash);
    • notes.link.add and remove;
    • system commands notes.trash.empty and notes.member_items.purge, both R3 irreversible.
  • Queries:
    • 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).
  • Audit facts carry ids, the visibility and changed field names, with auditValue markers for the text. Command results carry ids and the revision only.
  • notes joins AVAILABLE_MODULES. RPC routes notes/* 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 in worker.ts):

  • kernel.member.removed queues notes.member_items.purge 30 days later. The payload carries ids only, and the key is built with jobKey().
  • A daily cron empties the trash: trash_due, then notes.trash.empty per org.

Export: EXPORT_TABLES gains three entries:

  • notes.note, without search; the system context's policies leave private notes out;
  • notes.link;
  • notes.private_count, read from the crove_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 and db-types schema lists, and the hand-written generated types. CI's db-types check confirms them.

Decisions taken here, and why

  • Search uses the existing tsvector fold, not trigram (plan §3.7 named pg_trgm). Finance's search already uses crove_kernel.search_vector and search_query: words typed without marks match each word as a prefix, so "hoa don" finds "hóa đơn". pg_trgm is not installed anywhere. The column is search tsvector (classed pii, left out of the export), not search_text.
  • Error codes use underscores like the rest of the kernel (NOTE_REVISION_CONFLICT), not NOTES.REVISION_CONFLICT.
  • Revision rises on text and visibility changes only. Pin, archive and delete by an owner never conflict with the author's edit in progress.
  • notes.note.restore brings a note back from the archive or from the trash.
  • Viewers run the read queries. The row policies show them only their own notes, so a member made a viewer still reads what they wrote (ADR-0037 §2, "whatever their role"). Viewers write nothing.
  • Purge per removal event. A job carries its kernel.member.removed event 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, so id > means "later".
  • Export count through a definer used as the part's source, so the export stays one mechanism.
  • Activity: linking a shared note to a party writes "Thêm ghi chú" on the party's timeline, with no title. If the author later makes the note private, that row stays: it carries no text, and the note was shared when it was written.

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.

Acceptance item (#1671) Status in this PR Evidence
A member writes a private note on a client and a shared one Done (API) notes.db.test.ts, "lets a member write a private note on a client and a shared one…"
Another member, a viewer, an entity-limited member, an agent context and the system context read none of the private one (database tests) Done invariant/notes-private.db.test.ts (raw SQL per context: reads, searches, updates, sweeps, deletes, forged inserts, links), plus the N1 RLS matrix from main
The audit row holds markers, not text Done notes.db.test.ts, "keeps markers, not text, in the audit rows"
Removing the member hides their private notes and deletes them after 30 days Done 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)
The org export has the shared note and a count for the private one Done notes.db.test.ts, "exports the shared note and a count of the private one"
The org purge deletes both Done org-purge.db.test.ts seeds a private note, a shared note and a link; all are deleted, and no note text appears in the audit row
With notes off nothing loads Done (API) module-enablement.db.test.ts: commands absent, notes/* RPC answers 404
E2e: write, pin, find on the client's overview In the UI PR

Review results:

  • Coordinator's independent security review on c6fba33b: CHANGES REQUESTED, one blocking Medium finding: audit rows named the record a private note is about. Fixed in 876fd1aa, with tests; the two invariant-test gaps it named are taken too.
  • Fresh-context reviewer, told to attack private-row leaks for every actor: it found no leak of a private note to any actor.
    • Two blocking findings, both fixed in 72597f64:
      • trash_due worked in an agent context on the worker login and listed every org with trashed notes. It now refuses any context.
      • Locked loads hid a shared note from a plain member and named the wrong refusal. Commands now check the note first, then lock it.
    • Non-blocking findings:
      • A remove, add back and remove again sequence purged too early. Fixed: each job carries its removal event.
      • A tiny race between the purge and a re-add. Not fixed here: notes: close the re-add race in crove_notes.member_items_purge #1722.
      • The managers' update policy lacked the whole-org scope clause. Fixed.
      • search and tainted were updatable. Fixed: they are no longer granted for an update.
      • The activity row stays after a note is made private. Kept and documented above.
      • Test gaps. Filled: a manager making a shared note private, links on the shared note by others, set_visibility and link.remove run by an owner, and the re-removal case.
  • Codex CLI (codex review --base origin/main, effort high) on 78744b78 raised three findings, all fixed in 72597f64:
    • P1, the re-removal purge (as above).
    • P2, pg_catalog.greatest() and pg_catalog.least() don't exist (LEAST and GREATEST are SQL syntax), which broke trash_due. The CI run of 78744b78 caught it too.
    • P2, a member made a viewer lost access to their own notes.
  • Codex on the final head: two P2 findings.
  • CodeRabbit: out of its free review quota ("Review limit reached"), so it has not reviewed this PR.
  • Gemini Code Assist: one medium finding (notes.note.update read the note twice). Taken in 54fa20ef; 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:

    • User: any non-author user reads nothing of the note, whatever their role or scope.
    • System (no user, no agent): reads shared notes only.
    • Agent (no user, an agent set): reads nothing.

    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.

    • The author changes their own notes.
    • Owners and admins with whole-org scope change shared notes, and cannot make one private: the check keeps it shared.
    • Nobody changes author_user_id, org_id, search or tainted; they are not granted for an update.
    • DELETE is revoked.
    • The agent write guard is installed by 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 DEFINER with search_path = pg_catalog, pg_temp, REVOKE ALL ... FROM PUBLIC and explicit grants. None returns text.

    • trash_due: worker login, no context.
    • trash_empty and member_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.

    • Audit facts use auditValue (the test asserts the markers and searches the rows for every title, body and tag).
    • A private note's audit facts never name the record it is linked to: they say target: 'private' (coordinator review, 876fd1aa). A shared note's facts name the record.
    • Command results carry ids and the revision only.
    • Nothing goes to the outbox.
    • Error messages are fixed text.
    • NOTE_LINK_EXISTS is reachable by the author only.
    • The search predicate runs under the row policies, and the search vector is not exported.
    • Stream hints carry an id, as ADR-0037 §3 accepts.
    • An activity row on a party is written only for a shared note, with fixed text.
  • Purge and export.

    • org_purge is main's newest definition verbatim plus two names. The fresh reviewer diffed it independently.
    • The member purge refuses while the user is a member, and defers to a later removal's job.
    • Rebinding an export older than this PR is offboarding: rebinding an export made before new export tables were added #1726 (pre-existing behavior of every table addition).
    • The export reads under the system context's policies and records private counts without text.
  • 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.json and pnpm-lock.yaml (the new workspace package only; no external dependency added).

🤖 Generated with Claude Code

@vercel

vercel Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
crove Skipped Skipped Oct 28, 2026 11:10am UTC

Request Review

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9668ff89-1c22-4d08-a38f-b10821ef2d3b
Commits

Reviewing files that changed from the base of the PR and between bce55f9 and 876fd1a.

Files ignored due to path filters: 2
  • packages/db/src/generated/database.ts is excluded by !**/generated/**
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
Files selected for processing: 36
  • apps/app/src/i18n/messages/en.ts
  • apps/app/src/i18n/messages/vi.ts
  • apps/app/src/lib/kernel-errors.ts
  • apps/app/src/lib/stream.ts
  • apps/kernel/package.json
  • apps/kernel/src/config.ts
  • apps/kernel/src/jobs-schema.ts
  • apps/kernel/src/kernel.ts
  • apps/kernel/src/notes-worker.ts
  • apps/kernel/src/rpc.ts
  • apps/kernel/src/worker.ts
  • apps/kernel/test/http.test.ts
  • apps/kernel/test/invariant/notes-private.db.test.ts
  • apps/kernel/test/manifest-permissions.ts
  • apps/kernel/test/manifest.db.test.ts
  • apps/kernel/test/module-enablement.db.test.ts
  • apps/kernel/test/notes-worker.test.ts
  • apps/kernel/test/notes.db.test.ts
  • apps/kernel/test/org-purge.db.test.ts
  • docs/runbooks/org-offboarding.md
  • modules/notes/package.json
  • modules/notes/src/index.ts
  • modules/notes/src/manifest.ts
  • modules/notes/src/notes.ts
  • modules/notes/src/system.ts
  • modules/notes/tsconfig.json
  • packages/db/src/schema.ts
  • packages/db/test/check-unions.db.test.ts
  • packages/kernel-contract/src/contract.ts
  • packages/kernel-contract/src/index.ts
  • packages/kernel-contract/src/notes.ts
  • packages/kernel-contract/src/screens.ts
  • packages/kernel/src/offboarding.ts
  • supabase/migrations/20261103130000_crove_notes_core.sql
  • supabase/migrations/20261103140000_crove_kernel_org_purge_notes.sql
  • tools/db-types.mjs
  • 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread modules/notes/src/notes.ts Outdated
…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>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

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.

dos[bot]
dos Bot approved these changes Oct 28, 2026

@dos dos Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⏱️ 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.

dos[bot]
dos Bot approved these changes Oct 28, 2026

@dos dos Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⏱️ 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.

dos[bot]
dos Bot approved these changes Oct 28, 2026

@dos dos Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⏱️ 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>
@JOY

Copy link
Copy Markdown
Contributor Author

Coordinator security review on c6fba33 (CHANGES REQUESTED): taken in 876fd1a.

  • Blocking (Medium): notes.note.create, notes.link.add and notes.link.remove now write the linked record's type and id into the audit detail only for a shared note; for a private note the fact is target: 'private'. notes.db.test.ts checks that a private note's audit rows (create, update, link remove, link add) never contain the party id, while the shared note's create does.
  • Invariant test gaps: (a) every other actor reading the private note's link by id gets no row; (b) a second purge run after the committed one removes nothing.

dos[bot]
dos Bot approved these changes Oct 28, 2026

@dos dos Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⏱️ 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.

@JOY
JOY (JOY) merged commit 808a982 into main Oct 28, 2026
26 checks passed
@JOY
JOY (JOY) deleted the claude/notes-n2-1671 branch October 28, 2026 12:07

This branch was previously deployed

1 inactive deployment
Preview — 876fd1aa Deployed Oct 28, 2026 by vercel[bot]
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.

1 participant