fix(advisor): the front door was a thin caller, not a structural dead end - #20
Merged
Conversation
… end
`capability_advisor` could name 6 of 41 capabilities from free text, and the
other 35 came back as ABSENCE — no entry, no reason, indistinguishable from
"nothing else exists". That was read as a structural limit. It is not.
`capabilities._matches_trigger` matches a `{"kind": k, "name": n}` matcher
against a SAME-NAMED FIELD THE CALLER SUPPLIES, and says so itself: "Adding a
new trigger kind is then a caller-side change, not an edit here." The advisor
was passing a three-field trigger (repository/task_type/lane) and no kind
fields. Supply the context a caller actually has and those capabilities match
immediately — proven in the selftest for `closer_gate`/`adversarial-review`.
Three changes, no matcher rewritten and no task-signal widened (widening
keywords to move the number would corrupt the very associations this feeds):
1. ONE CONSTANT. `DIRECT_ENTRY` was a one-entry literal whose comment claimed
it mirrored `dispatcher.TASK_TYPE_CAPABILITY`. It did not — so the dispatcher
routed `runtime_ac` to `runtime-ac-checks` while the advisor named
`deliberate-break-verifier` for the same work. The two halves of the system
disagreed about one task type. Now derived from the dispatcher's map.
2. CONTEXT IS FORWARDED (`context=`, and `--context` on the CLI), confined to
kinds the ledger actually declares. Absent context still FAILS CLOSED.
3. THE WHOLE DENOMINATOR. Every non-match now reports its named reason from
`_matches_trigger` plus an `entry_requirement` derived from its own matcher
("entered at ci_workflow 'maint-87-docs-drift-fix-agent'"). Those reasons
were already computed and thrown away; discarding them is what turned 35
capabilities into silence.
Reach: 6 -> 7 from free text alone, 6 -> 12 with real caller context, 41 of 41
now accounted for with a reason. `_selftest_reach` pins it, because
`adversarial-review` and `docs-drift-fix-agent` both carry advisory match
history proving they were once reachable and dropped out unnoticed when their
matchers were tightened. Reach had never been measured, so it fell in silence.
Break->revert: reverting DIRECT_ENTRY fails on the dispatcher disagreement;
removing the reason-reporting fails with all 39 unaccounted capabilities named.
verify.py: 353 passed, 0 failed, 0 skipped, 81/81 selftests, 5/5 gates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…icating #18 CI caught a real defect in the first version of this change: the reach selftest asserted a floor against the LIVE ledger, which holds 41 rows here and 14 on a clean runner. It passed locally and failed CI — a machine-local assertion wearing the clothes of a correctness test, which is the shape this repo's skip discipline exists to catch. Fixed by isolation rather than by skipping, per .verify-floor.json's own rule ("Moving a test back below the ceiling is the preferred way to lower it — fix what made it machine-dependent, rather than agreeing to check less"): * MECHANISM assertions now run on a SYNTHETIC ledger, so they execute on every machine: a kind-based capability is reported with its named reason and entry requirement rather than as absence; context forwarding reaches it; and absent, empty or WRONG context still fails closed. * THE DRIFT GUARD is now code vs code. `DIRECT_ENTRY` is hoisted to `direct_entry()` so the dispatcher agreement is assertable with no ledger at all — drift would otherwise land unnoticed precisely on the clean runner where a ledger-dependent check can only skip. * REACH SHRINKAGE IS DELIBERATELY NOT CHECKED HERE. #18 already added `capability_firing_monitor.advisor_reach` with a declared baseline and an `advisor_reach_regression` defect. A second copy would be a parallel inventory. Said so in the code, at the point someone would add it back. Net: no new skip is spent (the one PREREQUISITE mark on a bare runner is the pre-existing front-door section), and CI now checks MORE than before. Also fixed: the synthetic-ledger assertion originally required not_applicable to equal exactly the two fixtures. `capabilities.load` seeds KNOWN_DECLARATIONS into any ledger it reads, so that was a test coupled to an unrelated constant — containment plus a denominator computed from the ledger instead. Break->revert, all three discriminating: emptying the dispatcher map fails on the task-type disagreement; removing reason-reporting fails with "came back as SILENCE, not as named non-matches"; letting context be assumed instead of fail-closed fails on the wrong-value case. verify.py: 353 passed, 0 failed, 0 skipped, 81/81 selftests, 5/5 gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Limit details: You’ve used the included review currently available. Your 66 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
stranske
added a commit
that referenced
this pull request
Aug 22, 2026
… opened (#21) Two modules were publishing different "advisor reach" figures for the same front door — `capability_activation_audit.advisor_reach` said 5, `capability_advisor` said 7 — with no statement of which population each covered. Two disagreeing inventories of the same thing is how a parallel inventory starts, which is the defect both modules were written to prevent. They disagreed for a good reason that had just expired. #18 excluded `offload` from the reach baseline with an explicit rationale: it was reachable only through a HARDCODED map inside `advise()`, and "a hardcode cannot shrink quietly — deleting it IS a diff." That was correct when written. #20, merged the same day, replaced the hardcode with `capability_advisor.direct_entry()` DERIVED from `dispatcher.TASK_TYPE_CAPABILITY` — so dropping an entry from the dispatcher's map now narrows the front door with no diff in either module. The exemption that was right for a literal is exactly wrong for a derivation. * `advisor_reach` now reads the direct-entry map from `capability_advisor` — one source, consumed by both, instead of a second list here. * The derived set gets its OWN baseline (`ADVISOR_DIRECT_ENTRY_BASELINE`), so it can grow freely and cannot shrink without appearing in a diff. Declared and derived reach stay separate, because #18's reasoning for separating them holds; only the "no baseline needed" part died. * `total_reachable_count` publishes 7 next to declared 5 and direct-only 2, so the numbers reconcile in the output rather than in someone's head. * Fixed a pointer in `capability_advisor.py` that named `capability_firing_monitor.advisor_reach`; it lives in `capability_activation_audit`. A comment pointing at a symbol that does not exist is the rot #18 spent four citations cleaning up. Break->revert: emptying `direct_entry()` must regress the derived baseline (it does, asserted with the map patched out); neutering `direct_entry_regressed` fails the new assertion. verify.py: 366 passed, 0 failed, 0 skipped, 81/81 selftests, 5/5 gates. Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #19, which GitHub auto-closed when its base branch (#17's) was deleted on merge. Same commits, rebased onto
main.The claim this fixes — mine, and wrong
I reported that 35 of 41 capabilities were structurally unreachable from the advisor front door. That framed the central problem of this whole effort as a design limit to work around, when it was a thin caller.
capabilities._matches_triggermatches a{"kind": k, "name": n}matcher against a same-named field the caller supplies, and says so itself: "Adding a new trigger kind is then a caller-side change, not an edit here."advise()was handing it{repository, task_type, lane}and nothing else.Three changes
DIRECT_ENTRYwas{"offload": "offload"}under a comment claiming it mirroreddispatcher.TASK_TYPE_CAPABILITY. It did not — the dispatcher routedruntime_actoruntime-ac-checkswhile the advisor nameddeliberate-break-verifierfor the same work. Nowdirect_entry(), derived from the dispatcher's map.context=,--context), confined to kinds the ledger declares. Absent, empty or wrong context still fails closed._matches_triggeralready returned, plus anentry_requirementderived from its own matcher —"entered at ci_workflow 'maint-87-docs-drift-fix-agent'"instead of nothing. Those reasons were computed and discarded; that discard was the silence.No matcher rewritten, no task-signal widened — widening keywords to move the number would corrupt the associations this feeds.
What CI caught in the first version
The reach selftest asserted a floor against the live ledger (41 rows here, 14 on a clean runner). Green locally, red on CI — a machine-local assertion dressed as a correctness test. Fixed by isolation, not by skipping: mechanism assertions moved to a synthetic ledger so they run everywhere, and the drift guard became code-vs-code so it runs on the clean runner where drift would actually land unseen. No new skip is spent.
Reach shrinkage is deliberately not checked here — #18's
capability_firing_monitor.advisor_reachalready owns it with a declared baseline and anadvisor_reach_regressiondefect. A second copy would be a parallel inventory. That's said in the code, at the spot someone would add it back.Break→revert (three, all discriminating)
"came back as SILENCE, not as named non-matches".verify.pyon the rebased branch: 365 passed, 0 failed, 0 skipped, 81/81 selftests, 5/5 gates.🤖 Generated with Claude Code