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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Fields

    Priority

    Medium

    Effort

    None yet

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions