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
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 collects every issue key the PR references (title, head branch, description; known key-shaped noise like UTF-8 or SHA-256 sinks below plausible keys before the cap of 5 applies) and fetches each 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.
18 changes: 18 additions & 0 deletions workflows/review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,24 @@ Two known interactions:

Optional:

- `REVIEW_JIRA_BASE_URL` (repo **variable**), `REVIEW_JIRA_EMAIL` and
`REVIEW_JIRA_API_TOKEN` (repo **secrets**): the linked-ticket staging
(`lib/stage-ticket.ts`). When all three are set, the pre-agent staging
collects every Jira issue key the PR references (title, head branch, and
description, deduped; known key-shaped noise like `UTF-8`, `SHA-256`, and
`CVE-2024-1234` sinks to the back before the cap of 5 applies), fetches
each read-only on the host, and stages the ones that resolve as
`ticket-context.json` (a `tickets` array) for the intent-reading
sub-agents (completeness, first-principles). Noise and stale keys 404 and
drop silently. The disclosure
bound (which tickets author-written keys can pull into a review that
posts publicly) is the service account's own Jira permissions, enforced
server-side: use a dedicated service account granted Browse Projects on
only the projects reviews may quote, with a read-only API token. The
agent sandbox never sees the credentials and has no Jira egress. 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
26 changes: 26 additions & 0 deletions workflows/review/eval/corpus/live.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,14 @@ export type CaseLive = {
* nothing more.
*/
tree: string;
/**
* Optional staged ticket-context.json content, for cases exercising the
* ticket-present path of the intent-reading prompts (the shape is
* lib/stage-ticket.ts's TicketContext; only `available` is validated).
* Absent, staging writes `{available: false, reason: "not-configured"}`,
* matching an unconfigured production consumer.
*/
ticket?: Record<string, unknown>;
/** Labeled defects a live run must catch. */
mustCatchSpecs?: LiveDefectSpec[];
/** Labeled traps a live run must NOT flag (clean-case ground truth). */
Expand Down Expand Up @@ -649,12 +657,30 @@ export const parseLive = (
errors,
);

const rawTicket = raw["ticket"];
let ticket: Record<string, unknown> | undefined;
if (rawTicket !== undefined) {
if (
!isRecord(rawTicket) ||
typeof rawTicket["available"] !== "boolean"
) {
errors.push(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): New live.ticket parsing in parseLive is untested. grep -rn "ticket" workflows/review/eval/corpus/loader.test.ts returns nothing, and parseLive is only reachable through parseCase in loader.test.ts, which has a per-field convention ("rejects a live block with a missing prContext field"). live-stage.test.ts covers the ticket write, but it builds the CaseLive object directly, so the parse→stage chain is never exercised end to end.

A sketch, not a committable replacement:

// workflows/review/eval/corpus/loader.test.ts, in `describe("parseCase: the live block")`
it("parses a live.ticket block and rejects a malformed one", () => {
    const withTicket = (ticket: unknown) =>
        liveCase({
            live: {
                prContext: {
                    title: "A change",
                    description: "",
                    author: "octocat",
                    baseBranch: "main",
                },
                ticket,
            },
        });
    const parsed = parseCase(
        withTicket({available: true, tickets: [{key: "KORE-1"}]}),
        "(case/redacted)
    );
    expect(parsed.live?.ticket).toEqual({
        available: true,
        tickets: [{key: "KORE-1"}],
    });
    expect(parseErrors(withTicket({tickets: []}))).toMatch(
        /live\.ticket: must be an object/,
    );
});
review details found by test-adequacy

"live.ticket: must be an object with a boolean `available` when present",
);
} else {
ticket = rawTicket;
}
}

const rereview = parseRereview(raw["rereview"], errors);

if (prContext === undefined) {
return undefined;
}
const live: CaseLive = {prContext, tree};
if (ticket !== undefined) {
live.ticket = ticket;
}
if (mustCatchSpecs !== undefined) {
live.mustCatchSpecs = mustCatchSpecs;
}
Expand Down
32 changes: 32 additions & 0 deletions workflows/review/eval/corpus/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,38 @@ describe("parseCase: the live block", () => {
).toMatch(/lineStart <= lineEnd/);
});

