Rectify: Config Schema Contamination & Misplaced Secrets Immunity — PART A ONLY by Trecek · Pull Request #476 · TalonT-Org/AutoSkillit

Trecek · 2026-03-22T08:32:59Z

Summary

The config system had two crash-inducing architectural weaknesses: _log_secret_scan_bypass wrote safety.secret_scan_bypass_accepted into config.yaml — a key not in SafetyConfig — causing every subsequent load_config() call to raise ConfigSchemaError after a bypass-accepted autoskillit init. Additionally, when github.token appeared in config.yaml, the error message provided no actionable fix steps (no exact YAML to copy, no removal instruction). A third structural gap: _SECRETS_ONLY_KEYS was manually maintained with no test enforcing completeness against the config dataclasses.

The fixes are threefold: (1) route bypass timestamps to .autoskillit/.state.yaml (never schema-validated), eliminating the self-inflicted crash; (2) make validate_layer_keys self-diagnosing for misplaced secrets — the error now includes the exact YAML block and a removal instruction; (3) add test_secrets_only_keys_completeness as a structural guard that fails if any secret-typed field in GitHubConfig is absent from _SECRETS_ONLY_KEYS. Part B (write-time config validation gateway and autoskillit doctor misplaced-secrets check) is included in this branch.

Architecture Impact

Error/Resilience Diagram

%%{init: {'flowchart': {'nodeSpacing': 45, 'rankSpacing': 55, 'curve': 'basis'}}}%%
flowchart TB
    classDef terminal 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 newComponent fill:#2e7d32,stroke:#81c784,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 gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000;

    LOAD_CONFIG([load_config])
    WRITE_CONFIG([● write_config_layer])

    subgraph LayerPipeline ["LAYER LOAD PIPELINE (_make_dynaconf)"]
        direction TB
        DEFAULTS["defaults.yaml<br/>━━━━━━━━━━<br/>skip validation"]
        USER_CFG["~/.autoskillit/config.yaml<br/>━━━━━━━━━━<br/>validated, secrets disallowed"]
        PROJ_CFG[".autoskillit/config.yaml<br/>━━━━━━━━━━<br/>validated, secrets disallowed"]
        SECRETS_LAYER[".autoskillit/.secrets.yaml<br/>━━━━━━━━━━<br/>validated, secrets allowed"]
    end

    subgraph ValidationGate ["● VALIDATION GATE (validate_layer_keys)"]
        VLK{"validate<br/>layer keys"}
        ERR_TOP["unknown top key<br/>━━━━━━━━━━<br/>difflib suggestion"]
        ERR_SECRET["● secret in config.yaml<br/>━━━━━━━━━━<br/>exact YAML fix block<br/>removal instruction"]
        ERR_SUB["unknown sub-key<br/>━━━━━━━━━━<br/>difflib suggestion"]
    end

    subgraph WriteGate ["● WRITE-TIME GATE (write_config_layer)"]
        WCL["● write_config_layer<br/>━━━━━━━━━━<br/>validates BEFORE write"]
        WCL_FAIL["write blocked<br/>━━━━━━━━━━<br/>no file touched"]
        WCL_OK["atomic write<br/>━━━━━━━━━━<br/>schema-valid only"]
    end

    subgraph StateIsolation ["● STATE ISOLATION (_log_secret_scan_bypass)"]
        BYPASS["● _log_secret_scan_bypass<br/>━━━━━━━━━━<br/>bypass timestamp"]
        STATE_YAML[".autoskillit/.state.yaml<br/>━━━━━━━━━━<br/>never validated<br/>internal state"]
    end

    subgraph DoctorDiag ["● DOCTOR DIAGNOSTIC (_check_config_layers_for_secrets)"]
        DOCTOR["● _check_config_layers_for_secrets<br/>━━━━━━━━━━<br/>scans user + project config.yaml<br/>returns DoctorResult, never raises"]
        DOC_OK["DoctorResult.OK<br/>━━━━━━━━━━<br/>no secrets in config layers"]
        DOC_ERR["DoctorResult.ERROR<br/>━━━━━━━━━━<br/>actionable YAML fix<br/>+ removal step"]
    end

    T_SCHEMA_ERR([ConfigSchemaError])
    T_LOADED([AutomationConfig loaded])
    T_WRITTEN([file written])

    LOAD_CONFIG --> DEFAULTS
    DEFAULTS --> USER_CFG
    USER_CFG --> VLK
    VLK -->|"pass"| PROJ_CFG
    PROJ_CFG --> VLK
    VLK -->|"pass"| SECRETS_LAYER
    SECRETS_LAYER --> VLK
    VLK -->|"pass — all layers valid"| T_LOADED

    VLK -->|"unknown top key"| ERR_TOP
    VLK -->|"secret in non-secrets layer"| ERR_SECRET
    VLK -->|"unknown sub-key"| ERR_SUB
    ERR_TOP --> T_SCHEMA_ERR
    ERR_SECRET --> T_SCHEMA_ERR
    ERR_SUB --> T_SCHEMA_ERR

    WRITE_CONFIG --> WCL
    WCL -->|"validation fails"| WCL_FAIL
    WCL -->|"schema valid"| WCL_OK
    WCL_FAIL --> T_SCHEMA_ERR
    WCL_OK --> T_WRITTEN

    BYPASS --> STATE_YAML

    DOCTOR -->|"clean"| DOC_OK
    DOCTOR -->|"violation found"| DOC_ERR

    class LOAD_CONFIG,WRITE_CONFIG,T_SCHEMA_ERR,T_LOADED,T_WRITTEN terminal;
    class DEFAULTS stateNode;
    class USER_CFG,PROJ_CFG,SECRETS_LAYER handler;
    class VLK,WCL detector;
    class ERR_TOP,ERR_SUB,WCL_FAIL,DOC_ERR gap;
    class ERR_SECRET,BYPASS,DOCTOR,WCL newComponent;
    class STATE_YAML,WCL_OK,DOC_OK output;

Color Legend:

Color	Category	Description
Dark Blue	Terminal	Entry points and final states
Orange	Handler	Config layer paths (validated)
Dark Teal	State/Output	Defaults layer, output states
Red	Detector	Validation gates
Yellow	Gap	Error paths: unknown keys, write blocked, doctor violation
Green	New/Modified	`ERR_SECRET` (actionable message), `_log_secret_scan_bypass`, `_check_config_layers_for_secrets`, `write_config_layer`

State Lifecycle Diagram

%%{init: {'flowchart': {'nodeSpacing': 45, 'rankSpacing': 55, 'curve': 'basis'}}}%%
flowchart TB
    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 newComponent fill:#2e7d32,stroke:#81c784,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 gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000;

    subgraph StorageLocations ["STORAGE LOCATIONS — MUTATION RULES"]
        direction LR
        DEFAULTS_YAML["config/defaults.yaml<br/>━━━━━━━━━━<br/>DEFAULTS<br/>read-only baseline<br/>never validated"]
        CONFIG_YAML["● .autoskillit/config.yaml<br/>━━━━━━━━━━<br/>SCHEMA_LOCKED<br/>secrets forbidden<br/>validated on every load + write"]
        SECRETS_YAML[".autoskillit/.secrets.yaml<br/>━━━━━━━━━━<br/>SECRETS_ONLY<br/>secrets allowed here only<br/>validated on every load"]
        STATE_YAML["● .autoskillit/.state.yaml<br/>━━━━━━━━━━<br/>INTERNAL_STATE<br/>no schema enforcement<br/>never passed to validate_layer_keys"]
    end

    subgraph SecretContract ["● SECRET KEY CONTRACT (_SECRETS_ONLY_KEYS)"]
        SOK["● _SECRETS_ONLY_KEYS<br/>━━━━━━━━━━<br/>frozenset{'github.token'}<br/>re-exported from config/__init__.py<br/>used by validate_layer_keys + doctor"]
        SOK_TEST["● test_secrets_only_keys_completeness<br/>━━━━━━━━━━<br/>structural guard: asserts every<br/>token/key/secret field in GitHubConfig<br/>is in _SECRETS_ONLY_KEYS"]
    end

    subgraph LoadGate ["LOAD-TIME SCHEMA GATE (validate_layer_keys)"]
        VLK{"validate<br/>layer keys"}
        VLK_SCHEMA["● schema check<br/>━━━━━━━━━━<br/>unknown top/sub keys<br/>→ ConfigSchemaError"]
        VLK_SECRET["● secrets-placement check<br/>━━━━━━━━━━<br/>SECRETS_ONLY key in non-secrets layer<br/>→ actionable ConfigSchemaError"]
    end

    subgraph WriteGate2 ["● WRITE-TIME SCHEMA GATE (write_config_layer)"]
        WCL2["● write_config_layer<br/>━━━━━━━━━━<br/>validates before write<br/>schema-valid only"]
        WCL_BLOCK["write blocked<br/>━━━━━━━━━━<br/>no file modified"]
    end

    subgraph BypassRoute ["● BYPASS STATE ISOLATION (_log_secret_scan_bypass)"]
        BYPASS_FN["● _log_secret_scan_bypass<br/>━━━━━━━━━━<br/>writes bypass timestamp to .state.yaml<br/>never touches config.yaml"]
    end

    subgraph DoctorGate2 ["● DOCTOR GATE (_check_config_layers_for_secrets)"]
        DOCTOR2["● _check_config_layers_for_secrets<br/>━━━━━━━━━━<br/>reads _SECRETS_ONLY_KEYS<br/>scans user + project config.yaml"]
        DOC_CLEAN["DoctorResult.OK"]
        DOC_CORRUPT["DoctorResult.ERROR<br/>━━━━━━━━━━<br/>contract violation detected<br/>actionable fix guidance"]
    end

    T_SCHEMA_ERR2([ConfigSchemaError])
    T_LOADED2([AutomationConfig loaded])
    T_WRITTEN2([file written])

    DEFAULTS_YAML -->|"merged first, no validation"| VLK
    CONFIG_YAML -->|"validated, secrets forbidden"| VLK
    SECRETS_YAML -->|"validated, is_secrets=True"| VLK
    VLK -->|"pass"| T_LOADED2
    VLK -->|"unknown key"| VLK_SCHEMA
    VLK -->|"secret in config.yaml"| VLK_SECRET
    VLK_SCHEMA --> T_SCHEMA_ERR2
    VLK_SECRET --> T_SCHEMA_ERR2

    WCL2 -->|"schema valid"| T_WRITTEN2
    WCL2 -->|"validation fails"| WCL_BLOCK
    WCL_BLOCK --> T_SCHEMA_ERR2

    BYPASS_FN -->|"writes to"| STATE_YAML

    SOK -->|"drives"| VLK_SECRET
    SOK -->|"drives"| DOCTOR2
    SOK_TEST -.->|"prevents drift"| SOK

    DOCTOR2 -->|"clean"| DOC_CLEAN
    DOCTOR2 -->|"violation"| DOC_CORRUPT

    class DEFAULTS_YAML stateNode;
    class CONFIG_YAML,SECRETS_YAML handler;
    class STATE_YAML,T_LOADED2,T_WRITTEN2 newComponent;
    class VLK detector;
    class VLK_SCHEMA gap;
    class VLK_SECRET,WCL2,BYPASS_FN,DOCTOR2,SOK,SOK_TEST newComponent;
    class WCL_BLOCK,DOC_CORRUPT gap;
    class DOC_CLEAN output;
    class T_SCHEMA_ERR2,T_LOADED2,T_WRITTEN2 terminal;

