Collapse dl's duplicate indent=2 JSON formatter onto core's - #522
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideMoves indented Python-compatible JSON formatting into core as a shared implementation, updates Sequence diagram for JSON document serializationsequenceDiagram
participant DL as dl
participant Core as devlaunch_core::json
participant Serializer as serde_json::Serializer
participant Formatter as PythonPrettyFormatter
participant WF as wf
DL->>Core: as_python_writes_it_indented(value)
Core->>Serializer: with_formatter(PythonPrettyFormatter)
Serializer->>Formatter: serialize layout and string fragments
Formatter-->>Serializer: PrettyFormatter layout + ensure_ascii escaping
Serializer-->>Core: indented JSON string
Core-->>DL: JSON document
DL-->>WF: --ls --json output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
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:
|
fc4e57e to
b6932ce
Compare
`dl --ls --json` and `metadata.json` are both `json.dumps(..., indent=2)` documents pinned against what the Python build wrote, and each had a hundred-line formatter of its own: the same nine layout methods forwarded to serde's `PrettyFormatter`, and the same `ensure_ascii` loop. Nothing held the two equal, and the last time that mattered the escaping gate was wrong about DEL in both and had to be closed twice. The indented spelling now lives in core's `json` module beside the compact one, exported as `as_python_writes_it_indented`, and the renderer calls it. The metadata store keeps going through the formatter directly, because its document is a struct and a `Value` would put its field order at the mercy of the map. Byte-identity is the bar, since `wf` parses `--ls --json`. Every pin on that document is the same assertion against the same call it was written against, with the formatter behind it deleted rather than edited.
b6932ce to
0ed90eb
Compare
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review in fresh context; none of this branch was written by me. Reviewed against merge base ae2c6ba652a56b66b2f7155bcddbb5a7c46c24e4, three-dot ae2c6ba...0ed90eb. The base branch wayfinder/devlaunch-349 is gone, so that is the merge base with main.
What I checked instead of trusting
The central claim is byte-identity, and the evidence for it — the differential sweep — is scratch and not in the diff. So I rebuilt it rather than reading about it.
1. Old formatter vs new, reconstructed. PythonPretty extracted verbatim from ae2c6ba:rust/dl/src/render.rs, stood beside as_python_writes_it_indented in a scratch crate pinned to serde_json =1.0.151 (what rust/Cargo.lock resolves), and run over every Unicode scalar value twice — once nested as a value in {"k": [s, {"inner": s}]}, once as an object key in {s: 1} — plus 23 adversarial shapes: empty array and object, bare null/0/-1/true/"", -0.0, 4e-5, 1e16, four-deep nesting, an empty key, all of ASCII, astral as key and as value, U+D7FF/U+E000/U+FFFE/U+FFFF/U+FEFF/U+10FFFF, serde's own escape set, a 6000-character mixed run.
2,224,151 documents, 0 mismatches. The claim holds.
2. The check the PR does not make: the new path against CPython itself. The whole BMP plus every 997th astral scalar, as key and as value, plus the adversarial set, against this machine's json.dumps(..., indent=2):
129,099 documents, 0 mismatches.
3. end_object_key, the one place the method sets diverged. Verified in the pinned crate rather than from the PR body: impl<'a> Formatter for PrettyFormatter<'a> in serde_json-1.0.151/src/ser.rs overrides exactly nine methods and end_object_key is not among them; the trait default at ser.rs:1899 is Ok(()). dl's tenth delegation forwarded the trait's own no-op through a type that does not override it. Every one of the 1.1M documents above carries at least one object key, so it is exercised, not merely argued.
4. The pins really did not move. The only non-comment added lines in render.rs are the forwarder body:
+pub(crate) fn python_json_document(value: &Value) -> String {
+ devlaunch_core::json::as_python_writes_it_indented(value)
Every other + in that file is a doc comment. No assertion, no literal, no test body changed.
5. The deletion is complete. PrettyFormatter now appears only in json.rs and at metadata.rs's call sites; the ' '..='~' gate exists once, at json.rs:335; no other encode_utf16 escape loop is left in rust/ (the metadata.rs:1845 hit is a not-UTF-8 corruption fixture). No page under docs/ or in the README described the duplication, so nothing went stale.
6. Snapshots. Exactly one new pub line in rust/devlaunch-core/src/. public-api.api.txt and devlaunch-runner/public-api.txt byte-unchanged. Tier is not a judgement call: scripts/public-api-snapshots.sh classifies on devlaunch_core::api\b and this row is under json::. And the hand-placement worry answers itself — CI's public-api job runs the same script into a scratch tree and diff -us the result, so the green tick is the proof the row is byte-exact. cargo test -p devlaunch-core --test public_api_snapshots 4/4 here.
7. Gates re-run here. devlaunch-core --lib json:: 10/10, dl --lib render:: 55/55, metadata's four the_indent_two_* 4/4, cargo fmt --check clean, cargo clippy --locked --all-targets -- -D warnings clean.
Standards
Nothing blocking. Tree compiles clean, all 14 json/metadata pins pass, no orphaned imports, snapshot tier correct, layering clean (json.rs still imports only std::io and serde::Serialize; domain reaching down is the documented direction).
Major — the float divergence is held by prose, not by a test. json.rs:186-190 admits PythonPrettyFormatter does not spell floats Python's way and defends it with an untested claim. That is the exact failure mode this PR exists to retire. Overriding write_f64 to forward to python_repr, as PythonFormatter already does at json.rs:124, is three lines, moves no byte today, and deletes the caveat.
The caveat is also wrong on the facts, which I checked separately: disk is not an integer, it is an object — {"exclusiveBytes": u64} or {"atLeastBytes": u64, "unreadable": usize}, or null (flows/listing.rs:1092-1101, flows/disk_usage.rs:438-449) — and the sentence never accounts for unsaved, an arbitrary serde_json::Value at listing.rs:1056. The conclusion survives: unsaved is bools and strings only, and there is no f64 anywhere on either wire, so no float can reach the formatter today. But the reason given for it is not the reason.
Minor — two of the three new core tests are verbatim copies of dl's. json.rs:552-569 asserts the identical literal and expectation as render.rs:3033-3044; json.rs:589-594 duplicates render.rs:3067-3071. Under the standing "a second hand-maintained copy of a fact needs a test beside it that diffs it against the first" rule, these are copies with no diffing guard. Keep render.rs's — they are the call-site pins, unchanged, and they are the evidence no byte moved. an_indented_document_escapes_through_the_nesting (json.rs:572-586) is the one that earns its place: it exercises a shape dl --ls --json never produces. metadata's three are legitimately different (struct-routed, field-order-sensitive), not a third copy.
Minor — python_json_document is not Middle Man, keep it. One production caller at commands.rs:247, five test callers, a rendering concern named in dl's rendering module. The justification is sound. The ten-line docstring at render.rs:180-193 restating it is not.
Nits. (a) Naming splits on two axes: PythonFormatter/PythonPrettyFormatter on "pretty", as_python_writes_it/as_python_writes_it_indented on "indented"; pick one word, since the two are peers rather than base and variant. (b) metadata.rs:1096-1099 and four test sites spell crate::json::PythonPrettyFormatter inline while JsonKind is imported at line 45. (c) json.rs:167 says "an empty document" of a String::new(); the deleted original correctly said "empty string". (d) #346 is narrated five times — module doc, function doc, write_ensure_ascii doc, render.rs, CHANGELOG. One telling.
One more, mine. The json.rs module docstring now ends "the enumeration above is now the whole of what this module spells and there is nowhere else to look" — but that enumeration is completions.json, dl --ls --json, dl --completion-data, the SOURCE column and the DEVLAUNCH_TIMING=json line, and metadata.json is not in it, despite now being spelled here through PythonPrettyFormatter. In a module whose own cautionary tale is a docstring that promised more than the code delivered, that sentence should either name metadata.json or stop claiming completeness.
Spec
Ticket #346. Satisfied on every clause in it.
- "Scope it as the whole formatter, not just the escaper." Done.
render.rs'sPythonPrettyis gone in full, all ten delegations;metadata.rs'sPythonJsonFormattermoved wholesale. Diffing the old impl against the new one gives exactly one changed token —crate::json::write_ensure_ascii→write_ensure_ascii, the same function now in-module. Nine method bodies and the#[derive(Default)]struct character-identical,encode()changed only in formatter type with itsMetadataError::Encodemap untouched.metadata.jsonis unaffected by construction, as claimed. - "Exporting one
pubformatter (or onepubdocument-writing fn) costs the same singlepubline." Exactly one;PythonPrettyFormatterstayedpub(crate). - "
PythonPrettyhas exactly one production caller ... reached only fromcommands.rs:225." Still true:commands.rs:247is the only non-test caller, andcommands.rsis byte-unchanged. - "a legitimate difference stays (this document is indented, the compact one is on one line); what must not stay is two copies of one fact." Two formatters, one
write_ensure_ascii, one layout delegation. - "Note the ordering trap: doing this before #338 merges would move a public-API snapshot twice." #338 landed; one snapshot file moved.
Minor — the pin the other two got is not all at the new seam. "Red-first at the shared seam, and add the pin the other two got." Three new tests sit on as_python_writes_it_indented, but the full-ASCII sweep and the serde-escape-set pin exist only at the two old call sites and on the compact seam. Coverage is real because the escaping is genuinely shared, but the pin local to the new function is thinner than the ticket asks for, and it is two assert_eq!s to close.
Nit. One squashed commit, so "Red-first" is unverifiable from history.
Verdict
Comment. Approve on the substance. Nothing in the Rust has to change for me to be satisfied that the collapse moved no byte — that is now checked two ways, one of them against CPython. But one thing blocks the merge, and it is not the code.
Blocking, mechanical, and it arrived during this review: main moved to 6df02bc (#506 landed) and this PR is now CONFLICTING / DIRTY on CHANGELOG.md. It read MERGEABLE when the review opened, which is the whole point of #527.
The resolution is not free-form. main's [Unreleased] already carries a ### Changed — the --force placement entry from #506 — and this PR adds a second ### Changed heading below ### Fixed. Fold this PR's bullet under the existing heading rather than re-adding one. I built that resolution locally to check it: git diff origin/main -- CHANGELOG.md comes out 17 additions, 0 deletions, the entry lands under ### Changed inside [Unreleased], and ## [0.25.0] - 2026-08-28 stays byte-identical. Confirm all three by eye after resolving, because MERGEABLE is precisely the signal that says nothing about this failure and the #346 build agent already hit it twice on this ticket.
Optional and cheap, in the order I would spend on them: fix the json.rs:186-190 float caveat (wrong about disk, and worth closing with a write_f64 override rather than a sentence); name metadata.json in the module docstring's enumeration or stop calling it complete; add the ASCII sweep at the new seam.
# Conflicts: # CHANGELOG.md
…short one document
The comment on `PythonPrettyFormatter` said the listing's `disk` "is an
i64". It is not: `disk` is an object (`{"exclusiveBytes": u64}`, or
`{"atLeastBytes", "unreadable"}`) or null, and `unsaved` went unmentioned
altogether. The conclusion held -- no float reaches either wire -- but the
reason given was not the reason, which is a poor way to end a change whose
whole point was retiring a divergence that prose had been holding.
So the claim is enforced instead of asserted: `write_f64` forwards to
`python_repr`, the same spelling the compact formatter uses. It moves no byte
of either document, because neither carries a float -- `metadata.json`'s only
bare number is its `version: i64`, and every number in the listing sits inside
`disk`. It is there because `as_python_writes_it_indented` is `pub` and takes
any `Value`, so the day one does arrive nobody has to have remembered the
paragraph.
The module docstring enumerated five documents and called that "the whole of
what this module spells" while spelling a sixth: `metadata.json`, which this
same change had just routed through here. Named now, with a pointer to the
list a compiler will give you, since the hand-maintained one is the half that
rots -- and an over-promising docstring is this module's own cautionary tale.
Adds the full-ASCII sweep at the indented seam, which is the pin the compact
formatter and `metadata.json` both already had and this one did not: the
per-class tests each pin a class someone thought to name, which is how DEL
stayed wrong through three copies. Inside an object rather than at a bare
string, so it measures layout and escaping together rather than being the
compact sweep under a second name. Escaping `/` fails it and no other
indented test.
Every documented figure still holds: 182 rows under the old classifier, 813 under this one, 631 moved, 39 residual types.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees, and it takes #522's `BareCache` and passes it to the probe underneath rather than deciding the tag question for itself. A site's own reachability probe gives no tag account: it names one revision and asks the clone about it, where the tag question is about which of a clone's refs the mirror does not have, so `by_tags: None` is the honest answer. `Unsaved` survives as the wire flattening.
The unit is a site and everything nested inside it, decided bottom-up and conjunctively, so a worktree nested inside one being removed cannot be lost with it: the collectable arm of a verdict is reachable only when every nested site handed one back, and the caller passes no child list. That is T1 from PR #442's second review, made unrepresentable rather than guarded. What decides a site is a verdict rather than a boolean: Collectable(Proof) | Stands(NonEmpty<Reason>), where the proof is a private-field witness only a probe that answered can mint, and reasons accumulate up the subtree, so a site that is both dirty and locked reports both and a parent's line names the child. A lock is an unproved, never a loss. The metadata operation is `git worktree remove <the path git printed>`, per registration by name, and the clone-wide `git worktree prune` is deleted rather than gated: its domain is a readdir at act time, so a registration created after the plan was printed was inside its blast radius and no plan could name it. Ownership is a join against this clone's own listing, never the gitfile tail: a live worktree of another repository, nested in one of ours, used to be offered for removal unopposed under a reason that was false. The clone is the root of the same forest, which closes the clone-level guard's blindness to nested worktrees, and it takes #522's `BareCache` and passes it to the probe underneath rather than deciding the tag question for itself. A site's own reachability probe gives no tag account: it names one revision and asks the clone about it, where the tag question is about which of a clone's refs the mirror does not have, so `by_tags: None` is the honest answer. `Unsaved` survives as the wire flattening.
1 participant
Stacked on #504 (
wayfinder/devlaunch-349), not on main: that PR closed the DEL divergence in both copies of this escaping, and branching off main would have rebuilt the divergence it just fixed. Retarget to main when #504 merges; the diff to read is the top commit alone.Closes #346
What was duplicated
dl --ls --jsonandmetadata.jsonare bothjson.dumps(..., indent=2)documents pinned against bytes the Python build wrote, and each was produced by its own ~100-line formatter: the same layout methods forwarded to serde'sPrettyFormatter, and the sameensure_asciiloop. Nothing held the two equal, and the bill for that arrived one PR ago — the escaping gate was wrong about DEL in both, and closing it meant closing it twice.The indented spelling now lives in core's
jsonmodule beside the compact one, asas_python_writes_it_indented, and the renderer calls it. The metadata store keeps going through the formatter directly rather than through the new function, because its document is a struct: routing it through aValuefirst would put its field order at the mercy of the map implementation, and the field order is part of what that file is pinned on.One
publine, which is what the ticket costed it at. The compact-vs-indented split stays, because that difference is real.Byte-identity
wfparses--ls --json, so the bar was byte-identity rather than equivalence, and it is shown two ways.The pins did not move. Every assertion on that document is the same literal against the same call it was written against, with the formatter behind it deleted rather than edited: the shaped listing, the empty document, an emoji as its surrogate pair, DEL, and the whole of ASCII against the line
json.dumpsprinted for''.join(chr(c) for c in range(0x80)).git diffover the renderer contains no changed line carrying an assertion or a literal — the test bodies are untouched, so what they now measure is the collapse. That full-ASCII sweep went in on #504 to hold the two copies character-for-character equal; with one copy gone, its cross-check becomes the collapse's proof.And a differential sweep, since #345's review set that bar. The deleted formatter was reinstated verbatim from the base beside the collapsed path and both were run over every Unicode scalar value (1,112,064 of them, each nested inside an object and an array so the layout hand-off is exercised too) plus 16 adversarial documents: empty array and object, bare
null/0/-1/true/"", four-deep nesting, mixed arrays of objects, an empty key, floats including4e-5and1e16, all of ASCII, astral characters as keys as well as values, the scalars either side of the surrogate block,U+FFFE/U+FFFF/U+FEFF/U+10FFFF, and a 5000-character mixed run. 1,112,080 documents, 0 mismatches. The probe was scratch and is not in the diff.That sweep also settles a difference between the two copies that reading them does not:
dl's overrodeend_object_keyand core's does not. It makes no difference, becausePrettyFormatterdoes not override it either, so the extra delegation forwarded the trait's own no-op. The two spellings disagreed about how many methods the job takes and wrote identical bytes anyway, which is the kind of thing a second copy accumulates.metadata.jsonis unaffected by construction: its formatter moved file and name, and its body is unchanged.Gates
cargo test --workspace— green, 29 suites, exit 0.cargo clippy --locked --all-targets -- -D warnings— clean.cargo fmt --check— clean.[Unreleased], above## [0.25.0], adding lines only.Snapshot: one row, in
public-api.rest.txt— the freely regenerated tripwire, not the frozen promise, which is what #338 landing first bought. Placed by hand in the generator's order, because regenerating properly needs a nightly toolchain this container does not carry. CI'spublic-apijob is the check on that placement, and it passes, so the hand-placed row is what the generator produces.Two flakes seen, neither in this code.
the_json_listing_migrates_the_cache_and_the_table_does_notandwhat_is_typed_appears_on_the_top_line_and_narrows_the_list_below_iteach failed once under a loaded full-workspace run and each passed every time afterwards, including a clean full run of the suite. Both are process/pty tests on paths that never reach a JSON formatter.🤖 Generated with Claude Code
Summary by Sourcery
Centralize indented Python-compatible JSON formatting without changing the serialized documents consumed by existing tooling.
Enhancements:
Tests: