diff --git a/.agents/skills/github-robot-review-gate/SKILL.md b/.agents/skills/github-robot-review-gate/SKILL.md index 77f7a81f0..845af60a0 100644 --- a/.agents/skills/github-robot-review-gate/SKILL.md +++ b/.agents/skills/github-robot-review-gate/SKILL.md @@ -10,78 +10,95 @@ description: >- ## Core rule -Diagnose the exact merge blocker before changing code or repository settings. -CodeRabbit/check-run success can satisfy this repo's robot-review policy only -when current-head CodeRabbit blocking findings, warnings, and failures are fixed, -rebutted with evidence, or superseded. It is not a GitHub `APPROVED` review. If -GitHub rulesets require human approval, fix the ruleset contract rather than -waiting for humans or disabling security. +Diagnose the exact merge blocker from the live protected branch, rulesets, PR +head, review state, threads, and check runs before changing code or repository +settings. CodeRabbit/check-run success can satisfy this repository's +robot-review evidence path only when current-head blocking findings, warnings, +and failures are fixed, rebutted with evidence, or superseded. Robot-review +evidence does not replace a live GitHub approval-count rule unless the evidence +is itself a qualifying `APPROVED` review object. + +The active organization ruleset currently requires one qualifying independent +approval (`required_approving_review_count=1`) and review-thread resolution. Do +not lower that count or the repository rule's +`require_last_push_approval=true` setting, add a bypass, dismiss a review, or +reinterpret a status or check as an approval merely to land a PR. The approval +must be recorded after the last push. If the canonical organization owner +later changes the rule, refetch the live ruleset first and update this skill and +the merge-gate policy from that evidence. ## Root-cause-first workflow -1. Capture the PR head SHA, mergeability, review decision, required checks, and - rule evaluation before proposing a fix. -2. Separate four signals: GitHub review state, CodeRabbit robot-review evidence, - required status contexts, and ruleset settings. +1. Capture the PR head SHA, base tip, mergeability, review decision, submitted + reviews, unresolved threads, required checks, and active rulesets. +2. Separate four signals: GitHub approval state, robot-review evidence, + required status/workflow contexts, and ruleset settings. 3. Identify the narrow blocker: missing current-head robot evidence, unresolved - robot findings, human-review ruleset count, unresolved threads, stale status - context, or failing check. -4. Apply only the minimal reversible fix, then re-capture the same evidence. + robot findings, missing qualifying independent approval, unresolved threads, + stale status context, queued runner acquisition, or a failing check. +4. Apply only a causal fix inside the correct ownership boundary. Do not mutate + a clean source head just to retrigger infrastructure and do not change the + approval count or bypass policy for an individual PR. +5. Re-capture the same evidence on the exact current head before any lifecycle + action. Predecessor reviews/checks do not transfer after a head movement. ## Evidence commands ```bash gh pr view \ - --json number,headRefOid,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup,latestReviews + --json number,headRefOid,baseRefOid,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup,latestReviews gh pr checks --required gh api repos///pulls//reviews gh api repos///commits//status gh api repos///commits//check-runs gh api repos///rulesets \ - --jq '.[] | {name, enforcement, conditions, rules}' + --jq '.[] | {id, name, enforcement, conditions, rules}' ``` -Record the current head SHA with every screenshot, review, and check summary so -stale evidence is not mistaken for current-head approval. +Record the current head SHA with every review, thread, check, screenshot, and +ruleset summary so stale evidence is not mistaken for current-head evidence. ## Guardrails - Do not bypass branch protection, add bypass actors, use admin merge, force - push, dismiss reviews, or disable security checks unless explicitly requested. -- Do not treat `Review skipped`, CodeRabbit walkthroughs, or check-run success as - a GitHub `APPROVED` review object. They are robot-review gate evidence only. -- Do not wait for human review by default in this repo when robot-review policy - applies; instead verify `required_approving_review_count=0`. -- Do not remove required review thread resolution; keep - `required_review_thread_resolution=true`. + push, destructively rebase, self-approve, dismiss reviews, or disable security + checks for routine delivery. +- Do not lower `required_approving_review_count=1` to unblock a PR. +- Do not treat `Review skipped`, CodeRabbit walkthroughs, overview comments, or + check-run success as a GitHub `APPROVED` review object. +- Keep `required_review_thread_resolution=true`. +- A qualifying independent approval is still required while the live ruleset + requires it, even when current-head robot evidence is otherwise satisfactory. +- Pending, queued, requested, waiting, in-progress, absent-required, cancelled, + failed, stale-head, and predecessor evidence is not permission to merge. ## Stale required status contexts If a PR that hardens or restores a workflow is blocked by a stale required -context (for example `strix` while fixing Strix), document the stale context and -use a temporary, reversible ruleset adjustment only when necessary. Capture -equivalent temporary evidence before merge, such as a trusted-base rerun, -scanner artifact, SARIF output, or manual security review evidence tied to the -current head SHA. The rollback requirement is part of the fix: restore the -`strix` required context immediately after the hardened workflow emits that -context successfully on the protected branch. - -## Safe temporary handling - -- Prefer rerunning or updating the branch before touching rulesets. -- If temporary removal is unavoidable, capture before/after ruleset JSON, owner, - expiry, current head SHA, equivalent temporary evidence, and a dated rollback - note in the PR. -- Restore required contexts and confirm `gh pr checks --required` shows the - hardened context before declaring the gate resolved. +context, prefer rerunning or updating the branch when that preserves exact-head +semantics. If the context cannot be emitted until the repair lands, document the +causal cycle and advance the canonical workflow/ruleset owner path instead of +silently weakening Naruon's gate. + +A temporary required-context adjustment is exceptional. Use it only when the +canonical ruleset owner explicitly authorizes it and equivalent current-head +evidence is captured. Record before/after ruleset JSON, owner, expiry, current +head SHA, equivalent scanner/test evidence, and a named restore condition. The +approval-count rule is not a stale status context and must not be lowered by +this procedure. ## Common mistakes -- Equating CodeRabbit status with GitHub `APPROVED`: treat it as repo - robot-review evidence, then check ruleset review count. -- Waiting for human review despite policy: verify ruleset count is zero and - robot evidence is current-head. -- Removing `strix` permanently to unblock Strix fixes: temporarily remove only - with evidence, then restore once Strix emits. -- Disabling scanners to merge faster: keep security gates on; fix the gate - contract or the failing scanner. +- Equating CodeRabbit or OpenCode status/check evidence with a GitHub approval: + inspect the submitted review object and live approval rule separately. +- Following historical zero-approval guidance after the organization ruleset + changed: live ruleset evidence wins; update stale docs instead of weakening + the rule. +- Treating a queued hosted runner as a product defect: distinguish runner + acquisition/startup failure from executed test failure and advance the + canonical CI owner path while other safe Naruon lanes continue. +- Removing a required scanner permanently to unblock its own repair: preserve + the dependency and use only an owner-authorized, evidenced, reversible + recovery path when a true bootstrap cycle is proven. +- Disabling scanners or dismissing reviews to merge faster: fix the underlying + gate, workflow, source, or ownership-path defect instead. diff --git a/backend/tests/test_merge_gate_live_ruleset_contract.py b/backend/tests/test_merge_gate_live_ruleset_contract.py new file mode 100644 index 000000000..bd91d0240 --- /dev/null +++ b/backend/tests/test_merge_gate_live_ruleset_contract.py @@ -0,0 +1,53 @@ +from pathlib import Path + + +REPOSITORY_ROOT = Path(__file__).resolve().parents[2] +MERGE_GATE_POLICY = REPOSITORY_ROOT / "docs/development/merge-gate-policy.md" +ROBOT_REVIEW_SKILL = ( + REPOSITORY_ROOT / ".agents/skills/github-robot-review-gate/SKILL.md" +) + + +def test_merge_gate_guidance_matches_live_one_approval_ruleset() -> None: + """Operator guidance must not instruct agents to weaken the live review gate.""" + policy = MERGE_GATE_POLICY.read_text(encoding="utf-8") + skill = ROBOT_REVIEW_SKILL.read_text(encoding="utf-8") + + for guidance in (policy, skill): + assert "required_approving_review_count=0" not in guidance + assert "required_approving_review_count=1" in guidance + assert "do not lower" in guidance.lower() + + +def test_live_last_push_approval_rule_is_explicit() -> None: + """Current guidance must preserve the repository's post-last-push approval rule.""" + policy = MERGE_GATE_POLICY.read_text(encoding="utf-8") + skill = ROBOT_REVIEW_SKILL.read_text(encoding="utf-8") + + for guidance in (policy, skill): + assert "require_last_push_approval=true" in guidance + assert "after the last push" in guidance.lower() + + +def test_robot_evidence_is_not_substituted_for_required_approval() -> None: + """Robot evidence and the live GitHub approval requirement remain distinct gates.""" + policy = MERGE_GATE_POLICY.read_text(encoding="utf-8") + + assert "one qualifying independent approval" in policy.lower() + assert "robot-review evidence does not replace" in policy.lower() + + +def test_merge_gate_policy_tracks_live_develop_application_ci_targets() -> None: + """Current operator policy names the protected develop CI path, not only legacy master.""" + policy = MERGE_GATE_POLICY.read_text(encoding="utf-8") + + assert "pull requests to `develop`, `master`, and `release/**`" in policy + assert "pushes to `develop` and `master`" in policy + + +def test_required_neutral_or_skipped_evidence_never_becomes_merge_permission() -> None: + """Required neutral/skipped evidence remains incomplete even if a workflow emits it.""" + policy = MERGE_GATE_POLICY.read_text(encoding="utf-8") + + assert "skipped-required and neutral evidence are non-passing" in policy.lower() + assert "success, pass, skipped, and neutral states satisfy" not in policy.lower() diff --git a/docs/development/merge-gate-policy.md b/docs/development/merge-gate-policy.md index 5875d21a6..60c40d7e1 100644 --- a/docs/development/merge-gate-policy.md +++ b/docs/development/merge-gate-policy.md @@ -1,26 +1,41 @@ # Merge Gate Policy This repository's merge gate is evidence-based: required checks must pass, -review threads must be resolved, and the current PR head must have current-head -CodeRabbit or structured OpenCode App robot-review evidence. Human review is not -awaited by default. +review threads must be resolved, robot-review evidence must be current-head, and +the live rulesets currently require one qualifying independent approval after +the last push. Robot-review evidence does not replace the GitHub approval +requirement unless that evidence is itself carried by a qualifying `APPROVED` +review object. ## Required gate contract - Required status checks must pass on the current head SHA. -- Application CI must run backend pytest and frontend test/lint/build checks on - pull requests to `master` and `release/**`, while release-branch pushes must - not create duplicate check noise; push checks are scoped to `master`. +- Application CI must run backend pytest and frontend test/lint/build checks on pull requests to `develop`, `master`, and `release/**`. +- Application CI pushes to `develop` and `master` are also checked so the current protected/default path is not omitted by legacy master-only guidance. - The robot-review gate prefers CodeRabbit evidence. When the current head has - CodeRabbit check-run evidence, it satisfies the gate only when current-head - blocking findings, warnings, and failures are fixed, rebutted with evidence, - or superseded. Authoritative current-head `Review skipped` evidence satisfies - that path only when applicable. When no CodeRabbit check-run exists, the gate - waits for an exact-current-head `APPROVED` review from the `opencode-agent` - GitHub App. The review body must name the head SHA and include structured - adversarial validation with `status=passed` and at least two probes whose - outcome is `falsified`. Stale-head reviews, `github-actions` reviews, and - insufficient probe evidence do not satisfy the gate. + CodeRabbit check-run evidence, it satisfies the robot-evidence gate only when + current-head blocking findings, warnings, and failures are fixed, rebutted + with evidence, or superseded. Authoritative current-head `Review skipped` + evidence satisfies that robot-evidence path only when it is not itself a + required check result and the same output carries no blocking warning/failure + language. When no CodeRabbit check-run exists, the gate waits for an + exact-current-head `APPROVED` review from the `opencode-agent` GitHub App. The + review body must name the head SHA and include structured adversarial + validation with `status=passed` and at least two probes whose outcome is + `falsified`. Stale-head reviews, `github-actions` reviews, and insufficient + probe evidence do not satisfy the robot-evidence gate. +- The active organization ruleset `CWL Central required workflows` currently + sets `required_approving_review_count=1`. Do not lower that count, add a + bypass, or reinterpret robot status/check evidence as an approval merely to + land a PR. +- The active repository ruleset `Lock default branch` independently sets + `required_approving_review_count=1`, `require_last_push_approval=true`, and + `required_review_thread_resolution=true`, with no repository-ruleset bypass + actor. The qualifying independent approval must therefore be current after + the last push. Any later push invalidates predecessor approval evidence. +- If a canonical ruleset owner later changes these contracts, refetch every + active ruleset first and update this document and the robot-review skill from + that live evidence. Historical repository settings are not current authority. - PR Governance automation is metadata-only: it must not checkout pull request code, clone the head branch, dismiss reviews, enable auto-merge, or use admin merge. It may read PR/check/review-thread metadata and post blocker comments; @@ -32,11 +47,13 @@ awaited by default. Trusted tarball materialization uses bounded retry plus archive validation for transient GitHub API truncation and fails closed instead of falling back to PR-head or local scripts. -- Pending, queued, requested, waiting, or in-progress checks are wait states, not - hard failure findings. Success, pass, skipped, and neutral states satisfy the - gate. Every other required-check state — including failed, cancelled, - timed-out, action-required, and any unrecognized state — is a blocker: the - gate fails closed rather than passing states it does not understand. +- Pending, queued, requested, waiting, and in-progress required checks are wait + states and remain non-passing. +- Skipped-required and neutral evidence are non-passing. Cancelled, failed, + timed-out, action-required, absent-required, + and unrecognized required-check states are blockers. Only exact-current-head + terminal success/pass evidence for every live required context authorizes the + required-check portion of the merge gate. - If gate evaluation itself errors (for example a transient GitHub API failure), the gate publishes a completed/failure check-run instead of leaving a previously published result in place. @@ -44,15 +61,11 @@ awaited by default. only; raw CLI diagnostics stay in the workflow run log. Inside Actions the gate runs with a pinned system PATH so earlier steps cannot influence tool resolution via GITHUB_PATH. -- Authoritative `Review skipped` evidence counts only when the same check - output carries no blocking warning/failure language alongside it. - `reviewDecision=CHANGES_REQUESTED` is a blocker until requested changes are addressed or superseded on the current head. - Blocker comments use the idempotent `` marker and are patched in place instead of duplicated on repeated workflow events. -- GitHub rulesets must use `required_approving_review_count=0` so GitHub does - not require a human `APPROVED` review when robot-review policy applies. - GitHub rulesets must keep `required_review_thread_resolution=true`. - CodeRabbit `request_changes_workflow` stays enabled so the robot can clear its own requested-changes review after comments are resolved. CodeRabbit @@ -60,7 +73,7 @@ awaited by default. evaluated by required checks and PR Governance; duplicating that gate inside CodeRabbit can strand stale GitHub `CHANGES_REQUESTED` review objects when an unrelated scanner is temporarily failing. -- Bypass actors must not be configured for routine delivery. +- Bypass actors must not be configured or used for routine delivery. - Security workflows and scanners are required gates, not optional paths. ## Evidence commands @@ -80,13 +93,18 @@ gh api repos///rulesets --jq '.[] | {name, enforcement, rules}' CodeRabbit review/check evidence satisfies this repo's preferred robot-review path only after current-head blocking comments, pre-merge warnings, and failure -findings are resolved or superseded. When CodeRabbit evidence is absent, an -exact-current-head `APPROVED` review from the `opencode-agent` GitHub App may -satisfy the fallback path only with the structured adversarial evidence defined -above. A `github-actions` review or an overview comment is not authoritative App -approval. If GitHub reports a missing approving review despite valid robot -evidence, inspect the ruleset before waiting for a human review. The expected -setting is `required_approving_review_count=0`. +findings are resolved or superseded. That evidence is distinct from the live +GitHub approval-count rule. A CodeRabbit or OpenCode review satisfies the +approval rule only when GitHub records it as a qualifying current-head +`APPROVED` review; a check-run, walkthrough, overview comment, or +`github-actions` review is not a substitute. + +The effective live rules currently require +`required_approving_review_count=1`, and the repository rule additionally sets +`require_last_push_approval=true`. Do not lower either rule to unblock a PR. +Refetch every active ruleset before each lifecycle decision, then require one +qualifying independent approval after the last push together with current-head +robot evidence, required checks, and thread resolution. ## Stale required contexts @@ -98,32 +116,41 @@ Handling policy: 1. Prefer branch update or rerun first. 2. If the required context cannot be emitted until the PR lands, document the - stale context and use only a temporary, reversible ruleset adjustment. - Capture equivalent temporary evidence before merge, such as a trusted-base - rerun, scanner artifact, SARIF output, or manual security review evidence - tied to the current head SHA. -3. Restore the `strix` required context after the hardened workflow emits it + stale context and use only a temporary, reversible ruleset adjustment when + the canonical ruleset owner explicitly authorizes that path. Capture + equivalent temporary evidence before merge, such as a trusted-base rerun, + scanner artifact, SARIF output, or manual security review evidence tied to + the current head SHA. +3. Restore the required context after the hardened workflow emits it successfully on the protected branch. -4. Re-run required-check evidence after restore. +4. Re-run required-check and live-ruleset evidence after restore. -## PR #108/#109 evidence summary +The approval count and `require_last_push_approval=true` are not stale status +contexts and must not be weakened by this procedure. -- PR #108 exposed the merge-gate ambiguity: CodeRabbit/robot-review evidence was - conflated with a GitHub `APPROVED` review, while ruleset configuration could - still require human approval despite repo policy. -- Issue #109 documents the durable fix: distinguish robot-review evidence from - GitHub approval objects, keep human approval count at zero, preserve review - thread resolution, and handle stale `strix` required contexts with explicit - rollback. -- The root cause was policy/evidence mismatch, not lack of human review. +## PR #108/#109 historical evidence + +PR #108 and issue #109 record an earlier repository policy in which the expected +approval count was zero while robot-review evidence was handled separately. +That history explains the old guidance but is not current merge authority. The +active rulesets now require one approval, including a repository rule requiring +that approval after the last push, so current delivery must follow live ruleset +evidence rather than replaying historical settings. + +The durable lesson from #108/#109 still applies: distinguish robot-review +evidence from GitHub approval objects, preserve review-thread resolution, and +handle stale required contexts with explicit evidence and rollback. Never +change the approval count or required workflows merely because an individual PR +is waiting. ## Rollback and recovery -- Do not add bypass actors, disable security checks, dismiss reviews, or use admin - merge for normal delivery. -- Any temporary ruleset change must have captured before/after JSON, owner, - expiry, head SHA, equivalent temporary evidence, and a named restore - condition. +- Do not add bypass actors, disable security checks, dismiss reviews, use admin + merge, lower the live approval count, or disable post-last-push approval for + normal delivery. +- Any explicitly authorized temporary required-context change must have captured + before/after ruleset JSON, owner, expiry, current head SHA, equivalent + temporary evidence, and a named restore condition. - Restore required contexts immediately after the repaired workflow emits them. - If the platform still rejects merge after policy-aligned settings and passing checks, record the rejection as an external blocker with the exact command