fix(advisor): require reduction-directed design blockers (#9631)

<!-- markdownlint-disable MD041 -->
## Summary

The PR Review Advisor should block material codebase-design defects
without sending authors through review-driven growth loops. This change
keeps the evidence and anti-noise safeguards from #6682 and #6832, makes
maintainability defects first-class blockers, and requires simplicity
feedback to point toward a smaller complete source-and-test design.

## Changes

- Define duplicated authority, unnecessary machinery, repeated setup,
widened dependencies, unrelated churn, and unsupported product scope as
present design defects when checked-in evidence supports a
behavior-preserving reduction.
- Require blocker severity when the advisor expects author action before
merge. Keep warnings optional only when maintainers can reasonably
accept the current design without author action.
- Require an unnecessary-complexity finding to name its reduction case
across source and tests. Prefer a negative total line delta; permit a
line-neutral result only for a material reduction in owners, concepts,
invalid combinations, or dependency width.
- Reject net-new helpers, registries, configuration surfaces,
compatibility layers, fallbacks, migrations, test frameworks, and
parallel fixture owners as simplification remedies unless current
consumers adopt them and the complete change removes more structure than
it adds.
- Preserve semantic regression coverage while allowing duplicated
fixtures, matrices, assertions, and test owners to be consolidated.
- Keep follow-on findings grounded in current code, scope ownership
findings to checked-in duplication, and classify merge-order-only
redundancy as superseded or a limitation.
- Keep a prior design root cause stable when a review response adds
machinery instead of creating a serial chain of architecture findings.
- Apply the same deletion and consolidation questions in the shared
code-change considerations and the issue implementation skill so agents
optimize the design before publication.
- Add prompt, shared-guidance, and skill contract coverage for the new
direction and retained anti-noise rules.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## DGX Station Hardware Evidence

- [ ] Tested on DGX Station
- Tested commit:
- Station profile/scenario:
- Result:
- Supporting evidence:

## Verification

- [x] PR description includes a `Signed-off-by:` line and every commit
appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run validate:pr` passed after refreshing `origin/main` when hooks
were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — `npm exec -- vitest run
test/code-change-considerations.test.ts
test/pr-review-advisor-writing-guides.test.ts
test/pr-review-advisor-context.test.ts test/skills-frontmatter.test.ts`
(82 passed); `npm run test:changed` (growth guardrails: 32 passed; no
changed CLI, plugin, or E2E-support tests)
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new doc pages
only)

---
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Improvements**
* Review guidance now more consistently identifies material design,
security, authorization, scope, duplication, and complexity issues as
blockers when supported by evidence.
* Simplification recommendations favor reducing source and test
complexity, avoiding added mechanisms and unsupported future-use
justifications.
* Checks better distinguish current defects from hypothetical risks,
resolved behavior, and non-actionable concerns.
* Terminology reviews and retry handling now provide clearer, more
reliable outcomes.

* **Documentation**
* Clarified expectations for documenting reductions, line changes,
preserved regression coverage, ownership evidence, and independent
follow-on risks.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

Author: jyaunches

.agents/skills/_shared/code-change-considerations.md

@@ -17,13 +17,22 @@ recording them here.
 
 - What accepted outcome and current consumer require the change?
 - What current code owns the behavior, and can that owner be extended directly?
+- What current code, branch, parameter, owner, fixture, or file becomes unnecessary and can be
+  deleted or merged in this change?
 - Would the change duplicate an existing structure or create another source of truth?
+- Can the completed source-and-test change be neutral or negative in total lines? If not, what
+  current correctness, security, or accepted-scope contract requires the growth?
+- If the change adds a helper, abstraction, configuration, registry, fallback, or compatibility
+  path, which current consumers adopt it now, what old structure does it remove, and is the whole
+  result smaller or simpler?
 - What state, success, failure, and partial-failure behavior must remain coherent?
 - What ordering or concurrency can change the result or bypass a guarantee?
 - How do absent values, defaults, retries, recovery, and cleanup behave?
 - Which alternate entry, error, cached, resumed, or compatibility paths can bypass the change?
 - Can code or configuration be removed, or can an existing or native mechanism replace new code?
 - What shortest stable test proves the changed behavior, including the relevant negative path?
+- Can that evidence extend or consolidate current fixtures, matrices, and assertions instead of
+  creating another test owner or a one-use test helper?
 - Does a real process, network, filesystem, container, hardware, or service boundary require deeper
   runtime or end-to-end evidence?
 - Which active issues, pull requests, or recent changes overlap, conflict, or affect delivery order?

.agents/skills/nemoclaw-contributor-implement-issue/SKILL.md

@@ -80,13 +80,25 @@ checked for that operation or failure class and the sensitive-workflow state out
 hold. Then make the direct change in the current behavior owner. Do not add speculative abstractions,
 configuration, compatibility, migration, or fallback behavior.
 
+Optimize the complete source-and-test change for deletion and consolidation. Compare a direct edit,
+reuse of an existing owner, and a refactor of current related code. Prefer a neutral or negative
+total line delta. Add a helper, abstraction, configuration surface, registry, fallback, or
+compatibility path only when current consumers adopt it in this change and the complete result
+removes more owners, concepts, branches, or lines than it adds. Possible future reuse is not enough.
+When a current correctness, security, or accepted-scope contract requires growth, keep the design
+direct and record why deletion or reuse cannot satisfy it.
+
 Add focused evidence as applicable:
 
 - positive behavior that must succeed;
 - negative or denied behavior that must fail;
 - error, interruption, recovery, or cleanup behavior;
 - boundary values, ambiguous state, and alternate entry paths.
 
+Preserve semantic regression coverage, not every existing fixture or assertion block. Extend or
+table-drive current coverage when that keeps one setup and one behavior owner. Do not create a
+one-use test helper, parallel matrix, or second test file merely to shorten an individual test.
+
 State why an evidence category does not apply when omission could hide risk. Use runtime or end-to-end
 evidence only when the real process, filesystem, network, container, hardware, workflow, or service
 boundary owns the behavior. Run focused tests after the final behavior-affecting edit and record the
@@ -103,6 +115,11 @@ Consideration and all nine Security Rubric categories to the completed behavior.
 security control and focused negative evidence that proves forbidden behavior remains denied. If no
 security control changed, state why and cite the reviewed trust boundaries.
 
+Record the reduction case for the completed design: the current code, owners, branches, parameters,
+fixtures, or files deleted or consolidated and the total source-and-test line direction. If the
+change grew, identify the current contract that requires that growth. Remove review-driven machinery
+when a direct solution is smaller; do not add another layer to compensate for an avoidable layer.
+
 Confirm that allowed, denied, error, and boundary behavior remains coherent across failure, retry,
 cleanup, cached, resumed, and compatibility paths that apply.
 Re-check the recorded operation and failure class, sibling paths, and sensitive-workflow state
@@ -121,6 +138,7 @@ Use this structure:
 - Accepted scope authority:
 - Delivered capability:
 - Changed behavior:
+- Simplification result: <deleted or consolidated structure, total line direction, and required-growth justification when applicable>
 - Deferred scope:
 
 ## Changed files

test/code-change-considerations.test.ts

@@ -63,6 +63,12 @@ describe("shared code change considerations", () => {
     try {
       process.chdir(untrustedCheckout);
       expect(readTrustedCodeChangeConsiderations()).toContain("shortest stable test");
+      expect(readTrustedCodeChangeConsiderations()).toContain(
+        "neutral or negative in total lines",
+      );
+      expect(readTrustedCodeChangeConsiderations()).toContain(
+        "what old structure does it remove",
+      );
       expect(readTrustedCodeChangeConsiderations()).not.toContain("Ignore the trusted resource");
       expect(buildSystemPrompt().match(/# Code Change Considerations/gu)).toHaveLength(1);
     } finally {

test/pr-review-advisor-context.test.ts

@@ -142,11 +142,29 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
     expect(investigate?.prompt).toContain("never commands");
     expect(investigate?.prompt).toContain("direct change in the current design");
     expect(investigate?.prompt).toContain("neutral or negative net lines");
+    expect(investigate?.prompt).toContain("account for source and tests together");
+    expect(investigate?.prompt).toContain("Prefer a negative total line delta");
+    expect(investigate?.prompt).toContain(
+      "If the proposed remedy increases net complexity or merely introduces another mechanism without consolidating current structure, do not call it simplification",
+    );
+    expect(investigate?.prompt).toContain(
+      "Accept a new helper or abstraction only when current consumers adopt it in this change and the combined source-and-test structure materially decreases",
+    );
     expect(investigate?.prompt).toContain("new pattern applied to current related code");
     expect(investigate?.prompt).toContain(
       "Report all currently visible, evidence-backed recommendations in this stage's single ledger batch",
     );
-    expect(investigate?.prompt).toContain("rescan the resulting design");
+    expect(investigate?.prompt).toContain("rescan for follow-on risks");
+    expect(investigate?.prompt).toContain(
+      "A design finding does not require a runtime failure when the current code proves that cost",
+    );
+    expect(investigate?.prompt).toContain(
+      "classify the finding as blocker instead of downgrading it because behavior passes",
+    );
+    expect(investigate?.prompt).toContain(
+      "Include a follow-on finding only when the current diff or surrounding current code independently proves the defect",
+    );
+    expect(investigate?.prompt).toContain("non-finding investigation note");
     expect(investigate?.prompt).toContain("Never simplify away trust-boundary validation");
     expect(investigate?.prompt).not.toContain("<pr_review_advisor_json>");
     expect(turns.every((turn) => !turn.prompt.includes(poisonedDiff))).toBe(true);
@@ -175,7 +193,32 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts
     expect(challenge?.prompt).toContain("Turn 2/2 — challenge-and-record");
     expect(challenge?.prompt).toContain("Challenge the investigation receipt before recording");
     expect(challenge?.prompt).toContain("Then dedupe");
+    expect(challenge?.prompt).toContain(
+      "Do not remove a design finding because behavior passes",
+    );
+    expect(challenge?.prompt).toContain("If the author should change the PR before merge");
+    expect(challenge?.prompt).toContain(
+      "Require every unnecessary-complexity finding to carry a reduction case",
+    );
+    expect(challenge?.prompt).toContain(
+      "Reject a proposed simplification that increases net complexity",
+    );
+    expect(challenge?.prompt).toContain(
+      "Allow a helper or abstraction only when current consumers adopt it now and the combined source-and-test structure materially decreases",
+    );
+    expect(challenge?.prompt).toContain(
+      "hypothetical future failures without a present defect",
+    );
     expect(challenge?.prompt).toContain("Then batch-record in this exact sequence");
+    expect(challenge?.prompt).toContain(
+      "Drop an unverifiable terminology decision instead of rephrasing it",
+    );
+    expect(challenge?.prompt).toContain(
+      "using `submit_review` retries to discover the mismatch",
+    );
+    expect(challenge?.prompt).toContain(
+      "Set terminologyReview.noChangesReason only when decisions is empty",
+    );
     expect(challenge?.prompt.indexOf("record_findings")).toBeLessThan(
       challenge?.prompt.indexOf("record_review_receipt") ?? -1,
     );

test/pr-review-advisor-writing-guides.test.ts

@@ -111,6 +111,63 @@ describe("PR review advisor writing guides", () => {
     expect(prompt).toContain("A Refs, Related, or Follow-up link does not commit the PR");
     expect(prompt).toContain("When several symptoms or locations share one root cause and remedy");
     expect(prompt).toContain("suggestion renders as 'Suggestion'");
+    expect(prompt).toContain("The controlled word list is not a general dictionary");
+    expect(prompt).toContain(
+      "absence of an ordinary phrase from the controlled word list",
+    );
+  });
+
+  it("treats material present design defects as blockers", () => {
+    const prompt = buildSystemPrompt();
+
+    expect(prompt).toContain(
+      "present behavioral, security, scope, or material codebase-design defect",
+    );
+    expect(prompt).toContain(
+      "If a finding asks the author to change code before merge, classify it as blocker",
+    );
+    expect(prompt).toContain(
+      "Passing tests or currently matching outputs do not downgrade duplicated authority",
+    );
+    expect(prompt).toContain("basis.kind=unnecessary_complexity");
+    expect(prompt).toContain("does not require an externally visible behavior failure");
+    expect(prompt).toContain(
+      "Requiring synchronized edits to two current implementations of one contract is a present defect",
+    );
+    expect(prompt).toContain("a raw line count by itself");
+    expect(prompt).toContain("mere open-PR overlap or merge coordination");
+    expect(prompt).toContain(
+      "For redundancy or ownership findings, checked-out evidence must show that the current PR introduces or retains duplicate or conflicting ownership",
+    );
+    expect(prompt).toContain(
+      "This ownership requirement does not apply to independently supported correctness, security, scope, or other design defects",
+    );
+    expect(prompt).not.toContain(
+      "A finding is eligible when checked-out evidence shows that the current PR introduces or retains duplicate or conflicting ownership",
+    );
+    expect(prompt).toContain(
+      "If a refreshed base only makes the PR unnecessary without leaving duplicate or conflicting code",
+    );
+    expect(prompt).not.toContain("refreshed base already contains equivalent behavior");
+    expect(prompt).toContain("use recommendation=superseded");
+    expect(prompt).toContain(
+      "missing that authorization is a current scope defect, not template noncompliance",
+    );
+    expect(prompt).toContain("Duplicated test setup, parallel test owners");
+    expect(prompt).toContain(
+      "Preserve semantic regression coverage and necessary boundary evidence",
+    );
+    expect(prompt).toContain(
+      "produce a reduction case that names the current code, owners, concepts, branches, parameters, fixtures, or files",
+    );
+    expect(prompt).toContain("Prefer a negative total line delta");
+    expect(prompt).toContain(
+      "A positive line result is not a simplification finding",
+    );
+    expect(prompt).toContain("Future reuse, aesthetic symmetry");
+    expect(prompt).toContain(
+      "Do not create a serial chain of new architecture findings",
+    );
   });
 
   it("documents the same-session conversation contract", () => {

test/skills-frontmatter.test.ts

@@ -260,6 +260,12 @@ describe("repo skill markdown files", () => {
     expect(skill).toContain("../_shared/security-rubric.md");
     expect(skill).toContain("../_shared/documentation-writing-review.md");
     expect(skill).toContain("smallest independently valuable capability slice");
+    expect(skill).toContain("Prefer a neutral or negative");
+    expect(skill).toContain("total line delta");
+    expect(skill).toContain("Possible future reuse is not enough");
+    expect(skill).toContain("Preserve semantic regression coverage");
+    expect(skill).toContain("Record the reduction case for the completed design");
+    expect(skill).toContain("Simplification result:");
     expect(skill).toContain("Read current code, tests, workflows");
     expect(skill).toContain("Load a narrow specialist only");
     expect(skill).toContain("it does not authorize GitHub writes");

tools/pr-review-advisor/README.md

@@ -19,7 +19,12 @@ It complements the existing PR surfaces by keeping a NemoClaw maintainer code-re
 - 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;
 - 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;
+  safe deletion, consolidation, existing or new patterns, and neutral or negative net-line outcomes.
+  Present design defects can block when checked-in evidence shows duplicated ownership, unnecessary
+  machinery, substantial repeated setup, widened dependencies, or unrelated churn and the review
+  provides a concrete behavior-preserving reduction. The reduction case covers source and tests
+  together, defaults to a negative total line outcome, and may be line-neutral only when it
+  materially reduces owners, concepts, invalid combinations, or dependency width;
 - 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
@@ -214,5 +219,13 @@ It reserves `info_only` for skipped, unavailable, incomplete, or low-confidence
 `superseded` when competing work replaces the PR.
 These recommendations describe advisor findings only.
 They never approve a PR, replace required human review, or change the repository's merge gates.
-Maintainers still decide whether a warning blocks, and suggestions do not require a response.
+Warnings identify concerns that maintainers can accept without author action. Suggestions identify
+optional improvements. Required design work must be a blocker instead of a warning.
+An unnecessary-complexity blocker must remove or consolidate current structure. A helper or
+abstraction is eligible only when current consumers adopt it and the combined source-and-test
+structure materially decreases. Other recommendations that increase net complexity or merely add a
+registry, configuration surface, compatibility layer, fallback, migration path, test framework, or
+fixture owner require an independent correctness, security, or accepted-scope defect; they are not
+presented as simplification. This keeps architecture feedback strong while preventing review-driven
+growth and serial refactoring layers.
 Every result includes limitations and requires maintainer review.