From cf5a418a48c1e77ea41da3845a7a0ccbdcebcff0 Mon Sep 17 00:00:00 2001 From: Adil Shaikh Date: Sun, 3 May 2026 18:30:27 +0530 Subject: [PATCH 01/30] fix(scm-github): silence HTTP 304 warnings in ETag guards (#1581) * fix(scm-github): silence HTTP 304 warnings in ETag guards and use observer logging ETag guard functions and the GraphQL batch handler used raw console.warn()/ console.error() that fired on every poll cycle, including expected HTTP 304 (Not Modified) responses. This flooded the orchestrator terminal with noise. - Add 304 fallback check in error message for cases where gh CLI doesn't populate stdout/stderr on non-zero exit - Migrate all 4 raw console.warn()/console.error() calls to observer?.log() for consistency with the rest of the file - Thread observer parameter through shouldRefreshPREnrichment and the three ETag guard functions (checkPRListETag, checkCommitStatusETag, checkReviewCommentsETag) - Update test to verify observer-based logging instead of console spy Closes #1580 * fix(scm-github): use word boundary in 304 regex to prevent false positives Use \b304\b instead of /304/ to avoid matching substrings like "3040" or "30400" in error messages. Closes #1580 * fix(scm-github): use is304() for error message fallback and thread observer into getReviewThreads 1. Replace \b304\b regex with is304() (anchored to HTTP status line) in all three ETag guard fallback paths. Prevents false positives from URL paths like "pulls/304/comments" appearing in Node's execFile error messages. 2. Capture instance-level observer in createGitHubSCM() and pass it to checkReviewCommentsETag and getReviewThreads error logging. Non-304 errors on the review polling path are now visible via observer instead of being silently swallowed by lifecycle's catch. 3. Restore "error" severity for partial batch failures in enrichSessionsPRBatchImpl (was incorrectly downgraded to "warn"). 4. Add tests for Guard 1 URL false-positive, Guard 2 error logging, and Guard 2 HTTP 304 fallback paths. Closes #1580 * test(scm-github): add Guard 3 (checkReviewCommentsETag) tests Add 5 tests for the review comments ETag guard covering: - 200 response (changed) and 304 response (unchanged) - Error with observer logging - HTTP 304 in error message treated as cache hit - URL containing "304" NOT treated as cache hit (false-positive prevention) This completes the test coverage for all three ETag guard functions' error and 304-fallback paths, as requested in review. Closes #1580 --- .../plugins/scm-github/src/graphql-batch.ts | 35 ++-- packages/plugins/scm-github/src/index.ts | 9 +- .../scm-github/test/graphql-batch.test.ts | 180 +++++++++++++++++- 3 files changed, 207 insertions(+), 17 deletions(-) diff --git a/packages/plugins/scm-github/src/graphql-batch.ts b/packages/plugins/scm-github/src/graphql-batch.ts index 5fe45e57b..108d47168 100644 --- a/packages/plugins/scm-github/src/graphql-batch.ts +++ b/packages/plugins/scm-github/src/graphql-batch.ts @@ -214,6 +214,7 @@ function updatePRMetadataCache( export async function shouldRefreshPREnrichment( prs: PRInfo[], extraRepos: string[] = [], + observer?: BatchObserver, ): Promise { const details: string[] = []; let shouldRefresh = false; @@ -248,7 +249,7 @@ export async function shouldRefreshPREnrichment( const prListUnchangedRepos = new Set(); for (const [repoKey] of repos) { const [owner, repo] = repoKey.split("/"); - const prListChanged = await checkPRListETag(owner, repo); + const prListChanged = await checkPRListETag(owner, repo, observer); if (prListChanged) { guard1DetectedChanges = true; shouldRefresh = true; @@ -292,6 +293,7 @@ export async function shouldRefreshPREnrichment( pr.owner, pr.repo, cached.headSha, + observer, ); if (statusChanged) { shouldRefresh = true; @@ -414,6 +416,7 @@ function extractETag(output: string): string | undefined { async function checkPRListETag( owner: string, repo: string, + observer?: BatchObserver, ): Promise { const repoKey = `${owner}/${repo}`; const cachedETag = etagCache.prList.get(repoKey); @@ -456,8 +459,13 @@ async function checkPRListETag( } const errorMsg = err instanceof Error ? err.message : String(err); - // eslint-disable-next-line no-console -- Observability logging for ETag errors - console.warn(`[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`); + // HTTP 304 may surface as an error message without stdout/stderr (e.g. gh cli versions + // that don't populate stdout on non-zero exit). Use is304() anchored to the HTTP status + // line to avoid false positives from URL paths like "pulls/304/comments". + if (is304(errorMsg)) { + return false; + } + observer?.log("warn", `[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`); return true; // Assume changed to be safe } } @@ -479,6 +487,7 @@ async function checkCommitStatusETag( owner: string, repo: string, sha: string, + observer?: BatchObserver, ): Promise { const commitKey = `${owner}/${repo}#${sha}`; const cachedETag = etagCache.commitStatus.get(commitKey); @@ -521,10 +530,10 @@ async function checkCommitStatusETag( } const errorMsg = err instanceof Error ? err.message : String(err); - // eslint-disable-next-line no-console -- Observability logging for ETag errors - console.warn( - `[ETag Guard 2] Commit status check failed for ${commitKey}: ${errorMsg}`, - ); + if (is304(errorMsg)) { + return false; + } + observer?.log("warn", `[ETag Guard 2] Commit status check failed for ${commitKey}: ${errorMsg}`); return true; // Assume changed to be safe } } @@ -548,6 +557,7 @@ export async function checkReviewCommentsETag( owner: string, repo: string, prNumber: number, + observer?: BatchObserver, ): Promise { const cacheKey = `${owner}/${repo}#${prNumber}`; const cachedETag = etagCache.reviewComments.get(cacheKey); @@ -583,8 +593,10 @@ export async function checkReviewCommentsETag( } 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}`); + if (is304(errorMsg)) { + return false; + } + observer?.log("warn", `[ETag Guard 3] Review comments check failed for ${cacheKey}: ${errorMsg}`); return true; // Assume changed to be safe } } @@ -1046,7 +1058,7 @@ export async function enrichSessionsPRBatch( // Step 1: Check if we need to refresh using 2-Guard ETag Strategy // 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); + const guardResult = await shouldRefreshPREnrichment(prs, repos, observer); // Report which repos had no PR list changes so the lifecycle can skip detectPR observer?.reportPRListUnchangedRepos?.(guardResult.prListUnchangedRepos); @@ -1152,8 +1164,7 @@ export async function enrichSessionsPRBatch( }); // Log error for observability but don't fail entirely - // eslint-disable-next-line no-console -- Observability logging for batch errors - console.error(`[GraphQL Batch Warning] Batch enrichment partially failed: ${errorMsg}`); + observer?.log("error", `[GraphQL Batch] Batch enrichment partially failed: ${errorMsg}`); // Don't add placeholder entries to result or cache. // This allows lifecycle-manager to fall back to individual API calls diff --git a/packages/plugins/scm-github/src/index.ts b/packages/plugins/scm-github/src/index.ts index 7f05b9272..05baa166c 100644 --- a/packages/plugins/scm-github/src/index.ts +++ b/packages/plugins/scm-github/src/index.ts @@ -486,6 +486,10 @@ function createGitHubSCM(): SCM { // ETag-controlled cache for review threads + reviews. Freshness is managed by // Guard 3 (checkReviewCommentsETag) — not a TTL timer. const reviewThreadsCache = new Map(); + // Instance-level observer captured from enrichSessionsPRBatch calls. + // Used by getReviewThreads (which can't accept observer via the SCM interface) + // to log non-304 errors that would otherwise be swallowed by lifecycle's catch. + let instanceObserver: BatchObserver | undefined; function prCacheKey(owner: string, repo: string, prKey: string, method: PRCacheMethod): string { return `${owner}/${repo}#${prKey}:${method}`; @@ -1006,7 +1010,7 @@ function createGitHubSCM(): SCM { 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); + const reviewsChanged = await checkReviewCommentsETag(pr.owner, pr.repo, pr.number, instanceObserver); if (!reviewsChanged) { const cached = reviewThreadsCache.get(cacheKey); if (cached) return cached; @@ -1133,6 +1137,8 @@ function createGitHubSCM(): SCM { reviewThreadsCache.set(cacheKey, result); return result; } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err); + instanceObserver?.log("warn", `[getReviewThreads] Failed for ${cacheKey}: ${errorMsg}`); throw new Error("Failed to fetch review threads", { cause: err }); } }, @@ -1241,6 +1247,7 @@ function createGitHubSCM(): SCM { observer?: BatchObserver, repos?: string[], ): Promise> { + if (observer) instanceObserver = observer; const batchResult = await enrichSessionsPRBatchImpl(prs, observer, repos); return batchResult.enrichment; }, diff --git a/packages/plugins/scm-github/test/graphql-batch.test.ts b/packages/plugins/scm-github/test/graphql-batch.test.ts index 154be9f35..2c987ea71 100644 --- a/packages/plugins/scm-github/test/graphql-batch.test.ts +++ b/packages/plugins/scm-github/test/graphql-batch.test.ts @@ -26,6 +26,7 @@ import { getPRMetadataCache, clearPRMetadataCache, shouldRefreshPREnrichment, + checkReviewCommentsETag, setExecFileAsync, } from "../src/graphql-batch.js"; @@ -898,14 +899,69 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { ]; // Mock gh CLI error - const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; mockExecFileImpl.mockRejectedValueOnce(new Error("gh CLI failed")); + const result = await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(result.shouldRefresh).toBe(true); // Fail-safe: assume changed on error + expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 1]")); + }); + + it("should treat error message with HTTP 304 status line as cache hit", async () => { + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + // Error where stdout/stderr extraction fails but message contains HTTP 304 status + const err = new Error("HTTP/1.1 304 Not Modified"); + mockExecFileImpl.mockRejectedValueOnce(err); + const result = await shouldRefreshPREnrichment(prs); - expect(result.shouldRefresh).toBe(true); // Fail-safe: assume changed on error - expect(consoleWarnSpy).toHaveBeenCalled(); - consoleWarnSpy.mockRestore(); + expect(result.shouldRefresh).toBe(false); // Treated as cache hit + }); + + it("should NOT treat error message with '304' in a URL path as cache hit", async () => { + const prs = [ + { + owner: "owner", + repo: "repo", + number: 304, + url: "https://github.com/owner/repo/pull/304", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + // Error message contains "304" in URL path but not as HTTP status + const err = new Error("failed to fetch pulls/304/comments"); + mockExecFileImpl.mockRejectedValueOnce(err); + + const result = await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(result.shouldRefresh).toBe(true); // Not a cache hit — real error + expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 1]")); }); }); @@ -1043,6 +1099,65 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { // Guard 1 called for PR list, Guard 2 skipped (no head SHA to check) expect(mockExecFileImpl).toHaveBeenCalledTimes(1); }); + + it("should refresh on Guard 2 error and log warning", async () => { + setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "pending" }); + + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + + // Guard 1: 304 (no change), Guard 2: error + mockExecFileImpl + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockRejectedValueOnce(new Error("gh CLI failed on commit status")); + + const result = await shouldRefreshPREnrichment(prs, [], mockObserver); + + expect(result.shouldRefresh).toBe(true); + expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 2]")); + }); + + it("should treat Guard 2 error with HTTP 304 status line as cache hit", async () => { + setPRMetadata("owner/repo#123", { headSha: "abc123", ciStatus: "pending" }); + + const prs = [ + { + owner: "owner", + repo: "repo", + number: 123, + url: "https://github.com/owner/repo/pull/123", + title: "Test PR", + branch: "feature", + baseBranch: "main", + isDraft: false, + }, + ]; + + // Guard 1: 304, Guard 2: error with HTTP 304 in message + mockExecFileImpl + .mockResolvedValueOnce({ stdout: "HTTP/2 304", stderr: "" }) + .mockRejectedValueOnce(new Error("HTTP/1.1 304 Not Modified")); + + const result = await shouldRefreshPREnrichment(prs); + + expect(result.shouldRefresh).toBe(false); + }); }); describe("Multiple Repositories", () => { @@ -1170,6 +1285,63 @@ describe("shouldRefreshPREnrichment - ETag Guard Strategy", () => { expect(callsWithHeader).toHaveLength(2); // Both Guard 1 and Guard 2 }); }); + + describe("Guard 3: Review Comments ETag", () => { + it("should return true (changed) on 200 response", async () => { + mockExecFileImpl.mockResolvedValueOnce({ + stdout: 'HTTP/2 200\netag: "review-etag"', + stderr: "", + }); + + const result = await checkReviewCommentsETag("owner", "repo", 42); + expect(result).toBe(true); + }); + + it("should return false (unchanged) on 304 response", async () => { + mockExecFileImpl.mockResolvedValueOnce({ + stdout: "HTTP/2 304", + stderr: "", + }); + + const result = await checkReviewCommentsETag("owner", "repo", 42); + expect(result).toBe(false); + }); + + it("should return true on error and log warning via observer", async () => { + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + mockExecFileImpl.mockRejectedValueOnce(new Error("gh CLI failed")); + + const result = await checkReviewCommentsETag("owner", "repo", 42, mockObserver); + + expect(result).toBe(true); // Fail-safe: assume changed + expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 3]")); + }); + + it("should treat error with HTTP 304 status line as cache hit", async () => { + mockExecFileImpl.mockRejectedValueOnce(new Error("HTTP/1.1 304 Not Modified")); + + const result = await checkReviewCommentsETag("owner", "repo", 42); + expect(result).toBe(false); + }); + + it("should NOT treat error with '304' in URL path as cache hit", async () => { + const mockObserver = { + recordSuccess: vi.fn(), + recordFailure: vi.fn(), + log: vi.fn(), + }; + mockExecFileImpl.mockRejectedValueOnce(new Error("failed to fetch pulls/304/comments")); + + const result = await checkReviewCommentsETag("owner", "repo", 304, mockObserver); + + expect(result).toBe(true); // Not a cache hit + expect(mockObserver.log).toHaveBeenCalledWith("warn", expect.stringContaining("[ETag Guard 3]")); + }); + }); }); describe("extractPREnrichment ciChecks", () => { From 703d5844f000c157530c7a059648d6ec843c3e6b Mon Sep 17 00:00:00 2001 From: Adil Shaikh Date: Sun, 3 May 2026 18:30:47 +0530 Subject: [PATCH 02/30] fix(core): deliver enriched review content on changes_requested transition (#1578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(core): deliver enriched review content on changes_requested transition The transition reaction for changes_requested sent a generic message ("Details will follow shortly") but the backlog dispatch that carries the actual review comment bodies was blocked by its own deduplication logic — the transition handler recorded the fingerprint hash as "dispatched" without ever sending the enriched content. Remove the premature hash recording and the transition guard so the backlog dispatch fires in the same poll cycle, delivering actual review comment details (file paths, line numbers, authors, bodies) to the agent immediately. Closes #1558 * fix(core): update stale comments from review feedback - Update throttle-bypass comment to reflect removed Branch B - Fix test comment: second check is throttled, not just fingerprint match * fix(core): prevent double-billing reaction attempts on changes_requested transition When a changes_requested transition fires, the transition handler calls executeReaction (attempt 1) and then maybeDispatchReviewBacklog calls it again for the enriched message (attempt 2). With retries:1, this caused premature escalation on the very first transition poll. Fix: when the transition handler already fired executeReaction for the same reaction key, send the enriched payload directly via sessionManager.send — bypassing the reaction tracker entirely. Also moves lastReviewBacklogCheckAt after the SCM fetch so a failed getReviewThreads call doesn't block retries for 2 minutes. Fixes #1578 * fix(core): gate review bypass on send-to-agent action type The direct sessionManager.send bypass (introduced to prevent double-billing reaction attempts) fired unconditionally, ignoring reactionConfig.action. With action: "notify", the enriched review content was pushed to the agent's stdin instead of routing through notifyHuman. Gate the bypass on action === "send-to-agent" so notify configs fall through to executeReaction which routes correctly. Applied to both human and automated review comment paths. Adds test verifying action: "notify" does not call sessionManager.send and does fire the notifier. Fixes #1578 --- .../fix-review-enrichment-double-billing.md | 13 ++ .../src/__tests__/lifecycle-manager.test.ts | 169 +++++++++++++++++- packages/core/src/config.ts | 2 +- packages/core/src/lifecycle-manager.ts | 112 +++++++----- 4 files changed, 246 insertions(+), 50 deletions(-) create mode 100644 .changeset/fix-review-enrichment-double-billing.md diff --git a/.changeset/fix-review-enrichment-double-billing.md b/.changeset/fix-review-enrichment-double-billing.md new file mode 100644 index 000000000..06cf75272 --- /dev/null +++ b/.changeset/fix-review-enrichment-double-billing.md @@ -0,0 +1,13 @@ +--- +"@aoagents/ao-core": patch +--- + +fix(core): prevent double-billing reaction attempts on changes_requested transition + +The enriched review dispatch in `maybeDispatchReviewBacklog` now sends directly via +`sessionManager.send` when the transition handler already called `executeReaction` for +the same reaction key. This prevents the attempt counter from incrementing twice in a +single poll cycle, which would cause premature escalation for projects with `retries: 1`. + +Also moves the review backlog throttle timestamp after the SCM fetch so a failed +`getReviewThreads` call doesn't block retries for 2 minutes. diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index bd5c82d66..1c4caad04 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -1998,7 +1998,7 @@ describe("reactions", () => { expect(metadata?.["lastPendingReviewDispatchHash"]).toBe("c1"); }); - it("does not double-send when changes_requested transition already triggered the reaction", async () => { + it("sends enriched review content on changes_requested transition alongside the generic message", async () => { config.reactions = { "changes-requested": { auto: true, @@ -2052,12 +2052,171 @@ describe("reactions", () => { }); await lm.check("app-1"); + + // First call is the transition reaction (generic message), second is + // the backlog dispatch with actual review comment content. + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + const enrichedMessage = vi.mocked(mockSessionManager.send).mock.calls[1]![1] as string; + expect(enrichedMessage).toContain("src/route.ts:44"); + expect(enrichedMessage).toContain("@reviewer"); + expect(enrichedMessage).toContain("Please add validation"); + + // Second check: throttled (within REVIEW_BACKLOG_THROTTLE_MS window) and + // fingerprint already matches dispatch hash — neither path re-sends. + vi.mocked(mockSessionManager.send).mockClear(); + await lm.check("app-1"); + expect(mockSessionManager.send).not.toHaveBeenCalled(); + }); + + it("does not double-bill reaction attempts on changes_requested transition with retries:1", async () => { + const notifier = createMockNotifier(); + + config.reactions = { + "changes-requested": { + auto: true, + action: "send-to-agent", + message: "Handle requested changes.", + retries: 1, + }, + }; + + const mockSCM = createMockSCM({ + getReviewDecision: vi.fn().mockResolvedValue("changes_requested"), + 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", + body: "Needs validation", + path: "src/handler.ts", + line: 10, + isResolved: false, + createdAt: new Date(), + url: "https://example.com/comment/retries", + isBot: false, + }, + ], + reviews: [], + }), + }); + + 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; + }), + }; + + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + 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); + // Transition handler sends the generic message (attempt 1), and the backlog + // dispatch sends the enriched message directly (no attempt increment). + // Total sends = 2 but reaction attempts = 1, so no escalation. + expect(mockSessionManager.send).toHaveBeenCalledTimes(2); + expect(notifier.notify).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "reaction.escalated" }), + ); + + // The enriched message should contain the actual review content + const enrichedMessage = vi.mocked(mockSessionManager.send).mock.calls[1]![1] as string; + expect(enrichedMessage).toContain("src/handler.ts:10"); + expect(enrichedMessage).toContain("Needs validation"); + }); + + it("routes enriched review dispatch through executeReaction when action is notify (not send-to-agent)", async () => { + const notifier = createMockNotifier(); + + config.reactions = { + "changes-requested": { + auto: true, + action: "notify", + message: "Review changes requested.", + }, + }; + config.notificationRouting = { + ...config.notificationRouting, + info: ["desktop"], + }; + + const mockSCM = createMockSCM({ + getReviewDecision: vi.fn().mockResolvedValue("changes_requested"), + 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", + body: "Fix the type", + path: "src/api.ts", + line: 5, + isResolved: false, + createdAt: new Date(), + url: "https://example.com/comment/notify", + isBot: false, + }, + ], + reviews: [], + }), + }); + + 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; + }), + }; + + vi.mocked(mockSessionManager.send).mockResolvedValue(undefined); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + + await lm.check("app-1"); + + // action: "notify" should NOT send to the agent — it routes through + // executeReaction → notifyHuman. The bypass branch must not fire. + expect(mockSessionManager.send).not.toHaveBeenCalled(); + expect(notifier.notify).toHaveBeenCalled(); }); it("dispatches detailed automated review comments when using the default sentinel message", async () => { diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 7d03ac4ea..57a236bf5 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -676,7 +676,7 @@ function applyDefaultReactions(config: OrchestratorConfig): OrchestratorConfig { auto: true, action: "send-to-agent", message: - "There are review comments on your PR. Details will follow shortly.", + "There are new review comments on your PR requesting changes.", escalateAfter: "30m", }, "bugbot-comments": { diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index d6c3f6a26..fdb8a3918 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -1439,7 +1439,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan async function maybeDispatchReviewBacklog( session: Session, - oldStatus: SessionStatus, + _oldStatus: SessionStatus, newStatus: SessionStatus, transitionReaction?: { key: string; result: ReactionResult | null }, ): Promise { @@ -1472,11 +1472,10 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan // (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 - // lastPendingReviewDispatchHash, which requires the current fingerprint from - // the API. If we throttle here, that metadata never gets written and the - // next unthrottled poll sees a "new" fingerprint, clears the reaction tracker, - // and fires a duplicate dispatch. + // review reaction key. The enriched dispatch needs the current fingerprint + // from the API so it can fire and record the hash in the same cycle. If we + // throttle here, the next unthrottled poll sees a "new" fingerprint, clears + // the reaction tracker, and fires a duplicate dispatch. const hasRelevantTransition = transitionReaction?.key === humanReactionKey || transitionReaction?.key === automatedReactionKey; @@ -1486,11 +1485,9 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return; } } - lastReviewBacklogCheckAt.set(session.id, Date.now()); - // 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 allThreads: ReviewComment[]; let reviewSummaries: ReviewSummary[] = []; try { if (scm.getReviewThreads) { @@ -1502,11 +1499,18 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan allThreads = await scm.getPendingComments(session.pr); } } catch { - // Failed to fetch — preserve existing metadata + // Failed to fetch — preserve existing metadata. + // Don't update the throttle timestamp so the next poll retries immediately + // instead of being blocked for 2 minutes with the agent left on a bare notification. + return; } + // Only stamp the throttle after a successful SCM fetch. If the fetch failed, + // we returned above so the next poll can retry without waiting 2 minutes. + lastReviewBacklogCheckAt.set(session.id, Date.now()); + // 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, @@ -1528,12 +1532,11 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } } - const pendingComments = allThreads ? allThreads.filter((c) => !c.isBot) : null; - const automatedComments = allThreads ? allThreads.filter((c) => c.isBot) : null; + const pendingComments = allThreads.filter((c) => !c.isBot); + const automatedComments = allThreads.filter((c) => c.isBot); // --- Pending (human) review comments --- - // null = SCM fetch failed; skip processing to preserve existing metadata. - if (pendingComments !== null) { + { const pendingFingerprint = makeFingerprint(pendingComments.map((comment) => comment.id)); const lastPendingFingerprint = session.metadata["lastPendingReviewFingerprint"] ?? ""; const lastPendingDispatchHash = session.metadata["lastPendingReviewDispatchHash"] ?? ""; @@ -1557,32 +1560,42 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan lastPendingReviewDispatchHash: "", lastPendingReviewDispatchAt: "", }); - } else if ( - transitionReaction?.key === humanReactionKey && - transitionReaction.result?.success - ) { - if (lastPendingDispatchHash !== pendingFingerprint) { - updateSessionMetadata(session, { - lastPendingReviewDispatchHash: pendingFingerprint, - lastPendingReviewDispatchAt: new Date().toISOString(), - }); - } - } else if ( - !(oldStatus !== newStatus && newStatus === "changes_requested") && - pendingFingerprint !== lastPendingDispatchHash - ) { + } else if (pendingFingerprint !== lastPendingDispatchHash) { const reactionConfig = getReactionConfigForSession(session, humanReactionKey); if ( reactionConfig && reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { - const enrichedConfig = { - ...reactionConfig, - message: formatReviewCommentsMessage(pendingComments, "reviewer", reviewSummaries), - }; - const result = await executeReaction(session, humanReactionKey, enrichedConfig); - if (result.success) { + const enrichedMessage = formatReviewCommentsMessage( + pendingComments, + "reviewer", + reviewSummaries, + ); + + // When the transition handler already called executeReaction for this + // key, send the enriched payload directly to avoid double-billing the + // reaction attempt budget. A project with retries:1 would otherwise + // escalate on the very first transition poll. + // Only bypass for "send-to-agent" — "notify" actions must go through + // executeReaction so they route to notifyHuman instead of the agent. + let success = false; + if ( + transitionReaction?.key === humanReactionKey && + reactionConfig.action === "send-to-agent" + ) { + try { + await sessionManager.send(session.id, enrichedMessage); + success = true; + } catch { + // Send failed — will retry on next unthrottled poll + } + } else { + const enrichedConfig = { ...reactionConfig, message: enrichedMessage }; + const result = await executeReaction(session, humanReactionKey, enrichedConfig); + success = result.success; + } + if (success) { updateSessionMetadata(session, { lastPendingReviewDispatchHash: pendingFingerprint, lastPendingReviewDispatchAt: new Date().toISOString(), @@ -1593,7 +1606,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan } // --- Automated (bot) review comments --- - if (automatedComments !== null) { + { const automatedFingerprint = makeFingerprint(automatedComments.map((comment) => comment.id)); const lastAutomatedFingerprint = session.metadata["lastAutomatedReviewFingerprint"] ?? ""; const lastAutomatedDispatchHash = session.metadata["lastAutomatedReviewDispatchHash"] ?? ""; @@ -1619,14 +1632,25 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan reactionConfig.action && (reactionConfig.auto !== false || reactionConfig.action === "notify") ) { - // Always enrich with formatted comment listing so the agent has inline - // data and doesn't need to re-fetch via gh api. - const enrichedConfig = { - ...reactionConfig, - message: formatReviewCommentsMessage(automatedComments, "bot"), - }; - const result = await executeReaction(session, automatedReactionKey, enrichedConfig); - if (result.success) { + const enrichedMessage = formatReviewCommentsMessage(automatedComments, "bot"); + + let success = false; + if ( + transitionReaction?.key === automatedReactionKey && + reactionConfig.action === "send-to-agent" + ) { + try { + await sessionManager.send(session.id, enrichedMessage); + success = true; + } catch { + // Send failed — will retry on next unthrottled poll + } + } else { + const enrichedConfig = { ...reactionConfig, message: enrichedMessage }; + const result = await executeReaction(session, automatedReactionKey, enrichedConfig); + success = result.success; + } + if (success) { updateSessionMetadata(session, { lastAutomatedReviewDispatchHash: automatedFingerprint, lastAutomatedReviewDispatchAt: new Date().toISOString(), From 3b9ba3122ed807a27743cd1c0a7650cea0b4cf83 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari <24b4506@iitb.ac.in> Date: Sun, 3 May 2026 20:39:57 +0530 Subject: [PATCH 03/30] feat(web): add 'Open orchestrator' to sidebar 3-dot menu (#1615) * feat(web): add 'Open orchestrator' to sidebar 3-dot menu Adds a labeled menu entry above 'Project settings' that navigates to the project's orchestrator session. The orchestrator is the most-used session in any project, but today the only path to it is the unlabeled icon button next to the dashboard icon - easy to miss for new users. The entry is hidden when no live orchestrator exists (matching the existing icon-button pattern), so the menu shrinks gracefully on projects where 'ao start' has never run or has stopped. Closes #1613 Co-Authored-By: Claude Opus 4.7 * refactor(web): drop redundant guard in orchestrator menu render Hold the validated session in `liveOrchestrator` instead of a separate boolean flag. TypeScript narrows automatically from the assignment, so the render condition no longer needs `&& orchestratorSession` to satisfy the type checker. Addresses review feedback on #1613. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Claude Opus 4.7 --- .../web/src/components/ProjectSidebar.tsx | 26 +++++- .../__tests__/ProjectSidebar.test.tsx | 93 +++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/packages/web/src/components/ProjectSidebar.tsx b/packages/web/src/components/ProjectSidebar.tsx index 8164f7c87..7d810c836 100644 --- a/packages/web/src/components/ProjectSidebar.tsx +++ b/packages/web/src/components/ProjectSidebar.tsx @@ -6,7 +6,7 @@ import { useRouter } from "next/navigation"; import { cn } from "@/lib/cn"; import type { ProjectInfo } from "@/lib/project-name"; import { getAttentionLevel, type DashboardSession, type AttentionLevel } from "@/lib/types"; -import { isOrchestratorSession } from "@aoagents/ao-core/types"; +import { isOrchestratorSession, isTerminalSession } from "@aoagents/ao-core/types"; import { getSessionTitle, humanizeBranch } from "@/lib/format"; import { usePopoverClamp } from "@/hooks/usePopoverClamp"; import { getOrchestratorSessionId } from "@/lib/orchestrator-utils"; @@ -397,6 +397,14 @@ function ProjectSidebarInner({ const orchestratorSession = sessions?.find( (s) => s.projectId === project.id && s.id === canonicalOrchestratorId, ); + const liveOrchestrator = + orchestratorSession && + !isTerminalSession({ + status: orchestratorSession.status, + activity: orchestratorSession.activity, + }) + ? orchestratorSession + : null; return (
@@ -563,6 +571,22 @@ function ProjectSidebarInner({ role="menu" aria-label={`${project.name} actions`} > + {liveOrchestrator ? ( + + ) : null}
- + @@ -509,8 +513,8 @@ function TerminalTestPageContent() {
  • Discovered XDA queries - Found that tmux sends{" "} - CSI > q (XTVERSION) to - detect terminal type + CSI > q (XTVERSION) + to detect terminal type
  • Traced the "iTerm2 magic" - Realized why clipboard worked when @@ -572,8 +576,8 @@ function TerminalTestPageContent() {
  • xterm.js parser API documentation
  • XTerm Control Sequences: XTVERSION / Device Attributes
  • - tmux tty-keys.c: Terminal - type detection logic + tmux tty-keys.c: + Terminal type detection logic
    • diff --git a/packages/web/src/app/globals.css b/packages/web/src/app/globals.css index bd833ab92..efdd58cf9 100644 --- a/packages/web/src/app/globals.css +++ b/packages/web/src/app/globals.css @@ -5760,7 +5760,7 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { /* -- Kanban board: stack columns vertically -- */ .kanban-board { - flex-direction: column; + grid-template-columns: minmax(0, 1fr); height: auto; min-height: auto; overflow-x: visible; diff --git a/packages/web/src/app/test-direct/page.tsx b/packages/web/src/app/test-direct/page.tsx index 06a8ab4ee..fc05d2189 100644 --- a/packages/web/src/app/test-direct/page.tsx +++ b/packages/web/src/app/test-direct/page.tsx @@ -81,6 +81,7 @@ function TestDirectPageContent() { diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index 42649b949..1b3ab1140 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -59,21 +59,15 @@ export function DirectTerminal({ const [fullscreen, setFullscreen] = useState(startFullscreen); const [fontSize, setFontSize] = useState(getStoredFontSize()); - const { - error, - followOutput, - scrollToLatest, - muxStatus, - terminalInstance, - fitAddon, - } = useXtermTerminal(terminalRef, sessionId, { - appearance, - variant, - fontSize, - autoFocus, - projectId, - tmuxName, - }); + const { error, followOutput, scrollToLatest, muxStatus, terminalInstance, fitAddon } = + useXtermTerminal(terminalRef, sessionId, { + appearance, + variant, + fontSize, + autoFocus, + projectId, + tmuxName, + }); useFullscreenResize(fullscreen, sessionId, projectId, terminalInstance, fitAddon, terminalRef); @@ -122,15 +116,18 @@ export function DirectTerminal({ aria-label="Jump to latest" title="Jump to latest" > - + ) : null} -
    +
    ); diff --git a/packages/web/src/components/SessionDetail.tsx b/packages/web/src/components/SessionDetail.tsx index e416a954a..b244efa3b 100644 --- a/packages/web/src/components/SessionDetail.tsx +++ b/packages/web/src/components/SessionDetail.tsx @@ -3,11 +3,7 @@ import { useState, useEffect, useMemo, useCallback } from "react"; import { useRouter, useSearchParams } from "next/navigation"; import { useMediaQuery, MOBILE_BREAKPOINT } from "@/hooks/useMediaQuery"; -import { - type DashboardSession, - TERMINAL_STATUSES, - NON_RESTORABLE_STATUSES, -} from "@/lib/types"; +import { type DashboardSession, TERMINAL_STATUSES, NON_RESTORABLE_STATUSES } from "@/lib/types"; import dynamic from "next/dynamic"; import { getSessionTitle } from "@/lib/format"; import type { ProjectInfo } from "@/lib/project-name"; @@ -16,10 +12,7 @@ import { projectDashboardPath, projectSessionPath } from "@/lib/routes"; import { ProjectSidebar, type ProjectSidebarOrchestrator } from "./ProjectSidebar"; import { MobileBottomNav } from "./MobileBottomNav"; -import { - SessionDetailHeader, - type OrchestratorZones, -} from "./SessionDetailHeader"; +import { SessionDetailHeader, type OrchestratorZones } from "./SessionDetailHeader"; import { SessionEndedSummary } from "./SessionEndedSummary"; import { sessionActivityMeta } from "./session-detail-utils"; @@ -190,10 +183,7 @@ export function SessionDetail({ ) : null} {mobileSidebarOpen && ( -
    setMobileSidebarOpen(false)} - /> +
    setMobileSidebarOpen(false)} /> )}
    @@ -231,9 +221,7 @@ export function SessionDetail({ activeTab={isOrchestrator ? "orchestrator" : undefined} dashboardHref={dashboardHref} prsHref={ - session.projectId - ? `/?project=${encodeURIComponent(session.projectId)}&tab=prs` - : "/" + session.projectId ? `/?project=${encodeURIComponent(session.projectId)}&tab=prs` : "/" } showOrchestrator={!!orchestratorHref} orchestratorHref={orchestratorHref} diff --git a/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx b/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx index e6173371d..d234bba79 100644 --- a/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx +++ b/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx @@ -143,12 +143,16 @@ describe("DirectTerminal render", () => { }); it("renders the shared accent chrome for orchestrator terminals", async () => { - render(); - - await waitFor(() => - expect(screen.getByText("Connected")).toBeInTheDocument(), + render( + , ); + await waitFor(() => expect(screen.getByText("Connected")).toBeInTheDocument()); + expect(screen.getByText("ao-orchestrator")).toHaveStyle({ color: "var(--color-accent)" }); expect(screen.getByText("XDA")).toHaveStyle({ color: "var(--color-accent)" }); }); @@ -157,6 +161,7 @@ describe("DirectTerminal render", () => { render( , @@ -171,7 +176,13 @@ describe("DirectTerminal render", () => { }); it("switches the terminal shell between inline and fullscreen positioning", async () => { - const { container } = render(); + const { container } = render( + , + ); await waitFor(() => expect(screen.getByRole("button", { name: "fullscreen" })).toBeInTheDocument(), @@ -196,7 +207,7 @@ describe("DirectTerminal render", () => { }); it("passes projectId to fullscreen resize hook for scoped mux resize", () => { - render(); + render(); expect(useFullscreenResizeMock).toHaveBeenCalledWith( false, diff --git a/packages/web/src/components/terminal/useFullscreenResize.ts b/packages/web/src/components/terminal/useFullscreenResize.ts index 734f222ba..eb9b06020 100644 --- a/packages/web/src/components/terminal/useFullscreenResize.ts +++ b/packages/web/src/components/terminal/useFullscreenResize.ts @@ -97,5 +97,13 @@ export function useFullscreenResize( clearTimeout(timer1); clearTimeout(timer2); }; - }, [fullscreen, sessionId, projectId, resizeTerminalMux, containerRef, fitAddon, terminalInstance]); + }, [ + fullscreen, + sessionId, + projectId, + resizeTerminalMux, + containerRef, + fitAddon, + terminalInstance, + ]); } diff --git a/packages/web/src/components/terminal/useXtermTerminal.ts b/packages/web/src/components/terminal/useXtermTerminal.ts index 60dc8278a..1dc918847 100644 --- a/packages/web/src/components/terminal/useXtermTerminal.ts +++ b/packages/web/src/components/terminal/useXtermTerminal.ts @@ -69,6 +69,7 @@ export function useXtermTerminal( useEffect(() => { if (!terminalRef.current) return; + setError(null); // Dynamically import xterm.js to avoid SSR issues let mounted = true; diff --git a/packages/web/src/providers/MuxProvider.tsx b/packages/web/src/providers/MuxProvider.tsx index 8449a36c3..26e33171e 100644 --- a/packages/web/src/providers/MuxProvider.tsx +++ b/packages/web/src/providers/MuxProvider.tsx @@ -156,7 +156,8 @@ export function MuxProvider({ children }: { children: ReactNode }) { } } } else if (msg.type === "opened") { - // Terminal opened successfully — preserve existing tmuxName + // Terminal opened successfully. Preserve any tmuxName stored by + // openTerminal so reconnects keep using the exact attach target. if (!openedTerminalsRef.current.has(key)) { openedTerminalsRef.current.set(key, { id: msg.id, @@ -357,7 +358,16 @@ export function MuxProvider({ children }: { children: ReactNode }) { sessions, lastError, }), - [subscribeTerminal, writeTerminal, openTerminal, closeTerminal, resizeTerminal, status, sessions, lastError], + [ + subscribeTerminal, + writeTerminal, + openTerminal, + closeTerminal, + resizeTerminal, + status, + sessions, + lastError, + ], ); return {children}; diff --git a/packages/web/src/providers/__tests__/MuxProvider.test.tsx b/packages/web/src/providers/__tests__/MuxProvider.test.tsx index 737418f91..878c2aed5 100644 --- a/packages/web/src/providers/__tests__/MuxProvider.test.tsx +++ b/packages/web/src/providers/__tests__/MuxProvider.test.tsx @@ -282,12 +282,18 @@ describe("MuxProvider connection lifecycle", () => { act(() => ws1.simulateOpen()); expect(result.current.status).toBe("connected"); - act(() => result.current.openTerminal("session-abc")); + act(() => result.current.openTerminal("session-abc", "project-a", "tmux.session abc")); // Confirm open message sent on ws1 expect( ws1.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "open" && p.id === "session-abc"; + return ( + p.ch === "terminal" && + p.type === "open" && + p.id === "session-abc" && + p.projectId === "project-a" && + p.tmuxName === "tmux.session abc" + ); }), ).toBe(true); @@ -304,7 +310,13 @@ describe("MuxProvider connection lifecycle", () => { expect( ws2.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "open" && p.id === "session-abc"; + return ( + p.ch === "terminal" && + p.type === "open" && + p.id === "session-abc" && + p.projectId === "project-a" && + p.tmuxName === "tmux.session abc" + ); }), ).toBe(true); } finally { @@ -347,9 +359,17 @@ describe("MuxProvider message handling", () => { const received: string[] = []; act(() => { - result.current.subscribeTerminal("s1", (d) => received.push(d)); + result.current.subscribeTerminal("s1", (d) => received.push(d), "project-a"); }); - act(() => ws.simulateMessage({ ch: "terminal", id: "s1", type: "data", data: "hello" })); + act(() => + ws.simulateMessage({ + ch: "terminal", + id: "s1", + projectId: "project-a", + type: "data", + data: "hello", + }), + ); expect(received).toContain("hello"); }); @@ -363,7 +383,9 @@ describe("MuxProvider message handling", () => { const ws1 = MockWebSocket.instances[0]; act(() => ws1.simulateOpen()); - act(() => ws1.simulateMessage({ ch: "terminal", id: "s1", type: "opened" })); + act(() => + ws1.simulateMessage({ ch: "terminal", id: "s1", projectId: "project-a", type: "opened" }), + ); // Reconnect → s1 should be re-opened act(() => ws1.simulateClose()); @@ -376,7 +398,7 @@ describe("MuxProvider message handling", () => { expect( ws2.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.id === "s1"; + return p.ch === "terminal" && p.id === "s1" && p.projectId === "project-a"; }), ).toBe(true); } finally { @@ -389,9 +411,17 @@ describe("MuxProvider message handling", () => { const received: string[] = []; act(() => { - result.current.subscribeTerminal("s1", (d) => received.push(d)); + result.current.subscribeTerminal("s1", (d) => received.push(d), "project-a"); }); - act(() => ws.simulateMessage({ ch: "terminal", id: "s1", type: "exited", code: 1 })); + act(() => + ws.simulateMessage({ + ch: "terminal", + id: "s1", + projectId: "project-a", + type: "exited", + code: 1, + }), + ); expect(received.some((m) => m.includes("exited"))).toBe(true); }); @@ -405,8 +435,18 @@ describe("MuxProvider message handling", () => { const ws1 = MockWebSocket.instances[0]; act(() => ws1.simulateOpen()); - act(() => ws1.simulateMessage({ ch: "terminal", id: "s-x", type: "opened" })); - act(() => ws1.simulateMessage({ ch: "terminal", id: "s-x", type: "exited", code: 0 })); + act(() => + ws1.simulateMessage({ ch: "terminal", id: "s-x", projectId: "project-a", type: "opened" }), + ); + act(() => + ws1.simulateMessage({ + ch: "terminal", + id: "s-x", + projectId: "project-a", + type: "exited", + code: 0, + }), + ); // Reconnect — s-x should NOT be re-opened act(() => ws1.simulateClose()); @@ -418,7 +458,9 @@ describe("MuxProvider message handling", () => { const reopened = ws2.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.id === "s-x" && p.type === "open"; + return ( + p.ch === "terminal" && p.id === "s-x" && p.projectId === "project-a" && p.type === "open" + ); }); expect(reopened).toBe(false); } finally { @@ -431,7 +473,13 @@ describe("MuxProvider message handling", () => { const { result, ws } = await setupConnected(); act(() => - ws.simulateMessage({ ch: "terminal", id: "s1", type: "error", message: "PTY failed" }), + ws.simulateMessage({ + ch: "terminal", + id: "s1", + projectId: "project-a", + type: "error", + message: "PTY failed", + }), ); expect(spy).toHaveBeenCalledWith(expect.stringContaining("Terminal error"), expect.any(String)); expect(result.current.status).toBe("connected"); @@ -492,11 +540,17 @@ describe("MuxProvider terminal operations", () => { it("writeTerminal sends data message", async () => { const { result, ws } = await setupConnected(); - act(() => result.current.writeTerminal("s1", "hello\n")); + act(() => result.current.writeTerminal("s1", "hello\n", "project-a")); expect( ws.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "data" && p.data === "hello\n"; + return ( + p.ch === "terminal" && + p.type === "data" && + p.id === "s1" && + p.projectId === "project-a" && + p.data === "hello\n" + ); }), ).toBe(true); }); @@ -504,41 +558,59 @@ describe("MuxProvider terminal operations", () => { it("writeTerminal is no-op when WebSocket is not open", async () => { const { result } = renderHook(() => useMux(), { wrapper }); // Don't flush init or open the WebSocket - act(() => result.current.writeTerminal("s1", "hello\n")); + act(() => result.current.writeTerminal("s1", "hello\n", "project-a")); // No crash expect(result.current.status).toBe("connecting"); }); it("openTerminal sends open message when connected", async () => { const { result, ws } = await setupConnected(); - act(() => result.current.openTerminal("session-abc")); + act(() => result.current.openTerminal("session-abc", "project-a", "tmux.session abc")); expect( ws.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "open" && p.id === "session-abc"; + return ( + p.ch === "terminal" && + p.type === "open" && + p.id === "session-abc" && + p.projectId === "project-a" && + p.tmuxName === "tmux.session abc" + ); }), ).toBe(true); }); it("closeTerminal sends close message", async () => { const { result, ws } = await setupConnected(); - act(() => result.current.openTerminal("session-abc")); - act(() => result.current.closeTerminal("session-abc")); + act(() => result.current.openTerminal("session-abc", "project-a", "tmux.session abc")); + act(() => result.current.closeTerminal("session-abc", "project-a")); expect( ws.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "close" && p.id === "session-abc"; + return ( + p.ch === "terminal" && + p.type === "close" && + p.id === "session-abc" && + p.projectId === "project-a" + ); }), ).toBe(true); }); it("resizeTerminal sends resize message with cols and rows", async () => { const { result, ws } = await setupConnected(); - act(() => result.current.resizeTerminal("session-abc", 120, 40)); + act(() => result.current.resizeTerminal("session-abc", 120, 40, "project-a")); expect( ws.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "resize" && p.cols === 120 && p.rows === 40; + return ( + p.ch === "terminal" && + p.type === "resize" && + p.id === "session-abc" && + p.projectId === "project-a" && + p.cols === 120 && + p.rows === 40 + ); }), ).toBe(true); }); @@ -546,12 +618,17 @@ describe("MuxProvider terminal operations", () => { it("subscribeTerminal sends open for untracked terminal", async () => { const { result, ws } = await setupConnected(); act(() => { - result.current.subscribeTerminal("session-new", () => {}); + result.current.subscribeTerminal("session-new", () => {}, "project-a"); }); expect( ws.sentMessages.some((m) => { const p = JSON.parse(m) as Record; - return p.ch === "terminal" && p.type === "open" && p.id === "session-new"; + return ( + p.ch === "terminal" && + p.type === "open" && + p.id === "session-new" && + p.projectId === "project-a" + ); }), ).toBe(true); }); @@ -562,11 +639,27 @@ describe("MuxProvider terminal operations", () => { let unsub!: () => void; act(() => { - unsub = result.current.subscribeTerminal("s1", (d) => received.push(d)); + unsub = result.current.subscribeTerminal("s1", (d) => received.push(d), "project-a"); }); - act(() => ws.simulateMessage({ ch: "terminal", id: "s1", type: "data", data: "before" })); + act(() => + ws.simulateMessage({ + ch: "terminal", + id: "s1", + projectId: "project-a", + type: "data", + data: "before", + }), + ); act(() => unsub()); - act(() => ws.simulateMessage({ ch: "terminal", id: "s1", type: "data", data: "after" })); + act(() => + ws.simulateMessage({ + ch: "terminal", + id: "s1", + projectId: "project-a", + type: "data", + data: "after", + }), + ); expect(received).toContain("before"); expect(received).not.toContain("after"); From eb06a4d090fd5d9e09ae7ae204c68ab1dcc9aba0 Mon Sep 17 00:00:00 2001 From: Harsh Batheja <40922251+harsh-batheja@users.noreply.github.com> Date: Mon, 4 May 2026 20:13:11 +0530 Subject: [PATCH 13/30] fix(web): render empty-state in sidebar when no projects configured (#1549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): render empty-state in sidebar when no projects configured A fresh-install user with zero projects saw a blank sidebar and had no way to open AddProjectModal from it. The early-return was originally projects.length <= 1 (#381), softened to === 0 in #927, but no empty- state UI was added at the same time. Replace the null branch with a small ProjectSidebarEmpty sibling that reuses the existing header (with the + button wired to AddProjectModal), shows a one-line explainer, and renders only the ThemeToggle in the footer (the show-killed/show-done/settings buttons are meaningless with zero projects). * fix(web): mark sidebar + button SVGs aria-hidden The decorative SVG inside the labeled + buttons (empty-state and populated sidebar) should not be announced — screen readers should rely on the button's aria-label. Adds aria-hidden="true" to both for consistency. * fix(web): always mount sidebar so empty-state renders on fresh installs Dashboard previously gated the sidebar on projects.length >= 1, leaving ProjectSidebarEmpty unreachable. ProjectSidebar handles both cases now, so drop the gate and add a dashboard-level test for the zero-project path. * fix(web): honor collapsed prop in empty sidebar branch ProjectSidebarEmpty discarded the collapsed prop, so on a fresh install the wrapper shrank to 44px while the inner sidebar stayed 224px and overlapped the main content. Render a 44px-wide rail with just the + button when collapsed, matching the populated sidebar's collapse path. --- packages/web/src/components/Dashboard.tsx | 101 +++++++++--------- .../web/src/components/ProjectSidebar.tsx | 71 +++++++++++- .../__tests__/Dashboard.emptyState.test.tsx | 11 ++ .../__tests__/ProjectSidebar.test.tsx | 66 +++++++++++- 4 files changed, 191 insertions(+), 58 deletions(-) diff --git a/packages/web/src/components/Dashboard.tsx b/packages/web/src/components/Dashboard.tsx index b66f7dc00..f9dd25245 100644 --- a/packages/web/src/components/Dashboard.tsx +++ b/packages/web/src/components/Dashboard.tsx @@ -197,13 +197,12 @@ function DashboardInner({ const showDebugBundleButton = !isMobile && (process.env.NODE_ENV === "development" || debugParam === "1" || debugParam === "true"); - const showSidebar = projects.length >= 1; const { showToast } = useToast(); const [doneExpanded, setDoneExpanded] = useState(false); const sessionsRef = useRef(sessions); sessionsRef.current = sessions; - const allProjectsView = projects.length > 1 && showSidebar && projectId === undefined; + const allProjectsView = projects.length > 1 && projectId === undefined; const currentProjectOrchestrator = useMemo( () => projectId @@ -495,41 +494,39 @@ function DashboardInner({
    - {showSidebar ? ( - - ) : null} +