review: stage the linked jira ticket; settled-decision rules for first-principles - #361
Conversation
…settled-decision rules for first-principles
🦋 Changeset detectedLatest commit: 017c0b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const key = extractIssueKey( | ||
| options.title, | ||
| options.headBranch, | ||
| options.description, |
There was a problem hiding this comment.
suggestion (non-blocking): Any issue key the PR author writes selects which internal ticket is fetched with the org token and fed to prompts that post publicly; no project allowlist or no-quoting rule bounds it. Introduced by this change: before it, the in-prompt Jira grant was dead text (I confirmed review.md's network.allowed is only defaults, github, *.sentry.io, so the agent never had Jira egress). I also grepped review.md for any instruction against quoting external context into posted comments and found none, and safe-outputs allowed-domains already permits khanacademy.atlassian.net links. The actor must be a collaborator, which caps severity, but the disclosure lands on a public PR page.
A sketch, not a committable replacement:
// stage-ticket.ts — only fetch keys whose project the consumer allowlisted
// (REVIEW_JIRA_PROJECTS repo variable), so an arbitrary key in author text
// cannot pull an unrelated internal ticket into a public review.
const inAllowedProject = (projects: string[], key: string): boolean =>
projects.length === 0 ? false : projects.includes(key.split("-")[0]);
review details
found by correctness-reviewer| change matches the stated rationale, repo convention, or the ticket's intent, you | ||
| have no finding: drop it rather than posting a hedge. | ||
|
|
||
| **One finding per premise.** Several observations hanging off the same underlying |
There was a problem hiding this comment.
thought (non-blocking): The premise 41609 falsified lives in the clusterer, not in the first-principles prompt. claim-clusterer explicitly instructs "Do NOT group two comments from the same source. A reviewer does not duplicate itself" (review.md:1779) — 41609 is a direct counterexample to that assumption, and this change works around it in one lens's prose instead of at the layer that owns dedup. Worth deciding whether the per-source grouping ban should be relaxed (or a cheap same-source merge added in code, where "every merge rule stays in code" per review.md:785) rather than re-litigated per prompt.
review details
found by first-principles
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…g in the eval arm Review feedback on the ticket-staging half: - REVIEW_JIRA_PROJECTS (repo variable) is now required alongside the other three values: the key regex alone matches UTF-8, SHA-256, and CVE-2024-1234, and an unbounded key also let any author-written key fetch an arbitrary internal ticket with the org token. Candidates outside the allowlist never block a real key. Credentials without the allowlist warn. - The description scan takes the LAST allowed match, not the first: the ~/bin/gh convention puts the tracking key at the end of the body, after any tickets the prose mentions (this PR's own body was the counterexample). - eval/live-stage.ts now writes ticket-context.json (default the unconfigured shape, overridable via a case's live.ticket block), so the A/B exercises the same staging contract as production. - ticketFetch is a required positional param of runStagePrCli like fs/ghGet/ghGraphql; stage-ticket.ts owns every degradation shape. - The CLI's Jira GET gets a hard 10s AbortSignal timeout. - A 200 with a non-object body degrades with a warning instead of relying on the catch; comment pagination envelopes (total > page) set truncated. - New tests: allowlist gating, last-in-description, half-configured warning, non-issue 200 bodies, envelope truncation, warning forwarding to the CLI. Prompt side: first-principles gets the same embedded-instruction reporting rule completeness carries, and the settled-decision rule defines rationale as checkable reasoning, not restated preference.
|
Pushed 651b86f addressing this round:
Not taken: the en-dash nitpick's direction. The three new lines now use colons rather than adding em dashes, resolving the inconsistency the other way. The clusterer observation is right that the same-source dedup premise lives in claim-clusterer, but relaxing that rule is a separate change with its own eval risk; tracked under PRA-43 rather than expanded here. |
The allowlist did two jobs and both have better owners. Extraction correctness: collect every key-shaped candidate (title, head branch, description, deduped, capped at 5) and fetch them all in parallel; noise like UTF-8/SHA-256/CVE-2024-1234 and stale keys just 404 and drop silently, so a bogus token can never block a real key and no denylist or per-source first/last rules are needed. Disclosure: the bound is the service account's own jira permissions, enforced server-side; the docs now say to grant it Browse Projects on only the projects reviews may quote. This also changes the staged shape: ticket-context.json carries a tickets array (every ticket that resolved, in candidate order), so a PR that references several tickets stages all of them. Degradations are unchanged in spirit: not-configured, no-issue-key, not-found (every candidate 404d), fetch-failed (a non-404 failure and nothing resolved). A failed candidate alongside a resolved one warns but still stages the survivors. Consumer config is back to three values (REVIEW_JIRA_BASE_URL, EMAIL, API_TOKEN); REVIEW_JIRA_PROJECTS is gone from the workflow env, README, and prompts. The completeness and first-principles prompts read the tickets array; the eval live.ticket block stages the new shape.
|
Direction change in 2be6b4e: the project allowlist from the last push is gone. It was doing two jobs, and both have better owners. Extraction correctness now comes from collecting every key-shaped candidate (title, branch, description; deduped, capped at 5) and fetching them all in parallel: noise like UTF-8/SHA-256/CVE-2024-1234 and stale keys just 404 and drop silently, so nothing can block a real key and no denylist or first/last ordering rules are needed. The disclosure bound is the service account's own jira permissions, enforced server-side; the docs now say to grant it Browse Projects on only the projects reviews may quote. This also means multi-ticket staging: |
There was a problem hiding this comment.
Changes requested — see inline comments.
13 of 15 prior review threads resolved; 2 still unaddressed as of 2be6b4e:
2 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/lib/stage-ticket.ts: Any issue key the PR author writes selects which internal ticket is fetched with the org token and fed to prompts that p... - thought (non-blocking)
workflows/review/review.md:2309: The premise 41609 falsified lives in the clusterer, not in the first-principles prompt.claim-clustererexplicitly ins...
Full finding
PR description: the test-plan sentence lists the same coverage twice. The description says "deterministic tests cover the key extraction, auth/URL shape, size caps, and every degradation path (candidate collection, multi-ticket staging, auth/URL shape, size caps, and every degradation path; 1901 total green)" — "auth/URL shape, size caps, and every degradation path" appears verbatim on both sides of the parenthesis, so the reader has to compare the two lists to find the only new items (candidate collection, multi-ticket staging). Collapsing to one list reads plainly: "deterministic tests cover candidate collection, multi-ticket staging, auth/URL shape, size caps, and every degradation path; 1901 total green."
review details
found by documentationreview details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| } | ||
| } | ||
| } | ||
| return keys.slice(0, MAX_TICKET_FETCHES); |
There was a problem hiding this comment.
issue (blocking): The blind pre-fetch cap contradicts the "noise can never block a real key" invariant this design rests on. The head commit dropped the project allowlist in favour of "try every candidate, let the misses 404" — an argument that only holds if every candidate is actually tried, but slice(0, MAX_TICKET_FETCHES) truncates before a single request goes out, and the invariant is nevertheless asserted verbatim in the module docstring (lines 20-23), README.md:803-806, and the changeset. I ran the real ISSUE_KEY_RE over this PR's own title/branch/description: it returns those five candidates with the real ticket in the last surviving slot. Cap the fetches, not the candidate list: sink well-known non-keys (or hoist keys appearing in a <baseUrl>/browse/KEY URL) before slicing.
A sketch, not a committable replacement:
/** Well-known key-shaped tokens that are never Jira issue keys. */
const NON_KEY_PREFIXES = new Set([
"UTF", "SHA", "MD", "CVE", "RFC", "ISO", "AES", "RSA", "HTTP", "IPV",
]);
// ...inside extractIssueKeys, replacing `return keys.slice(0, MAX_TICKET_FETCHES);`
// Cap the FETCHES, not the candidates: keep order of appearance, but sink
// the well-known non-keys so a noisy title/body can never crowd a real key
// out of the budget (the invariant this module's candidate stance claims).
const isNoise = (key: string): boolean =>
NON_KEY_PREFIXES.has(key.slice(0, key.indexOf("-")));
return keys
.map((key, index) => ({key, index}))
.sort((a, b) => Number(isNoise(a.key)) - Number(isNoise(b.key)) || a.index - b.index)
.slice(0, MAX_TICKET_FETCHES)
.map((candidate) => candidate.key);
Lower-confidence observations (2)
workflows/review/review.md:2307suggestion (non-blocking): The two halves of the diff treat ticket text incompatibly: untrusted data under review, and a premise-settling authority that silently kills findings. (holistic)workflows/review/review.md:2302thought (non-blocking): Both halves push first-principles toward deferring to author-stated rationale, and the ticket widens what counts as "stated". (first-principles)
review details
found by holistic | also flagged by skill-auditor (out-of-lane); completeness; documentation (at line 21): Header docblock's "can never block a real key" / "stages all of them" is contradicted by MAX_TICKET_FETCHES.| // 404 is normal candidate noise (key-shaped tokens, stale keys, | ||
| // tickets the service account cannot browse): dropped silently. | ||
| // 400 is the same noise in malformed-key form. | ||
| if (response.status === 404 || response.status === 400) { |
There was a problem hiding this comment.
suggestion (non-blocking): The 400-as-candidate-noise degradation path has no test. grep -n "status:" workflows/review/lib/stage-ticket.test.ts shows only 200, 404, and 401 (plus rejections); no test drives a 400, even though the PR description claims every degradation path is covered. One extra case alongside the existing all-404 assertion closes it.
A sketch, not a committable replacement:
// workflows/review/lib/stage-ticket.test.ts, in `describe("stageTicketContext")`
it("drops a 400 (malformed key) as silent candidate noise", async () => {
const {context, warnings} = await stageTicketContext(
() => Promise.resolve({status: 400, json: null}),
OPTIONS,
);
expect(context).toEqual({available: false, reason: "not-found"});
expect(warnings).toEqual([]);
});
review details
found by test-adequacy| !isRecord(rawTicket) || | ||
| typeof rawTicket["available"] !== "boolean" | ||
| ) { | ||
| errors.push( |
There was a problem hiding this comment.
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| auth: string, | ||
| key: string, | ||
| ): Promise<Attempt> => { | ||
| const url = `${baseUrl}/rest/api/2/issue/${key}?fields=${[ |
There was a problem hiding this comment.
suggestion (non-blocking, documentation): The deliberate choice of Jira REST v2 over v3 is unexplained. The added request hardcodes the older API version — const url = `${baseUrl}/rest/api/2/issue/${key}?fields=${[ — and nothing in the diff records why: the JiraIssue type just says "The fields this module reads from GET /rest/api/2/issue/{key}" and the test only asserts "against the v2 issue endpoint". Since the module's whole point is plain-text prompt input, that constraint is exactly what a comment should pin down.
A sketch, not a committable replacement:
// v2, not Jira Cloud's current v3: v2 returns `description` and comment
// bodies as plain text, while v3 returns Atlassian Document Format JSON,
// which is not usable as prompt input.
const url = `${baseUrl}/rest/api/2/issue/${key}?fields=${[
review details
found by documentation| /** Size caps: the staged file is prompt input, not an archive. */ | ||
| const DESCRIPTION_CAP = 8000; | ||
| const COMMENT_BODY_CAP = 2000; | ||
| const COMMENT_COUNT_CAP = 20; |
There was a problem hiding this comment.
question (non-blocking): The size caps are per-field only (8 000-char description, 20 comments × 2 000 chars each, per ticket) with no aggregate bound across the up-to-5 staged tickets, so ticket-context.json — which is prompt input for two high-effort Opus sub-agents on every run — can reach roughly a quarter of a megabyte.
review details
found by skill-auditor (out-of-lane)| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| REVIEW_PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} | ||
| REVIEW_JIRA_BASE_URL: ${{ vars.REVIEW_JIRA_BASE_URL }} |
There was a problem hiding this comment.
question (non-blocking): The input-side half ships inert unless a consumer provisions Jira credentials — the same shape as the dead grant it replaces. The PR's own diagnosis of the old completeness grant is "no consumer ever provided the token it promised"; grepping the repo, REVIEW_JIRA_* appears only in the files this PR touches, and the fetch is gated on all three values being non-empty (stage-ticket.ts:310). The new code is strictly better than dead prompt text — it degrades deterministically and is tested — but the binding constraint on whether first-principles ever reads a real ticket is a service account with scoped Browse Projects, which is human provisioning work outside this diff. Worth saying in the PR (or the changeset) whether that provisioning is landed/tracked, since without it the observed problem is addressed by the two prompt rules alone and the staging is speculative infrastructure.
review details
found by first-principles| * without fetching. The ticket staging itself is exercised in | ||
| * stage-ticket.test.ts. | ||
| */ | ||
| const noTicket = () => (): Promise<{status: number; json: unknown}> => |
There was a problem hiding this comment.
suggestion (non-blocking): Every other injected-dependency fake in this file names its contract type — const ghGetFromMap = (routes: Record<string, unknown>): GhGet => (line 56) and const noThreads = (): GhGraphql => () => (line 77), both importing the type at the top — and the sibling test added in this same PR does too (const okFetch = (json: unknown): TicketFetch =>, const spyFetch: TicketFetch = ... in stage-ticket.test.ts). The new helper instead writes const noTicket = () => (): Promise<{status: number; json: unknown}> =>, restating TicketFetch's shape inline. This is readability/consistency rather than a type hole — the fake is passed to runStagePrCli, whose parameter is typed TicketFetch (stage-pr.ts:375), so it is still checked against the contract at the call site — but naming the type makes the fake self-describing and matches its two neighbours.
A sketch, not a committable replacement:
import {
buildUnifiedDiff,
computeDiffFingerprint,
computeNewScope,
hashHunkAddedLines,
runStagePrCli,
type GhGet,
type StagePrFs,
} from "./stage-pr";
import type {TicketFetch} from "./stage-ticket";
// ...
const noTicket = (): TicketFetch => () =>
Promise.reject(new Error("unexpected ticket fetch"));
review details
found by conventions| - `pr-context.json` — the PR metadata (number, title, description, author, | ||
| `baseBranch`, `headSha`, `isDraft`, `repo`). The one authoritative PR-level | ||
| context surface: you and every sub-agent read PR metadata from here. | ||
| - `ticket-context.json`: the linked Jira tickets (every issue key the PR |
There was a problem hiding this comment.
nitpick (non-blocking): Every other entry in this staged-input list uses `name` + em dash — - \pr-context.json` — the PR metadata (number, title, ...), - `files.json` — each changed file's `path`..., - `full.diff` — the standard unified diff...— while the new one reads- `ticket-context.json`: the linked Jira tickets (every issue key the PR. The same swap appears in README.md's Optional config list (- `REVIEW_JIRA_BASE_URL` (repo variable), ... : the linked-ticket staging) whose siblings all use the em dash (- `REVIEW_BOT_LOGIN` — the account this workflow posts reviews as, - `ANTHROPIC_API_KEY` — used by the `claude` engine.`).
review details
found by conventions…low the fetch cap; close test gaps The cap contradicted the docstring: slice(0, MAX_TICKET_FETCHES) truncated candidates before any fetch, so 5 noise tokens (UTF-8, SHA-256, ...) ahead of a real key silently dropped it while the docs claimed that can't happen (this PR's own title/branch/body put the real key in the last surviving slot). Known non-key prefixes now sort behind plausible keys before the cap applies, and the docstring, readme, review.md, and changeset state the actual bound. Also from review: a 400-as-candidate-noise test, a parseLive live.ticket test (the parse-to-stage chain was never exercised), a comment pinning the v2-over-v3 API choice (v3 returns ADF JSON, useless as prompt input), and noTicket typed as TicketFetch like its sibling fakes.
|
Holding for human review — the automated review could not complete safely this run. 4 non-blocking threads still open (1 previously reported)
|
jeresig
left a comment
jeresig
left a comment
There was a problem hiding this comment.
Interesting - so we'll also need to have the REVIEW_JIRA_* tokens be included in the repo secrets as well, it seems?
correct, I'll have IT mint that for us and add it to consuming repos. |
🖍 _This is an audit!_ 🖍 This repo's installed reviewer was on review-v1.17.1; 1.18.0 is cut and carries the prose judge (#362), whose rollout wants a week of live artifacts to watch the judge's error rate, so let's just move our own install first. Also in the hop: shared-anchor grounding with casing-folded salient tokens (#363), staged jira ticket context (#361, optional config we don't set here; staging degrades to available:false), and the turn-cap/timeout raises (#369). Mechanics, per the hand-merge flow (not `gh aw update`): 3-way merge of the installed review.md from 1.17.1 to 1.18.0, one conflict where the new lib-checkout ref landed against the LOCAL OVERRIDE comment, resolved by keeping the override and taking the new ref; `source:` and the checkout `ref:` moved to review-v1.18.0 together; recompiled with gh-aw v0.85.4 (same compiler as the current lock); the compile's merge=ours strip on .gitattributes reverted. All 6 KHAN/ACTIONS LOCAL OVERRIDE blocks survived, and review-pins.test.ts is green locally (5 tests, including the content guard diffing the install against the pinned source). [KORE-2512](https://khanacademy.atlassian.net/browse/KORE-2512) [KORE-2512]: https://khanacademy.atlassian.net/browse/KORE-2512?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: jwbron Auditors: jeresig, jaredly Required Reviewers: Approved By: Checks: ⌛ 1 check is pending, ⏭️ 8 checks have been skipped, ✅ 1 check was successful, ❌ 1 check failed Pull Request URL: #373
3 participants
Two problems from the webapp#41609 feedback (the run that drew "raising the exact purpose of this whole PR as a problem"), one input-side and one prompt-side.
Input side: no lens can actually read the linked ticket. The completeness prompt granted itself a jira/confluence read, but the grant was dead text; no consumer ever provided the token it promised, and the firewall egress never included the atlassian host (the
khanacademy.atlassian.netentry in the lock'sGH_AW_ALLOWED_DOMAINSis link-sanitization only). So its "fall back to the PR description" clause fired on every run, and first-principles, whose whole mandate is the stated rationale, only ever saw the author's summary of the decision it was questioning.So let's just stage the tickets deterministically like every other input: new
lib/stage-ticket.ts, called fromstage-pr.ts, collects every issue key the PR references (title, head branch, description; deduped, capped at 5), fetches them read-only in parallel on the host before the agent starts, and writes the ones that resolve toticket-context.jsonas aticketsarray. Consumer config is optional (REVIEW_JIRA_BASE_URLvariable plusREVIEW_JIRA_EMAIL/REVIEW_JIRA_API_TOKENsecrets). Key-shaped noise (UTF-8/SHA-256/CVE-2024-1234 all match the regex) and stale keys 404 and drop silently, so a bogus token never blocks a real key; the disclosure bound is the service account's own jira permissions, enforced server-side (grant it Browse Projects on only the projects reviews may quote). Every degradation (unconfigured, no key, all candidates 404, fetch failure with nothing resolved) stages{available: false, reason}and never fails the run. The agent sandbox keeps zero jira egress and never sees the credentials. Completeness's network grant is replaced with the staged read; first-principles gets the same file.Prompt side, two rules for first-principles minted from the 41609 comments:
Test plan: deterministic tests cover candidate collection, multi-ticket staging, auth/URL shape, size caps, and every degradation path (1903 total green), plus lint and typecheck; the per-PR smoke A/B covers the prompt delta. The 41609 eval-case minting is tracked separately (PRA-43's done-when).
KORE-2510