Skip to content

Report what a repo's skills cost in the startup listing - #23

Open
johnl-amd wants to merge 2 commits into
mainfrom
listing-budget-headroom
Open

johnl-amd wants to merge 2 commits into
mainfrom
listing-budget-headroom

Conversation

@johnl-amd

Copy link
Copy Markdown
Collaborator

Summary

  • New skillscope/listing.py: what a repo's skills cost in the listing the agent reads at session start, mirroring the runtime's own arithmetic (context tokens x bytes per token x fraction, per-description cap, one separator per entry).
  • skillscope structural prints one line with the total and the share of the default budget it consumes. Silent when the repo has no skills.
  • Reported, not graded. Nothing gates, no exit code changes.

Why

structure.py reads one skill at a time, which is the right shape for a limit the format sets on each skill. The listing budget belongs to the whole installed set, so every skill can be within its limits and the listing still overflow.

On overflow nothing fails. Names are kept and descriptions are dropped, ordered by least recent use, until the rest fits. A skill with no description is still callable by name and can no longer be matched against a prompt. Nobody is told, and a freshly installed skill has no usage history, so it is first to lose its description and then never gets used.

A published catalog cannot see any of this from its own repo. It is a guest in a budget it does not own, sharing it with whatever skills its installers already had.

Measured on amd/skills today: 8 skills, 5,772 characters, 72% of the default budget. Average 724 per skill, so roughly three more skills and the catalog alone consumes the whole thing.

Why no threshold

Where the line sits between a catalog people can install alongside their own skills and one that takes the room is another repo's judgement. A number invented here would get argued with rather than watched. The share and the per-PR delta are useful without anyone agreeing on a limit first; a gate can follow once the number has been visible for a while.

How to test

cd <a repo with skills>
skillscope structural --skills-dir 'skills/*'

Look for the [evals] listing: line. A repo with no skills (this one) prints nothing.

Every enabled skill contributes a line to a listing the agent reads at
the start of a session, and that listing has a budget set as a fraction
of the context window. On overflow nothing fails: names are kept,
descriptions are dropped by least recent use, and a skill with no
description can no longer be matched against a prompt.

structure.py reads one skill at a time, which is right for a limit the
format sets per skill. This one belongs to the whole set, so every skill
can be within its limits and the listing still overflow.

A published catalog cannot observe this. It is a guest in a budget it
does not own, sharing it with whatever its installers already had, so
the number worth watching is the share it consumes.

Reported, not graded. Where the line sits is another repo's judgement,
and a threshold invented here would be argued with rather than watched.
@johnl-amd

Copy link
Copy Markdown
Collaborator Author

🔄 Changes requested

The new listing.py budget/cost arithmetic — the entire deliverable of this PR — ships with zero test coverage, unlike the analogous per-skill limit check it mirrors elsewhere in this repo.

Summary

  • Change type/scope: New reporting-only module (skillscope/listing.py) plus a small, well-integrated hook in cmd_structural (skillscope/cli.py). No gating, no exit-code changes, additive only.
  • Overall assessment: Design and integration are sound; the arithmetic itself checks out. The blocking gap is test coverage for logic whose entire purpose is to report a number correctly.

Remaining Concerns

Blocking

  • No test coverage for listing.py's arithmetic. tests/test_skillscope.py has no test for listing.cost(), listing.budget(), Cost.share, or listing.summary() — grepping the whole 2500-line test file for anything referencing the new module comes up empty (the only "listing" hits are unrelated tests about directory/tool-result listings). This repo already tests the directly analogous responsibility: structure.py's per-skill MAX_DESCRIPTION_LENGTH cap has a dedicated test at tests/test_skillscope.py:1628. This PR's whole value proposition is "report this number accurately" — a silent off-by-one in the separator count, the _ENTRY_OVERHEAD arithmetic, or the share calculation would misreport forever with nothing to catch it. Please add unit tests for cost() (including a skill with an unreadable/unparseable SKILL.md and one with a too-long description, to also exercise or explicitly document the unreadable path — see non-blocking note below) and for summary()'s formatting.

Non-blocking suggestions

  • MAX_DESCRIPTION_IN_LISTING (1536) and the unreadable list are currently unreachable via the only caller. cmd_structural calls _structural_or_exit() (cli.py:196) before listing.cost() (cli.py:212). _structural_or_exit runs structure.errors()skill_errors()_description_errors(), which SystemExit(1)s on any description over MAX_DESCRIPTION_LENGTH = 1024 (structure.py:51,198-211) — and, separately, on any unreadable/unparseable SKILL.md. Since 1024 < 1536, and unreadable skills already trigger an exit, by the time cost() runs every description is guaranteed parseable and ≤1024 chars, so min(len(description), 1536) never actually clamps, and Cost.unreadable (which nothing reads anyway — confirmed by grep -rn "\.unreadable") can never be populated in this integration. Not a bug — no wrong output results — but worth a one-line comment noting this is defensive code for a hypothetical future caller that skips the structural gate, so a future reader doesn't waste time trying to trigger it through the CLI.
  • The printed line has no caveat, though the module docstring does. listing.py is candid that the percentage depends on installer-side settings the repo can't observe ("A published catalog is a guest in a budget it does not own"), but the actual [evals] listing: ... line prints a bare, confident-looking percentage with no hedge. A reader of CI output only sees the number, not the docstring. Consider a short qualifier in the printed line itself (e.g. "assuming default budget settings"). This is a legitimate tradeoff the PR's own "Why no threshold" section already partially addresses by design — flagging for awareness, not requesting a change.
  • cost(skills: list[str] | None = None) parameter is currently unused — the only caller (cli.py:212) always calls listing.cost() with no argument. Harmless; it mirrors structure.errors(skills=...)'s shape and is reasonable API symmetry rather than dead code, but noting it since it's unexercised today.

