Skip to content

fix(benchmark): preserve unknown Harbor usage metrics - #7543

Merged
huangruiteng merged 1 commit into
mainfrom
codex/harbor-usage-completeness-20261028
Oct 28, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/harbor-usage-completeness-20261028

Conversation

@songoow

@songoow songoow commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Goal And Delivered Outcome

Preserve the distinction between measured zero and unknown usage in the existing Harbor benchmark adapter. This is a bounded measurement-correctness prerequisite for the controller/executor RFC's C7 evaluation work, not an implementation of the full C7 cost ledger or a token-efficiency claim.

Before, missing converted session metrics were added as zero, a partial set of costs could be presented as the total, and explicitly measured zero token totals became absent during serialization. Phase subtraction also treated an incomplete baseline as zero and could emit negative usage after counter regression.

After, aggregation preserves per-field uncertainty, serialization retains known zeroes, and phase deltas require known, non-regressing observations. AgentContext and the aggregate trajectory use the same parsed snapshot instead of parsing the sessions twice.

  • Author baseline: main@1fe6134e0b0fd0df6dc671a9bce0ea29c9613891.
  • Published head: 8db343119433ae5c2b1d6130ecf1496d186438aa.
  • Scope: two files, +265 / -38; existing Harbor adapter and focused synthetic regressions.

Author Declaration

  • Written by: model_agent — OpenAI GPT-6 Astra Pro, under the requesting contributor's direction.
  • Specification: the accepted bounded repair request above, using the existing Harbor FinalMetrics/AgentContext contracts and LoopX's benchmark adapter at the author baseline.
  • The commit includes the contributor's DCO sign-off and an assistance declaration.

Changes And Ownership

Requirement Implementation Regression coverage
Missing metrics must not become measured zero or a complete partial sum _totals propagates unknown independently for prompt, completion, cached input and cost; missing metrics objects and an empty observation remain unknown Missing field in either session order; absent metrics; empty observations; generator inputs
Known zero must survive reporting Aggregate FinalMetrics receives the values directly, without or None Zero-valued context and serialized trajectory fields
Invalid numeric measurements must not become totals Reject negative values and non-finite costs; overflowing cost sums remain unknown Negative fields, NaN, positive/negative Infinity and cost-sum overflow
A phase delta needs a usable baseline _populate_context distinguishes an absent baseline from a present but incomplete one; regressing cumulative values remain unknown Missing/null baseline fields, empty baseline, counter regression and unchanged-phase zero
Context and aggregate must use the same observation Pass the already-parsed trajectory list to the existing aggregate writer Single-read assertion, cumulative versus phase values, step reindexing without mutating source steps
Empty initial logs differ from an existing unconverted rollout run treats only a genuinely empty pre-phase session-file set as an absent baseline Synthetic preparation-failure path with and without an existing rollout

The implementation remains in the adapter that owns Harbor reporting. It introduces no control-plane vocabulary, scheduler, model session, accounting authority or alternate provider parser. Cached input remains a subset of prompt input, not an additional quantity to add to it.

Validation

The executed regression evidence is isolated-method qualification, not an installed-Harbor integration pass. The local environment lacked the Harbor dependency. A private harness executed the exact production method bodies extracted from the adapter and the new test cases, using synthetic Harbor model/import/constructor doubles and an identity prompt decorator. That harness is not part of the PR.

Check Actual result and boundary
Same isolated regression cases against the unchanged adapter 38 failed, 3 passed
Same isolated regression cases against the repaired adapter 41 passed
Python compilation of both changed files Passed
Staged diff whitespace check Passed
Remote publication integrity Both uploaded file blobs match the local validated candidates: adapter 1309fcc45493975c25bb5b99361b6c2d28f564be; tests 0974f3b06befa62ce9730e35d91a006c28b99e39. The original adapter preimage was verified as blob 1dd183ae83c866705ec6b2a3245d9558ca160d26.
Installed Harbor, real native JSONL parser and full repository CI Not run locally; remain required qualification. The 41 isolated passes do not qualify the real constructor, prompt decorator or provider parser.
Repository premerge canary, Ruff and full public-boundary scanner Not run in this reconstructed local validation environment; no passing gate is claimed. The two-file diff was manually checked for private content.

The committed test module imports the actual adapter and Harbor models, rather than shipping the private isolation harness. With the supported Harbor dependency installed, run:

python -m pytest -q benchmark/tests/test_harbor_usage_completeness.py
python -m pytest -q benchmark/tests/test_shared_codex_runtime.py

A missing-dependency skip is not a passing integration result. No previous batch's test count is reused as qualification of this head. No Docker benchmark, model inference, paid evaluation or historical dataset rewrite was performed.

Limits And Continuation

This repair governs the metrics of the trajectories supplied by Harbor's existing converter. It does not certify completeness of the raw session set, detect every lost/forked session, restore information already discarded by the upstream parser, reconcile provider invoices, or establish full request/role-level cost attribution. In particular, an otherwise unreported conversion failure is not solved by fixing aggregate arithmetic. Those boundaries remain explicit follow-up work rather than being labeled complete here.

The single-snapshot change removes one redundant adapter parse during context population, but no measured latency, token saving or task-score improvement is claimed. Difference-based reporting alone is not proof of identical session membership across phases.

This does not duplicate the quota/Turn unified-contract or differential-context PRs, change feature defaults, or close the controller/executor RFC. Existing runs retain their pinned code. Review, installed-dependency validation and normal required checks remain necessary before merge.

Frontend / Visual Evidence

UI impact: none. No frontend changes or screenshots are applicable.

Boundary Checklist

  • Only the existing benchmark adapter and synthetic regression tests change.
  • No private HF logs, raw traces, credentials, hidden evaluator data or host-local paths are included.
  • No active trial, task, score, budget, admission or settlement behavior is changed.
  • No RFC pair is modified.
  • DCO sign-off is present; no merge or independent approval is claimed.

中文摘要:本 PR 修复 Harbor 已转换会话指标在汇总、序列化和阶段差量计算中的零值/未知值混淆,同时复用同一份解析结果。它是 C7 测量正确性的一个独立修复,不代表完整成本归因或降 token 优化已经完成。本地 41 项通过来自明确披露的隔离测试;真实 Harbor 依赖、原生解析器和完整 CI 验证仍待完成。

Keep incomplete converted session metrics unknown, preserve measured zeroes,
and refuse unsupported phase differences. Use one parsed snapshot for the
AgentContext and aggregate trajectory. Add focused synthetic regressions.

Assisted-by: OpenAI GPT-6 Astra Pro
Signed-off-by: song <liusongstep@gmail.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-28T01:52:57.793380Z 8db3431 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cocolord cocolord left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer: model_agent; model=GPT-5; provider=OpenAI; declaration=self_reported

动机

Harbor benchmark 的运行者需要从一次真实任务中得到可信的模型 token 用量;旧实现会把无法读取的 usage 当成 0,并在阶段边界重复取样,使“确实为零”和“观测缺失”不可区分,也可能把前后快照不一致地相减。该 PR 的目标是在 exact head 8db343119433ae5c2b1d6130ecf1496d186438aa 上保留未知值、只用一份终态快照,并只在前后两端都已知时给出阶段增量。范围只涉及 Harbor runtime 的 usage 观测与对应回归,不改变 benchmark 评分、任务语义或启动行为。当前仓库没有为这项局部修复提供独立的书面验收规范,因此我按现有 runtime 合同和可复现回归判断。

改动思路

实现沿用 HarborSharedAgentRuntime 和现有 usage snapshot 数据流,没有新增第二个计量 authority。入口仍是 Harbor 运行结束后的共享 runtime 采样;_normalize_usage_value 把缺失、非法和非有限值收敛为 unknown,_phase_usage_delta 只有在基线与终态均为已知非负数时才相减,最终一次 get_usage() 的返回同时供累计值和阶段值消费。正路径是两端均有有效 usage 时得到精确 delta;负路径是 API 缺失、异常、NaN 或单端未知时保留 None,由上层明确呈现观测缺口,而不是伪造为零。失败与重试仍由既有 Harbor adapter/runtime 负责。

具体改动

关键代码讲解

  • benchmark/runtime/harbor.py::_normalize_usage_value:接受 Harbor 返回的动态值,拒绝布尔、负数、NaN/Inf 和不可解析输入,返回 int | None,把“未知”保持为 typed state,而不是靠字符串或隐式零值分类。
  • benchmark/runtime/harbor.py::_phase_usage_delta:以开始/结束快照为唯一输入;任一端未知就返回 None,否则计算非负差值,避免 reset 或计数回退被包装成错误的大数。
  • benchmark/runtime/harbor.py::run 的终态采样分支:任务完成后只取一次 usage snapshot,再派生累计量与 phase delta,消除了同一逻辑时点的双读取漂移。
  • benchmark/tests/test_harbor_usage_completeness.py:覆盖 unknown、zero、partial snapshot、non-finite、counter reset 和 single-snapshot 行为。测试是对现有 runtime 合同的回归,不是新的生产 authority。

全 diff 为一个 runtime 文件和一个聚焦测试文件,复用了现有 Harbor usage owner;没有新增配置、协议版本、CLI 或兼容分支。未发现需要顺手扩展的相邻重构,当前边界已经足够局部、可回退。

对主干的风险

最强反例是 Harbor 返回不完整或非有限 usage:若实现仍把它归零,报表会把“没有证据”发布成“没有消耗”;若结束阶段再次读取,两个消费者可能看到不同快照。exact-head 的 41 个新增聚焦用例全部通过;在 Python 3.12、harbor==0.20.0 下,补齐仓库测试导入路径后共享 Codex runtime 的 158 个用例也全部通过。当前远端红项分别是 delegation-search 浏览器超时和 Windows effect-runtime diagnostic 竞态;它们不在这两个文件的调用/不变量范围内,且 usage 变更已有独立通过证据,因此不归因于本 PR,但仍是合并前需要仓库 owner 处理的独立 CI hold。

四个额外 lens 也已检查:usage 的 known/unknown 由可空数值合同表达,没有 substring denylist;错误文字与 runtime 行为保持领域中立;默认行为的有意变化是 unknown 不再伪装成 zero,并由回归测试披露;这里没有把 advisory guidance 冒充 machine obligation。剩余风险主要是第三方 Harbor 将来改变 usage 字段形状,现有 normalize 和回归会把它显式暴露为 unknown,而不会静默篡改计量。

我的整体评价

这是一个明显正向、范围匹配且证据闭合的修复:它提升长期 benchmark 观测的可信度,也让操作者能区分零消耗与缺失数据;没有引入额外状态 owner、用户步骤或默认开启能力。代码量与问题成比例,兼容面保持在既有 adapter 内。对 exact head 8db343119433ae5c2b1d6130ecf1496d186438aa 给出 APPROVE;该结论不代表合并授权,远端无关红项仍应按仓库 merge gate 独立收敛。

English verdict: APPROVE - exact head 8db3431 preserves unknown usage, uses a single terminal snapshot, and passes 41 focused plus 158 shared-runtime tests; unrelated CI failures remain a separate merge-readiness hold.

@huangruiteng
huangruiteng merged commit 0cfbf11 into main Oct 28, 2026
25 of 34 checks passed
@huangruiteng
huangruiteng deleted the codex/harbor-usage-completeness-20261028 branch October 28, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants