harvesting-review

Harvest Review Lessons

User Input

$ARGUMENTS

What this does

Reviewers repeat themselves — the same idiom, naming, and layering nits recur PR after PR because each lesson lived in a thread and died there. This skill reads a PR's review comments, keeps the ones that generalize into a rule a future author should follow, investigates each against the actual code before deciding, checks whether it is already documented, and proposes the smallest edit to the right internal-doc file.

Counterpart to audit-docs: that sweeps a feature's changes for coverage; this starts from the review threads.

Internal documentation (where lessons go)

Source of truth is .agents/ — .claude/rules, .claude/skills, .claude/commands are symlinks to it, so you edit .agents/** once and both harnesses see it.

Workflow

1. Gather scope

Resolve $ARGUMENTS to a PR:

• A PR number (#1234 or 1234) — audit that PR.
• A branch name — resolve to its open PR (gh pr view <branch>).
• Empty — the current branch's PR (gh pr view). If none exists, ask.

Pull the raw material:

gh pr view <n> --json title,body,headRefName,commits
gh api repos/opsmill/infrahub/pulls/<n>/comments --paginate \
  -q '.[] | "--- \(.user.login) on \(.path):\(.line // .original_line)\n\(.body)\n"'

Read resolved and unresolved threads — a resolved thread whose lesson never made it into an internal doc is exactly the gap this skill exists to catch. Prioritise human reviewers, but do not filter by author: a bot comment (cubic, coderabbit) runs through the same step-3 investigation as a human one and is kept when the code confirms its rule is durable — several of the most reusable lessons arrive this way. Never sweep "all bot comments" into a PR-local bucket; when a bot-sourced lesson survives, flag its origin and hold its promotion to a lower-confidence bar. Also skim the "addressed-by" commit messages — they often state the lesson more crisply than the thread.

2. Extract & abstract candidate lessons

For each comment, abstract it to the underlying rule — promote the rule, not the reviewer's wording. The line the reviewer touched is the symptom; the rule is the disease — a comment fixing one call site usually points at a convention that spans many. Keep a candidate only if it passes all three:

1. Generalizes — applies beyond these lines (a convention, idiom, or architectural principle).
2. Actionable as an imperative — you can phrase it as "do X / don't do Y" an author follows next time.
3. Would prevent a repeat comment — a future reviewer would flag the same thing again if it isn't written down.

A fourth check bounds the other end: the rule must be Infrahub-specific or a non-obvious gotcha. Universal programming hygiene every competent author already applies — "keep comments near the code", "name things clearly", "write tests" — is not worth the per-turn token cost of a rule or guideline; set it aside like any other non-lesson.

Everything obviously PR-local — a real bug, a typo — is set aside now. Borderline cases are not judged yet; they go through the investigation, which is what tells you whether they generalize.

3. Investigate each candidate — the trail (do this BEFORE any decision)

For every surviving candidate, work the trail and write it down before reaching any verdict — no verdict, no home, no edit until it clears this step; it is the one most easily skipped. A comment is a symptom, not an instruction: read literally, a terse "avoid X, use Y" can look like a sweeping refactor when the durable rule is small and local. You tell them apart by reading the code, and by reading the correction that actually landed.

a. Reconstruct before → after, then verify the claim. Read the code the reviewer saw (the before) and the correction that actually landed — the commit(s) pushed after the comment (the after). The correction is the ground truth of the lesson; the comment only prompted it. A thread with no landed change is unresolved or a no-op — flag it, do not invent a fix. Then verify: is the reviewer technically right, and is the suggested alternative a real drop-in? Grep for real usages of the alternative elsewhere in the repo (the method/pattern) and confirm it behaves the same. A lesson built on an unverified claim — or on a correction that never landed — is worse than no lesson.

b. Interpret the intent — what is the reviewer actually asking? Separate:

• a local code-style preference for new code ("reach for the injected accessor here") from a codebase-wide lifecycle change (deprecate / migrate everything);
• directional guidance ("we'll delete this someday") from an actionable request for this PR;
• the precise boundary — which exact calls the rule covers and which it does not.

c. Size the scope of the naive reading. Ask what acting on the literal comment would cost. If it implies a sweeping refactor (hundreds of call sites), a deprecation, or a migration, that is a strong signal you have mis-read it — the durable rule is almost always the smaller "in new code, prefer X" form. Name the over-scoped reading you are ruling out; that record is what stops the next person from over-scoping too.

d. Derive the precise rule — and its root cause. Only now write the one-sentence imperative, scoped exactly as the investigation showed, including any explicit carve-outs. Then name the root cause: why would an agent have proposed the rejected shape in the first place? — a missing or violated convention, a reflexive idiom (or "", default=str), a copied legacy template, a misread requirement. The root cause is what tells you whether writing the rule down would actually prevent the repeat.

The investigation can also demote a candidate: if verifying shows the suggestion doesn't generalize, is already the universal pattern, or was reviewer error, it becomes a "not a lesson" with the reason recorded. Judge "generalizes" by whether the underlying idiom or preference recurs across the codebase, not by the size of the local fix — a one-line defensive or idiomatic change (.get(key, default) when reading untyped/external data, a naming or import convention, a preferred accessor) is still a durable styleguide rule.

Never demote a lesson on the assumption a linter or type-checker already enforces it — and do not run those tools to decide. A human reviewer having to raise it is itself evidence the tool did not catch it. Typing and annotation corrections in particular (a missing | None, an over-narrow or over-wide hint) are durable style rules: route them to dev/guidelines/backend/python.md (the Type Hints section), never drop them as "standard Python".

Worked shape — the trail on the commonest case, a terse "avoid X, use Y":

• Before → after: read the code at the comment and the commit that landed the fix; if nothing landed, flag it and stop.
• Verify: open Y, confirm it exposes the same method/return as X, and grep for existing Y usages to prove it is an established drop-in.
• Intent: a style preference for new code, or migrate everything? Directional ("we'll drop X someday") or actionable now?
• Scope: count X's call sites — a literal reading that means touching hundreds is a mis-read; name it, and note any spot where X legitimately stays.
• Rule + root cause: the smallest scoped imperative with its carve-out, and why an agent would have reached for X in the first place.

4. Check existing coverage, then route (dedup)

For each investigated lesson, grep the internal-doc layer for the rule:

grep -rin "<keyword>" .agents/rules/ dev/guidelines/ dev/knowledge/ dev/guides/ \
  AGENTS.md backend/AGENTS.md frontend/app/AGENTS.md

A grep hit is not coverage until you read it. Before you call a rule "already covered" — whether to report it as covered-but-flagged or to demote it as "already codified, not a lesson" — open the matched file at that line, confirm it states this rule and not an adjacent one, and cite the exact file:line. A keyword that merely co-occurs (an enum-default rule is not covered by a dependency-injection rule that happens to mention enums) is a coincidental match, not coverage.

A reviewer flagged this, so the verdicts are not a pass/fail of the docs — they are:

• Missing — the rule is written nowhere → propose the smallest addition in the most-specific home.

• Covered but ineffective — the rule is written, yet a reviewer still had to flag it. This is a finding, not a relief — there is deliberately no "covered and fine" verdict, because a documented rule a reviewer still had to raise is evidence the coverage is too weak, not proof it works. Report it prominently, never as "the layer works". Diagnose why it didn't land and propose how to make the existing doc land — do not add a duplicate rule. Pick the diagnosis:

  • too abstract / no worked example — states the principle but not the concrete case the author needed. The clearest signal: the reviewer had to describe the shape themselves (the class to build, the collaborators to inject, the method to call). If a reviewer has to draw the construction, the section is too abstract → add the ✅/❌ worked example that turns the flagged anti-pattern into the pattern.

  • not discoverable — the rule is in the right topical doc, but nothing pulled that doc into context when the author needed it. The fix is almost always two coordinated edits, not a move (the 1A+1B fix):

    • 1A — fix the load-trigger. Add or upgrade the doc's entry in the router/index (the relevant AGENTS.md "Knowledge/Guides" list) so it says when to load it — the triggering task or symptom — not just what it covers. A dev/knowledge/dev/guidelines doc absent from that list, or listed with a topic-only description ("Query patterns"), never gets loaded; most "covered but ignored" rules fail here. Keep the entry to one or two sentences, and rewrite rather than append — the router is a scannable index, not a second copy of the rule; once an entry swells into a paragraph it stops being read, reopening the gap 1A exists to close. If the trigger can't be stated briefly the doc's scope is too broad — that is not a licence for a longer entry. Name the trigger and topic, not the rule's mechanics: the entry says when to open the doc and roughly what it covers, never the specific method/attribute name or the value the rule turns on — those live in the doc, and copying them into the index rots the moment the symbol is renamed (write "workflow-name conventions", not "reference names via SomeClass.name").
    • 1B — strengthen the rule in place. Promote it out of any niche section into a prominent home and add the carve-out — but leave it in its topically-correct doc.

    Do not relocate a domain rule (schema, DB, events, async-tasks…) into a general style/guide doc because that doc is read more often: a rule in the topically-wrong home is less trustworthy, not more discoverable. .agents/rules/* auto-injects every turn; guidelines/knowledge load only when the router points an agent at them, so the router entry is the discoverability mechanism. If your proposed home is a doc whose subject doesn't match the rule (a schema/DB rule in a Python style guide), you have mis-diagnosed "not discoverable" as "mis-homed" — fix the load-trigger, not the location.

  • mis-homed — the rule genuinely sits in the wrong topical doc, or belongs in a task's pre-submit checklist → move it to the most-specific correct home (or add the checklist line), then apply 1A so that home is actually loadable.

  • stale / contradicted — the doc no longer matches the code, so authors discount it → correct it.

  Scope is not an alibi for the docs. "Too much for this PR", "out of scope", or an existing-code carve-out are reasons not to change code now — they never exempt the documentation from being made more concrete. Before you lean on such a carve-out, check it applies: an existing-code exemption protects pre-existing code, not new code written in the old style (verify in the diff). Concluding "covered, correctly scoped, no edit" is the rationalization this step exists to prevent — reach for it only when the existing section already contains the concrete example the reviewer was forced to supply.

• Not applicable — the grep match was coincidental, or the comment was not actually a violation of the rule (a question, a one-off) → demote to Not a lesson, with the reason.

Routing rule of thumb: most-specific existing home wins; edit before create; strengthen before duplicate; fix the load-trigger before relocating; .agents/rules only for a true always/never that must fire while coding. Confirm the target file exists (ls/grep it, match sibling naming) before you route a lesson there — never invent a plausible-looking path (e.g. dev/guidelines/backend/changelog.md when the real home is dev/guidelines/changelog.md).

5. Report

Present the findings (format below) and stop. Do not edit yet — internal-doc files shape every teammate's agent, so the blast radius is the whole team.

6. Apply (opt-in)

Ask which to apply: all / cherry-pick / none. Only then edit, following the §4 routing (edit an existing section before adding one; keep .agents/rules lean). Match any example code to .agents/rules/code-doc-style.md (no ticket/issue IDs, no naming specific callers). Never resolve review threads — reply if useful, but resolution is the human reviewer's call. After applying, run:

uv run invoke docs.lint

Report format

## Review-Lessons Report — PR #<n>

### Scope
<!-- PR, branch, how many threads read (resolved + unresolved) -->

### Existing coverage to strengthen (Covered but still flagged)

The rule already exists, yet a reviewer had to raise it — so the coverage is not landing. **This is the
highest-value output of the harvest, so it leads the report; when it is empty, open with "New rules to
add" instead.** For each:
- **Lesson** + **Source** (reviewer + quoted comment)
- **Already at**: the exact existing `file:line`, confirmed to be the same rule, not a keyword co-occurrence
- **Why it didn't land**: too abstract / not discoverable / mis-homed / missing from checklist / stale
- **Proposed edit**: how to make the *existing* coverage land — the concrete example to add, the
  load-trigger/relocation edit, or the checklist line. Not a duplicate rule.

### New rules to add (Missing)

For each:
- **Lesson**: the precise, scoped imperative (one sentence)
- **Source**: reviewer + quoted comment (and the addressing commit, if any)
- **Investigation**: the trail — (a) before → after (the code the reviewer saw vs. the correction that
  landed, citing the commit) and the claim verified in code (cite the files checked), (b) reviewer
  intent, (c) scope + the over-scoped reading ruled out
- **Root cause**: why an agent would have proposed the rejected shape — what writing the rule prevents
- **Home**: exact file (+ section) to create
- **Proposed edit**: the concrete text to add

### Not Lessons (PR-local or demoted after investigation)
<!-- One-off fixes, bugs, and design calls that do NOT generalize — and candidates the investigation
     demoted (unverified claim, coincidental grep match, reviewer error, a question, or universal advice
     with no Infrahub-specific edge). Say why, briefly. Every lesson is grounded in a real comment; none
     is invented, and promoting every comment to a rule is as useless as missing the real ones. -->