fix(playbook): a repo gotcha an auditor cannot see does not exist - #38
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 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 |
Found live during a repo-audit run of stranske/Trend_Model_Project. Three
defects in `repo_knowledge`, all of which made the playbook worse than silent.
1. THE SUMMARY WAS FALSE. It read "Trend opener work cuts from phase-3, not the
default branch" while `phase-3` IS that repo's default branch, so a reader
went looking for a `main` that does not exist. Checked every other seeded
summary against live state: the rest are `main` and make no branch claim, and
Counter_Risk's Black gate and the LMS Postgres claim both hold. The same wrong
belief was in `provision.py`'s comment, next to the literal the registry was
duplicating; the SEED now READS `provision.BASE_BRANCH_OVERRIDES` instead of
repeating the branch name, because a matching pair of literals can drift and
one name cannot.
2. EVERY SEEDED INVARIANT WAS TASK-TYPE SCOPED, so a review/audit consult got
308 chars where an implement consult got 627 — the lint convention, the base
branch and the owner direction were all filed under someone else's task_type.
`task_types`/`lanes` now mean "only matters while doing that kind of work";
a repo invariant carries no scope. The base branch is no longer gated on
`lane == "opener"` either, in the prompt, the AGENTS.md export and memory
search. `scope_audit()` reports scoped-vs-invariant-scoped in ONE place (12
scoped reads as housekeeping; 12 scoped / 12 invariant is instantly the bug),
`--audit-scopes` exits non-zero on the second number, and the selftest fails
if a seeded invariant is re-scoped.
3. THE HIGHEST-VALUE GOTCHA WAS ABSENT AND THE ADVISOR CONTRADICTED IT.
`frontend-verifier` is bound for this repo at two surfaces while the repo's
own audit record says `frontend_verify.py` snapshots its Streamlit SPA before
the websocket render completes. `repo-playbook` was offered in the same
response and did not carry it. New typed `contraindications` section
({capability, reason, instead, evidence}), rendered in the playbook and read
by `capability_advisor.advise(repository=...)` on BOTH answer paths —
including the classification-miss path a free-text audit consult actually
lands on. It annotates and de-ranks; it never conceals, because a concealed
candidate can never earn the evidence that would clear it. Repo-scoped, not
surface-scoped: a surface demotion would unbind the capability everywhere.
DEDUP FIRST (recorded in the repo-playbook ledger `notes`, not only here):
`contraindicat|not_recommended|discourag|denylist|blocklist` → zero hits, so the
contraindication is genuinely new. `suggest_from_docs` ALREADY reads an arbitrary
root plus an explicit `--repo`, so it CAN mine an out-of-tree audit README —
verified by running it before writing anything. It was extended, not rebuilt: it
split wrapped bullets at the physical newline and had no section scoping, so
round-history prose arrived as durable repo knowledge. `_doc_blocks` + `sections=`
fix both, and a named section implies the signal filter because the single most
valuable note in the real README carries no must/should/never token and would
have been dropped on the floor.
Ran the ingestion across all 12 `Code/Audits/<repo>/README.md`: only Trend has a
"Standing notes for the next round" section (21 candidates). Four were verified
against `phase-3` and seeded; the section also asserts "CI gates ruff check only",
which is FALSE — Black is gated via `pr-00-gate.yml format_check: true` — so the
notes stay a review queue, not a source of truth.
The SEED is only written when the registry file is ABSENT, so a code-only fix
would leave every running instance still reading the wrong line. Hence schema v2
plus a `load()` migration: exact-match text correction, seeded items and their
scope reasserted, instance-added entries never touched, idempotent, and
best-effort on write so a read-only volume cannot stop prompts being built.
`save()` is now write-then-rename, because `load()` writes on read and
`append_context` reaches it on every dispatch.
Verification: repo_knowledge / capability_advisor / capabilities selftests green,
each new behaviour carrying a deliberate-break→revert case. `verify.py`: 365
passed, 368 collected (unchanged from baseline), 82/82 selftest modules, 0 skips.
The 3 pytest failures and the two red gates are PRE-EXISTING and reproduce with
this change stashed — all three are the single ledger row `evidence-acquisition`,
registered 2026-08-22 against `evidence_acquisition.py:run`, a module that does
not exist in this tree. Not touched here; flagged separately.
The mirror is deliberately NOT synced — that is the owner's circuit breaker.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
70c412f to
fee9089
Compare
Mechanical re-export of the Orchestrator-managed section, produced by `repo_knowledge.py --export-agents-md stranske/Workflows --repo-path . --apply`. Nothing here was hand-edited. Two definition-of-done lines lose a `(tasks: ...)` scope suffix. Upstream (stranske/Orchestrator#38) stopped scoping repo INVARIANTS by task_type: a rule that is true regardless of the work being done was only reaching consults whose task_type happened to appear in its list, so a `review` or `audit` consult was answered with silence. Both of these state repo conventions that hold no matter who is asking, so they now carry no scope. The two genuinely work-kind-specific rules below them keep theirs (`tasks: codemod`, `tasks: epic, cross_repo`), as does the testgen validation line — scoping still works and is still correct where the rule really is specific to a kind of work. Verified: `--validate-agents-md` goes `mismatched` -> `current` with no errors, no warnings and no missing refs, and the repo's own `scripts/check_agents_md_freshness.py` passes. Content outside the managed block is untouched. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…dition, never weaken its binding (#48) 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: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…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>
Found live during a repo-audit run of
stranske/Trend_Model_Project. Three defects inrepo_knowledge, all of which made the playbook worse than silent — areviewconsult got 308 characters, and the summary in them was false.1. The summary was factually wrong
It read "Trend opener work cuts from phase-3, not the default branch" while
phase-3is that repo's default branch, so a reader went looking for amainthat does not exist.Checked every other seeded summary against live state: the rest are
mainand make no branch claim; Counter_Risk's Black gate and the LMS Postgres claim both hold. But the Trend lint gotcha was wrong in the same direction —pr-00-gate.ymlgatesblack --check --line-length 100alongsideruff check, so "CI convention is ruff check" omitted the actual format gate. Both corrected.The same wrong belief lived in
provision.py's comment, beside the literal"phase-3"the registry was duplicating. The SEED now readsprovision.BASE_BRANCH_OVERRIDESrather than repeating the branch name — a matching pair of literals can drift, one name cannot — with a selftest that fails if they ever disagree.2. Every seeded invariant was
task_type-scopedreview/audit/ux/NonetestgenimplementThe lint convention, the base branch and the owner direction were all filed under someone else's
task_type. Addingreview/auditto each list would just drift on the next task type, so the rule is now:task_types/lanesmean "only matters while doing that kind of work"; a repo invariant carries none. The base branch is no longer gated onlane == "opener"either — in the prompt, theAGENTS.mdexport, or memory search.scope_audit()reports both numbers in one place:3 scoped, 0 invariantreads differently from3 scoped, 3 invariant.--audit-scopesexits non-zero on the second, and the selftest fails if a seeded invariant is re-scoped.3. The highest-value gotcha was absent, and the advisor contradicted it
frontend-verifieris bound for this repo at two surfaces while the repo's own audit record saysfrontend_verify.pysnapshots its Streamlit SPA before the websocket render completes.repo-playbook— the capability whose whole job is per-repo gotchas — was offered in the same response and did not carry it.New typed
contraindicationssection ({capability, reason, instead, evidence}), rendered in the playbook and read bycapability_advisor.advise(repository=…)on both answer paths, including the classification-miss path a free-text audit consult actually lands on. Reproducing the original consult:It annotates and de-ranks; it never conceals — a concealed candidate can never earn the evidence that would clear it, the same reason binding does not conceal. Repo-scoped rather than surface-scoped on purpose: a surface demotion would unbind the capability for every other repo.
Dedup finding (recorded in the
repo-playbookledgernotes, not only here)contraindicat|not_recommended|discourag|denylist|blocklist→ zero hits, so the contraindication is genuinely new.suggest_from_docsalready reads an arbitrary root plus an explicit--repo, so it can mine an out-of-tree audit README — verified by running it before writing anything. Extended, not rebuilt: it split wrapped bullets at the physical newline (candidates starting mid-sentence) and had no section scoping, so round-history prose arrived as durable repo knowledge._doc_blocks+sections=fix both.must/should/nevertoken and would otherwise have been dropped on the floor.Ran the ingestion across all 12
Code/Audits/<repo>/README.md: only Trend has a "Standing notes for the next round" section (21 candidates). Four were verified againstphase-3and seeded. The section also asserts "CI gates ruff check only", which is false — so these notes stay a review queue, not a source of truth.Why a schema migration
The SEED is only written when the registry file is absent, so a code-only fix would leave every running instance still reading the wrong line. Hence schema v2 plus a
load()migration: exact-match text correction, seeded items and their scope reasserted, instance-added entries never touched, idempotent, and best-effort on write so a read-only volume cannot stop prompts being built.save()is now write-then-rename, becauseload()now writes on read andappend_contextreaches it on every dispatch.Verified against a copy of the real registry: Trend corrected to 7 unscoped gotchas + 1 contraindication, and Workflows' two instance-added scoped items kept their scope untouched.
Verification
Selftests green across
repo_knowledge/capability_advisor/capabilities, each new behaviour carrying a deliberate-break→revert case.verify.py: 365 passed, 368 collected (unchanged from baseline), 82/82 selftest modules, 0 skips.The 3 pytest failures and 2 red gates are pre-existing and reproduce with this change stashed. All three are the single ledger row
evidence-acquisition, registered 2026-08-22 againstevidence_acquisition.py:run— a module that does not exist in this tree. Untouched here; being handled separately.Downstream consequence
stranske/Workflowsis the only fleet repo with a committed managedorch-playbookblock in itsAGENTS.md. Unscoping the two seeded definition-of-done items changed the export, sovalidate_agents_md_exportnow reportsstatus: mismatched(a warning,ok: true). It needs a two-line re-export in that repo.🤖 Generated with Claude Code