feat: #104 skim lint — linter parsers (eslint, ruff, mypy, golangci-lint)

[dean0x]

Summary

• Adds skim lint subcommand with four linter parsers following the existing three-tier degradation pattern (Full JSON, Degraded regex, Passthrough)
• Includes LintResult/LintIssue/LintGroup canonical types, CommandType::Lint analytics variant, --json structured output flag, 9 rewrite rules, and comprehensive tests
• All 1652 tests pass (0 failures), cargo clippy -- -D warnings clean

Changes

Types & Infrastructure

• LintSeverity, LintIssue, LintGroup, LintResult in output/canonical.rs — follows exact same pattern as TestResult/BuildResult/GitResult (pre-rendered display, ensure_rendered(), AsRef<str>, serde roundtrip)
• CommandType::Lint variant in analytics/mod.rs
• lint registered in cmd/mod.rs (KNOWN_SUBCOMMANDS, dispatch)

Parsers (4 linters, 3 tiers each)

• eslint — Tier 1: --format json array; Tier 2: regex on default formatter
• ruff — Tier 1: --output-format json array; Tier 2: regex on text output
• mypy — Tier 1: --output json NDJSON (line-by-line); Tier 2: regex on text
• golangci-lint — Tier 1: --out-format json object with Issues key (handles null); Tier 2: regex on text

Rewrite Rules (9 rules)

• eslint, npx eslint (skip: --format, -f)
• ruff, ruff check (skip: --output-format)
• mypy, python -m mypy, python3 -m mypy (skip: --output)
• golangci-lint, golangci-lint run (skip: --out-format)

Tests

• Unit tests: LintResult display, serde roundtrip, ensure_rendered, per-parser tier 1/2/3
• E2E tests: cli_e2e_lint_parsers.rs — all linters at all tiers, --json flag, help, unknown linter
• Rewrite E2E tests: all 9 lint rules + skip-flag suppression

Test plan

• cargo test — all 1652 tests pass
• cargo clippy -- -D warnings — zero warnings
• E2E: each linter at Tier 1 (JSON piped via stdin)
• E2E: each linter at Tier 2 (text piped via stdin)
• E2E: each linter at Tier 3 (garbage input -> passthrough)
• E2E: --json flag produces structured JSON output
• E2E: skim lint --help shows all linters
• E2E: unknown linter produces error
• Rewrite rules: all 9 lint commands rewrite correctly
• Rewrite skip: eslint --format json suppressed

Closes #104

[dean0x]

Unbounded stdin read in run_json_mode (Confidence: 95%)

The io::stdin().read_to_string(&mut stdin_buf) call at this line reads without a size limit, creating an unbounded memory growth risk. The established pattern in run_parsed_command_with_mode (cmd/mod.rs) enforces a 64 MiB limit via .take(MAX_STDIN_BYTES). A malicious or runaway pipe could cause OOM.

Fix: Apply the bounded read pattern:

const MAX_STDIN_BYTES: u64 = 64 * 1024 * 1024;
let bytes_read = io::stdin()
    .take(MAX_STDIN_BYTES)
    .read_to_string(&mut stdin_buf)?;
if bytes_read as u64 >= MAX_STDIN_BYTES {
    anyhow::bail!("stdin input exceeded 64 MiB limit");
}

Note: This same pattern is needed in ruff.rs:79, mypy.rs:72, and golangci.rs:79.

[dean0x]

Exit code always SUCCESS in --json mode (Confidence: 85%)

When --json is used, this unconditionally returns Ok(ExitCode::SUCCESS). The non-json codepath (via run_parsed_command_with_mode) correctly preserves the linter's exit code. This means a linter reporting errors (exit code 1) will appear successful when --json is used. Downstream scripts checking 0 will not detect linter failures.

Fix: Capture and return the actual exit code:

let code = output.exit_code.unwrap_or(0);
Ok(ExitCode::from(code.clamp(0, 255) as u8))

