diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index f1fb03e7e45..5e5c1f4e122 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -801,6 +801,7 @@ export const make = Effect.gen(function* () { aheadCount: details.aheadCount, behindCount: details.behindCount, aheadOfDefaultCount: details.aheadOfDefaultCount, + ...(details.remoteRefHash == null ? {} : { remoteRefHash: details.remoteRefHash }), pr, } satisfies VcsStatusRemoteResult; }); diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts index 55aa8f38835..2400adbeeb0 100644 --- a/apps/server/src/vcs/GitVcsDriver.ts +++ b/apps/server/src/vcs/GitVcsDriver.ts @@ -73,6 +73,7 @@ export interface GitRemoteStatusDetails { isDefaultBranch: boolean; branch: string | null; upstreamRef: string | null; + remoteRefHash?: string | null; hasUpstream: boolean; aheadCount: number; behindCount: number; diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index dc58fc2543c..4f6b5ba5f99 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -187,6 +187,31 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }); describe("review diff previews", () => { + it.effect("omits an untracked file after it is deleted", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + const fileSystem = yield* FileSystem.FileSystem; + const pathService = yield* Path.Path; + const createdPath = pathService.join(cwd, "created.ts"); + yield* writeTextFile(cwd, "created.ts", "export const created = true;\n"); + + const beforeDelete = yield* driver.getReviewDiffPreview({ cwd }); + yield* fileSystem.remove(createdPath); + const afterDelete = yield* driver.getReviewDiffPreview({ cwd }); + + assert.include( + beforeDelete.sources.find((source) => source.kind === "working-tree")?.diff ?? "", + "created.ts", + ); + assert.strictEqual( + afterDelete.sources.find((source) => source.kind === "working-tree")?.diff, + "", + ); + }), + ); + it.effect("drops an unterminated path from truncated NUL-separated git output", () => Effect.sync(() => { const paths = splitNullSeparatedGitStdoutPaths({ @@ -326,6 +351,32 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }), ); + it.effect("changes the remote ref hash when a remote-tracking ref moves", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const initialCommit = yield* git(cwd, ["rev-parse", "HEAD"]); + yield* writeTextFile(cwd, "next.txt", "next\n"); + yield* git(cwd, ["add", "next.txt"]); + yield* git(cwd, ["commit", "-m", "next commit"]); + const nextCommit = yield* git(cwd, ["rev-parse", "HEAD"]); + const driver = yield* GitVcsDriver.GitVcsDriver; + + yield* git(cwd, ["update-ref", "refs/remotes/origin/base", initialCommit]); + const initialStatus = yield* driver.statusDetailsRemote(cwd, { + refreshUpstream: false, + }); + yield* git(cwd, ["update-ref", "refs/remotes/origin/base", nextCommit]); + const nextStatus = yield* driver.statusDetailsRemote(cwd, { + refreshUpstream: false, + }); + + assert.isString(initialStatus.remoteRefHash); + assert.isString(nextStatus.remoteRefHash); + assert.notEqual(initialStatus.remoteRefHash, nextStatus.remoteRefHash); + }), + ); + it.effect("can read cached remote divergence without fetching upstream", () => Effect.gen(function* () { const cwd = yield* makeTmpDir(); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index a406cbce549..c5e1bfa70fb 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -79,6 +79,7 @@ const NON_REPOSITORY_REMOTE_STATUS_DETAILS = Object.freeze 0 && branchValue !== "HEAD" ? branchValue : null; + const remoteRefs = yield* runGitStdout("GitVcsDriver.statusDetailsRemote.remoteRefs", cwd, [ + "for-each-ref", + "--sort=refname", + "--format=%(refname)%00%(objectname)", + "refs/remotes", + ]); + const remoteRefHash = yield* crypto + .digest("SHA-256", new TextEncoder().encode(remoteRefs)) + .pipe( + Effect.map(Encoding.encodeHex), + Effect.mapError( + (cause) => + new GitCommandError({ + operation: "GitVcsDriver.statusDetailsRemote.remoteRefHash", + command: "crypto.digest SHA-256", + cwd, + detail: "Failed to hash remote refs.", + cause, + }), + ), + ); const upstream = yield* resolveCurrentUpstream(cwd); const upstreamRef = upstream?.upstreamRef ?? null; let aheadCount = 0; @@ -1260,6 +1282,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* isDefaultBranch, branch, upstreamRef, + remoteRefHash, hasUpstream: upstreamRef !== null, aheadCount, behindCount, diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 032e48e4612..031aff3a283 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -42,6 +42,7 @@ const baseRemoteStatus: VcsStatusRemoteResult = { hasUpstream: true, aheadCount: 0, behindCount: 0, + remoteRefHash: "remote-ref-hash-1", pr: null, }; @@ -361,6 +362,7 @@ describe("VcsStatusBroadcaster", () => { _tag: "snapshot", local: baseLocalStatus, remote: null, + localGeneration: 1, } satisfies VcsStatusStreamEvent); assert.deepStrictEqual(remoteUpdated, { _tag: "remoteUpdated", @@ -369,6 +371,46 @@ describe("VcsStatusBroadcaster", () => { }).pipe(Effect.provide(makeTestLayer(state))); }); + it.effect("publishes a new local generation when the status summary is unchanged", () => { + const state = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + const snapshotDeferred = yield* Deferred.make(); + const localUpdatedDeferred = yield* Deferred.make(); + yield* Stream.runForEach(broadcaster.streamStatus({ cwd: "/repo" }), (event) => { + if (event._tag === "snapshot") { + return Deferred.succeed(snapshotDeferred, event).pipe(Effect.ignore); + } + if (event._tag === "localUpdated") { + return Deferred.succeed(localUpdatedDeferred, event).pipe(Effect.ignore); + } + return Effect.void; + }).pipe(Effect.forkScoped); + + const snapshot = yield* Deferred.await(snapshotDeferred); + yield* broadcaster.refreshLocalStatus("/repo"); + const localUpdated = yield* Deferred.await(localUpdatedDeferred); + + if (snapshot._tag !== "snapshot") { + return yield* Effect.die(`Expected snapshot, received ${snapshot._tag}`); + } + assert.strictEqual(snapshot.localGeneration, 1); + assert.deepStrictEqual(localUpdated, { + _tag: "localUpdated", + local: baseLocalStatus, + localGeneration: 2, + } satisfies VcsStatusStreamEvent); + }).pipe(Effect.provide(makeTestLayer(state))); + }); + it.effect("loads remote status once when periodic refreshes are disabled", () => { const state = { currentLocalStatus: baseLocalStatus, @@ -408,6 +450,7 @@ describe("VcsStatusBroadcaster", () => { _tag: "snapshot", local: baseLocalStatus, remote: null, + localGeneration: 1, } satisfies VcsStatusStreamEvent); assert.deepStrictEqual(remoteUpdated, { _tag: "remoteUpdated", @@ -545,7 +588,7 @@ describe("VcsStatusBroadcaster", () => { ); }); - it.effect("delays automatic refresh when a cached remote snapshot is available", () => { + it.effect("publishes a changed remote ref hash after the scheduled refresh", () => { const state = { currentLocalStatus: baseLocalStatus, currentRemoteStatus: baseRemoteStatus, @@ -560,15 +603,21 @@ describe("VcsStatusBroadcaster", () => { yield* broadcaster.getStatus({ cwd: "/repo" }); const scope = yield* Scope.make(); const snapshotDeferred = yield* Deferred.make(); + const remoteUpdatedDeferred = yield* Deferred.make(); yield* Stream.runForEach( broadcaster.streamStatus( { cwd: "/repo" }, { automaticRemoteRefreshInterval: Effect.succeed(Duration.minutes(1)) }, ), - (event) => - event._tag === "snapshot" - ? Deferred.succeed(snapshotDeferred, event).pipe(Effect.ignore) - : Effect.void, + (event) => { + if (event._tag === "snapshot") { + return Deferred.succeed(snapshotDeferred, event).pipe(Effect.ignore); + } + if (event._tag === "remoteUpdated") { + return Deferred.succeed(remoteUpdatedDeferred, event).pipe(Effect.ignore); + } + return Effect.void; + }, ).pipe(Effect.forkIn(scope)); yield* Deferred.await(snapshotDeferred); @@ -578,10 +627,18 @@ describe("VcsStatusBroadcaster", () => { yield* TestClock.adjust(Duration.seconds(59)); assert.equal(state.remoteStatusCalls, 1); + state.currentRemoteStatus = { + ...baseRemoteStatus, + remoteRefHash: "remote-ref-hash-2", + }; yield* TestClock.adjust(Duration.seconds(1)); - yield* Effect.yieldNow; + const remoteUpdated = yield* Deferred.await(remoteUpdatedDeferred); assert.equal(state.remoteStatusCalls, 2); assert.equal(state.remoteInvalidationCalls, 1); + assert.deepStrictEqual(remoteUpdated, { + _tag: "remoteUpdated", + remote: state.currentRemoteStatus, + } satisfies VcsStatusStreamEvent); yield* Scope.close(scope, Exit.void); }).pipe(Effect.provide(Layer.merge(makeTestLayer(state), TestClock.layer()))); diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index c238154f58c..56b36753b6e 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts @@ -124,8 +124,9 @@ interface CachedValue { } interface CachedVcsStatus { - readonly local: CachedValue | null; + readonly local: VcsStatusLocalResult | null; readonly remote: CachedValue | null; + readonly localGeneration: number; } interface ActiveRemotePoller { @@ -199,26 +200,25 @@ export const make = Effect.gen(function* () { const updateCachedLocalStatus = Effect.fn("VcsStatusBroadcaster.updateCachedLocalStatus")( function* (cwd: string, local: VcsStatusLocalResult, options?: { publish?: boolean }) { - const nextLocal = { - fingerprint: fingerprintStatusPart(local), - value: local, - } satisfies CachedValue; - const shouldPublish = yield* Ref.modify(cacheRef, (cache) => { - const previous = cache.get(cwd) ?? { local: null, remote: null }; + const localGeneration = yield* Ref.modify(cacheRef, (cache) => { + const previous = cache.get(cwd) ?? { local: null, remote: null, localGeneration: 0 }; + const nextLocalGeneration = previous.localGeneration + 1; const nextCache = new Map(cache); nextCache.set(cwd, { ...previous, - local: nextLocal, + local, + localGeneration: nextLocalGeneration, }); - return [previous.local?.fingerprint !== nextLocal.fingerprint, nextCache] as const; + return [nextLocalGeneration, nextCache] as const; }); - if (options?.publish && shouldPublish) { + if (options?.publish) { yield* PubSub.publish(changesPubSub, { cwd, event: { _tag: "localUpdated", local, + localGeneration, }, }); } @@ -234,7 +234,7 @@ export const make = Effect.gen(function* () { value: remote, } satisfies CachedValue; const shouldPublish = yield* Ref.modify(cacheRef, (cache) => { - const previous = cache.get(cwd) ?? { local: null, remote: null }; + const previous = cache.get(cwd) ?? { local: null, remote: null, localGeneration: 0 }; const nextCache = new Map(cache); nextCache.set(cwd, { ...previous, @@ -263,35 +263,30 @@ export const make = Effect.gen(function* () { remote: VcsStatusRemoteResult | null, options?: { publish?: boolean }, ) { - const nextLocal = { - fingerprint: fingerprintStatusPart(local), - value: local, - } satisfies CachedValue; const nextRemote = { fingerprint: fingerprintStatusPart(remote), value: remote, } satisfies CachedValue; - const shouldPublish = yield* Ref.modify(cacheRef, (cache) => { - const previous = cache.get(cwd) ?? { local: null, remote: null }; + const localGeneration = yield* Ref.modify(cacheRef, (cache) => { + const previous = cache.get(cwd) ?? { local: null, remote: null, localGeneration: 0 }; + const nextLocalGeneration = previous.localGeneration + 1; const nextCache = new Map(cache); nextCache.set(cwd, { - local: nextLocal, + local, remote: nextRemote, + localGeneration: nextLocalGeneration, }); - return [ - previous.local?.fingerprint !== nextLocal.fingerprint || - previous.remote?.fingerprint !== nextRemote.fingerprint, - nextCache, - ] as const; + return [nextLocalGeneration, nextCache] as const; }); - if (options?.publish && shouldPublish) { + if (options?.publish) { yield* PubSub.publish(changesPubSub, { cwd, event: { _tag: "snapshot", local, remote, + localGeneration, }, }); } @@ -311,7 +306,7 @@ export const make = Effect.gen(function* () { ) { const cached = yield* getCachedStatus(cwd); if (cached?.local) { - return cached.local.value; + return cached.local; } return yield* loadLocalStatus(cwd); }); @@ -324,11 +319,11 @@ export const make = Effect.gen(function* () { const cwd = yield* withFileSystem(normalizeCwd(input.cwd)); const cached = yield* getCachedStatus(cwd); if (cached?.local && cached.remote) { - return mergeGitStatusParts(cached.local.value, cached.remote.value); + return mergeGitStatusParts(cached.local, cached.remote.value); } const [local, remote] = yield* Effect.all( [ - cached?.local ? Effect.succeed(cached.local.value) : workflow.localStatus({ cwd }), + cached?.local ? Effect.succeed(cached.local) : workflow.localStatus({ cwd }), cached?.remote ? Effect.succeed(cached.remote.value) : workflow.remoteStatus({ cwd }), ], { concurrency: "unbounded" }, @@ -523,6 +518,7 @@ export const make = Effect.gen(function* () { _tag: "snapshot" as const, local: initialLocal, remote: initialRemote, + localGeneration: cachedStatus?.localGeneration, }), Stream.fromSubscription(subscription).pipe( Stream.filter((event) => event.cwd === cwd), diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index cbcd36ce05e..ac7cf26d438 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -270,6 +270,11 @@ export default function DiffPanel({ mode = "inline", composerDraftTarget }: Diff selectedTurn && (selectedTurn.checkpointTurnCount ?? inferredCheckpointTurnCountByTurnId[selectedTurn.turnId]); const latestTurn = orderedTurnDiffSummaries[0]; + const diffPreviewCacheKey = JSON.stringify({ + latestTurnId: latestTurn?.turnId ?? null, + localGeneration: gitStatusQuery.data?.localGeneration ?? null, + remoteRefHash: gitStatusQuery.data?.remoteRefHash ?? null, + }); const selectedScopeLabel = selectedTurnId === null ? selectedGitScope === "unstaged" @@ -316,6 +321,7 @@ export default function DiffPanel({ mode = "inline", composerDraftTarget }: Diff selectedTurnId === null && activeThread && activeCwd ? reviewEnvironment.diffPreview({ environmentId: activeThread.environmentId, + cacheKey: diffPreviewCacheKey, input: { cwd: activeCwd, ...(selectedBaseRef ? { baseRef: selectedBaseRef } : {}), @@ -333,6 +339,7 @@ export default function DiffPanel({ mode = "inline", composerDraftTarget }: Diff shouldRetryBranchDiffAtEnvironmentCwd && activeThread && serverConfig ? reviewEnvironment.diffPreview({ environmentId: activeThread.environmentId, + cacheKey: diffPreviewCacheKey, input: { cwd: serverConfig.cwd, ...(selectedBaseRef ? { baseRef: selectedBaseRef } : {}), diff --git a/packages/client-runtime/src/state/runtime.test.ts b/packages/client-runtime/src/state/runtime.test.ts index 7584e55d52e..3271cda242c 100644 --- a/packages/client-runtime/src/state/runtime.test.ts +++ b/packages/client-runtime/src/state/runtime.test.ts @@ -159,6 +159,9 @@ describe("environmentRpcKey", () => { input: originalTarget.input, }), ).not.toBe(environmentRpcKey(originalTarget)); + expect(environmentRpcKey({ ...originalTarget, cacheKey: "clean" })).not.toBe( + environmentRpcKey({ ...originalTarget, cacheKey: "dirty" }), + ); }); }); diff --git a/packages/client-runtime/src/state/runtime.ts b/packages/client-runtime/src/state/runtime.ts index 7bfeb81f5db..9397156706f 100644 --- a/packages/client-runtime/src/state/runtime.ts +++ b/packages/client-runtime/src/state/runtime.ts @@ -392,8 +392,11 @@ export async function settlePromise( export function environmentRpcKey(target: { readonly environmentId: EnvironmentIdType; readonly input: Input; + readonly cacheKey?: string; }): string { - return JSON.stringify([target.environmentId, target.input]); + return target.cacheKey === undefined + ? JSON.stringify([target.environmentId, target.input]) + : JSON.stringify([target.environmentId, target.input, target.cacheKey]); } function parseEnvironmentRpcKey(key: string): { @@ -450,6 +453,7 @@ function createEnvironmentQueryAtomFamily( ): (target: { readonly environmentId: EnvironmentIdType; readonly input: Input; + readonly cacheKey?: string; }) => Atom.Atom> { const rpcGenerationAtom = Atom.family((environmentId: EnvironmentIdType) => runtime.atom( diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index aa5cdf8432b..74affd3d109 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -223,6 +223,7 @@ const VcsStatusRemoteShape = { aheadCount: NonNegativeInt, behindCount: NonNegativeInt, aheadOfDefaultCount: Schema.optional(NonNegativeInt), + remoteRefHash: Schema.optional(TrimmedNonEmptyStringSchema), pr: Schema.NullOr(VcsStatusChangeRequest), }; @@ -235,6 +236,7 @@ export type VcsStatusRemoteResult = typeof VcsStatusRemoteResult.Type; export const VcsStatusResult = Schema.Struct({ ...VcsStatusLocalShape, ...VcsStatusRemoteShape, + localGeneration: Schema.optional(NonNegativeInt), }); export type VcsStatusResult = typeof VcsStatusResult.Type; @@ -242,9 +244,11 @@ export const VcsStatusStreamEvent = Schema.Union([ Schema.TaggedStruct("snapshot", { local: VcsStatusLocalResult, remote: Schema.NullOr(VcsStatusRemoteResult), + localGeneration: Schema.optional(NonNegativeInt), }), Schema.TaggedStruct("localUpdated", { local: VcsStatusLocalResult, + localGeneration: Schema.optional(NonNegativeInt), }), Schema.TaggedStruct("remoteUpdated", { remote: Schema.NullOr(VcsStatusRemoteResult), diff --git a/packages/shared/src/git.test.ts b/packages/shared/src/git.test.ts index 80578e262f9..53477155d62 100644 --- a/packages/shared/src/git.test.ts +++ b/packages/shared/src/git.test.ts @@ -122,6 +122,7 @@ describe("applyGitStatusStreamEvent", () => { aheadCount: 0, behindCount: 0, pr: null, + localGeneration: 3, }; const remote: VcsStatusRemoteResult = { @@ -139,4 +140,40 @@ describe("applyGitStatusStreamEvent", () => { pr: null, }); }); + + it("replaces the local generation even when the status summary is unchanged", () => { + const unchangedLocal = { + isRepo: true, + hasPrimaryRemote: true, + isDefaultRef: false, + refName: "feature/demo", + hasWorkingTreeChanges: false, + workingTree: { files: [], insertions: 0, deletions: 0 }, + } as const; + const current: VcsStatusResult = { + ...unchangedLocal, + hasUpstream: true, + aheadCount: 1, + behindCount: 0, + pr: null, + localGeneration: 1, + remoteRefHash: "remote-ref-hash", + }; + + expect( + applyGitStatusStreamEvent(current, { + _tag: "localUpdated", + local: unchangedLocal, + localGeneration: 2, + }), + ).toEqual({ + ...unchangedLocal, + hasUpstream: true, + aheadCount: 1, + behindCount: 0, + pr: null, + localGeneration: 2, + remoteRefHash: "remote-ref-hash", + }); + }); }); diff --git a/packages/shared/src/git.ts b/packages/shared/src/git.ts index ae50b148835..9f81cd693db 100644 --- a/packages/shared/src/git.ts +++ b/packages/shared/src/git.ts @@ -213,10 +213,12 @@ const EMPTY_GIT_STATUS_REMOTE: VcsStatusRemoteResult = { export function mergeGitStatusParts( local: VcsStatusLocalResult, remote: VcsStatusRemoteResult | null, + localGeneration?: number, ): VcsStatusResult { return { ...local, ...(remote ?? EMPTY_GIT_STATUS_REMOTE), + ...(localGeneration === undefined ? {} : { localGeneration }), }; } @@ -228,6 +230,7 @@ function toRemoteStatusPart(status: VcsStatusResult): VcsStatusRemoteResult { ...(status.aheadOfDefaultCount === undefined ? {} : { aheadOfDefaultCount: status.aheadOfDefaultCount }), + ...(status.remoteRefHash === undefined ? {} : { remoteRefHash: status.remoteRefHash }), pr: status.pr, }; } @@ -252,9 +255,13 @@ export function applyGitStatusStreamEvent( ): VcsStatusResult { switch (event._tag) { case "snapshot": - return mergeGitStatusParts(event.local, event.remote); + return mergeGitStatusParts(event.local, event.remote, event.localGeneration); case "localUpdated": - return mergeGitStatusParts(event.local, current ? toRemoteStatusPart(current) : null); + return mergeGitStatusParts( + event.local, + current ? toRemoteStatusPart(current) : null, + event.localGeneration ?? current?.localGeneration, + ); case "remoteUpdated": if (current === null) { return mergeGitStatusParts( @@ -269,6 +276,6 @@ export function applyGitStatusStreamEvent( event.remote, ); } - return mergeGitStatusParts(toLocalStatusPart(current), event.remote); + return mergeGitStatusParts(toLocalStatusPart(current), event.remote, current.localGeneration); } }