From 62cda45a91c34936e87a6b3400d42453cf4c0e7b Mon Sep 17 00:00:00 2001 From: iliya Date: Thu, 5 Mar 2026 19:58:08 +0200 Subject: [PATCH] feat: enhance member management with CLI auto-suffix validation and cleanup - Implemented validation for member names to prevent the use of auto-suffixed names (e.g., "alice-2") in TeamDataService and TeamProvisioningService. - Added cleanup functionality to remove auto-suffixed members from configuration and metadata, ensuring a cleaner team member list. - Enhanced error handling for member name inputs, including checks for empty names and reserved names like "team-lead". - Updated member management logic in TeamFSWorker to drop auto-suffixed members from the member map. - Improved overall robustness of team member handling across services. --- src/main/services/team/TeamDataService.ts | 39 +++- .../services/team/TeamProvisioningService.ts | 187 +++++++++++++++++- src/main/workers/team-fs-worker.ts | 21 ++ .../components/team/members/MemberLogsTab.tsx | 14 +- 4 files changed, 249 insertions(+), 12 deletions(-) diff --git a/src/main/services/team/TeamDataService.ts b/src/main/services/team/TeamDataService.ts index 4c2748b4..2635505d 100644 --- a/src/main/services/team/TeamDataService.ts +++ b/src/main/services/team/TeamDataService.ts @@ -12,6 +12,7 @@ import { killProcessByPid } from '@main/utils/processKill'; import { AGENT_BLOCK_CLOSE, AGENT_BLOCK_OPEN } from '@shared/constants/agentBlocks'; import { getMemberColor } from '@shared/constants/memberColors'; import { createLogger } from '@shared/utils/logger'; +import { parseNumericSuffixName } from '@shared/utils/teamMemberName'; import { randomUUID } from 'crypto'; import * as fs from 'fs'; import * as path from 'path'; @@ -619,18 +620,29 @@ export class TeamDataService { } async addMember(teamName: string, request: AddMemberRequest): Promise { + const name = request.name.trim(); + if (!name) { + throw new Error('Member name cannot be empty'); + } + const suffixInfo = parseNumericSuffixName(name); + if (suffixInfo && suffixInfo.suffix >= 2) { + throw new Error( + `Member name "${name}" is not allowed (reserved for Claude CLI auto-suffix). Use "${suffixInfo.base}" instead.` + ); + } + const members = await this.membersMetaStore.getMembers(teamName); - const existing = members.find((m) => m.name.toLowerCase() === request.name.toLowerCase()); + const existing = members.find((m) => m.name.toLowerCase() === name.toLowerCase()); if (existing) { if (existing.removedAt) { - throw new Error(`Name "${request.name}" was previously used by a removed member`); + throw new Error(`Name "${name}" was previously used by a removed member`); } - throw new Error(`Member "${request.name}" already exists`); + throw new Error(`Member "${name}" already exists`); } const newMember: TeamMember = { - name: request.name, + name, role: request.role?.trim() || undefined, workflow: request.workflow?.trim() || undefined, agentType: 'general-purpose', @@ -680,6 +692,12 @@ export class TeamDataService { if (name.toLowerCase() === 'team-lead') { throw new Error('Member name "team-lead" is reserved'); } + const suffixInfo = parseNumericSuffixName(name); + if (suffixInfo && suffixInfo.suffix >= 2) { + throw new Error( + `Member name "${name}" is not allowed (reserved for Claude CLI auto-suffix). Use "${suffixInfo.base}" instead.` + ); + } nextByName.add(name.toLowerCase()); const prev = existingByName.get(name.toLowerCase()); return { @@ -1193,7 +1211,18 @@ export class TeamDataService { await this.membersMetaStore.writeMembers( request.teamName, request.members.map((member, index) => ({ - name: member.name, + name: (() => { + const name = member.name.trim(); + if (!name) throw new Error('Member name cannot be empty'); + if (name.toLowerCase() === 'team-lead') throw new Error('Member name "team-lead" is reserved'); + const suffixInfo = parseNumericSuffixName(name); + if (suffixInfo && suffixInfo.suffix >= 2) { + throw new Error( + `Member name "${name}" is not allowed (reserved for Claude CLI auto-suffix). Use "${suffixInfo.base}" instead.` + ); + } + return name; + })(), role: member.role?.trim() || undefined, agentType: 'general-purpose', color: getMemberColor(index), diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index 71a7055a..b07f4318 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -21,6 +21,7 @@ import { getMemberColor } from '@shared/constants/memberColors'; import { resolveLanguageName } from '@shared/utils/agentLanguage'; import { isInboxNoiseMessage } from '@shared/utils/inboxNoise'; import { createLogger } from '@shared/utils/logger'; +import { createCliAutoSuffixNameGuard } from '@shared/utils/teamMemberName'; import { spawn } from 'child_process'; import { randomUUID } from 'crypto'; import * as fs from 'fs'; @@ -1924,12 +1925,19 @@ export class TeamProvisioningService { // IMPORTANT: The CLI auto-suffixes teammate names when they already exist in config.json. // Normalize config.json to keep only the team-lead before spawning the CLI, so we get stable names. - await this.normalizeTeamConfigForLaunch(request.teamName, configRaw); + try { + await this.normalizeTeamConfigForLaunch(request.teamName, configRaw); + await this.assertConfigLeadOnlyForLaunch(request.teamName); - // Update projectPath in config IMMEDIATELY so TeamDetailView shows the correct path - // even if provisioning is interrupted or the user stops the team early. - // If launch fails, restorePrelaunchConfig() will revert to the backup (old projectPath). - await this.updateConfigProjectPath(request.teamName, request.cwd); + // Update projectPath in config IMMEDIATELY so TeamDetailView shows the correct path + // even if provisioning is interrupted or the user stops the team early. + // If launch fails, restorePrelaunchConfig() will revert to the backup (old projectPath). + await this.updateConfigProjectPath(request.teamName, request.cwd); + } catch (error) { + // Restore pre-launch backup so config.json is not left in normalized (lead-only) state. + await this.restorePrelaunchConfig(request.teamName); + throw error; + } let claudePath: string | null; try { @@ -3138,6 +3146,10 @@ export class TeamProvisioningService { await this.updateConfigPostLaunch(run.teamName, run.request.cwd, run.detectedSessionId); await this.cleanupPrelaunchBackup(run.teamName); + // Defense in depth: if the CLI (or a stale config) produced auto-suffixed members (alice-2), + // clean them up so they don't persist and reappear in the UI. + await this.cleanupCliAutoSuffixedMembers(run.teamName); + // Best-effort: detect CLI-suffixed member names (alice-2, bob-2) that indicate // a stale config.json was present during launch (double-launch race). try { @@ -3898,6 +3910,106 @@ export class TeamProvisioningService { } } + private async cleanupCliAutoSuffixedMembers(teamName: string): Promise { + const configPath = path.join(getTeamsBasePath(), teamName, 'config.json'); + + let removedFromConfig: string[] = []; + try { + const raw = await tryReadRegularFileUtf8(configPath, { + timeoutMs: TEAM_JSON_READ_TIMEOUT_MS, + maxBytes: TEAM_CONFIG_MAX_BYTES, + }); + if (raw) { + const parsed = JSON.parse(raw) as Record; + const membersRaw = Array.isArray(parsed.members) + ? (parsed.members as Record[]) + : []; + if (membersRaw.length > 0) { + const teammateNames = membersRaw + .map((m) => (typeof m.name === 'string' ? m.name.trim() : '')) + .filter((n) => n.length > 0 && n.toLowerCase() !== 'team-lead' && n.toLowerCase() !== 'user'); + + const keepName = createCliAutoSuffixNameGuard(teammateNames); + const nextMembers: Record[] = []; + for (const m of membersRaw) { + const name = typeof m.name === 'string' ? m.name.trim() : ''; + const agentType = typeof m.agentType === 'string' ? m.agentType : ''; + if (!name) continue; + if (agentType === 'team-lead' || name === 'team-lead' || name === 'user') { + nextMembers.push(m); + continue; + } + if (!keepName(name)) { + removedFromConfig.push(name); + continue; + } + nextMembers.push(m); + } + + if (removedFromConfig.length > 0) { + parsed.members = nextMembers; + await atomicWriteAsync(configPath, JSON.stringify(parsed, null, 2)); + logger.warn( + `[${teamName}] Removed CLI auto-suffixed members from config.json: ${removedFromConfig.join(', ')}` + ); + } + } + } + } catch { + // best-effort + } + + let activeNamesForInboxCleanup: Set = new Set(); + try { + const metaMembers = await this.membersMetaStore.getMembers(teamName); + if (metaMembers.length > 0) { + const activeNames = metaMembers + .filter((m) => !m.removedAt) + .map((m) => m.name.trim()) + .filter((n) => n.length > 0 && n.toLowerCase() !== 'team-lead' && n.toLowerCase() !== 'user'); + + const keepName = createCliAutoSuffixNameGuard(activeNames); + const removedFromMeta: string[] = []; + const nextMeta = metaMembers.filter((m) => { + const name = m.name?.trim() ?? ''; + if (!name) return false; + const lower = name.toLowerCase(); + if (lower === 'team-lead' || lower === 'user' || m.agentType === 'team-lead') return true; + if (!m.removedAt && !keepName(name)) { + removedFromMeta.push(name); + return false; + } + return true; + }); + + if (removedFromMeta.length > 0) { + await this.membersMetaStore.writeMembers(teamName, nextMeta); + logger.warn( + `[${teamName}] Removed CLI auto-suffixed members from members.meta.json: ${removedFromMeta.join(', ')}` + ); + } + + activeNamesForInboxCleanup = new Set( + nextMeta + .filter((m) => !m.removedAt) + .map((m) => m.name.trim()) + .filter((n) => n.length > 0 && n.toLowerCase() !== 'team-lead' && n.toLowerCase() !== 'user') + ); + } + } catch { + // best-effort + } + + // Also attempt inbox cleanup (merge alice-2.json into alice.json). + if (activeNamesForInboxCleanup.size > 0) { + try { + await this.mergeAndRemoveDuplicateInboxes(teamName, activeNamesForInboxCleanup); + } catch { + // best-effort + } + } + } + /** * Fallback: scan the project directory for the newest JSONL file * that isn't already in sessionHistory. Returns the session ID or null. @@ -3933,6 +4045,55 @@ export class TeamProvisioningService { } } + private async assertConfigLeadOnlyForLaunch(teamName: string): Promise { + const configPath = path.join(getTeamsBasePath(), teamName, 'config.json'); + const raw = await tryReadRegularFileUtf8(configPath, { + timeoutMs: TEAM_JSON_READ_TIMEOUT_MS, + maxBytes: TEAM_CONFIG_MAX_BYTES, + }); + if (!raw) { + throw new Error('config.json unreadable'); + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw) as unknown; + } catch { + throw new Error('config.json could not be parsed'); + } + if (!parsed || typeof parsed !== 'object') { + throw new Error('config.json has invalid shape'); + } + + const config = parsed as Record; + const members = Array.isArray(config.members) + ? (config.members as Record[]) + : []; + if (members.length === 0) return; + + for (const member of members) { + const name = typeof member.name === 'string' ? member.name.trim() : ''; + if (!name) continue; + const lower = name.toLowerCase(); + const agentType = typeof member.agentType === 'string' ? member.agentType : ''; + + if (agentType === 'team-lead' || lower === 'team-lead' || lower === 'user') continue; + + const leadAgentId = config.leadAgentId; + if ( + typeof leadAgentId === 'string' && + typeof member.agentId === 'string' && + member.agentId === leadAgentId + ) { + continue; + } + + throw new Error( + `Refusing to launch: config.json still contains teammates (e.g. "${name}"), which can trigger CLI auto-suffixes like "${name}-2".` + ); + } + } + private async normalizeTeamConfigForLaunch(teamName: string, configRaw: string): Promise { const configPath = path.join(getTeamsBasePath(), teamName, 'config.json'); const backupPath = `${configPath}.prelaunch.bak`; @@ -4287,6 +4448,14 @@ export class TeamProvisioningService { }); } } + // Defense: ignore CLI auto-suffixed duplicates (alice-2) when base name exists. + const allNames = Array.from(byName.keys()); + const keepName = createCliAutoSuffixNameGuard(allNames); + for (const name of allNames) { + if (!keepName(name)) { + byName.delete(name); + } + } const members = Array.from(byName.values()).sort((a, b) => a.name.localeCompare(b.name)); if (members.length > 0) { return { members, source: 'members-meta' }; @@ -4386,6 +4555,14 @@ export class TeamProvisioningService { if (!name) continue; byName.set(name, { name }); } + // Defense: ignore CLI auto-suffixed duplicates (alice-2) when base name exists. + const allNames = Array.from(byName.keys()); + const keepName = createCliAutoSuffixNameGuard(allNames); + for (const name of allNames) { + if (!keepName(name)) { + byName.delete(name); + } + } return Array.from(byName.values()).sort((a, b) => a.name.localeCompare(b.name)); } catch { logger.warn(`[${teamName}] Failed to parse config.json for launch fallback members`); diff --git a/src/main/workers/team-fs-worker.ts b/src/main/workers/team-fs-worker.ts index ee21dc5d..ba9568f7 100644 --- a/src/main/workers/team-fs-worker.ts +++ b/src/main/workers/team-fs-worker.ts @@ -250,6 +250,25 @@ function mergeMember( }); } +function dropCliAutoSuffixedMembers( + memberMap: Map +): void { + const keys = Array.from(memberMap.keys()); + const allLower = new Set(keys); // keys are already lowercased + for (const key of keys) { + const member = memberMap.get(key); + const name = member?.name ?? ''; + const match = name.trim().match(/^(.+)-(\d+)$/); + if (!match?.[1] || !match[2]) continue; + const suffix = Number(match[2]); + if (!Number.isFinite(suffix) || suffix < 2) continue; + const baseLower = match[1].toLowerCase(); + if (allLower.has(baseLower)) { + memberMap.delete(key); + } + } +} + async function listTeams( payload: ListTeamsPayload ): Promise<{ teams: unknown[]; diag: ListTeamsDiag }> { @@ -392,6 +411,8 @@ async function listTeams( } } + dropCliAutoSuffixedMembers(memberMap); + const members = Array.from(memberMap.values()); const summary = { teamName, diff --git a/src/renderer/components/team/members/MemberLogsTab.tsx b/src/renderer/components/team/members/MemberLogsTab.tsx index faeecc5e..e8f2c1a9 100644 --- a/src/renderer/components/team/members/MemberLogsTab.tsx +++ b/src/renderer/components/team/members/MemberLogsTab.tsx @@ -60,10 +60,20 @@ export const MemberLogsTab = ({ }, []); const sortedLogs = useMemo(() => { + const nowMs = Date.now(); + const getLastActivityMs = (log: MemberLogSummary): number => { + const startMs = new Date(log.startTime).getTime(); + if (!Number.isFinite(startMs)) return Number.NaN; + const durationMs = Number.isFinite(log.durationMs) ? Math.max(0, log.durationMs) : 0; + const endMs = startMs + durationMs; + // Keep actively-updating logs at the top even if duration lags slightly. + return log.isOngoing ? Math.max(endMs, nowMs) : endMs; + }; + const withIndex = logs.map((log, index) => ({ log, index })); withIndex.sort((a, b) => { - const aTime = new Date(a.log.startTime).getTime(); - const bTime = new Date(b.log.startTime).getTime(); + const aTime = getLastActivityMs(a.log); + const bTime = getLastActivityMs(b.log); if (Number.isFinite(aTime) && Number.isFinite(bTime) && aTime !== bTime) return bTime - aTime; if (Number.isFinite(aTime) && !Number.isFinite(bTime)) return -1; if (!Number.isFinite(aTime) && Number.isFinite(bTime)) return 1;