fix(eval): fail closed when the egress audit log is incomplete - #2967
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The direction is right and the ENOENT narrowing at :719 is a real improvement — a read that fails for any other reason now surfaces instead of being swallowed. The sha256: prefix also aligns this artifact with maka-artifacts.ts:14,119, so that is a correction rather than a break.
One thing blocks, and it is the thing in the title.
The incomplete case is never actually closed
egress_filter.py:136,148-164 writes an audit_truncated marker when the log hits MAX_AUDIT_BYTES, and it works — I confirmed the marker is written once, that a torn tail gets its \n, and that subsequent hits are dropped. But grep audit_truncated packages/eval/src/ returns nothing. The TypeScript side only asks whether the file exists.
So a truncated log — the literal definition of incomplete — returns missing: false and the attempt completes with its reward intact. Filling the 1 MiB cap takes 251 intercepted requests (normalizedPath is capped at 4096 bytes, egress_filter.py:142); after that, every further contamination attempt leaves no trace. A subject that hammers the benchmark repository until the log fills is therefore less likely to be caught than one that touches it twice.
Both Python and TypeScript tests are green on this, which is the point: they cover a contract with a producer and no consumer. Either collectEgressAuditArtifact parses the tail for audit_truncated, or the title should say "missing".
Worth deciding on policy_error at the same time (egress_filter.py:107-119). When the filter itself throws it returns 503 for every request and records that ruleId — also unread on the TS side. A filter-wide failure makes the subject's every request fail, the task fails, and it lands in subject_failed, which is a scored zero. That is the one harness fault in this area that can actually depress a score, and it routes around this PR's fail-closed.
What the fail-closed currently proves
Less than it looks, because the trigger is nearly unreachable. egress-proxy/entrypoint.sh:5 truncates hits.jsonl into existence before mitmdump starts, and docker-compose-egress-proxy.yaml:9-11 holds main behind service_healthy. So "proxy never ran" cannot present as a missing file; missing is effectively equivalent to "artifact copy failed". That is worth catching, but it is a narrower claim than the one being made.
There is also a stronger signal already on disk that this PR passes over: Harbor's download_artifacts is explicitly best-effort (artifact_handler.py:182) and records status="failed" in artifacts/manifest.json when it gives up (:365-377). Reading that status distinguishes "the proxy container exited before collection" from "docker exec hiccuped under load"; inferring from file presence cannot, and failureReason: 'egress audit log missing' collapses both.
Misclassification risk, and why it is not symmetric
Mis-killing a good attempt lands in infra_failed (:725), which result.ts:68-70 and runner.ts:188-204 treat as replaceable — the attempt drops out of the result map rather than scoring zero, and a re-run recovers it. That is the right direction and I want to be clear it does not corrupt scored samples.
But the loss is not random. The only mis-kill channel is sidecar artifact collection failing, and that probability rises with container lifetime and docker concurrency — so it preferentially removes the attempts that ran longest and made the most requests. In a single-arm run that costs sample size.
In the head-to-head cohort spec now in flight it costs more than that. The result map is built per cell (runner.ts:188-204) with no group dimension, so removing one arm leaves the other arm's observation in place: the output is a mismatched observation, not one fewer pair. Since the loss favours the longer-running arm, what remains is biased rather than merely thinner. Pairing integrity is not something the framework promises and this is the consuming spec's problem to handle — flagging it only because this PR is what opens the channel that feeds it.
Also worth stating in the description: the fail-closed is opt-in (:169, Boolean(options.egressProxy)), so a spec without a configured proxy gets neither an audit nor any signal that it lacks one.
Coverage
The Python side is genuinely well tested — seven mutations all went red, including the >= vs > boundary and the torn-line \n repair. The gap is entirely on the TypeScript side and at the seam. Details inline; the short version is that changing readVerification(state, cell, Boolean(options.egressProxy)) to readVerification(state, cell, false) — switching this entire PR off — leaves every test green.
Two pre-existing issues I ran into, neither introduced here and neither blocking: egress_filter.py:24 folds the query string into path_query and :142 writes it to the audit log, so tokens or presigned parameters in a blocked URL end up in an archived artifact and its sha256 inventory; and docker-compose-egress-proxy.yaml:14 mounts /opt/maka-egress read-only into the subject while entrypoint.sh:12 puts mitmproxy's confdir there, leaving the CA private key readable by the subject. Happy to file those separately.
Review assistance: Claude Code (Opus) ran three independent fresh-eye passes — security boundary, benchmark impact, and mutation-based test-effectiveness — each blind to the others, in isolated worktrees. The 251-request fill, the read-only-volume record loss, and the switch-it-off mutation are reproduced probe results; I checked the classification chain through result.ts and runner.ts myself. The pairing-bias point came from the reviewer running the cohort spec.
中文
方向是对的，:719 把 ENOENT 收窄出来是实打实的改进——非 ENOENT 的读取失败现在会浮出来而不是被吞掉。sha256: 前缀也让这个产物和 maka-artifacts.ts:14,119 对齐了，属于修正而非破坏。
有一处阻塞，而且正是标题写的那件事。
标题里的 incomplete 从未真正关住
egress_filter.py:136,148-164 在日志达到 MAX_AUDIT_BYTES 时写入 audit_truncated 标记，这部分是work的——我确认过标记只写一次、被截断的尾行会补 \n、后续命中被丢弃。但 grep audit_truncated packages/eval/src/ 没有任何结果，TypeScript 侧只判断文件在不在。
于是一份被截断的日志——「不完整」的字面定义——返回 missing: false，attempt 带着完整 reward 通过。填满 1 MiB 只需要 251 次被拦截请求（normalizedPath 上限 4096 字节，egress_filter.py:142）；此后每一次污染尝试都不留痕迹。也就是说，一个疯狂访问题目仓库直到把日志刷爆的 subject，反而比只碰两次的更不容易被抓到。
Python 和 TypeScript 两侧的测试在这件事上都是绿的，而这恰恰是问题所在：它们覆盖的是一份有生产者、没有消费者的契约。要么让 collectEgressAuditArtifact 解析尾行的 audit_truncated，要么把标题改成 "missing"。
policy_error 值得一并决定（egress_filter.py:107-119）。过滤器自身抛异常时，它对所有请求返回 503 并记下这个 ruleId——TS 侧同样不读。过滤器全面失效会让 subject 的每个请求都失败、任务大面积失败，最终落到 subject_failed，而那是计入分母的 reward 0。这是这一带唯一能真正压低分数的 harness 故障，而它恰好绕开了本 PR 的 fail-closed。
这道 fail-closed 目前证明了什么
比看上去少，因为触发条件几乎不可达。egress-proxy/entrypoint.sh:5 在 mitmdump 启动前就把 hits.jsonl 截断创建出来，docker-compose-egress-proxy.yaml:9-11 又用 service_healthy 把 main 卡在 proxy 就绪之后。所以「proxy 从未运行」不可能表现为文件缺失；missing 实际等价于「产物拷贝失败」。这值得抓，但比 PR 声称的那个主张窄。
而且盘上已经有一个更强的信号被这个 PR 跳过了：Harbor 的 download_artifacts 明确是 best-effort（artifact_handler.py:182），放弃时会在 artifacts/manifest.json 里记 status="failed"（:365-377）。读这个 status 能区分「代理容器在收集前已退出」和「docker exec 在高负载下瞬时抖动」，而用文件存在性反推做不到区分，failureReason: 'egress audit log missing' 把两者压成了一句话。
误判风险，以及它为什么不是对称的
误杀一个正常 attempt 会落到 infra_failed（:725），而 result.ts:68-70 和 runner.ts:188-204 把它当作可替换——该 attempt 从结果 map 里掉出去，而不是记 0 分，重跑可以恢复。这个方向是对的，我想明确说清它不会污染已得分的样本。
但这个丢失不是随机的。唯一的误杀通道是 sidecar 产物收集失败，而这个概率随容器存活时长和 docker 并发压力上升——于是它优先剔除的恰恰是跑得最久、请求最多的那些 attempt。单臂跑分下这只是损失样本量。
而在正在推进的头对头 cohort spec 里，代价不止于此。结果 Map 是逐 cell 构造的（runner.ts:188-204），没有 group 这个维度，所以剔除一臂时另一臂的观测原样留在数据里：产出的是一个失配观测，而不是少一个配对。又因为丢失偏向跑得久的那一臂，残留下来的是有偏的，而不只是变稀疏。配对完整性并不是框架承诺的东西，这该由消费该 spec 的一方处理——在这里提，只是因为本 PR 正是打开这条通道的地方。
另外值得写进描述：这道 fail-closed 是 opt-in 的（:169 的 Boolean(options.egressProxy)），没有配置 proxy 的 spec 既拿不到审计，也不会收到任何「你没有审计」的信号。
覆盖
Python 侧测试质量确实好——七条 mutation 全部变红，包括 >= 与 > 的边界，以及被截断行补 \n 的修复。缺口完全在 TypeScript 侧和两侧接缝上。细节在行内；简版是：把 readVerification(state, cell, Boolean(options.egressProxy)) 改成 readVerification(state, cell, false)——等于把整个 PR 关掉——所有测试仍然全绿。
另有两个既有问题，都不是本 PR 引入的，也都不阻塞：egress_filter.py:24 把 query string 折进 path_query，:142 将其写入审计日志，于是被拦截 URL 里的 token 或预签名参数会进入归档产物及其 sha256 清单；以及 docker-compose-egress-proxy.yaml:14 把 /opt/maka-egress 只读挂载进 subject，而 entrypoint.sh:12 把 mitmproxy 的 confdir 也放在那里，CA 私钥对 subject 可读。我可以单独开 issue。
评审协助说明：Claude Code (Opus) 跑了三轮相互隔离的 fresh-eye 审查——安全边界、跑分影响、基于 mutation 的测试有效性——彼此不知道对方的发现，各自在独立 worktree 中进行。251 次填满、只读卷下的记录丢失、以及「把开关关掉仍全绿」这三条都是可复现的探针结果；归类链路（经 result.ts 与 runner.ts）是我自己核实的。配对偏倚这一点来自正在推进 cohort spec 的评审者。
7f0f09b to
c30d1f4
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Round two, on c30d1f47f. All five items from the first review landed, and the test gap is genuinely narrowed — every mutation that survived last time reddens now, including switching the whole PR off. EGRESS_AUDIT_DESTINATION closes the hardcoded-path concern end to end.
One thing blocks, and the frame matters more than the defect, so let me start there.
Enforcement and forensics fail independently
Every path out of request() ends in a 451 or a 503, including the one where the filter itself raises (egress_filter.py:115-123). So while the proxy runs, enforcement never degrades whatever the audit log looks like — a blocked request cannot contaminate.
An incomplete audit therefore costs forensics, not validity. Truncation says "at least 252 attempts", which is more information about a bad subject. policy_error says "this request was 503'd", which is enforcement working.
That is the axis mismatch below. EvalResultStatus encodes attribution; classifyEgressAudit classifies evidence completeness; the PR maps the second onto the first because infra_failed looks fail-closed. Commit 1 had no such problem — a missing artifact is unambiguously harness-attributed. Commit 2 introduces the first signals where attribution is ambiguous, with no dimension to resolve them.
P1 — both new conditions are subject-triggerable, and infra_failed means "not scored"
isReplaceableAttempt (result.ts:68-70) drops those attempts from the result map instead of scoring them.
egress_filter.py:115 wraps all of request() in one except Exception, and normalize_url raises ValueError by design on ordinary input:
https://example.com/discount-100%25 -> 503, policy_error
https://example.com/50%off -> 503, policy_error
https://example.com/%252525252541 -> 503, policy_error (decode-limit path)
https://example.com/a%20b -> pass
The repository's own tests pin this: test_preserves_unrelated_network_and_rejects_malformed_urls asserts %ZZ raises, and test_audit_is_bounded_and_policy_errors_fail_closed feeds that same URL through request() to produce the record. (Row 1 is a separate pre-existing bug — normalize_url:63-65 re-checks PERCENT_ESCAPE on each iteration's decoded value, so a correctly encoded literal % is rejected on the second pass. Worth fixing, but rows 2 and 3 genuinely are unparseable, so it does not close this.)
Truncation is the same lever: every byte of the 1 MiB cap comes from the subject's own blocked requests, and 252 requests to https://tbench.ai/<4200 chars> emits the marker.
Verified end to end through runAttempt, driving this PR's fake supervisor with AgentTimeoutError and reward 0:
clean -> subject_failed scored: true
policy -> infra_failed scored: false
truncated -> infra_failed scored: false
So a subject heading for a zero escapes it by touching one malformed URL. And since runner.ts:150-204 runs one attempt per cell per invocation and drops replaceable ones, a deterministic trigger is not a mulligan — the cell is never scored at all. This lands in experiments/terminal-bench-2.1-deepseek-v4-flash-{four,eight}-arm.json, which both configure egressProxy; the result map has no group dimension, so dropping one arm leaves the other's observation in place, and truncation is reachable only by the arm making contamination requests.
Not by patching harness-executor.ts:772 — that precedence is right and I withdraw my first-round objection to it. Two shapes that work:
- Record instead of judge.
truncatedandpolicyErrorCountbecome fields on theegress-auditartifact; status returns to commit 1's rule. Both findings vanish structurally: nothing in the log's contents can move an attempt into a replaceable state. Cost: the title becomes "missing". - Prove liveness. Content classification is tempting because an empty log is ambiguous — clean trial, or filter never ran. Have the addon write one
filter_readyline at load. Then empty is a failure,filter_readyalone is a proven clean trial, and everything after is forensics. More work, and it deliberately invertsegress-audit.test.ts's "empty file is a clean trial" case, so it may want its own PR.
Worth deciding now either way: the first is a step toward the second, and partitioning ruleIds by attribution is a step away from it.
P2 — the audit cannot prove the filter ran
Not this PR's to fix, but it is what makes the frame matter. The compose healthcheck asserts the CA file exists and something accepts TCP on 8080; it does not assert egress_filter.py loaded, and --quiet suppresses load diagnostics. If mitmdump serves traffic after a script load failure, an addon raising at import gives: gate green → traffic unfiltered → empty log → completed, indistinguishable from clean. Unverified — no mitmproxy in this environment. One docker run with a raising addon settles it; happy to file it either way.
Narrower and verified: with a pre-existing but unwritable hits.jsonl (the realistic shape, since entrypoint.sh:8 always pre-creates it), a real tbench.ai hit gives 503, policy_error, file contents '' — append_audit raises, the nested retry raises again and is swallowed at egress_filter.py:124-127. The block happened; nothing recorded it.
P2 — two surviving mutations
- Restricting
classifyEgressAuditto the last record —pass 13, fail 0. No test has apolicy_errorfollowed by a normal record, which is the ordinary case. - Reversing
subjectExceptionandaudit.failureReasonin the status ternary —pass 13, fail 0. No fake result carries an exception, so the precedence at the centre of the P1 is unpinned.
Same shape: every test supplies one fact at a time, never a subject fact and a harness fact together.
P3
harness-executor.ts:758-766— the unreadable-audit early return emits no egress artifact entry, unlike the missing case at:690-694.harness-executor.ts:755-756—egressAudit = undefinedis dead; the catch collapses toif (expectEgressAudit && code !== 'ENOENT') return {...}.
Checked clean: the subject cannot inject audit records (ensure_ascii=True escapes newlines, urlsplit strips \t\r\n); the whole-file scan matches the Python tail probe; the Python side reddened on five further mutations.
One thing I own: my first round described policy_error as a filter-wide failure, and what landed escalates a single-request rejection the same way. The gap is in what I asked for.
Review assistance: Claude Code (Opus 5), plus independent adversarial passes by Codex and a separate agent in isolated worktrees, each given my findings as claims to falsify. Codex refuted my proposed fix and my objection to the status precedence and found the two surviving mutations; the other agent found the decode-limit trigger and the unwritable-audit path. Every reproduction above I re-ran myself. The mitmdump conditional is explicitly unverified. AI-assisted review; not independent human review.
中文
第二轮，针对 c30d1f47f。第一轮五条都落实了，测试缺口也真的收窄——上轮存活的 mutation 现在全部变红，包括「把整个 PR 关掉」那个。EGRESS_AUDIT_DESTINATION 端到端闭合了硬编码路径的问题。
有一处阻塞，但框架比缺陷本身更要紧，所以从框架讲起。
执行与取证独立失效
request() 的每一条出口都是 451 或 503，包括过滤器自身抛异常的那条（egress_filter.py:115-123）。所以只要代理在跑，无论审计日志长成什么样，执行都没降级过——被拦下的请求不可能造成污染。
于是审计不完整损失的是取证，不是有效性。截断说的是"至少 252 次尝试"，这是关于一个坏 subject 的更多信息。policy_error 说的是"这次请求被 503 了"，那正是执行在生效。
这就是下面那条发现底下的轴错位。EvalResultStatus 编码的是归属，classifyEgressAudit 分类的是证据完整性，本 PR 把后者映射到前者，因为 infra_failed 看起来最 fail-closed。第一个提交没有这个问题——产物缺失的归属是明确的。第二个提交第一次引入归属含混的信号，而系统里没有解析归属的维度。
P1 — 新增的两个条件都能由 subject 触发，而 infra_failed 意味着「不计分」
isReplaceableAttempt（result.ts:68-70）会把这类 attempt 从结果 map 里剔除而不是计分。
egress_filter.py:115 用一个 except Exception 罩住整个 request()，而 normalize_url 是按设计对普通输入抛 ValueError 的：
https://example.com/discount-100%25 -> 503, policy_error
https://example.com/50%off -> 503, policy_error
https://example.com/%252525252541 -> 503, policy_error （解码上限路径）
https://example.com/a%20b -> 放行
仓库自己的测试把这件事钉住了：test_preserves_unrelated_network_and_rejects_malformed_urls 断言 %ZZ 抛异常，test_audit_is_bounded_and_policy_errors_fail_closed 又拿同一个 URL 走 request() 产生记录。（第一行是一个独立的既有 bug——normalize_url:63-65 对每轮迭代已解码的值重查 PERCENT_ESCAPE，于是正确编码的字面 % 在第二轮被拒。值得修，但第二、三行确实无法解析，所以修它关不掉这条。）
截断是同一个杠杆：1 MiB 上限里每个字节都来自 subject 自己被拦截的请求，252 次对 https://tbench.ai/<4200 字符> 的请求即可写出标记。
用本 PR 的 fake supervisor 灌 AgentTimeoutError 和 reward 0，端到端验证过 runAttempt：
clean -> subject_failed 计分: 是
policy -> infra_failed 计分: 否
truncated -> infra_failed 计分: 否
也就是说，本来要拿零分的 subject，碰一个畸形 URL 就能逃掉。又因为 runner.ts:150-204 每次调用每个 cell 只跑一个 attempt 并剔除可替换的那些，确定性的触发不是「重跑一次就好」——这个 cell 根本不会被计分。这落在 experiments/terminal-bench-2.1-deepseek-v4-flash-{four,eight}-arm.json 上，两个 spec 都配了 egressProxy；结果 map 没有 group 维度，剔除一臂时另一臂的观测原样留下，而截断只可能由发起污染请求的那一臂触达。
修法不是去改 harness-executor.ts:772——那里的次序是对的，我撤回第一轮的异议。两种可行形状：
- 记录，而不是判决。
truncated和policyErrorCount变成egress-auditartifact 上的字段，状态判定退回第一个提交的规则。两条发现于是结构性消失：日志内容里的任何东西都无法把 attempt 推入可替换状态。代价是标题改成 "missing"。 - 证明存活。 内容分类之所以诱人，是因为空日志含混——干净跑，还是过滤器根本没起来。让 addon 在加载时写一行
filter_ready：空文件即失败，只含filter_ready即被证明干净，其后一切都是取证。工作量更大，而且会有意地把egress-audit.test.ts里"空文件是干净跑"那条断言反过来，也许该单开 PR。
无论选哪个都值得现在就定：前者是朝后者走的一步，而按归属拆分 ruleId 是背离它的一步。
P2 — 审计无法证明过滤器跑起来了
不该由本 PR 修，但它正是让上面那个框架成立的东西。compose 的 healthcheck 只断言 CA 文件存在、8080 上有东西接受 TCP，并不断言 egress_filter.py 加载成功，而 --quiet 抑制了加载诊断。如果 mitmdump 在脚本加载失败后仍转发流量，一个 import 期就抛异常的 addon 会给出：健康门通过 → 流量未过滤 → 空日志 → completed，与干净跑无法区分。未经验证——本环境没有 mitmproxy。 一条 docker run 配一个会抛异常的 addon 即可定论；无论结果如何我都可以单独开 issue。
更窄但已验证的一种：用已存在但不可写的 hits.jsonl（真实形态，因为 entrypoint.sh:8 总会预先创建它），一次真实的 tbench.ai 命中给出 503, policy_error, 文件内容 ''——append_audit 抛出，嵌套重试再抛，被 egress_filter.py:124-127 吞掉。拦截发生了，没有任何记录。
P2 — 两个存活的 mutation
- 把
classifyEgressAudit限制为只看最后一条记录——pass 13, fail 0。没有测试构造「policy_error后面跟着正常记录」，而这是常态。 - 把状态三元里的
subjectException和audit.failureReason互换——pass 13, fail 0。没有 fake result 带 exception，所以那条 P1 的核心次序没被钉住。
形状相同：现有测试每次只喂一个事实，从没让 subject 事实和 harness 事实同时在场。
P3
harness-executor.ts:758-766——不可读审计的提前返回没有输出任何 egress artifact 条目，与:690-694的 missing 路径不对称。harness-executor.ts:755-756——egressAudit = undefined是死赋值；catch 可收敛成if (expectEgressAudit && code !== 'ENOENT') return {...}。
查过是干净的：subject 无法注入审计记录（ensure_ascii=True 转义换行，urlsplit 剥掉 \t\r\n）；whole-file 扫描与 Python 尾部探针一致；Python 侧另外五个 mutation 全部变红。
有一件我该认领：第一轮我把 policy_error 描述成过滤器整体失效，落地实现把单次请求被拒也做了同样升级。缺口在我提的要求里。
评审协助说明：Claude Code (Opus 5)，外加 Codex 与另一个 agent 在隔离 worktree 中各自独立的对抗性审查，都拿到我的发现作为待证伪的 claim。Codex 推翻了我提出的修法和我对状态次序的异议，并找出那两个存活的 mutation；另一个 agent 找到解码上限触发路径和不可写审计路径。上面每一处复现我都自己重跑过。mitmdump 那个条件句已明确标注未经验证。这是 AI 协助的评审，不构成独立人工评审。
|
Addressed in 45182ec.
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Round 3 on 45182ecf1, reviewed fresh by two independent arms given the diff and the runtime semantics but none of my earlier conclusions. 16 TS + 5 Python green, biome clean. Nothing blocking below.
The P1 is closed. The probe that failed last round now passes: policy_error + AgentTimeoutError → subject_failed, score 0, kept in the result map; missing + the same exception still → infra_failed. Both P3s are fixed and the last-record-only mutation is now red. "Record, don't judge" was the right one of the two shapes.
I was wrong about the healthcheck — don't build filter_ready for the reason I gave
I installed mitmproxy 12.2.3 (your image tag) and ran it: an addon that raises on import, is missing, or has a syntax error all make mitmdump exit rc=1 without listening. entrypoint.sh execs it as PID 1, so a load failure kills the container, the healthcheck's create_connection fails, and depends_on: service_healthy never releases main. Port reachability already proves the addon loaded. That hole doesn't exist.
filter_ready still earns its keep, for the restart case below instead.
P2 — request()'s blocking path has no test; five fail-open mutations stay green
request() is invoked once in the whole Python suite (test_egress_filter.py:74), only down the exception branch. Matched URL → 451 → one audit record is never exercised. Each mutation on a clean baseline, restored after — all 5/5 OK:
if not matched: return → if matched: return every contamination URL passes through
append_audit(...) → pass nothing ever recorded
451 → 200
flow.response = ... → dropped
normalized_path[:4096] → normalized_path[:1]
The second one matters most: with no record written, the TS side reads 0 bytes and returns completed. The whole evidence chain can be deleted and neither suite makes a sound. One test — hit URL through request(), assert 451, the rule header, and exactly one appended record — kills all five.
P2 — entrypoint.sh:8 destroys the audit log on restart
: > "$STATE_DIR/hits.jsonl" truncates. A proxy restart mid-trial erases every hit so far; the trial finishes normally and the artifact lands at 0 bytes, indistinguishable from an honest clean run. touch instead of : > fixes the destruction, and this is where a startup record makes an empty log decidable rather than ambiguous.
P2 — record integrity is never checked
Half a JSON object, raw garbage, invalid UTF-8, and valid-but-not-an-object all yield forensics identical to a healthy log (failureReason=null, truncated=false, policyErrorCount=0); inspectEgressAudit silently continues past every unparseable line. That was fine while the log was only evidence — now that the design is "record, don't judge", the record's trustworthiness is the deliverable.
P2 — ensure_ascii=True is the only thing stopping line injection, and nothing tests it
Flipping it to False survives. With it flipped, a path containing U+2028 makes Python's splitlines() see two lines where TS's split(/\r?\n/) sees one, fooling audit_already_truncated into duplicate markers. The hardening is right; it's just one keystroke from gone with no test to catch it.
P3
Status precedence isn't pinned — three surviving mutations including reordering audit.failureReason after subjectException; the missing cells are auditMode: 'missing' × AgentTimeoutError and missing × absent reward. And policyErrorCount += 1 → = 1 survives (both arms found this independently).
Worth confirming before merge
main:693 was readFile(...).catch(() => undefined) — a missing file changed nothing. This PR makes it infra_failed, so the system now depends on Harbor producing that file when a cell has zero hits, and nothing has ever verified that: the tests use a fake supervisor that writes the file itself, so real collection is never in the loop. docker cp semantics make it very likely fine, which is why this isn't higher — but one real zero-hit cell would settle it and the downside is total.
Pre-existing on main, not this PR's to fix — an issue each
normalize_url rejects legitimately encoded URLs: https://api.example.com/search?q=50%25 raises malformed percent escape, so an innocent request gets a 503 and the subject loses network it should have had — the decode loop at :63-65 re-runs PERCENT_ESCAPE against the already-decoded value. (MAX_DECODE_PASSES = 4 also accepts only 3.) Separately, policy_error still conflates a by-design ValueError from the subject's URL with append_audit IO failures and real filter bugs. Neither regressed here; if anything policyErrorCount finally makes the first one visible.
Checked clean
13 injection URLs across both arms (raw and encoded newlines, CRLF, U+2028, NEL, U+001C, JSON breakout) each land as a single well-formed record with no forged ruleId. The subject can't reach the audit file directly — /opt/maka-egress-state is in the proxy's writable layer and the only named volume is :ro to main. Percent-encoding can't evade the rule table; matching happens on the decoded value.
中文
第三轮，基于 45182ecf1，两臂独立 fresh-eye 复审：只给 diff 和运行时语义，不给我此前的结论。16 TS + 5 Python 全绿，biome 干净。以下没有阻塞项。
P1 已关闭。 上轮失败的探针现在通过：policy_error + AgentTimeoutError → subject_failed，score 0，保留在结果集；missing + 同样的异常仍然 → infra_failed。两个 P3 已修，"只看最后一条"变异现在会红。两种形状里选"记录而非判决"是对的。
healthcheck 那条我错了，别按我给的理由做 filter_ready。 我装了 mitmproxy 12.2.3（你镜像 tag 的版本）实测：addon 导入抛异常、文件缺失、语法错误，三种都让 mitmdump rc=1 退出且不监听端口。entrypoint.sh 用 exec 让它当 PID 1，加载失败即容器死，healthcheck 的 create_connection 必然失败，depends_on: service_healthy 就不会放行 main。端口可连接本身已经证明 addon 加载成功，那个洞不存在。filter_ready 仍然值得做，但理由是下面的重启场景。
P2 — request() 的阻断路径零测试，五个 fail-open 变异全绿。 request() 在整个 Python 套件里只被调用一次（test_egress_filter.py:74），且只走异常分支；命中 URL → 451 → 写一条审计记录这条主路径从未被驱动。每个变异都在干净基线上跑、跑完还原，全部 5/5 OK：反转匹配（污染 URL 全放行）、删掉 append_audit（一条都不记）、451 改 200、丢掉 flow.response 赋值、normalized_path[:1]。第二个最要紧：一条记录都不写，TS 侧读到 0 字节判 completed，整条证据链可以被静默删除而两套测试都不响。一个测试（命中 URL 驱动 request()，断言 451、rule 头、恰好追加一条记录）能一次关掉全部五个。
P2 — entrypoint.sh:8 会在重启时销毁审计日志。 : > "$STATE_DIR/hits.jsonl" 是截断。代理中途重启会抹掉此前所有命中，trial 正常结束，产物落地 0 字节，与一次诚实的干净运行不可区分。改成 touch 即可，而这正是启动记录真正有价值的地方：让空日志从不可分辨变成可判定。
P2 — 记录完整性从未被校验。 半条 JSON、纯垃圾、非法 UTF-8、合法但非对象，全部产生与健康日志相同的取证结果（failureReason=null, truncated=false, policyErrorCount=0）；inspectEgressAudit 对每一行解析失败都静默跳过。日志只作证据时无所谓；既然设计已是"记录而非判决"，记录本身的可信度就是交付物。
P2 — ensure_ascii=True 是行注入防护的唯一支点，且无测试。 改成 False 是存活变异。改掉之后路径里的 U+2028 会让 Python 的 splitlines() 看到两行、而 TS 的 split(/\r?\n/) 仍是一行，audit_already_truncated 被骗、重复写入截断标记。加固是对的，只是离消失只有一个键、且没有测试兜着。
P3。 状态优先级没被钉住——三个存活变异，含把 audit.failureReason 排到 subjectException 之后；缺的是 auditMode: 'missing' × AgentTimeoutError 和 missing × 无 reward 两格。另外 policyErrorCount += 1 → = 1 存活（两臂独立发现）。
合并前值得确认一条。 main:693 原本是 readFile(...).catch(() => undefined)，文件缺失什么都不影响。本 PR 把它变成 infra_failed，于是系统现在依赖"Harbor 在 cell 零命中时确实产出该文件"，而这从未被验证：测试用的 fake supervisor 自己写文件，真实收集从不在回路里。docker cp 的语义让它大概率没问题，所以没定更高——但一个真实的零命中 cell 就能定论，而反面代价是全损。
既存于 main、不该由本 PR 修——各建议开一个 issue。 normalize_url 拒绝合法编码的 URL：https://api.example.com/search?q=50%25 抛 malformed percent escape，于是无辜请求吃到 503、受试者失去本该有的网络——:63-65 的解码循环对已解码的值重跑 PERCENT_ESCAPE。（MAX_DECODE_PASSES = 4 也只实际接受 3 层。）另外 policy_error 仍然混合了来自受试者 URL 的设计内 ValueError、append_audit 的 IO 故障和真正的过滤器 bug。两条在本 PR 都没有回归；反倒是 policyErrorCount 让第一条终于可见。
检查干净的部分。 两臂合计 13 个注入 URL（裸换行与编码换行、CRLF、U+2028、NEL、U+001C、JSON 结构突破）每一个都落成单条合法记录，无伪造 ruleId。受试者也无法直接触碰审计文件：/opt/maka-egress-state 在代理容器可写层，唯一具名卷对 main 是 :ro。百分号编码无法绕过规则表——匹配发生在解码后的终值上。
AI-assisted review. Claude Code (Opus 5) ran the review and wrote this comment; Codex and a separate agent each reviewed the same head independently and fresh. Every claim here was re-run by me on a clean baseline before inclusion, including the mitmproxy experiment that retracts my own round-2 finding; several of the arms' claims were downgraded or dropped when I couldn't reproduce them as stated. The Harbor-collection assumption is explicitly unverified. This is not independent human review.
|
Addressed in ca7254e.
Left out, as requested:
|
Truncated audit files now write one audit_truncated record. A missing expected audit is inventoried as egress-audit-missing and marks the cell infra_failed. Present audits use the same sha256: prefix as other collected artifacts. Fixes apache#2959 Generated-by: Grok
Parse audit_truncated and policy_error in collectEgressAuditArtifact so an incomplete or filter-failed log is infra_failed instead of a scored pass. Cover the path through runAttempt and selectCellResult. Treat non-ENOENT audit reads as fail-closed with the artifact path. Ignore non-object JSON tails when probing for the truncation marker. Generated-by: Grok
…scores Log contents stay forensic: truncated and policyErrorCount are fields on the egress-audit artifact. Only a missing or unreadable expected file is infra_failed. A subject timeout with a truncated log remains subject_failed. Generated-by: Grok
…d lines Create the audit file with touch instead of truncating it on start. Count unparseable and non-object lines on the artifact without changing attribution. Cover the request() blocking path, ensure_ascii line injection, and missing-audit status precedence. Generated-by: Grok
ca7254e to
3b1a8a8
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The production findings from the previous round are closed on 3b1a8a8280e2eb2b51aa4e9c32899c09e0bf6cfb; I found no remaining P0–P3 behavior issue.
One merge blocker remains: the required typecheck job fails because packages/eval/src/__tests__/egress-audit.test.ts is not Biome-formatted. The Buffer.from(...) expression around line 97 needs to be formatted as the single line Biome prints. Please run the formatter and let CI cover the resulting head.
Focused verification otherwise passed: the Eval build, the egress-audit integration tests, the Python filter tests, and the lifecycle tests.
AI-assisted review: Codex coordinated three independent review passes covering the audit producer, Host attribution path, and test/CI evidence; the findings and focused validations were checked against the current rebased head. This is not independent human review.
Biome wants the Buffer.from argument on one line. Unformatted source fails the required typecheck job. Generated-by: Grok
|
Addressed in f257193.
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Confirmed on f257193482d6a22fcdf46d76cd2ffe231c172230: the only change since the previously approved head is the Biome formatting fix for the malformed-line fixture. There is no production or test-semantic change.
The previous production findings remain closed. The Eval build, focused egress-audit tests, required formatting check, and current CI all pass. I found no remaining P0–P3 issue.
简体中文
已在 f257193482d6a22fcdf46d76cd2ffe231c172230 上确认：相对之前批准的 head，唯一变化是修复 malformed-line fixture 的 Biome 格式，没有生产代码或测试语义变化。
此前的生产 finding 仍保持关闭。Eval build、focused egress-audit tests、格式检查和当前 CI 均已通过，没有剩余的 P0–P3 问题。
AI-assisted review disclosure: Codex coordinated two independent rereview passes and focused validation. I reviewed the delta and evidence and own the final judgment.
2 participants
Summary
The egress audit log could look clean while the expected file was missing. A consumer could not tell a clean trial from a failed artifact copy.
audit_truncatedrecord and do not append further hits.collectEgressAuditArtifactinventories a present log withtruncatedandpolicyErrorCount. Those fields are forensic; they do not change attempt attribution.egress-hits.jsonlis absent or unreadable, the cell isinfra_failedwith an explicit reason.sha256:prefix ascollectedArtifactInventory.egressProxystill gets neither an audit nor a missing-audit signal.Fixes #2959
Verification
python3.13 packages/eval/harbor/test_egress_filter.py— 5 passnode --test packages/eval/dist/**/*.test.js— 47 pass, includingrunAttemptcases for missing / truncated /policy_error/ policy-then-hit / empty / unreadable, plusAgentTimeoutErrorwith a truncated log remainingsubject_failedNot run: full
maka eval runTerminal-Bench cohort. Harborartifacts/manifest.jsonis not consumed. Filter liveness (filter_ready) is not in this PR.Checklist
Does this PR entail a change in behavior?
infra_failed. Truncation andpolicy_errorstay on the artifact and do not drop a scored attempt.