feat(core): wire review rules + settings into live reviewSummary (T-CR-501/502)

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.** <!-- done 2026-05-29: rules.ts loadReviewRules() reads .event4u-agent/review-rules.md (injected reader, trim, undefined-when-empty) and the text flows into stage1System(rules); tested. A project can point the file at docs/guidelines/. --> 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.** <!-- done 2026-05-29: config.ts ReviewSettingsSchema (group_size/label_threshold/potential_threshold/severity_floor/security_always_error/auto_review_on_stage) + resolveReviewSettings + voteOptionsFromSettings + applySeverityFloor (security exempt from the floor); tested. The settings-UI surface (MVP T-204 pattern) is the client layer on top. --> `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.** <!-- done 2026-05-29: rules.ts loadReviewRules() reads .event4u-agent/review-rules.md (injected reader, trim, undefined-when-empty) and the text flows into stage1System(rules); tested. A project can point the file at docs/guidelines/. LIVE-WIRED 2026-06-02 (ADR-051): GitHandler.reviewSummary now loads the rules best-effort (injected loadReviewRules dep) and passes them as pipeline.rules, so they reach the Stage-1 prompt on the real IDE review path; handler test asserts the rules text lands in the stage-1 system prompt. --> 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.** <!-- done 2026-05-29: config.ts ReviewSettingsSchema (group_size/label_threshold/potential_threshold/severity_floor/security_always_error/auto_review_on_stage) + resolveReviewSettings + voteOptionsFromSettings + applySeverityFloor (security exempt from the floor); tested. The settings-UI surface (MVP T-204 pattern) is the client layer on top. LIVE-WIRED 2026-06-02 (ADR-051): new review/settings-source.ts loadReviewSettings() reads `.agent-settings.yml :: review` (fail-open to defaults); GitHandler.reviewSummary maps it through voteOptionsFromSettings into runReview's vote options and applies applySeverityFloor to issues + potentialIssues BEFORE summarizeReview. Handler tests assert group_size threads through and the floor drops a low bug while the security exemption keeps a low security finding. --> `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).** <!-- skipped 2026-05-29: the hook system (road-to-v1-0.md Phase 11 T-1106) is NOT built (v1.0 0/122). The `auto_review_on_stage` toggle already exists in ReviewSettingsSchema (off by default); the actual stage-hook wiring re-lands when Phase 11 ships. --> 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.** <!-- done 2026-05-29: dismissals.ts DismissalStore — key binds file + FNV-1a hash of hunk content + normalized description, so a dismissal lapses once the hunk changes; JSON round-trip; tested (resurfaces on hunk change). --> 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.** <!-- skipped 2026-05-29: the only earlier deferral is T-CR-205 (duplicate detection), which is hard-gated on the Phase-8 embedder — still absent, so there is nothing un-blocked to pull up. Re-opens with Phase 8. --> Any T-CR-xxx deferred from earlier phases (e.g. T-CR-205 duplicate detection if Phase 8 was absent) lands here.

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.

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