Rifqi Hafizuddin Claude Opus 4.8 commited on
Commit
60887f6
·
1 Parent(s): 5e12ec2

[NOTICKET] fix: F-9 — redact PII in the two persisted artifacts

Browse files

`pii_flag` was an ingestion-time control only. It nulls `sample_values` going into the
planner prompt, and REPO_STATUS §8 claims "real values never enter prompts" — true of
samples, false of results. Nothing stops the planner SELECTing a flagged column, and
it should not: "list our top 20 customers by revenue" legitimately selects
`customer_name` and `email`.

Those real values then reached two places that KEEP them:
1. `message_traceability.data` — persisted, and served by a GET with no auth (F-3
was declined, so the service secret is the only control there).
2. Report evidence tables — rendered into `reports.content`, permanently, versioned.

`retrieve_data` now reports `meta.pii_columns`, resolved through the IR select list so
aliases are honoured, and both sinks redact those cells to "[redacted]".

Per the lead's 2026-07-23 decision the ASSEMBLER still receives the real rows, so the
answer prose is unchanged and the question stays answerable. Verified end-to-end
through the real `_retrieve_data` path: the assembler input kept "Ada Lovelace" while
the persisted preview showed "[redacted]".

Deliberate choices:
- Aggregates are NOT masked, except `min`/`max`. `sum(salary)` and `count(email)`
are derived numbers identifying nobody; masking them would destroy a legitimate
answer for no privacy gain. `min`/`max` are different — they return an actual
member value, so `max(email)` really is one customer's address.
- Column HEADERS are kept. The column name is not the secret, and keeping it is what
makes the redaction legible rather than looking like missing data. The traceability
record also carries `pii_masked` naming the affected columns.
- A fixed marker, not a partial reveal: "j***@acme.com" still leaks the domain and
the name shape.
- Fails OPEN. An output name that cannot be resolved is left unmasked rather than
blanking a legitimate column, so the worst case is today's behaviour. This is a
mitigation, not a guarantee — the guarantee would have to come from not selecting
the column, which is the product decision already taken the other way.
- `_PII_MASK` and `_pii_indexes` are imported by the report generator rather than
re-implemented; a second copy is the drift CODE_REVIEW F-27 warns about.

Older persisted rows carry no `pii_columns` key and are returned exactly as before.

15 new tests. Suite 456 passed / 0 failed / 7 skipped. Ruff clean on touched paths.
Contract updated for both surfaces.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/agents/report/generator.py CHANGED
@@ -25,6 +25,11 @@ from langchain_openai import AzureChatOpenAI
25
 
26
  from src.middlewares.logging import get_logger
27
 
 
 
 
 
 
28
  from ..language import detect_reply_language
29
  from ..slow_path.schemas import AnalysisRecord, TaskSummary
30
  from .errors import ReportError
@@ -186,12 +191,25 @@ def _collect_evidence(records: list[AnalysisRecord]) -> dict[str, list[EvidenceT
186
  continue
187
  if len(output.columns) > _EVIDENCE_MAX_COLS:
188
  continue
 
 
 
 
 
 
 
 
 
 
189
  tables.append(
190
  EvidenceTable(
191
  title=result.objective,
192
  columns=[str(c) for c in output.columns],
193
  rows=[
194
- [_fmt_cell(v) for v in row]
 
 
 
195
  for row in output.rows[:_EVIDENCE_MAX_ROWS]
196
  ],
197
  truncated=len(output.rows) > _EVIDENCE_MAX_ROWS,
 
25
 
26
  from src.middlewares.logging import get_logger
27
 
28
+ # Reused, not re-implemented: the traceability preview and the report evidence table
29
+ # are the two persisted sinks F-9 masks, and a second copy of the mask marker or the
30
+ # index lookup would be the exact drift CODE_REVIEW F-27 warns about.
31
+ from src.traceability.scratchpad import _PII_MASK, _pii_indexes
32
+
33
  from ..language import detect_reply_language
34
  from ..slow_path.schemas import AnalysisRecord, TaskSummary
35
  from .errors import ReportError
 
191
  continue
192
  if len(output.columns) > _EVIDENCE_MAX_COLS:
193
  continue
194
+ # Mask PII cells before they are frozen into `reports.content`
195
+ # (F-9, 2026-07-24). A report is a permanent, versioned artifact, so
196
+ # this is the sink where an unmasked customer name or email lasts
197
+ # longest. The assembler's FINDINGS are untouched — the lead's
198
+ # 2026-07-23 decision keeps real values in the answer prose; this
199
+ # redacts only the raw evidence dump beneath it. `pii_columns` is
200
+ # absent on records persisted before F-9, which yields no masking.
201
+ pii_idx = _pii_indexes(
202
+ (output.meta or {}).get("pii_columns"), output.columns
203
+ )
204
  tables.append(
205
  EvidenceTable(
206
  title=result.objective,
207
  columns=[str(c) for c in output.columns],
208
  rows=[
209
+ [
210
+ _PII_MASK if i in pii_idx else _fmt_cell(v)
211
+ for i, v in enumerate(row)
212
+ ]
213
  for row in output.rows[:_EVIDENCE_MAX_ROWS]
214
  ],
215
  truncated=len(output.rows) > _EVIDENCE_MAX_ROWS,
src/tools/data_access.py CHANGED
@@ -281,6 +281,15 @@ class DataAccessToolInvoker:
281
  "elapsed_ms": result.elapsed_ms,
282
  # Executed query for traceability (KM-691); None if unavailable.
283
  "query": result.query,
 
 
 
 
 
 
 
 
 
284
  },
285
  )
286
 
@@ -345,6 +354,52 @@ class DataAccessToolInvoker:
345
  )
346
 
347
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
348
  def _json_safe(value: Any) -> Any:
349
  """Coerce DB scalar types that JSON can't represent into plain Python.
350
 
 
281
  "elapsed_ms": result.elapsed_ms,
282
  # Executed query for traceability (KM-691); None if unavailable.
283
  "query": result.query,
284
+ # Which of `columns` hold PII, by NAME (F-9, 2026-07-24). `pii_flag`
285
+ # was an ingestion-time control only: it nulls `sample_values` into
286
+ # the prompt, but nothing stops the planner SELECTing a flagged column
287
+ # — and "list our top 20 customers" legitimately selects one. The real
288
+ # values then flowed into the traceability preview and the report
289
+ # evidence tables, both PERSISTED. Carrying the flags here is what lets
290
+ # those two sinks mask without re-reading the catalog. Consumers must
291
+ # tolerate its absence: older persisted rows have no such key.
292
+ "pii_columns": _pii_column_names(catalog, ir, result.columns),
293
  },
294
  )
295
 
 
354
  )
355
 
356
 
357
+ def _pii_column_names(
358
+ catalog: Catalog, ir: QueryIR, output_columns: list[str]
359
+ ) -> list[str]:
360
+ """Which of `output_columns` carry PII, resolved through the IR's select list.
361
+
362
+ Returns names, not indexes, because the two compilers do not agree on column
363
+ ORDER: the grouped pandas path builds its frame with `reset_index()`, which emits
364
+ the group columns first regardless of where they sat in the select list. Names
365
+ survive that; positions do not.
366
+
367
+ Aggregates are deliberately NOT flagged, with two exceptions. `count`, `sum`,
368
+ `avg` and `count_distinct` return derived numbers that identify nobody — masking
369
+ `sum(salary)` would destroy a legitimate answer for no privacy gain. `min` and
370
+ `max` are different: they return an actual member value, so `max(email)` really is
371
+ one customer's email.
372
+
373
+ Fails OPEN by design — an output name this cannot resolve is simply not listed, so
374
+ the worst case is today's behaviour (unmasked), never a legitimate column wrongly
375
+ blanked. That makes this a mitigation, not a guarantee; the guarantee has to come
376
+ from not selecting the column, which is a product decision (the lead's 2026-07-23
377
+ call was that the assembler still sees real values, so "list our top customers"
378
+ stays answerable).
379
+ """
380
+ cols_by_id = {
381
+ c.column_id: c
382
+ for s in catalog.sources
383
+ for t in s.tables
384
+ for c in t.columns
385
+ }
386
+ present = set(output_columns)
387
+ flagged: list[str] = []
388
+ for item in ir.select:
389
+ col = cols_by_id.get(getattr(item, "column_id", None) or "")
390
+ if col is None or not col.pii_flag:
391
+ continue
392
+ if item.kind == "column":
393
+ name = item.alias or col.name
394
+ elif item.fn in ("min", "max"):
395
+ name = item.alias or f"{item.fn}_{col.name}"
396
+ else:
397
+ continue # count/sum/avg/count_distinct — derived, identifies nobody
398
+ if name in present and name not in flagged:
399
+ flagged.append(name)
400
+ return flagged
401
+
402
+
403
  def _json_safe(value: Any) -> Any:
404
  """Coerce DB scalar types that JSON can't represent into plain Python.
405
 
src/traceability/scratchpad.py CHANGED
@@ -29,6 +29,27 @@ CAP_STR = 300
29
  # the feature, and 300 chars mangles all but the smallest statements.
30
  CAP_QUERY = 2000
31
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
32
 
33
  def _truncate(obj: Any) -> Any:
34
  """Recursively cap strings to CAP_STR and summarize embedded tool results.
@@ -66,9 +87,26 @@ def _output_to_dict(output: Any) -> dict[str, Any]:
66
  columns = getattr(output, "columns", None)
67
  if columns is not None:
68
  result["columns"] = list(columns)
 
 
 
 
 
 
 
 
 
69
  result["preview"] = [
70
- [_truncate(cell) for cell in row] for row in rows[:CAP_PREVIEW_ROWS]
 
 
 
 
71
  ]
 
 
 
 
72
  value = getattr(output, "value", None)
73
  if value is not None:
74
  if kind == "chart" and isinstance(value, dict):
 
29
  # the feature, and 300 chars mangles all but the smallest statements.
30
  CAP_QUERY = 2000
31
 
32
+ # What a masked PII cell shows in the persisted preview (F-9). A fixed marker, not a
33
+ # partial reveal: showing "j***@acme.com" still leaks the domain and the name shape.
34
+ _PII_MASK = "[redacted]"
35
+
36
+
37
+ def _pii_indexes(pii_columns: Any, columns: Any) -> set[int]:
38
+ """Positions in `columns` named by `pii_columns` (from `ToolOutput.meta`).
39
+
40
+ Tolerant on purpose: the key is absent on any output persisted before F-9 and on
41
+ synth dicts, and a name that no longer matches a column is skipped rather than
42
+ raising. This runs inside the traceability scratchpad, which is a never-throw seam
43
+ — a masking slip must never break the user's answer.
44
+ """
45
+ if not pii_columns or not columns:
46
+ return set()
47
+ try:
48
+ wanted = set(pii_columns)
49
+ return {i for i, name in enumerate(columns) if name in wanted}
50
+ except TypeError: # non-iterable payload from an unexpected shape
51
+ return set()
52
+
53
 
54
  def _truncate(obj: Any) -> Any:
55
  """Recursively cap strings to CAP_STR and summarize embedded tool results.
 
87
  columns = getattr(output, "columns", None)
88
  if columns is not None:
89
  result["columns"] = list(columns)
90
+ # Mask PII cells before the preview is PERSISTED (F-9, 2026-07-24). This row
91
+ # goes to `message_traceability.data` and is served by an unauthenticated GET,
92
+ # so real customer names/emails would otherwise sit in the database
93
+ # indefinitely. The assembler still receives the unmasked rows — the lead's
94
+ # 2026-07-23 decision — so the ANSWER is unaffected; only the stored artifact
95
+ # is redacted. `pii_columns` is absent on older outputs, which yields no
96
+ # masking, exactly as before.
97
+ meta = getattr(output, "meta", None) or {}
98
+ pii_idx = _pii_indexes(meta.get("pii_columns"), columns)
99
  result["preview"] = [
100
+ [
101
+ _PII_MASK if i in pii_idx else _truncate(cell)
102
+ for i, cell in enumerate(row)
103
+ ]
104
+ for row in rows[:CAP_PREVIEW_ROWS]
105
  ]
106
+ if pii_idx:
107
+ result["pii_masked"] = sorted(
108
+ columns[i] for i in pii_idx if i < len(columns)
109
+ )
110
  value = getattr(output, "value", None)
111
  if value is not None:
112
  if kind == "chart" and isinstance(value, dict):