feat(core-engineering)!: rewrite worktrees, consolidate code-reviewer (#56)

## Summary

- Rewrites `using-git-worktrees` and `finishing-a-development-branch`
with environment detection (`GIT_DIR != GIT_COMMON`), consent gate
before creating worktrees, native-tool preference (defers to
`EnterWorktree`-style harness commands), and provenance-based cleanup
that only removes worktrees this skill created
- Consolidates the `code-reviewer` named agent into the
`requesting-code-review` skill template — the persona/checklist was
duplicated and the two files could drift
- Drops the leaked "review every 3 tasks" cadence (it was meant for
`executing-plans`, not `subagent-driven-development`) and removes
vestigial `## Integration` sections from four skills

## Breaking change

Removes the `core-engineering:code-reviewer` named agent. Callers should
dispatch `Task(general-purpose)` with the template at
`skills/requesting-code-review/code-reviewer.md` instead.

Author: RomarQ

core-engineering/README.md

@@ -59,7 +59,6 @@ There's a bunch more to it, but that's the core of the system. And because the s
 
 ### Agents
 
-- **code-reviewer** - Reviews completed work against the original plan and coding standards
 - **dependency-manager** - Vulnerability scanning, version conflict resolution, and bundle optimization
 - **documentation-specialist** - API docs, tutorials, architecture guides, and documentation audits
 - **git-workflow-manager** - Branching strategies, release automation, and merge conflict resolution

core-engineering/agents/code-reviewer.md

@@ -61,10 +61,3 @@ After all tasks complete and verified:
 - Reference skills when plan says to
 - Stop when blocked, don't guess
 - Never start implementation on main/master branch without explicit user consent
-
-## Integration
-
-**Required workflow skills:**
-- **core-engineering:using-git-worktrees** - REQUIRED: Set up isolated workspace before starting
-- **core-engineering:writing-plans** - Creates the plan this skill executes
-- **core-engineering:finishing-a-development-branch** - Complete development after all tasks

core-engineering/skills/executing-plans/SKILL.md

@@ -9,7 +9,7 @@ description: Use when implementation is complete, all tests pass, and you need t
 
 Guide completion of development work by presenting clear options and handling chosen workflow.
 
-**Core principle:** Verify tests → Present options → Execute choice → Clean up.
+**Core principle:** Verify tests → Detect environment → Present options → Execute choice → Clean up.
 
 **Announce at start:** "I'm using the finishing-a-development-branch skill to complete this work."
 
@@ -25,6 +25,7 @@ npm test / cargo test / pytest / go test ./...
 ```
 
 **If tests fail:**
+
 ```
 Tests failing (<N> failures). Must fix before completing:
 
@@ -37,7 +38,24 @@ Stop. Don't proceed to Step 2.
 
 **If tests pass:** Continue to Step 2.
 
-### Step 2: Determine Base Branch
+### Step 2: Detect Environment
+
+**Determine workspace state before presenting options:**
+
+```bash
+GIT_DIR=$(cd "$(git rev-parse --git-dir)" 2>/dev/null && pwd -P)
+GIT_COMMON=$(cd "$(git rev-parse --git-common-dir)" 2>/dev/null && pwd -P)
+```
+
+This determines which menu to show and how cleanup works:
+
+| State | Menu | Cleanup |
+|-------|------|---------|
+| `GIT_DIR == GIT_COMMON` (normal repo) | Standard 4 options | No worktree to clean up |
+| `GIT_DIR != GIT_COMMON`, named branch | Standard 4 options | Provenance-based (see Step 6) |
+| `GIT_DIR != GIT_COMMON`, detached HEAD | Reduced 3 options (no merge) | No cleanup (externally managed) |
+
+### Step 3: Determine Base Branch
 
 ```bash
 # Try common base branches
@@ -46,9 +64,9 @@ git merge-base HEAD main 2>/dev/null || git merge-base HEAD master 2>/dev/null
 
 Or ask: "This branch split from main - is that correct?"
 
-### Step 3: Present Options
+### Step 4: Present Options
 
-Present exactly these 4 options:
+**Normal repo and named-branch worktree — present exactly these 4 options:**
 
 ```
 Implementation complete. What would you like to do?
@@ -61,30 +79,58 @@ Implementation complete. What would you like to do?
 Which option?
 ```
 
+**Detached HEAD — present exactly these 3 options:**
+
+```
+Implementation complete. You're on a detached HEAD (externally managed workspace).
+
+1. Push as new branch and create a Pull Request
+2. Keep as-is (I'll handle it later)
+3. Discard this work
+
+Which option?
+```
+
 **Don't add explanation** - keep options concise.
 
-### Step 4: Execute Choice
+**Detached-HEAD option mapping for Step 5/6:**
+
+| Detached HEAD | Standard equivalent | Execution | Cleanup (Step 6) |
+|---------------|---------------------|-----------|------------------|
+| 1. Push + PR | Standard Option 2 | Create branch from HEAD, then standard Option 2 flow | Skip — externally managed |
+| 2. Keep as-is | Standard Option 3 | Standard Option 3 flow | Skip — externally managed |
+| 3. Discard | Standard Option 4 | Standard Option 4 flow, but no branch to delete | Skip — externally managed |
+
+The host environment owns the workspace in detached HEAD mode, so Step 6 never removes the worktree regardless of which option the user picks.
+
+### Step 5: Execute Choice
 
 #### Option 1: Merge Locally
 
 ```bash
-# Switch to base branch
-git checkout <base-branch>
+# Capture worktree path BEFORE leaving it — Step 6 cleanup needs this
+WORKTREE_PATH=$(git rev-parse --show-toplevel)
 
-# Pull latest
-git pull
+# Get main repo root for CWD safety
+MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel)
+cd "$MAIN_ROOT"
 
-# Merge feature branch
+# Merge first — verify success before removing anything
+git checkout <base-branch>
+git pull
 git merge <feature-branch>
 
 # Verify tests on merged result
 <test command>
 
-# If tests pass
-git branch -d <feature-branch>
+# Only after merge succeeds: cleanup worktree (Step 6), then delete branch
 ```
 
-Then: Cleanup worktree (Step 5)
+Then: Cleanup worktree (Step 6), then delete branch:
+
+```bash
+git branch -d <feature-branch>
+```
 
 #### Option 2: Push and Create PR
 
@@ -103,7 +149,7 @@ EOF
 )"
 ```
 
-Then: Cleanup worktree (Step 5)
+**Do NOT clean up worktree** — user needs it alive to iterate on PR feedback.
 
 #### Option 3: Keep As-Is
 
@@ -114,6 +160,7 @@ Report: "Keeping branch <name>. Worktree preserved at <path>."
 #### Option 4: Discard
 
 **Confirm first:**
+
 ```
 This will permanently delete:
 - Branch <name>
@@ -126,53 +173,88 @@ Type 'discard' to confirm.
 Wait for exact confirmation.
 
 If confirmed:
+
+```bash
+# Capture worktree path BEFORE leaving it — Step 6 cleanup needs this
+WORKTREE_PATH=$(git rev-parse --show-toplevel)
+
+MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel)
+cd "$MAIN_ROOT"
+```
+
+Then: Cleanup worktree (Step 6), then force-delete branch:
+
 ```bash
