feat(cli): migrate fingerprint and separation onto the renderer - #82
Conversation
Third batch. Markdown coverage is now 6 of 12 commands. `fingerprint` exercises the Fields block (hash summary) plus a table (repeated motifs) plus a conditional note, and both its streaming and non-streaming paths are migrated. The streaming path gains an explicit note that the Merkle shape hash needs the whole tree in memory, which was previously implied by a bare "(not available in streaming mode)". `separation` was the most useful test of note fidelity, since its caveats are the point of the command: rank by MI not strength, SEP only for ordered fields, and precision at prevalence being far below corpus precision. All three survive both formats as blockquotes. Two things the migration deleted rather than moved: - `truncate_path`, a manual fixed-width JSONPath trimmer `separation` needed because it was formatting its own columns. The renderer computes widths, so the helper became dead code and is gone. - The per-command format match arms, which is the point. One regression caught by an existing test and fixed rather than papered over: the Fields block rendered `Nodes 25` where the hand-written output had `Nodes: 25`. `fingerprint_min_nodes.rs` asserted on `"Nodes:"` and failed. Rather than relax the test, Fields now emits the trailing colon, so text output stays byte-compatible with what anything grepping it expects. That is also why the tests assert on text shape and not only on JSON. Test bookkeeping: the tripwire moves from `fingerprint` to `cluster`, and the migrated-command loop now covers four commands. Refs #75. Remaining: cluster, drift, batch, cascade, score, governance, core-team, compare.
|
Warning Review limit reached
Next review available in: 51 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 (1)
📝 WalkthroughWalkthroughFingerprint and separation non-JSON output now uses structured reports and tables with Markdown support. Renderer coverage documentation, format-honesty tests, and text field-label formatting were updated accordingly. ChangesMarkdown renderer migration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 2169-2183: The report-generation paths retain unbounded rows and
rendered strings; replace the repeated-motif flow around report.table and the
output flow around class, feature, and rule reporting with bounded or streaming
sinks. For aligned text, use a two-pass or spooled approach so output is emitted
incrementally without retaining the complete report; update both
vajra-cli/src/main.rs:2169-2183 and vajra-cli/src/main.rs:2849-2934, preserving
Markdown and text formatting.
In `@vajra-cli/tests/format_honesty.rs`:
- Line 180: Extend the Markdown-honesty coverage loop in the relevant test to
exercise the separation command alongside the existing commands. Provide
separation’s labelled fixture and required CLI arguments, or add an equivalent
dedicated integration test, ensuring the renderer-backed separation output is
validated.
🪄 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: c6756b60-375f-4fd1-87b5-c8067fb255f8
📒 Files selected for processing (4)
docs/src/commands.mdvajra-cli/src/main.rsvajra-cli/src/render.rsvajra-cli/tests/format_honesty.rs
| let mut t = | ||
| render::Table::new(&["HASH", "COUNT"], "no repeated subtree shapes found"); | ||
| for m in &output.repeated_motifs { | ||
| t.push(vec![m.hash.clone(), m.count.to_string()]); | ||
| } | ||
| report.table(t); | ||
| } | ||
|
|
||
| print!( | ||
| "{}", | ||
| match cli.format { | ||
| Format::Markdown => report.to_markdown(), | ||
| _ => report.to_text(), | ||
| } | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid buffering unbounded rendered reports.
Both paths retain all rows in render::Table and then allocate the complete rendered output string. Fingerprint has no motif bound, and separation --top-k 0 emits every feature.
vajra-cli/src/main.rs#L2169-L2183: render repeated motifs through a bounded/streaming sink.vajra-cli/src/main.rs#L2849-L2934: stream class, feature, and rule output; use a two-pass/spooled strategy if aligned text requires widths.
As per coding guidelines, “Every algorithm must support arbitrary input scale through streaming and bounded-memory designs; non-streaming algorithms do not ship.”
📍 Affects 1 file
vajra-cli/src/main.rs#L2169-L2183(this comment)vajra-cli/src/main.rs#L2849-L2934
🤖 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 `@vajra-cli/src/main.rs` around lines 2169 - 2183, The report-generation paths
retain unbounded rows and rendered strings; replace the repeated-motif flow
around report.table and the output flow around class, feature, and rule
reporting with bounded or streaming sinks. For aligned text, use a two-pass or
spooled approach so output is emitted incrementally without retaining the
complete report; update both vajra-cli/src/main.rs:2169-2183 and
vajra-cli/src/main.rs:2849-2934, preserving Markdown and text formatting.
Source: Coding guidelines
Added separation to RENDERS_MARKDOWN in the previous commit without adding it to the coverage assertions — exactly the rot that loop exists to prevent. It needs a labelled fixture and its own flags so it cannot join the simple loop, hence a dedicated test, which also asserts the caveat notes survive as blockquotes since those are the point of the command. Raised by CodeRabbit on #82.
|
Both findings valid; one fixed, one folded into the tracked streaming work.
Unbounded buffering — real, but it is the render-side face of #75's streaming half, not a regression here. Worth being precise about what changed:
So this migration neither introduced nor worsened it — but you're right that it's now concentrated in one place, which is actually the useful part: a streaming renderer is one change here rather than twelve at the call sites. That belongs with the streaming work already tracked on #75, because the same decision governs it: I'd rather leave it as a documented characteristic of the current design than half-stream it. Noted on #75 so it isn't lost. |
1 participant
Refs #75. Third batch — Markdown coverage now 6 of 12 commands.
What these two added
fingerprintexercisesFields(hash summary) +Table(repeated motifs) + a conditionalNote, and both its streaming and non-streaming paths are migrated. The streaming path gains an explicit note that the Merkle shape hash needs the whole tree in memory — previously that was only implied by a bare(not available in streaming mode).separationwas the best test of note fidelity, because its caveats are the product: rank by MI not strength, SEP only for ordered fields, and precision-at-prevalence being far below corpus precision. All three survive as blockquotes:The migration deleted code, not just moved it
truncate_path— a manual fixed-width JSONPath trimmerseparationneeded because it formatted its own columns — became dead once the renderer computed widths. Removed rather than left behind.A regression an existing test caught
The
Fieldsblock renderedNodes 25where the hand-written output hadNodes: 25.fingerprint_min_nodes.rsasserted on"Nodes:"and failed.I fixed the renderer rather than relaxing the test:
Fieldsnow emits the trailing colon, so text output stays byte-compatible with anything grepping it. That's also the argument for those tests asserting on text shape and not only on JSON — a formatting change is a user-visible change.Bookkeeping
Tripwire moves
fingerprint→cluster. The migrated-command loop now covers four commands, soRENDERS_MARKDOWNstill can't claim more than is true.Full workspace suite: 79 test binaries, 0 failures.
cargo fmt --checkclean. 0 clippy diagnostics.Remaining
cluster,drift,batch,cascade,score,governance,core-team,compare.Summary by CodeRabbit
New Features
fingerprintandseparationcommands.Documentation
fingerprintandseparationas supporting real Markdown output.Style