feat(ce-review): enforce table format, require question tool, fix autofix_class calibration (#454)
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -114,10 +114,10 @@
|
||||
"P3": "Low-impact, narrow scope, minor improvement. User's discretion."
|
||||
},
|
||||
"autofix_classes": {
|
||||
"safe_auto": "Local, deterministic code or test fix suitable for the in-skill fixer in autonomous mode.",
|
||||
"gated_auto": "Concrete fix exists, but it changes behavior, permissions, contracts, or other sensitive areas that deserve explicit approval.",
|
||||
"manual": "Actionable issue that should become residual work rather than an in-skill autofix.",
|
||||
"advisory": "Informational or operational item that should be surfaced in the report only."
|
||||
"safe_auto": "Local, deterministic code or test fix suitable for the in-skill fixer. Examples: extract duplicated helper, add missing nil check, fix off-by-one, add missing test, remove dead code. Do not default to advisory when a concrete safe fix exists.",
|
||||
"gated_auto": "Concrete fix exists, but it changes behavior, permissions, contracts, or other sensitive areas that deserve explicit approval. Examples: add auth to unprotected endpoint, change API response shape.",
|
||||
"manual": "Actionable issue that requires design decisions or cross-cutting changes. Examples: redesign data model, add pagination strategy, choose between architectural approaches.",
|
||||
"advisory": "Informational or operational item that should be surfaced in the report only. Examples: design asymmetry the PR improves but does not fully resolve, residual risk notes, deployment considerations."
|
||||
},
|
||||
"owners": {
|
||||
"review-fixer": "The in-skill fixer can own this when policy allows.",
|
||||
|
||||
@@ -92,9 +92,30 @@ Use this **exact format** when presenting synthesized review findings. Findings
|
||||
> **Fix order:** P0 auth bypass -> P1 memory/pagination -> P2 error handling if straightforward
|
||||
```
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
Do NOT produce output like this. The following is wrong:
|
||||
|
||||
```markdown
|
||||
Findings
|
||||
|
||||
Sev: P1
|
||||
File: foo.go:42
|
||||
Issue: Some problem description
|
||||
Reviewer(s): adversarial
|
||||
Confidence: 0.85
|
||||
Route: advisory -> human
|
||||
────────────────────────────────────────
|
||||
Sev: P2
|
||||
File: bar.go:99
|
||||
Issue: Another problem
|
||||
```
|
||||
|
||||
This fails because: no pipe-delimited tables, no severity-grouped `###` headers, uses box-drawing horizontal rules, no numbered findings, no `## Code Review Results` title, and the verdict is not in a blockquote. Always use the table format from the example above.
|
||||
|
||||
## Formatting Rules
|
||||
|
||||
- **Pipe-delimited markdown tables** -- never ASCII box-drawing characters
|
||||
- **Pipe-delimited markdown tables** for findings -- never ASCII box-drawing characters or per-finding horizontal-rule separators between entries (the report-level `---` before the verdict is still required)
|
||||
- **Severity-grouped sections** -- `### P0 -- Critical`, `### P1 -- High`, `### P2 -- Moderate`, `### P3 -- Low`. Omit empty severity levels.
|
||||
- **Always include file:line location** for code review issues
|
||||
- **Reviewer column** shows which persona(s) flagged the issue. Multiple reviewers = cross-reviewer agreement.
|
||||
|
||||
@@ -44,7 +44,12 @@ Rules:
|
||||
- Every finding MUST include at least one evidence item grounded in the actual code.
|
||||
- Set pre_existing to true ONLY for issues in unchanged code that are unrelated to this diff. If the diff makes the issue newly relevant, it is NOT pre-existing.
|
||||
- You are operationally read-only. You may use non-mutating inspection commands, including read-oriented `git` / `gh` commands, to gather evidence. Do not edit files, change branches, commit, push, create PRs, or otherwise mutate the checkout or repository state.
|
||||
- Set `autofix_class` conservatively. Use `safe_auto` only when the fix is local, deterministic, and low-risk. Use `gated_auto` when a concrete fix exists but changes behavior/contracts/permissions. Use `manual` for actionable residual work. Use `advisory` for report-only items that should not become code-fix work.
|
||||
- Set `autofix_class` accurately -- not every finding is `advisory`. Use this decision guide:
|
||||
- `safe_auto`: The fix is local and deterministic — the fixer can apply it mechanically without design judgment. Examples: extracting a duplicated helper, adding a missing nil/null check, fixing an off-by-one, adding a missing test for an untested code path, removing dead code.
|
||||
- `gated_auto`: A concrete fix exists but it changes contracts, permissions, or crosses a module boundary in a way that deserves explicit approval. Examples: adding authentication to an unprotected endpoint, changing a public API response shape, switching from soft-delete to hard-delete.
|
||||
- `manual`: Actionable work that requires design decisions or cross-cutting changes. Examples: redesigning a data model, choosing between two valid architectural approaches, adding pagination to an unbounded query.
|
||||
- `advisory`: Report-only items that should not become code-fix work. Examples: noting a design asymmetry the PR improves but doesn't fully resolve, flagging a residual risk, deployment notes.
|
||||
Do not default to `advisory` when uncertain -- if a concrete fix is obvious, classify it as `safe_auto` or `gated_auto`.
|
||||
- Set `owner` to the default next actor for this finding: `review-fixer`, `downstream-resolver`, `human`, or `release`.
|
||||
- Set `requires_verification` to true whenever the likely fix needs targeted tests, a focused re-review, or operational validation before it should be trusted.
|
||||
- suggested_fix is optional. Only include it when the fix is obvious and correct. A bad suggestion is worse than none.
|
||||
|
||||
Reference in New Issue
Block a user