Production-audit remediation: safe-by-default cleaning, validator-gated PII, honest release posture#154
Merged
Merged
Conversation
- drop_duplicates now False under every strategy: duplicates are detected and reported, never removed, until explicitly requested. New duplicate_ratio_action="warn"|"error" (DuplicateRatioError) escalation, shared with all native backends. - dayfirst="auto" no longer infers a column-wide day/month order from sampled values; day/month-ambiguous dates quarantine into report.coerced_cells with an audit action, unless dayfirst is explicit. - outlier_action="auto" flags under every strategy including aggressive; winsorizing requires an explicit cap and then uses skew-aware log-space fences (widen-only) so lawful heavy tails survive. Column-name outlier heuristics moved behind domain_sensitive_names=False. - Closed a real invariant hole: the never-impute-coerced-cells guard read only the 1000-row-capped preview; new uncapped report.coerced_rows. - Gold-oracle harness and TruthBench duplicate-case observation updated to the detection-only default; goldens regenerated (sanctioned diffs only). - New regression suite tests/test_audit_core_defaults.py (21 tests).
…SV formula safety - PHONE/CREDIT_CARD/IBAN regex matches must now pass post-match validators: structural/boundary plausibility for phones, Luhn (13-19 digits) for cards, ISO 13616 mod-97 for IBANs (spaced and compact). Kills the date/UUID/licence/Aadhaar-like/semver -> PHONE and IBAN -> CREDIT_CARD false-positive classes; zero true positives lost (canary-verified). Checksum-verified findings report source=checksum. - anonymize() with no rules and no detection_config raises ValueError instead of silently returning unmasked data. - Every first-party CSV sink sanitizes spreadsheet formulas by default (clean_csv, clean/apply-plan CLI, streaming CLI incl. quarantine export, HTML-report ledger download), now also covering triggers behind leading whitespace; sanitize_formulas=False / --no-sanitize-formulas opt out. - New CLI --drop-duplicates opt-in flag; fd.clean docstring updated to the detection-only duplicates default. - New regression suite tests/test_audit_privacy_csv.py (41 tests).
… re-armed T5 gate - Development Status classifier 5 - Production/Stable -> 4 - Beta: the project's own absolute release gates (CleanBench T5 on release infrastructure) have not yet held through a completed release. - Refreshed benchmarks/truthbench/results/baseline.json: the four gates recorded KNOWN-RED (cleaning:raw_pii_leakage, cleaning:review_routing, cleaning:exact_repair, generated_code_sandbox) have passed since the v1.2.0 release-audit fixes; the stale record was itself the audit's P0-1 finding. All 48 gates green; any failure is now a PR-ratchet regression. - perf-regression.yml: removed the '|| true' and throwaway profiler left by the temporary T5 probes (PRs #152/#153), restoring the nightly gate and its failure alerting. - CHANGELOG/README/docs updated for the new safe defaults.
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
|
Warning Review limit reached
Next review available in: 33 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 changes cleaning safety defaults: duplicates are report-only unless explicitly removed, automatic outlier handling flags values, ambiguous dates are quarantined, PII detection is validated, and CSV exports sanitize formulas by default. Native backends, CLIs, reports, documentation, fixtures, tests, and quality gates are updated accordingly. ChangesProduction-readiness safety defaults
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cleaning-engine.md (1)
65-71: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale "Duplicates" section still says removed by default — contradicts line 25 and the new default.
This section wasn't updated alongside Layer 1's table entry (line 25) and still claims duplicates are removed automatically, which is no longer true (
drop_duplicatesnow defaults toFalse; rows are detected/reported only).📝 Proposed fix
### Duplicates -Exact duplicates are removed by default (count and percentage reported). +Exact duplicates are detected and reported by default, but not removed — +pass `drop_duplicates=True` to remove them (count and percentage reported). Time-indexed frames never lose rows unless `allow_timeseries_duplicates=True`.🤖 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 `@docs/cleaning-engine.md` around lines 65 - 71, Update the “Duplicates” section in cleaning-engine.md to reflect that drop_duplicates defaults to false: duplicates are detected and reported by default, not removed. Preserve the documented time-series exception and the duplicate_subset aggregation behavior, adjusting only statements that imply automatic removal.
🧹 Nitpick comments (2)
docs/threat-model.md (1)
20-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using "who" instead of "that" when referring to a person.
For better readability, use "who" when referring to "a consumer".
💡 Proposed grammatical tweak
-JSONL and Parquet are never altered. **Residual risk:** a consumer that +JSONL and Parquet are never altered. **Residual risk:** a consumer who opted out and opens the CSV in a spreadsheet re-accepts the injection risk.🤖 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 `@docs/threat-model.md` around lines 20 - 34, Update the residual-risk sentence in the threat-model documentation to use “who” instead of “that” when referring to a consumer, without changing its meaning or any surrounding CSV safety details.Source: Linters/SAST tools
src/freshdata/steps/duplicates.py (1)
175-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
check_duplicate_ratiocall on the detection-only branch.
high_ratiois computed unconditionally at line 178, but whendrop_duplicates=Falsethe code immediately callsreport_detected_duplicates, which recomputes the identical check internally — the line-178 result is discarded. Not a correctness bug (anaction="error"escalation still fires either way), just a wasted call and a slightly confusing dead value.♻️ Suggested fix
n_before = len(df) pct = 100.0 * n_dup / n_before where = f" (compared on {subset})" if subset else "" - high_ratio = check_duplicate_ratio(n_dup, n_before, config) if not config.drop_duplicates: report_detected_duplicates(n_dup, n_before, config, report, subset=subset) return df + high_ratio = check_duplicate_ratio(n_dup, n_before, config) + if isinstance(df.index, pd.DatetimeIndex) and not config.allow_timeseries_duplicates:🤖 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/steps/duplicates.py` around lines 175 - 182, Move the check_duplicate_ratio call and high_ratio assignment below the !config.drop_duplicates early-return branch in the duplicate-processing flow. Keep report_detected_duplicates as the detection-only path, and compute high_ratio only when the subsequent drop-duplicates logic uses it.
🤖 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/privacy.py`:
- Around line 946-956: Update the fail-closed guard in anonymize() to also raise
when no rules are provided and detection_config is disabled, matching the
downstream enabled check. Extend test_anonymize_raises_when_nothing_to_apply to
cover PIIDetectionConfig(enabled=False) while preserving the existing error
behavior.
---
Outside diff comments:
In `@docs/cleaning-engine.md`:
- Around line 65-71: Update the “Duplicates” section in cleaning-engine.md to
reflect that drop_duplicates defaults to false: duplicates are detected and
reported by default, not removed. Preserve the documented time-series exception
and the duplicate_subset aggregation behavior, adjusting only statements that
imply automatic removal.
---
Nitpick comments:
In `@docs/threat-model.md`:
- Around line 20-34: Update the residual-risk sentence in the threat-model
documentation to use “who” instead of “that” when referring to a consumer,
without changing its meaning or any surrounding CSV safety details.
In `@src/freshdata/steps/duplicates.py`:
- Around line 175-182: Move the check_duplicate_ratio call and high_ratio
assignment below the !config.drop_duplicates early-return branch in the
duplicate-processing flow. Keep report_detected_duplicates as the detection-only
path, and compute high_ratio only when the subsequent drop-duplicates logic uses
it.
🪄 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: 7b506b56-bfd9-484b-9713-f05c158dfc9e
📒 Files selected for processing (65)
.github/workflows/perf-regression.ymlCHANGELOG.mdREADME.mdbenchmarks/harness_metrics.pybenchmarks/truthbench/results/baseline.jsonbenchmarks/truthbench/runner.pydocs/cleaning-engine.mddocs/quickstart.mddocs/threat-model.mdpyproject.tomlsrc/freshdata/_util.pysrc/freshdata/api.pysrc/freshdata/cleaner.pysrc/freshdata/config.pysrc/freshdata/engine/missing.pysrc/freshdata/engine/model_select.pysrc/freshdata/engine/outliers.pysrc/freshdata/enterprise/cli.pysrc/freshdata/enterprise/privacy.pysrc/freshdata/execution/_native_steps.pysrc/freshdata/execution/_plan.pysrc/freshdata/execution/backends/_duckdb.pysrc/freshdata/execution/backends/_freshcore.pysrc/freshdata/execution/backends/_polars.pysrc/freshdata/execution/backends/_spark.pysrc/freshdata/experimental/ai_copilot.pysrc/freshdata/render/renderers.pysrc/freshdata/report.pysrc/freshdata/steps/dtypes.pysrc/freshdata/steps/duplicates.pysrc/freshdata/steps/outliers.pysrc/freshdata/streaming/_cli.pytests/fixtures/golden/aqi_sample.balanced.report.jsontests/fixtures/golden/duplicate_heavy.balanced.report.jsontests/fixtures/golden/fraud_signals.balanced.report.jsontests/fixtures/golden/large_panel.balanced.report.jsontests/fixtures/golden/sales_export.balanced.report.jsontests/fixtures/golden/sensor_timeseries.balanced.report.jsontests/fixtures/golden_diff_summary.jsonltests/fixtures/online/golden/air_quality_uci.balanced.report.jsontests/fixtures/online/golden/heart_cleveland.balanced.report.jsontests/fixtures/online/golden/iris.balanced.report.jsontests/fixtures/online/golden/wine_quality.balanced.report.jsontests/test_api.pytests/test_audit_core_defaults.pytests/test_audit_privacy_csv.pytests/test_backend_honesty.pytests/test_cleanbench.pytests/test_compare_matrix.pytests/test_config_tuple_coercion.pytests/test_csv_formula_sanitize.pytests/test_dtypes.pytests/test_engine_duplicates.pytests/test_engine_outliers.pytests/test_enterprise_privacy.pytests/test_execution/test_duckdb_engine.pytests/test_execution/test_fallback_policy.pytests/test_execution/test_plan_generator.pytests/test_explain.pytests/test_insight.pytests/test_outliers.pytests/test_peel_cli.pytests/test_properties.pytests/test_report.pytests/test_rows.py
The Validation Gauntlet caught a cascade from the detection-only duplicates default: an injected exact-duplicate row broke the all-unique key check, so a near-unique identifier (healthcare mrn) fell back to categorical and its missing value was filled with 'Unknown' — a labelled corruption. build_contexts now computes the full-row duplicate mask once; when the raw all-unique check fails, the id role is re-tested on distinct rows only, and only the upgrade to id is accepted. Unhashable payloads skip the mask. Gauntlet gates green again; TruthBench 48/48 and the full fast lane pass.
This was referenced Jul 20, 2026
Merged
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.
Remediates the July 2026 "FreshData v1.2.0 Production Readiness Audit" (run against 497c2f6). Every finding was independently reproduced before fixing; one audit conclusion turned out to be stale bookkeeping rather than a code defect, and one gate-integrity problem the audit missed was found and fixed.
P0 — release posture
--profile release --backends pandas,polars,duckdb --require-backends --repeats 2 --check) passes 48/48 gates at the audited revision 497c2f6 and at main — the release-audit fixes that shipped in v1.2.0 fixed them, but the committedbaseline.jsonstill recorded the pre-fix state, which is what the audit read. The baseline is refreshed from a green run: any future failure of those gates is now a PR-ratchet regression, not a shrug.4 - Beta. The project's own absolute gates have not yet held through a completed release (the v1.2.0 attempt stopped on the CleanBench T5 runtime gate), soProduction/Stablewas an overclaim.|| trueonperf-regression.yml's CleanBench T5 step, silently disabling both the nightly gate and its failure alerting. Removed, along with the throwaway profiler block (release.yml's copy was never affected).P1 — safe-by-default behavior (breaking by design, all documented in CHANGELOG)
drop_duplicates=True, CLI--drop-duplicates);duplicate_ratio_action="error"stops suspicious joins13/01/2023flipped a whole column to day-firstdayfirst="auto"never infers; ambiguous values quarantine intoreport.coerced_cellswith an audit action; explicitdayfirstparses fullyaggressivewinsorized (lognormal max 6.4M → 168k)autoflags under every strategy; explicitcapuses skew-aware log-space fences (widen-only)amountpreserved,xyzcapped on identical datadomain_sensitive_names=False; identical data → identical treatmentsource="checksum"ValueErrorwithout rules/detection config--no-sanitize-formulas/sanitize_formulas=Falseopt outAlso closes a real invariant hole found during the work: the never-impute-coerced-cells guard only read the 1000-row-capped preview list, so casualties beyond the cap could be imputed (new uncapped
report.coerced_rows).Verification
tests/test_audit_core_defaults.py,tests/test_audit_privacy_csv.py), each pinned to a confirmed audit repro; existing tests updated only where they encoded the old unsafe defaults (none weakened/skipped).-m "not online and not large", 4039 tests): pass. ruff (full tree): clean. mypysrc(3.12 target locally): clean. Wheel + sdist build: clean, wheel METADATA carries the Beta classifier.--check-gates: all release gates pass (T5 runtime +6.4% vs ≤20% budget).Known residual: local coverage reads 91.9% because this environment lacks the optional extras CI installs (the unmodified base reads 91.94% in the same venv; the diff's delta is −0.02pt with ~1,900 lines added) — CI's 93% gate is the authoritative check on this PR.
Prepared with AI-assisted tooling; reviewed and directed by the author.
Summary by CodeRabbit
New Features
Bug Fixes