From 19b98d7af7cfbf2bf02caecc7bb27596d8c52fbe Mon Sep 17 00:00:00 2001 From: leing2021 Date: Wed, 29 Jul 2026 20:57:14 +0800 Subject: [PATCH] feat(skills): add precision-over-recall review discipline and learn necessity gate - Add Precision Discipline section to rules/common/code-review.md (5 gate-keeping rules) - Add Precision gate subsection to 04-review SKILL.md - Add Necessity gate to 05-learn SKILL.md (decide FIRST whether to preserve) - Add rules/golang/review-checklist.md and rules/python/review-checklist.md with battle-tested defect patterns (absorbed from alibaba/open-code-review) - Update rules/README.md to document review-checklist.md file type - Bump version to 0.27.0 --- CHANGELOG.md | 6 +++ CHANGELOG_CN.md | 6 +++ package.json | 2 +- rules/README.md | 2 + rules/common/code-review.md | 17 ++++++++ rules/golang/review-checklist.md | 53 ++++++++++++++++++++++++ rules/python/review-checklist.md | 70 ++++++++++++++++++++++++++++++++ skills/04-review/SKILL.md | 9 ++++ skills/05-learn/SKILL.md | 23 +++++++++++ 9 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 rules/golang/review-checklist.md create mode 100644 rules/python/review-checklist.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b0c3ac..5421d7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +### 0.27.0 — precision-over-recall review discipline + learn necessity gate +- **Review philosophy** (`rules/common/code-review.md`, `04-review/SKILL.md`): added **Precision Discipline** — favor precision over recall, with 5 gate-keeping rules (verify before asserting, do not duplicate deterministic tools, stay silent when context is unclear, distinguish blocking vs non-blocking, no cargo-cult patterns). Inspired by [alibaba/open-code-review](https://github.com/alibaba/open-code-review)'s battle-tested design. +- **Review checklists** (new file type): `rules/golang/review-checklist.md` and `rules/python/review-checklist.md` hold precise, language-specific defect patterns with explicit "do not report" boundaries. Distinct from `patterns.md` (reusable design patterns). `rules/README.md` updated to document the new file type. +- **Learn necessity gate** (`05-learn/SKILL.md`): added **Necessity gate** as the first decision — most solved problems are NOT worth a solution artifact. Worth-preserving criteria (non-trivial + reusable + not already documented) and disqualifying red lines (one-off, common knowledge, already captured, trivial refactor, no root-cause insight). "Silence is acceptable; noise is not." +- 204 tests passing, 0 regressions. + ### 0.26.0 — absorb Matt Pocock skills methodology + external-resource verification gates - **Skill methodology absorbed**: Evaluated [Matt Pocock skills](https://github.com/mattpocock/skills) against super-pi's self-contained + artifact-driven constraints via a four-filter test (self-contained / artifact-driven / SKILL.md line-budget / testability). Six concepts passed, three deferred (wayfinder, tracer-bullet tickets, triage — all depend on issue trackers). - **New shared references** (all self-contained, no external path deps): diff --git a/CHANGELOG_CN.md b/CHANGELOG_CN.md index 8162a06..5acbd79 100644 --- a/CHANGELOG_CN.md +++ b/CHANGELOG_CN.md @@ -1,5 +1,11 @@ # 更新日志 +### 0.27.0 — precision-over-recall 审查纪律 + learn 必要性门禁 +- **审查哲学**（`rules/common/code-review.md`、`04-review/SKILL.md`）：新增 **Precision Discipline** 节——精度优先于召回，含 5 条门禁规则（先验证再断言、不重复确定性工具已覆盖的问题、上下文不清时保持沉默、区分阻断与非阻断、不盲从最佳实践）。灵感来自 [alibaba/open-code-review](https://github.com/alibaba/open-code-review) 经实战验证的设计。 +- **审查清单**（新文件类型）：`rules/golang/review-checklist.md` 和 `rules/python/review-checklist.md` 收录精确的语言专属缺陷模式，每条均附明确的“不报”边界。与 `patterns.md`（可复用设计模式）职责分离。`rules/README.md` 同步更新文件类型说明。 +- **Learn 必要性门禁**（`05-learn/SKILL.md`）：新增 **Necessity gate** 作为第一道决策——多数已解决的问题不值得沉淀为 artifact。值得沉淀的标准（非平凡 + 可复用 + 未已在别处记录）与一票否决红线（一次性问题、常识、已记录、琐碎重构、无根因洞见）。“沉默可接受，噪音不行。” +- 204 测试通过，0 回归。 + ### 0.26.0 — 吸纳 Matt Pocock skills 方法论 + 外部资源核实门禁 - **方法论吸纳**：以四过滤器（自包含 / artifact-driven / SKILL.md 行数预算 / 可测试性）评估 [Matt Pocock skills](https://github.com/mattpocock/skills)，6 项通过、3 项排除（wayfinder、tracer-bullet ticket 化、triage——均依赖 issue tracker）。 - **新增共享 references**（均自包含，无外部路径依赖）： diff --git a/package.json b/package.json index 551aee3..2a8532d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@leing2021/super-pi", - "version": "0.26.0", + "version": "0.27.0", "private": false, "type": "module", "description": "Pi-native Compound Engineering package for iterative development workflows", diff --git a/rules/README.md b/rules/README.md index f6d9c64..6e89d71 100644 --- a/rules/README.md +++ b/rules/README.md @@ -24,6 +24,7 @@ rules/ - **common/** contains universal principles — no language-specific code examples. - **Language directories** extend the common rules with framework-specific patterns, tools, and code examples. Each file references its common counterpart. +- **`review-checklist.md`** (optional, per language) holds precise, actionable defect patterns for code review — distinct from `patterns.md` which holds reusable design patterns. Currently used by `golang/` and `python/`. ## Installation @@ -83,6 +84,7 @@ To add support for a new language (e.g., `rust/`): - `patterns.md` — language-specific design patterns - `hooks.md` — PostToolUse hooks for formatters, linters, type checkers - `security.md` — secret management, security scanning tools + - `review-checklist.md` — *(optional)* precise defect patterns for code review, used together with `common/code-review.md` 3. Each file should start with: ``` > This file extends [common/xxx.md](../common/xxx.md) with specific content. diff --git a/rules/common/code-review.md b/rules/common/code-review.md index d79ba9b..4c074df 100644 --- a/rules/common/code-review.md +++ b/rules/common/code-review.md @@ -4,6 +4,22 @@ Code review ensures quality, security, and maintainability before code is merged. This rule defines when and how to conduct code reviews. +## Precision Discipline + +**Favor precision over recall.** Report only defects you are confident are real in the changed code and its reachable context. A false positive costs more reviewer trust than a missed minor issue. + +### Gate-keeping rules + +1. **Verify before asserting** — Before flagging a non-local claim (race condition, security boundary, resource leak), use `file_read` and `code_search` to establish call sites, ownership, synchronization, and input boundaries. Do not infer concurrent invocation, attacker control, or error contracts from function names or package imports alone. + +2. **Do not duplicate deterministic tools** — Do not flag issues that the language compiler, formatter, linter, or type checker already catches reliably, unless the diff shows a concrete user-visible consequence those tools will not express. + +3. **Stay silent when context is unclear** — If the surrounding code, ownership, or data flow cannot be determined from available evidence, do not guess. A miss on an unclear code path is acceptable; a false alarm on it damages trust. + +4. **Distinguish blocking vs non-blocking** — Treat correctness and security findings as blocking (CRITICAL/HIGH). Style-only, idiom, or naming suggestions are non-blocking (LOW). Explicitly label each finding's severity. + +5. **No cargo-cult patterns** — Do not report a pattern just because it is "best practice." Evaluate whether the pattern applies to THIS codebase, THIS data flow, and THIS risk profile. If the code deviates from convention for a deliberate reason visible in context, accept it. + ## When to Review **MANDATORY review triggers:** @@ -85,6 +101,7 @@ Use these agents for code review: ### Security +- **Precision gate:** Before reporting any security issue, confirm the data source and attack surface with `code_search`. A pattern that looks like SQL injection but uses parameterized queries is not a finding. - Hardcoded credentials (API keys, passwords, tokens) - SQL injection (string concatenation in queries) - XSS vulnerabilities (unescaped user input) diff --git a/rules/golang/review-checklist.md b/rules/golang/review-checklist.md new file mode 100644 index 0000000..521f517 --- /dev/null +++ b/rules/golang/review-checklist.md @@ -0,0 +1,53 @@ +--- +paths: + - "**/*.go" + - "**/go.mod" + - "**/go.sum" +--- +# Go Review Checklist + +> Go-specific defect patterns for code review. Used together with [common/code-review.md](../common/code-review.md). +> **Precision over recall:** report only defects you are confident are real in the changed code. Verify claims with `file_read` and `code_search` before reporting. + +## Errors, Panics, and API Contracts + +- Errors returned from calls that are ignored, overwritten, or converted into success/default values that hide a failure. A deliberately best-effort operation is acceptable only when the ignored failure is safe and evident from context. +- Error wrapping that loses the original cause (`fmt.Errorf("...: %v", err)` when callers need `errors.Is`/`errors.As`), wraps nil, returns a misleading sentinel, or exposes internal details at a public boundary. Prefer `%w` when preserving identity. +- `panic`, `log.Fatal`, `os.Exit` in request, worker, library, or cleanup paths where a recoverable error can be returned. Do not flag impossible internal invariants or documented programmer contracts. +- Deferred cleanup that overwrites a primary error, drops a meaningful `Close`/`Commit`/`Rollback` error, or returns success after cleanup makes the result invalid. + +## Nil, Interfaces, and Value Semantics + +- A typed nil pointer, map, slice, function, channel, or error stored in a non-nil interface and later treated as absent. Check the concrete assignment and all interface checks first. +- Nil maps written to, nil channels used unintentionally (block forever), or nil pointers dereferenced on paths inputs or constructors can actually produce. +- Copying a value after first use when it contains `sync.Mutex`, `sync.RWMutex`, `sync.Once`, `sync.Pool`, `atomic` state, or another non-copyable synchronization primitive. Flag through value receivers, assignment, return, append, map values only when the value can have been used first. +- Value receivers or copies that silently mutate only a copy when callers expect shared state, especially for structs holding maps, slices, pointers, locks, or atomic state. +- `sync.Once` used for work that must retry after failure. `Once.Do` considers its func complete even if it panics; a captured error does not make a later `Do` retry. + +## Context, Goroutines, and Cancellation + +- Request-scoped work started with `context.Background()`/`TODO()` when it should inherit the caller's deadline, cancellation, values, or tracing. Independent background work is valid. +- `context.Context` stored in a struct or replaced with a custom context interface when it should be passed explicitly as the first parameter. +- `context.WithCancel`, `WithTimeout`, `WithDeadline` whose cancel function is not called once the derived context is no longer needed. +- Blocking I/O, waits, retries, selects, or loops on a request/worker path that lack cancellation or deadline where the dependency can stall. +- Goroutines that can outlive their owner (wait forever on a channel, lock, I/O, unbounded retry), lack shutdown, or have no way for errors/completion to be observed when that matters. +- Fire-and-forget goroutines that capture request-local mutable data, write to a response after handler returns, panic without recovery at a process boundary, or race with cleanup. +- Loop-variable or mutable outer-variable captures in goroutines/callbacks where a closure can observe a later value. Verify the module's `go` directive (Go 1.22 changed range-loop semantics). + +## Channels, Locks, and Shared State + +Only report races or deadlocks with evidence that state is reachable concurrently; inspect surrounding call sites. Do not flag immutable data, per-goroutine locals, or synchronization guaranteed by ownership. + +- Unsynchronized concurrent reads/writes of maps, slices, pointers, counters, caches, or compound state; check-then-act sequences that can interleave. +- Holding a mutex/RWMutex across blocking I/O, channel operations, callbacks, network calls, or long CPU work when another path needs the lock to progress. +- `RLock` used while mutating protected data; unlocked mutation of a field whose peers protect it; atomic and non-atomic access mixed for the same state. +- Sends/receives that can block indefinitely because a peer may stop, a buffer may fill, or shutdown/cancellation is not selected. +- Multiple possible channel closers, send-on-closed-channel risk, or double-close. +- `select` defaults that busy-spin, drop required work, or bypass cancellation; unbounded retries without backoff/cancellation. +- WaitGroups with `Add` racing with `Wait`, missing `Done`, or concurrent Add after Wait begins. + +## Not for this rule + +- Do not report issues that `go vet`, Staticcheck, `go test -race`, the compiler, or `gofmt` already catch reliably. +- Do not infer concurrent invocation, attacker control, resource ownership, or error contracts solely from function names or package imports. +- Do not flag intentional immutable value objects, single-threaded locals, or documented ownership patterns. diff --git a/rules/python/review-checklist.md b/rules/python/review-checklist.md new file mode 100644 index 0000000..a7876fc --- /dev/null +++ b/rules/python/review-checklist.md @@ -0,0 +1,70 @@ +--- +paths: + - "**/*.py" + - "**/*.pyi" +--- +# Python Review Checklist + +> Python-specific defect patterns for code review. Used together with [common/code-review.md](../common/code-review.md). +> **Precision over recall:** only raise an issue when confident it is a real defect. Stay silent when surrounding context is unclear. Treat security/correctness as blocking; style/idiom as non-blocking. + +## Mutable Default Arguments and Shared State + +- Mutable default arguments (`def f(x=[])` or `def f(x={})`): the default is created once and shared across every call. Default to `None` and build inside the body. +- Class-level mutable attributes shared unintentionally across instances when a per-instance value was intended. +- Module-level mutable globals (lists, dicts, caches) mutated across requests or threads, retaining state that surprises the caller. +- Closures that capture a loop variable by reference and all end up seeing its final value. +- Do not report when the function never mutates the argument, or the shared default is a deliberate documented cache or sentinel. + +## Boundary and Edge-Case Handling + +- Empty inputs assumed non-empty: indexing `xs[0]`, `max()`/`min()`, slicing without first handling empty `list`, `str`, `dict`, or iterator. +- Off-by-one and out-of-range access on indices, ranges, or slices, especially at first/last element. +- `None` reaching code that assumes a value, when an upstream call or default can legitimately return `None`. Confirm the data source with `file_read` before flagging. +- Comparing floats for exact equality with `==`; use `math.isclose` or explicit tolerance. +- Integer/float division assumptions: unintended truncation with `//`, or `ZeroDivisionError` when a divisor can be zero. +- Dictionary access by key without handling the missing-key case (`d[k]` vs `d.get(k)`). +- Do not report edge cases that a caller or type contract has already ruled out, or inputs that cannot occur given validated boundaries upstream. + +## Error Handling and Exceptions + +- Bare `except:` swallows everything including `KeyboardInterrupt` and `SystemExit`; catch `except Exception` at minimum. +- `except Exception` broader than the failure being handled; narrow it to the specific exception types expected. +- Exceptions caught and silently discarded (`pass`) without logging or re-raising. +- Original traceback lost when re-raising; prefer `raise NewError(...) from err` to preserve the cause. +- Broad `try` blocks that wrap far more than the line that can actually fail, hiding where the error originates. +- `assert` used for runtime validation of external input — assertions are stripped under `python -O`. + +## Identity and Equality Comparisons + +- Using `is`/`is not` to compare against literals (strings, numbers, tuples); this relies on implementation-specific interning rather than value equality — use `==`. +- Comparing against `True`/`False` with `==`, where a truthy-but-not-`True` value (e.g. `1`) would compare unequal; prefer plain truthiness check. +- Comparing against `None` with `==`/`!=` rather than `is`/`is not` — minor style preference. + +## Resource Management + +- Files, sockets, locks, or DB connections opened without a `with` statement, risking leaks on early return or exception. +- Resources acquired in a `try` whose `finally` cleanup is missing or incomplete on the error path. +- Do not report short-lived scripts, or handles already managed by an enclosing `with` or framework. + +## Performance + +Confirm data scale and hot path before flagging: +- Building strings with `+=` in a loop instead of accumulating in a list and `"".join(...)`. +- Repeated membership tests against a `list` where a `set` or `dict` would turn O(n) into O(1). +- Building a full list when a generator would avoid holding everything in memory. +- Passing eagerly formatted f-string to `logging` (`logging.info(f"...")`) instead of `logging.info("%s", value)`, which defeats lazy formatting. + +## Concurrency + +Only flag when there is evidence of multi-threaded or async invocation (confirm with `code_search`): +- CPU-bound work parallelized with `threading` under the GIL where `multiprocessing` is the right tool. +- Check-then-act races on shared state without a `Lock`, or non-atomic compound operations. +- Do not report single-threaded locals, immutable data, or framework-managed async contexts. + +## Not for this rule + +- Do not report spelling errors at reference sites (determined by the declaration). +- Do not report dead code that is intentionally preserved for documentation or future use. +- Do not report resource management issues in short-lived scripts. +- Do not flag style preferences that the project's formatter (black, ruff) already enforces. diff --git a/skills/04-review/SKILL.md b/skills/04-review/SKILL.md index 945abce..4422b2d 100644 --- a/skills/04-review/SKILL.md +++ b/skills/04-review/SKILL.md @@ -38,6 +38,15 @@ Code review is **technical evaluation**, not social performance: - **Evidence before assertions:** cite specific code, not principles - **Architecture axis:** audit module depth and seams using `../references/module-design.md` +### Precision gate + +**Favor precision over recall.** A false positive costs more trust than a missed minor issue. +- Before reporting a non-local claim (race condition, security boundary, resource leak), use `file_read` and `code_search` to confirm evidence. Do not infer from names alone. +- Stay silent when the surrounding context is unclear. A miss on ambiguous code is acceptable; a false alarm is not. +- Do not flag issues that a compiler, formatter, linter, or type checker already catches, unless the diff shows a concrete user-visible consequence those tools miss. +- Label each finding with severity. Blocking (CRITICAL/HIGH) for correctness and security; non-blocking (LOW) for style and naming. +- Apply language-specific rules from `rules/{lang}/review-checklist.md` — they contain precise, actionable defect patterns per language. + ## Handling findings 1. **Read** — complete all findings without reacting diff --git a/skills/05-learn/SKILL.md b/skills/05-learn/SKILL.md index 8bc2015..d0c5e69 100644 --- a/skills/05-learn/SKILL.md +++ b/skills/05-learn/SKILL.md @@ -9,6 +9,29 @@ Use this skill after solving a problem so the repository gains a reusable learni See [shared pipeline instructions](../references/pipeline-config.md) for model routing and pipeline behavior. +## Necessity gate (decide FIRST) + +**Before writing anything, decide whether this learning is worth preserving.** Most solved problems are NOT worth a solution artifact. Silence is acceptable; noise is not. + +### Worth preserving (ALL must hold) + +1. **Non-trivial** — the solution required real investigation, not a one-glance fix. +2. **Reusable** — the root cause or fix pattern could recur in this or another project. +3. **Not already documented** — the knowledge is not trivially findable in framework docs, the codebase, a prior solution artifact, or a commit message. + +### Not worth preserving (any ONE is disqualifying) + +- **One-off** — a typo, a rename, a personal-environment quirk unlikely to recur. +- **Common knowledge** — standard framework usage, language basics, or anything a competent practitioner would know or find in official docs in under a minute. +- **Already captured** — the learning is fully expressed in the code, its tests, a commit message, or an existing `docs/solutions/` artifact. +- **Trivial refactor** — formatting, import sorting, or mechanical changes with no insight. +- **No root cause insight** — the fix worked but you cannot explain *why* it worked; without the "why", the artifact will not help future readers. + +### Outcome + +- If **not worth preserving**: respond concisely (e.g. "No solution artifact needed: ") and stop. Do not create a file. +- If **worth preserving**: proceed to Core rules below. + ## Core rules - Every solution MUST include YAML frontmatter per `references/solution-schema.yaml` (title, category, severity, tags, applies_when).