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
17 changes: 16 additions & 1 deletion test/automation/pull-requests/pr-review-advisor-local.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ function repository(): string {
"ignored.txt\nartifacts/pr-review-advisor-local/\n",
);
fs.writeFileSync(path.join(directory, "committed.txt"), "base\n");
fs.symlinkSync("committed.txt", path.join(directory, "tracked-internal-link"));
fs.symlinkSync("/etc/passwd", path.join(directory, "tracked-retargeted-link"));
fs.writeFileSync(path.join(directory, "staged.txt"), "base\n");
fs.writeFileSync(path.join(directory, "unstaged.txt"), "base\n");
fs.mkdirSync(path.join(directory, "tools", "pr-review-advisor"), { recursive: true });
Expand All @@ -144,6 +146,8 @@ function repository(): string {
fs.writeFileSync(path.join(directory, "staged.txt"), "staged\n");
git(directory, ["add", "staged.txt"]);
fs.writeFileSync(path.join(directory, "unstaged.txt"), "unstaged\n");
fs.rmSync(path.join(directory, "tracked-retargeted-link"));
fs.symlinkSync("committed.txt", path.join(directory, "tracked-retargeted-link"));
fs.writeFileSync(path.join(directory, "untracked.txt"), "untracked\n");
fs.symlinkSync("/etc/passwd", path.join(directory, "untracked-link"));
fs.writeFileSync(path.join(directory, "ignored.txt"), "ignored\n");
Expand Down Expand Up @@ -406,12 +410,23 @@ describe("local PR review advisor", () => {

expect(
git(snapshot, ["diff", "--name-only", refs.baseRef + ".." + refs.headRef]).split("\n"),
).toEqual(["committed.txt", "staged.txt", "unstaged.txt", "untracked-link", "untracked.txt"]);
).toEqual([
"committed.txt",
"staged.txt",
"tracked-retargeted-link",
"unstaged.txt",
"untracked-link",
"untracked.txt",
]);
expect(fs.readFileSync(path.join(snapshot, "committed.txt"), "utf8")).toBe("branch\n");
expect(fs.readFileSync(path.join(snapshot, "staged.txt"), "utf8")).toBe("staged\n");
expect(fs.readFileSync(path.join(snapshot, "unstaged.txt"), "utf8")).toBe("unstaged\n");
expect(fs.readFileSync(path.join(snapshot, "untracked.txt"), "utf8")).toBe("untracked\n");
expect(fs.existsSync(path.join(snapshot, "ignored.txt"))).toBe(false);
expect(fs.readlinkSync(path.join(snapshot, "tracked-internal-link"))).toBe("committed.txt");
expect(fs.readlinkSync(path.join(snapshot, "tracked-retargeted-link"))).toBe("committed.txt");
expect(git(snapshot, ["ls-tree", refs.headRef, "tracked-internal-link"])).toContain("120000 blob");
expect(git(snapshot, ["ls-tree", refs.headRef, "tracked-retargeted-link"])).toContain("120000 blob");
expect(git(snapshot, ["ls-tree", refs.headRef, "untracked-link"])).toContain("120000 blob");
expect(fs.existsSync(path.join(snapshot, "untracked-link"))).toBe(false);
expect(git(source, ["status", "--porcelain=v1", "-uall"])).toBe(before);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ describe("PR review advisor specialist prompts", () => {

const expected = fs.readFileSync(artifact, "utf8");
expect(path.basename(artifact)).toBe("pr-review-architecture-standard-work-summary.md");
expect(expected).toContain("PR Review Advisor — Architecture and standard work specialist");
expect(expected).toContain("PR Review Advisor — Architecture ownership specialist");
expect(expected).toContain("Complete specialist review for maintainers and review agents.");
expect(expected).toContain("Concrete reduction.");
});
Expand Down
2 changes: 1 addition & 1 deletion tools/pr-review-advisor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ model-backed analysis in OpenShell sandboxes from trusted GitHub Actions jobs an
read-only data. It posts a sticky comment that links to the complete specialist reviews in the
workflow run.

For each configured pull-request event, it runs every specialist prompt in `tools/pr-review-advisor/specialists`. Each prompt owns a distinct review concern and defines its purpose, method, scope, exclusions, review principles, and finding threshold.
For each configured pull-request event, it runs every specialist prompt in `tools/pr-review-advisor/specialists`. Each prompt owns a distinct review concern and defines its purpose, investigation method, evidence expectations, and finding threshold.

Specialists inspect their assigned concern and recommend the smallest direct correction. They run independently and publish separate reports. The advisor does not select, aggregate, or summarize their findings.

Expand Down
4 changes: 3 additions & 1 deletion tools/pr-review-advisor/investigate-turn.mts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ Treat code growth as suspect and compare it with direct modification, reuse, con

Assess checked-in regression evidence and choose only supported E2E selectors. Never claim a job ran or turn E2E guidance into a finding without a checked-in defect.

Return a concise specialist review with evidence-backed issues, exact citations, remedies, verification hints, positives, and limitations.`,
Before forming findings, enumerate the complete changed-file set and inspect the relevant changed hunks. For each candidate finding, compare the relevant parent state with the proposed state. Establish whether a changed line, changed omission, newly affected consumer, or changed contract introduces, worsens, exposes, expands, or materially relies on the problem. Repository-wide evidence can establish a call path or consequence, but its mere presence does not make an inherited condition attributable to this pull request. Verify production references through direct calls, qualified calls, imports, re-exports, wrappers, dependency injection, and selected immutable revisions where applicable. Distinguish a demonstrated behavior defect from missing evidence and from material uncertainty.

Return a specialist review with evidence-backed issues, exact citations, remedies, verification hints, positives, and limitations.`,
};
}
17 changes: 12 additions & 5 deletions tools/pr-review-advisor/local-review-implementation.mts
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,17 @@ function git(cwd: string, args: readonly string[], input?: string): string {
);
}
const gitValue = (cwd: string, args: readonly string[]): string => git(cwd, args).trim();
function removeSnapshotSymlinks(directory: string): void {
for (const entry of fs.readdirSync(directory, { recursive: true, withFileTypes: true }))
if (entry.isSymbolicLink()) fs.rmSync(path.join(entry.parentPath, entry.name), { force: true });
function removeSnapshotSymlinksResolvingOutside(directory: string): void {
const root = fs.realpathSync(directory);
for (const entry of fs.readdirSync(directory, { recursive: true, withFileTypes: true })) {
if (!entry.isSymbolicLink()) continue;
const link = path.join(entry.parentPath, entry.name);
const target = path.resolve(entry.parentPath, fs.readlinkSync(link));
const relative = path.relative(root, target);
const resolvesOutside =
relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative);
if (resolvesOutside) fs.rmSync(link, { force: true });
}
}
export function createLocalReviewSnapshot(
source: string,
Expand All @@ -152,7 +160,6 @@ export function createLocalReviewSnapshot(
const initialHead = gitValue(destination, ["rev-parse", "HEAD"]);
git(destination, ["read-tree", initialHead]);
git(destination, [...disabledFilters(source), "checkout-index", "--all", "--force"]);
removeSnapshotSymlinks(destination);
const patch = git(source, ["diff", "--binary", "--no-ext-diff", "--no-textconv", "HEAD"]);
if (patch) {
git(destination, ["apply", "--cached", "--binary", "-"], patch);
Expand All @@ -177,7 +184,7 @@ export function createLocalReviewSnapshot(
"Local review snapshot",
]);
git(destination, ["update-ref", "--no-deref", "HEAD", commit]);
removeSnapshotSymlinks(destination);
removeSnapshotSymlinksResolvingOutside(destination);
git(destination, ["cat-file", "-e", baseRef + "^{commit}"]);
return { baseRef, headRef: commit };
}
Expand Down
27 changes: 12 additions & 15 deletions tools/pr-review-advisor/specialists/architecture-standard-work.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,30 @@ SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
SPDX-License-Identifier: Apache-2.0
-->

# Architecture and standard work
# Architecture ownership

## Purpose

Determine whether one proportionate design owns the result through the simplest established path.
Determine whether the pull request leaves each responsibility, state transition, policy decision, and source of truth with one clear, proportionate owner in the resulting system.

## Review method

Inspect the complete source-and-test result. Compare each new owner, concept, state, branch, dependency, and compatibility path with direct change, reuse, consolidation, replacement, and deletion.
Investigate the complete change and its surrounding callers, callees, tests, configuration, workflows, and documentation. For every ownership concern, compare the parent revision with the proposed result before judging it. Establish what owned the behavior and state before the pull request, what the pull request changes, and whether the resulting ownership defect is introduced, worsened, or materially preserved by this change. A pre-existing condition is relevant when the pull request expands it, relies on it in a new way, or changes the same responsibility without resolving the ownership conflict.

## Own

- Responsibility, state ownership, dependency direction, and one source of truth.
- Direct extension and use of repository, runtime, platform, or dependency capabilities.
- Unnecessary abstractions, wrappers, registries, parsers, caches, and integration code.
- Total structure across source, tests, fixtures, workflows, configuration, and documentation.
- Migration and replacement completion.
- Obsolete callers, tests, documents, and compatibility paths.
Trace behavior end to end. Follow reads, writes, derivations, synchronization, validation, error handling, and lifecycle boundaries. Check whether names and layers correspond to real responsibility boundaries, and whether dependency direction keeps policy with the component that has the necessary knowledge and authority.

## Do not own
## Own

Do not report a wrong product result, a security defect, test-oracle quality, operational recovery, or writing style unless duplicated ownership is the present defect.
- Responsibility boundaries and accountable owners.
- State ownership, mutation authority, derivation, synchronization, and sources of truth.
- Dependency direction and placement of policy decisions.
- Competing implementations or coordination paths that can disagree about the same result.
- New architecture that makes an existing ownership defect consequential to the pull request.

## Review principles

Remove overprocessing, duplicate ownership, unnecessary handoffs, and speculative machinery. Do not move complexity to another file or surface.
Prefer one authoritative path for each decision and state transition. Distinguish intentional layering from split authority. Judge the resulting system, not merely the size or novelty of the diff. Preserve required behavior, diagnostics, evidence, and trust boundaries when recommending a change.

## Report a finding when

The change creates or retains duplicate authority, an unnecessary owner or concept, a wrong dependency direction, an incomplete replacement, or custom machinery that an established capability can replace. Name the current cost and one coherent reduction. Preserve behavior, diagnostics, evidence, and trust boundaries.
The pull request introduces, worsens, or materially relies on unclear or duplicate authority, conflicting sources of truth, misplaced policy, an invalid dependency direction, or a responsibility boundary that permits components to disagree. Cite the changed lines that make the issue attributable to the pull request and the parent-state evidence needed to show the comparison. Explain the concrete failure mode or maintenance cost, identify the intended owner, and give a coherent remedy with a verification approach.
28 changes: 11 additions & 17 deletions tools/pr-review-advisor/specialists/delivery-flow.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,26 @@ SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
SPDX-License-Identifier: Apache-2.0
-->

# Delivery flow
# Delivery and workflow causality

## Purpose

Determine whether the change moves evidence from commit to maintainer decision without avoidable delay or rework.
Determine how this pull request changes the path from a repository event or source change to trustworthy evidence and a maintainer decision.

## Review method

Follow changed CI, build, test, artifact, publication, and release work as a value stream. Identify each queue, dependency, batch, handoff, repeated operation, cancellation boundary, and feedback point.
Establish the parent-state workflow from parent versions of changed files and connected reusable workflows, actions, scripts, selectors, artifacts, and tests. Reconstruct the proposed state and compare the execution graphs.

## Own

- CI and workflow dependencies, fan-out, concurrency, and cancellation.
- Duplicate builds, tests, downloads, and artifact production.
- Artifact handoffs, cache use, and publication flow.
- Failure localization, retained diagnostics, and feedback latency.
- Superseded work, unnecessary batching, waiting, transport, and work in progress.

## Do not own
Trace each material change from trigger and changed-file classification through conditions, dependencies, fan-out, concurrency, cancellation, work, artifact transport, evidence retention, publication, and the consuming decision. Resolve what each expression, output, artifact, immutable action revision, or status actually controls. Classify behavior as introduced, worsened, removed, exposed, or unchanged.

Do not report deployment recovery, product runtime performance, security boundaries, generic workflow style, or hypothetical scale concerns. Do not report external CI status.

## Review principles
## Own

Map the value stream. Remove waiting, batching, transport, repeated work, and excess work in progress. Prefer early deterministic feedback and direct evidence flow.
- Trigger, path-selection, matrix, conditional, dependency, fan-out, concurrency, cancellation, and supersession behavior.
- Relationships between production changes, selected verification, generated plans, and required evidence.
- Duplicate or displaced builds, tests, installation, downloads, packaging, uploads, and reconstruction.
- Artifact identity, provenance handoffs, availability, retention, and consumption.
- Failure localization, diagnostic preservation, feedback ordering, queues, waiting, and handoffs.

## Report a finding when

Checked-in workflow or tooling causes a present avoidable delay, repeated operation, unnecessary handoff, broad fan-out, stale work, or late failure signal. Name the affected evidence path, current waste, measurable or structurally certain effect, and smallest flow-preserving change.
A changed workflow, selector, action, script, configuration, or delivery contract causally introduces or worsens delay, repeated or stale work, an unnecessary handoff, incorrect dependency, missing or late evidence, loss of diagnostics, overly broad fan-out, or an artifact path that no longer reaches its decision. Cite the changed control point, parent behavior, proposed behavior, downstream effect, and flow-preserving correction.
28 changes: 11 additions & 17 deletions tools/pr-review-advisor/specialists/documentation-standard-work.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,25 @@ SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
SPDX-License-Identifier: Apache-2.0
-->

# Documentation and standard work
# Documentation drift

## Purpose

Determine whether the intended reader can perform the correct action from the changed text.
Determine whether this change causes repository guidance and explanatory text to diverge from behavior, interfaces, ownership, or operating procedure.

## Review method

Apply the trusted writing guide to changed explanatory text. Compare commands, examples, prerequisites, limits, failure guidance, links, support claims, messages, and test titles with current repository evidence. Trace changed terminology when its meaning matters.
Establish parent state, then compare proposed state. Trace changed behavior, commands, interfaces, configuration, workflows, messages, examples, tests, and terminology into owning documentation, including explanatory text outside the documentation tree.

## Own
Determine whether the change introduces an inaccurate statement, leaves existing guidance newly stale, leaves readers routed to a former owner, edits a claim that depends on retained drift, or merely encounters a parent-state defect not made newly relevant. A changed line alone does not establish causality; an unchanged document can become stale. Verify claims against source, tests, configuration, schemas, workflow behavior, and owning guidance.

- Reader procedures and standard work.
- Commands, examples, prerequisites, limits, and recovery instructions.
- Support claims, links, user-visible messages, and meaningful test titles.
- Writing-only findings and terminology consistency.
## Review scope

## Do not own
- Procedures, prerequisites, commands, examples, limits, expected results, failure handling, and recovery.
- Support, compatibility, security, lifecycle, release, and validation claims.
- Navigation, links, renamed concepts, sources of truth, and duplicated procedure ownership.
- Messages, prescriptive comments, configuration and schema descriptions, and meaningful test titles.

Do not report an implementation defect. Cite it only as evidence that the text directs the reader incorrectly. Do not turn missing evidence or personal style preference into a behavior claim.
## Findings

## Review principles

Make normal and abnormal actions visible. Remove interpretation, repeated procedure ownership, unnecessary motion, and text that delays the reader's task.

## Report a finding when

Changed text can cause a wrong action, omit a required condition, conflict with the owning procedure, misstate support, hide the affected object or next action, or use a term with conflicting operational meaning. Group locations with one cause. Propose a shorter accurate rewrite. Treat writing-only defects as suggestions unless they change behavior, security, data safety, support, test meaning, release meaning, or required evidence.
For each documentation-drift issue caused, exposed, or materially worsened by the change, cite the changed behavior or ownership transition, parent comparison, affected reader-facing text or missing owning update, incorrect action or interpretation, and accurate remedy. Treat wording and terminology according to operational effect.
Loading
Loading