fix(cli): route the last five commands through the renderer; --streaming does not bound memory (#75) - #103
Merged
Merged
Conversation
CLAUDE.md required all user-facing output to go through "the rendering system". When #75 was filed no such system existed and every command built text with println!. `render.rs` was added since, and fifteen commands migrated onto it, but five were missed: cmd_fingerprint_corpus 24 println! cmd_population_drift 20 cmd_tree_diff 16 cmd_ingest_github 8 cmd_profiles 6 117 println! calls across the CLI, down to 53 — at most four per command, one per format branch, emitting an already-rendered string. That is the pattern the constraint is actually asking for. The cost the issue predicted was real and present. `fingerprint` is listed in RENDERS_MARKDOWN and emits no fallback warning, but `--corpus --format markdown` printed `=== Corpus Shape Index ===`: the claim held for the path the honesty test exercised and not for the sub-mode beside it. Same for `drift --tree`, `drift --group-by` and `profiles`. command_sub_modes_emit_real_markdown covers all four, asserting both that Markdown differs from text and that text carries no Markdown headings. population_drift's text_output_works asserted `"Group-by: repo"` with a single space; the renderer pads field labels to a common width. The assertion now squeezes whitespace rather than pinning one alignment. CLAUDE.md's constraint is reworded to describe what the CLI does, since the version in the file described a system that did not exist when it was written. Refs #75
The flag was documented as "bounded memory, sketch-based stats". Measured on a 15 MB, 120,000-record input with `vajra stats`: default (DOM) 233 MB peak RSS --streaming 402 MB peak RSS 1.7x more, not less. load_adaptive's large-input branch — the one taken when memory actually matters — reads the whole file, parses a full DOM, and then materialises a Vec<JsonEvent> beside it, plus two more full walks for node count and depth. The code comment says "For now, still parse DOM but convert to events"; the help text did not. A user passing --streaming to survive a large file got the opposite of what the name promised. Same defect class as --redact silently doing nothing (#71) and --format markdown falling through to text: a flag accepted without doing what it says. - Help text states what it selects and that it is not yet bounded. - Passing it warns, unless --quiet. The sketch accumulators it selects are real and tested; what is missing is a parser to feed them incrementally. - docs/src/streaming.md carried a whole page of guarantees for a system that does not exist, ending in "bounded memory independent of input size". It now opens with the measurement and marks the design as intended rather than implemented. This also recasts #75, which framed `separation` and `invariants` as two commands violating rule 1. Too narrow: nothing streams, so they are not exceptions to a rule the rest of the tool follows. Rule 1 is restated as a falsifiable design constraint plus an honest note that the codebase does not meet it — "if it doesn't stream, it doesn't ship" was unfalsifiable while nothing shipped that streamed. Refs #75 Refs #102
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR documents that streaming does not yet provide bounded memory, adds a corresponding CLI warning, and migrates several non-JSON command outputs to ChangesCLI output and streaming behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@vajra-cli/src/main.rs`:
- Around line 2141-2222: Update corpus_index_report to render the CorpusIndex
fields groups_indexed and shapes_in_multiple_groups alongside the existing
documents_indexed and shapes_in_multiple_documents fields, preserving the JSON
output’s distinction between document-level and group-level counts.
In `@vajra-cli/tests/format_honesty.rs`:
- Around line 393-480: Add the profiles command identifier to the
RENDERS_MARKDOWN allow-list used by warn_unimplemented_format(), so profiles
--format markdown does not emit the fallback warning with or without --quiet.
Preserve the existing Markdown rendering behavior and other command entries.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b80ec569-3f3e-476c-b969-91c41c1936de
📒 Files selected for processing (5)
CLAUDE.mddocs/src/streaming.mdvajra-cli/src/main.rsvajra-cli/tests/format_honesty.rsvajra-cli/tests/population_drift.rs
… test hole CodeRabbit on #103, both findings correct, plus a hole they exposed in my own test. - profiles emitted '## Built-in Profiles' while warning it had no markdown renderer — the inverse false claim to the one this PR fixes. Added to RENDERS_MARKDOWN. - corpus_index_report omitted groups_indexed and shapes_in_multiple_groups, which the JSON carries. Group-level reuse is the cross-package signal, so dropping it from the human-readable form was the wrong half to lose. Both counts now shown, and the document/group distinction is explicit in the labels. - command_sub_modes_emit_real_markdown asserted stderr was empty while passing --quiet, which suppresses the warning it was checking for. That is why the profiles claim survived the test I wrote to catch exactly this. The warning check now runs without --quiet; --quiet stays for the content comparison, where progress output would otherwise be noise.
This was referenced Jul 30, 2026
copyleftdev
added a commit
that referenced
this pull request
Jul 30, 2026
) * feat(streaming): feed the accumulators one record at a time `--streaming` read the whole file, parsed a full DOM, then materialised a Vec<JsonEvent> beside it — 402 MB against the DOM path's 233 MB on a 15 MB input. The sketch accumulators were already bounded; what was missing was a way to feed them. input DOM --streaming 15 MB / 120k 227 MB 7.2 MB 32x 142 MB / 1.2M 2,231 MB 7.7 MB 288x Ten times the input, the same memory: bounded, not merely smaller. vajra-core::records pulls top-level records one at a time — NDJSON through StreamDeserializer, a top-level array element by element through a SeqAccess visitor, which is the shape the docs use as the large-input example and the one that is not simply line-splitting. emit_record_events converts each into a reused buffer rooted at `$[*]`, so paths match a whole-document walk; there is a test asserting the value events are identical. Two limits are real and stated rather than implied. Memory is bounded by the largest single record, not the file. And stdin, URLs, git repositories and the non-JSON formats go through readers that produce a whole document by construction — passing --streaming to those now says the input was loaded whole rather than silently doing nothing. --- A wrong number this surfaced With a real record stream the exact-tracking threshold is actually reached, and the fallback was reporting the top-k buffer size as cardinality: 100 distinct values for a field with 120,000, and entropy log2(100) = 6.643856 against a true 16.87 — in the same fields the DOM path uses for measurements. Those paths now carry `exact: false`, omitted when true so DOM output is unchanged. `cardinality` is documented as a lower bound, which is provable: k occupied counters means at least k distinct values were seen. No bound is claimed for entropy — Space-Saving admits an evicted item at min_count + 1, so it over-attributes rather than cleanly grouping, and calling it a bound would be a claim I cannot prove. I wrote it as one first and removed it after checking the eviction rule. A distinct-count sketch is #106. Below the threshold streaming is not an approximation at all: entropy agrees with the DOM path to 1e-12, asserted. OUTPUT_SCHEMA_VERSION goes to 2 for the added field — the first use of the versioning added in #104. The "does not yet bound memory" warning from #103 is removed and the test that asserted it is replaced by its inverse, since a stale caveat is its own kind of false claim. CLAUDE.md rule 1 now says partially met, with the measurement. Closes #102 * review: root a lone object at $, show the basis in text output, scope the claims CodeRabbit on #107. Four findings, all correct; one was a correctness bug I introduced. A file holding a single JSON object was treated as a one-record stream and rooted at `$[*]`, so streaming reported `$[*].a` where the DOM path reports `$.a` — the same input answered two ways depending on a flag. My equivalence test only covered arrays, so it passed. `{"a":1}` and NDJSON share a leading byte, so they cannot be told apart from the prefix. `stream_ndjson` now holds the first value while attempting a second: one value means the file is that document, more means a stream. Two records live at the boundary, still bounded. RecordRoot carries the decision through to emit_record_events, and path_names_match_the_dom_path_for_every_top_level_shape covers all four shapes at the CLI level so this class cannot recur. The other three: - Text and Markdown showed cardinality and entropy with no indication which were sketch output, though the JSON has carried `exact` since #102. A BASIS column appears only when something is approximate — adding it always would put "exact" beside every row of ordinary output — with a note saying what approx means. - "bounded by the largest record" overclaimed. Memory also scales with distinct paths times per-path sketch state; the flat-RSS measurements share a schema. Both limits are now stated, along with what memory does *not* scale with, which is the number of records. - The global flag description implied every command streams. Only `stats` does, and only for JSON/NDJSON files. --------- Co-authored-by: copyleftdev <copyleftdev@users.noreply.github.com>
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
1 participant
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.
#75 raised two repo-wide guideline violations. Both turned out to be narrower and wider than filed.
1.
println!vs the rendering system — narrower than filedWhen #75 was written no rendering system existed.
render.rswas added since and fifteen commands migrated onto it; five were missed:cmd_fingerprint_corpuscmd_population_driftcmd_tree_diffcmd_ingest_githubcmd_profilesAt most four per command now, one per format branch, emitting an already-rendered string.
The cost the issue predicted was real and present.
fingerprintis listed inRENDERS_MARKDOWNand emits no fallback warning, but--corpus --format markdownprinted=== Corpus Shape Index ===. The claim held for the path the honesty test exercised and not for the sub-mode beside it — same fordrift --tree,drift --group-byandprofiles.command_sub_modes_emit_real_markdowncovers all four, asserting both that Markdown differs from text and that text carries no Markdown headings.2. Streaming — much wider than filed
#75 framed this as
separationandinvariantsbeing two commands that don't stream. Measuring it says otherwise.15 MB JSON, 120,000 records,
vajra stats:--streaming--streaminguses 1.7x more memory than the path it replaces, while its help text read "bounded memory, sketch-based stats".load_adaptive's large-input branch — taken precisely when memory matters — reads the whole file, parses a full DOM, materialises aVec<JsonEvent>beside it, then walks it twice more for node count and depth. The code comment admits it ("For now, still parse DOM but convert to events"); the flag did not.So
separationandinvariantsare not exceptions to a rule the rest of the tool follows. Nothing streams. Filed as #102 with the measurement and a proposed path (StreamDeserializerfor NDJSON; DDSketch cuts in pass 1 and contingency tables in pass 2 for the two-pass analyses).What ships here is the honest part:
--quiet— the sketch accumulators are real and tested, what is missing is a parser to feed themdocs/src/streaming.mdcarried a full page of guarantees ending in "bounded memory independent of input size". It now opens with the measurement and marks the design as intended rather than implemented.CLAUDE.md
Both constraints described systems that did not exist when they were written.
println!constraint referenced "the rendering system" before there was one. Reworded to describe theReportpattern the CLI now uses, and why building output line by line is what let--format markdowndrift.separation's docs as the example of stating what memory scales with.I have not weakened the target. I have made it possible to tell whether it is being met.
Also
population_drift'stext_output_worksasserted"Group-by: repo"with a single space; the renderer pads field labels to a common width. The assertion squeezes whitespace rather than pinning one alignment — the substance is unchanged and still checked.Gate
cargo test --workspaceclean,cargo clippy --workspace --all-features -- -D warningsclean,cargo fmt --checkclean.Found by dogfooding vajra against a trending repo.
Refs #75, refs #102
Summary by CodeRabbit
New Features
--quietsuppression.Documentation
Bug Fixes
Tests