Color Legend:

Color	Category	Description
Dark Teal	Defaults	Baseline layer (never validated)
Orange	Config/Secrets	Validated YAML storage locations
Green	New/Modified	`write_config_layer`, `_log_secret_scan_bypass`, `_SECRETS_ONLY_KEYS`, doctor check, `.state.yaml`
Red	Detector	`validate_layer_keys` gate
Yellow	Gap	Error paths: unknown key, write blocked, doctor violation
Dark Blue	Terminal	Entry/exit states and `ConfigSchemaError`

Operational Diagram

%%{init: {'flowchart': {'nodeSpacing': 45, 'rankSpacing': 55, 'curve': 'basis'}}}%%
flowchart TB
    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 newComponent fill:#2e7d32,stroke:#81c784,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 gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000;

    subgraph InitCmd ["autoskillit init (● modified)"]
        direction TB
        INIT_CMD["autoskillit init<br/>━━━━━━━━━━<br/>interactive project setup"]
        SCAN_CHECK["_check_secret_scanning<br/>━━━━━━━━━━<br/>detect pre-commit scanner<br/>or prompt for bypass consent"]
        BYPASS_OK["bypass accepted<br/>━━━━━━━━━━<br/>user typed exact phrase"]
        NO_BYPASS["scanner found / no consent<br/>━━━━━━━━━━<br/>normal path / abort"]
        LOG_BYPASS["● _log_secret_scan_bypass<br/>━━━━━━━━━━<br/>writes bypass timestamp<br/>to .state.yaml ONLY"]
        WRITE_CFG["write_config_layer<br/>━━━━━━━━━━<br/>writes user config to<br/>.autoskillit/config.yaml<br/>(schema-validated)"]
        REGISTER["_register_mcp_server<br/>━━━━━━━━━━<br/>writes ~/.claude.json"]
    end

    subgraph DoctorCmd ["autoskillit doctor (● modified)"]
        direction TB
        DOCTOR_CMD["autoskillit doctor<br/>━━━━━━━━━━<br/>runs 9 health checks<br/>always all, no early exit"]
        CHK1["Check 1–4: version, scanner,<br/>config existence, MCP server<br/>━━━━━━━━━━<br/>existing checks"]
        CHK4B["● Check 4b: _check_config_layers_for_secrets<br/>━━━━━━━━━━<br/>scans ~/.autoskillit/config.yaml<br/>+ .autoskillit/config.yaml<br/>for _SECRETS_ONLY_KEYS violations"]
        CHK5N["Check 5–9: hooks, git,<br/>worktree, recipe, branch<br/>━━━━━━━━━━<br/>existing checks"]
        DOCTOR_OK["All checks OK<br/>━━━━━━━━━━<br/>Severity.OK for each"]
        DOCTOR_ERR["● Severity.ERROR<br/>━━━━━━━━━━<br/>Shows: dotted key path,<br/>exact YAML fix block,<br/>removal instruction"]
    end

    subgraph ConfigFiles ["CONFIGURATION FILES"]
        direction TB
        USER_CFG2["~/.autoskillit/config.yaml<br/>━━━━━━━━━━<br/>user-level config<br/>SCHEMA_LOCKED<br/>secrets forbidden"]
        PROJ_CFG2[".autoskillit/config.yaml<br/>━━━━━━━━━━<br/>project-level config<br/>SCHEMA_LOCKED<br/>secrets forbidden"]
        SECRETS_FILE2[".autoskillit/.secrets.yaml<br/>━━━━━━━━━━<br/>secret keys here only<br/>e.g. github.token"]
        STATE_FILE2["● .autoskillit/.state.yaml<br/>━━━━━━━━━━<br/>internal operational state<br/>bypass timestamp<br/>never schema-validated"]
    end

    subgraph LoadConfig2 ["load_config (called by all commands)"]
        LC2["● load_config<br/>━━━━━━━━━━<br/>merges all layers<br/>validates each (except defaults)"]
        LC_ERR2["● ConfigSchemaError<br/>━━━━━━━━━━<br/>actionable: exact YAML fix<br/>+ removal instruction"]
    end

    INIT_CMD --> SCAN_CHECK
    SCAN_CHECK -->|"bypass phrase matched"| BYPASS_OK
    SCAN_CHECK -->|"scanner OK / no consent"| NO_BYPASS
    BYPASS_OK --> LOG_BYPASS
    LOG_BYPASS -->|"writes bypass timestamp"| STATE_FILE2
    NO_BYPASS --> WRITE_CFG
    WRITE_CFG -->|"schema-validated write"| PROJ_CFG2
    WRITE_CFG --> REGISTER

    DOCTOR_CMD --> CHK1
    CHK1 --> CHK4B
    CHK4B -->|"reads"| USER_CFG2
    CHK4B -->|"reads"| PROJ_CFG2
    CHK4B -->|"clean"| DOCTOR_OK
    CHK4B -->|"violation found"| DOCTOR_ERR
    CHK4B --> CHK5N

    LC2 -->|"validated layers"| USER_CFG2
    LC2 -->|"validated layers"| PROJ_CFG2
    LC2 -->|"validated layers"| SECRETS_FILE2
    LC2 -->|"secret in config.yaml"| LC_ERR2

    class INIT_CMD,DOCTOR_CMD cli;
    class SCAN_CHECK,NO_BYPASS,WRITE_CFG,REGISTER,CHK1,CHK5N handler;
    class BYPASS_OK,LOG_BYPASS,CHK4B,DOCTOR_ERR,LC_ERR2,LC2 newComponent;
    class USER_CFG2,PROJ_CFG2,SECRETS_FILE2 phase;
    class STATE_FILE2 newComponent;
    class DOCTOR_OK,LC2 output;
    class LC2 detector;

