fix: address review findings (#3991)

This commit is contained in:
Jinjing 2026-05-30 23:04:18 -07:00 committed by GitHub
parent c3a025793b
commit 5e7d097682
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 74 additions and 3 deletions

View File

@ -709,6 +709,43 @@ describe('SshGitProvider', () => {
})
})
it('worktreeIsClean sends git.worktreeIsClean request', async () => {
const cleanResult = { clean: false, stdout: '?? scratch.txt\n' }
mux.request.mockResolvedValue(cleanResult)
const result = await provider.worktreeIsClean('/home/user/feat')
expect(mux.request).toHaveBeenCalledWith('git.worktreeIsClean', {
worktreePath: '/home/user/feat'
})
expect(result).toEqual(cleanResult)
})
it('worktreeIsClean falls back to git.status for old relays', async () => {
const methodNotFound = Object.assign(new Error('Method not found: git.worktreeIsClean'), {
code: -32601
})
mux.request.mockRejectedValueOnce(methodNotFound).mockResolvedValueOnce({
entries: [{ path: 'scratch.txt', status: 'untracked', area: 'untracked' }],
conflictOperation: 'unknown'
})
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
try {
const result = await provider.worktreeIsClean('/home/user/feat')
expect(mux.request).toHaveBeenNthCalledWith(1, 'git.worktreeIsClean', {
worktreePath: '/home/user/feat'
})
expect(mux.request).toHaveBeenNthCalledWith(2, 'git.status', {
worktreePath: '/home/user/feat'
})
expect(result).toEqual({ clean: false, stdout: 'untracked untracked: scratch.txt' })
} finally {
warnSpy.mockRestore()
}
})
it('renameCurrentBranch sends the narrow branch-rename request', async () => {
await provider.renameCurrentBranch('/home/user/feat', 'you/fix-auth')
expect(mux.request).toHaveBeenCalledWith('git.renameCurrentBranch', {

View File

@ -17,6 +17,7 @@ import type {
} from '../../shared/types'
import type { GitHistoryOptions, GitHistoryResult } from '../../shared/git-history'
import { buildHostedRemoteFileUrl } from '../git/hosted-remote-url'
import { JsonRpcErrorCode } from '../ssh/relay-protocol'
import type { CommitMessageDraftContext } from '../../shared/commit-message-generation'
import type { CommitMessagePlan } from '../../shared/commit-message-plan'
import type { RemoteCommitMessageExecResult } from '../text-generation/commit-message-text-generation'
@ -28,10 +29,25 @@ type NonInteractiveExecQueueEntry = {
release: () => void
}
function isJsonRpcMethodNotFoundError(error: unknown): boolean {
if (!error || typeof error !== 'object') {
return false
}
return (error as { code?: unknown }).code === JsonRpcErrorCode.MethodNotFound
}
function formatStatusEntriesForCleanCheck(entries: GitStatusResult['entries']): string | undefined {
if (entries.length === 0) {
return undefined
}
return entries.map((entry) => `${entry.area} ${entry.status}: ${entry.path}`).join('\n')
}
export class SshGitProvider implements IGitProvider {
private connectionId: string
private mux: SshChannelMultiplexer
private nonInteractiveExecQueues = new Map<string, NonInteractiveExecQueueEntry[]>()
private loggedWorktreeIsCleanFallback = false
constructor(connectionId: string, mux: SshChannelMultiplexer) {
this.connectionId = connectionId
@ -443,9 +459,26 @@ export class SshGitProvider implements IGitProvider {
}
async worktreeIsClean(worktreePath: string): Promise<{ clean: boolean; stdout?: string }> {
return (await this.mux.request('git.worktreeIsClean', { worktreePath })) as {
clean: boolean
stdout?: string
try {
return (await this.mux.request('git.worktreeIsClean', { worktreePath })) as {
clean: boolean
stdout?: string
}
} catch (error) {
if (!isJsonRpcMethodNotFoundError(error)) {
throw error
}
if (!this.loggedWorktreeIsCleanFallback) {
this.loggedWorktreeIsCleanFallback = true
console.warn(
'[ssh-git] Relay does not implement git.worktreeIsClean; falling back to git.status clean check'
)
}
// Why: existing SSH relays may predate git.worktreeIsClean, but git.status
// is a narrow relay RPC and avoids the generic git.exec allowlist.
const status = await this.getStatus(worktreePath)
const clean = status.entries.length === 0
return { clean, stdout: formatStatusEntriesForCleanCheck(status.entries) }
}
}

View File

@ -62,6 +62,7 @@ describe('GitHandler', () => {
expect(methods).toContain('git.listWorktrees')
expect(methods).toContain('git.addWorktree')
expect(methods).toContain('git.removeWorktree')
expect(methods).toContain('git.worktreeIsClean')
expect(methods).toContain('git.renameCurrentBranch')
expect(methods).toContain('git.exec')
expect(methods).toContain('git.isGitRepo')