feat(debug-files): --strip-sources-content - #48
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let bytes = std::fs::read(map_path)?; | ||
| let Ok(serde_json::Value::Object(mut map)) = serde_json::from_slice(&bytes) else { | ||
| return Ok(None); | ||
| }; | ||
| if map.remove("sourcesContent").is_none() { | ||
| return Ok(None); | ||
| } | ||
| let stripped = serde_json::to_vec(&map)?; | ||
| let name = map_path | ||
| .file_name() | ||
| .map(Path::new) | ||
| .unwrap_or_else(|| Path::new("bundle.js.map")); | ||
| let out = tmpdir.join(name); | ||
| std::fs::write(&out, &stripped)?; |
There was a problem hiding this comment.
std::fs::read(map_path)? and std::fs::write(&out, &stripped)? propagate a bare std::io::Error through anyhow::Result here, not crate::error::Error::Io. Because this function's error type is anyhow::Result rather than crate::error::Result, ? picks anyhow's blanket From<io::Error> instead of the crate's #[from] impl, so the boxed error is a raw io::Error. main's classify does err.downcast_ref::<crate::error::Error>(), which will fail on that, falling through to ExitCode::Unexpected (1, "caller should fall back") — even though the exact same read failing in sourcemap::identify (src/symbols/sourcemap.rs:21, which returns crate::error::Result) correctly maps to ExitCode::InputNotFound (10, no fallback). So a map that goes unreadable/unwritable between pass 1 and this pass-2 stripping step gets a different, inconsistent exit code than the identical failure one call earlier — and per CLAUDE.md, integrators rely on that boundary to decide whether to fall back. Consider having this helper return crate::error::Result (like identify) instead of anyhow::Result, so the #[from] std::io::Error conversion applies.
Code reviewAdds Findings: 1 inline (1 blocking).
Otherwise the change holds together: identity/hash recomputation is scoped correctly to the stripped bytes, the per-file tempdir (created fresh inside |
Code reviewAdds I traced the full path: CLI validation → Findings: None. This is a well-scoped, well-tested addition — the implementation matches its documentation claims. Merge as is. |
Review of the full 0.7.11 delta — SEV1 found here, fixed in 3709d36
Verified with the release binary against a capturing mock: the PUT body contained
Also fixed here from the same review:
What the review checked and found sound: strip + 🤖 Generated with Claude Code |
… their source uploaded Audit §8: a source map's `sourcesContent` embeds the original source verbatim, and it rides into the upload with the map. That is how a symbolicated crash shows source lines — and also means the customer's code leaves the build machine. There was no way to say no. `--strip-sources-content` (`--type sourcemaps`) removes it from the COPY that is uploaded. File, line and column symbolication is unaffected; what is lost is the snippet shown beside a frame. - The map on disk is never modified. It is the caller's build output and their own debugging, and the bundler plugins delete these maps after a successful upload anyway. - The declared `hash` describes the stripped bytes, not the file that was read — the wire field is supposed to describe what was uploaded. - The debug-id is carried over untouched: it is the key, and re-deriving it would break the pair with the bundle `sourcemaps inject` stamped. - A map with no `sourcesContent`, or one that is not the JSON object we expect, is uploaded byte-for-byte unchanged. This is a privacy preference, not a validator. - Rejected with exit 20 for every other `--type`, like the other sourcemap-only flags: no other symbol format embeds source. Measured on a real esbuild bundle: 253 → 181 bytes uploaded, local map untouched. Five mutants caught (flag ignored, always-on, nothing-to-strip still re-serialized, wrong hash, flag accepted for other types) — the third needed the test strengthened to assert BYTE equality, not just the absence of the key. Two new e2e flows unzip the captured PUT and check both the upload and the file on disk. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…rce entirely
A review of the whole 0.7.11 delta found the flag's promise failing silently on the one map shape it
could not see. An INDEXED map (spec §Index-Map, `{"version":3,"sections":[{"offset":…,"map":{…}}]}`)
keeps its source inside each section, not at the top level — so `map.remove("sourcesContent")` found
nothing, the map uploaded verbatim WITH the source, and the run reported success without even a log
line. A team turning this on precisely so their code never leaves the build machine got the opposite,
quietly.
`sourcesContent` is now removed wherever a map can carry it: the top level and every
`sections[].map`, recursively (sections nest). Verified against a capturing mock — the PUT body no
longer contains the source, while sections, offsets, mappings and the debug-id survive.
Also from the same review: the strip's JSON round-trip moved to `spawn_blocking`. Parsing and
re-serializing a multi-megabyte map costs more than the zstd that already moved there, and every
upload future is polled by one task.
Docs: README's synopsis was missing `--strip-sources-content`, its "both flags" line had grown to
three, and neither document mentioned indexed maps. Three mutants caught (sections branch skipped,
recursion dropped, top-level removal dropped).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
1500e60 to
a295e0f
Compare
3709d36 to
9dc7c14
Compare
Code reviewAdds Traced the flow end to end: the CPU-bound strip runs on Findings: None. Looks solid — merge as is. |
1 participant
Stacked on #47. Audit §8's last CLI-side item, batched into the same release as #45/#46/#47.
Why
A source map's
sourcesContentembeds the original source verbatim. It is what lets a symbolicated crash show source lines — and it also means the customer's code leaves the build machine on every upload. There was no way to say no.What
--strip-sources-content(--type sourcemaps) removes it from the copy that is uploaded. File, line and column symbolication is unaffected; what is lost is the snippet shown beside a frame.Measured on a real esbuild bundle:
Decisions worth naming:
hashdescribes the stripped bytes, not the file that was read — the wire field is supposed to describe what was uploaded.sourcemaps injectstamped.sourcesContent(or one that is not the JSON object we expect) is uploaded byte-for-byte unchanged. This is a privacy preference, not a validator.--type, like the other sourcemap-only flags — no other symbol format embeds source.Tests
Four unit tests (strip removes it and keeps
mappings/sources/debug_idwhile leaving the file on disk alone; the declared hash matches the uploaded bytes; the default keeps the source; nothing-to-strip is byte-identical) plus the flag-rejection matrix and two e2e flows that unzip the captured PUT.Five mutants caught: flag ignored, always-on, nothing-to-strip still re-serialized, wrong hash, flag accepted for other types. The third initially survived — the test asserted the key was absent, which was true either way; it now asserts byte equality.
Gates:
cargo fmt --check,clippy --all-targets -D warnings(0), 500 unit tests + all suites,e2e_flows.pyALL PASS.🤖 Generated with Claude Code