Fail loudly on the three ways a bundle comes back empty

[davidhogeg-ch]

Why

One on-prem cluster, 12 nodes, seven bundles collected. Five described nothing at all, and the two that did arrived with a collection window that stopped eight days early and an alert section that had never evaluated. Every one of those runs reported success, and the gaps were only found by reading the execution logs by hand a week later.

Three independent failure modes, each of which the tool could have named at the time.

1 — Dotfiles were walked as query files

FindCompatibleQueries walked with no dotfile filter, and nothing downstream filters by extension — manager.go executes whatever the walk returns. Unpacking the release on macOS and copying the folder to a server leaves an AppleDouble ._name beside every file and directory, so the collector sent ._system.parts.sql to ClickHouse and tried to parse ._replica_readonly.yaml as a rule.

The real bundle:

47 of 84 collectors failed — "security validation failed for '._system.parts.sql': only SELECT queries are allowed"
15 of 15 alert rules failed  — "yaml: control characters are not allowed"
Alerts — 0 fired, 15 could not run, 1 not applicable, 30 total

Three of those 47 failures were real (system.crash_log, system.blob_storage_log_7_days and system.zookeeper_log_errors_1_day are absent on that version). The other 44 were noise, and the alert section was worthless.

Fixed in the walk, so both the query path and the alert path (FindVersionedFiles) are covered. rootDir itself stays exempt — . and ./queries.onprem are both legitimate, and there is a test for that.

2 — A -to in the past passed silently

Only to < from was validated. A window left over from an earlier incident truncates query_log, part_log, metric_log and text_log at that instant, while system.errors, system.parts and the other point-in-time tables still describe now — so the bundle looks complete and the recent hours are simply absent rather than healthy. The real run used -to 2026-09-16 on the 24th and lost two Keeper session losses that had happened in between; we only found them through zookeeper_connection.connected_time.

$ clickhouse-diagnostic -mode onprem -host … -from 2026-09-08T00:00:00Z -to 2026-09-16T23:59:59Z
Warning: -to is 281 hours in the past, so query_log, part_log, metric_log and text_log all stop
there. Anything after that is missing from the bundle rather than healthy — widen -to if the
problem is still live.

Two hours of slack, so a normal "collect the last few hours" run stays quiet.

3 — A user who can see no databases now warns before collecting

With SELECT ON system.* but no SHOW, the object-listing tables are filtered to nothing and the protected ones fail with 497. The run completes, and the archive is a few hundred KB whose system.tables, system.columns and system.databases files are 0 rows / 0 bytes — empty, not missing. Five nodes were collected that way and forwarded to support before anyone noticed.

Warning: user 'sysonly' can see no databases outside system, so this bundle will not describe any
of your tables, parts or replicas — the files will be empty rather than missing.
    Grant the diagnostic privileges and re-run:
    GRANT SHOW DATABASES, SHOW TABLES, SHOW COLUMNS ON *.* TO sysonly;
    GRANT SELECT ON system.* TO sysonly;

The dbErr != nil branch is defensive and not exercised by either shape I tested: system.databases is grant-filtered rather than access-denied, so even a user with no grants at all takes the count-is-zero path.

4 — README grant set

SHOW COLUMNS added. Without it system.columns comes back with no rows for user databases while every other file looks normal — the existing note covers SHOW DATABASES/SHOW TABLES but not this.

Also a warning not to substitute GRANT SELECT ON <db>.*. It populates the listing tables and collects an identical bundle — measured on 26.2.19.43, system.tables 2, system.columns 5, system.databases 2, system.parts 1 under either grant set — but it gives the diagnostic user read access to customer data, where the SHOW grants leave a SELECT against a user table refused with 497. That distinction matters for regulated customers, and this one had already been told the wrong thing.

Verification

• gofmt, go vet ./..., go test ./... — all clean (run in golang:1.23.9).
• Two new tests in internal/query/versioned_test.go. Reverting only finder.go makes them fail with the exact production symptom: dotfile leaked into results: name="._system.parts.sql" path="23.5.1.0/._system.parts.sql", plus the old Skipping directory with invalid version format: ._24.8.1.0 noise.
• Warning 2 observed against an unresolvable host, so it demonstrably fires before the connection attempt.
• Warning 3 observed against clickhouse/clickhouse-server:26.2 (26.2.19.43) on a private Docker network, with both a SELECT ON system.*-only user and a user with no grants.
• Grant-set equivalence measured on the same server across all four grant combinations.

Not in scope

query_log_details still duplicates every sum per table touched (LEFT ARRAY JOIN tables). The SQL comment warns, SKILL.md warns, and inspect_bundle.py handles it correctly — but reading the file directly still multiplies counts by the table fan-out, which is how an 8× over-count reached a customer draft on this case before review caught it. Emitting the tables cardinality as a column would let any reader divide; worth a separate discussion.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved test and preflight issues can cause failures or missed warnings.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

• Avoid conflicting file and directory paths in the test · New
• Distinguish privilege errors from other database query failures · New
• Update privilege guidance to include SHOW COLUMNS · New

What changed in this PR

Improves detection of incomplete diagnostic bundles by filtering dotfiles, warning about stale windows and missing visibility, and updating grant guidance.

Changes:

• Filter dotfiles from query and alert discovery.
• Add stale-window and database-visibility warnings.
• Update grant documentation and finder tests.

File	Summary
README.md	Documents grants; related operational references still need updating.
internal/​query/​versioned_test.go	Adds dotfile tests; one test fixture is invalid.
internal/​query/​finder.go	Skips dotfiles during discovery.
cmd/​main.go	Adds preflight warnings; format handling, error classification, and dry-run behavior require changes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[CamiloSierraH]

Not a conflict — the fixture uses two different names: ._23.5.1.0 is the standalone dotfile and ._24.8.1.0/system.parts.sql is the hidden directory, so both create fine. The test runs and passes in CI (test job green on every push). Leaving it as is.

[CamiloSierraH]

Fixed in b9d3cba — agreed. The outcome is now classified: the server's own denial (Code: 497 / ACCESS_DENIED) keeps the privilege message and the GRANT advice; a zero count keeps the "can see no databases" message; any other error — a dial timeout, a 159, a parse error on an old server — says the check could not be made, that collection continues, and lists the grants only as "the fix if the listing files come back empty". Table-tested with the real 497 text, an i/o timeout and a 159 (cmd/preflight_test.go). Also: the probe no longer runs in dry-run, where the client answers with a synthetic empty response and the verdict would have been about the client, not the server.

[CamiloSierraH]

Fixed in b9d3cba — and there were more than the two you found: running-the-tool.md (the grant block and the troubleshooting row), SKILL.md, health-checks.md's coverage row, and the README's own cloud grants box a few lines below this one had all kept the old set. All five now say SHOW DATABASES, SHOW TABLES, SHOW COLUMNS, and running-the-tool.md carries the same warning as the README against substituting GRANT SELECT ON <db>.*.

[CamiloSierraH]

Pushed b9d3cba on top of this branch for the review round (Camilo asked me to close it out): the visibility preflight now distinguishes a real 497 from a transient error and is skipped in dry-run, and SHOW COLUMNS is in every place the grant set is stated — five, not two; the README's cloud box had it missing too. One finding declined on the thread: the dotfile test fixture has no file/directory conflict (._23.5.1.0 vs ._24.8.1.0/), and it passes in CI. gofmt, go vet, go test ./... clean. @davidhogeg-ch — shout if you would rather have taken it differently.

[CamiloSierraH]

LGTM