Skip to content

Agents include non-finding findings in review #784

Description

@williamthorsen

Problem

Code review agents frequently emit "non-finding findings" — numbered F/W/T/R/S findings whose own body disqualifies them. Five recurring shapes, each captured as a codeassembly KB event:

  • Self-defeating hedge — the finding names the condition under which it would matter, and that condition isn't currently met ("call it out only if X", "consider when Y", "would matter once Z").
  • Non-actionable conclusion — the body ends "no action this PR / not actionable here / just capturing a thought".
  • Argues against itself — the body endorses the current state ("the current shape is correct") and then proposes a change anyway.
  • Below the value bar — trivially actionable but not worth reader attention, hedged instead of cut.

These dilute the signal of genuine findings and impose a cascading cost: reader time, tokens spent asking the agent to reconsider, author triage, and every later reader who reads both the finding and its rejection.

Context

  • The finding scheme (F/W/T/R/S) is defined once in skills/_data/artifact-conventions.md §"Finding scheme" and referenced by review-criteria, review-branch, review-pr, respond-to-review, and the five reviewer subagents.
  • The existing guidance is demonstrably too weak: artifact-conventions.md already says "every finding must include a concrete action," and all five reviewer subagents carry an identical "Only actionable findings" principle — yet the captured misses slipped past exactly that wording. The unaddressed gap is the self-disqualifying hedge pattern, and treating hedging language as a softening device rather than a delete trigger.
  • Delivery-path asymmetry: the five reviewer subagents receive review-criteria injected verbatim via skills: frontmatter but only link to the category criteria in artifact-conventions. Guidance placed only in the canonical file reaches the orchestrated path solely if a subagent chooses to read the link.
  • The five events live in the codeassembly-kb vault and should be threaded to this correction via the addressed-by relation (Add an addressed-by/addresses relation linking problems to their responses #763). The writer for that relation is Add a kb-edit operation to append addressed-by references to records #785, not yet landed.

Proposed solution

Sharpen the actionability bar into a named Actionability gate and place it so every review surface, interactive and orchestrated, carries it in context:

  • Canonical — a full "Actionability gate" subsection in artifact-conventions.md §"Finding scheme": the action must be takeable in the change under review; the cascade-cost framing; hedging language is a delete trigger, not a softening device, with the list of disqualifying tells; the "would I make this exact change right now if it were my code?" self-test; where dropped content is routed; and that the gate applies hardest to R and S. The R and S category Gate: bullets link to it.
  • Operational, injected — a tight form of the gate in the review-criteria skill body (not merely a link), so the five reviewer subagents receive it verbatim and the interactive path reads it directly.
  • Emission-site reminder — a new shared partial that sharpens the "Only actionable findings" principle to name the hedge-as-delete tell, included into all five reviewer subagents' Principles blocks (consistent with the existing _partials/review-writes-* partials).

Separately, once #785 lands the addressed-by writer, mark the five KB events addressed-by this correction (commit/PR ref) and refresh the vault's stale local schema so the write validates. This happens in the codeassembly-kb repo, outside this PR.

Acceptance criteria

Must have

  • artifact-conventions.md §"Finding scheme" defines an "Actionability gate" covering: the action must be takeable in the change under review; hedging language is a delete trigger (with the list of disqualifying tells); the self-test; where dropped content is routed; and that the gate applies most strictly to R and S.
  • The R and S category Gate: bullets reference the Actionability gate.
  • The review-criteria skill body carries an operational form of the gate, so the copy injected into the reviewer subagents includes it without a fetched link.
  • All five reviewer subagents (orchestrated-reviewer, aspect-code-reviewer, aspect-silent-failure-reviewer, aspect-test-reviewer, code-simplification-reviewer) carry the sharpened emission-site reminder via a shared partial, and their installed output includes it.
  • Content changes pass existing validation: no dangling partial includes, agents build/install succeeds, lint and format checks pass.

Must have — blocked on #785, lands in codeassembly-kb

  • The five codeassembly-kb events are marked addressed-by this correction's commit/PR reference.
  • The codeassembly-kb local .kb/schema.yaml declares addressed-by on event and assertion and drops the obsolete immutable: true on event (per Remove the unused immutable flag from the record-type schema #748), so the marking writes validate.

Dependencies

Metadata

Metadata

Labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions