Repository navigation
UN-4628 [FEAT] Global search: split adapters into LLM, vector DB, embedding and text extractor types - #2457
Open
ritwik-g wants to merge 2 commits into
Open
UN-4628 [FEAT] Global search: split adapters into LLM, vector DB, embedding and text extractor types#2457ritwik-g wants to merge 2 commits into
ritwik-g wants to merge 2 commits into
Conversation
Replace the single `adapter` search type with `llm`, `vector_db`, `embedding` and `text_extractor`, one per v1 rail destination, each with its own limit and has_more. OCR adapters are no longer returned. All four keep the shared adapter.detail / adapter.info route keys, which the notifications inbox also uses; adapter_type in the params picks the page. UN-4628 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ritwik-g
commented
Nov 6, 2026
ritwik-g
left a comment
ritwik-g
left a comment
Contributor
Author
There was a problem hiding this comment.
Standard review — round 1 of the self-review loop (tip 752705e09, review-deep, INITIAL)
Review choice: review-deep. The diff changes the search endpoint's wire format (its result type keys), which is a review-shallow disqualifier. Scope: merge base 849889259 to 752705e09, 7 files and 173 lines; the count matches GitHub. Signals raised: lens 7 (type keys renamed), lens 13 (tests changed) and lens 16 (README and docstrings changed). Security deep-dive: not triggered. No endpoint, auth, parser or input boundary changed; the change only narrows an existing queryset. Boundary sweep: 0 new sinks traced, clean.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 1 · Medium: 0 · Low: 3 · Lenses run: 19/19
Findings — posted inline below (4).
Unanchored findings
- [Low] [Lens 7] The UI test suite (
Zipstack/unstract-ui-test-automation,playwright-typescript/src/api/search.ts:22) still listsadapterinTYPE_ORDER. Its spec checks the order of known types only and tolerates unknown ones, so nothing fails, but the four adapter groups stop being order-checked. Follow-up there: replaceadapterwith the four keys, and add a COMPATIBILITY note on the release boundary.
Lens checklist
- Spec & intent — Clean — assessed by code-reviewer
- Architectural fit & precedent — Clean — assessed by code-reviewer
- Correctness & edge cases — Clean — assessed by code-reviewer
- Security — Clean — assessed by orchestrator, the visibility rule is unchanged, and
.filter(adapter_type=…)sits on top offor_user(), so it can only narrow results - Data integrity & migrations — N/A — read-only query change, no models or migrations
- Concurrency — N/A — no shared state, locks or async ordering
- API & contract compatibility — See findings #1 — assessed by type-design-analyzer for the contract shape, the orchestrator did the cross-repo consumer search
- Reliability & resilience — Clean — assessed by orchestrator, each new type is its own provider, so a failure lands in
failed_typesper type, as before - Performance & cost — Clean — assessed by code-reviewer, budget test p95 ≈ 55 ms, query bound scales per provider
- Observability — Clean — assessed by code-reviewer
- Operational safety — N/A — no flag, rollout or config surface
- LLM/agent-specific — N/A — no model calls or prompts
- Testing — Clean — assessed by pr-test-analyzer, 59 passed against real Postgres, three mutants killed
- Dependencies & build — N/A — no dependency changes
- Code quality — Clean — assessed by code-reviewer, orchestrator ran the pinned ruff v0.3.4 on the changed files. The only hits are on pre-existing lines,
base.py:132D209 andtest_contract.py:26N814, both unchanged from main. - Doc & comment accuracy — See findings #2, #3, #4 — assessed by comment-analyzer
- Cross-document rule consistency — Clean — assessed by orchestrator, merge-result sweep of the new identifiers, hits are only the PR's own lines and unrelated
adapter-opsskill docs. The out-of-repo restatement is reported under lens 7. - On-prem deployment experience — N/A — no chart, values, schema or on-prem build surface
- Scaling & workload lifecycle — N/A — no worker, concurrency or autoscaling values
Open questions for the author — none.
Assumptions made — that no released client sends or handles adapter. This rests on the consumer search above; a client that does would need an alias, not just a note.
cost: agents=4 wall=03:53 lenses-examined=12 files=7 lines=173
Unanchored findings
backend/global_search/oss_providers.py:5(outside the diff hunk): [Low] [Lens 16 — Doc accuracy] — The module docstring's list of narrowings leaves out the new adapter-kind filter- Location:
backend/global_search/oss_providers.py:L5-L6. This line isn't in the diff; the new filter at L177 made it incomplete. - Failure mode: someone reading why an OCR adapter that appears in the list is missing from search won't find the reason in this docstring.
- Suggested fix: add "adapter kind (OCR excluded)" to the parenthetical.
- Confidence: Medium
- Verification: VERIFIED by reading the docstring against
visible_queryset.
- Location:
…ture comments Round-1 self-review fixes: record in the README that the four adapter types replace the released `adapter` type (and that OCR results are dropped), say the inbox sends only adapter.info, drop a fixture comment that claimed every kind opens on adapter.detail, and list the adapter-kind narrowing in the module docstring. UN-4628 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ritwik-g
commented
Nov 6, 2026
ritwik-g
left a comment
ritwik-g
left a comment
Contributor
Author
There was a problem hiding this comment.
Standard review — round 2 of the self-review loop (tip 6766873f1, review-deep, FOLLOWUP)
Review choice: review-deep again. The PR as a whole still changes the search response's type keys, so review-shallow doesn't apply. Round delta: 752705e09..6766873f1, covering README.md, the oss_providers.py docstrings and a tests/base.py comment.
Verdict — APPROVE
Prior context fetched — followup-fetch.sh exited 0. The "since" boundary is 752705e09 (review 5799140571).
Scope change — NO
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0 · Lenses run: 19/19
Prior findings status
- #1 [High] The released
adaptersearch type was removed with no migration note →RESOLVEDin 6766873.backend/global_search/README.md:133-138now names the removed key, its four replacements, the dropped OCR results and the release boundary (v0.194.0). That claim was checked against the release tags. - #2 [Low] Inbox said to use both route keys →
RESOLVEDin 6766873 —oss_providers.py:154-157,README.md:129-131 - #3 [Low] Fixture comment claimed every kind opens on adapter.detail →
RESOLVEDin 6766873 —tests/base.py:122 - #4 [Low] Module docstring omitted the adapter-kind narrowing →
RESOLVEDin 6766873 —oss_providers.py:5-6
New findings — none. Two agents examined the round delta (code-reviewer and comment-analyzer); both reported clean after checking each new statement against the code and release tags. The suite was re-run: 59 passed against live Postgres and Redis.
Lens checklist
- Spec & intent — Clean — assessed by code-reviewer
- Architectural fit & precedent — Clean — assessed by code-reviewer
- Correctness & edge cases — Clean — assessed by code-reviewer
- Security — N/A — this round changes only comments, docstrings and README text
- Data integrity & migrations — N/A — no models or migrations
- Concurrency — N/A — no runtime change this round
- API & contract compatibility — Clean — assessed by orchestrator, finding 1 is resolved by the README migration note and the wire format is unchanged this round
- Reliability & resilience — N/A — no runtime change this round
- Performance & cost — Clean — assessed by code-reviewer
- Observability — Clean — assessed by code-reviewer
- Operational safety — N/A — no flag, rollout or config surface
- LLM/agent-specific — N/A — no model calls or prompts
- Testing — Clean — assessed by code-reviewer as test runner, 59 passed, and only a test comment changed
- Dependencies & build — N/A — no dependency changes
- Code quality — Clean — assessed by code-reviewer, orchestrator ran the pinned ruff v0.3.4 format check on the changed Python files, clean
- Doc & comment accuracy — Clean — assessed by comment-analyzer
- Cross-document rule consistency — Clean — assessed by orchestrator, the inbox route-key statement now matches backend/inbox/events.py and inbox/ReadMe.md
- On-prem deployment experience — N/A — no on-prem surface
- Scaling & workload lifecycle — N/A — no worker or scaling values
Open questions / assumptions — this assumes no released client outside the repos searched in round 1 sends or handles adapter.
cost: agents=2 wall=02:26 lenses-examined=10 files=3 lines=25
ritwik-g
marked this pull request as ready for review
November 6, 2026 15:38
Contributor
 [Medium impact] The PR appears safe to merge.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
llm(LLMs),vector_db(Vector DBs),embedding(Embedding) andtext_extractor(Text Extractor). Before, all adapters came back as a singleadaptertype.limitandhas_more, in rail order (80 to 83), between task pipelines and connectors.adapter.detail/adapter.inforoute keys, withidandadapter_typeas params.Why
How
AdapterProviderbecomes an unregistered base class that filters on itsadapter_type. Four subclasses register under the new keys.backend/inbox/events.py) already usesadapter.info.adapter_typein the params tells the client which adapter page to open.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
adaptersearch type key is renamed, but nothing consumes it yet: the P2-T10 panel isn't built.agentic_project,review_queue) and the inbox don't reference theadaptertype key, and none of the route keys change.types=adapternow gets no adapter groups, because unknown keys are ignored by contract.Database Migrations
Env Config
Relevant Docs
backend/global_search/README.md, Types table.Related Issues or PRs
Dependencies Versions
Notes on Testing
pytest backend/global_searchagainst Postgres 15 (pgvector) and Redis: 59 passed.adapter_typefilter removed, this test and the per-type contract and ranking tests fail.Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code