refactor: centralize cross-package contracts - #9497
Merged
Merged
Conversation
yiliang114
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
wenshao
as code owners
August 19, 2026 17:39
github-actions
Bot
added
the
review/self-reported
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real and evidenced. #9151 names four concrete duplicate pairs with file:line references, and I verified the first three against the base commit: Direction: aligned. This is the internal counterpart to #9145 with tight, explicit scope: the four named pairs, no repo-wide de-dup pass, no new shared package, no value changes. Being internal contract hygiene, there's no external parity signal to cite; the justification stands on the repo's own drift history. Size: ~63 production lines (7 files), ~124 test lines (the new cross-package contract test + Approach: scope feels right — single owner for each of the two value contracts, distinct names for the two display-name helpers (renaming rather than changing behavior matches the issue's non-goals), and Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 真实且有据。#9151 给出了四组具体的重复声明及 file:line 引用，我在 base commit 上核实了前三组： 方向： 对齐。这是 #9145 的内部对应项，范围明确收敛：只处理点名的四组，不做全仓库去重、不新建共享 package、不改任何取值。属于内部契约卫生治理，没有外部对标信号可引用；其正当性由本仓库自身的漂移历史支撑。 规模： 约 63 行生产代码（7 个文件）、约 124 行测试（新增的跨 package 契约测试 + 方案： 范围合理——两个值契约各设唯一归属方，两个显示名 helper 改用不同名称区分（改名而非改行为，符合 issue 的 non-goals）； 风险： 无升级风险信号——变更文件均未命中 revert 历史分析中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRead the full diff independently before comparing — my own plan for #9151 would have been exactly this ownership split (bridge owns the tool-name list, core owns the limit, rename the CLI helper rather than change its behavior), with one difference: I'd have kept the limit in What I verified by reading the base tree and the diff:
Two non-blocking observations, neither asking for changes in this PR:
TestingThis is an unattended CI run, so per policy I did not build or execute the PR's code — the evidence below is the PR's own CI on the reviewed commit, fetched via API, plus the static verification above. Snapshot at review time: three of the five PR-CI workflows are green ( This PR's central claim is structural (single ownership, unchanged values), not behavioural — it is pinned by reading the diff plus the contract test and the existing 100,000-char boundary tests that CI runs. There is no behavioural claim a sandboxed lane would settle, so no Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查先独立读了完整 diff 再做对比——我自己对 #9151 的方案与这个归属拆分完全一致（bridge 持有工具名列表、core 持有限额、重命名 CLI helper 而非改其行为），只有一处不同：我原本会把限额留在 我通过阅读 base 代码树和 diff 核实了：
两条不阻塞的观察，都不要求本 PR 改动：
测试这是无人值守 CI 运行，按策略不构建、不执行 PR 代码——以下证据是通过 API 获取的该 commit 自身 CI 结果，加上上述静态核实。 审查时点快照：五个 PR CI 工作流中三个已绿（ 本 PR 的核心主张是结构性的（唯一归属、取值不变），而非行为性的——由 diff 阅读、契约测试以及 CI 运行的既有 100,000 字符边界测试共同钉住。没有需要沙盒验证通道才能定论的行为主张，因此无需 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, tightly scoped contract consolidation that does exactly what #9151 asked; the only deductions are two non-blocking follow-ups, not doubts about this diff. Stepping back: this is what good hygiene looks like. My independent plan for #9151 before reading the diff was the same ownership split, and where the PR diverged from it — the dedicated constant leaf module behind a subpath instead of exporting from the tool file — it diverged for the better. Every consumer was checked against the base tree and resolves through retained re-exports; the wiring mirrors existing subpath conventions; and the enforcement is real, because the new contract test runs in The two observations from the review stay non-blocking: the scheduled-task route still keeps a third unstitched copy of the same 100,000-char ceiling (a natural next candidate for this treatment, but outside this issue's named scope), and the definition tripwire's line-start anchor is a known, acceptable limitation next to the import-source assertions. The one thing standing between this verdict and an approval is evidence that hasn't landed yet: the ubuntu unit suite — the job that actually executes the new contract test — was still running at review time. So approval is deferred until CI lands green on 中文说明置信度：4/5 —— 干净、范围收紧的契约整合，完全做到了 #9151 的要求；唯一的扣分来自两个不阻塞的后续项，而非对本 diff 的疑虑。 退一步看：这是代码卫生该有的样子。读 diff 之前我对 #9151 的独立方案就是同样的归属拆分；而 PR 与之不同之处——用专门的常量叶子模块 + 子路径，而不是从 tool 文件导出——反而更好。每个消费者都在 base 代码树上核对过，均能通过保留的 re-export 解析；接线镜像既有子路径惯例；约束也是真实的，因为新契约测试会在 审查中的两条观察维持不阻塞：scheduled-task 路由仍保留同一 100,000 字符限额的第三份未缝合副本（是下一步同样处理的自然候选，但超出本 issue 点名的范围）；定义绊线的行首锚点是其已知且可接受的局限，旁边还有 import 来源断言承担主要约束。 此结论与批准之间只差尚未落地的证据：ubuntu 单测套件——真正执行新契约测试的任务——在审查时仍在运行。因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Aug 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Collaborator
Author
|
Closeout summary for exact head
中文摘要已补 source alias，并修正声明与 import tripwire 的两个可绕过点；聚焦契约测试 12/12 通过，TypeScript trace 直接解析到 core 源文件。未引入仅改善报错文本的额外包装，当前等待 exact-head CI/自动评审。 |
Collaborator
Author
|
@qwen-code /review --timeout=360 |
Contributor
|
Qwen Code review request accepted. Review is queued in workflow run. |
doudouOUC
reviewed
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review of PR #9497 — refactor: centralize cross-package contracts
Verdict: COMMENT — no blockers found. The implementation is clean, correct, and well-structured.
Summary
This PR centralizes three cross-package contracts into single owners:
LIVE_TASK_TOOL_NAMES/LiveTaskToolName→acp-bridge/bridgeOptions.tsMAX_SUB_SESSION_PROMPT_CHARS→core/tools/sub-session-constants.tsgetExtensionDisplayName→ renamed togetSanitizedExtensionDisplayNamein CLI to distinguish from core locale-aware version
What I verified
- Contract test (12/12 passes) —
scripts/tests/cross-package-contracts.test.jscorrectly pins ownership of all three contracts and verifies import paths of all consumers. - Re-exports are correct —
live-task-tools.tsre-exports fromacp-bridge/bridgeOptions,bridgeOptions.tsre-exportsMAX_SUB_SESSION_PROMPT_CHARSfromcore/subSessionConstants. isLiveTaskToolNamenow uses the centralized list — no longer a hardcoded duplicate that could drift.getSanitizedExtensionDisplayNamerename — correctly distinguishes the prompt-sanitization function from core locale-awaregetExtensionDisplayName. The 10+ UI call sites in the CLI that import from@qwen-code/qwen-code-core(the locale-aware version) are unaffected.- Config files — tsconfig paths and vitest aliases follow the existing
transcriptRecords/goalWirepatterns. exportsincore/package.json— follows the same pattern as other subpath exports.
Existing blocker (re-checked)
The previous review flagged git grep exit status 1 in definitionFiles as a Suggestion. The author declined to address it, noting it is outside this PR's scope. After re-checking the code: the test fails closed either way, and converting the subprocess error to a cleaner assertion is a diagnostics-only improvement. I agree with the author's decision — this is not a correctness issue for this PR.
No new findings
The diff is clean. All changes are mechanically correct, the contract test covers the ownership assertions, and the naming change for getExtensionDisplayName resolves the ambiguity identified in the issue.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Collaborator
Author
CloseoutChanged: clarified that the delegated-prompt ceiling is the shared core and ACP contract, without claiming a permanent scheduled-task equality. Verified: cross-package contract suite 12/12 and diff checks. Intentionally not changed: the value was not pinned to 100,000 as a cross-feature product policy because that is not part of issue #9151 or this PR contract. Pending: exact-head GitHub CI and automatic review are running. 中文摘要已把注释收敛到 core 与 ACP 的真实共享契约，没有把 100,000 扩张成跨功能永久策略；契约测试 12/12 通过，等待 exact-head CI 与自动评审。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-1 value pin for MAX_SUB_SESSION_PROMPT_CHARS — already reported (comment 3816436220); author declined in the cd6e714 closeout
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
scripts/tests/cross-package-contracts.test.js:96 — [probe] import extractor still bypassable via block-comment ghost imports and //-strip trailing whitespacescripts/tests/cross-package-contracts.test.js:29 — [probe] owner gate misses renamed re-exports ( export { LOCAL as SYM } ) as invisible second owners
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
yiliang114
enabled auto-merge
August 20, 2026 03:50
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": none — all checks above completed within budget (~9 tool calls)..
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
scripts/tests/cross-package-contracts.test.js:96 — [probe] import extractor merges pinned-import statements when a trailing line comment is present, letting a wrong-source import pass (re-discovery of the round-3 deferral; anchored on code …
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：none — all checks above completed within budget (~9 tool calls).。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
ytahdn
approved these changes
Aug 20, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — internal refactor centralizing cross-package contracts; existing values and enforcement boundaries unchanged, focused contract test added, CI green.
Collaborator
|
Released in v0.21.15. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR establishes one authoritative owner for the live-task tool-name contract and one for the delegated prompt-length contract, then makes the CLI's prompt-safe extension-name sanitization distinct from locale-aware display-name resolution. The existing values and enforcement boundaries remain unchanged. A focused repository test now catches duplicate declarations and consumers importing these contracts from the wrong owner.
Why it's needed
These contracts were independently restated across packages, so a change on one side could silently drift from its downstream consumers. The extension helpers also shared a name despite providing different semantics, which made accidental substitution easy during maintenance.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — this is an internal refactor with no user-visible or TUI change. Local verification passed lint, full workspace type checking, the full workspace build, and 183 focused tests. A built-package smoke check also confirmed the prompt limit remains 100,000 and both shared contracts resolve to the same runtime values.
Tested on
Environment (optional)
Node.js 22.22.0, npm workspace build, no sandbox.
Risk & Scope
Linked Issues
Resolves #9151
Related to #9144, which moves adjacent CLI integration code.
中文说明
本 PR 做了什么
本 PR 为 live-task 工具名称契约和委派提示词长度契约分别建立唯一权威归属，并明确区分 CLI 中面向提示词安全的扩展名称清洗与基于 locale 的显示名称解析。现有取值和校验边界保持不变。新增的聚焦仓库测试会捕获重复声明，以及消费者从错误归属方导入契约的情况。
为什么需要
这些契约此前在多个 package 中独立重复声明，因此一侧修改后可能与下游消费者静默漂移。两个扩展辅助逻辑虽然语义不同却使用相同名称，也容易在维护时被误用。
Reviewer 测试计划
如何验证
证据（变更前后）
不适用——这是内部重构，没有用户可见或 TUI 变化。本地已通过 lint、全 workspace 类型检查、全 workspace 构建和 183 个聚焦测试。构建产物 smoke 检查也确认提示词限制仍为 100,000，并且两个共享契约在运行时解析为相同值。
测试平台
环境（可选）
Node.js 22.22.0、npm workspace 构建、无沙箱。
风险与范围
关联 Issue
Resolves #9151
关联 #9144，该 PR 会移动相邻的 CLI 集成代码。