fix: report TLS trust source in apm doctor

[Aryan Singh K. (aryansk)]

Closes #2035

Summary

• report the active TLS trust source and precedence in apm doctor
• isolate process-global SSL/truststore state between tests with a shared autouse fixture
• add regression coverage for the doctor TLS row

Validation

• patch syntax and application context were checked against current main
• repository tests could not be executed in this chat environment; CI should run the project test suite

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new “TLS trust” informational check to apm marketplace doctor and stabilizes test isolation around process-global truststore injection.

Changes:

• Introduced describe_tls_trust() to generate user-facing TLS trust-source and precedence text.
• Added a new “TLS trust” informational doctor check and a corresponding unit test assertion.
• Added an autouse pytest fixture to reset truststore/SSL-related global state between tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
tests/unit/commands/test_marketplace_doctor.py	Adds a unit test ensuring the doctor output includes the TLS trust section/source.
tests/conftest.py	Adds an autouse fixture to reset truststore injection and clear TLS trust configuration cache between tests.
src/apm_cli/core/tls_trust.py	Adds describe_tls_trust() to report TLS trust source and precedence.
src/apm_cli/commands/marketplace/doctor.py	Adds a new informational “TLS trust” check to doctor output.

---

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

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/apm_cli/commands/marketplace/doctor.py:198

• This introduces a second "Check 4" header; it makes the check numbering confusing when scanning the function. Consider removing the numeric label here (or renumbering all subsequent checks).

    # Check 4: TLS trust source (informational)

src/apm_cli/commands/marketplace/doctor.py:205

• TLS trust diagnostic detail can include non-ASCII text (explicit CA bundle paths, truststore error messages). That can trigger UnicodeEncodeError on Windows terminals and violates the repo's printable-ASCII output constraint. Sanitize the rendered strings before adding them to the doctor table (including the exception path).

    try:
        from ...core.tls_trust import describe_tls_trust

        tls_source, tls_precedence = describe_tls_trust()
        tls_detail = f"{tls_source}; precedence: {tls_precedence}"

tests/conftest.py:149

• The new autouse SSL isolation fixture calls truststore.extract_from_ssl(), but it does not snapshot/restore ssl.SSLContext itself. Some tests (e.g. TLS integration suites) directly assign ssl.SSLContext, and without restoring it here a future test could leak a patched SSLContext into subsequent tests.

@pytest.fixture(autouse=True)
def _isolate_ssl_state():
    """Restore process-global truststore injection between tests."""
    import contextlib

src/apm_cli/core/tls_trust.py:115

• describe_tls_trust() is documented as "user-facing", but it can embed non-ASCII bytes from REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE values or from exception messages recorded in _LAST_TLS_STATUS. If callers print this directly (as apm doctor does), it can violate the repo's printable-ASCII output constraint on Windows.

def describe_tls_trust(env: Mapping[str, str] | None = None) -> tuple[str, str]:
    """Return a user-facing trust source and precedence description."""
    if _env_flag(_DISABLE_ENV_VAR, env):
        source = f"bundled CA (certifi); {_DISABLE_ENV_VAR}=1 disables OS trust-store injection"
    elif has_explicit_ca_override(env):

tests/conftest.py:129

• The linked issue acceptance criteria calls out removing the per-suite SSL/truststore isolation guards once a shared autouse fixture exists, but those guards still remain (e.g. tests/integration/test_tls_custom_ca.py:_isolate_trust and tests/integration/test_tls_frozen_hook.py:_isolate_trust). This PR adds the shared fixture but does not remove the redundant per-suite ones.

@pytest.fixture(autouse=True)
def _isolate_ssl_state():
    """Restore process-global truststore injection between tests."""

[Sergio Sisternes (sergio-sisternes-epam)]

APM Review Panel: ship_with_followups

apm doctor now surfaces the active TLS trust source, unblocking corporate-proxy users who silently fail on custom CA bundles.

cc Aryan Singh K. (@aryansk) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

Panel consensus is clear: the PR lands a well-placed, non-blocking diagnostic that addresses the #1 silent failure mode for enterprise CLI adoption (corporate proxy / custom CA). Architecture is sound -- describe_tls_trust lives in the canonical TLS module, the doctor check is informational-only, and no security surfaces are breached. All six active panelists converged on 'ship with polish'.

The two highest-signal follow-ups are (1) missing unit tests for describe_tls_trust's 4 branches and the error-degradation path -- these are regression traps on a user-promise surface and rank above opinion findings per test-evidence weighting -- and (2) the verbose precedence string that three panelists independently flagged as too dense for an 80-col table cell. The stale-read concern (python-architect, supply-chain) is real but low-impact: doctor is a short-lived CLI invocation where configure_process_tls_trust fires early in the command chain; documenting or eagerly calling it is a safe follow-up, not a blocker.

