Skip to content

fix(advisor): the front door was a thin caller, not a structural dead end - #19

Closed
stranske wants to merge 2 commits into
claude/skill-advisor-wiringfrom
claude/advisor-reach
Closed

fix(advisor): the front door was a thin caller, not a structural dead end#19
stranske wants to merge 2 commits into
claude/skill-advisor-wiringfrom
claude/advisor-reach

Conversation

@stranske

Copy link
Copy Markdown
Owner

Stacks on #17 (same file), so the base is #17's branch — the diff here is only this change. Merge #17 first.

The claim I got wrong

I reported that 35 of 41 capabilities were structurally unreachable from the advisor. That was wrong, and it mattered — it framed the central problem of this whole effort as a design limit to work around instead of a caller to fix.

capabilities._matches_trigger matches a {"kind": k, "name": n} matcher against a same-named field the caller supplies, and its own comment says so: "Adding a new trigger kind is then a caller-side change, not an edit here." The advisor was handing it {repository, task_type, lane} and nothing else.

adversarial-review, matcher {kind: closer_gate, name: high_stakes_review}
  advisor trigger today   -> (False, ['closer_gate_not_in_trigger'])
  same matcher, one field -> (True, [])

What changed

  1. One constant. DIRECT_ENTRY was {"offload": "offload"} under a comment claiming it mirrored dispatcher.TASK_TYPE_CAPABILITY. It didn't — the dispatcher routed runtime_ac to runtime-ac-checks while the advisor named deliberate-break-verifier for the same work. Now derived from the dispatcher.
  2. Context forwarded (context=, --context), confined to kinds the ledger declares. Absent context still fails closed.
  3. The whole denominator. Every non-match reports the reason _matches_trigger already returned, plus an entry_requirement derived from its own matcher. Those reasons were computed and discarded — that discard was the silence.

Numbers

before after
reachable, free text 6 7
reachable, real caller context 6 12
accounted for with a reason 6 of 41 41 of 41

No matcher rewritten, no task-signal widened. Widening keywords to move the number would corrupt the associations this feeds.

Break→revert

  • Reverting DIRECT_ENTRY: fails on dispatcher routes 'runtime_ac' to 'runtime-ac-checks' but the advisor does not name it.
  • Removing reason-reporting: fails with all 39 unaccounted capabilities named.

verify.py: 353 passed, 0 failed, 0 skipped, 81/81 selftests, 5/5 gates. The repo's own test_verifying_the_system_never_writes_the_live_ledger guard caught a writing load() in my selftest — fixed to load_declared().

🤖 Generated with Claude Code

… 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>
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef33cd61-b6c5-4229-8da9-7d518ee75f39

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant