Skip to content

fix(effect-runtime): keep one startup envelope one record - #5058

Merged
huangruiteng merged 2 commits into
loopx-project:mainfrom
NIU-123370:niu/startup-diagnostic-framing
Sep 25, 2026
Merged

huangruiteng merged 2 commits into
loopx-project:mainfrom
NIU-123370:niu/startup-diagnostic-framing

Conversation

@NIU-123370

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

  • Goal/source and gap: [Bug]: A rejected idle timeout containing a line-break character loses its typed diagnostic #5057. The managed TypeScript runtime already rejects a bad LOOPX_EFFECT_RUNTIME_IDLE_MS with one typed envelope on stderr, and _startup_diagnostic exists to turn that envelope into the operator-facing diagnostic. For three specific characters it did not: the record was torn apart before parsing, so the caller reported runtime_exited_before_ready (exit_code=2) and the actionable guidance was lost.

  • Observable before → after, with the validation row that proves it: driving the real Python-to-TypeScript startup boundary on main at 3eac23563, a value padded with U+0085, U+2028 or U+2029 returned diagnostic_code=runtime_exited_before_ready; on this head all three return invalid_idle_timeout, and no runtime-info file is written either way. Values padded with U+001C or ASCII whitespace returned invalid_idle_timeout before and after, which is the half of the table that shows the accepted set did not move. See the real_entrypoint and regression_parity rows.

  • Issue/task and intended base: Closes [Bug]: A rejected idle timeout containing a line-break character loses its typed diagnostic #5057. Base main at 3eac23563.

