feat(advisor): the applies_to axis — evaluate a capability's precondition, never weaken its binding - #48
Merged
Conversation
…dition, never weaken its binding Three independent audit rounds on 2026-08-23 hit the same defect from both sides, and the third one ruled out the obvious fix. THE FALSE POSITIVE. `frontend-verifier` was offered at `repo-audit:phase-2` and `repo-audit:dimension-4` on two repositories with no application UI at all. Its own binding reason is CONDITIONAL — "dimension 4 uses the ux-review-overlay WHEN OBSERVABLE SURFACES EXIST" — and the condition lived in prose nothing read. THE MIRROR-IMAGE FALSE NEGATIVE. `capability:reference-sync-hygiene-test-gate` was filtered out as not-applicable during an audit OF SYNC HYGIENE. Both rounds diagnosed one cause: the capability is scoped to the Orchestrator's own runtime while the audit target is another repo. `repo-audit:dimension-8` is the clearest case — four well-chosen capabilities whose concepts transferred and whose INSTRUMENTS did not, eleven declines across the three rounds, all of that one shape. AND THE REFUTATION, which is why this annotates instead of suppressing. On a third repo, one that does have a display surface, `frontend-verifier` was READY on its first `--doctor` call and produced the finding with the highest evidence-to-effort ratio of that audit — a provenance banner promising "every number below was measured on your Mac" three lines above fabricated grant scope, which the code-reading path had missed. Its propensity moved off the floor onto real positive evidence. So the axis CHANGES NO ORDER AND NO MEMBERSHIP: a selftest asserts the returned list is identical, in membership and in order, with the axis populated and emptied, and asserts directly that the precondition-failing capability keeps first place. DEDUP (CLAUDE.md §0, checked against the tip). `_annotate_contraindications` + `repo_knowledge.contraindications_for` landed on main in #38 and already annotate a candidate a repository's own record says does not work there. This is deliberately NOT a second copy. A contraindication is a RECORDED per-(repo, capability) judgement — "broken against THIS app" — and ranks last, because a recorded judgement is high-confidence. A precondition is an INTRINSIC per-capability declaration evaluated per consult, and only annotates. `switch-review` is Orchestrator-scoped for every audited repo, so expressing it as a contraindication would need a hand-written note in all thirteen repo records: an N x M table nobody maintains. And the Workflows false positive happened BECAUSE no note existed — a mechanism requiring someone to have written one cannot catch the case where nobody did. A capability can be both, and the selftest pins that both reach the caller. THE CHECK IS BUILT, not just declared. `detect_observable_surface` is a bounded, deterministic probe that returns the markers it matched, so a verdict is evidence that can be argued with rather than a heuristic's bare boolean. Validated against the three repositories the audits actually ran on: FALSE for the two with no application UI (and for this repository), TRUE for the Streamlit SPA and for the one with `src/fine_art_archive/ui/index.html` — exactly the two negative and one positive observation on record. `docs/` and `site/` are excluded deliberately; counting a generated API-docs tree is what would turn the probe back into the false positive. Verdicts are three-valued. Undeclared, an unnamed repository, and a repo fact whose checkout was not supplied are all NOT EVALUATED, never failures — collapsing them into False would silently reclassify all 43 capabilities, and into True would restore the original defect. An unevaluated precondition NAMES its missing input (`repo_path`), because a condition nothing can even attempt to check is what this replaces. `advise()` gains `repo_path`, and the pre-existing MCP schema guard caught it immediately — proving itself before I could forget. That guard now also reads the handler's AST, so every caller-settable field is asserted FORWARDED and not merely advertised. `evaluate_precondition` hands back `suggested_decline_kind: "precondition_unmet"`, the kind `capability_propensity.DECLINE_KINDS` marks NON-demotable, so the two halves cannot disagree: an axis that explained a mismatch while the ledger quietly demoted the binding for it would be the forbidden correction taking the long way round. Break->revert, all confirmed to fail without the fix: a failed precondition sinks the entry; folded into the existing sort key; drops the entry; suggests a demotable decline kind; suppresses the recorded contraindication; keeps the contraindication reason JSON-only; an unnamed repository treated as `self`; `both` treated as a mismatch; undeclared treated as a mismatch; no-checkout conflated with no-surface; `docs/` counted as an application surface. Two attempts did NOT discriminate and were rewritten: the first invariance fixture used a capability that was already last alphabetically, and the first shadowing break was unreachable given the annotation order (stated in a comment instead, since an assertion that cannot fail is decoration). 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: 34 minutes Limit details: You’ve used the included review currently available. Your 70 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
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.
The finding, from three sides
Three independent audit rounds on 2026-08-23 hit the same defect — and the third one ruled out the obvious fix.
False positive.
frontend-verifierwas offered atrepo-audit:phase-2andrepo-audit:dimension-4on two repositories with no application UI at all. Its own binding reason is conditional — "dimension 4 uses the ux-review-overlay when observable surfaces exist" — and the condition lived in prose nothing read.Mirror-image false negative.
capability:reference-sync-hygiene-test-gatewas filtered out as not-applicable during an audit of sync hygiene. Both rounds diagnosed one cause: the capability is scoped to the Orchestrator's own runtime while the audit target is another repo.repo-audit:dimension-8is the clearest case — four well-chosen capabilities whose concepts transferred and whose instruments did not; eleven declines across the three rounds, all of that one shape.And the refutation. On a third repo, one that does have a display surface,
frontend-verifierwas ready on its first--doctorcall and produced the finding with the highest evidence-to-effort ratio of that audit — a provenance banner promising "every number below was measured on your Mac" three lines above fabricated grant scope, which the code-reading path had missed. Its propensity moved off the floor onto real positive evidence.So this axis annotates and changes nothing else. It turns "investigate this offer in order to discover it cannot apply" into "dismiss it in one line" — the cost those rounds actually paid — and where the precondition holds, the offer is exactly what it was.
Dedup — this is NOT the contraindication mechanism
_annotate_contraindications+repo_knowledge.contraindications_forlanded on main in #38 and already annotate a candidate a repository's own record says does not work there. The two must not merge:switch-reviewis Orchestrator-scoped for every audited repo, so expressing it as a contraindication would mean a hand-written note in all thirteen repo records — a table nobody maintains. And thefrontend-verifier-on-Workflowsfalse positive happened because no note existed: a mechanism that requires someone to have written one cannot catch the case where nobody did. A capability can be both, and a selftest pins that both reach the caller.The check is built, not just declared
detect_observable_surfaceis a bounded, deterministic probe returning the markers it matched, so a verdict is evidence that can be argued with rather than a heuristic's bare boolean. Validated against the three repositories the audits actually ran on:stranske/Workflowsstranske/Trend_Model_Projectstranske/Fine-Art-Archivesrc/fine_art_archive/ui/index.htmlExactly the two negative and one positive observations on record.
docs/andsite/are excluded deliberately — counting a generated API-docs tree is what would turn the probe back into the false positive.Three-valued on purpose
Undeclared, an unnamed repository, and a repo fact whose checkout was not supplied are all not evaluated, never failures. Collapsing them into False would silently reclassify all 43 capabilities; collapsing them into True would restore the original defect. An unevaluated precondition names its missing input (
repo_path) — a condition nothing can even attempt to check is precisely what this replaces.advise()gainsrepo_path, and the pre-existing MCP schema guard caught it immediately, proving itself before I could forget. That guard now also reads the handler's AST, so every caller-settable field is asserted forwarded, not merely advertised.evaluate_preconditionreturnssuggested_decline_kind: "precondition_unmet"— the kind #45 marks non-demotable — so the two halves cannot disagree. An axis that explained a mismatch while the ledger quietly demoted the binding for it would be the forbidden correction taking the long way round.Break→revert
Confirmed to fail without the fix: a failed precondition sinks the entry · folded into the existing sort key · drops the entry · suggests a demotable decline kind · suppresses the recorded contraindication · keeps the contraindication reason JSON-only · an unnamed repository treated as
self·bothtreated as a mismatch · undeclared treated as a mismatch · no-checkout conflated with no-surface ·docs/counted as an application surface.Two attempts did not discriminate and were rewritten: the first invariance fixture used a capability already last alphabetically (so a sink changed nothing), and the first shadowing break was unreachable given the annotation order — stated in a comment instead, because an assertion that cannot fail is decoration.
Verdict
python3 verify.pyon the rebased branch: 368 passed, 0 failed, 83/83 selftests spoke, 0 skipped, 5 of 5 gates green. Contract docs updated (ARCHITECTURE.mdlayer-1 section,README.md). No diagram change: no stage, component, rail/role classification or role-registry entry moved.