Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/agents/content/skills/_data/artifact-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -839,13 +839,23 @@ Apply this gate **hardest** to R and S, where the low criticality bar invites fi

## Knowledge items

Knowledge items capture observations and learnings worth preserving. They are not findings: They have no criticality, are never merge-blocking, and are never emitted by code review skills. They appear in housekeeping artifacts (wrap-up inventories, chat summaries, devlogs) where conveying knowledge — not assigning blame or action — is the point.
Knowledge items capture observations and learnings worth preserving. They are not findings: they have no criticality and are never merge-blocking. They belong wherever knowledge is worth carrying forward — housekeeping artifacts (wrap-up inventories, chat summaries, devlogs), run summaries, and, when they clear the Insight gate below, review artifacts.

| ID | Category | Icon | Kind |
| ------ | -------- | ---- | --------- |
| `I{n}` | Insight | 💡 | knowledge |

Consumers that present insights (`wrap-up`, `summarize-chat`) should render the icon alongside the prefix or label to mirror the convention used for findings.
Consumers that present insights (`wrap-up`, `summarize-chat`, review skills and reviewer agents) should render the icon alongside the prefix or label to mirror the convention used for findings.

### Insight gate

An insight is the deliberate complement to a finding: a finding hands the author a decision to act on now; an insight preserves knowledge a future reader would otherwise rediscover. Reviewers may emit insights, but only through a gate as strict as the [Actionability gate](#actionability-gate) — "no severity, no action" is exactly the low bar that invites filler.

Emit an insight only when it is **non-obvious knowledge a future reader is materially worse off without**, and name that benefit. "A thing I noticed" does not qualify, nor does anything the code, its comments, or its tests already make plain.

**Insight vs. Suggestion (`S`).** Both are non-blocking, so they are easy to conflate; the test is whether an action is implied. An `S` proposes a change to make in this code now (and must clear the Actionability gate); an `I` records knowledge with no action attached. When an item implies a change the author should weigh, it is an `S`, not an insight — and when in doubt with any action implied, classify it as `S`.

Insights never carry criticality, never block a merge, and never count toward a review score or the [Overall criticality mapping](#overall-criticality-mapping).

## Artifact lifecycle

Expand Down
6 changes: 3 additions & 3 deletions packages/agents/content/skills/orchestrate/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -692,14 +692,14 @@ Write run-summary artifact to `{run-dir}/{NN}_orchestrator_run-summary.md`. The

## Insights

{Notable observations that emerged during the run. Include only items worth preserving — omit this section entirely if nothing notable emerged.
{Aggregate the `I{n}` insights emitted across this run's reviewer artifacts, deduplicating an insight that several reviewers raised into a single entry. Reviewers emit these under the insight gate, so prefer their vetted items over re-derived narration; add an orchestrator-level observation only when it is worth preserving and no reviewer already captured it. Include only items worth preserving — omit this section entirely if none emerged.

Examples of what belongs here:
What belongs here:

- Architectural patterns discovered or validated
- Design trade-offs surfaced during review
- Conventions or project-specific patterns learned
- Surprising findings from reviewers that revealed something non-obvious
- Non-obvious knowledge a reviewer flagged as an insight
- Technical debt or risks identified but not in scope to address}

## Deferred items
Expand Down
12 changes: 11 additions & 1 deletion packages/agents/content/skills/review-branch/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,11 @@ Uniquely number all issues for easy reference. See [finding scheme](../_data/art
- Recommendations: `R{n}` — advisable but discretionary
- Suggestions: `S{n}` — optional improvement
- Legacy: `{F,W,T,R,S}{n}-L` — observation in pre-existing code, not authored in this change. Same severity letter as the equivalent author finding; number it from that letter's shared sequence first, then append `-L` as a marker (e.g., after `F1`, `F2`, the first legacy FIXME is `F3-L`)
- Insights: `I{n}` — knowledge worth preserving, gated per the [insight gate](../review-criteria/SKILL.md#insight-gate); no severity, does not count against the score

## Output format

Section-header icons (🚨, ⚠️, 📋, 🧠, ☝️, 🔍) come from the canonical [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix); render them as shown. Each finding under "Action required" and "Areas for improvement" follows the canonical per-finding template shown below — see [`review-criteria` § Finding references](../review-criteria/SKILL.md#finding-references) for the rules governing the `Location:` field.
Section-header icons (🚨, ⚠️, 📋, 🧠, ☝️, 🔍) come from the canonical [finding scheme](../_data/artifact-conventions.md#finding-scheme-fwtrs--legacy-suffix), and the 💡 insights icon from [knowledge items](../_data/artifact-conventions.md#knowledge-items); render them as shown. Each finding under "Action required" and "Areas for improvement" follows the canonical per-finding template shown below — see [`review-criteria` § Finding references](../review-criteria/SKILL.md#finding-references) for the rules governing the `Location:` field.

When `ticket_ref` is null (no ticket on the branch), omit the `{ticket_ref}: ` portion so the heading reads naturally without it — e.g., `# Code review: {description}`.

Expand Down Expand Up @@ -155,6 +156,15 @@ The body following the frontmatter has this structure:

{Observations in pre-existing code, using severity-tagged IDs with `-L` suffix (e.g., `F3-L`, `T2-L`). Frame as future opportunities, don't count against score}

## Insights

{Knowledge worth preserving that is not a finding — a pattern, gotcha, or architectural learning surfaced during review. Gated per the [insight gate](../review-criteria/SKILL.md#insight-gate); no severity, does not count against the score. Omit this section entirely when there are none.}

### I1: {title}

- **Description:** {the insight and why a future reader is worse off without it}
- **Destination:** {optional: `ticket comment` or `devlog`}

## Technical assessment

## Conclusion
Expand Down
4 changes: 4 additions & 0 deletions packages/agents/content/skills/review-criteria/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ Findings use the canonical [finding scheme](../_data/artifact-conventions.md#fin

Before emitting any F/W/T/R/S finding, confirm it hands the author a concrete decision they can act on **in this change**: fix, defer with a ticket, or explicitly accept. Hedging language inside a finding ("no action this PR", "not actionable here", "just capturing a thought", "call it out only if X", "would matter once Y") is your own signal that it does not belong; drop it, don't soften it. A finding that endorses the current state and then proposes a change anyway is incoherent; drop it. Self-test: _would I make this exact change right now if it were my code?_ If no, it is not a finding. Apply this hardest to R and S. Full treatment, including where dropped content goes: [finding scheme § Actionability gate](../_data/artifact-conventions.md#actionability-gate).

## Insight gate

Reviewers may emit insights (`I{n}`) — knowledge worth preserving that is not a finding. An insight must clear a gate as strict as the Actionability gate: emit it only when it is non-obvious knowledge a future reader is materially worse off without, and name that benefit. Distinguish it from a Suggestion (`S`): an `S` proposes a change to make now; an `I` records knowledge with no action attached. When an action is implied, it is an `S`, not an insight. Number insights sequentially (`I1`, `I2`, …) in their own sequence, with no severity and no `-L` marker. Full treatment: [knowledge items § Insight gate](../_data/artifact-conventions.md#insight-gate).

## Finding concision

Compose each finding at the tight altitude ([concision principle](../_data/concision.md)): State the defect, its location, and the decision the author must make, then stop. Cut code the author can already see, hedged narration, and rationale for why you looked. Every reader pays for each line, so weigh each sentence against the decision it enables, not its completeness.
Expand Down
19 changes: 15 additions & 4 deletions packages/agents/content/skills/wrap-up/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,21 @@ Items at levels 3–4 remain in the standard findings pool for the housekeeping

The complexity assessment feeds into the cost-aware disposition flow described in [`_data/scope-and-deferral.md`](../_data/scope-and-deferral.md): Trivial items prefer **do now** (Phase 2a drive-bys); items that can't ship as drive-bys but share scope or source prefer **batch later** (Phase 2b batch action); substantive items get a **separate ticket** (Phase 2b per-item ticketing).

#### 1c. Scan for insights
#### 1c. Collect insights

Insights are notable observations worth preserving — patterns learned, surprising findings, or knowledge that would benefit future work.

**Structured sources** (high confidence). Which source applies is fixed by the run type detected in 1a — the two never both apply, so there is no structured-vs-structured overlap to dedup:

- **Orchestrated run → run-summary**: Read the `## Insights` section of the most recent `*_orchestrator_run-summary.md` in the run directory. It already aggregates and dedups the `I{n}` insights from every reviewer-subagent artifact in the run, so reading it — rather than the per-reviewer artifacts — captures each insight exactly once.
- **Non-orchestrated run → review artifact**: Read the `## Insights` section of the standalone review artifact (`*_reviewer_review.md`). Reviewer-subagent artifacts exist only in orchestrated runs, so outside orchestration this is the sole structured insight source.

Either way, reviewers emit these under the insight gate, so they are vetted knowledge, not heuristic guesses.

**Conversation scanning** (heuristic — may produce false positives):

Structured sources take precedence. When scanning conversation, skip any insight already captured from a structured source (the same observation) to avoid duplicates — a review-emitted insight and its conversation echo are one item, and the structured one wins.

Scan the conversation for:

- Architectural patterns discovered or validated
Expand All @@ -113,10 +124,10 @@ Scan the conversation for:

Look for language like: "interesting", "discovered", "realized", "turns out", "surprisingly", "TIL", "worth noting", "insight", "lesson", "gotcha", "caveat".

For each insight found, assign an `I{n}` ID (sequentially: I1, I2, ...) and suggest a destination:
For each insight (structured or conversation-scanned), assign an `I{n}` ID (sequentially: I1, I2, ...) and resolve a destination:

- `ticket comment` — if the insight relates to the current ticket's work
- `devlog` — if the insight is general knowledge not specific to one ticket
- Honor a destination the source insight already carries — a reviewer may suggest `ticket comment` or `devlog`.
- Otherwise: `ticket comment` if the insight relates to the current ticket's work; `devlog` if it is general knowledge not specific to one ticket.

If no ticket is available (from the session-context manifest), default all destinations to `devlog`.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- **Insights are gated too**: You may emit an insight (`I{n}`) — knowledge worth preserving that is not a finding — but only when it is non-obvious knowledge a future reader is materially worse off without; "a thing I noticed" is not one. An insight records knowledge with no action attached; the moment it implies a change to make now, it is a Suggestion (`S`), not an insight. Insights carry no severity and never affect criticality. See the `review-criteria` skill for the full Insight gate.
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
### Finalize (reserved last 3 turns)

Replace `### Criticality: (pending)` with the aggregate enum value (`none|low|medium|high`) and replace `### Summary`'s `(pending)` placeholder with the 1-2 sentence overall assessment.

Insights are not findings — they carry no severity and never affect the criticality aggregate; a review with only insights and no findings is still `none`. Put any emitted insights under the `### Insights` section, one `#### I{n}: {title}` per insight with a `- **Description:**` line and an optional `- **Destination:** ticket comment | devlog`; omit the section when there are none. Gate each insight per the `review-criteria` insight gate first.
<!-- children -->
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
### Interim writes (after each finding)

After each finding crystallizes, overwrite the artifact with the current findings appended under `### Findings`. `### Criticality:` stays `(pending)` and `### Summary` stays `(pending)` until finalize. Example interim form with one finding present:
After each finding crystallizes, overwrite the artifact with the current findings appended under `### Findings` (and any gated insights under `### Insights`). `### Criticality:` stays `(pending)` and `### Summary` stays `(pending)` until finalize. Example interim form with one finding present:

```markdown
### Criticality: (pending)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ run_id: '{run id}'

### Findings

(none yet)

### Insights

(none yet)
```

Expand Down
1 change: 1 addition & 0 deletions packages/agents/content/subagents/aspect-code-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ Scope re-reviews to your domain: project guideline compliance, bugs, and logic e

- **Only actionable findings**: No praise, no style nits outside project conventions
<!-- include: _partials/review-finding-actionability-gate.md / -->
<!-- include: _partials/review-insight-gate.md / -->
- **No false positives**: If you're not confident something is wrong, don't flag it
- **Context-aware**: Understand the codebase conventions before flagging violations. What looks wrong in isolation might be the established pattern.
- **Proportional**: A typo fix doesn't need the same scrutiny as a security-critical change. Match your depth to the risk.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ Scope re-reviews to your domain: error handling, catch blocks, fallback behavior

- **Only actionable findings**: No praise, no generic advice
<!-- include: _partials/review-finding-actionability-gate.md / -->
<!-- include: _partials/review-insight-gate.md / -->
- **No false positives**: If you're not confident something is a silent-failure risk, don't flag it
- **Context-aware**: Understand the codebase error-handling conventions before flagging violations
- **Proportional**: Match scrutiny to the risk level of the code being reviewed
Expand Down
1 change: 1 addition & 0 deletions packages/agents/content/subagents/aspect-test-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ Scope re-reviews to your domain: test coverage quality, behavioral gaps, and mis

- **Only actionable findings**: No praise, no generic "add more tests" advice
<!-- include: _partials/review-finding-actionability-gate.md / -->
<!-- include: _partials/review-insight-gate.md / -->
- **No false positives**: If you're not confident a test gap matters, don't flag it
- **Context-aware**: Understand the project's testing conventions and framework before flagging violations
- **Proportional**: Match scrutiny to the risk level of the untested behavior
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ If no findings:

- **Only actionable findings**: No praise, no style nits outside project conventions
<!-- include: _partials/review-finding-actionability-gate.md / -->
<!-- include: _partials/review-insight-gate.md / -->
- **No false positives**: If you're not confident a simplification improves the code, don't flag it
- **Context-aware**: Understand the codebase conventions before flagging opportunities. What looks verbose in isolation might be the established pattern.
- **Proportional**: Match your depth to the risk and scope of the change
Expand Down
1 change: 1 addition & 0 deletions packages/agents/content/subagents/orchestrated-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ When reviewing after a coder has responded to previous findings:

- **Only actionable findings**: No praise, no style nits, no "consider doing X" without clear justification
<!-- include: _partials/review-finding-actionability-gate.md / -->
<!-- include: _partials/review-insight-gate.md / -->
- **No false positives**: If you're not confident something is wrong, don't flag it. Every finding should be worth the coder's time.
- **Context-aware**: Understand the codebase conventions before flagging violations. What looks wrong in isolation might be the established pattern.
- **Proportional**: A typo fix doesn't need the same scrutiny as a security-critical change. Match your depth to the risk.
Expand Down
Loading