chore(desktop): price only regressive dependency edges in the renderer ratchet - #4581
chore(desktop): price only regressive dependency edges in the renderer ratchet#4581orangeCatDeveloper wants to merge 2 commits into
Conversation
49c60ce to
c02303d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The two pricing decisions you identified are the right ones, and the mechanism you chose keeps the property that matters: type edges stay in dependencies, so every hard layering rule (validateDependencies, legacyFeatureImports, legacyPlatformImports, closure admission) still sees them, and only dependencyPaths stops counting. I checked the analyzer directly across import type, inline { type A }, mixed default plus type, bare side-effect import, export type … from, export … from, and TSImportType: closure reachability is preserved in all seven, and the only cases that drop debt are the ones that really are erased. The ledger regeneration matches the prediction exactly, which is good evidence there is no hidden loosening: 412 dependencyPaths entries removed, zero added, zero increased, and legacyRendererFiles, legacyGrowthDirectories, legacyFeatureImports, legacyPlatformImports and ownership are byte-identical.
Two things I would like resolved before this goes in, both about allowsMigrationDependency rather than about the pricing idea itself.
The change made part of allowsMigrationDependency dead, and it is still there. isSanctionedDependencyTarget is isValidatedCopyCatalog(…) || <the exact body of the legacyAppShell branch>, evaluated on the same desktopRoot, path and dependency. The per-key loop only iterates currentView.dependencyPaths, which has already had every sanctioned target filtered out, so by the time a dependency reaches allowsMigrationDependency that disjunction is guaranteed false. I confirmed it empirically rather than by reading: replacing the two return true paths in that branch with a hard throw leaves 75/75 checker tests passing and the real-tree check still passing against the merge base. Those lines can go.
Applying the sanction uniformly to all four sections quietly changes the rootDebt policy. allowsMigrationDependency deliberately discriminated per section: rootDebt allowed only bootstrap and composition, and only the closure sections allowed platform. The new filter runs before all of them, so shell, public application paths and public feature indexes are now free for rootDebt too, and platform is the only per-section rule that still does anything. That may well be what you want, but the PR body describes this as excluding sanctioned edges the way catalogs already were, and it is more than that. If it is intended, worth saying so, and worth asking whether allowsMigrationDependency should collapse into the single predicate instead of surviving as a second, now partly contradictory, answer to the same question.
Smaller notes, none blocking:
- The sanction for
applicationusesisPublicApplicationPath, which admits any top-levelapplication/<module>, not justcontracts/. Features are held to contracts byvalidateDependencies, so legacy code now has a broader free path into application than feature code has. Reusing the existing predicate is defensible, I just want to know it was a choice. - The
shelland public-feature-index arms ofisSanctionedDependencyTargetare not covered by the new fixtures, andsrc/renderer/shell/does not exist in the tree yet, so nothing exercises them today. There is also no negative fixture pinning that a non-public application or feature target still counts as debt, which is the property that keeps the filter from silently widening later. One fixture each would lock both directions down. - For
rootDebtfiles theimportDeclarationsandimportSpecifiersmetrics still price type-only imports:import type { A } from './x'records 1 and 1. So the specifier-count pressure you describe in the motivation is relieved for the re-export case but persists for root-debt files. Not something this PR has to fix, but the body reads as though it is fully gone. - Stray double blank line at 2439-2440.
Mechanically the branch needs a rebase: renderer-architecture.json conflicts, and --base origin/main fails with 23 violations across src/renderer/settings/, all of them from #4440 and #4571 landing after your merge base rather than from anything here. Against the merge base the tree is clean.
Evidence boundary: I ran the checker unit suite (75/75), check-renderer-architecture.mjs with no base, with --base origin/main, and with --base at the merge base; Biome check on both changed files; a direct analyzer probe over the seven import forms above; a structural diff of the regenerated ledger; and the dead-branch ablation. I did not run the desktop typecheck (scripts/ is outside every tsconfig include, so it cannot cover these .mjs files), any Playwright or Storybook run, or the full repository suite, and I did not verify how the bundler emits import { type A } from './x' at runtime, which does not affect the layering rules but does affect whether that form is truly zero-cost. This review was AI-assisted; the findings above were each reproduced locally before being written down.
简体中文
这两处计价确实该改,实现方式也挑得对:类型边仍然留在 dependencies 里,所以 validateDependencies、legacyFeatureImports、legacyPlatformImports 和闭包准入这些硬规则照样看得见它们,停止计数的只有 dependencyPaths。我直接拿分析器跑了七种写法(import type、内联 { type A }、默认加类型混合、裸副作用导入、export type … from、export … from、TSImportType),闭包可达性全部保留,掉计价的确实只有真正会被擦除的那些。账本重新生成的结果和预期完全吻合,这是没有暗中放松的有力证据:dependencyPaths 减少 412 条,新增 0,增加 0,其余几个列表和 ownership 一字未动。
合并前想先弄清两件事,都是关于 allowsMigrationDependency,不是关于计价思路本身。
第一,这次改动把 allowsMigrationDependency 的一部分变成了死代码,但没删。isSanctionedDependencyTarget 就等于 isValidatedCopyCatalog(…) 或上 legacyAppShell 分支的原样条件,输入完全相同。逐键循环只遍历过滤后的 currentView.dependencyPaths,能走到 allowsMigrationDependency 的依赖必然已经不是 sanctioned,所以那个分支永远返回不了 true。我不是靠读代码下的结论:把那两处 return true 换成直接抛异常,75 个测试仍然全过,对着 merge base 的真实检查也照样通过。这几行可以删掉。
第二,把过滤统一套到四个 section 上,其实悄悄改了 rootDebt 的策略。原本 allowsMigrationDependency 是分 section 区别对待的:rootDebt 只放行 bootstrap 和 composition,platform 只对闭包 section 有效。现在过滤跑在它前面,shell、公开 application 路径和 feature 公开入口对 rootDebt 也免费了,platform 成了唯一还起作用的分 section 规则。这也许正是你要的,但 PR 正文的说法是「像 catalog 那样把 sanctioned 边排除掉」,实际做的比这个多。如果是有意为之,写清楚比较好,也值得顺带考虑要不要干脆把 allowsMigrationDependency 并进这个单一判定,而不是留着第二套、而且已经开始互相矛盾的答案。
几个不阻塞的小点:
- application 那一侧用的是
isPublicApplicationPath,任何顶层application/<module>都算,不限于contracts/。而 feature 被validateDependencies卡死只能走 contracts。也就是说 legacy 代码进 application 的免费通道比 feature 还宽。沿用已有判定说得通,我只是想确认这是想清楚之后的选择。 isSanctionedDependencyTarget里shell和 feature 公开入口这两条新 fixture 没覆盖,而且树里目前根本没有src/renderer/shell/,所以它们今天没有任何东西在跑。另外也缺一条反向 fixture,钉住「非公开的 application 或 feature 目标仍然计价」,这条恰恰是防止过滤日后被悄悄放宽的性质。两边各加一条就锁住了。- 对
rootDebt文件,importDeclarations和importSpecifiers仍然给类型导入计价:import type { A } from './x'记成 1 和 1。所以你在动机里说的 specifier 计数压力,re-export 那种情况解决了,root-debt 文件那边还在。这个 PR 不必顺手修,但正文读起来像是已经彻底没了。 - 2439 到 2440 多了一个空行。
流程上分支需要 rebase:renderer-architecture.json 有冲突,--base origin/main 会报 23 条 src/renderer/settings/ 的违规,全部来自 #4440 和 #4571 在你的 merge base 之后落地,跟这次改动无关。对着 merge base 检查是干净的。
验证边界:我跑了 checker 单测(75/75)、无 base 的检查、--base origin/main、--base 指向 merge base、两个改动文件的 Biome 检查、上面七种导入写法的分析器探针、重新生成账本的结构化 diff,以及死分支的消融实验。没跑 desktop 的 typecheck(scripts/ 不在任何 tsconfig 的 include 里,覆盖不到这些 .mjs)、没跑 Playwright 和 Storybook、没跑全仓测试,也没有验证打包器在运行时如何处理 import { type A } from './x',这一点不影响分层规则,但影响这种写法是不是真的零成本。本次评审有 AI 参与,上面每条结论都在本地复现过。
e62e009 to
44e7a90
Compare
|
Thanks — both blocking points were real, and the second one was my mistake rather than a choice. Addressed in 44e7a90:
Not changed: |
Two miscalibrations made the ratchet punish work it should welcome. Type-only imports are erased at compile time yet were priced like runtime edges, so compliance produced token accounting (typeof extraction, statement merging) instead of structure; they now keep closure reachability but record no dependency or import-count debt, and turning one into a runtime import starts counting immediately. Edges into sanctioned targets — validated catalogs, shell, public application paths, feature public APIs — are the direction the migration exists to encourage, yet the net-zero rule blocked adding them outright; they are now excluded from dependency accounting. Root entries are the exception: main.tsx and app.tsx are meant to become thin mounts, so only catalogs are free for them. Runtime edges into legacy code and bare packages are priced exactly as before. Generated-by: Claude Code
44e7a90 to
090c186
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I ran both versions of the checker rather than reading the diff, because the 831-line ledger change is the kind of thing that has to be proved mechanical. It is: the main analyzer against the base tree reproduces main's committed ledger byte for byte, and the PR analyzer against this head reproduces this ledger byte for byte. Going through the delta entry by entry, every change is a removed dependency edge or a decreased import count. Nothing was hand-waived, no existing debt was forgiven, and nonTriviaTokens is untouched, so the size cap still holds on the two sections that have one.
The false positive is real too. On main, a closure file that adds an import of features/alpha/index is rejected outright unless it drops another edge, which is the ratchet blocking exactly the direction the migration wants. On this branch it passes.
I also confirmed the relaxation is only about pricing: dependencies still receives type-only edges, so closure computation and every hard boundary rule are unaffected, and a type-only import of a feature internal still fails with feature imports must use index.
[P2] Legacy type-only edges into application internals are now unpriced, and no hard rule covers them
validateDependencies holds composition (:1564) and platform (:1586) to isPublicApplicationPath, and holds every non-feature source to the feature public entry (:1493), which fires on type-only edges too. Legacy sources have no application rule at all, so pricing was the only thing standing there, and the new type-only guard removes it.
Executed, same fixture, edges rewritten as import type targeting application/sessions/impl.ts:
main => ["...: new dependency debt ./application/sessions/impl.js", ...]
PR => []
So within the checker's own model, legacy coupling to a feature internal is a hard violation while legacy coupling to an application internal is free, if it is type-only. Smallest fix is to give legacy sources the rule composition and platform already have, which is a structural rule independent of pricing.
[P3] Two smaller ones
The two closure sections lose their only cap on dependency growth. They ratchet only capability metrics, no importDeclarations and no token count, so dependencyPaths was the sole structural limit on those 234 files, and removing sanctioned edges from both views means metricTotal can no longer see them either. The body names this and accepts it, and the direction is right, but it is a genuine loosening rather than only a false-positive fix, so it is worth stating that way in the description.
inspectCopyCatalog still carries two hand-inlined copies of the type-only predicate this PR just gave a name to. They are equivalent today; folding them into typeOnlySourceDependency also turns the script hunk into a net deletion.
Next step
The application-boundary asymmetry is the one I would settle before merge, either by adding the rule or by saying plainly that legacy type-only coupling to application internals is deliberately unpriced. The other two are non-blocking.
Blast radius today is small, by the way: 31 recorded edges go invisible in legacyAppShell.files and 103 in the closure, and shell/ does not exist yet while application/ has three files, all public. The exemption is almost entirely forward-looking.
Evidence boundary: the ledger equivalence and all four relaxation experiments were executed by me against a scratch extraction of both trees, read only, with the root checkout untouched. I did not run the PR's own test file and relied on the green test job for it. Branch protection contexts were read from .asf.yaml at this head, not from the live settings API. The duplicate-predicate finding is read, not executed.
AI-assisted review: drafted with Maka.
Generated-by: OpenCode
|
@Astro-Han All issues are fixed and CI is green |
Summary
Follow-up calibration to #4493. The ratchet's purpose is to stop the legacy renderer from getting worse; two of its pricing decisions also punished getting better. (1) Type-only imports are erased at compile time and cannot carry behavior, yet were priced like runtime edges — compliance produced token accounting (
Parameters<typeof …>extraction, statement merging, re-export forms chosen for the counter) instead of structure. Type edges now keep closure reachability (one rewritten into a runtime import starts counting the moment it changes) but record nodependencyPaths,importDeclarations, orimportSpecifiersdebt; a mixedimport { a, type A }counts only its runtime specifier. (2) Edges into sanctioned targets — validated catalogs, shell, public application paths, feature public APIs — are exactly where the migration wants legacy files to point, yet net-zero blocked adding them outright; they are now excluded from dependency accounting for the AppShell family and both closures.This deliberately removes
dependencyPathsas the structural growth cap for sanctioned edges in the two closure sections; their capability metrics and closure-admission rules remain unchanged. Root entries are deliberately not widened:main.tsxandapp.tsxare meant to become thin mounts, so for them only validated catalogs are free and the same-count swap into bootstrap/composition stays their only migration path. With every target the AppShell family may move toward now sanctioned,allowsMigrationDependencycollapses to a per-section swap-zone table (rootDebt,rootDebtClosure); the AppShell sections have none. Runtime edges into legacy code and bare packages are priced exactly as before; every capability, token, and closure-admission ratchet is untouched.isPublicApplicationPath(any top-levelapplication/<module>, not onlycontracts/) is reused on purpose: it is the same boundaryvalidateDependenciesalready holds composition and platform to, and legacy code sits at that tier, not at the feature tier. Legacy imports of application internals are now hard violations independent of whether the edge exists at runtime, so type-only edges cannot bypass that boundary.Refs #2672
Verification
AI use
Select exactly one:
Tool(s) and scope: Claude Code — rule design (with cross-model review), implementation, fixtures, and the original description; OpenCode — reviewer-feedback implementation, regression fixture, and description update, under the contributor's direction. The affected commits carry the corresponding
Generated-bytrailers.Checklist