Skip to content

chore(doctor): address pre-existing tech debt found during PR #722 review #765

Description

@sonupreetam

Context

During the /review-council for PR #722 (doctor-grouped-output), the Divisor agents flagged several pre-existing issues in internal/doctor/ and cmd/complyctl/cli/doctor.go that were not introduced by the PR but surfaced during review.

Findings

1. registryErrors cache bypassed by resolvePinnedFallback (SRE HIGH)

File: internal/doctor/doctor.go

The registryErrors cache (line 370) prevents redundant ResolveLatestVersion calls to unreachable registries. However, when a cached error is found, resolvePinnedFallback still calls resolver.ResolveVersion() -- making a new network call to the same unreachable registry.

With N pinned policies from one unreachable registry: the first policy triggers 2 timeouts (latest + pinned), each subsequent triggers 1 timeout (pinned fallback). For 5 policies at 5s timeout = 30s of wasted waits.

Fix: Extend the registryErrors cache to be consulted inside resolvePinnedFallback, or skip the pinned resolution attempt when the error is a known network failure (not a 404).

2. CheckVariables complexity 38, 211 lines (Architect MEDIUM)

File: internal/doctor/doctor.go:580-791

CheckVariables handles four responsibilities: evaluator-target resolution, global variable validation, target variable validation, and verbose detail generation. Gaze flags it as Q4 Dangerous (complexity 38, GazeCRAP 38).

Fix: Extract resolveEvaluatorTargets() and validateProviderVariables() helpers. Each would be under 50 lines (CS-010).

3. Run() has 8 parameters (Architect MEDIUM)

File: internal/doctor/doctor.go:117

The signature exceeds the 99-character line limit and violates AP-001 (Options struct pattern).

Fix: Introduce a RunOptions struct to bundle the parameters.

4. printDiagnostics writes to stdout, not io.Writer (Architect LOW)

File: cmd/complyctl/cli/doctor.go:108-167

Uses fmt.Println/fmt.Printf directly instead of accepting io.Writer per AP-003 (testable CLI pattern). This will be naturally addressed when #611 (machine-readable output) is implemented.

Fix: Accept io.Writer parameter, use fmt.Fprintln/fmt.Fprintf. Likely addressed alongside #611.

Priority

Medium -- the registry cache bypass (#1) has real user impact for offline/air-gapped environments. The complexity items (#2-#4) are structural improvements.

Related

LLM-assisted: yes

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Fields

    Priority

    Medium

    Effort

    None yet

    Projects

    Status
    Ready 🚀

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions