refactor(control-plane): single owner for private-text classification - #5245
huangruiteng merged 2 commits into
Conversation
…t owner Refs loopx-project#5136, direction 1. loopx/public_safe_text.py is now the one home for the decision "does this string look private?", and control_plane/runtime/public_safety.py consumes the shapes from it instead of restating a competing set. What moved * SECRET_LIKE_SURFACE_PATTERN, LOCAL_PATH_SURFACE_PATTERN and REMOTE_LOCATION_SURFACE_PATTERN are defined once in public_safe_text.py, byte-identical to the versions public_safety.py compiled before the move. public_safety.py re-exports them (redundant-alias form, the house style under --no-implicit-reexport), so its importers and its recursive payload validation are unchanged. * The text-owner rule set is now a list of pattern + category + reason entries. PRIVATE_TEXT_PATTERNS and find_private_text_match are derived from it in the same order, so the four text owners (feedback, authority, boundary_authority, the TypeScript Vision checkpoint) keep their exact verdicts. * classify_private_text / matches_private_text_policy answer detection with an explicit category and reason, and a caller names the policy (a set of categories) that decides what its own surface rejects. Recognizing a value no longer implies rejecting it everywhere. * artifact_lifecycle._compact_text makes one policy-aware call instead of OR-ing find_private_text_match with SECRET_LIKE_SURFACE_PATTERN. The named policy reproduces this projection's historical verdict exactly: every category except a raw remote location, which it has always let through. Path-gap recognition, opt-in The classifier can now recognize the local-path shapes the legacy surface pattern misses: a home-relative ~/ path and a path:-prefixed local reference. Recognition is behind include_path_gaps and defaults to False, so this change tightens no surface. Choosing which surfaces enforce it is the disclosed behavior change tracked separately. ml_experiment's leading "/" or "~" rule stays a per-field alias constraint, not a second copy of the local-path decision: it rejects values the shared classifier does not flag even with path gaps on, so folding it in would lose coverage. Behavior preservation and evidence * Semantic inventory counts are unchanged (drift smoke reports identical same_runtime_forks / conflicting_values / twins on base and head). * A new test drives the shared corpus and asserts that whenever find_private_text_match returns a pattern, classify_private_text's first match is that same object. * The credential-shape and remote-location owner guards now name public_safe_text.py as the single owner; the literal scan still finds exactly one declaring file for each shape. * find_private_text_match, the four text owners and the TypeScript owner are untouched, so the Python/TypeScript corpus parity test still passes. No surface newly accepts or newly rejects a value in this commit. Signed-off-by: karenchuu <25980598+karenchuu@users.noreply.github.com>
…s too A mutation check on the consolidated classifier showed the new tests still passed when classify_private_text ignored its categories argument on the text-pattern path: neither existing policy excludes a category that a text pattern owns, so the filter was untested there. Add the missing case. Each value is matched only by a text pattern, so a policy that drops that pattern's category must accept it, and an empty policy must recognize nothing. With this row the no-op-filter mutation fails. Signed-off-by: karenchuu <25980598+karenchuu@users.noreply.github.com>
The two red architecture tests are main-side, not from this headHead
Both fail on unmodified
The full assertion text is the same single item in both trees:
Nine open PRs currently modify those two manifest files (#5106 #5130 #5240 #5242 #5247 #5248 #5249 #5250 #5251), so the regeneration looks to be in flight; I am not claiming which one lands it. No change pushed to this head. Both local runs used the repo's own strict venv, Node 22.23.2 on |
huangruiteng
left a comment
There was a problem hiding this comment.
APPROVE — 当前 exact head 未发现阻断回归;这是一份保留现有字段策略的 owner consolidation,不是 #5136 全部隐私政策的完成声明。主干已有的 census 失败另列为合并 hold,不作为本 PR 的 Request Changes 理由。
精确 head:01c0e3d6c217ba039adb69e76a67bb19f25cb9fe。全 PR 三点差异为六文件、+555/-58;实际分叉基线为 f679911568eee2b3c3cfa1a6ade43dcb17cfb06e,当前主干为 6643f367064b9921c864db75a042979fddc4b8c3。本人检查实现、未改动的调用者及真实公开入口,没有查询或等待 GitHub CI。
动机
按 #5136 的维护者验收说明,公共文本检查需要一个分类 owner,同时保留不同字段的明确策略。原来 shape regex 留在 runtime 模块,artifact lifecycle 另行拼接这些检查;下一次规则修复容易遗漏消费方。本次消除这个维护缺口,已有用户的 status 标签、错误结果和后续推进不应改变。更严格的普通单词、相对路径及公开导出政策仍属于既有 issue 的后续验收。
改动思路
扩展既有 public_safe_text,而不是增加能力或第二套权限引擎。shape detector 移到同一 owner,runtime 显式 re-export 原符号,既有文本校验仍使用原顺序。artifact 消费方只选择 credential、local-path、org-marker 类,继续允许它历史上允许的公开 URL;分类器不能反过来替字段决定“一切匹配都拒绝”。Python 放置符合现有文本 owner 的边界,TypeScript 既有 owner 和共享语料保持不变,不以本次重构为由扩大迁移。
具体改动
关键代码讲解
public_safe_text.py:265 / classify_private_text先按显式 categories 筛选既有有序规则,再检查三种 shape。相对/带前缀路径的新识别默认关闭;当前生产调用没有开启它,因此 helper 的可用性不等于改变默认拒绝政策。public_safe_text.py:228 / PrivateTextMatch是冻结的分类、原因、pattern 结果,不持久化新状态,也不赋予执行权限。find_private_text_match仍返回旧 regex 对象并保留原匹配次序,原来的调用者无需换接口。artifact_lifecycle.py:57 / _compact_text从三个独立 shape 检查改为一次显式字段策略调用;先检查完整值,再截断展示文本,避免长字符串尾部的私有形状在截断后消失。调用链仍为collect_status → attach_goal_artifact_lifecycle_projections → build_goal_artifact_lifecycle_projection,无文件状态写入。runtime/public_safety.py的显式同名 re-export 保留现有导入面。检查反馈、authority、boundary 与 TS vision 的未改动消费者,未新增一个平行的泛化状态规则 owner。
正例中公开 URL 和普通授权说明保留原输出;负例中本地路径、credential shape、长文本尾部匹配仍被拒绝或替换为原有安全标签。复验也纠正了一个描述细节:包含既有 home-directory 形状的 file URL 在 base 已被过滤,不是本 PR 新增拒绝。已存在的裸敏感单词误报不被本次重构宣称修复。
对主干的风险
326 项目标 Python 测试、TS typecheck、TS 共享语料两项测试、配置指定的十九文件 strict mypy、改动文件 ruff 均通过。风险分层 canary 的五项直接检查和十六项选中检查全部通过。首次 canary 尝试的报告落盘遇到 ENOSPC,未计为通过;环境恢复后的完整同命令重跑有独立结果。
我另写独立 oracle,使用真实文件后端的 collect_status 及四个现有公共文本消费方:十四项 status、七十六项共享语料、七项递归结构,共九十七项 base/head 完整结果一致;boundary builder 的时间由输入显式固定,未删除错误、状态或策略字段。fixture SHA-256 为 1fd288ad28a765fe2a29d3b1c6099fbc9149085358626dcdd87484c36de3e1b4,完整 observation SHA-256 为 b93ed8cc8c84ca0647218591a27a4333018abb9f655f0101efc3e5a1b491886c。强制扩大 URL 拒绝策略、或强制开启相对路径新规则,各使两个真实 status 反例失败,说明 oracle 能捕获这两种回归。测试使用合成状态,未操作活动 Goal。
语义与 CI 对齐
复用已有公共文本/字段策略,不改变 Goal、Todo、lease、quota 或 settlement 语义。没有把目录级展示策略当成授权,也没有把机器拒绝称为“指导”。现有 regex 的字符串启发式仍有误报边界,后续应在 #5136 的既有 owner 中改成明确、经语料验证的政策,而非暗中放宽。
本 PR 的源 head 上,两份 architecture inventory 文件九项测试通过。当前主干和“主干+本 PR”的临时合并树则都出现相同两项 census 失败,细节均为 contract.py 的 codec_read:load_registry#1 元数据不匹配;这条路径不在六文件差异中。比较的是相同命令、失败身份和细节,不仅是数量。该主干整合问题保留在严格质量/合并门槛中,代码 review 仍为 APPROVE。
我的整体评价
这份有限增量有实际维护价值,long_horizon 改善规则定位与后续修复,user_experience 经真实入口保持;全 issue 的更严格隐私验收尚未关闭。默认关闭兼容性、字段间的有意差异以及无副作用读回都有证据,未发现需要本次阻断的语义漂移。
未来向前的 bounded refine 已采用“一个 detector owner+显式字段策略”。非阻断建议:matches_private_text_policy 目前只有测试消费者,可等首个真实调用者再保留这层布尔包装;category 字符串可在同边界进一步收窄为 Literal,而无需新增框架。本轮只 review,不修源码、不合并、不升级;严格质量/整合 hold 不被 review 批准抹除。
English verdict: APPROVE - 01c0e3d. The existing text owner is consolidated with explicit field policies and independently verified public-entrypoint parity. No blocking PR regression was found. Identical current-main/integration census failures remain separate quality and merge holds; this is not full #5136 closeout.
|
对
结论 APPROVE;当前许可只覆盖审查与公开反馈,不执行 merge、source repair 或 runtime 升级。 |
tests/control_plane/test_public_safe_text_classifier.py (loopx-project#5245) carried the literals for a local filesystem path and an internal ext_data path, which the repository public/private boundary scanner flags. Join the segments at runtime, matching the file's existing credential-fixture discipline, so the classifier still sees the same text while repository-hygiene-smoke and `loopx check --scan-path .` pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: song <22676124+songoow@users.noreply.github.com>
What this does
Implements direction 1 of #5136:
loopx/public_safe_text.pybecomes the single home for the decision "does this string look private?", andcontrol_plane/runtime/public_safety.pyconsumes it instead of owning a competing shape set.This is the behavior-preserving slice. It relocates ownership, gives the classifier explicit categories and reasons, and closes the path-recognition gap in a way that enforces nothing yet. The behavior changes you flagged are deliberately not here.
1. Shapes have one owner now
SECRET_LIKE_SURFACE_PATTERN,LOCAL_PATH_SURFACE_PATTERNandREMOTE_LOCATION_SURFACE_PATTERNare defined once inpublic_safe_text.py, byte-identical to whatpublic_safety.pycompiled before.public_safety.pyimports them in the redundant-alias form (house style under--no-implicit-reexport) so its ~8 direct importers and 30+ recursive-validation callers keep the same objects.I had to make that re-export explicit because
--no-implicit-reexportrejects a plainfrom … import NAME: bare mypy flaggedperiodic_report/core.pyimportingSECRET_LIKE_SURFACE_PATTERNfrompublic_safety.2. Detection returns a category and a reason
The text-owner rule set is now a list of
pattern + category + reasonentries, andclassify_private_textreturns one of four categories (credential,local_path,remote_location,org_marker) with a stable reason. A caller names the policy — a set of categories — that decides what its own surface rejects, which is the separation you asked for in direction 2: recognizing a value never implies publishing it is forbidden.PRIVATE_TEXT_PATTERNSandfind_private_text_matchare derived from the categorized list in the same order, sofeedback,authority,boundary_authorityand the TypeScript Vision checkpoint keep their exact verdicts.3.
artifact_lifecyclemakes one policy-aware call_compact_textused to OR two independent detectors:Now it names one policy:
ARTIFACT_LIFECYCLE_CATEGORIESiscredential ∪ local_path ∪ org_marker— every category exceptremote_location, because this projection has always let an ordinaryhttp(s)URL through. Two reasons this stays behavior-identical:remote_location, so the classifier's text pass is exactlyfind_private_text_match._compact_textcallsvalidate_public_safe_valuefirst, and that already raises on anyLOCAL_PATH_SURFACE_PATTERNorSECRET_LIKE_SURFACE_PATTERNmatch — so the shape checks in the classifier are provably redundant, not new vetoes.4. Path-gap recognition is added but enforced nowhere
The classifier can now recognize the two local-path shapes the legacy surface pattern misses: a home-relative
~/…path and apath:-prefixed local reference. This is behindinclude_path_gaps, which defaults to False, so no existing surface tightens in this PR.5.
ml_experimentkeeps its own, stricter ruleIts
text.startswith(("/", "~"))check is a per-field alias constraint, not a second copy of the local-path decision, so I left it alone and pinned it with a test:~username/notesand/just-a-leading-slashare rejected byml_experimentbut are not flagged by the shared classifier even with path gaps on. Folding it into the owner would silently lose that coverage.Relationship to #5135 and #5196
Both are merged and both are in this branch's base, and this PR deliberately moves the landing spot they chose. #5135 folded the credential shapes into
public_safety.py; #5196 put the raw-remote-location shape there for the same reason. Direction 1 then saysruntime/public_safetyshould consume thepublic_safe_textcontract "rather than own a competing set of text shapes", so this moves those definitions one level up into the shared home and leavespublic_safetyimporting them.That includes editing the two owner guards those PRs added (
test_public_safety_credential_shape_owner.py,test_remote_location_shape_owner.py), which pinned the declaring module aspublic_safety.py. I only repointed them after re-running their own literal scan and confirming the invariant they protect still holds: each relocated shape is declared in exactly one module, nowpublic_safe_text.py, andpublic_safety.pydeclares none of them. Nothing from #5135 or #5196 is reverted — the per-site thresholds, the per-site error messages and the anchored whole-value provider-token rule inhistory_export.pyall stay exactly where those PRs left them.file://is a local path, and this slice does not act on it yetI first raised this as an open question and asked the owner. Direction 3 already settles it: the shared classifier should recognize "local paths behind
file://orpath:prefixes" and "Public-safe output should reject/redact those local references." So the answer is local path, and a public projection should stop carrying it. I am not waiting on that decision.It is still not in this PR, because today three things the direction assumes are not true, and changing them is a behavior change rather than a relocation:
LOCAL_PATH_SURFACE_PATTERNdeliberately excludes a preceding:or/, sofile:///Users/…andpath:/Users/…are not rejected by public-safe output today — they only failed to surface because the separate raw-location shape rejected anyfile://URL as a remote location.artifact_lifecyclehas always let an ordinaryhttp(s)URL through, so treatingfile://as a local path has to be stated and tested against that surface instead of arriving as a side effect. That is whyARTIFACT_LIFECYCLE_CATEGORIESkeeps excludingremote_locationhere: it reproduces the shipped verdict exactly.~/andpath:recognizers added above are behindinclude_path_gaps, default off, so they enforce nothing yet.The follow-up lands the tightening with a per-surface newly-rejected list, since it moves four shipped entry points at once.
Evidence that nothing changed in behavior
same_runtime_forks=12/12, drift smokeok)public_safe_text.py)mypy(repo strict config)Success: no issues found in 19 source filesruff checkon changed filesAll checks passed!canary premergeon the changed filesself_merge_validation_passed: true, catalog canaries 8/8, risk-profile smokes 8/8,manual_holds: 0tests/architecture+tests/control_planeMutation results
Because you noted a source-level duplication guard can only supplement, not prove
semantic correctness, I mutated the new logic and checked the tests react:
classify_private_textignores itscategoriesargumentinclude_path_gapsdefaults toTrueartifact_lifecyclepolicy widened to reject ordinary URLsartifact_lifecyclestops consulting the classifierpublic_safetyre-grows a competing scheme-list copypublic_safetyre-grows a competing credential-shape copyremote_locationshape detectorThe first mutation run left one mutant alive: dropping the category filter on the
text-pattern path kept all tests green, because neither existing policy
excludes a category that a text pattern owns. That is a real hole in the evidence,
so the branch adds a test where each value is matched only by a text pattern, which
makes a policy that drops that category have to accept it and an empty policy have to
recognize nothing. With it, all seven mutants are killed.
Test plan
All runs use Node 22.23.2 on
PATH(the effect-runtime tests are otherwise unavailable).pytest tests/control_plane/test_public_safe_text_classifier.py→25 passed— new: category/reason per shape, the two named policies, the policy filter applied to the text-owner patterns, opt-in path gaps,matches_private_text_policy, the corpus-driven "classifier's first text match is the same objectfind_private_text_matchreturns",PRIVATE_TEXT_PATTERNSorder/count, re-export identity, and theml_experimentalias constraint.pytest tests/control_plane/test_public_safe_text_owner_parity.py tests/control_plane/test_public_safety_{credential_shape_owner,path_shapes,field_name_spelling,text_budget}.py tests/control_plane/test_remote_location_shape_owner.py tests/control_plane/test_public_safe_decision_replay.py tests/control_plane/test_goal_{artifact_work,acceptance}_observation.py→301 passednode --no-warnings --experimental-sqlite --experimental-strip-types --test tests/control_plane_ts/public_safe_text_corpus.test.ts→pass 2 / fail 0(TypeScript owner unchanged)python examples/semantic-vocabulary-drift-smoke.py→ok, counts identical to baseloopx canary premerge→ greenpytest tests/capabilities tests/extensions→2802 passed, 9 failed; those 9 are all intest_repository_change_window.py(git-hook/worktree/ssh fixtures) and fail identically on an unmodified checkout of the same base commit, so they are environmental here, not regressions.pytest tests/architecture tests/control_plane tests/test_ml_experiment_volc_packet.py→18 failed, 6184 passed, 5 skippedin 38m53s, and all 18 failures are in one file,tests/control_plane/test_scheduler_compat_state_key.py. That file is unrelated to this change (git-hook/CLI transport fixtures) and fails identically — 18 failed — on an unmodified checkout of the same base commit under the same PATH, which I ran side by side. So this change adds no regression there.Deliberately not in this PR
token=x) — a behavior change, so it belongs in its own PR.Refs #5136