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 eecf3a7d..16f1b464 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. @@ -96,11 +134,102 @@ 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 | 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 | +## 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. + +## Criteria Coverage + +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: + +- **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 @@ -115,3 +244,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..d3d8c420 100644 --- a/bugfix/SKILL.md +++ b/bugfix/SKILL.md @@ -1,6 +1,6 @@ --- name: bugfix -version: 0.9.4 +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/bugfix/skills/pr.md b/bugfix/skills/pr.md index 616a0a59..e1557364 100644 --- a/bugfix/skills/pr.md +++ b/bugfix/skills/pr.md @@ -391,21 +391,113 @@ 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. 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`) +- all relevant untracked files (`git ls-files --others --exclude-standard`) + +Exclude only `.artifacts/` outputs and build logs — never exclude a +candidate source file based on extension or unchanged path presence. + +**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 + 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. 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 +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: -| 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) | +```text +review_rounds_remaining = 3 +validated_candidate = snapshot_candidate() # from Step 5's final state +reviewed_candidate = null +``` + +**Review loop.** + +1. Invoke the self-review gate: + + Read and follow `../../_shared/recipes/self-review-gate.md` with: + + | 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) | -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. + Deduct the gate's reported `Rounds: {N}` from + `review_rounds_remaining` immediately after return. -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. +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. + +7. Validation remediation produced a different candidate. If + `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. + +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 @@ -421,19 +513,39 @@ 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 (path, kind, mode, payload) in the final +reviewed-and-validated candidate snapshot from Step 6. ```bash -# Review what would be staged -git diff --stat - -# Stage the relevant files (exclude .artifacts/ and other non-code files) -git add path/to/changed/files - -# Verify staging -git status +# Stage exactly the candidate paths +git add {paths from converged candidate snapshot} ``` +**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:** ```bash 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..09520f10 100644 --- a/code-review/skills/continue.md +++ b/code-review/skills/continue.md @@ -308,14 +308,18 @@ 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 - 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 @@ -324,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 @@ -375,6 +387,15 @@ 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. +**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. + **If APPROVED with no findings anywhere in the review:** Present the approval to the user: @@ -425,23 +446,39 @@ 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 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, +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: +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:** +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 +519,18 @@ 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, or the required + 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 + 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..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 @@ -350,8 +358,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 +382,18 @@ 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, or the required + 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: + 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..5c236520 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 @@ -12,10 +25,21 @@ 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 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 + 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 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 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..5aadeb06 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -1,6 +1,6 @@ --- name: implement -version: 0.13.0 +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/implement/skills/publish.md b/implement/skills/publish.md index 8625ef65..f4235cde 100644 --- a/implement/skills/publish.md +++ b/implement/skills/publish.md @@ -117,16 +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 made code fixes, commit them before proceeding: +If the gate reported PASS **without making any code fixes** (candidate +unchanged), proceed directly to Step 3. -```bash -git add {fixed files} -``` +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. 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/SKILL.md b/ui-implement/SKILL.md index 8202fc9c..86f2abfd 100644 --- a/ui-implement/SKILL.md +++ b/ui-implement/SKILL.md @@ -1,6 +1,6 @@ --- name: ui-implement -version: 0.2.0 +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 diff --git a/ui-implement/skills/publish.md b/ui-implement/skills/publish.md index 3f1b84d2..b8860759 100644 --- a/ui-implement/skills/publish.md +++ b/ui-implement/skills/publish.md @@ -129,13 +129,38 @@ 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} git commit -m "{issue-key}: address cross-cutting review findings" ``` +The committed gate fixes changed the candidate since `/validate` +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 +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: