From 47344b6b19e1afddf6eaafb83a00862d8a2720fd Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:28:32 +0900 Subject: [PATCH] fix(api): skip the shared rate-limit budget when chat Q&A is off for the repo The chat-qa route counted a prior invocation and recorded a COMMAND_RATE_LIMIT_EVENT_TYPE audit row (the SAME counter the @loopover chat PR-comment command uses), then built a planNextWork grounding bundle, before calling generateChatQaAnswer -- which, on a repo where chat Q&A is not enabled, is guaranteed to return `disabled`. So every request to a chat-disabled repo spent a slot in the shared budget and paid for the grounding bundle for an answer that never came, and could drive the maintainer's genuine @loopover chat usage on that PR to its limit. Gate the route on isRepoChatQaEnabled(settings) -- the same predicate the maintainer dashboard's capability map already reads, so the two can never disagree -- immediately after resolving settings. When it is false, return generateChatQaAnswer's own disabled result (its bundle is unused on that path) before the rate-limit block and before planNextWork. The enabled path -- counting, the audit row, planNextWork, and the rate_limited response -- is unchanged, and ai-chat-qa.ts is not touched. Adds tests asserting a disabled repo writes no audit row and never calls planNextWork, while the existing enabled-path rate-limit/audit/rate_limited tests continue to pass. One existing enabled-path test that had never stubbed the chatQa-enabling manifest (and only passed because the route used to run unconditionally) now enables it explicitly. Closes #9714 --- src/api/routes.ts | 19 ++++++++++++++++ test/unit/maintainer-chat-qa.test.ts | 34 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/api/routes.ts b/src/api/routes.ts index 9d2c71494a..4b2a4cc1f2 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -3537,6 +3537,25 @@ export function createApp() { const [settings, pullRequest] = await Promise.all([resolveRepositorySettings(c.env, fullName), getPullRequest(c.env, fullName, number)]); if (!pullRequest) return c.json({ error: "pull_request_not_found" }, 404); + // When chat Q&A is disabled for the repo, generateChatQaAnswer is guaranteed to return `disabled` -- but only + // after this route has already spent a slot in the shared @loopover-chat rate-limit budget AND paid for the + // planNextWork grounding bundle (#9714). Gate on the SAME predicate the maintainer dashboard reads + // (isRepoChatQaEnabled at the capability map above), so the two can never disagree, and return + // generateChatQaAnswer's own disabled result (bundle unused on that path) rather than a second copy of it. + if (!isRepoChatQaEnabled(settings)) { + return c.json( + await generateChatQaAnswer(c.env, { + bundle: null, + question: parsed.data.question, + advisoryAiRouting: settings.advisoryAiRouting, + repoFullName: fullName, + issueNumber: number, + actor: resolveChatQaActor(gate.identity), + route: "app.maintainer_dashboard.chat_qa", + }), + ); + } + const actor = resolveChatQaActor(gate.identity); const targetKey = `${fullName}#${number}#chat`; const { policy, maxPerWindow, windowHours } = resolveChatQaRateLimit(settings); diff --git a/test/unit/maintainer-chat-qa.test.ts b/test/unit/maintainer-chat-qa.test.ts index 67789d9e1c..c4bbf68d2d 100644 --- a/test/unit/maintainer-chat-qa.test.ts +++ b/test/unit/maintainer-chat-qa.test.ts @@ -104,6 +104,37 @@ describe("POST /v1/repos/:owner/:repo/pulls/:number/chat-qa (#6489)", () => { await expect(res.json()).resolves.toEqual({ status: "disabled", reason: "Chat Q&A is not enabled on this instance (settings.advisoryAiRouting.chatQa is off)." }); }); + it("#9714: a disabled repo does NOT record a rate-limit audit row (it must not spend the shared @loopover-chat budget)", async () => { + const env = createTestEnv(); + await seedRepoWithPull(env); + // chatQa is off (no .loopover.yml stub). The request still returns disabled, but must not have consumed a slot. + const res = await app.request("/v1/repos/owner/repo/pulls/11/chat-qa", { method: "POST", headers: apiHeaders(env), body: JSON.stringify({ question: "why is this blocked?" }) }, env); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toMatchObject({ status: "disabled" }); + + const invocationRows = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.command_invocation' and target_key = 'owner/repo#11#chat'").first<{ n: number }>(); + expect(invocationRows?.n).toBe(0); + }); + + it("#9714: a disabled repo does NOT call planNextWork (it must not pay for the grounding bundle)", async () => { + vi.resetModules(); + const planNextWork = vi.fn(); + vi.doMock("../../src/services/agent-orchestrator", async () => { + const actual = await vi.importActual("../../src/services/agent-orchestrator"); + return { ...actual, planNextWork }; + }); + const { createApp: createMockedApp } = await import("../../src/api/routes"); + const mockedApp = createMockedApp(); + + const env = createTestEnv(); + await seedRepoWithPull(env); + // chatQa off (no stub): the short-circuit must return before the planNextWork grounding-bundle build. + const res = await mockedApp.request("/v1/repos/owner/repo/pulls/11/chat-qa", { method: "POST", headers: apiHeaders(env), body: JSON.stringify({ question: "why?" }) }, env); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toMatchObject({ status: "disabled" }); + expect(planNextWork).not.toHaveBeenCalled(); + }); + it("builds a bundle via planNextWork and passes it, the question, and settings through to generateChatQaAnswer, returning its result verbatim", async () => { vi.resetModules(); const generateChatQaAnswer = vi.fn().mockResolvedValue({ status: "ok", model: "test-model", estimatedNeurons: 12, text: "This PR is blocked on a failing check." }); @@ -153,6 +184,9 @@ describe("POST /v1/repos/:owner/:repo/pulls/:number/chat-qa (#6489)", () => { const env = createTestEnv(); await seedRepoWithPull(env, { authorLogin: null }); + // chatQa must be ENABLED for this test to reach planNextWork -- it exercises the grounding-login fallback on + // the enabled path. (#9714 now short-circuits a disabled repo before planNextWork, so the enable is explicit.) + stubChatQaManifestFetch(); const res = await mockedApp.request("/v1/repos/owner/repo/pulls/11/chat-qa", { method: "POST", headers: apiHeaders(env), body: JSON.stringify({ question: "why is this blocked?" }) }, env); expect(res.status).toBe(200);