fix(effect-runtime): validate the managed idle timeout before startup - #4349
huangruiteng merged 3 commits into
Conversation
`loopx/control_plane/effect_runtime_server.ts` parsed `LOOPX_EFFECT_RUNTIME_IDLE_MS` with a bare `Number()` and passed the result straight to `setTimeout()`. A non-numeric value became `NaN`, and zero or negative values had the same practical effect, so the managed server could close while the Python client was still waiting for readiness. The real configuration error was reported as `runtime_exited_before_ready`. Parse the setting as a base-10 integer bounded by `MIN_IDLE_MS` and `MAX_IDLE_MS`. The upper bound is `2 ** 31 - 1`, the largest delay `setTimeout` accepts before it overflows and fires immediately. An unset variable keeps the existing five-minute default; empty, non-numeric, zero, negative, fractional, out-of-range, and unsafe values are rejected before the server listens or publishes runtime information. A rejected startup now publishes one typed `loopx_effect_runtime_startup_error_v0` envelope on stderr and exits with status 2. `_start_runtime` captures the child's stderr in an unlinked temporary file and maps a valid envelope to its diagnostic code, so the caller sees `invalid_idle_timeout` with actionable guidance instead of a bare exit status. A rejected startup leaves no runtime-info file behind, and the valid lifecycle is unchanged. Tests cover the invalid values, the unset default, a valid short timeout, and the documented upper bound through the real managed Python-to-TypeScript startup boundary rather than a parser helper. Signed-off-by: yilin-succeed <204474593+yilin-succeed@users.noreply.github.com>
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
结论：REQUEST_CHANGES。当前 head afcc3c11fda2c7656719b9f10aa1e9498f2c171d 的解析与 typed startup envelope 方向正确，但真实 start-goal 入口丢掉了这次修复最重要的“可操作诊断”语义。
阻塞项（P1）：当 LOOPX_EFFECT_RUNTIME_IDLE_MS=0 时，Node 端会正确发出 invalid_idle_timeout，Python adapter 也保留了异常消息；然而 loopx start-goal --guided 在 loopx/cli_commands/start_goal.py 的既有错误投影中只保留错误码，把消息替换为固定的 “runtime is unavailable”，并因为 remediation map 没有新 code 而回退到 “run doctor / repair or reinstall”。我在当前 exact head 上走真实 CLI + 真实 Node runtime 复现到：退出码 1、code 正确，但输出完全没有指出应 unset/修正该环境变量及合法范围。底层 reason code 相等不等于 public remediation 语义完整；这会让一个一行配置修复被误导成重装排障。
最小修复：在现有 _EFFECT_RUNTIME_STARTUP_REMEDIATION_BY_CODE/failure payload owner 中为 invalid_idle_timeout 提供 domain-neutral、可执行的修复说明（明确变量名、十进制整数范围 1..2147483647，以及 unset 恢复默认值），或安全地保留 bounded typed startup message；同时新增真实 guided CLI 回归，不能只断言 adapter 层异常 code/message。
动机
修复的问题真实且有价值：旧的 Number() 会接受零、指数、十六进制等非预期形式，零值尤其可能退化为立即 shutdown，最终被观察成泛化的 early-exit。配置一旦存在，会阻塞该环境下所有新 managed runtime startup，值得 fail closed。
改动思路
把数值合法性放在 TypeScript runtime 这个决策 owner 内，用 bounded base-10 parser 在 listen/runtime-info 写入前拒绝；再通过 typed stderr envelope 让 Python 只做 adapter。这符合现有控制面边界，也优于 Python/TypeScript 各自维护一份规则。用 unlinked temporary file 捕获启动 stderr，可避免 pipe 填满死锁，也保留旧 untyped exit fallback。
具体改动
effect_runtime_server.ts：仅接受1..2147483647的十进制安全整数；非法值写出loopx_effect_runtime_startup_error_v0并以 2 退出。effect_runtime.py：启动时捕获 bounded stderr，识别 typed envelope 并抛出带 code/message 的EffectRuntimeStartupError。- 测试：当前分支
tests/control_plane/test_effect_runtime_integration.py43 passed，覆盖 unset、合法边界和 11 类非法输入；Python Ruff、control-plane TypeScript compile、git diff --check、DCO 均通过。 - 真实入口反例：
LOOPX_EFFECT_RUNTIME_IDLE_MS=0 ... loopx --format json start-goal --guided ...输出invalid_idle_timeout，但 remediation 是 doctor/reinstall，证明现有测试停在 public consumer 之前。
对主干的风险
正常 unset/合法值路径有充分正向覆盖；当前主要风险不是 parser，而是错误恢复路径。合入现状会让机器判定更准确、人工恢复反而更困难，并且 PR 描述所承诺的 typed configuration diagnostic 在最常见 public entrypoint 上并不完整。另一个非阻塞残余风险是本地未重跑 Windows temporary-file inheritance，但它不能解释上述跨平台 projection 缺陷。
我的整体评价
架构落点、范围和代码体量都合适，不需要另起抽象；相关 future-facing pass 也只需补齐既有 CLI remediation owner 与真实入口测试。修完这一处后可以快速复审。
English verdict: REQUEST_CHANGES — the typed runtime diagnostic is correct internally, but the real guided CLI discards its actionable message and recommends unrelated doctor/reinstall steps; preserve or map the invalid-idle-timeout remediation and cover the public entrypoint.
The typed `invalid_idle_timeout` diagnostic reached the CLI, but the guided entrypoint replaced its message with the generic "runtime is unavailable" and, because the code had no remediation entry, fell back to the doctor/reinstall advice. A one-line environment-variable fix therefore read as a broken installation. Add a domain-neutral remediation entry that names `LOOPX_EFFECT_RUNTIME_IDLE_MS`, states the accepted base-10 range 1..2147483647 milliseconds, and notes that unsetting it restores the default, so the public projection no longer recommends doctor or reinstall for this code. Cover it twice: the existing parametrized CLI projection test now asserts that this code omits the generic recovery advice, and a new regression drives the real guided entrypoint against the real managed Node runtime with `LOOPX_EFFECT_RUNTIME_IDLE_MS=0`. Signed-off-by: yilin-succeed <204474593+yilin-succeed@users.noreply.github.com>
|
Addressed in d1d2535 (new commit on top of the reviewed head, so the delta is a single-commit diff).
Validation at 修复点：为 |
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
结论：REQUEST_CHANGES。我复审了当前 exact head d1d2535545ba0578c7b577b860cd17e996940954 的完整 base-to-head diff，也单独检查了上一轮 afcc3c11f 到当前 head 的修复。上一轮指出的真实入口问题已经解决：LOOPX_EFFECT_RUNTIME_IDLE_MS=0 现在会通过真实 Node runtime 和 start-goal --guided 返回 invalid_idle_timeout，并明确变量名、合法范围与 unset 恢复默认。新的阻塞项不是当前文字写错，而是这份修复把同一条范围规则分别放进了 TypeScript 决策 owner 和 Python public remediation，留下了下一次细微语义漂移的必然入口。
阻塞项（P1）：effect_runtime_server.ts 的 MIN_IDLE_MS、MAX_IDLE_MS 与 idleTimeoutGuidance() 已经拥有合法性和完整诊断；effect_runtime.py::_startup_diagnostic 又把这条 typed message 规范化并限制为 240 字符后放进 EffectRuntimeStartupError。但是新加的 start_goal.py::_EFFECT_RUNTIME_STARTUP_REMEDIATION_BY_CODE["invalid_idle_timeout"] 没有复用这个 bounded message，而是再次硬编码 1..2147483647。如果以后 Node 的 timer 约束、最小值或允许语法调整，只改真正的 parser，CLI 就仍会告诉用户旧范围，甚至推荐一个 runtime 继续拒绝的值。当前测试也会漏掉这种漂移：一组测试断言 TS 当前范围，另一组测试断言 Python 当前字面量，两者今天一致不代表以后保持单一 owner。
最小修复：对已识别的 invalid_idle_timeout，让 public payload 使用 EffectRuntimeStartupError 中已经过 schema 校验、空白归一化和长度限制的 typed message，再只追加 CLI 自己拥有的 retry 文案；或者建立一个真正的生成式/共享 typed policy source。不要在 Python 中独立重述数值范围。增加一条回归：注入 invalid_idle_timeout 与一个刻意变化但仍 bounded 的 message，断言 recommended_action 保留该 message；同时保留现有真实 Node-to-guided-CLI 用例。
动机
这个 PR 解决的是一个真实且高影响的配置失败：旧代码用裸 Number() 解析环境变量，0、指数、十六进制及超出 setTimeout 上限的值都可能被接受，最后变成立即 shutdown 或错误 timer，而 Python 只能看到泛化的 runtime_exited_before_ready。一个进程级坏值会阻塞该环境中所有需要新启动 managed Effect runtime 的控制面调用，因此 fail closed 和可操作诊断都值得做。
我用同一合成项目走了 immutable baseline 9231d5b3 与当前 head 的真实 start-goal --guided。非法值 0 从“early exit + doctor/reinstall”变成“invalid_idle_timeout + 修正变量/范围/unset”；合法值 1000 和 unset 在归一化掉临时路径后仍保持成功契约。这证明本 PR 的用户价值和当前 observable change 都成立。
改动思路
总体边界是对的：
- TypeScript runtime 在 listen 与 runtime-info 写入前解析并拒绝非法值，保持 validity decision 在真正使用 timer 的 owner 内。
- 启动失败通过
loopx_effect_runtime_startup_error_v0写到 stderr；Python 用 unlinked temporary file 做 bounded capture，避免 pipe 背压，同时对旧的 untyped exit 保留兼容 fallback。 EffectRuntimeStartupError向上携带 typed code/message；guided CLI 再投影为用户可读 JSON/Markdown。
问题出在最后一步：presentation owner 可以决定“如何提示 retry”，但不应重新决定“什么值有效”。这里已有一条经过 trusted schema、8 KiB 读取上限和 240 字符归一化的 message 通道，所以第二份 Python range 不是必要的兼容边界，而是重复知识。
具体改动
effect_runtime_server.ts：新增 anchored decimal parser，只接受安全整数1..2147483647；unset 继续使用 300000 ms；非法值发出 typed envelope 并以 2 退出。effect_runtime.py：新增 bounded stderr 读取和 typed envelope 解码；匹配 schema/code 后保留规范化 message，其他 stderr 仍归入原有 early-exit fallback。start_goal.py：当前 head 为invalid_idle_timeout增加专用 remediation，解决了上一轮的错误恢复路径，但重复了 TS-owned range。- 测试：两个改动套件共 104 passed；Ruff 通过；control-plane TypeScript compile 通过；
git diff --check与两个 DCO trailer 均通过。GitHub 当前未为该 contributor branch 报告远端 checks，因此本结论依赖上述 repository-native 本地证据。 - 真实反例/正例：baseline/head 共六次 public probe 覆盖 invalid、valid 与 unset。现有测试对当前输出敏感，但还不对“只能有一个 range owner”敏感。
对主干的风险
当前 head 的即时行为风险已经显著降低：非法配置不会留下 runtime-info，正常路径没有观察到回归，typed stderr 也有旧格式 fallback。剩余阻塞风险是维护期的 silent semantic drift。它与这次问题的性质完全相同：机器判定与用户恢复提示分别维护，看起来 code 相同、测试都绿，但完整语义已经分叉。
Windows temporary-file handle 继承没有在本地复跑，是非阻塞残余风险；当前 POSIX 真实链路、TypeScript compile 和 focused integration 均通过。相比之下，重复 range owner 是确定存在的结构性问题，而且可以用很小的 adapter 修复消除。
我的整体评价
原始 parser、typed envelope、bounded temporary-file bridge 都是聚焦且合比例的实现，上一轮 public-entry 缺口也确实被修好。future-facing pass 的最高价值不是增加新框架，而是把新加的 Python 范围字面量删掉，让 TypeScript 继续是唯一规则 owner、Python 只做 bounded adaptation。完成这一个小修复并补上 message-preservation 回归后，可以快速复审。
English verdict: REQUEST_CHANGES for 4349@d1d2535545ba0578c7b577b860cd17e996940954 — the prior guided-CLI remediation bug is fixed and 104 focused tests pass, but the accepted idle-timeout range is now duplicated in TypeScript and Python. Reuse the already bounded typed startup message so a future parser change cannot leave public recovery guidance semantically stale.
The guided CLI re-hardcoded the accepted LOOPX_EFFECT_RUNTIME_IDLE_MS range (1..2147483647) in its remediation map while the TypeScript parser owns the same rules, so a future change to the Node-side bounds would leave the CLI advertising stale guidance. Drop the duplicated entry and reuse the schema-validated, length-bounded typed startup message carried by EffectRuntimeStartupError, appending only the CLI-owned retry instruction. The accepted range now exists in exactly one place: effect_runtime_server.ts. Cover it with a regression that feeds a deliberately varied bounded message through the projection and asserts it is preserved verbatim with the retry suffix, and update the real-runtime end-to-end test to assert the actual TS guidance text instead of the deleted Python copy. Signed-off-by: yilin-succeed <204474593+yilin-succeed@users.noreply.github.com>
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本次 exact head 5a9386231472dd3102865c4e2de01908cbbef913 解决的是托管 effect runtime 对 LOOPX_EFFECT_RUNTIME_IDLE_MS 配置错误时的可诊断性问题。此前无效值会让子进程在 ready 之前泛化退出，start-goal 只能给出“runtime exited before ready”一类信息，既没有说明允许范围，也没有说明收到的值。现在由实际负责 timer 的 TypeScript 启动边界统一解析，未设置时仍为 300000ms，只有有限的十进制安全整数 1..2147483647 被接受。无效值在 listen 和 runtime-info 写入之前以 code 2 退出，并通过既有 Python 启动适配和 CLI 投影展示明确原因。这样修复了 issue 所需的真实 Python→Node 启动路径，而没有在 Python 再造一个范围校验权威。
改动思路
入口保持不变：Python effect_runtime.py 启动 effect_runtime_server.ts，start_goal.py 消费启动异常。决策边界放在 TypeScript 的 parseIdleMs()，因为它紧邻实际 timer 和 server startup；Python 的职责是捕获有界 stderr、校验 loopx_effect_runtime_startup_error_v0 schema/code，并把信息投影给 CLI。无效输入的状态流是“解析失败 → typed invalid_idle_timeout → 进程 code 2、无 listener/info → start-goal 展示消息并给 retry hint”；未设置或合法短 timeout 继续走原 readiness 路径。最终提交还删除了原先重复的 Python 数值范围 owner，改为复用 TS 发出的 typed message。Malformed 或未识别 stderr 不会被误判为 timeout，而是保留既有 generic pre-ready fallback；因此协议是一个有界适配层，不会扩大 runtime 的生命周期或外部 authority。
具体改动
关键代码讲解
loopx/control_plane/effect_runtime_server.ts:parseIdleMs使用 anchored decimal 检查、Number.isSafeInteger和 1..2^31-1 闭区间校验；unset 仍选择 300000。错误由 startup diagnostic writer 输出 schema/code/message 后在 server listen 前退出。loopx/control_plane/effect_runtime.py:_startup_diagnostic从TemporaryFile读取最多 8KiB stderr，并把 message 规范化、截断到 240 字符；只有 schema 和 code 同时匹配才产生 typed exception，其他输入返回原 fallback。loopx/cli_commands/start_goal.py对invalid_idle_timeout复用实际 runtime message，再追加重试指导；其他启动失败分支保持原文和行为。tests/control_plane/test_effect_runtime_integration.py覆盖 valid/unset、边界、invalid、malformed 和真实子进程启动；tests/control_plane/test_start_goal_compact_projection.py覆盖 CLI 投影。
对主干的风险
我没有发现需要阻塞合并的 exact-head finding。验证包括：idle-timeout focused integration 15 passed，完整 effect-runtime integration 43 passed，start-goal projection 62 passed，Ruff、py_compile、git diff --check 和 npm run typecheck:control-plane 均通过；直接运行 exact-head Node server，idle=0 得到 code 2、无 runtime-info 文件和预期 JSON。baseline 的 valid/unset 路径与 head 保持成功，malformed envelope 仍走 generic fallback。剩余风险是当前环境无法执行 hosted Windows process-handle 场景，且该 PR 相对 origin/main 处于 BEHIND、没有已报告的 hosted checks；这不改变本次代码路径的验证结论，但合并前应让主干同步和跨平台检查完成。协议名只描述 startup error/timeout，不宣称 agent、quota、Todo 或 scheduler authority。
我的整体评价
这是一个范围合适、职责边界正确的修复：真正的配置规则只在 TS timer owner 存在，Python 负责有界翻译，CLI 负责用户指导；最终 head 还移除了重复规则。可观察差异仅发生在无效 timeout 场景，而且已用真实子进程和基线对照验证，合法与 unset 场景保持兼容。基于完整 policy-v3 evidence、exact-head freshness 和上述检查，我批准该提交；若后续 rebase 或修改 head，需要以新 exact head 重新复审。
English verdict: APPROVE — exact head 5a9386231472dd3102865c4e2de01908cbbef913 has no blocking finding; startup validation, bounded diagnostic adaptation, CLI remediation, and compatibility checks are verified. Re-review after any head change.
2 participants
Summary
LOOPX_EFFECT_RUNTIME_IDLE_MSbefore the managed TypeScript Effect runtime publishes runtime information or starts listening.Number(), so a non-numeric, zero, negative, fractional, out-of-range, or unsafe value can no longer silently become an immediate shutdown timer.loopx_effect_runtime_startup_error_v0envelope on stderr and exits with status 2; the Python startup path maps it toinvalid_idle_timeoutinstead of the misleadingruntime_exited_before_ready, and no runtime-info file is left behind.Issue Or Task
Validation
mainat 9231d5b)unitpassedpython -m pytest -q tests/control_plane/test_effect_runtime_integration.py -k "idle_timeout"— 14 new cases passing. Covers unset, a valid short timeout, the documented upper bound, and 11 rejected values: empty, non-numeric, zero, negative, fractional, exponent, hex, leading/trailing space, one above thesetTimeoutceiling, and an unsafe integer.integrationpassedpython -m pytest -q tests/control_plane/test_effect_runtime_integration.py— 41 passed. Every new case drives the real managed Python-to-TypeScript startup boundary througheffect_runtime_result, so the assertion is on the spawned runtime's diagnostic rather than on a parser helper. The invalid-value cases use the defaultretry_safe=True, which also proves the startup retry path does not mask the typed diagnostic.staticpassednpm run typecheck:control-plane(clean) andpython -m ruff check loopx/control_plane/effect_runtime.py tests/control_plane/test_effect_runtime_integration.py.unitfailednpm run test:control-planeon the rebased revision — 1429 passing, 6 failing, 1 cancelled of 1436. These failures cannot come from this change:tests/control_plane_ts/contains no reference toeffect_runtimeorLOOPX_EFFECT_RUNTIME, and the only TypeScript file this PR touches,effect_runtime_server.ts, is imported by zero TypeScript test files. The same suite passed 1289 / 0 on the previous base (7eb4b7bb), and the failures appeared only aftermainadvanced toddf6efa8, which added 146 tests. Reported asfailedrather thanpassedbecause the suite is not green on this host at the tested revision.integrationblockedtests/control_plane/test_effect_runtime_integration.py(test_runtime_ready_budget_starts_after_start_lock_acquisition,test_early_runtime_exit_surfaces_stable_startup_diagnostic) fail on this contributor host. Both fail identically on an unmodified checkout of the same revision, so they are host-environment failures rather than regressions from this change. The same applies to two cases intests/control_plane/test_turn_journal_runtime_readiness.py.manualpassedloopx check --scan-path loopx/control_plane --scan-path tests/control_plane—ok, errors=0, public boundary scan clean across 617 files.test_managed_runtime_releases_memory_after_idle_timeoutstill covers the valid idle-shutdown lifecycle, so the normal path is unchanged. Not covered: a Windows host and a non-UTF-8 locale; the stderr capture is an unlinked temporary file rather than a pipe, so the read path is not exercised under a pipe-backed stderr.Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
Core control-plane hardening
Long-horizon benchmark evidence
Operator surface and IM integration
Shared Goal Authority and cross-host coordination
Architecture and research incubator
Target base branch: main
Direction tracker or promotion unit: N/A
Shared-authority RFC fixture impact
N/A — this change does not claim progress against the TypeScript control-plane migration or the shared Goal Authority RFC.
Boundary Checklist
.loopx/,.codex/goals/, and liveACTIVE_GOAL_STATE.md).none.Signed-off-bytrailer (git commit -s).