Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/github/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 |",
"| --- | --- |",
Expand All @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/github/pr-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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 };
});
}

Expand Down
39 changes: 27 additions & 12 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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",
Expand Down
26 changes: 17 additions & 9 deletions src/services/ai-chat-qa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
24 changes: 23 additions & 1 deletion test/unit/ai-chat-qa.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
});

Expand Down
29 changes: 29 additions & 0 deletions test/unit/github-commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {},
Expand Down
10 changes: 10 additions & 0 deletions test/unit/github-pr-actions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
Loading