From 2fdbfd1810aaa61b5bdd0e7fb90f4ec1592231fe Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Wed, 28 Oct 2026 23:22:09 -0400 Subject: [PATCH 1/2] fix(server): pull request watch never wakes the agent about a replaced commit A cached read or a conflict carried over from an old head sent the agent after findings it had already pushed past. Signed-off-by: Yordis Prieto --- .../PullRequestWatchReactor.test.ts | 78 +++++++++++++++++++ .../PullRequestWatchReactor.ts | 4 +- .../orchestration-v2/pullRequestWatch.test.ts | 40 ++++++++++ .../src/orchestration-v2/pullRequestWatch.ts | 13 +++- .../src/orchestration-v2/runtimeLayer.test.ts | 7 ++ 5 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 apps/server/src/orchestration-v2/PullRequestWatchReactor.test.ts diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.test.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.test.ts new file mode 100644 index 0000000000000..20870e059d1fc --- /dev/null +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.test.ts @@ -0,0 +1,78 @@ +import { assert, describe, it } from "@effect/vitest"; +import * as Crypto from "effect/Crypto"; +import * as Effect from "effect/Effect"; + +import * as Orchestrator from "./Orchestrator.ts"; +import * as ProjectionStore from "./ProjectionStore.ts"; +import * as PullRequestService from "../pullRequest/PullRequestService.ts"; +import * as PullRequestWatchReactor from "./PullRequestWatchReactor.ts"; + +const STARTED = "2026-10-02T12:00:00.000Z"; + +const testCrypto = Crypto.make({ + randomBytes: (size) => new Uint8Array(size).fill(1), + digest: (_algorithm, data) => Effect.succeed(data), +}); + +describe("PullRequestWatchReactor", () => { + it.effect("asks the host again before reading when there is no fingerprint to compare", () => + Effect.gen(function* () { + const calls: Array = []; + const link = { + host: "github.com", + repository: "o/r", + number: 1, + url: "https://github.com/o/r/pull/1", + snapshot: null, + watch: { + startedAt: STARTED, + headSha: "aaaaaaaaaa", + failedChecks: [], + passed: false, + passedChecks: [], + remarksThrough: STARTED, + remarkIds: [], + conflicting: false, + wakes: 0, + }, + }; + const thread = { + id: "thread-1", + projectId: "project-1", + lineage: { rootThreadId: "thread-1", parentThreadId: null, relationshipToParent: null }, + settledOverride: null, + settledAt: null, + pullRequests: [link], + }; + const reactor = yield* PullRequestWatchReactor.make.pipe( + Effect.provideService(Orchestrator.OrchestratorV2, { + dispatch: () => Effect.void, + } as never), + Effect.provideService(ProjectionStore.ProjectionStoreV2, { + getThreadsWithPullRequests: () => Effect.succeed([thread]), + } as never), + Effect.provideService(PullRequestService.PullRequestService, { + watchFingerprint: () => Effect.succeed(null), + invalidate: () => Effect.sync(() => void calls.push("invalidate")), + detail: () => + Effect.sync(() => { + calls.push("detail"); + return { + state: "open", + headSha: "aaaaaaaaaa", + baseBranch: "main", + checks: [], + mergeability: "mergeable", + capabilities: { checksAlwaysReport: false }, + }; + }), + activity: () => + Effect.succeed({ comments: [], reviewThreads: [], commentsTruncated: false }), + } as never), + Effect.provideService(Crypto.Crypto, testCrypto), + ); + yield* reactor.sweep; + assert.deepEqual(calls, ["invalidate", "detail"]); + }), + ); +}); diff --git a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts index c83faa6c14b5c..00088acb68611 100644 --- a/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestWatchReactor.ts @@ -458,8 +458,8 @@ export const make = Effect.gen(function* () { threadTails.delete(group.key); } const reference = referenceOf(group); - // Invalidate cached reads before using the newer fingerprint; clients receive no notification. - if (fingerprint !== null) yield* pullRequests.invalidate({ reference }); + // A cached read can predate a push, and a viewer's read fills the cache too. + yield* pullRequests.invalidate({ reference }); const read = yield* Effect.exit( Effect.all( [ diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index 4361fae2c7eb4..f28df86f46a55 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -355,6 +355,27 @@ describe("evaluatePullRequestWatch", () => { ]); }); + it("reports a conflict on a new head even if the replaced head conflicted too", () => { + const first = evaluatePullRequestWatch( + watch(), + detail({ mergeability: "conflicting" }), + noRemarks, + ); + const pushed = evaluatePullRequestWatch( + first.next, + detail({ headSha: "bbbbbbbbbb", mergeability: "unknown" }), + noRemarks, + ); + assert.deepEqual( + evaluatePullRequestWatch( + pushed.next, + detail({ headSha: "bbbbbbbbbb", mergeability: "conflicting" }), + noRemarks, + ).changes, + [{ kind: "conflicting" }], + ); + }); + it("does not spend the comment wake limit on check results", () => { const tired = watch({ headSha: "aaaaaaaaaa", wakes: PULL_REQUEST_WATCH_WAKE_LIMIT - 1 }); const result = evaluatePullRequestWatch( @@ -420,4 +441,23 @@ describe("evaluatePullRequestWatch", () => { assert.include(pullRequestWatchMessage({ ...input, instructions }).text, instructions); assert.notInclude(pullRequestWatchMessage(input).text, "standing instructions"); }); + + it("names the head its findings are about and tells the agent to confirm it", () => { + const report = evaluatePullRequestWatch( + watch(), + detail({ mergeability: "conflicting" }), + noRemarks, + ); + const input = { + number: 7, + url: "https://github.com/pingdotgg/t3code/pull/7", + baseBranch: "main", + report, + }; + assert.include( + pullRequestWatchMessage({ ...input, headSha: "aaaaaaaaaa" }).text, + "These findings are about aaaaaaa. Before acting, confirm the pull request head is still that commit", + ); + assert.notInclude(pullRequestWatchMessage({ ...input, headSha: null }).text, "head is still"); + }); }); diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index 387a46e61c228..82a1bfcc1986d 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -129,9 +129,12 @@ export function evaluatePullRequestWatch( if (detail.mergeability === "conflicting" && !watch.conflicting) { changes.push({ kind: "conflicting" }); } - // "unknown" is a host still computing after a push; only a clean answer clears a conflict. + // "unknown" is a host still computing; only a clean answer clears a conflict. A conflict seen on + // a replaced head says nothing about the new one, which must be reported if it conflicts too. const conflicting = - detail.mergeability === "unknown" ? watch.conflicting : detail.mergeability === "conflicting"; + detail.mergeability === "unknown" + ? watch.conflicting && !headMoved + : detail.mergeability === "conflicting"; const commentsOnly = changes.length > 0 && changes.every((change) => change.kind === "remarks"); const progress = headMoved || (changes.length > 0 && !commentsOnly); @@ -232,6 +235,12 @@ export function pullRequestWatchMessage(input: { `Update on pull request #${input.number} (${input.url}), which T3 Code is watching for you:`, ...changes.flatMap((change) => changeLines(change, context)), "", + ...(input.headSha === null + ? [] + : [ + `These findings are about ${input.headSha.slice(0, 7)}. Before acting, confirm the pull request head is still that commit, and ignore findings about a commit that is no longer the head.`, + "", + ]), ...(input.instructions === undefined ? [] : [ diff --git a/apps/server/src/orchestration-v2/runtimeLayer.test.ts b/apps/server/src/orchestration-v2/runtimeLayer.test.ts index 386eeadb377d3..34fdb573c51ea 100644 --- a/apps/server/src/orchestration-v2/runtimeLayer.test.ts +++ b/apps/server/src/orchestration-v2/runtimeLayer.test.ts @@ -2984,6 +2984,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.die("host unreachable"), activity: () => Effect.die("host unreachable"), }), @@ -3045,6 +3046,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const host = Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ @@ -3143,6 +3145,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const host = Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ @@ -3287,6 +3290,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const host = Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ projectId, number: 13, at: commentAt }), @@ -3385,6 +3389,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const host = Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ @@ -3488,6 +3493,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { const host = Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.sync(() => ({ ...watchedPullRequestDetail({ @@ -3718,6 +3724,7 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => { Layer.mergeAll( NodeServices.layer, Layer.mock(PullRequestService.PullRequestService)({ + invalidate: () => Effect.void, detail: () => Effect.succeed(detail), activity: () => Effect.succeed({ From 2ea27de860335d0285153981a4cd2b164579ef2d Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 29 Oct 2026 00:06:05 -0400 Subject: [PATCH 2/2] fix(server): a push that still conflicts wakes the agent A conflict told about a replaced commit says nothing about the new head, so a push that fails to resolve it must be reported again. Signed-off-by: Yordis Prieto --- apps/server/src/orchestration-v2/pullRequestWatch.test.ts | 8 ++++++++ apps/server/src/orchestration-v2/pullRequestWatch.ts | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts index f28df86f46a55..acd39298d2724 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.test.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.test.ts @@ -374,6 +374,14 @@ describe("evaluatePullRequestWatch", () => { ).changes, [{ kind: "conflicting" }], ); + assert.deepEqual( + evaluatePullRequestWatch( + first.next, + detail({ headSha: "bbbbbbbbbb", mergeability: "conflicting" }), + noRemarks, + ).changes, + [{ kind: "conflicting" }], + ); }); it("does not spend the comment wake limit on check results", () => { diff --git a/apps/server/src/orchestration-v2/pullRequestWatch.ts b/apps/server/src/orchestration-v2/pullRequestWatch.ts index 82a1bfcc1986d..98836fe8c41a7 100644 --- a/apps/server/src/orchestration-v2/pullRequestWatch.ts +++ b/apps/server/src/orchestration-v2/pullRequestWatch.ts @@ -126,7 +126,7 @@ export function evaluatePullRequestWatch( ...atLatest.map((remark) => remark.id), ]; - if (detail.mergeability === "conflicting" && !watch.conflicting) { + if (detail.mergeability === "conflicting" && (!watch.conflicting || headMoved)) { changes.push({ kind: "conflicting" }); } // "unknown" is a host still computing; only a clean answer clears a conflict. A conflict seen on