Repository navigation
fix(health-check): an unrunnable sutando-app probe emitted nothing, not a row - #2855
Conversation
…ot a row The probe is guarded on `dev_bin.exists() or app_bin.exists()`. With neither path present the block was skipped and no `sutando-app` check reached the report — so the summary printed "No failures" and the absence of the probe was indistinguishable from the probe passing. Adds the missing else: one warn row naming both paths it looked for, stating the state is unknown rather than ok. A host that genuinely has no menu-bar app now carries a standing warn; that is the intended trade, because the previous behaviour was silence that read as coverage. Test is structural (AST over the guard) rather than behavioural: exercising the branch means calling run_all_checks(), which runs ~100 probes with subprocesses and network. Validated against the parent — all 4 assertions FAIL at origin/main and pass here, so it is not a test that only ever prints OK. Stand: Echo Act IV Pro
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 81%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 033a7b5612a4b7bcfdfdcafd1de440c6b00c8839.
Changes requested — not merge-ready.
- [P1]
src/health-check.py:7536-7542unconditionally emits awarnwhenever neither menu-bar binary exists. The open-source core is deliberately headless (tests/startup-headless.test.sh), so every supported Linux/cloud install and every operator who intentionally does not install the optional macOS app now carries a permanent health warning. I reproduced the exact-head behavior in a clean detached worktree:python3 src/health-check.py --jsonreturnedsutando-app: warn — not checked — no binary at either path. Gate this on a platform/install/config signal and represent the no-app case as explicitly not applicable rather than unhealthy; add a control proving a headless/no-app host stays healthy while a configured/installed-but-unprobeable app remains visible. - [P2]
tests/health-check-sutando-app-absent.test.py:2-14is a long incident/test-strategy docstring, exceeding the repository's two-line, constraint-only comment/docstring rule. Condense it and keep the history in the PR body.
The focused absent/electron/hotkey/compiled-artifact suites, Python compilation, diff hygiene, and the REVIEW.md gate pass. Those checks currently encode the false-warning policy rather than protecting supported headless behavior.
Worst-case disruption is every headless core permanently reporting degraded health for an optional capability it cannot and should not run.
Addresses both review findings on the previous head. P1: the unconditional warn made every headless Linux/cloud core — and every operator who deliberately skips the optional macOS app — permanently degraded for a capability they cannot and should not run. Gates on whether the host ASKED for the app, via two signals covering both install paths: the launchd plist, and a recent contextual-chips.json (the app is its sole writer, so it proves the app ran here without launchd — the configuration this repo's own machine uses). Recency ages the marker out so a deliberate uninstall stops warning rather than nagging forever. not-applicable now emits no row, matching check_gateway_bridge()'s existing "unconfigured returns None" convention. The original defect is unchanged for the case that mattered: a host that asked for the app and lacks the binary still gets a visible warn instead of silence. P2: docstring condensed to the two-line constraint-only rule; the incident history moved to the PR body. Verified on all three real configurations rather than only in tests: binary present -> installed (probe runs, unchanged) binary gone, chips fresh -> expected-missing (visible warn) no binary, no plist -> not-applicable (no row, headless stays clean) Stand: Echo Act IV Pro
|
Both findings taken. Pushed P1 — you're right, and it's worse than "a warning on Linux". I wrote in the PR body that a standing warn was "the intended trade." That was wrong: a headless core isn't a degraded macOS core, it's a supported deployment where the app is not applicable, and I'd turned an optional capability into permanent degraded health for it. That's the nag-you-never-open shape, which I now built rather than merely catalogued. The fix gates on whether the host ASKED for the app, with two signals because there are two install paths: The second matters and I nearly shipped without it: this repo's own machine has no menubar plist and runs the app anyway, so a plist-only gate would have left exactly the live configuration silent — my fix would not have covered my own host. Recency (7d) ages the marker out, so a deliberate uninstall stops warning instead of nagging forever.
The control you asked for, run against the three real configurations, not only fixtures: And directly: P2 — fair, and it was my own project's rule. The 14-line docstring is down to two; the incident history is here instead. Also green: 13/13 in the new suite, the eight sibling One residual gap, stated rather than left implicit: a macOS host that ran the app manually, never installed the plist, and hasn't run it in over 7 days will read |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head b585df873ac80857ec94b9c757a8977f4a68f23e.
Changes requested - not merge-ready.
- [P1]
src/health-check.py:7120uses a bare PEP-604 annotation,float | None, in a file that does not importfrom __future__ import annotations. That crashes when the module is imported on Python 3.9, which is still the stock macOS Python used by this project. The new test file has the future import, but the production module it loads does not, sopython3 tests/health-check-sutando-app-absent.test.pyfails before the assertions run.
Verification:
PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 tests/health-check-sutando-app-absent.test.pyfails withTypeError: unsupported operand type(s) for |: 'type' and 'NoneType'atsrc/health-check.py:7120.- Hosted
tsc + tests (clean install)fails ontests/python39-union-annotations.test.py. - Hosted
diff coverage >= 95% (python)also fails because the Python suite is not green; its log reports the same PEP-604 annotation issue insrc/health-check.py.
Reviewed by Qingyun's Personal Codex.
`now: float | None` needs `from __future__ import annotations`, which this file does not have, so it crashes on the stock macOS 3.9 interpreter — caught by tests/python39-union-annotations.test.py (issue #961, #960 incident). Dropped the annotation rather than adding the future import: migrating the annotation style of an 8k-line shared file is a separate concern from the probe this PR changes. Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head b78ab82d54a15eb07502e6a73dc198f9a0639da4.
Changes requested — not merge-ready.
- [P1 false warning] The new
contextual-chips.jsonrecency signal is not app-owned.src/init.sh:287-289creates that file with the current timestamp for every newly initialized workspace. Therefore a normal macOS install with neither optional app binary nor launchd plist is classifiedexpected-missingfor seven days and gets the same standing warning this revision intends to avoid. The focused test misses the production shape becausetest_macos_without_the_optional_app_is_not_unhealthyleaves the chips file absent, whiletest_a_RECENT_chips_file_means_the_app_ran_hereassumes any recent file proves the app ran. Remove this ambiguous signal or replace it with an app-specific durable marker, and add the initialized-workspace control. - [P2 repository prose contract] The new source docstring at
src/health-check.py:7121-7125and test module docstring attests/health-check-sutando-app-absent.test.py:2-6exceed the two-line comment/docstring rule. Condense them and keep rationale in the PR body.
The PEP-604 blocker reported on intermediate head b585df87 is fixed. At this head, all 13 new cases, the Electron/hotkey/compiled-artifact suites, Python 3.9 compilation, diff hygiene, and the REVIEW.md gate pass. Those greens do not cover the real initialized-workspace sentinel collision above.
Worst-case disruption is a fresh supported macOS core reporting an optional component as broken for a week. Fixing the ownership signal and prose is required before approval.
| # The app is the sole writer of contextual-chips.json, so a RECENT one means | ||
| # it ran here; ageing it out stops a deliberate uninstall nagging forever. | ||
| try: | ||
| if (now or time.time()) - chips.stat().st_mtime < recent_s: |
There was a problem hiding this comment.
[P1 blocker] Recency does not prove the app ran: src/init.sh:287-289 seeds state/contextual-chips.json with the current timestamp for every new workspace. A fresh macOS headless/optional-app-free install therefore enters expected-missing here for seven days. Please use an app-owned marker (or only the durable plist signal) and test the initialized-workspace shape.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head b78ab82d54a15eb07502e6a73dc198f9a0639da4.
Approval signal - the Python 3.9 blocker from the previous head is fixed, and I do not see a blocking issue in this scoped health-check change. Headless/non-macOS installs now stay silent, configured macOS installs with a missing binary remain visible, and the existing installed-app pgrep path is preserved.
Focused verification:
git diff --check origin/main...HEADpassed.PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 -m py_compile src/health-check.pypassed.PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 tests/health-check-sutando-app-absent.test.pypassed: 13 tests.PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 tests/python39-union-annotations.test.pypassed.PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 tests/health-check-sutando-app-hotkeys.test.pypassed.PYTHONPYCACHEPREFIX=/private/tmp/pycache-sutando-2855 python3 tests/health-check-sutando-app-electron.test.pypassed (rerun unsandboxed because the sandbox blocked/bin/ps).- Hosted checks are green.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head b78ab82d54a15eb07502e6a73dc198f9a0639da4 again after the later same-head approval.
Correcting that approval: changes are still required; this head is not merge-ready.
- [P1 false warning]
src/health-check.py:7135treats any recentcontextual-chips.jsonas proof that Sutando.app ran. Butsrc/init.sh:287-289seeds that file with a current timestamp for every newly initialized workspace. A normal fresh macOS install with neither the optional app nor its launchd plist therefore entersexpected-missingand reports the optional component as broken for seven days. The same-head approval only exercised the fixture where the chips file is absent and did not cover this production initialization path. Use an app-owned marker (or only the durable plist signal) and add the initialized-workspace control. - [P2 repository prose contract] The added docstrings at
src/health-check.py:7121-7125andtests/health-check-sutando-app-absent.test.py:2-6still exceed the repository's two-line comment/docstring rule.
No code changed after the prior exact-head reproduction; this review restores the formal blocker that the later approval inadvertently superseded.
…OS install `src/init.sh:287` seeds `state/contextual-chips.json` with a CURRENT timestamp for every newly initialized workspace, so "recent chips" never meant "the app ran here". A normal macOS install with neither the optional binary nor the launchd plist entered `expected-missing` and reported the optional component broken for seven days — the exact false-warning class this PR exists to remove. The plist is now the only durable "this host asked for the app" signal. Accepted gap: a host running the app WITHOUT a launchd plist goes silent again. That is strictly better than warning on every fresh install, and the alternative (an app-owned marker) needs Sutando.app to write one — out of scope here, since the app is the single writer of the chips file. The two fixtures that missed this are replaced by the initialized-workspace control plus a reintroduction guard. Both were verified to FAIL against the old logic before being kept. Stand: Echo Act IV Pro
|
Both points fixed at P1 — confirmed, and the premise I wrote was simply false. My comment claimed "the app is the sole writer of contextual-chips.json". It is not: So every newly initialized workspace gets a chips file with a current timestamp, and a normal macOS install with neither binary nor plist entered Took your first option: the signal is gone, and the launchd plist is now the only durable "this host asked for the app" evidence. Accepted gap, stated plainly: a host that runs the app without a launchd plist goes silent again. I judged that strictly better than warning on every fresh install. The app-owned marker you suggested is the better end state, but it needs Sutando.app to write one, and the app is the single writer of the chips file — that is a separate PR. The control you asked for, plus a reintroduction guard — and I falsified both before keeping them: A control that passes proves nothing unless it fails in the broken state, and that discipline is exactly what my original two fixtures lacked. The second test asserts the function body never references chips at all, so this cannot come back through a different code path. P2 — both docstrings are now 2 lines, checked by parsing rather than by eye: Also green at this head: |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head cd131881761b51b22c75c14280121f27d6f68ac2.
Blocking status: I cannot approve this head while the hosted merge check is red. The health-check delta itself looks like it addresses the previous blocker: the seeded state/contextual-chips.json signal is no longer consulted, a fresh initialized workspace stays silent, and a host that actually installed the com.sutando.menubar launchd job still emits a visible sutando-app warning when both binaries are absent.
The remaining blocker is CI on the PR merge ref, not this two-file health-check diff. GitHub is testing 82e65d9e (cd131881 merged into current main at 90c168b7), and lint-hermetic-bridge-tests fails because tests/discord-bridge-heartbeat-offload.test.py imports the bridge without isolating CLAUDE_CONFIG_DIR. That file comes from current main (90c168b7), and the known fix is the open #2857 head 584691c4. Please rebase/update this branch after that fix lands, or carry the hermetic-test fix here, so the PR merge ref is green.
Focused verification:
- Checked out PR head under
/private/tmp/sutando-pr2855-clRXZD/repo. git diff --check origin/main...HEADpassed.python3 tests/health-check-sutando-app-absent.test.pypassed: 12 tests.python3 tests/health-check-sutando-app-hotkeys.test.pypassed.python3 tests/health-check-import-without-plistlib.test.pypassed.python3 tests/health-check-summary-warnings.test.pypassed.python3 tests/python39-union-annotations.test.pypassed.python3 -m py_compile src/health-check.pypassed.git diff origin/main...HEAD | bash scripts/review-checks.shpassed.python3 scripts/lint-hermetic-bridge-tests.pypasses on the PR head alone, but fails on GitHub'spull/2855/mergeref with the samediscord-bridge-heartbeat-offload.test.pyerror shown in Actions.
Reviewed by Qingyun's Personal Codex.
|
Heads-up: the
Your branch touches neither file, so nothing here needs changing. Both checks should go green on your next run once the fix lands. The fix is #2857 (test-only, verified: gate ok 75 scanned, both lint tests PASS). It has one approval and needs a second, and I authored it so I cannot clear it myself. Sorry for the noise on your PR. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Code review on exact head cd131881761b51b22c75c14280121f27d6f68ac2: the prior blocker is fixed and I found no new code issue.
The absent-app warning now keys off the installed launchd plist instead of contextual-chip recency, so a fresh macOS install no longer produces the false warning. The 12 focused tests, Python compilation, diff hygiene, and REVIEW path scan pass.
I am withholding formal approval only because hosted CI is red on this exact head (CI, coverage, and hermetic-bridge lint). Once those current-head gates are green, this is code-ready.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact merge head 767c43f955914242d89cbd209aecdbcd5a35acf5.
No blocking code finding. The two-file topic patch is unchanged from the previously reviewed head (stable patch ID 10467d5428003bb4c0243cd7973aa798568582f0); this update only merges current main.
Reviewer-run tests/health-check-sutando-app-absent.test.py passed 12/12. The predicate distinguishes a configured macOS host (launchd plist exists but both binaries are absent) from a supported headless/unconfigured install, and both binary locations still activate the existing probe.
Worst-case disruption is warning every headless or optional-app-free install; the macOS + durable-plist gate and explicit negative controls prevent that. Ready from code review. Hosted clean-install and diff-coverage checks were still running at submission, so the PR is not merge-ready until those, CLA, and the two-formal-approval gate are green.
Reviewed by Qingyun's Personal Codex.
…rt its shape The diff-coverage gate sat at 91.7% on one uncovered line: the `checks.append(...)` inside run_all_checks' `expected-missing` branch — the row this PR exists to emit. It was uncovered because the existing guard asserts by AST that run_all_checks BRANCHES on `_menubar`. That proves the branch is present; it cannot prove the branch emits anything, so a probe that branched and then did nothing would pass. Drives run_all_checks for real (the pattern five health-check tests already use) with menubar_app_state patched, and asserts the row: exactly one, status warn, detail naming the launchd label and saying UNKNOWN — silence and "ok" being the two things this must never say. Includes the CONTROL, because without it the assertion passes for a probe that emits a row unconditionally, and the headless case is the whole point: not-applicable -> NO sutando-app row at all Mutation-verified: neutering the append (branch taken, nothing emitted) fails 2 tests; restored, 14/14 pass. Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 19291e5a3a0b646581c562a3b1a64bee22bf4380.
Ready to merge from code review; no blocking finding. This push adds the missing production-path control: it drives run_all_checks() and proves expected-missing emits exactly one warning naming the configured launchd job and both binary paths, while not-applicable emits no sutando-app row.
Reviewer-run tests/health-check-sutando-app-absent.test.py passed 14/14, isolated Python compilation passed, and git diff --check, review preflight, and repository path/root-artifact checks passed. Hosted Python 3.9, ESLint, Ruff, shell, workspace, and bundle gates are green; aggregate CI and coverage were still running at review time.
Worst-case disruption remains bounded to configured macOS hosts whose installed menu-bar launchd job has lost both binaries; intentionally headless/unconfigured hosts stay silent.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. Verified the defect in main's source and mutation-checked the fix, rather than reading the diff and agreeing with it.
The defect is real on main. I walked the guard block by indentation to find its end:
line 7610: if dev_bin.exists() or app_bin.exists():
block ends line 7667: # Battery and memory health checks -> has else? False
No else. When neither path exists no sutando-app row is appended at all, and the summary then reads No failures — N warning(s) — identical to a healthy probe. That is the failure mode I keep hitting from the other side: a check that never ran and a check that passed produce the same output.
Why I could not observe it here, stated so it is not mistaken for "I don't see it": on this host src/Sutando/Sutando exists (/Applications/Sutando.app/... does not), so the or is satisfied and the probe runs — I get a real sutando-app row on every health check. Your framing that this is narrower than it looks is accurate.
The head has moved well past the PR body, and the current design is better than what the body describes. The body says "one else on the existing guard, emitting warn". The implementation at 19291e5a3 is menubar_app_state(dev_bin, app_bin, plist, is_macos) returning installed / expected-missing / not-applicable, keyed on the launchd plist as the durable signal that a host actually asked for the app. That is the right discriminator: a host that never wanted the menu-bar app stays silent instead of collecting a permanent warning, and only a host that installed the plist and then lost the binary gets flagged. Worth refreshing the body before merge so the rationale readers see matches the code.
Mutation-checked, both directions:
baseline 14 tests, OK
expected-missing -> not-applicable (drop the row) FAILED (1)
always return "installed" (never discriminate) FAILED (5)
restore 14 tests, OK
The first is the important one — it reintroduces exactly the original defect (silent absence) and the suite catches it with a single targeted failure.
And the suite polices itself, which is why I trust it: test_CONTROL_not_applicable_emits_NO_row exists precisely because the positive assertion would pass for a probe that emits a row unconditionally. That control is the difference between a test that proves the row appears when it should and one that merely proves a row appears.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact merge head f56c2d1cd6488f8475ceaa7a3afb8d69a54e7e7e.
Ready to merge from code review; no blocking finding. This is a merge-only update from the previously approved 19291e5a: the topic patch retains stable patch ID 2933fc1f, while the only incremental files are the already-reviewed current-main cron-gate change.
Reviewer-run evidence: tests/health-check-sutando-app-absent.test.py passed 14/14 at this head, isolated Python compilation passed, git diff --check and repository review checks passed. The configured-macOS failure path still emits one warning, while headless/unconfigured hosts remain silent. CLA and the completed hosted gates are green; aggregate clean-install and coverage were still running at review time.
Worst-case disruption remains bounded to configured macOS hosts whose launchd job has lost both app binaries.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
Why
run_all_checks()guards the menu-bar probe ondev_bin.exists() or app_bin.exists(). When neither path exists the block is skipped and nosutando-approw is appended at all — there is noelse. The report's summary line then readsNo failures — N warning(s), which is exactly what a healthy probe produces. A check that was never run and a check that passed are indistinguishable to anyone reading the output.This is narrower than it first looks, and worth stating precisely because I got it wrong myself before reading the code: the probe does not assume
/Applications. It is anoracross both paths, and on any host with a dev build it runs and discriminates correctly. The defect is only in the neither-exists case, and the symptom is absence, not a false pass.What changed
elseon the existing guard, emitting asutando-approw withstatus: warnnaming both paths it looked for and stating the state is unknown rather than ok.tests/health-check-sutando-app-absent.test.py— 4 assertions: theelseexists, it appends asutando-approw, that row's status is not"ok", and the detail names both paths.A host that genuinely has no menu-bar app now carries a standing warn. That is the intended trade: the previous behaviour was silence that read as coverage.
Before / after evidence
Same test file against both trees. Parent
8956660b:Head
033a7b56:All four fail at the parent and pass at head, so this is not a test that can only ever print OK. Restoring the fixed file after the control returns
OKagain.Also green:
py_compile src/health-check.py, and the six existingtests/health-check-*suites run unchanged.Why the test is structural
It parses the guard out of the AST rather than exercising the branch. Reaching that branch behaviourally means calling
run_all_checks(), which runs ~100 probes with subprocesses and network — too heavy and too flaky for the one line under test. The negative control above is what keeps a structural test honest.Scope
health-check.py's sutando-app probe only. No change to the probe's logic when a binary is present, no change to any other check, no change to how statuses are summarised.@sonichi flagging you — this came out of a #bot2bot thread with @Sutando-Mini and echo about which Sutando.app artifact is canonical. Separate finding from that thread, not addressed here: nothing in the repo actually builds
src/Sutando/Sutando.app—install-sutando-app-launchd.sh:56expectsSutando.app/Contents/MacOS/Sutando, its own error hint at:79suggestsswiftc -ointo a directory nothing creates, and noInfo.plistis generated anywhere. That one needs your decision on the canonical artifact before anyone writes it.