fix(space): make root-repo sync an explicit post-approval merge step

[lsm]

Step 5 of PR_MERGE_POST_APPROVAL_INSTRUCTIONS claimed root-repo sync was "handled outside the isolated worktree" — false, since the reviewer post-approval session is the only actor that runs it. The disclaim caused a reviewer to skip syncing after merging PR #2307, leaving the checkout stale.

The final step 5 (after four review rounds):

• Branch-agnostic — derives the base from the PR's baseRefName ($BASE), not a hard-coded dev, so these product built-ins work against any base branch.
• Syncs the configured Space checkout — uses the already-threaded {{workspace_path}} token (= space.workspacePath, what createTaskWorktree branches future task worktrees from via git worktree add … HEAD). Earlier rounds tried the worktree-isolation banner (absent — no session.worktree), a literal <mainRepoPath> (not an interpolation token), and git rev-parse --git-common-dir (resolves to the shared main-repo .git, a different checkout when the workspace is a linked worktree); all incorrect.
• Guards — refuses a checkout not on $BASE, refuses to claim sync when local $BASE is ahead of origin/$BASE ("Already up to date" hides stray commits), and best-effort cleanup_warning on pull failure.
• Idempotent — step 1's already-merged path now runs the sync too, so a restart after merge still fast-forwards the checkout.

Fixes all three consumers (Coding / Research / QA) via the shared constant, plus strengthened regression tests.

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: APPROVE — zero P0–P3 findings. (Own-PR: GitHub rejects APPROVE from the author, so posting as COMMENT; this is an explicit approve.)

Verdict

The fix is correct, minimal, and faithful to the task. The false claim ("Root repo synchronization is handled outside the isolated worktree") is removed and replaced with an explicit root-repo sync instruction. Step 4 (branch deletion) left untouched as required.

Verified

