fix(pii): require a whole-output proof before masking instead of refusing by sandeep-agami · Pull Request #155 · AgamiAI/agami-core

sandeep-agami · 2026-07-27T23:19:55Z

The defect

all_maskable quantifies over sens.projections — the projections the detector found:

all_maskable = bool(sens.projections) and all(
    p.maskable and p.output_index is not None for p in sens.projections
)

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'):

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

The fix — two 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 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 exactly 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 no longer needs refusing at all — the detector now sees both columns and masks both:

SELECT ssn, t.ssn FROM customers, (SELECT ssn ...) t   -> REFUSED (sensitive_columns)
WITH q AS (SELECT ssn AS z ...) SELECT c.ssn, q.z ...  -> REFUSED (sensitive_columns)
SELECT ssn, c.ssn FROM customers C                     -> RAN  [('***','***')]

What this deliberately does NOT do

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 needs real lineage tracking and belongs to ACE-062.

This change stops trusting an incomplete detector. It does not try to complete it — chasing detector coverage inside a fail-closed fix is how you get a half-proof that looks whole.

I've also corrected the standing xfail comment, which called the alias-lineage gap "PRE-EXISTING (not introduced by ACE-041)". True of the standalone form; the co-projection form was new.

No over-refusal

The proof runs only after an offending projection is detected, so a query that merely counts or filters on a sensitive column never reaches it — including one with a derived table in the FROM. Pinned by test_no_over_refusal_when_nothing_sensitive_is_projected.

Test plan

python3 dev.py check green — 1910 passed, 66 skipped, 2 xfailed, ruff + gitleaks + vendored-lib drift clean
New tests fail before the fix and pass after (3 leak cases red → green)
test_provable_output_still_masks guards the other direction, so this can't silently degrade into refuse-everything
runtime.py is single-copy; the execute_sql.py change is mirrored via sync-lib

Targets agami-governance-branch, not main — ACE-041 has not shipped.

Spec: ACE-041

Copilot

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_provable and uses it (with all_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.

sandeep-agami · 2026-07-28T00:02:21Z

+    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


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 plain exp.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 a WHERE/HAVING/ORDER BY subquery, or a JOIN's ON condition, 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_id

One 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.

sandeep-agami · 2026-07-27T23:42:04Z

Review: whole-output proof (ACE-041)

Reviewed at 0aefb47 against agami-governance-branch (e817dc0). Every finding below was reproduced by running the real guard chain (execute_guarded) over the test file's own _org() fixture on both branches; nothing was modified.

Suite verified green via the project harness: uvx --with-editable "packages/agami-core[model,server]" pytest tests/ -q gives 1910 passed, 66 skipped, 2 xfailed, matching the test plan. (A bare python3 -m pytest shows 38 failures on this branch and the identical 38 on base: a local-env artifact from tests/conftest.py:56, not the PR.)

The diagnosis is right and the fix does real work. all_maskable was never a whole-output proof, and I reproduced both _CO_PROJECTION_LEAK cases returning ('***', '111-22-3333') on base and refusing here. The _resolve_col_table fold independently closes a live raw-PII leak that nothing had reported: SELECT c.ssn FROM customers C returned rows=[('111-22-3333',)] with masked_columns=() on base, and masks correctly now.

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: `output_provable=True` on an output the detector did not fully see

The exact failure mode this PR exists to eliminate is still reachable, with the proof returning True:

SELECT c.ssn, t.ssn FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)
  scope   = {'c': 'customers', 't': 'orders'}
  action=refuse  output_provable=True  projs=[('customers.ssn', True, 0)]
  RESULT  -> ('***', '111-22-3333')

Two defects compose, and neither is guarded:

_tables_in_scope flattens the whole tree. It builds alias -> table from tree.find_all(exp.Table) (runtime.py:1675), so the subquery's FROM orders t overwrites the outer t -> customers. The outer t.ssn resolves to orders, which declares no ssn, so it is judged non-sensitive and never enters projections. Meanwhile every qualifier does resolve, so the :563 branch is satisfied and the proof returns True.
EXISTS produces no exp.Subquery node. sqlglot parses it as exp.Exists wrapping a bare Select, so the :553 bail-out never fires, even though the detector is exactly as blind to that body as to a derived table. IN (SELECT ...) does produce a Subquery and does bail. The proof's coverage tracks an incidental AST shape rather than the semantics.

