Skip to content

fix(curator): protect external skills from background curation - #51875

Closed
yu-xin-c wants to merge 1 commit into
NousResearch:mainfrom
yu-xin-c:codex/skill-curator-external-readonly
Closed

yu-xin-c wants to merge 1 commit into
NousResearch:mainfrom
yu-xin-c:codex/skill-curator-external-readonly

Conversation

@yu-xin-c

Copy link
Copy Markdown
Contributor

Summary

  • add a shared external-dir ownership helper for skills.external_dirs
  • exclude external skills from curator provenance, reports, and archive paths
  • reject background-review skill_manage writes to external, hub-installed, bundled, and protected skills while preserving foreground external edits
  • update the curator prompt to treat external skills as read-only

Root cause

The curator path had provenance checks for bundled and hub-installed skills, but no shared external-dir ownership boundary. A background review could still target an externally owned skill through skill_manage or stale sidecar state.

Validation

  • scripts/run_tests.sh tests/agent/test_skill_utils.py tests/tools/test_skill_usage.py tests/tools/test_skill_manager_tool.py -- -q
  • scripts/run_tests.sh tests/agent/test_external_skills.py -- -q
  • .venv/bin/ruff check agent/skill_utils.py tools/skill_usage.py tools/skill_manager_tool.py agent/curator.py tests/agent/test_skill_utils.py tests/tools/test_skill_usage.py tests/tools/test_skill_manager_tool.py

Fixes #47688

@yu-xin-c
yu-xin-c force-pushed the codex/skill-curator-external-readonly branch from 3d4aba0 to 3b00dbd Compare June 24, 2026 12:38
@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) P1 High — major feature broken, no workaround labels Jun 24, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: competing fix for #47688 (curator mutates/deletes skills.external_dirs). This PR uses a shared external-dir ownership boundary + background-review write rejection + read-only curator prompt; the earlier open #47752 uses a curation-eligibility gate + prompt rule. Same P1 silent-data-loss target, different mechanism — maintainer should pick one.

@yu-xin-c
yu-xin-c marked this pull request as ready for review June 24, 2026 13:08
@yu-xin-c

Copy link
Copy Markdown
Contributor Author

Thanks for the triage note. A quick distinction from #47752: this PR keeps foreground/user-directed external skill edits intact and makes the external-dir boundary specific to autonomous curation. The shared is_external_skill_path() predicate is used by curator provenance/archive paths and by the background-review skill_manage guard, so stale sidecar records and direct curator tool calls both fail closed without reintroducing the older global read-only behavior for skills.external_dirs.

The added regression coverage exercises that split explicitly: external skills with stale created_by=agent usage records are excluded from curator reports/archive paths, and background-review patch/delete attempts are refused while existing foreground external-skill mutation tests continue to pass.

teknium1 added a commit that referenced this pull request Jun 26, 2026
…ration

The salvaged #51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(GH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #52849 (#52849). Your commit was cherry-picked onto current main with your authorship preserved in git log (commit 96bc524).

We added one follow-up fix on top: your background-review write guard in skill_manage was correct, but it only fires when is_background_review() is true — and the curator's LLM review fork ran under the default assistant_tool write origin, so the guard never triggered during the curation pass it's meant to protect against. We set _memory_write_origin="background_review" on the curator fork (so turn_context binds the origin and the guard fires), plus a regression test. Thanks for the solid foundation.

@yu-xin-c

Copy link
Copy Markdown
Contributor Author

Thanks for merging this via #52849 and for preserving authorship. The curator fork origin follow-up makes sense: the background-review guard relies on that execution context being bound correctly, so setting _memory_write_origin="background_review" on the curator fork closes the gap cleanly.

pai-scaffolde pushed a commit to Scaffolde/hermes-agent that referenced this pull request Jun 28, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
…ration

The salvaged NousResearch#51875 added a background-review write guard in skill_manage
that refuses mutations to skills.external_dirs skills — but it only fires
when is_background_review() is true. The curator's LLM review fork ran with
the default _memory_write_origin='assistant_tool', so the guard never
triggered during the exact curation pass it exists to protect against
(NousResearchGH-47688).

- Set _memory_write_origin='background_review' on the curator review fork so
  turn_context binds it onto the write-origin ContextVar and the guard fires.
- Add a regression test asserting the fork runs under the background_review
  origin (the invariant linking the fork to the guard).
- AUTHOR_MAP: map yu-xin-c for the salvaged commit.
QAbsolut added a commit to QAbsolut/hermes-agent that referenced this pull request Aug 27, 2026
Electron 40+ supports the XDG Desktop Portal GlobalShortcuts protocol
but ships it behind a disabled feature flag, so register() silently
returns false on Wayland without this (Electron upstream NousResearch#51875).
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 P1 High — major feature broken, no workaround 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]: Curator's LLM consolidation agent mutates/deletes skills in external_dirs; per-profile pins can't protect shared skills

3 participants