Skip to content

fix(agent): track visited realpaths in iter_skill_index_files to prevent symlink loops - #69248

Open
CarlitoDon wants to merge 1 commit into
NousResearch:mainfrom
CarlitoDon:fix/prevent-symlink-loop-in-skill-scanning
Open

fix(agent): track visited realpaths in iter_skill_index_files to prevent symlink loops#69248
CarlitoDon wants to merge 1 commit into
NousResearch:mainfrom
CarlitoDon:fix/prevent-symlink-loop-in-skill-scanning

Conversation

@CarlitoDon

Copy link
Copy Markdown

Summary

Hardens skill-directory scanning during system-prompt construction by preventing os.walk(..., followlinks=True) from recursing indefinitely when encountering circular/self-referential symlinks.

Changes

  • Track visited directory realpaths during iter_skill_index_files() traversal and prune recursion when a realpath repeats.
  • Add regression unit test in tests/agent/test_skill_utils.py.

Copilot AI review requested due to automatic review settings July 22, 2026 10:48

Copilot AI 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.

Pull request overview

This PR hardens skill-directory scanning used during system-prompt construction by preventing os.walk(..., followlinks=True) from recursing indefinitely when encountering circular/self-referential symlinks.

Changes:

  • Add realpath-based loop detection in iter_skill_index_files() to stop revisiting the same directory via symlinks.
  • Add a regression test that creates a self-referential symlink and asserts traversal terminates and returns the expected SKILL.md.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
agent/skill_utils.py Tracks visited directory realpaths during skill index traversal to prune recursion on repeats.
tests/agent/test_skill_utils.py Adds a unit test covering circular symlink traversal behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread agent/skill_utils.py
visited_dirs.add(real_root)

has_skill_md = "SKILL.md" in files
dirs[:] = [
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #18815: the current live diff implements the same iter_skill_index_files() realpath visited-set guard and self-symlink regression coverage.

@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 addressing the real symlink-loop risk. The iter_skill_index_files() guard is sound for that helper, but the current prompt-build path needs the same protection.

Problems

  • agent/prompt_builder.py:1350 independently calls os.walk(..., followlinks=True) without a visited-realpath guard. build_skills_system_prompt() reaches it during snapshot validation through _load_skills_snapshot() (agent/prompt_builder.py:1566, agent/prompt_builder.py:1383) and during cold snapshot creation (agent/prompt_builder.py:1633-1636), so a cyclic skill tree can still block prompt construction.
  • The added test covers only iter_skill_index_files(); it does not cover either prompt snapshot path.

Suggested changes

  • Apply a shared cycle-safe traversal strategy to both walkers while retaining non-cyclic symlink discovery.
  • Add build_skills_system_prompt() regressions for cold snapshot creation and existing-snapshot validation.

Automated hermes-sweeper review.

Comment thread agent/skill_utils.py
dirs[:] = []
continue
visited_dirs.add(real_root)

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 guard fixes the shared iterator, but the same cyclic tree still reaches the unguarded manifest walk in agent/prompt_builder.py:1350 during snapshot validation and cold snapshot writes. Please apply the same cycle-safe traversal strategy there and add prompt-build coverage.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Five PRs address the shared symlink-cycle cause reported in #18809 and #47659. Each diff adds realpath-based revisit detection to iter_skill_index_files(), but none changes the independently reachable os.walk(..., followlinks=True) in agent/prompt_builder.py, so prompt snapshot construction remains vulnerable according to the contributor reviews.

Related pull requests

Duplicates

#20658, #47775, #69244, and #69248 duplicate #18815's realpath-based cycle guard for iter_skill_index_files(); #47775 is already closed as a self-duplicate of #18815, and #69244 is also already closed. Issues #47659 and #18809 report the same underlying failure.

Suggested consolidation

Keep #18815 open with a salvage path as the recorded best existing fix: retain its iterator guard and tests, split the unrelated scripts/release.py change, apply one cycle-safe traversal strategy to the prompt snapshot walker, and add cold-construction and snapshot-validation regressions through build_skills_system_prompt(). Close #20658 and #69248 as duplicates of #18815; retain already-closed #47775 and #69244 only as duplicate/reference implementations.

Complex graph

flowchart TD
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I18809(["issue #18809 (open)"])
    I47659(["issue #47659 (open)"])
    subgraph Dup18815 ["PRs duplicating each other"]
        P18815["PR #18815 (open)"]
        P20658["PR #20658 (open)"]
        P47775["PR #47775 (closed)"]
        P69244["PR #69244 (closed)"]
        P69248["PR #69248 (open)"]
    end
    P69248 -->|fixes| I18809
    P69248 -.->|partial| I47659
    class I18809 open
    class I47659 open
    class P18815 open
    class P20658 open
    class P47775 closed
    class P69244 closed
    class P69248 open
    class P18815 best
    class P18815 best
    class P69248 target
    click I18809 "https://github.com/NousResearch/hermes-agent/issues/18809"
    click I47659 "https://github.com/NousResearch/hermes-agent/issues/47659"
    click P18815 "https://github.com/NousResearch/hermes-agent/pull/18815"
    click P20658 "https://github.com/NousResearch/hermes-agent/pull/20658"
    click P47775 "https://github.com/NousResearch/hermes-agent/pull/47775"
    click P69244 "https://github.com/NousResearch/hermes-agent/pull/69244"
    click P69248 "https://github.com/NousResearch/hermes-agent/pull/69248"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 5 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 18 kB of PR diffs, 14 kB of issue/PR text, 8 kB of discussion (15 comments), 15 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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.

5 participants