skill: refine repo-review skill instructions for clarity and calibration
This commit is contained in:
@@ -22,6 +22,8 @@ Use these severity levels independently of category:
|
||||
|
||||
Sort findings by severity, then by impact within the same severity. Security is a category, not a severity; a security finding can have any severity.
|
||||
|
||||
Make each `Analysis` identify the existing behavior, boundary, or invariant under review, the evidence and reachable violation path, and the material impact. Do not use any report section as a backlog for absent WIP, planned, or future capabilities.
|
||||
|
||||
Use this structure:
|
||||
|
||||
```markdown
|
||||
@@ -34,19 +36,22 @@ Use this structure:
|
||||
|
||||
- Findings
|
||||
- ID: <CATEGORY-NNN>
|
||||
- Severity: Critical | High | Medium | Low
|
||||
- Location: <filename:line>
|
||||
- Analysis: <evidence, reachable causal path, and material impact>
|
||||
- Recommendation: <root-cause repair direction>
|
||||
- Severity: Critical | High | Medium | Low
|
||||
- Location: <filename:line>
|
||||
- Analysis: <existing behavior or invariant, evidence and reachable violation path, and material impact>
|
||||
- Recommendation: <root-cause repair direction>
|
||||
|
||||
- ID: <CATEGORY-NNN>
|
||||
- <same as above>
|
||||
|
||||
- Architecture drift
|
||||
- <material differences between documented and implemented architecture, referencing finding IDs instead of duplicating analysis>
|
||||
- <material current differences between documented and implemented architecture, referencing finding IDs instead of duplicating analysis>
|
||||
|
||||
- Documentation drift
|
||||
- <documentation inaccuracies that can materially mislead implementation, operation, or review>
|
||||
|
||||
- Unverified areas
|
||||
- <unverified area, reason, and required validation>
|
||||
- <near-qualifying current defect, missing fact, and the specific validation needed>
|
||||
```
|
||||
|
||||
If there are no qualifying findings, write `Findings: None`. Write `None` for any other empty section. Do not omit verification failures or review limitations.
|
||||
|
||||
@@ -1,27 +1,27 @@
|
||||
# Review Calibration
|
||||
|
||||
Use this reference only after the open-ended discovery pass. Apply it to challenge candidate findings and suppress false positives.
|
||||
Use this reference only after open-ended discovery. Apply it to decide which candidates deserve a place in the report.
|
||||
|
||||
## Finding eligibility
|
||||
## Admit a finding
|
||||
|
||||
Accept a candidate as a finding only when all of these are present:
|
||||
Accept a candidate only when all of these are present:
|
||||
|
||||
- Concrete evidence in current code or documented design.
|
||||
- A violated invariant or reachable, plausible failure scenario.
|
||||
- A material consequence under supported behavior or realistic roadmap use.
|
||||
- A precise source location that lets another developer verify the analysis.
|
||||
- A recommendation that addresses the underlying cause.
|
||||
- An existing implemented behavior, boundary, or intrinsic invariant, including code already exposed within a WIP feature. An absent part of a WIP, planned, or future capability is not itself a basis.
|
||||
- Concrete evidence that current code violates that behavior or invariant through a reachable, plausible path.
|
||||
- A material consequence under production-grade operation of the existing capability, without requiring the finished product's full feature set.
|
||||
- A precise source location and a recommendation that primarily corrects existing behavior or structure rather than delivers a missing product capability.
|
||||
|
||||
Actively seek the strongest disconfirming evidence before accepting a candidate. Check whether another layer enforces the invariant, the path is unreachable, the behavior is explicitly unsupported, or the tradeoff is intentional and adequately contained.
|
||||
Ask: **Does the repair mainly correct existing behavior, or design and deliver a capability that does not yet exist?** The amount of code required is not decisive; the repair's purpose is.
|
||||
|
||||
## Calibration rules
|
||||
Seek the strongest disconfirming evidence. Check other layers, tests, supported scope, containment, and intentional tradeoffs before accepting a candidate.
|
||||
|
||||
- Do not report a missing abstraction, restriction, validation, limit, test, or defensive mechanism solely because it is absent. Demonstrate the failure path or violated invariant caused by the absence.
|
||||
- Do not treat a permissive configuration or documented tradeoff as a problem by itself. Report it only when a supported setting bypasses required processing or breaks security, lifecycle, or consistency invariants.
|
||||
- Do not treat a simple implementation as an architecture flaw without concrete coupling, duplicated responsibility, boundary leakage, unsafe change propagation, or realistic scaling impact.
|
||||
- Do not report theoretical performance limits without a reachable hot path, resource-growth mechanism, and material consequence under realistic use or roadmap requirements.
|
||||
- Do not report style preferences, naming preferences, speculative future features, or optional hardening as defects.
|
||||
- Do not weaken or reinterpret a valid test to remove an implementation failure. Confirm intended behavior before deciding whether the test or implementation is wrong.
|
||||
- Do not duplicate one root cause across review lenses. Assign one primary category and describe secondary impacts in its analysis.
|
||||
## Apply the boundary
|
||||
|
||||
Place credible concerns that lack required evidence in `Unverified areas`. State what is unknown, why it matters, and the smallest validation needed to resolve it.
|
||||
- A missing safeguard qualifies only when it is intrinsic to an existing interface or mechanism and its absence creates a demonstrated failure. Do not turn expected product policies or lifecycle subsystems into defects merely because a mature product will need them.
|
||||
- For configuration, distinguish enforceable technical validity from operator judgment. Report values that violate an implemented mechanism's objective preconditions or are silently misapplied; do not claim the program can prove secret entropy or prevent every poor but technically valid policy choice.
|
||||
- Report architecture or evolution risk only when current design causes concrete coupling, boundary leakage, unsafe change propagation, or material rework for a committed direction. A missing future subsystem is not an architecture flaw.
|
||||
- Require a reachable hot path, resource-growth mechanism, and material impact for performance findings. Do not report style, naming, speculative scale, or optional preference as a defect.
|
||||
- Keep one primary violated invariant and root cause per finding. Do not combine independent missing defenses to raise severity; assign one category and describe only supported secondary impacts.
|
||||
- Preserve valid tests and intended behavior. Never weaken a test to dismiss an implementation failure.
|
||||
|
||||
Use `Unverified areas` only for a near-qualifying current defect that a specific factual check can resolve. Omit undecided product policy, future capability, and general hardening ideas rather than using this section as a backlog.
|
||||
|
||||
Reference in New Issue
Block a user