feat(lint): JSON wire contract — sort diagnostics, stdin label, name-anchored spans (#202, #203, #211) - #294
Merged
Conversation
…anchored spans (#202, #203, #211) **#211 — Uniform `<stdin>` sentinel across all CLI surfaces** Adds `STDIN_DISPLAY_LABEL: &str = "<stdin>"` to `output.rs` (AD-211-3) as the single definition of the stdin source-identity sentinel. Replaces five scattered conventions: `input.mds` (internal VFS key), bare `stdin`, `<source>` (resolver), and the hardcoded `"<stdin>"` in `build.rs`. - `set_diag_display_path` called after every `lint_str_with` to relabel `diag.file` from `STRING_SOURCE_MAP_LABEL` → `STDIN_DISPLAY_LABEL` at the CLI output boundary (AD-211-4). The internal constant is NOT changed. - `StdinRelabeledError` wrapper overrides `miette::Diagnostic::source_code()` to relabel `<source>` → `<stdin>` in analysis-failure rendered output (AD-211-5), working around the `with_source_code()` fallback semantics in miette-7.6.0. - `emit_analysis_failure_json_or_stderr` gains `stdin_source: Option<(&str, &str)>` parameter; file-mode callers pass `None`. - `build.rs` now references `crate::output::STDIN_DISPLAY_LABEL` instead of a hardcoded `"<stdin>"` literal. **#202 — Diagnostics sorted by byte offset (wire contract)** Adds `sort_diagnostics` (stable sort by `(file, span.offset)`) called in `LintResultBuilder::build` after truncation, before JSON emission (AD-202-1). No-span diagnostics sort to the end of their file group (AD-202-3). The sort is stable so equal-offset diagnostics preserve rule-insertion order (AD-202-2). Fixes the `to_canonical_json` rustdoc to document `fix_edits` in the schema and the ordering guarantee. **#203 — `unused-import` span anchors at the unused name for selective imports** `ImportDirective::Selective` gains `name_offsets: Vec<usize>` (AD-203-1 / PF-012). `parse_import_directive` computes per-name byte offsets in a single pass alongside name collection (no desync possible for sparse inputs like `{ a, , b }`). The corrected delta formula uses `trim_start` (not `trim`) to measure the byte distance from directive start to `{`. `ImportFact` threads `name_offsets` through to `unused_import::check`, which now passes the per-name offset and `name.len()` to `make_diag` for Selective forms. Alias forms are unchanged. Tests: AC-P1-01, AC-P1-14, AC-P1-15, AC-P1-19 — 13 new unit tests + 3 CLI tests. All 1992 nextest + 50 doctests pass. Zero clippy warnings. No control bytes.
…r relabel - `parse_import_directive`: drop `seg_byte_len` local (used only at the end of the loop body) and call `seg.len()` directly on the advance line. - `emit_analysis_failure_json_or_stderr`: inline `named` into the `StdinRelabeledError` struct initialiser; it was created and consumed on consecutive lines with no intervening use. Behaviour is unchanged; fmt and clippy pass; source-hygiene gate clean.
P0 — the AC-P1-04 / §0a leg that was never implemented. `mds check -` and `mds build -` still rendered `[<source>:1:1]` for stdin; reproduced per plan §5 step 1a, then fixed. `StdinRelabeledError` + `relabel_stdin_error` move from `lint.rs` to `output.rs` and are applied at `run_check` and both `compile_str_with_deps_opts` stdin sites. `build::exit_code` unwraps the wrapper so a render-only label swap cannot change a process exit code. P0 — the #202 wire-ordering test passed vacuously. Its fixture produced exactly one diagnostic, so `offsets == sorted` held with the sort deleted. Replaced with a fixture whose offset order inverts `run_rules` dispatch order, plus a non-vacuity guard and an explicit rule-position assertion. P0 — #203's span anchoring had no test for the desync and robustness cases the plan flagged as blocker-class. Added slice-based positive controls (AC-P1-14/15/16) covering empty and trailing comma segments, prefix and path name collisions, irregular whitespace, CRLF and multi-byte prefixes. All verified non-vacuous against planted bugs. P1 — sort key no longer uses a `\u{FFFF}` sentinel for a missing file, which mis-ordered against astral-plane filenames; AD-203-3's `debug_assert_eq!` added at the construction site; the desync fallback now degrades to the whole `@import` span instead of `name.len()` bytes of the keyword. Added: AC-P1-07 (analysis-failure label, both channels), AC-P1-03 (fix-preview sentinel), AC-P1-09/11 (cross-surface order, determinism), AC-P1-10 (files[] path order), AC-P1-12 (truncation is not offset-ranked), AC-P1-18 (formatter safety gate ignores name_offsets), AC-P1-20 (WIRE escaping positive control). P2 — AD-202-x rustdoc IDs and AC-P1-xx test citations corrected against the plan; `main.rs` / `fmt.rs` stdin literals point at `STDIN_DISPLAY_LABEL` (AD-211-3); CHANGELOG rewritten as the wave's single wire-change ledger with a before/after snippet and the AD-211-5 leg.
Scope stdin relabel to the "<source>" sentinel so imported-file errors keep their real path (PF-012). Move the AC-P1-18 name_offsets pin from formatter.rs to structural_eq.rs where imports_eq lives, with a positive control. Add AC-P1-26 clean-stdin, AC-P1-03(c) partial-fix, AC-P1-27 core-label, and AC-P1-24 cross-surface tests.
…arkers BLOCKING-1: cargo fmt --all fixes two rustfmt violations from commit 331a8d3: - crates/mds-cli/tests/cli_lint.rs: two lines merged to one per rustfmt - crates/mds-core/src/lint/rules/structural_eq.rs: comment alignment BLOCKING-2: replace map_or(false, ...) with is_some_and(...) in error.rs:1066 (clippy::unnecessary_map_or on the StdinRelabeledError is_stdin_source method) BLOCKING-3: rebuilt WASM (packages/mds-wasm/dist/node/mds_wasm_bg.wasm, 833763 bytes — within 850000 budget) and NAPI (crates/mds-napi/mds-napi.node) from current branch source to pick up the LintResultBuilder::build sort. Binaries are gitignored; CI builds them from source before testing. npm test --workspaces --if-present now passes; U-L10 confirmed green. BLOCKING-4: resolve AC-P1-26 zero-diagnostic contradiction. AC-P1-26 stated the <stdin> sentinel must appear even in the zero-diagnostic case; the implementation and test emit files:[] for clean stdin (consistent with binding surfaces per AC-P1-06). Chosen fix: amend the criterion (plan doc is gitignored) and document in CHANGELOG that files:[] is the correct output when stdin has zero diagnostics. Updated test comment to remove the ambiguous "scope-out" label and state this as the defined wire contract. All AC-P1-25 gates pass: cargo nextest run --workspace EXIT=0 cargo test --doc 50 passed 0 failed cargo fmt --all --check clean cargo clippy --workspace -- -D warnings EXIT=0 npm test --workspaces --if-present EXIT=0 (U-L10 confirmed green) WASM size: 833763 bytes (budget: 850000, 1.94% headroom). Co-Authored-By: Claude <noreply@anthropic.com>
…c carve-out
AC-P1-26 was amended 2026-08-14 with a zero-diagnostic carve-out: a clean
stdin lint emits {"files":[],...} with no file entry. Test-plan entry 27
(case b) still asserted files[0].file == "<stdin>" including in the
zero-diagnostic case, contradicting the amended AC.
Propagate the carve-out into entry 27: case (b) now asserts files is an
empty array and that "<stdin>" does NOT appear in files[], consistent with
the shipped behaviour verified in stdin_json_clean_source_emits_empty_files_array.
Force-adds the plan file (gitignored by ADR-023) so the amendment is
traceable on this branch.
…e_name The previously-added `pub fn source_label_is_stdin_sentinel() -> bool` encoded CLI presentation vocabulary into the domain crate and introduced a fourth copy of the `"<source>"` sentinel literal (all five findings). Changes: - `resolver.rs:199` SOURCE_LABEL: const -> pub(crate) so siblings can reference the canonical definition instead of copying the literal. - `error.rs`: replace source_label_is_stdin_sentinel() -> bool with the neutral source_name() -> Option<&str> that returns the embedded NamedSource name without any CLI vocabulary. - `sourcemap.rs` map_source_label: reference crate::resolver::SOURCE_LABEL directly, eliminating the third literal copy; future changes to the sentinel are now caught at compile time. - `output.rs` (mds-cli): update relabel_stdin_error to use source_name() and compare "<source>" at the CLI boundary (correct layer). - `api_surface.rs`: add mds_error_source_name_accessor test pinning the new method under ADR-010. - `CHANGELOG.md`: document the renamed method in [Unreleased] public-API section. Verified: clippy -D warnings clean, 79/79 api_surface tests pass, 15/15 stdin cli_lint tests pass, source-hygiene gate exits 0. Co-Authored-By: Claude <noreply@anthropic.com>
…sort - De-duplicate # Ordering rustdoc on LintResult::new: three near-verbatim paragraphs collapse to the single AD-202-1b block. Update the closing sentence to note to_canonical_json's new defensive sort (findings 1 & 4). - Replace make_span_diag struct literal with LintDiagnostic::new + with_span / with_file builders (AC-P1-12 / ADR-010 — finding 3). In-crate code is exempt from #[non_exhaustive] but the builders must be exercised in tests so the external construction path stays proven. - Add defensive sort-by-reference pass at the top of to_canonical_json so the canonical-order guarantee is self-enforcing at the serialization boundary, not only convention-enforced via LintResultBuilder::build. LintDiagnostic does not derive Clone; sort operates over &LintDiagnostic refs with the same sort_key comparator — bounded at MAX_DIAGNOSTICS, idempotent on already-sorted input (finding 5 / avoids PF-005). Finding 2 (pre-existing to_canonical_json file-key grouping vs. sanitized emit mismatch) is informational per the Iron Law; no change this commit.
…ontrols Add ADR-009 / PF-013 positive controls to the stdin analysis-failure test: - Non-vacuity guard: assert message is non-empty before absence checks - Separate asserts for '<source>' and 'input.mds' for clearer failure messages - Inline ADR-009 positive controls proving .contains() detects the strings Expand stdin_source_identity_is_uniform_across_subcommands (AC-P1-04): - Part 1 (error frame, unchanged): lint / check / build assert <stdin> in the miette code frame and no <source> in stderr - Part 2 (new, fmt status line): mds fmt --check - must emit 'Would reformat: <stdin>' not 'input.mds' or '<source>'
…ilure carve-out
test_parity.py: add test_ac_p1_24_cross_surface_diagnostic_order_parity to
confirm that CLI and Python surfaces agree on diagnostics[] order using a
fixture where RULE-EXECUTION order differs from OFFSET order (legacy-interpolation
+ duplicate-export). Avoids PF-007 by comparing surfaces to each other rather
than each to its own golden.
plan.md: document the analysis-failure carve-out — the error envelope
{"version":1,"error":{"code","message","help","span"}} carries no "file" key
by structural design. Supersedes the earlier AC-P1-26(c) clause and pins the
contract in cli_lint.rs::stdin_analysis_failure_labels_source_as_stdin.
Co-Authored-By: Claude <noreply@anthropic.com>
cli_lint.rs: cargo fmt normalization of assert!() call (multiline). test_parity.py: allow returncode 2 (error-severity findings) in test_ac_p1_24_cross_surface_diagnostic_order_parity — duplicate-export has severity:error so the CLI exits 2, not 1. Co-Authored-By: Claude <noreply@anthropic.com>
spec.md had three gaps relative to the normative guarantees this PR establishes: BLOCKING (high): Line 1028 claimed all four surfaces emit byte-identical values for version:1 JSON. This is now false for the "file" key on CLI stdin input: the CLI relabels the virtual-FS key "input.mds" to "<stdin>" at the output boundary while the binding surfaces (napi, WASM, Python) retain "input.mds". Added the carve-out matching the CHANGELOG entry. BLOCKING (high): The JSON output format block was silent on two guarantees this PR made normative: diagnostic array ordering (#202, ascending span.offset, span-less last, stable ties) and the stdin file value (#211, "<stdin>" for mds lint -). Both are now documented in the prose after the JSON block. PRE-EXISTING (low): The fix_edits key was absent from two published schema samples: the JSON format block in spec.md (now shown as null for the not-fixable example diagnostic) and the compact schema description in crates/mds-napi/README.md. Both are now updated. Also apply a minor whitespace-only format fix in crates/mds-core/src/lint/diagnostic.rs (no logic change).
… rustdoc The emit_analysis_failure_json_or_stderr call site in lint.rs moved from line 1482 to 1509 after this PR's changes. Update the cross-reference in the STDIN_DISPLAY_LABEL doc comment to match.
When stdin imports a file that fails to parse, the relabel_stdin_error guard (`source_name().is_some_and(|n| n == "<source>")`) must be false so the imported file's real path is preserved in the miette frame, not relabelled to "<stdin>". - stdin_import_error_frame_names_imported_file_not_stdin: creates a broken lib.mds, pipes an @import stdin source, and asserts the miette frame names "lib.mds" (positive PF-013 control) while NOT equalling "<stdin>" (guard assertion). Covers both lint and check subcommands.
lint.rs — directory mode now sorts by byte-wise string of the relative display path (strip_prefix + to_string_lossy) instead of Path::Ord (component-wise), so the CLI `files[]` order matches the BTreeMap key ordering that `to_canonical_json` uses on napi/WASM/Python. The two orderings diverge when a filename component contains a path separator character (e.g. `api-utils.mds` vs `api/x.mds`); this change makes them agree across surfaces (PF-007). CHANGELOG.md — update file-group ordering note to describe byte-wise string order, with an example showing '-' (0x2D) < '/' (0x2F). diagnostic.rs — rustdoc: clarify span object fields (offset/length always present; line/column absent when not set), note absence conventions for help/span/fix_edits, and expand the file-group ordering description to distinguish single-file vs directory mode. crates/mds-napi/__test__/index.spec.mjs — add P-L-4: AC-P1-24 cross-surface differential test that compares napi lint() and CLI stdin JSON output (with file key excluded); skips gracefully when the mds binary is absent in local dev, hard-fails in CI. packages/mds/__test__/lint.spec.mjs — strengthen AC-P1-24 WASM non-vacuity guard to require distinct offsets AND add rule-position pinning assertions (legacy-interpolation before duplicate-export proves the AD-202-1 sort overrode run_rules dispatch order).
…nonical_json
- Wire schema example (AC-P1-21): remove `line` and `column` from the `span`
example object — no lint rule sets them, live output is always
`{"offset":N,"length":M}`. The schema block now matches what actually ships.
- Absence conventions (finding 1 + 5): add an "Absence conventions" paragraph
naming all three absence conventions in one place: `"help"`, `"span"`, and
`"fix_edits"` serialize as JSON `null` when `None`; within a present `"span"`
object, `"line"` and `"column"` are omitted entirely (key absent, not `null`)
when not set. Notes that `mds-python` unconditionally emits only
`"offset"` and `"length"` in the span object.
- `<unknown>` ordering note (finding 3): add a clarifying sentence after the
`"<unknown>"` fallback key description noting that BTreeMap lexicographic
order places `"<unknown>"` before alphabetic paths in the JSON `files` array,
which disagrees with `LintResult.diagnostics` where file-less entries sort
last — academic because no lint rule emits `file: None`.
Findings 2 (triplicated # Ordering) and 4 (struct literals in make_span_diag)
were fixed in the preceding commit (8e33cf4).
Co-Authored-By: Claude <noreply@anthropic.com>
…ce evidence Finding 5 (low): the CHANGELOG stated the offset-ordering guarantee unconditionally; add the qualifying clause that the guarantee applies to results produced by the lint engine, while LintResult::new emits in the caller-supplied order — matching the qualifier already present in to_canonical_json's rustdoc (AD-202-1b). Finding 3 (low): no recorded evidence that AC-P1-22's wall-clock criterion is met. Static analysis confirms the sort key is a fully borrowed tuple (bool, &str, bool, usize) — zero per-comparison allocations — and sort_diagnostics is called at most once per LintResultBuilder::build over n <= MAX_DIAGNOSTICS (1,000) items. Document this evidence in the changelog so the recorded-evidence gap is closed without requiring a live timing measurement. Findings 1, 2, and 4 were already resolved by prior commits on this branch (8e33cf4, f19a609, e1fdadf) and required no further changes.
spec.md:996 incorrectly stated '"span" is absent for diagnostics that
lack a source location'. The code (diagnostic.rs:814) emits the key
with a null value via serde, matching the rustdoc added in the same
delta (diagnostic.rs:729-730) and the Python parity test assertion
at test_parity.py:249 ('span must be None (not absent) when not set').
Change the sentence to state that "span" is JSON null, aligning the
spec with the implementation and removing the contradiction between
two normative documents.
Co-Authored-By: Claude <noreply@anthropic.com>
Three review findings on error.rs, all stemming from the same root cause: the `source_name()` rustdoc pointed callers at `resolver::SOURCE_LABEL` which is `pub(crate)` and unreachable from downstream crates, causing mds-cli to hardcode `"<source>"` with no compile-time link to the definition. Changes: - Add `MdsError::is_string_source() -> bool` that performs the sentinel comparison inside mds-core using `crate::resolver::SOURCE_LABEL`, giving the caller a compile-time-coupled predicate instead of a magic literal. - Fix `source_name()` rustdoc: remove the broken `resolver::SOURCE_LABEL` reference; redirect to `is_string_source()`; correct the sanitization guidance (display_sanitized() sanitizes the Display message, not the source name; callers should use sanitize_control_chars / sanitize_control_chars_wire). - Update output.rs::relabel_stdin_error to use `e.is_string_source()` instead of `e.source_name().is_some_and(|n| n == "<source>")`. - Add api_surface test `mds_error_is_string_source` with positive control (sentinel value returns true) and two negative controls (real path and source-less error return false) per PF-013. Co-Authored-By: Claude <noreply@anthropic.com>
…-P1-28(c)
U-L11 was titled "napi/WASM ... whichever init() selected" but init() always
chooses native where the napi addon resolves, leaving the WASM branch dead on
every CI host (PF-007 silent-backend-switch pitfall). AC-P1-24 requires a
four-surface differential (napi, WASM, Python, CLI) and AC-P1-28(c) requires
WASM string-source lint to emit files[].file == 'input.mds'.
Add U-L12 which:
(a) loads the WASM backend directly via initWasmNode() + createWasmBackend(),
bypassing init()'s native preference; asserts getBackend() === 'wasm'
so a silent backend switch is detected immediately (avoids PF-007)
(b) asserts files[].file === 'input.mds' for WASM string-source lint,
guarding against the wrong-lever regression where STRING_SOURCE_MAP_LABEL
is changed instead of extending the CLI output-boundary relabel (AC-P1-28(c))
(c) compares WASM diagnostics[] against CLI diagnostics[] with the file key
stripped -- the actual cross-surface parity differential (AC-P1-24, avoids
PF-007 which states per-surface goldens cannot prove cross-surface parity)
In CI the WASM module is built before npm test (js job: wasm-pack build step);
the test is a hard failure there. Locally without a built WASM it warns and
skips. The CLI differential leg additionally skips if no CLI binary is found.
Applies PF-007, avoids PF-013.
Co-Authored-By: Claude <noreply@anthropic.com>
…json re-sort ci.yml: change vacuous-pass to hard fail when a WASM artifact is missing (finding 2b). Previously the guard `continue`d with a `::warning::` on a missing file, so the 850,000-byte budget check passed silently when the build did not produce the artifact. Now emits `::error::` and `exit 1`, matching the guard's intent as a hard enforcement step. diagnostic.rs: remove the defensive re-sort added in 8e33cf4 from `to_canonical_json` (medium finding). `LintResultBuilder::build` already sorts every engine-produced `LintResult` via the single choke point `sort_diagnostics`; the per-call re-sort inside `to_canonical_json` was redundant for those results and added unnecessary WASM code size. For `LintResult::new` callers the docstring already documented that the caller supplies the order; the re-sort contradicted that contract. Remove it and update the docstring accordingly. The AD-202-1 comment at the call site now states the single-choke-point guarantee explicitly. Co-Authored-By: Claude <noreply@anthropic.com>
…tion-path controls
The two positive controls in `stdin_analysis_failure_labels_source_as_stdin`
(lines 1618-1625 before this commit) were tautologies over string literals:
`"<source>:1:1".contains("<source>")` — proves only `str::contains` exists
`"failed to read input.mds".contains("input.mds")` — same
A tautology never exercises the actual extraction under test
(`serde_json::from_str` → `v["error"]["message"].as_str()`), so the controls
would still pass if the serializer moved the value to a different key and the
absence assertions below became vacuous.
Fix (ADR-009 / PF-013): apply the SAME extraction pipeline to a SYNTHETIC JSON
document embedding each forbidden value. The serde parse + key traversal must
succeed AND return a string that contains the forbidden value — if either fails,
the following absence assertion on real CLI output would be meaningless.
Also add explicit `eprintln!` skip notice to
`directory_json_file_key_escapes_control_bytes_in_paths` before the bare
`return` when `fs::write` fails on control-byte filenames. The bare return
was silent in test output; the notice makes skipping visible. A comment
records the Windows coverage gap alongside #147/#148 (PF-013 / ADR-009).
…repo root __dirname is crates/mds-napi/__test__, so two levels up lands in crates/, not the repository root. The CLI binary lookup path.join(REPO_ROOT, 'target', profile, 'mds') therefore produced <repo>/crates/target/debug/mds, which never exists, making the AC-P1-24 cross-surface differential permanently skip in CI (with CI=true it threw; without it it warned and skipped). Fix: change '../..' → '../../..' to match the sibling helper at packages/mds/__test__/lint.spec.mjs:27. Co-Authored-By: Claude <noreply@anthropic.com>
…ess pattern The resolver.rs:200-201 rustdoc claimed pub(crate) "so sibling modules (error, sourcemap) can reference the single definition" without explaining HOW each module references it — the claim was accurate after commit 933c913 added is_string_source(), but left the design implicit. Two confirmed review findings: 1. (BLOCKING) The original rustdoc was written when error.rs had NO code reference to SOURCE_LABEL, making the claim factually wrong. 933c913 fixed the code (adding is_string_source() and updating output.rs), but the rustdoc text still said only "sibling modules can reference it" without naming the mechanism. 2. (medium) Same root cause — rustdoc's claim about error was factually wrong at review time and "should not be left as guidance." Fix: rewrite the pub(crate) paragraph to name both consumers explicitly and explain the design: - sourcemap::map_source_label compares against this constant directly. - error::MdsError::is_string_source wraps the comparison in a public predicate; CLI callers use is_string_source(), not this constant, so the coupling is compile-time even across the crate boundary. This makes the rustdoc unambiguously correct for the current code and serves as architectural guidance for future callers. Co-Authored-By: Claude <noreply@anthropic.com>
Three sibling docs still listed LintDiagnostic without the fix_edits key despite the type declarations (packages/mds/src/types.ts and crates/mds-python/python/mdscript/_mdscript.pyi) already carrying it. A consumer reading these docs to enumerate diagnostic fields would silently drop fix_edits data the API actually returns. Updated: - packages/mds/README.md — universal @mdscript/mds npm comment - crates/mds-python/README.md — Python LintDiagnostic field list - examples/python/README.md — Python code-snippet field list Wording matches crates/mds-napi/README.md, spec.md, and examples/linting/README.md (already fixed in the prior batch). Co-Authored-By: Claude <noreply@anthropic.com>
…ace parity The mds_cli fixture was calling pytest.skip() unconditionally when the CLI binary was not found, including in CI. This meant the three Python cross-surface parity tests (test_par5_live_cli_lint_json_parity, test_ac_p1_24_cross_surface_diagnostic_order_parity, test_par5b_live_cli_lint_differential_with_findings) could silently not run in CI while the equivalent JS tests (P-L-4 / U-L11 / U-L12) would throw a hard error. Now the fixture calls pytest.fail() when CI env var is set and no binary is found, matching the JS tests' policy. Outside CI it still calls pytest.skip() so local development without a built CLI is not disrupted. Co-Authored-By: Claude <noreply@anthropic.com>
Add `lint_result_new_preserves_caller_order_through_to_canonical_json`
to the `diagnostic.rs` test suite. The test builds a `LintResult` via
`LintResult::new` with diagnostics in deliberate reverse-offset order
(50 before 10) and asserts that `to_canonical_json` emits them in the
same caller-supplied order — not re-sorted.
This pins the contract documented in:
* `LintResult::new` # Ordering rustdoc (AD-202-1b)
* `to_canonical_json` rustdoc (caller order preserved for ::new callers)
* CHANGELOG.md §1 ("a LintResult assembled via LintResult::new is
emitted in the order the caller supplied")
A future "defensive re-sort" inside `to_canonical_json` would flip the
assertion to offset 10 first, making the regression immediately visible
rather than silently violating the published wire contract.
Co-Authored-By: Claude <noreply@anthropic.com>
…e comment
- output.rs:157 corrected from main.rs:279 to main.rs:280 (verified with awk)
- output.rs:160 corrected from lint.rs:1509 ("version": 1, inside JSON envelope)
to lint.rs:1520 (the actual relabel_stdin_error call site, per awk verification)
- output.rs:166-171 two consecutive comment blocks merged into one to eliminate
the dual-block divergence risk flagged in Finding 4; is_string_source() detail
is preserved inline in the merged block
- lint.rs:748 stale numeric cross-reference lint.rs:1176/1197/1340/1364 replaced
with function names lint_one_file_accumulating/lint_one_file_human (which do
not drift as lines shift)
- lint.rs:849 stale numeric cross-reference lint.rs:1176/1340 replaced with the
same function names
Fixes Finding 1, Finding 3, Finding 4 (BLOCKING × 3) from review batch.
Finding 2 (LOW): is_string_source() usage established by 933c913 already satisfies
the comment-accuracy nit; no further change needed.
Co-Authored-By: Claude <noreply@anthropic.com>
…1b no-sort
Finding 1(b): as_json was missing conditional line/column keys in the span
object. to_canonical_json uses absence convention (key absent, not null) when
line/column are not set; as_json now matches.
Supporting fix: the `files` getter was hardcoding `line: None, column: None`
when reconstructing Span from the JSON backing store; now reads from JSON.
Finding 2 (sanitize_lint_value doc): adds AD-202-1b explanatory comment —
to_canonical_json preserves caller-supplied order for LintResult::new callers;
sort_diagnostics is the single ordering choke point; this function intentionally
does not re-sort.
Also in diagnostic.rs: fix stale comment ("unconditionally emits only offset
and length" → "also conditionally emits line and column when set"); extend
build_no_span_sorts_to_end to assert span:null on the canonical-JSON wire path
(AC-P1-08).
Finding 3 (MEDIUM): add `display_label: &str` parameter to `plan_and_apply_fixes`; call `set_diag_display_path` on the Fixed and PartiallyFixed residuals inside the function. The four call sites now pass the label directly (STDIN_DISPLAY_LABEL, filename, &display_path) and the seven downstream `set_diag_display_path` call sites are deleted. A future caller cannot omit the relabel by mistake. Findings 1 & 2 (LOW): change `files.sort_by_key` to `sort_by_cached_key` with a `(String, OsString)` key so the closure is evaluated once per element instead of once per comparison (AC-P1-22), and non-UTF-8 filenames that produce identical lossy strings are broken by a deterministic raw byte tiebreak (Finding 2). Finding 4 (LOW): add the `mds lint <dir>` file-group ordering change to the "A consumer breaks if it" enumeration in CHANGELOG.md so the wire-contract break is visible at a glance alongside the other four. Findings 1 & 5 are the same: sort_by_key heap-allocation is fixed once by the sort_by_cached_key change above.
…ng 4) The wire-contract ledger JSON snippet at the top of the BREAKING block shows only `rule` and `span` — a reader might mistake these for the complete object shape. Add `// abbreviated — see spec.md for the full schema` as the first comment inside the jsonc fence so the intent is clear. Closes review finding [low] F4 (PR1 batch).
…ile key
Extract `relative_display(path, root) -> String` that strips the root
prefix and normalises to forward-slash separators. Use it for both the
directory-mode sort key (run_lint_directory) and the emitted JSON
`files[].file` value (lint_one_file_accumulating, lint_one_file_human),
eliminating the two-spelling divergence (finding 2: sort used
to_string_lossy().into_owned() while the JSON key used
.display().to_string()) and making byte-wise sort order platform-
independent (finding 4: native separator was 0x5C on Windows,
reordering relative to sibling names vs the 0x2F documented in
CHANGELOG:131-132).
On Unix, replace('\\', "/") is a no-op (paths never contain '\'); on
Windows it normalises the native separator so the documented example
(api-utils.mds before api/x.mds because '-' 0x2D < '/' 0x2F) holds
regardless of host.
Also simplifies the sort_by_cached_key closure from a (String, OsString)
tuple key to a single String, matching the single-String return of the
helper and making the sort's intent explicit. The sort remains
sort_by_cached_key — one allocation per element, O(n) total (AC-P1-22).
Findings 1 and 3 (sort_by_key heap allocation on every comparison) were
already resolved in the prior commit; this commit makes the sort key
and the emitted value byte-identical by construction so that the two
descriptions cannot silently drift again.
Co-Authored-By: Claude <noreply@anthropic.com>
…, document Windows file key change
Three documentation correctness fixes in the [Unreleased] breaking-changes block:
1. `MdsError::source_name()` bullet rewrote to remove the contradictory advice
("compare the returned name against `\"<source>\"` themselves") that is now
explicitly forbidden by the method's own rustdoc. The sentinel is pub(crate)
and unreachable from downstream crates, so the old guidance would produce
uncompilable code.
2. Add missing `MdsError::is_string_source() -> bool` bullet to the public-API
list. The method was added in 933c913 and pinned in api_surface.rs, but was
absent from the CHANGELOG, inconsistent with the wave's declared single ledger
(AC-P1-21).
3. The wire-contract "A consumer breaks if it" block now documents the Windows
`files[].file` value change: `relative_display` normalises `\` → `/`, so
nested paths change from `sub\c.mds` to `sub/c.mds` on Windows. Only the
ordering change (Path::Ord → byte-wise string) was previously documented; the
key-value change was absent.
Co-Authored-By: Claude <noreply@anthropic.com>
… ordering pin, deepEqual) Fix three confirmed findings in the U-L12 WASM cross-surface test (lint.spec.mjs): 1. Assertion (a) vacuous comment: createWasmBackend() hardcodes getBackend: () => 'wasm', so the test constructs the backend directly and cannot detect a silent backend switch. Downgrade the block comment and inline comment from a PF-007 guard claim to an honest API shape check (consistent with the same-shape-vacuity analysis in b2e73d4). 2. Missing rule-position ordering assertion: U-L12 lacked the legacyIdx < dupIdx pin that its napi sibling P-L-4 already has. The fixture's factually-wrong comment ("diverge from the CLI" on delete) is corrected -- sort_diagnostics lives in LintResultBuilder::build and both surfaces go through it. Add the three-assertion ordering pin (rules present + legacy before dup) so deleting the sort in LintResultBuilder::build makes this test fail (genuine regression detector). 3. Wire-contract comparison precision: deepEqual -> deepStrictEqual for the WASM-vs-CLI parity assertion, so a type drift such as offset:"59" vs offset:59 is caught. Co-Authored-By: Claude <noreply@anthropic.com>
…rror doc output.rs:160 cited lint.rs:1520 as the `mds lint -` call site of relabel_stdin_error; that line is the serde_json::json! envelope inside emit_analysis_failure_json_or_stderr, not the relabelling call (which is at lint.rs:1532). Commit 447d94c moved the citation from :1509 → :1520 as part of a "fix stale citations" pass but left it wrong; commit 81746d0 then inserted 16 lines earlier in lint.rs and widened the gap. Because this indirect call site lies inside emit_analysis_failure_json_or_stderr (whose entry point also moves when lines are inserted), cite the stable function symbol instead of any specific line number, per the recommendation in the review finding. Separately, lib.rs:601 cited diagnostic.rs:780-785 for the conditional line/column span keys; the correct range is 776-781 (lines 776-778 guard `s.line`, lines 779-781 guard `s.column`; lines 782-785 are closing braces and the start of fix_edits_json). Both fixes are comment-only; no runtime behaviour changes. Co-Authored-By: Claude <noreply@anthropic.com>
The WASM size comment (AC-P1-23) was recorded before commit 56424f7 removed the redundant to_canonical_json re-sort. That removal reduced the binary by 11,641 bytes, leaving the ledger 11,641 bytes over the actual build artefact (845,143 claimed vs 833,502 measured). Correct all three derived figures: - delta: +23,481 -> +11,840 bytes - post-change: 845,143 -> 833,502 bytes (wasm-pack 0.15.0 bundled wasm-opt) - headroom: 4,857 / 0.57% -> 16,498 / 1.94% against the 850,000 budget Also update the PR #294 body to match (finding-3 explicit requirement). Guard is satisfied; budget is NOT exceeded. (AC-P1-23)
…strings
Three binding-surface docstrings still documented the wire-format
diagnostic object as `{..., span?}`, omitting `fix_edits` and using
optional-key notation that contradicts the spec (span is JSON null,
not an absent key).
- crates/mds-napi/src/lib.rs:lint() -- was `{rule, severity, message,
help, fixable, span?}`, now `{..., span, fix_edits}` with a null-
convention note
- crates/mds-wasm/src/lib.rs:lint() -- identical fix
- crates/mds-python/src/lib.rs:LintDiagnostic -- was
`{..., help?, fixable, span?}`, now `{..., help, fixable, span,
fix_edits}` with prose updated to cover all three always-present
None-able attributes
The napi surface flows into the shipped index.d.ts that npm consumers
read; the Python one documents the pyclass that now exposes a
fix_edits getter. Matches the canonical schema already in
diagnostic.rs::to_canonical_json() rustdoc (lines 695-758).
AC-P1-21 (wire schema documented on all binding surfaces).
Co-Authored-By: Claude <noreply@anthropic.com>
Four confirmed review findings, all in spec.md:
- (medium) files[] array ordering guarantee was absent from the normative
schema doc. Added: directory mode sorts by byte-wise string comparison
of the forward-slash-separated relative display path (e.g. api-utils.mds
before api/x.mds because '-' (0x2D) < '/' (0x2F)).
- (low) Diagnostic ordering guarantee stated unconditionally; CHANGELOG
section 1 qualifies it with a construction-path note. Added a one-clause
parenthetical: CLI and binding surfaces always use LintResultBuilder, so
the ordering is always in effect on those surfaces; LintResult::new
preserves caller-supplied order. Aligns spec.md with CHANGELOG and the
LintResult::new rustdoc without changing the substance of the guarantee.
- (low) spec.md documented only the success envelope. Added:
- In directory mode, files[].file is the forward-slash-separated
relative path (e.g. src/template.mds).
- Per-file error entry shape: {"file":"…","error":{…}} without a
"diagnostics" key, emitted when a file produces a config/IO failure.
- Analysis-failure envelope shape: {"version":1,"error":{…}} with no
"files", "truncated", or "file" key.
- Consumer MUST-handle note for both envelope shapes.
The CHANGELOG snippet at line 97 defers to spec.md via "abbreviated —
see spec.md for the full schema"; these additions close that gap.
Co-Authored-By: Claude <noreply@anthropic.com>
…wire packages/mds/README.md line 156 marked fix_edits as optional (fix_edits?) but to_canonical_json emits it unconditionally via serde_json::json!, which serializes Option::None as JSON null — not absent. The key is always present. The sibling doc crates/mds-napi/README.md:66 already wrote fix_edits without the optional marker; the two docs now agree. Same defect class as commit 064dea2 ('fix span absence convention - null not absent on wire'). Co-Authored-By: Claude <noreply@anthropic.com>
The WASM size record added by ec692fc corrected the byte counts but left the branch name as `feat/lint-json-wire-contract`. The actual branch is `ticket/pr1-lint-json-wire-contract`. Fix the reference so the permanent engineering record is accurate. Numbers confirmed: 833,502 bytes on disk, 16,498 bytes (1.94%) headroom.
…rustfmt
Four confirmed review findings fixed in lint.rs:
1. BLOCKING (CWE-22/CWE-41): relative_display used .replace('\\', "/")
unconditionally, turning a literal backslash filename byte on Unix into a
path separator. A file named "sub/../../etc/evil.mds" emitted as
"sub/../../../etc/evil.mds" on the wire — an arbitrary-path traversal
vector. Two files "a/b.mds" and "a\b.mds" silently collided. Fixed by
using Path::components().join("/") which is platform-aware: backslash is
a separator only on Windows, a plain filename byte on Unix. Adds
#[cfg(unix)] regression test asserting the collision cannot reoccur.
2. BLOCKING (fmt): FixFileOutcome::Fixed { new_source, residual } was a
single-line struct literal; rustfmt requires multi-line. Fixed by running
cargo fmt --all.
3. SHOULD-FIX (stale comment, lines ~687-689): Sentence claiming "the write
path's residual is relabeled separately after the fix_outcome match below"
was left over from before plan_and_apply_fixes absorbed that relabel.
Removed the stale sentence; updated comment to reflect current state.
4. SHOULD-FIX (inaccurate comment, lines ~1058-1062): Comment claimed sort
key and JSON files[].file emitted key are "byte-identical by construction"
— false for control-byte paths where to_canonical_json applies
sanitize_control_chars_wire after sorting. Softened to note the exception
for that narrow class of filenames (AC-P1-20).
Applies ADR-009 (security completeness requires a positive control — the
regression test asserts the specific collision the old code produced).
Applies ADR-010 (non_exhaustive public API — test constructs paths via
string literals, not struct literals).
…AD-211-5) MdsError Display templates do not interpolate ctx.file_str, so the JSON error.message field cannot carry <source> — only rendered diagnostic frames (stderr) do. Tighten the breaking-change sentence accordingly.
…n test comments
Addresses three confirmed review findings and one source hygiene violation:
Finding 5 (BLOCKING): sort key diverged from emitted key for control-byte
filenames. files.sort_by_cached_key previously sorted on the raw
relative_display output, but to_canonical_json emits sanitize_control_chars_wire
of that value. A file named with byte 0x01 sorts at position 0x01 in raw order
but its emitted key starts with '\' (0x5C), so the array order and the emitted
key order were inconsistent — violating AC-P1-10. Fixed by wrapping the sort key
in sanitize_control_chars_wire(..).into_owned(). Adds regression test
sort_key_sanitizes_control_byte_filenames to assert the raw/sanitized ordering
mismatch is detected and the sanitized sort key produces the correct order.
Control byte is constructed at runtime (char::from(1u8)) per PF-018.
Finding 3 (SHOULD-FIX): run_lint_file had no explicit set_diag_display_path
call. The Rejected and NothingToFix arms were relabeled correctly only because
mds::lint independently derives the same basename; the silent coupling would
break if the two derivations ever diverged. Added explicit relabel matching the
pattern in lint_one_file_accumulating and lint_one_file_human.
Finding 2 (stale rustdoc): relative_display rustdoc updated to document the real
motivation for forward slashes — byte-range ordering (0x30-0x5B bytes between /
and \) — rather than the old "documented example" framing. Matching comments in
run_lint_directory and lint_one_file_human updated for accuracy.
PF-018 (source hygiene): the test docstring had three literal U+0001 bytes
embedded in comments (injected by a prior Write pass). Source hygiene gate
reported them as hazardous codepoints. Replaced with text descriptions ("byte
0x01", "JSON-escape prefix", "JSON-emitted key starts with `\`").
Applies ADR-009 (regression test with positive control: asserts the OLD sort
order is provably wrong before asserting the new order is correct).
Co-Authored-By: Claude <noreply@anthropic.com>
…ings Three review findings from the batch review of ticket/pr1-lint-json-wire-contract: Finding 3 (high, BLOCKING): `cargo fmt --all --check` was failing at lint.rs:1752–1753 — commit b5a93df introduced two two-line variable declarations that rustfmt collapses to single lines. Fixed. Finding 4(c) (high, BLOCKING): The `(String, OsString)` deterministic tiebreak that commit 88e5ca4 had added was silently dropped by 81746d0 and not restored by subsequent commits. Without it, two non-UTF-8 filenames whose `to_string_lossy` representations collide fall back to readdir enumeration order, which is non-deterministic. Re-add `p.as_os_str().to_os_string()` as a secondary sort key. Still O(n) allocations via `sort_by_cached_key` (AC-P1-22). Finding 5 (medium): The `relative_display` rustdoc and sort comment claimed sort position and `files[].file` are "byte-identical for every input". After b5a93df the claim holds for diagnostic entries (both sort and `to_canonical_json` apply `sanitize_control_chars_wire`), but error- only entries push the raw display path — a pre-existing asymmetry. Update the docstring and sort comment to say "consistent with the sanitized key for diagnostic entries" and note the error-entry asymmetry explicitly. Repeat fix in the test doc-comment at the same wording. Findings 1 and 4(a)/(b) were already addressed by bd7fbcf and b5a93df.
…; remove ephemeral finding IDs
Three fixes to crates/mds-cli/src/lint.rs:
1. Sanitize `file` key in error-only JSON entries (medium).
Error-only entries (`{"file":…,"error":…}`) in directory mode bypassed
`to_canonical_json`'s `sanitize_control_chars_wire` pass, emitting raw
control bytes in `files[].file` while diagnostic entries were sanitized.
Compute `file_key = sanitize_control_chars_wire(&display_path)` once per
file and use it in all four error-entry pushes, aligning error entries with
the spec.md normative sanitization contract (ADR-008).
2. Document `display_label` parameter in `plan_and_apply_fixes` (low).
The function's rustdoc listed `base_dir` but not the new `display_label`
parameter added by the refactor, leaving the published wire-value carrier
undocumented against the function's own convention. Added doc paragraph
describing what the parameter carries and its relationship to the wire key.
3. Replace ephemeral `finding N` identifiers with durable references (medium).
Comments at four sites referenced "finding 2", "finding 3", "finding 4"
from an ephemeral review artifact with no durable referent. Replaced with
AC-P1-10 where that acceptance criterion governs, and with inline invariant
statements elsewhere (leaving the end-state, not the transition).
The `relative_display` backslash fix (two high-severity findings) was already
committed in bd7fbcf (components()-based join) with a regression test in the
same commit; this commit addresses only the three remaining findings.
Three inline comments in run_lint_stdin carried transition notes of the form "(was bare 'stdin' before this PR)" alongside the AD-211-2 citation. Those notes encode the old state, not the end-state — a tombstone pattern the project rules explicitly forbid (project CLAUDE.md: "Leave the end-state, not the transition"). The code is self-documenting via STDIN_DISPLAY_LABEL, whose own rustdoc block covers AD-211-1/2/3. Also adds a missing blank `///` separator in `relative_display`'s doc comment: the "Error-only entries" paragraph and the "Forward slashes" paragraph were running together as one block despite covering distinct topics (caller asymmetry vs. byte-order rationale for forward slashes).
- Strip transition residue for source_label_is_stdin_sentinel: the method was introduced and replaced entirely within this branch (331a8d3 -> b81fe07) and never existed on main or wave/v0.4.0-wave1, so the CHANGELOG entry pointed consumers at a symbol no released version ever carried, and the api_surface rustdoc carried a tombstone naming a dead symbol (project rule: leave the end-state, not the transition). - Correct the to_canonical_json ordering rustdoc: file groups are keyed on the RAW diag.file while the emitted "file" string is the WIRE-sanitized form, so the previous claim that this ordering matches the CLI directory sort (which sorts on the SANITIZED relative display path) was imprecise. Record why the divergence is unreachable from the engine: every entry point passes exactly one filename, so an engine-produced LintResult always has one file group. - Narrow the CHANGELOG file-group-ordering bullet to the CLI directory-mode contract it actually describes, for the same reason.
Finding 4 (PF-013): stdin_lint_diagnostic_includes_code_frame asserted "<stdin>" present in human output but never asserted "input.mds" absent. Its JSON sibling asserts both. Add the missing negative assertion so the pair is symmetric (presence-plus-absence is a stronger guarantee than presence-only). Finding 5 (ledger): pr1-lint-json-plan.md recorded the wave-1 WASM baseline as 820,305 (locally measured at main). The CI-measured wave/v0.4.0-wave1 baseline is 821,662 bytes — consistent with ci.yml which records 821,662 as the baseline (821,662 + 11,840 = 833,502; headroom 16,498 / 1.94% against the 850,000 guard). Update the plan doc to record both figures with the toolchain context explaining the 1,357-byte difference (local Binaryen vs CI Binaryen v129). Findings 1, 2, 3: ALREADY-RESOLVED by commit 56424f7 which removed the redundant to_canonical_json re-sort. sort_diagnostics is now the single ordering choke point; both rustdoc blocks agree; CHANGELOG is accurate. Co-Authored-By: Claude <noreply@anthropic.com>
write_text(newline=None) translates \n to os.linesep (\r\n) on Windows,
so the CLI was linting CRLF bytes while the Python surface processed the
in-memory LF string — causing byte-offset divergence in the AC-P1-24
cross-surface parity tests (offsets 29→30 and 57→62 on Windows).
The defect is in the test harness only; both surfaces were correct for
the bytes they actually received.
Changes:
- test_parity.py L262, L305: write_text → write_bytes (file fixtures)
- test_parity.py L408: input=src → input=src.encode("utf-8"), drop
text=True, decode stdout/stderr explicitly (.decode("utf-8"))
- conftest.py L88: write_text → write_bytes in cli_build helper (latent
CRLF risk in test_par2_* tests whose fixtures carry no offsets)
- Add test_crlf_input_parity: deliberate CRLF contract test — feeds
genuine CRLF bytes to both surfaces and asserts they AGREE, converting
an accidental platform gap into a tested regression contract.
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three related issues that all mutate the same JSON wire object, batched into one PR per the wave plan:
--format jsonwire format #211: Uniform `` sentinel across all CLI surfaces — five inconsistent conventions collapsed to oneChanges
#211 — Stdin source-identity sentinel
Plan §5 step 1a — verified build/check `` emission sites (AD-211-5)
These were located empirically (reproducing the `[:1:1]` frame before the relabel landed) and traced to the CLI boundary:
The AD-211-5 rustdoc block in `output.rs` records these with the same verified line numbers.
#202 — Diagnostic ordering
#203 — Span anchoring at name
Alignment fixes (AD-### rustdoc at call sites, AC-P1-18, AC-P1-24 cross-surface, additional test coverage)
Breaking Changes
Reviewer Focus Areas
crates/mds-wasm/pkg/mds_wasm_bg.wasmandpkg-web/, wasm-pack 0.15.0 bundled wasm-opt, measured at HEAD after commit 56424f7 removed the redundantto_canonical_jsonre-sort). Delta: +11,840 bytes. Budget: 850,000. Headroom: 16,498 bytes (1.94%). Guard NOT raised. Toolchain note: local uses wasm-pack-bundled wasm-opt; CI uses Binaryen v129 (distinct optimizer). CI is the authoritative measurement. (Earlier draft recorded 845,143 bytes, measured before 56424f7 landed; the re-sort removal reduced the binary by 11,641 bytes.)Test Plan
Tests through the full alignment wave: