feat(core): expand ${session_id} in per-provider customHeaders - #11282
Conversation
Gateways like OpenCode Go reject requests without a stable
per-conversation header (x-opencode-session, enforced since 2026-09-06),
and customHeaders cannot carry runtime-dynamic values — the template
route was reviewed and rejected in the outbound-propagation design
(§12.7), which pre-specifies this setting instead.
Adds outboundCorrelation.sessionIdHeader { enabled, headerName,
trustedHosts }: default off with an empty host allowlist, HTTPS-only
exact-host matching, header-name token validation (an invalid name
skips the user branch and keeps the built-in first-party one), and the
enabled flag re-checked at the send site so the path fails closed. The
value resolves per request via the existing wrapFetchWithSessionId
seam, so /new and /resume rotate it without rebuilding the client.
The built-in Routify allowlist behaviour is unchanged.
Closes #10995
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR — re-running the gate at the new head Template looks good ✓ Problem: observed, not theoretical. #10995 is open and asks for exactly this mechanism, and the trigger is dated and external — OpenCode Go started rejecting requests without Direction: aligned. The interesting part is that this revision is a reduction. The earlier global Size: core paths are touched ( Approach: the scope feels right and I don't see anything to cut. The placeholder set is deliberately closed at one entry with a comment saying it should not grow without the same review, the gate is a strict Risk: Stage 1e matched. Moving on to code review. 🔍 中文说明感谢贡献！在新的 head 模板完整 ✓ 问题： 已观测，不是理论性加固。#10995 处于 open 状态，要求的正是这个机制；触发点有明确日期且来自外部——OpenCode Go 自 2026-09-06 起拒绝缺少 方向： 对齐。有意思的是这一版是一次收缩。早先的全局 规模： 触及核心路径（ 方案： 范围合理，我看不到需要砍掉的东西。占位符集合刻意收敛为一项，并在注释中写明「未经同等审查不应增长」；开关使用严格的 风险： Stage 1e 命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewStatic review only — this is an unattended CI run, so nothing in the PR tree was built, executed or checked out. All citations below were read from the head blobs at The reason this pass is mostly verification rather than discovery: D1 — Gemini expansion unreachable for a baseUrl-less entry: fixed. The prescribed acceptance test is present too — D2 — non-string D3 — the false provenance guarantee, and The documentation half was narrowed everywhere I checked, and narrowed to the same sentence rather than to seven variants of it: Also gone: my own earlier blocker. The ordering defect I flagged at One new observation, non-blocking
I don't think this blocks. The recipient set does not widen: it is still the same three first-party Routify hosts, still HTTPS-only, still exact-host matched — no third party gains anything, and session affinity on a ModelRouter-routed request is consistent with what the header is for. But Not verifiedThree of the four wire-reaching sequenceDiagram
participant P1 as settings customHeaders
participant P2 as envVarResolver
participant P3 as Provider client
participant P4 as fetch wrapper
participant P5 as Gemini httpOptions
participant P6 as Gateway
P1->>P2: resolve env placeholders at load
P2-->>P3: session_id tokens preserved literally
P3->>P3: warn if gate off, filter placeholder entries out of client options
P3->>P4: OpenAI, Anthropic, WebSearch install wrapper
P4->>P4: per request, gate on then expand, else drop header
P4->>P6: request carries expanded value
P3->>P5: Gemini re-emits subset per request
P5->>P5: expand above destination guard, built-in header stays last
P5->>P6: request carries expanded value
Files changed (20)
Test evidenceThe PR's own CI, read from the check-runs API for the reviewed commit — nothing was re-run here. 44 check-runs on
Nothing is red on this commit, so there is no failing job log to excerpt. On the three skips — they are pre-existing infrastructure behaviour, not a gap this PR created. I checked rather than assumed, because an earlier review pass on this PR requested changes partly on the grounds that The two Sandboxed verification would settle the remaining gap: 中文说明代码审查。 仅静态审查——这是无人值守的 CI 运行，PR 代码树没有被构建、执行或 checkout。以下所有引用都是透过 contents API 从 head 这一轮以核验为主而非发现新问题，原因是： D1（Gemini 展开对无 baseUrl 条目不可达）已修。 D2（非字符串 customHeaders 值会让每个请求抛错）已修。 两个抛错点都加了守卫，这一点很关键，因为只补一个不够： D3（虚假的 provenance 保证，以及 我自己先前的阻塞项也已消失。 我在 一处新观察，非阻塞。 我不认为这构成阻塞。接收方集合没有变宽：仍然是同样三个一方 Routify host，仍然仅 HTTPS，仍然精确 host 匹配——没有任何第三方获得新东西，而在经 ModelRouter 转发的请求上带会话亲和性，与该 header 的用途是一致的。但 未验证部分。 四条上网的 测试证据。 以上为被审提交自身 CI 的结果，透过 check-runs API 读取，本轮没有重跑任何测试。 关于三个 skipped——它们是既有的基础设施行为，不是本 PR 造成的缺口。 我做了核对而非假设，因为本 PR 早先的一轮审查曾部分以 仍在运行的两个 沙箱验证可以补齐剩余缺口： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the three Criticals are genuinely fixed in the direction the reviewer prescribed rather than around it, and the two things still standing are a doc sentence and three missing wiring tests, neither of which I'd hold a merge over at this round count. My independent proposal, formed from the title and the motivation before I read a line of the diff: substitute at the point where Stepping back on whether this should exist at all: yes. The need is external and dated, the issue asking for it is open, and the alternative is a user forking their traffic through a proxy to inject a header. The revision history is the part that actually raised my confidence — this PR got smaller and simpler under review, dropping a global header name and a On the six-months-from-now test: I'd thank the author. The placeholder set is closed at one entry with a comment saying it should not grow without the same review, the gate is What keeps it at 4 rather than 5, named plainly. First, I also want to correct the record on my own earlier pass. At Am I approving because it's good or because I ran out of reasons to say no? The former. Every blocking finding filed against this PR — mine and 中文说明Confidence: 4/5 —— 三个 Critical 是真正按审阅者给出的方向修掉的，不是绕过去的；仍然存在的两件事是一句文档和三个缺失的接线测试，在这一轮次上我都不认为足以拦住合并。 我的独立方案，是在读 diff 之前仅凭标题与动机形成的：在 退一步看这东西该不该存在：该。需求来自外部且有明确日期，提出它的 issue 处于 open 状态，而替代方案是用户自己搭代理去注入 header。真正提升我信心的是修订历史——这个 PR 在审查过程中变得更小、更简单，砍掉了一个全局 header 名、一个 按「六个月之后」这条标准：我会感谢作者。占位符集合收敛为一项，并注明未经同等审查不应增长；开关是 停在 4 分而非 5 分的原因，直说。其一， 我也想更正我自己上一轮的记录。在 我批准是因为它好，还是因为我说不出反对的理由了？是前者。针对本 PR 提出的每一个阻塞项——我的，以及 — 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.
Needs some rethinking — see my notes above. 🙏
One blocker: buildSessionIdHeaders calls the new getOutboundSessionIdHeaderSettings() inside the existing try, before the built-in branch short-circuits. Any Config collaborator without that method throws, the catch swallows it into a debug warning and returns {} — so the first-party Routify session_id header that ships today is silently dropped too. Four existing test files build partial as unknown as Config mocks with getSessionId only and assert that header (core/openaiContentGenerator/provider/default.test.ts, .../dashscope.test.ts, core/anthropicContentGenerator/anthropicContentGenerator.test.ts, core/llm-content-generator/llm-content-generator.test.ts); none are updated here, and the cast is why tsc doesn't complain.
Preferred fix: resolve and return the built-in branch before touching the new getter, so a setting that defaults to off is structurally unable to affect a path that defaults to on.
Also: docs/design/2026-09-03-outbound-session-id-header.md still says the behavior is "intentionally not configurable" and uses no "user-configurable allowlist" — please update or supersede it, and bring the threat-model section §12.7 asked this follow-up for.
The feature direction itself looks right and matches what the design doc pre-specified. Details in the stage 2 and stage 3 comments.
…-in branch
`buildSessionIdHeaders` called `getOutboundSessionIdHeaderSettings()`
before the built-in short-circuit and inside the shared `try`. A `Config`
collaborator without that method threw, the catch swallowed it into a
debug warning and returned `{}` — so the first-party Routify `session_id`
header that ships today was dropped too. CI caught it: 7 failures across
the four provider suites that build partial `as unknown as Config`
doubles (`AssertionError: expected null to be 'test-session'`).
Rather than teach those four suites about a setting they do not use, make
the branches independent by construction: the opt-in branch now resolves
through `configuredSessionHeaderName()`, which cannot throw. A setting
that ships off is structurally unable to alter the path that ships on.
Also in the same seam:
- Re-check the flag as `enabled === true`, not `enabled !== false`. The
old test treated a settings object with no `enabled` field as ON, which
is fail-open — the opposite of what the comment claimed to guarantee.
- Match the trusted host before validating the header name, and warn once
per distinct name. An invalid name previously logged on every outbound
HTTPS request, including ones to hosts that were never listed.
- Collapse a configured name that differs from `session_id` only by case,
since header names are case-insensitive on the wire.
Docs: `docs/design/2026-09-03-outbound-session-id-header.md` still said
the behavior is "intentionally not configurable" with "no user-configurable
allowlist". Updated, and given the threat-model section that §12.7 of the
outbound propagation design asked this follow-up to bring: recipient set,
de-anonymization window, redirect forwarding, misconfiguration, and why a
per-request UUID is out of scope. `settings.md` gains the redirect,
punycode and debug-log caveats; the `headerName` JSDoc no longer
contradicts the regex it describes.
Tests: the built-in header surviving a Config without the getter, an
absent `enabled` field treated as off, and the case-only-difference
collapse.
Not run locally (this box cannot): typecheck, build and vitest are CI's
to confirm. ESLint passes on the changed files.
The tests are no longer an unverified claim: 421 pass across six files, including the four provider suites whose built-in session_id assertions QwenLM#11282's first commit broke — they pass here unmodified. Also names what is still unverified: typecheck, the settings-schema regeneration (hand-written entry, CI's drift check is the authority), and the four provider call sites forwarding customHeaders, which is checked by reading only.
The branch was ~50 commits behind; merging before replacing the implementation so the PR diff shows only the new approach.
Replaces this PR's implementation. The previous approach added a global `outboundCorrelation.sessionIdHeader` with its own `trustedHosts` allowlist; this delivers the same user need through the mechanism #10995 actually asked for. Why the change of approach: - **`outboundCorrelation` is the wrong home.** It exists (from #4390) for correlation data *qwen-code itself* injects, is documented only in the developer telemetry docs, and defaults everything to off because it is optional. #10995 is the opposite on all three counts: the user configures it, they need it in the settings docs, and without it the gateway rejects every request. Filing it there repeats the category error #4390's review objected to, one level down. - **A single global `headerName` cannot serve two gateways.** One string and one host list mean a user with two gateways wanting different header names cannot express it, and two model entries on the same host cannot be distinguished at all. - **`trustedHosts` re-encodes `modelProviders[].baseUrl`.** A global setting has no scope of its own, so it has to rebuild one from the endpoint the user already wrote down. `customHeaders` is already per-provider, so scope comes for free: which hosts may receive the value is answered by the provider entry the header hangs on, and which providers need it by which entries carry it. No allowlist, and none of the host normalization, punycode conversion or wildcard rejection it required. What is kept from the #4390 review: the consent decision stays in `outboundCorrelation`, as `allowDynamicHeaderValues` (default false), because an expanded value carries live session state to a third party. It also stops a provider preset or extension from quietly promoting a `customHeaders` entry it ships into an identity header — provenance is lost once presets and user settings are merged. Fail-closed throughout: gate off, empty session ID, or a Config that cannot answer all drop the header rather than putting a literal `${session_id}` on the wire. Whether any placeholder exists is decided once at client construction, so providers without one keep the fetch wrapper's existing early return. Gemini needs its own path: its customHeaders are frozen into the SDK client options at construction, so placeholder-bearing entries are kept out of the client entirely and supplied per request instead. The built-in first-party Routify branch is untouched, and docs/design/2026-09-03-outbound-session-id-header.md stays accurate as written — the two mechanisms are complementary, which is how #10995 described them. Verified locally: 421 tests pass across the six affected core suites, including the four provider suites whose session_id assertions the earlier approach broke. Typecheck, build and the settings-schema regeneration are CI's to confirm — the schema entry was hand-written with its description extracted from settingsSchema.ts, so CI's drift check is the authority. Rationale and the comparison in full: docs/plans/2026-09-07-session-id-header-shape-comparison.md
yiliang114
changed the title
The rationale it carried is already in the PR description and the implementation commit message. The rest of it was review archaeology that does not belong in the repo.
#10995 treats writing `${session_id}` into a provider entry as the opt-in ("the header is only sent when the user configures it"). With the consent switch defaulting to off, a user following the issue's example got silence: the header was dropped and the only trace was a debug log that is normally disabled. Their observable symptom would be a gateway rejecting every request with nothing on screen to explain it — the same failure mode this PR's earlier approach was criticized for. Warn on the console at client construction instead, naming both the header and the setting to flip, once per distinct header set. The check cannot throw: it runs while a provider client is being built, and a partial Config must not break that. Docs now lead with the fact that setup is two steps, and say what the warning looks like. Also adds the end-to-end coverage that was missing: expansion through the seam the providers actually construct, the gate dropping the header rather than emitting the literal, rotation across sessions without a new client, and a provider with no placeholder keeping the untouched early-return path. 428 tests pass across the six affected core suites.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The existing Routify regression is fixed at this head, and the green CI covers the direct provider path. I found several remaining correctness and consent-boundary gaps; details are inline.
|
Resolved the review feedback in f1a52b6 after merging the latest main:
Verification: Core targeted tests 729/729; CLI settings tests 197/197; full workspace typecheck passed; changed-file ESLint passed; the root build compiled all earlier workspaces, and the initially failing final VS Code companion package passed on direct rerun after SDK output was present. All six inline threads are resolved and re-review has been requested from @qwen-code-ci-bot. |
|
Qwen Code review was cancelled before a review could be posted. Nothing failed and nothing is retried automatically: the run was cancelled — by an operator, an upstream event, or the job exceeding its execution time limit. If you still want a review of this PR, request one with |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head f1a52b6e906ed26cd125e1672ac6e68d259b1f68 (base main). Approving: every historical blocking finding is fixed on this commit, and a full read of the production surface turned up no Critical.
Historical blocking findings — verified against the code at this head
The CHANGES_REQUESTED blocker (first-party Routify session_id header silently dropped). Fixed. buildSessionIdHeaders (outbound-session-id.ts:38-60) no longer touches the new consent getter at all — it resolves the URL, checks the host allowlist and reads config.getSessionId(). The two places that do call getOutboundAllowDynamicHeaderValues() are both fail-safe against a partial Config: warnIfDynamicHeadersDisabled wraps the whole body in try/catch, and resolveDynamicHeaderValue calls it inside its try and returns undefined (drop the header) on a throw. A as unknown as Config mock with only getSessionId therefore still gets the built-in header, and the wrapper's early return (if (!sessionId && !expandsHeaders)) keeps the no-placeholder path byte-for-byte as before.
The five P1 findings and one P2 from the review at b9e79df0:
| Finding | Evidence at this head |
|---|---|
P1 ${session_id} was consumable by settings env expansion |
envVarResolver.ts:38-40 — if (match === '${session_id}' || isInternalSecretEnvVar(varName)) return match; reserves the exact braced token before any customEnv/process.env lookup, and the doc comment records the reservation |
| P1 malformed consent value could open the gate | config.ts constructor — allowDynamicHeaderValues: params.outboundCorrelation?.allowDynamicHeaderValues === true; the getter adds ?? false, so "false", 1, {}, [] all stay disabled |
| P1 Workspace scope could grant the consent | settingsUtils.ts:272 — { section: 'outboundCorrelation', key: 'allowDynamicHeaderValues' } added to WORKSPACE_RESTRICTED_SETTINGS, so a repository's .qwen/settings.json cannot flip it |
P1 Gemini let a custom session_id outrank the built-in value |
llm-content-generator.ts buildHttpOptions merges ...dynamicHeaders, ...sessionHeaders — the first-party header is written last and wins; the constructor also keeps placeholder-bearing entries out of the frozen client options entirely, so the literal never reaches the SDK client |
| P1 WebSearch bypassed the resolver and sent the literal | web-search.ts:653-672 — the client now installs buildSessionAwareFetch(runtimeOptions?.fetch, this.config, backend.customHeaders), and fetch: is placed after the runtimeOptions spread so it is not clobbered |
| P2 no operator-facing gate-off warning on Gemini | llm-content-generator.ts constructor calls warnIfDynamicHeadersDisabled(allCustomHeaders, cliConfig) before filtering, sharing the once-per-header-set console warning |
Critical-only scan of the current diff — nothing blocking
I read every production file this PR touches: outbound-dynamic-headers.ts (new, in full), outbound-session-id.ts (in full), envVarResolver.ts (in full), and the patches to config.ts, settingsSchema.ts, settingsUtils.ts, llm-content-generator.ts, provider/default.ts, provider/dashscope.ts, anthropicContentGenerator.ts and web-search.ts.
The fail-closed claims hold in the code: gate off → resolveDynamicHeaderValue returns undefined and applyDynamicHeaderValues deletes the header rather than sending ${session_id}; empty session ID → if (!resolved) return undefined, same drop; a Config that throws → caught, same drop. A value with no placeholder is returned untouched, and expandsHeaders is decided once at client construction, so every provider configured today keeps the wrapper's existing early return.
I also swept the non-test consumers of customHeaders across packages/core for a client that bakes them without installing the resolver — the four wire-reaching sites are provider/default.ts + provider/dashscope.ts (buildHeaders → defaultHeaders), anthropicContentGenerator.ts, llm-content-generator.ts and web-search.ts, and all five paths are covered (wrapper for the first three plus web-search, per-request re-emission for Gemini). anthropicContentGenerator.ts:712-722 reads customHeaders only to collect anthropic-beta flags, which cannot carry a session value out. No remaining bypass.
The Gemini design assumption — that buildHttpOptions runs per request, so /new and /resume rotate the value without rebuilding the client — is not something this PR introduces: the already-shipped first-party Routify header is supplied from the same method and depends on the same per-request hook.
CI at this head: Test (ubuntu-latest, Node 22.x), Lint & Static (which carries typecheck and the settings-schema drift check the PR body defers to), Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, TUI parity snapshots, OpenTUI no-flicker gate and both Desktop Shell lanes are green; review-pr is still running and is not treated as a gate. Nothing red.
Not audited, and not a gate under this review's scope: the ~350 lines of new and updated tests (outbound-dynamic-headers.test.ts, config.test.ts, web-search.test.ts, envVarResolver.test.ts, llm-content-generator.test.ts) and the three docs pages. The CHANGES_REQUESTED still showing on the page is anchored at the older head edb7f01c and is answered by the code above.
中文说明
在 head f1a52b6e 上执行 Critical-only 评审，结论为 Approve：历史阻塞问题全部在该提交上确认修复，生产代码全量通读后未发现 Critical。
历史阻塞问题： 门禁 Review 的阻塞项（一方 Routify session_id header 被静默丢弃）已修复——buildSessionIdHeaders 不再触碰新的同意开关 getter，而调用该 getter 的两处都有兜底（warnIfDynamicHeadersDisabled 全包 try/catch，resolveDynamicHeaderValue 在 try 内调用并在异常时返回 undefined 丢弃 header），只带 getSessionId 的部分 mock 仍能拿到内置 header，无占位符路径保持原有提前返回。b9e79df0 那轮的 5 条 P1 与 1 条 P2 也已逐条按代码核对修复：${session_id} 在 env 展开中被精确保留、同意开关只认字面量 true、该键已加入 WORKSPACE_RESTRICTED_SETTINGS、Gemini 侧内置 header 后写因而优先且带占位符的条目根本不进 client、WebSearch 已接入 buildSessionAwareFetch 且 fetch: 位于 spread 之后不被覆盖、Gemini 构造时补上了共享的 gate-off 警告。
本轮扫描： 已通读全部生产文件。fail-closed 三条路径（开关关闭、session ID 为空、Config 抛错）在代码中均为丢弃 header，绝不会把字面量发出去；无占位符的值原样返回，是否展开在 client 构造时判定一次。另外横扫了 packages/core 中所有非测试的 customHeaders 消费点，五条上网路径全部覆盖，未发现绕过 resolver 的客户端；anthropic-beta 那条读取不可能携带会话值。Gemini 依赖 buildHttpOptions 按请求执行这一前提并非本 PR 引入——已上线的一方 Routify header 同样出自该方法。
CI：本 head 上 Test (ubuntu)、Lint & Static（含 typecheck 与 PR 正文所依赖的 schema drift 检查）、no-AK 集成、web-shell E2E、tui-parity、no-flicker、Desktop Shell 全绿，review-pr 仍在运行、不作为卡点。
未审查（不属本次门禁范围）：约 350 行新增/修改测试与三个文档页面。页面上仍显示的 CHANGES_REQUESTED 锚定在旧 head edb7f01c，已由上述代码回答。
|
Verified at I ran the current TypeScript CLI ( First, the consent A/B behaved as documented: with the gate off, two outbound requests omitted the placeholder-bearing header, preserved a static control header, and never sent the literal I then matched the OpenCode Go configuration directly: {
"baseUrl": "http://127.0.0.1:<port>/zen/go/v1",
"generationConfig": {
"customHeaders": {
"x-opencode-session": "${session_id}"
}
}
}In one running interactive CLI process (PTY, no tmux), the receiver captured:
The static control header stayed unchanged, all three requests completed successfully, and the three OpenAI JSON logs carried the matching session IDs. A machine assertion over the endpoint, exact header name, non-literal expansion, static-header preservation, and This directly verifies the client-side contract OpenCode Go needs: |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Deep (crosses a trust boundary — session ID forwarded to third-party hosts; auth/credential class applies).
Scope and what was reviewed
Source files for outbound-dynamic-headers.ts, outbound-session-id.ts, config.ts, settingsSchema.ts, settingsUtils.ts, envVarResolver.ts, and all four provider call sites (default.ts, dashscope.ts, anthropicContentGenerator.ts, web-search.ts). Test files reviewed as subject matter. Documentation reviewed against code behaviour.
Not reviewed: macOS/Windows execution (no host available — linux only); end-to-end against the real OpenCode Go gateway; npm run typecheck and full build; npm run generate:settings-schema output (schema entry hand-written; CI's drift check is the authority per the PR's own note).
Findings
No blocking findings.
Minor observations (body-only):
-
M1 —
warnedGateOffdedup scope (outbound-dynamic-headers.ts:26): the module-levelSetsuppresses the misconfiguration warning for a given header-name combination for the entire process lifetime. If a user reconfigures and re-creates the client, the warning will not fire a second time for the same header names. Intentional dedup; not incorrect. -
M2 — Provider call-site test coverage (self-reported by PR): the
anthropicContentGenerator,dashscope, anddefaultprovider constructors passcustomHeaderstobuildSessionAwareFetchwithout an explicit test verifying the wiring. Single new optional argument on each call;web-search.test.tscovers the equivalent for the search path; core expansion logic is thoroughly tested inoutbound-dynamic-headers.test.ts.
What was checked
- Fail-closed contract (
resolveDynamicHeaderValue): three drop paths confirmed — gate off, empty session ID,Configthrows — all returnundefined(drop the header). Covered by tests. - Static headers unaffected:
hasDynamicPlaceholderearly-return path verified; no-placeholder case skips expansion entirely. Test confirmsinitpassthrough is unchanged. envVarResolver.tsexclusion:match === '${session_id}'correctly preserves the placeholder even whenprocess.env.session_idis set; test added and verified.- Consent gating / coercion:
allowDynamicHeaderValues === true(strict equality) prevents truthy coercion from strings, numbers, or objects. Confirmed inconfig.test.tsadditions. - Workspace restriction:
allowDynamicHeaderValuesadded toWORKSPACE_RESTRICTED_SETTINGS— prevents a workspace preset from silently enabling the feature. - Gemini precedence:
...dynamicHeadersspread before...sessionHeadersinbuildHttpOptions; built-in first-party header wins. Confirmed in the updated test. - API backward compatibility:
buildSessionAwareFetchandwrapFetchWithSessionIdnewcustomHeadersparameter is optional; all five call sites confirmed updated. - Cross-check against prior reviews:
yiliang114's P1 findings (env-expansion bypass, consent coercion, workspace-scope grant, Routify precedence on Gemini, missing WebSearch wiring, Gemini gate-off warning) are all fixed at this head; each confirmed by reading the corresponding diff hunk.
No blocking findings. Approval blockers: none.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 808cbc56ba8780130c1368ff1063b367501172b5 (base main). This supersedes my APPROVE at f1a52b6e: the head moved (merge of main plus fix(core): close remaining dynamic header gaps), so that approval does not apply here.
Historical blocking findings on this head
| Finding | Verdict | Evidence |
|---|---|---|
| R1-1 / D1 — baseUrl-less Gemini/Vertex entry never sends the placeholder header | Fixed | llm-content-generator.ts buildHttpOptions now early-returns only on !this.cliConfig; destination is computed after expandDynamicHeaders, and sessionHeaders = destination ? buildSessionIdHeaders(...) : {} keeps the first-party header destination-gated. Merge order ...dynamicHeaders, ...sessionHeaders is unchanged, so the built-in value still wins. New test expands Gemini dynamic headers without a base URL asserts httpOptions.headers === { 'X-Gemini-Session': 'session-1' } with no baseUrl. |
R1-2 / D2 — non-string customHeaders value throws a TypeError out of every Gemini request |
Fixed | resolveDynamicHeaderValue starts with if (typeof value !== 'string') return undefined;, and expandDynamicHeaders skips with typeof value !== 'string' || !hasDynamicPlaceholder(value). That closes the only unguarded hasDynamicPlaceholder call site; the other three (warnIfDynamicHeadersDisabled, wrapFetchWithSessionId's expandsHeaders, applyDynamicHeaderValues via Headers) were already string-guarded. Non-string entries still take the pre-existing static path (baked into client options / defaultHeaders), so nothing that worked before now throws. New test ignores non-string runtime values. |
R1-3 / D3 — the consent gate's provenance guarantee was false, and ${QWEN_CODE_SESSION_ID} reached the live value with the gate off |
Partly fixed — see the Critical below | The false claim is gone from config.ts, outbound-dynamic-headers.ts, settingsSchema.ts, the generated settings.schema.json and model-providers.md ("does not identify which settings source supplied the header"), and the braced token is reserved in envVarResolver.ts. The second route is only half closed. |
Re-checked and still holding from the earlier rounds: buildSessionIdHeaders remains free of the consent getter (so a partial Config cannot drop the first-party Routify header), the gate is still === true in the Config constructor, outboundCorrelation.allowDynamicHeaderValues is still in WORKSPACE_RESTRICTED_SETTINGS, WebSearch still installs buildSessionAwareFetch with the entry's customHeaders, and the Gemini constructor still warns and keeps placeholder entries out of the client options.
Critical — the bare $QWEN_CODE_SESSION_ID spelling still bypasses the consent gate
The new reservation in packages/core/src/utils/envVarResolver.ts:37-42 compares the whole match against two exact braced spellings:
if (
match === '${session_id}' ||
match === '${QWEN_CODE_SESSION_ID}' ||
isInternalSecretEnvVar(varName)
) {
return match;
}The regex on the line above is /\$(?:(\w+)|{([^}]+)})/g, so it also matches the bare form $QWEN_CODE_SESSION_ID — for which match is '$QWEN_CODE_SESSION_ID' and neither comparison fires. isInternalSecretEnvVar does not catch it either: INTERNAL_SECRET_ENV_VARS (sanitize-child-env.ts:36-41) is QWEN_SERVER_TOKEN, QWEN_DAEMON_TOKEN, QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN and the ACP capability variable — the session ID is not in it. And the live value is demonstrably in this process's environment: config.ts:2497 and config.ts:4483 both do process.env['QWEN_CODE_SESSION_ID'] = this.sessionId.
So a repository-shipped .qwen/settings.json, provider preset or extension carrying
"customHeaders": { "x-opencode-session": "$QWEN_CODE_SESSION_ID" }is interpolated by the settings env resolver into an ordinary static header holding the live session UUID, with outboundCorrelation.allowDynamicHeaderValues off and no outboundCorrelation key anywhere — the exact outcome R1-3 described, reached through the sibling spelling. Nothing downstream drops it: hasDynamicPlaceholder is false for the already-substituted value, so neither applyDynamicHeaderValues nor expandDynamicHeaders sees a placeholder, and the header goes out on every request to whatever host the entry names.
Reachability, stated precisely rather than assumed: on the first settings load of a fresh CLI process the variable is usually not yet populated (the placeholder survives, because an unset variable is preserved), so the leak needs a settings resolution that happens after a Config exists in the same process — a long-lived qwen serve/daemon, a settings reload, or a second Config in-process. I ran out of budget before pinning the exact reload call path, so I am reporting this as blocking on the strength of what is proven (the reservation is spelling-specific; the env var is written by Config; the name is not an internal secret) instead of concluding it is unreachable.
Minimal fix, either of:
- Reserve by variable name, not by match spelling —
if (varName === 'session_id' || varName === 'QWEN_CODE_SESSION_ID' || isInternalSecretEnvVar(varName)) return match;— which covers$NAME,${NAME}and, on Windows, case variants for free; or - Add
QWEN_CODE_SESSION_IDtoINTERNAL_SECRET_ENV_VARS, which is already case-insensitive and already means "never substitute this from settings/extension files that can come from a repository".
Either way envVarResolver.test.ts should pin both spellings: the current it.each(['session_id', 'QWEN_CODE_SESSION_ID']) only exercises `\${${name}}`, so it passes today with the bare form still leaking.
CI
Everything reported at this head is green (Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, TUI parity snapshots, OpenTUI no-flicker gate, Desktop Shell lanes, E2E shards); two review-pr runs are still in progress and are not treated as a gate. The CHANGES_REQUESTED on the page is anchored at f1a52b6e.
Not audited, not a gate under this review's scope: the new and updated test bodies beyond the three quoted above, and the docs pages other than the provenance wording quoted.
中文说明
在 head 808cbc56 上执行 Critical-only 评审，取代我在 f1a52b6e 上的 Approve（head 已移动，那次批准不适用于本提交）。
历史阻塞问题： R1-1/D1（无 baseUrl 的 Gemini/Vertex 条目静默不发 header）已修复——buildHttpOptions 只在 !this.cliConfig 时提前返回，destination 移到展开之后计算，内置 header 仍按目的地判定且合并顺序保持优先，并新增了无 baseUrl 的展开测试；R1-2/D2（非字符串 customHeaders 值抛 TypeError）已修复——resolveDynamicHeaderValue 与 expandDynamicHeaders 双双加了 typeof value !== 'string' 保护，这是唯一未受保护的调用点，其余三处原本已有保护，非字符串值仍走原有静态路径；R1-3/D3 只修好一半——虚假的 provenance 承诺已从代码注释、文档、schema 与生成的 schema 中删除，带花括号的 ${QWEN_CODE_SESSION_ID} 也已被保留，但第二条路径没有完全关闭。此前几轮的结论（内置 Routify header 不受部分 Config 影响、开关只认字面量 true、该键在 Workspace 限制列表内、WebSearch 已接入 resolver、Gemini 构造时告警并过滤占位符条目）复核后依然成立。
当前 Critical： envVarResolver.ts:37-42 的保留判断是拿整个 match 与两个带花括号的字符串比较，而上一行的正则 /\$(?:(\w+)|{([^}]+)})/g 同样会匹配裸写法 $QWEN_CODE_SESSION_ID（此时 match 是 '$QWEN_CODE_SESSION_ID'，两个比较都不成立）；isInternalSecretEnvVar 也拦不住，因为 INTERNAL_SECRET_ENV_VARS 只含 QWEN_SERVER_TOKEN、QWEN_DAEMON_TOKEN、QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN 与 ACP capability 变量；而 config.ts:2497、:4483 确实执行 process.env['QWEN_CODE_SESSION_ID'] = this.sessionId。因此仓库自带的 .qwen/settings.json、provider preset 或扩展只要写 "x-opencode-session": "$QWEN_CODE_SESSION_ID"，就能在同意开关关闭、且配置里没有任何 outboundCorrelation 键的情况下，被设置层 env 解析替换成携带实时会话 UUID 的普通静态 header；替换后 hasDynamicPlaceholder 为 false，下游 fail-closed 路径一律看不到占位符，header 会随每个请求发往该条目指定的主机。
可达性如实说明：全新进程首次加载设置时该变量通常还没写入（未定义的变量会原样保留），所以泄漏需要「同进程中已存在 Config 之后」的一次设置解析——长驻的 qwen serve/daemon、设置重载或进程内第二个 Config。预算内我没能钉死具体的重载调用路径，因此按已证实的部分（保留判断只认特定写法、该 env 变量由 Config 写入、且它不属内部机密名单）作为阻塞项上报，而不是替它假设不可达。
最小修法二选一：①按变量名保留（varName === 'session_id' || varName === 'QWEN_CODE_SESSION_ID' || isInternalSecretEnvVar(varName)），一次覆盖 $NAME、${NAME} 与 Windows 大小写变体；②把 QWEN_CODE_SESSION_ID 加入 INTERNAL_SECRET_ENV_VARS（该列表本身大小写不敏感，语义也正是「来自仓库的设置/扩展文件永不替换」）。无论哪种，envVarResolver.test.ts 都应把两种写法都钉住——现有 it.each 只测 `\${${name}}`，所以裸写法仍在泄漏时测试依旧全绿。
CI： 本 head 上已出结果的检查全绿，两个 review-pr 仍在运行、不作为卡点；页面上的 CHANGES_REQUESTED 锚定在 f1a52b6e。
未审查（不属本次门禁范围）：除上面引用的三个用例之外的新增/修改测试正文，以及除 provenance 措辞外的文档页面。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard (re-review at new head; prior review dismissed at f1a52b6e).
Blocker
B1 — bare $QWEN_CODE_SESSION_ID bypasses the consent gate · supported
The reservation in envVarResolver.ts compares the entire regex match against two braced spellings ('${session_id}' and '${QWEN_CODE_SESSION_ID}'). The regex above it is /\$(?:(\w+)|{([^}]+)})/g, which also matches the bare form $QWEN_CODE_SESSION_ID. For that form match is '$QWEN_CODE_SESSION_ID' (no braces), neither comparison fires, and isInternalSecretEnvVar does not cover it. Meanwhile config.ts:2497 and config.ts:4483 both execute process.env['QWEN_CODE_SESSION_ID'] = this.sessionId, so after a Config exists the env var is live. A repository-shipped or extension-supplied
"customHeaders": { "x-opencode-session": "$QWEN_CODE_SESSION_ID" }is expanded by the settings env resolver into a plain static header carrying the session UUID, with outboundCorrelation.allowDynamicHeaderValues unset and no outboundCorrelation key in sight. Downstream hasDynamicPlaceholder sees no placeholder in the already-substituted value, so neither applyDynamicHeaderValues nor expandDynamicHeaders intercepts it — the header reaches every request to whatever host the provider entry names. Reachability requires a settings-string resolution that happens after Config sets the env var (a daemon/qwen serve reload, or a second in-process Config); the exact call path was not pinned, so the finding is reported as a static proof chain rather than an execution witness.
This was independently found by reviewer qqqys at this head; I confirm their analysis.
Minimal fix (either):
- Reserve by variable name rather than full match:
varName === 'session_id' || varName === 'QWEN_CODE_SESSION_ID' || isInternalSecretEnvVar(varName)— covers both$NAMEand${NAME}in one check. - Add
QWEN_CODE_SESSION_IDtoINTERNAL_SECRET_ENV_VARS(already case-insensitive, semantics match).
Either way, envVarResolver.test.ts should add a test for the bare $QWEN_CODE_SESSION_ID spelling alongside the existing braced tests.
Historical findings (from f1a52b6e round) — disposition at current head
These are confirmed as fixed by qqqys at 808cbc56; I verified the diff confirms the same:
| Finding | Verdict |
|---|---|
| R1-1 — baseUrl-less Gemini/Vertex never sends placeholder header | Fixed — buildHttpOptions early-returns only on !this.cliConfig; expandDynamicHeaders runs before destination computation; new test pins it. |
R1-2 — non-string customHeaders value threw TypeError |
Fixed — resolveDynamicHeaderValue guards typeof value !== 'string' up front; expandDynamicHeaders skips non-string entries. |
Scope
Checked: outbound-dynamic-headers.ts (full logic — gate, resolver, apply, expand, warn); outbound-session-id.ts (wrapper changes, expandsHeaders decision, precedence ordering); llm-content-generator.ts (static/dynamic split in constructor, buildHttpOptions merge order); envVarResolver.ts (reservation logic and test); config.ts (=== true strictness, getOutboundAllowDynamicHeaderValues); all four provider call sites (Anthropic, default OpenAI-compat, DashScope, WebSearch). Cross-file: the SESSION_ID_HEADER = 'session_id' constant confirmed; config.ts env-var write confirmed; INTERNAL_SECRET_ENV_VARS contents confirmed (session ID is not in it).
Not checked: docs pages beyond the provenance wording; test bodies beyond representative new tests; CI; Windows-path behaviour.
Approval withheld: B1 is a confirmed security/consent bypass.
Reviewed with AI assistance.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ All three Criticals filed at the previous head are fixed here, each with a test that pins it; the two remaining items are a stale scope sentence in the design doc and three unpinned provider forwards, both follow-up material rather than a sixth round.
Correction to our two dismissal messages on this PR — one claim in them is false, and false in the understating directionAt ~02:40Z I dismissed both of our stale rows on What is VOIDMy dismissal message's "non-blocking observation" said:
That clause is false. The bare spelling The executed measurementI ran the head version of
The mechanism is one line wide. The regex directly above the guard is This is already filed twice — I am not restating it, and not filing a third copy
What is UNAFFECTEDThe D1 and D2 verifications in the dismissal message stand, and are independently confirmed by review How I got it wrong, since that is the part worth recordingI verified the reachability premise myself — I read One item for a human, not for me
中文摘要（更正我们自己两条 dismissal 留言中的一处错误结论） 约 02:40Z 我们撤销了在 |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Request Changes at head 808cbc56ba8780130c1368ff1063b367501172b5.
I am not restating the finding — it is already filed on this head in my comment row 5136751605 and independently confirmed as B1 by @chiga0 in 5136807136. Both explicitly left the reachability leg unpinned and reported it as a static proof chain rather than an execution witness. This row supplies the missing witness, the missing reachability, and the missing review state.
1. Reproduced, not inferred
Probe built from the verbatim head blobs — packages/core/src/utils/envVarResolver.ts (blob 481c7e1c03c0) and packages/core/src/utils/sanitize-child-env.ts (blob ff95afd4a1fb), with only PRIVATE_ACP_CAPABILITY_ENV stubbed to its real head value 'QWEN_CODE_PRIVATE_ACP_CAPABILITY' — run under node v24.18.1 and the repo's own tsx:
| input | behaviour at head |
|---|---|
$QWEN_CODE_SESSION_ID (bare) |
LEAK → aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee |
${QWEN_CODE_SESSION_ID} (braced) |
preserved ✅ — the fix works for the spelling it covers |
$session_id (bare, with session_id present in env) |
LEAK → sid-from-env |
${session_id} (braced) |
preserved ✅ |
$R112_CONTROL (positive control) |
substituted ⇒ the harness is live, not vacuous |
$QWEN_SERVER_TOKEN (denylist control, present in env) |
preserved ⇒ the denylist leg works |
And through the object resolver on the realistic settings shape:
resolveEnvVarsInObject({ modelProviders: { acme: {
customHeaders: { 'x-opencode-session': '$QWEN_CODE_SESSION_ID' } } } })
→ customHeaders['x-opencode-session'] === 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'
Mutation witness. The probe discriminates the fix rather than merely reporting the status quo: changing only the reservation's two comparisons from match to varName flips both bare spellings to preserved and leaves both braced ones preserved.
- match === '${session_id}' ||
- match === '${QWEN_CODE_SESSION_ID}' ||
+ varName === 'session_id' ||
+ varName === 'QWEN_CODE_SESSION_ID' ||| input | after that one-line change |
|---|---|
$QWEN_CODE_SESSION_ID |
preserved ✅ |
$session_id |
preserved ✅ |
| both braced forms | preserved ✅ |
The blob was restored byte-identical afterwards and the leak re-reproduced, so the witness is not an artifact of a mutated tree.
2. Reachability, now pinned from primary source
Both prior reviews said the exact call path was not pinned. It is — and it does not require an exotic second Config.
- The variable is process-global and never released.
config.ts:1652declares module-levellet sessionEnvClaimed = false;:2509-2511claims it first-writer-wins in theConfigconstructor, writingprocess.env['QWEN_CODE_SESSION_ID'] = this.sessionIdat:2510.startNewSession(:4458) rewrites it unconditionally at:4498— its own comment at:4494says so — so/clear,/reset,/newand/resumeall refresh it within the same process. - Settings are re-resolved after that, on three separate production paths.
settings.ts:791sits insidereloadScopeFromDisk(scope), which re-reads the file from disk and re-runsresolveEnvVarsInObject(parsed); during a live session it is driven by the file watcher atsettingsWatcher.ts:379.settings.ts:1212-1224re-resolves every scope insideloadSettings(), which is called from ~20 sites inside the long-livedserveprocess (server.ts:1409,server.ts:2167,workspace-service/index.ts:264,routes/workspace-settings.ts,routes/workspace-models.ts, …). The repository documents this itself atshared-env-keys.ts:493: "daemon-sideloadSettings()re-runs the .env load for every session."extensionManager.ts:1654runsresolveEnvVarsInObject(config)over an installed extension's manifest — a path with no workspace-trust gate at all.
process.envwins by design.settings.ts:1205-1208: "effective precedence is:process.env> home.env> unresolved placeholder." OnceConfighas written the variable, substitution is the documented outcome rather than an accident.
What I proved vs. what I did not. Proven and executed: the substitution, its inputs, and that it survives into a customHeaders value. Not executed end-to-end: that the substituted header is then sent on the wire — I stopped at the resolver, so that last hop remains an inference, albeit the precise one the consent gate exists to prevent. Two nuances that cut against severity, which neither prior review raised: workspace-scope settings are dropped when the workspace is untrusted (fast-path-settings.ts:741, const workspace = isTrusted ? workspaceFromDisk : {}), and settingsWatcher.ts suppresses the change event for restart-required keys (providers among them) — but reloadScopeFromDisk performs the substitution before that suppression, and neither the daemon-side loadSettings() path nor the extension path is gated by either. The exposed surface is therefore: a trusted workspace's repo-shipped .qwen/settings.json, or an installed extension, in any process that already has a Config.
3. Why the two APPROVED rows on this head do not discharge it
qwen-code-dev-bot5136751283 was submitted at 02:42:48Z — 5 seconds before my Critical at 02:42:53Z. It cannot have considered it.qwen-code-ci-bot5136817246 was submitted at 02:52:30Z, ten minutes after, but its body adjudicates only "all three Criticals filed at the previous head" — it evaluated thef1a52b6eset, not this one.chiga05136807136 explicitly withheld approval on this same finding.
So the page currently reads "two approvals plus a comment" over a maintainer-confirmed consent bypass. This row is what makes the blocking state match the finding.
4. The test gap is narrower and more telling than "add a case"
envVarResolver.test.ts:61-69 asserts only the braced spelling:
it.each(['session_id', 'QWEN_CODE_SESSION_ID'])(
'preserves the runtime session ID placeholder %s',
(name) => {
process.env[name] = 'environment-session';
expect(resolveEnvVarsInString(`\${${name}}`)).toBe(`\${${name}}`);
},
);The immediately following block, describe('Qwen-internal secrets') at :76-82, already pins both spellings for the denylist ($QWEN_SERVER_TOKEN and ${QWEN_SERVER_TOKEN}, plus a mixed-case bare case at :84-88). So this file's own idiom for "never resolves" covers bare and braced, and the session-id reservation is the single test that omits the bare form — which is exactly why the fix reads as complete while half the surface still leaks. Section 1's table is the missing case.
5. Citation correction
Both prior reviews cite config.ts:2497 and config.ts:4483. At this head the two writes are at config.ts:2510 and config.ts:4498. Substance unchanged; correcting so the author is not sent to the wrong lines.
6. Minimal fix
Option 1 from my earlier comment, now with the witness behind it: reserve by variable name, not by match spelling (one line, section 1). It closes both names and both spellings at once. Option 2 (adding QWEN_CODE_SESSION_ID to INTERNAL_SECRET_ENV_VARS) closes only that one name and leaves bare $session_id relying on nothing ever populating a session_id env var — section 1 shows it leaks the moment one exists — so option 1 is strictly better. Either way, extend :61-69 to the bare spelling.
CI
Not a CI complaint. Complete census at this head: 131/131 check-runs fetched (sum(returned) == total_count asserted), 0 non-green live lanes after deduplicating by lane name and taking the newest completed_at; raw census 108 skipped / 20 success / 1 cancelled / 2 in_progress. Product lanes green: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, TUI parity snapshots, OpenTUI no-flicker gate, both Desktop Shell lanes.
中文说明
在 head 808cbc56ba 上提交 Request Changes。不重复已有发现——该 Critical 已由我本人的评论行 5136751605 与 @chiga0 的 B1（5136807136）在本 head 上提出并确认；两份评审都明确说明「未能钉死具体调用路径」，只给出静态证明链。本行补上缺失的执行证据、可达性路径与评审状态。
一、已复现，非推断。 探针由 head 原始 blob 构建（envVarResolver.ts blob 481c7e1c03c0、sanitize-child-env.ts blob ff95afd4a1fb，仅将 PRIVATE_ACP_CAPABILITY_ENV 桩为其真实值），node v24.18.1 + 仓库自带 tsx 运行。结果：裸写法 $QWEN_CODE_SESSION_ID 泄漏为实时会话 UUID；带花括号的 ${QWEN_CODE_SESSION_ID} 被正确保留；正向对照 $R112_CONTROL 被替换（证明探针非空转）；黑名单对照 $QWEN_SERVER_TOKEN 被保留（证明黑名单分支有效）。对象解析器走真实设置形状 modelProviders.acme.customHeaders['x-opencode-session'] 同样泄漏。变异见证：仅把两处比较从 match 改为 varName，两个裸写法即全部变为保留、两个花括号写法仍保留；随后 blob 已按字节还原并重跑出泄漏，证明见证不来自被改动的树。
二、可达性已从一手源码钉死（此前两份评审都留空）。① 该变量是进程级且永不释放：config.ts:1652 的模块级 sessionEnvClaimed、:2510 构造函数内首次写入、:4498 在 startNewSession 中无条件重写（:4494 注释自述），因此 /clear、/reset、/new、/resume 都会在同一进程内刷新它。② 之后有三条生产路径会重新解析设置：settings.ts:791（位于 reloadScopeFromDisk，由 settingsWatcher.ts:379 的文件监听在会话运行期驱动）、settings.ts:1212-1224（loadSettings()，在长驻 serve 进程中被约 20 处调用；仓库自身在 shared-env-keys.ts:493 写明「daemon-side loadSettings() re-runs the .env load for every session」）、extensionManager.ts:1654（扩展清单，完全没有 workspace trust 门禁）。③ settings.ts:1205-1208 明确记载优先级为 process.env > home .env > 保留占位符，即变量一旦写入，替换是文档化行为而非意外。
已证明 / 未证明：已执行证明的是替换本身及其输入、以及它会留在 customHeaders 值里；未端到端执行的是该 header 随后被发往网络——我在 resolver 处停止，最后一跳仍是推断（但这正是同意门禁要防的那一跳）。同时如实披露两处对严重性不利、此前评审未提的细节：workspace 作用域设置在未受信工作区会被丢弃（fast-path-settings.ts:741），且 settingsWatcher.ts 对 restart-required 键（含 providers）会抑制变更事件——但 reloadScopeFromDisk 的替换发生在该抑制之前，而 daemon 侧 loadSettings() 与扩展两条路径都不受这两者约束。因此暴露面是：已受信工作区中仓库自带的 .qwen/settings.json，或已安装的扩展，且进程中已存在 Config。
三、本 head 上两条 APPROVED 不构成放行。 dev-bot 5136751283 提交于 02:42:48Z，比我 02:42:53Z 的 Critical 早 5 秒，不可能考虑到它；ci-bot 5136817246 虽晚 10 分钟，但正文只裁定「上一 head 提出的三个 Critical 均已修复」，评的是 f1a52b6e 那一批；chiga0 5136807136 则就同一发现明确保留批准。因此页面目前呈现为「两条批准 + 一条评论」覆盖在一个经维护者确认的同意绕过之上，本行使阻塞状态与发现一致。
四、测试缺口比「补个用例」更具体。 envVarResolver.test.ts:61-69 只断言花括号写法；而紧随其后的 describe('Qwen-internal secrets')（:76-82）对黑名单同时钉住了裸写法与花括号写法（另有 :84-88 的大小写混合用例）。也就是说本文件自身「永不解析」的惯用写法已覆盖两种拼写，唯独会话 ID 的保留测试漏掉裸写法——这正是该修复看似完整、实则漏掉一半的原因。第 1 节的表格就是缺失的那个用例。
五、引用订正。 两份评审均引用 config.ts:2497 与 :4483；本 head 上实际为 :2510 与 :4498，结论不变，订正以免作者被指向错误行号。
六、最小修法。 采用我上一条评论的方案①（按变量名保留，一行），它对两个名字、两种拼写一次生效。方案②（把 QWEN_CODE_SESSION_ID 加入 INTERNAL_SECRET_ENV_VARS）只关掉这一个名字，且让裸写法 $session_id 依赖「永不有人设置 session_id 环境变量」——第 1 节显示一旦设置即泄漏——因此方案①严格更优。无论哪种，都应把 :61-69 扩展到裸写法。
CI： 不是 CI 问题。本 head 完整普查 131/131 条 check-run（已断言 sum(returned) == total_count），按 lane 名去重取最新 completed_at 后非绿灯 lane = 0；原始普查 108 skipped / 20 success / 1 cancelled / 2 in_progress。产品 lane 全绿。
|
Correction to my VOIDThat row opens by claiming it "supplies the missing witness". It does not. Comment Why my guard missed itBefore writing I re-enumerated live state and gated on: head unmoved, UNAFFECTEDThe finding itself — now filed three times at this head, all in agreement (
One stale sentence in
|
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.
10 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Gemini staticEntries filter unpinned — already reported at packages/core/src/core/llm-content-generator/llm-content-generator.ts:97 (comment 3952254354, by qwen-code-ci-bot)
- Provider buildSessionAwareFetch third argument unpinned — already reported at provider/default.ts:135 (comment 3952254449), provider/dashscope.ts:329 (comment 3952254454), anthropicContentGenerator.ts:368 (comment 3952254371, all by qwen-co…
- Gemini rotation test asserts a key the built-in header overwrites — already reported at llm-content-generator.test.ts:133 (comment 3952254360, by qwen-code-ci-bot)
- settings.md row omits the workspace restriction and restart requirement — already reported at docs/users/configuration/settings.md:671 (comments 3952254466 and 3952220453, by qwen-code-ci-bot)
- Wrapper tests hand a plain record where the SDK delivers a Headers instance — already reported at outbound-dynamic-headers.test.ts:223 (comment 3952254434, by qwen-code-ci-bot)
- Warning assertion depends on test declaration order — already reported at outbound-dynamic-headers.test.ts:238 (comment 3952220423, by qwen-code-ci-bot)
- web-search expansion test asserts a call count only — already reported at web-search.test.ts:694 (comment 3952220487, by qwen-code-ci-bot)
- Wrapper's own gate-off warning call site unpinned — already reported at outbound-session-id.ts:76 (comment 3952220481) and outbound-dynamic-headers.test.ts:238 (comment 3952254445, by qwen-code-ci-bot)
- Two-phase pending collect unpinned (no two-placeholder test) — already reported at outbound-dynamic-headers.test.ts:104 (comment 3952254419, by qwen-code-ci-bot)
- warnedGateOff dedupe unpinned and process-global — already reported at outbound-dynamic-headers.ts:65 (comment 3952220437) and :50 (comment 3952254396, by qwen-code-ci-bot); presubmit flagged this one as a (path, line) overlap
Not reviewed: build-and-test (integration suite) — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the 19-workspace build and the scoped unit suites did run.
Not reviewed: build-and-test (OS matrix) — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and this review ran on Linux only, so the case-insensitive process.env behaviour R2-1's casing arm turns on was modelled rather than executed.
Not reviewed: build-and-test (mutation probe) — the test-efficacy harness tripped this repo's vitest globalSetup build guard and returned no verdict (harnessValidated: null), so that probe yields no mutation evidence in either direction.
Not reviewed: issue-fidelity closing-issue discovery — the installed gh predates 2.72.0 and cannot resolve closing-issue references, so the linked-issue set is unknown rather than empty; issue 10995 was fetched from the description's own Closes line and the incident replay ran against it.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": the Anthropic SDK's delivery shape to its custom fetch ( anthropicContentGenerator.ts:364 ) — I verified only the OpenAI SDK's, so the Headers -vs-record que…; "agent reverse-audit (round 2)": did not run provider/default.test.ts , provider/dashscope.test.ts , or llm-content-generator.test.ts at this commit; I read the latter's new Gemini cases (l….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): src/core/outbound-dynamic-headers.test.ts — no such file or directory; src/core/outbound-session-id.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/default.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/dashscope.test.ts — no such file or directory; src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts — no such file or directory; and 2 more.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 10 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test (integration suite) — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; the 19-workspace build and the scoped unit suites did run.
未审查（原文为英文）：build-and-test (OS matrix) — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and this review ran on Linux only, so the case-insensitive process.env behaviour R2-1's casing arm turns on was modelled rather than executed.
未审查（原文为英文）：build-and-test (mutation probe) — the test-efficacy harness tripped this repo's vitest globalSetup build guard and returned no verdict (harnessValidated: null), so that probe yields no mutation evidence in either direction.
未审查（原文为英文）：issue-fidelity closing-issue discovery — the installed gh predates 2.72.0 and cannot resolve closing-issue references, so the linked-issue set is unknown rather than empty; issue 10995 was fetched from the description's own Closes line and the incident replay ran against it.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：the Anthropic SDK's delivery shape to its custom fetch ( anthropicContentGenerator.ts:364 ) — I verified only the OpenAI SDK's, so the Headers -vs-record que…；"agent reverse-audit (round 2)"：did not run provider/default.test.ts , provider/dashscope.test.ts , or llm-content-generator.test.ts at this commit; I read the latter's new Gemini cases (l…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：src/core/outbound-dynamic-headers.test.ts — no such file or directory; src/core/outbound-session-id.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/default.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/dashscope.test.ts — no such file or directory; src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts — no such file or directory; and 2 more。
— qwen3.8-max via Qwen Code /review (v0.23.0)
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.
10 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-6 Gemini constructor staticEntries filter unpinned at llm-content-generator.ts:97 — already reported (comment 3952254354)
- R1-11 wrapper header precedence unpinned at outbound-session-id.ts:89 — already reported (comment 3952254439)
- R1-13 provider customHeaders argument unpinned at provider/default.ts:135 — already reported (comment 3952220410)
- R1-13 web-search ungated first-party header at web-search.ts:672 — already reported (comment 3952220460)
- R1-17 workspace-restricted entry unpinned at settingsUtils.ts:272 — already reported (comment 3952220444)
- R1-18 host-scoping premise at outbound-dynamic-headers.ts:21 — already reported (comment 3952220490)
- R1-20 namespace opt-in criterion at settingsSchema.ts:1462 — already reported (comment 3952220495)
- R1-12 settings.md scope Notes column at settings.md:671 — already reported (comment 3952220453)
- R1-24 unrecognised token ships silently at outbound-dynamic-headers.test.ts:38 — already reported (comment 3952254415)
- R1-16 /resume rotation claim at design doc:44 and model-providers.md:589 — already reported (comments 3952220470, 3952220474)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox), Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and did not run locally; only the Linux unit suites ran.
Not reviewed: issue-fidelity — GitHub's closing-issue set could not be enumerated (the installed gh lacks closingIssuesReferences, which needs gh >= 2.72.0); fidelity was judged against issue 10995 fetched directly, so any other auto-closed issue cannot be ruled out.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": whether web-search.ts:320-333 can pair customHeaders from a *different* modelProviders entry than the entry.baseUrl it is sent to — getResolvedModelCon….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): src/core/outbound-dynamic-headers.test.ts — no such file or directory; src/core/outbound-session-id.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/default.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/dashscope.test.ts — no such file or directory; src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts — no such file or directory; and 2 more.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
docs/users/configuration/settings.md:689 — [review] docs promise the gate-off warning is printed at startup; it fires at client construction for the selected provider onlypackages/core/src/core/outbound-dynamic-headers.ts:70 — [probe] gate-off breadcrumb says "settings.json" and names no scope, but the key is user-scope onlypackages/core/src/config/config.ts:650 — [probe] "such as ${session_id}" advertises an open placeholder set while unrecognised tokens ship verbatimdocs/users/configuration/settings.md:664 — [review] developer telemetry page's SECURITY-RELEVANT outbound-correlation inventory misses the new namespace member
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 10 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox), Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI and did not run locally; only the Linux unit suites ran.
未审查（原文为英文）：issue-fidelity — GitHub's closing-issue set could not be enumerated (the installed gh lacks closingIssuesReferences, which needs gh >= 2.72.0); fidelity was judged against issue 10995 fetched directly, so any other auto-closed issue cannot be ruled out.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：whether web-search.ts:320-333 can pair customHeaders from a *different* modelProviders entry than the entry.baseUrl it is sent to — getResolvedModelCon…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：src/core/outbound-dynamic-headers.test.ts — no such file or directory; src/core/outbound-session-id.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/default.test.ts — no such file or directory; src/core/openaiContentGenerator/provider/dashscope.test.ts — no such file or directory; src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts — no such file or directory; and 2 more。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Closeout round at Fixed the confirmed B1 / R2-1 consent bypass. General settings interpolation now reserves both session variable names by name rather than by one literal spelling, covering bare, braced, and case-insensitive forms. The dynamic header resolver normalizes those equivalent spellings to the supported Verification:
The full workspace build reached Web Templates and then failed on the current Five suggestion-only threads were deferred with inline rationale under the repository policy that limits PRs after roughly five review rounds to Critical fixes. The original human reviewers have been requested again. |
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 1539d18754.
This head is a merge of main onto the commit I approved, so the thing to review is the merge, not the feature again. The feature-side state I certified at 808cbc56ba still holds: the re-emission site is reachable without a baseUrl (llm-content-generator.ts:125 returns only on a missing cliConfig, expansion happens above the destination computation, and sessionHeaders stays last in the merge), the non-string customHeaders shape is guarded at both throw sites, the six provenance claims say what the gate can actually promise, and ${QWEN_CODE_SESSION_ID} no longer reaches a header uninvited.
The merge did not stay out of this PR's own files, and what it changed there is a hardening I checked line by line. outbound-dynamic-headers.ts gained SESSION_ID_PLACEHOLDER_PATTERN (/\$(?:session_id|QWEN_CODE_SESSION_ID)(?!\w)|\$\{(?:session_id|QWEN_CODE_SESSION_ID)\}/gi) at :35-38, hasDynamicPlaceholder now matches through it, and resolveDynamicHeaderValue normalizes every spelling to the canonical token before expanding (:116-121). That closes the real bypass class this feature was exposed to: the gate keyed on the literal ${session_id}, so a $session_id, ${SESSION_ID} or ${QWEN_CODE_SESSION_ID} spelling in a customHeaders value was previously an ordinary static string — it never tripped hasDynamicPlaceholder, never appeared in the gate-off warning, and still delivered a live session identifier to whatever host the provider entry points at. envVarResolver.ts:35-44 was fixed on the same axis, reserving both names case-insensitively (previously an exact-string comparison, so ${Session_Id} in a shipped settings.json was interpolable at load time). The test additions match the severity: it.each over $session_id, ${SESSION_ID}, ${QWEN_CODE_SESSION_ID}, $QWEN_CODE_SESSION_ID, ${qwen_code_session_id} and an embedded sess=$QWEN_CODE_SESSION_ID, with the negative cases $session_id_suffix and {session_id} kept out, plus keeps runtime session environment values behind the consent gate. So the boundary is now enforced on the meaning rather than on one spelling — which is the property the feature needed and the one I could not have certified from the earlier head.
The rest of the delta is main arriving: config.ts takes the ledger option rename (tokenLedger → ledger) that landed there, and acpAgent.worktree.test.ts picks up main's additions. Nothing in the merge touched the exact-host allowlist in outbound-session-id.ts or the interactive-only registration that makes the Web Shell hand-off correct (config.ts:9467, :9656 — resolveInteractionMode(this) === 'interactive'), both of which I re-checked at this head.
CI facts. At this head nothing had finished: Lint & Static, Test (ubuntu-latest) and review-pr were still running when I submitted, and nothing has reported red. All 58 threads are resolved. Because the merge resolution is where I expected an evil-merge to hide, I read every touched PR-owned file rather than diffing only the feature commit — with CI pending, the executed proof of the new pattern tests is still ahead of us, and I would want the core lane green before this merges given it is the fix for a live-identifier leak class.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard (round 3; prior rounds at f1a52b6e round 1, 808cbc56ba87 round 2).
Round-2 blocker — fixed at this head
B1 — bare $QWEN_CODE_SESSION_ID bypasses the consent gate (filed round 2; independently confirmed by qqqys CHANGES_REQUESTED at 808cbc56ba87): Fixed in c7fbe88c2320 fix(core): gate session environment aliases.
envVarResolver.ts now extracts varName from the regex match groups (name only, without $ or {}), normalizes via .toUpperCase(), and compares against 'SESSION_ID' and 'QWEN_CODE_SESSION_ID'. This covers all four spellings — $session_id, ${session_id}, $QWEN_CODE_SESSION_ID, ${QWEN_CODE_SESSION_ID} — in one branch. Tests add explicit bare-form cases with process.env[name] set.
What was checked
envVarResolver.tsfix: extraction logic,normalizedVarNamepath, new bare-form tests.outbound-dynamic-headers.ts:SESSION_ID_PLACEHOLDER_PATTERN(case-insensitive, negative lookahead), normalization step inresolveDynamicHeaderValue, fail-closed contract (gate-off / empty session / Config throws →undefined).outbound-session-id.ts:expandsHeadersdecision at construction, precedence ordering (applyDynamicHeaderValuesbefore built-insessionIdwrite),warnIfDynamicHeadersDisabledguard.llm-content-generator.ts: static/dynamic split in constructor,buildHttpOptionsearly-return on!this.cliConfigonly, merge order (...dynamicHeadersbefore...sessionHeaders).- All four provider call sites (
default.ts,dashscope.ts,anthropicContentGenerator.ts,web-search.ts): all passcustomHeadersas third arg tobuildSessionAwareFetch. config.ts:=== truestrict equality,getOutboundAllowDynamicHeaderValues,WORKSPACE_RESTRICTED_SETTINGSentry.
Cross-check — CI-bot Suggestions already on record (not blocking)
R1-7 (warnedGateOff test ordering) — Confirmed. Module-level Set not cleared; both the warn test and the gate-off wrapper test produce key x-opencode-session. Declaration order is the only guard. Test-only issue.
R1-9 (sibling design doc contradiction) — Confirmed. telemetry-outbound-propagation-design.md §12.2 still records that customHeaders cannot carry runtime-dynamic values and that method C was chosen; §12.7 still describes the abandoned mechanism. Documentation debt, not a code defect.
R1-16 (/resume rotation claim) — Confirmed. Design doc and user guide say the identifier "rotates on /resume"; config.ts:4489 and useResumeCommand.ts:161 restore the persisted ID verbatim. De-anonymization window is the conversation lifetime, not one process run.
R1-15 (anthropic-beta placeholder) — Confirmed. Gate-off deletion of a placeholder-bearing anthropic-beta entry removes Qwen-computed beta flags too. Requires pathological config; Suggestion label appropriate.
R1-6 series (unpinned invariants / surviving mutants) — Cannot rule out without local build; recorded as unreviewed dimension. Credible witnesses; test-coverage gaps, not production-path failures.
Not covered
macOS/Windows execution; end-to-end against real OpenCode Go gateway; npm run typecheck; schema drift check.
No blocking findings. Approval blockers: none.
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 1539d187540ca0295db07a2f80875fdc0686ac58 (base main). Approving: the blocking finding I filed at 808cbc56 is fixed on this head, and the fix commit's own delta introduced no new Critical.
The blocking finding is closed — both layers, verified by reading this head
B1 / R2-1 — a bare or differently-cased $QWEN_CODE_SESSION_ID reached the live session value with the consent gate off. Fixed in c7fbe88c on two independent layers:
- Settings interpolation —
envVarResolver.ts:36-45no longer compares the whole match against two braced spellings. It takes the name from the regex groups and folds case:Soconst varName = varName1 || varName2; const normalizedVarName = varName.toUpperCase(); if ( normalizedVarName === 'SESSION_ID' || normalizedVarName === 'QWEN_CODE_SESSION_ID' || isInternalSecretEnvVar(varName) ) { return match; }
$session_id,${session_id},$QWEN_CODE_SESSION_ID,${QWEN_CODE_SESSION_ID}and case variants are all preserved verbatim and never substituted fromcustomEnvorprocess.env— which matters becauseconfig.tswritesprocess.env['QWEN_CODE_SESSION_ID'] = this.sessionId.envVarResolver.test.tsnow pins five spellings including the bare and lowercase-braced forms, with the env var set, asserting the string comes back unchanged. - The header path itself —
outbound-dynamic-headers.tsno longer keys on one literal token.SESSION_ID_PLACEHOLDER_PATTERNis case-insensitive and covers both bare and braced spellings of both names, with(?!\w)so$session_id_suffixis not caught;hasDynamicPlaceholderuses it, andresolveDynamicHeaderValuenormalizes any alias to${session_id}after the gate check, so an alias that somehow reached the header path is still gated — dropped when the switch is off, expanded only when it is on. The new tests assert both gate states for each alias, and one end-to-end case runs the real resolver first (resolveEnvVarsInString('sess=$QWEN_CODE_SESSION_ID', { QWEN_CODE_SESSION_ID: 'ambient-session' })stays literal) and then shows the gate, not the ambient env, deciding the value.
The warnIfDynamicHeadersDisabled / applyDynamicHeaderValues / expandDynamicHeaders call sites all go through the same widened predicate, so the wrapper, the Gemini re-emission and the warning agree on what counts as a placeholder.
Nothing else in the feature moved, and the earlier fixes still hold
The PR-relevant delta from 808cbc56 to this head is exactly envVarResolver.ts (+5/−3), outbound-dynamic-headers.ts (+10/−2) and their two test files; the rest of the range is the merge of main (the only config.ts change in it is the unrelated goal-runtime tokenLedger → ledger rename). Re-checked at this head:
- the consent gate is still strict —
config.ts:2633-2634allowDynamicHeaderValues: params.outboundCorrelation?.allowDynamicHeaderValues === true, getter?? falseat:7533-7534; outboundCorrelation.allowDynamicHeaderValuesis still inWORKSPACE_RESTRICTED_SETTINGS, so a repository cannot grant the consent;- the fail-closed shape is intact in the edited function: no placeholder → value returned untouched; gate off →
undefined(header dropped, never the literal); empty resolution or a throwingConfig→undefined; llm-content-generator.tsis unchanged, so the baseUrl-less Gemini/Vertex expansion above the destination guard and the...dynamicHeaders, ...sessionHeadersprecedence (built-in Routify wins) still stand, as do the non-stringcustomHeadersguards and the WebSearch routing throughbuildSessionAwareFetch.
CI
Green at this head: Test (ubuntu-latest, Node 22.x) — the lane that runs the touched core suites, so the new alias tests pass — Lint & Static (typecheck), Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, Desktop Shell lanes and the rest. Only review-pr is still running, which is not a gate. The CHANGES_REQUESTED state on the page is anchored at 808cbc56 and is answered by c7fbe88c.
Suggestion-level items deferred by the author this round (the "warning at startup" doc wording in two places, a stale JSDoc sentence about overriding a client-level copy, unpinned if (sessionId) and provider-forward invariants, the restart-required wording in the warning) were read and are deliberately not tracked here — they do not gate this verdict.
中文说明
在 head 1539d187 上执行 Critical-only 评审，结论为 Approve：我在 808cbc56 上提交的阻塞项已在本 head 修复，且修复提交自身的增量未引入新的 Critical。
阻塞项已关闭（两层，均按本 head 代码核实）： B1 / R2-1——裸写或大小写不同的 $QWEN_CODE_SESSION_ID 能在同意开关关闭时取到实时会话值。c7fbe88c 从两条独立路径修好：①设置层插值——envVarResolver.ts:36-45 不再拿整个 match 与两个带花括号的字符串比较，而是从正则分组取出变量名并 .toUpperCase() 归一，与 'SESSION_ID'、'QWEN_CODE_SESSION_ID' 比较，因此 $session_id、${session_id}、$QWEN_CODE_SESSION_ID、${QWEN_CODE_SESSION_ID} 及大小写变体一律原样保留，绝不会从 customEnv 或 process.env 替换（关键在于 config.ts 会写入 process.env['QWEN_CODE_SESSION_ID'] = this.sessionId）；envVarResolver.test.ts 现已钉住五种写法（含裸写与小写花括号），且是在设置了该环境变量的前提下断言原样返回。②header 路径本身——outbound-dynamic-headers.ts 不再只认单一字面 token：SESSION_ID_PLACEHOLDER_PATTERN 大小写不敏感、同时覆盖两个名字的裸写与花括号形式，并用 (?!\w) 排除 $session_id_suffix；hasDynamicPlaceholder 使用该模式，resolveDynamicHeaderValue 在同意开关判定之后才把别名归一为 ${session_id}，因此即便别名走到 header 路径也仍受开关约束——关闭即丢弃、开启才展开。新增测试对每个别名都断言了两种开关状态，并有一条端到端用例先跑真实 resolver（sess=$QWEN_CODE_SESSION_ID 在环境里有值时仍保持字面量），再证明决定权在开关而不在环境。warnIfDynamicHeadersDisabled、applyDynamicHeaderValues、expandDynamicHeaders 都走同一个放宽后的判定，因此 fetch wrapper、Gemini 按请求重发与告警三者对「什么算占位符」的判断一致。
其余特性代码未动，此前修复仍然成立： 从 808cbc56 到本 head，与本 PR 相关的增量恰好是 envVarResolver.ts（+5/−3）、outbound-dynamic-headers.ts（+10/−2）与两个测试文件，其余区间内容是 main 合并（其中唯一的 config.ts 变化是无关的 goal runtime tokenLedger → ledger 重命名）。在本 head 复核：同意开关仍是严格布尔（config.ts:2633-2634 的 === true，getter ?? false）；outboundCorrelation.allowDynamicHeaderValues 仍在 WORKSPACE_RESTRICTED_SETTINGS 中，仓库无法代为授权；被改函数仍保持 fail-closed（无占位符原样返回、开关关闭返回 undefined 丢弃 header 而绝不发字面量、解析为空或 Config 抛错同样丢弃）；llm-content-generator.ts 未变，因此无 baseUrl 的 Gemini/Vertex 展开仍在目的地判定之前、...dynamicHeaders, ...sessionHeaders 的优先级（内置 Routify 胜出）仍成立，非字符串 customHeaders 保护与 WebSearch 接入 buildSessionAwareFetch 同样未变。
CI： 本 head 上 Test (ubuntu-latest, Node 22.x)（会跑被改动的 core 套件，即新别名测试通过）、Lint & Static（typecheck）、no-AK 集成、web-shell E2E、两端 Desktop Shell 等全绿，仅 review-pr 仍在运行、不作为卡点。页面上的 CHANGES_REQUESTED 锚定在 808cbc56，已由 c7fbe88c 回答。
作者本轮延后的 Suggestion 级条目（两处「启动时告警」的文档措辞、关于覆盖 client 级副本的过时 JSDoc、未钉住的 if (sessionId) 与 provider 转发不变量、告警未说明需重启）已读取，按职责不作为门禁追踪。
|
The repeated WebShell E2E failure is an existing history-viewport race, not a regression in this PR's custom-header changes. Under slow virtualized rendering, pagination can prepend records before a visible reading row is available to anchor; the resulting drift is exactly 286 px. I split the minimal fix into #11366. The A/B verification is conclusive: with the fix, all 16 pressure executions that entered the history scenario completed with zero anchor drift; with only the production fix reverted and the same regression test retained, 18/18 executions failed with |
|
Released in v0.23.1. |
6 participants
What this PR does
Adds a
${session_id}placeholder formodelProviders[].generationConfig.customHeaders, expanded per request:{ "id": "glm-5.3-flash", "baseUrl": "https://opencode.ai/zen/go/v1", "generationConfig": { "customHeaders": { "x-opencode-session": "${session_id}" } } }Off by default. It takes one global consent switch to enable:
{ "outboundCorrelation": { "allowDynamicHeaderValues": true } }The value is resolved per request from
Config.getSessionId(), so/newand/resumerotate it without rebuilding the client.Why it's needed
OpenCode Go rejects requests without
x-opencode-sessionsince 2026-09-06.customHeadersaccepts only static strings today, so a settings entry gives one value per machine, never "one stable ID per conversation" — which is exactly what #10995 asked for.Note: this PR changed approach
An earlier revision of this PR added a global
outboundCorrelation.sessionIdHeaderwith its owntrustedHostsallowlist. That has been replaced. Three reasons, in order of weight:headerNamecannot serve two gateways. One header name and one host list mean a user running two gateways that want different header names cannot express it at all, and two model entries pointing at the same host cannot be distinguished.trustedHostsre-encodesmodelProviders[].baseUrl. A global setting has no scope of its own, so it has to rebuild one from the endpoint the user already wrote down — and then roughly 90 lines of host normalization, punycode conversion and wildcard rejection exist to police that second copy.outboundCorrelationis the wrong home for it. That namespace exists (from feat(telemetry): client-side HTTP span + opt-in W3C traceparent propagation (#4384) #4390's R4 split) for correlation data qwen-code itself injects; it is documented only in the developer telemetry docs, and everything under it defaults to off because it is optional. customHeaders: support a ${session_id} template for per-conversation request headers #10995 is the opposite on all three counts — the user configures it, it belongs in the settings docs, and without it the gateway rejects every request.customHeadersis already per-provider, so scope comes for free: which hosts may receive the value is answered by the provider entry the header hangs on, and which providers need it by which entries carry it. #10995 described the two mechanisms as complementary, and that is how they are implemented here — the built-in first-party Routify branch is untouched.What is kept from #4390's review. The consent decision stays in
outboundCorrelation, asallowDynamicHeaderValues(default false), because an expanded value carries live session state to a third party — that was the substance of the objection that created the namespace. The switch governs only whether${session_id}may be expanded from live session state; it does not identify which settings source supplied the header after settings are merged.Safety
Fail-closed on every path — the gate being off, an empty session ID, or a
Configthat cannot answer all drop the header rather than putting a literal${session_id}on the wire. A value with no placeholder is returned untouched, so everycustomHeadersentry configured today is unaffected.Whether any placeholder exists at all is decided once at client construction, so a provider without one keeps the fetch wrapper's existing early return and costs what it costs today.
Gemini needs its own path: its
customHeadersare frozen into the SDK client options at construction, so placeholder-bearing entries are kept out of the client entirely and supplied per request instead — the client never carries the literal.Reviewer Test Plan
npm run test --workspace=@qwen-code/qwen-code-core -- src/core/outbound-dynamic-headers.test.ts— 20 tests: expansion, repeated placeholders, gate closed, empty session ID, aConfigthat throws, header rewrite and deletion, the no-placeholder no-op.npm run test --workspace=@qwen-code/qwen-code-core -- src/core/outbound-session-id.test.ts src/core/openaiContentGenerator/provider/default.test.ts src/core/openaiContentGenerator/provider/dashscope.test.ts src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts src/core/llm-content-generator/llm-content-generator.test.ts— the existing suites this touches.npm run generate:settings-schema && git diff --exit-code packages/vscode-ide-companion/schemas/settings.schema.jsonnpm run typecheck,npm run lintEvidence
Run locally across all six affected suites:
Tested on
Risk & Scope
customHeadersvalue without a placeholder never touches the new code path.npm run typecheckand a full build were not run locally.npm run generate:settings-schemacould not be run here either — thesettings.schema.jsonentry was written by hand with its description extracted programmatically fromsettingsSchema.ts, so CI's schema-drift check is the authority; if it reports a diff, the generator output should be committed over it. No test covers the four provider call sites forwardingcustomHeadersintobuildSessionAwareFetch; that wiring is verified by reading only.session_idbehavior is unchanged.Linked Issues
Closes #10995.
中文说明
背景:OpenCode Go 自 2026-09-06 起强制要求
x-opencode-session,缺失即拒绝请求(#10995)。customHeaders目前只支持静态字符串,配出来的是"每台机器一个值",而网关要的是"每次会话一个稳定值"。实现:在
modelProviders[].generationConfig.customHeaders的值里支持${session_id}占位符,按请求展开。全局开关outboundCorrelation.allowDynamicHeaderValues默认关闭。本 PR 换过一次方案。 早先的版本加的是全局
outboundCorrelation.sessionIdHeader+trustedHosts白名单,已替换。原因:①全局单个headerName表达不了"两个网关要不同 header 名";②trustedHosts重复了用户已经在baseUrl里写过的信息,并为此需要约 90 行 host 归一化/punycode/通配符校验;③outboundCorrelation这个 namespace 是为"qwen-code 自己注入的关联数据"建的,只在开发者 telemetry 文档里出现,且全部默认关闭因为它是可选的——而 #10995 三条都相反:用户主动配、该进用户设置文档、不配就用不了。customHeaders本身就是 per-provider 的,作用域天然继承:发给哪些 host,由挂了这个 header 的 provider 条目回答;哪些 provider 需要,由哪些条目带了它回答。#10995 原文也把这两套机制描述为互补的,内置的一方 Routify 分支一行未动。保留自 #4390 review 的部分:同意决策仍在
outboundCorrelation下(allowDynamicHeaderValues,默认 false),因为展开后的值把实时会话状态送给了第三方;这个开关只控制${session_id}是否能从实时会话状态展开，并不会在配置合并后识别 header 的来源。安全:全路径 fail-closed —— 开关关闭、session ID 为空、Config 取不到值,都是丢弃该 header,绝不把字面量
${session_id}发出去。不含占位符的值原样返回,今天已有的所有配置都不走新代码。是否存在占位符在 client 构造时判定一次,没有占位符的 provider 保持原有的提前返回。Gemini 单独处理:它的 customHeaders 在构造时冻进 SDK client options,所以带占位符的条目根本不放进 client,只按请求提供。验证:六个受影响套件本地全绿,421 个测试。typecheck、build、schema 重新生成交给 CI —— schema 那条是手写的(描述字符串从 TS 源程序化提取),CI 的 drift 检查是最终权威。