Implementation Plan: Low-Severity Test Suite Fixes (groupH)

[Trecek]

Summary

Four targeted test-quality improvements across three test files:

1. Req 2.2 — Delete the duplicate test_model_default_consistent_with_yaml test in TestReleaseReadinessConfig (tests/config/test_config.py:653). The identical assertion (ModelConfig().default == "sonnet") is already covered by TestDefaultConfig.test_default_model_config (line 35). The load_config round-trip variant at line 204 is kept as it exercises a distinct code path.

2. Req 3.1 — Strengthen the test_returns_bound_logger test in tests/core/test_logging.py. Replace the three callable(logger.*) assertions with a structural isinstance(logger, BoundLoggerLazyProxy) check plus a clarifying comment. callable() returns True for any structlog lazy proxy attribute via __getattr__—it does not confirm that logger is the expected type.

3. Req 3.2 — Remove two hasattr guards in tests/config/test_config.py. if hasattr(cfg, "field"): before assert cfg.field == ... is a weaker assertion: it passes silently when the field is absent. Replace with direct attribute access so a missing field raises AttributeError and fails the test rather than silently passing.

4. Req 3.8 — Rename the local _make_result helper in tests/execution/test_session.py to _make_session_result. The local helper shadows the conftest-level _make_result with different default parameters. Renaming removes the ambiguity.

Architecture Impact

Operational Diagram

%%{init: {'flowchart': {'nodeSpacing': 50, 'rankSpacing': 60, 'curve': 'basis'}}}%%
flowchart TB
    %% CLASS DEFINITIONS %%
    classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;

    subgraph Tasks ["TASK AUTOMATION"]
        TA["task test-all<br/>━━━━━━━━━━<br/>lint + pytest -n 4<br/>quality gate"]
        TC_CMD["task test-check<br/>━━━━━━━━━━<br/>TEST_RESULT=PASS/FAIL<br/>automation gate"]
    end

    subgraph TestSuite ["MODIFIED TEST FILES"]
        TC["● tests/config/test_config.py<br/>━━━━━━━━━━<br/>AutomationConfig fields<br/>LoggingConfig, LinuxTracingConfig"]
        TL["● tests/core/test_logging.py<br/>━━━━━━━━━━<br/>get_logger() type contract<br/>BoundLoggerLazyProxy assertion"]
        TS["● tests/execution/test_session.py<br/>━━━━━━━━━━<br/>ClaudeSessionResult extraction<br/>_make_session_result helper"]
    end

    subgraph Config ["CONFIGURATION (read-only at runtime)"]
        AC["AutomationConfig<br/>━━━━━━━━━━<br/>Root config aggregate<br/>dynaconf-backed"]
        LC["LoggingConfig<br/>━━━━━━━━━━<br/>level: INFO<br/>json_output: null"]
        LTC["LinuxTracingConfig<br/>━━━━━━━━━━<br/>enabled: true<br/>proc_interval: 5.0"]
        MC["ModelConfig<br/>━━━━━━━━━━<br/>default: sonnet"]
    end

    subgraph Logging ["LOGGING SYSTEM"]
        GL["get_logger(name)<br/>━━━━━━━━━━<br/>→ BoundLoggerLazyProxy<br/>lazy structlog binding"]
        CL["configure_logging()<br/>━━━━━━━━━━<br/>level + json_output<br/>routes to stderr only"]
    end

    subgraph ConfigSources ["CONFIGURATION HIERARCHY (lowest → highest)"]
        D["defaults.yaml<br/>━━━━━━━━━━<br/>bundled package defaults"]
        U["~/.autoskillit/config.yaml<br/>━━━━━━━━━━<br/>user-level overrides"]
        P[".autoskillit/config.yaml<br/>━━━━━━━━━━<br/>project-level overrides"]
        S[".autoskillit/.secrets.yaml<br/>━━━━━━━━━━<br/>github.token only"]
        E["AUTOSKILLIT_SECTION__KEY<br/>━━━━━━━━━━<br/>env vars (highest priority)"]
    end

    D --> U --> P --> S --> E --> AC
    AC --> LC
    AC --> LTC
    AC --> MC
    LC -.->|"configures"| CL
    GL --> CL
    TA --> TC
    TA --> TL
    TA --> TS
    TC -.->|"asserts fields"| AC
    TL -.->|"asserts type"| GL
    TC_CMD --> TC

    class TA,TC_CMD cli;
    class TC,TL,TS handler;
    class AC,LC,LTC,MC stateNode;
    class GL,CL phase;
    class D,U,P,S,E output;

Loading

Color Legend:

Color	Category	Description
Dark Blue	CLI	Task automation commands
Orange	Handler (●)	Modified test files
Teal	State	Configuration dataclasses
Purple	Phase	Logging system components
Dark Teal	Config Sources	Configuration resolution layers

Implementation Plan

Plan file: /home/talon/projects/autoskillit-runs/impl-groupH-20260328-084625-409744/.autoskillit/temp/make-plan/groupH_low_severity_fixes_plan_2026-03-28_084625.md

🤖 Generated with Claude Code via AutoSkillit

Token Usage Summary

Step	input	output	cached	count	time
group	2.7k	25.0k	721.4k	1	9m 3s
plan	462	84.7k	14.8M	9	41m 2s
verify	98	55.7k	3.3M	6	18m 12s
implement	536	45.2k	10.3M	9	25m 25s
fix	60	10.0k	1.6M	2	11m 53s
Total	3.8k	220.7k	30.7M		1h 45m

[Trecek]

AutoSkillit PR Review — Verdict: changes_requested (4 blocking issues found)

[Trecek]

[warning] tests: Removing assert hasattr(cfg, 'logging') leaves no check that the attribute exists before accessing it. If the field is ever removed or renamed, the test will raise AttributeError rather than a clean assertion failure, hiding the real contract violation.

[Trecek]

Investigated — this is intentional. The hasattr guard was removed per Req 3.2: direct attribute access (cfg.logging.level) already tests existence — if 'logging' is absent, Python raises AttributeError which is a clear, informative test failure. The hasattr check was redundant and could silently pass in edge cases. See commit 5613a40.

[Trecek]

[warning] tests: Removing assert hasattr(cfg, 'linux_tracing') leaves no check that the attribute exists before accessing it. A missing field produces AttributeError instead of a meaningful assertion failure.

[Trecek]

Investigated — this is intentional. The hasattr guard was removed per Req 3.2: direct attribute access (cfg.linux_tracing.enabled) already tests existence — if 'linux_tracing' is absent, Python raises AttributeError which clearly identifies the missing field. The hasattr guard was redundant. See commit 5613a40.

[Trecek]

[warning] tests: test_model_default_consistent_with_yaml was deleted entirely. This test enforced that the ModelConfig dataclass default and defaults.yaml stay in sync — a cross-layer contract that no remaining test covers. Deleting it without a replacement removes a meaningful regression guard.

[Trecek]

Investigated — this is intentional. The reviewer's claim that 'no remaining test covers' this contract is factually incorrect. TestDefaultConfig.test_default_model_config (line ~35) already asserts AutomationConfig().model.default == 'sonnet', which exercises ModelConfig through AutomationConfig composition — the same cross-layer contract. The deleted test was a duplicate per Req 2.2. See commit 5613a40.

[Trecek]

[warning] tests: Asserting isinstance(logger, BoundLoggerLazyProxy) couples the test to a private structlog internal class (structlog._config). If structlog renames this class in a future version, the test will break even though get_logger is still correct. Prefer checking the public API (e.g. hasattr(logger, 'info')) or a stable public type.

[Trecek]

Investigated — this is intentional. The reviewer's suggested alternative (hasattr(logger, 'info')) is weaker — it passes for stdlib logging.Logger instances too, defeating the purpose of verifying structlog is in use. The old callable() assertions were vacuously True due to BoundLoggerLazyProxy.getattr delegation (see the comment in the test). BoundLoggerLazyProxy is already imported by the sibling canary test in this file (commit c6b238e), and structlog<26.0 is pinned in pyproject.toml. The coupling concern is already mitigated. See commit 5613a40.

[Trecek]

[info] cohesion: Rename from _make_result to _make_session_result improves specificity, but the alias _flush_structlog_proxy_caches as _flush_logger_proxy_caches on L024 follows the opposite convention — importing under a shorter name. These two helper naming styles are asymmetric within the same file.

[Trecek]

AutoSkillit review found 4 blocking issues. See inline comments.

Verdict: changes_requested

4 warnings require attention before merge:

• tests/config/test_config.py L418: redundant hasattr removal leaves weaker failure mode
• tests/config/test_config.py L456: same issue for linux_tracing field
• tests/config/test_config.py L648: test_model_default_consistent_with_yaml deleted without replacement — cross-layer contract now uncovered
• tests/core/test_logging.py L26: isinstance(logger, BoundLoggerLazyProxy) binds test to private structlog internal class

1 informational finding (non-blocking):

• tests/execution/test_session.py L27: asymmetric naming conventions for helper aliases