Skip to content

test(coordination): restore the reviewed-promotion CLI suite that main cannot collect - #5045

Closed
NIU-123370 wants to merge 1 commit into
loopx-project:mainfrom
NIU-123370:codex/fix-reviewed-promotion-cli-collection
Closed

NIU-123370 wants to merge 1 commit into
loopx-project:mainfrom
NIU-123370:codex/fix-reviewed-promotion-cli-collection

Conversation

@NIU-123370

Copy link
Copy Markdown
Contributor

Goal And Delivered Outcome

  • Goal/source and gap: tests/control_plane/test_reviewed_promotion_cli.py
    cannot be collected on main. Its line 12 imports _workspace, _cli,
    _command, _env and REPO_ROOT from
    tests.control_plane.test_local_authority_shadow_cli_e2e, and the merge of
    #5011 (1e21976d6, 2026-09-25 12:34 UTC) rewrote that module into the
    retired-setting CLI journey and dropped all five names — the pre-merge version
    defines them at REPO_ROOT/:21/:59/:74/:80, the current one has two
    test functions and no helpers. The result is
    ImportError: cannot import name '_workspace' from 'tests.control_plane.test_local_authority_shadow_cli_e2e', and a 230-line
    real-CLI end-to-end suite — reviewed promotion cutover and recovery across both
    the file and sqlite provider arms, source-drift rejection, and the
    policy-override and missing-fence refusals — silently contributes nothing.
  • Observable before → after, with the validation row that proves it: before,
    python -m pytest -q --co tests/control_plane/test_reviewed_promotion_cli.py
    ends in no tests collected, 1 error; after, the same command reports
    4 tests collected and the run is 4 passed in 13.93s (the unit row
    below). The helpers are restored verbatim from the pre-deletion version and
    defined in the file that uses them, so no fixture content, CLI argument or
    assertion changes — the only structural change is that this suite no longer
    depends on a sibling test module's private helpers.
  • Issue/task and intended base: self-contained test-collection fix; no issue is
    completed here, so no Closes. Base main.

Scope And Continuation

  • Completed scope and remaining work: collection restored and verified green.
    The important negative finding is stated rather than glossed: the restored
    suite passes on current main
    , so the breakage hid no behavioral regression
    and this PR restores coverage rather than reporting a defect in promotion
    itself.
  • Slice boundary / successor: the structural follow-up would be to host these
    helpers in tests/control_plane/shadow_e2e_fixture.py, which is already the
    designated shared owner ("Disposable public CLI fixtures…") and is imported by
    five other files including the module this one used to import from. Deliberately
    not done in this PR: that fixture's workspace() pins the goal id to
    goal-e2e, always writes coordination.runtime_shadow into the registry and
    always bootstraps, while this suite needs a registry without a pre-configured
    shadow (it drives configure-goal --coordination-runtime-shadow-file --execute itself) and needs non-zero exit codes parsed as JSON. Reshaping a
    five-consumer fixture is the owner's call, so this diff keeps the repair
    minimal and local rather than widening into that surface.

Validation

Public-safe summaries only.

  • Tested revision: 3bb561fc3
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
unit passed python -m pytest -q tests/control_plane/test_reviewed_promotion_cli.py → 4 passed in 13.93s (both provider arms: test_saved_plan_cutover_and_recovery_after_canonical_write_and_missing_legacy[file] and [sqlite], plus the source-drift and policy-override cases).
unit passed python -m pytest -q tests/control_plane/test_local_authority_shadow_cli_e2e.py → 2 passed — the module whose rewrite caused the breakage is untouched by this diff and still green.
regression_parity passed Failing-before evidence is the collection itself: on the unmodified base the same command exits with no tests collected, 1 error (ImportError: cannot import name '_workspace' …), and with this diff reverted the selection re-collects 0 tests. No assertion was weakened, added or skipped to reach green.
static passed python -m ruff check → all checks passed; python -m ruff format --check → 1 file already formatted.
static not_applicable python -m mypy not run: [tool.mypy] files=[…] enumerates 24 modules, none under tests/, and no product file changes here.
  • Coverage and gaps: what this restores is exactly what was missing — the only
    end-to-end coverage of reviewed promotion through the installed CLI transport
    and real local stores. Gaps named plainly: (1) the suite spawns the real CLI as
    a subprocess and the sqlite arm additionally runs the TypeScript provider
    through node, so it depends on a Node new enough for the control plane; it
    was run here with Node 22.23.2 and a locally installed Python 3.12 environment
    (an editable install from another worktree on this machine) rather than
    uv sync --extra test, and loopx.__file__ was confirmed to resolve to this
    worktree; (2) while running a wider selection
    (python -m pytest -q tests/control_plane -k "projection") one unrelated
    failure appears,
    tests/control_plane/test_shared_goal_alignment.py::test_appending_one_event_rotates_the_projection_into_frontier_behind,
    which reproduces identically on an unmodified tree and is neither caused nor
    fixed by this diff; (3) this PR does not add any new assertion about promotion
    behavior — it only makes the existing ones run again.

Frontend / Visual Evidence

  • UI impact: none
  • Before: N/A
  • After: N/A
  • States and viewports shown: N/A
  • Source data: synthetic
  • Attention review: N/A

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring (no functional changes)
  • Documentation update
  • Test update

LoopX Area

  • Control plane (goals, todos, quota, scheduler, registry, runtime)
  • Benchmark boundary (adapters, runners, verifiers, scoring, evidence)
  • Capability or extension (providers, adapters, skills)
  • Public docs or presentation surface (README, protocols, dashboard)
  • Build, packaging, installer, or CI
  • Host or runtime integration

Technical Direction

  • Direction / acceptance reference, when applicable: Core control-plane
    hardening — keeping reviewed-promotion end-to-end coverage actually running.
    No roadmap S/G/R id claimed.

Shared-authority RFC fixture impact

  • Production-scale fixture schema: N/A — no fixture schema, envelope or
    generator changed.
  • Semantic dimensions changed, or reviewed no-impact rationale: N/A for new
    dimensions; the impact is that the promotion cutover/recovery and
    provider-parity arms of this suite are collected again.
  • Provider conformance arms run: the suite's own two arms both ran — the
    file_v0 provider for all four cases and the sqlite_v0 provider for the
    cutover-and-recovery case, through the real CLI transport.
  • Read-only legacy/file/PostgreSQL three-arm rehearsal: N/A — this diff changes
    no promotion, routing or projection behavior; the restored tests exercise the
    file and SQLite paths only, and PostgreSQL is not configured on this machine.

Boundary Checklist

  • Neither the diff nor this PR body/comments/attachments disclose private state, credentials, raw traces or verifier output, internal links, or local machine paths (including .loopx/, .codex/goals/, and live ACTIVE_GOAL_STATE.md).
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.
  • I completed the visual evidence section for UI changes, or marked UI impact none.
  • Every commit includes a DCO Signed-off-by trailer (git commit -s).

…ng test module

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.

Exact head: 3bb561f. This review is against current origin/main 21ee4f0092e39b462296c8aa9cd115a258eea4ab, not remote CI or merge authorization.

动机

PR 旨在修复 reviewed-promotion CLI 测试因导入已删除的兄弟测试文件而不能收集的问题。这个问题值得解决,但当前主干已在 09fe7399d 更新同一个文件,必须先验证“main 仍不能收集”的前提是否存在。

改动思路

本 PR 把工作区创建、命令构造、环境配置和 CLI 启动辅助函数复制进测试文件,以消除旧导入。相比之下,最新主干已经使用 tests.control_plane.shadow_e2e_fixture 的共享夹具,并扩展了 provider/strategy 覆盖。两套辅助函数不应在同一文件冲突后并行维护。

具体改动

tests/control_plane/test_reviewed_promotion_cli.py 改为在文件中定义 _workspace、_command、_env、_cli,保留 4 个 CLI 用例;sys.executable 的选择是正确的。在本 PR exact head 独立运行该文件,4 passed(89.73s),说明旧基线上的收集问题可被这份补丁绕开。但最新主干同一文件已经独立收集 9 个用例,且实跑一条 source-drift 用例通过。

关键代码讲解

  • 新的 _workspace 和 _cli 在测试文件中复制端到端启动与解释器环境逻辑,和主干现有共享 fixture 重叠。
  • 主干该文件已使用 shadow_e2e_fixture.REPO/workspace,测试 file/sqlite 与多种 cutover 策略;若把这份旧分支直接套上去,会发生文本冲突,手工错误解决还可能丢掉主干新增的覆盖。

对主干的风险

当前 PR 的目标前提已失效:origin/main 上执行 uv run --extra test python -m pytest --collect-only -q tests/control_plane/test_reviewed_promotion_cli.py 收集 9 项、无 ImportError;一条代表性真实 CLI 用例 33.55s 通过。GitHub 当前也标记为 CONFLICTING。这里不是“无关红 CI”,而是同一文件的主干演进已完成更宽的修复。最小处理是关闭/标记本 PR 被主干替代;如仍有未覆盖缺口,先指出可复现差异,再从最新主干只提交增量测试,并继续复用共享 fixture。

我的整体评价

exact head 在旧基线上可通过,但对当前主干已无可验证的新增仓库价值;重复辅助函数会增加未来维护成本,强行合并可能减少覆盖。没有产品运行时或用户入口改变,用户体验无直接变化;无需兼容性层。相关的未来友好重构在主干的共享 fixture 已实现,本 PR 不应另建平行夹具。请在最新主干确认新缺口后再提精简增量。

English verdict: REQUEST_CHANGES - current main already collects and runs broader reviewed-promotion coverage, while this exact head conflicts and duplicates the shared fixture.

@NIU-123370

Copy link
Copy Markdown
Contributor Author

确认前提已失效,按建议关闭。

我在 cc1d46dd8 上实测到的 ImportError 当时是真的(--collect-only → no tests collected, 1 error,起因是 #5011 的合并 1e21976d6 删掉了 _workspace/_cli/_command/_env/REPO_ROOT)。但主干已经用更宽的形态修好了:09fe7399d 让该文件改从 tests.control_plane.shadow_e2e_fixture 取 REPO, workspace。我在最新 main 85759ba44 上自己复跑了一次 python -m pytest --collect-only -q tests/control_plane/test_reviewed_promotion_cli.py → 9 collected、无 ImportError。

本 PR 只恢复 helper、不新增断言,而主干是 9 条(provider/strategy 扩臂)另加 tests/control_plane/test_shadow_registry_source.py,所以合它只会把覆盖收窄并制造文本冲突。关闭,不再另开增量 PR;如果之后发现共享夹具表达不了的形状(例如需要非零退出码的解析),我再按可复现差异单独提。谢谢指出重复夹具的问题,这个判断是对的。

@NIU-123370 NIU-123370 closed this Sep 25, 2026
@NIU-123370

Copy link
Copy Markdown
Contributor Author

复核结论:这条 PR 的前提确实已经失效,我同意「被主干替代」的处理,现在关闭。

在最新 main(85759ba44)实测:

  • python -m pytest --collect-only -q tests/control_plane/test_reviewed_promotion_cli.py → 9 tests collected,无 ImportError;
  • 该文件在 09fe7399d 已经改用 tests.control_plane.shadow_e2e_fixture 的共享夹具,函数集是本 PR 的超集:saved_plan_cutover_and_recovery_after_canonical_write_and_missing_legacy(provider × strategy 参数化)、source_drift_does_not_freeze_legacy_writes、rejects_policy_override_and_recovery_without_fence,再加一条本 PR 没有的 rechecks_current_registered_agents。

所以再留着只会在同一个文件上多养一套平行夹具,正如评审指出的:解冲突时还可能丢掉主干新增的覆盖。不做了。

两条同期实测、对别人可能有用的事实:

  1. 在 09fe7399d 之前的任何基线上(包括本 PR 的 base 19358f114),pytest tests/control_plane -k projection 会被这个 ImportError 整个中断(该选择集 0 collected),所以在那类基线上跑本地验证要先排除这个文件;主干修好之后不需要。
  2. 当时我在旧基线上把 helper 就地恢复后,4 条 CLI 用例全绿 ⇒ 那次 collection 破损只是让测试拿不到运行机会,没有藏住产品行为缺陷。

后续如果在这条线上发现最新 main 仍未覆盖的缺口,我会另开精简增量并复用共享夹具。(以上本地验证跑在检出对应 revision 的干净 worktree 上,用本机已准备好的 Python 环境,并把 Node 22.23.2 放在 PATH 前面以避开 effect_runtime 的版本门;未使用 uv sync --extra test。)

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.

2 participants