fix(harness): converge the deploy engine — prune, drift detection, all six surfaces - #948
Merged
Merged
Conversation
Three gaps in dotf doctor's harness-deploy convergence, reproduced live
on this machine:
- checkHarnessMirrorOrphans: a harness/{skills,agents} record deleted
from the repo survived forever in the ~/.dotfiles mirror (copy-only,
no prune) and kept failing checkCompileHarnessDrift, which runs FROM
the mirror. --fix now prunes it, per #802's decided doctor-prunes/
setup-only-warns semantic. Closes #843 (BUG-058).
- checkOptionalTools: a dotf version behind the versions.conf pin now
FAILs instead of WARNs -- a stale dotf silently carries none of the
guards merged since it was built, which is a harder failure than an
ordinary tool being one version behind. Closes #869 (OPS-025).
- checkDeployedSkillSymlinks: narrowed to only flag a symlink whose
name has a harness/skills/ record. Investigating the handoff's "4
symlinked skills = BUG-100 regression" evidence found it was a false
positive instead -- all four resolve to Orca's own ~/.agents/skills
mechanism, unrelated to this repo's records, with documented prior
art for the exclusion in specs/archive/AI-022-pi-harness-slot/. Filed
and closes #943 (BUG-074).
- checkInstructionDrift (new): reports a deployed instruction file
(~/.claude/CLAUDE.md and the other three presence targets) that has
drifted from its repo source, region-stripped so the AGENT-PRESENCE
and skill-catalog injections don't false-fail the comparison. Part of
#828 (HARNESS-058) -- the deploy-side half is the next commit.
All four confirmed against real machine state, not just synthetic
fixtures. Go tests only in this PR (tests/*.bats out of scope this
session per multi-session coordination).
Doctrine reached agy/codex (the compact payload) through --deploy but reached claude/opencode/pi/copilot only through a full setup-linux.sh run -- so a standalone --deploy left four of six surfaces stale after a merge. Extends the existing agents.presence[] manifest entries with source/requires_command fields (they already name the four target files) and adds deploy_instructions, run first in do_deploy so the later skill-catalog and AGENT-PRESENCE region injections land on top of a fresh copy instead of being clobbered by it. Also fixes a pre-existing bug found while sandbox-testing this: deploy_agent_presence always logged "[deploy] presence -> ..." as success even when inject_agent_presence skipped because the target file was absent, producing two directly contradicting log lines for the same path in one run. inject_agent_presence now returns non-zero on that no-op path and the caller only logs success when it actually wrote something. Verified with a sandboxed HOME=/tmp/fake-home --deploy run: all four files written, a second run is a byte-for-byte no-op, and --check stays green throughout. setup-linux.sh's existing copies are left in place untouched (now redundant, still correct) -- out of this session's assigned surface. Windows keeps its own unrelated deploy path (setup-windows.ps1 sets DotfilesDir = the checkout itself, so it never had this gap). Part of #828 (HARNESS-058); the detection-side half (checkInstructionDrift) was the previous commit.
Two lessons from this session: verifying a "known bug class" label against the actual symlink targets before building a fix on it (the BUG-100 evidence turned out to be a false positive, not a regression), and resolveRepoDir's cwd/git-root fallback silently defeating an "unresolvable repo" test case run from inside a real checkout.
…nifest sync checkInstructionDrift compared the copilot instruction file unconditionally, but deploy_instructions only ever writes it when `copilot` is on PATH -- on a machine where the file exists (e.g. left over from before copilot was uninstalled) but the binary is absent, that was a FAIL no remedy could ever clear, the exact #843 signal-rot this session exists to kill. deployedInstructionTargets now carries the same requires_command gate deploy_instructions already applies, and a new test (TestCheckInstructionDrift_MatchesManifest) parses harness/manifest.json's agents.presence[] and asserts it stays in sync with the Go mirror, since that manifest is the actual SSOT the shell side reads and this Go list only exists because doctor must run with no repo present. Also resets features.json's five entries from "state": "passing" back to "pending" -- the spec template is explicit that only the harness, not the agent, may write that terminal state; the evidence strings stay as the record of what was actually run.
📝 WalkthroughWalkthroughThe change adds manifest-driven instruction deployment, instruction drift checks, mirror orphan cleanup, managed symlink filtering, and dotf version drift failures. It also adds regression tests and HARNESS-070 deployment-convergence specifications. ChangesDeployment convergence
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to The deployment changes can falsely report drift after a clean deploy, delete an entire mirrored harness when run from the wrong checkout, and report success when an instruction source is missing. These correctness and availability risks should be fixed before merging. Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 tasks
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: mlorentedev <13562150+mlorentedev@users.noreply.github.com>
Contributor
mlorentedev
added
the
skip-archive
Owner
Author
|
Cross-session note from the branch working on BUG-074 (PR #950), which hit the identical spec-gate failure. spec-gate is red for archive-on-merge, not for the LOC threshold: Two things that cost me time and may save you some:
Also, unrelated to the gate: CodeRabbit reports |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/internal/doctor/checks_deploy.go`:
- Around line 358-379: Update stripHarnessRegions in
cli/internal/doctor/checks_deploy.go: remove one blank line immediately before
each BEGIN marker and trim trailing newlines before comparison. Update the
deployed fixture in cli/internal/doctor/checks_instruction_drift_test.go lines
98-115 to include the leading newline before the agent-presence BEGIN marker,
reproducing the injector’s deployed bytes.
- Around line 397-429: Update the orphan detection in checkHarnessMirrorOrphans
so each harness subtree is considered only when its corresponding repoDir
exists; skip pruning and reporting for mirror entries when the repo lacks
harness/<sub>. Preserve normal orphan handling when the counterpart directory
exists, and add the requested regression case to TestCheckHarnessMirrorOrphans.
In `@cli/internal/doctor/checks_symlinks_test.go`:
- Around line 15-117: Refactor TestCheckDeployedSkillSymlinks into table-driven
subtests with one case per behavior branch, splitting the combined
managed/unmanaged command case into separate cases. Replace assertions on
rendered prose and exact failure counts with stable report status tags or
structured report results, while continuing to verify managed paths are flagged
and unmanaged paths are ignored.
In `@scripts/compile-harness.sh`:
- Around line 509-519: Track missing instruction sources as a deployment failure
in deploy_agents, and propagate a non-zero status consistent with its existing
missing-record-directory failure path instead of continuing silently. Update
do_deploy to detect that failure and return before printing the success message,
while preserving the existing handling for valid sources and unavailable
requirements.
In `@specs/HARNESS-070-deploy-convergence/proposal.md`:
- Around line 33-34: Add the AC3 failing bats regression test to
specs/HARNESS-070-deploy-convergence/proposal.md:33-34 scope before implementing
the shell --deploy behavior; in
specs/HARNESS-070-deploy-convergence/tasks.md:24-25, schedule that test before
deploy_instructions; update specs/HARNESS-070-deploy-convergence/tasks.md:32
only after the automated test passes so the checklist accurately reflects TDD
completion.
- Line 25: Update the AC3 contract and evidence for the copilot command gate: in
specs/HARNESS-070-deploy-convergence/proposal.md:25,
specs/HARNESS-070-deploy-convergence/features.json:18-21, and
specs/HARNESS-070-deploy-convergence/verification.md:12, state that deployment
skips the Copilot surface when copilot is unavailable, and record tests covering
both unavailable and available copilot paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d4a17f7-8768-4467-8bdf-df48fa1c8339
📒 Files selected for processing (14)
cli/internal/doctor/checks_deploy.gocli/internal/doctor/checks_harness_mirror_test.gocli/internal/doctor/checks_instruction_drift_test.gocli/internal/doctor/checks_symlinks_test.gocli/internal/doctor/checks_test.gocli/internal/doctor/checks_tools.gocli/internal/doctor/doctor.godocs/lessons.mdharness/manifest.jsonscripts/compile-harness.shspecs/HARNESS-070-deploy-convergence/features.jsonspecs/HARNESS-070-deploy-convergence/proposal.mdspecs/HARNESS-070-deploy-convergence/tasks.mdspecs/HARNESS-070-deploy-convergence/verification.md
Comment on lines
+358
to
+379
| func stripHarnessRegions(content string) string { | ||
| lines := strings.Split(content, "\n") | ||
| out := make([]string, 0, len(lines)) | ||
| skip, endMarker := false, "" | ||
| for _, l := range lines { | ||
| if skip { | ||
| if l == endMarker { | ||
| skip = false | ||
| } | ||
| continue | ||
| } | ||
| switch { | ||
| case strings.HasPrefix(l, harnessBeginPrefix): | ||
| skip, endMarker = true, harnessEndMarker | ||
| case strings.HasPrefix(l, agentPresenceBeginPrefix): | ||
| skip, endMarker = true, agentPresenceEndMarker | ||
| default: | ||
| out = append(out, l) | ||
| } | ||
| } | ||
| return strings.Join(out, "\n") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Region stripping and its test fixture both ignore the blank line the injector writes. inject_agent_presence in scripts/compile-harness.sh line 802 appends a region as "\n" + BEGIN + body + END + "\n". deploy_instructions copies the source verbatim first, so the append branch always runs and the deployed file always gains that blank separator. Neither the comparison nor the test models it, so checkInstructionDrift reports drift for all four targets after a clean --deploy while the test suite stays green.
cli/internal/doctor/checks_deploy.go#L358-L379: drop a single blank line that immediately precedes a BEGIN marker, and trim trailing newlines before comparing.cli/internal/doctor/checks_instruction_drift_test.go#L98-L115: build the deployed fixture with the leading"\n"before<!-- BEGIN HARNESS AGENT-PRESENCE, so the subtest reproduces the deployed bytes.
📍 Affects 2 files
cli/internal/doctor/checks_deploy.go#L358-L379(this comment)cli/internal/doctor/checks_instruction_drift_test.go#L98-L115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/internal/doctor/checks_deploy.go` around lines 358 - 379, Update
stripHarnessRegions in cli/internal/doctor/checks_deploy.go: remove one blank
line immediately before each BEGIN marker and trim trailing newlines before
comparison. Update the deployed fixture in
cli/internal/doctor/checks_instruction_drift_test.go lines 98-115 to include the
leading newline before the agent-presence BEGIN marker, reproducing the
injector’s deployed bytes.
Comment on lines
+397
to
+429
| repo := resolveRepoDir(sys) | ||
| if repo == "" || filepath.Clean(repo) == filepath.Clean(cfg.DotfilesDir) { | ||
| return // no checkout found, or the "mirror" IS the checkout — nothing to compare | ||
| } | ||
|
|
||
| orphans := 0 | ||
| for _, sub := range []string{"skills", "agents"} { | ||
| mirrorDir := filepath.Join(cfg.DotfilesDir, "harness", sub) | ||
| if !isDir(mirrorDir) { | ||
| continue | ||
| } | ||
| entries, err := os.ReadDir(mirrorDir) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| repoDir := filepath.Join(repo, "harness", sub) | ||
| for _, e := range entries { | ||
| if !e.IsDir() || isDir(filepath.Join(repoDir, e.Name())) { | ||
| continue | ||
| } | ||
| orphans++ | ||
| rel := filepath.Join("harness", sub, e.Name()) | ||
| target := filepath.Join(mirrorDir, e.Name()) | ||
| if !fix { | ||
| rep.Fail("orphan mirror record: " + rel + " (no repo counterpart — run: dotf doctor --fix)") | ||
| continue | ||
| } | ||
| if err := os.RemoveAll(target); err != nil { | ||
| rep.Fail("failed to prune orphan mirror record: " + rel + " (" + err.Error() + ")") | ||
| } else { | ||
| rep.Fix("pruned orphan mirror record: " + rel) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
--fix deletes every mirror record when the resolved repo lacks harness/<sub>.
resolveRepoDir falls back to the current working directory and walks up for a .git directory. docs/lessons.md line 2254 records this behavior. If a user runs dotf doctor --fix from an unrelated git checkout, repo resolves to that checkout. repoDir then does not exist, isDir(filepath.Join(repoDir, e.Name())) is false for every entry, and line 424 removes every record under <mirror>/harness/skills and <mirror>/harness/agents.
The records are regenerable by re-running setup, but the deployed harness breaks until then, and the operation is not reversible from the doctor output alone.
Require the repo counterpart directory to exist before you treat mirror entries as orphans.
🛡️ Proposed guard
repoDir := filepath.Join(repo, "harness", sub)
+ if !isDir(repoDir) {
+ // The resolved checkout has no counterpart tree at all (wrong repo,
+ // partial clone). Every entry would look orphaned — refuse to prune.
+ rep.Skip("repo has no " + filepath.Join("harness", sub) + " — orphan comparison skipped")
+ continue
+ }
for _, e := range entries {Add the matching table case to TestCheckHarnessMirrorOrphans:
t.Run("repo without harness/skills must not prune the mirror", func(t *testing.T) {
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
checkHarnessMirrorOrphans(sys, cfg, capture(&buf), true)
if !isDir(filepath.Join(mirror, "harness", "skills", "kept")) {
t.Error("must not prune when the repo has no counterpart tree")
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| repo := resolveRepoDir(sys) | |
| if repo == "" || filepath.Clean(repo) == filepath.Clean(cfg.DotfilesDir) { | |
| return // no checkout found, or the "mirror" IS the checkout — nothing to compare | |
| } | |
| orphans := 0 | |
| for _, sub := range []string{"skills", "agents"} { | |
| mirrorDir := filepath.Join(cfg.DotfilesDir, "harness", sub) | |
| if !isDir(mirrorDir) { | |
| continue | |
| } | |
| entries, err := os.ReadDir(mirrorDir) | |
| if err != nil { | |
| continue | |
| } | |
| repoDir := filepath.Join(repo, "harness", sub) | |
| for _, e := range entries { | |
| if !e.IsDir() || isDir(filepath.Join(repoDir, e.Name())) { | |
| continue | |
| } | |
| orphans++ | |
| rel := filepath.Join("harness", sub, e.Name()) | |
| target := filepath.Join(mirrorDir, e.Name()) | |
| if !fix { | |
| rep.Fail("orphan mirror record: " + rel + " (no repo counterpart — run: dotf doctor --fix)") | |
| continue | |
| } | |
| if err := os.RemoveAll(target); err != nil { | |
| rep.Fail("failed to prune orphan mirror record: " + rel + " (" + err.Error() + ")") | |
| } else { | |
| rep.Fix("pruned orphan mirror record: " + rel) | |
| } | |
| } | |
| repo := resolveRepoDir(sys) | |
| if repo == "" || filepath.Clean(repo) == filepath.Clean(cfg.DotfilesDir) { | |
| return // no checkout found, or the "mirror" IS the checkout — nothing to compare | |
| } | |
| orphans := 0 | |
| for _, sub := range []string{"skills", "agents"} { | |
| mirrorDir := filepath.Join(cfg.DotfilesDir, "harness", sub) | |
| if !isDir(mirrorDir) { | |
| continue | |
| } | |
| entries, err := os.ReadDir(mirrorDir) | |
| if err != nil { | |
| continue | |
| } | |
| repoDir := filepath.Join(repo, "harness", sub) | |
| if !isDir(repoDir) { | |
| // The resolved checkout has no counterpart tree at all (wrong repo, | |
| // partial clone). Every entry would look orphaned — refuse to prune. | |
| rep.Skip("repo has no " + filepath.Join("harness", sub) + " — orphan comparison skipped") | |
| continue | |
| } | |
| for _, e := range entries { | |
| if !e.IsDir() || isDir(filepath.Join(repoDir, e.Name())) { | |
| continue | |
| } | |
| orphans++ | |
| rel := filepath.Join("harness", sub, e.Name()) | |
| target := filepath.Join(mirrorDir, e.Name()) | |
| if !fix { | |
| rep.Fail("orphan mirror record: " + rel + " (no repo counterpart — run: dotf doctor --fix)") | |
| continue | |
| } | |
| if err := os.RemoveAll(target); err != nil { | |
| rep.Fail("failed to prune orphan mirror record: " + rel + " (" + err.Error() + ")") | |
| } else { | |
| rep.Fix("pruned orphan mirror record: " + rel) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/internal/doctor/checks_deploy.go` around lines 397 - 429, Update the
orphan detection in checkHarnessMirrorOrphans so each harness subtree is
considered only when its corresponding repoDir exists; skip pruning and
reporting for mirror entries when the repo lacks harness/<sub>. Preserve normal
orphan handling when the counterpart directory exists, and add the requested
regression case to TestCheckHarnessMirrorOrphans.
Comment on lines
+15
to
+117
| func TestCheckDeployedSkillSymlinks(t *testing.T) { | ||
| newEnv := func(t *testing.T) (home, mirror string, cfg *Config, sys *System) { | ||
| t.Helper() | ||
| home = t.TempDir() | ||
| mirror = t.TempDir() | ||
| mkdirAll(t, filepath.Join(mirror, "harness", "skills", "orca-cli")) | ||
| cfg = &Config{DotfilesDir: mirror} | ||
| sys = newSys(map[string]string{"HOME": home, "DOTFILES_REPO_DIR": mirror}, nil, nil) | ||
| return | ||
| } | ||
|
|
||
| t.Run("symlinked dir at an unmanaged name is silent", func(t *testing.T) { | ||
| home, _, cfg, sys := newEnv(t) | ||
| foreignTarget := t.TempDir() | ||
| mustSymlink(t, foreignTarget, filepath.Join(home, ".claude", "skills", "computer-use")) | ||
|
|
||
| var buf bytes.Buffer | ||
| rep := capture(&buf) | ||
| checkDeployedSkillSymlinks(sys, cfg, rep) | ||
|
|
||
| if rep.Failures() != 0 { | ||
| t.Fatalf("unmanaged-name symlink must not fail\n%s", buf.String()) | ||
| } | ||
| if !strings.Contains(buf.String(), "no symlinks at managed skill names") { | ||
| t.Errorf("expected the narrowed pass line\n%s", buf.String()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("symlinked dir at a managed name fails and names the path", func(t *testing.T) { | ||
| home, _, cfg, sys := newEnv(t) | ||
| foreignTarget := t.TempDir() | ||
| managedPath := filepath.Join(home, ".claude", "skills", "orca-cli") | ||
| mustSymlink(t, foreignTarget, managedPath) | ||
|
|
||
| var buf bytes.Buffer | ||
| rep := capture(&buf) | ||
| checkDeployedSkillSymlinks(sys, cfg, rep) | ||
|
|
||
| if rep.Failures() == 0 { | ||
| t.Fatalf("managed-name symlink must fail\n%s", buf.String()) | ||
| } | ||
| if !strings.Contains(buf.String(), managedPath) { | ||
| t.Errorf("expected the flagged path in output\n%s", buf.String()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("symlinked SKILL.md one level inside a managed dir fails", func(t *testing.T) { | ||
| home, _, cfg, sys := newEnv(t) | ||
| realDir := filepath.Join(home, ".claude", "skills", "orca-cli") | ||
| mkdirAll(t, realDir) | ||
| foreignFile := filepath.Join(t.TempDir(), "SKILL.md") | ||
| if err := os.WriteFile(foreignFile, []byte("x"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| mustSymlink(t, foreignFile, filepath.Join(realDir, "SKILL.md")) | ||
|
|
||
| var buf bytes.Buffer | ||
| rep := capture(&buf) | ||
| checkDeployedSkillSymlinks(sys, cfg, rep) | ||
|
|
||
| if rep.Failures() == 0 { | ||
| t.Fatalf("symlinked SKILL.md at a managed name must fail\n%s", buf.String()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("symlinked command file at an unmanaged name is silent, managed one fails", func(t *testing.T) { | ||
| home, _, cfg, sys := newEnv(t) | ||
| foreignFile := filepath.Join(t.TempDir(), "src.md") | ||
| mustSymlink(t, foreignFile, filepath.Join(home, ".config", "opencode", "commands", "find-skills.md")) | ||
| mustSymlink(t, foreignFile, filepath.Join(home, ".config", "opencode", "commands", "orca-cli.md")) | ||
|
|
||
| var buf bytes.Buffer | ||
| rep := capture(&buf) | ||
| checkDeployedSkillSymlinks(sys, cfg, rep) | ||
|
|
||
| // header line + the one flagged path (findSymlinks' header counts as a | ||
| // Fail too, same as the pre-existing behavior this test guards). | ||
| if rep.Failures() != 2 { | ||
| t.Fatalf("expected 2 failures (header + orca-cli.md only), got %d\n%s", rep.Failures(), buf.String()) | ||
| } | ||
| if strings.Contains(buf.String(), "find-skills.md") { | ||
| t.Errorf("unmanaged command symlink must not appear in output\n%s", buf.String()) | ||
| } | ||
| if !strings.Contains(buf.String(), "orca-cli.md") { | ||
| t.Errorf("expected managed command symlink in output\n%s", buf.String()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("no deployed skill paths -> skip", func(t *testing.T) { | ||
| home := t.TempDir() | ||
| mirror := t.TempDir() | ||
| cfg := &Config{DotfilesDir: mirror} | ||
| sys := newSys(map[string]string{"HOME": home}, nil, nil) | ||
|
|
||
| var buf bytes.Buffer | ||
| rep := capture(&buf) | ||
| checkDeployedSkillSymlinks(sys, cfg, rep) | ||
|
|
||
| if rep.Failures() != 0 || !strings.Contains(buf.String(), "no deployed skill paths found") { | ||
| t.Errorf("expected a skip\n%s", buf.String()) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use table cases and stable report status tags.
Lines 26-115 use separate closures and match rendered diagnostic prose. Line 80 also tests two branches in one case. Convert this suite to table-driven cases with one case per branch. Assert report status tags or structured results instead of strings such as "no symlinks at managed skill names".
As per coding guidelines: “Use table-driven tests with one test case per branch, and assert stable status tags rather than prose.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/internal/doctor/checks_symlinks_test.go` around lines 15 - 117, Refactor
TestCheckDeployedSkillSymlinks into table-driven subtests with one case per
behavior branch, splitting the combined managed/unmanaged command case into
separate cases. Replace assertions on rendered prose and exact failure counts
with stable report status tags or structured report results, while continuing to
verify managed paths are flagged and unmanaged paths are ignored.
Source: Coding guidelines
Comment on lines
+509
to
+519
| local agent file source requires dest | ||
| while IFS=$'\t' read -r agent file source requires; do | ||
| [[ -n "$source" ]] || continue | ||
| if [[ -n "$requires" ]] && ! command -v "$requires" >/dev/null 2>&1; then | ||
| printf '[deploy] instructions target %s skipped: %s not on PATH\n' "$agent" "$requires" | ||
| continue | ||
| fi | ||
| if [[ ! -f "$REPO_ROOT/$source" ]]; then | ||
| printf '[ERROR] instruction source missing: %s\n' "$REPO_ROOT/$source" >&2 | ||
| continue | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A missing instruction source is reported but never fails the deploy.
Line 517 writes [ERROR] instruction source missing to stderr, then continue. do_deploy still reaches line 496 and prints [deploy] OK, and the exit status stays 0. The output therefore contains an [ERROR] line followed by a success line — the same contradiction this PR removes at lines 818-825.
The doctor side does not cover the gap either: checkInstructionDrift skips a target when the source path does not exist (!pathExists(source) → continue in cli/internal/doctor/checks_deploy.go). A mistyped source in harness/manifest.json is then invisible in both surfaces after that single stderr line.
Track the failure and propagate it. deploy_agents already exits 2 for a missing record directory, so a non-zero exit is consistent with the file.
🛠️ Proposed fix to propagate the missing-source failure
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
printf '[deploy] instructions target %s skipped: %s not on PATH\n' "$agent" "$requires"
continue
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"
[[ -L "$dest" ]] && rm -f "$dest"
mkdir -p "$(dirname "$dest")"
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")
+ return "$rc"
}do_deploy then needs to stop instead of printing OK:
if jq -e '.agents.presence' "$MANIFEST" >/dev/null 2>&1; then
- deploy_instructions
+ deploy_instructions || exit 2
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local agent file source requires dest | |
| while IFS=$'\t' read -r agent file source requires; do | |
| [[ -n "$source" ]] || continue | |
| if [[ -n "$requires" ]] && ! command -v "$requires" >/dev/null 2>&1; then | |
| printf '[deploy] instructions target %s skipped: %s not on PATH\n' "$agent" "$requires" | |
| continue | |
| fi | |
| if [[ ! -f "$REPO_ROOT/$source" ]]; then | |
| printf '[ERROR] instruction source missing: %s\n' "$REPO_ROOT/$source" >&2 | |
| continue | |
| fi | |
| 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 | |
| printf '[deploy] instructions target %s skipped: %s not on PATH\n' "$agent" "$requires" | |
| continue | |
| fi | |
| if [[ ! -f "$REPO_ROOT/$source" ]]; then | |
| printf '[ERROR] instruction source missing: %s\n' "$REPO_ROOT/$source" >&2 | |
| rc=1 | |
| continue | |
| fi |
Suggested change
| local agent file source requires dest | |
| while IFS=$'\t' read -r agent file source requires; do | |
| [[ -n "$source" ]] || continue | |
| if [[ -n "$requires" ]] && ! command -v "$requires" >/dev/null 2>&1; then | |
| printf '[deploy] instructions target %s skipped: %s not on PATH\n' "$agent" "$requires" | |
| continue | |
| fi | |
| if [[ ! -f "$REPO_ROOT/$source" ]]; then | |
| printf '[ERROR] instruction source missing: %s\n' "$REPO_ROOT/$source" >&2 | |
| continue | |
| fi | |
| deploy_instructions || exit 2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/compile-harness.sh` around lines 509 - 519, Track missing instruction
sources as a deployment failure in deploy_agents, and propagate a non-zero
status consistent with its existing missing-record-directory failure path
instead of continuing silently. Update do_deploy to detect that failure and
return before printing the success message, while preserving the existing
handling for valid sources and unavailable requirements.
|
|
||
| 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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the AC3 contract and evidence for the copilot command gate.
When copilot is unavailable, the deployment skips the Copilot surface. State this condition in proposal.md, features.json, and verification.md. Record tests for both unavailable and available copilot paths.
📍 Affects 3 files
specs/HARNESS-070-deploy-convergence/proposal.md#L25-L25(this comment)specs/HARNESS-070-deploy-convergence/features.json#L18-L21specs/HARNESS-070-deploy-convergence/verification.md#L12-L12
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/HARNESS-070-deploy-convergence/proposal.md` at line 25, Update the AC3
contract and evidence for the copilot command gate: in
specs/HARNESS-070-deploy-convergence/proposal.md:25,
specs/HARNESS-070-deploy-convergence/features.json:18-21, and
specs/HARNESS-070-deploy-convergence/verification.md:12, state that deployment
skips the Copilot surface when copilot is unavailable, and record tests covering
both unavailable and available copilot paths.
Comment on lines
+33
to
+34
| - `DOTFILES_AUTODEPLOY` disposition (#869 remedy (a)) — a per-machine judgment call for the user, not a code change. Surfaced as an open decision in the PR body. | ||
| - bats coverage for the shell-side `--deploy` change (`tests/*.bats` is out of scope for this session per explicit multi-session coordination) — the Go-side doctor checks get Go tests; the shell change is verified manually and by the existing `--check` gate, with a bats follow-up proposed, not filed, in the PR body. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add the AC3 regression test before closing the spec.
The proposal defers bats coverage, the task plan records manual-only verification, and the closing checklist claims every acceptance criterion has a test. These statements conflict with the repository TDD requirement.
specs/HARNESS-070-deploy-convergence/proposal.md#L33-L34: add the failing bats test to the scope before implementing the shell behavior.specs/HARNESS-070-deploy-convergence/tasks.md#L24-L25: place the failing test before thedeploy_instructionsimplementation.specs/HARNESS-070-deploy-convergence/tasks.md#L32-L32: update the checklist only after the automated test passes.
As per coding guidelines, the repository requires “TDD — failing test first, then the fix.”
📍 Affects 2 files
specs/HARNESS-070-deploy-convergence/proposal.md#L33-L34(this comment)specs/HARNESS-070-deploy-convergence/tasks.md#L24-L25specs/HARNESS-070-deploy-convergence/tasks.md#L32-L32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/HARNESS-070-deploy-convergence/proposal.md` around lines 33 - 34, Add
the AC3 failing bats regression test to
specs/HARNESS-070-deploy-convergence/proposal.md:33-34 scope before implementing
the shell --deploy behavior; in
specs/HARNESS-070-deploy-convergence/tasks.md:24-25, schedule that test before
deploy_instructions; update specs/HARNESS-070-deploy-convergence/tasks.md:32
only after the automated test passes so the checklist accurately reflects TDD
completion.
Source: Coding guidelines
This was referenced Aug 13, 2026
Owner
Author
|
This PR was squash-merged (18ccd60) before the CodeRabbit review-fix commit on this branch landed — that commit was pushed ~21h after this PR closed, to an already-merged branch, so it never got CI and never merged. It carried 3 confirmed Major findings from CodeRabbit's review here (blank-line false-positive in checkInstructionDrift, a data-loss guard for checkHarnessMirrorOrphans --fix, and an exit-code propagation fix in deploy_instructions). Landed via follow-up: #954 |
mlorentedev
added a commit
that referenced
this pull request
Aug 14, 2026
…rge (#954) * 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/<sub> 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. * 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three related gaps in harness deploy convergence, closed together as one thread (spec:
specs/HARNESS-070-deploy-convergence/):setup-linux.shis copy-only, so aharness/{skills,agents}record deleted from the repo survived forever in the~/.dotfilesmirror and kept failingcheckCompileHarnessDrift, which runs FROM the mirror.dotf doctornow detects the orphan anddotf doctor --fixprunes it, per BUG-051: the sensitive/ deploy never prunes - a deleted secret stays decryptable on every machine, forever #802's already-decided semantic (doctor--fixprunes, setup only copies/warns).dotfbinary behind theversions.confpin now FAILs doctor instead of WARNing. A staledotfsilently carries none of the guards merged since it was built — a harder failure than an ordinary tool drifting one version.agy/codexthroughcompile-harness.sh --deploybut reachedclaude/opencode/pi/copilotonly through a fullsetup-linux.shrun.--deploynow copies the four instruction files itself (ordered first, before the skill-catalog/AGENT-PRESENCE injections that land on top of them), anddotf doctorreports a deployed instruction file that has drifted from its repo source (region-stripped compare, so the injected regions don't false-fail it).Bonus, found while investigating this session's handoff evidence: the "4 symlinked skills" reported as a BUG-100 regression (
computer-use,find-skills,orca-cli,orchestration) turned out to be a false positive — all four resolve to Orca's own~/.agents/skillsmechanism, unrelated to this repo'sharness/skills/records, with documented prior art for the exact exclusion (specs/archive/AI-022-pi-harness-slot/). Filed and closed as #943 (BUG-074) in the same change.Also fixed a pre-existing bug found while sandbox-testing AC3:
deploy_agent_presencealways logged[deploy] presence -> ...as success even when the injection was skipped because the target file didn't exist, producing a directly contradicting pair of log lines.Not in this PR (stated, not silently dropped)
DOTFILES_AUTODEPLOYdisposition (opt-in vs. on-by-default) is a per-machine judgment call for @mlorentedev, not a code change. Remedy (b), the durable guard, is what this PR ships.scripts/,sensitive/,.zsh/,ssh/,secrets/(36+ orphan files measured on a live machine). Referenced, not closed — out of this session's assigned surface (scripts/compile-harness.sh,harness/, the doctor package).--deploychange —tests/*.batswas out of scope this session (three parallel sessions touching the repo). Verified instead by:shellcheck/bash -n/zsh -nclean, all 44 existingtests/compile-harness.bats+ 64tests/setup-linux.batscases still pass unmodified, and a manual sandboxedHOME=/tmp/fake-home --deployrun (idempotent, region-injection-safe). Proposing a follow-up ticket to add the bats regression — not filing it unilaterally.agents.presence[].sourcefield is consumed only bycompile-harness.sh(Linux).setup-windows.ps1is unaffected (per AC3) and never had this specific gap: it sets$DotfilesDir = $PSScriptRoot, i.e. the deploy dir IS the checkout, so there's no lightweight-deploy-vs-full-setup split to begin with on that side.Current machine state (informational, not actioned by this PR)
Running
dotf doctoron this machine right now still shows the drift this PR's checks were built to catch — expected, since fixing the detection doesn't retroactively converge the machine:dotfitself is one release behind (installed=0.37.0 pinned=0.38.0) — now a FAIL, was a WARN.~/.dotfilesmirror (9 PRs + a release cut since the last full setup run)..claude/CLAUDE.md, opencode/piAGENTS.md).Remediation, in order, left for @mlorentedev to run when convenient: merge this PR → next release cuts →
./scripts/install-dotf.sh→setup-linux.sh(orcompile-harness.sh --deployfor just the harness surface) →dotf doctorshould read clean. Worth noting for the #869 story: the new guards in this PR are themselves live only after that next release installs — which is the OPS-025 window working exactly as described.Test plan
cd cli && go build ./... && go vet ./... && go test ./...— 13 packages, all greengolangci-lint run ./...(pinned v2.12.2, matchesversions.conf) — 0 issuesshellcheck scripts/compile-harness.sh,bash -n,zsh -n— cleanjq empty harness/manifest.json— validbats tests/compile-harness.bats— 44/44 pass (unmodified)bats tests/setup-linux.bats— 64/64 pass (unmodified)HOME=/tmp/fake-home bash scripts/compile-harness.sh --deploy— all 4 instruction files written, second run byte-for-byte idempotent,--checkstays greenspecs/HARNESS-070-deploy-convergence/verification.md)Closes #843. Closes #869. Closes #828. Closes #943.
Refs #802
Archive skip rationale
The spec (
specs/HARNESS-070-deploy-convergence/) is left active rather than archived in this PR. This repo's/spec archivestep requires an independentadversarial-reviewpass (review.mdwith aPASS/PASS-WITH-GAPSverdict, ideally from a different session/agent than the implementer) before archiving — the whole point being that the reviewer is not the same eyes that just wrote the code. That review has not run yet. Rather than self-certify withreview: waivedor--force-without-reviewon a change I just wrote myself, the spec stays inspecs/until an independent/adversarial-reviewpass runs, then archives in a small follow-up PR. All acceptance criteria are met and verified (seeverification.md); this is a process-sequencing gap, not unfinished work.Summary by CodeRabbit
New Features
--fixcan remove orphaned harness records and converge deployments safely.Bug Fixes
Documentation