Note: This same issue exists in ruff.rs:148, mypy.rs:145, and golangci.rs:150.

[dean0x]

Massive run_json_mode duplication (Confidence: 95%)

This function (lines 67-145) is copy-pasted nearly identically across all four lint modules (~80 lines each, ~320 lines total). The only differences are: program name, env overrides, and install hint. The function body -- stdin reading, command execution, ANSI stripping, error handling, parse_impl call, serialization, stdout writing, stats, and analytics -- is duplicated verbatim.

Impact: When the JSON output format evolves (e.g., adding a field, updating tier serialization), all 4 files must be updated in lockstep. This is the exact OCP/DRY anti-pattern that the existing run_parsed_command_with_mode was designed to prevent.

Fix: Extract a shared helper in cmd/lint/mod.rs parameterized by the same ParsedCommandConfig struct already used by run_parsed_command_with_mode. Each linter would call this shared helper with its config and parse_impl closure.

Note: This duplication also exists in ruff.rs:70-148, mypy.rs:63-145, and golangci.rs:70-150.

[dean0x]

? operator inside loop silently discards all parsed issues (Confidence: 95%)

The early-return via ? inside the for-loop iteration means if a single JSON entry has a missing filePath or messages field, the entire function returns None -- discarding ALL previously parsed issues and falling through to Tier 2 regex parsing. A valid JSON response with one malformed entry causes complete parse failure.

Fix: Use continue to skip malformed entries instead of aborting:

for file_entry in &arr {
    let Some(file_path) = file_entry.get("filePath").and_then(|v| v.as_str()) else {
        continue;
    };
    let Some(messages) = file_entry.get("messages").and_then(|v| v.as_array()) else {
        continue;
    };
    // ... continue parsing
}

Note: Same issue in ruff.rs:190-194 and golangci.rs:207-209. Mypy correctly uses continue.

[dean0x]

Comment mismatch: says 26 rules, actual is 24 (Confidence: 95%)

The comment was updated from "15 rules" to "26 rules" but the actual count is 24 (15 original + 9 new lint rules). Intent vs reality mismatch.

Fix:

// Rule table (24 rules, ordered longest-prefix-first within same leading token)

[dean0x]

PR #106 Code Review Summary

Overall Status: ⚠️ CHANGES_REQUESTED

Review Coverage: 8 reviewers across 7 dimensions (security, architecture, performance, complexity, consistency, regression, tests)
Inline Comments: 5 (covering blocking issues)
This Summary: Lower-confidence suggestions and deferred findings

---

Findings by Severity

BLOCKING ISSUES (See inline comments)

Issue	Files	Confidence	Status
Unbounded stdin in run_json_mode	eslint.rs:76, ruff.rs:79, mypy.rs:72, golangci.rs:79	95%	💬 Commented
Exit code always SUCCESS in --json mode	eslint.rs:145, ruff.rs:148, mypy.rs:145, golangci.rs:150	85%	💬 Commented
run_json_mode duplication across 4 modules	eslint.rs:67-145, ruff.rs:70-148, mypy.rs:63-145, golangci.rs:70-150	95%	💬 Commented
? operator silently discards parsed issues in loops	eslint.rs:186, ruff.rs:190-194, golangci.rs:207-209	95%	💬 Commented
Rule count comment mismatch (26 vs 24)	rewrite.rs:123	95%	💬 Commented

---

Additional Findings (60-79% Confidence - Should Fix in Future PRs)

High-Priority Suggestions

1. Inconsistent error handling between --json and non---json paths (Architecture, Consistency)

• Problem: The run_json_mode functions use eprintln! + Ok(ExitCode::FAILURE) for "not found" errors, while the shared run_parsed_command_with_mode path uses anyhow::bail!. This creates different error reporting behaviors for the same error condition.
• Impact: Downstream consumers see different exit behavior depending on whether --json is passed.
• Fix: Use consistent error handling (prefer anyhow::bail! to match existing convention).
• Confidence: 85%