Color Legend:

Color	Category	Description
Dark Blue	CLI	Entry-point commands
Orange	Handler	Existing processing steps
Green	New/Modified	`● _log_secret_scan_bypass`, `● _check_config_layers_for_secrets`, `● .state.yaml`, `● ConfigSchemaError` (actionable)
Purple	Config	Configuration file layers
Red	Detector	`load_config` schema validation gate
Dark Teal	Output	Success results

Implementation Plan

Plan file: /home/talon/projects/autoskillit-runs/remediation-20260322-002721-721157/temp/rectify/rectify_config-schema-immunity_2026-03-22_000000_part_a.md

🤖 Generated with Claude Code via AutoSkillit

…t A) - SS-INIT-5: update to assert .state.yaml (not config.yaml) persists bypass - SS-INIT-ROUNDTRIP: round-trip test — bypass-accepted init then load_config must not raise - SS-INIT-STATE-FILE: _log_secret_scan_bypass must write to .state.yaml only - SEC-3b: validate_layer_keys misplaced-secret error must include exact fix guidance - SEC-3c: user-level ~/.autoskillit/config.yaml also rejects misplaced secrets - _SECRETS_ONLY_KEYS completeness contract test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…error guidance Change 2A: _log_secret_scan_bypass now writes to .autoskillit/.state.yaml instead of config.yaml. The .state.yaml file is never passed through validate_layer_keys, so the safety.secret_scan_bypass_accepted timestamp no longer contaminates the schema-validated config layer. Previously, every subsequent load_config call after a bypass-accepted init raised ConfigSchemaError. Change 2B: validate_layer_keys error for misplaced _SECRETS_ONLY_KEYS now includes the dotted key path, the exact YAML block to add to .secrets.yaml, and a removal instruction — making the error copy-paste actionable rather than a bare detection message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…rets check (Part B)