No panelist raised a blocking-severity finding. The CHANGELOG and docs gaps (doc-writer) are standard hygiene that should land in the same PR or an immediate follow-up before the next release cut.

Dissent. Python-architect and supply-chain-security both flagged stale _LAST_TLS_STATUS but diverged on remedy: architect wants an eager call, security wants documentation or querying ssl.SSLContext at call time. I side with the architect's eager-call approach -- it is one cached function call, zero-cost, and eliminates the confusing 'not selected yet' message that devx-ux independently flagged as user-hostile. The security suggestion to query SSLContext directly is over-engineered for an informational diagnostic.

Aligned with: Pragmatic as npm: Mirrors pip config debug and brew doctor patterns -- show users what is active, not a wall of internals. Progressive disclosure (--verbose for precedence chain) keeps the default output scannable. | Secure by default: Diagnostic is read-only, never mutates trust state. Supply-chain expert confirmed no credential leakage or security bypass. Path disclosure in CI logs is low-risk given the tool's local-execution model.

Growth signal. Corporate proxy/custom-CA is the #1 reason CLI tools silently fail at large orgs. This PR converts a 'file a ticket' moment into a self-service diagnostic -- worth a beat in release notes targeting enterprise adopters. Frame CHANGELOG as: 'apm doctor now tells you why TLS fails in corporate networks.' Compounds with any future apm doctor --fix auto-remediation.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	1	Solid placement of describe_tls_trust in the canonical TLS module. The module-global _LAST_TLS_STATUS read is acceptable for an informational diagnostic but has a stale-read edge case worth documenting. No blocking issues.
CLI Logging Expert	0	1	2	The TLS doctor check is well-structured but the precedence chain is too verbose for a table cell -- it belongs behind --verbose. The rest is clean.
DevX UX Expert	0	2	2	The TLS trust row is a good diagnostic addition but the inline precedence string is too dense for a table cell, the 'not selected yet' message is confusing, and the error fallback lacks actionable guidance.
Supply Chain Security Expert	0	2	2	No security bypasses or credential leakage. Minor concerns around path disclosure and misleading pass status, but nothing blocking.
OSS Growth Hacker	0	0	2	Strong adoption move. TLS diagnostics in apm doctor is exactly the kind of self-service debugging that unblocks enterprise users behind corporate proxies -- the #1 silent churn vector for CLI tools in regulated environments.
Doc Writer	0	2	1	The PR adds a new TLS trust row to apm doctor output. The CHANGELOG [Unreleased] section has no entry. The docs reference page Checks table does not list TLS trust. Two targeted updates needed.
Test Coverage Expert	0	2	1	New describe_tls_trust() has zero direct tests covering its 4 branches; doctor error path untested; autouse fixture leaks _LAST_TLS_STATUS across tests.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] Add parametrized unit tests for describe_tls_trust's 4 branches -- Missing regression trap on a user-promise surface (devx principle). No automated guardrail currently proves the precedence logic is correct.
2. [Test Coverage Expert] Add test for doctor TLS error-degradation path -- The except block that renders 'Unable to determine' has zero coverage -- if it regresses, doctor crashes instead of degrading gracefully.
3. [CLI Logging Expert] Move precedence chain behind --verbose; show only active source in default table -- Three panelists independently flagged the 120+ char detail string as too dense for 80-col terminals. Progressive disclosure is APM's output contract.
4. [Python Architect] Eagerly call configure_process_tls_trust() inside describe_tls_trust to eliminate stale-read / 'not selected yet' UX -- Eliminates confusing cold-state message and satisfies devx-ux's reword request in one line.
5. [Doc Writer] Add CHANGELOG entry and update doctor.md Checks table with TLS trust row -- User-visible behavior change with no changelog or docs update -- violates 'ship fast, communicate clearly' principle.

Architecture

classDiagram
    direction LR
    class tls_trust {
        <<Module>>
        +configure_process_tls_trust() None
        +describe_tls_trust(env) tuple
        +has_explicit_ca_override(env) bool
        -_LAST_TLS_STATUS tuple|None
        -_env_flag(name, env) bool
        -_explicit_ca_path(env) str
    }
    class doctor {
        <<Module>>
        +run_doctor() list~DoctorCheck~
    }
    class _DoctorCheck {
        <<Dataclass>>
        +name str
        +passed bool
        +detail str
        +informational bool
    }
    doctor ..> tls_trust : imports describe_tls_trust
    doctor *-- _DoctorCheck : builds
    class tls_trust:::touched
    class doctor:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm doctor command"] --> B{"configure_process_tls_trust\ncalled earlier?"}
    B -->|Yes| C["_LAST_TLS_STATUS populated"]
    B -->|No| D["_LAST_TLS_STATUS is None"]
    C --> E["describe_tls_trust()\nreads _LAST_TLS_STATUS"]
    D --> F["describe_tls_trust()\nreturns fallback string"]
    E --> G["Renders TLS source + precedence"]
    F --> G
    G --> H["_DoctorCheck\ninformational=True"]
    H --> I["Print doctor table"]

Loading

Recommendation

Ship now with the understanding that the test-coverage and --verbose follow-ups land before the next release cut. The core diagnostic is correct, non-blocking, and immediately valuable to the enterprise adoption surface. The missing tests are not blocking because the feature is informational-only (passed=True, informational=True) -- a regression here cannot break installs or corrupt state. Recommend the author addresses the top 3 follow-ups in a stacked PR or amends this one before merge.

---

Full per-persona findings

Python Architect

• [recommended] Stale-read of _LAST_TLS_STATUS produces misleading 'not selected yet' message if doctor runs before configure_process_tls_trust fires at src/apm_cli/core/tls_trust.py
  If apm doctor runs in a fresh process where configure_process_tls_trust() has not been called, _LAST_TLS_STATUS is None and the user sees 'not selected yet; OS trust store preferred with certifi fallback'. The actual trust source has not been determined yet. describe_tls_trust should eagerly call configure_process_tls_trust() (cached, so idempotent) to ensure the global is populated before reading it.
  Suggested: At the top of describe_tls_trust, call configure_process_tls_trust() to ensure _LAST_TLS_STATUS is populated.
• [nit] Stale Check 4 comment numbering after insertion -- two checks labeled Check 4 at src/apm_cli/commands/marketplace/doctor.py
  Purely cosmetic but makes grep-by-number unreliable.

CLI Logging Expert

• [recommended] Precedence chain concatenated into detail string is too verbose for default table output -- wraps badly at 80-col terminals at src/apm_cli/commands/marketplace/doctor.py
  The combined tls_detail string can exceed 120 chars (source + '; precedence: APM_DISABLE_TRUSTSTORE > REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE > APM_EXTRA_CA_BUNDLE (when supported) > OS trust store > certifi fallback'). In a table cell at 80-col terminal this wraps badly and buries the actionable source info. Show only tls_source in default mode; emit the full precedence chain only under --verbose. This follows APM's progressive-disclosure contract.
• [nit] Precedence string uses '(when supported)' hedging language -- specify or drop at src/apm_cli/core/tls_trust.py
  Either state the condition precisely or omit the qualifier for cleaner output.
• [nit] Error fallback truncation at 60 chars arbitrary -- may cut mid-word at src/apm_cli/commands/marketplace/doctor.py
  The [:60] slice could cut mid-word or mid-path. Consider truncating to last space or using a utility that appends '...' when truncated.

DevX UX Expert

• [recommended] Inline precedence chain is reference documentation, not diagnostic output -- belongs behind --verbose at src/apm_cli/commands/marketplace/doctor.py
  Show only the active source in the default view; move the precedence explanation behind --verbose or a apm doctor --explain flag. Compare: pip config debug prints the active config file, not the full resolution order inline.
• [recommended] 'not selected yet' message reads as broken state to users who haven't run network commands at src/apm_cli/core/tls_trust.py
  Reword to 'OS trust store (default)' or similar positive statement -- the user doesn't need to know about lazy initialization.
• [nit] Error fallback lacks actionable guidance for next steps at src/apm_cli/commands/marketplace/doctor.py
  Append a hint: 'run with --verbose or check REQUESTS_CA_BUNDLE'. Every doctor error should be one copy-paste away from recovery.
• [nit] Consider surfacing REQUESTS_CA_BUNDLE hint for corporate-proxy users when no custom bundle detected at src/apm_cli/core/tls_trust.py
  Surface 'set REQUESTS_CA_BUNDLE to your corporate CA path' when no custom bundle detected and OS trust store in use -- the primary audience for this check.

Supply Chain Security Expert

• [recommended] str(exc)[:60] in error fallback may leak filesystem paths from unexpected exceptions at src/apm_cli/commands/marketplace/doctor.py
  Consider sanitizing to just the exception type name when the message contains path separators or known sensitive patterns.
  Suggested: Use type(exc).name as the safe fallback, or filter the message through a path-redaction helper before truncating.
• [recommended] _LAST_TLS_STATUS is module-global mutable state read without synchronization -- may be stale at src/apm_cli/core/tls_trust.py
  In long-lived processes or concurrent test runs it could reflect a stale or different trust configuration than what is currently active, making the doctor output misleading rather than informative. Document that this is a snapshot.
  Suggested: Document clearly that this is a snapshot, or query ssl.SSLContext default_verify_paths() at call time.
