fix(review): land CodeRabbit findings from PR #948 that missed the merge - #954
Conversation
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.
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.
|
Warning Review limit reached
Next review available in: 96 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
1 participant
Summary
PR #948 (HARNESS-070: deploy convergence) was squash-merged before its
CodeRabbit review-fix commit landed on the branch — the fix commit was
pushed to an already-closed PR, so it never got CI and never merged.
This PR carries that commit forward onto current
main(cleancherry-pick, zero conflicts with #949's changes, which also touched
cli/internal/doctor/checks_deploy.go).Three confirmed CodeRabbit findings, all Major:
stripHarnessRegionsdidn't drop the blank separator line theinjector writes before a region, so
checkInstructionDriftcurrently false-FAILs on a genuinely clean
--deploy— the check'sactual, stated purpose was broken on
main.checkHarnessMirrorOrphans --fixhad no guard against deleting anentire
harness/<sub>mirror tree when the resolved repo checkoutlacks that subtree (e.g.
DOTFILES_REPO_DIRunset, run from anunrelated repo) — every mirror entry would read as orphaned. Added
an
isDir(repoDir)skip guard.deploy_instructions's missing-source-file case printed[ERROR]but still let
do_deployprint[deploy] OKand exit 0. Nowpropagates a non-zero exit, matching
deploy_agents' existingconvention.
Plus a
docs/lessons.mdentry on the misdiagnosis that delayed this:a PR's
head.shanot matching a fresh push looks like APIpropagation lag, but checking
state/mergedon the same queryimmediately shows a closed PR instead — a frozen
head.shabydefinition, not lag. A poll loop built around
head.sha == Xon amerged PR spins forever.
Full detail on the underlying fixes is in
specs/HARNESS-070-deploy-convergence/verification.md(already updated in the merged #948) — this PR is scoped to landing
what #948 was supposed to include.
Refs #948 #843 #869 #828
Test plan
cd cli && go build ./... && go vet ./... && go test ./...— all packages passgolangci-lint run ./...(pinned v2.12.2) — 0 issuesshellcheck scripts/compile-harness.sh— clean;bash -n/zsh -n— cleanbats tests/compile-harness.bats— 44/44 passbats tests/setup-linux.bats— 64/64 passmain(including fix(tests): repair four guards that pass without checking what they claim #949) with zero conflicts