fix(review): land CodeRabbit findings from PR #948 that missed the merge

cli/internal/doctor/checks_deploy.go

@@ -355,10 +355,23 @@ const (
 // stripHarnessRegions removes every harness-managed marker region (both the
 // GENERATED and AGENT-PRESENCE kinds) from content, mirroring
 // compile-harness.sh's region_content in reverse (strip instead of extract).
+//
+// Also drops the single blank line immediately preceding a BEGIN marker:
+// both inject_agent_presence and replace_region's append branch write a
+// region as "\n" + BEGIN + body + END + "\n" (compile-harness.sh), so an
+// appended region always leaves that blank separator behind in the deployed
+// file with nothing to match it in the un-appended repo source. Without
+// dropping it, checkInstructionDrift reported drift on every target
+// immediately after a clean --deploy — caught in review before merge.
 func stripHarnessRegions(content string) string {
 	lines := strings.Split(content, "\n")
 	out := make([]string, 0, len(lines))
 	skip, endMarker := false, ""
+	dropPrecedingBlank := func() {
+		if n := len(out); n > 0 && out[n-1] == "" {
+			out = out[:n-1]
+		}
+	}
 	for _, l := range lines {
 		if skip {
 			if l == endMarker {
@@ -368,8 +381,10 @@ func stripHarnessRegions(content string) string {
 		}
 		switch {
 		case strings.HasPrefix(l, harnessBeginPrefix):
+			dropPrecedingBlank()
 			skip, endMarker = true, harnessEndMarker
 		case strings.HasPrefix(l, agentPresenceBeginPrefix):
+			dropPrecedingBlank()
 			skip, endMarker = true, agentPresenceEndMarker
 		default:
 			out = append(out, l)
@@ -410,6 +425,17 @@ func checkHarnessMirrorOrphans(sys *System, cfg *Config, rep *Report, fix bool)
 			continue
 		}
 		repoDir := filepath.Join(repo, "harness", sub)
+		if !isDir(repoDir) {
+			// resolveRepoDir's DOTFILES_REPO_DIR/cwd-git-root cascade proves
+			// only "a git checkout", not "the dotfiles checkout" (no such
+			// validation exists — docs/lessons.md, the resolveRepoDir
+			// test-isolation lesson). If it resolved to an unrelated repo
+			// lacking this subtree entirely, every mirror entry would look
+			// orphaned and --fix would delete the whole harness/<sub> tree.
+			// Refuse to compare rather than risk that.
+			rep.Skip("repo has no " + filepath.Join("harness", sub) + " — orphan comparison skipped (wrong checkout resolved?)")
+			continue
+		}
 		for _, e := range entries {
 			if !e.IsDir() || isDir(filepath.Join(repoDir, e.Name())) {
 				continue

cli/internal/doctor/checks_harness_mirror_test.go

@@ -15,6 +15,7 @@ func TestCheckHarnessMirrorOrphans(t *testing.T) {
 		repo = t.TempDir()
 		mirror = t.TempDir()
 		mkdirAll(t, filepath.Join(repo, "harness", "skills", "kept"))
+		mkdirAll(t, filepath.Join(repo, "harness", "agents")) // present, possibly empty -- a real repo counterpart tree
 		mkdirAll(t, filepath.Join(mirror, "harness", "skills", "kept"))
 		return repo, mirror
 	}
@@ -117,4 +118,27 @@ func TestCheckHarnessMirrorOrphans(t *testing.T) {
 			t.Errorf("repo==mirror should be a silent no-op\n%s", buf.String())
 		}
 	})
+
+	t.Run("repo without harness/skills must not prune the mirror", func(t *testing.T) {
+		// resolveRepoDir proves only "a git checkout", not "the dotfiles
+		// checkout" -- a repo lacking harness/<sub> entirely (wrong repo
+		// resolved, e.g. DOTFILES_REPO_DIR unset + doctor run from inside an
+		// unrelated project) must not read every mirror entry as orphaned.
+		repo := t.TempDir() // no harness/ tree at all
+		mirror := t.TempDir()
+		mkdirAll(t, filepath.Join(mirror, "harness", "skills", "kept"))
+		cfg := &Config{DotfilesDir: mirror}
+		sys := newSys(map[string]string{"DOTFILES_REPO_DIR": repo}, nil, nil)
+
+		var buf bytes.Buffer
+		rep := capture(&buf)
+		checkHarnessMirrorOrphans(sys, cfg, rep, true)
+
+		if !isDir(filepath.Join(mirror, "harness", "skills", "kept")) {
+			t.Errorf("must not prune when the repo has no counterpart tree\n%s", buf.String())
+		}
+		if rep.Failures() != 0 {
+			t.Errorf("a missing counterpart tree is a SKIP, not a FAIL\n%s", buf.String())
+		}
+	})
 }

cli/internal/doctor/checks_instruction_drift_test.go

@@ -48,6 +48,20 @@ func TestStripHarnessRegions(t *testing.T) {
 				"tail\n",
 			want: "head\nmid\ntail\n",
 		},
+		{
+			// inject_agent_presence / replace_region's append branch write a
+			// region as "\n" + BEGIN + body + END + "\n" onto untouched
+			// content -- the blank line right before BEGIN must not survive
+			// the strip, or a freshly-appended region reads as drift forever.
+			name: "drops the blank separator line an appended region leaves behind",
+			in:   "shared content\n" + "\n<!-- BEGIN HARNESS AGENT-PRESENCE (sha256:abc) -->\npersona\n<!-- END HARNESS AGENT-PRESENCE -->\n",
+			want: "shared content\n",
+		},
+		{
+			name: "a genuine blank line NOT before a region is preserved",
+			in:   "para one\n\npara two\n",
+			want: "para one\n\npara two\n",
+		},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -76,12 +90,31 @@ func TestCheckInstructionDrift(t *testing.T) {
 		writeFile(t, filepath.Join(home, homeRel), content)
 	}
 
-	t.Run("matching content -> pass", func(t *testing.T) {
+	t.Run("matching content -> pass (copilot absent, 3 of 4 checked)", func(t *testing.T) {
 		repo, home := setup(t)
 		for _, tgt := range deployedInstructionTargets {
 			writeBoth(t, repo, home, tgt.repoRel, tgt.homeRel, "same content for "+tgt.repoRel+"\n")
 		}
-		sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, nil, nil)
+		sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, nil, nil) // "copilot" NOT on PATH
+
+		var buf bytes.Buffer
+		rep := capture(&buf)
+		checkInstructionDrift(sys, rep)
+
+		if rep.Failures() != 0 {
+			t.Fatalf("matching content should pass\n%s", buf.String())
+		}
+		if !strings.Contains(buf.String(), "match their repo source (3 checked)") {
+			t.Errorf("expected the 3-checked pass line (copilot gated out)\n%s", buf.String())
+		}
+	})
+
+	t.Run("matching content -> pass (copilot present, all 4 checked)", func(t *testing.T) {
+		repo, home := setup(t)
+		for _, tgt := range deployedInstructionTargets {
+			writeBoth(t, repo, home, tgt.repoRel, tgt.homeRel, "same content for "+tgt.repoRel+"\n")
+		}
+		sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, []string{"copilot"}, nil)
 
 		var buf bytes.Buffer
 		rep := capture(&buf)
@@ -90,17 +123,38 @@ func TestCheckInstructionDrift(t *testing.T) {
 		if rep.Failures() != 0 {
 			t.Fatalf("matching content should pass\n%s", buf.String())
 		}
-		if !strings.Contains(buf.String(), "match their repo source") {
-			t.Errorf("expected the pass line\n%s", buf.String())
+		if !strings.Contains(buf.String(), "match their repo source (4 checked)") {
+			t.Errorf("expected the 4-checked pass line (copilot on PATH)\n%s", buf.String())
+		}
+	})
+
+	t.Run("copilot present but genuinely stale -> fails", func(t *testing.T) {
+		repo, home := setup(t)
+		for _, tgt := range deployedInstructionTargets {
+			writeBoth(t, repo, home, tgt.repoRel, tgt.homeRel, "content\n")
+		}
+		writeFile(t, filepath.Join(home, ".copilot", "copilot-instructions.md"), "stale copilot content\n")
+		sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, []string{"copilot"}, nil)
+
+		var buf bytes.Buffer
+		rep := capture(&buf)
+		checkInstructionDrift(sys, rep)
+
+		if rep.Failures() != 1 || !strings.Contains(buf.String(), ".copilot/copilot-instructions.md") {
+			t.Fatalf("stale copilot file with copilot present must fail and name it\n%s", buf.String())
 		}
 	})
 
 	t.Run("deployed copy carries only the injected AGENT-PRESENCE region -> still pass", func(t *testing.T) {
 		repo, home := setup(t)
 		for _, tgt := range deployedInstructionTargets {
 			writeFile(t, filepath.Join(repo, tgt.repoRel), "shared content\n")
+			// Faithful to inject_agent_presence's actual append shape
+			// (compile-harness.sh): "\n" + BEGIN + body + END + "\n" tacked
+			// onto the untouched source — including the blank separator line,
+			// which is exactly what stripHarnessRegions must also drop.
 			deployed := "shared content\n" +
-				"<!-- BEGIN HARNESS AGENT-PRESENCE (sha256:abc) -->\npersona\n<!-- END HARNESS AGENT-PRESENCE -->\n"
+				"\n<!-- BEGIN HARNESS AGENT-PRESENCE (sha256:abc) -->\npersona\n<!-- END HARNESS AGENT-PRESENCE -->\n"
 			writeFile(t, filepath.Join(home, tgt.homeRel), deployed)
 		}
 		sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, nil, nil)

docs/lessons.md

@@ -2294,3 +2294,15 @@ The blast radius was also wider than the one function: because `compile-harness.
 **Rule**: when a cited path doesn't resolve, don't stop at "confirmed missing" — grep the repo for the cited content (a distinctive phrase, a finding ID, a table row) before concluding it was never committed. A file matching the cited *name* in a different repo is not evidence either way; verify its content actually matches what's being cited, not just its filename. And once a citation-convention bug is found in one issue, search for every other issue with the same defect before fixing only the one that was pointed at — the same generation process that produced one dangling citation likely produced several.
 
 **Tags**: `github`, `audit`, `documentation`, `verification`
+
+### [2026-08-13] A PR's `head.sha` not matching your latest push can mean the PR is already merged, not that the API is lagging
+
+**Context**: HARNESS-070 (#843/#869/#828, PR #948). After pushing a commit addressing CodeRabbit's review findings, `gh api .../pulls/948 --jq '.head.sha'` kept returning the previous commit even though `git fetch` confirmed the remote branch ref had the new one. The first read was "GitHub API/webhook propagation lag" — plausible after a session that had already hit a real rate-limit earlier — and a background poll was armed to wait for the field to catch up.
+
+**Problem**: it wasn't lag. `gh api .../pulls/948` also carried `"state": "closed"` and `"merged": true`, fields the lag theory never checked. The PR had been squash-merged (by the user, via the GitHub UI) roughly 21 hours *before* the review-fix commit was even authored. A merged PR's `head.sha` is frozen at merge time by definition — pushing more commits to its (now-orphaned) branch updates the branch ref, never the PR record, and triggers no CI, because there is no open PR for a workflow to run against. The poll loop's exit condition (`head.sha == <new commit>`) could never become true; it would have spun until manually killed regardless of how long anyone waited.
+
+**Solution**: checked `state`/`merged`/`merged_at` on the same API object instead of only `head.sha`, which immediately explained the mismatch. Fix-forward: opened a new branch off the now-updated `main`, cherry-picked the orphaned fix commit (applied with zero conflicts, confirming no semantic drift from other PRs merged in between), reran the full verification suite on the cherry-picked result, and opened a follow-up PR referencing (not closing) the already-closed issues.
+
+**Rule**: before diagnosing a mismatched `head.sha` (or any "why hasn't my push shown up" symptom) as replication lag, check the PR's `state`/`merged` fields on the very first query — a closed PR explains a frozen `head.sha` completely and rules out every lag-based theory in one call. Don't build a polling loop around a diff-based condition (`head.sha == X`) without first confirming the object it lives on is still open; a wait condition that assumes "eventually consistent" when the true shape is "permanently fixed" spins forever and burns a task slot for nothing.
+
+**Tags**: `github`, `ci`, `verification`, `debugging`

scripts/compile-harness.sh

@@ -482,7 +482,7 @@ do_deploy() {
     # these SAME files below. A full-file copy after either would wipe out what
     # they just wrote.
     if jq -e '.agents.presence' "$MANIFEST" >/dev/null 2>&1; then
-        deploy_instructions
+        deploy_instructions || exit 2
     fi
     if ! has_skills && ! has_agents; then
         printf '[deploy] no skills/agents block in manifest; nothing to deploy\n'
@@ -506,7 +506,7 @@ do_deploy() {
 # Entries with no `source` (none today) are skipped -- presence-only injection,
 # same as before this change.
 deploy_instructions() {
-    local agent file source requires dest
+    local agent file source requires dest rc=0
     while IFS=$'\t' read -r agent file source requires; do
         [[ -n "$source" ]] || continue
         if [[ -n "$requires" ]] && ! command -v "$requires" >/dev/null 2>&1; then
@@ -515,6 +515,7 @@ deploy_instructions() {
         fi
         if [[ ! -f "$REPO_ROOT/$source" ]]; then
             printf '[ERROR] instruction source missing: %s\n' "$REPO_ROOT/$source" >&2
+            rc=1
             continue
         fi
         dest="$HOME/$file"
@@ -523,6 +524,11 @@ deploy_instructions() {
         cp -f "$REPO_ROOT/$source" "$dest"
         printf '[deploy] instructions -> %s\n' "$dest"
     done < <(jq -r '.agents.presence[] | "\(.agent)\t\(.file)\t\(.source // "")\t\(.requires_command // "")"' "$MANIFEST")
+    # A missing source is a manifest/repo defect, not a transient skip -- an
+    # [ERROR] line followed by [deploy] OK was the same silently-contradicting
+    # shape deploy_agent_presence had (fixed earlier this PR); propagate it so
+    # `do_deploy` stops instead of claiming success.
+    return "$rc"
 }
 
 # Render committed skill records to their per-agent $HOME paths (offline),

specs/HARNESS-070-deploy-convergence/features.json

@@ -15,10 +15,10 @@
   },
   {
     "id": "HARNESS-070-deploy-convergence-f3",
-    "behavior": "compile-harness.sh --deploy alone (no setup-linux.sh) updates the 4 instruction files without clobbering skill catalog / agent-presence regions",
-    "verification": "HOME=/tmp/fake-home bash scripts/compile-harness.sh --deploy && bash scripts/compile-harness.sh --check",
+    "behavior": "compile-harness.sh --deploy alone (no setup-linux.sh) updates the instruction files without clobbering skill catalog / agent-presence regions, gated on `command -v copilot` for the copilot surface",
+    "verification": "HOME=/tmp/fake-home bash scripts/compile-harness.sh --deploy && bash scripts/compile-harness.sh --check   # copilot-absent path; copilot-present path verified separately with a PATH-stubbed `copilot` binary",
     "state": "pending",
-    "evidence": "Sandboxed run: all 4 files written (.claude/CLAUDE.md, .config/opencode/AGENTS.md, .pi/agent/AGENTS.md; copilot skipped, not on PATH), presence+catalog regions injected afterward with no clobber, second run byte-identical (diff -rq clean), --check exit 0"
+    "evidence": "Sandboxed run, copilot absent: 3 files written (.claude/CLAUDE.md, .config/opencode/AGENTS.md, .pi/agent/AGENTS.md; copilot correctly skipped), second run byte-identical (diff -rq clean), --check exit 0. Sandboxed run, copilot present (PATH-stubbed binary): all 4 files written including the copilot catalog + presence injections, no clobber. Both paths also covered by automated Go tests on the doctor side (checkInstructionDrift's 4-vs-3-checked subtests)."
   },
   {
     "id": "HARNESS-070-deploy-convergence-f4",

specs/HARNESS-070-deploy-convergence/proposal.md

@@ -22,7 +22,7 @@ Three open issues share one root shape: the harness deploy engine converges forw
 
 1. `dotf doctor` detects orphan records under `harness/{skills,agents}` in the deploy mirror (present in `$DOTFILES_DIR`, absent from the repo) and `dotf doctor --fix` prunes them.
 2. `dotf doctor` FAILs (not WARNs) when the installed `dotf` version differs from the `versions.conf` pin — a stale `dotf` means whatever guards shipped after it was built are not running at all, which is a harder failure than an ordinary tool being one version behind.
-3. `compile-harness.sh --deploy` copies the full doctrine/instruction files (`ai/claude/CLAUDE.md`, `AGENTS.md`, `ai/copilot/copilot-instructions.md`) to their per-agent `$HOME` paths itself, so a standalone `--deploy` run — without a full `setup-linux.sh` pass — brings all six surfaces (agy, codex, claude, opencode, pi, copilot) current in one command.
+3. `compile-harness.sh --deploy` copies the full doctrine/instruction files (`ai/claude/CLAUDE.md`, `AGENTS.md`, `ai/copilot/copilot-instructions.md`) to their per-agent `$HOME` paths itself, so a standalone `--deploy` run — without a full `setup-linux.sh` pass — brings all six surfaces (agy, codex, claude, opencode, pi, copilot) current in one command. The `copilot` surface is gated on `command -v copilot`, same as the skill deploy already was: when the binary is absent, that target is skipped (not written, not compared as drift), and the other three targets converge unconditionally.
 4. `dotf doctor` reports (never silently tolerates) a deployed instruction file that has drifted from its repo source.
 5. **Correction to the framing this spec was scoped under**: the 4 "symlinked skills" evidence (`computer-use`, `find-skills`, `orca-cli`, `orchestration`) is investigated and found to be a false positive in `checkDeployedSkillSymlinks`, not a BUG-100 regression — those names have no `harness/skills/` record; they are Orca's own `~/.agents/skills/` symlink mechanism, deliberately excluded from the strict symlink sweep by prior art (AI-022 spec, for the `pi` case). The check is narrowed to only flag symlinks at names the harness actually manages, matching the policy `warn_unmanaged_output` already applies on the deploy side.
 

specs/HARNESS-070-deploy-convergence/tasks.md

@@ -29,7 +29,7 @@ created: "2026-08-12"
 
 ## Closing
 
-- [x] Every acceptance criterion from `proposal.md` is covered by at least one test
+- [x] Every acceptance criterion from `proposal.md` has at least one form of verification: AC1/AC2/AC4/AC5 by automated Go tests, AC3 (the `compile-harness.sh --deploy` shell behavior) by repeated manual sandboxed runs only — no automated bats coverage this PR (`tests/*.bats` out of scope this session, see "Out of scope"; a follow-up ticket is proposed, not filed, in the PR body)
 - [x] `features.json` entries added for each criterion
 - [x] Type checks pass (`go build ./... && go vet ./...`)
 - [x] Lint passes (`golangci-lint run` — 0 issues; `shellcheck` — clean)