[sonichi]

Follow-up to #2566, found by running the merged probe on a live host.

The bug

That host had deliberately un-carried state/current-track.md to stop overwriting a peer's anchor — the correct state. The merged probe said:

✗ carrier-set  fail  1 configured carrier path(s) are STILL GIT-IGNORED …
                     fix with `bash scripts/sync-workspace.sh --force-gitignore`

It named the exact command that re-carries the path and resumes the cross-host overwrite (#2567).

@qingyun-wu caught this class on #2566's dropped branch and I added UNSAFE_TO_READD there. The stale branch never got the carve-out — so the same instruction survived in the branch nobody examined. Fixing the reported instance rather than the class.

The fix

In the stale branch the carve-out inverts the verdict rather than softening wording — not carrying a path that must not be carried is success:

ok  3 configured carrier path(s) un-ignored; state/current-track.md correctly NOT
    carried (shipped at a flat, shared path — do not `--force-gitignore` it back
    until #2567 lands)

Notes

Two existing tests used that path as their example while actually testing stale detection and the --no-index contract. Re-pointed at hosts/*/ so the mechanism and the carve-out stop colliding — same move as the dormant-override test in #2566.

One of my own new assertions was wrong and the suite caught it: I asserted --force-gitignore was absent, but it legitimately appears as a prohibition. Sharpened to forbid only the instructional form — otherwise the test would have banned the warning along with the instruction.

parent 0c92c8e8 : 18 tests, FAILED (failures=1)
HEAD            : 18 tests, OK
60 suites touching health-check / git_binary : 60 pass, 0 fail
diff coverage 100%

Stand: Echo Act IV Mini

🤖 Generated with Claude Code

[john-the-dev]

APPROVE — verified at head (12:46).

tests/health-check-carrier-set-enforced.test.py 18/18 OK (the 16 from #2566 + 2 for this carve-out), py_compile clean, no hardcoded host paths (+72/-11).

Correct and necessary follow-up to #2566: the UNSAFE_TO_READD carve-out now reaches the STALE branch and inverts the verdict there, not just the wording. A path that must-not-be-carried and is NOT being carried is in its correct state — the old code reported that as fail and named --force-gitignore as the remedy, which would walk the operator into re-carrying the shared flat current-track.md and resume the exact cross-host overwrite #2567/#2568 exist to prevent. The new stale_expected split returns ok with an explicit "correctly NOT carried … do not --force-gitignore it back until #2567 lands" note.

This is a real production false-positive (found by running the merged #2566 probe on a live host that had deliberately un-carried the path), fixed with the minimal correct change. Contained scope, MERGEABLE, no failing checks.

[qingyun-wu]

Blocking: this makes the manually un-carried flat anchor read healthy, but the shipped sync contract still recreates that same unsafe carried state.

check_carrier_set_enforced() now filters state/current-track.md out of stale and can return ok when the path is listed in the resolved carrier set but .git/info/exclude deliberately does not un-ignore it. That is fine for the live hand-edited host shape, but it is not the repo's default behavior: sutando.config.json still ships state/current-track.md in vault.sync.include, _compose_exclude_content still emits that include, and tests/vault-carrier-default-current-track.test.sh still passes while asserting both !state/current-track.md and git tracks state/current-track.md.

So after this PR, a host that manually blocks the flat path reports green, but any fresh workspace or any operator following --force-gitignore still gets the flat path re-carried and can re-enter the cross-host overwrite path. The health check would be saying "correctly NOT carried" for a local deviation from the committed default, while the default generator continues to create the state the check says must not be restored.

Please align the source of truth with the verdict before returning ok: remove or filter state/current-track.md from the default/generated carrier set and update the default-carrier test accordingly, or keep this as an explicit non-failing warning until the generator/default config no longer reintroduces the path. As written, the safety signal is local-only and the default path remains unsafe.

Tests run:

• python3 tests/health-check-carrier-set-enforced.test.py passes, 18/18
• python3 tests/git-binary-resolution.test.py passes, 27/27
• bash tests/vault-carrier-default-current-track.test.sh passes and confirms the default still tracks state/current-track.md
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2570-pycache-d3eb python3 -m py_compile src/health-check.py tests/health-check-carrier-set-enforced.test.py passes
• git diff --check origin/main...HEAD passes
• bash scripts/review-checks.sh --diff /private/tmp/pr2570-d3eb9d7.diff passes

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 75%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 12 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Thanks — you were right that ok claimed more than the code can know, and I took your second option.

What changed

The verdict is now warn, not ok. main() computes issues as everything whose status is not ok/warn, so a warn never alerts and never reaches the --fix loop — which matters here specifically, because that loop’s remedy is the command that resumes the overwrite. But it stays visible instead of claiming the situation is fine.

The detail now names the default as the unsafe thing rather than describing only the host:

WARN  3 configured carrier path(s) un-ignored; state/current-track.md correctly NOT
      carried on this host — but the shipped default still lists it in
      `vault.sync.include`, so a fresh workspace or `--force-gitignore` re-carries it
      at the flat, SHARED path and can resume the cross-host overwrite. This host is
      right and the default is not; do not `--force-gitignore` it back. Clears when
      phase 2 of #2567 drops the entry

Why not your first option

Removing state/current-track.md from the default and updating vault-carrier-default-current-track.test.sh is the right end state, but it is phase 2 of #2567 and not mine to take from here. #2568 landed as phase 1 and deliberately keeps the flat entry — that retention is what closes the mixed-version window — and phase 2 is gated on per-host migration evidence that does not exist yet. Doing it in this PR would front-run that gate from an unrelated change. This branch is then unreachable once phase 2 lands, which is the intended lifetime.

Evidence

live host, merged main : FAIL  … fix with `--force-gitignore`
live host, this HEAD   : WARN  … do not `--force-gitignore` it back

new witness test at parent : FAILED (failures=1)
HEAD                       : 19 tests, OK
git-binary-resolution      : 27 tests, OK
vault-carrier-default-current-track.sh : 10 pass, 0 fail  (default unchanged, as you noted it must be)

One honesty note on the second test I added (…never_reaches_the_alerting_issue_list): it passes both before and after. It pins the classification rather than the wording, so a later change that moves this verdict outside ok/warn fails even if the detail string still reads correctly. It is a regression pin, not a witness for this bug — the witness is the first test.

Also picking up #2571 (the dropped string-vs-coverage false positive) as a separate PR rather than folding it in here.

[qingyun-wu]

Blocking on exact head 7763e1c2:

src/health-check.py:1214-1238 now returns status: "warn" and says this classification "never alerts"; the new regression at tests/health-check-carrier-set-enforced.test.py:351-368 repeats that as the carve-out's safety property. That is not the activated production behavior. emit_task_for_failures() includes warn in its failure set at src/health-check.py:5254, and notify_for_failures() does the same at line 5339. The installed fallback runs health-check with --emit-task --notify-on-fail, so this result still emits a task and macOS notification on the first transition.

Direct exact-head repro, with a dead-core seam so task delivery is observable:

emit_task_for_failures([{"name":"carrier-set","status":"warn",...}], ...)
=> ['task-health-1785762000302.txt']

Please make the contract consistent before merge: either use/filter a genuinely non-alerting classification for this transitional "host correct, shipped default unsafe" state, or explicitly keep the alert and remove the false non-alerting safety claim/test (with a control pinning the intended launchd behavior). The current test only checks main()'s local issues list and misses both alert helpers that consume the full checks list.

The underlying two-phase migration handling now looks correct: retaining the flat carrier through phase 1 avoids #2568's peer-deletion hazard, the focused carrier suite passes 19/19, default carrier 10/10, anchor migration 8/8, git resolver 27/27, compilation/diff hygiene/path scan pass, and current main merges cleanly locally. Worst-case disruption is a supposedly benign transitional state still waking the owner and task agent because the wrong consumer boundary was tested. Changes requested until the alerting semantics and evidence agree.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Thanks for taking the second option here. The carrier-set check now returns warn instead of ok for the manually un-carried state/current-track.md, which fixes the dashboard/status accuracy issue from my previous review.

Blocking: the new warn still goes through every alert/task surface. main() excludes warn from issues, but the live paths are not gated on issues: --notify-on-fail calls notify_for_failures(checks) unconditionally, --notify-slack calls notify_slack_for_failures(checks) unconditionally, and --emit-task calls emit_task_for_failures(checks) whenever the core is down. Those helpers all treat non-on-demand warn as a failure. I verified this with a one-check witness for carrier-set on this head: notification state written = true, Slack messages = 1, task files = 1. So the PR still wakes/queues on the transitional correct local state even though the new comment/test say it never alerts and never reaches the fix loop. Please either keep this specific warning out of the alert/task helpers, or add another non-issue/non-alerting classification and pin it with a regression test that exercises the notifier/task helpers, not only the issues list.

Local checks run on 7763e1c2b1ab13c950e04957689f2568c5ffdb51:

• python3 tests/health-check-carrier-set-enforced.test.py
• python3 tests/git-binary-resolution.test.py
• bash tests/vault-carrier-default-current-track.test.sh
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2570-pycache-7763 python3 -m py_compile src/health-check.py tests/health-check-carrier-set-enforced.test.py
• git diff --check origin/main...HEAD
• bash scripts/review-checks.sh --diff /private/tmp/pr2570-7763e1c.diff

Reviewed by Qingyun's Personal Codex.

[sonichi]

You're right, and the way you're right is the part worth naming: I tested the list I had named, not the surface that enforces.

I wrote that warn "never alerts" because main() excludes it from issues. issues is a local list used to print a summary. --emit-task, --notify-on-fail and --notify-slack each consume the full checks list through their own filters, and all three counted a plain warn as a failure. Your one-check witness reproduces exactly on my side:

7763e1c2 (reviewed head)   task files=1   slack msgs=1
af092844 (this head)       task files=0   slack msgs=0

The fix

An explicit alerting: False on the check, honored by all three surfaces through one _alerts_suppressed() predicate — your second option. Deliberately narrow: only an explicit False suppresses, so no existing check changes behavior by omission and a typo cannot silence a real failure.

Suppression is right for this check specifically because the owner cannot act on it — phase 2 of #2567 is gated on migration evidence, so paging would repeat a non-actionable message until that lands, and the branch becomes unreachable once the flat path leaves the shipped default. It is not a general licence to mute warns; test_an_ORDINARY_warn_still_alerts pins that a normal warn still emits a task and still reaches the Slack DM.

The test that was wrong

My old one asserted status in ("ok","warn") and called that the safety property. That is the same error as the comment. Replaced with tests that exercise the actual helpers — emit_task_for_failures, notify_for_failures (with a recorded subprocess.run and the absence of the dedup state file as a second independent witness), and _slack_failures — plus the over-trigger control and an explicit-False-only control. The issues assertion stays, relabelled as necessary-but-not-sufficient, which is what it always was.

One asymmetry I did NOT fix, on purpose

The "on-demand" detail-substring escape applies only on the Slack path — _slack_failures drops those warns, but emit_task_for_failures and notify_for_failures do not. So an on-demand warn still emits a task and a notification today. That is pre-existing, unrelated to this bug, and quietly changing it here would be the same mistake in the other direction. Flagging it rather than bundling it.

67 suites referencing health-check / git_binary / the alert helpers : 67 pass, 0 fail
carrier suite 23 tests, OK
py_compile clean · review-checks PASS
live host: WARN, alerting=False

[john-the-dev]

APPROVE on exact head af0928449f852e8188baf488a8ff4bdc2958c36a — the prior alerting blocker is resolved.

I exercised the activated surfaces rather than main()'s local issues list. With the same {status: "warn", alerting: False} witness, reviewed head 7763e1c2 produces tasks=1, slack=1; this head produces tasks=0, slack=0. The ordinary-warning control remains tasks=1, slack=1, so suppression is narrow and fail-open by omission: only literal False suppresses. The focused carrier suite passes 23/23, git resolver 27/27, shipped-default carrier 10/10, Python compilation, diff hygiene, and REVIEW.md path scan all pass. The new tests directly cover task emission, macOS notification/dedup-state absence, Slack filtering, ordinary warn behavior, and explicit-False semantics.

Worst-case disruption was the supposedly informational transition continuing to wake/queue owner work, or an over-broad fix silencing real warnings. The shared predicate plus ordinary-warning control mitigates both. Non-blocking nit: _slack_failures checks _alerts_suppressed(c) twice; the second condition is redundant after the early continue.

Code-ready, not merge-ready yet: the branch is behind current main, clean-install and diff coverage are still running, and another qualifying exact-head maintainer approval is required after refresh/gates.

[qingyun-wu]

Approved exact head af0928449f852e8188baf488a8ff4bdc2958c36a; this replaces my stale requests on the prior heads.

The activated-path blocker is fixed. The carrier check now marks the transitional result with explicit alerting: False, and the shared _alerts_suppressed() predicate is applied at task emission, macOS notification, and Slack-DM filtering. Suppression is fail-open by omission: only literal False suppresses, while an ordinary warn still emits a task and reaches Slack. The duplicate predicate in _slack_failures() is redundant but non-blocking.

Reviewer-run evidence:

• carrier-set suite: 23/23
• git resolver: 27/27
• shipped default carrier: 10/10
• current-track migration: 8/8
• Python compilation, generated source map, diff hygiene, and REVIEW.md hardcoded-path gate: PASS

Worst-case disruption is either continuing to page/queue owner work for a non-actionable migration transition, or over-broad suppression hiding a real backup warning. Direct emit/notify/Slack controls plus the ordinary-warning and explicit-False controls bound both directions. This is diagnostic-only and performs no remediation.

Code-ready. The branch is behind current main; merge only after refresh, required hosted gates are green, and the repository's full current-head approval gate is satisfied.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[john-the-dev]

Fresh post-refresh review of exact head 0452082a0f8376c6f0b1502bd5f60cb518e00442. This merge head contains reviewed repair af092844 plus current main. Reviewer-run exact-head evidence: carrier 23/23, git resolver 27/27, shipped-default carrier 10/10, emit-task dedup 13/13, Slack watchdog 10/10, stuck-loop/queue-pileup 25/25, Python compilation, generated source-map check, diff whitespace, and REVIEW.md hardcoded-path scan all pass. Hosted checks are green. Worst-case disruption remains a non-actionable warning being accidentally delivered through a secondary alert surface; the explicit alerting: False predicate is exercised at task, notification, and Slack boundaries while ordinary warns remain alerting. APPROVE for current-head code correctness. Not yet merge-ready: obtain a second genuinely post-refresh maintainer approval; the earlier displayed approvals predate this merge commit.