diff --git a/tests/unit/test_policy_query_filter.py b/tests/unit/test_policy_query_filter.py new file mode 100644 index 00000000..afb90df3 --- /dev/null +++ b/tests/unit/test_policy_query_filter.py @@ -0,0 +1,167 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Regression tests: a policy query's quoted constants must FILTER rows (#326). + +``policy_result_line`` printed the whole extent of the policy predicate no +matter which entity the query pinned, so ``needs_review("Bob", R)?`` reported +three rows for a Bob the engine inferred NOTHING about — other subjects' reasons +rendered as Bob's. The report is the artifact SKILL.md tells the reader to show +verbatim before stating a conclusion, so a fabricated positive there is not a +cosmetic defect. + +The filter must apply at EVERY argument position, not just the first: fixing +only ``args[0]`` would leave ``pred(E, "stale")?`` mis-attributing rows while +making the first-argument form accurate — which removes the only signal a reader +had that the second line is untrustworthy. + +``ask_router.evaluate`` (the ``ask`` path) had the same defect at positions +other than the first. The parity class below pins the two paths together. + +FACTLOG_ROOT is bound to a throwaway dir by ``tests/unit/conftest.py`` BEFORE +any tool module is imported, which matters here: ``ask_router`` re-exports +``FACTLOG_ROOT`` at import time from argv/env, so without that pin the parity +test would read the developer's real knowledge base. +""" +from __future__ import annotations + +import re +import unicodedata + +import pytest + +import ask_router +import run_logic_check as rlc + +PREDICATE = "needs_review" +INFERRED = { + PREDICATE: { + ("Alice", "low_conf"), + ("Carol", "stale"), + ("Dave", "no_source"), + } +} + + +def _row_count(report_line: str) -> int: + """Extract N from ' results ...: N rows; ...'.""" + match = re.search(r"(\d+) rows", report_line) + assert match, f"unparseable result line: {report_line!r}" + return int(match.group(1)) + + +def _report_rows(draft: str, inferred=None) -> int: + return _row_count(rlc.policy_result_line(PREDICATE, draft, inferred or INFERRED)) + + +def _router_rows(monkeypatch, draft: str, inferred=None) -> int: + monkeypatch.setattr(ask_router, "policy_predicates", lambda program: {PREDICATE}) + monkeypatch.setattr(ask_router, "run_wirelog", lambda: inferred or INFERRED) + return ask_router.evaluate(draft, [])["count"] + + +class TestPolicyResultLineFiltersFixedEntity: + def test_named_entity_reports_only_its_own_rows(self): + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}("Alice", R)?', INFERRED) + assert _row_count(line) == 1 + assert "low_conf" in line + assert "stale" not in line and "no_source" not in line + + def test_entity_with_no_inferred_rows_reports_zero(self): + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}("Bob", R)?', INFERRED) + assert _row_count(line) == 0 + assert "low_conf" not in line and "stale" not in line and "no_source" not in line + + def test_second_argument_constant_also_filters(self): + # The defect is not first-argument-specific: a reason-pinned query must + # not report the other reasons' rows either. + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}(E, "stale")?', INFERRED) + assert _row_count(line) == 1 + assert "Carol" in line + assert "Alice" not in line and "Dave" not in line + + def test_second_argument_constant_with_no_match_reports_zero(self): + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}(E, "missing_reason")?', INFERRED) + assert _row_count(line) == 0 + + def test_both_arguments_constant_matching_row(self): + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}("Carol", "stale")?', INFERRED) + assert _row_count(line) == 1 + + def test_both_arguments_constant_crossed_pair_reports_zero(self): + # Carol and low_conf each exist, but not together — a per-column filter + # that ORed the positions would wrongly report a row here. + line = rlc.policy_result_line(PREDICATE, f'{PREDICATE}("Carol", "low_conf")?', INFERRED) + assert _row_count(line) == 0 + + def test_all_variable_query_still_reports_full_extent(self): + line = rlc.policy_result_line(PREDICATE, f"{PREDICATE}(E, R)?", INFERRED) + assert _row_count(line) == 3 + assert "E=Alice, R=low_conf" in line + + # The 0-arity row case lives in TestReportRouterParity: asserting it on the + # report alone let a router that dropped its short-row guard survive. + + +class TestReportRouterParity: + """The report and the ``ask`` router must answer a policy query identically. + + Parity is NOT correctness: two paths can agree and both be wrong, which is + exactly the state before this fix at non-first argument positions. So every + case asserts the ABSOLUTE row count as well as the agreement, and the router + side calls ``ask_router.evaluate`` directly rather than re-implementing the + filter in the test (a re-implementation would prove nothing about either + production path). + """ + + @pytest.mark.parametrize( + ("draft", "expected"), + [ + (f'{PREDICATE}("Alice", R)?', 1), + (f'{PREDICATE}("Bob", R)?', 0), + (f'{PREDICATE}(E, "stale")?', 1), + (f'{PREDICATE}(E, "missing_reason")?', 0), + (f'{PREDICATE}("Carol", "stale")?', 1), + (f'{PREDICATE}("Carol", "low_conf")?', 0), + (f"{PREDICATE}(E, R)?", 3), + ], + ) + def test_report_and_router_agree_on_the_verified_row_count(self, monkeypatch, draft, expected): + report_rows = _report_rows(draft) + router_rows = _router_rows(monkeypatch, draft) + assert report_rows == expected + assert router_rows == expected + assert report_rows == router_rows + + def test_zero_arity_row_is_dropped_by_both_paths(self, monkeypatch): + # The cases above are all 2-column rows, so neither path's short-row + # guard is exercised by them: a router that dropped the guard kept + # passing. A 0-arity row cannot satisfy a pinned constant, on EITHER path. + inferred = {PREDICATE: {(), ("Alice", "low_conf")}} + pinned = f'{PREDICATE}("Alice", R)?' + assert _report_rows(pinned, inferred) == 1 + assert _router_rows(monkeypatch, pinned, inferred) == 1 + # Non-vacuous: the empty row IS in the extent both paths start from, so + # the 1 above is a filter result, not an absent row. + variables = f"{PREDICATE}(E, R)?" + assert _report_rows(variables, inferred) == 2 + assert _router_rows(monkeypatch, variables, inferred) == 2 + + def test_nfd_stored_entity_does_not_meet_an_nfc_query_on_either_path(self, monkeypatch): + # Pins CURRENT behaviour, not desired behaviour: both paths compare raw + # values, so an NFD-stored entity is invisible to an NFC-typed constant + # and the query reports 0 rows — which reads as a verified negative. That + # is a known gap (see #213 and policy_row_matches' docstring); what this + # test forbids is FIXING IT ON ONE PATH ONLY, which would put the report + # and ask back into disagreement. The cases above are ASCII-only, so a + # router that folded to NFC on its own kept passing them. + nfd = unicodedata.normalize("NFD", "박수영") + nfc = unicodedata.normalize("NFC", "박수영") + assert nfd != nfc + inferred = {PREDICATE: {(nfd, "stale")}} + nfc_query = f'{PREDICATE}("{nfc}", R)?' + assert _report_rows(nfc_query, inferred) == 0 + assert _router_rows(monkeypatch, nfc_query, inferred) == 0 + # Non-vacuous: the row is reachable — an NFD-typed constant finds it on + # both paths, so the 0 above is the folding gap and not an empty extent. + nfd_query = f'{PREDICATE}("{nfd}", R)?' + assert _report_rows(nfd_query, inferred) == 1 + assert _router_rows(monkeypatch, nfd_query, inferred) == 1 diff --git a/tests/unit/test_policy_result_line_rendering.py b/tests/unit/test_policy_result_line_rendering.py new file mode 100644 index 00000000..923ed523 --- /dev/null +++ b/tests/unit/test_policy_result_line_rendering.py @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Regression tests: what a policy query result line may claim (#326). + +Two rendering defects that survive the quoted-constant filter: + +1. A MALFORMED query still got an answer. No trailing '?' makes ``query_args`` + return [] and ``pred()?`` returns one empty arg — either way no constant is + pinned, the filter passes every row, and the line reports the predicate's + whole extent. Wrong arity (``pred("Alice")?``, ``pred(E, R, "zzz")?``) filters + on whatever constants happen to line up with a column and reports a plausible + but meaningless count. All four shapes are rejected in the report's own Errors + section at the same time; an error plus a fabricated answer is worse than the + error alone. The guard here is the arity test ``validate_query`` already uses, + and ``ask_router.evaluate`` raises on the same shapes so neither path answers. + +2. The filtered result line and the "Policy evaluation:" extent line sit a few + lines apart in the same report and now legitimately disagree (3 rows there, + 0 rows here). Echoing the query is what makes that pair readable as scope + rather than contradiction. The echo is limited to constant-pinned queries so a + variable-only query keeps its original text; the extent line is deliberately + left alone (it is pinned by tests/golden/logic_report.txt). +""" +from __future__ import annotations + +import pytest + +import ask_router +import run_logic_check as rlc + +PREDICATE = "needs_review" +INFERRED = { + PREDICATE: { + ("Alice", "low_conf"), + ("Carol", "stale"), + ("Dave", "no_source"), + } +} + + +MALFORMED = [ + f'{PREDICATE}("Bob", R)', # no trailing '?' — query_args returns [] + PREDICATE, # bare predicate, not an atom at all + f"{PREDICATE}()?", # one empty arg, not zero args + f'{PREDICATE}("Alice")?', # too few + f'{PREDICATE}("Alice", R, "zzz")?', # too many +] + + +class TestMalformedQueryEmitsNoResultLine: + def _evaluate(self, monkeypatch, line): + monkeypatch.setattr(rlc, "query_lines", lambda: [line]) + return rlc.evaluate_queries([], INFERRED, {PREDICATE}) + + @pytest.mark.parametrize("line", MALFORMED) + def test_malformed_query_produces_no_result_line(self, monkeypatch, line): + assert self._evaluate(monkeypatch, line) == [] + + @pytest.mark.parametrize("line", MALFORMED) + def test_malformed_query_returns_none_not_a_row_count_string(self, line): + assert rlc.policy_result_line(PREDICATE, line, INFERRED) is None + + def test_wellformed_query_still_produces_a_result(self, monkeypatch): + results = self._evaluate(monkeypatch, f'{PREDICATE}("Alice", R)?') + assert len(results) == 1 + assert "1 rows" in results[0] + + +class TestMalformedQueryParity: + """A malformed policy query must get NO answer from either path. + + Before the guard the two paths disagreed outright on the same input: for + ``pred("Bob", R)`` (no '?') the router returned count=3 while the report + returned nothing, and for ``pred(E, R, "zzz")?`` both rendered '0 rows', + which reads as a verified negative for a query classify_query rejects as + BAD_ARITY. The router refuses by raising (cmd_evaluate turns that into error + JSON); the report refuses by emitting no line. Neither invents a count. + """ + + @pytest.mark.parametrize("draft", MALFORMED) + def test_router_refuses_and_report_stays_silent(self, monkeypatch, draft): + monkeypatch.setattr(ask_router, "policy_predicates", lambda program: {PREDICATE}) + monkeypatch.setattr(ask_router, "run_wirelog", lambda: INFERRED) + with pytest.raises(NotImplementedError): + ask_router.evaluate(draft, []) + assert rlc.policy_result_line(PREDICATE, draft, INFERRED) is None + + def test_wellformed_query_is_still_answered_by_both(self, monkeypatch): + monkeypatch.setattr(ask_router, "policy_predicates", lambda program: {PREDICATE}) + monkeypatch.setattr(ask_router, "run_wirelog", lambda: INFERRED) + draft = f'{PREDICATE}("Alice", R)?' + assert ask_router.evaluate(draft, [])["count"] == 1 + assert "1 rows" in rlc.policy_result_line(PREDICATE, draft, INFERRED) + + +class TestResultLineNamesTheQueryItAnswers: + def test_variable_only_query_text_is_byte_identical_to_before_the_fix(self): + # The fix promised not to change the output of a variable-only query. + # This is the literal upstream/main (c6d359d) rendering of this input, + # so the echo must not reach it: a variable-only query reports the extent, + # which is exactly what the 'Policy evaluation:' line says, so it cannot + # produce the mismatch the echo exists to explain. + line = rlc.policy_result_line(PREDICATE, f"{PREDICATE}(E, R)?", INFERRED) + assert line == ( + "needs_review results: 3 rows; " + "E=Alice, R=low_conf; E=Carol, R=stale; E=Dave, R=no_source" + ) + assert "query:" not in line + + def test_result_line_echoes_the_query(self): + draft = f'{PREDICATE}("Bob", R)?' + line = rlc.policy_result_line(PREDICATE, draft, INFERRED) + # Without the echo, '0 rows' here reads as a contradiction of the + # 'needs_review: 3 rows' extent line printed just above it. + assert draft in line + assert "0 rows" in line + + def test_echo_distinguishes_two_queries_on_the_same_predicate(self, monkeypatch): + monkeypatch.setattr( + rlc, + "query_lines", + lambda: [f'{PREDICATE}("Alice", R)?', f'{PREDICATE}("Bob", R)?'], + ) + first, second = rlc.evaluate_queries([], INFERRED, {PREDICATE}) + assert "Alice" in first and "1 rows" in first + assert "Bob" in second and "0 rows" in second + assert first != second diff --git a/tools/ask_router.py b/tools/ask_router.py index 062f81d8..9035780c 100644 --- a/tools/ask_router.py +++ b/tools/ask_router.py @@ -420,6 +420,46 @@ def _reachable_pairs(facts: list[dict[str, str]]) -> set[tuple[str, str]]: return pairs +def policy_row_matches(args: list[str], row: tuple[str, ...] | list[str]) -> bool: + """True when *row* satisfies every quoted constant *args* pins, by position. + + A quoted constant is a FILTER, at whatever position it appears. This branch + used to test args[0] only, so `pred(E, "stale")?` returned the whole extent + and `pred("Carol", "low_conf")?` returned Carol's row under a reason that is + not hers — a fabricated positive for the exact pair the user asked about. + + A row shorter than the pinned position cannot satisfy the constant, so the + 0-arity row an engine may emit is dropped from a constant-pinned query (an + all-variable query still returns it). + + Comparison is RAW (`arg_value` only), deliberately not `canonical_value` + which the relation branch uses: it mirrors run_logic_check's report path + exactly so `ask` and the report cannot diverge, which is the property + tests/unit/test_policy_query_filter.py pins. This means the policy path does + NOT go through the "#213 single query-value comparison chokepoint" + (common.py `_canonical_value`), contrary to what that docstring claims for + every query-match path. Measured consequence: an NFD-stored entity queried + with an NFC-typed constant now yields 0 rows, which reads as a verified + negative. Folding both sides belongs in one place for BOTH paths and is out + of scope here; see #213. + + The matching rule is kept identical to run_logic_check's `policy_row_matches` + (same body, module-specific docstring). The natural home is common.py + alongside the other query-parsing helpers, but hoisting it there is a wider + change than this fix needs; the report/router parity test fails if the two + copies ever drift. Two of its cases carry that load — the 0-arity row and the + NFD-stored/NFC-queried entity. Every other case is a 2-column ASCII row, + which a copy that lost the short-row guard, or that folded to NFC on its own, + still gets right; do not delete those two. + """ + for index, arg in enumerate(args): + if not is_quoted_string(arg): + continue + if index >= len(row) or arg_value(arg) != row[index]: + return False + return True + + def evaluate(draft: str, facts: list[dict[str, str]]) -> dict[str, object]: """Evaluate a validated engine query: relation, path, or a policy predicate. @@ -427,11 +467,12 @@ def evaluate(draft: str, facts: list[dict[str, str]]) -> dict[str, object]: - path: a fully-quoted query returns the dependency path (or none); a query with a variable returns the reachable (start, target) pairs. - policy predicate: the inferred (entity, reason) rows from the engine, - optionally filtered by a quoted entity argument. + filtered by every quoted constant the query pins, at whatever argument + position it appears (see policy_row_matches). A truly unknown predicate raises NotImplementedError rather than returning 0 rows, so a caller never mistakes an unsupported predicate for a verified - negative. + negative. A malformed count or policy query raises for the same reason. """ predicate = _predicate_of(draft) args = query_args(draft) @@ -496,15 +537,27 @@ def evaluate(draft: str, facts: list[dict[str, str]]) -> dict[str, object]: # verified negative). Catch broad Exception — never BaseException, so # KeyboardInterrupt/SystemExit still propagate — because the engine may # raise non-FactlogError types. + # Guard arity BEFORE evaluating, like the count branch above (#257) and + # for the same reason: with a malformed query no constant lines up with a + # column, so the filter passes rows it cannot have checked — `pred("X")?` + # returned a filtered-looking count and `pred(E, R, "zzz")?` returned 0 + # rows, which renders as a verified negative for a query classify_query + # already rejects as BAD_ARITY. classify_query rejects these too, so the + # render path never reaches here; the `evaluate` subcommand does, and + # cmd_evaluate turns NotImplementedError into a clean error JSON. + # run_logic_check drops the result line on the same shapes, so the report + # and ask agree that a malformed policy query has no answer. + if len(args) != 2: + raise NotImplementedError("policy query must have entity and reason arguments") try: inferred = run_wirelog() except Exception as exc: # noqa: BLE001 — engine/loader raise non-FactlogError too return {"rows": [], "count": 0, "policy_unevaluable": str(exc)} - rows = [] - for row in sorted(inferred.get(predicate, set())): - if args and is_quoted_string(args[0]) and (not row or arg_value(args[0]) != row[0]): - continue - rows.append(list(row)) + rows = [ + list(row) + for row in sorted(inferred.get(predicate, set())) + if policy_row_matches(args, row) + ] return {"rows": rows, "count": len(rows)} raise NotImplementedError(f"engine evaluation of predicate '{predicate}' is not supported") diff --git a/tools/run_logic_check.py b/tools/run_logic_check.py index 2a180db4..98839196 100644 --- a/tools/run_logic_check.py +++ b/tools/run_logic_check.py @@ -20,6 +20,7 @@ LOGIC_POLICY_DL, run_wirelog, arg_value, + is_quoted_string, query_args, quoted_constants, ) @@ -93,9 +94,83 @@ def validate_query(line: str, entities: set[str], policy_query_predicates: set[s return errors, warnings -def policy_result_line(predicate: str, line: str, inferred: dict[str, set[tuple[str, ...]]]) -> str: - rows = sorted(inferred[predicate]) +def policy_row_matches(args: list[str], row: tuple[str, ...] | list[str]) -> bool: + """True when *row* satisfies every quoted constant *args* pins, by position. + + A quoted constant is a FILTER, at whatever position it appears — not merely a + binding the display omits. Filtering only args[0] would still let + ``pred(E, "stale")?`` report the other reasons' rows, and would do so while + the first-argument form answers correctly, which is worse than filtering + nothing: the reader loses the one signal that the second line is untrustworthy. + + A row shorter than the pinned position cannot satisfy the constant, so the + 0-arity row an engine may emit is dropped from a constant-pinned query (it + still shows up for an all-variable query, as before). + + Comparison is RAW (``arg_value`` only), deliberately not + ``common.canonical_value``: it mirrors ask_router's policy branch exactly so + the report and ``ask`` cannot diverge, which is the property + tests/unit/test_policy_query_filter.py pins. This means the policy path does + NOT go through the "#213 single query-value comparison chokepoint" + (common.py `_canonical_value`), contrary to what that docstring claims for + every query-match path. Measured consequence: an NFD-stored entity queried + with an NFC-typed constant now yields `0 rows`, which reads as a verified + negative. Folding both sides belongs in one place for BOTH paths and is out + of scope here; see #213. + + The matching rule is kept identical to ask_router's `policy_row_matches` + (same body, module-specific docstring). The natural home is common.py + alongside the other query-parsing helpers, but hoisting it there is a wider + change than this fix needs; the report/router parity test fails if the two + copies ever drift. Two of its cases carry that load — the 0-arity row and the + NFD-stored/NFC-queried entity. Every other case is a 2-column ASCII row, + which a copy that lost the short-row guard, or that folded to NFC on its own, + still gets right; do not delete those two. + """ + for index, arg in enumerate(args): + if not is_quoted_string(arg): + continue + if index >= len(row) or arg_value(arg) != row[index]: + return False + return True + + +def policy_result_line(predicate: str, line: str, inferred: dict[str, set[tuple[str, ...]]]) -> str | None: + """Render one policy query's result, or None when the query is malformed. + + The arity test is the SAME one validate_query applies to a policy query + (entity + reason, i.e. exactly 2 args), so a line the report is rejecting in + its Errors section never also receives an answer here. Three shapes reach + this function malformed, and each used to be answered: + + - unparseable (no trailing '?'): `query_args` returns [], no constant is + pinned, so the filter passes everything -> the whole extent; + - `pred()?` -> one empty arg, likewise unfiltered -> the whole extent; + - `pred("Alice")?` / `pred("Alice", R, "zzz")?` -> wrong arity, filtered by + whatever constants happen to line up -> a plausible but meaningless count. + + "No usable args" is not "no constants to honour" — it means the query was + never understood, so answering it invents an answer for a line the report is + simultaneously calling an error. Emitting nothing leaves the Errors section + to speak. ask_router.evaluate raises NotImplementedError on the same shapes, + so neither path answers a malformed policy query. + + The query is echoed ONLY when a quoted constant is pinned. Such a line and + the "Policy evaluation:" extent line (": N rows", the count over ALL + entities) sit a few lines apart and now legitimately disagree — 3 rows there, + 0 rows here — so naming the query that produced the 0 is what makes the pair + readable as scope rather than contradiction, and it tells two queries on the + same predicate apart. A variable-only query cannot produce that mismatch (it + reports the extent, which is what the extent line says), so it keeps its + original text byte for byte — the query-shape whose output this fix promised + not to change. The extent line itself is left untouched: it is pinned by + tests/golden/logic_report.txt, and its section header already says it is the + policy evaluation rather than the answer to any one query. + """ args = query_args(line) + if len(args) != 2: + return None + rows = [row for row in sorted(inferred[predicate]) if policy_row_matches(args, row)] values: list[str] = [] for row in rows: bindings = [] @@ -104,7 +179,8 @@ def policy_result_line(predicate: str, line: str, inferred: dict[str, set[tuple[ bindings.append(f"{arg}={value}") values.append(", ".join(bindings) if bindings else ", ".join(row)) suffix = "; " + "; ".join(values) if values else "" - return f"{predicate} results: {len(rows)} rows{suffix}" + echo = f" (query: {line})" if any(is_quoted_string(arg) for arg in args) else "" + return f"{predicate} results{echo}: {len(rows)} rows{suffix}" def evaluate_queries(facts: list[dict[str, str]], inferred: dict[str, set[tuple[str, ...]]], policy_query_predicates: set[str]) -> list[str]: @@ -112,7 +188,9 @@ def evaluate_queries(facts: list[dict[str, str]], inferred: dict[str, set[tuple[ for line in query_lines(): predicate = line.split("(", 1)[0] if predicate in policy_query_predicates: - results.append(policy_result_line(predicate, line, inferred)) + result_line = policy_result_line(predicate, line, inferred) + if result_line is not None: + results.append(result_line) elif predicate == "path": constants = quoted_constants(line) if len(constants) >= 2: