Merge pull request #36 from laurentenhoor/fix/27-persist-sessions-on-complete

fix: add defensive safeguards and documentation for session persistence

lib/projects.test.ts

@@ -0,0 +1,189 @@
+/**
+ * Tests for projects.ts session persistence
+ * Run with: npm test
+ */
+import { describe, it, before, after } from "node:test";
+import assert from "node:assert";
+import fs from "node:fs/promises";
+import path from "node:path";
+import os from "node:os";
+import {
+  type ProjectsData,
+  activateWorker,
+  deactivateWorker,
+  readProjects,
+  writeProjects,
+} from "./projects.js";
+
+describe("Session persistence", () => {
+  let tempDir: string;
+  let testWorkspaceDir: string;
+
+  before(async () => {
+    // Create temp directory for test workspace
+    tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "devclaw-test-"));
+    testWorkspaceDir = tempDir;
+    await fs.mkdir(path.join(testWorkspaceDir, "memory"), { recursive: true });
+
+    // Create initial projects.json
+    const initialData: ProjectsData = {
+      projects: {
+        "-test-group": {
+          name: "test-project",
+          repo: "~/git/test-project",
+          groupName: "Test Project",
+          deployUrl: "https://test.example.com",
+          baseBranch: "main",
+          deployBranch: "main",
+          autoChain: false,
+          channel: "telegram",
+          dev: {
+            active: false,
+            issueId: null,
+            startTime: null,
+            model: null,
+            sessions: {
+              junior: null,
+              medior: null,
+              senior: null,
+            },
+          },
+          qa: {
+            active: false,
+            issueId: null,
+            startTime: null,
+            model: null,
+            sessions: {
+              qa: null,
+            },
+          },
+        },
+      },
+    };
+    await writeProjects(testWorkspaceDir, initialData);
+  });
+
+  after(async () => {
+    // Clean up temp directory
+    await fs.rm(tempDir, { recursive: true, force: true });
+  });
+
+  it("should preserve sessions after task completion (single tier)", async () => {
+    // Simulate task pickup: activate worker with senior tier
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "42",
+      model: "senior",
+      sessionKey: "agent:test:subagent:senior-session-123",
+      startTime: new Date().toISOString(),
+    });
+
+    // Verify session was stored
+    let data = await readProjects(testWorkspaceDir);
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.senior,
+      "agent:test:subagent:senior-session-123",
+      "Senior session should be stored after activation",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.active,
+      true,
+      "Worker should be active",
+    );
+
+    // Simulate task completion: deactivate worker
+    await deactivateWorker(testWorkspaceDir, "-test-group", "dev");
+
+    // Verify session persists after deactivation
+    data = await readProjects(testWorkspaceDir);
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.senior,
+      "agent:test:subagent:senior-session-123",
+      "Senior session should persist after deactivation",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.active,
+      false,
+      "Worker should be inactive",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.issueId,
+      null,
+      "Issue ID should be cleared",
+    );
+  });
+
+  it("should preserve all tier sessions after completion (multiple tiers)", async () => {
+    // Setup: create sessions for multiple tiers
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "10",
+      model: "junior",
+      sessionKey: "agent:test:subagent:junior-session-111",
+      startTime: new Date().toISOString(),
+    });
+    await deactivateWorker(testWorkspaceDir, "-test-group", "dev");
+
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "20",
+      model: "medior",
+      sessionKey: "agent:test:subagent:medior-session-222",
+      startTime: new Date().toISOString(),
+    });
+    await deactivateWorker(testWorkspaceDir, "-test-group", "dev");
+
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "30",
+      model: "senior",
+      sessionKey: "agent:test:subagent:senior-session-333",
+      startTime: new Date().toISOString(),
+    });
+    await deactivateWorker(testWorkspaceDir, "-test-group", "dev");
+
+    // Verify all sessions persisted
+    const data = await readProjects(testWorkspaceDir);
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.junior,
+      "agent:test:subagent:junior-session-111",
+      "Junior session should persist",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.medior,
+      "agent:test:subagent:medior-session-222",
+      "Medior session should persist",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.senior,
+      "agent:test:subagent:senior-session-333",
+      "Senior session should persist",
+    );
+  });
+
+  it("should reuse existing session when picking up new task", async () => {
+    // Setup: create a session for senior tier
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "100",
+      model: "senior",
+      sessionKey: "agent:test:subagent:senior-reuse-999",
+      startTime: new Date().toISOString(),
+    });
+    await deactivateWorker(testWorkspaceDir, "-test-group", "dev");
+
+    // Pick up new task with same tier (no sessionKey = reuse)
+    await activateWorker(testWorkspaceDir, "-test-group", "dev", {
+      issueId: "200",
+      model: "senior",
+    });
+
+    // Verify session was preserved (not overwritten)
+    const data = await readProjects(testWorkspaceDir);
+    assert.strictEqual(
+      data.projects["-test-group"].dev.sessions.senior,
+      "agent:test:subagent:senior-reuse-999",
+      "Senior session should be reused (not cleared)",
+    );
+    assert.strictEqual(
+      data.projects["-test-group"].dev.issueId,
+      "200",
+      "Issue ID should be updated to new task",
+    );
+  });
+});

lib/projects.ts

@@ -171,6 +171,11 @@ export function getWorker(
 /**
  * Update worker state for a project. Only provided fields are updated.
  * This prevents accidentally nulling out fields that should be preserved.
+ * 
+ * Session Preservation:
+ * - If updates.sessions is provided, it's merged with existing sessions (new keys added/updated, existing keys preserved)
+ * - If updates.sessions is NOT provided, existing sessions are preserved via spread operator
+ * - Sessions should NEVER be accidentally cleared during state updates
  */
 export async function updateWorker(
   workspaceDir: string,
@@ -185,10 +190,14 @@ export async function updateWorker(
   }
 
   const worker = project[role];
+  
   // Merge sessions maps if both exist
+  // This ensures we preserve existing sessions while adding/updating new ones
   if (updates.sessions && worker.sessions) {
     updates.sessions = { ...worker.sessions, ...updates.sessions };
   }
+  
+  // Spread worker first, then updates - this preserves any fields not in updates
   project[role] = { ...worker, ...updates };
 
   await writeProjects(workspaceDir, data);
@@ -198,6 +207,21 @@ export async function updateWorker(
 /**
  * Mark a worker as active with a new task.
  * Sets active=true, issueId, model (tier). Stores session key in sessions[tier].
+ * 
+ * Session Handling:
+ * - If sessionKey is provided: new session spawned, stored in sessions[model]
+ * - If sessionKey is omitted: existing session reused (sessions map preserved)
+ * - Other tier sessions in the sessions map are ALWAYS preserved
+ * 
+ * Example flow:
+ * 1. First senior task: activateWorker({model: "senior", sessionKey: "abc"}) 
+ *    → sessions = {junior: null, medior: null, senior: "abc"}
+ * 2. Task completes: deactivateWorker() 
+ *    → sessions = {junior: null, medior: null, senior: "abc"} (preserved!)
+ * 3. Next senior task: activateWorker({model: "senior"}) [no sessionKey]
+ *    → sessions = {junior: null, medior: null, senior: "abc"} (reused!)
+ * 4. Medior task: activateWorker({model: "medior", sessionKey: "xyz"})
+ *    → sessions = {junior: null, medior: "xyz", senior: "abc"} (both preserved!)
  */
 export async function activateWorker(
   workspaceDir: string,
@@ -215,7 +239,8 @@ export async function activateWorker(
     issueId: params.issueId,
     model: params.model,
   };
-  // Store session key in the sessions map for this tier
+  // Store session key in the sessions map for this tier (if new spawn)
+  // If sessionKey is omitted, existing sessions are preserved via updateWorker
   if (params.sessionKey !== undefined) {
     updates.sessions = { [params.model]: params.sessionKey };
   }
@@ -228,16 +253,52 @@ export async function activateWorker(
 /**
  * Mark a worker as inactive after task completion.
  * Clears issueId and active, PRESERVES sessions map, model, startTime for reuse.
+ * 
+ * IMPORTANT: This function MUST preserve the sessions map to enable session reuse
+ * across multiple tasks of the same tier. Do NOT pass `sessions` in the updates
+ * object, as this would overwrite the existing sessions.
  */
 export async function deactivateWorker(
   workspaceDir: string,
   groupId: string,
   role: "dev" | "qa",
 ): Promise<ProjectsData> {
-  return updateWorker(workspaceDir, groupId, role, {
+  // Read current state to verify sessions will be preserved
+  const data = await readProjects(workspaceDir);
+  const project = data.projects[groupId];
+  if (!project) {
+    throw new Error(`Project not found for groupId: ${groupId}`);
+  }
+  
+  const worker = project[role];
+  const sessionsBefore = worker.sessions;
+  
+  // Update worker state (active=false, issueId=null)
+  // Sessions are preserved via spread operator in updateWorker
+  const result = await updateWorker(workspaceDir, groupId, role, {
     active: false,
     issueId: null,
+    // Explicitly DO NOT set sessions here to preserve them
   });
+  
+  // Defensive verification: ensure sessions were not accidentally cleared
+  const updatedWorker = result.projects[groupId][role];
+  const sessionsAfter = updatedWorker.sessions;
+  
+  // Verify sessions map was preserved
+  if (sessionsBefore && sessionsAfter) {
+    for (const [tier, sessionKey] of Object.entries(sessionsBefore)) {
+      if (sessionKey !== null && sessionsAfter[tier] !== sessionKey) {
+        throw new Error(
+          `BUG: Session for tier "${tier}" was lost during deactivateWorker! ` +
+          `Before: ${sessionKey}, After: ${sessionsAfter[tier]}. ` +
+          `This should never happen - sessions must persist for reuse.`
+        );
+      }
+    }
+  }
+  
+  return result;
 }
 
 /**

package.json

@@ -28,7 +28,8 @@
   },
   "scripts": {
     "check": "tsc --noEmit",
-    "watch": "tsc --noEmit --watch"
+    "watch": "tsc --noEmit --watch",
+    "test": "node --test lib/**/*.test.ts"
   },
   "peerDependencies": {
     "openclaw": ">=2026.0.0"