fix: preserve paired owner workspace branch invariant
Safely re-anchor drifted paired owner workspaces, guard owner completion handling, and tighten generated attachment temp-dir allowlisting.
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
import fs from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
|
||||
import { DATA_DIR } from './config.js';
|
||||
@@ -16,6 +17,10 @@ export interface ValidateOutboundAttachmentsResult {
|
||||
|
||||
const MAX_ATTACHMENT_BYTES = 8 * 1024 * 1024;
|
||||
const IMAGE_EXTS = /\.(png|jpe?g|gif|webp|bmp)$/i;
|
||||
const DEFAULT_TEMP_ATTACHMENT_DIR_PREFIXES = [
|
||||
'ejclaw-attachment-',
|
||||
'ejclaw-discord-image-',
|
||||
] as const;
|
||||
|
||||
function unique(values: Array<string | null | undefined>): string[] {
|
||||
return [
|
||||
@@ -38,8 +43,9 @@ export function getDefaultAttachmentBaseDirs(): string[] {
|
||||
process.env.CODEX_HOME || (home ? path.join(home, '.codex') : null);
|
||||
// Keep defaults narrow. Runtime-specific workspaces must be passed via
|
||||
// attachmentBaseDirs so one room cannot attach another room's files by path.
|
||||
// Ad-hoc generated files under os.tmpdir()/ejclaw-attachment-* are handled
|
||||
// separately after resolving symlinks, without allowlisting all of /tmp.
|
||||
return unique([
|
||||
'/tmp',
|
||||
path.join(DATA_DIR, 'attachments'),
|
||||
codexHome ? path.join(codexHome, 'generated_images') : null,
|
||||
])
|
||||
@@ -59,6 +65,24 @@ function matchesAllowedBaseDir(realPath: string, baseDirs: string[]): boolean {
|
||||
return baseDirs.some((baseDir) => isWithinBaseDir(realPath, baseDir));
|
||||
}
|
||||
|
||||
function isWithinDefaultTempAttachmentDir(
|
||||
realPath: string,
|
||||
tempDir: string | null,
|
||||
): boolean {
|
||||
if (!tempDir) return false;
|
||||
const relative = path.relative(tempDir, realPath);
|
||||
if (!relative || relative.startsWith('..') || path.isAbsolute(relative)) {
|
||||
return false;
|
||||
}
|
||||
const [firstSegment, ...rest] = relative.split(path.sep);
|
||||
return (
|
||||
rest.length > 0 &&
|
||||
DEFAULT_TEMP_ATTACHMENT_DIR_PREFIXES.some((prefix) =>
|
||||
firstSegment.startsWith(prefix),
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
function detectImageMime(filePath: string): string | null {
|
||||
const handle = fs.openSync(filePath, 'r');
|
||||
try {
|
||||
@@ -114,6 +138,7 @@ export function validateOutboundAttachments(
|
||||
...getDefaultAttachmentBaseDirs(),
|
||||
...(options.baseDirs ?? []).map(resolveExistingDir),
|
||||
]).filter((dir): dir is string => Boolean(dir));
|
||||
const defaultTempDir = resolveExistingDir(os.tmpdir());
|
||||
const files: ValidatedOutboundAttachment[] = [];
|
||||
const rejected: Array<{ path: string; reason: string }> = [];
|
||||
const seen = new Set<string>();
|
||||
@@ -144,7 +169,10 @@ export function validateOutboundAttachments(
|
||||
rejected.push({ path: requestedPath, reason: 'too-large' });
|
||||
continue;
|
||||
}
|
||||
if (!matchesAllowedBaseDir(realPath, baseDirs)) {
|
||||
if (
|
||||
!matchesAllowedBaseDir(realPath, baseDirs) &&
|
||||
!isWithinDefaultTempAttachmentDir(realPath, defaultTempDir)
|
||||
) {
|
||||
rejected.push({ path: requestedPath, reason: 'outside-allowed-dirs' });
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -802,6 +802,42 @@ describe('paired execution context', () => {
|
||||
).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('re-anchors the owner workspace before handling a successful owner completion', () => {
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
status: 'active',
|
||||
round_trip_count: 1,
|
||||
}),
|
||||
);
|
||||
vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue(
|
||||
{
|
||||
ownerWorkspace: buildWorkspace('owner', '/tmp/paired/task-1/owner'),
|
||||
reviewerWorkspace: buildWorkspace(
|
||||
'reviewer',
|
||||
'/tmp/paired/task-1/reviewer',
|
||||
),
|
||||
},
|
||||
);
|
||||
|
||||
completePairedExecutionContext({
|
||||
taskId: 'task-1',
|
||||
role: 'owner',
|
||||
status: 'succeeded',
|
||||
summary: 'STEP_DONE\n1단계 완료, 후속 작업 계속',
|
||||
});
|
||||
|
||||
expect(
|
||||
pairedWorkspaceManager.provisionOwnerWorkspaceForPairedTask,
|
||||
).toHaveBeenCalledWith('task-1');
|
||||
const reanchorCallOrder = vi.mocked(
|
||||
pairedWorkspaceManager.provisionOwnerWorkspaceForPairedTask,
|
||||
).mock.invocationCallOrder[0];
|
||||
const reviewReadyCallOrder = vi.mocked(
|
||||
pairedWorkspaceManager.markPairedTaskReviewReady,
|
||||
).mock.invocationCallOrder[0];
|
||||
expect(reanchorCallOrder).toBeLessThan(reviewReadyCallOrder);
|
||||
});
|
||||
|
||||
it('requests review when the owner reports STEP_DONE in active mode', () => {
|
||||
vi.mocked(db.getPairedTaskById).mockReturnValue(
|
||||
buildPairedTask({
|
||||
|
||||
@@ -676,6 +676,27 @@ export function completePairedExecutionContext(args: {
|
||||
}
|
||||
|
||||
if (role === 'owner') {
|
||||
try {
|
||||
provisionOwnerWorkspaceForPairedTask(taskId);
|
||||
} catch (error) {
|
||||
if (isOwnerWorkspaceRepairNeededError(error)) {
|
||||
logger.warn(
|
||||
{
|
||||
taskId,
|
||||
role,
|
||||
repairMessage: error.blockMessage || error.message,
|
||||
},
|
||||
'Owner workspace post-run guard blocked completion handling',
|
||||
);
|
||||
handleFailedOwnerExecution({
|
||||
task,
|
||||
taskId,
|
||||
summary: error.blockMessage || error.message,
|
||||
});
|
||||
return;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
handleOwnerCompletion({ task, taskId, summary: args.summary });
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1145,7 +1145,7 @@ describe('paired workspace manager', () => {
|
||||
).toBe(ownerBranchName('named-existing-room'));
|
||||
});
|
||||
|
||||
it('blocks provisioning when a named owner workspace branch mismatch has local changes', async () => {
|
||||
it('re-anchors a dirty named owner workspace without touching local changes', async () => {
|
||||
const { db, manager } = await loadModules();
|
||||
db._initTestDatabase();
|
||||
|
||||
@@ -1156,6 +1156,10 @@ describe('paired workspace manager', () => {
|
||||
groupFolder: 'dirty-named-room',
|
||||
});
|
||||
|
||||
runGit(
|
||||
['branch', ownerBranchName('dirty-named-room'), 'HEAD'],
|
||||
canonicalDir,
|
||||
);
|
||||
const workspaceDir = ownerWorkspacePath('dirty-named-room');
|
||||
fs.mkdirSync(path.dirname(workspaceDir), { recursive: true });
|
||||
runGit(
|
||||
@@ -1173,14 +1177,90 @@ describe('paired workspace manager', () => {
|
||||
path.join(workspaceDir, 'README.md'),
|
||||
'dirty named branch\n',
|
||||
);
|
||||
fs.writeFileSync(path.join(workspaceDir, 'NOTES.md'), 'untracked keep\n');
|
||||
const dirtyBefore = runGit(['status', '--short'], workspaceDir);
|
||||
|
||||
expect(() =>
|
||||
manager.provisionOwnerWorkspaceForPairedTask(
|
||||
'paired-task-dirty-named-branch',
|
||||
const ownerWorkspace = manager.provisionOwnerWorkspaceForPairedTask(
|
||||
'paired-task-dirty-named-branch',
|
||||
);
|
||||
|
||||
expect(
|
||||
runGit(['branch', '--show-current'], ownerWorkspace.workspace_dir),
|
||||
).toBe(ownerBranchName('dirty-named-room'));
|
||||
expect(runGit(['status', '--short'], ownerWorkspace.workspace_dir)).toBe(
|
||||
dirtyBefore,
|
||||
);
|
||||
expect(
|
||||
fs.readFileSync(
|
||||
path.join(ownerWorkspace.workspace_dir, 'README.md'),
|
||||
'utf-8',
|
||||
),
|
||||
).toThrow(/Owner workspace needs repair/);
|
||||
expect(runGit(['branch', '--show-current'], workspaceDir)).toBe(
|
||||
'codex/owner/dirty-named-room-sync',
|
||||
).toBe('dirty named branch\n');
|
||||
expect(
|
||||
fs.readFileSync(
|
||||
path.join(ownerWorkspace.workspace_dir, 'NOTES.md'),
|
||||
'utf-8',
|
||||
),
|
||||
).toBe('untracked keep\n');
|
||||
expect(
|
||||
runGit(['branch', '--list', 'backup/dirty-named-room-*'], canonicalDir),
|
||||
).toContain('backup/dirty-named-room-current-pre-reanchor-');
|
||||
});
|
||||
|
||||
it('re-anchors a clean divergent named owner workspace to the current feature branch head', async () => {
|
||||
const { db, manager } = await loadModules();
|
||||
db._initTestDatabase();
|
||||
|
||||
const canonicalDir = path.join(tempRoot, 'canonical');
|
||||
initCanonicalRepo(canonicalDir);
|
||||
seedPairedTask(db, canonicalDir, {
|
||||
taskId: 'paired-task-divergent-named-branch',
|
||||
groupFolder: 'divergent-named-room',
|
||||
});
|
||||
|
||||
runGit(
|
||||
['branch', ownerBranchName('divergent-named-room'), 'HEAD'],
|
||||
canonicalDir,
|
||||
);
|
||||
const workspaceDir = ownerWorkspacePath('divergent-named-room');
|
||||
fs.mkdirSync(path.dirname(workspaceDir), { recursive: true });
|
||||
runGit(
|
||||
[
|
||||
'worktree',
|
||||
'add',
|
||||
'-b',
|
||||
'codex/feature-divergent-named-room',
|
||||
workspaceDir,
|
||||
'HEAD',
|
||||
],
|
||||
canonicalDir,
|
||||
);
|
||||
fs.writeFileSync(path.join(workspaceDir, 'README.md'), 'feature head\n');
|
||||
runGit(['add', 'README.md'], workspaceDir);
|
||||
runGit(['commit', '-m', 'feature work'], workspaceDir);
|
||||
const featureHead = runGit(['rev-parse', 'HEAD'], workspaceDir);
|
||||
|
||||
const ownerWorkspace = manager.provisionOwnerWorkspaceForPairedTask(
|
||||
'paired-task-divergent-named-branch',
|
||||
);
|
||||
|
||||
expect(
|
||||
runGit(['branch', '--show-current'], ownerWorkspace.workspace_dir),
|
||||
).toBe(ownerBranchName('divergent-named-room'));
|
||||
expect(
|
||||
runGit(
|
||||
['rev-parse', ownerBranchName('divergent-named-room')],
|
||||
canonicalDir,
|
||||
),
|
||||
).toBe(featureHead);
|
||||
expect(
|
||||
fs.readFileSync(
|
||||
path.join(ownerWorkspace.workspace_dir, 'README.md'),
|
||||
'utf-8',
|
||||
),
|
||||
).toBe('feature head\n');
|
||||
expect(runGit(['status', '--short'], ownerWorkspace.workspace_dir)).toBe(
|
||||
'',
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -152,6 +152,74 @@ Workspace: \`${args.workspaceDir}\`
|
||||
Repair the owner workspace branch, then retry the task.`;
|
||||
}
|
||||
|
||||
function buildOwnerReanchorBackupPrefix(targetBranch: string): string {
|
||||
const groupFolder = targetBranch.startsWith('codex/owner/')
|
||||
? targetBranch.slice('codex/owner/'.length)
|
||||
: targetBranch.replace(/\//g, '-');
|
||||
return `backup/${groupFolder}`;
|
||||
}
|
||||
|
||||
function buildOwnerReanchorBackupSuffix(): string {
|
||||
const timestamp = new Date()
|
||||
.toISOString()
|
||||
.replace(/[-:.TZ]/g, '')
|
||||
.slice(0, 14);
|
||||
return `${timestamp}-${crypto.randomBytes(3).toString('hex')}`;
|
||||
}
|
||||
|
||||
function reanchorNamedOwnerWorkspaceBranch(args: {
|
||||
canonicalWorkDir: string;
|
||||
workspaceDir: string;
|
||||
currentBranch: string;
|
||||
targetBranch: string;
|
||||
targetBranchCommit: string | null;
|
||||
currentHeadCommit: string;
|
||||
reason: string;
|
||||
}): boolean {
|
||||
const {
|
||||
canonicalWorkDir,
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
targetBranchCommit,
|
||||
currentHeadCommit,
|
||||
reason,
|
||||
} = args;
|
||||
|
||||
ensureBranchNotCheckedOutElsewhere(
|
||||
canonicalWorkDir,
|
||||
workspaceDir,
|
||||
targetBranch,
|
||||
);
|
||||
|
||||
const backupPrefix = buildOwnerReanchorBackupPrefix(targetBranch);
|
||||
const backupSuffix = buildOwnerReanchorBackupSuffix();
|
||||
const currentBackupBranch = `${backupPrefix}-current-pre-reanchor-${backupSuffix}`;
|
||||
const targetBackupBranch = `${backupPrefix}-target-pre-reanchor-${backupSuffix}`;
|
||||
|
||||
runGit(['branch', currentBackupBranch, currentBranch], workspaceDir);
|
||||
if (targetBranchCommit) {
|
||||
runGit(['branch', targetBackupBranch, targetBranch], workspaceDir);
|
||||
}
|
||||
runGit(['branch', '-f', targetBranch, currentHeadCommit], workspaceDir);
|
||||
runGit(['symbolic-ref', 'HEAD', branchRefName(targetBranch)], workspaceDir);
|
||||
|
||||
logger.warn(
|
||||
{
|
||||
workspaceDir,
|
||||
previousBranch: currentBranch,
|
||||
targetBranch,
|
||||
currentHeadCommit,
|
||||
targetBranchCommit,
|
||||
currentBackupBranch,
|
||||
targetBackupBranch: targetBranchCommit ? targetBackupBranch : null,
|
||||
reason,
|
||||
},
|
||||
'Re-anchored owner workspace branch mismatch while preserving worktree state',
|
||||
);
|
||||
return true;
|
||||
}
|
||||
|
||||
function maybeRepairNamedOwnerWorkspaceBranch(args: {
|
||||
canonicalWorkDir: string;
|
||||
workspaceDir: string;
|
||||
@@ -174,32 +242,39 @@ function maybeRepairNamedOwnerWorkspaceBranch(args: {
|
||||
}
|
||||
|
||||
if (!isGitWorktreeClean(workspaceDir)) {
|
||||
throw new OwnerWorkspaceRepairNeededError(
|
||||
buildOwnerWorkspaceRepairBlockMessage({
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
reason:
|
||||
'Automatic repair was skipped because the workspace has local changes.',
|
||||
}),
|
||||
);
|
||||
return reanchorNamedOwnerWorkspaceBranch({
|
||||
canonicalWorkDir,
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
targetBranchCommit,
|
||||
currentHeadCommit,
|
||||
reason: 'workspace has local changes',
|
||||
});
|
||||
}
|
||||
|
||||
if (!targetBranchCommit) {
|
||||
runGit(['switch', '-c', targetBranch], workspaceDir);
|
||||
return true;
|
||||
return reanchorNamedOwnerWorkspaceBranch({
|
||||
canonicalWorkDir,
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
targetBranchCommit,
|
||||
currentHeadCommit,
|
||||
reason: 'expected branch does not exist yet',
|
||||
});
|
||||
}
|
||||
|
||||
if (targetBranchCommit !== currentHeadCommit) {
|
||||
throw new OwnerWorkspaceRepairNeededError(
|
||||
buildOwnerWorkspaceRepairBlockMessage({
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
reason:
|
||||
'Automatic repair was skipped because the expected branch points at a different commit.',
|
||||
}),
|
||||
);
|
||||
return reanchorNamedOwnerWorkspaceBranch({
|
||||
canonicalWorkDir,
|
||||
workspaceDir,
|
||||
currentBranch,
|
||||
targetBranch,
|
||||
targetBranchCommit,
|
||||
currentHeadCommit,
|
||||
reason: 'expected branch points at a different commit',
|
||||
});
|
||||
}
|
||||
|
||||
ensureBranchNotCheckedOutElsewhere(
|
||||
|
||||
Reference in New Issue
Block a user