Conversation
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
| if (progressUpdater) progressUpdater(updateString) | ||
| if (!isTerminalInteractive()) { | ||
| args.push('-c', 'core.askpass=true') | ||
| } |
There was a problem hiding this comment.
Non-interactive clone: -c core.askpass=true is appended after clone and won’t apply
git -c key=value must come before the subcommand. Appending -c core.askpass=true after clone args results in git clone ... -c core.askpass=true ..., which does not set process config. In CI/non-interactive runs, cloning private repos may prompt for credentials and hang instead of failing fast.
Evidence:
const args = ['clone', '--recurse-submodules']
...
if (!isTerminalInteractive()) {
args.push('-c', 'core.askpass=true')
}
await execa('git', args)
| // git check-ignore exits with code 1 when no files are ignored | ||
| if (error instanceof AbortError) return [] | ||
| throw error | ||
| } |
There was a problem hiding this comment.
checkIfIgnoredInGitRepository: swallows all git failures as “no ignores”
gitCommand wraps any execa error into AbortError. checkIfIgnoredInGitRepository then treats any AbortError as the benign “exit code 1 => no ignored files” case, conflating real failures (not a repo, invalid paths, permissions, etc.) with “no matches” and returning [] silently.
Evidence:
} catch (error) {
// git check-ignore exits with code 1 when no files are ignored
if (error instanceof AbortError) return []
throw error
}
| } catch (error) { | ||
| if (error instanceof AbortError) return undefined | ||
| throw error | ||
| } |
There was a problem hiding this comment.
getLatestTag: returns undefined on any git failure due to broad AbortError handling
getLatestTag returns undefined for any AbortError, but gitCommand wraps all git failures as AbortError. This misreports repo errors (not a git repo, permission denied, etc.) as “no tags exist”.
| } catch (error) { | ||
| if (error instanceof Error && 'exitCode' in error) return false | ||
| throw error | ||
| } |
There was a problem hiding this comment.
insideGitDirectory: returns false for any error with an exitCode property (too broad)
The function returns false for any Error with an exitCode field, which can include missing git, permission errors, or other failures that should likely surface rather than being treated as “not a git repo”.
Code:
} catch (error) {
if (error instanceof Error && 'exitCode' in error) return false
throw error
}
| body: lines[4]!, | ||
| author_name: lines[5]!, | ||
| author_email: lines[6]!, | ||
| } |
There was a problem hiding this comment.
getLatestGitCommit: newline-splitting is brittle; multiline body breaks field mapping
The --format includes %b (body), which can contain arbitrary newlines. Splitting stdout on \n and indexing fixed positions causes mis-assignment of author_name/author_email when the body spans multiple lines.
Evidence:
const format = '%H%n%ai%n%s%n%D%n%b%n%an%n%ae'
const lines = stdout.split('\n')
return { body: lines[4]!, author_name: lines[5]!, author_email: lines[6]! }
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 5 findings 📋 History✅ 5 findings |
ryancbahan
force-pushed
the
remove-simple-git
branch
from
e4b6a6f to
20075f2
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/git.d.ts@@ -1,5 +1,13 @@
import { AbortError } from './error.js';
-import { DefaultLogFields, ListLogLine } from 'simple-git';
+export interface GitLogEntry {
+ hash: string;
+ date: string;
+ message: string;
+ refs: string;
+ body: string;
+ author_name: string;
+ author_email: string;
+}
/**
* Initialize a git repository at the given directory.
*
@@ -64,7 +72,7 @@ export declare function downloadGitRepository(cloneOptions: GitCloneOptions): Pr
* @param directory - The directory of the git repository.
* @returns The latest commit of the repository.
*/
-export declare function getLatestGitCommit(directory?: string): Promise<DefaultLogFields & ListLogLine>;
+export declare function getLatestGitCommit(directory?: string): Promise<GitLogEntry>;
/**
* Add all files to the git index from the given directory.
*
|
Coverage report
Test suite run success3812 tests passing in 1456 suites. Report generated by 🧪jest coverage report action from 0b402f3 |
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
I'm aligned with this approach. AI pair review found some bugs worth double-checking.
Review assisted by pair-review
|
|
||
| // Then | ||
| }).rejects.toThrowError(/Couldn't obtain the most recent tag of the repository http:\/\/repoUrl/) | ||
| }).rejects.toThrowError() |
There was a problem hiding this comment.
Nit: Is there a reason we can't add an error message to this check?
| } catch (err) { | ||
| if (err instanceof Error) { | ||
| const abortError = new AbortError(err.message) | ||
| abortError.stack = err.stack |
There was a problem hiding this comment.
🐛 Bug: The gitCommand helper wraps all errors in AbortError but only preserves the message and stack properties. The exitCode property from execa errors is lost, making it impossible for calling code to distinguish between different types of git failures. This is the root cause of several error handling issues throughout the file: getLatestTag, checkIfIgnoredInGitRepository, and other functions need to check specific exit codes to differentiate expected failures (like 'no tags found') from actual errors (permission denied, git not installed).
Suggestion: Preserve the exitCode property when wrapping errors:
| abortError.stack = err.stack | |
| const abortError = new AbortError(err.message) | |
| abortError.stack = err.stack | |
| if ('exitCode' in err) { | |
| Object.assign(abortError, {exitCode: err.exitCode}) | |
| } | |
| throw abortError |
| return stdout.split('\n').filter(Boolean) | ||
| } catch (error) { | ||
| // git check-ignore exits with code 1 when no files are ignored | ||
| if (error instanceof AbortError) return [] |
There was a problem hiding this comment.
🐛 Bug: The function catches any AbortError and returns an empty array, assuming it means 'no files are ignored'. However, since gitCommand wraps ALL errors in AbortError (including permission errors, invalid directories, or git not being installed), this will silently hide real failures. The comment at line 72 indicates the intent is to handle exit code 1 specifically, but the implementation doesn't check for this.
Suggestion: Check for the specific exit code to distinguish 'no files ignored' from actual errors. Note: This requires fixing the gitCommand helper to preserve exitCode first:
| if (error instanceof AbortError) return [] | |
| if (error instanceof AbortError && 'exitCode' in error && error.exitCode === 1) return [] |
There was a problem hiding this comment.
(Ref Line 152) 🐛 Bug: The progressUpdater parameter is still declared in the GitCloneOptions interface but is no longer used in the implementation (removed at line 160). This is a silent breaking change - callers can pass this parameter expecting progress updates, but it will be ignored without any error or warning. While no code in the codebase currently uses this parameter, it's still part of the public API contract.
Suggestion: Either remove the parameter from the interface to accurately reflect the implementation, or add a deprecation notice:
| /** @deprecated Progress updates are no longer supported after removing simple-git dependency */ | |
| progressUpdater?: (statusString: string) => void |
Alternatively, implement progress tracking using execa's stdio events, or remove it entirely with a migration note.
| ) | ||
| } | ||
| return logs.latest | ||
| const lines = stdout.split('\n') |
There was a problem hiding this comment.
🐛 Bug: The getLatestGitCommit function splits git log output on newlines, but the commit body (%b in the format string) can contain multiple lines. This will cause the parser to incorrectly assign array indices, breaking extraction of author_name and author_email. For example, if a commit body has 3 lines, lines[5] will be the second line of the body instead of the author name.
Suggestion: Use a null byte delimiter that won't appear in commit messages:
| const lines = stdout.split('\n') | |
| const format = '%H%x00%ai%x00%s%x00%D%x00%b%x00%an%x00%ae' | |
| const stdout = await gitCommand(['log', '-1', `--format=${format}`], directory) | |
| if (!stdout.trim()) { | |
| throw new AbortError( | |
| 'Must have at least one commit to run command', | |
| outputContent`Run ${outputToken.genericShellCommand( | |
| "git commit -m 'Initial commit'", | |
| )} to create your first commit.`, | |
| ) | |
| } | |
| const lines = stdout.split('\x00') |
| await execa('git', ['rev-parse', '--git-dir'], {cwd: directory}) | ||
| return true | ||
| } catch (error) { | ||
| if (error instanceof Error && 'exitCode' in error) return false |
There was a problem hiding this comment.
🐛 Bug: The function returns false for any error with an exitCode property, but should specifically check for exit code 128 which indicates 'not a git directory'. Other exit codes might indicate actual errors that should be thrown rather than silently treated as 'not a git repo'.
Suggestion: Check specifically for exit code 128:
| if (error instanceof Error && 'exitCode' in error) return false | |
| if (error instanceof Error && 'exitCode' in error && error.exitCode === 128) return false |
| const stdout = await gitCommand(['describe', '--tags', '--abbrev=0'], directory) | ||
| return stdout.trim() || undefined | ||
| } catch (error) { | ||
| if (error instanceof AbortError) return undefined |
There was a problem hiding this comment.
🐛 Bug: This function catches any AbortError and returns undefined, assuming it means 'no tags exist'. However, this will also hide real errors like permission issues, invalid directories, or git not being installed. The test at line 391 specifically creates an error with exitCode 128 and message 'fatal: No names found', but the implementation doesn't check for these specific conditions - it returns undefined for ANY AbortError.
Suggestion: Check for the specific error condition. Note: This requires fixing the gitCommand helper to preserve exitCode first:
| if (error instanceof AbortError) return undefined | |
| if (error instanceof AbortError && ('exitCode' in error && error.exitCode === 128 || error.message.includes('No names found'))) return undefined |
2 participants
WHY are these changes introduced?
This change removes the dependency on the
simple-gitlibrary and replaces it with directgitcommand execution usingexeca. This PR is part of an assertion I believe to be true: with the rise of agentic development and parallel rise of supply chain attacks, external dependencies have become less valuable and higher risk. We should be looking at our dependencies with a critical eye and removing ones that don't have a very clear value proposition.Using
shopify app initlocally should trigger code path and verify parity behavior.