From 446d6da0d7aac3bd3395b3ffa961c1efbbf4436b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 23 Aug 2026 22:36:04 -0400 Subject: [PATCH] =?UTF-8?q?fix(snippets):=20an=20annotated=20record=20is?= =?UTF-8?q?=20not=20a=20diverged=20one=20=E2=80=94=20a=20standing=20verdic?= =?UTF-8?q?t=20vouches=20at=20its=20commit=20(#2782)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pull-time freshness confirms a cached body by containment: normalised cached code must appear inside the fetched file. That is right for a record kept verbatim and permanently wrong for a deliberately annotated one. A record whose job is to say WHY the shape is what it is carries commentary the source does not, so containment fails on every pull, forever — #2508 was reading `diverged` although its declarations match the source exactly, and always would. Annotation is a sanctioned record style, so this was two deliberate designs colliding, and it was quietly poisoning the one honest drift signal: decision #2707's scoreboard watches body_freshness for `diverged` and was accruing false positives it could never age out. The escape hatch is the verdict itself. verify_snippet is exactly where a human or agent already judged this body a faithful rendering of that source, and `verification.commit_sha` records the repo commit they judged it at — a field whose own docstring (#2688) anticipated this: "makes 'the REPO moved on since the check' computable, once the forge integration can compare it against the current head." This is that comparison. When containment fails, a standing verdict can still vouch, on four conditions and no fewer: - the verdict says `ok`; - it has not EXPIRED — verification_view recomputes code_sha against the record's current body, so editing the record retires the verdict; - it was not INVALIDATED by a push touching the location (#2691); - the file just fetched is at the very commit the verdict was stamped at. That last one is what keeps it honest: the hatch vouches for a body against ONE known commit, never against whatever the source became since. The moment the file moves, containment resumes as the authority and the record reads `diverged` until someone re-verifies — correct, because at that point nobody has looked. The first three are checked by reusing verification_view rather than restating its rule, so "expired" keeps meaning one thing. Nothing is rewritten and no new freshness value is minted; `verification` already travels in the same payload, so a reader can see the basis rather than take "current" on faith. Verdicts predating commit stamping carry no commit to compare and therefore do not vouch — they fall through to containment rather than passing on age alone. Tests pin the fix and, more usefully, every condition that switches it back off: moved commit, expired verdict, push-invalidated verdict, non-ok verdict, no verdict, and a pre-#2688 verdict with no commit_sha. Plus a regression that a verbatim record still takes the containment path untouched. Co-Authored-By: Claude Fable 5 --- src/scribe/mcp/tools/snippets.py | 7 +++ src/scribe/services/snippets.py | 48 ++++++++++++++++++ tests/test_snippet_live_body.py | 86 ++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index 78425ce..f591eeb 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -207,6 +207,13 @@ async def get_snippet(snippet_id: int) -> dict: the source moved on — trust the location over the cached body and consider verify_snippet after you look. + A record kept VERBATIM is confirmed by containment. A deliberately + ANNOTATED one — commentary the source does not carry — cannot be, so it + reads "current" on the authority of a standing `ok` verdict stamped at + the very commit just fetched (#2782); `verification` in the same payload + shows that basis. Edit the record, or let the file move past that commit, + and it reads "diverged" again until someone re-runs verify_snippet. + When the shape ledger has judgments against this snippet, the response carries `instances` (shapes classified as conforming to it — the structured consumer map) and/or `variants` (named departures, each with diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 5365b68..064b685 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -987,6 +987,46 @@ async def _refresh_provenance(note, commit_sha: str) -> None: await notes_svc.update_note(note.user_id, note.id, data=data) +def _verdict_still_vouches(note, fields: dict, fetched_commit_sha: str) -> bool: + """Does a standing `ok` verdict still speak for this body, at this commit? + + Containment (cached code ∈ fetched file) is the fast path, and it is right + for a record kept verbatim. It is WRONG for a deliberately annotated one + (#2782): a record whose job is to say why the shape is what it is carries + commentary the source does not, so containment fails forever and the record + reads `diverged` on every pull. That turns the one honest drift signal into + a permanent false positive — and annotation is a sanctioned record style, + so this is two deliberate designs colliding, not a malformed record. + + The escape hatch is the verdict itself. `verify_snippet` is precisely where + a human or agent already judged this body a faithful rendering of that + source, and `verification.commit_sha` records the repo commit they judged + it at — a field whose own docstring (#2688) anticipated this use: "makes + 'the REPO moved on since the check' computable, once the forge integration + can compare it against the current head." This is that comparison. + + All four conditions, and none is optional: + - the verdict says `ok`; + - it has not EXPIRED — `verification_view` recomputes `code_sha` against + the record's current body, so editing the record retires the verdict; + - it was not INVALIDATED by a push touching the location (#2691); + - the file we just fetched is at the very commit the verdict was stamped + at. Any later commit means nobody has judged what is there now. + + The last one is what keeps this honest: it vouches for a body against ONE + known commit, never against whatever the source has become since. The + moment the file moves, containment resumes as the authority and the record + reads `diverged` until someone re-verifies — which is the correct outcome, + because at that point nobody has looked. + """ + if not fetched_commit_sha: + return False + view = verification_view(note, fields) + if view.get("status") != VERIFY_OK or view.get("needs_attention"): + return False + return view.get("commit_sha") == fetched_commit_sha + + async def attach_live_body(note, data: dict) -> None: """Decorate a PULL response with forge-checked freshness (#2690). @@ -1115,6 +1155,14 @@ async def attach_live_body(note, data: dict) -> None: _refresh_provenance(note, fetched.commit_sha), site="pull provenance-refresh", ) + elif _verdict_still_vouches(note, fields, fetched.commit_sha or ""): + # Containment failed, but an unexpired `ok` verdict stamped at exactly + # this commit already judged this body a faithful rendering of it — + # the annotated-record case (#2782). Trust the judgment over the + # substring test; `data["verification"]` travels in the same payload, + # so a reader can see the basis rather than take "current" on faith. + data["body_source"] = "forge" + data["body_freshness"] = "current" else: data["body_source"] = "cache" data["body_freshness"] = "diverged" diff --git a/tests/test_snippet_live_body.py b/tests/test_snippet_live_body.py index 47eea42..004e95b 100644 --- a/tests/test_snippet_live_body.py +++ b/tests/test_snippet_live_body.py @@ -272,3 +272,89 @@ async def test_forge_failure_inside_lookup_never_breaks_the_pull(): data = _data() await svc.attach_live_body(_note(), data) assert "body_source" not in data + + +# --- #2782: an annotated record is not a diverged one ------------------------ +# Containment is right for a verbatim record and wrong for a deliberately +# annotated one: the commentary that makes the record worth reading is exactly +# what makes `cached in fetched` false, forever. These pin the escape hatch — +# a standing `ok` verdict stamped at the commit we just fetched — and, just as +# importantly, every condition that must switch it back off. + +ANNOTATED = "# Membership is the contract — this record says WHY, the source can't.\n" + CODE + + +def _ok_verdict(code=ANNOTATED, commit=SHA, **extra): + verdict = svc.compose_verification( + status=svc.VERIFY_OK, checked_code_sha=svc.code_sha(code), commit_sha=commit + ) + verdict.update(extra) + return verdict + + +async def _freshness(data, *, file_commit=SHA, content=CODE): + forge = _forge_with(lambda r: _file_response(content, commit_sha=file_commit)) + with _patched(forge), patch.object(svc.notes_svc, "update_note", AsyncMock()): + await svc.attach_live_body(_note(), data) + await background.drain() + return data["body_source"], data["body_freshness"] + + +async def test_annotated_record_with_a_standing_verdict_reads_current(): + """The bug: the record's commentary is absent from the source, so + containment fails and every pull said `diverged`. A verdict that already + judged this body faithful, at this very commit, outranks the substring.""" + data = _data(code=ANNOTATED, verification=_ok_verdict()) + assert await _freshness(data) == ("forge", "current") + assert data["snippet"]["code"] == ANNOTATED # still never rewritten + + +async def test_the_verdict_vouches_for_one_commit_only(): + """The guard that keeps the hatch honest. The file has moved past the + commit the verdict was stamped at, so nobody has judged what is there + now — containment resumes as the authority and the record reads diverged + until someone re-verifies.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(commit="a" * 40)) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_an_expired_verdict_does_not_vouch(): + """The record was edited after the check, so `code_sha` no longer matches + and the verdict describes a body that is not this one.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(code="def other(): pass")) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_a_push_invalidated_verdict_does_not_vouch(): + """A push touched the recorded location since the check (#2691) — the repo + moved under the verdict even though the record didn't.""" + data = _data(code=ANNOTATED, verification=_ok_verdict(invalidated_by="c" * 40)) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_only_an_ok_verdict_vouches(): + """A drifted verdict is evidence AGAINST the body, not for it.""" + verdict = svc.compose_verification( + status=svc.VERIFY_CHANGED, checked_code_sha=svc.code_sha(ANNOTATED), commit_sha=SHA + ) + data = _data(code=ANNOTATED, verification=verdict) + assert await _freshness(data) == ("cache", "diverged") + + +async def test_a_verbatim_record_still_takes_the_containment_path(): + """No regression: the happy path does not route through the hatch, and an + unverified verbatim record is still confirmed by containment alone.""" + data = _data(code=CODE) + assert await _freshness(data) == ("forge", "current") + + +async def test_a_verdict_predating_commit_stamping_does_not_vouch(): + """Verdicts recorded before `commit_sha` existed (#2688) carry no commit to + compare, so they cannot tie the body to a known state of the source. They + fall through to containment rather than vouching on age alone.""" + verdict = svc.compose_verification( + status=svc.VERIFY_OK, checked_code_sha=svc.code_sha(ANNOTATED) + ) + assert "commit_sha" not in verdict + data = _data(code=ANNOTATED, verification=verdict) + assert await _freshness(data) == ("cache", "diverged")