fix(benchmark): distinguish source env usage from credential probes - #3298
Conversation
2663882 to
c29338a
Compare
|
Additional validation after the latest scored run: replaying a representative private post-run trajectory (91 tool calls; raw evidence not published) changes the current-main result from 7 credential-probe false positives to 0 under this PR, while the existing direct enumeration and sensitive-name tests remain green. This confirms the intent classifier fixes the observed task-source false positive without weakening the documented runtime-isolation boundary. |
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
English verdict: REQUEST_CHANGES for exact head c29338a4e59a2edb208d097e44499ed143dc07a0.
Findings
-
[P1] The new prose regex both misses real environment enumeration and still treats source inspection as credential access (
loopx/capabilities/benchmark_toolkit/integrity.py:108-118). The direct-command pattern only accepts a serializedcmd/commandwhose value starts with bareenvorprintenv; therefore{"cmd":"sh -c env"},{"command":"bash -lc printenv"}, and{"cmd":"/usr/bin/env"}all return false. The enumeration pattern also missesos.environ.copy()andos.environ.items(). At the same time,grep -R 'os.getenv("API_KEY")' srcstill returns true even though it is source inspection, which contradicts the PR's stated source/runtime distinction. This is an integrity-policy boundary: a real credential probe can become countable, while a source-only run can still be rejected.Required repair: classify typed tool/command structure rather than relying only on proximity regexes over canonicalized prose. At minimum, cover shell wrappers and absolute
env/printenvexecutables, environment copy/iteration, and add a negative test for source search containing a sensitive environment name. The direct enumeration positives and source-only negatives need to live in the same regression table so the two sides cannot drift independently.
动机
修复 benchmark trajectory 中“源码提到环境变量 API”被当成“运行时读取凭据”的误报,动机成立,也直接影响合格 run 是否可计数。
改动思路
PR 把原来的宽泛匹配拆成敏感变量访问、环境枚举、直接环境命令和 procfs 读取四类规则。方向比单一 denylist 更清楚,但当前仍然是对规范化文本做子串/邻近匹配,没有真正表达“执行了什么工具”和“只是在读源码”的类型差异。
具体改动
README 更新了边界说明;integrity.py 新增四组正则;测试增加了裸 env、print(os.environ) 等正例,以及非敏感源码提及和 subprocess(..., env={}) 等反例。现有 22 个聚焦测试和 Ruff 均通过,但上述双向反例未被覆盖。
对主干的风险
这是 benchmark integrity 的机器强制规则,不是提示性 guidance。当前 false negative 会弱化凭据隔离证明,false positive 会继续错误淘汰合法 run;两者都会影响 benchmark 证据可信度,因此不能以现有 head 合入。
我的整体评价
问题选得对,规则拆分也让后续修复更容易,但这版尚未证明“减少误报且不削弱运行时探测”。请先补齐 typed command/source distinction 与上述回归覆盖,再按新 head 复审。
Validation performed:
pytest -q tests/capabilities/test_benchmark_toolkit.py— 22 passed- Ruff on the changed implementation and test — passed
- Focused reproductions — bare
envdetected; shell-wrapped/absolute environment commands and environment copy/iteration missed; sensitive-name source grep misclassified - Repository CI checks shown on this exact head — green
Merge decision: request changes / hold.
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
e5fe352 to
feb09fb
Compare
Signed-off-by: huangruiteng <huangrt01@163.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Exact-head review conclusion for 9a592159fd4911ba291552b57c98dad2662929bc
English verdict: APPROVE / no remaining blockers (recorded as COMMENT because GitHub does not allow an author to formally approve the same PR).
Findings
No actionable findings remain. The previous P1 is resolved in both directions: executed environment access is detected through typed command structure, while neighboring prose, source searches, source-writing literals, and nested metadata do not become credential evidence.
动机
原实现把“命令参数中出现环境 API 文本”近似成“运行时读取环境”,既会误淘汰合法源码检查,也会漏掉 shell wrapper、绝对路径和环境枚举。该问题直接影响 benchmark integrity receipt 是否可信、run 是否可计数,修复必要。
改动思路
把判定边界从规范化后的整段 prose 收窄到真实执行字段:直接 shell tool 读取 cmd/command,Codex 编排调用只提取 tools.exec_command 顶层命令属性;再按 shell、Python、Node 的实际执行语义分类。敏感值扫描仍覆盖全部参数和 observation,不因本次 precision 修复而缩窄。
具体改动
- 覆盖
sh -c env、bash -lc printenv、绝对路径、procfs、Python copy/items、Node 枚举与敏感名; - 支持 Codex orchestration 的 literal、constant binding、shorthand command;
- 忽略 grep/rg 源码搜索、源码写入字符串、非敏感 lookup、child
env={}、嵌套 metadata; - 保留多行 shell command 的语句边界,防止后续行中的 probe 漏检;
- 正反例位于同一参数化回归表,避免两侧再次漂移。
对主干的风险
这是机器执行的 evidence-policy 边界,主要风险是 false negative 弱化完整性、false positive 错误淘汰 run。当前实现仍是保守的静态分类器,runtime isolation attestation 继续承担独立强边界;本 PR 没有改变 benchmark scoring、任务语义、runner 启动或权限。
我的整体评价
修复已从“调正则”升级为 typed execution/source boundary,并用真实 Codex 调用形态与双向回归锁住。范围集中在 benchmark toolkit 内部 classifier、契约文档和单测,具备合入条件。
Validation on this exact tree/head:
- focused pytest: 53 passed
- Ruff check/format and Python compile: passed
- representative private replay: 91 tool calls / 96 steps, qualified with all evidence counts zero; raw evidence not published
- premerge canary: 4 direct checks + 18 selected checks passed; zero failures/warnings
- public/private boundary: all 4 changed files scanned, zero skips/hits
- GitHub CI: DCO, dependency review, release build, Frontstage Pages, and Python Tests passed
- expected skips: deploy, upload-release, publish-pypi
- one pre-job Frontstage concurrency cancellation was rerun on the exact head and passed
- non-gating local mypy attempt was not counted because the host Python 3.13 runtime lacked
_sqlite3; CI's type-check step passed
Manual hold disposition: canary correctly retains benchmark_sensitive for explicit maintainer review. The repository owner explicitly authorized self-merge in this task; this exact-head review plus the green checks satisfies that hold. Change-quality receipt enforcement is disabled for this goal and is not claimed as evidence.
Merge decision: approve and self-merge with admin bypass.
Motivation
Benchmark trajectories can contain commands that inspect or edit task source code. Scanning canonicalized prose for environment API strings conflates those source references with an executed credential read, while also missing wrapped or absolute-path environment commands. Either error makes benchmark integrity receipts unreliable.
Change
cmd/commandfields instead of neighboring narrative text;env/printenv, procfs reads, Python environment enumeration or sensitive-name lookups, and equivalent Node access;Validation
uv run pytest -q tests/capabilities/test_benchmark_toolkit.py tests/capabilities/test_capability_documentation.py— 53 passedloopx canary premerge --from-git-diff --goal-id loopx-deepswe-bench— 4 direct checks and 18 selected checks passed; all 4 changed public files were scanned with no skipThe canary retains its expected
benchmark_sensitivemanual-review hold because this changes evidence policy. The repository owner explicitly authorized self-merge after exact-head review and green CI; that review and the hold disposition will be recorded before merge.