Fix: schema-discovery fail-open (unreadable migration silently disabled DB validation)

[0xLeif]

Audit Batch 4 — the schema half of the file-read-fail-loud theme (split out of #325 to keep that PR focused).

Root cause

build_schema silently skips a schema/migration file it can't read as UTF-8 (Err(_) => continue). If every schema file is unreadable, the discovered table set is empty, and validator.rs's !schema_tables.is_empty() guard then skips the db_tables and column checks entirely — so a corrupted migration made DB validation a silent no-op, passing green even under check --strict.

Fix

Add schema::schema_read_errors(schema_dir) — an error per schema file that exists but fails a UTF-8 read (scanning the same SQL_EXTENSIONS files build_schema would). Wire it once, project-level, into the shared run_validation after the per-spec loop, so it gates every validating command (check, coverage, generate, comment, issues) at a single point — no churn across the 8+ get_schema_table_names / build_schema_columns call sites. An unreadable migration is now a hard error: printed in the report and gating the exit under strict/enforcement.

Benign cases preserved: a genuinely-empty-but-readable schema, non-schema files, and an absent schema_dir (schema simply not configured) all produce nothing.

Reproduced

With schema_dir set and a non-UTF-8 001_init.sql: check went from silent exit 0 (no error at all, even --strict) to a surfaced could not be read as UTF-8; DB schema validation would be incomplete error that gates the strict exit. Readable migration and no-schema-configured → zero schema errors.

Tests

schema_read_errors unit test (absent dir / readable / non-schema file / non-UTF-8) + 2 integration (gates on unreadable migration; no false positive on readable). Documented as schema spec invariant #2a + API row. 751 unit + 179 integration, fmt / clippy / self-check 100%.

Part of the post-v4.7.1 audit sweep (Batch 4). Branches off the pre-#325 main; independent files (schema.rs, commands/mod.rs) — merges cleanly alongside #325.

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to detect and report schema or migration files that exist but cannot be read as UTF-8, preventing silent validation bypasses. It adds the schema_read_errors function to scan for these files and surfaces them as hard errors during validation. The review feedback suggests improving the robustness of schema_read_errors by explicitly handling potential errors from fs::read_dir and optimizing the file extension filtering to avoid unnecessary string allocations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Correctness & Efficiency Improvement

1. Error Handling: If schema_dir exists but is unreadable (e.g., due to permission issues or if it is actually a file), fs::read_dir will return an Err. The current implementation silently ignores this error using .into_iter().flatten().flatten(), which can lead to a silent fail-open (the exact issue this PR aims to prevent).
2. Performance: The current filter allocates a new String for every single file in the directory by calling .to_string(). We can avoid this allocation entirely by matching directly on the &str extension.

We can address both issues by explicitly handling the fs::read_dir result and optimizing the extension filter.

    let read_dir = match fs::read_dir(schema_dir) {
        Ok(rd) => rd,
        Err(err) => {
            errors.push(format!(
                "Could not read schema directory `{}`: {err}",
                schema_dir.display()
            ));
            return errors;
        }
    };

    let mut files: Vec<_> = read_dir
        .flatten()
        .filter(|e| {
            e.path()
                .extension()
                .and_then(|x| x.to_str())
                .map(|ext| SQL_EXTENSIONS.contains(&ext))
                .unwrap_or(false)
        })
        .collect();

[github-actions]

✅ Corvin says...

      _
    <(^\  .oO(Caw! ^v^)
     |/(\
      \(\\
      " "\\

"Looking sharp! Like a beak should be."

CI Summary

Check	Status
Validate action.yml	✅ Passed
Dependency Audit	✅ Passed
Code Coverage	✅ Passed
Format Check	✅ Passed
Docs Site	✅ Passed
Spec Validation	✅ Passed
Tests (build, test, clippy)	✅ Passed
VS Code Extension	✅ Passed

📋 Spec Validation Details

✅ SpecSync: Passed

Metric	Value
Specs checked	60
Passed	60
Errors	0
Warnings	0
File coverage	100% (76/76)
LOC coverage	100% (38524/38524)

---

Generated by specsync · Run specsync check --format github to reproduce

---

_{Powered by corvid-pet}

[0xLeif]

Adversarial review: READY_WITH_NITS, 93 — no correctness defects.

• Core fix confirmed real. On main, the same non-UTF-8 migration fixture reports 1 passed and check --strict exits 0 (the users db_tables check is a silent no-op — validator.rs:318 skips when schema_tables is empty). On the branch it prints the error, increments total_errors, and exits 1 under strict/enforcement. Counted once project-level (3 specs + 1 bad migration → exactly one error line).
• No false positives. Readable migration → 0; empty-but-readable .sql → 0; no schema_dir configured (common case) → 0 and byte-identical output to main; a README.md (even with a 0xFF byte) in schema_dir → not flagged (only SQL_EXTENSIONS).
• No double-count / regression. schema_read_errors runs once outside the per-spec loop; generate's multiple run_validation calls each recompute total_errors from 0 (no accumulation). 751 unit + 179 integration pass.
• Collect-mode JSON valid (error appears in errors[], structure intact); markdown/github render it too.
• Warm-cache is NOT a fail-open (verified): the cache persists only when total_errors==0, so a failing run never caches clean; --strict/--force bypass the cache; hash_file reads raw bytes so a readable→unreadable edit is detected.
• Self-check green; schema_read_errors documented in the schema spec (row + invariant 2a).

Non-blocking nits (noted, not fixed here):

1. MCP scoping: mcp.rs's check tool builds schema directly and doesn't go through run_validation, so it still under-validates an all-unreadable schema. The finding centers on CLI/CI gating; acceptable gap, flagged for consistency.
2. create_drift_issues groups all_errors by the specpath: prefix; the project-level schema error has no such prefix, so --create-issues/issues won't file a GitHub issue for it (it still prints and gates the exit). Defensible — it's an infra error, not per-spec drift — so left as a follow-up rather than expanding this PR's scope.

[Kyntrin]

Thanks for splitting out the schema half. The core direction is right, and CI is green, but I found two blockers before merge.

Requested changes:

1. The existing unresolved inline comment on src/schema.rs is valid: schema_read_errors still uses fs::read_dir(schema_dir).into_iter().flatten().flatten(). If schema_dir exists but cannot be read, or is a file rather than a directory, read_dir returns Err and this function returns no errors. That is another fail-open path for the same validation surface: schema discovery becomes empty and db_tables / column checks can be skipped without signal. Please handle the read_dir error explicitly and return a hard schema-read error.

2. specs/schema/schema.spec.md adds a new exported function and invariant, but the spec frontmatter version is still unchanged and the Change Log has no entry. Per the repo workflow, spec contract changes need a version bump and dated Change Log entry.

Non-blocking note: the extension filter can avoid allocating a String for every directory entry by matching on the &str extension directly, as Gemini suggested. That part is a small cleanup; the read_dir error handling is the blocking correctness issue.