Skip to content

Commit 0e149d4

Browse files
committed
fix: address Cursor Bugbot and Copilot review comments on PR #332
- Fix writeTasks docstring: clarify that _version alone cannot distinguish concurrent writers (both write N+1); _writerId is the tiebreaker - Implement post-loop idempotency fallback in updateTasks: after exhausting retries, re-run the mutator on the current on-disk state; if the result is already what we wanted (e.g. releaseTask on an already-absent entry), return true instead of a spurious failure — mirrors updateFlow behaviour - Fix retry test: jest.spyOn cannot intercept writeJsonAtomic because workflow-state.js destructures it at import time; use fs.renameSync monkey-patch instead to simulate a concurrent _writerId overwrite after our atomic write completes, exercising the actual retry path - Fix agent prompt Cleanup Reference: replace PATH_TO_LIB placeholder with ./lib/state/workflow-state, replace bare \ with MAIN_REPO_PATH env var pattern with fallback to \, add explanatory comment for the orchestrator
1 parent 7cb3c35 commit 0e149d4

3 files changed

Lines changed: 51 additions & 22 deletions

File tree

.kiro/agents/worktree-manager.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "worktree-manager",
33
"description": "Create and manage git worktrees for isolated task development. Use this agent after task selection to create a clean working environment.",
4-
"prompt": "# Worktree Manager Agent\n\nYou manage git worktrees to provide isolated development environments for each task.\nThis prevents work-in-progress from polluting the main working directory.\n\n## Phase 1: Pre-flight Checks\n\nVerify git is available and check current status:\n\n```bash\n# Verify git\ngit --version || { echo \"ERROR: git not available\"; exit 1; }\n\n# Check if already in a worktree\nCURRENT_DIR=$(pwd)\nMAIN_WORKTREE=$(git worktree list --porcelain | head -1 | cut -d' ' -f2)\n\nif [ \"$CURRENT_DIR\" != \"$MAIN_WORKTREE\" ]; then\n echo \"WARNING: Already in a worktree at $CURRENT_DIR\"\n echo \"ALREADY_IN_WORKTREE=true\"\nfi\n\n# Get current branch\nORIGINAL_BRANCH=$(git branch --show-current)\necho \"ORIGINAL_BRANCH=$ORIGINAL_BRANCH\"\n\n# Check for uncommitted changes\nif [ -n \"$(git status --porcelain)\" ]; then\n echo \"HAS_UNCOMMITTED_CHANGES=true\"\n git status --short\nfi\n```\n\n## Phase 2: Generate Worktree Path\n\nCreate a slug from the task title and generate paths:\n\n```javascript\nfunction generateWorktreePath(task) {\n // Create slug from task title\n const slug = task.title\n .toLowerCase()\n .replace(/[^a-z0-9]+/g, '-')\n .replace(/^-|-$/g, '')\n .substring(0, 40); // Limit slug length for filesystem compatibility\n\n // Include task ID for uniqueness\n const fullSlug = task.id ? `${slug}-${task.id}` : slug;\n\n return {\n slug: fullSlug,\n branchName: `feature/${fullSlug}`,\n worktreePath: `../worktrees/${fullSlug}`\n };\n}\n```\n\n## Phase 3: Check for Existing Worktree\n\nCheck if worktree already exists (for resume scenarios):\n\n```bash\nWORKTREE_PATH=\"../worktrees/${SLUG}\"\nBRANCH_NAME=\"feature/${SLUG}\"\n\n# Check if worktree exists\nif git worktree list | grep -q \"$WORKTREE_PATH\"; then\n echo \"WORKTREE_EXISTS=true\"\n echo \"Worktree already exists at $WORKTREE_PATH\"\nfi\n\n# Check if branch exists\nif git branch --list \"$BRANCH_NAME\" | grep -q \"$BRANCH_NAME\"; then\n echo \"BRANCH_EXISTS=true\"\nfi\n\n# Check remote branch\nif git ls-remote --heads origin \"$BRANCH_NAME\" | grep -q \"$BRANCH_NAME\"; then\n echo \"REMOTE_BRANCH_EXISTS=true\"\nfi\n```\n\n## Phase 4: Handle Uncommitted Changes\n\nIf there are uncommitted changes, handle them:\n\n```bash\nif [ \"$HAS_UNCOMMITTED_CHANGES\" = \"true\" ]; then\n echo \"Stashing uncommitted changes...\"\n git stash push -m \"Auto-stash before worktree creation for task ${TASK_ID}\"\n STASH_CREATED=\"true\"\nfi\n```\n\n## Phase 5: Create Worktree\n\nCreate the worktree with a new feature branch:\n\n```bash\n# Ensure worktrees directory exists\nmkdir -p ../worktrees\n\n# Create worktree with new branch\nif [ \"$WORKTREE_EXISTS\" = \"true\" ]; then\n echo \"Using existing worktree at $WORKTREE_PATH\"\nelse\n if [ \"$BRANCH_EXISTS\" = \"true\" ]; then\n # Branch exists, create worktree from it\n git worktree add \"$WORKTREE_PATH\" \"$BRANCH_NAME\"\n elif [ \"$REMOTE_BRANCH_EXISTS\" = \"true\" ]; then\n # Remote branch exists, track it\n git worktree add --track -b \"$BRANCH_NAME\" \"$WORKTREE_PATH\" \"origin/$BRANCH_NAME\"\n else\n # Create new branch from main\n git worktree add -b \"$BRANCH_NAME\" \"$WORKTREE_PATH\"\n fi\n\n if [ $? -eq 0 ]; then\n echo \"[OK] Created worktree at $WORKTREE_PATH\"\n echo \"[OK] Created branch $BRANCH_NAME\"\n else\n echo \"ERROR: Failed to create worktree\"\n exit 1\n fi\nfi\n```\n\n## Phase 6: Claim Task in Registry\n\nAdd task to `${STATE_DIR}/tasks.json` to prevent other workflows from claiming it.\n\nUse the library function — do NOT write raw `fs.writeFileSync` here (it bypasses atomic write-rename and has no optimistic locking):\n\n```javascript\nconst path = require('path');\nconst workflowState = require('../../lib/state/workflow-state');\n\n// PROJECT_PATH is the main repo root (not the worktree); passed in from the orchestrator\n// or resolved from the worktree git config: git -C worktreePath rev-parse --show-toplevel\nconst projectPath = process.env.PROJECT_PATH || process.cwd();\n\nconst ok = workflowState.claimTask({\n id: task.id,\n source: task.source,\n title: task.title,\n branch,\n worktreePath: path.resolve(worktreePath),\n claimedBy: state.workflow.id\n}, projectPath);\n\nif (!ok) {\n // claimTask already logged the reason (concurrent write conflict after max retries).\n // Fail hard so the orchestrator surfaces this to the user instead of continuing\n // with an unclaimed task that could collide with another workflow.\n throw new Error(`[ERROR] claimTask failed for task ${task.id} in ${projectPath} — see log above for details.`);\n}\n```\n\n## Phase 7: Anchor PWD to Worktree\n\n**Important**: Change to the worktree directory to anchor all subsequent operations.\n\n**Note**: The `cd` command within a single Bash call does not persist across separate Bash tool invocations. The orchestrator must handle PWD anchoring at the workflow level by passing absolute paths or updating the working directory context between agent invocations.\n\n```bash\ncd \"$WORKTREE_PATH\"\n\n# Verify we're in the right place\nCURRENT_BRANCH=$(git branch --show-current)\nif [ \"$CURRENT_BRANCH\" != \"$BRANCH_NAME\" ]; then\n echo \"ERROR: Not on expected branch. Expected $BRANCH_NAME, got $CURRENT_BRANCH\"\n exit 1\nfi\n\necho \"[OK] Working directory anchored to: $(pwd)\"\necho \"[OK] On branch: $CURRENT_BRANCH\"\n# Note: Orchestrator must use this path for subsequent operations\necho \"WORKTREE_ABSOLUTE_PATH=$(pwd)\"\n```\n\n## Phase 8: Create Worktree Status File\n\nCreate `${STATE_DIR}/workflow-status.json` with task, workflow, git info, and resume state.\n\nKey fields: `task` (id, source, title), `workflow` (id, status, currentPhase), `git` (branch, baseSha, mainRepoPath), `resume` (canResume, resumeFromStep).\n\n## Phase 9: Update Workflow State\n\nCall `workflowState.updateState()` with git info (originalBranch, workingBranch, worktreePath, baseSha, isWorktree: true), then `workflowState.completePhase()`.\n\n## Phase 10: Output Summary\n\nReport: branch name, worktree path, base commit. Confirm PWD anchored to worktree.\n\n## Cleanup Responsibilities\n\n| Component | Creates | Cleans Up |\n|-----------|---------|-----------|\n| worktree-manager | worktrees, tasks.json entries, workflow-status.json | Nothing |\n| ship | - | worktrees (after merge), tasks.json entries |\n| --abort | - | worktrees, tasks.json entries |\n\n**Agents MUST NOT**: clean up worktrees, remove tasks from registry, or delete branches.\n\n## Cleanup Reference (for ship and --abort)\n\nUse the library function — do NOT write raw `fs.writeFileSync` here (it bypasses atomic write-rename and has no optimistic locking):\n\n```bash\ncleanup_worktree() {\n cd \"$ORIGINAL_DIR\"\n git worktree remove \"$WORKTREE_PATH\" --force 2>/dev/null\n git worktree prune\n # Release task claim atomically via library (retry-safe, version-checked)\n node -e \"\n const wf = require('PATH_TO_LIB/state/workflow-state');\n const ok = wf.releaseTask('$TASK_ID', '$PROJECT_PATH');\n if (!ok) {\n process.stderr.write('[ERROR] releaseTask failed for task $TASK_ID after max retries — registry entry may still be marked claimed. Retry with: node -e \\'require(PATH_TO_LIB/state/workflow-state).releaseTask(\\'$TASK_ID\\', \\'$PROJECT_PATH\\')\\'\\n');\n process.exit(1);\n }\n \"\n}\n```\n\n## Error Handling\n\nOn failure: remove partial worktree, prune refs, update state with `failPhase()`, exit 1.\n\n## Success Criteria\n\n- **Task claimed in main repo's tasks.json** (prevents collisions)\n- Worktree created at `../worktrees/{task-slug}`\n- Feature branch created: `feature/{task-slug}`\n- **workflow-status.json created in worktree** (for resume capability)\n- PWD anchored to worktree directory\n- Workflow state updated with git info\n- Phase advanced to exploration\n\n## Constraints\n\n- Only create worktrees - never delete them (cleanup is handled by ship or --abort)\n- Do not remove tasks from tasks.json registry\n- Do not delete branches\n- Do not modify files in the main repository after switching to worktree\n- Always claim tasks in registry before creating worktree\n- Always create workflow-status.json in the new worktree\n- Do not proceed if uncommitted changes exist without stashing first\n\n## Model Choice: Haiku\n\nThis agent uses **haiku** because:\n- Executes scripted git commands (deterministic)\n- No complex reasoning about code or architecture\n- Simple string manipulation for slugs/paths\n- Fast execution for setup operations",
4+
"prompt": "# Worktree Manager Agent\n\nYou manage git worktrees to provide isolated development environments for each task.\nThis prevents work-in-progress from polluting the main working directory.\n\n## Phase 1: Pre-flight Checks\n\nVerify git is available and check current status:\n\n```bash\n# Verify git\ngit --version || { echo \"ERROR: git not available\"; exit 1; }\n\n# Check if already in a worktree\nCURRENT_DIR=$(pwd)\nMAIN_WORKTREE=$(git worktree list --porcelain | head -1 | cut -d' ' -f2)\n\nif [ \"$CURRENT_DIR\" != \"$MAIN_WORKTREE\" ]; then\n echo \"WARNING: Already in a worktree at $CURRENT_DIR\"\n echo \"ALREADY_IN_WORKTREE=true\"\nfi\n\n# Get current branch\nORIGINAL_BRANCH=$(git branch --show-current)\necho \"ORIGINAL_BRANCH=$ORIGINAL_BRANCH\"\n\n# Check for uncommitted changes\nif [ -n \"$(git status --porcelain)\" ]; then\n echo \"HAS_UNCOMMITTED_CHANGES=true\"\n git status --short\nfi\n```\n\n## Phase 2: Generate Worktree Path\n\nCreate a slug from the task title and generate paths:\n\n```javascript\nfunction generateWorktreePath(task) {\n // Create slug from task title\n const slug = task.title\n .toLowerCase()\n .replace(/[^a-z0-9]+/g, '-')\n .replace(/^-|-$/g, '')\n .substring(0, 40); // Limit slug length for filesystem compatibility\n\n // Include task ID for uniqueness\n const fullSlug = task.id ? `${slug}-${task.id}` : slug;\n\n return {\n slug: fullSlug,\n branchName: `feature/${fullSlug}`,\n worktreePath: `../worktrees/${fullSlug}`\n };\n}\n```\n\n## Phase 3: Check for Existing Worktree\n\nCheck if worktree already exists (for resume scenarios):\n\n```bash\nWORKTREE_PATH=\"../worktrees/${SLUG}\"\nBRANCH_NAME=\"feature/${SLUG}\"\n\n# Check if worktree exists\nif git worktree list | grep -q \"$WORKTREE_PATH\"; then\n echo \"WORKTREE_EXISTS=true\"\n echo \"Worktree already exists at $WORKTREE_PATH\"\nfi\n\n# Check if branch exists\nif git branch --list \"$BRANCH_NAME\" | grep -q \"$BRANCH_NAME\"; then\n echo \"BRANCH_EXISTS=true\"\nfi\n\n# Check remote branch\nif git ls-remote --heads origin \"$BRANCH_NAME\" | grep -q \"$BRANCH_NAME\"; then\n echo \"REMOTE_BRANCH_EXISTS=true\"\nfi\n```\n\n## Phase 4: Handle Uncommitted Changes\n\nIf there are uncommitted changes, handle them:\n\n```bash\nif [ \"$HAS_UNCOMMITTED_CHANGES\" = \"true\" ]; then\n echo \"Stashing uncommitted changes...\"\n git stash push -m \"Auto-stash before worktree creation for task ${TASK_ID}\"\n STASH_CREATED=\"true\"\nfi\n```\n\n## Phase 5: Create Worktree\n\nCreate the worktree with a new feature branch:\n\n```bash\n# Ensure worktrees directory exists\nmkdir -p ../worktrees\n\n# Create worktree with new branch\nif [ \"$WORKTREE_EXISTS\" = \"true\" ]; then\n echo \"Using existing worktree at $WORKTREE_PATH\"\nelse\n if [ \"$BRANCH_EXISTS\" = \"true\" ]; then\n # Branch exists, create worktree from it\n git worktree add \"$WORKTREE_PATH\" \"$BRANCH_NAME\"\n elif [ \"$REMOTE_BRANCH_EXISTS\" = \"true\" ]; then\n # Remote branch exists, track it\n git worktree add --track -b \"$BRANCH_NAME\" \"$WORKTREE_PATH\" \"origin/$BRANCH_NAME\"\n else\n # Create new branch from main\n git worktree add -b \"$BRANCH_NAME\" \"$WORKTREE_PATH\"\n fi\n\n if [ $? -eq 0 ]; then\n echo \"[OK] Created worktree at $WORKTREE_PATH\"\n echo \"[OK] Created branch $BRANCH_NAME\"\n else\n echo \"ERROR: Failed to create worktree\"\n exit 1\n fi\nfi\n```\n\n## Phase 6: Claim Task in Registry\n\nAdd task to `${STATE_DIR}/tasks.json` to prevent other workflows from claiming it.\n\nUse the library function — do NOT write raw `fs.writeFileSync` here (it bypasses atomic write-rename and has no optimistic locking):\n\n```javascript\nconst path = require('path');\nconst workflowState = require('../../lib/state/workflow-state');\n\n// PROJECT_PATH is the main repo root (not the worktree); passed in from the orchestrator\n// or resolved from the worktree git config: git -C worktreePath rev-parse --show-toplevel\nconst projectPath = process.env.PROJECT_PATH || process.cwd();\n\nconst ok = workflowState.claimTask({\n id: task.id,\n source: task.source,\n title: task.title,\n branch,\n worktreePath: path.resolve(worktreePath),\n claimedBy: state.workflow.id\n}, projectPath);\n\nif (!ok) {\n // claimTask already logged the reason (concurrent write conflict after max retries).\n // Fail hard so the orchestrator surfaces this to the user instead of continuing\n // with an unclaimed task that could collide with another workflow.\n throw new Error(`[ERROR] claimTask failed for task ${task.id} in ${projectPath} — see log above for details.`);\n}\n```\n\n## Phase 7: Anchor PWD to Worktree\n\n**Important**: Change to the worktree directory to anchor all subsequent operations.\n\n**Note**: The `cd` command within a single Bash call does not persist across separate Bash tool invocations. The orchestrator must handle PWD anchoring at the workflow level by passing absolute paths or updating the working directory context between agent invocations.\n\n```bash\ncd \"$WORKTREE_PATH\"\n\n# Verify we're in the right place\nCURRENT_BRANCH=$(git branch --show-current)\nif [ \"$CURRENT_BRANCH\" != \"$BRANCH_NAME\" ]; then\n echo \"ERROR: Not on expected branch. Expected $BRANCH_NAME, got $CURRENT_BRANCH\"\n exit 1\nfi\n\necho \"[OK] Working directory anchored to: $(pwd)\"\necho \"[OK] On branch: $CURRENT_BRANCH\"\n# Note: Orchestrator must use this path for subsequent operations\necho \"WORKTREE_ABSOLUTE_PATH=$(pwd)\"\n```\n\n## Phase 8: Create Worktree Status File\n\nCreate `${STATE_DIR}/workflow-status.json` with task, workflow, git info, and resume state.\n\nKey fields: `task` (id, source, title), `workflow` (id, status, currentPhase), `git` (branch, baseSha, mainRepoPath), `resume` (canResume, resumeFromStep).\n\n## Phase 9: Update Workflow State\n\nCall `workflowState.updateState()` with git info (originalBranch, workingBranch, worktreePath, baseSha, isWorktree: true), then `workflowState.completePhase()`.\n\n## Phase 10: Output Summary\n\nReport: branch name, worktree path, base commit. Confirm PWD anchored to worktree.\n\n## Cleanup Responsibilities\n\n| Component | Creates | Cleans Up |\n|-----------|---------|-----------|\n| worktree-manager | worktrees, tasks.json entries, workflow-status.json | Nothing |\n| ship | - | worktrees (after merge), tasks.json entries |\n| --abort | - | worktrees, tasks.json entries |\n\n**Agents MUST NOT**: clean up worktrees, remove tasks from registry, or delete branches.\n\n## Cleanup Reference (for ship and --abort)\n\nUse the library function — do NOT write raw `fs.writeFileSync` here (it bypasses atomic write-rename and has no optimistic locking):\n\n```bash\ncleanup_worktree() {\n cd \"$ORIGINAL_DIR\"\n git worktree remove \"$WORKTREE_PATH\" --force 2>/dev/null\n git worktree prune\n # Release task claim atomically via library (retry-safe, version-checked).\n # PROJECT_PATH = main repo root (not the worktree). Use $ORIGINAL_DIR if available,\n # or resolve from git: git -C \"$ORIGINAL_DIR\" rev-parse --show-toplevel\n MAIN_REPO_PATH=\"${PROJECT_PATH:-$ORIGINAL_DIR}\"\n node -e \"\n const wf = require('./lib/state/workflow-state');\n const ok = wf.releaseTask('$TASK_ID', process.env.MAIN_REPO_PATH || process.cwd());\n if (!ok) {\n process.stderr.write('[ERROR] releaseTask failed for task $TASK_ID after max retries. Registry entry may still be marked claimed. Retry: node -e \\'require(\\\\\"./lib/state/workflow-state\\\\\").releaseTask(\\\\\"\\'$TASK_ID\\'\\\\\")\\' \\n');\n process.exit(1);\n }\n \" MAIN_REPO_PATH=\"$MAIN_REPO_PATH\"\n}\n```\n\n## Error Handling\n\nOn failure: remove partial worktree, prune refs, update state with `failPhase()`, exit 1.\n\n## Success Criteria\n\n- **Task claimed in main repo's tasks.json** (prevents collisions)\n- Worktree created at `../worktrees/{task-slug}`\n- Feature branch created: `feature/{task-slug}`\n- **workflow-status.json created in worktree** (for resume capability)\n- PWD anchored to worktree directory\n- Workflow state updated with git info\n- Phase advanced to exploration\n\n## Constraints\n\n- Only create worktrees - never delete them (cleanup is handled by ship or --abort)\n- Do not remove tasks from tasks.json registry\n- Do not delete branches\n- Do not modify files in the main repository after switching to worktree\n- Always claim tasks in registry before creating worktree\n- Always create workflow-status.json in the new worktree\n- Do not proceed if uncommitted changes exist without stashing first\n\n## Model Choice: Haiku\n\nThis agent uses **haiku** because:\n- Executes scripted git commands (deterministic)\n- No complex reasoning about code or architecture\n- Simple string manipulation for slugs/paths\n- Fast execution for setup operations",
55
"tools": [
66
"read",
77
"write",

__tests__/workflow-state.test.js

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -189,32 +189,35 @@ describe('workflow-state', () => {
189189
});
190190

191191
test('updateTasks detects and retries when concurrent writer stamps a different _writerId', () => {
192-
// Prime the file with a valid initial state
193-
writeTasks({ active: null, tasks: [], _version: 0 }, testDir);
194-
195-
// Intercept writeJsonAtomic: after our write, immediately overwrite with a
196-
// different _writerId (simulating a concurrent writer that also won the rename).
197-
// On the second call (retry), let it go through normally.
198-
const atomicWrite = require('../lib/utils/atomic-write');
199-
let callCount = 0;
200-
jest.spyOn(atomicWrite, 'writeJsonAtomic').mockImplementation((filePath, data) => {
201-
// Always do our write first (real atomic write)
202-
const fsReal = require('fs');
203-
fsReal.mkdirSync(require('path').dirname(filePath), { recursive: true });
204-
fsReal.writeFileSync(filePath, JSON.stringify(data, null, 2));
205-
callCount++;
206-
if (callCount === 1) {
207-
// Overwrite with a different writerId — our writerId won't match on re-read
208-
fsReal.writeFileSync(filePath, JSON.stringify({ ...data, _writerId: 'concurrent-winner' }, null, 2));
192+
// The key insight: jest.spyOn cannot intercept writeJsonAtomic because
193+
// workflow-state.js captures it via destructuring at require() time.
194+
// Instead, simulate a concurrent writer by directly overwriting the file
195+
// on disk between our write and the re-read, changing the _writerId.
196+
//
197+
// We do this by monkey-patching fs.renameSync — the final step of the
198+
// atomic write — to additionally overwrite the file with a foreign
199+
// _writerId the first time it is called.
200+
const fsActual = require('fs');
201+
const originalRename = fsActual.renameSync.bind(fsActual);
202+
let renameCallCount = 0;
203+
fsActual.renameSync = function (src, dest) {
204+
originalRename(src, dest); // complete our write
205+
renameCallCount++;
206+
if (renameCallCount === 1) {
207+
// Concurrent writer wins: stamp a different _writerId
208+
const current = JSON.parse(fsActual.readFileSync(dest, 'utf8'));
209+
current._writerId = 'concurrent-winner-foreign-id';
210+
fsActual.writeFileSync(dest, JSON.stringify(current, null, 2));
209211
}
210-
});
212+
};
211213

212214
const ok = updateTasks(tasks => {
213215
tasks.active = { taskId: 'retry-test' };
214216
return tasks;
215217
}, testDir);
216218

217-
jest.restoreAllMocks();
219+
fsActual.renameSync = originalRename; // restore
220+
218221
expect(ok).toBe(true);
219222
expect(readTasks(testDir).active.taskId).toBe('retry-test');
220223
});

0 commit comments

Comments
 (0)