diff --git a/src/scribe/mcp/tools/shapes.py b/src/scribe/mcp/tools/shapes.py index 68a93a7..7765b8a 100644 --- a/src/scribe/mcp/tools/shapes.py +++ b/src/scribe/mcp/tools/shapes.py @@ -267,13 +267,23 @@ async def stamps_to_review(project_id: int, top: int = 10) -> dict: score, signature and derived form so you can judge rather than trust the threshold. - `incoherent` — canons whose judged rows do not agree on a form: the - `forms` histogram, how many members were hook-stamped weakly, and a - sample. A canon asserts that some shapes are the same sort of thing; when - its members are a class, three getters and a dozen tests, that assertion - has stopped being true and every base-rate reading built on it — the + `incoherent` — canons whose membership no longer agrees what the canon + is. A canon asserts that some shapes are the same sort of thing; when its + members are a class, three getters and a dozen tests, that assertion has + stopped being true and every base-rate reading built on it — the divergence prompt included — is reading noise. + Disagreement is judged by FAMILY (a sync helper and an async one are one + family), and a method defined beside a member class counts as part of the + shape rather than against it — so a model convention that covers both the + class and its `to_dict` reads as coherent, which it is (#4220). Each + entry carries `families` and `forms`, the majority `family`, `attached` + (minority rows excused as methods of a member), `strangers` — THE ROWS + THAT DO NOT FIT, which is what you judge — and `unattended` / `weak_rows`, + the count written by the hook with nobody reading. An incoherence whose + rows are all judged is likelier this check being strict than a bad + ledger; one made of unattended rows is the real thing. + A row listed here is a question, not a verdict. Some will be correct. Read-only; requires read access to the project. diff --git a/src/scribe/services/shape_ledger.py b/src/scribe/services/shape_ledger.py index 4c130e0..ff4fc07 100644 --- a/src/scribe/services/shape_ledger.py +++ b/src/scribe/services/shape_ledger.py @@ -918,6 +918,12 @@ FORM_UNKNOWN = "" # a canon poisoned by loose stamping falls silent instead of flagging. _FORM_SHARE = 0.6 +# How much of a canon's membership may be shapes it cannot account for +# before the canon stops meaning anything. Not a majority test — see +# `canon_coherence` for why a majority test goes blind exactly when the +# ledger is worst. +_STRANGER_SHARE = 0.2 + # Leading words that say nothing about the form of the thing being declared. _FORM_NOISE = ("export ", "default ", "public ", "private ", "static ", "final ") @@ -1069,6 +1075,124 @@ def canon_form(rows: Iterable, snippet_id: int) -> str: return top if n / sum(forms.values()) >= _FORM_SHARE else FORM_UNKNOWN +# A judgment nobody read before it was written. "hook" is the write-path +# stamp — a similarity score and no reader. Every other value ("agent", +# "audit", "import", "mechanical") came from something that examined the +# shape and said so, which is a different kind of evidence, not a stronger +# score. +_UNATTENDED_BY = ("hook",) + + +def canon_coherence(members: Iterable) -> dict: + """Does a canon's membership still agree on what the canon IS? + + The verdict is NOT "do all the rows share a form". That was the first + version, and on this surface's first live day both canons it reported + were sound (#4220) while the one genuinely polluted canon had already + been cleaned by hand. Two things were wrong with it, and the fix for the + first nearly broke the second: + + 1. FAMILY, NOT FORM. A sync loop-starter and the async tick it schedules + are one shape written two ways; `shape_family` already collapses `fn` + and `async-fn` into `callable` for exactly this reason on the + divergence side, and this side never asked. Snippet #2849 — four + starters, three ticks, every row judged by an audit — scored 4/7 and + was called incoherent when nothing about it is. + + 2. A METHOD OF A MEMBER IS NOT A STRANGER. Snippet #2844 is the + SQLAlchemy model convention and its own text covers the class AND the + `to_dict` the class must carry. Its 37 classes and 25 serialisers + scored 37/62 = 0.597, missing the bar by three thousandths for + containing exactly what it says it contains. + + THE TRAP, and it is why this is not simply a family histogram: grouping + by family and keeping a majority test makes the check BLIND. Before #2844 + was cleaned it held 37 classes and 56 callables; as families that is + 56/93 = 0.602, a clean pass, and the 31 rows that had no business being + there — Vue functions, route handlers, a dozen tests — would never have + been reported. A looser bar in the same shape is not a fix. + + So the verdict is inverted. Instead of asking whether most rows agree, it + asks how many rows the canon CANNOT ACCOUNT FOR: + + * a row in the majority family is accounted for; + * a callable defined in a FILE that also holds a majority-family `type` + row is accounted for — it is a method of a member; + * everything else is a STRANGER, and strangers above `_STRANGER_SHARE` + of the readable rows make the canon incoherent. + + The majority vote abstains those methods, so a class's own serialisers + cannot outvote the classes and turn the members into the strangers. On + the real ledger the three populations separate completely: clean #2844 + has 0 strangers in 62, #2849 has 0 in 7, and polluted #2844 had 31 in 93. + + Scoped to the REVIEW surface on purpose. `canon_form` still answers at + the precise form level for stamping and divergence, where a sync helper + beside an async canon is a fair question; nothing here changes what the + ledger writes. + + Returns a dict, deliberately. Widening a tuple return is the #4204 break + exactly — a 4-tuple grew a fifth field and one consumer went on unpacking + four, and Python said nothing until that line ran. + """ + forms: dict[str, int] = {} + families: dict[str, int] = {} + per_row: list[tuple[object, str]] = [] + has_type: set[str] = set() + for r in members: + f = shape_form(getattr(r, "signature", "") or "", r.kind) + if not f: + continue # unreadable: never counts either way + fam = shape_family(f) + forms[f] = forms.get(f, 0) + 1 + families[fam] = families.get(fam, 0) + 1 + per_row.append((r, fam)) + if fam == "type": + has_type.add(r.path) + + out = { + "readable": len(per_row), + "forms": dict(sorted(forms.items(), key=lambda kv: -kv[1])), + "families": dict(sorted(families.items(), key=lambda kv: -kv[1])), + "family": FORM_UNKNOWN, + "attached": 0, + "strangers": [], + "coheres": True, + } + if not per_row: + return out # nothing legible: say nothing, not "broken" + + # The vote, with methods abstaining. A callable sitting in a file that + # defines a class is presumed to belong to it and does not get to argue + # that the canon is really about callables. + def _attachable(row, fam: str) -> bool: + return fam == "callable" and row.path in has_type + + votes: dict[str, int] = {} + for r, fam in per_row: + if _attachable(r, fam): + continue + votes[fam] = votes.get(fam, 0) + 1 + if not votes: # every readable row is a method + votes = dict(families) + # Ties go to `type`: a canon that defines a class is about the class. + family = max(votes, key=lambda k: (votes[k], k == "type")) + + attached, strangers = 0, [] + for r, fam in per_row: + if fam == family: + continue + if family == "type" and _attachable(r, fam): + attached += 1 + else: + strangers.append(r) + out["family"] = family + out["attached"] = attached + out["strangers"] = strangers + out["coheres"] = len(strangers) / len(per_row) <= _STRANGER_SHARE + return out + + def signature_in(code: str, symbol: str, kind: str) -> str: """The line in ``code`` that DEFINES ``symbol``, or "" if none does. @@ -2270,12 +2394,26 @@ def stamps_to_review(rows: Iterable[CodeShape], *, top: int = 10) -> dict: person or an audit judged are never listed, however old: a human judgment is not weak evidence, it is a different kind of evidence. - `incoherent` — canons whose judged rows do not agree on a form. A canon is - a claim that some set of shapes are the same sort of thing; when its own - members are a class, three getters and a dozen tests, that claim has - stopped being true, and every base-rate reading built on it is reading - noise. `canon_form` already makes such a canon fall silent — this is what - makes it VISIBLE, which is the half that was missing. + `incoherent` — canons whose membership no longer agrees what the canon + IS. A canon is a claim that some set of shapes are the same sort of + thing; when its own members are a class, three getters and a dozen tests, + that claim has stopped being true, and every base-rate reading built on + it is reading noise. `canon_form` already makes such a canon fall silent + — this is what makes it VISIBLE, which is the half that was missing. + + What counts as disagreement is `canon_coherence`, and it is deliberately + looser than "one form": it compares FAMILIES, and it does not hold a + method against the class it is defined beside. Both corrections came from + this surface's first live day, when the only two canons it reported were + both sound (#4220). A review surface whose output is noise is one that + stops being read, which costs more than the check was ever worth. + + `strangers` names the rows that do not fit, not the first dozen members: + the reader's question is which ones are wrong. `unattended` counts rows + written by the hook with nobody reading — an incoherence made entirely of + judged rows is far more likely to be this check being too strict than a + ledger full of junk, and the reader should be able to see that without + opening the canon. """ live = [r for r in rows if r.vanished_at is None] weak = [] @@ -2300,32 +2438,39 @@ def stamps_to_review(rows: Iterable[CodeShape], *, top: int = 10) -> dict: by_canon.setdefault(int(r.snippet_id), []).append(r) incoherent = [] for sid, members in by_canon.items(): - forms: dict[str, int] = {} - for r in members: - f = shape_form(r.signature or "", r.kind) - if f: - forms[f] = forms.get(f, 0) + 1 - readable = sum(forms.values()) - if readable < _DENSITY_MIN_JUDGED: + coh = canon_coherence(members) + if coh["readable"] < _DENSITY_MIN_JUDGED: continue # too few to say anything either way - if canon_form(members, sid): - continue # a form holds the majority: coherent + if coh["coheres"]: + continue + unattended = sum(1 for r in members if r.classified_by in _UNATTENDED_BY) incoherent.append({ "snippet_id": sid, "judged": len(members), - "forms": dict(sorted(forms.items(), key=lambda kv: -kv[1])), + "forms": coh["forms"], + "families": coh["families"], + "family": coh["family"], + "attached": coh["attached"], + "unattended": unattended, + # The listing below is capped; without this you cannot tell a + # canon with twelve strangers from one with three hundred. + "stranger_count": len(coh["strangers"]), "weak_rows": sum( 1 for r in members - if r.classified_by == "hook" + if r.classified_by in _UNATTENDED_BY and (stamp_score(r.reason) or 1.0) < _RESEMBLE_MIN ), - "sample": [ + # The rows that do NOT fit — not the first dozen members. The + # reader's question is "which ones are wrong", and a sample of + # the majority cannot answer it. + "strangers": [ {"path": r.path, "symbol": r.symbol, - "signature": r.signature or "", "by": r.classified_by} - for r in members[:_REVIEW_ROWS_SHOWN] + "signature": r.signature or "", "by": r.classified_by, + "form": shape_form(r.signature or "", r.kind)} + for r in coh["strangers"][:_REVIEW_ROWS_SHOWN] ], }) - incoherent.sort(key=lambda d: (-d["weak_rows"], -d["judged"])) + incoherent.sort(key=lambda d: (-d["weak_rows"], -d["unattended"], -d["judged"])) return { "weak_count": len(weak), "weak": weak[:top], diff --git a/tests/test_stamps_to_review.py b/tests/test_stamps_to_review.py index eb7acb7..ac839b6 100644 --- a/tests/test_stamps_to_review.py +++ b/tests/test_stamps_to_review.py @@ -25,7 +25,8 @@ hook's own history is made of. import pytest from scribe.services.shape_ledger import ( - _RESEMBLE_MIN, _RESEMBLE_REASON, stamp_score, stamps_to_review, + _RESEMBLE_MIN, _RESEMBLE_REASON, _REVIEW_ROWS_SHOWN, canon_coherence, + stamp_score, stamps_to_review, ) @@ -135,17 +136,26 @@ def test_a_canon_whose_members_agree_is_not_listed() -> None: def test_a_canon_whose_members_are_all_different_things_is_listed() -> None: """Portal's #3283, in miniature: a class, a getter, a test and a binding - recorded as instances of one pattern.""" - rows = [_member("class SessionAbsent(RuntimeError):"), - _member("def build_channel() -> str:"), - _member("async def attach(self) -> None:"), - _member("MAX = 10")] + recorded as instances of one pattern — and in four different files, as + they really were. The paths matter now: a callable sharing a file with a + class is read as that class's method (#4220), so putting them all in one + file would test the excuse rather than the disagreement.""" + rows = [_member("class SessionAbsent(RuntimeError):", path="src/errors.py", + symbol="SessionAbsent"), + _member("def build_channel() -> str:", path="src/channel.py", + symbol="build_channel"), + _member("async def attach(self) -> None:", path="src/attach.py", + symbol="attach"), + _member("MAX = 10", path="src/limits.py", symbol="MAX")] out = stamps_to_review(rows) assert out["incoherent_count"] == 1 entry = out["incoherent"][0] assert entry["snippet_id"] == 7 and entry["judged"] == 4 assert set(entry["forms"]) == {"type", "fn", "async-fn", "binding"} - assert len(entry["sample"]) == 4 + # The two callables carry the vote; the class and the binding are what + # the canon cannot account for, and they are what the reader is shown. + assert entry["family"] == "callable" + assert {s["symbol"] for s in entry["strangers"]} == {"SessionAbsent", "MAX"} def test_too_few_readable_members_says_nothing_either_way() -> None: @@ -197,3 +207,146 @@ def test_the_service_carries_no_machinery_for_bulk_withdrawal() -> None: body = open(ledger.__file__).read() for banned in ("def retire_weak", "def auto_unclassify", "def bulk_withdraw"): assert banned not in body, banned + + +# ── coherence is about what a canon CANNOT account for (#4220) ──────────── +# +# The first version of this check asked whether every member shared a form. +# On its first live day the only two canons it reported were both sound, +# while the genuinely polluted one had already been cleaned by hand — a +# surface whose whole output is noise is one that stops being read. +# +# The obvious repair — group by family, keep the majority test — is a trap, +# and `test_family_grouping_alone_would_have_gone_blind` is the guard that +# stops anyone walking back into it. + +def _model(sym, path): + """A SQLAlchemy model class, as #2844 holds one.""" + return _member(f"class {sym}(Base, TimestampMixin):", path=path) + + +def _to_dict(path): + """The serialiser that model is required to carry, beside it.""" + return _member("def to_dict(self) -> dict:", path=path, symbol="to_dict") + + +def test_a_class_and_the_to_dict_beside_it_are_one_shape() -> None: + """#2844 exactly: the SQLAlchemy model convention, whose own text is + about the class AND the to_dict it must carry. Live it scored + 37/62 = 0.597 and was called incoherent for containing precisely what it + says it contains.""" + rows = [] + for i in range(6): + rows += [_model(f"M{i}", f"src/scribe/models/m{i}.py"), + _to_dict(f"src/scribe/models/m{i}.py")] + out = stamps_to_review(rows) + assert out["incoherent_count"] == 0 + + +def test_a_sync_starter_and_the_async_tick_it_schedules_are_one_shape() -> None: + """#2849: four loop-starters and three ticks. `shape_family` already + collapses fn and async-fn for the divergence gate; this surface was the + one place still asking at form level.""" + rows = [_member("def start_notification_loop() -> None:", path="src/n.py"), + _member("def start_log_retention_loop() -> None:", path="src/l.py"), + _member("def start_auth_token_retention_loop() -> None:", path="src/a.py"), + _member("async def _notification_tick() -> None:", path="src/n.py"), + _member("async def _retention_tick() -> None:", path="src/l.py"), + _member("async def _auth_token_retention_tick() -> None:", path="src/a.py")] + assert stamps_to_review(rows)["incoherent_count"] == 0 + + +def test_family_grouping_alone_would_have_gone_blind() -> None: + """THE REGRESSION GUARD. Polluted #2844 as it stood this morning: 37 + model classes, 25 to_dict methods beside them, and 31 rows that had no + business being there. Counted as families that is 56 callables to 37 + types — 0.602, a clean pass under any majority test. Those 31 rows are + the ones the surface exists to find, so a change that lets this canon + read as coherent has broken the feature while keeping every other test + in this file green.""" + rows = [] + for i in range(37): + rows.append(_model(f"M{i}", f"src/scribe/models/m{i}.py")) + for i in range(25): + rows.append(_to_dict(f"src/scribe/models/m{i % 37}.py")) + strangers = (["src/scribe/routes/rulebooks.py"] * 2 + + ["src/scribe/services/embeddings.py"] * 7 + + ["frontend/src/stores/rulebooks.ts"] * 3 + + ["tests/test_services_rulebooks.py"] * 19) + for i, path in enumerate(strangers): + rows.append(_member(f"async def f{i}():", path=path, symbol=f"f{i}")) + + out = stamps_to_review(rows) + assert out["incoherent_count"] == 1, "the polluted canon must still be caught" + entry = out["incoherent"][0] + assert entry["families"]["callable"] > entry["families"]["type"] + assert entry["family"] == "type", "methods must not outvote their classes" + assert entry["attached"] == 25 + assert entry["stranger_count"] == len(strangers) == 31 + # The listing is capped, the count is not — a reader must be able to + # tell twelve strangers from thirty-one. + assert len(entry["strangers"]) == _REVIEW_ROWS_SHOWN + assert all(not s["path"].startswith("src/scribe/models") + for s in entry["strangers"]) + + +def test_a_callable_in_a_file_with_no_class_is_a_stranger() -> None: + """The excuse is "method of a member", not "callable anywhere". Without + this, any function in the repo would be excused by the existence of a + class somewhere else in the canon.""" + rows = [_model("A", "src/models/a.py"), _to_dict("src/models/a.py"), + _model("B", "src/models/b.py"), _to_dict("src/models/b.py"), + _member("def helper():", path="src/services/free.py", symbol="helper"), + _member("def other():", path="src/services/free.py", symbol="other")] + out = stamps_to_review(rows) + assert out["incoherent_count"] == 1 + assert {s["symbol"] for s in out["incoherent"][0]["strangers"]} == {"helper", "other"} + + +def test_the_strangers_are_named_not_a_sample_of_the_majority() -> None: + """The reader's question is which rows are wrong. A sample of the + agreeing majority cannot answer it, which is what the first version + returned.""" + # Seven and two: 2 of 9 is over `_STRANGER_SHARE`, 2 of 11 is under it. + # The tolerance is real and the counts here sit deliberately on the far + # side of it — see the test below for the near side. + rows = [_member("async def a():", path=f"src/ok{i}.py", symbol=f"a{i}") + for i in range(7)] + rows.append(_member("class Odd:", path="src/odd.py", symbol="Odd")) + rows.append(_member("class Odder:", path="src/odder.py", symbol="Odder")) + out = stamps_to_review(rows) + assert out["incoherent_count"] == 1 + assert {s["symbol"] for s in out["incoherent"][0]["strangers"]} == {"Odd", "Odder"} + + +def test_a_single_odd_row_in_a_large_canon_is_tolerated() -> None: + """One stranger in twenty is a row to fix, not a canon that has stopped + meaning anything. The surface is for the second thing.""" + rows = [_member("async def a():", path=f"src/ok{i}.py", symbol=f"a{i}") + for i in range(19)] + rows.append(_member("class Odd:", path="src/odd.py", symbol="Odd")) + assert stamps_to_review(rows)["incoherent_count"] == 0 + + +def test_an_incoherence_made_of_judged_rows_says_so() -> None: + """The discriminator that tells a too-strict check from a bad ledger. + Both canons flagged on the first live day were entirely audit-judged, + and a reader could not see that without opening each one.""" + judged = [_member("async def a():", path=f"src/j{i}.py", symbol=f"a{i}") + for i in range(4)] + judged += [_member("class J:", path="src/jc.py", symbol="J"), + _member("class J2:", path="src/jc2.py", symbol="J2")] + out = stamps_to_review(judged) + assert out["incoherent_count"] == 1 + assert out["incoherent"][0]["unattended"] == 0 + assert out["incoherent"][0]["weak_rows"] == 0 + + +def test_the_coherence_verdict_reads_rows_and_writes_nothing() -> None: + """`canon_coherence` is called on live ORM rows; it must not touch them.""" + rows = [_model("A", "src/models/a.py"), _to_dict("src/models/a.py"), + _member("def loose():", path="src/x.py", symbol="loose")] + before = [(r.path, r.symbol, r.status, r.snippet_id, r.signature) for r in rows] + canon_coherence(rows) + assert [(r.path, r.symbol, r.status, r.snippet_id, r.signature) + for r in rows] == before