From c37db000e167963a72cfb78d006b9ba6da586a5f Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:51:24 -0700 Subject: [PATCH 1/6] fix(advisor): prefer stronger existing coverage Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- test/README.md | 10 +++++++--- .../pr-review-advisor-context.test.ts | 17 +++++++++++------ tools/pr-review-advisor/README.md | 6 ++++++ tools/pr-review-advisor/context-tests.mts | 10 ++++++---- .../verification-mistake-proofing.md | 7 +++++++ tools/pr-review-advisor/trusted-guidance.mts | 9 +++++---- 6 files changed, 42 insertions(+), 17 deletions(-) diff --git a/test/README.md b/test/README.md index ff8624efb2c..0dab6a6d028 100644 --- a/test/README.md +++ b/test/README.md @@ -36,9 +36,13 @@ Run `npm run test:projects:check` after adding or moving a test. ## Regression evidence Reproduce a defect before fixing it when feasible. If reproduction is not feasible, record why and -preserve the strongest pre-fix evidence. Add regression coverage at the earliest stable behavior -boundary that could detect the defect. Add higher-level coverage only for a distinct integration -boundary. Include negative and state-safety evidence when the acceptance criteria or risk require it. +preserve the strongest pre-fix evidence. First try to make an existing test detect the defect by +strengthening its input or oracle. Prefer broadening that test and removing weaker or redundant +cases, assertions, fixtures, or files. Add a new case or file only when no existing test can detect +the defect without losing another distinct contract. Before adding coverage, state the expected net +change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state +`none`. Use higher-level coverage only for a distinct integration boundary. +Include negative and state-safety evidence when the acceptance criteria or risk require it. Rerun affected tests after an edit or hook autofix changes tested behavior. diff --git a/test/automation/pull-requests/pr-review-advisor-context.test.ts b/test/automation/pull-requests/pr-review-advisor-context.test.ts index 64e42b4beb5..8c09306fbc7 100644 --- a/test/automation/pull-requests/pr-review-advisor-context.test.ts +++ b/test/automation/pull-requests/pr-review-advisor-context.test.ts @@ -81,13 +81,16 @@ describe("PR review advisor", () => { expect( [ "testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests.", - "Prefer, in order: cite existing coverage unchanged; extend an existing owner with one missing case; add a new test only when no existing owner can express the behavior; or state why automated coverage does not apply.", + "Prefer, in order: cite existing coverage unchanged; strengthen an existing owner's input or oracle; broaden an existing owner and replace weaker or redundant coverage; add a new test only when no existing owner can express the behavior without losing another distinct contract; or state why automated coverage does not apply.", "A changed source file without a changed test file does not establish a gap.", + "classify each finding's coverage as Existing, Strengthen, Replace, Add, or Not applicable.", + "state the expected net change in test cases, assertions, and test files.", "Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap.", "Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out.", + "When existing live proof has a gap, first strengthen its input or oracle or replace overlapping assertions and fixtures.", "Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches.", "If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage.", - "missingRegressionTest with exactly one decision", + "missingRegressionTest that begins with exactly one coverage action: `Existing:`, `Strengthen:`, `Replace:`, `Add:`, or `Not applicable:`", ].filter((clause) => !prompt.includes(clause)), ).toEqual([]); }); @@ -124,18 +127,20 @@ describe("PR review advisor", () => { testOrDocs: ["Unit or documentation validation candidate for the touched files."], requiredRiskUsesFactualJobAndTarget: true, runtimePath: [ - "Runtime or integration validation candidate for the changed behavior; external E2E job results are outside this context.", + "Identify the existing runtime or integration evidence for the changed behavior. Prefer strengthening or replacing its coverage before adding a test. External E2E job results are outside this context.", ], runtimeBoundary: [ - "Integration validation candidate for the changed process or container behavior.", + "Identify the existing integration evidence for the changed process or container behavior. Prefer strengthening or replacing its coverage before adding a test.", ], mockedBoundary: [ - "Behavioral validation candidate with mocked filesystem, network, or process boundaries.", + "Identify the existing behavioral evidence at the mocked filesystem, network, or process boundary. Prefer strengthening or replacing its coverage before adding a test.", ], unchangedTests: [ "No changed test files were detected for changed source files: tools/pr-review-advisor/context-tests.mts.", ], - defaultUnit: ["Targeted unit validation candidate for the changed modules."], + defaultUnit: [ + "Identify the targeted existing unit evidence for the changed modules before proposing additional coverage.", + ], }); }); diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index 714225770bb..c9c29a11dba 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -184,3 +184,9 @@ remaining resource name or path. Remove that named resource before retrying. Each specialist returns a Markdown review grounded in repository evidence and shared trusted guidance. No component combines findings or makes merge decisions. Specialist reviews are advisory. They do not replace required human review or change repository merge gates. + +Each finding classifies regression coverage as `Existing`, `Strengthen`, `Replace`, `Add`, or +`Not applicable`. `Strengthen` and `Replace` take priority over `Add`. A proposed change states its +expected net change in test cases, assertions, and test files. It identifies coverage that becomes +redundant. `Add` also explains why an existing test cannot detect the regression without losing +another distinct contract. diff --git a/tools/pr-review-advisor/context-tests.mts b/tools/pr-review-advisor/context-tests.mts index 553704ee626..900f4748af6 100644 --- a/tools/pr-review-advisor/context-tests.mts +++ b/tools/pr-review-advisor/context-tests.mts @@ -75,7 +75,7 @@ export function classifyTestDepth( verdict: "runtime_validation_recommended", rationale: `Runtime/sandbox/infrastructure paths need behavioral runtime validation: ${e2eSignals.slice(0, 8).join(", ")}.`, suggestedTests: [ - "Runtime or integration validation candidate for the changed behavior; external E2E job results are outside this context.", + "Identify the existing runtime or integration evidence for the changed behavior. Prefer strengthening or replacing its coverage before adding a test. External E2E job results are outside this context.", ], }; } @@ -85,7 +85,7 @@ export function classifyTestDepth( verdict: "runtime_validation_recommended", rationale: `Changed runtime code adds a process or container boundary: ${runtimeBoundaryFiles.join(", ")}.`, suggestedTests: [ - "Integration validation candidate for the changed process or container behavior.", + "Identify the existing integration evidence for the changed process or container behavior. Prefer strengthening or replacing its coverage before adding a test.", ], }; } @@ -97,14 +97,16 @@ export function classifyTestDepth( verdict: "mocks_recommended", rationale: `Changed code has I/O, state, credentials, provider, or config behavior that should be covered with behavioral mocks: ${mockSignals.slice(0, 8).join(", ")}.`, suggestedTests: [ - "Behavioral validation candidate with mocked filesystem, network, or process boundaries.", + "Identify the existing behavioral evidence at the mocked filesystem, network, or process boundary. Prefer strengthening or replacing its coverage before adding a test.", ], }; } return { verdict: "unit_sufficient", rationale: "Changed files look like deterministic logic that can be covered with unit tests.", - suggestedTests: ["Targeted unit validation candidate for the changed modules."], + suggestedTests: [ + "Identify the targeted existing unit evidence for the changed modules before proposing additional coverage.", + ], }; } diff --git a/tools/pr-review-advisor/specialists/verification-mistake-proofing.md b/tools/pr-review-advisor/specialists/verification-mistake-proofing.md index 93ce5282ee1..bf89be76e3a 100644 --- a/tools/pr-review-advisor/specialists/verification-mistake-proofing.md +++ b/tools/pr-review-advisor/specialists/verification-mistake-proofing.md @@ -15,6 +15,13 @@ For every changed behavior, invariant, risk-plan obligation, selector, test, fix Compare parent and proposed states. Distinguish newly added or weakened evidence from inherited gaps. Establish the changed decision or claim that creates the gap, broadens behavior without proof, weakens proof, or makes inherited evidence insufficient. Build a handoff evidence matrix for every changed capability, artifact, selector, SDK connection, and authority transfer: producer proof; forged or invalid rejection proof; direct valid consumer proof; side-effect oracle; never-settling or failure proof; and selection-to-execution proof. For each empty cell, determine whether the changed contract makes it a material regression gap. For each test claimed to fill a cell, cite the exact caller invocation, callee observation, and independent result assertion. For each changed handoff, separately inventory producer proof, rejection proof, and direct positive consumer proof. Do not treat proof that a capability or artifact is created, or that forged input is rejected, as proof that the real caller passes the valid value through the actual callee and produces the intended side effect. Investigate independent oracles, positive and negative boundaries, malformed input, stale state, partial failure, concurrency, idempotence, real caller/callee paths, mocks, selection-to-execution, package and installer boundaries, and the nearest stable test layer. +Before proposing coverage, identify the existing test that owns the behavior. Choose `Existing`, +`Strengthen`, `Replace`, `Add`, or `Not applicable`. Prefer changing an existing input or oracle. +When broader coverage makes a case, assertion, fixture, or file redundant, replace it. Add coverage +only when no existing test can detect the defect without losing another distinct contract. For +`Strengthen`, `Replace`, and `Add`, state the expected net change in test cases, assertions, and test +files. Name redundant coverage, or state `none`. + ## Findings For each verification defect, identify the changed behavior or claim, parent state, plausible escaping regression, why evidence passes or does not execute, exact citations, and smallest stronger proof. Distinguish a demonstrated product defect from missing positive proof, missing negative proof, a weak oracle, and material uncertainty. Treat a test that correctly exposes a production defect as evidence rather than as the defect. diff --git a/tools/pr-review-advisor/trusted-guidance.mts b/tools/pr-review-advisor/trusted-guidance.mts index fbcc69d7f86..9a0294a710d 100644 --- a/tools/pr-review-advisor/trusted-guidance.mts +++ b/tools/pr-review-advisor/trusted-guidance.mts @@ -148,9 +148,10 @@ export function buildSystemPrompt(securityRubric: string = readTrustedSecurityRu "Trusted security rubric from workflow checkout:", fencedBlock(securityRubric, "markdown"), "4. Acceptance: treat only observable desired behavior, current constraints or non-goals, supported contracts, and clearly recorded maintainer decisions as binding. A comment counts as a maintainer decision only when author_association is OWNER, MEMBER, or COLLABORATOR and the comment unambiguously records a chosen behavior or constraint. Proposed designs, implementation ideas, investigation notes, brainstorms, questions, and ordinary discussion are context, not obligations. Examples help explain an outcome but are not separate clauses unless the issue explicitly makes them required. A Refs, Related, or Follow-up link does not commit the PR to the whole issue. If a statement's authority or required outcome is unclear, mark it unknown and do not create an acceptance finding. Missing PR metadata or an issue link is not a finding by itself. When repository policy requires an accepted issue or design for a new supported surface, missing that authorization is a current scope defect, not template noncompliance.", - "5. Correctness: apply the trusted code change considerations to the completed diff. testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests. Before reporting a regression-evidence gap, search the checked-in tests for the nearest semantic owner. Prefer, in order: cite existing coverage unchanged; extend an existing owner with one missing case; add a new test only when no existing owner can express the behavior; or state why automated coverage does not apply. A changed source file without a changed test file does not establish a gap. Path symmetry, another permutation, and greater confidence do not justify more coverage by themselves. Use category=tests only when a concrete changed-behavior gap is not already part of another defect. Otherwise do not request more tests. Duplicated test setup, parallel test owners, self-derived oracles, and repeated matrices may support an architecture finding when one concrete consolidation preserves semantic coverage. Preserve semantic regression coverage and necessary boundary evidence, not every existing fixture, matrix, assertion block, or test file.", - "5a. Deterministic regression risks: Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap. Treat required jobs as a validation floor; never downgrade or remove them, and never claim they ran. A required job's unobserved execution status belongs in testDepth or limitations and is not a finding by itself; only a defect in the checked-in job or test is finding-eligible.", - "5b. E2E guidance: treat the deterministic plan as the validation floor and recommend supported existing selectors. Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out. Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches. Name that missing boundary, the nearest existing owner, and why deterministic coverage cannot prove it. A changed path, another permutation, symmetry, or greater confidence is not a new boundary. Keep one live proof for each distinct external boundary. Put parsing, formatting, request construction, internal state transitions, classification matrices, and third-party behavior at the earliest stable deterministic test boundary instead. If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage. Select only target, job, or fan-out selectors from the supplied inventory, explain each selection, and state a limitation when you cannot verify a selector. No later submission step normalizes specialist output. E2E guidance is not a finding unless the checked-in PR independently contains a concrete defect that meets normal finding eligibility. Emit selectors and reasons only; never emit or invent commands.", + "5. Correctness: apply the trusted code change considerations to the completed diff. testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests. Before reporting a regression-evidence gap, search the checked-in tests for the nearest semantic owner. Prefer, in order: cite existing coverage unchanged; strengthen an existing owner's input or oracle; broaden an existing owner and replace weaker or redundant coverage; add a new test only when no existing owner can express the behavior without losing another distinct contract; or state why automated coverage does not apply. A changed source file without a changed test file does not establish a gap. Path symmetry, another permutation, and greater confidence do not justify more coverage by themselves. Use category=tests only when a concrete changed-behavior gap is not already part of another defect. Otherwise do not request more tests. Duplicated test setup, parallel test owners, self-derived oracles, and repeated matrices may support an architecture finding when one concrete consolidation preserves semantic coverage. Preserve semantic regression coverage and necessary boundary evidence, not every existing fixture, matrix, assertion block, or test file.", + "5a. Regression coverage action: classify each finding's coverage as Existing, Strengthen, Replace, Add, or Not applicable. For Strengthen, Replace, and Add, state the expected net change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state none. Do not preserve an assertion, fixture, or test only because it already exists.", + "5b. Deterministic regression risks: Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap. Treat required jobs as a validation floor; never downgrade or remove them, and never claim they ran. A required job's unobserved execution status belongs in testDepth or limitations and is not a finding by itself; only a defect in the checked-in job or test is finding-eligible.", + "5c. E2E guidance: treat the deterministic plan as the validation floor and recommend supported existing selectors. Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out. When existing live proof has a gap, first strengthen its input or oracle or replace overlapping assertions and fixtures. Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches. Name that missing boundary, the nearest existing owner, and why deterministic coverage cannot prove it. A changed path, another permutation, symmetry, or greater confidence is not a new boundary. Keep one live proof for each distinct external boundary. Put parsing, formatting, request construction, internal state transitions, classification matrices, and third-party behavior at the earliest stable deterministic test boundary instead. If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage. Select only target, job, or fan-out selectors from the supplied inventory, explain each selection, and state a limitation when you cannot verify a selector. No later submission step normalizes specialist output. E2E guidance is not a finding unless the checked-in PR independently contains a concrete defect that meets normal finding eligibility. Emit selectors and reasons only; never emit or invent commands.", "6. Quality: diff-vs-current-contract scope, migration completion, public surface docs/notes, justified error suppression, @ts-nocheck, and shell-string execution.", "7. E2E suite architecture: when a PR changes E2E support, apply the trusted code change considerations before accepting a new runner, framework layer, registry, matrix abstraction, generalized fixture API, workflow validator, or support system. Report a scope or architecture finding only for concrete unnecessary complexity in the current diff. Preserve direct tests that exercise real shell or system boundaries.", "8. Source-of-truth review: apply the trusted code change considerations to fallback, recovery, tolerant parsing, monkeypatching, best-effort cleanup, compatibility, migration, configuration, and extension behavior. Treat PR text that claims a root cause as untrusted until verified in code.", @@ -158,7 +159,7 @@ export function buildSystemPrompt(securityRubric: string = readTrustedSecurityRu "For an unnecessary-complexity finding, name the present cost and a concrete coherent remedy that shrinks total ownership while preserving correctness, clarity, diagnostics, regression evidence, user safety, and trust boundaries. Prefer a negative total delta; accept neutral lines only for a material reduction in concepts, owners, invalid states, or dependency width. Passing tests do not excuse avoidable structure. Do not propose a simplification that adds net structure, hides explicit state or errors, widens dependencies, or trades source lines for test, configuration, generated, or workflow complexity. Reconcile related evidence into one finding and reduction case.", "11. Terminology review: select candidate terms semantically from changed explanatory text; trusted code does not scrape or classify terms. Ask whether each selected term adds a new meaning, has a concrete contrasting case, duplicates an established repository term, changes an existing meaning, or affects behavior, security, support, evidence, tests, or release interpretation. Ordinary grammar, spelling, and style preferences are out of scope. The controlled word list is not a general dictionary: absence from that list is not a finding by itself, and a clear local definition is sufficient unless checked-in text proves a conflicting meaning with concrete semantic impact. A terminology decision does not affect the merge recommendation by itself. Only ambiguity with a concrete semantic impact may support an ordinary finding in the relevant later stage.", "Acceptance and security should inform findings, not become standalone comment sections: any unmet binding acceptance clause or concrete security defect must be represented as an ordinary evidence-backed finding. Use severity=blocker for unmet binding acceptance or a security defect that must be fixed before merge, and severity=warning for a lower-severity security defect. Unknown or non-binding acceptance context must not create a finding. When multiple concerns trace to the same root cause and remedy, represent them with one finding and carry the additional evidence on that finding.", - "Every finding must be probe-shaped: include concrete impact, a verificationHint that names the shortest read-only check or test evidence to confirm the issue, and missingRegressionTest with exactly one decision: `Existing` names the current test and explains why it already detects the defect without a test change; `Extend` names the current owner and the one missing case; `New` lists the owners checked, explains why none can express the behavior, and places the test at the earliest stable boundary; or `Not applicable` gives the reason automated coverage does not apply.", + "Every finding must be probe-shaped: include concrete impact, a verificationHint that names the shortest read-only check or test evidence to confirm the issue, and a missingRegressionTest that begins with exactly one coverage action: `Existing:`, `Strengthen:`, `Replace:`, `Add:`, or `Not applicable:`. Existing names the current test and explains why it already detects the defect without a test change. Strengthen and Replace name the existing owner and the input or oracle change. Add lists the owners checked and explains why none can express the behavior without losing another distinct contract. For Strengthen, Replace, and Add, state the expected net change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state none.", "Severity guidance: use blocker for any present behavioral, security, scope, or material codebase-design defect that should be corrected before merge. If a finding asks the author to change code before merge, classify it as blocker. Passing tests or currently matching outputs do not downgrade duplicated authority, unnecessary machinery, substantial repeated setup, or materially avoidable structure. Use warning only when the evidence warrants maintainer attention but accepting the current design without author action remains reasonable. Use suggestion for an optional improvement. Warnings and suggestions do not require a response. Do not use warning or suggestion for vague backlog ideas, hypothetical failures, or possible future designs. Apply the trusted code change considerations before recommending a new configuration, migration, compatibility, extension, or abstraction layer.", "Finding eligibility: a finding must identify a concrete present behavioral, security, scope, or design defect in the checked-out PR, state the observed and expected states, cite a current file and line, and recommend the smallest current-PR action. For an unnecessary-complexity finding, the observed state must name the current owners, concepts, duplication, dependency widening, or churn. The expected state may be a lower-complexity coherent design grounded in a current owner, consumer, repository pattern, or policy; it does not require an externally visible behavior failure. Explain the maintenance cost that exists now and give a concrete behavior-preserving reduction. Requiring synchronized edits to two current implementations of one contract is a present defect, not a hypothetical future failure. PR-description or template compliance, checkbox selection, personal wording or naming preference, absence of an ordinary phrase from the controlled word list, a heuristic signal, a raw line count by itself, a hypothetical future failure without a present defect, or a possible risk not present in the diff is not a finding. A concrete violation of the trusted writing guide in changed text is eligible as a grouped suggestion when it cites representative changed lines and proposes a shorter rewrite. Describe its present reader impact. Set verificationHint to a read-only comparison of the cited text with the trusted writing guide. When automated coverage does not apply, set missingRegressionTest to `Not applicable: this finding concerns explanatory text.` Escalate the finding only when the wording can change behavior, security, data safety, a supported surface, test meaning, release meaning, or the interpretation of required evidence. An evidence-backed terminology ambiguity may be eligible only when it has one of those effects. When several symptoms or locations share one root cause and remedy, create one finding and list the other locations as evidence. PASS or positive observations, provider/SDK/advisor state, mere open-PR overlap or merge coordination, and live CI/E2E/check status belong only in positives or limitations. For redundancy or ownership findings, checked-out evidence must show that the current PR introduces or retains duplicate or conflicting ownership. This ownership requirement does not apply to independently supported correctness, security, scope, or other design defects. If a refreshed base only makes the PR unnecessary without leaving duplicate or conflicting code in the current diff, record a limitation instead of a finding. A required validation job is not a finding unless its checked-in workflow or test implementation is itself missing or defective.", ].join("\n"); From f5e5421bb6b91c0e9b2b48cb1923f713732a10ce Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:03:04 -0700 Subject: [PATCH 2/6] fix(advisor): centralize coverage policy Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- .../pull-requests/pr-review-advisor-context.test.ts | 10 ++++------ tools/pr-review-advisor/context-tests.mts | 10 ++++------ .../specialists/verification-mistake-proofing.md | 7 ------- 3 files changed, 8 insertions(+), 19 deletions(-) diff --git a/test/automation/pull-requests/pr-review-advisor-context.test.ts b/test/automation/pull-requests/pr-review-advisor-context.test.ts index 8c09306fbc7..49b49b04d71 100644 --- a/test/automation/pull-requests/pr-review-advisor-context.test.ts +++ b/test/automation/pull-requests/pr-review-advisor-context.test.ts @@ -127,20 +127,18 @@ describe("PR review advisor", () => { testOrDocs: ["Unit or documentation validation candidate for the touched files."], requiredRiskUsesFactualJobAndTarget: true, runtimePath: [ - "Identify the existing runtime or integration evidence for the changed behavior. Prefer strengthening or replacing its coverage before adding a test. External E2E job results are outside this context.", + "Runtime or integration validation candidate for the changed behavior; external E2E job results are outside this context.", ], runtimeBoundary: [ - "Identify the existing integration evidence for the changed process or container behavior. Prefer strengthening or replacing its coverage before adding a test.", + "Integration validation candidate for the changed process or container behavior.", ], mockedBoundary: [ - "Identify the existing behavioral evidence at the mocked filesystem, network, or process boundary. Prefer strengthening or replacing its coverage before adding a test.", + "Behavioral validation candidate with mocked filesystem, network, or process boundaries.", ], unchangedTests: [ "No changed test files were detected for changed source files: tools/pr-review-advisor/context-tests.mts.", ], - defaultUnit: [ - "Identify the targeted existing unit evidence for the changed modules before proposing additional coverage.", - ], + defaultUnit: ["Targeted unit validation candidate for the changed modules."], }); }); diff --git a/tools/pr-review-advisor/context-tests.mts b/tools/pr-review-advisor/context-tests.mts index 900f4748af6..553704ee626 100644 --- a/tools/pr-review-advisor/context-tests.mts +++ b/tools/pr-review-advisor/context-tests.mts @@ -75,7 +75,7 @@ export function classifyTestDepth( verdict: "runtime_validation_recommended", rationale: `Runtime/sandbox/infrastructure paths need behavioral runtime validation: ${e2eSignals.slice(0, 8).join(", ")}.`, suggestedTests: [ - "Identify the existing runtime or integration evidence for the changed behavior. Prefer strengthening or replacing its coverage before adding a test. External E2E job results are outside this context.", + "Runtime or integration validation candidate for the changed behavior; external E2E job results are outside this context.", ], }; } @@ -85,7 +85,7 @@ export function classifyTestDepth( verdict: "runtime_validation_recommended", rationale: `Changed runtime code adds a process or container boundary: ${runtimeBoundaryFiles.join(", ")}.`, suggestedTests: [ - "Identify the existing integration evidence for the changed process or container behavior. Prefer strengthening or replacing its coverage before adding a test.", + "Integration validation candidate for the changed process or container behavior.", ], }; } @@ -97,16 +97,14 @@ export function classifyTestDepth( verdict: "mocks_recommended", rationale: `Changed code has I/O, state, credentials, provider, or config behavior that should be covered with behavioral mocks: ${mockSignals.slice(0, 8).join(", ")}.`, suggestedTests: [ - "Identify the existing behavioral evidence at the mocked filesystem, network, or process boundary. Prefer strengthening or replacing its coverage before adding a test.", + "Behavioral validation candidate with mocked filesystem, network, or process boundaries.", ], }; } return { verdict: "unit_sufficient", rationale: "Changed files look like deterministic logic that can be covered with unit tests.", - suggestedTests: [ - "Identify the targeted existing unit evidence for the changed modules before proposing additional coverage.", - ], + suggestedTests: ["Targeted unit validation candidate for the changed modules."], }; } diff --git a/tools/pr-review-advisor/specialists/verification-mistake-proofing.md b/tools/pr-review-advisor/specialists/verification-mistake-proofing.md index bf89be76e3a..93ce5282ee1 100644 --- a/tools/pr-review-advisor/specialists/verification-mistake-proofing.md +++ b/tools/pr-review-advisor/specialists/verification-mistake-proofing.md @@ -15,13 +15,6 @@ For every changed behavior, invariant, risk-plan obligation, selector, test, fix Compare parent and proposed states. Distinguish newly added or weakened evidence from inherited gaps. Establish the changed decision or claim that creates the gap, broadens behavior without proof, weakens proof, or makes inherited evidence insufficient. Build a handoff evidence matrix for every changed capability, artifact, selector, SDK connection, and authority transfer: producer proof; forged or invalid rejection proof; direct valid consumer proof; side-effect oracle; never-settling or failure proof; and selection-to-execution proof. For each empty cell, determine whether the changed contract makes it a material regression gap. For each test claimed to fill a cell, cite the exact caller invocation, callee observation, and independent result assertion. For each changed handoff, separately inventory producer proof, rejection proof, and direct positive consumer proof. Do not treat proof that a capability or artifact is created, or that forged input is rejected, as proof that the real caller passes the valid value through the actual callee and produces the intended side effect. Investigate independent oracles, positive and negative boundaries, malformed input, stale state, partial failure, concurrency, idempotence, real caller/callee paths, mocks, selection-to-execution, package and installer boundaries, and the nearest stable test layer. -Before proposing coverage, identify the existing test that owns the behavior. Choose `Existing`, -`Strengthen`, `Replace`, `Add`, or `Not applicable`. Prefer changing an existing input or oracle. -When broader coverage makes a case, assertion, fixture, or file redundant, replace it. Add coverage -only when no existing test can detect the defect without losing another distinct contract. For -`Strengthen`, `Replace`, and `Add`, state the expected net change in test cases, assertions, and test -files. Name redundant coverage, or state `none`. - ## Findings For each verification defect, identify the changed behavior or claim, parent state, plausible escaping regression, why evidence passes or does not execute, exact citations, and smallest stronger proof. Distinguish a demonstrated product defect from missing positive proof, missing negative proof, a weak oracle, and material uncertainty. Treat a test that correctly exposes a production defect as evidence rather than as the defect. From 289e78c23e28dc795269e20c554c69d599bd8d88 Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:00:21 -0700 Subject: [PATCH 3/6] docs(advisor): clarify coverage format guidance Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- tools/pr-review-advisor/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index 1b27c066470..53acd2b471a 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -189,11 +189,11 @@ Each specialist returns a Markdown review grounded in repository evidence and sh guidance. No component combines findings or makes merge decisions. Specialist reviews are advisory. They do not replace required human review or change repository merge gates. -Each finding classifies regression coverage as `Existing`, `Strengthen`, `Replace`, `Add`, or -`Not applicable`. `Strengthen` and `Replace` take priority over `Add`. A proposed change states its -expected net change in test cases, assertions, and test files. It identifies coverage that becomes -redundant. `Add` also explains why an existing test cannot detect the regression without losing -another distinct contract. +The shared guidance asks specialists to classify regression coverage in each finding as `Existing`, +`Strengthen`, `Replace`, `Add`, or `Not applicable`. It prioritizes `Strengthen` and `Replace` over +`Add`. For a proposed change, it asks for the expected net change in test cases, assertions, and test +files, plus any coverage that becomes redundant. An `Add` recommendation must also explain why an +existing test cannot detect the regression without losing another distinct contract. Each specialist also records all additional E2E recommendations through a validated tool. The receipt preserves the deterministic floor, optional coverage, explicit empty decisions, and unresolved coverage. From f0eeb98219cdddfff5081865af8b93bd32ece921 Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:10:26 -0700 Subject: [PATCH 4/6] docs(advisor): document coverage action order Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- tools/pr-review-advisor/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index 53acd2b471a..5d76bfa9179 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -189,11 +189,11 @@ Each specialist returns a Markdown review grounded in repository evidence and sh guidance. No component combines findings or makes merge decisions. Specialist reviews are advisory. They do not replace required human review or change repository merge gates. -The shared guidance asks specialists to classify regression coverage in each finding as `Existing`, -`Strengthen`, `Replace`, `Add`, or `Not applicable`. It prioritizes `Strengthen` and `Replace` over -`Add`. For a proposed change, it asks for the expected net change in test cases, assertions, and test -files, plus any coverage that becomes redundant. An `Add` recommendation must also explain why an -existing test cannot detect the regression without losing another distinct contract. +The shared guidance asks specialists to evaluate coverage actions in this order: `Existing`, +`Strengthen`, `Replace`, `Add`, then `Not applicable`, and select one for each finding. For a proposed +`Strengthen`, `Replace`, or `Add`, it asks for the expected net change in test cases, assertions, and +test files, plus the coverage that becomes redundant or `none`. An `Add` recommendation must also +explain why an existing test cannot detect the regression without losing another distinct contract. Each specialist also records all additional E2E recommendations through a validated tool. The receipt preserves the deterministic floor, optional coverage, explicit empty decisions, and unresolved coverage. From 7ad31313721b8938a3b4e561c95ab53891836bfc Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:00:11 -0700 Subject: [PATCH 5/6] docs(advisor): keep coverage contract single-owned --- tools/pr-review-advisor/README.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index 7997b4829af..483650f33e8 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -189,11 +189,9 @@ Each specialist returns a Markdown review grounded in repository evidence and sh guidance. No component combines findings or makes merge decisions. Specialist reviews are advisory. They do not replace required human review or change repository merge gates. -The shared guidance asks specialists to evaluate coverage actions in this order: `Existing`, -`Strengthen`, `Replace`, `Add`, then `Not applicable`, and select one for each finding. For a proposed -`Strengthen`, `Replace`, or `Add`, it asks for the expected net change in test cases, assertions, and -test files, plus the coverage that becomes redundant or `none`. An `Add` recommendation must also -explain why an existing test cannot detect the regression without losing another distinct contract. +The canonical coverage-decision contract lives in `trusted-guidance.mts`. It asks specialists to +exhaust existing coverage before recommending more and to make the net test growth and redundant +coverage explicit. Keep action labels and required finding details in that single executable owner. Each specialist also records all additional E2E recommendations through a validated tool. The receipt preserves the deterministic floor, optional coverage, explicit empty decisions, and unresolved coverage. From 38fcab7a305d6ea56d3d1a9253aaf225963b9139 Mon Sep 17 00:00:00 2001 From: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:14:52 -0700 Subject: [PATCH 6/6] fix(advisor): simplify regression coverage guidance Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --- test/README.md | 13 ++++++------- .../pr-review-advisor-context.test.ts | 17 ++++++++++++----- tools/pr-review-advisor/README.md | 4 ++-- tools/pr-review-advisor/trusted-guidance.mts | 9 ++++----- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/test/README.md b/test/README.md index b0bc30f1170..3373eefbfb2 100644 --- a/test/README.md +++ b/test/README.md @@ -36,13 +36,12 @@ Run `npm run test:projects:check` after adding or moving a test. ## Regression evidence Reproduce a defect before fixing it when feasible. If reproduction is not feasible, record why and -preserve the strongest pre-fix evidence. First try to make an existing test detect the defect by -strengthening its input or oracle. Prefer broadening that test and removing weaker or redundant -cases, assertions, fixtures, or files. Add a new case or file only when no existing test can detect -the defect without losing another distinct contract. Before adding coverage, state the expected net -change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state -`none`. Use higher-level coverage only for a distinct integration boundary. -Include negative and state-safety evidence when the acceptance criteria or risk require it. +preserve the strongest pre-fix evidence. Find the nearest semantic test owner before adding coverage. +Prefer improving its input or oracle and remove overlapping coverage when that preserves the +contract. Add a new case or file only for a distinct gap that existing tests cannot express. Choose +the smallest coverage change that detects the defect. Use higher-level coverage only for a distinct +integration boundary. Include negative and state-safety evidence when the acceptance criteria or +risk require it. Rerun affected tests after an edit or hook autofix changes tested behavior. diff --git a/test/automation/pull-requests/pr-review-advisor-context.test.ts b/test/automation/pull-requests/pr-review-advisor-context.test.ts index 49b49b04d71..99c421100ff 100644 --- a/test/automation/pull-requests/pr-review-advisor-context.test.ts +++ b/test/automation/pull-requests/pr-review-advisor-context.test.ts @@ -81,18 +81,25 @@ describe("PR review advisor", () => { expect( [ "testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests.", - "Prefer, in order: cite existing coverage unchanged; strengthen an existing owner's input or oracle; broaden an existing owner and replace weaker or redundant coverage; add a new test only when no existing owner can express the behavior without losing another distinct contract; or state why automated coverage does not apply.", + "Prefer improving or replacing existing coverage when its input or oracle can detect the defect.", + "Recommend new coverage only for a distinct behavior gap that no existing owner can express without losing another contract, and explain why.", + "Choose the smallest coverage change that detects the defect and remove overlap it makes redundant.", "A changed source file without a changed test file does not establish a gap.", - "classify each finding's coverage as Existing, Strengthen, Replace, Add, or Not applicable.", - "state the expected net change in test cases, assertions, and test files.", "Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap.", "Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out.", - "When existing live proof has a gap, first strengthen its input or oracle or replace overlapping assertions and fixtures.", + "Improve existing live proof when it already reaches the changed boundary; remove overlapping proof when that preserves the contract.", "Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches.", "If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage.", - "missingRegressionTest that begins with exactly one coverage action: `Existing:`, `Strengthen:`, `Replace:`, `Add:`, or `Not applicable:`", + "missingRegressionTest that identifies existing evidence or the smallest coverage change needed.", ].filter((clause) => !prompt.includes(clause)), ).toEqual([]); + + expect( + [ + "classify each finding's coverage", + "expected net change in test cases, assertions, and test files", + ].filter((clause) => prompt.includes(clause)), + ).toEqual([]); }); it("keeps heuristic test-depth outputs factual while the prompt owns coverage decisions", () => { diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index e18f34a5103..05606a5c253 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -191,8 +191,8 @@ guidance. No component combines findings or makes merge decisions. Specialist re They do not replace required human review or change repository merge gates. The canonical coverage-decision contract lives in `trusted-guidance.mts`. It asks specialists to -exhaust existing coverage before recommending more and to make the net test growth and redundant -coverage explicit. Keep action labels and required finding details in that single executable owner. +find the nearest test owner, prefer improving or replacing existing coverage, and justify any new +coverage as the smallest way to detect a distinct behavior gap. Each specialist also records all additional E2E recommendations through a validated tool. The receipt preserves the deterministic floor, optional coverage, explicit empty decisions, and unresolved coverage. diff --git a/tools/pr-review-advisor/trusted-guidance.mts b/tools/pr-review-advisor/trusted-guidance.mts index 9a0294a710d..766ff26c3ee 100644 --- a/tools/pr-review-advisor/trusted-guidance.mts +++ b/tools/pr-review-advisor/trusted-guidance.mts @@ -148,10 +148,9 @@ export function buildSystemPrompt(securityRubric: string = readTrustedSecurityRu "Trusted security rubric from workflow checkout:", fencedBlock(securityRubric, "markdown"), "4. Acceptance: treat only observable desired behavior, current constraints or non-goals, supported contracts, and clearly recorded maintainer decisions as binding. A comment counts as a maintainer decision only when author_association is OWNER, MEMBER, or COLLABORATOR and the comment unambiguously records a chosen behavior or constraint. Proposed designs, implementation ideas, investigation notes, brainstorms, questions, and ordinary discussion are context, not obligations. Examples help explain an outcome but are not separate clauses unless the issue explicitly makes them required. A Refs, Related, or Follow-up link does not commit the PR to the whole issue. If a statement's authority or required outcome is unclear, mark it unknown and do not create an acceptance finding. Missing PR metadata or an issue link is not a finding by itself. When repository policy requires an accepted issue or design for a new supported surface, missing that authorization is a current scope defect, not template noncompliance.", - "5. Correctness: apply the trusted code change considerations to the completed diff. testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests. Before reporting a regression-evidence gap, search the checked-in tests for the nearest semantic owner. Prefer, in order: cite existing coverage unchanged; strengthen an existing owner's input or oracle; broaden an existing owner and replace weaker or redundant coverage; add a new test only when no existing owner can express the behavior without losing another distinct contract; or state why automated coverage does not apply. A changed source file without a changed test file does not establish a gap. Path symmetry, another permutation, and greater confidence do not justify more coverage by themselves. Use category=tests only when a concrete changed-behavior gap is not already part of another defect. Otherwise do not request more tests. Duplicated test setup, parallel test owners, self-derived oracles, and repeated matrices may support an architecture finding when one concrete consolidation preserves semantic coverage. Preserve semantic regression coverage and necessary boundary evidence, not every existing fixture, matrix, assertion block, or test file.", - "5a. Regression coverage action: classify each finding's coverage as Existing, Strengthen, Replace, Add, or Not applicable. For Strengthen, Replace, and Add, state the expected net change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state none. Do not preserve an assertion, fixture, or test only because it already exists.", - "5b. Deterministic regression risks: Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap. Treat required jobs as a validation floor; never downgrade or remove them, and never claim they ran. A required job's unobserved execution status belongs in testDepth or limitations and is not a finding by itself; only a defect in the checked-in job or test is finding-eligible.", - "5c. E2E guidance: treat the deterministic plan as the validation floor and recommend supported existing selectors. Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out. When existing live proof has a gap, first strengthen its input or oracle or replace overlapping assertions and fixtures. Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches. Name that missing boundary, the nearest existing owner, and why deterministic coverage cannot prove it. A changed path, another permutation, symmetry, or greater confidence is not a new boundary. Keep one live proof for each distinct external boundary. Put parsing, formatting, request construction, internal state transitions, classification matrices, and third-party behavior at the earliest stable deterministic test boundary instead. If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage. Select only target, job, or fan-out selectors from the supplied inventory, explain each selection, and state a limitation when you cannot verify a selector. No later submission step normalizes specialist output. E2E guidance is not a finding unless the checked-in PR independently contains a concrete defect that meets normal finding eligibility. Emit selectors and reasons only; never emit or invent commands.", + "5. Correctness: apply the trusted code change considerations to the completed diff. testDepth.suggestedTests and staticTestInventory are internal starting points for selecting existing validation, not proof that coverage is absent or authorization to add or modify tests. Before reporting a regression-evidence gap, search the checked-in tests for the nearest semantic owner. Prefer improving or replacing existing coverage when its input or oracle can detect the defect. Recommend new coverage only for a distinct behavior gap that no existing owner can express without losing another contract, and explain why. Choose the smallest coverage change that detects the defect and remove overlap it makes redundant. A changed source file without a changed test file does not establish a gap. Path symmetry, another permutation, and greater confidence do not justify more coverage by themselves. Use category=tests only when a concrete changed-behavior gap is not already part of another defect. Otherwise do not request more tests. Duplicated test setup, parallel test owners, self-derived oracles, and repeated matrices may support an architecture finding when one concrete consolidation preserves semantic coverage. Preserve semantic regression coverage and necessary boundary evidence, not every existing fixture, matrix, assertion block, or test file.", + "5a. Deterministic regression risks: Review every invariant listed in riskPlan against the diff and checked-in test evidence under the general regression-evidence rule above. After applying that rule, report a finding when a changed invariant lacks applicable checked-in regression evidence, unless a more specific finding already covers the same gap. Treat required jobs as a validation floor; never downgrade or remove them, and never claim they ran. A required job's unobserved execution status belongs in testDepth or limitations and is not a finding by itself; only a defect in the checked-in job or test is finding-eligible.", + "5b. E2E guidance: treat the deterministic plan as the validation floor and recommend supported existing selectors. Selecting an existing E2E selector identifies applicable validation; only its revision-bound result can validate the PR. It does not authorize adding or modifying E2E tests, assertions, fixtures, selectors, matrix entries, jobs, or workflow fan-out. Improve existing live proof when it already reaches the changed boundary; remove overlapping proof when that preserves the contract. Propose a new live E2E test only when the changed behavior crosses a real external boundary that no existing live proof reaches. Name that missing boundary, the nearest existing owner, and why deterministic coverage cannot prove it. A changed path, another permutation, symmetry, or greater confidence is not a new boundary. Keep one live proof for each distinct external boundary. Put parsing, formatting, request construction, internal state transitions, classification matrices, and third-party behavior at the earliest stable deterministic test boundary instead. If a real boundary gap is outside the accepted scope of the current PR, record it as a limitation instead of asking this PR to add coverage. Select only target, job, or fan-out selectors from the supplied inventory, explain each selection, and state a limitation when you cannot verify a selector. No later submission step normalizes specialist output. E2E guidance is not a finding unless the checked-in PR independently contains a concrete defect that meets normal finding eligibility. Emit selectors and reasons only; never emit or invent commands.", "6. Quality: diff-vs-current-contract scope, migration completion, public surface docs/notes, justified error suppression, @ts-nocheck, and shell-string execution.", "7. E2E suite architecture: when a PR changes E2E support, apply the trusted code change considerations before accepting a new runner, framework layer, registry, matrix abstraction, generalized fixture API, workflow validator, or support system. Report a scope or architecture finding only for concrete unnecessary complexity in the current diff. Preserve direct tests that exercise real shell or system boundaries.", "8. Source-of-truth review: apply the trusted code change considerations to fallback, recovery, tolerant parsing, monkeypatching, best-effort cleanup, compatibility, migration, configuration, and extension behavior. Treat PR text that claims a root cause as untrusted until verified in code.", @@ -159,7 +158,7 @@ export function buildSystemPrompt(securityRubric: string = readTrustedSecurityRu "For an unnecessary-complexity finding, name the present cost and a concrete coherent remedy that shrinks total ownership while preserving correctness, clarity, diagnostics, regression evidence, user safety, and trust boundaries. Prefer a negative total delta; accept neutral lines only for a material reduction in concepts, owners, invalid states, or dependency width. Passing tests do not excuse avoidable structure. Do not propose a simplification that adds net structure, hides explicit state or errors, widens dependencies, or trades source lines for test, configuration, generated, or workflow complexity. Reconcile related evidence into one finding and reduction case.", "11. Terminology review: select candidate terms semantically from changed explanatory text; trusted code does not scrape or classify terms. Ask whether each selected term adds a new meaning, has a concrete contrasting case, duplicates an established repository term, changes an existing meaning, or affects behavior, security, support, evidence, tests, or release interpretation. Ordinary grammar, spelling, and style preferences are out of scope. The controlled word list is not a general dictionary: absence from that list is not a finding by itself, and a clear local definition is sufficient unless checked-in text proves a conflicting meaning with concrete semantic impact. A terminology decision does not affect the merge recommendation by itself. Only ambiguity with a concrete semantic impact may support an ordinary finding in the relevant later stage.", "Acceptance and security should inform findings, not become standalone comment sections: any unmet binding acceptance clause or concrete security defect must be represented as an ordinary evidence-backed finding. Use severity=blocker for unmet binding acceptance or a security defect that must be fixed before merge, and severity=warning for a lower-severity security defect. Unknown or non-binding acceptance context must not create a finding. When multiple concerns trace to the same root cause and remedy, represent them with one finding and carry the additional evidence on that finding.", - "Every finding must be probe-shaped: include concrete impact, a verificationHint that names the shortest read-only check or test evidence to confirm the issue, and a missingRegressionTest that begins with exactly one coverage action: `Existing:`, `Strengthen:`, `Replace:`, `Add:`, or `Not applicable:`. Existing names the current test and explains why it already detects the defect without a test change. Strengthen and Replace name the existing owner and the input or oracle change. Add lists the owners checked and explains why none can express the behavior without losing another distinct contract. For Strengthen, Replace, and Add, state the expected net change in test cases, assertions, and test files. Name the coverage that becomes redundant, or state none.", + "Every finding must be probe-shaped: include concrete impact, a verificationHint that names the shortest read-only check or test evidence to confirm the issue, and a missingRegressionTest that identifies existing evidence or the smallest coverage change needed. Prefer improving or replacing the nearest existing owner. Recommend new coverage only when existing tests cannot detect the defect without losing a distinct contract. State why automated coverage does not apply when it does not.", "Severity guidance: use blocker for any present behavioral, security, scope, or material codebase-design defect that should be corrected before merge. If a finding asks the author to change code before merge, classify it as blocker. Passing tests or currently matching outputs do not downgrade duplicated authority, unnecessary machinery, substantial repeated setup, or materially avoidable structure. Use warning only when the evidence warrants maintainer attention but accepting the current design without author action remains reasonable. Use suggestion for an optional improvement. Warnings and suggestions do not require a response. Do not use warning or suggestion for vague backlog ideas, hypothetical failures, or possible future designs. Apply the trusted code change considerations before recommending a new configuration, migration, compatibility, extension, or abstraction layer.", "Finding eligibility: a finding must identify a concrete present behavioral, security, scope, or design defect in the checked-out PR, state the observed and expected states, cite a current file and line, and recommend the smallest current-PR action. For an unnecessary-complexity finding, the observed state must name the current owners, concepts, duplication, dependency widening, or churn. The expected state may be a lower-complexity coherent design grounded in a current owner, consumer, repository pattern, or policy; it does not require an externally visible behavior failure. Explain the maintenance cost that exists now and give a concrete behavior-preserving reduction. Requiring synchronized edits to two current implementations of one contract is a present defect, not a hypothetical future failure. PR-description or template compliance, checkbox selection, personal wording or naming preference, absence of an ordinary phrase from the controlled word list, a heuristic signal, a raw line count by itself, a hypothetical future failure without a present defect, or a possible risk not present in the diff is not a finding. A concrete violation of the trusted writing guide in changed text is eligible as a grouped suggestion when it cites representative changed lines and proposes a shorter rewrite. Describe its present reader impact. Set verificationHint to a read-only comparison of the cited text with the trusted writing guide. When automated coverage does not apply, set missingRegressionTest to `Not applicable: this finding concerns explanatory text.` Escalate the finding only when the wording can change behavior, security, data safety, a supported surface, test meaning, release meaning, or the interpretation of required evidence. An evidence-backed terminology ambiguity may be eligible only when it has one of those effects. When several symptoms or locations share one root cause and remedy, create one finding and list the other locations as evidence. PASS or positive observations, provider/SDK/advisor state, mere open-PR overlap or merge coordination, and live CI/E2E/check status belong only in positives or limitations. For redundancy or ownership findings, checked-out evidence must show that the current PR introduces or retains duplicate or conflicting ownership. This ownership requirement does not apply to independently supported correctness, security, scope, or other design defects. If a refreshed base only makes the PR unnecessary without leaving duplicate or conflicting code in the current diff, record a limitation instead of a finding. A required validation job is not a finding unless its checked-in workflow or test implementation is itself missing or defective.", ].join("\n");