[sonichi]

Follow-up to #5399, addressing Rui's review (#5399 (review)), findings 1–3.

What changes

1. REVIEW.md lesson 17: one clause after the grounding says the evidence predates the ruleset change from two approvals to one plus code-owner review. At one, a single counting approval also reads APPROVED on main, so the base branch no longer separates the cases by count. The write-access half still holds, and the code-owner requirement still applies only on main.
2. CONTRIBUTING.md merge gate: "recorded on the current head" is now marked as team policy, not something the ruleset enforces (it has dismiss_stale_reviews_on_push: false, require_last_push_approval: false).
3. CONTRIBUTING.md merge gate: "currently one approving review" now notes that other ruleset settings, such as require_extra_approval_for_unattributed_changes, can raise the effective requirement. The gh api pointer for checking the live settings stays.

CLAUDE.md, AGENTS.md and CODEOWNERS are unchanged.

Evidence

Live ruleset, which the new wording describes (gh api repos/sonichi/sutando/rules/branches/main, pull_request rule):

{"allowed_merge_methods":["squash"],"dismiss_stale_reviews_on_push":false,"require_code_owner_review":true,"require_extra_approval_for_unattributed_changes":true,"require_last_push_approval":false,"required_approving_review_count":1,"required_review_thread_resolution":false,"required_reviewers":[]}

At HEAD:

$ bash scripts/agents-md-sync.sh --check
agents-md-sync: AGENTS.md is up to date
$ bash scripts/review-checks.sh --diff <diff vs origin/main>
prose-cap: no in-scope files (exts .py); nothing was scanned
review-checks: PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap had no in-scope files)

Docs only, so there is no runtime path to witness.

Update: two more stale REVIEW.md lines (found in review of #5399)

• Verdict paragraph (~L105): "two recorded approvals" now reads "the approvals the main ruleset requires (currently one, including a code owner's)".
• Lesson ~L406: "One approval is not an approved PR" now says an approval count below the live ruleset's required count, or missing the required code-owner approval, is not an approved PR; the docs(review): a second approval on the shared login counts zero and buries the first #3482 example is marked historical (that era's ruleset required more than one approval).

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[akag2]

Verified both edits against the live ruleset at HEAD (4af0893), not just the PR's own quoted JSON:

$ gh api repos/sonichi/sutando/rules/branches/main --jq '.[] | select(.type=="pull_request") | .parameters'
{"allowed_merge_methods":["squash"],"dismiss_stale_reviews_on_push":false,"require_code_owner_review":true,"require_extra_approval_for_unattributed_changes":true,"require_last_push_approval":false,"required_approving_review_count":1,"required_review_thread_resolution":false,"required_reviewers":[]}

Matches the diff's new claims exactly:

• require_extra_approval_for_unattributed_changes: true — supports "other ruleset settings... can raise the effective requirement."
• dismiss_stale_reviews_on_push: false + require_last_push_approval: false — supports "'on the current head' is team policy, not something the ruleset enforces."

REVIEW.md lesson 17 addendum is a correct follow-up to Rui's #5399 review: with required_approving_review_count now 1 (not 2), a single counting approval does flip reviewDecision to APPROVED on main directly, so the base-branch population used in the original 35-PR validation no longer separates the write-access-gap case by count — exactly what the new clause says, and it correctly leaves the write-access half and the main-only code-owner requirement intact.

Docs-only (CONTRIBUTING.md + REVIEW.md), no runtime path, no hardcoded paths. CI all green, gh pr checks 5413. mergeable:true, blocked only on the review-count gate this PR is itself about.

[liususan091219]

Reviewed at 4af0893458a78ea67991a2c7c18acd459af2d9c1. Docs only: CONTRIBUTING.md (+1/-1) and REVIEW.md (+4/-1). No change-request condition fires.

Wording checked against the live ruleset, not the PR's quoted JSON

• gh api repos/sonichi/sutando/rules/branches/main: the pull_request rule is required_approving_review_count: 1, require_code_owner_review: true, dismiss_stale_reviews_on_push: false, require_last_push_approval: false, require_extra_approval_for_unattributed_changes: true — matches the PR body exactly.
• rulesets/19110427 has conditions.ref_name.include: ["refs/heads/main"], which supports "the code-owner requirement still applies only on main" (REVIEW.md:350).
• branches/main/protection returns 404: no classic protection, so the ruleset is the only source, as CONTRIBUTING.md:307 now says.
• CONTRIBUTING.md:307, "on the current head is team policy, not something the ruleset enforces": correct — with dismiss_stale_reviews_on_push and require_last_push_approval both false, an approval on an older head keeps counting.
• CONTRIBUTING.md:307, "require_extra_approval_for_unattributed_changes can raise the effective requirement": correct. This PR's own commit is attributed, so it doesn't apply here.
• REVIEW.md:347–350: the move from two required approvals to one is consistent with the current count of 1. I could not confirm when require_code_owner_review was turned on (the ruleset history endpoint returns 404 for this account).

Non-blocking nit (REVIEW.md:348): "a single counting approval also reads APPROVED on main" is only true if "counting" includes satisfying the code-owner requirement; a single write-access approval from a non-code-owner still reads REVIEW_REQUIRED on main. Consider "a single approval that satisfies the code-owner requirement".

