fix(security): D-1 load-time AST check in SkillRegistry.load + trusted-skill manifest

[AVADSA25]

What

Phase 1 Wave 1 — PR-1A. Closes D-1 (CRITICAL) — see docs/audits/PHASE-1-SECURITY.md and the consolidated triage.

Before this PR, SkillRegistry.load called spec.loader.exec_module(mod) on any .py file in the skills directory with no load-time validation — is_dangerous_skill_code only ran at write gates (/api/save_skill, /api/skill/approve, codec_self_improve._validate). A file written outside those gates loaded with full process privileges. That made "drop a file in ~/.codec/skills/, wait for restart" a full RCE primitive reachable from four independent paths (D-2 /api/forge, D-3 /api/save_skill, D-4 MCP-HTTP-exposed file_write, D-5 Step 9 permission_gate traversal).

How

Two-stage gate runs BEFORE exec_module (codec_skill_registry.py SkillRegistry.load):

1. Trusted manifest. <skills_dir>/.manifest.json maps each approved built-in to sha256(file-bytes). If the file's hash matches an entry, the skill is hash-pinned-trusted and loads. This is what lets the 23 legitimately-dangerous built-ins (calculator, system, file_write, pilot, password_generator, health_check, ...) continue to work — their source IS dangerous, their hash IS approved at PR-review time.
2. AST safety gate. Files NOT in the manifest run through codec_config.is_dangerous_skill_code. Any dangerous pattern → refuse → emit skill_load_blocked audit event → load() returns None. Fail-safe on any error inside the check (validator raised, file unreadable, UTF-8 decode failure).

Why manifest, not just AST-on-everything

The original PR-1A plan said "run is_dangerous_skill_code on every load." During TDD I confirmed this would break 23 of CODEC's 76 built-in skills because they legitimately use subprocess, os.system, eval, shutil.rmtree, etc. Per the plan's failure-mode section, I stopped and asked for direction. Mickael chose Option B (manifest of hashes). The manifest pins those 23 (and all other built-ins) at their reviewed hashes; any swap or replacement by an attacker invalidates the hash and re-engages the AST gate. Closes D-1 cleanly without false positives.

Before / after SkillRegistry.load

Before: if name in cache → return; read path → exec_module → cache → return. Zero validation. Any .py file in the skills dir gets executed with full process privileges on first load.

After: if name in cache → return; read bytes → sha256 hash → if hash ∈ trusted_manifest exec_module (path A); else decode UTF-8 → is_dangerous_skill_code → if dangerous refuse + emit skill_load_blocked audit + return None (path B); else exec_module (path C). Three fail-safe exit points (read fail / decode fail / validator exception) all return None. Cache-hit short-circuits both checks for performance + idempotence.

Tests

+13 new tests in tests/test_skill_registry.py (TDD: written first, watched fail, then implemented). Coverage:

• 4× refuses dangerous patterns (subprocess import, eval(), os.system(), __import__())
• 1× accepts safe skill
• 1× handles unreadable file
• 1× AST-validator exception fails safe (refuses, doesn't fall through to exec_module)
• 1× block event includes full metadata (skill_name, skill_path, reason, file_sha256)
• 1× cached module skips re-check (perf + idempotence)
• 1× trusted-manifest bypass (skill with dangerous source loads because hash matches)
• 1× hash mismatch still refused (manifest claims a different hash → file is NOT trusted → AST runs → refused)
• 1× corrupted manifest falls back to AST check (treat as empty trusted set)
• 1× no manifest at all → AST check applies (the typical ~/.codec/skills/ case)

Sample audit log entry

Hand-verified by dropping a synthetic malicious skill (import subprocess; subprocess.run([...]).read('/etc/passwd')) into a temp dir with no manifest, calling load(), then grepping ~/.codec/audit.log:

{
  "ts": "2026-05-17T11:12:02.529+00:00",
  "schema": 1,
  "event": "skill_load_blocked",
  "source": "codec-skill-registry",
  "outcome": "error",
  "level": "warning",
  "message": "Refused skill backdoor: Dangerous import: subprocess",
  "extra": {
    "skill_name": "backdoor",
    "skill_path": "/.../tmp/codec-handverify-.../skills/backdoor.py",
    "reason": "Dangerous import: subprocess",
    "file_sha256": "3e738d6243ebc3a04f0b73633c4e0a4d4712dd0b43f25baca0e208529e5cbd1f"
  }
}

is_dangerous_skill_code signature

No change required — it already returns tuple[bool, str] (verified by grep across the 5 existing call sites). Plan step 2 was conditional on its return type; condition didn't apply.

CI changes

Two new steps in .github/workflows/ci.yml:

• Trusted-skill manifest is current (D-1 gate) — runs python tools/generate_skill_manifest.py --check, fails the build with a diff if a built-in skill was edited without regenerating the manifest.
• Skill registry load-time AST gate tests (D-1) — runs pytest tests/test_skill_registry.py -v.

Files changed

• codec_skill_registry.py — two-stage gate + manifest loader
• tools/generate_skill_manifest.py — new, generates skills/.manifest.json with --write / --check modes
• skills/.manifest.json — new, sha256 of every approved built-in (74 entries; codec.py + _*.py excluded per tests/test_skill_contracts.py NON_SKILL_FILES)
• tests/test_skill_registry.py — new test file, 13 tests
• AGENTS.md §7 — new "Skill load-time safety gate" subsection + contributor workflow
• docs/audits/PHASE-1-SECURITY.md D-1 — closure footnote
• docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md — D-1 row status W1 → W1 — CLOSED (PR-1A)
• .github/workflows/ci.yml — two new CI steps

This PR includes a prior commit (docs(audits): add Phase 1 audit reports + consolidated triage) that lands the 6 Phase 1 audit documents the closure footnote references. Required because PR-1A is the first PR after the audits.

Regression

The pre-existing test failures on main documented in docs/known-issues.md remain (6 failures, identical to parent). No new regressions — confirmed by stash-and-rerun against the parent commit. The 23 built-in skills that would have broken with naive AST-on-everything are all hash-pinned in skills/.manifest.json and load normally.

Out of scope (later PRs)

• D-2 /api/forge review-gate routing (PR-1B)
• D-3 /api/save_skill review-gate routing (PR-1B)
• D-4 file_write block-roots expansion (PR-1C)
• D-5 permission_gate realpath + _PATH_BLOCKLIST_SUBSTRINGS (PR-1D)
• D-17 positive-allowlist for is_dangerous_skill_code AST check (optional PR-1E)
• Sandbox-exec by default (Wave 2 design discussion)

Merge model

Chat-review-then-merge per Mickael's brief. Not self-merging.

Test plan

• python3 -m pytest tests/test_skill_registry.py -v — 13 passed locally
• python3 -m pytest tests/test_skill_contracts.py tests/test_oauth_provider.py tests/test_retry.py — all pass (no regressions in CI-gated tests)
• python3 tests/test_skill_imports.py — 76 skills parsed, 0 errors
• python3 tools/generate_skill_manifest.py --check — manifest current
• ruff check codec_skill_registry.py tests/test_skill_registry.py tools/generate_skill_manifest.py — clean
• Hand-verify: drop malicious skill → load returns None → audit log entry written
• Stash-and-rerun against parent commit — same 6 pre-existing failures, no new regressions
• CI green on PR
• Mickael chat review

🤖 Generated with Claude Code