feat(groomer): recheck saved empty searches for freshness - #1114
Conversation
…dence Zero-hit search_code calls made global grooming evidence stale on every default-branch commit. The baseline now saves those queries and the freshness pass rechecks them once the code-search index has had time to catch up with the new head; a match or an unrunnable recheck still stales conservatively.
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Not ready yet, though the approach is right. The empty-result signal is solid ( Must fix:
Should fix:
Minor:
|
Review follow-up for #1091: bound the index-lag deferral by the oldest unverified commit so busy repos stale instead of starving forever, share the commit-date fetch per comparison, treat query-budget exhaustion as a deferral, save queries only for negative-search globals (surfaced paths and repository-context evidence stay conservative), sort the migration after everything on main, and use the committer date only.
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Much closer, thanks. The migration rename, the bounded young-head deferral (oldest commit via One real bug is left, and the change opened it: budget exhaustion now defers, but that path isn't bounded the way the young-head path is, and the caps guarantee it happens. To fix it:
Please add tests for an issue with the maximum saved queries completing a recheck, and for a persistently exhausted issue going stale once the oldest unverified commit passes the limit. Two notes, not blockers. The no-read-path rule now requires |
…hable Budget exhaustion now defers within the same oldest-unverified-commit limit as the young-head case instead of deferring forever, the pre-date-fetch exhaustion defers consistently, and saved queries are capped at ten so one issue can always complete its recheck inside the pass budget.
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ies all-or-nothing
Superseded by a newer automated review for this pull request.
There was a problem hiding this comment.
AI Automated Review (incremental)
Incremental review: reviewed the changes since the last managed review; unresolved findings from that review are carried forward.
Analysis engine: glm-5.3-flash@https://litellm.jory.dev/v1 (openai) — routed smart (risk match: db_or_migration_changes)
Recommendation: approve. The incremental delta (e244bcb → 3893e3a) strengthens the PR 1091 search-recheck feature in the right direction: it closes the silent-empty hazard in searchRepositoryCode and replaces lossy query truncation with an all-or-nothing save rule. All hard CI checks pass (Tests, Lint, Typecheck, Database migrations, Database integration); the only failing check is the review check itself (the AI review workflow), not a code gate. Remaining open findings are pre-existing, conservative-direction, and do not block merge.
Change-by-change findings
src/lib/github-code-search.ts(incomplete-results throw) —fetchPaginatednow tracksincomplete_resultsper page, and a 200 response withincomplete_results: trueand zero items throws instead of reporting "no matches". This is the correct fix: the freshness recheck treats empty as proof of absence, so a timed-out search must not advancegroomingVerifiedSha. Note the throw only covers the zero-items case;incomplete_results: truewith non-empty items is still returned as (possibly partial) matches — conservative for the recheck (any match stales) but unpinned by tests (see findings).src/lib/groomer/freshness.ts(all-or-nothing query saving) —emptySearchCodeQueriesnow returns[]when any empty search is unreadable, blank, over 200 chars, or pushes past the 10-query cap, instead of truncating/skipping. This eliminates the P2 truncated-query hazard: the recheck now verdicts only on the exact queries the groom ran. Trade-off (intentional, tested): a pass with 11+ distinct empty searches saves nothing and reverts to stale-on-every-commit for those issues.src/lib/github-code-search.test.ts— new test pins the incomplete-empty throw with an exact message match. The modified dedup test now feeds a 200-char query (the acceptance boundary); the over-length rejection test covers 201. The exact-200-is-saved assertion lives in the modified dedup test whose assertion body is outside the delta hunk — worth a glance but low risk given the 201 rejection test.src/lib/groomer/freshness.test.ts— tests pin: up to 10 queries saved in order; 11 queries → none saved; one unreadable (non-string) or over-length query → none saved. The all-or-nothing rule is well covered.src/lib/groomer/freshness-invalidation.test.ts/freshness-invalidation.ts(unchanged in this delta, re-verified) — the recheck path bounds query count/length before execution: queries come from the DB column written only under the save-time caps, each query costs oneremaining.searchCodeunit, and the commit-date fetch shares that budget once per comparison. Failure of any query stales conservatively; exhaustion defers within the two-hour bound. The test "conservatively stales global evidence without saved search queries" uses therow()defaultgroomedSearchCodeQueries: []and expects stale — so an empty-array baseline (vs never-saved) is pinned to stale conservatively, not treated as "nothing to recheck".
Standards Compliance (AGENTS.md)
- Migration safety:
20261003000000_add_issue_groomed_search_queriesis purely additive —ADD COLUMN IF NOT EXISTS ... TEXT[] NOT NULL DEFAULT ARRAY[]::TEXT[]with a comment. No data loss risk, no rewrites of existing rows, matches the repo's existing migration style. The CI "Database migrations" and "Database integration" checks passed. - Lint/typecheck must pass: both passed on the head commit.
- Error handling / validation: the new throw uses a meaningful message; input validation happens before persistence (type check, trim, length/count caps) per the repo's "validate inputs before database operations" rule.
- Secrets: no token/secret handling in the delta.
- Path handling must_check: the PR's
path_handling_changesrisk flag has no files attached, and nothing in the delta touches file serving or filesystem paths.intersectEvidencePathsoperates only on GitHub repo path strings for prefix matching — no traversal surface, so null-byte/symlink edge-case testing does not apply to this change.
Linked Issue Fit (PR 1091, via requirement ledger)
- Global evidence with only empty searches stays fresh after an unrelated commit when queries still find nothing, and the verified SHA advances — satisfied: "advances global evidence when saved empty queries remain empty" asserts
advancedwithgroomingVerifiedSha: "sha-2". - Stales when a saved query now matches — satisfied, with the matching query named in the stale detail.
- Re-checks limited per pass; failed or over-budget re-check leaves the result unverified, never fresh-and-verified — satisfied: budget exhaustion defers (never advances), a failed query stales conservatively; both are tested. Note the invariant evidence is unit-level with mocked facades; the end-to-end wiring of the real throw is not integrated-tested (see findings).
- Other global cases behave exactly as today — satisfied: no saved queries → stale as before; commit touching a relied-on read path → stale without rechecking; missing
searchCodefacade → stale.
Tool Harness Findings
8 read_file calls confirmed the merged state of freshness-invalidation.ts and freshness.ts: the recheck loop checks remaining.searchCode before each query and before the commit-date fetch, defers within SEARCH_RECHECK_DEFER_LIMIT_MS when the oldest unverified commit is young or the budget is exhausted, stales when the date is missing/unparseable/failed or a query throws, and advances only when every saved query returns empty after the index grace window. The Prisma store's advance/markStale remain guarded on groomedRunId and groomingStaleAt, preserving idempotency. The security lead's concern (recheck must bound query count/length before execution) is confirmed satisfied by this code.
Open Findings From the Previous Review
- P1 (major/bug) — still_open: the incomplete-empty case now throws with a unit test, but the broader throw-on-HTTP-error contract of
searchRepositoryCoderemains unverified from this delta. - P2 (minor/bug) — resolved by the all-or-nothing save rule.
- P3, P4, P5, P6 — still_open / unchanged: this delta touches neither
github-ci.tsnor the compare-endpoint ordering or budget-sharing behavior.
Unknowns or Needs Verification
- Whether
searchRepositoryCodethrows (rather than resolving[]) on non-ok HTTP responses after retries — the P1 residual. If it resolves empty on some failure mode, the recheck could advance on a failed search; a direct unit test for the 4xx/5xx path would close this. - The exact-200-char acceptance boundary assertion in the modified dedup test is outside the delta hunk; verify the 200-char query is asserted as saved.
- The
reviewCI check failure is the AI-review workflow itself, not a code gate; all substantive checks (tests, lint, typecheck, migrations, integration) are green on head commit 3893e3a.
Previous Review Findings
Resolved by this push:
- [P2] Truncating saved queries to 200 chars can change search semantics (dropped trailing terms broaden matching, a cut quoted phrase can invalidate the query), so the recheck verdict is made on an approxim
Still open (carried forward):
- [P1] (major) The recheck's failed-vs-empty decision relies on searchRepositoryCode throwing on API failure, but its error contract is unverified — if it resolves to [] on failure (as compareCommits in the same mod
- [P3] (minor) fetchCommitDate has no direct unit test for its 404-to-null and error-throw paths; it is only asserted to exist in the github-facades export list while all freshness tests mock the facade.
- [P4] (info) fetchCommitDate interpolates repoFullName into the API URL unencoded (ref is encoded), consistent with the existing fetchLatestCommit/fetchRepoJson pattern and sourced from tracked-repo config rather
- [P5] (info) The defer bound assumes GitHub's compare endpoint returns commits oldest-first so commits[0] with per_page=1 is the oldest unverified commit; tests only exercise a single-commit response, so a multi-c
- [P6] (info) The commit-date fetch draws from the same maxSearchCodeRechecks budget as query rechecks, so a pass with many negative-search globals can spend the 20-unit budget on date fetches alone, deferring most
Approval withheld: the previous review of this PR found blocking issues. This clean incremental review is advisory until a full review against a clean baseline confirms the PR as a whole.
| // Code search can time out and still answer 200 with incomplete_results | ||
| // and no items. That is not "no matches": the groomer treats an empty | ||
| // result as proof of absence and the freshness recheck advances on it | ||
| // (#1091), so surface it as a failure instead. |
There was a problem hiding this comment.
Automated finding from AI PR review.
| export async function fetchCommitDate(repoFullName: string, ref: string): Promise<string | null> { | ||
| const response = await fetchWithRetry(`${GITHUB_API}/repos/${repoFullName}/commits/${encodeURIComponent(ref)}`, { | ||
| headers: await getHeadersAsync(), | ||
| }); |
There was a problem hiding this comment.
Minor: fetchCommitDate still has no direct unit test for its 404-to-null and error-throw paths; this delta touches neither github-ci.ts nor its tests.
Automated finding from AI PR review.
| */ | ||
| export async function fetchCommitDate(repoFullName: string, ref: string): Promise<string | null> { | ||
| const response = await fetchWithRetry(`${GITHUB_API}/repos/${repoFullName}/commits/${encodeURIComponent(ref)}`, { | ||
| headers: await getHeadersAsync(), |
There was a problem hiding this comment.
Info (security): fetchCommitDate still interpolates repoFullName unencoded into the API URL, consistent with the existing facade pattern and sourced from tracked-repo config; unchanged by this delta.
Automated finding from AI PR review.
| const data = (await response.json()) as { | ||
| status?: string; | ||
| files?: Array<{ filename?: string; previous_filename?: string }>; | ||
| commits?: Array<{ commit?: { committer?: { date?: string } } }>; |
There was a problem hiding this comment.
Info: The defer bound still assumes the compare endpoint returns commits oldest-first so commits[0] with per_page=1 is the oldest unverified commit; multi-commit ordering remains untested.
Automated finding from AI PR review.
| let headDateState: "unresolved" | "ok" | "failed" | "exhausted" = "unresolved"; | ||
| const resolveHeadDate = async ( | ||
| fetchCommitDate: NonNullable<FreshnessGitHub["fetchCommitDate"]>, | ||
| ): Promise<{ at: number | null; state: "ok" | "failed" | "exhausted" }> => { |
There was a problem hiding this comment.
Info (performance): Commit-date fetches still draw from the same maxSearchCodeRechecks budget as query rechecks; a pass with many negative-search globals can spend the budget on date fetches alone, deferring most rechecks.
Automated finding from AI PR review.
| expect(fetchSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("fails an incomplete search with no items instead of reporting no matches", async () => { |
There was a problem hiding this comment.
Minor (bug): Only the incomplete-with-zero-items case is tested; incomplete_results=true with non-empty items still returns partial matches without any signal, and that behavior is unpinned.
Automated finding from AI PR review.
| for (const call of toolCalls) { | ||
| if (call.name !== "search_code" || !call.ok || call.bytes !== 0) continue; | ||
| // Every empty search is part of the negative evidence. One that can't be | ||
| // saved whole (unreadable, too long, or past the cap) would leave the |
There was a problem hiding this comment.
Info: The all-or-nothing rule means a grooming pass with 11+ distinct empty searches (or one unreadable/over-length query) persists no queries and falls back to stale-on-every-commit; conservative and intentional, but busy repos may see PR 1091 re-groom churn return for those issues.
Automated finding from AI PR review.
1 participant
Zero-hit
search_codecalls made global grooming evidence stale on every default-branch commit, re-grooming busy repos constantly (#1091).The baseline now saves the empty queries (
Issue.groomedSearchCodeQueries, additive migration) and the freshness pass rechecks them after the head moves: all still empty advancesgroomingVerifiedSha; any match stales with the query named. Conservative fallbacks: a failed recheck, a missing commit date, or a commit touching a relied-on read path stales as before; the deferral for a young head is bounded — once the oldest unverified commit outlives a two-hour limit the result stales instead of starving; and query-budget exhaustion defers rather than stales. Queries are saved only for negative-search globals: surfaced-path globals and no-read-path globals whose evidence came from repository context stay fully conservative.Follow-ups: repository-context searches are not saved yet (#1115); the apply-time head precondition can still disagree with a freshness pass that accepted the evidence (#1116).
Closes #1091.