From ea58b6439811423dc2de743997bae05c103e5a28 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 18:41:05 +0000 Subject: [PATCH 01/11] Clarify review commit-readiness objective and blocker substantiation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend the shared review protocol to define the review objective as judging commit readiness, not finding something to comment on. Zero findings with APPROVED is a valid outcome when required evaluation and mandatory quality gates are complete. Key changes to _shared/review-protocol.md (v0.2.0 → v0.3.0): - Add Review Objective and Stopping Rule section: no minimum findings quota, mandatory gates still apply, optional suggestions nonblocking - Add Evidence and Commit-readiness impact fields to finding format - Add Blocker Substantiation section: concrete problem, code evidence accounting for existing safeguards, necessity before commit - Add Verdict Rules with commit-readiness check: APPROVED requires complete evaluation, all mandatory gates passing (not unrun/incomplete), and no substantiated blockers; CHANGES_REQUESTED for unmet gates or substantiated blockers - Strengthen Validation Rules: verify applicability against existing guards; downgrade unsubstantiated blockers only if a validated useful concern remains, otherwise discard Align code-review workflow consumers (v0.4.4 → v0.5.0): - templates/code-review.md: add Evidence, Commit-readiness impact, and Design category to canonical finding format - skills/start.md: commit-readiness check on all approval paths; prevent unattended auto-loop on purely nonblocking suggestions - skills/continue.md: commit-readiness check guards every verdict route including override logic; unmet/incomplete gates prevent APPROVED even with no code-level blockers; nonblocking suggestions alone cannot force another round Transitive cascade bumps (review-protocol → self-review-gate consumers): bugfix 0.9.5, cve-fix 0.5.1, e2e 0.8.5, implement 0.13.1, ui-implement 0.2.1 Assisted-by: AI assistant --- _shared/review-protocol.md | 105 ++++++++++++++++++++++++++- bugfix/SKILL.md | 2 +- code-review/SKILL.md | 2 +- code-review/skills/continue.md | 59 ++++++++++++--- code-review/skills/start.md | 26 ++++++- code-review/templates/code-review.md | 9 ++- cve-fix/SKILL.md | 2 +- e2e/SKILL.md | 2 +- implement/SKILL.md | 2 +- ui-implement/SKILL.md | 2 +- 10 files changed, 186 insertions(+), 25 deletions(-) diff --git a/_shared/review-protocol.md b/_shared/review-protocol.md index eecf3a7d..9f397985 100644 --- a/_shared/review-protocol.md +++ b/_shared/review-protocol.md @@ -1,6 +1,6 @@ --- name: review-protocol -version: 0.2.0 +version: 0.3.0 --- # Code Review Protocol @@ -8,6 +8,34 @@ Shared evaluation criteria, finding format, and assessment model used by all code review activities in this repository — both the standalone `code-review` workflow and the self-review gates embedded in other workflows. +## Review Objective and Stopping Rule + +The purpose of a code review is to judge whether the change is ready to +commit — not to find something to comment on. The review evaluates all +applicable criteria below and checks that mandatory quality gates +(lint, tests, required CI) have been satisfied. + +- **Zero actionable findings with APPROVED is a successful review.** When + the required evaluation and checks are complete and no substantiated + blockers remain, approving without findings is a valid and expected + outcome. Do not invent low-value comments merely to fill the findings + section. +- **No minimum findings quota.** There is no duty to supply feedback. + A clean change deserves a clean approval. +- **Mandatory gates still apply.** Do not issue an unqualified APPROVED + while required verification (lint, tests, security checks, or other + mandatory quality gates discovered from the project) is incomplete or + failing. An approval means the review is complete *and* the change + meets the project's commit-readiness bar. +- **Optional suggestions are welcome but nonblocking.** Clearly valuable + improvements (better naming, a small safety margin, a useful test case) + may be reported as findings with MEDIUM or LOW severity, but they + are explicitly nonblocking. They cannot alone require CHANGES_REQUESTED + or trigger another review round. If a human chooses to implement an + optional suggestion, that is their prerogative — do not prohibit it, + but do not force another review cycle solely because nonblocking + advice exists. + ## Core Principles - **Challenge decisions, not just implementation.** Don't just verify that @@ -87,7 +115,17 @@ Each finding must include: - **Severity:** CRITICAL | HIGH | MEDIUM | LOW - **Category:** One of the evaluation criteria above - **Issue:** What the problem is +- **Evidence:** What in the code, tests, or documented requirements + substantiates the claim — including whether existing safeguards + (error handling, validation, tests, type constraints) already address + the concern. For a blocker (CRITICAL/HIGH), cite the concrete problem + scenario or the specific requirement that is violated. For a + nonblocking suggestion (MEDIUM/LOW), state why the change would be + valuable but is not required before commit. - **Suggestion:** Concrete, actionable fix +- **Commit-readiness impact:** `blocking` for CRITICAL/HIGH findings + that must be resolved before commit, or `nonblocking` for MEDIUM/LOW + suggestions that improve quality but do not prevent approval. Findings that cannot cite a specific file and location in the actual diff must be discarded — they indicate hallucinated references. @@ -101,6 +139,62 @@ be discarded — they indicate hallucinated references. | MEDIUM | Naming improvements, minor edge cases, convention violations | Fix if it adds value | | LOW | Readability suggestions, minor refactoring opportunities | Fix only if clearly valuable | +## Blocker Substantiation + +A finding marked as a blocker (CRITICAL or HIGH) must meet all of these +criteria: + +1. **Concrete problem.** Identify a specific, triggerable failure + scenario or a specific violated requirement — not a hypothetical + that requires unsupported assumptions about how the code might be + used. +2. **Evidence from the code.** Cite the relevant code path, test gap, + or documented requirement. Account for existing safeguards — if + the code already handles the concern (error checks, type + constraints, upstream validation, tests), the finding is not a + blocker. +3. **Necessity before commit.** Explain why the problem must be fixed + before the change lands, not as a follow-up. A desirable + alternative implementation, a discretionary polish item, or an + unsupported hypothetical use case is insufficient to block. + +**Disposition of unsubstantiated blockers.** A CRITICAL/HIGH finding +that fails the criteria above is not automatically converted to a +suggestion. If the finding identifies a validated, concrete, clearly +useful concern that simply does not rise to blocking severity, +downgrade it to MEDIUM/LOW (nonblocking). If the finding is +unsupported, already handled by existing safeguards, or based on +hypothetical scenarios without evidence, discard it — do not preserve +a finding merely to populate the feedback section. Substantive +design, testing, and maintainability concerns remain eligible as +blockers when all three criteria are met. + +## Verdict Rules + +Before routing on any verdict, apply the **commit-readiness check**: +APPROVED is valid only when **all three conditions hold**: + +1. The required evaluation across all applicable criteria is complete. +2. All applicable mandatory quality gates (lint, tests, required CI, + security checks discovered from the project) are passing — not + merely unrun or incomplete. +3. No substantiated blockers (CRITICAL/HIGH meeting the Blocker + Substantiation criteria) remain. + +If any condition fails, the verdict is CHANGES_REQUESTED regardless +of the reviewer's label or the absence of code-level findings. An +unrun or incomplete mandatory gate is not "passing." + +- **CHANGES_REQUESTED** requires at least one substantiated blocker + (CRITICAL/HIGH meeting the criteria above) or at least one unmet, + failing, or incomplete applicable mandatory quality gate. +- **APPROVED** is correct only when the commit-readiness check above + is satisfied — regardless of whether nonblocking suggestions exist. + Zero findings is a valid approved state. +- When issuing APPROVED with nonblocking suggestions, list them in the + findings section. They inform the developer but do not prevent the + approval and do not require another review round. + ## Validation Rules After obtaining a review (whether from a subagent, external tool, or inline @@ -115,3 +209,12 @@ review), validate every finding: 4. **Read surrounding context.** The diff shows what changed; the surrounding code reveals whether the change fits. Read full files around changed sections to understand context. +5. **Verify applicability.** Confirm the finding applies to the actual + code path and is not already addressed by existing guards, checks, + type constraints, or tests. A finding about an error condition that + is already handled is not valid. +6. **Verify blocker criteria.** For any CRITICAL or HIGH finding, confirm + it meets the Blocker Substantiation requirements above. Downgrade to + MEDIUM/LOW only if a validated, concrete, clearly useful nonblocking + concern remains; discard if the finding is unsupported or already + handled. diff --git a/bugfix/SKILL.md b/bugfix/SKILL.md index 73605819..94cea0e9 100644 --- a/bugfix/SKILL.md +++ b/bugfix/SKILL.md @@ -1,6 +1,6 @@ --- name: bugfix -version: 0.9.4 +version: 0.9.5 description: >- Diagnostic and repair workflow that analyzes error logs, traces root causes, implements fixes, and verifies with regression tests. diff --git a/code-review/SKILL.md b/code-review/SKILL.md index a85f85eb..b5ea3290 100644 --- a/code-review/SKILL.md +++ b/code-review/SKILL.md @@ -1,6 +1,6 @@ --- name: code-review -version: 0.4.4 +version: 0.5.0 description: >- AI-driven code review workflow that reviews uncommitted changes using a discoverable reviewer profile, presents findings for human decision, and diff --git a/code-review/skills/continue.md b/code-review/skills/continue.md index 11a0a26d..d9589f48 100644 --- a/code-review/skills/continue.md +++ b/code-review/skills/continue.md @@ -315,7 +315,10 @@ The reviewer should evaluate all categories in - Re-check **rejected** findings only if that finding's file **hash changed this round**. Do not re-argue settled rejects on files that are still in `git diff HEAD` but were not modified this round -- Issue a verdict: APPROVED or CHANGES_REQUESTED +- Issue a verdict: APPROVED or CHANGES_REQUESTED, following the Verdict + Rules in `../../_shared/review-protocol.md` — CHANGES_REQUESTED + requires at least one substantiated blocker or unmet mandatory quality + gate; nonblocking suggestions alone do not justify CHANGES_REQUESTED Parent Reads `../templates/code-review.md` **once**. Instruct the subagent to write @@ -377,6 +380,12 @@ other feedback content regardless of section headings. **If APPROVED with no findings anywhere in the review:** +Before accepting, apply the commit-readiness check: confirm all +applicable mandatory quality gates are passing (not merely unrun or +incomplete) and the required evaluation is complete. If any gate is +unmet or incomplete, do not approve — report the gap and set the +verdict to CHANGES_REQUESTED. + Present the approval to the user: Before cleanup, read all response files (`review-response-{NNN}.md`) and @@ -432,16 +441,35 @@ In **unattended mode**, still present the approval and suggestions table — the user can interrupt at any time, and the table gives them the information to decide whether to do so. Do not skip the table because the mode is unattended; unattended means the user delegated decisions, -not visibility. Then: if any remaining -suggestions have an "Agree" assessment, persist the decisions to -`decisions-{NNN}.json` and loop back to Step 3 to implement them. If all -remaining suggestions are assessed as "Disagree", compile -the aggregated change summary (same as the "APPROVED with no findings" -path) before proceeding to Step 10 (the review is approved and all -valuable feedback has been addressed). +not visibility. Then apply the commit-readiness check: if any +applicable mandatory quality gate is unmet, failing, or incomplete, +the review is not complete regardless of the reviewer's verdict — report +the gap and continue the round. If any remaining suggestions are +substantiated blockers (CRITICAL/HIGH with `commit-readiness impact: +blocking`), persist the decisions and loop back to Step 3. If the +commit-readiness check passes and the remaining suggestions are all +nonblocking (MEDIUM/LOW), the review is approved — nonblocking +suggestions alone do not trigger another round. Compile the aggregated +change summary (same as the "APPROVED with no findings" path) before +proceeding to Step 10. **If CHANGES_REQUESTED:** +Before routing, apply the **commit-readiness check** from the Verdict +Rules in `../../_shared/review-protocol.md`: APPROVED is valid only when +the required evaluation is complete, all applicable mandatory quality +gates are passing (not merely unrun or incomplete), and no substantiated +blockers remain. If any condition fails, the verdict is +CHANGES_REQUESTED regardless of the reviewer's label or the absence of +code-level findings. + +If the reviewer issued CHANGES_REQUESTED but every finding is +nonblocking (MEDIUM/LOW) **and** the commit-readiness check passes, +override the verdict to APPROVED and follow the "APPROVED with +remaining suggestions" path above. If the commit-readiness check +fails (a mandatory gate is unmet, incomplete, or unrun), do **not** +override — CHANGES_REQUESTED stands even with no code-level blockers. + Assess the new findings independently (same as start.md's "Assess on value" section, Step 7b) — every finding on its merits, honest recommendation for each. @@ -482,10 +510,17 @@ unattended means the user delegated decisions, not visibility. Then: escalate to the user — present the finding and explain the disagreement. Wait for the user to decide (same guardrail as `/start`). -2. Otherwise, treat the implementor's recommendations as decisions, - except do not accept a finding whose `id` matches a prior reject. - Persist them to `decisions-{NNN}.json`, and loop back to Step 3 to - implement the next round. +2. Apply the commit-readiness check: if any applicable mandatory + quality gate is unmet, failing, or incomplete, the review is not + complete — report the gap and loop back to Step 3. If the + commit-readiness check passes and the only remaining findings are + nonblocking (MEDIUM/LOW), the review is complete — compile the + aggregated summary and proceed to Step 10 (cleanup). Nonblocking + suggestions alone do not trigger another round. +3. Otherwise, treat the implementor's recommendations as decisions + for blocking findings, except do not accept a finding whose `id` + matches a prior reject. Persist them to `decisions-{NNN}.json`, + and loop back to Step 3 to implement the next round. ### Step 10: Clean Up Artifacts diff --git a/code-review/skills/start.md b/code-review/skills/start.md index c784978b..aa76e635 100644 --- a/code-review/skills/start.md +++ b/code-review/skills/start.md @@ -350,8 +350,17 @@ that string, append `#{finding_number}`. } ``` -If the verdict is APPROVED with no findings, tell the user the review -passed and clean up artifacts automatically. +If the verdict is APPROVED with no findings, apply the +**commit-readiness check** from the Verdict Rules in +`../../_shared/review-protocol.md`: confirm all applicable mandatory +quality gates are passing (not merely unrun or incomplete) and the +required evaluation is complete. If any gate is unmet or incomplete, +do not approve — report the gap and set the verdict to +CHANGES_REQUESTED. Otherwise tell the user the review passed and +clean up artifacts automatically. Zero findings with APPROVED is a +valid, expected outcome when the commit-readiness check passes — do +not treat it as a review failure or search for additional comments +merely to fill the findings section. #### Unattended Mode Behavior @@ -365,8 +374,17 @@ After presenting: 1. If any CRITICAL finding has a "Disagree" assessment, stop and escalate to the user — present the finding and explain the disagreement. Wait for the user to decide. -2. Otherwise treat Agree / Partially agree as Accept and Disagree as - Reject, persist `decisions-{NNN}.json`, and proceed to `/continue`. +2. Apply the commit-readiness check: if any applicable mandatory + quality gate is unmet, failing, or incomplete, the review is not + complete — report the gap and proceed to `/continue`. If the + commit-readiness check passes and there are only nonblocking + findings (all MEDIUM/LOW with `commit-readiness impact: + nonblocking`), treat the review as complete — compile the + aggregated summary and proceed to cleanup. Nonblocking suggestions + alone do not trigger a `/continue` round. +3. Otherwise treat Agree / Partially agree as Accept and Disagree as + Reject for blocking findings, persist `decisions-{NNN}.json`, and + proceed to `/continue`. ## Output diff --git a/code-review/templates/code-review.md b/code-review/templates/code-review.md index 1abd98ef..0ef9d8dd 100644 --- a/code-review/templates/code-review.md +++ b/code-review/templates/code-review.md @@ -12,10 +12,15 @@ Repeat the following block for each finding, incrementing - **File:** {file path} - **Location:** {line range or function name, as applicable} - **Severity:** {CRITICAL | HIGH | MEDIUM | LOW} -- **Category:** {Correctness | Error Handling | Security | Performance | - Naming/Clarity | Test Coverage | Conventions} +- **Category:** {Correctness | Error Handling | Security | Design | + Performance | Naming/Clarity | Test Coverage | Conventions} - **Issue:** {what the problem is} +- **Evidence:** {what in the code, tests, or requirements substantiates + the claim — for blockers: the concrete problem scenario or violated + requirement, accounting for existing safeguards; for suggestions: why + the change is valuable but not required before commit} - **Suggestion:** {concrete, actionable fix} +- **Commit-readiness impact:** {blocking | nonblocking} ## Questions diff --git a/cve-fix/SKILL.md b/cve-fix/SKILL.md index 668e13d5..8edfb475 100644 --- a/cve-fix/SKILL.md +++ b/cve-fix/SKILL.md @@ -1,6 +1,6 @@ --- name: cve-fix -version: 0.6.0 +version: 0.6.1 description: >- Automated CVE remediation that reads vulnerability details from Jira vulnerability tickets, applies multi-strategy dependency fixes, validates diff --git a/e2e/SKILL.md b/e2e/SKILL.md index 2ba0a55c..6c23e2c3 100644 --- a/e2e/SKILL.md +++ b/e2e/SKILL.md @@ -1,6 +1,6 @@ --- name: e2e -version: 0.8.4 +version: 0.8.5 description: >- Story-to-e2e-test workflow that takes a Jira [QE] Story, discovers the project's e2e testing infrastructure, plans test scenarios, writes e2e diff --git a/implement/SKILL.md b/implement/SKILL.md index 77eee28b..a32b9837 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -1,6 +1,6 @@ --- name: implement -version: 0.13.0 +version: 0.13.1 description: >- Story-to-code workflow that takes a Jira Story, plans the implementation, writes contract-based tests and production code via TDD, validates against diff --git a/ui-implement/SKILL.md b/ui-implement/SKILL.md index 8202fc9c..7809e40d 100644 --- a/ui-implement/SKILL.md +++ b/ui-implement/SKILL.md @@ -1,6 +1,6 @@ --- name: ui-implement -version: 0.2.0 +version: 0.2.1 description: >- Story-to-code workflow for UI/front-end stories. Takes a Jira [UI] Story, plans the implementation using discovered design-system and testing From 9023285b9065c13f01cf72a05df84ab25c1f400a Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 19:56:46 +0000 Subject: [PATCH 02/11] Address review feedback: PASS contract, severity boundary, evaluation completeness, template categories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback fixes for PR #154: 1. Self-review gate PASS contract (self-review-gate.md v0.1.0→v0.1.1): - Scope PASS to criteria evaluation with no substantiated blockers, not unconditional commit/publish approval - Require discovered-tool path (Step 1) to cover all protocol criteria or complete remaining via Steps 2-5 before reporting PASS - Require boundary callers to ensure mandatory checks pass and validation is current for the final diff after gate fixes - Add revalidation requirement to bugfix/pr.md Step 6 when gate makes code fixes (cve-fix and implement callers already correct) 2. Severity table: align "before merge" → "before commit" in both CRITICAL and HIGH rows to match commit-readiness boundary 3. Evaluation completeness: add "or the required evaluation is not complete" to all three unattended local commit-readiness checks (start.md:378, continue.md:446, continue.md:515) 4. Template categories: align to protocol canonical names (Error handling, Naming and clarity, Test coverage, Project conventions) Assisted-by: AI assistant --- _shared/recipes/self-review-gate.md | 20 ++++++++++++++++++-- _shared/review-protocol.md | 4 ++-- bugfix/skills/pr.md | 7 +++++-- code-review/skills/continue.md | 10 ++++++---- code-review/skills/start.md | 7 ++++--- code-review/templates/code-review.md | 4 ++-- 6 files changed, 37 insertions(+), 15 deletions(-) diff --git a/_shared/recipes/self-review-gate.md b/_shared/recipes/self-review-gate.md index 7b8f356c..09aa2a42 100644 --- a/_shared/recipes/self-review-gate.md +++ b/_shared/recipes/self-review-gate.md @@ -1,6 +1,6 @@ --- name: self-review-gate -version: 0.1.0 +version: 0.1.1 --- # Recipe: Self-Review Gate @@ -48,7 +48,12 @@ workflows with decision points), skip it and continue to Step 2. 2. If the tool reports findings, assess each on value (using the criteria in `../review-protocol.md`) and fix issues that add real value 3. Re-run the tool to confirm fixes pass -4. Report results and return to the calling workflow +4. Report results using the Step 5 format below and return to the calling + workflow. The tool path must evaluate the same review-protocol criteria + as the subagent path — a tool that only checks a subset (e.g., lint + without correctness or design review) satisfies its subset, but the + gate must still perform Steps 2-5 for uncovered criteria before + reporting PASS. **If no suitable review tool is found**, continue to Step 2. @@ -171,6 +176,17 @@ Report a brief summary to the calling workflow: them. The calling workflow decides whether to proceed or stop.} ``` +**What PASS means.** PASS indicates that the gate's evaluation of the +reviewed diff against all review-protocol criteria found no remaining +substantiated blockers. It does not certify mandatory quality gate +status (lint, tests, CI) or full branch readiness — those are the +calling workflow's responsibility at the applicable commit or +publication boundary. Early-phase callers (e.g., per-task code review) +should not treat PASS as final branch approval; later-phase callers +at the commit/publish boundary must ensure applicable mandatory checks +pass and validation is current for the final diff including any +self-review fixes before proceeding. + The calling workflow is responsible for any git operations needed after the gate (staging, committing review fixes, etc.). The gate only makes code changes — it does not commit or push. diff --git a/_shared/review-protocol.md b/_shared/review-protocol.md index 9f397985..455ba902 100644 --- a/_shared/review-protocol.md +++ b/_shared/review-protocol.md @@ -134,8 +134,8 @@ be discarded — they indicate hallucinated references. | Severity | Meaning | Action | |----------|---------|--------| -| CRITICAL | Would cause bugs, security vulnerabilities, or data loss in production | Must fix before merge | -| HIGH | Missing error handling, race conditions, incomplete coverage that could cause failures | Should fix before merge | +| CRITICAL | Would cause bugs, security vulnerabilities, or data loss in production | Must fix before commit | +| HIGH | Missing error handling, race conditions, incomplete coverage that could cause failures | Should fix before commit | | MEDIUM | Naming improvements, minor edge cases, convention violations | Fix if it adds value | | LOW | Readability suggestions, minor refactoring opportunities | Fix only if clearly valuable | diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index 616a0a59..8e66a3f1 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -404,8 +404,11 @@ If the gate reports FLAG (unfixed CRITICAL or HIGH findings), stop and present the findings to the user. Do not proceed to commit until the user decides how to handle them. -If the gate reports PASS, proceed to Step 7. Any code fixes made by the -gate are uncommitted changes that will be included in the commit below. +If the gate reports PASS, proceed to Step 7. If the gate made code fixes, +re-run **Step 5** (validation gate) to confirm the fixes did not break +checks — do not commit until validation passes on the final diff. Any +code fixes are uncommitted changes that will be included in the commit +below. **Note:** The bugfix workflow also has an optional `/review` phase that provides a thorough, interactive review with findings and verdict. The diff --git a/code-review/skills/continue.md b/code-review/skills/continue.md index d9589f48..aafa6fd8 100644 --- a/code-review/skills/continue.md +++ b/code-review/skills/continue.md @@ -443,8 +443,9 @@ information to decide whether to do so. Do not skip the table because the mode is unattended; unattended means the user delegated decisions, not visibility. Then apply the commit-readiness check: if any applicable mandatory quality gate is unmet, failing, or incomplete, -the review is not complete regardless of the reviewer's verdict — report -the gap and continue the round. If any remaining suggestions are +or the required evaluation is not complete, the review is not done +regardless of the reviewer's verdict — report the gap and continue +the round. If any remaining suggestions are substantiated blockers (CRITICAL/HIGH with `commit-readiness impact: blocking`), persist the decisions and loop back to Step 3. If the commit-readiness check passes and the remaining suggestions are all @@ -511,8 +512,9 @@ unattended means the user delegated decisions, not visibility. Then: disagreement. Wait for the user to decide (same guardrail as `/start`). 2. Apply the commit-readiness check: if any applicable mandatory - quality gate is unmet, failing, or incomplete, the review is not - complete — report the gap and loop back to Step 3. If the + quality gate is unmet, failing, or incomplete, or the required + evaluation is not complete, the review is not done — report the + gap and loop back to Step 3. If the commit-readiness check passes and the only remaining findings are nonblocking (MEDIUM/LOW), the review is complete — compile the aggregated summary and proceed to Step 10 (cleanup). Nonblocking diff --git a/code-review/skills/start.md b/code-review/skills/start.md index aa76e635..ac4e02ed 100644 --- a/code-review/skills/start.md +++ b/code-review/skills/start.md @@ -375,9 +375,10 @@ After presenting: escalate to the user — present the finding and explain the disagreement. Wait for the user to decide. 2. Apply the commit-readiness check: if any applicable mandatory - quality gate is unmet, failing, or incomplete, the review is not - complete — report the gap and proceed to `/continue`. If the - commit-readiness check passes and there are only nonblocking + quality gate is unmet, failing, or incomplete, or the required + evaluation is not complete, the review is not done — report the + gap and proceed to `/continue`. If the commit-readiness check + passes and there are only nonblocking findings (all MEDIUM/LOW with `commit-readiness impact: nonblocking`), treat the review as complete — compile the aggregated summary and proceed to cleanup. Nonblocking suggestions diff --git a/code-review/templates/code-review.md b/code-review/templates/code-review.md index 0ef9d8dd..f8d9b23f 100644 --- a/code-review/templates/code-review.md +++ b/code-review/templates/code-review.md @@ -12,8 +12,8 @@ Repeat the following block for each finding, incrementing - **File:** {file path} - **Location:** {line range or function name, as applicable} - **Severity:** {CRITICAL | HIGH | MEDIUM | LOW} -- **Category:** {Correctness | Error Handling | Security | Design | - Performance | Naming/Clarity | Test Coverage | Conventions} +- **Category:** {Correctness | Error handling | Security | Design | + Performance | Naming and clarity | Test coverage | Project conventions} - **Issue:** {what the problem is} - **Evidence:** {what in the code, tests, or requirements substantiates the claim — for blockers: the concrete problem scenario or violated From 3f6e23c0cd4a6f3cee1e1aec2dafb686533097d5 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 20:21:13 +0000 Subject: [PATCH 03/11] Address second-round review feedback: severity, convergence, coverage, prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Severity table (protocol:138): HIGH action "Should fix" → "Must fix before commit" to match existing blocker semantics (CRITICAL/HIGH are blocking per verdict rules and commit-readiness impact field) 2. Review-validation convergence (bugfix/pr.md Step 6): final candidate must be both reviewed and validated with no intervening code changes. Track whether validation remediation changed candidate code; re-enter self-review consuming remaining MAX_ROUNDS budget; stop with FLAG if unable to converge. Candidate-code detection uses git diff on working-tree files, excludes artifact/log churn. 3. Criteria coverage record (protocol + template + start/continue): - Protocol: define Criteria Coverage section — evaluated, not applicable (justified), or incomplete for every criterion; completeness requires all applicable evaluated, unjustified N/A or incomplete cannot support APPROVED - Template: add Criteria Coverage table between Summary and Findings - start.md/continue.md producer: instruct reviewer to fill coverage table for every criterion - start.md/continue.md consumer (Step 7/7a): check coverage table before proceeding to verdict; do not invent omitted coverage 4. Prose consistency: "not done" → "not complete" in three unattended local branches (start.md:384, continue.md:455, continue.md:525) Assisted-by: AI assistant --- _shared/review-protocol.md | 30 +++++++++++++++++++++++- bugfix/skills/pr.md | 26 ++++++++++++++++++--- code-review/skills/continue.md | 35 +++++++++++++++++----------- code-review/skills/start.md | 18 ++++++++++---- code-review/templates/code-review.md | 13 +++++++++++ 5 files changed, 100 insertions(+), 22 deletions(-) diff --git a/_shared/review-protocol.md b/_shared/review-protocol.md index 455ba902..e61a5c6a 100644 --- a/_shared/review-protocol.md +++ b/_shared/review-protocol.md @@ -135,7 +135,7 @@ be discarded — they indicate hallucinated references. | Severity | Meaning | Action | |----------|---------|--------| | CRITICAL | Would cause bugs, security vulnerabilities, or data loss in production | Must fix before commit | -| HIGH | Missing error handling, race conditions, incomplete coverage that could cause failures | Should fix before commit | +| HIGH | Missing error handling, race conditions, incomplete coverage that could cause failures | Must fix before commit | | MEDIUM | Naming improvements, minor edge cases, convention violations | Fix if it adds value | | LOW | Readability suggestions, minor refactoring opportunities | Fix only if clearly valuable | @@ -195,6 +195,34 @@ unrun or incomplete mandatory gate is not "passing." findings section. They inform the developer but do not prevent the approval and do not require another review round. +## Criteria Coverage + +A review report must include a **Criteria Coverage** record listing every +evaluation criterion from this protocol with its coverage status. This +is review metadata — a record of what was evaluated — not mandatory +findings or feedback. Zero findings with all criteria covered is a +valid APPROVED outcome. + +Each row states one of: + +- **evaluated** — the criterion was assessed against the changes in + scope. Briefly note what was checked (e.g., "no new error paths + introduced", "naming consistent with existing module conventions"). +- **not applicable** — the criterion does not apply to this change, + with a brief justification (e.g., "no performance-sensitive paths + changed"). Applicability must itself be assessed; an unjustified + "N/A" is incomplete coverage. +- **incomplete** — the criterion could not be fully evaluated (e.g., + "test coverage not assessed — no test runner available"). State what + is missing. + +**Completeness** means every applicable criterion is `evaluated` and +every non-applicable criterion has a justified `not applicable`. +Any `incomplete` row or unjustified `not applicable` cannot support +an APPROVED verdict, even with zero findings and passing quality +gates. A coverage record is a report of review activity, not proof +that all defects have been found. + ## Validation Rules After obtaining a review (whether from a subagent, external tool, or inline diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index 8e66a3f1..cfac6076 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -406,9 +406,29 @@ decides how to handle them. If the gate reports PASS, proceed to Step 7. If the gate made code fixes, re-run **Step 5** (validation gate) to confirm the fixes did not break -checks — do not commit until validation passes on the final diff. Any -code fixes are uncommitted changes that will be included in the commit -below. +checks — do not commit until validation passes on the final diff. + +**Review-validation convergence.** The final candidate committed in +Step 7 must be both reviewed (self-review PASS) and validated (Step 5 +PASS) with no intervening code changes. Track convergence as follows: + +- After re-running Step 5, check whether validation remediation changed + candidate code (source files, configs, or relevant new files — not + artifact logs or `.artifacts/` churn). Use `git diff` on working-tree + files, not only HEAD SHA, since changes are uncommitted. +- If validation passed without changing candidate code, the candidate + is both reviewed and validated. Proceed to Step 7. +- If validation remediation changed candidate code, the new candidate + has not been self-reviewed. Re-enter the self-review gate, consuming + remaining rounds from the **original** MAX_ROUNDS budget (do not + reset to 3 on re-entry). If the gate makes further fixes, re-run + Step 5 again, continuing to consume the same budget. +- If MAX_ROUNDS is exhausted before both gates pass on the same + unchanged candidate, stop and present the situation to the user as + a FLAG — do not commit an unreviewed or unvalidated candidate. + +Any code fixes are uncommitted changes that will be included in the +commit below. **Note:** The bugfix workflow also has an optional `/review` phase that provides a thorough, interactive review with findings and verdict. The diff --git a/code-review/skills/continue.md b/code-review/skills/continue.md index aafa6fd8..51481727 100644 --- a/code-review/skills/continue.md +++ b/code-review/skills/continue.md @@ -308,7 +308,8 @@ Store any new agent ID in the metadata. Evaluate the code as if you did not write it. The reviewer should evaluate all categories in -`../../_shared/review-protocol.md` and additionally: +`../../_shared/review-protocol.md`, fill in the **Criteria Coverage** +table in the report template for every criterion, and additionally: - Verify that accepted findings were addressed correctly - Check whether the changes introduced new issues @@ -327,16 +328,24 @@ number) using that skeleton. The subagent writes that file. The parent writes it **only** when reviewing sequentially (no subagent). Exactly one writer. Do not overwrite the subagent file. -#### 7a: Validate finding references - -For each finding, confirm that the cited file and location actually exist -in the change. AI reviewers hallucinate file paths and line numbers. If a -finding references a file that was not changed (not on the name-status / -untracked list), discard it — a context Read of a neighboring file is -not enough. If the location does not exist, or does not overlap the -changed lines (tracked) or the Read chunks (untracked), discard it. Do -not present discarded findings to the user. Note discarded findings and -why in your internal assessment (not in the user-facing table). +#### 7a: Validate criteria coverage and finding references + +First check the **Criteria Coverage** table in the review report: +every criterion must be `evaluated` or `not applicable` with a +justification. If any row is `incomplete` or missing, the review is +not complete — do not proceed to a verdict. Ask the reviewer (or +re-review inline) to cover the gap. Do not invent coverage the +reviewer omitted. + +Then for each finding, confirm that the cited file and location actually +exist in the change. AI reviewers hallucinate file paths and line +numbers. If a finding references a file that was not changed (not on +the name-status / untracked list), discard it — a context Read of a +neighboring file is not enough. If the location does not exist, or +does not overlap the changed lines (tracked) or the Read chunks +(untracked), discard it. Do not present discarded findings to the +user. Note discarded findings and why in your internal assessment +(not in the user-facing table). ### Step 8: Update Metadata @@ -443,7 +452,7 @@ information to decide whether to do so. Do not skip the table because the mode is unattended; unattended means the user delegated decisions, not visibility. Then apply the commit-readiness check: if any applicable mandatory quality gate is unmet, failing, or incomplete, -or the required evaluation is not complete, the review is not done +or the required evaluation is not complete, the review is not complete regardless of the reviewer's verdict — report the gap and continue the round. If any remaining suggestions are substantiated blockers (CRITICAL/HIGH with `commit-readiness impact: @@ -513,7 +522,7 @@ unattended means the user delegated decisions, not visibility. Then: `/start`). 2. Apply the commit-readiness check: if any applicable mandatory quality gate is unmet, failing, or incomplete, or the required - evaluation is not complete, the review is not done — report the + evaluation is not complete, the review is not complete — report the gap and loop back to Step 3. If the commit-readiness check passes and the only remaining findings are nonblocking (MEDIUM/LOW), the review is complete — compile the diff --git a/code-review/skills/start.md b/code-review/skills/start.md index ac4e02ed..d7c45104 100644 --- a/code-review/skills/start.md +++ b/code-review/skills/start.md @@ -231,9 +231,12 @@ skeleton, then return. **If subagents are not available:** Read `../../_shared/review-protocol.md` (not `../guidelines.md`) and review sequentially. Same hunk-Read rules. -Evaluate all categories in `../../_shared/review-protocol.md`. If the user -provided focus guidance, prioritize those areas but still report CRITICAL -and HIGH findings elsewhere. +Evaluate all categories in `../../_shared/review-protocol.md` and fill in +the **Criteria Coverage** table in the report template for every +criterion — `evaluated`, `not applicable` (with justification), or +`incomplete` (with what is missing). If the user provided focus guidance, +prioritize those areas but still report CRITICAL and HIGH findings +elsewhere and still cover every criterion in the table. The subagent writes `code-review-001.md`. The parent writes it **only** when reviewing sequentially (no subagent). Exactly one writer. Do not @@ -241,7 +244,12 @@ overwrite the subagent file. ### Step 7: Validate and Assess Findings -Read the review file and work through **every** finding. +Read the review file. First check the **Criteria Coverage** table: +every criterion must be `evaluated` or `not applicable` with a +justification. If any row is `incomplete` or missing, the review is +not complete — do not proceed to a verdict. Ask the reviewer (or +re-review inline) to cover the gap. Do not invent coverage the +reviewer omitted. Then work through **every** finding. #### 7a: Validate finding references @@ -376,7 +384,7 @@ After presenting: disagreement. Wait for the user to decide. 2. Apply the commit-readiness check: if any applicable mandatory quality gate is unmet, failing, or incomplete, or the required - evaluation is not complete, the review is not done — report the + evaluation is not complete, the review is not complete — report the gap and proceed to `/continue`. If the commit-readiness check passes and there are only nonblocking findings (all MEDIUM/LOW with `commit-readiness impact: diff --git a/code-review/templates/code-review.md b/code-review/templates/code-review.md index f8d9b23f..e8b2e530 100644 --- a/code-review/templates/code-review.md +++ b/code-review/templates/code-review.md @@ -3,6 +3,19 @@ ## Summary {2-3 sentence overall assessment} +## Criteria Coverage + +| Criterion | Status | Scope / Evidence | +|-----------|--------|------------------| +| Correctness | {evaluated | not applicable | incomplete} | {brief note} | +| Error handling | {evaluated | not applicable | incomplete} | {brief note} | +| Security | {evaluated | not applicable | incomplete} | {brief note} | +| Design | {evaluated | not applicable | incomplete} | {brief note} | +| Performance | {evaluated | not applicable | incomplete} | {brief note} | +| Naming and clarity | {evaluated | not applicable | incomplete} | {brief note} | +| Test coverage | {evaluated | not applicable | incomplete} | {brief note} | +| Project conventions | {evaluated | not applicable | incomplete} | {brief note} | + ## Findings Repeat the following block for each finding, incrementing From 28023976b3704b4d2d937c22348566f5a422bc1d Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 20:25:45 +0000 Subject: [PATCH 04/11] Tighten convergence snapshot, round budget, and coverage scope Address coordinator final-review concerns A/B/C: A. bugfix/pr.md convergence: record explicit reviewed-candidate snapshot (tracked staged+unstaged AND relevant untracked files, excluding .artifacts/ churn) before revalidation. Compare post-validation candidate against snapshot to detect actual code changes including new untracked files. Re-review only on genuine candidate content change, not on unchanged successful validation. B. bugfix/pr.md round budget: initialize convergence budget=3 once, pass remaining budget as MAX_ROUNDS on each re-entry, consume actual reported rounds. A candidate that consumed the last round is valid if both gates passed on it; FLAG only when re-review is needed with no budget remaining. C. review-protocol.md Criteria Coverage: scope the coverage-record requirement to structured formal review reports with a verdict (APPROVED/CHANGES_REQUESTED), not the self-review gate's compact PASS/FLAG summary which uses a different output format. Assisted-by: AI assistant --- _shared/review-protocol.md | 17 +++++++--- bugfix/skills/pr.md | 63 ++++++++++++++++++++++++++++---------- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/_shared/review-protocol.md b/_shared/review-protocol.md index e61a5c6a..16f1b464 100644 --- a/_shared/review-protocol.md +++ b/_shared/review-protocol.md @@ -197,11 +197,18 @@ unrun or incomplete mandatory gate is not "passing." ## Criteria Coverage -A review report must include a **Criteria Coverage** record listing every -evaluation criterion from this protocol with its coverage status. This -is review metadata — a record of what was evaluated — not mandatory -findings or feedback. Zero findings with all criteria covered is a -valid APPROVED outcome. +Structured review reports that issue an APPROVED or CHANGES_REQUESTED +verdict — such as the standalone `code-review` workflow's report +template — must include a **Criteria Coverage** record listing every +evaluation criterion from this protocol with its coverage status. +This requirement applies to formal review reports with a verdict, not +to the self-review gate's compact PASS/FLAG summary (which is +criteria-scoped but uses a different output format without a coverage +table). + +The coverage record is review metadata — a record of what was +evaluated — not mandatory findings or feedback. Zero findings with +all criteria covered is a valid APPROVED outcome. Each row states one of: diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index cfac6076..f4aad131 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -410,22 +410,53 @@ checks — do not commit until validation passes on the final diff. **Review-validation convergence.** The final candidate committed in Step 7 must be both reviewed (self-review PASS) and validated (Step 5 -PASS) with no intervening code changes. Track convergence as follows: - -- After re-running Step 5, check whether validation remediation changed - candidate code (source files, configs, or relevant new files — not - artifact logs or `.artifacts/` churn). Use `git diff` on working-tree - files, not only HEAD SHA, since changes are uncommitted. -- If validation passed without changing candidate code, the candidate - is both reviewed and validated. Proceed to Step 7. -- If validation remediation changed candidate code, the new candidate - has not been self-reviewed. Re-enter the self-review gate, consuming - remaining rounds from the **original** MAX_ROUNDS budget (do not - reset to 3 on re-entry). If the gate makes further fixes, re-run - Step 5 again, continuing to consume the same budget. -- If MAX_ROUNDS is exhausted before both gates pass on the same - unchanged candidate, stop and present the situation to the user as - a FLAG — do not commit an unreviewed or unvalidated candidate. +PASS) with no intervening candidate-code changes. + +Initialize a convergence budget of **3 rounds** (matching the gate's +MAX_ROUNDS). Each self-review gate invocation passes the remaining +budget as MAX_ROUNDS; the budget decreases by the number of rounds +the gate actually consumed (from its reported `Rounds: {N}`). + +After the self-review gate reports PASS, record the reviewed candidate +snapshot before re-running validation: + +```bash +# Tracked changes (staged + unstaged, includes deletions/renames) +git diff HEAD --name-only +git diff --cached --name-only +# Relevant untracked/new files (exclude artifacts and logs) +git ls-files --others --exclude-standard | grep -v '^\.artifacts/' +``` + +Record this combined path list as the **reviewed file set**. Then +re-run **Step 5** (validation gate). + +After Step 5 completes, capture the same three commands again as the +**post-validation file set** and diff the two lists plus file content: + +```bash +# Content comparison for tracked candidate files +git diff HEAD -- {reviewed file set paths} +# Check for new untracked candidate files not in the reviewed set +git ls-files --others --exclude-standard | grep -v '^\.artifacts/' +``` + +- If the post-validation candidate is identical to the reviewed + snapshot (no content changes in tracked files, no new relevant + untracked files), the candidate is both reviewed and validated. + Proceed to Step 7. +- If validation remediation changed candidate code (modified, added, + or deleted source files, configs, or relevant new files — not + `.artifacts/` output or log churn), the new candidate has not been + self-reviewed. Re-enter the self-review gate with the remaining + convergence budget as MAX_ROUNDS. If the gate makes further fixes, + record the new reviewed snapshot and re-run Step 5 again. +- A reviewed-and-validated candidate that consumed the last available + round is still valid — FLAG is needed only when another re-review + is required but no budget remains. If convergence budget reaches + zero before both gates pass on the same unchanged candidate, stop + and present the situation to the user as a FLAG — do not commit an + unreviewed or unvalidated candidate. Any code fixes are uncommitted changes that will be included in the commit below. From e42858566da0b7fb61f9695e236ddb03012c0bcf Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 20:32:38 +0000 Subject: [PATCH 05/11] Fix convergence snapshot and budget in bugfix/pr.md Step 6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace path-list-based candidate comparison with content-digest snapshot (tracked staged+unstaged + relevant untracked files, each with path/status/content-hash). Detects content changes to existing untracked files, new files, deletions, and renames — not just path list differences. Excludes only .artifacts/ outputs. Move budget initialization before first gate invocation. Pass review_rounds_remaining as MAX_ROUNDS in the parameter block (not hardcoded 3). Deduct actual reported Rounds:{N} after each gate return. Allow last-round PASS when candidate is converged; FLAG only when re-review is needed with zero remaining. State machine: snapshot after Step 5 as validated_candidate, snapshot after gate PASS as reviewed_candidate, compare digests for equality, re-enter review/validation only on actual content divergence. Assisted-by: AI assistant --- bugfix/skills/pr.md | 109 ++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 55 deletions(-) diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index f4aad131..46899324 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -391,72 +391,71 @@ Do not proceed to **Self-Review Gate** or commit. Before committing, run a self-review of the changes to catch issues before they reach external reviewers. -Read and follow `../../_shared/recipes/self-review-gate.md` with these -parameters: +**Candidate snapshot definition.** A candidate snapshot is a saved +immutable mapping of every candidate file's path, status (present, +deleted, renamed), and content digest. It covers: -| Parameter | Value | -|-----------|-------| -| DIFF_COMMAND | `git diff HEAD` | -| MAX_ROUNDS | `3` | -| CONTEXT_FILES | `.artifacts/bugfix/{issue}/root-cause.md`, `.artifacts/bugfix/{issue}/implementation-notes.md` (if they exist) | +- tracked staged edits (`git diff --cached`) +- tracked unstaged edits (`git diff`) +- all relevant untracked files (`git ls-files --others --exclude-standard`) -If the gate reports FLAG (unfixed CRITICAL or HIGH findings), stop and -present the findings to the user. Do not proceed to commit until the user -decides how to handle them. +Exclude only `.artifacts/` outputs and build logs — never exclude a +candidate source file based on extension or unchanged path presence. +Compute content digests with `git hash-object` for tracked files and +`sha256sum` (or equivalent) for untracked files. Do not stage, commit, +or destructively modify the working tree to build the snapshot. -If the gate reports PASS, proceed to Step 7. If the gate made code fixes, -re-run **Step 5** (validation gate) to confirm the fixes did not break -checks — do not commit until validation passes on the final diff. +Two snapshots are **equal** when they have the same set of paths with +the same status and content digest for each. A change to any candidate +file's content, a newly appearing relevant untracked file, or a +deletion/rename invalidates equality — even if the path list is +unchanged. -**Review-validation convergence.** The final candidate committed in -Step 7 must be both reviewed (self-review PASS) and validated (Step 5 -PASS) with no intervening candidate-code changes. +**Convergence state.** Initialize once for this Step 6: -Initialize a convergence budget of **3 rounds** (matching the gate's -MAX_ROUNDS). Each self-review gate invocation passes the remaining -budget as MAX_ROUNDS; the budget decreases by the number of rounds -the gate actually consumed (from its reported `Rounds: {N}`). +```text +review_rounds_remaining = 3 +validated_candidate = snapshot_candidate() # from Step 5's final state +reviewed_candidate = null +``` -After the self-review gate reports PASS, record the reviewed candidate -snapshot before re-running validation: +**Review loop.** -```bash -# Tracked changes (staged + unstaged, includes deletions/renames) -git diff HEAD --name-only -git diff --cached --name-only -# Relevant untracked/new files (exclude artifacts and logs) -git ls-files --others --exclude-standard | grep -v '^\.artifacts/' -``` +1. Invoke the self-review gate: -Record this combined path list as the **reviewed file set**. Then -re-run **Step 5** (validation gate). + Read and follow `../../_shared/recipes/self-review-gate.md` with: -After Step 5 completes, capture the same three commands again as the -**post-validation file set** and diff the two lists plus file content: + | Parameter | Value | + |-----------|-------| + | DIFF_COMMAND | `git diff HEAD` | + | MAX_ROUNDS | `{review_rounds_remaining}` | + | CONTEXT_FILES | `.artifacts/bugfix/{issue}/root-cause.md`, `.artifacts/bugfix/{issue}/implementation-notes.md` (if they exist) | -```bash -# Content comparison for tracked candidate files -git diff HEAD -- {reviewed file set paths} -# Check for new untracked candidate files not in the reviewed set -git ls-files --others --exclude-standard | grep -v '^\.artifacts/' -``` + Deduct the gate's reported `Rounds: {N}` from + `review_rounds_remaining` immediately after return. + +2. If FLAG: stop and present findings to the user. Do not proceed to + commit until the user decides how to handle them. + +3. On PASS: `reviewed_candidate = snapshot_candidate()`. + +4. If `reviewed_candidate == validated_candidate`: the final candidate + is both reviewed and validated. Proceed to Step 7. + +5. The gate made code fixes, so the reviewed candidate differs from + the validated one. Re-run **Step 5** (validation gate) on the + current working tree. After Step 5 passes, update: + `validated_candidate = snapshot_candidate()`. + +6. If `validated_candidate == reviewed_candidate`: validation did not + change the candidate further — both checks cover the same content. + Proceed to Step 7. -- If the post-validation candidate is identical to the reviewed - snapshot (no content changes in tracked files, no new relevant - untracked files), the candidate is both reviewed and validated. - Proceed to Step 7. -- If validation remediation changed candidate code (modified, added, - or deleted source files, configs, or relevant new files — not - `.artifacts/` output or log churn), the new candidate has not been - self-reviewed. Re-enter the self-review gate with the remaining - convergence budget as MAX_ROUNDS. If the gate makes further fixes, - record the new reviewed snapshot and re-run Step 5 again. -- A reviewed-and-validated candidate that consumed the last available - round is still valid — FLAG is needed only when another re-review - is required but no budget remains. If convergence budget reaches - zero before both gates pass on the same unchanged candidate, stop - and present the situation to the user as a FLAG — do not commit an - unreviewed or unvalidated candidate. +7. Validation remediation produced a different candidate. If + `review_rounds_remaining > 0`, go to step 1 (re-enter review with + remaining budget). If `review_rounds_remaining == 0`, stop and + present the situation to the user as a FLAG — do not commit an + unreviewed or unvalidated candidate. Any code fixes are uncommitted changes that will be included in the commit below. From edd7ebe5e87dee6640637926560070ec335f80b9 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 20:47:12 +0000 Subject: [PATCH 06/11] Clarify review-loop target and template impact mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. bugfix/pr.md Step 6 item 7: make re-entry target explicit as "Review loop, item 1" rather than ambiguous "step 1"; clarify that initialization block and budget are not re-run/reset. 2. code-review template: add compact severity-to-impact mapping hint (CRITICAL/HIGH substantiated → blocking; MEDIUM/LOW → nonblocking) matching the protocol's existing definition. Assisted-by: AI assistant --- bugfix/skills/pr.md | 5 +++-- code-review/templates/code-review.md | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index 46899324..704fee8c 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -452,8 +452,9 @@ reviewed_candidate = null Proceed to Step 7. 7. Validation remediation produced a different candidate. If - `review_rounds_remaining > 0`, go to step 1 (re-enter review with - remaining budget). If `review_rounds_remaining == 0`, stop and + `review_rounds_remaining > 0`, return to **Review loop, item 1** + above (re-enter the self-review gate with remaining budget — do + not re-run Step 6 initialization or reset the budget). If `review_rounds_remaining == 0`, stop and present the situation to the user as a FLAG — do not commit an unreviewed or unvalidated candidate. diff --git a/code-review/templates/code-review.md b/code-review/templates/code-review.md index e8b2e530..f6338496 100644 --- a/code-review/templates/code-review.md +++ b/code-review/templates/code-review.md @@ -34,6 +34,8 @@ Repeat the following block for each finding, incrementing the change is valuable but not required before commit} - **Suggestion:** {concrete, actionable fix} - **Commit-readiness impact:** {blocking | nonblocking} + (CRITICAL/HIGH with substantiated evidence → blocking; + MEDIUM/LOW → nonblocking) ## Questions From 1e6784197a340d14380a778e58b9341d23ac4a09 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 22:01:37 +0000 Subject: [PATCH 07/11] Align template evidence and impact hint with full blocker criteria Add explicit before-commit necessity to the Evidence placeholder for blockers, matching protocol criterion 3. Qualify the blocking impact hint to require substantiated evidence AND demonstrated before-commit necessity. Add canonical disposition guidance: discard unsupported or already-handled claims; downgrade to nonblocking only if a validated, clearly useful concern remains. Assisted-by: AI assistant --- code-review/templates/code-review.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/code-review/templates/code-review.md b/code-review/templates/code-review.md index f6338496..5c236520 100644 --- a/code-review/templates/code-review.md +++ b/code-review/templates/code-review.md @@ -30,12 +30,16 @@ Repeat the following block for each finding, incrementing - **Issue:** {what the problem is} - **Evidence:** {what in the code, tests, or requirements substantiates the claim — for blockers: the concrete problem scenario or violated - requirement, accounting for existing safeguards; for suggestions: why + requirement, accounting for existing safeguards, and why it must be + fixed before commit rather than as a follow-up; for suggestions: why the change is valuable but not required before commit} - **Suggestion:** {concrete, actionable fix} - **Commit-readiness impact:** {blocking | nonblocking} - (CRITICAL/HIGH with substantiated evidence → blocking; - MEDIUM/LOW → nonblocking) + (CRITICAL/HIGH with substantiated evidence and demonstrated + before-commit necessity → blocking; MEDIUM/LOW → nonblocking. + A CRITICAL/HIGH lacking substantiation is discarded if unsupported + or already handled; downgraded to nonblocking only if a validated, + clearly useful concern remains.) ## Questions From ac952e7ed054102ffeedfc52f2729d4169a700bf Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 22:48:44 +0000 Subject: [PATCH 08/11] Address third-round review: readiness guard, publish handoff, snapshot identity, staging binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. code-review/continue.md: move commit-readiness check before ALL verdict routing (was inside CHANGES_REQUESTED only). Re-check on attended user "done" so failing gates/incomplete evaluation cannot be waived by a done label. Preserves optional human suggestions and zero-findings approval. 2. ui-implement/publish.md: require fresh /validate with exact Result:PASS and ValidatedHEAD==localHEAD after gate-fix commits. Pre-push freshness check. Remote-ancestor semantics preserved for unpublished commits. implement/publish.md: re-run validation profile checks after gate-fix commits (aligned with e2e's existing pattern). e2e/publish.md already correct — not edited. 3. bugfix/pr.md snapshot: add file kind (regular/symlink/other), Git mode (100644/100755/120000), and kind-correct payload hashing (readlink target text for symlinks, not dereferenced content). Proven counterexamples: executable-bit, file→symlink, symlink retarget all invisible to prior hash-object/sha256sum-only approach. 4. bugfix/pr.md Step 7: bind staging to converged candidate snapshot. Verify staged delta matches intended candidate before commit. Ignored files require explicit candidate inclusion, review, and validation before staging — never silent force-add. Assisted-by: AI assistant --- bugfix/skills/pr.md | 69 ++++++++++++++++++++++++++-------- code-review/skills/continue.md | 31 ++++++++------- implement/skills/publish.md | 7 ++++ ui-implement/skills/publish.md | 19 ++++++++++ 4 files changed, 94 insertions(+), 32 deletions(-) diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index 704fee8c..dce35186 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -392,8 +392,15 @@ Before committing, run a self-review of the changes to catch issues before they reach external reviewers. **Candidate snapshot definition.** A candidate snapshot is a saved -immutable mapping of every candidate file's path, status (present, -deleted, renamed), and content digest. It covers: +immutable mapping. Each entry is a tuple of: + +- **path** — the file's working-tree path +- **status** — present, deleted, or renamed (with old path) +- **kind** — regular file, symlink, or other (gitlink, etc.) +- **mode** — Git-relevant file mode (e.g., 100644, 100755, 120000) +- **payload digest** — kind-correct content hash (see below) + +The snapshot covers: - tracked staged edits (`git diff --cached`) - tracked unstaged edits (`git diff`) @@ -401,15 +408,33 @@ deleted, renamed), and content digest. It covers: Exclude only `.artifacts/` outputs and build logs — never exclude a candidate source file based on extension or unchanged path presence. -Compute content digests with `git hash-object` for tracked files and -`sha256sum` (or equivalent) for untracked files. Do not stage, commit, -or destructively modify the working tree to build the snapshot. + +**Payload hashing by kind:** + +- **Regular files (tracked):** use Git mode from `git ls-files -s` + or `git diff` output (captures executable-bit changes that + `git hash-object` alone misses). Content digest via + `git hash-object`. +- **Regular files (untracked):** record POSIX file type and + executable bit. Content digest via `sha256sum`. +- **Symlinks:** hash the **link target text** (`readlink`), not the + dereferenced file contents. This detects retargeting even when + both targets have identical bytes, and correctly handles dangling + links. +- **Other kinds (gitlink, etc.):** record the kind and use the + appropriate Git identity; if unsupported, report the entry as + unhandled rather than silently hashing a directory as a regular + file. + +Do not stage, commit, or destructively modify the working tree to +build the snapshot. Do not include mtime, owner, or non-Git +permission bits — these create false churn. Two snapshots are **equal** when they have the same set of paths with -the same status and content digest for each. A change to any candidate -file's content, a newly appearing relevant untracked file, or a -deletion/rename invalidates equality — even if the path list is -unchanged. +the same status, kind, mode, and payload digest for each. A change to +any of these — content, mode, kind, a newly appearing relevant +untracked file, or a deletion/rename — invalidates equality even if +the path list is unchanged. **Convergence state.** Initialize once for this Step 6: @@ -475,19 +500,31 @@ self-review gate should be in the working tree as an uncommitted diff. This step consolidates them into a single commit. (For subsequent feedback rounds, `/feedback` handles its own commits.) -**Stage changes selectively** — don't blindly `git add .`: +**Stage only the converged candidate.** The staged set must match the +paths and identities in the final reviewed-and-validated candidate +snapshot from Step 6. Do not stage paths absent from that snapshot +(including pre-existing staged changes from unrelated work) and do +not omit paths present in it. ```bash -# Review what would be staged -git diff --stat +# Stage exactly the candidate paths +git add {paths from converged candidate snapshot} -# Stage the relevant files (exclude .artifacts/ and other non-code files) -git add path/to/changed/files - -# Verify staging +# Verify: staged delta should match the intended candidate +git diff --cached --stat git status ``` +If verification reveals staged paths not in the candidate snapshot, +unstage them (`git reset HEAD {path}`) and investigate — do not +commit unreviewed content. If a needed file is absent from the +snapshot because it is gitignored, do not silently force-add it. +Instead, explicitly include the intended path in the candidate +construction, provide its contents to the review and validation +gates, and only then stage it (with `git add -f` if necessary). +The project or user retains authority over any actual force-add +decision. + **Commit with a structured message:** ```bash diff --git a/code-review/skills/continue.md b/code-review/skills/continue.md index 51481727..09520f10 100644 --- a/code-review/skills/continue.md +++ b/code-review/skills/continue.md @@ -387,13 +387,16 @@ sometimes write "APPROVED" while still including findings in the body. Scan the entire review for findings, suggestions, questions, or any other feedback content regardless of section headings. -**If APPROVED with no findings anywhere in the review:** +**Commit-readiness check (apply before routing on any verdict).** From +the Verdict Rules in `../../_shared/review-protocol.md`, APPROVED is +valid only when all three conditions hold: (1) required evaluation +across all applicable criteria is complete, (2) all applicable +mandatory quality gates are passing — not merely unrun or incomplete, +(3) no substantiated blockers remain. If any condition fails, the +effective verdict is CHANGES_REQUESTED regardless of the reviewer's +label, the absence of code-level findings, or a user "done" decision. -Before accepting, apply the commit-readiness check: confirm all -applicable mandatory quality gates are passing (not merely unrun or -incomplete) and the required evaluation is complete. If any gate is -unmet or incomplete, do not approve — report the gap and set the -verdict to CHANGES_REQUESTED. +**If APPROVED with no findings anywhere in the review:** Present the approval to the user: @@ -443,8 +446,12 @@ You can: - Run another /continue round to address the suggestions you agree with ``` -If the user says done, compile the aggregated change summary (same as -the "APPROVED with no findings" path above) before proceeding to Step 10. +If the user says done, re-apply the commit-readiness check before +accepting. If any condition now fails (a gate broke, evaluation is +incomplete, or a substantiated blocker surfaced since the table was +presented), report the gap instead of proceeding. Otherwise compile +the aggregated change summary (same as the "APPROVED with no findings" +path above) before proceeding to Step 10. In **unattended mode**, still present the approval and suggestions table — the user can interrupt at any time, and the table gives them the @@ -465,14 +472,6 @@ proceeding to Step 10. **If CHANGES_REQUESTED:** -Before routing, apply the **commit-readiness check** from the Verdict -Rules in `../../_shared/review-protocol.md`: APPROVED is valid only when -the required evaluation is complete, all applicable mandatory quality -gates are passing (not merely unrun or incomplete), and no substantiated -blockers remain. If any condition fails, the verdict is -CHANGES_REQUESTED regardless of the reviewer's label or the absence of -code-level findings. - If the reviewer issued CHANGES_REQUESTED but every finding is nonblocking (MEDIUM/LOW) **and** the commit-readiness check passes, override the verdict to APPROVED and follow the "APPROVED with diff --git a/implement/skills/publish.md b/implement/skills/publish.md index 8625ef65..4782bbc9 100644 --- a/implement/skills/publish.md +++ b/implement/skills/publish.md @@ -127,6 +127,13 @@ git add {fixed files} git commit -m "{issue-key}: address cross-cutting review findings" ``` +The gate fixes changed the candidate after `/validate`. Re-run the +validation profile's required checks on the post-fix candidate to +confirm the fixes did not break anything. If checks fail or produce +further code changes, stop and present the gap to the user rather +than entering an unbounded review↔validate cycle. Skip this +revalidation when the gate reported PASS without making code fixes. + ### Step 3: Confirm Details Present the PR details to the user for confirmation: diff --git a/ui-implement/skills/publish.md b/ui-implement/skills/publish.md index 3f1b84d2..fb6ed09f 100644 --- a/ui-implement/skills/publish.md +++ b/ui-implement/skills/publish.md @@ -136,6 +136,25 @@ git add {fixed files} git commit -m "{issue-key}: address cross-cutting review findings" ``` +The committed gate fixes changed the candidate since `/validate` +recorded its ValidatedHEAD. Before proceeding, re-run `/validate` to +produce a fresh validation report with an exact standalone `Result: +PASS` and a ValidatedHEAD matching the current local HEAD. Do not +merely rewrite the SHA in the old report. If validation fails or +produces further code changes that cannot be validated, stop and +present the gap to the user rather than entering an unbounded +review↔validate cycle. Skip this revalidation when the gate reported +PASS without making any code fixes (candidate unchanged). + +Before pushing in Step 4, verify that the local HEAD still equals the +ValidatedHEAD from the fresh report. If any commits were added during +confirmation (by the user or another process), the validation evidence +is stale — stop and re-run `/validate`. Preserve the existing +unexpected-remote-divergence check for detecting changes pushed by +others, but do not require remote HEAD to equal the new local HEAD +before push — remote is legitimately an ancestor of unpublished +local commits until the normal push lands. + ### Step 3: Confirm Details Present the PR details to the user for confirmation: From 56bbc25f245bf01925440e9378dde44e38c65f72 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 23:00:09 +0000 Subject: [PATCH 09/11] Fix staging safety, hash consistency, publish ordering, report format A. bugfix/pr.md Step 7: stop and report on staging mismatch instead of auto-unstaging user work. Pre-commit verification compares actual staged entry identities (kind+mode+payload), not just stat/status. B. bugfix/pr.md snapshot: use git hash-object -t blob consistently for regular files regardless of tracked/untracked origin (stage-only transition yields same identity). Qualify mode detection: git diff shows prospective mode for unstaged chmod; ls-files -s shows index mode only. Normalize identity independent of M/A/R labels. C. implement/publish.md: move revalidation check BEFORE git add/commit (aligned with e2e's existing correct pattern). Stop if checks fail or produce unreviewed changes. D. ui-implement/publish.md: match actual validate report format (## Result section with standalone PASS, ## Validated HEAD section). Use Step 1's existing parse contract. Assisted-by: AI assistant --- bugfix/skills/pr.md | 81 +++++++++++++++++++++------------- implement/skills/publish.md | 20 ++++----- ui-implement/skills/publish.md | 34 +++++++------- 3 files changed, 79 insertions(+), 56 deletions(-) diff --git a/bugfix/skills/pr.md b/bugfix/skills/pr.md index dce35186..e1557364 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -409,18 +409,28 @@ The snapshot covers: Exclude only `.artifacts/` outputs and build logs — never exclude a candidate source file based on extension or unchanged path presence. -**Payload hashing by kind:** - -- **Regular files (tracked):** use Git mode from `git ls-files -s` - or `git diff` output (captures executable-bit changes that - `git hash-object` alone misses). Content digest via - `git hash-object`. -- **Regular files (untracked):** record POSIX file type and - executable bit. Content digest via `sha256sum`. -- **Symlinks:** hash the **link target text** (`readlink`), not the - dereferenced file contents. This detects retargeting even when - both targets have identical bytes, and correctly handles dangling - links. +**Mode detection:** For tracked files, `git diff` output shows the +prospective Git mode (e.g., `old mode 100644 / new mode 100755`) +which reflects unstaged chmod changes. `git ls-files -s` shows only +the index mode, which does NOT reflect unstaged mode changes — use it +only for staged entries. For untracked regular files, record whether +the file has the executable bit set (this is the mode Git would assign +on `git add`). Stage-only status transitions (untracked→staged, +unstaged→staged) must not change the recorded mode or payload for the +same working-tree file. + +**Payload hashing by kind (use consistently regardless of +tracked/untracked origin):** + +- **Regular files:** content digest via `git hash-object -t blob` + (computes the Git blob OID without staging). This produces the + same identity whether the file is tracked, untracked, or staged, + so a stage-only transition does not create a false mismatch. +- **Symlinks:** hash the **link target text** (`readlink -n`), not + the dereferenced file contents. Use `git hash-object -t blob` on + the target string (e.g., `readlink -n {path} | git hash-object + --stdin -t blob`). This detects retargeting even when both targets + have identical bytes, and correctly handles dangling links. - **Other kinds (gitlink, etc.):** record the kind and use the appropriate Git identity; if unsupported, report the entry as unhandled rather than silently hashing a directory as a regular @@ -428,7 +438,10 @@ candidate source file based on extension or unchanged path presence. Do not stage, commit, or destructively modify the working tree to build the snapshot. Do not include mtime, owner, or non-Git -permission bits — these create false churn. +permission bits — these create false churn. Normalized logical path +identity must not depend on M/A/R presentation labels — two entries +with the same path, kind, mode, and payload are equal regardless of +how Git labels the status transition. Two snapshots are **equal** when they have the same set of paths with the same status, kind, mode, and payload digest for each. A change to @@ -501,29 +514,37 @@ This step consolidates them into a single commit. (For subsequent feedback rounds, `/feedback` handles its own commits.) **Stage only the converged candidate.** The staged set must match the -paths and identities in the final reviewed-and-validated candidate -snapshot from Step 6. Do not stage paths absent from that snapshot -(including pre-existing staged changes from unrelated work) and do -not omit paths present in it. +paths and identities (path, kind, mode, payload) in the final +reviewed-and-validated candidate snapshot from Step 6. ```bash # Stage exactly the candidate paths git add {paths from converged candidate snapshot} - -# Verify: staged delta should match the intended candidate -git diff --cached --stat -git status ``` -If verification reveals staged paths not in the candidate snapshot, -unstage them (`git reset HEAD {path}`) and investigate — do not -commit unreviewed content. If a needed file is absent from the -snapshot because it is gitignored, do not silently force-add it. -Instead, explicitly include the intended path in the candidate -construction, provide its contents to the review and validation -gates, and only then stage it (with `git add -f` if necessary). -The project or user retains authority over any actual force-add -decision. +**Pre-commit identity verification.** Before committing, compare +the actual staged index entries against the converged candidate +snapshot — not merely `--stat` or `git status`, which show paths and +coarse change labels but cannot prove kind, mode, or payload +identity. For each candidate path, confirm the staged entry's mode, +kind, and blob OID match the snapshot's recorded values. For each +staged entry NOT in the candidate snapshot, the commit would include +unreviewed content. + +If any mismatch is found (extra staged path, missing candidate path, +or changed identity), **stop and report** — do not commit, and do not +automatically unstage or reset unrelated work that may belong to the +user. Return to the appropriate capture/review/validation step with +remaining convergence budget, or stop for a user decision if the +mismatch cannot be resolved within budget. + +If a needed file is absent from the snapshot because it is +gitignored, do not silently force-add it. Instead, explicitly +include the intended path as read-only candidate input (read its +contents without staging), provide it to the review and validation +gates, and only then stage it (with `git add -f` if the project or +user authorizes). The original workflow instructions did not specify +force-add; this is narrow hardening of the candidate invariant. **Commit with a structured message:** diff --git a/implement/skills/publish.md b/implement/skills/publish.md index 4782bbc9..fa8e804d 100644 --- a/implement/skills/publish.md +++ b/implement/skills/publish.md @@ -117,23 +117,21 @@ If the gate reports FLAG (unfixed CRITICAL or HIGH findings), stop and present the findings to the user. Do not proceed until the user decides how to handle them. -If the gate made code fixes, commit them before proceeding: +If the gate made code fixes, re-run the validation profile's required +checks on the post-fix working-tree candidate **before** staging or +committing. If checks fail or produce further unreviewed code changes, +stop and present the gap to the user — hand back to the existing +`/validate` and review workflow rather than entering an unbounded +automatic cycle. Skip this revalidation when the gate reported PASS +without making any code fixes (candidate unchanged). -```bash -git add {fixed files} -``` +Once checks pass on the unchanged post-fix candidate, commit: ```bash +git add {fixed files} git commit -m "{issue-key}: address cross-cutting review findings" ``` -The gate fixes changed the candidate after `/validate`. Re-run the -validation profile's required checks on the post-fix candidate to -confirm the fixes did not break anything. If checks fail or produce -further code changes, stop and present the gap to the user rather -than entering an unbounded review↔validate cycle. Skip this -revalidation when the gate reported PASS without making code fixes. - ### Step 3: Confirm Details Present the PR details to the user for confirmation: diff --git a/ui-implement/skills/publish.md b/ui-implement/skills/publish.md index fb6ed09f..eb46d485 100644 --- a/ui-implement/skills/publish.md +++ b/ui-implement/skills/publish.md @@ -137,23 +137,27 @@ git commit -m "{issue-key}: address cross-cutting review findings" ``` The committed gate fixes changed the candidate since `/validate` -recorded its ValidatedHEAD. Before proceeding, re-run `/validate` to -produce a fresh validation report with an exact standalone `Result: -PASS` and a ValidatedHEAD matching the current local HEAD. Do not -merely rewrite the SHA in the old report. If validation fails or -produces further code changes that cannot be validated, stop and -present the gap to the user rather than entering an unbounded -review↔validate cycle. Skip this revalidation when the gate reported -PASS without making any code fixes (candidate unchanged). +recorded its `## Validated HEAD`. Before proceeding, re-run +`/validate` to produce a fresh validation report. Parse it using the +same contract as Step 1: the first non-empty, non-comment line after +`## Result` must be exactly `PASS`, and the `## Validated HEAD` value +must equal the current local HEAD (`git rev-parse HEAD`). Do not +merely rewrite the SHA in the old report. If validation fails, the +result is not `PASS`, or validation produces further code changes +that cannot be validated, stop and present the gap to the user — +hand back to the existing `/validate` and review workflow rather than +entering an unbounded automatic cycle. Skip this revalidation when +the gate reported PASS without making any code fixes (candidate +unchanged). Before pushing in Step 4, verify that the local HEAD still equals the -ValidatedHEAD from the fresh report. If any commits were added during -confirmation (by the user or another process), the validation evidence -is stale — stop and re-run `/validate`. Preserve the existing -unexpected-remote-divergence check for detecting changes pushed by -others, but do not require remote HEAD to equal the new local HEAD -before push — remote is legitimately an ancestor of unpublished -local commits until the normal push lands. +`## Validated HEAD` from the fresh report. If any commits were added +during confirmation (by the user or another process), the validation +evidence is stale — stop and re-run `/validate`. Preserve the +existing unexpected-remote-divergence check for detecting changes +pushed by others, but do not require remote HEAD to equal the new +local HEAD before push — remote is legitimately an ancestor of +unpublished local commits until the normal push lands. ### Step 3: Confirm Details From 822fcbe0f6c78319ad54a6873cc5ca01ec6b4416 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Wed, 28 Oct 2026 23:11:06 +0000 Subject: [PATCH 10/11] Clarify publish gate-fix routing and UI report format reference implement/publish.md: make PASS-without-fixes proceed directly to Step 3; PASS-with-fixes completes revalidation and commit first. ui-implement/publish.md: same routing; reference actual validate report format (## Result section with standalone PASS token, ## Validated HEAD section) matching Step 1's existing parse contract. Assisted-by: AI assistant --- implement/skills/publish.md | 11 +++++++---- ui-implement/skills/publish.md | 28 +++++++++++++++------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/implement/skills/publish.md b/implement/skills/publish.md index fa8e804d..f4235cde 100644 --- a/implement/skills/publish.md +++ b/implement/skills/publish.md @@ -117,21 +117,24 @@ If the gate reports FLAG (unfixed CRITICAL or HIGH findings), stop and present the findings to the user. Do not proceed until the user decides how to handle them. +If the gate reported PASS **without making any code fixes** (candidate +unchanged), proceed directly to Step 3. + If the gate made code fixes, re-run the validation profile's required checks on the post-fix working-tree candidate **before** staging or committing. If checks fail or produce further unreviewed code changes, stop and present the gap to the user — hand back to the existing `/validate` and review workflow rather than entering an unbounded -automatic cycle. Skip this revalidation when the gate reported PASS -without making any code fixes (candidate unchanged). - -Once checks pass on the unchanged post-fix candidate, commit: +automatic cycle. Once checks pass on the unchanged post-fix candidate, +commit the gate fixes: ```bash git add {fixed files} git commit -m "{issue-key}: address cross-cutting review findings" ``` +Then proceed to Step 3. + ### Step 3: Confirm Details Present the PR details to the user for confirmation: diff --git a/ui-implement/skills/publish.md b/ui-implement/skills/publish.md index eb46d485..b8860759 100644 --- a/ui-implement/skills/publish.md +++ b/ui-implement/skills/publish.md @@ -129,7 +129,10 @@ If the gate reports FLAG (unfixed CRITICAL or HIGH findings), stop and present the findings to the user. Do not proceed until the user decides how to handle them. -If the gate made code fixes, commit them before proceeding: +If the gate reported PASS **without making any code fixes** (candidate +unchanged), proceed directly to Step 3. + +If the gate made code fixes, commit them first: ```bash git add {fixed files} @@ -137,18 +140,17 @@ git commit -m "{issue-key}: address cross-cutting review findings" ``` The committed gate fixes changed the candidate since `/validate` -recorded its `## Validated HEAD`. Before proceeding, re-run -`/validate` to produce a fresh validation report. Parse it using the -same contract as Step 1: the first non-empty, non-comment line after -`## Result` must be exactly `PASS`, and the `## Validated HEAD` value -must equal the current local HEAD (`git rev-parse HEAD`). Do not -merely rewrite the SHA in the old report. If validation fails, the -result is not `PASS`, or validation produces further code changes -that cannot be validated, stop and present the gap to the user — -hand back to the existing `/validate` and review workflow rather than -entering an unbounded automatic cycle. Skip this revalidation when -the gate reported PASS without making any code fixes (candidate -unchanged). +recorded its `## Validated HEAD`. Re-run `/validate` to produce a +fresh validation report. Parse it using the same contract as Step 1: +the first non-empty, non-comment line after `## Result` must be +exactly `PASS`, and the `## Validated HEAD` value must equal the +current local HEAD (`git rev-parse HEAD`). Do not merely rewrite the +SHA in the old report. If validation fails, the result is not `PASS`, +or validation produces further code changes that cannot be validated, +stop and present the gap to the user — hand back to the existing +`/validate` and review workflow rather than entering an unbounded +automatic cycle. Once the fresh report shows PASS with matching HEAD, +proceed to Step 3. Before pushing in Step 4, verify that the local HEAD still equals the `## Validated HEAD` from the fresh report. If any commits were added From c467d9c047ea2bed0a9b9fad21e2b351695526e9 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 29 Oct 2026 20:31:40 +0000 Subject: [PATCH 11/11] Escalate bugfix, implement, ui-implement version bumps to MINOR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer correctly identified that direct candidate convergence/staging rules (bugfix/pr.md) and post-review validation handoffs (implement/publish.md, ui-implement/publish.md) are behavioral changes, not merely shared-resource patch cascades: - bugfix/SKILL.md: 0.9.5 → 0.10.0 (MINOR from main's 0.9.4) - implement/SKILL.md: 0.13.1 → 0.14.0 (MINOR from main's 0.13.0) - ui-implement/SKILL.md: 0.2.1 → 0.3.0 (MINOR from main's 0.2.0) Assisted-by: AI assistant --- bugfix/SKILL.md | 2 +- implement/SKILL.md | 2 +- ui-implement/SKILL.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bugfix/SKILL.md b/bugfix/SKILL.md index 94cea0e9..d3d8c420 100644 --- a/bugfix/SKILL.md +++ b/bugfix/SKILL.md @@ -1,6 +1,6 @@ --- name: bugfix -version: 0.9.5 +version: 0.10.0 description: >- Diagnostic and repair workflow that analyzes error logs, traces root causes, implements fixes, and verifies with regression tests. diff --git a/implement/SKILL.md b/implement/SKILL.md index a32b9837..5aadeb06 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -1,6 +1,6 @@ --- name: implement -version: 0.13.1 +version: 0.14.0 description: >- Story-to-code workflow that takes a Jira Story, plans the implementation, writes contract-based tests and production code via TDD, validates against diff --git a/ui-implement/SKILL.md b/ui-implement/SKILL.md index 7809e40d..86f2abfd 100644 --- a/ui-implement/SKILL.md +++ b/ui-implement/SKILL.md @@ -1,6 +1,6 @@ --- name: ui-implement -version: 0.2.1 +version: 0.3.0 description: >- Story-to-code workflow for UI/front-end stories. Takes a Jira [UI] Story, plans the implementation using discovered design-system and testing