diff --git a/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.test.tsx b/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.test.tsx new file mode 100644 index 0000000000000..d5794cf908a69 --- /dev/null +++ b/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.test.tsx @@ -0,0 +1,140 @@ +import { RegistryContext } from "@effect/atom-react"; +import type { PullRequestChecks, PullRequestDetail } from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; +import { AsyncResult, Atom, AtomRegistry } from "effect/reactivity"; +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { expect, it, vi } from "vite-plus/test"; + +const fixture = vi.hoisted(() => ({ + active: "active", + focused: true, + reads: 0, + appListeners: new Set<() => void>(), + focusListeners: new Set<() => void>(), + query: null as Atom.Atom> | null, +})); + +vi.mock("react-native", () => ({ + AppState: { + get currentState() { + return fixture.active; + }, + addEventListener: (_event: string, listener: () => void) => { + fixture.appListeners.add(listener); + return { remove: () => fixture.appListeners.delete(listener) }; + }, + }, +})); +vi.mock("@react-navigation/native", async () => { + const { useEffect, useSyncExternalStore } = await import("react"); + const subscribe = (listener: () => void) => { + fixture.focusListeners.add(listener); + return () => fixture.focusListeners.delete(listener); + }; + const useIsFocused = () => useSyncExternalStore(subscribe, () => fixture.focused); + return { + useIsFocused, + useFocusEffect: (effect: () => (() => void) | undefined) => { + const focused = useIsFocused(); + useEffect(() => (focused ? effect() : undefined), [effect, focused]); + }, + }; +}); +vi.mock("../../state/entities", () => ({ + useEnvironmentServerConfig: () => ({ + environment: { capabilities: { pullRequestChecks: true } }, + }), +})); +vi.mock("../../state/pull-requests", () => ({ + composerPullRequests: { checks: () => fixture.query }, +})); +vi.mock("./usePullRequestConnected", () => ({ usePullRequestConnected: () => true })); +vi.mock("./usePullRequestDiff", () => ({ pullRequestTarget: (params: unknown) => params })); + +import { usePullRequestLiveChecks } from "./usePullRequestLiveChecks"; + +function setup(active = "active") { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + vi.useFakeTimers({ + toFake: ["Date", "setTimeout", "clearTimeout", "setInterval", "clearInterval"], + }); + vi.setSystemTime(0); + fixture.active = active; + fixture.focused = true; + fixture.reads = 0; + fixture.appListeners.clear(); + fixture.focusListeners.clear(); + fixture.query = Atom.make( + Effect.sync(() => { + fixture.reads++; + return { state: "open" as const, checks: [], headSha: "head", observedAt: fixture.reads }; + }), + ).pipe(Atom.setIdleTTL(0)); + const registry = AtomRegistry.make(); + const root = createRoot(document.createElement("div")); + const detail = { state: "open", checks: [], observedAt: 0 } as unknown as PullRequestDetail; + function Probe() { + usePullRequestLiveChecks( + { environmentId: "local", projectId: "project", repository: "acme/app", number: 7 }, + detail, + ); + return null; + } + act(() => + root.render( + + + , + ), + ); + return { + active(state: string) { + act(() => { + fixture.active = state; + for (const listener of fixture.appListeners) listener(); + }); + }, + focused(focused: boolean) { + act(() => { + fixture.focused = focused; + for (const listener of fixture.focusListeners) listener(); + }); + }, + async advance(milliseconds: number) { + await act(async () => vi.advanceTimersByTimeAsync(milliseconds)); + }, + [Symbol.dispose]() { + act(() => root.unmount()); + registry.dispose(); + vi.useRealTimers(); + }, + }; +} + +it("polls only the foreground focused route and resumes one checks subscription", async () => { + using screen = setup(); + expect(fixture.reads).toBe(1); + await screen.advance(45_000); + expect(fixture.reads).toBe(2); + screen.active("background"); + await screen.advance(180_000); + expect(fixture.reads).toBe(2); + screen.active("active"); + expect(fixture.reads).toBe(3); + await screen.advance(45_000); + expect(fixture.reads).toBe(4); + screen.focused(false); + await screen.advance(180_000); + expect(fixture.reads).toBe(4); + screen.focused(true); + expect(fixture.reads).toBe(5); +}); + +it("does not subscribe to checks when the route first mounts in the background", async () => { + using screen = setup("background"); + await screen.advance(180_000); + expect(fixture.reads).toBe(0); + screen.active("active"); + expect(fixture.reads).toBe(1); +}); diff --git a/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.ts b/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.ts index b7ce339761704..d108290b10940 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestLiveChecks.ts @@ -1,9 +1,11 @@ -import { useFocusEffect } from "@react-navigation/native"; +import { useIsFocused } from "@react-navigation/native"; +import { useEffect } from "react"; import { pullRequestChecksRefreshIntervalMs, resolvePullRequestDetailChecks, } from "@t3tools/client-runtime/pull-request-checks"; import type { PullRequestDetail } from "@t3tools/contracts"; +import { useAppActive } from "../../lib/appState"; import { useEnvironmentServerConfig } from "../../state/entities"; import { composerPullRequests } from "../../state/pull-requests"; import { useEnvironmentQuery } from "../../state/query"; @@ -19,24 +21,35 @@ export function usePullRequestLiveChecks( detail: PullRequestDetail | null, ): PullRequestDetail | null { const target = pullRequestTarget(params); + const active = useAppActive(); + const focused = useIsFocused(); const connected = usePullRequestConnected(target.environmentId); const supported = useEnvironmentServerConfig(target.environmentId)?.environment.capabilities.pullRequestChecks === true; const checks = useEnvironmentQuery( - supported && detail !== null ? composerPullRequests.checks(target) : null, + active && focused && connected && supported && detail !== null + ? composerPullRequests.checks(target) + : null, ); // Both reads carry the server's observedAt; without it the poll is the fresher stream. const live = resolvePullRequestDetailChecks(detail, checks.data, true); // A failed read keeps polling; only a host that answers "no checks" (null) stops it. const enabled = - connected && supported && live?.state === "open" && !(checks.isSuccess && checks.data === null); + active && + focused && + connected && + supported && + !checks.isPending && + live?.state === "open" && + !(checks.isSuccess && checks.data === null); const intervalMs = pullRequestChecksRefreshIntervalMs(live?.checks ?? []); const { refresh } = checks; - useFocusEffect(() => { + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- polls while the route is focused and the native app is active + useEffect(() => { if (!enabled) return; const timer = setInterval(refresh, intervalMs); return () => clearInterval(timer); - }); + }, [enabled, intervalMs, refresh]); return live; } diff --git a/apps/mobile/vite.config.ts b/apps/mobile/vite.config.ts index e3e21de11d9dd..603bbea0aac1e 100644 --- a/apps/mobile/vite.config.ts +++ b/apps/mobile/vite.config.ts @@ -22,6 +22,7 @@ const reactTests = [ "src/features/files/FileCodeRelationships.test.tsx", "src/features/loom/ProjectLoomRouteScreen.test.tsx", "src/features/pull-requests/usePullRequestList.test.tsx", + "src/features/pull-requests/usePullRequestLiveChecks.test.tsx", "src/features/files/ZipArchiveView.test.tsx", "src/features/pull-requests/guide/useReviewGuide.test.tsx", "src/features/settings/SettingsServerControlsRouteScreen.test.tsx", diff --git a/apps/server/src/bin.ts b/apps/server/src/bin.ts index ea863624ca225..d5dec56611989 100644 --- a/apps/server/src/bin.ts +++ b/apps/server/src/bin.ts @@ -17,7 +17,10 @@ if ( // Guard sockets before either dispatch path can load outbound-capable modules. const { ignoreMacosTypeOfServiceEinval } = await import("./outboundHttp.ts"); ignoreMacosTypeOfServiceEinval(); - if (command === "__document-text") { + if (command === "__github-cli") { + const { runAgentGitHubCli } = await import("./sourceControl/agentGitHubCli.ts"); + await runAgentGitHubCli(process.argv[3]!, process.argv.slice(4)); + } else if (command === "__document-text") { const { extractDocumentText } = await import("./provider/documentTextWorker.ts"); try { process.stdout.write(await extractDocumentText(process.argv[3]!, process.argv[4]!)); diff --git a/apps/server/src/mcp/toolkits/pullRequests/tools.ts b/apps/server/src/mcp/toolkits/pullRequests/tools.ts index 8b89b669d2f3d..b5da76cba42b7 100644 --- a/apps/server/src/mcp/toolkits/pullRequests/tools.ts +++ b/apps/server/src/mcp/toolkits/pullRequests/tools.ts @@ -423,7 +423,7 @@ const PullRequestStatusTool = Tool.make("pull_request_status", { const PullRequestFailuresTool = Tool.make("pull_request_failures", { description: - "Why a PR's CI failed: each failing check's failed steps, annotations, and a bounded log excerpt around the errors. Use instead of gh run view --log-failed or gh api .../jobs//logs.", + "Why one PR's CI failed: each failing check's failed steps, annotations, and a bounded log excerpt around the errors. Accepts one PR per call; make separate calls for several PRs. Use instead of gh run view --log-failed or gh api .../jobs//logs.", parameters: Schema.Struct({ environmentId: OrchestratorMcpEnvironmentTarget, ...PullRequestFailuresInput.fields, @@ -440,7 +440,7 @@ const PullRequestFailuresTool = Tool.make("pull_request_failures", { const PullRequestFeedbackTool = Tool.make("pull_request_feedback", { description: - "Unresolved review threads, review verdicts, and comments on a PR, bot findings included, newest first. Use instead of gh api .../reviews, .../comments, or reviewThreads GraphQL.", + "Unresolved review threads, review verdicts, and comments on one PR, bot findings included, newest first. Accepts one PR per call; make separate calls for several PRs. Use instead of gh api .../reviews, .../comments, or reviewThreads GraphQL.", parameters: Schema.Struct({ environmentId: OrchestratorMcpEnvironmentTarget, ...PullRequestReadInput.fields, diff --git a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts index 036d4cb42e785..9cbbc2ee96dc5 100644 --- a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts +++ b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts @@ -1142,12 +1142,12 @@ describe("CodexAdapterV2 post-settle continuation", () => { onRequest: (method: string, params: unknown) => Effect.Effect = () => Effect.void, readChildMetadata?: Parameters[2], runtimeOptions: Partial< - Pick - > & { readonly modelProvider?: string | null } = {}, + Pick + > & { readonly modelProvider?: string | null; readonly shellPath?: string } = {}, beforeEmitInbound?: CodexReplay.CodexAppServerReplayDriver["beforeEmitInbound"], ) => Effect.gen(function* () { - const { modelProvider = null, ...adapterRuntimeOptions } = runtimeOptions; + const { modelProvider = null, shellPath, ...adapterRuntimeOptions } = runtimeOptions; const fileSystem = yield* FileSystem.FileSystem; const idAllocator = yield* IdAllocator.IdAllocatorV2; const serverConfig = yield* ProviderReplayHarness.makeReplayServerConfig( @@ -1188,7 +1188,12 @@ describe("CodexAdapterV2 post-settle continuation", () => { ? Effect.sync(() => { configReadRequests.push(params); return { - config: { model_provider: modelProvider }, + config: { + model_provider: modelProvider, + ...(shellPath === undefined + ? {} + : { shell_environment_policy: { set: { PATH: shellPath } } }), + }, origins: {}, }; }) @@ -1208,8 +1213,8 @@ describe("CodexAdapterV2 post-settle continuation", () => { const adapter = yield* CodexAdapterV2.makeCodexAdapterV2({ instanceId: CodexAdapterV2.CODEX_DEFAULT_INSTANCE_ID, settings: DEFAULT_CODEX_SETTINGS, - ...adapterRuntimeOptions, environment: {}, + ...adapterRuntimeOptions, clientFactory, crypto: yield* Crypto.Crypto, fileSystem, @@ -1591,6 +1596,56 @@ describe("CodexAdapterV2 post-settle continuation", () => { ), ); + it.effect.each([undefined, "/explicit/codex/bin"])( + "retains the hydrated managed gh PATH while respecting explicit Codex PATH %s", + (shellPath) => + Effect.gen(function* () { + const managedPath = + "/t3/github/bin/launcher:/user/.nvm/bin:/user/.asdf/shims:/user/bin:/bin"; + const expectedConfig = { + ...CodexAdapterV2.CODEX_THREAD_CONFIG, + ...(shellPath === undefined ? { "shell_environment_policy.set.PATH": managedPath } : {}), + }; + const entries = codexReplayPreamble({ + nativeThreadId: "managed-gh-thread", + nativeTurnId: "unused", + prompt: "unused", + }).slice(0, 5); + entries[3] = { + type: "expect_outbound", + label: "thread/start", + frame: { id: 2, method: "thread/start", params: { config: expectedConfig } }, + }; + const requests: Array = []; + const harness = yield* makeCodexReplayHarness( + makeCodexReplayTranscript({ scenario: "managed-gh-path", entries }), + () => Effect.void, + (method, params) => + Effect.sync(() => { + if (method === "thread/start") requests.push(params); + }), + undefined, + { + ...(shellPath === undefined ? {} : { shellPath }), + environment: { + PATH: managedPath, + T3_GH_SHIM_DIR: "/t3/github/bin/launcher", + CUSTOM: "preserved", + }, + }, + ); + assert.deepEqual(requests, [ + { cwd: "/workspace", model: "gpt-5.4", config: expectedConfig }, + ]); + assert.equal(harness.configReadRequests.length, 1); + }).pipe( + Effect.scoped, + Effect.provide( + Layer.mergeAll(IdAllocator.layer, McpProviderSessions.layer, NodeServices.layer), + ), + ), + ); + it.effect("adds the session MCP server without replacing launch-argument servers", () => Effect.gen(function* () { const scenario = "launch-mcp-servers"; diff --git a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts index 3e094a5b15537..330cdd50ea85d 100644 --- a/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts +++ b/apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts @@ -1331,8 +1331,15 @@ export const CODEX_THREAD_CONFIG = { // T3 waits can block for an hour; keep Codex's MCP timeout above that. const CODEX_T3_MCP_TOOL_TIMEOUT_SEC = 65 * 60; +const decodeCodexShellEnvironment = Schema.decodeUnknownOption( + Schema.Struct({ + shell_environment_policy: Schema.Struct({ set: Schema.Record(Schema.String, Schema.String) }), + }), +); + function codexThreadRuntimeParams(input: { readonly mcpSession: McpProviderSession.McpProviderSessionConfig | undefined; + readonly environment: NodeJS.ProcessEnv; readonly modelSelection?: { readonly model: string }; readonly runtimePolicy?: ProviderAdapter.ProviderAdapterV2RuntimePolicy; }): { @@ -1341,11 +1348,20 @@ function codexThreadRuntimeParams(input: { readonly config: Readonly>; } { const { mcpSession } = input; + const managedPath = + input.environment.T3_GH_SHIM_DIR === undefined + ? undefined + : input.environment[ + Object.keys(input.environment).find((key) => key.toUpperCase() === "PATH") ?? "PATH" + ]; return { ...(input.runtimePolicy?.cwd == null ? {} : { cwd: input.runtimePolicy.cwd }), ...(input.modelSelection === undefined ? {} : { model: input.modelSelection.model }), config: { ...CODEX_THREAD_CONFIG, + // Reapply the hydrated startup PATH after Codex's snapshot, retaining startup profile + // tools and our gh launcher. Login profiles stay enabled. Explicit user PATH config wins. + ...(managedPath === undefined ? {} : { "shell_environment_policy.set.PATH": managedPath }), ...(mcpSession === undefined ? {} : { @@ -1668,13 +1684,14 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( }), ), ); + const environment = resolvedRuntime?.environment ?? adapterOptions.environment; const client = yield* clientFactory.open({ instanceId: adapterOptions.instanceId, threadId: input.threadId, providerSessionId: input.providerSessionId, runtimePolicy: input.runtimePolicy, settings: resolvedRuntime?.config ?? adapterOptions.settings, - environment: resolvedRuntime?.environment ?? adapterOptions.environment, + environment, }); const additionalContextByThread = yield* Ref.make( new Map< @@ -1763,6 +1780,7 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( // A failed open is discarded, so the next turn spawns the updated binary. if (adapterOptions.compatibility !== undefined) yield* ensureInitialized; const now = yield* DateTime.now; + const configuredShellPaths = yield* Ref.make(new Set()); const readModelProvider = (runtimePolicy = input.runtimePolicy) => ensureInitialized.pipe( Effect.andThen( @@ -1772,8 +1790,28 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( }), ), Effect.flatMap(Schema.decodeUnknownEffect(CodexSchema.V2ConfigReadResponse)), + Effect.tap((response) => + Ref.update(configuredShellPaths, (current) => { + const next = new Set(current); + const configured = decodeCodexShellEnvironment(response.config); + const hasPath = + Option.isSome(configured) && + Object.keys(configured.value.shell_environment_policy.set).some( + (key) => key.toUpperCase() === "PATH", + ); + if (hasPath) next.add(runtimePolicy.cwd ?? ""); + else next.delete(runtimePolicy.cwd ?? ""); + return next; + }), + ), Effect.map((response) => response.config.model_provider ?? "openai"), ); + const threadEnvironment = (runtimePolicy = input.runtimePolicy) => + Ref.get(configuredShellPaths).pipe( + Effect.map((configured) => + configured.has(runtimePolicy.cwd ?? "") ? {} : environment, + ), + ); const session = providerSession({ providerSessionId: input.providerSessionId, providerInstanceId: adapterOptions.instanceId, @@ -6968,15 +7006,25 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( }), ensureThread: (threadInput) => ensureInitialized.pipe( + Effect.andThen( + environment.T3_GH_SHIM_DIR === undefined + ? Effect.void + : readModelProvider(threadInput.runtimePolicy), + ), Effect.andThen(mcpSessions.read(threadInput.threadId)), Effect.flatMap((mcpSession) => - client.request( - "thread/start", - codexThreadRuntimeParams({ - mcpSession, - modelSelection: threadInput.modelSelection, - runtimePolicy: threadInput.runtimePolicy, - }), + threadEnvironment(threadInput.runtimePolicy).pipe( + Effect.flatMap((environment) => + client.request( + "thread/start", + codexThreadRuntimeParams({ + environment, + mcpSession, + modelSelection: threadInput.modelSelection, + runtimePolicy: threadInput.runtimePolicy, + }), + ), + ), ), ), Effect.map((response): OrchestrationV2ProviderThread => @@ -7006,6 +7054,7 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( modelProvider: yield* readModelProvider(threadInput.runtimePolicy), excludeTurns: true, ...codexThreadRuntimeParams({ + environment: yield* threadEnvironment(threadInput.runtimePolicy), mcpSession: yield* readMcpSession( threadInput.threadId ?? threadInput.providerThread.appThreadId, ), @@ -7794,6 +7843,7 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( modelProvider: yield* readModelProvider(), excludeTurns: true, ...codexThreadRuntimeParams({ + environment: yield* threadEnvironment(), mcpSession: yield* readMcpSession(threadInput.providerThread.appThreadId), modelSelection: input.modelSelection, runtimePolicy: input.runtimePolicy, @@ -7847,6 +7897,7 @@ export const makeCodexAdapterV2 = Effect.fn("makeCodexAdapterV2")(function* ( ? {} : { lastTurnId: boundary.lastTurnId }), ...codexThreadRuntimeParams({ + environment: yield* threadEnvironment(threadInput.runtimePolicy), mcpSession: yield* mcpSessions.read(threadInput.targetThreadId), ...(threadInput.modelSelection === undefined ? {} diff --git a/apps/server/src/orchestration-v2/FoundationPersistence.test.ts b/apps/server/src/orchestration-v2/FoundationPersistence.test.ts index 7b9c7c3b528b8..45d4b9d998634 100644 --- a/apps/server/src/orchestration-v2/FoundationPersistence.test.ts +++ b/apps/server/src/orchestration-v2/FoundationPersistence.test.ts @@ -2688,6 +2688,7 @@ it.layer(layerTest)("orchestration V2 foundation persistence", (it) => { ); const layerWorker = EffectWorker.layerWithOptions({ workerId: "hung-delegated-stop-worker", + maxAttempts: 1, }).pipe( Layer.provide( Layer.merge(Layer.succeed(EffectOutbox.EffectOutboxV2, outbox), layerExecutor), @@ -2707,15 +2708,8 @@ it.layer(layerTest)("orchestration V2 foundation persistence", (it) => { ]); yield* outbox.notifyAvailable(); - const turn = yield* Deferred.await(turnStarted).pipe( - Effect.timeoutOption("1 hour"), - Effect.forkChild, - ); - yield* TestClock.adjust("1 hour"); - assert.isTrue( - Option.isSome(yield* Fiber.join(turn)), - "provider-turn.start never ran behind the hung delegated-tasks.stop", - ); + yield* TestClock.adjust("30 seconds"); + yield* Deferred.await(turnStarted); const stop = yield* outbox.get(stopEffectId); assert.isTrue(Option.isSome(stop)); if (Option.isSome(stop)) { diff --git a/apps/server/src/orchestration-v2/RunFinalizationService.test.ts b/apps/server/src/orchestration-v2/RunFinalizationService.test.ts index 38b6732149289..455e340aa528b 100644 --- a/apps/server/src/orchestration-v2/RunFinalizationService.test.ts +++ b/apps/server/src/orchestration-v2/RunFinalizationService.test.ts @@ -17,6 +17,7 @@ import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import * as Stream from "effect/Stream"; +import * as TestClock from "effect/testing/TestClock"; import * as CodeIntelligence from "../project/knowledge/CodeIntelligence.ts"; import * as PullRequestService from "../pullRequest/PullRequestService.ts"; @@ -29,6 +30,47 @@ import * as ProjectionStore from "./ProjectionStore.ts"; import * as ProjectStore from "./ProjectStore.ts"; import * as RunFinalization from "./RunFinalizationService.ts"; +it.effect("coalesces completed turns without starving later project refreshes", () => { + const projectA = ProjectId.make("project-turn-refresh-a"); + const projectB = ProjectId.make("project-turn-refresh-b"); + const refreshed: ProjectId[] = []; + const layer = RunFinalization.layerObserver.pipe( + Layer.provide( + Layer.mergeAll( + Layer.mock(WorkspaceEntries.WorkspaceEntries)({}), + Layer.mock(CodeIntelligence.CodeIntelligence)({}), + Layer.mock(VcsStatusBroadcaster.VcsStatusBroadcaster)({}), + Layer.mock(ProjectionStore.ProjectionStoreV2)({}), + Layer.mock(PullRequestService.PullRequestService)({ + refreshAfterTurns: (projectIds) => + Effect.sync(() => { + refreshed.push(...projectIds); + }), + }), + ), + ), + ); + return Effect.gen(function* () { + const observer = yield* RunFinalization.RunFinalizationObserver; + yield* Effect.forEach(Array.from({ length: 20 }), () => observer.refreshAfterTurn(projectA)); + assert.deepEqual(refreshed, []); + yield* TestClock.adjust("9 seconds"); + yield* observer.refreshAfterTurn(projectA); + yield* observer.refreshAfterTurn(projectB); + yield* TestClock.adjust("1 second"); + assert.deepEqual(refreshed, [projectA, projectB]); + + // Continuous activity starts a new fixed window instead of postponing the last refresh. + yield* observer.refreshAfterTurn(projectA); + yield* TestClock.adjust("9 seconds"); + yield* observer.refreshAfterTurn(projectA); + yield* TestClock.adjust("1 second"); + assert.deepEqual(refreshed, [projectA, projectB, projectA]); + yield* TestClock.adjust("1 minute"); + assert.deepEqual(refreshed, [projectA, projectB, projectA]); + }).pipe(Effect.provide(layer)); +}); + it.effect("completes finalization when the post-checkpoint workspace refresh fails", () => { const threadId = ThreadId.make("thread_refresh_failure"); const runId = RunId.make("run_refresh_failure"); diff --git a/apps/server/src/orchestration-v2/RunFinalizationService.ts b/apps/server/src/orchestration-v2/RunFinalizationService.ts index 5951580a36e77..6c8fa30f260a3 100644 --- a/apps/server/src/orchestration-v2/RunFinalizationService.ts +++ b/apps/server/src/orchestration-v2/RunFinalizationService.ts @@ -2,6 +2,7 @@ import { CheckpointScopeId, ProjectId, RunId, ThreadId } from "@t3tools/contract import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; +import * as Queue from "effect/Queue"; import * as Schema from "effect/Schema"; import * as CodeIntelligence from "../project/knowledge/CodeIntelligence.ts"; @@ -98,8 +99,36 @@ export const layerObserver = Layer.effect( const projections = yield* ProjectionStore.ProjectionStoreV2; const pullRequests = yield* PullRequestService.PullRequestService; const codeIntelligence = yield* CodeIntelligence.CodeIntelligence; + + // Many agents can finish in the same project at once. Invalidate once per project in a + // fixed window, with a trailing refresh even under continuous activity. Pushes and explicit + // PR mutations still call PullRequestService directly and refresh immediately. + const pendingProjects = new Set(); + const refreshes = yield* Queue.dropping(1); + const refreshAfterTurn = Effect.fn("RunFinalizationService.refreshAfterTurn")( + (projectId: ProjectId) => + Effect.sync(() => { + const first = pendingProjects.size === 0; + pendingProjects.add(projectId); + if (first) Queue.offerUnsafe(refreshes, undefined); + }), + ); + yield* Effect.gen(function* () { + yield* Queue.take(refreshes); + yield* Effect.sleep("10 seconds"); + const projects = Array.from(pendingProjects); + pendingProjects.clear(); + yield* pullRequests.refreshAfterTurns(projects).pipe( + Effect.catchCause((cause) => + Effect.logWarning("failed to refresh pull requests after completed turns", { + projectIds: projects, + cause, + }), + ), + ); + }).pipe(Effect.forever, Effect.forkScoped); return { - refreshAfterTurn: pullRequests.refreshAfterTurn, + refreshAfterTurn, refresh: Effect.fn("RunFinalizationService.refresh")( function* ({ cwd, threadId, runId }) { // The index builds in the code intelligence service, off this turn's lane. diff --git a/apps/server/src/orchestration-v2/testkit/ReplayRuntimeInstructions.ts b/apps/server/src/orchestration-v2/testkit/ReplayRuntimeInstructions.ts index 83d5c4c930f36..a252866428bf0 100644 --- a/apps/server/src/orchestration-v2/testkit/ReplayRuntimeInstructions.ts +++ b/apps/server/src/orchestration-v2/testkit/ReplayRuntimeInstructions.ts @@ -23,6 +23,10 @@ export function materializeReplayTranscriptRuntimeInstructions( const pullRequestInstructions = instructions.match( /[\s\S]*?<\/pull_request_linking>/u, )?.[0]; + const githubReadInstructions = instructions.match(/[\s\S]*?<\/github_reads>/u)?.[0]; + const managedInstructions = [pullRequestInstructions, githubReadInstructions] + .filter((block): block is string => block !== undefined) + .join("\n\n"); return { ...transcript, @@ -55,8 +59,8 @@ export function materializeReplayTranscriptRuntimeInstructions( } // Keep each turn's recorded model, effort, and user input as protocol assertions. const text = firstPart.text.replace( - /^([\s\S]*?<\/runtime_info>\n\n)[\s\S]*?<\/pull_request_linking>/u, - (_match, header: string) => `${header}${pullRequestInstructions}`, + /^([\s\S]*?<\/runtime_info>\n\n)[\s\S]*?<\/pull_request_linking>(?:\n\n[\s\S]*?<\/github_reads>)?/u, + (_match, header: string) => `${header}${managedInstructions}`, ); return text === firstPart.text ? entry diff --git a/apps/server/src/provider/Drivers/AntigravityDriver.test.ts b/apps/server/src/provider/Drivers/AntigravityDriver.test.ts index 74f4daa1323af..84faf0352b140 100644 --- a/apps/server/src/provider/Drivers/AntigravityDriver.test.ts +++ b/apps/server/src/provider/Drivers/AntigravityDriver.test.ts @@ -289,10 +289,14 @@ it.layer(layerTest)("AntigravityDriver", (it) => { h.controls.selected = h.second; yield* h.refresh(); - expect(h.acquisitions).toEqual([ - { binaryPath: "", path: h.instancePath }, - { binaryPath: "", path: h.instancePath }, - ]); + expect(h.acquisitions).toHaveLength(2); + expect(h.acquisitions.map(({ binaryPath }) => binaryPath)).toEqual(["", ""]); + expect(h.acquisitions[0]?.path).toBe(h.acquisitions[1]?.path); + const acquiredPath = h.acquisitions[0]?.path; + // The managed gh launcher may prefix PATH, but must preserve the provider's PATH. + expect( + acquiredPath === h.instancePath || acquiredPath?.endsWith(`:${h.instancePath}`), + ).toBe(true); const nativeLaunches = h.launches.filter((launch) => launch.harnessPath !== undefined); expect(nativeLaunches.map((launch) => launch.command)).toEqual([ h.first.executablePath, diff --git a/apps/server/src/provider/ProviderHostLive.ts b/apps/server/src/provider/ProviderHostLive.ts index 8d73d697705fc..fa10b871d8473 100644 --- a/apps/server/src/provider/ProviderHostLive.ts +++ b/apps/server/src/provider/ProviderHostLive.ts @@ -1,15 +1,19 @@ import * as ProviderHost from "@t3tools/provider-core/server/ProviderHost"; import { ProviderCredentialError } from "@t3tools/provider-core/server/errors"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { resolveSelfInvocation } from "@t3tools/shared/nodeRuntime"; import * as Crypto from "effect/Crypto"; import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; +import * as Path from "effect/Path"; import { resolveAttachmentPath } from "../attachmentStore.ts"; import * as ServerSecretStore from "../auth/ServerSecretStore.ts"; import * as BackgroundPolicy from "../background/BackgroundPolicy.ts"; import * as ServerConfig from "../config.ts"; import * as ServerSettings from "../serverSettings.ts"; +import { prepareAgentGitHubEnvironment } from "../sourceControl/agentGitHubEnvironment.ts"; import * as DocumentText from "./DocumentText.ts"; import * as ProviderCredentialStore from "./ProviderCredentialStore.ts"; @@ -22,6 +26,9 @@ export const layer = Layer.effect( const secrets = yield* ServerSecretStore.ServerSecretStore; const crypto = yield* Crypto.Crypto; const fileSystem = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const platform = yield* HostProcess.Platform; + const invocation = yield* resolveSelfInvocation(); return ProviderHost.ProviderHost.of({ paths: { cwd: config.cwd, @@ -37,6 +44,28 @@ export const layer = Layer.effect( subscribe: serverSettings.subscribeChanges, }, shouldRunBackgroundWork: backgroundPolicy.shouldRunScopeWork, + prepareAgentEnvironment: Effect.fn("ProviderHost.prepareAgentEnvironment")((environment) => + serverSettings.getSettings.pipe( + Effect.flatMap((settings) => + prepareAgentGitHubEnvironment({ + environment, + enabled: settings.enableAgentGitHubReadGate, + stateDir: config.stateDir, + invocation, + platform, + }), + ), + Effect.provideService(FileSystem.FileSystem, fileSystem), + Effect.provideService(Path.Path, path), + Effect.provideService(Crypto.Crypto, crypto), + Effect.provideService(HostProcess.Platform, platform), + Effect.catch(() => + Effect.logWarning("Could not prepare the managed GitHub CLI launcher.").pipe( + Effect.as(environment), + ), + ), + ), + ), resolveAttachmentPath: (attachment) => resolveAttachmentPath({ attachmentsDir: config.attachmentsDir, attachment }), // DocumentText owns the path-resolution span. diff --git a/apps/server/src/provider/agentContextBudget.json b/apps/server/src/provider/agentContextBudget.json index 1251013e705cc..6224211bbfeb7 100644 --- a/apps/server/src/provider/agentContextBudget.json +++ b/apps/server/src/provider/agentContextBudget.json @@ -1,5 +1,5 @@ { "claudeSystemAppendBytes": 8528, "codexAdditionalContextBytes": 10254, - "mcpToolListJsonBytes": 63134 + "mcpToolListJsonBytes": 63454 } diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 6e4c69d2c2f01..8f57de29f79bd 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1496,53 +1496,61 @@ it.effect("keeps two hosts of one provider kind as two accounts", () => }), ); -it.effect("stops new reads after a rate limit while leaving manual actions available", () => - Effect.gen(function* () { - let listCalls = 0; - let actionCalls = 0; - const service = yield* makeService({ - projects: [ - project({ id: "p1", title: "cloud", workspaceRoot: "/cloud", repository: "acme/web" }), - ], - providers: [ - fakePullRequestProvider(SourceControlProviderKind.make("github"), { - listChangeRequests: () => - Effect.sync(() => { - listCalls += 1; - }).pipe( - Effect.andThen( - Effect.fail( - new PullRequestProviderError({ - provider: SourceControlProviderKind.make("github"), - operation: "listChangeRequests", - reason: "rate-limited", - detail: "GitHub API rate limit exceeded.", - }), +it.effect.each(["github", "gitlab"])( + "leaves GitHub backoff to its transport and protects other providers (%s)", + (kind) => + Effect.gen(function* () { + let listCalls = 0; + let actionCalls = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "cloud", + workspaceRoot: "/cloud", + repository: "acme/web", + provider: kind, + }), + ], + providers: [ + fakePullRequestProvider(SourceControlProviderKind.make(kind), { + listChangeRequests: () => + Effect.sync(() => { + listCalls += 1; + }).pipe( + Effect.andThen( + Effect.fail( + new PullRequestProviderError({ + provider: SourceControlProviderKind.make(kind), + operation: "listChangeRequests", + reason: "rate-limited", + detail: "GitHub API rate limit exceeded.", + }), + ), ), ), - ), - runAction: () => - Effect.sync(() => { - actionCalls += 1; - }), - }), - ], - }); + runAction: () => + Effect.sync(() => { + actionCalls += 1; + }), + }), + ], + }); - const first = yield* service.list({ state: "open", involvement: "all" }); - const paused = yield* service.list({ state: "open", involvement: "authored" }); - yield* service.runAction({ - projectId: "p1" as ProjectId, - repository: "acme/web", - number: 1, - action: "close", - }); + const first = yield* service.list({ state: "open", involvement: "all" }); + const paused = yield* service.list({ state: "open", involvement: "authored" }); + yield* service.runAction({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + action: "close", + }); - assert.strictEqual(listCalls, 1); - assert.strictEqual(actionCalls, 1); - assert.lengthOf(first.errors, 1); - assert.lengthOf(paused.errors, 1); - }), + assert.strictEqual(listCalls, kind === "github" ? 2 : 1); + assert.strictEqual(actionCalls, 1); + assert.lengthOf(first.errors, 1); + assert.lengthOf(paused.errors, 1); + }), ); it.effect("refuses a repository that does not belong to the requested project", () => @@ -2291,6 +2299,12 @@ it.effect("a refresh or a finished turn drops the provider's own change request undefined, undefined, ]); + + forgotten.length = 0; + yield* service.refreshAfterTurns([reference.projectId, "p2" as ProjectId, reference.projectId]); + assert.deepStrictEqual(forgotten, [undefined]); + yield* service.refreshAfterTurns([]); + assert.deepStrictEqual(forgotten, [undefined]); }), ); @@ -2310,6 +2324,13 @@ it.effect("a read during a refresh cannot keep the provider's old change request repository: "acme/web", provider: "gitlab", }), + project({ + id: "p2", + title: "api", + workspaceRoot: "/b", + repository: "acme/api", + provider: "gitlab", + }), ], providers: [ fakePullRequestProvider(SourceControlProviderKind.make("gitlab"), { @@ -2349,6 +2370,15 @@ it.effect("a read during a refresh cannot keep the provider's old change request hostTitle = "after refresh"; yield* refreshWithReadInBetween(service.invalidate({ reference })); assert.strictEqual((yield* service.detail(reference)).title, "after refresh"); + + const second = { projectId: "p2" as ProjectId, repository: "acme/api", number: 2 }; + assert.strictEqual((yield* service.detail(second)).title, "after refresh"); + hostTitle = "after batch"; + yield* refreshWithReadInBetween( + service.refreshAfterTurns([reference.projectId, second.projectId]), + ); + assert.strictEqual((yield* service.detail(reference)).title, "after batch"); + assert.strictEqual((yield* service.detail(second)).title, "after batch"); }), ); @@ -3090,9 +3120,15 @@ it.effect( expect(reads).toBe(0); yield* TestClock.adjust("16 seconds"); yield* service.detail(reference); + expect(yield* service.checks(reference)).toMatchObject({ + state: detail.state, + observedAt: 1_000, + }); + expect(reads).toBe(0); + yield* TestClock.adjust("30 seconds"); expect(yield* service.checks(reference)).toMatchObject({ state: "closed", - observedAt: 17_000, + observedAt: 47_000, }); expect(reads).toBe(1); yield* service.detail(reference); @@ -3101,6 +3137,56 @@ it.effect( }), ); +it.effect("shares an in-flight GitHub detail with concurrent checks readers", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let detailReads = 0; + let checkReads = 0; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakePullRequestProvider(SourceControlProviderKind.make("github"), { + getChangeRequest: () => + Effect.gen(function* () { + detailReads += 1; + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + return { ...hostedChangeRequest("Description"), headSha: "abc123" }; + }), + getChangeRequestChecks: () => + Effect.sync(() => { + checkReads += 1; + return { state: "open" as const, checks: [] }; + }), + }), + ], + }); + const detailFiber = yield* service + .detail(reference) + .pipe(Effect.forkChild({ startImmediately: true })); + yield* Deferred.await(started); + const readers = yield* Effect.all( + Array.from({ length: 8 }, () => service.checks(reference)), + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild({ startImmediately: true })); + yield* Deferred.succeed(release, undefined); + const detail = yield* Fiber.join(detailFiber); + const checks = yield* Fiber.join(readers); + expect(checks).toEqual( + Array.from({ length: 8 }, () => ({ + state: detail.state, + headSha: detail.headSha, + checks: detail.checks, + observedAt: detail.observedAt, + })), + ); + expect(detailReads).toBe(1); + expect(checkReads).toBe(0); + }), +); + it.effect("preserves check observation time on cache hits and advances it on refresh", () => Effect.gen(function* () { let reads = 0; @@ -3123,8 +3209,8 @@ it.effect("preserves check observation time on cache hits and advances it on ref yield* TestClock.adjust("1 second"); expect(yield* service.checks(reference)).toEqual(first); expect(reads).toBe(1); - yield* TestClock.adjust("15 seconds"); - expect(yield* service.checks(reference)).toMatchObject({ observedAt: 17_000 }); + yield* TestClock.adjust("45 seconds"); + expect(yield* service.checks(reference)).toMatchObject({ observedAt: 47_000 }); expect(reads).toBe(2); }), ); @@ -4689,7 +4775,7 @@ it.effect("coalesces completed check failures and separates credentials and atte expect((yield* service.checkFailures(input)).log).toMatchObject({ text: "second" }); expect(calls).toBe(2); attempt = 2; - yield* TestClock.adjust("16 seconds"); + yield* TestClock.adjust("46 seconds"); yield* service.checkFailures({ ...ref, handle: "job:123:2" }); expect(calls).toBe(3); const readsForTwoCalls = Effect.fn(function* (handle: string) { @@ -4700,7 +4786,7 @@ it.effect("coalesces completed check failures and separates credentials and atte }); attempt = 3; log = { _tag: "unavailable", reason: "still-running" }; - yield* TestClock.adjust("16 seconds"); + yield* TestClock.adjust("46 seconds"); expect(yield* readsForTwoCalls("job:123:3")).toBe(2); log = { _tag: "unavailable", reason: "no-access" }; expect(yield* readsForTwoCalls("job:123:3")).toBe(2); @@ -4708,11 +4794,11 @@ it.effect("coalesces completed check failures and separates credentials and atte expect(yield* readsForTwoCalls("job:123:3")).toBe(1); attempt = 4; log = { _tag: "unavailable", reason: "expired" }; - yield* TestClock.adjust("16 seconds"); + yield* TestClock.adjust("46 seconds"); expect(yield* readsForTwoCalls("job:123:4")).toBe(1); attempt = 5; log = { _tag: "unavailable", reason: "not-actions" }; - yield* TestClock.adjust("16 seconds"); + yield* TestClock.adjust("46 seconds"); expect(yield* readsForTwoCalls("job:123:5")).toBe(1); expect( (yield* Effect.flip(service.checkFailures({ ...input, expectedAccountId: "other" })))._tag, @@ -4720,9 +4806,12 @@ it.effect("coalesces completed check failures and separates credentials and atte const settled = calls; refuse = true; attempt = 6; - yield* TestClock.adjust("16 seconds"); + yield* TestClock.adjust("46 seconds"); yield* Effect.flip(service.checkFailures({ ...ref, handle: "job:123:6" })); - yield* Effect.flip(service.checkFailures(input)); + expect(yield* service.checkFailures(input)).toMatchObject({ + attempt: 1, + log: { _tag: "excerpt", text: "second" }, + }); expect(calls).toBe(settled + 1); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 13298490cf709..a268f40a92113 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -127,7 +127,7 @@ const DETAIL_CACHE_TTL = Duration.seconds(60); const MERGED_DETAIL_CACHE_TTL = Duration.minutes(10); const detailTimeToLive = (state: PullRequestState | undefined) => state === "merged" ? MERGED_DETAIL_CACHE_TTL : DETAIL_CACHE_TTL; -const CHECKS_CACHE_TTL = Duration.seconds(15); +const CHECKS_CACHE_TTL = Duration.seconds(45); const REPORTED_STATE_DEDUPE_MS = 5 * 60 * 1_000; const DIFF_CACHE_TTL = Duration.seconds(60); const COMMIT_DIFF_CACHE_TTL = Duration.minutes(10); @@ -227,6 +227,7 @@ export class PullRequestService extends Context.Service< ) => Effect.Effect; readonly subscribeRefreshes: Stream.Stream; readonly refreshAfterTurn: (projectId: ProjectId) => Effect.Effect; + readonly refreshAfterTurns: (projectIds: ReadonlyArray) => Effect.Effect; /** Project and remote lookups inside `effect` read once per filter, e.g. across one sync sweep. */ readonly withSharedProjectReads: ( effect: Effect.Effect, @@ -479,34 +480,37 @@ function withRateLimitBackoff( effect: Effect.Effect, allowPaused: boolean, ) => - limits.check(key, allowPaused ? { allowPaused: true } : undefined).pipe( - Effect.mapError( - (error) => - new PullRequestProviderError({ - provider: api.kind, - operation, - reason: "rate-limited", - detail: error.detail, - retryAt: error.retryAt, - cause: error, - }), - ), - Effect.flatMap((lease) => - effect.pipe( - Effect.provideService(SourceControlRateLimit.Interactive, allowPaused), - Effect.tap(() => limits.recordSuccess({ ...key, lease })), - Effect.tapError((error) => - error.reason === "rate-limited" - ? limits.recordRateLimit({ - ...key, - lease, - ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), - }) - : Effect.void, + // GitHub owns credential/resource cooldowns in its shared REST/GraphQL transport. + api.kind === "github" + ? effect.pipe(Effect.provideService(SourceControlRateLimit.Interactive, allowPaused)) + : limits.check(key, allowPaused ? { allowPaused: true } : undefined).pipe( + Effect.mapError( + (error) => + new PullRequestProviderError({ + provider: api.kind, + operation, + reason: "rate-limited", + detail: error.detail, + retryAt: error.retryAt, + cause: error, + }), ), - ), - ), - ); + Effect.flatMap((lease) => + effect.pipe( + Effect.provideService(SourceControlRateLimit.Interactive, allowPaused), + Effect.tap(() => limits.recordSuccess({ ...key, lease })), + Effect.tapError((error) => + error.reason === "rate-limited" + ? limits.recordRateLimit({ + ...key, + lease, + ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), + }) + : Effect.void, + ), + ), + ), + ); const wrap = , A>( operation: string, @@ -1806,7 +1810,7 @@ export const make = Effect.gen(function* () { baseBranch: changeRequest.baseBranch, createdAt: changeRequest.createdAt, updatedAt: changeRequest.updatedAt, - observedAt, + observedAt: changeRequest.observedAt ?? observedAt, mergedAt: changeRequest.mergedAt, closedAt: changeRequest.closedAt, reviewers: changeRequest.reviewers, @@ -3086,7 +3090,10 @@ export const make = Effect.gen(function* () { .pipe( Effect.mapError(toPullRequestError("checks")), observeRead, - Effect.map(({ value, observedAt }) => ({ ...value, observedAt })), + Effect.map(({ value, observedAt }) => ({ + ...value, + observedAt: value.observedAt ?? observedAt, + })), ), ), ); @@ -3110,10 +3117,6 @@ export const make = Effect.gen(function* () { }, ); - const checks: PullRequestService["Service"]["checks"] = credentialCached((input) => - Cache.get(checksCache, refCacheKey(input)), - ); - const checkFailureCache = yield* Cache.makeWith( Effect.fn("PullRequestService.readCheckFailure")(function* (key: string) { const [refKey, , handle] = decodeCheckFailureKey(key); @@ -3279,9 +3282,31 @@ export const make = Effect.gen(function* () { { capacity: DETAIL_CACHE_CAPACITY, timeToLive: (exit) => - Exit.isSuccess(exit) ? detailTimeToLive(exit.value.state) : Duration.zero, + Exit.isSuccess(exit) + ? Duration.millis( + Math.max( + 0, + (exit.value.observedAt ?? clock.currentTimeMillisUnsafe()) + + Duration.toMillis(detailTimeToLive(exit.value.state)) - + clock.currentTimeMillisUnsafe(), + ), + ) + : Duration.zero, }, ); + const checks: PullRequestService["Service"]["checks"] = credentialCached( + Effect.fn("PullRequestService.checks")(function* (input) { + const key = refCacheKey(input); + const cached = yield* Cache.getOption(checksCache, key); + if (Option.isSome(cached)) return cached.value; + // A detail read already in flight seeds its checks before completing. + yield* Cache.getOption(detailCache, key).pipe( + Effect.orElseSucceed(() => Option.none()), + ); + return yield* Cache.get(checksCache, key); + }), + ); + const summaryFromDetail = ( detail: PullRequestDetail, previous: PullRequestSummary | undefined, @@ -3662,28 +3687,37 @@ export const make = Effect.gen(function* () { } }); - const refreshAfterTurn: PullRequestService["Service"]["refreshAfterTurn"] = Effect.fn( - "PullRequestService.refreshAfterTurn", - )(function* (projectId) { + const refreshAfterTurns: PullRequestService["Service"]["refreshAfterTurns"] = Effect.fn( + "PullRequestService.refreshAfterTurns", + )(function* (projectIds) { + const projects = new Set(projectIds); + if (projects.size === 0) return; // Everything the provider holds, not just the project's repository: a turn can touch a // change request reached through another repository, and no epoch may move until those // reads are gone, or a reader in between keeps the old one under the new epoch. yield* forgetProviderReads(); listingsEpoch = ++epochCounter; - projectEpochs.delete(projectId); - if (projectEpochs.size >= REF_EPOCH_CAPACITY) { - const oldest = projectEpochs.keys().next().value; - if (oldest !== undefined) { - projectEpochFloor = projectEpochs.get(oldest)!; - projectEpochs.delete(oldest); + for (const projectId of projects) { + projectEpochs.delete(projectId); + if (projectEpochs.size >= REF_EPOCH_CAPACITY) { + const oldest = projectEpochs.keys().next().value; + if (oldest !== undefined) { + projectEpochFloor = projectEpochs.get(oldest)!; + projectEpochs.delete(oldest); + } } + projectEpochs.set(projectId, listingsEpoch); } - projectEpochs.set(projectId, listingsEpoch); - const epoch = listingsEpoch; - yield* readCache.invalidate(`project:${projectId}`); - yield* SubscriptionRef.set(pullRequestRefreshes, epoch); + yield* Effect.forEach(projects, (projectId) => readCache.invalidate(`project:${projectId}`), { + discard: true, + }); + yield* SubscriptionRef.set(pullRequestRefreshes, ++epochCounter); }); + const refreshAfterTurn: PullRequestService["Service"]["refreshAfterTurn"] = Effect.fn( + "PullRequestService.refreshAfterTurn", + )((projectId) => refreshAfterTurns([projectId])); + const invalidatedByMutation = ( method: (input: I) => Effect.Effect, @@ -3860,6 +3894,7 @@ export const make = Effect.gen(function* () { Stream.filter((revision) => revision > 0), ), refreshAfterTurn, + refreshAfterTurns, detail: credentialCached(detail), checks, checkFailures, diff --git a/apps/server/src/pullRequest/pullRequestReports.test.ts b/apps/server/src/pullRequest/pullRequestReports.test.ts index 1a0e2cfc304aa..a5b2907da3ad9 100644 --- a/apps/server/src/pullRequest/pullRequestReports.test.ts +++ b/apps/server/src/pullRequest/pullRequestReports.test.ts @@ -429,7 +429,7 @@ describe("feedbackOf", () => { }); describe("rateLimitedOf", () => { - const paused = (retryAt?: number) => + const paused = (retryAt?: number, quota: Partial = {}) => new PullRequestOperationError({ operation: "summary", detail: "paused", @@ -439,15 +439,34 @@ describe("rateLimitedOf", () => { reason: "rate-limited", detail: "paused", ...(retryAt === undefined ? {} : { retryAt }), + ...quota, }), }); - it("names the time and tells the agent its CLI shares the limit", () => { - assert.deepEqual(rateLimitedOf(paused(Date.UTC(2026, 9, 30, 20, 5)), GITHUB, 0), { - _tag: "RateLimited", - until: "2026-10-30T20:05:00.000Z", + it.each([ + { + name: "a host throttle with no quota metadata", + quota: {}, message: "GitHub is rate limited for this account until 20:05 UTC. Your gh shares that limit; stop GitHub reads until then.", + }, + { + name: "a protected REST reserve", + quota: { pauseReason: "reserve-protected", resource: "core", remaining: 282, limit: 5000 }, + message: + "T3 is protecting this account's GitHub REST reserve until 20:05 UTC (282 of 5000 remaining). Background refreshes are paused; use T3's cached reads or watch instead of polling with gh.", + }, + { + name: "an exhausted GraphQL quota", + quota: { pauseReason: "quota-exhausted", resource: "graphql", remaining: 0, limit: 5000 }, + message: + "This account's GitHub GraphQL quota is exhausted until 20:05 UTC. Your gh shares that quota; pause new GraphQL requests until then. T3 can still serve cached data.", + }, + ] as const)("reports $name accurately", ({ quota, message }) => { + assert.deepEqual(rateLimitedOf(paused(Date.UTC(2026, 9, 30, 20, 5), quota), GITHUB, 0), { + _tag: "RateLimited", + until: "2026-10-30T20:05:00.000Z", + message, }); }); diff --git a/apps/server/src/pullRequest/pullRequestReports.ts b/apps/server/src/pullRequest/pullRequestReports.ts index a63c0d2cebf18..d854377bda59d 100644 --- a/apps/server/src/pullRequest/pullRequestReports.ts +++ b/apps/server/src/pullRequest/pullRequestReports.ts @@ -281,6 +281,32 @@ export function rateLimitedOf( if (cause === null) return null; const until = isoOf(cause.retryAt ?? now + RATE_LIMIT_FALLBACK_MS); const time = `${until.slice(11, 16)} UTC`; + const resource = + cause.resource === "core" + ? "REST" + : cause.resource === "graphql" + ? "GraphQL" + : cause.resource === "search" + ? "search" + : "API"; + if (cause.pauseReason === "reserve-protected") { + const remaining = + cause.remaining !== undefined && cause.limit !== undefined + ? ` (${cause.remaining} of ${cause.limit} remaining)` + : ""; + return { + _tag: "RateLimited", + until, + message: `T3 is protecting this account's ${forge.label} ${resource} reserve until ${time}${remaining}. Background refreshes are paused; use T3's cached reads or watch instead of polling with ${forge.cli}.`, + }; + } + if (cause.pauseReason === "quota-exhausted") { + return { + _tag: "RateLimited", + until, + message: `This account's ${forge.label} ${resource} quota is exhausted until ${time}. Your ${forge.cli} shares that quota; pause new ${resource} requests until then. T3 can still serve cached data.`, + }; + } return { _tag: "RateLimited", until, diff --git a/apps/server/src/sourceControl/agentGitHubCli.ts b/apps/server/src/sourceControl/agentGitHubCli.ts new file mode 100644 index 0000000000000..7b02ccdac5ca5 --- /dev/null +++ b/apps/server/src/sourceControl/agentGitHubCli.ts @@ -0,0 +1,52 @@ +// @effect-diagnostics nodeBuiltinImport:off - CLI process boundary forwards the real gh's stdio and lifetime. +import * as NodeChildProcess from "node:child_process"; +import { resolveSpawnCommand } from "@t3tools/shared/shell"; +import * as Effect from "effect/Effect"; + +import { decideGitHubArguments } from "./agentGitHubReads.ts"; + +/** Refused reads use the same tool redirect as the native hook; all other argv reach real gh intact. */ +export async function runAgentGitHubCli( + realCommand: string, + args: ReadonlyArray, +): Promise { + const decision = decideGitHubArguments(args, process.env); + if (!decision.allow) { + process.stderr.write(`${decision.reason}\n`); + process.exitCode = 1; + return; + } + const resolved = await Effect.runPromise( + resolveSpawnCommand(realCommand, args, { env: process.env }), + ); + await new Promise((resolve) => { + const child = NodeChildProcess.spawn(resolved.command, resolved.args, { + env: process.env, + stdio: "inherit", + shell: resolved.shell, + windowsHide: true, + }); + const forwardInterrupt = () => child.kill("SIGINT"); + const forwardTerminate = () => child.kill("SIGTERM"); + process.on("SIGINT", forwardInterrupt); + process.on("SIGTERM", forwardTerminate); + const cleanup = () => { + process.off("SIGINT", forwardInterrupt); + process.off("SIGTERM", forwardTerminate); + }; + child.once("error", () => { + cleanup(); + process.stderr.write( + "Could not start the installed GitHub CLI. Restart the provider session after installing or moving gh.\n", + ); + process.exitCode = 127; + resolve(); + }); + child.once("exit", (code, signal) => { + cleanup(); + if (signal !== null) process.kill(process.pid, signal); + else process.exitCode = code ?? 1; + resolve(); + }); + }); +} diff --git a/apps/server/src/sourceControl/agentGitHubEnvironment.test.ts b/apps/server/src/sourceControl/agentGitHubEnvironment.test.ts new file mode 100644 index 0000000000000..1ee7685319fa0 --- /dev/null +++ b/apps/server/src/sourceControl/agentGitHubEnvironment.test.ts @@ -0,0 +1,200 @@ +// @effect-diagnostics nodeBuiltinImport:off - real subprocess regression for the agent-facing gh launcher. +import * as NodeChildProcess from "node:child_process"; +import * as NodeURL from "node:url"; +import * as NodeUtil from "node:util"; +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { describe, expect, it } from "@effect/vitest"; +import * as ProviderHost from "@t3tools/provider-core/server/ProviderHost"; +import { mergeProviderInstanceEnvironment } from "@t3tools/provider-core/server/instanceEnvironment"; +import * as TestProviderHost from "@t3tools/provider-testing/TestProviderHost"; +import * as HostProcess from "@t3tools/shared/HostProcess"; +import { CommandResolutionCache } from "@t3tools/shared/shell"; +import * as Effect from "effect/Effect"; +import * as Crypto from "effect/Crypto"; +import * as FileSystem from "effect/FileSystem"; +import * as Layer from "effect/Layer"; +import * as Path from "effect/Path"; + +import { prepareAgentGitHubEnvironment } from "./agentGitHubEnvironment.ts"; + +const exec = NodeUtil.promisify(NodeChildProcess.execFile); +const invocation = { + command: process.execPath, + entrypoint: NodeURL.fileURLToPath(new URL("../bin.ts", import.meta.url)), +}; +const quote = (value: string) => `'${value.replaceAll("'", `'"'"'`)}'`; + +describe("managed GitHub CLI environment", () => { + it.effect.skipIf(HostProcess.Platform.defaultValue() === "win32")( + "intercepts local provider reads before gh, preserving writes, bypass, argv and instance overrides", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const platform = yield* HostProcess.Platform; + const host = yield* ProviderHost.ProviderHost; + const stateDir = yield* fs + .makeTempDirectoryScoped({ prefix: "t3-gh-" }) + .pipe(Effect.flatMap(fs.realPath)); + const realDir = path.join(stateDir, "real gh ' $HOME `literal`"); + yield* fs.makeDirectory(realDir); + const profileDir = path.join(stateDir, ".nvm", "bin"); + yield* fs.makeDirectory(profileDir, { recursive: true }); + yield* fs.writeFileString( + path.join(profileDir, "profile-tool"), + "#!/bin/sh\nprintf 'profile tool works\\n'\n", + ); + yield* fs.chmod(path.join(profileDir, "profile-tool"), 0o755); + const providerPath = `${realDir}:${profileDir}`; + const calls = path.join(stateDir, "calls"); + const entry = path.join(realDir, "real.mjs"); + yield* fs.writeFileString( + entry, + `import { appendFileSync } from 'node:fs';\nappendFileSync(process.env.CALLS, 'called\\n');\nconsole.log(JSON.stringify({ args: process.argv.slice(2), cwd: process.cwd(), custom: process.env.CUSTOM }));\nprocess.exitCode = 23;\n`, + ); + yield* fs.writeFileString( + path.join(realDir, "gh"), + `#!/bin/sh\nexec ${quote(process.execPath)} ${quote(entry)} "$@"\n`, + ); + yield* fs.chmod(path.join(realDir, "gh"), 0o755); + const services = yield* Effect.context(); + const prepared = yield* mergeProviderInstanceEnvironment( + [{ name: "PATH", value: providerPath, sensitive: false }], + { PATH: "/not-installed", CALLS: calls, CUSTOM: "kept" }, + ).pipe( + Effect.provideService(ProviderHost.ProviderHost, { + ...host, + prepareAgentEnvironment: (environment) => + prepareAgentGitHubEnvironment({ + stateDir, + invocation, + platform, + environment, + enabled: true, + }).pipe(Effect.provide(services), Effect.orDie), + }), + ); + expect(prepared.T3_GH_SHIM_DIR).toBeDefined(); + expect(prepared.PATH?.split(":")[0]).toBe(prepared.T3_GH_SHIM_DIR); + expect(prepared.PATH).toBe(`${prepared.T3_GH_SHIM_DIR}:${providerPath}`); + expect( + (yield* Effect.promise(() => exec("profile-tool", [], { env: prepared }))).stdout, + ).toBe("profile tool works\n"); + const run = (args: ReadonlyArray, env = prepared) => + exec("gh", args, { env, cwd: stateDir }); + yield* Effect.promise(() => + expect(run(["pr", "checks", "12"])).rejects.toMatchObject({ + code: 1, + stdout: "", + stderr: expect.stringContaining("pull_request_status"), + }), + ); + yield* Effect.promise(() => + expect(run(["api", "repos/acme/app/pulls/12/reviews"])).rejects.toMatchObject({ + code: 1, + stdout: "", + stderr: expect.stringContaining("pull_request_feedback"), + }), + ); + expect(yield* fs.exists(calls)).toBe(false); + + const passthrough = [ + { + args: ["pr", "comment", "12", "--body", "a 'quote' \"and\" $HOME; $(literal)", ""], + env: prepared, + }, + { args: ["pr", "checks", "12"], env: { ...prepared, T3_GH_DIRECT: "1" } }, + { args: ["pr", "view", "12", "--json", "files"], env: prepared }, + { + args: ["api", "repos/acme/app/pulls/12/reviews", "-H", "Accept: custom"], + env: prepared, + }, + { args: ["pr", "checks", "12"], env: { ...prepared, GH_REPO: "elsewhere/repo" } }, + ]; + for (const { args, env } of passthrough) { + yield* Effect.promise(() => + expect(run(args, env)).rejects.toMatchObject({ + code: 23, + stderr: "", + stdout: JSON.stringify({ args, cwd: stateDir, custom: "kept" }) + "\n", + }), + ); + } + expect((yield* fs.readFileString(calls)).trim().split("\n")).toHaveLength( + passthrough.length, + ); + const again = yield* prepareAgentGitHubEnvironment({ + stateDir, + invocation, + platform, + environment: prepared, + enabled: true, + }); + expect(again).toEqual(prepared); + const disabled = yield* prepareAgentGitHubEnvironment({ + stateDir, + invocation, + platform, + environment: prepared, + enabled: false, + }); + expect(disabled.PATH).toBe(providerPath); + expect(disabled.T3_GH_SHIM_DIR).toBeUndefined(); + }).pipe( + Effect.scoped, + Effect.provide(TestProviderHost.layer().pipe(Layer.provideMerge(NodeServices.layer))), + ), + ); + + it.effect("does not manufacture an installed gh when the provider's PATH has none", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const platform = yield* HostProcess.Platform; + const stateDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-gh-missing-" }); + const environment = { PATH: stateDir, CUSTOM: "kept" }; + expect( + yield* prepareAgentGitHubEnvironment({ + stateDir, + invocation, + platform, + environment, + enabled: true, + }), + ).toEqual(environment); + expect(yield* fs.exists(path.join(stateDir, "github"))).toBe(false); + }).pipe(Effect.scoped, Effect.provide(NodeServices.layer)), + ); + + it.effect.each([".EXE", ".CMD"])( + "keeps Windows Path casing and resolves the original gh%s before adding gh.cmd", + (extension) => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const stateDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-gh-windows-" }); + const realDir = path.join(stateDir, "real"); + yield* fs.makeDirectory(realDir); + const realCommand = path.join(realDir, `gh${extension}`); + yield* fs.writeFileString(realCommand, "installed gh fixture"); + const prepared = yield* prepareAgentGitHubEnvironment({ + stateDir, + invocation: { command: "C:\\T3 Code\\t3.exe", entrypoint: undefined }, + platform: "win32", + environment: { Path: realDir, PATHEXT: extension + ";.BAT;.CMD", CUSTOM: "kept" }, + enabled: true, + }); + expect(prepared.PATH).toBeUndefined(); + expect(prepared.Path).toBe(`${prepared.T3_GH_SHIM_DIR};${realDir}`); + const source = yield* fs.readFileString(path.join(prepared.T3_GH_SHIM_DIR!, "gh.cmd")); + expect(source).toContain(`"C:\\T3 Code\\t3.exe" "__github-cli" "${realCommand}" %*`); + expect(source).toContain("DisableDelayedExpansion"); + expect(prepared.CUSTOM).toBe("kept"); + }).pipe( + Effect.scoped, + Effect.provideService(HostProcess.Platform, "win32"), + Effect.provideService(CommandResolutionCache, new Map()), + Effect.provide(NodeServices.layer), + ), + ); +}); diff --git a/apps/server/src/sourceControl/agentGitHubEnvironment.ts b/apps/server/src/sourceControl/agentGitHubEnvironment.ts new file mode 100644 index 0000000000000..0f559ac21d15f --- /dev/null +++ b/apps/server/src/sourceControl/agentGitHubEnvironment.ts @@ -0,0 +1,72 @@ +import type { SelfInvocation } from "@t3tools/shared/nodeRuntime"; +import { selfInvocationArgs } from "@t3tools/shared/nodeRuntime"; +import { resolveCommandPath } from "@t3tools/shared/shell"; +import * as Crypto from "effect/Crypto"; +import * as Effect from "effect/Effect"; +import * as FileSystem from "effect/FileSystem"; +import * as Hex from "effect/encoding/Hex"; +import * as Path from "effect/Path"; + +const quoteShell = (value: string) => `'${value.replaceAll("'", `'"'"'`)}'`; +const quoteBatch = (value: string) => `"${value.replaceAll("%", "%%")}"`; + +/** + * Normal PATH-resolved gh calls in local provider processes share Claude's managed-read gate. + * This is a request-saving launcher, not a sandbox: absolute binaries, replaced PATHs and remote + * provider servers stay outside it. It never modifies the environment of T3's own GitHub clients. + */ +export const prepareAgentGitHubEnvironment = Effect.fn( + "ProviderHost.prepareAgentGitHubEnvironment", +)(function* (input: { + readonly stateDir: string; + readonly invocation: SelfInvocation; + readonly platform: NodeJS.Platform; + readonly environment: NodeJS.ProcessEnv; + readonly enabled: boolean; +}) { + const windows = input.platform === "win32"; + const delimiter = windows ? ";" : ":"; + const pathKey = windows + ? (Object.keys(input.environment).find((key) => key.toUpperCase() === "PATH") ?? "PATH") + : "PATH"; + const inheritedPath = input.environment[pathKey]; + const previous = input.environment.T3_GH_SHIM_DIR; + const searchPath = + previous === undefined + ? inheritedPath + : inheritedPath + ?.split(delimiter) + .filter((entry) => + windows ? entry.toLowerCase() !== previous.toLowerCase() : entry !== previous, + ) + .join(delimiter); + const environment = { ...input.environment }; + if (searchPath !== undefined) environment[pathKey] = searchPath; + delete environment.T3_GH_SHIM_DIR; + if (!input.enabled) return environment; + + // Resolve before adding our launcher, and preserve the provider's own PATH overrides. + const realCommand = yield* resolveCommandPath("gh", { env: environment }); + if (realCommand === null) return environment; + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const crypto = yield* Crypto.Crypto; + const args = selfInvocationArgs(input.invocation, ["__github-cli", realCommand]); + const source = windows + ? `@echo off\r\nsetlocal EnableExtensions DisableDelayedExpansion\r\nset "ELECTRON_RUN_AS_NODE=1"\r\n${[input.invocation.command, ...args].map(quoteBatch).join(" ")} %*\r\n` + : `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${[input.invocation.command, ...args].map(quoteShell).join(" ")} "$@"\n`; + const digest = Hex.encode(yield* crypto.digest("SHA-256", new TextEncoder().encode(source))); + const directory = path.join(input.stateDir, "github", "bin", digest); + const launcher = path.join(directory, windows ? "gh.cmd" : "gh"); + // Immutable launchers allow concurrent provider instances with different gh installations. + if (!(yield* fs.exists(launcher))) { + yield* fs.makeDirectory(directory, { recursive: true }); + const temporary = yield* fs.makeTempFile({ directory, prefix: "gh-" }); + yield* fs.writeFileString(temporary, source); + if (!windows) yield* fs.chmod(temporary, 0o755); + yield* fs.rename(temporary, launcher); + } + environment[pathKey] = `${directory}${searchPath === undefined ? "" : delimiter + searchPath}`; + environment.T3_GH_SHIM_DIR = directory; + return environment; +}); diff --git a/apps/server/src/sourceControl/agentGitHubReads.test.ts b/apps/server/src/sourceControl/agentGitHubReads.test.ts index a22269bd48e6e..97dbd31822bee 100644 --- a/apps/server/src/sourceControl/agentGitHubReads.test.ts +++ b/apps/server/src/sourceControl/agentGitHubReads.test.ts @@ -9,6 +9,7 @@ import * as Schema from "effect/Schema"; import { PullRequestsToolkit } from "../mcp/toolkits/pullRequests/tools.ts"; import { + decideGitHubArguments, decideGitHubCommand, POLL_DENIED_REASON, PULL_REQUEST_READ_TOOLS, @@ -18,6 +19,41 @@ import { const redirect = (tool: keyof typeof PULL_REQUEST_READ_TOOLS, args: string, command: string) => `T3 already reads this. Call ${PULL_REQUEST_READ_TOOLS[tool]} with ${args} (load it with ToolSearch if it is deferred). If it can't answer what you need, rerun as \`T3_GH_DIRECT=1 ${command}\`.`; +describe("decideGitHubArguments", () => { + it("classifies actual argv without interpreting quoted text as shell commands", () => { + assert.deepEqual( + decideGitHubArguments(["pr", "view", "feature; gh pr checks 4", "--comments"]), + { allow: true }, + ); + assert.deepEqual( + decideGitHubArguments(["pr", "checks", "12"]), + decideGitHubCommand("gh pr checks 12"), + ); + assert.deepEqual( + decideGitHubArguments(["api", "repos/acme/app/pulls/12/reviews", "--jq", ".[] | .body"]), + decideGitHubCommand("gh api repos/acme/app/pulls/12/reviews --jq '.[] | .body'"), + ); + }); + + it("preserves environment-selected repositories and the explicit read bypass", () => { + for (const environment of [ + { T3_GH_DIRECT: "1" }, + { GH_HOST: "enterprise.example" }, + { GH_REPO: "other/repo" }, + ]) { + assert.deepEqual(decideGitHubArguments(["pr", "checks", "12"], environment), { allow: true }); + } + assert.deepEqual( + decideGitHubArguments(["pr", "checks", "12", "--watch"], { T3_GH_DIRECT: "1" }), + { allow: false, reason: POLL_DENIED_REASON }, + ); + assert.deepEqual(decideGitHubCommand("GH_HOST=enterprise.example gh pr checks 12"), { + allow: true, + }); + assert.deepEqual(decideGitHubCommand("GH_REPO=other/repo gh pr checks 12"), { allow: true }); + }); +}); + describe("decideGitHubCommand", () => { it.each([ { diff --git a/apps/server/src/sourceControl/agentGitHubReads.ts b/apps/server/src/sourceControl/agentGitHubReads.ts index d1774bc3487b8..8f23dab4ad665 100644 --- a/apps/server/src/sourceControl/agentGitHubReads.ts +++ b/apps/server/src/sourceControl/agentGitHubReads.ts @@ -1,7 +1,9 @@ import type { + ForgeCommand, ForgeCommandTarget, ForgeReadIntent, } from "@t3tools/source-control-core/shared/definition"; +import { definition as gitHubDefinition } from "@t3tools/source-control-github/shared/definition"; import { classifyAgentShellCommands } from "./agentShellCommands.ts"; @@ -34,7 +36,7 @@ export const POLL_DENIED_REASON = "Do not poll GitHub from a shell. A parent calls watch_pull_request and ends its turn; a subagent reads pull_request_status once and reports."; /** - * Whether a Claude Bash command may run. Reads a T3 tool answers are refused with the exact call + * Whether an agent's shell command may run. Reads a T3 tool answers are refused with the exact call * to make instead, polls are refused outright, and everything else runs as typed. */ export function decideGitHubCommand(command: string): GitHubCommandDecision { @@ -44,14 +46,35 @@ export function decideGitHubCommand(command: string): GitHubCommandDecision { } for (const { command: forgeCommand, bypass, bypassCommand } of commands) { if (forgeCommand.kind !== "read" || bypass) continue; - const tool = PULL_REQUEST_READ_TOOLS[forgeCommand.intent]; - const args = JSON.stringify(pullRequestReadArguments(forgeCommand.target)); - const bypassHint = `rerun as \`${bypassCommand.trim()}\`.`; - const reason = - forgeCommand.intent === "failures" - ? `T3 reads why a PR's checks failed. Call ${tool} with ${args} (load it with ToolSearch if it is deferred). It answers the failing checks of this thread's PR or the PR you name.${forgeCommand.target.repository === undefined ? "" : ` Your command reads ${forgeCommand.target.repository}.`} If this run or job belongs to another PR, ${bypassHint}` - : `T3 already reads this. Call ${tool} with ${args} (load it with ToolSearch if it is deferred). If it can't answer what you need, ${bypassHint}`; - return { allow: false, reason }; + return denyRead(forgeCommand, bypassCommand); } return { allow: true }; } + +function denyRead( + command: Extract, + bypassCommand: string, +): GitHubCommandDecision { + const tool = PULL_REQUEST_READ_TOOLS[command.intent]; + const args = JSON.stringify(pullRequestReadArguments(command.target)); + const bypassHint = `rerun as \`${bypassCommand.trim()}\`.`; + const reason = + command.intent === "failures" + ? `T3 reads why a PR's checks failed. Call ${tool} with ${args} (load it with ToolSearch if it is deferred). It answers the failing checks of this thread's PR or the PR you name.${command.target.repository === undefined ? "" : ` Your command reads ${command.target.repository}.`} If this run or job belongs to another PR, ${bypassHint}` + : `T3 already reads this. Call ${tool} with ${args} (load it with ToolSearch if it is deferred). If it can't answer what you need, ${bypassHint}`; + return { allow: false, reason }; +} + +/** The PATH launcher receives already-parsed argv; never reinterpret an argument as shell code. */ +export function decideGitHubArguments( + args: ReadonlyArray, + environment: NodeJS.ProcessEnv = {}, +): GitHubCommandDecision { + if (environment.GH_HOST || environment.GH_REPO) return { allow: true }; + const command = gitHubDefinition.classifyCommand?.(["gh", ...args]); + if (command?.kind === "poll") return { allow: false, reason: POLL_DENIED_REASON }; + if (command?.kind !== "read" || environment.T3_GH_DIRECT === "1") return { allow: true }; + const quote = (arg: string) => + /^[\w./:=,@+-]+$/u.test(arg) ? arg : `'${arg.replaceAll("'", `'"'"'`)}'`; + return denyRead(command, `T3_GH_DIRECT=1 gh ${args.map(quote).join(" ")}`); +} diff --git a/apps/server/src/sourceControl/agentShellCommands.test.ts b/apps/server/src/sourceControl/agentShellCommands.test.ts index ed20d71a9e84b..8ab857583f6a6 100644 --- a/apps/server/src/sourceControl/agentShellCommands.test.ts +++ b/apps/server/src/sourceControl/agentShellCommands.test.ts @@ -97,6 +97,7 @@ describe("agentShellCommands", () => { ), [ { + executable: "gh", words: ["gh", "api", "repos/acme/app/pulls/$n/comments", "--jq", ".[] | .body"], assignments: new Map(), repeats: false, diff --git a/apps/server/src/sourceControl/agentShellCommands.ts b/apps/server/src/sourceControl/agentShellCommands.ts index ff2e45d4744bb..3f9febdd21e56 100644 --- a/apps/server/src/sourceControl/agentShellCommands.ts +++ b/apps/server/src/sourceControl/agentShellCommands.ts @@ -1,7 +1,6 @@ import type { SourceControlProviderKind } from "@t3tools/contracts"; import type { ForgeCommand } from "@t3tools/source-control-core/shared/definition"; - -import { sourceControlDefinitions } from "./sourceControlDefinitions.ts"; +import { builtInSourceControlDefinitions as sourceControlDefinitions } from "@t3tools/source-control-definitions/registry"; // Where a shell starts a command, past `NAME=value` assignments and wrappers like `timeout 60`. // A command anywhere else, as in `echo "gh pr create"`, is text, not a command that runs. @@ -106,6 +105,8 @@ function shellWords(segment: string): Array { } export interface AgentShellCommand { + /** The executable as written, before basename normalization. */ + readonly executable: string; /** The program and its arguments, after assignments and wrappers such as `timeout 60`. */ readonly words: ReadonlyArray; /** `NAME=value` assignments that prefixed the command, including those after `env`. */ @@ -174,6 +175,7 @@ export function agentShellCommands(input: string): ReadonlyArray 0, @@ -197,8 +199,15 @@ export interface AgentForgeCommand { export function classifyAgentShellCommands(input: string): ReadonlyArray { return agentShellCommands(input).flatMap((shellCommand) => { for (const definition of sourceControlDefinitions.definitions) { - const command = definition.classifyCommand?.(shellCommand.words) ?? null; + let command = definition.classifyCommand?.(shellCommand.words) ?? null; if (command === null) continue; + // Environment-selected hosts/repos may differ from this thread's repository. + if ( + definition.kind === "github" && + (shellCommand.assignments.get("GH_HOST") || shellCommand.assignments.get("GH_REPO")) + ) { + command = { kind: "other" }; + } return [ { forge: definition.kind, diff --git a/apps/web/src/components/settings/SourceControlSettings.tsx b/apps/web/src/components/settings/SourceControlSettings.tsx index c9d816faac246..48d63665e254a 100644 --- a/apps/web/src/components/settings/SourceControlSettings.tsx +++ b/apps/web/src/components/settings/SourceControlSettings.tsx @@ -380,7 +380,7 @@ function AgentGitHubReadGateSetting() { serverScoped settingKeys={["enableAgentGitHubReadGate"]} {...searchableSetting("agent-github-reads")} - description="T3 stops Claude agents from running gh reads it already answers. Turn this off to let them run gh freely. The change applies when an agent session next starts." + description="T3 sends supported gh reads from local agent sessions to its shared PR tools. Turn this off to allow direct reads. The change applies when an agent session next starts." resetAction={ settings.enableAgentGitHubReadGate !== DEFAULT_SERVER_SETTINGS.enableAgentGitHubReadGate ? (