Proactively suggest an independent second-harness review flow

[realtonyyoung]

Linear: AI-2175

What & why

Most vibe coders never run an independent code review — not with a second coding agent, nor outside the implementing session. Review flows already exist and the reviewer can be any installed harness independent of the driver, but the review-flows skill is strictly opt-in and never proposes itself, so beginners never discover it.

This adds a model-driven skill that proactively offers an independent review flow at the two natural milestones — right after a spec/design is finalized, and right after implementation is complete — and recommends a reviewer harness that will actually run for this repo.

Changes (purely additive — one new skill + one new tool)

• list_reviewer_vendors MCP tool in kcap mcp flows (agent-only, read-only). Reads GET /api/daemons and computes, client-side, the reviewer vendors that can run an unattended review for this repo now — the intersection of same-machine daemons hosting the repo × their unattended vendors. Every empty result is disambiguated by a single typed reason (repo_unresolved · schema_skew · lookup_failed · no_daemons_connected · no_repo_hosting_daemon · no_unattended_reviewer). Conservative model_override (AND across daemons). Tolerant daemon parsing (case-insensitive; malformed records skipped + counted; all-unparseable ⇒ schema_skew).
• suggest-review-flow skill — offers at spec-complete → spec-review and implementation-complete → code-review; prefers a reviewer ≠ the driver, falls back to a same-vendor independent flow, and maps an empty result to one-line unlock guidance. Never auto-runs — it offers, and hands off to review-flows only on explicit consent. Ships identically to all nine harnesses; the driver vendor is inferred at call time from harness env (not stamped, since six harnesses share ~/.agents/skills), echoed as driver_vendor, unknown ⇒ null ⇒ no "different model" claim.
• DriverVendor — env-marker inference (claude, codex today; ambiguous/unverified ⇒ null).
• Registered in AgentsSkillsInstaller.SourceNames + help-plugin.txt + README.md (all test-pinned); conformance pins on the triggers and safety guardrails.
• Docs: docs/eval/suggest-review-flow.md — the reproducible triggering-eval corpus + methodology.

No server change

Verified against kcap-server: GET /api/daemons already returns DaemonInfo with repoPaths + unattendedVendors + machineId + unattendedVendorCapabilities, so this is CLI-only.

Review

The design (AI-2175) was hardened over a Codex spec-review flow, 6 rounds to clean (repo-affinity, a circular eval gate, and a branch-name identity collision were all caught there). Deterministic unit/contract tests cover the tool's aggregation, every typed reason + precedence, partial-record handling, and identity edge cases; static conformance pins carry the skill's model-driven surface. AOT publish is clean.

No submodule pin bump is included (that's the maintainer's).

🤖 Generated with Claude Code

[linear-code]

AI-2175

[qodo-code-review]

PR Summary by Qodo

Proactively suggest a second-harness review flow with repo-aware reviewer lookup

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add a read-only MCP tool to list unattended reviewer vendors that can run for this repo.
• Ship a new skill that offers spec-review/code-review at natural milestones, never auto-running.
• Register the skill and pin behavior via conformance/unit tests and reproducible eval docs.

Diagram

graph TD
A(["Driver harness"]) --> B["suggest-review-flow"] --> C["list_reviewer_vendors"] --> D["McpFlowsServer"] --> E{{"GET /api/daemons"}} --> F["ReviewerVendorLookup"] --> D
B -- "user accepts" --> G["review-flows"] --> D --> H{{"POST /api/flows/review/start"}}
subgraph Legend
direction LR
_actor(["Harness"]) ~~~ _svc["CLI / Skill"] ~~~ _ext{{"Server API"}}
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fold suggestion into existing review-flows skill

• ➕ Fewer shipped skills to discover/maintain
• ➕ Single place to document review-flow UX
• ➖ review-flows is execution-oriented; mixing proactive triggering into it increases accidental auto-run risk
• ➖ Harder to keep “offer-only” semantics crisp and test-pinned separately

2. Server-side reviewer availability endpoint

• ➕ One canonical implementation shared by all clients
• ➕ Avoids duplicating parsing/aggregation logic across CLIs
• ➖ Requires server rollout/compat management (explicitly avoided in this PR)
• ➖ Less flexibility for client-side tolerance/heuristics without server changes

3. Heuristic recommendation without daemon lookup

• ➕ No new MCP tool needed
• ➕ Simpler implementation
• ➖ Would often recommend a reviewer that cannot run, causing user friction
• ➖ Cannot provide precise, typed unlock guidance for empty availability

Recommendation: Keep the PR’s approach: a separate, offer-only skill plus a read-only, repo-aware availability tool. This preserves safety boundaries (no auto-run), avoids any server dependency, and materially improves UX by recommending only reviewers that will actually run and by returning a single typed reason for empty results that the skill can map to actionable guidance.

Files changed (14) +773 / -10

Enhancement (5) +361 / -6

SKILL.mdShip the suggest-review-flow model-driven skill definition and guardrails +98/-0

Ship the suggest-review-flow model-driven skill definition and guardrails

• Adds the new skill with explicit milestone triggers (spec complete / implementation complete), reviewer recommendation rules based on list_reviewer_vendors output, and strict safety constraints (never auto-run; defer execution to review-flows on consent).

kcap/skills/suggest-review-flow/SKILL.md

DriverVendor.csInfer the driver harness vendor from verified environment markers +23/-0

Infer the driver harness vendor from verified environment markers

• Adds a best-effort driver vendor inference (claude vs codex) that returns null when ambiguous or unverified to avoid incorrect “different model” claims.

src/Capacitor.Cli/Commands/DriverVendor.cs

McpFlowsServer.csAdd list_reviewer_vendors tool and pass through inferred driver_vendor +44/-6

Add list_reviewer_vendors tool and pass through inferred driver_vendor

• Threads a driverVendor value into tool dispatch and implements a new read-only tool that calls GET /api/daemons, tolerantly parses daemon records, aggregates eligible unattended reviewers for the current repo/machine, and returns a source-generated JSON result DTO.

src/Capacitor.Cli/Commands/McpFlowsServer.cs

McpReviewServer.csRegister ReviewerVendorsResult for source-generated JSON serialization +1/-0

Register ReviewerVendorsResult for source-generated JSON serialization

• Adds ReviewerVendorsResult to McpJsonContext so list_reviewer_vendors responses remain AOT-safe and reflection-free.

src/Capacitor.Cli/Commands/McpReviewServer.cs

ReviewerVendorLookup.csImplement daemon parsing and repo-aware reviewer vendor aggregation +195/-0

Implement daemon parsing and repo-aware reviewer vendor aggregation

• Introduces a pure aggregation function with explicit empty-result reasons (with precedence), conservative model_override AND semantics across hosting daemons, machine filtering, and tolerant case-insensitive parsing that reports schema skew vs malformed-record skips.

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs

Tests (5) +324 / -1

AgentsSkillsInstallerTests.csPin installer SourceNames to include suggest-review-flow +1/-1

Pin installer SourceNames to include suggest-review-flow

• Updates the mirrored SourceNames list used by unit tests to match the installer’s registered skills.

test/Capacitor.Cli.Core.Tests.Unit/AgentsSkillsInstallerTests.cs

DriverVendorTests.csAdd unit tests for driver vendor inference and ambiguity handling +24/-0

Add unit tests for driver vendor inference and ambiguity handling

• Covers claude-only, codex-only, both-markers (nested) and no-marker cases to ensure inference stays conservative.

test/Capacitor.Cli.Tests.Unit/Commands/DriverVendorTests.cs

McpFlowsServerReviewerVendorsTests.csAdd tool-call tests for list_reviewer_vendors behavior and reasons +72/-0

Add tool-call tests for list_reviewer_vendors behavior and reasons

• Uses WireMock to validate successful reviewer listing, server error mapping to lookup_failed, and repo_unresolved behavior when repoRoot is missing, including driver_vendor echoing.

test/Capacitor.Cli.Tests.Unit/Commands/McpFlowsServerReviewerVendorsTests.cs

ReviewerVendorLookupTests.csAdd comprehensive unit tests for aggregation/parsing and reason precedence +184/-0

Add comprehensive unit tests for aggregation/parsing and reason precedence

• Tests repo/machine intersection behavior, deduping, supported-but-not-unattended reporting, conservative model_override AND, and tolerant ParseDaemons behavior including schema skew detection.

test/Capacitor.Cli.Tests.Unit/Commands/ReviewerVendorLookupTests.cs