…ck (Part B)

…odule import violation

Trecek

AutoSkillit PR Review — Verdict: changes_requested

Trecek · 2026-03-22T08:40:30Z

src/autoskillit/cli/_init_helpers.py

-                                config_data, default_flow_style=False, allow_unicode=True
-                            ),
-                        )
+                        write_config_layer(config_path, config_data)


[critical] bugs: write_config_layer raises ConfigSchemaError if config_data contains schema-invalid keys, but the surrounding try/except only catches (OSError, YAMLError). A ConfigSchemaError will escape unhandled and propagate up the call stack, crashing the caller instead of printing the warning message. Fix: add ConfigSchemaError to the except clause.

Investigated — this is intentional. The reviewer's diff_hunk describes a hypothetical write_config_layer(config_data, config_path, *, validate: bool = True) -> bool signature that does not exist. The actual implementation at config/settings.py intentionally raises ConfigSchemaError before touching the filesystem (documented contract: 'Raises ConfigSchemaError before touching the file'). The call at L391 writes known-valid schema data. No change warranted for this finding.

src/autoskillit/cli/_init_helpers.py

src/autoskillit/cli/_doctor.py

Trecek · 2026-03-22T08:40:30Z

src/autoskillit/cli/_doctor.py

+    Scans the user-level and project-level config.yaml files for any keys
+    that belong only in .secrets.yaml. Reports ERROR with exact fix guidance.
+    """
+    from autoskillit.config import _SECRETS_ONLY_KEYS


[warning] cohesion: _check_config_layers_for_secrets uses lazy (inside-function) imports for _SECRETS_ONLY_KEYS and load_yaml, while _init_helpers.py imports them at module top-level. Mixing lazy and top-level import styles across files in the same PR creates inconsistency.

Investigated — this is intentional. Deferred function-level imports are the established pattern throughout _doctor.py (e.g., run_doctor() at line 236 uses deferred imports for _clear_plugin_cache). The imports at lines 189–190 follow this module-wide convention used to avoid circular import chains and reduce startup cost. Moving them to module level would break from the module's own consistent style.

tests/cli/test_doctor.py

tests/cli/test_init.py

Trecek · 2026-03-22T08:40:30Z

tests/cli/test_init.py

+
+
+# SS-INIT-ROUNDTRIP
+def test_bypass_accepted_init_load_config_round_trip(


[warning] tests: SS-INIT-ROUNDTRIP calls cli.init() which exercises many side effects. If any unrelated init step fails under tmp_path isolation, the test fails for the wrong reason. Consider a narrower unit test that calls _log_secret_scan_bypass directly then load_config.

Investigated — this is intentional. SS-INIT-ROUNDTRIP's docstring explicitly states it is a regression guard for _log_secret_scan_bypass writing safety.secret_scan_bypass_accepted to config.yaml — a bug undetected in prior tests because they never called load_config. The full cli.init() call is deliberate: the regression only manifests through the complete init path, and isolating to just the config-write part would miss the original bug.

tests/config/test_config.py

Trecek · 2026-03-22T08:40:56Z

src/autoskillit/cli/_init_helpers.py

-                                config_data, default_flow_style=False, allow_unicode=True
-                            ),
-                        )
+                        write_config_layer(config_path, config_data)


[critical] bugs: write_config_layer raises ConfigSchemaError if config_data contains schema-invalid keys, but the surrounding try/except only catches (OSError, YAMLError). A ConfigSchemaError will escape unhandled, crashing the caller instead of printing the warning. Fix: add ConfigSchemaError to the except clause at L392.

Trecek · 2026-03-22T08:40:57Z

src/autoskillit/cli/_init_helpers.py

                autoskillit_dir = project_dir / ".autoskillit"
                autoskillit_dir.mkdir(exist_ok=True)
-                atomic_write(config_path, f"github:\n  default_repo: '{github_repo}'\n")
+                write_config_layer(config_path, {"github": {"default_repo": github_repo}})


[critical] bugs: write_config_layer at L397 is outside the try/except block (which ends at L393). Any ConfigSchemaError, OSError, or YAMLError it raises will be completely unhandled. Fix: wrap this call in a try/except (OSError, YAMLError, ConfigSchemaError) block.

Trecek · 2026-03-22T08:40:58Z

src/autoskillit/cli/_doctor.py

+    for config_path in config_paths:
+        if not config_path.is_file():
+            continue
+        try:


[warning] arch: _check_config_layers_for_secrets re-implements the secrets-placement scan (iterating top_key/sub_key, checking _SECRETS_ONLY_KEYS, building fix guidance) entirely in the CLI layer. The same logic already lives in validate_layer_keys in config/settings.py, creating a second source of truth.

Trecek · 2026-03-22T08:40:59Z

src/autoskillit/cli/_doctor.py

+            continue
+        try:
+            data = load_yaml(config_path) or {}
+        except YAMLError:


[warning] defense: YAMLError is silently caught and the loop continues with no diagnostic output. A corrupt config.yaml will produce a false-negative OK result, masking a real problem. Surface this as DoctorResult(Severity.WARNING, ...) instead of silently continuing.

Trecek · 2026-03-22T08:40:59Z

src/autoskillit/cli/_doctor.py

+    Scans the user-level and project-level config.yaml files for any keys
+    that belong only in .secrets.yaml. Reports ERROR with exact fix guidance.
+    """
+    from autoskillit.config import _SECRETS_ONLY_KEYS


