diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 03b2ce5bb2..6b1105ab88 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,20 @@ 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 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. + - 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 +104,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/request-coderabbit-test-instructions.yml b/.github/workflows/request-coderabbit-test-instructions.yml index d39b73009a..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,19 +109,46 @@ 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** - **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)_ + + 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)` + **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 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) + 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 + ``` + **Guidelines:** - Include tests affected directly OR via fixture setup/teardown, `yield from` cleanup, or transitive utility call chains (caller calls modified helper) @@ -143,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 new file mode 100644 index 0000000000..04fff281fb --- /dev/null +++ b/.github/workflows/unresolve-coderabbit-threads.yml @@ -0,0 +1,137 @@ +# 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] + +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 + +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 }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + run: | + set -euo pipefail + + # 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 + } + } + } + pageInfo { + hasNextPage + endCursor + } + } + } + } + } + ' -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 "$ALL_THREADS" | jq -r --arg pr_author "$PR_AUTHOR" ' + .[] + | select(.isResolved == true) + | select(.opening_comment.nodes[0].author.login == "coderabbitai") + | select( + # 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" + if gh api graphql -f query=" + mutation { + unresolveReviewThread(input: {threadId: \"$thread_id\"}) { + thread { + id + isResolved + } + } + } + "; 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" + else + echo " Warning: failed to unresolve $thread_id" + fi + fi + done + + echo "Done checking CodeRabbit threads."