Rifqi Hafizuddin Claude Fable 5 commited on
Commit
74ad526
·
1 Parent(s): 089b69d

[KM-644] harden query path: grouped-select rule, int-date trends, sample decode, trace guard

Browse files

IRValidator rejects bare selects under group_by (planner retry now
self-corrects the Postgres GroupingError class); analyze_trend handles
integer year/month columns (was collapsing all rows into 1970-01);
catalog/sample_decode.py base64-decodes numeric sample_values at read
(self-disabling stopgap for Go's byte marshaling, 139 values on the live
catalog); traceability skips null db-sources on failed retrievals;
check_data hides -1 row counts. Docs: REPO_STATUS delta + DEV_PLAN §0.5.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

DEV_PLAN.md CHANGED
@@ -46,6 +46,26 @@ minter, stream-only). The **Phase 3 traceability build** — scratchpad + `GET /
46
 
47
  ---
48
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
49
  ## 1. The direction change (locked decisions from 2026-06-24)
50
 
51
  1. **"Problem statement" is replaced by two user-entered fields: `objective` + `business_questions`.**
 
46
 
47
  ---
48
 
49
+ ## 0.5. pr/13 sprint — agent-quality fixes (2026-07-08 live-test review)
50
+
51
+ Findings from the scoped live sessions (mining analysis, 2026-07-07/08 traces): the planner
52
+ force-mapped absent measures (`pa` aliased as "revenue"), top-N ranked raw rows (duplicate models),
53
+ `analyze_trend` collapsed integer months into a single 1970-01 bucket, an invalid grouped IR reached
54
+ Postgres, failed retrievals wrote all-null traceability sources, and numeric catalog samples arrive
55
+ base64-mangled from Go. Fix tasks (same status legend as §0):
56
+
57
+ | # | Task | Owner | Status | Note |
58
+ |---|---|---|---|---|
59
+ | Q1 | IR validator: reject bare selects under `group_by` (planner retry self-corrects) | Rifqi | ✅ | `query/ir/validator.py` |
60
+ | Q2 | Planner **infeasible** path: `TaskList.infeasible_reason` + deterministic EN/ID data-gap reply | Rifqi | ✅ | schemas/validator/coordinator/refusals + planner.md "When the catalog cannot answer"; refusal wording → Rifqi to review |
61
+ | Q3 | `analyze_trend`: integer year/month handling (epoch-parse bug) | Rifqi | ✅ | `temporal.py` + 5 local tests |
62
+ | Q4 | Planner few-shots: top-N (Example G) + infeasible (Example H) + entity-vs-row ranking rule | Rifqi | ✅ | live-tested 2026-07-08: backlog top-3 correct via single-IR group+sum; "best PA performance" correct in-process (avg-per-model, assumption recorded). Stale-server trace was a false alarm |
63
+ | Q5 | Catalog numeric `sample_values` base64-decode stopgap (`catalog/sample_decode.py`) | Rifqi | ✅ | self-disabling; **primary fix = Go marshaling — DDL-free handoff to Harry** |
64
+ | Q6 | Traceability null-source suppression + `check_data` `-1` row-count hiding | Rifqi | ✅ | `scratchpad.py` / `data_access.py` |
65
+ | Q7 | `analyze_merge` two-table combine tool (unblocks "worst A + biggest B" questions) | tool owner | ⬜ | flagged out of this sprint; request brief sent by Rifqi |
66
+ | Q8 | Report v2: business-question answer section, unresolved/excluded sections, evidence tables from `results_snapshot`, caveat dedupe, single language | Rifqi/Sofhia | ⬜ | next up; adds one LLM call (button-triggered); new prompt → eval per §7B |
67
+ | Q9 | Record-curation endpoint (`GET …/records` + `exclude_record_ids`) + readiness GET for the FE delta guard | Rifqi ↔ FE | ⬜ | contract addition → API_CONTRACT_BE_PYTHON.md |
68
+
69
  ## 1. The direction change (locked decisions from 2026-06-24)
70
 
71
  1. **"Problem statement" is replaced by two user-entered fields: `objective` + `business_questions`.**
REPO_STATUS.md CHANGED
@@ -2,7 +2,7 @@
2
 
3
  **Audience:** teammates onboarding onto the Python repo (`Agentic-Service-Data-Eyond-Catalog`).
4
  **Scope:** what the code does **right now** (branch `pr/4`, ticket KM-652). Describes current state only — no roadmap or to-dos.
5
- **Snapshot date:** 2026-06-25. **Data-layer reconcile 2026-07-01:** §8/§12 updated — dedorch cutover done, `data_catalog` model reconciled. **Query-path fix 2026-07-02:** §8/§13 — dedorch catalogs ship no FKs → Python infers them (`fk_inference.py`); shared-Fernet-key gotcha documented. **Cross-repo update 2026-06-29:** §2/§8/§11/§12 re-verified against
6
  the **Go source** (`Orchestrator-Agent-Service`), not its docs. The Go service has moved well past its
7
  own (uncommitted, stale) design docs: it now hosts the **dedorch SQL migrations** in-repo and a full
8
  **`/api/v1/analyses` + `/api/v1/skills`** REST surface. Go does **not** call Python yet — those skills
 
2
 
3
  **Audience:** teammates onboarding onto the Python repo (`Agentic-Service-Data-Eyond-Catalog`).
4
  **Scope:** what the code does **right now** (branch `pr/4`, ticket KM-652). Describes current state only — no roadmap or to-dos.
5
+ **Snapshot date:** 2026-06-25. **Data-layer reconcile 2026-07-01:** §8/§12 updated — dedorch cutover done, `data_catalog` model reconciled. **Query-path fix 2026-07-02:** §8/§13 — dedorch catalogs ship no FKs → Python infers them (`fk_inference.py`); shared-Fernet-key gotcha documented. **Agent-quality fixes 2026-07-08 (pr/13):** from the scoped live-test review — the planner gains an explicit **infeasible** outcome (`TaskList.infeasible_reason` → deterministic EN/ID data-gap reply via `refusals.data_gap_message`; no more force-mapping absent measures like `pa` AS "revenue"), the IR validator rejects bare selects under `group_by` (self-corrects via the planner retry), `analyze_trend` handles integer year/month columns (was collapsing every row into one 1970-01 bucket), planner few-shots add top-N (Example G) + infeasible (Example H), numeric catalog `sample_values` are base64-decoded at read (`catalog/sample_decode.py` — stopgap for Go's byte-marshaling; primary fix is Go-side), traceability no longer emits null source rows for failed retrievals, and `check_data` hides `-1` row counts. **Cross-repo update 2026-06-29:** §2/§8/§11/§12 re-verified against
6
  the **Go source** (`Orchestrator-Agent-Service`), not its docs. The Go service has moved well past its
7
  own (uncommitted, stale) design docs: it now hosts the **dedorch SQL migrations** in-repo and a full
8
  **`/api/v1/analyses` + `/api/v1/skills`** REST surface. Go does **not** call Python yet — those skills
src/catalog/sample_decode.py ADDED
@@ -0,0 +1,119 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ """Base64-sample-value decoding for catalogs affected by a dedorch bug.
2
+
3
+ Go's introspection JSON-marshals numeric sample bytes as base64 (a `[]byte`
4
+ serialization quirk), so every decimal/int-typed column's `sample_values`
5
+ currently arrive as base64 strings (e.g. ``'OTUuMA=='`` for ``"95.0"``)
6
+ instead of the plain numeric text the planner LLM expects. The planner then
7
+ sees gibberish instead of value ranges for exactly the columns it filters and
8
+ aggregates on. Until Go fixes the marshaling, decode these at catalog read
9
+ time.
10
+
11
+ Conservative by design (a wrong decode silently corrupts planner context):
12
+ - only numeric-typed columns are considered
13
+ - every non-null sample in the column must pass a strict base64 gate
14
+ (valid base64, decodes to printable ASCII, parses as a float) — a single
15
+ non-conforming entry leaves the WHOLE column untouched (mixed content is
16
+ suspicious, never guessed)
17
+ - columns with `sample_values is None` (e.g. PII-flagged columns, which
18
+ carry no samples by design) are skipped cleanly
19
+ - self-disabling: once Go ships real numeric samples (plain ``"95.0"`` or
20
+ actual numbers), the gate fails — plain digit strings are either not
21
+ base64-padded correctly or don't decode to printable numeric text — so
22
+ the pass becomes a no-op with no further changes needed here
23
+ """
24
+
25
+ from __future__ import annotations
26
+
27
+ import base64
28
+ import binascii
29
+
30
+ from src.middlewares.logging import get_logger
31
+
32
+ from .models import Catalog
33
+
34
+ logger = get_logger("sample_decode")
35
+
36
+ _NUMERIC_TYPES = {
37
+ "int",
38
+ "integer",
39
+ "bigint",
40
+ "decimal",
41
+ "numeric",
42
+ "float",
43
+ "double",
44
+ "number",
45
+ }
46
+
47
+
48
+ def _decode_one(value: str) -> str | None:
49
+ """Return the decoded numeric text for `value`, or None if it fails the gate."""
50
+ if len(value) < 2 or len(value) % 4 != 0:
51
+ return None
52
+ try:
53
+ decoded = base64.b64decode(value, validate=True)
54
+ except (binascii.Error, ValueError):
55
+ return None
56
+ try:
57
+ text = decoded.decode("ascii")
58
+ except UnicodeDecodeError:
59
+ return None
60
+ if not text.isprintable():
61
+ return None
62
+ try:
63
+ float(text)
64
+ except ValueError:
65
+ return None
66
+ return text
67
+
68
+
69
+ def _decode_column_samples(samples: list) -> tuple[list, int] | None:
70
+ """Return (decoded list, count decoded) if every non-null entry passes the gate.
71
+
72
+ Returns None if any entry fails the gate (mixed content is left untouched).
73
+ """
74
+ decoded_values = []
75
+ count = 0
76
+ for entry in samples:
77
+ if entry is None:
78
+ decoded_values.append(None)
79
+ continue
80
+ if not isinstance(entry, str):
81
+ return None
82
+ decoded = _decode_one(entry)
83
+ if decoded is None:
84
+ return None
85
+ count += 1
86
+ decoded_values.append(decoded)
87
+ if not count:
88
+ return None
89
+ return decoded_values, count
90
+
91
+
92
+ def decode_sample_values(catalog: Catalog) -> int:
93
+ """Decode base64-encoded numeric sample_values in place. Returns count decoded.
94
+
95
+ Never raises: any unexpected shape (wrong types, malformed entries) leaves
96
+ the offending column's values untouched.
97
+ """
98
+ total = 0
99
+ try:
100
+ for source in catalog.sources:
101
+ for table in source.tables:
102
+ for col in table.columns:
103
+ if col.data_type.lower() not in _NUMERIC_TYPES:
104
+ continue
105
+ samples = col.sample_values
106
+ if not samples:
107
+ continue
108
+ result = _decode_column_samples(samples)
109
+ if result is None:
110
+ continue
111
+ decoded_values, count = result
112
+ col.sample_values = decoded_values
113
+ total += count
114
+ except Exception as e:
115
+ logger.error("sample decode failed", error=repr(e))
116
+ return total
117
+ if total:
118
+ logger.info("decoded base64 sample values", user_id=catalog.user_id, count=total)
119
+ return total
src/catalog/store.py CHANGED
@@ -15,6 +15,7 @@ from src.middlewares.logging import get_logger
15
 
16
  from .fk_inference import infer_foreign_keys
17
  from .models import Catalog
 
18
 
19
  logger = get_logger("catalog_store")
20
 
@@ -41,7 +42,12 @@ class CatalogStore:
41
  # dedorch catalogs ship no foreign_keys (Go introspection drops them),
42
  # but the IR validator only allows FK-backed joins. Infer the obvious
43
  # edges so the planner and validator agree. No-op once Go emits real FKs.
44
- return infer_foreign_keys(Catalog.model_validate(row))
 
 
 
 
 
45
 
46
  async def get_by_analysis(self, analysis_id: str) -> Catalog | None:
47
  """Read the `scope_type='analysis'` catalog row for an analysis.
@@ -63,7 +69,9 @@ class CatalogStore:
63
  row = result.scalar_one_or_none()
64
  if row is None:
65
  return None
66
- return infer_foreign_keys(Catalog.model_validate(row))
 
 
67
 
68
  async def upsert(self, catalog: Catalog) -> None:
69
  # Legacy: Go's catalog.Service owns catalog writes now. Kept working (and
 
15
 
16
  from .fk_inference import infer_foreign_keys
17
  from .models import Catalog
18
+ from .sample_decode import decode_sample_values
19
 
20
  logger = get_logger("catalog_store")
21
 
 
42
  # dedorch catalogs ship no foreign_keys (Go introspection drops them),
43
  # but the IR validator only allows FK-backed joins. Infer the obvious
44
  # edges so the planner and validator agree. No-op once Go emits real FKs.
45
+ catalog = infer_foreign_keys(Catalog.model_validate(row))
46
+ # dedorch also JSON-marshals numeric sample bytes as base64 (Go bug) —
47
+ # decode them so the planner sees value ranges, not gibberish.
48
+ # No-op once Go emits plain numeric samples.
49
+ decode_sample_values(catalog)
50
+ return catalog
51
 
52
  async def get_by_analysis(self, analysis_id: str) -> Catalog | None:
53
  """Read the `scope_type='analysis'` catalog row for an analysis.
 
69
  row = result.scalar_one_or_none()
70
  if row is None:
71
  return None
72
+ catalog = infer_foreign_keys(Catalog.model_validate(row))
73
+ decode_sample_values(catalog)
74
+ return catalog
75
 
76
  async def upsert(self, catalog: Catalog) -> None:
77
  # Legacy: Go's catalog.Service owns catalog writes now. Kept working (and
src/query/ir/validator.py CHANGED
@@ -87,6 +87,22 @@ class IRValidator:
87
  for i, col_id in enumerate(ir.group_by):
88
  self._require_column(columns_by_id, col_id, f"group_by[{i}]")
89
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
90
  for i, ob in enumerate(ir.order_by):
91
  if ob.column_id not in columns_by_id and ob.column_id not in select_aliases:
92
  raise IRValidationError(
 
87
  for i, col_id in enumerate(ir.group_by):
88
  self._require_column(columns_by_id, col_id, f"group_by[{i}]")
89
 
90
+ # A grouped query must not select bare columns that aren't in group_by —
91
+ # the database rejects it only at execution ("must appear in the GROUP BY
92
+ # clause"), which is past the planner's corrective-retry window. Catching
93
+ # it here turns a failed turn into a self-correcting re-prompt.
94
+ if ir.group_by:
95
+ grouped = set(ir.group_by)
96
+ for i, item in enumerate(ir.select):
97
+ if item.kind == "column" and item.column_id not in grouped:
98
+ raise IRValidationError(
99
+ f"select[{i}].column_id {item.column_id!r} is selected bare "
100
+ "while group_by is present — every selected column must "
101
+ "either appear in group_by or be wrapped in an aggregate "
102
+ f'(e.g. {{"kind": "agg", "fn": "sum", '
103
+ f'"column_id": {item.column_id!r}}})'
104
+ )
105
+
106
  for i, ob in enumerate(ir.order_by):
107
  if ob.column_id not in columns_by_id and ob.column_id not in select_aliases:
108
  raise IRValidationError(
src/tools/analytics/temporal.py CHANGED
@@ -41,6 +41,11 @@ class UnsupportedAggregationError(ValueError):
41
  """The requested aggregation is not supported (maps to error_code UNSUPPORTED_AGG)."""
42
 
43
 
 
 
 
 
 
44
  def _clean(value: object) -> object:
45
  """Convert numpy scalars to plain Python; NaN -> None for JSON-clean output."""
46
  if value is None:
@@ -53,6 +58,63 @@ def _clean(value: object) -> object:
53
  return value
54
 
55
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
56
  def _period_label(ts: pd.Timestamp, freq: str) -> str:
57
  """Human-readable period label keyed off the friendly frequency name."""
58
  if freq == "month":
@@ -119,6 +181,8 @@ def analyze_trend(
119
  ColumnNotFoundError: if date_column or value_column is absent.
120
  InvalidFrequencyError: if freq is not a known period.
121
  UnsupportedAggregationError: if agg is not supported.
 
 
122
  """
123
  missing = [c for c in (date_column, value_column) if c not in df.columns]
124
  if missing:
@@ -134,7 +198,7 @@ def analyze_trend(
134
 
135
  # Build a clean datetime-indexed series, then resample into periods.
136
  s = df[[date_column, value_column]].copy()
137
- s[date_column] = pd.to_datetime(s[date_column])
138
  s = s.dropna(subset=[date_column]).set_index(date_column).sort_index()
139
  resampled = s[value_column].resample(FREQ_MAP[freq]).agg(agg)
140
 
 
41
  """The requested aggregation is not supported (maps to error_code UNSUPPORTED_AGG)."""
42
 
43
 
44
+ class InvalidDateColumnError(ValueError):
45
+ """date_column holds numeric values that aren't a recognizable date/year/month
46
+ (maps to error_code INVALID_DATE_COLUMN)."""
47
+
48
+
49
  def _clean(value: object) -> object:
50
  """Convert numpy scalars to plain Python; NaN -> None for JSON-clean output."""
51
  if value is None:
 
58
  return value
59
 
60
 
61
+ def _parse_date_column(df: pd.DataFrame, date_column: str) -> pd.Series:
62
+ """Parse date_column into datetimes, guarding against numeric epoch misparsing.
63
+
64
+ pd.to_datetime() treats bare numeric input as epoch-nanoseconds, so bare
65
+ month numbers (1-12) or calendar years (e.g. 2025) silently collapse to a
66
+ single 1970 timestamp instead of raising. Numeric columns are resolved
67
+ explicitly here rather than falling through to pd.to_datetime().
68
+ """
69
+ col = df[date_column]
70
+ if not pd.api.types.is_numeric_dtype(col):
71
+ return pd.to_datetime(col)
72
+
73
+ non_null = col.dropna()
74
+ is_whole = non_null.empty or (non_null == non_null.astype(int)).all()
75
+
76
+ if is_whole and non_null.between(1, 12).all():
77
+ year_col = next((c for c in df.columns if c.lower() == "year"), None)
78
+ year_series = df[year_col] if year_col is not None else None
79
+ year_non_null = year_series.dropna() if year_series is not None else pd.Series(dtype=float)
80
+ year_ok = (
81
+ year_series is not None
82
+ and pd.api.types.is_numeric_dtype(year_series)
83
+ and not year_non_null.empty
84
+ and (year_non_null == year_non_null.astype(int)).all()
85
+ and year_non_null.between(1900, 2100).all()
86
+ )
87
+ if not year_ok:
88
+ raise InvalidDateColumnError(
89
+ f"date_column '{date_column}' holds bare month numbers (1-12) and no "
90
+ "'year' column is present in the data — retrieve a year column "
91
+ "alongside month, or use a real date column."
92
+ )
93
+ valid = col.notna() & year_series.notna()
94
+ result = pd.Series(pd.NaT, index=col.index, dtype="datetime64[ns]")
95
+ result.loc[valid] = pd.to_datetime(
96
+ {
97
+ "year": year_series.loc[valid].astype(int),
98
+ "month": col.loc[valid].astype(int),
99
+ "day": 1,
100
+ }
101
+ )
102
+ return result
103
+
104
+ if is_whole and non_null.between(1900, 2100).all():
105
+ result = pd.Series(pd.NaT, index=col.index, dtype="datetime64[ns]")
106
+ valid = col.notna()
107
+ result.loc[valid] = pd.to_datetime(
108
+ col.loc[valid].astype(int).astype(str), format="%Y"
109
+ )
110
+ return result
111
+
112
+ raise InvalidDateColumnError(
113
+ f"date_column '{date_column}' is numeric but is not a recognizable date, "
114
+ "year, or month column."
115
+ )
116
+
117
+
118
  def _period_label(ts: pd.Timestamp, freq: str) -> str:
119
  """Human-readable period label keyed off the friendly frequency name."""
120
  if freq == "month":
 
181
  ColumnNotFoundError: if date_column or value_column is absent.
182
  InvalidFrequencyError: if freq is not a known period.
183
  UnsupportedAggregationError: if agg is not supported.
184
+ InvalidDateColumnError: if date_column is numeric but not a recognizable
185
+ date, year, or bare month number (needing a companion 'year' column).
186
  """
187
  missing = [c for c in (date_column, value_column) if c not in df.columns]
188
  if missing:
 
198
 
199
  # Build a clean datetime-indexed series, then resample into periods.
200
  s = df[[date_column, value_column]].copy()
201
+ s[date_column] = _parse_date_column(df, date_column)
202
  s = s.dropna(subset=[date_column]).set_index(date_column).sort_index()
203
  resampled = s[value_column].resample(FREQ_MAP[freq]).agg(agg)
204
 
src/tools/data_access.py CHANGED
@@ -154,7 +154,9 @@ class DataAccessToolInvoker:
154
  [
155
  t.table_id,
156
  t.name,
157
- t.row_count,
 
 
158
  c.column_id,
159
  c.name,
160
  c.data_type,
 
154
  [
155
  t.table_id,
156
  t.name,
157
+ # dedorch catalogs mark an uncounted table as -1; surface None so
158
+ # the planner prompt never sees a nonsensical "-1 rows".
159
+ t.row_count if (t.row_count or 0) >= 0 else None,
160
  c.column_id,
161
  c.name,
162
  c.data_type,
src/traceability/scratchpad.py CHANGED
@@ -124,13 +124,17 @@ class TraceabilityScratchpad:
124
  error=out_dict.get("error"),
125
  )
126
  )
127
- if name == "retrieve_data":
128
  self._record_db_source(output)
129
 
130
  def _record_db_source(self, output: Any) -> None:
131
  # retrieve_data's args are {"ir": ...}; the reliable source_id/table/query
132
  # live on the tool OUTPUT meta (see tools/data_access.py::_retrieve_data).
133
  meta = _meta_of(output)
 
 
 
 
134
  query = meta.get("query")
135
  table = meta.get("table_name") or meta.get("table_id")
136
  self._db_sources.append({
 
124
  error=out_dict.get("error"),
125
  )
126
  )
127
+ if name == "retrieve_data" and status == "success":
128
  self._record_db_source(output)
129
 
130
  def _record_db_source(self, output: Any) -> None:
131
  # retrieve_data's args are {"ir": ...}; the reliable source_id/table/query
132
  # live on the tool OUTPUT meta (see tools/data_access.py::_retrieve_data).
133
  meta = _meta_of(output)
134
+ if not meta.get("source_id"):
135
+ # A failed/aborted retrieval carries no provenance meta — emitting it
136
+ # anyway produced all-null source rows in the payload.
137
+ return
138
  query = meta.get("query")
139
  table = meta.get("table_name") or meta.get("table_id")
140
  self._db_sources.append({