fix(advisor): deliver the guidance a caller was never sent, and say what would evaluate a precondition by stranske · Pull Request #97 · stranske/Orchestrator

stranske · 2026-08-24T03:36:30Z

The repair channel produced 13 proposals, 5 worth-having-and-broken, 0 repairs recorded, and nothing had consumed them. This drains four of the five. Two needed no code (the repairs had already landed and were never marked — recorded against PR #38 and PR #32). 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 itself, 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 in the first place. The field is always present and None when unknown: "no guidance recorded" and "this answer does not carry the field" must not 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 neither a scope judgement nor a defect in the dispatcher; it is offload's intrinsic boundary, and it was written down nowhere a caller could see. A capability that cannot say what it cannot take gets investigated and declined once per surface, forever.

offload, adversarial-review and frontend-verifier now state their boundary beside their call, 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.

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.

Verification

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:

break	result
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 (on the merge result, rebased onto 1211808):
  pytest:     442 passed, 0 failed, 0/26 max skipped (442 collected; floor 442)
  selftests:  85 of 85 modules ran, 0/7 max skipped
  5 of 5 gates green; mypy ratchet 26/26

No floor change: this adds no pytest tests (the new assertions live in module selftests) and nothing new is skipped, so .verify-floor.json is untouched.

The caller half, which is machine-local

~/.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). Outside this repository, so CI cannot see it. A one-time hand edit to a caller — the same fix the lane TOMLs took for --surface — and provably not a loop that rewrites a prompt to increase selection: the precondition axis annotates, and a selftest pins that the returned set and its order are identical with it populated and emptied.

Reported, not fixed

local_verify grades the deliberate-break proof per test command, so one discriminating test masks any number of tautologies in the same file (the Fine-Art-Archive F3 case). That was never in any proposal's defect evidence, so it was deliberately not folded into deliberate-break-verifier's repair marker.

🤖 Generated with Claude Code

…hat would evaluate a precondition 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: Claude Opus 5 <noreply@anthropic.com>

coderabbitai · 2026-08-24T03:36:42Z

Warning

Review limit reached

Next included review available in 30 minutes.

View limit details

Limit details: You’ve used the included review currently available. Your 73 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1a19ec94-3e6a-48ff-97f2-059ef88bd276

📥 Commits

Reviewing files that changed from the base of the PR and between 1211808 and 74b78c6.

📒 Files selected for processing (2)

ARCHITECTURE.md
src/capability_advisor.py

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

agents-workflows-bot · 2026-08-24T03:37:09Z

Workflow source needed

PR #97 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely.

Please do one of:

Add  or a normal Closes #123 / Related to #123 line.
Check one Workflow Source option in the PR body.
Add a hidden marker such as , , , , or .
Add a workflow source label such as workflow:source-direct-pr, workflow:source-local-request, workflow:source-review-followup, workflow:source-sync, or workflow:no-automation.

Once a valid source is present, this warning will not be reposted.

…t talked the capability out of its own job WHAT WAS WRONG. `HOW_TO_USE["offload"]` told every caller that offload is "a read-only, text-returning hand-off" that "cannot run code and read exit codes". Offload has never been read-only. Three independent places say so: * adapters.py picks `sandbox = "read-only" if mode == "assess" else (permission_mode or "workspace-write")` — the DEFAULT is workspace-write, and the branch above it can send `--dangerously-bypass-approvals-and-sandbox`, i.e. no sandbox at all; * dispatcher.offload's own docstring gives `isolate=True` the job of letting "multiple code-building offloads run in parallel without same-dir races"; * the orchestrate skill's pitfall #1 says the default IS the coding mode and `--mode assess` is the read-only one you must opt into. WHERE IT CAME FROM, since the reasoning was sound and only the scope was not. #97 added the clause from six real declines in one window, all of which were genuinely correct declines. The error was generalising them into a claim about the MECHANISM. What those six shared was not a sandbox restriction; it was that the work had to be witnessed first-person — run the guard and see it go RED, drive the browser, hold one whole grep trace. The agent can do the typing; the orchestrator does not get the seeing. That is the true boundary and it is one level in from where the sentence put it. WHY IT MATTERED RATHER THAN JUST READING WRONG. This table is stamped onto every advisor answer by `_attach_how_to_use`, so a false narrowing does not merely mis-describe a capability — it removes it from consideration for exactly the work it is best at. It did: a caller reading this entry planned a 12-module type-drain around read-only classification passes instead of write-mode isolated offloads. A capability documented into a smaller box than it occupies is indistinguishable, at the point of selection, from one that is unavailable. THE TEST WAS DEFENDING THE DEFECT. `_selftest_how_to_use` asserted `"read-only" in off and "text-returning" in off` — it pinned the false claim, so the fix could not have landed without changing it. Replaced with pins on the true mechanical facts (`workspace-write`, `isolate=True`) plus an explicit `"read-only" not in off`, which is strictly stronger: the old wording can no longer satisfy the suite. The `BOUNDARY`/`first-person` pins are untouched — the concept was always right. Break->revert demonstrated: restoring the read-only wording fails the assertion; reverting is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…t talked the capability out of its own job (#113) * fix(advisor): offload is write-capable — correct a boundary claim that talked the capability out of its own job WHAT WAS WRONG. `HOW_TO_USE["offload"]` told every caller that offload is "a read-only, text-returning hand-off" that "cannot run code and read exit codes". Offload has never been read-only. Three independent places say so: * adapters.py picks `sandbox = "read-only" if mode == "assess" else (permission_mode or "workspace-write")` — the DEFAULT is workspace-write, and the branch above it can send `--dangerously-bypass-approvals-and-sandbox`, i.e. no sandbox at all; * dispatcher.offload's own docstring gives `isolate=True` the job of letting "multiple code-building offloads run in parallel without same-dir races"; * the orchestrate skill's pitfall #1 says the default IS the coding mode and `--mode assess` is the read-only one you must opt into. WHERE IT CAME FROM, since the reasoning was sound and only the scope was not. #97 added the clause from six real declines in one window, all of which were genuinely correct declines. The error was generalising them into a claim about the MECHANISM. What those six shared was not a sandbox restriction; it was that the work had to be witnessed first-person — run the guard and see it go RED, drive the browser, hold one whole grep trace. The agent can do the typing; the orchestrator does not get the seeing. That is the true boundary and it is one level in from where the sentence put it. WHY IT MATTERED RATHER THAN JUST READING WRONG. This table is stamped onto every advisor answer by `_attach_how_to_use`, so a false narrowing does not merely mis-describe a capability — it removes it from consideration for exactly the work it is best at. It did: a caller reading this entry planned a 12-module type-drain around read-only classification passes instead of write-mode isolated offloads. A capability documented into a smaller box than it occupies is indistinguishable, at the point of selection, from one that is unavailable. THE TEST WAS DEFENDING THE DEFECT. `_selftest_how_to_use` asserted `"read-only" in off and "text-returning" in off` — it pinned the false claim, so the fix could not have landed without changing it. Replaced with pins on the true mechanical facts (`workspace-write`, `isolate=True`) plus an explicit `"read-only" not in off`, which is strictly stronger: the old wording can no longer satisfy the suite. The `BOUNDARY`/`first-person` pins are untouched — the concept was always right. Break->revert demonstrated: restoring the read-only wording fails the assertion; reverting is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(autofix): formatting/lint * chore(codex-autofix): apply updates (PR #113) * fix(advisor): state offload's full range, and untrack the build artefacts a bot committed TWO CHANGES, both found by the same PR. The first widens offload's description everywhere it is described; the second removes four build artefacts this branch was carrying. ===== 1. OFFLOAD'S DESCRIBED SCOPE ===== The earlier commit on this branch removed a false claim -- that offload was "a read-only, text-returning hand-off" that "cannot run code and read exit codes". Removing it was not enough, for two reasons. IT WAS NOT ONE STRING. Five surfaces told the same wrong story, so a caller who dodged one met the next: `HOW_TO_USE["offload"]`; `dispatcher.offload`'s own docstring ("this is how the seat offloads token-heavy READING"); ORCHESTRATOR.md's CLI bullet ("Offload one synchronous read/proposal"); and the orchestrate skill, in BOTH its body ("Offload (read / summarize / research)") and its frontmatter description, which is the text a model reads when deciding whether to invoke the skill at all. DELETING A FALSE SENTENCE LEAVES SILENCE, AND SILENCE NARROWS TOO. An entry that merely stopped saying "read-only" would say nothing about scope, and a capability described only in terms of reading is selected only for reading -- the same lost work, with nothing wrong on the page to notice. So every surface now LEADS with the range: writing and editing code across files, running builds/tests/linters and reading their exit codes, iterating until something passes, debugging, refactors, migrations, design, review, research, web reading. Big-context reading is named as ONE use, and the docstring says outright that naming only that use is part of how callers came to believe it was the only one. THE BOUNDARY IS NOW A CLOSED SET OF EXACTLY TWO, which is the structural half of the fix. An open-ended list of things offload "cannot" do invites the reader to keep extending it -- which is precisely how six correct declines became a false claim about the mechanism. The two: FLEET ACTIONS (no claim, no PR, no label -- and the entry now NAMES `dispatcher.delegate` as the answer, so "opens no PR" stops reading as a shortcoming), and FIRST-PERSON OBSERVATION (a report is not your own witness). Everything else is stated to be a scoping decision rather than a limit, so the reader gets a rule that resolves the next case instead of a list that grows. The skill's (2)-vs-(3) split was the worst of them, because it taught the error by STRUCTURE rather than by wording: "Offload (read/summarize/research)" against "Delegate a coding task" reads as offload=reading, delegate=coding. The real distinction is where the RESULT goes, and both options now say so -- offload returns it to you, delegate opens a PR on a claimed target, and both do code. ORCHESTRATOR.md's OTHER use of "read-only" was accurate and is kept, reworded. "an offload is read-only w.r.t. the fleet -- no claim/PR/label" is a true statement on a different axis; it is now "touches nothing on the FLEET", because that phrase colliding with the filesystem sense is plausibly what seeded the confusion in the first place. The selftest now pins the BREADTH as well as the caveat (`FULL RANGE OF WHAT AN LLM CAN DO`, and `FLEET ACTIONS` naming `dispatcher.delegate`). This is the load-bearing new assertion: the recurring failure here is narrowing by OMISSION, which no absence-check can catch -- only a positive claim can. (The skill lives at ~/.claude/skills/orchestrate/SKILL.md, outside this repo, so that edit is machine-local and not in this diff. Three commands in it were also BROKEN and repointed: they still referenced Orchestrator/dispatcher.py and Orchestrator/capacity.py, which stopped existing when the modules moved to src/.) ===== 2. THE BUILD ARTEFACTS ===== This branch was carrying src/UNKNOWN.egg-info/{PKG-INFO,SOURCES.txt,dependency_links.txt, top_level.txt}, committed by the repo's own autofix bot in "chore(codex-autofix): apply updates (PR #113)". A CI step runs `pip install -e .`, setuptools writes the metadata beside the sources -- named UNKNOWN precisely because pyproject.toml declares no [project] on purpose -- and the bot stages whatever the working tree contains. .gitignore had no *.egg-info/ entry and test_repo_artifact_hygiene.py knew only the langsmith-fleet family, so nothing local objected. CodeRabbit caught it. Untracked, ignored (*.egg-info/, build/, dist/), and covered by three new tests. BOTH build locations are listed because a checkout builds into src/ while the EXEC MIRROR IS FLAT. The tracked-check is deliberately separate from the ignored-check: these four files were committed BEFORE any pattern existed, and adding a pattern does nothing to a path git already tracks -- so an ignore-only test passes on a repo still carrying the debris. Globbed rather than named, since the directory is only called UNKNOWN while [project] is absent. Break->revert demonstrated: `git add -f` a PKG-INFO and the tracked test fails; reverting is green. Verification: `python3 src/verify.py` pytest: 451 passed, 0 failed, 0/26 max skipped (451 collected; floor 451) selftests: 85 of 85 modules ran, 0/7 max skipped VERIFIED -- 451 tests actually executed and passed, 85 selftests spoke, 5 of 5 gates green ruff and black clean at CI's settings (596 files unchanged). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

