Skip to content

feat(skills): add targeted skill_view retrieval - #49503

Open
timbeaulac wants to merge 2 commits into
NousResearch:mainfrom
timbeaulac:feat/187-targeted-skill-view
Open

feat(skills): add targeted skill_view retrieval#49503
timbeaulac wants to merge 2 commits into
NousResearch:mainfrom
timbeaulac:feat/187-targeted-skill-view

Conversation

@timbeaulac

@timbeaulac timbeaulac commented Jun 20, 2026

Copy link
Copy Markdown

What does this PR do?

Adds a production targeted retrieval path for skill_view so mandatory skill loading can stay safe without flooding context on large skills.

The implementation lives in the Hermes core skill tool path and supports:

  • compact mode="brief" responses with manifest plus safety-critical sections;
  • deterministic section_slug / section_title retrieval;
  • explicit full=true / mode="full" escape hatch;
  • existing linked-file retrieval behavior;
  • savings metadata and repeat-load metadata;
  • plugin-provided skill support;
  • rollback-safe rollout via config/env flag.

The intended retrieval flow is progressive: request mode="brief" first, inspect the returned section manifest, then retrieve a specific section with section_slug or section_title only when the task needs it. This avoids loading an entire large skill solely to access one subsection, while preserving full=true / mode="full" for cases where the complete document is required.

Related Issue

Refs revise-repeat/siva-os#187

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/skills_tool.py
    • Adds mode, full, section_title, and section_slug support to skill_view.
    • Adds Markdown section indexing with stable unique slugs.
    • Preserves safety-critical sections in brief mode: prerequisites, mandatory rules, boundaries, pitfalls, warnings, safety/security, testing, and verification.
    • Adds omitted-section manifests and targeted retrieval savings metadata.
    • Wires targeted retrieval through plugin skill serving.
  • hermes_cli/config.py
    • Adds disabled-by-default skills.targeted_view rollout config with enabled and threshold_chars.
  • cli-config.yaml.example
    • Documents the targeted view rollout config and rollback behavior.
  • tests/tools/test_skills_tool.py
    • Covers brief mode, full escape hatch, section retrieval, safety preservation, duplicate slug collision behavior, and env flag auto-briefing.
  • tests/test_plugin_skills.py
    • Covers targeted section retrieval for plugin-provided skills.

How to Test

  1. Run the scoped test suite:
    /Users/timbeaulac/.hermes/hermes-agent/venv/bin/python -m pytest tests/tools/test_skills_tool.py tests/tools/test_skill_size_limits.py tests/tools/test_skill_view_traversal.py tests/test_plugin_skills.py -q -o 'addopts='
  2. Validate the config example:
    /Users/timbeaulac/.hermes/hermes-agent/venv/bin/python - <<'PY'
    import yaml
    from pathlib import Path
    cfg = yaml.safe_load(Path('cli-config.yaml.example').read_text())
    assert cfg['skills']['targeted_view']['enabled'] is False
    assert cfg['skills']['targeted_view']['threshold_chars'] == 20000
    print('cli-config targeted_view example OK')
    PY
  3. Smoke targeted retrieval against live local skills:
    PYTHONPATH=. HERMES_HOME=/Users/timbeaulac/.hermes/profiles/builder python3 - <<'PY'
    import json
    from tools.skills_tool import skill_view
    for name in ['hermes-agent', 'github-issues']:
        brief = json.loads(skill_view(name, mode='brief'))
        print(name, brief['targeted_retrieval'])
    PY

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
    • Scoped verification passed; a full-suite attempt showed broad unrelated/environment failures by ~15% and was stopped rather than conflating them with this PR.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.5.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
    • N/A for public docs; tool schema/config comments were updated.
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
    • N/A; this is tool behavior/config, not contributor workflow.
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
    • Uses Python stdlib/string processing only; no platform-specific path or process behavior added.
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A — this PR does not add a skill.

Screenshots / Logs

Scoped verification:

cli-config targeted_view example OK
148 passed, 1 warning in 3.09s

Live smoke savings:

hermes-agent: saved 28,583 chars (62.1%)
github-issues: saved 9,528 chars (71.4%)
github-projects-v2: saved 29,349 chars (69.0%)

Historical state DB analyzer from the linked SIVA OS worktree:

builder profile: estimated 86.9% savings
default profile: estimated 88.7% savings

Rollout/rollback:

  • Auto-briefing is disabled by default.
  • Enable with skills.targeted_view.enabled=true or HERMES_SKILL_VIEW_TARGETED=1.
  • Roll back by disabling config/unsetting env.
  • full=true or mode="full" always returns complete content.

Refs revise-repeat/siva-os#187

Co-authored-by: siva-splicer[bot] <292605095+siva-splicer[bot]@users.noreply.github.com>
Generated-By: hermes/2.0 (role: Splicer; model: gpt-5.5)
Agent-Session: 20260620_030045_2e35e647
Task-Reference: NousResearch#187
@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jun 20, 2026
Refs revise-repeat/siva-os#187

Co-authored-by: siva-splicer[bot] <292605095+siva-splicer[bot]@users.noreply.github.com>
Generated-By: hermes/2.0 (role: Splicer; model: gpt-5.5)
Agent-Session: 20260620_030045_2e35e647
Task-Reference: NousResearch#187

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough implementation and targeted tests.

Problems

  • tools/skills_tool.py:1030-1065 deliberately omits SKILL.md sections and asks the model to retrieve them later. This conflicts with the current repository rule in AGENTS.md:111-113: instructional tools, including skills, must be read fully because models otherwise stop after the first page.
  • tools/skills_tool.py:1098-1102 adds the user-facing non-secret HERMES_SKILL_VIEW_TARGETED behavior flag. AGENTS.md:102-107 requires behavioral settings to be configured through config.yaml, not new HERMES_* environment variables.
  • _TARGETED_SKILL_REPEAT_CACHE at tools/skills_tool.py:934 is populated at :1766-1769 without eviction, so a long-lived process retains an entry for every distinct task/skill/mode combination.

Suggested changes

  • Re-scope away from partial SKILL.md retrieval unless maintainers explicitly revise the full-read contract.
  • Remove the environment-variable override and avoid unbounded process-global repeat-load state.

Automated hermes-sweeper review.

Comment thread tools/skills_tool.py
for idx, section in enumerate(sections):
if section["start"] < covered_end:
continue
include = idx == 0 or _section_is_safety_critical(section)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lazy-reading path for an instructional document: it includes only the first/safety-matched sections and omits the rest. AGENTS.md:111-113 explicitly says skills must be read fully because models will otherwise read page 1 and skip the remainder. Please do not introduce this partial SKILL.md retrieval behavior without a maintainer-approved change to that contract.

Comment thread tools/skills_tool.py
threshold = int(targeted_cfg.get("threshold_chars", threshold))
except Exception:
pass
env_enabled = os.getenv("HERMES_SKILL_VIEW_TARGETED", "").strip().lower()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HERMES_SKILL_VIEW_TARGETED is a new user-facing non-secret behavioral flag. AGENTS.md:102-107 requires feature flags and other behavior settings to live in config.yaml; please remove this environment-variable interface.

Comment thread tools/skills_tool.py
)
_TARGETED_SKILL_DEFAULT_THRESHOLD = 20_000
_TARGETED_SKILL_SECTION_LIMIT = 3_500
_TARGETED_SKILL_REPEAT_CACHE: set[tuple[str, str, str]] = set()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This process-global set has no eviction path and is populated for every distinct task/skill/mode combination. Long-lived gateway processes will retain it indefinitely; use bounded or session-owned state if repeat-load metadata is retained.

@barteqpl

Copy link
Copy Markdown

Tested against a real production install (583 sessions / 30 days, 1,215 skill loads): my largest SKILL.md had grown to 119 KB because the changelog frontmatter accumulates one entry per pitfall patch. Trimmed it locally to 5 entries + a references/changelog-archive.md (-24% per load).

This PR's brief/section retrieval is exactly the right fix at the tool layer — progressive loading would have made that kind of bloat harmless. Would love to see this merged.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/skills Skills system (list, view, manage) type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants