Skip to content

docs: require a published exact-head review for every self-merge - #4583

Merged
huangruiteng merged 1 commit into
mainfrom
codex/selfmerge-review-record
Sep 16, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/selfmerge-review-record

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Goal And Delivered Outcome

Scope And Continuation

  • Completed scope and remaining work: the policy text and the merge skill's decision-record wording. The four missing reviews are already published on those PRs at their merged heads as retrospective reviews, with their non-blocking findings recorded there. Nothing machine-enforced changed, because the machine gate already existed — build_merge_readiness returns current_head_review_missing_or_invalid without a valid exact-head conclusion.
  • Slice boundary / successor: complete within this scope. The remedy is a policy correction, not a new gate; adding a second enforcement layer would duplicate check-merge-readiness.

Validation

  • Tested revision: af1d2d2 (branch head)
  • Run state: finished
  • Input classes: none
Check kind Result Public-safe evidence / limitation
static passed examples/pr-review-command-smoke.py reports pr-review-command-smoke ok; it asserts the merge-skill routing phrases and the repo-only delivery of loopx-pr-merge.
static passed examples/docs-governance-smoke.py reports docs-governance-smoke ok for the changed AGENTS.md.
manual passed Read both edited paragraphs against loopx/capabilities/pr_review_queue/merge_readiness.py, which already returns current_head_review_missing_or_invalid when the exact head has no valid conclusion, so the added policy names an existing gate rather than a new obligation.
  • Coverage and gaps: documentation-only change, no runtime path is touched. The policy cannot be proven by a test; the linked machine gate is the enforceable half and already exists.

Frontend / Visual Evidence

  • UI impact: none
  • Source data: none

Type of Change

  • Documentation update

LoopX Area

  • Public docs or presentation surface (README, protocols, dashboard)

Technical Direction

  • Direction / acceptance reference, when applicable: overall roadmap S12 (release, developer experience and community governance), maintainer review/delivery hygiene.

Shared-authority RFC fixture impact

  • N/A — no TypeScript control-plane migration or shared-authority fixture claim.

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).

`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>

@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.

Exact head: b52f439d1ea0c4b76ef844576c7e84f5efc4b1ff · policy revision 6 · pr-review --check-result 返回 approval_consistent: true。

动机

你指出"自合并的 PR 都需要自 review"。我按这条口径查了 GitHub:本账号合并的 64 个 PR 里有 4 个 reviews=0 —— #4488、#4489、#4491、#4562。它们都是自合并,且合并没有留下任何 review 记录。

更关键的是缺口在哪:skills/loopx-pr-merge/SKILL.md 已经写明"合并、批准、自合并或 admin 绕过都必须有 exact head 的 review 证据",loopx/capabilities/pr_review_queue/merge_readiness.py 也已经在没有有效 exact-head conclusion 时返回 current_head_review_missing_or_invalid。也就是说机器门禁存在,缺的是先读到的策略清单没有写这一条,所以自合并时容易只满足"CI 绿 + 读过 diff"就合并。

改动思路

不新增第二道门禁(那会与 check-merge-readiness 重复),而是把已经存在的义务写进 agent 最先读到的策略位置,并把"决定记在哪里"写死为 exact head 上的已发布 review。

具体改动

  • AGENTS.md:自合并条件清单新增一条——exact head 必须带已发布的 self-review,且 loopx pr-review --check-merge-readiness NUMBER@HEAD_OID 对该未变 head 返回 ready;并说明 GitHub 阻止作者正式自批准,因此 author-owned PR 的记录是带 approval conclusion 与英文 verdict 的 COMMENTED review,"CI 绿 / 读过 diff / 合并本身"都不是这条记录。
  • AGENTS.md:自合并 定义补一句——自合并是"自己 review/refine → 把该 review 发布到 exact head → admin 绕过合并",缺这条记录的合并是流程缺口,要补发布该 merged commit 的 review 并修掉放它过去的规则。
  • skills/loopx-pr-merge/SKILL.md:决策记录明确为"发布在 exact head 上的 review",而不是在别的渠道里做总结。

对主干的风险

纯策略文本,无运行时路径。两处显式区分了 guidance 与 machine-enforced:机器强制的一半是既有 check-merge-readiness,本次文本只是把它写清楚并加上记录格式的期望。风险是文本本身无法被机器证明——但这不是新增空头承诺,因为它指向的门禁已经在跑。

我的整体评价

这是把"流程缺口"落成"可读规则"的最小改动,没有为了显得严格而加第二套门禁。同时我已按此规则为那 4 个 PR 在各自 merged head 上补发了回顾性 review(含非阻断发现),并让本 PR 自己走一遍修正后的流程:capability review → exact-head 发布 → check-merge-readiness → 合并。

Approval conclusion (author-owned PR; GitHub blocks formal self-approval): APPROVE at b52f439d — delivery judgment problem_context = goal_achieved(缺 review 记录的实例已补、规则已成文;机器门禁本就存在,未新增第二套)。

English verdict: APPROVE — at b52f439d, the change states the self-merge condition that the review capability and check-merge-readiness already enforced but the policy list omitted, and it fixes where the decision record is published; the four review-less self-merges (#4488, #4489, #4491, #4562) are repaired with retrospective exact-head reviews that carry non-blocking findings, and this PR is itself reviewed and merged through the corrected flow.

@huangruiteng
huangruiteng merged commit 51492d8 into main Sep 16, 2026
15 of 16 checks passed
@huangruiteng
huangruiteng deleted the codex/selfmerge-review-record branch September 16, 2026 17:17
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

Merge record: exact-head review published at b52f439d (capability result checked, approval_consistent: true), local smokes green (pr-review-command-smoke, docs-governance-smoke), and the merge uses separately authorized admin bypass under the owner's standing instruction not to wait on CI — 13 status checks were still queued at merge time. The bypass is recorded here rather than treated as permission to skip review: the review record above exists at this head.

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