fix(core): redact error text in usage-statistics telemetry sink - #11649
Conversation
github-actions
Bot
added
the
review/self-reported
887b430 to
7df466e
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Re-run on the same head again — still Template looks good ✓ — all nine headings from Stage 1-pre: #11198 is OPEN ( Problem: observed, not theoretical, and re-verified in the base tree this pass rather than carried forward. Direction: telemetry is one of the areas this gate escalates rather than decides, so the escalation stands — @zjunothing is assigned and is the sole Size: core path → Stage 0 applies. Production 110 lines ( Approach: matches the proposal I wrote before opening the diff — same choke point, and it reuses One question that still stands, neither new nor a blocker: the description claims "any error-text key — including ones added later — is scrubbed". The implementation is a static allowlist ( Risk: no Stage 1e high-risk path match — both changed files are telemetry. Input-side blowup is bounded: Moving on to code review. 🔍 中文说明又是同一个 head 上的重跑 —— 仍是 模板完整 ✓ —— Stage 1-pre：#11198 处于 OPEN（ 问题： 已观测到的，不是理论性的，而且这一轮是在 base 代码里重新核实的，不是照搬上一轮。在 方向： 遥测属于本 gate 只转交、不自行判定的领域，所以转交继续有效 —— @zjunothing 已被指派，且是 规模： 触及核心路径 → 适用 Stage 0。生产代码 110 行（ 方案： 与我在打开 diff 之前写下的方案一致 —— 同一个收口点，并且复用了 有一个问题依然存在，不是新问题也不是阻断项：描述里说"任何错误文本字段 —— 包括以后新增的 —— 都会被清除"。实现是一个静态 allowlist（ 风险： Stage 1e 高风险路径无命中 —— 两个改动文件都是遥测。输入侧的爆炸是有界的： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Reviewed statically at Code reviewI wrote my own proposal before opening the diff (sink-side choke point; reuse
Completeness claims verified rather than trusted, in the base tree this pass:
|
| input | value length | outcome at head | why |
|---|---|---|---|
mysql --password hunter2 |
7 | leaks verbatim | unquoted alt needs 1 + {9,} = ≥10 |
--token abc123 |
6 | leaks verbatim | same |
--api-key sk-12345 |
8 | leaks verbatim | same |
--password="pw123" |
5 inside quotes | leaks verbatim | quoted alt needs {10,}; unquoted alt refuses a leading " |
--aws-access-key AKIA_testsecret123 |
18 | redacted | clears the floor |
Neither of the other two patterns rescues these: ENV_SECRET_PATTERN requires key=value (these are space-separated), and AUTHORIZATION_PATTERN requires the literal authorization. So the shapes fall through entirely.
This matters because --password and --token are named in the PR description as covered vectors ("secret-bearing flags (--token, --_authToken, --password)"). A reader of that description would believe a short password is scrubbed. It is not.
Two baselines, stated separately, because they give different answers and conflating them is how this got mis-filed:
- Against
main(the merge base): not a regression. Base has zero redaction; head closes all 8 vectors the description names for realistic long credentials./verifymeasured real-credential leaks 41/41 at base → 23/41 at head. Merging this is still a strict improvement. - Against the branch's own earlier revisions: a regression, and a measured one.
/verifybuilt566d6a74and31827e9das separate arms. Both redacted 12/12 short-flag shapes and sat at 10/41 real-credential leaks. Head redacts 3/12 and sits at 23/41. Both earlier revisions used the unboundedSECRET_VALUEfor flags; the last commit swapped inSECRET_FLAG_VALUEto stop over-matching--max-tokens 8192, and the floor overshot.
The fix is measured, not conjectural. /verify built a fifth arm with the floor lowered to 5 ({4,}) and kept the {0,64} bounds: 52/52 assertions pass, real-credential leaks 23/41 → 14/41, all 9 reopened shapes re-closed, all 8 description vectors stay redacted, benign text byte-identical 12/12, and the delta's own goal survives — --max-tokens 8192 (4 chars) and max_tokens=8192 still pass through untouched. That is the point: floor 5 dominates floor 10 on every axis measured. It is not a trade-off between coverage and over-matching, because 8192 is only 4 characters. One line.
The suite cannot see it — which is why it survived to head
This is the part I would treat as blocking-adjacent on a P1 security fix. Every positive redaction assertion in the new tests uses a value at or above the floor:
aws --aws-access-key AKIA_testsecret123(18 chars)npm publish --_authToken npm_testsecret123(18 chars)--password P@ss'w0rd123— the only short-looking one, and the value is actually 12 chars, asserted withnot.toContain('w0rd123')
There is no test anywhere in the 25 new cases with a flag secret under 10 characters. /verify confirmed it empirically: mutant M2 (floor 10 → floor 5) survived green, 69/69. The suite passes identically with either floor, so CI cannot catch this class of change in either direction — that is the "green and worthless" shape, and it is why a regression could land in the final commit of a security PR without anything turning red.
The cheap repair is a fixture pair that pins the boundary from both sides: a short secret that must redact (--password hunter2) and a short non-secret that must not (--max-tokens 8192). /verify wrote one and measured it RED at head / GREEN at floor 5.
What still stands (all non-blocking)
- The claim outruns the implementation. "Including ones added later" is not true of a static three-key allowlist, and no guard test was added. This is the difference between closing the class and closing today's instances — and closing the class is the PR's entire argument for the sink layer. Relatedly,
error_excerptin that allowlist has zero producers inpackages/*/src(repo-wide census: 1 hit, the declaration itself; mutant M8 survived green). Forward-looking for the still-open fix(core): halt turns on repeated identical tool errors #10916 — harmless, but by this repo's simplicity rule it is speculative code and shouldn't be described as covering something live. - One narrow fail-open. A secret flag value whose first quoted run is shorter than the floor and contains an escaped same-type quote —
--password="ab \"cd\" efghij"— matches none of the three value alternatives, so it passes through. Unusual shape; theAuthorizationpath has no floor so it is unaffected. - Quotes are consumed, not preserved. Measured 4/4 shapes unbalanced, wider than round 1 reported:
--_authToken "npm_…"and--password="…"lose both quotes (2→0). Predates the delta (from31827e9d), and the PR's own test asserts the unbalanced string as expected, so it is deliberate. Diagnostic impact only — the secret is genuinely gone. - JSON-quoted header names (
{"authorization":"Bearer x"},{"GITHUB_TOKEN":"…"}) don't match, because a"sits between name and separator. Reported in an earlier round, declined by the author; part of the 10-shape residual (F1) that base also leaks — incompleteness, not a regression. - Bare-newline-split URL credentials (
https://user:\npass@host) are no longer caught, since newlines are now deliberately preserved. Shell puts the command on one line, so this isn't the issue's vector — recording it as the visible cost of the (correct) newline fix. - A redundant but harmless second pass. After
SECRET_FLAG_PATTERNwrites--token=***REDACTED***,ENV_SECRET_PATTERNre-matches it and replaces it with the identical string. Idempotent — just a wasted pass worth knowing about if the pattern set grows. {0,64}bounds are load-bearing but unpinned./verifymeasured them at 121.9 ms → 0.7 ms at the 25k production ceiling, and mutants M4/M5/M9 all survived green. The bound is real protection with no test on it.
Nothing here is a correctness or security regression against main. Measured against base, the leak surface is strictly smaller. The finding above is about how much smaller it could be for one line, and about a suite that cannot tell the difference.
CI test evidence
This is the PR's own CI on the reviewed commit, fetched through the API just now. I ran nothing myself. Required Linux checks are green; the picture is unchanged from my last pass.
The caveat is coverage, not correctness: Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), and Integration Tests (CLI, No Sandbox) are all skipped on this head, so the 25 new redaction tests have executed on Linux only. For a pure string-transformation function with no platform-dependent API that is a low-risk gap — but it is a gap, and I am not going to describe Linux-only as cross-platform. The description marks Windows /verify likewise drove an LF-only corpus and did not exercise CRLF end to end (the continuation joiner does handle \\\r?\n).
review-pr is in_progress; it is a pull_request_target bot job and is correctly excluded from the PR's own CI count (PENDING = 0).
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Lint & Static (ubuntu-latest, Node 22.x) | success |
| Integration Tests (no-AK, No Sandbox) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| Classify PR | success |
| Test (macos-latest, Node 22.x) | skipped |
| Test (windows-latest, Node 22.x) | skipped |
| Integration Tests (CLI, No Sandbox) | skipped |
| review-pr (pull_request_target) | in_progress |
Sandboxed verification — already run, and it is what changed this pass
Unlike my last pass, I no longer have to name /verify as the remedy for an unsettled claim: it ran, twice, and round 2 is substantive. Advisory evidence, not a review or a CI check — but it is A/B measurement against real built arms, which is strictly more than static review can give, and I have hand-checked its central finding against the diff rather than taking it on faith (the regex table above is mine, not quoted).
What it settled:
- The redaction is load-bearing. Removing it moves the wire payload from clean to leaking; the four enqueue carriers (
properties.error_message, top-levelmessageon both exception and resource events,properties.error) are clean at head and leaking at base. The choke point is genuinely single. - The floor regression is real and is one line to fix (measured
fix5arm above). - The suite cannot see the floor (M2 survived 69/69) — the mutation result that makes the missing boundary fixture a concrete ask rather than a nit.
- The scary guesses do not hold.
stackbypass disproved (nostack:assignment anywhere inpackages/core/src/telemetry/, non-test — which also means the PR's declaredstackexclusion excludes nothing); retry-path bypass disproved (pushpost-redaction,unshiftre-queues redacted events); in-place mutation corrupting a caller disproved (42 internal call sites, external callers only in tests); ReDoS disproved and now hardened by the bounds.
What it did not cover, and I am not going to paper over it:
- Trial merge into current
main.mainwas unreachable in the verify container, so conflict-freedom is unconfirmed and nothing was re-measured on a merged tree. Worth a maintainer re-check before landing, since this branch has sat a day. - Full
packages/coresuite,npm run lint, repo-levelnpm run typecheck, and all integration tests — only the focusedqwen-logger.test.ts(69/69 at head) andtsc --noEmitforpackages/core(exit 0, proven live by planting a type error) were run. - No live network flush:
flushIfNeeded/flushToRumwere hard-blocked and counted (63 blocked attempts per arm). The oracle iscreateRumPayload(), whichflushToRum()serializes verbatim — the wire body was reconstructed, not captured off a socket. - The OTLP path (
attributes['error.message']inloggers.ts, plussession-tracing.ts/daemon-tracing.tsspans) is a separate, opt-in sink this PR neither touches nor claims. Still unredacted, still divergent from this policy, still with no tracking issue.
A re-run of @qwen-code /verify after the floor fix is what would settle the last gap — specifically that the 9 reopened shapes close and that --max-tokens 8192 still passes through. The author holds admin, so both /verify and /tmux are available; /verify is the one that matches this claim.
中文说明
在 41714d9f 上做静态审查。按本 gate 的规则，我没有构建、运行或执行本 PR 的任何代码 —— 下面所有关于测试的陈述，要么是通过 API 读取的本 PR 自己的 CI，要么是把第二轮 /verify 报告当作证据、再手工对照 diff 复核过的结论。凡是我无法核实的都明确标注。
代码审查。 我在打开 diff 之前先写了自己的方案（sink 侧收口；复用 redactUrlCredentials + stripAnsiAndControl；高置信形状 allowlist；给每个量词加上界；在匹配之前归一化 ANSI 与续行；断言"密钥不存在"而不是"标记不存在"）。PR 落在同一个设计上。/review 第二轮那 5 条 Critical 在当前 head 上确实关闭了，我逐条从 diff 和 base 代码重新推导：续行泄漏（R1-5）已关闭，redactTelemetryError 先把 \ + 换行 + 前导空白合并掉；反向断言的测试（R2-1）实质上已关闭，现有 12 条 not.toContain 密钥不存在断言；换行与 URL 凭据的顺序（R1-7）已关闭且是承重的，stripAnsiAndControl 逐行应用，因为 CONTROL_CHARS_RE 确实会删掉 \n；引号感知（R2-2）就已报告形状关闭；精确拼写缺口（R1-4）已关闭，关键词位于 {0,64}?…{0,64} 之间。
完整性声明这一轮是在 base 代码里核实的，不是采信：:191 的 this.events.push(event) 是唯一准入写入，:188 shift 与 :1162 pop 只淘汰，:1158 unshift 回放已脱敏事件；main 上完全没有脱敏（grep redact 零命中），所以"泄漏面严格小于 main"不是两个部分状态之间的比较，而是与零比较；就地修改不会污染调用方对象；失败时 fail-closed（脱敏是 try 里的第一条语句）；对顶层 message 的无差别脱敏今天是安全的（只有三个生产者，其中 :708 不含密钥）。
SECRET_VALUE（Authorization 路径）没有下限，而 SECRET_FLAG_VALUE 有 10 字符下限（{9,} 加首字符）。手工推导：mysql --password hunter2（7 字符）、--token abc123（6）、--api-key sk-12345（8）、--password="pw123"（引号内 5）全部原样泄漏；而 --aws-access-key AKIA_testsecret123（18）被脱敏。另外两个模式也救不了它们：ENV_SECRET_PATTERN 要求 key=value（这些是空格分隔），AUTHORIZATION_PATTERN 要求字面的 authorization。之所以重要：--password 和 --token 是PR 描述里点名认领的覆盖向量，读到那句描述的人会以为短口令会被清除 —— 实际不会。
两个基线要分开说，因为它们给出不同答案，而把它们混在一起正是我上一轮归错类的原因：
- 相对
main（合并基线）：不是回退。 base 完全没有脱敏；head 对真实长凭据关闭了描述点名的全部 8 个向量。/verify实测真实凭据泄漏 base 41/41 → head 23/41。合并它仍然是严格改善。 - 相对本分支自己更早的版本：是回退，而且是实测的。
/verify把566d6a74和31827e9d各自构建成独立对照臂，两版都脱敏 12/12 个短取值 flag 形态、泄漏数都是 10/41；head 只脱敏 3/12、泄漏 23/41。两个早期版本对 flag 用的是无下限的SECRET_VALUE，最后一个 commit 为了不再误伤--max-tokens 8192换成了SECRET_FLAG_VALUE，而下限 overshoot 了。
修复是实测过的，不是猜想。 /verify 构建了第五个臂：把下限降到 5（{4,}）、保留 {0,64} 边界，结果 52/52 断言通过，真实凭据泄漏 23/41 → 14/41，9 个被重新打开的形态全部重新关闭，8 个描述向量保持脱敏，无害文本逐字节一致 12/12，并且 delta 自己的目标依然成立 —— --max-tokens 8192（4 字符）和 max_tokens=8192 仍原样通过。这正是关键：下限 5 在每一个实测维度上都优于下限 10。它不是"覆盖面 vs 过度匹配"的取舍，因为 8192 只有 4 个字符。一行代码。
测试套件看不见它 —— 这也是它能活到 head 的原因。 在一个 P1 安全修复上，我会把这一条视为接近阻断。新增测试里每一条正向脱敏断言用的取值都在下限之上：AKIA_testsecret123（18）、npm_testsecret123（18），唯一看着短的 --password P@ss'w0rd123 实际是 12 字符、且用 not.toContain('w0rd123') 断言。25 个新用例里没有任何一个使用 10 字符以下的 flag 密钥。 /verify 实测确认：变异体 M2（下限 10 → 5）全绿存活，69/69。两种下限下套件结果完全相同，所以 CI 在任何一个方向上都抓不到这类改动 —— 这就是"绿了但没用"的形状，也是一处回退能落在安全 PR 的最后一个 commit 上而没有任何东西变红的原因。便宜的补救是一对钉住边界两侧的 fixture：一个必须脱敏的短密钥（--password hunter2）和一个必须保留的短非密钥（--max-tokens 8192）。/verify 写了一个并实测：head 上 RED、下限 5 上 GREEN。
仍然存在（全部非阻断）： 1. 说法超出实现 —— "包括以后新增的"对静态三键 allowlist 不成立，且没有守卫测试；error_excerpt 零生产者（全仓库 1 处命中即声明本身，变异体 M8 全绿存活），是为仍然 open 的 #10916 前瞻准备的推测性代码。2. 一处狭窄 fail-open —— --password="ab \"cd\" efghij"。3. 引号被吞而非保留，实测 4/4 形态不配对，flag 形态两个引号全丢（2→0），范围比第一轮报告的更宽；早于 delta，且 PR 自己的测试把不配对字符串断言为期望值，属有意为之，只影响诊断可读性。4. JSON 引号形式的头名不匹配，属 base 也泄漏的 10 形态残差（F1），是不完整而非回退。5. 被裸换行拆分的 URL 凭据不再被捕获，是（正确的）换行修复的可见代价。6. 一次冗余但无害的重复处理（幂等）。7. {0,64} 边界是承重的但没有测试钉住 —— 实测 121.9 ms → 0.7 ms，而变异体 M4/M5/M9 全部存活。
以上没有一条是相对 main 的正确性或安全性回归。真正的问题是：一行代码就能让它变小多少，以及一个分辨不出差别的测试套件。
CI 测试证据。 以上是刚刚通过 API 读取的、本 PR 在被审 commit 上自己的 CI，我本人没有运行任何东西。必需的 Linux 检查是绿的，与我上一轮一致。需要留意的是覆盖面而非正确性：Test (macos-latest, Node 22.x)、Test (windows-latest, Node 22.x) 和 Integration Tests (CLI, No Sandbox) 在这个 head 上全部 skipped，所以 25 个新脱敏测试只在 Linux 上执行过。对不依赖平台 API 的纯字符串变换来说这是低风险缺口 —— 但它确实是缺口，我不会把"仅 Linux"说成"跨平台"。描述里 Windows 与 Linux 都标 /verify 同样只跑了 LF 语料，没有端到端验证 CRLF（续行合并确实处理了 \\\r?\n）。review-pr 处于 in_progress，是 pull_request_target 机器人任务，已正确排除在计数外（PENDING = 0）。
沙箱验证 —— 已经跑过，而且它正是改变这一轮结论的东西。 与上一轮不同，我不必再把 /verify 作为某个未决主张的补救手段点名：它跑了两次，第二轮是实质性的。它只是评审证据，不构成评审或 CI 检查 —— 但它是针对真实构建臂的 A/B 实测，严格强于静态审查能给出的东西，而我对它的核心发现是手工对照 diff 复核过的（上面那张正则表是我推导的，不是引用的）。
它解决的：脱敏是承重的（移除后 wire payload 由干净变泄漏；四个入队载体在 head 上干净、在 base 上泄漏，收口点确实唯一）；下限回退真实存在且一行可修；套件看不见下限（M2 存活 69/69）；几个更吓人的猜测不成立 —— stack 旁路（packages/core/src/telemetry/ 非测试代码中无任何 stack: 赋值，这同时意味着 PR 声明的 stack 排除项其实排除了空集）、重试路径旁路、就地修改污染调用方、ReDoS（且被新边界加固）。
它没有覆盖、我不会掩盖的：与当前 main 的试合并（容器内 main 不可达，因此冲突与否未确认，也没有在合并后的树上重测 —— 这个分支已经放了一天，落地前值得维护者复核）；packages/core 全量套件、npm run lint、仓库级 npm run typecheck 和所有集成测试（只跑了聚焦的 qwen-logger.test.ts，head 上 69/69，以及 packages/core 的 tsc --noEmit，exit 0，并通过植入类型错误证明其有效）；没有真实网络上报（flushIfNeeded/flushToRum 被硬阻断并计数，每臂 63 次；oracle 是 createRumPayload()，wire body 是重建的而非抓包）；OTLP 路径（loggers.ts 的 attributes['error.message']，以及 session-tracing.ts / daemon-tracing.ts 的 span）是本 PR 既不触及也不认领的另一条 opt-in 上报路径 —— 仍未脱敏、策略仍不一致、仍没有跟踪 issue。
修完下限后再跑一次 @qwen-code /verify 就能解决最后一个缺口 —— 具体是确认那 9 个被重新打开的形态关闭、且 --max-tokens 8192 仍原样通过。作者持 admin，/verify 和 /tmux 都可用；与这个声明匹配的是 /verify。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at 41714d9fd0f7eaadd89dddb2646e79802315cb2d · re-run with @qwen-code /triage
|
Confidence: 3/5 — the design is right and the five round-2 Criticals are genuinely closed, but this head under-redacts a vector the PR claims for itself ( Stepping back. My independent proposal and this PR converge on the same design, and I looked for a materially simpler path and did not find one — the obvious alternative (fingerprint the error text instead of redacting shapes) is not a simplification, it is a different policy that trades away all diagnostic value, and #11198 names exactly that as the open decision. The code is 110 production lines: one function, four patterns, one mutation helper at a choke point I re-verified in the base tree this pass ( What changed since my last pass, and why the verdict reasoning is different even though the verdict is not. The round-2 Two things keep this from being a clean approve, and one thing keeps it from being a hard block:
Am I approving because I ran out of reasons to say no? No. Am I blocking because I found a reason to? Also no — and that distinction is the verdict. Defer: not approving, and not submitting a second One consequence worth flagging to whoever picks this up, because it cuts the other way: both standing Mechanics, recorded so the numbers aren't taken on faith: On volume, since the gate asks: the author has 48 open PRs, including a second telemetry-privacy PR the same day (#11670, ⏸️ Deferring to @zjunothing — already assigned, and the sole owner of
Also for the record: a @yiliang114 — the architecture is right, and the last two commits closed real defects rather than papering over them. One line before merge, in this order:
Items 5–7 in Stage 2 (quote consumption, the escaped-quote fail-open, the unpinned 中文说明信心度：3/5 —— 设计是对的，第二轮那 5 条 Critical 确实关闭了，但这个 head 对 PR 自己认领的向量（ 退一步看整体。我自己的独立方案和这个 PR 收敛到同一个设计，我也认真找过是否有明显更简的路径，没有找到 —— 那个显而易见的替代方案（对错误文本做指纹化而不是按形状脱敏）不是简化，而是另一种策略，它会把全部诊断价值换掉，而 #11198 正是把这个决定列为待决问题。生产代码 110 行：一个函数、四个正则、一个放在收口点上的修改辅助函数，而这一轮我在 base 代码里重新核实了收口点（ 自我上一轮以来变了什么，以及为什么结论没变但结论的理由变了。 第二轮 两件事让它无法干净批准，一件事让它不至于硬阻断：
我是不是因为说不出反对理由才批准？不是。我是不是因为找到了理由就阻断？也不是 —— 而这个区别就是结论。defer：不批准，也不再提交第二次 有一个后果值得提醒接手的人，因为它指向另一个方向：两个生效的 机制部分，记录在此以免数字被当作信条： 关于数量，既然 gate 要问：作者有 48 个 open PR，包括同一天另一个遥测隐私 PR（#11670），以及本 PR 的 ⏸️ 转交给 @zjunothing —— 已被指派，且是
另外记录在案：23:02 那条 @yiliang114 —— 架构是对的，最后两个 commit 修的是真实缺陷而不是掩盖。合并前按此顺序，一行代码起步：
Stage 2 的第 5–7 条（引号被吞、转义引号的 fail-open、没有测试钉住的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Close the highest-severity review findings on the redaction table: - Add `error` to ERROR_TEXT_PROPERTY_KEYS so hook failure text written to properties['error'] is redacted at the enqueue choke point (R1-1). - Skip any auth scheme (Basic/token/Digest/…) before the value, not only Bearer, so `Authorization: Basic <creds>` no longer leaks (R1-2). - Add `key` to the env key alternation so *_API_KEY / *_ACCESS_KEY* are redacted (R1-6). - Run redaction on the raw text before stripAnsiAndControl so the whitespace-delimited patterns still see their newline/tab delimiters (R1-7). - Share one secret-value pattern that accepts a quote-opened value (R1-3) and let the flag name contain the secret keyword anywhere (R1-4). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtwznj86xz
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtx1sp0ry2
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtx4nkoby7
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 92 passed · 10 failed · 102 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：92 通过 · 10 失败 · 102 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11649 Deep Verification —
|
| id | shape | base (d229c1bb) |
head (566d6a74) |
flip |
|---|---|---|---|---|
| A1-url-creds | https://x-access-token:ghs_…@github.com |
LEAK | redacted | closed |
| A2-auth-bearer | -H "Authorization: Bearer ghs_…" |
LEAK | redacted | closed |
| A3-auth-basic | Authorization: Basic dXNlcjpwYXNz… |
LEAK | redacted | closed |
| A4-secret-flag | --_authToken npm_… |
LEAK | redacted | closed |
| A5-env-secret | GITHUB_TOKEN=ghs_… |
LEAK | redacted | closed |
| A6-dsn | postgres://user:supersecret123@db |
LEAK | redacted | closed |
| A7-api-key-env | OPENAI_API_KEY=sk-proj-… |
LEAK | redacted | closed |
| A8-aws-secret-key | AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/… |
LEAK | redacted | closed |
| B9-npmrc-authtoken | //registry.npmjs.org/:_authToken=npm_… |
LEAK | redacted | closed |
| B11-git-extraheader | -c http.extraheader="AUTHORIZATION: bearer …" |
LEAK | redacted | closed |
| B1-json-auth-header | {"authorization":"Bearer ghs_…"} |
LEAK | LEAK | unchanged |
| B2-json-env-dump | {"GITHUB_TOKEN":"ghs_…"} |
LEAK | LEAK | unchanged |
| B3-prose-api-key | invalid api key sk-proj-… provided |
LEAK | LEAK | unchanged |
| B4-short-db-password | DB_PASSWORD=hunter2x (8 chars) |
LEAK | LEAK | unchanged |
| B5-curl-u | curl -u alice:supersecret123 |
LEAK | LEAK | unchanged |
| B6-curl-user-long | curl --user alice:supersecret123 |
LEAK | LEAK | unchanged |
| B7-header-colon | X-Api-Key: ak_… |
LEAK | LEAK | unchanged |
| B8-password-colon | password: supersecret123 |
LEAK | LEAK | unchanged |
| B10-mysql-short-p | mysql -pSuperSecret123 -h db |
LEAK | LEAK | unchanged |
| B12-bare-jwt | token rejected: eyJhbGci…SECRET.payload.sig |
LEAK | LEAK | unchanged |
Secret vectors: base leaks 20/20, head leaks 10/20 → 10 closed by this PR, 10 still open.
Choke-point carrier coverage (secondary claim (a) — holds). All four real carriers, same secret, driven end to end:
| carrier | base | head |
|---|---|---|
properties.error_message via logToolCallEvent |
LEAK | clean |
top-level message via logInvalidChunkEvent |
LEAK | clean |
top-level message via logApiErrorEvent (a resource event) |
LEAK | clean |
properties.error via logHookCallEvent |
LEAK | clean |
The enqueueLogEvent sink is structurally single: this.events is written at exactly one site (line 271, inside enqueueLogEvent) plus the retry path at line 1255, which unshifts events that were already redacted on their first pass. All 42 internal call sites route through it. Note the message redaction is typed as a cast to RumExceptionEvent but at runtime reads/writes event.message, so it correctly also covers RumResourceEvent.message — the logApiErrorEvent row above is that case, and it is clean.
Corrections
These are corrections to the PR description, not requests to change code.
-
"…so any error-text key — including ones added later — is scrubbed before it leaves the process" is not what the implementation does.
ERROR_TEXT_PROPERTY_KEYSis a hardcoded three-element allowlist (error_message,error_excerpt,error). A key added later is not scrubbed until someone remembers to add it to this list. The description's framing — "closes the hole once instead of once per error-text key … the shape of the earlier regressions, where a newly added error field re-opened the leak" — describes a property this change does not have. What the PR genuinely achieves is centralising the list at the sink (one place to update instead of N call sites), which is a real improvement but a weaker contract than stated. A reviewer accepting this PR should know the maintenance obligation survives. -
"
snapshotsandstackfields are intentionally not redacted" — thestackhalf of that sentence is moot. No code anywhere inpackages/core/src/telemetry/assignsstackto a Rum event (grep -rn "stack:" packages/core/src/telemetry/ --include=*.tsreturns nothing outside tests). The declared exclusion excludes nothing. This matters because a Node stack's first line is the message, so hadstackbeen populated it would have been a live bypass of themessageredaction — I checked specifically for that and it does not exist. -
The Risk section's tradeoff list names only over-matching. It does not name under-matching. Measured under-matching is 10 of 20 shapes (table above), including shapes inside the coverage the code's own doc comment claims:
ERROR_TEXT_PROPERTY_KEYSis documented as covering "command lines, HTTP headers, provider error bodies, hook failures", yet only theauthorizationheader name is matched (B7X-Api-Key:leaks), and JSON-shaped bodies leak outright (B1, B2).
Findings
F1 — 10 of 20 realistic credential shapes still reach the third-party backend (highest)
The mechanism works; its coverage is narrower than the surrounding prose implies. Grouped by cause:
-
JSON-shaped text (B1, B2). All three patterns require a bare
namefollowed by[:=]. A provider error body or an env dump is JSON, where a quote sits between the name and the separator:{"authorization":"Bearer ghs_…"},{"GITHUB_TOKEN":"ghs_…"}. Neither matches. This is the sharpest case because the doc comment onERROR_TEXT_PROPERTY_KEYSexplicitly names "provider error bodies", andlogApiErrorEventforwardsevent.error_message— a provider's JSON error body — into both the top-levelmessageandproperties.error_message. -
Colon-separated header/config forms (B7, B8).
X-Api-Key: ak_…andpassword: supersecret123. Onlyauthorizationgets the[:=]treatment; the env pattern requires a literal=. -
The
\S{10,}length floor (B4). Bisected exactly: value length ≤ 9 leaks, ≥ 10 redacted.valueLen= 7 LEAKS -> DB_PASSWORD=xxxxxxx psql failed valueLen= 9 LEAKS -> DB_PASSWORD=xxxxxxxxx psql failed valueLen=10 redacted -> DB_PASSWORD=***REDACTED*** psql failedThe comment justifies the floor as sparing
tokens_used=8192(4 chars). A floor of 5 would spare that counter too while closing 5–9-character secrets. Reproduce:node tmp/pr11649-verify-20260911-185251/probe.mjs /__w/qwen-code/qwen-code /tmp/x.json B4-short-db-password. -
Non-secret-named credential flags (B5, B6, B10).
curl -u user:pass,curl --user user:pass,mysql -pPass. The flag pattern keys on the flag name containing a secret keyword;--userand-pdo not. -
Prose markers and bare tokens (B3, B12).
invalid api key sk-proj-…, a bare JWT. No=and no--, so nothing matches.
This is incompleteness, not a regression — base leaks all 20, so the PR strictly improves the situation, and a regex allowlist can never be exhaustive. It is reported as the top finding because the residual set intersects the PR's own stated coverage, and because the description's "closes the hole once" framing invites a reviewer to believe the class is closed.
Measured candidate fix for the JSON shape (B1) — apply with its own fixture
Tolerating an optional quote on either side of the separator closes B1 with zero measured collateral:
const AUTHORIZATION_PATTERN = new RegExp(
- String.raw`\b(authorization\s*[:=]\s*)(?:[A-Za-z0-9._~+/-]+\s+)?` +
+ String.raw`\b(authorization["']?\s*[:=]\s*["']?)(?:[A-Za-z0-9._~+/-]+\s+)?` +
SECRET_VALUE,
'gi',
);Measured through the same real-dist harness (not eyeballed), all three required results:
| check | result |
|---|---|
| hostile fixture goes clean | B1-json-auth-header flips LEAK → redacted; leaks 10 → 9; all 8 claimed vectors still redacted |
| benign fixtures byte-identical | 12/12 C* cases still identical — zero new collateral; over-match set unchanged at the same 6 D* cases |
| affected suite counts unchanged | 61 passed (61) with and without the patch |
FIX-json-auth-shape SURVIVED (61/61 green). So this fix must ship with its own fixture, e.g.:
it('redacts a JSON-shaped authorization header', () => {
expect(
TEST_ONLY.redactTelemetryError(
'{"error":{"headers":{"authorization":"Bearer ghs_testsecret123"}}}',
),
).not.toContain('ghs_testsecret123');
});The same is true for B2/B7/B8: closing colon-separated and JSON-shaped KEY: value forms means deciding how much ordinary key: value prose one is willing to over-redact, which is a design call, not a one-line patch. I did not measure a fix for those.
F2 — error_excerpt is a dead key (Suggestion)
Full-repo census (grep -rn "error_excerpt", excluding node_modules/.git/tmp) returns 5 hits, all of them the same declaration: the source line, packages/core/dist, two copies inside packages/vscode-ide-companion/dist/extension.cjs, and dist/chunks/. There is no producer — nothing anywhere writes properties.error_excerpt.
The mutation matrix agrees: M8-error_excerpt-key (delete it from the list) SURVIVED at 61/61 green, while the other 11 of 12 production guards were all killed. Classification per the matrix rules: dead code — the clause cannot decide any outcome. Not a coverage gap (there is no behaviour to assert) and not redundant defence (no sibling hunk covers it, because nothing produces the key).
Matrix tally, from 02-mutation-matrix-11-of-12-guards-killed-error_excerpt-survives.png: 12 of 13 mutants killed — 11 of 12 production guards plus the positive control M1; the unmutated control M0 was GREEN (61/61), so the kills are meaningful; the sole survivor is M8. Both controls landing in the same file as the mutants is what makes M8's survival interpretable rather than a harness fault: M1 (drop error_message from the same list) turned exactly one test red, proving the runner does collect tests that exercise this file.
AGENTS.md Simplicity First is explicit about this shape: "No features beyond what was asked. No 'flexibility' or 'configurability' that wasn't requested." Dropping the entry, or landing it together with the key it anticipates, would match. Harmless as-is.
F3 — Redaction consumes the closing quote, producing unbalanced output (Nit)
SECRET_VALUE accepts an optional trailing quote, so when the opening quote sits before the matched region the closing quote is eaten:
in : curl -H "Authorization: Bearer ghs_testsecret123" https://api.github.com/repos
out: curl -H "Authorization: ***REDACTED*** https://api.github.com/repos
quote count in=2 out=1 -> UNBALANCED
The PR's own test asserts this unbalanced string as the expected value, so it is deliberate rather than an oversight. Diagnostic impact only — no security consequence, and the secret is genuinely gone. Worth a note because a reader diffing redacted error text against a real command line will see a quote vanish.
F4 — Acknowledged over-match, sized (informational)
The Risk section accepts over-matching. Measured: 6/6 benign inputs containing a secret keyword in a harmless position are mutated, and 12/12 benign inputs without one are preserved byte-identical (secondary claim (b) — holds). Witness: 04-benign-preserved-vs-acknowledged-overmatch.png.
in : vitest run --keyword-search integration --reporter=verbose failed with exit code 1
out: vitest run --keyword-search ***REDACTED*** --reporter=verbose failed with exit code 1
in : python train.py --tokenizer gpt2 --epochs 3
out: python train.py --tokenizer ***REDACTED*** --epochs 3
in : env KEYWORD_FILTER=integration,unit npm test
out: env KEYWORD_FILTER=***REDACTED*** npm test
The KEYWORD_FILTER row is the one that exceeds the description's framing: the Risk section describes the false-positive surface as following "an authorization, secret-flag, or secret-key marker", which reads as flag-shaped, but the env pattern fires on any variable whose name merely contains key/token/secret/credential and whose value is ≥10 chars. Ordinary CI config (KEYWORD_FILTER, MONKEYPATCH_MODE, TOKENIZER_PATH) is scrubbed. Still within the spirit of the accepted tradeoff, so informational — but the description's characterisation of the surface is narrower than the code's behaviour.
Scarier consequences I checked that do not hold
Bounding F1 matters more than maximising it. Each of these was a plausible escalation and each was disproved:
stackbypass — disproved. Nostack:assignment exists anywhere inpackages/core/src/telemetry/. Since a Node stack's first line is the message, a populatedstackwould have bypassed themessageredaction entirely. It is never populated.- Retry-path bypass — disproved.
this.eventshas exactly two writers: line 271 insideenqueueLogEvent(post-redaction) and line 1255unshiftinrequeueFailedEvents, which re-queues events already redacted on first enqueue. No unredacted path to the wire. - ReDoS hang — disproved as a hang. 32-rung ladder over 8 hostile shapes at 2 k / 3 k / 5 k / 20 k characters, 0 rungs over a 15 s cap. Worst:
H2-flag-many-keywords-no-terminatorat 20 000 chars = 121.6 ms. The curve is superlinear (1.2 → 2.9 → 7.6 → 121.6 ms, ≈ quadratic), and the input is genuinely outsider-authored — for shell exit/signal failuresshell.ts:3099setserror.messagetollmContent, i.e. the command's own output, so a malicious build script controls the text. But it is bounded:DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000chars, which extrapolates to ≈190 ms worst case. A one-off cost on an already-failing command, not a hang. - In-place mutation corrupting a caller's object — disproved.
redactEventErrorTextmutatesevent.properties[key]in place, but everylog*Eventbuilds that object fresh, andenqueueLogEventhas no production external callers (grepfinds onlyintegration.test.circular.tsand the.d.ts). Nothing outside the sink can observe the mutation. snapshotsas an alternate credential carrier — not observed. The foursnapshotspayloads carryexecution_summary, output-length counters, token counts, andtruncated_sequence(a truncated kitty escape sequence). None carries shell error text.truncated_sequenceis raw terminal bytes and is the only one that is even nominally free-form; it is outside this PR's scope and I did not attempt to construct a leak through it.
Not covered
- Per-commit attribution. The checkout is depth 2 (
git rev-parse --is-shallow-repository→true).git rev-list HEAD^1..HEAD^2returns 1 commit while$QWEN_VERIFY_CONTEXTlists 4 — exactly the shallow-boundary undercount the method warns about, so the per-commit claims (R1-1 … R1-7 in868e8bd7) were verified only in aggregate againstHEAD^1..HEAD. Several do map onto measurements above (R1-2 ↔M12, R1-7 ↔M13, R1-1 ↔M7), but I did not exercise the commits individually. - Base OID discrepancy. The metadata's
baseRefOidis20ecdaf6b2fbbfbd276bf05294e7b672632087e4, which is not reachable locally (git cat-file -t→ fatal). I used the merge-ref base tipHEAD^1=d229c1bb, which is the correct control for arefs/pull/11649/mergecheckout. The A/B is unaffected, but the PR's declared branch point could not be inspected. - Trial merge into current
main. Not possible at depth 2 —mainis unreachable locally, so I could not confirm the merge is conflict-free or re-measure on a merged tree.HEAD^1(d229c1bb) is by construction the base tip at the moment GitHub built the merge ref, so the control is the right one for this checkout; but how far that tip now sits behindmain, and whethermainhas since touchedqwen-logger.ts, are not measured here. A maintainer should re-check before landing. - Gates. Ran only the focused suite
packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts(61 passed / 61, re-confirmed on the restored tree) andtsc --buildforpackages/core(EXIT=0) as the typecheck. Did not run the fullpackages/coresuite,npm run lint,npm run typecheckat repo level, or any integration test. - No live network flush. Nothing was POSTed to the RUM endpoint — no credentials in this container, and sending test fixtures to a real third-party backend would be inappropriate. The oracle is
createRumPayload(), whichflushToRum()serializes verbatim; the wire body was reconstructed, not captured off a socket. - Other exfiltration paths.
loggers.tssetsattributes['error.message'] = event.error_messagefor the OTLP exporters, andsession-tracing.ts/daemon-tracing.tscarry error text into spans. Those are separate, opt-in sinks this PR does not touch and does not claim to touch; I did not verify whether they redact. - Base-build environment caveat. Building the control tree reported ~40
TS2307 Cannot find module '@opentelemetry/*'errors. Cause identified, not hand-waved: those packages live inpackages/core/node_modules(nested, unhoisted) in the main tree, and a freshgit worktreedoes not get one. They are confined to filesqwen-logger.tsdoes not import;qwen-logger.jsemitted complete (verified by tail and by the control arm exercising the real code path). The control arm's validity rests on that emit, andprobe.mjsasserts the module realpath is inside the base tree before running.
Methodology
CI merge-ref checkout at d07fcb5e; npm ci and npm run build pre-completed at head. Control built from git worktree add tmp/base-tree HEAD^1, wired to the root node_modules by symlink, packages/core rebuilt there with scripts/build_package.js; the worktree was removed after the cells were captured and git status --porcelain is clean.
Because node_modules/@qwen-code/qwen-code-core realpaths to /__w/qwen-code/qwen-code/packages/core (the head tree), every harness imports the module by absolute dist/ path and probe.mjs hard-fails if realpathSync(module) escapes the declared tree — that assertion is recorded in the ledger as control-arm-is-unfixed-build. qwen-logger.ts imports only relative paths and third-party packages, so no internal workspace link is crossed. The control was additionally confirmed to be the un-fixed build by symbol census: redactTelemetryError|redactEventErrorText appears 0 times in base dist and 6 times in head dist, and base TEST_ONLY lacks the export.
probe.mjs drives the compiled QwenLogger through real log*Event() calls with a fake Config (the only stub), clears the singleton deque between cases so each serialized payload carries exactly one event, and searches that payload for the literal secret. corpus.mjs holds 38 cases in four groups: 8 claimed vectors, 12 siblings, 12 benign, 6 acknowledged over-match. compare.mjs builds the cell table and the ledger; assertion encoding puts the security property on the head arm (a leaking shape is a real fail, never a documented pass) and the expected red on the base arm (base leaking is a passing control cell).
mutate.mjs applies 13 single-point source mutants plus an unmutated control, runs the focused vitest suite per mutant, and restores from the pristine copy each time — final byte-identity asserted (source restored byte-identical: true) and re-verified with git status. vacuity-detail.mjs re-runs selected mutants to print the actual AssertionError text, confirming each kill fails the intended assertion with an expected-vs-actual behavioural mismatch rather than a compile or import break; it also carries the FIX-json-auth-shape candidate. redos-ladder.mjs runs 8 hostile shapes × 4 size rungs under a 15 s per-rung cap.
Raw logs: base-probe.log, head-probe.log, head-plus-fix-probe.log, head-restored-probe.log, redos-head.log, ab-table.log, mutate.json, assertions-detail.json (per-assertion ledger). Harnesses are .mjs and rerunnable as-is.
Integrity check: after restoring the source and rebuilding, the head probe was re-run and reproduces the original results byte-for-byte (restored dist reproduces original head results: true, leaks 10 = 10, fidelity identical) — so the mutation and candidate-fix work left no residue in the reported numbers.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/qwen-logger/qwen-logger.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- unbounded-surface / fail-closed approach (qwen-logger.ts:178) — already reported (comment 5633025309, triage stage 2 finding 6); consolidation direction declined (comment 3989187979, R1-10)
- dead 'error_excerpt' allowlist entry (qwen-logger.ts:115) — already reported (comment 5633025309, triage stage 2 finding 4; comment 5633024923, stage 1)
- newline/tab flattening of multi-line error text (qwen-logger.ts:177) — already reported (comment 5633025309, triage stage 2 finding 3; comment 3989187992, R1-7)
- fourth credential-redaction table / OTel span drift (qwen-logger.ts:177) — already reported (comment 3989187979, R1-10, author declined; comment 5633025309, triage stage 2 finding 5)
- non-authorization secret headers and JSON-quoted forms (qwen-logger.ts:135) — already reported (comment 5633025309, triage stage 2 finding 6; comment 3989187968, R1-3 disclosed residual)
Not explored to full depth (tool budget reached): "agent 6b": confirming whether any upstream producer caps the length of error.message / error_excerpt before it reaches enqueueLogEvent (bears on the severity of the …; "agent 6b": running npx vitest run src/telemetry/qwen-logger/qwen-logger.test.ts in packages/core to confirm the added tests pass as written — I replicated the exact patt….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6b"：confirming whether any upstream producer caps the length of error.message / error_excerpt before it reaches enqueueLogEvent (bears on the severity of the …；"agent 6b"：running npx vitest run src/telemetry/qwen-logger/qwen-logger.test.ts in packages/core to confirm the added tests pass as written — I replicated the exact patt…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Harden the usage-statistics error-text redaction pass against the review findings on this branch: - Normalise shell line continuations (`\` + newline + leading whitespace) ahead of every pass, so a credential split across a continuation (including a URL credential) can't survive as a glued cleartext run. - Make SECRET_VALUE quote-aware (a quoted run matches to its closing quote), so interior quotes and quoted values containing spaces redact whole instead of leaking a suffix after the marker. - Give the env pattern the same quoted alternative behind its floor. - Skip an optional leading backslash in each separator so a shell-escaped nested quote (`Authorization: Bearer \"secret\"`) still redacts. - Strip ANSI/C0 per line (after widening tabs to spaces) before the patterns run, so an escape or control character can't hide or reassemble a credential the pattern pass never saw. Fix the line-continuation test to assert the secret is absent rather than merely that no marker is emitted, and add regression cases for interior quotes, quoted values with spaces, ANSI/C0, and continuations. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtxennouyn
- Bound the non-secret name chars around the secret keyword in
SECRET_FLAG_PATTERN and ENV_SECRET_PATTERN (previously unbounded `*`,
now `{0,64}`) to prevent quadratic backtracking on adversarial error
text that runs through the synchronous telemetry redaction pass.
- Give SECRET_FLAG_PATTERN the same 10-char value floor as
ENV_SECRET_PATTERN so `--max-tokens 8192` and `max_tokens=8192` are
handled consistently (both pass through untouched).
- Align the ENV_SECRET_PATTERN docblock examples with the actual 10-char
floor (the old `ghs_xxx` / `sk_xxx` examples were shorter than the floor
and were never redacted).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtxgstgfyr
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 171 passed · 23 failed · 194 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：171 通过 · 23 失败 · 194 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11649 Deep Verification (round 2) —
|
| # | Round-1 finding | Sev | Status at 41714d9f |
Re-measured evidence |
|---|---|---|---|---|
| F1 | 10 of 20 realistic credential shapes still reach the backend | highest | stands — identical set | Same 10 shapes leak (B1,B2,B3,B4,B5,B6,B7,B8,B10,B12); B9/B11 still redacted. ab-table.log |
| F1-fix | Measured candidate fix for the JSON shape (B1) | — | not adopted; superseded in priority | B1 still leaks at head; the new F5 regression is the higher-value fix |
| F2 | error_excerpt is a dead key |
Suggestion | stands | Full-repo census now 1 hit (the declaration, qwen-logger.ts:115); mutant M8 SURVIVED GREEN 69/69 |
| F3 | Redaction consumes the closing quote → unbalanced output | Nit | stands, scope wider than round 1 reported | Round 1 measured one shape (2→1). Now 4/4 unbalanced; flag shapes lose both quotes (--_authToken "npm_…" 2→0, --password="…" 2→0). Identical on prev and head, so it predates the delta. fixture-and-f3.log |
| F4 | Acknowledged over-match 6/6 | info | improved | Measured prev → head on this round's corpus: over-match 6/6 → 4/6 (round 1 independently reported 6/6 on its own corpus, so the starting point agrees). --tokenizer gpt2 and --max-tokens 8192 now pass through; benign byte-identity on this round's 12-case corpus 11/12 → 12/12 (C4 = run --max-tokens 8192 max_tokens=8192 flipped MUTATED → identical) |
| C1 | "any error-text key — including ones added later" ≠ the hardcoded 3-key allowlist | — | stands | ERROR_TEXT_PROPERTY_KEYS is still ['error_message','error_excerpt','error'] |
| C2 | Declared stack exclusion excludes nothing |
— | stands | grep -rn "stack:" packages/core/src/telemetry/ --include=*.ts (excl. tests) → 0 hits |
| C3 | Risk section names only over-matching, not under-matching | — | worsened | Under-matching grew from 10/20 to 23/41 real-credential shapes |
| — | ReDoS "disproved as a hang" (worst 121.6 ms) | — | superseded — the delta fixed the curve | At the 25 k production truncation cap: prev 121.9 ms → head 0.7 ms (~170×). Ladder below |
| — | In-place mutation cannot corrupt a caller | — | stands (re-confirmed) | 42 internal this.enqueueLogEvent( sites; external callers only in integration.test.circular.ts and the unit test |
| — | Per-commit attribution not possible at depth 2 | — | now covered | git fetch --depth=12 origin refs/pull/11649/head succeeded; all 6 PR commits in the metadata are locally reachable and match git log |
| — | Trial merge into current main |
— | still not covered | main unreachable locally; see Not covered |
Central claim and A/B (four build arms)
Central claim. Error text enqueued into the usage-statistics (RUM) sink is scrubbed of credentials at a single choke point.
Secondary claims. (a) The choke point is genuinely single. (b) Non-sensitive text survives verbatim. (c) New this round: the {0,64} bounds prevent quadratic backtracking. (d) New this round: flag and env value floors are unified so --max-tokens 8192 and max_tokens=8192 behave the same.
Oracle. flushToRum() does body = safeJsonStringify(rumPayload) and POSTs to gb4w8c3ygj-default-sea.rum.aliyuncs.com, so createRumPayload() is the wire body. The harness drives the real log*Event() methods on the compiled dist/ of each arm and searches the serialized payload for the literal secret. Nothing about the unit under test is stubbed except Config; flushIfNeeded/flushToRum are hard-blocked and their invocations counted (63 blocked attempts per arm, identical across arms — nothing left the container).
Witness: 01-three-arm-ab-base-vs-prev-vs-head.png (the per-case cell table, base/prev/head). The round1 and fix5 arms are tabulated below and in round1-probe.json / fixcheck.log respectively.
| arm | commit | real-credential leaks (of 41) | R group: short flag secrets redacted (of 12) | benign byte-identical (of 12) | over-matched (of 6) |
|---|---|---|---|---|---|
| base (control) | d0e39357 |
41 / 41 | 0 | 12/12 | 0 (no redaction at all) |
| round1 (head round 1 verified) | 566d6a74 |
10 / 41 | 12 | 8/12 | 6 |
| prev (delta's parent) | 31827e9d |
10 / 41 | 12 | 11/12 | 6 |
| head (under test) | 41714d9f |
23 / 41 | 3 | 12/12 | 4 |
| fix5 (measured candidate, §F5) | head + floor 5 | 14 / 41 | 12 | 12/12 | 4 |
The round1 arm exists so the regression claim is a measurement, not an inference from reading a pattern literal: both earlier revisions of this branch redacted all 12 short-flag shapes and sat at 10/41 real-credential leaks; head redacts 3 and sits at 23/41. Its 22 control assertions (12 R + 8 A + arm-identity + realpath) are all passing cells in the ledger — an earlier revision failing to redact would have falsified the claim. round1's lower benign score (8/12) is the over-match the delta then fixed, and it is the one thing the delta genuinely improved.
Per-group leak counts (ab-table.log; for group D the value is benign, so "LEAK" = preserved = good):
grp n base prev head
A 8 8 0 0 <- the 8 vectors the description names: all closed, all stay closed
B 12 12 10 10 <- round-1 residual F1: unchanged
R 12 12 0 9 <- NEW: 9 short flag secrets reopened by the delta
D 6 6 0 2 <- benign over-match: 2 now correctly preserved
N 9 9 0 4 <- NEW: {0,64} boundary, 65-char name segments
Choke-point carrier coverage (secondary claim (a) — holds). Same secret driven end to end through all four real carriers:
| carrier | base | prev | head |
|---|---|---|---|
properties.error_message via logToolCallEvent |
LEAK | clean | clean |
top-level message via logInvalidChunkEvent (exception) |
LEAK | clean | clean |
top-level message via logApiErrorEvent (resource) |
LEAK | clean | clean |
properties.error via logHookCallEvent |
LEAK | clean | clean |
Structurally single: this.events.push appears at exactly one site (line 283, inside enqueueLogEvent, after redactEventErrorText); the only other writer is unshift at line 1267 in requeueFailedEvents, which re-queues events already redacted on first pass. All 42 internal call sites route through it.
Backtracking ladder (secondary claim (c) — holds, with a caveat). 8 hostile shapes × 4 rungs (2 k / 3 k / 5 k / 20 k), one rung per child process under timeout 30, plus a 25 k rung at the real production ceiling (DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD = 25_000, config.ts:793). Zero rungs hit the 30 s cap on either arm. Raw: redos-ladder.jsonl.
| shape | prev 2 k → 20 k → 25 k | head 2 k → 20 k → 25 k |
|---|---|---|
| H1-env-many-keywords-no-eq | 1.1 → 48.7 → 75.8 ms | 0.5 → 0.6 → 0.6 ms |
| H2-flag-many-keywords-no-sep | 1.2 → 68.3 → 121.9 ms | 0.5 → 0.6 → 0.7 ms |
| H3–H8 (6 further shapes) | flat, 0.5 – 0.9 ms | flat, 0.5 – 1.0 ms |
The bound is genuinely load-bearing on the curve — H2's per-kchar cost rises 0.62 → 3.42 ms on prev (superlinear) and falls 0.26 → 0.03 ms on head (flat). Caveat: the thing it fixes was never a hang. The unbounded worst case at the largest input production can produce is 121.9 ms, on an already-failing command. The input is outsider-authored — shell.ts:3102 sets error.message = llmContent for exit/signal failures, i.e. the failing command's own output — so the hardening is worthwhile, but it is defence-in-depth, not a fix for an exploitable stall.
Corrections
Corrections to the PR description / commit message, not requests to change code.
- Round-1 correction C1 still stands. "any error-text key — including ones added later — is scrubbed" is not what the code does:
ERROR_TEXT_PROPERTY_KEYSis a hardcoded three-element allowlist. What the PR achieves is centralising that list at the sink, which is a real but weaker contract; the maintenance obligation survives. - Round-1 correction C2 still stands, and is now load-bearing for F2. The Risk section says
stackis "intentionally not redacted"; no code inpackages/core/src/telemetry/assignsstackto a Rum event. The declared exclusion excludes nothing. - The delta commit's "unify flag/env value floors" is literally accurate but leaves a three-way asymmetry.
AUTHORIZATION_PATTERNstill uses the floorlessSECRET_VALUE, so at head:Authorization: Bearer xyz(3 chars) is redacted (R11),Authorization: Basic YWJj(4 chars) is redacted (R12), while--password hunter2(7 chars) is not (R1). There are now three different value floors in one function — 1 char for authorization, 10 for flags, 10 for env. A reader taking "unified" at face value would expect one. - The commit's stated cost/benefit is inverted in effect. The message frames the floor as consistency hygiene ("
--max-tokens 8192andmax_tokens=8192… both pass through untouched"). That goal is achieved — but the same edit is what reopens 9 credential shapes (F5). The consistency was obtained by moving the flag pattern down to the env pattern's floor rather than by choosing a floor that satisfies both goals; §F5 measures one that does.
Findings
F5 — The delta commit reopens 9 short-secret flag leaks that both earlier revisions closed (highest)
41714d9f replaced SECRET_VALUE with a new SECRET_FLAG_VALUE carrying a ≥10-character floor:
prev (31827e9d): SECRET_VALUE = (?: "[^"]+" | '[^']+' | [^\s"'\`\\][^\s]* ) <- no floor
head (41714d9f): SECRET_FLAG_VALUE = (?: "[^"]{10,}" | '[^']{10,}' | [^\s"'\`\\][^\s]{9,} ) <- >=10 chars
Any secret of ≤9 characters behind a --*token|password|secret|credential|key* flag now reaches the RUM backend verbatim. Measured through the real dist, boundary bisected — and measured on both earlier revisions of this branch, not just the delta's parent (round1-probe.json): at 566d6a74 all 12 R shapes were redacted, at 31827e9d all 12 were redacted, at head 3 are.
| id | error text | prev | head |
|---|---|---|---|
| R1 | mysql --password hunter2 -h db.internal failed |
redacted | LEAK |
| R2 | deploy.sh --password=Passw0rd exited 1 |
redacted | LEAK |
| R3 | npm publish --token abc123 failed |
redacted | LEAK |
| R4 | client --api-key sk-12345 refused |
redacted | LEAK |
| R5 | vault --secret s3cr3tval denied |
redacted | LEAK |
| R6 | svc --credential cr3dX rejected |
redacted | LEAK |
| R7 | mysql --password="pw123" -h db failed |
redacted | LEAK |
| R8 | npm publish --_authToken npm_abc12 failed |
redacted | LEAK |
| R9 | run --password xxxxxxxxx end (9 chars) |
redacted | LEAK |
| R10 | run --password yyyyyyyyyy end (10 chars) |
redacted | redacted |
Reproduce: node tmp/pr11649-verify-20260911-224720/probe.mjs /__w/qwen-code/qwen-code head R1-flag-password-7
Why this is the top finding, bounded honestly:
- It is not a regression against
main— base leaks all 41 shapes, so the PR still strictly improves the situation. It is a regression against this branch's own previous revision, which is what a follow-up round exists to catch. --passwordis explicitly named in the PR description as a covered vector ("secret-bearing flags (--token,--_authToken,--password)").- Short values are the common case for user-chosen passwords (
hunter2,Passw0rd,pw123) even though machine tokens are long. The leak surface is real, not theoretical. - The env pattern already had this floor, so
DB_PASSWORD=hunter2xleaked before too (round-1 B4). The delta's contribution is extending the same blind spot to the flag spelling.
Measured candidate fix — lower the flag floor to 5, keep the {0,64} bounds
A floor of 5 satisfies both goals: it spares every counter the commit was protecting (8192, gpt2 — both 4 chars) while re-closing all nine shapes above.
-const SECRET_FLAG_VALUE = String.raw`(?:"[^"]{10,}"|'[^']{10,}'|[^\s"'\`\\][^\s]{9,})`;
+const SECRET_FLAG_VALUE = String.raw`(?:"[^"]{5,}"|'[^']{5,}'|[^\s"'\`\\][^\s]{4,})`;Built as a fourth arm (arms/fix5.js) and driven through the identical real-dist harness — 52/52 scripted assertions pass. Witness: 03-candidate-fix-floor5-recloses-9-of-9.png, raw fixcheck.log.
| required check | result |
|---|---|
| hostile fixtures go clean | R group 3/12 → 12/12 redacted; all 9 regressions re-closed |
| headline metric | real-credential leaks 23/41 → 14/41 (the residual 14 = 10 round-1 B shapes + 4 {0,64} boundary shapes the fix deliberately keeps) |
| benign fixtures byte-identical (zero collateral) | 12/12 identical at both head and fix5 — no new collateral |
| the delta's own intent survives | --max-tokens 8192 and --tokenizer gpt2 still preserved; over-match stays 4/6, does not revert to 6/6 |
| claimed vectors unaffected | A group 8/8 still redacted |
| round-1 residual unaffected | B group identical (2/12 redacted at both) — no shape moved either way |
| bounds retained | N group identical to head on all 9 shapes, so the ReDoS hardening is preserved |
M2 is this exact one-line edit and it scored GREEN 69/69, identical to head (mutate.json). The fixture that would pin it is in F7.
F6 — The {0,64} bound starts missing at 65-character name segments (low)
Bisected exactly, in both patterns and on both sides of the keyword:
| id | shape | prev | head |
|---|---|---|---|
| N1 | -- + 64×a + token <secret> |
redacted | redacted |
| N2 | -- + 65×a + token <secret> |
redacted | LEAK |
| N3 | --token + 64×b + <secret> |
redacted | redacted |
| N4 | --token + 65×b + <secret> |
redacted | LEAK |
| N5/N7 | env name, 64-char prefix / suffix | redacted | redacted |
| N6/N8 | env name, 65-char prefix / suffix | redacted | LEAK |
| N9 | --amazon-bedrock-agent-runtime-session-secret-access-key (37-char prefix, realistic) |
redacted | redacted |
Severity is low and I want to bound it rather than inflate it: the bound only bites above 64 characters in a single name segment, and N9 shows a genuinely long real-world flag stays covered. Total matched flag name at the bound is 64 + keyword + 64 ≈ 133 characters. This is reported because it is a new under-match the delta introduced (4 shapes moved redacted → leak), not because 65-character flag names are common. fix5 keeps the bounds, so it does not change this row; widening the bound would trade against the ladder in §(c).
F7 — The suite cannot distinguish a floor of 10 from a floor of 5, so F5 is invisible to the PR's own tests
The mutation matrix — 10 rows: an unmutated control, 8 single-point mutants, and one combination row; each edit applied in the scratch worktree and restored from a pristine copy afterwards (source restored byte-identical: true). Witness: 02-mutation-matrix-floor-and-bounds-unpinned.png, raw mutate.json.
| mutant | edit | suite | classification |
|---|---|---|---|
| M0 | none (control) | GREEN 69/69 | control green ⇒ the kills below are meaningful |
| M1 | drop error_message from ERROR_TEXT_PROPERTY_KEYS |
RED 1 failed / 68 passed | positive control, same file ⇒ the runner does collect tests exercising this module |
| M2 | flag floor 10 → 5 (the candidate fix) | GREEN 69/69 — SURVIVED | coverage gap — nothing pins the floor from above |
| M3 | flag floor removed entirely | RED 1 failed / 68 passed | killed by the delta's own new test |
| M4 | flag {0,64} → * |
GREEN — SURVIVED | coverage gap (see F8) |
| M5 | env {0,64} → * |
GREEN — SURVIVED | coverage gap (see F8) |
| M6/M7 | {0,64} → {0,63} |
GREEN — SURVIVED | boundary not pinned (expected; no test at 64) |
| M8 | delete error_excerpt from the allowlist |
GREEN — SURVIVED | dead code (round-1 F2, re-confirmed by census) |
| M9 | both bounds reverted together (combination row) | GREEN — SURVIVED | same as M4+M5; no layered-guard interaction hidden here |
M1 is the control that makes M2's survival interpretable, and it lands in the same file as the mutants, failing the intended assertion with a real expected-vs-actual mismatch:
FAIL > QwenLogger > error text redaction > redacts error_message on the enqueue boundary
AssertionError: expected 'Command: git clone https://token:ghs_…' not to contain 'ghs_testsecret123'
Expected: "ghs_testsecret123"
Received: "Command: git clone https://token:ghs_testsecret123@github.com/org/repo.git"
M3 also fails the intended assertion — the delta commit's own new test — which is what proves the floor is pinned only from below:
FAIL > QwenLogger > error text redaction > treats short counters the same in flag and env spellings
AssertionError: expected 'run --max-tokens ***REDACTED*** max_t…' to be 'run --max-tokens 8192 max_tokens=8192'
Expected: "run --max-tokens 8192 max_tokens=8192"
Received: "run --max-tokens ***REDACTED*** max_tokens=8192"
So the suite asserts "a 4-character value must survive" and never asserts "a short secret must be scrubbed". Any floor ≥5 satisfies it. That is precisely the gap F5 fell through.
The fixture that would pin it — measured, not proposed in the abstract. Adding one case and running the suite on both arms (fixture-and-f3.log):
it('redacts a short secret behind a secret flag', () => {
expect(
TEST_ONLY.redactTelemetryError('mysql --password hunter2 -h db.internal failed'),
).not.toContain('hunter2');
});| arm + fixture | suite |
|---|---|
| head (floor 10) | RED — 1 failed / 69 passed (70), AssertionError: expected 'mysql --password hunter2 -h db.intern…' not to contain 'hunter2' |
| floor 5 | GREEN — 70 passed (70) |
The fixture discriminates exactly along the axis the regression lives on. F5's fix should ship with it; without it, a future floor change is equally invisible.
F8 — The {0,64} bounds are behaviourally load-bearing but unpinned by any test
M4, M5 and the M9 combination row all survive at 69/69 green, yet the ladder in §(c) proves the bounds change real behaviour by ~170× at the production input ceiling. Classification: coverage gap, not dead code and not redundant defence — the guard decides an outcome (backtracking cost), nothing asserts it. A perf guard is awkward to pin in a unit test, so this is reported as completeness, not as a merge condition; the cheapest useful pin is a wall-clock ceiling on one hostile shape (e.g. H2 at 20 k must complete under ~20 ms), which M4/M5 would fail.
F9 — error_excerpt remains a dead key (Suggestion, carried forward)
Full-repo census at the new head: exactly 1 occurrence of error_excerpt outside node_modules/.git/dist/tmp — the declaration itself at qwen-logger.ts:115. No producer anywhere. M8 (delete it) survives green. Classification unchanged: dead code. AGENTS.md Simplicity First is explicit about this shape ("No 'flexibility' or 'configurability' that wasn't requested"). Harmless as-is.
F3 (carried) — Redaction still consumes quotes; scope is wider than round 1 reported
Identical on prev and head, so it predates the delta (it comes from 31827e9d, which round 1 never verified):
in (2q): curl -H "Authorization: Bearer ghs_testsecret123" https://api.github.com/repos
out (1q): curl -H "Authorization: ***REDACTED*** https://api.github.com/repos UNBALANCED 2->1
in (2q): npm publish --_authToken "npm_testsecret123" --registry x
out (0q): npm publish --_authToken ***REDACTED*** --registry x UNBALANCED 2->0
in (2q): run --password="hunter2secret" --host db
out (0q): run --password=***REDACTED*** --host db UNBALANCED 2->0
4/4 shapes unbalanced. Round 1 measured only the first. The PR's own test asserts the unbalanced string as the expected value, so it is deliberate. Diagnostic impact only — the secret is genuinely gone — but a reader diffing redacted text against a real command line will see quotes vanish, and the flag shapes lose both.
F1 (carried) — 10 residual shapes, unchanged
Re-measured, same set: JSON-shaped text (B1 {"authorization":"Bearer …"}, B2 {"GITHUB_TOKEN":"…"}), colon-separated header/config forms (B7 X-Api-Key: …, B8 password: …), the env \S{10,} floor (B4), non-secret-named credential flags (B5 curl -u, B6 curl --user, B10 mysql -pPass), and prose/bare tokens (B3, B12). Round 1's measured candidate fix for the JSON shape (optional quote around the separator) was not adopted; B1 still leaks. This remains incompleteness, not a regression — base leaks all of them.
Scarier consequences I checked that still do not hold
stackbypass — still disproved. Nostack:assignment inpackages/core/src/telemetry/(non-test). A populatedstackwould have bypassed themessageredaction, since a Node stack's first line is the message.- Retry-path bypass — still disproved.
this.eventswriters:pushat line 283 (post-redaction) andunshiftat line 1267 (re-queues already-redacted events). No unredacted path to the wire. - In-place mutation corrupting a caller — still disproved. 42 internal call sites; the only external callers are
integration.test.circular.tsand the unit test. No production observer. - ReDoS hang — disproved, and now also hardened. Zero of 32 ladder rungs hit the 30 s cap on either arm; unbounded worst case at the 25 k production ceiling was 121.9 ms.
- Backtracking blow-up introduced by the new bounds — checked, does not hold. Head is flat (≤1.0 ms) on all 8 shapes at all rungs; bounding cannot make it worse than the unbounded arm, and the ladder confirms it is ~170× better at the top rung.
Not covered
- Trial merge into current
main.mainis not reachable locally even after deepening (git fetch --depth=12 origin refs/pull/11649/headfetched the PR branch, notmain). I could not confirm the merge is conflict-free or re-measure on a merged tree.HEAD^1(d0e39357) is by construction the base tip when GitHub built the merge ref, so the control is the right one for this checkout; how far that tip now sits behindmain, and whethermainhas since touchedqwen-logger.ts, are not measured. A maintainer should re-check before landing. - Base OID discrepancy (carried). The metadata's
baseRefOidis20ecdaf6b2fbbfbd276bf05294e7b672632087e4, unreachable locally. I used the merge-ref base tipHEAD^1=d0e39357, which is the correct control for arefs/pull/11649/mergecheckout. The PR's declared branch point could not be inspected. - Per-commit attribution is now possible but was only partly exercised. All 6 PR commits are locally reachable and match the metadata. I verified the aggregate
HEAD^1..HEADdiff and isolated the delta (31827e9d→41714d9f) as its own arm. I did not exercise commits7df466e0,868e8bd7,b5d35fd1,566d6a74individually, so the per-claim mapping in868e8bd7's message (R1-1 … R1-7) is not independently attributed here — round 1 covered several in aggregate and those rows carry forward above. - Gates. Ran only the focused suite
packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts(69 passed / 69 at head) andtsc --noEmitforpackages/core(EXIT=0), the latter proven live by planting a type error and confirmingerror TS2322: Type 'string' is not assignable to type 'number'at the planted symbol, then restoring to a clean run. Did not run the fullpackages/coresuite,npm run lint, repo-levelnpm run typecheck, or any integration test. - No live network flush. Nothing was POSTed to the RUM endpoint — no credentials in this container, and sending fixtures to a real third-party backend would be inappropriate.
flushIfNeeded/flushToRumwere hard-blocked and counted (63 blocked attempts per arm). The oracle iscreateRumPayload(), whichflushToRum()serializes verbatim; the wire body was reconstructed, not captured off a socket. - Other exfiltration paths.
loggers.tssetsattributes['error.message']for the OTLP exporters, andsession-tracing.ts/daemon-tracing.tscarry error text into spans. Separate, opt-in sinks this PR neither touches nor claims; not verified. snapshotsas an alternate carrier. Round 1 found none of the foursnapshotspayloads carries shell error text. Not re-enumerated this round — the delta does not touch that path, andsnapshotsis outsideERROR_TEXT_PROPERTY_KEYSby design.- Windows / CRLF. The description marks Windows
⚠️ . The continuation joiner handles\\\r?\n, but I did not drive a full CRLF corpus; all fixtures were LF.
Methodology
CI merge-ref checkout at 164206dd; npm ci and npm run build pre-completed at head. Because this is a follow-up round, the control is four build arms plus a candidate-fix arm, not a single base, and every carried-forward measurement was rebuilt and re-run rather than diffed against previous-report.md.
git fetch --depth=12 origin refs/pull/11649/head made the branch's own history reachable, so the arms are real commits, not reconstructions from commit messages: base = git show d0e39357:…qwen-logger.ts, round1 = git show 566d6a74:… (the head round 1 verified), prev = git show 31827e9d:… (the delta's parent), head = the checked-out 41714d9f, and fix5 = head with one line edited by build-variant.sh. Building round1 was deliberate: without it, "both earlier revisions closed these shapes" would rest on reading a pattern literal (SECRET_VALUE used +, so ≥1 char) rather than on running the code. git diff --stat HEAD^1..HEAD confirms the PR touches only qwen-logger.ts and its test, so a single-file swap inside one worktree is an exact control. All arms were built in tmp/base-tree (a git worktree at HEAD, wired to the root node_modules and to packages/core/node_modules by symlink — the latter is what avoids the ~40 TS2307 @opentelemetry/* errors round 1 hit); each build was tsc --build, ~33 s, exit 0. The worktree was removed after the cells were captured and git status --porcelain is empty.
Workspace-link trap handled explicitly. readlink -f node_modules/@qwen-code/qwen-code-core resolves to /__w/qwen-code/qwen-code/packages/core — the head tree — so a naive control would silently load head code. Every harness therefore imports by absolute dist/ path, and probe.mjs hard-fails unless realpathSync of the loaded module stays inside tmp/base-tree; that check is recorded per arm as realpathInsideTree: true. Arm identity is pinned two ways. First, each saved arm module has a distinct sha256 prefix — base 906b79d4, round1 8e9b80b0, prev 737981e0, head 3fa8341c, fix5 438e2e79. Second, a symbol census recorded in each probe's census block:
| arm | redactTelemetryError |
SECRET_FLAG_VALUE |
{0,64} |
unbounded [A-Za-z0-9_-]* |
|---|---|---|---|---|
| base | absent | absent | absent | absent |
round1 (566d6a74) |
present | absent | absent | present |
prev (31827e9d) |
present | absent | absent | present |
head (41714d9f) |
present | present | present (×2) | absent |
Integrity: the worktree's head build is byte-identical (sha256 3fa8341c…) to the pre-built main-tree dist, and after every mutation the source was restored and re-verified byte-identical, with the main-tree dist still hashing to the same value at the end.
probe.mjs drives the compiled QwenLogger through real log*Event() calls with a fake Config (the only stub), clears the singleton deque between cases so each serialized payload carries exactly one event, and searches that payload for the literal secret. corpus.mjs holds 47 secret-bearing cases in five groups (8 claimed vectors, 12 round-1 siblings, 12 short-flag regression probes, 6 acknowledged over-match, 9 quantifier-boundary bisectors) plus 12 benign byte-identity cases. compare.mjs builds the cell table and ledger; encoding puts the security property on the head arm (a leaking credential is a real fail) and the expected red on the base arm (base leaking is a passing control cell), so fail=23 counts only unexpected outcomes. Group D is excluded from the fail ledger because its secret field is a benign value — counting it would have inflated the headline; the real-credential subtotal (41) is reported separately for that reason.
mutate.mjs applies 8 single-point mutants plus a combination row and an unmutated control (10 rows), locates each edit by a unique line anchor (a [^`]* regex silently no-ops on these literals because they contain an escaped backtick — two mutants reported NO-OP EDIT before I switched), runs the focused suite per mutant, and restores from a pristine copy. vacuity-detail.mjs re-runs selected mutants to capture the raw AssertionError text quoted in F7. fixture.mjs splices the proposed discriminating fixture into the worktree test file, runs the suite on both arms, and restores both files byte-identically. redos-rung.mjs + redos-driver.sh run one ladder rung per child process under timeout, so a cap is a recorded result rather than a hung harness. fixcheck.mjs adjudicates the fix5 arm against all five corpus groups. finalize.mjs merges every ledger and re-runs the typecheck gate.
Raw logs: base-probe.json, round1-probe.json, prev-probe.json, head-probe.json, fix5-probe.json (plus the matching *-probe.err), ab-table.log, ab-ledger.json, fixcheck.log, fixcheck-ledger.json, mutate.json, vacuity-detail.log, fixture-and-f3.log, redos-ladder.jsonl, final-ledger.log, assertions-detail.json (per-assertion ledger). Compiled arm modules are kept under arms/{base,round1,prev,head,fix5}.js so any cell can be re-run without rebuilding. Harnesses are .mjs (plus build-variant.sh and redos-driver.sh) and rerunnable as-is.
How to re-run. The harnesses splice a saved arm module into a build worktree, which was removed at the end of this round, so re-running needs it recreated first (the compiled arms themselves are preserved, so no per-arm rebuild is required):
cd /__w/qwen-code/qwen-code
git fetch --depth=12 origin refs/pull/11649/head
git worktree add tmp/base-tree HEAD
ln -s "$PWD/node_modules" tmp/base-tree/node_modules
ln -s "$PWD/packages/core/node_modules" tmp/base-tree/packages/core/node_modules
(cd tmp/base-tree/packages/core && npx tsc --build) # ~33 s; creates the dist tree spliced into
ART=tmp/pr11649-verify-20260911-224720
for a in base round1 prev head fix5; do node "$ART/probe.mjs" "$PWD" $a > "$ART/$a-probe.json"; done
node "$ART/compare.mjs" && node "$ART/fixcheck.mjs" && node "$ART/finalize.mjs"
git worktree remove --force tmp/base-tree # leave the tree cleanNote on PR text. The description and commit messages were treated as untrusted hypotheses, not evidence. No instruction in them attempted to steer this verification.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/qwen-logger/qwen-logger.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 323 passed · 35 failed · 358 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：323 通过 · 35 失败 · 358 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification report (report.md, truncated)Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
⏸️ Deferring — not approving, not requesting changes. Recorded against 41714d9f so the deferral is pinned to the commit I actually reviewed.
All five round-2 Criticals are closed at this head; I re-derived each from the diff and the base tree. The code is sound and the leak surface is strictly smaller than main. What stops this being an approval is ownership, not quality: telemetry redaction policy is a maintainer decision, and this gate escalates telemetry rather than deciding it. #11198 carries status/ready-for-human and names the policy as the actual work.
Full reasoning, the two corrections to my previous pass, and the three questions for the maintainer are in the Stage 3 comment above. Escalated to @zjunothing (already assigned, sole core-telemetry owner).
One mechanical note: the standing CHANGES_REQUESTED on this PR is pinned to 566d6a74, two commits behind head, and every Critical it named is now closed. It will hold mergeStateStatus at BLOCKED until dismissed or /review re-runs at 41714d9f.
— Qwen Code · qwen3.8-max-2026-09-02
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 192 passed · 22 failed · 214 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：192 通过 · 22 失败 · 214 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11649 Deep Verification (round 4) —
|
| # | Round-3 finding | Sev | Status at 41714d9f |
Re-measured evidence |
|---|---|---|---|---|
| F10 | Unterminated/backtick value unredactable; "25/25 chars reach the wire" | highest | stands as a mechanism; reachability narrower than round 3 stated | Corpus X1,X2,X5,X6,X7 leak at head (5/5), redacted at round1 (5/5), closed by fixq (5/5). Real truncator at the 30 k shell threshold: 1/60 offsets leak the full 18/18 chars; but realistic multi-line output S1/S2 → 0/40 leaks at head |
| F5 | Delta reopens 9 short-secret flag leaks | highest | stands — exactly 9 | R1…R9 leak at head, R10 (10 chars) redacted; round1/prev redact 12/12; fix5f/fixall reclose 9/9 (ab-table.log, R row) |
| F14 | (not probed in round 3) | high — NEW | new this round | Quoted value spans \n: Q4 910/932 chars (97.64%), 21→1 line; Q5 915/957 (95.61%); Q2 24938/24972 (99.86%). round1 on the same fixtures: 20 and 19 chars |
| F15 | (round 3 recommended fixall as "benign byte-identical 12/12") |
high — NEW | new this round | Q3 fixq/fixall destroy 24937 chars (99.88%); Q8 28/57 (49.12%); real truncator offset 752 → 2749 of 3933 chars (69.9%) destroyed, vs round1 52 |
| F6 | {0,64} bound misses at 65-char name segments |
low | stands — exactly 4 | N2/N4/N6/N8 leak at head; N1/N3/N5/N7 (64 chars) and N9 (realistic 57-char flag) redacted; round1/prev 0/9 |
| F1 | 10 residual credential shapes | highest | stands — same 10 | B group: head 10/12 leak, base 12/12; B9/B11 redacted. Incompleteness, not a regression |
| F12 | Benign CR-containing text not verbatim | low | stands | M group head 0/6 byte-identical; M2 line1\r\nline2\r\n 14→12 chars, M4 line1\rline2 11→10 |
| F3 | Redaction consumes quotes → unbalanced output | Nit | stands | Assertion redaction-can-emit-unbalanced-quotes passes on A2/X9/W4 |
| F4 | Acknowledged over-match | info | consistent with round 3 | D group preserved 2/6 (prev) → 4/6 (head); C group byte-identity 12/12 at head (best arm); D3/D4 still over-matched |
| C1 | "any error-text key … added later" ≠ hardcoded 3-key allowlist | — | stands | Per-arm symbol census on all ten compiled arms: ERROR_TEXT_PROPERTY_KEYS = ['error_message','error_excerpt','error'] |
| C2 | Declared stack exclusion excludes nothing |
— | stands (carried on identical closure) | Re-grepped this round: 0 non-test stack: assignments in packages/core/src/telemetry/ |
| Corr. 3 | Three different value floors in one function | — | stands | R11 Bearer xyz (3 chars) redacted, R12 Basic YWJj (4 chars) redacted, R1 --password hunter2 (7 chars) leaks |
| F7/F8/F9/F11 | Mutation matrix: floor, bounds, dead key, joiner all unpinned | — | not re-measured this round | The nojoin mutant arm was rebuilt to round 3's exact hash 5ab51541; the vitest matrix itself was not re-run (see Not covered) |
| — | Trial merge into current main, full packages/core suite |
— | not re-run this round | Round 3 covered both at this same head/base; see Not covered |
Central claim and A/B (ten build arms)
Central claim. Error text enqueued into the usage-statistics (RUM) sink is scrubbed of credentials at a single choke point.
Secondary claims. (a) the choke point is genuinely single; (b) non-sensitive text survives verbatim; (c) the {0,64} bounds prevent quadratic backtracking; (d) flag and env value floors are unified.
Oracle. flushToRum() serializes createRumPayload() with safeJsonStringify and POSTs it, so that payload is the wire body. The corpus job drives the real logToolCallEvent() → enqueueLogEvent() → redactEventErrorText() path on each arm's compiled dist/, serializes with the real safeJsonStringify, and searches the payload for the literal secret. The only stub is Config. Every case is also run through TEST_ONLY.redactTelemetryError directly and the two verdicts are asserted to agree — allAgree=true on all ten arms, so the pattern-level and wire-level oracles never diverge.
Egress. Blocked at net.Socket.prototype.connect (ESM namespaces are frozen, so tls.connect/https.request cannot be reassigned; both funnel through the socket prototype) with a positive control that runs before any measurement: a probe connect to the real RUM host must be intercepted, else the harness exits 4. egressControlFired=1 and blocked.socket === egressControlFired on all ten arms — no fixture left the container. flushToRum/flushIfNeeded are also replaced and counted.
Witness: 01-ten-arm-ab-base-68-of-68-leak-head-32.png.
| arm | commit / edit | real-credential leaks (of 68) | R: short flag secrets (of 12) | X: delimiter siblings (of 12) | benign byte-identical (C, of 12) |
|---|---|---|---|---|---|
| base (control) | d0e39357 |
68 / 68 | 12 | 12 | 12/12 (no redaction at all) |
| round1 | 566d6a74 |
14 / 68 | 0 | 1 | 10/12 |
| prev (delta's parent) | 31827e9d |
17 / 68 | 0 | 6 | 11/12 |
| head (under test) | 41714d9f |
32 / 68 | 9 | 7 | 12/12 |
| nojoin (F11 mutant) | head − joiner line | 33 / 68 | 9 | 7 | 12/12 |
| fixq (round 3) | head + unterminated quote/backtick | 27 / 68 | 9 | 2 | 12/12 |
fix5f (round 3's fix5) |
head + flag floor 5 | 22 / 68 | 0 | 7 | 12/12 |
fixallf (round 3's fixall) |
fixq + flag floor 5 | 17 / 68 | 0 | 2 | 12/12 |
| fix5 (new) | head + flag and env floor 5 | 20 / 68 | 0 | 7 | 12/12 |
| fixall (new) | fixq + flag and env floor 5 | 15 / 68 | 0 | 2 | 12/12 |
Per-group leak counts (ab-table.log; D/C/M are benign groups where "preserved" is the good outcome, so they are listed separately and excluded from the 68):
grp n base round1 prev head nojoin fixq fix5f fixallf fix5 fixall
A 8 8 0 0 0 0 0 0 0 0 0 <- the 8 vectors the description names: closed on every arm
B 12 12 10 10 10 10 10 10 10 9 9 <- F1 residual: incompleteness, not regression
R 12 12 0 0 9 9 9 0 0 0 0 <- F5: 9 short flag secrets reopened by the delta
N 9 9 0 0 4 4 4 4 4 4 4 <- F6: {0,64} boundary
X 12 12 1 6 7 7 2 7 2 7 2 <- F10: delimiter siblings, regressed by 31827e9d
W 8 8 2 0 0 1 0 0 0 0 0 <- CRLF/tab/ANSI security holds at head
T 3 3 1 1 2 2 2 1 1 0 0 <- truncation residues below the value floor
V 4 4 0 0 0 0 0 0 0 0 0 <- NEW: astral/CJK/NBSP all redacted at head
D 6 - 2 red 2 red 4 red 4 red 4 red 3 red 3 red 3 red 3 red <- benign over-match (head over-matches 2)
C 12 0 3 1 0 0 0 0 0 0 0 <- benign byte-identity: head is the best arm
M 6 0 6 6 6 5 6 6 6 6 6 <- benign text the normalisation pass mutates (F12)
Choke-point carrier coverage (secondary claim (a) — holds). One secret driven end to end through all four real carriers, on the real compiled modules:
| carrier | base | head |
|---|---|---|
properties.error_message via logToolCallEvent |
LEAK (marker absent) | clean, marker present |
top-level message via logInvalidChunkEvent (error_message → exception) |
LEAK | clean, marker present |
properties.error via logHookCallEvent (logPrompts on) |
LEAK | clean, marker present |
top-level message + properties.error_message via logApiErrorEvent |
LEAK | clean, marker present |
8/8 assertions. Structurally single: this.events.push appears at exactly one site (inside enqueueLogEvent, after redactEventErrorText); the only other writer is unshift in requeueFailedEvents, which re-queues already-redacted events. Note the exception carrier needs the event field error_message, not error (logInvalidChunkEvent maps event.error_message → message); my first harness passed the wrong field and reported a false clean with no redaction marker — caught by asserting markerPresent alongside wireLeaked, then fixed and both arms re-run. A clean result with no marker means the text never reached the payload, not that it was redacted.
Corrections
Corrections to the PR description / commit messages and to round 3's report, not requests to change code.
- C1 stands. "any error-text key — including ones added later — is scrubbed" is not what the code does:
ERROR_TEXT_PROPERTY_KEYSis a hardcoded three-element allowlist (verified on all ten compiled arms). Centralising the list at the sink is real but a weaker contract. - C2 stands. The Risk section says
stackis "intentionally not redacted"; 0 non-teststack:assignments exist inpackages/core/src/telemetry/. The declared exclusion excludes nothing. - Correction 3 stands. Three value floors in one function, not one: 1 char for
authorization, 10 for flags, 10 for env.R11/R12(3- and 4-char values) redact whileR1(7 chars) leaks. - NEW — the Risk section's stated bound on over-matching is wrong by ~4 orders of magnitude. It says "the patterns run to the next whitespace or quote, so the false-positive surface is a non-empty run of characters following an authorization, secret-flag, or secret-key marker." Measured, a quoted value runs to its matching closing quote, which
[^"]allows to be any number of newlines away:Q4lost 910 characters across 21 lines,Q2lost 24,938 characters. The real bound is the whole 25 k payload, not one token. See F14. - NEW — round 3's "floor 5" was flag-only, and that is now pinned by hash, not by reading. My flag-only rebuilds reproduce round 3's published
fix5 438e2e79andfixall afad45b8exactly; my flag+env variants do not (a0d31c3a,adc08e7b). This matters because it explains round 3's residualT3(GITHUB_TOKEN=ghs_tr, 6 chars) leaking underfixall: the env floor was never lowered. Lowering both reaches T 0/3 and 15/68 overall — the best arm measured this round. - NEW — round 3's "benign byte-identical 12/12" for
fixq/fixallis true but not a fidelity bound. That corpus is short and single-line. On multi-line and 25 k fixtures the same arms destroy up to 99.88% of the text (F15). The claim was accurate about what it measured and silent about what it did not.
Findings
F14 — A quoted value spans newlines, so one stray quote can destroy nearly all of a benign multi-line error text (high, new)
31827e9d replaced the optional-delimiter value pattern with balanced-quote alternatives:
round1 (566d6a74): SECRET_VALUE = ["'\`]?[^\s"'\`\\]+["'\`]? <- [^\s...] cannot cross a newline
prev/head : SECRET_VALUE = (?:"[^"]+"|'[^']+'|[^\s"'\`\\][^\s]*)
^^^^^^ [^"] MATCHES \n
redactTelemetryError splits on \n only to strip ANSI/C0 per line, then rejoins and runs the three .replace() calls over the whole string. So "[^"]{10,}", '[^']{10,}' and "[^"]+" all match across newlines. Every fixture below is entirely benign — no credential anywhere; the number is diagnostic text destroyed.
Witness: 02-crossline-quote-swallow-benign-chars-destroyed.png.
| id | fixture (all benign) | base | round1 | prev | head | fixq | fixall |
|---|---|---|---|---|---|---|---|
| Q4 | API_KEY="rotated + 19 diagnostic lines + end of key" |
0 | 20 | 910 | 910 / 932 (97.64%), 21→1 line | 910 | 910 |
| Q5 | authorization: Bearer "stale + 19 lines + token here" |
0 | 19 | 915 | 915 / 957 (95.61%), 21→1 line | 915 | 915 |
| Q2 | --password " + 24,950 chars + " tail |
0 | 24938 | 24938 | 24938 / 24972 (99.86%) | 24938 | 24938 |
| Q6 | control: benign quotes, no secret-named marker | 0 | 2 | 0 | 0 | 0 | 0 |
| Q7 | control: single-line quoted value (intended) | 0 | −6 | −2 | −2 | −2 | −2 |
Head's Q4 output is exactly API_KEY=***REDACTED*** — 22 characters from 932.
Why this is a finding and not just the accepted tradeoff. The Risk section accepts over-matching but bounds it (correction 4). The mechanism also has a direction: round1 destroyed 20 chars where head destroys 910 on the identical fixture, so this is a regression introduced by 31827e9d — the same commit, and the same balanced-quote requirement, that produced F10. One change, two opposite failure modes: an unterminated quote becomes unredactable (security), a stray quote becomes a text shredder (fidelity). What it destroys is precisely the diagnostic value the Risk section cites for rejecting fingerprinting.
Bounded honestly — what does not hold:
- It is not a security regression: nothing leaks that base did not leak, and
Cgroup byte-identity is 12/12 at head (its best result on any axis). - The trigger needs a secret-named marker immediately followed by a quote that does not close on the same line, plus a second quote later in the same error text. I did not find a common real command that does this, so the practical hit rate is unknown — the finding is about the unbounded magnitude when it fires and the incorrect bound stated in the description, not about frequency.
- Both controls hold (
Q60 on every arm,Q7the intended small redaction), so this is not "the redactor eats everything".
Reproduce:
cd /__w/qwen-code/qwen-code
ART=tmp/pr11649-verify-20260912-010107
for a in round1 prev head; do node "$ART/arm-runner.mjs" $a swallow; done # rows Q1..Q8F15 — Round 3's recommended fix closes the leak but destroys up to 99.88% of the error text, a cost it never measured (high, new)
fixq/fixall add "[^"]{10,} and '[^']{10,} — an unterminated alternative with no upper bound, so it matches greedily to end of string. Round 3 reported these arms as "benign byte-identical 12/12" and recommended fixall to ship. That corpus is short and single-line, so it could not see this.
| fixture | head | fixq / fixall / fixallf | round1 |
|---|---|---|---|
Q3 --password " + 24,950 chars, no closing quote |
0 destroyed (but leaks) | 24937 destroyed (99.88%) | 24937 |
Q8 --password 'abc + 2 short diagnostic lines |
0 (but leaks) | 28 / 57 (49.12%), 3→1 line | −8 |
And through the real truncateAndSaveToFile with production limits (threshold=30000 = DEFAULT_SHELL_OUTPUT_THRESHOLD, previewChars=min(4000,threshold), keep='both', lines=Infinity), at the one offset where head leaks:
| arm | leaked | longest surviving fragment | chars of the 3,933-char preview destroyed by redaction |
|---|---|---|---|
| base | yes (19/60 offsets) | 18/18 | 0 |
| round1 | no (0/60) | 3/18 | 52 |
| head | yes (1/60, offset 752) | 18/18 — the full credential | 0 |
| fixq | no (0/60) | 3/18 | 2749 (69.9%) |
| fixall | no (0/60) | 3/18 | 2749 (69.9%) |
Witness: 03-real-truncator-head-leaks-18-of-18-fixq-costs-2749-chars.png.
This does not make the fix wrong. A credential reaching a third-party metrics backend is worse than lost diagnostics, and fixall is still the best arm on the corpus (15/68). It makes round 3's recommendation incompletely priced: a maintainer reading "benign byte-identical 12/12" would not know that shipping it converts a ≤18-char leak into losing 70–99.9% of the error text on the shapes the fix exists for. The price is large enough to be worth measuring a narrower alternative first — bounding the quoted alternatives to a single line ("[^"\n]{10,}" / '[^'\n]{10,}') would close F14 and F10's backtick half while refusing to span the payload. I did not build or measure that variant, so it is a direction, not a recommendation.
Reproduce:
ART=tmp/pr11649-verify-20260912-010107
for a in head fixq fixall round1; do
POSITIONS=60 OFF_BASE=600 OFF_SPAN=500 node "$ART/arm-runner.mjs" $a truncate S3 30000
doneF10 (carried, re-measured) — Reachability is narrower than round 3 presented
The mechanism stands exactly as round 3 described: X1,X2,X5,X6,X7 leak at head, all five are redacted at round1, and fixq closes all five. Head's X1 output still puts the marker immediately before the surviving credential — curl -H "Authorization: ***REDACTED*** \ghs_backticksec1`" …` — which reads as redacted to a human while the secret is in the payload.
What changed is the production-reachability half. Round 3 drove the truncator at threshold=25000 (the generic DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD) and reported 25/25 credential chars reaching the wire. The shell tool — the PR's own stated dominant leak vector — uses DEFAULT_SHELL_OUTPUT_THRESHOLD = 30_000 (shell.ts:105) unless explicitly configured. At that threshold, with the real truncator:
- Realistic multi-line shell output (S1 URL credential, S2 quoted flag; 40 insertion positions each): head leaks at 0/40 on both. The credential line either survives whole and is redacted, or is dropped entirely.
maxSurvivingFragment5 (S1) and 3 (S2) — fragments of the surrounding literal, not the secret. - A single line larger than the preview budget, credential at a swept character offset (S3, 60 offsets): the secret reaches the truncated preview at 19/60 offsets, and head leaks the full 18/18 chars at 1/60 (offset 752).
round1,fixq,fixall: 0/60.base: 19/60.
So F10 is real and load-bearing, but it needs keep:'both' to slice an over-budget single line mid-value — minified JSON, a very long echoed command — not ordinary multi-line shell output. A maintainer weighing F15 against F10 should know the leak is 1 window in 60 on the shape I could construct, while the fidelity cost is 70–99.9% on the same shape.
F5 (carried) — 9 short-secret flag leaks, unchanged
R1…R9 leak at head (hunter2, Passw0rd, abc123, sk-12345, s3cr3tval, cr3dX, "pw123", npm_abc12, a 9-char value); R10 (10 chars) is redacted, bisecting the floor exactly. round1 and prev redact 12/12; fix5f, fixallf, fix5, fixall all reclose 9/9. --password is named in the description as a covered vector. Not a regression against main (base leaks all 12); a regression against this branch's own earlier revisions.
F6 (carried) — The {0,64} bound starts missing at 65-character name segments (low)
Exactly 4 shapes move redacted → leak: N2/N4 (65-char prefix/suffix, flag) and N6/N8 (65-char prefix/suffix, env). N1/N3/N5/N7 at 64 chars are redacted, bisecting the bound. N9 — a realistic 57-char flag, --amazon-bedrock-agent-runtime-session-secret-access-key — stays redacted. No fix arm changes this row; widening it would trade against the (unmeasured this round) backtracking curve.
F1 (carried) — 10 residual shapes, unchanged
B group: head 10/12 leak, base 12/12. Same classes as round 3: JSON-shaped text (B1, B2), colon-separated header/config forms (B7 X-Api-Key:, B8 password:), the env floor (B4 DB_PASSWORD=hunter2), non-secret-named credential flags (B5 curl -u, B6 curl --user, B10 mysql -pPass), and prose/bare tokens (B3, B12). B9/B11 redacted. Incompleteness, not a regression.
F12 (carried) — Benign CR-containing text is not preserved verbatim (low)
M group: head preserves 0/6 byte-identically. M2 line1\r\nline2\r\n 14→12 chars; M4 line1\rline2 11→10 chars (two lines become one); M1 tabs, M3 ANSI, M5 continuations, M6 bell also mutated. CONTROL_CHARS_RE (textUtils.ts:11) includes \r. Tabs/ANSI/C0/continuations are documented in the function's own docblock; the \r removal is not, and it contradicts the Reviewer Test Plan's "non-sensitive text is preserved verbatim". Identical on every arm, so no candidate fix changes it. Note C group (no control characters) is 12/12 byte-identical at head, so this is confined to text already containing C0 controls.
F9 (carried) — error_excerpt remains a dead key (Suggestion)
Repo-wide census at this head: 1 occurrence outside node_modules/.git/dist/tmp — the declaration. No producer. Classification: dead code; AGENTS.md Simplicity First is explicit about this shape. Harmless as-is.
NEW, clean — astral-plane and multibyte credentials are handled (V group)
Round 3 listed non-UTF8/astral as not covered. All four shapes are redacted at head (V1 trailing emoji, V2 env spelling with emoji, V3 CJK-prefixed value, V4 NBSP separator), 4/4, and all four leak at base — so this is a genuine improvement, not a vacuous pass. No [^\s]-class width bug here.
Scarier consequences I checked that still do not hold
stackbypass — disproved (re-grepped). 0 non-teststack:assignments inpackages/core/src/telemetry/.- Astral/multibyte class-width bug — disproved. V group 4/4 redacted at head, 4/4 leak at base.
- Wire-level and pattern-level oracles diverging — disproved.
allAgree=trueon all ten arms across 68 cases each. - A fixture escaping the container — disproved, with a positive control. The egress block is proven to fire before any measurement;
blocked.socket === egressControlFiredon all ten arms. - The cross-line swallow eating text with no secret marker — disproved.
Q6(benign quotes, no marker) destroys 0 chars on every arm. - F10 reachable via ordinary multi-line shell output — disproved. S1/S2 leak at 0/40 at head; it needs an over-budget single line.
- A false "clean" from a harness that never delivered the text — caught and fixed. The exception carrier initially reported clean with no redaction marker because I passed
errorwherelogInvalidChunkEventreadserror_message;markerPresentis now asserted besidewireLeaked.
Not covered
- The mutation matrix was NOT re-run this round. Round 3's 15 rows (M0 control, M1 positive control, M2 floor, M4/M5/M9 bounds, M8 dead key, M10/M11 reverse, M12 joiner, M13 ANSI, M14
errorkey) are the basis for F7/F8/F9/F11, and I am carrying those four statuses on the proven-identical input closure plus one fresh measurement: I rebuilt thenojoinmutant arm and it hashes to round 3's exact5ab51541. That is evidence the mutant is the same binary; it is not evidence about what the suite does with it. Treat F7/F8/F11 as unverified this round. - The backtracking ladder was NOT re-run — including on
fixq/fixall. This was a planned new probe and it did not fit the budget, so "does round 3's recommended fix reintroduce superlinear backtracking?" is unanswered. It matters:fixq/fixalladd greedy alternatives with no upper bound ("[^"]{10,}), and the{0,64}bounds the delta commit exists to add are still present (verified ×4 on every arm by symbol census), but the value side was never laddered. Secondary claim (c) is therefore carried from round 3, not re-measured. - No live network flush. Nothing was POSTed to the RUM endpoint. The oracle is
createRumPayload()serialized by the realsafeJsonStringify— the wire body was reconstructed, not captured off a socket. - Real
ShellToolnot driven end to end. The truncation cells chain the realtruncateAndSaveToFileinto the realredactTelemetryErrorwith production limits read fromshell.ts:2976; the intermediate links (shell.ts:2998llmContent = truncatedResult.content→error.message→logToolCallEvent→properties.error_message) are cited by line, not executed. This reproduces the production wire shape; it does not run a real failing shell command. - Full
packages/coresuite, trial merge into currentmain, repo-level lint/typecheck — not run this round. Round 3 covered the first two at this same head and base (conflict-free intofdb33117,+25 passing / +0 failing, 66 FAIL lines byte-identical to plainmain).mainhas almost certainly moved since; whether the merge is still conflict-free is not established here. - Per-commit attribution only partly exercised. All 6 PR commits are locally reachable (
git fetch --depth=12 origin refs/pull/11649/head) and match the metadata'scommitsarray exactly. I built566d6a74(round1) and31827e9d(prev) as arms and isolated31827e9d → 41714d9fas the delta, but did not exercise7df466e0,868e8bd7orb5d35fd1individually, so868e8bd7's R1-1…R1-7 claim mapping is not independently attributed. snapshotsas an alternate carrier — not enumerated; outsideERROR_TEXT_PROPERTY_KEYSby design.- Other exfiltration paths —
loggers.tsattributes['error.message']for OTLP,session-tracing.ts/daemon-tracing.tsspans. Separate opt-in sinks this PR neither touches nor claims. - Base OID discrepancy (carried). The metadata's
baseRefOidis20ecdaf6…, unreachable locally. I used the merge-ref base tipHEAD^1=d0e39357, the correct control for arefs/pull/11649/mergecheckout. - Whole-CRLF-file fixtures and non-UTF8 byte sequences. The
W/M/Vgroups cover CR, CRLF, tabs, ANSI, C0, emoji, CJK and NBSP inside JS strings; I did not drive a CRLF-encoded file or invalid UTF-8 bytes.
Methodology
CI merge-ref checkout at 164206dd; npm ci and npm run build pre-completed at head — the pre-built dist already hashed to 3fa8341c…, matching round 3's published head before I built anything. Follow-up round, so the control is ten build arms rather than a single base.
git fetch --depth=12 origin refs/pull/11649/head made all 6 PR commits reachable, so round1 = 566d6a74 and prev = 31827e9d are real commits, not reconstructions; base = HEAD^1 = d0e39357; head = the checked-out 41714d9f. git diff --name-status HEAD^1..HEAD confirms the PR touches only qwen-logger.ts and its test, so swapping those two files inside one worktree is an exact control (both are swapped together because head's test references a symbol base does not export). Candidate arms come from apply-edit.mjs, which refuses an anchor that does not match exactly once — all three anchors were dry-run against the pristine source and reported unique before any build. Builds ran in tmp/build-tree (git worktree at HEAD, wired to the root node_modules and packages/core/node_modules by symlink), each npx tsc --build exit 0; build-arms.sh restores the pristine sources afterwards and asserts the restore (source restored byte-identical to pristine head: true).
Workspace-link trap handled explicitly. readlink -f tmp/build-tree/node_modules/@qwen-code/qwen-code-core resolves to /__w/qwen-code/qwen-code/packages/core — the head tree — even from inside the worktree, so a naive base harness would silently load head code. Every harness therefore imports by absolute dist/ path inside the worktree and hard-fails unless realpathSync of the loaded module stays inside it: realpathInsideWorktree=true on all ten arms. Each arm is swapped into that dist path and its sha256 re-verified after the copy before import.
Arm identity pinned by hash, cross-checked against round 3. All eight of round 3's published prefixes reproduce: base 906b79d4, round1 8e9b80b0, prev 737981e0, head 3fa8341c, fix5f 438e2e79, fixq b0b7b428, fixallf afad45b8, nojoin 5ab51541. Two arms are new this round (fix5 a0d31c3a, fixall adc08e7b) and differ from round 3's by lowering the env floor as well as the flag floor. A per-arm symbol census (redactTelemetryError presence, {0,64} count, joiner line, the literal SECRET_VALUE/SECRET_FLAG_VALUE strings, the allowlist) is printed in logs/build-arms.log, so each arm's content is verified independently of its hash.
arm-runner.mjs drives the compiled QwenLogger through real log*Event() calls with a fake Config (the only stub), clears the singleton deque per case so each payload carries exactly one event, and searches the real safeJsonStringify'd payload for the literal secret; it also asserts the pattern-level and wire-level verdicts agree per case. corpus.mjs holds 68 credential cases in 8 groups plus 24 benign cases in 3, and the 8 SWALLOW fidelity fixtures. truncate drives the real truncateAndSaveToFile across three scenarios (realistic multi-line URL credential, realistic multi-line quoted flag, over-budget single line) sweeping insertion offsets. compare.mjs builds the cell tables and the ledger; counts in this report are read from its captured logs, not restated.
Ledger encoding. The security property is asserted on the head arm, so a credential reaching the wire is a real fail. Control arms carry their expectation in the assertion (control/base-leaks/* passes when base leaks — 68/68), so an expected red is a pass. All 22 fails are unexpected outcomes: 12 cross-line fidelity (Q2/Q4/Q5 × head, fixq, fixall, fixallf), 2 truncation-preview fidelity (fixq/fixall at 2,749 chars), 6 normalisation fidelity (F12, M1…M6), and 2 acknowledged over-match (D3/D4). No harness failure is counted as a PR finding; where a harness was wrong I fixed it and re-ran — two cases: W6/W7 originally carried a secret string that was not a literal substring of their own text (a control character sat inside it), which made base read as non-leaking, and the exception carrier passed error where logInvalidChunkEvent reads error_message, which produced a false clean. Both were corrected and all arms re-run.
Raw logs: {arm}-corpus.json, {arm}-swallow.json, {arm}-trunc-{S1,S2,S3}.json (+.err) for all ten arms, ab-table.log, swallow-table.log, compare.log, ledger.json, assertions.json, build-arms.log, {arm}-build.log, gate-head-focused.log. Compiled arms are kept under arms/*.js, so any cell can be re-run without rebuilding; harnesses are .mjs/.sh and rerunnable as-is. Focused suite at head: 69 passed / 69 (gate-head-focused.log), matching round 3.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/qwen-logger/qwen-logger.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 2 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 3 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 4 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
round 5 · packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
⏸️ Still deferring — not approving, not requesting changes. Pinned to 41714d9f, the commit I reviewed. This supersedes the framing of my 23:56 deferral on this same commit, which cannot be edited.
Correction to that earlier deferral. It said the blocker was "ownership, not quality". Ownership still blocks — telemetry redaction policy is a maintainer decision, and #11198 carries status/ready-for-human. But there is now a quality finding too, and it has a measurement behind it rather than an opinion.
The round-2 /verify report landed after that deferral was written. The final commit swapped the flag path from the unbounded SECRET_VALUE to SECRET_FLAG_VALUE, which carries a 10-character value floor. I hand-derived the consequence from the diff:
SECRET_FLAG_VALUE = (?:"[^"]{10,}"|'[^']{10,}'|[^\s"'\`\\][^\s]{9,})
mysql --password hunter2 (7), --token abc123 (6), --api-key sk-12345 (8) and --password="pw123" (5 inside quotes) match no alternative — the quoted ones need ≥10 interior chars, the unquoted one needs ≥10 total, and neither ENV_SECRET_PATTERN (requires key=value) nor AUTHORIZATION_PATTERN (requires the literal authorization) can pick them up. They reach the third-party RUM backend verbatim. --password and --token are named as covered vectors in the PR description.
Two baselines, because they answer differently: against main this is not a regression — base has zero redaction (grepped: 0 hits), head closes all 8 named vectors for realistic long credentials, measured leaks 41/41 → 23/41. Against the branch's own earlier revisions it is: /verify built 566d6a74 and 31827e9d as separate arms and both redacted 12/12 short-flag shapes at 10/41; head redacts 3/12 at 23/41.
The fix is one line, and it is measured rather than conjectural. A fifth arm with the floor at 5 ({5,} / {4,}, {0,64} bounds kept) passed 52/52: 9/9 reopened shapes re-closed, 8/8 description vectors still redacted, benign text byte-identical 12/12, and --max-tokens 8192 still passing through — because 8192 is 4 characters. Floor 5 dominates floor 10 on every axis measured, so this is not a coverage-vs-over-match trade-off.
Why it survived to head: the suite cannot tell the floors apart. Every positive redaction assertion uses a value ≥10 chars (the one short-looking fixture, --password P@ss'w0rd123, is 12), and mutant M2 (floor 10 → 5) survived green 69/69. On a P1 credential fix that is the load-bearing gap — a boundary CI cannot distinguish in either direction is how a coverage regression lands in the final commit with nothing turning red.
I am not submitting a third CHANGES_REQUESTED. This account already has two standing (7df466e0, 566d6a74), reviewDecision is CHANGES_REQUESTED, and mergeStateStatus is BLOCKED, so the PR cannot merge and another review from me adds no gate. One consequence for whoever picks this up, flagged because it cuts the other way: both standing reviews are pinned to commits whose named Criticals are now closed, so dismissing them as stale is legitimate — but doing so makes this mergeable with the floor regression still in it. Settle the floor and the boundary fixture before dismissing anything.
Full reasoning, the hand-derived table, the corroborated mutation result, and the three questions for the maintainer are in the Stage 2 and Stage 3 comments above (both re-derived this pass, not carried forward). Escalated to @zjunothing — already assigned, sole core-telemetry owner.
@yiliang114 — the architecture is right and I am not asking you to change it. Two items before merge: lower the flag floor to 5, and add the fixture pair that pins both sides of the boundary (--password hunter2 must redact, --max-tokens 8192 must not) so this cannot silently regress again.
中文说明
⏸️ 仍然 defer —— 不批准，也不 request changes。 钉在我实际审查的 commit 41714d9f 上。它取代我 23:56 在同一 commit 上那次 defer 的表述 —— 那一条无法编辑。
对上一次 defer 的更正。 它说阻碍是"归属权，不是质量"。归属权依然是阻碍 —— 遥测脱敏策略是维护者的决定，而 #11198 带 status/ready-for-human。但现在也有一条质量发现，而且它背后是实测数据而不是观点。
第二轮 /verify 报告在那次 defer 写完之后才落地。最后一个 commit 把 flag 路径从无下限的 SECRET_VALUE 换成了带 10 字符取值下限的 SECRET_FLAG_VALUE。我从 diff 手工推导了后果：mysql --password hunter2（7）、--token abc123（6）、--api-key sk-12345（8）、--password="pw123"（引号内 5）任何分支都不匹配 —— 引号分支要求内部 ≥10 字符，非引号分支要求总计 ≥10，而 ENV_SECRET_PATTERN（要求 key=value）和 AUTHORIZATION_PATTERN（要求字面的 authorization）都接不住。它们会原样送达第三方 RUM 后端。而 --password 与 --token 是 PR 描述里点名认领的覆盖向量。
两个基线要分开说，因为答案不同： 相对 main 这不是回退 —— base 完全没有脱敏（grep 零命中），head 对真实长凭据关闭了全部 8 个点名向量，实测泄漏 41/41 → 23/41。相对本分支自己更早的版本是回退 —— /verify 把 566d6a74 和 31827e9d 各自构建成独立对照臂，两版都脱敏 12/12 个短取值 flag 形态、泄漏 10/41；head 只脱敏 3/12、泄漏 23/41。
修复是一行，而且是实测过的，不是猜想。 第五个臂把下限设为 5（{5,} / {4,}，保留 {0,64} 边界），52/52 断言通过：9/9 被重新打开的形态关闭、8/8 描述向量保持脱敏、无害文本逐字节一致 12/12，并且 --max-tokens 8192 仍原样通过 —— 因为 8192 只有 4 个字符。下限 5 在每一个实测维度上都优于下限 10，所以这不是"覆盖面 vs 过度匹配"的取舍。
它为什么能活到 head： 套件分辨不出两种下限。每一条正向脱敏断言用的取值都 ≥10 字符（唯一看着短的 fixture --password P@ss'w0rd123 是 12 字符），变异体 M2（下限 10 → 5）全绿存活 69/69。在一个 P1 凭据修复上这就是承重的缺口 —— 一个 CI 在任一方向都分辨不出的边界，正是一处覆盖面回退能落在最后一个 commit 上而没有任何东西变红的原因。
我不提交第三个 CHANGES_REQUESTED。 本账号已有两个生效（7df466e0、566d6a74），reviewDecision 是 CHANGES_REQUESTED、mergeStateStatus 是 BLOCKED，所以 PR 合不了，我再加一个评审不会增加门禁。有一个后果要提醒接手的人，因为它指向另一个方向：两个生效评审都钉在其点名 Critical 已全部关闭的 commit 上，所以把它们当陈旧评审 dismiss 是合理的 —— 但那样做会让这个 PR 在下限回退仍然存在的情况下变成可合并。 在 dismiss 任何东西之前，先把下限和边界 fixture 定下来。
完整推理、手工推导的表格、复核过的变异结果，以及给维护者的三个问题，都在上方 Stage 2 与 Stage 3 评论中（两条都是本轮重新推导的，不是照搬）。已转交 @zjunothing —— 已被指派，core-telemetry 唯一 owner。
@yiliang114 —— 架构是对的，我不要求你改它。合并前两条：把 flag 下限降到 5；加上钉住边界两侧的 fixture 对（--password hunter2 必须脱敏，--max-tokens 8192 必须不脱敏），让它不能再静默回退。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- issue-scope mismatch:
Fixes #11198auto-closes an issue whose stated scope is wider than this diff (qwen-logger.ts:191-194) - already reported (comment 5633025309, triage stage 2 finding 8); no author reply - fourth credential-redaction table / reuse of the existing shape tables (qwen-logger.ts:191-194) - already reported (comment 3989187979, R1-10); consolidation direction declined twice by the author, and the structural direction is carried by…
- dead 'error_excerpt' allowlist entry (qwen-logger.ts:115) - already reported (comment 5633025309, triage stage 2 finding 4; comment 5633024923, stage 1)
Not reviewed: test-efficacy probe — harnessValidated: null (its baseline run tripped the vitest globalSetup prerequisite guard), so no revert/mutant/hunk evidence was measured for the 252 added test lines.
Not explored to full depth (tool budget reached): "agent 6a": none — I did not run npx vitest run src/telemetry/qwen-logger/qwen-logger.test.ts at HEAD (I relied on the PR's green CI at the anchor plus direct tsx execu….
Not reviewed: reverse audit — stopped before round 9 by the review time budget.
Convergence: round 3 posted 14 inline comment(s), 14 of them reported for the first time; the previous round posted 9 (8 new). Findings keep coming back to the same files: packages/core/src/telemetry/qwen-logger/qwen-logger.ts (findings in rounds 1, 2; 13 more now); packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts (findings in round 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：test-efficacy probe — harnessValidated: null (its baseline run tripped the vitest globalSetup prerequisite guard), so no revert/mutant/hunk evidence was measured for the 252 added test lines.
未探索到全部深度（达到工具调用预算）："agent 6a"：none — I did not run npx vitest run src/telemetry/qwen-logger/qwen-logger.test.ts at HEAD (I relied on the PR's green CI at the anchor plus direct tsx execu…。
未审查：反向审计——评审时间预算不足，未能开始第 9 轮。
收敛情况：第 3 轮发布了 14 条行内评论，其中 14 条是首次提出；上一轮发布了 9 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/core/src/telemetry/qwen-logger/qwen-logger.ts（第 1、2 轮已出过发现，本轮又有 13 条）；packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts（第 2 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- hand-maintained three-key allowlist does not deliver class closure (qwen-logger.ts:106) - already reported (comment 3994853920, R3-8)
- top-level arm visits only
message, sosnapshotsis never visited (qwen-logger.ts:218) - already reported (comment 3994853924, R3-10)
Not reviewed: test-efficacy probe — harnessValidated: null, its baseline run tripped the vitest globalSetup prerequisite guard, so no revert/mutant/hunk evidence was measured for the added test lines (this is a harness limitation, not a silent agent).
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Convergence: round 4 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 14 (14 new). Findings keep coming back to the same files: packages/core/src/telemetry/qwen-logger/qwen-logger.ts (findings in rounds 2, 3; 3 more now); packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts (findings in round 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：test-efficacy probe — harnessValidated: null, its baseline run tripped the vitest globalSetup prerequisite guard, so no revert/mutant/hunk evidence was measured for the added test lines (this is a harness limitation, not a silent agent).
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛情况：第 4 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 14 条（其中 14 条首次提出）。发现反复回到同一批文件：packages/core/src/telemetry/qwen-logger/qwen-logger.ts（第 2、3 轮已出过发现，本轮又有 3 条）；packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const MAX_RETRY_EVENTS = 100; | ||
|
|
||
| const ERROR_TEXT_PROPERTY_KEYS = ['error_message', 'error_excerpt', 'error']; | ||
| const REDACTED_ERROR_TEXT = '***REDACTED***'; |
There was a problem hiding this comment.
[Suggestion] R4-3: This head ships the opposite redaction policy from the one recorded on the linked issue, and Fixes #11198 will auto-close that issue on merge — so a decision the triage gate expressly reserved to a maintainer ends up documented against a false record. #11198 carries status/ready-for-human because, per its own triage stage-2 comment, "choosing between them is a product/privacy call, not an engineering one: 1. Shape-based masking … Keeps errors debuggable; can never be provably complete. 2. Fingerprint / argument-drop … Cannot leak by construction; loses diagnostic value." The only author statement in that thread resolves it the other way from this code (2026-09-11T10:07:40Z): "Policy: shape-based masking, not fingerprinting. The pass strips URL credentials, Authorization: Bearer <token> headers, secret-bearing flags (--token, --_authToken, --password), KEY=value env-style secrets … and ANSI/control chars. Masking over fingerprinting is deliberate: errors stay debuggable." Commit c62948304f (2026-09-12, roughly 19 hours later) deleted all five of those named pattern families and adopted strategy 2. The concrete cost: @zjunothing — sole core-telemetry owner, escalated twice in reviews 5184325298 and 5184495508 with "telemetry redaction policy is a maintainer decision" — rules on the policy by reading an issue thread whose last technical word says diagnostics are preserved and five shapes are masked, while the code being merged drops 100% of free-form error text on a default-on channel. The Fixes keyword then closes the issue with that contradiction as its final record, so nothing prompts a re-read. The PR description does state the tradeoff under Risk & Scope, which is why this is a Suggestion rather than a blocker — but the description does not reach the issue thread.
Before merge, post a correction on #11198 stating that the shipped policy is now strategy 2 (whole-field replacement with a fixed ***REDACTED*** marker at the enqueueLogEvent choke point, no shape matching), that the earlier "shape-based masking, not fingerprinting" comment is superseded, and that the diagnostic-loss tradeoff is the one recorded in this PR's Risk & Scope. Alternatively drop the Fixes keyword and let the maintainer close #11198 after ruling on the policy it was marked ready-for-human for.
Witness:
ISSUE #11198, author comment 2026-09-11T10:07:40Z:
"Policy: shape-based masking, not fingerprinting. The pass strips URL credentials,
Authorization: Bearer <token> headers, secret-bearing flags (--token, --_authToken,
--password), KEY=value env-style secrets ..., and ANSI/control chars."
HEAD c62948304f (2026-09-12), pattern-family census over qwen-logger.ts:
git show HEAD:.../qwen-logger.ts | grep -cE \
"Bearer|Authorization|--token|_authToken|password|stripAnsi|redactUrlCredentials|API_KEY|DSN"
-> 0 (same census at the round-3 anchor 566d6a74ad -> 18)
the only redaction left in the file:
:213 properties[key] = REDACTED_ERROR_TEXT;
:220 (event as RumExceptionEvent).message = REDACTED_ERROR_TEXT;
No behavioural probe applies to a documentary claim; both arms above are quoted from the issue evidence and from the tree at the reviewed commit.
中文说明
当前 head 落地的脱敏策略，与作者在关联 issue 上记录下来的策略正好相反，而 Fixes #11198 会在合并时自动关闭该 issue —— 于是一个被 triage gate 明确保留给维护者的决定，最终以一份错误的记录被关闭。
#11198 带 status/ready-for-human，正是因为其 triage stage-2 评论写明："在两者之间做选择是产品/隐私决定，不是工程决定：1. 基于形状的掩码 …… 保留错误可调试性，但永远无法被证明是完备的。2. 指纹化 / 丢弃参数 …… 结构上不可能泄漏，但失去诊断价值。" 而该 thread 中唯一的作者表态与当前代码方向相反（2026-09-11T10:07:40Z）："策略：基于形状的掩码，不是指纹化。该处理会清除 URL 凭据、Authorization: Bearer <token> 头、含密钥的命令参数（--token、--_authToken、--password）、KEY=value 形式的环境变量密钥……以及 ANSI/控制字符。选择掩码而非指纹化是有意为之：错误保持可调试。"
commit c62948304f（2026-09-12，约 19 小时后）删掉了上述全部五类被点名的模式，改用了方案 2。
具体代价：@zjunothing —— core-telemetry 唯一 owner，在评审 5184325298 与 5184495508 中两次被转交，并被告知"遥测脱敏策略是维护者的决定" —— 在判断该策略时读到的是一个 issue thread，而它最后的技术表述仍说诊断信息被保留、五类形状被掩码；但即将合并的代码在一条默认开启的通道上丢弃了 100% 的自由文本错误信息。随后 Fixes 关键字会以这份矛盾作为最终记录关闭该 issue，因此不会再有任何提示促使人们重读。
本 PR 描述确实在"风险与范围"中说明了这一取舍，所以这是一条建议而非阻断项 —— 但描述触达不到 issue thread。
修法：合并前在 #11198 上发一条更正，说明落地的策略现在是方案 2（在 enqueueLogEvent 收口点用固定的 ***REDACTED*** 标记整字段替换，不做任何形状匹配）、此前"基于形状的掩码，不是指纹化"的表态已被取代、以及诊断信息损失这一取舍即本 PR"风险与范围"所记录的内容。或者去掉 Fixes 关键字，让维护者在就该 issue 被标记 ready-for-human 的策略做出判断之后再关闭它。
上方 Witness：不存在适用于文档性主张的行为探针；两侧证据分别引自 issue 材料与被审 commit 上的代码树。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[Suggestion] R4-3: still stands — this head ships the opposite redaction policy from the one recorded on the linked issue, and Fixes #11198 will auto-close that issue on merge, so a decision the triage gate expressly reserved to a maintainer ends up documented against a false record. #11198 carries status/ready-for-human because its own triage records the choice as "a product/privacy call, not an engineering one: 1. Shape-based masking … Keeps errors debuggable; can never be provably complete. 2. Fingerprint / argument-drop … Cannot leak by construction; loses diagnostic value." The only author statement in that thread resolves it the other way from this code: "Policy: shape-based masking, not fingerprinting … Masking over fingerprinting is deliberate: errors stay debuggable." The code then deleted all five of the pattern families that comment named and adopted strategy 2. The sole core-telemetry owner — escalated twice with "telemetry redaction policy is a maintainer decision" — rules on the policy by reading an issue thread whose last technical word says diagnostics are preserved, while the code being merged drops 100% of free-form error text on a default-on channel. The Fixes keyword then closes the issue with that contradiction as its final record, so nothing prompts a re-read. No correction has been posted on the issue as of this head, and the description still carries Fixes #11198. The PR description does state the tradeoff under Risk & Scope, which is why this is a Suggestion rather than a blocker — but the description does not reach the issue thread.
Before merge, post a correction on #11198 stating that the shipped policy is now strategy 2 (whole-field replacement with a fixed ***REDACTED*** marker at the enqueueLogEvent choke point, no shape matching), that the earlier "shape-based masking, not fingerprinting" comment is superseded, and that the diagnostic-loss tradeoff is the one recorded in this PR's Risk & Scope. Alternatively drop the Fixes keyword and let the maintainer close #11198 after ruling on the policy it was marked ready-for-human for.
Witness:
ISSUE #11198, author comment 2026-09-11T10:07:40Z:
"Policy: shape-based masking, not fingerprinting. The pass strips URL credentials,
Authorization: Bearer <token> headers, secret-bearing flags (--token, --_authToken,
--password), KEY=value env-style secrets ..., and ANSI/control chars."
Pattern-family census over qwen-logger.ts, re-run this round at head ea640b45:
git show HEAD:.../qwen-logger.ts | grep -cE \
"Bearer|Authorization|--token|_authToken|password|stripAnsi|redactUrlCredentials|API_KEY|DSN"
-> 0 (same census at the round-3 anchor 566d6a74ad -> 18)
the only redaction left in the file:
:213 properties[key] = REDACTED_ERROR_TEXT;
:220 (event as RumExceptionEvent).message = REDACTED_ERROR_TEXT;
No behavioural probe applies to a documentary claim; both arms are quoted from the issue evidence and from the tree at the reviewed commit.
中文说明
本条依然成立：当前 head 落地的脱敏策略与关联 issue 上记录下来的策略正好相反，而 Fixes #11198 会在合并时自动关闭该 issue —— 于是一个被 triage gate 明确保留给维护者的决定，最终以一份错误的记录被关闭。
#11198 带 status/ready-for-human，正是因为其 triage 把这个选择记录为「产品/隐私决定，不是工程决定：1. 基于形状的掩码 …… 保留错误可调试性，但永远无法被证明是完备的。2. 指纹化 / 丢弃参数 …… 结构上不可能泄漏，但失去诊断价值。」而该 thread 中唯一的作者表态与当前代码方向相反：「策略：基于形状的掩码，不是指纹化 …… 选择掩码而非指纹化是有意为之：错误保持可调试。」随后代码删掉了那条评论点名的全部五类模式，改用了方案 2。
core-telemetry 唯一 owner —— 已被两次转交并告知「遥测脱敏策略是维护者的决定」—— 在判断该策略时读到的是一个 issue thread，而它最后的技术表述仍说诊断信息被保留；但即将合并的代码在一条默认开启的通道上丢弃了 100% 的自由文本错误信息。随后 Fixes 关键字会以这份矛盾作为最终记录关闭该 issue，因此不会再有任何提示促使人们重读。截至当前 head，issue 上没有更正说明，描述里仍然写着 Fixes #11198。本 PR 描述确实在「风险与范围」中说明了这一取舍，所以这是一条建议而非阻断项 —— 但描述触达不到 issue thread。
修法：合并前在 #11198 上发一条更正，说明落地的策略现在是方案 2（在 enqueueLogEvent 收口点用固定的 ***REDACTED*** 标记整字段替换，不做任何形状匹配）、此前「基于形状的掩码，不是指纹化」的表态已被取代、以及诊断信息损失这一取舍即本 PR「风险与范围」所记录的内容。或者去掉 Fixes 关键字，让维护者在就该 issue 被标记 ready-for-human 的策略做出判断之后再关闭它。
上方 Witness：不存在适用于文档性主张的行为探针；两侧证据分别引自 issue 材料与被审 commit 上的代码树（模式族普查在本轮于 head ea640b45 重新执行，结果为 0，而第 3 轮锚点 566d6a74ad 上为 18）。
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
Closeout at R5-1 is stale: both language versions of the PR description now say Four threads remain for explicit disposition:
Recommendation: explicitly accept or reject whole-field masking first. I favor keeping raw error text out of this default-on channel, with structured diagnostics designed separately. No broader privacy guarantee is claimed for unknown keys, snapshots, stack, or other telemetry sinks. No additional review run is requested for this comment-only pass. |
`redactEventErrorText` already replaces every string under `error_message` / `error_excerpt` / top-level `message` with a fixed marker at the enqueue boundary, so `logHookCallEvent`'s `getTelemetryLogPromptsEnabled()` gate on `properties['error']` could only ever emit the constant `'***REDACTED***'` — a consent flag that no longer gates any content. Hook failure is already signalled by `success` / `exit_code`, so the raw error text is dropped fail-closed instead, and the now-dead `'error'` entry is removed from `ERROR_TEXT_PROPERTY_KEYS`. The failed-hook test now drives a config with telemetry log prompts enabled and asserts `properties` carries no `error` key, which goes red when the gated assignment is restored. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtycy85205
github-actions
Bot
removed
the
review/self-reported
|
Removed the automatic-closing keyword from the PR and made the current whole-field replacement policy and pending maintainer decision explicit in its description. This corrects the stale completion claim; it does not decide the privacy-versus-diagnostics tradeoff. Leaving the policy thread open for that decision. 中文说明已移除 PR 的自动关闭关键字，并明确当前实现是整字段替换、仍需维护者决定隐私与诊断信息的取舍。此次仅纠正完成声明，不代替策略决策，因此保留本线程。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- dead 'error_excerpt' allowlist entry (qwen-logger.ts:106) - already reported (issue comment 5633025309; review summaries 5182358058, 5184937341)
- blanket top-level message redaction / invalid_chunk flattening (qwen-logger.ts:218) - already reported (comment 3995556248, R4-4)
- two-name allowlist fails open for unlisted fields (qwen-logger.ts:106) - already reported (comment 3994853920, R3-8; comment 3994853924, R3-10)
Not reviewed: test-efficacy probe — harnessValidated: null; its baseline run tripped the vitest globalSetup prerequisite guard, so the systematic revert/mutant/hunk sweep measured nothing for the four changed hunks (targeted mutations were executed instead by this round's verifier, in an isolated mirror).
Convergence: round 5 posted 6 inline comment(s), 3 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/core/src/telemetry/qwen-logger/qwen-logger.ts (findings in round 4; 2 more now); packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts (findings in round 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：test-efficacy probe — harnessValidated: null; its baseline run tripped the vitest globalSetup prerequisite guard, so the systematic revert/mutant/hunk sweep measured nothing for the four changed hunks (targeted mutations were executed instead by this round's verifier, in an isolated mirror).
收敛情况：第 5 轮发布了 6 条行内评论，其中 3 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/core/src/telemetry/qwen-logger/qwen-logger.ts（第 4 轮已出过发现，本轮又有 2 条）；packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts（第 4 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
|
||
| logger.enqueueLogEvent(event); | ||
|
|
||
| expect(event.message).toBe('***REDACTED***'); |
There was a problem hiding this comment.
[Suggestion] R4-2: still stands — redactEventErrorText rewrites the caller's event object in place, and every test that observes an event does so through that same mutated reference, so nothing in the suite pins the producer-to-property mapping or the object that is actually queued and serialized. enqueueLogEvent mutates its argument at :187/:207-221 and returns nothing, on a class re-exported as public package API (packages/core/src/index.ts:587), and createRumEvent shallow-spreads ...(properties || {}), so rumEvent.properties is the caller's inner object. This diff already shows the cost: the tool-call privacy expectation had to be rewritten from error_message: 'failed' to '***REDACTED***' purely because vi.spyOn records arguments by reference and the call-through spy mutated the recorded object afterwards. So remapping error_message: event.error (:574) to event.error_type ships the wrong field to the RUM endpoint with the entire telemetry suite green, while a behaviour-preserving copy-then-redact fix turns this new assertion red — which invites deleting the assertion instead of the mutation. No in-tree caller is harmed today (all ~45 call sites build fresh literals), so the cost is test observability plus a public-API footgun rather than a live bug.
Redact a shallow copy and queue the copy, leaving the caller's object untouched, then re-point the assertions at what actually ships — the queued deque ((logger['events'].toArray() as RumEvent[])[0], precedent at test:280) or createRumPayload() (precedent at test:163-176). If in-place mutation is deliberately the contract, stub the real implementation in the two producer tests instead (vi.spyOn(logger, 'enqueueLogEvent').mockImplementation(() => {})) and restore their raw-value expectations, so the mapping stays pinned.
Witness:
BASE (merge base e40bf35eb9 source+test) + mapping mutant error_message: event.error -> event.error_type:
AssertionError: - "error_message": "failed" / + "error_message": "unknown" Tests 1 failed | 43 passed (44)
PR (head ea640b45) + the same mutant:
src/telemetry/qwen-logger/qwen-logger.test.ts : Tests 44 passed (44)
src/telemetry (whole dir, 30 files) : Tests 1001 passed (1001)
=> MUTANT SURVIVES
Comparator self-test at head (symmetric remap of the NON-redacted sibling :573 error_type -> event.error):
AssertionError - "error_type": "unknown" / + "error_type": "failed" 1 failed | 43 passed => comparator live
Shallow copy-then-redact variant at :187: Tests 2 failed | 42 passed (44)
error text redaction > replaces error text at the enqueue boundary -> expected 'raw top-level error' to be '***REDACTED***'
The base/head pair is the deciding evidence: at the merge base the suite caught this remap, at head it does not, across 1001 telemetry tests. That is a measured loss of discriminating power caused by this diff. (Round 4 recorded witness: not run for this mutant; it has now been executed.)
One correction to the fix constraint this thread carried last round: the clone must stay shallow, but not because a test would go red. integration.test.circular.ts:76 does enqueue an event carrying a circular httpAgent, yet a JSON deep clone's throw lands inside enqueueLogEvent's own try and is swallowed by the catch at qwen-logger.ts:203-205 — measured: threw:false deque 0 -> 0 enqueued:false swallowed err:1, with that file still reporting Tests 2 passed (2). The real constraint is that silent drop, and it is invisible because neither *.test.circular.ts file is executed by any job (vitest's default include **/*.{test,spec}.?(c|m)[jt]s?(x) does not match them, and no npm script, workflow or config references them).
Acceptance criterion: the re-pointed error text redaction case must still go red when this.redactEventErrorText(event) is removed from enqueueLogEvent (qwen-logger.ts:187) while staying green under a copy-on-enqueue implementation — please run that mutation and confirm it reds.
中文说明
本条依然成立：redactEventErrorText 就地改写调用方传入的事件对象，而所有观察该事件的测试读到的都是同一个被改写过的引用，因此整个套件里没有任何东西钉住「生产者字段 → RUM 属性」的映射，也没有任何东西观察真正入队并被序列化的那个对象。enqueueLogEvent 在 :187/:207-221 改写自己的入参且不返回任何值，而这个类是包的公开 API（packages/core/src/index.ts:587）；createRumEvent 对 ...(properties || {}) 做浅展开，所以 rumEvent.properties 就是调用方的那个内层对象。
本 diff 已经暴露了这个代价：tool-call 隐私用例的期望值不得不从 error_message: 'failed' 改写成 '***REDACTED***'，纯粹是因为 vi.spyOn 按引用保存参数、而调用真实实现的 spy 在保存之后又改写了那个对象。于是把 error_message: event.error（:574）改成 event.error_type 会让错误字段上报到 RUM，而整个遥测套件全绿；反过来，一个保持行为不变的「复制后再脱敏」修法会让这条新断言变红 —— 这等于在诱导删掉断言而不是删掉变异。
今天没有仓库内调用方受损（约 45 处调用都现场构造字面量），所以代价是测试可观测性加上一个公开 API 陷阱，而不是线上缺陷。
修法：对副本脱敏并入队副本，让调用方对象保持原样；再把断言改为指向真正上报的内容 —— 队列（(logger['events'].toArray() as RumEvent[])[0]，先例见 test:280）或 createRumPayload()（先例见 test:163-176）。如果就地改写确实是有意为之的契约，那就在那两个生产者测试里替换掉真实实现（vi.spyOn(logger, 'enqueueLogEvent').mockImplementation(() => {})）并恢复它们对原始值的断言。
上方 Witness：base 与 head 两臂的对照是决定性证据 —— 在 merge base 上套件能抓到这个字段重映射，在 head 上抓不到（跨越 1001 个遥测测试）。这是本 diff 造成的、可测量的判别力损失。（第 4 轮记录的是 witness: not run，现已实际执行。）
对上一轮该 thread 所记约束的一处更正：克隆必须保持浅拷贝，但原因不是某个测试会变红。integration.test.circular.ts:76 确实入队了一个带循环引用 httpAgent 的事件，然而 JSON 深克隆抛出的异常落在 enqueueLogEvent 自己的 try 里，被 qwen-logger.ts:203-205 的 catch 吞掉 —— 实测：threw:false deque 0 -> 0 enqueued:false swallowed err:1，而该文件仍报告 Tests 2 passed (2)。真正的约束是这次静默丢弃，而它之所以不可见，是因为两个 *.test.circular.ts 文件根本不会被任何 job 执行（vitest 默认 include **/*.{test,spec}.?(c|m)[jt]s?(x) 匹配不到它们，也没有任何 npm script、workflow 或配置引用它们）。
验收标准：改为指向队列/payload 后的 error text redaction 用例，在移除 enqueueLogEvent 里的 this.redactEventErrorText(event)（qwen-logger.ts:187）时必须变红，而在「复制后脱敏」的实现下必须保持绿 —— 请实际执行该变异并确认它变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| exit_code: event.exit_code, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R4-1: (fix-induced) the reported input is closed — this round took the first of the two options that thread prescribed, deleting the gated properties['error'] = event.error assignment and dropping 'error' from ERROR_TEXT_PROPERTY_KEYS, so the switch that could only ever emit a constant is gone. But the change that closed it opened a new defect at the same site: the rationale recorded beside the fix ("the failure is already signalled by success / exit_code") holds only for hook_type: 'command', and exit_code is undefined for the other three hook types, so their failures now reach the sink with no cause discriminator at all. HookExecutionResult.exitCode is optional (hooks/types.ts:1340) and only the command runner sets it — a repo-wide grep for exitCode in packages/core/src/hooks (non-test) hits only the declaration, the read at hookEventHandler.ts:1085, and hookRunner.ts; httpHookRunner's four success: false results (:147, :189, :206, :331) carry only error, and the prompt/function runners never set it either. safeJsonStringify (:385) then drops the undefined key, so an HTTP hook that fails URL validation, one that is aborted, one that times out and one that hits a transport error all serialize to the byte-identical {hook_event_name, hook_type: 'http', hook_name, duration_ms, success: 0} — and logHookCallEvent writes exactly six properties at head, so nothing else discriminates cause. Hook-failure triage from usage telemetry is therefore impossible for 3 of 4 hook types. The command-hook half is narrower than it first looks: hookRunner.ts:1431-1439 sets exitCode ?? -1 for a signal kill, so signal vs. non-zero exit is distinguishable; what collapses is timeout vs. abort vs. spawn error, since hookRunner.ts:1234-1246 and :1454-1462 call finish() with no exitCode.
Keep the fail-closed drop of raw text, but re-establish a non-raw discriminator in the same properties record: forward the existing coarse HookExecutionOutcome union ('success' | 'blocking' | 'non_blocking_error' | 'cancelled', hooks/types.ts:124-128) through HookCallEvent, or derive an error_type/failure_kind enum from the failure class the way logToolCallEvent (:573) and logRipgrepRuntimeRecoveryEvent (:1005) already do. An enum carries no user or command text, so it does not weaken the privacy fix — and it also gives HookCallEvent.error a reader again.
Witness:
PROBE hook wire {"name":"hook_call#PostToolUse","properties":{"hook_event_name":"PostToolUse","hook_type":"http","hook_name":"notify","duration_ms":12,"success":0}} <- error: 'URL validation failed: blocked scheme'
PROBE hook wire {"name":"hook_call#PostToolUse","properties":{"hook_event_name":"PostToolUse","hook_type":"http","hook_name":"notify","duration_ms":12,"success":0}} <- error: 'HTTP hook execution cancelled (aborted): id'
PROBE hook wire {"name":"hook_call#PostToolUse","properties":{"hook_event_name":"PostToolUse","hook_type":"prompt","hook_name":"ctx","duration_ms":12,"success":0}} <- error: 'Prompt hook timed out after 5000ms'
PROBE hook wire {"name":"hook_call#PostToolUse","properties":{"hook_event_name":"PostToolUse","hook_type":"command","hook_name":"cleanup.sh","duration_ms":12,"success":0}} <- error: 'Hook timed out after 5000ms'
Two different HTTP failures serialize to byte-identical property sets; exit_code is absent
(not null) because flushToRum serializes with safeJsonStringify (qwen-logger.ts:385).
The new key must not be named error_message or error_excerpt — const ERROR_TEXT_PROPERTY_KEYS = ['error_message', 'error_excerpt']; (qwen-logger.ts:106) replaces any string under those keys with ***REDACTED*** at the enqueue boundary, so a discriminator so named would itself be erased. It must not be named error either, which expect(callArgs.properties).not.toHaveProperty('error'); (qwen-logger.test.ts:825) pins absent. error_type satisfies both: it is already the house classification key and is not in the allowlist.
Acceptance criterion: a new case in describe('logHookCallEvent') building a failed non-command hook — new HookCallEvent('PostToolUse', 'http', 'notify', { tool_name: 'shell' }, 200, false, undefined, undefined, undefined, undefined, 'HTTP hook execution cancelled (aborted): h1') — and asserting the discriminator (e.g. expect(rumEvent.properties.error_type).toBe('cancelled')). Removing the mapping must turn it red; the neighbouring test at :782 uses objectContaining, so adding a key does not disturb existing assertions.
中文说明
（本条由上一轮的修复引入）原报告的问题已经关闭 —— 本轮采用了该 thread 给出的两个选项中的第一个：删掉了带条件的 properties['error'] = event.error 赋值，并把 'error' 从 ERROR_TEXT_PROPERTY_KEYS 中去掉，于是那个只能输出常量的开关不复存在。但关闭它的这次改动在同一个位置打开了一个新缺陷：写在修复旁边的理由（「失败信息已由 success / exit_code 承载」）只对 hook_type: 'command' 成立，而另外三种 hook 类型的 exit_code 是 undefined，因此它们的失败现在到达 sink 时完全没有任何原因判别字段。
HookExecutionResult.exitCode 是可选的（hooks/types.ts:1340），且只有 command runner 会设置它 —— 在 packages/core/src/hooks（非测试）中对 exitCode 做全仓 grep，只命中声明本身、hookEventHandler.ts:1085 的读取，以及 hookRunner.ts；httpHookRunner 的四个 success: false 结果（:147、:189、:206、:331）只带 error，prompt/function runner 也从不设置它。随后 safeJsonStringify（:385）会丢弃 undefined 键，于是一个 URL 校验失败的 HTTP hook、一个被中止的、一个超时的、一个传输错误的，全部序列化成逐字节相同的 {hook_event_name, hook_type: 'http', hook_name, duration_ms, success: 0} —— 而 logHookCallEvent 在 head 上只写六个属性，没有任何其他字段能区分原因。因此 4 种 hook 类型中有 3 种无法再从使用统计里做失败分诊。
command hook 那一半比看上去要窄：hookRunner.ts:1431-1439 对被信号杀死的场景设置了 exitCode ?? -1，所以「信号杀死」与「非零退出」是可以区分的；真正塌缩的是超时 vs 中止 vs spawn 错误，因为 hookRunner.ts:1234-1246 与 :1454-1462 调用 finish() 时都没有 exitCode。
修法：保留对原始文本的 fail-closed 丢弃，但在同一个 properties 记录里重新建立一个非原始文本的判别字段 —— 把已有的粗粒度 HookExecutionOutcome 联合类型（'success' | 'blocking' | 'non_blocking_error' | 'cancelled'，hooks/types.ts:124-128）通过 HookCallEvent 传过来，或者像 logToolCallEvent（:573）与 logRipgrepRuntimeRecoveryEvent（:1005）那样，从失败类别推导出一个 error_type/failure_kind 枚举。枚举不携带任何用户或命令文本，因此不会削弱这次隐私修复 —— 而且它顺带让 HookCallEvent.error 重新有了读取方。
上方 Witness 是实测的 hook wire payload：两种不同的 HTTP 失败序列化出逐字节相同的属性集合；exit_code 是缺失（不是 null），因为 flushToRum 用 safeJsonStringify（qwen-logger.ts:385）序列化。
约束：新字段不能命名为 error_message 或 error_excerpt —— const ERROR_TEXT_PROPERTY_KEYS = ['error_message', 'error_excerpt'];（qwen-logger.ts:106）会在入队边界把这些键下的任何字符串替换为 ***REDACTED***，如此命名的判别字段自己就会被抹掉；也不能命名为 error，因为 expect(callArgs.properties).not.toHaveProperty('error');（qwen-logger.test.ts:825）钉住了该键必须不存在。error_type 同时满足这两条：它已经是本仓库的分类字段惯例，且不在 allowlist 中。
验收标准：在 describe('logHookCallEvent') 中新增一个用例，构造一个失败的非 command hook —— new HookCallEvent('PostToolUse', 'http', 'notify', { tool_name: 'shell' }, 200, false, undefined, undefined, undefined, undefined, 'HTTP hook execution cancelled (aborted): h1') —— 并断言该判别字段（例如 expect(rumEvent.properties.error_type).toBe('cancelled')）。移除该映射后它必须变红；相邻的 :782 用例使用 objectContaining，因此新增一个键不会干扰既有断言。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const callArgs = enqueueSpy.mock.calls[0][0]; | ||
| expect(callArgs.properties).not.toHaveProperty('error'); |
There was a problem hiding this comment.
[Suggestion] R5-2: this test was renamed to an effect-shaped claim ("without forwarding raw error text") and given the comment below stating that guarantee, but its only value-level guard is the absence of one key name — the raw strings its own fixture still carries are never checked against the serialized event. The fixture is still constructed with stderr: 'error output' and error: 'Command failed' (:789-801; the 11th positional is error, per telemetry/types.ts:1150-1176), so re-forwarding either string under any other property key — hook_error, failure_reason — ships raw hook error text to the RUM endpoint with this suite green: expect.objectContaining at :805-819 tolerates added keys, and not.toHaveProperty('error') names only the key this round deleted. This round made the test weaker rather than stronger — at c62948304f it asserted error: '***REDACTED***', a value assertion, and the merge kept only the key-absence one. The sibling prompt-privacy test 40 lines above (:762-779) already models the effect-shaped check for this exact event type, serializing the event and asserting not.toContain on the raw strings.
Witness:
INTACT (unmodified PR): Test Files 1 passed (1) / Tests 44 passed (44)
ARM A (restore the deleted line, key `error`):
FAIL > logHookCallEvent > should log a failed hook call event without forwarding raw error text -> 1 failed
ARM B (identical raw text, key `hook_error`):
Test Files 1 passed (1) <- suite stays GREEN
ARM B payload: {"type":"hook","name":"hook_call#PostToolUse","properties":{"hook_event_name":"PostToolUse",
"hook_type":"command","hook_name":"cleanup.sh","duration_ms":200,"success":0,"exit_code":1,
"hook_error":"Command failed"}}
The probe flips: re-forwarding under the deleted key goes red, re-forwarding the identical raw string one key over ships to the sink with 44/44 green.
| const callArgs = enqueueSpy.mock.calls[0][0]; | |
| expect(callArgs.properties).not.toHaveProperty('error'); | |
| const callArgs = enqueueSpy.mock.calls[0][0]; | |
| expect(callArgs.properties).not.toHaveProperty('error'); | |
| const serializedEvent = JSON.stringify(callArgs); | |
| for (const raw of ['Command failed', 'error output']) { | |
| expect(serializedEvent).not.toContain(raw); | |
| } |
Scope note: this covers the key-renaming hole only. The "raw text routed into snapshots" arm of the same scenario is already on this PR as R3-10 (author: "Still live at head c629483"), so the fix should not be written as though it closes that one too.
Acceptance criterion: the assertion above must go red if the lines this round deleted from logHookCallEvent are restored (properties['error'] = event.error;), or if the same text is routed through another key — that is the ARM B mutation, green today. Please run it and confirm.
中文说明
这个用例被改成了以效果命名的断言（「不转发原始错误文本」），并加上了下方这段说明该保证的注释，但它唯一的取值级守卫只是某一个键名不存在 —— 它自己的 fixture 里仍然带着的那些原始字符串，从来没有被拿去和序列化后的事件比对过。
fixture 仍然以 stderr: 'error output' 和 error: 'Command failed' 构造（:789-801；第 11 个位置参数是 error，见 telemetry/types.ts:1150-1176）。因此把这两个字符串中的任意一个改用别的属性键转发出去 —— hook_error、failure_reason —— 就会把原始 hook 错误文本送到 RUM 端点，而本套件依然是绿的：:805-819 的 expect.objectContaining 容忍新增键，而 not.toHaveProperty('error') 只点名了本轮删掉的那个键。
本轮实际上让这个用例变弱了而不是变强了 —— 在 c62948304f 上它断言的是 error: '***REDACTED***'，那是一个取值断言；合并之后只保留了「键不存在」这一个。上方 40 行处的同类 prompt 隐私用例（:762-779）已经为完全相同的事件类型示范了以效果为形状的写法：序列化事件，然后对原始字符串断言 not.toContain。
上方 Witness：探针发生了翻转 —— 用被删掉的那个键重新转发会变红；把完全相同的原始字符串换一个键转发，则会送到 sink，而 44/44 全绿。
范围说明：本条只覆盖「换键名」这个漏洞。同一场景里「原始文本被放进 snapshots」的那一半，已经作为 R3-10 存在于本 PR（作者回复：「在 head c629483 上依然存在」），所以修复不应写成好像连那一条也一起关闭了。
验收标准：如果本轮从 logHookCallEvent 删掉的那几行被恢复（properties['error'] = event.error;），或者同样的文本被换到另一个键下，上面的断言必须变红 —— 后者就是实测中的 ARM B 变异，今天是绿的。请实际执行并确认。
— qwen3.8-max via Qwen Code /review (v0.23.3)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed
packages/core/src/telemetry/qwen-logger/qwen-logger.tspackages/core/src/telemetry/qwen-logger/qwen-logger.test.ts
Findings
No blockers found.
Clean areas
- Single choke point:
redactEventErrorTextis called at the top ofenqueueLogEventbefore the event enters the deque. All callers route throughenqueueLogEvent, so no error text can bypass redaction. - Fail-closed hook approach: Instead of redacting
properties['error'], the PR removes the hook error forwarding code entirely. Hook failures still signal viasuccess: 0/exit_code. Most conservative option — eliminates the leakage class rather than sanitizing it. - Redaction coverage:
ERROR_TEXT_PROPERTY_KEYS = ['error_message', 'error_excerpt']plus top-levelmessageon exception/resource events covers all error-text carriers. - Type safety:
(event as RumExceptionEvent).messagecast is TypeScript-only narrowing;typeof message === 'string'guard prevents mutation on events without the field. - Test correctness: Uses
toEqualfor exact property-shape matching, correctly verifies redaction while preservingerror_type. Hook test correctly assertsnot.toHaveProperty('error').
Needs human review
- Diagnostic trade-off: Hook error text is now completely absent from telemetry. Confirm that
success: 0+exit_codeprovides sufficient diagnostic signal. - Stale CHANGES_REQUESTED: The standing review references regex patterns that no longer exist at HEAD. Should be dismissed if the current approach is accepted, otherwise
mergeStateStatusremainsBLOCKED.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE
核对基线：head ea640b45c2fde6767ebbff99712dac8b48461583。
历史阻塞问题：已解除
本 PR 历史上有三次 CHANGES_REQUESTED，最后一次是 2026-09-12T02:54:50Z 针对 41714d9f；当前 head 之后最新的一次 review（2026-09-12T14:57:47Z，针对 ea640b45）已降级为 COMMENTED，不再要求变更。43 条线程里 4 条未解决（R4-1、R4-2、R4-3、R5-2），逐条读过，全部是 [Suggestion] 级，没有 Critical、安全、数据损坏或回归级别的历史阻塞问题。按本渠道策略 Suggestion 不作为合入门禁。
本轮独立扫描：未发现 Critical
这是一个隐私收口改动，我按「是否存在绕过 redaction 的出口」和「是否存在未被覆盖的错误文本载体」两条主线核对：
- redaction 确实是唯一收口点。 出站队列
this.events的写入只有两处：enqueueLogEvent内的this.events.push(event)（:195），以及重试回填的this.events.unshift(eventsToRequeue[i])（:1175）。前者的第一行就是this.redactEventErrorText(event)（:187）；后者回填的是此前已经入过队、因而已经被改写过的事件，不会把原文重新带回来。发送侧this.events.toArray()（:304、:377）只从队列读取，没有旁路构造 payload 的路径。因此所有出站事件都经过 redaction。 - 错误文本的载体被覆盖完整。 该 sink 里携带错误文本的字段只有三个：
properties.error_message（:574、:703、:880-881、:908）、properties.error_excerpt，以及 exception 事件顶层的message（:696、:716）。ERROR_TEXT_PROPERTY_KEYS覆盖了前两个，redactEventErrorText的后半段单独处理了message（:218-221），且都做了typeof === 'string'判定，不会把非字符串值改坏。我另外按error|stack|stack_trace|details|excerpt|reason|message扫过整个文件的字段位，没有发现第四个承载原文的键。 - hook 路径的处理是收紧而非放松。 删掉的
if (event.error && this.config?.getTelemetryLogPromptsEnabled()) properties['error'] = event.error;原本会在开启 logPrompts 时把 hook 的原始错误文本放进properties.error——而这个键不在ERROR_TEXT_PROPERTY_KEYS里。直接删除该赋值后，这条路径不再产生任何错误原文，方向是更严格，不构成泄漏。 - 过度改写只发生在一处且是安全方向。
:729的message: \Content retry failed after ${event.total_attempts} attempts`是只含数字的构造串，也会被统一改写成REDACTED`。这是损失一点诊断信号，不是正确性问题，且失败方向是「少发数据」，可以接受。
CI：当前 head 上 Lint & Static、Test (ubuntu-latest, Node 22.x)、Integration Tests (no-AK, No Sandbox)、review-pr 全部 pass，没有由本 PR 引入的失败。
其余意见
未解决的 4 条 Suggestion 里有两条值得后续处理，但都不阻塞本次合入：R4-2 指出 redactEventErrorText 是就地改写传入的事件对象（生产路径上事件由各 create*Event 帮助函数在 enqueue 前逐次新建，因此不影响调用方状态，但测试若在 enqueue 后再观察同一对象就会看到被改写后的值，容易写出恒真断言）；R4-3 指出本 head 采用的 redaction 策略与关联 issue 上记录的相反，而 Fixes #11198 会自动关闭该 issue，建议在 issue 或 PR 里补一句说明以免留下矛盾记录。
结论：历史阻塞问题已解除，本轮未发现可证明的 Critical，提交 APPROVE。
|
Posted as a PR comment rather than a review: this head already carries a pending review by the author (11 drafted replies), and GitHub rejects a second pending review. Nothing was deleted. Reviewed at Verdict: no blocking defect at this head. The approach is right and strictly narrower than Two notes that are real but non-blocking. Both are already carried by the standing bot threads (
Two things I checked and found clean, so they are not findings:
On the standing |
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
已核对 head ea640b45c2fde6767ebbff99712dac8b48461583（vs merge-base 20ecdaf6b2）。required CI 全绿。方向我是赞成的：把遮罩从生产者侧移到 sink 入口（enqueueLogEvent → redactEventErrorText，qwen-logger.ts:184-221）比逐个生产者各自记得脱敏可靠，error_message / error_excerpt / exception message 三类字符串字段被整体替换为固定标记，按构造就不会漏。拦的是下面两条，都落在本改动自己身上。
1. 这个改动自己的隐私保证没有用例钉住（线程 R5-2，成立）
qwen-logger.test.ts:782-824 这条改名成 “without forwarding raw error text” 的用例，唯一的值级断言是 expect(callArgs.properties).not.toHaveProperty('error')，其余全是 expect.objectContaining（容忍新增键），而它自己的 fixture 仍然带着 stderr: 'error output' 与 error: 'Command failed'（第 10、11 位实参）。所以同样的原文换到 hook_error / failure_reason 任何别的键下，这条用例照绿、原文照发到 RUM。上面的注释还写着「no raw error property is forwarded to the sink」，即用例在声明一个它并不检查的保证；而 sink 的防护本身是按键名匹配的黑名单，对新键名不设防，这一点恰是需要值级断言的理由。
同文件 :762-779 已经示范了该写的形状：把事件序列化后对原文取 not.toContain。请把这条补上（对 fixture 里的两个原文串断言 not.toContain），否则本 PR 的核心契约只有键名缺席在守。
2. 上一轮的修法顺手让 4 类 hook 里 3 类的失败再无归因（线程 R4-1，成立）
本轮删掉了 properties['error'] = event.error（连同 ERROR_TEXT_PROPERTY_KEYS 里的 'error'），留下的注释理由是「失败已由 success / exit_code 表明」。这个前提只对 hook_type: 'command' 成立：HookExecutionResult.exitCode 是可选的（hooks/types.ts:1340），全仓非测试代码里只有 hookRunner.ts:1439 会写它（http / prompt / function 三类都不写），logHookCallEvent 只有 6 个属性，序列化时 undefined 键被丢弃，于是 HTTP hook 的 URL 校验失败、被中止、超时、传输错误四类在用量遥测里落成逐字节相同的记录，hook_type 之外没有任何区分因。
请给一个不含自由文本的归因位（例如固定的错误类别枚举，或对非命令类失败也落一个 sentinel），并保持整字段遮罩不回退；如果决定本轮就不补，请把这一点写进 PR 正文的取舍段并在 #11198 上记一笔，别让它随合并静默消失。
两条已核为过期或不阻塞的说明
- 线程 R4-3 说本 head 的隐私策略与
#11198上记录的方案相反、且Fixes #11198会在合并时自动关闭该 issue。策略相反这点确实成立（issue 上作者 2026-09-11T10:07 那条还在说 shape-based masking「让错误保持可调试」，现已改为整字段替换），但 自动关闭的前提在本 head 已不成立：PR 正文用的是Refs #11198（:49、:102），并明确写了「this PR must not automatically close that issue before the decision is recorded」。不过该 issue 仍带status/ready-for-human、category/security、scope/data-privacy且核心遥测 owner 已两次把「遮罩策略由维护者定」升级上来 —— 请在合并前先由维护者在#11198记下裁决（整字段替换 vs 形状遮罩），别让这个 P1 隐私决策随合并默认生效。 - 线程 R4-2（
redactEventErrorText就地改写调用方对象、createRumEvent浅拷贝properties，QwenLogger又是指向公开的包 API）我复核为事实，但正如提出者所说，当前 45 个左右调用点都传新构造的字面量，所以今天不伤生产，只伤测试可观察性与易用性。建议顺手改成先浅拷贝再脱敏、入队那份拷贝，断言指向真正被序列化的对象（同文件:280有现成写法）。
Merge origin/main into codex/issue-11198-redact-rum-errors. Main landed QwenLM#11649, which replaces the RUM enqueue boundary's `message`, `error_message`, and `error_excerpt` with a fixed marker outright. This branch masks only credential-bearing shapes in those same fields plus `stack` and `properties.error`. Both sides edited the one choke point, so the calls collided. Keep both passes rather than picking a side: the blanket one runs first and owns the three fields it replaces, then the targeted one covers what it leaves raw. Neither side's coverage is lost, and the stricter base-branch policy is not weakened. `redactErrorText` is a no-op on the marker, so the order is not output-load-bearing and only avoids re-scanning text that is about to be discarded.
6 participants
What this PR does
Usage-statistics telemetry forwards raw tool and provider error text to the RUM endpoint. This PR replaces the known free-form error fields (
error_message,error_excerpt, and top-levelmessage) with a fixed***REDACTED***marker at the single enqueue boundary before an event can leave the process. (The hookerrorproperty is removed at its producerlogHookCallEventrather than marker-replaced, sohook_call#*.properties.erroris no longer collected at all.)Why it's needed
A failed shell command can contain a credential in a URL, an authorization header, a flag, an environment assignment, truncated output, or an unanticipated form. Parsing that untrusted text with a growing regex denylist cannot prove that the credential is gone and can itself become a CPU denial-of-service path. Replacing the whole error string is smaller and fails closed.
Reviewer Test Plan
How to verify
Run the focused unit suite:
The regression drives the public enqueue boundary with all currently known error-text properties plus a top-level exception message. It asserts that every raw value is replaced while a non-text classification field is unchanged.
Evidence (Before & After)
Before: an event whose error text contained
git clone https://x-access-token:ghs_testsecret123@github.com/...was enqueued with the complete command and token.After: the entire error-text field is enqueued as
***REDACTED***; no input substring is retained or parsed.Tested on
Environment (optional)
Unit tests only —
npx vitest runinsidepackages/core.Risk & Scope
The implementation now replaces whole error fields, superseding the earlier shape-based masking proposal recorded on #11198. The privacy/diagnostics tradeoff still needs maintainer confirmation; this PR must not automatically close that issue before the decision is recorded.
snapshotsandstackare separate structured fields and are not changed here; any sensitive producer-to-sink path through them should be handled separately.Linked Issues
Refs #11198
中文说明
本 PR 做了什么
usage-statistics 遥测会把工具和 provider 的原始错误文本发送到 RUM endpoint。本 PR 在事件离开进程前的唯一入队边界，将当前已知的自由文本错误字段（
error_message、error_excerpt和顶层message）统一替换为固定的***REDACTED***标记。（hook 的error属性在其生产者logHookCallEvent处被移除、而非替换为标记，因此hook_call#*.properties.error不再被采集。）为什么需要它
失败的 shell 命令可能以 URL、Authorization header、命令参数、环境变量、截断输出或任意其他形式携带凭据。不断扩展正则黑名单无法证明凭据已被完整清除，还会为不可信错误文本引入 CPU 拒绝服务风险。整段替换更小，也能失败关闭。
Reviewer 测试计划
如何验证
运行聚焦的单元测试：
回归测试直接走公开的入队边界，覆盖当前所有已知错误文本属性和顶层异常消息；它断言原始值全部被替换，同时非文本分类字段保持不变。
证据（Before & After）
Before：错误文本包含
git clone https://x-access-token:ghs_testsecret123@github.com/...的事件，会带着完整命令和 token 入队。After：整段错误文本被替换为
***REDACTED***，不保留也不解析任何输入片段。测试环境
环境（可选）
仅单元测试——在
packages/core内运行npx vitest run。风险与范围
当前实现替换整个错误字段，已不同于 #11198 先前记录的按内容模式遮罩方案。隐私与诊断信息之间的取舍仍需维护者确认，因此本 PR 不应在决策记录之前自动关闭该 issue。
snapshots和stack是独立的结构化字段，本 PR 不修改；如果它们存在敏感的 producer-to-sink 路径，应单独处理。关联 Issue
Refs #11198