fix(pii): require a whole-output proof before masking instead of refusing - #155
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens ACE-041 PII masking by requiring a “whole-output” proof before allowing a refuse→mask downgrade, preventing partial masking that can imply safety while leaking co-projected sensitive values. It also fixes case-insensitive table-alias resolution so qualified sensitive columns are reliably detected.
Changes:
- Adds
SensitiveCheckResult.output_provableand uses it (withall_maskable) to gate when a sensitive projection can be treated as “provable” (mask) vs “uncertain” (refuse). - Improves column-table resolution by case-folding table alias lookup in
_resolve_col_table. - Adds regression tests for the co-projection leak cases and for “no over-refusal” when sensitive columns are only filtered/aggregated.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_ace041_masking.py | Adds regression coverage for co-projection leakage and ensures masking still works when output is provable. |
| packages/agami-core/src/semantic_model/runtime.py | Introduces output_provable, computes it via _output_lineage_provable, and fixes case-insensitive alias resolution. |
| packages/agami-core/src/execute_sql.py | Requires sens.output_provable to treat maskability as “provable” for policy(mask). |
| plugins/agami/lib/execute_sql.py | Mirrors the same masking-proof gating change for the plugin copy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tree.find(exp.CTE) is not None: | ||
| return False # a CTE body is not walked, so its output columns cannot be attributed | ||
| for sel in _output_selects(tree): | ||
| if sel.find(exp.Subquery) is not None: | ||
| return False # derived table (or a scalar subquery in the projection): same blindness | ||
| scope = _tables_in_scope(sel) | ||
| # Case-fold the scope keys: SQL identifiers are case-insensitive unless quoted, so | ||
| # `FROM customers C` + `c.ssn` is one table, and a case-sensitive miss would silently | ||
| # read as "not a sensitive column" rather than "could not resolve". | ||
| lowered = {alias.lower() for alias in scope} | ||
| for proj in sel.expressions: | ||
| for col in proj.find_all(exp.Column): | ||
| if col.table: | ||
| if col.table.lower() not in lowered: | ||
| return False # qualifier names nothing in scope | ||
| elif len(scope) != 1: | ||
| return False # bare column, ambiguous across several tables | ||
| return True |
There was a problem hiding this comment.
Agreed, and fixed in 7002680 — the scoping you describe is exactly what landed.
_output_lineage_provable now tests only what can put a value in the output:
- (a)+(b) an arm's own FROM/JOIN sources, via a new
_arm_sources, bailing when a source is not a plainexp.Table(derived table,VALUES,LATERAL, table function) or when it names a CTE the arm selects from. A CTE that no arm selects from no longer bails. - (c)
proj.find(exp.Subquery)over the projection list only, so a scalar subquery there still bails while aWHERE/HAVING/ORDER BYsubquery, or a JOIN'sONcondition, does not.
Reading sources by node type rather than by arg key on purpose: sqlglot renamed the FROM key (from → from_) inside the sqlglot>=20 range this package accepts, and a missing key would silently degrade to "no sources", which reads as provable.
I also dropped the bare-column bail (len(scope) != 1) that you didn't mention but which turned out to be the highest-volume refusal — it rejected any unqualified column in a two-table join. It was unnecessary: the sensitive match is by NAME against the model, so it doesn't depend on which table the column binds to. Either the name is declared sensitive somewhere and _sensitive_col_ref flags it (falling back to the folded bare name when the table is ambiguous), or it's declared sensitive nowhere and no binding could make it sensitive.
These all masked on base, were refused by the first cut, and mask again now — each pinned by test_unrelated_nesting_still_masks:
SELECT ssn FROM customers WHERE id IN (SELECT cust_id FROM orders)
SELECT ssn FROM customers c WHERE EXISTS (SELECT 1 FROM orders o WHERE o.cust_id = c.id)
SELECT ssn FROM customers GROUP BY ssn HAVING COUNT(*) > (SELECT COUNT(*) FROM orders)
WITH unused AS (SELECT id FROM orders) SELECT ssn FROM customers
SELECT name, ssn FROM customers JOIN orders ON customers.id = orders.cust_idOne caveat worth recording, since it constrains any future narrowing here: the co-projection leaks are contained by refusal, not detection. The detector never sees the laundered column in WITH q AS (SELECT ssn AS z ...) SELECT c.ssn, q.z ... — it reports only the plainly-projected customers.ssn. Narrowing the opaque-source test further would silently re-open those. test_opaque_output_source_is_still_refused pins it.
Review: whole-output proof (ACE-041)Reviewed at Suite verified green via the project harness: The diagnosis is right and the fix does real work. Two things block merge for me: the proof does not establish its stated invariant, and the over-refusal is much wider than the PR intends or its tests can see. 1. Critical:
|
Review follow-up on the whole-output proof (PR #155). Two defects made the proof itself unsound, and one made it far broader than intended. UNSOUND — `output_provable` could be True while a sensitive value reached the output unseen, which is the `('***', raw)` receipt the feature exists to prevent: * A subquery alias could SHADOW an outer alias of the same name. The gate's alias map was built with `find_all(exp.Table)` over the whole subtree, so in `... FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)` the inner `t` overwrote the outer one and `t.ssn` bound to `orders`, which declares no `ssn`. The column read as un-sensitive and was never detected, while the qualifier still "resolved" so the output read as provable. `_sensitive_scope` now records a conflicting alias as unresolved rather than last-one-wins; `_sensitive_col_ref` already falls back to the conservative folded bare name from there. Scope BREADTH is unchanged, so a bare column still resolves through a derived-table body exactly as before. * Two aliases folding to one name (`FROM x "AB", customers ab` with `AB.ssn`) resolved exact-match-first to the quoted `"AB"`, while the engine folds the unquoted `AB` onto `ab`. `_resolve_col_table` now resolves by folded match and returns None when more than one alias folds together, rather than guessing. OVER-BROAD — the proof withheld itself for constructs that cannot contribute an output value at all: any CTE anywhere including one no arm selects from, and any subquery anywhere in an arm including WHERE/HAVING/ORDER BY and a JOIN's ON. That refused ordinary analytics SQL (`SELECT ssn FROM customers WHERE id IN (SELECT ...)`, a plain two-table join with an unqualified projection) which ACE-041 masks, buying no safety. Both tests are now scoped to an arm's own FROM/JOIN sources and its projection list, mirroring the distinction `check_column_scope` already draws. The bare-column test is dropped outright: the sensitive match is by NAME against the model and does not depend on which table the column binds to. Tests: the adversarial matrix this needed, in both directions — alias shadowing (EXISTS/NOT EXISTS/JOIN, co-projected and standalone), quoted/unquoted collision, eight unrelated-nesting shapes that must still mask, and the opaque-source cases that must still refuse. `test_provable_output_still_masks` now fixtures each case to its real projection shape so "no raw value survives" is assertable; it previously asserted only that a token appeared somewhere, and passed on base for the very case it documents. Renamed `test_no_over_refusal_...` to say what it actually pins (the early-return path, not the proof). Also: correct the `projections` comment (it called a field that now drives MaskPlan "latent"), record that the co-projection leaks are closed by refusal rather than detection, and read `output_provable` via `getattr` in the vendored copy so a plugin/package version skew fails closed. `python3 dev.py check` green — 1957 passed, 66 skipped, 2 xfailed, ruff + gitleaks + lib drift clean. Spec: ACE-041
0aefb47 to
7002680
Compare
Review follow-up pushed — 7002680Rebased onto Fixed: the proof was unsoundTwo binding bugs let Alias shadowing. The gate's alias map was built with The inner
Quoted/unquoted collision. Fixed: the proof was far broader than intendedThis is Copilot's comment and finding 2 of my review, same root cause. Scoped to an arm's own FROM/JOIN sources and its projection list, mirroring what Also
Deliberately not in this PRBoth pre-existing, both worth their own issue:
The standalone alias-rename bypass stays |
| - **Required a whole-output proof before masking a sensitive projection, instead of | ||
| refusing.** The mask decision quantified over the projections the detector *found* |
There was a problem hiding this comment.
Agreed, fixed in d142f10. The header stated the change backwards — masking is what got gated, and an unprovable output is what now refuses. It was carried over from the PR title, where "instead of refusing" referred to ACE-041's earlier refuse→mask flip, which is context a release note does not have.
Now reads:
Gated PII masking on a whole-output proof; an output that cannot be proven clean is now refused.
| @@ -1691,7 +1810,24 @@ def collect(node): | |||
|
|
|||
| def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]: | |||
There was a problem hiding this comment.
Good catch, fixed in d142f10.
Widened to Mapping[str, Optional[str]] rather than dict[str, Optional[str]], and applied it to the two helpers that pass the scope through as well (_sensitive_col_ref, _classify_projection). Mapping matters here: its value type is covariant while dict's is invariant, so the fan/chasm and aggregation callers keep passing a plain dict[str, str] with no change. Verified with mypy 2.3 — the new signature accepts both maps, the old one rejects the Optional-valued one:
error: Argument 1 to "old_style" has incompatible type "dict[str, str | None]"; expected "dict[str, str]"
On the len(matches) == 1 path returning None: that is intended and now stated in the docstring. A shadowed alias resolving to None is the whole point — _sensitive_col_ref treats unresolved as "sensitive somewhere in scope" and falls back to the conservative folded bare name, so it still gets flagged and masked rather than silently binding to the wrong table.
…sing
The mask verdict was derived from an incomplete detector, and the refuse->mask flip
turned detector misses into live PII leaks.
`all_maskable` quantifies over `sens.projections` — the projections the detector
FOUND — so it proves "every offending projection we saw is maskable", never "no
sensitive value reaches the output". Under the earlier refuse-only behaviour that
gap leaked only when NOTHING was detected, because anything detected blocked the
whole query. Once a detected-and-maskable projection let the query RUN, a single
seen column unblocked the entire result set, and every co-projected value the
name-based detector missed came back raw — beside a `***` implying the row had
been protected.
Reproduced against this branch's own fixtures; each returned ('***', '111-22-3333'):
SELECT ssn, t.ssn FROM customers, (SELECT ssn FROM customers) t
WITH q AS (SELECT ssn AS z FROM customers) SELECT c.ssn, q.z FROM customers c, q
SELECT ssn, c.ssn FROM customers C
Two changes:
1. `SensitiveCheckResult.output_provable` (new) — whether the detector could have
seen EVERY value reaching the output, not just whether what it found was
maskable. False when an output-bearing arm draws through a derived table or CTE
(whose bodies `_output_selects` deliberately does not walk), or projects a column
whose qualifier resolves to nothing in scope. `certainty="provable"` now requires
it, so an unprovable output falls back to `uncertain` -> reject: the pre-flip
behaviour, restored exactly where it was load-bearing.
2. `_resolve_col_table` folds case on the alias lookup. `FROM customers C` + `c.ssn`
is one table, but the exact-match lookup missed and fell through to the raw
qualifier, which the sensitive index then failed to match — reading as "not
sensitive" rather than "unresolved". `4bd39bb` folded the COLUMN name for this
reason; this is the other half. Exact match still wins first, so a genuinely
distinct quoted identifier is unaffected.
Because of (2) the third case above no longer needs refusing at all — the detector
now sees both columns and masks both, which beats failing closed.
Deliberately NOT widened: the STANDALONE alias-rename bypass
(`SELECT z FROM (SELECT ssn AS z FROM customers) q`) stays xfail. Nothing sensitive
is detected there, so the proof never engages — that is real lineage tracking and
belongs to ACE-062. This change only stops trusting an incomplete detector; it does
not try to complete it.
No over-refusal: the proof runs only after an offending projection is detected, so
COUNT/WHERE/GROUP BY use of a sensitive column is untouched even with a derived
table in the FROM. Pinned by tests.
runtime.py is single-copy; the execute_sql.py change is mirrored via sync-lib.
dev.py check green — 1910 passed, 66 skipped, 2 xfailed.
Spec: ACE-041
Review follow-up on the whole-output proof (PR #155). Two defects made the proof itself unsound, and one made it far broader than intended. UNSOUND — `output_provable` could be True while a sensitive value reached the output unseen, which is the `('***', raw)` receipt the feature exists to prevent: * A subquery alias could SHADOW an outer alias of the same name. The gate's alias map was built with `find_all(exp.Table)` over the whole subtree, so in `... FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)` the inner `t` overwrote the outer one and `t.ssn` bound to `orders`, which declares no `ssn`. The column read as un-sensitive and was never detected, while the qualifier still "resolved" so the output read as provable. `_sensitive_scope` now records a conflicting alias as unresolved rather than last-one-wins; `_sensitive_col_ref` already falls back to the conservative folded bare name from there. Scope BREADTH is unchanged, so a bare column still resolves through a derived-table body exactly as before. * Two aliases folding to one name (`FROM x "AB", customers ab` with `AB.ssn`) resolved exact-match-first to the quoted `"AB"`, while the engine folds the unquoted `AB` onto `ab`. `_resolve_col_table` now resolves by folded match and returns None when more than one alias folds together, rather than guessing. OVER-BROAD — the proof withheld itself for constructs that cannot contribute an output value at all: any CTE anywhere including one no arm selects from, and any subquery anywhere in an arm including WHERE/HAVING/ORDER BY and a JOIN's ON. That refused ordinary analytics SQL (`SELECT ssn FROM customers WHERE id IN (SELECT ...)`, a plain two-table join with an unqualified projection) which ACE-041 masks, buying no safety. Both tests are now scoped to an arm's own FROM/JOIN sources and its projection list, mirroring the distinction `check_column_scope` already draws. The bare-column test is dropped outright: the sensitive match is by NAME against the model and does not depend on which table the column binds to. Tests: the adversarial matrix this needed, in both directions — alias shadowing (EXISTS/NOT EXISTS/JOIN, co-projected and standalone), quoted/unquoted collision, eight unrelated-nesting shapes that must still mask, and the opaque-source cases that must still refuse. `test_provable_output_still_masks` now fixtures each case to its real projection shape so "no raw value survives" is assertable; it previously asserted only that a token appeared somewhere, and passed on base for the very case it documents. Renamed `test_no_over_refusal_...` to say what it actually pins (the early-return path, not the proof). Also: correct the `projections` comment (it called a field that now drives MaskPlan "latent"), record that the co-projection leaks are closed by refusal rather than detection, and read `output_provable` via `getattr` in the vendored copy so a plugin/package version skew fails closed. `python3 dev.py check` green — 1957 passed, 66 skipped, 2 xfailed, ruff + gitleaks + lib drift clean. Spec: ACE-041
`b883633` renamed `model_store.write_organization` -> `write_datasource` (same
signature) when `Organization` became `Datasource`, and separately ADDED a
`write_organization_record` for the new company-level OrgRecord. `tests/e2e/
harness.py` was not updated, so `seed_db_model` raised
AttributeError: module 'model_store' has no attribute 'write_organization'.
Did you mean: 'write_organization_record'?
and Python's suggestion points at the wrong function — the OrgRecord writer is a
different concept, not the renamed one.
Pre-existing on agami-governance-branch, not introduced by this branch: only the
Postgres integration job imports this harness, and `dev.py check` does not run it,
so both branches were green locally while that job failed.
Reproduced against a local PostgreSQL 16 with the job's own command and env:
before, `6 passed, 58 errors` — byte-identical to the CI run; after, `64 passed`.
…g header Both from Copilot's re-review. `_resolve_col_table` is now called with `_sensitive_scope`'s map, which uses None to mark a shadowed alias, so `dict[str, str]` was no longer accurate. Widened it and the two sensitive-gate helpers that pass it through to `Mapping[str, Optional[str]]` — Mapping rather than dict because a Mapping's value type is covariant, so the fan/chasm and aggregation callers keep passing a plain `dict[str, str]` unchanged. Verified with mypy 2.3: the new signature accepts both maps, while the old one rejects the Optional-valued map. The changelog header read "before masking ... instead of refusing", which states the change backwards for release notes — masking is what got GATED, and an unprovable output is what now refuses.
d142f10 to
a9a8393
Compare
The defect
all_maskablequantifies oversens.projections— the projections the detector found:It reads as a fail-closed
all(), and it isn't. It proves "every offending projection we saw is maskable" — never "no sensitive value reaches the output". It can say nothing about what the detector missed.The refuse→mask flip is what turned that into a leak. Previously a detector miss leaked only when nothing was detected, because anything detected blocked the whole query. Once a detected-and-maskable projection let the query run, one seen column unblocked the entire result set — so every co-projected value the name-based detector missed came back raw, beside a
***implying the row had been protected.Reproduced against this branch's own fixtures. Each returned
('***', '111-22-3333'):The fix — three parts
1.
SensitiveCheckResult.output_provable— whether the detector could have seen every value reaching the output, not just whether what it found was maskable. False when an output-bearing arm draws through a source whose body is not walked (a derived table, a CTE the arm selects from, aVALUES/LATERALsource), projects a scalar subquery, or projects a qualifier naming none of that arm's own sources.certainty="provable"now requires it, so an unprovable output falls back touncertain→reject.2. Alias binding is shadow-aware, and fails closed when genuinely ambiguous. Two bugs here, and both let
output_provablecome back True while a sensitive value reached the output unseen — the exact receipt this PR exists to prevent:find_all(exp.Table)over the whole subtree, so a subquery alias overwrote an outer alias of the same name. In... FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)the innertrebound the outert.ssntoorders, which declares nossn— so it read as un-sensitive and was never detected, while the qualifier still "resolved".EXISTScompounds it: sqlglot parses it asexp.Existsaround a bareSelect, with noexp.Subquerynode for a derived-table check to catch.FROM x "AB", customers abwithAB.ssn) resolved exact-match-first to the quoted"AB"→x, where the engine folds the unquotedABontoab→customers._sensitive_scopenow records a conflicting alias as unresolved rather than last-one-wins, and_resolve_col_tableresolves by folded match, returningNonewhen more than one alias folds together instead of guessing. Scope breadth is deliberately unchanged, so a bare column still resolves through a derived-table body exactly as before and the existingtest_sensitive_projection_maskabletruth table keeps its precise refs.3.
_resolve_col_tablefolds case on the alias lookup.FROM customers C+c.ssnis one table; the exact-match lookup missed and fell through to the raw qualifier, which the sensitive index then failed to match — reading as "not sensitive" rather than "unresolved". ACE-041 slice 3 (4bd39bb) folded the COLUMN name for the same reason; this is the other half. It closes a live raw-PII leak nothing had reported:SELECT C.ssn FROM customers creturnedrows=[('111-22-3333',)]withmasked_columns=().What review changed
The first cut over-refused, and its own guard against that did not work.
sel.find(exp.Subquery)searched the whole arm andtree.find(exp.CTE)the whole tree, so aWHERE ... IN (SELECT ...), an unreferenced CTE, or an unqualified column in an ordinary two-table join all flipped from masked to refused.test_no_over_refusal_when_nothing_sensitive_is_projectedcould not catch it: every case in it returns ataction="allow"and never reaches the proof.Both tests are now scoped to what can actually put a value in the output — an arm's own FROM/JOIN sources and its projection list — mirroring the distinction
check_column_scopealready draws. The bare-column bail is dropped outright: the sensitive match is by name against the model, so it does not depend on which table the column binds to. These masked on base, were refused by the first cut, and mask again now:That misleading test is renamed to
test_aggregate_or_filter_use_returns_before_the_proof, which is what it actually pins, andtest_unrelated_nesting_still_masksnow covers the direction it claimed to.test_provable_output_still_masksalso asserted only that a redaction token appeared somewhere in the row, which passed on base for the very case it documents. Each case now fixtures its real projection shape, with the secret only in cells the engine would fill with a sensitive value, so "no raw value survives" is directly assertable.What this deliberately does NOT do
The standalone alias-rename bypass (
SELECT z FROM (SELECT ssn AS z FROM customers) q) staysxfail— nothing sensitive is detected there, so the proof never engages. That needs real lineage tracking and belongs to ACE-062. (The alias-shadowing class above is not part of it: it came from scope flattening, not the rename gap, so it is closed here rather than deferred.)This change stops trusting an incomplete detector. It does not try to complete it.
Two pre-existing defects found during review are not fixed here — each needs its own tests and blast-radius check, and folding them into a PII fix makes it harder to review, not safer. Both are now tracked:
SELECT `ssn` FROM `customers`returns a raw SSN today. The read-only/safety lexer is a separate path and is unaffected, so this is disclosure, not integrity._apply_mask_planapplies plan indices to the executor's rows positionally with no check that the result has the shape the guard assumed. Its docstring asserts the invariant and never verifies it. Holds for the built-in executors; an injectedports.Executorthat prepends or reorders a column returns raw PII beside a receipt claiming redaction.Also in this PR
b883633renamedmodel_store.write_organization→write_datasourceand missedtests/e2e/harness.py, so the Postgres integration job failed onagami-governance-branchtoo. Only that job imports the harness anddev.py checkdoes not run it, which is why both branches were green locally. Reproduced against a local PostgreSQL 16 with the job's own command:6 passed, 58 errorsbefore,64 passedafter.projectionsfield comment, which called a field that now drivesMaskPlan.indices"latent … a later masking slice reads it".test_opaque_output_source_is_still_refusedpins it.getattr(sens, "output_provable", False)in the vendored copy, so a plugin/package version skew fails closed.Test plan
python3 dev.py checkgreen — 1957 passed, 66 skipped, 2 xfailed, ruff + gitleaks + vendored-lib drift cleanMapping[str, Optional[str]]accepts both the plain and the shadow-aware map; the previousdict[str, str]rejects the latterruntime.pyis single-copy; theexecute_sql.pychange is mirrored viasync-libTargets
agami-governance-branch, notmain— ACE-041 has not shipped.Spec: ACE-041