[johnl-amd]

Summary

• routing_gate now fails when skills_missing_from_session is non-empty, alongside the two infrastructure checks already there. Placed before the accuracy bar, so neither a clean sweep nor --min-accuracy 0 can pass a room that was not the one asked for.
• Why: a staged skill the CLI never reports at session init is not in the room the run claims to measure. Its cases still run, every one reads as missed_trigger, and the report tells the author a description does not trigger when that description was never offered to the agent. The two outcomes are indistinguishable in the verdict.
• The condition was already detected and already rendered as a warning above the numbers. It just never reached the gate, so the run went green apart from a line that is easy to read past.
• Risk: low. One branch in one function, an optional third parameter so existing callers are unaffected, no change to what is measured or reported.

Why it is not hypothetical

Every enabled skill contributes one line to a listing the agent reads at session start, costing len(name) + 4 + min(len(description), 1536), plus a separator between entries. That listing has a budget:

context window in tokens x bytes per token x skillListingBudgetFraction
200,000 x 4 x 0.01 = 8,000 characters

Measured on one developer machine: 34 enabled skills needing 18,050 characters against that 8,000 budget, 2.3x over. At least 12 of the 34 are reduced to a bare name right now, in any session at the default context size.

On overflow nothing fails. Names are kept, descriptions are dropped, and the order is:

usageCount x max(0.5 ^ (daysSinceLastUse / 7), 0.1)

A skill staged for an eval has been invoked zero times, so its score is zero and it is first to lose its description by construction. Not merely likely to be near the front of the queue: last in line for restoration, every time, until it has been used, which it cannot be while it has no description to match on.

It also competes with whatever the runner already has. A real run against a catalog of 8 skills reported 17 further skills registered from the runner's own config, which this code already warns about separately.

Test plan

• 3 new cases in TestRoutingGate: a missing skill fails the run and names it; it outranks the accuracy bar at both 1.0 and 0.0; None and [] leave the gate alone.
• Full suite 229, green via python -m unittest discover -s tests -t ..
• Mutation-tested: reverting the new branch fails exactly those three and nothing else, so they are pinned to the behaviour rather than to the shape of the code.

Note

The failure message is longer than its neighbours on purpose. This is the quietest of the three infrastructure conditions, because the run still produces a plausible number, so the message says what the number would otherwise have been read as.

Related: #23 reports what a repo's own skills cost against the same budget, so a catalog can see the pressure it adds before it ships.

[johnl-amd]

Repository: amd/skillscope · PR: #22 · Head: c34542ed14902109dd66555a347694c075a64919 · Base: 0f734b2dd344cd72b5e016ad6c3b1ac2ce36f6f3 · Skill dirs reviewed: none (no skill or instruction file in scope) · Standard commit: 8ee477d0133d37cb598e57ba0e0513385c18f2cb

Approved

