[rnambaale]

Description

Proposes the phased implementation plan for the platform's search capability, covering both human (coach/coachee UI, org admin, super admin) and AI-agent (MCP) consumers. This PR is documentation only - it establishes the API contract, authorization model, and PR-by-PR delivery path for team review before any code lands.

GitHub Issue: [Resolves] #404

Changes

• Adds docs/implementation-plans/search-capability-backend.md:
  • GET /search contract emulating the shape (not the product) of Tiptap's retired search API: single scored, type-tagged hit list with snippets and navigation context, keyset cursor pagination, mode=keyword|semantic|hybrid reserved from day one
  • Authorization compiled into the queries via a visible_relationships scope mirroring grants_access_to exactly (participant + current org membership), plus org-admin and super-admin tiers; disallowed types silently dropped
  • Phase 1 = Postgres FTS (websearch_to_tsquery + GIN expression indexes, no extensions); later phases: transcripts, members/orgs, MCP search tool, notes projection via the planned self-hosted docs-collab-server, then pgvector semantic and hybrid RRF modes
  • Migration DDL sketches (including the CREATE INDEX CONCURRENTLY transaction caveat), testing strategy, and sizing per PR

Testing Strategy

Docs-only change — review the rendered markdown. The plan itself defines the testing strategy for each implementation PR (scope-isolation integration tests, envelope-pinning controller tests, a shared "searchable corpus" scenario builder).

Concerns

• Phase 1 searches the local notes.body mirror, which can lag the live Tiptap doc; accurate note search arrives with the docs-collab-server projection (PR 5).
• The PR-2 transcript index may need a manual CREATE INDEX CONCURRENTLY as doadmin if transcript_segments is large in production by then; the migration is written IF NOT EXISTS to no-op afterward.

[greptile-apps]

Confidence Score: 5/5

The documentation-only PR appears safe to merge.

Summary

The PR documents a phased backend search capability and its accompanying manual test strategy.

• Defines keyword, semantic, and hybrid search contracts, authorization scoping, filters, pagination, and result hydration.
• Describes phased indexing, collaborative-note projection, transcript grouping, embedding ingestion, and retrieval behavior.
• Adds manual scenarios covering response contracts, pagination, authorization isolation, lifecycle filtering, MCP parity, and semantic recall.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Client[Web UI or MCP client] --> Web[GET /search]
  Web --> Scope[Compile caller scope]
  Scope --> Relationships[Resolve visible relationships]
  Relationships --> Searchers[Bounded per-entity searchers]
  Searchers --> Merge[Merge and rank hits]
  Merge --> Hydrate[Hydrate titles and snippets]
  Hydrate --> Results[Paginated search results]
  Sources[(Postgres entities)] --> Searchers
  Collab[(Collaborative documents)] --> Projection[Plain-text projection]
  Projection --> Searchers
  Chunks[(Semantic search chunks)] --> Searchers

Loading

_{Reviews (4) · Last reviewed commit: "docs(search): cover the coaching_relatio..."}

[DrBourg]

This is a great idea. Would this require us to duplicate our authorization logic? Looking at the next bullet as well. If so, the main concern would be maintaining multiple duplicate rules.

[rnambaale]

Yes the one rule ends up expressed twice, but that's probably not avoidable at reasonable cost.

grants_access_to needs to run in-memory (extractors hold one relationship, roles preloaded, check is free), while search needs the same rule inside Postgres to filter thousands of rows before LIMIT. A single expression would require SELECT EXISTS per authz check or RLS.

So the plan keeps the duplication at exactly two copies with a guard (updated here: afd60f0):

1. One shared visible_relationships fragment - the nine searchers join through it and never write their own scoping SQL (their per-entity part is just the FK path to a relationship id).
2. Tier logic stays in Rust: admin/member org ids are derived from the preloaded roles and enter SQL only as bind parameters.
3. A matrix equivalence test (participant/non-participant × member/removed/admin/ super-admin, two orgs) asserts grants_access_to and the fragment agree for every (user, relationship) pair; we can have unmirrored edits fail CI.

The alternative is coaching_relationships::visible_to(scope) -> Condition on the
entity, next to grants_access_to - same cost and same test, but it becomes a named reusable source of truth for future corpus-wide consumers (MCP search, exports), and the extractors could converge on it later.

I'm happy to explore this path as well.

[DrBourg]

I like that idea a lot if you're willing to give it a shot! Thanks for the explanation and exploration!

[rnambaale]

Updated: 035747d

[DrBourg]

I don't think we actually use notes at the moment. That was an artifact of the past before we started working with TipTap as far as I remember. This could have changed since then though. If that's the case and we do want to use something like this, we'd have to do some sort of import from what is currently in Tiptap. We should consider what tables will be used when we self-host Tiptap. There may be another more logical place to import the body data.

[rnambaale]

Great point. I have eliminated the notes table from search scope. It will remain as a valid search type but will only become active after we have implemented the TipTap projections in PR 5

[jhodapp]

This is looking great Raymond, thank you for this. This review of mine goes through up to the "Response Shape" section with more to follow.

[jhodapp]

Could we make these be a more generalized pattern instead of explicit? Something like an optional id and type of entity param set, like ("0000-00-000-000...", ORGANIZATION_ID_TYPE)? There's probably an even better way of expressing this, but I hope this at least conveys my intent.

[rnambaale]

I considered this, and I thought keeping them explicit would be more idomatic. the catch is the filters aren't actually uniform: user_id means "creator" for notes/goals/actions but "participant" for sessions/transcripts, and goal_id means "linked goal" for actions but "the goal itself" for goals.

A generic (id, entity_type) pair would hide those semantics rather than remove them, and it costs param composability; organization_id=X&user_id=Y&goal_id=Z intersect naturally, while a single pair can only anchor one thing at a time.

The generalization does exist one layer down, though: all the params compile into one Filters struct that every Searcher implementation consumes uniformly, so a future anchor is one field there, not a contract change.

I am happy to iterate on this further, had you thought about it the same way?

[jhodapp]

I think as long as there's a type in place in all relevant layers that results in non-duplicate code and a strong interface contract (where possible), this is good enough. Composability at the param level is important.

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[rnambaale]

@jhodapp Thanks for this comprehensive review.

1. The ANN + authorization filtering recall problem (biggest gap, borderline factual). The plan says "authorization scoping applies identically (the chunk table carries the ids needed to join the same visibility scope)" — as if scoping semantic search is just adding the same WHERE clause. That's true for correctness but wrong for recall. An HNSW scan returns the approximate top-k nearest neighbors of the whole index, and Postgres filters them afterward. A regular user typically sees a tiny slice of the corpus (one or two relationships out of every org's data), so it's entirely plausible that all k nearest neighbors are rows they can't see and the query returns zero hits even though visible matches exist. This is the classic pre-filter/post-filter ANN problem, and for this platform it's acute because the visibility scope is always highly selective for tier-1 users. Mitigations exist — pgvector 0.8+'s iterative index scans (hnsw.iterative_scan), raising hnsw.ef_search, or just doing an exact scan when the visible chunk count is small (a scoped exact scan over a few thousand chunks is fast and gives perfect recall) — but the plan should choose one, and the manual test plan should get a scenario for it ("tier-1 user's semantic search still finds their content when the org corpus is large"). Right now the leak-testing story is thorough but the miss-testing story for semantic doesn't exist.

Great point! The "applies identically" claim was written from the leak-prevention side only; it's true for correctness but, as said, HNSW post-filters the scope join, so a tier-1 user's highly selective scope can filter away all k global neighbors and return a false empty.

