diff --git a/.agents/plans/completed/phase-2.5-unified-isolation.plan.md b/.agents/plans/completed/phase-2.5-unified-isolation.plan.md new file mode 100644 index 00000000..b2d2ad58 --- /dev/null +++ b/.agents/plans/completed/phase-2.5-unified-isolation.plan.md @@ -0,0 +1,1602 @@ +# Plan: Phase 2.5 - Unified Isolation Architecture + +## Summary + +Centralize all isolation logic (currently split between GitHub adapter and orchestrator) into the orchestrator with a new work-centric database schema. This enables cross-platform worktree sharing and consistent isolation behavior across all adapters (GitHub, Slack, Discord, Telegram). + +## Intent + +The current architecture has fragmented isolation logic: GitHub adapter handles worktree creation, cleanup, and UX messaging while other adapters have no automation. This causes code duplication, inconsistent behavior, and prevents cross-platform worktree sharing. By centralizing in the orchestrator with a proper data model, we get DRY code, consistent behavior across platforms, and the foundation for future features like workflow-aware isolation. + +## Persona + +**Primary**: Remote developer using AI assistants via Telegram/Slack who wants automatic worktree isolation without manual `/worktree create` commands. + +**Secondary**: Developer working across platforms (starts in Slack, opens GitHub PR) who needs the same worktree shared across conversations. + +## UX + +### Before (Current State) + +``` +[Telegram/Slack/Discord] [GitHub] +┌───────────────────────┐ ┌───────────────────────┐ +│ User: @bot fix login │ │ User: @bot fix #42 │ +│ │ │ │ +│ Bot: Working... │ │ Bot: Working in │ +│ (uses main repo) │ │ isolated branch │ +│ │ │ `issue-42`... │ +│ Risk: Conflicts with │ │ │ +│ other conversations! │ │ (auto-isolation!) │ +└───────────────────────┘ └───────────────────────┘ + +User must run /worktree create GitHub auto-isolates +manually on non-GitHub platforms +``` + +### After (Phase 2.5) + +``` +[ALL PLATFORMS - Telegram/Slack/Discord/GitHub] +┌─────────────────────────────────────────────────────────┐ +│ User: @bot fix the login bug │ +│ │ +│ Bot: Working in isolated branch `thread-a7f3b2c1` │ +│ (auto-created, no manual command needed) │ +│ │ +│ [User opens GitHub issue #42 for same work] │ +│ │ +│ Bot (on GitHub): Linked to existing worktree │ +│ (shares isolation via metadata) │ +└─────────────────────────────────────────────────────────┘ + +Consistent auto-isolation across ALL platforms +Cross-platform worktree sharing via workflow identity +``` + +## External Research + +### Git Worktree Performance (Tested in Research Doc) +- Creation time: 0.099 seconds +- Worktree size: 2.5 MB (vs 981 MB main repo) +- Space overhead: 0.25% +- **Conclusion**: Worktrees are cheap. Create aggressively. + +### Branch Merge Detection +```bash +git branch --merged main | grep "issue-42" +``` +Git natively supports this. Trivial to implement in cleanup service. + +### Race Condition Handling +Existing `ConversationLockManager` handles concurrent access: +```typescript +lockManager.acquireLock(conversationId, async () => { + await handleMessage(...); +}); +``` +No additional work needed. + +## Patterns to Mirror + +### Database Migration Pattern +From `migrations/005_isolation_abstraction.sql:1-26`: +```sql +-- Add isolation provider abstraction columns +ALTER TABLE remote_agent_conversations +ADD COLUMN IF NOT EXISTS isolation_env_id VARCHAR(255), +ADD COLUMN IF NOT EXISTS isolation_provider VARCHAR(50) DEFAULT 'worktree'; + +-- Migrate existing data +UPDATE remote_agent_conversations +SET isolation_env_id = worktree_path, + isolation_provider = 'worktree' +WHERE worktree_path IS NOT NULL + AND isolation_env_id IS NULL; + +CREATE INDEX IF NOT EXISTS idx_conversations_isolation +ON remote_agent_conversations(isolation_env_id, isolation_provider); +``` + +### Database Query Functions Pattern +From `src/db/conversations.ts:93-138`: +```typescript +export async function updateConversation( + id: string, + updates: Partial< + Pick< + Conversation, + 'codebase_id' | 'cwd' | 'worktree_path' | 'isolation_env_id' | 'isolation_provider' + > + > +): Promise { + const fields: string[] = []; + const values: (string | null)[] = []; + let i = 1; + + if (updates.codebase_id !== undefined) { + fields.push(`codebase_id = $${String(i++)}`); + values.push(updates.codebase_id); + } + // ... pattern continues for each field +} +``` + +### Database Test Pattern +From `src/db/conversations.test.ts:1-20`: +```typescript +import { mock, describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { createQueryResult } from '../test/mocks/database'; + +const mockQuery = mock(() => Promise.resolve(createQueryResult([]))); + +// Mock the connection module before importing the module under test +mock.module('./connection', () => ({ + pool: { + query: mockQuery, + }, +})); + +import { getOrCreateConversation, updateConversation } from './conversations'; +``` + +### Isolation Provider Pattern +From `src/isolation/types.ts:1-47`: +```typescript +export interface IsolationRequest { + codebaseId: string; + canonicalRepoPath: string; + workflowType: 'issue' | 'pr' | 'review' | 'thread' | 'task'; + identifier: string; + prBranch?: string; + prSha?: string; + description?: string; +} + +export interface IsolatedEnvironment { + id: string; + provider: 'worktree' | 'container' | 'vm' | 'remote'; + workingPath: string; + branchName?: string; + status: 'active' | 'suspended' | 'destroyed'; + createdAt: Date; + metadata: Record; +} +``` + +### Orchestrator handleMessage Pattern +From `src/orchestrator/orchestrator.ts:36-43`: +```typescript +export async function handleMessage( + platform: IPlatformAdapter, + conversationId: string, + message: string, + issueContext?: string, + threadContext?: string, + parentConversationId?: string +): Promise +``` + +### GitHub Adapter Worktree UX Pattern +From `src/adapters/github.ts:712-725`: +```typescript +// UX feedback about isolation +if (prHeadSha) { + const shortSha = prHeadSha.substring(0, 7); + await this.sendMessage( + conversationId, + `Reviewing PR at commit \`${shortSha}\` (branch: \`${prHeadBranch}\`)` + ); +} else { + await this.sendMessage( + conversationId, + `Working in isolated branch \`${branchName}\`` + ); +} +``` + +## Files to Change + +| File | Action | Justification | +|------|--------|---------------| +| `migrations/006_isolation_environments.sql` | CREATE | New work-centric table with UUID PK | +| `src/db/isolation-environments.ts` | CREATE | CRUD for new isolation_environments table | +| `src/db/isolation-environments.test.ts` | CREATE | Unit tests for new DB module | +| `src/services/cleanup-service.ts` | CREATE | Cleanup logic (scheduler added in Phase 3C) | +| `src/services/cleanup-service.test.ts` | CREATE | Unit tests for cleanup service | +| `src/types/index.ts` | UPDATE | Add IsolationHints, IsolationEnvironmentRow types | +| `src/orchestrator/orchestrator.ts` | UPDATE | Add isolationHints param, validateAndResolveIsolation | +| `src/orchestrator/orchestrator.test.ts` | UPDATE | Add tests for new isolation logic | +| `src/adapters/github.ts` | UPDATE | Remove worktree creation, add IsolationHints | +| `src/adapters/github.test.ts` | UPDATE | Update tests for refactored adapter | +| `src/handlers/command-handler.ts` | UPDATE | Add `/worktree link` command | +| `src/db/conversations.ts` | UPDATE | Add query for conversations by isolation_env_id UUID | + +## NOT Building + +- **Cleanup scheduler startup** - Deferred to Phase 3C; cleanup service is ready but not auto-started +- **Force-thread response model** - Deferred to Phase 3A; not needed for isolation +- **Worktree limits enforcement** - Deferred to Phase 3D; focus on core architecture first +- **AI-assisted cross-platform linking** - Future feature; MVP uses metadata + manual `/worktree link` +- **Drop legacy columns** - Deferred to Phase 4; keep backwards compatibility + +--- + +## Tasks + +### Task 1: Create isolation_environments migration + +**Why**: New work-centric table enables independent lifecycle from conversations and cross-platform sharing. + +**Mirror**: `migrations/005_isolation_abstraction.sql` + +**Do**: +Create `migrations/006_isolation_environments.sql`: +```sql +-- Work-centric isolation environments +-- Version: 6.0 +-- Description: Independent isolation entities with workflow identity + +CREATE TABLE IF NOT EXISTS remote_agent_isolation_environments ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + codebase_id UUID NOT NULL REFERENCES remote_agent_codebases(id) ON DELETE CASCADE, + + -- Workflow identification (what work this is for) + workflow_type TEXT NOT NULL, -- 'issue', 'pr', 'review', 'thread', 'task' + workflow_id TEXT NOT NULL, -- '42', 'pr-99', 'thread-abc123' + + -- Implementation details + provider TEXT NOT NULL DEFAULT 'worktree', + working_path TEXT NOT NULL, -- Actual filesystem path + branch_name TEXT NOT NULL, -- Git branch name + + -- Lifecycle + status TEXT NOT NULL DEFAULT 'active', -- 'active', 'destroyed' + created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), + created_by_platform TEXT, -- 'github', 'slack', etc. + + -- Cross-reference metadata (for linking) + metadata JSONB DEFAULT '{}', + + CONSTRAINT unique_workflow UNIQUE (codebase_id, workflow_type, workflow_id) +); + +-- Indexes for common queries +CREATE INDEX IF NOT EXISTS idx_isolation_env_codebase + ON remote_agent_isolation_environments(codebase_id); +CREATE INDEX IF NOT EXISTS idx_isolation_env_status + ON remote_agent_isolation_environments(status); +CREATE INDEX IF NOT EXISTS idx_isolation_env_workflow + ON remote_agent_isolation_environments(workflow_type, workflow_id); + +-- Rename old column to legacy (for migration) +ALTER TABLE remote_agent_conversations + RENAME COLUMN isolation_env_id TO isolation_env_id_legacy; + +-- Add new UUID FK column +ALTER TABLE remote_agent_conversations + ADD COLUMN IF NOT EXISTS isolation_env_id UUID + REFERENCES remote_agent_isolation_environments(id) ON DELETE SET NULL; + +-- Add last_activity_at for staleness detection +ALTER TABLE remote_agent_conversations + ADD COLUMN IF NOT EXISTS last_activity_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(); + +-- Create index for FK lookups +CREATE INDEX IF NOT EXISTS idx_conversations_isolation_env_id + ON remote_agent_conversations(isolation_env_id); + +COMMENT ON TABLE remote_agent_isolation_environments IS + 'Work-centric isolated environments with independent lifecycle'; +COMMENT ON COLUMN remote_agent_isolation_environments.workflow_type IS + 'Type of work: issue, pr, review, thread, task'; +COMMENT ON COLUMN remote_agent_isolation_environments.workflow_id IS + 'Identifier for the work (issue number, PR number, thread hash, etc.)'; +``` + +**Don't**: +- Don't drop `worktree_path` or `isolation_provider` yet (Phase 4) +- Don't migrate existing data in SQL (do it in application code) + +**Verify**: `psql $DATABASE_URL < migrations/006_isolation_environments.sql` + +--- + +### Task 2: Add new types to types/index.ts + +**Why**: TypeScript needs types for the new table and IsolationHints parameter. + +**Mirror**: `src/types/index.ts:5-17` (Conversation type pattern) + +**Do**: +Add to `src/types/index.ts`: +```typescript +/** + * Isolation hints provided by adapters to orchestrator + * Allows platform-specific context without orchestrator knowing platform internals + */ +export interface IsolationHints { + // Workflow identification (adapter knows this) + workflowType?: 'issue' | 'pr' | 'review' | 'thread' | 'task'; + workflowId?: string; + + // PR-specific (for reproducible reviews) + prBranch?: string; + prSha?: string; + + // Cross-reference hints (for linking) + linkedIssues?: number[]; + linkedPRs?: number[]; + + // Adoption hints + suggestedBranch?: string; +} + +/** + * Database row for isolation_environments table + */ +export interface IsolationEnvironmentRow { + id: string; + codebase_id: string; + workflow_type: string; + workflow_id: string; + provider: string; + working_path: string; + branch_name: string; + status: string; + created_at: Date; + created_by_platform: string | null; + metadata: Record; +} +``` + +Update Conversation interface to add new fields: +```typescript +export interface Conversation { + id: string; + platform_type: string; + platform_conversation_id: string; + codebase_id: string | null; + cwd: string | null; + worktree_path: string | null; // Legacy field + isolation_env_id_legacy: string | null; // Renamed from isolation_env_id (TEXT) + isolation_env_id: string | null; // NEW: UUID FK to isolation_environments + isolation_provider: string | null; // Legacy field + ai_assistant_type: string; + last_activity_at: Date | null; // NEW: for staleness detection + created_at: Date; + updated_at: Date; +} +``` + +**Don't**: +- Don't remove existing fields (backwards compatibility) + +**Verify**: `bun run type-check` + +--- + +### Task 3: Create src/db/isolation-environments.ts + +**Why**: CRUD operations for the new work-centric table. + +**Mirror**: `src/db/conversations.ts` (query patterns) + +**Do**: +Create `src/db/isolation-environments.ts`: +```typescript +/** + * Database operations for isolation environments + */ +import { pool } from './connection'; +import { IsolationEnvironmentRow } from '../types'; + +/** + * Get an isolation environment by UUID + */ +export async function getById(id: string): Promise { + const result = await pool.query( + 'SELECT * FROM remote_agent_isolation_environments WHERE id = $1', + [id] + ); + return result.rows[0] ?? null; +} + +/** + * Find an isolation environment by workflow identity + */ +export async function findByWorkflow( + codebaseId: string, + workflowType: string, + workflowId: string +): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND workflow_type = $2 AND workflow_id = $3 AND status = 'active'`, + [codebaseId, workflowType, workflowId] + ); + return result.rows[0] ?? null; +} + +/** + * Find all active environments for a codebase + */ +export async function listByCodebase(codebaseId: string): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND status = 'active' + ORDER BY created_at DESC`, + [codebaseId] + ); + return result.rows; +} + +/** + * Create a new isolation environment + */ +export async function create(env: { + codebase_id: string; + workflow_type: string; + workflow_id: string; + provider?: string; + working_path: string; + branch_name: string; + created_by_platform?: string; + metadata?: Record; +}): Promise { + const result = await pool.query( + `INSERT INTO remote_agent_isolation_environments + (codebase_id, workflow_type, workflow_id, provider, working_path, branch_name, created_by_platform, metadata) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + RETURNING *`, + [ + env.codebase_id, + env.workflow_type, + env.workflow_id, + env.provider ?? 'worktree', + env.working_path, + env.branch_name, + env.created_by_platform ?? null, + JSON.stringify(env.metadata ?? {}), + ] + ); + return result.rows[0]; +} + +/** + * Update environment status + */ +export async function updateStatus(id: string, status: 'active' | 'destroyed'): Promise { + await pool.query( + 'UPDATE remote_agent_isolation_environments SET status = $1 WHERE id = $2', + [status, id] + ); +} + +/** + * Update environment metadata (merge with existing) + */ +export async function updateMetadata( + id: string, + metadata: Record +): Promise { + await pool.query( + `UPDATE remote_agent_isolation_environments + SET metadata = metadata || $1::jsonb + WHERE id = $2`, + [JSON.stringify(metadata), id] + ); +} + +/** + * Find environments by related issue (from metadata) + */ +export async function findByRelatedIssue( + codebaseId: string, + issueNumber: number +): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 + AND status = 'active' + AND metadata->'related_issues' ? $2 + LIMIT 1`, + [codebaseId, String(issueNumber)] + ); + return result.rows[0] ?? null; +} + +/** + * Count active environments for a codebase (for limit checks) + */ +export async function countByCodebase(codebaseId: string): Promise { + const result = await pool.query<{ count: string }>( + `SELECT COUNT(*) as count FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND status = 'active'`, + [codebaseId] + ); + return parseInt(result.rows[0]?.count ?? '0', 10); +} + +/** + * Find conversations using an isolation environment + */ +export async function getConversationsUsingEnv(envId: string): Promise { + const result = await pool.query<{ id: string }>( + 'SELECT id FROM remote_agent_conversations WHERE isolation_env_id = $1', + [envId] + ); + return result.rows.map(r => r.id); +} +``` + +**Don't**: +- Don't add cleanup logic here (that's in cleanup-service) + +**Verify**: `bun run type-check` + +--- + +### Task 4: Create src/db/isolation-environments.test.ts + +**Why**: Unit tests for the new DB module. + +**Mirror**: `src/db/conversations.test.ts` + +**Do**: +Create `src/db/isolation-environments.test.ts`: +```typescript +import { mock, describe, test, expect, beforeEach } from 'bun:test'; +import { createQueryResult } from '../test/mocks/database'; + +const mockQuery = mock(() => Promise.resolve(createQueryResult([]))); + +mock.module('./connection', () => ({ + pool: { + query: mockQuery, + }, +})); + +import { getById, findByWorkflow, create, updateStatus, countByCodebase } from './isolation-environments'; +import { IsolationEnvironmentRow } from '../types'; + +describe('isolation-environments', () => { + beforeEach(() => { + mockQuery.mockClear(); + }); + + const sampleEnv: IsolationEnvironmentRow = { + id: 'env-123', + codebase_id: 'codebase-456', + workflow_type: 'issue', + workflow_id: '42', + provider: 'worktree', + working_path: '/workspace/worktrees/project/issue-42', + branch_name: 'issue-42', + status: 'active', + created_at: new Date(), + created_by_platform: 'github', + metadata: {}, + }; + + describe('getById', () => { + test('returns environment when found', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await getById('env-123'); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + 'SELECT * FROM remote_agent_isolation_environments WHERE id = $1', + ['env-123'] + ); + }); + + test('returns null when not found', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([])); + + const result = await getById('nonexistent'); + + expect(result).toBeNull(); + }); + }); + + describe('findByWorkflow', () => { + test('finds active environment by workflow identity', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await findByWorkflow('codebase-456', 'issue', '42'); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('workflow_type = $2 AND workflow_id = $3'), + ['codebase-456', 'issue', '42'] + ); + }); + }); + + describe('create', () => { + test('creates new environment with defaults', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await create({ + codebase_id: 'codebase-456', + workflow_type: 'issue', + workflow_id: '42', + working_path: '/workspace/worktrees/project/issue-42', + branch_name: 'issue-42', + }); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('INSERT INTO remote_agent_isolation_environments'), + expect.arrayContaining(['codebase-456', 'issue', '42', 'worktree']) + ); + }); + }); + + describe('updateStatus', () => { + test('updates status to destroyed', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([], 1)); + + await updateStatus('env-123', 'destroyed'); + + expect(mockQuery).toHaveBeenCalledWith( + 'UPDATE remote_agent_isolation_environments SET status = $1 WHERE id = $2', + ['destroyed', 'env-123'] + ); + }); + }); + + describe('countByCodebase', () => { + test('returns count of active environments', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([{ count: '5' }])); + + const result = await countByCodebase('codebase-456'); + + expect(result).toBe(5); + }); + }); +}); +``` + +**Verify**: `bun test src/db/isolation-environments.test.ts` + +--- + +### Task 5: Create src/services/cleanup-service.ts + +**Why**: Centralized cleanup logic, separate from orchestrator. Called by adapters and (later) scheduler. + +**Mirror**: `src/adapters/github.ts:444-516` (existing cleanup logic) + +**Do**: +Create `src/services/cleanup-service.ts`: +```typescript +/** + * Cleanup service for isolation environments + * Handles removal triggered by events, schedule, or commands + */ +import * as isolationEnvDb from '../db/isolation-environments'; +import * as conversationDb from '../db/conversations'; +import * as sessionDb from '../db/sessions'; +import { getIsolationProvider } from '../isolation'; +import { execFileAsync } from '../utils/git'; + +export interface CleanupReport { + removed: string[]; + skipped: Array<{ id: string; reason: string }>; + errors: Array<{ id: string; error: string }>; +} + +/** + * Called when a platform conversation is closed (e.g., GitHub issue/PR closed) + * Cleans up the associated isolation environment if no other conversations use it + */ +export async function onConversationClosed( + platformType: string, + platformConversationId: string +): Promise { + console.log(`[Cleanup] Conversation closed: ${platformType}/${platformConversationId}`); + + // Find the conversation + const conversation = await conversationDb.getConversationByPlatformId( + platformType, + platformConversationId + ); + + if (!conversation?.isolation_env_id) { + console.log('[Cleanup] No isolation environment to clean up'); + return; + } + + const envId = conversation.isolation_env_id; + + // Deactivate any active sessions first + const session = await sessionDb.getActiveSession(conversation.id); + if (session) { + await sessionDb.deactivateSession(session.id); + console.log(`[Cleanup] Deactivated session ${session.id}`); + } + + // Get the environment + const env = await isolationEnvDb.getById(envId); + if (!env) { + console.log(`[Cleanup] Environment ${envId} not found in database`); + return; + } + + // Clear this conversation's reference (regardless of whether we remove the worktree) + await conversationDb.updateConversation(conversation.id, { + isolation_env_id: null, + worktree_path: null, + isolation_provider: null, + // Keep cwd pointing to main repo (will be set by caller or orchestrator) + }); + + // Check if other conversations still use this environment + const otherConversations = await isolationEnvDb.getConversationsUsingEnv(envId); + if (otherConversations.length > 0) { + console.log(`[Cleanup] Environment still used by ${otherConversations.length} conversation(s), keeping`); + return; + } + + // No other users - attempt removal + await removeEnvironment(envId, { force: false }); +} + +/** + * Remove a specific environment + */ +export async function removeEnvironment( + envId: string, + options?: { force?: boolean } +): Promise { + const env = await isolationEnvDb.getById(envId); + if (!env) { + console.log(`[Cleanup] Environment ${envId} not found`); + return; + } + + if (env.status === 'destroyed') { + console.log(`[Cleanup] Environment ${envId} already destroyed`); + return; + } + + const provider = getIsolationProvider(); + + try { + // Check for uncommitted changes (unless force) + if (!options?.force) { + const hasChanges = await hasUncommittedChanges(env.working_path); + if (hasChanges) { + console.warn(`[Cleanup] Environment ${envId} has uncommitted changes, skipping`); + return; + } + } + + // Remove the worktree + await provider.destroy(env.working_path, { force: options?.force }); + + // Mark as destroyed in database + await isolationEnvDb.updateStatus(envId, 'destroyed'); + + console.log(`[Cleanup] Removed environment ${envId} at ${env.working_path}`); + } catch (error) { + const err = error as Error; + console.error(`[Cleanup] Failed to remove environment ${envId}:`, err.message); + throw err; + } +} + +/** + * Check if a worktree has uncommitted changes + */ +export async function hasUncommittedChanges(workingPath: string): Promise { + try { + const { stdout } = await execFileAsync('git', ['-C', workingPath, 'status', '--porcelain']); + return stdout.trim().length > 0; + } catch { + // If git fails, assume it's safe to remove (path might not exist) + return false; + } +} + +/** + * Check if a branch has been merged into main + */ +export async function isBranchMerged( + repoPath: string, + branchName: string, + mainBranch = 'main' +): Promise { + try { + const { stdout } = await execFileAsync('git', [ + '-C', repoPath, + 'branch', '--merged', mainBranch + ]); + const mergedBranches = stdout.split('\n').map(b => b.trim().replace(/^\* /, '')); + return mergedBranches.includes(branchName); + } catch { + return false; + } +} + +/** + * Get the last commit date for a worktree + */ +export async function getLastCommitDate(workingPath: string): Promise { + try { + const { stdout } = await execFileAsync('git', [ + '-C', workingPath, + 'log', '-1', '--format=%ci' + ]); + return new Date(stdout.trim()); + } catch { + return null; + } +} + +/** + * Clean up to make room when limit reached (Phase 3D will call this) + * Attempts to remove merged branches first + */ +export async function cleanupToMakeRoom( + codebaseId: string, + mainRepoPath: string +): Promise { + const envs = await isolationEnvDb.listByCodebase(codebaseId); + let removed = 0; + + for (const env of envs) { + // Try merged branches first + const merged = await isBranchMerged(mainRepoPath, env.branch_name); + if (merged) { + const hasChanges = await hasUncommittedChanges(env.working_path); + if (!hasChanges) { + try { + await removeEnvironment(env.id); + removed++; + } catch { + // Continue to next + } + } + } + } + + return removed; +} +``` + +**Don't**: +- Don't add scheduler (Phase 3C) +- Don't add limit enforcement (Phase 3D) + +**Verify**: `bun run type-check` + +--- + +### Task 6: Create src/services/cleanup-service.test.ts + +**Why**: Unit tests for cleanup logic. + +**Mirror**: `src/db/conversations.test.ts` (mock patterns) + +**Do**: +Create `src/services/cleanup-service.test.ts`: +```typescript +import { mock, describe, test, expect, beforeEach } from 'bun:test'; +import { createQueryResult } from '../test/mocks/database'; + +// Mock database modules +const mockQuery = mock(() => Promise.resolve(createQueryResult([]))); +mock.module('../db/connection', () => ({ + pool: { query: mockQuery }, +})); + +// Mock git utility +const mockExecFileAsync = mock(() => Promise.resolve({ stdout: '', stderr: '' })); +mock.module('../utils/git', () => ({ + execFileAsync: mockExecFileAsync, +})); + +// Mock isolation provider +const mockDestroy = mock(() => Promise.resolve()); +mock.module('../isolation', () => ({ + getIsolationProvider: () => ({ + destroy: mockDestroy, + }), +})); + +import { hasUncommittedChanges, isBranchMerged, getLastCommitDate } from './cleanup-service'; + +describe('cleanup-service', () => { + beforeEach(() => { + mockQuery.mockClear(); + mockExecFileAsync.mockClear(); + mockDestroy.mockClear(); + }); + + describe('hasUncommittedChanges', () => { + test('returns true when git status shows changes', async () => { + mockExecFileAsync.mockResolvedValueOnce({ stdout: ' M file.ts\n', stderr: '' }); + + const result = await hasUncommittedChanges('/workspace/test'); + + expect(result).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith('git', ['-C', '/workspace/test', 'status', '--porcelain']); + }); + + test('returns false when git status is clean', async () => { + mockExecFileAsync.mockResolvedValueOnce({ stdout: '', stderr: '' }); + + const result = await hasUncommittedChanges('/workspace/test'); + + expect(result).toBe(false); + }); + + test('returns false when git fails (path not found)', async () => { + mockExecFileAsync.mockRejectedValueOnce(new Error('not a git repository')); + + const result = await hasUncommittedChanges('/nonexistent'); + + expect(result).toBe(false); + }); + }); + + describe('isBranchMerged', () => { + test('returns true when branch is in merged list', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' feature-a\n issue-42\n* main\n', + stderr: '' + }); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(true); + }); + + test('returns false when branch is not merged', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' feature-a\n* main\n', + stderr: '' + }); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(false); + }); + }); + + describe('getLastCommitDate', () => { + test('returns date from git log', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: '2025-01-15 10:30:00 +0000\n', + stderr: '' + }); + + const result = await getLastCommitDate('/workspace/test'); + + expect(result).toBeInstanceOf(Date); + expect(result?.getFullYear()).toBe(2025); + }); + + test('returns null when git fails', async () => { + mockExecFileAsync.mockRejectedValueOnce(new Error('no commits')); + + const result = await getLastCommitDate('/workspace/test'); + + expect(result).toBeNull(); + }); + }); +}); +``` + +**Verify**: `bun test src/services/cleanup-service.test.ts` + +--- + +### Task 7: Update src/db/conversations.ts for UUID lookups + +**Why**: Need to support both legacy TEXT paths and new UUID FK lookups during transition. + +**Mirror**: Existing `getConversationByIsolationEnvId` at line 144 + +**Do**: +Add to `src/db/conversations.ts`: +```typescript +/** + * Find all conversations using a specific isolation environment (new UUID model) + */ +export async function getConversationsByIsolationEnvId( + envId: string +): Promise { + const result = await pool.query( + 'SELECT * FROM remote_agent_conversations WHERE isolation_env_id = $1', + [envId] + ); + return result.rows; +} + +/** + * Update last_activity_at for staleness tracking + */ +export async function touchConversation(id: string): Promise { + await pool.query( + 'UPDATE remote_agent_conversations SET last_activity_at = NOW() WHERE id = $1', + [id] + ); +} +``` + +Update `updateConversation` to also accept `isolation_env_id` as UUID (string type covers both). + +**Verify**: `bun test src/db/conversations.test.ts` + +--- + +### Task 8: Update orchestrator with validateAndResolveIsolation + +**Why**: This is the core change - moving all isolation logic from GitHub adapter to orchestrator. + +**Mirror**: +- `src/orchestrator/orchestrator.ts:239-268` (existing cwd validation) +- `src/adapters/github.ts:629-753` (worktree creation flow to move) + +**Do**: +Update `src/orchestrator/orchestrator.ts`: + +1. Add `isolationHints` parameter to `handleMessage`: +```typescript +export async function handleMessage( + platform: IPlatformAdapter, + conversationId: string, + message: string, + issueContext?: string, + threadContext?: string, + parentConversationId?: string, + isolationHints?: IsolationHints // NEW parameter +): Promise +``` + +2. Add `validateAndResolveIsolation` function (add before handleMessage): +```typescript +import * as isolationEnvDb from '../db/isolation-environments'; +import { IsolationHints, IsolationEnvironmentRow, Codebase } from '../types'; +import { worktreeExists, findWorktreeByBranch, getCanonicalRepoPath } from '../utils/git'; + +/** + * Validate existing isolation and create new if needed + * This is the single source of truth for isolation decisions + */ +async function validateAndResolveIsolation( + conversation: Conversation, + codebase: Codebase | null, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise<{ cwd: string; env: IsolationEnvironmentRow | null; isNew: boolean }> { + + // 1. Check existing isolation reference (new UUID model) + if (conversation.isolation_env_id) { + const env = await isolationEnvDb.getById(conversation.isolation_env_id); + + if (env && await worktreeExists(env.working_path)) { + // Valid - use it + return { cwd: env.working_path, env, isNew: false }; + } + + // Stale reference - clean up + console.warn(`[Orchestrator] Stale isolation: ${conversation.isolation_env_id}`); + await db.updateConversation(conversation.id, { + isolation_env_id: null, + worktree_path: null, + isolation_provider: null, + }); + + if (env) { + await isolationEnvDb.updateStatus(env.id, 'destroyed'); + } + } + + // 2. Legacy fallback (worktree_path without new UUID) + const legacyPath = conversation.worktree_path ?? conversation.isolation_env_id_legacy; + if (legacyPath && await worktreeExists(legacyPath)) { + // Migrate to new model on-the-fly + const env = await migrateToIsolationEnvironment(conversation, codebase, legacyPath, platform); + if (env) { + return { cwd: legacyPath, env, isNew: false }; + } + } + + // 3. No valid isolation - check if we should create + if (!codebase) { + return { cwd: conversation.cwd ?? '/workspace', env: null, isNew: false }; + } + + // 4. Create new isolation (auto-isolation for all platforms!) + const env = await resolveIsolation(conversation, codebase, platform, conversationId, hints); + if (env) { + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + worktree_path: env.working_path, + isolation_provider: env.provider, + cwd: env.working_path, + }); + return { cwd: env.working_path, env, isNew: true }; + } + + return { cwd: codebase.default_cwd, env: null, isNew: false }; +} + +/** + * Resolve which isolation environment to use + * Handles reuse, sharing, adoption, and creation + */ +async function resolveIsolation( + conversation: Conversation, + codebase: Codebase, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise { + + // Determine workflow identity + const workflowType = hints?.workflowType ?? 'thread'; + const workflowId = hints?.workflowId ?? conversationId; + + // 1. Check for existing environment with same workflow + const existing = await isolationEnvDb.findByWorkflow(codebase.id, workflowType, workflowId); + if (existing && await worktreeExists(existing.working_path)) { + console.log(`[Orchestrator] Reusing environment for ${workflowType}/${workflowId}`); + return existing; + } + + // 2. Check linked issues for sharing (cross-conversation) + if (hints?.linkedIssues?.length) { + for (const issueNum of hints.linkedIssues) { + const linkedEnv = await isolationEnvDb.findByWorkflow( + codebase.id, 'issue', String(issueNum) + ); + if (linkedEnv && await worktreeExists(linkedEnv.working_path)) { + console.log(`[Orchestrator] Sharing worktree with linked issue #${issueNum}`); + // Send UX message + await platform.sendMessage( + conversationId, + `Reusing worktree from issue #${issueNum}` + ); + return linkedEnv; + } + } + } + + // 3. Try PR branch adoption (skill symbiosis) + if (hints?.prBranch) { + const canonicalPath = await getCanonicalRepoPath(codebase.default_cwd); + const adoptedPath = await findWorktreeByBranch(canonicalPath, hints.prBranch); + if (adoptedPath && await worktreeExists(adoptedPath)) { + console.log(`[Orchestrator] Adopting existing worktree at ${adoptedPath}`); + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: adoptedPath, + branch_name: hints.prBranch, + created_by_platform: platform.getPlatformType(), + metadata: { adopted: true, adopted_from: 'skill' }, + }); + return env; + } + } + + // 4. Create new worktree + const provider = getIsolationProvider(); + const canonicalPath = await getCanonicalRepoPath(codebase.default_cwd); + + try { + const isolatedEnv = await provider.create({ + codebaseId: codebase.id, + canonicalRepoPath: canonicalPath, + workflowType, + identifier: workflowId, + prBranch: hints?.prBranch, + prSha: hints?.prSha, + }); + + // Create database record + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: isolatedEnv.workingPath, + branch_name: isolatedEnv.branchName ?? `${workflowType}-${workflowId}`, + created_by_platform: platform.getPlatformType(), + metadata: { + related_issues: hints?.linkedIssues ?? [], + related_prs: hints?.linkedPRs ?? [], + }, + }); + + // UX message + if (hints?.prSha) { + const shortSha = hints.prSha.substring(0, 7); + await platform.sendMessage( + conversationId, + `Reviewing PR at commit \`${shortSha}\` (branch: \`${hints.prBranch}\`)` + ); + } else { + await platform.sendMessage( + conversationId, + `Working in isolated branch \`${env.branch_name}\`` + ); + } + + return env; + } catch (error) { + console.error('[Orchestrator] Failed to create isolation:', error); + return null; + } +} + +/** + * Migrate a legacy worktree_path to the new isolation_environments model + */ +async function migrateToIsolationEnvironment( + conversation: Conversation, + codebase: Codebase | null, + legacyPath: string, + platform: IPlatformAdapter +): Promise { + if (!codebase) return null; + + try { + const { workflowType, workflowId } = inferWorkflowFromConversation(conversation, legacyPath); + const branchName = await getBranchNameFromWorktree(legacyPath); + + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: legacyPath, + branch_name: branchName, + created_by_platform: platform.getPlatformType(), + metadata: { migrated: true, migrated_at: new Date().toISOString() }, + }); + + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + }); + + console.log(`[Orchestrator] Migrated legacy worktree to environment: ${env.id}`); + return env; + } catch (error) { + console.error('[Orchestrator] Failed to migrate legacy worktree:', error); + return null; + } +} + +function inferWorkflowFromConversation( + conversation: Conversation, + legacyPath: string +): { workflowType: string; workflowId: string } { + // Try to infer from platform conversation ID + if (conversation.platform_type === 'github') { + const match = /#(\d+)$/.exec(conversation.platform_conversation_id); + if (match) { + const isPR = legacyPath.includes('/pr-') || legacyPath.includes('-pr-'); + return { + workflowType: isPR ? 'pr' : 'issue', + workflowId: match[1], + }; + } + } + + return { + workflowType: 'thread', + workflowId: conversation.platform_conversation_id, + }; +} + +async function getBranchNameFromWorktree(path: string): Promise { + try { + const { stdout } = await execFileAsync('git', ['-C', path, 'rev-parse', '--abbrev-ref', 'HEAD']); + return stdout.trim(); + } catch { + return 'unknown'; + } +} +``` + +3. Replace the cwd resolution section (around line 239-268) with call to `validateAndResolveIsolation`: +```typescript +// Replace existing cwd resolution with: +const { cwd, env: isolationEnv, isNew: isNewIsolation } = await validateAndResolveIsolation( + conversation, + codebase, + platform, + conversationId, + isolationHints +); + +// If cwd changed, deactivate stale sessions (existing logic) +if (cwd !== conversation.cwd) { + const existingSession = await sessionDb.getActiveSession(conversation.id); + if (existingSession) { + console.log('[Orchestrator] CWD changed, deactivating existing session'); + await sessionDb.deactivateSession(existingSession.id); + } +} + +// Update last_activity_at +await db.touchConversation(conversation.id); +``` + +**Don't**: +- Don't remove existing session logic +- Don't change AI streaming logic + +**Verify**: `bun run type-check && bun test src/orchestrator/orchestrator.test.ts` + +--- + +### Task 9: Update orchestrator tests + +**Why**: Add tests for new isolation logic. + +**Mirror**: `src/orchestrator/orchestrator.test.ts` + +**Do**: +Add test cases to `src/orchestrator/orchestrator.test.ts`: +```typescript +describe('validateAndResolveIsolation', () => { + test('reuses existing valid isolation environment', async () => { + // Setup: conversation with isolation_env_id pointing to valid worktree + // Expect: returns existing env, isNew = false + }); + + test('cleans up stale isolation and creates new', async () => { + // Setup: conversation with isolation_env_id pointing to non-existent path + // Expect: clears reference, creates new env + }); + + test('migrates legacy worktree_path to new model', async () => { + // Setup: conversation with worktree_path but no isolation_env_id + // Expect: creates isolation_environments record + }); + + test('creates new isolation for thread workflow', async () => { + // Setup: conversation with no isolation, no hints + // Expect: creates thread-type isolation + }); + + test('shares worktree with linked issue', async () => { + // Setup: hints with linkedIssues pointing to existing env + // Expect: reuses linked env + }); + + test('adopts skill-created worktree', async () => { + // Setup: hints with prBranch matching existing worktree + // Expect: creates env record pointing to existing path + }); +}); +``` + +**Verify**: `bun test src/orchestrator/orchestrator.test.ts` + +--- + +### Task 10: Refactor GitHub adapter + +**Why**: Remove worktree creation logic, keep close event trigger, add IsolationHints. + +**Mirror**: `src/adapters/github.ts:629-753` (code to remove) + +**Do**: +Update `src/adapters/github.ts`: + +1. Remove worktree creation in `handleBotMention` (lines 629-753): + - Remove `if (!existingIsolation)` block that creates worktrees + - Keep the conversation lookup and context building + +2. Add IsolationHints building: +```typescript +import { IsolationHints } from '../types'; +import { getLinkedIssueNumbers } from '../utils/github-graphql'; + +// In handleBotMention, before calling handleMessage: +const hints: IsolationHints = { + workflowType: isPR ? 'pr' : 'issue', + workflowId: String(number), + prBranch: isPR ? await this.getPRHeadBranch(owner, repo, number) : undefined, + prSha: isPR ? await this.getPRHeadSha(owner, repo, number) : undefined, + linkedIssues: await getLinkedIssueNumbers(owner, repo, number), +}; + +await handleMessage( + this, + conversationId, + finalMessage, + contextToAppend, + undefined, // threadContext + undefined, // parentConversationId + hints // NEW parameter +); +``` + +3. Update close event handler to call cleanup service: +```typescript +import { onConversationClosed } from '../services/cleanup-service'; + +// In handleCloseEvent (around line 591-596): +if (isCloseEvent) { + await onConversationClosed('github', conversationId); + return; +} +``` + +4. Remove: + - `cleanupPRWorktree` function (moved to cleanup-service) + - Worktree UX messages (now in orchestrator) + - Database updates for isolation (now in orchestrator) + +**Don't**: +- Don't remove webhook signature verification +- Don't remove event parsing +- Don't remove @mention detection +- Don't remove context building + +**Verify**: `bun test src/adapters/github.test.ts` + +--- + +### Task 11: Add /worktree link command + +**Why**: Allow manual cross-platform worktree sharing. + +**Mirror**: `src/handlers/command-handler.ts:922-1134` (worktree commands) + +**Do**: +Add to `src/handlers/command-handler.ts` in the worktree switch block: +```typescript +case 'link': { + const target = args[1]; + if (!target) { + return { + success: false, + message: 'Usage: /worktree link \n\nExamples:\n /worktree link issue-42\n /worktree link pr-99\n /worktree link thread-abc123' + }; + } + + // Parse target: "issue-42", "pr-99", "thread-abc123", "task-my-feature" + const match = /^(issue|pr|thread|task)-(.+)$/.exec(target); + if (!match) { + return { + success: false, + message: 'Invalid format. Use: issue-42, pr-99, thread-xxx, or task-name' + }; + } + + const [, workflowType, workflowId] = match; + + // Import isolation-environments db + const isolationEnvDb = await import('../db/isolation-environments'); + + const targetEnv = await isolationEnvDb.findByWorkflow( + conversation.codebase_id, + workflowType, + workflowId + ); + + if (!targetEnv) { + return { + success: false, + message: `No worktree found for ${target}` + }; + } + + // Update conversation to use this environment + await db.updateConversation(conversation.id, { + isolation_env_id: targetEnv.id, + worktree_path: targetEnv.working_path, + isolation_provider: targetEnv.provider, + cwd: targetEnv.working_path, + }); + + return { + success: true, + message: `Linked to worktree \`${targetEnv.branch_name}\`\n\nPath: ${shortenPath(targetEnv.working_path, mainPath)}`, + modified: true, + }; +} +``` + +Update help text to include link command: +``` +Worktrees: + /worktree create - Create isolated worktree + /worktree list - Show worktrees for this repo + /worktree remove [--force] - Remove current worktree + /worktree link - Link to existing worktree + /worktree orphans - Show all git worktrees +``` + +**Verify**: `bun test src/handlers/command-handler.test.ts` + +--- + +### Task 12: Update imports and wire everything together + +**Why**: Ensure all new modules are properly imported and accessible. + +**Do**: +1. Update `src/index.ts` to import cleanup service (for future scheduler) +2. Export new types from `src/types/index.ts` +3. Ensure `src/db/isolation-environments.ts` is importable + +**Verify**: `bun run type-check && bun run build` + +--- + +## Validation Strategy + +### Automated Checks +- [ ] `bun run type-check` - Types valid +- [ ] `bun run lint` - No lint errors +- [ ] `bun run format:check` - Formatting correct +- [ ] `bun test` - All tests pass +- [ ] `bun run build` - Build succeeds + +### New Tests to Write +| Test File | Test Case | What It Validates | +|-----------|-----------|-------------------| +| `isolation-environments.test.ts` | CRUD operations | Database layer works | +| `cleanup-service.test.ts` | hasUncommittedChanges, isBranchMerged | Git checks work | +| `orchestrator.test.ts` | validateAndResolveIsolation | Isolation logic | +| `github.test.ts` | IsolationHints building | Adapter provides hints | +| `command-handler.test.ts` | /worktree link | Manual linking works | + +### Manual/E2E Validation +```bash +# 1. Run migration +psql $DATABASE_URL < migrations/006_isolation_environments.sql + +# 2. Start app +docker-compose --profile with-db up -d postgres +bun run dev + +# 3. Test via test adapter +curl -X POST http://localhost:3000/test/message \ + -H "Content-Type: application/json" \ + -d '{"conversationId":"test-isolation","message":"hello"}' + +# 4. Verify worktree created +curl http://localhost:3000/test/messages/test-isolation | jq + +# 5. Check database +psql $DATABASE_URL -c "SELECT * FROM remote_agent_isolation_environments;" +``` + +### Edge Cases +- [ ] Stale worktree path (path deleted on disk) - should clean up and create new +- [ ] Legacy conversation with worktree_path but no isolation_env_id - should migrate +- [ ] GitHub PR linked to existing issue worktree - should share +- [ ] Concurrent requests to same conversation - lock manager handles +- [ ] Empty codebase (no cwd) - should skip isolation + +### Regression Check +- [ ] Existing GitHub workflow still works (webhook → isolation → AI response) +- [ ] `/worktree create` manual command still works +- [ ] `/status` shows worktree info correctly +- [ ] Session resume across messages still works + +--- + +## Risks + +1. **Migration complexity**: TEXT → UUID migration requires application-level backfill. Mitigated by keeping legacy columns during transition. + +2. **Breaking GitHub adapter**: Large refactor. Mitigated by extensive tests and keeping close event trigger. + +3. **UX message duplication**: Risk of both adapter and orchestrator sending messages. Mitigated by removing all UX messages from adapter. + +4. **Performance**: Creating worktree on every message. Research shows 0.1s overhead is acceptable. diff --git a/.gitignore b/.gitignore index d9d82f3b..254f53c8 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,7 @@ api_sessions/ .agents/rca-reports/ .claude/PRPs/features .agents/pr-reviews +.agents/implementation-reports # Local workspace workspace/ diff --git a/docs/architecture.md b/docs/architecture.md index b7d8e0b3..0db1c399 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -476,17 +476,17 @@ export interface IIsolationProvider { ```typescript interface IsolationRequest { codebaseId: string; - canonicalRepoPath: string; // Main repo path, never a worktree + canonicalRepoPath: string; // Main repo path, never a worktree workflowType: 'issue' | 'pr' | 'review' | 'thread' | 'task'; - identifier: string; // "42", "feature-auth", etc. - prBranch?: string; // For PR adoption - prSha?: string; // For reproducible PR reviews + identifier: string; // "42", "feature-auth", etc. + prBranch?: string; // For PR adoption + prSha?: string; // For reproducible PR reviews } interface IsolatedEnvironment { - id: string; // Worktree path (for worktree provider) + id: string; // Worktree path (for worktree provider) provider: 'worktree' | 'container' | 'vm' | 'remote'; - workingPath: string; // Where AI should work + workingPath: string; // Where AI should work branchName?: string; status: 'active' | 'suspended' | 'destroyed'; createdAt: Date; @@ -517,13 +517,13 @@ export class WorktreeProvider implements IIsolationProvider { ### Branch Naming Convention -| Workflow | Identifier | Generated Branch | -|----------|------------|------------------| -| issue | `"42"` | `issue-42` | -| pr | `"123"` | `pr-123` | -| pr + SHA | `"123"` | `pr-123-review` | -| task | `"my-feature"` | `task-my-feature` | -| thread | `"C123:ts.123"` | `thread-a1b2c3d4` (8-char hash) | +| Workflow | Identifier | Generated Branch | +| -------- | --------------- | ------------------------------- | +| issue | `"42"` | `issue-42` | +| pr | `"123"` | `pr-123` | +| pr + SHA | `"123"` | `pr-123-review` | +| task | `"my-feature"` | `task-my-feature` | +| thread | `"C123:ts.123"` | `thread-a1b2c3d4` (8-char hash) | ### Storage Location @@ -533,6 +533,7 @@ DOCKER: /workspace/worktrees/// ← FIXED, no override ``` **Logic in `getWorktreeBase()`:** + 1. Docker detected? → `/workspace/worktrees` (always, no override) 2. `WORKTREE_BASE` set? → use it (local only) 3. Default → `~/tmp/worktrees` @@ -602,6 +603,7 @@ remote_agent_conversations ``` **Lookup pattern:** + ```typescript const envId = conversation.isolation_env_id ?? conversation.worktree_path; ``` diff --git a/docs/worktree-orchestration-research.md b/docs/worktree-orchestration-research.md index 4cfb8433..b93abd78 100644 --- a/docs/worktree-orchestration-research.md +++ b/docs/worktree-orchestration-research.md @@ -1,162 +1,532 @@ # Worktree Orchestration Research > **Status**: Research Complete - Ready for Implementation (2025-12-17) -> **Context**: Phase 3 of Isolation Provider Migration - extending auto-isolation to Slack/Discord/Telegram adapters +> **Context**: Unified isolation architecture across all platforms with work-centric data model ## Executive Summary -This document captures the design decisions for auto-isolation across all platform adapters. Key decisions: +This document captures the design decisions for a **unified isolation architecture** that centralizes all worktree management in the orchestrator. Key decisions: -| Decision | Choice | Rationale | -|----------|--------|-----------| -| **Isolation trigger** | Auto on every @mention (for AI interactions) | Simplicity > efficiency; worktrees are cheap (0.1s, 2.5MB) | -| **Threading model** | ALL bot responses → thread | Never pollute main channel; consistent UX | -| **Cleanup strategy** | Git-based merge detection + background scheduler | No "close events" needed; git is source of truth | -| **Limits** | 25 worktrees/codebase (configurable), auto-cleanup merged | Mental model limit, not resource constraint | -| **UX message** | Verbose (branch name + instructions) | Helpful for new users | -| **Removal** | `git worktree remove`, keep branch | Git is source of truth; branch preserved for restore | +| Decision | Choice | Rationale | +| -------------------------- | --------------------------------------------- | -------------------------------------------- | +| **Isolation authority** | Orchestrator only | Single source of truth; adapters are thin | +| **Data model** | Work-centric (`isolation_environments` table) | Enables cross-platform sharing | +| **Isolation trigger** | Auto on every @mention | Simplicity > efficiency; worktrees are cheap | +| **Threading model** | ALL bot responses → thread | Never pollute main channel | +| **Cleanup service** | Separate service, git-first | Clean separation; git is source of truth | +| **Cross-platform linking** | Metadata + `/worktree link` fallback | Automated discovery with manual override | +| **Limits** | 25 worktrees/codebase (configurable) | Mental model limit, not resource constraint | **Implementation phases**: -1. **3A**: Force-thread response model (Slack/Discord `createThread()`) -2. **3B**: Auto-isolation in orchestrator (centralized logic) -3. **3C**: Git-based cleanup scheduler -4. **3D**: Limits and user feedback -5. **Phase 4**: Drop `worktree_path` column + +1. **Phase 2.5**: Unified Isolation Architecture (schema + centralization + auto-isolation) ← **DO FIRST** +2. **Phase 3A**: Force-thread response model (Slack/Discord) +3. **Phase 3C**: Git-based cleanup scheduler (starts the scheduler from Phase 2.5) +4. **Phase 3D**: Limits and user feedback +5. **Phase 4**: Schema cleanup (drop legacy columns) + +> Note: Original Phase 3B ("Auto-Isolation in Orchestrator") was merged into Phase 2.5. + +--- ## Problem Statement -GitHub adapter has working auto-isolation: worktrees are created automatically on @mention and cleaned up on issue/PR close. The other adapters (Slack, Discord, Telegram) lack this automation, requiring manual `/worktree create` commands. +The current architecture has **fragmented isolation logic**: -**Goal**: Make isolation effortless across all platforms while keeping resource usage reasonable. +- GitHub adapter has full auto-isolation (create, cleanup, UX messages) +- Orchestrator has partial logic (cwd validation) +- Other adapters (Slack, Discord, Telegram) have no automation -## Current Architecture +This causes: -### Platform Conversation Models +1. Code duplication and inconsistent behavior +2. Double UX messages risk when both adapter and orchestrator act +3. No ability to share worktrees across platforms +4. Difficult to maintain and extend -| Platform | Conversation ID | Threading Model | Natural Close Event | -|----------|-----------------|-----------------|---------------------| -| GitHub | `owner/repo#42` | Issue/PR as conversation | Issue/PR close/merge | -| Telegram | `chat_id` (number) | Single persistent chat | None | -| Slack | `channel:thread_ts` | Thread per conversation | None | -| Discord | `channel_id` | Thread = separate channel | None | +**Goal**: Centralize ALL isolation logic in the orchestrator with a work-centric data model. -### How GitHub Auto-Isolation Works +--- + +## Architecture Decision: Single Isolation Authority + +### Current State (Problematic) ``` -@bot mention detected - │ - ▼ -┌─────────────────────────────────────┐ -│ Check: conversation.isolation_env_id │ -│ ?? conversation.worktree_path │ -└──────────────┬──────────────────────┘ - │ - exists? │ - ┌──YES─────┴─────NO────┐ - ▼ ▼ - REUSE CREATE via - existing IsolationProvider - │ │ - └───────┬───────────┘ - ▼ - Work in isolated - worktree - │ - ▼ - Issue/PR closed → cleanupWorktree() +GitHub Adapter Orchestrator +├── Auto-isolation logic + ├── CWD validation +├── Worktree creation ├── (no creation) +├── UX messages └── (no UX messages) +├── Cleanup on close +└── 800+ lines + +Slack/Discord/Telegram: No isolation logic ``` -**Key insight**: GitHub's lifecycle is clean because issues/PRs have explicit close events. - -## Design Decision: Isolation Trigger - -### Options Considered - -| Option | Description | Pros | Cons | -|--------|-------------|------|------| -| **A) Auto on @mention** | Create worktree immediately | Effortless, consistent, safe | Resource-heavy for read-only queries | -| **B) Lazy (on first write)** | Detect modifying commands | Efficient | Hard to detect intent reliably | -| **C) User explicit** | `/worktree create` | Full control | Users forget, chaos ensues | - -### Decision: Option A - Auto on @mention - -**Rationale**: -1. **Simplicity**: Same behavior as GitHub, easy mental model -2. **Safety**: Users can't accidentally modify main repo -3. **Future-proofing**: Workflow engine (planned) will have better control anyway -4. **Resource cost is acceptable**: Worktrees are cheap (~symlinks + index), not full clones - -**The resource concern is overstated**: -- Git worktrees share object database with main repo -- Only unique files are duplicated (working tree) -- A 500MB repo might only add 50MB per worktree -- Disk space is cheap; developer confusion is expensive - -## Design Decision: Cleanup Strategy - -### The Key Insight: Git as Source of Truth - -Unlike GitHub webhooks, Slack/Discord/Telegram don't have "close" events. But we don't need them! - -**Git already knows**: -- Is this branch merged to main? → `git branch --merged main` -- Does this worktree have uncommitted changes? → `git status` -- When was last commit? → `git log -1 --format=%ci` - -### Proposed Cleanup Flow +### Target State (Unified) ``` -┌─────────────────────────────────────────────────────────────┐ -│ CLEANUP TRIGGERS │ -├─────────────────────────────────────────────────────────────┤ -│ │ -│ 1. USER EXPLICIT │ -│ /worktree remove [name] │ -│ /worktree done (marks current as done) │ -│ │ -│ 2. PR MERGED (detected via git) │ -│ Periodic check: is worktree branch merged to main? │ -│ If yes → auto-cleanup candidate │ -│ │ -│ 3. STALE (time-based) │ -│ No messages in conversation for N days │ -│ AND no commits in worktree for N days │ -│ → Mark as stale, candidate for removal │ -│ │ -│ 4. LIMIT REACHED │ -│ User hits max worktrees (default: 25) │ -│ → Must cleanup before creating new │ -│ │ -└─────────────────────────────────────────────────────────────┘ +ALL Adapters (Thin) Orchestrator (Authority) +├── Parse platform events ├── ALL isolation creation +├── Detect @mentions ├── ALL UX messages +├── Build context ├── CWD validation +├── Provide IsolationHints ├── Reuse/sharing logic +└── Trigger cleanup events └── Cross-platform linking + + Cleanup Service (Separate) + ├── Git-first checks + ├── Scheduled cleanup + └── On-demand removal ``` -### Branch Merge Detection +### Benefits + +1. **DRY**: Single implementation for all platforms +2. **Consistency**: Same behavior everywhere +3. **Testability**: Isolation logic isolated (pun intended) +4. **Extensibility**: New platforms just need to call orchestrator +5. **Cross-platform**: Work can span multiple conversations + +--- + +## Data Model: Work-Centric Isolation + +### Current Schema (Platform-Centric) + +```sql +conversations +├── worktree_path -- Path as identifier (legacy) +├── isolation_env_id -- Also path (redundant) +└── isolation_provider -- Provider type +``` + +**Problems**: + +- Worktree identified by filesystem path (implementation detail) +- No independent lifecycle from conversations +- Can't easily share across conversations +- Dual-column confusion + +### New Schema (Work-Centric) + +```sql +-- Isolated work environments (independent entities) +CREATE TABLE remote_agent_isolation_environments ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + codebase_id UUID NOT NULL REFERENCES remote_agent_codebases(id), + + -- Workflow identification (what work this is for) + workflow_type TEXT NOT NULL, -- 'issue', 'pr', 'thread', 'task' + workflow_id TEXT NOT NULL, -- '42', 'pr-99', 'thread-abc123' + + -- Implementation details + provider TEXT NOT NULL DEFAULT 'worktree', + working_path TEXT NOT NULL, -- Actual filesystem path + branch_name TEXT NOT NULL, -- Git branch name + + -- Lifecycle + status TEXT NOT NULL DEFAULT 'active', -- 'active', 'destroyed' + created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), + created_by_platform TEXT, -- 'github', 'slack', etc. + + -- Cross-reference metadata (for linking) + metadata JSONB DEFAULT '{}', + + CONSTRAINT unique_workflow UNIQUE (codebase_id, workflow_type, workflow_id) +); + +-- Index for common queries +CREATE INDEX idx_isolation_env_codebase ON remote_agent_isolation_environments(codebase_id); +CREATE INDEX idx_isolation_env_status ON remote_agent_isolation_environments(status); +``` + +### Migration Strategy (TEXT → UUID) + +**Important**: The current schema has `isolation_env_id` as TEXT (storing worktree paths). +The new schema uses UUID FK. This requires a careful migration: + +```sql +-- Step 1: Rename old column +ALTER TABLE remote_agent_conversations + RENAME COLUMN isolation_env_id TO isolation_env_id_legacy; + +-- Step 2: Add new UUID column +ALTER TABLE remote_agent_conversations + ADD COLUMN isolation_env_id UUID REFERENCES remote_agent_isolation_environments(id); + +-- Step 3: Backfill (done in application code, not SQL) +-- For each conversation with isolation_env_id_legacy: +-- 1. Create isolation_environments record for the path +-- 2. Set new isolation_env_id to the created record's UUID + +-- Step 4 (Phase 4): Drop legacy columns +-- ALTER TABLE remote_agent_conversations DROP COLUMN isolation_env_id_legacy; +-- ALTER TABLE remote_agent_conversations DROP COLUMN worktree_path; +-- ALTER TABLE remote_agent_conversations DROP COLUMN isolation_provider; +``` + +**Backfill logic** (application code): ```typescript -// Check if worktree's branch is merged to main -async function isBranchMerged(worktreePath: string, mainBranch: string = 'main'): Promise { - const repoPath = await getCanonicalRepoPath(worktreePath); - const worktrees = await listWorktrees(repoPath); - const wt = worktrees.find(w => w.path === worktreePath); +async function backfillIsolationEnvironments(): Promise { + const conversations = await db.query(` + SELECT * FROM remote_agent_conversations + WHERE isolation_env_id_legacy IS NOT NULL + AND isolation_env_id IS NULL + `); - if (!wt?.branch) return false; + for (const conv of conversations.rows) { + const legacyPath = conv.isolation_env_id_legacy; + if (!(await worktreeExists(legacyPath))) { + // Stale reference, skip + continue; + } - // Check if branch is in merged list - const { stdout } = await execFileAsync('git', [ - '-C', repoPath, - 'branch', '--merged', mainBranch - ]); + // Infer workflow type from path or conversation context + const workflowType = inferWorkflowType(conv, legacyPath); + const workflowId = inferWorkflowId(conv, legacyPath); - return stdout.includes(wt.branch); + const env = await isolationEnvDb.createIsolationEnvironment({ + codebase_id: conv.codebase_id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: legacyPath, + branch_name: await getBranchName(legacyPath), + created_by_platform: conv.platform_type, + metadata: { migrated: true }, + }); + + await db.updateConversation(conv.id, { isolation_env_id: env.id }); + } } ``` -**This enables cross-platform cleanup**: -1. User creates worktree in Slack thread -2. User works, commits, creates PR (via AI or manually) -3. PR gets merged on GitHub -4. Periodic cleanup job detects branch is merged -5. Auto-cleanup the worktree (no Slack "close event" needed!) +### Metadata Schema for Cross-Platform Linking + +```typescript +interface IsolationEnvironmentMetadata { + // Auto-populated from context + related_issues?: number[]; // GitHub issues this work relates to + related_prs?: number[]; // GitHub PRs + + // For adoption tracking + adopted?: boolean; + adopted_from?: 'skill' | 'branch' | 'path'; + + // For future AI-assisted discovery + keywords?: string[]; // Extracted from commit messages + ai_suggested_links?: Array<{ + type: 'issue' | 'pr' | 'conversation'; + id: string; + confidence: number; + reason: string; + }>; +} +``` + +### Why This Model is Better + +1. **Independent lifecycle**: Environment exists even if all conversations close +2. **Natural sharing**: Multiple conversations can reference same environment +3. **Query-friendly**: "Find all conversations for this worktree" is trivial +4. **Cross-platform**: Work spans platforms via shared environment reference +5. **Future-proof**: Metadata enables smart linking without schema changes + +--- + +## API Design: IsolationHints Parameter + +The orchestrator needs platform-specific context to make good isolation decisions. Rather than having the orchestrator know about each platform's internals, adapters provide **hints**. + +### Interface + +```typescript +interface IsolationHints { + // Workflow identification (adapter knows this) + workflowType?: 'issue' | 'pr' | 'review' | 'thread' | 'task'; + workflowId?: string; + + // PR-specific (for reproducible reviews) + prBranch?: string; + prSha?: string; + + // Cross-reference hints (for linking) + linkedIssues?: number[]; // From "Fixes #X" parsing + linkedPRs?: number[]; + + // Adoption hints + suggestedBranch?: string; // "Maybe reuse branch X" +} + +// Updated handleMessage signature +export async function handleMessage( + platform: IPlatformAdapter, + conversationId: string, + message: string, + context?: string, // Human-readable for AI + isolationHints?: IsolationHints // Machine-readable for orchestrator +): Promise; +``` + +### How Adapters Provide Hints + +**GitHub Adapter**: + +```typescript +const hints: IsolationHints = { + workflowType: isPR ? 'pr' : 'issue', + workflowId: String(number), + prBranch: prHeadBranch, // From GitHub API + prSha: prHeadSha, // From GitHub API + linkedIssues: await getLinkedIssueNumbers(owner, repo, number), +}; +await handleMessage(this, conversationId, message, contextString, hints); +``` + +**Slack/Discord/Telegram Adapters**: + +```typescript +// No special hints needed - orchestrator infers 'thread' type +await handleMessage(this, conversationId, message, undefined, undefined); +``` + +### How Orchestrator Uses Hints + +```typescript +async function resolveIsolation( + conversation: Conversation, + codebase: Codebase, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise { + // 1. Prefer explicit hints from adapter + const workflowType = hints?.workflowType ?? 'thread'; + const workflowId = hints?.workflowId ?? conversationId; + + // 2. Check for existing environment (reuse) + const existing = await isolationEnvDb.findByWorkflow(codebase.id, workflowType, workflowId); + if (existing && (await validatePath(existing.working_path))) { + return existing; + } + + // 3. Check linked issues for sharing (cross-conversation) + if (hints?.linkedIssues?.length) { + for (const issueNum of hints.linkedIssues) { + const linkedEnv = await isolationEnvDb.findByWorkflow(codebase.id, 'issue', String(issueNum)); + if (linkedEnv && (await validatePath(linkedEnv.working_path))) { + return linkedEnv; // Share with linked issue + } + } + } + + // 4. Try PR branch adoption (skill symbiosis) + if (hints?.prBranch) { + const adoptedPath = await findWorktreeByBranch(codebase.default_cwd, hints.prBranch); + if (adoptedPath) { + return await adoptExistingWorktree(adoptedPath, codebase, workflowType, workflowId); + } + } + + // 5. Create new + return await createNewEnvironment(codebase, workflowType, workflowId, hints); +} +``` + +--- + +## CWD Validation (Critical Safety Check) + +Before using any worktree path, validate it exists on disk. This prevents errors from stale database references. + +### Edge Case: Canonical Repo Path + +**Important**: `codebase.default_cwd` might itself be a worktree if someone cloned into a worktree path. The isolation provider needs the **canonical** (main) repo path to create new worktrees. Use `getCanonicalRepoPath()` to resolve this: + +```typescript +const canonicalPath = await getCanonicalRepoPath(codebase.default_cwd); +// canonicalPath is always the main repo, even if default_cwd is a worktree +``` + +### Validation and Resolution Logic + +```typescript +async function validateAndResolveIsolation( + conversation: Conversation, + codebase: Codebase | null, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise<{ cwd: string; env: IsolationEnvironmentRow | null; isNew: boolean }> { + // 1. Check existing isolation reference (new UUID model) + if (conversation.isolation_env_id) { + const env = await isolationEnvDb.getById(conversation.isolation_env_id); + + if (env && (await worktreeExists(env.working_path))) { + // Valid - use it + return { cwd: env.working_path, env, isNew: false }; + } + + // Stale reference - clean up + console.warn(`[Orchestrator] Stale isolation: ${conversation.isolation_env_id}`); + await db.updateConversation(conversation.id, { isolation_env_id: null }); + + if (env) { + await isolationEnvDb.updateStatus(env.id, 'destroyed'); + } + } + + // 2. Legacy fallback (worktree_path or isolation_env_id_legacy without new UUID) + const legacyPath = conversation.worktree_path ?? conversation.isolation_env_id_legacy; + if (legacyPath && (await worktreeExists(legacyPath))) { + // Migrate to new model on-the-fly + const env = await migrateToIsolationEnvironment(conversation, codebase, legacyPath); + return { cwd: legacyPath, env, isNew: false }; + } + + // 3. No valid isolation - check if we should create + if (!codebase) { + return { cwd: conversation.cwd ?? '/workspace', env: null, isNew: false }; + } + + // 4. Create new isolation + const env = await resolveIsolation(conversation, codebase, platform, conversationId, hints); + if (env) { + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + cwd: env.working_path, + }); + return { cwd: env.working_path, env, isNew: true }; + } + + return { cwd: codebase.default_cwd, env: null, isNew: false }; +} + +/** + * Migrate a legacy worktree_path to the new isolation_environments model. + * Creates an environment record for the existing worktree. + */ +async function migrateToIsolationEnvironment( + conversation: Conversation, + codebase: Codebase | null, + legacyPath: string +): Promise { + if (!codebase) return null; + + try { + // Infer workflow type from conversation context + const { workflowType, workflowId } = inferWorkflowFromConversation(conversation, legacyPath); + + // Get branch name from git + const branchName = await getBranchNameFromWorktree(legacyPath); + + const env = await isolationEnvDb.createIsolationEnvironment({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: legacyPath, + branch_name: branchName, + created_by_platform: conversation.platform_type, + metadata: { migrated: true, migrated_at: new Date().toISOString() }, + }); + + // Update conversation to use new model + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + }); + + console.log(`[Orchestrator] Migrated legacy worktree to environment: ${env.id}`); + return env; + } catch (error) { + console.error('[Orchestrator] Failed to migrate legacy worktree:', error); + return null; + } +} + +/** + * Infer workflow type and ID from conversation context and path. + */ +function inferWorkflowFromConversation( + conversation: Conversation, + legacyPath: string +): { workflowType: string; workflowId: string } { + // Try to infer from platform conversation ID + if (conversation.platform_type === 'github') { + // Format: owner/repo#42 + const match = /#(\d+)$/.exec(conversation.platform_conversation_id); + if (match) { + // Check path for pr- or issue- prefix + const isPR = legacyPath.includes('/pr-') || legacyPath.includes('-pr-'); + return { + workflowType: isPR ? 'pr' : 'issue', + workflowId: match[1], + }; + } + } + + // Default: treat as thread with conversation ID + return { + workflowType: 'thread', + workflowId: conversation.platform_conversation_id, + }; +} +``` + +--- + +## Cleanup Service Architecture + +### Design Principles + +1. **Git is source of truth**: Branch merged? Uncommitted changes? Git knows. +2. **Separate service**: Clean separation from orchestrator +3. **Multiple triggers**: Events, schedules, manual commands +4. **Non-destructive by default**: Respect uncommitted changes + +### Configuration Constants + +```typescript +// Cleanup thresholds (configurable via env vars) +const STALE_THRESHOLD_DAYS = parseInt(process.env.STALE_THRESHOLD_DAYS ?? '14'); +const MAX_WORKTREES_PER_CODEBASE = parseInt(process.env.MAX_WORKTREES_PER_CODEBASE ?? '25'); +const CLEANUP_INTERVAL_HOURS = parseInt(process.env.CLEANUP_INTERVAL_HOURS ?? '6'); +``` + +### Service Interface + +```typescript +// src/services/cleanup-service.ts + +class CleanupService { + /** + * Called when a platform conversation is closed (e.g., GitHub issue/PR closed). + * Uses platform identifiers, not internal UUIDs. + */ + async onConversationClosed(platformType: string, platformConversationId: string): Promise; + + /** + * Scheduled cleanup - runs every CLEANUP_INTERVAL_HOURS. + * Removes merged branches, stale environments. + */ + async runScheduledCleanup(): Promise; + + /** + * Remove specific environment (from /worktree remove command). + */ + async removeEnvironment(envId: string, options?: { force?: boolean }): Promise; + + /** + * Clean up to make room when limit reached. + * Returns number of environments cleaned. + */ + async cleanupToMakeRoom(codebaseId: string): Promise; + + // Git-first checks + private async isBranchMerged(env: IsolationEnvironment, mainBranch?: string): Promise; + private async hasUncommittedChanges(workingPath: string): Promise; + private async getLastCommitDate(workingPath: string): Promise; +} +``` ### Cleanup State Machine @@ -170,594 +540,67 @@ async function isBranchMerged(worktreePath: string, mainBranch: string = 'main') │ │ │ ▼ ▼ ▼ ┌─────────┐ ┌─────────┐ ┌─────────┐ - │ MERGED │ │ STALE │ │ ORPHAN │ - │ │ │ (14d)* │ │ (no DB) │ + │ MERGED │ │ STALE │ │ ORPHAN │ + │(branch) │ │ (14d)* │ │ (no DB) │ └────┬────┘ └────┬────┘ └────┬────┘ │ │ │ - ▼ ▼ ▼ - ┌─────────────────────────────────────────┐ - │ REMOVE WORKTREE │ - │ git worktree remove │ - │ (branch kept in git, can restore) │ - └─────────────────────────────────────────┘ + │ Has uncommitted changes? │ + │ ┌──────┴──────┐ │ + │ YES NO │ + │ │ │ │ + │ ▼ ▼ │ + │ ┌─────────┐ ┌─────────┐ │ + │ │ SKIP │ │ REMOVE │◀──┘ + │ │ (warn) │ │ │ + │ └─────────┘ └────┬────┘ + │ │ + └────────────────────────┴─────▶ git worktree remove + (branch kept in git) -* Telegram worktrees are NEVER marked stale (no auto-cleanup) -* Stale = no conversation messages AND no git commits for 14 days (BOTH required) -* Merged branches = IMMEDIATE cleanup (no waiting period) +* STALE_THRESHOLD_DAYS = 14 (configurable) +* Telegram worktrees are NEVER marked stale (persistent workspaces) +* Merged branches = immediate cleanup candidate ``` -**Platform-specific cleanup behavior**: - -| Platform | Merged Branch | Stale (14d) | Manual | -|----------|---------------|-------------|--------| -| GitHub | Auto-remove | Auto-remove | Yes | -| Slack | Auto-remove | Auto-remove | Yes | -| Discord | Auto-remove | Auto-remove | Yes | -| Telegram | Auto-remove | **Never** | Yes | - -### Cleanup = Remove Worktree, Keep Branch (Git is Source of Truth) - -**Principle**: Git is the source of truth. Don't overcomplicate. - -**Cleanup simply means**: -```bash -git worktree remove /path/to/worktree -# Branch remains in repo, can restore later: -git worktree add /new/path branch-name -``` - -**No special "archive" concept**. Just: -- Remove the worktree working directory -- Branch stays in git (commits preserved) -- User can restore anytime with `/worktree create ` - -**Why this is sufficient**: -1. Git already tracks all commits (nothing lost) -2. Branch name preserved (easy to find and restore) -3. `git worktree remove` fails if uncommitted changes (safety built-in) -4. `git worktree remove` is idempotent (safe to call on non-existent worktree) -5. Simpler = fewer bugs - -## Design Decision: Limits - -### Proposed Limits - -| Limit | Value | Rationale | -|-------|-------|-----------| -| Max worktrees per codebase | **25** (configurable) | Mental model limit, not resource constraint | -| Stale threshold | 14 days | Two weeks of inactivity | -| Branch retention after removal | Forever | Git keeps branch, user can restore | -| Auto-cleanup merged | Immediate | No reason to keep merged branch worktrees | - -**Configuration**: -```env -MAX_WORKTREES_PER_CODEBASE=25 # Default, can override for power users -``` - -**Rationale for limit**: -- Not a resource constraint (worktrees are cheap: 0.1s, 2.5MB) -- Mental model limit for users: 25+ parallel tasks becomes hard to track -- Prevents runaway accumulation for abandoned conversations -- **This is POC/v1** - conservative limit to test performance at scale -- Can increase later once we verify no issues with many worktrees -- Power users can increase via env var - -### What Happens at Limit +### Cleanup Flow ``` -User: @bot help me with a new feature - -Bot: You have 10 active worktrees for this codebase. - -📊 Worktree Status: -• 3 merged (can auto-remove) -• 2 stale (no activity in 14+ days) -• 5 active - -Options: -• `/worktree cleanup merged` - Remove merged worktrees -• `/worktree cleanup stale` - Remove stale worktrees -• `/worktree list` - See all worktrees -• `/worktree remove ` - Remove specific worktree +┌─────────────────────────────────────────────────────────────────────────┐ +│ CLEANUP SERVICE │ +├─────────────────────────────────────────────────────────────────────────┤ +│ │ +│ TRIGGERS: │ +│ ├── GitHub adapter: onConversationClosed('github', 'owner/repo#42') │ +│ ├── Scheduler: runScheduledCleanup() every 6 hours │ +│ ├── User command: /worktree remove [name] │ +│ └── Limit reached: cleanupToMakeRoom(codebaseId) │ +│ │ +│ GIT-FIRST CHECKS: │ +│ ├── isBranchMerged(env) → git branch --merged main │ +│ ├── hasUncommittedChanges(path) → git status --porcelain │ +│ └── getLastCommitDate(path) → git log -1 --format=%ci │ +│ │ +│ SAFETY: │ +│ ├── Check for other conversations using same environment │ +│ ├── Respect uncommitted changes (no force by default) │ +│ └── Log and continue on errors (don't crash cleanup cycle) │ +│ │ +└─────────────────────────────────────────────────────────────────────────┘ ``` -## Future: Workflow Engine Integration +### Who Calls What -### Current Command System Limitations +| Trigger | Caller | Method | +| ------------------ | --------------------- | ------------------------------------------------- | +| GitHub close event | GitHub adapter | `onConversationClosed('github', 'owner/repo#42')` | +| Scheduled (6h) | Scheduler in index.ts | `runScheduledCleanup()` | +| `/worktree remove` | Command handler | `removeEnvironment(envId)` | +| Hit limit | Orchestrator | `cleanupToMakeRoom(codebaseId)` | -Today, commands are simple markdown files executed via `/command-invoke`: -- No metadata about read-only vs write operations -- No control over which tools AI can use -- Router is just another command template +### Uncommitted Changes UX -### Planned Workflow Engine +When cleanup is blocked by uncommitted changes: -The command handler will evolve into a workflow engine where: - -```typescript -interface WorkflowCommand { - name: string; - description: string; - prompt: string; - - // NEW: Metadata for orchestration - metadata: { - type: 'read' | 'write' | 'mixed'; - requiresIsolation: boolean; - allowedTools?: string[]; // Restrict AI tool access - timeout?: number; - }; -} -``` - -**How this helps isolation**: -1. Router receives natural language: "explain the login flow" -2. Router routes to `explain` workflow (type: 'read') -3. Orchestrator sees `requiresIsolation: false` -4. AI runs on main repo (no worktree needed) - -**vs:** -1. Router receives: "fix the login bug" -2. Router routes to `fix-issue` workflow (type: 'write') -3. Orchestrator sees `requiresIsolation: true` -4. Worktree created automatically - -### Router as Gatekeeper - -``` -User Message - │ - ▼ -┌─────────────────────────────────────┐ -│ ROUTER │ -│ (LLM-powered intent detection) │ -│ │ -│ Analyzes message, determines: │ -│ • Which workflow to invoke │ -│ • Read-only vs write operation │ -│ • Required isolation level │ -└──────────────┬──────────────────────┘ - │ - ▼ -┌─────────────────────────────────────┐ -│ WORKFLOW ENGINE │ -│ │ -│ Based on workflow metadata: │ -│ • Create/reuse worktree if needed │ -│ • Configure AI tool access │ -│ • Execute workflow prompt │ -│ • Track state for multi-step flows │ -└─────────────────────────────────────┘ -``` - -**Key insight**: The router's classification becomes the source of truth for whether isolation is needed. This moves the complexity from "detect file modifications" to "classify intent" - which LLMs are good at. - -## Implementation Phases - -### Phase 3A: Force-Thread Response Model - -**Scope**: Bot ALWAYS responds in threads, never in main channel - -**Changes needed**: - -1. **Add `createThread()` to adapter interfaces** (`src/types/index.ts`): -```typescript -interface IPlatformAdapter { - // ... existing ... - createThread?(channelId: string, initialMessage: string, parentTs?: string): Promise; -} -``` - -2. **Implement `createThread()` in Slack adapter** (`src/adapters/slack.ts`): -```typescript -async createThread(channelId: string, initialMessage: string, parentTs?: string): Promise { - const result = await this.app.client.chat.postMessage({ - channel: channelId, - thread_ts: parentTs, // Reply to the @mention message - text: initialMessage, - }); - return `${channelId}:${result.ts}`; // Thread conversation ID -} -``` - -3. **Implement `createThread()` in Discord adapter** (`src/adapters/discord.ts`): -```typescript -async createThread(channelId: string, initialMessage: string, parentMessageId?: string): Promise { - const channel = await this.client.channels.fetch(channelId); - if (!channel?.isTextBased()) throw new Error('Invalid channel'); - - const parentMessage = parentMessageId - ? await (channel as TextChannel).messages.fetch(parentMessageId) - : null; - - const thread = await parentMessage?.startThread({ - name: `Bot conversation ${Date.now()}`, - autoArchiveDuration: 1440, // 24 hours - }) ?? await (channel as TextChannel).threads.create({ - name: `Bot conversation ${Date.now()}`, - autoArchiveDuration: 1440, - }); - - await thread.send(initialMessage); - return thread.id; // Thread ID is the conversation ID -} -``` - -4. **Update message handlers in `src/index.ts`**: -```typescript -// Slack -slack.onMessage(async event => { - let conversationId = slack!.getConversationId(event); - - // If NOT in a thread, force-create one - if (!slack!.isThread(event)) { - conversationId = await slack!.createThread( - event.channel, - 'Starting work...', - event.ts // Reply to the @mention - ); - } - - // Rest of handling uses thread conversationId -}); -``` - -**Note**: Telegram doesn't have threads - it gets special handling (next phase). - -**Phase 3A Checklist**: -- [ ] Add `createThread()` method to `IPlatformAdapter` interface -- [ ] Implement `Slack.createThread()` with thread_ts handling -- [ ] Implement `Discord.createThread()` with thread creation -- [ ] Update Slack message handler to force-create thread if not in thread -- [ ] Update Discord message handler to force-create thread if not in thread -- [ ] Test: @mention in channel → response appears in new thread -- [ ] Test: @mention in existing thread → response appears in same thread -- [ ] Test: conversation ID format is consistent with DB schema - -### Phase 3B: Auto-Isolation in Orchestrator - -**Scope**: Centralized isolation logic in orchestrator - -**Changes needed**: - -1. **Add auto-isolation to `handleMessage()`** (`src/orchestrator/orchestrator.ts`): -```typescript -// After conversation lookup, before AI invocation -if (codebase && !conversation.isolation_env_id && !conversation.worktree_path) { - const env = await autoCreateIsolation(conversation, codebase, platform, conversationId); - if (env) { - await platform.sendMessage(conversationId, - `Working on **${codebase.name}** in isolated branch \`${env.branchName}\`\n(Use /repos to switch if this isn't the right codebase)` - ); - } -} -``` - -2. **New helper function**: -```typescript -async function autoCreateIsolation( - conversation: Conversation, - codebase: Codebase, - platform: IPlatformAdapter, - conversationId: string -): Promise { - try { - const provider = getIsolationProvider(); - const env = await provider.create({ - codebaseId: codebase.id, - canonicalRepoPath: codebase.default_cwd, - workflowType: 'thread', - identifier: conversationId, - description: `${platform.getPlatformType()} thread`, - }); - - await db.updateConversation(conversation.id, { - cwd: env.workingPath, - worktree_path: env.workingPath, - isolation_env_id: env.id, - isolation_provider: env.provider, - }); - - return env; - } catch (error) { - console.error('[Orchestrator] Auto-isolation failed:', error); - return null; // Non-fatal, continue without isolation - } -} -``` - -**Phase 3B Checklist**: -- [ ] Add `autoCreateIsolation()` helper function to orchestrator -- [ ] Add auto-isolation check early in `handleMessage()` (after conversation lookup) -- [ ] Send verbose UX message when worktree created -- [ ] Handle Telegram specially (no thread forcing, just auto-isolate) -- [ ] Test: First message in Slack thread → worktree created automatically -- [ ] Test: Second message in same thread → reuses existing worktree -- [ ] Test: Error in worktree creation → continues without isolation (non-fatal) -- [ ] Test: Telegram chat → worktree created, persists across sessions - -### Phase 3C: Git-Based Cleanup - -**Scope**: -1. Periodic job to check worktree branch status -2. Auto-cleanup merged branches -3. Mark stale worktrees -4. Removal/cleanup system (no archival - branches stay in git) - -**New components**: -- `src/cleanup/worktree-cleanup.ts` - cleanup logic -- `src/cleanup/scheduler.ts` - periodic job runner -- Database: Add `last_activity_at` to conversations - -**Cleanup scheduler** (`src/cleanup/scheduler.ts`): -```typescript -const CLEANUP_INTERVAL_MS = parseInt(process.env.CLEANUP_INTERVAL_HOURS ?? '6') * 60 * 60 * 1000; - -export async function startCleanupScheduler(): Promise { - // Run immediately on startup - await runCleanupCycle(); - // Then periodically - setInterval(runCleanupCycle, CLEANUP_INTERVAL_MS); -} - -async function runCleanupCycle(): Promise { - console.log('[Cleanup] Starting cleanup cycle'); - - // 1. Find and remove worktrees with merged branches (ALL platforms) - const allCodebases = await db.getAllCodebases(); - for (const codebase of allCodebases) { - const merged = await findMergedWorktrees(codebase.default_cwd); - for (const worktreePath of merged) { - await safeRemoveWorktree(worktreePath); - } - } - - // 2. Remove stale worktrees (14 days no activity) - // IMPORTANT: Skip Telegram - those are persistent workspaces - const staleConversations = await db.getStaleConversationsWithWorktrees(14); - for (const conv of staleConversations) { - if (conv.platform_type === 'telegram') { - continue; // Telegram worktrees never auto-cleanup - } - await safeRemoveWorktree(conv.isolation_env_id ?? conv.worktree_path); - } - - console.log('[Cleanup] Cleanup cycle complete'); -} -``` - -**Phase 3C Checklist**: -- [ ] Create `src/cleanup/worktree-cleanup.ts` with cleanup logic -- [ ] Create `src/cleanup/scheduler.ts` with `startCleanupScheduler()` -- [ ] Add `findMergedWorktrees()` function using `git branch --merged` -- [ ] Add `safeRemoveWorktree()` that handles errors gracefully -- [ ] Add database migration for `last_activity_at` column -- [ ] Add `db.getStaleConversationsWithWorktrees()` query -- [ ] Call `startCleanupScheduler()` from `src/index.ts` on startup -- [ ] Test: Merged branch worktree → removed immediately -- [ ] Test: Stale Slack worktree (14d) → removed -- [ ] Test: Stale Telegram worktree → NOT removed -- [ ] Test: Worktree with uncommitted changes → skipped with warning - -### Phase 3D: Limits and User Feedback - -**Scope**: -1. Enforce worktree limits -2. User-facing cleanup commands -3. Status dashboard in `/status` output - -**Phase 3D Checklist**: -- [ ] Add limit check before auto-isolation in orchestrator -- [ ] Add `/worktree cleanup merged` command -- [ ] Add `/worktree cleanup stale` command -- [ ] Update `/status` to show worktree count and status -- [ ] Test: Hit limit → user sees helpful message with options -- [ ] Test: `/worktree cleanup merged` → removes merged branch worktrees -- [ ] Test: Limit is configurable via `MAX_WORKTREES_PER_CODEBASE` - -### Phase 4: Drop `worktree_path` Column - -**Prerequisites**: -- All adapters using `isolation_env_id` -- All queries using fallback pattern verified -- Migration script to backfill `isolation_env_id` - -**Phase 4 Checklist**: -- [ ] Verify all code uses `isolation_env_id ?? worktree_path` pattern -- [ ] Create migration to backfill `isolation_env_id` from `worktree_path` -- [ ] Remove fallback pattern from all queries (use only `isolation_env_id`) -- [ ] Create migration to drop `worktree_path` column -- [ ] Test: Existing conversations with only `worktree_path` → migrated correctly -- [ ] Test: New conversations → only `isolation_env_id` populated - -## Resolved Questions - -### 1. Isolation in Orchestrator vs Adapters - -**Decision**: **Orchestrator** - centralized in `handleMessage()` - -**Rationale**: -- DRY: Single implementation for all platforms -- GitHub already has platform-specific cleanup (on close events) -- Other platforms use git-based cleanup (branch merge detection) -- Platform adapters remain "dumb" - just route messages - -**Implementation location**: Early in `src/orchestrator/orchestrator.ts:handleMessage()`, after conversation lookup but before AI invocation: - -```typescript -async function handleMessage(...) { - const conversation = await db.getOrCreateConversation(...); - - // Auto-isolation for new conversations with codebase - if (codebase && !conversation.isolation_env_id && !conversation.worktree_path) { - try { - const provider = getIsolationProvider(); - const env = await provider.create({ - codebaseId: codebase.id, - canonicalRepoPath: codebase.default_cwd, - workflowType: 'thread', - identifier: conversationId, - description: `${platform.getPlatformType()} conversation`, - }); - - await db.updateConversation(conversation.id, { - cwd: env.workingPath, - worktree_path: env.workingPath, - isolation_env_id: env.id, - isolation_provider: env.provider, - }); - - // Inform user - await platform.sendMessage(conversationId, - `Working in isolated branch \`${env.branchName}\``); - - } catch (error) { - console.error('[Orchestrator] Auto-isolation failed:', error); - // Continue without isolation - not fatal - } - } - - // ... rest of message handling -} -``` - -### 2. Telegram: One Worktree or Many? - -**Decision**: **One persistent worktree per chat per codebase** - -**Rationale**: -- Telegram has no threading concept (unlike Slack/Discord) -- Each chat is a persistent 1:1 conversation -- User expects continuity across sessions (like a workspace) -- If user wants fresh start: `/worktree remove` + next message auto-creates new -- Simpler than trying to invent "sessions" within Telegram - -**Implementation**: -- Same `workflowType: 'thread'` with `chat_id` as identifier -- Hash ensures deterministic branch name: `thread-{hash(chat_id)}` -- Auto-create on first message if codebase is configured -- Never auto-cleanup (user controls via `/worktree remove`) - -**Telegram-specific UX differences**: -- No "force into thread" logic (Telegram has no threads) -- Worktree persists forever until explicitly removed -- Cleanup scheduler skips Telegram worktrees (no staleness concept) - -### 3. Thread Context Inheritance & Threading Model - -**Key Decision**: **ALL bot responses go to threads - never pollute main channel** - -This is a fundamental architectural rule: -- **Every bot response** creates or uses a thread (no exceptions) -- When user @mentions bot in channel → bot creates thread, responds there -- When user @mentions bot in existing thread → bot responds in that thread -- Each thread gets its own isolation (worktree) for AI interactions - -**Which commands need isolation?** - -| Command Type | Thread Required? | Isolation Required? | -|-------------|------------------|---------------------| -| `/status`, `/help`, `/repos`, `/commands` | **Yes** | No | -| `/worktree list`, `/worktree orphans` | **Yes** | No | -| `/clone`, `/setcwd`, `/codebase-switch` | **Yes** | No | -| `/command-invoke *`, `/worktree create` | **Yes** | **Yes** | -| Any AI query (natural language) | **Yes** | **Yes** | - -**Rationale**: -- Main channel stays clean - all bot activity in threads -- Consistent UX - users always know where to find bot responses -- Natural isolation boundary: 1 thread = 1 worktree = 1 task -- DMs are out of scope for now (handle separately later) - -**Implementation requirements**: - -1. **Slack/Discord adapters need `createThread()` capability**: -```typescript -interface IPlatformAdapter { - // ... existing methods ... - - /** - * Create a new thread and return its conversation ID - * Used when bot is @mentioned in main channel - */ - createThread?( - channelId: string, - initialMessage: string, - parentMessageTs?: string // The message that triggered the thread - ): Promise; // Returns thread conversation ID -} -``` - -2. **Orchestrator flow**: -```typescript -// If mentioned in channel (not thread), force-create thread -if (!isThread && adapter.createThread) { - const threadId = await adapter.createThread(channelId, 'Starting work...'); - conversationId = threadId; // Use thread as conversation -} -// Now create isolation for this thread -``` - -3. **No inheritance needed** - each thread is independent: - - Thread = conversation = worktree - - Simpler model than parent/child inheritance - - User starts new thread for new task - -**Current code impact**: -- `parentConversationId` logic becomes irrelevant for isolation -- Still useful for inheriting `codebase_id` (so user doesn't re-clone) -- Remove isolation field inheritance from parent - -### 4. Cleanup Job Timing - -**Decision**: **Startup + periodic (6 hours) + on-demand at limits** - -**Implementation**: -```typescript -// src/cleanup/scheduler.ts -const CLEANUP_INTERVAL_MS = 6 * 60 * 60 * 1000; // 6 hours - -export async function startCleanupScheduler(): Promise { - // Run immediately on startup - await runCleanupCycle(); - - // Then periodically - setInterval(runCleanupCycle, CLEANUP_INTERVAL_MS); -} - -async function runCleanupCycle(): Promise { - console.log('[Cleanup] Starting cleanup cycle'); - await cleanupMergedWorktrees(); - await removeStaleWorktrees(14); // 14 days threshold - console.log('[Cleanup] Cleanup cycle complete'); -} -``` - -**On-demand check** in auto-isolation path: -```typescript -const worktreeCount = await countWorktreesForCodebase(codebase.id); -if (worktreeCount >= MAX_WORKTREES) { - // Attempt quick cleanup of merged branches - const cleaned = await cleanupMergedWorktrees(codebase.id); - if (cleaned === 0) { - // No merged branches - show limit message - await platform.sendMessage(conversationId, limitReachedMessage); - return; - } -} -``` - -### 5. Handling Uncommitted Changes - -**Decision**: **Respect git** - let git refuse, show clear error - -**Rationale**: -- Git's refusal is a safety feature, not a bug -- User should consciously decide what to do with uncommitted work -- Auto-stash risks data loss (user might forget about stash) -- Force-delete is destructive - -**Error message**: ``` Cannot remove worktree - you have uncommitted changes. @@ -767,216 +610,561 @@ Options: 3. Force remove: `/worktree remove --force` (LOSES CHANGES!) ``` -**Removal behavior**: Cleanup attempts `git worktree remove` without force. If it fails due to uncommitted changes, the worktree stays active and is logged for user attention. +--- -### 6. Auto-Isolation UX Message +## Cross-Platform Linking -**Decision**: **Verbose message with branch name and instructions** +### The Scenario -**Format**: ``` -Working on **{codebase_name}** in isolated branch `{branch_name}` -(Use /repos to switch if this isn't the right codebase) +1. User starts work in Slack → creates thread-abc123 worktree +2. User opens GitHub Issue #42 for same work +3. User wants both to share the same worktree ``` -**Example**: +### MVP Solution: Metadata + Manual Linking + +**Automatic metadata collection**: + +- When AI creates commits, extract issue/PR references from messages +- When AI creates PRs, store PR number in environment metadata +- Store keywords from conversation for future discovery + +**Manual linking fallback**: + ``` -Working on **remote-coding-agent** in isolated branch `thread-a7f3b2c1` -(Use /repos to switch if this isn't the right codebase) +/worktree link issue-42 ``` +Links current conversation's worktree to issue-42's workflow identifier. + +**Future: AI-assisted discovery**: + +- AI can search git history: `git log --all --grep="#42"` +- AI can suggest: "Found commits mentioning issue #42 on branch thread-abc123. Link?" +- User confirms, orchestrator updates metadata + +### Implementation + +```typescript +// In orchestrator, when resolving isolation: +if (hints?.linkedIssues?.length) { + for (const issueNum of hints.linkedIssues) { + const linkedEnv = await isolationEnvDb.findByWorkflow(codebase.id, 'issue', String(issueNum)); + if (linkedEnv) { + // Found! Share this environment + console.log(`[Orchestrator] Sharing worktree with linked issue #${issueNum}`); + await db.updateConversation(conversation.id, { + isolation_env_id: linkedEnv.id, + }); + return linkedEnv; + } + } +} + +// /worktree link command handler +async function handleWorktreeLink(conversation: Conversation, target: string): Promise { + // Parse target: "issue-42", "pr-99", "thread-abc123" + const match = /^(issue|pr|thread|task)-(.+)$/.exec(target); + if (!match) return 'Invalid target format. Use: issue-42, pr-99, etc.'; + + const [, workflowType, workflowId] = match; + + const targetEnv = await isolationEnvDb.findByWorkflow( + conversation.codebase_id, + workflowType, + workflowId + ); + + if (!targetEnv) return `No worktree found for ${target}`; + + await db.updateConversation(conversation.id, { + isolation_env_id: targetEnv.id, + cwd: targetEnv.working_path, + }); + + return `Linked to worktree \`${targetEnv.branch_name}\``; +} +``` + +--- + +## Skill Symbiosis (Preserved) + +The worktree-manager Claude Code skill creates worktrees at `~/tmp/worktrees///`. The new architecture preserves symbiosis through **adoption**. + +### Flow + +``` +1. Skill creates: ~/tmp/worktrees/myapp/feature-auth/ + (NOT in app's database - skill has its own registry) + +2. App receives PR webhook for branch "feature/auth" + +3. Orchestrator resolves isolation: + a. Check existing environment → none + b. Check linked issues → none + c. Check for branch adoption via findWorktreeByBranch() → FOUND! + +4. Orchestrator adopts the worktree: + - Creates isolation_environments record pointing to skill's worktree + - Sets metadata: { adopted: true, adopted_from: 'skill' } + +5. Conversation uses skill's worktree, both systems happy +``` + +### Key Code + +```typescript +// In resolveIsolation(): +if (hints?.prBranch) { + const existingPath = await findWorktreeByBranch(codebase.default_cwd, hints.prBranch); + + if (existingPath && (await worktreeExists(existingPath))) { + // Adopt into our system + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: 'pr', + workflow_id: hints.workflowId, + provider: 'worktree', + working_path: existingPath, + branch_name: hints.prBranch, + metadata: { adopted: true, adopted_from: 'skill' }, + }); + + console.log(`[Orchestrator] Adopted skill worktree: ${existingPath}`); + return env; + } +} +``` + +--- + +## GitHub Adapter Simplification + +### Before (Current - Complex) + +```typescript +// github.ts - 800+ lines +async handleWebhook(...) { + // ... 200 lines of event parsing ... + + // AUTO-ISOLATION LOGIC (REMOVE THIS) + if (!existingIsolation) { + // Check linked issues (50 lines) + // Fetch PR branch/SHA (30 lines) + // Create worktree (40 lines) + // Send UX messages (20 lines) + // Update database (15 lines) + } + + await handleMessage(...); +} +``` + +### After (Thin) + +```typescript +// github.ts - ~400 lines +async handleWebhook(...) { + // Signature verification + // Event parsing + // @mention detection + + // Handle close events (trigger cleanup) + if (isCloseEvent) { + await cleanupService.onConversationClosed(conversationId); + return; + } + + // Build hints (GitHub-specific context) + const hints: IsolationHints = { + workflowType: isPR ? 'pr' : 'issue', + workflowId: String(number), + prBranch: await this.getPRHeadBranch(owner, repo, number), + prSha: await this.getPRHeadSha(owner, repo, number), + linkedIssues: await getLinkedIssueNumbers(owner, repo, number), + }; + + // Build context for AI + const context = this.buildContext(event); + + // Let orchestrator handle EVERYTHING else + await handleMessage(this, conversationId, message, context, hints); +} +``` + +### What Stays in GitHub Adapter + +| Responsibility | Stays | Moves | +| ------------------------------ | ----- | -------------- | +| Webhook signature verification | ✅ | | +| Event parsing | ✅ | | +| @mention detection | ✅ | | +| Building IsolationHints | ✅ | | +| Calling handleMessage() | ✅ | | +| Close event → trigger cleanup | ✅ | | +| Worktree creation | | → Orchestrator | +| UX messages for isolation | | → Orchestrator | +| Database updates for isolation | | → Orchestrator | +| Linked issue worktree sharing | | → Orchestrator | + +--- + +## Implementation Phases + +> **Note**: The original Phase 3B ("Auto-Isolation in Orchestrator") has been merged into Phase 2.5. +> Phase 2.5 now encompasses both the new data model AND the auto-isolation logic centralization. + +### Phase 2.5: Unified Isolation Architecture (DO FIRST) + +**Goal**: Centralize all isolation logic with new data model. This phase combines: + +- New work-centric database schema +- Migration from TEXT paths to UUID FKs +- Moving auto-isolation from GitHub adapter to orchestrator +- Auto-isolation for ALL platforms (previously only GitHub had it) + +**Database Changes**: + +1. Create `remote_agent_isolation_environments` table +2. Rename `isolation_env_id` → `isolation_env_id_legacy` (TEXT, for migration) +3. Add new `isolation_env_id` UUID FK to conversations +4. Add `last_activity_at` to conversations + +**New Modules**: + +1. `src/db/isolation-environments.ts` - CRUD for new table +2. `src/services/cleanup-service.ts` - Cleanup logic (scheduler not started yet) + +**Orchestrator Changes**: + +1. Add `isolationHints` parameter to `handleMessage()` +2. Add `validateAndResolveIsolation()` with cwd validation +3. Add `resolveIsolation()` with reuse/sharing logic +4. Add `migrateToIsolationEnvironment()` for legacy path migration +5. Move all UX messaging for isolation here + +**Adapter Changes**: + +1. **GitHub**: Remove worktree creation, keep close event trigger, add hints +2. **Slack/Discord/Telegram**: No changes needed (orchestrator handles) + +**Phase 2.5 Checklist**: + +- [ ] Create migration for `isolation_environments` table + column renames +- [ ] Create `src/db/isolation-environments.ts` +- [ ] Create `src/services/cleanup-service.ts` (without scheduler) +- [ ] Add `IsolationHints` interface to types +- [ ] Update `handleMessage()` signature +- [ ] Add `validateAndResolveIsolation()` to orchestrator +- [ ] Add `migrateToIsolationEnvironment()` for legacy support +- [ ] Add auto-isolation logic to orchestrator +- [ ] Refactor GitHub adapter (remove isolation logic, add hints) +- [ ] Add `/worktree link` command +- [ ] Update tests +- [ ] Test: GitHub issue → worktree created by orchestrator +- [ ] Test: GitHub PR → shares linked issue's worktree +- [ ] Test: Slack thread → worktree created by orchestrator +- [ ] Test: Stale path → cleaned up, new worktree created +- [ ] Test: Skill-created worktree → adopted correctly +- [ ] Test: Legacy worktree_path → migrated on-the-fly + +### Phase 3A: Force-Thread Response Model + +**Scope**: Bot ALWAYS responds in threads (Slack/Discord). + +- [ ] Add `createThread()` to `IPlatformAdapter` interface +- [ ] Implement `Slack.createThread()` +- [ ] Implement `Discord.createThread()` +- [ ] Update message handlers to force-create threads +- [ ] Test: @mention in channel → response in new thread + +### Phase 3C: Git-Based Cleanup Scheduler + +**Scope**: Scheduled cleanup using git as source of truth. + +- [ ] Add `startCleanupScheduler()` to index.ts +- [ ] Implement `runScheduledCleanup()` in cleanup service +- [ ] Add `isBranchMerged()` git check +- [ ] Add `findStaleEnvironments()` query +- [ ] Test: Merged branch → auto-removed +- [ ] Test: Stale Slack worktree → removed +- [ ] Test: Stale Telegram worktree → NOT removed + +### Phase 3D: Limits and User Feedback + +**Scope**: Enforce limits, provide helpful cleanup commands. + +**Limit Enforcement UX**: +When user hits MAX_WORKTREES_PER_CODEBASE (default: 25): + +``` +You have 25 active worktrees for **myproject** (limit reached). + +📊 Worktree Status: +• 3 merged (can auto-remove) +• 2 stale (no activity in 14+ days) +• 20 active + +Options: +• `/worktree cleanup merged` - Remove merged worktrees (3) +• `/worktree cleanup stale` - Remove stale worktrees (2) +• `/worktree list` - See all worktrees +• `/worktree remove ` - Remove specific worktree +``` + +**Checklist**: + +- [ ] Add limit check in orchestrator before creating new isolation +- [ ] Attempt auto-cleanup of merged branches when limit hit +- [ ] If auto-cleanup insufficient, show limit message with options +- [ ] Add `/worktree cleanup merged` command +- [ ] Add `/worktree cleanup stale` command +- [ ] Update `/status` to show worktree count and breakdown +- [ ] Test: Hit limit → helpful message shown +- [ ] Test: Auto-cleanup makes room → continue without user action + +### Phase 4: Schema Cleanup + +**Scope**: Remove legacy columns after migration complete. + +- [ ] Verify all code uses new model +- [ ] Create migration to drop `worktree_path` column +- [ ] Create migration to drop `isolation_provider` column +- [ ] Remove fallback patterns from queries + +--- + +## Resolved Questions + +### 1. Isolation Authority Location + +**Decision**: **Orchestrator only** + **Rationale**: -- Helpful for new users who don't understand isolation -- Branch name visible for debugging/reference -- Escape hatch (/repos) if wrong codebase was auto-selected -- Single message, not intrusive -## Research Tasks +- Single source of truth eliminates duplication +- Adapters become thin and testable +- Cross-platform sharing requires centralized logic +- Easier to maintain and extend -### Completed Research +### 2. Data Model -#### 1. Worktree Creation Overhead (TESTED) +**Decision**: **Work-centric with `isolation_environments` table** -**Findings** (tested on remote-coding-agent repo, 2025-12-17): +**Rationale**: -| Metric | Value | Notes | -|--------|-------|-------| -| Creation time | **0.099 seconds** | Sub-100ms, negligible | -| Main repo size | 981 MB | Includes node_modules, .git | -| Worktree size | **2.5 MB** | Only working tree files | -| Space overhead | **0.25%** | Worktrees are extremely cheap | +- Independent lifecycle from conversations +- Natural sharing across conversations +- Query-friendly for cleanup and status +- Metadata enables future smart linking -**Conclusion**: Resource concerns are completely unfounded. Creating worktrees is: -- Fast enough to not impact UX (~100ms) -- Space-efficient (shared .git, shared node_modules via symlinks) -- Safe to create aggressively (auto on every @mention) +### 3. Cross-Platform Linking -#### 2. Branch Merge Detection (TESTED) +**Decision**: **Metadata + `/worktree link` fallback** + +MVP: + +- Store `related_issues`, `related_prs` in metadata automatically +- Manual `/worktree link` command for explicit linking + +Future: + +- AI-assisted discovery via git log searches +- Parse issue/PR bodies for references + +### 4. GitHub Adapter Role + +**Decision**: **Thin adapter providing hints** + +Keeps: + +- Webhook handling, event parsing +- @mention detection +- Building `IsolationHints` +- Triggering cleanup on close events + +Removes: + +- All worktree creation logic +- UX messages for isolation +- Database updates for isolation + +### 5. Cleanup Service Location + +**Decision**: **Separate service, called by orchestrator/adapters** + +**Rationale**: + +- Clean separation of concerns +- Git-first logic isolated +- Multiple entry points (events, schedule, commands) +- Easier to test + +### 6. Skill Symbiosis + +**Decision**: **Preserved through adoption** + +- Skill worktrees discovered via `findWorktreeByBranch()` +- App creates DB record pointing to existing worktree +- Both systems can coexist + +--- + +## Branch Naming Strategy + +### Semantic Names by Workflow Type + +| Workflow Type | Branch Name Pattern | Example | +| ------------- | -------------------- | -------------------- | +| `issue` | `issue-{number}` | `issue-42` | +| `pr` | `pr-{number}` | `pr-99` | +| `review` | `pr-{number}-review` | `pr-99-review` | +| `thread` | `thread-{hash}` | `thread-a7f3b2c1` | +| `task` | `task-{slug}` | `task-add-dark-mode` | + +### Uniqueness Constraint + +```sql +UNIQUE (codebase_id, workflow_type, workflow_id) +``` + +This ensures: + +- Only one `issue-42` per codebase +- Multiple conversations can share the same workflow + +--- + +## Research Findings + +### Worktree Performance (Tested) + +| Metric | Value | +| -------------- | ---------------------------- | +| Creation time | 0.099 seconds | +| Worktree size | 2.5 MB (vs 981 MB main repo) | +| Space overhead | 0.25% | + +**Conclusion**: Worktrees are cheap. Create aggressively. + +### Branch Merge Detection -**Command that works**: ```bash -# List all branches merged into main -git branch --merged main - -# For a specific branch git branch --merged main | grep "issue-42" ``` -**Conclusion**: Git natively supports this. Implementation is trivial. +Git natively supports this. Trivial to implement. -#### 3. Cleanup Scheduler Design +### Race Conditions -**Options considered**: -- `setInterval()` - Simple, no dependencies, in-process -- `node-cron` - Cron syntax, still in-process -- External cron - Requires separate process management - -**Recommendation**: `setInterval()` with configurable period (default: 6 hours) +Existing `ConversationLockManager` handles this: ```typescript -// In src/index.ts or new src/cleanup/scheduler.ts -const CLEANUP_INTERVAL_MS = parseInt(process.env.CLEANUP_INTERVAL_HOURS ?? '6') * 60 * 60 * 1000; - -setInterval(async () => { - await cleanupMergedWorktrees(); - await markStaleWorktrees(); -}, CLEANUP_INTERVAL_MS); - -// Also run on startup -await cleanupMergedWorktrees(); -``` - -**Why not cron?**: -- Single-server deployment (no need for distributed scheduling) -- Simpler (no additional dependencies) -- Cleanup is idempotent (safe to run multiple times) - -#### 4. SDK Tool Restriction Capabilities (RESEARCHED) - -**Claude Agent SDK findings** (from [official docs](https://code.claude.com/docs/en/sdk/sdk-permissions) and [GitHub issues](https://github.com/anthropics/claude-agent-sdk-typescript/issues/19)): - -| Feature | Status | Notes | -|---------|--------|-------| -| `allowedTools` | **Broken** | Does not work with `bypassPermissions` mode | -| `disallowedTools` | **Works** | Can blacklist specific tools | -| `permissionMode` | Works | `'default'`, `'acceptEdits'`, `'bypassPermissions'` | -| `canUseTool` callback | Works | Runtime permission check, but adds latency | - -**Current limitation**: When using `permissionMode: 'bypassPermissions'` (which we use), the `allowedTools` whitelist is ignored. Tools can still be used even if not in the list. - -**Workarounds for future workflow engine**: -1. Use `disallowedTools` blacklist (works) -2. Use `canUseTool` callback for runtime enforcement -3. Wait for SDK fix (open issue since 2025) - -**For now**: Not blocking for Phase 3. Revisit when building workflow engine. - -### Remaining Research - -- [ ] Design workflow engine schema (deferred to future phase) - -## Implementation Considerations - -### Race Condition Prevention - -**Problem**: Two simultaneous messages for same conversation could both trigger worktree creation. - -**Solution**: The existing `ConversationLockManager` already handles this! - -```typescript -// src/index.ts - already implemented lockManager.acquireLock(conversationId, async () => { await handleMessage(...); }); ``` -**Lock scope**: From `handleMessage()` entry through `db.updateConversation()` completion. This ensures: -1. Only one message processed per conversation at a time -2. Auto-isolation check + creation + DB update is atomic -3. Second message waits until first completes, then sees isolation already exists - -**No additional work needed** - the concurrency protection is already in place. - -### Database Migrations Needed - -**New column**: `last_activity_at` on `conversations` table - -```sql --- migrations/XXX_add_last_activity_at.sql -ALTER TABLE remote_agent_conversations -ADD COLUMN last_activity_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(); - --- Update existing rows -UPDATE remote_agent_conversations SET last_activity_at = updated_at; - --- Create index for cleanup queries -CREATE INDEX idx_conversations_last_activity ON remote_agent_conversations(last_activity_at); -``` - -**When to update**: On every message received (in `handleMessage()` or adapter layer). - -### Error Handling in Cleanup - -**What if cleanup fails?** -- Log the error, continue to next worktree -- Don't crash the cleanup cycle for one failure -- Worktree stays active, will be retried next cycle - -**What if worktree has uncommitted changes?** -- `git worktree remove` fails (expected) -- Log warning: "Worktree X has uncommitted changes, skipping" -- Keep worktree active -- User must manually commit/discard or force-remove - -## References - -- `docs/worktree-orchestration.md` - Current architecture documentation -- `src/isolation/` - Isolation provider implementation -- `src/adapters/github.ts` - Reference implementation for auto-isolation -- `.agents/plans/completed/isolation-provider-phase2-migration.plan.md` - Phase 2 completion +No additional work needed. --- -## Appendix: Platform-Specific Considerations +## Platform-Specific Notes ### Telegram -**Conversation model**: Single persistent chat (`chat_id`) -**Worktree mapping**: One persistent worktree per chat per codebase -**Cleanup trigger**: Manual only (`/worktree remove`) -**Special considerations**: -- No threading - each chat IS the conversation -- Worktree persists forever (user's permanent workspace) -- No staleness cleanup (user controls lifecycle) -- Auto-create on first message with configured codebase -- `/worktree remove` + next message = fresh start +- No threads - each chat is persistent +- Worktrees never auto-cleanup (user controls) +- Perfect for "permanent workspace" use case -### Slack +### Slack/Discord -**Conversation model**: `channel:thread_ts` -**Worktree mapping**: One worktree per thread -**Cleanup trigger**: Git merge detection, staleness -**Special considerations**: -- Threads can be revived after long time -- Thread history available for context -- App mention required (`@bot`) +- Threads isolate work naturally +- 1 thread = 1 worktree = 1 task +- Staleness cleanup after 14 days -### Discord +### GitHub -**Conversation model**: Thread = separate channel ID -**Worktree mapping**: One worktree per thread -**Cleanup trigger**: Git merge detection, staleness -**Special considerations**: -- Similar to Slack threading -- Threads auto-archive after inactivity (Discord feature) -- Could hook into Discord thread archive event? +- Cleanest lifecycle (explicit close events) +- Linked issues share worktrees ("Fixes #X") +- Adapter triggers cleanup, orchestrator handles logic -### GitHub (Reference) +--- -**Conversation model**: `owner/repo#number` -**Worktree mapping**: One worktree per issue/PR -**Cleanup trigger**: Issue/PR close event -**Special considerations**: -- Linked issues share worktree (via "Fixes #X") -- PR inherits issue worktree when linked -- Cleanest lifecycle model +## Future: Workflow Engine Integration + +The current implementation creates isolation for ALL AI interactions. In the future, a workflow engine could be smarter about this. + +### Planned Enhancement + +```typescript +interface WorkflowCommand { + name: string; + description: string; + prompt: string; + + // Future: Metadata for smarter orchestration + metadata: { + type: 'read' | 'write' | 'mixed'; + requiresIsolation: boolean; + allowedTools?: string[]; // Restrict AI tool access + }; +} +``` + +### How This Would Work + +1. Router receives: "explain the login flow" +2. Router routes to `explain` workflow (type: 'read', requiresIsolation: false) +3. Orchestrator skips worktree creation +4. AI runs on main repo (no isolation overhead for read-only queries) + +**vs:** + +1. Router receives: "fix the login bug" +2. Router routes to `fix-issue` workflow (type: 'write', requiresIsolation: true) +3. Orchestrator creates worktree +4. AI works in isolated environment + +### Why Not Implement Now? + +- Current worktree overhead is negligible (0.1s, 2.5MB) +- Simpler mental model: every conversation is isolated +- Workflow engine is a larger project +- Can add this optimization later without breaking changes + +--- + +## Research Notes + +### SDK Tool Restriction (2025) + +**Claude Agent SDK findings** (from official docs and GitHub issues): + +| Feature | Status | Notes | +| --------------------- | ---------- | --------------------------------------------------- | +| `allowedTools` | **Broken** | Does not work with `bypassPermissions` mode | +| `disallowedTools` | **Works** | Can blacklist specific tools | +| `permissionMode` | Works | `'default'`, `'acceptEdits'`, `'bypassPermissions'` | +| `canUseTool` callback | Works | Runtime permission check, but adds latency | + +**Current limitation**: When using `permissionMode: 'bypassPermissions'` (which we use), the `allowedTools` whitelist is ignored. + +**Workarounds for future workflow engine**: + +1. Use `disallowedTools` blacklist (works) +2. Use `canUseTool` callback for runtime enforcement +3. Wait for SDK fix (open issue) + +**For now**: Not blocking. Revisit when building workflow engine. + +--- + +## References + +- `docs/worktree-orchestration.md` - Architecture overview +- `src/isolation/` - Current isolation provider +- `src/adapters/github.ts` - Current GitHub implementation +- `.agents/plans/phase-*.plan.md` - Detailed implementation plans diff --git a/docs/worktree-orchestration.md b/docs/worktree-orchestration.md index cdaa1fe1..f0a1afa5 100644 --- a/docs/worktree-orchestration.md +++ b/docs/worktree-orchestration.md @@ -1,5 +1,7 @@ # Worktree Orchestration +> **Note**: This document describes the current architecture. See `docs/worktree-orchestration-research.md` for the planned unified architecture (Phase 2.5+) which centralizes all isolation logic in the orchestrator. + ## Storage Location ``` @@ -8,6 +10,7 @@ DOCKER: /workspace/worktrees/// ← FIXED, no override ``` Detection order in `getWorktreeBase()`: + ``` 1. isDocker? → /workspace/worktrees (ALWAYS) 2. WORKTREE_BASE set? → use it (local only) @@ -52,21 +55,21 @@ Detection order in `getWorktreeBase()`: ```typescript interface IsolationRequest { codebaseId: string; - canonicalRepoPath: string; // Main repo, never a worktree + canonicalRepoPath: string; // Main repo, never a worktree workflowType: 'issue' | 'pr' | 'review' | 'thread' | 'task'; identifier: string; - prBranch?: string; // For PR adoption - prSha?: string; // For reproducible reviews + prBranch?: string; // For PR adoption + prSha?: string; // For reproducible reviews } ``` -| Workflow | Identifier | Branch Name | -|----------|------------|-------------| -| issue | `"42"` | `issue-42` | -| pr | `"123"` | `pr-123` | -| pr + SHA | `"123"` | `pr-123-review` | -| task | `"my-feature"` | `task-my-feature` | -| thread | `"C123:ts.123"` | `thread-a1b2c3d4` (hash) | +| Workflow | Identifier | Branch Name | +| -------- | --------------- | ------------------------ | +| issue | `"42"` | `issue-42` | +| pr | `"123"` | `pr-123` | +| pr + SHA | `"123"` | `pr-123-review` | +| task | `"my-feature"` | `task-my-feature` | +| thread | `"C123:ts.123"` | `thread-a1b2c3d4` (hash) | ## Creation Flow @@ -225,6 +228,7 @@ conversations ``` Lookup pattern: + ```typescript const envId = conversation.isolation_env_id ?? conversation.worktree_path; ``` @@ -234,6 +238,7 @@ const envId = conversation.isolation_env_id ?? conversation.worktree_path; The worktree-manager Claude Code skill uses `~/.claude/worktree-registry.json`. **Adoption scenarios:** + 1. **Path match**: Skill created worktree at expected path → adopted 2. **Branch match**: Skill created worktree for PR's branch → adopted @@ -253,11 +258,96 @@ App checks: findWorktreeByBranch("feature/auth") ## Key Files -| File | Purpose | -|------|---------| -| `src/isolation/types.ts` | `IIsolationProvider`, `IsolationRequest`, `IsolatedEnvironment` | -| `src/isolation/providers/worktree.ts` | `WorktreeProvider` implementation | -| `src/isolation/index.ts` | `getIsolationProvider()` factory | -| `src/utils/git.ts` | `getWorktreeBase()`, `listWorktrees()`, low-level git ops | -| `src/adapters/github.ts` | Webhook handling, `cleanupPRWorktree()` | -| `src/handlers/command-handler.ts` | `/worktree` command handling | +| File | Purpose | +| ------------------------------------- | --------------------------------------------------------------- | +| `src/isolation/types.ts` | `IIsolationProvider`, `IsolationRequest`, `IsolatedEnvironment` | +| `src/isolation/providers/worktree.ts` | `WorktreeProvider` implementation | +| `src/isolation/index.ts` | `getIsolationProvider()` factory | +| `src/utils/git.ts` | `getWorktreeBase()`, `listWorktrees()`, low-level git ops | +| `src/adapters/github.ts` | Webhook handling, `cleanupPRWorktree()` | +| `src/handlers/command-handler.ts` | `/worktree` command handling | + +--- + +## Planned Architecture (Phase 2.5+) + +The current architecture has isolation logic split between the GitHub adapter and orchestrator. Phase 2.5 will unify all isolation in the orchestrator. + +### Target Architecture + +``` +┌─────────────────────────────────────────────────────────────────────────┐ +│ ALL ADAPTERS (Thin) │ +│ GitHub, Slack, Discord, Telegram │ +├─────────────────────────────────────────────────────────────────────────┤ +│ ✓ Parse platform events │ +│ ✓ Detect @mentions │ +│ ✓ Build context + IsolationHints │ +│ ✓ Call handleMessage(platform, convId, message, context, hints) │ +│ ✓ Trigger cleanup events (GitHub only: close/merge) │ +│ ✗ NO worktree creation │ +│ ✗ NO isolation UX messages │ +└───────────────────────────────────┬─────────────────────────────────────┘ + │ + ▼ +┌─────────────────────────────────────────────────────────────────────────┐ +│ ORCHESTRATOR (Authority) │ +├─────────────────────────────────────────────────────────────────────────┤ +│ validateAndResolveIsolation(): │ +│ 1. Validate existing isolation (cwd exists?) │ +│ 2. Check for reuse (same workflow_type + workflow_id) │ +│ 3. Check linked issues for sharing │ +│ 4. Check for skill adoption (findWorktreeByBranch) │ +│ 5. Create new if needed │ +│ 6. Send UX message │ +│ 7. Update database │ +└───────────────────────────────────┬─────────────────────────────────────┘ + │ + ┌───────────────┴───────────────┐ + ▼ ▼ +┌───────────────────────────────┐ ┌───────────────────────────────────┐ +│ ISOLATION PROVIDER │ │ CLEANUP SERVICE │ +│ (WorktreeProvider) │ │ src/services/cleanup-service.ts │ +├───────────────────────────────┤ ├───────────────────────────────────┤ +│ create() → IsolatedEnv │ │ onConversationClosed() │ +│ destroy() │ │ runScheduledCleanup() │ +│ get() / list() │ │ isBranchMerged() - git-first │ +│ adopt() │ │ hasUncommittedChanges() │ +└───────────────────────────────┘ └───────────────────────────────────┘ +``` + +### New Database Schema + +```sql +-- Work-centric isolation (independent lifecycle) +CREATE TABLE remote_agent_isolation_environments ( + id UUID PRIMARY KEY, + codebase_id UUID REFERENCES remote_agent_codebases(id), + workflow_type TEXT NOT NULL, -- 'issue', 'pr', 'thread', 'task' + workflow_id TEXT NOT NULL, -- '42', 'thread-abc123' + provider TEXT DEFAULT 'worktree', + working_path TEXT NOT NULL, + branch_name TEXT NOT NULL, + status TEXT DEFAULT 'active', + created_at TIMESTAMP DEFAULT NOW(), + created_by_platform TEXT, + metadata JSONB DEFAULT '{}', + UNIQUE (codebase_id, workflow_type, workflow_id) +); + +-- Conversations link to environments (many-to-one) +ALTER TABLE remote_agent_conversations + ADD COLUMN isolation_env_id UUID REFERENCES remote_agent_isolation_environments(id); +``` + +### Implementation Phases + +| Phase | Description | Status | +| ----- | ------------------------------------------- | ------- | +| 2.5 | Unified Isolation Architecture | Planned | +| 3A | Force-Thread Response Model (Slack/Discord) | Planned | +| 3C | Git-Based Cleanup Scheduler | Planned | +| 3D | Limits and User Feedback | Planned | +| 4 | Drop Legacy Columns | Planned | + +See `docs/worktree-orchestration-research.md` for detailed implementation plans. diff --git a/migrations/006_isolation_environments.sql b/migrations/006_isolation_environments.sql new file mode 100644 index 00000000..fcbf5ebc --- /dev/null +++ b/migrations/006_isolation_environments.sql @@ -0,0 +1,70 @@ +-- Work-centric isolation environments +-- Version: 6.0 +-- Description: Independent isolation entities with workflow identity + +CREATE TABLE IF NOT EXISTS remote_agent_isolation_environments ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + codebase_id UUID NOT NULL REFERENCES remote_agent_codebases(id) ON DELETE CASCADE, + + -- Workflow identification (what work this is for) + workflow_type TEXT NOT NULL, -- 'issue', 'pr', 'review', 'thread', 'task' + workflow_id TEXT NOT NULL, -- '42', 'pr-99', 'thread-abc123' + + -- Implementation details + provider TEXT NOT NULL DEFAULT 'worktree', + working_path TEXT NOT NULL, -- Actual filesystem path + branch_name TEXT NOT NULL, -- Git branch name + + -- Lifecycle + status TEXT NOT NULL DEFAULT 'active', -- 'active', 'destroyed' + created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), + created_by_platform TEXT, -- 'github', 'slack', etc. + + -- Cross-reference metadata (for linking) + metadata JSONB DEFAULT '{}', + + CONSTRAINT unique_workflow UNIQUE (codebase_id, workflow_type, workflow_id) +); + +-- Indexes for common queries +CREATE INDEX IF NOT EXISTS idx_isolation_env_codebase + ON remote_agent_isolation_environments(codebase_id); +CREATE INDEX IF NOT EXISTS idx_isolation_env_status + ON remote_agent_isolation_environments(status); +CREATE INDEX IF NOT EXISTS idx_isolation_env_workflow + ON remote_agent_isolation_environments(workflow_type, workflow_id); + +-- Rename old column to legacy (for migration) +-- Note: This will fail if column doesn't exist or is already renamed - that's OK +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name = 'remote_agent_conversations' + AND column_name = 'isolation_env_id' + AND data_type = 'character varying' + ) THEN + ALTER TABLE remote_agent_conversations + RENAME COLUMN isolation_env_id TO isolation_env_id_legacy; + END IF; +END $$; + +-- Add new UUID FK column +ALTER TABLE remote_agent_conversations + ADD COLUMN IF NOT EXISTS isolation_env_id UUID + REFERENCES remote_agent_isolation_environments(id) ON DELETE SET NULL; + +-- Add last_activity_at for staleness detection +ALTER TABLE remote_agent_conversations + ADD COLUMN IF NOT EXISTS last_activity_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(); + +-- Create index for FK lookups +CREATE INDEX IF NOT EXISTS idx_conversations_isolation_env_id + ON remote_agent_conversations(isolation_env_id); + +COMMENT ON TABLE remote_agent_isolation_environments IS + 'Work-centric isolated environments with independent lifecycle'; +COMMENT ON COLUMN remote_agent_isolation_environments.workflow_type IS + 'Type of work: issue, pr, review, thread, task'; +COMMENT ON COLUMN remote_agent_isolation_environments.workflow_id IS + 'Identifier for the work (issue number, PR number, thread hash, etc.)'; diff --git a/src/adapters/github.test.ts b/src/adapters/github.test.ts index fd4f6ca8..34f1ce18 100644 --- a/src/adapters/github.test.ts +++ b/src/adapters/github.test.ts @@ -210,7 +210,8 @@ describe('GitHubAdapter', () => { }); const crypto = require('crypto'); - const signature = 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); + const signature = + 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); await adapter.handleWebhook(payload, signature); @@ -290,7 +291,8 @@ describe('GitHubAdapter', () => { }); const crypto = require('crypto'); - const signature = 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); + const signature = + 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); await adapter.handleWebhook(payload, signature); @@ -373,7 +375,8 @@ describe('GitHubAdapter', () => { }); const crypto = require('crypto'); - const signature = 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); + const signature = + 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); await adapter.handleWebhook(payload, signature); @@ -447,7 +450,8 @@ describe('GitHubAdapter', () => { }); const crypto = require('crypto'); - const signature = 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); + const signature = + 'sha256=' + crypto.createHmac('sha256', 'test-secret').update(payload).digest('hex'); await adapter.handleWebhook(payload, signature); diff --git a/src/adapters/github.ts b/src/adapters/github.ts index 3a38c1d0..40b358fd 100644 --- a/src/adapters/github.ts +++ b/src/adapters/github.ts @@ -4,20 +4,19 @@ */ import { Octokit } from '@octokit/rest'; import { createHmac, timingSafeEqual } from 'crypto'; -import { IPlatformAdapter } from '../types'; +import { IPlatformAdapter, IsolationHints } from '../types'; import { handleMessage } from '../orchestrator/orchestrator'; import { classifyAndFormatError } from '../utils/error-formatter'; import * as db from '../db/conversations'; import * as codebaseDb from '../db/codebases'; -import * as sessionDb from '../db/sessions'; import { exec } from 'child_process'; import { promisify } from 'util'; import { readdir, access } from 'fs/promises'; import { join, resolve } from 'path'; import { parseAllowedUsers, isGitHubUserAuthorized } from '../utils/github-auth'; -import { isWorktreePath } from '../utils/git'; -import { getIsolationProvider } from '../isolation'; import { getLinkedIssueNumbers } from '../utils/github-graphql'; +import { onConversationClosed } from '../services/cleanup-service'; +import { isWorktreePath } from '../utils/git'; const execAsync = promisify(exec); @@ -439,80 +438,19 @@ export class GitHubAdapter implements IPlatformAdapter { /** * Clean up worktree when an issue/PR is closed - * Handles shared worktrees: only removes if no other conversations reference it + * Delegates to cleanup service for unified handling */ private async cleanupWorktree(owner: string, repo: string, number: number): Promise { const conversationId = this.buildConversationId(owner, repo, number); - const conversation = await db.getConversationByPlatformId('github', conversationId); + console.log(`[GitHub] Cleaning up isolation for ${conversationId}`); - // Check both old and new isolation fields for backwards compatibility - if (!conversation) { - console.log(`[GitHub] No conversation to cleanup for ${conversationId}`); - return; - } - - const isolationEnvId = conversation.isolation_env_id ?? conversation.worktree_path; - if (!isolationEnvId) { - console.log(`[GitHub] No worktree to cleanup for ${conversationId}`); - return; - } - - // Deactivate any active session for this conversation - // This prevents resume attempts with a stale cwd - const activeSession = await sessionDb.getActiveSession(conversation.id); - if (activeSession) { - await sessionDb.deactivateSession(activeSession.id); - console.log(`[GitHub] Deactivated session ${activeSession.id} for worktree cleanup`); - } - - const { codebase } = await this.getOrCreateCodebaseForRepo(owner, repo); - - // Clear isolation references from THIS conversation first - // This must happen before checking for other users of the worktree - await db.updateConversation(conversation.id, { - worktree_path: null, - isolation_env_id: null, - isolation_provider: null, - cwd: codebase.default_cwd, - }); - - // Check if any OTHER conversations still use this worktree (shared worktree case) - // Check both old and new fields for backwards compatibility - const otherConvByWorktree = await db.getConversationByWorktreePath(isolationEnvId); - const otherConvByEnvId = await db.getConversationByIsolationEnvId(isolationEnvId); - const otherConv = otherConvByWorktree ?? otherConvByEnvId; - if (otherConv) { - console.log( - `[GitHub] Keeping worktree ${isolationEnvId}, still used by ${otherConv.platform_conversation_id}` - ); - console.log(`[GitHub] Cleanup complete for ${conversationId}`); - return; - } - - // No other conversations use this worktree - safe to remove via provider - const provider = getIsolationProvider(); try { - await provider.destroy(isolationEnvId); - console.log(`[GitHub] Removed worktree: ${isolationEnvId}`); + await onConversationClosed('github', conversationId); + console.log(`[GitHub] Cleanup complete for ${conversationId}`); } catch (error) { const err = error as Error; - // Handle already-deleted worktree gracefully (e.g., manual cleanup) - if (err.message.includes('is not a working tree')) { - console.log(`[GitHub] Worktree already removed: ${isolationEnvId}`); - } else { - console.error('[GitHub] Failed to remove worktree:', error); - // Notify user about orphaned worktree (likely has uncommitted changes) - const hasUncommittedChanges = err.message.includes('contains modified or untracked files'); - if (hasUncommittedChanges) { - await this.sendMessage( - conversationId, - `Warning: Could not remove worktree at \`${isolationEnvId}\` because it contains uncommitted changes. You may want to manually commit or discard these changes.` - ); - } - } + console.error(`[GitHub] Cleanup failed for ${conversationId}:`, err.message); } - - console.log(`[GitHub] Cleanup complete for ${conversationId}`); } /** @@ -626,130 +564,40 @@ ${userComment}`; await this.autoDetectAndLoadCommands(repoPath, codebase.id); } - // 10. Create worktree for this issue/PR (if conversation doesn't have one) - let worktreePath: string | null = null; - let prHeadBranch: string | undefined; - let prHeadSha: string | undefined; - // Detect PR: either pull_request event, or issue_comment on a PR (indicated by issue.pull_request or pullRequest) + // 10. Gather isolation hints for orchestrator + // The orchestrator now handles all isolation decisions const isPR = eventType === 'pull_request' || !!pullRequest || !!issue?.pull_request; - const existingIsolation = existingConv.isolation_env_id ?? existingConv.worktree_path; - if (!existingIsolation) { - // For PRs: Check if this PR is linked to an existing issue with a worktree - if (isPR) { - const linkedIssues = await getLinkedIssueNumbers(owner, repo, number); - for (const issueNum of linkedIssues) { - // Check if the linked issue has a worktree we can reuse - const issueConvId = this.buildConversationId(owner, repo, issueNum); - const issueConv = await db.getConversationByPlatformId('github', issueConvId); + // Build isolation hints for orchestrator + const isolationHints: IsolationHints = { + workflowType: isPR ? 'pr' : 'issue', + workflowId: String(number), + }; - const issueIsolation = issueConv?.isolation_env_id ?? issueConv?.worktree_path; - if (issueIsolation) { - // Reuse the issue's worktree - worktreePath = issueIsolation; - console.log( - `[GitHub] PR #${String(number)} linked to issue #${String(issueNum)}, sharing worktree: ${worktreePath}` - ); - - // Send user feedback about shared worktree - await this.sendMessage( - conversationId, - `Reusing worktree from issue #${String(issueNum)}` - ); - - // Update this conversation to use the shared worktree - await db.updateConversation(existingConv.id, { - codebase_id: codebase.id, - cwd: worktreePath, - worktree_path: worktreePath, - isolation_env_id: issueConv?.isolation_env_id ?? worktreePath, - isolation_provider: issueConv?.isolation_provider ?? 'worktree', - }); - break; // Use first found worktree - } - } + // For PRs: get linked issues and branch info + if (isPR) { + // Get linked issues for worktree sharing + const linkedIssues = await getLinkedIssueNumbers(owner, repo, number); + if (linkedIssues.length > 0) { + isolationHints.linkedIssues = linkedIssues; + console.log(`[GitHub] PR #${String(number)} linked to issues: ${linkedIssues.join(', ')}`); } - // If no shared worktree found, create new one via provider - if (!worktreePath) { - try { - // For PRs, fetch the head branch name and SHA from GitHub API - if (isPR) { - try { - const { data: prData } = await this.octokit.rest.pulls.get({ - owner, - repo, - pull_number: number, - }); - prHeadBranch = prData.head.ref; - prHeadSha = prData.head.sha; - console.log( - `[GitHub] PR #${String(number)} head branch: ${prHeadBranch}, SHA: ${prHeadSha}` - ); - } catch (error) { - console.warn( - '[GitHub] Failed to fetch PR head branch, will create new branch instead:', - error - ); - } - } - - // Use isolation provider for worktree creation - const provider = getIsolationProvider(); - const env = await provider.create({ - codebaseId: codebase.id, - canonicalRepoPath: repoPath, - workflowType: isPR ? 'pr' : 'issue', - identifier: String(number), - prBranch: prHeadBranch, - prSha: prHeadSha, - description: `GitHub ${isPR ? 'PR' : 'issue'} #${String(number)}`, - }); - - worktreePath = env.workingPath; - console.log(`[GitHub] Created worktree: ${worktreePath}`); - - // Send user feedback about isolation (single line, not verbose) - if (isPR && prHeadBranch && prHeadSha) { - const shortSha = prHeadSha.substring(0, 7); - await this.sendMessage( - conversationId, - `Reviewing PR at commit \`${shortSha}\` (branch: \`${prHeadBranch}\`)` - ); - } else { - const branchName = isPR ? `pr-${String(number)}` : `issue-${String(number)}`; - await this.sendMessage( - conversationId, - `Working in isolated branch \`${branchName}\`` - ); - } - - // Update conversation with isolation info (both old and new fields for compatibility) - await db.updateConversation(existingConv.id, { - codebase_id: codebase.id, - cwd: worktreePath, - worktree_path: worktreePath, - isolation_env_id: env.id, - isolation_provider: env.provider, - }); - } catch (error) { - const err = error as Error; - console.error('[GitHub] Failed to create worktree:', error); - const branchName = isPR ? `pr-${String(number)}` : `issue-${String(number)}`; - await this.sendMessage( - conversationId, - `Failed to create isolated worktree for branch \`${branchName}\`. This may be due to a branch name conflict or filesystem issue.\n\nError: ${err.message}\n\nPlease resolve the issue and try again.` - ); - return; // Don't continue without isolation - } + // Fetch PR head branch and SHA for isolation + try { + const { data: prData } = await this.octokit.rest.pulls.get({ + owner, + repo, + pull_number: number, + }); + isolationHints.prBranch = prData.head.ref; + isolationHints.prSha = prData.head.sha; + console.log( + `[GitHub] PR #${String(number)} head: ${prData.head.ref}@${prData.head.sha.substring(0, 7)}` + ); + } catch (error) { + console.warn('[GitHub] Failed to fetch PR head info:', error); } - } else { - // Conversation already has isolation, use it - worktreePath = existingIsolation; - - // Send user feedback about reusing existing worktree - const branchName = isPR ? `pr-${String(number)}` : `issue-${String(number)}`; - await this.sendMessage(conversationId, `Reusing worktree \`${branchName}\``); } // 11. Build message with context @@ -758,38 +606,27 @@ ${userComment}`; let contextToAppend: string | undefined; // IMPORTANT: Slash commands must be processed deterministically (not by AI) - // Extract only the first line if it's a slash command const isSlashCommand = strippedComment.trim().startsWith('/'); - const isCommandInvoke = strippedComment.trim().startsWith('/command-invoke'); if (isSlashCommand) { - // For slash commands, use only the first line to avoid mixing commands with instructions - const firstLine = strippedComment.split('\n')[0].trim(); - finalMessage = firstLine; - console.log(`[GitHub] Processing slash command: ${firstLine}`); + // For slash commands, use only the first line + finalMessage = strippedComment.split('\n')[0].trim(); + console.log(`[GitHub] Processing slash command: ${finalMessage}`); - // For /command-invoke, pass just the issue/PR number (not full description) - // This avoids tempting the AI to implement before planning - if (isCommandInvoke) { - const activeSession = await sessionDb.getActiveSession(existingConv.id); - const isFirstCommandInvoke = !activeSession; - - if (isFirstCommandInvoke) { - console.log('[GitHub] Adding issue/PR reference for first /command-invoke'); - if (eventType === 'issue' && issue) { - contextToAppend = `GitHub Issue #${String(issue.number)}: "${issue.title}"\nUse 'gh issue view ${String(issue.number)}' for full details if needed.`; - } else if (eventType === 'issue_comment' && issue) { - contextToAppend = `GitHub Issue #${String(issue.number)}: "${issue.title}"\nUse 'gh issue view ${String(issue.number)}' for full details if needed.`; - } else if (eventType === 'pull_request' && pullRequest) { - contextToAppend = `GitHub Pull Request #${String(pullRequest.number)}: "${pullRequest.title}"\nUse 'gh pr view ${String(pullRequest.number)}' for full details if needed.`; - } else if (eventType === 'issue_comment' && pullRequest) { - contextToAppend = `GitHub Pull Request #${String(pullRequest.number)}: "${pullRequest.title}"\nUse 'gh pr view ${String(pullRequest.number)}' for full details if needed.`; - } + // Add issue/PR reference context + if (eventType === 'issue' && issue) { + contextToAppend = `GitHub Issue #${String(issue.number)}: "${issue.title}"\nUse 'gh issue view ${String(issue.number)}' for full details if needed.`; + } else if (eventType === 'pull_request' && pullRequest) { + contextToAppend = `GitHub Pull Request #${String(pullRequest.number)}: "${pullRequest.title}"\nUse 'gh pr view ${String(pullRequest.number)}' for full details if needed.`; + } else if (eventType === 'issue_comment') { + if (pullRequest) { + contextToAppend = `GitHub Pull Request #${String(pullRequest.number)}: "${pullRequest.title}"\nUse 'gh pr view ${String(pullRequest.number)}' for full details if needed.`; + } else if (issue) { + contextToAppend = `GitHub Issue #${String(issue.number)}: "${issue.title}"\nUse 'gh issue view ${String(issue.number)}' for full details if needed.`; } } } else { - // For non-command messages, always add issue/PR context - // Router needs this context to understand what the user is asking about + // For non-command messages, add rich context if (eventType === 'issue' && issue) { finalMessage = this.buildIssueContext(issue, strippedComment); } else if (eventType === 'issue_comment' && issue) { @@ -801,35 +638,17 @@ ${userComment}`; } } - // Add worktree context if working in an isolated branch - if (worktreePath) { - // Use the actual PR head branch name if available, otherwise use the worktree naming convention - const branchName = - isPR && prHeadBranch - ? prHeadBranch - : isPR - ? `pr-${String(number)}` - : `issue-${String(number)}`; - let worktreeContext: string; - - if (isPR && prHeadBranch && prHeadSha) { - // SHA-based PR review - show commit SHA for reproducibility - const shortSha = prHeadSha.substring(0, 7); - worktreeContext = `\n\n[Working in isolated worktree at commit ${shortSha} (PR #${String(number)} branch: ${branchName}). This is the exact commit that triggered the review for reproducibility.]`; - } else if (isPR && prHeadBranch) { - // Branch-based PR review (fallback) - worktreeContext = `\n\n[Working in isolated worktree with PR branch: ${branchName}. This is the actual PR branch with all changes.]`; - } else { - // Issue workflow - worktreeContext = `\n\n[Working in isolated branch: ${branchName}. When done, changes can be committed and pushed, then a PR can be created from this branch.]`; - } - - contextToAppend = contextToAppend ? contextToAppend + worktreeContext : worktreeContext; - } - - // 12. Route to orchestrator + // 12. Route to orchestrator with isolation hints try { - await handleMessage(this, conversationId, finalMessage, contextToAppend); + await handleMessage( + this, + conversationId, + finalMessage, + contextToAppend, + undefined, // threadContext + undefined, // parentConversationId + isolationHints + ); } catch (error) { const err = error as Error; console.error('[GitHub] Message handling error:', error); diff --git a/src/db/conversations.ts b/src/db/conversations.ts index aa1de8cc..26da03bc 100644 --- a/src/db/conversations.ts +++ b/src/db/conversations.ts @@ -138,15 +138,33 @@ export async function updateConversation( } /** - * Find a conversation by isolation environment ID + * Find a conversation by isolation environment ID (legacy - single result) * Used for provider-based lookup and shared environment detection */ -export async function getConversationByIsolationEnvId( - envId: string -): Promise { +export async function getConversationByIsolationEnvId(envId: string): Promise { const result = await pool.query( 'SELECT * FROM remote_agent_conversations WHERE isolation_env_id = $1 LIMIT 1', [envId] ); return result.rows[0] ?? null; } + +/** + * Find all conversations using a specific isolation environment (new UUID model) + */ +export async function getConversationsByIsolationEnvId(envId: string): Promise { + const result = await pool.query( + 'SELECT * FROM remote_agent_conversations WHERE isolation_env_id = $1', + [envId] + ); + return result.rows; +} + +/** + * Update last_activity_at for staleness tracking + */ +export async function touchConversation(id: string): Promise { + await pool.query('UPDATE remote_agent_conversations SET last_activity_at = NOW() WHERE id = $1', [ + id, + ]); +} diff --git a/src/db/isolation-environments.test.ts b/src/db/isolation-environments.test.ts new file mode 100644 index 00000000..c24f02d0 --- /dev/null +++ b/src/db/isolation-environments.test.ts @@ -0,0 +1,237 @@ +import { mock, describe, test, expect, beforeEach } from 'bun:test'; +import { createQueryResult } from '../test/mocks/database'; +import { IsolationEnvironmentRow } from '../types'; + +const mockQuery = mock(() => Promise.resolve(createQueryResult([]))); + +mock.module('./connection', () => ({ + pool: { + query: mockQuery, + }, +})); + +import { + getById, + findByWorkflow, + listByCodebase, + create, + updateStatus, + updateMetadata, + countByCodebase, + getConversationsUsingEnv, +} from './isolation-environments'; + +describe('isolation-environments', () => { + beforeEach(() => { + mockQuery.mockClear(); + }); + + const sampleEnv: IsolationEnvironmentRow = { + id: 'env-123', + codebase_id: 'codebase-456', + workflow_type: 'issue', + workflow_id: '42', + provider: 'worktree', + working_path: '/workspace/worktrees/project/issue-42', + branch_name: 'issue-42', + status: 'active', + created_at: new Date(), + created_by_platform: 'github', + metadata: {}, + }; + + describe('getById', () => { + test('returns environment when found', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await getById('env-123'); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + 'SELECT * FROM remote_agent_isolation_environments WHERE id = $1', + ['env-123'] + ); + }); + + test('returns null when not found', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([])); + + const result = await getById('nonexistent'); + + expect(result).toBeNull(); + }); + }); + + describe('findByWorkflow', () => { + test('finds active environment by workflow identity', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await findByWorkflow('codebase-456', 'issue', '42'); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('workflow_type = $2 AND workflow_id = $3'), + ['codebase-456', 'issue', '42'] + ); + }); + + test('returns null when no matching active environment', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([])); + + const result = await findByWorkflow('codebase-456', 'issue', '99'); + + expect(result).toBeNull(); + }); + }); + + describe('listByCodebase', () => { + test('returns all active environments for codebase', async () => { + const envs = [sampleEnv, { ...sampleEnv, id: 'env-456', workflow_id: '43' }]; + mockQuery.mockResolvedValueOnce(createQueryResult(envs)); + + const result = await listByCodebase('codebase-456'); + + expect(result).toEqual(envs); + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('codebase_id = $1 AND status'), + ['codebase-456'] + ); + }); + + test('returns empty array when no environments', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([])); + + const result = await listByCodebase('empty-codebase'); + + expect(result).toEqual([]); + }); + }); + + describe('create', () => { + test('creates new environment with defaults', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([sampleEnv])); + + const result = await create({ + codebase_id: 'codebase-456', + workflow_type: 'issue', + workflow_id: '42', + working_path: '/workspace/worktrees/project/issue-42', + branch_name: 'issue-42', + }); + + expect(result).toEqual(sampleEnv); + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('INSERT INTO remote_agent_isolation_environments'), + expect.arrayContaining(['codebase-456', 'issue', '42', 'worktree']) + ); + }); + + test('creates environment with custom provider and metadata', async () => { + const customEnv = { ...sampleEnv, provider: 'container', metadata: { custom: true } }; + mockQuery.mockResolvedValueOnce(createQueryResult([customEnv])); + + await create({ + codebase_id: 'codebase-456', + workflow_type: 'issue', + workflow_id: '42', + provider: 'container', + working_path: '/workspace/worktrees/project/issue-42', + branch_name: 'issue-42', + created_by_platform: 'slack', + metadata: { custom: true }, + }); + + expect(mockQuery).toHaveBeenCalledWith( + expect.stringContaining('INSERT INTO remote_agent_isolation_environments'), + [ + 'codebase-456', + 'issue', + '42', + 'container', + '/workspace/worktrees/project/issue-42', + 'issue-42', + 'slack', + '{"custom":true}', + ] + ); + }); + }); + + describe('updateStatus', () => { + test('updates status to destroyed', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([], 1)); + + await updateStatus('env-123', 'destroyed'); + + expect(mockQuery).toHaveBeenCalledWith( + 'UPDATE remote_agent_isolation_environments SET status = $1 WHERE id = $2', + ['destroyed', 'env-123'] + ); + }); + + test('updates status to active', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([], 1)); + + await updateStatus('env-123', 'active'); + + expect(mockQuery).toHaveBeenCalledWith( + 'UPDATE remote_agent_isolation_environments SET status = $1 WHERE id = $2', + ['active', 'env-123'] + ); + }); + }); + + describe('updateMetadata', () => { + test('merges metadata with existing', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([], 1)); + + await updateMetadata('env-123', { pr_number: 42 }); + + expect(mockQuery).toHaveBeenCalledWith(expect.stringContaining('metadata = metadata ||'), [ + '{"pr_number":42}', + 'env-123', + ]); + }); + }); + + describe('countByCodebase', () => { + test('returns count of active environments', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([{ count: '5' }])); + + const result = await countByCodebase('codebase-456'); + + expect(result).toBe(5); + expect(mockQuery).toHaveBeenCalledWith(expect.stringContaining('COUNT(*)'), ['codebase-456']); + }); + + test('returns 0 when no environments', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([{ count: '0' }])); + + const result = await countByCodebase('empty-codebase'); + + expect(result).toBe(0); + }); + }); + + describe('getConversationsUsingEnv', () => { + test('returns conversation IDs using the environment', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([{ id: 'conv-1' }, { id: 'conv-2' }])); + + const result = await getConversationsUsingEnv('env-123'); + + expect(result).toEqual(['conv-1', 'conv-2']); + expect(mockQuery).toHaveBeenCalledWith( + 'SELECT id FROM remote_agent_conversations WHERE isolation_env_id = $1', + ['env-123'] + ); + }); + + test('returns empty array when no conversations use env', async () => { + mockQuery.mockResolvedValueOnce(createQueryResult([])); + + const result = await getConversationsUsingEnv('unused-env'); + + expect(result).toEqual([]); + }); + }); +}); diff --git a/src/db/isolation-environments.ts b/src/db/isolation-environments.ts new file mode 100644 index 00000000..5255fac8 --- /dev/null +++ b/src/db/isolation-environments.ts @@ -0,0 +1,140 @@ +/** + * Database operations for isolation environments + */ +import { pool } from './connection'; +import { IsolationEnvironmentRow } from '../types'; + +/** + * Get an isolation environment by UUID + */ +export async function getById(id: string): Promise { + const result = await pool.query( + 'SELECT * FROM remote_agent_isolation_environments WHERE id = $1', + [id] + ); + return result.rows[0] ?? null; +} + +/** + * Find an isolation environment by workflow identity + */ +export async function findByWorkflow( + codebaseId: string, + workflowType: string, + workflowId: string +): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND workflow_type = $2 AND workflow_id = $3 AND status = 'active'`, + [codebaseId, workflowType, workflowId] + ); + return result.rows[0] ?? null; +} + +/** + * Find all active environments for a codebase + */ +export async function listByCodebase(codebaseId: string): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND status = 'active' + ORDER BY created_at DESC`, + [codebaseId] + ); + return result.rows; +} + +/** + * Create a new isolation environment + */ +export async function create(env: { + codebase_id: string; + workflow_type: string; + workflow_id: string; + provider?: string; + working_path: string; + branch_name: string; + created_by_platform?: string; + metadata?: Record; +}): Promise { + const result = await pool.query( + `INSERT INTO remote_agent_isolation_environments + (codebase_id, workflow_type, workflow_id, provider, working_path, branch_name, created_by_platform, metadata) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + RETURNING *`, + [ + env.codebase_id, + env.workflow_type, + env.workflow_id, + env.provider ?? 'worktree', + env.working_path, + env.branch_name, + env.created_by_platform ?? null, + JSON.stringify(env.metadata ?? {}), + ] + ); + return result.rows[0]; +} + +/** + * Update environment status + */ +export async function updateStatus(id: string, status: 'active' | 'destroyed'): Promise { + await pool.query('UPDATE remote_agent_isolation_environments SET status = $1 WHERE id = $2', [ + status, + id, + ]); +} + +/** + * Update environment metadata (merge with existing) + */ +export async function updateMetadata(id: string, metadata: Record): Promise { + await pool.query( + `UPDATE remote_agent_isolation_environments + SET metadata = metadata || $1::jsonb + WHERE id = $2`, + [JSON.stringify(metadata), id] + ); +} + +/** + * Find environments by related issue (from metadata) + */ +export async function findByRelatedIssue( + codebaseId: string, + issueNumber: number +): Promise { + const result = await pool.query( + `SELECT * FROM remote_agent_isolation_environments + WHERE codebase_id = $1 + AND status = 'active' + AND metadata->'related_issues' ? $2 + LIMIT 1`, + [codebaseId, String(issueNumber)] + ); + return result.rows[0] ?? null; +} + +/** + * Count active environments for a codebase (for limit checks) + */ +export async function countByCodebase(codebaseId: string): Promise { + const result = await pool.query<{ count: string }>( + `SELECT COUNT(*) as count FROM remote_agent_isolation_environments + WHERE codebase_id = $1 AND status = 'active'`, + [codebaseId] + ); + return parseInt(result.rows[0]?.count ?? '0', 10); +} + +/** + * Find conversations using an isolation environment + */ +export async function getConversationsUsingEnv(envId: string): Promise { + const result = await pool.query<{ id: string }>( + 'SELECT id FROM remote_agent_conversations WHERE isolation_env_id = $1', + [envId] + ); + return result.rows.map(r => r.id); +} diff --git a/src/handlers/command-handler.test.ts b/src/handlers/command-handler.test.ts index 2012b488..61bd6106 100644 --- a/src/handlers/command-handler.test.ts +++ b/src/handlers/command-handler.test.ts @@ -852,14 +852,21 @@ describe('CommandHandler', () => { expect(result.success).toBe(true); expect(result.message).toContain('Repository cloned successfully'); expect(result.message).toContain('✓ Loaded 2 commands'); - const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [string, Record]; + const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [ + string, + Record, + ]; expect(updateCall[0]).toBe('cb-new'); const commands = updateCall[1]; expect(commands['test-command']).toBeDefined(); expect(commands['another-command']).toBeDefined(); // Check path ends with expected relative path (platform-agnostic) - expect(commands['test-command'].path).toMatch(/\.claude[\\\/]commands[\\\/]test-command\.md$/); - expect(commands['another-command'].path).toMatch(/\.claude[\\\/]commands[\\\/]another-command\.md$/); + expect(commands['test-command'].path).toMatch( + /\.claude[\\\/]commands[\\\/]test-command\.md$/ + ); + expect(commands['another-command'].path).toMatch( + /\.claude[\\\/]commands[\\\/]another-command\.md$/ + ); expect(commands['test-command'].description).toBe('From .claude/commands'); expect(commands['another-command'].description).toBe('From .claude/commands'); }); @@ -886,7 +893,10 @@ describe('CommandHandler', () => { expect(result.success).toBe(true); expect(result.message).toContain('✓ Loaded 1 commands'); - const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [string, Record]; + const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [ + string, + Record, + ]; expect(updateCall[0]).toBe('cb-new'); const commands = updateCall[1]; expect(commands.rca).toBeDefined(); @@ -1023,7 +1033,10 @@ describe('CommandHandler', () => { // Verify commands were updated expect(mockUpdateCodebaseCommands).toHaveBeenCalled(); - const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [string, Record]; + const updateCall = mockUpdateCodebaseCommands.mock.calls[0] as [ + string, + Record, + ]; const commands = updateCall[1]; // Should preserve existing command diff --git a/src/isolation/providers/worktree.test.ts b/src/isolation/providers/worktree.test.ts index e9d05e99..3408f370 100644 --- a/src/isolation/providers/worktree.test.ts +++ b/src/isolation/providers/worktree.test.ts @@ -28,7 +28,7 @@ describe('WorktreeProvider', () => { worktreeExistsSpy.mockResolvedValue(false); listWorktreesSpy.mockResolvedValue([]); findWorktreeByBranchSpy.mockResolvedValue(null); - getCanonicalRepoPathSpy.mockImplementation(async (path) => path); + getCanonicalRepoPathSpy.mockImplementation(async path => path); }); afterEach(() => { @@ -150,7 +150,15 @@ describe('WorktreeProvider', () => { // Verify git worktree add was called with -b flag expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'add', expect.any(String), '-b', 'issue-42']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'add', + expect.any(String), + '-b', + 'issue-42', + ]), expect.any(Object) ); }); @@ -176,14 +184,28 @@ describe('WorktreeProvider', () => { // Verify worktree add with SHA expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'add', expect.any(String), 'abc123def456']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'add', + expect.any(String), + 'abc123def456', + ]), expect.any(Object) ); // Verify checkout -b for tracking branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', expect.any(String), 'checkout', '-b', 'pr-42-review', 'abc123def456']), + expect.arrayContaining([ + '-C', + expect.any(String), + 'checkout', + '-b', + 'pr-42-review', + 'abc123def456', + ]), expect.any(Object) ); }); @@ -201,14 +223,27 @@ describe('WorktreeProvider', () => { // Verify fetch with PR ref and local branch creation expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'fetch', 'origin', 'pull/42/head:pr-42-review']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'fetch', + 'origin', + 'pull/42/head:pr-42-review', + ]), expect.any(Object) ); // Verify worktree add with the local branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'add', expect.any(String), 'pr-42-review']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'add', + expect.any(String), + 'pr-42-review', + ]), expect.any(Object) ); }); @@ -262,7 +297,9 @@ describe('WorktreeProvider', () => { callCount++; // First worktree add call fails (branch exists) if (callCount === 1 && args.includes('-b')) { - const error = new Error('fatal: A branch named issue-42 already exists.') as Error & { stderr?: string }; + const error = new Error('fatal: A branch named issue-42 already exists.') as Error & { + stderr?: string; + }; error.stderr = 'fatal: A branch named issue-42 already exists.'; throw error; } @@ -274,14 +311,29 @@ describe('WorktreeProvider', () => { // Verify first call attempted new branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'add', expect.any(String), '-b', 'issue-42']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'add', + expect.any(String), + '-b', + 'issue-42', + ]), expect.any(Object) ); // Verify second call used existing branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'add', expect.any(String), 'issue-42']), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'add', + expect.any(String), + 'issue-42', + ]), expect.any(Object) ); }); @@ -301,7 +353,9 @@ describe('WorktreeProvider', () => { return { stdout: '', stderr: '' }; }); - await expect(provider.create(request)).rejects.toThrow('Failed to create worktree for PR #42'); + await expect(provider.create(request)).rejects.toThrow( + 'Failed to create worktree for PR #42' + ); }); }); @@ -331,7 +385,14 @@ describe('WorktreeProvider', () => { expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', '/workspace/repo', 'worktree', 'remove', '--force', worktreePath]), + expect.arrayContaining([ + '-C', + '/workspace/repo', + 'worktree', + 'remove', + '--force', + worktreePath, + ]), expect.any(Object) ); }); diff --git a/src/isolation/providers/worktree.ts b/src/isolation/providers/worktree.ts index 9e15f4f5..58990804 100644 --- a/src/isolation/providers/worktree.ts +++ b/src/isolation/providers/worktree.ts @@ -278,9 +278,13 @@ export class WorktreeProvider implements IIsolationProvider { }); // Create worktree at the specific SHA - await execFileAsync('git', ['-C', repoPath, 'worktree', 'add', worktreePath, request.prSha], { - timeout: 30000, - }); + await execFileAsync( + 'git', + ['-C', repoPath, 'worktree', 'add', worktreePath, request.prSha], + { + timeout: 30000, + } + ); // Create a local tracking branch so it's not detached HEAD await execFileAsync( @@ -325,9 +329,13 @@ export class WorktreeProvider implements IIsolationProvider { ): Promise { try { // Try to create with new branch - await execFileAsync('git', ['-C', repoPath, 'worktree', 'add', worktreePath, '-b', branchName], { - timeout: 30000, - }); + await execFileAsync( + 'git', + ['-C', repoPath, 'worktree', 'add', worktreePath, '-b', branchName], + { + timeout: 30000, + } + ); } catch (error) { const err = error as Error & { stderr?: string }; // Branch already exists - use existing branch diff --git a/src/orchestrator/orchestrator.test.ts b/src/orchestrator/orchestrator.test.ts index 2d88c021..5b239b7d 100644 --- a/src/orchestrator/orchestrator.test.ts +++ b/src/orchestrator/orchestrator.test.ts @@ -6,6 +6,7 @@ import { join } from 'path'; // Setup mocks before importing the module under test const mockGetOrCreateConversation = mock(() => Promise.resolve(null)); const mockUpdateConversation = mock(() => Promise.resolve()); +const mockTouchConversation = mock(() => Promise.resolve()); const mockGetCodebase = mock(() => Promise.resolve(null)); const mockGetActiveSession = mock(() => Promise.resolve(null)); const mockCreateSession = mock(() => Promise.resolve(null)); @@ -20,11 +21,57 @@ const mockParseCommand = mock((message: string) => { }); const mockGetAssistantClient = mock(() => null); const mockReadFile = mock(() => Promise.resolve('')); -const mockAccess = mock(() => Promise.resolve()); + +// Isolation environment mocks +const mockIsolationEnvGetById = mock(() => Promise.resolve(null)); +const mockIsolationEnvFindByWorkflow = mock(() => Promise.resolve(null)); +const mockIsolationEnvCreate = mock(() => Promise.resolve(null)); +const mockIsolationEnvUpdateStatus = mock(() => Promise.resolve()); + +// Git utils mocks +const mockWorktreeExists = mock(() => Promise.resolve(false)); +const mockFindWorktreeByBranch = mock(() => Promise.resolve(null)); +const mockGetCanonicalRepoPath = mock((path: string) => Promise.resolve(path)); +const mockExecFileAsync = mock(() => Promise.resolve({ stdout: 'main', stderr: '' })); + +// Isolation provider mock +const mockIsolationProviderCreate = mock(() => + Promise.resolve({ + id: 'env-123', + provider: 'worktree', + workingPath: '/workspace/worktrees/test/thread-abc', + branchName: 'thread-abc', + status: 'active', + createdAt: new Date(), + metadata: {}, + }) +); +const mockGetIsolationProvider = mock(() => ({ + create: mockIsolationProviderCreate, +})); mock.module('../db/conversations', () => ({ getOrCreateConversation: mockGetOrCreateConversation, updateConversation: mockUpdateConversation, + touchConversation: mockTouchConversation, +})); + +mock.module('../db/isolation-environments', () => ({ + getById: mockIsolationEnvGetById, + findByWorkflow: mockIsolationEnvFindByWorkflow, + create: mockIsolationEnvCreate, + updateStatus: mockIsolationEnvUpdateStatus, +})); + +mock.module('../utils/git', () => ({ + worktreeExists: mockWorktreeExists, + findWorktreeByBranch: mockFindWorktreeByBranch, + getCanonicalRepoPath: mockGetCanonicalRepoPath, + execFileAsync: mockExecFileAsync, +})); + +mock.module('../isolation', () => ({ + getIsolationProvider: mockGetIsolationProvider, })); mock.module('../db/codebases', () => ({ @@ -54,7 +101,6 @@ mock.module('../clients/factory', () => ({ mock.module('fs/promises', () => ({ readFile: mockReadFile, - access: mockAccess, })); import { handleMessage } from './orchestrator'; @@ -84,7 +130,11 @@ describe('orchestrator', () => { ai_assistant_type: 'claude', codebase_id: 'codebase-789', cwd: '/workspace/project', - worktree_path: null, + worktree_path: '/workspace/project', // Simulate existing worktree + isolation_env_id_legacy: null, + isolation_env_id: 'env-existing', // Simulate existing isolation + isolation_provider: 'worktree', + last_activity_at: null, created_at: new Date(), updated_at: new Date(), }; @@ -126,6 +176,7 @@ describe('orchestrator', () => { platform = new MockPlatformAdapter(); mockGetOrCreateConversation.mockClear(); mockUpdateConversation.mockClear(); + mockTouchConversation.mockClear(); mockGetCodebase.mockClear(); mockGetActiveSession.mockClear(); mockCreateSession.mockClear(); @@ -137,10 +188,21 @@ describe('orchestrator', () => { mockParseCommand.mockClear(); mockGetAssistantClient.mockClear(); mockReadFile.mockClear(); - mockAccess.mockClear(); mockClient.sendQuery.mockClear(); mockClient.getType.mockClear(); + // New isolation mocks + mockIsolationEnvGetById.mockClear(); + mockIsolationEnvFindByWorkflow.mockClear(); + mockIsolationEnvCreate.mockClear(); + mockIsolationEnvUpdateStatus.mockClear(); + mockWorktreeExists.mockClear(); + mockFindWorktreeByBranch.mockClear(); + mockGetCanonicalRepoPath.mockClear(); + mockExecFileAsync.mockClear(); + mockIsolationProviderCreate.mockClear(); + mockGetIsolationProvider.mockClear(); + // Default mocks mockGetOrCreateConversation.mockResolvedValue(mockConversation); mockGetCodebase.mockResolvedValue(mockCodebase); @@ -148,11 +210,42 @@ describe('orchestrator', () => { mockCreateSession.mockResolvedValue(mockSession); mockGetTemplate.mockResolvedValue(null); // No templates by default mockGetAssistantClient.mockReturnValue(mockClient); - mockAccess.mockResolvedValue(undefined); // Path exists by default mockParseCommand.mockImplementation((message: string) => { const parts = message.split(/\s+/); return { command: parts[0].substring(1), args: parts.slice(1) }; }); + + // Default isolation mocks - simulate existing isolation env + mockIsolationEnvGetById.mockResolvedValue({ + id: 'env-existing', + codebase_id: 'codebase-789', + workflow_type: 'thread', + workflow_id: 'chat-456', + provider: 'worktree', + working_path: '/workspace/project', + branch_name: 'thread-chat-456', + status: 'active', + created_at: new Date(), + created_by_platform: 'telegram', + metadata: {}, + }); + mockIsolationEnvFindByWorkflow.mockResolvedValue(null); + mockIsolationEnvCreate.mockResolvedValue({ + id: 'env-new', + codebase_id: 'codebase-789', + workflow_type: 'thread', + workflow_id: 'chat-456', + provider: 'worktree', + working_path: '/workspace/worktrees/test/thread-chat-456', + branch_name: 'thread-chat-456', + status: 'active', + created_at: new Date(), + created_by_platform: 'telegram', + metadata: {}, + }); + mockWorktreeExists.mockResolvedValue(true); // Existing worktree valid + mockGetCanonicalRepoPath.mockImplementation((path: string) => Promise.resolve(path)); + mockExecFileAsync.mockResolvedValue({ stdout: 'main', stderr: '' }); }); describe('slash commands (non-invoke)', () => { @@ -538,11 +631,34 @@ describe('orchestrator', () => { ); }); - test('falls back to codebase default_cwd', async () => { + test('falls back to codebase default_cwd when no isolation env', async () => { + // Conversation without isolation, will get auto-created mockGetOrCreateConversation.mockResolvedValue({ ...mockConversation, + isolation_env_id: null, + worktree_path: null, cwd: null, }); + + // No isolation env in DB, no existing worktree + mockIsolationEnvGetById.mockResolvedValue(null); + mockWorktreeExists.mockResolvedValue(false); + + // Auto-create will be triggered, returns a new env + mockIsolationEnvCreate.mockResolvedValue({ + id: 'env-auto-created', + codebase_id: 'codebase-789', + workflow_type: 'thread', + workflow_id: 'chat-456', + provider: 'worktree', + working_path: '/workspace/test-project/worktrees/thread-chat-456', + branch_name: 'thread-chat-456', + status: 'active', + created_at: new Date(), + created_by_platform: 'telegram', + metadata: {}, + }); + mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); mockReadFile.mockResolvedValue('Plan command'); mockClient.sendQuery.mockImplementation(async function* () { @@ -551,20 +667,38 @@ describe('orchestrator', () => { await handleMessage(platform, 'chat-456', '/command-invoke plan'); + // Uses the auto-created worktree path expect(mockClient.sendQuery).toHaveBeenCalledWith( wrapCommandForExecution('plan', 'Plan command'), - '/workspace/test-project', // codebase.default_cwd - 'claude-session-xyz' // Uses existing session's ID + '/workspace/test-project/worktrees/thread-chat-456', // From auto-created env + 'claude-session-xyz' ); }); - test('uses isolation_env_id over worktree_path and cwd', async () => { + test('uses isolation_env_id (UUID) to look up working path', async () => { + // conversation has a UUID isolation_env_id mockGetOrCreateConversation.mockResolvedValue({ ...mockConversation, - isolation_env_id: '/workspace/isolation-env', + isolation_env_id: 'env-priority', worktree_path: '/workspace/old-worktree', cwd: '/workspace/project', }); + + // Mock the env lookup to return a specific working_path + mockIsolationEnvGetById.mockResolvedValue({ + id: 'env-priority', + codebase_id: 'codebase-789', + workflow_type: 'thread', + workflow_id: 'chat-456', + provider: 'worktree', + working_path: '/workspace/isolation-env', // This is the path to use + branch_name: 'thread-chat-456', + status: 'active', + created_at: new Date(), + created_by_platform: 'telegram', + metadata: {}, + }); + mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); mockReadFile.mockResolvedValue('Plan command'); mockClient.sendQuery.mockImplementation(async function* () { @@ -573,9 +707,10 @@ describe('orchestrator', () => { await handleMessage(platform, 'chat-456', '/command-invoke plan'); + // Env lookup is used, working_path from env takes priority expect(mockClient.sendQuery).toHaveBeenCalledWith( wrapCommandForExecution('plan', 'Plan command'), - '/workspace/isolation-env', // isolation_env_id takes priority + '/workspace/isolation-env', // working_path from isolation env 'claude-session-xyz' ); }); @@ -604,64 +739,32 @@ describe('orchestrator', () => { }); describe('stale worktree handling', () => { - test('should deactivate session and clear worktree when cwd does not exist', async () => { - // Setup: conversation with worktree_path that doesn't exist - const conversationWithStaleWorktree = { + test('should clear isolation fields when isolation_env_id points to non-existent path', async () => { + // Setup: conversation with isolation_env_id pointing to stale env + const conversationWithStaleIsolation = { ...mockConversation, + isolation_env_id: 'env-stale', + isolation_provider: 'worktree', worktree_path: '/nonexistent/worktree/path', cwd: '/nonexistent/worktree/path', }; - mockGetOrCreateConversation.mockResolvedValue(conversationWithStaleWorktree); - - // Mock fs.access to throw (path doesn't exist) - mockAccess.mockRejectedValueOnce(new Error('ENOENT: no such file or directory')); - - // Mock active session - mockGetActiveSession.mockResolvedValue({ - ...mockSession, - id: 'stale-session-id', - }); - - mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); - mockReadFile.mockResolvedValue('Plan command'); - mockClient.sendQuery.mockImplementation(async function* () { - yield { type: 'result', sessionId: 'session-id' }; - }); - - await handleMessage(platform, 'chat-456', '/command-invoke plan'); - - // Verify session was deactivated - expect(mockDeactivateSession).toHaveBeenCalledWith('stale-session-id'); - - // Verify worktree_path was cleared - expect(mockUpdateConversation).toHaveBeenCalledWith( - 'conv-123', - expect.objectContaining({ - worktree_path: null, - cwd: '/workspace/test-project', // Falls back to codebase default_cwd - }) - ); - }); - - test('should clear all isolation fields when isolation_env_id is stale', async () => { - // Setup: conversation with isolation_env_id that doesn't exist - const conversationWithStaleIsolation = { - ...mockConversation, - isolation_env_id: '/nonexistent/isolation/path', - isolation_provider: 'worktree', - worktree_path: null, - cwd: '/workspace/project', - }; mockGetOrCreateConversation.mockResolvedValue(conversationWithStaleIsolation); - // Mock fs.access to throw (path doesn't exist) - mockAccess.mockRejectedValueOnce(new Error('ENOENT: no such file or directory')); - - // Mock active session - mockGetActiveSession.mockResolvedValue({ - ...mockSession, - id: 'stale-session-id', + // Mock: env exists in DB but path doesn't exist on disk + mockIsolationEnvGetById.mockResolvedValue({ + id: 'env-stale', + codebase_id: 'codebase-789', + workflow_type: 'thread', + workflow_id: 'chat-456', + provider: 'worktree', + working_path: '/nonexistent/worktree/path', + branch_name: 'thread-chat-456', + status: 'active', + created_at: new Date(), + created_by_platform: 'telegram', + metadata: {}, }); + mockWorktreeExists.mockResolvedValue(false); // Path doesn't exist mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); mockReadFile.mockResolvedValue('Plan command'); @@ -671,58 +774,22 @@ describe('orchestrator', () => { await handleMessage(platform, 'chat-456', '/command-invoke plan'); - // Verify all isolation fields are cleared + // Verify isolation fields are cleared expect(mockUpdateConversation).toHaveBeenCalledWith( 'conv-123', expect.objectContaining({ worktree_path: null, isolation_env_id: null, isolation_provider: null, - cwd: '/workspace/test-project', }) ); + + // Verify env marked as destroyed + expect(mockIsolationEnvUpdateStatus).toHaveBeenCalledWith('env-stale', 'destroyed'); }); - test('should use default cwd when worktree path is stale', async () => { - // Setup: conversation with worktree_path that doesn't exist - const conversationWithStaleWorktree = { - ...mockConversation, - worktree_path: '/nonexistent/worktree/path', - cwd: '/nonexistent/worktree/path', - }; - mockGetOrCreateConversation.mockResolvedValue(conversationWithStaleWorktree); - - // Mock fs.access to throw (path doesn't exist) - mockAccess.mockRejectedValueOnce(new Error('ENOENT')); - - // Create a new session without assistant_session_id (simulating fresh start) - const freshSession = { - ...mockSession, - assistant_session_id: null, - }; - mockCreateSession.mockResolvedValue(freshSession); - - mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); - mockReadFile.mockResolvedValue('Plan command'); - mockClient.sendQuery.mockImplementation(async function* () { - yield { type: 'result', sessionId: 'session-id' }; - }); - - await handleMessage(platform, 'chat-456', '/command-invoke plan'); - - // Verify AI client was called with the fallback cwd - expect(mockClient.sendQuery).toHaveBeenCalledWith( - wrapCommandForExecution('plan', 'Plan command'), - '/workspace/test-project', // Falls back to codebase default_cwd - undefined // New session created (assistant_session_id is null -> undefined) - ); - }); - - test('should not deactivate session if cwd exists', async () => { - // Mock fs.access to succeed (path exists) - mockAccess.mockResolvedValue(undefined); - - // Mock active session + test('should not clear isolation if path exists', async () => { + // Default setup: valid isolation env mockGetActiveSession.mockResolvedValue(mockSession); mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); @@ -733,24 +800,17 @@ describe('orchestrator', () => { await handleMessage(platform, 'chat-456', '/command-invoke plan'); - // Verify session was NOT deactivated (for stale worktree reason) - // Note: It might be deactivated for plan→execute transition, but not for stale worktree + // Verify session was NOT deactivated (isolation is valid) + // updateConversation should NOT be called with null isolation fields expect(mockUpdateConversation).not.toHaveBeenCalledWith( expect.any(String), - expect.objectContaining({ worktree_path: null }) + expect.objectContaining({ worktree_path: null, isolation_env_id: null }) ); }); - test('should handle conversation without worktree_path gracefully', async () => { - // Conversation without worktree_path - mockGetOrCreateConversation.mockResolvedValue({ - ...mockConversation, - worktree_path: null, - cwd: '/workspace/project', - }); - - // cwd exists - mockAccess.mockResolvedValue(undefined); + test('should use existing valid isolation environment', async () => { + // Default mocks already set up valid isolation + mockGetActiveSession.mockResolvedValue(mockSession); mockParseCommand.mockReturnValue({ command: 'command-invoke', args: ['plan'] }); mockReadFile.mockResolvedValue('Plan command'); @@ -760,10 +820,10 @@ describe('orchestrator', () => { await handleMessage(platform, 'chat-456', '/command-invoke plan'); - // Should work normally, no worktree cleanup needed + // Should work normally with existing isolation expect(mockClient.sendQuery).toHaveBeenCalledWith( wrapCommandForExecution('plan', 'Plan command'), - '/workspace/project', + '/workspace/project', // From existing env working_path 'claude-session-xyz' ); }); diff --git a/src/orchestrator/orchestrator.ts b/src/orchestrator/orchestrator.ts index ce0b4b43..3e358d36 100644 --- a/src/orchestrator/orchestrator.ts +++ b/src/orchestrator/orchestrator.ts @@ -2,18 +2,274 @@ * Orchestrator - Main conversation handler * Routes slash commands and AI messages appropriately */ -import { readFile, access } from 'fs/promises'; +import { readFile } from 'fs/promises'; import { join } from 'path'; -import { IPlatformAdapter } from '../types'; +import { + IPlatformAdapter, + IsolationHints, + IsolationEnvironmentRow, + Conversation, + Codebase, +} from '../types'; import * as db from '../db/conversations'; import * as codebaseDb from '../db/codebases'; import * as sessionDb from '../db/sessions'; import * as templateDb from '../db/command-templates'; +import * as isolationEnvDb from '../db/isolation-environments'; import * as commandHandler from '../handlers/command-handler'; import { formatToolCall } from '../utils/tool-formatter'; import { substituteVariables } from '../utils/variable-substitution'; import { classifyAndFormatError } from '../utils/error-formatter'; import { getAssistantClient } from '../clients/factory'; +import { getIsolationProvider } from '../isolation'; +import { + worktreeExists, + findWorktreeByBranch, + getCanonicalRepoPath, + execFileAsync, +} from '../utils/git'; + +/** + * Validate existing isolation and create new if needed + * This is the single source of truth for isolation decisions + */ +async function validateAndResolveIsolation( + conversation: Conversation, + codebase: Codebase | null, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise<{ cwd: string; env: IsolationEnvironmentRow | null; isNew: boolean }> { + // 1. Check existing isolation reference (new UUID model) + if (conversation.isolation_env_id) { + const env = await isolationEnvDb.getById(conversation.isolation_env_id); + + if (env && (await worktreeExists(env.working_path))) { + // Valid - use it + return { cwd: env.working_path, env, isNew: false }; + } + + // Stale reference - clean up + console.warn(`[Orchestrator] Stale isolation: ${conversation.isolation_env_id}`); + await db.updateConversation(conversation.id, { + isolation_env_id: null, + worktree_path: null, + isolation_provider: null, + }); + + if (env) { + await isolationEnvDb.updateStatus(env.id, 'destroyed'); + } + } + + // 2. Legacy fallback (worktree_path without new UUID) + const legacyPath = conversation.worktree_path ?? conversation.isolation_env_id_legacy; + if (legacyPath && (await worktreeExists(legacyPath))) { + // Migrate to new model on-the-fly + const env = await migrateToIsolationEnvironment(conversation, codebase, legacyPath, platform); + if (env) { + return { cwd: legacyPath, env, isNew: false }; + } + } + + // 3. No valid isolation - check if we should create + if (!codebase) { + return { cwd: conversation.cwd ?? '/workspace', env: null, isNew: false }; + } + + // 4. Create new isolation (auto-isolation for all platforms!) + const env = await resolveIsolation(codebase, platform, conversationId, hints); + if (env) { + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + worktree_path: env.working_path, + isolation_provider: env.provider, + cwd: env.working_path, + }); + return { cwd: env.working_path, env, isNew: true }; + } + + return { cwd: codebase.default_cwd, env: null, isNew: false }; +} + +/** + * Resolve which isolation environment to use + * Handles reuse, sharing, adoption, and creation + */ +async function resolveIsolation( + codebase: Codebase, + platform: IPlatformAdapter, + conversationId: string, + hints?: IsolationHints +): Promise { + // Determine workflow identity + const workflowType = hints?.workflowType ?? 'thread'; + const workflowId = hints?.workflowId ?? conversationId; + + // 1. Check for existing environment with same workflow + const existing = await isolationEnvDb.findByWorkflow(codebase.id, workflowType, workflowId); + if (existing && (await worktreeExists(existing.working_path))) { + console.log(`[Orchestrator] Reusing environment for ${workflowType}/${workflowId}`); + return existing; + } + + // 2. Check linked issues for sharing (cross-conversation) + if (hints?.linkedIssues?.length) { + for (const issueNum of hints.linkedIssues) { + const linkedEnv = await isolationEnvDb.findByWorkflow(codebase.id, 'issue', String(issueNum)); + if (linkedEnv && (await worktreeExists(linkedEnv.working_path))) { + console.log(`[Orchestrator] Sharing worktree with linked issue #${String(issueNum)}`); + // Send UX message + await platform.sendMessage( + conversationId, + `Reusing worktree from issue #${String(issueNum)}` + ); + return linkedEnv; + } + } + } + + // 3. Try PR branch adoption (skill symbiosis) + if (hints?.prBranch) { + const canonicalPath = await getCanonicalRepoPath(codebase.default_cwd); + const adoptedPath = await findWorktreeByBranch(canonicalPath, hints.prBranch); + if (adoptedPath && (await worktreeExists(adoptedPath))) { + console.log(`[Orchestrator] Adopting existing worktree at ${adoptedPath}`); + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: adoptedPath, + branch_name: hints.prBranch, + created_by_platform: platform.getPlatformType(), + metadata: { adopted: true, adopted_from: 'skill' }, + }); + return env; + } + } + + // 4. Create new worktree + const provider = getIsolationProvider(); + const canonicalPath = await getCanonicalRepoPath(codebase.default_cwd); + + try { + const isolatedEnv = await provider.create({ + codebaseId: codebase.id, + canonicalRepoPath: canonicalPath, + workflowType, + identifier: workflowId, + prBranch: hints?.prBranch, + prSha: hints?.prSha, + }); + + // Create database record + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: isolatedEnv.workingPath, + branch_name: isolatedEnv.branchName ?? `${workflowType}-${workflowId}`, + created_by_platform: platform.getPlatformType(), + metadata: { + related_issues: hints?.linkedIssues ?? [], + related_prs: hints?.linkedPRs ?? [], + }, + }); + + // UX message + if (hints?.prSha) { + const shortSha = hints.prSha.substring(0, 7); + await platform.sendMessage( + conversationId, + `Reviewing PR at commit \`${shortSha}\` (branch: \`${hints.prBranch}\`)` + ); + } else { + await platform.sendMessage( + conversationId, + `Working in isolated branch \`${env.branch_name}\`` + ); + } + + return env; + } catch (error) { + console.error('[Orchestrator] Failed to create isolation:', error); + return null; + } +} + +/** + * Migrate a legacy worktree_path to the new isolation_environments model + */ +async function migrateToIsolationEnvironment( + conversation: Conversation, + codebase: Codebase | null, + legacyPath: string, + platform: IPlatformAdapter +): Promise { + if (!codebase) return null; + + try { + const { workflowType, workflowId } = inferWorkflowFromConversation(conversation, legacyPath); + const branchName = await getBranchNameFromWorktree(legacyPath); + + const env = await isolationEnvDb.create({ + codebase_id: codebase.id, + workflow_type: workflowType, + workflow_id: workflowId, + working_path: legacyPath, + branch_name: branchName, + created_by_platform: platform.getPlatformType(), + metadata: { migrated: true, migrated_at: new Date().toISOString() }, + }); + + await db.updateConversation(conversation.id, { + isolation_env_id: env.id, + }); + + console.log(`[Orchestrator] Migrated legacy worktree to environment: ${env.id}`); + return env; + } catch (error) { + console.error('[Orchestrator] Failed to migrate legacy worktree:', error); + return null; + } +} + +function inferWorkflowFromConversation( + conversation: Conversation, + legacyPath: string +): { workflowType: string; workflowId: string } { + // Try to infer from platform conversation ID + if (conversation.platform_type === 'github') { + const match = /#(\d+)$/.exec(conversation.platform_conversation_id); + if (match) { + const isPR = legacyPath.includes('/pr-') || legacyPath.includes('-pr-'); + return { + workflowType: isPR ? 'pr' : 'issue', + workflowId: match[1], + }; + } + } + + return { + workflowType: 'thread', + workflowId: conversation.platform_conversation_id, + }; +} + +async function getBranchNameFromWorktree(path: string): Promise { + try { + const { stdout } = await execFileAsync('git', [ + '-C', + path, + 'rev-parse', + '--abbrev-ref', + 'HEAD', + ]); + return stdout.trim(); + } catch { + return 'unknown'; + } +} /** * Wraps command content with execution context to signal the AI should execute immediately @@ -39,7 +295,8 @@ export async function handleMessage( message: string, issueContext?: string, // Optional GitHub issue/PR context to append AFTER command loading threadContext?: string, // Optional thread message history for context - parentConversationId?: string // Optional parent channel ID for thread inheritance + parentConversationId?: string, // Optional parent channel ID for thread inheritance + isolationHints?: IsolationHints // Optional hints from adapter for isolation decisions ): Promise { try { console.log(`[Orchestrator] Handling message for conversation ${conversationId}`); @@ -147,9 +404,9 @@ export async function handleMessage( return; } - // Read command file - const cwd = conversation.isolation_env_id ?? conversation.worktree_path ?? conversation.cwd ?? codebase.default_cwd; - const commandFilePath = join(cwd, commandDef.path); + // Read command file (use worktree_path or cwd for command location) + const commandCwd = conversation.worktree_path ?? conversation.cwd ?? codebase.default_cwd; + const commandFilePath = join(commandCwd, commandDef.path); try { const commandText = await readFile(commandFilePath, 'utf-8'); @@ -231,42 +488,33 @@ export async function handleMessage( const aiClient = getAssistantClient(conversation.ai_assistant_type); console.log(`[Orchestrator] Using ${conversation.ai_assistant_type} assistant`); - // Get or create session (handle plan→execute transition) - let session = await sessionDb.getActiveSession(conversation.id); + // Get codebase for isolation and session management const codebase = conversation.codebase_id ? await codebaseDb.getCodebase(conversation.codebase_id) : null; - let cwd = - conversation.isolation_env_id ?? conversation.worktree_path ?? conversation.cwd ?? codebase?.default_cwd ?? '/workspace'; - // Validate cwd exists - handle stale worktree paths gracefully - try { - await access(cwd); - } catch { - console.warn(`[Orchestrator] Working directory ${cwd} does not exist`); + // Validate and resolve isolation - this is the single source of truth + const { cwd, isNew: isNewIsolation } = await validateAndResolveIsolation( + conversation, + codebase, + platform, + conversationId, + isolationHints + ); - // Deactivate stale session to force fresh start - if (session) { - await sessionDb.deactivateSession(session.id); - session = null; - console.log('[Orchestrator] Deactivated session with stale worktree'); - } + // Get or create session (handle plan→execute transition) + let session = await sessionDb.getActiveSession(conversation.id); - // Clear stale isolation reference from conversation - if (conversation.isolation_env_id || conversation.worktree_path) { - await db.updateConversation(conversation.id, { - worktree_path: null, - isolation_env_id: null, - isolation_provider: null, - cwd: codebase?.default_cwd ?? '/workspace', - }); - console.log('[Orchestrator] Cleared stale isolation environment from conversation'); - } - - // Use default cwd for this request - cwd = codebase?.default_cwd ?? '/workspace'; + // If cwd changed (new isolation), deactivate stale sessions + if (isNewIsolation && session) { + console.log('[Orchestrator] New isolation, deactivating existing session'); + await sessionDb.deactivateSession(session.id); + session = null; } + // Update last_activity_at for staleness tracking + await db.touchConversation(conversation.id); + // Check for plan→execute transition (requires NEW session per PRD) // Supports both regular and GitHub workflows: // - plan-feature → execute (regular workflow) diff --git a/src/services/cleanup-service.test.ts b/src/services/cleanup-service.test.ts new file mode 100644 index 00000000..5f3f0e2b --- /dev/null +++ b/src/services/cleanup-service.test.ts @@ -0,0 +1,170 @@ +import { mock, describe, test, expect, beforeEach } from 'bun:test'; + +// Mock git utility +const mockExecFileAsync = mock(() => Promise.resolve({ stdout: '', stderr: '' })); +mock.module('../utils/git', () => ({ + execFileAsync: mockExecFileAsync, +})); + +// Mock isolation provider +const mockDestroy = mock(() => Promise.resolve()); +mock.module('../isolation', () => ({ + getIsolationProvider: () => ({ + destroy: mockDestroy, + }), +})); + +import { hasUncommittedChanges, isBranchMerged, getLastCommitDate } from './cleanup-service'; + +describe('cleanup-service', () => { + beforeEach(() => { + mockExecFileAsync.mockClear(); + mockDestroy.mockClear(); + }); + + describe('hasUncommittedChanges', () => { + test('returns true when git status shows changes', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' M file.ts\n', + stderr: '', + }); + + const result = await hasUncommittedChanges('/workspace/test'); + + expect(result).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith('git', [ + '-C', + '/workspace/test', + 'status', + '--porcelain', + ]); + }); + + test('returns false when git status is clean', async () => { + mockExecFileAsync.mockResolvedValueOnce({ stdout: '', stderr: '' }); + + const result = await hasUncommittedChanges('/workspace/test'); + + expect(result).toBe(false); + }); + + test('returns false when git fails (path not found)', async () => { + mockExecFileAsync.mockRejectedValueOnce(new Error('not a git repository')); + + const result = await hasUncommittedChanges('/nonexistent'); + + expect(result).toBe(false); + }); + + test('returns false when git status is only whitespace', async () => { + mockExecFileAsync.mockResolvedValueOnce({ stdout: ' \n', stderr: '' }); + + const result = await hasUncommittedChanges('/workspace/test'); + + expect(result).toBe(false); + }); + }); + + describe('isBranchMerged', () => { + test('returns true when branch is in merged list', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' feature-a\n issue-42\n* main\n', + stderr: '', + }); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(true); + expect(mockExecFileAsync).toHaveBeenCalledWith('git', [ + '-C', + '/workspace/repo', + 'branch', + '--merged', + 'main', + ]); + }); + + test('returns false when branch is not merged', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' feature-a\n* main\n', + stderr: '', + }); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(false); + }); + + test('returns false when git command fails', async () => { + mockExecFileAsync.mockRejectedValueOnce(new Error('git error')); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(false); + }); + + test('handles current branch marker (*)', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: '* issue-42\n main\n', + stderr: '', + }); + + const result = await isBranchMerged('/workspace/repo', 'issue-42'); + + expect(result).toBe(true); + }); + + test('uses custom main branch', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: ' issue-42\n master\n', + stderr: '', + }); + + await isBranchMerged('/workspace/repo', 'issue-42', 'master'); + + expect(mockExecFileAsync).toHaveBeenCalledWith('git', [ + '-C', + '/workspace/repo', + 'branch', + '--merged', + 'master', + ]); + }); + }); + + describe('getLastCommitDate', () => { + test('returns date from git log', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: '2025-01-15 10:30:00 +0000\n', + stderr: '', + }); + + const result = await getLastCommitDate('/workspace/test'); + + expect(result).toBeInstanceOf(Date); + expect(result?.getFullYear()).toBe(2025); + expect(result?.getMonth()).toBe(0); // January is 0 + expect(result?.getDate()).toBe(15); + }); + + test('returns null when git fails', async () => { + mockExecFileAsync.mockRejectedValueOnce(new Error('no commits')); + + const result = await getLastCommitDate('/workspace/test'); + + expect(result).toBeNull(); + }); + + test('handles different date formats', async () => { + mockExecFileAsync.mockResolvedValueOnce({ + stdout: '2024-12-25 23:59:59 -0500\n', + stderr: '', + }); + + const result = await getLastCommitDate('/workspace/test'); + + expect(result).toBeInstanceOf(Date); + expect(result?.getFullYear()).toBe(2024); + }); + }); +}); diff --git a/src/services/cleanup-service.ts b/src/services/cleanup-service.ts new file mode 100644 index 00000000..e06f0b45 --- /dev/null +++ b/src/services/cleanup-service.ts @@ -0,0 +1,192 @@ +/** + * Cleanup service for isolation environments + * Handles removal triggered by events, schedule, or commands + */ +import * as isolationEnvDb from '../db/isolation-environments'; +import * as conversationDb from '../db/conversations'; +import * as sessionDb from '../db/sessions'; +import { getIsolationProvider } from '../isolation'; +import { execFileAsync } from '../utils/git'; + +export interface CleanupReport { + removed: string[]; + skipped: { id: string; reason: string }[]; + errors: { id: string; error: string }[]; +} + +/** + * Called when a platform conversation is closed (e.g., GitHub issue/PR closed) + * Cleans up the associated isolation environment if no other conversations use it + */ +export async function onConversationClosed( + platformType: string, + platformConversationId: string +): Promise { + console.log(`[Cleanup] Conversation closed: ${platformType}/${platformConversationId}`); + + // Find the conversation + const conversation = await conversationDb.getConversationByPlatformId( + platformType, + platformConversationId + ); + + if (!conversation?.isolation_env_id) { + console.log('[Cleanup] No isolation environment to clean up'); + return; + } + + const envId = conversation.isolation_env_id; + + // Deactivate any active sessions first + const session = await sessionDb.getActiveSession(conversation.id); + if (session) { + await sessionDb.deactivateSession(session.id); + console.log(`[Cleanup] Deactivated session ${session.id}`); + } + + // Get the environment + const env = await isolationEnvDb.getById(envId); + if (!env) { + console.log(`[Cleanup] Environment ${envId} not found in database`); + return; + } + + // Clear this conversation's reference (regardless of whether we remove the worktree) + await conversationDb.updateConversation(conversation.id, { + isolation_env_id: null, + worktree_path: null, + isolation_provider: null, + // Keep cwd pointing to main repo (will be set by caller or orchestrator) + }); + + // Check if other conversations still use this environment + const otherConversations = await isolationEnvDb.getConversationsUsingEnv(envId); + if (otherConversations.length > 0) { + console.log( + `[Cleanup] Environment still used by ${String(otherConversations.length)} conversation(s), keeping` + ); + return; + } + + // No other users - attempt removal + await removeEnvironment(envId, { force: false }); +} + +/** + * Remove a specific environment + */ +export async function removeEnvironment( + envId: string, + options?: { force?: boolean } +): Promise { + const env = await isolationEnvDb.getById(envId); + if (!env) { + console.log(`[Cleanup] Environment ${envId} not found`); + return; + } + + if (env.status === 'destroyed') { + console.log(`[Cleanup] Environment ${envId} already destroyed`); + return; + } + + const provider = getIsolationProvider(); + + try { + // Check for uncommitted changes (unless force) + if (!options?.force) { + const hasChanges = await hasUncommittedChanges(env.working_path); + if (hasChanges) { + console.warn(`[Cleanup] Environment ${envId} has uncommitted changes, skipping`); + return; + } + } + + // Remove the worktree + await provider.destroy(env.working_path, { force: options?.force }); + + // Mark as destroyed in database + await isolationEnvDb.updateStatus(envId, 'destroyed'); + + console.log(`[Cleanup] Removed environment ${envId} at ${env.working_path}`); + } catch (error) { + const err = error as Error; + console.error(`[Cleanup] Failed to remove environment ${envId}:`, err.message); + throw err; + } +} + +/** + * Check if a worktree has uncommitted changes + */ +export async function hasUncommittedChanges(workingPath: string): Promise { + try { + const { stdout } = await execFileAsync('git', ['-C', workingPath, 'status', '--porcelain']); + return stdout.trim().length > 0; + } catch { + // If git fails, assume it's safe to remove (path might not exist) + return false; + } +} + +/** + * Check if a branch has been merged into main + */ +export async function isBranchMerged( + repoPath: string, + branchName: string, + mainBranch = 'main' +): Promise { + try { + const { stdout } = await execFileAsync('git', [ + '-C', + repoPath, + 'branch', + '--merged', + mainBranch, + ]); + const mergedBranches = stdout.split('\n').map(b => b.trim().replace(/^\* /, '')); + return mergedBranches.includes(branchName); + } catch { + return false; + } +} + +/** + * Get the last commit date for a worktree + */ +export async function getLastCommitDate(workingPath: string): Promise { + try { + const { stdout } = await execFileAsync('git', ['-C', workingPath, 'log', '-1', '--format=%ci']); + return new Date(stdout.trim()); + } catch { + return null; + } +} + +/** + * Clean up to make room when limit reached (Phase 3D will call this) + * Attempts to remove merged branches first + */ +export async function cleanupToMakeRoom(codebaseId: string, mainRepoPath: string): Promise { + const envs = await isolationEnvDb.listByCodebase(codebaseId); + let removed = 0; + + for (const env of envs) { + // Try merged branches first + const merged = await isBranchMerged(mainRepoPath, env.branch_name); + if (merged) { + const hasChanges = await hasUncommittedChanges(env.working_path); + if (!hasChanges) { + try { + await removeEnvironment(env.id); + removed++; + } catch { + // Continue to next + } + } + } + } + + return removed; +} diff --git a/src/types/index.ts b/src/types/index.ts index feb08646..906e02a3 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -8,14 +8,54 @@ export interface Conversation { platform_conversation_id: string; codebase_id: string | null; cwd: string | null; - worktree_path: string | null; - isolation_env_id: string | null; - isolation_provider: string | null; + worktree_path: string | null; // Legacy field + isolation_env_id_legacy: string | null; // Renamed from isolation_env_id (TEXT) + isolation_env_id: string | null; // NEW: UUID FK to isolation_environments + isolation_provider: string | null; // Legacy field ai_assistant_type: string; + last_activity_at: Date | null; // NEW: for staleness detection created_at: Date; updated_at: Date; } +/** + * Isolation hints provided by adapters to orchestrator + * Allows platform-specific context without orchestrator knowing platform internals + */ +export interface IsolationHints { + // Workflow identification (adapter knows this) + workflowType?: 'issue' | 'pr' | 'review' | 'thread' | 'task'; + workflowId?: string; + + // PR-specific (for reproducible reviews) + prBranch?: string; + prSha?: string; + + // Cross-reference hints (for linking) + linkedIssues?: number[]; + linkedPRs?: number[]; + + // Adoption hints + suggestedBranch?: string; +} + +/** + * Database row for isolation_environments table + */ +export interface IsolationEnvironmentRow { + id: string; + codebase_id: string; + workflow_type: string; + workflow_id: string; + provider: string; + working_path: string; + branch_name: string; + status: string; + created_at: Date; + created_by_platform: string | null; + metadata: Record; +} + export interface Codebase { id: string; name: string; diff --git a/src/utils/git.test.ts b/src/utils/git.test.ts index 398ce1d5..7ee02396 100644 --- a/src/utils/git.test.ts +++ b/src/utils/git.test.ts @@ -315,7 +315,14 @@ branch refs/heads/feature/auth // Verify worktree add was called with the local branch created from PR ref expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', repoPath, 'worktree', 'add', expect.any(String), 'pr-42-review']), + expect.arrayContaining([ + '-C', + repoPath, + 'worktree', + 'add', + expect.any(String), + 'pr-42-review', + ]), expect.any(Object) ); @@ -381,7 +388,9 @@ branch refs/heads/feature/auth callCount++; // First call: worktree add -b fails (branch exists) if (callCount === 1 && args.includes('-b')) { - const error = new Error('fatal: A branch named issue-42 already exists.') as Error & { stderr?: string }; + const error = new Error('fatal: A branch named issue-42 already exists.') as Error & { + stderr?: string; + }; error.stderr = 'fatal: A branch named issue-42 already exists.'; throw error; } @@ -394,7 +403,15 @@ branch refs/heads/feature/auth // Verify first call attempted to create new branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', repoPath, 'worktree', 'add', expect.any(String), '-b', 'issue-42']), + expect.arrayContaining([ + '-C', + repoPath, + 'worktree', + 'add', + expect.any(String), + '-b', + 'issue-42', + ]), expect.any(Object) ); @@ -418,9 +435,9 @@ branch refs/heads/feature/auth return { stdout: '', stderr: '' }; }); - await expect(git.createWorktreeForIssue(repoPath, issueNumber, true, prHeadBranch)).rejects.toThrow( - 'Failed to create worktree for PR #42' - ); + await expect( + git.createWorktreeForIssue(repoPath, issueNumber, true, prHeadBranch) + ).rejects.toThrow('Failed to create worktree for PR #42'); }); test('provides helpful error message with PR number', async () => { @@ -454,7 +471,15 @@ branch refs/heads/feature/auth // Verify worktree add was called with -b flag for new pr-XX branch expect(execSpy).toHaveBeenCalledWith( 'git', - expect.arrayContaining(['-C', repoPath, 'worktree', 'add', expect.any(String), '-b', 'pr-42']), + expect.arrayContaining([ + '-C', + repoPath, + 'worktree', + 'add', + expect.any(String), + '-b', + 'pr-42', + ]), expect.any(Object) ); diff --git a/src/utils/git.ts b/src/utils/git.ts index 5e83b368..d9918af7 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -21,10 +21,7 @@ export async function execFileAsync( } // Mockable mkdir wrapper -export async function mkdirAsync( - path: string, - options?: { recursive?: boolean } -): Promise { +export async function mkdirAsync(path: string, options?: { recursive?: boolean }): Promise { await fsMkdir(path, options); } @@ -199,9 +196,13 @@ export async function createWorktreeForIssue( if (prHeadSha) { // Fetch the specific commit SHA using PR refs (works for both fork and non-fork PRs) // GitHub creates refs/pull//head for all PRs automatically - await execFileAsync('git', ['-C', repoPath, 'fetch', 'origin', `pull/${String(issueNumber)}/head`], { - timeout: 30000, - }); + await execFileAsync( + 'git', + ['-C', repoPath, 'fetch', 'origin', `pull/${String(issueNumber)}/head`], + { + timeout: 30000, + } + ); // Create worktree at the specific SHA await execFileAsync('git', ['-C', repoPath, 'worktree', 'add', worktreePath, prHeadSha], { @@ -219,14 +220,28 @@ export async function createWorktreeForIssue( } else { // Use GitHub's PR refs which work for both fork and non-fork PRs // GitHub automatically creates refs/pull//head for all PRs - await execFileAsync('git', ['-C', repoPath, 'fetch', 'origin', `pull/${String(issueNumber)}/head:pr-${String(issueNumber)}-review`], { - timeout: 30000, - }); + await execFileAsync( + 'git', + [ + '-C', + repoPath, + 'fetch', + 'origin', + `pull/${String(issueNumber)}/head:pr-${String(issueNumber)}-review`, + ], + { + timeout: 30000, + } + ); // Create worktree using the fetched PR ref - await execFileAsync('git', ['-C', repoPath, 'worktree', 'add', worktreePath, `pr-${String(issueNumber)}-review`], { - timeout: 30000, - }); + await execFileAsync( + 'git', + ['-C', repoPath, 'worktree', 'add', worktreePath, `pr-${String(issueNumber)}-review`], + { + timeout: 30000, + } + ); } } catch (error) { const err = error as Error & { stderr?: string };