stranske merged commit 0eb9a67 into main Aug 24, 2026
17 checks passed

stranske deleted the claude/repair-channel-drain branch August 24, 2026 03:38

stranske mentioned this pull request Aug 24, 2026

fix(advisor): offload is write-capable — correct a boundary claim that talked the capability out of its own job #113

Merged

Navigation Menu

fix(advisor): deliver the guidance a caller was never sent, and say what would evaluate a precondition - #97

fix(advisor): deliver the guidance a caller was never sent, and say what would evaluate a precondition#97
stranske merged 1 commit into
mainfrom
claude/repair-channel-drain

stranske commented Aug 24, 2026

Uh oh!

coderabbitai Bot commented Aug 24, 2026

Review limit reached

Uh oh!

agents-workflows-bot Bot commented Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

stranske commented Aug 24, 2026

1. HOW_TO_USE was read by format_advice alone

2. The boundary belongs in that field as much as the call does

3. Naming a missing input is a diagnosis, and a diagnosis is not an instruction

Verification

The caller half, which is machine-local

Reported, not fixed

Uh oh!

coderabbitai Bot commented Aug 24, 2026

Review limit reached

Uh oh!

agents-workflows-bot Bot commented Aug 24, 2026

Workflow source needed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

1. `HOW_TO_USE` was read by `format_advice` alone