fix(advisor): a suppressed surface must actually be quiet - #32
Merged
Conversation
Found by the first two real audit runs against this system, independently, in the
same session.
`NO_BINDING` suppressed only the DECLARED half. So `repo-audit:phase-1` — whose
entire point is that the playbook says "Orient (bash only, NO agents)" — returned
`binding_for() == {}` as designed while `advise()` still offered
`deliberate-break-verifier` and `testgen-lane` from the keyword classifier. Two
agent capabilities recommended at a phase defined by having no agents.
The Workflows audit put it more precisely than I had: the deliberately-empty
binding was "byte-identical to a classification failure". capability_advisor
declares NO_BINDING with a rationale, says in its own comment that silent absence
and deliberate emptiness "must not look alike -- that is this repo's founding
defect", and then emitted none of it: phase-1 and a control consult against a
nonexistent surface returned the identical `reason`. The vocabulary already
existed for the other direction ("N capability(ies) are DECLARED...").
Suppression now covers the whole answer and says why: confidence
`"suppressed"`, empty `capabilities`, and a reason naming the surface and quoting
its declared rationale. "No agents here" is a statement about the CONTEXT, not
about one code path.
MY SELFTEST ASSERTED THE WRONG LAYER. It checked `binding_for(surface) == {}` --
the table -- and never what a CALLER receives. That is exactly the gap I have been
catching in other people's work this session. It now asserts on `advise()`:
empty capabilities, `confidence == "suppressed"`, and a reason that explains the
emptiness. Break->revert: reverting suppression to the binding layer fails with
the leaked `unbound-testgen` candidate printed in full.
verify.py unchanged otherwise; local suite still shows the three pre-existing
evidence-acquisition failures.
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: 41 minutes Limit details: You’ve used the included review currently available. Your 69 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 24, 2026
…hat would evaluate a precondition (#97) Draining the repair channel: it produced 13 proposals, 5 worth-having-and-broken, and 0 repairs recorded. Two of the five were repairs that had ALREADY LANDED and were never marked (recorded against PR #38 and PR #32; nothing to change here). The other three were the SAME defect, twice: committed guidance that only the text renderer read, so the callers that matter never received it. 1. `HOW_TO_USE` WAS READ BY `format_advice` ALONE. Every real consult arrives through the `capability_advice` MCP tool and receives the result DICT, which carried `entrypoint`, `blocker` and `next_step` and never this. So a caller was offered `adversarial-review` with `blocker: "matched but a gate blocked invocation"` and no gate NAMED, went and read the ledger row, found `{kind: closer_gate, name: high_stakes_review}` and declined it as "a lane gate, not an audit dimension" — while the table held the direct call that answers exactly that. `_attach_how_to_use` stamps it onto every entry on both answer branches, and `format_advice` now reads it FROM THE ENTRY: one lookup, because reading the table twice is how the render and the answer came apart. The field is always present and None when unknown, so "no guidance recorded" and "this answer does not carry the field" cannot look alike. 2. THE BOUNDARY BELONGS IN THAT FIELD AS MUCH AS THE CALL DOES. Six `offload` declines in one window were one sentence repeated: the work had to be first-person (run the code and read exit codes, re-run a guard with the break in place, hold a whole grep trace, drive a browser). That is not a scope judgement and not a defect in the dispatcher — it is offload's intrinsic boundary, and it was written down nowhere a caller could see. `offload`, `adversarial-review` and `frontend-verifier` now state what they CANNOT take beside how to call them, and `repo-playbook` gets an entry at all. The binding is deliberately NOT narrowed: narrowing on structural declines is the demotion path, and demoting the fleet's most-used capability would silence what should be explained. 3. NAMING A MISSING INPUT IS A DIAGNOSIS, AND A DIAGNOSIS IS NOT AN INSTRUCTION. `frontend-verifier`'s `applies_to`/`observable_surface` declaration and its probe both worked; the sole caller supplied neither `repository` nor `repo_path`, so four decline records read "the binding's own precondition is never evaluated" against a mechanism that was fine. `unevaluated_because` already named the missing input and three audit rounds re-asked nothing. So `advise()` now returns `precondition.missing_inputs` — derived from the declarations through one `PRECONDITION_INPUT_FOR` table, so the remedy cannot drift from what is actually read — and `precondition.how_to_evaluate`, the re-ask in words, PRINTED rather than left under --json. It goes empty once the inputs are supplied: a remedy that prints when nothing is missing is noise a reader learns to skip. That is this workspace's runtime rule one level down — report the drainable quantity beside the blocking one. ASSERTED ON WHAT A CALLER RECEIVES, on a synthetic ledger, on BOTH return branches — the classification-miss branch is the one a free-text audit consult actually lands on, so covering only the classified one would leave the reported case untested. Six break->reverts, each CONFIRMED to discriminate: A drop the stamp from the classified branch -> RED B drop the stamp from the classification-miss branch -> RED (KeyError how_to_use) C delete offload's BOUNDARY clause -> RED D1 missing_precondition_inputs always [] -> RED D2 missing_precondition_inputs always both names -> RED E format_advice stops printing the remedy -> RED Break A is also caught by the pre-existing front-door assertion, so it was re-run against `_selftest_how_to_use` ALONE to prove the new check discriminates on its own — front-door needs a populated ledger and skips on a bare runner, which is exactly where the break would otherwise land unnoticed. verify.py: 442 passed / 442 floor, 0 failed, 0/26 skipped, 85/85 selftests, 43/43 can-fire, 5/5 gates, mypy 43/43. NO FLOOR CHANGE: this adds no pytest tests (the new assertions live in module selftests), and nothing new is skipped. The caller half is machine-local and outside this repository: ~/.claude/skills/repo-audit/SKILL.md now passes `repository` and `repo_path` and tells the auditor to read `how_to_use` rather than `blocker` (backed up in place). A one-time hand edit to a caller, the same fix the lane TOMLs took for `--surface` — not a loop that rewrites a prompt to increase selection, and provably not that: the axis annotates and a selftest pins that the returned set and its order are identical with it populated and emptied. Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
stranske
added a commit
that referenced
this pull request
Aug 24, 2026
…OMMAND (#100) `local_verify.verify()` ran `test_cmd` twice -- once in the worktree, once against the extracted base -- and returned ONE verdict from `red["ok"]`. `red["ok"]` goes False as soon as ANY test in the command fails against the base, so ONE genuinely discriminating test earned a PASS for every tautology sitting beside it in the same file, and named none of them. Hit for real: a three-test file where 2 of 3 were tautologies returned a bare PASS (Fine-Art-Archive audit finding F3). `_analyse_nodes` re-runs the candidate paths ALONE against the same extracted base with `pytest -v --tb=no` and reads the per-node outcomes: FAILED/ERROR `discriminates`, PASSED `hollow` (named by node id), SKIPPED/XFAIL/XPASS `inconclusive`. ONE extra subprocess, not one per node, sharing the caller's `timeout`. The result gains `hollow_nodes`, `node_verdict` and `node_analysis.counts`. ADVISORY BY CONSTRUCTION. `verdict`, `ok` and the CLI exit code keep their exact meaning, so `runtime_ac`, `synthesis_promotion` and `record_verdict` read what they always read. The finding travels on `reason`, which is what `record_verdict` writes to `outcomes.notes`, and node ids go into the completion event's `test_ids` -- the only test-identity field the payload schema has, since an extra key is REJECTED by `feedback._sanitize_completion_payload`. Scoping the probe to `test_paths` rather than the whole `test_cmd` tightens the evidence a second way: a failure in some unrelated test the command happens to run can no longer read as this change's proof. WHEN IT CANNOT ATTRIBUTE IT SAYS SO, with the prerequisite NAMED -- no pytest, a collection error, non-Python paths, a timeout -- and a PASS carrying an INDETERMINATE analysis appends that to its own reason. An empty hollow list and "could not look" read identically otherwise, which is the masking this change exists to remove. Two under-reporting traps closed, each proven by break->revert: - the probe parses UNTRUNCATED stdout. `_run`'s 4000-char tail drops the earliest nodes of any sizeable file. - the probe clears inherited `PYTEST_ADDOPTS`. `-x` stops at the first failing node -> false clean `node_verdict: PASS` with zero hollow nodes; `-n auto` activates xdist, whose verbose lines put the outcome BEFORE the node id -> nothing parses. Both measured with the guard removed. DEDUP (CLAUDE.md 0): hollow detection exists twice, both at coarser grain -- `local_verify.verify` per COMMAND and `objective_anchor.arm_signals` per ARM (base_pass boolean) -- and `testgen_gate.pytest_cmd(collect_only=True)` builds a pytest collection command but reads only `ok`. No per-node parser or attribution exists anywhere in the tree. The improvement log records this precision limit as STILL OPEN; extended `local_verify` rather than building anything new, and the `deliberate-break-verifier` capability's lifecycle state is unchanged. Selftest adds the mixed real+tautology fixture on the existing `_init_repo`, with a DELIBERATE BREAK -> REVERT on `NODE_OUTCOME_DISPOSITIONS`: count a base-passing node as part of the proof and the run reverts to exactly the old masking PASS. Also pins the two parse shapes that would under-report (an outcome word inside a summary error message; xdist's outcome-first line) and the named-prerequisite INDETERMINATE path. It needs only git and pytest, so it runs on a bare runner -- NO ceiling moved and nothing new is skipped. NO FLOOR CHANGE, and that is measured not assumed: the new coverage is in the module selftest, so pytest collects the same 442. Re-measured on the merge result after rebasing onto origin/main 146b458 (#98 and #91 landed underneath): 442 passed / 442 collected / floor 442, 85/85 selftests, 5/5 gates, mypy ratchet 20/20 unchanged and `local_verify` stays off the exempt list. Deliberately NOT recorded as a `capability_propensity.record_repair`: the `deliberate-break-verifier` repair proposal was drained on 2026-08-23 for the unrelated NO_BINDING-surface defect in PR #32, and this precision limit was never in that proposal's defect evidence. Recorded as its own finding on improvement-log item 0 instead. 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.
Found by the first two real audit runs against this system, independently, in the same session.
NO_BINDINGsuppressed only the declared half. Sorepo-audit:phase-1— whose entire point is that the playbook says "Orient (bash only, NO agents)" — returnedbinding_for() == {}as designed whileadvise()still offereddeliberate-break-verifierandtestgen-lanefrom the keyword classifier. Two agent capabilities recommended at a phase defined by having no agents.The Workflows audit put it better than I had: the deliberately-empty binding was "byte-identical to a classification failure." The module declares
NO_BINDINGwith a rationale, states in its own comment that silent absence and deliberate emptiness "must not look alike — that is this repo's founding defect," and then emitted none of it. Phase-1 and a control consult against a nonexistent surface returned the identicalreason.Suppression now covers the whole answer and says why:
confidence: "suppressed", emptycapabilities, and a reason naming the surface and quoting its declared rationale.My selftest asserted the wrong layer
It checked
binding_for(surface) == {}— the table — and never what a caller receives. That's precisely the gap I've spent this session catching in other people's work. It now asserts onadvise(): empty capabilities,confidence == "suppressed", and a reason that explains the emptiness.Break→revert: reverting suppression to the binding layer fails with the leaked
unbound-testgencandidate printed in full.🤖 Generated with Claude Code