suggest-review-flow: steering delivery for non-skill harnesses + loadable description (#644)

Author: realtonyyoung

docs/eval/suggest-review-flow.md

@@ -29,15 +29,22 @@ There is no in-repo skill-triggering eval runner in kcap-cli or kcap-server, so
   returned, its independence claim matches `driver_vendor` known/unknown, and it **never starts a flow
   without explicit consent**.
 
-## Harness coverage
+## Harness coverage — two mechanisms
 
-The fixed supported set is the nine harnesses kcap ships the skill to: Claude Code, Codex, Cursor,
-Copilot, Gemini, Kiro, Pi, OpenCode, Antigravity. The `SKILL.md` is byte-identical across all nine, so
-the trigger surface is the same text everywhere. The skill-creator eval runs on the Claude Code
-reference harness; the other eight are covered by the shared text plus a manual smoke (below).
-**Known gap:** an *automated* multi-harness triggering eval is not buildable without a runner — that is
-future work, and until it exists the eight non-reference harnesses rest on the shared text + manual
-smoke, never on "it passed on Claude so it ships everywhere."
+The nine supported harnesses split by HOW they surface the proactive behaviour (see the corrected
+cross-harness sweep in Results):
+
+- **Skill-consulting harnesses — Claude, Codex, Cursor, Kiro:** consult a `SKILL.md` as an invocable
+  skill, so the `suggest-review-flow` skill triggers directly (confirmed 4/4).
+- **Steering harnesses — Copilot, Gemini, Pi, OpenCode, Antigravity:** do NOT consult a `SKILL.md`
+  proactively, but DO read kcap's always-in-context steering block, so the proactive-offer behaviour
+  is delivered to them via that block (`KcapAgentInstructions.Body`), not the skill. Validated on
+  Copilot (interactive): the plain skill did nothing; the steering nudge made it offer.
+
+There is no overlap — kcap installs steering for exactly those five and skills-only for the other
+four — so a harness gets the behaviour from one mechanism, never both (no double-offer). **Testing
+note:** interactive is the faithful method; a cold-prompt eval and headless one-shot both
+under-measure skill/steering consultation (see Results).
 
 ## Corpus
 
@@ -77,7 +84,68 @@ smoke, never on "it passed on Claude so it ships everywhere."
 - `list_reviewer_vendors` missing from the session (stale MCP schema) → tell the user to reconnect;
   do not guess, do not shell out.
 
-## Results
-
-_Pending the dev-time skill-creator run. Record the runner + model versions, per-case pass counts,
-and the computed family metrics here when it is executed._
+## Results (2026-08-21)
+
+### Cold-prompt triggering eval (`run_eval.py`, N=5) — NOT a valid gate for this skill
+
+Run against the frontmatter description via `claude -p`, 17 queries × 5 reps:
+
+| Family | Recall (positives) | FP rate (negatives) |
+|---|---|---|
+| spec | 0.00–0.10 | — |
+| code | 0.16–0.20 | — |
+| negatives | — | 0.00 |
+
+A markedly pushier description barely moved recall (0.0→0.1 spec, 0.16→0.20 code). That near-zero
+response is the finding: `claude -p` given a **cold completion statement** does not consult any skill,
+because a bare statement is not a task and harnesses only reach for skills on tasks they can't handle
+alone. So a cold-prompt triggering eval **structurally under-measures a proactive/agent-state skill**
+whose real trigger is the agent's own mid-session recognition. The clean 0.00 false-positive rate
+confirms the description's *guard* is sound; only the cold-fire measurement is invalid.
+
+### In-session test — the faithful method, 5/5 correct
+
+Five agents were run through realistic tasks with the skill surfaced as a harness would (description
+always-visible, body on-demand), reaching a genuine milestone, and their closing messages scored:
+
+| Case | Milestone | Result |
+|---|---|---|
+| P1 code — implementation complete | offered a **code-review** flow, different-vendor framing, availability-aware, consent-gated (no auto-run) | PASS |
+| P2 code — ready to commit | offered a **code-review** flow, asked before doing anything | PASS |
+| P3 spec — finalized | offered a **spec-review** flow (correct kind), asked first | PASS |
+| N1 — "review this yourself" | performed the review locally, no flow offer | PASS |
+| N2 — mid-implementation | continued the work, no flow offer | PASS |
+
+**Conclusion (Claude):** in the real trigger path the skill fires reliably and correctly, and the
+negative guard holds. The cold-prompt gate is retired for this skill in favour of the in-session
+method above.
+
+### Cross-harness sweep — CORRECTED (loadable description; interactive where headless is unfaithful)
+
+The first sweep had two confounds, both found via real testing and removed: (1) an **over-length
+description** (1386 > 1024 chars) that silently FAILED TO LOAD on strict harnesses (Copilot surfaced
+the load error), and (2) **headless one-shot mode does not surface skills like interactive does** on
+several harnesses (Cursor triggers interactively but not under `-p`). Re-tested with a 765-char
+loadable description, interactive where headless is unfaithful:
+
+| Harness | Triggers? | Evidence |
+|---|---|---|
+| claude | **YES** | 5/5 in-session |
+| codex | **YES** | headless `codex exec` — offered / reconnect-guidance |
+| kiro | **YES** | headless — read the SKILL.md, offered |
+| cursor | **YES** | interactive — proactive offer + list_reviewer_vendors attempt (headless `-p` did NOT) |
+| copilot | no | interactive AND headless — completed, never consulted the skill |
+| antigravity (agy) | no | interactive AND headless — never consulted the skill (exec-per-turn architecture) |
+| opencode | no | interactive — completed, did not offer |
+| gemini | untestable | free Gemini CLI tier deprecated (IneligibleTierError) — no working auth on this machine |
+| pi | untestable | CLI would not run (auth/env) |
+
+**Corrected finding:** the proactive skill triggers on **Claude, Codex, Kiro, Cursor** (4/9); does
+NOT trigger on **Copilot, Antigravity, OpenCode** (3/9 — those harnesses do not surface/consult a
+`SKILL.md` as an invocable skill for proactive use); **Gemini/Pi** were environment-blocked. The
+earlier "3/9" figure was an artifact of the over-length load bug plus the unfaithful headless mode.
+
+**Implication:** "ships to all 9 harnesses" is NOT achievable with the model-driven skill alone.
+Reaching Copilot/Antigravity/OpenCode (and likely Gemini/Pi) needs the deterministic Stop/SessionEnd
+hook the spec deferred — it fires regardless of skill consultation, but must de-duplicate against the
+skill on the 4 harnesses where the skill already triggers, to avoid double-offers.

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

@@ -1,19 +1,15 @@
 ---
 name: suggest-review-flow
 description: >-
-  Use this skill to PROACTIVELY OFFER an independent review flow at a natural
-  milestone — right after you finish writing or finalizing a spec/design, and
-  right after implementation is complete (a feature or bugfix is functionally
-  finished, or you are about to commit / open a PR / merge). It suggests handing
-  the work to a SEPARATE reviewer harness and, using list_reviewer_vendors,
-  recommends a reviewer that will actually run for this repo. Triggers:
-  "implementation is complete", "I've finished implementing", "the feature is
-  done", "ready to commit", "about to open a PR", "spec is finalized", "just
-  wrote the design". It only OFFERS and asks — it never starts a flow on its
-  own, and it hands execution to the review-flows skill once the user accepts.
-  Do NOT use it when the user asks you to review something yourself ("review
-  this", "code review this", "look over my spec") — just do that review
-  directly; and do not use it to START a flow (that is the review-flows skill).
+  Use this skill when implementation is complete — a feature or bugfix is functionally
+  finished, tests pass, or you are about to commit / open a PR / merge — or when a spec is
+  finalized. At that moment, proactively OFFER an independent second-harness review even if
+  the user did not ask, because a fresh, separate reviewer catches what the author's own model
+  misses. It calls list_reviewer_vendors to recommend a reviewer that will actually run for
+  this repo, offers a spec-review or code-review flow, and hands off to the review-flows skill
+  only after the user accepts — it never starts a flow itself. Do NOT use it when the user
+  asks you to review something yourself ("review this", "code review this", "look over my
+  spec"), or mid-task before the work is finished.
 ---
 
 # Suggest a review flow

src/Capacitor.Cli.Core/Instructions/KcapAgentInstructions.cs

@@ -41,5 +41,14 @@ expose them.
         the target. Do NOT run the review yourself or spawn your own reviewer for these requests.
         (When instead asked to review something *yourself* — "review this", "code review this" —
         review directly and do not call `kcap-flows`.)
+
+        When you **finish implementing a change** — a feature or bugfix is functionally complete,
+        tests pass, or you are about to commit / open a PR / merge — or you **finalize a spec or
+        design**, proactively OFFER an independent second-harness review before wrapping up, even if
+        the user didn't ask, because a fresh, separate reviewer catches what your own model misses.
+        Call `kcap-flows` `list_reviewer_vendors` to see which reviewers can run for this repo,
+        recommend one (prefer a vendor other than yourself), and call `kcap-flows` `start_review_flow`
+        ONLY after the user accepts — never auto-start it. Skip the offer when the user asked you to review
+        something yourself, or while you're mid-task and the work isn't finished.
         """;
 }

test/Capacitor.Cli.Core.Tests.Unit/Instructions/AgentInstructionsWriterTests.cs

@@ -3,6 +3,15 @@
 namespace Capacitor.Cli.Core.Tests.Unit.Instructions;
 
 public class AgentInstructionsWriterTests {
+    [Test]
+    public async Task Body_carries_the_proactive_review_offer_nudge() {
+        // The steering block is how the harnesses that do NOT consult a SKILL.md as an invocable
+        // skill (Copilot, Gemini, Pi, OpenCode, Antigravity) get the proactive review-offer
+        // behavior — dropping this paragraph silently removes the feature on those harnesses.
+        await Assert.That(KcapAgentInstructions.Body).Contains("proactively OFFER an independent second-harness review");
+        await Assert.That(KcapAgentInstructions.Body).Contains("list_reviewer_vendors");
+    }
+
     [Test]
     public async Task Write_creates_file_with_marked_block() {
         using var tmp = new TempDir();

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

@@ -14,11 +14,31 @@ static string SkillText() => File.ReadAllText(
     public async Task Registered_in_source_names()
         => await Assert.That(AgentsSkillsInstaller.SourceNames).Contains("suggest-review-flow");
 
+    [Test]
+    public async Task Description_stays_within_the_1024_char_skill_limit() {
+        // Strict harnesses (e.g. Copilot) reject a skill whose description exceeds 1024 chars and
+        // then SILENTLY fail to load it — so the skill never triggers there. Keep it under the cap.
+        var len = FoldedDescription().Length;
+        await Assert.That(len).IsGreaterThan(0);            // guard: a broken extraction must fail, not pass vacuously
+        await Assert.That(len).IsLessThanOrEqualTo(1024);
+    }
+
+    // The folded description — whitespace collapsed, as YAML unfolds a `>-` block and as the harness
+    // actually sees it. Raw-text substring checks are fragile to line wrapping (a wrapped phrase looks
+    // absent), so pin against the folded form.
+    static string FoldedDescription() {
+        // Normalize CRLF → LF first: a Windows checkout has `>-\r\n`, which the `\n`-anchored regex
+        // would otherwise miss, silently yielding an empty fold (green on mac/Linux, red on Windows).
+        var text = SkillText().Replace("\r\n", "\n").Replace("\r", "\n");
+        var m = System.Text.RegularExpressions.Regex.Match(text, @"(?m)^description: >-\n((?:^  .*\n)+)");
+        return System.Text.RegularExpressions.Regex.Replace(m.Groups[1].Value, @"\s+", " ").Trim();
+    }
+
     [Test]
     public async Task Description_carries_both_milestone_triggers() {
-        var text = SkillText();
-        await Assert.That(text).Contains("implementation is complete");
-        await Assert.That(text).Contains("spec is finalized");
+        var d = FoldedDescription();
+        await Assert.That(d).Contains("implementation is complete");
+        await Assert.That(d).Contains("spec is finalized");
     }
 
     [Test]