diff --git a/src/github/commands.ts b/src/github/commands.ts index 8894de53e1..8c7a0d10be 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -417,6 +417,11 @@ export function buildPublicAgentCommandComment(args: { * mention to `matchedCommand` -- shown as a visible "interpreted as" note (req 6) so a wrong match is * immediately correctable, rather than silently answering a different question than the one asked. */ interpretedFrom?: { question: string; matchedCommand: GittensoryMentionCommandName } | undefined; + /** GitHub's own `html_url` for the triggering comment (from the webhook payload), set by the dispatcher only + * for ask/chat -- these two post a FRESH reply per invocation instead of updating the shared PR panel in + * place, so this renders a visible "replying to" link back to the specific question being answered. Every + * other command still shares the panel slot and has no single triggering comment to point back at. */ + replyingToUrl?: string | undefined; /** Resolved by the caller from `env.PUBLIC_SITE_ORIGIN` -- see `gittensoryFooter` (#4613). */ env: GittensoryFooterEnv; }): string { @@ -447,7 +452,9 @@ export function buildPublicAgentCommandComment(args: { "", "> [!NOTE]", `> **${COMMAND_TITLES[commandName]}**`, - "> Gittensory updated this command response in place from cached public-safe context.", + args.replyingToUrl + ? "> Gittensory posted this as a fresh reply -- it never updates or replaces the PR review panel." + : "> Gittensory updated this command response in place from cached public-safe context.", "", "| Signal | State |", "| --- | --- |", @@ -457,6 +464,7 @@ export function buildPublicAgentCommandComment(args: { "", `Command: \`@gittensory ${commandName}\``, "", + ...(args.replyingToUrl ? [`> 💬 Replying to [this comment](${args.replyingToUrl}).`, ""] : []), // (#4596 req 6) Free-form contributor text, same neutralization as the chat question line (#2457) -- // this is the first place a re-routed mention's own text is echoed back into a trusted bot comment. ...(args.interpretedFrom diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index 1c41c36c0e..43e7d4802f 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -181,7 +181,7 @@ export async function updatePullRequestBranch( } /** Post a plain issue/PR comment (used for the templated close message before closing). */ -export async function createIssueComment(env: Env, installationId: number, repoFullName: string, issueNumber: number, body: string): Promise<{ id: number }> { +export async function createIssueComment(env: Env, installationId: number, repoFullName: string, issueNumber: number, body: string): Promise<{ id: number; html_url?: string | undefined }> { const { owner, repo } = splitRepo(repoFullName); return withInstallationTokenRetry(env, installationId, async (token) => { const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId)); @@ -191,7 +191,8 @@ export async function createIssueComment(env: Env, installationId: number, repoF issue_number: issueNumber, body, }); - return { id: (response.data as { id: number }).id }; + const data = response.data as { id: number; html_url?: string }; + return { id: data.id, html_url: data.html_url }; }); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c6b20ca4e1..50069b9f93 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -12652,6 +12652,14 @@ async function maybeProcessGittensoryMentionCommand( route: "github_app.chat_qa", }) : null; + // Q&A commands (#5063): ask/chat answer a SPECIFIC question at a point in time, unlike every other command + // here (preflight/blockers/etc.), which reports the PR's CURRENT state and therefore reasonably shares one + // persistent, continuously-updated panel comment. Reusing that same panel slot for ask/chat meant each new + // question silently overwrote the previous answer (and could overwrite the review verdict itself), with the + // reply landing wherever the panel comment originally happened to sit -- never near the question that + // prompted it. Post a fresh reply per invocation instead, linking back to the triggering comment. + const isQaCommand = command.name === "ask" || command.name === "chat"; + const replyingToUrl = isQaCommand ? (payload.comment?.html_url ?? undefined) : undefined; const body = buildPublicAgentCommandComment({ command, repo, @@ -12665,16 +12673,21 @@ async function maybeProcessGittensoryMentionCommand( maintainerDigest, chatAnswer, interpretedFrom, + replyingToUrl, env, }); - const responseComment = await createOrUpdateAgentCommandComment( - env, - installationId, - repoFullName, - issue.number, - body, - mentionMode, - ); + const responseComment = isQaCommand + ? mentionMode === "live" + ? await createIssueComment(env, installationId, repoFullName, issue.number, body) + : null + : await createOrUpdateAgentCommandComment( + env, + installationId, + repoFullName, + issue.number, + body, + mentionMode, + ); await upsertAgentCommandAnswer(env, { id: answerId, repoFullName, @@ -12690,10 +12703,12 @@ async function maybeProcessGittensoryMentionCommand( responseCommentStored: Boolean(responseComment?.id), }, }); - // createOrUpdateAgentCommandComment already suppresses the answer-card post for a non-live mode. As with - // gate-override above, what must NOT happen unconditionally is recording this as a completed reply: a - // paused/dry-run mention command never posted the card, so telemetry (and the feedback prompt, which - // presumes a real reply exists to react to) must reflect that instead of a reply that never happened. + // Both posting paths above suppress the actual write for a non-live mode -- createOrUpdateAgentCommandComment + // does it internally; the isQaCommand branch checks mentionMode itself before calling createIssueComment, + // which has no such awareness of its own. As with gate-override above, what must NOT happen unconditionally + // is recording this as a completed reply: a paused/dry-run mention command never posted anything, so + // telemetry (and the feedback prompt, which presumes a real reply exists to react to) must reflect that + // instead of a reply that never happened. if (mentionMode === "live") { await recordAuditEvent(env, { eventType: "github_app.agent_command_replied", diff --git a/src/services/ai-chat-qa.ts b/src/services/ai-chat-qa.ts index ad2def9d6b..11e1bfcd29 100644 --- a/src/services/ai-chat-qa.ts +++ b/src/services/ai-chat-qa.ts @@ -148,15 +148,23 @@ export async function generateChatQaAnswer(env: Env, req: ChatQaRequest): Promis } try { - const response = await ai.run(model, { - messages: [ - { role: "system", content: CHAT_QA_SYSTEM_PROMPT }, - { role: "user", content: prompt }, - ], - max_tokens: maxOutputTokens, - temperature: 0.1, - }); - const rawText = extractAiText(response); + // A local/quantized model occasionally returns a genuinely empty completion for no discernible reason -- + // ai.run() resolves normally, extractAiText() just finds nothing usable in it (not a network/auth failure, + // which throws instead and is never retried here). One bare retry recovers most of these transient blanks + // without masking a real, persistent failure: if it's STILL empty on the second try, the loop falls + // through with rawText === "" and the check below throws exactly as it always did. + let rawText = ""; + for (let attempt = 0; attempt < 2 && !rawText; attempt += 1) { + const response = await ai.run(model, { + messages: [ + { role: "system", content: CHAT_QA_SYSTEM_PROMPT }, + { role: "user", content: prompt }, + ], + max_tokens: maxOutputTokens, + temperature: 0.1, + }); + rawText = extractAiText(response) ?? ""; + } if (!rawText) throw new Error("empty_chat_answer"); if (containsPublicForbiddenText(rawText)) { await recordChatAi(env, req, { model, status: "unsafe", estimatedNeurons, detail: "chat answer failed public sanitizer", usedFrontier }); diff --git a/test/unit/ai-chat-qa.test.ts b/test/unit/ai-chat-qa.test.ts index 7a56b1cfe1..c0f7ea1c94 100644 --- a/test/unit/ai-chat-qa.test.ts +++ b/test/unit/ai-chat-qa.test.ts @@ -302,11 +302,33 @@ describe("generateChatQaAnswer", () => { expect(result).toMatchObject({ status: "error", reason: "chat_answer_failed" }); }); - it("reports an error status when the provider returns an empty/unrecognized response shape", async () => { + it("reports an error status when the provider returns an empty/unrecognized response shape on BOTH attempts (retries once, then gives up)", async () => { const run = vi.fn(async () => ({ unexpected: "shape" })); const env = createTestEnv({ AI_ADVISORY: { run } as unknown as Ai, AI_DAILY_NEURON_BUDGET: "10000" }); const result = await generateChatQaAnswer(env, { bundle: bundleFixture(), question: "why?", advisoryAiRouting: ADVISORY_ON, repoFullName: "owner/repo", issueNumber: 1 }); expect(result).toMatchObject({ status: "error", reason: "empty_chat_answer" }); + expect(run).toHaveBeenCalledTimes(2); // one bare retry on an empty completion, not an unbounded loop + }); + + it("recovers a transiently-empty first completion: retries once and succeeds when the second attempt returns real text", async () => { + const run = vi + .fn() + .mockResolvedValueOnce({ unexpected: "shape" }) + .mockResolvedValueOnce({ response: "Recovered on the second attempt." }); + const env = createTestEnv({ AI_ADVISORY: { run } as unknown as Ai, AI_DAILY_NEURON_BUDGET: "10000" }); + const result = await generateChatQaAnswer(env, { bundle: bundleFixture(), question: "why?", advisoryAiRouting: ADVISORY_ON, repoFullName: "owner/repo", issueNumber: 1 }); + expect(result).toMatchObject({ status: "ok", text: "Recovered on the second attempt." }); + expect(run).toHaveBeenCalledTimes(2); + }); + + it("never retries when the provider throws (network/auth failure), only when it resolves empty", async () => { + const run = vi.fn(async () => { + throw new Error("provider_down"); + }); + const env = createTestEnv({ AI_ADVISORY: { run } as unknown as Ai, AI_DAILY_NEURON_BUDGET: "10000" }); + const result = await generateChatQaAnswer(env, { bundle: bundleFixture(), question: "why?", advisoryAiRouting: ADVISORY_ON, repoFullName: "owner/repo", issueNumber: 1 }); + expect(result).toMatchObject({ status: "error", reason: "provider_down" }); + expect(run).toHaveBeenCalledTimes(1); // a thrown error is not the "empty completion" case -- no retry }); }); diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index 5657fd82c5..cf3a7c2c85 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -795,6 +795,35 @@ describe("GitHub mention commands", () => { expect(notRouted).not.toContain("Interpreted"); }); + it("#5063: renders a 'replying to' link and the fresh-reply phrasing when replyingToUrl is set (ask/chat only), and the original in-place phrasing when it is not", () => { + const reply = buildPublicAgentCommandComment({ + env: {}, + command: parseGittensoryMentionCommand("@gittensory ask what should I fix first?")!, + repo: null, + issue: { number: 40, title: "PR", state: "open", pull_request: {} }, + pullRequest: null, + actorKind: "author", + bundle: sampleBundle(), + replyingToUrl: "https://github.com/acme/widget/pull/40#issuecomment-123456789", + }); + expect(reply).toContain("> 💬 Replying to [this comment](https://github.com/acme/widget/pull/40#issuecomment-123456789)."); + expect(reply).toContain("Gittensory posted this as a fresh reply -- it never updates or replaces the PR review panel."); + expect(reply).not.toContain("Gittensory updated this command response in place"); + + const panelUpdate = buildPublicAgentCommandComment({ + env: {}, + command: parseGittensoryMentionCommand("@gittensory preflight")!, + repo: null, + issue: { number: 41, title: "PR", state: "open", pull_request: {} }, + pullRequest: null, + actorKind: "author", + bundle: sampleBundle(), + }); + expect(panelUpdate).not.toContain("Replying to"); + expect(panelUpdate).toContain("Gittensory updated this command response in place from cached public-safe context."); + expect(panelUpdate).not.toContain("fresh reply"); + }); + it("REGRESSION (#4596): neutralizes markdown/HTML and zero-width-spaces @mentions in the interpreted-from question, same as the ask/chat question lines (#2457)", () => { const forged = buildPublicAgentCommandComment({ env: {}, diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index f7a3d7f83e..20c7a1bb47 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -224,6 +224,16 @@ describe("GitHub PR action primitives (#778)", () => { expect(calls[0]?.url).toMatch(/\/repos\/owner\/repo\/issues\/7\/comments$/); }); + it("#5063: surfaces the created comment's html_url (used to build the ask/chat 'replying to' link) when GitHub returns one", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + return Response.json({ id: 9, html_url: "https://github.com/owner/repo/pull/7#issuecomment-9" }); + }); + const result = await createIssueComment(envWithKey(), 123, "owner/repo", 7, "hello"); + expect(result).toEqual({ id: 9, html_url: "https://github.com/owner/repo/pull/7#issuecomment-9" }); + }); + it("walks paginated issue events to find the true most recent closer", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 2bbdd09be0..7ededcc289 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1301,6 +1301,151 @@ describe("queue processors", () => { expect(seen.comments[0]).not.toContain("not enabled on this instance"); }); + it("#5063: posts a FRESH, separate reply comment for each chat invocation (never edits a shared comment), each linking back to its own triggering comment", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: "Answer to the question." }) } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 320, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const posted: Array<{ body: string; commentId: number }> = []; + let nextResponseCommentId = 9500; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/320/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/320/comments") && method === "POST") { + const body = String(JSON.parse(String(init?.body ?? "{}")).body ?? ""); + const id = nextResponseCommentId++; + posted.push({ body, commentId: id }); + return Response.json({ id, html_url: `https://github.com/JSONbored/gittensory/pull/320#issuecomment-${id}` }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + const basePayload = { + action: "created" as const, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 320, title: "Rate limit target", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + }; + await processJob(env, { + type: "github-webhook", + deliveryId: "qa-reply-first", + eventName: "issue_comment", + payload: { + ...basePayload, + comment: { id: 9001, body: "@gittensory chat what does this PR add?", html_url: "https://github.com/JSONbored/gittensory/pull/320#issuecomment-9001", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }); + await processJob(env, { + type: "github-webhook", + deliveryId: "qa-reply-second", + eventName: "issue_comment", + payload: { + ...basePayload, + comment: { id: 9002, body: "@gittensory chat tell me more?", html_url: "https://github.com/JSONbored/gittensory/pull/320#issuecomment-9002", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }); + expect(posted).toHaveLength(2); // two distinct replies -- never one comment overwritten twice + expect(posted[0]?.body).toContain("Replying to [this comment](https://github.com/JSONbored/gittensory/pull/320#issuecomment-9001)"); + expect(posted[1]?.body).toContain("Replying to [this comment](https://github.com/JSONbored/gittensory/pull/320#issuecomment-9002)"); + expect(posted[0]?.commentId).not.toBe(posted[1]?.commentId); + }); + + it("#5063: never edits an existing PR-panel review comment when dispatching chat -- posts a separate reply instead", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: "Answer." }) } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 321, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const patchCalls: string[] = []; + const postedBodies: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/321/comments") && method === "GET") { + // An existing PR-panel review comment: createOrUpdateAgentCommandComment would find + edit this for + // any OTHER command's answer card, but chat must never reach (or touch) it. + return Response.json([{ id: 500, body: "\n\nExisting review verdict.", user: { login: "gittensory-orb[bot]", type: "Bot" } }]); + } + if (url.includes("/issues/comments/500") && method === "PATCH") { + patchCalls.push(url); + return Response.json({ id: 500 }); + } + if (url.includes("/issues/321/comments") && method === "POST") { + const body = String(JSON.parse(String(init?.body ?? "{}")).body ?? ""); + postedBodies.push(body); + return Response.json({ id: 999, html_url: "https://github.com/JSONbored/gittensory/pull/321#issuecomment-999" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + await processJob(env, { + type: "github-webhook", + deliveryId: "qa-reply-preserves-panel", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 321, title: "Rate limit target", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 7001, body: "@gittensory chat what changed?", html_url: "https://github.com/JSONbored/gittensory/pull/321#issuecomment-7001", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }); + expect(patchCalls).toHaveLength(0); // the existing panel comment (id 500) is never edited + expect(postedBodies).toHaveLength(1); // chat's answer is a brand-new comment instead + expect(postedBodies[0]).toContain("Replying to"); + expect(postedBodies[0]).not.toContain("Existing review verdict"); + }); + + it("#5063: dry-run mode never posts a live chat reply, but still records the reply as never-happened (not a completed answer)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: "Answer to the question." }) } as unknown as Ai, + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", agentDryRun: true }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 322, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const postedBodies: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n chatQa: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/322/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/322/comments") && method === "POST") { + postedBodies.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: 1 }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + await processJob(env, { + type: "github-webhook", + deliveryId: "qa-reply-dry-run", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 322, title: "Rate limit target", state: "open", pull_request: {}, user: { login: "oktofeesh1" }, author_association: "NONE" }, + comment: { id: 7002, body: "@gittensory chat what changed?", html_url: "https://github.com/JSONbored/gittensory/pull/322#issuecomment-7002", user: { login: "maintainer", type: "User" }, author_association: "OWNER" }, + }, + }); + expect(postedBodies).toHaveLength(0); // dry-run: createIssueComment is never called at all + const replied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.agent_command_replied'").first<{ n: number }>(); + expect(replied?.n).toBe(0); // no reply was actually posted, so it must not be recorded as one + }); + it("#4595: chat declines gracefully end-to-end (never posts model text) when chatQa is off, the default", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 308, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" });