Skip to content

Commit 44bf03a

Browse files
benibenjCopilot
andcommitted
agentHost: distinguish a detached HEAD from a failed git probe
Addresses PR review feedback: keying the repair off a missing `branchName` alone also matched a detached HEAD, which reports no branch by design. Those sessions would have refreshed git state on every evaluation -- a periodic git call and log noise that could never produce a branch. `parseGitStatusV2` already recognises `(detached)`; it now reports that as `isDetachedHead` so the distinction survives into persisted session git state, and a shared `needsSessionGitStateRefresh` predicate keeps the Agent Merge and subscribe-time call sites in agreement about which states are worth recomputing. The controller additionally caps the repair at one attempt per runtime, so any other checkout that cannot report a branch costs a single git call rather than one per backstop, and logs a warning when a refresh still yields no branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 344c0af commit 44bf03a

6 files changed

Lines changed: 154 additions & 14 deletions

File tree

src/vs/platform/agentHost/common/state/sessionState.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1366,6 +1366,12 @@ export interface ISessionGitState {
13661366
readonly hasGitHubRemote?: boolean;
13671367
/** Current branch name. */
13681368
readonly branchName?: string;
1369+
/**
1370+
* Whether `HEAD` is detached, which is why {@link branchName} is absent.
1371+
* Distinguishes a legitimately branch-less checkout from git state left
1372+
* behind by a probe that failed before it could resolve the branch.
1373+
*/
1374+
readonly isDetachedHead?: boolean;
13691375
/** Base branch the work targets (e.g. `main`). */
13701376
readonly baseBranchName?: string;
13711377
/** Upstream tracking branch (e.g. `origin/feature`). */
@@ -1581,6 +1587,7 @@ export function readSessionGitState(meta: SessionMeta | undefined): ISessionGitS
15811587
const result: {
15821588
hasGitHubRemote?: boolean;
15831589
branchName?: string;
1590+
isDetachedHead?: boolean;
15841591
baseBranchName?: string;
15851592
upstreamBranchName?: string;
15861593
incomingChanges?: number;
@@ -1593,6 +1600,7 @@ export function readSessionGitState(meta: SessionMeta | undefined): ISessionGitS
15931600
} = {};
15941601
if (typeof raw['hasGitHubRemote'] === 'boolean') { result.hasGitHubRemote = raw['hasGitHubRemote']; }
15951602
if (typeof raw['branchName'] === 'string') { result.branchName = raw['branchName']; }
1603+
if (typeof raw['isDetachedHead'] === 'boolean') { result.isDetachedHead = raw['isDetachedHead']; }
15961604
if (typeof raw['baseBranchName'] === 'string') { result.baseBranchName = raw['baseBranchName']; }
15971605
if (typeof raw['upstreamBranchName'] === 'string') { result.upstreamBranchName = raw['upstreamBranchName']; }
15981606
if (typeof raw['incomingChanges'] === 'number') { result.incomingChanges = raw['incomingChanges']; }
@@ -1605,6 +1613,23 @@ export function readSessionGitState(meta: SessionMeta | undefined): ISessionGitS
16051613
return result;
16061614
}
16071615

1616+
/**
1617+
* Whether a session's git state should be recomputed because it does not
1618+
* describe a usable checkout.
1619+
*
1620+
* A state that was never computed obviously qualifies. So does one that is
1621+
* missing its branch without a detached `HEAD` to explain it: `git status` is
1622+
* the only probe that reports the branch, so such a state is the residue of a
1623+
* probe that failed, and consumers that key off the branch (Agent Merge binds
1624+
* its pull request that way) stay stranded until it is recomputed. A detached
1625+
* `HEAD` is a legitimate branch-less checkout and must not be mistaken for it,
1626+
* or every caller would refresh in a loop against a repository that will never
1627+
* report a branch.
1628+
*/
1629+
export function needsSessionGitStateRefresh(gitState: ISessionGitState | undefined): boolean {
1630+
return gitState === undefined || (gitState.branchName === undefined && !gitState.isDetachedHead);
1631+
}
1632+
16081633
/**
16091634
* Returns a new {@link SessionMeta} with the git-state payload set to
16101635
* `gitState`, or with the git slot removed if `gitState` is `undefined`.

src/vs/platform/agentHost/node/agentHostGitService.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -994,6 +994,7 @@ export class AgentHostGitService implements IAgentHostGitService {
994994
const result: ISessionGitState = {
995995
hasGitHubRemote,
996996
branchName: status.branchName,
997+
isDetachedHead: status.isDetachedHead,
997998
baseBranchName,
998999
upstreamBranchName: status.upstreamBranchName,
9991000
incomingChanges: status.incomingChanges,
@@ -1526,6 +1527,7 @@ export function parseGitDiffRawNumstat(output: string, repositoryRoot: URI, sess
15261527
*/
15271528
export function parseGitStatusV2(output: string | undefined): {
15281529
branchName?: string;
1530+
isDetachedHead?: boolean;
15291531
upstreamBranchName?: string;
15301532
outgoingChanges?: number;
15311533
incomingChanges?: number;
@@ -1535,6 +1537,7 @@ export function parseGitStatusV2(output: string | undefined): {
15351537
return {};
15361538
}
15371539
let branchName: string | undefined;
1540+
let isDetachedHead: boolean | undefined;
15381541
let upstreamBranchName: string | undefined;
15391542
let outgoingChanges: number | undefined;
15401543
let incomingChanges: number | undefined;
@@ -1544,8 +1547,11 @@ export function parseGitStatusV2(output: string | undefined): {
15441547
if (!line) { continue; }
15451548
if (line.startsWith('# branch.head ')) {
15461549
const head = line.substring('# branch.head '.length).trim();
1547-
// `(detached)` is what git emits for a detached HEAD. Treat as no branch.
1548-
branchName = head === '(detached)' ? undefined : head;
1550+
// `(detached)` is what git emits for a detached HEAD. Treat as no
1551+
// branch, but report why so consumers can tell an intentionally
1552+
// branch-less checkout from a status probe that never ran.
1553+
isDetachedHead = head === '(detached)' ? true : undefined;
1554+
branchName = isDetachedHead ? undefined : head;
15491555
} else if (line.startsWith('# branch.upstream ')) {
15501556
upstreamBranchName = line.substring('# branch.upstream '.length).trim();
15511557
} else if (line.startsWith('# branch.ab ')) {
@@ -1558,7 +1564,7 @@ export function parseGitStatusV2(output: string | undefined): {
15581564
uncommittedChanges++;
15591565
}
15601566
}
1561-
return { branchName, upstreamBranchName, outgoingChanges, incomingChanges, uncommittedChanges };
1567+
return { branchName, isDetachedHead, upstreamBranchName, outgoingChanges, incomingChanges, uncommittedChanges };
15621568
}
15631569

15641570
/** Exported for tests. */

src/vs/platform/agentHost/node/agentMergeController.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { deriveGitHubEndpoints } from '../common/githubEndpoints.js';
2121
import { SessionConfigKey } from '../common/sessionConfigKeys.js';
2222
import { ActionType } from '../common/state/protocol/common/actions.js';
2323
import { AuthRequiredReason } from '../common/state/sessionActions.js';
24-
import { getSessionRelatedPullRequestUrls, isAhpChatChannel, isSessionStatusArchived, parseRequiredSessionUriFromChatUri, readSessionGitHubState, readSessionGitState, SessionLifecycle, TurnState } from '../common/state/sessionState.js';
24+
import { getSessionRelatedPullRequestUrls, isAhpChatChannel, isSessionStatusArchived, needsSessionGitStateRefresh, parseRequiredSessionUriFromChatUri, readSessionGitHubState, readSessionGitState, SessionLifecycle, TurnState } from '../common/state/sessionState.js';
2525
import { IAgentConfigurationService } from './agentConfigurationService.js';
2626
import { IAgentHostGitHubEndpointService } from './agentHostGitHubEndpointService.js';
2727
import { AgentHostStateManager, IAgentHostStateManager } from './agentHostStateManager.js';
@@ -47,6 +47,13 @@ class AgentMergeRuntime extends Disposable {
4747
readonly evaluationScheduler: RunOnceScheduler;
4848
readonly backstopScheduler: RunOnceScheduler;
4949
ref: PullRequestRef | undefined;
50+
/**
51+
* Whether this runtime already tried to recompute git state that reported
52+
* no usable branch. Caps that repair at one git call per runtime so a
53+
* checkout that can never report a branch does not spawn one on every
54+
* backstop.
55+
*/
56+
didRefreshForMissingBranch = false;
5057

5158
constructor(
5259
readonly session: string,
@@ -483,7 +490,7 @@ export class AgentMergeController extends Disposable {
483490

484491
/**
485492
* Resolves the branch Agent Merge should act on, repairing session git
486-
* state that is missing it.
493+
* state that does not report one.
487494
*
488495
* A failed git probe can leave persisted git state without a branch. The
489496
* refresh that would repair it normally rides along with a client watching
@@ -492,12 +499,21 @@ export class AgentMergeController extends Disposable {
492499
* — binding the pull request, subscribing to it, acting on its feedback —
493500
* is gated on the branch, so without this the session idles on the backstop
494501
* indefinitely and Agent Merge silently never runs.
502+
*
503+
* A detached `HEAD` is excluded: it reports no branch by design, so
504+
* refreshing would never produce one. The attempt is capped at once per
505+
* runtime regardless, so any other checkout that cannot report a branch
506+
* costs a single git call rather than one per backstop.
495507
*/
496508
private async _resolveCurrentBranch(session: string, runtime: AgentMergeRuntime, state: NonNullable<ReturnType<AgentHostStateManager['getSessionState']>>): Promise<string | undefined> {
497-
const branchName = readSessionGitState(state._meta)?.branchName;
498-
if (branchName) {
499-
return branchName;
509+
const gitState = readSessionGitState(state._meta);
510+
if (gitState?.branchName) {
511+
return gitState.branchName;
512+
}
513+
if (runtime.didRefreshForMissingBranch || !needsSessionGitStateRefresh(gitState)) {
514+
return undefined;
500515
}
516+
runtime.didRefreshForMissingBranch = true;
501517
this._logService.debug(`[AgentMergeController] Refreshing git state because the session reports no branch: session=${session}`);
502518
await this._gitStateService.refreshSessionGitState(session, state.workingDirectories?.[0] ? URI.parse(state.workingDirectories[0]) : undefined);
503519
if (!this._isCurrentRuntime(session, runtime)) {
@@ -506,6 +522,8 @@ export class AgentMergeController extends Disposable {
506522
const refreshed = readSessionGitState(this._stateManager.getSessionState(session)?._meta)?.branchName;
507523
if (refreshed) {
508524
this._logService.info(`[AgentMergeController] Recovered the session branch after refreshing git state: session=${session}`);
525+
} else {
526+
this._logService.warn(`[AgentMergeController] Session still reports no branch after refreshing git state: session=${session}`);
509527
}
510528
return refreshed;
511529
}

src/vs/platform/agentHost/node/agentService.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ import type { InvokeChangesetOperationParams, InvokeChangesetOperationResult } f
3737
import { AhpErrorCodes, AHP_SESSION_NOT_FOUND, ContentEncoding, JSON_RPC_INTERNAL_ERROR, ProtocolError, ResourceChangeType, ResourceType, ResourceWriteMode, type CreateResourceWatchParams, type CreateResourceWatchResult, type DirectoryEntry, type ResourceCopyParams, type ResourceCopyResult, type ResourceDeleteParams, type ResourceDeleteResult, type ResourceListResult, type ResourceMkdirParams, type ResourceMkdirResult, type ResourceMoveParams, type ResourceMoveResult, type ResourceReadResult, type ResourceResolveParams, type ResourceResolveResult, type ResourceWatchState, type ResourceWriteParams, type ResourceWriteResult, type IStateSnapshot } from '../common/state/sessionProtocol.js';
3838
import { ChangesSummary, ChatInteractivity, ChatOriginKind, MessageAttachmentKind, type Annotation, type AnnotationEntry, type AnnotationsState, type ChatOrigin, type Customization, type Message, type MessageAttachment, type MessageResourceAttachment } from '../common/state/protocol/state.js';
3939
import type { ChatPendingMessageSetAction, ChatTurnStartedAction, SessionConfigChangedAction } from '../common/state/protocol/actions.js';
40-
import { ISessionGitHubState, ISessionGitState, MessageKind, ResponsePartKind, SESSION_META_GITHUB_KEY, SESSION_META_GIT_KEY, SESSION_META_MULTI_ROOT_KEY, SESSION_META_SOURCE_CONTROL_KEY, AH_META_ORCHESTRATION_DB_KEY, readSessionSpawnDepth, parseSessionOrchestration, withSessionSpawnDepth, withSessionOrchestration, SessionLifecycle, SessionStatus, ToolCallStatus, ToolResultContentType, AH_META_WORKSPACELESS_DB_KEY, AH_META_IS_ARCHIVED_DB_KEY, AH_META_IS_DONE_DB_KEY, AH_META_IS_READ_DB_KEY, buildChatUri, buildDefaultChatUri, buildResourceWatchChannelUri, buildSubagentChatUri, buildSubagentSessionUriPrefix, hostBuildInfoFromProduct, isAhpChatChannel, isDefaultChatUri, isSubagentChatUri, isSubagentSession, parseChatUri, parseDefaultChatUri, parseRequiredSessionUriFromChatUri, parseResourceWatchChannelUri, parseSessionMultiRootMetadata, parseSubagentSessionUri, readSessionExternal, readSessionGitHubState, readSessionGitState, readSessionMultiRootMetadata, readSessionSourceControlState, readSessionWorkspaceless, withSessionExternal, withSessionGitHubState, withSessionGitState, withSessionMultiRootMetadata, withSessionSourceControlState, withSessionStatusFlag, withSessionWorkspaceless, withSessionFolderPickerDecision, readSessionFolderPickerDecision, parseSessionFolderPickerDecision, SESSION_META_FOLDER_PICKER_KEY, readSessionEhcliAdoptable, type ISessionSourceControlState, type SessionConfigState, type SessionSummary, type ToolResultSubagentContent, type Turn, type UsageInfo, chatStorageUri, hasReportedUsage } from '../common/state/sessionState.js';
40+
import { ISessionGitHubState, ISessionGitState, MessageKind, ResponsePartKind, SESSION_META_GITHUB_KEY, SESSION_META_GIT_KEY, SESSION_META_MULTI_ROOT_KEY, SESSION_META_SOURCE_CONTROL_KEY, AH_META_ORCHESTRATION_DB_KEY, readSessionSpawnDepth, parseSessionOrchestration, withSessionSpawnDepth, withSessionOrchestration, SessionLifecycle, SessionStatus, ToolCallStatus, ToolResultContentType, AH_META_WORKSPACELESS_DB_KEY, AH_META_IS_ARCHIVED_DB_KEY, AH_META_IS_DONE_DB_KEY, AH_META_IS_READ_DB_KEY, buildChatUri, buildDefaultChatUri, buildResourceWatchChannelUri, buildSubagentChatUri, buildSubagentSessionUriPrefix, hostBuildInfoFromProduct, isAhpChatChannel, isDefaultChatUri, isSubagentChatUri, isSubagentSession, needsSessionGitStateRefresh, parseChatUri, parseDefaultChatUri, parseRequiredSessionUriFromChatUri, parseResourceWatchChannelUri, parseSessionMultiRootMetadata, parseSubagentSessionUri, readSessionExternal, readSessionGitHubState, readSessionGitState, readSessionMultiRootMetadata, readSessionSourceControlState, readSessionWorkspaceless, withSessionExternal, withSessionGitHubState, withSessionGitState, withSessionMultiRootMetadata, withSessionSourceControlState, withSessionStatusFlag, withSessionWorkspaceless, withSessionFolderPickerDecision, readSessionFolderPickerDecision, parseSessionFolderPickerDecision, SESSION_META_FOLDER_PICKER_KEY, readSessionEhcliAdoptable, type ISessionSourceControlState, type SessionConfigState, type SessionSummary, type ToolResultSubagentContent, type Turn, type UsageInfo, chatStorageUri, hasReportedUsage } from '../common/state/sessionState.js';
4141
import { readToolCallMeta } from '../common/meta/agentToolCallMeta.js';
4242
import { readEphemeralSessionMeta, withEphemeralSessionMeta } from '../common/meta/agentEphemeralSessionMeta.js';
4343
import { readChatSurfaceMeta, withChatSurfaceMeta } from '../common/meta/agentChatSurfaceMeta.js';
@@ -3842,11 +3842,12 @@ export class AgentService extends Disposable implements IAgentService {
38423842
// restore path that normally calls `_attachGitState` is skipped — so
38433843
// trigger it lazily here for the first subscriber. `_attachGitState`
38443844
// is async and updates `_meta.git` once ready, which clients see via
3845-
// the normal state-update stream. A state without a branch is treated
3846-
// as missing too: a failed git probe can persist one, and it would
3847-
// otherwise mask the very repair this lazy refresh exists to perform.
3845+
// the normal state-update stream. State that does not describe a
3846+
// usable checkout counts as missing too: a failed probe can persist
3847+
// a branch-less remnant, and it would otherwise mask the very
3848+
// repair this lazy refresh exists to perform.
38483849
const sessionState = this._stateManager.getSessionState(resourceStr);
3849-
if (!isAhpChatChannel(resourceStr) && sessionState && readSessionGitState(sessionState._meta)?.branchName === undefined) {
3850+
if (!isAhpChatChannel(resourceStr) && sessionState && needsSessionGitStateRefresh(readSessionGitState(sessionState._meta))) {
38503851
const workingDirectory = sessionState.workingDirectories?.[0]
38513852
? URI.parse(sessionState.workingDirectories[0])
38523853
: undefined;

src/vs/platform/agentHost/test/node/agentHostGitService.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { formatGitError, getRemoteTrackingRef, GitCheckoutProgressParser, isRetr
99
import { buildGitBlobUri } from '../../node/gitDiffContent.js';
1010
import { URI } from '../../../../base/common/uri.js';
1111
import { EMPTY_TREE_OBJECT, getBranchCompletions, resolveDiffBaseBranchName } from '../../common/agentHostGitService.js';
12+
import { needsSessionGitStateRefresh } from '../../common/state/sessionState.js';
1213

1314
suite('AgentHostGitService', () => {
1415
ensureNoDisposablesAreLeakedInTestSuite();
@@ -104,6 +105,7 @@ suite('AgentHostGitService', () => {
104105
].join('\n');
105106
assert.deepStrictEqual(parseGitStatusV2(out), {
106107
branchName: 'main',
108+
isDetachedHead: undefined,
107109
upstreamBranchName: 'origin/main',
108110
outgoingChanges: 0,
109111
incomingChanges: 0,
@@ -123,6 +125,7 @@ suite('AgentHostGitService', () => {
123125
].join('\n');
124126
assert.deepStrictEqual(parseGitStatusV2(out), {
125127
branchName: 'feature',
128+
isDetachedHead: undefined,
126129
upstreamBranchName: 'origin/feature',
127130
outgoingChanges: 3,
128131
incomingChanges: 2,
@@ -137,6 +140,7 @@ suite('AgentHostGitService', () => {
137140
].join('\n');
138141
assert.deepStrictEqual(parseGitStatusV2(out), {
139142
branchName: undefined,
143+
isDetachedHead: true,
140144
upstreamBranchName: undefined,
141145
outgoingChanges: undefined,
142146
incomingChanges: undefined,
@@ -149,6 +153,24 @@ suite('AgentHostGitService', () => {
149153
});
150154
});
151155

156+
suite('needsSessionGitStateRefresh', () => {
157+
test('separates a branch-less probe failure from a detached HEAD', () => {
158+
assert.deepStrictEqual({
159+
neverComputed: needsSessionGitStateRefresh(undefined),
160+
// The residue of a failed `git status`, as persisted before the
161+
// probe learned to withhold state it could not compute.
162+
probeFailureRemnant: needsSessionGitStateRefresh({ baseBranchName: 'main' }),
163+
detachedHead: needsSessionGitStateRefresh({ isDetachedHead: true, baseBranchName: 'main' }),
164+
onABranch: needsSessionGitStateRefresh({ branchName: 'feature', baseBranchName: 'main' }),
165+
}, {
166+
neverComputed: true,
167+
probeFailureRemnant: true,
168+
detachedHead: false,
169+
onABranch: false,
170+
});
171+
});
172+
});
173+
152174
suite('parseHasGitHubRemote', () => {
153175
test('detects ssh github remote', () => {
154176
assert.strictEqual(parseHasGitHubRemote('origin\tgit@github.com:owner/repo.git (fetch)\n'), true);

0 commit comments

Comments
 (0)