From 0054fc6508ba153c5f31db9db2db2a6b694c11f6 Mon Sep 17 00:00:00 2001 From: matze4u <171579628+matze4u@users.noreply.github.com> Date: Tue, 2 Jun 2026 14:06:45 +0200 Subject: [PATCH 1/2] feat(core): wire review rules + settings into live reviewSummary (T-CR-501/502) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHandler.reviewSummary ran the review engine with hard-coded defaults and never loaded the workspace review config — the shipped, tested Phase-5 seams (loadReviewRules, ReviewSettings helpers) had zero live callers. Thread both into the live IDE review path: - Project `.event4u-agent/review-rules.md` now feeds the Stage-1 prompt via pipeline.rules (T-CR-501). - New review/settings-source.ts loadReviewSettings() reads `.agent-settings.yml :: review` (fail-open to schema defaults, no AgentSettingsSchema coupling). Its values drive runReview's vote options (voteOptionsFromSettings) and the severity floor (applySeverityFloor), applied to issues + potentialIssues BEFORE summarizeReview so all counts stay consistent (T-CR-502). Both readers are optional injected GitHandlerDeps (default to the real file readers) so the handler stays pure and unit-testable. The severity floor never hides a security finding (security_always_error exemption), regression-tested. Pure-core, additive: no protocol/DTO/codegen change. Council (gemini + codex, 2026-06-02): Q0/Q2/Q3/Q4 unanimous; Q1 (settings source) synthesised. ADR-051. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/git/handler.test.ts | 161 ++++++++++++++++++ packages/core/src/git/handler.ts | 48 +++++- .../core/src/review/settings-source.test.ts | 49 ++++++ packages/core/src/review/settings-source.ts | 49 ++++++ 4 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/review/settings-source.test.ts create mode 100644 packages/core/src/review/settings-source.ts diff --git a/packages/core/src/git/handler.test.ts b/packages/core/src/git/handler.test.ts index 3bc06cd..ab09364 100644 --- a/packages/core/src/git/handler.test.ts +++ b/packages/core/src/git/handler.test.ts @@ -9,6 +9,7 @@ import { TrackingDb } from '../tracking/db.js'; import { CapsEvaluator } from '../tracking/caps.js'; import { PricingBook } from '../pricing/loader.js'; import { GitHandler, GitRequestError } from './handler.js'; +import { resolveReviewSettings } from '../review/config.js'; /** * A backend that returns one scripted reply per `stream()` call, clamping to @@ -167,6 +168,166 @@ describe('GitHandler.reviewSummary', () => { }); }); +describe('GitHandler.reviewSummary — Phase-5 config + rules wiring (T-CR-501/502)', () => { + /** Records the system prompt of every stage call; returns no findings. */ + function capturingBackend(systems: string[]): LlmBackend { + return { + id: 'fake', + mode: 'api', + async *stream(req: LlmRequest): AsyncIterable { + systems.push(req.system ?? ''); + yield { kind: 'text_delta', text: '' }; + yield { kind: 'stop', reason: 'end_turn', usage: { input_tokens: 1, output_tokens: 1 } }; + }, + }; + } + + /** Stage-aware backend that emits the given findings (single-pass review). */ + function findingsBackend(issues: unknown[]): LlmBackend { + return { + id: 'fake', + mode: 'api', + async *stream(req: LlmRequest): AsyncIterable { + const sys = req.system ?? ''; + let tool: { name: string; input: unknown }; + if (sys.includes('skeptical second reviewer')) { + const content = + typeof req.messages[0]?.content === 'string' ? req.messages[0].content : ''; + const ids = [...content.matchAll(/\[([^\]]+)\]/g)].map((m) => m[1]); + tool = { + name: 'submit_decisions', + input: { decisions: ids.map((id) => ({ issueId: id, keep: true })) }, + }; + } else if (sys.includes('stress-testing')) { + tool = { name: 'submit_findings', input: { changeSummary: '', issues: [] } }; + } else { + tool = { name: 'submit_findings', input: { changeSummary: 'sums up', issues } }; + } + yield { kind: 'tool_use_start', id: 'x', name: tool.name }; + yield { kind: 'tool_use_end', id: 'x', name: tool.name, input: tool.input }; + yield { kind: 'stop', reason: 'tool_use', usage: { input_tokens: 10, output_tokens: 5 } }; + }, + }; + } + + // Findings quote lines present in the test DIFF's hunks, so span validation + // resolves them via the hunk fallback (no observer.readFile needed). + const LOW_BUG = { + file: 'src/foo.ts', + startLine: 3, + endLine: 3, + verbatimSnippet: 'const c = 4;', + description: 'unused const', + severity: 'low', + category: 'bug', + confidence: 0.9, + }; + const LOW_SECURITY = { + file: 'src/foo.ts', + startLine: 2, + endLine: 2, + verbatimSnippet: 'const b = 3;', + description: 'weak value', + severity: 'low', + category: 'security', + confidence: 0.9, + }; + + it('threads the workspace review rules into the Stage-1 prompt (T-CR-501)', async () => { + const systems: string[] = []; + const handler = new GitHandler({ + resolveBackend: () => capturingBackend(systems), + resolveModel: () => 'claude-sonnet-4-6', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewRules: async () => 'PROJECT-RULE-ZZZ: forbid magic numbers', + loadReviewSettings: async () => resolveReviewSettings(), + }); + await handler.reviewSummary({ cwd: '/repo', source: 'unstaged' }); + // Stage 1 ('senior code reviewer') carries the project rules; the rules are + // only ever injected into that stage's system prompt. + const stage1 = systems.find((s) => s.includes('senior code reviewer')); + expect(stage1).toContain('PROJECT-RULE-ZZZ'); + }); + + it('does not inject a rules block when no review-rules.md is present', async () => { + const systems: string[] = []; + const handler = new GitHandler({ + resolveBackend: () => capturingBackend(systems), + resolveModel: () => 'claude-sonnet-4-6', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewRules: async () => undefined, + loadReviewSettings: async () => resolveReviewSettings(), + }); + await handler.reviewSummary({ cwd: '/repo', source: 'unstaged' }); + expect(systems.every((s) => !s.includes('Project-specific review rules'))).toBe(true); + }); + + it('passes the settings group_size through as the vote group size (T-CR-502)', async () => { + // groupSize 5 runs the review 5× per group; groupSize 1 disables the vote + // (one pass). Equal scripted replies → call count scales exactly 5×. + const calls = { one: 0, five: 0 }; + const countingBackend = (counter: { n: number }): LlmBackend => ({ + id: 'fake', + mode: 'api', + async *stream(_req: LlmRequest): AsyncIterable { + counter.n += 1; + yield { kind: 'text_delta', text: '' }; + yield { kind: 'stop', reason: 'end_turn', usage: { input_tokens: 1, output_tokens: 1 } }; + }, + }); + const c1 = { n: 0 }; + const c5 = { n: 0 }; + await new GitHandler({ + resolveBackend: () => countingBackend(c1), + resolveModel: () => 'm', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewSettings: async () => resolveReviewSettings({ group_size: 1 }), + }).reviewSummary({ cwd: '/repo', source: 'unstaged' }); + await new GitHandler({ + resolveBackend: () => countingBackend(c5), + resolveModel: () => 'm', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewSettings: async () => resolveReviewSettings({ group_size: 5 }), + }).reviewSummary({ cwd: '/repo', source: 'unstaged' }); + calls.one = c1.n; + calls.five = c5.n; + expect(calls.one).toBeGreaterThan(0); + expect(calls.five).toBe(calls.one * 5); + }); + + it('applies the severity floor before summarising, but never hides security (T-CR-502)', async () => { + // No floor (info): both the low bug and low security finding survive. + const open = await new GitHandler({ + resolveBackend: () => findingsBackend([LOW_BUG, LOW_SECURITY]), + resolveModel: () => 'm', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewSettings: async () => + resolveReviewSettings({ group_size: 1, severity_floor: 'info' }), + }).reviewSummary({ cwd: '/repo', source: 'unstaged' }); + expect(open.totalFindings).toBe(2); + + // Floor 'high' drops the low *bug*, but the low *security* finding is exempt + // (security_always_error defaults on) — the council-flagged trap. + const floored = await new GitHandler({ + resolveBackend: () => findingsBackend([LOW_BUG, LOW_SECURITY]), + resolveModel: () => 'm', + defaultCwd: '/repo', + runner: fakeRunner(), + loadReviewSettings: async () => + resolveReviewSettings({ group_size: 1, severity_floor: 'high' }), + }).reviewSummary({ cwd: '/repo', source: 'unstaged' }); + expect(floored.totalFindings).toBe(1); + expect(floored.topFindings).toHaveLength(1); + expect(floored.topFindings[0]?.category).toBe('security'); + expect(floored.topFindings[0]?.severity).toBe('low'); + }); +}); + describe('GitHandler.reviewSummary — tracked observer wiring (T-CR-206)', () => { const PRICING_YAML = ` version: 3 diff --git a/packages/core/src/git/handler.ts b/packages/core/src/git/handler.ts index ee12b8c..d7e4398 100644 --- a/packages/core/src/git/handler.ts +++ b/packages/core/src/git/handler.ts @@ -50,6 +50,13 @@ import { getDiff, type DiffSource } from '../review/diff-source.js'; import { runReview } from '../review/run.js'; import { createTrackedReviewObserver } from '../review/observer.js'; import { CapsBlockedError } from '../review/pipeline.js'; +import { loadReviewRules } from '../review/rules.js'; +import { loadReviewSettings } from '../review/settings-source.js'; +import { + applySeverityFloor, + voteOptionsFromSettings, + type ReviewSettings, +} from '../review/config.js'; import type { TrackingDb } from '../tracking/db.js'; import type { CapsEvaluator } from '../tracking/caps.js'; import type { PricingBook } from '../pricing/loader.js'; @@ -110,6 +117,16 @@ export interface GitHandlerDeps { * surfaces as a coded `cost_cap_blocked` error. */ caps?: CapsEvaluator; + /** + * Workspace review-config readers (road-to-code-review.md Phase 5). Both + * default to the real file readers; tests inject fakes. `reviewSummary` + * threads the loaded `review-rules.md` into the Stage-1 prompt (T-CR-501) + * and the `.agent-settings.yml :: review` block into the group-vote options + * + severity floor (T-CR-502). Each reader fails open (missing/invalid → + * no rules / default settings) so a broken config never breaks review. + */ + loadReviewRules?: (cwd: string) => Promise; + loadReviewSettings?: (cwd: string) => Promise; } const ALL_SEVERITIES = Object.keys(SEVERITY_RANK) as Severity[]; @@ -118,11 +135,17 @@ export class GitHandler { private readonly runner: GitRunner; private readonly maxTokens: number; private readonly maxCommitAttempts: number; + private readonly loadReviewRules: (cwd: string) => Promise; + private readonly loadReviewSettings: (cwd: string) => Promise; constructor(private readonly deps: GitHandlerDeps) { this.runner = deps.runner ?? defaultGitRunner; this.maxTokens = deps.maxTokens ?? 2048; this.maxCommitAttempts = Math.max(1, deps.maxCommitAttempts ?? 2); + // Default to the real file readers (module-scope imports); the bare names + // resolve to the imports, `this.loadReview*` to these private fields. + this.loadReviewRules = deps.loadReviewRules ?? loadReviewRules; + this.loadReviewSettings = deps.loadReviewSettings ?? loadReviewSettings; } /** @@ -217,6 +240,16 @@ export class GitHandler { const backend = this.deps.resolveBackend(req.providerId); const model = this.deps.resolveModel(req.providerId); + // Workspace review config (Phase 5): the project `review-rules.md` feeds the + // Stage-1 prompt (T-CR-501); the `review:` settings drive the group-vote + // options + severity floor (T-CR-502). Both reads are best-effort and run + // unconditionally (council Q3=A) — the readers fail open, so a missing file + // yields no rules / default settings. + const [rules, settings] = await Promise.all([ + this.loadReviewRules(cwd), + this.loadReviewSettings(cwd), + ]); + // Wire the tracked observer (T-CR-206) only when a pricing book + tracking // trail are available — recording no-ops without pricing (the same gate the // chat/agent step recorder uses). The review action has no conversation, so @@ -240,8 +273,10 @@ export class GitHandler { cwd, source, runner: this.runner, + vote: voteOptionsFromSettings(settings), pipeline: { config: { model, maxTokens: this.maxTokens }, + ...(rules ? { rules } : {}), ...(observer ? { observer } : {}), }, }); @@ -255,7 +290,18 @@ export class GitHandler { throw error; } const changes = await getDiff(cwd, source, this.runner); - const summary = summarizeReview(result, changes); + // Apply the severity floor BEFORE summarising (council Q2=A) so the counts, + // top findings, and potential tally all reflect the same filtered set. + // `applySeverityFloor` exempts `security` findings when `security_always_error` + // is on — they are never hidden by the floor (the council-flagged trap). + const summary = summarizeReview( + { + ...result, + issues: applySeverityFloor(result.issues, settings), + potentialIssues: applySeverityFloor(result.potentialIssues, settings), + }, + changes, + ); const findingsBySeverity: GitSeverityCount[] = ALL_SEVERITIES.map((severity) => ({ severity, diff --git a/packages/core/src/review/settings-source.test.ts b/packages/core/src/review/settings-source.test.ts new file mode 100644 index 0000000..97d01ec --- /dev/null +++ b/packages/core/src/review/settings-source.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; +import { loadReviewSettings, type SettingsReader } from './settings-source.js'; + +/** A reader that returns fixed YAML text, or rejects to simulate a missing file. */ +function reader(yaml: string | Error): SettingsReader { + return { + read: () => (yaml instanceof Error ? Promise.reject(yaml) : Promise.resolve(yaml)), + }; +} + +describe('loadReviewSettings', () => { + it('parses the review block and applies the values', async () => { + const settings = await loadReviewSettings( + '/repo', + reader('review:\n group_size: 1\n severity_floor: high\n security_always_error: false\n'), + ); + expect(settings.group_size).toBe(1); + expect(settings.severity_floor).toBe('high'); + expect(settings.security_always_error).toBe(false); + // Unset keys fall back to schema defaults. + expect(settings.label_threshold).toBe(4); + }); + + it('returns full defaults when the file is missing (fail-open)', async () => { + const settings = await loadReviewSettings('/repo', reader(new Error('ENOENT'))); + expect(settings.group_size).toBe(5); + expect(settings.severity_floor).toBe('info'); + expect(settings.security_always_error).toBe(true); + }); + + it('returns defaults when the file has no review block', async () => { + const settings = await loadReviewSettings('/repo', reader('llm:\n default_mode: api\n')); + expect(settings.group_size).toBe(5); + expect(settings.severity_floor).toBe('info'); + }); + + it('returns defaults when the review block is malformed (fail-open, no throw)', async () => { + // group_size must be a positive int — a string makes the schema throw; the + // reader catches it and resolves to defaults rather than breaking review. + const settings = await loadReviewSettings('/repo', reader('review:\n group_size: "five"\n')); + expect(settings.group_size).toBe(5); + expect(settings.severity_floor).toBe('info'); + }); + + it('returns defaults when the YAML is unparseable (fail-open)', async () => { + const settings = await loadReviewSettings('/repo', reader('review: : : not yaml\n - [')); + expect(settings.group_size).toBe(5); + }); +}); diff --git a/packages/core/src/review/settings-source.ts b/packages/core/src/review/settings-source.ts new file mode 100644 index 0000000..347dd92 --- /dev/null +++ b/packages/core/src/review/settings-source.ts @@ -0,0 +1,49 @@ +/** + * Workspace review-settings reader (road-to-code-review.md Phase 5, T-CR-502). + * + * Sibling of `rules.ts` `loadReviewRules`: where that reads the project's + * `review-rules.md`, this reads the `review:` block from the consumer's + * `.agent-settings.yml` and resolves it through {@link resolveReviewSettings} + * (applying schema defaults). The settings-UI that *writes* these values is + * the IDE client layer (the MVP T-204 pattern); the Core only reads. + * + * Fail-open by construction (mirrors `main.ts`'s best-effort settings read): + * a missing file, malformed YAML, or an invalid `review:` block all resolve to + * the default `ReviewSettings` rather than throwing — a broken config must + * never break the review action. + */ + +import { readFile as fsReadFile } from 'node:fs/promises'; +import { resolve } from 'node:path'; +import { parse as parseYaml } from 'yaml'; +import { resolveReviewSettings, type ReviewSettings } from './config.js'; + +/** Consumer settings file the `review:` block is read from. */ +export const AGENT_SETTINGS_PATH = '.agent-settings.yml'; + +export interface SettingsReader { + read(path: string): Promise; +} + +const defaultReader: SettingsReader = { + read: (path) => fsReadFile(path, 'utf8'), +}; + +/** + * Load review settings from `/.agent-settings.yml :: review`, applying + * `ReviewSettingsSchema` defaults. Returns the full default settings when the + * file is absent, the YAML is malformed, or the `review` block fails schema + * validation (fail-open — the floor/vote wiring still works on defaults). + */ +export async function loadReviewSettings( + cwd: string, + reader: SettingsReader = defaultReader, +): Promise { + try { + const text = await reader.read(resolve(cwd, AGENT_SETTINGS_PATH)); + const parsed = parseYaml(text) as { review?: unknown } | null; + return resolveReviewSettings(parsed?.review); + } catch { + return resolveReviewSettings(undefined); + } +} From 21872f616ac5095fab3152f1669712a7925e106c Mon Sep 17 00:00:00 2001 From: matze4u <171579628+matze4u@users.noreply.github.com> Date: Tue, 2 Jun 2026 14:06:57 +0200 Subject: [PATCH 2/2] docs: ADR-051 review config+rules wiring; T-CR-501/502 done-notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record the decision for threading the dead Phase-5 review config/rules seam into the live reviewSummary path: the Q0/Q2/Q3/Q4-unanimous council answers, the Q1 settings-source synthesis (review-owned fail-open reader injected as a dep, no AgentSettingsSchema extension), and the security-exemption trap both members flagged. Add the index row and update the T-CR-501/502 done-comments to cite the live wiring (no checkbox flip — both were already [x]). Co-Authored-By: Claude Opus 4.8 (1M context) --- agents/roadmaps/road-to-code-review.md | 4 +- .../adr/ADR-051-review-config-rules-wiring.md | 120 ++++++++++++++++++ docs/adr/index.md | 1 + 3 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 docs/adr/ADR-051-review-config-rules-wiring.md diff --git a/agents/roadmaps/road-to-code-review.md b/agents/roadmaps/road-to-code-review.md index 7f5aa9f..418067a 100644 --- a/agents/roadmaps/road-to-code-review.md +++ b/agents/roadmaps/road-to-code-review.md @@ -152,8 +152,8 @@ complexity: heavy > **Goal.** Make the reviewer configurable and rule-aware, and absorb slip. -- [x] **T-CR-501 — Workspace review rules.** A `.event4u-agent/review-rules.md` (our analog of sweep's `SWEEP.md`, `user_prompt_special_rules_format`) whose contents are injected as project-specific review criteria. Compatible with agent-config guidelines (a project can point at `docs/guidelines/`). -- [x] **T-CR-502 — Settings.** `review.group_size` (default 5, lower for cost), `review.label_threshold` (default 4), `review.severity_floor` (hide Information-level), `review.security_always_error` (default true). Surfaced in the settings UI (MVP T-204 pattern). +- [x] **T-CR-501 — Workspace review rules.** A `.event4u-agent/review-rules.md` (our analog of sweep's `SWEEP.md`, `user_prompt_special_rules_format`) whose contents are injected as project-specific review criteria. Compatible with agent-config guidelines (a project can point at `docs/guidelines/`). +- [x] **T-CR-502 — Settings.** `review.group_size` (default 5, lower for cost), `review.label_threshold` (default 4), `review.severity_floor` (hide Information-level), `review.security_always_error` (default true). Surfaced in the settings UI (MVP T-204 pattern). - [-] **T-CR-503 — Auto-review-on-stage (opt-in).** Optional hook: when files are staged, kick a background review (respecting caps). Off by default; a single low-friction toggle. Reuses the hook system (`road-to-v1-0.md` Phase 11 T-1106) when available. - [x] **T-CR-504 — Dedup against prior review.** Within a session, don't re-surface a finding the user already dismissed for the same unchanged hunk. Local-only dismissal state under `.event4u-agent/`. - [-] **T-CR-505 — Pull-up slot.** Any T-CR-xxx deferred from earlier phases (e.g. T-CR-205 duplicate detection if Phase 8 was absent) lands here. diff --git a/docs/adr/ADR-051-review-config-rules-wiring.md b/docs/adr/ADR-051-review-config-rules-wiring.md new file mode 100644 index 0000000..7e7e471 --- /dev/null +++ b/docs/adr/ADR-051-review-config-rules-wiring.md @@ -0,0 +1,120 @@ +--- +adr: 051 +title: Review Config + Rules Wiring — Threading The Dead Phase-5 loadReviewRules / ReviewSettings Into The Live reviewSummary Path (T-CR-501/502) +status: Proposed (drafted 2026-06-02 — awaits user sign-off before flip to Accepted) +deciders: solo-dev (event4u team lead) — sign-off required +consulted: AI Council — gemini-cli 0.41.2 (Q0=A, Q1=C, Q2=A, Q3=A, Q4=A) + codex-cli 0.134.0 (Q0=A, Q1=B, Q2=A, Q3=A, Q4=A), both 2026-06-02, run serially (`gemini -p` / `codex exec --skip-git-repo-check`). Both answered cleanly this round (no codex hang). The one divergence is Q1 (settings source) — synthesised below; Q0/Q2/Q3/Q4 unanimous. +related: sibling of the wire-a-dead-seam ADRs (ADR-048 command-palette data path, ADR-050 config registry data path). Builds on ADR-042 (review tracked-observer wiring) — the same live `reviewSummary` entry point. The settings WRITE surface (settings UI) remains the IDE client layer (MVP T-204 pattern); this ADR is the Core READ + apply path. +date: 2026-06-02 +--- + +# ADR-051 — Review Config + Rules Wiring (T-CR-501/502) + +## Status + +**Proposed** — awaits sign-off. One branch / one PR, committed in logical +chunks (core seam → tests → docs), preserving minimal-safe-diff. + +CI-verified locally: `task ci` exit 0 (lint, format, build, typecheck, test — +core 1141 pass / 1 skip, +11 over baseline). Pure-core TypeScript: no protocol +/ DTO / codegen change, so the JetBrains + package matrix jobs are unaffected. +**No checkbox flip** — T-CR-501/502 are already `[x]` (their cores shipped +2026-05-29); this PR lands the live wiring those cores were built for, and the +done-comments are updated to cite it. + +## Context + +road-to-code-review.md Phase 5 shipped two pieces tested but with **zero live +callers** — a dead seam, the exact shape ADR-048/050 wired before: + +- `review/rules.ts::loadReviewRules(cwd)` reads `.event4u-agent/review-rules.md` + and returns the text (or `undefined`). `ReviewPipelineOptions.rules` already + exists and is consumed by `prompts.ts::stage1System(rules)`. But the live + review entry point — `GitHandler.reviewSummary` (ADR-042) — never set + `pipeline.rules`, so a project's review rules never reached the model. +- `review/config.ts` defines `ReviewSettingsSchema` + `resolveReviewSettings` + + `voteOptionsFromSettings` + `applySeverityFloor` (security-exempt). All + four had zero callers: `reviewSummary` always ran with the hard-coded + defaults (groupSize 5) and never applied a severity floor. + +The capability gap is real: the review engine was configurable and +rules-aware by construction, but the only path that runs it ignored both. + +## Decision + +Thread both into `reviewSummary` as one slice. Per council (Q0=A unanimous): +wire rules **and** settings together — they share the one edit and the same +"read the workspace config" step. + +- **Rules (T-CR-501).** `reviewSummary` loads the rules best-effort and passes + them as `pipeline.rules`; `stage1System` injects them into the Stage-1 + system prompt. Unconditional (Q3=A unanimous) — the reader already + fail-opens, so a missing file simply yields no rules block. +- **Settings (T-CR-502).** `reviewSummary` resolves `ReviewSettings`, maps them + through `voteOptionsFromSettings` into `runReview({ vote })` (so `group_size` + / thresholds drive the group-vote), and applies `applySeverityFloor` to both + `result.issues` and `result.potentialIssues` **before** `summarizeReview` + (Q2=A unanimous) so the counts, top findings, and potential tally all reflect + the same filtered set. +- **Testability (Q4=A unanimous).** `GitHandlerDeps` gains two optional readers + — `loadReviewRules?` and `loadReviewSettings?` — defaulting to the real file + readers; tests inject fakes. Separate readers (not one combined provider) + keep their independent failure modes visible. +- **Settings source (Q1 — reasoned synthesis).** gemini chose C (inject a + resolved reader into deps, keep the handler pure); codex chose B (keep review + config owned by `review/`, no `config/ → review/` coupling). The Q4=A + injection is unanimous and reconciles both: the **default reader lives in + `review/settings-source.ts`** (codex B — self-contained, reads + `.agent-settings.yml :: review` via the existing `yaml` dep, fail-open to + defaults; `AgentSettingsSchema` is **not** extended, so no config→review + coupling), and it is **injected as an optional `GitHandlerDeps` reader** + (gemini C — the handler stays pure and the sidecar can override). Rejected + the alternative of extending `AgentSettingsSchema` (couples the config reader + to the review schema) and of adding the settings to the `GitReviewSummary` + wire DTO (the values are persisted in the workspace file, not passed + per-call). + +## Consequences + +- A project's `.event4u-agent/review-rules.md` now shapes the review prompt, + and the `review:` block in `.agent-settings.yml` now tunes vote size, + thresholds, and the severity floor — both on the live IDE "Review changes" + path. +- **The council-flagged trap** (both members named it): the severity floor must + never hide a `security` finding. `applySeverityFloor` exempts + `category === 'security'` when `security_always_error` is on; a regression + test asserts a low-severity security finding survives a `high` floor while a + low-severity bug is dropped. +- No behaviour change when no config is present: missing `review-rules.md` → + no rules; missing/invalid `review:` block → default settings (groupSize 5, + floor `info` = keep all). Identical output to before for an unconfigured + workspace. +- Pure-core, additive: no protocol/DTO change, no new dependency (`yaml` was + already a core dep), `CommandHandler`/other handlers untouched. + +## Alternatives considered + +- **Leave the seam dead (Q0=D).** Rejected — the cores were built for this + exact wiring; leaving them unreachable is the gap, not a safe default. +- **Extend `AgentSettingsSchema` with a `review` key (Q1=A).** Rejected — + couples the generic settings reader to the review schema for a single narrow + read; the self-contained `review/` reader keeps the boundary clean. +- **Pass settings on the wire DTO (Q1=D).** Rejected — settings are persisted + workspace config the Core reads, not a per-request transport input. +- **Apply the floor only to `topFindings` after summarising (Q2=B).** Rejected + — the per-severity counts and potential tally would then disagree with the + shown findings. + +## References + +- `packages/core/src/git/handler.ts::reviewSummary` — the live entry point now + threading rules + settings. +- `packages/core/src/review/settings-source.ts` — the new fail-open + `.agent-settings.yml :: review` reader (the wired settings seam). +- `packages/core/src/review/rules.ts::loadReviewRules` — the wired rules seam. +- `packages/core/src/review/config.ts` — `resolveReviewSettings`, + `voteOptionsFromSettings`, `applySeverityFloor` (the wired settings helpers). +- ADR-042 — the `reviewSummary` tracked-observer wiring this extends. +- ADR-048 / ADR-050 — the sibling wire-a-dead-seam data-path ADRs. +- `agents/roadmaps/road-to-code-review.md` T-CR-501 (rules) / T-CR-502 + (settings) — the Phase-5 cores wired here. diff --git a/docs/adr/index.md b/docs/adr/index.md index c19ad01..2efadde 100644 --- a/docs/adr/index.md +++ b/docs/adr/index.md @@ -54,6 +54,7 @@ | [ADR-048](ADR-048-command-palette-data-path.md) | Command-Palette Data Path — Wiring The Dead Picker + Loader As commandList / commandRead Protocol Methods (T-402 / T-1103) | Proposed | 2026-06-02 | road-to-v1-0 T-402/T-1103 — the shipped-but-dead `commands/picker.ts` (`commandsToPickerItems`/`pickCommands`) + `commands/loader.ts` (`loadCommandProcedure`) had zero live callers; the live collaborator `walkAgentConfig` is already walked (rules loader, ADR-043). Adds two read-only methods on a new `CommandHandler` over the walk-once-cached command index: `commandList {query?}` (empty → all alphabetical, query → ranked; `total` = match count before a `MAX_COMMAND_LIST_RESULTS=100` cap) + `commandRead {name}` (MCP-first/local-fallback body), behind `requireCommands()`. Council UNANIMOUS Q0=A (wire the data path), Q1=B (both methods), Q2=A (one `commandList{query?}`), Q3=A (`CommandSummary {name,description,path}` = PickerItem), Q4=A (keep absolute path for IDE click-through), Q6=A; Q5 born-dead risk → core stays the resolution authority. +5 DTOs (59→64); `jetbrains:check` BUILD SUCCESSFUL; no checkbox flip (the overlay render + invocation UX stay IDE) | | [ADR-049](ADR-049-retire-dead-context-inject.md) | Retire The Dead Context-Injection Twin — context/inject.ts Superseded By The Live buildContextInjection (T-605 / T-606) | Proposed | 2026-06-02 | road-to-v1-0 T-605/T-606 — `context/inject.ts` (`buildContextBlock`/`injectContext`, user-message placement) was the recorded T-605/T-606 impl but had zero live callers; the live path is `chat/context-injection.ts::buildContextInjection` (T-MR13, ADR-025, system-prompt placement) used in both handlers. Retires the dead twin + its test. Council UNANIMOUS Q0=A (retire), Q1=A (dedicated ADR mirroring ADR-039), Q2=A (T-605/T-606 stay `[x]`, done-comments updated to cite the live impl), Q3=A (nothing to port — the fixed 8000-char cap supersedes the dynamic 20%-window budget), Q4 no trap. No protocol/codegen change; no checkbox flip (capability is live via the system-prompt placement) | | [ADR-050](ADR-050-config-list-skills-rules-data-path.md) | Agent-Config Registry Data Path — Wiring The Dead indexByKind As A configList Protocol Method For Skills + Rules (T-401) | Proposed | 2026-06-02 | road-to-v1-0 T-401 — `agent-config-walker.ts::indexByKind` (groups walked `ConfigNode[]` into `{skill,rule,command}`) was unit-tested with zero live callers; the only shipped data path (ADR-048 `CommandHandler`) filters to `kind==='command'`, so skills + rules — also walked — had no protocol path for a skill picker / rules viewer. Adds read-only `configList {kind?, limit?}` → `{items: ConfigSummary[], total}` (`ConfigSummary {kind,name,description,path}`) on a dedicated `ConfigHandler` behind `requireConfig()`, the local-walker authority/offline sibling of the MCP `listSkills` (T-1102). Council (gemini-cli; codex-cli hung >7 min, terminated) Q0=A (wire it), Q1=B (all kinds, kind-filterable), Q2=A (optional `kind`), Q3=B (flat `{items,total}` + per-item kind), Q4=A (path for click-through), Q5=A (lightweight only), Q7=A (`MAX_CONFIG_LIST_RESULTS=100` + total); Q6 reasoned divergence — dedicated handler over extending `CommandHandler` (one-handler-per-domain + per-handler fail-open walk cache vs a shared promise caching a transient FS error). +3 DTOs (64→67); `task ci` exit 0; `jetbrains:check` BUILD SUCCESSFUL; no checkbox flip (the picker/viewer render stays IDE) | +| [ADR-051](ADR-051-review-config-rules-wiring.md) | Review Config + Rules Wiring — Threading The Dead Phase-5 loadReviewRules / ReviewSettings Into The Live reviewSummary Path (T-CR-501/502) | Proposed | 2026-06-02 | road-to-code-review Phase 5 — `loadReviewRules` + `ReviewSettings` helpers (`resolveReviewSettings`/`voteOptionsFromSettings`/`applySeverityFloor`) shipped tested with zero live callers; `GitHandler.reviewSummary` (ADR-042) ran with hard-coded defaults and ignored both. Threads project `review-rules.md` into the Stage-1 prompt (T-CR-501) and the `.agent-settings.yml :: review` block into the group-vote options + severity floor applied BEFORE `summarizeReview` (T-CR-502). Council (gemini + codex, both clean) Q0=A (wire rules+settings together), Q2=A (floor before summarise), Q3=A (unconditional best-effort load), Q4=A (inject optional `loadReviewRules?`/`loadReviewSettings?` readers into `GitHandlerDeps`); Q1 divergence (gemini C inject / codex B no config→review coupling) synthesised — default reader lives in `review/settings-source.ts` (no `AgentSettingsSchema` extension) and is injected as a dep. Trap both flagged: severity floor must never hide a `security` finding — `applySeverityFloor` exempts it, regression-tested. Pure-core (no DTO/codegen change); `task ci` exit 0 (core +11 tests); no checkbox flip (T-CR-501/502 already `[x]`; done-comments cite this wiring) | ## Status legend