Conversation
helsome
left a comment
There was a problem hiding this comment.
整体实现方向是对的:用 Record<EvaluationFailureMode, ...> 把新增 failure mode 变成编译期必分类,并补齐 aggregate/baseline 测试,测试报告也完整。当前只卡一个会直接污染评测口径的语义问题:请不要把 judge_error 固化为 pass。
仓库方法论文档已经定义 judge_error 为 “excluded and counted”,而 summarizeExperiment 的 pass rate 会把 pass 同时计入分子和分母。于是 judge 本身失败会抬高 Agent pass rate;本 PR 新增的穷尽表和测试如果按当前写法落地,会把这个已有矛盾正式锁死。
建议在本 PR 一并收口:
- 仅
judge_error(没有更严重的 Agent 自身 failure mode)→not-applicable,从 pass-rate 分子/分母排除,但仍由countFailureModes计数; - 若同一 run 同时存在真实 Agent failure(如
missing_tool)与judge_error,真实 Agent failure 仍应主导为 fail/partial,而不是被 judge error 抹掉; - 增加对应 aggregate/pass-rate 测试。
resource_unavailable -> partial、未知跨版本 mode fail-closed、baseline 字段 optional chaining 这几部分我认可,不需要拆 PR。改完上述口径并保持现有 focused/shared/typecheck 绿即可继续审核。
|
已按 review 意见完成修改并推送(30fd9b7):仅有 本地验证:
麻烦批准并运行 workflow,方便的话也请复审,谢谢。 |
judge_error 语义已按 review 修正:judge-only run 排除 pass rate,真实 Agent failure 仍主导,且补齐聚合/计数测试。原 blocker 已解决。
helsome
left a comment
There was a problem hiding this comment.
语义 blocker 已解决:judge-only run 现在排除 pass rate 但仍计入 failure mode,真实 Agent failure 与 judge_error 并存时仍由真实 failure 主导;resource_unavailable/unknown-mode fail-closed/baseline 防御也都合理。代码与测试可批准。当前 main 已继续前进、GitHub 显示不可直接合并,请只 rebase 最新 main 后重跑 aggregate focused test + shared/typecheck/basic CI,不需要扩大 scope。action_required 仅是 Actions 授权状态,不作为质量失败。
30fd9b7 to
cd466a1
Compare
|
@helsome rebase 已完成,现在基于最新 main:head 唯一卡住的是 麻烦授权跑一下 CI 就可以合了,谢谢!(本地 Windows 环境有点 symlink 问题,跑不了完整 typecheck —— 就是 #99 记录的那个,所以想借 CI 确认。) |
改动说明
packages/shared/src/evaluation/aggregate.ts是评测汇总与回归门禁的入口,却没有测试文件。本 PR 做三件事:把失败模式分类改成编译期穷尽、补齐门禁读取 baseline 时的字段级守卫、补上该模块缺失的测试。1.
verdictForRun的失败模式分类改为编译期穷尽原实现用两个手写
Set表达严重度:FAILING_MODES(9 个)→fail,PARTIAL_MODES(5 个)→partial,共覆盖 14 个;不在任一集合里的模式会直接掉到函数末尾的return 'pass'。EvaluationFailureMode联合类型有 16 个成员,落在集合外的正是:resource_unavailable—— 按语义应为partial;judge_error—— 它同样不在集合里,但本来就该判pass(裁判/打分器失败不是 agent 失败,见evaluators/deterministic.ts)。所以这一处是"歪打正着"。关于影响面,需要如实说明:
resource_unavailable当前在全仓库没有任何赋值点(grep只命中类型定义、i18n 文案、UI 格式化表和docs/EVALUATION.md的分类清单)。因此本改动今天不改变任何运行时结果,也没有回归风险。它的价值在两处:Set与联合类型之间没有任何强制关系,以后往EvaluationFailureMode加一个成员而忘记分类,它会静默判pass;换成Record<EvaluationFailureMode, VerdictSeverity>之后这是编译错误。严重度沿用
evaluators/deterministic.ts的既有语义,没有改变任何既有分类:judge_error仍为pass,其余 15 项与原两个集合逐一对应。2.
compareToBaseline补齐 baseline 字段级可选链baseline有两条来源,行为并不一致:scripts/eval/ci-baselines/*.json,经scripts/eval/run.ts的loadCommittedBaseline()metrics直接返回undefined,thresholds兜底为{},并补齐id/name/experimentId等字段 → 这条路径没问题EvaluationStore.listBaselines()(experiment-service.ts按baselineId解析)EvaluationStore.load()只校验Array.isArray(raw.baselines),逐条不做形状校验(对比同一函数里的settings会过sanitizeSettings)所以由旧版本写入的 store 条目可能既没有
metrics也没有thresholds。原代码只对baseline本身用了可选链,字段没有:改为
baseline?.metrics?.[m]/baseline?.thresholds?.[m],缺键时该指标按"没有基线值"跳过(沿用下游已有的passed = true语义),其余指标照常比较。门禁的失败应当是有信息量的信号(真回归),而不是读取外部持久化数据引发的异常。这一条比第 1 条弱,属于防御性一致性修复(同一函数内一半做了守卫、一半没做)。如果认为它超出本 PR 的 scope,我可以拆成单独 PR 或直接去掉。
3. 补齐该模块缺失的测试
新增
aggregate.test.ts(20 个用例),覆盖verdictForRun、aggregateScores、countFailureModes、compositeScore、summarizeExperiment、compareToBaseline/gatePassed。两条关键测试:classifies every failure mode in the union—— 先用运行时列表断言EXPECTED_SEVERITY的键集合等于联合类型的成员集合,再逐一断言每个模式的具体严重度。前者防"漏分类",后者防"分类写错"。never treats an unrecognised failure mode as a pass—— 锁定防御性分支:带未知模式的记录(跨版本持久化)必须判fail。关联 Issue
Closes #107
Related to #15([Eval] Add Gold Case dataset and real end-to-end evaluation harness)。#15 已被 @xxstar-01 认领,本 PR 不与其重叠,也不关闭它。
测试报告(正式审核前必填)
环境
upstream/main@3eee5fb(本 PR 的父提交)实际执行命令与结果
已知失败 / Baseline
无。 上述命令在
upstream/main@3eee5fb上全部通过(packages/shared925 pass / 0 fail),没有需要归因的失败项。UI 截图(仅可见 UI 变化时必填)
Scope / 后续
aggregate.ts的判定与门禁读取,并补上该模块的测试;不改数据集、runner 或 Langfuse 集成。docs/EVALUATION-METHODOLOGY.md:94-95写的是 "judge_errorruns are excluded and counted",而代码里judge_error→ verdictpass,summarizeExperiment又按passRate = passed / (verdict !== 'not-applicable' 的数量)计算 —— 即该 run 既进分母、也算通过,与文档的"excluded"不一致。本 PR 保持现状(沿用deterministic.ts的"裁判失败不是 agent 失败"语义),未改动judge_error的判定。如果文档才是意图,正确做法是把judge_error判为not-applicable(与skipped一致,从分子分母同时排除);需要的话我可以另开 PR 或在本文中一并调整,请指示。resource_unavailable在全仓库只有类型 / i18n / UI / 文档中的声明,没有任何赋值点,属于"声明但未使用"。是否要接入(例如把 provider 不可用映射到它)是另一个话题。EvaluationStore.load()对baselines只做集合级Array.isArray校验、逐条不归一化,而同一函数里的settings会过sanitizeSettings。如果认为持久化读入应当统一做形状归一化,可另开 issue 讨论。