2. parse_impl duplicated across 4 linters (Complexity, Consistency)

• Problem: The three-tier dispatch skeleton in parse_impl is identical in all 4 modules (~20 lines each). Only the try_parse_json and try_parse_regex calls differ.
• Fix: Extract a generic three_tier_parse helper into lint/mod.rs.
• Confidence: 90%

3. Missing stdin size limit protection in JSON mode (Security, Performance, Regression)

• Already flagged in inline comments - Multiple reviewers flagged this as HIGH across security/performance/regression dimensions.

4. Test assertion gaps (Testing)

• Weak assertion in test_tier2_eslint_regex (eslint.rs:317): Uses loose assert!(result.errors > 0 || result.warnings > 0) instead of exact counts. Fix: assert_eq!(result.errors, 2); assert_eq!(result.warnings, 2);
• Weak assertion in test_tier1_golangci_fail (golangci.rs:287): Uses sum assert_eq!(result.errors + result.warnings == 4) instead of individual counts. Fix: assert_eq!(result.errors, 1); assert_eq!(result.warnings, 3);
• Missing assertion in E2E test (cli_e2e_lint_parsers.rs:224-232): test_golangci_tier1_json_fail only asserts output contains "LINT:" without verifying counts. Fix: Add .stdout(predicate::str::contains("1 error")).stdout(predicate::str::contains("3 warnings"))
• Confidence: 82-85%

5. Performance optimizations (Performance)

• Unnecessary stdout.clone() on Tier 2 fallback (eslint.rs:183, ruff.rs:175, mypy.rs:157, golangci.rs:163): Can use Cow<str> to avoid allocation when stderr.is_empty().
• serde_json::Value instead of typed deserialization (all 4 linters): Consider defining typed structs (EslintFileEntry, etc.) for direct deserialization instead of heap-allocating intermediate Value DOM.
• Confidence: 80-85%

---

Pre-existing Issues Observed (Not Blocking This PR)

Tech Debt

1. canonical.rs file growing too large (731 lines, 197 new) -- Consider splitting into canonical/lint.rs, canonical/build.rs, canonical/test.rs when it exceeds 1000 lines.
2. Module duplication pattern: The build/mod.rs has its own run_parsed_command that partially duplicates cmd/mod.rs::run_parsed_command_with_mode. The lint module's run_json_mode duplication now compounds this pattern.
3. Rewrite rules table growing linearly (15 → 26 entries) -- Not urgent, but consider grouping by category or a declarative definition if it grows beyond 40+ rules.

---

What Passed Review ✓

• Three-tier degradation architecture: Well-designed, consistent pattern across all tiers.
• Canonical LintResult type: Follows same new() + pre-rendered display pattern as TestResult and BuildResult.
• Strategy Pattern dispatcher: Clean, idiomatic dispatch in lint/mod.rs with no special-case conditionals.
• Rewrite rules: Thorough, longest-prefix-first ordering, covers 9 new lint variations.
• Test coverage: Excellent across all 3 degradation tiers, both unit and E2E tests for all 4 linters.
• Fixture-based testing: Consistent pattern with real-world JSON and text samples.
• Security: No injection vectors, no secrets, proper parameterized SQL queries, safe deserialization patterns (uses .ok()? instead of unwrap()).
• Backward compatibility: No exports removed, no breaking changes to public API, all consumers updated.

---

Recommendation Summary

CHANGES_REQUESTED - Please address:

1. ✅ All 5 inline comments (blocking issues)
2. ⚠️ Test assertion tightening (before merge, should be quick)
3. ⚠️ Consider error handling consistency refactor (good-to-have, can defer to follow-up)
4. ⚠️ Consider run_json_mode extraction + parse_impl deduplication (can defer if time is critical, but HIGH priority)

Regression Status: Partial

• ✅ Exit codes, stdin safety, rule count comment must be fixed
• ⚠️ Once fixed, full backward compatibility maintained

---

Scores by Dimension

Dimension	Score	Status
Security	8/10	Issues require fixes
Architecture	5/10	Duplication needs refactoring
Performance	7/10	Unbounded stdin is main risk
Complexity	7/10	Duplication inflates line count
Consistency	7/10	Needs alignment across 4 modules
Regression	7/10	Exit codes + stdin limits must be fixed
Tests	7/10	Few weak assertions, mostly good
Rust	7/10	Early-return via ?, duplication, casting

Consensus: Well-structured with excellent test coverage, but requires addressing 5 blocking issues and test assertion tightening before merge.

---

Created by Claude Code | Review timestamp: 2026-03-28

[dean0x]

Missing emit_markers call (Confidence: 88%)

The JSON mode path does not emit degradation-tier markers ([warning], [notice]) to stderr. This breaks the behavioral contract established by the non-JSON path in cmd/mod.rs:224-226.

When --json falls back to Tier 2/3, the markers should still be written to stderr for observability. Without markers, users piping JSON output won't see that the result is degraded.

Fix: Add marker emission after parsing (following the pattern in cmd/mod.rs:222-226):

[dean0x]

Analytics label inconsistency (Confidence: 85%)

The JSON code path records analytics as "skim lint eslint ..." while the non-JSON path (via run_parsed_command_with_mode) records as "skim eslint ..." — missing the lint subcommand.

The build module avoids this by using its own run_parsed_command helper. Consider passing a custom label through config or extracting a run_lint_parsed_command helper to keep labels consistent across both code paths.

[dean0x]

Unnecessary clone when stderr is empty (Confidence: 90%)

This function always returns String, cloning stdout even when stderr is empty. The existing combine_output in pytest.rs:210 correctly uses Cow<'_, str> to avoid this allocation.

Fix: Use Cow pattern:

pub(crate) fn combine_stdout_stderr<'a>(output: &'a CommandOutput) -> Cow<'a, str> {
    if output.stderr.is_empty() {
        Cow::Borrowed(&output.stdout)
    } else {
        Cow::Owned(format!("{}\n{}", output.stdout, output.stderr))
    }
}

[dean0x]

Duplicates ~80 lines from run_parsed_command_with_mode (Confidence: 88%)

The stdin reading, command execution, analytics recording, and exit code mapping are nearly identical to cmd/mod.rs:157+. This violates DRY and creates a maintenance burden.

Fix: Refactor to share the execution infrastructure. Extract a lower-level helper that both paths call, keeping only output formatting (JSON vs plain text) distinct. Alternatively, add an output formatter strategy (trait/closure) to run_parsed_command_with_mode.

[dean0x]

High complexity: 98 lines, 4 nested branches, mixed responsibilities (Confidence: 85%)

This function handles stdin reading, command execution, error detection, three-tier serialization, stdout writing, stats, and analytics — each a separate change reason.

Suggested refactoring (follow the complexity report):

1. Extract stdin/command execution into obtain_output() helper
2. Extract ParseResult→JSON serialization into serialize_parse_result() helper
3. This brings each function under 40 lines with single responsibility

[dean0x]

Unchecked u64 as u32 cast truncates large line numbers (Confidence: 82%)

JSON as_u64() returns values up to u64::MAX. Casting to u32 without bounds checking silently truncates values > 4,294,967,295.

While line numbers exceeding 4 billion are unrealistic, Rust idiom is to use saturating casts:

line: u32::try_from(line).unwrap_or(u32::MAX),

Same issue appears in ruff.rs:133, mypy.rs:144, golangci.rs:149.

[dean0x]

Unnecessary format!() calls in render loop (Confidence: 85%)

Lines that do format!("{}", group.severity) allocate a String just to get "error"/"warning" strings. These are then immediately written to the output buffer.

Fix: Use severity directly with inline pluralization:

let _ = write!(
    output,
    "\n  {} ({} {}{}): ",
    group.rule,
    group.count,
    group.severity,
    if group.count == 1 { "" } else { "s" }
);

[dean0x]

High nesting (4 levels) in try_parse_json (Confidence: 82%)

The function iterates over JSON array → iterates over messages → nested let-else guards. This cumulative density makes the 44-line function harder to follow.

Fix: Extract inner loop into parse_eslint_message() helper:

fn parse_eslint_message(file_path: &str, msg: &Value) -> Option<LintIssue> {
    let severity_num = msg.get("severity").and_then(|v| v.as_u64())?;
    let severity = match severity_num { 2 => Error, 1 => Warning, _ => Info };
    let rule_id = msg.get("ruleId").and_then(|v| v.as_str()).unwrap_or("(unknown)");
    let message = msg.get("message").and_then(|v| v.as_str())?;
    let line = msg.get("line").and_then(|v| v.as_u64())?;
    Some(LintIssue { file: file_path.to_string(), line: line as u32, rule: rule_id.to_string(), message: message.to_string(), severity })
}

Then refactor outer loop to filter_map over the extracted helper.

[dean0x]

Code Review Summary

Overall Assessment: 8 high-confidence (≥80%) inline comments created covering blocking and should-fix findings.

Critical Issues Addressed (≥80% confidence)

• Behavioral divergence: Missing emit_markers() in JSON path (88%)
• Duplication: run_lint_json_mode reproduces 80+ lines from shared infrastructure (88%)
• Analytics inconsistency: Labels differ between JSON and non-JSON paths (85%)
• Complexity: run_lint_json_mode at 98 lines with 4+ nesting levels (85%)
• Performance: combine_stdout_stderr clones unnecessarily (90%)
• Unsafe casts: u64 as u32 truncates without bounds checking (82-90%)
• Test coverage gap: Missing Degraded tier tests in 3 of 4 linters (85%)

Lower-Confidence Findings (60-79%) - Summary Only

These are documented in the review reports but below the 80% threshold for inline comments:

1. Linter run() signature divergence (80%): All four lint parsers take (args, show_stats, json_output) while test/build use (args, show_stats). Inconsistent pattern for handling --json flag threading through individual parsers.

2. Structural duplication in parse_impl functions (83%): All four linters implement identical three-tier dispatch. A generic three_tier_parse(try_json, try_regex) helper would eliminate this.

3. Repeated run() boilerplate (85%): All four linters clone args, inject format flags, branch on json_output. A LinterConfig struct could reduce per-linter duplication.

4. Golangci severity always defaults to Warning (82%): Tier 2 regex has no severity field in text output. This is a known limitation, just document it.

5. Missing Degraded tier E2E test for JSON path (80%): The --json envelope with degraded results is not tested at E2E level.

6. combine_stdout_stderr unit test gap (82%): No test for non-empty stderr branch.

Pre-existing / Non-blocking

• Stale test comment: "all 15 rules" should be "24 rules" (rewrite.rs:1513)
• Various Vec::new() without capacity hints (low impact)
• Regex .unwrap() on LazyLock (idiomatic, covered by tests)

Test Coverage Notes

The PR adds 60+ tests across:

• 21 E2E tests with real stdin piping
• 35+ unit tests for parse tiers
• 10 fixture files per-linter

All 818+ existing tests pass. The main gap is asymmetric parse_impl coverage (eslint has all 3 tiers, others missing Degraded).

Next Steps for Merge Readiness

1. Address emit_markers() missing call (behavioral contract)
2. Consider refactoring run_lint_json_mode complexity and duplication
3. Add Degraded tier tests for ruff, mypy, golangci
4. Fix u64 as u32 casts to use saturating bounds
5. Fix combine_stdout_stderr to use Cow pattern

---

Reviewed with 80% confidence threshold for inline comments. Review reports at .docs/reviews/feat-104-lint-parsers/2026-03-29_0215/ include full analysis of all findings.

Review conducted by Claude Code on 2026-03-29.

[dean0x]

Merged via consolidation PR #108