fix: add user-friendly error messages for remote git operation failures#1164
fix: add user-friendly error messages for remote git operation failures#1164hztBUAA wants to merge 2 commits intoyamadashy:mainfrom
Conversation
Improve error handling for remote git operations (clone, fetch, ls-remote) by wrapping raw errors in descriptive RepomixError messages that distinguish between timeout, authentication, repository-not-found, and connection errors. This builds on the existing GIT_TERMINAL_PROMPT=0 and 30s timeout to provide clear, actionable error messages when accessing inaccessible repositories. Fixes yamadashy#1077
Summary of ChangesHello @hztBUAA, 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 significantly enhances the error handling for remote Git operations by providing clear, actionable, and user-friendly messages. It centralizes error processing through a new helper function, which intelligently parses raw Git errors to categorize and explain common failure scenarios like timeouts, authentication problems, and repository unavailability. This improvement prevents redundant operations and offers immediate guidance to users, streamlining the debugging process for remote repository interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a centralized error handling mechanism for Git remote operations through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/core/git/gitCommand.ts (1)
201-204:⚠️ Potential issue | 🟡 MinorShort-SHA retry path propagates raw errors instead of
RepomixError.If the full
git fetch origin(line 203) orgit checkout(line 204) fails — for example due to a timeout or authentication issue — the error is not wrapped withcreateGitRemoteError, so the user gets a raw error message instead of the friendly one. This is inconsistent with the other error paths in this function.Suggested fix
// Maybe the error is due to a short SHA, let's try again // Can't use --depth 1 here as we need to fetch the specific commit - await deps.execFileAsync('git', ['-C', directory, 'fetch', 'origin'], gitRemoteOpts); - await deps.execFileAsync('git', ['-C', directory, 'checkout', remoteBranch]); + try { + await deps.execFileAsync('git', ['-C', directory, 'fetch', 'origin'], gitRemoteOpts); + await deps.execFileAsync('git', ['-C', directory, 'checkout', remoteBranch]); + } catch (retryErr) { + throw createGitRemoteError(retryErr, url, 'fetch'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/git/gitCommand.ts` around lines 201 - 204, The short-SHA retry path currently calls deps.execFileAsync('git', ['-C', directory, 'fetch', 'origin'], gitRemoteOpts) and deps.execFileAsync('git', ['-C', directory, 'checkout', remoteBranch]) without wrapping failures, so propagate failures using createGitRemoteError like the other branches: catch errors from both fetch and checkout (around the execFileAsync calls), pass the caught error into createGitRemoteError along with gitRemoteOpts (and any context such as directory/remoteBranch), and rethrow the resulting RepomixError so callers receive the consistent error type.
🧹 Nitpick comments (4)
src/core/git/gitCommand.ts (3)
14-59:createGitRemoteErroris well-structured; one robustness nit on the generic fallback.When
erroris not anErrorinstance (e.g., a string ornullthrown from a dependency),err.messageat line 58 will beundefined, producing a message like"Git fetch failed for url: undefined". Consider a defensive fallback:Suggested improvement
- // Generic fallback - return new RepomixError(`Git ${operation} failed for ${url}: ${err.message}`); + // Generic fallback + const message = err.message || String(error); + return new RepomixError(`Git ${operation} failed for ${url}: ${message}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/git/gitCommand.ts` around lines 14 - 59, The generic fallback in createGitRemoteError can produce "undefined" when error isn't an Error; update createGitRemoteError (and its use of ExecFileError) to defensively derive a safe message and stderr: compute a string message from the incoming error (e.g., if typeof error === 'string' use it, else use err?.message ?? String(error) ?? '') and use that when building stderr and the final RepomixError; ensure all accesses use optional chaining (err?.stderr) to avoid exceptions when error is null/primitive so the final message never contains "undefined".
39-44: The'not found'substring check is broad.
stderr.includes('not found')could match unrelated messages (e.g.,"credential helper not found"). Consider tightening the match — for instance, checking for'not found'only after a pattern likeremote:or combining it with the other more specific checks, or removing the generic'not found'and relying on'does not exist'and'Repository not found'which are more precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/git/gitCommand.ts` around lines 39 - 44, The check for repository-not-found is too broad because stderr.includes('not found') can match unrelated messages; update the condition that builds the RepomixError to only treat "not found" as a repo error when it's clearly a remote response (e.g., require stderr.toLowerCase().includes('remote:') && stderr.toLowerCase().includes('not found')) or remove the generic 'not found' check entirely and rely on the more specific checks ('does not exist' and 'Repository not found'); modify the condition around stderr (the variables stderr, operation, url and the RepomixError creation in gitCommand.ts) accordingly so only the tightened match triggers the Repository not found error.
165-167:git initandgit remote addfailures propagate as raw errors.Lines 166–167 are not inside the try-catch block that starts at line 168. If these local git commands fail (e.g., invalid directory, permissions), the user receives a raw error rather than a
RepomixError. Since these are local operations (not remote),createGitRemoteErrormay not be the right wrapper, but you might still want aRepomixErrorwith a clear message for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/git/gitCommand.ts` around lines 165 - 167, The two local git invocations guarded by remoteBranch (deps.execFileAsync('git', ['-C', directory, 'init']) and deps.execFileAsync('git', ['-C', directory, 'remote', 'add', '--', 'origin', url'])) are executed outside the try-catch that throws createGitRemoteError, so failures surface as raw errors; wrap these calls in their own try-catch and rethrow a RepomixError (or construct a new error type consistent with existing flows) with a clear message like "failed to initialize local git repository" or "failed to add git remote" and include the original error details, referencing deps.execFileAsync, remoteBranch and createGitRemoteError/RepomixError to locate and apply the change.tests/core/git/gitCommand.test.ts (1)
503-573: Consider adding a test for the short-SHA retry path failure.The integration tests don't cover the scenario where the short-SHA retry (
git fetch originwithout--depth 1) itself fails — e.g., times out or hits an auth error. This relates to the unhandled error path at lines 203–204 ingitCommand.ts. A test here would help catch that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/git/gitCommand.test.ts` around lines 503 - 573, Add a test exercising the short‑SHA retry failure in execGitShallowClone: mock execFileAsync to succeed for git init and git remote add, then reject the first git fetch (triggering the short‑SHA fallback) and also reject the subsequent fallback fetch (e.g., with a timeout or auth error); assert execGitShallowClone rejects with the expected error and that execFileAsync was called the expected number of times (including the fallback), referencing execGitShallowClone and the git fetch/short‑SHA retry path so the unhandled error branch is covered.
🤖 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 `@src/core/git/gitCommand.ts`:
- Around line 201-204: The short-SHA retry path currently calls
deps.execFileAsync('git', ['-C', directory, 'fetch', 'origin'], gitRemoteOpts)
and deps.execFileAsync('git', ['-C', directory, 'checkout', remoteBranch])
without wrapping failures, so propagate failures using createGitRemoteError like
the other branches: catch errors from both fetch and checkout (around the
execFileAsync calls), pass the caught error into createGitRemoteError along with
gitRemoteOpts (and any context such as directory/remoteBranch), and rethrow the
resulting RepomixError so callers receive the consistent error type.
---
Nitpick comments:
In `@src/core/git/gitCommand.ts`:
- Around line 14-59: The generic fallback in createGitRemoteError can produce
"undefined" when error isn't an Error; update createGitRemoteError (and its use
of ExecFileError) to defensively derive a safe message and stderr: compute a
string message from the incoming error (e.g., if typeof error === 'string' use
it, else use err?.message ?? String(error) ?? '') and use that when building
stderr and the final RepomixError; ensure all accesses use optional chaining
(err?.stderr) to avoid exceptions when error is null/primitive so the final
message never contains "undefined".
- Around line 39-44: The check for repository-not-found is too broad because
stderr.includes('not found') can match unrelated messages; update the condition
that builds the RepomixError to only treat "not found" as a repo error when it's
clearly a remote response (e.g., require
stderr.toLowerCase().includes('remote:') && stderr.toLowerCase().includes('not
found')) or remove the generic 'not found' check entirely and rely on the more
specific checks ('does not exist' and 'Repository not found'); modify the
condition around stderr (the variables stderr, operation, url and the
RepomixError creation in gitCommand.ts) accordingly so only the tightened match
triggers the Repository not found error.
- Around line 165-167: The two local git invocations guarded by remoteBranch
(deps.execFileAsync('git', ['-C', directory, 'init']) and
deps.execFileAsync('git', ['-C', directory, 'remote', 'add', '--', 'origin',
url'])) are executed outside the try-catch that throws createGitRemoteError, so
failures surface as raw errors; wrap these calls in their own try-catch and
rethrow a RepomixError (or construct a new error type consistent with existing
flows) with a clear message like "failed to initialize local git repository" or
"failed to add git remote" and include the original error details, referencing
deps.execFileAsync, remoteBranch and createGitRemoteError/RepomixError to locate
and apply the change.
In `@tests/core/git/gitCommand.test.ts`:
- Around line 503-573: Add a test exercising the short‑SHA retry failure in
execGitShallowClone: mock execFileAsync to succeed for git init and git remote
add, then reject the first git fetch (triggering the short‑SHA fallback) and
also reject the subsequent fallback fetch (e.g., with a timeout or auth error);
assert execGitShallowClone rejects with the expected error and that
execFileAsync was called the expected number of times (including the fallback),
referencing execGitShallowClone and the git fetch/short‑SHA retry path so the
unhandled error branch is covered.
![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
The pull request significantly improves the user experience by providing actionable and descriptive error messages for common Git remote operation failures (timeouts, authentication issues, missing repositories, and connection problems). However, it introduces a potential security risk where sensitive credentials embedded in Git URLs could be leaked in error messages and logs. My feedback focuses on enhancing the robustness of error parsing to handle non-object errors safely, ensuring that the original error context is preserved using the cause property in RepomixError, and redacting credentials from the URL before including it in any error messages.
| export const createGitRemoteError = (error: unknown, url: string, operation: string): RepomixError => { | ||
| const err = error as ExecFileError; | ||
| const stderr = err.stderr || err.message || ''; | ||
|
|
||
| // Timeout: process was killed by the timeout option | ||
| if (err.killed || err.signal === 'SIGTERM') { | ||
| return new RepomixError( | ||
| `Git ${operation} timed out after ${GIT_REMOTE_TIMEOUT / 1000} seconds for ${url}. The repository may be inaccessible, or the network connection is too slow.`, | ||
| ); | ||
| } | ||
|
|
||
| // Authentication failure | ||
| if (stderr.includes('Authentication failed') || stderr.includes('could not read Username')) { | ||
| return new RepomixError( | ||
| `Git ${operation} failed for ${url}: Authentication required. The repository may be private or the URL may be incorrect.`, | ||
| ); | ||
| } | ||
|
|
||
| // Repository not found | ||
| if (stderr.includes('not found') || stderr.includes('does not exist') || stderr.includes('Repository not found')) { | ||
| return new RepomixError( | ||
| `Git ${operation} failed for ${url}: Repository not found. Please verify the URL is correct.`, | ||
| ); | ||
| } | ||
|
|
||
| // Connection errors | ||
| if ( | ||
| stderr.includes('Could not resolve host') || | ||
| stderr.includes('Failed to connect') || | ||
| stderr.includes('Connection refused') | ||
| ) { | ||
| return new RepomixError( | ||
| `Git ${operation} failed for ${url}: Unable to connect to the remote host. Please check your network connection and the URL.`, | ||
| ); | ||
| } | ||
|
|
||
| // Generic fallback | ||
| return new RepomixError(`Git ${operation} failed for ${url}: ${err.message}`); | ||
| }; |
There was a problem hiding this comment.
The createGitRemoteError function includes the raw url in the error message. If the URL contains embedded credentials (e.g., https://user:password@github.com/repo.git), they will be included in the error message, leading to sensitive information leakage. You should redact credentials from the URL before including it in the error message. Additionally, the function currently assumes that the error parameter is an object that can be safely cast to ExecFileError. If error is null or a primitive, accessing its properties will throw a TypeError, masking the original issue. It's also a best practice to preserve the original error context by passing it as the cause to the new RepomixError.
export const createGitRemoteError = (error: unknown, url: string, operation: string): RepomixError => {
const err = error as Partial<ExecFileError>;
const message = err?.message || String(error);
const stderr = err?.stderr || message || '';
// Redact embedded credentials in URLs to avoid PII leakage
const redactedUrl = url.startsWith('https://') ? url.replace(/^(https?://)([^@/]+)@/i, '$1***@') : url;
// Timeout: process was killed by the timeout option
if (err?.killed || err?.signal === 'SIGTERM') {
return new RepomixError(
`Git ${operation} timed out after ${GIT_REMOTE_TIMEOUT / 1000} seconds for ${redactedUrl}. The repository may be inaccessible, or the network connection is too slow.`,
{ cause: error },
);
}
// Authentication failure
if (stderr.includes('Authentication failed') || stderr.includes('could not read Username')) {
return new RepomixError(
`Git ${operation} failed for ${redactedUrl}: Authentication required. The repository may be private or the URL may be incorrect.`,
{ cause: error },
);
}
// Repository not found
if (stderr.includes('not found') || stderr.includes('does not exist') || stderr.includes('Repository not found')) {
return new RepomixError(
`Git ${operation} failed for ${redactedUrl}: Repository not found. Please verify the URL is correct.`,
{ cause: error },
);
}
// Connection errors
if (
stderr.includes('Could not resolve host') ||
stderr.includes('Failed to connect') ||
stderr.includes('Connection refused')
) {
return new RepomixError(
`Git ${operation} failed for ${redactedUrl}: Unable to connect to the remote host. Please check your network connection and the URL.`,
{ cause: error },
);
}
// Generic fallback
return new RepomixError(`Git ${operation} failed for ${redactedUrl}: ${message}`, { cause: error });
};
src/core/git/gitCommand.ts
Outdated
| const execErr = err as ExecFileError; | ||
| if (execErr.killed || execErr.signal === 'SIGTERM') { | ||
| throw createGitRemoteError(err, url, 'fetch'); | ||
| } |
There was a problem hiding this comment.
Similar to the helper function, this direct access to err properties should be made safer to avoid potential crashes if err is not an object. Using optional chaining and a partial cast provides better safety.
const execErr = err as Partial<ExecFileError>;
if (execErr?.killed || execErr?.signal === 'SIGTERM') {
throw createGitRemoteError(err, url, 'fetch');
}…s issues Address PR review feedback from gemini-code-assist and coderabbitai: - Redact embedded credentials from URLs in error messages to prevent sensitive information leakage (e.g. https://user:pass@host -> https://***@host) - Use Partial<ExecFileError> with optional chaining for safe property access, preventing crashes when error is null or a primitive - Preserve original error context via { cause: error } in RepomixError - Tighten 'not found' substring check to avoid false positives from unrelated stderr messages (removed broad 'not found', kept specific 'Repository not found' and 'does not exist') - Wrap git init/remote add in try-catch with descriptive RepomixError - Wrap short-SHA retry path in try-catch with createGitRemoteError - Add tests for credential redaction, non-Error inputs, null inputs, cause preservation, short-SHA retry failure, and git init failure
|
Thanks for the review and feedback. I am following up on this PR now and will either push the requested changes or reply point-by-point shortly. |
|
Quick follow-up: I am reviewing the feedback and will update this PR shortly. |
1 participant
Summary
createGitRemoteErrorhelper that inspects error properties (killed,signal,stderr) to produce clear, actionable error messages for different failure modesexecLsRemote,execGitShallowClone(both clone and fetch paths) with descriptiveRepomixErrormessagescreateGitRemoteError+ 5 integration tests for timeout/error handling in actual git operations)This builds on the existing
GIT_TERMINAL_PROMPT=0and 30s timeout from #1078 to complete the error handling story — users now get messages like:Git clone timed out after 30 seconds for <url>. The repository may be inaccessible...Git ls-remote failed for <url>: Authentication required. The repository may be private...Git clone failed for <url>: Repository not found. Please verify the URL is correct.Fixes #1077
Test plan
gitCommand.test.tspass (13 new tests added)