refactor(public-safety): own the lowercase public-safe slug shape - #5360
Conversation
Seven modules compiled the same slug shape themselves. This converts the three that no other open branch is editing: the periodic-report request core, the issue-fix report source and the hand-off review batch. Two of them also dropped a now-unused import re. The canonical private-text module is the owner because the shapes a public-safe field may hold already live there, and control_plane plus two capability packages can reach it without crossing the import-boundary guard. Signed-off-by: karenchuu <25980598+karenchuu@users.noreply.github.com>
The scan folds a pattern through same-file string constants and normalizes anchors, so a half-anchored or assembled restatement is reported the same as a literal copy. A construction whose value cannot be folded must be declared with its file and count. Four periodic_report files keep their own copy, declared by file and count: their import blocks are being rewritten by an open branch, so converting them here would only create a conflict between two pending pull requests. Both directions of each count are checked, so converting one without retiring its entry fails. Signed-off-by: karenchuu <25980598+karenchuu@users.noreply.github.com>
|
CI triage for exact head The three failed Python shards all point to the source-fingerprint churn boundary, not to this PRs public-safe slug changes:
Those failures are in The branch is currently 44 commits behind This explains the red checks only; it is not an approval. The updated exact head still needs review. |
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本次完整评审针对 c804d5c9fe71416ff298ab305f69e6c6db99df75,以不可变 merge-base a9ee074dea6ab5c482024b3ca59a974d06f07197 对比全部五个文件。没有阻塞发现。实际交付是把 report core、review batch、issue-fix report source 的相同 lowercase slug 语法收敛到现有 public-safe lexical owner,减少后续改规则时三处手动同步,并保持普通输入、错误反馈和后续使用结果不变;不是完成所有七个副本的全量迁移。
改动思路
复用 public_safe_text 比新建 token capability、类型状态机或通用 provider 框架更小且合适:这里只统一稳定的字符串语法,不授予发布或操作权限。各入口的 normalization、错误文本和 tags 过滤仍由原调用者负责,没有把不同语义的 identity、reference 或 connector token 正则混为一谈。四个 adapters/archive/audience/bindings 副本仍显式留在 guard 清单中;它们同时涉及 #5337 的 digest 改动,集成后由 periodic-report owner 继续替换并删除对应 allowance。#5337 是集成依赖,不是已经完成这四处迁移的证据。三处当前真实调用路径可以独立验证和回滚,因此我将本 PR 判断为有用的 bounded increment。
具体改动
关键代码讲解
loopx/public_safe_text.py:156 — PUBLIC_SAFE_SLUG_PATTERN:保留原先 ASCII 小写开头、后续小写/数字/点/下划线/连字符、总长至多 128 的 grammar。统一的是 regex 对象及其 owner,不是扩展输入域。loopx/capabilities/periodic_report/core.py:89 — _token与loopx/control_plane/handoff/review_batch.py:58 — _token:仍先做str(value or "").strip().lower(),再 fullmatch,失败走原 ValueError。公开build_periodic_report_run和build_review_batch的完整输出、摘要和错误优先级没有变化。loopx/capabilities/issue_fix/periodic_report.py:54 — _tags:仍先 trim/lower、把下划线改成连字符,再过滤非法 tags;不能因共享 pattern 而把它改成 report 字段的硬拒绝路径。tests/architecture/test_public_safe_slug_owner.py:159 — shape_rows:常量折叠、owner inventory、真实对象 identity 和边界 corpus 提供长期维护保护,但扫描器不是 Python import resolver,也不是安全授权检查。
独立验证结果:focused pytest 122 passed,涵盖新 architecture guard、report/batch、adapters/profile/audience/bindings 和 import boundaries;五个 changed paths 的 Ruff、mypy(19 source files)、git diff --check 通过,native public-boundary scan 无错误。另用同一独立 fixture,在实际 base/head checkout 上对三个公开 builder 执行 431 组 omitted/null/空值、大小写、Unicode、控制字符、长度边界、类型和组合输入,以及六组重叠错误条件,共 1,299 条完整输出/异常观察逐项相同;没有丢弃错误文本、摘要或筛选结果。将 report 上限在进程内缩小一个字符后,独立真实入口 oracle 如预期失败;恢复当前 head 后通过,证明不是只比较“看起来一样”的 decision code。
语义与 CI 对齐
既有 lexical contract 的 owner 收敛,未新增状态、协议词汇、权限、schema 或默认行为;没有 prose denylist 或 Python/TS 并行状态决策源。该旧 checkout 的 development advisory 尚不支持 --changed-from,本次未将其误报为通过,也未用 advisory 空结果替代语法和入口 parity。按当前 PR-review capability 的 wait_for_ci=false 合同使用本地 native 验证,未获取或等待 GitHub CI;没有凭作者的 CI 说明推断远端失败归因。
对主干的风险
主要风险是未来维护 guard 被误当成完整的“唯一 owner”证明。独立 synthetic probe 用 import re as rx; PATTERN=rx.compile(...) 声明相同形状时,shape_rows 返回空列表:新副本经别名进入非已知 consumer 就可能漏检。非阻塞 P2 建议:在 architecture-test parser 这一相邻边界解析 import re as ... / from re import ... as ...,补别名负例;或者明确扫描范围。与 #5361 的折叠/扫描代码相近,两个 PR 集成时可共用小型测试 resolver,避免复制修复。这是具体、可逆的后续简化,不需要生产框架或无关 TS 重写;当前三个实际 consumer 还有对象 identity 检查及独立入口 parity,所以不构成当前运行行为 blocker。
没有 UI/config/CLI 字段变化,也没有存储写入、scheduler 或权限路径变化,因而不需要 frontend companion。未执行全树 suite、packaged frontend 或 deployed promotion;这些不被当前纯 lexical owner 移动覆盖,也不作为已验证结论。native scan 的外部 Goal 状态 warnings 与这五个公开文件扫描结果分开,不当成 PR 缺陷或公开内部细节。
我的整体评价
APPROVE:三处重复规则真实退役,普通用户调用不增加设置、确认或恢复步骤;1,299 条 base/head 观察保持完整语义,122 个 focused tests 和 static/boundary 检查支撑这一有界交付。未来面向 pass 已考虑 architecture guard 的 alias coverage/公共 parser,并作为非阻塞建议延后;四个剩余副本的存在明确披露,不能用本批准关闭全量 slug 收敛目标。批准只针对本 exact head,不是合并或 CI-ready 证明;批准发布/readback 后会按 capability 的 approval-closeout 流程检查有效旧阻塞评审,只有逐项验证已解决且权限成立时才撤销,保留历史讨论。
English verdict: APPROVE
Goal And Delivered Outcome
Outcome basis / anchor: no pre-existing issue. The anchor is a measurement on the intended base
a9ee074de: the shape^[a-z][a-z0-9_.-]{0,127}$-- "is this identifier a lowercase public-safe slug" -- was compiled by seven modules themselves:capabilities/periodic_report/core.py:17,capabilities/issue_fix/periodic_report.py:20,control_plane/handoff/review_batch.py:15,capabilities/periodic_report/adapters.py:28,capabilities/periodic_report/archive.py:27,capabilities/periodic_report/audience.py:17andcapabilities/periodic_report/bindings.py:27. Consequence: tightening or loosening the slug rule was a seven-file change, and four of the seven lived in one package where the others could not see them.Observable before → after:
loopx/public_safe_text.pystates the shape once asPUBLIC_SAFE_SLUG_PATTERN, and the three modules no open branch is editing ask it for the answer -- the other four are declared by file and count in the guard, not silently skipped. Product side is 4 files, +14 / -8: three compiled copies and two now-unusedimport relines leave, one owner line and four imports come in. Every accepted and rejected value stays the same, which the guard's accept/reject corpus and 48 pre-existing test files confirm.Why this owner and not a new module: the shapes a public-safe field may hold already live in
public_safe_text.py(the module docstring calls it the canonical private-text owner, and refactor(public-safety): centralize compact identifier shapes #5351'sPUBLIC_SAFE_REFERENCE_PATTERNset the precedent of putting an allowlist shape there), it is reachable fromcontrol_plane/handoffand both capability packages without crossingtest_control_plane_import_boundaries.py, and it holds zero census rows. A new top-level module was not available at all:tests/architecture/top_level_module_budget.jsonallows 147 andloopx/*.pyis at exactly 147.Intended base:
a9ee074de.Scope And Continuation
adapters.py,archive.py,audience.py,bindings.py. Converting them in this slice would have created a conflict between two pending pull requests of the same author for no semantic gain.DECLARED_INDIVIDUAL_SITESpins one site per file, and the guard fails in both directions: a second copy appearing there, and a site converted without its entry retired. Successor: delete four entries when refactor(digest): one owner builds the stored SHA-256 envelope #5337 lands._IDENTITY_RE = ^[a-z][a-z0-9_.:-]{2,127}$(periodic_report/request_action.py:34,pending_intent.py:72) differs in bound and in allowing:-- a different decision with a different rejection, kept where it is.{0,199}reference shape with/:#-belongs to refactor(public-safety): centralize compact identifier shapes #5351's owner._token-style helper and its own error text, because those describe the owning surface, not the shared rule -- the same splitpublic_safe_text.pydocuments for the four text validators.Validation
c804d5c9f(2 commits, 5 files, +484 -8).unitpytest tests/architecture/test_public_safe_slug_owner.py-> 41 passed in 6.4s: value scan overloopx/with anchor normalization and same-file constant folding, declared-site counts in both directions, per-consumer identity plus a real reference, 7 accept / 14 reject cases including the 128/129 boundary, 9 spelling probes (6 that must be reported, 3 that must not) and the unfoldable-declaration probe.integrationpytest tests/architecture tests/canaryin full at this head -> 1112 passed, 0 failed in 2m04s. This branch adds a module-level constant to a file that several architecture guards read by name, so the whole pair was run rather than a slice.regression_paritypytestover the 48 test files that mentionperiodic_report,review_batchorpublic_safe_text-> 1204 passed, 1 failed in 2m51s. The one failure istests/control_plane/test_quota_settlement_cli.py::test_read_only_settlement_omits_non_causal_delivery_workspace. Attribution, run under identical conditions on an unmodifieda9ee074deworktree and on this head, alternating: the unmodified base failed that same file in round 1 (1 failed, 78 passed) while head passed both rounds (79 passed, 79 passed), and the node id alone passed 3/3 on each tree. It is a quota/heartbeat subprocess case that reads shared local state, so it flips with order and load; this branch adds nothing it depends on.staticpython -m ruff checkon all five changed paths: clean (it is also what caught the twoimport relines left unused after the migration).python -m mypy(no arguments, as CI runs it):Success: no issues found in 19 source files.git diff --check: clean.staticloopx check --scan-pathfor each of the five changed paths through this tree's own entrypoint:ok: true, "public boundary scan clean: 5 files"; both warnings concern the absent local.loopx/registry.json. One non-literal credential reference was downgraded, as it is on the unmodified base.semantics budgetexamples/semantic-vocabulary-drift-smoke.pyon the unmodified base worktree and on this head, same venv, same Node 22.23.2, samenode_modules: output byte-identical,conflicting_definitions=55/55,conflicting_values=16/16. As in the benchmark slice, are.compilevalue is not one of the inventory's counted kinds, so the new constant name enters no budget and no anchor needed editing; the name itself is unique in the tree (0 prior hits).canaryloopx canary premergewith the five changed files passed explicitly:selected 18 / executed 18 / failures 0 / warnings 0,status: passed, no manual holds.mutation^[a-z][a-z0-9_.-]{0,127}(M2, 3 cases -- the scan normalizes anchors because they are redundant underfullmatch); a shape assembled from two same-file constants (M3); an inlinere.fullmatchinside a function (M4); the owner's bound tightened by one character (M5); the owner's class allowing one more character (M6); a consumer keeping the import while deciding with a weaker local check (M7); a declared site gaining a second copy without retiring its count (M8, 3 cases); an unfoldable construction in a module that mentions the class (M9); a validator helper restating the shape inside a converted module (M10, 3 cases). Disclosure: M9 and M10 first reported survived, and the cause was my driver, not the guard -- twoedit()calls on one file each rewrote from the pristine copy, so the second silently discarded the first. Fixed by making each mutation one edit, then both were caught. Survived, and why it is equivalent: M11 addstoken.islower()alongside the owner check; for every value the pattern accepts that predicate already holds, and the periodic-report suite stayed green, so it is not a second owner.frontendType Of Change
LoopX Area
loopx/public_safe_text.py,loopx/capabilities/periodic_report/core.py,loopx/capabilities/issue_fix/periodic_report.py,loopx/control_plane/handoff/review_batch.py,tests/architecture/.Technical Direction
fullmatchmakes them redundant, which is exactly the case a text-diff review would wave through.Boundary Checklist
none.