feat(il2cpp): validate LineNumberMappings.json before packing it

[krassx]

Stacked on #75 (→ #74 → #73 → #72 → #70). Retarget as those merge.

debug-files upload --type il2cpp-linemap packed LineNumberMappings.json as-is, so a truncated, empty, binary or wrong file uploaded "successfully" and every IL2CPP crash then failed to symbolicate with nothing pointing back at the upload. (This was the gap pinned as a characterisation test in #75, and left for a decision.)

What it checks

il2cpp_linemap::validate_mappings streams the document with struson (these files run to tens of MB; nothing is held in memory) against the shape the symbolicator reads, {cpp_path: {cs_path: {cpp_line: cs_line}}}:

• objects at all three levels;
• C++ line keys are non-negative integers fitting u32;
• C# line values are non-negative integers fitting u32 (no strings, null, floats, exponents, negatives).

Corrupt content → InputInvalid (exit 11) naming LineNumberMappings.json, the file path and the JSON path of the problem — in --dry-run too, before anything is packed or sent. A genuine I/O failure stays an I/O error (10). An empty map {} is accepted with a warning (a project can have none).

Behaviour change (changelog ### Changed)

Corrupt mappings used to exit 0. They now exit 11. Integrators that treated success as proof of a usable map can now rely on it; any that fed this command known-bad files will see the new failure. --help and README updated.

Tests

• Fixtures validate with exact counts (2 / 3 / 7).
• 23 malformed or misshapen documents are rejected, each with a message naming the file: empty, whitespace, truncated, trailing junk, two documents, BOM, binary, wrong top-level type, one level short, string/null/float/exponent/negative/over-u32 values, non-numeric / empty / negative / over-u32 line keys, 50 000-deep nesting.
• Every strict prefix of a real map is rejected (none accepted, none crashes).
• A 300 000-entry map validates.
• CLI level: corrupt maps exit 11 for dry and real runs with nothing sent (expect(0) on POST and PUT); the shipped android/ios fixtures still pass.
• fmt / clippy -D warnings / full suite (647 unit + integration) / e2e / cargo +1.88 check green.

🤖 Generated with Claude Code

[cursor]

Stale comment

Deep review (PR tip 4234404)

No remaining P0–P3. The change does what it claims: a corrupt LineNumberMappings.json can no longer upload as success.

What I checked

• Call site: run_il2cpp_linemap_upload runs validate_mappings before tempdir / pack_entries / the presigned POST, including --dry-run. I/O open failures stay Error::Io (exit 10); shape/syntax failures are InputInvalid (11). Integrators in the 10–39 band will not fall back and re-upload the same bytes — that is the right contract for this gate.
• Shape vs the symbolicator: Sentry symbolic-il2cpp LineMapping::parse is the documented reader. It requires a top-level object, skips exact key __debug-id__, parses C++ line keys with str::parse::<u32>(), and takes C# line values via as_u64().try_into(). This validator is the same shape, slightly stricter on non-object siblings (fail the file instead of silently dropping the entry). Unity/Embrace samples are "2576": 24 — string keys, JSON-number values — which matches the fixtures and the tests.
• __debug-id__: The first commit would have treated a non-object sentinel as a misshapen cpp entry. Sentry writes "__debug-id__": {"<uuid>": {}} (that would have passed as an empty nested object), but a string/null sentinel would not. The follow-up skip-whatever-it-holds (still syntax-checked via skip_value) matches both the epic doc and Sentry’s continue. Only that exact top-level key is special.
• Streaming: Names are held one cpp/cs path at a time; the file is not loaded into a Value. Default struson nesting cap is 128; this document is three levels. Invalid UTF-8 is classified as content (11), not I/O — pinned by the binary case.
• Public surface: --type il2cpp-linemap clap text, README, and changelog all describe the new exit 11. Empty {} still exits 0 with a warning, by design.

Tests I ran

• cargo test --bin bugsee-cli symbols::il2cpp_linemap::tests — 12 passed on 4234404 (fixtures, 23 malformed cases, every prefix of the iOS fixture, 300k-entry stream, __debug-id__ skip + syntax-fail, missing file → Io).
• cargo test --test broken_files il2cpp — 2 passed: corrupt maps exit 11 for dry and real runs with expect(0) on POST/PUT; android/ios fixtures still dry-run 0.

Residual (not findings)

• Success is proof of a well-formed map, not a non-empty one. {} / { "a.cpp": { "A.cs": {} } } still warn and exit 0; that is documented.
• The 50k-nesting case is rejected at depth 3 ("a" is not a C++ line key) rather than by walking the whole tree. Harmless: a real map is only three objects deep, and skip_value on the sentinel still hits struson’s 128 cap.

---

1. Overall risk: Low
2. Merge recommendation: Approve once #75 (the stack base) is in. Behaviour change (corrupt maps 0 → 11) is changelogged.
3. Most important issues to fix: None remaining.
4. Positives: Gate sits at the right point in the upload path; streaming is the right tool for tens-of-MB maps; error messages name the file and the JSON path; catching __debug-id__ in follow-up commits (and testing string/null/object/truncated sentinels) was the right call.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep Code Review

PR intent: debug-files upload --type il2cpp-linemap used to pack LineNumberMappings.json as-is, so a truncated, empty, binary, or wrong-shape file uploaded with exit 0 and every later IL2CPP crash failed to symbolicate with nothing pointing at the upload. This gates the document as a stream against the 3-level {cpp_path: {cs_path: {cpp_line: cs_line}}} shape (non-negative u32 line numbers), fails InputInvalid (exit 11) before pack/POST (dry-run included), skips the documented top-level __debug-id__ sentinel, and still accepts an empty {} with a warning.

Reviewed with surrounding code, not the diff alone: validate_mappings + tests in src/symbols/il2cpp_linemap.rs; call site in run_il2cpp_linemap_upload (validate before pack_entries / presigned POST); exit-code mapping (InputInvalid → 11, no fallback); --help / README / CHANGELOG; fixtures; inject::mapjson struson settings (where restrict_number_values: false is actually justified); Sentry symbolic-il2cpp LineMapping shape (__debug-id__ as a fake cpp entry {uuid: {}}, cpp-line keys u32, cs-line values u32).

Adversarial checks that did not become findings: empty vs missing file (11 vs 10); every strict prefix of the iOS fixture; BOM / two documents / trailing junk; string/null/float/exponent/u32-overflow line values; __debug-id__ as string/number/null/object (skipped) vs __other__ (rejected); Sentry’s {uuid: {}} sentinel (valid 3-level empty object, also fine without the skip); TOCTOU between validate and pack (same class as every other CLI read of a finished Unity artefact); empty {} accepted with a warning (documented exception to “success means usable”).

Tests I ran (green): cargo test --bin bugsee-cli il2cpp (14); cargo test --test broken_files il2cpp (2); cargo test --test debug_files_flags il2cpp (3); cargo test --test metadata_format il2cpp (1). Also inspected debug-files upload --help — the type description matches the new gate. Did not run the full crate, clippy -D warnings, e2e, or cargo +1.88 check.

Findings

One non-blocking P3 (inline). No P0–P2.

Verdict

1. Overall risk: Low
2. Merge recommendation: Approve — the behaviour change is the point, changelogged, and the gate sits in the right place. Stacked on #75; retarget as that merges.
3. Most important to fix: Optional: leave struson’s number-length/exponent cap on (this parser needs the opposite of mapjson). Not a merge blocker.
4. Positives: Call site fails before pack/HTTP in both dry and real runs (expect(0) on POST/PUT). Corrupt vs I/O split is correct (empty/truncated/binary → 11; missing file → 10). Sentinel skip landed in a follow-up with tests for “whatever value, still syntax-checked, only that exact key”. Streaming walk is iterative (no recursion on the 3-level shape); 300k-entry test exists. --help, README, and CHANGELOG ### Changed stay in sync.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Re-review (5c0e340)

This tip is a merge of main (the #75 stack) into fix/il2cpp-validate-mappings. No change to validate_mappings, the upload call site, tests, --help, README, or changelog vs 4234404.

Prior P3 (restrict_number_values: false copied from mapjson) is not addressed. No new findings from the merge. IL2CPP unit tests still green (14).

1. Overall risk: Low
2. Merge recommendation: Approve. The stack base is now in main; this PR can retarget main.
3. Most important to fix: Optional P3 only — leave struson's number cap on. Not a merge blocker.
4. Positives: Same as before — gate still sits before pack/POST.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Re-review (a9c734c)

Prior P3 fully addressed. JsonStreamReader::new keeps struson's default number guard (no exponent beyond ±99, no token over 100 characters). C# line values that still fail parse::<u32>() are shown truncated to 32 characters; reader messages that echo a token are capped at 240 characters before they hit stderr. Pinned by a_huge_number_is_rejected_cheaply_with_a_short_message (50 000-digit leaf and 1e + 40 nines; error < 1000 bytes).

IL2CPP unit tests: 15 passed on this tip, including the new case and the previous malformed/prefix/sentinel/300k suite.

No remaining P0–P3.

1. Overall risk: Low
2. Merge recommendation: Approve
3. Most important issues to fix: None remaining
4. Positives: The follow-up also capped struson's own UnsupportedNumberValue text (the 50k-digit test failed until that), which is the part of the P3 that would actually have shown up in a CI log.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Re-review (a9c734c)

Prior P3 fully_addressed. bab32ed drops the custom ReaderSettings (JsonStreamReader::new — default number-length/exponent cap stays on) and truncates a failed C# line token to 32 chars. a9c734c caps the reader’s own message at 240 chars before it is interpolated into the InputInvalid text, so a UnsupportedNumberValue cannot dump a huge token on stderr. New test a_huge_number_is_rejected_cheaply_with_a_short_message covers a 50 000-digit leaf and 1e + 40 nines; message stays under 1 000 bytes.

1e3 is still rejected (exponent 3 is inside the reader cap, then parse::<u32>() fails). Sentinel skip, empty-map warning, and the exit-11-before-pack path are unchanged.

Ran: cargo test --bin bugsee-cli symbols::il2cpp_linemap::tests — 13 passed on a9c734c.

No remaining P0–P3.

1. Overall risk: Low
2. Merge recommendation: Approve. Stack base is on main; this can retarget main.
3. Most important issues to fix: None remaining.
4. Positives: The optional P3 was the right tightening and is now pinned by a test that would fail if the error echoed the number.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Re-review (a9c734c)

PR intent: debug-files upload --type il2cpp-linemap used to pack LineNumberMappings.json as-is, so a truncated, empty, binary, or wrong-shape file uploaded with exit 0 and every later IL2CPP crash failed to symbolicate with nothing pointing at the upload. This gates the document as a stream against {cpp_path: {cs_path: {cpp_line: cs_line}}} (non-negative u32 lines), fails InputInvalid (exit 11) before pack/POST (dry-run included), skips the documented top-level __debug-id__ sentinel, and still accepts {} with a warning.

What changed since 5c0e340: bab32ed + a9c734c address the open P3. JsonStreamReader::new keeps struson's default number guard (no exponent beyond ±99, no token over 100 chars). A C# line that fails parse::<u32>() shows at most 32 chars; any reader error detail is capped at 240 chars before it reaches stderr. Pinned by a_huge_number_is_rejected_cheaply_with_a_short_message (50k-digit leaf and a huge exponent).

Reviewed with surrounding code, not the diff alone: validate_mappings + tests; run_il2cpp_linemap_upload (validate before tempdir / pack_entries / presigned POST); InputInvalid → 11 (no integrator fallback); clap --help / README / CHANGELOG ### Changed; Embrace/Sentry shape ("2576": 24, __debug-id__ as {uuid: {}} or ignored); inject::mapjson still disables the number guard for a reason that does not apply here.

Adversarial checks that did not become findings: empty vs missing (11 vs 10); every strict prefix of the iOS fixture; BOM / two documents / trailing junk; string/null/float/exponent/u32-overflow values; __debug-id__ as string/number/null/object (skipped, still syntax-checked) vs __other__ (rejected); Sentry's {uuid: {}} sentinel (valid 3-level empty object even without the skip); TOCTOU between validate and pack (same class as every other finished-artefact read); empty {} / { "a.cpp": { "A.cs": {} } } warn and exit 0 (documented exception to “success means usable”). struson reads a number token before applying the 100-char guard, so a pathological leaf is still buffered internally — stderr no longer echoes it, and a real Unity map is many small integers, not one giant token.

Tests I ran (green): cargo test --bin bugsee-cli il2cpp (15); cargo test --test broken_files il2cpp (2); cargo test --test debug_files_flags il2cpp (3). Inspected debug-files upload --help — type text matches the gate. CI on this tip is green (test/e2e all platforms, clippy, rustfmt).

Findings

Prior P3 fully_addressed. No remaining P0–P3.

Verdict

1. Overall risk: Low
2. Merge recommendation: Approve. Behaviour change (corrupt maps 0 → 11) is changelogged; the gate sits before pack/HTTP; the stack base is on main.
3. Most important to fix: None remaining.
4. Positives: Call site fails before pack/HTTP in dry and real runs (expect(0) on POST/PUT). Corrupt vs I/O split is correct. Sentinel skip landed with tests for “whatever value, still syntax-checked, only that exact key”. Streaming walk is iterative; 300k-entry test exists. --help, README, and CHANGELOG stay in sync. The follow-up correctly treated the number guard as the opposite of mapjson, then capped struson's own error text when the 50k-digit test proved it echoes the token.

  

_{Sent by Cursor Automation: Bugsee code review}