From 81d270ad07be8389928d5a96e62cf24e9e9dcb0c Mon Sep 17 00:00:00 2001 From: Manu Date: Thu, 13 Aug 2026 18:18:27 -0600 Subject: [PATCH 1/2] fix(review): address CodeRabbit findings on PR #948 Six findings, each verified against actual behavior before acting: - stripHarnessRegions didn't drop the blank separator line inject_agent_presence/replace_region's append branch always writes before a region. Reproduced empirically with a real sandboxed --deploy run: a genuinely clean deploy produced 3 false FAILs in checkInstructionDrift. Fixed and re-verified clean. Most severe finding -- the check was actively broken for its stated purpose. - checkHarnessMirrorOrphans --fix could delete an entire harness/ tree in the mirror if resolveRepoDir ever resolved to a checkout lacking that subtree (unrelated repo, DOTFILES_REPO_DIR unset) -- every mirror entry would read as orphaned. Added a guard: skip the whole subtree comparison when the repo counterpart directory is absent, rather than treating that as "everything is orphaned." - deploy_instructions' missing-source case printed [ERROR] but let do_deploy still print [deploy] OK and exit 0 -- the same contradicting-log-lines shape as the deploy_agent_presence bug fixed earlier this PR. Now propagates a non-zero exit, matching deploy_agents' existing convention for a missing record directory. - AC3's copilot-gate condition wasn't stated in the spec's "What" section (only in the AC itself) and had no test coverage for the copilot-PRESENT path. Added Go tests for both paths (verified against a real sandboxed deploy with a PATH-stubbed copilot binary first) and tightened the spec wording. - tasks.md's closing checklist overstated test coverage ("every AC covered by at least one test") when AC3 is manual-only, no bats. Reworded to be precise about automated vs. manual verification. - Declined: table-driven-tests-with-status-tags suggestion for checks_symlinks_test.go. This package's own established convention (checks_deploy_drift_test.go) asserts on prose substrings throughout, so the suggestion would make this one file inconsistent with its neighbors. Recorded, not silently ignored. Full details in specs/HARNESS-070-deploy-convergence/verification.md. --- cli/internal/doctor/checks_deploy.go | 26 ++++++++ .../doctor/checks_harness_mirror_test.go | 24 +++++++ .../doctor/checks_instruction_drift_test.go | 64 +++++++++++++++++-- scripts/compile-harness.sh | 10 ++- .../features.json | 6 +- .../proposal.md | 2 +- specs/HARNESS-070-deploy-convergence/tasks.md | 2 +- .../verification.md | 9 ++- 8 files changed, 130 insertions(+), 13 deletions(-) diff --git a/cli/internal/doctor/checks_deploy.go b/cli/internal/doctor/checks_deploy.go index 6bf693eb..a8084025 100644 --- a/cli/internal/doctor/checks_deploy.go +++ b/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/ 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 diff --git a/cli/internal/doctor/checks_harness_mirror_test.go b/cli/internal/doctor/checks_harness_mirror_test.go index 854253cf..b5a4b911 100644 --- a/cli/internal/doctor/checks_harness_mirror_test.go +++ b/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/ 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()) + } + }) } diff --git a/cli/internal/doctor/checks_instruction_drift_test.go b/cli/internal/doctor/checks_instruction_drift_test.go index 0de3b2d2..b179b116 100644 --- a/cli/internal/doctor/checks_instruction_drift_test.go +++ b/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\npersona\n\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,8 +123,25 @@ 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()) } }) @@ -99,8 +149,12 @@ func TestCheckInstructionDrift(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" + - "\npersona\n\n" + "\n\npersona\n\n" writeFile(t, filepath.Join(home, tgt.homeRel), deployed) } sys := newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": repo}, nil, nil) diff --git a/scripts/compile-harness.sh b/scripts/compile-harness.sh index 1512a2bc..d69f701e 100755 --- a/scripts/compile-harness.sh +++ b/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), diff --git a/specs/HARNESS-070-deploy-convergence/features.json b/specs/HARNESS-070-deploy-convergence/features.json index d52fa629..bdde5097 100644 --- a/specs/HARNESS-070-deploy-convergence/features.json +++ b/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", diff --git a/specs/HARNESS-070-deploy-convergence/proposal.md b/specs/HARNESS-070-deploy-convergence/proposal.md index ccc8f626..b469ec5b 100644 --- a/specs/HARNESS-070-deploy-convergence/proposal.md +++ b/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. diff --git a/specs/HARNESS-070-deploy-convergence/tasks.md b/specs/HARNESS-070-deploy-convergence/tasks.md index 5e368d0b..82b6c0b0 100644 --- a/specs/HARNESS-070-deploy-convergence/tasks.md +++ b/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) diff --git a/specs/HARNESS-070-deploy-convergence/verification.md b/specs/HARNESS-070-deploy-convergence/verification.md index 12f4aca8..7debdaa5 100644 --- a/specs/HARNESS-070-deploy-convergence/verification.md +++ b/specs/HARNESS-070-deploy-convergence/verification.md @@ -9,7 +9,7 @@ created: "2026-08-12" - [x] AC1 -> `cli/internal/doctor/checks_harness_mirror_test.go`, test `TestCheckHarnessMirrorOrphans` (5 subtests, all pass) - [x] AC2 -> `cli/internal/doctor/checks_test.go`, test `TestCheckOptionalTools_DotfDrift` (updated to assert FAIL) -- [x] AC3 -> manual: `HOME=/tmp/fake-home bash scripts/compile-harness.sh --deploy` updated all 4 instruction files; a second run was a byte-for-byte no-op (`diff -rq` clean); `--check` stayed green throughout +- [x] AC3 -> manual, both copilot paths: `HOME=/tmp/fake-home bash scripts/compile-harness.sh --deploy` (copilot absent — 3 files updated, gated correctly) and the same with a PATH-stubbed `copilot` binary (all 4 files updated, including the catalog + presence injections into copilot-instructions.md); each a byte-for-byte no-op on a second run (`diff -rq` clean); `--check` stayed green throughout. No automated bats coverage this PR (see proposal.md "Out of scope"). The copilot-gate condition itself is additionally covered by an automated Go test on the doctor side (`checkInstructionDrift`). - [x] AC4 -> `cli/internal/doctor/checks_instruction_drift_test.go`, tests `TestStripHarnessRegions`, `TestCheckInstructionDrift`, `TestHarnessMarkerConstants` (all pass); confirmed live on this machine — correctly FAILs on the 4 files that are genuinely stale here right now - [x] AC5 -> `cli/internal/doctor/checks_symlinks_test.go`, test `TestCheckDeployedSkillSymlinks` (5 subtests); confirmed live — the 4 previously-flagged Orca-managed symlinks (`computer-use`, `find-skills`, `orca-cli`, `orchestration`) no longer fail `dotf doctor` @@ -32,6 +32,13 @@ created: "2026-08-12" - `checkHarnessMirrorOrphans` and `checkInstructionDrift` are gated to SKIP silently (not FAIL) when the repo checkout is unresolvable, matching `checkDeployDrift`'s existing convention — a doctor run on a machine with only the deploy mirror present (no checkout) is a legitimate, common case. - Test isolation lesson: `resolveRepoDir`'s `os.Getwd()`/git-root fallback resolves to the REAL checkout during `go test` execution (this repo has `.git` above `cli/internal/doctor`), so a naive "unresolvable repo" test case is nondeterministic across environments — worked around by either avoiding the scenario (mirror-orphans test) or by skipping the test when the fallback actually resolves (instruction-drift test). - A pre-PR advisor pass caught two real blockers before push: (1) `checkInstructionDrift` compared the copilot file unconditionally, which would have made it a permanent FAIL on any machine with a leftover file and no `copilot` binary — the exact "FAIL no remedy clears" shape #843 is about; fixed by gating on `requires_command`, same as the deploy side. (2) `deployedInstructionTargets`'s doc comment promised a manifest-sync test that didn't exist yet; written as `TestCheckInstructionDrift_MatchesManifest`. Also caught: `features.json` had self-set `"state": "passing"`, which the spec template reserves for the harness only — reset to `"pending"`. +- CodeRabbit's post-push review on the PR found 6 issues, verified individually against the actual behavior rather than trusted at face value: + - **Confirmed and fixed (Major)**: `stripHarnessRegions` didn't drop the blank separator line `inject_agent_presence`/`replace_region`'s append branch always writes before a region — reproduced empirically with a real sandboxed `--deploy` run (3 false FAILs on a genuinely clean deploy), fixed, re-verified clean. This was the most serious finding: `checkInstructionDrift` was actively broken for its stated purpose. + - **Confirmed and fixed (Major, data-loss risk)**: `checkHarnessMirrorOrphans --fix` would delete an entire `harness/` tree in the mirror if `resolveRepoDir` ever resolved to a checkout lacking that subtree (e.g. run from an unrelated repo with `DOTFILES_REPO_DIR` unset) — every mirror entry would read as orphaned. Added a guard: skip (not fail) the whole subtree comparison when the repo counterpart directory doesn't exist at all. + - **Confirmed and fixed (Major)**: `deploy_instructions`' missing-source case printed `[ERROR]` but let `do_deploy` still print `[deploy] OK` and exit 0 — the same contradicting-log-lines shape as the `deploy_agent_presence` bug found earlier this session. Now propagates a non-zero exit, matching `deploy_agents`' existing convention. + - **Confirmed and fixed (Major, documentation)**: AC3's copilot-gate condition wasn't stated in the "## What" section (only in the AC itself) and wasn't tested for the copilot-PRESENT path at all — added Go test coverage for both paths and tightened the wording in `proposal.md`/`features.json`/this file. + - **Confirmed and fixed (Major, documentation)**: `tasks.md`'s closing checklist claimed blanket "every AC covered by at least one test," which overstated AC3 (manual-only, no bats). Reworded to be precise about which ACs have automated vs. manual verification. + - **Investigated and deferred (Minor)**: table-driven-tests-with-status-tags suggestion for `checks_symlinks_test.go`. Declined — this package's own established convention (`checks_deploy_drift_test.go`) asserts on prose substrings (`wantSubstr`) throughout, so the suggestion would make this one file inconsistent with its neighbors rather than more consistent. Not blocking; noted here rather than silently ignored. ## Promotion candidates From 5b482ce2ef633c55cb3a1d180c9101d865600d75 Mon Sep 17 00:00:00 2001 From: Manu Date: Thu, 13 Aug 2026 18:38:20 -0600 Subject: [PATCH 2/2] docs(lessons): record the merged-PR-vs-API-lag misdiagnosis PR #948 was squash-merged before this branch's CodeRabbit review-fix commit landed, so the fix commit never got CI and never merged. Record the diagnostic lesson (check state/merged before diagnosing head.sha mismatch as lag) alongside the follow-up fix. --- docs/lessons.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/lessons.md b/docs/lessons.md index 99d57d13..3c2a116d 100644 --- a/docs/lessons.md +++ b/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 == `) 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`