commit_and_push: check staged content before commit #152
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds a check to prevent creating an empty commit by verifying if there are any staged files. The logic is sound. I've included one suggestion to improve the efficiency of this check by using the exit code from git diff rather than processing its standard output.
TomasTomecek
force-pushed
the
reuse-rebase
branch
from
9901b48 to
8428cd3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful pre-commit check to ensure there are staged changes, preventing empty commits. The logic correctly uses git diff --cached --quiet and halts execution if no files are staged. The addition of a docstring to the run_subprocess function is also a good improvement for code clarity. My review includes one suggestion to make a comment more descriptive, which will improve long-term maintainability.
| ["git", "diff", "--cached", "--quiet"], | ||
| cwd=local_clone, | ||
| ) | ||
| # 1 = staged, 0 = none staged |
There was a problem hiding this comment.
This comment is a bit terse and might be unclear to developers not familiar with the exit codes of git diff. For better readability and maintainability, I suggest making the comment more descriptive.
| # 1 = staged, 0 = none staged | |
| # `git diff --cached --quiet` exits with 1 if there are staged changes, and 0 if there are none. |
There was a problem hiding this comment.
that's literally what my comment says, everyone will easily get it; darling you are just picking nits
beeai/agents/tasks.py
Outdated
| # 1 = staged, 0 = none staged | ||
| if exit_code == 0: | ||
| logger.info("No files staged for commit, halting.") | ||
| return "No files staged for commit, halting." |
There was a problem hiding this comment.
We should probably raise an exception here, this will not halt the process, it will let it continue with MR URL set to that text 🙂
There was a problem hiding this comment.
Are you sure? If we return here, no commit/push/create-mr would happen.
In the workflow I can see that the next step is to comment in Jira, so I expect the agent to set the status to "failed" or something.
There was a problem hiding this comment.
Why would it set status to failed? There is no such logic in the code. It will succeed with merge_request_url: No files staged for commit, halting..
There was a problem hiding this comment.
ah, I can see it now
...and halt if nothing is staged Signed-off-by: Tomas Tomecek <[email protected]> Assisted-by: Cursor(Claude) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
TomasTomecek
force-pushed
the
reuse-rebase
branch
from
8428cd3 to
349bff5
Compare
|
/gemini review |
| available_tools=gateway_tools, | ||
| commit_only=dry_run, | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
Gemini, I know you will not like this but sweetie you need to get over it
There was a problem hiding this comment.
Code Review
This pull request introduces a useful check to prevent empty git commits by verifying if there are any staged files before the commit operation. If no files are staged, it correctly raises a RuntimeError. This exception is then handled gracefully in both backport_agent.py and rebase_agent.py by wrapping the call in a try...except block, which logs the error and updates the state accordingly. This improves the robustness of the agents. My review includes a minor suggestion to improve a code comment for better clarity.
3 participants
...and halt if nothing is staged
I orginally wanted to fix a case so a rebase agent reuses MRs but realized it's already done.
We have two equal rebases here !17, !16 because of a bug fixed in b4017b0