fix(audit): drift check skip-with-info on cache miss (closes #1200)

[sergio-sisternes-epam]

Description

Treat cache miss during apm audit drift as skip-with-info instead of a check failure. When the install cache is not populated, the drift check now passes with an informational skip message rather than failing CI.

Fixes #1200

Changes

• ci_checks.py: _check_drift() CacheMissError handler now returns passed=True with skip message instead of passed=False
• audit.py: New elif branch echoes skip message to stderr for UX visibility
• baseline-checks.md: Drift check entry updated with "Skips when" bullet for cache-miss case
• drift-detection.md: Added cache-miss skip behaviour explanation
• CHANGELOG.md: Fixed entry under [Unreleased]

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass (8299 passed)
• Added tests for new functionality
  • TestCheckDriftCacheMiss: 3 unit tests covering passed=True, skip message, non-blocking behaviour
  • Updated integration test test_e10_bare_audit_surfaces_cache_miss_on_stderr

[Copilot]

Pull request overview

Adjusts apm audit drift behavior so a cold install cache (CacheMissError during replay) is treated as a non-blocking skip-with-info rather than a failing check, aligning CI ergonomics for fresh checkouts.

Changes:

• Update _check_drift to return passed=True with a clear "drift skipped: install cache not populated ..." message on cache miss.
• Update apm audit stderr UX so bare audit surfaces the drift-skip reason (not just drift failures) when no drift findings are produced.
• Add/adjust unit + integration tests and update docs + changelog to reflect the new skip semantics.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/apm_cli/policy/ci_checks.py	Make CacheMissError in drift replay skip-with-info (passed=True) with a new informational message.
src/apm_cli/commands/audit.py	Emit a stderr warning when drift was skipped (cache cold) during bare apm audit.
tests/unit/policy/test_ci_checks.py	Add unit tests around cache-miss drift behavior (one test currently doesn’t exercise drift).
tests/integration/test_drift_check.py	Update integration expectations for the new "drift skipped" stderr contract.
docs/src/content/docs/reference/baseline-checks.md	Document drift check as failing on diffs but skipping on cold cache.
docs/src/content/docs/enterprise/drift-detection.md	Document cache-cold behavior as skip-with-info and how to enable drift by warming cache.
CHANGELOG.md	Add Unreleased Fixed entry describing the new drift skip-with-info behavior.

[github-actions]

APM Review Panel: ship_with_followups

PR #1289 converts cold-cache CI failures into informational skips, unblocking the most common first-run friction point for new APM adopters -- but ships without an integration test defending the --ci exit-code promise and with a symbol/string coupling that four panelists flagged independently.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

The correctness call is unambiguous: treating CacheMissError as passed=True is the right behavior and every active panelist agrees. The CHANGELOG and docstring both document the intent clearly. The growth signal is real -- cold-cache failures are the highest-friction first-run CI event for new adopters, and removing them without user action is a meaningful onboarding win.

Two findings demand immediate follow-up before this becomes load-bearing in production pipelines. First, the test-coverage-expert identified a missing integration test for the --ci + CacheMissError path (outcome: missing, principle: governed-by-policy). The docstring in _check_drift explicitly promises "CI does not red-mark a fresh checkout" but no automated guardrail defends this promise -- the next refactor of the CI gate logic could silently re-break issue #1200 with no test catching it. Second, supply-chain-security and devx-ux-expert converge on the same structural risk: an adversary (or misconfigured pipeline) that cold-caches the CI artifact store will cause apm audit --ci to exit 0 with drift silently unchecked. This is not a CVE -- drift is advisory by design -- but it needs a documentation commit to enterprise/security.md and a --require-drift escape hatch tracked in a follow-up issue.

The remaining convergent signal across python-architect, cli-logging-expert, devx-ux-expert, and test-coverage-expert on the CheckResult.skipped field is consistent enough to treat as a single technical-debt follow-up rather than a blocker. The [!] vs [i] symbol issue from cli-logging-expert is a real UX contract violation (passed=True should not emit a warning symbol) and is a one-line fix worth landing in this PR or as an immediate patch.

Dissent. No panelist disagreed on the core correctness of the change. The only tension is between devx-ux-expert (exit 2 on skipped checks in --ci mode) and the current advisory-only design intent. Changing the default exit code would itself be a breaking change -- the correct resolution is a future --require-drift flag, not a default behavior change in this PR.

Aligned with: Secure by default -- cold-cache bypass is a real (if low-severity) weakening of the drift gate for hardened pipelines; production CI must document the apm install prerequisite. Community-over-feature-count -- fixing #1200 directly addresses the highest-friction first-run contributor pain point. Ship fast, communicate clearly -- the behavior change is minimal, correct, and documented; the missing --ci integration test is the only gap.

Growth signal. PR #1289 removes the most common first-run CI failure for new APM adopters. Worth a dedicated bullet in the next release post under "CI onboarding improvements": "Fresh checkouts no longer fail apm audit on a cold cache -- the drift check skips with a clear message guiding you to run apm install first." Risk to monitor: if drift skips too frequently in pipelines that never call apm install, the check loses signal value and becomes background noise.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Routine correctness fix, correct and minimal; elif guard couples to message-string prefix instead of first-class skipped state on CheckResult.
CLI Logging Expert	0	2	1	Cache-miss branch routes to stderr correctly; wrong STATUS_SYMBOLS key ([!] not [i]) and startswith() coupling both need attention.
DevX UX Expert	0	2	1	Directionally correct for bare audit; --ci silently exits 0 on cache miss with no distinguishable signal from a clean drift check.
Supply Chain Security Expert	0	1	1	Not a clean bypass, but cold-cache path removes the hard gate; document CI prerequisite and track --require-drift.
OSS Growth Hacker	0	2	1	Solid onboarding friction-reducer; CHANGELOG leads with impl detail and --no-drift in skip message needs qualification.
Doc Writer	0	1	2	Both changed doc files accurate; exit-code table missing the drift-skipped=exit-0 row.
Test Coverage Expert	0	1	1	Three unit tests correct; integration test e10 covers bare audit; missing --ci + CacheMissError integration test.

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 integration test: apm audit --ci + CacheMissError asserts exit_code == 0 and passed=true in JSON output -- outcome: missing on a governed-by-policy promise; the docstring documents the intent but no automated guardrail defends it. Suggested: test_e11_ci_audit_exits_zero_on_cache_miss in tests/integration/test_drift_check.py.
2. [CLI Logging Expert] Change STATUS_SYMBOLS['warning'] to STATUS_SYMBOLS['info'] in the passed=True skip branch (audit.py:729) -- [!] on a passed=True result is a UX contract violation; one-line fix suitable for this PR or immediate patch.
3. [Supply Chain Security Expert] Document cold-cache bypass in enterprise/security.md and open a follow-up issue for a --require-drift flag -- convergent signal from supply-chain and devx-ux: pipelines that skip the cache-warm step will silently skip drift.
4. [Python Architect / CLI Logging / DevX UX / Test Coverage] Add skipped: bool = False to CheckResult dataclass; replace startswith('drift skipped') guard with drift_check.skipped -- four panelists flagged this stringly-typed skip detection independently; clear technical-debt follow-up.
5. [OSS Growth Hacker] Rewrite CHANGELOG entry to lead with user benefit, not implementation detail; qualify --no-drift mention in skip message -- low-cost copy edit before release tagging.

Architecture

classDiagram
    direction LR
    class CheckResult {
        <<ValueObject>>
        +name: str
        +passed: bool
        +message: str
        +details: list[str]
    }
    class CIAuditResult {
        <<ValueObject>>
        +checks: list[CheckResult]
        +passed: bool
    }
    class _check_drift {
        <<Pure>>
        +__call__(config, logger) tuple[CheckResult, list]
    }
    class CacheMissError {
        <<Exception>>
    }
    class _audit_content_scan {
        <<IOBoundary>>
        +__call__(cfg, ...) None
    }
    _check_drift ..> CheckResult : returns
    _check_drift ..> CacheMissError : catches
    _audit_content_scan ..> _check_drift : calls
    _audit_content_scan ..> CheckResult : reads passed, message
    CIAuditResult *-- CheckResult : aggregates
    class _check_drift:::touched
    class _audit_content_scan:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm audit / apm audit --ci"]) --> B["_audit_content_scan\naudit.py:710"]
    B --> C["_check_drift\nci_checks.py:421"]
    C --> D["run_replay\ndrift.py"]
    D -->|success| E["CheckResult\npassed=True, findings=list"]
    D -->|CacheMissError| F["CheckResult\npassed=True\nmessage='drift skipped: ...'\n[BEFORE: passed=False]"]
    D -->|other error| G["CheckResult\npassed=False"]
    E --> H{"drift_failed?"}
    F --> H
    G --> H
    H -->|"passed=False and no findings"| I["stderr: [!] drift check could not run"]
    H -->|"passed=True, no findings, startswith('drift skipped')"| J["stderr: [!] drift skipped...\n[NEW elif branch]"]
    H -->|"passed=True with findings"| K["render drift findings"]

Loading

sequenceDiagram
    participant CI as CI runner
    participant audit as audit.py
    participant check as ci_checks.py
    participant cache as install cache
    CI->>audit: apm audit --ci (fresh checkout)
    audit->>check: _check_drift(config, logger)
    check->>cache: run_replay(config, logger)
    cache-->>check: CacheMissError
    note over check: BEFORE: passed=False, AFTER: passed=True + skip message
    check-->>audit: CheckResult(passed=True, message='drift skipped...')
    audit->>CI: stderr: [!] drift skipped: install cache not populated
    note over CI: BEFORE: exit 1, AFTER: exit 0 (non-blocking)

Loading

Recommendation

Ship PR #1289. The correctness fix is sound, the growth case is concrete, and every active panelist agrees on direction. Two items should be addressed before this is cited in release notes as a CI reliability guarantee: (1) the missing --ci integration test (follow-up 1 above -- the docstring promise needs an automated guardrail or it will silently regress), and (2) the [!] vs [i] symbol fix (follow-up 2 -- a one-line change). Items 3-5 are clean follow-up issues, not blockers. The cold-cache bypass (follow-up 3) is a real supply-chain consideration for hardened enterprise pipelines and requires a documentation commit, but it does not block the merge: the behavior is correct for the documented advisory-mode design.

---

Full per-persona findings

Python Architect

• [nit] audit.py elif couples to message string prefix instead of a first-class skipped state
  The new elif in audit.py disambiguates a cache-miss skip via drift_check.message.startswith("drift skipped"). This makes the caller depend on the exact wording of a message produced in ci_checks.py. A skipped: bool = False field on CheckResult would let the caller test drift_check.skipped instead.

CLI Logging Expert

• [recommended] Wrong STATUS_SYMBOLS key for a passed=True skip at src/apm_cli/commands/audit.py:729
STATUS_SYMBOLS['warning'] ([!]) means "yellow -- should act". But drift_check.passed is True; the check did not fail. The correct symbol is STATUS_SYMBOLS['info'] ([i]).
  Suggested: Change STATUS_SYMBOLS['warning'] to STATUS_SYMBOLS['info'] in the elif drift_check.passed branch only.

• [recommended] startswith('drift skipped') couples routing logic to message text at src/apm_cli/commands/audit.py:726
  A refactor that rephrases the message prefix will silently regress the UX. A dedicated skipped: bool field on CheckResult makes the contract explicit.
  Suggested: Add skipped: bool = False to CheckResult dataclass. Set skipped=True in the CacheMissError handler. Change guard to elif drift_check.passed and drift_check.skipped and not drift_findings.

• [nit] Message string embeds the discriminator prefix
  Once a skipped field exists, the 'drift skipped: ' prefix can be dropped from the message.

DevX UX Expert

• [recommended] apm audit --ci exits 0 on cache miss with no distinguishable signal from a clean drift check at src/apm_cli/policy/ci_checks.py:450
  CI pipelines relying on apm audit --ci as a drift gate will silently regress to zero drift coverage on any workflow that does not warm the cache first. Consider a --require-drift flag or a structured skipped: true field in JSON output.

• [recommended] Skip detection relies on brittle startswith('drift skipped') string match at src/apm_cli/commands/audit.py:726
  A skipped: bool field or CheckStatus enum would make this explicit and refactor-safe.

• [nit] No structured skipped field in JSON/SARIF output
  passed=True with message='drift skipped...' is indistinguishable from a clean pass to machine consumers.

Supply Chain Security Expert

• [recommended] Cold-cache bypass path: attacker who invalidates CI cache skips drift entirely at src/apm_cli/policy/ci_checks.py:450
  An adversary who can invalidate the CI artifact cache will cause apm audit --ci to exit 0 with drift silently skipped. All other ci_checks.py checks still run, so this is a partial bypass, but supply chain tampering manifesting only as a working-tree delta would go undetected.
  Suggested: (1) Document in enterprise/security.md that production CI must run apm install before apm audit --ci. (2) Track a --require-drift flag for hardened pipelines.

• [nit] CacheMissError exception variable exc dropped -- lose diagnostic detail in structured logs at src/apm_cli/policy/ci_checks.py:450
  Dropping as exc loses the specific cache miss reason in CheckResult.message; forensic SARIF/JSON output loses the root cause string.

OSS Growth Hacker

• [recommended] CHANGELOG entry leads with implementation detail rather than user benefit
  The entry front-loads passed=True, stderr before the user outcome. Reorder to lead with: "Fresh checkouts no longer fail apm audit due to a cold cache -- the drift check now skips with a clear message guiding you to run apm install first."

• [recommended] Skip message offers --no-drift as escape hatch without sufficient context
  For a new user, "or pass --no-drift" implies "skip drift forever" -- the wrong lesson. Qualify: "to permanently disable the check in performance-constrained CI, pass --no-drift".

• [nit] drift-detection.md: consider "automatically enables" for zero-config recovery feel

Auth Expert -- inactive

PR only touches audit drift-check cache-miss handling (audit.py, ci_checks.py, tests, docs/changelog). No auth.py, token_manager.py, credential resolution, host classification, or HTTP authorization surfaces are modified.

Doc Writer

• [recommended] Exit-code table in baseline-checks.md missing the drift-skipped=exit-0 case at docs/src/content/docs/reference/baseline-checks.md
  After this PR a third observable outcome exists: drift is skipped and job exits 0 with informational message. A reader scanning the exit-code table won't find this case.
  Suggested: Add row: | Drift check skipped (cache not warmed) | 0 (informational message on stderr) |.

• [nit] Skips when bullet contains redundant passive clause "returns a pass"
  Suggested: "Skips when. The install cache has not been warmed (the replay is cache-only by design so audit stays deterministic); an informational message advises running apm install first."

• [nit] drift-detection.md new paragraph has run-on construction
  Suggested: "When the install cache is cold (fresh checkout before the first apm install), the drift check is skipped with an informational message; run apm install to warm the cache before the next apm audit --ci run."

Test Coverage Expert

• [recommended] No test asserts --ci exit code is 0 on CacheMissError
  The three new unit tests confirm the CheckResult shape, and test_e10 covers bare audit stderr. But no test exercises the full --ci command path with a CacheMissError and asserts exit_code == 0.
  Proof (missing at integration-with-fixtures): tests/integration/test_drift_check.py::test_e11_ci_audit_exits_zero_on_cache_miss -- proves: apm audit --ci exits 0 (non-blocking) when drift cache is cold [devx, governed-by-policy]

• [nit] startswith guard has no boundary test for non-skip passed=True messages
  No unit test asserts that a CheckResult with passed=True but a message not starting with 'drift skipped' does not accidentally trigger the new elif branch.
  Proof (missing at unit): tests/unit/policy/test_ci_checks.py::test_non_skip_message_does_not_trigger_skip_branch

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

Generated by PR Review Panel for issue #1289 · ● 3.1M · ◷

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.