diff --git a/docs/developer-guide/en/independent-review.md b/docs/developer-guide/en/independent-review.md new file mode 100644 index 000000000..32b5617da --- /dev/null +++ b/docs/developer-guide/en/independent-review.md @@ -0,0 +1,149 @@ +# Independent Review + +[中文版](../zh/independent-review.md) + +A recommended (optional, not a CI gate) checklist for reviewing anolisa code +changes — self-checking your own change before a PR, or reviewing someone +else's. It complements human CODEOWNERS review and CI; it does not replace them. + +## Core discipline + +The whole value is in reviewing **without contamination**: + +- Do it in a **fresh review pass that does not inherit the implementation + context** — a separate agent session (if you drive one) or a second reviewer. + It must not carry the author's intent, suspicions, or fix conclusions. +- **Review-only**: read the code, `git diff`, tests and in-repo docs. Do not edit + code while reviewing. +- **Zero-direction**: do not enter with "verify that known bug X is fixed". Look + for problems from scratch. +- If several reviewers/angles run, **do not cross-feed** one's conclusions into + another — it biases them. + +## When to run how much + +- **Simple change → one holistic pass.** Simple = single file / local, no + interface-contract change, not cross-component, no concurrency or lifecycle, not + on a hot path, narrow blast radius. +- **Risky change → a full set of review angles.** In anolisa, a change is **never + "simple"** if it touches any of: another component / crate, a public or FFI + interface, `src/*/probes/*.bpf.c`, sec-core sandbox/privilege, kernel / arch / + page-size code, or `*.spec.in` / `component.toml`. A small-but-dangerous + eBPF / FFI / sandbox diff is not simple. + +## Generic review axes (starting skeleton) + +architecture · correctness · stability · security · performance · holistic. + +Readability is **deprioritized** — clippy / rustfmt / ruff / eslint already gate +style in CI. Only raise what mechanical linters cannot see (naming, dead +abstraction, `mod.rs` layout per AGENTS.md). Apply the language-appropriate +conventions per changed file (Rust / TypeScript / Python / eBPF). + +## anolisa-specific review angles (mount by changed path) + +These are the classes CI structurally cannot catch. Treat as a checklist keyed +by what the diff touches, not as separate reviewer roles. + +1. **eBPF verifier & kernel safety** — `src/*/probes/*.bpf.c` has no automated + test gate, so review is the only net. Check: bounded loops; explicit size + clamps before `bpf_probe_read_*` (an unclamped size trips the verifier with + "R2 …negative"); no unbounded variable-length access; every producer + initializes newly added shared-header fields; stack under 512 B; per-CPU vs + shared-map concurrency. **State whether it was verifier-load-tested on a real + kernel** — an x86 CI load is not an aarch64 load. + +2. **Architecture / environment matrix** — kernel-parameter, `/proc`, `/sys` + code carries silent arch/kernel assumptions. Flag hardcoded page sizes (page + size is arch-dependent — use `sysconf(_SC_PAGESIZE)`), kernel-version gates, + container-vs-host `/proc` reads. Name which (arch × kernel × container) cells + go untested; recommend a run on the affected arch rather than asserting safety. + +3. **FFI / cbindgen ABI boundary** — anolisa crosses Rust ↔ C ↔ Python. Verify + the generated header == the Rust signature == the example in docs/headers; all + crossing types are `#[repr(C)]`; no panic unwinds across the FFI boundary + (`catch_unwind`); the cbindgen drift-guard was actually re-run, not assumed. + Header/example drift is silent and high-impact; the generic correctness axis + misses it. + +4. **Cross-component contract** — the seams between agentsight, cosh, + anolisa-cli, and the genai storage schema; SKILL.md auto-discovery; + `component.toml` fields; CLI/JSON shapes consumed across components. Trigger + when a diff changes a **producer without its consumer**; ask "does the other + side assume a stronger contract than this now provides?" + +5. **Agent security** — sec-core is a whole component, and agent workloads bring + specific threats: prompt injection, sandbox escape, deny-list bypass (a + code-execution deny-list must match on the resolved path, not the spelling), + command injection in generated shell, privilege escalation on root / sysctl + writes, secret leakage into logs or PR bodies. Give privileged-write and + untrusted-input paths a dedicated look. + +6. **Packaging / distribution correctness** — `*.spec.in` / `component.toml` / + manifests are not exercised by code-test CI. Check spec dependencies match + Cargo/npm dependencies, `requires_*` matches reality, versions are bumped + consistently, and there is no orphaned or missing registration. + +## Verification discipline (before claiming "verified") + +Hard-won on this repo's CI: + +- **Reproduce with the toolchain CI pins**, not just your local default — a lint + that passes on an older toolchain can fail on the pinned one. Run + `cargo + fmt --check`, `clippy --all-targets -- -D warnings`, `test`. +- **New logic goes in a lib crate, not a bin** — otherwise incremental + coverage reads 0 %. +- **commitlint**: `type(scope):` lowercase, scope in + `.github/commitlint.config.json`, header ≤ 120 chars, body wrapped ≤ 100. +- **rustfmt is a build dependency** (libbpf-cargo's skeleton builder), not only a + linter. +- **If it can be end-to-end tested, end-to-end test it.** Compiling or unit tests + alone is not "verified". In the PR body, split the testing status into + done-E2E / unit-only / not-verified — never a blanket "all passing". +- **Tests must discriminate**: reverting the fix should make the test fail + (prove it, e.g. by a quick mutation). Cover both directions. Beware + short-circuits (`mtime` checks, `Ok(false)`) that give false confidence. +- **Extract pure functions** so logic that doesn't need the kernel / root / I/O + is unit-testable off a privileged box. + +## Finding discipline (avoid false findings) + +- **"The code differs from what I expected" ≠ a bug.** First read `git blame` / + the commit message / surrounding design intent and answer "why is it this way". + Findings raised without checking design intent are frequently walked back. +- **Don't trust the PR's abstract terms — verify the real mechanism.** Confirm + the claimed threat model / dependency / mechanism ("an X injection", "uses + library Y") actually exists in the code — the real reader/parser may differ + from the description, and a fix built on the description may not match the real + implementation. Review to the code, not to the PR's wording. +- **Kernel / security bugs are not discounted by trigger frequency** — a + use-after-free, uninitialized read, info leak or race is a deterministic + property of the source; "not seen in a synthetic run" is about frequency, not + existence. Report it. +- **Adversarially verify a conclusion before acting on it** — especially before + opening an issue, so it isn't based on a stale reading of the code. +- **Withdraw a finding that doesn't hold up on verification** — don't keep it to + look thorough. +- **"No change needed" is a valid outcome** — if an audit finds nothing, say so + and close; don't manufacture changes to look busy. + +## When reviewing someone else's PR + +- **State the problem, don't prescribe the fix**: give the fact, the evidence + (`file:line`), and the impact — let the author choose how to address it. +- Read the author's design intent before raising a finding (same lesson as above). + +## Output contract + +- Findings first, ranked by severity, each with `file:line`. If clean, say so + explicitly and note residual risk. Don't lead with a long summary. +- **Out of scope**: do not re-report what clippy / rustfmt / ruff / eslint / + commitlint / coverage already gate — that is noise and erodes trust. Focus only + on what mechanical CI and a single reviewer cannot see. + +## Integration loop + +Dedupe all findings → fix the real ones first → re-run affected tests → if a fix +materially widens the diff, review again (at least holistic / correctness / +stability / performance) → report honestly: which tests ran, which angles were +reviewed, what was found, what was fixed, what residual risk remains. diff --git a/docs/developer-guide/zh/independent-review.md b/docs/developer-guide/zh/independent-review.md new file mode 100644 index 000000000..3c62c0b77 --- /dev/null +++ b/docs/developer-guide/zh/independent-review.md @@ -0,0 +1,76 @@ +# 独立 Review + +[English](../en/independent-review.md) + +一套推荐的(可选、非 CI gate)anolisa 代码审查 checklist——提 PR 前自检自己的改动,或 review 别人的。它补充人工 CODEOWNERS review 与 CI,不替代它们。 + +## 核心纪律 + +全部价值在于**无污染**地审查: + +- 在一个**不继承实现上下文的全新 review pass** 里做——一个独立的 agent session(如果你用),或第二位 reviewer。它不能带着作者的意图、怀疑点或修复结论。 +- **只读**:读代码、`git diff`、测试和仓内文档。review 时不改代码。 +- **无方向**:不要带着"验证已知 bug X 修好了"进入,从 0 找问题。 +- 如果跑多个 reviewer / 角度,**不要把一个的结论交叉喂给另一个**——会带偏。 + +## 何时跑多少 + +- **简单改动 → 一个 holistic pass。** 简单 = 单文件 / 局部、不改接口契约、不跨组件、不涉并发或生命周期、不在热点路径、影响面窄。 +- **风险改动 → 起一组多角度审查。** 在 anolisa 里,只要动到以下之一就**永远不算"简单"**:另一个组件 / crate、public 或 FFI 接口、`src/*/probes/*.bpf.c`、sec-core 沙箱 / 权限、内核 / arch / 页大小相关代码、`*.spec.in` / `component.toml`。一个小但危险的 eBPF / FFI / 沙箱 diff 不算简单。 + +## 通用审查 axis(起点骨架) + +architecture · correctness · stability · security · performance · holistic。 + +readability **降级**——clippy / rustfmt / ruff / eslint 已在 CI gate 风格。只报机械 linter 看不到的(命名、死抽象、`mod.rs` 布局,见 AGENTS.md)。按改动文件的语言用对应约定(Rust / TypeScript / Python / eBPF)。 + +## anolisa 特有审查角度(按改动路径挂载) + +这些是 CI 结构上抓不到的类别。当作一份按 diff 触及内容挂载的 checklist,不是一组独立 reviewer 角色。 + +1. **eBPF verifier 与内核安全** —— `src/*/probes/*.bpf.c` 没有自动测试门禁,review 是唯一的网。检查:bounded loop;`bpf_probe_read_*` 前显式 size clamp(size 不 clamp 会让 verifier 报 "R2 …negative");无 unbounded 变长访问;每个 producer 都初始化新增的共享 header 字段;stack < 512 B;per-CPU vs 共享 map 的并发。**说明有没有在真内核上 verifier-load 过**——x86 CI 的 load 不等于 aarch64 的 load。 + +2. **架构 / 环境矩阵** —— 内核参数、`/proc`、`/sys` 相关代码携带隐含的 arch/内核假设。排查硬编码页大小(页大小随 arch 不同——用 `sysconf(_SC_PAGESIZE)`)、内核版本 gate、容器 vs host 的 `/proc` 读取。点名 (arch × kernel × container) 哪些格子没测;建议在受影响 arch 上真跑,而不是断言安全。 + +3. **FFI / cbindgen ABI 边界** —— anolisa 跨 Rust ↔ C ↔ Python。核对生成的 header == Rust 签名 == docs/header 里的示例;所有跨界类型 `#[repr(C)]`;FFI 边界不 panic unwind(`catch_unwind`);cbindgen drift-guard **真跑过**、不是假设。header / 示例漂移是静默高危,通用 correctness axis 会漏。 + +4. **跨组件契约** —— agentsight、cosh、anolisa-cli、genai storage schema 之间的接缝;SKILL.md 自动发现;`component.toml` 字段;跨组件消费的 CLI / JSON 形状。当一个 diff **改了生产方却没改消费方**时触发;问"另一端是不是假设了比现在更强的契约"。 + +5. **agent 安全** —— sec-core 本身是一个组件,且 agent 负载带来特有威胁:prompt 注入、沙箱逃逸、deny-list 绕过(代码执行 deny-list 必须匹配解析后的路径,而非拼写)、生成 shell 的命令注入、root / sysctl 写入的提权、secret 泄漏进日志或 PR body。给特权写入和不可信输入路径专门看一眼。 + +6. **打包 / 分发正确性** —— `*.spec.in` / `component.toml` / manifests 不被 code-test CI 覆盖。检查 spec 依赖对齐 Cargo/npm 依赖、`requires_*` 符合现实、版本一致 bump、无孤儿或缺失的注册。 + +## 验证纪律(report "已验证" 前) + +本仓 CI 上的血泪: + +- **用 CI 锁定的 toolchain 复现**,别只用本地默认——旧 toolchain 上过的 lint 可能在锁定版上挂。跑 `cargo + fmt --check`、`clippy --all-targets -- -D warnings`、`test`。 +- **新逻辑放 lib crate 不放 bin**——否则增量覆盖率读 0%。 +- **commitlint**:`type(scope):` 小写开头、scope 在 `.github/commitlint.config.json`、header ≤ 120 字符、body 换行 ≤ 100。 +- **rustfmt 是编译依赖**(libbpf-cargo 的 skeleton builder),不只是 linter。 +- **能端到端测就端到端测。** 只编译或只单测不算"已验证"。PR body 里把测试状态拆成 已 E2E / 仅单测 / 未验证——绝不整篇"全部通过"。 +- **测试要有判别力**:撤回修复应让测试失败(要证实,比如快速 mutation)。正反双向都覆盖。警惕给假信心的短路(`mtime` 检查、`Ok(false)`)。 +- **抽取纯函数**,让不需要内核 / root / I/O 的逻辑能在非特权机器上单测。 + +## finding 纪律(避免假 finding) + +- **"代码和我预期的不同" ≠ bug。** 先读 `git blame` / commit message / 周边设计意图,回答"为什么现在是这样"。不查设计意图就报的 finding 经常被撤回。 +- **别信 PR 描述的抽象名词,查真实机制。** PR 声称的威胁模型 / 依赖 / 机制("某某注入""用了某某库")都去代码里核实是否真实存在——真正的读取/解析方可能和描述不符,据此的修复也可能与真实实现不匹配。按代码审,别照 PR 措辞审。 +- **内核 / 安全 bug 不用触发频率打折** —— UAF、未初始化读、信息泄漏、race 是源码的确定性属性;"合成场景里没见到"只关乎频率,不关乎是否存在。发现就报。 +- **下结论前对抗式验证** —— 尤其开 issue 前,别基于对代码的过期理解。 +- **核实后不成立的 finding 主动撤回** —— 别为显得审得深而硬留。 +- **"无需改动"是合法产出** —— 审计查完没问题就明说并收尾;别为显得做了事而制造改动。 + +## review 别人 PR 时 + +- **只陈述问题,不替作者开药方**:给事实、证据(`file:line`)、影响——让作者自己选怎么处理。 +- 报 finding 前先读作者的设计意图(同上教训)。 + +## 输出契约 + +- findings 优先,按严重度排序,每条带 `file:line`。若干净,明说无 finding 并注明残余风险。不要先写大段总结。 +- **不在范围内**:不重报 clippy / rustfmt / ruff / eslint / commitlint / coverage 已经 gate 的东西——那是噪音、蚀信任。只报机械 CI 和单个 reviewer 看不到的。 + +## 集成循环 + +去重所有 findings → 先修真问题 → 重跑受影响测试 → 若某个修复明显扩大 diff,再 review 一轮(至少 holistic / correctness / stability / performance)→ 如实报告:跑了哪些测试、review 了哪些角度、发现了什么、修了什么、还剩哪些残余风险。