SuggestReviewFlowSkillConformanceTests.csPin suggest-review-flow triggers and safety guardrails in shipped SKILL.md +43/-0

Pin suggest-review-flow triggers and safety guardrails in shipped SKILL.md

• Adds conformance tests ensuring the skill is registered, includes both milestone triggers, calls list_reviewer_vendors, defers to review-flows, avoids ordinary self-review prompting, and explicitly prohibits auto-running without consent.

test/Capacitor.Cli.Tests.Unit/Commands/SuggestReviewFlowSkillConformanceTests.cs

Documentation (3) +86 / -2

README.mdDocument the new suggest-review-flow skill in the shipped skill list +2/-1

Document the new suggest-review-flow skill in the shipped skill list

• Updates the install documentation to reflect ten shared-tree skills and adds the new kcap-suggest-review-flow entry to the skill table.

README.md

suggest-review-flow.mdAdd reproducible trigger-eval corpus and acceptance gate for suggest-review-flow +83/-0

Add reproducible trigger-eval corpus and acceptance gate for suggest-review-flow

• Introduces a documented two-layer gate (CI conformance pins + manual skill-creator eval) and a curated positive/negative corpus with fixed thresholds to validate the model-driven trigger surface.

docs/eval/suggest-review-flow.md

help-plugin.txtUpdate plugin help text to include suggest-review-flow install path +1/-1

Update plugin help text to include suggest-review-flow install path

• Adjusts the documented ~/.agents/skills/kcap-{...}/ set to include suggest-review-flow.

src/Capacitor.Cli.Core/Resources/help-plugin.txt

Other (1) +2 / -1

AgentsSkillsInstaller.csRegister suggest-review-flow as a shipped skill +2/-1

Register suggest-review-flow as a shipped skill

• Extends AgentsSkillsInstaller.SourceNames to include the new suggest-review-flow skill so it’s installed alongside existing skills.

src/Capacitor.Cli.Core/AgentsSkillsInstaller.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Local repo path leaked ✓ Resolved 🐞 Bug ⛨ Security

Description

list_reviewer_vendors returns repo.identity as repoRoot (an absolute local filesystem path), which
can leak sensitive local path information to the MCP client/model despite the MCP server explicitly
avoiding leaking local paths in other error paths.

Code

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[R66-68]

+        var resolved = !string.IsNullOrEmpty(repoRoot);
+        var repo     = new RepoIdent(repoRoot, resolved);
+

Evidence

ReviewerVendorLookup stores the local repoRoot string directly into the returned repo.identity,
and McpFlowsServer’s own comments indicate local path leakage to the client is something it tries to
avoid.

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[23-68]
src/Capacitor.Cli/Commands/McpFlowsServer.cs[71-86]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The tool result includes `repo.identity` set to `repoRoot`, which is an absolute local filesystem path. This can expose usernames and workstation layout to the connected harness/model.

### Issue Context
The MCP flows server already treats leaking local paths to the client as a concern (it logs unexpected exceptions to stderr specifically to avoid leaking paths to the client). Returning `repoRoot` in the normal successful response contradicts that stance.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[23-68]
- src/Capacitor.Cli/Commands/McpFlowsServer.cs[78-86]

### Suggested change
- Change `RepoIdent.Identity` to a non-sensitive identifier (e.g., `owner/repo` from `repoInfo`, a repo hash, or null) while keeping `resolved`.
- If the skill needs a stable dedup key, return a hash of the repo root (non-reversible) instead of the raw path.
- Update tests expecting `/repo/a` in the returned JSON accordingly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Tool writes machine.json ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new read-only list_reviewer_vendors tool calls MachineId.Get(), which can generate and persist
machine.json on first use, violating the tool’s “read-only/side-effect-free” contract and
potentially failing under restricted filesystem permissions.

Code

src/Capacitor.Cli/Commands/McpFlowsServer.cs[R399-402]