This pull request touches only application code and its test suite — skillscope/cli.py (a routing_gate gating change) and tests/test_skillscope.py (three new unit tests) — and contains no SKILL.md, CLAUDE.md, AGENTS.md, command file (commands/*.md), agent file (agents/*.md), or path-scoped rules file (frontmatter with paths:). The ai-instruction-quality standard applies only to AI instruction files, and none are part of this diff.

Per the review scope rules, that makes this a clean pass rather than a failure to review: there is nothing here to check against the checklist, so no findings, Tier 1 through 3, apply.

Note: attempting to add the review claim/release labels failed with an HTTP 403 (token cannot write labels on this repository), so the claim/release labeling step was skipped; the review itself was still completed and posted.

---

🤖 Automated skill review by an agent.
Model claude-sonnet-4-5 · took 1m · diff 5a6fdc903c0f
Not a human review. Reply here if a finding is wrong.

[nowycondro]

🤖 Automated review · review-code · head c34542ed

Review — needs work

Full review of the whole change (single pass: the diff is two tightly-coupled files, skillscope/cli.py and tests/test_skillscope.py, well within what one reviewer can read completely alongside their surrounding code).
No ticket named as closed by this change; #23 is mentioned only as related, informational work on the same budget problem.

Blocking

• docs/usage.md still describes only two unconditional infrastructure checks, now stale — docs/usage.md:182-185
  The user-facing docs say: "No value turns off the infrastructure checks — a run where nothing was graded, or where no skill activated anywhere, fails at any bar, since those numbers are an artifact rather than a result." This change adds a third such check (skills_missing_from_session, skillscope/cli.py:170-178) that also cannot be turned off by --min-accuracy, including at 0. The in-code docstring for routing_gate was updated to say "the first three answers are infrastructure," but the user guide enumerating this exact behavior was not, so a reader of the docs is told there are only two unconditional failure modes when there are now three.
  Confidence 85 · mechanical · Fix: add the missing-skill condition to the docs/usage.md "Routing, and the bar it is held to" section alongside the other two.

Non-blocking

None

Decisions for the author

None

Positive signals

• The new gate branch is placed before the min_accuracy <= 0 short-circuit, so --min-accuracy 0 cannot be used to bypass it — this is exercised directly by test_a_missing_skill_outranks_the_accuracy_bar and holds under mutation (see verification below).
• Failure message names the exact missing skills rather than just signaling a boolean failure, keeping the fix actionable for whoever reads the CI log.

Deployment notes

This changes CI behavior for any skillscope routing consumer: a run that previously went green with only a warning line (when skills_missing_from_session was non-empty) will now exit 1 regardless of --min-accuracy, including 0. Existing pipelines relying on the old pass-with-warning behavior will start failing after this merges.

What this covered

Read skillscope/cli.py (routing_gate, cmd_routing, and the only call site) and tests/test_skillscope.py (TestRoutingGate) completely, against commits 0f734b2d…c34542ed, plus the surrounding code that produces and consumes skills_missing_from_session in skillscope/routing.py (summarize, run_case) to check whether elevating this from a warning to a gate enlarges the blast radius of any existing weakness in that computation — it does not: visible_skills is only populated from a case that actually reached a session-init event, so a timed-out or inconclusive case cannot contaminate the missing-skill set.

Verified by running, not just reading: full suite (python -m unittest discover -s tests -t .) is green at 229 tests; reverting the new if missing_skills: branch and rerunning TestRoutingGate fails exactly the three new tests and no others, confirming each is pinned to behavior rather than shape. CI's own statusCheckRollup for this commit is all green (4 unit-test jobs + the throwaway-repo action run).

Did not run: history pass found nothing notable in recent cli.py/routing.py log; prior-changes pass — no accessible record of earlier review comments on these two files outside this PR's own (excluded) discussion thread; agent-instruction adherence — no CLAUDE.md/AGENTS.md/skill files are in this repo or touched by this diff, so nothing to check there. Not reconciled against this PR's existing review thread or comments.

---

Not a human review. This is an automated code review; please verify findings independently before acting on them.

[johnl-amd]

Review — needs work

Full review of the whole change: a small diff touching skillscope/cli.py (the routing_gate function and its single call site in cmd_routing) and tests/test_skillscope.py (TestRoutingGate).
Adds a third, unconditional ("infrastructure, not score") failure condition to skillscope routing. Mentions open PR #23 ("Report what a repo's skills cost in the startup listing") as related, non-overlapping work on the same skill-listing-budget pressure — accurate; #23 reports the cost, this change gates on a symptom of it. No ticket claimed as closed.

Blocking

• A single anomalous case can hard-fail an otherwise clean run — skillscope/routing.py (summarize(), the missing = sorted({...}) comprehension), consumed at skillscope/cli.py:170-178 and wired at skillscope/cli.py:431-435.
  The comprehension is a union over outcomes, not an intersection: a configured skill lands in skills_missing_from_session if it is absent from just one case's session-init report, out of however many cases the room runs — each case is an independently spawned claude -p subprocess (run_case), commonly run concurrently via ThreadPoolExecutor. Before this PR that union only produced a warning line in the markdown report; this change makes it an unconditional exit 1 that overrides even --min-accuracy 0. Nothing in the surrounding isolation logic (fresh workspace and optional fresh config dir per case) rules out a single transient session-init misreport, and the existing warning text itself already hedges with "may not have been installed for the run" rather than claiming certainty. This enlarges the blast radius of a pre-existing, single-occurrence-sensitive computation from a footnote to a hard CI/merge gate — so a run with 100% routing accuracy can now go red because of one outlier case among many.
  Confidence 80 · architectural · Fix: require the skill be missing from every graded case (or a clear majority/threshold) before gating on it, rather than any single occurrence; or re-verify before trusting one session's listing.

• Check ordering hides the new diagnostic behind the old one in the single-skill room — the simplest, documented-default case — skillscope/cli.py:162-178 and 428-435.
  The new missing_skills check runs after the existing totals["activations"] == 0 and totals["activations_expected"] check. In a single-skill routing room (first-class per the module's own docstring — "a repo with a single skill need not name it: there is only one room its skill can be in") where that one skill is exactly the one missing from session init, nothing else in the room could possibly have activated either, so the activations-zero branch is also true and fires first, returning the generic "no skill activated in any case -- the skills were not installed, or activation detection is broken." The new, specific message — naming the missing skill and the session-init mechanism — is never reached. The run still fails correctly, but the one piece of diagnostic value this PR adds is masked in exactly the configuration the repo documents as the common case.
  Confidence 85 · logic · Fix: check missing_skills ahead of (or together with) the activations-zero check, so the more specific, already-computed cause is shown whenever both are true.

• docs/usage.md still describes only two infrastructure checks — docs/usage.md:181-183 (section "Routing, and the bar it is held to").
  Unmodified by the diff, it reads: "No value turns off the infrastructure checks — a run where nothing was graded, or where no skill activated anywhere, fails at any bar, since those numbers are an artifact rather than a result." This enumerates only the two pre-existing checks. After this change a third one — a staged skill missing from session init — also fails the run at any bar, and nothing in this doc says so.
  Confidence 85 · mechanical · Fix: add the missing-skill case to this paragraph.

Non-blocking

None.

Decisions for the author

• Only one of three comparably-described "room is wrong" conditions is gated — tradeoff.
  unexpected_skills (an uninstalled skill fired) and extra_skills_in_session (the runner's own skills joined the room) remain warnings only, both before and after this PR; only skills_missing_from_session is promoted to a hard gate. The code's own comments describe extra_skills_in_session as affecting every case in the room ("compete for every prompt... room nobody asked for"), which by the same "infrastructure, not score" reasoning used to justify this change would seem to qualify too. One side: a missing skill guarantees a deterministic wrong verdict for every case expecting it, which is categorically worse than contamination that merely can bias results — a real distinction. Other side: the asymmetry isn't drawn from a stated principle in the code, and could look like incremental, one-condition-at-a-time hardening rather than a considered line. Worth confirming whether the other two are intentionally left for later.

Positive signals

• The new check is placed ahead of the min_accuracy <= 0 early return, and the tests exercise both bar=1.0 and bar=0.0 explicitly (test_a_missing_skill_outranks_the_accuracy_bar) — correctly closing the one loophole (--min-accuracy 0) that would otherwise have let a wrong room through silently.

Deployment notes

• This changes skillscope routing's exit behavior: a condition that previously only printed a warning line in the report (skills_missing_from_session non-empty) now fails the run unconditionally. Any CI pipeline currently green with that warning present will start failing after this lands — see the first Blocking finding above for why that can include runs that are not actually broken.

What this covered

Read completely: skillscope/cli.py and tests/test_skillscope.py at the PR head, skillscope/routing.py at the PR head (unchanged by the diff but read in full for the context missing_skills originates from), docs/usage.md, README.md, and docs/authoring-evals.md; confirmed no CLAUDE.md/AGENTS.md exists in the tree. Commit range 39eff043(origin/main)…c34542ed(pr/22, a single commit). Checked routing_gate's sole production call site and every reference to skills_missing_from_session/missing_skills for consistency (all consistent). Confirmed via GitHub's check-run API that all 5 CI checks on the PR head (unit tests on Linux/Windows x Python 3.10/3.12, plus the action's own smoke test) are green. History and prior-change passes found nothing contradicted by this commit; the repo's history shows a repeated pattern of promoting a detected-but-unenforced condition into a hard failure, which this change is consistent with.

Fan-out: three independent passes were run (a code-correctness/test-quality pass; a combined history/prior-changes/agent-instruction/code-comments/impact-analysis pass; a design-review pass on three targeted questions) — the first two attempts at launching the latter two passes hit a concurrency limit and were re-issued successfully; all three completed as independent, blind passes (none were given the PR description or discussion). The PR description itself was incidentally fetched once during scope resolution (a single gh pr view call requested baseRefName alongside other fields and returned the body text too); it was not passed to any of the three blind workers and was not used to shape or filter findings — it was read deliberately, as the method's Step 9 requires, only when checking the change's stated intent against what the review had already found.

Not reconciled against any discussion here — this PR had no review comments at the time of this review.

---

🤖 Automated review by an agent.
Model claude-sonnet-5[1m] · Not a human review. Reply here if a finding is wrong.

[johnl-amd]

Review — needs work

Full review of the whole change (small diff: skillscope/cli.py's routing_gate and its single call site, plus three new tests in tests/test_skillscope.py; fanned out to two independent blind workers — one code-correctness/test-quality/impact-analysis pass, one design-review pass — then consolidated here).
No ticket named as closed; the PR description names #23 as related, non-overlapping work on the same skill-listing-budget problem, which checks out.

Blocking

• A single case's incomplete session-init report can hard-fail an otherwise-correct run, and nothing can override it — skillscope/routing.py (summarize(), the missing = sorted({...}) comprehension), consumed at skillscope/cli.py:170-178 via cmd_routing (skillscope/cli.py:431-435).
  missing is a union over cases, not a majority or intersection: a skill name is added to it if even one case among however many ran (often dozens, concurrently, each an independent claude -p subprocess in its own temp workspace) has a non-empty-but-incomplete visible_skills list that happens to omit it — regardless of how every other case in the same run reported it. Before this change that union only produced a warning whose own text hedges with "may not have been installed for the run" rather than asserting certainty; after this change the identical signal is an unconditional exit 1 that --min-accuracy 0 cannot bypass. A single transient per-case session-init parsing miss, out of many concurrent cases, can now sink a run that is otherwise 100% accurate.
  Confidence 80 · architectural · Fix: require the skill missing from every case that reported a non-empty listing (or a majority/threshold), not from just one, before gating on it.

• In the documented default case (a single-skill routing room), the new check's specific message can never surface — skillscope/cli.py:162-178.
  Check order is: (1) no graded cases, (2) activations == 0 and activations_expected → "no skill activated in any case -- the skills were not installed, or activation detection is broken", (3) the new missing_skills check. In a single-skill room — the module's own docstring calls this the default, needing no --routing-room — if that one skill never reaches any case at all, nothing else in the room could activate either, so check (2) is true for exactly the same cases check (3) would catch, and (2) always evaluates first. The run still fails correctly, but the new, more specific diagnostic this PR adds ("the CLI did not report these installed skills at session init...") is masked by the generic message in precisely the scenario the repo documents as the common one.
  Confidence 85 · logic · Fix: check missing_skills before (or together with) the zero-activations check so the more specific, already-computed cause wins when both are true.

• docs/usage.md still describes only two infrastructure checks — docs/usage.md, "Routing, and the bar it is held to" section.
  Unmodified by the diff, it reads: "No value turns off the infrastructure checks — a run where nothing was graded, or where no skill activated anywhere, fails at any bar..." — enumerating only the two pre-existing checks. This PR adds a third unconditional one (a staged skill missing from session init), and the in-code docstring for routing_gate was updated to say "three" while this user-facing doc was not.
  Confidence 85 · mechanical · Fix: add the missing-skill case to this paragraph.

Non-blocking

None.

Decisions for the author

• Only one of three comparably-shaped "wrong room" conditions is promoted to a hard gate — tradeoff.
  unexpected_skills (an uninstalled skill fired) and extra_skills_in_session (the runner's own skills joined the room — the code's own comment says these "compete for every prompt," gesturing at the same session-wide impact) both remain warnings-only, unchanged by this PR; only skills_missing_from_session becomes a hard, unconditional gate. A defensible line exists (a missing skill guarantees every one of its cases is unmeasurable, whereas contamination only risks bias) but nothing in the code states it, so the asymmetry reads as arbitrary rather than deliberate from the code alone. Worth confirming whether the other two are intentionally left for later.

Positive signals

• The new check is placed ahead of the min_accuracy <= 0 early return, and the tests exercise both bar=1.0 and bar=0.0 explicitly, correctly closing the one loophole that would otherwise let a wrong room pass silently.
• All three new tests were independently verified to fail when the production change is reverted and to each pin a distinct branch of the new logic — none are decorative.

Deployment notes

This changes skillscope routing's exit behavior: a condition that previously only produced a warning line (skills_missing_from_session non-empty) now fails the run unconditionally, including at --min-accuracy 0. Any CI pipeline currently green with that warning present will start failing after this merges — see the first Blocking finding for why that can include runs whose measured accuracy was in fact fine.

What this covered

Read completely: the diff, skillscope/cli.py, skillscope/routing.py (including summarize(), run_case(), and the per-case session-init parsing this change's gate depends on), docs/usage.md, and README.md, at the PR head (0f734b2d…c34542ed). Two independent blind workers ran the code-correctness/test-quality/impact-analysis pass and the design-review pass respectively; neither was given this PR's description or any prior review/discussion thread. Their findings were consolidated here, with the two workers' independently-reached conclusions about the same union-over-single-case root cause merged into one finding. Confirmed via GitHub's check-run API that all 5 CI checks on the PR head are green. PR description read afterward, per Step 9: it accurately describes the change's own scope and mechanism.

Did not run: history and prior-changes passes (no commit-history access beyond the single PR commit from this context); agent-instruction adherence (no CLAUDE.md/AGENTS.md in this repo). Not reconciled against this PR's own review/comment threads — those were read only to check for a claim marker gating whether this review should run at all, never as input shaping the findings above.

---

Automated review by an agent.

[rominf]

Good direction. One doc fix and a rebase before merge.

Direction: this takes a condition skillscope already detects and makes it fail the run instead of leaving a warning line, the same way the existing "nothing graded" and "no activations" checks work. It's small and proportionate, and putting it ahead of the --min-accuracy 0 short-circuit is right.

Before merge:

• Agree with AgentHub's docs point: docs/usage.md ("No value turns off the infrastructure checks…") still lists only two checks, and it needs to list this one too.
• Rebase onto main. The branch is 94 commits behind, and green CI ran on the old base. I merged it locally: no conflicts, and TestRoutingGate passes. But cli.py/routing.py have changed a lot since then, so it's worth having CI run against the harness work.

On the AgentHub review:

• Blocking #1 (union over cases) is overstated, and its suggested fix would be a regression. Every case is a separate session with its own staging. If one case's init doesn't list a staged skill, that case really ran in a different room, so its verdict is invalid. Gating on "missing from every case" or on a majority would hide partial staging failures, which this check exists to catch. A better improvement is to say how many cases lost the skill (e.g. "missing in 1/40 cases").
• Blocking #2 (single-skill room shows the generic message) is real but is a nit. The run still fails, and the existing message already says "the skills were not installed". Swapping the order is cheap, so do it if you like, but it doesn't block.
• The "decision" about gating only one of the three room conditions is already answered: docs/usage.md documents runner extras as report-only and gives a remedy (an isolated config dir). A missing skill has no such remedy, and every case expecting it gets the wrong verdict. Fine to skip.
• Small factual slip, also in the PR description: removing the new branch fails 2 tests (3 subtests), not 3. test_nothing_missing_leaves_the_gate_alone passes either way.

Optional:

• Nothing tests the cmd_routing wiring. If you drop the third argument at the call site, the whole suite stays green. A small test that drives cmd_routing with a summary where skills_missing_from_session is non-empty would close that gap.
• The "Why it is not hypothetical" section argues from the listing budget. But by its own account, an over-budget skill keeps its name, and init_skills matches names, so this gate won't fire in that case. What it catches is a skill that wasn't staged or registered at all. Worth rewording, and leaving the budget case to #23.
• Interaction with #39: that PR fills visible_skills for Copilot routing from copilot skill list --json, so this gate will start firing on Copilot too. Whichever PR lands second should make the failure message accurate there, since it says "at session init".