# Harvesting Review > Harvesting Review is an Agent Skill for documents published in the opsmill/infrahub repository on GitHub: a SKILL.md instruction file an AI agent can load to do this task. Mines a pull request's review threads for lessons that generalize beyond that PR, reconstructs each against the actual code before deciding, checks whether it is already codified, and routes the genuinely-new, durable ones into Infrahub's internal documentation — `dev/knowledge/`, `dev/guides/`, `dev/guidelines/`, the `AGENTS.md` files, and `.agents/rules/` — proposing edits first and applying only with the user's approval. TRIGGER when: the user wants to turn PR review feedback into durable conventions, guidelines, or rules; capture recurring reviewer comments as internal documentation; or check whether review lessons are reflected in the knowledge/guides/guidelines/AGENTS.md/rules layer. DO NOT TRIGGER when: sweeping a feature's changes for documentation coverage across all layers → use `audit-docs`; extracting knowledge from completed spec directories → `speckit-opsmill-extract`; distilling the current chat session rather than a PR into docs → `feedback`; only replying to or resolving review threads → normal git/gh flow. - Skill name: `harvesting-review` - Category: [Documents](https://www.openparable.com/skill/documents/1) - Source: https://github.com/opsmill/infrahub/blob/HEAD/.agents/skills/harvesting-review/SKILL.md - Repository: opsmill/infrahub (494 GitHub stars when collected) ## SKILL.md # Harvest Review Lessons ## User Input ```text $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) | Destination | What lives there | Cost / bar | |-------------|------------------|------------| | `.agents/rules/*.md` | Terse, imperative `always/never` rules, **auto-injected into every agent turn** | Highest bar — every rule costs tokens on every turn. Reserve for high-frequency, high-value discipline. Edit an existing rule before adding a file. | | `dev/guidelines/**` | The fuller coding standard, consulted while writing code, with ✅/❌ examples | Home for idioms, style, testing practice. | | `dev/knowledge/**` | How the system actually works — technical reference | When the lesson is an *explanation* a future dev needs (a constraint, an invariant, a non-obvious mechanism), not a do/don't. | | `dev/guides/**` | How to do X — task-oriented guides and their checklists | When the lesson belongs in a step or a pre-submit checklist for a recurring task. | | root `AGENTS.md` | Repo-wide facts, style, boundaries (Always Do / Ask First / Never Do), navigation | For *what to do / ask-first / where things live* — not code idioms. | | `**/*/AGENTS.md` | Area-specific agent instructions and component maps | For area-scoped behavioural facts. | 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 `). - **Empty** — the current branch's PR (`gh pr view`). If none exists, ask. Pull the raw material: ```bash gh pr view --json title,body,headRefName,commits gh api repos/opsmill/infrahub/pulls//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: ```bash grep -rin "" .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: ```bash uv run invoke docs.lint ``` ## Report format ```markdown ## Review-Lessons Report — PR # ### Scope ### 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) ``` --- Source: https://www.openparable.com/skill/documents/harvesting-review (OpenParable). Cite the canonical URL above.