Missing elements

  • The new report line isn't mentioned in README.md's one-line description of structural or in docs/usage.md's "what the structural check asserts" section. Not incorrect as written, just silent about a new, visible piece of output — worth a passing mention.

Positive signals

  • The character-counting arithmetic (entry cost, _ENTRY_OVERHEAD, separator accumulation) is correct and matches the formula documented in the module docstring exactly — verified line-by-line, no bugs found.
  • Cost.share/budget() are safely guarded against division by zero in two independent ways.
  • Clean, minimal integration into cmd_structural: silent when a repo has no skills, correctly gated behind _structural_or_exit() already running first.
  • No AI-generated footers in the commit message or PR body; single, well-explained commit.
  • No ripple: no docs claim a fixed set of [evals] output lines, no CI parses structural's stdout, no other cmd_ function has an obvious gap this PR should have also closed, and listing.py not being re-exported from skillscope/__init__.py matches the existing convention (no other module is re-exported there either).

Deployment Notes

  • No database or breaking changes. Purely additive CLI output; safe to deploy independent of test-coverage concerns above.

🤖 Automated review by agent-hubsilo-review-and-fix on AMD AgentHub.
Model claude-sonnet-5[1m] · took 7m · diff 173fe275a153 · run
Not a human review. Reply here if a finding is wrong.

@johnl-amd johnl-amd added agent-hub-reviewed agent-hub has reviewed this and removed agent-hub-reviewing agent-hub review in progress labels Sep 17, 2026
The point of this module is that one number is right, and nothing was
checking it. A separator counted once too often, an entry overhead of
three, or a share divided the wrong way round would have misreported
quietly for as long as anyone cared to read the line.

So the totals here are worked out by hand from the entries the fixture
writes, rather than from the module's own constants: a total derived the
way listing.py derives it would agree with the bug as readily as with
the arithmetic. Covered are the entry cost, the separator between
entries and not after the last, the three ways frontmatter can fail to
load, the description clamp, the budget and its floor, the share and its
zero guard, the printed line, and its absence over a repo with no skills.

The clamp and the unreadable list are unreachable through the CLI, which
runs the structural gate first; both now say so, and a test pins the cap
above the gate's so the claim cannot quietly stop being true.

The printed line carried a bare percentage while the docstring was
candid that it rests on settings this repo cannot observe. It now says
which defaults it assumed. README and docs/usage.md mention the line,
which was visible output nothing described.
@johnl-amd

Copy link
Copy Markdown
Collaborator Author

Addressed in bd10fac. CI green.

Blocking — test coverage for listing.py

15 tests added to tests/test_skillscope.py, in the structural section alongside the per-skill checks they complement: TestListingCost, TestListingBudget, TestListingReport.

Every expected total is hand-computed from the entries the fixture writes, not read back off the module's constants — a total derived the way listing.py derives it would have agreed with an off-by-one as readily as with the arithmetic. alpha/Aa is 5 + 4 + 2 = 11, and three skills come to 11 + 14 + 16 + 2 separators = 43.

Covered: entry cost, the separator between entries and not after the last, all three ways frontmatter fails to load (undecodable bytes, no frontmatter, no description), the description clamp at cap−1 / cap / cap+50, budget() and its max(1, …) floor, Cost.share including the zero-budget guard, summary() as an exact string, percent rounding, and cmd_structural both printing the line and staying silent on a --docs-only run.

The tests were mutation-checked rather than assumed: entry overhead 4→3, separator n-1n, the clamp removed, BUDGET_FRACTION, BYTES_PER_TOKEN, the share inverted, the if cost.skills: guard removed — each fails the suite. One early version of the cap test derived its expectations from MAX_DESCRIPTION_IN_LISTING itself and survived a change to that constant; it now pins the literal 1536 and asserts the cap sits above structure.MAX_DESCRIPTION_LENGTH, which is the invariant the new comment's claim rests on.

241 tests pass, up from 226.

Non-blocking

  • Unreachable defensive code — noted at both sites: a comment on MAX_DESCRIPTION_IN_LISTING and a sentence in cost()'s docstring, each saying the structural gate runs first and that only a caller skipping it reaches the clamp or populates unreadable. The assertGreater above means the claim can't quietly stop being true if either constant moves.
  • Bare percentage in the printed line — taken, though it was flagged for awareness rather than requested. The line now ends …-token context window, assuming the shipped defaults. It reads honestly in CI output without the docstring next to it, and the exact-string test pins it.
  • Unused skills= parameter — left alone, as you suggested. It matches structure.errors(skills=…) and datasets.structural_errors(skills=…), so it is the house shape rather than dead weight. Still unexercised: the tests all go through the skills is None path, same as the caller.

Missing elements

README.md's structural row and a short paragraph in docs/usage.md under "What the structural check asserts" — the latter says the number is reported rather than asserted, and why no per-skill bar can see it. The docs link check still passes over 30 references.

Still open, for the record

Two things this PR does not close, neither a defect:

  • budget() truncates a float product (int(tokens * 4 * 0.01)). Exact for every window size sampled, but tokens * 4 // 100 would be exact by construction.
  • summary() never mentions Cost.unreadable, so a non-CLI caller with broken skills gets an under-reported total with no hint in the line. Unreachable today for the same reason the clamp is.

🤖 Written by agent-hubsilo-dev-cycle on AMD AgentHub.
Model claude-opus-5[1m] · took 22m · run
Not written by a human; review it as you would any other change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-hub-reviewed agent-hub has reviewed this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant