Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions .changeset/review-ticket-context-premise-rules.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"review": minor
---

The PR's linked Jira ticket is now staged deterministically as `ticket-context.json` (new lib/stage-ticket.ts, run from stage-pr.ts): when a consumer configures `REVIEW_JIRA_BASE_URL` (variable) plus `REVIEW_JIRA_EMAIL`/`REVIEW_JIRA_API_TOKEN` (secrets), the staging resolves the issue key from the PR title, head branch, or description and fetches the ticket read-only on the host, before the agent starts; every degradation (unconfigured, no key, 404, fetch failure) stages `{available: false, reason}` and never fails the run. This replaces the completeness reviewer's in-prompt Jira/Confluence read grant, which was dead text: no consumer ever provided the token it promised and the firewall egress never included the Jira host, so its fallback clause fired on every run. The agent sandbox needs no Jira egress and never sees the credentials. The first-principles reviewer gets the staged ticket too (its whole mandate is the stated rationale, which it previously read only via the author's summary), plus two prompt rules minted from webapp#41609: a finding that pushes against a stated, rationale-backed decision must rebut the rationale with new evidence or not post (an observation your own prose concedes is not a finding), and several observations sharing one premise merge into one finding.
11 changes: 11 additions & 0 deletions workflows/review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,17 @@ Two known interactions:

Optional:

- `REVIEW_JIRA_BASE_URL` (repo **variable**), `REVIEW_JIRA_EMAIL` and
`REVIEW_JIRA_API_TOKEN` (repo **secrets**) – the linked-ticket staging
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved
(`lib/stage-ticket.ts`). When all three are set, the pre-agent staging
resolves the PR's Jira issue key (title, then head branch, then
description), fetches the ticket read-only on the host, and stages it as
`ticket-context.json` for the intent-reading sub-agents (completeness,
first-principles). The agent sandbox never sees the credentials and has no
Jira egress. Use a service-account API token with read-only scope. Without
these, the file stages `{available: false, reason: "not-configured"}` and
those sub-agents fall back to the PR description; a ticket is context,
never a prerequisite, so nothing else changes.
- `REVIEW_BOT_LOGIN` — the account this workflow posts reviews as, default
`github-actions[bot]`. Set it only in a repo that posts under its own GitHub
App, in the installed `review.md`'s workflow-level `env:` block (the one
Expand Down
48 changes: 47 additions & 1 deletion workflows/review/lib/stage-pr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ const PR_META = {
body: "d",
user: {login: "octo"},
base: {ref: "main"},
head: {sha: "abc123"},
head: {sha: "abc123", ref: "feature/KORE-9"},
draft: false,
};