The plan now commits to a layered mitigation: iterative index scans (hnsw.iterative_scan = relaxed_order, making pgvector ≥ 0.8 an explicit PR 6 prerequisite to verify against the managed-Postgres offering) as the primary fix, with a scoped exact scan over the pre-filtered visible set when the visible chunk count is small; exactly the population the problem hits, and at that size gives perfect recall cheaply. Raising ef_search alone is considered and set aside (it shifts the failure point rather than removing it).

The miss-testing story now exists on both sides.

bccc74e

---

2. Keyset cursor pagination doesn't transfer to semantic/hybrid, and the plan implies it does. The API contract promises identical response shape and cursor mechanics in every mode, and the elaborate (score DESC, type ASC, id ASC) continuation predicate is designed for deterministic FTS scores. Neither leg carries over cleanly:

• A "resume after distance > X" predicate can't ride the HNSW index; you'd re-run the ANN query with a deeper k and skip, which is a different mechanism than the documented predicate.
• Worse for hybrid: an RRF-fused score is a function of an item's rank within the retrieved lists, so fetching deeper lists on page 2 can change the fused scores of items you already returned. The cursor invariant ("every row below the cursor stays below it") doesn't hold under RRF unless you fix the retrieval depth up front and paginate within that frozen fused list.
  The pragmatic answer is probably "semantic/hybrid retrieve a fixed pool (say top 200 fused) and paginate inside it, documented as bounded-depth" — but that's a materially different contract than keyword's, and deciding it at PR 6/7 time risks discovering the day-one cursor encoding doesn't fit. Since the plan is proud (rightly) of reserving mode so later phases are additive-never-breaking, this is the one place where that promise isn't yet demonstrated.

Agreed on both legs. The plan now commits up front to the pragmatic answer: semantic/hybrid retrieve a fixed pool (~top 200 per arm, pinned with data at PR 6/7), fuse once, and paginate within the frozen pool — next_cursor goes null at exhaustion, documented as bounded-depth. The cursor was already opaque, but its payload now leads with a mode/version discriminator byte so decode can branch on payload shape, and the keyset predicate is explicitly labeled keyword-only.

3cb19d2

---

3. No embedding-model versioning or migration story. vector(1536) hard-codes a specific provider's dimensionality into the schema while the EmbeddingProvider trait exists precisely to keep providers swappable - those two decisions fight each other. Vectors from different models (or even different versions of one model) are not comparable, so a provider/model change means a full re-embed and, if dims change, a column migration plus HNSW rebuild. The fix is cheap at design time: store embedding_model (name+version) on search_chunks, filter retrieval to the current model, and let the backfill job double as the re-embedding path. Absent from the plan entirely.

Right, this was absent. Fixed as suggested: search_chunks now carries embedding_model (name+version), retrieval always filters to the current model. We flip the filter only once the re-embed completes. One caveat added to the doc: HNSW needs a typmod-fixed column, so vector(1536) can't be dim-agnostic — the column makes same-dim swaps free and dim-changing swaps survivable.

3f78281

---

4. Chunk-level hits need entity-level grouping, and the plan already knows this — for transcripts only. PR 2 carefully groups transcript segments per transcription (top 3, max-rank) precisely so long sessions don't flood results. The semantic phase chunks notes and transcripts into overlapping ~300–500-token windows and says nothing about grouping, so a long note whose topic matches the query would return several near-duplicate adjacent-chunk hits (the 15% overlap makes duplicates more likely). The transcript precedent should be stated as the general rule: dedupe/group chunks per entity before the fused merge, group score = max chunk score.

Stated as the general rule now, with one ordering detail made explicit: the collapse happens within each retrieval arm before RRF — raw chunks entering fusion would let one entity hold several rank positions in the semantic arm, crowding out others and distorting its own fused score. Also pinned how it composes with the frozen pool and that the winning chunk supplies the snippet and, for transcripts, the start_ms deep link — same as PR 2's grouped segments.

aeeed2f

---

5. Ingestion covers create/update but not delete/revocation lifecycle. "Embed on create/update via a background job plus a batch backfill" — nothing about deleting chunks when the source row is deleted or soft-deleted (a deleted topic's chunks lingering in search_chunks is exactly the kind of thing scenario E tests for keyword). Also worth stating what mode=semantic does when the embedding provider is down at query time (the query itself must be embedded per-request): 502-equivalent error, or fall back to keyword? The plan's own auto-mode discussion shows the author has thought about fallback, but the failure behavior of the shipped modes is undefined.

Both now pinned.

Sync removal is now part of teh lifecycle — chunk deletion runs inside the same transaction as the source delete/soft-delete/archive (no lag window where deleted content stays findable; query-time re-checks of deleted_at/archived_at were set aside since they'd re-import every searcher predicate into the chunk query).

Outage: semantic and hybrid fail loud with a 502-style upstream error

91e27cc

---

6. Privacy of the corpus leaving the building. Coaching notes and transcripts are about as sensitive as this platform's data gets, and the semantic phase ships them wholesale to an external embedding API. The meeting-AI precedent means the org has swallowed this class of tradeoff before, but the plan should say it out loud — provider choice here is a data-processing decision (DPA, retention, region), not just a trait implementation, and a self-hosted embedding model is a legitimate option the abstraction already permits.

4455b12

---

Two smaller factual nits:

• "CREATE EXTENSION vector … the ALTER TYPE … OWNER TO refactor rule applies to any type created there." Not quite — the vector type is an extension-member object owned by the extension; you don't (and generally can't usefully) reassign its ownership the way the project rule does for hand-created enums. The real operational note is different: CREATE EXTENSION on DO must run as doadmin, but migrations run as refactor, so the extension creation is a manual pre-step outside the migration (same shape as the CONCURRENTLY fallback the plan already documents for PR 2). The plan gestures at this but conflates it with the type-ownership rule.
• The search_chunks column list (entity_type, entity_id, coaching_session_id?, organization_id, …) doesn't include coaching_relationship_id, but the visibility scope is keyed on relationship ids. Goals hang off a relationship with no session, so scoping a goal chunk requires a per-entity-type join back through its source table — which quietly reintroduces the per-type join complexity the unified table was supposed to flatten. Adding coaching_relationship_id (nullable, for users/orgs if they're ever chunked) to the projection makes the scope join uniform: one CTE join for every chunk, no per-type paths.

Both corrected. e439092

---

Peformnace-related reviews

1. Resolve the visibility scope once per request, not once per searcher. As written, every searcher's SQL embeds the visible_relationships CTE, so one request runs that relationship-scoping join 7 times. But the visible relationship set for a caller is small (a handful of ids for a regular user, an org's worth for an admin) and constant for the whole request. Compute it once — a single cheap query, or even derivable in-memory for tier 1 — and pass the ids to each searcher as an array bind: coaching_relationship_id = ANY($ids). Every searcher's query collapses from a join-through-scope to an indexed array membership test, and the equivalence test against grants_access_to still pins the one query that remains. Super admins skip it entirely (the plan already omits the WHERE for them). This is the single most obvious win and costs nothing architecturally — visible_to just gets evaluated once instead of rendered seven times.

Applied. The scoping query now runs once per request in domain::search, producing visible_relationship_ids: Option<Vec>, and every searcher takes it as an array bind.

One trade-off on the "derivable in-memory for tier 1" part: roles are preloaded on AuthenticatedUser but relationships aren't, so it's one indexed query rather than zero.

d970ab8

---

2. Don't compute ts_headline for rows that lose the merge. ts_headline is the notorious FTS cost center — it re-parses the raw document text per row, doesn't use the index, and its cost scales with document length. The plan has each searcher return snippets for its limit + 1 rows, then the domain merges and keeps limit overall — so with 7 searchers at limit 25 you can pay for ~180 headline computations to display 25. Two-pass it: searchers return ids + ranks only; after the merge picks winners, one batched query computes headlines for exactly the returned page. This matters most for precisely the types most likely to match (long note projections, transcript text) and is invisible in the API contract.

