Skip to content

test(install): expect the fixed install to keep repo-only skills in the checkout - #4562

Merged
huangruiteng merged 1 commit into
mainfrom
codex/repo-only-skill-install-expectation
Sep 16, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/repo-only-skill-install-expectation

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Problem

main is red on examples/install-local-smoke.py:

tests/... AssertionError
  File "examples/install-local-smoke.py", line 367, in main
    assert set(skill_readback["materialized_skill_ids"]) == {

#4487 (fix(install): never deliver a repo-only workflow skill to a host) moved skill-scope classification into the capability that owns the marker and made the fixed install deliver only declared global scopes. A repo-kept workflow has no marker on purpose, so loopx-pr-merge is now repo_only and is never copied onto a host.

This smoke still encoded the old behavior — it expected the source install to materialize loopx-pr-merge, with the comment "Source installer also ships the merge workflow". The contract moved and this reader did not.

Change

  • Expect the delivered set to be the loopx entry plus PACKAGED_HOST_SKILL_IDS (the declared global skills).
  • Make the repo-only omission explicit the same way examples/release/local-install-promotion-boundary-smoke.py already does: collect the marker-less sources from the checkout and assert none of them reached the host, with a non-vacuous guard so the assertion cannot silently pass if the checkout stops carrying one.

This is a test-expectation alignment, not a behavior change: the installed set on a host is whatever #4487 decided.

Validation

  • Tested revision: 38bb42c
  • Run state: finished
  • Input classes: synthetic
Check kind Result Public-safe evidence / limitation
signal passed Baseline: the same smoke fails on a pristine main checkout at the set assertion above; with this diff both examples/install-local-smoke.py and examples/release/local-install-promotion-boundary-smoke.py are ok
unit passed tests/test_skill_delivery_parity.py, tests/test_project_skill_cli.py, tests/capabilities/test_material_project_skill_delivery.py, tests/test_packaged_skill_metadata.py — 50 passed
static passed ruff check examples/install-local-smoke.py; loopx canary premerge --from-git-diff — status: passed, merge_gate_passed: true, manual_holds: 0, 1 changed file, surfaces public_boundary, python
regression_parity passed Mutation check: re-introducing the pre-#4487 classification (an unmarked source classified as deliverable) makes this smoke fail again at the new assertion, so the assertion is load-bearing rather than descriptive
  • Coverage and gaps: the changed behavior is the fixed installer's delivery set, and it is covered by the install smoke itself plus the promotion-boundary smoke that #4487 extended. No product code, installer, permission, or delivery behavior changes in this diff.
  • One catalog entry is reported as advisory_inherited_failure: examples/control_plane/control-plane-maintainability-ratchet-smoke.py (module_metric_budget:loopx/chat_runtime.py) fails on main too, on a file this diff does not touch; recorded here and not treated as this diff's regression.
  • Lineage: this was the last leftover of the closed #4486, closed as superseded by #4487. That branch was deleted, so the leftover is proposed fresh here rather than by reopening a superseded PR.
  • Public/private boundary: no private state, credentials, raw logs, or local paths.
  • Frontend / Visual Evidence: Before: N/A. After: N/A.

…he checkout

The fixed installer classifies skill sources in the capability that owns the
scope marker and delivers only declared global scopes, so a repo-kept workflow
with no `.loopx-skill-scope` never reaches a host. This smoke still expected the
source install to materialize `loopx-pr-merge`, which is why it fails on main.

Assert the delivered set is the entry plus the declared global skills, and make
the repo-only omission explicit the way the promotion-boundary smoke already
does, so a re-introduced delivery fails here instead of shipping a
merge-decision workflow to hosts that never merge LoopX pull requests.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

Self-merge validation (38bb42c)

Owner-authorized self-merge for a one-file test-expectation alignment.

  • Changed surface: python (examples/install-local-smoke.py), plus the public_boundary catalog surface this smoke belongs to. One file, +14/-2, no product code.
  • loopx canary premerge --from-git-diff: status: passed, merge_gate_passed: true, self_merge_allowed: true, manual_holds: 0; catalog 5/5 executed with zero failures (this file's own smoke now passes), 1 advisory inherited failure recorded below.
  • examples/install-local-smoke.py — ok; examples/release/local-install-promotion-boundary-smoke.py — ok.
  • tests/test_skill_delivery_parity.py + test_project_skill_cli.py + test_material_project_skill_delivery.py + test_packaged_skill_metadata.py — 50 passed.
  • Mutation check: reverting the #4487 classification (an unmarked source counting as deliverable) makes this smoke fail at the new assertion, so the assertion is load-bearing.
  • Baseline: this smoke fails on a pristine main checkout at the expectation this diff replaces.

Failures and skips, named: the only catalog failure is advisory_inherited_failure on examples/control_plane/control-plane-maintainability-ratchet-smoke.py (module_metric_budget:loopx/chat_runtime.py), which fails identically on main on a file this diff does not touch. Main's four test-shard jobs are also red for inherited goal-channel and ratchet tests unrelated to skill delivery; the merge therefore uses the maintainer bypass on a main-level baseline, recorded rather than papered over.

Why the coverage is enough: the behavior under test is exactly the fixed installer's delivered skill set, and it is asserted from two sides (this source-readback smoke and the promotion-boundary smoke #4487 extended), with a mutation check proving the assertion fails if delivery regresses.

@huangruiteng
huangruiteng merged commit 83c4220 into main Sep 16, 2026
3 checks passed
@huangruiteng
huangruiteng deleted the codex/repo-only-skill-install-expectation branch September 16, 2026 14:38
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

自 review(补录)— 复核于已合并 head 38bb42c288debfe5445d108483502936732d96e6

本 PR 合并时没有留下 review 记录,这是流程缺口;按"自合并的 PR 都要自 review"补一次 at-merged-head 复核。

动机

固定安装脚本不再把 repo-only 工作流投递到宿主,但 install smoke 仍把 loopx-pr-merge 写进"已物化 skill"的期望集合里,于是测试编码的是修复前的行为。

改动思路

把期望从硬编码名单改成从 checkout 里推导:凡是没有 .loopx-skill-scope 标记的 skill 目录就是 repo-only,安装后必须不在物化集合里。这样规则变化时测试跟着推导而不是跟着记忆。

具体改动

examples/install-local-smoke.py(+14/-2):删掉名单里的 loopx-pr-merge,改为推导 repo_only_skill_ids 并断言物化集合与它不相交。

对主干的风险

只改断言。风险是推导式断言本身可能过度约束(见下)。

我的整体评价

方向上比原来的硬编码名单更好:它把"哪些 skill 会被投递"重新交给 .loopx-skill-scope 这一个所有权,而不是在测试里复制一份名单。在今天的 main(e6427250)上复核:skills/ 下唯一没有 scope 标记的目录仍是 skills/loopx-pr-merge/,所以这条推导仍然成立、守卫仍在生效。一条非阻断观察:

  • P3(推导式断言会在合法未来状态下失败):assert repo_only_skill_ids 要求 checkout 里必须存在至少一个 repo-only skill。如果哪天所有随包 skill 都声明了 scope(即没有 repo-only 工作流了),这条断言会以"the checkout no longer carries a repo-only skill source"失败,而那是一个合法的未来状态。建议改成断言"推导集合 == 显式声明的 repo-only 集合"(或者当推导为空时跳过不相交断言),让失败只在真正的投递错误上出现。

English verdict: APPROVE (retrospective, no blocking findings) — at merged head 38bb42c2, the smoke now derives repo-only skills from the single .loopx-skill-scope ownership instead of restating a list, and the derivation still holds on today's main; the only note is that the non-empty assertion would fail on the legitimate future state where no repo-only workflow remains.

huangruiteng added a commit that referenced this pull request Sep 16, 2026
`skills/loopx-pr-merge/SKILL.md` already states that a merge, approval,
self-merge or admin-bypass decision requires review evidence for the exact head,
and `pr-review --check-merge-readiness` already fails closed without it. The
policy list an agent reads first did not say so, and four self-merged PRs
(#4488, #4489, #4491, #4562) reached main with no review record at all.

- Add the published-exact-head-review condition to the self-merge list, naming
  the `COMMENTED` review an author-owned PR uses because GitHub blocks formal
  self-approval, and naming the `check-merge-readiness` result it must have.
- State in the 自合并 definition that a self-merge without that record is a
  process gap to repair, not a smaller form of review.
- Make the merge skill's decision record unambiguous: publish it on the exact
  head rather than summarizing it in another channel.

Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Co-authored-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approval conclusion (author-owned PR; GitHub blocks formal self-approval)

动机

问题是真的,我在父提交上复现了同一处失败:examples/install-local-smoke.py 在 materialized_skill_ids 的集合断言处 AssertionError(exit 1)。原因也正如正文所说:#4487 把 skill scope 分类收进了拥有该 marker 的 capability,固定安装只交付声明为 global 的 scope;repo-kept workflow 故意没有 marker,于是 loopx-pr-merge 成了 repo_only,再也不会被复制到宿主。旧 smoke 仍写着 "Source installer also ships the merge workflow"——契约搬了,读它的 smoke 没跟上。

改动思路

修法选得对,且没有把守卫删掉:把期望集合改成 loopx + PACKAGED_HOST_SKILL_IDS,然后额外加一条"marker-less 的源一个都没被交付"的断言(与 examples/release/local-install-promotion-boundary-smoke.py 已有的做法一致),并加了非空守卫 assert repo_only_skill_ids——保证这条断言不会因为 checkout 里不再有 repo-only skill 而静默通过。

这一点很关键:如果只是把旧期望删掉,smoke 变绿但不再守卫 #4487 交付的行为;现在它既对齐了新契约,又对"把 repo-only workflow 复制到宿主"这类回归保持敏感。

具体改动

examples/install-local-smoke.py +14/-2,单文件、无产品代码。

我做的验证:

  • 本 head 实跑安装 smoke → install-local-smoke ok(exit 0)。
  • 父提交(5d66197c6)实跑同一 smoke → exit 1,失败位置与正文引用完全一致。
  • examples/release/local-install-promotion-boundary-smoke.py → ok(另一侧边界仍然守得住)。
  • pytest tests/test_skill_delivery_parity.py tests/test_project_skill_cli.py tests/capabilities/test_material_project_skill_delivery.py tests/test_packaged_skill_metadata.py → 50 passed。
  • git diff --check 干净。

关于"这个 smoke 是否值得保留",我做了覆盖扫描:单元测试覆盖的是分类规则与元数据(test_skill_delivery_parity.py、test_packaged_skill_metadata.py、capability 自身代码),而本 smoke 覆盖的是真实固定安装后的宿主清单,这是单测无法替代的;promotion-boundary smoke 覆盖安装器报告的边界,两者是同一契约的两侧,不构成重复。

顺带记录(不作为 findings):新加的 repo-only 收集把 marker 文件名 .loopx-skill-scope 硬编码在 smoke 里,而该文件名由 loopx/capabilities/project_skill_delivery/core.py 的 PROJECT_SKILL_SCOPE_FILE 拥有。它与相邻的 promotion-boundary smoke 写法完全一致(既有的既有模式),所以不是本次引入的新分歧;若以后要收敛,两处一起改成 import owner 的常量即可。

另外做了同作者批量扫描:#4534(#4521 之后的陈旧 marker 期望)与 #4718("restore the three public smoke contracts broken on main")是同一形状——读方没跟上已交付的契约变更。它们都在恢复/对齐真实守卫,没有批量生产脚手架,也不启动任何 benchmark 作业,所以属于反复出现的维护形状而不是 batch farming,不需要升级为贡献限制;我在 #4521 的审查里已经记下那条流程建议(搬动契约的 PR 必须跑读它的 smoke)。

对主干的风险

风险很低:单个 example 文件,不含产品代码,安装器/权限/交付行为一律未改;smoke 只是在读取 #4487 已经决定的交付集合。新断言在两个方向上都成立——既怕"交付了 repo-only"(交集非空会失败),也怕"checkout 里没有 repo-only 可测"(非空守卫会失败),所以不会退化成描述性断言。

唯一的观察是上面那条 marker 名硬编码(与邻居一致,属可选收敛项),不构成风险。

我的整体评价

这是一次正确的期望对齐:真实 red gate、单文件小改、不删守卫、并补上非空约束使新断言真正承重;父/head 两个方向我都实跑过(失败→通过),相关 50 个测试与另一侧边界 smoke 也都通过。覆盖扫描显示它与单测、promotion-boundary smoke 互补而非重复,保留价值成立。

建议(可选)后续把两处 smoke 的 marker 名收敛到 capability 的常量,不构成合并阻塞。

English verdict: APPROVE (exact head 38bb42c)

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.

1 participant