feat(fx-dev): add a materiality bar so reviews converge on what matters

[fx]

Summary

Review loops did not terminate. Every observation was treated as a finding, so each pass fixed what the last one raised and surfaced a fresh crop of smaller ones — the findings got less important, the passes kept costing the same, and the loop ended only when someone gave up.

Scope already answered what to review. Nothing answered what is worth reporting or when to stop. This adds a materiality bar to the shared scope contract that 17 skills import, and — after review of the change surfaced how much the rules had been copied around — restructures the review skills into one canonical procedure with seven adapters.

The rule

A finding is worth reporting when acting on it would change what the artifact does, or change how a competent reader acts on it. Blocking findings are reported individually; immaterial ones go in a single non-blocking closing note and never buy another pass. The test: if this shipped uncorrected, what breaks? If the answer is "nothing, it's just not as good", it's immaterial.

Three filters, in order — scope, then contract, then materiality. An out-of-scope finding is deferred however material it looks in isolation. Project rules, security and privacy invariants, and any other mandatory requirement the project wrote down — including a change document or a spec it links — block by virtue of being rules the project already decided mattered, so they never reach the bar at all. Materiality ranks only what survives both. Getting this order wrong was the single biggest source of contradictions found while reviewing this change.

Three shapes are not findings — generalized from the observed failure rather than baked in as examples: an entry missing from a list the artifact itself declares non-exhaustive (assess the rule; the supply of such entries never runs out, so these are not reported at any tier, not even the closing note), a preference disagreement with a decision the artifact records with its rationale (escalate once, don't re-argue), and a limit the artifact admits and gates. With an explicit carve-out: a recorded rationale does not make a decision safe — if the decision is the defect, it stays blocking however well reasoned.

Convergence is a ledger test, not a property of the latest pass: no blocking finding left unresolved. Not "zero output", which spends full cycles on wording, and not "a quiet pass" — a blocker carried from an earlier pass still blocks even if this pass didn't repeat it, and a finding that never produced a thread is never discharged by a thread count. There are exactly two exits: converged, or the same disagreement in successive passes (a human decision — escalate by name). A rising blocking count in one class is explicitly not an exit: the last fix is generating them, so fix the cause and keep looping, or the loop ends on a root fix nothing has reviewed.

Fix the class, not the instance. The single biggest cause of a loop that will not end: a reviewer reports the instance it happened to read, you fix exactly that, and the next pass finds a sibling and reports it as new. Every pass is productive and the loop still never terminates. Sweeps must be proven with a search whose empty result is the evidence.

One canonical procedure, seven adapters

Seven skills each carried their own copy of these rules. A copy drifts the moment the original changes, and a stale copy is reliably the narrower one — so it under-blocks, under-converges, or re-opens scope it should not. That was not hypothetical: the divergence was found and fixed repeatedly during this change, which is what motivated the restructure.

Layer	Holds
dev/references/scope-contract.md	Definitions — the Scope Brief, the bar, what blocking means, the three filters, resolver dispositions, convergence, the iteration bound, fix-the-class
fx-dev:review (new skill)	Procedure — Steps 1–8, how a reviewer applies those definitions from first pass to final report
The seven adapters	Only what is true of one tool: its command, flags, quirks, output shape, and the threads or gates it exposes

Every normative rule is defined once and referenced. One exception, and only one: text sent verbatim to an external tool. A prompt handed to codex review or an instructions file handed to cr cannot follow a link, so it must inline the rules — there is now exactly one such block, in fx-dev:review, marked as a mirror, and both adapters send it rather than keeping copies.

Per adapter

• codex-review — the scope prompt is mandatory (a run without it is an incomplete pass); MCP servers must be disabled per server by name or the run can hang forever with zero output; scope flags and a custom prompt are mutually exclusive; and the model and effort are pinned per invocation, because the inherited default made every pass a 10–15 minute wait and the convergence loop unaffordable long before its bound.
• coderabbit-review — cr review takes -c, --config <files...>, so it is prompt-capable and the brief reaches it before the review rather than only at triage. Fixed three flags this skill had been documenting incorrectly; anyone who ran the documented command got an error.
• copilot-review / copilot-feedback-resolver — the loop-specific trap: every push re-opens the gate, so editing for an immaterial finding costs a full wait cycle and manufactures the next round's input. Reply-and-resolve without a commit instead. Every thread still ends resolved — the gate is zero unresolved threads, not zero observations acted on. A [nitpick] prefix is an input to triage, never a verdict.
• rabbit-feedback-resolver / resolve-pr-feedback — a rejected suggestion routes through the incorrect handler and the REVIEW.md entry, which is the only thing that stops the finding recurring.
• pr-reviewer — state counts per tier; "a review that reports twenty things equally has reported nothing".
• dev — the bar applies to findings a reviewer originates from its own judgment, never to contract violations, so the coordinator can't demote something the reviewer correctly marked blocking.

Also adds "verify the premise before acting". A finding's premise can be wrong, and changing working artifacts to satisfy a misreading is worse than leaving the finding unaddressed — a suggestion applied on sight has been observed to introduce the very bug it claimed to report. A false premise is not a fourth disposition: there are three, and a thread whose premise fails carries none of them, which is what routes it to the outdated/incorrect handler.

Validation

The rules were tested on themselves. Fifteen local codex review passes, each carrying the Scope Brief and the prior passes' dispositions:

Passes	Findings	Character
1–11	9, 5, 6, 7, 5, 5, 2, 4, 2, 7, 4	Building the bar. Substantive contradictions throughout — filter precedence, leftover "fix everything / until clean" directives, half-swept classes
12–14	5, 4, 4	The restructure reviewed as a fresh artifact, looking specifically for a rule lost in the extraction. None was lost in any of the three
15	0	"The reviewed instruction artifacts are internally consistent, their canonical references resolve, and the new Codex model/effort guidance matches the documented CLI configuration mechanism"

All 69 findings were genuine; every one was fixed. None was a wording nitpick, and none was rejected as immaterial, deferred, or a false premise. For comparison, the change that motivated this ran twelve passes at 12, 13, 10, 5, 5, 5, 3, 4, 3, 3, 4, 6 and never converged — it plateaued, and the tail was list-completeness nitpicks plus one settled decision re-argued in new words each round.

Pass 15 covered every commit through the model pinning. Three commits postdate it: removing the benchmark table from codex-review (measurements are provenance and belong in git history, not in an instruction file an agent acts on), the path fix below, and the version collapse.

Copilot reviewed two heads and reported "no new comments" both times — while carrying five findings in its suppressed-comments blocks, which create no review threads and which no thread count would ever have surfaced. Four were valid and are fixed:

• dev/references/scope-contract.md resolved from nowhere — the odd one out of 31 citations repo-wide, and of the seven in that same file.
• The external mirror opened with CONVERGENCE PASS <N> while the prose said to send it verbatim on pass 1, so a first pass could either ask the reviewer to honour a list of prior findings that does not exist, or strip the prefix and hope. Now two labelled parts on two schedules, fixed across all four sites that describe sending it. Assembling this branch's own pass 15 prompt, I had to guess — which is what makes it blocking rather than cosmetic.
• coderabbit-review banned run_in_background outright because it "loses the script output", while dev Step 6.3 mode B backgrounds that exact waiter with stdout redirected to a log it reads afterwards. The rule was always about losing the output, not the process.

One is deferred, not fixed: resolve-pr-feedback's thread query pages at first: 100 with no pageInfo, so a PR with more than 100 threads could silently under-report. The finding is fair, but the cap is pre-existing on main and untouched by this change — adding pagination is a follow-up, not a widening of this one. Worth doing, since silent false convergence is exactly what this PR is about.

Version bump: fx-dev 3.8.2 → 3.9.0, marketplace 0.21.2 → 0.22.0 — minor, for a new skill and new mandatory rules with nothing removed. One step, not the per-commit walk to 3.14.1 the branch had accumulated: the pre-commit hook compares against the merge base with main, so a feature branch needs exactly one bump, and only the value landing on main signals anything to the plugin cache.

[Copilot]

Pull request overview

Adds a “materiality bar” to the shared scope contract so automated review loops converge on substantive issues (behavioral/correctness/security/contract violations) while batching immaterial observations into a single non-blocking closing note. This updates the core scope contract reference and wires the new triage/stop rules into the main review/resolution skills, along with a “verify before fixing” check to avoid churn from incorrect premises.

Changes:

• Extend scope-contract.md to define the materiality bar, filter precedence (scope → contract → materiality), and a convergence definition based on absence of material/substantive findings.
• Update reviewer/resolver skills (codex-review, copilot-review, coderabbit-review, pr-reviewer, resolve-pr-feedback, dev) to apply materiality in triage and redefine stopping conditions accordingly.
• Bump versions for fx-dev and the marketplace metadata to reflect the new mandatory review rule.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md	Adds materiality triage and updates convergence criteria for the resolve loop.
plugins/fx-dev/skills/pr-reviewer/SKILL.md	Instructs reviewers to rank findings by materiality and report tier counts.
plugins/fx-dev/skills/dev/SKILL.md	Applies materiality bar to reviewer-originated findings while preserving contract-required blocking rules.
plugins/fx-dev/skills/dev/references/scope-contract.md	Introduces the materiality bar, precedence rules, and convergence definition into the canonical contract.
plugins/fx-dev/skills/copilot-review/SKILL.md	Adds materiality-aware triage and convergence definition for Copilot-driven loops.
plugins/fx-dev/skills/codex-review/SKILL.md	Shifts from “until clean” to “until converged” and adds a convergence-pass prompt block.
plugins/fx-dev/skills/coderabbit-review/SKILL.md	Updates local/PR-level CodeRabbit guidance to fix only material/substantive findings and converge.
plugins/fx-dev/.claude-plugin/plugin.json	Bumps fx-dev version to 3.9.0.
.claude-plugin/marketplace.json	Bumps marketplace metadata version to 0.22.0.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

Suppressed comments (1)

plugins/fx-dev/skills/dev/SKILL.md:321

• The new materiality paragraph says reviewer-originated findings must clear the bar “before they can be classified”, but the ledger’s classes are defined earlier as required-by-contract / regression-caused-by-change / follow-up/out-of-scope. As written, it’s unclear whether “classified” means any ledger classification or specifically the two blocking classes, and it also reads ungrammatically (“findings a reviewer originates”). Clarifying that the bar gates only the two blocking classes will prevent mis-triage (e.g., treating immaterial observations as needing a blocking-class classification).

**Findings a reviewer originates from its own judgment must clear the materiality bar before they can be classified** (`references/scope-contract.md`). An observation that would change nothing if it shipped uncorrected is neither `required-by-contract` nor `regression-caused-by-change` — it is recorded in the ledger as immaterial and never triggers a rerun.

**The bar does NOT apply to violations of a mandatory rule.** Anything Step 2.5 defines as `required-by-contract` — project, security, privacy, test and merge rules the project wrote down — is blocking by virtue of being a rule, whatever its direct behavioural impact. The contract filter runs before the bar, exactly as `references/scope-contract.md` § Three filters specifies, so the coordinator can never demote a violation the reviewer correctly marked blocking. Two shapes in particular are non-findings and MUST NOT enter the ledger as blocking: a missing entry in a list the artifact declares non-exhaustive, and a decision the artifact records with its rationale. The second, raised twice, is an escalation to the user rather than a third cycle.

[Copilot]

Pull request overview

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

Suppressed comments (2)

plugins/fx-dev/skills/coderabbit-review/SKILL.md:17

• This still says to “Resolve all actionable findings” before opening the PR, but the skill now defines convergence as “no material or substantive findings” and allows remaining immaterial observations to be carried as a closing note. Keeping the older “actionable” wording can reintroduce a fix-everything interpretation and churn.

- Open the PR once the local review has **converged** — no material or substantive findings — **or is correctly degraded as `skipped (rate-limited)`**. Resolve all actionable findings already received before proceeding.

plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:206

• The convergence condition here can be satisfied with zero unresolved threads even if Copilot’s latest review body contains new findings in the “Suppressed comments” block (which produces no threads). That contradicts this skill’s success criteria (#1) requiring suppressed comments for the current head to be included/triaged, and could cause premature convergence.

6. Stop when a pass produces **no new material or substantive findings** on a head SHA that was actually reviewed (verify: the newest Copilot review's `commit_id` equals `headRefOid`) **and every automated thread on that head is resolved**. Immaterial threads resolved by reply satisfy this — they are not "new feedback" for the purpose of another cycle, because actioning them would produce a push and therefore manufacture the next round's input. Cap at 4 outer iterations and escalate to the user if not converged.

[Copilot]

Pull request overview

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

Suppressed comments (4)

plugins/fx-dev/skills/dev/SKILL.md:321

• These two paragraphs read as contradictory: the first says the materiality bar gates entry into required-by-contract, but the next says the bar does not apply to mandatory-rule violations that are also classified required-by-contract. Clarify that the bar only gates judgment-originated findings before they can be treated as blocking; rule violations remain blocking regardless of materiality.

**A finding that a reviewer raised on its own judgment must clear the materiality bar before it can enter either BLOCKING class** — `required-by-contract` or `regression-caused-by-change` (`references/scope-contract.md`). The bar gates those two classes only; it does not keep anything out of the ledger. An observation that would change nothing if it shipped uncorrected is still recorded, classified immaterial, and never triggers a rerun.

**The bar does NOT apply to violations of a mandatory rule.** Anything Step 2.5 defines as `required-by-contract` — project, security, privacy, test and merge rules the project wrote down — is blocking by virtue of being a rule, whatever its direct behavioural impact. The contract filter runs before the bar, exactly as `references/scope-contract.md` § Three filters specifies, so the coordinator can never demote a violation the reviewer correctly marked blocking. Two shapes in particular are non-findings and MUST NOT enter the ledger as blocking: a missing entry in a list the artifact declares non-exhaustive, and a decision the artifact records with its rationale. The second, raised twice, is an escalation to the user rather than a third cycle.

plugins/fx-dev/skills/dev/references/scope-contract.md:75

• The materiality section defines a reportable finding as one where acting on it changes behavior/reader action, but the very next section establishes a contract filter where mandatory-rule violations are blocking regardless of behavioral impact. To avoid readers applying the first sentence too broadly, scope the definition to the post-scope/post-contract set (judgment-originated findings) so it matches the three-filter precedence described below.

**A finding is worth reporting when acting on it would change what the artifact does, or change how a competent reader acts on it. Everything else is an observation, not a finding.**

plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:26

• This line calls below-the-bar items “findings”, but the materiality bar definition in scope-contract.md explicitly defines those as “observations, not findings”. Using consistent terminology here matters because this skill’s loop logic hinges on whether something is a new finding that should drive another cycle.

An in-scope finding that would change nothing if it shipped uncorrected — wording, formatting, a count nothing keys on, an entry missing from a list the artifact itself declares non-exhaustive — is **resolved by replying with that reasoning**, not by editing. Every thread still ends resolved; the gate is zero *unresolved* threads, not zero observations acted on.

plugins/fx-dev/skills/copilot-review/SKILL.md:105

• This paragraph refers to below-the-bar items as “findings”, but the scope contract now defines those as “observations, not findings”. Aligning the terminology here helps prevent reviewers from treating immaterial items as “new findings” that require a push/re-review cycle.

Triage by **materiality** as well as scope — see the materiality bar in the same
reference. An in-scope finding that would change nothing if it shipped
uncorrected is **resolved by replying with that reasoning**, not by editing. The
gate is zero *unresolved* Copilot threads, not zero observations acted on.

[Copilot]

Pull request overview

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

Suppressed comments (2)

plugins/fx-dev/skills/coderabbit-review/SKILL.md:122

• This step says "Do not open the PR with known unresolved actionable findings", which is ambiguous/inconsistent with the new definition of convergence (material/substantive findings block; immaterial observations travel as a closing note). Tightening this wording helps prevent reintroducing the "fix until silent" loop.

- When you stop, report the per-pass trend and whether the last round's fixes were themselves reviewed.

### Step 4: Open the PR when clean or correctly degraded

A clean local CodeRabbit review is preferred before PR creation. A rate-limited review is correctly degraded and does not block PR creation once all findings already received are addressed. Do not open the PR with known unresolved actionable findings.

plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:24

• This paragraph says “a project-rule or security violation blocks regardless of the bar”, but omits privacy. Since the contract filter explicitly treats privacy invariants as blocking, this wording could cause a resolver to incorrectly triage privacy issues as non-blocking/immaterial.

Scope decides whether a finding is *ours*. Materiality decides whether it is *worth a fix* — see the materiality bar in `fx-dev/skills/dev/references/scope-contract.md`, which also fixes the order: **scope, then contract, then materiality**. An out-of-scope finding is deferred however material it looks, and a project-rule or security violation blocks regardless of the bar. Materiality ranks only what survives both.

[Copilot]

Pull request overview

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

Suppressed comments (1)

plugins/fx-dev/skills/dev/references/scope-contract.md:75

• This sentence is grammatically ambiguous: “...one is worth reporting...” can read as “only one finding is worth reporting,” which conflicts with the intended rule and could mislead reviewers applying the contract. Rephrase to clearly state that a reviewer-originated finding is worth reporting when it meets the bar.

**Of the findings a reviewer originates from its own judgment — that is, what survives the scope and contract filters below — one is worth reporting when acting on it would change what the artifact does, or change how a competent reader acts on it. Everything else is an observation, not a finding.**

[Copilot]

Pull request overview

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

Suppressed comments (1)

plugins/fx-dev/skills/review/SKILL.md:29

• The reference dev/references/scope-contract.md doesn’t match an actual path in this repo (the canonical file is under fx-dev/skills/dev/references/…). This makes the “define-once” pointer hard to follow for readers and risks link-rot in future edits.

This skill is the application of `dev/references/scope-contract.md`
§ Blocking's define-once rule to the review skills themselves.

[Copilot]

Pull request overview

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

Suppressed comments (4)

plugins/fx-dev/skills/coderabbit-review/SKILL.md:165

• This section says not to use run_in_background because it loses the waiter output, but fx-dev:dev Mode B explicitly backgrounds the CodeRabbit waiter with output redirected to a log file and later read. Either the prohibition here needs to be qualified (background is OK with redirection), or the Mode B recipe becomes invalid.

**⚠️ Do NOT use `run_in_background`** — that loses the script output and breaks
the cycle.

plugins/fx-dev/skills/review/SKILL.md:315

• The external-reviewer block includes the “CONVERGENCE PASS” preamble, but the surrounding text implies it’s only for re-runs. Since the block is meant to be sent verbatim, clarify how pass 1 should populate (or zero) that preamble to avoid contradictory guidance (also referenced by fx-dev:codex-review).

Send it on **every** pass, the first as well as every re-run. Pass 1 is where a
reviewer with no bar produces the largest crop of low-value findings, so omitting
it there costs the most. On a re-run the CONVERGENCE PASS preamble goes on top.

plugins/fx-dev/skills/codex-review/SKILL.md:233

• This says the CONVERGENCE PASS preamble is added “from pass 2”, but fx-dev:review’s external-reviewer block currently includes that preamble as part of the verbatim text. Align this guidance so the prompt assembly is unambiguous for pass 1.

Send **`fx-dev:review` § The external-reviewer block, verbatim**, as the last part
of every prompt — the first pass as well as every re-run. That block is the single
mirror both external reviewers use; this skill does not keep its own copy, and
must not paraphrase it. Its CONVERGENCE PASS preamble goes on top from pass 2.

plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:103

• This skill claims to find every unresolved automated thread, but the GraphQL query only fetches the first 100 reviewThreads and provides no way to detect pagination. On large PRs (or when multiple bots comment heavily), this can miss unresolved threads and falsely report convergence. Consider adding pageInfo (and looping with after: endCursor when hasNextPage is true).

      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          path

[Copilot]

Pull request overview

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