feat: mds lint — static analysis and diagnostics (#61)

[dean0x]

Summary

Implements mds lint per the approved design: a 9-rule static analyzer (6 warnings + 3 errors) with severity-aware exit codes (0/1/2/3), tiered --fix with reverify safety gate, --format json canonical wire protocol, mds.json lint config, and new info severity with shadow-variable default-off. Byte-identical JSON proven by cross-surface parity goldens + live CLI-vs-binding tests across ALL surfaces: CLI, WASM, napi, Python, and the @mdscript/mds universal wrapper. Closes the napi parity-gap pattern (scan_imports/#164 class) — napi ships lint on day one.

Closes #61

No breaking changes — purely additive.

Reviewer Focus Areas

(a) --fix safety chain: overlap rejection fail-closed → right-to-left apply → reverify (recompile-success + no-new-diagnostics + compiled-output byte-equality) → atomic temp+rename write with re-check_symlink

(b) Severity-aware exit table: incl. deliberate divergence from mds check (analysis failure → 2; info never affects exit)

(c) WASM size: two documented budget raises 700K→750K→800K in ci.yml budget history, final binary 749,272 raw

(d) 4-surface JSON parity: canonical wire protocol byte-identical across CLI, WASM, napi, Python

(e) unused-variable deliberately Tier C never-fixed: frontmatter keys emit into compiled output

Quality-Gate Provenance

Component	Coverage	Status
Rust tests (cargo)	1574+	✅ PASS
npm tests (bundler)	371 incl. napi 83 + wrapper 205	✅ PASS
Python tests (pytest)	182	✅ PASS
WASM spec tests	42	✅ PASS
Scrutinizer (self-review)	reverify over-refusal P1, non-ASCII frontmatter-key panic P1, redundant-else/@elseif FP, napi lintVirtual input caps	✅ FIXED
Alignment (spec-catalog)	unreachable-branch FP	✅ FIXED
Scenario QA	44/44 incl. CRLF --fix byte-cleanliness, CLI↔Python byte parity	✅ PASS

Known Limitation

Tier A block-spanning fixes (empty-block, unreachable-branch) are currently always refused by the reverify gate: single-line removal leaves orphaned @end, so the reverify gate fails (fail-closed, correct safety behavior, no user harm). To make Tier A real for these rules, the planner needs a block-span edit (directive-line start through the matching @end line, inclusive). Documented as best-effort in spec §8 footnote. Follow-up: #172.

[dean0x]

Code Review Summary — PR #171: mds lint (6 reviewers)

Merge Posture

✅ STRONG ADDITIVE IMPLEMENTATION with defined conditions to address before merge.

Blockers: Documentation accuracy + one reachable panic + fail-closed test gaps in --fix safety paths.
Verdict: APPROVED_WITH_CONDITIONS (all 6 reviewers recommend merge after fixes)

---

Critical Issues Requiring Fixes

1. Documentation: "all rules warn by default" is factually false (README.md:210, CHANGELOG.md:80)

Severity: CRITICAL (98% confidence)
Scope: README and CHANGELOG contradict the spec and the code

The text claims "all warn by default", but the code defaults 4 of 9 rules differently:

• ✗ duplicate-import, duplicate-export, unreachable-branch → Error (not warn)
• ✗ shadow-variable → Off (not warn)
• ✓ The remaining 5 rules → warn (as claimed)

This breaks the exit-code behavior documented in the same section: a file with a duplicate import exits 2 (error), not 1 (warning). It directly contradicts spec §8 ("not all rules default to warn").

Fix: Reword the intro line and update the severity table:

Rules (Most default to `warn`; `duplicate-*` and `unreachable-branch` default to `error`, 
`shadow-variable` is `info` and off by default; configure via `mds.json` `lint.rules`):

| Rule | Severity | Description |
| `duplicate-import` | error | ... (auto-fixable) |
| `duplicate-export` | error | ... (auto-fixable) |
| `unreachable-branch` | error | ... (auto-fixable) |
| `redundant-else` | warn | ... |
| `shadow-variable` | info (default-off) | ... |

2. Documentation: Phantom --set-rules flag (README.md:210)

Severity: HIGH (97% confidence)

The text mentions "or --set-rules", but this flag does not exist in the CLI. Grep finds it only in README.md. Per-rule configuration is via mds.json lint.rules (or library API), never a command-line flag.

Fix: Delete or --set-rules from the line.

3. redundant-else Misdescribed and Wrongly Marked Fixable (README.md:219, CHANGELOG.md:86, spec.md:1049)

Severity: HIGH (94% confidence)

Two errors:

1. Wrong description: "After an always-returning branch" — MDS has no "return" concept. The rule fires when @else body is structurally identical to the @if body.
2. Wrongly tagged auto-fixable: README and CHANGELOG mark it "(auto-fixable)", but redundant-else is Tier C — never fixed. The --fix CLI skips it.

Fix:

• Reword: "An @else branch whose body is identical to the @if/preceding branch body (structurally redundant)"
• Remove "(auto-fixable)" tags from README and CHANGELOG

4. Auto-fixable Annotations Scrambled (README.md:214/lines 218–222, CHANGELOG.md:87)

Severity: MEDIUM (90% confidence)

Tier A rules (auto-fixable): duplicate-import, duplicate-export, unreachable-branch, empty-block.
README/CHANGELOG mark only empty-block and duplicate-import as "(auto-fixable)"; duplicate-export and unreachable-branch are missing the tag, and redundant-else is wrongly marked (Tier C, never fixed).

Fix: Add "(auto-fixable)" to duplicate-export and unreachable-branch rows; remove from redundant-else.

5. Unchecked Byte Slice Can Panic on Out-of-Range Offset (crates/mds-core/src/lint/fix.rs:184)

Severity: MEDIUM (85% confidence), Reachability: Public API

diag_to_edit does source[..offset].rfind('\n') without bounds-checking offset. On the CLI's own pipeline, offset originates from the AST (safe), but mds::fix::plan_fixes is shipped public API. A downstream consumer (e.g., future LSP integration) that hand-builds a LintDiagnostic with an out-of-range or non-char-boundary offset crashes instead of returning None.

Failure scenario: mds::fix::plan_fixes(&result, "é") with span.offset == 1 (mid-byte of 2-byte é) → panic.

Fix: Use source.get(..offset)? instead of direct slicing:

let offset = span.offset;
let prefix = source.get(..offset)?;  // Returns None for out-of-range or non-boundary
let line_start = prefix.rfind('\n').map(|p| p + 1).unwrap_or(0);

6. write_stdout Diverges from Sibling fmt.rs — No Flush Before Exit (crates/mds-cli/src/lint.rs:214)

Severity: MEDIUM (85% confidence), Impact: Silent stdout truncation

The fmt.rs sibling explicitly flushes stdout and handles BrokenPipe. The lint.rs reimplementation does neither. In the --fix stdin filter path, Rust's LineWriter keeps a trailing partial line buffered; std::process::exit skips flushing, so fixed output without a trailing newline is silently truncated.

Fix: Reuse or duplicate fmt.rs's write_stdout:

fn write_stdout(s: &str) -> Result<()> {
    let mut stdout = std::io::stdout();
    if let Err(e) = stdout.write_all(s.as_bytes()) {
        if e.kind() == std::io::ErrorKind::BrokenPipe { return Ok(()); }
        return Err(...);
    }
    if let Err(e) = stdout.flush() {
        if e.kind() != std::io::ErrorKind::BrokenPipe {
            return Err(...);
        }
    }
    Ok(())
}

7. Committed Internal Handoff Doc Should Be Deleted (.devflow/docs/handoff-feat-mds-lint-61.md)

Severity: MEDIUM (90% confidence)

.gitignore marks .devflow/ as "local-only, not shared via git", but the 567-line handoff doc is tracked in HEAD and added by this PR. It's not referenced by spec.md/README.md/CHANGELOG, so removal is safe.

Fix: git rm .devflow/docs/handoff-feat-mds-lint-61.md

---

High-Confidence Issues Affecting Testing & Refactoring

8. Test Gaps in --fix Refusal Paths (testing.rs)

Severity: HIGH coverage gap

Three fail-closed safety mechanisms exist but lack affirmative tests:

• Overlap rejection: apply_fixes rejects same-line overlaps, but the test is guarded by if plan.overlap_rejected { ... } — if overlap detection regresses, test passes vacuously.
• Block-spanning orphaned @end refusal: --fix on a multi-line empty-block removes only the opening directive, orphaning @end. Reverify gate should refuse this; never tested in the real pipeline.
• CLI exit-3 (ResourceLimit): Never asserted; exit codes 0/1/2 are tested but not 3.

Fix Priorities:

1. Assert overlap rejection unconditionally: assert!(plan.overlap_rejected) + test the apply_fixes refusal path
2. Add CLI integration test: --fix a multi-line empty-block, assert file unchanged + "fix rejected" on stderr
3. Add nesting-depth test: >64 nested @if blocks → exit 3

9. Complexity: CLI lint_one_file_* Functions Are Near-Identical 80-Line Twins (crates/mds-cli/src/lint.rs:738, 822)

Severity: HIGH duplication (88% confidence)

lint_one_file_accumulating and lint_one_file_human are structurally identical except for emit strategy. The 3-arm match fix_outcome appears 4 times across the module. Any fix-outcome logic change requires shotgun surgery.

Fix: Extract to one function parameterized by emit sink; collapse duplicates.

10. Deeply Nested Rule Checks (unreachable_branch.rs:95, duplicate_*.rs)

Severity: HIGH nesting (85–88% confidence)

check_if_block reaches 6 levels of indentation; duplicate_export/duplicate_import repeat the None → insert, Some → push pattern 3 times each. All mechanically refactorable.

Fix: Split concerns, extract helpers for duplicate logic.

11. 7× Copy-Pasted Exit-Code Blocks (crates/mds-cli/src/lint.rs)

Severity: MEDIUM duplication (92% confidence)

The let exit = result_exit_code(&X); if exit != 0 { process::exit(exit); } idiom is repeated 7 times. Invisible opportunity for referencing the wrong result.

Fix: Add helper fn finish(result: &LintResult) -> ! or Result<()> variant.

---

Type & Interface Issues

12. TypeScript: LintSpan Missing line? / column? Fields (packages/mds/src/types.ts:66)

Severity: MEDIUM (90% confidence)

The Rust wire format emits span: { offset, length, line?, column? }, but the TS LintSpan interface declares only offset and length. Runtime data is present (unmarked extra fields tolerated by shallow validator), but TypeScript consumers get a compile error accessing .line / .column. The sibling MdsErrorSpan already declares these fields for the identical shape.

Fix: Add optional fields:

export interface LintSpan {
  offset: number;
  length: number;
  line?: number;      // 1-based line number, if available
  column?: number;    // 1-based column number, if available
}

13. TypeScript: severity Should Be Closed Union, Not string (packages/mds/src/types.ts:78)

Severity: MEDIUM (85% confidence)

severity is typed as string, but Rust only serializes "error" | "warn" | "info" (Off diagnostics are filtered). Unknown severities fail loudly. Typing it as string defeats exhaustive switch checks and allows typos to pass type-check.

Fix:

export type LintSeverity = 'error' | 'warn' | 'info';
export interface LintDiagnostic {
  severity: LintSeverity;  // Closed set
  // ...
}

14. Python: typecheck_sample.py Not Extended to Lint Surface (crates/mds-python/tests/typecheck_sample.py)

Severity: MEDIUM (80% confidence)

The AC-C6 strict type-check tests (mypy --strict, pyright) don't exercise the new lint, lint_file, lint_virtual, or LintResult stubs. Stubtest verifies parity but not usability; a stub type error manifesting only in real usage would ship undetected.

Fix: Add lint usage samples to typecheck_sample.py:

from mdscript import LintResult
def lint_sample(source: str) -> int:
    r: LintResult = mdscript.lint(source, rules={"unused-variable": "off"})
    total = len(r.files[0]["diagnostics"]) if r.files else 0
    return -1 if r.truncated else total

---

Lower-Confidence Suggestions (60–79%)

Architecture: Backend file-key divergence (WASM "input.mds" vs native "") is intentional per KNOWLEDGE.md but not documented in LintResult TSDoc — add a note that only lintVirtual guarantees backend-stable keys.

Rust:

• Severity enum should derive Copy (fieldless enum, used with .clone() 20+ times)
• compute_line_column widened visibility but never called — either revert or backfill line/column in lint spans
• check_for_var_shadow double-allocates String to search Vec — use iter().any(|v| v == var)
• apply_plan clones and re-sorts already-sorted edits — iterate reversed instead

Performance:

• collect_facts documented as "pre-sized" but uses zero-capacity collections — drop "pre-sized" wording or pre-size against AST size
• 3 local-AST rules re-walk the AST (4 traversals total, not 1) — optional fold into facts walk; soften docs if not done

Complexity: 7× repeated result_exit_code + process::exit idiom invites bugs; add centralized helper.

---

Clean Reviews ✅

Regression & Dependencies: ✅ Clean (verified no breaking exports changes; no new WASM deps; lock file safe)

Security: No findings (input validation at boundaries; no injection points; secrets clean)

Architecture: Lint module is a clean leaf (no resolver/validator reentry); single serializer invariant holds; no binding internals leakage (per verification checks)

Reliability: Engine is bounded (64-depth recursion, 1000-diagnostic cap, reverify gate fail-closed); no production unwrap in core/CLI

---

Summary Table

Category	Finding	Severity	Status
Docs	"all warn default" false	CRITICAL	Must fix
Docs	Phantom --set-rules	HIGH	Must fix
Docs	redundant-else wrong	HIGH	Must fix
API Safety	Unchecked slice panic	MEDIUM	Must fix (public API)
I/O	write_stdout no flush	MEDIUM	Must fix (stdout truncation)
Repo	Handoff doc tracked	MEDIUM	Must fix (gitignore violation)
Testing	Overlap rejection vacuous	HIGH	Should fix
Testing	Block-span refusal untested	HIGH	Should fix
Testing	Exit-3 never asserted	MEDIUM	Should fix
Complexity	CLI twin functions	HIGH	Should refactor
Types	LintSpan missing fields	MEDIUM	Should fix
Types	severity not union	MEDIUM	Should fix
Python	typecheck_sample gaps	MEDIUM	Should extend
Rust	Severity no Copy	LOW	Polish
Perf	collect_facts docs gap	LOW	Polish

Recommendation: ✅ APPROVED_WITH_CONDITIONS
Address the 6 CRITICAL/MEDIUM blocking items before merge. The 5+ HIGH/MEDIUM should-fix items are mechanical refactors that improve safety/maintainability; address before v0.5.0. Lower-confidence suggestions (60–79%) can follow.

---

Review generated by 6 agents (documentation, reliability, testing, complexity, consistency, architecture, rust, typescript, python, performance; regression & dependencies verified clean). All findings are ≥80% confidence unless marked otherwise.

[dean0x]

Truncation limit doc says "default 500", but the cap is a fixed 1000 — BLOCKING (97%)

This line says truncation happens at the "per-file diagnostic limit (default 500)", but the shipped constant is MAX_DIAGNOSTICS: usize = 1_000 (crates/mds-core/src/limits.rs:94), enforced in LintResultBuilder (diagnostic.rs:282; the builder_truncates_at_max_diagnostics test asserts len == 1000). It is a compile-time pub(crate) const — not configurable — so "default" also wrongly implies a tunable knob.

Suggested fix: …capped by the per-file diagnostic limit (**1000** per file). — and drop the word "default" (the cap is a fixed constant, not a configurable default).

---

_{Cycle 2 review · documentation lens · confidence 97% · 🤖 Claude Code}

[dean0x]

JSON example key order contradicts the canonical (alphabetical) wire format — BLOCKING (85%)

This example prints the diagnostic keys as rule, severity, message, help, fixable, span and the file object as file, diagnostics, but to_canonical_json() serializes via serde_json compiled without preserve_order, so keys emit in BTreeMap (alphabetical) order — exactly what the sentence just below (spec.md:971, "Keys are in alphabetical order") and the checked-in goldens assert (crates/mds-napi/__test__/index.spec.mjs:969, crates/mds-python/tests/test_parity.py:162, packages/mds/__test__/lint.spec.mjs:231). The example self-contradicts the alphabetical-order note directly beneath it and the PR's own "byte-identical canonical JSON" guarantee. (The compact-vs-pretty difference is fine; the key order is the substantive contradiction.)

Suggested fix: reorder to diagnostic fixable, help, message, rule, severity, span; span { "length": …, "offset": … }; file object { "diagnostics": […], "file": "template.mds" }. The compact example at CHANGELOG.md:102 is already alphabetical and can serve as the reference.

---

_{Cycle 2 review · documentation lens · confidence 85% · 🤖 Claude Code}

[dean0x]

Footnote ¹ mis-attributes why unused-function is never auto-applied — SHOULD-FIX (80%)

The @import→non-standalone argument is valid for unused-import, but it does not hold for unused-function: that rule fires on an uncalled @define in a file with explicit exports, which can be standalone (no @import/@extends). For such a file is_standalone == true and is_fixable("unused-function", true) == true (tier.rs:41-45; tier_b_rules_fixable_only_standalone), so the Tier B edit is planned and attempted by --fix. It is refused by the block-span reverify gate — removing the single @define line orphans @end → parse failure (footnote ³ / ADR-001) — not by the standalone rule. The "never auto-applied" conclusion is correct, but the stated reason is wrong for unused-function and could mislead a maintainer reasoning about the fix pipeline.

Suggested fix: split the reasoning — keep the @import→non-standalone argument for unused-import, and for unused-function note that its edit targets a multi-line @define block whose single-line removal orphans @end, so the reverify gate refuses it (cross-reference footnote ³).

---

_{Cycle 2 review · documentation lens · confidence 80% · 🤖 Claude Code}

[dean0x]

Tautological assertion — can never fail (dead test) — BLOCKING (92%)

assert "help" in d or d.get("help") is None is always True: when "help" is absent, d.get("help") returns None → the right operand is True; when present, the left operand is True. The assertion provides no coverage — including for the exact regression it appears intended to catch (the binding silently dropping the help field). The canonical lint wire format always carries help (golden: test_parity.py:162 — "help":"Remove the frontmatter key..."). The sibling assertions in this function (rule, severity, message, fixable) are meaningful; only this line is dead.

Suggested fix:

assert "help" in d  # help is always present in the canonical wire format

(or, if absence is genuinely allowed: assert d.get("help") is None or isinstance(d["help"], str)).

---

_{Cycle 2 review · python lens · confidence 92% · 🤖 Claude Code}

[dean0x]

lintVirtual error text says options.modules, but napi/Python say modules — text-only (~80%)

This new lint_virtual takes modules as a positional argument, then validates caps via the shared parse_modules_from_map (:283), which hardcodes the prefix options.modules in every message — e.g. "options.modules exceeds maximum module count of 256 …" (:289), :306, :317. That prefix is correct for the compile/check path (modules arrive via options.modules), but wrong here where no options.modules field exists. The sibling surfaces get it right — napi lintVirtual (crates/mds-napi/src/lib.rs:891) and python lint_virtual (crates/mds-python/src/lib.rs:683,741,751) emit "modules …". It is also internally inconsistent: the non-object guard at :764 already says "modules must be a plain object". Error CODES match (mds::resource_limit/mds::invalid_options), so this is text-only.

Suggested fix: parameterize the field label in the helper — parse_modules_from_map(mods, label) — passing "options.modules" from extract_modules and "modules" from lint_virtual; or inline the cap checks in wasm lint_virtual mirroring napi (crates/mds-napi/src/lib.rs:888-925).

---

_{Cycle 2 review · consistency lens · confidence 85% · 🤖 Claude Code}

[dean0x]

Entry source read from disk but never used in the JSON directory report-only path — SHOULD-FIX (90%)

lint_one_file_accumulating unconditionally calls read_source_file(file) here (a symlink-check + size-capped full-file read + String allocation), but the resulting source is consumed only inside the if fix && !check && !diff branch (plan_and_apply_fixes(result, &source, …) at :798). For the common report-only invocation mds lint --format json <dir>, that branch is never taken, so every file's contents are read + allocated then dropped unused — one wasted disk read + one wasted full-file String alloc per file, scaling O(N) over the tree. The actual lint work is done by mds::lint(file, …) (:770), which reads the file itself and already surfaces read/symlink/size errors via its own Err arm.

Suggested fix: move the read_source_file read inside the fix branch; the report-only path only needs accumulate_result_json(&result, json_files). (Contrast lint_one_file_human, whose read is justified — it needs source for named_source span rendering even in report-only mode.)

Shares a root cause with the mds::lint double-read flagged at crates/mds-core/src/lib.rs:1009.

---

_{Cycle 2 review · performance lens · confidence 90% · 🤖 Claude Code}

[dean0x]

Entry file read 2–3× per lint invocation (redundant I/O + FS-abstraction bypass) — SHOULD-FIX (~82%) · reported by both architecture and performance reviewers

The check gate reads the entry file (resolve_path_intrinsic, :1006), then this std::fs::read(path) at :1009 re-reads the same file for the lint re-parse — byte-identical content. On top of that, the CLI reads a third time via read_source_file(path) before calling mds::lint(path, …) in single-file (crates/mds-cli/src/lint.rs:541), dir-human (:843), and dir-JSON (:756) modes. The KB "re-parse entry independently" invariant is about not sharing the parsed AST (it operates at the string level) — it does not require a second disk read.

Two secondary concerns compound here:

1. FS-abstraction bypass (DIP). This read uses raw std::fs::read and re-implements the MAX_FILE_SIZE guard inline (:1011-1017), bypassing the NativeFs/FileSystem abstraction and its symlink re-check (entry is symlink-validated on the gate read, but not on this one).
2. Latent source-of-truth split. Spans are computed against the copy mds::lint read; the CLI's --fix byte edits and named_source caret rendering operate on the separately-read read_source_file copy — different byte sequences if the file changes between reads.

Suggested fix: thread the entry source out of the resolve step so lint_source reuses it (route any retained read through NativeFs::read), or add a source-accepting public entry (e.g. lint_source_with_filename(source, filename, config)) so the CLI reads once and passes the bytes + the real filename as the JSON file key — today lint_str_with hardcodes "input.mds" (:979), which is why dir mode can't reuse it. Non-urgent (small files, OS-cached), but worth doing while the code is fresh.

---

_{Cycle 2 review · architecture + performance lenses · confidence 82–85% · 🤖 Claude Code}

[dean0x]

Doc says diagnostics without a file are grouped under null, but the code uses "<unknown>" — LOW (82%)

This comment states "diagnostics without a file are grouped under null key", but line 210 uses diag.file.clone().unwrap_or_else(|| "<unknown>".to_string()) — a string key, not JSON null. The branch is currently dead (all 9 rules construct diagnostics with file: Some(filename.to_string())), so there is no runtime impact today, but the comment misdescribes the wire contract for any future rule that omits a file.

Suggested fix: align the comment to the code (grouped under the "<unknown>" key), or — if null grouping is the intended LSP contract — emit the entry under a JSON null deliberately. Since it is unreachable, a one-line doc correction is sufficient:

/// Per-file grouping: diagnostics without a `file` are grouped under the
/// `"<unknown>"` key (currently unreachable — every rule sets `file`).

---

_{Cycle 2 review · rust lens · confidence 82% · 🤖 Claude Code}

[dean0x]

extend_to_line_end returns an out-of-range offset for pos > source.len(), contradicting its documented clamp — LOW (82%)

The doc comment (line 206) states "If pos is past the end of source, returns source.len()." But with let mut i = pos;, the while i < bytes.len() && … loop never runs when pos > bytes.len(), the if i < bytes.len() terminator block is skipped, and the function returns i == pos — a value strictly greater than source.len(), not the promised source.len().

Low impact today: the sole in-crate caller (diag_to_edit, :190) is guarded upstream by source.get(..offset)? (offset ≤ len) and downstream by if line_start >= line_end || line_end > source.len() { return None; }. However, extend_to_line_end is pub and re-exported on the stable fix surface (mds::fix::extend_to_line_end, via crates/mds-core/src/lib.rs:61); an external/binding caller that trusts the documented source.len() clamp and slices with the return value would get an out-of-bounds byte index and panic.

Suggested fix: clamp the entry — let mut i = pos.min(bytes.len()); — so the postcondition matches the doc and the returned offset is always ≤ source.len().

---

_{Cycle 2 review · reliability lens · confidence 82% · 🤖 Claude Code}

[dean0x]

Empty [dev-dependencies] table left behind — LOW (82%)

tempfile was the sole entry under [dev-dependencies]; moving it to [dependencies] (correctly — it is now used on the production --fix atomic-write path) leaves this [dev-dependencies] section empty. Cargo tolerates it, but it is transition residue (quality rule: "leave the end-state, not the transition"). Tests still get tempfile automatically via [dependencies], so the empty table serves no purpose.

Suggested fix: delete the now-empty [dev-dependencies] line, leaving the [target.'cfg(unix)'.dev-dependencies] block intact.

---

_{Cycle 2 review · dependencies lens · confidence 82% · 🤖 Claude Code}

[dean0x]

Repeated empty-body-check-and-recurse pattern (~6 occurrences) — MEDIUM (82%)

The same block — if is_empty_or_whitespace(body) { if !builder.push(make_diag(..)) { return; } } else { check_nodes(body, ..) } — is hand-repeated six times: @for/@define/@message in check_nodes (68-119) and then-body/@elseif/@else in check_if_block (135, 142-191). Each copy differs only in (offset, span-len literal, message, help). The two functions total ~135 lines to express "walk the tree and flag empty bodies"; adding a future block kind means copy-pasting the pattern a seventh time.

Suggested fix: extract one helper and reduce each site to a single call:

/// Returns false when the diagnostic cap is hit (caller should stop).
fn flag_if_empty(
    body: &[Node], offset: usize, span_len: usize,
    message: String, help: String,
    filename: &str, severity: &Severity, builder: &mut LintResultBuilder,
) -> bool {
    if is_empty_or_whitespace(body) {
        builder.push(make_diag(*severity, filename, message, Some(help), offset, span_len))
    } else {
        check_nodes(body, filename, severity, builder);
        true
    }
}

Each arm becomes if !flag_if_empty(&b.body, b.offset, "@for".len(), .., builder) { return; }.

---

_{Cycle 2 review · complexity lens · confidence 82% · 🤖 Claude Code}

[dean0x]

run_lint_file is a ~119-line multi-mode dispatcher — MEDIUM (80%)

The function does config-load, file-read, lint, truncation warning, then branches into three distinct execution modes — the write path (573-601), the --fix --check/--diff preview path (604-625), and report-only (627-633). The length plus the multiple early-exit/early-return modes make the control flow harder to hold in one read than the section comments suggest.

Suggested fix: extract the two --fix branches into run_fix_write(...) and run_fix_preview(...), leaving run_lint_file as a thin ~40-line orchestrator that resolves config/source/result and dispatches.

Note: the FixFileOutcome match inside the write path (576-598) is the third copy already tracked by CPX-1 (#173) — do not fix it in isolation; fold it into that consolidation (a shared FixFileOutcome renderer callable from both the directory-mode lint_one_file_* twins and run_fix_write).

---

_{Cycle 2 review · complexity lens · confidence 80% · 🤖 Claude Code}

[dean0x]

Code Review Summary — Cycle 2 · PR #171 mds lint (#61) · 13 reviewers

Aggregate merge recommendation: ✅ APPROVE WITH CONDITIONS

A strong, almost entirely additive implementation. No CRITICAL or HIGH code defects across any lens. The only true blockers are two factual contradictions in the newly added spec §7.5 plus one dead (tautological) Python test assertion — all three are cheap, high-certainty edits. Everything else is LOW/MEDIUM cleanup or an already-tracked follow-up.

Conditions before merge (all posted as inline comments):

1. spec.md:971 — truncation limit doc says "default 500"; the constant is a fixed 1000 (97%).
2. spec.md:956 — JSON example key order contradicts the alphabetical/byte-parity wire format (85%).
3. crates/mds-python/tests/test_lint.py:207 — tautological help assertion never fails; restores no coverage (92%).

Recommend also landing the two other ≥80% doc/consistency fixes (unused-function footnote reason spec.md:1054; WASM lintVirtual options.modules→modules error text) and the one HIGH should-fix below in the same pass; the LOW/MEDIUM items can be batched or deferred.

Lens	Score	Recommendation	Lens	Score	Recommendation
security	8/10	APPROVED	reliability	9/10	APPROVED
architecture	9/10	APPROVED_W/_COND	rust	9/10	APPROVED
performance	8/10	APPROVED_W/_COND	typescript	9/10	APPROVED
complexity	8/10	APPROVED	python	9/10	APPROVED_W/_COND
consistency	8/10	APPROVED_W/_COND	dependencies	9/10	APPROVED
regression	10/10	APPROVED	documentation	7/10	CHANGES_REQUESTED
testing	8/10	APPROVED_W/_COND

12 inline comments were posted for the ≥80%-confidence findings (3 BLOCKING, 1 dead test, plus should-fix/LOW/MEDIUM anchored items). The two entry-file-read findings from architecture and performance were consolidated into a single comment at crates/mds-core/src/lib.rs:1009.

---

⚠️ HIGH should-fix — no single line anchor, summarized here

Tier B fix application (unused-import/unused-function on a standalone file) is never exercised end-to-end — crates/mds-core/src/lint/fix.rs:480 (tier_b_rules_fixable_only_standalone), testing lens, 85%.
The only Tier B test asserts the is_fixable(rule, is_standalone) classification predicate — it never drives plan_fixes_with_options(result, source, include_tier_b=true) with a real Tier B diagnostic through apply_fixes. Tier B is the subtlest fix path (extra output-byte-equality reverify gate; CLI passes is_standalone into the planner). Every apply_fixes test in fix.rs (693–877) uses a Tier A duplicate-import diagnostic, so the branch that generates a Tier B edit, applies it, and gates on output-neutrality is uncovered — a regression there would ship green.
Fix: add a fix.rs test that plans with include_tier_b=true on a standalone source with an unused selective import, asserts the plan is non-vacuous, applies it, and verifies (a) removal when output is neutral, (b) Rejected when removal would change compiled output, and (c) no Tier B edit with include_tier_b=false / non-standalone.

---

Lower-confidence suggestions (60–79%), by reviewer — non-blocking

testing (edge-path coverage; reviewer-classified as lower-severity suggestions despite confidence)

• unreachable-branch --fix behavior never asserted — fix.rs:8 (85%): declared Tier A/auto-fixable but no test pins apply-vs-fail-closed-refuse.
• "truncated": true never asserted in serialized output — diagnostic.rs builder_truncates_at_max_diagnostics (82%): cap test checks the in-struct bool, not the wire field; all goldens hardcode truncated:false.
• shadow-variable Info-severity exit-code neutrality untested at the CLI — shadow_variable.rs (72%); no CLI test drives config-based rule enabling at all.

documentation

• empty-block coverage list omits @elseif/@else — spec.md:1048, README.md:218, CHANGELOG.md:85 (70%): engine also fires on empty @elseif/@else bodies.

architecture

• LintOptions exposes basePath but CompileOptions hides it — packages/mds/src/types.ts:125 vs :50 (70%): decide parity vs documented divergence.
• Dir-mode JSON re-serializes the full canonical envelope per file just to extract files — crates/mds-cli/src/lint.rs:953 (65%).

performance

• Per-file runtime_vars.clone()/config.clone() in the directory loop — lint.rs:703,:712,:407 (70%): O(files×vars); &HashMap/Arc borrow avoids N deep clones.
• unreachable-branch duplicate-@elseif detection is O(k²) — unreachable_branch.rs:146-203 (65%): deliberate simplicity trade-off (Condition has no Hash); a length guard would mitigate if ever needed.

complexity

• Duplicated untargeted-diagnostic count loops in apply_fixes — fix.rs:340 (75%): extract count_untargeted(...).
• unused_import::check inline diagnostic duplication (Alias/Selective arms) — unused_import.rs:56 (72%).

typescript

• LintOptions.rules is Record<string, string> — a LintRuleSeverity = 'error'|'warn'|'info'|'off' union would catch typos at the boundary — types.ts:119,:133 (75%).
• NapiAddon.lintVirtual modules typed Record<string, unknown> vs Record<string, string> everywhere else — native.ts:28 (68%).
• Repeated inline lint-option shape literal (~4×) — native.ts:26-51 (62%).

python

• LintResult unhashability/frozen enforcement lacks a behavioral test — test_contract.py:153 (70%): add m.lint("Hi\n") to test_unhashable.
• extract_rules parses Severity via from_str(&format!("\"{s}\"")) string interpolation — lib.rs:794 (65%): from_value(Value::String(s)) is cleaner (fail-closed, not a bug).
• lint keyword-arg order (source, *, base_path, vars, rules) diverges from compile's (source, *, vars, base_path) — lib.rs:945 (65%).

consistency

• LintFileResult type name is ambiguous vs the lintFile() function (which returns LintResult) — types.ts:90 (65%).
• napi/wasm/python lint docstrings cite different sibling for error shape (compile vs check) — napi :787 / wasm :669 / python :939 (62%).

rust

• unreachable-branch @elseif diagnostics anchor to the @if offset (AST elseif_branches carries no per-branch offset) — unreachable_branch.rs:135,:164,:193 (68%): imprecise caret; fail-closed.
• fix::plan_fixes/plan_fixes_with_options/apply_fixes lack #[must_use] — fix.rs:122,:130,:340 (66%).

reliability

• Local-AST rule walks lack the defense-in-depth depth-guard comment the facts walk has — unreachable_branch.rs:62 (+ redundant_else.rs:48, empty_block.rs, duplicate_*) (66%): safe (parser bounds nesting to MAX_NESTING_DEPTH=64 pre-check); worth a one-line invariant note.

dependencies

• One-line license/rationale note for the now-production tempfile (# MIT OR Apache-2.0; atomic --fix writes) to match the notify/ctrlc/similar pattern — Cargo.toml:22 (62%).

regression

• Native backend construction now hard-fails if the addon lacks lint/lintFile — native.ts:67 (65%): a facet of the already-documented REG-1 interface expansion (well-mitigated by coordinated single-version release), not a new regression.

security

• sanitize_control_chars does not escape DEL (U+007F) — diagnostic.rs:312-325 (62%): minor defense-in-depth completeness gap; ESC is already caught.

---

Cross-cycle context

• Cycle 2 review. Cycle 1 (2026-07-11) resolved 33/39 items with 0 false positives reported by reviewers this cycle — all prior-cycle fixes were verified to have held and were deliberately not re-raised: DOC-1..7, SPAN-1, RUST-1..4, REL-1, ARC-1..3, TS-2/3, PY-1, CON-1/2, TEST-1..7, PERF-1/2, REG-1.
• Already-tracked follow-ups confirmed still present and intentionally NOT re-reported as new: Refactor mds lint CLI per-file dispatch spine #173 (CPX-1 — lint_one_file_* twins + triplicated FixFileOutcome match), Decompose check_if_block in unreachable-branch rule #174 (CPX-2 — check_if_block length/depth), Centralize Severity string parsing across bindings #175 (XSURF-1 — serde_json::from_str(&format!("\"{s}\"")) severity-parse hack triplicated across napi/wasm/python), Harden terminal-escape rendering across all MdsError variants (CWE-150) #176 (SEC-1 — miette raw-source terminal-escape, CWE-150, 90%), Preserve original file permission mode in atomic --fix/--write writes #177 (SEC-2 — atomic_write_file drops original file mode, 85%), Pin the WASM toolchain and re-tighten the CI size guard #178 (DEP-1 — WASM size guard raised 700K→800K). The run_lint_file:515 inline comment explicitly defers its FixFileOutcome match to the Refactor mds lint CLI per-file dispatch spine #173 consolidation.
• Security tracked items Harden terminal-escape rendering across all MdsError variants (CWE-150) #176/Preserve original file permission mode in atomic --fix/--write writes #177 are pre-existing/repo-wide classes (not introduced by this PR beyond adding another render/write site) and remain out of blocking scope.

---

_{🤖 Generated with Claude Code · code-review orchestration, cycle 2}