Skip pre-commit hooks for shadow repo (#13331)#13488
Skip pre-commit hooks for shadow repo (#13331)#13488scidomino merged 1 commit intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @vishvananda, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where system-wide Git pre-commit hooks could disrupt the application's internal Git operations by adding the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces the --no-verify flag when creating git snapshots for checkpointing, which prevents failures caused by global pre-commit hooks. The implementation also includes a significant and beneficial refactoring of the snapshotting logic within the useGeminiStream hook, moving it from a useEffect to the processGeminiStreamEvents callback for more timely and robust execution.
My review focuses on the new error handling paths. I've identified one area for improvement in useGeminiStream.ts where the error handling for a falsy commit hash can be made more consistent with other failure cases to ensure the model is always informed of a failed operation, leading to a better conversational experience.
| if (!commitHash) { | ||
| addItem( | ||
| { | ||
| type: MessageType.ERROR, | ||
| text: 'Failed to get a commit hash for the file snapshot. Aborting operation.', | ||
| }, | ||
| Date.now(), | ||
| ); | ||
| setSnapshottingMessage(null); | ||
| cancelOngoingRequest(); | ||
| return StreamProcessingStatus.Completed; | ||
| } |
There was a problem hiding this comment.
The error handling for a falsy commitHash is inconsistent with the error handling in the catch block. Currently, if createFileSnapshot returns a falsy hash without throwing, cancelOngoingRequest() is called. This aborts the operation but does not inform the model of the failure, which can lead to the conversation abruptly ending.
To improve robustness and conversational flow, this case should be handled similarly to a thrown error: by creating an explicit error response and sending it to the model. This allows the model to be aware of the failure and potentially recover or respond appropriately.
I suggest replacing the current implementation with logic that constructs and sends an error response for the failed tool calls.
if (!commitHash) {
const errorMessage =
'Failed to get a commit hash for the file snapshot. Aborting operation.';
addItem(
{
type: MessageType.ERROR,
text: errorMessage,
},
Date.now(),
);
// Inform the model that the tool calls failed.
const erroredToolCalls: TrackedToolCall[] = toolCallRequests.map(
(request) => ({
request,
status: 'error',
response: {
callId: request.callId,
resultDisplay: '',
error: new Error(errorMessage),
errorType: ToolErrorType.UNKNOWN,
responseParts: [
{
functionResponse: {
name: request.name,
response: {
error: errorMessage,
},
},
},
],
},
responseSubmittedToGemini: false,
}),
);
void handleCompletedToolsRef.current!(erroredToolCalls);
setSnapshottingMessage(null);
return StreamProcessingStatus.Completed;
}
vishvananda
force-pushed
the
no-verify
branch
from
bc7fd5d to
de728df
Compare
|
Note the first version of the pr accidentally included #13486 and the comment by the code assist tool was addressed in that PR. |
…emini#13488) Co-authored-by: Vishvananda Abrams <vabrams@google.com>
…emini#13488) Co-authored-by: Vishvananda Abrams <vabrams@google.com>
2 participants
Summary
When there is a globally configured pre-commit hook installed in the system it can cause commits to the shadow repository to fail. The easiest way to work around this is to run the commit with --no-verify.
Details
It is possible that we want to note his issue with documentation instead of running with --no-verify, but I don't know that there is an easy way to fix this in corporate settings where the system administrator globally enforces pre-commit hooks.
Related Issues
Partial for #13331.
How to Validate
Turn on checkpointing and make changes. Note that failures happen silently unless you also include #13486. You can go into the shadow git directory in .gemini/history// and manually add a failing pre-commit hook:
Pre-Merge Checklist