Skip to content

refactor(public-safety): own the lowercase public-safe slug shape - #5360

Merged
huangruiteng merged 3 commits into
loopx-project:mainfrom
karenchuu:codex/public-safe-slug-owner
Oct 1, 2026
Merged

huangruiteng merged 3 commits into
loopx-project:mainfrom
karenchuu:codex/public-safe-slug-owner

Conversation

@karenchuu

Copy link
Copy Markdown
Contributor

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:17 and capabilities/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.py states the shape once as PUBLIC_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-unused import re lines 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's PUBLIC_SAFE_REFERENCE_PATTERN set the precedent of putting an allowlist shape there), it is reachable from control_plane/handoff and both capability packages without crossing test_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.json allows 147 and loopx/*.py is at exactly 147.

  • Intended base: a9ee074de.

Scope And Continuation

  • Done: the owner constant, three converted modules, one 41-case guard.
  • Declared, not hidden -- the four files whose import blocks an open branch of mine (refactor(digest): one owner builds the stored SHA-256 envelope #5337) is rewriting: 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_SITES pins 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.
  • Not collapsed, asserted as probes that must not be reported:
  • Deliberately per-surface: each converted module keeps its own _token-style helper and its own error text, because those describe the owning surface, not the shared rule -- the same split public_safe_text.py documents for the four text validators.
  • Slice boundary / successor: complete within scope; reverting is three imports and one constant.

Validation

  • Tested revision: c804d5c9f (2 commits, 5 files, +484 -8).
  • Run state: finished.
  • Input classes: synthetic fixtures; the affected scope also runs the CLI-subprocess quota family.
Check kind Result Public-safe evidence / limitation
unit passed pytest tests/architecture/test_public_safe_slug_owner.py -> 41 passed in 6.4s: value scan over loopx/ 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.
integration passed pytest tests/architecture tests/canary in 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_parity passed pytest over the 48 test files that mention periodic_report, review_batch or public_safe_text -> 1204 passed, 1 failed in 2m51s. The one failure is tests/control_plane/test_quota_settlement_cli.py::test_read_only_settlement_omits_non_causal_delivery_workspace. Attribution, run under identical conditions on an unmodified a9ee074de worktree 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.
static passed python -m ruff check on all five changed paths: clean (it is also what caught the two import re lines 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.
static passed loopx check --scan-path for 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 budget passed examples/semantic-vocabulary-drift-smoke.py on the unmodified base worktree and on this head, same venv, same Node 22.23.2, same node_modules: output byte-identical, conflicting_definitions=55/55, conflicting_values=16/16. As in the benchmark slice, a re.compile value 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).
canary passed loopx canary premerge with the five changed files passed explicitly: selected 18 / executed 18 / failures 0 / warnings 0, status: passed, no manual holds.
mutation passed 11 mutations, one at a time in a separate worktree at this head, all files restored before every round, control round green (41 passed) before and after: 10 caught / 1 survived. Caught: a converted consumer restating the regex and calling it (M1, 7 cases incl. the periodic-report suite); a half-anchored restatement ^[a-z][a-z0-9_.-]{0,127} (M2, 3 cases -- the scan normalizes anchors because they are redundant under fullmatch); a shape assembled from two same-file constants (M3); an inline re.fullmatch inside 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 -- two edit() 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 adds token.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.
frontend none No UI, projection or rendered surface changes.

Type Of Change

  • Internal refactor / single-owner convergence with an anti-regression guard. No behaviour 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

  • Seven copies of one shape is the pattern this repository already rewards with a guard: the decision, not the spelling, is what gets pinned. Anchors are normalized because fullmatch makes them redundant, which is exactly the case a text-diff review would wave through.
  • Staged boundary: four sites are declared because a sibling branch owns their import blocks. The declaration is machine-checked, so the boundary cannot quietly become permanent.

Boundary Checklist

  • Neither the diff nor this PR body/comments/attachments disclose private state, credentials, raw traces or verifier output, internal links, or local machine paths.
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.
  • I completed the visual evidence section for UI changes, or marked UI impact none.

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>
@karenchuu

Copy link
Copy Markdown
Contributor Author

CI triage for exact head c804d5c9fe71416ff298ab305f69e6c6db99df75:

The three failed Python shards all point to the source-fingerprint churn boundary, not to this PRs public-safe slug changes:

  • test-shard (1): test_runtime_source_churn_has_a_stable_readiness_diagnostic
  • test-shard (2): test_runtime_fingerprint_rescans_when_a_snapshotted_file_disappears_while_reading
  • test-shard (3): test_runtime_request_source_churn_raises_a_stable_startup_diagnostic

Those failures are in loopx/control_plane/effect_runtime.py and tests/control_plane/test_turn_journal_runtime_readiness.py; neither path is changed here. This PR changes the canonical slug owner, three consumers, and its architecture guard. The TypeScript shards and other required checks passed.

The branch is currently 44 commits behind main (tip 638d0e39c, measured when this comment was written). Current main includes 647756d21 / #5367, which specifically updates the source-fingerprint snapshot implementation and these readiness tests. Please update the branch from current main and rerun CI rather than adding unrelated runtime work to this slug-owner refactor.

This explains the red checks only; it is not an approval. The updated exact head still needs review.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

本次完整评审针对 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

@huangruiteng
huangruiteng merged commit 8c136de into loopx-project:main Oct 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants