From e2e36b834ad3121d5476c2a22232d157494875cf Mon Sep 17 00:00:00 2001 From: Ahmed Date: Wed, 19 Aug 2026 16:10:30 +0100 Subject: [PATCH 1/4] fix(reviewer-protocol): don't clear findings on unchanged callers of a changed method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared "unchanged code = out of scope" rule was being applied literally to the call site, not to the callee's behavior. A caller can be textually unchanged and still be newly unsafe when a hunk elsewhere in the diff changes what the callee does on failure (e.g. it now throws where it used to fail silently). reliability-reviewer traced a real bug of exactly this shape to the correct file:line, then discarded it because the caller's own file showed no diff — the same bug a human reviewer later found and blocked a PR on. Adds an explicit exception to the shared STOP CHECK: anchor the finding at the changed hunk that introduced the new failure behavior (which is in-diff and passes the check), and describe the unguarded unchanged caller as blast radius rather than clearing the concern. Cross-referenced from reliability-reviewer's False Positive Gate, where the miss occurred. Also tightens RULE 0's "Observable" definition: reliability-reviewer had logged the same PR's fail-closed fallback path at debug level as a "Positive Observation" without questioning whether debug-level logging of a silent, user-facing degradation actually satisfies observability (it doesn't — debug sits below most installs' default log threshold). --- plugins/pirategoat-tools/agents/reliability-reviewer.md | 4 ++-- plugins/pirategoat-tools/agents/shared/reviewer-protocol.md | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/pirategoat-tools/agents/reliability-reviewer.md b/plugins/pirategoat-tools/agents/reliability-reviewer.md index 20b27b7c..3cca3f61 100644 --- a/plugins/pirategoat-tools/agents/reliability-reviewer.md +++ b/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. Match the log level to what breaks if nobody sees it: a silent behavioral degradation needs `error`/`warning`, not `debug`. 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.) diff --git a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md index 6d271219..0d662e21 100644 --- a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md +++ b/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 failure behavior reaching an unchanged caller.** "Unchanged code" means unchanged in isolation, not unaffected. If a hunk changes what a function does on failure — it now throws where it previously returned a default, swallowed the error, or was caught internally — every caller reachable from that changed behavior is in scope, even a caller in a file with zero diff. Anchor the finding at the **changed hunk that introduced the new failure 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 with `grep -rn` across the whole repo, not just the diff. 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. + + +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. + + **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: From e584d5825ebbbdb49b657287c54f657c66c589c1 Mon Sep 17 00:00:00 2001 From: Ahmed Date: Wed, 19 Aug 2026 17:19:13 +0100 Subject: [PATCH 2/4] fix(reviewer-protocol): address CodeRabbit round-2 nits on caller tracing and log-level rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Previous behavior: the debug-log rule was absolute (silent degradation always needs error/warning), and the caller-tracing rule prescribed grep-rn as the only method. - Problem: CodeRabbit flagged both as too rigid. A debug log can be fine if an equivalent production-visible metric/alert already covers the same degradation, and grep-rn alone misses aliases, re-exports, overrides, callbacks, and generated/string-built call sites that symbol-aware tooling would catch. - Solution: allow debug-level logging when an equivalent signal exists elsewhere, and note log thresholds vary by deployment; prefer symbol-aware search/AST/repo-index tooling for caller tracing, keeping grep-rn as a supplemental pass, with each candidate site reviewed before clearing. - Trade-offs: none — this tightens correctness of the guidance without changing the core requirement in either rule. Refs woocommerce/woocommerce#67334 --- plugins/pirategoat-tools/agents/reliability-reviewer.md | 2 +- plugins/pirategoat-tools/agents/shared/reviewer-protocol.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/pirategoat-tools/agents/reliability-reviewer.md b/plugins/pirategoat-tools/agents/reliability-reviewer.md index 3cca3f61..2ffd8ee1 100644 --- a/plugins/pirategoat-tools/agents/reliability-reviewer.md +++ b/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. 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. Match the log level to what breaks if nobody sees it: a silent behavioral degradation needs `error`/`warning`, not `debug`. 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. +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 diff --git a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md index 0d662e21..0e5e8e6e 100644 --- a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md +++ b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md @@ -132,7 +132,7 @@ 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 failure behavior reaching an unchanged caller.** "Unchanged code" means unchanged in isolation, not unaffected. If a hunk changes what a function does on failure — it now throws where it previously returned a default, swallowed the error, or was caught internally — every caller reachable from that changed behavior is in scope, even a caller in a file with zero diff. Anchor the finding at the **changed hunk that introduced the new failure 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 with `grep -rn` across the whole repo, not just the diff. 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. +**Exception — a changed hunk's new failure behavior reaching an unchanged caller.** "Unchanged code" means unchanged in isolation, not unaffected. If a hunk changes what a function does on failure — it now throws where it previously returned a default, swallowed the error, or was caught internally — every caller reachable from that changed behavior is in scope, even a caller in a file with zero diff. Anchor the finding at the **changed hunk that introduced the new failure 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 catch aliases, re-exports/imports, overrides, callbacks, and generated or string-built call sites that plain text search misses. Use `grep -rn` across the whole repo as a supplemental pass, not the sole method. 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. 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. From 02d2f1882c19677d6f6886371003ad2befd757d0 Mon Sep 17 00:00:00 2001 From: Ahmed Date: Thu, 20 Aug 2026 10:17:41 +0100 Subject: [PATCH 3/4] fix(reviewer-protocol): generalize unchanged-caller exception beyond failure behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vlad's review on #7 asked not to over-index on the failure-behavior scenario: the reviewer-protocol.md exception for tracing callers of a changed function only fired when the callee's *failure* behavior changed. The same review gap applies to any changed behavior a caller silently depends on — return shape, side effects, ordering, validation — so narrow it to failure paths and the next miss just gets a second bespoke exception bolted on. Generalized the exception's wording and its example to cover behavior changes broadly, with the failure-throw case framed as one instance rather than the boundary. Bumped pirategoat-tools to 1.111.2 and regenerated the Codex compat outputs. Co-Authored-By: Claude Code --- .claude-plugin/marketplace.json | 2 +- .../.codex-plugin/plugin.json | 2 +- plugins/pirategoat-tools/CHANGELOG.md | 24 +++++++++++++++++++ .../agents/shared/reviewer-protocol.md | 4 ++-- 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 3acca510..c833d29b 100644 --- a/.claude-plugin/marketplace.json +++ b/.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.2", "author": { "name": "Vlad Olaru" }, diff --git a/plugins/pirategoat-tools/.codex-plugin/plugin.json b/plugins/pirategoat-tools/.codex-plugin/plugin.json index 1ce6e4d4..451b261f 100644 --- a/plugins/pirategoat-tools/.codex-plugin/plugin.json +++ b/plugins/pirategoat-tools/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "pirategoat-tools", - "version": "1.111.1", + "version": "1.111.2", "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", diff --git a/plugins/pirategoat-tools/CHANGELOG.md b/plugins/pirategoat-tools/CHANGELOG.md index 41bcd099..5144c351 100644 --- a/plugins/pirategoat-tools/CHANGELOG.md +++ b/plugins/pirategoat-tools/CHANGELOG.md @@ -5,6 +5,30 @@ 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.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 diff --git a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md index 0e5e8e6e..36552e19 100644 --- a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md +++ b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md @@ -132,10 +132,10 @@ 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 failure behavior reaching an unchanged caller.** "Unchanged code" means unchanged in isolation, not unaffected. If a hunk changes what a function does on failure — it now throws where it previously returned a default, swallowed the error, or was caught internally — every caller reachable from that changed behavior is in scope, even a caller in a file with zero diff. Anchor the finding at the **changed hunk that introduced the new failure 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 catch aliases, re-exports/imports, overrides, callbacks, and generated or string-built call sites that plain text search misses. Use `grep -rn` across the whole repo as a supplemental pass, not the sole method. 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. +**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 catch aliases, re-exports/imports, overrides, callbacks, and generated or string-built call sites that plain text search misses. Use `grep -rn` across the whole repo as a supplemental pass, not the sole method. 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. -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. +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. **CRITICAL — use SOURCE FILE line numbers only:** From ff9ca3ea6a8f94957820a086b27d74f09eb385fd Mon Sep 17 00:00:00 2001 From: Ahmed Date: Thu, 20 Aug 2026 10:43:04 +0100 Subject: [PATCH 4/4] fix(reviewer-protocol): don't overclaim static tool coverage for caller tracing CodeRabbit flagged (PR #7, discussion_r3820242441) that the unchanged-caller exception's claim -- that symbol-aware search / AST analysis / repo index tooling "catch generated or string-built call sites" -- overstates what those tools can do. They reliably resolve statically-bound references but cannot enumerate arbitrary runtime string dispatch or unindexed generated code, so a reviewer could treat tool silence as proof a dynamic call site doesn't exist. Reworded to state the actual coverage boundary, added targeted-search guidance for registrations/dispatch strings/generator templates, and require recording unresolved dynamic call sites as verification gaps instead of clearing on tool output alone. Bumped pirategoat-tools to 1.111.3 and regenerated the Codex compat outputs. Co-Authored-By: Claude Code --- .claude-plugin/marketplace.json | 2 +- .../.codex-plugin/plugin.json | 2 +- plugins/pirategoat-tools/CHANGELOG.md | 21 +++++++++++++++++++ .../agents/shared/reviewer-protocol.md | 2 +- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index c833d29b..206911da 100644 --- a/.claude-plugin/marketplace.json +++ b/.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.2", + "version": "1.111.3", "author": { "name": "Vlad Olaru" }, diff --git a/plugins/pirategoat-tools/.codex-plugin/plugin.json b/plugins/pirategoat-tools/.codex-plugin/plugin.json index 451b261f..44bd1d6e 100644 --- a/plugins/pirategoat-tools/.codex-plugin/plugin.json +++ b/plugins/pirategoat-tools/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "pirategoat-tools", - "version": "1.111.2", + "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", diff --git a/plugins/pirategoat-tools/CHANGELOG.md b/plugins/pirategoat-tools/CHANGELOG.md index 5144c351..2d08229f 100644 --- a/plugins/pirategoat-tools/CHANGELOG.md +++ b/plugins/pirategoat-tools/CHANGELOG.md @@ -5,6 +5,27 @@ 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 diff --git a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md index 36552e19..c7b022ea 100644 --- a/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md +++ b/plugins/pirategoat-tools/agents/shared/reviewer-protocol.md @@ -132,7 +132,7 @@ 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 catch aliases, re-exports/imports, overrides, callbacks, and generated or string-built call sites that plain text search misses. Use `grep -rn` across the whole repo as a supplemental pass, not the sole method. 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. +**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. 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.