Repository navigation
When a gate's class is drawn on an axis orthogonal to the failure #123
blinkinglight
started this conversation in
General
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Split out of #122, because it stopped being about memory. That thread is about an entry point that spilled; this one is about why the check that should have caught it could not, and why that is not a story about anyone being careless.
Posting it as an argument rather than a conclusion. I have a preferred answer and I have tried to state the strongest objection to it, including one I have not answered.
The observation
ADR-044governs "every read path that returns memory content to a caller." It enumerates its own class by command rather than from memory, which is exactly the discipline this repo asks for:It then names its exclusions out loud, including the one that matters:
Every part of that is good practice. The class is derived from source, not remembered. The exclusions are enumerated rather than silent — this repo's stated position is that "a member left unmentioned reads as a member that does not exist." And the exclusion carries a re-entry condition, so it is not permanent by fiat.
And yet:
am_kg_queryis the one read surface in the system that actually exceeds the transport ceiling, and under that re-entry condition it can never join the class.am_kg_queryreturns facts. It will never render a memory throughtoView. The condition is unsatisfiable for the member it governs.The exclusion is drawn on payload type. The failure is payload size. Neither axis is wrong; they are just orthogonal, and the gate lives on the one that does not intersect the hazard.
The counter-example, in the same tree
What makes this interesting rather than a bug report is that the repo does the other thing too, one package over.
internal/mcpserver/wirekeys_test.gogates that everyomitemptywire key is described somewhere a caller can find it — and its own header says:Same repo, same rigour, same honesty about scope. Both records name their exclusions. So "document why you excluded it" is not the variable — both did that, and only one of them stayed safe.
The variable is whether the exclusion's exit condition is something a future commit can actually satisfy:
wirekeys_test.goADR-044toView" — a condition the excluded member cannot meetOne exclusion is a deferred obligation. The other is a permanent exemption wearing the costume of a deferred obligation. They read almost identically.
Comparing this against how the wider field frames it
I want to check this against what is usually said, because the failure is not novel and I would rather not reinvent a worse version of a known idea.
"That's just a coverage gap." Partly. Line and branch coverage measure what ran, not what was selectable, and this repo has already written the sharper version of that: "a mutant proves a test notices a change, never that the thing under test is reachable." Mutation testing would not have caught this either — mutate anything inside
am_kg_queryand its tests go red, because the tests exercise the surface fine. The surface works. What is missing is its membership in a policy. Mutation testing measures a suite against a component; nothing in the standard toolkit measures a policy against its universe."That's the scope-of-a-lint-rule problem." Closest analogue, and it is underrated in general. Every ESLint
overrides, everystaticcheckexclusion, every# type: ignoreis a policy decision that gets one review at authoring time and none afterwards. The industry answer is roughly expiring suppressions — a suppression that must be re-justified or it fails. That is a good idea and it maps here."Document the exclusion" / Chesterton's fence. This is the received advice, and this case is a clean refutation of it as sufficient. ADR-044 documented the fence and named the field it was made of. It did not help, because documenting why something is excluded says nothing about whether it can ever stop being excluded. I would state the amended rule as: an exclusion needs a falsifiable re-entry condition, not a justification. A justification is read once by a reviewer; a re-entry condition is evaluated by every future commit.
Where this repo is genuinely ahead of the standard practice: it already has meta-gates that check the checkers — a test asserting
AGENTS.mdonly names gates that exist, one assertingdoctoris actually registered inmain.go, and falsifiability halves that drive verdicts through a substitutabletesting.TBbecause a test cannot pin its own reporting. That is further than most codebases go. What none of them check is an exclusion list for satisfiable exit conditions — which is the one shape that reads as safe from every angle.Three ways out, argued rather than recommended
A. Require every exclusion to carry a falsifiable re-entry condition, and gate it.
For: cheap, matches what
wirekeys_test.goalready does voluntarily, and converts permanent exemptions into tracked obligations.Against, and I think this objection wins on its own terms: "falsifiable" is not machine-checkable. You would be gating on prose quality — and this repo has already measured and rejected exactly that move once, in ADR-037's Alternatives, where comment length and presence were tried as proxies and thrown out. A gate that can only check that a sentence exists is a human review wearing a check's clothes, and this codebase's own standard is that documentation is load-bearing precisely because a command reads it. I do not have an answer to this. It may be that this one is a review norm and not a gate, and saying so plainly is better than shipping a gate that cannot fail for the right reason.
B. Split the obligation instead of widening the class.
This is what I would argue for. ADR-044 currently fuses two obligations that happen to have applied to the same surfaces:
content_coverage. Genuinely type-scoped: you cannot report content coverage on a row that has no content. ADR-044 is correct as written for this.Fusing them is what let a size failure escape through a type-shaped door.
For: it keeps ADR-044 exactly as it is rather than stretching it, and the second obligation is already half-built —
am_kg_queryemitswithheld{cause}with a hint naming the parameter that restores what was withheld, and the search side'swithheldByBudget = "budget"constant says in its own comment that it borrowed that key-by-cause shape fromkg_query. The two sides were designed to converge and simply did not.withheld: {"budget": N}is a shape callers already parse.Against: the size-scoped class is large and mostly trivial — most responses will never approach 40,000 runes, so the gate's universe is dominated by non-hazards, and a gate whose universe is mostly harmless trains people to allowlist without reading. That is a genuine cost and I do not think it is fatal, but it is not nothing.
C. Do nothing server-side; sharding is the fix and it already works.
For: cheapest. It is measured, it works, and the graph is a navigation surface where a caller that learned the by-name discipline never comes near the ceiling.
Against, and this is the argument I find hardest to dismiss: the current fix is a convention enforced by nothing. "No node over ~35 leaves" lives in a skill and in prose. A single
am_kg_addfrom any session can push a node past the ceiling, and the failure mode is a successful call that delivers nothing — no error, no flag, nothing in the response that says so. So option C asks a distributed set of agents to hold a shared invariant with no mechanism, which is the exact shape of the defect the repo already catalogues under reachability. And there is nolimitparameter onam_kg_queryat all, so even a caller who knows the risk cannot bound the response voluntarily.What I would like argued back
am_kg_query). I cannot yet name one for the first.No code changed for any of this; it is a read of the tree at
8aefd81.All reactions