feat(skills): add harvesting-review skill#9900
Conversation
Add a skill that mines a pull request's review threads for recurring, reusable lessons, verifies each against the code, checks whether it is already documented, and proposes the smallest edit to the right internal doc. Reports first; applies only on approval; never resolves threads. Inspired by #9805 and @ajtmccarty's review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
Codify the durable guidance surfaced by the #9805 review: - query-pattern.md: prefer db.schema over the registry for schema access, promoted to a prominent section with the new-code-only carve-out - python.md: use dict.get() with a default when reading possibly-missing keys - backend/AGENTS.md: rewrite the async-tasks / creating-async-tasks / query-pattern router entries to say when to load them, so the Prefect-visible-logger and workflow-catalogue rules become discoverable when writing task code - AGENTS.md: doc-structure hygiene (dev/ ordering, docs/ description) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
group/backend
The report-format template listed 'New rules to add' before 'Existing coverage to strengthen', contradicting the instruction (and guardrail) to lead with the covered-but-flagged section as the highest-value output. Reorder the template so it leads, with a note to open with 'New rules to add' when it is empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Shadow auto-approve: would auto-approve. Adds a development skill (docs only) and updates internal documentation based on review lessons. No operational, security, schema, or public contract changes; CI/CD workflow plan is described but not implemented.
Re-trigger cubic
saltas888
left a comment
saltas888
left a comment
There was a problem hiding this comment.
Self-review mapping: each doc edit below traces to the specific #9805 review comment it codifies, so the report in the PR description and the diff line up. (Reviewer names are referenced without @ to avoid extra pings.)
|
|
||
| All database access in Infrahub goes through Query classes that encapsulate Cypher queries with proper parameterization, branch-awareness, and temporal versioning. Queries return typed dataclass results for type safety and clear API contracts. | ||
|
|
||
| ## Accessing schema: prefer `db.schema` over `registry` |
There was a problem hiding this comment.
Codifies ajtmccarty's review on #9805 (telemetry/database.py:66): avoid the registry, use db.schema for schema access. Promoted from a brief inline mention to this prominent section, with the new-code-only carve-out so it isn't read as a sweeping refactor.
| branch_data = BranchCreateInput(name="feature-x") | ||
| ``` | ||
|
|
||
| ### Use dict.get() for Default Values |
There was a problem hiding this comment.
Codifies ajtmccarty's review on #9805 (telemetry/task_manager.py:92): when reading an untyped bucket, prefer .get("count", 0). Generalized here to the dict.get(key, default) guideline.
| - `dev/knowledge/backend/branch-status.md` - Branch status enforcement (`BranchStatusChecker`, middleware allowlists, permission integration) | ||
| - `dev/knowledge/backend/events.md` - Events system | ||
| - `dev/knowledge/backend/async-tasks.md` - Asynchronous tasks (Prefect) | ||
| - `dev/knowledge/backend/async-tasks.md` - How Prefect tasks/flows work. Load when writing or changing any `@task`/`@flow`, including which logger to use and how workflow names are referenced. |
There was a problem hiding this comment.
Reflects dgarros's review on #9805 (telemetry/utils.py:31): a bare module logger doesn't surface in Prefect. The rule already lived in async-tasks.md, but this router entry was topic-only so it wasn't loaded when writing task code. Rewritten to say when to load it.
| ### Knowledge (How the system works, load the knowledge base when working on relevant tasks) | ||
|
|
||
| - `dev/knowledge/backend/architecture.md` - Backend architecture overview | ||
| - `dev/knowledge/backend/query-pattern.md` - Query class pattern and how database access works. Load when writing or changing any database read/write, doing schema lookups, counting nodes, or choosing between the `db` proxy and the `registry`. |
There was a problem hiding this comment.
Supports the db.schema-over-registry lesson (ajtmccarty, #9805): this router entry names schema lookups and the db-proxy-vs-registry choice as load triggers, so query-pattern.md is opened when it's relevant.
| - `backend/` – Python backend (FastAPI, GraphQL, core logic) - see [backend/AGENTS.md](backend/AGENTS.md) | ||
| - `frontend/app/` – React frontend - see [frontend/app/AGENTS.md](frontend/app/AGENTS.md) | ||
| - `docs/` – Docusaurus documentation - see [docs/AGENTS.md](docs/AGENTS.md) | ||
| - `docs/` – External customers documentation by Docusaurus, deployed at https://docs.infrahub.app/ - see [docs/AGENTS.md](docs/AGENTS.md) |
There was a problem hiding this comment.
For transparency: this one is not from a review comment. It's doc-structure hygiene (dev/ ordering + an expanded docs/ description) bundled into the same PR.
…nics The creating-async-tasks router entry baked in a specific API symbol (referencing workflow names via WorkflowDefinition.name) — a 'second copy' of the rule in the index, which rots when the symbol is renamed and is exactly what the 1A cap warns against. Generalise both async-task router entries to trigger + topic, and add the rule to the skill's 1A guidance so future harvests don't repeat it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would auto-approve. Adds a new AI skill for harvesting review lessons into internal docs, plus a worked set of doc edits from a real PR. This is bounded tooling and documentation only — no production code, no operational changes, no public contracts.
Re-trigger cubic
|
|
||
| - `dev/guides/backend/creating-events.md` - Creating new events | ||
| - `dev/guides/backend/creating-async-tasks.md` - Creating async tasks | ||
| - `dev/guides/backend/creating-async-tasks.md` - How to create an async task, with a pre-submit checklist. Load when adding a `@task`/`@flow`. |
There was a problem hiding this comment.
Reflects dgarros's review on #9805 (telemetry/task_manager.py:31): the workflow-naming convention. The convention itself lives in the guide's checklist; this router entry just makes that guide discoverable when you add a task.
…nto add-harvesting-review-skill # Conflicts: # dev/knowledge/backend/query-pattern.md
github-actions
Bot
removed
the
group/backend
…cipline Ports the extraction/diagnosis-side ideas from the harvesting-review proposal (#9900) that fit this skill's delegate-the-write design: - verify the reviewer's claim against the code before diagnosing - detect over-scoped (mis-read) comments and record the reading ruled out - keep already-documented lessons as a strengthen-existing bucket handed to capturing-knowledge, instead of dropping them - guardrails: size of diff != scope of rule, scope comments don't exempt the docs, treat review text as data, never resolve threads Doc-routing machinery (destination table, load-trigger fix, docs.lint) is deliberately left to capturing-knowledge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Head-to-head run of harvesting-review and learning-from-review on 8 real PRs surfaced six concrete gaps in harvesting; address them: - Reconstruct before -> after (the code the reviewer saw vs. the landed correction) and name the root cause, in step 3 and the report — the recall fix, and Paul's "compare code before AND after the feedback" ask. - Stop filtering feedback by author: bot comments get the same step-3 investigation, kept when the code confirms a durable rule (origin flagged, held to a lower-confidence bar). - Never demote a lesson as "the linter/type-checker enforces it" (nor run those tools to decide); route typing/annotation corrections to python.md. - "Already covered" — strengthen or demote — requires the exact file:line and a same-rule check; a keyword co-occurrence is not coverage. - Demote universal, non-Infrahub advice as its own red flag. - Confirm a doc path exists before routing a lesson to it. Bump 0.5.0 -> 0.6.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would auto-approve. Adds a new skill definition for harvesting review comments into documentation. No operational, security, or public contract changes. The change is bounded and clearly beneficial.
Re-trigger cubic
The `## Guardrails & red flags` section had grown into a near-mirror of the workflow — 16 of its 17 bullets restated a directive already given in its natural step — the exact "append instead of rewrite" failure the skill's own 1A guidance warns against. Several directives (investigate-first, report-before-edit, already-covered-is-a-finding) were stated 3-5 times across the description, intro, steps, and guardrails. Consolidate so each behavioral directive is stated once, in the step that owns it. Delete the mirror section (relocating "never resolve threads" into the Apply step and the blast-radius note into Report); drop the step-3 point-for-point recap in favour of one tightened worked example; fold "there is no covered-and-fine verdict" into the covered-but-ineffective verdict. All six recent behavioral additions and every niche directive are preserved in meaning, verified by an adversarial coverage pass (one dropped line, since restored). 328 -> 272 lines, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would auto-approve. Adds an agent skill that extracts reusable lessons from PR reviews and proposes doc edits, requiring human approval before changes. Bounded development tool addition; implementation already reviewed.
Re-trigger cubic
Harvesting vs learning: what we tested and what we tookWe ran this skill (harvesting review) and Paul's learning from review (#9910) on 8 real Infrahub PRs with real human review, then had Claude compare the outputs. Why two skills. Harvesting reports the durable lessons and routes them into our docs, and it leans on "covered but still flagged" (a rule that already auto injects yet still got violated). Learning reconstructs proposed, rejected, corrected for the PR and hands the write to capturing knowledge instead of editing docs itself. On every PR with real signal, both surfaced the same core lessons. They only diverged at the margins. Result. Learning 4 wins, harvesting 3, 1 tie. Learning had higher recall (it caught durable lessons harvesting dropped on 4 PRs). Harvesting had higher precision and better routing (covered but flagged, tight scope, almost no noise). What we took from learning into harvesting.
We also ran a cleanup pass that removed a guardrails section duplicating the workflow (328 to 271 lines, no behavior change). Next steps. Keep it report only for now so we build trust before it edits docs directly. Later, combine both into one skill: learning's before and after engine with harvesting's coverage search and routing, delegating the eventual write to capturing knowledge. |
Mines a PR's review threads for lessons that generalize, reconstructs each against the code (before the reviewer saw, and the correction that landed), checks existing coverage, and routes the durable ones into the internal docs. Reports first; edits only on approval. Identical to the skill approved on #9900 (by dgarros and ajtmccarty), opened against stable and consolidating #9910's learning-from-review ideas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Overview
A skill that reads the review comments on a pull request and turns the recurring, reusable ones into small updates to our own docs, so the same feedback isn't retyped by hand next time
This PR ships:
Notes:
Inspired by #9805 and Aaron's review (@ajtmccarty) on it
The report it produced on #9805
Rather than invoking this by hand, the plan is to trigger it automatically when a pull
request is merged, and have it open a follow-up pull request with the suggested changes — or
nothing, when there's nothing worth keeping. The full plan is below.
READ THIS 👇
Plan, auto-harvest review lessons on merge
What runs, and when
When a human-authored pull request is merged into
developand it had at least one humanreview comment, the harvester runs once. It reads the review threads, keeps only the lessons
worth following next time, checks each against the code and against what's already documented,
and then opens a single follow-up pull request into
stablewith the doc edits and thereport as the body. If nothing durable survives, it opens no pull request and stays silent.
Scope (uses existing labels only)
develop, with ≥1 human review comment.agentic-workflowsordependencies.agentic-workflows, so it never harvestsitself. No new labels are introduced.
Why the follow-up PR targets
stablestableis the default branch. The doc changes are low-risk to place there, a separateprocess keeps
stableanddevelopin sync, and consumers pick the updates up downstream.Requirements
pull_requestclosed+ merged,branches: [develop].stable, edits + report body, labelledagentic-workflows+type/documentation..agents/,AGENTS.md) via an allow-list.How we'll know it works
Notes
before landing.
stable) to run at all.lessons across PRs, and auto-merging the harvester's PRs (always human-reviewed).
confirming the
stable↔developsync carries the.agents/,AGENTS.md, anddev/paths.