refactor(public-safety): decide credential shapes in one owner - #5135
huangruiteng merged 2 commits into
Conversation
Seven surfaces each compiled their own credential test and no two agreed: the owner behind the public-output boundary recognized a GitHub token or a JWT while decision_context, material_lifecycle, periodic_report, extensions.presentation and artifact_lifecycle did not, and two of those five were byte-identical copies. artifact_lifecycle said the gap out loud in its own comment and patched the caller instead of the owner. SECRET_LIKE_SURFACE_PATTERN now owns the shapes, widened to the union every site already guarded. periodic_report and artifact_lifecycle drop their copies; the other three consult the owner and keep only their local threshold policy. A guard test pins the wiring at each site and refuses a new surface copy. Signed-off-by: JunZ-Leo <100498253+JunZ-Leo@users.noreply.github.com>
…oth owners The projection refuses a value either owner calls private, so its rows fail in two directions: dropping the credential-shape owner hides a provider token, and dropping the private-text corpus hides a bare word. A positive control row keeps a surface that refuses everything from passing. Signed-off-by: JunZ-Leo <100498253+JunZ-Leo@users.noreply.github.com>
|
Validation is now complete on Three things a reviewer should not have to rediscover:
#5136 is filed for the larger finding this change deliberately leaves alone: |
huangruiteng
left a comment
There was a problem hiding this comment.
English verdict: APPROVE
Reviewed head: fb560e0f73a392ead5e2c3d8c87e4dd72b4d87fe;immutable base: b15413ffcb9d362e6b30c960a6be49359dab1471。无阻断项。这里统一的是 credential shape 判断,不是宣告所有 private-text 策略已统一。
动机
同一段 provider token 在公共 payload 边界被拒绝,却能通过 Decision Context、Material Lifecycle、Periodic Report 和 extension presentation 的局部检查。独立 base/head probe 在四个真实公开 builder/validator 上重现了这个差异:基线接受合成 GitHub token,当前 head 拒绝;正常输入六个入口的完整输出指纹保持一致。持续使用时,这种出口差异会让每个新 provider shape 都需要逐处修补,并使“public_safe”标签失去可组合性。
改动思路
保留 runtime public-safety 为 credential shape 的 owner,把 provider-prefix/assignment 规则收敛到它;各出口继续保留本来更严格的短值、词语、路径和 URL 策略。更小的方案是继续给每个出口复制前缀,但会延续同一个维护缺口。反过来直接删除 public_safe_text 并非等价替换:它仍约束 authority/feedback 的词语和私有位置,#5136 已明确记录两套语义的后续 owner 决策,本 PR 不应顺便改掉这些现有契约。
具体改动
完整差异为 7 个文件、+193/-20:6 个生产文件负责共享规则与接线,157 行测试覆盖形状并集、调用者拒绝和正常值,不引入状态表、开关、provider 或新权限。
关键代码讲解
runtime/public_safety.py::SECRET_LIKE_SURFACE_PATTERN:输入完整文本,覆盖既有 provider 前缀与赋值形状并集;GitHub 形状阈值从 20 收到 16,是实际拒绝范围扩大,不是纯代码搬移。decision_context/packets.py::_compact_text/material_lifecycle/_validation.py::compact_text:长度、路径和原有局部严格策略仍先后生效,再调用共享 shape owner;异常仍由所属 packet builder 返回。它们不授予建议执行或 material cutover 权限。periodic_report/core.py::_reject_raw_keys:递归检查完整请求,由同一 shape owner 决定 credential 值;run identity、sink receipt 与重试状态归一化未改动。artifact_lifecycle.py::_compact_text:先检查完整来源,再做显示压缩,同时保留 private-text 的更严格规则。拒绝后仍用原有安全 fallback,不把被过滤内容重新贴上 public-safe 标签。presentation 的实际 validator 同样接入共享规则。
对主干的风险
主要风险是无意删掉局部短值/词语规则,或扩大 shape 拒绝范围误伤正常标识。独立对照覆盖六个公开入口的正常输出,以及 short assignment、bare word、short bearer:base/head 的结果和错误保持一致;合成 GitHub token 的四个漏口已闭合。进程内移除四个调用者的共享规则会重新接受 token,因此反例确实能发现“owner 有规则、出口没有接线”的退化,不只是断言 regex 命中。
原生 public-safety 定向测试 240 项通过;相关 decision/material/report/extension/Goal projection 集成测试 122 项通过;标准 diff-selected premerge 的 16 个 smoke 和 5 个 direct check 全部通过。使用当前 checkout 的 Python 3.13,未继承作者较早 revision 或借用环境的测试结论;未查询或等待远端 CI。扫描与 diff whitespace 检查未见公开边界问题。
共享语义与边界
这是公开输出拒绝规则的明确加强,无 default-off 声明;不是 Goal/Todo 状态分类,regex 在这里用于 credential 数据形状而非靠文本猜状态。无新增协议权限、调度义务、持久化 schema 或前端设置;公开 UI 消费的是已校验 projection,现有成功 shape 不变,不需要 companion editor。形状检测不是完整 secret scanner,双 owner 的 private-text 哲学仍待 #5136 处理。
我的整体评价
APPROVE。改动交付了一个可观测的安全出口一致性结果,生产机制比逐处复制更小,兼容成本可控。未来改动的收敛检查已经应用在这次共享接线上;保留局部策略和 #5136 的独立 owner 决策是有依据的边界,不需要再加一套框架。一个非阻断的文案建议:PR 模板“Refactoring (no functional changes)”应取消或补充说明,因为新的 union 确实扩大了拒绝范围,正文已正确披露这一点。
Goal And Delivered Outcome
no two agreed. Measured on the base revision, the owner behind the busiest public-output
boundary (
control_plane.runtime.public_safety) rejected a GitHub token or a JWT, whiledecision_context,material_lifecycle,periodic_report,extensions.presentationandcontrol_plane.goals.artifact_lifecycleaccepted both — so the same value could be refused byone published surface and carried by another. Two of those five were a byte-for-byte copy of
each other (verified by comparing the compiled sources), and
artifact_lifecyclesaid theproblem out loud in its own comment: "Provider token shapes the shared private-text rules do not
cover. … a leaked token there must never reach a public projection just because the shared corpus
did not list its prefix." It patched the caller instead of the owner, which is how the next
caller ends up with its own copy.
SECRET_LIKE_SURFACE_PATTERNis now the single owner of credential shapes, widened to the union of shapes every site
already guarded (
password=/secret=,api_key:,access_token=/refresh_token=, PEM privatekey headers,
github_pat_,AKIA/ASIA,xox*,AIza,sk_live_/rk_test_,npm_,pypi-).periodic_reportandartifact_lifecyclelost their private copies entirely;decision_context,material_lifecycleandextensions.presentationconsult the owner and keeponly what is genuinely per-surface — how short a value still counts, and whether a bare word is
enough. The
unitandregression_parityrows below pin the wiring at each site, not justthe pattern, so a surface that quietly stops consulting the owner goes red.
fix(public-safety): reject non-string mapping field names #5109; this one is the value side, found while consolidating around it) and to [Architecture]: two modules both claim to own "private-looking text" #5136, which
records the larger finding this PR deliberately leaves alone: two modules both document
themselves as the canonical private-text owner, and no value is private in both. Intended base
main.Scope And Continuation
against re-forking it, and each site's local strictness is now visibly a policy rather than an
accidental copy. Deliberately not changed, with the reason:
supervisor_events._INLINE_SECRET_PATTERNanswers a different question (does this commandline look like it embeds a secret), and
history_export._SECRET_SHAPED_GOAL_ID_REis anchored to a whole path segment (is this goal identirely a provider token). Neither can reuse a surface scan, so the drift guard names the
second one as an allow-list entry instead of pretending it is clean.
extensions.presentationrejects a bearer value from 8characters while the owner requires 16; adopting one number for every surface is a product
judgement about how much prose should be refused, not a duplication fix. Each site keeps its
own number and the owner's shape set.
that each document themselves as the canonical private-text rule set —
loopx/public_safe_text.py("Canonical private-looking-text rules for public-safecontrol-plane fields", with a fixture that pins Python and TypeScript to one contract) and
control_plane/runtime/public_safety.py— and neither dominates the other. That one needs anowner decision about which philosophy wins (a bare-word corpus vs a shape corpus), so this PR
stays inside the shape dimension it can prove safe.
two-canonical-corpora decision named above.
Validation
fb560e0f7(supersedes10e450830, which predates the addedartifact_lifecyclerows; every row below was produced onfb560e0f7)unitpython -m pytest -q tests/control_plane/test_public_safety_credential_shape_owner.py— 106 passed; together with the four existing public-safety files (…field_name_spelling,…path_shapes,…text_budget,…public_safe_text_owner_parity) — 240 passed / 0 failed. The corpus is 11 synthetic credential shapes built from filler characters (no live token); each shape is asserted against the owner and against every surface's own entry point, so the wiring is pinned per surface rather than only the pattern.integrationpython -m pytest -q tests/control_plane tests/capabilities tests/extensions tests/test_explore_presentation_views.py(35m03s) — 44 failed / 7,951 passed / 41 skipped. The same 44 node ids, compared name by name, fail in 53.6s on a pristine5c40583d8worktree with zero local changes, so the failure sets are identical and this change adds none. That bucket spawns theloopxexecutable or a packaged chat bundle, and the interpreter used here was borrowed from another worktree, so its console script resolves elsewhere.staticpython -m mypyrun as CI runs it (no arguments, thepyprojectfile list) →Success: no issues found in 20 source files.python -m ruff checkreports the same 606 findings over the same file set on untouched base and on this head, so nothing new is introduced (the local ruff build disagrees with CI's baseline, hence the comparison).git diff --checkclean.loopx canary premerge --from-git-diff --git-diff-base 5c40583d8→changed_files=8, every groupok: true,failures: 0. The first attempt reported one failure,examples/semantic-vocabulary-drift-smoke.py, which raised the identicalrepository npm dev dependencies are not installed … run npm ci --ignore-scriptson the untouched base too; after linking anode_moduleswhose dependency set is field-for-field identical to this tree's, the group passed.regression_parityperiodic_reportback on a private copy → 16 failed / 90 passed (alltest_periodic_report_rejects_a_shape_*rows).extensions.presentationstops consulting the owner → 12 failed / 94 passed (all…-extensions.presentationrows).artifact_lifecyclere-forks its private provider list → 1 failed, the structural guard only: the behavioural rows at that site stay green because that private list already covered the two shapes exercised there, so the source-level guard is what protects it and this row is not claimed as behavioural evidence. A brand-new module declaringgh[pousr]_for itself → 1 failed, the same guard, which is what shows the guard has teeth.than
uv sync --extra test, which is why every static claim is stated as a base/head comparison.a surface that stops consulting the owner goes red even if its own local list still looks
plausible. Not covered: the anchored whole-value goal-id rule in
history_exportand thecommand-line rule in
supervisor_events, named above as different decisions; and thepublic_safe_textcorpus, which this PR deliberately does not merge.Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
decision, in the same direction as refactor(authority): deliver outbox entries through one typed recovery owner #4968. No roadmap id claimed.
Shared-authority RFC fixture impact
envelope, provider arm or bridge path is touched.
Boundary Checklist
raw traces or verifier output, internal links, or local machine paths. Every credential in
this PR and its tests is a synthetic filler pattern, not a live token.
issue for it.
none.Signed-off-bytrailer (git commit -s).