Implementation Plan: T5-P5-A10-WP1 — CLI Config-Acceptance Probe (Check 34)#4140
Conversation
Adds a new doctor check that probes whether the backend CLI accepts a minimal TOML config. Writes a probe.toml via atomic_write to a tempdir, invokes the CLI with -c override and --version, and reports OK/WARNING based on the exit code. Mirrors the _check_codex_version gating pattern (skip when no backend, no version_check_command, or CLI unavailable). Wired as Check 34 in run_doctor(). AGENTS.md check count updated to 34.
…ount test Adds TestCheckCliConformanceProbes class with 6 tests: - test_skip_for_none_backend - test_skip_for_backend_without_version_check_command - test_skip_when_cli_unavailable - test_skip_when_cli_times_out - test_ok_when_config_accepted - test_warning_when_config_rejected Subprocess.run is monkeypatched and a stateful callable distinguishes the availability gate (first call) from the config probe (second call). Renames test_doctor_check_count_is_40 to test_doctor_check_count_is_41 and updates the assertion + comment block to reflect the new check.
- Update prose claim to reflect fleet + backend checks active max - Fix file path reference: cli/_doctor.py -> cli/doctor/__init__.py - Add rows 29-34 to the doctor checks table covering fleet state schema, codex version, script(1) binary, MCP timeouts, codex graduation, and CLI conformance.
Cross-package submodule import violated test_no_cross_package_submodule_imports. All other callers use 'from autoskillit.core import atomic_write'.
…inding in _check_cli_conformance_probes - Replace .split() with shlex.split() for robust tokenization of version_check_command - Replace hardcoded 'gpt-4o' probe model with backend-agnostic 'test-model' - Initialize result=None before the subprocess try block and guard returncode access Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…test coverage - test_skip_when_cli_unavailable: patch on module under test instead of global subprocess - test_skip_when_cli_times_out: use stateful fake (succeed call 1, raise TimeoutExpired call 2) so the config-probe timeout handler is actually exercised; tighten assertion to 'timed out' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bering scheme Group checks 1-17 + 18-23 as '23 numbered' to match installation.md's perspective; note that 2e and 7c are internal-only sub-checks not documented in the user-visible table. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Checks 30-34 (backend runtime probes) run unconditionally — they are not fleet-gated. Base count is 33 (23 numbered + 5 lettered + 5 backend runtime); up to 39 with the fleet feature enabled (adds checks 24-29). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: approved_with_comments
|
|
||
| Doctor runs 28 checks (23 numbered + 5 lettered sub-checks: `2b`, `2c`, `2d`, `4b`, `7b`); up to 33 with the fleet feature enabled. | ||
| Enumerated by `run_doctor` in `src/autoskillit/cli/_doctor.py`: | ||
| Doctor runs 33 checks (23 numbered + 5 lettered sub-checks: `2b`, `2c`, `2d`, `4b`, `7b` + 5 backend runtime probes, checks 30–34); up to 39 with the fleet feature enabled. |
There was a problem hiding this comment.
[warning] cohesion: prose says "Doctor runs 33 checks" but src/autoskillit/cli/doctor/AGENTS.md says "34 checks" — one count must be wrong. Verify whether the base (non-fleet) total is 33 (23 numbered + 5 lettered + 5 backend runtime probes 30–34) or 34, and update both files to agree.
| # doctor/ | ||
|
|
||
| Diagnostic health checks for the autoskillit installation (33 checks). | ||
| Diagnostic health checks for the autoskillit installation (34 checks). |
There was a problem hiding this comment.
[warning] cohesion: AGENTS.md says 34 checks but docs/installation.md prose says 33. These two authoritative sources give different totals — whichever is wrong must be corrected.
| {"returncode": 0, "stdout": "codex 0.130.0\n", "stderr": ""}, | ||
| )() | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", _fake_run) |
There was a problem hiding this comment.
[warning] tests: monkeypatch.setattr(subprocess, "run", _fake_run) patches the global subprocess module object instead of the module-scoped path "autoskillit.cli.doctor._doctor_runtime.subprocess.run" used by every other test in this class (L727, L750). This patch may not intercept the production code's call, making test_ok_when_config_accepted unreliable. Change to monkeypatch.setattr("autoskillit.cli.doctor._doctor_runtime.subprocess.run", _fake_run).
| {"returncode": 1, "stdout": "", "stderr": "unknown flag"}, | ||
| )() | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", _fake_run) |
There was a problem hiding this comment.
[warning] tests: Same monkeypatch scope bug as L774 — monkeypatch.setattr(subprocess, "run", _fake_run) patches globally instead of "autoskillit.cli.doctor._doctor_runtime.subprocess.run". The production code's subprocess.run calls may not be intercepted, and the broad patch risks interference with concurrent tests under pytest-xdist.
| monkeypatch.setattr(subprocess, "run", _fake_run) | ||
| result = _check_cli_conformance_probes(backend=CodexBackend()) | ||
| assert result.severity == Severity.WARNING | ||
| assert "rejected" in result.message.lower() or "exit 1" in result.message |
There was a problem hiding this comment.
[warning] tests: Weak disjunctive assertion — assert "rejected" in result.message.lower() or "exit 1" in result.message lets either clause pass independently, masking a regression where the exit code is dropped from the message. Assert both conditions separately: assert "rejected" in result.message.lower() and assert "exit 1" in result.message.
|
|
||
| cli_argv = shlex.split(backend.capabilities.version_check_command) | ||
| try: | ||
| subprocess.run(cli_argv, capture_output=True, text=True, timeout=5) |
There was a problem hiding this comment.
[warning] defense: OSError from the version-check subprocess.run is silently swallowed with a generic "CLI unavailable" message, discarding diagnostic context. A permission error or bad binary is indistinguishable from "not installed". Log the exception at debug level before returning so operators can distinguish failure modes.
Trecek
deleted the
t5-p5-a10-wp1-implement-a-lightweight-gated-cli-config-accep/4028
branch
1 participant
Summary
Add
_check_cli_conformance_probestosrc/autoskillit/cli/doctor/_doctor_runtime.pyas a new doctor check (Check 34) that writes a minimal TOML config to a temp directory, invokes the backend CLI with a config override flag, and reports whether the CLI accepted the config. Follows the exact gating pattern from_check_codex_version: skip when backend isNoneorversion_check_commandis empty, skip when CLI is unavailable or times out.Check numbering note: The issue references "Check 33" but the current codebase already assigns Check 33 to
_check_codex_graduation(line 217–218 of__init__.py). This plan assigns the new check as Check 34, appended after Check 33.Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260628-044315-253111/.autoskillit/temp/make-plan/T5-P5-A10-WP1_cli_conformance_probes_plan_2026-06-28_050100.mdCloses #4028
🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary
* Step used a non-Anthropic provider; caching behavior may differ.
Token Efficiency
Model Usage Breakdown