fix(rules): a shortened rule line must not decide what it says about holding (#3851)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 40s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m24s
CI & Build / Build & push image (push) Successful in 26s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 40s
CI & Build / TypeScript typecheck (push) Successful in 52s
CI & Build / Python tests (push) Successful in 1m24s
CI & Build / Build & push image (push) Successful in 26s
CI run 6485 was red. Six failures, three causes, and only one of them was a stale test. THE REAL DEFECT. The compact branch dropped the `seen` TAIL along with the trigger, so a rule the session had already been told rendered exactly like one it had not. #3750's whole argument is that those are different claims — a repeat is rendered precisely because the session may no longer HOLD what it was told — and the tail is the entire difference a reader can act on. test_a_rule_the_session_already_holds_is_referenced_not_re_offered caught it within one commit, which is that guard working as intended. Fixed by keeping the tail and dropping only the trigger, which is both the cheaper and the safer cut: a trigger runs 300-400 characters after #3855, a tail about 100. Re-measured on the real renderer — top-full-plus-references is ~299 tokens against ~568 for five full lines, so about 2x the old single line rather than the 1.4x claimed before, for four more rules and no lost information. The comments carrying the old figure are corrected rather than left to read as a decision nobody made. THE FIXTURE THAT STRADDLED THE BAND. `_THREE_HITS` spanned 0.81-0.74 against a 0.05 band, so the act arms dropped its lowest hit and four cases of test_both_recorders_report_the_same_rules_for_one_call failed reporting a count mismatch — under a message blaming the exclusion filter. A guard pointing confidently at the wrong subsystem costs more than no guard, because it is believed. Scores retightened to 0.81/0.80/0.79 and the precondition is now asserted by a named test, so a future band change is told where the problem is instead of through four confusing failures. THE STALE CONSTANT GUARD. test_the_rule_arm_asks_for_one_rule_not_two pinned RULEHINT_LIMIT == 1 — a real decision, correctly guarded, for a world with a resident set. Rewritten to pin what replaced it, as relationships rather than values (rule 115): the arm can return several, and rules are narrowed HARDER than the notes menu because they measured flatter, not sharper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
This commit is contained in:
@@ -221,8 +221,12 @@ _RULEHINT_BAND = 0.05
|
||||
# a fourth voice which speaks at full volume twice is where a reader stops
|
||||
# reading — is the half worth keeping, and it is answered by making the
|
||||
# later lines quieter rather than by refusing to have them. Top hit full,
|
||||
# the rest as references: ~198 tokens, 1.4x the old single line, for four
|
||||
# more rules.
|
||||
# the rest as references: ~299 tokens against ~568 for five full lines, so
|
||||
# roughly 2x the old single line for four more rules.
|
||||
#
|
||||
# The reference keeps its TAIL and loses only its trigger, which is both the
|
||||
# cheaper and the safer cut — see `_rule_hint_line`, where the first attempt
|
||||
# dropped the tail as well and #3750's guard caught it within one commit.
|
||||
#
|
||||
# The consequence is deliberate and worth naming: a rule that keeps ranking
|
||||
# first for a recurring situation keeps being referenced, every time the
|
||||
@@ -1201,14 +1205,17 @@ def _rule_hint_line(rule, *, where: str, seen: bool, compact: bool = False) -> s
|
||||
like them it reads none of the others: it says how much ROOM this line
|
||||
gets, which is a fact about its rank among today's hits rather than about
|
||||
the rule. A compact line is still a full claim that the rule may apply —
|
||||
it simply cites the rule instead of quoting its trigger.
|
||||
it simply cites the rule instead of quoting its trigger. Crucially it
|
||||
still carries the `seen` tail, so the three axes stay genuinely
|
||||
independent: shortening a line must not decide what it says about whether
|
||||
the session is holding the rule.
|
||||
|
||||
WHY THE LATER LINES ARE QUIETER. Measured at #3851: a full line runs ~143
|
||||
tokens once the trigger is rendered, and #3855 tripled trigger lengths
|
||||
across the corpus, so five full lines cost ~646 tokens before every Bash
|
||||
call. Top-full-plus-references costs ~198 — 1.4x the old single line, for
|
||||
four more rules. The budget argument that once justified a single slot was
|
||||
real; what it actually forbids is four voices at full volume, not four
|
||||
across the corpus, so five full lines cost ~568 tokens before every Bash
|
||||
call. Top-full-plus-references costs ~299 — about 2x the old single line,
|
||||
for four more rules. The budget argument that once justified a single slot
|
||||
was real; what it actually forbids is four voices at full volume, not four
|
||||
voices.
|
||||
|
||||
The top hit keeps the full rendering because it is the one the ranker is
|
||||
@@ -1282,12 +1289,20 @@ def _rule_hint_line(rule, *, where: str, seen: bool, compact: bool = False) -> s
|
||||
"session's loaded set."
|
||||
)
|
||||
if compact:
|
||||
# No trigger, and a shortened tail. What survives is everything a
|
||||
# reader needs in order to DECIDE to pull it: the force (noun), the
|
||||
# identity (title) and the pointer. What goes is the trigger, which is
|
||||
# the expensive half and the half the top line already demonstrates.
|
||||
# THE TRIGGER GOES; THE TAIL STAYS. Only one of the two is expensive \u2014
|
||||
# a trigger runs 300-400 characters after #3855, the tail about 100 \u2014
|
||||
# so dropping the trigger is nearly the whole saving and dropping the
|
||||
# tail would be mostly sacrifice.
|
||||
#
|
||||
# It would also destroy the one thing #3750 exists to say. The tail is
|
||||
# what tells a reader whether they were already told this and may no
|
||||
# longer be holding it, and that is the entire difference they can act
|
||||
# on; a reference with no tail reads as a first surfacing whether it is
|
||||
# one or not. This branch shipped without it for one commit and
|
||||
# test_a_rule_the_session_already_holds_is_referenced_not_re_offered
|
||||
# caught it, which is the guard working exactly as #3750 intended.
|
||||
return (
|
||||
f"Also \u2014 {noun.lower()} \u201c{rule.title}\u201d: get_rule({rule.id})."
|
||||
f"Also \u2014 {noun.lower()} \u201c{rule.title}\u201d. {tail}"
|
||||
)
|
||||
return (
|
||||
f"{noun} that may apply {where} \u2014 \u201c{rule.title}\u201d"
|
||||
|
||||
@@ -124,11 +124,11 @@ def test_the_leading_line_carries_the_trigger(seen):
|
||||
def test_a_later_line_cites_its_rule_without_quoting_the_trigger(seen):
|
||||
"""Rank > 0 is a reference: identity and pointer, no trigger.
|
||||
|
||||
The trigger is the expensive half — it is what made a full line ~143
|
||||
tokens — and the leading line has already demonstrated the shape. Both
|
||||
assertions matter: dropping the trigger is the saving, and keeping
|
||||
`get_rule(id)` is what makes the saving safe, because a cited rule the
|
||||
reader cannot pull is just noise.
|
||||
The trigger is the expensive half — 300-400 characters after #3855 — and
|
||||
the leading line has already demonstrated the shape. Both assertions
|
||||
matter: dropping the trigger is the saving, and keeping `get_rule(id)` is
|
||||
what makes the saving safe, because a cited rule the reader cannot pull is
|
||||
just noise.
|
||||
"""
|
||||
line = _rule_hint_line(
|
||||
fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER),
|
||||
@@ -139,14 +139,48 @@ def test_a_later_line_cites_its_rule_without_quoting_the_trigger(seen):
|
||||
assert "get_rule(4)" in line
|
||||
|
||||
|
||||
def test_shortening_a_line_does_not_decide_what_it_says_about_holding():
|
||||
"""A reference still tells a repeat from a first surfacing (#3750 x #3851).
|
||||
|
||||
This is the regression the first cut of #3851 actually shipped: the
|
||||
compact branch dropped the tail along with the trigger, so a rule the
|
||||
session had already been told read exactly like one it had not. #3750's
|
||||
whole argument is that the two are different claims — a repeat is rendered
|
||||
precisely because the session may no longer HOLD what it was told — and
|
||||
the tail is the entire difference a reader can act on.
|
||||
|
||||
`compact` and `seen` are independent axes. How much room a line gets is a
|
||||
fact about its rank; whether the session holds it is a fact about the
|
||||
ledger; and neither may be allowed to answer the other's question.
|
||||
"""
|
||||
rule = fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER)
|
||||
seen = _rule_hint_line(rule, where="here", seen=True, compact=True)
|
||||
fresh = _rule_hint_line(rule, where="here", seen=False, compact=True)
|
||||
assert seen != fresh
|
||||
assert "no longer hold it" in seen
|
||||
assert "not in this session's loaded set" in fresh
|
||||
|
||||
|
||||
def test_a_compact_line_is_materially_shorter_than_a_full_one():
|
||||
"""The cost claim, asserted rather than asserted-in-a-comment.
|
||||
"""The cost claim, asserted rather than left in a comment.
|
||||
|
||||
Not a token count — that would pin the tokenizer. Half the characters is
|
||||
the property that makes widening the arm affordable, and it is what fails
|
||||
if a later edit reintroduces the trigger into the compact branch.
|
||||
if a later edit puts the trigger back into the compact branch.
|
||||
|
||||
Measured against a REALISTIC trigger, because that is where the saving
|
||||
lives: the rules this arm carries run 300-400 characters of trigger after
|
||||
#3855, and a toy one-line trigger would make this pass on a compact branch
|
||||
that had stopped saving anything.
|
||||
"""
|
||||
rule = fake_rule(id=4, title="dev is home", when_to_apply=_TRIGGER)
|
||||
long_trigger = (
|
||||
"Opening or merging a `dev`->`main` pull request, running "
|
||||
"`git push origin main`, `git tag`, or minting a release, image tag "
|
||||
"or other public artifact. Also whenever CI has just gone green and "
|
||||
"the next step feels like shipping it, and whenever an earlier merge "
|
||||
"this session reads like standing permission for the next one."
|
||||
)
|
||||
rule = fake_rule(id=4, title="dev is home", when_to_apply=long_trigger)
|
||||
full = _rule_hint_line(rule, where="here", seen=False, compact=False)
|
||||
compact = _rule_hint_line(rule, where="here", seen=False, compact=True)
|
||||
assert len(compact) * 2 < len(full)
|
||||
|
||||
@@ -1026,10 +1026,37 @@ async def test_a_shown_hit_is_not_counted_as_suppressed():
|
||||
|
||||
_THREE_HITS = [
|
||||
(0.81, fake_rule(id=156, title="A wait with no deadline is a bug")),
|
||||
(0.77, fake_rule(id=157, title="A loop re-arms in a finally")),
|
||||
(0.74, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
||||
(0.80, fake_rule(id=157, title="A loop re-arms in a finally")),
|
||||
(0.79, fake_rule(id=161, title="Reach the forge through its MCP tools")),
|
||||
]
|
||||
|
||||
def test_the_three_hit_fixture_sits_inside_the_rule_band():
|
||||
"""The fixture's own precondition, asserted rather than commented (#3851).
|
||||
|
||||
The act arms band before they dedup, so a fixture whose spread straddles
|
||||
`_RULEHINT_BAND` loses its lowest hit to the BAND and then reports a count
|
||||
mismatch — under a message blaming the exclusion filter. That is the
|
||||
failure this file is least able to survive: a guard pointing confidently
|
||||
at the wrong subsystem costs more than no guard, because it is believed.
|
||||
|
||||
Not hypothetical. The spread was 0.07 against a 0.05 band, and four cases
|
||||
of `test_both_recorders_report_the_same_rules_for_one_call` failed that
|
||||
way the moment the band shipped.
|
||||
|
||||
Widening the band leaves this alone; narrowing it past the spread must
|
||||
retighten these scores, and says so here rather than through four
|
||||
confusing failures elsewhere.
|
||||
"""
|
||||
from scribe.services import plugin_context as pc
|
||||
|
||||
spread = _THREE_HITS[0][0] - _THREE_HITS[-1][0]
|
||||
assert spread < pc._RULEHINT_BAND, (
|
||||
f"the rule-arm fixture spans {spread:.3f} against a band of "
|
||||
f"{pc._RULEHINT_BAND}: the act arms will drop its lowest hit as "
|
||||
"out-of-band, and every count assertion below will blame the "
|
||||
"exclusion filter for it"
|
||||
)
|
||||
|
||||
_ARMS = [
|
||||
("write_path_rule", _run_arm),
|
||||
("pre_tool_rule", _run_tool_arm),
|
||||
|
||||
@@ -499,13 +499,31 @@ def test_the_rule_bar_defaults_above_the_code_bar():
|
||||
assert pc.RULEHINT_DEFAULT_THRESHOLD > pc.WRITEPATH_DEFAULT_THRESHOLD
|
||||
|
||||
|
||||
def test_the_rule_arm_asks_for_one_rule_not_two():
|
||||
"""With a corpus this small, top-k does as much damage as the threshold:
|
||||
k=2 over a few dozen candidates means the second line is almost always the
|
||||
second-best noise, carrying the same confident framing as the first."""
|
||||
def test_the_rule_arm_asks_for_a_set_and_lets_the_band_narrow_it():
|
||||
"""WAS `..._asks_for_one_rule_not_two`, pinning `RULEHINT_LIMIT == 1`.
|
||||
|
||||
That guarded a real decision: with retrieval SUPPLEMENTING a 33-rule
|
||||
resident set, k=2 over a few dozen candidates made the second line the
|
||||
second-best noise wearing the first line's confident framing. Milestone
|
||||
394 removes residency, so this arm becomes the whole delivery and a
|
||||
`git push` governed by four rules cannot be served by one slot (#3851).
|
||||
|
||||
What replaces it is not simply a bigger k — that is the thing the old
|
||||
test was right to fear, because a fixed k fills its slots whether or not
|
||||
anything deserves them. The cap is a ceiling and the BAND is the control,
|
||||
so both must exist for the arm to be shaped as intended.
|
||||
|
||||
Pinned as relationships rather than values, like the threshold test above
|
||||
and for the same rule-115 reason: the band is a tuning number measured
|
||||
against one corpus, and a test asserting 0.05 would fail on every retune
|
||||
while proving nothing. What must not silently invert is that the arm can
|
||||
return several, and that rules — which measured FLATTER than notes, not
|
||||
sharper — are narrowed harder than the notes menu is.
|
||||
"""
|
||||
from scribe.services import plugin_context as pc
|
||||
|
||||
assert pc.RULEHINT_LIMIT == 1
|
||||
assert pc.RULEHINT_LIMIT > 1
|
||||
assert 0 < pc._RULEHINT_BAND < pc._AUTOINJECT_BAND
|
||||
|
||||
|
||||
# --- the minimum-substance floor on the semantic arm (#2223) ------------------
|
||||
|
||||
Reference in New Issue
Block a user