The quoted/unquoted alias collision is the same class:

SELECT c.ssn, AB.ssn FROM customers c, x "AB", customers ab
  -> ('***', '111-22-3333')   output_provable=True

AB exact-matches the quoted "AB" before the new fold is consulted, so it resolves to x. Postgres folds unquoted AB to ab = customers and returns a real SSN.

Both leak on base too, so this is not a regression. It matters because output_provable is the token that unlocks masking, and here it certifies an output the detector never saw. t is an extremely common alias in generated SQL, so this is reachable without adversarial intent. The standalone forms (SELECT t.ssn FROM customers t WHERE EXISTS (SELECT 1 FROM orders t)) return a fully raw SSN at action=allow, and are caused by scope flattening rather than the rename/lineage gap, so ACE-062 would not close them: they need their own tracking.

2. Important: the over-refusal is wide, and nothing pins it

These all masked on base and refuse here:

SELECT ssn FROM customers WHERE id IN (SELECT cust_id FROM orders)
SELECT name, ssn FROM customers JOIN orders ON customers.id = orders.cust_id
WITH unused AS (SELECT id FROM orders) SELECT ssn FROM customers          -- CTE never referenced
SELECT ssn, (SELECT COUNT(*) FROM orders) AS n FROM customers
SELECT ssn FROM customers GROUP BY ssn HAVING COUNT(*) > (SELECT COUNT(*) FROM orders)
SELECT ssn FROM customers a, customers b

Three independent causes, all broader than "the output cannot be traced": tree.find(exp.CTE) bails on any CTE anywhere including a dead one; sel.find(exp.Subquery) bails on any subquery anywhere in the arm including WHERE/HAVING/ORDER BY; and len(scope) != 1 (:565) bails on any unqualified column whenever more than one table is in scope, which is an ordinary two-table join. None of these can leak, since a WHERE subquery yields a filter and not an output column.

Worth noting the inconsistency this produces: WHERE ... IN (SELECT ...) refuses but WHERE EXISTS (SELECT ...) masks, purely on node shape. Two rewrites of the same query, opposite PII verdicts.

check_column_scope already draws the right line at runtime.py:958-960: if isinstance(sq.parent, (exp.From, exp.Join)). Scoping the CTE check to CTEs actually referenced by an output arm's FROM, and the subquery check to From/Join parents, preserves all three leak fixes in the description while dropping the six shapes above.

One coupling to record if you do narrow it: the _CO_PROJECTION_LEAK cases are contained by refusal, not by detection. The detector reports projections=[('customers.ssn', True, 0)] for both, on base and on HEAD, and never sees the laundered column. So narrowing the CTE test re-opens the WITH q AS (SELECT ssn AS z ...) leak while every test in the file still passes.

3. Important: the docstring claims a guarantee the parse cannot deliver

runtime.py:532 asks "Could the detector have seen EVERY value reaching the output?" No sqlglot.parse_one in the guard path passes a dialect= (runtime.py:93, 697, 787, 860, 907). The plugin advertises MySQL, BigQuery and Databricks, all backtick-quoting. Confirmed end-to-end:

SELECT `ssn` FROM `customers`        -> tables seen: []   action=allow   rows=[('111-22-3333',)]
SELECT `c`.`ssn` FROM customers `c`  -> tables seen: []   action=allow   rows=[('111-22-3333',)]

Pre-existing, but load-bearing here: the proof returns True on a tree where the detector has seen nothing. Either thread the datasource dialect through (credentials already carry type), or downgrade the wording to what is actually established.

Related: pyproject.toml:35 pins sqlglot>=20 with no ceiling, and the verdict is version-dependent. WHERE c.id IN (SELECT ...) yields no Subquery on 20.11.0 and does on 25.34.1 and 30.10.0, so the same query masks on one and refuses on the other.

