Peel: three-layer progressive-disclosure output system#135
Merged
Conversation
…mparator) Adds freshdata/render/view.py: PeelView/Metric/AttentionItem/Section frozen dataclasses, the shared status and severity vocabularies, the attention comparator (privacy > corruption > policy > reliability > cosmetic, then severity, count, name, stable id), and the confidence ladder. Pure stdlib, nothing imports it yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
normalize.py turns a CleanReport into a PeelView: status derivation (CLEAN/CHANGED + REVIEW + PARTIAL), ranked attention items with stable per-source ids (W/R/S/D/C/F), glance metrics, a single recommended next step, and lazy inspect sections (per-column aggregation, full action log, audit fields — undo_log excluded as ever). register_normalizer() gives third-party report kinds the same path. _vocabulary.py centralizes the display-language mapping; audit output keeps exact terms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
options.py: RenderOptions + set_display/get_display/reset_display with the documented precedence (explicit args > set_display > FRESHDATA_DISPLAY / NO_COLOR / FRESHDATA_NO_PREVIEWS > defaults); auto mode resolves by TTY. plain.py: the reference renderer — compact/standard/verbose/debug/json/ plain/silent modes over PeelView, zero ANSI, grep-able status labels, explicit empty-attention state, ASCII transliteration in plain mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HtmlReprMixin.show gains additive mode/renderer parameters: no-arg calls keep the exact legacy behavior (inline HTML in Jupyter, temp .html file elsewhere); mode= or renderer="terminal" prints the Peel text rendering. The Peel path never raises — unknown kinds or bad modes fall back to summary()/repr. fd.set_display/get_display/reset_display exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
render/terminal.py renders PeelView as a rich panel (severity-styled attention table, metric chips, verbose/debug section tables); rich is a new optional extra and everything falls back to the plain reference renderer when it is missing or for text-first modes. show(renderer= "terminal") routes through it; content parity with plain is tested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
render/notebook.py builds the Peel card from PeelView over the existing
escaped html.py primitives: status chips, metric strip, ranked attention
list, next-step code, native <details> inspect sections (capped at 50 rows
with an honest 'showing N of M' line), and the JSON export control. A new
RenderOptions.style plus fd.set_display("peel") / FRESHDATA_DISPLAY=peel
opt in; legacy layout stays default and FRESHDATA_LEGACY_DISPLAY=1 forces
it. Kinds without a normalizer fall back to legacy. All user values escaped.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
render/_semantic.py reads the semantic Action.metadata payload (raw/proposed
values, signed evidence, backend, calibration, memory provenance) and turns
it into: a confidence label pairing number with the plain-language ladder
(~ marks uncalibrated), review/ambiguous attention text ('x' -> 'y' or
'no change made'), and a 'Semantic proposals' section ordered ambiguous ->
review -> applied with a signed evidence digest and a coverage note naming
which sources ran / were skipped. Ambiguous proposals now reach the attention
list so declining to act is visible. Full raw evidence stays in the audit
section.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ParseResult now inherits HtmlReprMixin (_render_kind='parse') and gains summary()/__str__. A new parse normalizer builds the view reading only frame row counts (len), never contents: PARTIAL when any warning or empty frame is present, a parsed->validated->cleaned->reference stage ladder that only reaches 'parsed', advisory suggested_domain, per-frame status (ready / N warnings / unsupported items), frame warnings folded into their frame's item, and a 'read, not checked' banner. to_html routes kinds with only a Peel normalizer (no legacy layout) straight to Peel, so parse always renders and FRESHDATA_LEGACY_DISPLAY can't break it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CopilotReport inherits HtmlReprMixin (_render_kind='copilot') and gains an .attention property; __str__ unchanged (still returns the summary string). The copilot normalizer builds a ranked attention queue via the shared comparator (privacy > corruption > policy > reliability > cosmetic) with A-prefixed ids assigned after ranking, so a grade-A trust score can never bury a PII or policy finding. Privacy state is always shown (found+masked or 'no PII detected'); the trust metric carries a 'data quality only' scope note; the experimental banner is permanent and provider failures render as a labeled PARTIAL state, not an absence. Plan, machine-generated code, masked model context, and audit live in inspect/audit sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
freshdata clean gains -v/--verbose (verbose, -vv debug), --output-format
{text,json} (json prints the report to stdout), --no-color, and --display
{legacy,peel}. Default output is byte-for-byte unchanged (legacy summary);
the new flags opt into Peel via a shared _emit_report helper that renders
through the terminal renderer and falls back to legacy text on any error.
--quiet and --report keep their existing behavior and coexist with the new
flags.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
docs/peel.md explains the three layers, status/severity language, display modes, notebook opt-in, CLI flags, and the unchanged machine-readable surface; registered in mkdocs nav after Interactive output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rich isn't a direct dev dependency; it currently only reaches CI as a transitive dependency of twine. Two tests asserted real ANSI output, so they'd silently break if that transitive chain ever changed.
Contributor
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
📝 WalkthroughWalkthroughAdds the opt-in Peel output system with normalized report views, plain/terminal/notebook renderers, display configuration, CLI flags, Copilot and parser integration, documentation, and tests covering rendering, ranking, privacy, compatibility, and fallbacks. ChangesPeel output system
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Report
participant Normalizer
participant Renderer
participant CLI_or_Notebook
Report->>Normalizer: normalize report by render kind
Normalizer->>Renderer: provide PeelView
Renderer->>CLI_or_Notebook: return text or HTML output
CLI_or_Notebook-->>Report: display rendered report
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|---|---|---|---|---|---|---|---|---|---|---|
| crm | 10,200 | 40 | 1.467 | 1.478 | 13.8 | 100.0 | 0.0 | 100.0 | 93.84 | ✅ | 100.0 |
| event_log | 10,000 | 25 | 0.594 | 0.595 | 5.1 | 100.0 | 0.0 | 100.0 | 99.677 | ✅ | 100.0 |
| finance | 10,200 | 60 | 1.896 | 1.897 | 18.3 | 100.0 | 0.0 | 100.0 | 99.499 | ✅ | 100.0 |
| gold | 10,200 | 7 | 0.220 | 0.222 | 3.7 | 100.0 | 0.0 | 100.0 | 98.3 | ✅ | 100.0 |
| provenance | 10,000 | 18 | 0.483 | 0.486 | 7.2 | 100.0 | 0.0 | 100.0 | 99.765 | ✅ | 100.0 |
| wide_schema | 10,000 | 100 | 3.574 | 3.575 | 13.7 | 100.0 | 0.0 | 100.0 | 96.042 | ✅ | 100.0 |
Authored-code reduction (Metric 6)
- FreshData: 3 lines
- pandas baseline: 25 lines (88.0% reduction)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
src/freshdata/render/normalize.py (1)
311-311: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_audit_rows(rep)is computed twice.
count=len(_audit_rows(rep))forces an eager pass while theSectionlambda recomputes it lazily. Since it iterates 16 attributes it's cheap, but you can compute once and reuse.🤖 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 `@src/freshdata/render/normalize.py` at line 311, Update the surrounding section construction to compute _audit_rows(rep) once, store the result in a local variable, and reuse it for both the Section content lambda and its count, preserving the existing audit behavior.tests/test_peel_normalize.py (1)
133-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case where a non-matching finding precedes the failed one.
test_contract_failures_become_policy_itemsplaces thefailedfinding first, so it never exercises ID assignment when apassed/skipped finding comes before a violated one. Adding such a case would catch the non-contiguous-ID behavior flagged innormalize.py.🤖 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_peel_normalize.py` around lines 133 - 151, The test_contract_failures_become_policy_items case should place a non-matching passed or skipped finding before the failed contract finding, then assert the resulting policy item receives the correct non-contiguous ID while preserving its existing severity, domain, and text assertions.src/freshdata/render/notebook.py (1)
138-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
optionsparameter is assigned but never used.
options = options or get_display()resolves the display options, but the variable is never referenced afterward — the notebook renderer always produces full HTML regardless of mode, color, or width settings. If this is intentional for API consistency withrender_plain/render_terminal_text, consider adding a brief comment noting that notebook output is mode-independent. Otherwise, remove the parameter to avoid misleading callers.🤖 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 `@src/freshdata/render/notebook.py` around lines 138 - 140, The render_notebook function resolves options but never uses them, making the parameter misleading. Either apply the resolved RenderOptions to notebook rendering so mode, color, and width affect output, or remove the options parameter and its get_display assignment if notebook output is intentionally mode-independent; preserve API consistency with the related render functions.src/freshdata/render/mixins.py (1)
41-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider narrowing
except KeyErrorscope to the normalize call only.The
except KeyErrorcatchesKeyErrorfrom bothnormalize.normalize(self)andnotebook.render_notebook(...). If the notebook renderer raisesKeyErrorfor an unrelated reason (e.g., a missing dict key in its own logic), it would be silently swallowed and fall back to legacy, masking a renderer bug. Moving the normalize call outside the try or using a nested try would isolate the expected KeyError:♻️ Suggested refactor
if want_peel: - try: - from . import normalize, notebook - - return notebook.render_notebook(normalize.normalize(self)) - except KeyError: - if not has_legacy: - raise # nothing else can render this kind + from . import normalize, notebook + try: + view = normalize.normalize(self) + except KeyError: + if not has_legacy: + raise # nothing else can render this kind + else: + return notebook.render_notebook(view) return renderers.render(self, kind)🤖 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 `@src/freshdata/render/mixins.py` around lines 41 - 47, In the render path around normalize.normalize and notebook.render_notebook, narrow the KeyError handler to only the normalization call. Keep notebook.render_notebook outside that handler so renderer-raised KeyError exceptions propagate, while preserving the existing legacy fallback for normalization failures when has_legacy is true.
🤖 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 `@src/freshdata/enterprise/cli.py`:
- Around line 69-71: Update cmd_clean() and _cmd_clean_engine() so all fallback,
profile, and backend-difference diagnostics are sent to stderr when
--output-format json is selected, keeping stdout exclusively the serialized
report from the json branch. Preserve existing stdout behavior for non-JSON
formats.
- Around line 65-84: Update the report output flow around the `display` and
legacy fast path: default `--display` to unset, resolve effective display
options via `get_display()` before deciding the legacy return, and honor
`FRESHDATA_DISPLAY` or configured Peel mode. Only print `legacy_text` when
neither CLI nor environment/configuration selects Peel; preserve JSON output and
the existing terminal rendering paths.
In `@src/freshdata/render/_semantic.py`:
- Around line 60-71: Refactor the semantic rendering flow around attention_text
and the section-row rendering at the referenced range so the structured change
text is constructed once and reused, rather than deriving it by splitting
rendered strings on " — ". Preserve the existing ambiguous, raw/proposed, and
description fallback behavior while keeping confidence labeling separate from
the change text.
In `@src/freshdata/render/normalize.py`:
- Around line 151-185: Keep attention IDs contiguous by basing the counters in
the domain_findings and contract_violations loops on emitted items rather than
enumerate positions, incrementing only when an AttentionItem is appended.
Preserve the existing filtering and severity behavior, and add a regression test
covering a skipped first finding that verifies the first emitted IDs are D1 or
C1.
In `@src/freshdata/render/options.py`:
- Around line 42-75: Update the display-options flow in set_display and
get_display so environment values are applied before explicitly configured
process fields, ensuring set_display(color="always") and
set_display(previews=True) take precedence over NO_COLOR and
FRESHDATA_NO_PREVIEWS. Track only fields explicitly provided through set_display
rather than storing a fully materialized RenderOptions object, while preserving
get_display overrides as the highest-precedence layer.
In `@src/freshdata/render/plain.py`:
- Around line 56-60: Update the plain attention rendering loop to preserve each
full item.subject instead of truncating it with the 12-character subject_width
limit. Remove or bypass the width-based slicing while retaining appropriate
alignment and the existing label, text, ID, and output formatting behavior.
In `@src/freshdata/render/terminal.py`:
- Around line 47-53: Update the color selection in the terminal rendering setup
around Console so the “auto” mode enables color only when the output stream is a
TTY, while preserving explicit “never” behavior. Ensure export_text(styles=True)
does not replay ANSI for non-TTY output, and add a regression test covering the
non-TTY auto-color case.
In `@src/freshdata/render/view.py`:
- Around line 121-124: Update __post_init__ to validate that self.status is a
duplicate-free, in-order subset of STATUSES, rather than only rejecting unknown
labels. Reject any repeated or out-of-order known statuses so renderers always
receive the documented ordered subset.
---
Nitpick comments:
In `@src/freshdata/render/mixins.py`:
- Around line 41-47: In the render path around normalize.normalize and
notebook.render_notebook, narrow the KeyError handler to only the normalization
call. Keep notebook.render_notebook outside that handler so renderer-raised
KeyError exceptions propagate, while preserving the existing legacy fallback for
normalization failures when has_legacy is true.
In `@src/freshdata/render/normalize.py`:
- Line 311: Update the surrounding section construction to compute
_audit_rows(rep) once, store the result in a local variable, and reuse it for
both the Section content lambda and its count, preserving the existing audit
behavior.
In `@src/freshdata/render/notebook.py`:
- Around line 138-140: The render_notebook function resolves options but never
uses them, making the parameter misleading. Either apply the resolved
RenderOptions to notebook rendering so mode, color, and width affect output, or
remove the options parameter and its get_display assignment if notebook output
is intentionally mode-independent; preserve API consistency with the related
render functions.
In `@tests/test_peel_normalize.py`:
- Around line 133-151: The test_contract_failures_become_policy_items case
should place a non-matching passed or skipped finding before the failed contract
finding, then assert the resulting policy item receives the correct
non-contiguous ID while preserving its existing severity, domain, and text
assertions.
🪄 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: 223e8e52-cb86-4477-a79a-9e7bc9330e8c
📒 Files selected for processing (26)
docs/peel.mdmkdocs.ymlpyproject.tomlsrc/freshdata/__init__.pysrc/freshdata/enterprise/cli.pysrc/freshdata/experimental/ai_copilot.pysrc/freshdata/parsers/base.pysrc/freshdata/render/_semantic.pysrc/freshdata/render/_vocabulary.pysrc/freshdata/render/mixins.pysrc/freshdata/render/normalize.pysrc/freshdata/render/notebook.pysrc/freshdata/render/options.pysrc/freshdata/render/plain.pysrc/freshdata/render/terminal.pysrc/freshdata/render/view.pytests/test_peel_cli.pytests/test_peel_copilot.pytests/test_peel_normalize.pytests/test_peel_notebook.pytests/test_peel_parse.pytests/test_peel_plain.pytests/test_peel_semantic.pytests/test_peel_show.pytests/test_peel_terminal.pytests/test_peel_view.py
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.
Summary
Implements the Peel output system end-to-end, in 11 dependency-ordered layers (L1–L11), plus a sync merge with
mainand one test-robustness fix:PeelView, statuses, attention comparator)CleanReportnormalizer + plain-language vocabularyshow(mode=, renderer=)andfd.set_display(additive; no-argshow()unchanged)rich(newrichextra, graceful fallback)fd.set_display("peel")/FRESHDATA_DISPLAY=peel)ParseResultdisplay (stage ladder, honestPARTIAL, never reads DataFrame contents)CopilotReportdisplay, with a privacy-over-trust invariant enforced by a regression testfreshdata clean(--verbose,-vv,--output-format json,--no-color,--display); default CLI output unchangeddocs/peel.md+ nav entryPeel is a rendering layer over the existing report objects — it's opt-in, backward-compatible by default, and never replaces
to_dict()/to_json()/summary(). Full design spec (repo inspection, output inventory, per-feature mockups, renderer architecture, migration plan) was written and reviewed before implementation, with AI tooling assisting the implementation.Test plan
ruff check .— cleanmypy src/freshdata— clean (199 source files)pytest -m "not online and not large", matches CI'squality-fast/test-matrix) — 93.67% coverage, above the 93% gatetests/test_plugins.py(plugin-contract PR check) — passtests/test_packaging.py(install-hint / distribution-name check) — passmkdocs build --strict— cleantests/test_peel_*.py)mainin (8 commits: plan/apply, ValidationSuite, fallback_policy, comparators/exporters) to avoid opening against a stale base; re-verified all of the above after the mergeSummary by CodeRabbit
cleancommand, including format and color options while preserving existing cleaning behavior.