From 7b0a221967023c6d26482b238945b0539edd7fcb Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 16:19:26 -0700 Subject: [PATCH 1/4] fix(pii): require a whole-output proof before masking instead of refusing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/agami-core/src/execute_sql.py | 11 ++- .../agami-core/src/semantic_model/runtime.py | 61 +++++++++++- plugins/agami/lib/execute_sql.py | 11 ++- tests/test_ace041_masking.py | 94 ++++++++++++++++++- 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/packages/agami-core/src/execute_sql.py b/packages/agami-core/src/execute_sql.py index 6f0e2285..ecc6e31b 100644 --- a/packages/agami-core/src/execute_sql.py +++ b/packages/agami-core/src/execute_sql.py @@ -1296,14 +1296,23 @@ def _model_safety( mask_plan: MaskPlan | None = None sens = RT.check_sensitive_projection(sql, org, ctx=ctx) if sens.action == "refuse": + # Two conditions, and BOTH are required. `all_maskable` quantifies over the projections + # the detector FOUND, so on its own it proves only "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 (anything detected blocked the + # whole query); once a maskable projection let the query RUN, one 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. `output_provable` is the + # missing half: the detector could see the whole output, not just part of it. all_maskable = bool(sens.projections) and all( p.maskable and p.output_index is not None for p in sens.projections ) + provable = all_maskable and sens.output_provable verdict = Verdict( cls="data_protection", rule="sensitive_projection", severity="high", - certainty="provable" if all_maskable else "uncertain", + certainty="provable" if provable else "uncertain", detail=sens.reason, remediation=sens.suggestion or "", ) diff --git a/packages/agami-core/src/semantic_model/runtime.py b/packages/agami-core/src/semantic_model/runtime.py index 4e8a7ef1..3a32f115 100644 --- a/packages/agami-core/src/semantic_model/runtime.py +++ b/packages/agami-core/src/semantic_model/runtime.py @@ -462,6 +462,13 @@ class SensitiveCheckResult: # projection index; duplicates and multi-arm collisions on `output_index` are preserved (unlike # `columns`, which is the de-duplicated sorted set the refuse message is built from). projections: list["SensitiveProjection"] = field(default_factory=list) + # Whether the WHOLE output was resolvable, not just whether each FOUND projection was + # maskable (ACE-041 review). `projections` can only describe what the detector saw; this + # says whether it could have seen everything. False when an output-bearing arm draws + # through a derived table / CTE, or projects a column whose qualifier does not resolve — + # i.e. exactly where the name-based match is blind. Masking a partially-understood output + # is worse than refusing it: the caller gets a `***` that certifies the other columns. + output_provable: bool = False def as_dict(self) -> dict[str, Any]: return { @@ -470,6 +477,7 @@ def as_dict(self) -> dict[str, Any]: "reason": self.reason, "suggestion": self.suggestion, "projections": [p.as_dict() for p in self.projections], + "output_provable": self.output_provable, } @@ -521,6 +529,44 @@ def _direct_from_tables(tree: "exp.Select") -> set[str]: return names +def _output_lineage_provable(tree: "exp.Expression") -> bool: + """Could the sensitive-projection detector have seen EVERY value reaching the output? + + The detector matches sensitive columns by name against the tables in scope. That is + sound only while every output value traces back to a declared table it can name. Two + things break the trace, and both are common: + + * a derived table or CTE — `_output_selects` deliberately does not descend into their + bodies, so `(SELECT ssn FROM customers) t` presents an opaque `t` whose columns the + detector cannot attribute (and a rename inside makes even the name useless); + * a qualifier that does not resolve — `FROM customers C` puts `C` in scope while the + query says `c.ssn`, so the lookup misses and the column reads as un-sensitive. + + Returning False here does NOT refuse the query. It only withholds the *proof* that the + output is fully understood, which downgrades a mask decision to a refusal. That + distinction matters: this runs only after an offending projection has already been + found, so an unrelated query using a CTE is untouched — see the caller. + """ + 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 + + def _output_selects(node: "exp.Expression") -> list["exp.Select"]: """The SELECTs whose projection reaches the query OUTPUT: the top-level SELECT, or — for a set operation (UNION / INTERSECT / EXCEPT) — every arm. @@ -683,6 +729,7 @@ def check_sensitive_projection( suggestion="Aggregate it (e.g. COUNT(DISTINCT )) for a count, or omit it and " "select the entity's non-sensitive key (e.g. id) instead.", projections=projections, + output_provable=_output_lineage_provable(tree), ) @@ -1691,7 +1738,19 @@ def collect(node): def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]: if col.table: - return scope.get(col.table, col.table) + if col.table in scope: + return scope[col.table] + # SQL identifiers are case-insensitive unless quoted, so `FROM customers C` and `c.ssn` + # name the same table. A case-sensitive miss does not read as "unresolved" downstream — + # it falls through to the raw qualifier, which the sensitive gate then fails to match + # against its table index and treats as NOT sensitive. `4bd39bb` folded the COLUMN name + # for exactly this reason; the alias lookup is the other half of that fix. Exact match + # still wins, so a genuinely distinct quoted identifier is unaffected. + folded = col.table.lower() + for alias, name in scope.items(): + if alias.lower() == folded: + return name + return col.table # unqualified column: ambiguous; only safe to attribute if single table if len(scope) == 1: return next(iter(scope.values())) diff --git a/plugins/agami/lib/execute_sql.py b/plugins/agami/lib/execute_sql.py index 6f0e2285..ecc6e31b 100644 --- a/plugins/agami/lib/execute_sql.py +++ b/plugins/agami/lib/execute_sql.py @@ -1296,14 +1296,23 @@ def _model_safety( mask_plan: MaskPlan | None = None sens = RT.check_sensitive_projection(sql, org, ctx=ctx) if sens.action == "refuse": + # Two conditions, and BOTH are required. `all_maskable` quantifies over the projections + # the detector FOUND, so on its own it proves only "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 (anything detected blocked the + # whole query); once a maskable projection let the query RUN, one 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. `output_provable` is the + # missing half: the detector could see the whole output, not just part of it. all_maskable = bool(sens.projections) and all( p.maskable and p.output_index is not None for p in sens.projections ) + provable = all_maskable and sens.output_provable verdict = Verdict( cls="data_protection", rule="sensitive_projection", severity="high", - certainty="provable" if all_maskable else "uncertain", + certainty="provable" if provable else "uncertain", detail=sens.reason, remediation=sens.suggestion or "", ) diff --git a/tests/test_ace041_masking.py b/tests/test_ace041_masking.py index dc569cfb..18f19b07 100644 --- a/tests/test_ace041_masking.py +++ b/tests/test_ace041_masking.py @@ -456,11 +456,103 @@ def test_mask_note_helper_is_accurate_and_deterministic(): assert execute_sql._mask_note(("customers.ssn",), (0, 9), ["ssn"]) == ("customers.ssn",) +# --------------------------------------------------------------------------- +# CO-PROJECTION FAIL-CLOSED — the mask verdict must be a WHOLE-OUTPUT proof. +# +# `all_maskable` quantifies over the projections the DETECTOR FOUND, which proves "every +# offending projection we saw is maskable" — not "no sensitive value reaches the output". +# Under the old refuse-only behaviour a detector miss leaked only when NOTHING was detected, +# because anything detected blocked the whole query. Once a detected-and-maskable projection +# started letting the query RUN, one seen column began unblocking the entire result set — so +# every co-projected value the name-based detector missed came back raw, next to a `***` that +# makes it look like masking worked. +# +# These are the co-projection forms of the alias-lineage gap below. The standalone forms are +# genuinely pre-existing; these are NOT — each returned a raw SSN only after the refuse->mask +# flip. Refusing is the correct outcome: when the output cannot be proven clean, fail closed. +# --------------------------------------------------------------------------- + +_CO_PROJECTION_LEAK = [ + # a detected `ssn` beside the same column laundered through a derived table + "SELECT ssn, t.ssn FROM customers, (SELECT ssn FROM customers) t", + # ...through a CTE with a rename + "WITH q AS (SELECT ssn AS z FROM customers) SELECT c.ssn, q.z FROM customers c, q", +] + + +@pytest.mark.parametrize("sql", _CO_PROJECTION_LEAK) +def test_co_projected_unprovable_output_is_refused_not_partially_masked(guarded, sql): + """A raw sensitive value must never ride along beside a masked one. + + Each of these ran and returned `('***', '111-22-3333')` before the whole-output proof: + one column redacted, its twin in the clear. + """ + spy = _SpyExecutor(_R(["ssn", "ssn_2"], [("111-22-3333", "111-22-3333")])) + with pytest.raises(execute_sql.GuardRefused) as ei: + guarded(sql, spy) + assert ei.value.refusal.kind == "sensitive_columns" + assert not spy.calls, "an unprovable output must be refused BEFORE the executor runs" + + +@pytest.mark.parametrize( + "sql", + [ + "SELECT ssn FROM customers", + "SELECT c.ssn FROM customers c", + 'SELECT "SSN" FROM customers', + "SELECT name, ssn FROM customers", + "SELECT ssn FROM customers UNION ALL SELECT ssn FROM customers", + # Case-mismatched alias: `FROM customers C` + `c.ssn`. `4bd39bb` folded the COLUMN + # name; the table ALIAS lookup was the other half. Once it resolves, this is an + # ordinary provable projection — so it MASKS rather than refusing, which is strictly + # better than the fail-closed outcome and better still than the raw value it returned. + "SELECT ssn, c.ssn FROM customers C", + "SELECT C.ssn FROM customers c", + ], +) +def test_provable_output_still_masks(guarded, sql): + """The whole-output proof must not collapse ACE-041 back into refuse-everything. + + Every projection here resolves to a declared table, so the output IS provable and the + feature works as specified: the query runs and the sensitive column is redacted. + """ + # Every cell carries the raw value, so whichever index the plan targets, a successful mask + # is observable without the fixture having to mirror each query's projection order. + spy = _SpyExecutor(_R(["a", "b"], [("111-22-3333", "111-22-3333")])) + result = guarded(sql, spy) + assert spy.calls, f"a provable maskable projection must still RUN: {sql!r}" + assert result.masked_columns, f"a provable projection must be MASKED, not passed through: {sql!r}" + assert execute_sql.REDACTION_TOKEN in [v for row in result.rows for v in row] + + +@pytest.mark.parametrize( + "sql", + [ + "SELECT COUNT(ssn) FROM customers", + "SELECT COUNT(*) FROM customers WHERE ssn IS NOT NULL", + "SELECT COUNT(*) FROM customers c, (SELECT id FROM orders) o WHERE c.ssn IS NOT NULL", + ], +) +def test_no_over_refusal_when_nothing_sensitive_is_projected(guarded, sql): + """Aggregate/filter use of a sensitive column stays allowed, derived table or not. + + The proof runs ONLY once an offending projection has been 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, which the proof would otherwise judge unprovable. + """ + spy = _SpyExecutor(_R(["n"], [(3,)])) + result = guarded(sql, spy) + assert spy.calls, f"aggregate/filter use must not be refused: {sql!r}" + assert result.rows == [(3,)] + + # --------------------------------------------------------------------------- # KNOWN LIMITATION (ACE-041 review, finding 2): a sensitive column RENAMED inside a derived-table / # CTE body escapes both mask and refuse, because `_output_selects` excludes subquery/CTE bodies and # the sensitive match is name-based with no alias/lineage tracking. This is PRE-EXISTING (not -# introduced by ACE-041). These xfail tests assert the DESIRED secure behaviour (the outer projection +# introduced by ACE-041) in its STANDALONE form — nothing sensitive is detected at all, so the +# whole-output proof above never engages. (Its CO-PROJECTION form was new, and is closed above.) +# These xfail tests assert the DESIRED secure behaviour (the outer projection # is masked OR refused); they xfail today and will flip to xpass when a future alias-lineage fix # lands — that flip is the tracking signal. Do NOT "fix" them by weakening the assertion. See the # `# ACE-041 known limitation` comment in semantic_model/runtime.py::_output_selects. From 0656442d39eef47eaabdb7e5e7abd265dcb087a5 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 17:01:56 -0700 Subject: [PATCH 2/4] fix(pii): make the whole-output proof sound, and scope it to the output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 22 ++ packages/agami-core/src/execute_sql.py | 6 +- .../agami-core/src/semantic_model/runtime.py | 161 ++++++++++---- plugins/agami/lib/execute_sql.py | 6 +- tests/test_ace041_masking.py | 206 ++++++++++++++++-- 5 files changed, 336 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9244525..0fc5652d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,28 @@ below corresponds to one such version. ### Security +- **Required a whole-output proof before masking a sensitive projection, instead of + refusing.** The mask decision quantified over the projections the detector *found* + (`all_maskable`), which proves "every offending projection we saw is maskable" but never + "no sensitive value reaches the output". While any detected projection refused the whole + query that gap was harmless; once a maskable projection let the query **run**, one 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. + `SensitiveCheckResult.output_provable` is the missing half: it is False when an + output-bearing arm draws through a source whose body is not walked (a derived table, a CTE + the arm selects from, a `VALUES`/`LATERAL` source), 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 to a refusal. Two further binding bugs made the proof + itself unsound and are closed with it: a subquery alias could **shadow** an outer alias of + the same name (`FROM customers t` outside, `FROM orders t` inside an `EXISTS` body), silently + rebinding the outer `t.ssn` to a table declaring no `ssn` so it was never detected while the + qualifier still "resolved"; and two aliases folding to the same name (`FROM x "AB", customers + ab` with `AB.ssn`) resolved by exact-match-first to the quoted one, where the engine folds + onto the other. Alias binding is now shadow-aware and fails closed on a genuinely ambiguous + fold. The proof is scoped to what can actually put a value in the output, so a subquery in + `WHERE`/`HAVING`/`ORDER BY`, a JOIN's `ON` condition, or a CTE no arm selects from no longer + downgrades an ordinary maskable query to a refusal. + - **Closed a read-only-guard bypass via a welded quoted identifier.** A double-quoted identifier is self-delimiting in SQL on **both** ends, so `SELECT*FROM"pg_read_file"(…)` and `SELECT "x"INTO evil FROM t` are valid statements with no whitespace either side of diff --git a/packages/agami-core/src/execute_sql.py b/packages/agami-core/src/execute_sql.py index ecc6e31b..04fa54b9 100644 --- a/packages/agami-core/src/execute_sql.py +++ b/packages/agami-core/src/execute_sql.py @@ -1307,7 +1307,11 @@ def _model_safety( all_maskable = bool(sens.projections) and all( p.maskable and p.output_index is not None for p in sens.projections ) - provable = all_maskable and sens.output_provable + # `getattr`, not attribute access: this file is VENDORED into the plugin while + # semantic_model/runtime.py resolves from the separately-versioned installed package, so a + # newer plugin can meet an older `SensitiveCheckResult`. A missing field then means "this + # build cannot prove the output", which is exactly the fail-closed answer. + provable = all_maskable and getattr(sens, "output_provable", False) verdict = Verdict( cls="data_protection", rule="sensitive_projection", diff --git a/packages/agami-core/src/semantic_model/runtime.py b/packages/agami-core/src/semantic_model/runtime.py index 3a32f115..4e6e15e3 100644 --- a/packages/agami-core/src/semantic_model/runtime.py +++ b/packages/agami-core/src/semantic_model/runtime.py @@ -457,17 +457,22 @@ class SensitiveCheckResult: reason: str = "" suggestion: Optional[str] = None # Per-offending-projection traceability (ACE-041): the maskable-vs-must-refuse decision and, for - # maskable ones, the output-column index. Latent in THIS slice (the caller still refuses on any - # sensitive projection); a later masking slice reads it. Deterministic order: arm order, then - # projection index; duplicates and multi-arm collisions on `output_index` are preserved (unlike - # `columns`, which is the de-duplicated sorted set the refuse message is built from). + # maskable ones, the output-column index. LOAD-BEARING, not informational — `_model_safety` + # reads `maskable` / `output_index` off this list to build `MaskPlan.indices`, i.e. the exact + # output columns that get redacted, so order and duplicates are significant and must not be + # re-sorted or de-duplicated. Deterministic order: arm order, then projection index; duplicates + # and multi-arm collisions on `output_index` are preserved (unlike `columns`, which is the + # de-duplicated sorted set the refuse message is built from). projections: list["SensitiveProjection"] = field(default_factory=list) # Whether the WHOLE output was resolvable, not just whether each FOUND projection was # maskable (ACE-041 review). `projections` can only describe what the detector saw; this - # says whether it could have seen everything. False when an output-bearing arm draws - # through a derived table / CTE, or projects a column whose qualifier does not resolve — - # i.e. exactly where the name-based match is blind. Masking a partially-understood output - # is worse than refusing it: the caller gets a `***` that certifies the other columns. + # says whether it could have seen everything. False when an output-bearing arm draws through + # a source whose body is not walked (a derived table, a CTE the arm selects from, or a + # non-table source such as VALUES / LATERAL), or projects a scalar subquery, or projects a + # column whose qualifier names none of that arm's own sources — i.e. exactly where the + # name-based match is blind. See `_output_lineage_provable` for why a bare column needs no + # such test. Masking a partially-understood output is worse than refusing it: the caller gets + # a `***` that certifies the other columns. output_provable: bool = False def as_dict(self) -> dict[str, Any]: @@ -529,41 +534,105 @@ def _direct_from_tables(tree: "exp.Select") -> set[str]: return names -def _output_lineage_provable(tree: "exp.Expression") -> bool: - """Could the sensitive-projection detector have seen EVERY value reaching the output? +def _arm_sources(sel: "exp.Select") -> list["exp.Expression"]: + """The FROM / JOIN clauses belonging to THIS select, never a nested one. - The detector matches sensitive columns by name against the tables in scope. That is - sound only while every output value traces back to a declared table it can name. Two - things break the trace, and both are common: + Read off `sel`'s own args by NODE TYPE rather than by arg key: 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. Matching on `exp.From` / + `exp.Join` is stable across that range.""" + out: list["exp.Expression"] = [] + for value in sel.args.values(): + for item in value if isinstance(value, list) else [value]: + if isinstance(item, (exp.From, exp.Join)): + out.append(item) + return out - * a derived table or CTE — `_output_selects` deliberately does not descend into their - bodies, so `(SELECT ssn FROM customers) t` presents an opaque `t` whose columns the - detector cannot attribute (and a rename inside makes even the name useless); - * a qualifier that does not resolve — `FROM customers C` puts `C` in scope while the - query says `c.ssn`, so the lookup misses and the column reads as un-sensitive. - Returning False here does NOT refuse the query. It only withholds the *proof* that the - output is fully understood, which downgrades a mask decision to a refusal. That - distinction matters: this runs only after an offending projection has already been - found, so an unrelated query using a CTE is untouched — see the caller. - """ - if tree.find(exp.CTE) is not None: - return False # a CTE body is not walked, so its output columns cannot be attributed +def _direct_scope(sel: "exp.Select") -> dict[str, str]: + """alias (or table name) -> table, for THIS arm's own FROM/JOIN sources only. + + Used by the whole-output proof to decide whether a projected qualifier names one of the + sources the arm actually draws from. Deliberately narrower than `_sensitive_scope`.""" + scope: dict[str, str] = {} + for src in _arm_sources(sel): + node = src.this + if isinstance(node, exp.Table): + scope[node.alias_or_name] = node.name + return scope + + +def _sensitive_scope(sel: "exp.Select") -> dict[str, Optional[str]]: + """alias -> table for the sensitive gate, with a SHADOWED alias resolved to None. + + Same whole-subtree walk as `_tables_in_scope`, so a bare column still resolves through a + derived-table or CTE body exactly as it did before. The one difference is what happens when a + single alias is bound to two different tables — `FROM customers t` outside and `FROM orders t` + inside an EXISTS body: + + SELECT c.ssn, t.ssn FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t) + + A last-one-wins map silently rebinds the OUTER `t.ssn` to `orders`, which declares no `ssn`, + so the column reads as un-sensitive and is never detected — while the qualifier still + "resolves", so the output reads as provable and the query masks one column and hands back its + twin raw. A subquery alias is scoped to its own body and must not capture an outer qualifier. + Mapping the conflict to None makes the binding UNRESOLVED, which `_sensitive_col_ref` already + handles by falling back to the conservative folded bare name (offending, and maskable).""" + bound: dict[str, set[str]] = {} + for tbl in sel.find_all(exp.Table): + bound.setdefault(tbl.alias_or_name, set()).add(tbl.name) + return {alias: (next(iter(t)) if len(t) == 1 else None) for alias, t in bound.items()} + + +def _output_lineage_provable(tree: "exp.Expression") -> bool: + """Could the sensitive-projection detector have seen EVERY value reaching the output? + + The detector matches sensitive columns by name against the tables in scope. That is sound + only while every output value traces back to a declared table it can name. Two things break + the trace: + + * an OUTPUT-BEARING source whose body is not walked — a derived table, a CTE the arm + selects from, or a non-table source (VALUES / LATERAL / table function). `(SELECT ssn + FROM customers) t` presents an opaque `t` whose columns cannot be attributed, and a + rename inside makes even the name useless; + * a qualifier that names nothing among the arm's own sources, so the column cannot be + bound to a table at all. + + Both tests are deliberately scoped to what can actually PUT A VALUE IN THE OUTPUT: an arm's + own FROM/JOIN sources, and its projection list. A subquery in WHERE / HAVING / ORDER BY, or a + JOIN's ON condition, yields a filter rather than a column, and a CTE no arm selects from + contributes nothing — withholding the proof for those refuses ordinary analytics SQL and buys + no safety. (This mirrors the distinction `check_column_scope` already draws.) + + A BARE column needs no test here. The sensitive match is by NAME against the whole model, so + it does not 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 is declared sensitive nowhere and no binding could make it + sensitive. Refusing on ambiguity alone would reject every unqualified column in a join. + + Returning False here does NOT refuse the query. It only withholds the *proof* that the output + is fully understood, which downgrades a mask decision to a refusal. This runs only after an + offending projection has already been found, so a query that projects nothing sensitive never + reaches it — see the caller.""" + cte_names = {cte.alias_or_name.lower() for cte in tree.find_all(exp.CTE)} 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) + for src in _arm_sources(sel): + node = src.this + if not isinstance(node, exp.Table): + return False # derived table / VALUES / LATERAL / table function: body not walked + if node.name.lower() in cte_names: + return False # a CTE body is not walked either, and a rename inside hides the name + scope = _direct_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: + if proj.find(exp.Subquery) is not None: + return False # a scalar subquery in the PROJECTION does reach the output 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 + if col.table and col.table.lower() not in lowered: + return False # qualifier names nothing among this arm's own sources return True @@ -710,7 +779,10 @@ def check_sensitive_projection( offending: set[str] = set() projections: list[SensitiveProjection] = [] for sel in _output_selects(tree): - scope = _tables_in_scope(sel) + # `_sensitive_scope`, NOT `_tables_in_scope`: a last-one-wins alias map lets an alias + # declared inside a subquery overwrite the outer alias of the same name, which binds an + # outer qualified column to the wrong table and hides it from the match entirely. + scope = _sensitive_scope(sel) direct = _direct_from_tables(sel) for index, proj in enumerate(sel.expressions): _classify_projection( @@ -1738,18 +1810,23 @@ def collect(node): def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]: if col.table: - if col.table in scope: - return scope[col.table] # SQL identifiers are case-insensitive unless quoted, so `FROM customers C` and `c.ssn` # name the same table. A case-sensitive miss does not read as "unresolved" downstream — # it falls through to the raw qualifier, which the sensitive gate then fails to match - # against its table index and treats as NOT sensitive. `4bd39bb` folded the COLUMN name - # for exactly this reason; the alias lookup is the other half of that fix. Exact match - # still wins, so a genuinely distinct quoted identifier is unaffected. + # against its table index and treats as NOT sensitive. ACE-041 slice 3 (`4bd39bb`) folded + # the COLUMN name for exactly this reason; the alias lookup is the other half of that fix. + # + # Resolve by FOLDED match rather than exact-match-first. When two aliases in scope fold + # together — `FROM x "AB", customers ab` with `AB.ssn` — an exact hit on the quoted `"AB"` + # would bind to `x`, while the engine folds the unquoted `AB` onto `ab` and reads + # `customers`. The binding is genuinely ambiguous, so return None (unresolved) and let the + # caller fail closed; picking either one is a coin flip that can read as un-sensitive. folded = col.table.lower() - for alias, name in scope.items(): - if alias.lower() == folded: - return name + matches = {name for alias, name in scope.items() if alias.lower() == folded} + if len(matches) == 1: + return next(iter(matches)) + if matches: + return None # ambiguous binding -> unresolved, never a guess return col.table # unqualified column: ambiguous; only safe to attribute if single table if len(scope) == 1: diff --git a/plugins/agami/lib/execute_sql.py b/plugins/agami/lib/execute_sql.py index ecc6e31b..04fa54b9 100644 --- a/plugins/agami/lib/execute_sql.py +++ b/plugins/agami/lib/execute_sql.py @@ -1307,7 +1307,11 @@ def _model_safety( all_maskable = bool(sens.projections) and all( p.maskable and p.output_index is not None for p in sens.projections ) - provable = all_maskable and sens.output_provable + # `getattr`, not attribute access: this file is VENDORED into the plugin while + # semantic_model/runtime.py resolves from the separately-versioned installed package, so a + # newer plugin can meet an older `SensitiveCheckResult`. A missing field then means "this + # build cannot prove the output", which is exactly the fail-closed answer. + provable = all_maskable and getattr(sens, "output_provable", False) verdict = Verdict( cls="data_protection", rule="sensitive_projection", diff --git a/tests/test_ace041_masking.py b/tests/test_ace041_masking.py index 18f19b07..1e34c9a4 100644 --- a/tests/test_ace041_masking.py +++ b/tests/test_ace041_masking.py @@ -495,33 +495,41 @@ def test_co_projected_unprovable_output_is_refused_not_partially_masked(guarded, @pytest.mark.parametrize( - "sql", + "sql,columns,row", [ - "SELECT ssn FROM customers", - "SELECT c.ssn FROM customers c", - 'SELECT "SSN" FROM customers', - "SELECT name, ssn FROM customers", - "SELECT ssn FROM customers UNION ALL SELECT ssn FROM customers", - # Case-mismatched alias: `FROM customers C` + `c.ssn`. `4bd39bb` folded the COLUMN - # name; the table ALIAS lookup was the other half. Once it resolves, this is an - # ordinary provable projection — so it MASKS rather than refusing, which is strictly - # better than the fail-closed outcome and better still than the raw value it returned. - "SELECT ssn, c.ssn FROM customers C", - "SELECT C.ssn FROM customers c", + ("SELECT ssn FROM customers", ["ssn"], ("111-22-3333",)), + ("SELECT c.ssn FROM customers c", ["ssn"], ("111-22-3333",)), + ('SELECT "SSN" FROM customers', ["SSN"], ("111-22-3333",)), + ("SELECT name, ssn FROM customers", ["name", "ssn"], ("Alice", "111-22-3333")), + ("SELECT ssn FROM customers UNION ALL SELECT ssn FROM customers", ["ssn"], + ("111-22-3333",)), + # Case-mismatched alias: `FROM customers C` + `c.ssn`. ACE-041 slice 3 (`4bd39bb`) folded + # the COLUMN name; the table ALIAS lookup was the other half. Once it resolves, this is an + # ordinary provable projection, so it masks rather than refusing — which restores the + # intended ACE-041 outcome. Before the alias fold, `SELECT C.ssn FROM customers c` + # returned the raw value with no mask at all. + ("SELECT ssn, c.ssn FROM customers C", ["ssn", "ssn_2"], + ("111-22-3333", "111-22-3333")), + ("SELECT C.ssn FROM customers c", ["ssn"], ("111-22-3333",)), ], ) -def test_provable_output_still_masks(guarded, sql): +def test_provable_output_still_masks(guarded, sql, columns, row): """The whole-output proof must not collapse ACE-041 back into refuse-everything. Every projection here resolves to a declared table, so the output IS provable and the feature works as specified: the query runs and the sensitive column is redacted. """ - # Every cell carries the raw value, so whichever index the plan targets, a successful mask - # is observable without the fixture having to mirror each query's projection order. - spy = _SpyExecutor(_R(["a", "b"], [("111-22-3333", "111-22-3333")])) + # The fixture mirrors each query's real projection shape, with the raw secret ONLY in cells the + # engine would fill with a sensitive value. That is what makes "no raw value survives" + # assertable — a fixture that puts the secret in every cell can only ever check that a token + # appeared SOMEWHERE, which passes even when the plan masks the wrong column. + spy = _SpyExecutor(_R(columns, [tuple(row)])) result = guarded(sql, spy) assert spy.calls, f"a provable maskable projection must still RUN: {sql!r}" assert result.masked_columns, f"a provable projection must be MASKED, not passed through: {sql!r}" + flat = [v for r in result.rows for v in r] + assert execute_sql.REDACTION_TOKEN in flat, f"must be masked: {sql!r}" + assert "111-22-3333" not in flat, f"raw value survived: {result.rows!r} for {sql!r}" assert execute_sql.REDACTION_TOKEN in [v for row in result.rows for v in row] @@ -533,12 +541,15 @@ def test_provable_output_still_masks(guarded, sql): "SELECT COUNT(*) FROM customers c, (SELECT id FROM orders) o WHERE c.ssn IS NOT NULL", ], ) -def test_no_over_refusal_when_nothing_sensitive_is_projected(guarded, sql): +def test_aggregate_or_filter_use_returns_before_the_proof(guarded, sql): """Aggregate/filter use of a sensitive column stays allowed, derived table or not. - The proof runs ONLY once an offending projection has been 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, which the proof would otherwise judge unprovable. + Note what this does and does NOT cover. Every case here ends at `action="allow"`, so it + returns BEFORE the whole-output proof is ever consulted — it pins the early-return path, not + the proof. The over-refusal direction (a query that DOES project a sensitive column and also + contains unrelated nesting) is pinned separately by + `test_unrelated_nesting_still_masks`, which is the test that would catch the proof + being widened back out. """ spy = _SpyExecutor(_R(["n"], [(3,)])) result = guarded(sql, spy) @@ -551,7 +562,14 @@ def test_no_over_refusal_when_nothing_sensitive_is_projected(guarded, sql): # CTE body escapes both mask and refuse, because `_output_selects` excludes subquery/CTE bodies and # the sensitive match is name-based with no alias/lineage tracking. This is PRE-EXISTING (not # introduced by ACE-041) in its STANDALONE form — nothing sensitive is detected at all, so the -# whole-output proof above never engages. (Its CO-PROJECTION form was new, and is closed above.) +# whole-output proof above never engages. +# +# Its CO-PROJECTION form WAS new, and is closed above — but closed by REFUSAL, not by detection. +# The detector still never sees the laundered column (it reports only the plainly-projected +# `customers.ssn`); what stops the leak is that an arm drawing on a derived table or a CTE cannot +# be proven, so the statement is refused. That coupling matters: narrowing the opaque-source test +# in `_output_lineage_provable` would silently re-open these leaks while every test here still +# passes. `test_opaque_output_source_is_still_refused` is what pins it. # These xfail tests assert the DESIRED secure behaviour (the outer projection # is masked OR refused); they xfail today and will flip to xpass when a future alias-lineage fix # lands — that flip is the tracking signal. Do NOT "fix" them by weakening the assertion. See the @@ -726,3 +744,149 @@ def test_real_subprocess_fork_masks_ssn_in_the_emitted_csv(tmp_path): masked = [json.loads(x)["masked"] for x in proc.stderr.splitlines() if x.strip().startswith("{") and '"masked"' in x] assert masked == [["customers.ssn"]] + + +# --------------------------------------------------------------------------- +# WHOLE-OUTPUT PROOF — adversarial matrix (PR #155 review). +# +# The proof answers one question: could the detector have seen EVERY value reaching the output? +# Two failure directions, and both must be pinned, because a fix for either one silently breaks +# the other: +# +# * UNSOUND — `output_provable` True while a sensitive value reaches the output unseen. That is +# the `('***', raw)` receipt this whole feature exists to prevent, so it is the direction that +# must never regress. Caused by the detector RESOLVING A QUALIFIER TO THE WRONG TABLE. +# * OVER-BROAD — the proof withholds itself for a construct that cannot contribute an output +# value at all (a WHERE/HAVING subquery, a CTE nothing selects from), refusing an ordinary +# query that ACE-041 is supposed to mask. +# +# The fixtures below put the raw secret ONLY in cells the engine would really fill with a sensitive +# value, and a distinct sentinel elsewhere, so "no raw value survives" is directly assertable +# instead of being inferred from the presence of a token somewhere in the row. +# --------------------------------------------------------------------------- + +_SECRET = "111-22-3333" +_SAFE = "not-sensitive" + + +def _assert_no_raw_value(guarded, sql, columns, row): + """Run `sql` and assert the raw sensitive value never reaches the caller. + + Either outcome is secure: refusing the statement, or running it with every sensitive cell + redacted. What is NOT acceptable is a raw secret in the result — with or without a `***` + beside it, which is the partial-mask receipt that reads as "this row was protected". + """ + spy = _SpyExecutor(_R(columns, [tuple(row)])) + try: + result = guarded(sql, spy) + except execute_sql.GuardRefused: + return # fail-closed is a secure outcome + flat = [v for r in result.rows for v in r] + assert _SECRET not in flat, f"raw sensitive value survived: {result.rows!r} for {sql!r}" + + +# An alias declared in a subquery must not capture a qualifier belonging to the OUTER query. The +# detector resolves `t.ssn` through its alias->table map; if the map is built over the whole tree, +# the inner `FROM orders t` overwrites the outer `t -> customers` and the outer `t.ssn` is judged a +# column of `orders` (which declares no `ssn`), so it is never detected. Every qualifier still +# "resolves", so the proof reports provable and the query masks ONE column and returns the other raw. +_ALIAS_SHADOWING_LEAK = [ + # `EXISTS (...)` parses to exp.Exists wrapping a bare Select — NO exp.Subquery node — so a + # derived-table check keyed on that node type never fires here. + ("SELECT c.ssn, t.ssn FROM customers c, customers t WHERE EXISTS (SELECT 1 FROM orders t)", + ["ssn", "ssn_2"], (_SECRET, _SECRET)), + ("SELECT c.ssn, t.ssn FROM customers c, customers t WHERE NOT EXISTS (SELECT 1 FROM x t)", + ["ssn", "ssn_2"], (_SECRET, _SECRET)), + # Same root cause with no co-projection to unblock the query: the single projection resolves to + # the wrong table, nothing is detected at all, and the raw value flows straight through. + ("SELECT t.ssn FROM customers t WHERE EXISTS (SELECT 1 FROM orders t)", + ["ssn"], (_SECRET,)), + # A JOIN body, not a WHERE body — same shadowing, different clause. + ("SELECT c.ssn, t.ssn FROM customers c JOIN customers t ON t.id = c.id " + "WHERE EXISTS (SELECT 1 FROM x t)", ["ssn", "ssn_2"], (_SECRET, _SECRET)), +] + + +@pytest.mark.parametrize("sql,columns,row", _ALIAS_SHADOWING_LEAK) +def test_subquery_alias_must_not_shadow_an_outer_qualifier(guarded, sql, columns, row): + _assert_no_raw_value(guarded, sql, columns, row) + + +# A quoted alias must not capture an unquoted qualifier that the ENGINE folds onto a different +# table. `FROM x "AB", customers ab` + `AB.ssn`: Postgres folds the unquoted `AB` to `ab` and reads +# `customers`, but an exact-match-first lookup binds it to the quoted `"AB"` -> `x`, whose `ssn` is +# not declared sensitive. When two aliases in scope fold together the binding is genuinely +# ambiguous, and an ambiguous binding must fail closed, not pick one. +_QUOTED_ALIAS_COLLISION = [ + ('SELECT c.ssn, AB.ssn FROM customers c, x "AB", customers ab', + ["ssn", "ssn_2"], (_SECRET, _SECRET)), + ('SELECT AB.ssn FROM x "AB", customers ab', ["ssn"], (_SECRET,)), +] + + +@pytest.mark.parametrize("sql,columns,row", _QUOTED_ALIAS_COLLISION) +def test_case_colliding_aliases_fail_closed(guarded, sql, columns, row): + _assert_no_raw_value(guarded, sql, columns, row) + + +# The proof must engage ONLY for constructs that can actually put a value in the output. A subquery +# in WHERE/HAVING/ORDER BY yields a filter, not a column; a CTE nothing selects from contributes +# nothing at all. Refusing these buys no safety and rolls back the feature for ordinary analytics +# SQL, which is what LLM-generated queries look like. +_MASKABLE_DESPITE_UNRELATED_NESTING = [ + # WHERE subquery — the archetypal false refusal. + ("SELECT ssn FROM customers WHERE id IN (SELECT cust_id FROM orders)", ["ssn"], (_SECRET,)), + ("SELECT c.ssn FROM customers c WHERE c.id IN (SELECT cust_id FROM orders)", ["ssn"], (_SECRET,)), + ("SELECT ssn FROM customers c WHERE EXISTS (SELECT 1 FROM orders o WHERE o.cust_id = c.id)", + ["ssn"], (_SECRET,)), + # HAVING / ORDER BY subqueries. + ("SELECT ssn FROM customers GROUP BY ssn HAVING COUNT(*) > (SELECT COUNT(*) FROM orders)", + ["ssn"], (_SECRET,)), + ("SELECT ssn FROM customers ORDER BY (SELECT COUNT(*) FROM orders)", ["ssn"], (_SECRET,)), + # A CTE that no output arm selects from cannot launder anything. + ("WITH unused AS (SELECT id FROM orders) SELECT ssn FROM customers", ["ssn"], (_SECRET,)), + # An ordinary two-table join with an unqualified projection. A bare column is matched by NAME + # against the sensitive set, which does not depend on which table it binds to, so ambiguity here + # cannot hide a sensitive value — `_sensitive_col_ref` already falls back to the folded bare name. + ("SELECT name, ssn FROM customers JOIN orders ON customers.id = orders.cust_id", + ["name", "ssn"], (_SAFE, _SECRET)), + ("SELECT ssn FROM customers a, customers b", ["ssn"], (_SECRET,)), +] + + +@pytest.mark.parametrize("sql,columns,row", _MASKABLE_DESPITE_UNRELATED_NESTING) +def test_unrelated_nesting_still_masks(guarded, sql, columns, row): + """Nesting that cannot reach the output must not downgrade a maskable projection to a refusal.""" + spy = _SpyExecutor(_R(columns, [tuple(row)])) + result = guarded(sql, spy) + assert spy.calls, f"must RUN, not refuse: {sql!r}" + flat = [v for r in result.rows for v in r] + assert execute_sql.REDACTION_TOKEN in flat, f"must be masked: {sql!r}" + assert _SECRET not in flat, f"raw value survived: {result.rows!r} for {sql!r}" + + +# The narrowing above must not re-open what the whole-output proof closed. Each of these puts an +# output-bearing arm behind a body the detector does not walk, so the statement must still refuse. +_OUTPUT_BEARING_OPAQUE_SOURCE = [ + # derived table in FROM + "SELECT ssn, t.ssn FROM customers, (SELECT ssn FROM customers) t", + # CTE actually selected from by the output arm + "WITH q AS (SELECT ssn AS z FROM customers) SELECT c.ssn, q.z FROM customers c, q", + # a set-operation arm whose source is opaque, the other arm clean + "SELECT ssn FROM customers UNION ALL SELECT z FROM (SELECT ssn AS z FROM customers) q", + "WITH q AS (SELECT ssn AS z FROM customers) SELECT ssn FROM customers UNION ALL SELECT z FROM q", + # nested two deep (no `*` — the star ban would refuse that one first, for a different reason) + "SELECT ssn, t.z FROM customers, (SELECT z FROM (SELECT ssn AS z FROM customers) i) t", + # a scalar subquery in the PROJECTION list can contribute an output value, and a rename inside + # it defeats the name-based match, so it stays opaque even though it is not a FROM source + "SELECT ssn, (SELECT z FROM (SELECT ssn AS z FROM customers) i LIMIT 1) AS leaked FROM customers", +] + + +@pytest.mark.parametrize("sql", _OUTPUT_BEARING_OPAQUE_SOURCE) +def test_opaque_output_source_is_still_refused(guarded, sql): + spy = _SpyExecutor(_R(["a", "b"], [(_SECRET, _SECRET)])) + with pytest.raises(execute_sql.GuardRefused) as ei: + guarded(sql, spy) + assert ei.value.refusal.kind == "sensitive_columns" + assert not spy.calls, "an unprovable output must be refused BEFORE the executor runs" From 308e058dbed89ac6e5cc92156fbfb94be5260f9c Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 17:13:37 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix(test):=20repair=20the=20e2e=20harness?= =?UTF-8?q?=20call=20renamed=20by=20ACE-067=E2=80=93070?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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`. --- tests/e2e/harness.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index d58d8d8b..7a5c11c8 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -120,7 +120,7 @@ def seed_db_model(url: str, ds: str = "acme") -> None: s = Store.connect(url) s.run_migrations() - model_store.write_organization(s, ds, build_org()) + model_store.write_datasource(s, ds, build_org()) s.close() From a9a8393bc0c36df1c1ec8b008c3b4f589be3dee4 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 18:05:46 -0700 Subject: [PATCH 4/4] fix(types): annotate the shadow-aware scope, and correct the changelog header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 4 ++-- packages/agami-core/src/semantic_model/runtime.py | 13 ++++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fc5652d..44f93766 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,8 +14,8 @@ below corresponds to one such version. ### Security -- **Required a whole-output proof before masking a sensitive projection, instead of - refusing.** The mask decision quantified over the projections the detector *found* +- **Gated PII masking on a whole-output proof; an output that cannot be proven clean is + now refused.** The mask decision quantified over the projections the detector *found* (`all_maskable`), which proves "every offending projection we saw is maskable" but never "no sensitive value reaches the output". While any detected projection refused the whole query that gap was harmless; once a maskable projection let the query **run**, one seen diff --git a/packages/agami-core/src/semantic_model/runtime.py b/packages/agami-core/src/semantic_model/runtime.py index 4e6e15e3..59e861cb 100644 --- a/packages/agami-core/src/semantic_model/runtime.py +++ b/packages/agami-core/src/semantic_model/runtime.py @@ -31,6 +31,7 @@ from __future__ import annotations import re +from collections.abc import Mapping from dataclasses import dataclass, field from difflib import SequenceMatcher from typing import Any, Callable, Optional @@ -668,7 +669,7 @@ def _output_selects(node: "exp.Expression") -> list["exp.Select"]: def _sensitive_col_ref( col: "exp.Column", - scope: dict[str, str], + scope: Mapping[str, Optional[str]], by_table: dict[str, dict[str, str]], allnames: set[str], ) -> Optional[str]: @@ -694,7 +695,7 @@ def _sensitive_col_ref( def _classify_projection( proj: "exp.Expression", index: int, - scope: dict[str, str], + scope: Mapping[str, Optional[str]], direct: set[str], by_table: dict[str, dict[str, str]], allnames: set[str], @@ -1808,7 +1809,13 @@ def collect(node): return referenced -def _resolve_col_table(col: "exp.Column", scope: dict[str, str]) -> Optional[str]: +def _resolve_col_table(col: "exp.Column", scope: Mapping[str, Optional[str]]) -> Optional[str]: + """The table `col` binds to, or None when that cannot be decided. + + `scope` values are Optional because the sensitive gate passes `_sensitive_scope`, which maps a + SHADOWED alias (one bound to two different tables) to None on purpose. `Mapping` rather than + `dict` so the fan/chasm and aggregation callers can keep passing a plain `dict[str, str]` — a + Mapping's value type is covariant, a dict's is not.""" if col.table: # SQL identifiers are case-insensitive unless quoted, so `FROM customers C` and `c.ssn` # name the same table. A case-sensitive miss does not read as "unresolved" downstream —