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
49 changes: 49 additions & 0 deletions .github/workflows/python-quality.yml
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,46 @@ jobs:
shell: pwsh
run: "& plugins/capability/darrow-git/tests/fresh-install.test.ps1"

review-windows:
name: Review Python ${{ matrix.python-version }} on windows-latest
needs: changes
if: contains(needs.changes.outputs.packages, 'plugins/capability/darrow-review/backend')
strategy:
fail-fast: false
matrix:
python-version: ["3.10", "3.11", "3.12", "3.13"]
runs-on: windows-latest
defaults:
run:
shell: bash
env:
UV_PYTHON: ${{ matrix.python-version }}
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- uses: astral-sh/setup-uv@v6
with:
enable-cache: true
- run: scripts/check-python --package plugins/capability/darrow-review/backend

review-fresh-install:
name: Fresh install review on ${{ matrix.os }}
needs: changes
if: contains(needs.changes.outputs.packages, 'plugins/capability/darrow-review/backend')
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.13"
- uses: astral-sh/setup-uv@v6
- run: uv run --quiet --frozen --no-dev --project plugins/capability/darrow-review/backend python plugins/capability/darrow-review/backend/tests/fresh_install.py
tickets-windows:
name: Tickets Python ${{ matrix.python-version }} on windows-latest
needs: changes
Expand Down Expand Up @@ -462,6 +502,8 @@ jobs:
- verification-windows
- git-windows
- git-fresh-install
- review-windows
- review-fresh-install
- tickets-windows
- tickets-fresh-install
- decisions-windows
Expand Down Expand Up @@ -495,6 +537,9 @@ jobs:
DISCOVERY_INSTALL_RESULT: ${{ needs.discovery-fresh-install.result }}
VERIFICATION_INSTALL_RESULT: ${{ needs.verification-fresh-install.result }}
PERFORMANCE_RESULT: ${{ needs.performance.result }}
REVIEW_SELECTED: ${{ contains(needs.changes.outputs.packages, 'plugins/capability/darrow-review/backend') }}
REVIEW_WINDOWS_RESULT: ${{ needs.review-windows.result }}
REVIEW_INSTALL_RESULT: ${{ needs.review-fresh-install.result }}
GIT_SELECTED: ${{ contains(needs.changes.outputs.packages, 'plugins/capability/darrow-git/backend') }}
GIT_WINDOWS_RESULT: ${{ needs.git-windows.result }}
GIT_INSTALL_RESULT: ${{ needs.git-fresh-install.result }}
Expand Down Expand Up @@ -534,3 +579,7 @@ jobs:
if [ "$IA_SELECTED" = true ]; then expected=success; fi
test "$IA_WINDOWS_RESULT" = "$expected"
test "$IA_INSTALL_RESULT" = "$expected"
expected=skipped
if [ "$REVIEW_SELECTED" = true ]; then expected=success; fi
test "$REVIEW_WINDOWS_RESULT" = "$expected"
test "$REVIEW_INSTALL_RESULT" = "$expected"
26 changes: 20 additions & 6 deletions docs/specs/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -392,12 +392,26 @@ the prior-to-current repair delta remains nonempty and exact-target-bound.
command or manifest, its own source material, a strict finding schema, and a
concise output budget. It MUST NOT receive the other reviewer's analysis or
an unrelated conversation transcript.
5. **CR-P5 — Mechanics in scripts.** Fixed-point validation, diff-scope
construction, and structured-result validation belong in bundled portable
scripts. Review judgment remains in `SKILL.md` and small references.
6. **CR-P6 — Portable scripts.** Bundled shell mechanics follow Darrow's Bash
3.2 and Bash 5 requirements, preserve pre-existing changes, and emit
absolute model-facing paths where paths are needed.
5. **CR-P5 — Packaged mechanics.** Fixed-point validation, diff-scope
construction, reviewer routing, provider observation, check capture, and
structured-result validation and rendering belong in one self-contained
Python + UV package. Invoke its public commands through frozen resolution;
do not retain Bash/AWK runtime aliases. Review judgment remains in
`SKILL.md` and small references.
6. **CR-P6 — Portable mechanics.** The package supports Python 3.10–3.13 on
Linux, macOS, and native Windows, preserves pre-existing changes, and emits
absolute model-facing paths. Use argument-vector subprocesses, explicit
UTF-8 records, native temporary files, and cancellation that terminates
owned subprocesses. The deliberately literal `review-check --command`
boundary uses the host shell (Bash on Unix, PowerShell on native Windows);
all other commands execute without shell interpolation. Preserve public
command names, TSV formats, scope/repair binding, diagnostics, and exit
codes. Register the locked package in the Python inventory, enforce the
repository's strict quality gates, and exercise copied runtime-only plugins
on all three native platforms with provider boundaries controlled. Keep
deterministic plugin regression tests in the Python package; do not retain
shell test wrappers after their contract coverage has migrated. Tests of
the shared eval runner remain with that runner.
7. **CR-P7 — Portable composition.** Consumers request independent code review
by intent and interpret its reported findings and outcome; neither side needs
a sibling plugin path, implementation name, output schema, tracker, or
Expand Down
90 changes: 56 additions & 34 deletions evals/runner/goal-review-fixture.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,50 @@ const source = new URL(
"../../plugins/orchestration/darrow-adaptive-delivery/skills/adaptive-delivery/evals/high-risk-routine.yaml",
import.meta.url,
);
const validatorSource = new URL(
"../../plugins/capability/darrow-review/bin/review-result",
const backendSource = new URL(
"../../plugins/capability/darrow-review/backend/",
import.meta.url,
);
const proofSource = new URL("fixtures/require-clear-review.sh", source);
const scopeSource = new URL("review-scope", validatorSource);
const reportSource = new URL("review-report", validatorSource);

async function reviewFiles(prefix = ".git/review-plugin/backend") {
const files: Record<string, string> = {};
for (const name of ["pyproject.toml", "uv.lock"])
files[`${prefix}/${name}`] = await Bun.file(
new URL(name, backendSource),
).text();
const glob = new Bun.Glob("src/darrow_review/*.{py,typed}");
for await (const name of glob.scan(backendSource.pathname))
files[`${prefix}/${name}`] = await Bun.file(
new URL(name, backendSource),
).text();
return files;
}

async function duplicateReview(repo: string, conflict?: boolean) {
const copied = await reviewFiles(".git/plugin-cache/backend");
for (const [path, text] of Object.entries(copied))
await Bun.write(`${repo}/${path}`, text);
if (conflict)
await Bun.write(
`${repo}/.git/plugin-cache/backend/src/darrow_review/scope.py`,
"raise RuntimeError('conflicting copy')\n",
);
}

function reviewCommand(command: string, ...args: string[]) {
return [
"uv",
"run",
"--quiet",
"--frozen",
"--no-dev",
"--project",
".git/review-plugin/backend",
command,
...args,
];
}

test("high-risk composition requires verification and supporting review reads", async () => {
const evalCase = parse(await Bun.file(source).text()) as EvalCase;
Expand Down Expand Up @@ -79,12 +116,11 @@ async function selectedArtifact(
if (!scenario.markdown) return record;
const verification = record.endsWith("verification.tsv");
const rendered = Bun.spawnSync(
[
"/bin/bash",
".git/review-plugin/bin/review-report",
reviewCommand(
"review-report",
verification ? "render-verification" : "render",
record,
],
),
{ cwd: repo },
);
expect(rendered.exitCode).toBe(0);
Expand Down Expand Up @@ -219,10 +255,7 @@ for (const scenario of [
files: {
".git/darrow-review.fixture/result.tsv":
records.map((row) => row.join("\t")).join("\n") + "\n",
".git/review-plugin/bin/review-result":
await Bun.file(validatorSource).text(),
".git/review-plugin/bin/review-report":
await Bun.file(reportSource).text(),
...(await reviewFiles()),
".git/fixture-bin/review-proof": await Bun.file(proofSource).text(),
},
},
Expand Down Expand Up @@ -303,23 +336,23 @@ for (const scenario of [
fixture: {
commits: [{ message: "Initial", files: { "value.txt": "before\n" } }],
files: {
".git/review-plugin/bin/review-result":
await Bun.file(validatorSource).text(),
".git/review-plugin/bin/review-scope":
await Bun.file(scopeSource).text(),
".git/review-plugin/bin/review-report":
await Bun.file(reportSource).text(),
...(await reviewFiles()),
".git/fixture-bin/review-proof": await Bun.file(proofSource).text(),
},
},
skillDir: "",
skillMounts: [],
});
const run = (...args: string[]) =>
Bun.spawnSync(["/bin/bash", ...args], { cwd: repo });
Bun.spawnSync(
args[0]!.startsWith("review-")
? reviewCommand(args[0]!, ...args.slice(1))
: ["/bin/bash", ...args],
{ cwd: repo },
);
const scope = () => {
const result = run(
".git/review-plugin/bin/review-scope",
"review-scope",
"prepare",
"--repo",
repo,
Expand All @@ -335,14 +368,7 @@ for (const scenario of [
rows.map((row) => row.join("\t")).join("\n") + "\n";
try {
if (scenario.duplicate) {
await Bun.write(
`${repo}/.git/plugin-cache/bin/review-result`,
await Bun.file(validatorSource).text(),
);
await Bun.write(
`${repo}/.git/plugin-cache/bin/review-scope`,
scenario.conflict ? "exit 1\n" : await Bun.file(scopeSource).text(),
);
await duplicateReview(repo, scenario.conflict);
}
await Bun.write(`${repo}/value.txt`, "incorrect change\n");
const original = scope();
Expand Down Expand Up @@ -428,11 +454,7 @@ for (const scenario of [
// Every negative is a valid public artifact: rejection must be the gate's
// outcome, current-content, or original-evidence check, not bad test TSV.
expect(
run(
".git/review-plugin/bin/review-result",
"validate-verification",
record,
).exitCode,
run("review-result", "validate-verification", record).exitCode,
).toBe(0);
if (scenario.stale)
await Bun.write(`${repo}/value.txt`, "changed after review\n");
Expand Down Expand Up @@ -460,5 +482,5 @@ for (const scenario of [
} finally {
await destroyFixture(repo);
}
});
}, 30_000);
}
73 changes: 65 additions & 8 deletions evals/runner/review-outcome-eval-checks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,57 @@ import {
import { tmpdir } from "node:os";
import { join, resolve } from "node:path";
import { parse as parseYaml } from "yaml";
import { buildFixture, destroyFixture } from "./fixture";
import type { EvalCase } from "./types";

const plugin = resolve(
import.meta.dir,
"../../plugins/capability/darrow-review",
);
const roots: string[] = [];

for (const name of [
"fix-verification-resolved",
"fix-verification-progress-advisory",
"fix-verification-regression-scope",
"fix-verification-regression-second-round",
"fix-verification-unavailable",
"goal-contract-repair-rereview",
"repair-guidance-alternative",
"repair-guidance-unresolved",
]) {
test(`review package fixture setup: ${name}`, async () => {
const caseDir = join(plugin, "skills/code-review/evals");
const evalCase = parseYaml(
await readFile(join(caseDir, `${name}.yaml`), "utf8"),
) as EvalCase;
const repo = await buildFixture({
fixture: evalCase.fixture,
skillDir: "",
skillMounts: [],
caseDir,
});
try {
if (name === "goal-contract-repair-rereview") {
expect(
(await readFile(join(repo, ".git/review-backend"), "utf8")).trim(),
).toBe(join(plugin, "backend"));
} else {
const input = await readFile(
join(repo, ".git/verification-input"),
"utf8",
);
const manifest = input.match(/^prior_manifest\t(.+)$/m)?.[1];
expect(manifest).toBeTruthy();
expect(await readFile(manifest!, "utf8")).toContain(
`repository\t${repo}`,
);
}
} finally {
await destroyFixture(repo);
}
}, 30_000);
}
afterEach(async () => {
await Promise.all(
roots.splice(0).map((root) => rm(root, { recursive: true })),
Expand All @@ -31,10 +75,14 @@ async function fixture() {
);
roots.push(root);
const artifacts = join(root, ".git/darrow-review.fixture");
const bin = join(root, ".git/eval-tools/bin");
const backend = join(root, ".git/eval-tools/backend");
await mkdir(artifacts, { recursive: true });
await cp(join(plugin, "bin"), bin, { recursive: true });
return { root, artifacts, bin };
await mkdir(backend, { recursive: true });
for (const name of ["src", "pyproject.toml", "uv.lock"])
await cp(join(plugin, "backend", name), join(backend, name), {
recursive: true,
});
return { root, artifacts, backend };
}

async function gate(root: string, file: string, name: string, shell: string) {
Expand Down Expand Up @@ -93,13 +141,22 @@ function verification(
async function render(
setup: Awaited<ReturnType<typeof fixture>>,
record: string,
shell: string,
) {
const path = join(setup.artifacts, "verification.tsv");
await writeFile(path, record);
const rendered = spawnSync(
shell,
[join(setup.bin, "review-report"), "render-verification", path],
"uv",
[
"run",
"--quiet",
"--frozen",
"--no-dev",
"--project",
setup.backend,
"review-report",
"render-verification",
path,
],
{ encoding: "utf8" },
);
expect(rendered.status, rendered.stderr).toBe(0);
Expand Down Expand Up @@ -153,7 +210,7 @@ for (const shell of ["bash", "/bin/bash"]) {
for (const resolved of [true, false]) {
test(`resolved case ${resolved ? "accepts all resolved states" : "rejects an unresolved advisory despite misleading prose"}`, async () => {
const setup = await fixture();
await render(setup, verification(resolved), shell);
await render(setup, verification(resolved));
expect(
await gate(
setup.root,
Expand Down Expand Up @@ -195,7 +252,7 @@ for (const shell of ["bash", "/bin/bash"]) {
`previous_verification\tprior-checksum\t${previous}\n`,
);
}
const output = await render(setup, verification(), shell);
const output = await render(setup, verification());
if (variant === "matching summaries" || variant === "empty reports") {
const summary =
variant === "empty reports"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "darrow-review",
"description": "Read-only comprehensive code review and fix-scoped repair verification",
"version": "0.4.0",
"version": "0.5.1",
"license": "BUSL-1.1",
"author": {
"name": "Björn Rochel",
Expand Down
2 changes: 1 addition & 1 deletion plugins/capability/darrow-review/.codex-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "darrow-review",
"version": "0.4.0",
"version": "0.5.1",
"description": "Read-only comprehensive code review and fix-scoped repair verification",
"author": {
"name": "Björn Rochel",
Expand Down
Loading
Loading