Skip to content

refactor(public-safety): decide the raw-location shape in one owner - #5196

Merged
huangruiteng merged 1 commit into
loopx-project:mainfrom
karenchuu:karenchuu/single-remote-location-shape-owner
Sep 27, 2026
Merged

huangruiteng merged 1 commit into
loopx-project:mainfrom
karenchuu:karenchuu/single-remote-location-shape-owner

Conversation

@karenchuu

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

  • Outcome basis / optional anchor: Issue [Architecture]: two modules both claim to own "private-looking text" #5136 - "two modules both claim to own private-looking text". The maintainer accepted that boundary question as a real one in the review of refactor(public-safety): decide credential shapes in one owner #5135, and this PR is the half of it that needs no decision: a shape the canonical owner did not cover at all.

  • Goal/source and gap: three validators compiled the byte-identical pattern (?i)\b(?:https?|file|s3|gs|tos|hdfs):// under private names - loopx/capabilities/decision_context/packets.py, loopx/capabilities/material_lifecycle/_validation.py and loopx/domain_packs/ml_experiment.py - while loopx/control_plane/runtime/public_safety.py, which owns the sibling decisions (LOCAL_PATH_SURFACE_PATTERN, SECRET_LIKE_SURFACE_PATTERN), had no counterpart for "this text carries a raw remote location". The gap is concrete: a fourth caller had nothing to import and would write a fifth spelling, and adding one object-store scheme meant finding three unlinked literals. Unlike the credential-shape copies that refactor(public-safety): decide credential shapes in one owner #5135 left in place, none of these three carries a per-surface threshold - they are the same list, character for character.

  • Observable before → after, with the validation row that proves it: before, grep for the scheme list returns three modules; after, it returns exactly the owner, which is what test_the_pattern_is_compiled_once_by_the_owner asserts on every run. Each site still rejects through its own entry point with its own message, pinned per site by test_each_site_rejects_a_raw_location_through_its_own_entry_point. See unit, regression_parity.

  • Issue/task and intended base: Refs [Architecture]: two modules both claim to own "private-looking text" #5136; it stays open because the other half of it is not resolved here (below). Base is current main.

Scope And Continuation

  • Completed scope and remaining work: the raw-location shape is decided once. The local-path shape deliberately is not touched, because there the four spellings genuinely disagree and picking a winner is an owner call, not a deduplication. Measured on main with a probe set run against the live patterns:

    input public_safety owner periodic_report.core copy packets / _validation copy ml_experiment copy
    /Users/x/a.py rejects rejects rejects rejects
    ~/notes.md misses misses rejects rejects
    path:/Users/x/a.txt misses rejects rejects rejects
    /etc/passwd rejects misses misses rejects
    /var/folders/ab12/tmp/x rejects rejects misses rejects
    C:\Users\x\a.txt rejects misses misses rejects
    \\\\srv\share\a (UNC) rejects misses misses misses
    see /private/tmp/x now rejects rejects rejects rejects

    Read down any column and each one is stricter than the others somewhere: the owner is the only dialect that rejects a Windows UNC share and the only one that rejects /etc/passwd, yet it misses ~/notes.md (which the packets / _validation and ml_experiment dialects both reject) and it misses path:/Users/x/a.txt (which the periodic_report copy rejects). No caller can pick a winner, which is the substance of [Architecture]: two modules both claim to own "private-looking text" #5136 rather than a mechanical merge. Folding the four would change what four shipped validators reject, so it needs a decision with a named consumer; this PR hands over the measurements instead of guessing. ml_experiment reads broad in the table partly because its rule also rejects any value starting with / or ~, which is a different shape of decision again.

  • One new dependency edge, stated so it is reviewed rather than noticed later: loopx/domain_packs/ml_experiment.py now imports from ..control_plane.runtime.public_safety. It was the first such import from loopx/domain_packs/, so the arbiter used here is the repository's own rule rather than my judgement: tests/architecture/test_control_plane_import_boundaries.py passes unchanged (15 passed), and loopx/capabilities/ already holds dozens of inward imports of the same kind. If you would rather the domain packs stay control-plane-free, the one-line alternative is to leave ml_experiment out of this deduplication and keep the other two.

  • Slice boundary / successor: the local-path table above is the successor, gated on [Architecture]: two modules both claim to own "private-looking text" #5136.

Validation

Public-safe summaries only.

  • Tested revision: head of this branch, based on current main; baseline comparisons run on the unmodified base commit in a separate tree.
  • Run state: finished.
  • Input classes: synthetic strings (paths, URLs, opaque references); no fixtures added.
Check kind Result Public-safe evidence / limitation
unit passed python3 -m pytest -q tests/control_plane/test_remote_location_shape_owner.py → 17 passed. It pins: the scheme literal exists in exactly one module; all six schemes plus their upper-case spellings are recognised by the owner; ftp:// is still not recognised (so the merge changed no coverage, and widening the list stays a separate decision); each of the three sites rejects s3://… and file:///Users/… with its own wording through its own entry point; each site still accepts an opaque run-7/metrics.json; and the three sites keep their own length thresholds (320 / 320 / 160) and the domain pack's vendor marker terms, which were not folded in.
integration passed python3 -m pytest -q tests/architecture/test_control_plane_import_boundaries.py → 15 passed (the gate that judges the new import edge and the facade allowlist). python3 -m pytest -q tests/capabilities tests/control_plane -k "decision_context or material_lifecycle or ml_experiment or domain_pack or public_safe or periodic or packets or remote_location" → 802 passed, 0 failed (45s). python3 -m pytest -q tests/architecture on head and on the unmodified base: 2 failed each, the same two ids on both sides (test_goal_instance_binding_inventory.py::test_goal_instance_inventory_does_not_replace_the_registry_io_census, test_project_registry_io_census.py::test_checked_in_project_registry_io_manifest_is_current), both pre-existing at this base - and #5193 is already reconciling the census manifest, so they are not this PR's and not free to grab.
static passed ruff check over loopx/ and the new test; ruff format --check on the new test; mypy with no arguments → 19 source files clean (the pinned file set; this PR does not widen it); loopx canary premerge --from-git-diff → premerge done: passed selected=19 failures=0, public boundary ok: true.
regression_parity passed Seven mutations, each run against the new file: re-privatise the pattern inside packets (1 failed); drop tos from the owner (2 failed); remove the case-insensitive flag (7 failed); send ml_experiment back to its own literal (1 failed); widen the owner to include ftp (1 failed); edit one site's error wording (1 failed); make one site reject any dotted name (2 failed). One of these deserves emphasis: re-privatising packets leaves every behavioural test green - the site still rejects the same inputs with the same message - and is caught only by the literal scan. That is the definition of this defect class, and it is why the guard reads source text rather than outcomes.
real_entrypoint not_applicable No shipped CLI command, projection or host round trip changes: the three entry points are reached through the same call paths with an identical pattern object, and the suites above exercise them through those entry points rather than the pattern.
real_backend not_applicable No authority store, database or provider is involved.
  • Coverage and gaps: the diff is four modules plus one test file, and every changed line is exercised by a site-level test through the real entry point. Not covered / not claimed: (1) the local-path half, left open deliberately with the table above; (2) the two tests/architecture failures are pre-existing and untouched; (3) the full tests/ lane was not re-run for this PR - the pattern is character-identical to what all three sites already applied, so the risk this diff carries is wiring and ownership, which the 802-case neighbourhood run, the boundary gate and the mutation set target directly; treat the full lane as unexecuted rather than as a pass. Environment, for reproducibility: the interpreter is a virtualenv built with uv venv plus uv pip install -e ".[test]" rather than uv sync, and Node is pinned to the repository's qualified 22 line so the TypeScript-backed scans in the architecture lane could run.

Frontend / Visual Evidence

  • UI impact: none
  • Before:
  • After:
  • States and viewports shown:
  • Source data: none
  • Attention review: N/A - no user-visible surface changes.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

Shared-authority RFC fixture impact

N/A - no canonical record, provider arm, fixture dimension, projection or compatibility path changes.

  • Production-scale fixture schema:
  • Semantic dimensions changed, or reviewed no-impact rationale:
  • Provider conformance arms run:
  • Read-only legacy/file/PostgreSQL three-arm rehearsal (required for promotion, runtime-routing, or compatibility-projection changes):

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 (including .loopx/, .codex/goals/, and live ACTIVE_GOAL_STATE.md).
  • 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.

Refs loopx-project#5136: three validators compiled the identical scheme list for "this text
carries a raw remote location" under private names, while the public-safety owner
that already decides the sibling shapes had no counterpart, so a fourth caller had
nothing to import. The owner now holds the one pattern and each site keeps its own
error text and thresholds.

The local-path shape is left alone on purpose: those four dialects genuinely
disagree, and measuring them is the input loopx-project#5136 needs rather than a guess here.

Signed-off-by: karenchuu <25980598+karenchuu@users.noreply.github.com>

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

APPROVE:本次审查针对 c08ae0c0fd88dbae06dc6c2b8aeaf0b865ef25ba。没有发现该 PR 引入的阻塞问题;两项本地架构失败已经在不可变基线复现,不能据此要求本 PR 修改无关代码。

动机

三个公开字段校验器各自保存同一份远端位置 scheme 列表,后续增加或纠正一个 scheme 时容易漏改。这个 PR 只把形状识别移到同一个现有 owner,而不把各字段的拒绝策略混成一份。这个范围符合 #5136 最新维护者说明:当前抽取可作为行为保持的独立增量,完整的分类与目的地策略收敛仍归该 issue。长期维护风险下降,调用者的合法输入、失败提示和修正后的继续使用保持不变。

改动思路

我先挑战了两个替代方案:不改会保留三份必须同步的知识;把所有 URL 一律交给递归公开安全校验拒绝,则会误伤正常公开链接。当前设计复用 runtime/public_safety.py 的文本形状 owner,decision-context、material-lifecycle 和 ML domain pack 仅导入它;长度、别名要求和错误消息仍由各字段契约负责。没有增加 capability、配置开关、队列或持久化状态,也没有把字符串形状检测伪装成 Todo 状态分类规则。已有 Python 纯文本边界足以完成这次机械抽取,不需要为了语言迁移再造一个平行决策源。

具体改动

实际 diff 是五个文件,新增 151、删除 8 行,其中新增测试 135 行;生产侧只是共享常量及导入替换。测试里的字面量扫描能发现原样复制的 scheme 列表,但不是任意等价实现都不可能重复的证明。因此我另外通过三个真实公开 builder 做了完整输出和异常对照,而不是只调用私有 helper 或比较 reason code。未改动前端/API 投影格式;现有消费者接收的 packet 和错误契约没有变化,所以本次不需要设置页伴随修改。

关键代码讲解

对主干的风险

最危险的回归不是编译失败,而是导入后漏拒绝原有位置,或顺手扩大拒绝范围。我在实际 merge base 473263cbfd7d877e058941c9a770de4c7953d3bd 和该 head 上使用同一组 90 条观察:覆盖 scheme 大小写、合法别名、长度边界、空值、位置与敏感词重叠、完整异常文本、默认禁用和无副作用。全部规范化输出一致。另用独立进程把共享规则改成“永不匹配”或增加 ftp,两次都使真实 builder 的独立契约断言失败;恢复原实现后通过。

直接测试及 import-boundary 共 32 条通过,相关能力与控制面测试 802 条通过,配置的 Ruff、mypy 和 whitespace 检查通过。按实际 diff 选择的 premerge 验收另外完成五项直接检查和十六项 smoke,均通过且没有 tracked side effect;它不代表已获得合并权限。没有读取或等待远端 CI。

语义与 CI 对齐

本地完整架构套件在 base/head 均为 812 通过、2 失败。失败是 test_goal_instance_inventory_does_not_replace_the_registry_io_census 与 test_checked_in_project_registry_io_manifest_is_current;两边的完整八项诊断一致,涉及 authority-archive、peer-host-route、manager-inbox、peers 的 registry census,因果路径和 manifest 均不在本 PR diff 内。相关变更语义已有独立通过证据,因此这是另行处理的集成/merge-readiness hold,不是本 PR 的 REQUEST_CHANGES 理由。初次缺 npm 依赖的环境失败已补齐依赖后重跑,不算产品缺陷。

我的整体评价

这是必要而有界的去重:长期演进收益明确,用户体验和现有公开字段语义保持。未来方向检查也已执行:继续收敛分类/目的地策略应沿 #5136 的既有 owner 做,而不在这个机械抽取里扩大拒绝集合或另建框架。字面量扫描的局限、完整安全策略尚未收敛以及无关架构红项均已明确;这些不否定当前独立可回滚增量。批准该精确 head,未执行合并。

English verdict: APPROVE — exact head c08ae0c. Real public-builder parity and independent narrowing/widening mutations pass; unrelated architecture failures are identical on the immutable baseline and head. This approves the bounded extraction, not completion of #5136 or merge readiness.

@huangruiteng

Copy link
Copy Markdown
Collaborator

Reviewed frame: behavior-preserving extraction of the shared remote-location shape, not a global URL deny policy or completion of public-safety consolidation. This follows the current #5136 maintainer acceptance. Per-field diagnostics and thresholds remain independently owned; the ongoing policy-aware consolidation stays on that existing issue. Exact head: c08ae0c0fd88dbae06dc6c2b8aeaf0b865ef25ba.

Full exact-head review.

@huangruiteng
huangruiteng merged commit 8636bbd into loopx-project:main Sep 27, 2026
1 check 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