diff --git a/.github/skills/python-code-reviewer/skill.md b/.github/skills/python-code-reviewer/skill.md index f0aac16..d7c8e7e 100644 --- a/.github/skills/python-code-reviewer/skill.md +++ b/.github/skills/python-code-reviewer/skill.md @@ -39,13 +39,30 @@ upgrade policy") — do not report it as a defect or recommend widening it. 6. **Assess Boundary and Coupling Health:** Evaluate UI/service/persistence/provider dependency flow, identify circular dependencies, leaky abstractions, and transaction ownership ambiguity. 7. **Assess Invariant Placement:** For each hard rule, decide whether it belongs in docs (rationale), instructions (active steering), skills (periodic audit procedure), or deterministic tests (enforcement). 8. **Verify Claims:** This is a `uv` project (`uv.lock`, root `ruff.toml`). Run `uv run ruff check .`, `uv run ty check`, and `uv run pytest -q -m "not external"` rather than guessing, and record the exact commands and their outcomes in the report. -9. **Prioritize Hot Paths:** Focus deeply on request handling, database sessions, background workers, and external API calls. -10. **Enforce Read-Only Safety:** Do not modify code unless explicitly instructed. -11. **Escalate Provenance Audits:** For evidence/provenance-heavy changes, apply invariant checks from `.github/skills/evidence-provenance-auditor/skill.md` and include pass/fail outcomes in the report. -12. **Escalate Test-Suite Audits:** When findings touch test coverage, redundancy, or assertion strength, apply `.github/skills/test-effectiveness-auditor/skill.md` and include its outcomes alongside the provenance results. +9. **Validate Recommendations Against Consumers:** A recommendation is a claim about the future and must be verified like any other. Before recommending a change to a shared symbol — a model field, an exception attribute, a helper's return value, a function signature — enumerate **every** consumer of that symbol (`grep` the whole repo, including tests) and confirm the fix is safe for each one. Record the consumers in the finding's **Blast Radius**. A fix that is correct for the path that produced the finding can silently break a second consumer, and evidence/provenance and logging paths are the usual casualties because they read the same fields the UI does. +10. **Prioritize Hot Paths:** Focus deeply on request handling, database sessions, background workers, and external API calls. +11. **Enforce Read-Only Safety:** Do not modify code unless explicitly instructed. +12. **Escalate Provenance Audits:** For evidence/provenance-heavy changes, apply invariant checks from `.github/skills/evidence-provenance-auditor/skill.md` and include pass/fail outcomes in the report. +13. **Escalate Test-Suite Audits:** When findings touch test coverage, redundancy, or assertion strength, apply `.github/skills/test-effectiveness-auditor/skill.md` and include its outcomes alongside the provenance results. + +### Worked example: why step 9 exists + +The 2026-08-23 review recommended fixing a filesystem-path leak in +`classify_unexpected_error` by making `AppError.message` generic and logging the exception +detail instead. The analysis of the leak was correct, and the fix was implemented as written. + +It was wrong. `AppError.message` had a second consumer the review never traced: +`format_error_detail`, which writes `ExecutionAttempt.error_detail` — a **provenance record**. +The recommended fix closed a privacy leak by silently stripping root-cause data from the +evidence history this system exists to preserve. It was caught only because an unrelated +integration test asserted on the persisted error text. + +The correct fix separated the audiences — a user-safe `message` and an internal-only `detail` +that still reaches evidence and logs. One `grep` for consumers of `.message` during the review +would have found this. Treat any recommendation that changes a widely-read field as unverified +until its consumers are enumerated. ## Repo-Specific Deterministic Checks (Transcription) - When reviewing this repository, always include explicit pass/fail checks for the following. Where **Enforced by** reads *unenforced*, recommending a deterministic test is itself a finding. @@ -127,6 +144,30 @@ Severity reflects concrete consequence, never style preference or effort to fix. - **Medium:** Correctness risk under load or edge conditions (N+1, missing eager load, leaked task, missing timeout); drift between docs and code with no immediate runtime impact. - **Low:** Maintainability, typing completeness, duplication, naming, or dead code with no behavioral risk. +### Reachability + +Severity states how bad the consequence is; **Reachability** states whether it can happen today. +They are independent, and a finding is not complete without both. Record one of: + +- **Live:** reachable in the current configuration and deployment. +- **Latent:** the defective code is present but unreachable because of a current setting, single- + instance deployment, or absent caller. **State the exact condition that unblocks it.** +- **Theoretical:** requires a combination the project has explicitly ruled out. + +Latent findings carry a scheduling constraint that severity alone cannot express: a latent defect +must usually be fixed *before* the change that makes it live, not after. Say so explicitly in the +finding and reflect the ordering in the §9 action plan — for example, "fix the retry-category gate +before raising `worker_max_retries` above 0," or "handle this `IntegrityError` before deploying a +second worker replica." Do not downgrade severity merely because a finding is latent. + +### Conflicting invariants + +When a fix sits between two invariants that pull in opposite directions, say so in the +**Recommendation** and name both, along with the test that guards each. Flag explicitly what the +over-correction would be, because the simplest-looking fix usually satisfies one invariant by +silently destroying the other. A recommendation that resolves one side without naming the other is +incomplete and will be implemented incorrectly. + ## Output Report Structure & Template Generate Markdown reports at `./docs/reviews/-code-review.md` following this exact @@ -157,8 +198,13 @@ template structure. Reports are dated, non-canonical artifacts: `docs/reviews/** ### Critical Severity #### [CRIT-01] Title - **Location:** `path/to/file.py:lines` +- **Reachability:** Live / Latent (state the exact condition that unblocks it) / Theoretical - **Problem & Consequence:** Concrete consequence, not a style opinion. -- **Recommendation:** Fix with before/after sketch. +- **Blast Radius:** Every consumer of the symbols the recommendation changes, each confirmed + safe. Write `None — change is local` only after actually searching. If the fix touches a + shared field or helper, list the call sites (including tests and evidence/logging paths). +- **Recommendation:** Fix with before/after sketch. If two invariants conflict here, name both, + name the test guarding each, and state what the over-correction would be. - **Effort:** S / M / L ### High Severity