test(runtime): pin the projection writer's refuse-before-write and readback guards - #5044
Conversation
…adback guards Signed-off-by: NIU-123370 <191000457+NIU-123370@users.noreply.github.com>
d356a01 to
cd74cad
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
Exact head: cd74cad. This review concerns the test evidence on this head, not remote CI or merge authorization.
动机
共享 runtime projection writer 的拒绝写入与追加后读回是实际产品边界;用持久测试保护它有价值。但本 PR 声称新增的“读回不匹配”用例可以证明该边界,必须先确认测试在正常输入下会成功。
改动思路
新增测试通过 _call 构造 writer 输入,再把 load_index 改成空结果,期望 writer 拒绝声称投影成功。真实路径应先用合法的 record/index_record 投影成功,再仅因读回失配而失败;否则异常可能来自夹具本身,无法验证所宣称的因果关系。
具体改动
tests/control_plane/test_runtime_projection_writer_guards.py 增加 6 个测试(+122 行):四个缺失/非法身份的写前拒绝、一个 dry-run、一个追加后读回失败。写前拒绝测试和真实调用 smoke 有意义。可是 _call 的 index_record 在第 43 行没有 shared_runtime_projection marker,writer 追加它后做索引读回时必然找不到与 record 相同的 marker。
关键代码讲解
_record在第 26–35 行创建 marker;_call在第 38–50 行却只把 goal_id/kind 放入 index_record,破坏 writer 的正常成功前提。test_readback_mismatch_fails_closed_after_append在第 106–122 行只断言 OSError 和索引已追加。去掉第 112 行的 monkeypatch,原测试仍因缺 marker 得到同一个 OSError,因此无法证明“load_index 看不见刚写入的行”导致拒绝。
对主干的风险
这是测试本身的阻断,不是远端 CI 状态:在 exact head 运行该文件为 6 passed;两条真实 caller smoke 也通过。但我独立调用无 monkeypatch 的 _call(tmp_path),得到 OSError: runtime projection append did not pass index readback;给 index_record 加上与 record 一致的 marker 才返回 projected/readback_verified=True。这意味着新增读回负例会在实际读回守卫失效时仍可能绿灯。请修正夹具,新增或复用未 monkeypatch 的正向断言,再运行该文件和真实 caller smoke;不要只保留期待异常的用例。
我的整体评价
保护真实共享 writer 的方向正确,写前拒绝测试也提供增量价值;但关键读回用例目前是假阳性,长期回归保护尚未成立。变更不触及运行时或用户入口,所以用户体验无直接变化;也无兼容性迁移。相关的小重构就是让一处夹具复用相同 marker,不需额外测试框架。修复并证实“正常成功、仅故障失败”后可复审。
English verdict: REQUEST_CHANGES - fix the invalid index fixture and prove the unpatched success path before relying on the readback-failure test.
…control case Signed-off-by: NIU-123370 <191000457+NIU-123370@users.noreply.github.com>
|
Amendment pushed on head What changed:
Numbers on
The |
|
Attribution note on the red rows here, so they are not charged to this PR: on this head This diff adds one file under Local numbers on this head, for the record: focused |
huangruiteng
left a comment
There was a problem hiding this comment.
Exact head: 7ec5a32. Re-review after my REQUEST_CHANGES review on cd74cad.
动机
共享 runtime projection writer 同时服务 material 与 refresh 两条真实调用路径。旧有端到端 smoke 已覆盖成功写入和 replay,却没有把“无效 marker 必须写前拒绝”和“追加后读不到不得声称成功”固定为直接回归测试。上一版测试的索引夹具本身缺 marker,导致读回负例即使移除故障注入也会失败;本次更新需要先排除这个假阳性,才谈得上长期保护。
改动思路
本 PR 没改产品 writer,而是在可丢弃的临时 runtime root 上调用真实 write_compact_runtime_projection。修订把与 record 相同的 marker 放进 index_record,然后用一个未 monkeypatch 的成功用例作控制,再让相同夹具只在 load_index 被置盲时得到 OSError。写前拒绝、dry run 和真实调用方分别由该文件及现有 material/refresh smoke 守护,测试责任没有扩成第二套生产实现。
具体改动
整份 PR 只新增一个测试文件(+149 行,零产品代码变更);相对我上次审过的 head,新增 36 行并改写 9 行。_index_record 补足共享 marker,_index_rows 统一读取真实追加记录,test_unpatched_append_projects_and_reads_back 明确检查正常写入的状态、读回布尔值、两个产物路径和一条索引行。旧负例继续验证盲读后仍抛错且追加行已落盘,准确描述这个 guard 是“拒绝成功声明”而不是事务回滚。
关键代码讲解
tests/control_plane/test_runtime_projection_writer_guards.py:40的_index_record让索引行包含 writer 在读回时查找的shared_runtime_projectionmarker,消除了上一版夹具独立于注入故障就会失败的前提错误。:124的test_unpatched_append_projects_and_reads_back是正向对照:不修改读侧时返回projected/readback_verified=True,磁盘上同时有 JSON、Markdown 和一条 index 行。:134的test_readback_mismatch_fails_closed_after_append仅把load_index改为空结果,然后断言同一写入过程抛出读回 OSError,且真实 index 已追加一行;它和正向对照一起定位了失败原因。
对主干的风险
这是一项只改测试的回归保护,不会改变现有 CLI、runtime 状态或用户入口。我在当前 worktree 的 uv run --extra test 环境核对了 loopx.__file__ 指向该 head,运行新增文件得到 7 passed;material 与 refresh 两条真实调用方 smoke 均通过,ruff check 与 git diff --check 也通过。对照现有 smoke,成功写入与 already_current 已被真实调用方覆盖,而无效 marker、缺失身份和盲读拒绝此前没有直接断言;这个测试文件的 149 行是集中覆盖共享 guard,并非重复制作一个运行时实现。未在本轮查询或等待远端 CI,亦未把作者提及的其他测试失败作为本 PR 的阻断证据。跨进程并发写入仍不在此测试范围,也不是本 PR 声称要解决的问题。
我的整体评价
上次的唯一实质阻断已在同一现行 head 上修复:现在“正常成功、注入盲读才失败”的因果关系成立,且两条真实调用链没有回退。长期效果是未来弱化共享 writer 拒绝条件时更容易被发现;用户体验保持原样,因为没有产品行为变更。相关的未来重构可以考虑让测试索引 marker 直接由同一个 record 参数生成,但目前固定夹具与现有覆写用例一致,不值得为此扩展抽象。基于上述本地证据,我批准这个精确 head 的测试更新;这不是对未观察的远端 CI、并发行为或合并就绪状态的保证。
English verdict: APPROVE - the corrected index fixture and unpatched success control make the readback-failure test causally valid; all focused local tests and both shipped caller smokes passed.
|
谢谢复审,P1 接受—— 残留问题:读回只证明“有 marker”,没证明“是这次的身份”。 我补的用例(在 def test_readback_requires_the_same_identity(tmp_path: Path) -> None:
# Proof that "my append landed" has to be identity equality, not the presence
# of some well-formed marker: a caller that indexes a different projection
# must not be reported as projected.
other = _record(source_projection_sha256_16="ffffffffffffffff")
with pytest.raises(OSError, match="did not pass index readback"):
_call(
tmp_path,
index_record={**_index_record(), MARKER_FIELD: other[MARKER_FIELD]},
)
assert len(_index_rows(tmp_path)) == 1实测(exact
邻域与归因: 本地环境披露:跑在检出对应 revision 的干净 worktree 上,复用本机已装好的解释器(Python 3.12.14),并把 Node 22.23.2 放到 PATH 最前以避开 后续默认走最小打扰:不改你已批准的 head。如果你更希望它进这一条,我把这 15 行推成一个新 head 供你重审(父提交就是 |
Goal And Delivered Outcome
index_recordwithout the projection marker, so the writer refused for a fixture defect
rather than for the injected fault. The fixture now carries the same marker the
writer scans for, and an unpatched control case asserts the normal success at
this boundary before the blinded-read case asserts the refusal.
write_compact_runtime_projection(
loopx/control_plane/runtime/runtime_projection_writer.py:12) is the singleshared writer behind both shipped compact runtime projections
(
loopx/control_plane/runtime/shared_runtime_material_projection.py:190andloopx/control_plane/runtime/shared_runtime_refresh_projection.py:169, whichpass their own
dry_runstraight through). Its happy path, itsalready_currentreplay and its readback success are all exercised by theprojection smokes — but its three refusal paths were named nowhere in
tests/,examples/orregression/: themust include object markerrejection, the
is missing identity fieldsrejection, and theruntime projection append did not pass index readbackfailure. A writer thatis shared by two callers is exactly where a silently-weakened guard spreads.
caller could pass a record whose marker field is absent, a string, or missing
one identity component, and a future edit that dropped the pre-write
validation would be caught by no test at all; after, six tests pin that the
writer raises before creating any path under the runtime root, that the
rejection still happens on a dry run, that a dry run reports
would_projectwithout writing, and that a blind read side turns the append into an
OSErrorinstead of a verified projection. Proven by the
unitandregression_parityrows below.
by this diff, so no
Closes. Basemain.Scope And Continuation
Deliberately not re-asserted here: the happy-path bytes, the markdown renderer
output and the
already_currentreplay, which the owning smokes already pin(mutation E below shows the smoke still catches the replay, so moving those
assertions here would duplicate an existing owner). Not covered and not
claimed: concurrent appenders under
exclusive_run_index_lock— this diff addsno cross-process contention case.
Validation
7ec5a3216unitpassedpython -m pytest -q tests/control_plane/test_runtime_projection_writer_guards.py→7 passed in 0.27s.unitpassedpython -m pytest -q tests/control_plane -k "projection" --ignore=tests/control_plane/test_reviewed_promotion_cli.py: base19358f114in a separate detached worktree → 422 passed / 1 failed; this head → 429 passed / 1 failed, the same-named failure in both runs. The +7 is exactly this PR's tests; no previously-green test turned red.regression_paritypassedisinstance(marker, dict)guard →3 failed;all(identity)→any(identity)→1 failed; move thedry_runearly return above validation →1 failed; drop theif not readback_verified: raise OSError→1 failed; forcereadback_verified = True or any(...)so the guard can never fire →1 failed. Sixth mutation, not caught by this file: disabling thealready_currentreplay leaves all 7 green — by design, because that behavior has an existing owner;python3 examples/control_plane/shared-runtime-material-projection-smoke.pyfails (AssertionError, exit 1) under that mutation and passes on an unmodified tree. The refusal mutations leave the caller smoke green, which is why the writer-level file is needed at all.real_entrypointpassedtest_unpatched_append_projects_and_reads_backis the control the first revision lacked: with the same fixture and no monkeypatch the writer returnsprojected/readback_verified=Trueand appends exactly one index row, so the readback refusal below is about the blinded read side and not about the fixture.real_entrypointpassedshared-runtime-material-projection-smoke.pyandrefresh-state-shared-runtime-projection-smoke.py→ ok.staticpassedpython -m ruff checkon the new file → all checks passed;ruff formatapplied to the new file only (no pre-existing file reformatted).staticnot_applicablepython -m mypywas not run:[tool.mypy] files=[…]enumerates 24 modules, none undertests/, and the module under test is not among them; this diff changes no product file, so no listed target can change verdict.through a caller, so it covers the guard ordering (validate → reserve →
append → read back) and every early return, which is the part a future edit to
a shared writer tends to break. Gaps named plainly: (1) one failure in the
neighborhood selection,
tests/control_plane/test_shared_goal_alignment.py::test_appending_one_event_rotates_the_projection_into_frontier_behind,is pre-existing on
main— it fails identically with this PR's file removedand fails when run alone, so it is not caused by and not fixed by this
diff; (2) the same selection cannot collect
tests/control_plane/test_reviewed_promotion_cli.pyonmain(
ImportError: cannot import name '_workspace' from 'tests.control_plane.test_local_authority_shadow_cli_e2e', that file's line12), which is why it is excluded above — unrelated to this PR and reported
separately rather than silently widened into this scope; (3) the readback test
pins that the writer refuses to claim verification, and asserts honestly that
the appended row is still on disk afterwards — it is a claim guard, not a
rollback. Interpreter note: checks ran with repository-relative commands
against a locally installed Python 3.12 environment (an editable install from
another worktree on this machine), not
uv sync --extra test;loopx.__file__was confirmed to resolve to this worktree before the runs.
Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
hardening — durable-write guard coverage. No roadmap S/G/R id claimed.
Shared-authority RFC fixture impact
changed.
tests assert refusals on a disposable runtime root.
Python append path, not a provider arm; no provider behavior changes.
runtime-routing or compatibility-projection behavior is touched.
Boundary Checklist
.loopx/,.codex/goals/, and liveACTIVE_GOAL_STATE.md).none.Signed-off-bytrailer (git commit -s).