Expand Down Expand Up @@ -215,6 +215,46 @@ describe("computeDiffFingerprint", () => {
describe("runStagePrCli", () => {
const options = {repo: "o/r", prNumber: 7, repoRoot: "/work"};

it("wires the ticket staging through env and the PR's head branch", async () => {
const fs = makeFakeFs();
const fetched: string[] = [];
await runStagePrCli(
fs,
ghGetFromMap(
baseRoutes([
{filename: "a.ts", status: "modified", patch: PATCH_ONE},
]),
),
noThreads(),
{
...options,
env: {
REVIEW_JIRA_BASE_URL: "https://khanacademy.atlassian.net",
REVIEW_JIRA_EMAIL: "bot@khanacademy.org",
REVIEW_JIRA_API_TOKEN: "tok",
},
ticketFetch: (url) => {
fetched.push(url);
return Promise.resolve({
status: 200,
json: {fields: {summary: "the ticket"}},
});
},
},
);
// The key comes from the head branch (the title carries none).
expect(fetched).toEqual([
"https://khanacademy.atlassian.net/rest/api/2/issue/KORE-9?fields=summary,description,status,resolution,issuetype,labels,comment",
]);
expect(
JSON.parse(fs.files[`${REVIEW}/ticket-context.json`]),
).toMatchObject({
available: true,
key: "KORE-9",
summary: "the ticket",
});
});

it("stages the full Step 1 + Step 3 contract for a first review", async () => {
const fs = makeFakeFs({
"/tmp/gh-aw/aw-prompts/prompt.txt": [
Expand Down Expand Up @@ -250,6 +290,12 @@ describe("runStagePrCli", () => {
{path: "a.ts", status: "modified", hasPatch: true},
{path: "bin.png", status: "modified", hasPatch: false},
]);
// ticket-context.json is ALWAYS written (readers never need an
// existence check); without a ticketFetch it stages not-configured.
expect(JSON.parse(read("ticket-context.json"))).toEqual({
available: false,
reason: "not-configured",
});
expect(read("full.diff")).toContain("diff --git a/a.ts b/a.ts");
const facts = JSON.parse(read("diff-facts.json"));
expect(Object.keys(facts.diffFingerprint)).toEqual(["a.ts", "bin.png"]);
Expand Down
52 changes: 51 additions & 1 deletion workflows/review/lib/stage-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
* What it stages under /tmp/gh-aw/review/ (the Step 1 contract, unchanged):
*
* pr-context.json PR metadata (untrusted author text included verbatim)
* ticket-context.json the linked Jira ticket (stage-ticket.ts): the PR's
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved
* issue key resolved and fetched read-only when the
* consumer configures credentials; otherwise (and on
* any fetch failure) {available: false, reason} — a
* ticket is context, never a prerequisite
* files.json path/status/hasPatch per changed file
* full.diff standard unified diff rebuilt from the per-file
* patches (diff --git + ---/+++ headers per file, which
Expand Down Expand Up @@ -88,6 +93,7 @@ import {
} from "./diff";
import {runProvenanceCli} from "./provenance";
import type {StagedThread} from "./rereview";
import {stageTicketContext, type TicketFetch} from "./stage-ticket";
import {runRereviewPlanCli} from "./rereview-mode";
import {runCli as runRouterCli} from "./router";
import {
Expand All @@ -111,6 +117,7 @@ const CACHE_MEMORY_DIR = "/tmp/gh-aw/cache-memory";
* reads it.
*/
const PR_CONTEXT_OUT = `${REVIEW_DIR}/pr-context.json`;
const TICKET_CONTEXT_OUT = `${REVIEW_DIR}/ticket-context.json`;
const FILES_OUT = `${REVIEW_DIR}/files.json`;
const FULL_DIFF_OUT = `${REVIEW_DIR}/full.diff`;
const DIFF_FACTS_OUT = `${REVIEW_DIR}/diff-facts.json`;
Expand Down Expand Up @@ -163,6 +170,12 @@ export type StagePrOptions = {
repoRoot: string;
/** Env forwarded to the router (`REVIEW_MAX_AI_CREDITS`). */
env?: Record<string, string | undefined>;
/**
* Jira GET for the linked-ticket staging (stage-ticket.ts). Absent (tests
* that don't exercise it) behaves like an unconfigured consumer:
* ticket-context.json stages `{available: false}`.
*/
ticketFetch?: TicketFetch;
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved
/** Cache-memory dir override (tests). */
cacheMemoryDir?: string;
/** Rendered-prompt path override (tests); default gh-aw's prompt.txt. */
Expand Down Expand Up @@ -386,7 +399,7 @@ export const runStagePrCli = async (
body?: string | null;
user?: {login?: string};
base?: {ref?: string};
head?: {sha?: string};
head?: {sha?: string; ref?: string};
draft?: boolean;
};
if (
Expand Down Expand Up @@ -421,6 +434,29 @@ export const runStagePrCli = async (
),
);

// 1b. The linked Jira ticket → ticket-context.json (never a
// prerequisite: every degradation stages {available: false, reason} and
// the intent-reading sub-agents fall back to the PR description).
const ticket =
options.ticketFetch === undefined
? {
context: {
available: false as const,
reason: "not-configured" as const,
},
warnings: [],
}
: await stageTicketContext(options.ticketFetch, {
baseUrl: env.REVIEW_JIRA_BASE_URL ?? "",
email: env.REVIEW_JIRA_EMAIL ?? "",
apiToken: env.REVIEW_JIRA_API_TOKEN ?? "",
title: pr.title ?? "",
headBranch: pr.head?.ref ?? "",
description: pr.body ?? "",
});
warnings.push(...ticket.warnings);
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved
write(TICKET_CONTEXT_OUT, JSON.stringify(ticket.context, null, 2));
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved

// 2. Changed files → files.json + full.diff (hard prerequisite).
const files = await fetchAllFiles(ghGet, repo, prNumber);
write(
Expand Down Expand Up @@ -865,11 +901,25 @@ if (typeof require !== "undefined" && require.main === module) {
);
process.exit(2);
}
// The linked-ticket GET (stage-ticket.ts). Plain fetch, no retry: a
// ticket is context, not a prerequisite, and stage-ticket degrades every
// failure to {available: false} rather than failing the staging.
const ticketFetch = async (
url: string,
headers: Record<string, string>,
): Promise<{status: number; json: unknown}> => {
const response = await fetch(url, {headers});
Comment thread
khan-actions-bot marked this conversation as resolved.
Resolved
return {
status: response.status,
json: await response.json().catch(() => null),
};
};
void runStagePrCli(nodeFs, ghGet, ghGraphql, {
repo,
prNumber,
repoRoot,
env: process.env,
ticketFetch,
})
.then((result) => {
// eslint-disable-next-line no-console
Expand Down
Loading
Loading