OpenHands Agent Canvas 定制代码评审实践：用可执行架构测试约束 AI 与人类评审

本文围绕 OpenHands 仓库中 .agents/skills/custom-codereview-guide.md 这份仓库专属代码评审指南展开，讲清它如何把“评审决策规则、仓库所有权边界、可执行架构守卫”落地为可操作的 PR 评审方法论。读完本文，你将掌握：如何判断一个 PR 应给 APPROVE 还是 COMMENT、Agent Canvas 前端中哪些 API 访问路径属于“被机械禁止”的反模式、依赖精确锁定与测试证据标准是如何用测试代码（而非口头约定）强制执行的，以及 useEffect、事件契约、遥测所有权等具体评审检查点的源码级依据。

评审指南的定位：给 /codereview 技能补充仓库专属规则

这份指南位于 .agents/skills/custom-codereview-guide.md，文件头部的 YAML frontmatter 声明了它的技能元信息：

name: custom-codereview-guide
description: Repository-specific review rules for the OpenHands Agent Canvas frontend.
triggers:
  - /codereview

也就是说，当开发者或 AI Agent 在仓库中触发 /codereview 时，这份文档会作为“仓库特定规则”补充通用的 code-review 技能。文档开宗明义地给出三条总原则：

1. 先读 AGENTS.md——它是当前架构与测试约定的权威来源（source of truth），评审指南只是对它的补充；
2. 直接且有建设性，评审的是正确性与架构，而不是 lint 或编译器已经检查过的格式问题；
3. 指南中多处强调“可执行的源码才是权威”——评审规则若能用一个测试守住，就不应该靠文档反复口头强调。这一理念贯穿全文，下文会逐一对应到具体测试文件。

评审决策规则：只有 APPROVE 和 COMMENT

指南对“提交怎样的评审结论”给出了非常明确的约束，值得逐条拆解：

• 只提交一次评审，且只有两种结果：APPROVE 或 COMMENT，永远不使用 REQUEST_CHANGES；
• 默认倾向 APPROVE：没有重要发现时应当批准。“nitpick（吹毛求疵）和可选的重构建议不是拒绝批准的理由”；
• COMMENT 用于：正确性缺陷、安全问题、架构问题、缺少证据、或未满足验收标准。此时让人类维护者来做阻塞性决定（blocking decision）；
• 影响 Agent 行为的变更不能直接批准：涉及 prompts、工具选择、会话 payload、终端行为、规划、记忆或评测路径的改动，必须经过人类评审和相应轻量级 eval；
• 验收标准要落成清单：阅读关联 issue，为每条验收标准生成一个紧凑 checklist。满足 checklist 是必要条件，但不能替代针对回归、安全、可维护性的评审。

这套规则的核心思想是：评审结论要表达“信心等级”而不是“否决权”——AI 或自动化评审给出 COMMENT 指出问题，但阻塞合并的决定权保留在人类维护者手中；同时通过“默认批准”抑制无意义的评审噪音。

仓库所有权：行为应该放在拥有它的仓库里

指南给出了一张五仓库所有权矩阵，要求评审时把行为放到“拥有该行为”的仓库：

仓库	拥有范围
OpenHands/OpenHands（本仓库）	Agent Canvas UI、前端状态、后端选择、前端服务集成、本地栈编排
OpenHands/software-agent-sdk	Agent Server、agents、tools、conversations、events、workspaces，以及规范的 server API
OpenHands/typescript-client	浏览器兼容的 Agent Server API 类型化访问层
OpenHands/extensions	可复用的 skills、plugins 与集成
OpenHands/automation	调度、webhooks、运行历史、automation 派发

正常的依赖方向是单向的：

Agent Server 契约 → TypeScript client → Canvas（本仓库）

因此评审时要标记三类越界：

1. 绕过类型化客户端、直接对 Agent Server 端点做原始 HTTP 访问（raw endpoint reimplementation）；
2. 在 Canvas 本地复制服务端契约（Canvas-local copies of server contracts）；
3. 变更开在了错误的仓库里。

指导 Agent 的架构：把“常规路径”变成最便宜的路径

指南专门有一节讨论“代码库本身就是在指导 Agent 的决策”——因为 AI Agent 倾向于复制最近的模式、选择最短的能编译的路径，所以架构必须从产品层面引导这些选择。它给出五条原则：

1. 让常规路径成本最低：新工作应当自然地复用已有的命名 hook、service、store 或 feature 模块，而不是在共享的根组件里再加一个分支；
2. 让被禁止的依赖机械地失败：反复出现的评审指导应该沉淀为 lint 规则、编译器边界或架构测试。“当一个小而可执行的守卫更清晰时，不要继续增长这份文档”——这正是下一节那些架构测试的由来；
3. 持久状态只有一个显式写入者：后端设置、consent 值、会话缓存条目、持久化的浏览器值，都应该有一个命名 owner。要标记“第二个写入者”以及组件本地对权威状态的镜像副本；
4. 偏好功能自有文件，而非共享开关：产品工作通常应该扩展 feature 自有模块；共享注册表和根级条件分支需要给出具体理由；
5. 例外要窄且可见：例外清单（allowlist）应该放在执行规则的守卫旁边，并按架构变更来评审。