4. Important: the fold changes four non-PII call sites, untested

check_table_scope and check_column_scope build their own folded maps and are unaffected. The other consumers of _resolve_col_table are _lookup_column:1292, _check_aggregation_semantics:1420, _aggregate_source_tables:1691, and _tables_referenced_outside_from:1718. Every measured delta moves in the safe direction, and one is a genuine bug fix worth calling out:

SELECT SUM(o.total_amount) FROM orders o JOIN line_items li ON li.order_id=o.id WHERE LI.qty > 1
  pre-fold : fan_trap -> auto_rewrite, "... FROM orders AS o WHERE LI.qty > 1"   (dangling alias, executed)
  post-fold: fan_trap -> refuse

The PR touches only test_ace041_masking.py, and the whole test tree contains exactly two case-mismatched-alias SQL literals, both in the new PII parametrize list. These four behaviors will regress silently.

Minor: the fold returns the first dict-order match. Collecting all fold-matches and returning None on more than one would fail closed for one extra line.

5. Test gaps

test_no_over_refusal_when_nothing_sensitive_is_projected does not test over-refusal. All three cases return action="allow" and never enter the if sens.action == "refuse": block, so the proof is never consulted. It passes identically with the fix reverted.
test_provable_output_still_masks asserts REDACTION_TOKEN in flat, never the absence of the secret. [SELECT ssn, c.ssn FROM customers C] therefore passes on base, where the actual output is ('***', '111-22-3333'). That is the one case carrying a five-line comment about why the alias fold matters. The strong form is 60 lines below in the xfail test (assert "111-22-3333" not in flat); the fixture shape (secret in every cell) is what blocks using it here.
Only 3 of the 12 new tests fail on base. The other 9 pass with the whole fix reverted.
No unit test for _output_lineage_provable, and output_provable appears nowhere under tests/. tests/test_sensitive_projection_maskable.py is the purpose-built truth table for this function and was not touched, though two of its documented-maskable cases now carry output_provable=False and no longer mask end to end. tests/safety/corpus.py still holds one PII case.
Both in-loop return False branches (:563, :565) are unreachable from any test, including the highest-volume over-refusal.
The xfail is strict=False, so the "flip to xpass is the tracking signal" claim is not enforced by CI.

6. Smaller items

Rebase before merge. Branch is two commits behind base, one of them f3097a2 security: close a read-only-guard bypass via a welded quoted identifier. This has never run against the merged result.
No CHANGELOG entry. Base now carries ## [Unreleased] / ### Security, added by that sibling fix; this is also a security fix.
_apply_mask_plan has no post-condition (execute_sql.py:1547-1570). Indices are applied to result.rows positionally with no cross-check against result.columns. With an injected ports.Executor returning an extra leading column, the result is (1, '***', '111-22-3333') with a receipt claiming redaction. Not reachable on the built-in path, but execute_guarded exists precisely so consumers can inject one.
plugins/agami/lib/execute_sql.py:1310 reads sens.output_provable unguarded. That file is vendored into the marketplace plugin; runtime.py is not, and resolves from the separately-versioned pip-installed agami-core. getattr(sens, "output_provable", False) fails closed for free.
Comment accuracy. The field comment at :465 and the docstring at :536 both say "two things break the trace" and omit the len(scope) != 1 branch, which is the one that fires most. The docstring's second bullet illustrates a non-resolving qualifier with FROM customers C + c.ssn, which this PR's own fold now resolves (a real one is SELECT zz.ssn FROM customers c). And projections' pre-existing comment still reads "Latent in THIS slice ... a later masking slice reads it", directly above the new comment describing the masking that landed; it now drives MaskPlan.indices, where order and duplicates are significant.

What is solid

Exception handling is genuinely fail-closed. _output_lineage_provable did not raise across a 50-statement x 5-dialect corpus, and when forced to raise, every boundary (execute_guarded, tools._run_in_process, tool_execute_sql, mcp_harness, mcp_http, subprocess main) propagates without the executor running. No path turns an exception into an allow.
output_provable=False is unreachable as a mask: all six construction sites verified, the only reader is inside the refuse branch, and False ANDs to reject.
The two execute_sql.py copies are byte-identical (md5 85ae51b907fbaa987bf8d9f53b1d05e2), and plugins/agami/lib/ ships no runtime.py, so the plugin copy cannot bind a stale dataclass in-tree.
assert not spy.calls is the strongest assertion in the new suite: it pins that refusal happens before execution.
apply_default_filters touches only where, so mask indices stay valid across the rewrite at :1327.
The "all_maskable quantifies over the projections the detector FOUND" comment is the clearest statement of the invariant anywhere in the change, and every historical clause in it checks out against 4e9b3c2 and 4bd39bb.

Suggested order

Fix scope flattening (per-SELECT _tables_in_scope) and treat exp.Exists like exp.Subquery, then add the four alias-shadowing statements as refuse-tests.
Narrow the CTE and subquery bails to output-bearing positions, and pin all six over-refusal shapes with their intended verdicts either way.
Rewrite the test_provable_output_still_masks fixture so "no raw value survives" is assertable, and assert it.
Add an _output_lineage_provable truth table to test_sensitive_projection_maskable.py, and case-mismatched-alias cases to the aggregation and fan/chasm tests.
Rebase onto e817dc0, add the CHANGELOG entry, re-run.

…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

sandeep-agami · 2026-07-28T00:02:48Z

Review follow-up pushed — `7002680`

Rebased onto e817dc0 (picking up the read-only-guard weld fix this branch had never run against) and addressed the review. python3 dev.py check green: 1957 passed, 66 skipped, 2 xfailed, ruff + gitleaks + lib drift clean.

Fixed: the proof was unsound

Two binding bugs let output_provable come back True while a sensitive value reached the output unseen — the exact ('***', raw) receipt this feature exists to prevent.

Alias shadowing. The gate's alias map was built with find_all(exp.Table) over the whole subtree, so a subquery alias overwrote an outer one of the same name:

SELECT c.ssn, t.ssn FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)
  before: output_provable=True  ->  ('***', '111-22-3333')
  after : masked both           ->  ('***', '***')

The inner t rebound the outer t.ssn to orders, which declares no ssn, so it read as un-sensitive and was never detected — while the qualifier still "resolved", so the output read as provable. EXISTS compounded it: sqlglot parses it as exp.Exists wrapping a bare Select with no exp.Subquery node, so the derived-table bail never fired. The standalone form SELECT t.ssn FROM customers t WHERE EXISTS (SELECT 1 FROM orders t) returned a fully raw SSN at action=allow.

_sensitive_scope now records a conflicting alias as unresolved instead of last-one-wins. Scope breadth is deliberately unchanged, so a bare column still resolves through a derived-table body exactly as before and the existing test_sensitive_projection_maskable truth table keeps its precise customers.ssn refs. My first attempt narrowed scope to the arm's own sources and did shift three of those expectations to a bare ssn — silent semantic drift, so I backed it out and fixed the actual defect instead.

Quoted/unquoted collision. FROM x "AB", customers ab with AB.ssn resolved exact-match-first to the quoted "AB" → x, while Postgres folds the unquoted AB onto ab → customers. _resolve_col_table now resolves by folded match and returns None when more than one alias folds together, rather than guessing. Both call sites that consume it (_lookup_column, _check_aggregation_semantics) already handle None.

Fixed: the proof was far broader than intended

This 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 check_column_scope already does. Also dropped the bare-column bail, which rejected any unqualified column in a two-table join and was unnecessary — the sensitive match is by name against the model, so it doesn't depend on which table the column binds to. Six shapes that masked on base, were refused by the first cut, and mask again now.

Also

