feat: add an operational-history prior-art lane to the code-reviewer agent
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -12,6 +12,31 @@ agents while handling coordination and final reporting.
|
|||||||
- 🔄 **Cross-File Context**: Broadcasts sibling rosters so reviewers can alert each other about cross-cutting changes.
|
- 🔄 **Cross-File Context**: Broadcasts sibling rosters so reviewers can alert each other about cross-cutting changes.
|
||||||
- 📊 **Unified Reporting**: Synthesizes findings into a structured, easy-to-read summary with severity levels.
|
- 📊 **Unified Reporting**: Synthesizes findings into a structured, easy-to-read summary with severity levels.
|
||||||
- ⚡ **Parallel Execution**: Runs reviews concurrently for maximum speed.
|
- ⚡ **Parallel Execution**: Runs reviews concurrently for maximum speed.
|
||||||
|
- 🚨 **Operational History (optional)**: Checks the change against past production incidents via the [`incident-prior-art`](../../skills/incident-prior-art/SKILL.md) skill.
|
||||||
|
|
||||||
|
## Operational History Lane
|
||||||
|
|
||||||
|
Code review answers "is this code good?" — this lane answers "did we already get burned by this?"
|
||||||
|
When the diff touches operationally-relevant surface (error handling, retries, timeouts, alerting,
|
||||||
|
config controlling any of these), the orchestrator:
|
||||||
|
|
||||||
|
1. **Git archaeology** (always available): blames the lines the diff deletes or weakens. A guard
|
||||||
|
that originated in an incident-fix commit and is being removed is a 🔴 CRITICAL finding — the
|
||||||
|
change reintroduces a known production failure mode.
|
||||||
|
2. **Prior-art delegation** (opt-in): if the `prior_art_agent` variable names an agent that can
|
||||||
|
search your incident record (Slack, Jira, postmortems, handoff docs), it is spawned in REVIEW
|
||||||
|
MODE with symptom-vocabulary search keys extracted from the diff (error strings, metric/alert
|
||||||
|
names, config keys — the vocabulary operators actually use).
|
||||||
|
|
||||||
|
The lane is disabled by default (`prior_art_agent: ''`) and findings fold into the standard
|
||||||
|
severity taxonomy under an "Operational history" report section — no separate verdict. Wire it up
|
||||||
|
in a bundle or your local config:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
variables:
|
||||||
|
- name: prior_art_agent
|
||||||
|
default: 'oncall-historian' # any spawnable agent that can search your incident record
|
||||||
|
```
|
||||||
|
|
||||||
## Pro-Tip: Use an IDE MCP Server for Improved Performance
|
## Pro-Tip: Use an IDE MCP Server for Improved Performance
|
||||||
Many modern IDEs now include MCP servers that let LLMs perform operations within the IDE itself and use IDE tools. Using
|
Many modern IDEs now include MCP servers that let LLMs perform operations within the IDE itself and use IDE tools. Using
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
name: code-reviewer
|
name: code-reviewer
|
||||||
description: CodeRabbit-style code reviewer - spawns per-file reviewers, synthesizes findings
|
description: CodeRabbit-style code reviewer - spawns per-file reviewers, synthesizes findings
|
||||||
version: 2.0.0
|
version: 2.1.0
|
||||||
|
|
||||||
auto_continue: true
|
auto_continue: true
|
||||||
max_auto_continues: 20
|
max_auto_continues: 20
|
||||||
@@ -14,11 +14,15 @@ skills_enabled: true
|
|||||||
enabled_skills:
|
enabled_skills:
|
||||||
- delegation-protocol
|
- delegation-protocol
|
||||||
- parallel-research
|
- parallel-research
|
||||||
|
- incident-prior-art
|
||||||
|
|
||||||
variables:
|
variables:
|
||||||
- name: project_dir
|
- name: project_dir
|
||||||
description: Project directory to review
|
description: Project directory to review
|
||||||
default: '.'
|
default: '.'
|
||||||
|
- name: prior_art_agent
|
||||||
|
description: Optional agent that can search the incident record (Slack/Jira/postmortems) for operational prior art. Empty disables the delegation lane; git archaeology still runs.
|
||||||
|
default: ''
|
||||||
- name: auto_confirm
|
- name: auto_confirm
|
||||||
description: Auto-confirm command execution
|
description: Auto-confirm command execution
|
||||||
default: '1'
|
default: '1'
|
||||||
@@ -46,11 +50,12 @@ instructions: |
|
|||||||
|
|
||||||
1. **Get the diff:** Run `get_diff` to get the git diff (defaults to staged changes, falls back to unstaged)
|
1. **Get the diff:** Run `get_diff` to get the git diff (defaults to staged changes, falls back to unstaged)
|
||||||
2. **Parse changed files:** Extract the list of files from the diff
|
2. **Parse changed files:** Extract the list of files from the diff
|
||||||
3. **Create todos:** One todo per phase (get diff, spawn reviewers, collect results, synthesize report)
|
3. **Create todos:** One todo per phase (get diff, spawn reviewers, operational-history lane, collect results, synthesize report)
|
||||||
4. **Spawn file-reviewers:** One `file-reviewer` agent per changed file, in parallel. Apply the `delegation-protocol` structured prompt format.
|
4. **Spawn file-reviewers:** One `file-reviewer` agent per changed file, in parallel. Apply the `delegation-protocol` structured prompt format.
|
||||||
5. **Broadcast sibling roster:** Send each file-reviewer a message with all sibling IDs and their file assignments
|
5. **Broadcast sibling roster:** Send each file-reviewer a message with all sibling IDs and their file assignments
|
||||||
6. **Collect all results:** Per `parallel-research`, do not poll. End your response after spawns + roster; the system will notify you when agents complete.
|
6. **Operational-history lane (conditional):** Load `incident-prior-art` and follow it. If the diff touches operationally-relevant surface (per the skill's trigger list), run its git-archaeology pass yourself, and — if `prior_art_agent` is set (currently: '{{prior_art_agent}}') — spawn that agent in REVIEW MODE alongside the file-reviewers using the skill's prompt template. If the surface is not operationally relevant, skip with a one-line note.
|
||||||
7. **Synthesize:** Combine all findings into a CodeRabbit-style report
|
7. **Collect all results:** Per `parallel-research`, do not poll. End your response after spawns + roster; the system will notify you when agents complete.
|
||||||
|
8. **Synthesize:** Combine all findings into a CodeRabbit-style report. Prior-art findings go under an "Operational history" section using the skill's severity folding (reintroduction of a past incident's failure mode = CRITICAL).
|
||||||
|
|
||||||
## Spawning File Reviewers
|
## Spawning File Reviewers
|
||||||
|
|
||||||
@@ -138,6 +143,9 @@ instructions: |
|
|||||||
## Cross-File Concerns
|
## Cross-File Concerns
|
||||||
<any cross-cutting issues identified by the teammate pattern>
|
<any cross-cutting issues identified by the teammate pattern>
|
||||||
|
|
||||||
|
## Operational history
|
||||||
|
<only when the lane ran: archaeology + prior-art findings with incident/commit references, or "no relevant incident history found">
|
||||||
|
|
||||||
---
|
---
|
||||||
*Reviewed N files, found X critical, Y warnings, Z suggestions, W nitpicks*
|
*Reviewed N files, found X critical, Y warnings, Z suggestions, W nitpicks*
|
||||||
```
|
```
|
||||||
|
|||||||
@@ -0,0 +1,87 @@
|
|||||||
|
---
|
||||||
|
description: Check a code change against operational history - past incidents, outages, and on-call fixes - so a review catches regressions of hard-won production lessons. Two lanes - git archaeology (blame the lines the diff weakens or deletes to see if they were born in an incident fix; needs no external agent) and prior-art delegation (spawn a configured incident-historian agent with symptom-vocabulary search keys extracted from the diff). Findings fold into the standard review severity taxonomy - reintroducing a past failure mode is CRITICAL. Grants shell access for git history commands.
|
||||||
|
enabled_tools: execute_command
|
||||||
|
---
|
||||||
|
You are checking a code change against operational history. Code review answers "is this code good?"; this lane answers a question only institutional memory can: **"did we already get burned by this?"** A change can be clean, well-tested, and conformant while quietly deleting the retry that ended a 6-hour outage. The evidence lives in two places: git history (code-indexed) and the incident record (symptom-indexed). Work both.
|
||||||
|
|
||||||
|
## When this lane runs
|
||||||
|
|
||||||
|
This lane is OPTIONAL and runs only when both hold:
|
||||||
|
|
||||||
|
1. **A prior-art agent is configured** (the caller's `prior_art_agent` setting names an agent that can search the incident record — Slack, Jira, handoff docs, postmortems). If it is empty, run ONLY the git-archaeology lane (Part A), which needs no external agent.
|
||||||
|
2. **The diff touches operationally-relevant surface**: services with on-call history, code that emits alerts/metrics/log lines operators watch, error handling, retries, timeouts, rate limits, queue/batch processing, or config controlling any of these. A docs change or a pure-UI tweak does not need an incident sweep — skip and say so in one line.
|
||||||
|
|
||||||
|
## Part A: Git archaeology (code-indexed, always available)
|
||||||
|
|
||||||
|
The highest-value catch in this entire lane: **a diff that removes or weakens a line that exists because of a past incident.** Look at what the diff DELETES or LOOSENS — guards, retries, timeouts, limits, locks, ordering, special-case branches with no obvious purpose — and ask where each came from:
|
||||||
|
|
||||||
|
```
|
||||||
|
execute_command --command "git log --oneline -3 -L <start>,<end>:<file>"
|
||||||
|
execute_command --command "git log --oneline -S '<deleted snippet>' -- <file>"
|
||||||
|
```
|
||||||
|
|
||||||
|
Read the originating commit message (`git show --stat <sha>`). Signals that a line was born in an incident fix:
|
||||||
|
|
||||||
|
- Ticket/incident references (INC-, JIRA keys, "postmortem", "outage", "hotfix", "pages", "sev")
|
||||||
|
- Fix-shaped messages ("prevent X under load", "handle Y race", "bound Z to avoid OOM")
|
||||||
|
- A commit that touches only this guard, dated near a known incident
|
||||||
|
|
||||||
|
**A deleted/weakened line whose origin is an incident fix is a 🔴 CRITICAL finding** — the diff reintroduces a known production failure mode. Cite the line, the originating commit, and its message. If the origin is ordinary feature work, no finding — do not manufacture history.
|
||||||
|
|
||||||
|
## Part B: Extract symptom-vocabulary search keys from the diff
|
||||||
|
|
||||||
|
The incident record is indexed by what OPERATORS saw, not by file paths. Before delegating, translate the diff into that vocabulary:
|
||||||
|
|
||||||
|
1. **Error/log strings** added, changed, or deleted — incidents are found by error strings more than by anything else. A DELETED log line is itself a lead: someone may rely on it for triage.
|
||||||
|
2. **Metric, alert, and dashboard names** the code emits or the change affects.
|
||||||
|
3. **Config keys** and their old/new values (timeouts, limits, feature flags).
|
||||||
|
4. **Service/feature/domain terms** an operator would use ("invoice proration", "webhook retries", "usage export") — not function names.
|
||||||
|
5. **External dependencies touched** (queues, third-party APIs, databases) — their names appear in incident titles.
|
||||||
|
|
||||||
|
Collect 3-8 strong keys. Weak generic keys ("error", "billing") flood the search; skip them.
|
||||||
|
|
||||||
|
## Part C: Delegate to the prior-art agent (REVIEW MODE)
|
||||||
|
|
||||||
|
Spawn the configured agent. Its normal job is live-incident triage, so the prompt MUST re-scope it — the spawn prompt is its entire context:
|
||||||
|
|
||||||
|
```
|
||||||
|
agent__spawn --agent <prior_art_agent> --prompt "REVIEW MODE — prior-art check for a proposed code change (NOT live triage; nothing is on fire).
|
||||||
|
|
||||||
|
## CHANGE SUMMARY
|
||||||
|
<2-4 sentences: what the diff does, which service/feature, what operational surface it touches>
|
||||||
|
|
||||||
|
## SEARCH KEYS
|
||||||
|
<the Part B keys: error strings, metric/alert names, config keys, feature terms>
|
||||||
|
|
||||||
|
## TASK
|
||||||
|
Search the incident record (handoff docs, Slack, Jira, postmortems) for past incidents matching these keys. For each relevant hit report:
|
||||||
|
- Reference (ticket/thread/doc section) and date
|
||||||
|
- What happened and what the resolution was
|
||||||
|
- Relevance: does this change RISK REINTRODUCING that failure mode, or should it ADOPT a safeguard from that resolution?
|
||||||
|
|
||||||
|
Only report incidents with a concrete connection to these search keys. 'The billing system has had incidents' is noise. If nothing relevant exists, say so plainly — a clean result is a valid result.
|
||||||
|
|
||||||
|
You are read-only. Do not post, comment, or edit anything."
|
||||||
|
```
|
||||||
|
|
||||||
|
## Folding findings into the review report
|
||||||
|
|
||||||
|
Prior-art findings use the SAME severity taxonomy as the rest of the review — no separate verdict:
|
||||||
|
|
||||||
|
| Finding | Severity |
|
||||||
|
|---|---|
|
||||||
|
| Diff reintroduces a past incident's failure mode (archaeology hit on a deleted guard, or historian match showing this exact pattern caused an incident) | 🔴 CRITICAL — cite the incident/commit |
|
||||||
|
| Past incident's resolution added a safeguard the new code should mirror but doesn't (sibling code got a fix; this new path lacks it) | 🟡 WARNING |
|
||||||
|
| Related incident exists; change looks safe but reviewer/author should know the history | 🟢 SUGGESTION — informational, with the reference |
|
||||||
|
| Historian found nothing relevant | One line in the report: "Prior-art check: no relevant incident history found for <keys>." |
|
||||||
|
|
||||||
|
Present these under a dedicated **"Operational history"** section in the final report, each finding citing its incident reference or originating commit.
|
||||||
|
|
||||||
|
## Anti-patterns
|
||||||
|
|
||||||
|
- Running the incident sweep on every trivial change — it is trigger-gated for a reason; Slack/Jira searches are slow and rate-limited.
|
||||||
|
- Blocking on vague similarity ("this area had incidents once") — a 🔴 requires a concrete reintroduction path tied to a specific incident or originating commit.
|
||||||
|
- Searching by file paths or function names — the incident record doesn't know them; translate to symptom vocabulary first.
|
||||||
|
- Skipping Part A because no prior-art agent is configured — archaeology is local git work and always available.
|
||||||
|
- Treating a clean historian result as wasted effort — "no prior art" is signal, and it belongs in the report as one line, not zero.
|
||||||
|
- Manufacturing findings from ordinary-feature-work commits to have something to say. Most deleted lines were not incident fixes.
|
||||||
Reference in New Issue
Block a user