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
10 changes: 9 additions & 1 deletion test/pr-review-advisor-context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
const synthesisTurn = turns.find((turn) => turn.name === "synthesize-json");
const validationTurn = turns.find((turn) => turn.name === "validate-synthesis-json");
const expectedAnalysis = [
["scope-risk-map-analysis", 8, ["pr_review_scope_risk_context", "pr_review_git_diff"]],
["scope-risk-map-analysis", 12, ["pr_review_scope_risk_context", "pr_review_git_diff"]],
[
"terminology-review-analysis",
8,
Expand Down Expand Up @@ -143,6 +143,14 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
expect(analysisTurns[1]?.prompt).toContain("what concrete contrasting case");
expect(analysisTurns[1]?.prompt).toContain("pr_review_trace_term");
expect(analysisTurns[2]?.prompt).toContain("trusted code change considerations");
expect(analysisTurns[0]?.prompt).toContain("direct change in the current design");
expect(analysisTurns[0]?.prompt).toContain("neutral or negative net lines");
expect(analysisTurns[0]?.prompt).toContain("new pattern applied to current related code");
expect(analysisTurns[0]?.prompt).toContain(
"Report all currently visible, evidence-backed recommendations",
);
expect(analysisTurns[0]?.prompt).toContain("rescan the resulting design");
expect(analysisTurns[0]?.prompt).toContain("single ledger batch");
expect(analysisTurns[3]?.prompt).toContain("sandbox escape");
expect(analysisTurns[4]?.prompt).toContain("every riskPlan invariant");
expect(analysisTurns[4]?.prompt).toContain("inputs for e2e.coverage");
Expand Down
80 changes: 80 additions & 0 deletions test/pr-review-advisor-rendering.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,86 @@ describe("PR review advisor", () => {
expect(comment.match(/`PRA-1`/g)).toHaveLength(1);
});

it("keeps blocker evidence separate from brief refactoring guidance", () => {
const result = normalizeReviewResult(
validResult({
findings: [
{
severity: "blocker",
category: "architecture",
file: "src/lib/example.ts",
line: 24,
title: "Keep one cleanup result path",
description: "Cleanup failure has four state representations.",
impact: "The removal order is harder to verify.",
recommendation: "Preserve one fail-closed result path before removal.",
verificationHint: "Inspect the cleanup result at the removal boundary.",
missingRegressionTest: "The existing cleanup ordering test covers the boundary.",
evidence: "The diff adds a flag, exception, catch branch, and outer result.",
simplification: {
tag: "shrink",
cut: "the flag, exception, helper, catch branch, and repeated completion call",
replacement: "the existing cleanup result path",
estimatedNetLines: -31,
safetyBoundary: "Preserve failure ordering and authority-drift coverage.",
},
},
],
}),
metadata(),
);

const comment = buildComment({ summary: renderSummary(result), result });
const blockers = comment.indexOf("### Blockers");
const refactoring = comment.indexOf("### Recommended refactoring");

expect(blockers).toBeGreaterThan(-1);
expect(refactoring).toBeGreaterThan(blockers);
expect(comment.slice(blockers, refactoring)).not.toContain("Simplification (shrink)");
expect(comment).toContain(
"- **`PRA-1`:** Remove the flag, exception, helper, catch branch, and repeated completion call; use the existing cleanup result path. Net: -31 lines. Keep: Preserve failure ordering and authority-drift coverage.",
);
expect(comment).toContain(
"_Implementation guidance; a fix with equal or lower complexity is acceptable._",
);
expect(comment.match(/`PRA-1`/g)).toHaveLength(2);
});

it("keeps refactoring guidance within the visible blocker cap", () => {
const result = normalizeReviewResult(
validResult({
findings: Array.from({ length: 21 }, (_, index) => ({
severity: "blocker",
category: "architecture",
file: "src/lib/example.ts",
line: index + 1,
title: `Blocker ${index + 1}`,
description: "The changed path has an unresolved design issue.",
impact: "The implementation remains harder to maintain.",
recommendation: "Use the existing result path.",
...(index === 20
? {
simplification: {
tag: "shrink",
cut: "the extra state path",
replacement: "the existing result path",
estimatedNetLines: -12,
safetyBoundary: "Preserve cleanup ordering.",
},
}
: {}),
})),
}),
metadata(),
);

const comment = buildComment({ summary: renderSummary(result), result });

expect(comment).toContain("#### `PRA-20` Blocker — Blocker 20");
expect(comment).not.toContain("`PRA-21`");
expect(comment).not.toContain("### Recommended refactoring");
});

it("keeps warning-only reviews non-blocking without synthetic test tasks", () => {
const result = normalizeReviewResult(
validResult({
Expand Down
9 changes: 6 additions & 3 deletions tools/pr-review-advisor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ It complements the existing PR surfaces by keeping a NemoClaw maintainer code-re
- codebase drift and architecture review grounded in current behavior and contracts;
- source-of-truth review for fallback, recovery, tolerant parsing, monkeypatching, and other localized workaround behavior;
- static test-inventory context from changed test files and nearby test names;
- simplification review for safe delete/stdlib/native/YAGNI/shrink opportunities;
- a complete simplicity sweep that considers the changed code and its surrounding area, including
safe deletion, consolidation, existing or new patterns, and neutral or negative net-line outcomes;
- semantic terminology review for terms that changed explanatory text introduces, expands, or
redefines, with repository evidence for each model-selected candidate;
- E2E coverage, job, target, and fan-out selections normalized against the checked-in
Expand All @@ -40,7 +41,7 @@ It intentionally does not report GitHub mergeability, branch protection, CI stat
7. Creates a sandbox from a digest-pinned Pi image under a no-egress, hard-Landlock policy. The trusted advisor checkout, PR workspace, prepared GitHub context, and verified search binaries enter through advisor-only read-only Docker bind mounts before the first sandbox process starts. A capped tmpfs is the only writable application-data subtree. Before model code runs, a trusted probe reads every input canary, resolves the mounted checkout and `HEAD` through an explicit `GIT_DIR` and `GIT_WORK_TREE`, verifies that chmod, overwrite, replacement, and creation fail in every input, and exercises the complete runtime write lifecycle.
8. Runs the trusted `tools/pr-review-advisor/run-analysis.mts` entrypoint inside the sandbox. The unchanged multi-turn Pi SDK session reaches the host-configured model only through `https://inference.local/v1`; the sandbox receives an inert SDK key and neither the upstream model credential nor a GitHub token.
9. Runs the same advisor conversation in parallel for the primary GPT-5.6 Terra lane and an artifact-only Nemotron Ultra evaluation lane.
10. Opens one Pi session per model variant and reviews the PR in 16 bounded turns: seven analysis/commit pairs for scope/risk, terminology, correctness/state, security/trust, tests/regressions, CI/operations, and reconciliation, followed by draft and validation JSON synthesis turns in that same session. The terminology turn selects candidates semantically from changed explanatory text. Trusted code then traces each selected term across the base and head commits, including hyphen and space variants, changed source locations, and available history. Changed-location tracing streams the complete Git diff with bounded per-line memory, so selected terms remain traceable beyond 4 MiB of diff output. Repository occurrence counts and samples still use bounded `git grep` output and report when evidence is truncated. The tests/regressions turn analyzes E2E coverage and new-test gaps, while the CI/operations turn selects supported E2E jobs, targets, or fan-out. Only trusted identifiers from these receipts reach normalized E2E output; free-form model E2E prose is discarded. No second advisor session is opened, including for synthesis repair.
10. Opens one Pi session per model variant and reviews the PR in 16 bounded turns: seven analysis/commit pairs for scope/risk, terminology, correctness/state, security/trust, tests/regressions, CI/operations, and reconciliation, followed by draft and validation JSON synthesis turns in that same session. The scope/risk turn completes the simplicity sweep before it commits findings. It considers whether the touched area can use fewer lines or concepts, remove existing code, or apply an existing or new pattern across current related code. It batches related recommendations and rescans the proposed end state before continuing. The terminology turn selects candidates semantically from changed explanatory text. Trusted code then traces each selected term across the base and head commits, including hyphen and space variants, changed source locations, and available history. Changed-location tracing streams the complete Git diff with bounded per-line memory, so selected terms remain traceable beyond 4 MiB of diff output. Repository occurrence counts and samples still use bounded `git grep` output and report when evidence is truncated. The tests/regressions turn analyzes E2E coverage and new-test gaps, while the CI/operations turn selects supported E2E jobs, targets, or fan-out. Only trusted identifiers from these receipts reach normalized E2E output; free-form model E2E prose is discarded. No second advisor session is opened, including for synthesis repair.
11. Gives each commit turn one job: apply one successful atomic commit for the preceding analysis. Finding commit turns update the finding ledger with one flat object containing homogeneous additions, updates, resolutions, and supersessions arrays plus a no-change reason. The terminology commit turn writes one separate canonical receipt through `pr_review_update_terminology`. A terminology decision must reference a trusted trace and changed file and line bound to the head commit. The commit tool is the turn's only active tool, and the runner rejects prose, other tool calls, or activity after the successful commit. Rejected attempts do not mutate either canonical store and can be corrected before one success. If a commit turn ends with no successful call and every attempt settled without mutating state, the runner permits one tool-only retry and then fails closed. Finding additions require a structured observed-versus-expected basis, a file and line, and eligibility for the active stage. Ledger findings receive stable `F-...` IDs, terminology decisions receive stable `T-...` IDs, and conclusion changes require a reason plus new evidence.
12. Treats open finding-ledger records and the terminology receipt as separate canonical results. Final synthesis cannot silently add, drop, merge, reword, or reclassify either result. Unresolved source-of-truth review entries must reference their covering open finding ID structurally rather than relying on prose matching. A terminology decision does not affect the merge recommendation by itself. A later correctness or security stage can create an ordinary finding only when terminology ambiguity has a concrete effect on behavior, security, data safety, a supported surface, evidence, test meaning, or release meaning.
13. Logs each turn start and settled status and writes the assistant response immediately, preserving partial failed/timed-out turn evidence and the raw transcript. If trusted prompt inputs are unavailable before the model session starts, the runner writes failed analysis and schema-valid final-result artifacts. If a later stage fails, already-committed canonical findings and terminology decisions remain in the low-confidence incomplete result instead of being replaced by a generic unavailable finding.
Expand Down Expand Up @@ -237,7 +238,9 @@ the normalized advisory tier, not merge requirements. Rendered comments label th
the independent PR E2E controller does not consume advisor output.
Findings can also include safe simplification metadata with delete, stdlib,
native, YAGNI, or shrink tags; those suggestions must keep validation, security, data-loss prevention,
and required tests intact.
and required tests intact. A blocker keeps its evidence and required outcome in the blocker card.
Its simplification metadata renders once in a brief `Recommended refactoring` section below the
blockers.
The canonical ledger normalizer reports `merge_as_is` only when a completed, non-low-confidence review
has no open findings.
It reports `merge_after_fixes` when any blocker, warning, or suggestion remains open.
Expand Down
Loading
Loading