fix: implement PR commit message retrieval and validation#188
Open
shenxianpeng wants to merge 9 commits intomainfrom
Open
fix: implement PR commit message retrieval and validation#188shenxianpeng wants to merge 9 commits intomainfrom
shenxianpeng wants to merge 9 commits intomainfrom
Conversation
Contributor
Commit-Check ❌ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds get_pr_commit_messages() and changes run_commit_check() to, for pull_request events with MESSAGE=="true", run commit-check --message per PR commit message (accumulating return codes) then run non-message checks once; otherwise preserves the previous single-run behavior. Changes
Sequence DiagramsequenceDiagram
participant Runner as Code Runner
participant Main as main.py
participant Git as Git
participant CommitCheck as Commit Check Tool
participant Aggregator as Result Aggregator
Runner->>Main: run_commit_check()
alt PR event && MESSAGE=="true"
Main->>Git: get_pr_commit_messages()
Git-->>Main: [msg1, msg2, ...]
loop per commit message
Main->>CommitCheck: run --message "msgN"
CommitCheck-->>Main: rcN
Main->>Aggregator: accumulate(rcN)
end
Main->>CommitCheck: run non-message checks
CommitCheck-->>Main: rc_nonmsg
Main->>Aggregator: accumulate(rc_nonmsg)
Aggregator-->>Main: total_rc
else
Main->>CommitCheck: run all checks (single run)
CommitCheck-->>Main: total_rc
end
Main-->>Runner: exit(total_rc)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 58-60: The except Exception: pass that swallows all errors around
PR commit retrieval (the block immediately before the return []) must not
silently hide failures; replace the bare pass with explicit error handling:
catch specific expected exceptions where possible or at minimum log the
exception details using logger.exception(...) or similar (e.g.,
process_logger.exception) so the failure and stack trace are recorded, and
decide whether to re-raise the error (to fail fast) or return the empty-list
fallback intentionally; update the except block around the PR commit retrieval
logic to include the chosen logging and/or re-raise behavior instead of pass.
- Around line 108-120: The two subprocess.run calls that assign to result (the
one inside the loop that updates total_rc and the final single-call result when
running "commit-check") are missing text=True, which makes them return
CompletedProcess[bytes] and causes mypy type errors; update both subprocess.run
invocations to include text=True so they consistently return
CompletedProcess[str] (matching the earlier call that already uses text=True)
and preserve existing stdout/stderr handling and check=False behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
main.py (2)
106-116: Consider using spread operator for list construction.Ruff suggests using
["commit-check", *other_args]instead of list concatenation for slightly cleaner syntax.♻️ Proposed style improvement
- command = ["commit-check"] + other_args + command = ["commit-check", *other_args]- command = ["commit-check"] + args + command = ["commit-check", *args]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 106 - 116, Replace list concatenation when building the command with PEP-448 unpacking to improve readability: where the code constructs command = ["commit-check"] + other_args (and later command = ["commit-check"] + args), change these to use the spread/unpacking form command = ["commit-check", *other_args] and command = ["commit-check", *args]; keep the rest of the call sites (e.g., subprocess.run(..., stdout=result_file, stderr=subprocess.PIPE, check=False)) unchanged.
85-113: Return code accumulation may produce values > 1.The
total_rcaccumulates return codes from multiple commit message checks. If several commits fail validation,total_rccould be 2, 3, or higher. While this doesn't break functionality (non-zero still indicates failure), it differs from typical CLI conventions where 1 = failure.Consider whether you want to cap this at 1 or use
total_rc = total_rc or result.returncodefor a boolean-style failure signal. If the current behavior is intentional (e.g., to surface how many commits failed), this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 85 - 113, The loop accumulates numeric return codes into total_rc so multiple failing commits can make total_rc > 1; decide and enforce a single non-zero failure value by capping or using boolean-style accumulation: replace the arithmetic accumulation (total_rc += result.returncode and total_rc += other_result.returncode) with a cap/OR pattern (e.g., set total_rc = min(1, total_rc + result.returncode) or total_rc = total_rc or result.returncode) so total_rc ends up as 0 or 1; update the code paths around total_rc, get_pr_commit_messages, the per-commit subprocess.run calls, and the final return to reflect this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 106-116: Replace list concatenation when building the command with
PEP-448 unpacking to improve readability: where the code constructs command =
["commit-check"] + other_args (and later command = ["commit-check"] + args),
change these to use the spread/unpacking form command = ["commit-check",
*other_args] and command = ["commit-check", *args]; keep the rest of the call
sites (e.g., subprocess.run(..., stdout=result_file, stderr=subprocess.PIPE,
check=False)) unchanged.
- Around line 85-113: The loop accumulates numeric return codes into total_rc so
multiple failing commits can make total_rc > 1; decide and enforce a single
non-zero failure value by capping or using boolean-style accumulation: replace
the arithmetic accumulation (total_rc += result.returncode and total_rc +=
other_result.returncode) with a cap/OR pattern (e.g., set total_rc = min(1,
total_rc + result.returncode) or total_rc = total_rc or result.returncode) so
total_rc ends up as 0 or 1; update the code paths around total_rc,
get_pr_commit_messages, the per-commit subprocess.run calls, and the final
return to reflect this change.
There was a problem hiding this comment.
Pull request overview
This PR enhances the action’s commit-check execution in pull_request runs by retrieving all PR commit messages and validating each message individually (instead of validating only the synthetic merge commit message).
Changes:
- Added PR commit message retrieval via
git log HEAD^1..HEAD^2forpull_requestevents. - Updated
run_commit_check()to run--messagechecks per commit message, then run other enabled checks once. - Preserved the previous “single command” behavior when commit messages aren’t available or message checks are disabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
@shenxianpeng I've opened a new pull request, #189, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@shenxianpeng I've opened a new pull request, #190, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
1-286:⚠️ Potential issue | 🟡 MinorPlease run Black to unblock CI.
The current pipeline reports a Black reformat diff for this file. Re-run formatting locally and commit the result so pre-commit passes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 1 - 286, The file main.py needs to be reformatted with Black to match the repository's style; run Black using the project's configuration (e.g., pip install black && black . or black main.py), stage and commit the resulting changes so pre-commit/CI stops reporting a reformat diff; you can verify by running the formatter and re-running pre-commit or CI locally. Locate main.py (symbols to inspect before/after: run_commit_check, add_job_summary, add_pr_comments, log_error_and_exit, main) to confirm only formatting changes were applied. Ensure you commit the formatted file and push the commit to unblock CI.
♻️ Duplicate comments (1)
main.py (1)
58-60:⚠️ Potential issue | 🟠 MajorDon’t silently suppress PR commit retrieval failures.
Line 58 catches everything and Line 59 drops it, so PR-message validation can quietly degrade to fallback behavior without any signal.
Suggested fix
- except Exception: - pass + except (OSError, subprocess.SubprocessError) as e: + print( + f"::warning::Failed to retrieve PR commit messages: {e}", + file=sys.stderr, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 58 - 60, The code currently swallows all errors in the "except Exception:" block and returns an empty list, hiding PR commit retrieval failures; change that block to "except Exception as e:" and either log the error with stacktrace (e.g., logger.exception or similar) and re-raise the exception or return an explicit error/result state instead of silently returning [] so callers can detect failures—update the exception handler tied to the PR commit retrieval logic (the current except block and the subsequent "return []") to emit the error and propagate or surface it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 85-113: The loop over PR commit messages currently sums return
codes into total_rc (total_rc += result.returncode), which can overflow past 255
and wrap to 0; change the accumulation to preserve a nonzero failure without
additive growth — e.g., set total_rc = max(total_rc, result.returncode) (or set
a boolean failure flag and map to 1 on exit) when processing each result (the
variables/functions to change: total_rc, result.returncode,
other_result.returncode in the PR branch, and the get_pr_commit_messages /
MESSAGE handling block); ensure the final exit code is clamped to a valid 0-255
value before returning.
---
Outside diff comments:
In `@main.py`:
- Around line 1-286: The file main.py needs to be reformatted with Black to
match the repository's style; run Black using the project's configuration (e.g.,
pip install black && black . or black main.py), stage and commit the resulting
changes so pre-commit/CI stops reporting a reformat diff; you can verify by
running the formatter and re-running pre-commit or CI locally. Locate main.py
(symbols to inspect before/after: run_commit_check, add_job_summary,
add_pr_comments, log_error_and_exit, main) to confirm only formatting changes
were applied. Ensure you commit the formatted file and push the commit to
unblock CI.
---
Duplicate comments:
In `@main.py`:
- Around line 58-60: The code currently swallows all errors in the "except
Exception:" block and returns an empty list, hiding PR commit retrieval
failures; change that block to "except Exception as e:" and either log the error
with stacktrace (e.g., logger.exception or similar) and re-raise the exception
or return an explicit error/result state instead of silently returning [] so
callers can detect failures—update the exception handler tied to the PR commit
retrieval logic (the current except block and the subsequent "return []") to
emit the error and propagate or surface it.
shenxianpeng
changed the title
shenxianpeng
force-pushed
the
bugfix/issue-184
branch
from
fef03f0 to
44e900c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.py (2)
1-1:⚠️ Potential issue | 🔴 CriticalFix Black formatting to unblock CI.
The pipeline failure indicates Black reformatted
main.py. Runblack main.pylocally and commit the changes to fix the CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 1, CI is failing because main.py is not formatted by Black; run Black on the file (e.g., black main.py) or apply Black's formatting changes to main.py and commit the updated file so the shebang and file conform to Black's style; ensure the committed change updates main.py to match the formatter used by the repository.
273-276:⚠️ Potential issue | 🟡 MinorSame exit-code overflow risk in
main().The additive pattern
ret_code += add_job_summary() + add_pr_comments()has the same overflow risk mentioned earlier. Whileadd_job_summary()andadd_pr_comments()currently return 0 or 1, combining withrun_commit_check()'s accumulated value could still overflow.Suggested fix
# Combine return codes ret_code = run_commit_check() - ret_code += add_job_summary() - ret_code += add_pr_comments() + ret_code = max(ret_code, add_job_summary()) + ret_code = max(ret_code, add_pr_comments())Or use a boolean-based approach consistent with the suggested fix in
run_commit_check().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 273 - 276, The current additive pattern in main() (ret_code = run_commit_check(); ret_code += add_job_summary(); ret_code += add_pr_comments()) risks overflow if many components return non-zero; replace the additive accumulation with a boolean/ capped approach: call run_commit_check(), add_job_summary(), and add_pr_comments(), convert each result to a boolean (or normalize to 0/1) and then set ret_code = 1 if any call returned non-zero (e.g., ret_code = int(bool(run_commit_check()) or bool(add_job_summary()) or bool(add_pr_comments()))), or use ret_code = max(run_commit_check(), add_job_summary(), add_pr_comments()) to cap the exit code at 1; update main() to use one of these patterns and keep function return semantics for run_commit_check, add_job_summary, and add_pr_comments unchanged.
🧹 Nitpick comments (1)
main.py (1)
106-106: Use spread operator for list construction.Static analysis suggests using unpacking instead of concatenation for cleaner list construction.
Suggested fix
- command = ["commit-check"] + other_args + command = ["commit-check", *other_args]- command = ["commit-check"] + args + command = ["commit-check", *args]Also applies to: 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 106, The list construction currently concatenates a single-item list with other_args (assignments to the variable command), so replace those concatenations with Python list unpacking using the * operator to build the list in-place (i.e., make the single command string the first element and unpack other_args into the rest) for both the "commit-check" occurrence and the other similar assignment; update the command assignments that reference other_args accordingly so they use unpacking instead of the + operator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main.py`:
- Line 1: CI is failing because main.py is not formatted by Black; run Black on
the file (e.g., black main.py) or apply Black's formatting changes to main.py
and commit the updated file so the shebang and file conform to Black's style;
ensure the committed change updates main.py to match the formatter used by the
repository.
- Around line 273-276: The current additive pattern in main() (ret_code =
run_commit_check(); ret_code += add_job_summary(); ret_code +=
add_pr_comments()) risks overflow if many components return non-zero; replace
the additive accumulation with a boolean/ capped approach: call
run_commit_check(), add_job_summary(), and add_pr_comments(), convert each
result to a boolean (or normalize to 0/1) and then set ret_code = 1 if any call
returned non-zero (e.g., ret_code = int(bool(run_commit_check()) or
bool(add_job_summary()) or bool(add_pr_comments()))), or use ret_code =
max(run_commit_check(), add_job_summary(), add_pr_comments()) to cap the exit
code at 1; update main() to use one of these patterns and keep function return
semantics for run_commit_check, add_job_summary, and add_pr_comments unchanged.
---
Nitpick comments:
In `@main.py`:
- Line 106: The list construction currently concatenates a single-item list with
other_args (assignments to the variable command), so replace those
concatenations with Python list unpacking using the * operator to build the list
in-place (i.e., make the single command string the first element and unpack
other_args into the rest) for both the "commit-check" occurrence and the other
similar assignment; update the command assignments that reference other_args
accordingly so they use unpacking instead of the + operator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38dd1294-06d3-4e82-bf94-49b050347fee
📒 Files selected for processing (3)
.pre-commit-config.yamlmain.pyrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #184
Summary by CodeRabbit