fix(reviewer-protocol): don't clear findings on unchanged callers of a changed method

.claude-plugin/marketplace.json

@@ -13,7 +13,7 @@
       "name": "pirategoat-tools",
       "source": "./plugins/pirategoat-tools",
       "description": "Code review orchestration (28 domain reviewers + pipeline/cross-validation agents), WordPress/WooCommerce development patterns, Figma-to-code workflow, accessibility guidance, testing patterns, and browser automation.",
-      "version": "1.111.1",
+      "version": "1.111.3",
       "author": {
         "name": "Vlad Olaru"
       },

plugins/pirategoat-tools/.codex-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "pirategoat-tools",
-  "version": "1.111.1",
+  "version": "1.111.3",
   "description": "Code review orchestration (28 domain reviewers + pipeline/cross-validation agents), WordPress/WooCommerce development patterns, Figma-to-code workflow, accessibility guidance, testing patterns, and browser automation.",
   "author": {
     "name": "Vlad Olaru",

plugins/pirategoat-tools/CHANGELOG.md

@@ -5,6 +5,51 @@ All notable changes to the pirategoat-tools plugin will be documented in this fi
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [1.111.3] - 2026-08-20
+
+Addresses a CodeRabbit review nit on the unchanged-caller exception added in
+1.111.2: the claim that symbol-aware search / AST analysis / repository
+indexes catch generated or string-built call sites overstated what those
+tools can do, risking a reviewer treating tool silence as proof a dynamic
+call site doesn't exist.
+
+### Fixed
+
+- **Caller-tracing guidance no longer overclaims tool coverage.** The
+  `reviewer-protocol.md` "unchanged caller" exception now states plainly
+  that symbol-aware search, AST analysis, and repository index tooling
+  reliably resolve statically-bound references (aliases, re-exports/imports,
+  overrides) but do not enumerate arbitrary runtime string dispatch or
+  generated code that isn't indexed. Reviewers now supplement with targeted
+  searches for registrations, callback wiring, dispatch strings, and
+  generator templates, and record any call site they can't resolve
+  statically as a verification gap rather than clearing the finding on
+  tool silence.
+
+## [1.111.2] - 2026-08-20
+
+Closes a reliability-reviewer blind spot where a callee's changed failure
+behavior left an unchanged, unguarded caller out of scope — a miss a human
+reviewer caught independently on WooCommerce PR woocommerce/woocommerce#67334
+after the agent had already found and then discarded the same call site.
+
+### Fixed
+
+- **Unchanged-caller blast radius, generalized beyond failure paths.** The
+  shared STOP CHECK's "unchanged caller" exception in `reviewer-protocol.md`
+  now fires for any behavior change in a hunk that could strand an unchanged
+  caller — return shape, side effects, ordering, validation — not only
+  failure/error-path changes. Reviewers trace reachable callers of a changed
+  function using symbol-aware search when available and anchor the finding
+  at the changed hunk, never at the unchanged caller.
+- **Under-leveled failure logging is a gap, not a positive.** reliability-reviewer's
+  "Observable" rule now flags a well-built catch block whose only signal is a
+  `debug`-level log call as a resilience gap for a silent, user-facing
+  degradation, unless an equivalent production-visible metric or alert already
+  covers it. Its False Positive Gate cross-references the shared exception
+  before clearing a resilience gap as "existing infrastructure unchanged by
+  this PR."
+
 ## [1.111.1] - 2026-07-29
 
 Closes an API contract review blind spot where a filter invocation could remain

plugins/pirategoat-tools/agents/reliability-reviewer.md

@@ -41,7 +41,7 @@ This review matters. A missed resilience gap becomes an outage.
 ## RULE 0 (MOST IMPORTANT): Every Production Code Path Must Have an Observable Failure Mode
 
 Every external call, state transition, and data mutation must be:
-1. **Observable** — logged or metriced so failures are detectable
+1. **Observable** — logged or metriced so failures are detectable. A catch block that only logs at `debug` is not observable for a failure that silently changes user-facing behavior (e.g. a fail-closed fallback to a degraded code path) — `debug` sits below the default log threshold on most installs and will not surface the regression, unless an equivalent production-visible metric or alert already detects the same degradation through another path. When logging is the only signal, match the log level to what breaks if nobody sees it: a silent behavioral degradation needs `error`/`warning`, not `debug`. Effective log thresholds vary by deployment — check what the target environment actually surfaces before asserting a level is invisible. This applies even when the surrounding try/catch itself is well-built — a good catch with an under-leveled log call is still a resilience gap, not a positive observation.
 2. **Recoverable** — error handling that allows graceful degradation
 3. **Reversible** — rollback path exists for state-changing operations
 
@@ -170,7 +170,7 @@ For each suspected gap, reason through:
 
 1. Is this a **concurrency correctness** issue? (Race conditions, TOCTOU, idempotency → concurrency-reviewer's domain.)
 2. Is this a **security vulnerability**? (Injection, XSS, auth bypass → security-reviewer's domain.)
-3. Is this **existing infrastructure** unchanged by this PR? (Only flag resilience gaps in changed code.)
+3. Is this **existing infrastructure** unchanged by this PR, AND does no changed hunk alter what happens when it fails? (Only flag resilience gaps caused by changed code — but a callee's new failure behavior can put an untouched caller in scope. Before clearing on this ground, apply the shared protocol's "unchanged caller" exception: trace reachable callers of any function whose failure behavior changed, and anchor the finding at the changed hunk, not at the caller.)
 4. Is the failure mode **already handled by the framework**? (e.g., WordPress catches fatal errors, WooCommerce has default retry logic — verify it's actually missing before reporting.)
 5. Is this a **style preference** about error message format without operational impact? (Inconsistent but functional error formats are LOW, not missing error handling.)
 

plugins/pirategoat-tools/agents/shared/reviewer-protocol.md

@@ -132,6 +132,12 @@ Both must be YES. Findings on unchanged code are false positives.
 
 **Exception — findings that are line-less BY NATURE.** Some legitimate findings have no line to anchor to: a whole changed file has no test coverage, a git-history precedent applies to the change, a cross-file architectural concern. For these, call `add_issue(..., line=None)` — the builder records a **file-scoped issue** (`line: null`, `scope: "file"`) that counts toward the verdict. Check 1 still applies: the file must be in `CHANGED_FILES`. Never use `line=None` for a point defect that has a line — that weakens verification downstream.
 
+**Exception — a changed hunk's new behavior reaching an unchanged caller.** "Unchanged code" means unchanged in isolation, not unaffected. If a hunk changes what a function *does* — its failure behavior (now throws where it previously returned a default, swallowed the error, or was caught internally), its return value or type, a side effect it performs, an ordering or timing guarantee, a validation rule, or any other externally observable contract — every caller reachable from that changed behavior is in scope, even a caller in a file with zero diff. This is not limited to error paths: a changed success-path return shape or a newly added side effect can strand an unchanged caller exactly as a changed failure path can. Anchor the finding at the **changed hunk that introduced the new behavior** (that file:line is in `CHANGED_FILES` and passes the STOP CHECK above), and describe the unguarded unchanged caller as blast radius in the finding body — never anchor the finding at the caller itself. Before clearing a suspected gap on "existing infrastructure unchanged by this PR," trace every reachable caller of the changed function using symbol-aware search, AST analysis, or repository index tooling when available — these reliably resolve statically-bound references such as aliases, re-exports/imports, and overrides, but they do not enumerate arbitrary runtime string dispatch or generated code that isn't indexed. Supplement with targeted searches for registrations, callback wiring, dispatch strings, and generator templates, and use `grep -rn` across the whole repo as a further supplemental pass — none of these are the sole method. Record any call site you cannot resolve statically as a verification gap rather than treating tool silence as proof of safety. Review each candidate call site before clearing the finding — do not clear on tool output alone. A caller's empty `git diff` proves the caller is unchanged; it does not prove the caller is safe against the callee's new behavior.
+
+<example type="FAILURE — this shipped a caller regression a reviewer had already found and then dismissed">
+A PR changed a context method from "fails silently, callers unaffected" to "throws and caches the exception." Two of its three call sites gained a new try/catch in the same PR; the third — in a file with zero diff — did not. A reviewer traced all three call sites, correctly identified the third as unguarded against the new throw, then discarded the finding because `git diff` on that third file returned no output ("pre-existing, out of scope"). The finding was correct and had already been made; it was cleared by checking the wrong file's diff. A human reviewer found the same call site independently and blocked the PR on it. The same reasoning would apply just as well if the hunk had instead changed the method's return shape, added a new side effect, or dropped a validation step the caller relied on — the failure-throw case above is one instance of the general pattern, not the boundary of it.
+</example>
+
 **CRITICAL — use SOURCE FILE line numbers only:**
 
 The Read tool's display numbers (e.g., `227→+class Foo`) are positions *within the patch file*. Use `@@ ... @@` hunk headers for source lines: