fix(quota): replace raw exception strings with public-safe values - #2846
fix(quota): replace raw exception strings with public-safe values#2846rootkiller6788 wants to merge 2 commits into
Conversation
098f347 to
dce9fb9
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
结论:Request changes。方向正确,精确 head dce9fb98c8c07de7d2b3695256357463099e7194 已消除我构造的异常路径泄漏;但当前实现让最关键的 quota should-run / monitor 失败分支没有稳定 error_code,同时删除了非事件命令既有的 error 字段,形成机器可判定性和兼容性缺口。建议在合并前做一次小而完整的契约修复并补回归测试。
动机
当前 _quota_failure_payload 把捕获到的 Exception 直接转成 str(error),而 FileNotFoundError、锁失败或配置解析异常可能包含本机路径和内部状态。quota 输出又会进入 heartbeat、状态视图和自动化日志,所以这里确实属于公开边界,不能继续透传原始异常。这个 PR 要把失败结果收敛为稳定、公开安全、可供人和机器消费的值,必要性成立。
改动思路
PR 在 loopx/cli_commands/quota.py::_quota_failure_payload 的两个分支里分别替换三处原始异常文本:非 quota-event 分支把 error 改成 error_code=QUOTA_COLLECTION_FAILED;quota-event 分支把 reason 改成固定描述;两边都把 recommended_action 改成固定操作提示。现有 lock_timeout_error_fields(error) 继续在字典尾部展开,因此锁超时时仍可提供更具体的公开安全字段和 operator action。
更稳妥的契约应是:所有失败分支都保留稳定分类码;非事件分支保留既有 error 键但只写公开安全的固定消息,以避免 JSON 消费者因字段消失而破坏;锁超时仍允许用更具体的类型码覆盖通用码。这样既完成脱敏,也不把“脱敏”误变成一次未声明的输出 schema 删除。
具体改动
本 PR 只改 1 个文件,5 行新增、3 行删除,没有测试和状态检查。
关键代码讲解
-
loopx/cli_commands/quota.py:497的_quota_failure_payload是handle_quota_command在任意异常时的统一失败出口。输入包括命令、registry/runtime 参数和原始异常;输出会直接交给 JSON/Markdown renderer,并以ok=False决定进程退出码。这里是公开边界,不能携带原始异常字符串。 -
loopx/cli_commands/quota.py:506-533的非事件分支当前返回:
{"ok": False, "error_code": "QUOTA_COLLECTION_FAILED", ...}它成功阻止了异常路径进入 payload,但同时删除了旧有 error 键。最小兼容修复是同时返回公开安全的 error(例如 "quota collection failed")和稳定 error_code,而不是以新键替换旧键。
loopx/cli_commands/quota.py:535-550的 quota-event 分支覆盖should-run、monitor-poll、scheduler 和 spend/void。当前只写固定reason,没有写error_code。我用包含/private/internal/secret.json的FileNotFoundError直接执行该函数:status得到QUOTA_COLLECTION_FAILED,但should-run与monitor-poll的error_code均为null。因此最常用的自动化入口反而无法按稳定码分类失败。
正向示例应是:status 收集失败 -> payload 同时包含公开安全 error 与 error_code -> renderer/旧消费者继续工作;负向示例应是:异常包含本机路径 -> JSON 和 Markdown 中均找不到该路径,should-run 仍能按 QUOTA_COLLECTION_FAILED 判定失败类型。
对主干的风险
[P1] 失败契约在命令间不一致,且删除既有字段。 should-run 是 heartbeat 的前置门,当前改动后它只有模糊 reason、没有稳定 error_code;status/plan 则有 error_code、没有旧 error。这会让调用方必须按命令写两套异常分支,也可能让依赖 error 的旧 JSON 消费者把失败误判为“无错误详情”。最小修复:两类分支都携带通用 error_code;非事件分支保留公开安全的 error;增加至少 status + should-run 两个回归测试,断言原始路径不出现、稳定字段存在,并覆盖 Markdown 输出。
此外,PR 基于较早主干;当前 origin/main 已在同一文件合入 #2857。Git 显示可合并,但修复时应先更新主干,避免新增测试基于旧布局。
我的整体评价
方向和修改位置都对,作用域也很小。我检查了完整 patch、调用/renderer 路径和当前 origin/main,运行了 py_compile、tests/presentation/test_quota_markdown_boundary.py(3 passed),并用三种命令直接构造带私有路径的异常:现 head 没有泄漏,但 should-run/monitor-poll 缺失 error_code。补齐上述契约和 focused regression 后,这个 PR 就具备合并条件;请推送新 head 后再复审。
huangruiteng
left a comment
There was a problem hiding this comment.
Review: REQUEST_CHANGES
head dce9fb98c8c07de7d2b3695256357463099e7194。这是对 fix(quota): replace raw exception strings with public-safe values 的完整 PR 解读,覆盖 1 个文件、3 个连续 commit 和当前 main 的差异。方向正确,但当前 head 基于旧 main,且会把 main 已有的 typed error code 覆盖掉,必须 rebase 后再审。
动机
_quota_failure_payload 在旧 main 上会把 str(error) 放进 error、recommended_action 和 reason,可能泄露本地文件系统路径或内部状态。目标是把异常文本换成稳定、public-safe 的诊断值。这个目标是对的。
改动思路
PR 用三个 commit 分别处理:
error->error_code: "QUOTA_COLLECTION_FAILED";recommended_action-> 静态 operator guidance;reason->"quota collection failed"。
思路是“保留字段名,替换值为稳定 token”,避免破坏 renderer 和调用方。
具体改动
loopx/cli_commands/quota.py:_quota_failure_payload的三个字段从str(error)改为静态/稳定值。
关键代码讲解
error_code:PR 使用硬编码QUOTA_COLLECTION_FAILED。但当前origin/main已有"error_code": quota_error_code(error),来自loopx/control_plane/quota/error_codes.py。rebase 后如果直接采用 PR 的硬编码,会丢掉 typed exception classification,属于回归。recommended_action:当前 main 已经是静态"fix quota/status collection before spending automatic compute",PR 这条改动在 main 上已存在。reason:这是当前 main 仍然泄漏str(error)的字段,也是 PR 唯一仍需要的改动。
正向路径
如果 rebase 到当前 main,只保留 reason 的脱敏,并继续使用 quota_error_code(error) 作为 error_code,最终 payload 会是:typed error_code + 静态 recommended_action + 静态 reason,没有原始异常文本。
负向路径
当前 head 是 CONFLICTING,且没有 GitHub checks。直接合并会把旧分支的三个 commit 带进 main,其中硬编码 QUOTA_COLLECTION_FAILED 会替换掉 main 已经采用的 quota_error_code(error),降低故障可诊断性;前两个 commit 也大概率与 main 现有实现冲突。
对主干的风险
P1:
- 分支落后且冲突。
mergeStateStatus=DIRTY/mergeable=CONFLICTING,statusCheckRollup 为空。必须先 rebase 到最新 main、解决冲突并让 build/pytest 全绿。 - 不要覆盖 typed error code。 当前 main 已经用
quota_error_code(error)输出稳定 error_code;PR 的硬编码QUOTA_COLLECTION_FAILED会让所有异常都折叠成同一个粗粒度 token,丢失FileNotFoundError、permission、timeout 等分类。rebase 后应保留quota_error_code(error)。 - 前两条改动已过时。
recommended_action静态值已经在 main 上;真正剩余待修的是reason字段。
我的整体评价
目标正确,但当前 head 不能合并:它基于旧 main,会破坏已有的 typed error code,且没有 checks。最小修复是把分支 rebase 到最新 main,只保留 reason 脱敏(以及任何 main 尚未覆盖的字段),保留 quota_error_code(error),再跑完整 pytest 和 import-boundary。
English Verdict
Request changes. Head dce9fb98c8c07de7d2b3695256357463099e7194. The sanitization goal is correct, but the branch is based on old main, is conflicting, and has no checks. Current main already provides typed error_code via quota_error_code(error) and a static recommended_action; this PR must rebase, preserve the typed error code, and keep only the reason sanitization that main still needs. Full pytest and checks must be green before merge.
Latest main already provides typed error_code via quota_error_code(error) and static recommended_action. Three str(error) references remained: - Non-event branch: "error" field now uses static description - Non-event branch: health_items "recommended_action" now uses static guidance - Event branch: "reason" field now uses static description All three preserve existing field names for renderer compatibility.
dce9fb9 to
b550a80
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
PR #2846 新 head 复核(head b550a805e21c2e1350a4bf65ee38c91bd905ef54)
详细中文评审
动机(本次 head 更新)
上一轮 REQUEST_CHANGES 指出 _quota_failure_payload 的 error/recommended_action/reason 三个字段直接暴露 str(error),FileNotFoundError 等异常可能把内部文件系统路径泄漏进 CLI 输出。作者这次把三个字段都换成稳定的公开安全值(error → "quota collection failed"、recommended_action → 静态指引、reason → "quota collection failed"),方向正确,但实现破坏了既有的参数诊断契约。
改动思路
单文件 loopx/cli_commands/quota.py(+5/-3):把 _quota_failure_payload 中三个泄漏字段统一替换为静态公开安全字符串,不再使用 str(error)。动机对,但替换粒度过粗。
具体改动
_quota_failure_payload:
error: str(error)→"quota collection failed"recommended_action: str(error)→"fix quota/status collection before spending automatic compute"reason: str(error)→"quota collection failed"
对主干的风险
- P1(阻塞):CI pytest 失败(
tests/test_cli_argument_diagnostics.py3 项)。_quota_failure_payload同时服务于参数诊断错误与配额收集失败两类路径。参数诊断场景(如quota monitor-poll --include-detail scheduler这种不支持的组合)的既有契约要求reason == "quota {command}does not accept --include-detail {section}";新代码把所有失败一律写成通用"quota collection failed",导致:test_quota_include_detail_rejects_non_should_run_commandtest_quota_include_detail_rejects_other_command_sections[should-run-decisions]test_quota_include_detail_rejects_other_command_sections[monitor-poll-scheduler]
全部失败(本地复现:3 failed / 80 passed)。修复方向:把"参数诊断失败"与"配额收集失败"分开——诊断路径保留具体的公开安全 reason(这些消息本身不包含str(error),没有泄漏风险),仅对收集失败路径做通用脱敏;或给_quota_failure_payload增加一个公开安全reason参数,由调用方按失败类型传入精确文案。
- P2:PR 模板卫生——body 中
Closes #为空、验证复选框未勾选;补上py_compile/loopx check的实际执行结果更利于合入前确认。 - 除上述外,脱敏方向正确:错误码已由
quota_error_code(error)提供稳定分类,静态指引也是合适的。
我的整体评价
方向正确(消灭 str(error) 泄漏),但通用化过度:参数诊断路径有既有的、可区分的公开安全 reason 契约,不应被"配额收集失败"一刀切覆盖。这是本 PR 自己引入的 CI 回归,需要先修这个再合入。结论 REQUEST_CHANGES(单点、聚焦)。
验证
- exact head
b550a805e21c2e1350a4bf65ee38c91bd905ef54(复核时 head 未变):tests/test_cli_argument_diagnostics.py:3 failed / 80 passed(复现 CI)- CI:pytest FAILURE(同上)、dependency-review/build SUCCESS
- 未独立验证:无其他声称的验证项(PR body 验证区未勾选)。
Review: REQUEST_CHANGES
Head: b550a805e21c2e1350a4bf65ee38c91bd905ef54
Verdict: REQUEST_CHANGES — the sanitization direction is correct (removing str(error) from error/recommended_action/reason), but the generic "quota collection failed" reason overwrites the existing public-safe argument-diagnostic contract, breaking 3 CI tests (test_cli_argument_diagnostics.py): e.g. quota <cmd> --include-detail <section> must keep its exact public-safe reason. Separate argument-diagnostic failures (specific public-safe reason) from collection failures (generic sanitized reason), or pass the public-safe reason into _quota_failure_payload per failure type.
Key finding: The change fixes a real leak but regresses the CLI diagnostic reason contract; CI is red on this head.
Validation: reproduced locally (3 failed / 80 passed); CI pytest FAILURE, dependency-review/build SUCCESS.
…ta collection failure Validation ValueErrors raised by _prepare_quota_command_context carry bounded, public-safe messages (command names, section values). Route them through a dedicated ValueError handler that preserves the message, rather than letting the broad except-Exception handler mask them with the static _quota_failure_payload text.
huangruiteng
left a comment
There was a problem hiding this comment.
PR #2846 新 head 复核(head cb945ae5073b00022b9f512bb3ffec06b77a2186)
详细中文评审
动机(本次 head 更新)
上一轮 REQUEST_CHANGES 指出:_quota_failure_payload 把参数诊断失败与配额收集失败统一写成 "quota collection failed",破坏 quota <cmd> --include-detail <section> 既有公开安全 reason 契约,导致 tests/test_cli_argument_diagnostics.py 3 项失败。作者本次新增 except ValueError 分支(+38 行,仍只改 loopx/cli_commands/quota.py),把参数校验类失败与收集失败分开处理。
改动思路
新分支直接承接 _prepare_quota_command_context / _scheduler_execution_context_from_args 抛出的 ValueError:非事件命令返回 error_code=QUOTA_VALIDATION_FAILED 并保留具体公开安全 error;事件命令(should-run/monitor-poll/scheduler/spend/void)返回 reason=str(exc)、status=quota_validation_failed、state=blocked_validation。通用 Exception 仍走 _quota_failure_payload 的稳定脱敏出口。这样"诊断失败给精确文案、收集失败给通用脱敏"的边界清晰。
关键代码讲解
quota.py:616新增except ValueError as exc位于通用except Exception之前,校验错误不再被脱敏覆盖。- 分支内按
command not in QUOTA_EVENT_KINDS分两类载荷:非事件保留error键(兼容旧消费者),事件命令提供reason+ 稳定error_code。 - 我核对了
quota.py与loopx/control_plane/quota/全部raise ValueError站点,消息均为静态或由受控参数生成(goal id 合法性、source 枚举、settlement identity 冲突等),不含本机路径或内部状态,str(exc)进入 payload 无泄漏风险。
对主干的风险
无阻塞项。上一轮 P1 已修复且 CI 全绿。两个非阻塞建议:
- P2:PR 模板卫生——
Closes #仍为空、Validation/Boundary 复选框未勾选;下次提交时补上并附实际验证输出。 - P2(可选):可为新
QUOTA_VALIDATION_FAILED契约补一条聚焦回归测试(断言事件/非事件两类载荷的稳定字段),现有测试已覆盖行为,但独立测试更抗未来漂移。
验证
- exact head
cb945ae5:py_compile通过。 - 本地测试:
test_cli_argument_diagnostics.py+test_quota_markdown_boundary.py86 passed(含此前失败的 3 项);test_actual_default_model_behavior_portfolio.py/test_cli_output_budget.py/test_turn_envelope.py60 passed;test_cli_entrypoint.py3 passed。 - CI:pytest SUCCESS、build SUCCESS、dependency-review SUCCESS。
我的整体评价
改动小而聚焦,精准解决了上一轮 P1:参数诊断失败恢复精确公开安全 reason,收集失败保留通用脱敏与稳定 error_code,未引入新的字段删除或泄漏面。达到合并条件,结论 APPROVE。
Review: APPROVE
Head: cb945ae5073b00022b9f512bb3ffec06b77a2186
Verdict: APPROVE — the new except ValueError branch cleanly separates argument-diagnostic failures (specific public-safe reason, QUOTA_VALIDATION_FAILED, blocked_validation) from quota collection failures (generic sanitized payload), resolving the previous blocking regression. All local ValueError raise sites inspected are bounded and public-safe; py_compile and 149 local tests pass, and CI pytest/build/dependency-review are all SUCCESS. Non-blocking follow-ups: fill the PR template (Closes #, validation checkboxes) and optionally add a focused regression test for the new validation payload contract.
huangruiteng
left a comment
There was a problem hiding this comment.
想了解为什么要防止错误信息泄露呢?是出于什么真实场景的目的么?我觉得应该透传出来呀
huangruiteng
left a comment
There was a problem hiding this comment.
详细中文评审(重审)
精确评审头: 2846@cb945ae5073b00022b9f512bb3ffec06b77a2186
动机
按全队列重审要求复审本 PR。它把 loopx quota 失败 payload 中的 str(error) 替换为公开安全的静态描述,并为校验类 ValueError 增加专用透传路径。owner 在 11:18Z 提出疑问:为什么要防止错误信息泄露?是否应该透传?
改动思路
两个 commit:commit 1 将非事件/事件分支的 error/reason/recommended_action 从原始 str(error) 改为静态公开文本,同时保留 quota_error_code(error) 类型化错误码与 lock-timeout 字段;commit 2 为 _prepare_quota_command_context 抛出的 ValueError 增加独立 handler,error 字段直接透传校验消息并使用 QUOTA_VALIDATION_FAILED。exact head 已核实两处改动均生效。
具体改动(关键内容讲解)
- commit 1(静态化通用失败):
_quota_failure_payload现在输出"quota collection failed"等静态文本。这在公开边界上是有依据的:原始str(error)可能携带内部路径/上下文;类型化error_code与recommended_action已保留可操作性。合理。 - commit 2(校验错误透传):
except ValueError分支把 bounded、public-safe 的校验消息直接放进error字段。这正面回应了 owner “应该透传”的诉求;校验类错误不再被通用失败文案掩盖。合理。 - remaining gap(blocking,非代码):owner 的 CHANGES_REQUESTED 要求作者说明真实场景与目的;当前 PR 无任何评论回复。作者需要在 PR 上回复 owner 的疑问,并明确通用失败路径下维护者的调试通道(例如原始错误仅进日志/verbose,而不进默认 payload),否则不应合并。
对主干的风险
风险低:改动限定在失败 payload 文案与 ValueError 路由,类型化错误码保留;渲染器字段名未变。风险主要是流程性的——owner 问题未答复即合并会破坏评审共识。
我的整体评价
REQUEST_CHANGES(与 owner 一致,但指出 commit 2 已解决大部分顾虑)。 修复要求:作者在 PR 上回复 owner 的“为何防止泄露/真实场景”问题,并补充维护者调试通道说明(原始错误进日志/verbose);若 owner 认可,可复审后放行。贡献行为警告按 PR #3134 政策适用。
English Verdict
REQUEST_CHANGES — exact head cb945ae5073b00022b9f512bb3ffec06b77a2186.
The code is sound: generic failure payloads use public-safe static text with typed error_code preserved, and commit 2 already routes validation ValueErrors through a dedicated handler that surfaces the message directly (addressing the owner's passthrough concern). The blocking item is process: the author has not replied to the owner's CHANGES_REQUESTED question or documented the maintainer debug channel for generic failures. Reply on the PR and clarify the debug path, then re-review.
_quota_failure_payload exposed str(error) in three fields (error, recommended_action, reason), which can leak internal filesystem paths from exceptions like FileNotFoundError.
Replaced with stable public-safe values:
Summary
Issue Or Task
Validation
python3 -m py_compile loopx/*.pyloopx check --scan-root .Boundary Checklist
.loopx/,.codex/goals/, liveACTIVE_GOAL_STATE.md, credentials, private benchmark traces, verifier output, raw agent sessions, internal document links, or local machine paths.