Conversation
Proj/git semantics 684
feat: [ENG-685] isomorphic-git status local
feat: [ENG-686] isomorphic-git log local
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (localSha && remoteSha) { | ||
| const matrix = await git.statusMatrix({dir, fs}) | ||
| const dirtyFiles = matrix.filter((row) => row[2] !== 1 || row[3] !== 1).map((row) => String(row[0])) | ||
|
|
There was a problem hiding this comment.
nitpick: The variable names localRef/remoteRef shadow the outer localSha/remoteSha that were assigned just above (lines 606–607). Using the same names for different things in the same scope is confusing, even though the assignments themselves are correct.
| if (localSha && remoteSha) { | |
| const matrix = await git.statusMatrix({dir, fs}) | |
| const dirtyFiles = matrix.filter((row) => row[2] !== 1 || row[3] !== 1).map((row) => String(row[0])) | |
| const localCommitOid = localSha | |
| const remoteCommitOid = remoteSha | |
| const wouldBeOverwritten = await Promise.all( | |
| dirtyFiles.map(async (filepath) => { | |
| const [localFileOid, remoteFileOid] = await Promise.all([ | |
| git | |
| .readBlob({dir, filepath, fs, oid: localCommitOid}) | |
| .then((r) => r.oid) | |
| .catch(() => null), | |
| git | |
| .readBlob({dir, filepath, fs, oid: remoteCommitOid}) | |
| .then((r) => r.oid) | |
| .catch(() => null), | |
| ]) | |
| return localFileOid !== remoteFileOid | |
| }), | |
| ) |
| // Fresh auto-init — remove .git and .gitignore so clone starts clean | ||
| await fs.promises.rm(join(contextTreeDir, '.git'), {force: true, recursive: true}) |
There was a problem hiding this comment.
suggestion: Silently deleting a non-empty .git is risky. The isEmptyRepository guard above prevents this path for non-empty repos, but if isEmptyRepository has a gap (e.g. a repo that has stashes or packed refs but no visible commits/branches), a user's .git could be wiped. Consider logging a warning or requiring an explicit --force acknowledgment before deleting existing .git content.
| } | ||
|
|
||
| return { | ||
| gitDir: join(contextTreeDir, '.git'), |
There was a problem hiding this comment.
nitpick: gitDir (an absolute server filesystem path like /home/user/.brv/...) is returned to the TUI client over the transport. If the client is running on a different machine, or if this path is displayed verbatim, it exposes the daemon's filesystem layout unnecessarily. Consider whether a relative path or a simple boolean gitInitialized: true is sufficient for the client's needs.
|
|
||
| // Set upstream tracking BEFORE push so pull works even if push fails with non_fast_forward |
There was a problem hiding this comment.
suggestion: Upstream tracking is written before the push succeeds (line 923). If the push subsequently fails with a network error or similar non-non_fast_forward error, the tracking is already committed but the branch hasn't been pushed — the user's config is now inconsistent (upstream points to a remote branch that doesn't exist yet).
Consider setting tracking only after a successful push:
| // Set upstream tracking BEFORE push so pull works even if push fails with non_fast_forward | |
| let upstreamSet = false | |
| let alreadyUpToDate = false | |
| try { | |
| const result = await this.gitService.push({branch, directory, remote: 'origin'}) | |
| if (!result.success && result.reason === 'non_fast_forward') { | |
| throw new VcError('Remote has changes. Pull first with brv vc pull.', VcErrorCode.NON_FAST_FORWARD) | |
| } | |
| if (result.success) { | |
| alreadyUpToDate = result.alreadyUpToDate ?? false | |
| if (data.setUpstream) { | |
| await this.gitService.setTrackingBranch({branch, directory, remote: 'origin', remoteBranch: branch}) | |
| upstreamSet = true | |
| } | |
| } |
| return {files, isClean: files.length === 0} | ||
| } | ||
|
|
||
| private buildBasicAuthHeaders(userId: string, sessionKey: string): Record<string, string> { |
There was a problem hiding this comment.
nitpick: Both headers (which include Authorization: Basic ...) and onAuth are passed to every network operation. onAuth is only called by isomorphic-git when headers are absent or when the server returns 401. Since headers are always present, onAuth is effectively unreachable under normal conditions. This is harmless but adds noise. Either rely solely on headers or solely on onAuth — mixing both makes the auth flow harder to reason about.
| try { | ||
| const branch = params.branch ?? (await git.currentBranch({dir, fs})) ?? 'main' |
There was a problem hiding this comment.
nitpick: params.branch is used as both the logical "which branch to push" (line 713) and the ref passed to git.push (line 729). When the user calls brv vc push origin with no branch, branch is resolved to the current branch from git.currentBranch (line 713) — but git.push({ref: undefined}) may behave differently from git.push({ref: 'main'}). Since branch now always has a value, consider passing it explicitly:
| try { | |
| const branch = params.branch ?? (await git.currentBranch({dir, fs})) ?? 'main' | |
| const branch = params.branch ?? (await git.currentBranch({dir, fs})) ?? 'main' | |
| const remote = params.remote ?? 'origin' |
| http, | ||
| onAuth: this.getOnAuth(), | ||
| onAuthFailure: this.getOnAuthFailure(), | ||
| ref: params.branch, |
There was a problem hiding this comment.
bug: params.branch is passed as ref here even though branch (the resolved value) was computed on line 713. If params.branch is undefined, isomorphic-git's git.push({ref: undefined}) defaults to HEAD's branch — which happens to be correct, but it's inconsistent with the resolved branch variable used for all other logic in this method (SHA checks, result message). Use the resolved value:
| ref: params.branch, | |
| ref: branch, |
| * Classify a raw isomorphic-git error into a specific VcError by its `.code` property. | ||
| * Returns undefined if the error is not a recognized isomorphic-git error. | ||
| */ | ||
| function classifyIsomorphicGitError(error: unknown, notFoundCode: VcErrorCodeType): undefined | VcError { |
There was a problem hiding this comment.
nitpick: The cast error as {code: string} bypasses TypeScript's type safety. Since the check 'code' in error has already narrowed error to have a code property, this could be made safer:
| function classifyIsomorphicGitError(error: unknown, notFoundCode: VcErrorCodeType): undefined | VcError { | |
| const code = (error as {code: unknown}).code | |
| if (typeof code !== 'string') return undefined | |
| if (code === 'HttpError' || code === 'SmartHttpError') { |
| async getAheadBehind(params: GetAheadBehindParams): Promise<AheadBehind> { | ||
| const dir = this.requireDirectory(params) | ||
|
|
||
| const localSha = await git.resolveRef({dir, fs, ref: params.localRef}).catch(() => null) |
There was a problem hiding this comment.
nitpick: The ahead/behind calculation is log-based rather than merge-base-based. This is documented ("Counts beyond this are truncated") and acceptable for the depth limit case, but there's a subtler issue: if local and remote have diverged by more than MAX_AHEAD_BEHIND_DEPTH commits, both counts are silently capped. Consider returning a sentinel (e.g. null) or a flag when truncation occurs so the UI can show >500 rather than an inaccurate number.
|
|
||
| export default class VcClone extends Command { | ||
| public static args = { | ||
| url: Args.string({description: `Clone URL (e.g. ${getGitRemoteBaseUrl()}/<team>/<space>.git)`}), |
There was a problem hiding this comment.
nitpick: getGitRemoteBaseUrl() is called at class definition time (module load), not at command execution time. If BRV_GIT_REMOTE_BASE_URL is not set in the environment when the module loads (e.g. in tests), this silently falls back to the hardcoded default rather than throwing. This is intentional per the function's design, but it means the static args and examples fields always show the default URL even when a custom one is configured. No fix needed unless this causes confusion in practice — just documenting the behaviour.
| await this.config.runCommand('vc:init') | ||
| } catch { | ||
| // vc:init logs its own errors |
There was a problem hiding this comment.
suggestion: Swallowing the vc:init error with return leaves the user without output if the sub-command fails for a reason other than logging its own error (e.g. a crash in the command parser). The comment "vc:init logs its own errors" is only true when the command reaches its own error handling — not for oclif framework errors.
Consider at minimum logging the caught error:
| await this.config.runCommand('vc:init') | |
| } catch { | |
| // vc:init logs its own errors | |
| } catch (error) { | |
| // vc:init logs most errors, but log unexpected ones | |
| if (error instanceof Error) this.warn(error.message) | |
| return | |
| } |
|
Claude finished @bao-byterover's task in 4m 44s —— View job Code Review —
|
| // Set upstream tracking BEFORE push so pull works even if push fails with non_fast_forward | ||
| let upstreamSet = false | ||
| if (data.setUpstream) { | ||
| await this.gitService.setTrackingBranch({branch, directory, remote: 'origin', remoteBranch: branch}) | ||
| upstreamSet = true |
There was a problem hiding this comment.
bug: Upstream tracking is persisted before the push attempt. If gitService.push() subsequently throws a network error (or any non-non_fast_forward failure), the tracking config is already committed but the remote branch may not exist yet — leaving branch.${branch}.remote and branch.${branch}.merge pointing to a ghost branch.
The inline comment acknowledges this is intentional for the non_fast_forward case (so the user can pull then re-push), but that reasoning doesn't hold for transient errors where the push never reached the server.
Consider a two-step approach: write tracking after a successful push, and only apply the "set tracking before" optimisation when detecting non-fast-forward so pull can recover:
| // Set upstream tracking BEFORE push so pull works even if push fails with non_fast_forward | |
| let upstreamSet = false | |
| if (data.setUpstream) { | |
| await this.gitService.setTrackingBranch({branch, directory, remote: 'origin', remoteBranch: branch}) | |
| upstreamSet = true | |
| let upstreamSet = false | |
| let alreadyUpToDate = false | |
| try { | |
| const result = await this.gitService.push({branch, directory, remote: 'origin'}) | |
| if (!result.success && result.reason === 'non_fast_forward') { | |
| // Set tracking even on non_fast_forward so `brv vc pull` knows where to pull from. | |
| if (data.setUpstream) { | |
| await this.gitService.setTrackingBranch({branch, directory, remote: 'origin', remoteBranch: branch}) | |
| upstreamSet = true | |
| } | |
| throw new VcError('Remote has changes. Pull first with brv vc pull.', VcErrorCode.NON_FAST_FORWARD) | |
| } | |
| if (result.success) { | |
| alreadyUpToDate = result.alreadyUpToDate ?? false | |
| if (data.setUpstream) { | |
| await this.gitService.setTrackingBranch({branch, directory, remote: 'origin', remoteBranch: branch}) | |
| upstreamSet = true | |
| } | |
| } |
| const fetchResponse = await fetch(`${baseUrl}/git-upload-pack`, { | ||
| body: LS_REFS_REQUEST, | ||
| headers: { | ||
| ...params.headers, | ||
| 'Content-Type': 'application/x-git-upload-pack-request', | ||
| 'Git-Protocol': 'version=2', | ||
| }, | ||
| method: 'POST', | ||
| }) |
There was a problem hiding this comment.
suggestion: This fetch() call has no timeout. If the CoGit server is slow or unresponsive, the ls-refs POST will hang indefinitely, blocking the entire clone/fetch operation with no way for the user to recover other than killing the process.
Add a 30-second AbortSignal timeout (consistent with typical git client behaviour):
| const fetchResponse = await fetch(`${baseUrl}/git-upload-pack`, { | |
| body: LS_REFS_REQUEST, | |
| headers: { | |
| ...params.headers, | |
| 'Content-Type': 'application/x-git-upload-pack-request', | |
| 'Git-Protocol': 'version=2', | |
| }, | |
| method: 'POST', | |
| }) | |
| const controller = new AbortController() | |
| const timeoutId = setTimeout(() => controller.abort(), 30_000) | |
| const fetchResponse = await fetch(`${baseUrl}/git-upload-pack`, { | |
| body: LS_REFS_REQUEST, | |
| headers: { | |
| ...params.headers, | |
| 'Content-Type': 'application/x-git-upload-pack-request', | |
| 'Git-Protocol': 'version=2', | |
| }, | |
| method: 'POST', | |
| signal: controller.signal, | |
| }) | |
| clearTimeout(timeoutId) |
| const fetchResponse = await fetch(url, { | ||
| body: Buffer.from(v2), | ||
| headers: { | ||
| ...params.headers, | ||
| 'Content-Type': 'application/x-git-upload-pack-request', | ||
| 'Git-Protocol': 'version=2', | ||
| }, | ||
| method: 'POST', | ||
| }) |
There was a problem hiding this comment.
suggestion: Same missing timeout as in translateUploadPackV2ToV1 — this pack-data POST can hang indefinitely on an unresponsive server. The same AbortController/clearTimeout pattern from the earlier call should be applied here.
The pack-data response can be large (full clone), so consider a longer timeout (e.g. 120 s) or making it configurable rather than the same 30 s used for ls-refs.
| // Fresh auto-init — remove .git and .gitignore so clone starts clean | ||
| await fs.promises.rm(join(contextTreeDir, '.git'), {force: true, recursive: true}) | ||
| await fs.promises.rm(join(contextTreeDir, '.gitignore'), {force: true}).catch(() => {}) |
There was a problem hiding this comment.
suggestion: The .git directory is deleted silently whenever isEmptyRepository returns true. The guard is reasonable, but isEmptyRepository checks commits, remotes, branches, tags, and clean status — it does not account for the stash reflog or packed-refs that some tools write. A user who ran git stash on an otherwise-empty repo would have their stash silently wiped.
At minimum log a warning so the operation is visible:
| // Fresh auto-init — remove .git and .gitignore so clone starts clean | |
| await fs.promises.rm(join(contextTreeDir, '.git'), {force: true, recursive: true}) | |
| await fs.promises.rm(join(contextTreeDir, '.gitignore'), {force: true}).catch(() => {}) | |
| // Fresh auto-init — remove .git and .gitignore so clone starts clean | |
| this.broadcastToProject(projectPath, VcEvents.CLONE_PROGRESS, { | |
| message: 'Removing existing empty repository before clone...', | |
| step: 'cloning', | |
| }) | |
| await fs.promises.rm(join(contextTreeDir, '.git'), {force: true, recursive: true}) | |
| await fs.promises.rm(join(contextTreeDir, '.gitignore'), {force: true}).catch(() => {}) |
|
|
||
| return { | ||
| gitDir: join(contextTreeDir, '.git'), | ||
| spaceName, |
There was a problem hiding this comment.
nitpick: gitDir returns the daemon's absolute filesystem path (e.g. /home/user/.brv/projects/abc/.git) to the TUI client over the transport layer. If the client and daemon are ever on different machines, this path is meaningless. Even locally, exposing internal filesystem layout is an unnecessary coupling.
The TUI currently uses gitDir to display the path in InitProjectPage. Consider whether a boolean gitInitialized: true or a short relative display string (e.g. .brv/context-tree/.git) is sufficient — and reserve the full absolute path for server-side logging only.
| const localShas = new Set(localLog.map((c) => c.oid)) | ||
| const remoteShas = new Set(remoteLog.map((c) => c.oid)) | ||
|
|
||
| const ahead = localLog.filter((c) => !remoteShas.has(c.oid)).length | ||
| const behind = remoteLog.filter((c) => !localShas.has(c.oid)).length | ||
| return {ahead, behind} |
There was a problem hiding this comment.
suggestion: When either branch has more than MAX_AHEAD_BEHIND_DEPTH (500) commits diverged, the counts are silently truncated. The UI will display ahead: 500, behind: 3 instead of ahead: >500, behind: 3, which is misleading — the user may think the repo is only 500 commits ahead when it could be thousands.
Consider returning a truncated flag or using null as a sentinel for capped values so the presentation layer can render >500:
| const localShas = new Set(localLog.map((c) => c.oid)) | |
| const remoteShas = new Set(remoteLog.map((c) => c.oid)) | |
| const ahead = localLog.filter((c) => !remoteShas.has(c.oid)).length | |
| const behind = remoteLog.filter((c) => !localShas.has(c.oid)).length | |
| return {ahead, behind} | |
| const ahead = localLog.filter((c) => !remoteShas.has(c.oid)).length | |
| const behind = remoteLog.filter((c) => !localShas.has(c.oid)).length | |
| const aheadTruncated = localLog.length === MAX_AHEAD_BEHIND_DEPTH && ahead === MAX_AHEAD_BEHIND_DEPTH | |
| const behindTruncated = remoteLog.length === MAX_AHEAD_BEHIND_DEPTH && behind === MAX_AHEAD_BEHIND_DEPTH | |
| return {ahead, aheadTruncated, behind, behindTruncated} |
| const localRef = localSha | ||
| const remoteRef = remoteSha |
There was a problem hiding this comment.
nitpick: localRef and remoteRef are immediately-assigned aliases for localSha and remoteSha that were resolved just four lines above (lines 606–607). Introducing new names for the same values in the same scope makes readers double-check whether they differ.
The aliases exist because readBlob takes an oid parameter — the names are communicating "these are OIDs, not refs". Using more descriptive names makes the intent clearer:
| const localRef = localSha | |
| const remoteRef = remoteSha | |
| const localCommitOid = localSha | |
| const remoteCommitOid = remoteSha | |
| const wouldBeOverwritten = await Promise.all( | |
| dirtyFiles.map(async (filepath) => { | |
| const [localFileOid, remoteFileOid] = await Promise.all([ | |
| git | |
| .readBlob({dir, filepath, fs, oid: localCommitOid}) | |
| .then((r) => r.oid) | |
| .catch(() => null), | |
| git | |
| .readBlob({dir, filepath, fs, oid: remoteCommitOid}) | |
| .then((r) => r.oid) | |
| .catch(() => null), | |
| ]) | |
| return localFileOid !== remoteFileOid | |
| }), | |
| ) |
| fs, | ||
| headers: this.buildBasicAuthHeaders(token.userId, token.sessionKey), | ||
| http, | ||
| onAuth: this.getOnAuth(), | ||
| onAuthFailure: this.getOnAuthFailure(), |
There was a problem hiding this comment.
nitpick: All network operations (clone, fetch, pull, push) pass both headers (containing a pre-built Authorization: Basic …) and onAuth (a callback that returns the same credentials). Since isomorphic-git only invokes onAuth when headers are absent or the server returns 401, the callback is effectively dead code under normal conditions.
Pick one strategy and remove the other to keep the auth flow simple and auditable. headers is the simpler choice for a pre-known token; onAuth makes sense for lazy credential resolution. Having both makes it harder to reason about what credentials are actually sent.
| if (!(error instanceof Error) || !('code' in error)) return undefined | ||
| const {code} = error as {code: string} | ||
| if (code === 'HttpError' || code === 'SmartHttpError') { |
There was a problem hiding this comment.
nitpick: The 'code' in error check on line 63 has already narrowed error to Error & {code: unknown}. The subsequent as {code: string} cast re-widens back to unknown-then-trusts-string without the compiler verifying it, bypassing the type narrowing that was just established.
A safer pattern that avoids the cast entirely:
| if (!(error instanceof Error) || !('code' in error)) return undefined | |
| const {code} = error as {code: string} | |
| if (code === 'HttpError' || code === 'SmartHttpError') { | |
| function classifyIsomorphicGitError(error: unknown, notFoundCode: VcErrorCodeType): undefined | VcError { | |
| if (!(error instanceof Error) || !('code' in error)) return undefined | |
| const code = (error as {code: unknown}).code | |
| if (typeof code !== 'string') return undefined | |
| if (code === 'HttpError' || code === 'SmartHttpError') { |
| try { | ||
| await this.config.runCommand('vc:init') | ||
| } catch { | ||
| // vc:init logs its own errors | ||
| return |
There was a problem hiding this comment.
suggestion: The empty catch block silently swallows all errors from vc:init. The comment "vc:init logs its own errors" is only accurate when vc:init reaches its own error-handling code — it does not hold for oclif framework errors (e.g. missing command, plugin load failure) or uncaught exceptions thrown before the command's run() method is reached.
A user hitting one of those cases gets no output at all, making brv init look like it succeeded when vc:init actually failed.
| try { | |
| await this.config.runCommand('vc:init') | |
| } catch { | |
| // vc:init logs its own errors | |
| return | |
| } catch (error) { | |
| // vc:init handles and logs its own domain errors. | |
| // Log unexpected framework-level failures so the user isn't left in the dark. | |
| if (error instanceof Error) this.warn(`vc:init failed unexpectedly: ${error.message}`) | |
| return | |
| } |
| public static args = { | ||
| url: Args.string({description: `Clone URL (e.g. ${getGitRemoteBaseUrl()}/<team>/<space>.git)`}), | ||
| } | ||
| public static description = 'Clone a ByteRover space repository' | ||
| public static examples = [`<%= config.bin %> vc clone ${getGitRemoteBaseUrl()}/acme/project.git`] |
There was a problem hiding this comment.
nitpick: getGitRemoteBaseUrl() is called at class-definition time (module load), not at command execution time. This means static args and static examples are evaluated once when the module is first imported — before any environment variables have been fully resolved for the current invocation.
Two consequences:
- In test environments where
BRV_GIT_REMOTE_BASE_URLis not set,args.url.descriptionandexamplessilently use the hardcoded default URL rather than throwing, so tests won't surface a misconfigured env. - If
BRV_GIT_REMOTE_BASE_URLis set after module load (e.g. deferred config injection), the static fields show a stale value.
This is low-risk in practice, but worth being aware of if the env var ever becomes required.
bao-byterover
dismissed
cuongdo-byterover’s stale review
The merge-base changed after approval.
3 participants
Summary
brvhad no local git operations — all version control required the server/daemon. Users couldn't init, commit, branch, merge, push, or pull locally.brv vccommands (init, add, commit, branch, checkout, clone, fetch, pull, push, log, status, merge, reset, remote, config) viaisomorphic-gitIsomorphicGitService(1184 lines) +GitHttpWrapper(369 lines) +CogitUrlBuilderfor URL parsingVcHandler(1383 lines) routing all VC transport eventsInitProjectPagefor interactive setupbrv initoclif command with interactive project setup.brv/auto-added to.gitignoreon project initFileVcGitConfigStorefor git config persistence.env.examplewithBRV_GIT_API_BASE_URLandBRV_GIT_REMOTE_BASE_URLType of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)N/A
Test plan
test/unit/infra/git/isomorphic-git-service.test.ts(1320 lines)test/unit/infra/transport/handlers/vc-handler.test.ts(3683 lines)test/unit/infra/vc/file-vc-git-config-store.test.tstest/unit/infra/git/git-remote-url.test.tstest/unit/infra/transport/handlers/init-handler.test.tstest/unit/infra/transport/handlers/pull-handler.test.tstest/unit/infra/transport/handlers/push-handler.test.tstest/unit/infra/transport/handlers/reset-handler.test.tstest/unit/infra/transport/handlers/space-handler.test.tstest/unit/utils/gitignore.test.tstest/commands/init.test.ts.brv/gitignore idempotencyUser-visible changes
brv vc *command family (15+ subcommands)brv initinteractive project setup.brv/auto-added to.gitignoreon init.env.exampleaddsBRV_GIT_API_BASE_URLandBRV_GIT_REMOTE_BASE_URLEvidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
isomorphic-gitedge cases with large repos or unusual git configs.env.exampleadds 2 new vars — devs missing them get runtime errorreadRequiredEnv()throws clear message naming the missing var