Skip to content

[🔥AUDIT🔥] review: bump installed reviewer to review-v1.18.0 - #373

Merged
jwbron merged 1 commit into
mainfrom
jwbron/bump-review-v1.18.0
Aug 24, 2026
Merged

[🔥AUDIT🔥] review: bump installed reviewer to review-v1.18.0#373
jwbron merged 1 commit into
mainfrom
jwbron/bump-review-v1.18.0

Conversation

@jwbron

@jwbron jwbron commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🖍 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

…w-v1.18.0

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)
@jwbron jwbron added the audit label Aug 24, 2026
@changeset-bot

changeset-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 101b3b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@khan-actions-bot
khan-actions-bot requested review from a team, jaredly and jeresig and removed request for a team August 24, 2026 16:53
@jwbron
jwbron merged commit 894fbfc into main Aug 24, 2026
13 of 14 checks passed
@jwbron
jwbron deleted the jwbron/bump-review-v1.18.0 branch August 24, 2026 16:58
@github-actions

Copy link
Copy Markdown
Contributor

Guidance for reviewers

Triage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review.

github-actions (1 file)
File Reason
review.md This repo's own reviewer workflow: the diff moves the pinned ref: of the lib the reviewer executes, the agent timeout and Bash ceiling, and wires in two new secrets, so a wrong pin or a mis-scoped secret changes what code reviews every PR here.

Common patterns

2 files: Version pin bumped from review-v1.17.1 to review-v1.18.0 everywhere it appears (ref, source:, GH_AW_WORKFLOW_SOURCE/URL env vars, comments).

- ref: review-v1.17.1
+ ref: review-v1.18.0

2 files: Timeout and Bash ceiling raised: timeout-minutes 40 to 50 and BASH_MAX_TIMEOUT_MS 1200000 to 1800000, in both the compiled lock and the source.

-    BASH_MAX_TIMEOUT_MS: "1200000"
-timeout-minutes: 40
+    BASH_MAX_TIMEOUT_MS: "1800000"
+timeout-minutes: 50

2 files: REVIEW_JIRA_API_TOKEN and REVIEW_JIRA_EMAIL wired in as secrets and env vars for staged Jira ticket context, in both the compiled lock and the source.

-          GH_AW_SECRET_NAMES: '...KHAN_ACTIONS_BOT_TOKEN'
+          GH_AW_SECRET_NAMES: '...KHAN_ACTIONS_BOT_TOKEN,REVIEW_JIRA_API_TOKEN,REVIEW_JIRA_EMAIL'
+          REVIEW_JIRA_API_TOKEN: ${{ secrets.REVIEW_JIRA_API_TOKEN }}
+          REVIEW_JIRA_EMAIL: ${{ secrets.REVIEW_JIRA_EMAIL }}
Excluded from review (1 file)

Not individually reviewed — generated, formatting-only, or
fully explained by a common pattern above:

  • .github/workflows/review.lock.yml — generated
review details review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation

@khan-actions-bot
khan-actions-bot requested a review from a team August 24, 2026 17:15

@github-actions github-actions Bot left a comment

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.

review details review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation

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 }}
REVIEW_JIRA_EMAIL: ${{ secrets.REVIEW_JIRA_EMAIL }}

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.

note (non-blocking): The other places this install diverges for being public say so in a KHAN/ACTIONS LOCAL OVERRIDE comment (the roles: all note at line 21, the fork guard at line 31), but the new REVIEW_JIRA_* wiring carries only the upstream comment, which bounds disclosure to the service account's Jira permissions and never mentions that reviews here post publicly. Nothing is exposed today — stage-ticket.ts stages {available: false, reason: "not-configured"} while the three values are empty — so this is a note for whoever configures them later, not a defect introduced by this change.

Lower-confidence observations (2)
  • .github/workflows/review.md:834 suggestion (non-blocking): The prose judge adds one cold session per finding, but max-turn-cache-misses: 25 is still sized to the agent roster. (holistic)
  • .github/workflows/review.md:210 question (non-blocking): The run's wall-clock budget is raised (BASH_MAX_TIMEOUT_MS 1200000 -> 1800000, timeout-minutes 40 -> 50, and the Step 2 dispatcher timeout to 1800000) while the repo's local max-ai-credits: 2500 / REVIEW_MAX_AI_CREDITS: "2500" ceiling at lines 489/508 is left unchanged, so wall-clock stops being the binding limit on a long run and credits become it. (skill-auditor (out-of-lane))
review details found by correctness-reviewer

BASH_DEFAULT_TIMEOUT_MS: "60000"
BASH_MAX_TIMEOUT_MS: "1200000"
timeout-minutes: 40
BASH_MAX_TIMEOUT_MS: "1800000"

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): No test couples the compiled Bash ceiling in review.lock.yml to review.md's frontmatter. grep -rn BASH_MAX_TIMEOUT_MS --include=*.ts matches nothing outside node_modules, and review-pins.test.ts checks only review-v* literals in the lock, so nothing ties the compiled ceiling back to the frontmatter the way cache-miss-guard.test.ts ties maxCacheMisses. The values are correct in this PR (lock lines 1013 and 1630 both read 1800000); this is the missing backstop for the next hand-merge, and CI runs no gh aw compile --check.

A sketch, not a committable replacement:

// .github/workflows/review-pins.test.ts, alongside the existing lock-pins describe:
describe("compiled review.lock.yml engine config", () => {
    it("carries this review.md's Bash ceiling on every engine step", () => {
        const declared = reviewMd.match(
            /^\s*BASH_MAX_TIMEOUT_MS:\s*"(\d+)"\s*$/m,
        )?.[1];
        expect(declared).toBeDefined();
        const baked = [
            ...reviewLock.matchAll(/^\s*BASH_MAX_TIMEOUT_MS:\s*(\d+)\s*$/gm),
        ].map((m) => m[1]);
        expect(baked.length).toBeGreaterThan(0);
        expect(new Set(baked)).toEqual(new Set([declared]));
    });
});
review details found by test-adequacy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant