fix: address code review feedback on ensureCompositeWorkspace
- Fix dedup collision: check generated names against all reserved names to avoid EEXIST when parentName-base matches an existing non-collision basename - Fix silent fs.rm failure: log warning and skip instead of letting EEXIST propagate uncaught when a directory occupies a link path - Filter blank multipleWorkspaces entries before resolving paths - Pick most specific (longest) multipleWorkspaces match in reverse lookup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
0614937b98
commit
4681cb36cc
@ -267,7 +267,10 @@ export function resolveAgentMultipleWorkspaces(
|
||||
if (!Array.isArray(mw) || mw.length === 0) {
|
||||
return undefined;
|
||||
}
|
||||
return mw.map((p) => stripNullBytes(resolveUserPath(p)));
|
||||
const resolved = mw
|
||||
.filter((p) => typeof p === "string" && p.trim().length > 0)
|
||||
.map((p) => stripNullBytes(resolveUserPath(p)));
|
||||
return resolved.length > 0 ? resolved : undefined;
|
||||
}
|
||||
|
||||
export function resolveAgentWorkspaceDir(cfg: OpenClawConfig, agentId: string) {
|
||||
@ -332,16 +335,21 @@ export function resolveAgentIdsByWorkspacePath(
|
||||
matches.push({ id, workspaceDir, order: index });
|
||||
continue;
|
||||
}
|
||||
// Also match individual multipleWorkspaces entries.
|
||||
// Also match individual multipleWorkspaces entries — pick the most specific (longest path).
|
||||
const multiWs = resolveAgentMultipleWorkspaces(cfg, id);
|
||||
if (multiWs) {
|
||||
let bestMatch: string | null = null;
|
||||
for (const ws of multiWs) {
|
||||
const normalizedWs = normalizePathForComparison(ws);
|
||||
if (isPathWithinRoot(normalizedWorkspacePath, normalizedWs)) {
|
||||
matches.push({ id, workspaceDir: normalizedWs, order: index });
|
||||
break;
|
||||
if (!bestMatch || normalizedWs.length > bestMatch.length) {
|
||||
bestMatch = normalizedWs;
|
||||
}
|
||||
}
|
||||
}
|
||||
if (bestMatch) {
|
||||
matches.push({ id, workspaceDir: bestMatch, order: index });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -668,6 +668,16 @@ export async function ensureCompositeWorkspace(params: {
|
||||
const base = path.basename(ws);
|
||||
nameCount.set(base, (nameCount.get(base) ?? 0) + 1);
|
||||
}
|
||||
|
||||
// First pass: collect all non-collision names so collision resolution can avoid them.
|
||||
const reservedNames = new Set<string>();
|
||||
for (const ws of workspacePaths) {
|
||||
const base = path.basename(ws);
|
||||
if ((nameCount.get(base) ?? 0) === 1) {
|
||||
reservedNames.add(base);
|
||||
}
|
||||
}
|
||||
|
||||
const nameUsed = new Map<string, number>();
|
||||
for (const ws of workspacePaths) {
|
||||
const base = path.basename(ws);
|
||||
@ -675,9 +685,15 @@ export async function ensureCompositeWorkspace(params: {
|
||||
if ((nameCount.get(base) ?? 0) > 1) {
|
||||
const parentName = path.basename(path.dirname(ws));
|
||||
const candidate = `${parentName}-${base}`;
|
||||
const usedCount = nameUsed.get(candidate) ?? 0;
|
||||
linkName = usedCount > 0 ? `${candidate}-${usedCount}` : candidate;
|
||||
nameUsed.set(candidate, usedCount + 1);
|
||||
let suffix = nameUsed.get(candidate) ?? 0;
|
||||
linkName = suffix > 0 ? `${candidate}-${suffix}` : candidate;
|
||||
// Ensure the resolved name doesn't collide with a non-collision entry or existing entry.
|
||||
while (reservedNames.has(linkName)) {
|
||||
suffix += 1;
|
||||
linkName = `${candidate}-${suffix}`;
|
||||
}
|
||||
nameUsed.set(candidate, suffix + 1);
|
||||
reservedNames.add(linkName);
|
||||
} else {
|
||||
linkName = base;
|
||||
}
|
||||
@ -722,8 +738,13 @@ export async function ensureCompositeWorkspace(params: {
|
||||
// Target changed — remove old symlink.
|
||||
await fs.unlink(linkPath);
|
||||
} catch {
|
||||
// Symlink doesn't exist or isn't a symlink — remove whatever is there.
|
||||
await fs.rm(linkPath, { force: true, recursive: false }).catch(() => {});
|
||||
// Symlink doesn't exist or isn't a symlink — try to remove whatever is there.
|
||||
try {
|
||||
await fs.rm(linkPath, { force: true, recursive: false });
|
||||
} catch (rmErr) {
|
||||
compositeLog.warn(`Cannot replace non-symlink at ${linkPath}: ${String(rmErr)}`);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
await fs.symlink(target, linkPath);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user