This repository was archived by the owner on Sep 3, 2026. It is now read-only.
curator: enforce skill lock/pin, drop the review fork's shell, stop self-inflating review telemetry - #3
Merged
Conversation
fabiosiqueira
pushed a commit
that referenced
this pull request
Aug 4, 2026
… (re-review #3) The last_activity_at/description/provenance columns already live in SCHEMA_SQL and the column reconciler; existing DBs heal via the reconciler, but the version stamp must advance so downgrade/upgrade tooling sees the new layout. No version-literal test assertions exist (tests compare against the imported constant).
…ible archive_skill() and the other 4 call sites of is_curation_eligible only checked provenance (external/hub/bundled/protected); a skill's own SKILL.md frontmatter opt-out (pinned/locked, already enforced by skill_manager_tool's _pinned_guard/_locked_guard for edit/delete) was never read on the curation-eligibility path, so the curator's auto- archive could still take a pinned/locked skill. ref NousResearch#77
_skill_view_with_bump bumped telemetry for every caller unconditionally. The curator's background-review fork calls skill_view to inspect candidates while judging them for curation — that inspection was counting as use, self-reinforcing the very signal (last_used_at) the curator's stale timer reads. Guard with the existing is_background_review() provenance check, already used by skill_manager_tool's write guards for the same fork. ref NousResearch#77
Upstream pins the review fork to enabled_toolsets=["skills", "terminal"] so it can `mv` support files into an umbrella's references/ while consolidating. The prompt's "DO NOT call terminal to mv skill directories into .archive/" is advisory only, and this fork is autonomous with no user in the loop: a shell it can reach archives an operator-authored skill past every guard the skill_manage path enforces. Drop `terminal` from the list rather than stacking a contrary disabled_toolsets kwarg on the same call. Consolidation moves are the cost of this carry; we don't use them. ref NousResearch#77
fabiosiqueira
force-pushed
the
fix/curator-frontmatter-lock-77
branch
from
August 4, 2026 18:10
f90fe57 to
3163c20
Compare
fabiosiqueira
changed the title
fabiosiqueira
pushed a commit
that referenced
this pull request
Aug 20, 2026
…age_id) Live-canary finding #3 (Alice, staging): the relay inbound leg is at-least-once. On WS re-handshake the connector replays its durable per-instance buffer; a long multi-tool turn (60-100s) straddling a quiet socket drop got its ORIGINAL inbound replayed after the turn finished, re-running the entire turn — the user saw the final answer posted 2-5x (each a separate execution, hence slightly different texts). Receipts: same msg text at history=0 in back-to-back sessions 121647/121840, no Slack-side retry on the connector (envelope dedupe never fired). Consumer-side idempotency: bounded FIFO seen-set (512) keyed by platform message identity; events without a message_id never dedupe (fail-open — dropping a real message is worse than rerunning one). No wire change; contract v1 untouched. Transplanted-from: victor-fork/feat/relay-slack-live-cards@73ce04ae75 (extracted for the rc.4 relay-fixes train; tests moved to a standalone file with no live-cards dependencies)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Some skills wrap deterministic tooling that was exhaustively tested against its exact prompt text — automatic curation rewriting or archiving them degrades behavior that was tuned deliberately. Three independent gaps let the curator's autonomous review pass reach such a skill anyway.
What
tools/skill_usage.py—is_curation_eligible()never read the skill's own frontmatter. It gated on provenance only (external/hub/bundled/protected), so a skill opting out viametadata.hermes.pinned/metadata.hermes.lockedwas still a valid auto-archive candidate. Now checks the frontmatter, fail-open on a missing/unparseable SKILL.md (matching_locked_guard's existing fallback).agent/curator.py— the review fork could shell out. Its only defense against archiving viaterminal mvwas a prompt instruction ("DO NOT call terminal to mv skill directories into .archive/"); advisory text is not an enforcement boundary. The fork is now pinned toenabled_toolsets=["skills"].Carry divergence: upstream ships
["skills", "terminal"](perf(curator): pin the LLM review fork to the skills+terminal toolsets (salvage #62082) NousResearch/hermes-agent#77667) so the fork canmvsupport files into an umbrella'sreferences/while consolidating. We dropterminaland lose that consolidation move — an autonomous fork with a shell reaches past every guard theskill_managepath enforces, and we don't use umbrella consolidation. The two upstream tests that pin the intended surface are adapted in-place, with the divergence stated in their docstrings.tools/skills_tool.py—skill_viewinflated the signal the review reads. The bump fired for every caller including the review fork, so inspecting a skill to judge whether it was stale counted as use. Now guarded bytools.skill_provenance.is_background_review().Upstream routing
Fix 3 is the only one that stands alone against upstream
main, and it went there as NousResearch#78819.Fixes 1 and 2 stay fork-only, deliberately:
metadata.hermes.pinned/metadata.hermes.locked, which upstream does not define — that frontmatter convention is our carry (d840a282e) plus the third-party PR feat(skill_manager): honor metadata.hermes.locked frontmatter flag NousResearch/hermes-agent#23715, still open. Upstream's own opt-out is the usage-record pin, and every autonomous caller ofarchive_skillthere already pre-checks it (agent/curator.py:331,agent/learning_mutations.py:134,_background_curator_guard), so there is no upstream bug to fix.terminalinto the fork on purpose.Tests
TDD throughout — tests written first and confirmed failing against the old behavior.
tests/agent/test_curator.py,tests/tools/test_skill_usage.py,tests/tools/test_skills_tool.py: 86 passed (test_curator_slot_is_canonical_aux_taskdeselected — it fails identically on pristineupstream/mainfor a missingpython-multipartdependency, not a regression from this branch).