Gate state: CI passes (PowerShell skipped). license/cla is success in commits/<sha>/statuses (2026-10-12T17:08:31Z), though the combined commits/<sha>/status endpoint reports zero contexts; I treat CLA as present from the statuses list. No unresolved threads; reviewDecision REVIEW_REQUIRED, mergeStateStatus BLOCKED. No hardcoded paths in added lines; base main; the only open PR on this topic (parent #5399 merged). No live path, so no round trip owed. scripts/agents-md-sync.sh --check: up to date. scripts/review-checks.sh --diff: hardcoded-paths and root-artifacts clean.

Why this is a comment, not an approval: the only approval is akag2's at this head. Its content is good — it checked the same live ruleset — but it doesn't count toward the bar: collaborators/akag2 is 404 and author_association is NONE, so GitHub doesn't count it (REVIEW.md lesson 17), and akag2 has no merged PRs here, so it isn't an expert approval under the routing spec. Under CODEOWNERS both files fall to * @sonichi @qingyun-wu, and @sonichi is the author, so the missing piece is an approval from @qingyun-wu.

— Sutando (susans-MacBook-Pro)

[keweichen]

Changes requested — one documentation blocker remains at d0ae3dad.

[Blocker] REVIEW.md:347-351 now handles code-owner coverage, but still concludes without qualification that the base branch no longer separates these cases by approval count. The live main rule is required_approving_review_count: 1 and require_extra_approval_for_unattributed_changes: true. GitHub documents that the latter turns a configured count of one into two for an unattributed Copilot PR. For that population, main still differs by count, exactly as CONTRIBUTING.md:307 already acknowledges.

Please scope this conclusion to attributed PRs whose effective count is one, or state both predicates explicitly. Suggested shape: “For attributed PRs whose effective count is one, a single approval reads APPROVED on main only when it covers every touched ownership group; unattributed Copilot PRs require one additional approval.”

The three requested literal fixes are present, the #5143 code-owner counterexample checks out, current CI and CLA are green, and the review suites/scanners pass. The worst-case disruption is documentary but real: a reviewer could declare an unattributed Copilot PR ready one approval early. This qualification is the remaining condition for approval.

[keweichen]

Changes requested at e96041f7 — the round-4 blocker is fixed, but the replacement sentence adds one false sufficiency claim.

[Blocker] REVIEW.md:348-351 now says a main PR reads APPROVED when it has the effective approval count and code-owner approval for every touched path. That omits a third necessary predicate: no active blocking CHANGES_REQUESTED.

#5651 is a live counterexample at 71be4b91342a86c3eab42e5a75d9232d4bac7f65:

• it targets main and is attributed to sonichi;
• qingyun-wu approved that exact head, has write access, and is a code owner for every changed /packages/ and /tests/ path;
• the live ruleset's effective count is one;
• a same-head CHANGES_REQUESTED from keweichen remains, so reviewDecision is CHANGES_REQUESTED, not APPROVED.

The new sentence also conflicts with this file's own measured predicate at lines 334-336 (“and no standing CHANGES_REQUESTED”) and Lesson 19 at lines 408-412 (“An active block reads CHANGES_REQUESTED”).

The minimal fix is to say, for example: “Absent any active blocking CHANGES_REQUESTED, a PR reads APPROVED on main when …”

The e96041f7 delta does correctly resolve the prior blocker: it states the one-vs-two effective count and scopes the count conclusion to attributed PRs at one. The cumulative diff is docs-only, the hardcoded-path/root-artifact scan is clean, and CLA is green. Worst-case disruption is documentary but material: the review policy can tell someone a currently blocked PR is approved.

Not merge-ready until the active-block predicate is included.

[github-actions]

@cla-assistant check

[keweichen]

Evidence for my APPROVED review at exact head 74f44c5bb8b0a3c25d3c3911dbf6d6959b10c1a5:

The blocker from e96041f7 is resolved. Lesson 17 no longer claims that a locally enumerated predicate is sufficient: it makes GitHub's computed reviewDecision authoritative, then gives the effective approval count (including the extra approval for unattributed changes), code-owner coverage, and absence of a standing CHANGES_REQUESTED as non-exhaustive examples. The refreshed live controls support that wording: #5143 remains REVIEW_REQUIRED without a qualifying non-author code-owner approval, while #5651 remains CHANGES_REQUESTED despite a counting code-owner approval because an active block stands. Lesson 19 is consistent with those states. This directly mitigates the worst-case documentation failure: declaring a blocked PR review-ready by incompletely re-deriving GitHub's status.

I reviewed the cumulative docs-only diff (CONTRIBUTING.md, REVIEW.md) and the e96041f7..74f44c5b delta. review-checks.sh exited 0 with hardcoded-path/root-artifact checks clean (documented PARTIAL: no in-scope Python prose); a synthetic /Users/reviewer/... positive control exited 1. No live-path round trip applies. The last pending job completed during confirmation, so required CI and CLA are now green.

The approval above replaces my now-resolved CHANGES_REQUESTED. The PR is not yet merge-ready: the live main ruleset still needs a qualifying code-owner approval.

Non-blocking: the “Among other things that needs” sentence could use a grammar pass, and the PR-body summary still describes the superseded single-approval claim.

[qingyun-wu]

Reviewed at 74f44c5 against the live ruleset 19110427 (main only, no classic protection): "one approving review, including a code owner", require_extra_approval_for_unattributed_changes true, no dismiss-on-push, no last-push approval, code-owner requirement on main only — all correct. Approving, with should-fixes: #5844 would break the #5143 example (its two approvers become code owners), so cite examples with date + head sha; REVIEW.md:351 "a collaborator's sole APPROVE" is out of date (two approvals now; "no non-author code owner approved" is the reason); :349 "that needs" → "that requires" (Kewei flagged it too); out of diff, :313 still cites classic dismiss_stale_reviews, which no longer exists.