• [nit] CA bundle path exposed in diagnostic output -- low-risk but reveals filesystem layout in CI logs at src/apm_cli/core/tls_trust.py
  Exposing the full CA bundle path via _explicit_ca_path(env) is low-risk (diagnostic tool user runs locally) but in CI logs it reveals internal filesystem layout.
• [nit] passed=True unconditionally may green-light a potentially insecure config (certifi fallback in corp env) at src/apm_cli/commands/marketplace/doctor.py
  Consider making it informational=True with passed=None or a tri-state so it does not green-light a potentially insecure config.

OSS Growth Hacker

• [nit] Consider linking to a docs troubleshooting anchor in the CHANGELOG entry so the surface compounds at src/apm_cli/commands/marketplace/doctor.py
  The detail string showing env-var precedence is excellent -- users paste this into Slack. Link to docs anchor to make it compound.
• [nit] CHANGELOG opportunity: frame as 'apm doctor now tells you why TLS fails in corporate networks'
  That framing is a repostable one-liner for enterprise DevRel channels and HN threads about proxy pain.

Auth Expert -- inactive

PR only adds a read-only TLS diagnostic reporter; no auth.py, token_manager.py, AuthResolver, or HTTP auth surfaces touched.

Doc Writer

• [recommended] Missing CHANGELOG entry for new TLS trust row in apm doctor at CHANGELOG.md
  The [Unreleased] section has no entry for this behavior change. The PR closes Add apm doctor tls trust-source diagnostic + test-suite ssl-isolation fixture #2035 and adds a new informational TLS trust row -- this is user-visible and belongs in the changelog.
  Suggested: Add under ### Fixed: apm doctor now reports the active TLS trust source as an informational row. (closes Add apm doctor tls trust-source diagnostic + test-suite ssl-isolation fixture #2035)
• [recommended] docs/src/content/docs/reference/cli/doctor.md Checks table missing TLS trust row at docs/src/content/docs/reference/cli/doctor.md
  The reference page lists every apm doctor check row but omits TLS trust. Users running apm doctor will see the row and find no explanation in the docs.
• [nit] packages/apm-guide commands.md does not mention TLS trust in apm doctor description at packages/apm-guide/.apm/skills/apm-usage/commands.md
  Secondary agent-usage reference omits the new check. Lower priority.

Test Coverage Expert

• [recommended] No unit tests for describe_tls_trust() branch logic -- 4 branches uncovered at src/apm_cli/core/tls_trust.py
  describe_tls_trust has 4 distinct branches (disable env, explicit CA, cached status, cold state). grep of tests/ shows only one hit: a mock in the doctor test that never exercises the real function. No tests in tests/unit/core/test_tls_trust.py cover it.
  Suggested: Add parametrized unit tests in tests/unit/core/test_tls_trust.py exercising each branch.
  Proof (missing): tests/unit/core/test_tls_trust.py::test_describe_tls_trust_branches -- proves: describe_tls_trust returns correct source string for each precedence branch [devx]
  assert source == 'bundled CA (certifi); APM_DISABLE_TRUSTSTORE=1 disables OS trust-store injection'
• [recommended] Doctor TLS error path (except Exception block) has no test at tests/unit/commands/test_marketplace_doctor.py
  The only doctor TLS test patches describe_tls_trust to return a tuple (happy path). No test verifies the error rendering or that the check degrades gracefully.
  Suggested: Add a test that patches describe_tls_trust to raise RuntimeError and asserts exit_code == 0 and 'Unable to determine' in output.
  Proof (missing): tests/unit/commands/test_marketplace_doctor.py::test_tls_trust_error_degrades_gracefully -- proves: doctor does not crash when TLS introspection fails; user sees informational degradation [devx]
  assert 'Unable to determine TLS trust source' in result.output
• [nit] Autouse fixture _isolate_ssl_state does not reset _LAST_TLS_STATUS global -- state leaks between tests at tests/conftest.py
  The fixture clears the lru_cache and calls truststore.extract_from_ssl() but never resets the module-level _LAST_TLS_STATUS. A test calling configure_tls_trust pollutes describe_tls_trust's third branch for all subsequent tests.
  Suggested: Add import apm_cli.core.tls_trust as _tls; _tls._LAST_TLS_STATUS = None in the fixture teardown.

Performance Expert -- inactive

PR adds a diagnostic-only describe_tls_trust() function with no loops, no I/O, no subprocess calls; no hot-path (install/resolve/cache/transport) surfaces touched.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}