Harden python-code-reviewer skill with lessons from executing its own review

Three gaps surfaced by implementing the 2026-08-23 review's recommendations.

1. Recommendations were never verified the way claims were. The report's fix for
   the error path leak would have stripped root-cause data from evidence records,
   because the review traced one consumer of AppError.message and missed that
   format_error_detail writes it to ExecutionAttempt.error_detail. Adds workflow
   step 9 (validate recommendations against consumers), a Blast Radius field on
   findings, and the worked example so the failure mode is concrete.

2. Fixes that sit between competing invariants were not flagged. The atomicity
   recommendation did not note that per-page durability and terminal-status
   atomicity pull in opposite directions, so the obvious simplification silently
   breaks multi-page durability. Recommendations must now name both invariants,
   the test guarding each, and the over-correction to avoid.

3. Severity could not express reachability. Two findings were latent behind a
   default setting and a single-instance deployment, which is a sequencing
   constraint: they must be fixed before the change that makes them live. Adds an
   explicit Reachability field with Live / Latent / Theoretical.

Verified: meta contract guards and traceability tests pass.

Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
Jim Lancaster
2026-08-23 19:14:35 -05:00
co-authored by Copilot App
parent 12f125761a
commit 626b5d4b10
+52 -6
View File
@@ -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/<YYYY-MM-DD>-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