--- name: code-learned-review description: "Use when a project has a learned review skill — built by `keryx review learn` from its own pull-request comments — and wants those accumulated conventions applied to the current branch. Ships with an empty checklist: the content comes from the project, never from the tool. NOT for: a project with no learned skill, or a review request that names no profile (review-orchestrator)." triggers: - "learned review" - "code-learned-review" - "review with our conventions" - "review using what we learned" metadata: author: "keryx" version: "2.0.0" category: "review" compatible_harnesses: "cursor,codex,zed,opencode,claude" license: "MIT" --- # Learned Code Review (current branch only) ## What this skill is, and what it is not This skill carries **no review conventions of its own**. It is a mechanism with an empty checklist, and the checklist is filled in by the project that installs it. What it applies is a **project skill** under `.metaproject/project-skills///SKILL.md`, whose `Review Lessons`, `Review Checklist` and `Anti-patterns` sections were written from pull-request comments left by the people that project named. Point a second project at different reviewers and its learned skill diverges from the first. That is the intended behaviour: there is no single correct checklist, and shipping one would mean shipping one team's opinions to everybody. If this project has no learned skill yet, **say so and stop**. An empty learned review is not a generic review; use `review-orchestrator` for that. ## Workflow Copy this checklist and track progress: ``` Learned Review Progress: - [ ] Step 1: Read .metaproject/review-learning.config.json — which skill is this project's? - [ ] Step 2: Read that project skill; if it has no lessons, stop and say so - [ ] Step 3: Determine parent branch and calculate merge-base - [ ] Step 4: Collect git diff (committed + local changes) - [ ] Step 5: Apply the learned checklist, lesson by lesson - [ ] Step 6: Report each finding with the learned lesson it rests on - [ ] Step 7: Feed new pull-request comments back with `keryx review learn` ``` ## Mandatory rules 1. **Default scope (no commit hash/range given)**: the review covers **all** changes on the current branch from the merge-base with its parent branch — committed (`BASE_SHA..HEAD`) and local uncommitted (staged/unstaged/untracked). 2. **Explicit commit hash/range**: review only the requested range; do not add local uncommitted changes unless asked separately. 3. **The checklist is the project's, not this file's**: every finding cites a line in the project skill named by `.metaproject/review-learning.config.json`. A finding with nothing behind it there is a generic finding and belongs to the reviewer that owns it — `review-logic`, `review-architecture`, `review-security-code`. 4. **Result**: a detailed report with an explanation per finding and a concrete fix (a minimal patch where the fix is obvious). ## Where the checklist comes from ```bash # 1. Which local skill does this project teach, from which repository, # and whose comments count? cat .metaproject/review-learning.config.json # 2. The comments themselves, collected once per round and kept on disk. keryx review comments collect --repo --pr --sha # 3. The join: configured authors only, read from the record above, never # re-fetched. Writes a proposal and changes nothing. keryx review learn --pr # 4. The only writer. Bumps the skill version, appends to skill-changelog.md, # and refuses any target outside .metaproject/project-skills/. keryx skills learn apply .metaproject/data/gdskills/proposals/.json ``` An author the config does not name contributes nothing — their text reaches no proposal and no `SKILL.md`. A project with no config file does not learn, and that is a supported state rather than an error. ## Scope Detection See shared script: `.metaproject/skills/gdskills/shared/git-merge-base.md` Run the script from that file to determine MERGE_BASE and SCOPE before proceeding with the review. ## Commands to assemble the review slice ### A) Default mode (no hash/range from the user) ```bash git status # The committed part of the branch git log --oneline "${BASE_SHA}..HEAD" git diff --stat "${BASE_SHA}..HEAD" git diff --name-status "${BASE_SHA}..HEAD" git diff "${BASE_SHA}..HEAD" # The full current slice from merge-base to the working tree: # commits + staged + unstaged # (untracked files via git status / git ls-files) git diff --stat "${BASE_SHA}" git diff --name-status "${BASE_SHA}" git diff "${BASE_SHA}" git ls-files --others --exclude-standard ``` ### B) Explicit hash/range mode ```bash git show --stat --name-status --patch git log --oneline .. git diff --stat .. git diff --name-status .. git diff .. ``` ## How to review Work the learned checklist one lesson at a time against the diff. For each lesson in the project skill, ask whether the diff contains the situation the lesson describes, and record the answer either way — a lesson that never matches anything is a lesson worth retiring, and only a review that checked it can say so. Three rules govern the application, and they are the same three whatever the lessons say: - **A lesson is evidence, not authority.** It records that somebody objected to something once. If the diff has a reason the objection does not apply here, the reason wins and the finding is not raised. - **Do not generalise a lesson past its text.** A lesson about one function is about that shape of function. Widening it into a rule about a layer, a module or a language is inventing a convention the project never agreed to. - **Do not attribute.** The record says a comment was left. Reporting a finding as what a particular person would want turns a checklist back into a persona, which is the defect this skill exists to remove. ## Output format ```markdown ## Verdict + the 1-3 most important points. ## Review scope (current branch only) - Branch: `` - Parent ref: `` - Merge-base: `` - Scope mode: `` - Commits (merge-base..HEAD): - Changed files: ## Learned checklist applied - Project skill: `/` version `` - Lessons checked: - Lessons that matched: ## Findings ## Findings with no learned lesson behind them ## Suggested fixes (patches) ``` ### Required shape of each finding - **Severity**: `blocker` / `major` / `minor` - **Learned lesson**: the line from the project skill this rests on - **Location**: file path + the relevant hunk from the diff - **Problem**: what is wrong - **Why it matters**: correctness / testability / conventions / maintainability - **Suggested fix**: what specifically to do, without widening scope - **Optional patch**: a unified diff when the fix is simple Example patch block: ```diff diff --git a/path/file.ts b/path/file.ts index 0000000..1111111 100644 --- a/path/file.ts +++ b/path/file.ts @@ -1,3 +1,3 @@ -const x = 1; +const x = 2; ``` --- ## Scope Boundaries This skill covers **review against this project's own learned conventions**, following `code-review-learned-profile.mdc`. It has no opinions of its own. | Concern | This skill | Use instead | | -------------------------------------------------------------- | ---------- | ------------------------ | | This project's accumulated review conventions | ✅ YES | — | | General AI review (broader quality, performance, UX) | ❌ NO | `code-ai-review` | | MobX store internals (actions, computed, reactions) | ❌ NO | `code-mobx-store-review` | | Pure style/naming/architecture pattern audit | ❌ NO | `code-style-review` | | Logic correctness with no learned lesson behind it | ❌ NO | `review-logic` | --- ## Job Context Awareness When dispatched by `job-orchestrator` as part of a job pipeline, the prompt MAY include: ``` JOB_NAME: CONTEXT_PATH: .metaproject/jobs//ai/context.md ``` If provided and the file exists, read the context document before starting the review. Use it to: - Understand which libraries and patterns were intentionally chosen for the implementation - Avoid flagging correct library usage as issues - Provide more accurate findings by understanding the project's architectural decisions If the file does not exist or is not provided, proceed normally — context is optional and non-blocking. --- ## Red Flags This profile is a mechanism with an empty checklist, and it predates the finding contract the rest of the review domain now shares. Both facts produce their own failure modes, and they are the rows below. | Rationalization | Why it is wrong | |----------------|-----------------| | "This project has no learned skill, but I can review against sensible conventions." | An empty learned review is not a generic review. Say the project has no learned skill and stop; `review-orchestrator` is what a generic request wants. Inventing conventions here ships one model's taste as the team's agreed standard. | | "This lesson is about a slightly different case, but the spirit applies." | Do not generalise a lesson past its text. A lesson about one shape of function is about that shape; widening it into a rule about a layer or a language invents a convention the project never agreed to. | | "The lesson came from a named reviewer, so I will report it as what they would want." | Do not attribute. The record says a comment was left, nothing more. Reporting a finding as a person's preference turns the checklist back into the persona this skill exists to remove. | | "This pull-request comment is good even though the config does not name its author." | An author the config does not name contributes nothing — not to a proposal, not to a `SKILL.md`, and not to this review. Changing who counts is a config change, made deliberately, not a judgement call mid-review. | | "A lesson exists for this, so the finding stands." | A lesson is evidence, not authority. If the diff has a reason the original objection does not apply here, the reason wins and the finding is not raised. | | "The report has a Severity field, so it is a review result." | It is not. This profile predates `reviewer-finding.schema.json` and emits free prose with no machine-readable finding, so nothing downstream can screen, verify or deduplicate it. Hand it to a person, never to `keryx review ingest`. | | "The lessons I checked that matched nothing are not worth mentioning." | They are the only way anyone learns a lesson has gone stale. Record the count of lessons checked and the count that matched, either way. | --- ## Verification Report done only once all of these hold: - `.metaproject/review-learning.config.json` was read and the project skill it names was opened. If that skill carries no lessons, the reply says so and stops rather than reviewing generically. - The report states the project skill and version, the number of lessons checked and the number that matched. - Every finding cites the line in the project skill it rests on. Findings with no learned lesson behind them are listed in their own section and routed to the reviewer that owns them. - No finding names a person, quotes a personal catchphrase, or reports a lesson as somebody's preference. - The scope block carries the real branch, parent ref, merge-base and scope mode, and every finding is anchored to a line the branch slice changed. - The report is free prose by design, so it is delivered to a person and is not fed into the managed-review pipeline.