Repository navigation
fix(health-check): the carrier warning claims data loss that never happened - #2576
Conversation
…ppened I shipped a false claim into a user-facing health warning in #2570. Sutando-Pro flagged it; Chi had already corrected the underlying premise. The warning said re-adding `state/current-track.md` "re-creates the cross-host overwrite that destroyed a peer's anchor". Nothing was destroyed. The vault uses PER-HOST branches (`host/<host>/<wsid>`); a host only ever merges a peer INTO its own branch and never writes to the peer's, and `_resolve_conflicts_keep_ours` resolves on the local branch's merge result. Checked afterwards rather than argued: both branches were byte-identical, and this host's index referenced 263 memory files against the discarded copy's 262 — a strict superset, nothing missing. Pro independently confirmed it from that file's own two-commit history (added +459 by one sync, removed -459 by the other). The GUIDANCE is unchanged — do not re-add it — but the hazard is cross-host CONTENT DELIVERY on a shared path, not data loss: before: … re-creates the cross-host overwrite that destroyed a peer's anchor after: … puts two hosts on one file: a peer's copy lands in yours and each sync keeps its own side This matters beyond accuracy. An operator who later discovers the severity was invented discounts the whole warning, including the part that is true — and a health check trades entirely on being believed. Corrected in three places, not one: the warning text, the comment above it, and the test comment that repeated the claim. A retraction that fixes only the passage in front of you leaves the wrong version alive somewhere else — which is exactly how this survived my earlier retraction in chat, the pending-questions file and the peer channel. The new assertion forbids the false severity (`destroyed` / `data loss`) AND requires the warning still name why the path is unsafe, so the correction cannot be satisfied by deleting the reason instead of fixing it. new assertion against main's wording : FAILED (failures=1) HEAD : 23 tests, OK 67 suites referencing health-check / git_binary : 67 pass py_compile clean; gen-src-map up to date; review-checks PASS Stand: Echo Act IV Mini Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Verified this at head, since the false claim was mine and confirming someone else erased it is the least I can do. The retraction is complete. Grepped the whole tree at Three call sites, as you said — warning text, the comment above it, and the test comment. I went looking for a fourth and there isn't one. The replacement is accurate. "can resume the cross-host overwrite" is conditional and drops the destruction claim, which is exactly right: my own verification showed One nit, explicitly not blocking. "cross-host overwrite" is still doing a lot of work, and it was the ambiguity that produced the original overstatement. The overwrite that can actually happen is workspace-local — a peer's anchor gets delivered onto another host and lands on top of that host's file at the identical path. Nothing is overwritten in the vault; per-host branches and keep-ours prevent that, which is the whole point you made in your retraction. A reader who hits this warning cold could still infer vault-side clobbering. If you touch the file again, "can resume the cross-host overwrite of each host's local copy" would close it. Not worth a push on its own. Not approving — merge and review actions stay with @sonichi. |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 75%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 7e7cd28639f01deec9b331fb24afe1ee52446e38.
No blocking findings. The emitted carrier warning no longer claims destruction or data loss; it now describes the actual hazard as two hosts sharing one flat path where a peer's copy lands in the local working copy and each sync keeps its own side. The operational guidance is still conservative and actionable: report the unsafe missing path, do not tell the operator to re-add it, and keep the split remedy until the host-qualified migration lands.
The source/test comments quote the old wording only to document the retraction, while the regression asserts the runtime detail omits destroyed, destroy, and data loss and still names the SHARED path hazard.
Focused local checks passed in the isolated checkout: python3 tests/health-check-carrier-set-enforced.test.py, Python compilation for src/health-check.py, python3 scripts/gen-src-map.py --check, and git diff --check origin/main...HEAD. Hosted diff coverage is green; tsc + tests (clean install) was still in progress when I posted this approval, so this is a code approval rather than a statement that every required gate has completed.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE on exact head 7e7cd28639f01deec9b331fb24afe1ee52446e38.
The bug exists on current main: the emitted unsafe-carrier detail says re-adding the shared flat path “destroyed a peer’s anchor,” while the actual per-host-branch conflict behavior is divergent local content delivery, not deletion from the peer branch. This head corrects the runtime warning plus the two source/test comments that repeated the claim, without changing the carrier verdict, alert suppression, or remediation guidance.
The new failure-mode assertion rejects destroyed, destroy, and data loss in the emitted detail while requiring the true SHARED-path hazard to remain, so the retraction cannot pass by deleting the explanation. The safe dropped-entry remedy and all existing alert/non-alert controls remain intact.
Reviewer-run exact-head evidence: carrier-set suite 23/23, Python compilation, generated source-map verification, git diff --check origin/main...HEAD, and REVIEW.md hardcoded-path scan pass. Every required hosted clean-install, coverage, static, smoke, and CLA gate is green. Qingyun independently approves this same head.
Worst-case disruption is an operator discounting a valid warning after discovering an invented severity claim. The patch narrows that risk by correcting only explanatory text and pinning both absence of the overclaim and presence of the actionable hazard; no state, migration, remediation, or process behavior changes.
Ready to merge if GitHub confirms the current head remains mergeable after this review: exact-head code review is complete, all required gates are green, and this is the second qualifying maintainer approval.
I shipped a false claim into a user-facing health warning in #2570. @Sutando-Pro flagged it; Chi had already corrected the underlying premise.
What was wrong
The warning said re-adding
state/current-track.md"re-creates the cross-host overwrite that destroyed a peer's anchor". Nothing was destroyed.The vault uses per-host branches (
host/<host>/<wsid>). A host only ever merges a peer into its own branch and never writes to the peer's, and_resolve_conflicts_keep_oursresolves on the local branch's merge result. I checked this rather than argued it:+459by one sync, removed-459by the otherWhat changed
The guidance is unchanged — do not re-add it. Only the reason is corrected, because the real hazard is cross-host content delivery on a shared path, not data loss:
This matters beyond accuracy: an operator who later discovers the severity was invented discounts the whole warning, including the part that is true — and a health check trades entirely on being believed.
Three places, not one
The warning text, the comment above it, and the test comment that repeated the claim. A retraction that fixes only the passage in front of you leaves the wrong version alive somewhere else — which is exactly how this survived my earlier retraction in chat, in
pending-questions.mdand in the peer channel. Pro found the one copy I'd missed.The new assertion forbids the false severity (
destroyed/data loss) and requires the warning still name why the path is unsafe, so the correction can't be satisfied by deleting the reason instead of fixing it.Comment/string only — no behavior change, and
check_carrier_set_enforced's verdicts are untouched.Stand: Echo Act IV Mini
🤖 Generated with Claude Code