Skip to content

Commit 0f4276d

Browse files
authored
fix(api): skip the shared rate-limit budget when chat Q&A is off for the repo (#9734)
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
1 parent 0c67c5e commit 0f4276d

2 files changed

Lines changed: 53 additions & 0 deletions

File tree

src/api/routes.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3537,6 +3537,25 @@ export function createApp() {
35373537
const [settings, pullRequest] = await Promise.all([resolveRepositorySettings(c.env, fullName), getPullRequest(c.env, fullName, number)]);
35383538
if (!pullRequest) return c.json({ error: "pull_request_not_found" }, 404);
35393539

3540+
// When chat Q&A is disabled for the repo, generateChatQaAnswer is guaranteed to return `disabled` -- but only
3541+
// after this route has already spent a slot in the shared @loopover-chat rate-limit budget AND paid for the
3542+
// planNextWork grounding bundle (#9714). Gate on the SAME predicate the maintainer dashboard reads
3543+
// (isRepoChatQaEnabled at the capability map above), so the two can never disagree, and return
3544+
// generateChatQaAnswer's own disabled result (bundle unused on that path) rather than a second copy of it.
3545+
if (!isRepoChatQaEnabled(settings)) {
3546+
return c.json(
3547+
await generateChatQaAnswer(c.env, {
3548+
bundle: null,
3549+
question: parsed.data.question,
3550+
advisoryAiRouting: settings.advisoryAiRouting,
3551+
repoFullName: fullName,
3552+
issueNumber: number,
3553+
actor: resolveChatQaActor(gate.identity),
3554+
route: "app.maintainer_dashboard.chat_qa",
3555+
}),
3556+
);
3557+
}
3558+
35403559
const actor = resolveChatQaActor(gate.identity);
35413560
const targetKey = `${fullName}#${number}#chat`;
35423561
const { policy, maxPerWindow, windowHours } = resolveChatQaRateLimit(settings);

test/unit/maintainer-chat-qa.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,37 @@ describe("POST /v1/repos/:owner/:repo/pulls/:number/chat-qa (#6489)", () => {
104104
await expect(res.json()).resolves.toEqual({ status: "disabled", reason: "Chat Q&A is not enabled on this instance (settings.advisoryAiRouting.chatQa is off)." });
105105
});
106106

107+
it("#9714: a disabled repo does NOT record a rate-limit audit row (it must not spend the shared @loopover-chat budget)", async () => {
108+
const env = createTestEnv();
109+
await seedRepoWithPull(env);
110+
// chatQa is off (no .loopover.yml stub). The request still returns disabled, but must not have consumed a slot.
111+
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);
112+
expect(res.status).toBe(200);
113+
await expect(res.json()).resolves.toMatchObject({ status: "disabled" });
114+
115+
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 }>();
116+
expect(invocationRows?.n).toBe(0);
117+
});
118+
119+
it("#9714: a disabled repo does NOT call planNextWork (it must not pay for the grounding bundle)", async () => {
120+
vi.resetModules();
121+
const planNextWork = vi.fn();
122+
vi.doMock("../../src/services/agent-orchestrator", async () => {
123+
const actual = await vi.importActual<typeof import("../../src/services/agent-orchestrator")>("../../src/services/agent-orchestrator");
124+
return { ...actual, planNextWork };
125+
});
126+
const { createApp: createMockedApp } = await import("../../src/api/routes");
127+
const mockedApp = createMockedApp();
128+
129+
const env = createTestEnv();
130+
await seedRepoWithPull(env);
131+
// chatQa off (no stub): the short-circuit must return before the planNextWork grounding-bundle build.
132+
const res = await mockedApp.request("/v1/repos/owner/repo/pulls/11/chat-qa", { method: "POST", headers: apiHeaders(env), body: JSON.stringify({ question: "why?" }) }, env);
133+
expect(res.status).toBe(200);
134+
await expect(res.json()).resolves.toMatchObject({ status: "disabled" });
135+
expect(planNextWork).not.toHaveBeenCalled();
136+
});
137+
107138
it("builds a bundle via planNextWork and passes it, the question, and settings through to generateChatQaAnswer, returning its result verbatim", async () => {
108139
vi.resetModules();
109140
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)", () => {
153184

154185
const env = createTestEnv();
155186
await seedRepoWithPull(env, { authorLogin: null });
187+
// chatQa must be ENABLED for this test to reach planNextWork -- it exercises the grounding-login fallback on
188+
// the enabled path. (#9714 now short-circuits a disabled repo before planNextWork, so the enable is explicit.)
189+
stubChatQaManifestFetch();
156190

157191
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);
158192
expect(res.status).toBe(200);

0 commit comments

Comments
 (0)