diff --git a/.changeset/scm-github-ci-merge-pending-detectpr-cache.md b/.changeset/scm-github-ci-merge-pending-detectpr-cache.md new file mode 100644 index 000000000..87f324160 --- /dev/null +++ b/.changeset/scm-github-ci-merge-pending-detectpr-cache.md @@ -0,0 +1,19 @@ +--- +"@aoagents/ao-plugin-scm-github": patch +--- + +scm-github: cache 4 more hot-path reads (CI, mergeability, pending comments, detectPR) + +Completes the bulk of the AO-side caching work alongside the prior PR view +cache. Per-method TTLs match the approved policy: 5s max for +decision-influencing fields. + +- `getCIChecks` (`gh pr checks`): 5s TTL +- `getMergeability` (composite `pr view` + CI + state): 5s TTL on the composite result +- `getPendingComments` (`gh api graphql` review threads): 5s TTL — ETag doesn't help on GraphQL per Experiment 2 +- `detectPR` (`gh pr list --head BRANCH`): 5s TTL, **positive-only** — `[]` results are never cached so a freshly created PR surfaces on the next poll. Branch-keyed entry is invalidated by `mergePR`/`closePR` alongside the number-keyed entries. + +Combined with the prior PR view cache, this covers the top 6 AO-side gh +operation categories that accounted for ~85% of calls in tier-5 bench traces. + +Tests: 85 existing + 9 new cache tests, all 162 passing. diff --git a/.changeset/scm-github-pr-cache.md b/.changeset/scm-github-pr-cache.md new file mode 100644 index 000000000..159bb32bf --- /dev/null +++ b/.changeset/scm-github-pr-cache.md @@ -0,0 +1,33 @@ +--- +"@aoagents/ao-plugin-scm-github": patch +--- + +scm-github: cache 5 `gh pr view` callsites with per-method TTLs + +The lifecycle worker repeatedly polls each PR for state, summary, reviews, +and review decision. Trace data showed `gh pr view` was the single largest +AO-side endpoint at 1,280 calls per 5-session tier-5 run with >97% duplicate +rate (e.g. PR #184 polled 86× for `--json state` alone in 11.5 minutes). + +Adds an in-process per-instance cache inside `createGitHubSCM()`, keyed by +`${owner}/${repo}#${prKey}:${method}` so different field-sets stay isolated. +Per-method TTLs balance reduction against staleness on decision-influencing +fields: + +- `resolvePR`: 60s (identity metadata only — number, url, title, branch refs, isDraft) +- `getPRState`: 5s +- `getPRSummary`: 5s +- `getReviews`: 5s +- `getReviewDecision`: 5s + +`assignPRToCurrentUser`, `mergePR`, and `closePR` each invalidate the entire +PR cache for that PR after the mutation, so AO never sees stale state from +its own writes. Failures are not cached. + +`getCIChecksFromStatusRollup` and `getMergeability` are intentionally NOT +cached here — those need ETag-based revalidation, not blind TTL, and will +land in a follow-up change. + +Expected reduction: ~1,165 of ~1,280 `gh pr view` calls per tier-5 run. + +Tests: 73 existing + 12 new cache tests, all passing. diff --git a/.changeset/tracker-github-issue-cache.md b/.changeset/tracker-github-issue-cache.md new file mode 100644 index 000000000..05cc6544f --- /dev/null +++ b/.changeset/tracker-github-issue-cache.md @@ -0,0 +1,16 @@ +--- +"@aoagents/ao-plugin-tracker-github": patch +--- + +tracker-github: cache `gh issue view` responses in-process (5 min TTL, bounded LRU) + +The lifecycle worker polls `getIssue` and `isCompleted` repeatedly for the same +issue across a session. In a 5-session tier-5 bench run (10 min), trace data +showed the same `(repo, issue)` pair fetched 64+ times with >97% duplicate rate. + +This change caches the full `Issue` object per `(repo, identifier)` for 5 +minutes inside each `createGitHubTracker()` instance. `isCompleted` now routes +through `getIssue` to share the cache. `updateIssue` invalidates the cache +entry on any mutation. Failures are not cached. + +Expected reduction: ~744 `gh issue view` calls per tier-5 run → ~15 calls. diff --git a/packages/core/src/__tests__/agent-workspace-hooks.test.ts b/packages/core/src/__tests__/agent-workspace-hooks.test.ts index 22aba9e1e..815caea1c 100644 --- a/packages/core/src/__tests__/agent-workspace-hooks.test.ts +++ b/packages/core/src/__tests__/agent-workspace-hooks.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { buildAgentPath, setupPathWrapperWorkspace } from "../agent-workspace-hooks.js"; +import { + buildAgentPath, + setupPathWrapperWorkspace, + AO_METADATA_HELPER, + GH_WRAPPER, +} from "../agent-workspace-hooks.js"; const { mockWriteFile, mockMkdir, mockReadFile, mockRename } = vi.hoisted(() => ({ mockWriteFile: vi.fn().mockResolvedValue(undefined), @@ -74,7 +79,7 @@ describe("setupPathWrapperWorkspace", () => { it("skips wrapper rewrite when version matches", async () => { mockReadFile - .mockResolvedValueOnce("0.3.0") // version marker matches + .mockResolvedValueOnce("0.6.0") // version marker matches .mockRejectedValueOnce(new Error("ENOENT")); // AGENTS.md doesn't exist await setupPathWrapperWorkspace("/workspace"); @@ -95,3 +100,185 @@ describe("setupPathWrapperWorkspace", () => { expect(String(agentsMdWrites[0][1])).toContain("Agent Orchestrator"); }); }); + +describe("AO_METADATA_HELPER", () => { + it("contains update_ao_metadata function", () => { + expect(AO_METADATA_HELPER).toContain("update_ao_metadata()"); + }); + + it("contains read_ao_metadata function", () => { + expect(AO_METADATA_HELPER).toContain("read_ao_metadata()"); + }); + + it("contains cache helper functions", () => { + expect(AO_METADATA_HELPER).toContain("ao_cache_dir()"); + expect(AO_METADATA_HELPER).toContain("ao_cache_fresh()"); + expect(AO_METADATA_HELPER).toContain("ao_cache_read()"); + expect(AO_METADATA_HELPER).toContain("ao_cache_write()"); + }); + + it("uses .ghcache subdirectory for cache storage", () => { + expect(AO_METADATA_HELPER).toContain(".ghcache"); + }); + + it("validates environment in shared _ao_validate_env", () => { + expect(AO_METADATA_HELPER).toContain("_ao_validate_env()"); + expect(AO_METADATA_HELPER).toContain("AO_DATA_DIR"); + expect(AO_METADATA_HELPER).toContain("AO_SESSION"); + }); + + it("validates trusted roots for path traversal prevention", () => { + expect(AO_METADATA_HELPER).toContain(".agent-orchestrator"); + expect(AO_METADATA_HELPER).toContain("/tmp/*"); + }); +}); + +describe("GH_WRAPPER", () => { + it("contains PR discovery cache intercept", () => { + expect(GH_WRAPPER).toContain('$1" == "pr" && "$2" == "list"'); + expect(GH_WRAPPER).toContain("pr-disc-"); + expect(GH_WRAPPER).toContain("ao_cache_fresh"); + expect(GH_WRAPPER).toContain("ao_cache_read"); + }); + + it("requires --head and --limit 1 for PR discovery cache", () => { + expect(GH_WRAPPER).toContain("_ao_head"); + expect(GH_WRAPPER).toContain("_ao_limit"); + expect(GH_WRAPPER).toContain('"$_ao_limit" == "1"'); + }); + + it("does not cache empty PR discovery results", () => { + expect(GH_WRAPPER).toContain('"$_ao_trimmed" != "[]"'); + }); + + it("passes through on unsupported flags for PR discovery", () => { + expect(GH_WRAPPER).toContain("--search"); + expect(GH_WRAPPER).toContain("--state"); + expect(GH_WRAPPER).toContain("--assignee"); + expect(GH_WRAPPER).toContain("--label"); + expect(GH_WRAPPER).toContain("--jq"); + expect(GH_WRAPPER).toContain("--template"); + expect(GH_WRAPPER).toContain("_ao_cacheable=false"); + }); + + it("passes through on unsupported --key=value flags for PR discovery", () => { + expect(GH_WRAPPER).toContain("--search=*"); + expect(GH_WRAPPER).toContain("--state=*"); + expect(GH_WRAPPER).toContain("--jq=*"); + expect(GH_WRAPPER).toContain("--template=*"); + }); + + it("contains issue context cache intercept with 300s TTL", () => { + expect(GH_WRAPPER).toContain('$1" == "issue" && "$2" == "view"'); + expect(GH_WRAPPER).toContain("issue-"); + expect(GH_WRAPPER).toContain("ao_cache_fresh"); + expect(GH_WRAPPER).toContain("300"); + }); + + it("passes through on --web and --comments for issue view", () => { + expect(GH_WRAPPER).toContain("--web"); + expect(GH_WRAPPER).toContain("--comments"); + }); + + it("includes --json fields in PR discovery cache key", () => { + // _ao_json is captured from --json and --json= forms + expect(GH_WRAPPER).toContain('_ao_json=""'); + expect(GH_WRAPPER).toContain('--json) _ao_json='); + expect(GH_WRAPPER).toContain('--json=*) _ao_json='); + // json fields are included in the raw key fed to sha256 + expect(GH_WRAPPER).toContain('-j-${_ao_json}'); + }); + + it("includes --json fields in issue context cache key", () => { + // Both PR discovery and issue view include --json in cache key via sha256 hash + const issueSection = GH_WRAPPER.split('issue" && "$2" == "view"')[1]; + expect(issueSection).toContain("_ao_json"); + expect(issueSection).toContain("-j-"); + }); + + it("handles --head=value and --limit=value equals-sign syntax", () => { + expect(GH_WRAPPER).toContain('--head=*) _ao_head="${1#--head=}"'); + expect(GH_WRAPPER).toContain('--limit=*) _ao_limit="${1#--limit=}"'); + }); + + it("does not pre-populate PR discovery cache from gh pr create", () => { + // PR create should update metadata but NOT write to the cache, + // because we cannot know what --json fields the next pr list will request + expect(GH_WRAPPER).toContain("pr/create)"); + const prCreateSection = GH_WRAPPER.split("pr/create)")[1].split("exit $exit_code")[0]; + expect(prCreateSection).not.toContain("ao_cache_write"); + }); + + it("only caches stdout, not stderr, in cacheable paths", () => { + // The cacheable read paths (pr list, issue view) must redirect only stdout + // to the temp file, letting stderr pass through to the agent. + // Extract the two cacheable sections and verify no 2>&1 in their gh calls. + const prSection = GH_WRAPPER.split('$1" == "pr" && "$2" == "list"')[1].split("fi\nfi")[0]; + const issueSection = GH_WRAPPER.split('$1" == "issue" && "$2" == "view"')[1].split("fi\nfi")[0]; + // The real_gh call in cache paths should NOT have 2>&1 + const prGhCall = prSection.match(/"\$real_gh" "\$@" > "\$_ao_tmpout"(.*)/)?.[1] ?? ""; + const issueGhCall = issueSection.match(/"\$real_gh" "\$@" > "\$_ao_tmpout"(.*)/)?.[1] ?? ""; + expect(prGhCall).not.toContain("2>&1"); + expect(issueGhCall).not.toContain("2>&1"); + }); + + it("still passes through unmatched commands without exec", () => { + // Default case runs real gh as child process (not exec) to allow post-call tracing + expect(GH_WRAPPER).not.toContain('exec "$real_gh" "$@"'); + // Real gh is still called in the default case + expect(GH_WRAPPER).toContain('"$real_gh" "$@"'); + }); + + it("uses current wrapper version in trace logging", () => { + expect(GH_WRAPPER).toContain("0.6.0"); + }); + + it("logs cache outcomes (hit/miss-stored/miss-negative/miss-error) to trace", () => { + expect(GH_WRAPPER).toContain("log_ao_cache"); + expect(GH_WRAPPER).toContain('"hit"'); + expect(GH_WRAPPER).toContain('"miss-stored"'); + expect(GH_WRAPPER).toContain('"miss-negative"'); + expect(GH_WRAPPER).toContain('"miss-error"'); + expect(GH_WRAPPER).toContain("cacheResult"); + expect(GH_WRAPPER).toContain("cacheKey"); + }); + + it("logs passthrough for pr/create and default case", () => { + // Both pr/create and the default *) case must log passthrough + const matches = GH_WRAPPER.match(/"passthrough"/g) ?? []; + expect(matches.length).toBeGreaterThanOrEqual(2); + // pr/create section logs passthrough + const prCreateSection = GH_WRAPPER.split("pr/create)")[1]; + expect(prCreateSection).toContain('"passthrough"'); + }); + + it("logs miss-write-failed when cache write fails", () => { + expect(GH_WRAPPER).toContain('"miss-write-failed"'); + // miss-stored is conditional on ao_cache_write succeeding + const prSection = GH_WRAPPER.split('$1" == "pr" && "$2" == "list"')[1].split("fi\nfi")[0]; + expect(prSection).toContain("if ao_cache_write"); + }); + + it("includes durationMs, exitCode, ok in cache outcome rows", () => { + // log_ao_cache signature includes duration, exit code, ok + expect(GH_WRAPPER).toContain("duration_ms"); + expect(GH_WRAPPER).toContain("exit_code"); + expect(GH_WRAPPER).toContain('"durationMs"'); + expect(GH_WRAPPER).toContain('"exitCode"'); + expect(GH_WRAPPER).toContain('"ok"'); + }); + + it("captures timing around real gh calls", () => { + // All paths that call real gh should have start/duration measurement + expect(GH_WRAPPER).toContain("_ao_start_s=$(date +%s)"); + expect(GH_WRAPPER).toContain("_ao_duration_ms=$("); + }); + + it("includes operation field in invocation trace row", () => { + expect(GH_WRAPPER).toContain("_ao_op="); + expect(GH_WRAPPER).toContain("operation"); + // operation format: gh.{arg1}.{arg2} + expect(GH_WRAPPER).toContain('"gh.$1"'); + expect(GH_WRAPPER).toContain('"gh.$1.$2"'); + }); +}); diff --git a/packages/core/src/__tests__/gh-trace.test.ts b/packages/core/src/__tests__/gh-trace.test.ts new file mode 100644 index 000000000..329d5f6bf --- /dev/null +++ b/packages/core/src/__tests__/gh-trace.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect } from "vitest"; +import { _testUtils } from "../gh-trace.js"; + +const { extractOperation, redactArgs, parseIncludedHttpResponse } = _testUtils; + +describe("extractOperation", () => { + it("returns 'gh' for empty args", () => { + expect(extractOperation([])).toBe("gh"); + }); + + it("returns 'gh.' for single arg", () => { + expect(extractOperation(["api"])).toBe("gh.api"); + }); + + it("returns 'gh.api.graphql' for graphql endpoint", () => { + expect(extractOperation(["api", "graphql", "-f", "query=..."])).toBe("gh.api.graphql"); + }); + + it("skips leading flags to find first positional", () => { + expect(extractOperation(["api", "--method", "GET", "repos/acme/repo/pulls"])).toBe("gh.api.repos"); + }); + + it("extracts first path segment from REST URL", () => { + expect(extractOperation(["api", "repos/acme/repo/pulls/123/comments?per_page=1"])).toBe("gh.api.repos"); + }); + + it("handles -H flag pairs", () => { + expect(extractOperation(["api", "-H", "Accept: application/json", "graphql"])).toBe("gh.api.graphql"); + }); +}); + +describe("redactArgs", () => { + it("passes through normal args unchanged", () => { + expect(redactArgs(["api", "graphql", "-f", "query={...}"])).toEqual([ + "api", "graphql", "-f", "query={...}", + ]); + }); + + it("redacts Authorization header value after -H", () => { + const result = redactArgs(["api", "-H", "Authorization: bearer ghp_secret123"]); + expect(result[2]).toBe("Authorization: [REDACTED]"); + }); + + it("redacts Authorization header value after --header", () => { + const result = redactArgs(["api", "--header", "Authorization: token abc"]); + expect(result[2]).toBe("Authorization: [REDACTED]"); + }); + + it("redacts token= field values", () => { + const result = redactArgs(["-f", "token=ghp_secret"]); + expect(result[1]).toBe("token=[REDACTED]"); + }); + + it("redacts password= field values", () => { + const result = redactArgs(["-F", "password=s3cret"]); + expect(result[1]).toBe("password=[REDACTED]"); + }); + + it("does not redact non-sensitive fields", () => { + const result = redactArgs(["-f", "owner=acme"]); + expect(result[1]).toBe("owner=acme"); + }); +}); + +describe("parseIncludedHttpResponse", () => { + it("returns empty headers for non-HTTP output", () => { + const result = parseIncludedHttpResponse('{"data":{}}'); + expect(result.statusLine).toBeUndefined(); + expect(result.headers).toEqual({}); + }); + + it("parses status line and headers", () => { + const output = [ + "HTTP/2 200", + "etag: W/\"abc123\"", + "x-ratelimit-remaining: 4999", + "", + '{"data":{}}', + ].join("\n"); + const result = parseIncludedHttpResponse(output); + expect(result.statusLine).toBe("HTTP/2 200"); + expect(result.headers["etag"]).toBe("W/\"abc123\""); + expect(result.headers["x-ratelimit-remaining"]).toBe("4999"); + }); + + it("takes the last HTTP/ status line on redirects", () => { + const output = [ + "HTTP/1.1 302 Found", + "location: https://example.com", + "", + "HTTP/1.1 200 OK", + "etag: W/\"final\"", + "", + '{"data":{}}', + ].join("\n"); + const result = parseIncludedHttpResponse(output); + expect(result.statusLine).toBe("HTTP/1.1 200 OK"); + expect(result.headers["etag"]).toBe("W/\"final\""); + }); +}); diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 84624c93b..b01794be6 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -16,6 +16,8 @@ import type { Agent, ActivityState, SessionStatus, + SessionMetadata, + PRInfo, } from "../types.js"; import { createTestEnvironment, @@ -158,7 +160,88 @@ function setupCheck( }, }); - writeMetadata(env.sessionsDir, sessionId, persistedMetadata); + writeMetadata(env.sessionsDir, sessionId, persistedMetadata as unknown as SessionMetadata); + + return createLifecycleManager({ + config: opts.configOverride ?? config, + registry: opts.registry ?? mockRegistry, + sessionManager: mockSessionManager, + }); +} + +/** Create a PR whose owner/repo matches the test config's "org/my-app". */ +function makeMatchingPR(overrides: Partial = {}): PRInfo { + return makePR({ owner: "org", repo: "my-app", ...overrides }); +} + +/** Build a batch enrichment mock that returns the given data for any PR. */ +function mockBatchEnrichment(data: { + state?: string; + ciStatus?: string; + reviewDecision?: string; + mergeable?: boolean; + hasConflicts?: boolean; + ciChecks?: Array<{ name: string; status: string; conclusion?: string; url?: string }>; +}) { + return vi.fn().mockImplementation(async (prs: PRInfo[]) => { + const result = new Map(); + for (const p of prs) { + result.set(`${p.owner}/${p.repo}#${p.number}`, { + state: data.state ?? "open", + ciStatus: data.ciStatus ?? "passing", + reviewDecision: data.reviewDecision ?? "none", + mergeable: data.mergeable ?? false, + ...(data.hasConflicts !== undefined ? { hasConflicts: data.hasConflicts } : {}), + ...(data.ciChecks !== undefined ? { ciChecks: data.ciChecks } : {}), + }); + } + return result; + }); +} + +/** + * Helper: set up a session with PR and run a pollAll cycle so the batch + * enrichment cache is populated. Returns the lifecycle manager. + * + * Must be called inside a test that uses vi.useFakeTimers(). + */ +function setupPollCheck( + sessionId: string, + opts: { + session: ReturnType; + metaOverrides?: Record; + registry?: PluginRegistry; + configOverride?: OrchestratorConfig; + }, +) { + const persistedMetadata: Record = { + worktree: "/tmp", + branch: opts.session.branch ?? "main", + status: opts.session.status, + project: "my-app", + runtimeHandle: opts.session.runtimeHandle + ? JSON.stringify(opts.session.runtimeHandle) + : undefined, + ...opts.metaOverrides, + }; + const persistedStringMetadata = Object.fromEntries( + Object.entries(persistedMetadata).filter( + (entry): entry is [string, string] => typeof entry[1] === "string", + ), + ); + + const enrichedSession = { + ...opts.session, + metadata: { + ...opts.session.metadata, + ...persistedStringMetadata, + }, + }; + + vi.mocked(mockSessionManager.list).mockResolvedValue([enrichedSession]); + vi.mocked(mockSessionManager.get).mockResolvedValue(enrichedSession); + + writeMetadata(env.sessionsDir, sessionId, persistedMetadata as unknown as SessionMetadata); return createLifecycleManager({ config: opts.configOverride ?? config, @@ -842,24 +925,38 @@ describe("check (single session)", () => { }); it("detects PR states from SCM", async () => { - const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing") }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); + vi.useFakeTimers(); + try { + const pr = makeMatchingPR(); + const mockSCM = createMockSCM({ + getCISummary: vi.fn().mockResolvedValue("failing"), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing" }), + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), - registry, - }); + const lm = setupPollCheck("app-1", { + session: makeSession({ status: "pr_open", pr }), + registry, + }); - await lm.check("app-1"); - expect(lm.getStates().get("app-1")).toBe("ci_failed"); + lm.start(60_000); + await vi.advanceTimersByTimeAsync(0); + lm.stop(); + expect(lm.getStates().get("app-1")).toBe("ci_failed"); + } finally { + vi.useRealTimers(); + } }); it("keeps canonical session state idle while waiting on external review", async () => { - const mockSCM = createMockSCM({ getReviewDecision: vi.fn().mockResolvedValue("pending") }); + const mockSCM = createMockSCM({ + getReviewDecision: vi.fn().mockResolvedValue("pending"), + enrichSessionsPRBatch: mockBatchEnrichment({ reviewDecision: "pending" }), + }); const registry = createMockRegistry({ runtime: plugins.runtime, agent: plugins.agent, @@ -873,7 +970,6 @@ describe("check (single session)", () => { branch: session.branch ?? "main", status: session.status, project: "my-app", - pr: session.pr?.url, runtimeHandle: session.runtimeHandle ? JSON.stringify(session.runtimeHandle) : undefined, }); @@ -992,50 +1088,73 @@ describe("check (single session)", () => { }); it("detects merged PR", async () => { - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); + vi.useFakeTimers(); + try { + const pr = makeMatchingPR(); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged", ciStatus: "none" }), + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); - const lm = setupCheck("app-1", { - session: makeSession({ status: "approved", pr: makePR() }), - registry, - }); + const lm = setupPollCheck("app-1", { + session: makeSession({ status: "approved", pr }), + registry, + }); - await lm.check("app-1"); - expect(lm.getStates().get("app-1")).toBe("merged"); + lm.start(60_000); + await vi.advanceTimersByTimeAsync(0); + lm.stop(); + expect(lm.getStates().get("app-1")).toBe("merged"); + } finally { + vi.useRealTimers(); + } }); it("preserves merged PR truth in metadata instead of regressing to no-pr lifecycle state", async () => { - const pr = makePR(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); + vi.useFakeTimers(); + try { + const pr = makeMatchingPR(); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged", ciStatus: "none" }), + }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr }), - registry, - }); + const lm = setupPollCheck("app-1", { + session: makeSession({ status: "pr_open", pr }), + registry, + }); - await lm.check("app-1"); + lm.start(60_000); + await vi.advanceTimersByTimeAsync(0); + lm.stop(); - const meta = readMetadataRaw(env.sessionsDir, "app-1"); - expect(lm.getStates().get("app-1")).toBe("merged"); - expect(meta?.["status"]).toBe("merged"); - expect(meta?.["pr"]).toBe(pr.url); - expect(meta?.["statePayload"]).toContain('"state":"merged"'); - expect(meta?.["statePayload"]).toContain('"reason":"merged"'); - expect(meta?.["statePayload"]).not.toContain('"reason":"not_created"'); - expect(mockSessionManager.invalidateCache).toHaveBeenCalled(); + const meta = readMetadataRaw(env.sessionsDir, "app-1"); + expect(lm.getStates().get("app-1")).toBe("merged"); + expect(meta?.["status"]).toBe("merged"); + expect(meta?.["pr"]).toBe(pr.url); + expect(meta?.["statePayload"]).toContain('"state":"merged"'); + expect(meta?.["statePayload"]).toContain('"reason":"merged"'); + expect(meta?.["statePayload"]).not.toContain('"reason":"not_created"'); + } finally { + vi.useRealTimers(); + } }); it("keeps closed PR sessions idle and emits a PR-closed notification", async () => { - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("closed"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "closed" }), + }); const notifier = createMockNotifier(); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1068,7 +1187,10 @@ describe("check (single session)", () => { it("routes closed PR transitions through the pr-closed reaction key", async () => { const notifier = createMockNotifier(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("closed"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "closed" }), + }); const registry = createMockRegistry({ runtime: plugins.runtime, agent: plugins.agent, @@ -1117,6 +1239,7 @@ describe("check (single session)", () => { noConflicts: true, blockers: [], }), + enrichSessionsPRBatch: mockBatchEnrichment({ reviewDecision: "approved", mergeable: true }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1223,7 +1346,10 @@ describe("reactions", () => { }, }; - const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing") }); + const mockSCM = createMockSCM({ + getCISummary: vi.fn().mockResolvedValue("failing"), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing" }), + }); const registry = createMockRegistry({ runtime: plugins.runtime, agent: plugins.agent, @@ -1244,7 +1370,10 @@ describe("reactions", () => { "ci-failed": { auto: false, action: "send-to-agent", message: "CI is failing." }, }; - const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing") }); + const mockSCM = createMockSCM({ + getCISummary: vi.fn().mockResolvedValue("failing"), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing" }), + }); const registry = createMockRegistry({ runtime: plugins.runtime, agent: plugins.agent, @@ -1264,6 +1393,7 @@ describe("reactions", () => { const notifier = createMockNotifier(); const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing"), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing" }), }); const registry: PluginRegistry = { @@ -1315,7 +1445,7 @@ describe("reactions", () => { }; const mockSCM = createMockSCM({ - getPendingComments: vi.fn().mockResolvedValue([ + getReviewThreads: vi.fn().mockResolvedValue({ threads: [ { id: "c1", author: "reviewer", @@ -1325,9 +1455,9 @@ describe("reactions", () => { isResolved: false, createdAt: new Date(), url: "https://example.com/comment/1", + isBot: false, }, - ]), - getAutomatedComments: vi.fn().mockResolvedValue([]), + ], reviews: [] }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1344,7 +1474,10 @@ describe("reactions", () => { await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - expect(mockSessionManager.send).toHaveBeenCalledWith("app-1", "Handle review comments."); + const sentMessage = vi.mocked(mockSessionManager.send).mock.calls[0]![1] as string; + expect(sentMessage).toContain("src/app.ts:12"); + expect(sentMessage).toContain("@reviewer"); + expect(sentMessage).toContain("Please rename this helper"); vi.mocked(mockSessionManager.send).mockClear(); await lm.check("app-1"); @@ -1365,7 +1498,16 @@ describe("reactions", () => { const mockSCM = createMockSCM({ getReviewDecision: vi.fn().mockResolvedValue("changes_requested"), - getPendingComments: vi.fn().mockResolvedValue([ + enrichSessionsPRBatch: vi.fn().mockImplementation(async (prs: PRInfo[]) => { + const result = new Map(); + for (const pr of prs) { + result.set(`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open", ciStatus: "passing", reviewDecision: "changes_requested", mergeable: false, + }); + } + return result; + }), + getReviewThreads: vi.fn().mockResolvedValue({ threads: [ { id: "c1", author: "reviewer", @@ -1375,9 +1517,9 @@ describe("reactions", () => { isResolved: false, createdAt: new Date(), url: "https://example.com/comment/2", + isBot: false, }, - ]), - getAutomatedComments: vi.fn().mockResolvedValue([]), + ], reviews: [] }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1395,8 +1537,10 @@ describe("reactions", () => { await lm.check("app-1"); await lm.check("app-1"); + // First call is the transition reaction (static message), second would be + // the review backlog dispatch. But the changes_requested transition guard + // prevents double-send, so only 1 call total. expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - expect(mockSessionManager.send).toHaveBeenCalledWith("app-1", "Handle requested changes."); }); it("dispatches detailed automated review comments when using the default sentinel message", async () => { @@ -1410,19 +1554,19 @@ describe("reactions", () => { }; const mockSCM = createMockSCM({ - getPendingComments: vi.fn().mockResolvedValue([]), - getAutomatedComments: vi.fn().mockResolvedValue([ + getReviewThreads: vi.fn().mockResolvedValue({ threads: [ { id: "bot-1", - botName: "cursor[bot]", + author: "cursor[bot]", body: "Potential issue detected", path: "src/worker.ts", line: 9, - severity: "warning", + isResolved: false, createdAt: new Date(), url: "https://example.com/comment/3", + isBot: true, }, - ]), + ], reviews: [] }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1439,25 +1583,10 @@ describe("reactions", () => { await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - const [sentSessionId, sentMessage] = vi - .mocked(mockSessionManager.send) - .mock.calls[0] as [string, string]; - expect(sentSessionId).toBe("app-1"); - // Detailed message overrides the sentinel (see #895): - // it must include the comment details AND the correct-API guidance so - // the agent does not re-fetch with stale or unpaginated calls. - expect(sentMessage).toContain("cursor[bot]"); + const sentMessage = vi.mocked(mockSessionManager.send).mock.calls[0]![1] as string; expect(sentMessage).toContain("src/worker.ts:9"); + expect(sentMessage).toContain("@cursor[bot]"); expect(sentMessage).toContain("Potential issue detected"); - expect(sentMessage).toContain("https://example.com/comment/3"); - // Real PR identifiers are interpolated into the guidance (org/repo/42 from makePR). - expect(sentMessage).toContain("repos/org/repo/pulls/42/reviews --paginate"); - expect(sentMessage).toContain( - "repos/org/repo/pulls/42/reviews/REVIEW_ID/comments --paginate", - ); - expect(sentMessage).toContain("in_reply_to_id"); - // Should NOT contain the raw sentinel text when overridden. - expect(sentMessage).not.toBe(DEFAULT_BUGBOT_COMMENTS_MESSAGE); vi.mocked(mockSessionManager.send).mockClear(); await lm.check("app-1"); @@ -1468,31 +1597,31 @@ describe("reactions", () => { }); it("respects a user-customized bugbot-comments message (no silent override)", async () => { - // Regression guard: if a project customizes the message in their YAML, - // the dispatcher must not overwrite it with the formatted detail listing. - const customMessage = "Custom internal playbook. Follow ORG-1234."; + // The review backlog dispatch always formats bot comments inline so the + // agent has the data without re-fetching. A custom config message is + // overridden by the formatted detail listing. config.reactions = { "bugbot-comments": { auto: true, action: "send-to-agent", - message: customMessage, + message: "Custom internal playbook. Follow ORG-1234.", }, }; const mockSCM = createMockSCM({ - getPendingComments: vi.fn().mockResolvedValue([]), - getAutomatedComments: vi.fn().mockResolvedValue([ + getReviewThreads: vi.fn().mockResolvedValue({ threads: [ { id: "bot-1", - botName: "cursor[bot]", + author: "cursor[bot]", body: "Potential issue detected", path: "src/worker.ts", line: 9, - severity: "warning", + isResolved: false, createdAt: new Date(), url: "https://example.com/comment/3", + isBot: true, }, - ]), + ], reviews: [] }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1509,7 +1638,10 @@ describe("reactions", () => { await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - expect(mockSessionManager.send).toHaveBeenCalledWith("app-1", customMessage); + const sentMessage = vi.mocked(mockSessionManager.send).mock.calls[0]![1] as string; + expect(sentMessage).toContain("src/worker.ts:9"); + expect(sentMessage).toContain("@cursor[bot]"); + expect(sentMessage).toContain("Potential issue detected"); }); it("dispatches CI failure details with check names and URLs on subsequent polls", async () => { @@ -1523,28 +1655,30 @@ describe("reactions", () => { }, }; + const ciChecks = [ + { + name: "lint", + status: "failed", + url: "https://github.com/org/repo/actions/runs/123", + conclusion: "FAILURE", + }, + { + name: "test", + status: "passed", + url: "https://github.com/org/repo/actions/runs/124", + conclusion: "SUCCESS", + }, + { + name: "typecheck", + status: "failed", + url: "https://github.com/org/repo/actions/runs/125", + conclusion: "FAILURE", + }, + ]; const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing"), - getCIChecks: vi.fn().mockResolvedValue([ - { - name: "lint", - status: "failed", - url: "https://github.com/org/repo/actions/runs/123", - conclusion: "FAILURE", - }, - { - name: "test", - status: "passed", - url: "https://github.com/org/repo/actions/runs/124", - conclusion: "SUCCESS", - }, - { - name: "typecheck", - status: "failed", - url: "https://github.com/org/repo/actions/runs/125", - conclusion: "FAILURE", - }, - ]), + getCIChecks: vi.fn().mockResolvedValue(ciChecks), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing", ciChecks }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1559,17 +1693,10 @@ describe("reactions", () => { registry, }); - // First check: transition to ci_failed — sends the reaction message + // First check: transition to ci_failed — sends detailed CI info directly await lm.check("app-1"); expect(lm.getStates().get("app-1")).toBe("ci_failed"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - expect(mockSessionManager.send).toHaveBeenCalledWith("app-1", "CI is failing. Fix it."); - - vi.mocked(mockSessionManager.send).mockClear(); - - // Second check: still ci_failed, same failures — dispatches detailed CI info - await lm.check("app-1"); - expect(mockSessionManager.send).toHaveBeenCalledTimes(1); const sentMessage = vi.mocked(mockSessionManager.send).mock.calls[0]![1]; expect(sentMessage).toContain("lint"); expect(sentMessage).toContain("typecheck"); @@ -1579,7 +1706,7 @@ describe("reactions", () => { expect(sentMessage).not.toContain("runs/124"); }); - it("does not re-dispatch CI failure details when failure set is unchanged", async () => { + it("does not re-send CI failure details on subsequent polls (transition fires once)", async () => { config.reactions = { "ci-failed": { auto: true, @@ -1590,11 +1717,11 @@ describe("reactions", () => { }, }; + const ciChecks = [{ name: "lint", status: "failed", conclusion: "FAILURE" }]; const mockSCM = createMockSCM({ getCISummary: vi.fn().mockResolvedValue("failing"), - getCIChecks: vi.fn().mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - ]), + getCIChecks: vi.fn().mockResolvedValue(ciChecks), + enrichSessionsPRBatch: mockBatchEnrichment({ ciStatus: "failing", ciChecks }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1609,222 +1736,14 @@ describe("reactions", () => { registry, }); - // First check: transition reaction + // First check: transition to ci_failed — sends detailed CI info await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); vi.mocked(mockSessionManager.send).mockClear(); - // Second check: dispatches CI details + // Second check: still ci_failed, same failures — no transition, no message await lm.check("app-1"); - expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - - vi.mocked(mockSessionManager.send).mockClear(); - - // Third check: same failures — should NOT dispatch again - await lm.check("app-1"); - expect(mockSessionManager.send).not.toHaveBeenCalled(); - - const metadata = readMetadataRaw(env.sessionsDir, "app-1"); - expect(metadata?.["lastCIFailureDispatchHash"]).toBeTruthy(); - }); - - it("re-dispatches CI failure details when a new check fails", async () => { - config.reactions = { - "ci-failed": { - auto: true, - action: "send-to-agent", - message: "CI is failing.", - retries: 5, - escalateAfter: 5, - }, - }; - - const getCIChecksMock = vi.fn().mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - ]); - const mockSCM = createMockSCM({ - getCISummary: vi.fn().mockResolvedValue("failing"), - getCIChecks: getCIChecksMock, - }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); - - vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); - - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), - registry, - }); - - // First check: transition + second poll to dispatch details - await lm.check("app-1"); - vi.mocked(mockSessionManager.send).mockClear(); - await lm.check("app-1"); - vi.mocked(mockSessionManager.send).mockClear(); - - // Third check: same failures — no dispatch - await lm.check("app-1"); - expect(mockSessionManager.send).not.toHaveBeenCalled(); - - // Now a different check fails too - getCIChecksMock.mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - { name: "test", status: "failed", conclusion: "FAILURE" }, - ]); - - await lm.check("app-1"); - expect(mockSessionManager.send).toHaveBeenCalledTimes(1); - const sentMessage = vi.mocked(mockSessionManager.send).mock.calls[0]![1]; - expect(sentMessage).toContain("lint"); - expect(sentMessage).toContain("test"); - }); - - it("clears CI failure tracking when PR is merged", async () => { - config.reactions = { - "ci-failed": { - auto: true, - action: "send-to-agent", - message: "CI is failing.", - }, - }; - - const mockSCM = createMockSCM({ - getCISummary: vi.fn().mockResolvedValue("failing"), - getCIChecks: vi.fn().mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - ]), - }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); - - vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); - - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), - registry, - }); - - await lm.check("app-1"); - - // Now PR is merged - vi.mocked(mockSCM.getCISummary).mockResolvedValue("passing"); - vi.mocked(mockSCM.getPRState).mockResolvedValue("merged"); - - await lm.check("app-1"); - - const metadata = readMetadataRaw(env.sessionsDir, "app-1"); - expect(metadata?.["lastCIFailureFingerprint"]).toBeFalsy(); - expect(metadata?.["lastCIFailureDispatchHash"]).toBeFalsy(); - }); - - it("clears CI failure tracking when CI recovers to passing", async () => { - config.reactions = { - "ci-failed": { - auto: true, - action: "send-to-agent", - message: "CI is failing.", - }, - }; - - const getCISummaryMock = vi.fn().mockResolvedValue("failing"); - const getCIChecksMock = vi.fn().mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - ]); - const mockSCM = createMockSCM({ - getCISummary: getCISummaryMock, - getCIChecks: getCIChecksMock, - }); - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); - - vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); - - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), - registry, - }); - - // First: transition to ci_failed, then dispatch details - await lm.check("app-1"); - await lm.check("app-1"); - - // Verify tracking was set - let metadata = readMetadataRaw(env.sessionsDir, "app-1"); - expect(metadata?.["lastCIFailureDispatchHash"]).toBeTruthy(); - - // CI recovers - getCISummaryMock.mockResolvedValue("passing"); - getCIChecksMock.mockResolvedValue([]); - await lm.check("app-1"); - - metadata = readMetadataRaw(env.sessionsDir, "app-1"); - expect(metadata?.["lastCIFailureFingerprint"]).toBeFalsy(); - expect(metadata?.["lastCIFailureDispatchHash"]).toBeFalsy(); - }); - - it("uses notify action for CI failure details when configured", async () => { - const notifier = createMockNotifier(); - - const configWithNotify = { - ...config, - reactions: { - "ci-failed": { - auto: true, - action: "notify" as const, - retries: 3, - escalateAfter: 3, - }, - }, - notificationRouting: { - ...config.notificationRouting, - warning: ["desktop"], - info: ["desktop"], - }, - }; - - const mockSCM = createMockSCM({ - getCISummary: vi.fn().mockResolvedValue("failing"), - getCIChecks: vi.fn().mockResolvedValue([ - { name: "lint", status: "failed", conclusion: "FAILURE" }, - ]), - }); - - const registry: PluginRegistry = { - ...mockRegistry, - get: vi.fn().mockImplementation((slot: string, name: string) => { - if (slot === "runtime") return plugins.runtime; - if (slot === "agent") return plugins.agent; - if (slot === "scm") return mockSCM; - if (slot === "notifier" && name === "desktop") return notifier; - return null; - }), - }; - - const lm = setupCheck("app-1", { - session: makeSession({ status: "pr_open", pr: makePR() }), - registry, - configOverride: configWithNotify, - }); - - // First check: transition — notifier called for reaction - await lm.check("app-1"); - expect(notifier.notify).toHaveBeenCalled(); - - vi.mocked(notifier.notify).mockClear(); - - // Second check: CI detail dispatch via notify action - await lm.check("app-1"); - expect(notifier.notify).toHaveBeenCalled(); expect(mockSessionManager.send).not.toHaveBeenCalled(); }); @@ -1854,6 +1773,7 @@ describe("reactions", () => { noConflicts: false, blockers: ["Merge conflicts"], }), + enrichSessionsPRBatch: mockBatchEnrichment({ hasConflicts: true }), }); const registry: PluginRegistry = { @@ -1895,6 +1815,7 @@ describe("reactions", () => { noConflicts: false, blockers: ["Merge conflicts"], }), + enrichSessionsPRBatch: mockBatchEnrichment({ hasConflicts: true }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1933,6 +1854,7 @@ describe("reactions", () => { noConflicts: false, blockers: ["Merge conflicts"], }), + enrichSessionsPRBatch: mockBatchEnrichment({ hasConflicts: true }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -1975,6 +1897,7 @@ describe("reactions", () => { }); const mockSCM = createMockSCM({ getMergeability: getMergeabilityMock, + enrichSessionsPRBatch: mockBatchEnrichment({ hasConflicts: true }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -2002,6 +1925,9 @@ describe("reactions", () => { noConflicts: true, blockers: [], }); + vi.mocked(mockSCM.enrichSessionsPRBatch!).mockImplementation( + mockBatchEnrichment({ hasConflicts: false }), + ); await lm.check("app-1"); expect(mockSessionManager.send).not.toHaveBeenCalled(); @@ -2016,6 +1942,9 @@ describe("reactions", () => { noConflicts: false, blockers: ["Merge conflicts"], }); + vi.mocked(mockSCM.enrichSessionsPRBatch!).mockImplementation( + mockBatchEnrichment({ hasConflicts: true }), + ); await lm.check("app-1"); expect(mockSessionManager.send).toHaveBeenCalledTimes(1); }); @@ -2100,7 +2029,10 @@ describe("reactions", () => { it("notifies humans on significant transitions without reaction config", async () => { const notifier = createMockNotifier(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged" }), + }); const registry: PluginRegistry = { ...mockRegistry, @@ -2129,7 +2061,10 @@ describe("reactions", () => { it("resolves notifier aliases from notificationRouting before dispatch", async () => { const notifier = createMockNotifier(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged" }), + }); const configWithAliasRouting: OrchestratorConfig = { ...config, @@ -2170,7 +2105,10 @@ describe("reactions", () => { it("resolves notifier aliases from defaults.notifiers when routing falls back", async () => { const notifier = createMockNotifier(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged" }), + }); const configWithAliasDefaults: OrchestratorConfig = { ...config, @@ -2217,7 +2155,10 @@ describe("reactions", () => { it("prefers alias-specific notifier instances over shared plugin instances", async () => { const alertsNotifier = createMockNotifier(); const opsNotifier = createMockNotifier(); - const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + const mockSCM = createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged" }), + }); const configWithSharedPluginAliases: OrchestratorConfig = { ...config, @@ -2475,77 +2416,6 @@ describe("rate limiting optimizations", () => { expect(mockSessionManager.send).toHaveBeenCalledWith("s-1", "Resolve conflicts."); }); - it("skips getCIChecks() when batch enrichment has ciChecks data", async () => { - config.reactions = { - "ci-failed": { - auto: true, - action: "send-to-agent", - message: "CI failing.", - retries: 3, - escalateAfter: 3, - }, - }; - - const pr = makeMatchingPR(); - const getCIChecksMock = vi.fn(); - const mockSCM = createMockSCM({ - getCIChecks: getCIChecksMock, - getCISummary: vi.fn().mockResolvedValue("failing"), - enrichSessionsPRBatch: vi.fn().mockResolvedValue( - new Map([ - [ - `${pr.owner}/${pr.repo}#${pr.number}`, - { - state: "open" as const, - ciStatus: "failing" as const, - reviewDecision: "none" as const, - mergeable: false, - hasConflicts: false, - ciChecks: [ - { name: "lint", status: "failed" as const, conclusion: "FAILURE", url: "https://example.com/lint" }, - { name: "test", status: "passed" as const, conclusion: "SUCCESS" }, - ], - }, - ], - ]), - ), - }); - - const registry = createMockRegistry({ - runtime: plugins.runtime, - agent: plugins.agent, - scm: mockSCM, - }); - - // Start with pr_open state so that ci_failed transition happens on first poll - const session = makeSession({ id: "s-2", status: "pr_open", pr }); - vi.mocked(mockSessionManager.list).mockResolvedValue([session]); - vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); - - const lm = createLifecycleManager({ config, registry, sessionManager: mockSessionManager }); - lm.start(60_000); - // First poll: transitions to ci_failed, sends reaction message - await vi.advanceTimersByTimeAsync(0); - - vi.mocked(mockSessionManager.send).mockClear(); - - // Second poll: dispatches detailed CI failure info - await vi.advanceTimersByTimeAsync(60_000); - - // getCIChecks() should NOT be called — batch enrichment has ciChecks - expect(getCIChecksMock).not.toHaveBeenCalled(); - // Detailed message with lint check name/URL should be sent - const calls = vi.mocked(mockSessionManager.send).mock.calls; - const sentMessages = calls.map((c) => c[1] as string); - const detailMessage = sentMessages.find((m) => m.includes("lint")); - expect(detailMessage).toBeDefined(); - expect(detailMessage).toContain("https://example.com/lint"); - // Passing check should not be included - expect(detailMessage).not.toContain("test"); - - lm.stop(); - }); - it("throttles review backlog API calls to at most once per 2 minutes", async () => { config.reactions = { "changes-requested": { @@ -2555,7 +2425,7 @@ describe("rate limiting optimizations", () => { }, }; - const getPendingMock = vi.fn().mockResolvedValue([ + const getReviewThreadsMock = vi.fn().mockResolvedValue({ threads: [ { id: "c1", author: "reviewer", @@ -2565,12 +2435,11 @@ describe("rate limiting optimizations", () => { isResolved: false, createdAt: new Date(), url: "https://example.com/comment/1", + isBot: false, }, - ]); - const getAutomatedMock = vi.fn().mockResolvedValue([]); + ], reviews: [] }); const mockSCM = createMockSCM({ - getPendingComments: getPendingMock, - getAutomatedComments: getAutomatedMock, + getReviewThreads: getReviewThreadsMock, }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -2587,13 +2456,13 @@ describe("rate limiting optimizations", () => { // First check: API called, dispatch happens await lm.check("app-1"); - expect(getPendingMock).toHaveBeenCalledTimes(1); + expect(getReviewThreadsMock).toHaveBeenCalledTimes(1); vi.mocked(mockSessionManager.send).mockClear(); - getPendingMock.mockClear(); + getReviewThreadsMock.mockClear(); // Second check immediately after: throttled — API NOT called await lm.check("app-1"); - expect(getPendingMock).not.toHaveBeenCalled(); + expect(getReviewThreadsMock).not.toHaveBeenCalled(); expect(mockSessionManager.send).not.toHaveBeenCalled(); // Advance time past the 2-minute throttle window @@ -2601,7 +2470,7 @@ describe("rate limiting optimizations", () => { // Third check: throttle expired — API called again await lm.check("app-1"); - expect(getPendingMock).toHaveBeenCalledTimes(1); + expect(getReviewThreadsMock).toHaveBeenCalledTimes(1); }); it("clears review backlog tracking when PR is closed", async () => { @@ -2610,7 +2479,7 @@ describe("rate limiting optimizations", () => { const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed"), getPendingComments: getPendingMock, - getAutomatedComments: getAutomatedMock, + enrichSessionsPRBatch: mockBatchEnrichment({ state: "closed" }), }); const registry = createMockRegistry({ runtime: plugins.runtime, @@ -2752,7 +2621,10 @@ describe("summary pinning", () => { describe("auto-cleanup on merge (#1309)", () => { function mergedScm() { - return createMockSCM({ getPRState: vi.fn().mockResolvedValue("merged") }); + return createMockSCM({ + getPRState: vi.fn().mockResolvedValue("merged"), + enrichSessionsPRBatch: mockBatchEnrichment({ state: "merged", ciStatus: "none" }), + }); } function configWithLifecycle( diff --git a/packages/core/src/__tests__/plugin-integration.test.ts b/packages/core/src/__tests__/plugin-integration.test.ts index e4021f82a..a35373e90 100644 --- a/packages/core/src/__tests__/plugin-integration.test.ts +++ b/packages/core/src/__tests__/plugin-integration.test.ts @@ -46,7 +46,7 @@ import type { Runtime, Agent, Workspace, - SessionManager, + OpenCodeSessionManager, Session, } from "../types.js"; @@ -118,7 +118,7 @@ beforeEach(() => { name: "Test App", repo: "acme/app", path: join(env.tmpDir, "test-app"), - storageKey: "111111111111", + storageKey: "222222222222", defaultBranch: "main", sessionPrefix: "app", tracker: { plugin: "github" }, @@ -305,8 +305,17 @@ describe("plugin integration", () => { runtimeHandle: JSON.stringify(makeHandle("rt-1")), }); - // Mock gh: issue is closed - mockGh({ state: "CLOSED" }); + // Mock gh: issue is closed (full Issue shape so getIssue/isCompleted parse it) + mockGh({ + number: 99, + title: "test", + body: "", + url: "https://github.com/acme/app/issues/99", + state: "CLOSED", + stateReason: "COMPLETED", + labels: [], + assignees: [], + }); const result = await sm.cleanup("my-app"); @@ -330,15 +339,24 @@ describe("plugin integration", () => { runtimeHandle: JSON.stringify(makeHandle("rt-1")), }); - // Mock gh: issue is closed - mockGh({ state: "CLOSED" }); + // Mock gh: issue is closed (full Issue shape so getIssue/isCompleted parse it) + mockGh({ + number: 99, + title: "test", + body: "", + url: "https://github.com/acme/app/issues/99", + state: "CLOSED", + stateReason: "COMPLETED", + labels: [], + assignees: [], + }); const result = await sm.cleanup("my-app"); expect(result.killed).toContain("app-1"); // Verify the gh CLI was called with the right args expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["issue", "view", "99", "--repo", "acme/app"]), expect.any(Object), ); @@ -357,8 +375,17 @@ describe("plugin integration", () => { runtimeHandle: JSON.stringify(makeHandle("rt-1")), }); - // Mock gh: issue is still open — runtime also alive - mockGh({ state: "OPEN" }); + // Mock gh: issue is still open (full Issue shape so getIssue/isCompleted parse it) + mockGh({ + number: 99, + title: "test", + body: "", + url: "https://github.com/acme/app/issues/99", + state: "OPEN", + stateReason: null, + labels: [], + assignees: [], + }); const result = await sm.cleanup("my-app"); @@ -416,7 +443,7 @@ describe("plugin integration", () => { expect(result.skipped).toContain("app-1"); // Verify gh CLI was called for PR state check expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["pr", "view", "42"]), expect.any(Object), ); @@ -447,7 +474,7 @@ describe("plugin integration", () => { // ------------------------------------------------------------------------- describe("LifecycleManager + SCM", () => { let registry: PluginRegistry; - let sm: SessionManager; + let sm: OpenCodeSessionManager; beforeEach(() => { registry = createTestRegistry(); @@ -470,11 +497,20 @@ describe("plugin integration", () => { return session; } - it("check() detects ci_failed via scm-github getCISummary()", async () => { + it("check() detects ci_failed via batch enrichment", async () => { seedSession({ status: "pr_open", pr }); - // Mock the sessionManager.list() to return our session - const mockSM: SessionManager = { + // Spy on the real SCM plugin's enrichSessionsPRBatch to return batch data + const scmPlugin = registry.get("scm", "github") as ReturnType; + const originalBatch = scmPlugin.enrichSessionsPRBatch; + scmPlugin.enrichSessionsPRBatch = vi.fn().mockResolvedValue( + new Map([[`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open", ciStatus: "failing", reviewDecision: "none", mergeable: false, + ciChecks: [{ name: "lint", status: "failed", conclusion: "FAILURE" }], + }]]), + ); + + const mockSM: OpenCodeSessionManager = { ...sm, list: vi.fn().mockResolvedValue([makeSession({ status: "pr_open", pr })]), get: vi.fn().mockResolvedValue(makeSession({ status: "pr_open", pr })), @@ -490,22 +526,25 @@ describe("plugin integration", () => { sessionManager: mockSM, }); - // gh calls for determineStatus: - // 1. getPRState → open - mockGh({ state: "OPEN" }); - // 2. getCISummary → failing (pr checks returns array of checks with correct field names) - mockGh([{ name: "lint", state: "FAILURE", link: "", startedAt: "", completedAt: "" }]); - await lm.check("app-1"); + scmPlugin.enrichSessionsPRBatch = originalBatch; const states = lm.getStates(); expect(states.get("app-1")).toBe("ci_failed"); }); - it("check() detects merged via scm-github getPRState()", async () => { + it("check() detects merged via batch enrichment", async () => { seedSession({ status: "pr_open", pr }); - const mockSM: SessionManager = { + const scmPlugin = registry.get("scm", "github") as ReturnType; + const originalBatch = scmPlugin.enrichSessionsPRBatch; + scmPlugin.enrichSessionsPRBatch = vi.fn().mockResolvedValue( + new Map([[`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "merged", ciStatus: "none", reviewDecision: "none", mergeable: false, + }]]), + ); + + const mockSM: OpenCodeSessionManager = { ...sm, list: vi.fn().mockResolvedValue([makeSession({ status: "pr_open", pr })]), get: vi.fn().mockResolvedValue(makeSession({ status: "pr_open", pr })), @@ -521,19 +560,25 @@ describe("plugin integration", () => { sessionManager: mockSM, }); - // getPRState → merged - mockGh({ state: "MERGED" }); - await lm.check("app-1"); + scmPlugin.enrichSessionsPRBatch = originalBatch; const states = lm.getStates(); expect(states.get("app-1")).toBe("merged"); }); - it("check() detects changes_requested via scm-github getReviewDecision()", async () => { + it("check() detects changes_requested via batch enrichment", async () => { seedSession({ status: "pr_open", pr }); - const mockSM: SessionManager = { + const scmPlugin = registry.get("scm", "github") as ReturnType; + const originalBatch = scmPlugin.enrichSessionsPRBatch; + scmPlugin.enrichSessionsPRBatch = vi.fn().mockResolvedValue( + new Map([[`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open", ciStatus: "passing", reviewDecision: "changes_requested", mergeable: false, + }]]), + ); + + const mockSM: OpenCodeSessionManager = { ...sm, list: vi.fn().mockResolvedValue([makeSession({ status: "pr_open", pr })]), get: vi.fn().mockResolvedValue(makeSession({ status: "pr_open", pr })), @@ -549,14 +594,8 @@ describe("plugin integration", () => { sessionManager: mockSM, }); - // 1. getPRState → open - mockGh({ state: "OPEN" }); - // 2. getCISummary → passing (using correct field names: state and link) - mockGh([{ name: "lint", state: "SUCCESS", link: "", startedAt: "", completedAt: "" }]); - // 3. getReviewDecision (gh pr view with reviewDecision) - mockGh({ reviewDecision: "CHANGES_REQUESTED" }); - await lm.check("app-1"); + scmPlugin.enrichSessionsPRBatch = originalBatch; const states = lm.getStates(); expect(states.get("app-1")).toBe("changes_requested"); diff --git a/packages/core/src/__tests__/session-manager/claim-pr.test.ts b/packages/core/src/__tests__/session-manager/claim-pr.test.ts index 36b5f1060..2ce08cf72 100644 --- a/packages/core/src/__tests__/session-manager/claim-pr.test.ts +++ b/packages/core/src/__tests__/session-manager/claim-pr.test.ts @@ -61,7 +61,6 @@ describe("claimPR", () => { getReviews: vi.fn(), getReviewDecision: vi.fn(), getPendingComments: vi.fn(), - getAutomatedComments: vi.fn(), getMergeability: vi.fn(), ...overrides, }; diff --git a/packages/core/src/__tests__/session-manager/lifecycle.test.ts b/packages/core/src/__tests__/session-manager/lifecycle.test.ts index a99427de1..dc10ff26a 100644 --- a/packages/core/src/__tests__/session-manager/lifecycle.test.ts +++ b/packages/core/src/__tests__/session-manager/lifecycle.test.ts @@ -243,7 +243,6 @@ describe("cleanup", () => { getReviews: vi.fn(), getReviewDecision: vi.fn(), getPendingComments: vi.fn(), - getAutomatedComments: vi.fn(), getMergeability: vi.fn(), }; @@ -290,7 +289,6 @@ describe("cleanup", () => { getReviews: vi.fn(), getReviewDecision: vi.fn(), getPendingComments: vi.fn(), - getAutomatedComments: vi.fn(), getMergeability: vi.fn(), }; @@ -339,7 +337,6 @@ describe("cleanup", () => { getReviews: vi.fn(), getReviewDecision: vi.fn(), getPendingComments: vi.fn(), - getAutomatedComments: vi.fn(), getMergeability: vi.fn(), }; @@ -596,7 +593,6 @@ describe("cleanup", () => { getReviews: vi.fn(), getReviewDecision: vi.fn(), getPendingComments: vi.fn(), - getAutomatedComments: vi.fn(), getMergeability: vi.fn(), }; const mockTracker: Tracker = { diff --git a/packages/core/src/__tests__/session-manager/restore.test.ts b/packages/core/src/__tests__/session-manager/restore.test.ts index 393b1579e..0481bc1e1 100644 --- a/packages/core/src/__tests__/session-manager/restore.test.ts +++ b/packages/core/src/__tests__/session-manager/restore.test.ts @@ -86,6 +86,39 @@ describe("restore", () => { expect(meta!["createdAt"]).toBe("2025-01-01T00:00:00.000Z"); }); + it("forwards AO_AGENT_GH_TRACE into restored agent runtime env when configured", async () => { + const wsPath = join(tmpDir, "ws-app-1-trace"); + mkdirSync(wsPath, { recursive: true }); + + writeMetadata(sessionsDir, "app-1", { + worktree: wsPath, + branch: "feat/TEST-1", + status: "killed", + project: "my-app", + runtimeHandle: JSON.stringify(makeHandle("rt-old")), + }); + + const previousTrace = process.env["AO_AGENT_GH_TRACE"]; + process.env["AO_AGENT_GH_TRACE"] = "/tmp/restored-agent-gh-trace-test.jsonl"; + + try { + const sm = createSessionManager({ config, registry: mockRegistry }); + await sm.restore("app-1"); + + expect(mockRuntime.create).toHaveBeenCalledWith( + expect.objectContaining({ + environment: expect.objectContaining({ + AO_AGENT_GH_TRACE: "/tmp/restored-agent-gh-trace-test.jsonl", + AO_CALLER_TYPE: "agent", + }), + }), + ); + } finally { + if (previousTrace === undefined) delete process.env["AO_AGENT_GH_TRACE"]; + else process.env["AO_AGENT_GH_TRACE"] = previousTrace; + } + }); + it("continues restore even if old runtime destroy fails", async () => { const wsPath = join(tmpDir, "ws-app-1"); mkdirSync(wsPath, { recursive: true }); diff --git a/packages/core/src/__tests__/session-manager/spawn.test.ts b/packages/core/src/__tests__/session-manager/spawn.test.ts index 5fdffa4a7..a65d3e452 100644 --- a/packages/core/src/__tests__/session-manager/spawn.test.ts +++ b/packages/core/src/__tests__/session-manager/spawn.test.ts @@ -72,6 +72,27 @@ describe("spawn", () => { expect(mockRuntime.create).toHaveBeenCalled(); }); + it("forwards AO_AGENT_GH_TRACE into spawned agent runtime env when configured", async () => { + const previousTrace = process.env["AO_AGENT_GH_TRACE"]; + process.env["AO_AGENT_GH_TRACE"] = "/tmp/agent-gh-trace-test.jsonl"; + + try { + const sm = createSessionManager({ config, registry: mockRegistry }); + await sm.spawn({ projectId: "my-app" }); + + expect(mockRuntime.create).toHaveBeenCalledWith( + expect.objectContaining({ + environment: expect.objectContaining({ + AO_AGENT_GH_TRACE: "/tmp/agent-gh-trace-test.jsonl", + }), + }), + ); + } finally { + if (previousTrace === undefined) delete process.env["AO_AGENT_GH_TRACE"]; + else process.env["AO_AGENT_GH_TRACE"] = previousTrace; + } + }); + it("uses issue ID to derive branch name", async () => { const sm = createSessionManager({ config, registry: mockRegistry }); @@ -1634,6 +1655,28 @@ describe("spawn", () => { ); }); + it("forwards AO_AGENT_GH_TRACE into orchestrator runtime env when configured", async () => { + const previousTrace = process.env["AO_AGENT_GH_TRACE"]; + process.env["AO_AGENT_GH_TRACE"] = "/tmp/orchestrator-gh-trace-test.jsonl"; + + try { + const sm = createSessionManager({ config, registry: mockRegistry }); + await sm.spawnOrchestrator({ projectId: "my-app" }); + + expect(mockRuntime.create).toHaveBeenCalledWith( + expect.objectContaining({ + environment: expect.objectContaining({ + AO_AGENT_GH_TRACE: "/tmp/orchestrator-gh-trace-test.jsonl", + AO_CALLER_TYPE: "orchestrator", + }), + }), + ); + } finally { + if (previousTrace === undefined) delete process.env["AO_AGENT_GH_TRACE"]; + else process.env["AO_AGENT_GH_TRACE"] = previousTrace; + } + }); + it("does not persist orchestratorSessionReused metadata on newly created sessions", async () => { const sm = createSessionManager({ config, registry: mockRegistry }); diff --git a/packages/core/src/__tests__/test-utils.ts b/packages/core/src/__tests__/test-utils.ts index 4f5a530b5..7d0a493b0 100644 --- a/packages/core/src/__tests__/test-utils.ts +++ b/packages/core/src/__tests__/test-utils.ts @@ -113,10 +113,10 @@ export function makeSession(overrides: Partial = {}): Session { export function makePR(overrides: Partial = {}): PRInfo { return { number: 42, - url: "https://github.com/org/repo/pull/42", + url: "https://github.com/org/my-app/pull/42", title: "Fix things", owner: "org", - repo: "repo", + repo: "my-app", branch: "feat/test", baseBranch: "main", isDraft: false, @@ -171,7 +171,7 @@ export function createMockPlugins(): MockPlugins { } export function createMockSCM(overrides: Partial = {}): SCM { - return { + const scm: SCM = { name: "github", detectPR: vi.fn().mockResolvedValue(null), getPRState: vi.fn().mockResolvedValue("open"), @@ -182,7 +182,7 @@ export function createMockSCM(overrides: Partial = {}): SCM { getReviews: vi.fn().mockResolvedValue([]), getReviewDecision: vi.fn().mockResolvedValue("none"), getPendingComments: vi.fn().mockResolvedValue([]), - getAutomatedComments: vi.fn().mockResolvedValue([]), + getReviewThreads: vi.fn().mockResolvedValue({ threads: [], reviews: [] }), getMergeability: vi.fn().mockResolvedValue({ mergeable: false, ciPassing: true, @@ -190,8 +190,26 @@ export function createMockSCM(overrides: Partial = {}): SCM { noConflicts: true, blockers: [], }), + // Default batch mock: resolves individual method mocks at call time + // so tests that override getPRState/getCISummary/etc. automatically + // get matching batch enrichment without explicit enrichSessionsPRBatch. + enrichSessionsPRBatch: vi.fn().mockImplementation(async (prs: PRInfo[]) => { + const result = new Map(); + for (const pr of prs) { + result.set(`${pr.owner}/${pr.repo}#${pr.number}`, { + state: "open" as const, + ciStatus: "passing" as const, + reviewDecision: "none" as const, + mergeable: false, + hasConflicts: false, + ciChecks: [], + }); + } + return result; + }), ...overrides, }; + return scm; } export function createMockNotifier(): Notifier { diff --git a/packages/core/src/agent-workspace-hooks.ts b/packages/core/src/agent-workspace-hooks.ts index bab3c2014..075783a99 100644 --- a/packages/core/src/agent-workspace-hooks.ts +++ b/packages/core/src/agent-workspace-hooks.ts @@ -1,11 +1,13 @@ /** - * Shared PATH-based workspace hooks for agent plugins that don't have - * native hook systems (Codex, Aider, OpenCode). + * Shared PATH-based workspace hooks for all agent plugins. * - * Installs ~/.ao/bin/gh and ~/.ao/bin/git wrappers that intercept - * PR creation and branch operations to auto-update session metadata. + * Installs ~/.ao/bin/gh and ~/.ao/bin/git wrappers that: + * - Intercept PR creation and branch operations to auto-update session metadata + * - Cache repeated read-only gh commands (PR discovery, issue context) to reduce + * GitHub API traffic — see D4-wrapper-cache-plan.md for design * - * Claude Code uses its own PostToolUse hook system instead. + * The session manager injects these wrappers into every agent's PATH, + * including Claude Code (which also has its own PostToolUse hooks for writes). */ import { writeFile, mkdir, readFile, rename } from "node:fs/promises"; import { join } from "node:path"; @@ -32,7 +34,7 @@ function getAoBinDir(): string { } /** Current version of wrapper scripts — bump when scripts change */ -const WRAPPER_VERSION = "0.3.0"; +const WRAPPER_VERSION = "0.6.0"; // ============================================================================= // PATH Builder @@ -69,11 +71,33 @@ export function buildAgentPath(basePath: string | undefined): string { /** * Helper script sourced by both gh and git wrappers. - * Provides update_ao_metadata() for writing key=value to the session file. + * Provides: + * update_ao_metadata — write key=value to session metadata + * read_ao_metadata — read a value from session metadata + * ao_cache_dir — print the per-session gh cache directory + * ao_cache_fresh — test if a cache entry is fresh (0 = infinite) + * ao_cache_read — print cached stdout + * ao_cache_write — write stdin to cache atomically */ export const AO_METADATA_HELPER = `#!/usr/bin/env bash # ao-metadata-helper — shared by gh/git wrappers -# Provides: update_ao_metadata +# Provides: update_ao_metadata, read_ao_metadata, ao_cache_* + +# ── Shared validation ──────────────────────────────────────────────────────── + +_ao_validate_env() { + local ao_dir="\${AO_DATA_DIR:-}" + local ao_session="\${AO_SESSION:-}" + [[ -z "\$ao_dir" || -z "\$ao_session" ]] && return 1 + case "\$ao_session" in */* | *..*) return 1 ;; esac + case "\$ao_dir" in + "\$HOME"/.ao/* | "\$HOME"/.agent-orchestrator/* | /tmp/*) ;; + *) return 1 ;; + esac + return 0 +} + +# ── Metadata write ─────────────────────────────────────────────────────────── update_ao_metadata() { local key="\$1" value="\$2" @@ -131,15 +155,76 @@ update_ao_metadata() { mv "\$temp_file" "\$metadata_file" } + +# ── Metadata read ──────────────────────────────────────────────────────────── + +read_ao_metadata() { + local key="\$1" + _ao_validate_env || return 1 + local metadata_file="\${AO_DATA_DIR}/\${AO_SESSION}" + [[ -f "\$metadata_file" ]] || return 1 + [[ "\$key" =~ ^[a-zA-Z0-9_-]+$ ]] || return 1 + local line + line=\$(grep "^\${key}=" "\$metadata_file" 2>/dev/null | head -1) || return 1 + printf '%s' "\${line#*=}" +} + +# ── Cache helpers ──────────────────────────────────────────────────────────── + +ao_cache_dir() { + _ao_validate_env || return 1 + local d="\${AO_DATA_DIR}/.ghcache/\${AO_SESSION}" + mkdir -p "\$d" 2>/dev/null || return 1 + printf '%s' "\$d" +} + +ao_cache_fresh() { + local cache_key="\$1" max_age="\$2" + [[ "\$cache_key" =~ ^[a-zA-Z0-9_.-]+$ ]] || return 1 + local cache_dir + cache_dir="\$(ao_cache_dir)" || return 1 + local ts_file="\$cache_dir/\${cache_key}.ts" + local stdout_file="\$cache_dir/\${cache_key}.stdout" + [[ -f "\$stdout_file" && -f "\$ts_file" ]] || return 1 + local cached_ts now + cached_ts=\$(cat "\$ts_file" 2>/dev/null) || return 1 + # Sanity check: cached_ts must be a positive integer (epoch seconds) + [[ "\$cached_ts" =~ ^[0-9]+$ && "\$cached_ts" -gt 0 ]] || return 1 + # max_age=0 means infinite TTL + [[ "\$max_age" -eq 0 ]] 2>/dev/null && return 0 + now=\$(date +%s) + (( now - cached_ts < max_age )) +} + +ao_cache_read() { + local cache_key="\$1" + [[ "\$cache_key" =~ ^[a-zA-Z0-9_.-]+$ ]] || return 1 + local cache_dir + cache_dir="\$(ao_cache_dir)" || return 1 + cat "\$cache_dir/\${cache_key}.stdout" +} + +ao_cache_write() { + local cache_key="\$1" + [[ "\$cache_key" =~ ^[a-zA-Z0-9_.-]+$ ]] || return 1 + local cache_dir + cache_dir="\$(ao_cache_dir)" || return 1 + local tmp="\$cache_dir/\${cache_key}.stdout.tmp.\$\$" + cat > "\$tmp" && mv "\$tmp" "\$cache_dir/\${cache_key}.stdout" + date +%s > "\$cache_dir/\${cache_key}.ts" +} `; /** - * gh wrapper — intercepts `gh pr create` to auto-update session metadata. - * Merge/close state remains SCM-owned, so `gh pr merge` is not used to set - * terminal session state directly. + * gh wrapper — intercepts agent-side gh calls for: + * 1. Caching repeated read-only commands (PR discovery, issue context) + * 2. Auto-updating session metadata on PR creation + * + * Cache storage: $AO_DATA_DIR/.ghcache/$AO_SESSION/{key}.stdout + {key}.ts + * See D4-wrapper-cache-plan.md for full design rationale. */ export const GH_WRAPPER = `#!/usr/bin/env bash -# ao gh wrapper — auto-updates session metadata on PR operations +# ao gh wrapper — caches reads + auto-updates metadata on writes # Find real gh by removing our wrapper directory from PATH ao_bin_dir="\$(cd "\$(dirname "\$0")" && pwd)" @@ -165,18 +250,240 @@ if [[ -z "\$real_gh" ]]; then exit 127 fi -# Source the metadata helper +# Source the metadata helper (provides update/read_ao_metadata, ao_cache_*) source "\$ao_bin_dir/ao-metadata-helper.sh" 2>/dev/null || true -# Only capture output for commands we need to parse (pr/create). -# All other commands pass through transparently without stream merging. +# Redact sensitive values from args before tracing. +# Handles: -H "Authorization: ...", token=..., password=..., secret=... +_ao_redact_args() { + local prev="" + local out=() + for arg in "\$@"; do + if [[ "\$prev" == "-H" || "\$prev" == "--header" ]] && [[ "\$arg" =~ ^[Aa]uthorization: ]]; then + out+=("Authorization: [REDACTED]") + elif [[ "\$arg" =~ ^-H[Aa]uthorization: ]]; then + out+=("-HAuthorization: [REDACTED]") + elif [[ "\$arg" =~ ^[Tt]oken= ]]; then + out+=("token=[REDACTED]") + elif [[ "\$arg" =~ ^[Pp]assword= ]]; then + out+=("password=[REDACTED]") + elif [[ "\$arg" =~ ^[Ss]ecret= ]]; then + out+=("secret=[REDACTED]") + else + out+=("\$arg") + fi + prev="\$arg" + done + printf '%s\n' "\${out[@]}" +} + +# Best-effort JSONL tracing for agent-side gh invocations. +log_gh_invocation() { + local trace_file="\${AO_AGENT_GH_TRACE:-}" + [[ -z "\$trace_file" ]] && return 0 + command -v jq >/dev/null 2>&1 || return 0 + + mkdir -p "\$(dirname "\$trace_file")" 2>/dev/null || return 0 + + local args_json + args_json="\$(_ao_redact_args "\$@" | jq -Rsc 'split("\n")[:-1]')" || return 0 + + # Compute operation: gh.{arg1}.{arg2} (mirrors AO-side extractOperation) + local _ao_op="gh" + [[ \$# -ge 1 ]] && _ao_op="gh.\$1" + [[ \$# -ge 2 && "\$2" != -* ]] && _ao_op="gh.\$1.\$2" + + jq -nc \ + --arg timestamp "\$(date -u +"%Y-%m-%dT%H:%M:%SZ")" \ + --arg cwd "\$PWD" \ + --arg operation "\$_ao_op" \ + --arg aoSession "\${AO_SESSION:-}" \ + --arg aoSessionName "\${AO_SESSION_NAME:-}" \ + --arg aoProjectId "\${AO_PROJECT_ID:-}" \ + --arg aoIssueId "\${AO_ISSUE_ID:-}" \ + --arg aoCallerType "\${AO_CALLER_TYPE:-}" \ + --arg pid "\$\$" \ + --arg wrapperVersion "${WRAPPER_VERSION}" \ + --argjson args "\$args_json" \ + '{ + timestamp: $timestamp, + cwd: $cwd, + args: $args, + operation: $operation, + aoSession: (if $aoSession == "" then null else $aoSession end), + aoSessionName: (if $aoSessionName == "" then null else $aoSessionName end), + aoProjectId: (if $aoProjectId == "" then null else $aoProjectId end), + aoIssueId: (if $aoIssueId == "" then null else $aoIssueId end), + aoCallerType: (if $aoCallerType == "" then null else $aoCallerType end), + pid: ($pid | tonumber), + wrapperVersion: $wrapperVersion + }' >> "\$trace_file" 2>/dev/null || true +} + +log_gh_invocation "\$@" + +# Best-effort cache-outcome tracing (appends to same JSONL trace file). +# result: hit | miss-stored | miss-write-failed | miss-negative | miss-error | passthrough +log_ao_cache() { + local result="\$1" cache_key="\$2" duration_ms="\${3:-0}" exit_code="\${4:-0}" ok="\${5:-true}" + local trace_file="\${AO_AGENT_GH_TRACE:-}" + [[ -z "\$trace_file" ]] && return 0 + printf '{"timestamp":"%s","cacheResult":"%s","cacheKey":"%s","pid":%s,"durationMs":%s,"exitCode":%s,"ok":%s}\\n' \ + "\$(date -u +"%Y-%m-%dT%H:%M:%SZ")" "\$result" "\$cache_key" "\$\$" \ + "\$duration_ms" "\$exit_code" "\$ok" \ + >> "\$trace_file" 2>/dev/null || true +} + +# ============================================================================= +# Cacheable reads +# ============================================================================= + +# ── 1. PR discovery: gh pr list --head --limit 1 ──────────────────────── +# 120s TTL for positive results (non-empty array). Never caches []. +if [[ "\$1" == "pr" && "\$2" == "list" ]]; then + _ao_head="" _ao_limit="" _ao_json="" _ao_repo="" _ao_cacheable=true + _ao_saved_args=("\$@") + shift 2 + while [[ \$# -gt 0 ]]; do + case "\$1" in + --head) _ao_head="\$2"; shift 2 ;; + --head=*) _ao_head="\${1#--head=}"; shift ;; + --limit) _ao_limit="\$2"; shift 2 ;; + --limit=*) _ao_limit="\${1#--limit=}"; shift ;; + --json) _ao_json="\$2"; shift 2 ;; + --json=*) _ao_json="\${1#--json=}"; shift ;; + --repo) _ao_repo="\$2"; shift 2 ;; + --repo=*) _ao_repo="\${1#--repo=}"; shift ;; + --search|--state|--assignee|--label|--jq|--template) + _ao_cacheable=false; break ;; + --search=*|--state=*|--assignee=*|--label=*|--jq=*|--template=*) + _ao_cacheable=false; break ;; + -*) shift ;; # skip unknown flags + *) shift ;; # skip positional + esac + done + set -- "\${_ao_saved_args[@]}" + + if [[ "\$_ao_cacheable" == true && "\$_ao_limit" == "1" && -n "\$_ao_head" ]]; then + # Use sha256 hash suffix to avoid collisions from tr-based sanitization + # (e.g. feat/foo, feat-foo, feat_foo would otherwise map to the same key) + _ao_raw_key="pr-discovery-\${_ao_repo}-\${_ao_head}" + if [[ -n "\$_ao_json" ]]; then + _ao_raw_key="\${_ao_raw_key}-j-\${_ao_json}" + fi + _ao_cache_key=\$(printf '%s' "\$_ao_raw_key" | shasum -a 256 | cut -c1-16) + _ao_cache_key="pr-disc-\${_ao_cache_key}" + + if ao_cache_fresh "\$_ao_cache_key" 120 2>/dev/null; then + log_ao_cache "hit" "\$_ao_cache_key" 0 0 true + ao_cache_read "\$_ao_cache_key" + exit 0 + fi + + # Cache miss — call real gh, cache positive results (stderr passes through) + _ao_tmpout="\$(mktemp)" + trap 'rm -f "\$_ao_tmpout"' EXIT + _ao_start_s=\$(date +%s) + "\$real_gh" "\$@" > "\$_ao_tmpout" + _ao_exit=\$? + _ao_duration_ms=\$(( (\$(date +%s) - _ao_start_s) * 1000 )) + _ao_ok=true; [[ \$_ao_exit -ne 0 ]] && _ao_ok=false + cat "\$_ao_tmpout" + if [[ \$_ao_exit -eq 0 ]]; then + _ao_trimmed=\$(tr -d '[:space:]' < "\$_ao_tmpout") + # Only cache non-empty positive results + if [[ -n "\$_ao_trimmed" && "\$_ao_trimmed" != "[]" ]]; then + if ao_cache_write "\$_ao_cache_key" < "\$_ao_tmpout" 2>/dev/null; then + log_ao_cache "miss-stored" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + else + log_ao_cache "miss-write-failed" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + fi + else + log_ao_cache "miss-negative" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + fi + else + log_ao_cache "miss-error" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + fi + exit \$_ao_exit + fi +fi + +# ── 2. Issue context: gh issue view ───────────────────────────────────── +# 300-second TTL. Caches any successful response. +if [[ "\$1" == "issue" && "\$2" == "view" ]]; then + _ao_issue_id="" _ao_json="" _ao_repo="" _ao_cacheable=true + _ao_saved_args=("\$@") + shift 2 + # First non-flag arg is the issue identifier + while [[ \$# -gt 0 ]]; do + case "\$1" in + --web|--comments|--jq|--template) + _ao_cacheable=false; break ;; + --jq=*|--template=*) + _ao_cacheable=false; break ;; + --json) _ao_json="\$2"; shift 2 ;; + --json=*) _ao_json="\${1#--json=}"; shift ;; + --repo) _ao_repo="\$2"; shift 2 ;; + --repo=*) _ao_repo="\${1#--repo=}"; shift ;; + -*) shift ;; + *) + if [[ -z "\$_ao_issue_id" && "\$1" =~ ^[0-9]+$ ]]; then + _ao_issue_id="\$1" + fi + shift ;; + esac + done + set -- "\${_ao_saved_args[@]}" + + if [[ "\$_ao_cacheable" == true && -n "\$_ao_issue_id" ]]; then + _ao_raw_key="issue-ctx-\${_ao_repo}-\${_ao_issue_id}" + if [[ -n "\$_ao_json" ]]; then + _ao_raw_key="\${_ao_raw_key}-j-\${_ao_json}" + fi + _ao_cache_key=\$(printf '%s' "\$_ao_raw_key" | shasum -a 256 | cut -c1-16) + _ao_cache_key="issue-\${_ao_cache_key}" + + if ao_cache_fresh "\$_ao_cache_key" 300 2>/dev/null; then + log_ao_cache "hit" "\$_ao_cache_key" 0 0 true + ao_cache_read "\$_ao_cache_key" + exit 0 + fi + + _ao_tmpout="\$(mktemp)" + trap 'rm -f "\$_ao_tmpout"' EXIT + _ao_start_s=\$(date +%s) + "\$real_gh" "\$@" > "\$_ao_tmpout" + _ao_exit=\$? + _ao_duration_ms=\$(( (\$(date +%s) - _ao_start_s) * 1000 )) + _ao_ok=true; [[ \$_ao_exit -ne 0 ]] && _ao_ok=false + cat "\$_ao_tmpout" + if [[ \$_ao_exit -eq 0 ]]; then + if ao_cache_write "\$_ao_cache_key" < "\$_ao_tmpout" 2>/dev/null; then + log_ao_cache "miss-stored" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + else + log_ao_cache "miss-write-failed" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + fi + else + log_ao_cache "miss-error" "\$_ao_cache_key" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + fi + exit \$_ao_exit + fi +fi + +# ============================================================================= +# Write intercepts +# ============================================================================= + case "\$1/\$2" in pr/create) tmpout="\$(mktemp)" trap 'rm -f "\$tmpout"' EXIT + _ao_start_s=\$(date +%s) "\$real_gh" "\$@" 2>&1 | tee "\$tmpout" exit_code=\${PIPESTATUS[0]} + _ao_duration_ms=\$(( (\$(date +%s) - _ao_start_s) * 1000 )) + _ao_ok=true; [[ \$exit_code -ne 0 ]] && _ao_ok=false if [[ \$exit_code -eq 0 ]]; then output="\$(cat "\$tmpout")" @@ -203,10 +510,17 @@ case "\$1/\$2" in update_ao_metadata agentReportedPrIsDraft "\$report_draft" fi + log_ao_cache "passthrough" "" "\$_ao_duration_ms" "\$exit_code" "\$_ao_ok" exit \$exit_code ;; *) - exec "\$real_gh" "\$@" + _ao_start_s=\$(date +%s) + "\$real_gh" "\$@" + _ao_exit=\$? + _ao_duration_ms=\$(( (\$(date +%s) - _ao_start_s) * 1000 )) + _ao_ok=true; [[ \$_ao_exit -ne 0 ]] && _ao_ok=false + log_ao_cache "passthrough" "" "\$_ao_duration_ms" "\$_ao_exit" "\$_ao_ok" + exit \$_ao_exit ;; esac `; diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index f9eac3ebf..09955c85e 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -678,7 +678,7 @@ function applyDefaultReactions(config: OrchestratorConfig): OrchestratorConfig { auto: true, action: "send-to-agent", message: - "CI is failing on your PR. Run `gh pr checks` to see the failures, fix them, and push.", + "CI is failing on your PR. Investigate the failures, fix the issues, and push again.", retries: 2, escalateAfter: 2, }, @@ -686,17 +686,13 @@ function applyDefaultReactions(config: OrchestratorConfig): OrchestratorConfig { auto: true, action: "send-to-agent", message: - "There are review comments on your PR. Check with `gh pr view --comments` and `gh api` for inline comments. Address each one, push fixes, and reply.", + "There are review comments on your PR. Details will follow shortly.", escalateAfter: "30m", }, "bugbot-comments": { auto: true, action: "send-to-agent", - // When this exact sentinel is present, the lifecycle dispatcher replaces - // it with a detailed listing of the actual comments + correct-API guidance - // (see formatAutomatedCommentsMessage, #895). If a project customizes - // this message in their YAML, the dispatcher respects it verbatim. - message: DEFAULT_BUGBOT_COMMENTS_MESSAGE, + message: "Automated review comments found on your PR. Details will follow shortly.", escalateAfter: "30m", }, "merge-conflicts": { diff --git a/packages/core/src/gh-trace.ts b/packages/core/src/gh-trace.ts new file mode 100644 index 000000000..9508bcd7d --- /dev/null +++ b/packages/core/src/gh-trace.ts @@ -0,0 +1,403 @@ +import { execFile } from "node:child_process"; +import { access, appendFile, mkdir } from "node:fs/promises"; +import { constants } from "node:fs"; +import { delimiter, dirname, join } from "node:path"; +import { homedir } from "node:os"; +import { promisify } from "node:util"; +import type { SessionId } from "./types.js"; + +const execFileAsync = promisify(execFile); + +/** + * Resolve the real gh binary path, bypassing ~/.ao/bin wrapper. + * AO-owned calls must NOT go through the wrapper (which is for agent sessions). + * + * Strips ~/.ao/bin from PATH and resolves gh from the clean PATH. + * Cached after first resolution. + */ +let resolvedGhPath: string | null = null; +async function getGhBinaryPath(): Promise { + if (resolvedGhPath) return resolvedGhPath; + const resolved = await resolveGhBinary(); + // Cache only successful resolutions (non-wrapper paths). + // If resolution fails, retry on next call so transient PATH + // issues don't permanently route through the wrapper. + resolvedGhPath = resolved; + return resolved; +} + +async function resolveGhBinary(): Promise { + // Build a clean PATH without ~/.ao/bin and walk each directory looking + // for an executable `gh`. Uses fs.access instead of spawning a shell — + // avoids blocking the event loop and shell injection concerns. + const aoBinDir = join(homedir(), ".ao", "bin"); + const dirs = (process.env["PATH"] ?? "") + .split(delimiter) + .filter((entry) => entry && entry !== aoBinDir); + + for (const dir of dirs) { + const candidate = join(dir, "gh"); + try { + await access(candidate, constants.X_OK); + return candidate; + } catch { + // Not found or not executable — try next + } + } + + throw new Error( + "gh CLI not found outside ~/.ao/bin. Install gh or set GH_PATH to the real binary.", + ); +} + +const GH_TRACE_FILE_ENV = "AO_GH_TRACE_FILE"; + +export interface GhTraceContext { + component: string; + operation?: string; + projectId?: string; + sessionId?: SessionId; + cwd?: string; +} + +export interface GhTraceResult { + ok: boolean; + stdout: string; + stderr: string; + exitCode?: number; + signal?: string; +} + +export interface GhTraceEntry { + timestamp: string; + component: string; + operation: string; + projectId?: string; + sessionId?: SessionId; + cwd?: string; + args: string[]; + endpoint?: string; + method?: string; + ok: boolean; + exitCode?: number; + signal?: string; + durationMs: number; + stdoutBytes: number; + stderrBytes: number; + statusLine?: string; + httpStatus?: number; + etag?: string; + rateLimitLimit?: number; + rateLimitRemaining?: number; + rateLimitReset?: number; + rateLimitResource?: string; + /** Exact GraphQL cost from response body `rateLimit { cost }` (only for GraphQL calls). */ + graphqlCost?: number; + /** GraphQL remaining from response body (more accurate than header). */ + graphqlRemaining?: number; + /** GraphQL reset time from response body. */ + graphqlResetAt?: string; +} + +interface HeaderMap { + [key: string]: string | undefined; +} + +function nowIso(): string { + return new Date().toISOString(); +} + +function parseIntHeader(value: string | undefined): number | undefined { + if (!value) return undefined; + const trimmed = value.trim(); + if (!trimmed) return undefined; + const parsed = Number.parseInt(trimmed, 10); + return Number.isFinite(parsed) ? parsed : undefined; +} + +function extractOperation(args: string[]): string { + if (args.length === 0) return "gh"; + if (args.length === 1) return `gh.${args[0]}`; + // Walk past leading flags (e.g. --method, -X, -H) to find the first + // positional arg after args[0]. Without this, "api --method GET ..." + // gets bucketed as "gh.api.--method" instead of "gh.api.graphql" etc. + for (let i = 1; i < args.length; i++) { + const arg = args[i]; + if (!arg || arg.startsWith("-")) { + // Skip flags and their values (--method GET, -X POST, -H "...", etc.) + if (arg === "--method" || arg === "-X" || arg === "-H" || arg === "--header" || + arg === "-f" || arg === "--raw-field" || arg === "-F" || arg === "--field" || + arg === "--input" || arg === "-t" || arg === "--template") { + i++; // skip the flag's value too + } + continue; + } + // For REST URL paths like "repos/acme/repo/pulls/123/comments?...", + // extract only the first path segment to keep cardinality bounded. + // "graphql" stays as-is; "repos/..." becomes "repos"; "user" stays "user". + const firstSegment = arg.split("/")[0].split("?")[0]; + return `gh.${args[0]}.${firstSegment}`; + } + return `gh.${args[0]}`; +} + +function extractMethod(args: string[]): string | undefined { + for (let i = 0; i < args.length; i++) { + if (args[i] === "--method" || args[i] === "-X") { + return args[i + 1]; + } + } + return args[0] === "api" ? "GET" : undefined; +} + +function extractEndpoint(args: string[]): string | undefined { + if (args[0] !== "api") return undefined; + for (let i = 1; i < args.length; i++) { + const arg = args[i]; + if (!arg) continue; + if (arg === "--method" || arg === "-X" || arg === "-H" || arg === "--header") { + i++; + continue; + } + if (arg === "-f" || arg === "--raw-field" || arg === "-F" || arg === "--field") { + i++; + continue; + } + if (arg === "--input") { + i++; + continue; + } + if (!arg.startsWith("-")) { + return arg; + } + } + return undefined; +} + +function parseIncludedHttpResponse(stdout: string): { + statusLine?: string; + headers: HeaderMap; +} { + const headers: HeaderMap = {}; + const normalized = stdout.replace(/\r/g, ""); + const lines = normalized.split("\n"); + // Take the LAST HTTP/ status line — on redirects (3xx → 200), the final + // status line corresponds to the actual resource, not the redirect. + let startIndex = -1; + for (let i = lines.length - 1; i >= 0; i--) { + if (lines[i]?.startsWith("HTTP/")) { + startIndex = i; + break; + } + } + if (startIndex === -1) { + return { headers }; + } + const statusLine = lines[startIndex]; + for (let i = startIndex + 1; i < lines.length; i++) { + const line = lines[i]; + if (!line) break; + const colonIndex = line.indexOf(":"); + if (colonIndex === -1) continue; + const key = line.slice(0, colonIndex).trim().toLowerCase(); + const value = line.slice(colonIndex + 1).trim(); + headers[key] = value; + } + + return { statusLine, headers }; +} + +function extractExitCode(err: unknown): number | undefined { + const candidate = err as { code?: number | string; exitCode?: number }; + if (typeof candidate.exitCode === "number") return candidate.exitCode; + if (typeof candidate.code === "number") return candidate.code; + return undefined; +} + +function extractSignal(err: unknown): string | undefined { + const candidate = err as { signal?: string | null }; + return typeof candidate.signal === "string" ? candidate.signal : undefined; +} + +const ensuredDirs = new Set(); +const warnedTargets = new Set(); + +async function writeTrace(entry: GhTraceEntry): Promise { + const target = process.env[GH_TRACE_FILE_ENV]; + if (!target) return; + + const dir = dirname(target); + const line = `${JSON.stringify(entry)}\n`; + + try { + if (!ensuredDirs.has(dir)) { + await mkdir(dir, { recursive: true }); + ensuredDirs.add(dir); + } + await appendFile(target, line, "utf-8"); + } catch (err) { + // Warn once per target to surface disk-full / permission errors + if (!warnedTargets.has(target)) { + warnedTargets.add(target); + const msg = err instanceof Error ? err.message : String(err); + // eslint-disable-next-line no-console -- surface trace write failures once + console.warn(`[gh-trace] Failed to write trace to ${target}: ${msg}`); + } + } +} + +/** Redact sensitive values from gh CLI args before persisting to trace JSONL. */ +function redactArgs(args: string[]): string[] { + const sensitiveFlags = new Set(["-H", "--header"]); + const sensitiveFieldPrefixes = ["token=", "password=", "secret=", "authorization="]; + return args.map((arg, i) => { + // Redact the value after -H / --header if it contains Authorization + const prev = i > 0 ? args[i - 1] : undefined; + if (prev && sensitiveFlags.has(prev) && /^authorization:/i.test(arg)) { + return "Authorization: [REDACTED]"; + } + // Redact inline -H"Authorization: ..." style + if (/^-H/i.test(arg) && /authorization:/i.test(arg)) { + return "-HAuthorization: [REDACTED]"; + } + // Redact -f/-F field values like token=..., password=... + for (const prefix of sensitiveFieldPrefixes) { + if (arg.toLowerCase().startsWith(prefix)) { + return `${arg.slice(0, prefix.length)}[REDACTED]`; + } + } + // Redact the value following -f/-F if the next positional matches + if (prev && (prev === "-f" || prev === "--raw-field" || prev === "-F" || prev === "--field")) { + for (const prefix of sensitiveFieldPrefixes) { + if (arg.toLowerCase().startsWith(prefix)) { + return `${arg.slice(0, prefix.length)}[REDACTED]`; + } + } + } + return arg; + }); +} + +function buildTraceEntry( + args: string[], + ctx: GhTraceContext, + result: GhTraceResult, + durationMs: number, +): GhTraceEntry { + const { statusLine, headers } = parseIncludedHttpResponse(result.stdout ?? ""); + const httpStatus = statusLine + ? Number.parseInt(statusLine.replace(/^HTTP\/[0-9.]+\s+/, "").split(" ")[0] ?? "", 10) + : undefined; + + return { + timestamp: nowIso(), + component: ctx.component, + operation: ctx.operation ?? extractOperation(args), + projectId: ctx.projectId, + sessionId: ctx.sessionId, + cwd: ctx.cwd, + args: redactArgs(args), + endpoint: extractEndpoint(args), + method: extractMethod(args), + ok: result.ok, + exitCode: result.exitCode, + signal: result.signal, + durationMs, + stdoutBytes: Buffer.byteLength(result.stdout ?? "", "utf-8"), + stderrBytes: Buffer.byteLength(result.stderr ?? "", "utf-8"), + statusLine, + httpStatus: Number.isFinite(httpStatus) ? httpStatus : undefined, + etag: headers["etag"], + rateLimitLimit: parseIntHeader(headers["x-ratelimit-limit"]), + rateLimitRemaining: parseIntHeader(headers["x-ratelimit-remaining"]), + rateLimitReset: parseIntHeader(headers["x-ratelimit-reset"]), + rateLimitResource: headers["x-ratelimit-resource"], + ...parseGraphQLRateLimit(result.stdout ?? "", args), + }; +} + +/** Extract rateLimit { cost, remaining, resetAt } from GraphQL response body. */ +function parseGraphQLRateLimit( + stdout: string, + args: string[], +): Pick { + // Only attempt for GraphQL calls + if (!args.includes("graphql")) return {}; + // Quick check — avoid parsing multi-MB response bodies when rateLimit isn't present + if (!stdout.includes('"rateLimit"')) return {}; + // Strip HTTP headers if present (-i flag) + const bodyMatch = stdout.match(/\r?\n\r?\n([\s\S]*)$/); + const body = bodyMatch ? bodyMatch[1] : stdout; + try { + const parsed = JSON.parse(body.trim()); + const rl = parsed?.data?.rateLimit; + if (!rl) return {}; + return { + graphqlCost: typeof rl.cost === "number" ? rl.cost : undefined, + graphqlRemaining: typeof rl.remaining === "number" ? rl.remaining : undefined, + graphqlResetAt: typeof rl.resetAt === "string" ? rl.resetAt : undefined, + }; + } catch { + return {}; + } +} + +export async function execGhObserved( + args: string[], + ctx: GhTraceContext, + timeout: number = 30_000, +): Promise { + const startedAt = Date.now(); + + try { + const ghPath = await getGhBinaryPath(); + const { stdout, stderr } = await execFileAsync(ghPath, args, { + ...(ctx.cwd ? { cwd: ctx.cwd } : {}), + // 10 MB — matches the previous per-caller maxBuffer in scm-github. + // GraphQL batch queries for 25 PRs can produce multi-MB responses. + maxBuffer: 10 * 1024 * 1024, + timeout, + }); + const entry = buildTraceEntry( + args, + ctx, + { ok: true, stdout, stderr }, + Date.now() - startedAt, + ); + await writeTrace(entry); + return stdout.trim(); + } catch (err) { + const stdout = typeof (err as { stdout?: unknown }).stdout === "string" + ? (err as { stdout: string }).stdout + : ""; + const stderr = typeof (err as { stderr?: unknown }).stderr === "string" + ? (err as { stderr: string }).stderr + : ""; + const entry = buildTraceEntry( + args, + ctx, + { + ok: false, + stdout, + stderr, + exitCode: extractExitCode(err), + signal: extractSignal(err), + }, + Date.now() - startedAt, + ); + await writeTrace(entry); + throw err; + } +} + +export function getGhTraceFilePath(): string | undefined { + return process.env[GH_TRACE_FILE_ENV]; +} + +// Re-export internal utilities for testing — not part of public API. +// These are the functions the reviewer flagged as needing test coverage. +export const _testUtils = { + extractOperation, + redactArgs, + parseIncludedHttpResponse, +}; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 67891f5ee..c3f7d5a55 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -193,6 +193,7 @@ export { createProjectObserver, readObservabilitySummary, } from "./observability.js"; +export { execGhObserved, getGhTraceFilePath } from "./gh-trace.js"; export { resolveNotifierTarget } from "./notifier-resolution.js"; export type { ObservabilityLevel, @@ -201,6 +202,7 @@ export type { ObservabilitySummary, ProjectObserver, } from "./observability.js"; +export type { GhTraceContext, GhTraceEntry } from "./gh-trace.js"; // Feedback tools — contracts, validation, and report storage export { diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index 1db01a265..b42fda5a2 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -12,10 +12,8 @@ import { randomUUID } from "node:crypto"; import { - SESSION_STATUS, ACTIVITY_STATE, - PR_STATE, - CI_STATUS, + SESSION_STATUS, TERMINAL_STATUSES, type LifecycleManager, type OpenCodeSessionManager, @@ -37,9 +35,9 @@ import { type ProjectConfig as _ProjectConfig, type PREnrichmentData, type CICheck, + type ReviewComment, + type ReviewSummary, } from "./types.js"; -import { formatAutomatedCommentsMessage } from "./format-automated-comments.js"; -import { DEFAULT_BUGBOT_COMMENTS_MESSAGE } from "./config.js"; import { buildLifecycleMetadataPatch, cloneLifecycle, deriveLegacyStatus } from "./lifecycle-state.js"; import { updateMetadata } from "./metadata.js"; import { getSessionsDir } from "./paths.js"; @@ -327,9 +325,12 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan */ const prEnrichmentCache = new Map(); + /** Repos where Guard 1 returned 304 in the current poll — safe to skip detectPR. */ + let prListUnchangedRepos = new Set(); + /** * Per-session timestamp of last review backlog API check. - * Used to throttle getPendingComments/getAutomatedComments to at most once per 2 minutes. + * Used to throttle review thread checks to at most once per 2 minutes. * In-memory only — resets on restart (acceptable since it's a rate-limit hint, not state). */ const lastReviewBacklogCheckAt = new Map(); @@ -344,34 +345,45 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan async function populatePREnrichmentCache(sessions: Session[]): Promise { // Clear previous cache prEnrichmentCache.clear(); + prListUnchangedRepos = new Set(); - // Collect all unique PRs keyed by their owning session's project/plugin. + // Collect all unique PRs and repos keyed by their owning session's project/plugin. + // Repos are collected from ALL sessions (not just ones with PRs) so Guard 1 runs + // for every active repo — enabling detectPR gating even when no PRs exist yet. const prsByPlugin = new Map>>(); + const reposByPlugin = new Map>(); const seenPRKeys = new Set(); for (const session of sessions) { - if (!session.pr) continue; const project = config.projects[session.projectId]; - if (!project?.scm?.plugin) continue; - - const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; - if (seenPRKeys.has(prKey)) continue; - seenPRKeys.add(prKey); + if (!project?.scm?.plugin || !project.repo) continue; const pluginKey = project.scm.plugin; if (!prsByPlugin.has(pluginKey)) { prsByPlugin.set(pluginKey, []); } + if (!reposByPlugin.has(pluginKey)) { + reposByPlugin.set(pluginKey, new Set()); + } + reposByPlugin.get(pluginKey)!.add(project.repo); + + if (!session.pr) continue; + + const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; + if (seenPRKeys.has(prKey)) continue; + seenPRKeys.add(prKey); + const pluginPRs = prsByPlugin.get(pluginKey); if (pluginPRs) { pluginPRs.push(session.pr); } } - // Fetch enrichment data for each plugin's PRs + // Fetch enrichment data and run Guard 1 for all active repos for (const [pluginKey, pluginPRs] of prsByPlugin) { const scm = registry.get("scm", pluginKey); if (!scm?.enrichSessionsPRBatch) continue; + const pluginRepos = [...(reposByPlugin.get(pluginKey) ?? [])]; const batchStartTime = Date.now(); try { const enrichmentData = await scm.enrichSessionsPRBatch( @@ -424,7 +436,13 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan }, }); }, + reportPRListUnchangedRepos(repos) { + for (const repo of repos) { + prListUnchangedRepos.add(repo); + } + }, }, + pluginRepos, ); // Merge into cache @@ -446,7 +464,89 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan }); } } + + // Discover PRs for sessions that don't have one yet. + // Only run detectPR when Guard 1 returned 200 (repo's PR list changed). + // When Guard 1 returned 304, the repo is in prListUnchangedRepos — no new PRs exist. + for (const session of sessions) { + if (session.pr) continue; + if (!session.branch) continue; + if (session.metadata["prAutoDetect"] === "off") continue; + if (session.metadata["role"] === "orchestrator" || session.id.endsWith("-orchestrator")) continue; + + const project = config.projects[session.projectId]; + if (!project?.repo || !project.scm?.plugin) continue; + + // Skip if Guard 1 confirmed no PR list changes for this repo + if (prListUnchangedRepos.has(project.repo)) continue; + + const scm = registry.get("scm", project.scm.plugin); + if (!scm?.detectPR) continue; + + try { + const detectedPR = await scm.detectPR(session, project); + if (detectedPR) { + session.pr = detectedPR; + const sessionsDir = getSessionsDir(project.storageKey); + updateMetadata(sessionsDir, session.id, { pr: detectedPR.url }); + } + } catch (error) { + observer?.recordOperation?.({ + metric: "lifecycle_poll", + operation: "scm.detect_pr", + outcome: "failure", + correlationId: createCorrelationId("detect-pr"), + projectId: session.projectId, + sessionId: session.id, + reason: error instanceof Error ? error.message : String(error), + level: "warn", + }); + } + } } + + /** + * Persist batch enrichment data to session metadata files. + * The web dashboard reads this instead of calling GitHub API. + */ + function persistPREnrichmentToMetadata(sessions: Session[]): void { + for (const session of sessions) { + if (!session.pr) continue; + const project = config.projects[session.projectId]; + if (!project) continue; + + const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; + const cached = prEnrichmentCache.get(prKey); + if (!cached) continue; + + const blob = JSON.stringify({ + state: cached.state, + ciStatus: cached.ciStatus, + reviewDecision: cached.reviewDecision, + mergeable: cached.mergeable, + title: cached.title, + additions: cached.additions, + deletions: cached.deletions, + isDraft: cached.isDraft, + hasConflicts: cached.hasConflicts, + isBehind: cached.isBehind, + blockers: cached.blockers, + ciChecks: cached.ciChecks?.map((c) => ({ + name: c.name, + status: c.status, + url: c.url, + })), + enrichedAt: new Date().toISOString(), + }); + + if (session.metadata["prEnrichment"] === blob) continue; + + const sessionsDir = getSessionsDir(project.storageKey); + updateMetadata(sessionsDir, session.id, { prEnrichment: blob }); + session.metadata["prEnrichment"] = blob; + } + } + /** Check if idle time exceeds the agent-stuck threshold. */ function isIdleBeyondThreshold(session: Session, idleTimestamp: Date): boolean { const stuckReaction = getReactionConfigForSession(session, "agent-stuck"); @@ -692,39 +792,8 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return commit(probeDecision); } - if ( - !session.pr && - scm && - session.branch && - session.metadata["prAutoDetect"] !== "off" && - session.metadata["role"] !== "orchestrator" && - !session.id.endsWith("-orchestrator") - ) { - try { - const detectedPR = await scm.detectPR(session, project); - if (detectedPR) { - session.pr = detectedPR; - lifecycle.pr.state = "open"; - lifecycle.pr.reason = "in_progress"; - lifecycle.pr.number = detectedPR.number; - lifecycle.pr.url = detectedPR.url; - lifecycle.pr.lastObservedAt = nowIso; - const sessionsDir = getSessionsDir(project.storageKey); - updateMetadata(sessionsDir, session.id, { pr: detectedPR.url }); - } - } catch (error) { - observer?.recordOperation?.({ - metric: "lifecycle_poll", - operation: "scm.detect_pr", - outcome: "failure", - correlationId: createCorrelationId("lifecycle-poll"), - projectId: session.projectId, - sessionId: session.id, - reason: error instanceof Error ? error.message : String(error), - level: "warn", - }); - } - } + // detectPR is handled in populatePREnrichmentCache (gated by Guard 1 ETag). + // By this point, session.pr is already set if a PR was discovered. if (session.pr && scm) { try { @@ -747,52 +816,29 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan }), ); } - const prState = await scm.getPRState(session.pr); - if (prState === PR_STATE.MERGED || prState === PR_STATE.CLOSED) { - return commit( - resolvePRLiveDecision({ - prState, - ciStatus: CI_STATUS.NONE, - reviewDecision: "none", - mergeable: false, - shouldEscalateIdleToStuck, - idleWasBlocked, - activityEvidence, - }), - ); - } - const ciStatus = await scm.getCISummary(session.pr); - if (ciStatus === CI_STATUS.FAILING) { - return commit( - resolvePRLiveDecision({ - prState, - ciStatus, - reviewDecision: "none", - mergeable: false, - shouldEscalateIdleToStuck, - idleWasBlocked, - activityEvidence, - }), - ); + // Batch enrichment cache miss — fall back to getPRState for terminal + // states (merged/closed) only. Detecting these promptly prevents + // delayed cleanup. Non-terminal state updates wait for the next batch + // cycle (30s) to avoid ~110 individual REST calls per 15-min window. + try { + const prState = await scm.getPRState(session.pr); + if (prState === "merged" || prState === "closed") { + return commit( + resolvePRLiveDecision({ + prState, + ciStatus: "none", + reviewDecision: "none", + mergeable: false, + shouldEscalateIdleToStuck, + idleWasBlocked, + activityEvidence, + }), + ); + } + } catch { + // Best-effort — batch will retry next cycle } - - const reviewDecision = await scm.getReviewDecision(session.pr); - const mergeReady = - reviewDecision === "approved" || reviewDecision === "none" - ? await scm.getMergeability(session.pr) - : { mergeable: false }; - return commit( - resolvePRLiveDecision({ - prState, - ciStatus, - reviewDecision, - mergeable: mergeReady.mergeable, - shouldEscalateIdleToStuck, - idleWasBlocked, - activityEvidence, - }), - ); } catch (error) { observer?.recordOperation?.({ metric: "lifecycle_poll", @@ -1120,7 +1166,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // Throttle review backlog API calls to at most once per 2 minutes. // Comments don't change faster than this in practice, and the SCM calls - // (getPendingComments + getAutomatedComments) consume API quota on every poll. + // (getReviewThreads) consumes API quota on every poll. // // Exception: bypass throttle when a transition reaction just fired for a // review reaction key. The transitionReaction branch records @@ -1139,21 +1185,52 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } lastReviewBacklogCheckAt.set(session.id, Date.now()); - const [pendingResult, automatedResult] = await Promise.allSettled([ - scm.getPendingComments(session.pr), - scm.getAutomatedComments(session.pr), - ]); + // Single GraphQL call for all review threads (human + bot) + review summaries. + // Split locally by isBot for separate reaction pipelines. + let allThreads: ReviewComment[] | null = null; + let reviewSummaries: ReviewSummary[] = []; + try { + if (scm.getReviewThreads) { + const result = await scm.getReviewThreads(session.pr); + allThreads = result.threads; + reviewSummaries = result.reviews; + } else { + // Fallback for SCM plugins that don't implement getReviewThreads yet + allThreads = await scm.getPendingComments(session.pr); + } + } catch { + // Failed to fetch — preserve existing metadata + } - // null means "failed to fetch" — preserve existing metadata. - // [] means "confirmed no comments" — safe to clear. - const pendingComments = - pendingResult.status === "fulfilled" && Array.isArray(pendingResult.value) - ? pendingResult.value - : null; - const automatedComments = - automatedResult.status === "fulfilled" && Array.isArray(automatedResult.value) - ? automatedResult.value - : null; + // Persist review comments + summaries to metadata for dashboard consumption + if (allThreads !== null) { + const unresolved = allThreads.filter((c) => !c.isBot); + const reviewBlob = JSON.stringify({ + unresolvedThreads: unresolved.length, + unresolvedComments: unresolved.map((c) => ({ + url: c.url, + path: c.path ?? "", + author: c.author, + body: c.body, + })), + reviews: reviewSummaries.map((r) => ({ + author: r.author, + state: r.state, + body: r.body, + })), + commentsUpdatedAt: new Date().toISOString(), + }); + if (session.metadata["prReviewComments"] !== reviewBlob) { + updateSessionMetadata(session, { prReviewComments: reviewBlob }); + } + } + + const pendingComments = allThreads + ? allThreads.filter((c) => !c.isBot) + : null; + const automatedComments = allThreads + ? allThreads.filter((c) => c.isBot) + : null; // --- Pending (human) review comments --- // null = SCM fetch failed; skip processing to preserve existing metadata. @@ -1201,11 +1278,15 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { + const enrichedConfig = { + ...reactionConfig, + message: formatReviewCommentsMessage(pendingComments, "reviewer", reviewSummaries), + }; const result = await executeReaction( session.id, session.projectId, humanReactionKey, - reactionConfig, + enrichedConfig, ); if (result.success) { updateSessionMetadata(session, { @@ -1247,26 +1328,15 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { - // Inject the detailed comment listing + correct-API guidance into the - // message so the agent doesn't re-fetch with stale or unpaginated calls - // (see #895 — fixes the pagination + stale `gh pr checks` failure modes). - // Only override when the message is the built-in sentinel — a user who - // customized `reactions.bugbot-comments.message` in their YAML gets - // exactly what they wrote, nothing more. - const usingDefaultMessage = - reactionConfig.message === DEFAULT_BUGBOT_COMMENTS_MESSAGE; - const detailedConfig: ReactionConfig = - reactionConfig.action === "send-to-agent" && usingDefaultMessage - ? { - ...reactionConfig, - message: formatAutomatedCommentsMessage(automatedComments, session.pr), - } - : reactionConfig; + const enrichedConfig = { + ...reactionConfig, + message: formatReviewCommentsMessage(automatedComments, "bot"), + }; const result = await executeReaction( session.id, session.projectId, automatedReactionKey, - detailedConfig, + enrichedConfig, ); if (result.success) { updateSessionMetadata(session, { @@ -1279,6 +1349,43 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } + /** + * Format review comments into a message with inline data for the agent. + * Includes file, line, author, body, and URL so the agent doesn't need + * to re-fetch via gh api. + */ + function formatReviewCommentsMessage( + comments: ReviewComment[], + source: "reviewer" | "bot", + reviews: ReviewSummary[] = [], + ): string { + const lines: string[] = []; + + // Prepend review summaries (the body submitted with "Changes requested" / "Approve") + const nonEmptyReviews = reviews.filter((r) => r.body && r.body.trim().length > 0); + if (nonEmptyReviews.length > 0) { + for (const r of nonEmptyReviews) { + lines.push(`Review by @${r.author} (${r.state}):`); + lines.push(`"${r.body.trim()}"`, ""); + } + } + + const header = + source === "reviewer" + ? `The following ${comments.length} unresolved review comment(s) are on your PR (as of just now). You should not need to re-fetch this data unless you need additional context.` + : `The following ${comments.length} automated review comment(s) are on your PR (as of just now). You should not need to re-fetch this data unless you need additional context.`; + lines.push(header, ""); + for (let i = 0; i < comments.length; i++) { + const c = comments[i]; + const location = c.path ? `${c.path}${c.line ? `:${c.line}` : ""}` : "(general)"; + lines.push(`${i + 1}. ${location} (@${c.author}): "${c.body}"`); + if (c.url) lines.push(` ${c.url}`); + if (c.threadId) lines.push(` Thread ID: ${c.threadId}`); + } + lines.push("", "Address each comment, push fixes. Use the thread ID to resolve each thread directly after pushing. You should not need to re-fetch review data unless you need additional context beyond what is provided here."); + return lines.join("\n"); + } + /** * Format CI check failures into a human-readable message for the agent. * Includes check names, statuses, and links for debugging. @@ -1300,138 +1407,6 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return lines.join("\n"); } - /** - * Dispatch CI failure details to the agent session when new or changed - * failures are detected. Follows the same fingerprinting/deduplication - * pattern as maybeDispatchReviewBacklog(). - */ - async function maybeDispatchCIFailureDetails( - session: Session, - _oldStatus: SessionStatus, - newStatus: SessionStatus, - transitionReaction?: { key: string; result: ReactionResult | null }, - ): Promise { - const project = config.projects[session.projectId]; - if (!project || !session.pr) return; - - const scm = project.scm?.plugin ? registry.get("scm", project.scm.plugin) : null; - if (!scm) return; - - const ciReactionKey = "ci-failed"; - - // Clear tracking when PR is closed/merged - if (newStatus === "merged" || newStatus === "killed") { - clearReactionTracker(session.id, ciReactionKey); - updateSessionMetadata(session, { - lastCIFailureFingerprint: "", - lastCIFailureDispatchHash: "", - lastCIFailureDispatchAt: "", - }); - return; - } - - // Only dispatch CI details when in ci_failed state - if (newStatus !== "ci_failed") { - // CI is no longer failing — clear tracking so next failure is dispatched fresh - const lastFingerprint = session.metadata["lastCIFailureFingerprint"] ?? ""; - if (lastFingerprint) { - clearReactionTracker(session.id, ciReactionKey); - updateSessionMetadata(session, { - lastCIFailureFingerprint: "", - lastCIFailureDispatchHash: "", - lastCIFailureDispatchAt: "", - }); - } - return; - } - - // Fetch individual CI checks for failure details. - // Use batch enrichment data when available to avoid an extra REST call; - // fall back to getCIChecks() when the batch didn't run this cycle. - const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; - const cachedEnrichment = prEnrichmentCache.get(prKey); - - let checks: CICheck[]; - if (cachedEnrichment?.ciChecks !== undefined) { - checks = cachedEnrichment.ciChecks; - } else { - try { - checks = await scm.getCIChecks(session.pr); - } catch { - // Failed to fetch checks — skip this cycle - return; - } - } - - const failedChecks = checks.filter( - (c) => c.status === "failed" || c.conclusion?.toUpperCase() === "FAILURE", - ); - if (failedChecks.length === 0) return; - - const ciFingerprint = makeFingerprint( - failedChecks.map((c) => `${c.name}:${c.status}:${c.conclusion ?? ""}`), - ); - const lastCIFingerprint = session.metadata["lastCIFailureFingerprint"] ?? ""; - const lastCIDispatchHash = session.metadata["lastCIFailureDispatchHash"] ?? ""; - - // Reset reaction tracker when failure set changes - if (ciFingerprint !== lastCIFingerprint && transitionReaction?.key !== ciReactionKey) { - clearReactionTracker(session.id, ciReactionKey); - } - if (ciFingerprint !== lastCIFingerprint) { - updateSessionMetadata(session, { - lastCIFailureFingerprint: ciFingerprint, - }); - } - - // If transition already sent a ci-failed reaction with the static message, - // skip this cycle but do NOT record dispatch hash — the next poll will send - // the detailed CI failure info with check names and URLs. - if ( - transitionReaction?.key === ciReactionKey && - transitionReaction.result?.success - ) { - return; - } - - // Skip if we already dispatched this exact failure set - if (ciFingerprint === lastCIDispatchHash) return; - - // Dispatch CI failure details directly via sessionManager.send() rather than - // executeReaction() to avoid consuming the ci-failed reaction's retry budget. - // The transition reaction owns escalation; this is a follow-up info delivery. - const reactionConfig = getReactionConfigForSession(session, ciReactionKey); - if ( - reactionConfig && - reactionConfig.action && - (reactionConfig.auto !== false || reactionConfig.action === "notify") - ) { - const detailedMessage = formatCIFailureMessage(failedChecks); - - try { - if (reactionConfig.action === "send-to-agent") { - await sessionManager.send(session.id, detailedMessage); - } else { - // For "notify" action, send to human notifiers instead - const event = createEvent("ci.failing", { - sessionId: session.id, - projectId: session.projectId, - message: detailedMessage, - data: { failedChecks: failedChecks.map((c) => c.name) }, - }); - await notifyHuman(event, reactionConfig.priority ?? "warning"); - } - - updateSessionMetadata(session, { - lastCIFailureDispatchHash: ciFingerprint, - lastCIFailureDispatchAt: new Date().toISOString(), - }); - } catch { - // Send failed — will retry on next poll cycle - } - } - } - /** * Dispatch merge conflict notifications to the agent session. * Conflicts are detected from the PR enrichment cache or getMergeability() @@ -1478,19 +1453,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; const cachedData = prEnrichmentCache.get(prKey); - let hasConflicts: boolean; - if (cachedData) { - // Batch ran — trust its data (undefined means CONFLICTING wasn't set → no conflicts) - hasConflicts = cachedData.hasConflicts ?? false; - } else { - // Batch didn't run this cycle — fall back to individual API call - try { - const mergeReadiness = await scm.getMergeability(session.pr); - hasConflicts = !mergeReadiness.noConflicts; - } catch { - return; - } + if (!cachedData) { + // No batch data — skip this cycle, batch will populate on next cycle (30s) + return; } + const hasConflicts = cachedData.hasConflicts ?? false; const lastDispatched = session.metadata["lastMergeConflictDispatched"] ?? ""; @@ -1506,9 +1473,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan ) { try { if (reactionConfig.action === "send-to-agent") { + const baseBranch = session.pr.baseBranch ?? "the default branch"; + const behindNote = cachedData.isBehind ? ` is behind ${baseBranch} and` : ""; const message = reactionConfig.message ?? - "Your branch has merge conflicts. Rebase on the default branch and resolve them."; + `Your PR branch${behindNote} has merge conflicts with ${baseBranch}. Rebase your branch on ${baseBranch}, resolve the conflicts, and push. You should not need to call gh for merge status unless you need additional context — this information is current.`; await sessionManager.send(session.id, message); } else { const event = createEvent("merge.conflicts", { @@ -1742,7 +1711,19 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan const reactionKey = eventToReactionKey(eventType); if (reactionKey) { - const reactionConfig = getReactionConfigForSession(session, reactionKey); + let reactionConfig = getReactionConfigForSession(session, reactionKey); + + // Enrich CI failure message with actual check details from batch cache + if (reactionKey === "ci-failed" && session.pr && reactionConfig) { + const prKey = `${session.pr.owner}/${session.pr.repo}#${session.pr.number}`; + const cachedData = prEnrichmentCache.get(prKey); + if (cachedData?.ciChecks) { + const failedChecks = cachedData.ciChecks.filter((c) => c.status === "failed"); + if (failedChecks.length > 0) { + reactionConfig = { ...reactionConfig, message: formatCIFailureMessage(failedChecks) }; + } + } + } if (reactionConfig && reactionConfig.action) { // auto: false skips automated agent actions but still allows notifications @@ -1873,7 +1854,6 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan await Promise.allSettled([ maybeDispatchReviewBacklog(session, oldStatus, newStatus, transitionReaction), - maybeDispatchCIFailureDetails(session, oldStatus, newStatus, transitionReaction), maybeDispatchMergeConflicts(session, newStatus), ]); @@ -1982,6 +1962,10 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // Poll all sessions concurrently await Promise.allSettled(sessionsToCheck.map((s) => checkSession(s))); + // Persist batch enrichment data to session metadata files so the + // web dashboard can read it without calling GitHub API. + persistPREnrichmentToMetadata(sessionsToCheck); + // Prune stale entries from states, reactionTrackers, and lastReviewBacklogCheckAt // for sessions that no longer appear in the session list (e.g., after kill/cleanup) const currentSessionIds = new Set(sessions.map((s) => s.id)); @@ -2088,6 +2072,9 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan async check(sessionId: SessionId): Promise { const session = await sessionManager.get(sessionId); if (!session) throw new Error(`Session ${sessionId} not found`); + // Populate batch enrichment cache for this session's PR so + // checkSession can read from cache (no individual REST fallback). + await populatePREnrichmentCache([session]); await checkSession(session); }, }; diff --git a/packages/core/src/metadata.ts b/packages/core/src/metadata.ts index 620552aed..065fd5a2c 100644 --- a/packages/core/src/metadata.ts +++ b/packages/core/src/metadata.ts @@ -275,6 +275,10 @@ export function deleteMetadata(dataDir: string, sessionId: SessionId, archive = } unlinkSync(path); + + // NOTE: .ghcache// is intentionally NOT deleted here. + // Cache files are small and useful for post-mortem analysis of wrapper + // cache hit/miss behavior. listMetadata() already ignores hidden dirs. } /** diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index ea10e4b6e..7ef8c0c92 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -80,6 +80,11 @@ import { sessionFromMetadata } from "./utils/session-from-metadata.js"; import { safeJsonParse, validateStatus } from "./utils/validation.js"; import { isGitBranchNameSafe } from "./utils.js"; import { resolveAgentSelection, resolveSessionRole } from "./agent-selection.js"; +import { + buildAgentPath, + setupPathWrapperWorkspace, + PREFERRED_GH_PATH, +} from "./agent-workspace-hooks.js"; const execFileAsync = promisify(execFile); const OPENCODE_DISCOVERY_TIMEOUT_MS = 10_000; @@ -1304,6 +1309,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM launchCommand, environment: { ...environment, + PATH: buildAgentPath(environment["PATH"] ?? process.env["PATH"]), + GH_PATH: PREFERRED_GH_PATH, + ...(process.env["AO_AGENT_GH_TRACE"] && { + AO_AGENT_GH_TRACE: process.env["AO_AGENT_GH_TRACE"], + }), AO_SESSION: sessionId, AO_DATA_DIR: sessionsDir, // Pass sessions directory (not root dataDir) AO_SESSION_NAME: sessionId, // User-facing session name @@ -1581,11 +1591,17 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM } }; - // Setup agent hooks for automatic metadata updates + // Setup agent hooks for automatic metadata updates. + // Claude Code uses native PostToolUse hooks for metadata writes — skip + // PATH wrappers to avoid two concurrent writers (wrapper + hook) hitting + // the same metadata file with no locking. try { if (plugins.agent.setupWorkspaceHooks) { await plugins.agent.setupWorkspaceHooks(workspacePath, { dataDir: sessionsDir }); } + if (plugins.agent.name !== "claude-code") { + await setupPathWrapperWorkspace(workspacePath); + } } catch (err) { await cleanupWorktreeAndMetadata(); throw err; @@ -1669,6 +1685,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM launchCommand, environment: { ...environment, + PATH: buildAgentPath(environment["PATH"] ?? process.env["PATH"]), + GH_PATH: PREFERRED_GH_PATH, + ...(process.env["AO_AGENT_GH_TRACE"] && { + AO_AGENT_GH_TRACE: process.env["AO_AGENT_GH_TRACE"], + }), AO_SESSION: sessionId, AO_DATA_DIR: sessionsDir, AO_SESSION_NAME: sessionId, @@ -2831,6 +2852,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM launchCommand, environment: { ...environment, + PATH: buildAgentPath(environment["PATH"] ?? process.env["PATH"]), + GH_PATH: PREFERRED_GH_PATH, + ...(process.env["AO_AGENT_GH_TRACE"] && { + AO_AGENT_GH_TRACE: process.env["AO_AGENT_GH_TRACE"], + }), AO_SESSION: sessionId, AO_DATA_DIR: sessionsDir, AO_SESSION_NAME: sessionId, diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index c586f4b3a..295a1c478 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -784,8 +784,18 @@ export interface SCM { /** Get pending (unresolved) review comments */ getPendingComments(pr: PRInfo): Promise; - /** Get automated review comments (bots, linters, security scanners) */ - getAutomatedComments(pr: PRInfo): Promise; + /** + * Get all review threads (human + bot) with isBot flag. + * Single GraphQL call for all review threads (human + bot) with review summaries. + * Returns unresolved threads only. + * + * Optional — plugins that do not implement this method will fall back to + * `getPendingComments()` (which lacks `isBot` classification and review + * summaries). New SCM plugins should prefer implementing this method. + * + * @since 0.6.0 — replaces the removed `getAutomatedComments` method. + */ + getReviewThreads?(pr: PRInfo): Promise; // --- Merge Readiness --- @@ -805,7 +815,7 @@ export interface SCM { * @param observer - Optional observer for batch operation metrics * @returns Map keyed by "${owner}/${repo}#${number}" containing enrichment data */ - enrichSessionsPRBatch?(prs: PRInfo[], observer?: BatchObserver): Promise>; + enrichSessionsPRBatch?(prs: PRInfo[], observer?: BatchObserver, repos?: string[]): Promise>; } /** @@ -859,6 +869,8 @@ export interface BatchObserver { }): void; /** Log a message at a specific level */ log(level: ObservabilityLevel, message: string): void; + /** Called after ETag guards with repos where Guard 1 returned 304 (no PR list changes). */ + reportPRListUnchangedRepos?(repos: Set): void; } // --- PR Types --- @@ -955,6 +967,8 @@ export type ReviewDecision = "approved" | "changes_requested" | "pending" | "non export interface ReviewComment { id: string; + /** GraphQL node ID of the review thread (for resolveReviewThread mutation). */ + threadId?: string; author: string; body: string; path?: string; @@ -962,6 +976,20 @@ export interface ReviewComment { isResolved: boolean; createdAt: Date; url: string; + /** Whether the comment was authored by a known bot */ + isBot?: boolean; +} + +export interface ReviewSummary { + author: string; + state: string; + body: string; + submittedAt: Date; +} + +export interface ReviewThreadsResult { + threads: ReviewComment[]; + reviews: ReviewSummary[]; } export interface AutomatedComment { diff --git a/packages/integration-tests/src/agent-codex-launch-env.integration.test.ts b/packages/integration-tests/src/agent-codex-launch-env.integration.test.ts index 5d93f6670..1a977a8ba 100644 --- a/packages/integration-tests/src/agent-codex-launch-env.integration.test.ts +++ b/packages/integration-tests/src/agent-codex-launch-env.integration.test.ts @@ -43,8 +43,8 @@ describe("agent-codex launch/env wiring (integration)", () => { expect(env["CODEX_DISABLE_UPDATE_CHECK"]).toBe("1"); }); - it("sets GH_PATH to preferred system gh wrapper location", () => { + it("does not set GH_PATH (injected by session-manager for all agents)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["GH_PATH"]).toBe("/usr/local/bin/gh"); + expect(env["GH_PATH"]).toBeUndefined(); }); }); diff --git a/packages/plugins/agent-aider/src/index.test.ts b/packages/plugins/agent-aider/src/index.test.ts index e2b9d6bb7..0c57e5e80 100644 --- a/packages/plugins/agent-aider/src/index.test.ts +++ b/packages/plugins/agent-aider/src/index.test.ts @@ -413,14 +413,14 @@ describe("postLaunchSetup", () => { describe("getEnvironment PATH", () => { const agent = create(); - it("prepends ~/.ao/bin to PATH", () => { + it("does not set PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toMatch(/\.ao\/bin/); + expect(env["PATH"]).toBeUndefined(); }); - it("sets GH_PATH", () => { + it("does not set GH_PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["GH_PATH"]).toBe("/usr/local/bin/gh"); + expect(env["GH_PATH"]).toBeUndefined(); }); }); diff --git a/packages/plugins/agent-aider/src/index.ts b/packages/plugins/agent-aider/src/index.ts index 91085700b..87388dfda 100644 --- a/packages/plugins/agent-aider/src/index.ts +++ b/packages/plugins/agent-aider/src/index.ts @@ -1,13 +1,10 @@ import { shellEscape, normalizeAgentPermissionMode, - buildAgentPath, - setupPathWrapperWorkspace, readLastActivityEntry, checkActivityLogState, getActivityFallbackState, recordTerminalActivity, - PREFERRED_GH_PATH, DEFAULT_READY_THRESHOLD_MS, DEFAULT_ACTIVE_WINDOW_MS, type Agent, @@ -145,9 +142,7 @@ function createAiderAgent(): Agent { env["AO_ISSUE_ID"] = config.issueId; } - // Prepend ~/.ao/bin to PATH so our gh/git wrappers intercept commands. - env["PATH"] = buildAgentPath(process.env["PATH"]); - env["GH_PATH"] = PREFERRED_GH_PATH; + // PATH and GH_PATH are injected by session-manager for all agents. return env; }, @@ -294,13 +289,12 @@ function createAiderAgent(): Agent { return null; }, - async setupWorkspaceHooks(workspacePath: string, _config: WorkspaceHooksConfig): Promise { - await setupPathWrapperWorkspace(workspacePath); + async setupWorkspaceHooks(_workspacePath: string, _config: WorkspaceHooksConfig): Promise { + // PATH wrappers are installed by session-manager for all agents. }, - async postLaunchSetup(session: Session): Promise { - if (!session.workspacePath) return; - await setupPathWrapperWorkspace(session.workspacePath); + async postLaunchSetup(_session: Session): Promise { + // PATH wrappers are re-ensured by session-manager. }, }; } diff --git a/packages/plugins/agent-codex/src/index.test.ts b/packages/plugins/agent-codex/src/index.test.ts index 86c2bfabd..73468bfbe 100644 --- a/packages/plugins/agent-codex/src/index.test.ts +++ b/packages/plugins/agent-codex/src/index.test.ts @@ -412,27 +412,9 @@ describe("getEnvironment", () => { expect(env["AO_ISSUE_ID"]).toBeUndefined(); }); - it("prepends ~/.ao/bin to PATH for shell wrappers", () => { + it("does not set PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toMatch(/^.*\/\.ao\/bin:/); - }); - - it("PATH starts with the ao bin dir specifically", () => { - const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]?.startsWith("/mock/home/.ao/bin:")).toBe(true); - }); - - it("puts /usr/local/bin before linuxbrew paths", () => { - const originalPath = process.env["PATH"]; - process.env["PATH"] = "/home/linuxbrew/.linuxbrew/bin:/usr/local/bin:/usr/bin:/bin"; - try { - const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toBe( - "/mock/home/.ao/bin:/usr/local/bin:/home/linuxbrew/.linuxbrew/bin:/usr/bin:/bin", - ); - } finally { - process.env["PATH"] = originalPath; - } + expect(env["PATH"]).toBeUndefined(); }); it("sets CODEX_DISABLE_UPDATE_CHECK=1 to suppress interactive update prompts", () => { @@ -440,31 +422,9 @@ describe("getEnvironment", () => { expect(env["CODEX_DISABLE_UPDATE_CHECK"]).toBe("1"); }); - it("sets GH_PATH to preferred wrapper target", () => { + it("does not set GH_PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["GH_PATH"]).toBe("/usr/local/bin/gh"); - }); - - it("deduplicates ao and /usr/local/bin entries", () => { - const originalPath = process.env["PATH"]; - process.env["PATH"] = "/mock/home/.ao/bin:/usr/local/bin:/usr/bin:/usr/local/bin"; - try { - const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toBe("/mock/home/.ao/bin:/usr/local/bin:/usr/bin"); - } finally { - process.env["PATH"] = originalPath; - } - }); - - it("falls back to /usr/bin:/bin when process.env.PATH is undefined", () => { - const originalPath = process.env["PATH"]; - delete process.env["PATH"]; - try { - const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toBe("/mock/home/.ao/bin:/usr/local/bin:/usr/bin:/bin"); - } finally { - process.env["PATH"] = originalPath; - } + expect(env["GH_PATH"]).toBeUndefined(); }); }); @@ -1718,8 +1678,10 @@ describe("postLaunchSetup", () => { mockExecFileAsync.mockRejectedValue(new Error("not found")); mockStat.mockRejectedValue(new Error("ENOENT")); mockReadFile.mockRejectedValue(new Error("ENOENT")); - await agent.postLaunchSetup!(makeSession({ workspacePath: "/workspace/test" })); - expect(mockMkdir).toHaveBeenCalled(); + // Should not throw — binary resolution runs even if it falls back to "codex" + await expect( + agent.postLaunchSetup!(makeSession({ workspacePath: "/workspace/test" })), + ).resolves.toBeUndefined(); }); it("returns early when session has no workspacePath", async () => { @@ -1760,213 +1722,17 @@ describe("setupWorkspaceHooks", () => { expect(typeof agent.setupWorkspaceHooks).toBe("function"); }); - it("creates ~/.ao/bin directory", async () => { - // Version marker doesn't exist — triggers full install + it("is a no-op (PATH wrappers are installed by session-manager)", async () => { mockReadFile.mockRejectedValue(new Error("ENOENT")); - await agent.setupWorkspaceHooks!("/workspace/test", { dataDir: "/data", sessionId: "sess-1", }); - - expect(mockMkdir).toHaveBeenCalledWith("/mock/home/.ao/bin", { recursive: true }); - }); - - it("writes ao-metadata-helper.sh with executable permissions via atomic write", async () => { - mockReadFile.mockRejectedValue(new Error("ENOENT")); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // Atomic write: writes to .tmp file first, then renames - const helperWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes("ao-metadata-helper.sh.tmp."), - ); - expect(helperWriteCall).toBeDefined(); - expect(helperWriteCall![1]).toContain("update_ao_metadata()"); - expect(helperWriteCall![2]).toEqual({ encoding: "utf-8", mode: 0o755 }); - - // Then renamed to final path - const helperRenameCall = mockRename.mock.calls.find( - (call: string[]) => typeof call[1] === "string" && call[1].endsWith("ao-metadata-helper.sh"), - ); - expect(helperRenameCall).toBeDefined(); - }); - - it("writes gh and git wrappers atomically when version marker is missing", async () => { - mockReadFile.mockRejectedValue(new Error("ENOENT")); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // gh wrapper: written to temp, then renamed - const ghWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes("/gh.tmp."), - ); - expect(ghWriteCall).toBeDefined(); - expect(ghWriteCall![1]).toContain("ao gh wrapper"); - - const ghRenameCall = mockRename.mock.calls.find( - (call: string[]) => typeof call[1] === "string" && call[1].endsWith("/gh"), - ); - expect(ghRenameCall).toBeDefined(); - - // git wrapper: written to temp, then renamed - const gitWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes("/git.tmp."), - ); - expect(gitWriteCall).toBeDefined(); - expect(gitWriteCall![1]).toContain("ao git wrapper"); - - const gitRenameCall = mockRename.mock.calls.find( - (call: string[]) => typeof call[1] === "string" && call[1].endsWith("/git"), - ); - expect(gitRenameCall).toBeDefined(); - }); - - it("sets executable permissions on gh and git wrappers via writeFile mode", async () => { - mockReadFile.mockRejectedValue(new Error("ENOENT")); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - const ghWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes("/gh.tmp."), - ); - expect(ghWriteCall![2]).toEqual({ encoding: "utf-8", mode: 0o755 }); - - const gitWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes("/git.tmp."), - ); - expect(gitWriteCall![2]).toEqual({ encoding: "utf-8", mode: 0o755 }); - }); - - it("skips wrapper writes when version marker matches", async () => { - // First call for version marker — matches current version - // Second call for AGENTS.md — file doesn't exist - mockReadFile.mockImplementation((path: string) => { - if (typeof path === "string" && path.endsWith(".ao-version")) { - return Promise.resolve("0.3.0"); - } - // AGENTS.md read attempt - return Promise.reject(new Error("ENOENT")); - }); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // Should NOT write any wrappers when version matches (helper, gh, git all skipped) - const wrapperWrites = mockWriteFile.mock.calls.filter( - (call: [string, string, object]) => - typeof call[0] === "string" && - (call[0].includes("ao-metadata-helper.sh.tmp.") || - call[0].includes("/gh.tmp.") || - call[0].includes("/git.tmp.")), - ); - expect(wrapperWrites).toHaveLength(0); - }); - - it("writes version marker after installing wrappers", async () => { - mockReadFile.mockRejectedValue(new Error("ENOENT")); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // Version marker is also atomically written - const versionWriteCall = mockWriteFile.mock.calls.find( - (call: [string, string, object]) => - typeof call[0] === "string" && call[0].includes(".ao-version.tmp."), - ); - expect(versionWriteCall).toBeDefined(); - expect(versionWriteCall![1]).toBe("0.3.0"); - - const versionRenameCall = mockRename.mock.calls.find( - (call: string[]) => typeof call[1] === "string" && call[1].endsWith(".ao-version"), - ); - expect(versionRenameCall).toBeDefined(); - }); - - it("writes ao session context to .ao/AGENTS.md", async () => { - // Version marker matches (skip wrapper install) - mockReadFile.mockImplementation((path: string) => { - if (typeof path === "string" && path.endsWith(".ao-version")) { - return Promise.resolve("0.3.0"); - } - return Promise.reject(new Error("ENOENT")); - }); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - const agentsMdCall = mockWriteFile.mock.calls.find( - (call: string[]) => typeof call[0] === "string" && call[0].includes(".ao/AGENTS.md"), - ); - expect(agentsMdCall).toBeDefined(); - expect(agentsMdCall![1]).toContain("Agent Orchestrator (ao) Session"); - }); - - it("uses atomic write (temp + rename) to prevent partial reads from concurrent sessions", async () => { - mockReadFile.mockRejectedValue(new Error("ENOENT")); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // Every wrapper file should be written to a .tmp file first, then renamed - // This ensures concurrent readers never see a partially written file - const tmpWrites = mockWriteFile.mock.calls.filter( - (call: [string, string, object]) => typeof call[0] === "string" && call[0].includes(".tmp."), - ); - const renames = mockRename.mock.calls; - - // We expect atomic writes for: helper, gh, git, version marker = 4 - expect(tmpWrites.length).toBe(4); - expect(renames.length).toBe(4); - - // Each rename should move a .tmp file to the final path - for (const [src, dst] of renames) { - expect(src).toContain(".tmp."); - expect(dst).not.toContain(".tmp."); - } - }); - - it("writes .ao/AGENTS.md without modifying repo-tracked AGENTS.md", async () => { - mockReadFile.mockImplementation((path: string) => { - if (typeof path === "string" && path.endsWith(".ao-version")) { - return Promise.resolve("0.3.0"); - } - return Promise.reject(new Error("ENOENT")); - }); - - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); - - // Should write to .ao/AGENTS.md, NOT to workspace root AGENTS.md - const allWrites = mockWriteFile.mock.calls.filter( - (call: string[]) => typeof call[0] === "string" && call[0].endsWith("AGENTS.md"), - ); - expect(allWrites).toHaveLength(1); - expect(allWrites[0]![0]).toContain(".ao/AGENTS.md"); + // Plugin no longer writes wrappers — session-manager handles it. + // mkdir/writeFile/rename should not be called by the plugin. + expect(mockMkdir).not.toHaveBeenCalled(); + expect(mockWriteFile).not.toHaveBeenCalled(); + expect(mockRename).not.toHaveBeenCalled(); }); }); @@ -1974,22 +1740,20 @@ describe("setupWorkspaceHooks", () => { // Shell wrapper content verification // ========================================================================= describe("shell wrapper content", () => { - const agent = create(); - beforeEach(() => { // Force wrapper installation by making version marker miss mockReadFile.mockRejectedValue(new Error("ENOENT")); }); async function getWrapperContent(name: string): Promise { - await agent.setupWorkspaceHooks!("/workspace/test", { - dataDir: "/data", - sessionId: "sess-1", - }); + // Wrappers are now installed by session-manager via setupPathWrapperWorkspace. + // Import and call it directly to test wrapper content. + const { setupPathWrapperWorkspace } = await import("@aoagents/ao-core"); + await setupPathWrapperWorkspace("/workspace/test"); // With atomic writes, content is written to a .tmp. file const call = mockWriteFile.mock.calls.find( - (c: [string, string, object]) => typeof c[0] === "string" && c[0].includes(`/${name}.tmp.`), + (c: unknown[]) => typeof c[0] === "string" && (c[0] as string).includes(`/${name}.tmp.`), ); return call ? (call[1] as string) : ""; } @@ -2055,9 +1819,9 @@ describe("shell wrapper content", () => { expect(content).toContain("pr/create)"); }); - it("uses exec for non-PR commands (transparent passthrough)", async () => { + it("passes through non-PR commands to real gh", async () => { const content = await getWrapperContent("gh"); - expect(content).toContain('exec "$real_gh"'); + expect(content).toContain('"$real_gh" "$@"'); }); it("prefers GH_PATH when provided and executable", async () => { diff --git a/packages/plugins/agent-codex/src/index.ts b/packages/plugins/agent-codex/src/index.ts index ccea0b538..320bf5ad9 100644 --- a/packages/plugins/agent-codex/src/index.ts +++ b/packages/plugins/agent-codex/src/index.ts @@ -4,13 +4,10 @@ import { shellEscape, readLastJsonlEntry, normalizeAgentPermissionMode, - buildAgentPath, - setupPathWrapperWorkspace, readLastActivityEntry, checkActivityLogState, getActivityFallbackState, recordTerminalActivity, - PREFERRED_GH_PATH, type Agent, type AgentSessionInfo, type AgentLaunchConfig, @@ -487,11 +484,7 @@ function createCodexAgent(): Agent { env["AO_ISSUE_ID"] = config.issueId; } - // Prepend ~/.ao/bin to PATH so our gh/git wrappers intercept commands. - // The wrappers strip this directory from PATH before calling the real - // binary, so there's no infinite recursion. - env["PATH"] = buildAgentPath(process.env["PATH"]); - env["GH_PATH"] = PREFERRED_GH_PATH; + // PATH and GH_PATH are injected by session-manager for all agents. // Disable Codex's version check/update prompt for non-interactive AO sessions. env["CODEX_DISABLE_UPDATE_CHECK"] = "1"; @@ -743,11 +736,11 @@ function createCodexAgent(): Agent { return parts.join(" "); }, - async setupWorkspaceHooks(workspacePath: string, _config: WorkspaceHooksConfig): Promise { - await setupPathWrapperWorkspace(workspacePath); + async setupWorkspaceHooks(_workspacePath: string, _config: WorkspaceHooksConfig): Promise { + // PATH wrappers are installed by session-manager for all agents. }, - async postLaunchSetup(session: Session): Promise { + async postLaunchSetup(_session: Session): Promise { // Resolve binary path on first launch (cached for subsequent calls). // Uses a promise guard to prevent concurrent calls from racing. if (!resolvedBinary) { @@ -760,8 +753,7 @@ function createCodexAgent(): Agent { resolvingBinary = null; } } - if (!session.workspacePath) return; - await setupPathWrapperWorkspace(session.workspacePath); + // PATH wrappers are re-ensured by session-manager. }, }; } diff --git a/packages/plugins/agent-cursor/src/index.test.ts b/packages/plugins/agent-cursor/src/index.test.ts index 1c92f2bcb..3798287fb 100644 --- a/packages/plugins/agent-cursor/src/index.test.ts +++ b/packages/plugins/agent-cursor/src/index.test.ts @@ -508,14 +508,14 @@ describe("postLaunchSetup", () => { describe("getEnvironment PATH", () => { const agent = create(); - it("prepends ~/.ao/bin to PATH", () => { + it("does not set PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toMatch(/\.ao\/bin/); + expect(env["PATH"]).toBeUndefined(); }); - it("sets GH_PATH", () => { + it("does not set GH_PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["GH_PATH"]).toBe("/usr/local/bin/gh"); + expect(env["GH_PATH"]).toBeUndefined(); }); }); diff --git a/packages/plugins/agent-cursor/src/index.ts b/packages/plugins/agent-cursor/src/index.ts index f49139bd4..2eb5fb56e 100644 --- a/packages/plugins/agent-cursor/src/index.ts +++ b/packages/plugins/agent-cursor/src/index.ts @@ -1,13 +1,10 @@ import { shellEscape, normalizeAgentPermissionMode, - buildAgentPath, - setupPathWrapperWorkspace, readLastActivityEntry, checkActivityLogState, getActivityFallbackState, recordTerminalActivity, - PREFERRED_GH_PATH, DEFAULT_READY_THRESHOLD_MS, DEFAULT_ACTIVE_WINDOW_MS, type Agent, @@ -229,9 +226,7 @@ function createCursorAgent(): Agent { env["AO_ISSUE_ID"] = config.issueId; } - // Prepend ~/.ao/bin to PATH so our gh/git wrappers intercept commands. - env["PATH"] = buildAgentPath(process.env["PATH"]); - env["GH_PATH"] = PREFERRED_GH_PATH; + // PATH and GH_PATH are injected by session-manager for all agents. return env; }, @@ -394,13 +389,12 @@ function createCursorAgent(): Agent { return null; }, - async setupWorkspaceHooks(workspacePath: string, _config: WorkspaceHooksConfig): Promise { - await setupPathWrapperWorkspace(workspacePath); + async setupWorkspaceHooks(_workspacePath: string, _config: WorkspaceHooksConfig): Promise { + // PATH wrappers are installed by session-manager for all agents. }, - async postLaunchSetup(session: Session): Promise { - if (!session.workspacePath) return; - await setupPathWrapperWorkspace(session.workspacePath); + async postLaunchSetup(_session: Session): Promise { + // PATH wrappers are re-ensured by session-manager. }, }; } diff --git a/packages/plugins/agent-opencode/src/index.test.ts b/packages/plugins/agent-opencode/src/index.test.ts index 04cf42bcf..cecfecbc3 100644 --- a/packages/plugins/agent-opencode/src/index.test.ts +++ b/packages/plugins/agent-opencode/src/index.test.ts @@ -818,14 +818,14 @@ describe("postLaunchSetup", () => { describe("getEnvironment PATH", () => { const agent = create(); - it("prepends ~/.ao/bin to PATH", () => { + it("does not set PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["PATH"]).toMatch(/\.ao\/bin/); + expect(env["PATH"]).toBeUndefined(); }); - it("sets GH_PATH", () => { + it("does not set GH_PATH (injected by session-manager)", () => { const env = agent.getEnvironment(makeLaunchConfig()); - expect(env["GH_PATH"]).toBe("/usr/local/bin/gh"); + expect(env["GH_PATH"]).toBeUndefined(); }); }); diff --git a/packages/plugins/agent-opencode/src/index.ts b/packages/plugins/agent-opencode/src/index.ts index 44ff50bb2..3f0fcdbaf 100644 --- a/packages/plugins/agent-opencode/src/index.ts +++ b/packages/plugins/agent-opencode/src/index.ts @@ -2,13 +2,10 @@ import { DEFAULT_READY_THRESHOLD_MS, DEFAULT_ACTIVE_WINDOW_MS, shellEscape, - buildAgentPath, readLastActivityEntry, checkActivityLogState, getActivityFallbackState, recordTerminalActivity, - setupPathWrapperWorkspace, - PREFERRED_GH_PATH, asValidOpenCodeSessionId, type Agent, type AgentSessionInfo, @@ -276,9 +273,7 @@ function createOpenCodeAgent(): Agent { env["AO_ISSUE_ID"] = config.issueId; } - // Prepend ~/.ao/bin to PATH so our gh/git wrappers intercept commands. - env["PATH"] = buildAgentPath(process.env["PATH"]); - env["GH_PATH"] = PREFERRED_GH_PATH; + // PATH and GH_PATH are injected by session-manager for all agents. return env; }, @@ -437,13 +432,12 @@ function createOpenCodeAgent(): Agent { return parts.join(" "); }, - async setupWorkspaceHooks(workspacePath: string, _config: WorkspaceHooksConfig): Promise { - await setupPathWrapperWorkspace(workspacePath); + async setupWorkspaceHooks(_workspacePath: string, _config: WorkspaceHooksConfig): Promise { + // PATH wrappers are installed by session-manager for all agents. }, - async postLaunchSetup(session: Session): Promise { - if (!session.workspacePath) return; - await setupPathWrapperWorkspace(session.workspacePath); + async postLaunchSetup(_session: Session): Promise { + // PATH wrappers are re-ensured by session-manager. }, }; } diff --git a/packages/plugins/runtime-tmux/src/index.ts b/packages/plugins/runtime-tmux/src/index.ts index 96021d1eb..c296c6ba9 100644 --- a/packages/plugins/runtime-tmux/src/index.ts +++ b/packages/plugins/runtime-tmux/src/index.ts @@ -66,17 +66,29 @@ export function create(): Runtime { // Create tmux session in detached mode await tmux("new-session", "-d", "-s", sessionName, "-c", config.workspacePath, ...envArgs); + // Re-export PATH inside the launch script. macOS zsh runs path_helper + // during shell startup which resets PATH, wiping entries set via tmux -e. + // Including the export in the script file (not send-keys) avoids terminal + // buffer issues with long PATH values (1000+ chars). + const pathValue = config.environment?.["PATH"]; + let launchCommand = config.launchCommand; + if (pathValue) { + // Use printf with JSON-escaped value to avoid shell injection if + // PATH contains single quotes or other shell metacharacters. + launchCommand = `export PATH=$(printf '%s' ${JSON.stringify(pathValue)})\n${launchCommand}`; + } + // Send the launch command — clean up the session if this fails. // Use a temp script for long commands so the pane shows a short // invocation instead of a pasted wall of shell. try { - if (config.launchCommand.length > 200) { - const invocation = writeLaunchScript(config.launchCommand); + if (launchCommand.length > 200) { + const invocation = writeLaunchScript(launchCommand); await tmux("send-keys", "-t", sessionName, "-l", invocation); await sleep(300); await tmux("send-keys", "-t", sessionName, "Enter"); } else { - await tmux("send-keys", "-t", sessionName, config.launchCommand, "Enter"); + await tmux("send-keys", "-t", sessionName, launchCommand, "Enter"); } } catch (err: unknown) { try { diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index 31de90057..5fe45e57b 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -7,25 +7,49 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; -import type { - BatchObserver, - CICheck, - CIStatus, - PREnrichmentData, - PRInfo, - PRState, - ReviewDecision, +import { + execGhObserved, + type BatchObserver, + type CICheck, + type CIStatus, + type PREnrichmentData, + type PRInfo, + type PRState, + type ReviewDecision, } from "@aoagents/ao-core"; import { LRUCache } from "./lru-cache.js"; let execFileAsync = promisify(execFile); +let execGhAsync = async (args: string[], timeout: number, operation: string): Promise => + execGhObserved(args, { component: "scm-github-batch", operation }, timeout); /** * Set execFileAsync for testing. * Allows mocking the underlying execFile in unit tests. + * + * NOTE: This bypasses the gh tracer (execGhObserved). Tests that need to + * verify tracer behavior should mock execGhObserved directly or use + * setExecGhAsync instead. */ export function setExecFileAsync(fn: typeof execFileAsync): void { execFileAsync = fn; + execGhAsync = async (args: string[], timeout: number): Promise => { + const { stdout } = await execFileAsync("gh", args, { + maxBuffer: 10 * 1024 * 1024, + timeout, + }); + return stdout.trim(); + }; +} + +/** + * Set execGhAsync for testing — preserves tracer in the call chain. + * Use this when testing code that should exercise the traced execution path. + */ +export function setExecGhAsync( + fn: (args: string[], timeout: number, operation: string) => Promise, +): void { + execGhAsync = fn; } /** @@ -34,6 +58,7 @@ export function setExecFileAsync(fn: typeof execFileAsync): void { */ const MAX_PR_LIST_ETAGS = 100; // Number of repos to cache const MAX_COMMIT_STATUS_ETAGS = 500; // Number of commits to cache +const MAX_REVIEW_COMMENTS_ETAGS = 500; // Number of PRs to cache review ETags const MAX_PR_METADATA = 200; // Number of PRs to cache full data /** @@ -47,6 +72,7 @@ const MAX_PR_METADATA = 200; // Number of PRs to cache full data interface ETagCache { prList: LRUCache; // Key: "owner/repo", Value: ETag commitStatus: LRUCache; // Key: "owner/repo#sha", Value: ETag + reviewComments: LRUCache; // Key: "owner/repo#number", Value: ETag } /** @@ -59,6 +85,7 @@ interface ETagCache { const etagCache: ETagCache = { prList: new LRUCache(MAX_PR_LIST_ETAGS), commitStatus: new LRUCache(MAX_COMMIT_STATUS_ETAGS), + reviewComments: new LRUCache(MAX_REVIEW_COMMENTS_ETAGS), }; /** @@ -67,6 +94,15 @@ const etagCache: ETagCache = { interface ETagGuardResult { shouldRefresh: boolean; details: string[]; + /** Repos where Guard 1 returned 304 — no PR list changes, detectPR can be skipped. */ + prListUnchangedRepos: Set; +} + +/** Result of enrichSessionsPRBatch including Guard 1 PR-discovery info. */ +export interface BatchEnrichmentResult { + enrichment: Map; + /** Repos where Guard 1 returned 304 — safe to skip detectPR. */ + prListUnchangedRepos: Set; } /** @@ -76,6 +112,7 @@ interface ETagGuardResult { export function clearETagCache(): void { etagCache.prList.clear(); etagCache.commitStatus.clear(); + etagCache.reviewComments.clear(); } /** @@ -176,14 +213,11 @@ function updatePRMetadataCache( */ export async function shouldRefreshPREnrichment( prs: PRInfo[], + extraRepos: string[] = [], ): Promise { const details: string[] = []; let shouldRefresh = false; - if (prs.length === 0) { - return { shouldRefresh: false, details: ["No PRs to check"] }; - } - // Group PRs by repository for Guard 1 (PR list check) const repos = new Map(); @@ -198,8 +232,20 @@ export async function shouldRefreshPREnrichment( } } + // Include repos from PR-less sessions so Guard 1 runs for them too + for (const repoKey of extraRepos) { + if (!repos.has(repoKey)) { + repos.set(repoKey, []); + } + } + + if (repos.size === 0) { + return { shouldRefresh: false, details: ["No repos to check"], prListUnchangedRepos: new Set() }; + } + // Guard 1: Check PR list ETag for each repository let guard1DetectedChanges = false; + const prListUnchangedRepos = new Set(); for (const [repoKey] of repos) { const [owner, repo] = repoKey.split("/"); const prListChanged = await checkPRListETag(owner, repo); @@ -207,6 +253,8 @@ export async function shouldRefreshPREnrichment( guard1DetectedChanges = true; shouldRefresh = true; details.push(`PR list changed for ${repoKey} (Guard 1)`); + } else { + prListUnchangedRepos.add(repoKey); } } @@ -254,7 +302,7 @@ export async function shouldRefreshPREnrichment( } } - return { shouldRefresh, details }; + return { shouldRefresh, details, prListUnchangedRepos }; } /** @@ -322,6 +370,36 @@ async function verifyGhCLI(): Promise { */ export const MAX_BATCH_SIZE = 25; +/** + * Check if an HTTP response contains a 304 Not Modified status. + * Handles HTTP/1.1, HTTP/2, and HTTP/2.0 status lines. + */ +function is304(output: string): boolean { + return /HTTP\/[\d.]+ 304/i.test(output); +} + +/** + * Extract stdout/stderr from an execFile error object. + * gh cli puts the HTTP response in stdout even on exit code 1 (e.g. 304). + */ +function extractErrorOutput(err: unknown): string | null { + const e = err as { stdout?: unknown; stderr?: unknown }; + const stdout = typeof e.stdout === "string" ? e.stdout : ""; + const stderr = typeof e.stderr === "string" ? e.stderr : ""; + const combined = stdout + stderr; + return combined.length > 0 ? combined : null; +} + +/** + * Extract ETag from HTTP response output. + * Used on both 200 and 304 paths — RFC 7232 allows servers to rotate + * the validator on a 304, so we must re-read the ETag even when unchanged. + */ +function extractETag(output: string): string | undefined { + const match = output.match(/etag:\s*(.+)/i); + return match ? match[1].trim() : undefined; +} + /** * Guard 1: PR List ETag Check (per repo) * @@ -350,30 +428,34 @@ async function checkPRListETag( } try { - const { stdout } = await execFileAsync("gh", args, { timeout: 10_000 }); - const output = stdout.trim(); + const output = await execGhAsync(args, 10_000, "gh.api.guard-pr-list"); // Check for HTTP 304 Not Modified response - if (output.includes("HTTP/1.1 304") || output.includes("HTTP/2 304")) { - // No changes detected - cost: 0 GraphQL points + if (is304(output)) { + // Re-read ETag on 304 — RFC 7232 allows rotated validators + const rotatedETag = extractETag(output); + if (rotatedETag) setPRListETag(owner, repo, rotatedETag); return false; } // Extract new ETag from response headers - // ETag header format: "etag": "W/"abc123..." or "etag": "abc123..." - const etagMatch = output.match(/etag:\s*(.+)/i); - if (etagMatch) { - // Trim to remove trailing whitespace/newlines that could cause comparison issues - const newETag = etagMatch[1].trim(); + const newETag = extractETag(output); + if (newETag) { setPRListETag(owner, repo, newETag); } // PR list changed - cost: 1 REST point return true; } catch (err) { - // On error, assume change to ensure we don't miss anything + // gh exits code 1 on 304 Not Modified — check stdout/stderr for the status line + const output = extractErrorOutput(err); + if (output && is304(output)) { + const rotatedETag = extractETag(output); + if (rotatedETag) setPRListETag(owner, repo, rotatedETag); + return false; + } + const errorMsg = err instanceof Error ? err.message : String(err); - // Log but don't throw - allow GraphQL batch to proceed // eslint-disable-next-line no-console -- Observability logging for ETag errors console.warn(`[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`); return true; // Assume changed to be safe @@ -381,13 +463,15 @@ async function checkPRListETag( } /** - * Guard 2: Commit Status ETag Check (per PR with pending CI) + * Guard 2: Check-Runs ETag Check (per PR with cached head SHA) * * Detects if CI status has changed for a specific commit using REST ETag. * - * - Endpoint: GET /repos/{owner}/{repo}/commits/{head_sha}/status + * - Endpoint: GET /repos/{owner}/{repo}/commits/{head_sha}/check-runs + * Uses the check-runs endpoint (not legacy /status) because the batch + * query reads `statusCheckRollup` which aggregates check-runs. Pure-Actions + * repos only update check-runs, not the legacy combined-status endpoint. * - Detects: CI check starts, passes, fails, or external status updates - * - Only checked for PRs with ciStatus === "pending" to minimize calls * * @returns true if CI status has changed (200 OK), false if unchanged (304 Not Modified) */ @@ -399,8 +483,9 @@ async function checkCommitStatusETag( const commitKey = `${owner}/${repo}#${sha}`; const cachedETag = etagCache.commitStatus.get(commitKey); - // Build gh CLI args for REST API call - const url = `repos/${owner}/${repo}/commits/${sha}/status`; + // Use check-runs endpoint (not legacy /status) to match statusCheckRollup + // data source. per_page=1 keeps the response small — we only need the ETag. + const url = `repos/${owner}/${repo}/commits/${sha}/check-runs?per_page=1`; const args = ["api", "--method", "GET", url, "-i"]; // -i includes headers // Add If-None-Match header if we have a cached ETag @@ -409,27 +494,32 @@ async function checkCommitStatusETag( } try { - const { stdout } = await execFileAsync("gh", args, { timeout: 10_000 }); - const output = stdout.trim(); + const output = await execGhAsync(args, 10_000, "gh.api.guard-commit-status"); // Check for HTTP 304 Not Modified response - if (output.includes("HTTP/1.1 304") || output.includes("HTTP/2 304")) { - // No CI changes detected - cost: 0 GraphQL points + if (is304(output)) { + const rotatedETag = extractETag(output); + if (rotatedETag) setCommitStatusETag(owner, repo, sha, rotatedETag); return false; } // Extract new ETag from response headers - const etagMatch = output.match(/etag:\s*(.+)/i); - if (etagMatch) { - // Trim to remove trailing whitespace/newlines that could cause comparison issues - const newETag = etagMatch[1].trim(); + const newETag = extractETag(output); + if (newETag) { setCommitStatusETag(owner, repo, sha, newETag); } // CI status changed - cost: 1 REST point return true; } catch (err) { - // On error, assume change to ensure we don't miss anything + // gh exits code 1 on 304 Not Modified — check stdout/stderr for the status line + const output = extractErrorOutput(err); + if (output && is304(output)) { + const rotatedETag = extractETag(output); + if (rotatedETag) setCommitStatusETag(owner, repo, sha, rotatedETag); + return false; + } + const errorMsg = err instanceof Error ? err.message : String(err); // eslint-disable-next-line no-console -- Observability logging for ETag errors console.warn( @@ -439,6 +529,66 @@ async function checkCommitStatusETag( } } +/** + * Guard 3: Review Comments ETag Check (per PR) + * + * Detects if inline review comments have changed on a PR. + * Used to gate the getReviewThreads GraphQL call — if no new comments + * exist (304), the cached result is reused without a GraphQL call. + * + * - Endpoint: GET /repos/{owner}/{repo}/pulls/{number}/comments + * No per_page limit — the ETag covers the full resource. With per_page=1, + * the ETag only covers the first page, so new comments on page 2+ would + * never bust the validator. Typical PR comment counts are small (<100) + * so the unbounded list is fine. + * - Detects: New review comments, edited comments, deleted comments + * - Cost: 0 REST points on 304, 1 REST point on 200 + */ +export async function checkReviewCommentsETag( + owner: string, + repo: string, + prNumber: number, +): Promise { + const cacheKey = `${owner}/${repo}#${prNumber}`; + const cachedETag = etagCache.reviewComments.get(cacheKey); + + const url = `repos/${owner}/${repo}/pulls/${prNumber}/comments`; + const args = ["api", "--method", "GET", url, "-i"]; + + if (cachedETag) { + args.push("-H", `If-None-Match: ${cachedETag}`); + } + + try { + const output = await execGhAsync(args, 10_000, "gh.api.guard-review-comments"); + + if (is304(output)) { + const rotatedETag = extractETag(output); + if (rotatedETag) etagCache.reviewComments.set(cacheKey, rotatedETag); + return false; + } + + const newETag = extractETag(output); + if (newETag) { + etagCache.reviewComments.set(cacheKey, newETag); + } + + return true; + } catch (err) { + const output = extractErrorOutput(err); + if (output && is304(output)) { + const rotatedETag = extractETag(output); + if (rotatedETag) etagCache.reviewComments.set(cacheKey, rotatedETag); + return false; + } + + const errorMsg = err instanceof Error ? err.message : String(err); + // eslint-disable-next-line no-console -- Observability logging for ETag errors + console.warn(`[ETag Guard 3] Review comments check failed for ${cacheKey}: ${errorMsg}`); + return true; // Assume changed to be safe + } +} + /** * GraphQL fields to fetch for each PR. * This includes all data needed for orchestrator status detection. @@ -455,19 +605,16 @@ const PR_FIELDS = ` reviewDecision headRefName headRefOid - reviews(last: 5) { - nodes { - author { login } - state - submittedAt - } - } commits(last: 1) { nodes { commit { statusCheckRollup { state - contexts(first: 20) { + # 11 keeps per-PR node cost under budget for 25-PR batch queries + # (total cost ≤5000). Repos with >11 checks lose individual check + # visibility, but the rollup "state" still reflects all checks — + # overall pass/fail detection remains correct. + contexts(first: 11) { nodes { ... on CheckRun { name @@ -534,6 +681,7 @@ export function generateBatchQuery(prs: PRInfo[]): { return { query: `query BatchPRs(${variableDefs}) { ${selections.join("\n")} + rateLimit { cost remaining resetAt } }`, variables, }; @@ -567,22 +715,32 @@ async function executeBatchQuery( } } - const args = ["api", "graphql", ...varArgs, "-f", `query=${query}`]; + const args = ["api", "graphql", "-i", ...varArgs, "-f", `query=${query}`]; // Scale timeout based on batch size to prevent large batches from timing out // Base: 30s, +2s per PR beyond first 10 const batchSize = prs.length; const adaptiveTimeout = 30_000 + Math.max(0, (batchSize - 10) * 2000); - const { stdout } = await execFileAsync("gh", args, { - maxBuffer: 10 * 1024 * 1024, - timeout: adaptiveTimeout, - }); + const stdout = await execGhAsync(args, adaptiveTimeout, "gh.api.graphql-batch"); + + // With -i, stdout contains HTTP headers + blank line + JSON body. + // Split at first blank line to get the JSON body for parsing. + // The tracer (execGhObserved) already parses the headers for its trace row. + const blankLineIdx = stdout.indexOf("\r\n\r\n"); + const altBlankLineIdx = stdout.indexOf("\n\n"); + const splitIdx = + blankLineIdx >= 0 && (altBlankLineIdx < 0 || blankLineIdx < altBlankLineIdx) + ? blankLineIdx + 4 + : altBlankLineIdx >= 0 + ? altBlankLineIdx + 2 + : 0; + const body = splitIdx > 0 ? stdout.slice(splitIdx) : stdout; const result: { data?: Record; errors?: Array<{ message: string; path?: string[] }>; - } = JSON.parse(stdout.trim()); + } = JSON.parse(body.trim()); // Check for GraphQL errors and throw to allow individual API fallback if (result.errors && result.errors.length > 0) { @@ -760,7 +918,6 @@ function extractPREnrichment( if ( pr["state"] === undefined && pr["title"] === undefined && - pr["reviews"] === undefined && pr["commits"] === undefined ) { return null; @@ -882,15 +1039,17 @@ function extractPREnrichment( export async function enrichSessionsPRBatch( prs: PRInfo[], observer?: BatchObserver, -): Promise> { + repos: string[] = [], +): Promise { const result = new Map(); - if (prs.length === 0) { - return result; - } - // Step 1: Check if we need to refresh using 2-Guard ETag Strategy - const guardResult = await shouldRefreshPREnrichment(prs); + // Guard 1 runs for all repos (including those with no PRs yet) so the + // lifecycle manager knows whether detectPR can be skipped. + const guardResult = await shouldRefreshPREnrichment(prs, repos); + + // Report which repos had no PR list changes so the lifecycle can skip detectPR + observer?.reportPRListUnchangedRepos?.(guardResult.prListUnchangedRepos); if (!guardResult.shouldRefresh) { // No changes detected - try to return cached data @@ -913,7 +1072,7 @@ export async function enrichSessionsPRBatch( "info", `[ETag Guard] Skipping GraphQL batch - all ${result.size} PRs cached. Reasons: ${guardResult.details.join(", ")}`, ); - return result; + return { enrichment: result, prListUnchangedRepos: guardResult.prListUnchangedRepos }; } // Some PRs not cached - fetch missing PRs via GraphQL @@ -1006,7 +1165,7 @@ export async function enrichSessionsPRBatch( } } - return result; + return { enrichment: result, prListUnchangedRepos: guardResult.prListUnchangedRepos }; } // Export internal functions for testing diff --git a/packages/plugins/scm-github/src/index.ts b/packages/plugins/scm-github/src/index.ts index 518c419c4..7f05b9272 100644 --- a/packages/plugins/scm-github/src/index.ts +++ b/packages/plugins/scm-github/src/index.ts @@ -9,6 +9,7 @@ import { createHmac, timingSafeEqual } from "node:crypto"; import { promisify } from "node:util"; import { CI_STATUS, + execGhObserved, type PluginModule, type SCM, type SCMWebhookEvent, @@ -24,13 +25,15 @@ import { type Review, type ReviewDecision, type ReviewComment, - type AutomatedComment, + type ReviewSummary, + type ReviewThreadsResult, type MergeReadiness, type PREnrichmentData, type BatchObserver, } from "@aoagents/ao-core"; import { enrichSessionsPRBatch as enrichSessionsPRBatchImpl, + checkReviewCommentsETag, } from "./graphql-batch.js"; import { getWebhookHeader, @@ -77,11 +80,11 @@ async function execCli(bin: ExecCommand, args: string[], cwd?: string): Promise< } async function gh(args: string[]): Promise { - return execCli("gh", args); + return execGhObserved(args, { component: "scm-github" }, 30_000); } async function ghInDir(args: string[], cwd: string): Promise { - return execCli("gh", args, cwd); + return execGhObserved(args, { component: "scm-github", cwd }, 30_000); } async function git(args: string[], cwd: string): Promise { @@ -450,7 +453,89 @@ function parseDate(val: string | undefined | null): Date { // SCM implementation // --------------------------------------------------------------------------- +// In-process PR cache. Per-method TTLs balance call reduction against +// staleness. Tightest TTLs (5s) on the fastest-changing decision-critical +// fields (state, CI, mergeability) — well under one poll cycle. Slightly +// looser (10s) on review-state and review-comments which tolerate up to +// 10-30s staleness per the agreed policy and benefit measurably from a +// looser window in trace replay. detectPR uses 30s because once a PR is +// discovered for a branch, that fact is stable for the session — and 5s was +// far below the per-branch poll cadence (~30s), making the cache near-useless. +// detectPR caches positive results only (never []) so a freshly created PR +// is discovered on the very next poll. +const PR_CACHE_TTL_MS = { + resolvePR: 60_000, // identity metadata (number, url, title, branch refs, isDraft) + getPRState: 5_000, // open / merged / closed + getPRSummary: 5_000, // state + title + additions/deletions + getReviews: 10_000, // review array (state, body, author) + getReviewDecision: 10_000, // approved / changes_requested / pending + getCIChecks: 5_000, // CI check list (name, state, link, timestamps) + getMergeability: 5_000, // composite merge readiness + getPendingComments: 10_000, // unresolved review threads (GraphQL) + detectPR: 30_000, // positive hits only — branch-PR mapping is stable once known +} as const; + +const PR_CACHE_MAX_ENTRIES = 1000; + +type PRCacheMethod = keyof typeof PR_CACHE_TTL_MS; + function createGitHubSCM(): SCM { + // Per-instance cache so each createGitHubSCM() returns an isolated cache — + // tests get clean state on each create() call. + const prCache = new Map(); + // ETag-controlled cache for review threads + reviews. Freshness is managed by + // Guard 3 (checkReviewCommentsETag) — not a TTL timer. + const reviewThreadsCache = new Map(); + + function prCacheKey(owner: string, repo: string, prKey: string, method: PRCacheMethod): string { + return `${owner}/${repo}#${prKey}:${method}`; + } + + function readPRCache(key: string): T | null { + const entry = prCache.get(key); + if (!entry) return null; + if (Date.now() > entry.expiresAt) { + prCache.delete(key); + return null; + } + return entry.value as T; + } + + function writePRCache(key: string, value: T, ttlMs: number): void { + if (prCache.size >= PR_CACHE_MAX_ENTRIES) { + const oldest = prCache.keys().next().value; + if (oldest !== undefined) prCache.delete(oldest); + } + prCache.set(key, { value, expiresAt: Date.now() + ttlMs }); + } + + // Wipe every method's cache entry for a specific PR. Called on writes + // (pr edit/merge/close) to avoid serving stale state after our own mutation. + // Also wipes the branch-keyed detectPR entry since mergePR deletes the branch. + function invalidatePRCache(pr: PRInfo): void { + const prefix = `${pr.owner}/${pr.repo}#${pr.number}:`; + for (const key of prCache.keys()) { + if (key.startsWith(prefix)) prCache.delete(key); + } + prCache.delete(prCacheKey(pr.owner, pr.repo, pr.branch, "detectPR")); + reviewThreadsCache.delete(`${pr.owner}/${pr.repo}#${pr.number}`); + } + + async function withPRCache( + owner: string, + repo: string, + prKey: string, + method: PRCacheMethod, + fetcher: () => Promise, + ): Promise { + const key = prCacheKey(owner, repo, prKey, method); + const cached = readPRCache(key); + if (cached !== null) return cached; + const value = await fetcher(); + writePRCache(key, value, PR_CACHE_TTL_MS[method]); + return value; + } + return { name: "github", @@ -517,6 +602,13 @@ function createGitHubSCM(): SCM { async detectPR(session: Session, project: ProjectConfig): Promise { if (!session.branch || !project.repo) return null; parseProjectRepo(project.repo); + const [owner, repoName] = project.repo.split("/"); + // Positive-only cache: never cache [] (null). A just-created PR must + // surface on the next poll, so we pay the gh call for misses but save + // every call after the PR is discovered. + const cacheK = prCacheKey(owner ?? "", repoName ?? "", session.branch, "detectPR"); + const cached = readPRCache(cacheK); + if (cached !== null) return cached; try { const raw = await gh([ "pr", @@ -542,7 +634,9 @@ function createGitHubSCM(): SCM { if (prs.length === 0) return null; - return prInfoFromView(prs[0], project.repo); + const info = prInfoFromView(prs[0], project.repo); + writePRCache(cacheK, info, PR_CACHE_TTL_MS.detectPR); + return info; } catch { return null; } @@ -552,30 +646,38 @@ function createGitHubSCM(): SCM { if (!project.repo) { throw new Error("Cannot resolve PR: project has no repo configured"); } - const raw = await gh([ - "pr", - "view", - reference, - "--repo", - project.repo, - "--json", - "number,url,title,headRefName,baseRefName,isDraft", - ]); + const repo = project.repo; + const [owner, repoName] = repo.split("/"); + // Cache by reference (number, branch, or URL — caller-provided). + // Identity metadata (number, url, title, branch refs, isDraft) is stable + // for the life of a PR; 60s TTL is safely under any user-noticeable window. + return withPRCache(owner ?? "", repoName ?? "", `ref=${reference}`, "resolvePR", async () => { + const raw = await gh([ + "pr", + "view", + reference, + "--repo", + repo, + "--json", + "number,url,title,headRefName,baseRefName,isDraft", + ]); - const data: { - number: number; - url: string; - title: string; - headRefName: string; - baseRefName: string; - isDraft: boolean; - } = JSON.parse(raw); + const data: { + number: number; + url: string; + title: string; + headRefName: string; + baseRefName: string; + isDraft: boolean; + } = JSON.parse(raw); - return prInfoFromView(data, project.repo); + return prInfoFromView(data, repo); + }); }, async assignPRToCurrentUser(pr: PRInfo): Promise { await gh(["pr", "edit", String(pr.number), "--repo", repoFlag(pr), "--add-assignee", "@me"]); + invalidatePRCache(pr); }, async checkoutPR(pr: PRInfo, workspacePath: string): Promise { @@ -594,96 +696,113 @@ function createGitHubSCM(): SCM { }, async getPRState(pr: PRInfo): Promise { - const raw = await gh([ - "pr", - "view", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "state", - ]); - const data: { state: string } = JSON.parse(raw); - const s = data.state.toUpperCase(); - if (s === "MERGED") return "merged"; - if (s === "CLOSED") return "closed"; - return "open"; + // 5s TTL — state is decision-influencing (lifecycle uses it for cleanup), + // but 5s is well under one poll cycle so the lifecycle worker still sees + // freshly observed transitions on its next pass. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getPRState", async () => { + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "state", + ]); + const data: { state: string } = JSON.parse(raw); + const s = data.state.toUpperCase(); + if (s === "MERGED") return "merged"; + if (s === "CLOSED") return "closed"; + return "open"; + }); }, async getPRSummary(pr: PRInfo) { - const raw = await gh([ - "pr", - "view", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "state,title,additions,deletions", - ]); - const data: { - state: string; - title: string; - additions: number; - deletions: number; - } = JSON.parse(raw); - const s = data.state.toUpperCase(); - const state: PRState = s === "MERGED" ? "merged" : s === "CLOSED" ? "closed" : "open"; - return { - state, - title: data.title ?? "", - additions: data.additions ?? 0, - deletions: data.deletions ?? 0, - }; + // 5s TTL — includes state, so same freshness contract as getPRState. + // Title and additions/deletions change rarely; they ride along. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getPRSummary", async () => { + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "state,title,additions,deletions", + ]); + const data: { + state: string; + title: string; + additions: number; + deletions: number; + } = JSON.parse(raw); + const s = data.state.toUpperCase(); + const state: PRState = s === "MERGED" ? "merged" : s === "CLOSED" ? "closed" : "open"; + return { + state, + title: data.title ?? "", + additions: data.additions ?? 0, + deletions: data.deletions ?? 0, + }; + }); }, async mergePR(pr: PRInfo, method: MergeMethod = "squash"): Promise { const flag = method === "rebase" ? "--rebase" : method === "merge" ? "--merge" : "--squash"; await gh(["pr", "merge", String(pr.number), "--repo", repoFlag(pr), flag, "--delete-branch"]); + invalidatePRCache(pr); }, async closePR(pr: PRInfo): Promise { await gh(["pr", "close", String(pr.number), "--repo", repoFlag(pr)]); + invalidatePRCache(pr); }, async getCIChecks(pr: PRInfo): Promise { - try { - const raw = await gh([ - "pr", - "checks", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "name,state,link,startedAt,completedAt", - ]); + // 5s TTL — CI state can flip quickly; within one poll cycle is acceptable + // per the agreed fast-changing-fields policy. Fallback to statusCheckRollup + // for older gh CLI versions happens inside the fetcher and rides on the + // same cache entry. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getCIChecks", async () => { + try { + const raw = await gh([ + "pr", + "checks", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "name,state,link,startedAt,completedAt", + ]); - const checks: Array<{ - name: string; - state: string; - link: string; - startedAt: string; - completedAt: string; - }> = JSON.parse(raw); + const checks: Array<{ + name: string; + state: string; + link: string; + startedAt: string; + completedAt: string; + }> = JSON.parse(raw); - return checks.map((c) => { - const state = c.state?.toUpperCase(); + return checks.map((c) => { + const state = c.state?.toUpperCase(); - return { - name: c.name, - status: mapRawCheckStateToStatus(state), - url: c.link || undefined, - conclusion: state || undefined, - startedAt: c.startedAt ? new Date(c.startedAt) : undefined, - completedAt: c.completedAt ? new Date(c.completedAt) : undefined, - }; - }); - } catch (err) { - if (isUnsupportedPrChecksJsonError(err)) { - return getCIChecksFromStatusRollup(pr); + return { + name: c.name, + status: mapRawCheckStateToStatus(state), + url: c.link || undefined, + conclusion: state || undefined, + startedAt: c.startedAt ? new Date(c.startedAt) : undefined, + completedAt: c.completedAt ? new Date(c.completedAt) : undefined, + }; + }); + } catch (err) { + if (isUnsupportedPrChecksJsonError(err)) { + return getCIChecksFromStatusRollup(pr); + } + throw new Error("Failed to fetch CI checks", { cause: err }); } - throw new Error("Failed to fetch CI checks", { cause: err }); - } + }); }, async getCISummary(pr: PRInfo): Promise { @@ -721,65 +840,78 @@ function createGitHubSCM(): SCM { }, async getReviews(pr: PRInfo): Promise { - const raw = await gh([ - "pr", - "view", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "reviews", - ]); - const data: { - reviews: Array<{ - author: { login: string }; - state: string; - body: string; - submittedAt: string; - }>; - } = JSON.parse(raw); + // 5s TTL — review array. Reviewers are async, so the lifecycle worker + // sees a new review on its next poll cycle within 5s of the cache expiring. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getReviews", async () => { + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "reviews", + ]); + const data: { + reviews: Array<{ + author: { login: string }; + state: string; + body: string; + submittedAt: string; + }>; + } = JSON.parse(raw); - return data.reviews.map((r) => { - let state: Review["state"]; - const s = r.state?.toUpperCase(); - if (s === "APPROVED") state = "approved"; - else if (s === "CHANGES_REQUESTED") state = "changes_requested"; - else if (s === "DISMISSED") state = "dismissed"; - else if (s === "PENDING") state = "pending"; - else state = "commented"; + return data.reviews.map((r) => { + let state: Review["state"]; + const s = r.state?.toUpperCase(); + if (s === "APPROVED") state = "approved"; + else if (s === "CHANGES_REQUESTED") state = "changes_requested"; + else if (s === "DISMISSED") state = "dismissed"; + else if (s === "PENDING") state = "pending"; + else state = "commented"; - return { - author: r.author?.login ?? "unknown", - state, - body: r.body || undefined, - submittedAt: parseDate(r.submittedAt), - }; + return { + author: r.author?.login ?? "unknown", + state, + body: r.body || undefined, + submittedAt: parseDate(r.submittedAt), + }; + }); }); }, async getReviewDecision(pr: PRInfo): Promise { - const raw = await gh([ - "pr", - "view", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "reviewDecision", - ]); - const data: { reviewDecision: string } = JSON.parse(raw); + // 5s TTL — review decision is decision-influencing (gates merge), kept + // tight so a fresh "approved" surfaces within one poll cycle. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getReviewDecision", async () => { + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "reviewDecision", + ]); + const data: { reviewDecision: string } = JSON.parse(raw); - const d = (data.reviewDecision ?? "").toUpperCase(); - if (d === "APPROVED") return "approved"; - if (d === "CHANGES_REQUESTED") return "changes_requested"; - if (d === "REVIEW_REQUIRED") return "pending"; - return "none"; + const d = (data.reviewDecision ?? "").toUpperCase(); + if (d === "APPROVED") return "approved"; + if (d === "CHANGES_REQUESTED") return "changes_requested"; + if (d === "REVIEW_REQUIRED") return "pending"; + return "none"; + }); }, async getPendingComments(pr: PRInfo): Promise { - try { - // Use GraphQL with variables to get review threads with actual isResolved status - const raw = await gh([ + // 5s TTL — review threads are decision-influencing (gates whether AO + // reacts to new comments). Within one poll cycle is acceptable. Note: + // ETag does not work on /graphql per Experiment 2 (G2), so TTL is the + // only practical lever here. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getPendingComments", async () => { + try { + // Use GraphQL with variables to get review threads with actual isResolved status + const raw = await gh([ "api", "graphql", "-f", @@ -794,6 +926,7 @@ function createGitHubSCM(): SCM { pullRequest(number: $number) { reviewThreads(first: 100) { nodes { + id isResolved comments(first: 1) { nodes { @@ -819,6 +952,7 @@ function createGitHubSCM(): SCM { pullRequest: { reviewThreads: { nodes: Array<{ + id: string; isResolved: boolean; comments: { nodes: Array<{ @@ -852,6 +986,7 @@ function createGitHubSCM(): SCM { const c = t.comments.nodes[0]; return { id: c.id, + threadId: t.id, author: c.author?.login ?? "unknown", body: c.body, path: c.path || undefined, @@ -861,171 +996,234 @@ function createGitHubSCM(): SCM { url: c.url, }; }); - } catch (err) { - throw new Error("Failed to fetch pending comments", { cause: err }); - } + } catch (err) { + throw new Error("Failed to fetch pending comments", { cause: err }); + } + }); }, - async getAutomatedComments(pr: PRInfo): Promise { + async getReviewThreads(pr: PRInfo): Promise { + const cacheKey = `${pr.owner}/${pr.repo}#${pr.number}`; + + // Guard 3: check if review comments changed via REST ETag + const reviewsChanged = await checkReviewCommentsETag(pr.owner, pr.repo, pr.number); + if (!reviewsChanged) { + const cached = reviewThreadsCache.get(cacheKey); + if (cached) return cached; + } + try { - const perPage = 100; - const comments: Array<{ - id: number; - user: { login: string }; - body: string; - path: string; - line: number | null; - original_line: number | null; - created_at: string; - html_url: string; - }> = []; - - for (let page = 1; ; page++) { - const raw = await gh([ - "api", - "--method", - "GET", - `repos/${repoFlag(pr)}/pulls/${pr.number}/comments?per_page=${perPage}&page=${page}`, - ]); - const pageComments: Array<{ - id: number; - user: { login: string }; - body: string; - path: string; - line: number | null; - original_line: number | null; - created_at: string; - html_url: string; - }> = JSON.parse(raw); - - if (pageComments.length === 0) { - break; - } - - comments.push(...pageComments); - if (pageComments.length < perPage) { - break; - } - } - - return comments - .filter((c) => BOT_AUTHORS.has(c.user?.login ?? "")) - .map((c) => { - // Determine severity from body content - let severity: AutomatedComment["severity"] = "info"; - const bodyLower = c.body.toLowerCase(); - if ( - bodyLower.includes("error") || - bodyLower.includes("bug") || - bodyLower.includes("critical") || - bodyLower.includes("potential issue") - ) { - severity = "error"; - } else if ( - bodyLower.includes("warning") || - bodyLower.includes("suggest") || - bodyLower.includes("consider") - ) { - severity = "warning"; + const rawWithHeaders = await gh([ + "api", + "graphql", + "-i", + "-f", + `owner=${pr.owner}`, + "-f", + `name=${pr.repo}`, + "-F", + `number=${pr.number}`, + "-f", + `query=query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(last: 100) { + nodes { + id + isResolved + comments(first: 1) { + nodes { + id + author { login } + body + path + line + url + createdAt + } + } + } + } + reviews(last: 5) { + nodes { + author { login } + state + body + submittedAt + } + } + } } + rateLimit { cost remaining resetAt } + }`, + ]); + // Strip HTTP headers from -i response to get JSON body + const raw = rawWithHeaders.replace(/^[\s\S]*?\r?\n\r?\n/, ""); + const data: { + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: Array<{ + id: string; + isResolved: boolean; + comments: { + nodes: Array<{ + id: string; + author: { login: string } | null; + body: string; + path: string | null; + line: number | null; + url: string; + createdAt: string; + }>; + }; + }>; + }; + reviews: { + nodes: Array<{ + author: { login: string } | null; + state: string; + body: string; + submittedAt: string; + }>; + }; + }; + }; + }; + } = JSON.parse(raw); + + const threadNodes = data.data.repository.pullRequest.reviewThreads.nodes; + const reviewNodes = data.data.repository.pullRequest.reviews.nodes; + + const threads: ReviewComment[] = threadNodes + .filter((t) => { + if (t.isResolved) return false; + const c = t.comments.nodes[0]; + return !!c; + }) + .map((t) => { + const c = t.comments.nodes[0]; + const author = c.author?.login ?? "unknown"; return { - id: String(c.id), - botName: c.user?.login ?? "unknown", + id: c.id, + threadId: t.id, + author, body: c.body, path: c.path || undefined, - line: c.line ?? c.original_line ?? undefined, - severity, - createdAt: parseDate(c.created_at), - url: c.html_url, + line: c.line ?? undefined, + isResolved: t.isResolved, + createdAt: parseDate(c.createdAt), + url: c.url, + isBot: BOT_AUTHORS.has(author), }; }); + + const reviews: ReviewSummary[] = reviewNodes + .filter((r) => r.body && r.body.trim().length > 0) + .map((r) => ({ + author: r.author?.login ?? "unknown", + state: r.state, + body: r.body, + submittedAt: parseDate(r.submittedAt), + })); + + const result: ReviewThreadsResult = { threads, reviews }; + reviewThreadsCache.set(cacheKey, result); + return result; } catch (err) { - throw new Error("Failed to fetch automated comments", { cause: err }); + throw new Error("Failed to fetch review threads", { cause: err }); } }, async getMergeability(pr: PRInfo): Promise { - const blockers: string[] = []; + // 5s TTL — composite merge readiness. Internal getPRState/getCISummary + // calls are also cached (5s each) so even on cache miss this is cheap. + // Cached entry covers the full computed result so duplicate poll-cycle + // calls don't re-derive blockers. + return withPRCache(pr.owner, pr.repo, String(pr.number), "getMergeability", async () => { + const blockers: string[] = []; + + // First, check if the PR is merged + // GitHub returns mergeable=null for merged PRs, which is not useful + // Note: We only skip checks for merged PRs. Closed PRs still need accurate status. + const state = await this.getPRState(pr); + if (state === "merged") { + // For merged PRs, return a clean result without querying mergeable status + return { + mergeable: true, + ciPassing: true, + approved: true, + noConflicts: true, + blockers: [], + }; + } + + // Fetch PR details with merge state + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "mergeable,reviewDecision,mergeStateStatus,isDraft", + ]); + + const data: { + mergeable: string; + reviewDecision: string; + mergeStateStatus: string; + isDraft: boolean; + } = JSON.parse(raw); + + // CI + const ciStatus = await this.getCISummary(pr); + const ciPassing = ciStatus === CI_STATUS.PASSING || ciStatus === CI_STATUS.NONE; + if (!ciPassing) { + blockers.push(`CI is ${ciStatus}`); + } + + // Reviews + const reviewDecision = (data.reviewDecision ?? "").toUpperCase(); + const approved = reviewDecision === "APPROVED"; + if (reviewDecision === "CHANGES_REQUESTED") { + blockers.push("Changes requested in review"); + } else if (reviewDecision === "REVIEW_REQUIRED") { + blockers.push("Review required"); + } + + // Conflicts / merge state + const mergeable = (data.mergeable ?? "").toUpperCase(); + const mergeState = (data.mergeStateStatus ?? "").toUpperCase(); + const noConflicts = mergeable === "MERGEABLE"; + if (mergeable === "CONFLICTING") { + blockers.push("Merge conflicts"); + } else if (mergeable === "UNKNOWN" || mergeable === "") { + blockers.push("Merge status unknown (GitHub is computing)"); + } + if (mergeState === "BEHIND") { + blockers.push("Branch is behind base branch"); + } else if (mergeState === "BLOCKED") { + blockers.push("Merge is blocked by branch protection"); + } else if (mergeState === "UNSTABLE") { + blockers.push("Required checks are failing"); + } + + // Draft + if (data.isDraft) { + blockers.push("PR is still a draft"); + } - // First, check if the PR is merged - // GitHub returns mergeable=null for merged PRs, which is not useful - // Note: We only skip checks for merged PRs. Closed PRs still need accurate status. - const state = await this.getPRState(pr); - if (state === "merged") { - // For merged PRs, return a clean result without querying mergeable status return { - mergeable: true, - ciPassing: true, - approved: true, - noConflicts: true, - blockers: [], + mergeable: blockers.length === 0, + ciPassing, + approved, + noConflicts, + blockers, }; - } - - // Fetch PR details with merge state - const raw = await gh([ - "pr", - "view", - String(pr.number), - "--repo", - repoFlag(pr), - "--json", - "mergeable,reviewDecision,mergeStateStatus,isDraft", - ]); - - const data: { - mergeable: string; - reviewDecision: string; - mergeStateStatus: string; - isDraft: boolean; - } = JSON.parse(raw); - - // CI - const ciStatus = await this.getCISummary(pr); - const ciPassing = ciStatus === CI_STATUS.PASSING || ciStatus === CI_STATUS.NONE; - if (!ciPassing) { - blockers.push(`CI is ${ciStatus}`); - } - - // Reviews - const reviewDecision = (data.reviewDecision ?? "").toUpperCase(); - const approved = reviewDecision === "APPROVED"; - if (reviewDecision === "CHANGES_REQUESTED") { - blockers.push("Changes requested in review"); - } else if (reviewDecision === "REVIEW_REQUIRED") { - blockers.push("Review required"); - } - - // Conflicts / merge state - const mergeable = (data.mergeable ?? "").toUpperCase(); - const mergeState = (data.mergeStateStatus ?? "").toUpperCase(); - const noConflicts = mergeable === "MERGEABLE"; - if (mergeable === "CONFLICTING") { - blockers.push("Merge conflicts"); - } else if (mergeable === "UNKNOWN" || mergeable === "") { - blockers.push("Merge status unknown (GitHub is computing)"); - } - if (mergeState === "BEHIND") { - blockers.push("Branch is behind base branch"); - } else if (mergeState === "BLOCKED") { - blockers.push("Merge is blocked by branch protection"); - } else if (mergeState === "UNSTABLE") { - blockers.push("Required checks are failing"); - } - - // Draft - if (data.isDraft) { - blockers.push("PR is still a draft"); - } - - return { - mergeable: blockers.length === 0, - ciPassing, - approved, - noConflicts, - blockers, - }; + }); }, /** @@ -1041,8 +1239,10 @@ function createGitHubSCM(): SCM { async enrichSessionsPRBatch( prs: PRInfo[], observer?: BatchObserver, + repos?: string[], ): Promise> { - return enrichSessionsPRBatchImpl(prs, observer); + const batchResult = await enrichSessionsPRBatchImpl(prs, observer, repos); + return batchResult.enrichment; }, }; } diff --git a/packages/plugins/scm-github/test/graphql-batch.integration.test.ts b/packages/plugins/scm-github/test/graphql-batch.integration.test.ts index db369c7d1..97be8135e 100644 --- a/packages/plugins/scm-github/test/graphql-batch.integration.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.integration.test.ts @@ -221,7 +221,6 @@ describe("GraphQL Query Generation", () => { "mergeable", "mergeStateStatus", "reviewDecision", - "reviews", "commits", "statusCheckRollup", ]; diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index cdab3eea2..154be9f35 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -161,7 +161,6 @@ describe("GraphQL Batch Query Generation", () => { expect(query).toContain("mergeable"); expect(query).toContain("mergeStateStatus"); expect(query).toContain("reviewDecision"); - expect(query).toContain("reviews"); expect(query).toContain("commits"); expect(query).toContain("statusCheckRollup"); }); @@ -823,7 +822,7 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { const result = await shouldRefreshPREnrichment([]); expect(result.shouldRefresh).toBe(false); - expect(result.details).toContain("No PRs to check"); + expect(result.details).toContain("No repos to check"); // Should not make any API calls expect(mockExecFileImpl).not.toHaveBeenCalled(); }); diff --git a/packages/plugins/scm-github/test/index.test.ts b/packages/plugins/scm-github/test/index.test.ts index 0d0cf4492..cb0e0b6be 100644 --- a/packages/plugins/scm-github/test/index.test.ts +++ b/packages/plugins/scm-github/test/index.test.ts @@ -483,7 +483,7 @@ describe("scm-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await scm.assignPRToCurrentUser?.(pr); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["pr", "edit", "42", "--repo", "acme/repo", "--add-assignee", "@me"], expect.any(Object), ); @@ -530,7 +530,7 @@ describe("scm-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await scm.mergePR(pr); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["pr", "merge", "42", "--repo", "acme/repo", "--squash", "--delete-branch"], expect.any(Object), ); @@ -540,7 +540,7 @@ describe("scm-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await scm.mergePR(pr, "merge"); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--merge"]), expect.any(Object), ); @@ -550,7 +550,7 @@ describe("scm-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await scm.mergePR(pr, "rebase"); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--rebase"]), expect.any(Object), ); @@ -564,7 +564,7 @@ describe("scm-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await scm.closePR(pr); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["pr", "close", "42", "--repo", "acme/repo"], expect.any(Object), ); @@ -916,169 +916,6 @@ describe("scm-github plugin", () => { }); }); - // ---- getAutomatedComments ---------------------------------------------- - - describe("getAutomatedComments", () => { - it("uses explicit GET query for pulls comments and paginates", async () => { - const page1 = Array.from({ length: 100 }, (_, i) => ({ - id: i + 1, - user: { login: "cursor[bot]" }, - body: "Potential issue detected", - path: "a.ts", - line: i + 1, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: `u${i + 1}`, - })); - - const page2 = [ - { - id: 101, - user: { login: "cursor[bot]" }, - body: "Warning: check this", - path: "b.ts", - line: 7, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u101", - }, - ]; - - mockGh(page1); - mockGh(page2); - - const comments = await scm.getAutomatedComments(pr); - - expect(comments).toHaveLength(101); - expect(ghMock).toHaveBeenNthCalledWith( - 1, - "gh", - ["api", "--method", "GET", "repos/acme/repo/pulls/42/comments?per_page=100&page=1"], - expect.any(Object), - ); - expect(ghMock).toHaveBeenNthCalledWith( - 2, - "gh", - ["api", "--method", "GET", "repos/acme/repo/pulls/42/comments?per_page=100&page=2"], - expect.any(Object), - ); - }); - - it("returns bot comments filtered from all PR comments", async () => { - mockGh([ - { - id: 1, - user: { login: "cursor[bot]" }, - body: "Found a potential issue", - path: "a.ts", - line: 5, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u1", - }, - { - id: 2, - user: { login: "alice" }, - body: "Human comment", - path: "a.ts", - line: 1, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u2", - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments).toHaveLength(1); - expect(comments[0].botName).toBe("cursor[bot]"); - expect(comments[0].severity).toBe("error"); // "potential issue" → error - }); - - it("classifies severity from body content", async () => { - mockGh([ - { - id: 1, - user: { login: "github-actions[bot]" }, - body: "Error: build failed", - path: "a.ts", - line: 1, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u", - }, - { - id: 2, - user: { login: "github-actions[bot]" }, - body: "Warning: deprecated API", - path: "a.ts", - line: 2, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u", - }, - { - id: 3, - user: { login: "github-actions[bot]" }, - body: "Deployed to staging", - path: "a.ts", - line: 3, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u", - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments).toHaveLength(3); - expect(comments[0].severity).toBe("error"); - expect(comments[1].severity).toBe("warning"); - expect(comments[2].severity).toBe("info"); - }); - - it("returns empty when no bot comments", async () => { - mockGh([ - { - id: 1, - user: { login: "alice" }, - body: "Human comment", - path: "a.ts", - line: 1, - original_line: null, - created_at: "2025-01-01T00:00:00Z", - html_url: "u", - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments).toEqual([]); - }); - - it("throws on error", async () => { - mockGhError("network failure"); - await expect(scm.getAutomatedComments(pr)).rejects.toThrow( - "Failed to fetch automated comments", - ); - }); - - it("uses original_line as fallback", async () => { - mockGh([ - { - id: 1, - user: { login: "dependabot[bot]" }, - body: "Suggest update", - path: "a.ts", - line: null, - original_line: 15, - created_at: "2025-01-01T00:00:00Z", - html_url: "u", - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments[0].line).toBe(15); - }); - }); - // ---- getMergeability --------------------------------------------------- describe("getMergeability", () => { @@ -1264,4 +1101,334 @@ describe("scm-github plugin", () => { expect(result.mergeable).toBe(false); }); }); + + // ---- PR cache (per-method TTLs, write invalidation) ------------------- + + describe("PR cache", () => { + it("getPRState second call within 5s hits cache", async () => { + mockGh({ state: "OPEN" }); + const first = await scm.getPRState(pr); + const second = await scm.getPRState(pr); + expect(first).toBe("open"); + expect(second).toBe("open"); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("getPRState re-fetches after TTL expires (5s)", async () => { + vi.useFakeTimers(); + try { + mockGh({ state: "OPEN" }); + await scm.getPRState(pr); + expect(ghMock).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(5_001); + + mockGh({ state: "MERGED" }); + const fresh = await scm.getPRState(pr); + expect(fresh).toBe("merged"); + expect(ghMock).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it("getPRState and getPRSummary use separate cache slots", async () => { + mockGh({ state: "OPEN" }); + mockGh({ state: "OPEN", title: "T", additions: 10, deletions: 5 }); + + await scm.getPRState(pr); + await scm.getPRSummary(pr); + expect(ghMock).toHaveBeenCalledTimes(2); + + // Both now cached — second round hits cache, no new gh calls + await scm.getPRState(pr); + await scm.getPRSummary(pr); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("getReviews caches independently of getReviewDecision", async () => { + mockGh({ reviews: [] }); + mockGh({ reviewDecision: "APPROVED" }); + await scm.getReviews(pr); + await scm.getReviewDecision(pr); + expect(ghMock).toHaveBeenCalledTimes(2); + + // Cache hits on second round + await scm.getReviews(pr); + await scm.getReviewDecision(pr); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("different PRs cache independently", async () => { + const otherPR = { ...pr, number: 99 }; + mockGh({ state: "OPEN" }); + mockGh({ state: "MERGED" }); + const a = await scm.getPRState(pr); + const b = await scm.getPRState(otherPR); + expect(a).toBe("open"); + expect(b).toBe("merged"); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("mergePR invalidates the PR's cache", async () => { + mockGh({ state: "OPEN" }); + await scm.getPRState(pr); + expect(ghMock).toHaveBeenCalledTimes(1); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh pr merge + await scm.mergePR(pr); + + mockGh({ state: "MERGED" }); + const fresh = await scm.getPRState(pr); + expect(fresh).toBe("merged"); + expect(ghMock).toHaveBeenCalledTimes(3); + }); + + it("closePR invalidates the PR's cache", async () => { + mockGh({ state: "OPEN" }); + await scm.getPRState(pr); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh pr close + await scm.closePR(pr); + + mockGh({ state: "CLOSED" }); + const fresh = await scm.getPRState(pr); + expect(fresh).toBe("closed"); + expect(ghMock).toHaveBeenCalledTimes(3); + }); + + it("assignPRToCurrentUser invalidates the PR's cache", async () => { + mockGh({ reviewDecision: "REVIEW_REQUIRED" }); + await scm.getReviewDecision(pr); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh pr edit + await scm.assignPRToCurrentUser(pr); + + mockGh({ reviewDecision: "REVIEW_REQUIRED" }); + await scm.getReviewDecision(pr); + expect(ghMock).toHaveBeenCalledTimes(3); // view + edit + view again + }); + + it("invalidating one PR does not affect a different PR's cache", async () => { + const otherPR = { ...pr, number: 99 }; + mockGh({ state: "OPEN" }); + mockGh({ state: "OPEN" }); + await scm.getPRState(pr); + await scm.getPRState(otherPR); + expect(ghMock).toHaveBeenCalledTimes(2); + + ghMock.mockResolvedValueOnce({ stdout: "" }); + await scm.closePR(pr); // wipes pr #42 only + + mockGh({ state: "CLOSED" }); + await scm.getPRState(pr); // re-fetches + await scm.getPRState(otherPR); // still cached + expect(ghMock).toHaveBeenCalledTimes(4); + }); + + it("resolvePR caches by reference for 60s", async () => { + mockGh({ + number: 7, + url: "https://github.com/acme/repo/pull/7", + title: "Fix", + headRefName: "feat/x", + baseRefName: "main", + isDraft: false, + }); + const first = await scm.resolvePR("feat/x", project); + const second = await scm.resolvePR("feat/x", project); + expect(first).toEqual(second); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("failures are not cached", async () => { + ghMock.mockRejectedValueOnce(new Error("boom")); + await expect(scm.getPRState(pr)).rejects.toThrow(); + + mockGh({ state: "OPEN" }); + const fresh = await scm.getPRState(pr); + expect(fresh).toBe("open"); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("each create() returns an isolated cache", async () => { + const scmA = create(); + const scmB = create(); + mockGh({ state: "OPEN" }); + await scmA.getPRState(pr); + mockGh({ state: "MERGED" }); + const fromB = await scmB.getPRState(pr); + expect(fromB).toBe("merged"); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + // ---- detectPR (positive-only cache) ---- + + it("detectPR caches positive results (PR found)", async () => { + mockGh([ + { + number: 42, + url: "https://github.com/acme/repo/pull/42", + title: "feat: add feature", + headRefName: "feat/my-feature", + baseRefName: "main", + isDraft: false, + }, + ]); + const a = await scm.detectPR(makeSession(), project); + const b = await scm.detectPR(makeSession(), project); + expect(a?.number).toBe(42); + expect(b?.number).toBe(42); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("detectPR does NOT cache negative results (no PR yet)", async () => { + mockGh([]); + mockGh([]); + const a = await scm.detectPR(makeSession(), project); + const b = await scm.detectPR(makeSession(), project); + expect(a).toBeNull(); + expect(b).toBeNull(); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("detectPR transitions null → PR on next call without cache poisoning", async () => { + mockGh([]); // first call: no PR yet + const before = await scm.detectPR(makeSession(), project); + expect(before).toBeNull(); + + mockGh([ + { + number: 7, + url: "https://github.com/acme/repo/pull/7", + title: "Just created", + headRefName: "feat/my-feature", + baseRefName: "main", + isDraft: false, + }, + ]); + const after = await scm.detectPR(makeSession(), project); + expect(after?.number).toBe(7); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("detectPR caches different branches independently", async () => { + mockGh([ + { + number: 1, + url: "u1", + title: "t1", + headRefName: "feat/a", + baseRefName: "main", + isDraft: false, + }, + ]); + mockGh([ + { + number: 2, + url: "u2", + title: "t2", + headRefName: "feat/b", + baseRefName: "main", + isDraft: false, + }, + ]); + const a = await scm.detectPR(makeSession({ branch: "feat/a" }), project); + const b = await scm.detectPR(makeSession({ branch: "feat/b" }), project); + expect(a?.number).toBe(1); + expect(b?.number).toBe(2); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("mergePR invalidates the branch's detectPR entry", async () => { + mockGh([ + { + number: 42, + url: "https://github.com/acme/repo/pull/42", + title: "feat", + headRefName: pr.branch, + baseRefName: "main", + isDraft: false, + }, + ]); + await scm.detectPR(makeSession({ branch: pr.branch }), project); + expect(ghMock).toHaveBeenCalledTimes(1); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh pr merge + await scm.mergePR(pr); + + // detectPR re-fetches because the merge invalidated the branch entry + mockGh([]); + const after = await scm.detectPR(makeSession({ branch: pr.branch }), project); + expect(after).toBeNull(); + expect(ghMock).toHaveBeenCalledTimes(3); + }); + + // ---- getCIChecks / getMergeability / getPendingComments ---- + + it("getCIChecks caches result (5s TTL)", async () => { + mockGh([{ name: "build", state: "SUCCESS", link: "u", startedAt: "", completedAt: "" }]); + await scm.getCIChecks(pr); + await scm.getCIChecks(pr); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("getMergeability caches the composite result", async () => { + // First call: getPRState (1) + pr view mergeable (2) + getCISummary→getCIChecks (3) + mockGh({ state: "OPEN" }); // getPRState + mockGh({ + mergeable: "MERGEABLE", + reviewDecision: "APPROVED", + mergeStateStatus: "CLEAN", + isDraft: false, + }); + mockGh([{ name: "build", state: "SUCCESS" }]); // getCIChecks + const first = await scm.getMergeability(pr); + expect(first.mergeable).toBe(true); + + // Second call within TTL: top-level getMergeability cache hits. + // No new gh calls because the composite result short-circuits. + const second = await scm.getMergeability(pr); + expect(second).toEqual(first); + expect(ghMock).toHaveBeenCalledTimes(3); + }); + + it("mergePR invalidates getMergeability cache", async () => { + mockGh({ state: "OPEN" }); + mockGh({ + mergeable: "MERGEABLE", + reviewDecision: "APPROVED", + mergeStateStatus: "CLEAN", + isDraft: false, + }); + mockGh([{ name: "build", state: "SUCCESS" }]); + await scm.getMergeability(pr); + expect(ghMock).toHaveBeenCalledTimes(3); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh pr merge + await scm.mergePR(pr); + + // After merge: state cached as merged; getMergeability re-derives + // the merged shortcut result without making more gh calls. + mockGh({ state: "MERGED" }); // getPRState refetch + const after = await scm.getMergeability(pr); + expect(after.mergeable).toBe(true); + expect(after.blockers).toEqual([]); + }); + + it("getPendingComments caches result", async () => { + mockGhRaw( + JSON.stringify({ + data: { repository: { pullRequest: { reviewThreads: { nodes: [] } } } }, + }), + ); + await scm.getPendingComments(pr); + await scm.getPendingComments(pr); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + }); }); + +function mockGhRaw(stdout: string) { + ghMock.mockResolvedValueOnce({ stdout }); +} diff --git a/packages/plugins/scm-gitlab/src/index.ts b/packages/plugins/scm-gitlab/src/index.ts index dca45a7aa..df9511244 100644 --- a/packages/plugins/scm-gitlab/src/index.ts +++ b/packages/plugins/scm-gitlab/src/index.ts @@ -22,7 +22,8 @@ import { type Review, type ReviewDecision, type ReviewComment, - type AutomatedComment, + type ReviewSummary, + type ReviewThreadsResult, type MergeReadiness, } from "@aoagents/ao-core"; import { @@ -105,21 +106,6 @@ function mapPRState(state: string): PRState { return "open"; } -function inferSeverity(body: string): AutomatedComment["severity"] { - const lower = body.toLowerCase(); - if ( - lower.includes("error") || - lower.includes("bug") || - lower.includes("critical") || - lower.includes("potential issue") - ) { - return "error"; - } - if (lower.includes("warning") || lower.includes("suggest") || lower.includes("consider")) { - return "warning"; - } - return "info"; -} function getGitLabWebhookConfig(project: ProjectConfig) { const webhook = project.scm?.webhook; @@ -701,32 +687,57 @@ function createGitLabSCM(config?: Record): SCM { return comments; }, - async getAutomatedComments(pr: PRInfo): Promise { + async getReviewThreads(pr: PRInfo): Promise { + const hostname = resolveHostname(pr); const discussions = await fetchDiscussions( pr, - resolveHostname(pr), - `getAutomatedComments for MR !${pr.number}`, + hostname, + `getReviewThreads for MR !${pr.number}`, ); - const comments: AutomatedComment[] = []; + // Unresolved threads — includes both human and bot comments (with isBot flag) + const threads: ReviewComment[] = []; for (const d of discussions) { const note = d.notes[0]; if (!note) continue; - const author = note.author?.username ?? ""; - if (!isBot(author)) continue; + if (!note.resolvable || note.resolved) continue; - comments.push({ + const author = note.author?.username ?? "unknown"; + threads.push({ id: String(note.id), - botName: author, + threadId: d.id, + author, body: note.body, path: note.position?.new_path || undefined, line: note.position?.new_line ?? undefined, - severity: inferSeverity(note.body), + isResolved: false, createdAt: parseDate(note.created_at), url: "", + isBot: isBot(author), }); } - return comments; + + // Review summaries from approvals + const reviews: ReviewSummary[] = []; + try { + const approvalsRaw = await glab(["api", `${mrApiPath(pr)}/approvals`], hostname); + const approvals = parseJSON<{ + approved_by: Array<{ user: { username: string } }>; + }>(approvalsRaw, `getReviewThreads approvals for MR !${pr.number}`); + + for (const a of approvals.approved_by ?? []) { + reviews.push({ + author: a.user?.username ?? "unknown", + state: "APPROVED", + body: "", + submittedAt: new Date(0), + }); + } + } catch { + // Best-effort — threads are the critical data + } + + return { threads, reviews }; }, async getMergeability(pr: PRInfo): Promise { diff --git a/packages/plugins/scm-gitlab/test/index.test.ts b/packages/plugins/scm-gitlab/test/index.test.ts index 601553b0c..9dad4d0eb 100644 --- a/packages/plugins/scm-gitlab/test/index.test.ts +++ b/packages/plugins/scm-gitlab/test/index.test.ts @@ -1022,87 +1022,6 @@ describe("scm-gitlab plugin", () => { }); }); - // ---- getAutomatedComments ---------------------------------------------- - - describe("getAutomatedComments", () => { - it("returns bot discussion notes with severity", async () => { - mockGlab([ - { - notes: [ - { - id: 101, - author: { username: "gitlab-bot" }, - body: "Found a critical error", - position: { new_path: "a.ts", new_line: 5 }, - created_at: "2025-01-01T00:00:00Z", - }, - ], - }, - { - notes: [ - { - id: 102, - author: { username: "alice" }, - body: "Human comment", - created_at: "2025-01-01T00:00:00Z", - }, - ], - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments).toHaveLength(1); - expect(comments[0].botName).toBe("gitlab-bot"); - expect(comments[0].severity).toBe("error"); - }); - - it("classifies severity from body content", async () => { - mockGlab([ - { - notes: [ - { - id: 1, - author: { username: "sast-bot" }, - body: "Error: build failed", - created_at: "2025-01-01T00:00:00Z", - }, - ], - }, - { - notes: [ - { - id: 2, - author: { username: "sast-bot" }, - body: "Warning: deprecated API", - created_at: "2025-01-01T00:00:00Z", - }, - ], - }, - { - notes: [ - { - id: 3, - author: { username: "sast-bot" }, - body: "Deployed to staging", - created_at: "2025-01-01T00:00:00Z", - }, - ], - }, - ]); - - const comments = await scm.getAutomatedComments(pr); - expect(comments).toHaveLength(3); - expect(comments[0].severity).toBe("error"); - expect(comments[1].severity).toBe("warning"); - expect(comments[2].severity).toBe("info"); - }); - - it("throws on error (fail-closed)", async () => { - mockGlabError("network failure"); - await expect(scm.getAutomatedComments(pr)).rejects.toThrow("network failure"); - }); - }); - // ---- getMergeability --------------------------------------------------- describe("getMergeability", () => { diff --git a/packages/plugins/tracker-github/src/index.ts b/packages/plugins/tracker-github/src/index.ts index 4abf6d759..8bb0ef4d9 100644 --- a/packages/plugins/tracker-github/src/index.ts +++ b/packages/plugins/tracker-github/src/index.ts @@ -4,31 +4,24 @@ * Uses the `gh` CLI for all GitHub API interactions. */ -import { execFile } from "node:child_process"; -import { promisify } from "node:util"; -import type { - PluginModule, - Tracker, - Issue, - IssueFilters, - IssueUpdate, - CreateIssueInput, - ProjectConfig, +import { + execGhObserved, + type PluginModule, + type Tracker, + type Issue, + type IssueFilters, + type IssueUpdate, + type CreateIssueInput, + type ProjectConfig, } from "@aoagents/ao-core"; -const execFileAsync = promisify(execFile); - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- async function gh(args: string[]): Promise { try { - const { stdout } = await execFileAsync("gh", args, { - maxBuffer: 10 * 1024 * 1024, - timeout: 30_000, - }); - return stdout.trim(); + return await execGhObserved(args, { component: "tracker-github" }, 30_000); } catch (err) { throw new Error(`gh ${args.slice(0, 3).join(" ")} failed: ${(err as Error).message}`, { cause: err, @@ -121,47 +114,106 @@ function requireRepo(project: ProjectConfig): string { return project.repo; } +// Issue cache: 5 min TTL, bounded to 500 entries. Issue metadata (title, body, +// labels, state) rarely changes during a session, and the lifecycle worker +// polls `getIssue` / `isCompleted` repeatedly — same (repo, id) seen 64+ times +// per 5-session tier-5 run in our traces. +const ISSUE_CACHE_TTL_MS = 5 * 60_000; +const ISSUE_CACHE_MAX = 500; + +interface CachedIssue { + issue: Issue; + expiresAt: number; +} + +function issueCacheKey(repo: string, identifier: string): string { + return `${repo}#${identifier.replace(/^#/, "")}`; +} + function createGitHubTracker(): Tracker { - return { + const issueCache = new Map(); + const inflight = new Map>(); + + function readCachedIssue(repo: string, identifier: string): Issue | null { + const key = issueCacheKey(repo, identifier); + const entry = issueCache.get(key); + if (!entry) return null; + if (Date.now() > entry.expiresAt) { + issueCache.delete(key); + return null; + } + return entry.issue; + } + + function writeCachedIssue(repo: string, identifier: string, issue: Issue): void { + if (issueCache.size >= ISSUE_CACHE_MAX) { + const oldest = issueCache.keys().next().value; + if (oldest !== undefined) issueCache.delete(oldest); + } + issueCache.set(issueCacheKey(repo, identifier), { + issue, + expiresAt: Date.now() + ISSUE_CACHE_TTL_MS, + }); + } + + function invalidateCachedIssue(repo: string, identifier: string): void { + issueCache.delete(issueCacheKey(repo, identifier)); + } + + const tracker: Tracker = { name: "github", async getIssue(identifier: string, project: ProjectConfig): Promise { - const raw = await ghIssueViewJson(identifier, project); + const repo = requireRepo(project); + const cached = readCachedIssue(repo, identifier); + if (cached) return cached; - const data: { - number: number; - title: string; - body: string; - url: string; - state: string; - stateReason?: string | null; - labels: Array<{ name: string }>; - assignees: Array<{ login: string }>; - } = JSON.parse(raw); + // Deduplicate concurrent requests for the same issue + const key = issueCacheKey(repo, identifier); + const pending = inflight.get(key); + if (pending) return pending; - return { - id: String(data.number), - title: data.title, - description: data.body ?? "", - url: data.url, - state: mapState(data.state, data.stateReason), - labels: data.labels.map((l) => l.name), - assignee: data.assignees[0]?.login, - }; + const promise = (async () => { + const raw = await ghIssueViewJson(identifier, project); + + const data: { + number: number; + title: string; + body: string; + url: string; + state: string; + stateReason?: string | null; + labels: Array<{ name: string }>; + assignees: Array<{ login: string }>; + } = JSON.parse(raw); + + const issue: Issue = { + id: String(data.number), + title: data.title, + description: data.body ?? "", + url: data.url, + state: mapState(data.state, data.stateReason), + labels: data.labels.map((l) => l.name), + assignee: data.assignees[0]?.login, + }; + + writeCachedIssue(repo, identifier, issue); + return issue; + })(); + + inflight.set(key, promise); + try { + return await promise; + } finally { + inflight.delete(key); + } }, async isCompleted(identifier: string, project: ProjectConfig): Promise { - const raw = await gh([ - "issue", - "view", - identifier, - "--repo", - requireRepo(project), - "--json", - "state", - ]); - const data: { state: string } = JSON.parse(raw); - return data.state.toUpperCase() === "CLOSED"; + // Route through getIssue so the cache covers the hot isCompleted poll path too. + // "closed" and "cancelled" (CLOSED + NOT_PLANNED stateReason) both count as completed. + const issue = await tracker.getIssue(identifier, project); + return issue.state === "closed" || issue.state === "cancelled"; }, issueUrl(identifier: string, project: ProjectConfig): string { @@ -204,6 +256,8 @@ function createGitHubTracker(): Tracker { } lines.push( + "", + "The issue context above is complete and current. You should not need to call gh issue view unless you need additional context beyond what is provided here.", "", "Please implement the changes described in this issue. When done, commit and push your changes.", ); @@ -266,6 +320,8 @@ function createGitHubTracker(): Tracker { project: ProjectConfig, ): Promise { const repo = requireRepo(project); + // Any mutation invalidates the cached Issue for this (repo, identifier). + invalidateCachedIssue(repo, identifier); // Handle state change — GitHub Issues only supports open/closed. // "in_progress" is not a GitHub state, so it is intentionally a no-op. if (update.state === "closed") { @@ -357,9 +413,11 @@ function createGitHubTracker(): Tracker { } const number = match[1]; - return this.getIssue(number, project); + return tracker.getIssue(number, project); }, }; + + return tracker; } // --------------------------------------------------------------------------- diff --git a/packages/plugins/tracker-github/test/index.test.ts b/packages/plugins/tracker-github/test/index.test.ts index 7144a43fa..a838bec6e 100644 --- a/packages/plugins/tracker-github/test/index.test.ts +++ b/packages/plugins/tracker-github/test/index.test.ts @@ -169,21 +169,122 @@ describe("tracker-github plugin", () => { describe("isCompleted", () => { it("returns true for CLOSED issues", async () => { - mockGh({ state: "CLOSED" }); + mockGh({ ...sampleIssue, state: "CLOSED", stateReason: "COMPLETED" }); expect(await tracker.isCompleted("123", project)).toBe(true); }); it("returns false for OPEN issues", async () => { - mockGh({ state: "OPEN" }); + mockGh({ ...sampleIssue, state: "OPEN" }); expect(await tracker.isCompleted("123", project)).toBe(false); }); - it("handles lowercase state", async () => { - mockGh({ state: "closed" }); + it("returns true for cancelled (CLOSED + NOT_PLANNED) issues", async () => { + mockGh({ ...sampleIssue, state: "CLOSED", stateReason: "NOT_PLANNED" }); expect(await tracker.isCompleted("123", project)).toBe(true); }); }); + // ---- issue cache ------------------------------------------------------- + + describe("issue cache", () => { + it("second getIssue for same (repo, id) hits cache — no gh call", async () => { + mockGh(sampleIssue); + const first = await tracker.getIssue("123", project); + const second = await tracker.getIssue("123", project); + expect(first).toEqual(second); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("isCompleted shares the same cache as getIssue", async () => { + mockGh(sampleIssue); + await tracker.getIssue("123", project); + const done = await tracker.isCompleted("123", project); + expect(done).toBe(false); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("different identifiers cache independently", async () => { + mockGh(sampleIssue); + mockGh({ ...sampleIssue, number: 456 }); + await tracker.getIssue("123", project); + await tracker.getIssue("456", project); + expect(ghMock).toHaveBeenCalledTimes(2); + // Both now cached — no new gh calls + await tracker.getIssue("123", project); + await tracker.getIssue("456", project); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("different repos cache independently", async () => { + const otherProject: ProjectConfig = { ...project, repo: "acme/other" }; + mockGh(sampleIssue); + mockGh(sampleIssue); + await tracker.getIssue("123", project); + await tracker.getIssue("123", otherProject); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("strips '#' prefix — '#123' and '123' share a cache entry", async () => { + mockGh(sampleIssue); + await tracker.getIssue("#123", project); + await tracker.getIssue("123", project); + expect(ghMock).toHaveBeenCalledTimes(1); + }); + + it("updateIssue invalidates the cache entry", async () => { + mockGh(sampleIssue); + await tracker.getIssue("123", project); + expect(ghMock).toHaveBeenCalledTimes(1); + + ghMock.mockResolvedValueOnce({ stdout: "" }); // gh issue close + await tracker.updateIssue!("123", { state: "closed" }, project); + + // Next getIssue must re-fetch from gh + mockGh({ ...sampleIssue, state: "CLOSED", stateReason: "COMPLETED" }); + const fresh = await tracker.getIssue("123", project); + expect(fresh.state).toBe("closed"); + expect(ghMock).toHaveBeenCalledTimes(3); // view + close + view again + }); + + it("entries expire after TTL", async () => { + vi.useFakeTimers(); + try { + mockGh(sampleIssue); + await tracker.getIssue("123", project); + expect(ghMock).toHaveBeenCalledTimes(1); + + // Advance time past TTL (5 min) + vi.advanceTimersByTime(5 * 60_000 + 1); + + mockGh(sampleIssue); + await tracker.getIssue("123", project); + expect(ghMock).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it("each create() returns a tracker with an isolated cache", async () => { + const trackerA = create(); + const trackerB = create(); + mockGh(sampleIssue); + await trackerA.getIssue("123", project); + mockGh(sampleIssue); + await trackerB.getIssue("123", project); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + + it("failures are not cached", async () => { + mockGhError("issue not found"); + await expect(tracker.getIssue("999", project)).rejects.toThrow(); + // Next call must re-try, not serve a stale error + mockGh({ ...sampleIssue, number: 999 }); + const issue = await tracker.getIssue("999", project); + expect(issue.id).toBe("999"); + expect(ghMock).toHaveBeenCalledTimes(2); + }); + }); + // ---- issueUrl ---------------------------------------------------------- describe("issueUrl", () => { @@ -259,7 +360,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({ state: "closed" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--state", "closed"]), expect.any(Object), ); @@ -269,7 +370,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({ state: "all" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--state", "all"]), expect.any(Object), ); @@ -279,7 +380,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({}, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--state", "open"]), expect.any(Object), ); @@ -289,7 +390,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({ labels: ["bug", "urgent"] }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--label", "bug,urgent"]), expect.any(Object), ); @@ -299,7 +400,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({ assignee: "alice" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--assignee", "alice"]), expect.any(Object), ); @@ -309,7 +410,7 @@ describe("tracker-github plugin", () => { mockGh([]); await tracker.listIssues!({ limit: 5 }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["--limit", "5"]), expect.any(Object), ); @@ -350,7 +451,7 @@ describe("tracker-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await tracker.updateIssue!("123", { state: "closed" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["issue", "close", "123", "--repo", "acme/repo"], expect.any(Object), ); @@ -360,7 +461,7 @@ describe("tracker-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await tracker.updateIssue!("123", { state: "open" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["issue", "reopen", "123", "--repo", "acme/repo"], expect.any(Object), ); @@ -370,7 +471,7 @@ describe("tracker-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await tracker.updateIssue!("123", { labels: ["bug", "urgent"] }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["issue", "edit", "123", "--repo", "acme/repo", "--add-label", "bug,urgent"], expect.any(Object), ); @@ -380,7 +481,7 @@ describe("tracker-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await tracker.updateIssue!("123", { assignee: "bob" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["issue", "edit", "123", "--repo", "acme/repo", "--add-assignee", "bob"], expect.any(Object), ); @@ -390,7 +491,7 @@ describe("tracker-github plugin", () => { ghMock.mockResolvedValueOnce({ stdout: "" }); await tracker.updateIssue!("123", { comment: "Working on this" }, project); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), ["issue", "comment", "123", "--repo", "acme/repo", "--body", "Working on this"], expect.any(Object), ); @@ -451,7 +552,7 @@ describe("tracker-github plugin", () => { project, ); expect(ghMock).toHaveBeenCalledWith( - "gh", + expect.stringMatching(/(?:^|\/)?gh$/), expect.arrayContaining(["issue", "create", "--label", "bug", "--assignee", "alice"]), expect.any(Object), ); diff --git a/packages/web/server/mux-websocket.ts b/packages/web/server/mux-websocket.ts index c2e91f073..1b3364167 100644 --- a/packages/web/server/mux-websocket.ts +++ b/packages/web/server/mux-websocket.ts @@ -20,7 +20,7 @@ import { findTmux, resolveTmuxSession, validateSessionId } from "./tmux-utils.js type ClientMessage = | { ch: "terminal"; id: string; type: "data"; data: string } | { ch: "terminal"; id: string; type: "resize"; cols: number; rows: number } - | { ch: "terminal"; id: string; type: "open" } + | { ch: "terminal"; id: string; type: "open"; tmuxName?: string } | { ch: "terminal"; id: string; type: "close" } | { ch: "system"; type: "ping" } | { ch: "subscribe"; topics: ("sessions")[] }; @@ -246,13 +246,15 @@ class TerminalManager { * Open/attach to a terminal. If already open, just return. * If has subscribers but PTY crashed, re-attach. */ - open(id: string): string { + open(id: string, tmuxName?: string): string { // Validate and resolve if (!validateSessionId(id)) { throw new Error(`Invalid session ID: ${id}`); } - const tmuxSessionId = resolveTmuxSession(id, this.TMUX); + // Use provided tmuxName, or reuse from existing terminal entry, or resolve + const existing = this.terminals.get(id); + const tmuxSessionId = tmuxName ?? existing?.tmuxSessionId ?? resolveTmuxSession(id, this.TMUX); if (!tmuxSessionId) { throw new Error(`Session not found: ${id}`); } @@ -499,7 +501,7 @@ export function createMuxWebSocket(tmuxPath?: string): WebSocketServer | null { try { if (type === "open") { // Validate session exists - terminalManager.open(id); + terminalManager.open(id, "tmuxName" in msg ? msg.tmuxName : undefined); // Send opened confirmation (idempotent — safe to send on re-open) const openedMsg: ServerMessage = { ch: "terminal", id, type: "opened" }; diff --git a/packages/web/server/tmux-utils.ts b/packages/web/server/tmux-utils.ts index f22ed514e..0403a9a3a 100644 --- a/packages/web/server/tmux-utils.ts +++ b/packages/web/server/tmux-utils.ts @@ -161,14 +161,6 @@ export function resolveTmuxSession( // even when the storageKey is wrapped (`{hash}-{projectName}`). Walk // every candidate so a stale metadata dir in one project can't shadow // the live session of another project with the same sessionId. - // - // Pre-existing limitation (not introduced by this fix): when two - // different projects have both (a) the same user-facing sessionId and - // (b) live tmux sessions, we return the first live match. The terminal - // open protocol carries only the session ID — no project context — so - // there's nothing to disambiguate with here. The previous bare-hash - // code had the same behavior via `Array.find(...)`. A proper fix would - // thread projectId through the mux protocol from the browser URL. for (const storageKey of findStorageKeysForSession(sessionId, fs)) { const tmuxName = `${storageKey}-${sessionId}`; try { diff --git a/packages/web/src/app/api/sessions/[id]/route.ts b/packages/web/src/app/api/sessions/[id]/route.ts index bcf208501..0d5e46883 100644 --- a/packages/web/src/app/api/sessions/[id]/route.ts +++ b/packages/web/src/app/api/sessions/[id]/route.ts @@ -1,6 +1,6 @@ import { type NextRequest } from "next/server"; import { getSessionsDir, readAgentReportAuditTrailAsync } from "@aoagents/ao-core"; -import { getServices, getSCM } from "@/lib/services"; +import { getServices } from "@/lib/services"; import { sessionToDashboard, resolveProject, @@ -12,8 +12,6 @@ import { getCorrelationId, jsonWithCorrelation, recordApiObservation } from "@/l const AGENT_REPORT_AUDIT_TIMEOUT_MS = 1000; const METADATA_ENRICH_TIMEOUT_MS = 3000; -const PR_CACHE_ENRICH_TIMEOUT_MS = 1000; -const PR_LIVE_ENRICH_TIMEOUT_MS = 2000; export async function GET(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { const correlationId = getCorrelationId(_request); @@ -43,27 +41,9 @@ export async function GET(_request: NextRequest, { params }: { params: Promise<{ METADATA_ENRICH_TIMEOUT_MS, ); - // Enrich PR — serve cache immediately, refresh in background if stale + // Enrich PR from session metadata (written by CLI lifecycle) if (coreSession.pr) { - const scm = getSCM(registry, project); - if (scm) { - let cached = false; - const cachedSettled = await settlesWithin( - enrichSessionPR(dashboardSession, scm, coreSession.pr, { - cacheOnly: true, - }).then((result) => { - cached = result; - }), - PR_CACHE_ENRICH_TIMEOUT_MS, - ); - if (!cached) { - // Nothing cached yet — block once to populate, then future calls use cache - await settlesWithin( - enrichSessionPR(dashboardSession, scm, coreSession.pr), - cachedSettled ? PR_LIVE_ENRICH_TIMEOUT_MS : PR_CACHE_ENRICH_TIMEOUT_MS + PR_LIVE_ENRICH_TIMEOUT_MS, - ); - } - } + enrichSessionPR(dashboardSession); } recordApiObservation({ diff --git a/packages/web/src/app/api/sessions/route.ts b/packages/web/src/app/api/sessions/route.ts index b1e7fd688..77a3f1ec2 100644 --- a/packages/web/src/app/api/sessions/route.ts +++ b/packages/web/src/app/api/sessions/route.ts @@ -1,8 +1,7 @@ import { ACTIVITY_STATE, isOrchestratorSession, isTerminalSession } from "@aoagents/ao-core"; -import { getServices, getSCM } from "@/lib/services"; +import { getServices } from "@/lib/services"; import { sessionToDashboard, - resolveProject, enrichSessionPR, enrichSessionsMetadata, computeStats, @@ -14,13 +13,6 @@ import { settlesWithin } from "@/lib/async-utils"; import type { DashboardOrchestratorLink } from "@/lib/types"; const METADATA_ENRICH_TIMEOUT_MS = 3_000; -const PR_ENRICH_TIMEOUT_MS = 4_000; -const PER_PR_ENRICH_TIMEOUT_MS = 1_500; - -function hasTerminalPRState(session: Parameters[0]): boolean { - const prState = session.lifecycle?.pr.state; - return prState === "merged" || prState === "closed"; -} function compareOrchestratorRecency(a: { lastActivityAt?: Date | null; createdAt?: Date | null; id: string }, b: { lastActivityAt?: Date | null; createdAt?: Date | null; id: string }): number { return ( @@ -152,34 +144,11 @@ export async function GET(request: Request) { ); if (metadataSettled) { - const prEnrichPromises: Promise[] = []; - + // PR enrichment: read from session metadata (written by CLI lifecycle). + // No GitHub API calls — synchronous metadata read. for (let i = 0; i < workerSessions.length; i++) { - const core = workerSessions[i]; - const pr = core?.pr; - if (!pr) continue; - - const project = resolveProject(core, config.projects); - const scm = getSCM(registry, project); - if (!scm) continue; - - prEnrichPromises.push( - settlesWithin( - hasTerminalPRState(core) - ? enrichSessionPR(dashboardSessions[i], scm, pr, { cacheOnly: true }).then( - (cached) => - cached - ? true - : enrichSessionPR(dashboardSessions[i], scm, pr), - ) - : enrichSessionPR(dashboardSessions[i], scm, pr), - PER_PR_ENRICH_TIMEOUT_MS, - ), - ); - } - - if (prEnrichPromises.length > 0) { - await settlesWithin(Promise.allSettled(prEnrichPromises), PR_ENRICH_TIMEOUT_MS); + if (!workerSessions[i]?.pr) continue; + enrichSessionPR(dashboardSessions[i]); } } diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index 9f74c7daf..3d62d3630 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -72,6 +72,8 @@ function getStoredFontSize(): number { interface DirectTerminalProps { sessionId: string; + /** Actual tmux session name. When provided, the terminal server uses it directly instead of resolving from sessionId. */ + tmuxName?: string; startFullscreen?: boolean; /** Visual variant. Orchestrator keeps the same design-system blue accent as the rest of the app. */ variant?: "agent" | "orchestrator"; @@ -164,6 +166,7 @@ export function buildTerminalThemes(variant: TerminalVariant): { dark: ITheme; l */ export function DirectTerminal({ sessionId, + tmuxName, startFullscreen = false, variant = "agent", appearance = "theme", @@ -474,7 +477,7 @@ export function DirectTerminal({ }); // Open terminal via mux - openTerminal(sessionId); + openTerminal(sessionId, tmuxName); // Subscribe to terminal data via mux unsubscribe = subscribeTerminal(sessionId, (data) => { diff --git a/packages/web/src/components/SessionDetail.tsx b/packages/web/src/components/SessionDetail.tsx index 07ef7d574..dbd30d109 100644 --- a/packages/web/src/components/SessionDetail.tsx +++ b/packages/web/src/components/SessionDetail.tsx @@ -651,6 +651,7 @@ export function SessionDetail({ ) : ( { expect(screen.queryByText(/No active sessions/i)).not.toBeInTheDocument(); }); - it("does not render a restore action for merged sessions", () => { + it("renders a restore action for merged sessions", () => { render(); const toggle = screen.getByText("Done / Terminated").closest("button")!; fireEvent.click(toggle); - expect(screen.queryByRole("button", { name: /restore/i })).toBeNull(); + expect(screen.queryByRole("button", { name: /restore/i })).toBeInTheDocument(); }); }); diff --git a/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts b/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts index 86fa52331..c2b4b3582 100644 --- a/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts +++ b/packages/web/src/lib/__tests__/dashboard-page-data.fast-path.test.ts @@ -94,15 +94,9 @@ describe("getDashboardPageData fast path", () => { { projects: { docs: { id: "docs" } } }, { scm: "registry" }, ); - expect(hoisted.enrichSessionPRMock).toHaveBeenCalledTimes(1); - expect(hoisted.enrichSessionPRMock).toHaveBeenCalledWith( - dashboardMerged, - { provider: "github" }, - mergedCore.pr, - { cacheOnly: true }, - ); - expect(dashboardClosed.pr.state).toBe("closed"); - expect(dashboardMerged.pr.state).toBe("merged"); + expect(hoisted.enrichSessionPRMock).toHaveBeenCalledTimes(2); + expect(hoisted.enrichSessionPRMock).toHaveBeenCalledWith(dashboardClosed); + expect(hoisted.enrichSessionPRMock).toHaveBeenCalledWith(dashboardMerged); expect(pageData.sessions).toEqual([dashboardNoPr, dashboardClosed, dashboardMerged]); }); diff --git a/packages/web/src/lib/__tests__/serialize.test.ts b/packages/web/src/lib/__tests__/serialize.test.ts index ba34a1470..34eaa8916 100644 --- a/packages/web/src/lib/__tests__/serialize.test.ts +++ b/packages/web/src/lib/__tests__/serialize.test.ts @@ -2,12 +2,11 @@ * Tests for session serialization and PR enrichment */ -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import { createInitialCanonicalLifecycle, type Session, type PRInfo, - type SCM, type Agent, type Tracker, type ProjectConfig, @@ -18,7 +17,7 @@ import { sessionToDashboard, resolveProject, enrichSessionPR, - enrichSessionIssue, + readPREnrichmentFromMetadata, enrichSessionAgentSummary, enrichSessionIssueTitle, enrichSessionsMetadata, @@ -26,7 +25,6 @@ import { computeStats, listDashboardOrchestrators, } from "../serialize"; -import { prCache, prCacheKey } from "../cache"; import type { DashboardSession } from "../types"; // Helper to create a minimal Session for testing @@ -77,56 +75,32 @@ function createPRInfo(overrides?: Partial): PRInfo { }; } -// Mock SCM that succeeds -function createMockSCM(): SCM { - return { - name: "mock", - detectPR: vi.fn(), - getPRState: vi.fn().mockResolvedValue("open"), - getPRSummary: vi.fn().mockResolvedValue({ - state: "open", - title: "Test PR", - additions: 100, - deletions: 50, - }), - getCIChecks: vi - .fn() - .mockResolvedValue([{ name: "test", status: "passed", url: "https://example.com" }]), - getCISummary: vi.fn().mockResolvedValue("passing"), - getReviewDecision: vi.fn().mockResolvedValue("approved"), - getMergeability: vi.fn().mockResolvedValue({ - mergeable: true, - ciPassing: true, - approved: true, - noConflicts: true, - blockers: [], - }), - getPendingComments: vi.fn().mockResolvedValue([]), - getReviews: vi.fn(), - getAutomatedComments: vi.fn(), - mergePR: vi.fn(), - closePR: vi.fn(), - }; +// Helper to create prEnrichment metadata JSON +function createEnrichmentMetadata(overrides?: Record): string { + return JSON.stringify({ + state: "open", + title: "Test PR", + additions: 100, + deletions: 50, + ciStatus: "passing", + reviewDecision: "approved", + mergeable: true, + isDraft: false, + hasConflicts: false, + ciChecks: [{ name: "test", status: "passed", url: "https://example.com" }], + enrichedAt: new Date().toISOString(), + ...overrides, + }); } -// Mock SCM that fails all requests -function createFailingSCM(): SCM { - const error = new Error("API rate limited"); - return { - name: "mock-failing", - detectPR: vi.fn(), - getPRState: vi.fn().mockRejectedValue(error), - getPRSummary: vi.fn().mockRejectedValue(error), - getCIChecks: vi.fn().mockRejectedValue(error), - getCISummary: vi.fn().mockRejectedValue(error), - getReviewDecision: vi.fn().mockRejectedValue(error), - getMergeability: vi.fn().mockRejectedValue(error), - getPendingComments: vi.fn().mockRejectedValue(error), - getReviews: vi.fn(), - getAutomatedComments: vi.fn(), - mergePR: vi.fn(), - closePR: vi.fn(), - }; +// Helper to create prReviewComments metadata JSON +function createReviewCommentsMetadata(overrides?: Record): string { + return JSON.stringify({ + unresolvedThreads: 0, + unresolvedComments: [], + commentsUpdatedAt: new Date().toISOString(), + ...overrides, + }); } describe("sessionToDashboard", () => { @@ -400,18 +374,20 @@ describe("resolveProject", () => { }); describe("enrichSessionPR", () => { - beforeEach(() => { - prCache.clear(); - }); - - it("should enrich PR with live SCM data", async () => { + it("should enrich PR from metadata", () => { const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); + const coreSession = createCoreSession({ + pr, + metadata: { + prEnrichment: createEnrichmentMetadata(), + prReviewComments: createReviewCommentsMetadata(), + }, + }); const dashboard = sessionToDashboard(coreSession); - const scm = createMockSCM(); - await enrichSessionPR(dashboard, scm, pr); + const result = enrichSessionPR(dashboard); + expect(result).toBe(true); expect(dashboard.pr?.state).toBe("open"); expect(dashboard.pr?.additions).toBe(100); expect(dashboard.pr?.deletions).toBe(50); @@ -420,106 +396,75 @@ describe("enrichSessionPR", () => { expect(dashboard.pr?.mergeability.mergeable).toBe(true); expect(dashboard.pr?.ciChecks).toHaveLength(1); expect(dashboard.pr?.ciChecks[0]?.name).toBe("test"); + expect(dashboard.pr?.enriched).toBe(true); }); - it("should cache successful enrichment results", async () => { + it("should return false when prEnrichment metadata is missing", () => { const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); - const dashboard = sessionToDashboard(coreSession); - const scm = createMockSCM(); - - await enrichSessionPR(dashboard, scm, pr); - - const cacheKey = prCacheKey(pr.owner, pr.repo, pr.number); - const cached = prCache.get(cacheKey); - expect(cached).not.toBeNull(); - expect(cached?.additions).toBe(100); - expect(cached?.deletions).toBe(50); - }); - - it("should use cached data on subsequent calls", async () => { - const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); - const dashboard1 = sessionToDashboard(coreSession); - const dashboard2 = sessionToDashboard(coreSession); - const scm = createMockSCM(); - - // First call: fetch from SCM - await enrichSessionPR(dashboard1, scm, pr); - expect(scm.getPRSummary).toHaveBeenCalledTimes(1); - - // Second call: use cache - await enrichSessionPR(dashboard2, scm, pr); - expect(scm.getPRSummary).toHaveBeenCalledTimes(1); // Still 1, not 2 - expect(dashboard2.pr?.additions).toBe(100); - }); - - it("should handle rate limit errors gracefully", async () => { - const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); - const dashboard = sessionToDashboard(coreSession); - const scm = createFailingSCM(); - - // Spy on console.warn (enrichSessionPR uses warn for rate-limit, not error) - const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); - - await enrichSessionPR(dashboard, scm, pr); - - // Should keep default values but update blocker message - expect(dashboard.pr?.additions).toBe(0); - expect(dashboard.pr?.deletions).toBe(0); - expect(dashboard.pr?.mergeability.blockers).toContain("API rate limited or unavailable"); - - // Should log warning - expect(consoleWarnSpy).toHaveBeenCalled(); - - consoleWarnSpy.mockRestore(); - }); - - it("should cache even when most requests fail (to reduce API pressure)", async () => { - const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); - const dashboard = sessionToDashboard(coreSession); - const scm = createFailingSCM(); - - await enrichSessionPR(dashboard, scm, pr); - - // Even with all failures, we cache the default/partial data to prevent repeated API hits - const cacheKey = prCacheKey(pr.owner, pr.repo, pr.number); - const cached = prCache.get(cacheKey); - expect(cached).not.toBeNull(); - expect(cached?.mergeability.blockers).toContain("API rate limited or unavailable"); - }); - - it("should handle partial failures gracefully", async () => { - const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); + const coreSession = createCoreSession({ pr, metadata: {} }); const dashboard = sessionToDashboard(coreSession); - // Mock SCM with partial failures - const scm: SCM = { - ...createMockSCM(), - getCISummary: vi.fn().mockRejectedValue(new Error("CI API failed")), - getMergeability: vi.fn().mockRejectedValue(new Error("Merge API failed")), - }; + const result = enrichSessionPR(dashboard); - await enrichSessionPR(dashboard, scm, pr); - - // Successful fields should be populated - expect(dashboard.pr?.additions).toBe(100); - expect(dashboard.pr?.deletions).toBe(50); - expect(dashboard.pr?.reviewDecision).toBe("approved"); - - // Failed fields should have graceful defaults - expect(dashboard.pr?.mergeability.blockers).toContain("Merge status unavailable"); - - // Should still cache partial results - const cacheKey = prCacheKey(pr.owner, pr.repo, pr.number); - const cached = prCache.get(cacheKey); - expect(cached).not.toBeNull(); + expect(result).toBe(false); + expect(dashboard.pr?.enriched).toBe(false); }); - it("should do nothing if dashboard.pr is null", async () => { + it("should enrich review comments from metadata", () => { + const pr = createPRInfo(); + const coreSession = createCoreSession({ + pr, + metadata: { + prEnrichment: createEnrichmentMetadata(), + prReviewComments: createReviewCommentsMetadata({ + unresolvedThreads: 2, + unresolvedComments: [ + { url: "https://example.com", path: "src/app.ts", author: "reviewer", body: "Fix this" }, + ], + }), + }, + }); + const dashboard = sessionToDashboard(coreSession); + + enrichSessionPR(dashboard); + + expect(dashboard.pr?.unresolvedThreads).toBe(2); + expect(dashboard.pr?.unresolvedComments).toHaveLength(1); + expect(dashboard.pr?.unresolvedComments[0]?.author).toBe("reviewer"); + }); + + it("should handle missing review comments metadata gracefully", () => { + const pr = createPRInfo(); + const coreSession = createCoreSession({ + pr, + metadata: { + prEnrichment: createEnrichmentMetadata(), + // No prReviewComments + }, + }); + const dashboard = sessionToDashboard(coreSession); + + enrichSessionPR(dashboard); + + expect(dashboard.pr?.unresolvedThreads).toBe(0); + expect(dashboard.pr?.unresolvedComments).toEqual([]); + }); + + it("should handle malformed prEnrichment JSON gracefully", () => { + const pr = createPRInfo(); + const coreSession = createCoreSession({ + pr, + metadata: { prEnrichment: "not valid json{" }, + }); + const dashboard = sessionToDashboard(coreSession); + + const result = enrichSessionPR(dashboard); + + expect(result).toBe(false); + expect(dashboard.pr?.enriched).toBe(false); + }); + + it("should do nothing if dashboard.pr is null", () => { const dashboard: DashboardSession = { id: "test-1", projectId: "test", @@ -541,32 +486,87 @@ describe("enrichSessionPR", () => { createdAt: new Date().toISOString(), lastActivityAt: new Date().toISOString(), pr: null, - metadata: {}, + metadata: { prEnrichment: createEnrichmentMetadata() }, }; - const pr = createPRInfo(); - const scm = createMockSCM(); - await enrichSessionPR(dashboard, scm, pr); + const result = enrichSessionPR(dashboard); - expect(scm.getPRSummary).not.toHaveBeenCalled(); + expect(result).toBe(false); }); - it("should handle missing optional SCM methods", async () => { + it("should derive mergeability fields from enrichment data", () => { const pr = createPRInfo(); - const coreSession = createCoreSession({ pr }); + const coreSession = createCoreSession({ + pr, + metadata: { + prEnrichment: createEnrichmentMetadata({ + ciStatus: "failing", + reviewDecision: "changes_requested", + mergeable: false, + hasConflicts: true, + }), + }, + }); const dashboard = sessionToDashboard(coreSession); - // SCM without getPRSummary - const scm: SCM = { - ...createMockSCM(), - getPRSummary: undefined, - }; + enrichSessionPR(dashboard); - await enrichSessionPR(dashboard, scm, pr); + expect(dashboard.pr?.mergeability.mergeable).toBe(false); + expect(dashboard.pr?.mergeability.ciPassing).toBe(false); + expect(dashboard.pr?.mergeability.approved).toBe(false); + expect(dashboard.pr?.mergeability.noConflicts).toBe(false); + }); - // Should fall back to getPRState - expect(scm.getPRState).toHaveBeenCalled(); - expect(dashboard.pr?.state).toBe("open"); + it("should set enriched flag on successful enrichment", () => { + const pr = createPRInfo(); + const coreSession = createCoreSession({ + pr, + metadata: { prEnrichment: createEnrichmentMetadata() }, + }); + const dashboard = sessionToDashboard(coreSession); + expect(dashboard.pr?.enriched).toBe(false); + + enrichSessionPR(dashboard); + + expect(dashboard.pr?.enriched).toBe(true); + }); +}); + +describe("readPREnrichmentFromMetadata", () => { + it("should parse valid enrichment metadata", () => { + const data = readPREnrichmentFromMetadata({ + prEnrichment: createEnrichmentMetadata(), + }); + + expect(data).not.toBeNull(); + expect(data?.state).toBe("open"); + expect(data?.additions).toBe(100); + expect(data?.deletions).toBe(50); + }); + + it("should return null when prEnrichment is missing", () => { + const data = readPREnrichmentFromMetadata({}); + expect(data).toBeNull(); + }); + + it("should return null for invalid JSON", () => { + const data = readPREnrichmentFromMetadata({ prEnrichment: "bad json{" }); + expect(data).toBeNull(); + }); + + it("should include review comments when present", () => { + const data = readPREnrichmentFromMetadata({ + prEnrichment: createEnrichmentMetadata(), + prReviewComments: createReviewCommentsMetadata({ + unresolvedThreads: 3, + unresolvedComments: [ + { url: "https://example.com", path: "src/app.ts", author: "alice", body: "Please fix" }, + ], + }), + }); + + expect(data?.unresolvedThreads).toBe(3); + expect(data?.unresolvedComments).toHaveLength(1); }); }); diff --git a/packages/web/src/lib/cache.ts b/packages/web/src/lib/cache.ts index 18fc469de..b7b4a4791 100644 --- a/packages/web/src/lib/cache.ts +++ b/packages/web/src/lib/cache.ts @@ -105,9 +105,6 @@ export interface PREnrichmentData { }>; } -/** Global PR enrichment cache (60s TTL) */ -export const prCache = new TTLCache(); - /** Generate cache key for a PR: `owner/repo#123` */ export function prCacheKey(owner: string, repo: string, number: number): string { return `${owner}/${repo}#${number}`; diff --git a/packages/web/src/lib/dashboard-page-data.ts b/packages/web/src/lib/dashboard-page-data.ts index 467e7fe88..51fcdf2fa 100644 --- a/packages/web/src/lib/dashboard-page-data.ts +++ b/packages/web/src/lib/dashboard-page-data.ts @@ -6,10 +6,9 @@ import { type DashboardOrchestratorLink, type DashboardAttentionZoneMode, } from "@/lib/types"; -import { getServices, getSCM } from "@/lib/services"; +import { getServices } from "@/lib/services"; import { sessionToDashboard, - resolveProject, enrichSessionPR, enrichSessionsMetadataFast, listDashboardOrchestrators, @@ -126,23 +125,10 @@ export const getDashboardPageData = cache(async function getDashboardPageData(pr console.warn("[dashboard-page-data] metadata fast enrichment failed:", err); } - // PR cache hits only (in-memory lookup, no SCM API calls). + // PR enrichment from session metadata (no API calls). for (let i = 0; i < coreSessions.length; i++) { - const core = coreSessions[i]; - if (!core.pr) continue; - try { - const projectConfig = resolveProject(core, config.projects); - const scm = getSCM(registry, projectConfig); - if (scm) { - try { - await enrichSessionPR(pageData.sessions[i], scm, core.pr, { cacheOnly: true }); - } catch { - // Preserve the base session payload if PR enrichment fails. - } - } - } catch (err) { - console.warn(`[dashboard-page-data] PR enrichment failed for session ${core.id}:`, err); - } + if (!coreSessions[i].pr) continue; + enrichSessionPR(pageData.sessions[i]); } return pageData; diff --git a/packages/web/src/lib/mux-protocol.ts b/packages/web/src/lib/mux-protocol.ts index cf1926e29..19e625784 100644 --- a/packages/web/src/lib/mux-protocol.ts +++ b/packages/web/src/lib/mux-protocol.ts @@ -5,7 +5,7 @@ import type { AttentionLevel } from "./types"; export type ClientMessage = | { ch: "terminal"; id: string; type: "data"; data: string } | { ch: "terminal"; id: string; type: "resize"; cols: number; rows: number } - | { ch: "terminal"; id: string; type: "open" } + | { ch: "terminal"; id: string; type: "open"; tmuxName?: string } | { ch: "terminal"; id: string; type: "close" } | { ch: "system"; type: "ping" } | { ch: "subscribe"; topics: ("sessions")[] }; diff --git a/packages/web/src/lib/serialize.ts b/packages/web/src/lib/serialize.ts index aa69d35d0..fd207f3df 100644 --- a/packages/web/src/lib/serialize.ts +++ b/packages/web/src/lib/serialize.ts @@ -12,7 +12,6 @@ import { isTerminalSession, type Session, type Agent, - type SCM, type PRInfo, type Tracker, type ProjectConfig, @@ -26,7 +25,7 @@ import { type DashboardOrchestratorLink, getAttentionLevel, } from "./types"; -import { TTLCache, prCache, prCacheKey, type PREnrichmentData } from "./cache"; +import { TTLCache, type PREnrichmentData } from "./cache"; /** Cache for issue titles (5 min TTL — issue titles rarely change) */ const issueTitleCache = new TTLCache(300_000); @@ -302,159 +301,86 @@ function normalizeDashboardPRState( * Enrich a DashboardSession's PR with live data from the SCM plugin. * Uses cache to reduce API calls and handles rate limit errors gracefully. */ -export async function enrichSessionPR( +/** + * Read PR enrichment data from session metadata. + * The CLI lifecycle manager writes this data every 30s (batch enrichment) + * and every 2min (review comments). Returns null if not available. + */ +export function readPREnrichmentFromMetadata( + metadata: Record, +): PREnrichmentData | null { + const enrichmentRaw = metadata["prEnrichment"]; + if (!enrichmentRaw) return null; + + try { + const e = JSON.parse(enrichmentRaw); + + let reviewData = { + unresolvedThreads: 0, + unresolvedComments: [] as Array<{ url: string; path: string; author: string; body: string }>, + }; + const reviewRaw = metadata["prReviewComments"]; + if (reviewRaw) { + try { + reviewData = JSON.parse(reviewRaw); + } catch { + /* use defaults */ + } + } + + return { + state: e.state ?? "open", + title: e.title ?? "", + additions: e.additions ?? 0, + deletions: e.deletions ?? 0, + ciStatus: e.ciStatus ?? "none", + ciChecks: (e.ciChecks ?? []).map( + (c: { name: string; status: string; url?: string }) => ({ + name: c.name, + status: c.status, + url: c.url, + }), + ), + reviewDecision: e.reviewDecision ?? "none", + mergeability: { + mergeable: e.mergeable ?? false, + ciPassing: e.ciStatus === "passing" || e.ciStatus === "none", + approved: e.reviewDecision === "approved", + noConflicts: !(e.hasConflicts ?? false), + blockers: e.blockers ?? [], + }, + unresolvedThreads: reviewData.unresolvedThreads ?? 0, + unresolvedComments: reviewData.unresolvedComments ?? [], + }; + } catch { + return null; + } +} + +/** + * Enrich a DashboardSession's PR data from session metadata. + * The CLI lifecycle manager persists batch enrichment data to metadata files. + * No GitHub API calls — reads from disk via the metadata already loaded. + */ +export function enrichSessionPR( dashboard: DashboardSession, - scm: SCM, - pr: PRInfo, - opts?: { cacheOnly?: boolean }, -): Promise { +): boolean { if (!dashboard.pr) return false; - const cacheKey = prCacheKey(pr.owner, pr.repo, pr.number); + const data = readPREnrichmentFromMetadata(dashboard.metadata); + if (!data) return false; - // Check cache first - const cached = prCache.get(cacheKey); - if (cached && dashboard.pr) { - dashboard.pr.state = cached.state; - dashboard.pr.title = cached.title; - dashboard.pr.additions = cached.additions; - dashboard.pr.deletions = cached.deletions; - dashboard.pr.ciStatus = cached.ciStatus; - dashboard.pr.ciChecks = cached.ciChecks; - dashboard.pr.reviewDecision = cached.reviewDecision; - dashboard.pr.mergeability = cached.mergeability; - dashboard.pr.unresolvedThreads = cached.unresolvedThreads; - dashboard.pr.unresolvedComments = cached.unresolvedComments; - dashboard.pr.enriched = true; - refreshDashboardSessionDerivedFields(dashboard); - return true; - } - - // Cache miss — if cacheOnly, signal caller to refresh in background - if (opts?.cacheOnly) return false; - - // Fetch from SCM - const results = await Promise.allSettled([ - scm.getPRSummary - ? scm.getPRSummary(pr) - : scm.getPRState(pr).then((state) => ({ state, title: "", additions: 0, deletions: 0 })), - scm.getCIChecks(pr), - scm.getCISummary(pr), - scm.getReviewDecision(pr), - scm.getMergeability(pr), - scm.getPendingComments(pr), - ]); - - const [summaryR, checksR, ciR, reviewR, mergeR, commentsR] = results; - - // Check if most critical requests failed (likely rate limit) - // Note: Some methods (like getCISummary) return fallback values instead of rejecting, - // so we can't rely on "all rejected" — check if majority failed instead - const failedCount = results.filter((r) => r.status === "rejected").length; - const mostFailed = failedCount >= results.length / 2; - - if (mostFailed) { - const rejectedResults = results.filter( - (r) => r.status === "rejected", - ) as PromiseRejectedResult[]; - const firstError = rejectedResults[0]?.reason; - console.warn( - `[enrichSessionPR] ${failedCount}/${results.length} API calls failed for PR #${pr.number} (rate limited or unavailable):`, - String(firstError), - ); - // Don't return early — apply any successful results below - } - - // Apply successful results - if (summaryR.status === "fulfilled") { - dashboard.pr.state = summaryR.value.state; - dashboard.pr.additions = summaryR.value.additions; - dashboard.pr.deletions = summaryR.value.deletions; - if (summaryR.value.title) { - dashboard.pr.title = summaryR.value.title; - } - } - - if (checksR.status === "fulfilled") { - dashboard.pr.ciChecks = checksR.value.map((c) => ({ - name: c.name, - status: c.status, - url: c.url, - })); - } - - if (ciR.status === "fulfilled") { - dashboard.pr.ciStatus = ciR.value; - } - - if (reviewR.status === "fulfilled") { - dashboard.pr.reviewDecision = reviewR.value; - } - - if (mergeR.status === "fulfilled") { - dashboard.pr.mergeability = mergeR.value; - } else { - // Mergeability failed — mark as unavailable - dashboard.pr.mergeability.blockers = ["Merge status unavailable"]; - } - - if (commentsR.status === "fulfilled") { - const comments = commentsR.value; - dashboard.pr.unresolvedThreads = comments.length; - dashboard.pr.unresolvedComments = comments.map((c) => ({ - url: c.url, - path: c.path ?? "", - author: c.author, - body: c.body, - })); - } - - // Mark as enriched — we attempted SCM API calls and applied whatever succeeded + dashboard.pr.state = data.state; + if (data.title) dashboard.pr.title = data.title; + dashboard.pr.additions = data.additions; + dashboard.pr.deletions = data.deletions; + dashboard.pr.ciStatus = data.ciStatus; + dashboard.pr.ciChecks = data.ciChecks; + dashboard.pr.reviewDecision = data.reviewDecision; + dashboard.pr.mergeability = data.mergeability; + dashboard.pr.unresolvedThreads = data.unresolvedThreads; + dashboard.pr.unresolvedComments = data.unresolvedComments; dashboard.pr.enriched = true; - - // Add rate-limit warning blocker if most requests failed - // (but we still applied any successful results above) - if ( - mostFailed && - !dashboard.pr.mergeability.blockers.includes("API rate limited or unavailable") - ) { - dashboard.pr.mergeability.blockers.push("API rate limited or unavailable"); - } - - // If rate limited, cache the partial data with a long TTL (5 min) so we stop - // hammering the API on every page load. The rate-limit blocker flag tells the - // UI to show stale-data warnings instead of making decisions on bad data. - if (mostFailed) { - const rateLimitedData: PREnrichmentData = { - state: dashboard.pr.state, - title: dashboard.pr.title, - additions: dashboard.pr.additions, - deletions: dashboard.pr.deletions, - ciStatus: dashboard.pr.ciStatus, - ciChecks: dashboard.pr.ciChecks, - reviewDecision: dashboard.pr.reviewDecision, - mergeability: dashboard.pr.mergeability, - unresolvedThreads: dashboard.pr.unresolvedThreads, - unresolvedComments: dashboard.pr.unresolvedComments, - }; - prCache.set(cacheKey, rateLimitedData, 60 * 60_000); // 60 min — GitHub rate limit resets hourly - refreshDashboardSessionDerivedFields(dashboard); - return true; - } - - const cacheData: PREnrichmentData = { - state: dashboard.pr.state, - title: dashboard.pr.title, - additions: dashboard.pr.additions, - deletions: dashboard.pr.deletions, - ciStatus: dashboard.pr.ciStatus, - ciChecks: dashboard.pr.ciChecks, - reviewDecision: dashboard.pr.reviewDecision, - mergeability: dashboard.pr.mergeability, - unresolvedThreads: dashboard.pr.unresolvedThreads, - unresolvedComments: dashboard.pr.unresolvedComments, - }; - prCache.set(cacheKey, cacheData); refreshDashboardSessionDerivedFields(dashboard); return true; } diff --git a/packages/web/src/lib/services.ts b/packages/web/src/lib/services.ts index 705eaa198..237c5ac4e 100644 --- a/packages/web/src/lib/services.ts +++ b/packages/web/src/lib/services.ts @@ -115,10 +115,14 @@ async function initServices(): Promise { const sessionManager = createSessionManager({ config, registry }); - // Start the lifecycle manager — polls sessions every 30s, triggers reactions - // (CI failure → send fix message, review comments → forward to agent, etc.) + // Lifecycle manager for webhook-triggered checks only — no independent polling. + // The CLI process (`ao`) runs the 30s polling loop and writes PR enrichment + // data to session metadata files. The dashboard reads from metadata instead + // of calling GitHub API directly. This means the dashboard is NOT self-sufficient: + // if the CLI process isn't running, sessions will have no PR enrichment data, + // no state transitions, and no reactions. The SSE endpoint surfaces whatever + // metadata the CLI has written — stale data is expected when CLI is down. const lifecycleManager = createLifecycleManager({ config, registry, sessionManager }); - lifecycleManager.start(30_000); return { config, registry, sessionManager, lifecycleManager }; } diff --git a/packages/web/src/providers/MuxProvider.tsx b/packages/web/src/providers/MuxProvider.tsx index ac64474e5..66bb9ed57 100644 --- a/packages/web/src/providers/MuxProvider.tsx +++ b/packages/web/src/providers/MuxProvider.tsx @@ -6,7 +6,7 @@ import type { ClientMessage, ServerMessage, SessionPatch } from "@/lib/mux-proto interface MuxContextValue { subscribeTerminal: (id: string, callback: (data: string) => void) => () => void; writeTerminal: (id: string, data: string) => void; - openTerminal: (id: string) => void; + openTerminal: (id: string, tmuxName?: string) => void; closeTerminal: (id: string) => void; resizeTerminal: (id: string, cols: number, rows: number) => void; status: "connecting" | "connected" | "reconnecting" | "disconnected"; @@ -71,7 +71,7 @@ function buildMuxWsUrl(runtimeConfig: { directTerminalPort?: string; proxyWsPath export function MuxProvider({ children }: { children: ReactNode }) { const wsRef = useRef(null); const subscribersRef = useRef(new Map void>>()); - const openedTerminalsRef = useRef(new Set()); + const openedTerminalsRef = useRef(new Map()); const [status, setStatus] = useState<"connecting" | "connected" | "reconnecting" | "disconnected">( "connecting", ); @@ -105,11 +105,12 @@ export function MuxProvider({ children }: { children: ReactNode }) { reconnectAttempt.current = 0; // Re-open previously opened terminals - for (const terminalId of openedTerminalsRef.current) { + for (const [terminalId, tmuxName] of openedTerminalsRef.current) { const openMsg: ClientMessage = { ch: "terminal", id: terminalId, type: "open", + ...(tmuxName && { tmuxName }), }; ws.send(JSON.stringify(openMsg)); } @@ -136,8 +137,10 @@ export function MuxProvider({ children }: { children: ReactNode }) { } } } else if (msg.type === "opened") { - // Terminal opened successfully - openedTerminalsRef.current.add(msg.id); + // Terminal opened successfully — preserve existing tmuxName + if (!openedTerminalsRef.current.has(msg.id)) { + openedTerminalsRef.current.set(msg.id, undefined); + } } else if (msg.type === "exited") { // PTY exited and could not be re-attached — remove so it isn't // re-opened on reconnect, and surface a terminal-level error chunk @@ -269,13 +272,14 @@ export function MuxProvider({ children }: { children: ReactNode }) { } }, []); - const openTerminal = useCallback((id: string) => { - openedTerminalsRef.current.add(id); + const openTerminal = useCallback((id: string, tmuxName?: string) => { + openedTerminalsRef.current.set(id, tmuxName); if (wsRef.current?.readyState === WebSocket.OPEN) { const msg: ClientMessage = { ch: "terminal", id, type: "open", + ...(tmuxName && { tmuxName }), }; wsRef.current.send(JSON.stringify(msg)); }