[warning] cohesion: _check_config_layers_for_secrets uses lazy (inside-function) imports for _SECRETS_ONLY_KEYS and load_yaml, while _init_helpers.py imports them at module top-level in the same PR. Prefer one import style consistently across files.

Trecek · 2026-03-22T08:41:00Z

tests/cli/test_doctor.py

+
+
+# DR-SECRETS-1
+def test_doctor_detects_misplaced_token_in_project_config(


[warning] tests: DR-SECRETS-1 patches Path.home() to tmp_path AND passes project_dir=tmp_path, so both config_paths resolve to the same .autoskillit directory. The test cannot distinguish whether the ERROR is from the home-layer or project-layer check. Add a separate test with home pointing to a different empty directory.

tests/cli/test_doctor.py

Trecek · 2026-03-22T08:41:02Z

tests/cli/test_init.py

        # MCP server should be registered to user home, not project dir
        assert (tmp_path / ".claude.json").exists()

+    def test_register_all_config_write_is_validated(


[warning] tests: test_register_all_config_write_is_validated asserts ConfigSchemaError is raised but does not assert that no partial file was written. Add: assert not (config_dir / 'config.yaml').exists() after the raises block to verify the no-write-on-error guarantee.

Trecek · 2026-03-22T08:41:03Z

tests/cli/test_init.py

+
+
+# SS-INIT-ROUNDTRIP
+def test_bypass_accepted_init_load_config_round_trip(


[warning] tests: SS-INIT-ROUNDTRIP calls cli.init() which exercises many unrelated side effects. If any init step fails under tmp_path isolation, the test fails for the wrong reason. Consider a narrower unit test calling _log_secret_scan_bypass directly then load_config.

Trecek · 2026-03-22T08:41:04Z

tests/config/test_config.py

+        # Must include the exact YAML to add (so user can copy-paste)
+        assert "token:" in msg
+        # Must include removal instruction
+        assert "remove" in msg.lower() or "delete" in msg.lower() or "config.yaml" in msg


[warning] tests: The actionability assertion uses a trivially-true third disjunct: 'config.yaml' in msg will always be true for this error. Tighten to: assert 'remove' in msg.lower() or 'delete' in msg.lower().

Trecek

AutoSkillit review found 10 blocking issues (2 critical, 8 warning). See inline comments.

Critical (must fix):

_init_helpers.py:391 — ConfigSchemaError not caught in try/except; crashes caller silently
_init_helpers.py:397 — write_config_layer call outside try/except entirely

Warnings:

_doctor.py — secrets scan logic duplicated from config layer; YAMLError silently swallowed; lazy vs top-level import inconsistency
tests/ — DR-SECRETS tests ambiguous (home/project paths overlap); weak/missing assertions in test_init.py and test_config.py

verdict=changes_requested

…ability assertion

…alidation fails

…revent uncaught OSError

…face YAML parse failures as WARNING

…endently exercise each config path

…sfy cross-submodule import guard

Trecek and others added 7 commits March 22, 2026 00:46

test: add failing tests for write_config_layer gateway and doctor sec…

a2cfd9b

…rets check (Part B)

feat: add write_config_layer gateway and doctor secrets placement che…

fa27fe5

…ck (Part B)

fix: add missing Path import to test_config.py for TestWriteConfigLayer

3da9968

chore: commit auto-generated files

de65fd6

fix: use package re-export for _SECRETS_ONLY_KEYS to avoid cross-subm…

b79daab

…odule import violation

Trecek commented Mar 22, 2026

View reviewed changes

tests/cli/test_doctor.py Show resolved Hide resolved

Trecek commented Mar 22, 2026

View reviewed changes

Trecek added 6 commits March 22, 2026 01:48

fix(review): remove trivially-true 'config.yaml' disjunct from action…

0c29bb4

…ability assertion

fix(review): assert config file not created when write_config_layer v…

b034426

…alidation fails

fix(review): wrap new-file write_config_layer call in try/except to p…

6955cb8

…revent uncaught OSError

fix(review): delegate config secrets scan to validate_layer_keys; sur…

1c8aebc

…face YAML parse failures as WARNING

fix(review): separate home and project_dir in DR-SECRETS-1/2 to indep…

3a90e7a

…endently exercise each config path

fix(review): re-export validate_layer_keys via config package to sati…

3639b21

…sfy cross-submodule import guard

Trecek added this pull request to the merge queue Mar 22, 2026

Merged via the queue into integration with commit 4c03177 Mar 22, 2026
2 checks passed

Trecek deleted the remediation/20260322 branch March 22, 2026 15:44

Trecek mentioned this pull request Mar 24, 2026

Promote integration to main (28 PRs, 25 issues, 16 fixes, 16 features) #496

Merged

This was referenced Apr 2, 2026

Promote integration to main (66 PRs, 56 issues, 39 fixes, 30 features) #575

Closed

Promote integration to main (68 PRs, 57 issues, 40 fixes, 31 features) #580

Merged



		# SS-INIT-ROUNDTRIP
		def test_bypass_accepted_init_load_config_round_trip(



		# DR-SECRETS-1
		def test_doctor_detects_misplaced_token_in_project_config(

Conversation

Trecek commented Mar 22, 2026

Summary

Architecture Impact

Error/Resilience Diagram

State Lifecycle Diagram

Operational Diagram

Implementation Plan

Uh oh!

Trecek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trecek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant