Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions docs/developer-guide/en/independent-review.md
Original file line number Diff line number Diff line change
@@ -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 +<CI-version> 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.
76 changes: 76 additions & 0 deletions docs/developer-guide/zh/independent-review.md
Original file line number Diff line number Diff line change
@@ -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 +<CI-version> 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 了哪些角度、发现了什么、修了什么、还剩哪些残余风险。
Loading