feat(phase4): terminal formatter — ranked review path for wild diff - #19
feat(phase4): terminal formatter — ranked review path for wild diff#19avikalpg wants to merge 8 commits into
Conversation
TerminalFormatter renders a schema v2 DiffGraph dict as a priority-ranked terminal output. This is the main user-facing change in DiffGraph v2: 'wild diff' stops opening a browser and instead prints a ranked review path to the terminal. Key features: - Three-bucket ranking: REVIEW FIRST (imported by changed files), REVIEW NEXT (isolated changes), CONTEXT (unchanged symbols in touched files) - Score = importer_count * 3 + lines_changed; evidence-based, LLM-free - Symbol truncation cap (default 10) with --all override - ANSI color with NO_COLOR and TTY auto-detection - --compact flag omits CONTEXT section - File-level fallback when symbol extraction unavailable - Footer shows analysis tier, languages, duration, diff context - 14 unit tests; all pass; pure functions, no git/network Wires as 'wild diff --format terminal'. Default swap (browser → terminal) pending B1 answer from Avikalp. Can ship as --format terminal opt-in today. Spec: docs/DiffGraph-CLI/design/TERMINAL-FORMATTER.md
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a schema v2 terminal formatter that validates DiffGraph data, ranks symbols by review priority, renders colored or plain terminal output, supports compact and truncation options, and integrates terminal output with the CLI. ChangesTerminal formatter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant StructuralAnalysis
participant TerminalFormatter
participant TerminalOutput
CLI->>StructuralAnalysis: request analysis for terminal format
StructuralAnalysis->>TerminalFormatter: provide DiffGraph artifact
TerminalFormatter->>TerminalOutput: render terminal review output
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.
🧹 Nitpick comments (3)
tests/test_terminal_formatter.py (2)
369-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer unpacking over list concatenation (Ruff RUF005).
🔧 Proposed fix
- files = importer_files + [target_file] + files = [*importer_files, target_file]🤖 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 `@tests/test_terminal_formatter.py` around lines 369 - 374, Update the files construction near importer_files and target_file to use iterable unpacking rather than list concatenation, preserving the existing order and resulting list contents.Source: Linters/SAST tools
71-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplicit
Optionalon_make_diffgraphparams (Ruff RUF013).
files,symbols,relationships,metadata, anddiff_refare typed as barelist/dictwithNonedefaults, which PEP 484 prohibits implicitly.🔧 Proposed fix
+from typing import Optional + def _make_diffgraph( - files: list = None, - symbols: list = None, - relationships: list = None, - metadata: dict = None, - diff_ref: dict = None, + files: Optional[list] = None, + symbols: Optional[list] = None, + relationships: Optional[list] = None, + metadata: Optional[dict] = None, + diff_ref: Optional[dict] = None, ) -> dict:🤖 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 `@tests/test_terminal_formatter.py` around lines 71 - 77, Update the `_make_diffgraph` parameters `files`, `symbols`, `relationships`, `metadata`, and `diff_ref` to explicitly use nullable `Optional` type annotations while retaining their existing `None` defaults and return type.Source: Linters/SAST tools
diffgraph/formatters/terminal.py (1)
307-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
section_styleparameter is accepted but never used.
_write_sectionhardcodes header styling viaif title == "REVIEW FIRST"/elif title == "CONTEXT"branches, ignoring thesection_styleargument passed by every caller (Line 155-158). This currently renders correctly only because the branches happen to match the caller-supplied styles — any future call with a differentsection_stylewould be silently ignored.♻️ Use the passed style instead of re-deriving it from title
+_STYLE_FNS = {"bold_yellow": bold_yellow, "bold": bold, "dim": dim} + def _write_section( self, title: str, items: list[_RankedSymbol], out, color: bool, section_style: str = "bold", ) -> None: ... prefix = "▶ " if not _is_dumb_terminal() else "> " header_text = f"{prefix}{title}" - if title == "REVIEW FIRST": - header = bold_yellow(header_text, color) - elif title == "CONTEXT": - header = dim(header_text, color) - else: - header = bold(header_text, color) + header = _STYLE_FNS.get(section_style, bold)(header_text, color)🤖 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 `@diffgraph/formatters/terminal.py` around lines 307 - 337, Update _write_section to derive the header styling from its section_style parameter instead of matching title values. Preserve the existing bold-yellow, dim, and bold behavior by mapping the caller-provided styles accordingly, and ensure all callers’ current rendering remains unchanged.
🤖 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.
Nitpick comments:
In `@diffgraph/formatters/terminal.py`:
- Around line 307-337: Update _write_section to derive the header styling from
its section_style parameter instead of matching title values. Preserve the
existing bold-yellow, dim, and bold behavior by mapping the caller-provided
styles accordingly, and ensure all callers’ current rendering remains unchanged.
In `@tests/test_terminal_formatter.py`:
- Around line 369-374: Update the files construction near importer_files and
target_file to use iterable unpacking rather than list concatenation, preserving
the existing order and resulting list contents.
- Around line 71-77: Update the `_make_diffgraph` parameters `files`, `symbols`,
`relationships`, `metadata`, and `diff_ref` to explicitly use nullable
`Optional` type annotations while retaining their existing `None` defaults and
return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c5dc8d-8d25-4b84-ab00-8df54579687c
📒 Files selected for processing (3)
diffgraph/formatters/__init__.pydiffgraph/formatters/terminal.pytests/test_terminal_formatter.py
nia-sg-bot
added
priority:P1
Product-direction review — 2026-08-01Verdict: REVISE · P1 The terminal formatter is a good schema consumer and should survive. Before merge: wire it into the CLI, use actual changed-line/hunk evidence rather than symbol span, reconcile relationship endpoint semantics with the producer, report mixed provenance honestly, reject unsupported schema majors, and test against shared golden fixtures after the canonical producer stabilizes. This is a scope/alignment review, not an automatic closure decision. Items marked DISCUSS CLOSE should be resolved with maintainer context before closing. |
nia-sg-bot
left a comment
nia-sg-bot
left a comment
There was a problem hiding this comment.
Direction gate on current head 32c8150d856ee35cba9b74106b7589536fda85b2: the formatter itself is healthy (python3 -m pytest -q: 45 passed; git diff --check: clean), but this PR is not yet merge-ready.
Please address these contained contract/integration gaps before merge:
- Wire
TerminalFormatterinto a real CLI output path; the current CLI contains no formatter reference, so users cannot exercise it. - Rank modified symbols from actual diff/hunk changed-line evidence.
_rank_symbols()currently computeslines_changedas the full symbol span (line_end - line_start + 1), which overstates tiny edits in large symbols. - Validate
schema_versionand reject unsupported major versions before rendering; tests currently only construct"2.0", while the formatter accepts arbitrary/missing versions. - Confirm
importsrelationship endpoint semantics against the canonical producer and cover them with a shared/golden schema-v2 fixture rather than formatter-only synthetic fixtures.
This preserves the useful formatter while keeping the CLI artifact canonical, versioned, deterministic, and honestly ranked.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
diffgraph/formatters/terminal.py (3)
337-340: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not cap
CONTEXTunder the current option contract.
max_itemsis documented forREVIEW FIRSTandREVIEW NEXT, but this code truncatesCONTEXTtoo. Apply the cap only to review sections, or update the public option contract and tests.🤖 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 `@diffgraph/formatters/terminal.py` around lines 337 - 340, Update the item-capping logic near total, capped, and hidden so max_items is applied only to REVIEW FIRST and REVIEW NEXT sections, not CONTEXT. Preserve the existing uncapped CONTEXT output and hidden-count behavior, unless you explicitly revise the public option contract and corresponding tests.
233-245: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse changed-line or hunk data for ranking.
Line 237 uses the complete symbol span, not the changed lines. A one-line edit in a large symbol can rank above a larger real change. Added and deleted symbols also have
lines_changed == 0during scoring because their line count is calculated only while rendering.Populate line counts from canonical changed-line or hunk evidence before Line 239. Apply the same value to ranking and display for every change kind.
Also applies to: 384-388
🤖 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 `@diffgraph/formatters/terminal.py` around lines 233 - 245, Update the ranking preparation before score calculation to derive lines_changed from canonical changed-line or hunk data rather than the complete symbol span, including added and deleted symbols. Reuse this single computed value for both _RankedSymbol.score and the later display path so every change kind reports and ranks by actual changed lines.
165-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWire
--format terminalinto the CLI dispatch.
TerminalFormatterexists and has unit coverage, butdiffgraph/cli.pydoes not define or select it forwild diff --format terminal, and there is no integration test for the CLI output routes. Add the parser option dispatch todiffgraph.formatters.terminal.TerminalFormatter, keepdiffgraph.html+browser as the default, and add CLI integration tests for both paths.🤖 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 `@diffgraph/formatters/terminal.py` around lines 165 - 178, Update the CLI parser and format dispatch in diffgraph/cli.py so --format terminal constructs and uses diffgraph.formatters.terminal.TerminalFormatter, while preserving diffgraph.html plus browser behavior as the default route. Add CLI integration coverage for both explicit terminal output and the existing default HTML/browser path.tests/test_terminal_formatter.py (2)
1-90: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a production-backed fixture for relationship provenance tests.
The importer ranking still builds
_make_import_relfixtures instead of using golden DiffGraph v2 fixtures. Add producer-backed cases, including mixedanalysis_sourcevalues, and assert the resulting warning/footer output.🤖 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 `@tests/test_terminal_formatter.py` around lines 1 - 90, Replace synthetic _make_import_rel-based importer ranking cases with production-backed golden DiffGraph v2 fixtures. Include cases containing mixed relationship analysis_source values, then assert the resulting ranking warnings and footer output from TerminalFormatter. Keep the existing fixture helpers for unrelated formatter tests.
316-345: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAssert changed-line evidence, not symbol location spans.
_make_symbol()storeslocationonly, and_rank_symbols()scoreslines_changedfromlocation.line_end - location.line_start + 1. This test locks in a span-derived ranking contract. Store hunk/diff evidence with the symbol span, include a case where span lines differ from changed-line evidence, and assert that actual changed-line evidence controlsREVIEW FIRSTordering.🤖 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 `@tests/test_terminal_formatter.py` around lines 316 - 345, Update test_rank_symbols_score_ordering and its symbol fixtures to provide hunk/diff changed-line evidence separately from each symbol’s location span. Include symbols whose span lengths differ from their changed-line counts, then assert REVIEW FIRST ordering follows the actual changed-line evidence rather than location.line_start/line_end calculations.
🤖 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 `@tests/test_terminal_formatter.py`:
- Around line 98-99: Update the pytest.raises assertion for TerminalFormatter to
use a regex pattern that escapes the literal dots in “MAJOR.MINOR”, ensuring
only the exact error-text format matches.
---
Outside diff comments:
In `@diffgraph/formatters/terminal.py`:
- Around line 337-340: Update the item-capping logic near total, capped, and
hidden so max_items is applied only to REVIEW FIRST and REVIEW NEXT sections,
not CONTEXT. Preserve the existing uncapped CONTEXT output and hidden-count
behavior, unless you explicitly revise the public option contract and
corresponding tests.
- Around line 233-245: Update the ranking preparation before score calculation
to derive lines_changed from canonical changed-line or hunk data rather than the
complete symbol span, including added and deleted symbols. Reuse this single
computed value for both _RankedSymbol.score and the later display path so every
change kind reports and ranks by actual changed lines.
- Around line 165-178: Update the CLI parser and format dispatch in
diffgraph/cli.py so --format terminal constructs and uses
diffgraph.formatters.terminal.TerminalFormatter, while preserving diffgraph.html
plus browser behavior as the default route. Add CLI integration coverage for
both explicit terminal output and the existing default HTML/browser path.
In `@tests/test_terminal_formatter.py`:
- Around line 1-90: Replace synthetic _make_import_rel-based importer ranking
cases with production-backed golden DiffGraph v2 fixtures. Include cases
containing mixed relationship analysis_source values, then assert the resulting
ranking warnings and footer output from TerminalFormatter. Keep the existing
fixture helpers for unrelated formatter tests.
- Around line 316-345: Update test_rank_symbols_score_ordering and its symbol
fixtures to provide hunk/diff changed-line evidence separately from each
symbol’s location span. Include symbols whose span lengths differ from their
changed-line counts, then assert REVIEW FIRST ordering follows the actual
changed-line evidence rather than location.line_start/line_end calculations.
🪄 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 Plus
Run ID: b6f98847-327c-4707-9158-6391ce76f47e
📒 Files selected for processing (2)
diffgraph/formatters/terminal.pytests/test_terminal_formatter.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@diffgraph/cli.py`:
- Around line 212-219: Add `--compact` and `--all` click options alongside the
existing `output_format` option, then update the terminal-rendering path and its
`_structural_scope` invocation to pass the compact value and use
`max_items=None` when all items are requested. Add CLI coverage verifying both
options are accepted and affect terminal rendering.
🪄 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 Plus
Run ID: 1f03149e-0220-410b-a73f-62e5e31bfe80
📒 Files selected for processing (2)
diffgraph/cli.pytests/test_structural.py
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 `@diffgraph/cli.py`:
- Around line 220-221: Update the Click option handling for --compact and --all
so they are parsed only for terminal diff invocations, or rename them to avoid
consuming Git pass-through flags. Ensure main’s Git pass-through path forwards
these arguments unchanged, including commands such as wild branch --all, while
retaining the terminal diff behavior.
In `@tests/test_structural.py`:
- Around line 235-236: Update the symbol names generated in the structural
test’s before and after fixtures so lexical sorting matches numeric order, or
add a sentinel symbol that sorts beyond the default cap; then assert that this
sentinel is included when exercising the --all behavior, ensuring the test
verifies selection beyond the default limit.
🪄 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 Plus
Run ID: 162f0b21-73a2-429a-acce-a3190868e174
📒 Files selected for processing (2)
diffgraph/cli.pytests/test_structural.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
nia-sg-bot
left a comment
nia-sg-bot
left a comment
There was a problem hiding this comment.
Direction gate cleared on current head 85272827ac4ff031f63ebec3fccfb899d15114e8.
Evidence: python3 -m pytest (57 passed), focused formatter/structural suite (47 passed), compileall, and git diff --check origin/main...HEAD all pass. Unstaged and staged wild diff --format terminal smoke runs both produced deterministic local structural output. Canonical v2 schema production/validation remains covered by the structural suite; the topology fixture is intentionally a partial ranking fixture, not a standalone v2 artifact. No unresolved review threads; CodeRabbit current-head check passes and its incremental re-review reports no unreviewed changes. No extension contract or website released-product claim is changed by this PR.
2 participants
Phase 4 — Terminal Formatter
Status: 14 tests passing · Standalone — no dependency on Phases 1–3
Spec:
docs/DiffGraph-CLI/design/TERMINAL-FORMATTER.mdRoadmap:
docs/DiffGraph-CLI/design/V2-IMPLEMENTATION-ROADMAP.mdPhase 4What this does
Adds
TerminalFormatterindiffgraph/formatters/terminal.py.When wired as
wild diff --format terminal, instead of openingdiffgraph.htmlin a browser, the tool prints a priority-ranked review path to the terminal:Files changed
diffgraph/formatters/__init__.pydiffgraph/formatters/terminal.pyTerminalFormatterclass (430 lines)tests/test_terminal_formatter.pyHow the ranking works
Three-bucket ranking, fully evidence-based (no LLM required):
Score formula:
importers × 3 + lines_changed— importers weighted heavier than lines.Acceptance criteria (all met)
diffgraph/formatters/terminal.pycreated withTerminalFormatter_rank_symbols()is a pure function (14 tests pass; no stdout capture needed)--compactomits CONTEXT section--alldisables 10-symbol truncation capNO_COLOR=1and piped stdout suppress ANSIB1 decision point
The formatter ships as
wild diff --format terminal(opt-in). Whether this becomes the default output depends on B1:cli.pydefault fromhtmltoterminal(1-line change)This PR is mergeable regardless of B1. The B1 change is a trivial 1-line follow-up.
Phase context
This is Phase 4 of the V2 implementation roadmap. It is standalone — it does not depend on Phases 1, 2, or 3 merging first. The formatter is a pure consumer of any schema v2 dict and can be reviewed and merged independently.
The critical unblocked path:
Summary by CodeRabbit
New Features
--formatoption.Bug Fixes