From 41535e72e7883313d3c44e7604753455cb926887 Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Sun, 27 Sep 2026 13:57:45 -0600 Subject: [PATCH 1/4] feat(groomer): recheck saved empty searches before staling global evidence 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. --- .../migration.sql | 6 + prisma/schema.prisma | 1 + src/lib/github-ci.ts | 18 +++ src/lib/github-facades.test.ts | 1 + .../groomer/freshness-invalidation.test.ts | 93 +++++++++++++++- src/lib/groomer/freshness-invalidation.ts | 105 ++++++++++++++++-- src/lib/groomer/freshness.test.ts | 42 ++++++- src/lib/groomer/freshness.ts | 26 +++++ src/lib/groomer/run.ts | 4 +- 9 files changed, 283 insertions(+), 13 deletions(-) create mode 100644 prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql diff --git a/prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql b/prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql new file mode 100644 index 00000000..e4b80f5f --- /dev/null +++ b/prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql @@ -0,0 +1,6 @@ +-- Preserve empty code-search queries so freshness checks can verify whether +-- repo-wide negative evidence still holds after a default-branch commit. +ALTER TABLE "Issue" + ADD COLUMN IF NOT EXISTS "groomedSearchCodeQueries" TEXT[] NOT NULL DEFAULT ARRAY[]::TEXT[]; + +COMMENT ON COLUMN "Issue"."groomedSearchCodeQueries" IS 'Successful empty search_code queries backing global grooming evidence'; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index f338fc12..7c5e8a06 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -72,6 +72,7 @@ model Issue { groomedEvidenceCapturedAt DateTime? groomedEvidenceScope String? groomedEvidencePaths String[] @default([]) + groomedSearchCodeQueries String[] @default([]) groomedDependencyKeys String[] @default([]) groomedOpenBlockerKeys String[] @default([]) groomedRelatedWork Json? diff --git a/src/lib/github-ci.ts b/src/lib/github-ci.ts index 9e5ddd01..7adeb2a1 100644 --- a/src/lib/github-ci.ts +++ b/src/lib/github-ci.ts @@ -160,6 +160,24 @@ export async function fetchLatestCommit(repoFullName: string, branch: string): P return response.json(); } +/** + * Committer timestamp (ISO string) for an existing commit sha, or null when + * the ref does not resolve. Used by the grooming freshness pass to judge + * whether the code-search index has had time to catch up with a new head. + */ +export async function fetchCommitDate(repoFullName: string, ref: string): Promise { + const response = await fetchWithRetry(`${GITHUB_API}/repos/${repoFullName}/commits/${encodeURIComponent(ref)}`, { + headers: await getHeadersAsync(), + }); + if (!response.ok) { + if (response.status === 404) return null; + const text = await response.text(); + throw new Error(`Failed to fetch commit ${ref} for ${repoFullName}: ${response.status} ${text}`); + } + const data = (await response.json()) as { commit?: { committer?: { date?: string }; author?: { date?: string } } }; + return data.commit?.committer?.date ?? data.commit?.author?.date ?? null; +} + export function jobIdFromCheckRunUrl(url: string | undefined | null): string | null { const m = /\/job\/(\d+)/.exec(url ?? ""); return m ? m[1] : null; diff --git a/src/lib/github-facades.test.ts b/src/lib/github-facades.test.ts index d54eb211..9741d5fa 100644 --- a/src/lib/github-facades.test.ts +++ b/src/lib/github-facades.test.ts @@ -34,6 +34,7 @@ describe("github domain modules expose expected exports", () => { it("github-ci exports CI/workflows/runs/jobs/releases/packages/commits/logs symbols", () => { expect(Object.keys(Ci).sort()).toEqual([ "extractLogExcerpt", + "fetchCommitDate", "fetchFailedJobLogExcerpt", "fetchLatestCommit", "fetchPackages", diff --git a/src/lib/groomer/freshness-invalidation.test.ts b/src/lib/groomer/freshness-invalidation.test.ts index a5d35e0c..14c75557 100644 --- a/src/lib/groomer/freshness-invalidation.test.ts +++ b/src/lib/groomer/freshness-invalidation.test.ts @@ -34,6 +34,7 @@ function row(overrides: Partial = {}): FreshnessIssueRow { groomedEvidenceCapturedAt: CAPTURED, groomedEvidenceScope: "paths", groomedEvidencePaths: ["src/a.ts"], + groomedSearchCodeQueries: [], groomedDependencyKeys: [], groomedOpenBlockerKeys: [], groomedRelatedWork: null, @@ -86,9 +87,12 @@ function fakeStore(rows: FreshnessIssueRow[]): FakeStore { function fakeGitHub() { return { fetchHeadSha: vi.fn(async (): Promise => "sha-1"), + searchCode: vi.fn(async (): Promise<{ path: string }[]> => []), compareCommits: vi.fn( async (): Promise => ({ ok: true, status: "ahead", files: [], truncated: false }), ), + // Old enough that the code-search index is assumed caught up (#1091). + fetchCommitDate: vi.fn(async (): Promise => new Date(Date.now() - 3_600_000).toISOString()), fetchRecentComments: vi.fn(async (): Promise> => []), fetchIssueState: vi.fn(async (): Promise<"open" | "closed" | null> => "open"), fetchPullRequestState: vi.fn(async (): Promise<"open" | "closed" | "merged" | null> => "open"), @@ -263,12 +267,99 @@ describe("runGroomingFreshnessPass", () => { expect(store.stale.get("issue-1")?.detail).toContain("src/a.ts"); }); - it("conservatively stales global (negative) evidence on any commit", async () => { + it("conservatively stales global evidence without saved search queries", async () => { github.fetchHeadSha.mockResolvedValue("sha-2"); github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: ["docs/readme.md"], truncated: false }); const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [] }]); await pass(store, github); expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(github.searchCode).not.toHaveBeenCalled(); + }); + + it("advances global evidence when saved empty queries remain empty", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: ["docs/readme.md"], truncated: false }); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["new symbol", "missing call"] }]); + await pass(store, github); + expect(store.stale.size).toBe(0); + expect(store.advanced).toEqual([{ id: "issue-1", data: { groomingVerifiedSha: "sha-2" } }]); + expect(github.searchCode).toHaveBeenCalledTimes(2); + }); + + it("stales global evidence when a saved query now matches", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + github.searchCode.mockResolvedValueOnce([]).mockResolvedValueOnce([{ path: "src/new.ts" }]); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["first", "specific missing query"] }]); + await pass(store, github); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(store.stale.get("issue-1")?.detail).toContain("specific missing query"); + }); + + it("falls back to global staleness when the search budget is exhausted", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["first", "second"] }]); + await pass(store, github, { ...DEFAULT_FRESHNESS_BUDGET, maxSearchCodeRechecks: 2 }); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + // One budget unit goes to the commit-date check, one to the first query. + expect(github.searchCode).toHaveBeenCalledTimes(1); + }); + + it("defers the recheck while the new head is younger than the index grace window", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + github.fetchCommitDate.mockResolvedValue(new Date(Date.now() - 60_000).toISOString()); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }]); + const result = await pass(store, github); + expect(store.stale.size).toBe(0); + expect(store.advanced).toEqual([]); + expect(github.searchCode).not.toHaveBeenCalled(); + expect(result.deferred).toBe(1); + }); + + it("falls back to global staleness when the commit date is missing or unavailable", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + const issue = { ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }; + github.fetchCommitDate.mockResolvedValue(null); + const missing = fakeStore([issue]); + await pass(missing, github); + expect(missing.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + + github.fetchCommitDate.mockRejectedValueOnce(new Error("boom")); + const failed = fakeStore([issue]); + await pass(failed, github); + expect(failed.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + }); + + it("stales a global result whose relied-on read path the commit touched, without rechecking", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: ["src/a.ts"], truncated: false }); + const store = fakeStore([ + { ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: ["src/a.ts"], groomedSearchCodeQueries: ["query"] }, + ]); + await pass(store, github); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(store.stale.get("issue-1")?.detail).toContain("src/a.ts"); + expect(github.searchCode).not.toHaveBeenCalled(); + }); + + it("falls back to global staleness when search is missing or fails", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + const issue = { ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }; + const missing = fakeStore([issue]); + const withoutSearch = { ...github } as { searchCode?: typeof github.searchCode } & Omit, "searchCode">; + delete withoutSearch.searchCode; + await pass(missing, withoutSearch as ReturnType); + expect(missing.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + + github.searchCode.mockRejectedValueOnce(new Error("rate limited")); + const failed = fakeStore([issue]); + await pass(failed, github); + expect(failed.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(failed.stale.get("issue-1")?.detail).toBe("default branch moved sha-1...sha-2 and the result relied on repo-wide evidence"); }); it("ignores commits for a result that used no repository evidence", async () => { diff --git a/src/lib/groomer/freshness-invalidation.ts b/src/lib/groomer/freshness-invalidation.ts index 036d070a..a6fd6351 100644 --- a/src/lib/groomer/freshness-invalidation.ts +++ b/src/lib/groomer/freshness-invalidation.ts @@ -16,8 +16,8 @@ import { prisma } from "@/lib/prisma"; import { fetchIssue, fetchIssueComments } from "@/lib/github-issues"; import { fetchPullRequestState } from "@/lib/github-prs"; -import { fetchLatestCommit } from "@/lib/github-ci"; -import { compareCommits, type CommitComparison } from "@/lib/github-code-search"; +import { fetchLatestCommit, fetchCommitDate } from "@/lib/github-ci"; +import { compareCommits, searchRepositoryCode, type CommitComparison } from "@/lib/github-code-search"; import { dependencyKey } from "@/lib/issue-dependencies"; import { findOpenIssueKeys } from "@/lib/issue-dependency-annotation"; import { isAutomationAuthor } from "./context"; @@ -52,6 +52,7 @@ export interface FreshnessIssueRow { groomedEvidenceCapturedAt: Date | null; groomedEvidenceScope: string | null; groomedEvidencePaths: string[]; + groomedSearchCodeQueries: string[]; groomedDependencyKeys: string[]; groomedOpenBlockerKeys: string[]; groomedRelatedWork: unknown; @@ -79,6 +80,9 @@ export interface FreshnessStore { export interface FreshnessGitHub { fetchHeadSha(repoFullName: string, branch: string): Promise; + searchCode?(repoFullName: string, query: string, limit: number): Promise<{ path: string }[]>; + /** Committer timestamp for a sha; used to judge code-search index catch-up. */ + fetchCommitDate?(repoFullName: string, sha: string): Promise; compareCommits(repoFullName: string, base: string, head: string): Promise; fetchRecentComments( repoFullName: string, @@ -98,6 +102,8 @@ export interface FreshnessBudget { maxCommentFetches: number; /** Related issue/PR state reads per pass (tracked issues come from the cache, free). */ maxRelatedFetches: number; + /** Saved negative code-search queries rechecked per pass. */ + maxSearchCodeRechecks: number; /** Comments read per comment check. */ commentWindow: number; } @@ -107,9 +113,18 @@ export const DEFAULT_FRESHNESS_BUDGET: FreshnessBudget = { maxCompares: 10, maxCommentFetches: 10, maxRelatedFetches: 10, + maxSearchCodeRechecks: 20, commentWindow: 30, }; +/** + * GitHub code search runs against an index that can lag the default branch. + * A saved empty query only counts as "still absent" once the new head commit + * is at least this old; younger heads defer the recheck to a later pass + * (#1091). Failures and missing timestamps stay conservative instead. + */ +export const SEARCH_RECHECK_INDEX_GRACE_MS = 30 * 60 * 1000; + export interface FreshnessPassResult { issuesChecked: number; markedStale: Array<{ repo: string; issueNumber: number; reasons: GroomingStaleReason[] }>; @@ -160,6 +175,7 @@ export async function runGroomingFreshnessPass( compares: budget.maxCompares, comments: budget.maxCommentFetches, related: budget.maxRelatedFetches, + searchCode: budget.maxSearchCodeRechecks, }; for (const repo of repos) { @@ -177,7 +193,7 @@ async function evaluateRepo( store: FreshnessStore, github: FreshnessGitHub, budget: FreshnessBudget, - remaining: { compares: number; comments: number; related: number }, + remaining: { compares: number; comments: number; related: number; searchCode: number }, result: FreshnessPassResult, now: () => Date, ): Promise { @@ -284,7 +300,7 @@ async function evaluateRepo( } // 5. Default-branch commits since the last verified SHA. - await evaluateCommits(repo.fullName, live(), github, remaining, result); + await evaluateCommits(repo.fullName, live(), github, remaining, result, now); // Persist. const at = now(); @@ -342,8 +358,9 @@ async function evaluateCommits( repoFullName: string, evaluations: Evaluation[], github: FreshnessGitHub, - remaining: { compares: number }, + remaining: { compares: number; searchCode: number }, result: FreshnessPassResult, + now: () => Date, ): Promise { const sensitive = evaluations.filter( (evaluation) => @@ -399,19 +416,22 @@ async function evaluateCommits( remaining.compares--; result.githubCalls++; const comparison = await github.compareCommits(repoFullName, base, head); - applyComparison(members, comparison, base, head, repoFullName, result); + await applyComparison(members, comparison, base, head, repoFullName, github, remaining, result, now); } } } -function applyComparison( +async function applyComparison( members: Evaluation[], comparison: CommitComparison, base: string, head: string, repoFullName: string, + github: FreshnessGitHub, + remaining: { searchCode: number }, result: FreshnessPassResult, -): void { + now: () => Date, +): Promise { const range = `${base.slice(0, 12)}...${head.slice(0, 12)}`; if (!comparison.ok) { if (comparison.definitive) { @@ -435,6 +455,72 @@ function applyComparison( } for (const evaluation of members) { if (evaluation.issue.groomedEvidenceScope === "global") { + const queries = evaluation.issue.groomedSearchCodeQueries ?? []; + // A commit that touches a relied-on read path invalidates the result + // regardless of what the saved searches say (#1091: other global + // evidence keeps the conservative behaviour). + const pathHits = intersectEvidencePaths(evaluation.issue.groomedEvidencePaths, comparison.files); + if (pathHits.length > 0) { + const shown = pathHits.slice(0, 5).join(", ") + (pathHits.length > 5 ? `, +${pathHits.length - 5} more` : ""); + stale(evaluation, "global_evidence_commit", `default branch moved ${range}; commit touched relied-on evidence paths: ${shown}`); + continue; + } + if (queries.length > 0 && github.searchCode && github.fetchCommitDate) { + if (remaining.searchCode <= 0) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); + continue; + } + remaining.searchCode--; + result.githubCalls++; + let committedAt: string | null = null; + try { + committedAt = await github.fetchCommitDate(repoFullName, head); + } catch { + committedAt = null; + } + const at = committedAt ? Date.parse(committedAt) : Number.NaN; + if (Number.isNaN(at)) { + // No trustworthy timestamp: cannot confirm the index caught up, so + // stay conservative (stale), never fresh-and-verified. + stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); + continue; + } + if (now().getTime() - at < SEARCH_RECHECK_INDEX_GRACE_MS) { + // The head is too recent for the code-search index to have caught + // up; an empty recheck now would not mean "still absent". Defer to + // a later pass without advancing or staling. + evaluation.deferred = true; + continue; + } + let matchedQuery: string | null = null; + let allEmpty = true; + for (const query of queries) { + if (remaining.searchCode <= 0) { + allEmpty = false; + break; + } + remaining.searchCode--; + result.githubCalls++; + try { + if ((await github.searchCode(repoFullName, query, 1)).length > 0) { + matchedQuery = query; + allEmpty = false; + break; + } + } catch { + allEmpty = false; + break; + } + } + if (matchedQuery) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range}; previously empty search query now matches: ${matchedQuery}`); + continue; + } + if (allEmpty) { + evaluation.advance.groomingVerifiedSha = head; + continue; + } + } stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); continue; } @@ -562,6 +648,7 @@ const FRESHNESS_SELECT = { groomedEvidenceCapturedAt: true, groomedEvidenceScope: true, groomedEvidencePaths: true, + groomedSearchCodeQueries: true, groomedDependencyKeys: true, groomedOpenBlockerKeys: true, groomedRelatedWork: true, @@ -634,6 +721,8 @@ export const defaultFreshnessGitHub: FreshnessGitHub = { return (await fetchLatestCommit(repoFullName, branch))?.sha ?? null; }, compareCommits, + searchCode: searchRepositoryCode, + fetchCommitDate, async fetchRecentComments(repoFullName, issueNumber, max) { const comments = await fetchIssueComments(repoFullName, issueNumber, max, "desc"); return comments.map((comment) => ({ author: comment.user?.login ?? "unknown", createdAt: comment.created_at ?? "" })); diff --git a/src/lib/groomer/freshness.test.ts b/src/lib/groomer/freshness.test.ts index daee31d4..0861ccef 100644 --- a/src/lib/groomer/freshness.test.ts +++ b/src/lib/groomer/freshness.test.ts @@ -259,11 +259,49 @@ describe("buildGroomingFreshnessBaseline", () => { expect(baseline.groomedEvidencePaths).toEqual([]); }); - it("marks a negative code search as global evidence", async () => { + it("records bounded, deduplicated empty-search queries only for global evidence", async () => { + const calls = [ + { name: "search_code", ok: true, bytes: 0, arguments: { query: " missing symbol " } }, + { name: "search_code", ok: true, bytes: 0, arguments: { query: "missing symbol" } }, + { name: "search_code", ok: true, bytes: 0, arguments: { query: "x".repeat(250) } }, + { name: "search_code", ok: true, bytes: 0, arguments: { query: " " } }, + { name: "search_code", ok: false, bytes: 0, arguments: { query: "failed" } }, + ]; + const global = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: calls })); + expect(global.groomedEvidenceScope).toBe("global"); + expect(global.groomedSearchCodeQueries).toEqual(["missing symbol", "x".repeat(200)]); + + const paths = await buildGroomingFreshnessBaseline( + input({ + evidence: { ...evidence, sources: [{ path: "src/a.ts", provenance: "repository", via: "read", ref: "sha-1" }] }, + explorationToolCalls: [{ name: "search_code", ok: false, bytes: 0, arguments: { query: "missing symbol" } }], + citations: [{ id: "repo:src/a.ts", subject: "repository", state: null }], + }), + ); + expect(paths.groomedEvidenceScope).toBe("paths"); + expect(paths.groomedSearchCodeQueries).toEqual([]); + }); + + it("does not save queries for a global scope caused by a surfaced path", async () => { const baseline = await buildGroomingFreshnessBaseline( - input({ explorationToolCalls: [{ name: "search_code", ok: true, bytes: 0 }] }), + input({ + citations: [{ id: "repo:src/hit.ts", subject: "repository", state: null }], + explorationToolCalls: [{ name: "search_code", ok: true, bytes: 0, arguments: { query: "missing symbol" } }], + }), ); expect(baseline.groomedEvidenceScope).toBe("global"); + expect(baseline.groomedSearchCodeQueries).toEqual([]); + }); + + it("bounds saved empty-search queries to twenty", async () => { + const toolCalls = Array.from({ length: 25 }, (_, index) => ({ + name: "search_code", + ok: true, + bytes: 0, + arguments: { query: `missing ${index}` }, + })); + const baseline = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: toolCalls })); + expect(baseline.groomedSearchCodeQueries).toHaveLength(20); }); }); diff --git a/src/lib/groomer/freshness.ts b/src/lib/groomer/freshness.ts index 77195fd9..424b0b81 100644 --- a/src/lib/groomer/freshness.ts +++ b/src/lib/groomer/freshness.ts @@ -47,6 +47,9 @@ export const MAX_BASELINE_PATHS = 60; export const MAX_BASELINE_RELATED_WORK = 20; /** Dependency keys kept on the baseline. */ export const MAX_BASELINE_DEPENDENCIES = 20; +/** Empty code-search queries retained to recheck global evidence. */ +export const MAX_BASELINE_SEARCH_CODE_QUERIES = 20; +export const MAX_BASELINE_SEARCH_CODE_QUERY_CHARS = 200; /** * Statuses a worker owns. The freshness pass does not evaluate these (a claim @@ -95,6 +98,21 @@ export interface ExplorationToolCallLike { name: string; ok: boolean; bytes: number; + arguments?: Record; +} + +function emptySearchCodeQueries(toolCalls: ExplorationToolCallLike[]): string[] { + const queries: string[] = []; + for (const call of toolCalls) { + if (call.name !== "search_code" || !call.ok || call.bytes !== 0) continue; + const raw = call.arguments?.query; + if (typeof raw !== "string") continue; + const query = raw.trim().slice(0, MAX_BASELINE_SEARCH_CODE_QUERY_CHARS); + if (!query || queries.includes(query)) continue; + queries.push(query); + if (queries.length >= MAX_BASELINE_SEARCH_CODE_QUERIES) break; + } + return queries; } /** @@ -320,6 +338,7 @@ export interface GroomingFreshnessBaseline { groomedEvidenceCapturedAt: Date; groomedEvidenceScope: GroomingEvidenceScope; groomedEvidencePaths: string[]; + groomedSearchCodeQueries: string[]; groomedDependencyKeys: string[]; groomedOpenBlockerKeys: string[]; groomedRelatedWork: RelatedWorkBaselineEntry[]; @@ -340,6 +359,7 @@ export const UNKNOWN_FRESHNESS: Record = { groomedEvidenceCapturedAt: null, groomedEvidenceScope: null, groomedEvidencePaths: [], + groomedSearchCodeQueries: [], groomedDependencyKeys: [], groomedOpenBlockerKeys: [], groomedRelatedWork: null, @@ -407,6 +427,12 @@ export async function buildGroomingFreshnessBaseline(input: GroomingFreshnessInp groomedEvidenceCapturedAt: input.evidenceWindowStart, groomedEvidenceScope: scope, groomedEvidencePaths: reliance.repositoryPaths, + // Only negative-search globals may be rechecked later (#1091). The other + // global cases — a relied-on path that was only surfaced, or repository + // access with no read path — keep the conservative stale-on-commit + // behaviour even when an empty search also happened during the run. + groomedSearchCodeQueries: + scope === "global" && !reliance.reliesOnSurfacedPath ? emptySearchCodeQueries(input.explorationToolCalls) : [], groomedDependencyKeys: dependencyKeys, groomedOpenBlockerKeys: dependencyKeys.filter((key) => openKeys.has(key)).sort(), groomedRelatedWork: reliance.relatedWork, diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index 172730a1..6eeb080d 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -676,7 +676,7 @@ async function executeGroomerRun( plan, repositoryQueries: repositoryContext.queries, explorationRan: exploration !== null, - explorationToolCalls: exploration?.toolCalls ?? [], + explorationToolCalls: exploration?.toolCalls.map(({ name, arguments: args, ok, bytes }) => ({ name, arguments: args, ok, bytes })) ?? [], }, reader, ); @@ -913,7 +913,7 @@ async function executeGroomerRun( evidenceWindowStart, repositoryQueries: repositoryContext.queries, explorationRan: exploration !== null, - explorationToolCalls: exploration?.toolCalls ?? [], + explorationToolCalls: exploration?.toolCalls.map(({ name, arguments: args, ok, bytes }) => ({ name, arguments: args, ok, bytes })) ?? [], citations: plan.citations, }), ); From af2f3e4b8c0a955530f19299930e50f3dd845c9d Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Sun, 27 Sep 2026 14:22:36 -0600 Subject: [PATCH 2/4] fix(groomer): bound search recheck deferral and tighten query saving 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. --- .../migration.sql | 0 src/lib/github-ci.ts | 7 +- src/lib/github-code-search.test.ts | 9 +- src/lib/github-code-search.ts | 10 ++ .../groomer/freshness-invalidation.test.ts | 50 ++++++- src/lib/groomer/freshness-invalidation.ts | 128 +++++++++++------- src/lib/groomer/freshness.test.ts | 35 +++++ src/lib/groomer/freshness.ts | 21 ++- src/lib/groomer/run.test.ts | 19 +++ src/lib/groomer/run.ts | 5 +- 10 files changed, 224 insertions(+), 60 deletions(-) rename prisma/migrations/{20260928000000_add_issue_groomed_search_queries => 20261003000000_add_issue_groomed_search_queries}/migration.sql (100%) diff --git a/prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql b/prisma/migrations/20261003000000_add_issue_groomed_search_queries/migration.sql similarity index 100% rename from prisma/migrations/20260928000000_add_issue_groomed_search_queries/migration.sql rename to prisma/migrations/20261003000000_add_issue_groomed_search_queries/migration.sql diff --git a/src/lib/github-ci.ts b/src/lib/github-ci.ts index 7adeb2a1..b4554f67 100644 --- a/src/lib/github-ci.ts +++ b/src/lib/github-ci.ts @@ -164,6 +164,9 @@ export async function fetchLatestCommit(repoFullName: string, branch: string): P * Committer timestamp (ISO string) for an existing commit sha, or null when * the ref does not resolve. Used by the grooming freshness pass to judge * whether the code-search index has had time to catch up with a new head. + * GitHub always returns a committer date for a commit; the author date is + * deliberately not used as a fallback (it can make a rebased head look + * older than it is). */ export async function fetchCommitDate(repoFullName: string, ref: string): Promise { const response = await fetchWithRetry(`${GITHUB_API}/repos/${repoFullName}/commits/${encodeURIComponent(ref)}`, { @@ -174,8 +177,8 @@ export async function fetchCommitDate(repoFullName: string, ref: string): Promis const text = await response.text(); throw new Error(`Failed to fetch commit ${ref} for ${repoFullName}: ${response.status} ${text}`); } - const data = (await response.json()) as { commit?: { committer?: { date?: string }; author?: { date?: string } } }; - return data.commit?.committer?.date ?? data.commit?.author?.date ?? null; + const data = (await response.json()) as { commit?: { committer?: { date?: string } } }; + return data.commit?.committer?.date ?? null; } export function jobIdFromCheckRunUrl(url: string | undefined | null): string | null { diff --git a/src/lib/github-code-search.test.ts b/src/lib/github-code-search.test.ts index 9cb8b740..89a0a29b 100644 --- a/src/lib/github-code-search.test.ts +++ b/src/lib/github-code-search.test.ts @@ -306,10 +306,17 @@ describe("github-code-search: compareCommits (grooming freshness, #1064)", () => mockResponse({ status: "ahead", files: [{ filename: "src/new.ts", previous_filename: "src/old.ts" }, { filename: "docs/a.md" }], + commits: [{ commit: { committer: { date: "2026-09-28T00:00:00Z" } } }], }), ); const result = await compareCommits("org/repo", "base1", "head1"); - expect(result).toEqual({ ok: true, status: "ahead", files: ["src/new.ts", "src/old.ts", "docs/a.md"], truncated: false }); + expect(result).toEqual({ + ok: true, + status: "ahead", + files: ["src/new.ts", "src/old.ts", "docs/a.md"], + truncated: false, + firstCommitDate: "2026-09-28T00:00:00Z", + }); expect(fetchSpy).toHaveBeenCalledTimes(1); expect(String(fetchSpy.mock.calls[0][0])).toContain("/repos/org/repo/compare/base1...head1?per_page=1"); }); diff --git a/src/lib/github-code-search.ts b/src/lib/github-code-search.ts index 5c3b0729..c1f24f61 100644 --- a/src/lib/github-code-search.ts +++ b/src/lib/github-code-search.ts @@ -161,6 +161,12 @@ export type CommitComparison = files: string[]; /** True when GitHub's file cap was reached, so `files` may be incomplete. */ truncated: boolean; + /** + * Committer timestamp of the first (oldest) commit in base...head, when + * the response carried one. Lets callers bound how long a recheck may + * stay deferred on an unverified range (#1091). + */ + firstCommitDate?: string | null; } | { ok: false; @@ -196,6 +202,7 @@ export async function compareCommits( const data = (await response.json()) as { status?: string; files?: Array<{ filename?: string; previous_filename?: string }>; + commits?: Array<{ commit?: { committer?: { date?: string } } }>; }; const rawFiles = Array.isArray(data.files) ? data.files : []; const files = new Set(); @@ -203,11 +210,14 @@ export async function compareCommits( if (typeof file.filename === "string" && file.filename) files.add(file.filename); if (typeof file.previous_filename === "string" && file.previous_filename) files.add(file.previous_filename); } + // per_page=1 still returns the first (oldest) commit of the range. + const firstCommitDate = Array.isArray(data.commits) ? data.commits[0]?.commit?.committer?.date ?? null : null; return { ok: true, status: typeof data.status === "string" ? data.status : "unknown", files: [...files], truncated: rawFiles.length >= COMPARE_MAX_FILES, + firstCommitDate, }; } catch (err) { return { diff --git a/src/lib/groomer/freshness-invalidation.test.ts b/src/lib/groomer/freshness-invalidation.test.ts index 14c75557..be00efe2 100644 --- a/src/lib/groomer/freshness-invalidation.test.ts +++ b/src/lib/groomer/freshness-invalidation.test.ts @@ -296,19 +296,33 @@ describe("runGroomingFreshnessPass", () => { expect(store.stale.get("issue-1")?.detail).toContain("specific missing query"); }); - it("falls back to global staleness when the search budget is exhausted", async () => { + it("defers instead of staling when the search budget runs out mid-recheck", async () => { github.fetchHeadSha.mockResolvedValue("sha-2"); - github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 5 * 60_000).toISOString(), + }); const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["first", "second"] }]); await pass(store, github, { ...DEFAULT_FRESHNESS_BUDGET, maxSearchCodeRechecks: 2 }); - expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); - // One budget unit goes to the commit-date check, one to the first query. + // One budget unit goes to the commit-date check, one to the first query; + // the second query exhausts the budget, which defers rather than stales. + expect(store.stale.size).toBe(0); + expect(store.advanced).toEqual([]); expect(github.searchCode).toHaveBeenCalledTimes(1); }); it("defers the recheck while the new head is younger than the index grace window", async () => { github.fetchHeadSha.mockResolvedValue("sha-2"); - github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 5 * 60_000).toISOString(), + }); github.fetchCommitDate.mockResolvedValue(new Date(Date.now() - 60_000).toISOString()); const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }]); const result = await pass(store, github); @@ -318,6 +332,32 @@ describe("runGroomingFreshnessPass", () => { expect(result.deferred).toBe(1); }); + it("bounds the deferral: stales once the oldest unverified commit outlives the defer limit", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 3 * 3_600_000).toISOString(), + }); + github.fetchCommitDate.mockResolvedValue(new Date(Date.now() - 60_000).toISOString()); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }]); + const result = await pass(store, github); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(store.advanced).toEqual([]); + expect(result.deferred).toBe(0); + }); + + it("stales instead of deferring when the oldest unverified commit age is unknown", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); + github.fetchCommitDate.mockResolvedValue(new Date(Date.now() - 60_000).toISOString()); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }]); + await pass(store, github); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + }); + it("falls back to global staleness when the commit date is missing or unavailable", async () => { github.fetchHeadSha.mockResolvedValue("sha-2"); github.compareCommits.mockResolvedValue({ ok: true, status: "ahead", files: [], truncated: false }); diff --git a/src/lib/groomer/freshness-invalidation.ts b/src/lib/groomer/freshness-invalidation.ts index a6fd6351..b40ddcfd 100644 --- a/src/lib/groomer/freshness-invalidation.ts +++ b/src/lib/groomer/freshness-invalidation.ts @@ -125,6 +125,14 @@ export const DEFAULT_FRESHNESS_BUDGET: FreshnessBudget = { */ export const SEARCH_RECHECK_INDEX_GRACE_MS = 30 * 60 * 1000; +/** + * How long a recheck may stay deferred because the code-search index has not + * demonstrably caught up. Once the oldest unverified commit in base...head is + * older than this, the deferral is bounded and the result goes stale + * conservatively instead of starving forever in a busy repo (#1091 review). + */ +export const SEARCH_RECHECK_DEFER_LIMIT_MS = 2 * 60 * 60 * 1000; + export interface FreshnessPassResult { issuesChecked: number; markedStale: Array<{ repo: string; issueNumber: number; reasons: GroomingStaleReason[] }>; @@ -453,6 +461,27 @@ async function applyComparison( for (const evaluation of members) stale(evaluation, "compare_unreliable", `${range}: ${why}`); return; } + // The head commit timestamp is shared by every member of the comparison + // (they compare against the same head), so it is fetched at most once and + // costs one budget unit per comparison, not per issue (#1091 review). + let headCommittedAt: number | null = null; + let headDateResolved = false; + const resolveHeadDate = async (fetchCommitDate: NonNullable): Promise => { + if (headDateResolved) return headCommittedAt; + headDateResolved = true; + if (remaining.searchCode <= 0) return null; + remaining.searchCode--; + result.githubCalls++; + try { + const date = await fetchCommitDate(repoFullName, head); + const parsed = date ? Date.parse(date) : Number.NaN; + headCommittedAt = Number.isNaN(parsed) ? null : parsed; + } catch { + headCommittedAt = null; + } + return headCommittedAt; + }; + for (const evaluation of members) { if (evaluation.issue.groomedEvidenceScope === "global") { const queries = evaluation.issue.groomedSearchCodeQueries ?? []; @@ -466,59 +495,60 @@ async function applyComparison( continue; } if (queries.length > 0 && github.searchCode && github.fetchCommitDate) { - if (remaining.searchCode <= 0) { - stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); - continue; - } - remaining.searchCode--; - result.githubCalls++; - let committedAt: string | null = null; - try { - committedAt = await github.fetchCommitDate(repoFullName, head); - } catch { - committedAt = null; - } - const at = committedAt ? Date.parse(committedAt) : Number.NaN; - if (Number.isNaN(at)) { - // No trustworthy timestamp: cannot confirm the index caught up, so - // stay conservative (stale), never fresh-and-verified. - stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); - continue; - } - if (now().getTime() - at < SEARCH_RECHECK_INDEX_GRACE_MS) { - // The head is too recent for the code-search index to have caught - // up; an empty recheck now would not mean "still absent". Defer to - // a later pass without advancing or staling. - evaluation.deferred = true; - continue; - } - let matchedQuery: string | null = null; - let allEmpty = true; - for (const query of queries) { - if (remaining.searchCode <= 0) { - allEmpty = false; - break; + const fetchCommitDate = github.fetchCommitDate; + const headAt = await resolveHeadDate(fetchCommitDate); + const headAge = headAt === null ? Number.NaN : now().getTime() - headAt; + if (!Number.isNaN(headAge)) { + if (headAge < SEARCH_RECHECK_INDEX_GRACE_MS) { + // The head is too recent for the code-search index to have caught + // up; an empty recheck now would not mean "still absent". Defer, + // but only while the oldest unverified commit is young enough: + // code search indexes the branch as a whole, so once that commit + // has waited past the limit the index cannot be trusted and the + // result goes stale instead of starving forever (#1091 review). + const firstAt = comparison.firstCommitDate ? Date.parse(comparison.firstCommitDate) : Number.NaN; + const firstAge = Number.isNaN(firstAt) ? Number.POSITIVE_INFINITY : now().getTime() - firstAt; + if (Number.isNaN(firstAt) || firstAge > SEARCH_RECHECK_DEFER_LIMIT_MS) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); + continue; + } + evaluation.deferred = true; + continue; } - remaining.searchCode--; - result.githubCalls++; - try { - if ((await github.searchCode(repoFullName, query, 1)).length > 0) { - matchedQuery = query; - allEmpty = false; + let matchedQuery: string | null = null; + let exhausted = false; + let failed = false; + for (const query of queries) { + if (remaining.searchCode <= 0) { + exhausted = true; + break; + } + remaining.searchCode--; + result.githubCalls++; + try { + if ((await github.searchCode(repoFullName, query, 1)).length > 0) { + matchedQuery = query; + break; + } + } catch { + failed = true; break; } - } catch { - allEmpty = false; - break; } - } - if (matchedQuery) { - stale(evaluation, "global_evidence_commit", `default branch moved ${range}; previously empty search query now matches: ${matchedQuery}`); - continue; - } - if (allEmpty) { - evaluation.advance.groomingVerifiedSha = head; - continue; + if (matchedQuery) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range}; previously empty search query now matches: ${matchedQuery}`); + continue; + } + if (!failed) { + if (exhausted) { + // Out of budget, not evidence of change: leave unverified for a + // later pass rather than forcing a re-groom (#1091 review). + evaluation.deferred = true; + continue; + } + evaluation.advance.groomingVerifiedSha = head; + continue; + } } } stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); diff --git a/src/lib/groomer/freshness.test.ts b/src/lib/groomer/freshness.test.ts index 0861ccef..8f3dbe67 100644 --- a/src/lib/groomer/freshness.test.ts +++ b/src/lib/groomer/freshness.test.ts @@ -6,6 +6,7 @@ import { deriveEvidenceScope, deriveGroomingFreshness, dependencyKeysForIssue, + explorationCallsForFreshness, hasNegativeSearchResult, intersectEvidencePaths, isFreshnessTrackedStatus, @@ -293,6 +294,40 @@ describe("buildGroomingFreshnessBaseline", () => { expect(baseline.groomedSearchCodeQueries).toEqual([]); }); + it("does not save queries for a no-read-path global when repository-context queries ran", async () => { + const baseline = await buildGroomingFreshnessBaseline( + input({ + evidence: { ...evidence, sources: [] }, + repositoryQueries: ["sslmode"], + explorationToolCalls: [{ name: "search_code", ok: true, bytes: 0, arguments: { query: "missing symbol" } }], + }), + ); + expect(baseline.groomedEvidenceScope).toBe("global"); + expect(baseline.groomedSearchCodeQueries).toEqual([]); + }); + + it("does not save queries for a no-read-path global when list_directory surfaced evidence", async () => { + const baseline = await buildGroomingFreshnessBaseline( + input({ + evidence: { ...evidence, sources: [] }, + explorationToolCalls: [ + { name: "list_directory", ok: true, bytes: 50, arguments: { path: "src" } }, + { name: "search_code", ok: true, bytes: 0, arguments: { query: "missing symbol" } }, + ], + }), + ); + expect(baseline.groomedEvidenceScope).toBe("global"); + expect(baseline.groomedSearchCodeQueries).toEqual([]); + }); + + it("explorationCallsForFreshness keeps arguments and drops other fields", () => { + expect( + explorationCallsForFreshness([ + { name: "search_code", arguments: { query: "q" }, ok: true, bytes: 0, preview: "No matches" }, + ]), + ).toEqual([{ name: "search_code", arguments: { query: "q" }, ok: true, bytes: 0 }]); + }); + it("bounds saved empty-search queries to twenty", async () => { const toolCalls = Array.from({ length: 25 }, (_, index) => ({ name: "search_code", diff --git a/src/lib/groomer/freshness.ts b/src/lib/groomer/freshness.ts index 424b0b81..6e43eae6 100644 --- a/src/lib/groomer/freshness.ts +++ b/src/lib/groomer/freshness.ts @@ -101,6 +101,16 @@ export interface ExplorationToolCallLike { arguments?: Record; } +/** + * Map exploration tool records to the freshness input shape, keeping the + * call arguments so saved empty search queries can be recovered later. + */ +export function explorationCallsForFreshness( + toolCalls: Array<{ name: string; arguments: Record; ok: boolean; bytes: number; preview?: string }>, +): ExplorationToolCallLike[] { + return toolCalls.map(({ name, arguments: args, ok, bytes }) => ({ name, arguments: args, ok, bytes })); +} + function emptySearchCodeQueries(toolCalls: ExplorationToolCallLike[]): string[] { const queries: string[] = []; for (const call of toolCalls) { @@ -431,8 +441,17 @@ export async function buildGroomingFreshnessBaseline(input: GroomingFreshnessInp // global cases — a relied-on path that was only surfaced, or repository // access with no read path — keep the conservative stale-on-commit // behaviour even when an empty search also happened during the run. + // A no-read-path global may still save its queries when exploration + // searches were its ONLY repository evidence: no repository-context + // queries ran and nothing else (e.g. list_directory) surfaced paths. groomedSearchCodeQueries: - scope === "global" && !reliance.reliesOnSurfacedPath ? emptySearchCodeQueries(input.explorationToolCalls) : [], + scope === "global" && + !reliance.reliesOnSurfacedPath && + (reliance.repositoryPaths.length > 0 || + (input.repositoryQueries.length === 0 && + !input.explorationToolCalls.some((call) => call.name === "list_directory"))) + ? emptySearchCodeQueries(input.explorationToolCalls) + : [], groomedDependencyKeys: dependencyKeys, groomedOpenBlockerKeys: dependencyKeys.filter((key) => openKeys.has(key)).sort(), groomedRelatedWork: reliance.relatedWork, diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index 4258a731..fd885878 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -1632,6 +1632,25 @@ Investigate session handling in auth module.`; expect(issueUpdateData()).toMatchObject({ groomedEvidenceScope: "global", groomedEvidencePaths: [] }); }); + it("saves empty exploration search queries in the freshness baseline (#1091)", async () => { + mocks.getHostedGroomerConfig.mockReturnValue({ ...mockConfig, toolLoopEnabled: true }); + mocks.exploreRepository.mockResolvedValue({ + ...mockExploration, + sources: [], + readSources: [], + toolCalls: [ + { name: "search_code", arguments: { query: "missing symbol" }, ok: true, bytes: 0, preview: "No matches" }, + { name: "search_code", arguments: { query: "also missing" }, ok: true, bytes: 0, preview: "No matches" }, + { name: "search_code", arguments: { query: "found it" }, ok: true, bytes: 400, preview: "src/a.ts" }, + ], + }); + await runHostedGroomer(); + expect(issueUpdateData()).toMatchObject({ + groomedEvidenceScope: "global", + groomedSearchCodeQueries: ["missing symbol", "also missing"], + }); + }); + it("does not record a baseline for a skipped in-flight run, leaving the prior one untouched", async () => { mocks.selectGroomingCandidate.mockResolvedValue({ ...mockCandidate, labels: ["status/in-progress", "priority/p0"] }); await runHostedGroomer(); diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index 6eeb080d..6d34ecaa 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -20,6 +20,7 @@ import { } from "./evidence-snapshot"; import type { RepositoryContextInput, RepositoryContextConfig } from "./repository-context"; import { createGroomingRunRecord, completeGroomingRunRecord, updateGroomingRunRecord } from "./history"; +import { explorationCallsForFreshness } from "./freshness"; import { freshnessBaselineIssueData } from "./freshness-invalidation"; import { compareCommits } from "@/lib/github-code-search"; import { validateApplyPreconditions, type LiveComment, type PreconditionReader } from "./mutation-validator"; @@ -676,7 +677,7 @@ async function executeGroomerRun( plan, repositoryQueries: repositoryContext.queries, explorationRan: exploration !== null, - explorationToolCalls: exploration?.toolCalls.map(({ name, arguments: args, ok, bytes }) => ({ name, arguments: args, ok, bytes })) ?? [], + explorationToolCalls: exploration ? explorationCallsForFreshness(exploration.toolCalls) : [], }, reader, ); @@ -913,7 +914,7 @@ async function executeGroomerRun( evidenceWindowStart, repositoryQueries: repositoryContext.queries, explorationRan: exploration !== null, - explorationToolCalls: exploration?.toolCalls.map(({ name, arguments: args, ok, bytes }) => ({ name, arguments: args, ok, bytes })) ?? [], + explorationToolCalls: exploration ? explorationCallsForFreshness(exploration.toolCalls) : [], citations: plan.citations, }), ); From e244bcb83d1afb2a03954b30c3caa0e248ed0bcb Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Sun, 27 Sep 2026 14:30:25 -0600 Subject: [PATCH 3/4] fix(groomer): make search-recheck budget exhaustion bounded and finishable 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. --- .../groomer/freshness-invalidation.test.ts | 51 ++++++++++ src/lib/groomer/freshness-invalidation.ts | 98 ++++++++++++------- src/lib/groomer/freshness.test.ts | 4 +- src/lib/groomer/freshness.ts | 9 +- 4 files changed, 120 insertions(+), 42 deletions(-) diff --git a/src/lib/groomer/freshness-invalidation.test.ts b/src/lib/groomer/freshness-invalidation.test.ts index be00efe2..9f5076b0 100644 --- a/src/lib/groomer/freshness-invalidation.test.ts +++ b/src/lib/groomer/freshness-invalidation.test.ts @@ -314,6 +314,57 @@ describe("runGroomingFreshnessPass", () => { expect(github.searchCode).toHaveBeenCalledTimes(1); }); + it("stales a persistently exhausted issue once the oldest unverified commit passes the defer limit", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 3 * 3_600_000).toISOString(), + }); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["first", "second"] }]); + await pass(store, github, { ...DEFAULT_FRESHNESS_BUDGET, maxSearchCodeRechecks: 2 }); + expect(store.stale.get("issue-1")?.reasons).toEqual(["global_evidence_commit"]); + expect(store.advanced).toEqual([]); + }); + + it("defers within the bound when the budget is exhausted before the date fetch", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 5 * 60_000).toISOString(), + }); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: ["query"] }]); + const result = await pass(store, github, { ...DEFAULT_FRESHNESS_BUDGET, maxSearchCodeRechecks: 0 }); + expect(store.stale.size).toBe(0); + expect(store.advanced).toEqual([]); + expect(github.searchCode).not.toHaveBeenCalled(); + expect(result.deferred).toBe(1); + }); + + it("completes the recheck for an issue with the maximum saved queries", async () => { + github.fetchHeadSha.mockResolvedValue("sha-2"); + github.compareCommits.mockResolvedValue({ + ok: true, + status: "ahead", + files: [], + truncated: false, + firstCommitDate: new Date(Date.now() - 5 * 60_000).toISOString(), + }); + const queries = Array.from({ length: 10 }, (_, index) => `missing ${index}`); + const store = fakeStore([{ ...row(), groomedEvidenceScope: "global", groomedEvidencePaths: [], groomedSearchCodeQueries: queries }]); + await pass(store, github); + // Commit-date fetch (1) plus all ten queries fit the default budget. + expect(store.stale.size).toBe(0); + expect(store.advanced).toEqual([{ id: "issue-1", data: { groomingVerifiedSha: "sha-2" } }]); + expect(github.searchCode).toHaveBeenCalledTimes(10); + expect(github.fetchCommitDate).toHaveBeenCalledTimes(1); + }); + it("defers the recheck while the new head is younger than the index grace window", async () => { github.fetchHeadSha.mockResolvedValue("sha-2"); github.compareCommits.mockResolvedValue({ diff --git a/src/lib/groomer/freshness-invalidation.ts b/src/lib/groomer/freshness-invalidation.ts index b40ddcfd..4103b9af 100644 --- a/src/lib/groomer/freshness-invalidation.ts +++ b/src/lib/groomer/freshness-invalidation.ts @@ -464,22 +464,49 @@ async function applyComparison( // The head commit timestamp is shared by every member of the comparison // (they compare against the same head), so it is fetched at most once and // costs one budget unit per comparison, not per issue (#1091 review). + // "exhausted" (budget ran out) defers within the defer limit like every + // other unverified case; "failed" (fetch errored / unusable date) is + // conservative and stales. let headCommittedAt: number | null = null; - let headDateResolved = false; - const resolveHeadDate = async (fetchCommitDate: NonNullable): Promise => { - if (headDateResolved) return headCommittedAt; - headDateResolved = true; - if (remaining.searchCode <= 0) return null; - remaining.searchCode--; - result.githubCalls++; - try { - const date = await fetchCommitDate(repoFullName, head); - const parsed = date ? Date.parse(date) : Number.NaN; - headCommittedAt = Number.isNaN(parsed) ? null : parsed; - } catch { - headCommittedAt = null; + let headDateState: "unresolved" | "ok" | "failed" | "exhausted" = "unresolved"; + const resolveHeadDate = async ( + fetchCommitDate: NonNullable, + ): Promise<{ at: number | null; state: "ok" | "failed" | "exhausted" }> => { + if (headDateState === "unresolved") { + if (remaining.searchCode <= 0) { + headDateState = "exhausted"; + } else { + remaining.searchCode--; + result.githubCalls++; + try { + const date = await fetchCommitDate(repoFullName, head); + const parsed = date ? Date.parse(date) : Number.NaN; + if (Number.isNaN(parsed)) { + headDateState = "failed"; + } else { + headCommittedAt = parsed; + headDateState = "ok"; + } + } catch { + headDateState = "failed"; + } + } + } + return { at: headCommittedAt, state: headDateState }; + }; + + // A deferral is only safe while it is bounded: once the oldest unverified + // commit has waited past the limit, the code-search index cannot be + // trusted to ever confirm "still absent" for this range, so the result + // goes stale conservatively instead of starving forever (#1091 review). + const deferWithinBound = (evaluation: Evaluation): void => { + const firstAt = comparison.firstCommitDate ? Date.parse(comparison.firstCommitDate) : Number.NaN; + const firstAge = Number.isNaN(firstAt) ? Number.POSITIVE_INFINITY : now().getTime() - firstAt; + if (Number.isNaN(firstAt) || firstAge > SEARCH_RECHECK_DEFER_LIMIT_MS) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); + return; } - return headCommittedAt; + evaluation.deferred = true; }; for (const evaluation of members) { @@ -496,23 +523,12 @@ async function applyComparison( } if (queries.length > 0 && github.searchCode && github.fetchCommitDate) { const fetchCommitDate = github.fetchCommitDate; - const headAt = await resolveHeadDate(fetchCommitDate); - const headAge = headAt === null ? Number.NaN : now().getTime() - headAt; - if (!Number.isNaN(headAge)) { - if (headAge < SEARCH_RECHECK_INDEX_GRACE_MS) { + const resolved = await resolveHeadDate(fetchCommitDate); + if (resolved.state === "ok" && resolved.at !== null) { + if (now().getTime() - resolved.at < SEARCH_RECHECK_INDEX_GRACE_MS) { // The head is too recent for the code-search index to have caught - // up; an empty recheck now would not mean "still absent". Defer, - // but only while the oldest unverified commit is young enough: - // code search indexes the branch as a whole, so once that commit - // has waited past the limit the index cannot be trusted and the - // result goes stale instead of starving forever (#1091 review). - const firstAt = comparison.firstCommitDate ? Date.parse(comparison.firstCommitDate) : Number.NaN; - const firstAge = Number.isNaN(firstAt) ? Number.POSITIVE_INFINITY : now().getTime() - firstAt; - if (Number.isNaN(firstAt) || firstAge > SEARCH_RECHECK_DEFER_LIMIT_MS) { - stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); - continue; - } - evaluation.deferred = true; + // up; an empty recheck now would not mean "still absent". + deferWithinBound(evaluation); continue; } let matchedQuery: string | null = null; @@ -539,16 +555,22 @@ async function applyComparison( stale(evaluation, "global_evidence_commit", `default branch moved ${range}; previously empty search query now matches: ${matchedQuery}`); continue; } - if (!failed) { - if (exhausted) { - // Out of budget, not evidence of change: leave unverified for a - // later pass rather than forcing a re-groom (#1091 review). - evaluation.deferred = true; - continue; - } - evaluation.advance.groomingVerifiedSha = head; + if (failed) { + stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); + continue; + } + if (exhausted) { + // Out of budget, not evidence of change: leave unverified for a + // later pass, bounded by the defer limit like the young-head case. + deferWithinBound(evaluation); continue; } + evaluation.advance.groomingVerifiedSha = head; + continue; + } + if (resolved.state === "exhausted") { + deferWithinBound(evaluation); + continue; } } stale(evaluation, "global_evidence_commit", `default branch moved ${range} and the result relied on repo-wide evidence`); diff --git a/src/lib/groomer/freshness.test.ts b/src/lib/groomer/freshness.test.ts index 8f3dbe67..8ac6b661 100644 --- a/src/lib/groomer/freshness.test.ts +++ b/src/lib/groomer/freshness.test.ts @@ -328,7 +328,7 @@ describe("buildGroomingFreshnessBaseline", () => { ).toEqual([{ name: "search_code", arguments: { query: "q" }, ok: true, bytes: 0 }]); }); - it("bounds saved empty-search queries to twenty", async () => { + it("bounds saved empty-search queries to ten, below the pass search budget", async () => { const toolCalls = Array.from({ length: 25 }, (_, index) => ({ name: "search_code", ok: true, @@ -336,7 +336,7 @@ describe("buildGroomingFreshnessBaseline", () => { arguments: { query: `missing ${index}` }, })); const baseline = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: toolCalls })); - expect(baseline.groomedSearchCodeQueries).toHaveLength(20); + expect(baseline.groomedSearchCodeQueries).toHaveLength(10); }); }); diff --git a/src/lib/groomer/freshness.ts b/src/lib/groomer/freshness.ts index 6e43eae6..9a069a71 100644 --- a/src/lib/groomer/freshness.ts +++ b/src/lib/groomer/freshness.ts @@ -47,8 +47,13 @@ export const MAX_BASELINE_PATHS = 60; export const MAX_BASELINE_RELATED_WORK = 20; /** Dependency keys kept on the baseline. */ export const MAX_BASELINE_DEPENDENCIES = 20; -/** Empty code-search queries retained to recheck global evidence. */ -export const MAX_BASELINE_SEARCH_CODE_QUERIES = 20; +/** + * Empty code-search queries retained to recheck global evidence. Capped well + * below the freshness pass's search budget (20) so a single issue can always + * complete its recheck — commit-date fetch plus every saved query — even + * when it is first in the pass (#1091 review). + */ +export const MAX_BASELINE_SEARCH_CODE_QUERIES = 10; export const MAX_BASELINE_SEARCH_CODE_QUERY_CHARS = 200; /** From 3893e3ae452725dd468a8bf8d294bb0d259a7e3b Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Sun, 27 Sep 2026 14:49:56 -0600 Subject: [PATCH 4/4] fix(groomer): fail incomplete empty code searches and save empty queries all-or-nothing --- src/lib/github-code-search.test.ts | 8 +++++++ src/lib/github-code-search.ts | 18 ++++++++++----- src/lib/groomer/freshness.test.ts | 36 ++++++++++++++++++++---------- src/lib/groomer/freshness.ts | 13 +++++++---- 4 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/lib/github-code-search.test.ts b/src/lib/github-code-search.test.ts index 89a0a29b..30e3681a 100644 --- a/src/lib/github-code-search.test.ts +++ b/src/lib/github-code-search.test.ts @@ -137,6 +137,14 @@ describe("github-code-search: searchRepositoryCode (the groomer repo-exploration expect(fetchSpy).toHaveBeenCalledTimes(1); }); + it("fails an incomplete search with no items instead of reporting no matches", async () => { + fetchSpy.mockResolvedValueOnce(mockResponse({ total_count: 0, incomplete_results: true, items: [] })); + + await expect(searchRepositoryCode("org/repo", "prisma", 5)).rejects.toThrow( + /^Code search failed for org\/repo: search timed out with incomplete results and no matches$/, + ); + }); + // (c) HTTP 429 retried with backoff. it("retries a 429 rate-limit response with backoff and succeeds on the next attempt", async () => { fetchSpy diff --git a/src/lib/github-code-search.ts b/src/lib/github-code-search.ts index c1f24f61..e1a0fe8c 100644 --- a/src/lib/github-code-search.ts +++ b/src/lib/github-code-search.ts @@ -57,11 +57,19 @@ export async function searchRepositoryCode( // so a search with more matches than fit on one page is not silently // truncated at the first page. Each page is fetched through fetchWithRetry, // so transient 429/5xx responses are retried here as well. - const items = await fetchPaginated<{ path?: string; html_url?: string }>( - url, - limit, - (data) => (data as { items?: { path?: string; html_url?: string }[] }).items ?? [], - ); + let incomplete = false; + const items = await fetchPaginated<{ path?: string; html_url?: string }>(url, limit, (data) => { + const page = data as { items?: { path?: string; html_url?: string }[]; incomplete_results?: boolean }; + if (page.incomplete_results === true) incomplete = true; + return page.items ?? []; + }); + // 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. + if (incomplete && items.length === 0) { + throw new Error("search timed out with incomplete results and no matches"); + } return items.map((item) => ({ path: item.path ?? "", url: item.html_url ?? "", diff --git a/src/lib/groomer/freshness.test.ts b/src/lib/groomer/freshness.test.ts index 8ac6b661..460d3412 100644 --- a/src/lib/groomer/freshness.test.ts +++ b/src/lib/groomer/freshness.test.ts @@ -260,12 +260,11 @@ describe("buildGroomingFreshnessBaseline", () => { expect(baseline.groomedEvidencePaths).toEqual([]); }); - it("records bounded, deduplicated empty-search queries only for global evidence", async () => { + it("records trimmed, deduplicated empty-search queries only for global evidence", async () => { const calls = [ { name: "search_code", ok: true, bytes: 0, arguments: { query: " missing symbol " } }, { name: "search_code", ok: true, bytes: 0, arguments: { query: "missing symbol" } }, - { name: "search_code", ok: true, bytes: 0, arguments: { query: "x".repeat(250) } }, - { name: "search_code", ok: true, bytes: 0, arguments: { query: " " } }, + { name: "search_code", ok: true, bytes: 0, arguments: { query: "x".repeat(200) } }, { name: "search_code", ok: false, bytes: 0, arguments: { query: "failed" } }, ]; const global = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: calls })); @@ -328,15 +327,28 @@ describe("buildGroomingFreshnessBaseline", () => { ).toEqual([{ name: "search_code", arguments: { query: "q" }, ok: true, bytes: 0 }]); }); - it("bounds saved empty-search queries to ten, below the pass search budget", async () => { - const toolCalls = Array.from({ length: 25 }, (_, index) => ({ - name: "search_code", - ok: true, - bytes: 0, - arguments: { query: `missing ${index}` }, - })); - const baseline = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: toolCalls })); - expect(baseline.groomedSearchCodeQueries).toHaveLength(10); + const emptySearches = (queries: unknown[]) => + queries.map((query) => ({ name: "search_code", ok: true, bytes: 0, arguments: { query } })); + + it("saves up to ten empty-search queries, below the pass search budget", async () => { + const queries = Array.from({ length: 10 }, (_, index) => `missing ${index}`); + const baseline = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: emptySearches(queries) })); + expect(baseline.groomedSearchCodeQueries).toEqual(queries); + }); + + it("saves none when the empty searches exceed the cap, so a subset is never rechecked", async () => { + const queries = Array.from({ length: 11 }, (_, index) => `missing ${index}`); + const baseline = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: emptySearches(queries) })); + expect(baseline.groomedSearchCodeQueries).toEqual([]); + }); + + it("saves none when an empty search can't be saved whole", async () => { + const tooLong = await buildGroomingFreshnessBaseline( + input({ explorationToolCalls: emptySearches(["short", "x".repeat(201)]) }), + ); + expect(tooLong.groomedSearchCodeQueries).toEqual([]); + const unreadable = await buildGroomingFreshnessBaseline(input({ explorationToolCalls: emptySearches(["short", 42]) })); + expect(unreadable.groomedSearchCodeQueries).toEqual([]); }); }); diff --git a/src/lib/groomer/freshness.ts b/src/lib/groomer/freshness.ts index 9a069a71..ae911b37 100644 --- a/src/lib/groomer/freshness.ts +++ b/src/lib/groomer/freshness.ts @@ -120,12 +120,17 @@ function emptySearchCodeQueries(toolCalls: ExplorationToolCallLike[]): string[] const queries: string[] = []; 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 + // recheck verifying the result on a subset, so save none and keep the + // conservative stale-on-commit behaviour instead. const raw = call.arguments?.query; - if (typeof raw !== "string") continue; - const query = raw.trim().slice(0, MAX_BASELINE_SEARCH_CODE_QUERY_CHARS); - if (!query || queries.includes(query)) continue; + if (typeof raw !== "string") return []; + const query = raw.trim(); + if (!query) return []; + if (queries.includes(query)) continue; + if (query.length > MAX_BASELINE_SEARCH_CODE_QUERY_CHARS || queries.length >= MAX_BASELINE_SEARCH_CODE_QUERIES) return []; queries.push(query); - if (queries.length >= MAX_BASELINE_SEARCH_CODE_QUERIES) break; } return queries; }