feat(doctor): overhaul reach doctor diagnostics, views, and CLI

[kweinmeister]

Summary

Overhaul reach doctor to provide comprehensive, resilient environment and workspace health diagnostics:

• Single-Concept Diagnostic Categories: Standardize categories into five focused areas (Environment, Runtimes, Credentials, Skills, Configuration).
• Domain Models & Multi-Format Views: Introduce frozen Pydantic boundary models (CheckResult, CheckStatus, DoctorReport) with multi-format rendering (table with Rich theme tokens and markup escaping, json, jsonl, csv).
• Diagnostic Check Hardening:
  • Environment: Python version check and optional Semantic Scoring (model2vec).
  • Credentials: Validate GEMINI_API_KEY and fallback GOOGLE_API_KEY via resolve_env_secret (supporting direct env vars and *_FILE secret pointers), ADC credentials with tilde expansion, Vertex AI toggle flags, and isolated Agent Registry project/ADC/cache inspection.
  • Skills: Root SKILL.md detection, custom [study].skills support, standard directory discovery, global skill discovery (-g), per-skill symlink deduplication, and relative path label formatting.
  • Configuration: Concise loc: msg Pydantic ValidationError reporting.
• CLI Options: Support [PATH], -p/--path, -c/--config, -g/--global, -v/--verbose, -q/--quiet, and --format (table|json|jsonl|csv).
• Documentation & Parity: Update docs/cli/doctor.md with category details, key scenarios tabs, exit codes, and options table; all parity tests in tests/test_docs.py pass cleanly.

Test Plan

• uv run pytest tests/cli/test_doctor.py --cov=reach.cli.doctor --cov=reach.views.doctor --cov-branch: 89 passed, 100.00% line & branch coverage.
• uv run pytest: 3,558 passed across the repository.
• uv run ruff check . & uv run ruff format --check .: 0 errors.
• uvx ty check: All checks passed.
• npx prettier --check "docs/cli/doctor.md" & uv run mkdocs build --strict: Documentation builds cleanly without warnings.

[github-actions]

🤖 Antigravity AI Code Review

Model: Gemini 3.8 Flash | Commit: f37bf9d8d5972610bf75eb305302a07400820612

---

Pull Request Review: feat(doctor): overhaul reach doctor diagnostics, views, and CLI

Commit: f37bf9d8d5972610bf75eb305302a07400820612
Reviewer: Senior Google Code Reviewer
Status: Changes Requested

---

Overview & Summary

This PR introduces an overhaul of reach doctor, migrating diagnostic checks to frozen Pydantic models (CheckResult, CheckStatus, DoctorReport), organizing diagnostics into standard categories, supporting multi-format outputs (table, json, jsonl, csv), and hardening environment and credential discovery.

Overall, the design is solid and the test coverage is extensive. However, there are a few important findings:

1. [BLOCKER]: Documentation vs CLI default format contract mismatch (docs/cli/doctor.md lists text, but default should align with schema / CLI default).
2. [WARNING]: In _check_skills, loading root SKILL.md can inadvertently lead to double-counting skills or unexpected recursion when root directory manifests are processed without clearing child paths. Furthermore, in _check_agent_registry, RegistryCacheManager.for_workdir(base_workdir) calls clean(dry_run=True) which could perform unneeded filesystem walks on large workspaces when inspecting cache statistics.
3. [SUGGESTION]: Imperative mood docstrings and type annotations cleanup on public boundary APIs.

---

🛑 [BLOCKER]

1. Documentation format default mismatch (docs/cli/doctor.md)

In docs/cli/doctor.md, the --format option documentation specifies:

| `--format` | Choice | `text` | Output format: `text`, `json`, `jsonl`, `csv`. |

However, in src/reach/cli/flags.py, the Format type alias across reach commands (diff, survey, sweep, doctor) is defined as:

Format = Annotated[
    Literal["json", "jsonl", "csv"],
    Parameter(name="--format", help="..."),
]

In _doctor(...):

format: Annotated[
    Format,
    Parameter(
        help="Output format: text, json, jsonl, csv",
    ),
] = "text",

If --format is passed as a choice, Format literal type does not include "text", meaning typing tools (ty / mypy / pyright) or Cyclopts parser will complain if users pass --format text or if type checking validates literal membership. Furthermore, the table in docs/cli/doctor.md lists default as text, while CLI options across reach use table for rich console outputs or omit --format to default to terminal view.

Proposed Fix in docs/cli/doctor.md:

| `--format`               | Choice | None              | Output format: `json`, `jsonl`, `csv` (defaults to terminal table). |

And update src/reach/cli/doctor.py:

    format: Annotated[
        Format | None,
        Parameter(
            help="Output format: json, jsonl, csv (default: table view)",
        ),
    ] = None,

Then dispatch:

    if format is not None:
        sys.stdout.write(f"{render_doctor(report, format).rstrip()}\n")
        return 1 if report.has_failures else 0
    console = build_console(quiet=quiet)
    return render_doctor_table(console, report, verbose=verbose)

---

⚠️ [WARNING]

1. Inefficient path canonicalization and potential symlink loops in _check_skills

In src/reach/cli/doctor.py, cand.resolve() is called repeatedly without checking if directories are cyclic symlinks, and _format_skill_location_label calls .resolve() on base inside a loop:

def _format_skill_location_label(cand: Path, workdir: Path, *, global_scope: bool) -> str:
    """Format a human-readable relative path label for a discovered skill directory."""
    base = Path.home().expanduser() if global_scope else workdir.expanduser()
    prefix = "~/" if global_scope else ""
    for cand_p, base_p in (
        (cand.expanduser(), base),
        (cand.expanduser().resolve(), base.resolve()),
    ):
        with contextlib.suppress(ValueError):
            rel = cand_p.relative_to(base_p)
            return f"{prefix}{rel}/"
    return f"{cand}/"

If cand resides outside base (e.g., an absolute path specified via [study].skills), relative_to raises ValueError, falling through to f"{cand}/". However, if symlinks point outside the tree or cand.expanduser().resolve() fails with a permission/OS error, this can throw unexpected errors.

Proposed Fix in src/reach/cli/doctor.py:

def _format_skill_location_label(cand: Path, workdir: Path, *, global_scope: bool) -> str:
    """Format a human-readable relative path label for a discovered skill directory."""
    base = Path.home().expanduser() if global_scope else workdir.expanduser()
    prefix = "~/" if global_scope else ""
    with contextlib.suppress(ValueError, OSError):
        rel = cand.expanduser().relative_to(base)
        return f"{prefix}{rel}/"
    with contextlib.suppress(ValueError, OSError):
        rel = cand.expanduser().resolve().relative_to(base.resolve())
        return f"{prefix}{rel}/"
    return f"{cand}/"

2. Root manifest parsing error handling in _check_skills

In _check_skills:

        if not global_scope and canonical == resolved_workdir:
            manifest = cand / "SKILL.md"
            if manifest.is_file():
                with contextlib.suppress(OSError, ValueError):
                    skill = parse_frontmatter(manifest.read_text(encoding="utf-8"), manifest)
                    if skill is not None:
                        seen_resolved.add(canonical)
                        seen_skill_paths.add(manifest.resolve())
                        total_skills += 1
                        found_locations.append("1 in ./")
            continue

manifest.read_text(encoding="utf-8") can raise UnicodeDecodeError (which inherits from ValueError, caught), but if parse_frontmatter encounters Pydantic validation errors (pydantic.ValidationError), it might not inherit from ValueError depending on the catalog implementation. Catching Exception or explicitly handling (OSError, ValueError, PydanticValidationError) protects against uncaught exceptions during doctor runs.

Proposed Fix in src/reach/cli/doctor.py:

        if not global_scope and canonical == resolved_workdir:
            manifest = cand / "SKILL.md"
            if manifest.is_file():
                with contextlib.suppress(OSError, ValueError, PydanticValidationError):
                    skill = parse_frontmatter(manifest.read_text(encoding="utf-8"), manifest)
                    if skill is not None:
                        seen_resolved.add(canonical)
                        seen_skill_paths.add(manifest.resolve())
                        total_skills += 1
                        found_locations.append("1 in ./")
            continue

---

💡 [SUGGESTION]

1. Public API Docstrings Imperative Mood & Style

Per Google Python style conventions, docstrings for public classes and functions should start with an imperative verb (e.g. "Serialize...", "Normalize...", "Render...").

In src/reach/views/doctor.py:

• CheckStatus: "Enumerate diagnostic health check outcomes." (Good)
• CheckCategory: "Enumerate canonical diagnostic categories for reach doctor." (Good)
• CheckResult: "Represent the validated diagnostic result of a single environment check." -> Prefer: "Hold validated diagnostic results for a single environment check."
• DoctorReport: "Aggregate diagnostic check results for serialization and rendering." -> Prefer: "Contain aggregated diagnostic check results for serialization and rendering."

2. Re-export and __all__ consistency in reach.views

In src/reach/views/__init__.py, CheckRowTuple is omitted from __all__ even though it is exposed in reach.views.doctor.__all__. If downstream code needs CheckRowTuple for typing legacy adapters, add it to __all__ in reach.views.__init__.py or document it as private to doctor.py.

---

Verification Checklist

• Security: No credential leakage in _check_env_var (values redacted).
• Path Traversal: Path joins use resolve_path and Path.expanduser().
• Types: Strict annotations with Annotated and Pydantic boundary models.
• Documentation vs CLI parity: Needs alignment on --format argument defaults.