fix(public-safety): decide the local-path check in one owner - #5296
huangruiteng merged 2 commits into
Conversation
Refs loopx-project#5136 direction 3. `decision_context/packets.py` and `material_lifecycle/_validation.py` each enforced "does this text carry a local path?" with an identical private regex, while the credential and raw-remote-location rules beside them already came from the shared seam. `loopx/public_safe_text.py` now answers that question once through `find_public_safe_local_path`, over the absolute-root pattern, the two direction-3 gap shapes loopx-project#5245 left opt-in, and a colon/equals boundary arm that preserves what both sites already rejected. Sites keep their own messages and length limits; `LOCAL_PATH_SURFACE_PATTERN` and `classify_private_text(..., include_path_gaps=False)` are unchanged, so the consumers that read the narrower shape are untouched. Recognition widens at the migrated surfaces: drive letters, UNC shares, `path:`-prefixed and home-relative paths with a Windows separator, roots like `/home`, `/var/folders`, `/opt`, and colon-introduced references are now caught. Nothing previously rejected is accepted. `file://` keeps failing through the raw-remote-location rule, and `validate_public_safe_value` is deliberately not tightened here because direction 3 decides rejection per destination. Signed-off-by: gcl-coder <88359731+gcl-coder@users.noreply.github.com>
The two migrated surfaces had no test asserting their local-path rejection at all, so nothing pinned even the narrow set their copies covered. `tests/control_plane/test_public_safe_local_path_owner.py` pins: - both sites reach the same function object the owner exports, and neither declares a module-level local-path regex any more (AST check over those two modules, with the inlined-copy limitation stated rather than hidden); - the owner's four arms in order, so dropping one fails; - no loosening, twice: as a differential over every rendered row of the shared corpus, and as an enumerated 14 boundaries x 22 roots x 2 positions product; - the 14 shapes the copies missed, which is the disclosed widening; - rejection through the shipped public builders with each site's own message and its 320/128 length limits intact; - the two deliberate non-changes: `file://` still failing as a raw URL, and `validate_public_safe_value` still accepting a home-relative reference so a later per-destination pass has to change it on purpose. Signed-off-by: gcl-coder <88359731+gcl-coder@users.noreply.github.com>
CI red: attribution and controls (exact head
|
| CI failing test | control on unmodified 5ebeb5579 |
this PR's relationship |
|---|---|---|
tests/control_plane/test_quota_settlement_cli.py::test_standard_codex_app_settlement_is_receipted_and_idempotent |
fails (shard sweep, 3 failed / 903 passed) | no file in the diff touches quota, scheduler or settlement |
tests/control_plane/test_quota_settlement_cli.py::test_todoless_autonomous_replan_settles_quota_refresh_spend_chain |
fails (same run) | same |
benchmark/widesearch/tests/test_run_config.py::test_app_server_environment_keeps_profile_temp_scope_and_nonsecret_sentinel |
fails — run as a single test id, same assertion at test_run_config.py:70, 0.06s on base vs 0.11s on this head |
this PR changes no file under benchmark/ |
Whole-tree sweep on this head: 2 failed, 13253 passed, 49 skipped (54m04s) — the two quota ids above. Base run of the same scope: 3 failed, 903 passed, i.e. the identical two plus ::test_read_only_settlement_omits_non_causal_delivery_workspace, which fails on base and passes here. The head failure set is a strict subset of the base failure set, so net regression is zero.
Two disclosures about that sweep, so the numbers can be re-used:
- The
benchmark/widesearch/tests/tree is not collected bypytest tests/, which is why the third CI failure does not appear in the 2-failed aggregate. It is covered by the single-test control row above instead. - My first full sweep of this tree reported 97 failures. The cause was environmental, not semantic:
tests/architecture/test_semantic_*parses TypeScript and needs the repository's npm dev dependencytypescript, which a freshgit worktreedoes not carry. After installing/linkingnode_modulesthe same files pass, and the 2/13253 figures above are from the re-run.
Independent corroboration: the maintainer's own PR #5293 is red today on the same ids — test-shard (1)/(2)/(4) at 1 failed each, including test_app_server_environment_keeps_profile_temp_scope_and_nonsecret_sentinel and test_standard_codex_app_settlement_is_receipted_and_idempotent. PR #5285 is red on the quota id plus tests/architecture/test_project_registry_io_census.py::test_checked_in_project_registry_io_manifest_is_current and test_goal_instance_binding_inventory.py::test_goal_instance_inventory_does_not_replace_the_registry_io_census.
So this PR cannot go green before the in-flight main repairs land — #5288 (census line plus the two settlement journeys) and #5292 (hygiene fixture, IP-030 goal_storage, dashboard home-route smoke, chat throughput guard). Nothing in this PR's scope overlaps those files, so no rebase is needed to keep this head current apart from the usual merge.
Local validation used the checkout interpreter with the borrowed venv on PATH and PYTHONPATH pointed at this worktree; the two migrated files and the new test are ruff check clean, and ruff format --check reports loopx/public_safe_text.py and loopx/capabilities/decision_context/packets.py as needing reformatting on unmodified main as well, so they were left as-is rather than reformatted in this diff.
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
动机
评审 exact head 03e0fb106586bd02758c1e4c664bc05c5518278d。#5136 direction 3 要求路径形状识别归属共享 owner，同时由各输出边界决定拒绝策略。decision-context 与 material-lifecycle 过去各复制一个窄 regex，导致它们能把 home-relative Windows、显式 path-prefix、drive/UNC 或其他常见根目录写进标为 public-safe 的字段。这份 PR 收拢两个公开 builder 的路径判断，不宣称完成所有 publication sinks 的治理。
改动思路
共享 public_safe_text 负责“是否是本地路径”，两个已有 validator 继续拥有长度、字段、拒绝原因及 credential/URL 的先后顺序。复用已有 absolute/home-relative/path-prefix 形状，并补回旧 regex 已认识的冒号/等号边界，避免迁移时反而放宽旧输入。新 helper 仅由这两处显式采用；不修改通用 recursive export gate，也不悄悄打开 classifier 的 include_path_gaps 默认值。
具体改动
关键代码讲解
find_public_safe_local_path（loopx/public_safe_text.py:280）按固定 pattern tuple 返回首个命中；这是路径类别的检测 owner，不会创建 authority、读取文件或让所有调用者自动采用更严的政策。它复用已有形状，旧 colon/equals 边界另有明确兼容 arm。_compact_text（loopx/capabilities/decision_context/packets.py:78）用该 helper 替换私有_LOCAL_PATH_RE，仍先做空值/320 字符限制，再做 local-path、raw URL、credential 检查。实际build_decision_evidence_packet的嵌套 fact summary 会带着具体字段位置拒绝，合法 opaque ref 的包和 digest 与 base 相同。compact_text（loopx/capabilities/material_lifecycle/_validation.py:54）完成同样替换，但保留 material 自己的错误措辞。除了 intake token 路径，我还通过真实build_material_rerank_proposal的no_change_reason验证文本字段：base 接受的路径在 head 被 local-path 错误拒绝，替换成普通 opaque 描述后能继续生成不带 apply authority 的 proposal。
同一组 15 个合成输入分别经过 base/head 两个公开 builder、runtime export validator 及 classifier 的默认/显式 gap 分支。新增的五类路径只使两个 builder 从 accepted 变为 rejected；其余十组，包括合法文字、旧 home/colon-equals 拒绝、空值、长度、file URL，以及路径叠加 URL 的拒绝优先级，结果完全一致。全部十五组的 runtime export/classifier 输出也一致，没有把旧内部/布局 policy 一起收紧。普通任务是用已有公开描述生成 evidence/proposal，最短合法路径仍是原来的单次 builder 调用，没有新增表单、确认或重填既有 authority 信息。拒绝后的唯一新增纠正是把不适合公开的 locator 换成 opaque 描述；实际成功包的单独输出对照证明它能恢复，而非只给出一条拒绝建议。
对主干的风险
主要风险是共享规则扩张造成未迁移入口的误拒绝，或遗漏旧边界造成信息泄漏。我检查了 re-export 调用方、未修改的 extension presentation 与实验别名策略；这些不同场景不因 regex 外观相似就强行合并。实际 builder 的错误字段、完整错误文本、长度优先级与 correction 后的成功包都做了对照；相关 Python 测试 567/567、TypeScript 共享 corpus 2/2、仓库 lint 与 mypy 通过。按不可变 base 选择的 premerge 5 个 direct checks 与 16 个 selected checks 全部通过，包括 semantic vocabulary 与 public/private boundary 检查。
语义与 CI 对齐
这是两个 public-safe 文本入口的明确默认收紧，PR 说明、owner 注释及 durable regression tests 已点名新增路径形式和保留的策略。共享 classifier 的 detection/category 合同、recursive export gate、packet schema、authority 标记与 apply gate 没有改动；file:// 在这里仍按既有 raw-URL 原因拒绝，并没有把分类名称的后续工作冒充已完成。验证采用当前 capability 的仓库本地策略，不查询、不等待远端 CI；作者提到的远端红灯不会在没有因果证据时转成 request-changes。
本次覆盖 Python 公开 builder 与共享 TS corpus，不包含所有外部 publication destinations 的最终重验证；#5136 的其余调用方与 per-destination policy 仍由既有后续边界负责。没有新增 UI 配置或 runtime 开关：直接调用现有 builder 就经过其已有 validator，frontend 或 Lark 不需要在这个纯文本合同 slice 新增另一份路径规则。
我的整体评价
APPROVE：未发现阻断性问题。它把两个活跃公开入口的重复检测知识交回已有 owner，并用真实 consumer 对照证明“该收紧的收紧、不该变化的不变”，是 #5136 的合理增量。持续推进保持原样：合法 packet/digest、未迁移入口和 apply gate 不变；用户体验改善在于以前会流进 public-safe 包的路径被具体字段错误挡住，改成 opaque 描述即可恢复。它复用既有路径类别，不创建新的 authority 词表。未来可维护性复查已在最邻近 owner 完成：删除两份私有 regex，而不是新建 public-safety capability 或统一所有 policy。保留各入口自己的错误/长度规则是有意的调用方合同，不是需要再抽象的重复 authority；全体 sinks 的路线仍按原 issue 推进，不要求为已交付的 slice 另造仪式性 Todo。
English verdict: APPROVE — Both public builders now use the shared local-path detector, with intentional tightening validated against the immutable base and unchanged behavior verified for unrelated export/classifier consumers. No blocking finding; broader destination-specific policy work remains outside this slice.
2 participants
Goal And Delivered Outcome
Outcome basis / optional anchor:
Refs #5136, direction 3 as written by @huangruiteng on 2026-09-27 ("The shared classifier should recognize home-relative paths, Windows drive/UNC paths, and local paths behindfile://orpath:prefixes. Public-safe output should reject/redact those local references."). Related to refactor(control-plane): single owner for private-text classification #5245, which madeloopx/public_safe_text.pythe text-classification owner and left the two path-gap patterns opt-in.Issue/task and intended base: [Architecture]: two modules both claim to own "private-looking text" #5136 (open). This PR delivers the local-path half of direction 3. It does not claim the bare-word or short-assignment half of direction 2, so [Architecture]: two modules both claim to own "private-looking text" #5136 stays open.
The gap. Two public-safe contract modules still decided "does this text carry a local path?" with their own copies of the same regex:
loopx/capabilities/decision_context/packets.py:43—_LOCAL_PATH_RE = re.compile(r"(^|[\s:=])(?:/Users/|/private/|/tmp/|~/)")loopx/capabilities/material_lifecycle/_validation.py:18— the identical patternBoth modules already imported the credential and remote-location shapes from the shared seam (
SECRET_LIKE_SURFACE_PATTERN,REMOTE_LOCATION_SURFACE_PATTERN), so they were half-migrated: two of the three rules they enforce came from the owner, and the local-path rule did not. Neither copy was covered by any test that asserts its rejection message (git grep "must not contain a local path" -- tests/returns zero hits before this PR), so nothing pinned even the narrow set the copies did cover.Observable before → after. Both surfaces now answer through
loopx/public_safe_text.find_public_safe_local_path, which recognizes:/Users/…,/private/…,/tmp/…,~/…~\…(home-relative, Windows separator)path:/srv/…,PATH:\…C:\…,D:/…,\\server\share\…/home/…,/var/folders/…,/etc/…,/opt/…,/srv/…,/mnt/…,/root/…,/data/…,/workspace/…/USERS/…,"/Users/…"(case, quote-boundary):/Users/…,=/private/…(colon/equals boundary)Measured, not asserted: over 51 samples (hand-written shapes plus every rendered row of the shared corpus
tests/fixtures/public_safe_text_corpus.json) the change rejects 17 additional values and accepts 0 values that were previously rejected.test_owner_covers_the_whole_legacy_equivalence_classthen enumerates 14 boundary spellings × 22 roots × 2 positions = 616 combinations and asserts the one-directional property (no loosening) over the whole product rather than a sample.Scope And Continuation
Completed scope and remaining work: complete within this scope for the local-path decision at these two surfaces. Deliberately not done, each pinned or named below:
file://is not reclassified. Direction 3 does call it a local path, but both sites already reject everyfile://value through the raw-remote-location rule they import, with their own wording, so the observable verdict does not change here.test_file_url_verdicts_are_unchanged_and_still_named_as_raw_urlspins that. Making the category local instead of remote would only move the reason string, and for surfaces that keep ordinaryhttp(s)URLs it is a per-surface disclosure — the same judgement refactor(control-plane): single owner for private-text classification #5245 left opt-in.validate_public_safe_valueis not tightened. It has 14 product call sites whose destinations differ (dashboard readback, artifact lifecycle, peer routes, chat status, …), and direction 3 decides rejection per destination. How many of those sites would newly reject was not measured in this PR;test_runtime_export_gate_is_unchangedpins the current, narrower verdict so that a later pass has to change it deliberately rather than inherit it silently.classify_private_text(..., include_path_gaps=False)keeps its default, andLOCAL_PATH_SURFACE_PATTERNis byte-identical, so the ~30 consumers that read the narrower shape are untouched. The new helper is a separate, additive decision entry point consumed only by the two migrated surfaces.loopx/extensions/presentation.py:49rejects//Users/alex/xand///tmp/a.log, which the owner's negative lookbehind does not match, so migrating it would loosen that surface; it also acceptsx=/private/a, which both migrated sites reject. Its set and the owner's are incomparable in both directions, so a drop-in is not behavior-preserving and needs its own decision.loopx/registry.py:89is not a projection-text admission at all: it is combined withPath(text).is_absolute(), skips/path/to/placeholders, and classifies stored registry keys, which answers "may this path be recorded", not "may this text be published".loopx/domain_packs/ml_experiment.py:66's leading-/-or-~rule is untouched, per direction 3's last sentence. It is already pinned as a stricter alias constraint than the shared classifier bytests/control_plane/test_public_safe_text_classifier.py:239from refactor(control-plane): single owner for private-text classification #5245, so this PR does not restate it.Slice boundary / successor: the useful delta is one owner for the local-path decision plus the widened recognition at the two surfaces that publish packet text. Remaining work belongs to [Architecture]: two modules both claim to own "private-looking text" #5136, and the next candidate is the per-destination export pass in (2) — it needs a caller/field → destination table, which is direction 4's contribution.
Validation
03e0fb106586bd02758c1e4c664bc05c5518278dunitpassedtests/control_plane/test_public_safe_local_path_owner.py— 50 tests: owner-object identity through the re-export seam, the four-arm tuple in pinned order, the enumerated no-loosening product, the shared corpus differential, both sites' own messages and 320/128 length limits, and the two pinned non-changes.unit(neighbourhood)passedtests/control_plane/test_public_safe_{text_classifier,text_owner_parity,safety_path_shapes,safety_field_name_spelling,safety_credential_shape_owner}.pyandtests/control_plane/test_remote_location_shape_owner.pyplus everytests/capabilities/test_decision_context_*.pyandtests/capabilities/test_material_*.pyfile: 567 passed, 0 failed. These are the suites that own the three shapes the migrated sites consume.real_entrypointpassedbuild_decision_evidence_packetwith a nestedchanged_facts[0].summary;build_material_candidate_intake_proposalwithmaterial_ref), not the private helpers, and the asserted values are shapes only the owner recognizes — so a site reverting to its old copy fails.regression_paritypassed:/=boundary arm (3), owner recognizing nothing (25),decision_contextlosing the check (1),material_lifecyclereusing the raw-URL wording (2), and the runtime export gate being widened (1, the pinned non-change). The whole shared corpus stays verdict-identical, which is the parity evidence for the surfaces that were not migrated.integrationfinishedtests/sweep on this head: 2 failed, 13253 passed, 49 skipped (54m04s). Both failures aretests/control_plane/test_quota_settlement_cli.py::test_standard_codex_app_settlement_is_receipted_and_idempotentand::test_todoless_autonomous_replan_settles_quota_refresh_spend_chain, and a control re-run of the same scope on unmodified base5ebeb5579gives 3 failed, 903 passed — the identical two plus::test_read_only_settlement_omits_non_causal_delivery_workspace, which passes here and fails there. So the head failure set is a strict subset of the base failure set: net regression zero. Environment disclosure: the first sweep of this tree reported 97 failures because the repository npm dev dependencytypescriptwas not installed, which is whattests/architecture/test_semantic_*parses with; after linkingnode_modulesthe same files pass, and the numbers above are from the re-run.integration(CI third failure)failed(base-present)test-shard (4)reports a third failing test,benchmark/widesearch/tests/test_run_config.py::test_app_server_environment_keeps_profile_temp_scope_and_nonsecret_sentinel. It lives outsidetests/, which is why the sweep above does not count it. Running that one test id in isolation gives the same assertion attest_run_config.py:70on this head (0.11s) and on unmodified5ebeb5579(0.06s); this PR changes no file underbenchmark/. The maintainer's own PR #5293 is red today on the same two test ids (test-shard (1)/(2)/(4), 1 failed each).staticpassedpython -m mypy(the configured CI scope, 19 source files) clean;ruff checkclean on all five touched files;python -m loopx.cli check --scan-pathover the four changed modules and the new test — public boundary scan clean, 0 errors.static(disclosed)not_applicableruff format --checkreportsloopx/public_safe_text.pyandloopx/capabilities/decision_context/packets.pyas needing reformatting on unmodifiedmain(verified in a clean worktree at the same base revision), so this PR does not reformat them; only the new test file is ruff-formatted. Two capability modules are outside the mypy allowlist, so no new typed surface is claimed.real_backendnot_applicablevalidate_public_safe_value(explicitly out of scope, item 2 above), and the two non-equivalent regexes in item 4 — forextensions/presentation.pythe PR states the measured counterexample rather than asserting a reason. Source-level guard limitation, stated plainly: the AST scan in the new test rejects a re-declared module-level copy in the two migrated modules, but an inlinedre.search(...)at a call site is caught only by the behavior tests (mutation M1/M2 confirm exactly one test fails for that case). Per [Architecture]: two modules both claim to own "private-looking text" #5136 direction 4, a source-level duplication guard supplements these tests and cannot establish semantic correctness.Frontend / Visual Evidence
Type of Change
Widening recognition is a behavior change: at both surfaces the accepted set grows by the 17 measured shapes above, so this is deliberately not filed as "no functional changes". The strings users see on the paths that already failed are unchanged, and no previously rejected value is accepted.
LoopX Area
Technical Direction
tests/fixtures/public_safe_text_corpus.jsonis the same file, byte-for-byte, so the shared corpus pins both runtimes exactly as before and the widened half is Python-side only.Shared-authority RFC fixture impact
Boundary Checklist
.loopx/,.codex/goals/, and liveACTIVE_GOAL_STATE.md).none.Signed-off-bytrailer (git commit -s).Relationship To Merged Work
LOCAL_PATH_GAP_PATTERNSis still the same pair, andPUBLIC_SAFE_LOCAL_PATH_PATTERNSreferences those objects instead of restating them.