The mechanism is a set difference, which is why exactly those three characters failed: str.splitlines() treats U+0085, U+2028 and U+2029 as line breaks, and JSON.stringify escapes everything below U+0020 but leaves those three raw. The rejected value is quoted back inside the message, so a raw line break lands inside the envelope and every fragment that starts with { fails to parse.

The fix frames the stream on \n, the separator the server actually writes and the one the response reader in this same module already uses. This matches the function's own documented contract ("writes one JSON envelope to stderr").

Scope And Continuation

  • Completed scope and remaining work: the reader's record framing, plus tests that pin it. Unchanged by design: which values are accepted or rejected, the envelope schema and its code values, the exit status, the guidance text, and the "a stack trace is not a typed diagnostic" rule, which is asserted rather than assumed.

    One alternative was considered and deliberately not taken: escaping U+0085/U+2028/U+2029 in parseIdleMs before the value is echoed, so the wire format can never be torn by any consumer. That would put the framing guarantee in two places with one real consumer today, and it would leave this reader still mis-framing for any other future record that carries such a character.

  • Slice boundary / successor: complete within this scope. [Bug]: Validate LOOPX_EFFECT_RUNTIME_IDLE_MS before starting the managed runtime #4106 and fix(effect-runtime): validate the managed idle timeout before startup #4349 own validation; this owns only how the published envelope is read back.

Validation

  • Tested revision: 962a4a9ea328e9a8950a84e672a647e812f7b12e
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
static passed python3 -m ruff check on both changed files; python3 -m mypy run without arguments as CI does, Success: no issues found in 23 source files; git diff --check clean.
unit passed tests/control_plane/test_effect_runtime_integration.py: 8 selected by -k startup_diagnostic pass, covering the three tearing characters, two already-correct paddings, multi-record stderr, and the no-envelope negative. Full file 57 passed.
real_entrypoint passed pytest -k invalid_idle_timeout_configuration drives effect_runtime_result("runtime.ping", {}) through the real managed Python-to-TypeScript startup boundary with Node 22.23.2, now including U+0085/U+2028/U+2029 padding in the existing parametrised invalid-value list.
regression_parity passed Before/after on the same selection at 3eac23563 vs this head: the three line-break paddings reported runtime_exited_before_ready and now report invalid_idle_timeout; the 11 previously covered invalid values are unchanged. Mutations, each run separately against the same selection: reverting to str.splitlines() fails exactly 6 (the 3 unit cases and the 3 boundary params) with the rest green; replacing \n with \r\n, \r or \x1e each fails exactly 1, the multi-record case, which is what proves the separator is pinned rather than incidental.
manual passed loopx canary premerge --from-git-diff --git-diff-base HEAD~1: 8 checks selected, 8 executed, 0 failures, 0 advisories. loopx check --scan-path on both changed files: public boundary scan clean: 2 files.
  • Coverage and gaps: the change is one expression in one reader, and it is exercised both where it is decided (_startup_diagnostic directly) and through the only production caller path (the managed startup boundary), so both the framing rule and its operator-visible consequence are covered. The negative direction is also pinned: stderr with no envelope still yields None, so the fix cannot be satisfied by reporting any parsed JSON as a diagnostic.

    Disclosed rather than smoothed over: ruff format --check is not clean on either changed file before this PR, and I did not reformat them. I measured the deviation count on the pristine main versions and on this head and it is unchanged (effect_runtime.py 36 → 36, the test file 58 → 58). My first draft of the added test lines did introduce 4 new deviations; they were rewritten to conform rather than grandfathered in.

    The validation environment is a borrowed interpreter from an existing local virtualenv, not a fresh uv sync --extra test; the same commands and selection sets are what I ran. Node is 22.23.2, above the module's stated minimum.

    Remote check status is not part of this evidence per the repository's review convention; if a required lane is red for an unrelated trunk reason I will post the attribution separately rather than treat it as a result.

`_startup_diagnostic` framed the managed runtime's stderr with
`str.splitlines()`, which honours U+0085, U+2028 and U+2029 as line breaks.
A rejected `LOOPX_EFFECT_RUNTIME_IDLE_MS` echoes the offending value back
inside the typed envelope, and `JSON.stringify` leaves those three
characters unescaped, so the single documented record was torn into
fragments that no longer parsed. The caller then fell back to
`runtime_exited_before_ready (exit_code=2)` and hid the actionable
`invalid_idle_timeout` guidance.

Frame the diagnostic stream on `\n`, the separator the server actually
writes, matching the response reader in this module. Values padded with
U+001C or ASCII whitespace already reported correctly because those are
escaped or are not record breaks; they stay pinned so a future framing
change cannot silently widen or narrow the accepted set.

Signed-off-by: NIU-123370 <191000457+NIU-123370@users.noreply.github.com>
Framing on `\n` is only correct because the server terminates the envelope
with `\n`. The earlier test set could not tell that separator from `\r\n`,
`\r` or the ASCII record separator, because each rejection publishes one
record and a single-record stream survives any of them.

Add a stderr capture that mixes the envelope with unrelated Node output, so
the reader has to frame records rather than treat the whole capture as one
line. This is a regression pin for the framing rule, not a second fix.

Signed-off-by: NIU-123370 <191000457+NIU-123370@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English verdict: APPROVE - exact head 962a4a9 restores the typed startup diagnostic for U+0085/U+2028/U+2029; base/head real-entrypoint comparison, 57 tests, ruff, mypy and diff check passed.

动机

Issue #5057 指出:TypeScript effect runtime 拒绝非法空闲超时时已经输出一个带 invalid_idle_timeout 的 JSON 错误封套,但 Python 读取端把 U+0085、U+2028、U+2029 当成换行拆断,用户只得到笼统的 runtime_exited_before_ready。目标是保留原有拒绝规则、错误码和可操作提示,仅恢复真实启动入口对这三个值的诊断。这一小修复完整覆盖该 issue 的可观察结果。

改动思路

生产边界仍由 TypeScript failStartup 写一个以 LF 结束的封套,Python _start_runtime 在子进程退出时读取 stderr,并交由 _startup_diagnostic 解析。把 splitlines() 换为 split("\n"),按写端实际使用的记录分隔符切分,既保留消息内部的 Unicode 分隔符,也与同模块正常响应读取的 LF 规则一致。它复用当前单一读取器,没有新增状态、协议版本、可选开关或第二套诊断判定。

具体改动

关键代码讲解

  • loopx/control_plane/effect_runtime.py:653 的 _startup_diagnostic 仍反向扫描 stderr 记录,校验 JSON、schema_version 和非空 code;改变的只有第 667 行的分帧规则。普通 Node 堆栈不能伪装为类型化错误。
  • 同文件第 779–791 行的 _start_runtime 是实际消费者:解析成功则抛出带具体 code 的 EffectRuntimeStartupError,否则保留 runtime_exited_before_ready 后备。
  • 未修改的 effect_runtime_server.ts:52 中 failStartup 用 LF 输出封套;第 70–90 行 parseIdleMs 继续按原数字规则拒绝非法值。没有借修读取器放宽输入。
  • tests/control_plane/test_effect_runtime_integration.py:915–1029 新增封套分帧、混合噪声、无封套负例,并扩展真实启动参数化用例。约 82 行测试围绕一个持久的线协议不变量,属于必要的回归覆盖而非未来脚手架。

对主干的风险

最强回归假设是 LF 切分误把普通 stderr 当作错误封套,或让既有非法值被接受。代码仍要求完整 JSON、既有 schema_version 和非空 code;无封套堆栈的负例返回 None。用同一真实 Python→TypeScript 启动脚本比较 base 27f0fc93b 与本 head:三种 Unicode 值由泛化错误变成 invalid_idle_timeout,U+001C 和空格前后均保持该类型化错误,五种情况均无 runtime-info 文件。精确 head 的集成文件 57/57、ruff、mypy、diff check 通过。按本 Goal 的评审设置未查询远端 CI;合并就绪仍由独立门槛决定。

语义与 CI 对齐

共享错误封套 schema 和 invalid_idle_timeout 词汇未变化;只修读取端对既有写端 LF 契约的遵从。没有默认行为之外的激活路径、权限扩大或新的 Agent 权威声称。

我的整体评价

APPROVE。就 #5057 的长期运行和用户体验而言,启动失败仍会 fail closed,但操作者现在拿到原本已经发布的可操作诊断;没有引入重复效果或新的恢复步骤。生产机制仅一行语义变化,测试量与此前确实漏掉的分帧边界相称。相邻的更大 TS/Python 重构不是此处的必要前提;当前规则已在写端与读端收敛,未发现值得塞入本 PR 的额外兼容层或抽象。此结论仅针对 exact head 962a4a9ea328e9a8950a84e672a647e812f7b12e,是 review 结论而非合并授权。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: A rejected idle timeout containing a line-break character loses its typed diagnostic

2 participants