test_provable_output_still_masks now fixtures each case to its real projection shape and asserts the raw value is absent. It previously asserted only that a token appeared somewhere in the row, which passed on base for the very case it documents.
Renamed test_no_over_refusal_when_nothing_sensitive_is_projected to test_aggregate_or_filter_use_returns_before_the_proof — every case in it ends at action="allow" and returns before the proof, so it never tested over-refusal.
Corrected the projections comment, which called a field that now drives MaskPlan.indices "latent ... a later masking slice reads it".
Recorded that the co-projection leaks are closed by refusal, not detection, so a future narrowing can't re-open them unnoticed.
getattr(sens, "output_provable", False) in the vendored copy: that file ships in the plugin while runtime.py resolves from the separately-versioned installed package, so a skew now fails closed.
CHANGELOG entry under [Unreleased] / Security.

Deliberately not in this PR

Both pre-existing, both worth their own issue:

Dialect-less parse. No sqlglot.parse_one in the guard passes dialect=, so backtick identifiers parse to nonsense and the guard sees no tables at all: SELECT \ssn` FROM `customers`returns a raw SSN today. It undercuts every gate, not just this one, and the fix is to thread the datasource dialect through (credentials already carrytype`).
_apply_mask_plan has no post-condition. Indices are applied to result.rows positionally with no cross-check against result.columns. Not reachable on the built-in path, but execute_guarded exists so consumers can inject an executor.

The standalone alias-rename bypass stays xfail for ACE-062 as before. Note that the alias-shadowing class above was not part of it — it came from scope flattening rather than the rename gap — and is now closed rather than deferred.

`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`.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

sandeep-agami · 2026-07-28T01:05:58Z

+- **Required a whole-output proof before masking a sensitive projection, instead of
+  refusing.** The mask decision quantified over the projections the detector *found*


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.

sandeep-agami · 2026-07-28T01:05:55Z

@@ -1691,7 +1810,24 @@ def collect(node):

 def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]:


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.

…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.

sandeep-agami requested a review from ashwin-agami as a code owner July 27, 2026 23:19

sandeep-agami requested a review from Copilot July 27, 2026 23:22

Copilot started reviewing on behalf of sandeep-agami July 27, 2026 23:22 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

sandeep-agami added 2 commits July 27, 2026 16:51

sandeep-agami force-pushed the fix/ace041-mask-whole-output-proof branch from 0aefb47 to 7002680 Compare July 28, 2026 00:02

sandeep-agami requested a review from Copilot July 28, 2026 00:51

Copilot started reviewing on behalf of sandeep-agami July 28, 2026 00:51 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(pii): require a whole-output proof before masking instead of refusing - #155

fix(pii): require a whole-output proof before masking instead of refusing#155
sandeep-agami wants to merge 4 commits into
agami-governance-branchfrom
fix/ace041-mask-whole-output-proof

sandeep-agami commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sandeep-agami Jul 28, 2026

Uh oh!

sandeep-agami commented Jul 27, 2026

Uh oh!

sandeep-agami commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sandeep-agami Jul 28, 2026

Uh oh!

sandeep-agami Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		- **Required a whole-output proof before masking a sensitive projection, instead of
		refusing.** The mask decision quantified over the projections the detector found

		@@ -1691,7 +1810,24 @@ def collect(node):

		def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]:

Uh oh!

Conversation

sandeep-agami commented Jul 27, 2026

The defect

The fix — two parts

What this deliberately does NOT do

No over-refusal

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

sandeep-agami Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

sandeep-agami commented Jul 27, 2026

Review: whole-output proof (ACE-041)

1. Critical: output_provable=True on an output the detector did not fully see

2. Important: the over-refusal is wide, and nothing pins it

3. Important: the docstring claims a guarantee the parse cannot deliver

4. Important: the fold changes four non-PII call sites, untested

5. Test gaps

6. Smaller items

What is solid

Suggested order

Uh oh!

sandeep-agami commented Jul 28, 2026

Review follow-up pushed — 7002680

Fixed: the proof was unsound

Fixed: the proof was far broader than intended

Also

Deliberately not in this PR

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

sandeep-agami Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

sandeep-agami Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. Critical: `output_provable=True` on an output the detector did not fully see

Review follow-up pushed — `7002680`