Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace CodeEntry-based namespace records with a dedicated NsEntry (doc + code) across snapshot types, detailed snapshots, build parsing, CLI edit handlers, and cr_sync; update EDN conversions, parsing, rendering, and embedded calcit snapshots; remove ns.schema/examples handling and add formatting-based migration behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Snapshot as SnapshotModule
participant CrSync
participant Disk
rect rgba(200,200,255,0.5)
User->>CLI: cr edit add-ns / modify file
CLI->>SnapshotModule: construct NsEntry(doc, Cirru)
CLI->>SnapshotModule: render_snapshot_content(snapshot)
SnapshotModule->>CLI: formatted content
CLI->>Disk: write file only if content changed
end
rect rgba(200,255,200,0.5)
CLI->>CrSync: trigger sync / notify
CrSync->>SnapshotModule: detect changes (SnapshotEntry::Ns / ::Def)
CrSync->>Disk: update detailed snapshot (DetailedNsEntry)
CrSync->>CLI: display change summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates snapshot namespace (:ns) entries away from reusing CodeEntry into a dedicated NsEntry type, aligning the in-memory model and serialized snapshot formats with the intended semantics (namespace has doc + code, but no schema/examples).
Changes:
- Introduces
NsEntry { doc, code }and updates compact snapshot (FileInSnapShot.ns) parsing/serialization to use it (including EDN compatibility handling). - Updates detailed snapshot support via
DetailedNsEntry, and adjustscr_syncto distinguish namespace vs definition payloads. - Migrates repository snapshot/cirru data to emit
:NsEntryfor namespaces; bumps crate/package versions to0.12.2.
Reviewed changes
Copilot reviewed 56 out of 61 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/snapshot.rs | Adds NsEntry, switches FileInSnapShot.ns to it, updates parsing/EDN conversions, removes ns schema validation on write. |
| src/detailed_snapshot.rs | Adds DetailedNsEntry and updates detailed snapshot file model to use it for ns. |
| build.rs | Updates embedded core snapshot build-time structs and parsing to use NsEntry for namespace entries. |
| src/bin/cr_sync.rs | Introduces SnapshotEntry enum to separate namespace vs def change payloads; updates detailed snapshot serialization for namespaces. |
| src/bin/cli_handlers/edit.rs | Updates edit add-ns flow to create NsEntry instead of CodeEntry. |
| src/cirru/calcit-core.cirru | Migrates embedded core snapshot namespace entries to :NsEntry. |
| calcit/**.cirru | Bulk migration of snapshot namespace entries from :CodeEntry to :NsEntry in fixtures/tests/debug/demo files. |
| docs/CalcitAgent.md | Documents that cr edit format also normalizes legacy namespace CodeEntry into NsEntry. |
| editing-history/2026-0310-1754-ns-entry-snapshot-migration.md | Adds migration note and validation checklist for this change. |
| Cargo.toml / Cargo.lock / package.json | Version bump to 0.12.2. |
| src/bin/cr_tests/type_fail.rs | Trivial formatting-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
build.rs (1)
213-236: Factor the shared:doc/:codeparsing out ofparse_ns_entry.
parse_ns_entrynow reimplements the same:doc/:codeextraction thatparse_code_entryalready owns. Keeping those paths separate makes the legacyCodeEntrycompatibility behavior easy to drift in a later cleanup. A small shared helper here, plus a regression test for both record shapes, would make the migration boundary safer.♻️ Possible refactor sketch
+fn parse_doc_and_code(record: &EdnRecordView, owner: &str, kind: &str) -> Result<(String, Cirru), String> { + let mut doc = String::new(); + let mut code: Option<Cirru> = None; + for (key, value) in &record.pairs { + match key.arc_str().as_ref() { + "doc" => doc = from_edn(value.clone()).map_err(|e| format!("{owner}: invalid `:doc`: {e}"))?, + "code" => code = Some(from_edn(value.clone()).map_err(|e| format!("{owner}: invalid `:code`: {e}"))?), + _ => {} + } + } + Ok((doc, code.ok_or_else(|| format!("{owner}: missing `:code` field in {kind}"))?)) +} + fn parse_ns_entry(edn: Edn, owner: &str) -> Result<NsEntry, String> { let record: EdnRecordView = match edn { Edn::Record(r) => r, other => { return Err(format!( "{owner}: expected NsEntry/CodeEntry record, got {}", format_edn_preview(&other) )); } }; - let mut doc = String::new(); - let mut code: Option<Cirru> = None; - for (key, value) in &record.pairs { - match key.arc_str().as_ref() { - "doc" => doc = from_edn(value.clone()).map_err(|e| format!("{owner}: invalid `:doc`: {e}"))?, - "code" => code = Some(from_edn(value.clone()).map_err(|e| format!("{owner}: invalid `:code`: {e}"))?), - _ => {} - } - } - Ok(NsEntry { - doc, - code: code.ok_or_else(|| format!("{owner}: missing `:code` field in NsEntry"))?, - }) + let (doc, code) = parse_doc_and_code(&record, owner, "NsEntry")?; + Ok(NsEntry { doc, code }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` around lines 213 - 236, parse_ns_entry duplicates the :doc/:code extraction implemented by parse_code_entry, risking drift; factor that shared logic into a small helper (e.g., parse_doc_and_code_from_record) that accepts an EdnRecordView and owner &str and returns (String, Cirru) or a Result with the same error messages, then call that helper from both parse_ns_entry and parse_code_entry and add regression tests covering both NsEntry and legacy CodeEntry record shapes to ensure behavior stays identical.src/bin/cr_sync.rs (1)
744-750: Defensive handling for unlikely variant mismatch.The
SnapshotEntry::Def(_)branch here handles a case that shouldn't occur based ondetect_snapshot_changeslogic (which always pairsNamespaceDefinitionwithSnapshotEntry::Ns). The defensive fallback is safe, but consider usingunreachable!()or logging a warning to surface any invariant violations during development.Optional: Make invariant violation explicit
ns: match new_entry { SnapshotEntry::Ns(entry) => entry.clone().into(), - SnapshotEntry::Def(_) => DetailedNsEntry { - doc: String::new(), - code: cirru_parser::Cirru::Leaf("".into()).into(), - }, + SnapshotEntry::Def(_) => { + eprintln!("Warning: NamespaceDefinition change received SnapshotEntry::Def - this is unexpected"); + DetailedNsEntry { + doc: String::new(), + code: cirru_parser::Cirru::Leaf("".into()).into(), + } + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/cr_sync.rs` around lines 744 - 750, The code currently falls back to constructing an empty DetailedNsEntry when matching SnapshotEntry::Def(_) in the ns assignment, masking a logic invariant expected by detect_snapshot_changes; replace that silent fallback with an explicit invariant assertion or observable warning: either call unreachable!() (or debug_assert!(false)) in the SnapshotEntry::Def(_) arm to panic in development, or log a warning via log::warn! (including the variant and context) before returning the fallback to surface violations; update the match arms around SnapshotEntry::Ns and SnapshotEntry::Def(_) accordingly so the invariant failure is explicit and debuggable.src/snapshot.rs (1)
1208-1211: Consider adding unit tests for NsEntry round-trip.While the integration tests exercise
NsEntrythrough the full snapshot loading path, dedicated unit tests forNsEntryparsing and serialization (similar totest_code_entry_with_examples) would improve coverage for edge cases.Would you like me to generate unit tests for
NsEntryround-trip serialization?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/snapshot.rs` around lines 1208 - 1211, Add a unit test that performs a round-trip serialize/deserialize of NsEntry (the struct with fields doc and code) similar to test_code_entry_with_examples: construct several NsEntry instances (including empty doc, long strings, and unicode), serialize them using the same snapshot entry serializer used by the existing tests, parse them back using the corresponding parser, and assert equality of the original and parsed NsEntry values; name the test something like test_nsentry_roundtrip and reuse the existing helper functions used by test_code_entry_with_examples to locate the correct serialization/parsing paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@build.rs`:
- Around line 213-236: parse_ns_entry duplicates the :doc/:code extraction
implemented by parse_code_entry, risking drift; factor that shared logic into a
small helper (e.g., parse_doc_and_code_from_record) that accepts an
EdnRecordView and owner &str and returns (String, Cirru) or a Result with the
same error messages, then call that helper from both parse_ns_entry and
parse_code_entry and add regression tests covering both NsEntry and legacy
CodeEntry record shapes to ensure behavior stays identical.
In `@src/bin/cr_sync.rs`:
- Around line 744-750: The code currently falls back to constructing an empty
DetailedNsEntry when matching SnapshotEntry::Def(_) in the ns assignment,
masking a logic invariant expected by detect_snapshot_changes; replace that
silent fallback with an explicit invariant assertion or observable warning:
either call unreachable!() (or debug_assert!(false)) in the
SnapshotEntry::Def(_) arm to panic in development, or log a warning via
log::warn! (including the variant and context) before returning the fallback to
surface violations; update the match arms around SnapshotEntry::Ns and
SnapshotEntry::Def(_) accordingly so the invariant failure is explicit and
debuggable.
In `@src/snapshot.rs`:
- Around line 1208-1211: Add a unit test that performs a round-trip
serialize/deserialize of NsEntry (the struct with fields doc and code) similar
to test_code_entry_with_examples: construct several NsEntry instances (including
empty doc, long strings, and unicode), serialize them using the same snapshot
entry serializer used by the existing tests, parse them back using the
corresponding parser, and assert equality of the original and parsed NsEntry
values; name the test something like test_nsentry_roundtrip and reuse the
existing helper functions used by test_code_entry_with_examples to locate the
correct serialization/parsing paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f4591fd-19bb-47c9-9f42-57ab01134efb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
Cargo.tomlbuild.rscalcit/add.cirrucalcit/debug/check-args.cirrucalcit/debug/debug-overflow.cirrucalcit/debug/macro-ns.cirrucalcit/debug/var-shadow.cirrucalcit/editor/calcit.cirrucalcit/editor/compact.cirrucalcit/fibo.cirrucalcit/test-algebra.cirrucalcit/test-cond.cirrucalcit/test-doc-smoke.cirrucalcit/test-edn.cirrucalcit/test-enum.cirrucalcit/test-fn.cirrucalcit/test-generics.cirrucalcit/test-gynienic.cirrucalcit/test-hygienic.cirrucalcit/test-invalid-tag.cirrucalcit/test-ir-type-info.cirrucalcit/test-js.cirrucalcit/test-lens.cirrucalcit/test-list.cirrucalcit/test-macro.cirrucalcit/test-map.cirrucalcit/test-math.cirrucalcit/test-method-errors.cirrucalcit/test-method-validation.cirrucalcit/test-nested-types.cirrucalcit/test-nil.cirrucalcit/test-optimize.cirrucalcit/test-proc-type-warnings.cirrucalcit/test-record.cirrucalcit/test-recur-arity.cirrucalcit/test-recursion.cirrucalcit/test-set.cirrucalcit/test-string.cirrucalcit/test-sum-types.cirrucalcit/test-tag-match-validation.cirrucalcit/test-traits.cirrucalcit/test-tuple.cirrucalcit/test-types-inference.cirrucalcit/test-types.cirrucalcit/test.cirrucalcit/type-fail/schema-call-arg-type-mismatch.cirrucalcit/type-fail/schema-kind-mismatch.cirrucalcit/type-fail/schema-required-arity.cirrucalcit/type-fail/schema-rest-missing.cirrucalcit/type-fail/schema-rest-unexpected.cirrucalcit/util.cirrudocs/CalcitAgent.mdediting-history/2026-0310-1754-ns-entry-snapshot-migration.mdpackage.jsonsrc/bin/cli_handlers/edit.rssrc/bin/cr_sync.rssrc/bin/cr_tests/type_fail.rssrc/cirru/calcit-core.cirrusrc/detailed_snapshot.rssrc/snapshot.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snapshot.rs`:
- Around line 96-105: The incremental-change payload currently only carries
FileChangeInfo { ns: Option<Cirru> } and handle_inc() exports only file.ns.code,
which drops namespace docs; update the change model to include the full NsEntry
(or add a separate ns_doc field) in FileChangeInfo and adjust handle_inc() and
any serialization/deserialization paths to emit/import the NsEntry (or ns_doc)
rather than only the Cirru code string; update references to FileInSnapShot and
any consumers that construct/read incremental payloads so namespace docs
round-trip through .compact-inc.cirru/watcher sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0bcdcd6-f3e4-46c0-9ab7-f613d4352912
📒 Files selected for processing (2)
src/bin/cli_handlers/edit.rssrc/snapshot.rs
Comment on lines
+96
to
105
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct NsEntry { | ||
| pub doc: String, | ||
| pub code: Cirru, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct FileInSnapShot { | ||
| pub ns: CodeEntry, | ||
| pub ns: NsEntry, | ||
| pub defs: HashMap<String, CodeEntry>, |
Contributor
There was a problem hiding this comment.
Incremental change export still drops namespace docs.
NsEntry makes namespace docs first-class here, but the incremental-change model is still FileChangeInfo { ns: Option<Cirru> }, and handle_inc() only exports file.ns.code. After this migration, cr edit ns-doc persists to compact.cirru but cannot round-trip through .compact-inc.cirru / watcher sync. The change payload needs to carry NsEntry too, or at least a separate namespace-doc field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/snapshot.rs` around lines 96 - 105, The incremental-change payload
currently only carries FileChangeInfo { ns: Option<Cirru> } and handle_inc()
exports only file.ns.code, which drops namespace docs; update the change model
to include the full NsEntry (or add a separate ns_doc field) in FileChangeInfo
and adjust handle_inc() and any serialization/deserialization paths to
emit/import the NsEntry (or ns_doc) rather than only the Cirru code string;
update references to FileInSnapShot and any consumers that construct/read
incremental payloads so namespace docs round-trip through
.compact-inc.cirru/watcher sync.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bin/cli_handlers/query.rs (1)
470-484:⚠️ Potential issue | 🟠 MajorKeep the JSON/raw schema path on the wrapped format too.
handle_def,handle_peek, and the plainquery schemapath now render wrapped schemas, butcode_entry_to_jsonand Line 666 still serialize fromto_schema_edn(). That leaves the CLI exposing two different schema shapes for the same definition, which is likely to break--jsonconsumers during this migration.Suggested fix
fn code_entry_to_json(entry: &snapshot::CodeEntry) -> serde_json::Value { let schema_json = match entry.schema.as_ref() { - CalcitTypeAnnotation::Fn(fn_annot) => snapshot::schema_edn_to_cirru(&fn_annot.to_schema_edn()) + CalcitTypeAnnotation::Fn(fn_annot) => snapshot::schema_edn_to_cirru(&fn_annot.to_wrapped_schema_edn()) .ok() .map(|c| cirru_to_json(&c)), _ => None, }; @@ if json { let schema_edn: cirru_edn::Edn = match code_entry.schema.as_ref() { CalcitTypeAnnotation::Dynamic => cirru_edn::Edn::Nil, - CalcitTypeAnnotation::Fn(fn_annot) => fn_annot.to_schema_edn(), + CalcitTypeAnnotation::Fn(fn_annot) => fn_annot.to_wrapped_schema_edn(), _ => cirru_edn::Edn::Nil, }; println!("{}", cirru_edn::format(&schema_edn, true)?); return Ok(()); }Also applies to: 510-516, 663-669, 675-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/cli_handlers/query.rs` around lines 470 - 484, The JSON path is still serializing the old/raw schema via to_schema_edn(), causing inconsistent shapes; update code_entry_to_json (and the JSON branches in handle_def, handle_peek, and the plain "query schema" path) to detect when code_entry.schema is CalcitTypeAnnotation::Fn and use fn_annot.to_wrapped_schema_edn() for the JSON/EDN output (the same value passed to snapshot::schema_edn_to_cirru), falling back to the existing to_schema_edn() for other variants so consumers always see the wrapped schema shape for function entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bin/cli_handlers/query.rs`:
- Around line 470-484: The JSON path is still serializing the old/raw schema via
to_schema_edn(), causing inconsistent shapes; update code_entry_to_json (and the
JSON branches in handle_def, handle_peek, and the plain "query schema" path) to
detect when code_entry.schema is CalcitTypeAnnotation::Fn and use
fn_annot.to_wrapped_schema_edn() for the JSON/EDN output (the same value passed
to snapshot::schema_edn_to_cirru), falling back to the existing to_schema_edn()
for other variants so consumers always see the wrapped schema shape for function
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d37023e6-2589-45ff-b047-b71bf54b0874
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlpackage.jsonsrc/bin/cli_handlers/query.rssrc/cirru/calcit-core.cirru
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/cirru/calcit-core.cirru
… ir mode with schema annotations); tag 0.12.5
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/codegen/gen_ir.rs`:
- Around line 274-287: The Map/Set arms in gen_ir.rs call dump_code on arbitrary
members but dump_code can still hit unreachable! for data-carrying Calcit
variants (e.g., Ref, Recur, Buffer, Trait, Impl) causing a panic; update
dump_code (the serializer used by emit_ir) to be total by either (A) adding
match arms to serialize those Calcit variants into Edn (or a safe
placeholder/tagged form) so nested Map/Set members never fall through to
unreachable!, or (B) change dump_code's signature to return Result<Edn, Error>,
propagate that Result through the Map/Set arms in gen_ir.rs and up to emit_ir,
and return a descriptive error when encountering unsupported Calcit variants;
locate and update the dump_code function and the Map/Set handling in gen_ir.rs
accordingly and ensure emit_ir returns/propagates the new error type instead of
panicking.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1a84866d-0d99-4bdd-b814-bad434f5de72
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlpackage.jsonsrc/codegen/gen_ir.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- Cargo.toml
…n error snapshots and format-cirru-edn; tag 0.12.6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.