Repository navigation
refactor(material-lifecycle): decide the apply outcome vocabulary in one owner - #7398
Jiaqiiii523 wants to merge 2 commits into
Conversation
…one owner
`ranking.py` and `rebuild.py` each stated `{"applied", "no_change", "rejected",
"rolled_back"}` at module level and each guarded its own receipt builder with
its private copy, so one question -- is this token a legal apply outcome -- had
two owners. A fifth outcome added on either side would have stayed illegal on
the other, and no test could see the split because each copy only consulted
its own file.
The set now lives once in `_validation.py`, the module that already owns this
package's shared validation vocabulary and the two validators both call sites
invoke, and it is a `frozenset` so the owner cannot be mutated at runtime. Both
consumers import and decide with that object; the accepted tokens, the refusal
message and every companion-field rule are unchanged.
Signed-off-by: Jiaqiiii523 <293262669+Jiaqiiii523@users.noreply.github.com>
A census of the package reports every foldable set construction whose members are the apply outcomes, at module level *or* inline inside a function body -- module level alone let a consumer keep the owner import alive while deciding with its own literal, which is how the first version of this guard survived a mutation. Four probes pin the value-not-spelling rule (plain literal, reordered, `set(...)`, `frozenset(...)`), one pins that a widened copy is not a match, and the two consumers are checked for object identity, for naming the owner in the membership test, and for holding no private copy. Five behavior rows keep the convergence honest: each accepted status with the companion fields its own rule requires, an unknown status refused, and the legal-status table required to cover the owner set exactly. Signed-off-by: Jiaqiiii523 <293262669+Jiaqiiii523@users.noreply.github.com>
songoow
left a comment
songoow
left a comment
There was a problem hiding this comment.
Reviewer: model_agent; model=GPT-6 Astra Pro; provider=OpenAI; declaration_source=self_reported
详细中文评审
动机
本次评审针对 50ed43b9ac9688cb25a3679744159b40ebe4a265，审阅 diff 的实际 merge base 是 2953354fe9cc1df1e25c12bf81b1a9f74b4a2f5d。结论是 REQUEST_CHANGES：生产代码的常量收敛合理且未发现行为回归，但新增架构测试存在一个确定的 Windows 路径比较缺陷，需要修正。
ranking.py 和 rebuild.py 原本各自定义相同的四个 apply outcome。当前两份集合没有已知取值分歧；这里解决的是后续维护时一边新增状态、另一边仍拒绝它的漂移风险。这是边界清晰、可独立验证和回滚的维护改进，不需要用新增用户功能或全仓语义 inventory 指标下降来证明价值。
评审依据和本次适用边界如下：
| 依据 | 本 PR 的适用边界 | 审阅结果 |
|---|---|---|
| Semantic Vocabulary Convergence RFC：single owner / closed sets | 在现有 capability 内让同一个 apply outcome 集合有明确的定义位置 | 两个消费者均引用既有 _validation.py 中的同一对象；未声称改变全仓 inventory 预算 |
| Engineering scope / bounded refactoring | 复用最近的实际 owner，保持相邻行为和可回滚边界 | 未新增模块、通用框架或平行决策层，生产修改范围合适 |
| Good smokes：独立语义依据与负向验证 | 区分 source wiring、状态行为和扫描器覆盖 | 合法/非法状态验证有价值；扫描器的声明覆盖范围需要收窄或补足 |
| Source-checkout validation / supported platforms | 使用审阅版本的代码执行验证，并保留平台差异 | 聚焦测试通过；新增路径比较在 Windows 表示下确定失败，详见下文 |
改动思路
把 APPLY_STATUSES 放入已有的 material_lifecycle/_validation.py 是合适的归属：两个消费者已经依赖该模块的验证函数，这四个值也属于同一 capability 的 apply receipt 语义。frozenset 表达不可变词表，两个 membership check 共享同一对象，避免新增 helper 层或重复权威来源。
本 PR 复用已有 Python receipt builder，未增加新的跨域决策 owner；没有必要为这次有限重构附加无关的 TypeScript 迁移。两个 builder 的状态附带约束仍分别保留，也避免了仅因代码形状相似就强行合并不同 receipt 合同。
具体改动
已审阅全部四个变更文件及两处 receipt builder 的相邻校验逻辑：
_validation.py增加唯一的不可变APPLY_STATUSES定义。ranking.py::build_material_rerank_apply_receipt删除私有集合，改为引用共享集合。rebuild.py::build_material_ranked_entry_rebuild_apply_receipt作相同的引用调整。- 新增架构测试检查定义位置、对象引用、部分等价集合写法，以及 rerank receipt 的四种合法状态和一种未知状态。
生产词表仍为 applied / no_change / rejected / rolled_back。状态规范化、错误信息、revision/rollback/rejection 附带约束、receipt 字段和内容引用计算均保持原有逻辑。检索没有发现其他依赖已删除私有集合或修改其内容的消费者。
非阻塞建议 A：使扫描器的覆盖声明与实现一致。
测试文件开头 声称会识别相同成员的 comprehension，并要求无法折叠的构造声明文件和计数。但真实的 ast.SetComp 带有 generators，当前分支 返回 None，随后 扫描循环 直接忽略该结果，也没有 unfoldable 声明校验层。
直接调用本版本 helper 验证：普通字面量被识别；set comprehension、包装后的 comprehension、generator，以及 helper 的 return 字面量均返回空扫描结果。现有生产消费者没有通过这些形式复制词表，因此这不是当前运行时缺陷。建议补足明确承诺的有限语法与对应 mutation case，或者把说明收窄到实际测试覆盖的语法，避免为这个小重构建立过度复杂的静态分析框架。source.count("not in APPLY_STATUSES") 也应被理解为写法检查，而不是完整的运行时语义证明。
非阻塞建议 B：修正 PR 描述中的 rebuild 测试覆盖说明。
描述称 rebuild 的 accept path 需要完整 plan fixture 且没有覆盖，但该 builder 接收的是 plan_ref 和标量计数；既有测试 已直接覆盖 applied 成功以及缺少 rollback 的拒绝。准确的缺口是：新增的完整状态表没有同时参数化覆盖 rebuild 的其余状态及未知状态。可以用小型 builder 专属参数表补齐，无需构建完整 plan；至少应先纠正证据说明。
对主干的风险
[P2] 新增 owner census 使用平台相关路径，导致 Windows 上的正确实现也被判为失败
位置：tests/architecture/test_material_lifecycle_apply_status_owner.py:108–111。
prefix = rel_prefix or str(root.relative_to(REPO_ROOT))
...
sites.append(f"{prefix}/{path.name}:{line}")OWNER_MODULE 使用固定的 POSIX 路径 loopx/capabilities/material_lifecycle/_validation.py。Windows 上的 str(Path.relative_to(...)) 使用反斜杠，因此实际发现结果是：
loopx\capabilities\material_lifecycle/_validation.py:48
于是 test_only_the_owner_states_the_apply_vocabulary 在第 118 行比较失败，即使词表确实只有一个正确 owner。运行此测试的 Windows 贡献者会因路径拼写而失败，无法把结果用于判断真正的语义漂移。
已导入本次 head 的实际测试模块验证：原生 POSIX 路径下通过；保留真实文件扫描和内容，仅使用等价 PureWindowsPath 相对路径表示后，原断言失败，输出上述混合分隔符路径。没有执行原生 Windows 测试，也没有观察或声称 Windows CI 已失败。 当前 Windows workflow 仅选取部分测试，并未显式运行本新增架构文件。
建议最小修复为 root.relative_to(REPO_ROOT).as_posix()，并补充路径表示回归验证。更完整的路径构造可以保留每个文件相对扫描根的完整路径，避免未来嵌套目录中的中间路径被 path.name 丢弃。
除该测试可移植性问题外，未发现生产行为回归。此次没有改变 UI、provider/store、权限、quota、scheduler、发布权限、默认开关或持久化格式，也没有引入需要额外迁移的兼容层。
我的整体评价
建议先修复上面的 P2 路径问题，再满足其余项目合入门槛。 常量收敛本身值得保留；两项非阻塞建议用于提高测试声明与证据的准确性，不应被扩大为当前生产缺陷。
本次独立执行的证据如下：
| 验证 | 结果与边界 |
|---|---|
python -m pytest -q tests/architecture/test_material_lifecycle_apply_status_owner.py tests/capabilities -k material --no-cov --tb=short |
198 passed, 2872 deselected；包含新增 14 个 owner 测试和该筛选范围内的 184 个既有测试 |
| Ruff：Material Lifecycle 包及新增架构测试 | 通过 |
| diff whitespace 检查 | 通过 |
| 独立 base/head receipt 对比 | 392 个输入组合结果相同：ranking 112 个、rebuild 280 个；同时比较返回内容与异常类型/消息 |
| 实际 owner 测试 + POSIX 路径 | 通过 |
| 实际 owner 测试 + Windows 相对路径表示 | 确定复现上述断言失败 |
| 原生 Windows、全仓 architecture/canary、完整 CI | 本轮未运行或未独立验证，不计为通过 |
392 个对比输入涵盖合法、未知、带空白和空状态；相同/不同 revision；存在/缺失 rollback 与 rejection reason；空/非空 material refs；合法/非法 rebuild counters。对比加载 merge base 的两个 builder 源码与本次 head，所用共享验证行为未被此 PR 改动。这是 receipt 合同的聚焦等价证据，不代表完整仓库的端到端资格。
评审流程限制： 本轮环境无法通过 GitHub CLI 完成仓库 pr-review 所需的元数据读取，因此未生成该流程的 review packet，也未执行成功其 packet/result 一致性校验。本评审没有把这些步骤标记为通过；上述结论来自指定 head 的源码审阅、实际测试和可重复的失败证据。这是评审环境限制，不是本 PR 的缺陷，也不替代合并就绪检查。
English verdict
English verdict: REQUEST_CHANGES — reviewed head 50ed43b9ac9688cb25a3679744159b40ebe4a265 against merge base 2953354fe9cc1df1e25c12bf81b1a9f74b4a2f5d. The production extraction is appropriately scoped and no runtime regression was found. Please fix the new owner census's platform-dependent relative-path rendering: on Windows it compares a mixed-separator discovered path against a POSIX OWNER_MODULE and fails for a correct owner. The actual test reproduced this with a Windows path projection; native Windows was not run and no Windows CI failure is claimed. The focused suite passed 198 tests, scoped Ruff and diff checks passed, and 392 base/head receipt comparisons matched. Scanner coverage claims and the description of rebuild acceptance coverage also warrant nonblocking corrections. The repository review packet/result checks could not be completed because the review environment lacked working GitHub CLI metadata access; they are not claimed as passed and this limitation is not a PR defect.
| source = path.read_text(encoding="utf-8") | ||
| except (OSError, UnicodeDecodeError): | ||
| continue | ||
| prefix = rel_prefix or str(root.relative_to(REPO_ROOT)) |
There was a problem hiding this comment.
[P2] 用 POSIX 相对路径比较 owner，避免 Windows 下误报
OWNER_MODULE 固定为 loopx/capabilities/material_lifecycle/_validation.py，而 Windows 上的 str(root.relative_to(REPO_ROOT)) 使用反斜杠。这里会生成 loopx\capabilities\material_lifecycle/_validation.py:48，导致第 118 行的 owner 比较失败，即使只有一个正确 owner。
已导入实际新增测试，保留真实文件扫描，仅用 PureWindowsPath 表达相对路径后复现这一差异；没有运行原生 Windows，也不声称当前 Windows CI 已失败。请至少改为 .as_posix()；保留每个文件的完整相对路径还可避免未来嵌套目录被 path.name 丢失。
| if isinstance(inner, ast.SetComp): | ||
| return None if inner.generators else frozenset( | ||
| _folded_members(ast.Set(elts=inner.elt and [inner.elt])) or () | ||
| ) |
There was a problem hiding this comment.
非阻塞建议：让 comprehension 覆盖声明与实际扫描能力一致
文件头声明相同成员的 comprehension 会被识别，无法折叠的构造必须声明文件与数量。但合法 SetComp 都有 generators，这个分支会返回 None；普通 SetComp 也不在外层支持类型中，扫描循环随后直接忽略 None，没有 unfoldable 声明检查。实际调用 helper，普通字面量能识别，集合推导式及包装后的推导式均返回空列表。
当前生产消费者已正确引用 owner，所以这不是当前运行时回归。可以补上已承诺的有限语法及 mutation case，或收窄文件头声明；不必为本次常量抽取建立大型分析框架。
Goal And Delivered Outcome
Outcome basis / optional anchor: self-contained maintenance outcome under CONTRIBUTING's find-work rule (no pre-existing issue needed). It continues the semantic-vocabulary convergence program of [Task]: Semantic vocabulary convergence follow-ups after RFC PR #4433 / 语义词表收敛后续任务 #4447, which owns "one module, one decision" for closed sets; no new roadmap id is invented for it.
Goal/source and gap:
loopx/capabilities/material_lifecycleasked one question -- is this token a legal apply outcome -- and answered it twice:_APPLY_STATUSESatranking.py:39, consumed bybuild_material_rerank_apply_receipt, and the identical set atrebuild.py:36, consumed bybuild_material_ranked_entry_rebuild_apply_receipt. Both validate an owner-supplied status before a receipt is written, so a fifth outcome added on one side stayed illegal on the other, and nothing in the suite could see the split because each copy only ever looked at its own file.Observable before → after, with the validation row that proves it: the vocabulary now has one owner (
_validation.APPLY_STATUSES, afrozenset) and both consumers decide through that object; a value-based census fails if any module in the package states the same member set again, at module level or inline inside a function. Proven bytests/architecture/test_material_lifecycle_apply_status_owner.py(14 rows) and by the five mutation arms below, each of which is killed.Issue/task and intended base: no issue. Base
mainat2953354fe.Author Declaration
Implemented against
Specification and revision:
docs/architecture/rfcs/semantic-vocabulary-convergence-v0.mdas enforced onmainat2953354fe(closed sets need one owner; decisions are taken on the folded value, not the spelling), plus the package's own nearest existing ownerloopx/capabilities/material_lifecycle/_validation.py.Criteria:
APPLY_STATUSESinloopx/capabilities/material_lifecycle/_validation.py::test_only_the_owner_states_the_apply_vocabularyset_literalscensus::test_a_rewritten_copy_is_still_recognised(plain literal, reordered,set(...),frozenset(...))not in APPLY_STATUSESat both call sites::test_each_consumer_decides_with_the_owner_object::test_the_legal_shapes_cover_the_whole_vocabulary,::test_one_member_off_is_not_the_vocabularybuild_material_rerank_apply_receipt::test_every_owner_status_is_a_legal_apply_outcome(four rows, each with the companion fields its own status requires)::test_a_status_outside_the_vocabulary_is_refusedUNSAFE_FIELDSand thecompact_token/check_record_keysvalidators that both call sites use), the existing*_owner.pyguards for the census style, and every_APPLY_STATUSESreference in the tree. Verified: the new guard, 184 existing material-lifecycle rows,tests/architecture+tests/canaryin two trees at the same revision, the CI lint scope, five mutation arms, and the generated semantic inventory across both revisions. Deliberately left out: no new module (the owner is the package's existing validation module), and no rename of the two receipt builders. What I assumed rather than measured: that the two copies were never meant to differ -- nothing in either file, its comments, or the suite stated a divergent policy, and both reject with the same message.Scope And Continuation
material_lifecyclewas touched.scripts/generate_semantic_inventory.pyprints an identical summary on both revisions, so this fork was not one of the countedsame_runtime_fork_definitionsentries. The value here is the guard and the drift risk, not a budget number -- no inventory artifact or budget anchor is touched by this PR.Validation
unittests/architecture/test_material_lifecycle_apply_status_owner.py— 14 passed: census, four rewritten-copy probes, one widened-set non-match probe, two consumer-wiring rows, four legal-outcome rows, one refusal row, one coverage rowintegrationtests/capabilities -k material— 184 passed, unchanged from base, including the receipt builders that the two call sites belong toregression_paritytests/architecture+tests/canaryin two checkouts of the same base revision: BASE 4 failed / 1692 passed, HEAD 4 failed / 1706 passed, failure sets identical (the +14 is this PR's guard file), net regression 0. The 4 shared failures are main-side and named here so nobody has to re-derive them:test_content_digest_single_owner.py::test_only_the_owner_module_states_a_whole_value_digest_shape,test_lark_identity_shape_owner.py::test_no_lark_module_hides_an_unfoldable_identifier_construction(that one is #7387, repaired in #7395, not here),test_source_session_registry_denial.py::test_direct_project_registry_loaders_have_source_session_denial,test_top_level_module_budget.py::test_top_level_module_count_stays_at_the_pinned_budgetstaticruff check loopx/capabilities/material_lifecycle tests/architecture/test_material_lifecycle_apply_status_owner.py→ All checks passed (pinned range, 0.15.22).mypy's declaredfileslist (19 entries) contains none of the three edited modules; no new module, so the top-level budget is untouchedmutationreal_backendintegration/windowsbuild_material_ranked_entry_rebuild_apply_receipt's own accept path (it needs a full rebuild plan fixture; the guard covers it through the wiring and census rows rather than a behavior row), which is the same coverage it had before this change.Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
Shared-authority RFC fixture impact
Boundary Checklist
.loopx/,.codex/goals/, and liveACTIVE_GOAL_STATE.md).