From a74ea4bad03d24dbc02cd5b663be7ce8447e1448 Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Sat, 25 Apr 2026 12:02:02 +0900 Subject: [PATCH] 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. --- prompts/owner-common-paired-room.md | 22 ++--- src/outbound-attachments.ts | 32 +++++++- src/paired-execution-context.test.ts | 36 +++++++++ src/paired-execution-context.ts | 21 +++++ src/paired-workspace-manager.test.ts | 94 ++++++++++++++++++++-- src/paired-workspace-manager.ts | 115 ++++++++++++++++++++++----- 6 files changed, 281 insertions(+), 39 deletions(-) diff --git a/prompts/owner-common-paired-room.md b/prompts/owner-common-paired-room.md index 0a1babd..f91f0a5 100644 --- a/prompts/owner-common-paired-room.md +++ b/prompts/owner-common-paired-room.md @@ -44,23 +44,25 @@ Challenge the reviewer's reasoning. Point out logical gaps, over-engineering, sc ## 🔴 Workspace Branch Protocol (MANDATORY) -The owner workspace is managed by EJClaw's paired-room state machine. The workspace branch name MUST be `codex/owner/` at the moment your turn ends. If any other branch is checked out when the next message arrives, the entire room goes BLOCKED with "branch mismatch" and needs manual git recovery. +The owner workspace is managed by EJClaw's paired-room state machine. The persistent owner workspace branch name MUST remain `codex/owner/`. Treat this as an invariant for the whole turn, not just a cleanup step at the end. If any other branch is checked out when the next message arrives, the entire room can go BLOCKED with "branch mismatch" and need manual git recovery. ### Every turn, in order -1. **Start**: verify `git branch --show-current`. If it is not `codex/owner/`, check it out before doing anything else. -2. **Work**: feel free to create feature branches (`fix/...`, `feat/...`) while implementing. -3. **Before you finish the turn**: - - If you committed on a feature branch, merge it back into the owner branch (`git checkout codex/owner/ && git merge --ff-only `), then optionally delete the feature branch. - - If the feature branch is behind or diverged, use the appropriate merge/rebase to land the work on the owner branch. - - Confirm a clean end-state: `git branch --show-current` prints `codex/owner/` and `git status --short` is empty (or at least there is no merge conflict and no stray dirty state that the next turn cannot understand). -4. **Only then** emit your DONE / DONE_WITH_CONCERNS / BLOCKED / NEEDS_CONTEXT line and exit. +1. **Start**: verify `git branch --show-current`. It must print `codex/owner/` before you modify files. +2. **Work in place on the owner branch** by default. Do not run `git checkout -b`, `git switch -c`, or switch the persistent owner workspace to `fix/*`, `feat/*`, `codex/main-*`, or any other branch. +3. **If a temporary branch is truly necessary**, create it in a separate worktree outside the persistent owner workspace (for example under `/tmp`) and merge/cherry-pick the finished commits back into `codex/owner/`. Do not leave the persistent `/owner` workspace checked out on that temporary branch. +4. **Before you finish the turn**: + - Confirm the invariant again: `git branch --show-current` prints `codex/owner/`. + - Confirm there is no unresolved git operation: no merge conflict, no detached HEAD, no rebase/cherry-pick in progress. + - `git status --short` should be empty unless the task intentionally requires uncommitted files for the next owner step; if so, explain that state explicitly in your status line. +5. **Only then** emit your DONE / DONE_WITH_CONCERNS / BLOCKED / NEEDS_CONTEXT line and exit. ### Hard rules -- Never end a turn on a `fix/*` or `feat/*` branch. +- Never switch the persistent owner workspace itself to a `fix/*`, `feat/*`, `codex/main-*`, `codex/hotfix-*`, or detached-HEAD branch. +- Never end a turn on any branch except `codex/owner/`. - Never end a turn with unresolved merge conflicts. -- Never leave the workspace in detached-HEAD or rebase-in-progress state at turn end. +- Never leave the workspace in detached-HEAD, rebase-in-progress, cherry-pick-in-progress, or bisect state at turn end. ### If you discover a mismatch at turn start diff --git a/src/outbound-attachments.ts b/src/outbound-attachments.ts index fc7feb7..fffd8e3 100644 --- a/src/outbound-attachments.ts +++ b/src/outbound-attachments.ts @@ -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[] { 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(); @@ -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; } diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index 1c4ab42..ba33ec2 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -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({ diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index 3732eee..e8777ea 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -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; } diff --git a/src/paired-workspace-manager.test.ts b/src/paired-workspace-manager.test.ts index d1a4e24..b5eb64d 100644 --- a/src/paired-workspace-manager.test.ts +++ b/src/paired-workspace-manager.test.ts @@ -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( + '', ); }); diff --git a/src/paired-workspace-manager.ts b/src/paired-workspace-manager.ts index 2cb88af..966ce47 100644 --- a/src/paired-workspace-manager.ts +++ b/src/paired-workspace-manager.ts @@ -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(