+                if (!daemonsResp.IsSuccessStatusCode) {
+                    result = ReviewerVendorLookup.Aggregate(null, repoRoot, MachineId.Get(), driverVendor);
+                } else {
+                    var daemonsBody = await daemonsResp.Content.ReadAsStringAsync();

Evidence

The tool handler calls MachineId.Get(), which can create/write machine.json on first use, so the
tool is not side-effect-free and can fail when it cannot write to the config directory.

src/Capacitor.Cli/Commands/McpFlowsServer.cs[387-406]
src/Capacitor.Cli.Core/MachineId.cs[21-59]
src/Capacitor.Cli.Core/MachineId.cs[75-79]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`list_reviewer_vendors` is documented/positioned as read-only and side-effect-free, but it calls `MachineId.Get()` which may create and write `machine.json` on first call. This introduces an unexpected side effect and can also throw under restricted config-directory permissions.

### Issue Context
- The tool branch in `McpFlowsServer.HandleToolCallAsync` invokes `MachineId.Get()` in both the non-2xx and 2xx paths.
- `MachineId.Get()` explicitly “generat[es] and persist[s] one on first call”, and persistence involves filesystem writes.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowsServer.cs[387-406]
- src/Capacitor.Cli.Core/MachineId.cs[21-79]

### Suggested change
- Prefer `MachineId.ReadPersisted()` for this tool; if it returns null, pass `requesterMachineId: null` (no machine filter) or fail closed with a clear diagnostics.reason (within the existing reason taxonomy).
- If you must keep `Get()`, update the tool description/comments/tests to acknowledge it can write (but that conflicts with the PR’s stated contract).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. JsonValueKind checks in ParseDaemons ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

ReviewerVendorLookup.ParseDaemons() uses direct JsonElement.ValueKind comparisons instead of the
repo-standard JsonElementExtensions helpers. This violates the JSON-handling standard and makes
JSON parsing logic less consistent/maintainable across the codebase.

Code

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[R137-139]

+            if (doc.RootElement.ValueKind != JsonValueKind.Array)
+                return ([], 0, true);
+

Evidence

PR Compliance ID 25 requires using JsonElementExtensions instead of direct JsonValueKind checks.
The added ParseDaemons implementation performs direct ValueKind comparisons (e.g., array/object
checks), which is exactly the failure condition described by the rule.

CLAUDE.md: Prefer JsonElementExtensions Over Direct JsonValueKind Checks
src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[137-139]
src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[148-149]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReviewerVendorLookup.ParseDaemons()` performs multiple direct `JsonElement.ValueKind` checks (e.g., `== JsonValueKind.Array`, `!= JsonValueKind.Object`). The repo compliance rule requires using `JsonElementExtensions` helpers instead of checking `ValueKind` directly.

## Issue Context
The repository already provides `JsonElementExtensions` (e.g., `IsArray`, `IsObject`, `IsString`, etc.) used elsewhere to standardize JSON parsing patterns.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[137-193]
- src/Capacitor.Cli.Core/JsonElementExtensions.cs[5-38]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Repo-path match too strict ✓ Resolved 🐞 Bug ☼ Reliability

Description

ReviewerVendorLookup matches daemon RepoPaths to repoRoot using only TrimEnd and ordinal string
equality, which is brittle across case differences and path separator/canonicalization differences
and can incorrectly produce no_repo_hosting_daemon.

Code

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[R78-80]

+        var hosting = daemons
+            .Where(d => d.RepoPaths.Any(p => Normalize(p) == norm) &&
+                        (requesterMachineId is null || d.MachineId == requesterMachineId))

Evidence

The hosting filter depends on exact string equality after only trimming trailing slashes, and does
not account for platform-specific path equivalence rules, making false mismatches likely.

src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[77-81]
src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[120-125]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Repo hosting detection uses `Normalize(p) == norm` where `Normalize` only trims trailing separators. This fails when equivalent paths differ by casing (common on Windows), separator style (`/` vs `\`), or other trivial canonicalization differences.

### Issue Context
Incorrect hosting detection changes the user-facing `diagnostics.reason` to `no_repo_hosting_daemon` and prevents recommending a reviewer even when one exists.

### Fix Focus Areas
- src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[77-81]
- src/Capacitor.Cli/Commands/ReviewerVendorLookup.cs[124-125]

### Suggested change
- Normalize via `Path.GetFullPath(...)` + `Path.TrimEndingDirectorySeparator(...)`.
- Compare with `StringComparison.OrdinalIgnoreCase` on Windows (or use an OS-appropriate comparer).
- Optionally normalize directory separators before comparing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