Single-copy the JSON escaper and the shell quote - #345
Conversation
The compact json.dumps formatter and the metadata store's indent=2 pretty formatter each carried their own ensure_ascii + surrogate-pair loop, in the crate whose own docstring names a second copy as the drift. Extract one crate-internal write_ensure_ascii; both formatters delegate. The pretty-vs- compact layout split stays.
render's private `shell_quoted` was a third hand-written spelling of CPython's `shlex.quote`, next to `shell::quote` in core — which `dl` already re-exports for `aid`. Delete the copy; the `sudo rm -rf` line `--purge` and `--prune` print goes through `shell::join`.
Reviewer's GuideDeduplicates JSON string escaping and shell quoting logic by introducing a single internal ensure_ascii helper used by both JSON formatters, and reusing the core shell::join/quote path from the renderer instead of a local implementation, with tests pinning behavior against CPython output. Sequence diagram for shared shell quoting in refusal advicesequenceDiagram
participant Renderer as dl renderer
participant Shell as devlaunch_core::shell
participant Output as Advice line
Renderer->>Renderer: report_refusals(remove_by_hand)
Renderer->>Shell: join(paths)
Shell-->>Renderer: shell-quoted command arguments
Renderer->>Output: format sudo rm -rf command
Flow diagram for unified formatting behaviorflowchart LR
A[JSON string fragment] --> B{Formatter}
B --> C[PythonFormatter compact JSON]
B --> D[PythonJsonFormatter indented metadata]
C --> E[write_ensure_ascii]
D --> E
E --> F[ASCII passthrough or lowercase UTF-16 escapes]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Fixed point: git diff a1c6fe8...a34e648 (3 .rs files, +155/-47). Preflight green: 12 checks pass, only publish-job skipped. The two axes were run in parallel in fresh contexts; findings are not merged or reranked across them.
Standards
1. json.rs:139 — "the only copy of it" — blocking (documentation).
The new helper's docstring says "The escaping half of the spelling, and the only copy of it". A third copy is live at rust/dl/src/render.rs:281-294 (PythonPretty::write_string_fragment), which the PR body itself names as deliberately deferred. The problem is not the deferral — it is that the same module's own docstring (json.rs:3-10) enumerates dl --ls --json among the documents its "One copy of it, at the bottom of the crate" covers, and that document is spelled by dl's formatter, not by this module. That docstring then cites, as its cautionary precedent, the timing document that "lost the spacing while its own docstring promised byte-comparability" — i.e. a docstring promising more than the code delivers is the exact defect this module names.
This PR already gets the wording right one file over: metadata.rs:1013 says "the crate's one copy of it". Scope json.rs:139 the same way (and consider the same trim at json.rs:7). One sentence.
2. metadata.rs:1011 — "Only the layout differs from the compact PythonFormatter" — non-blocking (documentation, newly added by this PR).
The float spelling differs too. PythonFormatter overrides write_f64 with python_repr (json.rs:118); PythonJsonFormatter has no write_f64 override (verified: zero occurrences in its impl block), so floats fall through to ryu — which disagrees with CPython on when to go exponential and how to pad the exponent, the very thing json.rs:104-118 documents at length. Unreachable today, since Document is i64 + strings (metadata.rs:927-931), so no bug — but the claim is broader than the code, which is finding 1's failure mode in miniature. "Only the escaping and the layout are shared" would be true.
3. The deferral reason holds — verified, no change requested.
write_ensure_ascii is pub(crate), honouring lib.rs's "the Python-spelling writers stay pub(crate)"; devlaunch-core/public-api.txt exposes no escaper, no formatter and no rendered-document fn, so collapsing copy three genuinely does need a new pub line. One correction for whoever picks that up: the residue is not one loop but two near-identical ~100-line delegating indent=2 formatters (render.rs:196-295 vs metadata.rs:1015+). Scope the follow-up as one pub formatter/document fn, not one pub escaper.
4. Not a finding. The Vec<String> collect at render.rs:1342-1348 is forced by pub fn shell::join(impl IntoIterator<Item = &'a str>) and mirrors the existing call at aid/src/main.rs:138. Widening to Item: AsRef<str> would move the very snapshot this PR is avoiding.
5. Layering — clean. json.rs:12-14 documents the module as below the four layers precisely so shared helpers can live there; domain/metadata.rs reaching crate::json is downward, not a reach-up. No Fowler smell introduced (no Feature Envy, Speculative Generality or Refused Bequest); test names are in house style.
Standards verdict: Request changes — both items are documentation-only, together about three lines.
Spec
Spec is #315: "domain/metadata.rs:1016-1036 reimplements json.rs:129-147's ensure_ascii + surrogate-pair escaping … the pretty-vs-compact formatter split is legitimate, the escaping half is not. And dl/src/render.rs:1322-1330 (shell_quoted) duplicates shell::quote … Share the escaping; delete render's copy."
Every clause is satisfied:
- "Share the escaping."
git grep encode_utf16on the branch returns exactly one escaping loop in core (json.rs:165, insidewrite_ensure_ascii);metadata.rs:1025delegates to it. The only other hit in core is a UTF-16 corruption fixture, not an escaper. - "delete render's copy."
git grep shell_quoted: zero hits.report_refusalsroutes throughshell::join. - "the pretty-vs-compact formatter split is legitimate."
PythonJsonFormatterstill delegates every layout method toPrettyFormatter; the compact spacing is unchanged. Onlywrite_string_fragmentmoved. - "Nothing here moves a
pubpath" (PR body).git diff --statagainst the fixed point: three.rsfiles, no non-.rsfile at all;public-api.txtuntouched, consistent withpub(crate).cargo test --workspaceexits 0 on the branch (20/20 suites) and on the baseline. - Ordering vs map #299 — "the builds that churn many
pubpaths … are blocked on the snapshot re-scope build #338." This PR churns zeropubpaths, so it was never in that blocked set; deferring the render escaper is what honours the note. - The deferred third copy is faithful to the ticket, which names only
shell_quotedinrender.rs, and names only the two core sites for the escaping.
Independent verification of "behaviour-preserving"
The branch claims both de-duplications are byte-for-byte no-ops. That was not taken on the tests' word:
- Escaper, full input space. The two deleted loops and the new shared helper were reimplemented side by side and run over all 1,112,064 Unicode scalar values, plus 17 adversarial strings (empty, DEL, C0 controls,
"/\, the U+FFFE and U+FFFF non-characters, U+FEFF, U+10FFFF, the scalars either side of the surrogate block, 300- and 5000-character mixed runs, back-to-back astral pairs). 0 mismatches. Lone surrogates are unrepresentable in&str, so they cannot arise. Spot-checked against CPython 3.14json.dumps— agrees. So there is no case where the shared version imposes something new on the other caller, and no byte ofmetadata.jsoncan change. - Shell quoting, adversarial paths. The deleted
shell_quotedandcore::shell::quoteare byte-identical over 19 paths (space, embedded',", backslash, a$HOMEplus a backtickedid,;rm -rf /, newline, tab, DEL, non-ASCII, emoji, glob metacharacters, empty,a@b%c,-rf) — including the empty-path edge, where the old guard was spelled differently (!text.is_empty() && …versus an early return) and still lands on''. The joined advice line is byte-identical to CPythonshlex.joinfor that corpus, and round-trips through bothbashanddashback to the exact input paths. The pasteable line is genuinely pasteable. - The new pins are load-bearing. Mutation-tested in a scratch worktree: flipping the shared escaper to uppercase hex fails 3 tests, including the new
the_indent_two_document_escapes_an_astral_character_as_the_pair; makingshell::quotedouble-quote (theshlex-crate spelling) fails the newthe_advice_line_quotes_a_path_a_shell_would_mis_splitand nothing else indl— so that test is the only guard on that boundary, and it earns its place.
One pre-existing divergence surfaced by the probing — non-blocking, not introduced here
serde escapes only < 0x20, " and \, so exactly one non-printable ASCII character ever reaches write_string_fragment: U+007F (DEL) — confirmed with a spy formatter over U+0000..U+2FFFF. CPython escapes it: json.dumps of a lone DEL yields the six characters \u007f. This escaper — both deleted copies and the new shared one alike — passes the raw 0x7F byte straight through, because the gate is is_ascii() rather than CPython's ' ' <= c <= '~'.
Both deleted loops did the same, so this is not a regression and does not block. It earns a line only because metadata.rs:1-7 makes byte-parity an explicit contract ("the one piece of state the Python and Rust builds share … the escaping [is] pinned against bytes the real Python wrote"), and the loader parses into serde_json::Value and re-encodes — so a metadata.json the Python build wrote carrying a \u007f escape would load and re-save as a raw DEL byte, breaking that round-trip. Unreachable in practice: git rejects control characters in ref names. If you want it closed, it is now one || character == '\u{7f}' in the shared helper plus a row in the new table — which is exactly the payoff this PR was built for.
Spec verdict: Approve.
Verdict
Request changes — on Standards only, and only on documentation. The engineering is sound, and on the two questions that actually matter for a de-duplicated encoder it is proven rather than asserted: the three escaper spellings agree across the entire Unicode scalar space, and the shell-quoted advice line is CPython-exact and survives a real shell round-trip.
Blocking:
rust/devlaunch-core/src/json.rs:139— "the only copy of it" overclaims whilerender.rs:281-294still holds a third copy of the same escaping. Scope it to the crate, asmetadata.rs:1013already does.
Non-blocking, worth folding into the same push:
rust/devlaunch-core/src/domain/metadata.rs:1011— "Only the layout differs" is false;write_f64differs too (ryu versuspython_repr). Unreachable today.- Optional: a DEL row in
ensure_ascii_writes_what_json_dumps_writes_between_the_quotes, either pinning today's passthrough as deliberate or closing it.
Neither blocking item touches behaviour, so nothing here needs the equivalence work above re-run.
The review of #345 caught the new helper's docstring calling itself "the only copy" while a third copy of the same escaping is live in dl's own formatter for `--ls --json` — the exact overclaim this module's docstring holds up as its cautionary case, and the class of defect map #299 exists to close. Both the module doc and the helper's now say where the third copy is and which ticket collapses it (#346). Also corrects metadata.rs's "only the layout differs": the compact formatter additionally overrides write_f64 for Python's float spelling, which no Document can reach today because its numbers are all i64. Docs only, no behaviour. Refs #315.
Follow-up on my review above: both findings resolved at Re-read only the two files I raised findings against, at the new head. Finding 1 (was blocking) — resolved, and more thoroughly than I asked. I only asked that Finding 2 (was non-blocking) — resolved, and the new parenthetical's factual claim holds. I checked the "numbers are all Docs only, confirmed mechanically: every added/removed line in On graduating the DEL divergence to #349 rather than folding it in — right call, and I would have argued for it. This PR's whole warrant is "byte-for-byte no-op", and that is exactly what made it cheap to verify exhaustively: a full-input-space differential sweep is only a valid argument when nothing is supposed to change. Folding in a DEL fix would have made the same diff both a no-op and a behaviour change, forcing the equivalence evidence to be re-derived with a carve-out. #349 also improves on how I wrote it up, by adding the reverse-direction check — whether any character in One note for #346 when it is picked up, not a request here and not about this PR: #346 frames the work as collapsing the copy onto the shared escaper. Per finding 3 of my review, the residue is really two near-identical ~100-line delegating Standards: Approve. Spec: Approve. Overall: Approve — nothing outstanding from this review. |
The extraction's own tests cover the fragment seam — ASCII, BMP, astral, mixed, empty — which is every class `write_ensure_ascii` can be handed. The classes that never reach it are the ones serde's escape table spells: the quotes, the backslash and everything under 0x20. Nothing in either crate asserted that table agrees with Python's, so a de-duplication that touches the escaping had no net under the half a person is most likely to reach for next. One document-level pin per formatter, compact and indent=2, plus `/` (bare in Python) and U+2028/U+2029 (not singled out). The advice line gets the same treatment: `$`, a backtick, a newline and a path that stringifies to nothing, since single quoting is what makes the first two inert in a line pasted under sudo and an empty word has to survive as a word. All expectations are CPython's `json.dumps`/`shlex.join` output. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Closes #315
Two behaviour-preserving de-duplications, each landed test-first against literals taken from the frozen CPython build rather than recomputed from the code.
The
ensure_asciiescaper.devlaunch-core's compactjson.dumpsformatter and the metadata store'sindent=2pretty formatter each carried their own hand-written escaping loop — in the crate whose own docstring says a second copy is how one of these documents drifts. There is now one crate-internalwrite_ensure_ascii, and both formatters delegate to it. The pretty-vs-compact layout split is legitimate and stays; only the escaping was shared.The red was the helper's own tests, which cover the cases the two loops each had to get identically right: ASCII passthrough, a BMP escape, an astral character written as its UTF-16 surrogate pair, and a mixed run where the ASCII on either side has to survive intact. Two document-level pins went in alongside as the net for the extraction — the compact spelling of a non-ASCII value, and the
indent=2bytes for an emoji, which the existing golden did not reach (it carries an umlaut only).The shell quote.
render's privateshell_quotedwas a third hand-written spelling ofshlex.quote, besideshell::quotein core — whichdlalready re-exports foraid. Deleted; thesudo rm -rfadvice line that--purgeand--pruneprint goes throughshell::join. Pinned first at the renderer's own boundary, asserting the exact pasteable line for a path with a space and a path with an embedded single quote — the case where theshlexcrate and CPython disagree on the bytes, and so the case a hand-rolled copy drifts on.Nothing here moves a
pubpath, so the public-API snapshots are untouched.Left out of scope, deliberately: there is a third copy of the same escaping, in the
dlrenderer's own pretty formatter for--ls --json. Collapsing it needs a newpubitem on core, which moves a snapshot file another ticket on this map owns.🤖 Generated with Claude Code
Summary by Sourcery
Deduplicate JSON escaping and shell quoting while preserving the existing output formats and behavior.
Enhancements:
ensure_asciiescaping so compact and pretty formatters share the same behavior.Tests: