diff --git a/docs/transform-rules.md b/docs/transform-rules.md index 93f2fb2..d3d70fb 100644 --- a/docs/transform-rules.md +++ b/docs/transform-rules.md @@ -2,19 +2,28 @@ The model can suggest only rules that the executor implements, and a contract test (`lambdas/tests/test_contracts.py`) keeps the two lists in sync. In auto-clean, each rule's risk tier sets the number of committee votes it needs: LOW 1/3, MEDIUM 2/3, HIGH 3/3. When a rule fails or is unsafe (an unknown type, a missing column, more than 20% row loss, a bad regex, or a cast that would lose data), the executor restores the frame and records the rule as *not applied*, with the reason. +**Rows are never removed silently.** Only the row-selecting rules (`drop_nulls`, `filter`, `filter_extended`, `deduplicate`, `semantic_deduplicate`) can remove rows. The Data PR marks them "removes rows". The executor records how many rows each rule removed (`transform_rules.parameters._execution.rows_removed`), and the run page shows the count under the rule and the total in the run summary. A value that can't be converted (a placeholder in a numeric column, for example) becomes null in that cell. It never removes the row. + +**Bad-cell guard.** A row-removing rule is flagged when it would remove otherwise valid rows only because one cell holds a bad value: a common placeholder (`.`, `-`, `?`, `N/A`, `#VALUE!`, ...) or, in a mostly numeric column, a value that is not a number. The check is deterministic and runs twice: + +- **Before review** (`src/lib/rule-guard.ts`): after the AI returns rules, `filter`/`filter_extended` rules that remove rows for such a value (`neq`, `not_in`, `not_contains`), and `drop_nulls` on a column an earlier rule casts to a number, get `parameters._guard`. The rule card shows a warning and the suggested alternative (`type_cast` with `null_values`, so the cell becomes blank and the row stays). "Approve all" skips flagged rules, so each one needs its own decision, and auto-validate never approves one. +- **During execution** (`bad_cell_rows()` in the executor): the executor counts the rows each rule removed for one bad cell, where the rest of the row is at least half filled in, and records `_execution.bad_cell_rows`. The run page shows the count under the rule and in the run summary. In auto mode, where nobody approved the rule, the executor skips such a rule and keeps the rows. A rule a person approved in the Data PR still runs. + +Legitimate filters are not flagged: comparisons (`gt`, `lt`, ...), keep-only matches (`eq`, `in`, `regex`), numeric values (`neq 0`), real text values (`neq "deleted"`, `not_in ["test"]`), removing blank cells, and rows that are mostly empty. Numeric codes such as `99999` are not treated as bad cells, because they can be real values. The placeholder list is shared by both checks (`PLACEHOLDER_TOKENS`, kept in sync by `lambdas/tests/test_contracts.py`). + ## Tabular | Rule | What it does | |---|---| -| `trim_whitespace` | Strips leading and trailing whitespace in text columns | +| `trim_whitespace` | Strips leading and trailing whitespace from text values. Numbers in a mixed column stay numbers | | `deduplicate` | Drops exact duplicate rows | | `semantic_deduplicate` | Drops near-duplicate rows, keeping the first. Uses a MinHash Jaccard estimate ≥ `threshold` (default 0.8) over lowercased word sets, with LSH banding | | `fill_nulls` | Fills nulls with the mean, median, mode or a constant | | `drop_nulls` | With a column, drops rows where that column is null. Without one, drops rows with at least `ceil(threshold × columns)` nulls | -| `type_cast` | Casts to float, int, datetime or str. Not applied if values would be lost (for example `2.5` → int) | +| `type_cast` | Casts to float, int, datetime or str. Values that can't be parsed become null, and the row is kept. Optional `null_values` (for example `[99999, "N/A"]`) also turns those placeholder codes into null. Not applied if values would be lost (for example `2.5` → int) | | `normalize` | Text and date columns only. Converts dates to `YYYY-MM-DD` and trims and lowercases other text, or maps it through `value_map` | -| `filter`, `filter_extended` | Removes rows that match a condition | -| `rename`, `column_header_normalize` | Renames columns, or converts headers to snake_case | +| `filter`, `filter_extended` | Removes rows that match a condition. Comparison operators (`gt`, `lt`, `gte`, `lte`) remove only rows that hold a number outside the range. Rows whose value is missing or not a number are kept | +| `rename`, `column_header_normalize` | Renames columns, or converts headers to snake_case. A column's `__orig_` sidecar is renamed with it | | `ner_redact` | Regex-based redaction of names, organizations, addresses or ZIP codes, and dates. This is a heuristic, not a trained NER model | | `ffill`, `bfill` | Fills nulls from the previous or next row | | `bool_cast` | Normalizes yes/no, true/false and 1/0 variants | @@ -22,7 +31,7 @@ The model can suggest only rules that the executor implements, and a contract te | `multi_currency_strip` | Strips currency symbols and converts the values to numbers | | `split_column` | Splits a column on a delimiter into new columns | -Rules that rewrite values keep the original in an `__orig_` sidecar, which is written only to `audit.csv`. +Rules that rewrite values keep the original in an `__orig_` sidecar, which is written only to `audit.csv` and never to the deliverable. A sidecar is kept only if at least one value in the column lost information: a value became null or appeared, the text changed, or text became a number it doesn't spell exactly (`"$1,200"` → 1200, `"007"` → 7). A cast of `"34"` to 34, or a fill or cap that changed nothing, adds no sidecar. ## Documents (PDF, DOCX, TXT) diff --git a/lambdas/executor/handler.py b/lambdas/executor/handler.py index 7391396..023de57 100644 --- a/lambdas/executor/handler.py +++ b/lambdas/executor/handler.py @@ -660,6 +660,18 @@ def _to_numeric_clean(series: pd.Series) -> pd.Series: ROW_LOSS_GUARD = 0.20 # per-rule maximum row loss for LOSS rules +# Bad-cell guard. A row-removing rule is flagged when rows it removes are otherwise valid +# and were removed only because ONE cell holds a placeholder (below) or, in a mostly numeric +# column, a non-number. Must stay identical to PLACEHOLDER_TOKENS in src/lib/rule-guard.ts +# (test_contracts.py), which flags the same rules in the Data PR before they are approved. +PLACEHOLDER_TOKENS = frozenset({ + ".", "..", "...", "-", "--", "---", "?", "??", "???", + "n/a", "na", "n.a.", "n.a", "#n/a", "null", "none", "nil", "nan", "missing", + "unknown", "undefined", "not available", "not applicable", "tbd", "tbc", + "#value!", "#ref!", "#div/0!", "#name?", "#num!", "#null!", "#error!", +}) +BAD_CELL_GUARD_RULES = {"filter", "filter_extended", "drop_nulls"} + class RuleSkipped(Exception): """Raised inside a rule branch when the rule cannot be applied safely; the frame is restored.""" @@ -671,6 +683,89 @@ def _add_sidecar(df: pd.DataFrame, col) -> None: df[sidecar] = df[col] +def _sidecar_base(name) -> str | None: + """'__orig_price' -> 'price'; None for a column that is not a sidecar.""" + s = str(name) + return s[len(SIDECAR_PREFIX):] if s.startswith(SIDECAR_PREFIX) else None + + +# A text value is "just the number" when it is a plain decimal literal: no whitespace, +# thousands separators, currency, exponent or leading zeros ("007" and "02139" are kept +# as text in the audit file because the zeros would be lost). +_PLAIN_NUMBER = re.compile(r"[+-]?(?:0|[1-9]\d*)(?:\.\d+)?") + + +def _is_null(v) -> bool: + try: + return bool(pd.isna(v)) + except (TypeError, ValueError): + return False # list-like cell (JSON input): never null + + +def _is_number(v) -> bool: + return isinstance(v, (int, float, np.integer, np.floating)) and not isinstance(v, (bool, np.bool_)) + + +def _value_preserved(orig, new) -> bool: + """True when ``new`` carries all the information of ``orig`` (no audit copy needed).""" + o_null, n_null = _is_null(orig), _is_null(new) + if o_null or n_null: + return o_null and n_null # a value that appeared or disappeared is a change + if isinstance(orig, str): + if isinstance(new, str): + return orig == new + if _is_number(new): + return _PLAIN_NUMBER.fullmatch(orig) is not None and float(orig) == float(new) + return False + if _is_number(orig) and _is_number(new): + return float(orig) == float(new) + if type(orig) is not type(new) and (isinstance(orig, (bool, np.bool_)) or isinstance(new, (bool, np.bool_))): + return False + try: + return bool(orig == new) + except Exception: + return False + + +def _column_changed_lossily(orig: pd.Series, new: pd.Series) -> bool: + if orig.equals(new): + return False + return not all(_value_preserved(o, n) for o, n in zip(orig.tolist(), new.tolist())) + + +def _matches_null_value(v, null_values: list) -> bool: + """True when cell ``v`` equals one of ``null_values`` (text case-insensitively, numbers by value).""" + if _is_null(v): + return False + text = str(v).strip().lower() + for nv in null_values: + if text == str(nv).strip().lower(): + return True + try: + if float(text) == float(nv): + return True + except (TypeError, ValueError): + pass + return False + + +def _drop_noop_sidecars(df: pd.DataFrame, before_cols) -> pd.DataFrame: + """Drop __orig_* columns a rule just added when no value in the column lost information. + + Sidecars exist so the audit file can show what a lossy change replaced (a value that + became null, "$1,200" that became 1200). A cast of "34" to 34 loses nothing, so it + gets no copy. + """ + noop = [] + for c in df.columns: + base = _sidecar_base(c) + if base is None or c in before_cols or base not in df.columns: + continue + if not _column_changed_lossily(df[c], df[base]): + noop.append(c) + return df.drop(columns=noop) if noop else df + + def _row_loss_guard(before: pd.DataFrame, after: pd.DataFrame, rtype: str) -> None: loss_pct = 1 - len(after) / max(len(before), 1) if loss_pct > ROW_LOSS_GUARD: @@ -929,6 +1024,14 @@ def _apply_rule(df: pd.DataFrame, rtype: str, col, params: dict) -> pd.DataFrame if rtype == "type_cast": target = params.get("target_type", "str") + null_values = params.get("null_values") + if null_values: + # Placeholder codes the parser would accept as numbers (99999, -9999) or text it + # would not ("N/A") become null; the row is kept. The original goes to the sidecar. + if not isinstance(null_values, list): + null_values = [null_values] + _add_sidecar(df, col) + df[col] = df[col].where(~df[col].map(lambda v: _matches_null_value(v, null_values)), other=None) if target in ("float", "float64", "numeric", "number", "int", "int64", "datetime", "date", "timestamp"): _add_sidecar(df, col) if target in ("float", "float64", "numeric", "number"): @@ -957,7 +1060,10 @@ def _apply_rule(df: pd.DataFrame, rtype: str, col, params: dict) -> pd.DataFrame new_name = params.get("new_name") if not new_name: raise RuleSkipped("missing new_name") - return df.rename(columns={col: new_name}) + mapping = {col: new_name} + if f"{SIDECAR_PREFIX}{col}" in df.columns: + mapping[f"{SIDECAR_PREFIX}{col}"] = f"{SIDECAR_PREFIX}{new_name}" + return df.rename(columns=mapping) if rtype in ("filter", "filter_extended"): before = df @@ -976,7 +1082,9 @@ def _apply_rule(df: pd.DataFrame, rtype: str, col, params: dict) -> pd.DataFrame num = _to_numeric_clean(series) v = float(value) mask = {"gt": num > v, "lt": num < v, "gte": num >= v, "lte": num <= v}[operator] - df = df[mask] + # A missing or unparseable cell ("." placeholder) is not "out of range": keep the + # row. Only rows holding a number that fails the comparison are removed. + df = df[mask | num.isna()] elif rtype == "filter_extended" and operator == "contains": df = df[series.astype(str).str.contains(str(value), na=False, regex=False)] elif rtype == "filter_extended" and operator == "not_contains": @@ -1024,10 +1132,14 @@ def _apply_rule(df: pd.DataFrame, rtype: str, col, params: dict) -> pd.DataFrame if rtype == "trim_whitespace": targets = [col] if (col and col in df.columns) else _text_cols(df) for c in targets: - present = df[c].notna() - stripped = df[c][present].astype(str).str.strip() + # Only text values are stripped. Numbers in a mixed column (Excel cells holding + # 34 next to a "." placeholder) stay numbers instead of becoming "34". + is_str = df[c].map(lambda v: isinstance(v, str)) + if not is_str.any(): + continue + stripped = df[c][is_str].str.strip() new_col = df[c].astype(object).copy() - new_col[present] = stripped.where(stripped != "", other=None) + new_col[is_str] = stripped.where(stripped != "", other=None) df[c] = new_col return df @@ -1110,7 +1222,18 @@ def _to_snake(name: str) -> str: s = re.sub(r'([a-z])([A-Z])', r'\1_\2', s) s = re.sub(r'_+', '_', s) return s.lower().strip('_') - rename_map = {c: _to_snake(str(c)) for c in df.columns if _to_snake(str(c)) != str(c)} + # Sidecars are renamed with their column ("__orig_Acceptance Rate(%)" -> + # "__orig_acceptance_rate"). Snake-casing the sidecar name itself would strip the + # "__" prefix, and the copy would leak into the deliverable as "orig_". + rename_map = {} + for c in df.columns: + if _sidecar_base(c) is not None: + continue + new_name = _to_snake(str(c)) + if new_name != str(c): + rename_map[c] = new_name + if f"{SIDECAR_PREFIX}{c}" in df.columns: + rename_map[f"{SIDECAR_PREFIX}{c}"] = f"{SIDECAR_PREFIX}{new_name}" if rename_map: df = df.rename(columns=rename_map) return df @@ -1168,34 +1291,111 @@ def _apply_ner(text: str) -> str: raise RuleSkipped(f"unsupported rule type {rtype!r}") -def apply_transforms(df: pd.DataFrame, rules: list[dict], results: list | None = None) -> pd.DataFrame: +def _blank(v) -> bool: + return _is_null(v) or (isinstance(v, str) and v.strip() == "") + + +def _is_bad_cell(v, mostly_numeric: bool) -> bool: + """A placeholder token, or a non-number in a mostly numeric column. Blanks are not bad cells.""" + if _blank(v) or _is_number(v) or isinstance(v, (bool, np.bool_)): + return False + s = str(v).strip() + if s.lower() in PLACEHOLDER_TOKENS: + return True + return mostly_numeric and pd.isna(_to_numeric_clean(pd.Series([s])).iloc[0]) + + +def _mostly_numeric(values: pd.Series) -> bool: + present = values[~values.map(_blank)] + if len(present) < 2: + return False + return bool(_to_numeric_clean(present).notna().mean() >= 0.5) + + +def bad_cell_rows(before: pd.DataFrame, after: pd.DataFrame, rtype: str, col) -> int: + """Rows a rule removed only because of one bad cell in ``col`` (see PLACEHOLDER_TOKENS). + + A removed row counts when its ``col`` value (or, if a cast already blanked it, the original + kept in the sidecar) is a bad cell and the rest of the row is mostly filled in. Rows that are + mostly empty, rows removed for a real value ("deleted", 0, out of range) and blank cells + removed by drop_nulls do not count, so legitimate filters are never flagged. + """ + if rtype not in BAD_CELL_GUARD_RULES or not col or col not in before.columns: + return 0 + if not after.index.isin(before.index).all(): + return 0 # the rule rebuilt the index: not a row selection we can attribute + removed = before.loc[~before.index.isin(after.index)] + if removed.empty: + return 0 + sidecar = f"{SIDECAR_PREFIX}{col}" + + def original(frame: pd.DataFrame) -> pd.Series: + if sidecar not in frame.columns: + return frame[col] + return frame[col].where(~frame[col].map(_is_null), frame[sidecar]) + + mostly_numeric = _mostly_numeric(original(before)) + others = [c for c in before.columns if c != col and _sidecar_base(c) is None] + if not others: + return 0 # a single-column file: the bad cell is the whole row + need = max(1, (len(others) + 1) // 2) + filled = removed[others].apply(lambda r: sum(not _blank(v) for v in r), axis=1) + bad = original(removed).map(lambda v: _is_bad_cell(v, mostly_numeric)) + return int((bad & (filled >= need)).sum()) + + +def apply_transforms(df: pd.DataFrame, rules: list[dict], results: list | None = None, + skip_bad_cell_rules: bool = False) -> pd.DataFrame: """Apply rules in order. A rule that is skipped or raises leaves the frame exactly as it was before that rule - (no half-applied changes, no stray sidecar columns). When ``results`` is given, one entry - per rule is appended: {"id", "rule_type", "column_name", "applied", "reason"}. + (no half-applied changes, no stray sidecar columns). A sidecar a rule added is dropped + again when no value in its column lost information. When ``results`` is given, one entry + per rule is appended: {"id", "rule_type", "column_name", "applied", "reason", + "rows_removed"}. Only rules that select rows (drop_nulls, filter, deduplicate, ...) can + remove rows; every removal is counted and logged, never silent. + + Each entry also has "bad_cell_rows": rows the rule removed (or, when skipped by the + guard, would have removed) only because of one bad cell (bad_cell_rows()). With + ``skip_bad_cell_rules`` (auto mode, where no person approved the rule) such a rule is + skipped and the rows are kept; otherwise it is applied as approved and reported. """ for rule in rules: rtype = rule["rule_type"] col = rule.get("column_name") params = _parse_params(rule.get("parameters")) snapshot = df.copy() - applied, reason = True, None + before_cols = set(df.columns) + rows_before = len(df) + applied, reason, bad_rows = True, None, 0 try: if rtype not in SUPPORTED_TABULAR_RULES: raise RuleSkipped(f"unsupported rule type {rtype!r}") if rtype in COLUMN_REQUIRED_RULES and (not col or col not in df.columns): raise RuleSkipped(f"column {col!r} not found") - df = _apply_rule(df, rtype, col, params) + df = _drop_noop_sidecars(_apply_rule(df, rtype, col, params), before_cols) + bad_rows = bad_cell_rows(snapshot, df, rtype, col) + if bad_rows and skip_bad_cell_rules: + raise RuleSkipped( + f"guard: would remove {bad_rows} row(s) only because of one bad cell in {col!r} " + "(a placeholder or non-number); the rows were kept. Convert that value to blank " + "instead (type_cast with null_values)." + ) except RuleSkipped as e: df, applied, reason = snapshot, False, str(e) except Exception as e: # unexpected failure: restore and report, never silently half-apply df, applied, reason = snapshot, False, f"error: {type(e).__name__}: {e}"[:500] + rows_removed = rows_before - len(df) if not applied: print(f"[executor] rule {rtype} on {col} not applied: {reason}") + elif rows_removed: + print(f"[executor] rule {rtype} on {col} removed {rows_removed} of {rows_before} rows") + if applied and bad_rows: + print(f"[executor] WARNING rule {rtype} on {col} removed {bad_rows} row(s) only because of one bad cell") if results is not None: results.append({"id": rule.get("id"), "rule_type": rtype, "column_name": col, - "applied": applied, "reason": reason}) + "applied": applied, "reason": reason, "rows_removed": rows_removed, + "bad_cell_rows": bad_rows}) return df @@ -1308,7 +1508,9 @@ def _record_rule_results(cur, results: list) -> None: continue cur.execute( "UPDATE transform_rules SET parameters = COALESCE(parameters, '{}'::jsonb) || %s::jsonb WHERE id = %s", - (json.dumps({"_execution": {"applied": r["applied"], "reason": r["reason"]}}), r["id"]), + (json.dumps({"_execution": {"applied": r["applied"], "reason": r["reason"], + "rows_removed": int(r.get("rows_removed") or 0), + "bad_cell_rows": int(r.get("bad_cell_rows") or 0)}}), r["id"]), ) @@ -1512,7 +1714,9 @@ def process_run(run_id: str, receive_count: int = 1) -> dict: else: df = load_raw_dataframe(file_bytes, fmt) input_row_count = len(df) - df = apply_transforms(df, rules, rule_results) + # Auto mode: nobody approved the rules, so a rule that would drop rows only + # because of one bad cell is skipped (bad_cell_rows()). + df = apply_transforms(df, rules, rule_results, skip_bad_cell_rules=auto_mode) # Row count guard for auto-mode passes 2+ — abort if >10% rows deleted if auto_mode and iteration > 1 and row_count_raw: diff --git a/lambdas/tests/test_bad_cell_guard.py b/lambdas/tests/test_bad_cell_guard.py new file mode 100644 index 0000000..096294e --- /dev/null +++ b/lambdas/tests/test_bad_cell_guard.py @@ -0,0 +1,87 @@ +"""Bad-cell guard: a row-removing rule that drops otherwise valid rows only because ONE cell +holds a placeholder or (in a mostly numeric column) a non-number is reported, and skipped in +auto mode. Legitimate filters are never flagged.""" +import pandas as pd +import pytest + +from conftest import rule + + +def _frame(): + # 10 rows (the row-loss guard allows 20% per rule); row 3 has one bad score cell, + # row 9 is a mostly empty spacer whose only value is a placeholder. + names = [f"item {i}" for i in range(10)] + scores = [10, 20, 30, ".", 50, 60, 70, 80, 90, "."] + status = ["active"] * 8 + ["deleted", None] + cities = ["Austin"] * 9 + [None] + names[9] = None + return pd.DataFrame({"name": names, "score": scores, "status": status, "city": cities}) + + +def _run(executor, df, rules, **kw): + results = [] + out = executor.apply_transforms(df.copy(), rules, results, **kw) + return out, results + + +def test_reports_rows_removed_for_one_bad_cell(executor): + out, res = _run(executor, _frame(), [rule("filter", "score", operator="neq", value=".")]) + assert res[0]["applied"] is True + assert res[0]["rows_removed"] == 2 + assert res[0]["bad_cell_rows"] == 1 # row 3; the spacer row is not "otherwise valid" + assert "item 3" not in set(out["name"]) + + +def test_auto_mode_skips_the_rule_and_keeps_the_rows(executor): + df = _frame() + out, res = _run(executor, df, [rule("filter", "score", operator="neq", value=".")], + skip_bad_cell_rules=True) + assert res[0]["applied"] is False + assert res[0]["reason"].startswith("guard: would remove 1 row(s) only because of one bad cell") + assert res[0]["rows_removed"] == 0 + assert res[0]["bad_cell_rows"] == 1 + assert len(out) == len(df) + + +def test_placeholder_in_a_text_column_is_flagged(executor): + df = _frame() + df.loc[2, "status"] = "N/A" + _, res = _run(executor, df, [rule("filter_extended", "status", operator="not_in", values=["N/A"])]) + assert res[0]["bad_cell_rows"] == 1 + + +def test_drop_nulls_after_a_cast_counts_the_blanked_placeholder(executor): + df = _frame() + df.loc[5, "city"] = None # a real blank elsewhere does not matter + out, res = _run(executor, df, [rule("type_cast", "score", target_type="float"), + rule("drop_nulls", "score")]) + assert res[1]["rows_removed"] == 2 + assert res[1]["bad_cell_rows"] == 1 + + +@pytest.mark.parametrize("rules", [ + [rule("filter", "status", operator="neq", value="deleted")], # real value in a text column + [rule("filter", "score", operator="gt", value=15)], # comparison + [rule("filter", "score", operator="neq", value=10)], # a number + [rule("filter", "status", operator="eq", value="active")], # keep-only + [rule("drop_nulls", "status")], # raw blanks + [rule("filter_extended", "name", operator="not_contains", value="item 1")], +]) +def test_legitimate_filters_are_not_flagged(executor, rules): + df = _frame() + out, res = _run(executor, df, rules, skip_bad_cell_rules=True) + assert res[0]["bad_cell_rows"] == 0 + assert res[0]["applied"] is True + assert res[0]["rows_removed"] == len(df) - len(out) + + +def test_single_column_file_is_not_flagged(executor): + df = pd.DataFrame({"score": [1, 2, 3, 4, 5, 6, 7, 8, 9, "."]}) + _, res = _run(executor, df, [rule("filter", "score", operator="neq", value=".")], + skip_bad_cell_rules=True) + assert res[0]["applied"] is True and res[0]["bad_cell_rows"] == 0 + + +def test_non_row_removing_rules_report_zero(executor): + _, res = _run(executor, _frame(), [rule("type_cast", "score", target_type="float")]) + assert res[0]["bad_cell_rows"] == 0 diff --git a/lambdas/tests/test_college_list.py b/lambdas/tests/test_college_list.py new file mode 100644 index 0000000..fa05d84 --- /dev/null +++ b/lambdas/tests/test_college_list.py @@ -0,0 +1,214 @@ +"""Regression tests from a real run on a small college list (xlsx) where every suggested +rule was approved and the output: + +1. lost the 'Brown' row, whose three numeric cells held the placeholder "."; +2. carried orig_ text copies of three numeric columns although no value changed; +3. lost the 'COLLEGEPOND REFERENCE' section label (merged A2:F2, right under the header). + +The fixture has the same shape: a header, a merged section label in row 2, blank spacer +rows, two more section labels, '.' placeholders, padded text and integer cells. +""" +import io + +import openpyxl +import pandas as pd +import pytest + +from conftest import rule + +U = "UNIVERESITY" +WR = "WORLD RANKING(FOR DATA SCIENCE)" +NAR = "NORTH AMERICA RANKING(FOR DATA SCIENCE)" +AR = "Acceptance Rate(%)" +HEADER = [U, WR, NAR, "LOCATION", "TYPE", "CHANCES", AR] +ROWS = [ + ["COLLEGEPOND REFERENCE"], # merged A2:F2 + [], + ["University of California, San Diago", 12, 10, "San Diego, California", "State University", "Super Ambitious", 34], + [" Columbia University", 24, 17, "New York", "Ivy League", "Super Ambitious", 4], + [], + ["Duke", 64, 42, "North Carolina", "Private University", "Ambitious", 6], + ["Brown", ".", ".", "Providence, Rhode Island", "Ivy League", "Ambitious", "."], + [], + ["MY PRIORITY"], # merged + [], + ["University of Washington", 5, 3, "Seattle, Washington", "State University", None, 53], + ["University of Illinois at Urbana - Champaign", 16, 12, "Champaign, Illinois", "State University", None, None], + ["LESS CHANCE BUT MY PRIORITY"], # merged + [], + ["Harvard", 3, 2, "Cambridge, Massachusetts", "Ivy League", None, 4], +] +LABELS = ["COLLEGEPOND REFERENCE", "MY PRIORITY", "LESS CHANCE BUT MY PRIORITY"] +BLANK_ROWS = sum(1 for r in ROWS if not r) + + +def college_xlsx() -> bytes: + wb = openpyxl.Workbook() + ws = wb.active + ws.append(HEADER) + for r in ROWS: + ws.append(r) + ws.merge_cells("A2:F2") + ws.merge_cells("A10:F10") + ws.merge_cells("A14:G14") + buf = io.BytesIO() + wb.save(buf) + return buf.getvalue() + + +# What the AI suggested for the real file, minus any row filter: no rule in this list +# targets rows by value, so every non-blank row must survive. +SUGGESTED = [ + rule("trim_whitespace", rule_id="trim"), + rule("deduplicate", rule_id="dedup"), + rule("drop_nulls", U, rule_id="drop-blank"), + rule("type_cast", AR, rule_id="cast-ar", target_type="float"), + rule("type_cast", WR, rule_id="cast-wr", target_type="int"), + rule("type_cast", NAR, rule_id="cast-nar", target_type="int"), + rule("column_header_normalize", rule_id="headers"), +] + + +@pytest.fixture +def raw(executor): + return executor.load_raw_dataframe(college_xlsx(), "xlsx") + + +def run(executor, df, rules): + results = [] + return executor.apply_transforms(df.copy(), rules, results), results + + +# ---------------------------------------------------------------- 1. Brown +def test_placeholder_cells_become_null_and_the_row_is_kept(executor, raw): + out, res = run(executor, raw, [rule("type_cast", c, target_type="int") for c in (WR, NAR, AR)]) + assert len(out) == len(raw) + brown = out[out[U] == "Brown"].iloc[0] + assert all(pd.isna(brown[c]) for c in (WR, NAR, AR)) + assert brown["LOCATION"] == "Providence, Rhode Island" + assert all(r["applied"] and r["rows_removed"] == 0 for r in res) + + +def test_suggested_rules_keep_brown_and_every_label_row(executor, raw): + out, res = run(executor, raw, SUGGESTED) + names = out["univeresity"].tolist() + assert "Brown" in names + for label in LABELS: + assert label in names + assert len(out) == len(raw) - BLANK_ROWS + removed = {r["id"]: r["rows_removed"] for r in res} + # Only the blank spacer rows go: the first by drop_nulls, the repeats by deduplicate. + assert removed["dedup"] + removed["drop-blank"] == BLANK_ROWS + assert all(v == 0 for k, v in removed.items() if k not in ("dedup", "drop-blank")) + + +def test_an_explicit_row_filter_still_works_and_reports_what_it_removed(executor, raw): + out, res = run(executor, raw, [rule("filter", WR, rule_id="f", operator="neq", value=".")]) + assert "Brown" not in out[U].tolist() + assert res == [{"id": "f", "rule_type": "filter", "column_name": WR, + "applied": True, "reason": None, "rows_removed": 1, "bad_cell_rows": 1}] + + +def test_auto_mode_guard_keeps_brown(executor, raw): + # Nobody approved the rule in auto mode: the bad-cell guard skips it and keeps the row. + results = [] + out = executor.apply_transforms(raw.copy(), [rule("filter", WR, operator="neq", value=".")], + results, skip_bad_cell_rules=True) + assert "Brown" in out[U].tolist() + assert results[0]["applied"] is False and results[0]["bad_cell_rows"] == 1 + + +def test_rows_removed_is_zero_for_a_rule_that_was_not_applied(executor, raw): + # drop_nulls on CHANCES would remove most rows: the row-loss guard skips it. + out, res = run(executor, raw, [rule("drop_nulls", "CHANCES")]) + assert len(out) == len(raw) + assert res[0]["applied"] is False and res[0]["rows_removed"] == 0 + + +# ---------------------------------------------------------------- 2. orig_ copies +def test_trim_whitespace_leaves_numbers_numeric(executor, raw): + out, _ = run(executor, raw, [rule("trim_whitespace")]) + assert out.loc[out[U] == "Duke", WR].iloc[0] == 64 + assert out.loc[out[U] == "Columbia University", U].iloc[0] == "Columbia University" + + +def test_lossless_cast_adds_no_sidecar(executor): + df = pd.DataFrame({"rank": [12, 24, None], "rate": ["34", "4", None]}) + out, _ = run(executor, df, [rule("type_cast", "rank", target_type="int"), + rule("type_cast", "rate", target_type="float")]) + assert list(out.columns) == ["rank", "rate"] + assert out["rate"].tolist()[:2] == [34.0, 4.0] + + +@pytest.mark.parametrize("original", [".", "$1,200", "007", " 12", "1e3"]) +def test_lossy_cast_keeps_the_original_in_a_sidecar(executor, original): + df = pd.DataFrame({"v": ["5", original]}) + out, _ = run(executor, df, [rule("type_cast", "v", target_type="float")]) + assert out["__orig_v"].tolist() == ["5", original] + + +def test_noop_fill_and_outlier_cap_add_no_sidecar(executor): + df = pd.DataFrame({"x": [1.0, 2.0, 3.0, 2.5]}) + out, _ = run(executor, df, [rule("fill_nulls", "x", strategy="mean"), + rule("outlier_cap", "x", min_val=0, max_val=10), + rule("ffill", "x")]) + assert list(out.columns) == ["x"] + + +def test_header_normalize_keeps_sidecars_out_of_the_deliverable(executor, raw): + # Brown's "." becoming null is a real loss, so these sidecars are kept, in audit.csv only. + out, _ = run(executor, raw, SUGGESTED) + assert "__orig_acceptance_rate" in out.columns + assert not [c for c in out.columns if str(c).startswith("orig_")] + deliverable, audit = executor.split_deliverable(out) + assert list(deliverable.columns) == ["univeresity", "world_ranking_for_data_science", + "north_america_ranking_for_data_science", "location", + "type", "chances", "acceptance_rate"] + assert audit is not None + brown = audit[audit["univeresity"] == "Brown"].iloc[0] + assert brown["__orig_world_ranking_for_data_science"] == "." + + +def test_rename_moves_the_sidecar_with_its_column(executor): + df = pd.DataFrame({"Amount": ["$5", "7"]}) + out, _ = run(executor, df, [rule("type_cast", "Amount", target_type="float"), + rule("rename", "Amount", new_name="amount_usd")]) + assert list(out.columns) == ["amount_usd", "__orig_amount_usd"] + + +# ---------------------------------------------------------------- 3. COLLEGEPOND REFERENCE +def test_section_label_under_the_header_is_read_as_a_data_row(executor, raw): + assert list(raw.columns) == HEADER + assert raw.iloc[0][U] == "COLLEGEPOND REFERENCE" + assert raw[U].tolist().count("COLLEGEPOND REFERENCE") == 1 + + +def test_profiler_also_reads_the_label_as_a_data_row(profiler): + df = profiler.load_dataframe(college_xlsx(), "xlsx") + assert list(df.columns) == HEADER + assert df.iloc[0][U] == "COLLEGEPOND REFERENCE" + + +def test_comparison_filter_keeps_rows_whose_value_is_missing_or_unparseable(executor, raw): + # Before: num > 0 was False for "." and null, so Brown, the labels and the spacer rows + # were all removed by a rule meant to drop non-positive rankings. + out, res = run(executor, raw, [rule("filter", WR, rule_id="f", operator="gt", value=0)]) + assert len(out) == len(raw) and res[0]["rows_removed"] == 0 + df = pd.DataFrame({"r": [5, -1, None, ".", 7, 8]}) + out, res = run(executor, df, [rule("filter", "r", operator="gt", value=0)]) + assert -1 not in out["r"].tolist() and len(out) == 5 and res[0]["rows_removed"] == 1 + + +@pytest.mark.parametrize("target", ["int", "float"]) +def test_type_cast_null_values_turns_numeric_codes_into_null_and_keeps_the_row(executor, target): + df = pd.DataFrame({"score": [12, 99999, "99999", -9999.0, "N/A", 7], "id": range(6)}) + out, res = run(executor, df, [rule("type_cast", "score", target_type=target, null_values=[99999, -9999, "n/a"])]) + assert len(out) == 6 and res[0]["rows_removed"] == 0 + assert out["score"].isna().tolist() == [False, True, True, True, True, False] + assert out["__orig_score"].tolist()[1:3] == [99999, "99999"] + + +def test_type_cast_without_null_values_is_unchanged(executor): + df = pd.DataFrame({"score": [12, 99999]}) + out, _ = run(executor, df, [rule("type_cast", "score", target_type="int")]) + assert out["score"].tolist() == [12, 99999] and list(out.columns) == ["score"] diff --git a/lambdas/tests/test_contracts.py b/lambdas/tests/test_contracts.py index 01c1936..b9bf3e6 100644 --- a/lambdas/tests/test_contracts.py +++ b/lambdas/tests/test_contracts.py @@ -40,3 +40,11 @@ def test_committee_has_no_thresholds_for_unimplemented_rules(executor): def test_sidecar_prefix_matches_typescript(executor): src = (REPO / "src/lib/sidecar.ts").read_text() assert f'SIDECAR_PREFIX = "{executor.SIDECAR_PREFIX}"' in src + + +def test_placeholder_tokens_match_typescript(executor): + """The bad-cell guard flags the same placeholders in the Data PR and in the executor.""" + src = (REPO / "src/lib/rule-guard.ts").read_text() + block = src[src.index("PLACEHOLDER_TOKENS"):] + block = block[block.index("["):block.index("];")] + assert set(re.findall(r'"([^"]*)"', block)) == set(executor.PLACEHOLDER_TOKENS) diff --git a/lambdas/tests/test_executor_handler.py b/lambdas/tests/test_executor_handler.py index 99d0c83..89cc73f 100644 --- a/lambdas/tests/test_executor_handler.py +++ b/lambdas/tests/test_executor_handler.py @@ -108,6 +108,7 @@ def test_success_writes_clean_deliverable_records_results_and_purges_raw(executo results = [json.loads(p[0]) for q, p in cur.sql if q.startswith("UPDATE transform_rules")] assert results[0]["_execution"]["applied"] is True assert results[1]["_execution"]["applied"] is False + assert all(r["_execution"]["rows_removed"] == 0 for r in results) assert any("SET status = 'completed'" in q for q in _statuses(cur)) # all versions + delete markers under the run prefix (incl. extracted_text.txt) deleted deleted = s3.delete_objects.call_args.kwargs["Delete"]["Objects"] @@ -168,13 +169,24 @@ def test_every_record_in_a_batch_is_processed(executor): def test_audit_file_written_when_sidecars_exist(executor): cur = FakeCursor() - cur.rules = [("rule-1", "type_cast", "b", {"target_type": "float"})] + cur.rules = [("rule-1", "type_cast", "a", {"target_type": "float"})] # " x " -> null: lossy s3 = _s3() with mock.patch.object(executor, "get_db_conn", return_value=FakeConn(cur)), mock.patch.object(executor, "s3", s3): executor.handler(_event(), None) puts = {c.kwargs["Key"]: c.kwargs["Body"] for c in s3.put_object.call_args_list} assert b"__orig_" not in puts["processed/pipe-1/run-1/output.csv"] - assert b"__orig_b" in puts["processed/pipe-1/run-1/audit.csv"] + assert b"__orig_a" in puts["processed/pipe-1/run-1/audit.csv"] + + +def test_no_audit_file_for_a_lossless_cast(executor): + cur = FakeCursor() + cur.rules = [("rule-1", "type_cast", "b", {"target_type": "float"})] # 1, 2 -> 1.0, 2.0 + s3 = _s3() + with mock.patch.object(executor, "get_db_conn", return_value=FakeConn(cur)), mock.patch.object(executor, "s3", s3): + executor.handler(_event(), None) + keys = [c.kwargs["Key"] for c in s3.put_object.call_args_list] + assert "processed/pipe-1/run-1/output.csv" in keys + assert "processed/pipe-1/run-1/audit.csv" not in keys def test_purge_falls_back_to_current_versions_when_listing_denied(executor): diff --git a/lambdas/tests/test_transforms.py b/lambdas/tests/test_transforms.py index 4e277a2..f606a61 100644 --- a/lambdas/tests/test_transforms.py +++ b/lambdas/tests/test_transforms.py @@ -110,7 +110,8 @@ def test_failed_rule_is_reported_and_frame_restored(executor): assert list(out.columns) == ["qty"] # no stray sidecar from the half-applied rule assert out["qty"].tolist() == ["1", "2.5", "3"] assert res == [{"id": "r1", "rule_type": "type_cast", "column_name": "qty", - "applied": False, "reason": res[0]["reason"]}] + "applied": False, "reason": res[0]["reason"], "rows_removed": 0, + "bad_cell_rows": 0}] assert res[0]["reason"].startswith("error:") diff --git a/src/app/api/auto-validate/[runId]/route.test.ts b/src/app/api/auto-validate/[runId]/route.test.ts index bbe3e18..26012b4 100644 --- a/src/app/api/auto-validate/[runId]/route.test.ts +++ b/src/app/api/auto-validate/[runId]/route.test.ts @@ -66,4 +66,26 @@ describe("POST /api/auto-validate/[runId]", () => { const fail = queryOne.mock.calls.find((c) => String(c[0]).includes("SET status = 'failed'"))!; expect(fail[1]).toEqual([RUN, "queued", body.error]); }); + + it("never auto-approves a rule the bad-cell guard flagged, even if every judge approves", async () => { + query.mockResolvedValue([ + { id: "r1", rule_type: "trim_whitespace", column_name: "name", ai_reasoning: "x", parameters: {} }, + { id: "r2", rule_type: "filter", column_name: "score", ai_reasoning: "y", + parameters: { operator: "neq", value: ".", _guard: { reason: "one bad cell", values: ["."] } } }, + ]); + generateText.mockResolvedValue({ + text: JSON.stringify({ votes: [ + { rule_id: "r1", vote: "APPROVE", reason: "ok" }, + { rule_id: "r2", vote: "APPROVE", reason: "ok" }, + ] }), + usage: { inputTokens: 1, outputTokens: 1 }, + }); + const res = await call(); + const body = await res.json(); + expect(body.approved).toBe(1); + expect(body.details.approved).toEqual(["r1"]); + const reject = queryOne.mock.calls.find((c) => String(c[0]).includes("status = 'rejected'"))!; + expect(reject[1][0]).toBe("r2"); + expect(JSON.parse(reject[1][1])._reject_reasons[0]).toBe("Guard: one bad cell"); + }); }); diff --git a/src/app/api/auto-validate/[runId]/route.ts b/src/app/api/auto-validate/[runId]/route.ts index a3942a5..2d84d05 100644 --- a/src/app/api/auto-validate/[runId]/route.ts +++ b/src/app/api/auto-validate/[runId]/route.ts @@ -11,6 +11,7 @@ import type { TransformRule, DataProfile } from "@/lib/types"; import { requireEnv, optionalEnv, awsRegion } from "@/lib/env"; import { logger } from "@/lib/logger"; import { EXECUTOR_QUEUE_FAILED_MESSAGE, failRunAfterQueueError, queueErrorResponse } from "@/lib/queue-failure"; +import { ruleGuardOf } from "@/lib/rule-guard"; const log = logger.child({ route: "POST /api/auto-validate" }); @@ -244,7 +245,12 @@ ${responseFormat}`, .filter((v) => v?.vote === "REJECT") .map((v) => v!.reason); - if (approveCount >= threshold) { + // Rules the deterministic guard flagged (src/lib/rule-guard.ts) are never + // auto-approved: they would drop rows only because of one bad cell. + const guard = ruleGuardOf(rule.parameters); + if (guard) { + rejected.push({ id: rule.id, reasons: [`Guard: ${guard.reason}`, ...rejectReasons] }); + } else if (approveCount >= threshold) { approved.push(rule.id); } else { rejected.push({ id: rule.id, reasons: rejectReasons }); diff --git a/src/app/api/suggest-transforms/route.test.ts b/src/app/api/suggest-transforms/route.test.ts new file mode 100644 index 0000000..215af94 --- /dev/null +++ b/src/app/api/suggest-transforms/route.test.ts @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { NextRequest } from "next/server"; + +const query = vi.fn(); +const queryOne = vi.fn(); +const generateText = vi.fn(); + +vi.mock("@/lib/db", () => ({ + query: (...a: unknown[]) => query(...a), + queryOne: (...a: unknown[]) => queryOne(...a), +})); +vi.mock("ai", () => ({ + generateText: (...a: unknown[]) => generateText(...a), + Output: { object: (o: unknown) => o }, +})); +vi.mock("@/lib/ai-model", () => ({ languageModel: () => ({}) })); +vi.mock("@/lib/bedrock-meter", () => ({ + checkAiBudget: async () => ({ ok: true }), + meterBedrockCall: async () => {}, +})); +vi.mock("@/lib/rate-limit", () => ({ aiLimiter: {}, checkRateLimit: async () => null })); +vi.mock("@/lib/billing", () => ({ checkQuota: async () => ({ blocked: false }), quotaBlockedMessage: () => "" })); +vi.mock("@/lib/auth", () => ({ userEmailById: async () => "a@b.c" })); + +const SECRET = "s".repeat(40); +process.env.WEBHOOK_SECRET = SECRET; +const { POST } = await import("./route"); + +const RUN = "11111111-2222-3333-4444-555555555555"; +const call = () => + POST(new NextRequest("http://x/api/suggest-transforms", { + method: "POST", + headers: { "x-webhook-secret": SECRET, "content-type": "application/json" }, + body: JSON.stringify({ run_id: RUN }), + })); + +const inserted = () => + queryOne.mock.calls + .filter((c) => String(c[0]).includes("INSERT INTO transform_rules")) + .map((c) => ({ rule_type: c[1][2], column_name: c[1][3], parameters: JSON.parse(c[1][4]) })); + +describe("POST /api/suggest-transforms: bad-cell guard", () => { + beforeEach(() => { + vi.clearAllMocks(); + queryOne.mockImplementation(async (sql: string) => { + if (sql.includes("FROM pipeline_runs pr")) { + return { id: RUN, pipeline_id: "p", team_id: "user_1", mode: "tabular", template_id: null, iteration: 1, auto_mode: false }; + } + if (sql.includes("RETURNING id")) return { id: RUN }; + if (sql.includes("SELECT team_id FROM pipelines")) return { team_id: "user_1" }; + if (sql.includes("FROM data_profiles")) { + return { + total_rows: 10, + column_stats: { + score: { type: "object", null_count: 0, unique_count: 9, sample_values: [10, 20, "."], min: 10, max: 90, sentinel_examples: ["."] }, + status: { type: "object", null_count: 0, unique_count: 2, sample_values: ["active", "deleted"] }, + }, + }; + } + return null; + }); + generateText.mockResolvedValue({ + output: { rules: [ + { rule_type: "filter", column_name: "score", parameters: { operator: "neq", value: "." }, ai_reasoning: "x" }, + { rule_type: "filter", column_name: "status", parameters: { operator: "neq", value: "deleted" }, ai_reasoning: "y" }, + ] }, + usage: { inputTokens: 1, outputTokens: 1 }, + }); + }); + + it("flags a filter that removes rows for one placeholder cell and leaves a legitimate filter alone", async () => { + const res = await call(); + expect(res.status).toBe(200); + const [placeholder, legit] = inserted(); + expect(placeholder.parameters._guard.values).toEqual(["."]); + expect(placeholder.parameters._guard.alternative.parameters).toEqual({ target_type: "float", null_values: ["."] }); + expect(placeholder.parameters.operator).toBe("neq"); // the rule itself is unchanged + expect(legit.parameters).toEqual({ operator: "neq", value: "deleted" }); + }); +}); diff --git a/src/app/api/suggest-transforms/route.ts b/src/app/api/suggest-transforms/route.ts index 6eb4315..ffa5078 100644 --- a/src/app/api/suggest-transforms/route.ts +++ b/src/app/api/suggest-transforms/route.ts @@ -14,6 +14,7 @@ import { aiLimiter, checkRateLimit } from "@/lib/rate-limit"; export const maxDuration = 300; import { z } from "zod"; import { queryOne, query } from "@/lib/db"; +import { applyRuleGuard } from "@/lib/rule-guard"; import type { DataProfile, PipelineRun, PipelineTemplate, TemplateRule } from "@/lib/types"; import { requireEnv, optionalEnv } from "@/lib/env"; import { logger } from "@/lib/logger"; @@ -505,8 +506,8 @@ For every column in the profile, work through ALL of the following checks. Do no → filter with operator "gt", value "0" (or appropriate threshold) - Ratings/scores outside valid range (e.g. rating = -1, rating = 99 when scale is 0–5)? → filter with operator "gt"/"lt" as needed - - Sentinel/placeholder values like 99999, 9999, -9999 in numeric columns? - → filter to exclude them + - Sentinel/placeholder values (e.g. "N/A", "-", "?", 99999, -9999) in a mostly-numeric column? + → type_cast to "int"/"float" so they become null and the row is kept; list numeric codes in "null_values" (e.g. {"target_type": "int", "null_values": [99999]}). Filter the row only if the whole row is invalid. - Future dates in a "created_at" or "signup_date" column that should be historical? → filter with operator "lt" and today's date - When suggesting outlier_cap for a column with a known valid domain range, ALWAYS include explicit min_val and/or max_val in parameters instead of relying solely on IQR. @@ -689,8 +690,12 @@ For each rule, write ai_reasoning as one precise sentence that references the sp return NextResponse.json({ error: "No rules generated" }, { status: 500 }); } + // Deterministic check before the rules are shown: flag row-removing rules that + // would drop rows only because of one bad cell (src/lib/rule-guard.ts). + const guardedRules = applyRuleGuard(output.rules, columnStats); + await Promise.all( - output.rules.map((rule, idx) => + guardedRules.map((rule, idx) => queryOne( `INSERT INTO transform_rules (pipeline_id, run_id, rule_type, column_name, parameters, ai_reasoning, status, order_index) diff --git a/src/app/pipelines/[id]/runs/[rid]/page.tsx b/src/app/pipelines/[id]/runs/[rid]/page.tsx index 2d69329..e663689 100644 --- a/src/app/pipelines/[id]/runs/[rid]/page.tsx +++ b/src/app/pipelines/[id]/runs/[rid]/page.tsx @@ -16,7 +16,7 @@ import { import Nav from "@/components/Nav"; import { queryOne, query } from "@/lib/db"; import type { PipelineRun, DataProfile, TransformRule } from "@/lib/types"; -import { isApprovedButNotApplied, ruleExecution } from "@/lib/types"; +import { badCellRowsByRules, isApprovedButNotApplied, ruleExecution, rowsRemovedByRules } from "@/lib/types"; import QualityGauge from "@/components/QualityGauge"; import ColumnStatsTable from "@/components/ColumnStatsTable"; import QualityTrendChart from "@/components/QualityTrendChart"; @@ -103,6 +103,8 @@ export default async function RunDetailPage({ ]); const canDownload = run.status === "completed" && !!run.processed_s3_key; + const rowsRemoved = rowsRemovedByRules(rules); + const badCells = badCellRowsByRules(rules); // % improvement = (this_pass_processed - parent_processed) / parent_processed * 100 const iterationImprovement = @@ -233,6 +235,24 @@ export default async function RunDetailPage({
{Number(run.row_count_processed).toLocaleString()}
)} + {run.mode !== "document" && rowsRemoved > 0 && ( +
+
Rows removed by rules
+
{rowsRemoved.toLocaleString()}
+
+ )} + {run.mode !== "document" && badCells.removed > 0 && ( +
+
Removed for one bad cell
+
{badCells.removed.toLocaleString()}
+
+ )} + {run.mode !== "document" && badCells.kept > 0 && ( +
+
Kept by bad-cell guard
+
{badCells.kept.toLocaleString()}
+
+ )} {canDownload && ( @@ -380,6 +400,7 @@ export default async function RunDetailPage({ {rule.ai_reasoning && (

{rule.ai_reasoning}

)} + {isApprovedButNotApplied(rule) && (

Not applied{ruleExecution(rule.parameters)?.reason ? `: ${ruleExecution(rule.parameters)?.reason}` : ""} @@ -453,3 +474,17 @@ export default async function RunDetailPage({ ); } + +/** "Removed N rows" under a rule that removed rows, so no row disappears silently. */ +function RowsRemovedNote({ parameters }: { parameters: Record | null }) { + const exec = ruleExecution(parameters); + const removed = exec?.rows_removed ?? 0; + const bad = exec?.applied ? exec.bad_cell_rows ?? 0 : 0; + if (removed <= 0) return null; + return ( +

+ Removed {removed.toLocaleString()} {removed === 1 ? "row" : "rows"} + {bad > 0 && `, ${bad.toLocaleString()} of them only because of one bad cell (a placeholder or non-number)`} +

+ ); +} diff --git a/src/app/pipelines/[id]/runs/[rid]/review/ReviewClient.tsx b/src/app/pipelines/[id]/runs/[rid]/review/ReviewClient.tsx index 395d930..0334399 100644 --- a/src/app/pipelines/[id]/runs/[rid]/review/ReviewClient.tsx +++ b/src/app/pipelines/[id]/runs/[rid]/review/ReviewClient.tsx @@ -15,6 +15,7 @@ import Nav from "@/components/Nav"; import PRHeader from "@/components/DataPR/PRHeader"; import RuleCard, { type RuleDecision } from "@/components/DataPR/RuleCard"; import type { PipelineRun, TransformRule } from "@/lib/types"; +import { ruleGuardOf } from "@/lib/rule-guard"; interface ReviewClientProps { pipelineId: string; @@ -32,8 +33,18 @@ export default function ReviewClient({ pipelineId, run, rules }: ReviewClientPro setDecisions((prev) => new Map(prev).set(d.rule_id, d)); }, []); + // Rules flagged by the bad-cell guard are left out of "Approve all": each one + // needs its own decision (src/lib/rule-guard.ts). + const flaggedCount = rules.filter((r) => ruleGuardOf(r.parameters)).length; function approveAll() { - setDecisions(new Map(rules.map((r) => [r.id, { rule_id: r.id, action: "approved" as const, modifications: null }]))); + setDecisions((prev) => { + const next = new Map(prev); + for (const r of rules) { + if (ruleGuardOf(r.parameters)) continue; + next.set(r.id, { rule_id: r.id, action: "approved" as const, modifications: null }); + } + return next; + }); } function rejectAll() { setDecisions(new Map(rules.map((r) => [r.id, { rule_id: r.id, action: "rejected" as const, modifications: null }]))); @@ -115,7 +126,7 @@ export default function ReviewClient({ pipelineId, run, rules }: ReviewClientPro className="inline-flex items-center gap-1.5 px-3 py-1.5 border border-green-500/40 text-green-400 hover:bg-green-500/10 hover:border-green-500/60 text-sm font-medium rounded-lg transition-colors" >