Skip to content

Commit 3ba945a

Browse files
committed
Add the WM-side knowledge service and a lookup-literal lint rule (ADR-0072, ADR-0028)
goals.md §4.10 puts the WM in charge of the logical fact store and the memo table, but nothing before this owned either: an ER had no path to send `knowledge/query` or `knowledge/registerSchema` requests, and the DAG needed to memoize a query and refresh a stale bucket on demand did not exist anywhere in the WM. - services/knowledge_service.py: loads the fact store, runs R11's confirmation walk, holds the schema an ER registers, executes queries over the interpreter, and memoizes them keyed to the fact DAG's revision. Mode.VERIFIED re-checks tracked inputs before answering; Mode.CACHED skips that walk for a live WM's latency-sensitive callers (ADR-0014 D6). A stale bucket a walk discovers is refreshed by dispatching a scoped `extract_knowledge` run into an ER and ingesting the facts it returns, deduped per bucket and capped in flight so concurrent queries needing the same bucket cost one round trip. - runner/knowledge_bridge.py: the installable slot (ADR-0072) an ER's JSON-RPC client calls into, so the runner layer never imports the service that sits above it in the WM's layer stack. - finecode_knowledge/query/validate.py: adds `_warn_lookup_literals`, the mirror of `_warn_unconstrained_key_bindings` -- it flags a FIELD literal that exists only to bind a head `Prov`, which in a conjunctive language is an existence test the author never asked for. ADR-0028 is the case this closes: `expected_in` was bound by adding a `def_path` literal to two rules, so a project missing that fact produced no finding at all even though the real violation held.
1 parent c0f9fc4 commit 3ba945a

7 files changed

Lines changed: 2964 additions & 16 deletions

File tree

finecode_knowledge/src/finecode_knowledge/query/validate.py

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
from finecode_knowledge.model.registry import SchemaRegistry
2828