it("parses a live.ticket block and rejects a malformed one", () => {
const withTicket = (ticket: unknown) =>
liveCase({
live: {
prContext: {
title: "A change",
description: "",
author: "octocat",
baseBranch: "main",
},
ticket,
},
});
const parsed = parseCase(
withTicket({available: true, tickets: [{key: "KORE-1"}]}),
"test://case",
);
expect(parsed.live?.ticket).toEqual({
available: true,
tickets: [{key: "KORE-1"}],
});
// `available` is the only validated field: the shape is
// stage-ticket.ts's TicketContext, owned by lib, not re-specified
// here.
expect(parseErrors(withTicket({tickets: []}))).toMatch(
/live\.ticket: must be an object with a boolean `available`/,
);
expect(parseErrors(withTicket("KORE-1"))).toMatch(
/live\.ticket: must be an object/,
);
});

it("requires a diff on a live case", () => {
const raw = liveCase();
delete (raw as Record<string, unknown>)["diff"];
Expand Down
39 changes: 39 additions & 0 deletions workflows/review/eval/live-stage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,45 @@ describe("stageCase", () => {
"lensesToSpawn",
);
expect(vol.existsSync("/stage/context/out")).toBe(true);

// ticket-context.json is ALWAYS written (stage-ticket.ts's
// no-existence-check contract); without a live.ticket block it
// stages the production unconfigured shape.
expect(JSON.parse(read("/stage/context/ticket-context.json"))).toEqual({
available: false,
reason: "not-configured",
});
});

it("stages a case's live.ticket block as ticket-context.json", () => {
const vol = treeVol();
const ticket = {
available: true,
tickets: [{key: "KORE-1", summary: "the ticket"}],
};
stageCase(
liveCase({
live: {
prContext: {
title: "A staged change",
description: "body text",
author: "octocat",
baseBranch: "main",
},
ticket,
},
}),
"/stage",
volFs(vol),
);
expect(
JSON.parse(
vol.readFileSync(
"/stage/context/ticket-context.json",
"utf8",
) as string,
),
).toEqual(ticket);
});

it("copies the tree recursively into the checkout", () => {
Expand Down
19 changes: 19 additions & 0 deletions workflows/review/eval/live-stage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
* run against a real PR:
*
* <dest>/context/pr-context.json PR metadata (from the case's live block)
* <dest>/context/ticket-context.json the linked-ticket surface (the case's
* live.ticket block, or the production
* unconfigured shape {available: false,
* reason: "not-configured"}): always
* written, matching stage-ticket.ts's
* no-existence-check contract
* <dest>/context/full.diff the case diff (git-style unified diff)
* <dest>/context/full-stripped.diff = full.diff (corpus diffs carry no
* generated files to strip)
Expand Down Expand Up @@ -322,6 +328,19 @@ export const stageCase = (
),
);

// The linked-ticket surface (stage-ticket.ts's contract: the file is
// ALWAYS written, so prompt readers never need an existence check). A
// case without a ticket block stages the same shape an unconfigured
// production consumer does.
fs.writeFileSync(
`${contextDir}/ticket-context.json`,
JSON.stringify(
live.ticket ?? {available: false, reason: "not-configured"},
null,
2,
),
);

// The post-change checkout the sub-agents read and investigate.
const lastSlash = corpusCase.sourcePath.lastIndexOf("/");
const caseDir =
Expand Down
1 change: 1 addition & 0 deletions workflows/review/lib/disciplines.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ describe("the shared disciplines section", () => {
},
},
}),
() => Promise.reject(new Error("unexpected ticket fetch")),
{repo: "o/r", prNumber: 1, repoRoot: "/work"},
);
const staged = files["/tmp/gh-aw/review/disciplines.md"];
Expand Down
Loading
Loading