refactor(codegen): serialize JSON artifacts with serde - #311
Conversation
Replace handwritten writers for semantics, decisions, optimization, and Rust support manifests with typed Serialize views and one shared pretty-JSON helper. Preserve the existing JSON schemas while removing duplicated escaping and formatting logic. Persist Rust support trust decisions as versioned JSON while retaining locked merges and atomic replacement. Replace the TOML parser stack with serde_json, and use the same serializer for the G4 frontend corpus snapshots.
Copy/Paste DetectionNo duplications found in 10 changed non-generated Rust file(s) (threshold: 100 tokens). |
📝 WalkthroughWalkthroughThe code generator replaces manual JSON and TOML serialization with typed ChangesJSON Serialization Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @tinovyatkin's task in 45m 1s —— View job Code reviewReviewed 🔴 1. The port-evidence oracle lock was deleted instead of re-pinned
Also deleted along the way: Net effect (verified):
Why this trade is bad: the change it accommodates has provably zero output impact. serde_json's escape table escapes exactly Suggested fix: revert 🟠 2. Trust-store format switch: silent reset by default, hard error via the documented env var
(i) Default path — silent reset. The (ii) Nothing hints that the file is simply in the legacy TOML format — the build just stops until the user finds and deletes it. Only This is the same concern Codex raised. A minimal fix: when the parse fails, check whether the content isn't JSON (or the path ends in 🟡 3. Nit: stale
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/antlr-rust-codegen/src/grammar/transform/artifact.rs`:
- Around line 15-125: Preserve omitted optional fields in the optimization
manifest serialization. In
crates/antlr-rust-codegen/src/grammar/transform/artifact.rs (lines 15-125), add
serde skip-if-none handling to boundary_rule, projected, and SourceManifest
fields path, start, and end. In
crates/antlr-rust-codegen/src/rust_support/mod.rs (lines 73-89), apply the same
handling to any corresponding optional serialized fields so the legacy artifact
shape remains unchanged.
In `@crates/antlr-rust-codegen/src/rust_support/identity.rs`:
- Around line 413-416: Add #[allow(clippy::disallowed_methods)] to the mod tests
declaration in identity.rs, covering all insta snapshot macros within that test
module.
In `@crates/antlr-rust-codegen/src/rust_support/mod.rs`:
- Line 214: Update the manifest construction in the module’s transform
configuration to derive the transform value from the existing TRANSFORM_FILE
symbol instead of the hardcoded "Rust/transformGrammar.py" literal. Define a
shared module-level manifest-path constant next to TRANSFORM_FILE if needed,
ensuring the path used by the generator and the manifest remain synchronized
when the filename changes.
In `@crates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/rust_support.rs`:
- Around line 226-230: The tests currently assert JSON content via
formatting-sensitive substring checks. Replace the raw-text policy assertions in
the shown manifest validation and in
current_java_transform_runs_from_the_staged_tree with structured lookups on the
parsed serde_json::Value, asserting the policy value equals "assume-true";
retain the existing JSON parsing and other test behavior.
- Around line 237-246: The complete JSON structure assertion in the enclosing
test should use a named external insta snapshot instead of the hand-written
serde_json::json! assert_eq!. Replace the assertion around option with an
available snapshot macro, place the named snapshot under the sibling snapshots
directory, and confirm the test has #[allow(clippy::disallowed_methods)].
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bb4e90f5-5ae0-4640-a070-c17e5a8defca
⛔ Files ignored due to path filters (10)
Cargo.lockis excluded by!**/*.lockcrates/antlr-rust-codegen/src/generator/snapshots/antlr_rust_codegen__generator__tests__fixed_lookahead_decisions_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/src/generator/snapshots/antlr_rust_codegen__generator__tests__render_forced_adaptive_decisions_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/src/generator/snapshots/antlr_rust_codegen__generator__tests__semantics_manifest_with_untranslated_predicate.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/src/rust_support/snapshots/antlr_rust_codegen__rust_support__identity__tests__persisted_trust_store.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/snapshots/antlr4_rust_gen_cli__compatibility__antlr4rust_compat_semantics_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/snapshots/antlr4_rust_gen_cli__optimizations__ll1_no_fallback_decisions_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/snapshots/antlr4_rust_gen_cli__semantics__named_parser_actions_semantics_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/snapshots/antlr4_rust_gen_cli__semantics__recog_receiver_semantics_manifest.snapis excluded by!**/*.snapcrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/snapshots/antlr4_rust_gen_cli__transforms__precedence_ladder_optimization_manifest.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
crates/antlr-rust-codegen/Cargo.tomlcrates/antlr-rust-codegen/README.mdcrates/antlr-rust-codegen/src/grammar/transform/artifact.rscrates/antlr-rust-codegen/src/json.rscrates/antlr-rust-codegen/src/lib.rscrates/antlr-rust-codegen/src/rust_support/identity.rscrates/antlr-rust-codegen/src/rust_support/mod.rscrates/antlr-rust-codegen/src/semantics/manifest.rscrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/rust_support.rscrates/antlr-rust-codegen/tests/antlr4_rust_gen_cli/semantics.rscrates/antlr-rust-g4-parser/Cargo.tomlcrates/antlr-rust-g4-parser/src/frontend.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 330f7832de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Regenerate the checked-in Rust syntax manifests with the Serde formatter, centralize the Rust support transform path, and make JSON assertions structure-aware with an insta snapshot for the complete option value. Drop the obsolete whole-module lock on frontend test implementation details from the six Phase A evidence ledgers while retaining the port-evidence validator for all declared evidence and remaining locked sections.
|
Addressed the Claude review in
Also reran workspace clippy, all Rust-support CLI tests, both grammar-frontend validators, and the Rust syntax drift check. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aef0a9be0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
📊 Source Code Metrics (this PR vs
|
| File | Cyclomatic | Cognitive | Functions | LLOC | MI |
|---|---|---|---|---|---|
| crates/antlr-rust-g4-parser/src/frontend.rs | 217 (main: 227) 🟢 | 88 (main: 91) 🟢 | 68 ⚪ | 307 (main: 311) 🟢 | 0 ⚪ |
| crates/antlr-rust-codegen/src/rust_support/identity.rs | 96 (main: 97) 🟢 | 53 (main: 54) 🟢 | 23 ⚪ | 162 (main: 161) 🔴 | 0 ⚪ |
| crates/antlr-rust-codegen/src/rust_support/mod.rs | 125 (main: 136) 🟢 | 84 (main: 94) 🟢 | 22 (main: 23) 🟢 | 133 (main: 160) 🟢 | 0 ⚪ |
| crates/antlr-rust-codegen/src/grammar/transform/artifact.rs | 26 (main: 54) 🟢 | 2 (main: 41) 🟢 | 12 (main: 14) 🟢 | 14 (main: 122) 🟢 | 14.11 (main: 10.04) 🟢 |
| crates/antlr-rust-codegen/src/semantics/manifest.rs | 21 (main: 43) 🟢 | 4 (main: 40) 🟢 | 6 (main: 8) 🟢 | 13 (main: 98) 🟢 | 18.64 (main: 15.22) 🟢 |
| crates/antlr-rust-codegen/src/lib.rs | 3 ⚪ | 0 ⚪ | 2 ⚪ | 4 ⚪ | 38.36 (main: 38.57) 🔴 |
| crates/antlr-rust-codegen/src/json.rs | 2 🆕 | 0 🆕 | 1 🆕 | 3 🆕 | 63.69 🆕 |
| crates/antlr-rust-codegen/src/rust_support/python.rs | 11 ⚪ | 7 ⚪ | 1 ⚪ | 27 ⚪ | 36.57 (main: 36.52) 🟢 |
Generated by mehen v1.8.1 — the code quality watcher.
Summary
semantics.json,decisions.json,optimizations.json, andrust-support.jsonwith typed Serde modelstrusted-support.jsonwhile preserving locked merges and atomic replacementtomlparser stack withserde_jsonand use thesame serializer for the G4 frontend corpus snapshots
whole-test-module hash from the six affected Phase A evidence ledgers
Why
Codegen emitted several JSON artifacts through three independent escaping and
layout implementations. Meanwhile, the
tomldependency was used only for thesmall persistent trust store. A single serializer reduces format-handling code
owned by the project and removes the larger TOML parser stack.
The existing generated manifest schemas and values are unchanged. Every
updated snapshot was compared as parsed, sorted JSON before acceptance; only
pretty-print layout changed. Generated Rust and the generated-code/runtime API
contract are unaffected.
The default persistent trust path changes from
trusted-support.tomltotrusted-support.json. This trust-store feature has not appeared in a publishedrelease, so there is no released TOML store to migrate.
The port-evidence validator still checks all 523 active ledgers, declared
evidence files, and remaining locked sections. The removed lock hashed the
entire frontend test module, coupling evidence validity to implementation-only
test helpers such as JSON string escaping.
Validation
cargo test --locked --workspace --all-featurescargo test --locked -p antlr-rust-g4-parser --all-featurescargo test --locked -p antlr-rust-codegen --test antlr4_rust_gen_cli rust_support::cargo clippy --locked --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checktools/rust-syntax/update-generated.sh --checknode tools/grammar-frontend/validate-port-evidence.mjsnode tools/grammar-frontend/validate-interp-fixtures.mjsgit diff --checkjq -Stomlpackage remains and the runtime graphcontains neither
tomlnorserde_json