Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a meaningful fixed prompt-cost source. The full-index premise still exists on current main (agent/prompt_builder.py:1660-1667), but the proposed global category-only strategy conflicts with a later verified design decision.
Problems
- Current main intentionally keeps every skill name visible:
agent/prompt_builder.py:1622-1628and commitee1a744acdocument that models did not reliably useskills_listto rediscover omitted agent-created skills. The PR'sskills_list(category=...)replacement atagent/prompt_builder.py:1177-1179reintroduces that failure mode. - Renaming the block at
agent/prompt_builder.py:1199-1201leavesagent/context_breakdown.py:15andhermes_cli/prompt_size.py:21unable to find and attribute the skills index.
Suggested changes
- Preserve name visibility and reduce description noise only, following the current names-only demotion contract and tests at
tests/agent/test_prompt_builder.py:429-475. - Retain the existing wrapper tag or update all block consumers and their tests.
This is an automated hermes-sweeper review.
| "Before replying, scan the skills below. If a skill matches or is even partially relevant " | ||
| "to your task, you MUST load it with skill_view(name) and follow its instructions. " | ||
| "Before replying, scan the skill categories below. If a category matches or is even partially relevant " | ||
| "to your task, you MUST call skills_list(category=...) to inspect the available skills, then load " |
There was a problem hiding this comment.
Current main intentionally does not rely on skills_list to rediscover omitted skills: ee1a744ac records a real failure where an agent-created skill disappeared from the index and was not rediscovered. Preserve skill names rather than making this category-only.
| "pitfalls you discovered, update it before finishing.\n" | ||
| "\n" | ||
| "<available_skills>\n" | ||
| "<available_skill_categories>\n" |
There was a problem hiding this comment.
This renamed wrapper is not consumed by the current prompt diagnostics: agent/context_breakdown.py:15 and hermes_cli/prompt_size.py:21 only match <available_skills>. Update those consumers and tests, or retain the existing tag.
|
Closing on the design axis after checking the current skills-index implementation: main deliberately keeps every skill visible in the index at all times — the never-hide decision (ee1a744) demotes long categories to names-only under pressure but never removes entries, precisely because models do NOT reliably reach for skills_list to rediscover what the index stops showing them. This PR's categories-only index (with a 'call skills_list(category=...) to inspect' nudge) is the exact pattern that decision rejected: in practice the model skips the extra round-trip and misses skills. The category-count suffix rider is neutral-to-negative once full entries stay (extra tokens, no signal). Thank you — the token concern is real, and the compact-guidance angle (#72813) is the shape of it we can take. |
Summary
skills_list(category=...)before loading matching skills withskill_view(...).Test Plan
python -m pytest tests/agent/test_prompt_builder.py -q -o 'addopts='Closes #26786