From 73ff2d2f45b26856882423d9ae87a76a727ac7cd Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 20 Aug 2026 23:47:14 -0700 Subject: [PATCH] fix(agents): distinguish review routing from enforcement (#126216) --- .agents/skills/openclaw-pr-maintainer/SKILL.md | 5 +++-- .github/CODEOWNERS | 5 +++-- AGENTS.md | 2 +- CONTRIBUTING.md | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.agents/skills/openclaw-pr-maintainer/SKILL.md b/.agents/skills/openclaw-pr-maintainer/SKILL.md index 9d12a8cf2f70..921f7b01f8b3 100644 --- a/.agents/skills/openclaw-pr-maintainer/SKILL.md +++ b/.agents/skills/openclaw-pr-maintainer/SKILL.md @@ -355,8 +355,9 @@ gh search issues --repo openclaw/openclaw --match title,body --limit 50 \ or release-generation mechanics, not a correctness finding. - If bot review conversations exist on your PR, address them and resolve them yourself once fixed. - Leave a review conversation unresolved only when reviewer or maintainer judgment is still needed. -- Interpret CODEOWNERS as ownership routing, not an automatic independent-approval gate. Before calling an owner review missing, resolve the authenticated GitHub writer and check whether that login is an active member/maintainer of every matched owner team (or is the directly listed owner). An owner-authored change plus the lead's completed review satisfies a plain "owner ask/review" requirement. A pending team review request, empty `reviewDecision`, or `mergeStateStatus=UNSTABLE` alone does not prove that a second party is required. -- Require independent approval only when an explicit source says so: branch/ruleset protection, a SHA-bound dependency/security guard, a named security policy, or the user's instruction. The dependency and security-sensitive guards already classify a PR author who is a repository admin or active secops member as trusted; do not invent an additional self-approval requirement after those exact-head checks pass. If an explicit independent gate really remains and the author cannot self-approve, state the distinction once and ask whether to wait or use an available maintainer/admin override—never create a repetitive polling loop. +- Separate repository authorization from GitHub merge enforcement. `CODEOWNERS` routes review requests; a pending request or zero submitted reviews does not prove that approval is mandatory. Restricted/security paths require listed-owner authorship, review, or direction. For governance changes to ownership/review policy itself, explicit direction from an organization owner also satisfies repository policy only when live `GET /orgs/{org}/memberships/{username}` evidence shows `state: active` and `role: admin`. Repository `ADMIN`, `viewerCanAdminister`, and bypass permission do not establish organization ownership. Neither route waives a live GitHub-enforced review rule. +- Before reporting a mandatory approval blocker, inspect live branch protection and every matching ruleset, the PR review decision/requests, and the authenticated actor's permission and bypass state. If using the organization-owner governance route, record the live organization-membership result separately. Name the exact enforced rule and whether it is satisfied. Bypass state is evidence about the likely server outcome, never authorization. If no review rule is enforced, do not stop before native prepare/merge solely because a requested team has not reviewed. +- Explicit user direction resolves this repository-policy question only from the applicable listed owner or through the verified organization-owner governance route; it cannot override server enforcement. If GitHub requires an independent approval and it remains unsatisfied, stop with the exact blocker even when the actor has bypass permission. Otherwise continue through the native landing flow and let its verified merge command exercise the live rule. - Before landing any PR with non-trivial code changes, run fresh `$autoreview` until no accepted/actionable findings remain; prior CI, ClawSweeper, or manual review is not a substitute. Skip only for truly trivial/docs-only changes or when the user explicitly opts out. - When an agent is landing or merging a PR targeting `main`, use only the repo-native `scripts/pr` wrapper: run `scripts/pr review-init `, follow its emitted checkout/guard guidance, initialize and complete review artifacts with `scripts/pr review-artifacts-init `, validate them with `scripts/pr review-validate-artifacts `, then run `OPENCLAW_TESTBOX=1 scripts/pr prepare-run ` and `scripts/pr merge-run `. The Testbox flag is mandatory for agents: it verifies hosted CI/Testbox on the current head or reuses a patch-identical pre-rebase run green within 24 hours instead of running full `pnpm` gates locally. `prepare-run` fails fast; invoke only after exact-head CI is complete and green, and do not idle on `auto-response` or `check-docs`. For owner-approved reviewed fork code without hosted Testbox, use `OPENCLAW_PR_GATES_REMOTE=testbox` instead. Do not rebase only because `main` advanced; behind-main drift is advisory unless strict drift is explicitly enabled, while GitHub still blocks conflicts. - `scripts/pr` gotchas: subcommands require a PR number (no subcommand `--help` placeholder). Artifacts preserve template enum values with evidence detail in summaries; validate before prepare, from PR-head mode (moving main invalidates the main-baseline guard). Review flow: checkout main baseline, then PR, before artifact validation. After every PR push, rerun `scripts/pr review-init`; checkout alone leaves a stale guard SHA. Locally unset `GITHUB_TOKEN`, `GH_TOKEN`, `HOMEBREW_GITHUB_API_TOKEN`; ambient tokens can select an exhausted or wrong identity. Review JSON: land-ready recommendation `READY FOR /prepare-pr`, `issueValidation.status=valid`; never `APPROVE`. After `scripts/pr merge-run` removes its worktree, `cd` to a persistent repo before follow-up commands. diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b6b03c8ff462..79872ebb1d0d 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -3,8 +3,9 @@ # WARNING: GitHub CODEOWNERS uses last-match-wins semantics. # If you add overlapping rules below the secops block, include @openclaw/openclaw-secops -# on those entries too or you can silently remove required secops review. -# Security-sensitive code, config, and docs require secops review. +# on those entries too or you can silently remove secops review routing. +# Security-sensitive code, config, and docs require secops owner involvement +# under repository policy. Live branch/ruleset settings decide merge enforcement. /SECURITY.md @openclaw/openclaw-secops /.github/dependabot.yml @openclaw/openclaw-secops /.github/codeql/ @openclaw/openclaw-secops diff --git a/AGENTS.md b/AGENTS.md index 7fcee3472067..6c08ec46bd94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,7 +15,7 @@ Skills own workflows; root owns hard policy and routing. Product direction and m - Provider model changes: update the owning plugin manifest; after landing, verify `openclaw/catalog/models/v1/catalog.json` refreshes and dispatch the catalog publish workflow when needed. - Live-verify is the default, not a nicety: user-facing behavior gets live-tested through the real flow before landing. Skipping requires a concrete infeasibility stated in the PR, not convenience. Never print secrets. - Missing deps in a normal checkout: `pnpm install`, retry once, then report first actionable error. Worktrees: see Commands — never reconcile there. -- CODEOWNERS: maint/refactor/tests ok. Larger behavior/product/security/ownership: owner ask/review. The authenticated writer counts as the owner when they are an active member/maintainer of the matched CODEOWNERS team; a pending team review request alone does not require a second party. Independent approval is required only when an explicit guard, branch rule, security policy, or user instruction says so. +- `CODEOWNERS` routes reviewers; it does not itself enforce approval. Maint/refactor/tests need no separate owner ask unless a path has explicit restricted/security ownership; those paths need listed-owner involvement. For governance changes to ownership/review policy itself, explicit direction from an organization owner is an alternative only when live GitHub organization membership shows `state: active` and `role: admin`; repository `ADMIN`, `viewerCanAdminister`, or bypass permission alone never qualifies. Larger behavior/product/security/ownership otherwise needs listed-owner involvement. Neither authorization route bypasses a GitHub-enforced review rule; verify live branch protection/rulesets and PR review state before calling approval mandatory. - Product/docs/UI/changelog wording: "plugin/plugins"; `extensions/` is internal. - New channel/plugin/app/doc surface: update `.github/labeler.yml` + GH labels. - New `AGENTS.md`: add sibling `CLAUDE.md` symlink; edit `AGENTS.md` only. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e2751bf5494..2aaf8bfae41b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,7 +76,7 @@ For coordinated change sets that genuinely need more than 20 PRs, join the **#cl - Describe what & why - **Include screenshots** — one showing the problem/before, one showing the fix/after (for UI or visual changes) - Use American English spelling and grammar in code, comments, docs, and UI strings -- Do not edit files covered by `CODEOWNERS` security ownership unless a listed owner explicitly asked for the change or is already reviewing it with you. Treat those paths as restricted review surfaces, not opportunistic cleanup targets. +- Do not edit files covered by `CODEOWNERS` security ownership unless a listed owner authored or explicitly requested the change, or is already reviewing it with you. For governance changes to ownership/review policy itself, explicit direction from an organization owner is also sufficient only when live GitHub organization membership shows `state: active` and `role: admin`; repository `ADMIN`, `viewerCanAdminister`, or bypass permission alone never qualifies. Neither route waives a GitHub-enforced approval rule. Treat those paths as restricted review surfaces, not opportunistic cleanup targets. ## Review Conversations Are Author-Owned