-git checkout <base-branch>
 git branch -D <feature-branch>
 ```
 
-Then: Cleanup worktree (Step 5)
+### Step 6: Cleanup Workspace
 
-### Step 5: Cleanup Worktree
+**Only runs for Options 1 and 4.** Options 2 and 3 always preserve the worktree.
 
-**For Options 1, 2, 4:**
+`WORKTREE_PATH` was captured in Step 5 before the `cd "$MAIN_ROOT"`. Re-derive
+environment state from the original worktree path (not the current CWD):
 
-Check if in worktree:
 ```bash
-git worktree list | grep $(git branch --show-current)
+ORIG_GIT_DIR=$(cd "$WORKTREE_PATH" && git rev-parse --git-dir 2>/dev/null)
+ORIG_GIT_COMMON=$(cd "$WORKTREE_PATH" && git rev-parse --git-common-dir 2>/dev/null)
 ```
 
-If yes:
+**If `ORIG_GIT_DIR == ORIG_GIT_COMMON`:** The original workspace was the main checkout, not a linked worktree. No worktree to clean up. Done.
+
+**If `$WORKTREE_PATH` is under `.worktrees/`, `worktrees/`, or `~/.config/skills/worktrees/`:** This skill created the worktree — we own cleanup.
+
 ```bash
-git worktree remove <worktree-path>
+# CWD is already $MAIN_ROOT from Step 5
+git worktree remove "$WORKTREE_PATH"
+git worktree prune  # Self-healing: clean up any stale registrations
 ```
 
-**For Option 3:** Keep worktree.
+**Otherwise:** The host environment (harness) owns this workspace. Do NOT remove it. If your platform provides a workspace-exit tool, use it. Otherwise, leave the workspace in place.
 
 ## Quick Reference
 
 | Option | Merge | Push | Keep Worktree | Cleanup Branch |
 |--------|-------|------|---------------|----------------|
-| 1. Merge locally | ✓ | - | - | ✓ |
-| 2. Create PR | - | ✓ | ✓ | - |
-| 3. Keep as-is | - | - | ✓ | - |
-| 4. Discard | - | - | - | ✓ (force) |
+| 1. Merge locally | yes | - | - | yes |
+| 2. Create PR | - | yes | yes | - |
+| 3. Keep as-is | - | - | yes | - |
+| 4. Discard | - | - | - | yes (force) |
 
 ## Common Mistakes
 
 **Skipping test verification**
+
 - **Problem:** Merge broken code, create failing PR
 - **Fix:** Always verify tests before offering options
 
 **Open-ended questions**
-- **Problem:** "What should I do next?" → ambiguous
-- **Fix:** Present exactly 4 structured options
 
-**Automatic worktree cleanup**
-- **Problem:** Remove worktree when might need it (Option 2, 3)
+- **Problem:** "What should I do next?" is ambiguous
+- **Fix:** Present exactly 4 structured options (or 3 for detached HEAD)
+
+**Cleaning up worktree for Option 2**
+
+- **Problem:** Remove worktree user needs for PR iteration
 - **Fix:** Only cleanup for Options 1 and 4
 
+**Deleting branch before removing worktree**
+
+- **Problem:** `git branch -d` fails because worktree still references the branch
+- **Fix:** Merge first, remove worktree, then delete branch
+
+**Running git worktree remove from inside the worktree**
+
+- **Problem:** Command fails silently when CWD is inside the worktree being removed
+- **Fix:** Always `cd` to main repo root before `git worktree remove`
+
+**Cleaning up harness-owned worktrees**
+
+- **Problem:** Removing a worktree the harness created causes phantom state
+- **Fix:** Only clean up worktrees under `.worktrees/`, `worktrees/`, or `~/.config/skills/worktrees/`
+
 **No confirmation for discard**
+
 - **Problem:** Accidentally delete work
 - **Fix:** Require typed "discard" confirmation
 
@@ -183,18 +265,15 @@ git worktree remove <worktree-path>
 - Merge without verifying tests on result
 - Delete work without confirmation
 - Force-push without explicit request
+- Remove a worktree before confirming merge success
+- Clean up worktrees you didn't create (provenance check)
+- Run `git worktree remove` from inside the worktree
 
 **Always:**
 - Verify tests before offering options
-- Present exactly 4 options
+- Detect environment before presenting menu
+- Present exactly 4 options (or 3 for detached HEAD)
 - Get typed confirmation for Option 4
 - Clean up worktree for Options 1 & 4 only
-
-## Integration
-
-**Called by:**
-- **subagent-driven-development** (Step 7) - After all tasks complete
-- **executing-plans** (Step 5) - After all batches complete
-
-**Pairs with:**
-- **using-git-worktrees** - Cleans up worktree created by that skill
+- `cd` to main repo root before worktree removal
+- Run `git worktree prune` after removal

core-engineering/skills/finishing-a-development-branch/SKILL.md

@@ -5,7 +5,7 @@ description: Use when completing tasks, implementing major features, or before m
 
 # Requesting Code Review
 
-Dispatch core-engineering:code-reviewer subagent to catch issues before they cascade. The reviewer gets precisely crafted context for evaluation — never your session's history. This keeps the reviewer focused on the work product, not your thought process, and preserves your own context for continued work.
+Dispatch a code reviewer subagent to catch issues before they cascade. The reviewer gets precisely crafted context for evaluation — never your session's history. This keeps the reviewer focused on the work product, not your thought process, and preserves your own context for continued work.
 
 **Core principle:** Review early, review often.
 
@@ -29,16 +29,15 @@ BASE_SHA=$(git rev-parse HEAD~1)  # or origin/main
 HEAD_SHA=$(git rev-parse HEAD)
 ```
 
-**2. Dispatch code-reviewer subagent:**
+**2. Dispatch code reviewer subagent:**
 
-Use Task tool with core-engineering:code-reviewer, fill template at `code-reviewer.md`
+Use Task tool with `general-purpose` type, fill template at `code-reviewer.md`
 
 **Placeholders:**
-- `{WHAT_WAS_IMPLEMENTED}` - What you just built
+- `{DESCRIPTION}` - Brief summary of what you built
 - `{PLAN_OR_REQUIREMENTS}` - What it should do
 - `{BASE_SHA}` - Starting commit
 - `{HEAD_SHA}` - Ending commit
-- `{DESCRIPTION}` - Brief summary
 
 **3. Act on feedback:**
 - Fix Critical issues immediately
@@ -56,12 +55,11 @@ You: Let me request code review before proceeding.
 BASE_SHA=$(git log --oneline | grep "Task 1" | head -1 | awk '{print $1}')
 HEAD_SHA=$(git rev-parse HEAD)
 
-[Dispatch core-engineering:code-reviewer subagent]
-  WHAT_WAS_IMPLEMENTED: Verification and repair functions for conversation index
+[Dispatch code reviewer subagent]
+  DESCRIPTION: Added verifyIndex() and repairIndex() with 4 issue types
   PLAN_OR_REQUIREMENTS: Task 2 from docs/plans/deployment-plan.md
   BASE_SHA: a7981ec
   HEAD_SHA: 3df7661
-  DESCRIPTION: Added verifyIndex() and repairIndex() with 4 issue types
 
 [Subagent returns]:
   Strengths: Clean architecture, real tests
@@ -82,7 +80,7 @@ You: [Fix progress indicators]
 - Fix before moving to next task
 
 **Executing Plans:**
-- Review after each batch (3 tasks)
+- Review after each task or at natural checkpoints
 - Get feedback, apply, continue
 
 **Ad-Hoc Development:**