refactor(digest): one owner builds the stored SHA-256 envelope - #5337
sakurahello1 wants to merge 5 commits into
Conversation
Refs loopx-project#5336. The shape owner recognizes `sha256:<64 lowercase hex>` and exports nothing else: the leaf is generated, and the generator deliberately refuses syntax it cannot translate, so writing the envelope had no counterpart. 94 sites in loopx/ concatenated the prefix by hand, 10 of them in periodic_report, each restating the envelope that the same package's readers then checked against the owner's pattern. The new module borrows the owner's pattern object rather than restating a shape and refuses any value the owner would not recognize, so a producer cannot emit a digest its reader rejects. Migrated sites keep their own byte recipes -- two canonicalize with ensure_ascii=True and one hashes a string rather than canonical JSON -- because the envelope is the shared decision and the bytes are each surface's own question. No digest value changes. The three remaining periodic_report sites are census hosts whose import lines are pinned by row number in the project-registry I/O manifest; they are recorded as deferred rather than converted blind. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
A second architecture test covers what the existing single-owner guard names as its own unfinished half: hand-built envelopes. It judges folded values, so a prefix moved into a module constant is the same offender as a literal, and it records the three not-yet-converted files in an allowlist that fails if one of them converts itself, so the list cannot rot. The behavioural cases enter through each migrated helper and compare against the expression it replaced, which is the invariant that makes the conversion safe to land. The new module is pinned into the existing guard's consumer manifest. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
The semantic inventory groups named string constants across modules, and `ENVELOPE_PREFIX` was already bound in handoff_fragments.py to a different value. Naming it after what it envelopes makes the collision disappear: the drift smoke returns to its reviewed budgets instead of growing three of them (conflicting_values, conflicting_definitions and the semantic half of the first), which is the check this repository uses to stop a consolidation from quietly adding a second meaning to a name. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
…ound Written after running the mutations, each of which found a real gap: * folding only module-level assignments let a converted file move the prefix into a function-local name and keep building the envelope unnoticed; the folder now walks every scope and the bypass corpus carries that form; * `re.compile` caches by pattern and flags, so a restated copy of the owner's pattern compares *identical* to the borrowed object -- the identity assertion proves nothing on its own, so the builder is now also required to compile no pattern of its own; * the event-digest recipe used ASCII-only input, where `ensure_ascii` is not observable and flipping it passed. A non-ASCII id is the only input that pins the byte recipe this PR deliberately left per-module. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
cocolord
left a comment
There was a problem hiding this comment.
评审提交:cc93a8932e123211e9f5027e80f970eb57210e3c。这是 policy-11 whole-PR、exact-head 评审。我检查了 12 个变更文件、#5336 的自述目标、所有 periodic-report 迁移点、新 builder、两层 architecture tests、远端 checks,并在真实测试 helper 上做了反例验证。生产替换保持 digest 值等价,但新增 guard 不能可靠证明它声称的 single-owner 约束,而且这一“首个 surface”仍留下同包内 3 个手写 producer;当前成本与收益不匹配。
动机
把 sha256:<hex> 的构造集中到一个 helper,方向上能减少 writer/reader 规则漂移;periodic-report 的多个私有 digest helper 确实重复了相同 envelope 拼接。若一个完整 capability 都迁到稳定 owner,后续修改格式、验证非法输入和审查调用方会更集中。
但这是无可观察行为变化的维护性 refactor,收益门槛应高于“发现很多相同字符串”。当前 PR 用 455 行新增、28 行删除替换 10 个一行构造,其中 381 行是新的 bespoke AST guard;同时同一 periodic-report package 仍保留 3 个手写 producer,仓库其他 97 个 Python/TypeScript site 也不受此 guard 约束。作者需要把可持续收益做成真实、完整且低维护成本的边界,而不是把第一批机械替换本身当成收益。
改动思路
digest_envelope.py 新增 enveloped_sha256 与 sha256_envelope:前者拼接 prefix 后复用既有 ENVELOPED_SHA256_PATTERN 做 fullmatch,后者计算 SHA-256 再调用前者。9 个 periodic-report 模块的 10 个构造点改为调用该 helper,各模块原有 JSON canonicalization、字符串编码与截断语义保持在原 owner。
新增 test_content_digest_production_owner.py 一方面逐个调用迁移后的 helper 与旧表达式比较,另一方面扫描所谓“converted surface”,阻止再次手写 prefix;3 个未迁移文件通过 CONVERTED_SURFACES allowlist 排除。既有 test_content_digest_single_owner.py 只新增新 builder 的 consumer 声明。正向值等价路径成立,负向 guard 路径却有结构性缺口。
具体改动
阻塞问题
-
[P1] 新 production scan 的 name folding 不遵守 Python lexical scope,既误报也漏报。
_string_constants用一次全树ast.walk把所有作用域的同名变量压进一个 flat dict,再由_denotes_prefix在任意位置查询。reviewer 直接调用 exact-head 的_hand_built_envelopes复现:一个函数里未使用的 localprefix = "sha256:"会让另一个函数的参数prefix + value被误报;而 annotated constant、str.format和str.join三种手写 envelope 都返回空命中。这个 scanner 因此既会阻塞无关后续代码,又可以被常见写法绕过,不能支撑“converted surface 只能经 owner 构造”的核心收益。更关键的是,同仓库现有 single-owner guard 已有 scope-aware_Scope/_collect_scopes/_fold_text,并专门测试 local binding 不泄漏到其他 scope;新文件复制了一套更弱实现。请抽取/复用现有 folding owner,至少覆盖 AnnAssign 与明确支持的构造,并把无法判定的 form 报为 unknown,而不是当成无违规。 -
[P1] 请完成一个真实 periodic-report 边界,或把这 381 行 guard 的独立收益讲清并显著收窄。
CONVERTED_SURFACES名义上登记整个loopx/capabilities/periodic_report,却主动跳过pending_intent.py、post_writeback_hook.py、request_action.py的手写 producer;理由只是新增 import 会移动已生成 manifest 的行号。生成并审核 manifest 本来就是这次同域机械迁移的一部分,不构成独立架构边界。结果是一个 capability 内同时保留新旧 producer,新增 381 行测试和 deferred protocol,却没有交付“一个 surface 只有一个 builder”的完整收益。请优先迁移这 3 个同包 site 并更新 manifest,然后把 guard 缩到真正稳定的 contract;若仍要保留分阶段例外,请明确给出受影响用户/维护者、before/after 可观察收益、预计消除的实际失败或维护成本,以及为什么这些收益足以抵消新增 AST framework 的长期成本。
关键代码讲解
enveloped_sha256复用 reader 的ENVELOPED_SHA256_PATTERN验证构造结果,sha256_envelope只负责 bytes -> lowercase digest -> envelope;这两个函数本身边界清晰。- periodic-report 的
_digest/_canonical_digest/_content_digest保留各自 canonical bytes,只替换最后的 prefix 拼接。focused parity 覆盖了 non-ASCIIensure_ascii=True分支,未发现值变化。 _string_constants、_denotes_prefix、_hand_built_envelopes是新 guard 的实际 decision owner;由于 flat name table 和有限 AST forms,其通过不等于 single-owner invariant 成立。CONVERTED_SURFACES同时充当扫描范围和临时例外清单,但三个例外与迁移点处在同一 package、同一 change reason,没有真实部署或兼容边界。- existing
test_content_digest_single_owner.py已实现 scope-aware binding/folding;新增 381 行测试没有复用这个更强 owner,是重复知识而不是必要隔离。
对主干的风险
生产路径的直接行为风险较低:两组 focused tests 分别通过 242 和 268 个用例,迁移 helper 的输出与旧表达式相同,非法 bare digest 也被拒绝。远端仅 test-shard 1/2、aggregate pytest 与 merge-gate 为红;我在 immutable base 和 exact head 上重跑三个具体失败,双方都是相同的 turn-contract count 与 prompt-upgrade-hook 断言,因此这些 CI 红灯是既有基线问题,不归因于本 PR,也不是本次拒绝理由。
真正风险在长期维护:这个 PR 的主要净新增是一个不健全的静态 analyzer。它会制造 false-positive CI 阻塞,也会让常见 bypass 在绿色测试下继续手写;同时临时 allowlist 把同一 capability 的剩余迁移固化为永久维护面。失败后没有 runtime 回退,维护者只能调 scanner、扩大例外或继续追加 bypass case,正是本次 refactor 想消除的重复成本。
语义与 CI 对齐
digest 的 runtime 值语义在已迁移调用点保持一致,没有默认用户行为变化,也没有新权限、状态机或 guidance/obligation 混淆。当前不对齐的是 architecture claim:测试名和 issue 使用“one owner / converted surface”,但 scanner 无法证明这一点,且该 surface 仍显式包含三条 legacy producer。应先让 contract 名称、扫描能力和实际迁移范围一致,再把它作为长期 required guard。
我的整体评价
结论是 REQUEST_CHANGES。小型 builder 与机械替换本身可以接受,且作者对 byte recipe 差异的测试是认真且正向的;但 whole PR 当前为 10 个一行替换引入 381 行重复且不健全的 AST enforcement,并用 manifest 行号作为同包迁移的暂停理由。对长期工程质量,这是净复杂度上升而非已经证明的收敛。
建议把 scope-aware folding 从现有 single-owner guard 抽成一个复用 helper,完成 periodic-report 同包 3 个剩余 producer 和 manifest 更新,再保留薄的行为 parity/architecture assertion。若不同意这条更小路径,请在 PR 中明确量化实际维护收益与失败历史,而不只是 site 数量和假设性 drift。修复后需要加入上述 false-positive/false-negative scanner 反例、重跑 focused suites 与 required CI。本次 review 不授权 merge。
English verdict: REQUEST_CHANGES on exact head cc93a8932e123211e9f5027e80f970eb57210e3c. The migrated digest values are behaviorally equivalent and the focused suites pass, but the new 381-line production guard is not a sound ownership check: its flat name table leaks constants across lexical scopes, while annotated constants, .format(), and .join() bypass it. It also duplicates the repository's existing scope-aware folding machinery. The claimed converted periodic-report surface still exempts three same-package producers solely to avoid regenerating a manifest, leaving the refactor fragmented and its maintenance ROI unproven. Reuse/extract the existing scope model, finish or honestly narrow the surface, provide concrete before/after maintenance benefit, and rerun the targeted and required checks.
Signed-off-by: 牛锐博 <niuruibo@niuruibodeMacBook-Air.local>
Goal And Delivered Outcome
Outcome basis / optional anchor: Closes [Architecture]: the digest owner recognizes an envelope, 94 sites still build it by hand #5336, which is anchored in the repository's own text:
tests/architecture/test_content_digest_single_owner.pyends its docstring with "producers that concatenate\"sha256:\"by hand are the other half of the decision and are deliberately unchanged". The single-owner guard therefore already names this gap and already declines to close it silently.Goal/source and gap: the shape owner recognizes the two whole-value digest shapes and exports nothing else. It is generated (
scripts/generate_semantic_bindings.py), and that generator refuses every construct except two flagless literal patterns -- by design, "Reject extra syntax rather than execute TS or guess its meaning" -- andtest_the_owner_is_a_leaf_and_exports_only_the_two_shapesasserts the leaf's namespace is exactly{re, BARE_SHA256_PATTERN, ENVELOPED_SHA256_PATTERN}. So there was no production counterpart, and every writer restated the envelope instead of asking for it.Measured on the base revision: 94 hand-built sites in 81 Python files, plus 26 sites in 17 TypeScript files. About thirty of them are near-identical private
_digest/_sha256/_canonical_digesthelpers. Consequence today: a digest written by one module and read by another is validated by the owner's pattern at the reader and by nothing at the writer.Observable before → after, with the validation row that proves it: inside
loopx/capabilities/periodic_report, the envelope is now built in one place (10 sites across 9 files) and an architecture test fails if any converted file restates it. No digest value changes: the behavioural cases call each migrated helper and compare against the expression it replaced.Intended base:
3ec049e13. Closes [Architecture]: the digest owner recognizes an envelope, 94 sites still build it by hand #5336 (the issue I filed for this half; it is not a pre-existing maintainer task).Scope And Continuation
loopx/control_plane/digest_envelope.py), the first surface converted, the production scan added, and the new module pinned into the existing guard'sCONSUMER_MODULES(one line -- layer 2 of that guard requires every importer of the owner to be listed).pending_intent.py,post_writeback_hook.pyandrequest_action.pyare census hosts whose import lines are pinned by row number inloopx/semantics/project_registry_io_manifest_v1.json. I regenerated that manifest in place to check this branch moves nothing (output identical: 260 sites, 0 unclassified). The remaining 71 Python sites and 26 TypeScript ones stay outside the scan until each surface is converted.CONVERTED_SURFACES; the scan, the allowlist-rot check and the value-based bypass corpus are already generic.Validation
cc93a8932(4 commits, 12 files, +455 -28).unitpytest tests/architecture/test_content_digest_production_owner.py-> 29 passed: the scan over the converted surface, the allowlist-rot check, eight bypass-corpus cases and the builder's own contract.unitpytest tests/architecture/test_content_digest_single_owner.py-> 211 passed with the new module added toCONSUMER_MODULES, so the existing four layers still hold against a second module in the family.unitpytest tests/capabilities/test_periodic_report*.py-> 268 passed;pytest tests/control_plane tests/architecture tests/canary -k "digest or envelope"-> 110 passed.integrationpytest tests/architecture tests/canaryin full, since this branch adds a module and touches a generated-adjacent manifest;tests/architecture tests/canary-> 1101 passed, 1 failed in 1m50s. The one failure,test_new_independent_twin_cannot_hide_behind_generated_pair, is not from this branch: the same six node ids that failed in the first run of this selection were replayed on an unmodified3ec049e13worktree under identical conditions (same venv, same Node on PATH,node_moduleslinked in both trees) and that one id is the only one that fails there too. The other five were caused by this branch and are fixed -- see the inventory row below.regression_parity_reference()in the test, the literal spelling each site used), covering both canonicalization families and the one site that hashes a string rather than canonical JSON. The project-registry I/O manifest regenerated in place is byte-identical, which is what proves no census anchor row moved.staticpython -m ruff checkon every changed path: clean.python -m mypy(no arguments, as CI runs it):Success: no issues found in 19 source files.git diff --check: clean.loopx check --scan-pathon the new module, the migrated package and the new test: public boundary scan clean.staticruff format --checkis not a gate here, and four of the nine migrated files (adapters,cadence_journal,machine_defaults,runtime_producer) already report would-reformat hunks on the unmodified base. I counted hunks per file on base and on head: 5, 10, 2 and 6 in both, so this branch adds none. Both new files are format-clean.mutationre.compilecaches by pattern and flags, so a restated copy of the owner's pattern compares identical to the borrowed object and the identity assertion proved nothing on its own (N7); and the event-digest recipe used ASCII-only input, whereensure_asciiis not observable, so flipping it passed (N8). Remaining caught: a converted file reverted to hand-building (N1, N4); the constant folder removed (N2); the percent arm removed (N3); builder validation removed (N5, 7 cases) and builder emitting the bare digest (N6, 12 cases); the consumer pin removed (N9); the prefix constant renamed back into a name collision (N11, 18 cases).staticENVELOPE_PREFIX, whichhandoff_fragments.pyalready binds to a different value, andexamples/semantic-vocabulary-drift-smoke.pycounts named string constants across modules: the branch grewconflicting_values16->17,conflicting_definitions55->57 andconflicting_values_semantic0->1 -- three budgets, none reviewed. Renaming the constant returns the smoke tookwith every budget at its reviewed value, so this consolidation does not add a second meaning to a name.test-shard (1)andtest-shard (2)are red, with two failing tests between them.test_turn_contract_generation.py::test_new_independent_twin_cannot_hide_behind_generated_pairis the same failure reproduced locally on an unmodified base worktree (above).test_prompt_upgrade_hook.py::test_live_decision_adds_only_existing_required_read_channelI could not see locally at first because CI shards it differently, so I ran that one test file in both trees under identical conditions: 2 failed on this head, the same 2 failed on the unmodified base, same parametrizations. Neither is upstream-of-me in the sense of a main-side aggregate gate -- both are pre-existing repository failures this branch inherits.kernel-static-checks,typescript-core (1/3)and(2/3),chat-bundle,dashboard-acceptanceandwindows-powershellare green at this head.test_the_owner_is_the_only_module_that_states_the_prefix_ruledoes run tree-wide, but only for the narrow fact that no second module binds the prefix as a constant; it cannot see an inline literal, which is what the per-surface scan is for. The byte recipes stay per-module (two useensure_ascii=True, one hashes a string): consolidating those is a different decision about canonicalization, not about the envelope, and this PR does not claim it.See validation disclosure guidance.
Frontend / Visual Evidence
Type of Change
No observable behavior changes: every migrated site emits the same bytes it emitted before, which is what the
regression_parityrow exists to show rather than assert in prose.LoopX Area
Technical Direction
Shared-authority RFC fixture impact
N/A -- no RFC dimension is claimed here. The shared coordination fixture, the envelope and the provider arms are untouched.
Boundary Checklist
none.