fix: require structured kapi-agent revision comments (#125)

Require current-head structured revision evidence for stale or non-approving kapi-agent reviews, with tests and documentation for the PR re-review workflow.

Author: devkade

README.md

@@ -103,7 +103,7 @@ Issue #37 is intentionally split into reviewable child slices: executable CLI se
 
 ### Review CLI Harness
 
-`kapi-review github-pr` emits non-posting JSON for kapi-agent review/check automation and enforces local size, verification, stale-review revision explanation, structured approval-summary, and no-blocking-issues gates. GitHub merge enforcement for formal kapi-agent approval lives in `.github/workflows/kapi-agent-formal-approval-gate.yml`; require `require formal kapi-agent approval` plus `kapi-agent/review` in branch protection/rulesets.
+`kapi-review github-pr` emits non-posting JSON for kapi-agent review/check automation and enforces local size, verification, stale-review revision explanation, structured approval-summary, and no-blocking-issues gates. GitHub merge enforcement for formal kapi-agent approval lives in `.github/workflows/kapi-agent-formal-approval-gate.yml`; require `require formal kapi-agent approval` plus `kapi-agent/review` in branch protection/rulesets. Re-review requests after stale/non-approving kapi-agent reviews must put `@kapi-agent review`, the current head SHA, `What changed`, `Why this closes the prior feedback`, and `Verification` in the same author comment; see `docs/kapi-agent-approval-gate.md`.
 
 ### Agent Tools
 

docs/kapi-agent-approval-gate.md

@@ -42,6 +42,37 @@ Branch protection or rulesets for `dev` should require both:
 
 This makes the PR unmergeable in GitHub until the formal current-head review and bot check are both present. Local CLI/report logic may still explain the state, but it is not the enforcement boundary.
 
+## Revision comment rules
+
+After any stale or non-approving `kapi-agent` review, the author/supervisor must request re-review with a single same-comment trigger and revision explanation. Do not post `@kapi-agent review` separately from the explanation.
+
+Required template:
+
+```md
+@kapi-agent review
+
+Revision explanation for current head `<HEAD_SHA>`:
+
+What changed:
+- <actual code/docs/test change>
+
+Why this closes the prior feedback:
+- Prior blocking issue: <quote or summarize the blocker>
+- Resolution: <how this head addresses it>
+
+Verification:
+- `<command>` passed.
+```
+
+Rules:
+
+1. The comment must include `@kapi-agent review` and the `Revision explanation for current head` line in the same body.
+2. The head SHA in the comment must match the current PR head.
+3. `What changed`, `Why this closes the prior feedback`, and `Verification` are required headings and each must contain at least one bullet.
+4. New pushes require a fresh revision comment for the new head; old explanations become stale.
+5. The author comment must not mimic the bot review format (`## kapi-agent review` or `**Verdict:** APPROVE`). Verdict headers are reserved for formal kapi-agent review bodies.
+6. Verification must report executed evidence. Speculative wording such as `should pass` or `expected to work` does not satisfy the gate.
+
 ## Incident reference
 
 PR #110 was merged after an approval-shaped issue comment but before a formal kapi-agent review. The subsequent formal review requested changes. The corrected rule is: **comment verdicts are never merge approval**.

src/cli/kapi-review-cli.ts

@@ -15,13 +15,15 @@ interface ParsedArgs { repo: string; pr: number; headSha: string; changedLines:
 
 export function buildKapiReview(input: KapiReviewInput): KapiReviewResult {
   const previousReviewOnOlderHead = Boolean(input.priorReviewState && (!input.priorHeadSha || input.priorHeadSha !== input.headSha));
+  const priorReviewIsNonApproving = input.priorReviewState === "REQUEST_CHANGES" || input.priorReviewState === "COMMENT";
+  const revisionExplanationRequired = previousReviewOnOlderHead || priorReviewIsNonApproving;
   const supersedesPreviousApproval = input.priorReviewState === "APPROVE" && previousReviewOnOlderHead;
   const approvalBody = /^## kapi-agent review\s*\n\s*\n\*\*Verdict:\*\*\s*APPROVE(?:\s*\n)/i.test(input.reviewBody.trimStart());
   const commentBody = /^## kapi-agent review\s*\n\s*\n\*\*Verdict:\*\*\s*COMMENT(?:\s*\n)/i.test(input.reviewBody.trimStart());
   const gates = {
     size: input.changedLines < KAPI_REVIEW_MAX_CHANGED_LINES ? "pass" as const : "fail" as const,
     verify: input.verification,
-    revision_explanation: previousReviewOnOlderHead && !hasRevisionExplanation(input.revisionExplanation ?? "") ? "fail" as const : "pass" as const,
+    revision_explanation: revisionExplanationRequired && !hasRevisionExplanation(input.revisionExplanation ?? "", input.headSha) ? "fail" as const : "pass" as const,
     final_approval_summary: !approvalBody || hasFinalApprovalSummary(input.reviewBody) ? "pass" as const : "fail" as const,
   };
   const gatesPass = Object.values(gates).every((status) => status === "pass");
@@ -77,7 +79,30 @@ async function persistAttempt(root: string, input: ParsedArgs, result: KapiRevie
   ]);
 }
 
-function hasRevisionExplanation(text: string): boolean { return /what changed|changes?|changed|fix(?:ed)?|address(?:ed)?/i.test(text) && /why|because|prior|blocker|risk|reason/i.test(text) && /verification|verified|tests?|checks?|pass(?:ed)?/i.test(text); }
+function hasRevisionExplanation(text: string, headSha: string): boolean {
+  const body = text.trim();
+  if (!body) return false;
+  if (!/@kapi-agent\s+review/i.test(body)) return false;
+  if (new RegExp(`Revision explanation for current head\\s+\\\`${escapeRegExp(headSha)}\\\`\\s*:`, "i").test(body) === false) return false;
+  if (!/^What changed:\s*$/im.test(body) || !sectionHasBullet(body, "What changed")) return false;
+  if (!/^Why this closes the prior feedback:\s*$/im.test(body) || !sectionHasBullet(body, "Why this closes the prior feedback")) return false;
+  if (!/^Verification:\s*$/im.test(body) || !sectionHasBullet(body, "Verification")) return false;
+  if (/^##\s+kapi-agent review\s*$/im.test(body) || /^\*\*Verdict:\*\*\s*(?:APPROVE|REQUEST_CHANGES|COMMENT)\s*$/im.test(body)) return false;
+  if (/\b(?:should|expected to|probably|hopefully)\s+(?:pass|work|be green)\b/i.test(body)) return false;
+  return true;
+}
+function sectionHasBullet(text: string, heading: string): boolean {
+  const lines = text.split(/\r?\n/);
+  const start = lines.findIndex((line) => line.trim().toLowerCase() === `${heading.toLowerCase()}:`);
+  if (start === -1) return false;
+  const sectionLines = [];
+  for (const line of lines.slice(start + 1)) {
+    if (/^\S.*:\s*$/.test(line)) break;
+    sectionLines.push(line);
+  }
+  return sectionLines.some((line) => /^\s*-\s+\S/.test(line));
+}
+function escapeRegExp(value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); }
 function hasFinalApprovalSummary(text: string): boolean { const body = text.trimStart(); const final = body.match(/^## kapi-agent review\s*\n\s*\n\*\*Verdict:\*\*\s*APPROVE\s*\n\s*\n## Final approval summary\s*\n([\s\S]*?)\n### Blocking issues\s*\n([\s\S]*?)(?=\n### |\n## |$)/i); const summary = final?.[1] ?? "", issues = final?.[2]?.trim(); return Boolean(final) && /^### Review journey\s*$/im.test(summary) && !/^### Blocking issues\s*$/im.test(summary) && /^### What changed\s*$/im.test(summary) && /^### Why this is correct\s*$/im.test(summary) && /^### Evidence\s*$/im.test(summary) && /^### Remaining risks and approval rationale\s*$/im.test(summary) && /semantic scope/i.test(summary) && issues === "- none"; }
 async function nextAttemptNumber(base: string): Promise<number> { try { const dirs = await readdir(path.join(base, "attempts")); return Math.max(0, ...dirs.map((name) => Number(name)).filter(Number.isInteger)) + 1; } catch { return 1; } }
 function formatResult(result: KapiReviewResult): string { return `kapi-review ${result.context.repo}#${result.context.pr}: ${result.verdict}\ncheck: ${result.check_conclusion}\ngates: ${Object.entries(result.gates).map(([key, value]) => `${key}=${value}`).join(", ")}\n`; }

test/kapi-review-cli.test.ts

@@ -6,6 +6,19 @@ import { test } from "node:test";
 import { buildKapiReview, runKapiReviewCli } from "../src/cli/kapi-review-cli.js";
 const approvingBody = "## kapi-agent review\n\n**Verdict:** APPROVE\n\n## Final approval summary\n\n### Review journey\n- Reviewed the semantic scope and prior feedback.\n\n### What changed\n- Harness gates are covered.\n\n### Why this is correct\n- The JSON result follows policy.\n\n### Evidence\n- tests pass\n\n### Remaining risks and approval rationale\n- none\n\n### Blocking issues\n- none\n";
 const headSha = "0123456789abcdef0123456789abcdef01234567", priorHeadSha = "fedcba9876543210fedcba9876543210fedcba98";
+const validRevisionExplanation = `@kapi-agent review
+
+Revision explanation for current head \`${headSha}\`:
+
+What changed:
+- Tightened the revision gate.
+
+Why this closes the prior feedback:
+- Prior review asked for current-head comment evidence.
+
+Verification:
+- Tests passed.
+`;
 
 async function withBodyFile<T>(fn: (workspace: string, bodyFile: string) => Promise<T>): Promise<T> {
   const workspace = await mkdtemp(path.join(os.tmpdir(), "kapi-review-cli-"));
@@ -34,12 +47,39 @@ test("kapi-review maps gates to review verdict and check conclusion", () => {
   const comment = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 42, verification: "pass", reviewBody: "## kapi-agent review\n\n**Verdict:** COMMENT\n\n### Notes\n- non-blocking\n" }); assert.equal(comment.verdict, "COMMENT"); assert.equal(comment.check_conclusion, "neutral");
 });
 
-test("kapi-review refresh-check requires revision explanation after any stale review", () => {
+test("kapi-review refresh-check requires same-comment current-head revision explanation", () => {
   for (const prior of ["APPROVE", "COMMENT"] as const) {
-    const result = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: prior, priorHeadSha, refreshCheck: true });
-    assert.equal(result.verdict, "REQUEST_CHANGES");
-    assert.equal(result.check_conclusion, "failure");
-    assert.equal(result.gates.revision_explanation, "fail");
+    const missing = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: prior, priorHeadSha, refreshCheck: true });
+    assert.equal(missing.verdict, "REQUEST_CHANGES");
+    assert.equal(missing.check_conclusion, "failure");
+    assert.equal(missing.gates.revision_explanation, "fail");
+
+    const valid = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: prior, priorHeadSha, refreshCheck: true, revisionExplanation: validRevisionExplanation });
+    assert.equal(valid.verdict, "APPROVE");
+    assert.equal(valid.gates.revision_explanation, "pass");
+  }
+
+  for (const prior of ["REQUEST_CHANGES", "COMMENT"] as const) {
+    const missing = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: prior, priorHeadSha: headSha, refreshCheck: true });
+    assert.equal(missing.verdict, "REQUEST_CHANGES");
+    assert.equal(missing.check_conclusion, "failure");
+    assert.equal(missing.gates.revision_explanation, "fail");
+
+    const valid = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: prior, priorHeadSha: headSha, refreshCheck: true, revisionExplanation: validRevisionExplanation });
+    assert.equal(valid.verdict, "APPROVE");
+    assert.equal(valid.gates.revision_explanation, "pass");
+  }
+
+  for (const revisionExplanation of [
+    validRevisionExplanation.replace("@kapi-agent review\n\n", ""),
+    validRevisionExplanation.replace(headSha, priorHeadSha),
+    validRevisionExplanation.replace("Why this closes the prior feedback:", "Why:"),
+    validRevisionExplanation.replace("- Tests passed.", "Tests should pass."),
+    `@kapi-agent review\n\n## kapi-agent review\n\n**Verdict:** APPROVE\n\nRevision explanation for current head \`${headSha}\`:\n\nWhat changed:\n- x\n\nWhy this closes the prior feedback:\n- y\n\nVerification:\n- tests passed\n`,
+  ]) {
+    const invalid = buildKapiReview({ repo: "devkade/kapi", pr: 35, headSha, changedLines: 41, verification: "pass", reviewBody: approvingBody, priorReviewState: "REQUEST_CHANGES", priorHeadSha: headSha, refreshCheck: true, revisionExplanation });
+    assert.equal(invalid.verdict, "REQUEST_CHANGES");
+    assert.equal(invalid.gates.revision_explanation, "fail");
   }
 });
 
@@ -65,7 +105,7 @@ test("kapi-review JSON CLI enforces revision explanation and preserves attempts"
   await withBodyFile(async (workspace, bodyFile) => {
     const revisionFile = path.join(workspace, "revision.md");
     const artifactRoot = path.join(workspace, "artifacts");
-    await writeFile(revisionFile, "Changes: tightened gates.\nWhy: addresses the prior review risk.\nTests passed.\n", "utf8");
+    await writeFile(revisionFile, validRevisionExplanation, "utf8");
     const args = ["github-pr", "--repo", "devkade/kapi", "--pr", "35", "--head-sha", headSha, "--changed-lines", "41", "--verification", "pass", "--body-file", bodyFile, "--prior-review-state", "REQUEST_CHANGES", "--revision-explanation-file", revisionFile, "--artifact-root", artifactRoot, "--json"];
     const first = await runKapiReviewCli(args);
     const second = await runKapiReviewCli(args);