2929
__all__ = [
30+
"LookupLiteralWarning",
3031
"UnconstrainedKeyBindingWarning",
3132
"validate_body",
3233
"validate_head_parameters",
@@ -65,6 +66,7 @@ def validate_body(
6566
_check_negation_safety(body, context=context)
6667
_check_range_restriction(body, context=context, projected=projected)
6768
_warn_unconstrained_key_bindings(body, context=context, projected=projected)
69+
_warn_lookup_literals(body, context=context, projected=projected)
6870

6971

7072
def _check_literals_against_schema(
@@ -298,3 +300,83 @@ def _warn_unconstrained_key_bindings(
298300

299301
class UnconstrainedKeyBindingWarning(UserWarning):
300302
"""See ``_warn_unconstrained_key_bindings``."""
303+
304+
305+
def _warn_lookup_literals(
306+
body: Conjunction, *, context: str, projected: typing.Sequence[object]
307+
) -> None:
308+
"""Warn where a FIELD literal exists only to bind provenance for the head.
309+
310+
**The mirror of ``_warn_unconstrained_key_bindings``.** That one catches a
311+
body that addresses an entity and never requires it to exist -- possibly
312+
under-constrained. This one catches the opposite: a body that *requires a
313+
fact to exist* only because the author wanted its provenance -- possibly
314+
over-constrained.
315+
316+
In a conjunctive language every literal is a filter, so retrieving a value
317+
and requiring it are the same act. A literal added to fetch something the
318+
head reports silently narrows the rule's extension, and it narrows it toward
319+
**under-reporting** -- a rule that stops reporting a real violation, which is
320+
the direction ADR-0013 D4.3 and ADR-0002 C3 refuse. ADR-0028 is the case
321+
this was written for: ``expected_in`` was bound by adding
322+
``ProjectFields.def_path(subject, _, at=expected_in)`` to two rules, and a
323+
project with no ``def_path`` fact therefore produced no finding at all.
324+
325+
The rule it enforces:
326+
327+
A ``Prov`` in the head must ride on a literal the body would contain
328+
anyway. If you had to *add* a literal to bind it, the value is not part
329+
of the rule's truth -- it is reporting metadata, and it belongs to
330+
whoever displays the finding.
331+
332+
So the shape flagged is a literal whose removal would leave every one of its
333+
variables still bound: the entity is constrained elsewhere, the value is a
334+
throwaway variable read by nobody, and the only thing the literal yields is
335+
a head-bound ``Prov``. Its whole contribution to the answer is an existence
336+
test the author never asked for.
337+
338+
A warning rather than an error, for ``_warn_unconstrained_key_bindings``'
339+
reason: requiring the fact is occasionally what the author meant. When it is,
340+
the value term is usually wanted too, or the entity is bound here and nowhere
341+
else -- both of which this check already declines to flag.
342+
"""
343+
used_elsewhere: dict[int, int] = {}
344+
for literal in body:
345+
for term in literal.terms:
346+
if isinstance(term, Var):
347+
used_elsewhere[id(term)] = used_elsewhere.get(id(term), 0) + 1
348+
349+
exempt = {id(term) for term in projected if isinstance(term, Var)}
350+
flagged: list[str] = []
351+
for literal in body:
352+
if literal.negated or literal.kind is not LiteralKind.FIELD:
353+
continue
354+
# Only a head-bound provenance makes the literal a lookup. With no `at=`
355+
# the existence test is the literal's whole point, which is deliberate.
356+
if not isinstance(literal.at, Prov) or id(literal.at) not in exempt:
357+
continue
358+
entity, value = literal.terms[0], literal.terms[1]
359+
# A value the body reads, or projects, is a real constraint.
360+
if not isinstance(value, Var) or id(value) in exempt:
361+
continue
362+
if used_elsewhere.get(id(value), 0) > 1:
363+
continue
364+
# If the entity is bound only here, the literal is load-bearing.
365+
if not isinstance(entity, Var) or used_elsewhere.get(id(entity), 0) < 2:
366+
continue
367+
flagged.append(literal.predicate)
368+
369+
if flagged:
370+
warnings.warn(
371+
f"{context}: {len(flagged)} field literal(s) -- {', '.join(sorted(flagged))} -- "
372+
"require a fact to exist only so their provenance can bind a head parameter. "
373+
"In a conjunction that is a filter, so a subject missing the fact yields no "
374+
"finding at all. Bind the head from a literal the rule needs anyway, or let "
375+
"whoever displays the finding look the location up (ADR-0028).",
376+
LookupLiteralWarning,
377+
stacklevel=3,
378+
)
379+
380+
381+
class LookupLiteralWarning(UserWarning):
382+
"""See ``_warn_lookup_literals``."""

finecode_knowledge/tests/test_cypher.py

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
from __future__ import annotations
1818

19+
import warnings
20+
1921
import pytest
2022
from libcat import LIBCAT_SCHEMA, Author, Book, BookFields, Rel, Shelf
2123

@@ -25,6 +27,7 @@
2527
compile_query,
2628
compile_rule,
2729
)
30+
from finecode_knowledge.query.validate import LookupLiteralWarning
2831

2932

3033
@q.derived
@@ -48,22 +51,31 @@ def _(src: q.Var[Shelf], shelf: q.Var[Shelf]) -> q.Body:
4851
LIBCAT_SCHEMA.register_predicate(_predicate)
4952

5053

51-
@q.rule
52-
def shelved_book_not_borrowed(
53-
subject: q.Var[Author],
54-
missing: q.Var[str],
55-
asserted_at: q.Prov,
56-
expected_in: q.Prov,
57-
) -> q.Body:
58-
"""author {subject} shelved a book but never borrowed {missing}"""
59-
book, shelf = q.var(Book), q.var(Shelf)
60-
return q.all_(
61-
Rel.wrote(subject, book, at=asserted_at),
62-
Rel.shelved_on(book, shelf),
63-
Shelf.key(shelf, code=missing),
64-
BookFields.title(book, q.var(str), at=expected_in),
65-
q.not_(_borrowed_isbn(subject, missing)),
66-
)
54+
with warnings.catch_warnings():
55+
# `title` is bound only to feed `expected_in`, which is exactly the shape
56+
# `LookupLiteralWarning` exists to flag (ADR-0028) -- and here it is the point:
57+
# `test_a_field_facts_provenance_binds_to_a_fact_node` below needs a rule that
58+
# compiles a field fact's provenance to a `Fact` node, so this fixture must keep
59+
# the literal a real rule should not have. Suppressed at the definition rather
60+
# than filtered suite-wide, so the check still guards every other rule here.
61+
warnings.simplefilter("ignore", LookupLiteralWarning)
62+
63+
@q.rule
64+
def shelved_book_not_borrowed(
65+
subject: q.Var[Author],
66+
missing: q.Var[str],
67+
asserted_at: q.Prov,
68+
expected_in: q.Prov,
69+
) -> q.Body:
70+
"""author {subject} shelved a book but never borrowed {missing}"""
71+
book, shelf = q.var(Book), q.var(Shelf)
72+
return q.all_(
73+
Rel.wrote(subject, book, at=asserted_at),
74+
Rel.shelved_on(book, shelf),
75+
Shelf.key(shelf, code=missing),
76+
BookFields.title(book, q.var(str), at=expected_in),
77+
q.not_(_borrowed_isbn(subject, missing)),
78+
)
6779

6880

6981
# ---- the dialect's constraints show up in the output -------------------
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
"""A field literal that exists only to bind provenance for the head (ADR-0028).
2+
3+
The defect this closes: in a conjunction every literal is a filter, so retrieving
4+
a value and requiring it are the same act. A literal added to fetch something the
5+
head *reports* narrows the rule's extension -- toward **under-reporting**, which
6+
is the direction ADR-0013 D4.3 and ADR-0002 C3 refuse.
7+
8+
Concretely, two shipped rules bound ``expected_in`` by adding
9+
``ProjectFields.def_path(subject, _, at=expected_in)``, and a project with no
10+
``def_path`` fact then produced no finding at all -- every real condition held.
11+
12+
This is the **mirror** of ``UnconstrainedKeyBindingWarning``
13+
(``tests/test_key_literal.py``): that one catches a body that never requires an
14+
entity to exist, this one a body that requires a fact to exist by accident.
15+
16+
Written against ``libcat`` (``tests/fixtures/libcat``), like every other engine
17+
test -- the engine must not know FineCode's vocabulary (R18/R19).
18+
"""
19+
20+
from __future__ import annotations
21+
22+
import pytest
23+
from libcat import LIBCAT_SCHEMA, Author, Book, BookFields, Rel
24+
25+
from finecode_knowledge import query as q
26+
from finecode_knowledge.query.validate import LookupLiteralWarning, validate_body
27+
28+
29+
def _lookup_warnings(recwarn: pytest.WarningsRecorder) -> list:
30+
return [w for w in recwarn if issubclass(w.category, LookupLiteralWarning)]
31+
32+
33+
def test_a_field_literal_bound_only_to_a_head_prov_warns() -> None:
34+
"""The ADR-0028 shape: `title` is required to exist purely so `at` can bind.
35+
36+
`book` is already bound by the edge, and the value term is a throwaway
37+
variable nobody reads -- so removing the literal would leave every variable
38+
still bound, and its whole contribution to the answer is an existence test.
39+
"""
40+
author, book, at = q.var(Author), q.var(Book), q.Prov()
41+
body = q.all_(
42+
Rel.wrote(author, book),
43+
BookFields.title(book, q.var(str), at=at),
44+
)
45+
46+
with pytest.warns(LookupLiteralWarning, match="provenance can bind a head parameter"):
47+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(author, book, at))
48+
49+
50+
def test_a_prov_riding_on_a_literal_the_rule_needs_does_not_warn(
51+
recwarn: pytest.WarningsRecorder,
52+
) -> None:
53+
"""`asserted_at`'s shape, and the rule the warning enforces.
54+
55+
Binding provenance off an edge the body needs anyway adds no filter -- the
56+
edge was already required. That is the legitimate half of ADR-0028.
57+
"""
58+
author, book, at = q.var(Author), q.var(Book), q.Prov()
59+
body = q.all_(Rel.wrote(author, book, at=at))
60+
61+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(author, book, at))
62+
63+
assert not _lookup_warnings(recwarn)
64+
65+
66+
def test_a_field_literal_whose_value_the_body_reads_does_not_warn(
67+
recwarn: pytest.WarningsRecorder,
68+
) -> None:
69+
"""A value another literal consumes is a real join, not a throwaway."""
70+
author, book, title, at = q.var(Author), q.var(Book), q.var(str), q.Prov()
71+
other = q.var(Book)
72+
body = q.all_(
73+
Rel.wrote(author, book),
74+
BookFields.title(book, title, at=at),
75+
BookFields.label(other, title),
76+
)
77+
78+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(author, book, at))
79+
80+
assert not _lookup_warnings(recwarn)
81+
82+
83+
def test_a_field_literal_whose_value_is_projected_does_not_warn(
84+
recwarn: pytest.WarningsRecorder,
85+
) -> None:
86+
"""`locations.project_locations`' shape: the caller wants the value itself."""
87+
book, title, at = q.var(Book), q.var(str), q.Prov()
88+
body = q.all_(BookFields.title(book, title, at=at))
89+
90+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(book, title, at))
91+
92+
assert not _lookup_warnings(recwarn)
93+
94+
95+
def test_a_field_literal_with_no_provenance_binding_does_not_warn(
96+
recwarn: pytest.WarningsRecorder,
97+
) -> None:
98+
"""Without `at=` the existence test is the literal's whole point.
99+
100+
"Books that were actually scanned" is a thing a rule may legitimately ask,
101+
and ADR-0019 D4 keeps the field spelling available precisely to ask it.
102+
"""
103+
author, book = q.var(Author), q.var(Book)
104+
body = q.all_(Rel.wrote(author, book), BookFields.title(book, q.var(str)))
105+
106+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(author, book))
107+
108+
assert not _lookup_warnings(recwarn)
109+
110+
111+
def test_a_field_literal_that_alone_binds_its_entity_does_not_warn(
112+
recwarn: pytest.WarningsRecorder,
113+
) -> None:
114+
"""Load-bearing: drop this literal and `book` is unbound, so it is not a lookup."""
115+
book, at = q.var(Book), q.Prov()
116+
body = q.all_(BookFields.title(book, q.var(str), at=at))
117+
118+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(book, at))
119+
120+
assert not _lookup_warnings(recwarn)
121+
122+
123+
def test_a_prov_that_is_not_in_the_head_does_not_warn(
124+
recwarn: pytest.WarningsRecorder,
125+
) -> None:
126+
"""The warning is about feeding the *head*. A body-internal `at` reports nothing."""
127+
author, book, at = q.var(Author), q.var(Book), q.Prov()
128+
body = q.all_(
129+
Rel.wrote(author, book),
130+
BookFields.title(book, q.var(str), at=at),
131+
)
132+
133+
validate_body(body, LIBCAT_SCHEMA, context="test", projected=(author, book))
134+
135+
assert not _lookup_warnings(recwarn)
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
"""Slot through which whoever owns the fact store serves the runner layer.
2+
3+
``knowledge/query`` and ``knowledge/registerSchema`` arrive on the **runner**'s
4+
JSON-RPC client. The code that answers them lives in
5+
``services/knowledge_service.py``, because it owns the fact store and the memo
6+
table -- and services sit *above* the runner in the WM's layer stack. See ADR-0072
7+
for why this is a slot the owner fills on import rather than an upward import.
8+
"""
9+
10+
from __future__ import annotations
11+
12+
import typing
13+
14+
if typing.TYPE_CHECKING:
15+
from finecode.wm_server import context
16+
17+
__all__ = ["KnowledgeHandlers", "handlers", "install", "reset"]
18+
19+
20+
class KnowledgeHandlers(typing.Protocol):
21+
"""What the runner needs from whoever owns the store."""
22+
23+
async def register_schema(self, snapshot: dict) -> bool:
24+
"""Install *snapshot* as the schema the WM's store is read against."""
25+
26+
async def run_query(
27+
self,
28+
ws_context: context.WorkspaceContext,
29+
query: dict,
30+
*,
31+
mode: str,
32+
limit: int | None,
33+
) -> dict:
34+
"""Execute a serialized query and return ``{"rows": ..., "freshness": ...}``."""
35+
36+
async def fetch_records(
37+
self, ws_context: context.WorkspaceContext, refs: list[dict]
38+
) -> dict:
39+
"""Read whole entity records and return ``{"records": [...]}``.
40+
41+
The read a query cannot express,
42+
routed here for the same reason ``run_query`` is: it touches the store, so
43+
it happens where the store is.
44+
"""
45+
46+
47+
_installed: KnowledgeHandlers | None = None
48+
49+
50+
def install(implementation: KnowledgeHandlers) -> None:
51+
"""Nominate *implementation* as the answer to knowledge requests from an ER."""
52+
global _installed
53+
_installed = implementation
54+
55+
56+
def reset() -> None:
57+
"""Forget the installed implementation. Tests only."""
58+
global _installed
59+
_installed = None
60+
61+
62+
def handlers() -> KnowledgeHandlers | None:
63+
"""The installed implementation, or ``None`` if the service was never imported.
64+
65+
``None`` is a real state rather than a defect: a WM built without the
66+
knowledge service answers ``knowledge/query`` with a method error, which is
67+
what an ER asking a WM that cannot serve it should hear.
68+
"""
69+
return _installed

0 commit comments

Comments
 (0)