指南还给出一个重要的纠偏：“deep module（深模块）是设计启发式，不是行数目标”。好的模块接口窄而稳定、隐藏内聚的复杂度；不要仅仅因为文件长就拆分，也不要创建只做“重命名或转发参数”的中间层。只有当一个小的纯函数 seam（接缝）能消除重复决策、让所有权显式化、或支持聚焦测试时，才值得引入。

React effects：useEffect 只用于同步外部系统

指南对 useEffect 的误用给出了一个明确的“红旗清单”，评审中遇到 effect 在做以下事情时应标记：

• 从 props 或 state 派生渲染数据；
• 响应本可以在事件处理器中完成的用户动作；
• 初始化本应放在惰性 state 初始化器（lazy state initializer）里的值；
• 把一个 store 或缓存镜像进另一个组件的 state 值；
• 修复“竞争性写入者”造成的时序问题。

但同时强调：effect 并非天然错误。订阅、浏览器 API、定时器、网络同步仍然属于 effect 的正当用途——前提是 cleanup 和依赖语义是显式的。

阻塞性架构检查点

这一节是整份指南中“可执行守卫”最集中的部分，每条规则都对应仓库中真实存在的测试或单一 owner 模块。

Agent Server 与 Cloud API 访问：架构测试即权威白名单

文档声明：src/api/no-direct-agent-server-calls.test.ts 是“可执行的权威来源”。不得批准任何新的对 Agent Server 端点的原始 fetch、axios、共享 openHands axios 实例或底层 HTTP 客户端访问；正确做法是使用 @openhands/typescript-client 加上 src/api/agent-server-client-options.ts 提供的选项。

从源码看，这个守卫的实现方式是对 src/ 全目录做文本模式扫描，任何命中即失败：

// src/api/no-direct-agent-server-calls.test.ts#L6-L11
const EXCLUDED_SEGMENTS = new Set(["mocks", "routeTree.gen.ts"]);
const ALLOWED_AD_HOC_HTTP_FILES = new Set([
  "api/automation-service/automation-service.api.ts",
  "api/cloud/proxy.ts",
  "api/main-app-auth.ts",
]);

它检查的违规模式包括（见 该测试 L38-L73）：

违规模式	说明
openHands. 调用	直接使用共享 axios 实例
createHttpClient(	直接调用底层客户端工厂
从 @openhands/typescript-client/client/http-client 导入	直接引用 SDK 的底层 HttpClient 模块
new HttpClient(	直接构造底层 HTTP 客户端
axios(...) / axios.get/post/...	直接用 axios 发请求（白名单文件除外）
fetch('/api/...')	用 fetch 直连 /api 路径（白名单文件除外）

注意一个精妙的细节：扫描会排除 mocks/ 与 routeTree.gen.ts（mock 需要模拟真实 HTTP，路由树是生成产物），同时只允许三个文件做“ad-hoc HTTP”——它们恰好都是 HTTP 抽象的定义层本身（云代理、automation 服务适配、主应用鉴权），而不是业务代码。这正是指南第 5 条原则“例外窄且可见、且放在守卫旁边”的实例化：白名单不写在指南文档里，而是作为测试里的常量，随代码一起演进。指南甚至明确警告：“不要把它的当前条目复制进这份指南——测试应保持为唯一权威清单。”

那“正确做法”长什么样？src/api/agent-server-client-options.ts 就是所有类型化访问的统一入口。它导出两个关键函数：

• getAgentServerClientOptions(overrides)（L52-L69）：host 的解析优先级为 overrides.host → overrides.conversationUrl（经 buildHttpBaseUrl 归一化）→ 当前有效本地后端的 backend.host；API key 的优先级为 sessionApiKey → apiKey → backend.apiKey；没有可用后端且无覆盖项时抛出 NoBackendAvailableError，配套的类型守卫 isNoBackendAvailableError 支持跨打包边界的识别（连 name 属性也做了兼容判断）。
• getAgentServerHttpClientOptions(overrides)（L71-L80）：把上面的选项转换为 SDK 需要的 { baseUrl, apiKey, timeout } 形状，超时默认 60000ms。

对于 Cloud 与 runtime sandbox 请求，规则是必须走 callCloudProxy，且 runtime 请求必须提供正确的 hostOverride 与鉴权模式。从 src/api/cloud/proxy.ts 的源码可以确认其契约：

export interface CloudProxyRequest {
  backend: Backend;
  method: CloudRequestOptions["method"];
  path: string;
  body?: unknown;
  headers?: Record<string, string>;
  timeoutSeconds?: number;
  hostOverride?: string;              // 提供后走 runtime 专用客户端
  authMode?: "bearer" | "session-api-key" | "none";
  sessionApiKey?: string | null;
  responseType?: "blob";
}

callCloudProxy 内部根据 hostOverride 是否存在，选择 createCloudClientForRuntime(backend)（指向运行时沙箱）或 createCloudClient(backend)（指向云端），authMode 缺省按 "bearer" 处理。评审时对“代理白名单/守卫的改动”要按架构变更对待，而不是普通逻辑修改。

事件线契约：SDK 是唯一 wire 权威

指南确立了事件模型的三方关系：SDK 事件模型是 wire 权威，TypeScript client 是它的镜像，Canvas 消费已发布的 client 类型。评审中不得批准：

• Canvas 本地的重新声明（redeclarations）；
• 局部交叉类型（partial intersections）；
• 对 wire 事件接口的模块增强（module augmentation）；
• 往 wire 事件接口里塞展示（presentation）字段。

契约变更必须按固定顺序落地：

1. SDK 模型/ schema 与序列化覆盖；
2. TypeScript client 中从 SDK payload 派生的镜像类型；
3. 已发布的 client 版本；
4. Canvas 的消费与渲染/遥测覆盖。

只属于 Canvas 的展示状态，应放在以事件标识为键的独立 view model 中，与 wire 契约隔离。这条规则防止的是一类隐蔽腐化：UI 需求不断往 wire 类型上“借”字段，最终让前端类型与 SDK 真实报文悄悄分叉。

遥测与持久化前端状态：单一 owner 原则的具体化

指南对遥测与持久状态给出五条硬规则，并且每一条都能在源码中找到对应实现：

• src/services/telemetry.ts 是 Canvas PostHog 客户端的唯一 owner。从源码看（L1-L27 的模块注释与 L45-L54 的常量定义），它统一管理 consent 相关的存储键（openhands-telemetry-consent、...-pending-cloud-sync、...-pending-local-revocation、...-change 事件、first-use 与 session 键）和单例 PostHog 实例（POSTHOG_INSTANCE_NAME = "agent-canvas"）；
• React 事件必须走 src/hooks/use-tracking.ts 中的类型化函数，组件不得直接调用 PostHog。该文件的 useTracking hook 注释明确写道：“共享的 PostHog 客户端强制执行 telemetry.ts 配置的规范 consent 状态；本 hook 不得以 backend settings 作为 capture 门控，因为后端切换时它们可能过期”——这是对“consent 只由单一控制器决定”原则在 hook 层的再次声明；
• consent 渲染使用遥测 consent 的 external store，而不是镜像的本地 state；setTelemetryConsent 保持为唯一的 consent 控制器（见 src/services/telemetry.ts 中的 SetTelemetryConsentOptions，其 syncToCloud 选项专门用于区分“用户主动设置”与“从后端镜像回来”两种写入场景，避免镜像值被二次回写 Cloud）；
• 一个业务里程碑只有一个规范 capture 点，重复的条件性 capture 要被标记；
• 其他持久值：优先使用既有的命名 service/store/hook，标记来自任意组件的新存储写入。

模块注释里还展示了这套 owner 化设计的实际收益：consent 语义被显式写成“canvas_install 在安装时立即发送、无论是否同意（匿名、无 PII）；session/自定义事件仅在用户同意后才发送；用户可通过 VITE_DO_NOT_TRACK=1 或浏览器 Do Not Track 完全关闭遥测”。因为规则集中在单一模块，评审者只需要审这一个文件就能判断 consent 行为是否被破坏。

依赖与发布：精确锁定 + 供应链审慎

指南在依赖治理上给出四条规则，其中三条由 tests/package-library.test.ts 以测试形式强制执行：

1. 直接依赖精确锁定（exact-pinned）。通过 npm 保持 package.json 与 package-lock.json 同步，不要只手改一侧。对应测试用 EXACT_SEMVER_PATTERN（见 L20-L21）校验 dependencies 与 devDependencies 中的每个版本必须形如 \d+\.\d+\.\d+（可带 pre-release/build 元数据）；
2. 依赖豁免、git pin、安全覆盖属于可评审的政策变更。测试中的 ALLOWED_STACK_PIN_DEPS（L24-L27）只豁免 @openhands/extensions 与 @openhands/typescript-client 两个 git 依赖，并且源码注释写明了各自豁免的原因与回收条件（其中 typescript-client 的豁免挂着 TODO(#917)，要求其在对应分支合并并发布到 npm 后移除）——豁免本身是带理由和到期日期的，评审时应把它当作政策变更来审；
3. 警惕新发布的第三方版本带来的供应链风险。第一方 OpenHands 包豁免“等待期”，但不豁免契约与发布顺序评审；
4. 包版本变更属于显式的 release PR，必须与 release workflow 的预期一致。

该测试还锁定了 @openhands/agent-canvas 包的发布面（L29-L61）：main/module/types 入口以及 .、./conversation、./settings、./terminal、./i18n 五个 export 子路径的类型/import/require 三元组都必须精确匹配，防止发布配置在无意中被改动。

测试与证据：证据要与行为变更成比例

指南的“Testing and Evidence”一节定义了评审时对证据的最低要求：

• 证据强度与行为变更成比例：UI 行为要用真实应用的截图或视频；CLI、API、脚本类变更要求给出精确的运行时命令与观察到的结果；“单测本身不构成端到端证据”；
• 奖励真实逻辑与可观察状态：不要奖励“只证明另一个 mock 被调用过”的 mock 测试；
• 保持测试聚焦：一个行为一条有意义的断言路径；不重复覆盖库自身行为；不要脆弱的、纯展示层的快照；
• 遵循 AGENTS.md 的测试路由：如果变更跨越完整全栈流程且缺少合适覆盖，应建议 mock-LLM E2E 并视情况添加 e2e-tests 标签；
• 永不为了方便而扩大 live E2E 触发范围或暴露 secret。

结合 AGENTS.md 的测试框架说明，评审时的路由决策可以具体化为：

• 常规验证命令为 npm run lint、npm test、npm run build 和 npm run build:lib；
• mock-LLM E2E（tests/e2e/mock-llm/，本地 npm run test:e2e:mock-llm）走“浏览器 → 真实 agent-server → 脚本化 mock LLM”的完整栈，不消耗真实 LLM 凭据，是绝大多数全栈行为变更的默认 E2E 证据来源；其 CI workflow 还通过 detect-pr-changes 轻量任务对纯文档 PR 跳过重型任务；
• live E2E（tests/e2e/live/，仅 npm run test:e2e:live）跑真实 LLM，必须与 mocked 路径隔离，且强制 VITE_DO_NOT_TRACK=1、在本地拦截任何 PostHog 请求以防污染分析数据。评审中若发现有人为了让测试“更容易过”而放宽 live E2E 触发条件，属于明确的红线问题。

不该评论什么：抑制评审噪音

指南同样明确了负面清单——以下情形不应留下评审评论：

• 工具链已处理的格式或小风格问题；
• 与变更无关的可选“nice to have”重构；
• 纯夸奖式观察（应直接 approve）；
• 对简单数据/配置变更额外索要测试（当现有检查已覆盖该风险时）；
• 临时的 .pr/ 产物（仓库自动化会清理它们）。

提出发现（finding）时有一条方法论要求：沿着相关的调用流或数据流追踪到足以展示具体失败模式，并且“一条高信号评论优于同一所有权问题的多条症状评论”。这与前面“持久状态单一写入者”的原则首尾呼应：评审的目标是定位所有权缺陷的根因，而不是罗列表面症状。

沟通风格：小步修正，不制造反馈

指南最后规定了评审沟通的基线：

• 简洁、具体、友好；
• 解释用户可见或架构层面的后果（说明“不改会怎样”，而不是只说“这里不对”）；
• 给出最小可行修正；
• 局部修复使用 GitHub 的 suggestion 语法（```suggestion），让作者可一键采纳；
• PR 没问题就批准，不要为了产出而制造反馈（approve it without manufacturing feedback）。

小结

.agents/skills/custom-codereview-guide.md 的价值不在规则条目本身，而在它示范的一种工程实践：

1. 文档只写不可执行的部分——“评审决策倾向”“沟通风格”“证据标准”这类判断性规则留在文档；“禁止 ad-hoc HTTP”“禁止 git 依赖”“包入口面锁定”这类可机械验证的规则，全部沉淀为 no-direct-agent-server-calls.test.ts、package-library.test.ts 等架构测试，文档只引用测试为权威（src/api/agent-server-client-options.ts、src/api/cloud/proxy.ts、src/services/telemetry.ts 等则作为“唯一 owner”被指名道姓）；
2. 把 AI Agent 当作被架构影响的决策者——“让常规路径最便宜、让违规机械失败、让例外可见”，这套原则同时服务于人类与自动化评审；
3. 用所有权矩阵与依赖方向约束多仓库协作，把“这个改动应该开在哪个仓库”变成可核查的评审点。

如果你要在这类多仓库、强类型化、Agent 深度参与的代码库中开展评审（人工或 AI 自动化），这份指南提供了可直接迁移的检查点清单：先确认仓库所有权，再过 API 访问守卫、事件契约落地顺序、遥测单一 owner、依赖精确锁定与 E2E 证据成比例性这五道闸门，最后用“最小可行修正 + 后果解释”输出结论。