Applied as a two-pass design. 6d481c1

One refinement over "one batched query": it's one per entity type present in the page, each over at most a page of ids. Users/orgs are unaffected (ILIKE searchers snippet from name/email fields), transcripts headline only segments whose transcription survives the merge, and the rule carries forward: hybrid never snippets the ~200/arm pool, and semantic-mode chunk snippets hydrate in the same post-merge step.

---

3. The concurrent fan-out will fight the connection pool. "Concurrent fan-out to searchers" means up to ~7 connections per search request, on a pool where — per this codebase's own architecture notes — a request can already hold up to 3 (session auth + middleware + handler), and where pool exhaustion has previously caused production 500s. Search-as-you-type multiplies that. Either bound the searcher concurrency (2–3 at a time; each query is fast, so wall-clock barely moves), or note that the rejected UNION ALL option quietly becomes attractive for connection economics even though it lost on testability — one round-trip, one connection, scope CTE evaluated once. At minimum the plan should acknowledge the pool math; it currently doesn't mention it.

Bounded, as suggested: the fan-out now runs 2–3 searchers in flight, with the pool math acknowledged in the searcher decision bullet, so wall clock barely moves while per-request draw drops from 7 to the bound.

91ce3ff

---

4. Know that ts_rank recomputes the tsvector per matching row — that's the real trigger for stored columns. With expression indexes, the GIN index finds candidate rows, but ranking re-runs to_tsvector() over the raw text for each one (GIN doesn't store the positional data ranking needs). Fine for titles and action bodies; measurable for large note projections. The plan already reserves stored generated tsvector columns as a later move but frames the trigger as "ranking tuning" (setweight). The likelier trigger is this recompute cost on the big-text tables — worth stating so whoever implements PR 5/PR 2 measures it rather than discovering it.

Stated as the primary trigger now: a9eb250.

---

5. Semantic mode: the embedding API call is the latency floor — architect around it. Everything server-side will be single-digit milliseconds; the external embed of the query will be 50–300ms+. Three cheap mitigations the plan omits: (a) in hybrid mode, fire the keyword retrieval in parallel with the embedding call rather than after it — the fusion barrier is the only sync point; (b) an LRU cache of normalized-query → embedding, since search-as-you-type and repeated queries make hit rates real; (c) for tier-1 users, skip HNSW and do an exact scan over their scoped chunks — at a few thousand visible chunks it's faster than tuning ef_search, and it solves the filtered-ANN recall problem from my earlier review as a side effect. Same fix, two birds.

3138dc8

(a) and (b) added as a "Query-time latency budget" bullet: hybrid fires the keyword arm concurrently with the embed call, and a bounded in-process LRU caches query embeddings keyed on. (c) is already in the plan via the bullet for the ANN-recall "scoped exact scan" fallback.

---

6. A product-level mismatch that looks like a perf problem: as-you-type vs. FTS semantics. The throttle policy is explicitly sized for search-as-you-type, but websearch_to_tsquery has no prefix matching — while someone types "quart…", every intermediate keystroke is a full query returning zero hits ("quart" doesn't stem to "quarterly"). That's wasted queries and a dead-feeling UI. Either the FE debounces to word boundaries, or the API prefix-matches the final token (to_tsquery's lexeme:* form, which GIN supports well). Decide it now, because it changes the query construction in PR 1.

Decided for server-side final-token prefixing. 1e60579

I treated debounce as complementary rather than an alternative option here. A debounced query still carries a partially typed final word, which is the normal state of as-you-type.

[jhodapp]

@rnambaale thanks for going through the feedback from Claude in-depth and making it very clear to me what you decided and each commit. Excellent work.

I feel ready for you to proceed if you want to begin implementation, perhaps starting with the first phase and testing it out with a new frontend branch that has a prototype search box in the global header, like this?

The reason I'm recommending trying with a real frontend prototype is so we can try out and get a sense for the latency of search across the API boundary.