Skip to content

review: bump installed reviewer to review-v1.7.0 #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 3 commits into from
Jul 21, 2026
Merged

review: bump installed reviewer to review-v1.7.0 #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d54d921
[jwies/review-bump-1.6.1] review: bump installed reviewer to review-v…
jwbron Jul 20, 2026
b97b384
[jwies/review-bump-1.6.1] review: guard the hand-merged install; spli…
jwbron Jul 21, 2026
e2857d8
[jwies/review-bump-1.6.1] review: bump the installed reviewer to revi…
jwbron Jul 21, 2026
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
87 changes: 87 additions & 0 deletions .github/workflows/review-pins.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
* utils/sync-workflow-versions.ts and its backstop in
* workflows/review/version-sync.test.ts; neither covers these files.)
*/
import {spawnSync} from "node:child_process";
import * as fs from "fs";
import * as os from "os";
import * as path from "path";
import {describe, expect, it} from "vitest";

const reviewMd = fs.readFileSync(
Expand Down Expand Up @@ -56,3 +59,87 @@ describe("compiled review.lock.yml pins", () => {
expect(new Set(literals)).toEqual(new Set([sourceRef]));
});
});

/**
* Content guard for the hand-merged install. `gh aw update` cannot resolve
* changesets-style tags (review-v*), so bumps of the installed copy are
* manual 3-way merges; the pins above check version consistency but nothing
* verified the merged CONTENT. This diffs the installed copy against the
* shared source at the pinned release (this repo hosts both) and requires
* every hunk to carry a `KHAN/ACTIONS LOCAL OVERRIDE` marker, so a manual
* bump that silently drops an override or an upstream hunk fails CI instead
* of surfacing in a live run. Convention enforced as a side effect: each
* override edit inserts its marker comment adjacent to the edited lines
* (within the diff hunk's context window).
*/
describe("installed review.md content vs the pinned source", () => {
const repoRoot = path.resolve(
new URL(".", import.meta.url).pathname,

@github-actions github-actions Bot Jul 21, 2026

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): new URL(".", import.meta.url).pathname is not a portable filesystem path (URL percent-encoding for e.g. spaces; a leading-slash-drive form on Windows), and it is the only .pathname-off-a-file-URL in the repo — this same file otherwise passes new URL("./review.md", import.meta.url) straight to fs (line 24). It works on Linux CI today. If you want the idiomatic form:

import {fileURLToPath} from "node:url";
const repoRoot = path.resolve(
    path.dirname(fileURLToPath(import.meta.url)),
    "../..",
);

"../..",
);
const sourcePath = "workflows/review/review.md";

const gitShow = (ref: string): string | null => {
const show = () =>
spawnSync("git", ["show", `${ref}:${sourcePath}`], {
cwd: repoRoot,
encoding: "utf-8",
maxBuffer: 32 * 1024 * 1024,
});
let result = show();
if (result.status !== 0) {
// A shallow or tag-less clone (CI checks out at depth 1): fetch
// just the pinned tag, then retry.
spawnSync(
"git",
["fetch", "--quiet", "--depth=1", "origin", "tag", ref],

@github-actions github-actions Bot Jul 21, 2026

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): This fetch looks like the steady-state CI path, not a rare fallback: node-ci.yml checks out with actions/checkout@v5 and no fetch-depth, so the default depth-1 shallow clone carries no tags and git show <tag>:... fails on essentially every run — making the network git fetch run each time (a flake surface for a unit test). Committing the pinned source as a snapshot instead of fetching the tag would make this hermetic.

{cwd: repoRoot, encoding: "utf-8"},
);
result = show();
}
return result.status === 0 ? result.stdout : null;
};

it("differs from the pinned release only inside LOCAL OVERRIDE hunks", () => {
expect(sourceRef).toBeDefined();
const source = gitShow(sourceRef as string);
if (source === null) {
throw new Error(
`cannot read ${sourcePath} at tag ${sourceRef}: fetch the ` +
`tag (git fetch origin tag ${sourceRef}) and re-run`,
);
}
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "review-pins-"));
try {
const sourceFile = path.join(dir, "source.md");
fs.writeFileSync(sourceFile, source);
const installedFile = path.join(dir, "installed.md");
fs.writeFileSync(installedFile, reviewMd);
const diff = spawnSync("diff", ["-u", sourceFile, installedFile], {
encoding: "utf-8",
maxBuffer: 32 * 1024 * 1024,
});
// 0: identical, 1: differences found, 2: trouble.
expect([0, 1]).toContain(diff.status);
const hunks: string[][] = [];
for (const line of diff.stdout.split("\n")) {
if (line.startsWith("@@")) {
hunks.push([line]);
} else {
hunks.at(-1)?.push(line);
}
}
const unmarked = hunks.filter(

@github-actions github-actions Bot Jul 21, 2026

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.

thought (non-blocking): The marker requirement is per-hunk, not per-edit: a hunk is exonerated if KHAN/ACTIONS LOCAL OVERRIDE appears anywhere in it, and diff -u folds edits within ~3 context lines into one hunk — so an unmarked edit adjacent to an existing marker (e.g. right after max-ai-credits: 2500) can ride through undetected. The docstring already discloses this, so it is a documented tradeoff. If you ever want the stronger "every divergence is individually justified" guarantee, committing the pinned source as a snapshot/patch and asserting byte-exactness would close it (and would also make the test hermetic — see the fetch note).

(hunk) =>
!hunk.some((line) =>
line.includes("KHAN/ACTIONS LOCAL OVERRIDE"),
),
);
expect(
unmarked.map((hunk) => hunk.slice(0, 8).join("\n")),
).toEqual([]);

@github-actions github-actions Bot Jul 21, 2026

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): The hunk-parse and marker-filter logic is asserted only against the real, currently-clean files (5 marked hunks today), so a future refactor that breaks the @@-split or the marker substring match would still pass green — and the next silently-dropped override would slip through the guard built to catch it. Consider a fixture-driven negative case that feeds synthetic diff -u output containing one marked and one unmarked hunk and asserts only the unmarked one survives, so the detection path itself is exercised.

Low-confidence (1)
  • .github/workflows/review-pins.test.ts:64 — Open question: could release automation publish a tag/alias gh aw update can resolve, retiring the bespoke guard plus the manual 3-way merge on every bump? Not a defect.

} finally {
fs.rmSync(dir, {recursive: true, force: true});
}
});
});
48 changes: 27 additions & 21 deletions .github/workflows/review.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading