Skip to content

Three Python skills: best-practices v1.5.0, async-best-practices v1.0.0, pytest v1.0.0 + claim-proof harness - #3

Merged
nathan-gage merged 9 commits into
mainfrom
review-skills
Aug 20, 2026
Merged

nathan-gage merged 9 commits into
mainfrom
review-skills

Conversation

@nathan-gage

@nathan-gage nathan-gage commented Aug 20, 2026 •

Copy link
Copy Markdown
Owner

Splits the repo into three delineated skills, adds executable claim verification (runtime + typing-checker lanes) with CI, and lands two review cycles of semantic and architectural fixes.

Skills

Skill Version Rules Sections
python-best-practices 1.5.0 75 8
python-async-best-practices 1.0.0 5 1
python-pytest 1.0.0 15 5

python-best-practices 1.3.0 → 1.5.0: five new rules (types-modern-syntax, types-sequence-over-list-params, data-reject-bool-as-int, error-match-types-not-messages, imports-lightweight-init) plus accuracy fixes throughout: cached_property lock semantics by version, lru_cache retention, TYPE_CHECKING runtime-annotation consumers, duplicate-key policy in dict indexing, optional-import scoping via ModuleNotFoundError.name, boundary-validated TypedDict narrowing, shallow-vs-transitive immutability, instants vs civil time, and function/dataclass/Pydantic mutable-default behaviors separated.

python-async-best-practices (new): event-loop discipline grounded against pydantic-ai's async machinery and proven against CPython — async-no-blocking-event-loop, async-own-your-tasks (owner-cancellation-safe shutdown drain via owner.cancelling()), async-bound-concurrency (in-flight vs admission), async-generator-cleanup, async-preserve-cancellation (absorbed cancellation reports success and silently defeats asyncio.timeout — both proven).

python-pytest (new): test value (observable contracts, independent oracles), determinism (events not sleeps, no flake masking, strict xfail), fixtures, mocking (patch-where-used, wire-format assertions), structure (import identities incl. the lone-tests/__init__.py collision, proven; plugin hygiene scoped to observed drift).

Judgment architecture (from review)

  • Every rule carries a structural counter-signal: a standalone paragraph opening with an approved marker (**When ...** / **Scope:** / **Preserve ...** / **Exception ...** / ...) saying when NOT to apply the rule. validate.py enforces presence on all impacts; detection is positional, never natural-language classification (unit-tested: an affirmative thesis containing "keep" is not extracted; code blocks never match).
  • test-cases.json exports counter_signals per case and declares the scoring contract: condition-aware application and restraint, not similarity to the Correct block.
  • All three SKILL.md maps state: quick-reference lines are triggers, not licenses — open the rule and check its counter-signal before applying.

Executable claim verification (proofs/, not vendored)

  • Runtime lane: 39 proofs, green on CPython 3.11.14 / 3.12.12 / 3.13.8 / 3.14.3 (make -C proofs). Covers version splits (cached_property.lock, utcnow deprecation, executor cpu_count→process_cpu_count) and counterintuitive semantics (gather orphans, absorbed cancellation false-success, timeout silently lost, owner-cancellation delegation during shutdown drain, lru_cache retention, import-identity collisions).
  • Typing lane: fixtures run through mypy, pyright, and ty with per-checker expected verdicts (invariance, Sequence covariance, assert_never exhaustiveness, cast unchecked) — verdicts observed before being encoded.
  • CI: validators + generated-output drift check, the 4-version runtime matrix, and the typing lane run per PR.

Notes

  • Rule substance is SDK-agnostic; examples use invented domains.
  • Tooling standardized on uv run and make.

…(v1.0.0), claim-proof harness

python-best-practices 1.3.0 -> 1.4.0 (79 rules, 9 sections):
- new Concurrency & Async section: no-blocking-event-loop, own-your-tasks,
  bound-concurrency, generator-cleanup (grounded against pydantic-ai's async
  machinery and tests)
- new rules: types-modern-syntax, types-sequence-over-list-params,
  data-reject-bool-as-int, error-match-types-not-messages,
  imports-lightweight-init
- accuracy fixes: cached_property lock semantics by version, lru_cache
  instance retention, TYPE_CHECKING runtime-annotation caveat, gather
  return_exceptions typing, httpx client lifetime, datetime.UTC,
  api-required-before-optional impact recalibration

python-pytest 1.0.0 (15 rules, 5 sections): test value, determinism,
fixtures, mocking, structure — observational voice, same build pipeline.

proofs/: uv + pytest + Makefile harness; 29 executable proofs for
version-sensitive claims, green on CPython 3.11-3.14. Docs standardized
on uv run / make.
python-best-practices 1.4.0 -> 1.5.0: back to 75 rules / 8 sections; async
guidance moved out, description points at the companion skill. Cancellation
semantics for except clauses stay in error-specific-exceptions.

python-async-best-practices 1.0.0: no-blocking-event-loop, own-your-tasks,
bound-concurrency, generator-cleanup — same build pipeline, independently
vendorable. Cross-skill references qualified by skill name; proofs citations
updated.
…actices (5 rules)

error-specific-exceptions keeps broad-catch hygiene plus a one-paragraph
cancellation-safety note pointing at the async skill; the asyncio depth
(gather return_exceptions handling, shielded cleanup, framework MRO caveat)
now lives in async-preserve-cancellation with a full incorrect/correct pair.
Cancellation proofs relocated to test_async_claims.py and extended with the
isinstance(r, Exception)-misses-CancelledError trap.
…semantics

Absorbed cancellation does not hang the canceller — it reports success:
task.cancelled() is False and a surrounding asyncio.timeout expires without
raising, silently losing the deadline. Both premises now have executable
proofs (31 proofs green on 3.11-3.14). Inline await asyncio.shield() removed
from the correct example: it is not a must-complete guarantee (the awaiting
line still raises; detached work needs an owner) — cleanup is brief-then-raise,
with owned-task shield/drain reserved for genuinely non-interruptible cleanup.
@nathan-gage nathan-gage changed the title Async section + accuracy fixes (v1.4.0), python-pytest skill (v1.0.0), claim-proof harness Three Python skills: best-practices v1.5.0, async-best-practices v1.0.0, pytest v1.0.0 + claim-proof harness Aug 20, 2026
…ors and proof matrix

- error-trust-validated-state: frozen=True is shallow — example now uses
  tuple[Item, ...]; 'was validated once is not remains valid'
- types-fix-types-not-cast: Correct example now validates at the parse
  boundary instead of annotating unvalidated json.loads output
- async-own-your-tasks: shutdown drain now observes pre-cancellation
  failures (await + cancelled() check) instead of discarding them via
  gather(return_exceptions=True); TaskGroup fate-coupling stated
- async-bound-concurrency: in-flight vs admission bounding distinguished
  (worker pool / bounded queue / windowing for large or streaming inputs);
  bound validated >= 1
- perf-dict-index-over-nested-loops: duplicate-key policy — scan is
  first-match, plain dict comprehension is last-wins; example uses
  setdefault and states the policy
- imports-optional-dependencies: guard moved to the integration-specific
  module; ModuleNotFoundError.name distinguishes missing from broken
- perf-lru-cache-pure-fns: stale-file example replaced with a pure one;
  key-completeness, result-ownership, and concurrent-miss caveats
- error-consolidate-try-except: scope-widening caveat + shared-handler
  helper alternative for distinct-stage diagnostics
- structure-unique-module-names: lone tests/__init__.py does not
  disambiguate (proven: both trees become tests.test_utils); options are
  unique filenames, importlib mode, or full package chains
- light reframes: mutation APIs returning new information are fine;
  keyword-only name-surface trade + positional-only; instants vs civil
  time; transitive immutability for safe defaults; pytest test-level,
  named smoke contracts, and explicit quarantine carve-outs
- proofs: +1 (package-identity nuance); 32 green on 3.11-3.14
- .github/workflows/ci.yml: skill validation + generated-output drift
  check + proof matrix as required evidence
@nathan-gage

Copy link
Copy Markdown
Owner Author

Thanks — this is a strong review. c5af5f0 addresses the semantic blockers; the architecture items are queued for a maintainer decision. Disposition:

Fixed in c5af5f0 (accepted as-is):

  • Calibrate python-best-practices to v1.3.0 (70 rules) #2 shallow frozen: example is now tuple[Item, ...] with "frozen=True stops field reassignment, not graph mutation" spelled out; closing line is your framing — was validated once ≠ remains valid.
  • Three Python skills: best-practices v1.5.0, async-best-practices v1.0.0, pytest v1.0.0 + claim-proof harness #3 unvalidated TypedDict: the Correct example now performs real checks at the parse boundary and casts once there, with prose explaining why the bare annotation merely relocates the assertion (adapter/model mentioned as the library form, kept SDK-agnostic per repo policy).
  • #4 drain suppression: shutdown now awaits the cancelled task so pre-cancellation failures propagate, suppressing only the task's own cancellation (task.cancelled() check); gather(return_exceptions=True)-and-ignore is explicitly called out as discarding failures. TaskGroup fate-coupling vs independent per-item accounting stated.
  • #5 admission vs in-flight: new paragraph distinguishes bounding work in progress from bounding task population/results (worker pool + bounded queue, windowing, yielding iterator); bound validated >= 1.
  • #6 duplicate policy: example now preserves the scan's first-match semantics via setdefault and the rule requires stating a duplicate policy; the 50×50 number demoted to an explicitly movable heuristic.
  • #7 optional imports: guard relocated to the integration-specific module (shared modules must not import the dep — cross-linked to imports-lightweight-init); ModuleNotFoundError.name distinguishes missing from broken.
  • #8 stale cache: file-reading example replaced with a pure derivation; key-completeness, result-ownership (shared instance), and concurrent-miss caveats added.
  • #9 handler scope: scope-widening caveat + the shared-_config_warning-helper alternative for distinct-stage diagnostics; new keep-separate bullet for broad exception types.
  • #10 package identity: you were right that my __init__.py fix was itself wrong — a lone tests/__init__.py in both trees still collides as tests.test_utils. Rewritten (unique filenames primary; importlib recommended with its test-to-test import caveat; full package chains as the third option) and added as executable proof test_tests_package_init_alone_does_not_disambiguate, green on 3.11–3.14.
  • Reframes: mutation APIs returning new information (pop, setdefault) are fine — ambiguity is the target; keyword-only "non-negotiable" replaced with the name-surface trade + positional-only; instants vs civil time added to data-aware-datetimes; transitive immutability for safe defaults; pytest: test-level scoping on mock-stable-boundaries, named smoke contracts as a deliberate exception, explicit quarantine lanes as triage-not-masking.
  • Criterion 7 (CI): .github/workflows/ci.yml now runs all three validators, fails on generated-output drift, and runs the proof matrix on 3.11–3.14 per PR — the matrix is no longer author-reported.

Queued for maintainer decision (will follow up on this PR): the template/eval architecture (#1), claim_type/applicability metadata, the opt-in style-pack split, and the evidence taxonomy. These trade against the repo's stated philosophy (examples-over-prose, 20–40-line rules, no when-X/when-Y taxonomies), so they're a deliberate design call rather than a fix — the current mitigation is that counter-signal/"preserve this" prose already exists in most MEDIUM+ rules and was extended throughout this pass.

Current state: 95 rules across three skills, all validators at 0 failures, 32 proofs × {3.11.14, 3.12.12, 3.13.8, 3.14.3} green.

…idence lane

Architecture decisions from PR review, as resolved by maintainer:

- validate.py now requires a counter-signal passage (when NOT to apply /
  what to preserve) on every MEDIUM+ rule; 14 rules gained explicit
  counter-signals, including the review's flatness (api-model-cohesion),
  open-union (assert_never), non-strict-xfail-matrix, and clock-injection
  carve-outs. Template stays lean per repo philosophy.
- extract_tests.py exports counter_signals per case and the payload
  declares the scoring contract: condition-aware application and
  preservation, not similarity to the Correct block.
- proofs/typing_tests/: checker-fixture lane running mypy, pyright, and
  ty with per-checker expected verdicts (invariance, Sequence covariance,
  assert_never exhaustiveness, cast unchecked) — verdicts observed before
  encoding. Wired into make and CI.
- Style rules stay in place (impact levels communicate priority).

3 validators at 0 failures; 32 runtime proofs x {3.11..3.14} and 12
typing verdicts green.
@nathan-gage

Copy link
Copy Markdown
Owner Author

Follow-up on the queued architecture items — maintainer decisions landed in d5d94d3:

  • Template (Tighten rules and add Vercel-style validate/extract pipeline #1): kept lean per repo philosophy, but the substance is now enforced: validate.py fails any MEDIUM+ rule lacking a counter-signal passage (when NOT to apply / what to preserve). 14 rules gained explicit counter-signals in the pass, including your flatness (api-model-cohesion), open-union (assert_never), non-strict-xfail matrix, and clock-injection carve-outs.
  • Evals: extract_tests.py now exports counter_signals per case, and the payload's scoring field states the contract — condition-aware application and preservation, not similarity to the Correct block.
  • Evidence taxonomy (criterion 6): typing-behavior claims now have an executable lane — proofs/typing_tests/ runs mypy, pyright, and ty over fixtures with per-checker expected verdicts (invariance, Sequence covariance, assert_never exhaustiveness, cast unchecked); verdicts were observed before being encoded. Wired into make -C proofs and CI alongside the runtime matrix.
  • Style-pack split: declined — impact levels already communicate priority, and section framing marks those rules opportunistic-only.

State: 3 validators at 0 failures with enforcement active; 32 runtime proofs × {3.11.14, 3.12.12, 3.13.8, 3.14.3} and 12 typing verdicts green; CI runs all of it per PR.

…make counter-signals structural

Re-review blockers:
- async-own-your-tasks: task.cancelled() cannot identify whose CancelledError
  was caught (owner cancellation delegates to the awaited child) — the aclose
  example now consults owner.cancelling() first. Two regression proofs:
  owner-cancellation-during-drain propagates; pre-cancellation failure
  surfaces through the drain.
- data-mutable-defaults: function / dataclass / Pydantic-v2 behaviors
  separated (shared object / ValueError / per-instance deep copy) — Pydantic
  model defaults are no longer flagged as the function-argument bug.
- counter-signals are now structural: detection requires a standalone
  paragraph opening with an approved marker (When/Scope/Preserve/Exception/
  Keep/Caveat/Don't/Watch); fuzzy inline phrase matching removed; requirement
  extended to ALL impacts; 40 rules gained marker-opened counter-signal
  paragraphs (mostly promoting existing prose); detector unit tests added
  (thesis 'Keep the old name' is not extracted; code blocks never match).
- _template.md documents the required counter-signal; SKILL.md maps carry
  'triggers, not licenses — check the rule's counter-signal' in all three
  skills; data-mutation-contract quick-ref and body allow new-info returns
  and explicit fluent builders; empty_parameter_set_mark scoped to suites
  where nonempty is contractual.
- Smaller re-review items: trust-validated comment precision, config example
  checks dict shape and rejects bool-as-int via _is_int helper.

3 validators x 0 failures; 39 runtime proofs x {3.11..3.14}; 12 typing
verdicts; detector unit tests green.
@nathan-gage

Copy link
Copy Markdown
Owner Author

Re-review blockers addressed in ed49c23:

  1. Owner-cancellation bug (async-own-your-tasks): confirmed — task.cancelled() can't identify whose CancelledError was caught because cancelling the owner mid-await delegates the cancel to the child. The example now checks owner.cancelling() first (3.11+ baseline), and your exact reproduction is a regression proof: pump whose cancellation cleanup parks on an await → aclose() task cancelled mid-drain → caller cancellation propagates (closer.cancelled() asserted) even though the pump also ends cancelled(). A second proof pins the pre-cancellation-failure passthrough.
  2. Mutable defaults: the three behaviors are now separated — plain def shares the object (the bug), @dataclass rejects with ValueError, Pydantic v2 deep-copies per instance — with an explicit "don't flag model defaults as the function-argument bug" counter-signal. The conflicting "same trap applies" sentence is gone.
  3. Counter-signals structural: fuzzy phrase matching removed. Detection now requires a standalone paragraph opening with an approved marker; your api-deprecated-aliases false positive is a detector unit test (affirmative thesis containing "Keep the old name" is not extracted; code blocks never match; mid-paragraph markers don't count). The requirement extends to all impacts per your suggestion — 40 rules gained marker-opened paragraphs, almost all by promoting existing prose. _template.md now documents the requirement (the conflicting taxonomy advice is reworded), and all three SKILL.md maps carry the "triggers, not licenses — check the counter-signal" instruction.

Smaller items: trust-validated comment precision, dict-shape + bool-rejecting _is_int in the config example, fluent-builder third contract in data-mutation-contract (quick-ref softened to match), empty_parameter_set_mark scoped to suites where nonempty is contractual, and the PR description refreshed (39 runtime proofs × 3.11–3.14, 12 typing verdicts, CI lanes).

State: 3 validators × 0 failures under structural enforcement; 39 runtime proofs × {3.11.14, 3.12.12, 3.13.8, 3.14.3}; 12 checker verdicts; detector unit tests green.

…ment markers

Semantic audit of every counter_signals export across the three skills
(the surface the structural detector cannot judge): 93 are genuine
preserve/when-not passages. Two were a rule thesis wearing a marker —
exactly the false-positive class the re-review flagged:

- api-underscore-for-private exported the sibling rule's thesis (don't
  reach into _private) as its counter-signal; de-marked, and a real one
  added (string-dispatched names are public contracts; leaf application
  modules don't need exhaustive underscoring).
- error-validate-at-boundaries had its own thesis renamed to **Scope:**
  to satisfy the validator; restored to plain prose, with a real
  counter-signal on deliberate layered enforcement (DB constraints,
  domain constructors, security-sensitive ops).
…pace counter-signal label

- data-mutable-defaults: default_factory does not validate its output;
  point at Field(default_factory=..., validate_default=True) for defaults
  that must be validated (per reviewer note + Pydantic fields docs).
- api-instance-vs-module-fn: relabel counter-signal to 'When module scope
  is right:' so the exported eval intent reads as a condition, not
  reinforcement.
@nathan-gage
nathan-gage merged commit c5b961b into main Aug 20, 2026
6 checks passed
@nathan-gage
nathan-gage deleted the review-skills branch August 20, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant