Use serde_json_lenient, implement setup-zed experimental command - #1195
Conversation
📝 WalkthroughWalkthroughThe workspace replaces ChangesLenient JSON migration
Zed SSH setup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant MinimalCLI
participant SessionStore
participant ZedSettings
Operator->>MinimalCLI: invoke hidden setup-zed
MinimalCLI->>SessionStore: resolve session and daemon connection
SessionStore-->>MinimalCLI: return session connection data
MinimalCLI->>ZedSettings: read and parse settings
MinimalCLI->>ZedSettings: insert or update SSH entry
ZedSettings-->>MinimalCLI: write settings and backup
MinimalCLI-->>Operator: print entry or report outcome
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
3add122 to
e3e379d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
crates/minimal/src/lib.rs (1)
2383-2484: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove the settings-file I/O off the async path.
cmd_session_setup_zedisasync, andzed::read_settingsandzed::write_settingscallstd::fsdirectly. If the settings path lives on a wedged filesystem (for example a stalled network mount for$HOME), the runtime worker blocks. Wrap the read/upsert/write block intokio::task::spawn_blocking, ascrates/minimal/src/diag/collect.rsdoes for filesystem work.Based on learnings: "In this Rust repo, avoid blocking filesystem work in async contexts (e.g., don't call
std::fsdirectly from async tasks)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal/src/lib.rs` around lines 2383 - 2484, Move the settings-file I/O in cmd_session_setup_zed into a tokio::task::spawn_blocking closure, including zed::read_settings, zed::upsert, and zed::write_settings, then await and propagate both task and operation errors. Keep path selection, outcome handling, and user-facing messages on the async path while ensuring no direct filesystem work remains there.Source: Learnings
crates/minimal/src/zed.rs (2)
216-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrite the settings file atomically.
std::fs::writetruncatessettings.jsonfirst. If the process dies or the disk fills during the write, the user is left with a truncated settings file, and Zed then starts with no settings. Write the rendered document to a temporary file in the same directory, then rename it over the target. The.bakcopy limits the damage, but a rename removes the window entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal/src/zed.rs` around lines 216 - 221, Replace the direct std::fs::write call in the settings-writing flow with an atomic same-directory temporary-file write followed by a rename over path. Ensure the complete rendered content is flushed to the temporary file before renaming, preserve the existing error context, and clean up the temporary file if writing or renaming fails.
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCentralize the shared SSH contract constants.
SESSION_ID_ENVmatches the daemon value, and both workspace values resolve to/workbench. Define the constants inminimald-rpc, which both crates already depend on, to prevent silent session-routing or project-path drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal/src/zed.rs` around lines 21 - 29, Move the shared SSH contract constants SESSION_ID_ENV and WORKSPACE_ROOT from the minimal crate into minimald-rpc, defining them once with the existing values. Update the daemon and CLI references to use the centralized minimald-rpc symbols, removing duplicate local definitions while preserving the current session-routing and workspace-root behavior.crates/minimal-tui/src/state.rs (1)
71-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise lenient JSON syntax in the state tests.
These tests cover valid JSON and empty input, but they do not prove support for comments or trailing commas. Add a fixture with both forms so a future parser replacement cannot pass without preserving the intended lenient behavior. The crate documents these forms as enabled by default. (docs.rs)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal-tui/src/state.rs` around lines 71 - 84, Add a state deserialization test alongside the existing DashState tests that uses JSON containing both comments and trailing commas, then assert it parses successfully into the expected DashState value. Keep the existing valid-JSON and empty-input tests unchanged, and exercise the default lenient parser behavior through serde_json_lenient.Source: MCP tools
crates/sessions/src/store.rs (1)
336-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression fixture for lenient storage input.
The loader tests currently create canonical JSON with
serde_json_lenient::to_vec. A regression to strict parsing would keep those tests green. Add hand-writtenrecord.jsonandindex.jsonfixtures with a comment or trailing comma, then verifyDiskLoader::newand self-healing accept them. These are the lenient input forms supported by the dependency. (docs.rs)Also applies to: 403-403
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sessions/src/store.rs` at line 336, Add regression fixtures for lenient storage input in the loader tests: hand-write record.json and index.json using supported comments or trailing commas instead of generating them with serde_json_lenient::to_vec. Verify both DiskLoader::new and the self-healing path successfully load these fixtures, preserving coverage against a future switch to strict JSON parsing.Source: MCP tools
deny.toml (1)
13-14: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftTrack the transitive
bitmapsdependency.bitmaps 3.2.1enters throughnickel-lang-core→nickel-lang-vector→imbl-sized-chunks. Track an upstream update or replacement, then remove this ignore whenbitmapsleaves the lockfile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deny.toml` around lines 13 - 14, Track the transitive bitmaps 3.2.1 dependency through nickel-lang-core, nickel-lang-vector, and imbl-sized-chunks; update or replace the upstream dependency so bitmaps leaves the lockfile, then remove the RUSTSEC-2026-0247 ignore entry from deny.toml.Source: MCP tools
crates/graph/src/wire.rs (2)
1144-1144: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise lenient input in the wire tests.
These fixtures now use
serde_json_lenient::to_vec, but they still generate canonical JSON. They do not prove that readers accept the parser's added comment or trailing-comma forms. Add one hand-written fixture using a supported lenient form and retain a canonical fixture for compatibility. The dependency documents comments and trailing commas as its added syntax. (docs.rs)Also applies to: 1245-1245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/graph/src/wire.rs` at line 1144, Update the wire tests around the LocalFileHeader fixtures to include one hand-written JSON fixture containing a supported lenient form, such as a comment or trailing comma, and retain a separate canonical JSON fixture to verify compatibility. Ensure both fixtures are exercised by the existing reader tests and avoid generating the lenient case with serde_json_lenient::to_vec.
265-275: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve the graph stream byte contract.
GraphWriterhashes the exact record payload bytes, butSTREAM_VERSIONremains unchanged. Verify that everyserde_json_lenient::to_veccall preserves the previous writer's bytes and that an older reader can consume a new stream. If the bytes can change, add a version bump or compatibility path. Add a golden stream fixture because synchronous and asynchronous round trips can pass when both sides use the same new serializer.Also applies to: 296-310, 338-338, 744-755, 777-793, 822-822
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/graph/src/wire.rs` around lines 265 - 275, Preserve the existing serialized record bytes across every GraphWriter serde_json_lenient::to_vec call, including the BuildSpecRecord and the additional call sites noted in the review. Compare against the previous writer format and either retain byte compatibility or bump STREAM_VERSION and add the corresponding reader compatibility path. Add a golden stream fixture that validates exact bytes and is exercised by both synchronous and asynchronous round trips.crates/op/src/oci_image.rs (1)
129-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify OCI blob serialization before changing the digest contract.
The image configuration and manifest bytes are external OCI JSON blobs, and this function hashes their exact bytes for blob paths and descriptor digests. The OCI image specification defines these as JSON media types. (github.com)
serde_json_lenientadds lenient input syntax and provides its own serializer. (docs.rs) Verify thatserde_json_lenient::to_vecemits strict JSON and preserves expected image digest fixtures. If byte changes are intentional, document the digest migration; otherwise keep strict serialization at this external boundary.Also applies to: 179-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/op/src/oci_image.rs` at line 129, Verify the serialization used for OCI image configuration and manifest bytes in the relevant image-building function, including both serde_json_lenient::to_vec calls. Ensure the external OCI blobs are emitted as strict JSON and preserve existing digest and blob-path fixtures; if lenient serialization changes bytes, replace it with the established strict JSON serializer, or document an intentional digest migration.docs/fuzzing.md (1)
247-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winQualify the fuzzing exemption for the lenient parser.
serde_json_lenientadds comment and trailing-comma grammar to itsserde_jsonfork. Upstreamserde_jsonfuzzing does not, by itself, establish coverage of those added parser paths. (docs.rs) Keep this exemption only if the workspace's dependency version covers the lenient paths with tests or fuzzing. Otherwise, add a target or narrow the rationale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/fuzzing.md` at line 247, Update the lcache::EntryMeta::read_from fuzzing-exemption entry to verify that the workspace’s serde_json_lenient version has tests or fuzzing covering its comment and trailing-comma parsing paths; if not, add a dedicated fuzz target or narrow the exemption rationale so upstream serde_json coverage is not treated as sufficient.crates/minimald-rpc/src/lib.rs (1)
897-897: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise lenient JSON syntax in an RPC test.
This test only parses standard
{}. Add a request with a comment or trailing comma, then assert the same defaults. This verifies the intended lenient behavior at the RPC boundary. (docs.rs)Suggested regression case
assert_eq!(req, DiagBundleRequest::default()); + + let lenient: DiagBundleRequest = serde_json_lenient::from_str( + r#"{"include_state_listing": true, // accepted comment + }"#, + ) + .expect("deserialize lenient JSON"); + assert_eq!(lenient, DiagBundleRequest::default());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald-rpc/src/lib.rs` at line 897, Update the RPC test around DiagBundleRequest deserialization to parse a lenient JSON payload containing a comment or trailing comma instead of only standard "{}". Keep the existing assertions and verify the deserialized request retains the same default values.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/checkouts/src/error.rs`:
- Line 32: Review the public checkouts::Error API around StatefileInvalid before
merging: either preserve the existing serde_json::Error-compatible payload via a
crate-owned wrapper and update workspace consumers that format it, or explicitly
treat the serde_json_lenient::Error payload change as breaking by applying the
required version bump and updating all affected consumers.
In `@crates/minimal/src/zed.rs`:
- Line 1: Update the module documentation comment to show the parser’s command
order, using `min session setup-zed <session>` instead of `min session <id>
setup-zed`; leave the parsing implementation and
setup_zed_parses_as_a_session_verb unchanged.
- Around line 205-214: Update the backup path construction in the read/backup
block of cmd_session_setup_zed to append “.json.bak” to the complete filename
rather than replacing its existing extension; preserve all existing read, write,
and error-handling behavior while ensuring settings.jsonc becomes
settings.jsonc.json.bak and extensionless names receive the same suffix.
---
Nitpick comments:
In `@crates/graph/src/wire.rs`:
- Line 1144: Update the wire tests around the LocalFileHeader fixtures to
include one hand-written JSON fixture containing a supported lenient form, such
as a comment or trailing comma, and retain a separate canonical JSON fixture to
verify compatibility. Ensure both fixtures are exercised by the existing reader
tests and avoid generating the lenient case with serde_json_lenient::to_vec.
- Around line 265-275: Preserve the existing serialized record bytes across
every GraphWriter serde_json_lenient::to_vec call, including the BuildSpecRecord
and the additional call sites noted in the review. Compare against the previous
writer format and either retain byte compatibility or bump STREAM_VERSION and
add the corresponding reader compatibility path. Add a golden stream fixture
that validates exact bytes and is exercised by both synchronous and asynchronous
round trips.
In `@crates/minimal-tui/src/state.rs`:
- Around line 71-84: Add a state deserialization test alongside the existing
DashState tests that uses JSON containing both comments and trailing commas,
then assert it parses successfully into the expected DashState value. Keep the
existing valid-JSON and empty-input tests unchanged, and exercise the default
lenient parser behavior through serde_json_lenient.
In `@crates/minimal/src/lib.rs`:
- Around line 2383-2484: Move the settings-file I/O in cmd_session_setup_zed
into a tokio::task::spawn_blocking closure, including zed::read_settings,
zed::upsert, and zed::write_settings, then await and propagate both task and
operation errors. Keep path selection, outcome handling, and user-facing
messages on the async path while ensuring no direct filesystem work remains
there.
In `@crates/minimal/src/zed.rs`:
- Around line 216-221: Replace the direct std::fs::write call in the
settings-writing flow with an atomic same-directory temporary-file write
followed by a rename over path. Ensure the complete rendered content is flushed
to the temporary file before renaming, preserve the existing error context, and
clean up the temporary file if writing or renaming fails.
- Around line 21-29: Move the shared SSH contract constants SESSION_ID_ENV and
WORKSPACE_ROOT from the minimal crate into minimald-rpc, defining them once with
the existing values. Update the daemon and CLI references to use the centralized
minimald-rpc symbols, removing duplicate local definitions while preserving the
current session-routing and workspace-root behavior.
In `@crates/minimald-rpc/src/lib.rs`:
- Line 897: Update the RPC test around DiagBundleRequest deserialization to
parse a lenient JSON payload containing a comment or trailing comma instead of
only standard "{}". Keep the existing assertions and verify the deserialized
request retains the same default values.
In `@crates/op/src/oci_image.rs`:
- Line 129: Verify the serialization used for OCI image configuration and
manifest bytes in the relevant image-building function, including both
serde_json_lenient::to_vec calls. Ensure the external OCI blobs are emitted as
strict JSON and preserve existing digest and blob-path fixtures; if lenient
serialization changes bytes, replace it with the established strict JSON
serializer, or document an intentional digest migration.
In `@crates/sessions/src/store.rs`:
- Line 336: Add regression fixtures for lenient storage input in the loader
tests: hand-write record.json and index.json using supported comments or
trailing commas instead of generating them with serde_json_lenient::to_vec.
Verify both DiskLoader::new and the self-healing path successfully load these
fixtures, preserving coverage against a future switch to strict JSON parsing.
In `@deny.toml`:
- Around line 13-14: Track the transitive bitmaps 3.2.1 dependency through
nickel-lang-core, nickel-lang-vector, and imbl-sized-chunks; update or replace
the upstream dependency so bitmaps leaves the lockfile, then remove the
RUSTSEC-2026-0247 ignore entry from deny.toml.
In `@docs/fuzzing.md`:
- Line 247: Update the lcache::EntryMeta::read_from fuzzing-exemption entry to
verify that the workspace’s serde_json_lenient version has tests or fuzzing
covering its comment and trailing-comma parsing paths; if not, add a dedicated
fuzz target or narrow the exemption rationale so upstream serde_json coverage is
not treated as sufficient.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43419166-8fd2-4538-bad0-4b0dc5047ee1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
Cargo.tomlcrates/checkouts/Cargo.tomlcrates/checkouts/src/error.rscrates/checkouts/src/lib.rscrates/common/Cargo.tomlcrates/common/src/spec_hash.rscrates/diagnostics/Cargo.tomlcrates/diagnostics/src/bundle.rscrates/diagnostics/src/kmsg.rscrates/diagnostics/src/procs.rscrates/diagnostics/src/redact.rscrates/diagnostics/src/system.rscrates/graph/Cargo.tomlcrates/graph/src/loader.rscrates/graph/src/wire.rscrates/lcache/Cargo.tomlcrates/lcache/src/entry_meta.rscrates/minimal-client/Cargo.tomlcrates/minimal-client/src/lib.rscrates/minimal-tui/Cargo.tomlcrates/minimal-tui/src/state.rscrates/minimal/Cargo.tomlcrates/minimal/src/diag/collect.rscrates/minimal/src/diag/guest.rscrates/minimal/src/diag/net.rscrates/minimal/src/lib.rscrates/minimal/src/zed.rscrates/minimal/tests/bug.rscrates/minimal/tests/cli.rscrates/minimald-rpc/Cargo.tomlcrates/minimald-rpc/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/connection.rscrates/minimald/src/diag.rscrates/minimald/src/net/policy.rscrates/minimald/src/rpc.rscrates/minimald/src/test_harness.rscrates/minvmd/Cargo.tomlcrates/minvmd/examples/exec.rscrates/minvmd/src/cmd/config.rscrates/minvmd/src/cmd/status.rscrates/minvmd/src/rpc_client.rscrates/minvmd/tests/config_cli_integration.rscrates/minvmd/tests/minimald_session_integration.rscrates/minvmd/tests/resource_vm_integration.rscrates/minvmd/tests/supervision_integration.rscrates/mip/Cargo.tomlcrates/mip/src/cmd_dump.rscrates/mip/src/cmd_run.rscrates/mlog/Cargo.tomlcrates/mlog/src/lib.rscrates/op/Cargo.tomlcrates/op/src/error.rscrates/op/src/oci_image.rscrates/sessions/Cargo.tomlcrates/sessions/src/lib.rscrates/sessions/src/store.rscrates/sessions/src/wire/errors.rscrates/sessions/src/wire/policy.rscrates/sessions/src/wire/primitives.rscrates/sessions/src/wire/request.rscrates/sessions/tests/client_flow1.rsdeny.tomldocs/fuzzing.md
e3e379d to
b0a6789
Compare
Note
Replace
serde_jsonwithserde_json_lenientworkspace-wide and addsetup-zedsession commandserde_jsonwithserde_json_lenientacross all crates at the workspace level, so JSON parsing throughout the daemon, client, RPC layer, session store, graph wire format, and diagnostics now accepts lenient JSON syntax (e.g. trailing commas, comments).min session setup-zed <session>subcommand that generates a Zed SSH connection entry for a session; with--printit outputs the JSON entry, otherwise it upserts the entry into Zed'ssettings.jsonand reports whether it was Added, Updated, or Unchanged.ssh_connectionsentry, and tracking upsert outcomes.serde_json_lenientis a drop-in replacement but error types change fromserde_json::Errortoserde_json_lenient::Error; any code that matches on error internals may need updates. RUSTSEC-2026-0247 is suppressed indeny.toml.Macroscope summarized b0a6789.
Summary by CodeRabbit