feat(doctor): default to the whole installation, filter with --project/--type/--team - #654
Merged
Conversation
…t/--type/--team doctor previously required <project> <type> up front, which was backwards: a reporter who does not already know which project/type to look at cannot use a doctor that demands one. Positional <project> <type> is dropped, not kept for compatibility -- doctor has few enough callers that carrying a stale positional form alongside flags that mean something different by default would be its own source of confusion. New interface: doctor default = whole install (every team, project, type) doctor --project <path> narrow to one project doctor --type <type> narrow to one type doctor --team <team> narrow to one team (all three combine; --redacted and --help unchanged) Argument parsing stays separate from scope-building, so a future flag change is a parsing-only change. Also fixes three defects found by running the rebuilt scope against the real installation: - A type whose delivery_modes is nothing but "off" (agmsg-app, hermes -- the desktop app owns its own send/receive UI) was queried against delivery.sh anyway, which exits 1 by design (nothing to report); doctor turned that into a warning, breaking the exit-code contract on an otherwise healthy install. Now checked via the manifest before querying, the same agmsg_type_get/delivery_modes key PR #631 already reads. - A (project, type) group with nothing held, no warning, and nothing beyond a bare idle mode line now collapses to one line instead of a 6-line block -- unreadable at real scale (dozens of such groups on a real install). - The "watch processes: N alive, M stale pidfiles" line default runtime status emits is an installation-wide fact (it scans the whole run/ directory), not a per-(project, type) fact; it and the warning it can produce now surface once, not once per group using default runtime status. Read-only behavior, the 0/1/2 exit-code contract, and --redacted's masking behavior are unchanged. SKILL.md and the per-type templates are not wired to this yet -- deliberately deferred to a separate PR once this scope shape is settled, so the 9 templates + the Windows dispatcher aren't rewritten twice. Addresses part of #267.
…gap, allow an empty install Three defects from static review of PR #654 (co2), plus one more found verifying the fix against the real installation: - --team was interpolated directly into a path segment (teams/$FILTER_TEAM/config.json) without running it through agmsg_validate_team_name, the existing validator every other entry point that turns a team name into a path already uses. A value containing ".." could resolve to a config-shaped file outside teams/ entirely. - The installation-wide "watch processes: N alive, M stale pidfiles" line was captured opportunistically from whichever pair's own delivery.sh call happened to emit it first. An installation whose registrations are ALL a no-delivery type (skips the call) and/or codex (overrides runtime status with its own per-role bridge lines) never captured it at all -- losing run/watch.*.pid stale-watcher detection entirely, not just deduplicating it. Now captured independently, once, via `delivery.sh status` with no <type>/<project> -- delivery.sh's own documented path for exactly this. - An explicit --project/--type/--team matching nothing is a usage error (exit 2, unchanged), but a genuinely empty installation with NO filters given is a valid whole-install scan whose answer happens to be empty -- that was also exiting 2, meaning diagnosing an empty installation itself violated the exit-code contract. Now falls through to the normal report path, which naturally produces a clean 0/0/0 with no special-casing. - _team_filter_lines's own exit status leaked out of its while loop's last executed command: a non-matching final input line left the loop (and the function) returning 1, and no caller guarded the assignment. Under set -e this silently aborted the whole script -- with zero output -- whenever --team's target sorted alphabetically before some other team sharing a registration on the same (project, type). Found running --team against the real installation; none of the existing --team fixtures happened to hit this ordering. Fixed with an explicit trailing return 0.
pull Bot
pushed a commit
to itsbrex/agmsg
that referenced
this pull request
Aug 16, 2026
Brings across the two commits main had and this branch did not: f271cd4 fix(check-inbox): deliver with exit 0 when the poll failed part-way (fujibee#658) (fujibee#745) 4d3202d feat(doctor): default to the whole installation, narrow with --project/--type/--team (fujibee#654) ## check-inbox.sh, test_inbox.bats -- this branch's side Both branches fixed fujibee#658 independently, so the conflict is two implementations of one property set rather than a disagreement. The properties, read from f271cd4's diff INCLUDING its removed lines -- two of them are invisible if only the additions are read: A capture WHICH team failed (the break itself pre-dated this commit; what is new is CLAIM_FAILED_TEAM) B the delivering path exits 0 and states the partial failure inside the payload C the empty path lets the exit status carry the failure D "no new messages" is printed only when the poll actually completed (before, the line was printed and then the status re-raised) All four are present in this branch's implementation, checked as behaviour rather than by variable name. Nothing in main's version exists outside them. My first reading of this diff listed three properties and got two of them wrong, because I read only the added lines. Additions say what is there; they do not say what changed. Corrected against a second reader's list. ## doctor.sh -- main's, with the collapse made reachable doctor.sh and test_doctor.bats arrive whole; this branch had never touched them. But main's collapse condition cannot fire here, and that is a semantic conflict git did not mark: main's condition delivery output <= 1 line AND mode = "off" this branch no bare `off` exists at all -- all four assignments in delivery.sh carry an annotation no settings file `off (unrecognized: ...)` 1 line settings present `off (no hooks...)` + path + 3 counts 5 lines So the only state satisfying <= 1 line is the one that must NOT collapse: `unrecognized` is a claim about what this check could not determine, not about the configuration. Collapsing it would tell an operator delivery is off when the truth is we could not tell. Ruled: collapse a genuine `off`, and keep the settings path on the collapsed line. nothing to report — <settings file> Five lines become one; what goes is the mode and three "entries: 0". The path stays because it is the only one answering a different question -- WHICH file was consulted, i.e. "looked and found nothing" versus "did not look". The line-count gate is removed: with it, the collapse is unreachable. Tests: the cases that assert the collapse now build a project with a readable settings file (a genuine off), and one new case fixes the other half -- a project that could not be read is not called "nothing to report" and keeps its "may not be registered" line. ## Verified bats test_doctor.bats + test_inbox.bats + test_delivery.bats 222 ok, exit 0 mutation: drop the path from the collapsed line -> the test fails
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
1 participant
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.
Reworks
doctor's scope selection. The previous form required<project> <type>up front, which was backwards in practice: a reporter who doesn't already know which project/type to look at can't use a doctor that demands one, and there was no way to see a whole project across types, or the whole installation at once.This addresses part of #267, which asked for both a
doctorcommand and aviewer/statusdashboard; this PR only touches thedoctorhalf.Interface change
Positional
<project> <type>is dropped, not kept for compatibility — doctor has few enough callers that a stale positional form alongside flags that mean something different by default would be its own source of confusion. Argument parsing stays separate from scope-building, so a future change to the flags is a parsing-only change.Also fixes, found by running the rebuilt scope against the real installation
delivery_modesmanifest key is nothing butoff(agmsg-app— the desktop app's own identity, which owns its own send/receive UI;hermes) has no agmsg-side delivery to ask about. Queryingdelivery.sh statusfor such a type exits 1 by design (nothing to report), and doctor was turning that into a warning — on a real installation with nothing actually wrong, this produced9 team(s), 56 registration(s), 5 warning(s)and exit 1, breaking the exit-code contract on day one. Now checked via the manifest before querying (agmsg_type_get <type> delivery_modes, the same function/key fix(resolve): detect a type's agent process from its manifest, not a fixed list #631 already reads for a different purpose) rather than by type name, so a future no-delivery type is picked up the same way automatically.mode: offline now collapses to one line (<project> [<type>] N registration(s), nothing to report) instead of a 6-line block. A real installation had dozens of these.watch processes: N alive, M stale pidfilesline default runtime status emits scans the wholerun/directory — an installation-wide fact, not a per-(project, type) one. It (and the warning it can produce) now surface once, not once per group using default runtime status.Unchanged
Read-only behavior (never claims/releases/removes a lock, pidfile, or registration), the exit-code contract (0 clean / 1 warnings / 2 usage-or-resolution error), and
--redacted's masking behavior — including that it now works consistently across the multi-project default scope, reusing the same per-run pseudonym-table approach the existing team/agent redaction already used.Not in this PR
SKILL.md and the per-type templates are not wired to this new interface yet — deliberately deferred to a separate PR once this scope shape is settled, so the 9 templates plus the Windows dispatcher aren't rewritten twice.
Testing
tests/test_doctor.bats: 30 tests (was 17 before this PR), covering the new default/--project/--type/--team scope selection (including that --project and --team each narrow out what they exclude, and that a project registered under two teams isn't double-counted), the dropped positional form now being a usage error, the no-delivery-type fix, the readability collapse (including that an explicitly-configured mode is never collapsed), the global-line dedup, and the full carried-over suite from the original PR.bats tests/test_doctor.bats— 30 ok, 0 not ok, exit 0.bats tests/test_delivery.bats— adjacent suite (doctor shells out todelivery.sh) — all green.9 team(s), 57 registration(s), 0 warning(s), exit 0,no warnings.