• Correctness of the new instruction. git -C <mainRepoPath> pull --ff-only origin dev is the right way to fast-forward the root checkout from inside the worktree. <mainRepoPath> is the path the worktree-isolation banner already surfaces (query-options-builder.ts:977 → Main repository (read-only unless post-approval instructions say otherwise): ${wt.mainRepoPath}), and the banner's own clause is exactly what permits this write. mainRepoPath is typed string (non-null) on the internal worktree type and resolves to the git root, so it is always present in the reviewer session.
• Worktree-safe. Step 5a keeps git fetch origin dev and forbids git checkout dev in the isolated worktree (a branch can't be checked out in two worktrees at once). Good.
• Best-effort handling. ff-only failure → NON-result cleanup_warning artifact + continue, mirroring step 4. Consistent.
• Single source of truth. All three consumers (built-in-workflows.ts:517 Coding, :663 Research, :1090 Fullstack QA) reference the one constant — fixing it fixes all three. Confirmed.
• Regression test is well-placed (next to the step-4 test), well-commented, asserts both the absence of the false claim and the presence of git -C … pull --ff-only. Passes.
• No test collisions. The QA-prompt not.toContain('git pull --ff-only') at end-node-handoff.test.ts:241 targets a different prompt (endNodePrompt(FULLSTACK_QA_LOOP_WORKFLOW)), and the "safe for isolated worktrees" test at built-in-workflows.test.ts:6212 (git fetch origin dev, do NOT \git checkout dev`, no git checkout dev && git pull`) still holds. Both files: 348 pass / 0 fail.

Checks

• ./scripts/test-daemon.sh → end-node-handoff + built-in-workflows: 348 pass, 0 fail.
• bun run check: lint ✅ typecheck ✅ knip ✅ session-guards ✅ db-schema-parity ✅. The one red is check:test-quality flagging 1-core/providers/provider-registry.test.ts:748 — identical to dev and untouched by this PR, so pre-existing baseline noise, out of scope here.
• CI (PR matrix): 31 jobs pass; CLI/Build/E2E/Discover correctly skipping (gated to dev/workflow_dispatch); no failures. Zero review conversations to resolve.

Surgical, two-file change, no scope creep. Approving.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f7f2832be

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: REQUEST_CHANGES — superseding my earlier APPROVE. Fresh round triggered by chatgpt-codex-connector's inline comments, which I verified against the code. One P1 blocks: the new step 5b cannot actually run as written, so the PR does not achieve its stated goal.

I previously approved based on the diff being faithful to the task spec and passing tests. That was incomplete — I did not verify the runtime assumption the new wording depends on (that the reviewer session receives the worktree-isolation banner). It does not. Reversing on the new evidence.

---

P1 — <mainRepoPath> is an unresolved literal; the worktree-isolation banner is never delivered to the post-approval session

Step 5b tells the reviewer to read mainRepoPath from "the worktree-isolation banner above." That banner is appended by QueryOptionsBuilder.getWorktreeIsolationText() only when session.worktree is set (query-options-builder.ts:932, 945). For the post-approval reviewer session it is not set:

• Space agent sessions are type: "worker" (custom-agent.ts:690), built via resolveAgentInit → createCustomAgentInit, which carry only workspacePath, never a WorktreeMetadata.
• createSessionFromInit (agent-session.ts:675) returns a Session with no worktree field; createSubSession's new-session path never sets it; task-agent-manager stores only the path string in taskWorktreePaths (line 491); createTaskWorktree returns { path } (line 749) without touching any session.
• query-lifecycle-manager.test.ts:1357+ has to manually set mockContext.session.worktree = {…} to exercise the banner; archive-session.test.ts:151,226 asserts session.worktree is undefined for task sessions.

Net effect: the reviewer runs git -C <mainRepoPath> pull --ff-only origin dev with the literal string, the command fails, the new best-effort clause emits a cleanup_warning, and the root repo stays stale — the exact regression this PR claims to fix. The regression test only checks the string git -C .*pull --ff-only is present, so it passes while the behavior is still broken.

Fix (stays in this one file — no token/banner plumbing): derive the root path from git instead of referencing an absent banner:

ROOT=$(cd "$(git rev-parse --git-common-dir)/.." && pwd)
git -C "$ROOT" pull --ff-only origin dev

--git-common-dir inside a worktree resolves to the main repo's .git, so its parent is the root checkout. (Larger alternative: thread mainRepoPath as a real {{main_repo_path}} token through PostApprovalRouter + interpolatePostApprovalTemplate.) Detailed verification + fix posted as an inline reply on line 211.

P2 — guard the pull against a non-dev root branch (bot comment #2)

git -C "$ROOT" pull --ff-only origin dev merges origin/dev into whatever branch the root checkout is currently on, not necessarily dev. --ff-only only aborts on non-fast-forward; if the checked-out branch is an ancestor of origin/dev, it silently fast-forwards the wrong branch to the dev tip. Cheap mitigation while reworking 5b: confirm git -C "$ROOT" rev-parse --abbrev-ref HEAD is dev first; otherwise record a cleanup_warning and skip.

P2 — run the root sync on the "already merged" early-exit path (bot comment #3)

Step 1 exits early when state === MERGED ("record an audit artifact and exit — the work is done"). If the reviewer/daemon restarts after a successful merge, the resumed session starts at step 1, sees MERGED, and exits — never reaching step 5, so the root is left stale in precisely the restart window where post-approval is meant to be idempotent. The merged-exit branch should still perform the root sync (5b) before completing.

P2 — treat a root dev that is ahead of origin/dev as a warning, not success (bot comment #4)

If the root dev has unpushed local commits ahead of origin/dev, pull --ff-only reports "Already up to date" and leaves those commits at HEAD; step 5 reports success while the root tip differs from origin/dev. If later task worktrees branch from the root's HEAD, they inherit those commits. Cheap mitigation: after git fetch origin dev, compare git -C "$ROOT" rev-parse dev with origin/dev; on an ahead-only state, record a cleanup_warning rather than claiming sync.

---

The P1 is the blocker. P2s are cheap and touch the same step, so worth folding in while reworking 5b. The single-constant fix covering Coding/Research/QA and the worktree-safe git fetch origin dev in 5a remain correct and should be kept.

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: APPROVE — zero P0–P3 findings. (Own-PR: GitHub rejects APPROVE from the author, so posting as COMMENT; this is an explicit approve of commit 16c86e5.)

Round 2 (fresh): all four round-1 findings addressed correctly and verified.

P1 (was the blocker) — resolved & empirically verified

Step 5b no longer references the absent worktree-isolation banner or the literal <mainRepoPath>. The root path is derived from git: ROOT=$(cd "$(git rev-parse --git-common-dir)/.." && pwd). I ran this in the actual worktree — --git-common-dir resolves to /Users/lsm/focus/dev-neokai/.git, so the parent is the correct main repo /Users/lsm/focus/dev-neokai; it also resolves correctly when cwd is the main repo itself. The new wording even explains why the banner is absent ("the post-approval session receives NO worktree-isolation banner (it has no session.worktree)"), which will stop the assumption recurring. The strengthened regression test now asserts <mainRepoPath> is absent and git rev-parse --git-common-dir is present — round-1's test only checked the command string and gave false confidence; this one would catch a regression to that approach.

P2s — all folded into the same step, correct

1. Non-dev root guard: checks git -C "$ROOT" rev-parse --abbrev-ref HEAD == dev before pulling; otherwise a NON-result cleanup_warning and skip. Correct — pull --ff-only origin dev fast-forwards the checked-out branch, not dev by name.
2. Already-merged path runs the sync: step 1 no longer exits immediately on state === MERGED — it records an audit artifact, performs step 5 only, then exits (and skips the step-6 merge artifact since no merge happened this session). Closes the restart-after-merge idempotency gap.
3. Ahead-of-origin guard: after the ff-only pull, compares rev-parse HEAD vs rev-parse origin/dev; they differ only when local dev is ahead (the "Already up to date" case hiding stray commits) → NON-result cleanup_warning. Logic is sound: a successful fast-forward makes HEAD == origin/dev, so the warning fires precisely in the intended case.

Checks

• end-node-handoff + built-in-workflows: 348 pass, 0 fail (1541 expects); strengthened regression test passes (6 expects).
• bun run lint, typecheck, knip, check:session-guards, check:db-schema-parity: all clean. (The unrelated pre-existing check:test-quality noise in 1-core/providers/provider-registry.test.ts is identical to dev and out of scope.)
• All 5 review conversations resolved; CI on 16c86e5 green (applicable jobs pass; CLI/Build/E2E/Discover correctly skipping on a PR).

Clean fix to a genuine integration defect. Approving.

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: REQUEST_CHANGES — supersedes the round-2 APPROVE. New finding raised by the operator: the template hardcodes dev, which is HyperNeo-specific. As a product built-in that runs against arbitrary user repos, it must be branch-agnostic.

P1 — Template hardcodes dev throughout; derive the base branch from the PR

dev is this project'"'"'s convention (CLAUDE.md: "All PRs target dev"), but PR_MERGE_POST_APPROVAL_INSTRUCTIONS ships as a built-in for any repo. Every literal dev / origin/dev breaks on a main-based (or any non-dev) repo — step 3'"'"'s git fetch origin dev would fetch a non-existent branch, the merge-tree/rebase targets would be wrong, and step 5 would refuse to pull. The PR'"'"'s own base ref is the correct, repo-agnostic target.

Plan (operator-approved scope: whole template, derive from the PR):

1. Extract the base branch once, up front (fold into step 1'"'"'s existing gh pr view, or a dedicated preamble line):

   BASE=$(gh pr view {{pr_url}} --json baseRefName --jq .baseRefName)
   

   Use baseRefName (the branch the PR merges INTO), NOT defaultBranchRef — you fast-forward the branch the PR was merged into, which can differ from the repo default (e.g. a PR targeting a release branch).

2. Replace every literal dev / origin/dev in steps 3–5 with $BASE / origin/$BASE. Functional occurrences (current line numbers):

   • Step 3: L85 origin/dev (prose), L88 git fetch origin dev, L89 git merge-tree … origin/dev, L96 & L122 base_branch: "dev" (conflict artifacts), L125 origin/dev (rebase target).
   • Step 4: L206 Do NOT delete \dev`→Do NOT delete `$BASE``.
   • Step 5: L210 git checkout dev, L212 git fetch origin dev, L218 pull --ff-only origin dev, L219 root not on \dev`, L220 local `dev` ahead of `origin/dev`, L223 guard != "dev", L225 root not on dev, L227 fetch origin dev, L228 pull --ff-only origin dev, L229 rev-parse origin/dev, L230 & L232 dev ahead of origin/dev`.

3. Genericize the prose for consistency: file-header L38 ("conflict with the base branch (dev)"), and step-5 intro L64 / L207 / L209 ("track the freshly-merged dev", "inherits old dev") — refer to "the base branch ($BASE)" instead.

4. Update the tests that pin dev-specific strings (they will break):

   • end-node-handoff.test.ts:518 toContain('base_branch: "dev"'), :534 toContain('origin/dev').
   • built-in-workflows.test.ts:6213 toContain('git fetch origin dev'), :6214 toContain('do NOT \git checkout dev`')`.
   • The round-2 regression test'"'"'s root dev ahead of origin/dev assertion (end-node-handoff ~L401) → the $BASE form.
   • Add a branch-agnostic guard so it can'"'"'t regress: expect(...).not.toMatch(/origin\/dev\b/) and expect(...).toContain('--jq .baseRefName').
   • Unaffected / leave as-is: built-in-workflows.test.ts:968 (asserts PLAN_AND_DECOMPOSE_WORKFLOW.description — a different workflow, not this template) and the QA-prompt not.toContain('git checkout dev') at end-node-handoff.test.ts:242 (negative assertion on a different prompt).

Keep as-is: step 5b'"'"s git rev-parse --git-common-dir root derivation (already branch-independent), the non-dev / ahead-of-origin guards, the already-merged→step-5-only routing, and the best-effort cleanup_warning handling — all of those remain correct once dev→$BASE.

Net: one preamble line + a mechanical dev→$BASE sweep across steps 3–5 + test updates. The round-2 root-derivation fix is preserved.

[lsm]

🤖 Review by glm-5.1 (Anthropic)

Model: glm-5.1 | Client: NeoKai | Provider: Anthropic

Recommendation: APPROVE — zero P0–P3 findings. (Own-PR: GitHub rejects APPROVE from the author, so posting as COMMENT; explicit approve of commit d402209.)

Round 4 (fresh): the branch-agnosticism concern is fully resolved and verified.

Verified

• Base branch derived from the PR, not hard-coded. BASE=$(gh pr view {{pr_url}} --json baseRefName --jq .baseRefName) is extracted in step 1 (line 61), before its first use in step 3 (line 89), with clear prose noting baseRefName (the branch the PR merges INTO) is used over the repo default — correct for release-branch PRs. Sweep is complete: zero functional origin/dev; the only dev left is line 64'"'"'s intentional example list (dev/main/master/...) explaining why the built-in must be repo-agnostic.
• $BASE substitutions are correct and quoted, including the step-5 guard != "$BASE" and the ahead-check rev-parse "origin/$BASE". Logic preserved end-to-end across steps 3 (fetch / merge-tree / both base_branch artifacts / rebase target), 4 (Do NOT delete \$BASE`), and 5. The non-base guard correctly refuses to fast-forward when the root sits on a different branch (e.g. a release-branch PR with a root on main`) — defensive, not a bug.
• Round-2 work intact: git-derived root path (git rev-parse --git-common-dir), non-base + ahead-of-origin guards, already-merged→step-5-only routing, best-effort cleanup_warning handling — all still correct under $BASE.
• Regression lock present: end-node-handoff.test.ts:412 asserts --jq .baseRefName; :413 asserts not /origin\/dev\b/. So a future edit can'"'"'t silently re-introduce a hard-coded branch.

Checks

• end-node-handoff + built-in-workflows: 349 pass, 0 fail (1544 expects).
• bun run lint, typecheck, knip, check:session-deletion-callers, check:db-schema-parity: all clean.
• Review threads: 5/5 resolved, 0 unresolved.
• CI on d402209: 17 pass, 12 pending, 0 fail (in-flight — the slower integration shards). The merge template self-gates on CI at step 1 (gh pr checks before merge), so the PR cannot merge until CI is green regardless of approval timing.

Optional polish (not a finding)

The PR description still narrates the round-2 wording ("guards for a root not on dev, a local dev ahead of origin/dev"). The code is correct; only the description text is stale. Feel free to refresh it to baseRefName/$BASE for accuracy — no blocker.

Clean, repo-agnostic built-in. Approving.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d402209c5d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 971625b532

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a81043548b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lsm]

🤖 Review by glm-5.1 (GLM)

Model: glm-5.1 | Client: NeoKai | Provider: GLM

Recommendation: APPROVE (posted as COMMENT — GitHub blocks self-APPROVE because the round-5 commits below were authored by this same reviewer account during the prior post-approval session.)

Round 5 re-review — APPROVE for merge. The branch advanced three commits (971625b5, a8104354, 3d65b631f) past the round-4 approval at d402209c5, so those needed a fresh sign-off before merge. Independently verified each against the codebase:

• {{workspace_path}} is a real token — POST_APPROVAL_TEMPLATE_KEYS lists it (post-approval-template.ts:59), populated from space.workspacePath at space-runtime.ts:4232. Renders verbatim via the interpolator.
• Syncing the Space checkout (not the git-inferred root) is correct — createTaskWorktree runs git worktree add … HEAD with cwd: space.workspacePath (space-worktree-manager.ts:144-146), so future task worktrees branch from the workspace's HEAD. git rev-parse --git-common-dir would resolve to the main-repo .git whose parent is a different checkout when the workspace is a linked worktree — round-5a's switch to the supplied path is the right fix.
• For this Space, workspacePath == /Users/lsm/focus/dev-neokai (the main repo) — so step 5b syncs exactly the path the original "root repo stale" bug was about, and is product-correct for the linked-worktree case.
• Shell correctness — SPACE_WS='{{workspace_path}}' single-quoted (no re-expansion); $BASE re-derived per Bash block (fresh shell per call); <base branch> placeholder used in tool-call payloads where $BASE would be sent literally.

Verification: end-node-handoff.test.ts 50/0 pass; lint + typecheck + knip + session-guards + db-schema-parity all clean; CI green on 3d65b631f (all PR checks pass). PR diff is correctly scoped to 3 files (+109/−20: template + two test files); step 4 untouched. The lone check:test-quality failure is pre-existing on dev (provider-registry.test.ts, untouched by this PR) — out of scope.

Non-blocking nit (P3): step 1's MERGED early-exit still glosses "fast-forward the root repository" while step 5 authoritatively syncs the "Space checkout." For this Space they're the same path and step 1 defers to "perform step 5 ONLY," so behavior is unaffected; the wording can be reconciled later.

Proceeding to squash-merge.