Repository navigation
feat(neighbors): filter by state and merge time before ranking - #239
masatohoshino wants to merge 2 commits into
Conversation
Add --merged-after, --open-at, and --created-after to neighbors. Each takes a time (--created-after also takes an issue or pull request reference) and is applied in SQL before vectors are scored and the limit is taken, so rows that rank below the limit are no longer dropped by a later filter. Each row now carries state, html_url, is_draft and, when set, the author, author association, and GitHub created, closed, and merged times; the output also reports state_as_of, the start of the newest complete open or all sync. Existing fields and their order are unchanged. Without the new flags the vector filter emits the same SQL and arguments as before, which a test pins against the previous implementation, and golden files pin the default neighbors output. docs/clustering.md describes how far row state and the time filters can be trusted.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. What this changesThis PR adds three filters before neighbor ranking, thread metadata in results, and a complete-sync freshness timestamp. Example: Find similar PRs merged after openclaw/openclaw pull request 160462 was created at 2026-09-28T12:59:02Z.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 1 item remains Keep this PR open: current main lacks these filters, the contribution demonstrates useful behavior, and no actionable introduced defect was found; the public CLI surface still needs owner direction. Priority: P2 Decision needed
Before merge
FindingsNone. Agent review detailsHow this fits togetherThe neighbors CLI reads cached SQLite thread embeddings, selects eligible vectors, ranks similarity, and returns hydrated thread metadata alongside recorded sync freshness. flowchart LR
A[CLI arguments] --> B[Neighbors command]
C[SQLite archive] --> D[Filtered vectors]
B --> D
D --> E[Similarity ranking]
E --> F[Neighbor output]
C --> G[Sync freshness]
G --> F
Technical reviewBest possible solution: Add approved filters within the existing neighbor query while preserving default selection, scoring, storage, and documented history limitations. Do we have a high-confidence way to reproduce the issue? Current-main source confirms the missing filters and metadata, and supplied baseline CLI output demonstrates the expensive workaround; this review executed no target code. Is this the best way to solve the issue? Filtering in the existing SQL query before decoding and scoring directly addresses the workload without a competing implementation or storage migration. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against bd30790d7735. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 10, 2026, 12:56 PM ET / 16:56 UTC (Revision 2). |
Drop TestThreadVectorWhereAppendsOnlySetFilters, which restated the SQL fragments of the new filters. The boundaries it guarded are now rows of TestNeighborsTimeFilters checked by what neighbors returns: a row created, closed or merged exactly at the given time falls on the documented side of each filter, and each filter compares instants, not text, for a time with a fraction of a second. TestThreadVectorWhereWithoutTimeFiltersIsUnchanged still pins the default query.
This branch has not been deployed
1 participant
Closes #238
What Problem This Solves
Checking whether an open pull request was already landed by a later one means finding similar pull requests merged after it was created, but
neighborscannot tell a merged row from a closed one or filter by time, so the only route is--include-closedwith a very large--limitplus a separatethreadsdump, about 18 s per check on a warm cache.User Impact
User impact:
gitcrawl neighbors <repo> --number <n> --merged-after <time>answers that question in about 1.4 s (median; p95 4.3 s) on a 165k-thread archive instead of about 18 s, and each row now shows its state, URL, author and times. Existing fields and their order, the default query, the schema and the default timing are unchanged; the new fields are additive.Why This Change Was Made
neighborsgains three filters, each applied in SQL before vectors are scored and before--limit, so rows that rank below the limit are no longer lost:--merged-after <time>: pull requests merged after the time.--open-at <time>: rows open at the time (created by then, not closed by then).--created-after <time|ref>: rows created after the time; a thread reference compares numbers, which GitHub assigns in creation order.--merged-afterand--open-atimply--include-closedand take a time; a pull request's creation time comes fromgitcrawl threads owner/repo --numbers N --json | jq -r '.threads[0].created_at_gh'(accepting a thread reference there too would be a small follow-up if wanted). Each row additionally carriesstate,html_url,is_draftand, when set,author_login,author_association,created_at_gh,closed_at_gh,merged_at_ghandclosed_at_local. The output also reportsstate_as_of: the start of the newest successful complete open or all sync, read from the existingclosed_sweep_throughinsync_runs;sync --numbers,--limitand--sinceruns do not move it. Without the new flags, the shared vector filter builds the same SQL and arguments as before.The archive keeps current values, not history, so the filters can only err in one direction each:
--open-atcan include a row that was closed at that moment and later reopened, never drop one that was open, and--merged-aftercan only miss merges afterstate_as_of. These directions, and each row'sstate, assume closed-thread sweeps have covered the archive since its rows were stored (an archive's first sweep reaches back only to the earliest pull of a still-open row, or 24 hours;sync --state allrefreshes every row).docs/clustering.mddocuments this.Cold-cache runs gain less than warm ones: the time columns sit after
bodyandraw_jsonin each row, so the filter still reads every row, and the saving comes from decoding and scoring only the rows that pass. The schema is left as is.Evidence
Built CLI on a read-only copy of an openclaw/openclaw archive (165,815 threads, 3,306 open pull requests,
state_as_of2026-10-10T09:06:46Z,qwen3-embedding:0.6b1024-dimension vectors), this branch's code against main bd30790. For 10 open pull requests (one per decile of later-merge count) and two questions each, the top 10 matched main's fastest equivalent in every case, warm and cold:--include-closed --limit 32765, then a local filter** main's rows carry no state or merge time, so the filter also needs one
threads --include-closed --jsondump (992 MB): 5.5 s warm, 32 s cold. main's time does not depend on--limit(18.0–18.5 s at 1,000, 5,000 and 32,765), because it scores all 165,815 stored vectors on every call; above 32,765 the CLI fails withtoo many SQL variables.--merged-aftertheir creation time, warm: median 1.41 s, p95 4.29 s, max 5.75 s, no errors.neighbors1.05 s vs 1.06 s,neighbors --include-closed17.58 s vs 17.57 s (medians of 5, alternating builds);search,clustersandcluster-detailoutput byte-identical; the schema of a freshly initialized database is identical.--open-atrows, and all 1,100--merged-afterrows match GitHub'smergedAt.TestNeighborsTimeFilterschecks the new filters by the rowsneighborsreturns, including rows exactly at each boundary and times with a fraction of a second. Only the default path is pinned at the SQL level:TestThreadVectorWhereWithoutTimeFiltersIsUnchangedcompares the filter's SQL and arguments with the previous implementation for all 16 combinations of the existing options, andTestNeighborsDefaultOutputGoldenpins the defaultneighborsoutput (JSON and text). The golden files generated with main's code differ from these only by added lines for the new keys (94 added, 0 removed). The new tests fail on main and catch 23 deliberately wrong variants (for example>=instead of>, times compared as text, a no-op clause on the default path, orstate_as_ofmoved bysync --numbers).go test ./...,go vet, govulncheck, deadcode andmake smoke/docspass locally, exceptTestRefreshPortableStoreRecoversFromStaleIndexLock, which fails the same way on unchanged main on this machine (portable store checkout has local changes).gofmt -llists only the untouchedinternal/cli/cloud_ingest.go, also on main.CLI output: one check, warm cache (… marks omitted lines)