PDX-473/471: fix(mcp) — all-level violations in stop decision; read-only diff

[mrdailey99]

Summary

Five correctness fixes surfaced during the 1.5.1 PR #172 release review.

• B1 testPlanValidate — calcNextAction(score, false) was called without remainingViolationCount, defaulting to 0. A plan whose test cases were structurally valid returned "stop" while PLAN-META-*, suite-level, or BP violations remained. Now passes the all-level violation count via new countAllPlanViolations / countSuiteViolations helpers.
• B2 testSuiteValidate — collectAllViolations collected tc.issues but not tc.best_practices_violations. A suite whose test cases failed only BP rules returned "stop" and an empty diff. Mirrors the testcase-level fix from PR PDX-470/471/473: feat(mcp): add detail level, diff mode, and completeness score to validation tools #168 review for the suite collector.
• B3 projectValidateFromPath — currentViolations (top-level project_violations only) was used for both the diff snapshot and the stop decision. A project with nested plan/suite violations but a clean root returned "stop". Diff snapshot is unchanged (stable response shape); stop decision now uses a new countAllProjectViolations helper.
• B4 projectValidateFromPath read-only diff — Baseline load was gated on save_results !== false, so a valid baseline_run_id returned BASELINE_NOT_FOUND in read-only mode. save_results should control persistence of the current run only, not whether existing runs can be read. Decoupled to match the testcase tool's pattern.
• B5 wireit cache — docs/PROVAR_TOOL_GUIDE.md was copied into lib/mcp/docs/ by compile but missing from wireit.compile.files inputs, so wireit could ship a stale guide after edits.

AC traceback

• PDX-473 (recommended_next_action correctness) — B1, B2, B3
• PDX-471 (baseline_run_id diff mode) — B4

Test plan

• Updated stale "stop when score=100" assertions in testPlanValidate.test.ts and testSuiteValidate.test.ts that were locking in the bug — these passed pre-fix because the broken stop decision matched the broken assertion
• New explicit B1 tests: plan with BP-violation-only TCs → NOT stop; plan with valid TC + missing metadata → NOT stop
• New explicit B2 test: suite with BP-violation-only TC → NOT stop
• New explicit B3 test: project with nested violations at completeness 100 → NOT stop
• New explicit B4 test: save_results=false + valid baseline_run_id → returns diff (not BASELINE_NOT_FOUND)
• yarn compile clean
• Full mocha run: 1143 passing / 0 failing
• yarn lint clean
• node scripts/mcp-smoke.cjs — 55/55 PASS

Notes for reviewer

• ValidatedTestCase (project-layer test case type) does not carry best_practices_violations; that's a pre-existing limitation of the project-validation service mapping. The B3 helper counts every other violation surface and falls back gracefully — explicit comment in countAllProjectViolations. Surfacing BP at the project layer is a follow-on (would belong in PDX-487 or similar).
• The renamed B1/B2 tests intentionally use assert.notEqual (not a positive stop assertion) because building a truly-zero-violations TC fixture in unit tests is brittle. The positive stop path is covered by validationScore.test.ts (calcNextAction(100, false, 0) === 'stop').

🤖 Generated with Claude Code

[github-actions]

Quality Orchestrator

🟢 LOW · 6 / 100 · All changed files have mapped tests.

---

🧪 Tests to Run · Running 3 of 51 tests

• unit/mcp/projectValidateFromPath.test.ts
• unit/mcp/testPlanValidate.test.ts
• unit/mcp/testSuiteValidate.test.ts

▶ Run command

npx vitest run \
  unit/mcp/projectValidateFromPath.test.ts \
  unit/mcp/testPlanValidate.test.ts \
  unit/mcp/testSuiteValidate.test.ts

---

_{⚡ quality-orchestrator  ·  /qo stub <file>  ·  qo analyze-local}

[Copilot]

Pull request overview

Fixes five correctness issues from the 1.5.1 release review of PR #172, primarily around the recommended_next_action stop signal incorrectly firing when violations remained at non-tested levels (plan metadata, suite-level, BP), and a read-only diff mode that erroneously required save_results !== false.

Changes:

• B1/B2/B3: Compute and pass an all-level violation count into calcNextAction for the plan, suite (via collectAllViolations adding best_practices_violations), and project validators so stop no longer fires while violations remain.
• B4: Decouple baseline read from save_results in projectValidateFromPath so a read-only diff against a saved baseline_run_id works.
• B5: Add docs/PROVAR_TOOL_GUIDE.md to wireit.compile.files so wireit invalidates the cache on edits.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/mcp/tools/testSuiteValidate.ts	collectAllViolations now includes per-tc best_practices_violations so they feed the stop-decision count.
src/mcp/tools/testPlanValidate.ts	New countSuiteViolations/countAllPlanViolations helpers; pass remaining count to calcNextAction.
src/mcp/tools/projectValidateFromPath.ts	New countAllProjectViolations helper; baseline load and hasAnyRun no longer gated on save_results.
package.json	Add docs/PROVAR_TOOL_GUIDE.md to wireit.compile.files.
test/unit/mcp/testSuiteValidate.test.ts	Replace stale stop-when-100 assertion with B2 BP-violations-remain notEqual('stop') test.
test/unit/mcp/testPlanValidate.test.ts	Rework stale test for B1 (BP remain) and add new B1 test for plan-metadata-remain.
test/unit/mcp/projectValidateFromPath.test.ts	Add B4 read-only diff test and B3 nested-violations stop-decision test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.