diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 456d47db4b..ee931ca369 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -157,12 +157,30 @@ jobs: MAX_PRS: ${{ github.event.client_payload.max_prs || inputs.max_prs || '100' }} PROJECT_FLOW_INPUT: ${{ github.event.client_payload.project_flow || inputs.project_flow || vars.PROJECT_FLOW || '' }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || github.event.client_payload.pr_number || inputs.pr_number || '' }} - TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_run' || github.event_name == 'push' || github.event_name == 'pull_request_target' || (github.event_name == 'repository_dispatch' && github.event.client_payload.trigger_reviews != false) || inputs.trigger_reviews == true }} + # toJSON(...) != 'false' && toJSON(...) != '"false"' below (not a + # direct comparison against the boolean literal): GitHub Actions + # coerces both `false` and an absent/`null` client_payload property to + # 0 for a mixed-type inequality check, so that direct-comparison form + # is false -- not true, as the "default enabled unless explicitly + # disabled" intent requires -- whenever a repository_dispatch payload + # omits the field entirely. Stringifying first compares 'null' against + # 'false', which is exact and unaffected by that coercion. Verified + # empirically: a targeted merge-scheduler dispatch with no + # trigger_reviews field produced TRIGGER_REVIEWS=false on the old + # direct-comparison expression before this fix. The second clause + # additionally rejects the JSON *string* "false" (toJSON produces + # '"false"', quotes included) -- a real possibility since `gh api -f + # field=false` sends a string, not a boolean (CodeRabbit review + # finding on #1238) -- so a naive `-f trigger_reviews=false` caller is + # also treated as disabled, not silently left enabled. + TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_run' || github.event_name == 'push' || github.event_name == 'pull_request_target' || (github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.trigger_reviews) != 'false' && toJSON(github.event.client_payload.trigger_reviews) != '"false"') || inputs.trigger_reviews == true }} REVIEW_DISPATCH_LIMIT_INPUT: ${{ github.event.client_payload.review_dispatch_limit || inputs.review_dispatch_limit || vars.REVIEW_DISPATCH_LIMIT || '1' }} BRANCH_UPDATE_LIMIT_INPUT: ${{ github.event.client_payload.branch_update_limit || inputs.branch_update_limit || vars.BRANCH_UPDATE_LIMIT || '1' }} - ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'pull_request_target' || github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && github.event.client_payload.enable_auto_merge != false) || inputs.enable_auto_merge == true }} + # Same toJSON(...) coercion fix as TRIGGER_REVIEWS above. + ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'pull_request_target' || github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.enable_auto_merge) != 'false' && toJSON(github.event.client_payload.enable_auto_merge) != '"false"') || inputs.enable_auto_merge == true }} MERGE_MODE: ${{ github.event.client_payload.merge_mode || inputs.merge_mode || vars.PR_MERGE_MODE || 'direct_or_auto' }} - UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'pull_request_target' || github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && github.event.client_payload.update_branches != false) || inputs.update_branches == true }} + # Same toJSON(...) coercion fix as TRIGGER_REVIEWS above. + UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'push' || github.event_name == 'pull_request_target' || github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.update_branches) != 'false' && toJSON(github.event.client_payload.update_branches) != '"false"') || inputs.update_branches == true }} STALE_OPENCODE_MINUTES: ${{ github.event.client_payload.stale_opencode_minutes || inputs.stale_opencode_minutes || vars.STALE_OPENCODE_MINUTES || '90' }} steps: - name: Exchange OpenCode app token for scheduler mutations @@ -616,10 +634,14 @@ jobs: ORG_SWEEP_REVIEW_DISPATCH_LIMIT: ${{ github.event.client_payload.review_dispatch_limit || inputs.review_dispatch_limit || vars.ORG_SWEEP_REVIEW_DISPATCH_LIMIT || '1' }} ORG_SWEEP_STACKED_REVIEW_DISPATCH_LIMIT: ${{ github.event.client_payload.stacked_review_dispatch_limit || vars.ORG_SWEEP_STACKED_REVIEW_DISPATCH_LIMIT || '1' }} ORG_SWEEP_BRANCH_UPDATE_LIMIT: ${{ github.event.client_payload.branch_update_limit || inputs.branch_update_limit || vars.ORG_SWEEP_BRANCH_UPDATE_LIMIT || '1' }} - ORG_SWEEP_TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.trigger_reviews != false || inputs.trigger_reviews == true }} - ORG_SWEEP_ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.enable_auto_merge != false || inputs.enable_auto_merge == true }} + # toJSON(...) coercion fix -- see the TRIGGER_REVIEWS comment earlier + # in this env block for why the natural-looking `!= false` form + # silently defaults to disabled on repository_dispatch, and why the + # JSON string "false" needs its own clause alongside the boolean. + ORG_SWEEP_TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.trigger_reviews) != 'false' && toJSON(github.event.client_payload.trigger_reviews) != '"false"' || inputs.trigger_reviews == true }} + ORG_SWEEP_ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.enable_auto_merge) != 'false' && toJSON(github.event.client_payload.enable_auto_merge) != '"false"' || inputs.enable_auto_merge == true }} ORG_SWEEP_MERGE_MODE: ${{ github.event.client_payload.merge_mode || inputs.merge_mode || 'direct_or_auto' }} - ORG_SWEEP_UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.update_branches != false || inputs.update_branches == true }} + ORG_SWEEP_UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.update_branches) != 'false' && toJSON(github.event.client_payload.update_branches) != '"false"' || inputs.update_branches == true }} ORG_SWEEP_STALE_QUEUE_HOURS: ${{ vars.ORG_SWEEP_STALE_QUEUE_HOURS || '24' }} # The review-dispatch, stacked-review, and branch-update budgets above are organization-wide # per sweep tick (sized to bound LLM review-provider cost/rate exposure, not diff --git a/CHANGELOG.md b/CHANGELOG.md index fc84661ed6..43d45656a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -215,6 +215,21 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Made `pr-review-merge-scheduler.yml`'s `TRIGGER_REVIEWS`, `ENABLE_AUTO_MERGE`, + and `UPDATE_BRANCHES` (and their `ORG_SWEEP_` counterparts) actually default + to enabled on a `repository_dispatch`, as their `!= false` comparisons were + written to intend. GitHub Actions coerces both boolean `false` and an + absent/`null` `client_payload` property to `0` for a mixed-type `!=` + comparison, so a targeted self-service dispatch that simply omitted one of + these fields (rather than explicitly setting it to `false`) silently + produced the same `false` result as an explicit opt-out for that field + specifically — each of the three flags is independent, so a call that + set, say, `trigger_reviews: true` but omitted `enable_auto_merge` still + disabled only the merge step, not the whole dispatch. Replaced the direct + comparison with `toJSON(...) != 'false'`, an exact string comparison + unaffected by that coercion. Verified empirically: an identical dispatch + payload produced `TRIGGER_REVIEWS=false` before this fix and + `TRIGGER_REVIEWS=true` after, with no other change. - Web verification now checks services through local readiness addresses only. Start the backend and frontend on this computer and use their local health URLs when running the check. diff --git a/docs/doctoring/agent-mention-concurrency-isolation.md b/docs/doctoring/agent-mention-concurrency-isolation.md index 1beacd4f8a..4da547e321 100644 --- a/docs/doctoring/agent-mention-concurrency-isolation.md +++ b/docs/doctoring/agent-mention-concurrency-isolation.md @@ -13,6 +13,24 @@ Trusted `@opencode-agent` comments could remain unacknowledged and fail to start None of these defects is evidence that the requesting maintainer, model, repository allowlist, or final review result is invalid. +**Follow-up — scheduler-side root cause of item 4 closed.** The wrapper-side +workaround above (always send `trigger_reviews=true`, `enable_auto_merge`, +and `update_branches` explicitly) remains correct and in place, but it +addressed only one caller. `pr-review-merge-scheduler.yml`'s +`TRIGGER_REVIEWS`/`ENABLE_AUTO_MERGE`/`UPDATE_BRANCHES` env-block expressions +(and their `ORG_SWEEP_` counterparts) used `client_payload. != false`, +which GitHub Actions evaluates identically for an explicit `false` and for a +field the payload never mentions at all (both coerce to `0` on a mixed-type +`!=` comparison) — so *any* `repository_dispatch` caller that omitted one of +these three fields silently got the disabled behavior, not only this +wrapper. Fixed at the scheduler by comparing `toJSON(client_payload.) +!= 'false'` instead, an exact string comparison the `null`/`false` coercion +cannot collapse. Verified empirically against a live dispatch: an identical +payload produced `TRIGGER_REVIEWS=false` before the fix and `true` after. +Every existing explicit-payload caller (including this wrapper) is +unaffected; a caller that now omits the field gets the originally-intended +default instead of a silent no-op. + ## Test-first repair The permanent regression contracts were committed before their corresponding production changes. diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index afd2546e8c..687bd0f0c0 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1536,11 +1536,11 @@ assert_pr_review_merge_scheduler_uses_github_actions_bot_token() { assert_file_contains "$workflow_file" "ORG_SWEEP_ENABLE_AUTO_MERGE: \${{ github.event_name == 'schedule' ||" "scheduled organization sweeps merge approved current heads" assert_file_contains "$workflow_file" "ORG_SWEEP_UPDATE_BRANCHES: \${{ github.event_name == 'schedule' ||" "scheduled organization sweeps refresh eligible stale branches" assert_file_contains "$workflow_file" 'github.event.workflow_run.pull_requests[0].number' "scheduler scopes OpenCode workflow_run events to the completed review PR" - assert_file_contains "$workflow_file" "github.event.client_payload.trigger_reviews != false" "scheduler enables review dispatch by default for default-branch dispatch events" + assert_file_contains "$workflow_file" "toJSON(github.event.client_payload.trigger_reviews) != 'false'" "scheduler enables review dispatch by default for default-branch dispatch events" assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' || github.event_name == 'push'" "scheduler can dispatch a bounded follow-up OpenCode review after review workflow completion" assert_file_contains "$workflow_file" "github.event_name == 'push' || github.event_name == 'pull_request_target'" "scheduler treats base-branch pushes as queue-maintenance events" - assert_file_contains "$workflow_file" "github.event.client_payload.enable_auto_merge != false" "scheduler enables auto-merge by default for default-branch dispatch events" - assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && github.event.client_payload.update_branches != false) || inputs.update_branches == true" "scheduler enables branch updates after review completion or an explicit default-branch dispatch" + assert_file_contains "$workflow_file" "toJSON(github.event.client_payload.enable_auto_merge) != 'false'" "scheduler enables auto-merge by default for default-branch dispatch events" + assert_file_contains "$workflow_file" "github.event_name == 'workflow_run' || (github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.update_branches) != 'false' && toJSON(github.event.client_payload.update_branches) != '\"false\"') || inputs.update_branches == true" "scheduler enables branch updates after review completion or an explicit default-branch dispatch" assert_file_contains "$workflow_file" "review_dispatch_limit:" "scheduler exposes a bounded review dispatch budget" assert_file_contains "$workflow_file" "REVIEW_DISPATCH_LIMIT_INPUT" "scheduler forwards the review dispatch budget to the canonical script" assert_file_contains "$workflow_file" 'review_dispatch_limit="-1"' "scheduler dispatches every eligible same-head review or Strix evidence job immediately unless an explicit budget overrides it" diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 77594cc1fb..6b8bf430e7 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1245,20 +1245,38 @@ def test_org_queue_sweep_manual_cadence_inputs_reach_the_sweep_job() -> None: "ORG_SWEEP_MAX_PRS: ${{ github.event.client_payload.max_prs || inputs.max_prs || vars.ORG_SWEEP_MAX_PRS || '1000' }}" ) in workflow assert ( - "ORG_SWEEP_TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.trigger_reviews != false || inputs.trigger_reviews == true }}" + "ORG_SWEEP_TRIGGER_REVIEWS: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.trigger_reviews) != 'false' && toJSON(github.event.client_payload.trigger_reviews) != '\"false\"' || inputs.trigger_reviews == true }}" in workflow ) assert ( - "ORG_SWEEP_ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.enable_auto_merge != false || inputs.enable_auto_merge == true }}" + "ORG_SWEEP_ENABLE_AUTO_MERGE: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.enable_auto_merge) != 'false' && toJSON(github.event.client_payload.enable_auto_merge) != '\"false\"' || inputs.enable_auto_merge == true }}" ) in workflow assert ( "ORG_SWEEP_MERGE_MODE: ${{ github.event.client_payload.merge_mode || inputs.merge_mode || 'direct_or_auto' }}" in workflow ) assert ( - "ORG_SWEEP_UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && github.event.client_payload.update_branches != false || inputs.update_branches == true }}" + "ORG_SWEEP_UPDATE_BRANCHES: ${{ github.event_name == 'schedule' || github.event_name == 'repository_dispatch' && toJSON(github.event.client_payload.update_branches) != 'false' && toJSON(github.event.client_payload.update_branches) != '\"false\"' || inputs.update_branches == true }}" in workflow ) + # Neither the top-level nor the org-sweep variant of these three flags + # may use the bare `!= false` form: GitHub Actions coerces an absent + # repository_dispatch payload field and boolean `false` to the same + # value (0) for a mixed-type comparison, so `!= false` is false -- + # meaning "not explicitly disabled" -- for both a payload that + # explicitly disabled the flag AND one that never mentioned it at all. + # Verified empirically against a live dispatch before this fix. Nor may + # either variant omit the JSON-string-"false" clause: `gh api -f + # field=false` sends a JSON string, not a boolean, and toJSON of that + # string is '"false"' (quotes included) -- distinct from toJSON(false) + # == 'false' -- so a bare `!= 'false'` alone would leave a naive `-f` + # caller's explicit disable silently ignored (CodeRabbit finding on #1238). + assert "client_payload.trigger_reviews != false" not in workflow + assert "client_payload.enable_auto_merge != false" not in workflow + assert "client_payload.update_branches != false" not in workflow + assert 'toJSON(github.event.client_payload.trigger_reviews) != \'"false"\'' in workflow + assert 'toJSON(github.event.client_payload.enable_auto_merge) != \'"false"\'' in workflow + assert 'toJSON(github.event.client_payload.update_branches) != \'"false"\'' in workflow assert 'if [ "$ORG_SWEEP_TRIGGER_REVIEWS" = "true" ]; then' in workflow assert 'if [ "$ORG_SWEEP_ENABLE_AUTO_MERGE" = "true" ]; then' in workflow assert '--merge-mode "$ORG_SWEEP_MERGE_MODE"' in workflow