docs: classify architecture invariants by enforcement mechanism (#9152) - #9689
Conversation
Record the policy decision asked by #9152: which architectural invariants are enforced mechanically, which are left to review, and which are not worth enforcing. Covers every invariant asserted in AGENTS.md and every open architecture issue (#8084, #9145, #9146, #9151, #4063). The drift-guard decision: do not extract a reusable framework from check-voice-guard-sync.js. The two new drift guards (cross-package-contracts.test.js and approval-mode-drift.test.ts) share a pattern but not enough structure to justify abstraction. The pattern is documented for copy-when-needed.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 The PR body doesn't follow the repository's pull request template — none of the required sections (What this PR does, Why it's needed, Reviewer Test Plan, Risk & Scope, Linked Issues) are present, and the bilingual Chinese <details> section is missing too. The substance is largely already written (Motivation, Changes, Drift-guard decision, Verification, Closes #9152), so this should be a reformat rather than new work: please restructure the body along .github/pull_request_template.md. For a docs-only change, Evidence (Before & After) can be N/A and the Tested-on table is a one-liner.
This is a formatting gate, not a judgement on the content — the review stops here and picks back up once the body follows the template. Re-running @qwen-code /triage after the update will continue it.
中文说明
@yiliang114 PR 正文没有使用仓库的 pull request 模板——必需章节(What this PR does、Why it's needed、Reviewer Test Plan、Risk & Scope、Linked Issues)均缺失,双语中文 <details> 部分也不存在。内容本身基本已经写好(Motivation、Changes、Drift-guard decision、Verification、Closes #9152),所以这应该只是一次重排,而不是补写新内容:请按 .github/pull_request_template.md 重组正文。纯文档改动的 Evidence (Before & After) 可以写 N/A,Tested-on 表格一行即可。
这是格式门槛,不是对内容的否定——审查在此停下,正文符合模板后继续。更新后 re-run @qwen-code /triage 即可恢复审查。
— Qwen Code · qwen3.8-max
|
📄 Docs-only change — the automatic review ran at
Reviewed head: 中文说明📄 纯文档变更 —— 自动评审以 |
|
@qwen-code /triage |
|
Sandboxed verification: This PR changes documentation/assets only — there is no code to execute, so a sandboxed verification has nothing to verify. 中文 — 判定:
|
|
Round-6 re-run — new commit
Moving on to verifying the fixes. 🔍 中文说明第六轮 re-run——自上轮以来新增提交
继续验证这两处修复。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRound 6 — new head The blocking #9145 clause is fixed, exactly as prescribed. The sentence no longer attributes any desktop drift to the issue. It now reads: "the issue identifies drift in the Python and Java SDKs, which PR #9003 (in progress) addresses. The remaining item worth naming is desktop's The line-count nit is fixed too. "each 40-60 lines" → "114 and 44 lines of table-driven test code respectively" — re-verified against The rest of the commit is Prettier normalization, as claimed. The full diff between the two versions contains only: table column alignment (classification scheme + summary tables), blank lines inserted before three bullet lists, and Fresh spot checks this pass (worktree at No new findings. The unchanged remainder stands on rounds 4–5's verification, which this round's diff confirms is untouched. Test evidenceDocs-only change; CI on
Real-scenario testing: N/A — documentation-only change, nothing user-visible to drive. 中文说明第六轮——head 自第五轮起更新为 阻断性的 #9145 从句已按处方修复。 该句不再把任何 desktop 漂移归于该 issue,现在写作"该 issue 指出了 Python 与 Java SDK 的漂移,PR #9003(进行中)正在处理。剩下值得点名的是 desktop 的 行数小问题也已修复。 "各 40-60 行" → "分别为 114 与 44 行"——对照 提交其余部分确为 Prettier 规范化。 两个版本的完整 diff 仅含:表格列对齐、三处列表前补空行、 本轮新做的抽查(worktree 在 无新发现。未变动部分以第四、五轮的核验为准,本轮 diff 确认其原封未动。 测试证据:纯文档改动, — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — the one blocking clause is fixed and verified against the primary source, the nit is fixed too, and nothing else moved. Stepping back: five rounds in, this document had earned every approval except one sentence — the #9145 attribution. @yiliang114 fixed exactly that sentence (and the line-count nit), and this pass I checked the fix against the issue itself rather than the closeout note: #9145's body identifies Python and Java drift and never touches desktop, which is precisely what the reworded paragraph now says. The version diff also confirms the commit carries nothing besides the two fixes and Prettier normalization. What I'm approving: a single 316-line design document that classifies the repo's invariants by enforcement mechanism, tracks the open gaps honestly (❌/ The two standing request-changes reviews are superseded by this approval. Approving now, pinned to the reviewed commit. ✅ 中文说明置信度:5/5 —— 唯一的阻断从句已修复并经一手来源核实,行数小问题也已修复,别无其他改动。 退一步看:五轮下来,这份文档除了那一句 #9145 归属之外已配得上任何批准。作者精确地修复了该句(连带行数小问题),本轮我直接对照 issue 本身而非其总结评论核验:#9145 正文指出的是 Python 与 Java 的漂移、完全不涉及 desktop——改写后的段落所说的正是这一点。版本 diff 同时确认该提交除两处修复与 Prettier 规范化外别无其他内容。 我批准的是这样一份 316 行的设计文档:按执行机制给仓库不变量分类,诚实跟踪开放缺口(❌/ 两条在案的 request-changes 审查由本次批准取代。现在批准,钉在所审提交上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two factual errors block this as a canonical record, @yiliang114 — full detail in my stage-2 comment above:
- The
lane4-rebasebranch does not exist in this repository (404). Theno-core-root-barrel-import.jsrule lives in open PRs #8139 (lane3-core-root-barrel) and #9635 (codex/pr-9152-root-barrel-cleanup) — point the #4063-item-5 section and open action 2 at the PR(s) instead. approval-mode-drift.test.tsdoes not "explicitly exclude" desktop'scyclablePermissionModes— it never mentions desktop; it simply asserts SDK ↔ core sequence equality. Reword to "does not cover desktop" (the conclusion "no further guard needed" can stay if that's the intended call).
While you're in there: #9152's own table lists scripts/check-desktop-isolation.js as one of the four existing mechanical guards, and the two daemon-route review invariants from AGENTS.md's Code Review section — none of the three are classified here despite the "every invariant asserted in AGENTS.md or an open architecture issue" claim.
Everything else I verified holds up (guard configs, issue/PR statuses, the drift-guard decision), CI is green on the reviewed commit, and the fixes are small — expecting this to land quickly after. 🙏
|
Closeout for the CHANGES_REQUESTED review — all findings addressed in 951b4b9. Every corrected claim was re-verified against Blocking
Coverage gap — added all three missing items in the document's existing style:
Minor nits
|
|
@qwen-code /triage |
|
Sandboxed verification: This PR changes documentation/assets only — there is no code to execute, so a sandboxed verification has nothing to verify. 中文 — 判定:
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Sandboxed verification: This PR changes documentation/assets only — there is no code to execute, so a sandboxed verification has nothing to verify. 中文 — 判定:
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
|
|
No blocking findings. The previous CHANGES_REQUESTED reviews (both on Tier: Scan — docs-only, one new markdown file, no source changes. CI: Classify PR / Dependency CVE audit / Secret scan / Desktop Shell / Test (ubuntu, 22s path-filter only) — all green. E2E Smoke / coverage / platform matrix SKIPPED correctly (no source change). Factual accuracy cross-check (inverted brief):
Not covered: ESLint config and tsconfig files not read in full — spot-checked via PR states and branch names only. Reviewed with AI assistance. |
Review summaryThe previous review findings are addressed in
|
|
Closeout for the round-5 findings: Changed (a5670f3):
The remainder of the commit's diff is Prettier markdown normalization applied by the repo's pre-commit hook (table alignment, Verified: both corrected claims against Pending: nothing — awaiting the automated re-check. |
|
@qwen-code /triage |
|
Sandboxed verification: This PR changes documentation/assets only — there is no code to execute, so a sandboxed verification has nothing to verify. 中文 — 判定:
|
|
@qwen-code-review-bot review it and approve if no blockers |
|
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
I can’t approve this yet. Two blockers remain in the canonical classification:
The docs-only scope is otherwise cohesive, and all required checks are green. |
|
Both blockers addressed in 82869c0:
Docs-only; both claims verified against |
|
@qwen-code-review-bot review it and approve if no blockers |
|
|
|
One blocker remains before approval: the new Web Shell section says
Since this document claims canonical coverage of every The Node |
|
Addressed in c6c3f59: the Web Shell section no longer claims a count. It now classifies the entire AGENTS.md Web Shell UI development section as review-only and explicitly covers every requirement you listed — the shadcn workflow (run from |
|
@qwen-code-review-bot review it and approve if no blockers |
|
|
|
LGTM — no blockers on the current head. The Web Shell conventions are now comprehensively classified without the incorrect count claim, the Node |
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
LGTM. The new commits address the two blockers from the earlier review:
- Web Shell UI conventions are now classified (shared primitives, forwardRef, semantic tokens, portal root)
- Node engines reclassified to reflect the partial guard gap
Docs-only, well-structured, CI green.
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Re-review of head c6c3f59 (prior review on 951b4b9 was dismissed after new commits).
Tier: Scan — docs-only, single markdown file, no source changes. CI all green.
New content vs. previous head — verified clean:
-
§15 Web Shell UI conventions (new section): classified Review-only with accurate rationale — no lint rule can detect a missing
forwardRef, an overwritten CSS scoping, or a raw portal container; enforcement is via reviewer judgment against AGENTS.md andpackages/web-shell/README.md. Summary table row added. ✅ -
Node ≥22 reclassified to "partial": Previous head said "Mechanically enforced" (no qualifier). New text correctly notes that
"engines"withoutengine-strictin.npmrconly produces an npm warning — gap explicitly documented with the fix path. ✅
Factual accuracy (carried forward from prior round, no regressions):
- PR #9144 OPEN, PR #8139 (
lane3-core-root-barrel) OPEN, PR #9635 OPEN, PR #9003 OPEN, PR #9497 MERGED — all still correct as of this review.
Reviewed with AI assistance.
|
Released in v0.22.0. |
What this PR does
Adds
docs/design/9152-architecture-invariant-classification.md— a canonical record classifying every architectural invariant in the repository by its enforcement mechanism: which invariants are enforced mechanically (custom ESLint rules, source-level boundary tests, drift-detection tests,tsconfig.jsonstrict mode,package.jsonengines field), which are left to review, and which are not worth enforcing. It covers every convention asserted in AGENTS.md (ESM, strict mode, Prettier, noany, kebab-case filenames, collocated tests, Conventional Commits, Node ≥22, core module two-tier gate) and every invariant from open architecture issues (#8084, #9145, #9146, #9151, #4063). It also records the drift-guard decision: do not extract a reusable framework fromcheck-voice-guard-sync.js— the two existing drift guards (cross-package-contracts.test.jsfrom #9151 andapproval-mode-drift.test.tsfrom #9145) share a pattern but not enough structure to justify a shared abstraction, so the copy-the-structure policy is documented instead, with an explicit trigger (a fourth case) for revisiting it.Why it's needed
Issue #9152 asks for a policy decision so that a new invariant encounters an existing answer instead of being handled case by case. The repository already enforces invariants in four places, each invented independently; this PR writes down the classification so the next invariant lands in the right category by default.
Reviewer Test Plan
How to verify
Static documentation only — no code, build, or test changes. Cross-reference each guard named in the document against the current
mainbranch state and open PR status; confirm the classification categories (mechanical vs review vs not worth enforcing) match the actual enforcement mechanism for each listed invariant.Evidence (Before & After)
N/A (documentation-only change)
Tested on
Environment (optional)
N/A — documentation only, verified against
mainbranch sources and open issue/PR state.Risk & Scope
Linked Issues
Closes #9152
Related (tracked, not closed): #8084, #9145, #9146, #9151, #4063
中文说明
这个 PR 做了什么
新增
docs/design/9152-architecture-invariant-classification.md——一份权威记录,把仓库里每条架构不变量按其执行机制分类:哪些由机械手段强制执行(自定义 ESLint 规则、源码级边界测试、漂移检测测试、tsconfig.jsonstrict 模式、package.jsonengines 字段),哪些交给 review 把关,哪些不值得强制。它覆盖 AGENTS.md 中断言的所有约定(ESM、strict 模式、Prettier、禁用any、kebab-case 文件名、测试就近放置、Conventional Commits、Node ≥22、core 模块两层门禁),以及所有开放架构 issue(#8084、#9145、#9146、#9151、#4063)中的不变量。同时记录了 drift-guard 决策:不从check-voice-guard-sync.js抽取可复用框架——现有两个 drift guard(#9151 的cross-package-contracts.test.js和 #9145 的approval-mode-drift.test.ts)有共同模式,但结构上不足以支撑共享抽象,因此改为记录「复制现有结构」的策略,并给出明确的重新评估触发条件(出现第四个案例)。为什么需要
Issue #9152 要求一个策略决策:让新出现的不变量直接对号入座,而不是逐案处理。仓库已经在四个地方各自独立地强制执行不变量;本 PR 把分类写下来,让下一条不变量默认落到正确的类别里。
Reviewer 测试计划
如何验证
纯静态文档——没有代码、构建或测试改动。将文档中列出的每个 guard 与当前
main分支状态及开放 PR 状态交叉核对;确认分类类别(机械强制 / review 把关 / 不值得强制)与每条不变量的实际执行机制一致。证据(Before & After)
N/A(纯文档改动)
测试环境
环境(可选)
N/A——纯文档,基于
main分支源码与 issue/PR 状态验证。风险与范围
关联 Issue
Closes #9152
相关(仅跟踪,不关闭):#8084、#9145、#9146、#9151、#4063