From 213c22805b63bd5b6548d13cbc79c23afb094eb9 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 16:41:49 +0300 Subject: [PATCH 1/7] feat: improve CodeRabbit review guardrails and verification 1. Skip STD/STP checks for utilities/unittests/ directory 2. Add PR verification checklist to test execution plan 3. Auto-unresolve CodeRabbit threads resolved without response 4. Add approval policy for test plan comment responses 5. Guard against rule override attempts via @coderabbitai comments 6. Require real cluster test commands (not just unit tests) when changes affect session/runtime code that unit tests mock Assisted-by: Claude Signed-off-by: rnetser --- .coderabbit.yaml | 29 +++- .github/workflows/guard-coderabbit-rules.yml | 157 ++++++++++++++++++ .../request-coderabbit-test-instructions.yml | 50 ++++++ .../unresolve-coderabbit-threads.yml | 100 +++++++++++ 4 files changed, 334 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/guard-coderabbit-rules.yml create mode 100644 .github/workflows/unresolve-coderabbit-threads.yml diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 03b2ce5bb2..6797b0579b 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -18,6 +18,18 @@ issue_enrichment: {} reviews: # Explicit instructions applied to every review, including PR description validation path_instructions: + - path: "utilities/unittests/**" + instructions: | + ## Unit Tests — Relaxed Rules + Files under `utilities/unittests/` are unit tests for shared utilities. + Do NOT enforce: + - STD docstring format (Preconditions/Steps/Expected sections) + - STP link requirements + - RFE/Jira link requirements + - Coverage tracking rules + These rules apply only to feature/integration tests under `tests/`. + DO still enforce: code quality, type hints, naming conventions, and all other coding standards. + - path: "**" instructions: | ## PR Template Validation @@ -32,6 +44,16 @@ reviews: If any required section is absent, or `What this PR does / why we need it:` has no content, flag it as HIGH severity and ask the author to restore the missing template section(s). + ## Approval Policy + You may approve the PR when ALL of the following are true: + - All your review comments have been addressed (resolved or fixed), OR you had no review comments. + - If you posted a test execution plan comment requesting tests, and the PR author replied + with a comment explaining why the requested tests are not needed or were already covered, + treat that as an acceptable response — do not block approval on the test plan alone. + - The author's explanation must be reasonable and specific (not just "N/A" or "not needed"). + Accept explanations like: "these tests were already run in CI", "this change is docs-only", + "the affected tests are quarantined", or "verified manually on cluster X". + # Assertive profile for strict enforcement of coding standards profile: assertive @@ -78,9 +100,12 @@ reviews: mode: error instructions: | Check ONLY code that is newly added (not modified) in this PR's diff. + SKIP this check entirely for files under `utilities/unittests/` — unit tests + do not require STP/RFE/Jira links. + This applies to two cases: - A) A newly added test file (test_*.py) - B) A newly added test function (def test_*) in an existing test file + A) A newly added test file (test_*.py) under `tests/` + B) A newly added test function (def test_*) in an existing test file under `tests/` For each case, at least one of the following MUST appear in the module, class, or test function docstring: diff --git a/.github/workflows/guard-coderabbit-rules.yml b/.github/workflows/guard-coderabbit-rules.yml new file mode 100644 index 0000000000..a5a605c9d3 --- /dev/null +++ b/.github/workflows/guard-coderabbit-rules.yml @@ -0,0 +1,157 @@ +# description: Detect and flag attempts to teach CodeRabbit rules that contradict +# the repository's coding standards (AGENTS.md, .coderabbit.yaml). +# +# Triggers on PR comments and review comments that mention @coderabbitai with +# instructions to ignore, skip, or override repo rules. + +name: Guard CodeRabbit rules + +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + +permissions: + pull-requests: write + contents: read + +jobs: + guard-rules: + # Only run on PR comments (not issue comments) that mention coderabbitai + if: | + (github.event.issue.pull_request || github.event.pull_request) && + contains(github.event.comment.body, '@coderabbitai') && + !endsWith(github.event.comment.user.login, '[bot]') + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Check for rule override attempts + env: + GH_TOKEN: ${{ secrets.BOT3_TOKEN }} + COMMENT_BODY: ${{ github.event.comment.body }} + COMMENT_AUTHOR: ${{ github.event.comment.user.login }} + COMMENT_ID: ${{ github.event.comment.id }} + COMMENT_URL: ${{ github.event.comment.html_url }} + PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number }} + REPO: ${{ github.repository }} + IS_REVIEW_COMMENT: ${{ github.event_name == 'pull_request_review_comment' }} + # Maintainers from OWNERS file — approvers who can override rules + MAINTAINERS: "rnetser,myakove,dshchedr,vsibirsk" + run: | + set -euo pipefail + + # Skip if the comment author is a maintainer + if echo "$MAINTAINERS" | tr ',' '\n' | grep -qx "$COMMENT_AUTHOR"; then + echo "Comment author $COMMENT_AUTHOR is a maintainer — skipping check." + exit 0 + fi + + BODY_LOWER=$(echo "$COMMENT_BODY" | tr '[:upper:]' '[:lower:]') + + # Patterns that indicate rule override attempts + # These match instructions to CodeRabbit to ignore/skip/disable repo rules + OVERRIDE_PATTERNS=( + "ignore.*rule" + "ignore.*check" + "ignore.*guideline" + "skip.*rule" + "skip.*check" + "skip.*guideline" + "disable.*rule" + "disable.*check" + "don.t check" + "don.t enforce" + "don.t flag" + "don.t require" + "stop checking" + "stop enforcing" + "stop flagging" + "stop requiring" + "never flag" + "never check" + "never enforce" + "never require" + "always allow" + "always accept" + "always approve" + "no.?qa" + "type.?ignore" + "pylint.?disable" + ) + + MATCHED=false + MATCHED_PATTERN="" + for pattern in "${OVERRIDE_PATTERNS[@]}"; do + if echo "$BODY_LOWER" | grep -qP "$pattern"; then + MATCHED=true + MATCHED_PATTERN="$pattern" + break + fi + done + + if [ "$MATCHED" = "false" ]; then + echo "No rule override attempt detected." + exit 0 + fi + + echo "⚠️ Detected rule override attempt by $COMMENT_AUTHOR (pattern: $MATCHED_PATTERN)" + + # Build maintainer mentions + MENTIONS="" + IFS=',' read -ra MAINT_ARRAY <<< "$MAINTAINERS" + for m in "${MAINT_ARRAY[@]}"; do + MENTIONS="$MENTIONS @$m" + done + + WARNING_BODY="⚠️ **Rule override attempt detected** + + A comment by @$COMMENT_AUTHOR appears to instruct CodeRabbit to ignore or override repository coding standards. + + **Comment:** $COMMENT_URL + **Pattern detected:** \`$MATCHED_PATTERN\` + + Repository rules are defined in \`AGENTS.md\` and \`.coderabbit.yaml\` and cannot be overridden via PR comments. Only repo maintainers can modify these rules. + + cc $MENTIONS — please review this comment." + + # Post warning as a PR comment + gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$WARNING_BODY" + + # If it's a review comment that's part of a thread, unresolve it + if [ "$IS_REVIEW_COMMENT" = "true" ]; then + THREAD_ID=$(gh api graphql -f query=' + query($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100) { + nodes { + id + isResolved + comments(first: 20) { + nodes { + databaseId + } + } + } + } + } + } + } + ' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUMBER" \ + --jq ".data.repository.pullRequest.reviewThreads.nodes[] + | select(.comments.nodes[].databaseId == $COMMENT_ID) + | .id" 2>/dev/null || true) + + if [ -n "$THREAD_ID" ]; then + echo "Unresolving thread $THREAD_ID" + gh api graphql -f query=" + mutation { + unresolveReviewThread(input: {threadId: \"$THREAD_ID\"}) { + thread { id isResolved } + } + } + " || echo "Warning: failed to unresolve thread" + fi + fi diff --git a/.github/workflows/request-coderabbit-test-instructions.yml b/.github/workflows/request-coderabbit-test-instructions.yml index d39b73009a..77c0218e9d 100644 --- a/.github/workflows/request-coderabbit-test-instructions.yml +++ b/.github/workflows/request-coderabbit-test-instructions.yml @@ -122,6 +122,56 @@ jobs: - `-m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run) - Tag each listed test or group with its marker when not obvious, e.g. `(gating)` or `(smoke)` + **PR Verification Checklist** + + After the test execution plan, add the following checklist for the PR author to fill in. + The checklist must be formatted as GitHub markdown checkboxes so the author can mark items directly in the comment. + + ``` + --- + ### PR Verification Checklist + _PR author: check each item as you verify it._ + + - [ ] Smoke tests pass (or N/A if Run smoke tests: False) + - [ ] Gating tests pass (or N/A if Run gating tests: False) + - [ ] Affected tests listed above were executed and pass + - [ ] No new test failures introduced + - [ ] Pre-commit hooks pass (`uv run pre-commit run --all-files`) + - [ ] Unit tests pass (`uv run tox -e utilities-unittests`) + ``` + + **Test categories (MANDATORY — always separate in your output):** + + When the affected code runs at session/collection time (conftest fixtures, pytest plugins, + config hooks, session-scoped setup) or modifies runtime behavior that unit tests mock away, + you MUST separate your plan into two sections: + + 1. **Unit tests (no cluster needed)** — isolated tests under `utilities/unittests/` + 2. **Real tests (cluster required)** — concrete `pytest` commands the PR author must run + on a real cluster to verify the change works end-to-end. Include: + - A command for the **error/fix path** (the scenario the PR fixes) + - A command for the **happy path** (regression: the normal case still works) + - Use lightweight tests (e.g., `--collect-only` for startup failures, + a single small test for runtime behavior) + + If the PR only changes test logic (not utilities/libs/conftest), real test commands + are the affected test paths themselves — no separate section needed. + + Example output for a session-startup fix: + ``` + **Unit tests (no cluster needed)** + - `utilities/unittests/test_pytest_utils.py::TestFoo` — validation logic coverage + + **Real tests (cluster required)** + Error path (the fix): + `pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only` + Expected: ValueError with clear message, not IndexError + + Happy path (regression): + `pytest tests/storage/.../test_foo.py --storage-class-matrix= -k test_bar` + Expected: session starts normally + ``` + **Guidelines:** - Include tests affected directly OR via fixture setup/teardown, `yield from` cleanup, or transitive utility call chains (caller calls modified helper) diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml new file mode 100644 index 0000000000..a88d6717d3 --- /dev/null +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -0,0 +1,100 @@ +# description: Unresolve CodeRabbit review threads that were resolved by the PR author +# without addressing them. Runs on every push to a PR. +# +# Logic: If a CodeRabbit review thread was resolved by someone other than +# coderabbitai[bot], and the resolver's last reply does NOT contain a substantive +# response (e.g., "fixed", "addressed", code snippet, or explanation), the thread +# is unresolved so CodeRabbit can re-evaluate it. + +name: Unresolve unaddressed CodeRabbit threads + +on: + pull_request_target: + types: [synchronize] + +permissions: + pull-requests: write + contents: read + +jobs: + unresolve-threads: + if: "!endsWith(github.event.pull_request.user.login, '[bot]')" + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Unresolve prematurely resolved CodeRabbit threads + env: + GH_TOKEN: ${{ secrets.BOT3_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + + # Fetch all resolved review threads from CodeRabbit + THREADS=$(gh api graphql -f query=' + query($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100) { + nodes { + id + isResolved + comments(first: 20) { + nodes { + author { + login + } + body + } + } + } + } + } + } + } + ' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUMBER") + + # Process each resolved thread + echo "$THREADS" | jq -r ' + .data.repository.pullRequest.reviewThreads.nodes[] + | select(.isResolved == true) + | select(.comments.nodes[0].author.login == "coderabbitai[bot]") + | select( + # Check if the last comment is NOT from coderabbitai[bot] + # (meaning a human resolved it) + (.comments.nodes[-1].author.login != "coderabbitai[bot]") + and + # And the human reply is too short to be a real response + # (less than 20 chars = likely just clicked resolve without explaining) + ((.comments.nodes[-1].body | length) < 20) + ) + | .id + ' | while read -r thread_id; do + if [ -n "$thread_id" ]; then + echo "Unresolving thread: $thread_id" + gh api graphql -f query=" + mutation { + unresolveReviewThread(input: {threadId: \"$thread_id\"}) { + thread { + id + isResolved + } + } + } + " || echo " Warning: failed to unresolve $thread_id" + + # Add a comment to the thread explaining why it was unresolved + gh api graphql -f query=" + mutation { + addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: \"$thread_id\", body: \"\u26a0\ufe0f This thread was automatically unresolved because it was resolved without a substantive response. Please address the review comment and explain how it was resolved before resolving this thread again.\"}) { + comment { + id + } + } + } + " || echo " Warning: failed to add comment to $thread_id" + fi + done + + echo "Done checking CodeRabbit threads." From 66f9cd191c49481c6b8eb6fd8584518c9d71ad1d Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 17:03:20 +0300 Subject: [PATCH 2/7] fix: improve test plan checklist and unresolve logic - Format listed tests as GitHub checkboxes for PR author tracking - Remove generic PR verification checklist (tests ARE the checklist) - Remove unit test references from test plan (CI handles those) - Add real cluster test commands section for session/runtime changes - Catch resolve-without-reply (no comment at all) in unresolve workflow - Set trivial reply threshold to 15 chars - Remove guard-coderabbit-rules.yml workflow Assisted-by: Claude Signed-off-by: rnetser --- .github/workflows/guard-coderabbit-rules.yml | 157 ------------------ .../request-coderabbit-test-instructions.yml | 67 +++----- .../unresolve-coderabbit-threads.yml | 18 +- 3 files changed, 34 insertions(+), 208 deletions(-) delete mode 100644 .github/workflows/guard-coderabbit-rules.yml diff --git a/.github/workflows/guard-coderabbit-rules.yml b/.github/workflows/guard-coderabbit-rules.yml deleted file mode 100644 index a5a605c9d3..0000000000 --- a/.github/workflows/guard-coderabbit-rules.yml +++ /dev/null @@ -1,157 +0,0 @@ -# description: Detect and flag attempts to teach CodeRabbit rules that contradict -# the repository's coding standards (AGENTS.md, .coderabbit.yaml). -# -# Triggers on PR comments and review comments that mention @coderabbitai with -# instructions to ignore, skip, or override repo rules. - -name: Guard CodeRabbit rules - -on: - issue_comment: - types: [created] - pull_request_review_comment: - types: [created] - -permissions: - pull-requests: write - contents: read - -jobs: - guard-rules: - # Only run on PR comments (not issue comments) that mention coderabbitai - if: | - (github.event.issue.pull_request || github.event.pull_request) && - contains(github.event.comment.body, '@coderabbitai') && - !endsWith(github.event.comment.user.login, '[bot]') - runs-on: ubuntu-latest - timeout-minutes: 5 - - steps: - - name: Check for rule override attempts - env: - GH_TOKEN: ${{ secrets.BOT3_TOKEN }} - COMMENT_BODY: ${{ github.event.comment.body }} - COMMENT_AUTHOR: ${{ github.event.comment.user.login }} - COMMENT_ID: ${{ github.event.comment.id }} - COMMENT_URL: ${{ github.event.comment.html_url }} - PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number }} - REPO: ${{ github.repository }} - IS_REVIEW_COMMENT: ${{ github.event_name == 'pull_request_review_comment' }} - # Maintainers from OWNERS file — approvers who can override rules - MAINTAINERS: "rnetser,myakove,dshchedr,vsibirsk" - run: | - set -euo pipefail - - # Skip if the comment author is a maintainer - if echo "$MAINTAINERS" | tr ',' '\n' | grep -qx "$COMMENT_AUTHOR"; then - echo "Comment author $COMMENT_AUTHOR is a maintainer — skipping check." - exit 0 - fi - - BODY_LOWER=$(echo "$COMMENT_BODY" | tr '[:upper:]' '[:lower:]') - - # Patterns that indicate rule override attempts - # These match instructions to CodeRabbit to ignore/skip/disable repo rules - OVERRIDE_PATTERNS=( - "ignore.*rule" - "ignore.*check" - "ignore.*guideline" - "skip.*rule" - "skip.*check" - "skip.*guideline" - "disable.*rule" - "disable.*check" - "don.t check" - "don.t enforce" - "don.t flag" - "don.t require" - "stop checking" - "stop enforcing" - "stop flagging" - "stop requiring" - "never flag" - "never check" - "never enforce" - "never require" - "always allow" - "always accept" - "always approve" - "no.?qa" - "type.?ignore" - "pylint.?disable" - ) - - MATCHED=false - MATCHED_PATTERN="" - for pattern in "${OVERRIDE_PATTERNS[@]}"; do - if echo "$BODY_LOWER" | grep -qP "$pattern"; then - MATCHED=true - MATCHED_PATTERN="$pattern" - break - fi - done - - if [ "$MATCHED" = "false" ]; then - echo "No rule override attempt detected." - exit 0 - fi - - echo "⚠️ Detected rule override attempt by $COMMENT_AUTHOR (pattern: $MATCHED_PATTERN)" - - # Build maintainer mentions - MENTIONS="" - IFS=',' read -ra MAINT_ARRAY <<< "$MAINTAINERS" - for m in "${MAINT_ARRAY[@]}"; do - MENTIONS="$MENTIONS @$m" - done - - WARNING_BODY="⚠️ **Rule override attempt detected** - - A comment by @$COMMENT_AUTHOR appears to instruct CodeRabbit to ignore or override repository coding standards. - - **Comment:** $COMMENT_URL - **Pattern detected:** \`$MATCHED_PATTERN\` - - Repository rules are defined in \`AGENTS.md\` and \`.coderabbit.yaml\` and cannot be overridden via PR comments. Only repo maintainers can modify these rules. - - cc $MENTIONS — please review this comment." - - # Post warning as a PR comment - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$WARNING_BODY" - - # If it's a review comment that's part of a thread, unresolve it - if [ "$IS_REVIEW_COMMENT" = "true" ]; then - THREAD_ID=$(gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - nodes { - id - isResolved - comments(first: 20) { - nodes { - databaseId - } - } - } - } - } - } - } - ' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUMBER" \ - --jq ".data.repository.pullRequest.reviewThreads.nodes[] - | select(.comments.nodes[].databaseId == $COMMENT_ID) - | .id" 2>/dev/null || true) - - if [ -n "$THREAD_ID" ]; then - echo "Unresolving thread $THREAD_ID" - gh api graphql -f query=" - mutation { - unresolveReviewThread(input: {threadId: \"$THREAD_ID\"}) { - thread { id isResolved } - } - } - " || echo "Warning: failed to unresolve thread" - fi - fi diff --git a/.github/workflows/request-coderabbit-test-instructions.yml b/.github/workflows/request-coderabbit-test-instructions.yml index 77c0218e9d..1645f784c7 100644 --- a/.github/workflows/request-coderabbit-test-instructions.yml +++ b/.github/workflows/request-coderabbit-test-instructions.yml @@ -116,60 +116,39 @@ jobs: - **Run smoke tests: True / False** — If True, state the dependency path (test → fixture → changed symbol). True ONLY with a verified path. - **Run gating tests: True / False** — If True, state the dependency path. True if any gating-marked test is in the affected set. - **Affected tests to run** _(required when utilities/, libs/, or shared conftest changes — list concrete paths even when smoke is False)_ - - `path/to/test_file.py` - When the entire test file needs verification - - `path/to/test_file.py::TestClass::test_method` - When specific test(s) needed - - `path/to/test_file.py::test_function` - When specific test(s) needed - - `-m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run) - - Tag each listed test or group with its marker when not obvious, e.g. `(gating)` or `(smoke)` - - **PR Verification Checklist** - - After the test execution plan, add the following checklist for the PR author to fill in. - The checklist must be formatted as GitHub markdown checkboxes so the author can mark items directly in the comment. - ``` - --- - ### PR Verification Checklist - _PR author: check each item as you verify it._ - - - [ ] Smoke tests pass (or N/A if Run smoke tests: False) - - [ ] Gating tests pass (or N/A if Run gating tests: False) - - [ ] Affected tests listed above were executed and pass - - [ ] No new test failures introduced - - [ ] Pre-commit hooks pass (`uv run pre-commit run --all-files`) - - [ ] Unit tests pass (`uv run tox -e utilities-unittests`) - ``` + Format EVERY listed test as a GitHub markdown checkbox so the PR author can check them off after running. + Use these formats: + - `- [ ] path/to/test_file.py` - When the entire test file needs verification + - `- [ ] path/to/test_file.py::TestClass::test_method` - When specific test(s) needed + - `- [ ] path/to/test_file.py::test_function` - When specific test(s) needed + - `- [ ] -m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run) + - Tag each listed test or group with its marker when not obvious, e.g. `(gating)` or `(smoke)` - **Test categories (MANDATORY — always separate in your output):** + **Real test commands (MANDATORY when changes affect session/runtime code):** When the affected code runs at session/collection time (conftest fixtures, pytest plugins, config hooks, session-scoped setup) or modifies runtime behavior that unit tests mock away, - you MUST separate your plan into two sections: - - 1. **Unit tests (no cluster needed)** — isolated tests under `utilities/unittests/` - 2. **Real tests (cluster required)** — concrete `pytest` commands the PR author must run - on a real cluster to verify the change works end-to-end. Include: - - A command for the **error/fix path** (the scenario the PR fixes) - - A command for the **happy path** (regression: the normal case still works) - - Use lightweight tests (e.g., `--collect-only` for startup failures, - a single small test for runtime behavior) + you MUST include concrete `pytest` commands the PR author must run on a real cluster + to verify the change works end-to-end. Include: + - A command for the **error/fix path** (the scenario the PR fixes) + - A command for the **happy path** (regression: the normal case still works) + - Use lightweight tests (e.g., `--collect-only` for startup failures, + a single small test for runtime behavior) + - Format each command as a checkbox too - If the PR only changes test logic (not utilities/libs/conftest), real test commands - are the affected test paths themselves — no separate section needed. + If the PR only changes test logic (not utilities/libs/conftest), the affected test + paths themselves serve as the real test commands — no separate section needed. Example output for a session-startup fix: ``` - **Unit tests (no cluster needed)** - - `utilities/unittests/test_pytest_utils.py::TestFoo` — validation logic coverage - **Real tests (cluster required)** - Error path (the fix): - `pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only` - Expected: ValueError with clear message, not IndexError - - Happy path (regression): - `pytest tests/storage/.../test_foo.py --storage-class-matrix= -k test_bar` - Expected: session starts normally + - [ ] Error path (the fix): + `pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only` + Expected: ValueError with clear message, not IndexError + - [ ] Happy path (regression): + `pytest tests/storage/.../test_foo.py --storage-class-matrix= -k test_bar` + Expected: session starts normally ``` **Guidelines:** diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml index a88d6717d3..d74f958175 100644 --- a/.github/workflows/unresolve-coderabbit-threads.yml +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -61,13 +61,17 @@ jobs: | select(.isResolved == true) | select(.comments.nodes[0].author.login == "coderabbitai[bot]") | select( - # Check if the last comment is NOT from coderabbitai[bot] - # (meaning a human resolved it) - (.comments.nodes[-1].author.login != "coderabbitai[bot]") - and - # And the human reply is too short to be a real response - # (less than 20 chars = likely just clicked resolve without explaining) - ((.comments.nodes[-1].body | length) < 20) + # Case 1: Resolved without ANY reply — last comment is still + # from coderabbitai[bot] (human just clicked "Resolve conversation") + (.comments.nodes[-1].author.login == "coderabbitai[bot]") + or + # Case 2: Human replied but with an empty/trivial response + # (less than 15 chars = likely just clicked resolve without explaining) + ( + (.comments.nodes[-1].author.login != "coderabbitai[bot]") + and + ((.comments.nodes[-1].body | length) < 15) + ) ) | .id ' | while read -r thread_id; do From f9a02bb4e64337f840cc271116d27d7d466c57ae Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 18:03:59 +0300 Subject: [PATCH 3/7] fix: address CodeRabbit review comments - Tighten approval gate: require substantive fix or author response, thread "resolved" state alone is not sufficient - Add concurrency control to unresolve workflow to prevent race conditions - Use comments(last: 5) instead of comments(first: 20) to correctly get the most recent comments in threads with many replies Assisted-by: Claude Signed-off-by: rnetser --- .coderabbit.yaml | 6 +++++- .github/workflows/unresolve-coderabbit-threads.yml | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 6797b0579b..6b1105ab88 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -46,7 +46,11 @@ reviews: ## Approval Policy You may approve the PR when ALL of the following are true: - - All your review comments have been addressed (resolved or fixed), OR you had no review comments. + - All your review comments have been addressed with either: + - a code/doc change that fixes the issue, or + - a substantive author response that justifies no code change. + Thread "resolved" state alone is not sufficient. + OR you had no review comments. - If you posted a test execution plan comment requesting tests, and the PR author replied with a comment explaining why the requested tests are not needed or were already covered, treat that as an acceptable response — do not block approval on the test plan alone. diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml index d74f958175..39c0646460 100644 --- a/.github/workflows/unresolve-coderabbit-threads.yml +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -12,7 +12,12 @@ on: pull_request_target: types: [synchronize] +concurrency: + group: unresolve-threads-${{ github.event.pull_request.number }} + cancel-in-progress: true + permissions: + # Required to unresolve review threads and add comments pull-requests: write contents: read @@ -40,7 +45,7 @@ jobs: nodes { id isResolved - comments(first: 20) { + comments(last: 5) { nodes { author { login From 19520dfdfe5ca41543f8267d967bf97846c06d58 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 18:12:57 +0300 Subject: [PATCH 4/7] fix: use plain list instead of checkboxes in test plan PR authors from forks cannot check boxes in bot comments (requires write access). Use plain list format instead. Assisted-by: Claude Signed-off-by: rnetser --- .../request-coderabbit-test-instructions.yml | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/.github/workflows/request-coderabbit-test-instructions.yml b/.github/workflows/request-coderabbit-test-instructions.yml index 1645f784c7..2d43a05fd2 100644 --- a/.github/workflows/request-coderabbit-test-instructions.yml +++ b/.github/workflows/request-coderabbit-test-instructions.yml @@ -117,12 +117,11 @@ jobs: - **Run gating tests: True / False** — If True, state the dependency path. True if any gating-marked test is in the affected set. - **Affected tests to run** _(required when utilities/, libs/, or shared conftest changes — list concrete paths even when smoke is False)_ - Format EVERY listed test as a GitHub markdown checkbox so the PR author can check them off after running. Use these formats: - - `- [ ] path/to/test_file.py` - When the entire test file needs verification - - `- [ ] path/to/test_file.py::TestClass::test_method` - When specific test(s) needed - - `- [ ] path/to/test_file.py::test_function` - When specific test(s) needed - - `- [ ] -m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run) + - `path/to/test_file.py` - When the entire test file needs verification + - `path/to/test_file.py::TestClass::test_method` - When specific test(s) needed + - `path/to/test_file.py::test_function` - When specific test(s) needed + - `-m marker` - When a marker covers multiple affected tests (e.g. `-m gating` only if ALL gating tests in scope need run) - Tag each listed test or group with its marker when not obvious, e.g. `(gating)` or `(smoke)` **Real test commands (MANDATORY when changes affect session/runtime code):** @@ -135,20 +134,19 @@ jobs: - A command for the **happy path** (regression: the normal case still works) - Use lightweight tests (e.g., `--collect-only` for startup failures, a single small test for runtime behavior) - - Format each command as a checkbox too - If the PR only changes test logic (not utilities/libs/conftest), the affected test paths themselves serve as the real test commands — no separate section needed. Example output for a session-startup fix: ``` **Real tests (cluster required)** - - [ ] Error path (the fix): - `pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only` - Expected: ValueError with clear message, not IndexError - - [ ] Happy path (regression): - `pytest tests/storage/.../test_foo.py --storage-class-matrix= -k test_bar` - Expected: session starts normally + Error path (the fix): + `pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only` + Expected: ValueError with clear message, not IndexError + + Happy path (regression): + `pytest tests/storage/.../test_foo.py --storage-class-matrix= -k test_bar` + Expected: session starts normally ``` **Guidelines:** From 85c8e631af1a0db27c47e158498ec6ca83cb9f44 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 18:28:26 +0300 Subject: [PATCH 5/7] fix: add pagination and fix thread opener detection - Paginate reviewThreads with pageInfo to handle PRs with >100 threads - Split comments query: opening_comment (first: 1) for thread opener detection, recent_comments (last: 5) for last reply check - Fixes false negatives when thread opener falls outside last-5 window Assisted-by: Claude Signed-off-by: rnetser --- .../unresolve-coderabbit-threads.yml | 75 +++++++++++++------ 1 file changed, 53 insertions(+), 22 deletions(-) diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml index 39c0646460..6e328e22c2 100644 --- a/.github/workflows/unresolve-coderabbit-threads.yml +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -36,46 +36,77 @@ jobs: run: | set -euo pipefail - # Fetch all resolved review threads from CodeRabbit - THREADS=$(gh api graphql -f query=' - query($owner: String!, $repo: String!, $pr: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr) { - reviewThreads(first: 100) { - nodes { - id - isResolved - comments(last: 5) { - nodes { - author { - login + # Fetch resolved review threads with pagination + ALL_THREADS="[]" + CURSOR="" + while true; do + if [ -n "$CURSOR" ]; then + AFTER_ARG="-f after=$CURSOR" + else + AFTER_ARG="" + fi + PAGE=$(gh api graphql -f query=' + query($owner: String!, $repo: String!, $pr: Int!, $after: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $after) { + nodes { + id + isResolved + opening_comment: comments(first: 1) { + nodes { + author { + login + } + } + } + recent_comments: comments(last: 5) { + nodes { + author { + login + } + body } - body } } + pageInfo { + hasNextPage + endCursor + } } } } } - } - ' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUMBER") + ' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F pr="$PR_NUMBER" $AFTER_ARG) + + # Append nodes to accumulated list + NODES=$(echo "$PAGE" | jq '.data.repository.pullRequest.reviewThreads.nodes') + ALL_THREADS=$(echo "$ALL_THREADS $NODES" | jq -s 'add') + + # Check for next page + HAS_NEXT=$(echo "$PAGE" | jq -r '.data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage') + if [ "$HAS_NEXT" != "true" ]; then + break + fi + CURSOR=$(echo "$PAGE" | jq -r '.data.repository.pullRequest.reviewThreads.pageInfo.endCursor') + done # Process each resolved thread - echo "$THREADS" | jq -r ' - .data.repository.pullRequest.reviewThreads.nodes[] + echo "$ALL_THREADS" | jq -r ' + .[] | select(.isResolved == true) - | select(.comments.nodes[0].author.login == "coderabbitai[bot]") + | select(.opening_comment.nodes[0].author.login == "coderabbitai[bot]") | select( # Case 1: Resolved without ANY reply — last comment is still # from coderabbitai[bot] (human just clicked "Resolve conversation") - (.comments.nodes[-1].author.login == "coderabbitai[bot]") + (.recent_comments.nodes[-1].author.login == "coderabbitai[bot]") or # Case 2: Human replied but with an empty/trivial response # (less than 15 chars = likely just clicked resolve without explaining) ( - (.comments.nodes[-1].author.login != "coderabbitai[bot]") + (.recent_comments.nodes[-1].author.login != "coderabbitai[bot]") and - ((.comments.nodes[-1].body | length) < 15) + ((.recent_comments.nodes[-1].body | length) < 15) ) ) | .id From 257e99cd7d8d4e19d9f745e010a325f2cc2524cd Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 3 Jun 2026 20:08:05 +0300 Subject: [PATCH 6/7] fix: correct bot login, improve heuristic, clarify wording - Fix bot login: coderabbitai[bot] -> coderabbitai (GraphQL returns login without [bot] suffix) - Improve unresolve heuristic: check that NO substantive non-bot reply (>=15 chars) exists anywhere in recent window, not just last comment - Clarify test plan as non-blocking COMMENT flow, not REQUEST_CHANGES - Replace "change request comment" with "inline informational comment" Assisted-by: Claude Signed-off-by: rnetser --- .../request-coderabbit-test-instructions.yml | 12 ++++++------ .../unresolve-coderabbit-threads.yml | 19 +++++++------------ 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/.github/workflows/request-coderabbit-test-instructions.yml b/.github/workflows/request-coderabbit-test-instructions.yml index 2d43a05fd2..7682a9bc5e 100644 --- a/.github/workflows/request-coderabbit-test-instructions.yml +++ b/.github/workflows/request-coderabbit-test-instructions.yml @@ -1,5 +1,4 @@ -# description: Add comment for CodeRabbit to add tests instructions. A change-request comment will be added to the PR. -# Triggers on the 'verified' label (post-approval) or '/test-plan' comment (on-demand during review). +# description: Add comment for CodeRabbit to add test instructions. An inline informational test-plan comment will be added. name: Request CodeRabbit tests instructions @@ -50,7 +49,8 @@ jobs: CRITICAL: You MUST post an inline review comment on the first changed line of the first file. The inline comment should contain the full Test Execution Plan (smoke decision, gating decision, and specific affected tests). - Do NOT submit a formal review - just post the inline comment directly. + Do NOT submit a blocking review event (REQUEST_CHANGES/APPROVE). + Post a single inline PR comment on Files Changed (non-blocking COMMENT flow). As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. You will post an inline review comment with the test execution plan on the first changed file. @@ -109,7 +109,7 @@ jobs: Do NOT include analysis step numbers (1-8) in your visible output. **Your deliverable:** - Your change request comment will be based on the following requirements: + Your inline informational comment will be based on the following requirements: **Test Execution Plan** @@ -170,9 +170,9 @@ jobs: When responding to this test execution plan request, you MUST follow these rules EXACTLY: - 1. **YOUR ONLY DELIVERABLE**: Post an inline review comment containing the test execution plan on the first changed line + 1. **YOUR ONLY DELIVERABLE**: Post one non-blocking inline comment containing the test execution plan on the first changed line 2. **THEN STOP IMMEDIATELY** - Do NOT generate any additional response - 3. **FALLBACK ONLY**: If submitting the review fails after retrying, post as a regular PR comment + 3. **FALLBACK ONLY**: If inline comment API calls fail after retrying, post as a regular PR comment 4. **SILENCE = SUCCESS**: After successfully submitting the review, your task is complete. No confirmation needed. **ABSOLUTE PROHIBITIONS** (violating these creates empty/meaningless reviews): diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml index 6e328e22c2..ac319fa891 100644 --- a/.github/workflows/unresolve-coderabbit-threads.yml +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -95,19 +95,14 @@ jobs: echo "$ALL_THREADS" | jq -r ' .[] | select(.isResolved == true) - | select(.opening_comment.nodes[0].author.login == "coderabbitai[bot]") + | select(.opening_comment.nodes[0].author.login == "coderabbitai") | select( - # Case 1: Resolved without ANY reply — last comment is still - # from coderabbitai[bot] (human just clicked "Resolve conversation") - (.recent_comments.nodes[-1].author.login == "coderabbitai[bot]") - or - # Case 2: Human replied but with an empty/trivial response - # (less than 15 chars = likely just clicked resolve without explaining) - ( - (.recent_comments.nodes[-1].author.login != "coderabbitai[bot]") - and - ((.recent_comments.nodes[-1].body | length) < 15) - ) + # No substantive non-bot reply exists anywhere in the recent window + # (all comments are from coderabbitai, or human replies are < 15 chars) + (all(.recent_comments.nodes[]; + .author.login == "coderabbitai" + or (.body | length) < 15 + )) ) | .id ' | while read -r thread_id; do From 1ba9d1b7edf90c4bd69773de3f610980ffd31832 Mon Sep 17 00:00:00 2001 From: Ron Netser Date: Thu, 4 Jun 2026 19:17:59 +0300 Subject: [PATCH 7/7] fix: require PR author reply and conditional unresolve warning - Check for substantive PR-author reply (not just any non-bot reply) - Only post warning comment if unresolveReviewThread mutation succeeded Signed-off-by: Ron Netser Assisted-by: Claude --- .../unresolve-coderabbit-threads.yml | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/.github/workflows/unresolve-coderabbit-threads.yml b/.github/workflows/unresolve-coderabbit-threads.yml index ac319fa891..04fff281fb 100644 --- a/.github/workflows/unresolve-coderabbit-threads.yml +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -33,6 +33,7 @@ jobs: GH_TOKEN: ${{ secrets.BOT3_TOKEN }} PR_NUMBER: ${{ github.event.pull_request.number }} REPO: ${{ github.repository }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} run: | set -euo pipefail @@ -92,23 +93,22 @@ jobs: done # Process each resolved thread - echo "$ALL_THREADS" | jq -r ' + echo "$ALL_THREADS" | jq -r --arg pr_author "$PR_AUTHOR" ' .[] | select(.isResolved == true) | select(.opening_comment.nodes[0].author.login == "coderabbitai") | select( - # No substantive non-bot reply exists anywhere in the recent window - # (all comments are from coderabbitai, or human replies are < 15 chars) - (all(.recent_comments.nodes[]; - .author.login == "coderabbitai" - or (.body | length) < 15 - )) + # No substantive PR-author reply exists in the recent window + (any(.recent_comments.nodes[]; + .author.login == $pr_author + and ((.body // "") | length) >= 15 + ) | not) ) | .id ' | while read -r thread_id; do if [ -n "$thread_id" ]; then echo "Unresolving thread: $thread_id" - gh api graphql -f query=" + if gh api graphql -f query=" mutation { unresolveReviewThread(input: {threadId: \"$thread_id\"}) { thread { @@ -117,18 +117,20 @@ jobs: } } } - " || echo " Warning: failed to unresolve $thread_id" - - # Add a comment to the thread explaining why it was unresolved - gh api graphql -f query=" - mutation { - addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: \"$thread_id\", body: \"\u26a0\ufe0f This thread was automatically unresolved because it was resolved without a substantive response. Please address the review comment and explain how it was resolved before resolving this thread again.\"}) { - comment { - id + "; then + # Add a comment to the thread explaining why it was unresolved + gh api graphql -f query=" + mutation { + addPullRequestReviewThreadReply(input: {pullRequestReviewThreadId: \"$thread_id\", body: \"\u26a0\ufe0f This thread was automatically unresolved because it was resolved without a substantive response. Please address the review comment and explain how it was resolved before resolving this thread again.\"}) { + comment { + id + } } } - } - " || echo " Warning: failed to add comment to $thread_id" + " || echo " Warning: failed to add comment to $thread_id" + else + echo " Warning: failed to unresolve $thread_id" + fi fi done