Persist pending actions to MongoDB

[NanaKay007]

Summary

• Wired ActionService to use PendingActionRepository (MongoDB) instead of an in-memory Map
• Pending actions now survive server restarts
• Added user_id and description fields to the pending_actions collection schema
• Added findPendingByUserId query with a (user_id, status) compound index
• All ActionService methods are now async; updated controller, chat service, and all test files accordingly

Test plan

• Start the server, send a chat message that creates a pending action (e.g. "schedule a meeting tomorrow at 10am")
• Verify the pending action appears in the UI
• Restart the server and reload the page — pending action should still be visible
• Approve or reject the action — verify it updates correctly
• Run npm test — all existing integration tests should pass

🤖 Generated with Claude Code

[claude]

PR Review: Persist pending actions to MongoDB

I've reviewed the code changes. The implementation is solid overall, but there are 2 critical security/reliability issues that need to be addressed:

---

🔴 Critical Issue #1: Race Condition in _clear() Method

Location: backend/src/services/action.service.ts:123-128

The _clear() method has a type-unsafe MongoDB collection access that bypasses the repository pattern:

async _clear(): Promise<void> {
  const repo = await this.getRepo();
  // Access the collection directly via a known method — drop all docs
  const db = (repo as any).db as Db;
  await db.collection('pending_actions').deleteMany({});
}

Problems:

1. Uses (repo as any).db - the PendingActionRepository doesn't expose a db property, so this will be undefined at runtime
2. This method will silently fail in tests without actually clearing data
3. Type casting with as any masks the actual issue

Fix: The repository should expose a proper deleteAll() method:

// In PendingActionRepository:
async deleteAll(): Promise<void> {
  await this.collection.deleteMany({});
}

// In ActionService:
async _clear(): Promise<void> {
  const repo = await this.getRepo();
  await repo.deleteAll();
}

---

🔴 Critical Issue #2: JSON Parsing Without Error Handling

Location: backend/src/services/action.service.ts:14

The rowToAction() function parses JSON without any error handling:

params: JSON.parse(row.action_payload),

Security/Reliability Risk:

• If the database contains corrupted or malformed JSON (due to bugs, migrations, or manual edits), this will throw an unhandled exception
• This crashes the entire API endpoint whenever a user tries to fetch pending actions
• Could be exploited to cause denial-of-service by corrupting a single action record

Fix: Add error handling with proper logging:

function rowToAction(row: PendingActionRow): PendingAction {
  let params;
  try {
    params = JSON.parse(row.action_payload);
  } catch (error) {
    console.error(`Failed to parse action_payload for action ${row.id}:`, error);
    // Return a safe default or throw a more specific error
    throw new Error(`Corrupted action data for action ${row.id}`);
  }
  
  return {
    id: row.id,
    userId: row.user_id,
    conversationId: row.conversation_id,
    actionType: row.action_type as ActionType,
    params,
    description: row.description,
    status: row.status as PendingAction['status'],
    createdAt: row.created_at,
    resolvedAt: row.resolved_at ?? undefined,
  };
}

---

✅ What's Good

• Proper async/await conversion throughout
• Good use of compound indexes (user_id, status) and (conversation_id, status)
• Comprehensive test updates
• Clean separation of concerns with repository pattern
• Authorization checks are maintained in approveAction and rejectAction

---

📝 Minor Observations

1. The toRow() method in pendingActionRepository.ts:72 uses non-null assertion (!) which could fail if the document structure is unexpected. Consider adding validation.

2. The fallback logic in approveAction (line 98) and rejectAction (line 119) seems defensive but would only trigger if updateStatus returns undefined, which shouldn't happen. Consider simplifying or documenting why this is needed.

---

Recommendation: Address the two critical issues before merging. The race condition in _clear() will cause test failures, and the JSON parsing issue is a latent security vulnerability.