diff --git a/src/__tests__/main/cue/cue-cleanup-service.test.ts b/src/__tests__/main/cue/cue-cleanup-service.test.ts new file mode 100644 index 000000000..6173ecddc --- /dev/null +++ b/src/__tests__/main/cue/cue-cleanup-service.test.ts @@ -0,0 +1,248 @@ +/** + * Unit tests for CueCleanupService. + * + * Tests cover: + * - No-op sweep when nothing is stale + * - Eviction of fan-in trackers for removed sessions + * - Eviction of fan-in trackers exceeding 2× timeout + * - Non-eviction of recent fan-in trackers + * - Eviction of stale scheduled dedup keys + * - onTick cadence (sweeps only every CLEANUP_INTERVAL_TICKS ticks) + * - sweep() can be called directly (bypasses tick counter) + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { + createCueCleanupService, + CLEANUP_INTERVAL_TICKS, + type CueCleanupServiceDeps, +} from '../../../main/cue/cue-cleanup-service'; +import type { CueFanInTracker } from '../../../main/cue/cue-fan-in-tracker'; +import type { CueSessionRegistry } from '../../../main/cue/cue-session-registry'; + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function makeMockTracker(overrides: Partial = {}): CueFanInTracker { + return { + handleCompletion: vi.fn(), + clearForSession: vi.fn(), + reset: vi.fn(), + getActiveTrackerKeys: vi.fn(() => []), + getTrackerCreatedAt: vi.fn(() => undefined), + expireTracker: vi.fn(), + ...overrides, + }; +} + +function makeMockRegistry(overrides: Partial = {}): CueSessionRegistry { + return { + register: vi.fn(), + unregister: vi.fn(), + get: vi.fn(() => undefined), + has: vi.fn(() => false), + snapshot: vi.fn(() => new Map()), + size: vi.fn(() => 0), + markScheduledFired: vi.fn(() => true), + evictStaleScheduledKeys: vi.fn(), + clearScheduledForSession: vi.fn(), + markStartupFired: vi.fn(() => true), + clearStartupForSession: vi.fn(), + clear: vi.fn(), + sweepStaleScheduledKeys: vi.fn(() => 0), + ...overrides, + }; +} + +function makeDeps(overrides: Partial = {}): CueCleanupServiceDeps { + return { + fanInTracker: makeMockTracker(), + registry: makeMockRegistry(), + getSessions: vi.fn(() => []), + getSessionTimeoutMs: vi.fn(() => 30 * 60 * 1000), + getCurrentMinute: vi.fn(() => '09:00'), + onLog: vi.fn(), + ...overrides, + }; +} + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe('createCueCleanupService', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + describe('sweep — no-op cases', () => { + it('returns zero counts when no trackers or keys exist', () => { + const deps = makeDeps(); + const service = createCueCleanupService(deps); + const result = service.sweep(); + expect(result).toEqual({ fanInEvicted: 0, scheduledKeysEvicted: 0 }); + }); + + it('does not evict a recent fan-in tracker', () => { + const tracker = makeMockTracker({ + getActiveTrackerKeys: vi.fn(() => ['session-1:sub-a']), + getTrackerCreatedAt: vi.fn(() => Date.now() - 1000), // 1 second old + expireTracker: vi.fn(), + }); + const deps = makeDeps({ + fanInTracker: tracker, + getSessions: () => [{ id: 'session-1' }], + getSessionTimeoutMs: () => 30 * 60 * 1000, // 30 min — 2× = 60 min + }); + const service = createCueCleanupService(deps); + const result = service.sweep(); + + expect(tracker.expireTracker).not.toHaveBeenCalled(); + expect(result.fanInEvicted).toBe(0); + }); + }); + + describe('sweep — fan-in eviction', () => { + it('evicts a fan-in tracker whose owner session is no longer active', () => { + const tracker = makeMockTracker({ + getActiveTrackerKeys: vi.fn(() => ['removed-session:sub-a']), + getTrackerCreatedAt: vi.fn(() => Date.now()), + expireTracker: vi.fn(), + }); + const deps = makeDeps({ + fanInTracker: tracker, + getSessions: () => [], // removed-session is gone + }); + const service = createCueCleanupService(deps); + const result = service.sweep(); + + expect(tracker.expireTracker).toHaveBeenCalledWith('removed-session:sub-a'); + expect(result.fanInEvicted).toBe(1); + expect(deps.onLog).toHaveBeenCalledWith('warn', expect.stringContaining('removed session')); + }); + + it('evicts a fan-in tracker whose age exceeds 2× the session timeout', () => { + const timeoutMs = 30 * 60 * 1000; // 30 minutes + const createdAt = Date.now() - 3 * timeoutMs; // 90 minutes ago > 2× timeout + const tracker = makeMockTracker({ + getActiveTrackerKeys: vi.fn(() => ['session-1:sub-a']), + getTrackerCreatedAt: vi.fn(() => createdAt), + expireTracker: vi.fn(), + }); + const deps = makeDeps({ + fanInTracker: tracker, + getSessions: () => [{ id: 'session-1' }], + getSessionTimeoutMs: () => timeoutMs, + }); + const service = createCueCleanupService(deps); + const result = service.sweep(); + + expect(tracker.expireTracker).toHaveBeenCalledWith('session-1:sub-a'); + expect(result.fanInEvicted).toBe(1); + expect(deps.onLog).toHaveBeenCalledWith('warn', expect.stringContaining('2× timeout')); + }); + + it('handles a key without a colon (edge case) by treating the whole key as session ID', () => { + const tracker = makeMockTracker({ + getActiveTrackerKeys: vi.fn(() => ['orphaned-key']), + getTrackerCreatedAt: vi.fn(() => undefined), + expireTracker: vi.fn(), + }); + const deps = makeDeps({ + fanInTracker: tracker, + getSessions: () => [], // no sessions + }); + const service = createCueCleanupService(deps); + const result = service.sweep(); + + // Should be evicted because "orphaned-key" is not in active sessions + expect(tracker.expireTracker).toHaveBeenCalledWith('orphaned-key'); + expect(result.fanInEvicted).toBe(1); + }); + }); + + describe('sweep — scheduled key eviction', () => { + it('reports evicted scheduled key count from registry.sweepStaleScheduledKeys', () => { + const registry = makeMockRegistry({ + sweepStaleScheduledKeys: vi.fn(() => 3), + }); + const deps = makeDeps({ registry }); + const service = createCueCleanupService(deps); + const result = service.sweep(); + + expect(result.scheduledKeysEvicted).toBe(3); + expect(registry.sweepStaleScheduledKeys).toHaveBeenCalledWith('09:00'); + expect(deps.onLog).toHaveBeenCalledWith( + 'info', + expect.stringContaining('3 stale scheduled key') + ); + }); + + it('passes the current minute from getCurrentMinute to the registry sweep', () => { + const registry = makeMockRegistry({ sweepStaleScheduledKeys: vi.fn(() => 0) }); + const deps = makeDeps({ + registry, + getCurrentMinute: () => '14:32', + }); + const service = createCueCleanupService(deps); + service.sweep(); + + expect(registry.sweepStaleScheduledKeys).toHaveBeenCalledWith('14:32'); + }); + }); + + describe('onTick cadence', () => { + it('does not sweep before CLEANUP_INTERVAL_TICKS ticks', () => { + const registry = makeMockRegistry({ sweepStaleScheduledKeys: vi.fn(() => 0) }); + const tracker = makeMockTracker({ getActiveTrackerKeys: vi.fn(() => []) }); + const deps = makeDeps({ registry, fanInTracker: tracker }); + const service = createCueCleanupService(deps); + + for (let i = 0; i < CLEANUP_INTERVAL_TICKS - 1; i++) { + service.onTick(); + } + + expect(tracker.getActiveTrackerKeys).not.toHaveBeenCalled(); + expect(registry.sweepStaleScheduledKeys).not.toHaveBeenCalled(); + }); + + it('triggers a sweep on exactly the Nth tick', () => { + const registry = makeMockRegistry({ sweepStaleScheduledKeys: vi.fn(() => 0) }); + const tracker = makeMockTracker({ getActiveTrackerKeys: vi.fn(() => []) }); + const deps = makeDeps({ registry, fanInTracker: tracker }); + const service = createCueCleanupService(deps); + + for (let i = 0; i < CLEANUP_INTERVAL_TICKS; i++) { + service.onTick(); + } + + expect(tracker.getActiveTrackerKeys).toHaveBeenCalledTimes(1); + expect(registry.sweepStaleScheduledKeys).toHaveBeenCalledTimes(1); + }); + + it('sweeps again after another full interval', () => { + const registry = makeMockRegistry({ sweepStaleScheduledKeys: vi.fn(() => 0) }); + const tracker = makeMockTracker({ getActiveTrackerKeys: vi.fn(() => []) }); + const deps = makeDeps({ registry, fanInTracker: tracker }); + const service = createCueCleanupService(deps); + + for (let i = 0; i < CLEANUP_INTERVAL_TICKS * 2; i++) { + service.onTick(); + } + + expect(registry.sweepStaleScheduledKeys).toHaveBeenCalledTimes(2); + }); + }); + + describe('sweep — direct invocation', () => { + it('sweep() bypasses the tick counter and runs immediately', () => { + const registry = makeMockRegistry({ sweepStaleScheduledKeys: vi.fn(() => 0) }); + const tracker = makeMockTracker({ getActiveTrackerKeys: vi.fn(() => []) }); + const deps = makeDeps({ registry, fanInTracker: tracker }); + const service = createCueCleanupService(deps); + + // No ticks fired — sweep still runs when called directly + service.sweep(); + + expect(tracker.getActiveTrackerKeys).toHaveBeenCalledTimes(1); + expect(registry.sweepStaleScheduledKeys).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/src/__tests__/main/cue/cue-completion-chains.test.ts b/src/__tests__/main/cue/cue-completion-chains.test.ts index 8415b9552..021cae824 100644 --- a/src/__tests__/main/cue/cue-completion-chains.test.ts +++ b/src/__tests__/main/cue/cue-completion-chains.test.ts @@ -44,6 +44,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ pruneCueEvents: vi.fn(), recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock reconciler (not exercised in these tests, but avoids heavy imports) diff --git a/src/__tests__/main/cue/cue-concurrency.test.ts b/src/__tests__/main/cue/cue-concurrency.test.ts index 4a9b2621f..48b27b968 100644 --- a/src/__tests__/main/cue/cue-concurrency.test.ts +++ b/src/__tests__/main/cue/cue-concurrency.test.ts @@ -42,6 +42,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ isCueDbReady: () => true, recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock crypto diff --git a/src/__tests__/main/cue/cue-db.test.ts b/src/__tests__/main/cue/cue-db.test.ts index 798e66dce..0f1c180e3 100644 --- a/src/__tests__/main/cue/cue-db.test.ts +++ b/src/__tests__/main/cue/cue-db.test.ts @@ -82,6 +82,8 @@ import { hasAnyGitHubSeen, pruneGitHubSeen, clearGitHubSeenForSubscription, + safeRecordCueEvent, + safeUpdateCueEventStatus, } from '../../../main/cue/cue-db'; beforeEach(() => { @@ -418,3 +420,76 @@ describe('cue-db github seen tracking', () => { expect(lastRun[0]).toBe('sub-1'); }); }); + +describe('safeRecordCueEvent', () => { + const dbPath = path.join(os.tmpdir(), 'test-cue-safe.db'); + + beforeEach(() => { + initCueDb(undefined, dbPath); + vi.clearAllMocks(); + runCalls.length = 0; + prepareCalls.length = 0; + }); + + const testEvent = { + id: 'safe-evt-1', + type: 'time.heartbeat', + triggerName: 'test-trigger', + sessionId: 'session-1', + subscriptionName: 'test-sub', + status: 'running', + } as const; + + it('calls through successfully when DB is ready', () => { + safeRecordCueEvent(testEvent); + expect(mockDb.prepare).toHaveBeenCalledWith( + expect.stringContaining('INSERT OR REPLACE INTO cue_events') + ); + }); + + it('logs warn and does not throw when underlying function throws', () => { + mockStatement.run.mockImplementationOnce(() => { + throw new Error('DB locked'); + }); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(() => safeRecordCueEvent(testEvent)).not.toThrow(); + consoleSpy.mockRestore(); + }); + + it('does not throw when DB is unavailable (not initialized)', () => { + closeCueDb(); + expect(() => safeRecordCueEvent(testEvent)).not.toThrow(); + }); +}); + +describe('safeUpdateCueEventStatus', () => { + const dbPath = path.join(os.tmpdir(), 'test-cue-safe-update.db'); + + beforeEach(() => { + initCueDb(undefined, dbPath); + vi.clearAllMocks(); + runCalls.length = 0; + prepareCalls.length = 0; + }); + + it('calls through successfully when DB is ready', () => { + safeUpdateCueEventStatus('evt-1', 'completed'); + expect(mockDb.prepare).toHaveBeenCalledWith( + expect.stringContaining('UPDATE cue_events SET status') + ); + }); + + it('logs warn and does not throw when underlying function throws', () => { + mockStatement.run.mockImplementationOnce(() => { + throw new Error('DB locked'); + }); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(() => safeUpdateCueEventStatus('evt-1', 'completed')).not.toThrow(); + consoleSpy.mockRestore(); + }); + + it('does not throw when DB is unavailable (not initialized)', () => { + closeCueDb(); + expect(() => safeUpdateCueEventStatus('evt-1', 'completed')).not.toThrow(); + }); +}); diff --git a/src/__tests__/main/cue/cue-engine.test.ts b/src/__tests__/main/cue/cue-engine.test.ts index b5de595b5..93a2470d6 100644 --- a/src/__tests__/main/cue/cue-engine.test.ts +++ b/src/__tests__/main/cue/cue-engine.test.ts @@ -60,6 +60,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ isCueDbReady: () => true, recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock crypto @@ -1618,7 +1620,10 @@ describe('CueEngine', () => { expect(graph[0].sessionId).toBe('session-1'); }); - it('includes subscriptions with agent_id targeting a different session', () => { + it('filters out subscriptions whose agent_id targets a different session', () => { + // getGraphData reports only subscriptions that belong to each session + // (agent_id matches, or agent_id absent) so the pipeline editor never + // sees a subscription under an unrelated session. const config = createMockConfig({ subscriptions: [ { @@ -1644,13 +1649,14 @@ describe('CueEngine', () => { const graph = engine.getGraphData(); expect(graph).toHaveLength(1); - expect(graph[0].subscriptions).toHaveLength(2); - expect(graph[0].subscriptions.map((s) => s.name)).toEqual(['step-a', 'step-b']); + // Only step-a (no agent_id, so unbound) is reported — step-b targets a + // different session. + expect(graph[0].subscriptions.map((s) => s.name)).toEqual(['step-a']); engine.stop(); }); - it('includes subscriptions with foreign agent_id when loading from disk', () => { + it('filters foreign-agent_id subscriptions when loading from disk (engine disabled)', () => { const config = createMockConfig({ subscriptions: [ { @@ -1676,11 +1682,12 @@ describe('CueEngine', () => { const graph = engine.getGraphData(); expect(graph).toHaveLength(1); - expect(graph[0].subscriptions).toHaveLength(2); - expect(graph[0].subscriptions.map((s) => s.name)).toEqual(['trigger', 'downstream']); + // Only `trigger` (unbound) is reported — `downstream` is bound to a + // session that doesn't exist in this view. + expect(graph[0].subscriptions.map((s) => s.name)).toEqual(['trigger']); }); - it('returns all subscriptions from multiple sessions sharing the same config', () => { + it('scopes subscriptions to their owning session when multiple sessions share a config', () => { const config = createMockConfig({ subscriptions: [ { @@ -1712,9 +1719,10 @@ describe('CueEngine', () => { const graph = engine.getGraphData(); expect(graph).toHaveLength(2); - // Each session should report ALL subscriptions (not just its own) - expect(graph[0].subscriptions).toHaveLength(2); - expect(graph[1].subscriptions).toHaveLength(2); + // Each session reports only its own subscriptions (agent_id match). + const byId = new Map(graph.map((g) => [g.sessionId, g])); + expect(byId.get('session-1')!.subscriptions.map((s) => s.name)).toEqual(['step-a']); + expect(byId.get('session-2')!.subscriptions.map((s) => s.name)).toEqual(['step-b']); engine.stop(); }); diff --git a/src/__tests__/main/cue/cue-executor.test.ts b/src/__tests__/main/cue/cue-executor.test.ts index 5dd07d93b..a0fbaa677 100644 --- a/src/__tests__/main/cue/cue-executor.test.ts +++ b/src/__tests__/main/cue/cue-executor.test.ts @@ -399,7 +399,10 @@ describe('cue-executor', () => { expect.any(Array), expect.objectContaining({ cwd: '/projects/test', - stdio: ['pipe', 'pipe', 'pipe'], + // Local mode uses 'ignore' for stdin so agents like Codex don't + // emit "Reading additional input from stdin..." into the run + // output before observing EOF. + stdio: ['ignore', 'pipe', 'pipe'], }) ); diff --git a/src/__tests__/main/cue/cue-fan-in-tracker.test.ts b/src/__tests__/main/cue/cue-fan-in-tracker.test.ts new file mode 100644 index 000000000..364c6e1f8 --- /dev/null +++ b/src/__tests__/main/cue/cue-fan-in-tracker.test.ts @@ -0,0 +1,237 @@ +/** + * Unit tests for CueFanInTracker — focused on the three new methods added + * in Phase 8C: getActiveTrackerKeys, getTrackerCreatedAt, expireTracker, + * plus the lifecycle cleanup of fanInCreatedAt in clearForSession and reset. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { + CueSettings, + CueSubscription, + AgentCompletionData, +} from '../../../main/cue/cue-types'; +import { createCueFanInTracker } from '../../../main/cue/cue-fan-in-tracker'; + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function makeSub(overrides: Partial = {}): CueSubscription { + return { + name: 'fan-in-sub', + event: 'agent.completed', + enabled: true, + prompt: 'compile results', + source_sessions: ['session-a', 'session-b'], + ...overrides, + }; +} + +function makeSettings(overrides: Partial = {}): CueSettings { + return { + timeout_minutes: 30, + timeout_on_fail: 'break', + max_concurrent: 1, + queue_size: 10, + ...overrides, + }; +} + +function makeCompletion(overrides: Partial = {}): AgentCompletionData { + return { + sessionName: 'agent-a', + status: 'completed', + exitCode: 0, + durationMs: 1000, + stdout: 'output from agent', + triggeredBy: 'fan-in-sub', + chainDepth: 0, + ...overrides, + }; +} + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe('CueFanInTracker — new inspection methods', () => { + let dispatch: ReturnType; + let onLog: ReturnType; + + beforeEach(() => { + dispatch = vi.fn(); + onLog = vi.fn(); + vi.useFakeTimers(); + }); + + function makeTracker() { + return createCueFanInTracker({ + onLog, + getSessions: () => [ + { id: 'session-a', name: 'Agent A', toolType: 'claude-code', cwd: '/', projectRoot: '/' }, + { id: 'session-b', name: 'Agent B', toolType: 'claude-code', cwd: '/', projectRoot: '/' }, + ], + dispatchSubscription: dispatch, + }); + } + + describe('getActiveTrackerKeys', () => { + it('returns empty array when no trackers are active', () => { + const tracker = makeTracker(); + expect(tracker.getActiveTrackerKeys()).toEqual([]); + }); + + it('returns the key after the first completion arrives', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + + tracker.handleCompletion( + 'owner-session', + settings, + sub, + ['session-a', 'session-b'], + 'session-a', + 'Agent A', + makeCompletion() + ); + + expect(tracker.getActiveTrackerKeys()).toEqual(['owner-session:fan-in-sub']); + }); + + it('removes the key after all sources complete (fan-in fires)', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + const sources = ['session-a', 'session-b']; + + tracker.handleCompletion( + 'owner', + settings, + sub, + sources, + 'session-a', + 'Agent A', + makeCompletion() + ); + tracker.handleCompletion( + 'owner', + settings, + sub, + sources, + 'session-b', + 'Agent B', + makeCompletion() + ); + + // Fan-in fired — no more active trackers + expect(tracker.getActiveTrackerKeys()).toEqual([]); + }); + }); + + describe('getTrackerCreatedAt', () => { + it('returns undefined for an unknown key', () => { + const tracker = makeTracker(); + expect(tracker.getTrackerCreatedAt('nonexistent:key')).toBeUndefined(); + }); + + it('returns the timestamp set when the first completion arrives', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + const before = Date.now(); + + tracker.handleCompletion( + 'owner-session', + settings, + sub, + ['session-a', 'session-b'], + 'session-a', + 'Agent A', + makeCompletion() + ); + + const createdAt = tracker.getTrackerCreatedAt('owner-session:fan-in-sub'); + expect(createdAt).toBeGreaterThanOrEqual(before); + expect(createdAt).toBeLessThanOrEqual(Date.now()); + }); + }); + + describe('expireTracker', () => { + it('removes the tracker and its timer without dispatching', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + + tracker.handleCompletion( + 'owner', + settings, + sub, + ['session-a', 'session-b'], + 'session-a', + 'Agent A', + makeCompletion() + ); + + const key = 'owner:fan-in-sub'; + expect(tracker.getActiveTrackerKeys()).toContain(key); + + tracker.expireTracker(key); + + expect(tracker.getActiveTrackerKeys()).not.toContain(key); + expect(tracker.getTrackerCreatedAt(key)).toBeUndefined(); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('is a no-op for an unknown key', () => { + const tracker = makeTracker(); + expect(() => tracker.expireTracker('nonexistent:key')).not.toThrow(); + }); + }); + + describe('clearForSession — cleans up fanInCreatedAt', () => { + it('removes createdAt entry when the owning session is cleared', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + + tracker.handleCompletion( + 'owner', + settings, + sub, + ['session-a', 'session-b'], + 'session-a', + 'Agent A', + makeCompletion() + ); + + expect(tracker.getTrackerCreatedAt('owner:fan-in-sub')).toBeDefined(); + + tracker.clearForSession('owner'); + + expect(tracker.getTrackerCreatedAt('owner:fan-in-sub')).toBeUndefined(); + expect(tracker.getActiveTrackerKeys()).toEqual([]); + }); + }); + + describe('reset — clears all fanInCreatedAt entries', () => { + it('removes all createdAt entries on reset', () => { + const tracker = makeTracker(); + const sub = makeSub(); + const settings = makeSettings(); + + tracker.handleCompletion( + 'owner', + settings, + sub, + ['session-a', 'session-b'], + 'session-a', + 'Agent A', + makeCompletion() + ); + + expect(tracker.getActiveTrackerKeys()).toHaveLength(1); + + tracker.reset(); + + expect(tracker.getActiveTrackerKeys()).toEqual([]); + expect(tracker.getTrackerCreatedAt('owner:fan-in-sub')).toBeUndefined(); + }); + }); +}); diff --git a/src/__tests__/main/cue/cue-ipc-handlers.test.ts b/src/__tests__/main/cue/cue-ipc-handlers.test.ts index 4dacc6c56..bc56185b4 100644 --- a/src/__tests__/main/cue/cue-ipc-handlers.test.ts +++ b/src/__tests__/main/cue/cue-ipc-handlers.test.ts @@ -51,6 +51,7 @@ vi.mock('../../../main/cue/config/cue-config-repository', () => ({ writeCueConfigFile: vi.fn(), deleteCueConfigFile: vi.fn(), writeCuePromptFile: vi.fn(), + pruneOrphanedPromptFiles: vi.fn(() => []), })); vi.mock('../../../main/cue/pipeline-layout-store', () => ({ diff --git a/src/__tests__/main/cue/cue-multi-hop-chains.test.ts b/src/__tests__/main/cue/cue-multi-hop-chains.test.ts index fc7d3b3b5..a366783e7 100644 --- a/src/__tests__/main/cue/cue-multi-hop-chains.test.ts +++ b/src/__tests__/main/cue/cue-multi-hop-chains.test.ts @@ -42,6 +42,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ pruneCueEvents: vi.fn(), recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock reconciler diff --git a/src/__tests__/main/cue/cue-process-lifecycle.test.ts b/src/__tests__/main/cue/cue-process-lifecycle.test.ts index 608b8f325..d0471847d 100644 --- a/src/__tests__/main/cue/cue-process-lifecycle.test.ts +++ b/src/__tests__/main/cue/cue-process-lifecycle.test.ts @@ -115,12 +115,14 @@ describe('cue-process-lifecycle', () => { const resultPromise = runProcess('run-1', spec, createOptions()); await vi.advanceTimersByTimeAsync(0); + // Local mode: stdin is `'ignore'` so agents like Codex don't print + // "Reading additional input from stdin..." into the run output. expect(mockSpawn).toHaveBeenCalledWith( 'claude', ['--print', '--', 'test prompt'], expect.objectContaining({ cwd: '/projects/test', - stdio: ['pipe', 'pipe', 'pipe'], + stdio: ['ignore', 'pipe', 'pipe'], }) ); diff --git a/src/__tests__/main/cue/cue-run-manager.test.ts b/src/__tests__/main/cue/cue-run-manager.test.ts index 3acf169b3..3c6dc74a1 100644 --- a/src/__tests__/main/cue/cue-run-manager.test.ts +++ b/src/__tests__/main/cue/cue-run-manager.test.ts @@ -15,6 +15,8 @@ import type { CueEvent, CueRunResult, CueSettings } from '../../../main/cue/cue- vi.mock('../../../main/cue/cue-db', () => ({ recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); const mockCaptureException = vi.fn(); diff --git a/src/__tests__/main/cue/cue-session-lifecycle.test.ts b/src/__tests__/main/cue/cue-session-lifecycle.test.ts index 647b7bf3b..900977069 100644 --- a/src/__tests__/main/cue/cue-session-lifecycle.test.ts +++ b/src/__tests__/main/cue/cue-session-lifecycle.test.ts @@ -42,6 +42,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ pruneCueEvents: vi.fn(), recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock reconciler @@ -55,6 +57,8 @@ vi.mock('crypto', () => ({ })); import { CueEngine, type CueEngineDeps } from '../../../main/cue/cue-engine'; +import { createCueSessionRuntimeService } from '../../../main/cue/cue-session-runtime-service'; +import { createCueSessionRegistry } from '../../../main/cue/cue-session-registry'; import { createMockSession, createMockConfig, createMockDeps } from './cue-test-helpers'; describe('CueEngine session lifecycle', () => { @@ -580,6 +584,87 @@ describe('CueEngine session lifecycle', () => { engine.stop(); }); + it('initSession idempotency guard — calling twice logs warn and re-initializes cleanly', async () => { + // Use a config with no subscriptions so createTriggerSource is never invoked + const config = createMockConfig({ subscriptions: [] }); + mockLoadCueConfig.mockReturnValue(config); + + const yamlWatcherCleanup = vi.fn(); + mockWatchCueYaml.mockReturnValue(yamlWatcherCleanup); + + const registry = createCueSessionRegistry(); + const clearQueue = vi.fn(); + const clearFanInState = vi.fn(); + const onLog = vi.fn(); + + const service = createCueSessionRuntimeService({ + enabled: () => true, + getSessions: () => [createMockSession()], + onRefreshRequested: vi.fn(), + onLog, + registry, + dispatchSubscription: vi.fn(), + clearQueue, + clearFanInState, + }); + + const session = createMockSession(); + + // First initSession — normal registration + service.initSession(session, { reason: 'system-boot' }); + expect(registry.has(session.id)).toBe(true); + + // Second initSession — should trigger idempotency guard + service.initSession(session, { reason: 'user-toggle' }); + + // Guard must have logged a warning + expect(onLog).toHaveBeenCalledWith( + 'warn', + expect.stringContaining('initSession called for already-initialized session') + ); + + // Session is still registered after re-init (not left in broken state) + expect(registry.has(session.id)).toBe(true); + + // teardownSession was invoked (clearFanInState is called by teardown) + expect(clearFanInState).toHaveBeenCalledWith(session.id); + }); + + it('initSession idempotency guard — does not double-register the session in the registry', async () => { + // NOTE: this test uses an empty `subscriptions: []` config so it does + // NOT exercise trigger-source registration directly; it only verifies + // the registry-level dedupe behavior (calling initSession twice still + // leaves exactly one entry in the registry snapshot). A separate test + // would need a real subscription wired through createTriggerSource to + // assert non-duplication of trigger sources themselves. + const config = createMockConfig({ subscriptions: [] }); + mockLoadCueConfig.mockReturnValue(config); + mockWatchCueYaml.mockReturnValue(vi.fn()); + + const registry = createCueSessionRegistry(); + + const service = createCueSessionRuntimeService({ + enabled: () => true, + getSessions: () => [createMockSession()], + onRefreshRequested: vi.fn(), + onLog: vi.fn(), + registry, + dispatchSubscription: vi.fn(), + clearQueue: vi.fn(), + clearFanInState: vi.fn(), + }); + + const session = createMockSession(); + + // Call initSession twice + service.initSession(session, { reason: 'system-boot' }); + service.initSession(session, { reason: 'system-boot' }); + + // After two calls, session should appear exactly once in the registry + // (not duplicated). The registry snapshot size is 1. + expect(registry.snapshot().size).toBe(1); + }); + it('removeSession clears startup keys so re-adding the session can re-fire', () => { mockLoadCueConfig.mockReturnValue(makeStartupConfig()); const deps = createMockDeps(); diff --git a/src/__tests__/main/cue/cue-session-registry.test.ts b/src/__tests__/main/cue/cue-session-registry.test.ts index af3038170..e49e904d5 100644 --- a/src/__tests__/main/cue/cue-session-registry.test.ts +++ b/src/__tests__/main/cue/cue-session-registry.test.ts @@ -201,4 +201,46 @@ describe('cue-session-registry', () => { expect(registry.size()).toBe(0); }); }); + + describe('sweepStaleScheduledKeys', () => { + it('returns 0 and does nothing when no keys exist', () => { + const evicted = registry.sweepStaleScheduledKeys('09:00'); + expect(evicted).toBe(0); + }); + + it('does not evict a key that matches the current time', () => { + registry.markScheduledFired('s1', 'sub-1', '09:00'); + const evicted = registry.sweepStaleScheduledKeys('09:00'); + expect(evicted).toBe(0); + // Key still in set — re-firing is still deduped + expect(registry.markScheduledFired('s1', 'sub-1', '09:00')).toBe(false); + }); + + it('evicts keys whose time component differs from the current time', () => { + registry.markScheduledFired('s1', 'sub-1', '08:59'); + registry.markScheduledFired('s1', 'sub-1', '09:00'); + // At 09:01 — the 08:59 key is stale, the 09:00 key is also stale + const evicted = registry.sweepStaleScheduledKeys('09:01'); + expect(evicted).toBe(2); + // After eviction, both slots can fire again + expect(registry.markScheduledFired('s1', 'sub-1', '08:59')).toBe(true); + expect(registry.markScheduledFired('s1', 'sub-1', '09:00')).toBe(true); + }); + + it('evicts stale keys across multiple sessions and subscriptions', () => { + registry.markScheduledFired('s1', 'sub-a', '10:00'); + registry.markScheduledFired('s2', 'sub-b', '10:00'); + registry.markScheduledFired('s1', 'sub-a', '10:01'); + const evicted = registry.sweepStaleScheduledKeys('10:01'); + // The two 10:00 keys are stale; the 10:01 key is current + expect(evicted).toBe(2); + }); + + it('does not affect startup fired keys', () => { + registry.markStartupFired('s1', 'init'); + registry.sweepStaleScheduledKeys('09:00'); + // Startup key is unaffected — still deduped + expect(registry.markStartupFired('s1', 'init')).toBe(false); + }); + }); }); diff --git a/src/__tests__/main/cue/cue-sleep-prevention.test.ts b/src/__tests__/main/cue/cue-sleep-prevention.test.ts index bc07952e3..662705b84 100644 --- a/src/__tests__/main/cue/cue-sleep-prevention.test.ts +++ b/src/__tests__/main/cue/cue-sleep-prevention.test.ts @@ -51,6 +51,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ isCueDbReady: () => true, recordCueEvent: vi.fn(), updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock crypto diff --git a/src/__tests__/main/cue/cue-sleep-wake.test.ts b/src/__tests__/main/cue/cue-sleep-wake.test.ts index f5765a36e..e0868cacc 100644 --- a/src/__tests__/main/cue/cue-sleep-wake.test.ts +++ b/src/__tests__/main/cue/cue-sleep-wake.test.ts @@ -25,6 +25,10 @@ vi.mock('../../../main/cue/cue-db', () => ({ updateHeartbeat: () => mockUpdateHeartbeat(), getLastHeartbeat: () => mockGetLastHeartbeat(), pruneCueEvents: (...args: unknown[]) => mockPruneCueEvents(...args), + recordCueEvent: vi.fn(), + updateCueEventStatus: vi.fn(), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Track reconciler calls diff --git a/src/__tests__/main/cue/cue-startup.test.ts b/src/__tests__/main/cue/cue-startup.test.ts index 7e1ccef3a..fa1480b02 100644 --- a/src/__tests__/main/cue/cue-startup.test.ts +++ b/src/__tests__/main/cue/cue-startup.test.ts @@ -62,6 +62,8 @@ vi.mock('../../../main/cue/cue-db', () => ({ updateCueEventStatus: vi.fn(), updateHeartbeat: vi.fn(), getLastHeartbeat: vi.fn(() => null), + safeRecordCueEvent: vi.fn(), + safeUpdateCueEventStatus: vi.fn(), })); // Mock reconciler diff --git a/src/__tests__/main/cue/cue-yaml-loader.test.ts b/src/__tests__/main/cue/cue-yaml-loader.test.ts index 3aebcf46b..f3840c48b 100644 --- a/src/__tests__/main/cue/cue-yaml-loader.test.ts +++ b/src/__tests__/main/cue/cue-yaml-loader.test.ts @@ -334,23 +334,47 @@ subscriptions: } }); - it('returns { ok: false, reason: "invalid", errors } when validation fails', () => { + it('skips per-subscription validation errors and surfaces them as warnings', () => { + // Lenient loader: a single broken subscription must not block valid + // subs in the same YAML. The bad sub is dropped, others load, and + // the failure is surfaced as a warning so the user can fix it. mockExistsSync.mockReturnValue(true); mockReadFileSync.mockReturnValue(` subscriptions: - name: bad-sub event: time.heartbeat prompt: Hi + - name: good-sub + event: time.heartbeat + prompt: Check status + interval_minutes: 5 +`); + + const result = loadCueConfigDetailed('/projects/test'); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.config.subscriptions.map((s) => s.name)).toEqual(['good-sub']); + expect(result.warnings).toEqual( + expect.arrayContaining([ + expect.stringMatching(/Skipped invalid subscription.*interval_minutes/), + ]) + ); + } + }); + + it('returns { ok: false, reason: "invalid" } only for config-level errors', () => { + mockExistsSync.mockReturnValue(true); + mockReadFileSync.mockReturnValue(` +subscriptions: not-an-array `); - // Missing interval_minutes for time.heartbeat — validator rejects this. const result = loadCueConfigDetailed('/projects/test'); expect(result.ok).toBe(false); if (!result.ok && result.reason === 'invalid') { - expect(result.errors.length).toBeGreaterThan(0); expect(result.errors).toEqual( - expect.arrayContaining([expect.stringMatching(/interval_minutes/)]) + expect.arrayContaining([expect.stringMatching(/subscriptions/)]) ); } }); diff --git a/src/__tests__/renderer/components/CueModal.test.tsx b/src/__tests__/renderer/components/CueModal.test.tsx index 1acb499fa..542da1271 100644 --- a/src/__tests__/renderer/components/CueModal.test.tsx +++ b/src/__tests__/renderer/components/CueModal.test.tsx @@ -11,7 +11,8 @@ * - Unsaved changes confirmation on close */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { useCueDirtyStore } from '../../../renderer/stores/cueDirtyStore'; import { render, screen, fireEvent, act } from '@testing-library/react'; import { CueModal } from '../../../renderer/components/CueModal'; import type { Theme } from '../../../renderer/types'; @@ -41,12 +42,8 @@ vi.mock('../../../renderer/components/CueYamlEditor', () => ({ isOpen ?
YAML Editor Mock
: null, })); -// Capture the onDirtyChange callback from CuePipelineEditor -let capturedOnDirtyChange: ((isDirty: boolean) => void) | undefined; - vi.mock('../../../renderer/components/CuePipelineEditor', () => ({ - CuePipelineEditor: ({ onDirtyChange }: { onDirtyChange?: (isDirty: boolean) => void }) => { - capturedOnDirtyChange = onDirtyChange; + CuePipelineEditor: () => { return
Pipeline Editor Mock
; }, })); @@ -193,10 +190,13 @@ const mockFailedRun = { describe('CueModal', () => { const mockOnClose = vi.fn(); + afterEach(() => { + useCueDirtyStore.setState({ pipelineDirty: false, yamlDirty: false }); + }); + beforeEach(() => { vi.clearAllMocks(); mockUseCueReturn = { ...defaultUseCueReturn }; - capturedOnDirtyChange = undefined; }); describe('rendering', () => { @@ -505,9 +505,8 @@ describe('CueModal', () => { render(); // Simulate pipeline becoming dirty - expect(capturedOnDirtyChange).toBeDefined(); act(() => { - capturedOnDirtyChange!(true); + useCueDirtyStore.getState().setPipelineDirty(true); }); // Trigger escape (which goes through the same dirty check) @@ -532,7 +531,7 @@ describe('CueModal', () => { // Simulate pipeline becoming dirty act(() => { - capturedOnDirtyChange!(true); + useCueDirtyStore.getState().setPipelineDirty(true); }); // Trigger escape @@ -548,10 +547,10 @@ describe('CueModal', () => { // Simulate pipeline becoming dirty then saved act(() => { - capturedOnDirtyChange!(true); + useCueDirtyStore.getState().setPipelineDirty(true); }); act(() => { - capturedOnDirtyChange!(false); + useCueDirtyStore.getState().setPipelineDirty(false); }); // Trigger escape @@ -669,7 +668,7 @@ describe('CueModal', () => { // Simulate dirty pipeline act(() => { - capturedOnDirtyChange!(true); + useCueDirtyStore.getState().setPipelineDirty(true); }); // Trigger escape @@ -685,7 +684,7 @@ describe('CueModal', () => { // Make pipeline dirty act(() => { - capturedOnDirtyChange!(true); + useCueDirtyStore.getState().setPipelineDirty(true); }); // Enter help view diff --git a/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.allPipelinesViewLock.test.tsx b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.allPipelinesViewLock.test.tsx new file mode 100644 index 000000000..f26c0bcb3 --- /dev/null +++ b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.allPipelinesViewLock.test.tsx @@ -0,0 +1,452 @@ +/** + * All Pipelines view — read-only lock regression tests. + * + * In "All Pipelines" view (`selectedPipelineId === null`), the canvas must be + * completely locked: + * - Nodes cannot be dragged; drag-stop must not mutate canonical state. + * - Drag-drop from the trigger/agent drawers must not add nodes. + * - Edge connection attempts must not create edges. + * - `isValidConnection` must refuse every connection. + * - Keyboard Delete/Backspace must not delete the selected node/edge. + * - Right-click must not open the context menu. + * - Clicking a node/edge must not open the config panel for editing. + * - The per-node "Configure" callback must not set selection. + * + * These tests drive each entry point directly via the captured props and + * verify no canonical mutation (setPipelineState, onDeleteNode, onDeleteEdge) + * and no selection mutation (setSelectedNodeId, setSelectedEdgeId) happens. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, fireEvent } from '@testing-library/react'; +import React from 'react'; + +// ─── Captured props from PipelineCanvas ────────────────────────────────────── +let captured: { + onNodesChange: any; + onNodeDragStop: any; + onDrop: any; + onConnect: any; + isValidConnection: any; + onNodeClick: any; + onEdgeClick: any; + onNodeContextMenu: any; + handleConfigureNode: any; + isReadOnly: boolean | undefined; +} = { + onNodesChange: null, + onNodeDragStop: null, + onDrop: null, + onConnect: null, + isValidConnection: null, + onNodeClick: null, + onEdgeClick: null, + onNodeContextMenu: null, + handleConfigureNode: null, + isReadOnly: undefined, +}; + +vi.mock('reactflow', () => ({ + default: (props: any) =>
{props.children}
, + ReactFlowProvider: ({ children }: any) => <>{children}, + useReactFlow: () => ({ + fitView: vi.fn(), + screenToFlowPosition: vi.fn((pos: any) => pos), + setViewport: vi.fn(), + }), + useNodesInitialized: () => false, + applyNodeChanges: (_changes: any[], nodes: any[]) => nodes, + Background: () => null, + Controls: () => null, + MiniMap: () => null, + ConnectionMode: { Loose: 'loose' }, + Position: { Left: 'left', Right: 'right' }, + Handle: () => null, + MarkerType: { ArrowClosed: 'arrowclosed' }, +})); + +// Capture all interaction callbacks + the read-only flag that CuePipelineEditor +// passes down. The mock intentionally captures these at mount so tests can +// invoke them directly — much more reliable than simulating ReactFlow events. +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineCanvas', () => ({ + PipelineCanvas: React.memo((props: any) => { + captured.onNodesChange = props.onNodesChange; + captured.onNodeDragStop = props.onNodeDragStop; + captured.onDrop = props.onDrop; + captured.onConnect = props.onConnect; + captured.isValidConnection = props.isValidConnection; + captured.onNodeClick = props.onNodeClick; + captured.onEdgeClick = props.onEdgeClick; + captured.onNodeContextMenu = props.onNodeContextMenu; + captured.isReadOnly = props.isReadOnly; + return
; + }), +})); +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineToolbar', () => ({ + PipelineToolbar: () =>
, +})); +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineContextMenu', () => ({ + PipelineContextMenu: (props: any) => ( +
+ ), +})); + +// Hoisted — tests toggle this and re-render. `stableStateHook` returns the +// same object reference each call (avoids the setState loop the initial +// viewport test ran into). +const mockSetPipelineState = vi.fn(); +const mockOnDeleteNode = vi.fn(); +const mockOnDeleteEdge = vi.fn(); +const mockPersistLayout = vi.fn(); + +const TRIGGER_NODE = { + id: 'trigger-1', + type: 'trigger' as const, + position: { x: 0, y: 0 }, + data: { eventType: 'time.heartbeat', label: 'Test', config: {} }, +}; +const AGENT_NODE = { + id: 'agent-1', + type: 'agent' as const, + position: { x: 200, y: 0 }, + data: { sessionId: 's1', sessionName: 'Agent', toolType: 'claude-code' }, +}; +const EDGE = { id: 'e1', source: 'trigger-1', target: 'agent-1', mode: 'pass' as const }; + +// Two stable state objects — one per view mode — so the pipelineState prop is +// referentially identical across renders (prevents ReactFlow re-renders +// triggering a setState loop). +const lockedPipelineState = { + pipelines: [ + { + id: 'p1', + name: 'Pipeline 1', + color: '#06b6d4', + nodes: [TRIGGER_NODE, AGENT_NODE], + edges: [EDGE], + }, + ], + selectedPipelineId: null as string | null, +}; +const unlockedPipelineState = { + pipelines: lockedPipelineState.pipelines, + selectedPipelineId: 'p1' as string | null, +}; +let currentPipelineState = lockedPipelineState; + +// Selection mocks — used to verify click-to-select is blocked +const mockSetSelectedNodeId = vi.fn(); +const mockSetSelectedEdgeId = vi.fn(); +const mockHandleConfigureNode = vi.fn(); +const stableSelectionHook = { + selectedNodeId: null as string | null, + setSelectedNodeId: mockSetSelectedNodeId, + selectedEdgeId: null as string | null, + setSelectedEdgeId: mockSetSelectedEdgeId, + selectedNode: null, + selectedNodePipelineId: null, + selectedNodeHasOutgoingEdge: false, + hasIncomingAgentEdges: false, + incomingAgentEdgeCount: 0, + incomingTriggerEdges: [], + selectedEdge: null, + selectedEdgePipelineId: null, + selectedEdgePipelineColor: '#06b6d4', + edgeSourceNode: null, + edgeTargetNode: null, + onCanvasSessionIds: new Set(), + onNodeClick: vi.fn((_e: any, node: any) => mockSetSelectedNodeId(node.id)), + onEdgeClick: vi.fn((_e: any, edge: any) => mockSetSelectedEdgeId(edge.id)), + onPaneClick: vi.fn(), + handleConfigureNode: mockHandleConfigureNode, +}; + +const stableCueSettings = { + timeout_minutes: 30, + timeout_on_fail: 'break' as const, + max_concurrent: 1, + queue_size: 10, +}; +const stableRunningPipelineIds = new Set(); + +// usePipelineState mock that flips `isAllPipelinesView` based on +// `currentPipelineState.selectedPipelineId`. +function makeStateHook() { + const state = currentPipelineState; + return { + pipelineState: state, + setPipelineState: mockSetPipelineState, + isAllPipelinesView: state.selectedPipelineId === null, + isDirty: false, + setIsDirty: vi.fn(), + saveStatus: 'idle' as const, + validationErrors: [], + cueSettings: stableCueSettings, + setCueSettings: vi.fn(), + showSettings: false, + setShowSettings: vi.fn(), + runningPipelineIds: stableRunningPipelineIds, + persistLayout: mockPersistLayout, + pendingSavedViewportRef: { current: null as null | { x: number; y: number; zoom: number } }, + handleSave: vi.fn(), + handleDiscard: vi.fn(), + createPipeline: vi.fn(), + deletePipeline: vi.fn(), + renamePipeline: vi.fn(), + selectPipeline: vi.fn(), + changePipelineColor: vi.fn(), + onUpdateNode: vi.fn(), + onUpdateEdgePrompt: vi.fn(), + onDeleteNode: mockOnDeleteNode, + onUpdateEdge: vi.fn(), + onDeleteEdge: mockOnDeleteEdge, + }; +} +let stateHookCache = makeStateHook(); + +vi.mock('../../../../renderer/hooks/cue/usePipelineState', () => ({ + usePipelineState: () => stateHookCache, + DEFAULT_TRIGGER_LABELS: { 'time.heartbeat': 'Heartbeat' }, + validatePipelines: vi.fn(), +})); + +vi.mock('../../../../renderer/hooks/cue/usePipelineSelection', () => ({ + usePipelineSelection: () => stableSelectionHook, +})); + +vi.mock('../../../../renderer/components/CuePipelineEditor/utils/pipelineGraph', () => ({ + convertToReactFlowNodes: vi.fn((_pipelines: any, _sel: any, onConfigureNode: any) => { + // Expose the guarded configure callback the editor passes into nodes. + captured.handleConfigureNode = onConfigureNode; + return [ + { id: 'p1:trigger-1', type: 'trigger', position: { x: 0, y: 0 }, data: {} }, + { id: 'p1:agent-1', type: 'agent', position: { x: 200, y: 0 }, data: {} }, + ]; + }), + convertToReactFlowEdges: vi.fn(() => []), + computePipelineYOffsets: vi.fn(() => new Map()), +})); + +import { CuePipelineEditor } from '../../../../renderer/components/CuePipelineEditor/CuePipelineEditor'; + +const mockTheme = { + name: 'test', + colors: { + bgMain: '#1a1a2e', + bgActivity: '#16213e', + border: '#333', + textMain: '#e4e4e7', + textDim: '#a1a1aa', + accent: '#06b6d4', + accentForeground: '#fff', + success: '#22c55e', + }, +} as any; + +function renderEditor() { + stateHookCache = makeStateHook(); + render( + + ); +} + +describe('CuePipelineEditor — All Pipelines view is fully read-only', () => { + beforeEach(() => { + vi.clearAllMocks(); + Object.assign(captured, { + onNodesChange: null, + onNodeDragStop: null, + onDrop: null, + onConnect: null, + isValidConnection: null, + onNodeClick: null, + onEdgeClick: null, + onNodeContextMenu: null, + handleConfigureNode: null, + isReadOnly: undefined, + }); + currentPipelineState = lockedPipelineState; + }); + + it('passes isReadOnly=true to PipelineCanvas in All Pipelines view', () => { + renderEditor(); + expect(captured.isReadOnly).toBe(true); + }); + + it('passes isReadOnly=false to PipelineCanvas when a pipeline is selected', () => { + currentPipelineState = unlockedPipelineState; + renderEditor(); + expect(captured.isReadOnly).toBe(false); + }); + + it('onNodeDragStop is a no-op — position commits are refused', () => { + renderEditor(); + const draggedNode = { id: 'p1:agent-1', position: { x: 999, y: 999 } }; + captured.onNodeDragStop({} as any, draggedNode, [draggedNode]); + expect(mockSetPipelineState).not.toHaveBeenCalled(); + expect(mockPersistLayout).not.toHaveBeenCalled(); + }); + + it('onDrop is a no-op — new trigger/agent drops are refused', () => { + renderEditor(); + const triggerDropEvent = { + preventDefault: vi.fn(), + stopPropagation: vi.fn(), + dataTransfer: { + getData: () => JSON.stringify({ type: 'trigger', eventType: 'time.heartbeat' }), + }, + clientX: 100, + clientY: 100, + }; + captured.onDrop(triggerDropEvent); + expect(mockSetPipelineState).not.toHaveBeenCalled(); + + const agentDropEvent = { + preventDefault: vi.fn(), + stopPropagation: vi.fn(), + dataTransfer: { + getData: () => + JSON.stringify({ + type: 'agent', + sessionId: 's1', + sessionName: 'Agent', + toolType: 'claude-code', + }), + }, + clientX: 100, + clientY: 100, + }; + captured.onDrop(agentDropEvent); + expect(mockSetPipelineState).not.toHaveBeenCalled(); + }); + + it('onConnect is a no-op — new edges are refused', () => { + renderEditor(); + captured.onConnect({ + source: 'p1:trigger-1', + target: 'p1:agent-1', + sourceHandle: null, + targetHandle: null, + }); + expect(mockSetPipelineState).not.toHaveBeenCalled(); + }); + + it('isValidConnection returns false for every connection', () => { + renderEditor(); + expect( + captured.isValidConnection({ + source: 'p1:trigger-1', + target: 'p1:agent-1', + sourceHandle: null, + targetHandle: null, + }) + ).toBe(false); + }); + + it('onNodeContextMenu refuses to open the context menu', () => { + renderEditor(); + const event = { + preventDefault: vi.fn(), + clientX: 100, + clientY: 100, + }; + const node = { id: 'p1:agent-1', type: 'agent' }; + captured.onNodeContextMenu(event, node); + expect(event.preventDefault).toHaveBeenCalled(); + // No menu rendered → no selection mutation, no state mutation + expect(mockSetPipelineState).not.toHaveBeenCalled(); + expect(mockSetSelectedNodeId).not.toHaveBeenCalled(); + }); + + it('onNodeClick does not set node selection (guarded wrapper)', () => { + renderEditor(); + captured.onNodeClick({} as any, { id: 'p1:agent-1', type: 'agent' }); + expect(mockSetSelectedNodeId).not.toHaveBeenCalled(); + }); + + it('onEdgeClick does not set edge selection (guarded wrapper)', () => { + renderEditor(); + captured.onEdgeClick({} as any, { id: 'p1:e1' }); + expect(mockSetSelectedEdgeId).not.toHaveBeenCalled(); + }); + + it('handleConfigureNode (per-node configure icon) is a no-op', () => { + renderEditor(); + expect(captured.handleConfigureNode).toBeTruthy(); + captured.handleConfigureNode('p1:agent-1'); + expect(mockHandleConfigureNode).not.toHaveBeenCalled(); + }); + + it('keyboard Delete/Backspace does not delete the selected node', () => { + // Single-pipeline view would delete; All Pipelines view must not. + // Prime a selection so the delete path is reachable if the guard were absent. + stableSelectionHook.selectedNode = AGENT_NODE as any; + (stableSelectionHook as any).selectedNodePipelineId = 'p1'; + renderEditor(); + fireEvent.keyDown(window, { key: 'Delete' }); + fireEvent.keyDown(window, { key: 'Backspace' }); + expect(mockOnDeleteNode).not.toHaveBeenCalled(); + // Reset so other tests aren't affected + stableSelectionHook.selectedNode = null; + (stableSelectionHook as any).selectedNodePipelineId = null; + }); + + it('keyboard Delete/Backspace does not delete the selected edge', () => { + (stableSelectionHook as any).selectedEdge = EDGE; + (stableSelectionHook as any).selectedEdgePipelineId = 'p1'; + renderEditor(); + fireEvent.keyDown(window, { key: 'Delete' }); + fireEvent.keyDown(window, { key: 'Backspace' }); + expect(mockOnDeleteEdge).not.toHaveBeenCalled(); + (stableSelectionHook as any).selectedEdge = null; + (stableSelectionHook as any).selectedEdgePipelineId = null; + }); +}); + +describe('CuePipelineEditor — single-pipeline view remains editable (negative control)', () => { + beforeEach(() => { + vi.clearAllMocks(); + Object.assign(captured, { + onNodesChange: null, + onNodeDragStop: null, + onDrop: null, + onConnect: null, + isValidConnection: null, + onNodeClick: null, + onEdgeClick: null, + onNodeContextMenu: null, + handleConfigureNode: null, + isReadOnly: undefined, + }); + currentPipelineState = unlockedPipelineState; + }); + + it('onNodeDragStop commits to state when a pipeline is selected', () => { + renderEditor(); + const draggedNode = { id: 'p1:agent-1', position: { x: 999, y: 999 } }; + captured.onNodeDragStop({} as any, draggedNode, [draggedNode]); + expect(mockSetPipelineState).toHaveBeenCalledTimes(1); + expect(mockPersistLayout).toHaveBeenCalledTimes(1); + }); + + it('onNodeClick sets selection when a pipeline is selected', () => { + renderEditor(); + captured.onNodeClick({} as any, { id: 'p1:agent-1', type: 'agent' }); + expect(mockSetSelectedNodeId).toHaveBeenCalledWith('p1:agent-1'); + }); + + it('handleConfigureNode calls through when a pipeline is selected', () => { + renderEditor(); + captured.handleConfigureNode('p1:agent-1'); + expect(mockHandleConfigureNode).toHaveBeenCalledWith('p1:agent-1'); + }); +}); diff --git a/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.drag.test.tsx b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.drag.test.tsx index 2d49d748d..df26d7cfb 100644 --- a/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.drag.test.tsx +++ b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.drag.test.tsx @@ -25,6 +25,7 @@ vi.mock('reactflow', () => { screenToFlowPosition: vi.fn((pos: any) => pos), setViewport: vi.fn(), }), + useNodesInitialized: () => false, applyNodeChanges: (changes: any[], nodes: any[]) => nodes, Background: () => null, Controls: () => null, @@ -97,6 +98,7 @@ vi.mock('../../../../renderer/hooks/cue/usePipelineState', () => ({ setShowSettings: vi.fn(), runningPipelineIds: new Set(), persistLayout: mockPersistLayout, + pendingSavedViewportRef: { current: null }, handleSave: vi.fn(), handleDiscard: vi.fn(), createPipeline: vi.fn(), diff --git a/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx new file mode 100644 index 000000000..a8aaf92aa --- /dev/null +++ b/src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx @@ -0,0 +1,285 @@ +/** + * Regression tests for CuePipelineEditor's initial viewport behavior. + * + * Guards against the bug where, on first open, the canvas appeared empty + * because the initial `fitView` was scheduled on a `setTimeout(150)` that + * could fire BEFORE ReactFlow had measured the freshly-rendered nodes. + * Switching pipelines and switching back "fixed" the canvas only because + * the selection-change `fitView` ran later, after measurements had + * completed from the first render. + * + * The fix gates the initial viewport step on ReactFlow's + * `useNodesInitialized()` and coordinates saved-viewport restoration + * (owned by usePipelineLayout) with fitView in a single effect — no race. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render } from '@testing-library/react'; +import React from 'react'; + +const mockFitView = vi.fn(); +const mockSetViewport = vi.fn(); +let mockNodesInitialized = false; + +vi.mock('reactflow', () => ({ + default: (props: any) =>
{props.children}
, + ReactFlowProvider: ({ children }: any) => <>{children}, + useReactFlow: () => ({ + fitView: mockFitView, + screenToFlowPosition: vi.fn((pos: any) => pos), + setViewport: mockSetViewport, + }), + useNodesInitialized: () => mockNodesInitialized, + applyNodeChanges: (_changes: any[], nodes: any[]) => nodes, + Background: () => null, + Controls: () => null, + MiniMap: () => null, + ConnectionMode: { Loose: 'loose' }, + Position: { Left: 'left', Right: 'right' }, + Handle: () => null, + MarkerType: { ArrowClosed: 'arrowclosed' }, +})); + +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineCanvas', () => ({ + PipelineCanvas: React.memo(() =>
), +})); +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineToolbar', () => ({ + PipelineToolbar: () =>
, +})); +vi.mock('../../../../renderer/components/CuePipelineEditor/PipelineContextMenu', () => ({ + PipelineContextMenu: () => null, +})); + +// Stable references across renders — returning a new object literal from the +// mock every render caused a setState loop in `useEffect(() => setDisplayNodes, +// [computedNodes])`, which OOM'd the Node worker. +const mockPendingSavedViewportRef = { + current: null as null | { x: number; y: number; zoom: number }, +}; +const stablePipelineState = { + pipelines: [ + { + id: 'p1', + name: 'Pipeline 1', + color: '#06b6d4', + nodes: [ + { + id: 'trigger-1', + type: 'trigger', + position: { x: 0, y: 0 }, + data: { eventType: 'time.heartbeat', label: 'Test', config: {} }, + }, + ], + edges: [], + }, + ], + selectedPipelineId: null, +}; +const stableRunningPipelineIds = new Set(); +const stableCueSettings = { + timeout_minutes: 30, + timeout_on_fail: 'break', + max_concurrent: 1, + queue_size: 10, +}; +const stableStateHook = { + pipelineState: stablePipelineState, + setPipelineState: vi.fn(), + isAllPipelinesView: true, + isDirty: false, + setIsDirty: vi.fn(), + saveStatus: 'idle' as const, + validationErrors: [], + cueSettings: stableCueSettings, + setCueSettings: vi.fn(), + showSettings: false, + setShowSettings: vi.fn(), + runningPipelineIds: stableRunningPipelineIds, + persistLayout: vi.fn(), + pendingSavedViewportRef: mockPendingSavedViewportRef, + handleSave: vi.fn(), + handleDiscard: vi.fn(), + createPipeline: vi.fn(), + deletePipeline: vi.fn(), + renamePipeline: vi.fn(), + selectPipeline: vi.fn(), + changePipelineColor: vi.fn(), + onUpdateNode: vi.fn(), + onUpdateEdgePrompt: vi.fn(), + onDeleteNode: vi.fn(), + onUpdateEdge: vi.fn(), + onDeleteEdge: vi.fn(), +}; + +vi.mock('../../../../renderer/hooks/cue/usePipelineState', () => ({ + usePipelineState: () => stableStateHook, + DEFAULT_TRIGGER_LABELS: {}, + validatePipelines: vi.fn(), +})); + +// Stable mock — see note on stableStateHook above. +const stableSelectionHook = { + selectedNodeId: null, + setSelectedNodeId: vi.fn(), + selectedEdgeId: null, + setSelectedEdgeId: vi.fn(), + selectedNode: null, + selectedNodePipelineId: null, + selectedNodeHasOutgoingEdge: false, + hasIncomingAgentEdges: false, + incomingAgentEdgeCount: 0, + incomingTriggerEdges: [], + selectedEdge: null, + selectedEdgePipelineId: null, + selectedEdgePipelineColor: '#06b6d4', + edgeSourceNode: null, + edgeTargetNode: null, + onCanvasSessionIds: new Set(), + onNodeClick: vi.fn(), + onEdgeClick: vi.fn(), + onPaneClick: vi.fn(), + handleConfigureNode: vi.fn(), +}; + +vi.mock('../../../../renderer/hooks/cue/usePipelineSelection', () => ({ + usePipelineSelection: () => stableSelectionHook, +})); + +vi.mock('../../../../renderer/components/CuePipelineEditor/utils/pipelineGraph', () => ({ + // Return one node so computedNodes.length > 0 + convertToReactFlowNodes: vi.fn(() => [ + { id: 'p1:trigger-1', type: 'trigger', position: { x: 0, y: 0 }, data: {} }, + ]), + convertToReactFlowEdges: vi.fn(() => []), + computePipelineYOffsets: vi.fn(() => new Map()), +})); + +import { CuePipelineEditor } from '../../../../renderer/components/CuePipelineEditor/CuePipelineEditor'; + +const mockTheme = { + name: 'test', + colors: { + bgMain: '#1a1a2e', + bgActivity: '#16213e', + border: '#333', + textMain: '#e4e4e7', + textDim: '#a1a1aa', + accent: '#06b6d4', + accentForeground: '#fff', + success: '#22c55e', + }, +} as any; + +function renderEditor() { + return render( + + ); +} + +describe('CuePipelineEditor — initial viewport (regression: empty canvas on first open)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockNodesInitialized = false; + mockPendingSavedViewportRef.current = null; + }); + + it('does NOT fitView when nodes have not yet been measured', () => { + // Simulates the first render after layout restore — nodes are present + // in state but ReactFlow's dimension measurement has not completed. + mockNodesInitialized = false; + renderEditor(); + + expect(mockFitView).not.toHaveBeenCalled(); + expect(mockSetViewport).not.toHaveBeenCalled(); + }); + + it('fits view once nodes have been measured (no saved viewport)', () => { + // Mount with nodesInitialized=false to mirror the real ReactFlow timing + // (initial render happens before the dimension measurement finishes), + // then flip to true and rerender so the initial-viewport effect's + // dependency change drives it post-mount. Asserting after a single + // mount with nodesInitialized=true would mask a regression where the + // effect doesn't react to the false→true transition. + mockNodesInitialized = false; + mockPendingSavedViewportRef.current = null; + const { rerender } = renderEditor(); + + expect(mockFitView).not.toHaveBeenCalled(); + + mockNodesInitialized = true; + rerender( + + ); + + expect(mockFitView).toHaveBeenCalledTimes(1); + expect(mockSetViewport).not.toHaveBeenCalled(); + }); + + it('restores saved viewport immediately on mount (no need to wait for measurement)', () => { + // setViewport is a pure (x, y, zoom) restore — it doesn't depend on + // node measurement, so it fires immediately. Waiting for + // nodesInitialized would briefly show the wrong viewport before + // snapping to the saved one. + mockNodesInitialized = false; + mockPendingSavedViewportRef.current = { x: 100, y: 200, zoom: 1.5 }; + renderEditor(); + + expect(mockSetViewport).toHaveBeenCalledWith({ x: 100, y: 200, zoom: 1.5 }); + expect(mockFitView).not.toHaveBeenCalled(); + }); + + it('consumes the pending saved viewport after applying it', () => { + mockNodesInitialized = false; + mockPendingSavedViewportRef.current = { x: 100, y: 200, zoom: 1.5 }; + renderEditor(); + + // Ref is nulled out so the viewport isn't re-applied on subsequent renders + // (e.g. after selection changes trigger the other fitView effect). + expect(mockPendingSavedViewportRef.current).toBeNull(); + }); + + it('runs the initial viewport step exactly once', () => { + mockNodesInitialized = false; + mockPendingSavedViewportRef.current = null; + const { rerender } = renderEditor(); + + mockNodesInitialized = true; + rerender( + + ); + + expect(mockFitView).toHaveBeenCalledTimes(1); + + // Subsequent renders must NOT re-fit — that's the job of the + // selection-change fitView effect, not the initial one. + rerender( + + ); + + expect(mockFitView).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.ts b/src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.ts index b4fb632e4..84c33706a 100644 --- a/src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.ts +++ b/src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.ts @@ -80,6 +80,22 @@ describe('mergePipelinesWithSavedLayout', () => { expect(result.selectedPipelineId).toBe('p1'); }); + it('falls back to first pipeline when saved selectedPipelineId no longer exists', () => { + // Repro for the "pipeline vanished after save" bug: createPipeline assigned + // a timestamp-based id (`pipeline-1700000000000`) that the next YAML reload + // regenerates as `pipeline-MyPipeline`. The saved layout still references + // the old timestamp id; without this fallback the canvas renders empty + // because convertToReactFlowNodes skips every pipeline whose id != selected. + const livePipelines = [makePipeline({ id: 'pipeline-MyPipeline' })]; + const savedLayout: PipelineLayoutState = { + pipelines: [makePipeline({ id: 'pipeline-1700000000000' })], + selectedPipelineId: 'pipeline-1700000000000', + }; + + const result = mergePipelinesWithSavedLayout(livePipelines, savedLayout); + expect(result.selectedPipelineId).toBe('pipeline-MyPipeline'); + }); + it('merges saved node positions with live pipeline data', () => { const livePipelines = [makePipeline()]; const savedLayout: PipelineLayoutState = { diff --git a/src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts b/src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts index e14cde1b2..cd6737ac7 100644 --- a/src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts +++ b/src/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.ts @@ -381,9 +381,11 @@ describe('subscriptionsToPipelines', () => { expect(agentNames).toContain('tester'); }); - it('overrides stale agent_id when subscription name matches a different session', () => { - // Bug scenario: agent_id was corrupted (points to Maestro) but subscription - // name "Pedsidian" matches the Pedsidian session. Name match should win. + it('trusts explicit agent_id even when subscription name matches a different session', () => { + // Per-project-root YAML partitioning guarantees agent_id is the source + // of truth. A coincidental pipeline-name/session-name overlap must NOT + // flip the resolved agent — doing so caused the "Maestro swap reverts" + // bug where replacing an agent would snap back on reload. const subs: CueSubscription[] = [ { name: 'Pedsidian', @@ -392,7 +394,7 @@ describe('subscriptionsToPipelines', () => { prompt: 'Do briefing', schedule_times: ['08:30'], schedule_days: ['mon', 'tue', 'wed', 'thu', 'fri'], - agent_id: 'maestro-uuid', // Wrong! Should be pedsidian-uuid + agent_id: 'maestro-uuid', }, ]; const sessions: SessionInfo[] = [ @@ -417,9 +419,9 @@ describe('subscriptionsToPipelines', () => { const agents = pipelines[0].nodes.filter((n) => n.type === 'agent'); expect(agents).toHaveLength(1); - // Should resolve to Pedsidian (name match), not Maestro (stale agent_id) - expect((agents[0].data as { sessionName: string }).sessionName).toBe('Pedsidian'); - expect((agents[0].data as { sessionId: string }).sessionId).toBe('pedsidian-uuid'); + // agent_id wins — resolves to Maestro despite the name match on Pedsidian. + expect((agents[0].data as { sessionName: string }).sessionName).toBe('Maestro'); + expect((agents[0].data as { sessionId: string }).sessionId).toBe('maestro-uuid'); }); it('uses subscription name to find target when agent_id is absent', () => { diff --git a/src/__tests__/renderer/components/CueYamlEditor.test.tsx b/src/__tests__/renderer/components/CueYamlEditor.test.tsx index b630ae9f9..bae273f4b 100644 --- a/src/__tests__/renderer/components/CueYamlEditor.test.tsx +++ b/src/__tests__/renderer/components/CueYamlEditor.test.tsx @@ -678,7 +678,7 @@ describe('CueYamlEditor', () => { fireEvent.click(screen.getByText('Save')); await waitFor(() => { - expect(mockWriteYaml).toHaveBeenCalledWith('/test/project', 'new content'); + expect(mockWriteYaml).toHaveBeenCalledWith('/test/project', 'new content', undefined); }); expect(mockRefreshSession).toHaveBeenCalledWith('sess-1', '/test/project'); expect(defaultProps.onClose).toHaveBeenCalledOnce(); diff --git a/src/__tests__/renderer/hooks/cue/usePipelineLayout.test.ts b/src/__tests__/renderer/hooks/cue/usePipelineLayout.test.ts index d528a318e..19ca49b8b 100644 --- a/src/__tests__/renderer/hooks/cue/usePipelineLayout.test.ts +++ b/src/__tests__/renderer/hooks/cue/usePipelineLayout.test.ts @@ -56,6 +56,7 @@ function createDefaultParams( pipelineState: { pipelines: [], selectedPipelineId: null }, setPipelineState: vi.fn(), savedStateRef: { current: '' }, + lastWrittenRootsRef: { current: new Set() }, setIsDirty: vi.fn(), ...overrides, }; @@ -159,7 +160,7 @@ describe('usePipelineLayout', () => { expect(setPipelineState).toHaveBeenCalledTimes(1); }); - it('restores viewport when saved layout has viewport', async () => { + it('stashes saved viewport in pendingSavedViewportRef for the editor to apply once nodes are measured', async () => { const livePipelines = [makePipeline('p1')]; mockGraphSessionsToPipelines.mockReturnValue(livePipelines as any); @@ -178,18 +179,56 @@ describe('usePipelineLayout', () => { } as unknown as UsePipelineLayoutParams['reactFlowInstance'], }); - renderHook(() => usePipelineLayout(params)); + const { result } = renderHook(() => usePipelineLayout(params)); await act(async () => { await vi.advanceTimersByTimeAsync(200); }); - // The viewport restore is inside a setTimeout(fn, 100) - act(() => { - vi.advanceTimersByTime(100); + // Hook no longer applies the viewport directly — that's the editor's job, + // gated on ReactFlow's useNodesInitialized so nodes have been measured first. + expect(setViewport).not.toHaveBeenCalled(); + expect(result.current.pendingSavedViewportRef.current).toEqual({ + x: 100, + y: 200, + zoom: 1.5, }); + }); + + it('leaves pendingSavedViewportRef null when saved layout has no viewport', async () => { + const livePipelines = [makePipeline('p1')]; + mockGraphSessionsToPipelines.mockReturnValue(livePipelines as any); + + const savedLayout = { + pipelines: [makePipeline('p1')], + selectedPipelineId: 'p1', + // no `viewport` key + }; + (window as any).maestro.cue.loadPipelineLayout.mockResolvedValue(savedLayout); - expect(setViewport).toHaveBeenCalledWith({ x: 100, y: 200, zoom: 1.5 }); + const params = createDefaultParams(); + const { result } = renderHook(() => usePipelineLayout(params)); + + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + + expect(result.current.pendingSavedViewportRef.current).toBeNull(); + }); + + it('leaves pendingSavedViewportRef null when there is no saved layout at all', async () => { + const livePipelines = [makePipeline('p1')]; + mockGraphSessionsToPipelines.mockReturnValue(livePipelines as any); + (window as any).maestro.cue.loadPipelineLayout.mockResolvedValue(null); + + const params = createDefaultParams(); + const { result } = renderHook(() => usePipelineLayout(params)); + + await act(async () => { + await vi.advanceTimersByTimeAsync(200); + }); + + expect(result.current.pendingSavedViewportRef.current).toBeNull(); }); it('uses first pipeline when no saved layout exists', async () => { @@ -239,8 +278,15 @@ describe('usePipelineLayout', () => { expect(setPipelineState).not.toHaveBeenCalled(); }); - it('does not restore layout when graphSessions is empty', async () => { - const params = createDefaultParams({ graphSessions: [] }); + it('does not restore pipelines when graphSessions is empty', async () => { + // Pipeline restore is gated on live graph data; with no graphSessions + // the pipeline-restore branch never runs. NOTE: loadPipelineLayout + // IS still called once by the standalone writtenRoots-reseed effect + // (which must run independent of graphSessions so orphan-root + // metadata is hydrated before the user takes their first save + // action). Pipeline state must remain untouched. + const setPipelineState = vi.fn(); + const params = createDefaultParams({ graphSessions: [], setPipelineState }); renderHook(() => usePipelineLayout(params)); @@ -249,7 +295,7 @@ describe('usePipelineLayout', () => { }); expect(mockGraphSessionsToPipelines).not.toHaveBeenCalled(); - expect((window as any).maestro.cue.loadPipelineLayout).not.toHaveBeenCalled(); + expect(setPipelineState).not.toHaveBeenCalled(); }); it('does not restore layout when graphSessionsToPipelines returns empty array', async () => { diff --git a/src/__tests__/renderer/hooks/cue/usePipelineState.test.ts b/src/__tests__/renderer/hooks/cue/usePipelineState.test.ts index ac0ddc36f..d327a31ae 100644 --- a/src/__tests__/renderer/hooks/cue/usePipelineState.test.ts +++ b/src/__tests__/renderer/hooks/cue/usePipelineState.test.ts @@ -1,4 +1,5 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { useCueDirtyStore } from '../../../../renderer/stores/cueDirtyStore'; import { renderHook, act } from '@testing-library/react'; import { usePipelineState, @@ -19,8 +20,14 @@ vi.mock('../../../../renderer/utils/sentry', () => ({ })); const mockPersistLayout = vi.fn(); +const mockPendingSavedViewportRef = { + current: null as null | { x: number; y: number; zoom: number }, +}; vi.mock('../../../../renderer/hooks/cue/usePipelineLayout', () => ({ - usePipelineLayout: vi.fn(() => ({ persistLayout: mockPersistLayout })), + usePipelineLayout: vi.fn(() => ({ + persistLayout: mockPersistLayout, + pendingSavedViewportRef: mockPendingSavedViewportRef, + })), })); vi.mock('../../../../renderer/components/CuePipelineEditor/utils/pipelineToYaml', () => ({ @@ -54,6 +61,13 @@ const mockGetSettings = vi.fn().mockResolvedValue({ const mockWriteYaml = vi.fn().mockResolvedValue(undefined); const mockRefreshSession = vi.fn().mockResolvedValue(undefined); const mockGetGraphData = vi.fn().mockResolvedValue([]); +// readYaml returns the same content pipelinesToYaml is mocked to produce, so +// handleSave's write-back verification passes. +const mockReadYaml = vi.fn().mockResolvedValue('test'); + +afterEach(() => { + useCueDirtyStore.setState({ pipelineDirty: false, yamlDirty: false }); +}); beforeEach(() => { vi.clearAllMocks(); @@ -61,6 +75,7 @@ beforeEach(() => { cue: { getSettings: mockGetSettings, writeYaml: mockWriteYaml, + readYaml: mockReadYaml, refreshSession: mockRefreshSession, getGraphData: mockGetGraphData, }, @@ -75,7 +90,6 @@ function createDefaultParams(overrides?: Partial): UsePi sessions: [], graphSessions: [], activeRuns: [], - onDirtyChange: vi.fn(), reactFlowInstance: { getViewport: vi.fn(() => ({ x: 0, y: 0, zoom: 1 })) } as any, selectedNodePipelineId: null, selectedEdgePipelineId: null, @@ -88,14 +102,23 @@ function createDefaultParams(overrides?: Partial): UsePi } function makeTriggerNode(id: string, eventType = 'file.changed' as const): PipelineNode { + // Provide event-appropriate defaults so the per-event trigger config + // validation (introduced to prevent broken YAML being saved) doesn't + // fail tests that aren't asserting on trigger config specifics. + const defaults: Record> = { + 'file.changed': { watch: '**/*' }, + 'task.pending': { watch: '**/*.md' }, + 'time.heartbeat': { interval_minutes: 5 }, + 'time.scheduled': { schedule_times: ['09:00'] }, + }; return { id, type: 'trigger', position: { x: 0, y: 0 }, data: { eventType, - label: 'File Change', - config: {}, + label: eventType, + config: defaults[eventType] ?? {}, }, }; } @@ -162,9 +185,12 @@ describe('DEFAULT_TRIGGER_LABELS', () => { // ─── validatePipelines (pure function) ─────────────────────────────────────── describe('validatePipelines', () => { - it('returns empty array for empty pipeline (no nodes)', () => { - const errors = validatePipelines([makePipeline()]); - expect(errors).toEqual([]); + it('flags an empty pipeline (no nodes) so the save surfaces feedback', () => { + // Previously this returned [] and save silently succeeded without + // persisting anything — the "didn't save" class of user-reported bugs. + const errors = validatePipelines([makePipeline({ name: 'Empty' })]); + expect(errors).toHaveLength(1); + expect(errors[0]).toMatch(/Empty.*add a trigger and an agent/); }); it('returns empty array for empty pipelines array', () => { @@ -930,17 +956,22 @@ describe('usePipelineState', () => { expect(result.current.showSettings).toBe(true); }); - it('notifies onDirtyChange when isDirty changes', () => { - const onDirtyChange = vi.fn(); - const { result } = renderHook(() => usePipelineState(createDefaultParams({ onDirtyChange }))); + it('pushes isDirty into cueDirtyStore.pipelineDirty when isDirty changes', () => { + const { result } = renderHook(() => usePipelineState(createDefaultParams())); - // Initial call with false - expect(onDirtyChange).toHaveBeenCalledWith(false); + // Initially not dirty + expect(useCueDirtyStore.getState().pipelineDirty).toBe(false); act(() => { result.current.setIsDirty(true); }); - expect(onDirtyChange).toHaveBeenCalledWith(true); + expect(useCueDirtyStore.getState().pipelineDirty).toBe(true); + + act(() => { + result.current.setIsDirty(false); + }); + + expect(useCueDirtyStore.getState().pipelineDirty).toBe(false); }); }); diff --git a/src/__tests__/renderer/services/cue.test.ts b/src/__tests__/renderer/services/cue.test.ts new file mode 100644 index 000000000..497835452 --- /dev/null +++ b/src/__tests__/renderer/services/cue.test.ts @@ -0,0 +1,267 @@ +/** + * Tests for src/renderer/services/cue.ts + * + * Covers: + * - Read methods return their default value on IPC error + * - Write methods rethrow on IPC error + * - Successful calls pass values through unchanged + * - onActivityUpdate is a direct passthrough + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { cueService } from '../../../renderer/services/cue'; + +// ─── Mock helpers ───────────────────────────────────────────────────────────── + +const mockCue = { + getSettings: vi.fn(), + getStatus: vi.fn(), + getGraphData: vi.fn(), + getActiveRuns: vi.fn(), + getActivityLog: vi.fn(), + getQueueStatus: vi.fn(), + readYaml: vi.fn(), + loadPipelineLayout: vi.fn(), + validateYaml: vi.fn(), + enable: vi.fn(), + disable: vi.fn(), + stopRun: vi.fn(), + stopAll: vi.fn(), + triggerSubscription: vi.fn(), + refreshSession: vi.fn(), + removeSession: vi.fn(), + writeYaml: vi.fn(), + deleteYaml: vi.fn(), + savePipelineLayout: vi.fn(), + onActivityUpdate: vi.fn(), +}; + +beforeEach(() => { + vi.clearAllMocks(); + (window as any).maestro = { cue: mockCue }; + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +// ─── Read methods ───────────────────────────────────────────────────────────── + +describe('cueService — read methods', () => { + describe('getSettings', () => { + it('passes resolved value through', async () => { + const settings = { timeout_minutes: 10 } as any; + mockCue.getSettings.mockResolvedValue(settings); + expect(await cueService.getSettings()).toBe(settings); + }); + + it('returns empty object on error', async () => { + mockCue.getSettings.mockRejectedValue(new Error('fail')); + expect(await cueService.getSettings()).toEqual({}); + expect(console.error).toHaveBeenCalledWith('Cue getSettings error:', expect.any(Error)); + }); + }); + + describe('getStatus', () => { + it('passes resolved value through', async () => { + const statuses = [{ sessionId: 's1' }] as any; + mockCue.getStatus.mockResolvedValue(statuses); + expect(await cueService.getStatus()).toBe(statuses); + }); + + it('returns [] on error', async () => { + mockCue.getStatus.mockRejectedValue(new Error('fail')); + expect(await cueService.getStatus()).toEqual([]); + }); + }); + + describe('getGraphData', () => { + it('passes resolved value through', async () => { + const data = [{ id: 'g1' }] as any; + mockCue.getGraphData.mockResolvedValue(data); + expect(await cueService.getGraphData()).toBe(data); + }); + + it('returns [] on error', async () => { + mockCue.getGraphData.mockRejectedValue(new Error('fail')); + expect(await cueService.getGraphData()).toEqual([]); + }); + }); + + describe('getActiveRuns', () => { + it('passes resolved value through', async () => { + const runs = [{ runId: 'r1' }] as any; + mockCue.getActiveRuns.mockResolvedValue(runs); + expect(await cueService.getActiveRuns()).toBe(runs); + }); + + it('returns [] on error', async () => { + mockCue.getActiveRuns.mockRejectedValue(new Error('fail')); + expect(await cueService.getActiveRuns()).toEqual([]); + }); + }); + + describe('getActivityLog', () => { + it('passes limit parameter through', async () => { + mockCue.getActivityLog.mockResolvedValue([]); + await cueService.getActivityLog(50); + expect(mockCue.getActivityLog).toHaveBeenCalledWith(50); + }); + + it('returns [] on error', async () => { + mockCue.getActivityLog.mockRejectedValue(new Error('fail')); + expect(await cueService.getActivityLog()).toEqual([]); + }); + }); + + describe('getQueueStatus', () => { + it('passes resolved value through', async () => { + const status = { s1: 3 }; + mockCue.getQueueStatus.mockResolvedValue(status); + expect(await cueService.getQueueStatus()).toBe(status); + }); + + it('returns {} on error', async () => { + mockCue.getQueueStatus.mockRejectedValue(new Error('fail')); + expect(await cueService.getQueueStatus()).toEqual({}); + }); + }); + + describe('readYaml', () => { + it('passes resolved value through', async () => { + mockCue.readYaml.mockResolvedValue('yaml content'); + expect(await cueService.readYaml('/root')).toBe('yaml content'); + expect(mockCue.readYaml).toHaveBeenCalledWith('/root'); + }); + + it('passes through null when handler reports the file does not exist', async () => { + mockCue.readYaml.mockResolvedValue(null); + expect(await cueService.readYaml('/root')).toBeNull(); + }); + + it('rethrows IPC errors instead of swallowing them as null', async () => { + // The handler distinguishes "no file" (null) from a transport + // failure (throws). Swallowing IPC errors as null hid bugs and + // caused callers (e.g. CueYamlEditor) to silently fall back to a + // template on transport failures. + mockCue.readYaml.mockRejectedValue(new Error('fail')); + await expect(cueService.readYaml('/root')).rejects.toThrow('fail'); + }); + }); + + describe('loadPipelineLayout', () => { + it('passes resolved value through', async () => { + const layout = { pipelines: [] }; + mockCue.loadPipelineLayout.mockResolvedValue(layout); + expect(await cueService.loadPipelineLayout()).toBe(layout); + }); + + it('returns null on error', async () => { + mockCue.loadPipelineLayout.mockRejectedValue(new Error('fail')); + expect(await cueService.loadPipelineLayout()).toBeNull(); + }); + }); + + describe('validateYaml', () => { + it('passes resolved value through', async () => { + const validation = { valid: false, errors: ['bad yaml'] }; + mockCue.validateYaml.mockResolvedValue(validation); + expect(await cueService.validateYaml('bad')).toBe(validation); + }); + + it('rethrows on IPC error so callers gate Save (no false-positive valid)', async () => { + // The previous default was `{ valid: true, errors: [] }` — a + // transport failure would have surfaced as "yaml is valid, save + // freely". Callers must now catch and treat the failure as invalid. + mockCue.validateYaml.mockRejectedValue(new Error('fail')); + await expect(cueService.validateYaml('content')).rejects.toThrow('fail'); + }); + }); +}); + +// ─── Write methods ──────────────────────────────────────────────────────────── + +describe('cueService — write methods', () => { + it('enable — resolves on success', async () => { + mockCue.enable.mockResolvedValue(undefined); + await expect(cueService.enable()).resolves.toBeUndefined(); + }); + + it('enable — rethrows on error', async () => { + mockCue.enable.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.enable()).rejects.toThrow('IPC fail'); + }); + + it('disable — rethrows on error', async () => { + mockCue.disable.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.disable()).rejects.toThrow('IPC fail'); + }); + + it('stopRun — passes runId and rethrows on error', async () => { + mockCue.stopRun.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.stopRun('run-1')).rejects.toThrow('IPC fail'); + expect(mockCue.stopRun).toHaveBeenCalledWith('run-1'); + }); + + it('stopRun — passes resolved boolean through', async () => { + mockCue.stopRun.mockResolvedValue(true); + expect(await cueService.stopRun('run-1')).toBe(true); + }); + + it('stopAll — rethrows on error', async () => { + mockCue.stopAll.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.stopAll()).rejects.toThrow('IPC fail'); + }); + + it('triggerSubscription — passes args and rethrows on error', async () => { + mockCue.triggerSubscription.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.triggerSubscription('sub-1', 'prompt')).rejects.toThrow('IPC fail'); + expect(mockCue.triggerSubscription).toHaveBeenCalledWith('sub-1', 'prompt'); + }); + + it('refreshSession — passes args and rethrows on error', async () => { + mockCue.refreshSession.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.refreshSession('sess-1', '/root')).rejects.toThrow('IPC fail'); + expect(mockCue.refreshSession).toHaveBeenCalledWith('sess-1', '/root'); + }); + + it('removeSession — rethrows on error', async () => { + mockCue.removeSession.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.removeSession('sess-1')).rejects.toThrow('IPC fail'); + }); + + it('writeYaml — passes args (including promptFiles) and rethrows on error', async () => { + mockCue.writeYaml.mockRejectedValue(new Error('IPC fail')); + const promptFiles = { 'p.md': 'content' }; + await expect(cueService.writeYaml('/root', 'yaml', promptFiles)).rejects.toThrow('IPC fail'); + expect(mockCue.writeYaml).toHaveBeenCalledWith('/root', 'yaml', promptFiles); + }); + + it('deleteYaml — passes resolved boolean through', async () => { + mockCue.deleteYaml.mockResolvedValue(true); + expect(await cueService.deleteYaml('/root')).toBe(true); + }); + + it('deleteYaml — rethrows on error', async () => { + mockCue.deleteYaml.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.deleteYaml('/root')).rejects.toThrow('IPC fail'); + }); + + it('savePipelineLayout — rethrows on error', async () => { + mockCue.savePipelineLayout.mockRejectedValue(new Error('IPC fail')); + await expect(cueService.savePipelineLayout({ x: 1 })).rejects.toThrow('IPC fail'); + expect(mockCue.savePipelineLayout).toHaveBeenCalledWith({ x: 1 }); + }); +}); + +// ─── Event passthrough ──────────────────────────────────────────────────────── + +describe('cueService — onActivityUpdate', () => { + it('is a direct passthrough to window.maestro.cue.onActivityUpdate', () => { + const unsubscribe = vi.fn(); + mockCue.onActivityUpdate.mockReturnValue(unsubscribe); + const callback = vi.fn(); + + const result = cueService.onActivityUpdate(callback); + + expect(mockCue.onActivityUpdate).toHaveBeenCalledWith(callback); + expect(result).toBe(unsubscribe); + }); +}); diff --git a/src/__tests__/renderer/stores/cueDirtyStore.test.ts b/src/__tests__/renderer/stores/cueDirtyStore.test.ts new file mode 100644 index 000000000..b67a86493 --- /dev/null +++ b/src/__tests__/renderer/stores/cueDirtyStore.test.ts @@ -0,0 +1,61 @@ +/** + * Tests for src/renderer/stores/cueDirtyStore.ts + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { useCueDirtyStore } from '../../../renderer/stores/cueDirtyStore'; + +beforeEach(() => { + useCueDirtyStore.setState({ pipelineDirty: false, yamlDirty: false }); +}); + +describe('useCueDirtyStore', () => { + it('has both flags false initially', () => { + const { pipelineDirty, yamlDirty } = useCueDirtyStore.getState(); + expect(pipelineDirty).toBe(false); + expect(yamlDirty).toBe(false); + }); + + it('isAnyDirty() returns false when neither flag is set', () => { + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(false); + }); + + it('setPipelineDirty(true) sets pipelineDirty and isAnyDirty returns true', () => { + useCueDirtyStore.getState().setPipelineDirty(true); + expect(useCueDirtyStore.getState().pipelineDirty).toBe(true); + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(true); + }); + + it('setYamlDirty(true) sets yamlDirty and isAnyDirty returns true', () => { + useCueDirtyStore.getState().setYamlDirty(true); + expect(useCueDirtyStore.getState().yamlDirty).toBe(true); + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(true); + }); + + it('isAnyDirty() is true when only pipeline is dirty', () => { + useCueDirtyStore.getState().setPipelineDirty(true); + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(true); + }); + + it('isAnyDirty() is true when only yaml is dirty', () => { + useCueDirtyStore.getState().setYamlDirty(true); + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(true); + }); + + it('resetAll() clears both flags', () => { + useCueDirtyStore.getState().setPipelineDirty(true); + useCueDirtyStore.getState().setYamlDirty(true); + + useCueDirtyStore.getState().resetAll(); + + expect(useCueDirtyStore.getState().pipelineDirty).toBe(false); + expect(useCueDirtyStore.getState().yamlDirty).toBe(false); + expect(useCueDirtyStore.getState().isAnyDirty()).toBe(false); + }); + + it('setPipelineDirty(false) clears the pipeline flag', () => { + useCueDirtyStore.getState().setPipelineDirty(true); + useCueDirtyStore.getState().setPipelineDirty(false); + expect(useCueDirtyStore.getState().pipelineDirty).toBe(false); + }); +}); diff --git a/src/main/cue/config/cue-config-repository.ts b/src/main/cue/config/cue-config-repository.ts index d9495af95..4b85e8cc2 100644 --- a/src/main/cue/config/cue-config-repository.ts +++ b/src/main/cue/config/cue-config-repository.ts @@ -17,6 +17,7 @@ import { LEGACY_CUE_CONFIG_PATH, MAESTRO_DIR, } from '../../../shared/maestro-paths'; +import { captureException } from '../../utils/sentry'; /** * Resolve the cue config file path, preferring `.maestro/cue.yaml` @@ -93,11 +94,19 @@ export function writeCuePromptFile( } const promptsDir = path.resolve(path.join(projectRoot, CUE_PROMPTS_DIR)); const absPath = path.resolve(path.join(projectRoot, relativePath)); - if (!absPath.startsWith(promptsDir + path.sep) && absPath !== promptsDir) { + // Must be strictly inside .maestro/prompts/ — equality with promptsDir would + // mean the caller asked to write to the directory path itself. + if (!absPath.startsWith(promptsDir + path.sep)) { throw new Error( `writeCuePromptFile: path "${relativePath}" resolves outside the prompts directory` ); } + // Must be a .md file — pruneOrphanedPromptFiles only removes .md files, + // so any other extension would create a permanently orphaned file. Enforce + // the same trust boundary here as in the IPC layer (defense in depth). + if (path.extname(absPath).toLowerCase() !== '.md') { + throw new Error(`writeCuePromptFile: path "${relativePath}" must end with .md`); + } const dir = path.dirname(absPath); if (!fs.existsSync(dir)) { fs.mkdirSync(dir, { recursive: true }); @@ -106,6 +115,68 @@ export function writeCuePromptFile( return absPath; } +/** + * Remove `.md` files under `.maestro/prompts/` that are not referenced by the + * current YAML. Called after a successful `cue:writeYaml` so that renames and + * deletions do not leave orphan prompt files behind. + * + * `referencedRelativePaths` is the set of project-root-relative paths the YAML + * references (via `prompt_file` / `output_prompt_file`). Any `.md` file inside + * `.maestro/prompts/` whose relative path is not in this set is deleted. + * + * Silently skips when the prompts directory does not exist. Errors on + * individual files are swallowed to keep the save path non-fatal. + */ +export function pruneOrphanedPromptFiles( + projectRoot: string, + referencedRelativePaths: Iterable +): string[] { + const promptsDir = path.resolve(path.join(projectRoot, CUE_PROMPTS_DIR)); + if (!fs.existsSync(promptsDir)) return []; + + const keep = new Set(); + for (const rel of referencedRelativePaths) { + if (path.isAbsolute(rel)) continue; + keep.add(path.resolve(path.join(projectRoot, rel))); + } + + const removed: string[] = []; + const walk = (dir: string) => { + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch (err) { + // Don't re-throw: pruning runs AFTER a successful YAML write, and + // re-throwing here would surface a failed-save toast to the user + // even though the YAML did persist. Report to Sentry so we can see + // readdir failures in production, then continue (skip this dir). + captureException(err, { operation: 'pruneOrphanedPromptFiles.readdir', dir }); + return; + } + for (const entry of entries) { + const abs = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(abs); + continue; + } + if (!entry.isFile() || !entry.name.toLowerCase().endsWith('.md')) continue; + // Defense-in-depth: only touch files strictly inside the prompts dir. + if (!abs.startsWith(promptsDir + path.sep)) continue; + if (keep.has(abs)) continue; + try { + fs.unlinkSync(abs); + removed.push(abs); + } catch (err) { + // Same rationale as readdir: never let a per-file delete + // failure poison a successful save. Report and move on. + captureException(err, { operation: 'pruneOrphanedPromptFiles.unlink', file: abs }); + } + } + }; + walk(promptsDir); + return removed; +} + /** * Watches both canonical and legacy Cue config paths. * Debounces onChange by 1 second. diff --git a/src/main/cue/config/cue-config-validator.ts b/src/main/cue/config/cue-config-validator.ts index ed6571ddb..c5b79f3ec 100644 --- a/src/main/cue/config/cue-config-validator.ts +++ b/src/main/cue/config/cue-config-validator.ts @@ -17,251 +17,358 @@ function validateGlobPattern(pattern: string, prefix: string, errors: string[]): } } -export function validateCueConfigDocument(config: unknown): { valid: boolean; errors: string[] } { +/** + * Validate a single subscription. Returns errors specific to this subscription + * (with the supplied `prefix` prepended). Used both by the strict whole-config + * validator and the lenient per-subscription partitioner used by the loader. + * + * Note: name-uniqueness is a cross-subscription concern and lives in the caller. + */ +export function validateSubscription(sub: unknown, prefix: string): string[] { const errors: string[] = []; - if (!config || typeof config !== 'object') { - return { valid: false, errors: ['Config must be a non-null object'] }; + if (!sub || typeof sub !== 'object') { + errors.push(`${prefix}: must be an object`); + return errors; } - const cfg = config as Record; - - if (!Array.isArray(cfg.subscriptions)) { - errors.push('Config must have a "subscriptions" array'); - } else { - const seenNames = new Set(); - for (let i = 0; i < cfg.subscriptions.length; i++) { - const sub = cfg.subscriptions[i] as Record; - const prefix = `subscriptions[${i}]`; + const subRecord = sub as Record; - if (!sub || typeof sub !== 'object') { - errors.push(`${prefix}: must be an object`); - continue; - } + const normalized = + subRecord.name && typeof subRecord.name === 'string' ? String(subRecord.name).trim() : ''; + if (!normalized) { + errors.push(`${prefix}: "name" is required and must be a non-empty string`); + } - const normalized = sub.name && typeof sub.name === 'string' ? String(sub.name).trim() : ''; - if (!normalized) { - errors.push(`${prefix}: "name" is required and must be a non-empty string`); - } else if (seenNames.has(normalized)) { - errors.push(`${prefix}: duplicate subscription name "${normalized}"`); - } else { - seenNames.add(normalized); - } + if (!subRecord.event || typeof subRecord.event !== 'string') { + errors.push(`${prefix}: "event" is required and must be a string`); + } - if (!sub.event || typeof sub.event !== 'string') { - errors.push(`${prefix}: "event" is required and must be a string`); - } + if (subRecord.prompt !== undefined && typeof subRecord.prompt !== 'string') { + errors.push(`${prefix}: "prompt" must be a string when provided`); + } + if (subRecord.prompt_file !== undefined && typeof subRecord.prompt_file !== 'string') { + errors.push(`${prefix}: "prompt_file" must be a string when provided`); + } - if (sub.prompt !== undefined && typeof sub.prompt !== 'string') { - errors.push(`${prefix}: "prompt" must be a string when provided`); - } - if (sub.prompt_file !== undefined && typeof sub.prompt_file !== 'string') { - errors.push(`${prefix}: "prompt_file" must be a string when provided`); - } + const hasPrompt = typeof subRecord.prompt === 'string'; + const hasPromptFile = typeof subRecord.prompt_file === 'string'; + if (!hasPrompt && !hasPromptFile) { + errors.push(`${prefix}: "prompt" or "prompt_file" is required`); + } - const hasPrompt = typeof sub.prompt === 'string'; - const hasPromptFile = typeof sub.prompt_file === 'string'; - if (!hasPrompt && !hasPromptFile) { - errors.push(`${prefix}: "prompt" or "prompt_file" is required`); - } + validateEventSpecificFields(subRecord, prefix, errors); - const event = sub.event as string; - if (event === 'time.heartbeat') { + if (subRecord.filter !== undefined) { + if ( + typeof subRecord.filter !== 'object' || + subRecord.filter === null || + Array.isArray(subRecord.filter) + ) { + errors.push(`${prefix}: "filter" must be a plain object`); + } else { + for (const [filterKey, filterVal] of Object.entries( + subRecord.filter as Record + )) { if ( - typeof sub.interval_minutes !== 'number' || - !Number.isFinite(sub.interval_minutes) || - sub.interval_minutes <= 0 || - sub.interval_minutes > 10080 + typeof filterVal !== 'string' && + typeof filterVal !== 'number' && + typeof filterVal !== 'boolean' ) { errors.push( - `${prefix}: "interval_minutes" is required and must be a positive number no greater than 10080 (7 days) for time.heartbeat events` - ); - } - } else if (event === 'time.scheduled') { - if (!Array.isArray(sub.schedule_times) || sub.schedule_times.length === 0) { - errors.push( - `${prefix}: "schedule_times" is required and must be a non-empty array of time strings (e.g. ["09:00", "17:00"]) for time.scheduled events` + `${prefix}: filter key "${filterKey}" must be a string, number, or boolean (got ${typeof filterVal})` ); - } else { - const timeRegex = /^\d{2}:\d{2}$/; - for (const time of sub.schedule_times as string[]) { - if (typeof time !== 'string' || !timeRegex.test(time)) { - errors.push(`${prefix}: schedule_times value "${time}" must be in HH:MM format`); - } else { - const [hours, minutes] = time.split(':').map(Number); - if (hours < 0 || hours > 23 || minutes < 0 || minutes > 59) { - errors.push( - `${prefix}: schedule_times value "${time}" has invalid hour (0-23) or minute (0-59)` - ); - } - } - } } - if (sub.schedule_days !== undefined) { - if (!Array.isArray(sub.schedule_days)) { - errors.push( - `${prefix}: "schedule_days" must be an array of day names (mon, tue, wed, thu, fri, sat, sun)` - ); - } else { - for (const day of sub.schedule_days as string[]) { - if (!CUE_SCHEDULE_DAYS.includes(day as CueScheduleDay)) { - errors.push( - `${prefix}: schedule_days value "${day}" must be one of: ${CUE_SCHEDULE_DAYS.join(', ')}` - ); - } - } - } - } - } else if (event === 'file.changed') { - if (!sub.watch || typeof sub.watch !== 'string') { - errors.push( - `${prefix}: "watch" is required and must be a non-empty string for file.changed events` - ); + } + } + } + + return errors; +} + +function validateEventSpecificFields( + sub: Record, + prefix: string, + errors: string[] +): void { + const event = sub.event as string; + if (event === 'time.heartbeat') { + if ( + typeof sub.interval_minutes !== 'number' || + !Number.isFinite(sub.interval_minutes) || + sub.interval_minutes <= 0 || + sub.interval_minutes > 10080 + ) { + errors.push( + `${prefix}: "interval_minutes" is required and must be a positive number no greater than 10080 (7 days) for time.heartbeat events` + ); + } + } else if (event === 'time.scheduled') { + if (!Array.isArray(sub.schedule_times) || sub.schedule_times.length === 0) { + errors.push( + `${prefix}: "schedule_times" is required and must be a non-empty array of time strings (e.g. ["09:00", "17:00"]) for time.scheduled events` + ); + } else { + const timeRegex = /^\d{2}:\d{2}$/; + for (const time of sub.schedule_times as string[]) { + if (typeof time !== 'string' || !timeRegex.test(time)) { + errors.push(`${prefix}: schedule_times value "${time}" must be in HH:MM format`); } else { - validateGlobPattern(sub.watch as string, prefix, errors); - } - } else if (event === 'agent.completed') { - if (!sub.source_session) { - errors.push(`${prefix}: "source_session" is required for agent.completed events`); - } else if (typeof sub.source_session !== 'string' && !Array.isArray(sub.source_session)) { - errors.push( - `${prefix}: "source_session" must be a string or array of strings for agent.completed events` - ); - } else if ( - typeof sub.source_session === 'string' && - sub.source_session.trim().length === 0 - ) { - errors.push( - `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` - ); - } else if (Array.isArray(sub.source_session)) { - if (sub.source_session.length === 0) { - errors.push( - `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` - ); - } else if ( - sub.source_session.some( - (source) => typeof source !== 'string' || source.trim().length === 0 - ) - ) { + const [hours, minutes] = time.split(':').map(Number); + if (hours < 0 || hours > 23 || minutes < 0 || minutes > 59) { errors.push( - `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` + `${prefix}: schedule_times value "${time}" has invalid hour (0-23) or minute (0-59)` ); } } - } else if (event === 'task.pending') { - if (!sub.watch || typeof sub.watch !== 'string') { - errors.push( - `${prefix}: "watch" is required and must be a non-empty glob string for task.pending events` - ); - } else { - validateGlobPattern(sub.watch as string, prefix, errors); - } - if (sub.poll_minutes !== undefined) { - if ( - typeof sub.poll_minutes !== 'number' || - !Number.isFinite(sub.poll_minutes) || - sub.poll_minutes < 1 - ) { - errors.push(`${prefix}: "poll_minutes" must be a number >= 1 for task.pending events`); - } - } - } else if (event === 'github.pull_request' || event === 'github.issue') { - if (sub.repo !== undefined && typeof sub.repo !== 'string') { - errors.push( - `${prefix}: "repo" must be a string (e.g., "owner/repo") for ${event} events` - ); - } - if (sub.poll_minutes !== undefined) { - if ( - typeof sub.poll_minutes !== 'number' || - !Number.isFinite(sub.poll_minutes) || - sub.poll_minutes < 1 - ) { - errors.push(`${prefix}: "poll_minutes" must be a number >= 1 for ${event} events`); - } - } - if (sub.gh_state !== undefined) { - if ( - typeof sub.gh_state !== 'string' || - !CUE_GITHUB_STATES.includes(sub.gh_state as CueGitHubState) - ) { - errors.push(`${prefix}: "gh_state" must be one of: ${CUE_GITHUB_STATES.join(', ')}`); - } - if (sub.gh_state === 'merged' && event === 'github.issue') { + } + } + if (sub.schedule_days !== undefined) { + if (!Array.isArray(sub.schedule_days)) { + errors.push( + `${prefix}: "schedule_days" must be an array of day names (mon, tue, wed, thu, fri, sat, sun)` + ); + } else { + for (const day of sub.schedule_days as string[]) { + if (!CUE_SCHEDULE_DAYS.includes(day as CueScheduleDay)) { errors.push( - `${prefix}: "gh_state" value "merged" is only valid for github.pull_request events` + `${prefix}: schedule_days value "${day}" must be one of: ${CUE_SCHEDULE_DAYS.join(', ')}` ); } } - } else if (event === 'app.startup') { - // No additional required fields for the startup trigger. - } else if (event === 'cli.trigger') { - // No additional required fields — triggered manually via maestro-cli. + } + } + } else if (event === 'file.changed') { + if (!sub.watch || typeof sub.watch !== 'string') { + errors.push( + `${prefix}: "watch" is required and must be a non-empty string for file.changed events` + ); + } else { + validateGlobPattern(sub.watch as string, prefix, errors); + } + } else if (event === 'agent.completed') { + if (!sub.source_session) { + errors.push(`${prefix}: "source_session" is required for agent.completed events`); + } else if (typeof sub.source_session !== 'string' && !Array.isArray(sub.source_session)) { + errors.push( + `${prefix}: "source_session" must be a string or array of strings for agent.completed events` + ); + } else if (typeof sub.source_session === 'string' && sub.source_session.trim().length === 0) { + errors.push( + `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` + ); + } else if (Array.isArray(sub.source_session)) { + if (sub.source_session.length === 0) { + errors.push( + `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` + ); } else if ( - sub.event && - typeof sub.event === 'string' && - !CUE_EVENT_TYPES.includes(event as any) + sub.source_session.some( + (source) => typeof source !== 'string' || source.trim().length === 0 + ) + ) { + errors.push( + `${prefix}: "source_session" must be a non-empty string or non-empty array of non-empty strings for agent.completed events` + ); + } + } + } else if (event === 'task.pending') { + if (!sub.watch || typeof sub.watch !== 'string') { + errors.push( + `${prefix}: "watch" is required and must be a non-empty glob string for task.pending events` + ); + } else { + validateGlobPattern(sub.watch as string, prefix, errors); + } + if (sub.poll_minutes !== undefined) { + if ( + typeof sub.poll_minutes !== 'number' || + !Number.isFinite(sub.poll_minutes) || + sub.poll_minutes < 1 + ) { + errors.push(`${prefix}: "poll_minutes" must be a number >= 1 for task.pending events`); + } + } + } else if (event === 'github.pull_request' || event === 'github.issue') { + if (sub.repo !== undefined && typeof sub.repo !== 'string') { + errors.push(`${prefix}: "repo" must be a string (e.g., "owner/repo") for ${event} events`); + } + if (sub.poll_minutes !== undefined) { + if ( + typeof sub.poll_minutes !== 'number' || + !Number.isFinite(sub.poll_minutes) || + sub.poll_minutes < 1 ) { + errors.push(`${prefix}: "poll_minutes" must be a number >= 1 for ${event} events`); + } + } + if (sub.gh_state !== undefined) { + if ( + typeof sub.gh_state !== 'string' || + !CUE_GITHUB_STATES.includes(sub.gh_state as CueGitHubState) + ) { + errors.push(`${prefix}: "gh_state" must be one of: ${CUE_GITHUB_STATES.join(', ')}`); + } + if (sub.gh_state === 'merged' && event === 'github.issue') { errors.push( - `${prefix}: unknown event type "${event}". Valid types: ${CUE_EVENT_TYPES.join(', ')}` + `${prefix}: "gh_state" value "merged" is only valid for github.pull_request events` ); } + } + } else if (event === 'app.startup') { + // No additional required fields for the startup trigger. + } else if (event === 'cli.trigger') { + // No additional required fields — triggered manually via maestro-cli. + } else if ( + sub.event && + typeof sub.event === 'string' && + !CUE_EVENT_TYPES.includes(event as any) + ) { + errors.push( + `${prefix}: unknown event type "${event}". Valid types: ${CUE_EVENT_TYPES.join(', ')}` + ); + } +} - if (sub.filter !== undefined) { - if (typeof sub.filter !== 'object' || sub.filter === null || Array.isArray(sub.filter)) { - errors.push(`${prefix}: "filter" must be a plain object`); - } else { - for (const [filterKey, filterVal] of Object.entries( - sub.filter as Record - )) { - if ( - typeof filterVal !== 'string' && - typeof filterVal !== 'number' && - typeof filterVal !== 'boolean' - ) { - errors.push( - `${prefix}: filter key "${filterKey}" must be a string, number, or boolean (got ${typeof filterVal})` - ); +function validateSettings(rawSettings: unknown): string[] { + const errors: string[] = []; + if (rawSettings === undefined) return errors; + if (typeof rawSettings !== 'object' || rawSettings === null || Array.isArray(rawSettings)) { + errors.push('"settings" must be an object'); + return errors; + } + const settings = rawSettings as Record; + if (settings.timeout_on_fail !== undefined) { + if (settings.timeout_on_fail !== 'break' && settings.timeout_on_fail !== 'continue') { + errors.push('"settings.timeout_on_fail" must be "break" or "continue"'); + } + } + if (settings.max_concurrent !== undefined) { + if ( + typeof settings.max_concurrent !== 'number' || + !Number.isInteger(settings.max_concurrent) || + settings.max_concurrent < 1 || + settings.max_concurrent > 10 + ) { + errors.push('"settings.max_concurrent" must be a positive integer between 1 and 10'); + } + } + if (settings.queue_size !== undefined) { + if ( + typeof settings.queue_size !== 'number' || + !Number.isInteger(settings.queue_size) || + settings.queue_size < 0 || + settings.queue_size > 50 + ) { + errors.push('"settings.queue_size" must be a non-negative integer between 0 and 50'); + } + } + return errors; +} + +export function validateCueConfigDocument(config: unknown): { valid: boolean; errors: string[] } { + const errors: string[] = []; + + if (!config || typeof config !== 'object') { + return { valid: false, errors: ['Config must be a non-null object'] }; + } + + const cfg = config as Record; + + if (!Array.isArray(cfg.subscriptions)) { + errors.push('Config must have a "subscriptions" array'); + } else { + const seenNames = new Set(); + for (let i = 0; i < cfg.subscriptions.length; i++) { + const sub = cfg.subscriptions[i]; + const prefix = `subscriptions[${i}]`; + errors.push(...validateSubscription(sub, prefix)); + + if (sub && typeof sub === 'object') { + const name = (sub as Record).name; + if (typeof name === 'string') { + const normalized = name.trim(); + if (normalized) { + if (seenNames.has(normalized)) { + errors.push(`${prefix}: duplicate subscription name "${normalized}"`); } + seenNames.add(normalized); } } } } } - if (cfg.settings !== undefined) { - if (typeof cfg.settings !== 'object' || cfg.settings === null || Array.isArray(cfg.settings)) { - errors.push('"settings" must be an object'); - } else { - const settings = cfg.settings as Record; - if (settings.timeout_on_fail !== undefined) { - if (settings.timeout_on_fail !== 'break' && settings.timeout_on_fail !== 'continue') { - errors.push('"settings.timeout_on_fail" must be "break" or "continue"'); - } - } - if (settings.max_concurrent !== undefined) { - if ( - typeof settings.max_concurrent !== 'number' || - !Number.isInteger(settings.max_concurrent) || - settings.max_concurrent < 1 || - settings.max_concurrent > 10 - ) { - errors.push('"settings.max_concurrent" must be a positive integer between 1 and 10'); - } - } - if (settings.queue_size !== undefined) { - if ( - typeof settings.queue_size !== 'number' || - !Number.isInteger(settings.queue_size) || - settings.queue_size < 0 || - settings.queue_size > 50 - ) { - errors.push('"settings.queue_size" must be a non-negative integer between 0 and 50'); + errors.push(...validateSettings(cfg.settings)); + + return { valid: errors.length === 0, errors }; +} + +/** + * Lenient partition for the loader: returns indices of subscriptions that + * passed validation along with per-subscription errors for the failures, plus + * any config-level errors (missing subscriptions array, bad settings). + * + * Used to load valid subs while logging warnings for broken ones — one bad + * subscription must not block an entire project's Cue config (which can be + * shared across multiple agents in the same project root). + */ +export interface PartitionedValidation { + configErrors: string[]; + validIndices: number[]; + subscriptionErrors: Array<{ index: number; errors: string[] }>; +} + +export function partitionValidSubscriptions(config: unknown): PartitionedValidation { + const result: PartitionedValidation = { + configErrors: [], + validIndices: [], + subscriptionErrors: [], + }; + + if (!config || typeof config !== 'object') { + result.configErrors.push('Config must be a non-null object'); + return result; + } + + const cfg = config as Record; + + if (!Array.isArray(cfg.subscriptions)) { + result.configErrors.push('Config must have a "subscriptions" array'); + return result; + } + + const seenNames = new Set(); + for (let i = 0; i < cfg.subscriptions.length; i++) { + const sub = cfg.subscriptions[i]; + const prefix = `subscriptions[${i}]`; + const subErrors = validateSubscription(sub, prefix); + + // Cross-subscription: duplicate names. Mark the duplicate as broken. + let dupeError: string | null = null; + if (sub && typeof sub === 'object') { + const name = (sub as Record).name; + if (typeof name === 'string') { + const normalized = name.trim(); + if (normalized) { + if (seenNames.has(normalized)) { + dupeError = `${prefix}: duplicate subscription name "${normalized}" — skipped`; + } else { + seenNames.add(normalized); + } } } } + + const allErrors = dupeError ? [...subErrors, dupeError] : subErrors; + if (allErrors.length === 0) { + result.validIndices.push(i); + } else { + result.subscriptionErrors.push({ index: i, errors: allErrors }); + } } - return { valid: errors.length === 0, errors }; + result.configErrors.push(...validateSettings(cfg.settings)); + + return result; } diff --git a/src/main/cue/cue-cleanup-service.ts b/src/main/cue/cue-cleanup-service.ts new file mode 100644 index 000000000..7c8bca046 --- /dev/null +++ b/src/main/cue/cue-cleanup-service.ts @@ -0,0 +1,93 @@ +/** + * Cue Cleanup Service — periodic sweep of stale fan-in trackers and + * time.scheduled dedup keys. + * + * Designed to be called on every heartbeat tick via an `onTick` callback + * passed to `createCueHeartbeat`. Actually sweeps every CLEANUP_INTERVAL_TICKS + * ticks (≈5 minutes at 30s heartbeat). Evicts: + * 1. Fan-in trackers whose owner session is no longer registered + * 2. Fan-in trackers open longer than 2× their session's timeout_minutes + * 3. time.scheduled dedup keys whose time component differs from the current minute + */ + +import type { MainLogLevel } from '../../shared/logger-types'; +import type { CueFanInTracker } from './cue-fan-in-tracker'; +import type { CueSessionRegistry } from './cue-session-registry'; + +/** Number of heartbeat ticks between sweeps (10 × 30 s = 5 minutes). */ +export const CLEANUP_INTERVAL_TICKS = 10; + +export interface CueCleanupServiceDeps { + fanInTracker: CueFanInTracker; + registry: CueSessionRegistry; + /** Returns active session IDs used to detect orphaned fan-in trackers. */ + getSessions: () => { id: string }[]; + /** Returns the configured timeout in milliseconds for a session. */ + getSessionTimeoutMs: (sessionId: string) => number; + /** Returns the current wall-clock minute as "HH:MM". */ + getCurrentMinute: () => string; + onLog: (level: MainLogLevel, message: string) => void; +} + +export interface CueCleanupService { + /** Increment tick counter; triggers a sweep every CLEANUP_INTERVAL_TICKS ticks. */ + onTick(): void; + /** Run a sweep immediately (useful for testing or on-demand maintenance). */ + sweep(): { fanInEvicted: number; scheduledKeysEvicted: number }; +} + +export function createCueCleanupService(deps: CueCleanupServiceDeps): CueCleanupService { + let tickCount = 0; + + function sweep(): { fanInEvicted: number; scheduledKeysEvicted: number } { + let fanInEvicted = 0; + const activeSessions = new Set(deps.getSessions().map((s) => s.id)); + + for (const key of deps.fanInTracker.getActiveTrackerKeys()) { + // key format: "${ownerSessionId}:${subName}" + const colonIdx = key.indexOf(':'); + const ownerSessionId = colonIdx === -1 ? key : key.substring(0, colonIdx); + + // Evict if the owning session is no longer registered + if (!activeSessions.has(ownerSessionId)) { + deps.fanInTracker.expireTracker(key); + fanInEvicted++; + deps.onLog('warn', `[CUE] Evicted stale fan-in tracker for removed session: ${key}`); + continue; + } + + // Evict if the tracker has been open longer than 2× the session's timeout + const createdAt = deps.fanInTracker.getTrackerCreatedAt(key); + if (createdAt !== undefined) { + const timeoutMs = deps.getSessionTimeoutMs(ownerSessionId); + const ageMs = Date.now() - createdAt; + if (ageMs > 2 * timeoutMs) { + deps.fanInTracker.expireTracker(key); + fanInEvicted++; + deps.onLog( + 'warn', + `[CUE] Evicted stale fan-in tracker (age ${Math.round(ageMs / 60000)}m > 2× timeout): ${key}` + ); + } + } + } + + // Sweep time.scheduled dedup keys that no longer match the current minute + const scheduledKeysEvicted = deps.registry.sweepStaleScheduledKeys(deps.getCurrentMinute()); + if (scheduledKeysEvicted > 0) { + deps.onLog('info', `[CUE] Swept ${scheduledKeysEvicted} stale scheduled key(s)`); + } + + return { fanInEvicted, scheduledKeysEvicted }; + } + + return { + onTick(): void { + tickCount++; + if (tickCount % CLEANUP_INTERVAL_TICKS === 0) { + sweep(); + } + }, + sweep, + }; +} diff --git a/src/main/cue/cue-db.ts b/src/main/cue/cue-db.ts index 107965097..0aa597360 100644 --- a/src/main/cue/cue-db.ts +++ b/src/main/cue/cue-db.ts @@ -10,6 +10,7 @@ import Database from 'better-sqlite3'; import * as path from 'path'; import * as fs from 'fs'; import { app } from 'electron'; +import { captureException } from '../utils/sentry'; const LOG_CONTEXT = '[CueDB]'; @@ -191,6 +192,54 @@ export function updateCueEventStatus(id: string, status: string): void { .run(status, Date.now(), id); } +/** + * Safe wrapper: records a Cue event; logs warn on failure instead of throwing. + * Non-fatal — callers must not rely on successful persistence. + */ +export function safeRecordCueEvent(event: Parameters[0]): void { + try { + recordCueEvent(event); + } catch (err) { + log( + 'warn', + `Failed to record Cue event (id=${event.id}): ${err instanceof Error ? err.message : String(err)}` + ); + // Persist warns to Sentry too — DB write failures here are silent at + // runtime (callers must remain non-fatal) but accumulate observability + // gaps if not surfaced; keep returning without throwing. + // Strip event.payload before reporting: for agent.completed runs it + // contains the upstream agent's stdout (sourceOutput), which can carry + // user content / secrets. Send only identifiers + size. + const sanitizedEvent = { + id: event.id, + type: event.type, + triggerName: event.triggerName, + sessionId: event.sessionId, + subscriptionName: event.subscriptionName, + status: event.status, + payloadSize: event.payload?.length ?? 0, + payloadRedacted: event.payload != null, + }; + captureException(err, { operation: 'safeRecordCueEvent', event: sanitizedEvent }); + } +} + +/** + * Safe wrapper: updates Cue event status; logs warn on failure instead of throwing. + * Non-fatal — callers must not rely on successful persistence. + */ +export function safeUpdateCueEventStatus(id: string, status: string): void { + try { + updateCueEventStatus(id, status); + } catch (err) { + log( + 'warn', + `Failed to update Cue event status (id=${id}, status=${status}): ${err instanceof Error ? err.message : String(err)}` + ); + captureException(err, { operation: 'safeUpdateCueEventStatus', id, status }); + } +} + /** * Retrieve recent Cue events created after a given timestamp. */ diff --git a/src/main/cue/cue-engine.ts b/src/main/cue/cue-engine.ts index f7ad7f87f..5410185ae 100644 --- a/src/main/cue/cue-engine.ts +++ b/src/main/cue/cue-engine.ts @@ -50,6 +50,7 @@ import { createCueSessionRuntimeService } from './cue-session-runtime-service'; import type { CueSessionRuntimeService, SessionInitReason } from './cue-session-runtime-service'; import { createCueSessionRegistry, type CueSessionRegistry } from './cue-session-registry'; import { createCueRecoveryService, type CueRecoveryService } from './cue-recovery-service'; +import { createCueCleanupService, type CueCleanupService } from './cue-cleanup-service'; import { loadCueConfig } from './cue-yaml-loader'; const MAX_CHAIN_DEPTH = 10; @@ -85,6 +86,7 @@ export class CueEngine { private queryService: CueQueryService; private sessionRuntimeService: CueSessionRuntimeService; private recoveryService: CueRecoveryService; + private cleanupService: CueCleanupService; private deps: CueEngineDeps; constructor(deps: CueEngineDeps) { @@ -205,7 +207,21 @@ export class CueEngine { getActiveRunCount: (sessionId) => this.runManager.getActiveRunCount(sessionId), loadConfigForProjectRoot: loadCueConfig, }); - this.heartbeat = createCueHeartbeat(); + this.cleanupService = createCueCleanupService({ + fanInTracker: this.fanInTracker, + registry: this.registry, + getSessions: () => deps.getSessions().map((s) => ({ id: s.id })), + getSessionTimeoutMs: (sessionId) => { + const state = this.registry.get(sessionId); + return (state?.config.settings?.timeout_minutes ?? 30) * 60 * 1000; + }, + getCurrentMinute: () => { + const now = new Date(); + return `${String(now.getHours()).padStart(2, '0')}:${String(now.getMinutes()).padStart(2, '0')}`; + }, + onLog: deps.onLog, + }); + this.heartbeat = createCueHeartbeat(() => this.cleanupService.onTick()); this.recoveryService = createCueRecoveryService({ onLog: deps.onLog, getSessions: () => { diff --git a/src/main/cue/cue-fan-in-tracker.ts b/src/main/cue/cue-fan-in-tracker.ts index e8b622ea8..7aaa7af44 100644 --- a/src/main/cue/cue-fan-in-tracker.ts +++ b/src/main/cue/cue-fan-in-tracker.ts @@ -52,11 +52,19 @@ export interface CueFanInTracker { ): void; clearForSession(sessionId: string): void; reset(): void; + /** Returns all active tracker keys (for cleanup inspection). */ + getActiveTrackerKeys(): string[]; + /** Returns the ms timestamp when the first completion arrived for a tracker, or undefined if not found. */ + getTrackerCreatedAt(key: string): number | undefined; + /** Force-expire a tracker by key without dispatching or waiting for timeout. Used by the cleanup service. */ + expireTracker(key: string): void; } export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { const fanInTrackers = new Map>(); const fanInTimers = new Map>(); + /** Tracks when the first completion arrived for each tracker key (for cleanup staleness checks). */ + const fanInCreatedAt = new Map(); /** * Resolve a user-authored `sources` list (names or IDs, possibly mixed) to a @@ -114,6 +122,7 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { // Fire with partial data const completions = [...tracker.values()]; fanInTrackers.delete(key); + fanInCreatedAt.delete(key); const event = createCueEvent('agent.completed', sub.name, { completedSessions: completions.map((c) => c.sessionId), @@ -139,6 +148,7 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { } else { // 'break' mode — log failure and clear fanInTrackers.delete(key); + fanInCreatedAt.delete(key); deps.onLog( 'cue', `[CUE] Fan-in "${sub.name}" timed out (break mode) — ${completedNames.length}/${totalSources} completed, waiting for: ${timedOutSources.join(', ')}` @@ -173,6 +183,7 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { // Start timeout timer on first source completion if (tracker.size === 1 && !fanInTimers.has(key)) { + fanInCreatedAt.set(key, Date.now()); const timeoutMs = (sub.fan_in_timeout_minutes ?? settings.timeout_minutes ?? 30) * 60 * 1000; const timer = setTimeout(() => { @@ -205,6 +216,12 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { fanInTimers.delete(key); } fanInTrackers.delete(key); + // Drop the timestamp alongside the tracker — leaving it behind would + // leak a key into fanInCreatedAt forever (the success path used to + // only delete fanInTrackers, while every other path — timeout/break + // modes, clearForSession, expireTracker, reset — already cleaned up + // fanInCreatedAt correctly). + fanInCreatedAt.delete(key); const completions = [...tracker.values()]; const event = createCueEvent('agent.completed', sub.name, { @@ -229,6 +246,7 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { for (const key of [...fanInTrackers.keys()]) { if (key.startsWith(`${sessionId}:`)) { fanInTrackers.delete(key); + fanInCreatedAt.delete(key); const timer = fanInTimers.get(key); if (timer) { clearTimeout(timer); @@ -244,6 +262,25 @@ export function createCueFanInTracker(deps: CueFanInDeps): CueFanInTracker { } fanInTrackers.clear(); fanInTimers.clear(); + fanInCreatedAt.clear(); + }, + + getActiveTrackerKeys(): string[] { + return [...fanInTrackers.keys()]; + }, + + getTrackerCreatedAt(key: string): number | undefined { + return fanInCreatedAt.get(key); + }, + + expireTracker(key: string): void { + fanInTrackers.delete(key); + fanInCreatedAt.delete(key); + const timer = fanInTimers.get(key); + if (timer) { + clearTimeout(timer); + fanInTimers.delete(key); + } }, }; } diff --git a/src/main/cue/cue-heartbeat.ts b/src/main/cue/cue-heartbeat.ts index a357faf2c..ff39645cf 100644 --- a/src/main/cue/cue-heartbeat.ts +++ b/src/main/cue/cue-heartbeat.ts @@ -19,7 +19,7 @@ export interface CueHeartbeat { stop(): void; } -export function createCueHeartbeat(): CueHeartbeat { +export function createCueHeartbeat(onTick?: () => void): CueHeartbeat { let heartbeatInterval: ReturnType | null = null; function startHeartbeat(): void { @@ -35,6 +35,7 @@ export function createCueHeartbeat(): CueHeartbeat { } catch { // Non-fatal } + onTick?.(); }, HEARTBEAT_INTERVAL_MS); } diff --git a/src/main/cue/cue-process-lifecycle.ts b/src/main/cue/cue-process-lifecycle.ts index 68f5b69e3..c3e0b05ca 100644 --- a/src/main/cue/cue-process-lifecycle.ts +++ b/src/main/cue/cue-process-lifecycle.ts @@ -152,11 +152,21 @@ export function runProcess( return new Promise((resolve) => { let child: ChildProcess; + // Only attach a writable stdin pipe when the SSH wrapper actually + // needs to write a script or prompt down it. In local mode the prompt + // is already passed as a CLI argument, and leaving stdin as an open + // pipe causes some agents (notably Codex `exec`) to emit "Reading + // additional input from stdin..." into the run output before they + // observe EOF. `'ignore'` gives the child /dev/null for stdin so it + // never tries to read — Claude already behaves correctly with either, + // so this is safe across all agents. + const needsStdinWrite = sshRemoteEnabled && (Boolean(sshStdinScript) || Boolean(stdinPrompt)); + const stdinMode: 'pipe' | 'ignore' = needsStdinWrite ? 'pipe' : 'ignore'; try { child = spawn(spec.command, spec.args, { cwd: spec.cwd, env: spec.env, - stdio: ['pipe', 'pipe', 'pipe'], + stdio: [stdinMode, 'pipe', 'pipe'], }); } catch (err) { captureException(err, { operation: 'cue:spawn', runId, command: spec.command }); diff --git a/src/main/cue/cue-query-service.ts b/src/main/cue/cue-query-service.ts index 5769feb8f..f11a7c246 100644 --- a/src/main/cue/cue-query-service.ts +++ b/src/main/cue/cue-query-service.ts @@ -89,7 +89,13 @@ export function createCueQueryService(deps: CueQueryServiceDeps): CueQueryServic sessionId, sessionName: session.name, toolType: session.toolType, - subscriptions: state.config.subscriptions, + // Only report subscriptions that belong to this session. A subscription + // with no agent_id is unbound (legacy / shared) and surfaces under every + // session sharing this project root; one with an agent_id is owned + // exclusively by that session. + subscriptions: state.config.subscriptions.filter( + (sub) => !sub.agent_id || sub.agent_id === sessionId + ), }); } @@ -103,7 +109,9 @@ export function createCueQueryService(deps: CueQueryServiceDeps): CueQueryServic sessionId: session.id, sessionName: session.name, toolType: session.toolType, - subscriptions: config.subscriptions, + subscriptions: config.subscriptions.filter( + (sub) => !sub.agent_id || sub.agent_id === session.id + ), }); } } diff --git a/src/main/cue/cue-run-manager.ts b/src/main/cue/cue-run-manager.ts index 03b6553bc..8be2e7fac 100644 --- a/src/main/cue/cue-run-manager.ts +++ b/src/main/cue/cue-run-manager.ts @@ -13,7 +13,7 @@ import * as crypto from 'crypto'; import type { MainLogLevel } from '../../shared/logger-types'; import type { CueEvent, CueRunResult, CueSettings, CueSubscription } from './cue-types'; -import { recordCueEvent, updateCueEventStatus } from './cue-db'; +import { updateCueEventStatus, safeRecordCueEvent, safeUpdateCueEventStatus } from './cue-db'; import { SOURCE_OUTPUT_MAX_CHARS } from './cue-fan-in-tracker'; import { captureException } from '../utils/sentry'; @@ -184,19 +184,15 @@ export function createCueRunManager(deps: CueRunManagerDeps): CueRunManager { activeRuns.set(runId, { result, abortController, phase: 'running' }); deps.onPreventSleep?.(`cue:run:${runId}`); const timeoutMs = (settings?.timeout_minutes ?? 30) * 60 * 1000; - try { - recordCueEvent({ - id: runId, - type: event.type, - triggerName: event.triggerName, - sessionId, - subscriptionName, - status: 'running', - payload: JSON.stringify(event.payload), - }); - } catch { - // Non-fatal if DB is unavailable - } + safeRecordCueEvent({ + id: runId, + type: event.type, + triggerName: event.triggerName, + sessionId, + subscriptionName, + status: 'running', + payload: JSON.stringify(event.payload), + }); deps.onLog('cue', `[CUE] Run started: ${subscriptionName}`, { type: 'runStarted', runId, @@ -239,40 +235,43 @@ export function createCueRunManager(deps: CueRunManagerDeps): CueRunManager { }, }; - try { - recordCueEvent({ - id: outputRunId, - type: event.type, - triggerName: event.triggerName, - sessionId, - subscriptionName: `${subscriptionName}:output`, - status: 'running', - payload: JSON.stringify(outputEvent.payload), - }); - } catch { - // Non-fatal if DB is unavailable - } + safeRecordCueEvent({ + id: outputRunId, + type: event.type, + triggerName: event.triggerName, + sessionId, + subscriptionName: `${subscriptionName}:output`, + status: 'running', + payload: JSON.stringify(outputEvent.payload), + }); // Track the output prompt's process ID so stopRun can kill it const run = activeRuns.get(runId); if (run) run.processRunId = outputRunId; const contextPrompt = `${outputPrompt}\n\n---\n\nContext from completed task:\n${result.stdout.substring(0, SOURCE_OUTPUT_MAX_CHARS)}`; - const outputResult = await deps.onCueRun({ - runId: outputRunId, - sessionId, - prompt: contextPrompt, - subscriptionName: `${subscriptionName}:output`, - event: outputEvent, - timeoutMs, - }); - + let outputResult: CueRunResult; try { - updateCueEventStatus(outputRunId, outputResult.status); - } catch { - // Non-fatal if DB is unavailable + outputResult = await deps.onCueRun({ + runId: outputRunId, + sessionId, + prompt: contextPrompt, + subscriptionName: `${subscriptionName}:output`, + event: outputEvent, + timeoutMs, + }); + } catch (outputErr) { + // onCueRun rejected — the outputRunId DB row is still 'running' + // because safeUpdateCueEventStatus below was skipped. Finalize it + // here so the activity log doesn't show a phantom never-ending + // run, then re-throw so the outer catch records the failure on + // the parent run. + safeUpdateCueEventStatus(outputRunId, 'failed'); + throw outputErr; } + safeUpdateCueEventStatus(outputRunId, outputResult.status); + if (!activeRuns.has(runId)) { return; } diff --git a/src/main/cue/cue-session-registry.ts b/src/main/cue/cue-session-registry.ts index 98594cf92..20cc66f3f 100644 --- a/src/main/cue/cue-session-registry.ts +++ b/src/main/cue/cue-session-registry.ts @@ -62,6 +62,13 @@ export interface CueSessionRegistry { * (i.e., a new registry instance). */ clear(): void; + + /** + * Sweep all `time.scheduled` fired-keys whose time component does not match + * `currentTime` ("HH:MM"). Returns the number of evicted keys. + * Intended for periodic cleanup to prevent unbounded growth of the dedup set. + */ + sweepStaleScheduledKeys(currentTime: string): number; } export function createCueSessionRegistry(): CueSessionRegistry { @@ -149,5 +156,19 @@ export function createCueSessionRegistry(): CueSessionRegistry { scheduledFiredKeys.clear(); // startupFiredKeys deliberately preserved across stop/start. }, + + sweepStaleScheduledKeys(currentTime: string): number { + // Keys have format: ${sessionId}:${subName}:HH:MM + // The time is always the trailing ":HH:MM" suffix (e.g. ":09:30"). + const suffix = `:${currentTime}`; + let evicted = 0; + for (const key of scheduledFiredKeys) { + if (!key.endsWith(suffix)) { + scheduledFiredKeys.delete(key); + evicted++; + } + } + return evicted; + }, }; } diff --git a/src/main/cue/cue-session-runtime-service.ts b/src/main/cue/cue-session-runtime-service.ts index 1ac69d332..f3d0c8407 100644 --- a/src/main/cue/cue-session-runtime-service.ts +++ b/src/main/cue/cue-session-runtime-service.ts @@ -85,6 +85,18 @@ export function createCueSessionRuntimeService( function initSession(session: SessionInfo, opts: InitSessionOptions): void { if (!deps.enabled()) return; + // Idempotency guard: tear down any pre-existing registration to prevent + // duplicate trigger sources if initSession is called twice for the same + // session (race between auto-discovery and manual refresh). + if (registry.has(session.id)) { + deps.onLog( + 'warn', + `[CUE] initSession called for already-initialized session "${session.name}" — tearing down first` + ); + teardownSession(session.id); + registry.unregister(session.id); + } + const loadResult = loadCueConfigDetailed(session.projectRoot); if (!loadResult.ok) { // Distinguish missing (silent) from parse / validation failures (loud). diff --git a/src/main/cue/cue-yaml-loader.ts b/src/main/cue/cue-yaml-loader.ts index 78c5da8ee..d4905dc35 100644 --- a/src/main/cue/cue-yaml-loader.ts +++ b/src/main/cue/cue-yaml-loader.ts @@ -9,7 +9,10 @@ import * as yaml from 'js-yaml'; import type { CueConfig } from './cue-types'; import { readCueConfigFile, watchCueConfigFile } from './config/cue-config-repository'; import { materializeCueConfig, parseCueConfigDocument } from './config/cue-config-normalizer'; -import { validateCueConfigDocument } from './config/cue-config-validator'; +import { + partitionValidSubscriptions, + validateCueConfigDocument, +} from './config/cue-config-validator'; export { resolveCueConfigPath } from './config/cue-config-repository'; @@ -60,23 +63,69 @@ export function loadCueConfigDetailed(projectRoot: string): LoadCueConfigDetaile }; } - const validation = validateCueConfigDocument(parsed); - if (!validation.valid) { - return { ok: false, reason: 'invalid', errors: validation.errors }; + // Lenient partition: config-level errors (missing subscriptions array, bad + // settings) are still fatal, but per-subscription errors only drop that one + // subscription. A single malformed subscription must not block valid + // pipelines belonging to other agents that share the same project root. + const partitioned = partitionValidSubscriptions(parsed); + if (partitioned.configErrors.length > 0) { + return { ok: false, reason: 'invalid', errors: partitioned.configErrors }; } const document = parseCueConfigDocument(file.raw, projectRoot); if (!document) { - // Should be unreachable since validation passed, but guard defensively. + // Should be unreachable since the config-level shape passed, but guard defensively. return { ok: false, reason: 'parse-error', - message: 'Cue config could not be normalized after validation', + message: 'Cue config could not be normalized', }; } - const materialized = materializeCueConfig(document); - return { ok: true, config: materialized.config, warnings: materialized.warnings }; + // Map raw-YAML subscription indices (used by the validator) to their + // position in document.subscriptions, which is the normalized array. + // parseCueConfigDocument silently skips raw entries that aren't objects + // (e.g. `- "string-not-an-object"`), so the two arrays' indices drift — + // filtering the normalized array using raw-index errors would drop the + // wrong subscriptions. Build a translation table from the raw array. + const rawSubs = (parsed as Record).subscriptions as unknown[]; + const rawToNormalized = new Map(); + let normIdx = 0; + for (let i = 0; i < rawSubs.length; i++) { + const entry = rawSubs[i]; + if (entry && typeof entry === 'object') { + rawToNormalized.set(i, normIdx); + normIdx++; + } + } + + const skippedNormalizedIndices = new Set(); + for (const entry of partitioned.subscriptionErrors) { + const norm = rawToNormalized.get(entry.index); + if (norm !== undefined) skippedNormalizedIndices.add(norm); + } + + const filteredDocument = + skippedNormalizedIndices.size === 0 + ? document + : { + ...document, + subscriptions: document.subscriptions.filter( + (_, idx) => !skippedNormalizedIndices.has(idx) + ), + }; + + const materialized = materializeCueConfig(filteredDocument); + + // Surface skipped subscriptions as warnings so the user sees what was + // excluded and can fix the YAML, but the rest of the config still loads. + const warnings = [...materialized.warnings]; + for (const entry of partitioned.subscriptionErrors) { + const detail = entry.errors.join('; '); + warnings.push(`Skipped invalid subscription at index ${entry.index} — ${detail}`); + } + + return { ok: true, config: materialized.config, warnings }; } /** diff --git a/src/main/ipc/handlers/cue.ts b/src/main/ipc/handlers/cue.ts index 83f5c2f23..6714712c8 100644 --- a/src/main/ipc/handlers/cue.ts +++ b/src/main/ipc/handlers/cue.ts @@ -19,10 +19,12 @@ import { validateCueConfig } from '../../cue/cue-yaml-loader'; import { deleteCueConfigFile, readCueConfigFile, + pruneOrphanedPromptFiles, writeCueConfigFile, writeCuePromptFile, } from '../../cue/config/cue-config-repository'; import { loadPipelineLayout, savePipelineLayout } from '../../cue/pipeline-layout-store'; +import { captureException } from '../../utils/sentry'; import type { CueEngine } from '../../cue/cue-engine'; import type { CueGraphSession, @@ -217,6 +219,7 @@ export function registerCueHandlers(deps: CueHandlerDependencies): void { content: string; promptFiles?: Record; }): Promise => { + const keepPaths = new Set(); if (options.promptFiles) { const promptsBase = path.resolve(options.projectRoot, '.maestro/prompts'); for (const [relativePath, content] of Object.entries(options.promptFiles)) { @@ -226,16 +229,69 @@ export function registerCueHandlers(deps: CueHandlerDependencies): void { ); } const target = path.resolve(options.projectRoot, relativePath); - if (!target.startsWith(promptsBase + path.sep) && target !== promptsBase) { + // Must resolve strictly INSIDE .maestro/prompts/. The earlier + // check allowed `target === promptsBase` which would attempt + // to write to the directory path itself. + if (!target.startsWith(promptsBase + path.sep)) { throw new Error( `cue:writeYaml: promptFiles key "${relativePath}" resolves outside the .maestro/prompts directory` ); } + // Must be a .md file. pruneOrphanedPromptFiles only deletes + // .md files, so accepting other extensions here would let + // non-markdown junk accumulate forever (it's never on the + // prune keep-set's enforcement path). + if (path.extname(target).toLowerCase() !== '.md') { + throw new Error(`cue:writeYaml: promptFiles key "${relativePath}" must end with .md`); + } writeCuePromptFile(options.projectRoot, relativePath, content); + keepPaths.add(relativePath); + } + } + + // Parse the YAML BEFORE writing so we can derive the full + // referenced-paths keep-set up front — and so a parse failure + // becomes a hard skip on pruning instead of a partial keep-set + // that could mass-delete prompt files referenced only inside + // options.content (and not duplicated in options.promptFiles). + let parseSucceeded = true; + try { + const parsed = yaml.load(options.content) as + | { subscriptions?: Array> } + | null + | undefined; + const subs = parsed?.subscriptions; + if (Array.isArray(subs)) { + for (const sub of subs) { + if (!sub || typeof sub !== 'object') continue; + const pf = (sub as Record).prompt_file; + const opf = (sub as Record).output_prompt_file; + if (typeof pf === 'string' && pf.length > 0 && !path.isAbsolute(pf)) { + keepPaths.add(pf); + } + if (typeof opf === 'string' && opf.length > 0 && !path.isAbsolute(opf)) { + keepPaths.add(opf); + } + } } + } catch (parseErr) { + parseSucceeded = false; + captureException(parseErr, { + operation: 'cue:writeYaml.parseForPrune', + projectRoot: options.projectRoot, + }); } writeCueConfigFile(options.projectRoot, options.content); + + // Only prune when we have an authoritative keep-set. If the YAML + // failed to parse, the keep-set may be missing prompt files the + // YAML actually references — running prune anyway risks + // mass-deleting files we'd lose forever. The next successful save + // (with valid YAML) will catch up. + if (parseSucceeded) { + pruneOrphanedPromptFiles(options.projectRoot, keepPaths); + } } ) ); diff --git a/src/renderer/components/CueModal/CueModal.tsx b/src/renderer/components/CueModal/CueModal.tsx index 9aa45c669..7607d1d25 100644 --- a/src/renderer/components/CueModal/CueModal.tsx +++ b/src/renderer/components/CueModal/CueModal.tsx @@ -33,6 +33,8 @@ import { ActivityLog } from './ActivityLog'; import { buildSubscriptionPipelineMap } from './cueModalUtils'; import { notifyToast } from '../../stores/notificationStore'; import { captureException } from '../../utils/sentry'; +import { cueService } from '../../services/cue'; +import { useCueDirtyStore } from '../../stores/cueDirtyStore'; type CueModalTab = 'dashboard' | 'pipeline'; @@ -130,7 +132,7 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { setShowHelp(false); return; } - if (pipelineDirtyRef.current) { + if (useCueDirtyStore.getState().pipelineDirty) { getModalActions().showConfirmation( 'You have unsaved changes in the pipeline editor. Discard and close?', () => onCloseRef.current() @@ -160,7 +162,7 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { useEffect(() => { let cancelled = false; setGraphError(null); - window.maestro.cue + cueService .getGraphData() .then((data: CueGraphSession[]) => { if (!cancelled) setGraphSessions(data); @@ -192,10 +194,12 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { const showHelpRef = useRef(false); showHelpRef.current = showHelp; - // Pipeline dirty state (unsaved changes) - const [pipelineDirty, setPipelineDirty] = useState(false); - const pipelineDirtyRef = useRef(false); - pipelineDirtyRef.current = pipelineDirty; + // Reset pipeline dirty state when the modal unmounts + useEffect(() => { + return () => { + useCueDirtyStore.getState().resetAll(); + }; + }, []); const handleEditYaml = useCallback((session: CueSessionStatus) => { getModalActions().openCueYamlEditor(session.sessionId, session.projectRoot); @@ -211,7 +215,7 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { `Remove Cue configuration for "${session.sessionName}"?\n\nThis will delete the cue.yaml file from this project. This cannot be undone.`, async () => { try { - await window.maestro.cue.deleteYaml(session.projectRoot); + await cueService.deleteYaml(session.projectRoot); } catch (err) { captureException(err, { extra: { context: 'handleRemoveCue', projectRoot: session.projectRoot }, @@ -243,7 +247,7 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { // Close with unsaved changes confirmation const handleCloseWithConfirm = useCallback(() => { - if (pipelineDirtyRef.current) { + if (useCueDirtyStore.getState().pipelineDirty) { getModalActions().showConfirmation( 'You have unsaved changes in the pipeline editor. Discard and close?', () => onClose() @@ -532,7 +536,6 @@ export function CueModal({ theme, onClose, cueShortcutKeys }: CueModalProps) { graphSessions={graphSessions} onSwitchToSession={handleSwitchToSession} onClose={onClose} - onDirtyChange={setPipelineDirty} theme={theme} activeRuns={activeRuns} onTriggerPipeline={triggerSubscription} diff --git a/src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx b/src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx index ca2617333..122bae713 100644 --- a/src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx +++ b/src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx @@ -11,8 +11,10 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { ReactFlowProvider, useReactFlow, + useNodesInitialized, applyNodeChanges, type Node, + type Edge, type NodeChange, type OnNodesChange, type OnEdgesChange, @@ -50,7 +52,6 @@ export interface CuePipelineEditorProps { graphSessions: CueGraphSession[]; onSwitchToSession: (id: string) => void; onClose: () => void; - onDirtyChange?: (isDirty: boolean) => void; theme: Theme; activeRuns?: ActiveRunInfo[]; /** Callback to manually trigger a pipeline by name */ @@ -72,7 +73,6 @@ function CuePipelineEditorInner({ groups, graphSessions, onSwitchToSession, - onDirtyChange, theme, activeRuns: activeRunsProp, onTriggerPipeline, @@ -93,7 +93,6 @@ function CuePipelineEditorInner({ sessions, graphSessions, activeRuns: activeRunsProp, - onDirtyChange, reactFlowInstance, selectedNodePipelineId: selectionRef.current.selectedNodePipelineId, selectedEdgePipelineId: selectionRef.current.selectedEdgePipelineId, @@ -129,6 +128,7 @@ function CuePipelineEditorInner({ setShowSettings, runningPipelineIds, persistLayout, + pendingSavedViewportRef, handleSave, handleDiscard, createPipeline, @@ -192,6 +192,18 @@ function CuePipelineEditorInner({ handleConfigureNode, } = selectionHook; + // The per-node "Configure" icon calls this directly via node data, bypassing + // onNodeClick. In All Pipelines view everything is read-only, so we refuse + // to open the edit panel. Declared here (before computedNodes) so the memo + // embeds the stable guarded callback. + const handleConfigureNodeGuarded = useCallback( + (compositeId: string) => { + if (isAllPipelinesView) return; + handleConfigureNode(compositeId); + }, + [isAllPipelinesView, handleConfigureNode] + ); + // ─── ReactFlow nodes/edges ─────────────────────────────────────────────── // Compute canonical nodes from pipeline state. This is the "source of truth" @@ -201,7 +213,7 @@ function CuePipelineEditorInner({ convertToReactFlowNodes( pipelineState.pipelines, pipelineState.selectedPipelineId, - handleConfigureNode, + handleConfigureNodeGuarded, { onTriggerPipeline, isSaved: !isDirty, @@ -213,7 +225,7 @@ function CuePipelineEditorInner({ [ pipelineState.pipelines, pipelineState.selectedPipelineId, - handleConfigureNode, + handleConfigureNodeGuarded, onTriggerPipeline, isDirty, runningPipelineIds, @@ -283,19 +295,35 @@ function CuePipelineEditorInner({ return () => clearTimeout(timer); }, [pipelineState.selectedPipelineId, reactFlowInstance]); - // ─── Initial fit view ──────────────────────────────────────────────────── - // With the always-on fitView prop removed from ReactFlow, we need to - // fit the viewport once after the first render with nodes. usePipelineLayout - // will overwrite this if it has a saved viewport. + // ─── Initial viewport (saved viewport OR fit view) ────────────────────── + // Two distinct paths with different timing requirements: + // + // - Saved viewport: a pure (x, y, zoom) restore. setViewport doesn't read + // node geometry, so it can — and SHOULD — fire immediately on mount. + // Waiting for nodesInitialized would briefly show the wrong viewport + // before snapping to the saved one. + // - fitView fallback: computes bounds from rendered node dimensions. Must + // wait for `useNodesInitialized()` — fitting before measurement + // completes produced the original "canvas appears empty on first open" + // symptom (selection-change fitView later "fixed" it because nodes + // were measured by then). + const nodesInitialized = useNodesInitialized(); const hasInitialFitRef = useRef(false); useEffect(() => { - if (hasInitialFitRef.current || computedNodes.length === 0) return; + if (hasInitialFitRef.current) return; + const saved = pendingSavedViewportRef.current; + if (saved) { + // Restore immediately — setViewport doesn't depend on measurement. + pendingSavedViewportRef.current = null; + reactFlowInstance.setViewport(saved); + hasInitialFitRef.current = true; + return; + } + // fitView path: must wait for nodes to be measured. + if (!nodesInitialized || computedNodes.length === 0) return; + reactFlowInstance.fitView({ padding: 0.15, duration: 200 }); hasInitialFitRef.current = true; - const timer = setTimeout(() => { - reactFlowInstance.fitView({ padding: 0.15, duration: 200 }); - }, 150); - return () => clearTimeout(timer); - }, [computedNodes.length, reactFlowInstance]); + }, [nodesInitialized, computedNodes.length, reactFlowInstance, pendingSavedViewportRef]); // ─── Canvas callbacks ──────────────────────────────────────────────────── @@ -316,8 +344,13 @@ function CuePipelineEditorInner({ // Commit final positions to canonical pipelineState when drag ends. // ReactFlow's onNodeDragStop reliably provides the final node with its // position, unlike onNodesChange which may omit position on drag end. + // + // In All Pipelines view everything is locked in place — even if ReactFlow + // somehow fired a drag-stop (shouldn't, since `nodesDraggable={false}`), + // we refuse to mutate canonical state. const onNodeDragStop = useCallback( (_event: React.MouseEvent, _node: Node, draggedNodes: Node[]) => { + if (isAllPipelinesView) return; if (draggedNodes.length === 0) return; setPipelineState((prev) => { @@ -349,13 +382,14 @@ function CuePipelineEditorInner({ persistLayout(); }, - [persistLayout, setPipelineState] + [isAllPipelinesView, persistLayout, setPipelineState] ); const onEdgesChange: OnEdgesChange = useCallback(() => {}, []); const onConnect = useCallback( (connection: Connection) => { + if (isAllPipelinesView) return; if (!connection.source || !connection.target) return; const sourcePipelineId = connection.source.split(':')[0]; @@ -412,11 +446,12 @@ function CuePipelineEditorInner({ }; }); }, - [setPipelineState] + [isAllPipelinesView, setPipelineState] ); const isValidConnection = useCallback( (connection: Connection) => { + if (isAllPipelinesView) return false; if (!connection.source || !connection.target) return false; if (connection.source === connection.target) return false; @@ -434,7 +469,7 @@ function CuePipelineEditorInner({ return true; }, - [nodes, edges] + [isAllPipelinesView, nodes, edges] ); const onDragOver = useCallback((event: React.DragEvent) => { @@ -448,6 +483,12 @@ function CuePipelineEditorInner({ event.preventDefault(); event.stopPropagation(); + // All Pipelines view is read-only — refuse to place new nodes. + // The toolbar disables the drawer buttons in this view, but a drag + // from an already-open drawer (possible if the view changed mid-drag) + // must still be rejected here. + if (isAllPipelinesView) return; + const raw = event.dataTransfer.getData('application/cue-pipeline'); if (!raw) return; @@ -550,7 +591,7 @@ function CuePipelineEditorInner({ }; }); }, - [reactFlowInstance, setPipelineState, setSelectedNodeId, setSelectedEdgeId] + [isAllPipelinesView, reactFlowInstance, setPipelineState, setSelectedNodeId, setSelectedEdgeId] ); // ─── Keyboard shortcuts ────────────────────────────────────────────────── @@ -563,6 +604,9 @@ function CuePipelineEditorInner({ if (e.key === 'Delete' || e.key === 'Backspace') { if (isInput) return; + // All Pipelines view is read-only — no deletions. + // (Save via Cmd+S and Escape-to-deselect remain available.) + if (isAllPipelinesView) return; if (selectedNode && selectedNodePipelineId) { e.preventDefault(); onDeleteNode(selectedNode.id); @@ -588,6 +632,7 @@ function CuePipelineEditorInner({ window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); }, [ + isAllPipelinesView, selectedNode, selectedNodePipelineId, selectedEdge, @@ -605,30 +650,49 @@ function CuePipelineEditorInner({ // ─── Context menu handlers ─────────────────────────────────────────────── - const onNodeContextMenu = useCallback((event: React.MouseEvent, node: Node) => { - event.preventDefault(); - const sepIdx = node.id.indexOf(':'); - if (sepIdx === -1) return; - const pipelineId = node.id.substring(0, sepIdx); - const nodeId = node.id.substring(sepIdx + 1); - setContextMenu({ - x: event.clientX, - y: event.clientY, - nodeId, - pipelineId, - nodeType: node.type as 'trigger' | 'agent', - }); - }, []); + // In All Pipelines view, right-clicking a node does nothing — the context + // menu's actions (Configure/Delete/Duplicate) are all editing operations. + const onNodeContextMenu = useCallback( + (event: React.MouseEvent, node: Node) => { + event.preventDefault(); + if (isAllPipelinesView) return; + const sepIdx = node.id.indexOf(':'); + if (sepIdx === -1) return; + const pipelineId = node.id.substring(0, sepIdx); + const nodeId = node.id.substring(sepIdx + 1); + setContextMenu({ + x: event.clientX, + y: event.clientY, + nodeId, + pipelineId, + nodeType: node.type as 'trigger' | 'agent', + }); + }, + [isAllPipelinesView] + ); + // All three handlers re-check isAllPipelinesView even though onNodeContextMenu + // also blocks open: a context menu opened in the per-pipeline view stays + // rendered if the user switches to All Pipelines mode while it's open, and + // without the guard the still-clickable Configure/Delete/Duplicate items + // would mutate state that isn't editable in the All Pipelines view. const handleContextMenuConfigure = useCallback(() => { if (!contextMenu) return; + if (isAllPipelinesView) { + setContextMenu(null); + return; + } setSelectedNodeId(`${contextMenu.pipelineId}:${contextMenu.nodeId}`); setSelectedEdgeId(null); setContextMenu(null); - }, [contextMenu, setSelectedNodeId, setSelectedEdgeId]); + }, [contextMenu, isAllPipelinesView, setSelectedNodeId, setSelectedEdgeId]); const handleContextMenuDelete = useCallback(() => { if (!contextMenu) return; + if (isAllPipelinesView) { + setContextMenu(null); + return; + } setPipelineState((prev) => ({ ...prev, pipelines: prev.pipelines.map((p) => { @@ -644,10 +708,14 @@ function CuePipelineEditorInner({ })); setSelectedNodeId(null); setContextMenu(null); - }, [contextMenu, setPipelineState, setSelectedNodeId]); + }, [contextMenu, isAllPipelinesView, setPipelineState, setSelectedNodeId]); const handleContextMenuDuplicate = useCallback(() => { if (!contextMenu || contextMenu.nodeType !== 'trigger') return; + if (isAllPipelinesView) { + setContextMenu(null); + return; + } setPipelineState((prev) => { const pipeline = prev.pipelines.find((p) => p.id === contextMenu.pipelineId); if (!pipeline) return prev; @@ -668,7 +736,28 @@ function CuePipelineEditorInner({ }; }); setContextMenu(null); - }, [contextMenu, setPipelineState]); + }, [contextMenu, isAllPipelinesView, setPipelineState]); + + // ─── Read-only click wrappers for All Pipelines view ─────────────────── + // Clicking a node/edge normally sets selection, which opens the node or + // edge config panel with editable fields. In All Pipelines view nothing + // is editable, so short-circuit selection at the source. Any pre-existing + // selection from before the view switch is additionally guarded at panel + // render time in PipelineCanvas. + const onNodeClickGuarded = useCallback( + (event: React.MouseEvent, node: Node) => { + if (isAllPipelinesView) return; + onNodeClick(event, node); + }, + [isAllPipelinesView, onNodeClick] + ); + const onEdgeClickGuarded = useCallback( + (event: React.MouseEvent, edge: Edge) => { + if (isAllPipelinesView) return; + onEdgeClick(event, edge); + }, + [isAllPipelinesView, onEdgeClick] + ); // ─── Render ────────────────────────────────────────────────────────────── @@ -701,12 +790,13 @@ function CuePipelineEditorInner({ theme={theme} nodes={nodes} edges={edges} + isReadOnly={isAllPipelinesView} onNodesChange={onNodesChange} onEdgesChange={onEdgesChange} onConnect={onConnect} isValidConnection={isValidConnection} - onNodeClick={onNodeClick} - onEdgeClick={onEdgeClick} + onNodeClick={onNodeClickGuarded} + onEdgeClick={onEdgeClickGuarded} onPaneClick={onPaneClick} onNodeContextMenu={onNodeContextMenu} onNodeDragStop={onNodeDragStop} diff --git a/src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx b/src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx index b35c39264..8abe1dcab 100644 --- a/src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx +++ b/src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx @@ -46,6 +46,14 @@ const nodeTypes = { export interface PipelineCanvasProps { theme: Theme; + /** + * When true (All Pipelines view), the canvas is fully read-only: + * nodes can't be dragged, connected, selected, or edited; no config + * panels render even if a selection is already set. The parent also + * guards each edit callback, so this is defense-in-depth at the + * ReactFlow interaction layer. + */ + isReadOnly?: boolean; // ReactFlow nodes: Node[]; edges: Edge[]; @@ -109,6 +117,7 @@ export interface PipelineCanvasProps { export const PipelineCanvas = React.memo(function PipelineCanvas({ theme, + isReadOnly = false, nodes, edges, onNodesChange, @@ -251,6 +260,11 @@ export const PipelineCanvas = React.memo(function PipelineCanvas({ onDragOver={onDragOver} onDrop={onDrop} connectionMode={ConnectionMode.Loose} + // All Pipelines view is read-only. These ReactFlow props are the + // first line of defense — the parent also guards each callback. + nodesDraggable={!isReadOnly} + nodesConnectable={!isReadOnly} + elementsSelectable={!isReadOnly} style={{ backgroundColor: theme.colors.bgMain, }} @@ -365,8 +379,11 @@ export const PipelineCanvas = React.memo(function PipelineCanvas({ /> )} - {/* Config panels */} - {selectedNode && + {/* Config panels — suppressed in read-only (All Pipelines) view so + any selection carried over from a previous single-pipeline view + does not expose editable fields. */} + {!isReadOnly && + selectedNode && !selectedEdge && (() => { const selectedPipeline = pipelines.find((pl) => @@ -394,7 +411,7 @@ export const PipelineCanvas = React.memo(function PipelineCanvas({ /> ); })()} - {selectedEdge && !selectedNode && ( + {!isReadOnly && selectedEdge && !selectedNode && ( p.id === saved)) { + resolvedSelected = saved; + } else { + resolvedSelected = mergedPipelines[0]?.id ?? null; + } + } else { + resolvedSelected = mergedPipelines[0]?.id ?? null; + } + return { pipelines: mergedPipelines, - selectedPipelineId: - 'selectedPipelineId' in savedLayout - ? savedLayout.selectedPipelineId - : (mergedPipelines[0]?.id ?? null), + selectedPipelineId: resolvedSelected, }; } diff --git a/src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts b/src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts index e2c1b3d56..ad641549a 100644 --- a/src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts +++ b/src/renderer/components/CuePipelineEditor/utils/yamlToPipeline.ts @@ -448,20 +448,14 @@ function findTargetSession( allSubs: CueSubscription[], sessions: PipelineSessionInfo[] ): string | null { - // If the subscription has an explicit agent_id, resolve by session ID directly. + // If the subscription has an explicit agent_id, trust it — the editor writes + // agent_id whenever the user binds a subscription to a specific session, and + // per-project-root YAML partitioning guarantees it is never a cross-session leak. if (sub.agent_id) { const session = sessions.find((s) => s.id === sub.agent_id); - if (session) { - // Cross-check: if subscription base name matches a DIFFERENT session, - // prefer the name match. agent_id may be stale from a previous bad - // resolution (e.g., pre-agent_id YAML that got locked to the wrong session). - const baseName = getBasePipelineName(sub.name); - const nameMatch = sessions.find((s) => s.name === baseName); - if (nameMatch && nameMatch.name !== session.name) { - return nameMatch.name; - } - return session.name; - } + if (session) return session.name; + // agent_id references a session that no longer exists — fall through + // to owner / name-based resolution below. } // If the subscription was tagged with owning sessions from getGraphData(), diff --git a/src/renderer/components/CueYamlEditor/CueYamlEditor.tsx b/src/renderer/components/CueYamlEditor/CueYamlEditor.tsx index 8837b6bb6..f2f3d7bc7 100644 --- a/src/renderer/components/CueYamlEditor/CueYamlEditor.tsx +++ b/src/renderer/components/CueYamlEditor/CueYamlEditor.tsx @@ -20,6 +20,8 @@ import { PatternPicker } from './PatternPicker'; import { PatternPreviewModal } from './PatternPreviewModal'; import { CueAiChat } from './CueAiChat'; import { YamlTextEditor } from './YamlTextEditor'; +import { cueService } from '../../services/cue'; +import { captureException } from '../../utils/sentry'; interface CueYamlEditorProps { isOpen: boolean; @@ -59,22 +61,37 @@ export function CueYamlEditor({ async function loadYaml() { setLoading(true); try { - const content = await window.maestro.cue.readYaml(projectRoot); + const content = await cueService.readYaml(projectRoot); if (cancelled) return; const initial = content ?? CUE_YAML_TEMPLATE; setYamlContent(initial); setOriginalContent(initial); try { - const validationResult = await window.maestro.cue.validateYaml(initial); + const validationResult = await cueService.validateYaml(initial); if (!cancelled) { setIsValid(validationResult.valid); setValidationErrors(validationResult.errors); } - } catch { - // Validation failure on load is non-fatal + } catch (err: unknown) { + // Gate the Save button when validation fails to actually run — + // otherwise isValid keeps its initial `true` and the user + // could save unvalidated YAML. Surface the error to telemetry + // AND to the user via setValidationErrors so they know what + // happened. Not re-thrown: the outer catch handles readYaml + // failures, not validateYaml; consolidating recovery here. + captureException(err, { + extra: { operation: 'cueYamlEditor.loadValidate', projectRoot }, + }); + if (!cancelled) { + setIsValid(false); + setValidationErrors([ + `Failed to validate YAML: ${err instanceof Error ? err.message : String(err)}`, + ]); + } } - } catch { + } catch (err: unknown) { if (cancelled) return; + captureException(err, { extra: { operation: 'cueYamlEditor.loadRead', projectRoot } }); setYamlContent(CUE_YAML_TEMPLATE); setOriginalContent(CUE_YAML_TEMPLATE); } finally { @@ -95,7 +112,7 @@ export function CueYamlEditor({ } validateTimerRef.current = setTimeout(async () => { try { - const result = await window.maestro.cue.validateYaml(content); + const result = await cueService.validateYaml(content); setIsValid(result.valid); setValidationErrors(result.errors); } catch { @@ -124,8 +141,8 @@ export function CueYamlEditor({ const handleSave = useCallback(async () => { if (!isValid) return; - await window.maestro.cue.writeYaml(projectRoot, yamlContent); - await window.maestro.cue.refreshSession(sessionId, projectRoot); + await cueService.writeYaml(projectRoot, yamlContent); + await cueService.refreshSession(sessionId, projectRoot); onClose(); }, [isValid, projectRoot, yamlContent, sessionId, onClose]); @@ -134,20 +151,33 @@ export function CueYamlEditor({ const refreshYamlFromDisk = useCallback(async () => { try { - const content = await window.maestro.cue.readYaml(projectRoot); - if (content) { + const content = await cueService.readYaml(projectRoot); + // `if (content)` would skip an intentionally empty YAML — an empty + // string is a legitimate result (e.g. user cleared the file) and + // should still trigger state updates and revalidation. Only skip + // when the read returned null (no file on disk). + if (content != null) { setYamlContent(content); setOriginalContent(content); try { - const result = await window.maestro.cue.validateYaml(content); + const result = await cueService.validateYaml(content); setIsValid(result.valid); setValidationErrors(result.errors); - } catch { - // non-fatal + } catch (err: unknown) { + // Same gating as loadValidate: don't leave isValid=true after + // a validation failure or the user could save unvalidated + // content from a refresh. + captureException(err, { + extra: { operation: 'cueYamlEditor.refreshValidate', projectRoot }, + }); + setIsValid(false); + setValidationErrors([ + `Failed to validate YAML: ${err instanceof Error ? err.message : String(err)}`, + ]); } } - } catch { - // non-fatal + } catch (err: unknown) { + captureException(err, { extra: { operation: 'cueYamlEditor.refreshRead', projectRoot } }); } }, [projectRoot]); diff --git a/src/renderer/components/SessionList/SessionList.tsx b/src/renderer/components/SessionList/SessionList.tsx index a9bd1fa30..895d56eab 100644 --- a/src/renderer/components/SessionList/SessionList.tsx +++ b/src/renderer/components/SessionList/SessionList.tsx @@ -36,6 +36,8 @@ import { SkinnySidebar } from './SkinnySidebar'; import { LiveOverlayPanel } from './LiveOverlayPanel'; import { useSessionCategories } from '../../hooks/session/useSessionCategories'; import { useSessionFilterMode } from '../../hooks/session/useSessionFilterMode'; +import { cueService } from '../../services/cue'; +import { captureException } from '../../utils/sentry'; // ============================================================================ // SessionContextMenu - Right-click context menu for session items @@ -145,7 +147,7 @@ function SessionListInner(props: SessionListProps) { const fetchCueStatus = async () => { try { - const statuses = await window.maestro.cue.getStatus(); + const statuses = await cueService.getStatus(); if (!mounted) return; const map = new Map(); for (const s of statuses) { @@ -157,13 +159,20 @@ function SessionListInner(props: SessionListProps) { } } setCueSessionMap(map); - } catch { - // Cue engine may not be initialized yet + } catch (err: unknown) { + // "Cue engine not initialized" is the expected pre-init case; + // treat anything else as a real failure and surface it. Note + // that cueService.getStatus already swallows IPC failures and + // returns the default ([]), so this catch is a defense-in-depth + // backstop for engine-not-ready and any future contract change. + const message = err instanceof Error ? err.message : String(err); + if (message.includes('Cue engine not initialized')) return; + captureException(err, { extra: { context: 'SessionList.fetchCueStatus' } }); } }; fetchCueStatus(); - const unsubscribe = window.maestro.cue.onActivityUpdate(() => { + const unsubscribe = cueService.onActivityUpdate(() => { fetchCueStatus(); }); diff --git a/src/renderer/hooks/cue/usePipelineLayout.ts b/src/renderer/hooks/cue/usePipelineLayout.ts index 960ab1f30..4b5577c60 100644 --- a/src/renderer/hooks/cue/usePipelineLayout.ts +++ b/src/renderer/hooks/cue/usePipelineLayout.ts @@ -6,8 +6,9 @@ */ import { useCallback, useEffect, useRef } from 'react'; -import type { ReactFlowInstance } from 'reactflow'; +import type { ReactFlowInstance, Viewport } from 'reactflow'; import type { + AgentNodeData, CuePipelineState, CueGraphSession, PipelineLayoutState, @@ -15,6 +16,7 @@ import type { import { graphSessionsToPipelines } from '../../components/CuePipelineEditor/utils/yamlToPipeline'; import { mergePipelinesWithSavedLayout } from '../../components/CuePipelineEditor/utils/pipelineLayout'; import { captureException } from '../../utils/sentry'; +import { cueService } from '../../services/cue'; import type { CuePipelineSessionInfo as SessionInfo } from '../../../shared/cue-pipeline-types'; @@ -25,11 +27,31 @@ export interface UsePipelineLayoutParams { pipelineState: CuePipelineState; setPipelineState: React.Dispatch>; savedStateRef: React.MutableRefObject; + /** + * Set of project roots that the current saved state corresponds to. Seeded + * from the initial loaded pipelines so handleSave knows which roots to + * clear if their last pipeline disappears, even when the agent that owned + * those pipelines was renamed/removed since the load. + */ + lastWrittenRootsRef: React.MutableRefObject>; setIsDirty: React.Dispatch>; } export interface UsePipelineLayoutReturn { persistLayout: () => void; + /** + * Pending saved viewport from disk, captured during initial restore. + * CuePipelineEditor reads this once nodes have been measured and either + * applies it via `setViewport` or falls back to `fitView`. Consumed (set + * back to null) after the first read to prevent re-application. + * + * Owning the viewport-apply step in the component (rather than scheduling + * `setViewport` on a timeout here) eliminates the race against ReactFlow's + * node measurement — the previous implementation could set or fit the + * viewport before nodes were measured, leaving the canvas appearing empty + * on first open. + */ + pendingSavedViewportRef: React.MutableRefObject; } export function usePipelineLayout({ @@ -39,17 +61,19 @@ export function usePipelineLayout({ pipelineState, setPipelineState, savedStateRef, + lastWrittenRootsRef, setIsDirty, }: UsePipelineLayoutParams): UsePipelineLayoutReturn { const layoutSaveTimerRef = useRef | null>(null); const hasRestoredLayoutRef = useRef(false); const latestRestoreIdRef = useRef(0); + const pendingSavedViewportRef = useRef(null); // Keep a ref to current pipeline state for layout persistence (avoids unstable callback) const pipelineStateRef = useRef(pipelineState); pipelineStateRef.current = pipelineState; - // Debounced layout persistence (positions + viewport) + // Debounced layout persistence (positions + viewport + written roots) const persistLayout = useCallback(() => { if (layoutSaveTimerRef.current) clearTimeout(layoutSaveTimerRef.current); layoutSaveTimerRef.current = setTimeout(() => { @@ -59,14 +83,19 @@ export function usePipelineLayout({ pipelines: state.pipelines, selectedPipelineId: state.selectedPipelineId, viewport, + // Persist the written-roots snapshot so the next mount can + // reseed lastWrittenRootsRef even if the originating agent has + // been renamed/removed (sessionId/Name lookup would miss the + // root in that case, leaving stale YAML uncleared). + writtenRoots: [...lastWrittenRootsRef.current], }; - window.maestro.cue + cueService .savePipelineLayout(layout as unknown as Record) .catch((err: unknown) => { captureException(err, { extra: { operation: 'savePipelineLayout' } }); }); }, 500); - }, [reactFlowInstance]); + }, [reactFlowInstance, lastWrittenRootsRef]); // Clean up debounce timer on unmount useEffect(() => { @@ -75,6 +104,39 @@ export function usePipelineLayout({ }; }, []); + // Reseed lastWrittenRootsRef from the persisted writtenRoots set as early + // as possible — independent of graphSessions / livePipelines availability. + // The main load-layout effect below ALSO rebuilds the ref, but it's gated + // on graphSessions being non-empty; without this early seed, opening the + // editor with no live sessions (engine disabled, no registered sessions) + // would miss orphan-root metadata for the very first save. + useEffect(() => { + let cancelled = false; + const loadWrittenRoots = async () => { + let savedLayout: PipelineLayoutState | null = null; + try { + savedLayout = (await cueService.loadPipelineLayout()) as PipelineLayoutState | null; + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + if (!message.includes('no saved layout') && !message.includes('ENOENT')) { + captureException(err, { extra: { operation: 'loadPipelineLayout.writtenRoots' } }); + } + return; + } + if (cancelled) return; + if (!savedLayout?.writtenRoots || !Array.isArray(savedLayout.writtenRoots)) return; + for (const root of savedLayout.writtenRoots) { + if (typeof root === 'string' && root.length > 0) { + lastWrittenRootsRef.current.add(root); + } + } + }; + loadWrittenRoots(); + return () => { + cancelled = true; + }; + }, [lastWrittenRootsRef]); + // Load pipelines once on mount from saved layout merged with live graph data. // The pipeline editor is the primary editor — we don't re-sync from disk // while the user is working. Save writes back to disk. @@ -93,7 +155,7 @@ export function usePipelineLayout({ let savedLayout: PipelineLayoutState | null = null; try { - savedLayout = (await window.maestro.cue.loadPipelineLayout()) as PipelineLayoutState | null; + savedLayout = (await cueService.loadPipelineLayout()) as PipelineLayoutState | null; } catch (err: unknown) { // loadPipelineLayout may fail if no layout has been saved yet — that's expected. // Report anything else to Sentry. @@ -106,25 +168,57 @@ export function usePipelineLayout({ // Guard: if a newer load started or a previous one already completed, bail out if (reqId !== latestRestoreIdRef.current || hasRestoredLayoutRef.current) return; + let pipelinesForRoots: CuePipelineState['pipelines']; if (savedLayout && savedLayout.pipelines) { const merged = mergePipelinesWithSavedLayout(livePipelines, savedLayout); setPipelineState(merged); savedStateRef.current = JSON.stringify(merged.pipelines); + pipelinesForRoots = merged.pipelines; - // Restore viewport if available + // Stash the saved viewport for the editor to apply once ReactFlow + // has measured the restored nodes. Applying it here on a timeout + // raced against `fitView` and — more importantly — against node + // measurement, which caused the initial canvas to appear empty. if (savedLayout.viewport) { - const viewportToRestore = savedLayout.viewport; - setTimeout(() => { - if (reqId === latestRestoreIdRef.current) { - reactFlowInstance.setViewport(viewportToRestore); - } - }, 100); + pendingSavedViewportRef.current = savedLayout.viewport; } } else { setPipelineState({ pipelines: livePipelines, selectedPipelineId: livePipelines[0].id }); savedStateRef.current = JSON.stringify(livePipelines); + pipelinesForRoots = livePipelines; + } + + // Seed lastWrittenRootsRef from two sources, unioned: + // 1. The persisted writtenRoots set from the previous save — + // authoritative even when the originating agent has since been + // renamed or deleted (the session lookup below would miss it + // in that case, leaving stale YAML at that root uncleared). + // 2. Session-resolved roots from the just-loaded pipelines — + // catches any roots that aren't in writtenRoots yet (e.g. + // first-ever editor open with pre-existing pipelines, or + // writtenRoots was cleared/missing on disk). + const loadedRoots = new Set(); + if (savedLayout?.writtenRoots && Array.isArray(savedLayout.writtenRoots)) { + for (const root of savedLayout.writtenRoots) { + if (typeof root === 'string' && root.length > 0) { + loadedRoots.add(root); + } + } + } + const sessionsById = new Map(sessions.map((s) => [s.id, s])); + const sessionsByName = new Map(sessions.map((s) => [s.name, s])); + for (const pipeline of pipelinesForRoots) { + for (const node of pipeline.nodes) { + if (node.type !== 'agent') continue; + const data = node.data as AgentNodeData; + const root = + sessionsById.get(data.sessionId)?.projectRoot ?? + sessionsByName.get(data.sessionName)?.projectRoot; + if (root) loadedRoots.add(root); + } } + lastWrittenRootsRef.current = loadedRoots; hasRestoredLayoutRef.current = true; setIsDirty(false); @@ -133,5 +227,5 @@ export function usePipelineLayout({ loadLayout(); }, [graphSessions, sessions]); - return { persistLayout }; + return { persistLayout, pendingSavedViewportRef }; } diff --git a/src/renderer/hooks/cue/usePipelineState.ts b/src/renderer/hooks/cue/usePipelineState.ts index 29d9ca9d8..3d73209d8 100644 --- a/src/renderer/hooks/cue/usePipelineState.ts +++ b/src/renderer/hooks/cue/usePipelineState.ts @@ -7,12 +7,13 @@ */ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; -import type { ReactFlowInstance } from 'reactflow'; +import type { ReactFlowInstance, Viewport } from 'reactflow'; import type { CuePipelineState, CuePipeline, CueGraphSession, PipelineEdge as PipelineEdgeType, + PipelineNode, TriggerNodeData, AgentNodeData, CueEventType, @@ -25,6 +26,9 @@ import { DEFAULT_CUE_SETTINGS } from '../../../shared/cue'; import { usePipelineLayout } from './usePipelineLayout'; import { captureException } from '../../utils/sentry'; import { getModalActions } from '../../stores/modalStore'; +import { cueService } from '../../services/cue'; +import { useCueDirtyStore } from '../../stores/cueDirtyStore'; +import { notifyToast } from '../../stores/notificationStore'; // ─── Shared types ──────────────────────────────────────────────────────────── @@ -50,6 +54,64 @@ export const DEFAULT_TRIGGER_LABELS: Record = { 'cli.trigger': 'CLI Trigger', }; +/** + * Validate trigger node config against the YAML schema's per-event + * requirements. Catches misconfigured triggers (e.g. a `time.scheduled` + * trigger with no `schedule_times`) at SAVE time so they never hit disk — + * otherwise the YAML loader rejects the whole file on next launch and + * blocks valid pipelines belonging to other agents in the same project. + */ +function validateTriggerConfig( + pipelineName: string, + trigger: PipelineNode, + errors: string[] +): void { + const data = trigger.data as TriggerNodeData; + const cfg = data.config ?? {}; + const label = data.customLabel ? `"${data.customLabel}"` : `${data.eventType}`; + switch (data.eventType) { + case 'time.heartbeat': + if ( + typeof cfg.interval_minutes !== 'number' || + !Number.isFinite(cfg.interval_minutes) || + cfg.interval_minutes <= 0 + ) { + errors.push(`"${pipelineName}": ${label} trigger needs a positive interval (minutes)`); + } + break; + case 'time.scheduled': + if (!Array.isArray(cfg.schedule_times) || cfg.schedule_times.length === 0) { + errors.push( + `"${pipelineName}": ${label} trigger needs at least one schedule time (e.g. 09:00)` + ); + } + break; + case 'file.changed': + if (!cfg.watch || (typeof cfg.watch === 'string' && cfg.watch.trim().length === 0)) { + errors.push(`"${pipelineName}": ${label} trigger needs a "watch" glob pattern`); + } + break; + case 'task.pending': + if (!cfg.watch || (typeof cfg.watch === 'string' && cfg.watch.trim().length === 0)) { + errors.push(`"${pipelineName}": ${label} trigger needs a "watch" glob pattern`); + } + break; + case 'github.pull_request': + case 'github.issue': + // repo is optional in the YAML schema (defaults to current repo via gh CLI) + // but if provided it must be non-empty. + if ( + cfg.repo !== undefined && + (typeof cfg.repo !== 'string' || cfg.repo.trim().length === 0) + ) { + errors.push( + `"${pipelineName}": ${label} trigger has an empty "repo" — leave blank or set "owner/repo"` + ); + } + break; + } +} + /** Validates pipeline graph before save. Returns array of error messages. */ export function validatePipelines(pipelines: CuePipeline[]): string[] { const errors: string[] = []; @@ -58,7 +120,13 @@ export function validatePipelines(pipelines: CuePipeline[]): string[] { const triggers = pipeline.nodes.filter((n) => n.type === 'trigger'); const agents = pipeline.nodes.filter((n) => n.type === 'agent'); - if (triggers.length === 0 && agents.length === 0) continue; // Empty pipeline, skip + // Completely empty pipelines cannot be persisted (no subscriptions in YAML). + // Silent-skipping them here led to saves that appeared to succeed but + // wrote nothing to disk — flag them so the user gets clear feedback. + if (triggers.length === 0 && agents.length === 0) { + errors.push(`"${pipeline.name}": add a trigger and an agent before saving`); + continue; + } if (triggers.length === 0) { errors.push(`"${pipeline.name}": needs at least one trigger`); @@ -67,6 +135,10 @@ export function validatePipelines(pipelines: CuePipeline[]): string[] { errors.push(`"${pipeline.name}": needs at least one agent`); } + for (const trigger of triggers) { + validateTriggerConfig(pipeline.name, trigger, errors); + } + // Check for disconnected agents (no incoming edge) const targetsWithIncoming = new Set(pipeline.edges.map((e) => e.target)); for (const agent of agents) { @@ -141,7 +213,6 @@ export interface UsePipelineStateParams { sessions: SessionInfo[]; graphSessions: CueGraphSession[]; activeRuns?: ActiveRunInfo[]; - onDirtyChange?: (isDirty: boolean) => void; reactFlowInstance: ReactFlowInstance; // From usePipelineSelection (wired by shell): selectedNodePipelineId: string | null; @@ -168,6 +239,8 @@ export interface UsePipelineStateReturn { setShowSettings: React.Dispatch>; runningPipelineIds: Set; persistLayout: () => void; + /** Saved viewport awaiting application once ReactFlow has measured nodes. */ + pendingSavedViewportRef: React.MutableRefObject; handleSave: () => Promise; handleDiscard: () => Promise; createPipeline: () => void; @@ -188,7 +261,6 @@ export function usePipelineState({ sessions, graphSessions, activeRuns, - onDirtyChange, reactFlowInstance, selectedNodePipelineId, selectedEdgePipelineId, @@ -210,24 +282,33 @@ export function usePipelineState({ const [validationErrors, setValidationErrors] = useState([]); const savedStateRef = useRef(''); + // Project roots that the most recent successful save (or initial load) wrote + // to. Used by handleSave to know which roots may need an empty-YAML clear + // when they drop out of the current set. Avoids re-deriving roots from the + // JSON snapshot in savedStateRef — that re-derivation fails when an agent + // has been renamed or removed since the previous save (sessionId/Name no + // longer resolve to a projectRoot, so the stale root would never be cleared). + const lastWrittenRootsRef = useRef>(new Set()); + // Cue global settings const [cueSettings, setCueSettings] = useState({ ...DEFAULT_CUE_SETTINGS }); const [showSettings, setShowSettings] = useState(false); // Layout persistence (composed hook) - const { persistLayout } = usePipelineLayout({ + const { persistLayout, pendingSavedViewportRef } = usePipelineLayout({ reactFlowInstance, graphSessions, sessions, pipelineState, setPipelineState, savedStateRef, + lastWrittenRootsRef, setIsDirty, }); // Load global Cue settings from engine useEffect(() => { - window.maestro.cue + cueService .getSettings() .then((settings) => setCueSettings(settings)) .catch((err: unknown) => { @@ -244,102 +325,254 @@ export function usePipelineState({ } }, [pipelineState.pipelines]); - // Notify parent of dirty state changes + // Push dirty state into the shared store so CueModal can read it without prop-drilling + useEffect(() => { + useCueDirtyStore.getState().setPipelineDirty(isDirty); + }, [isDirty]); + + // Safety net: if `selectedPipelineId` ever points at a pipeline that no + // longer exists in `pipelines`, reset to "All Pipelines" so the canvas + // stays populated. This was the user-visible "pipeline vanished after + // save" symptom — `convertToReactFlowNodes` skips every pipeline whose id + // doesn't match the selected id, so a stale selection caused the entire + // canvas to render empty until the editor was remounted (tab switch). useEffect(() => { - onDirtyChange?.(isDirty); - }, [isDirty, onDirtyChange]); + const sel = pipelineState.selectedPipelineId; + if (sel === null) return; + if (pipelineState.pipelines.length === 0) return; + if (pipelineState.pipelines.some((p) => p.id === sel)) return; + setPipelineState((prev) => ({ ...prev, selectedPipelineId: null })); + }, [pipelineState.pipelines, pipelineState.selectedPipelineId]); const handleSave = useCallback(async () => { - // Validate before save + // Validate graph shape first const errors = validatePipelines(pipelineState.pipelines); - // Find unique project roots from sessions involved in pipelines - const sessionNames = new Set(); + // Build session lookup maps. Prefer sessionId since agents can be + // renamed, but fall back to sessionName for pipelines loaded from older + // YAML that referenced agents purely by name. + const sessionsById = new Map(); + const sessionsByName = new Map(); + for (const s of sessions) { + sessionsById.set(s.id, s); + if (!sessionsByName.has(s.name)) sessionsByName.set(s.name, s); + } + + const resolveRoot = (agent: AgentNodeData): string | null => { + const byId = sessionsById.get(agent.sessionId); + if (byId?.projectRoot) return byId.projectRoot; + const byName = sessionsByName.get(agent.sessionName); + if (byName?.projectRoot) return byName.projectRoot; + return null; + }; + + // Partition pipelines by project root. A pipeline must live in exactly + // one root — cross-root pipelines are rejected so each .maestro/cue.yaml + // remains the sole owner of its pipelines (prevents the historical + // mirroring / deleted-pipeline-reappears class of bugs). + const pipelinesByRoot = new Map(); + const unresolvedPipelines: string[] = []; + for (const pipeline of pipelineState.pipelines) { - for (const node of pipeline.nodes) { - if (node.type === 'agent') { - sessionNames.add((node.data as AgentNodeData).sessionName); + const agents = pipeline.nodes.filter((n) => n.type === 'agent'); + if (agents.length === 0) continue; // validatePipelines already flagged this + + const roots = new Set(); + let missingRoot = false; + for (const agent of agents) { + const root = resolveRoot(agent.data as AgentNodeData); + if (!root) { + missingRoot = true; + continue; } + roots.add(root); } - } - const projectRoots = new Set(); - for (const session of sessions) { - if (session.projectRoot && sessionNames.has(session.name)) { - projectRoots.add(session.projectRoot); + if (roots.size === 0) { + unresolvedPipelines.push(pipeline.name); + continue; + } + if (roots.size > 1) { + errors.push( + `"${pipeline.name}": agents span multiple project roots (${[...roots].join(', ')}) — a Cue pipeline must live in a single project.` + ); + continue; } + if (missingRoot) { + errors.push( + `"${pipeline.name}": one or more agents have no resolvable project root — assign a working directory to the agent(s).` + ); + continue; + } + + const root = [...roots][0]; + const existing = pipelinesByRoot.get(root) ?? []; + existing.push(pipeline); + pipelinesByRoot.set(root, existing); } - // If no specific project roots found, use first session's project root - if (projectRoots.size === 0 && sessions.length > 0) { - const firstWithRoot = sessions.find((s) => s.projectRoot); - if (firstWithRoot?.projectRoot) { - projectRoots.add(firstWithRoot.projectRoot); - } + if (unresolvedPipelines.length > 0) { + errors.push( + `No project root found for pipeline(s): ${unresolvedPipelines.join(', ')} — agents need a working directory.` + ); } - // No project root means we can't write YAML - if (projectRoots.size === 0) { - errors.push('No project root found — agents must have a working directory to save YAML'); + // Safety net: if the editor has pipelines but nothing will be written and + // no previously-saved root needs clearing, the save would silently succeed + // with no effect. Surface that rather than masking it as "Saved". + if (pipelineState.pipelines.length > 0 && pipelinesByRoot.size === 0 && errors.length === 0) { + errors.push( + 'Nothing to save — pipelines are empty. Add a trigger and an agent, then try again.' + ); } setValidationErrors(errors); if (errors.length > 0) return; + // Use the project roots written by the previous successful save (or + // seeded from the initial load). Re-deriving roots from savedStateRef + // at save time fails when an agent has been renamed or removed since + // the previous save — its sessionId/Name no longer resolves to a + // projectRoot, so the stale YAML at that root would never be cleared. + const previousRoots = new Set(lastWrittenRootsRef.current); + setSaveStatus('saving'); try { - const { yaml: yamlContent, promptFiles } = pipelinesToYaml( - pipelineState.pipelines, - cueSettings - ); - - // Convert prompt files Map to plain object for IPC - const promptFilesObj: Record = {}; - for (const [filePath, content] of promptFiles) { - promptFilesObj[filePath] = content; + const currentRoots = new Set(pipelinesByRoot.keys()); + const touchedRoots = new Set([...currentRoots, ...previousRoots]); + let totalPipelinesWritten = 0; + let rootsCleared = 0; + + // Write each root's YAML with only that root's pipelines. + for (const root of currentRoots) { + const rootPipelines = pipelinesByRoot.get(root)!; + const { yaml: yamlContent, promptFiles } = pipelinesToYaml(rootPipelines, cueSettings); + const promptFilesObj: Record = {}; + for (const [filePath, content] of promptFiles) { + promptFilesObj[filePath] = content; + } + await cueService.writeYaml(root, yamlContent, promptFilesObj); + + // Write-back verification: read the YAML we just wrote and + // confirm our content is on disk. Guards against any silent + // IPC failure path — if disk doesn't match memory, we throw + // so the user sees an error instead of a fake "Saved". + const onDisk = await cueService.readYaml(root); + if (onDisk === null) { + throw new Error(`writeYaml to "${root}" did not persist: no file on disk`); + } + if (onDisk !== yamlContent) { + throw new Error( + `writeYaml to "${root}" did not persist the expected content (${onDisk.length} bytes on disk vs ${yamlContent.length} expected)` + ); + } + totalPipelinesWritten += rootPipelines.length; } - // Write YAML + prompt files and refresh sessions - for (const root of projectRoots) { - await window.maestro.cue.writeYaml(root, yamlContent, promptFilesObj); + // Clear any root whose pipelines were all removed this save. Use the + // same write-and-verify path as non-empty writes so an empty YAML + // clear can never be a silent no-op (the user would see the deleted + // pipeline reappear on next launch). + for (const root of previousRoots) { + if (currentRoots.has(root)) continue; + const { yaml: emptyYaml } = pipelinesToYaml([], cueSettings); + await cueService.writeYaml(root, emptyYaml, {}); + const onDisk = await cueService.readYaml(root); + if (onDisk === null) { + throw new Error(`writeYaml clear of "${root}" did not persist: no file on disk`); + } + if (onDisk !== emptyYaml) { + throw new Error( + `writeYaml clear of "${root}" did not persist the expected content (${onDisk.length} bytes on disk vs ${emptyYaml.length} expected)` + ); + } + rootsCleared++; } - // Refresh all sessions involved + // Refresh every session whose project root was touched so the engine + // reloads the freshly written YAML into its in-memory registry. for (const session of sessions) { - if ( - session.projectRoot && - (projectRoots.has(session.projectRoot) || sessionNames.has(session.name)) - ) { - await window.maestro.cue.refreshSession(session.id, session.projectRoot); + if (session.projectRoot && touchedRoots.has(session.projectRoot)) { + await cueService.refreshSession(session.id, session.projectRoot); } } savedStateRef.current = JSON.stringify(pipelineState.pipelines); + // Snapshot the roots we actually wrote so the next save knows which + // of them may need an empty-YAML clear if their pipelines vanish. + lastWrittenRootsRef.current = new Set(currentRoots); setIsDirty(false); setSaveStatus('success'); persistLayout(); setTimeout(() => setSaveStatus('idle'), 2000); + + // Explicit confirmation so the user cannot miss the brief in-button + // status flash — "didn't save" used to happen when the 2-second + // success indicator was blinked past without the user noticing. + const rootLabel = currentRoots.size === 1 ? 'project' : 'projects'; + const pipelineLabel = totalPipelinesWritten === 1 ? 'pipeline' : 'pipelines'; + const clearedSuffix = + rootsCleared > 0 + ? ` (cleared ${rootsCleared} empty ${rootsCleared === 1 ? 'project' : 'projects'})` + : ''; + notifyToast({ + type: 'success', + title: 'Cue pipelines saved', + message: `Saved ${totalPipelinesWritten} ${pipelineLabel} to ${currentRoots.size} ${rootLabel}${clearedSuffix}.`, + }); } catch (err: unknown) { captureException(err, { extra: { operation: 'cue.pipelineSave' } }); setSaveStatus('error'); setTimeout(() => setSaveStatus('idle'), 3000); + // Keep isDirty = true so the user knows their changes are still + // unsaved (do NOT update savedStateRef on failure). + const message = err instanceof Error ? err.message : String(err); + notifyToast({ + type: 'error', + title: 'Cue save failed', + message: `Your changes were NOT saved. ${message}`, + }); } - }, [pipelineState.pipelines, sessions, cueSettings, persistLayout]); + }, [ + pipelineState.pipelines, + sessions, + cueSettings, + persistLayout, + savedStateRef, + lastWrittenRootsRef, + ]); const handleDiscard = useCallback(async () => { try { - const data = await window.maestro.cue.getGraphData(); + const data = await cueService.getGraphData(); + let restoredPipelines: CuePipeline[] = []; if (data && data.length > 0) { - const pipelines = graphSessionsToPipelines(data, sessions); + restoredPipelines = graphSessionsToPipelines(data, sessions); setPipelineState({ - pipelines, - selectedPipelineId: pipelines.length > 0 ? pipelines[0].id : null, + pipelines: restoredPipelines, + selectedPipelineId: restoredPipelines.length > 0 ? restoredPipelines[0].id : null, }); - savedStateRef.current = JSON.stringify(pipelines); + savedStateRef.current = JSON.stringify(restoredPipelines); } else { setPipelineState({ pipelines: [], selectedPipelineId: null }); savedStateRef.current = '[]'; } + // Re-derive the written-roots set from what was just loaded so the + // next save knows which roots to clear if pipelines disappear again. + const sessionsById = new Map(sessions.map((s) => [s.id, s])); + const sessionsByName = new Map(sessions.map((s) => [s.name, s])); + const restoredRoots = new Set(); + for (const pipeline of restoredPipelines) { + for (const node of pipeline.nodes) { + if (node.type !== 'agent') continue; + const data = node.data as AgentNodeData; + const root = + sessionsById.get(data.sessionId)?.projectRoot ?? + sessionsByName.get(data.sessionName)?.projectRoot; + if (root) restoredRoots.add(root); + } + } + lastWrittenRootsRef.current = restoredRoots; setIsDirty(false); setValidationErrors([]); } catch (err: unknown) { @@ -553,6 +786,7 @@ export function usePipelineState({ setShowSettings, runningPipelineIds, persistLayout, + pendingSavedViewportRef, handleSave, handleDiscard, createPipeline, diff --git a/src/renderer/hooks/remote/useRemoteIntegration.ts b/src/renderer/hooks/remote/useRemoteIntegration.ts index 7a9873435..6d266298c 100644 --- a/src/renderer/hooks/remote/useRemoteIntegration.ts +++ b/src/renderer/hooks/remote/useRemoteIntegration.ts @@ -1,5 +1,7 @@ import { useEffect, useRef } from 'react'; import type { Session, SessionState, ThinkingMode } from '../../types'; +import { cueService } from '../../services/cue'; +import { captureException } from '../../utils/sentry'; import { createTab, closeTab } from '../../utils/tabHelpers'; /** @@ -886,10 +888,23 @@ export function useRemoteIntegration(deps: UseRemoteIntegrationDeps): UseRemoteI const unsubscribe = window.maestro.process.onRemoteTriggerCueSubscription( async (subscriptionName: string, prompt: string | undefined, responseChannel: string) => { try { - const result = await window.maestro.cue.triggerSubscription(subscriptionName, prompt); + const result = await cueService.triggerSubscription(subscriptionName, prompt); window.maestro.process.sendRemoteTriggerCueSubscriptionResponse(responseChannel, result); } catch (error) { console.error('[Remote Cue Trigger] Failed:', subscriptionName, error); + // Never send the raw prompt to telemetry — remote-triggered + // Cue prompts can carry user-authored content with PII or + // secrets. Send length/presence so we can correlate failures + // against payload size without leaking the body. + captureException(error, { + extra: { + context: 'remoteTriggerCueSubscription', + subscriptionName, + responseChannel, + promptLength: prompt?.length ?? 0, + promptProvided: prompt !== undefined, + }, + }); window.maestro.process.sendRemoteTriggerCueSubscriptionResponse(responseChannel, false); } } diff --git a/src/renderer/hooks/useCue.ts b/src/renderer/hooks/useCue.ts index 7ad05529a..cda7ba5af 100644 --- a/src/renderer/hooks/useCue.ts +++ b/src/renderer/hooks/useCue.ts @@ -1,6 +1,7 @@ import { useState, useEffect, useCallback, useRef } from 'react'; import type { CueRunResult, CueSessionStatus } from '../../shared/cue'; export type { CueRunResult, CueSessionStatus } from '../../shared/cue'; +import { cueService } from '../services/cue'; export interface UseCueReturn { sessions: CueSessionStatus[]; @@ -36,6 +37,12 @@ export function useCue(): UseCueReturn { const refresh = useCallback(async () => { try { setError(null); + // NOTE: these reads bypass the cueService wrapper deliberately. + // cueService.* read methods swallow IPC failures and return safe + // defaults so the rest of the UI degrades silently — but useCue + // powers the Cue dashboard, where an IPC failure must surface as + // a user-visible error banner. Going direct preserves the catch + // path below; the wrapper would make `err` unreachable here. const [statusData, runsData, logData, queueData] = await Promise.all([ window.maestro.cue.getStatus(), window.maestro.cue.getActiveRuns(), @@ -58,31 +65,31 @@ export function useCue(): UseCueReturn { }, []); const enable = useCallback(async () => { - await window.maestro.cue.enable(); + await cueService.enable(); await refresh(); }, [refresh]); const disable = useCallback(async () => { - await window.maestro.cue.disable(); + await cueService.disable(); await refresh(); }, [refresh]); const stopRun = useCallback( async (runId: string) => { - await window.maestro.cue.stopRun(runId); + await cueService.stopRun(runId); await refresh(); }, [refresh] ); const stopAll = useCallback(async () => { - await window.maestro.cue.stopAll(); + await cueService.stopAll(); await refresh(); }, [refresh]); const triggerSubscription = useCallback( async (subscriptionName: string) => { - await window.maestro.cue.triggerSubscription(subscriptionName); + await cueService.triggerSubscription(subscriptionName); await refresh(); }, [refresh] @@ -94,7 +101,7 @@ export function useCue(): UseCueReturn { refresh(); // Subscribe to real-time activity updates - const unsubscribe = window.maestro.cue.onActivityUpdate(() => { + const unsubscribe = cueService.onActivityUpdate(() => { refresh(); }); diff --git a/src/renderer/services/cue.ts b/src/renderer/services/cue.ts new file mode 100644 index 000000000..c8cc28943 --- /dev/null +++ b/src/renderer/services/cue.ts @@ -0,0 +1,198 @@ +/** + * Cue IPC service + * + * Wraps all window.maestro.cue.* IPC calls with consistent error handling via + * createIpcMethod. Read methods return a safe default on failure; write methods + * rethrow so callers can handle or report errors. + */ + +import type { + CueSettings, + CueSessionStatus, + CueGraphSession, + CueRunResult, +} from '../../shared/cue/contracts'; +import { createIpcMethod } from './ipcWrapper'; + +export const cueService = { + // ── Read methods (return default on error) ──────────────────────────────── + + async getSettings(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.getSettings(), + errorContext: 'Cue getSettings', + defaultValue: {} as CueSettings, + }); + }, + + async getStatus(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.getStatus(), + errorContext: 'Cue getStatus', + defaultValue: [], + }); + }, + + async getGraphData(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.getGraphData(), + errorContext: 'Cue getGraphData', + defaultValue: [], + }); + }, + + async getActiveRuns(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.getActiveRuns(), + errorContext: 'Cue getActiveRuns', + defaultValue: [], + }); + }, + + async getActivityLog(limit?: number): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.getActivityLog(limit), + errorContext: 'Cue getActivityLog', + defaultValue: [], + }); + }, + + async getQueueStatus(): Promise> { + return createIpcMethod({ + call: () => window.maestro.cue.getQueueStatus(), + errorContext: 'Cue getQueueStatus', + defaultValue: {}, + }); + }, + + async readYaml(projectRoot: string): Promise { + // rethrow (instead of swallow + null) so callers can distinguish two + // outcomes that the IPC handler models distinctly: + // - resolves to null → file does not exist (handler returned null) + // - throws → IPC / fs read failure + // The previous defaultValue: null collapsed both into "null" and made + // callers like CueYamlEditor silently fall back to a template even on + // transport errors. Existing call sites already cope with throws via + // outer try/catch (CueYamlEditor) or the write-back verification path + // in handleSave (which is now strictly more informative — the IPC + // error message propagates instead of "did not persist"). + return createIpcMethod({ + call: () => window.maestro.cue.readYaml(projectRoot), + errorContext: 'Cue readYaml', + rethrow: true, + }); + }, + + async loadPipelineLayout(): Promise | null> { + return createIpcMethod({ + call: () => window.maestro.cue.loadPipelineLayout(), + errorContext: 'Cue loadPipelineLayout', + defaultValue: null, + }); + }, + + async validateYaml(content: string): Promise<{ valid: boolean; errors: string[] }> { + // rethrow on IPC failure (instead of swallowing as `{ valid: true }`). + // The previous default was actively dangerous: a transport failure + // would surface as "yaml is valid, save freely" — exactly the wrong + // fallback. Callers (CueYamlEditor) already catch the rejection and + // gate Save by setting isValid=false + a meaningful error. + return createIpcMethod({ + call: () => window.maestro.cue.validateYaml(content), + errorContext: 'Cue validateYaml', + rethrow: true, + }); + }, + + // ── Write methods (rethrow on error) ────────────────────────────────────── + + async enable(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.enable(), + errorContext: 'Cue enable', + rethrow: true, + }); + }, + + async disable(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.disable(), + errorContext: 'Cue disable', + rethrow: true, + }); + }, + + async stopRun(runId: string): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.stopRun(runId), + errorContext: 'Cue stopRun', + rethrow: true, + }); + }, + + async stopAll(): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.stopAll(), + errorContext: 'Cue stopAll', + rethrow: true, + }); + }, + + async triggerSubscription(subscriptionName: string, prompt?: string): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.triggerSubscription(subscriptionName, prompt), + errorContext: 'Cue triggerSubscription', + rethrow: true, + }); + }, + + async refreshSession(sessionId: string, projectRoot: string): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.refreshSession(sessionId, projectRoot), + errorContext: 'Cue refreshSession', + rethrow: true, + }); + }, + + async removeSession(sessionId: string): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.removeSession(sessionId), + errorContext: 'Cue removeSession', + rethrow: true, + }); + }, + + async writeYaml( + projectRoot: string, + content: string, + promptFiles?: Record + ): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.writeYaml(projectRoot, content, promptFiles), + errorContext: 'Cue writeYaml', + rethrow: true, + }); + }, + + async deleteYaml(projectRoot: string): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.deleteYaml(projectRoot), + errorContext: 'Cue deleteYaml', + rethrow: true, + }); + }, + + async savePipelineLayout(layout: Record): Promise { + return createIpcMethod({ + call: () => window.maestro.cue.savePipelineLayout(layout), + errorContext: 'Cue savePipelineLayout', + rethrow: true, + }); + }, + + // ── Event passthrough ───────────────────────────────────────────────────── + + onActivityUpdate(callback: (data: CueRunResult) => void): () => void { + return window.maestro.cue.onActivityUpdate(callback); + }, +}; diff --git a/src/renderer/services/ipcWrapper.ts b/src/renderer/services/ipcWrapper.ts index 84bf150d8..b651002bb 100644 --- a/src/renderer/services/ipcWrapper.ts +++ b/src/renderer/services/ipcWrapper.ts @@ -4,7 +4,7 @@ * Provides a utility for wrapping IPC calls with consistent error handling patterns. * Reduces boilerplate in service files by abstracting try-catch patterns. * - * Used by: git.ts, process.ts + * Used by: git.ts, process.ts, cue.ts * * @example * // For methods that return a default value on error (swallow errors): @@ -23,6 +23,8 @@ * }); */ +import { captureException } from '../utils/sentry'; + /** * Options for createIpcMethod when errors should be swallowed * and a default value returned instead. @@ -86,16 +88,26 @@ export type IpcMethodOptions = IpcMethodOptionsWithDefault | IpcMethodOpti * }); */ export async function createIpcMethod(options: IpcMethodOptions): Promise { + // Only catch the IPC call itself. The previous shape wrapped the transform + // in the same try/catch, which silently converted programmer errors in + // transform() into the swallow-path defaultValue — masking real bugs. The + // transform now runs outside the catch so its exceptions propagate. + let result: T; try { - const result = await options.call(); - return options.transform ? options.transform(result) : result; + result = await options.call(); } catch (error) { console.error(`${options.errorContext} error:`, error); if (options.rethrow) { + // Caller is responsible for handling/reporting. throw error; } + // Swallow path: the caller never sees this error, so report it to + // Sentry here — otherwise IPC failures behind read methods (return + // default on error) would be invisible in production. + void captureException(error, { extra: { context: options.errorContext } }); return options.defaultValue as T; } + return options.transform ? options.transform(result) : result; } // ============================================================================ diff --git a/src/renderer/stores/cueDirtyStore.ts b/src/renderer/stores/cueDirtyStore.ts new file mode 100644 index 000000000..4b5f0be77 --- /dev/null +++ b/src/renderer/stores/cueDirtyStore.ts @@ -0,0 +1,27 @@ +/** + * cueDirtyStore — Unified dirty-state for the Cue pipeline editor and YAML editor. + * + * Centralises unsaved-change flags so CueModal can read them from one place + * (via getState()) without prop-drilling through CuePipelineEditor and + * CueYamlEditor. + */ + +import { create } from 'zustand'; + +interface CueDirtyState { + pipelineDirty: boolean; + yamlDirty: boolean; + setPipelineDirty: (dirty: boolean) => void; + setYamlDirty: (dirty: boolean) => void; + isAnyDirty: () => boolean; + resetAll: () => void; +} + +export const useCueDirtyStore = create((set, get) => ({ + pipelineDirty: false, + yamlDirty: false, + setPipelineDirty: (dirty) => set({ pipelineDirty: dirty }), + setYamlDirty: (dirty) => set({ yamlDirty: dirty }), + isAnyDirty: () => get().pipelineDirty || get().yamlDirty, + resetAll: () => set({ pipelineDirty: false, yamlDirty: false }), +})); diff --git a/src/shared/cue-pipeline-types.ts b/src/shared/cue-pipeline-types.ts index 7d9209d24..54f93ba2f 100644 --- a/src/shared/cue-pipeline-types.ts +++ b/src/shared/cue-pipeline-types.ts @@ -112,6 +112,15 @@ export interface PipelineLayoutState { pipelines: CuePipeline[]; selectedPipelineId: string | null; viewport?: PipelineViewport; + /** + * Set of project roots that the most recent successful save wrote to. + * Persisted alongside the layout so we can re-seed lastWrittenRootsRef on + * editor mount even when an agent that previously wrote to a root has been + * renamed or removed since (in which case sessionId/sessionName lookup + * would otherwise miss the root and a future "delete the orphaned pipeline" + * save would leave a stale YAML at that root). + */ + writtenRoots?: string[]; } /** Session data with subscriptions for the Cue graph/pipeline visualization */