Skip to content

fix(skills): prevent infinite loop on cyclic symlinks in iter_skill_index_files - #18815

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/cyclic-symlink-skill-scan
Open

fix(skills): prevent infinite loop on cyclic symlinks in iter_skill_index_files#18815
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/cyclic-symlink-skill-scan

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

iter_skill_index_files() uses os.walk(followlinks=True) but has no cycle detection. When the skill directory tree contains cyclic symlinks (e.g. A → B → A or self-referencing directories), os.walk enters an infinite loop, causing the agent to hang during skill scanning.

Related Issue

Fixes #18809

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

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
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • 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 and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

…ndex_files

os.walk(followlinks=True) can loop forever when the skill directory
tree contains cyclic symlinks (e.g. A→B→A or self-referencing dirs).

Track visited real paths via os.path.realpath() and skip directories
whose canonical path has already been processed.  Non-cyclic symlinks
are still followed as before.

Fixes NousResearch#18809
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) labels May 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #18809 — same cyclic symlink bug in iter_skill_index_files(). Also related to #7634 which proposes the same cycle guard fix.

1 similar comment
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #18809 — same cyclic symlink bug in iter_skill_index_files(). Also related to #7634 which proposes the same cycle guard fix.

- Add sunsky.lau@gmail.com to AUTHOR_MAP for check-attribution CI
- Use resolve() in test_cyclic_symlink_does_not_loop since os.walk
  traversal order varies by platform (macOS alphabetical vs Linux hash)
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real regression introduced when symlink following was enabled in 02aba4a728e26b4756d2932bafd890d663f6328a.

Problems

  • The guard only covers iter_skill_index_files, but current main also walks skill directories with followlinks=True in agent/prompt_builder.py:1281. build_skills_system_prompt() reaches that unguarded manifest traversal through snapshot validation at agent/prompt_builder.py:1314, and through cold-path snapshot creation at agent/prompt_builder.py:1566. A cyclic skill tree can therefore still block prompt startup.
  • The PR tests cover only iter_skill_index_files; add coverage for the prompt snapshot path. The added tests also call symlink_to() without the unavailable-symlink handling used by tests/agent/test_skill_commands.py:47-49 and tests/agent/test_skill_utils.py:376-378.

Suggested changes

  • Use one cycle-safe traversal strategy for both agent/skill_utils.py:796 and agent/prompt_builder.py:1281, preserving intended symlink discovery.
  • Add cold and snapshot-validation regression coverage through build_skills_system_prompt().
  • Skip the symlink tests when the platform/filesystem cannot create them.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: os.walk(followlinks=True) in iter_skill_index_files may infinite-loop on cyclic symlinks

3 participants